Messages in this thread | | | Date | Fri, 27 Aug 2021 00:44:51 +0530 | From | Pratyush Yadav <> | Subject | Re: [PATCH v4 1/6] phy: cdns-dphy: Prepare for Rx support |
| |
On 23/08/21 04:25AM, Laurent Pinchart wrote: > Hi Pratyush, > > Thank you for the patch. > > On Sat, Aug 21, 2021 at 12:33:41AM +0530, Pratyush Yadav wrote: > > The Rx programming sequence differs from the Tx programming sequence. > > Currently only Tx mode is supported. For example, the power on and off, > > validation, and configuration procedures are all different between Rx > > and Tx DPHYs. Currently they are only written from a Tx point of view > > and they won't work with an Rx DPHY. Move them to cdns_dphy_ops so they > > can be defined by the implementation, accommodating both Rx and Tx mode > > DPHYs. > > > > The clocks "psm" and "pll_ref" are not used by the Rx path so make them > > optional in the probe and then check if they exist in the Tx power_on() > > hook. > > I think it would be better to check them at probe time.
Right. Now that we no longer switch mode dynamically, this can be done.
> > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > > > --- > > > > Changes in v4: > > - Instead of having both Rx and Tx modes in the same driver data, keep > > them separate since the op selection is based on compatible now. For > > that reason, the cdns_dphy_driver_data struct is no longer needed. > > - Rename ref_dphy_ops to tx_ref_dphy_ops to clarify their purpose. > > - Drop submode checks in validate() hook. > > > > drivers/phy/cadence/cdns-dphy.c | 123 ++++++++++++++++++++++---------- > > 1 file changed, 87 insertions(+), 36 deletions(-) > > > > diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c > > index ba042e39cfaf..0a169d649216 100644 > > --- a/drivers/phy/cadence/cdns-dphy.c > > +++ b/drivers/phy/cadence/cdns-dphy.c > > @@ -75,6 +75,11 @@ struct cdns_dphy; > > struct cdns_dphy_ops { > > int (*probe)(struct cdns_dphy *dphy); > > void (*remove)(struct cdns_dphy *dphy); > > + int (*power_on)(struct cdns_dphy *dphy); > > + int (*power_off)(struct cdns_dphy *dphy); > > + int (*validate)(struct cdns_dphy *dphy, enum phy_mode mode, int submode, > > + union phy_configure_opts *opts); > > + int (*configure)(struct cdns_dphy *dphy, union phy_configure_opts *opts); > > void (*set_psm_div)(struct cdns_dphy *dphy, u8 div); > > void (*set_clk_lane_cfg)(struct cdns_dphy *dphy, > > enum cdns_dphy_clk_lane_cfg cfg); > > @@ -86,6 +91,7 @@ struct cdns_dphy_ops { > > struct cdns_dphy { > > struct cdns_dphy_cfg cfg; > > void __iomem *regs; > > + struct device *dev; > > struct clk *psm_clk; > > struct clk *pll_ref_clk; > > const struct cdns_dphy_ops *ops; > > @@ -199,20 +205,9 @@ static void cdns_dphy_ref_set_psm_div(struct cdns_dphy *dphy, u8 div) > > dphy->regs + DPHY_PSM_CFG); > > } > > > > -/* > > - * This is the reference implementation of DPHY hooks. Specific integration of > > - * this IP may have to re-implement some of them depending on how they decided > > - * to wire things in the SoC. > > - */ > > -static const struct cdns_dphy_ops ref_dphy_ops = { > > - .get_wakeup_time_ns = cdns_dphy_ref_get_wakeup_time_ns, > > - .set_pll_cfg = cdns_dphy_ref_set_pll_cfg, > > - .set_psm_div = cdns_dphy_ref_set_psm_div, > > -}; > > - > > -static int cdns_dphy_config_from_opts(struct phy *phy, > > - struct phy_configure_opts_mipi_dphy *opts, > > - struct cdns_dphy_cfg *cfg) > > +static int cdns_dphy_tx_config_from_opts(struct phy *phy, > > + struct phy_configure_opts_mipi_dphy *opts, > > + struct cdns_dphy_cfg *cfg) > > { > > struct cdns_dphy *dphy = phy_get_drvdata(phy); > > unsigned int dsi_hfp_ext = 0; > > @@ -232,24 +227,13 @@ static int cdns_dphy_config_from_opts(struct phy *phy, > > return 0; > > } > > > > -static int cdns_dphy_validate(struct phy *phy, enum phy_mode mode, int submode, > > - union phy_configure_opts *opts) > > +static int cdns_dphy_tx_configure(struct cdns_dphy *dphy, > > + union phy_configure_opts *opts) > > { > > struct cdns_dphy_cfg cfg = { 0 }; > > - > > - if (mode != PHY_MODE_MIPI_DPHY) > > - return -EINVAL; > > - > > - return cdns_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg); > > -} > > - > > -static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts) > > -{ > > - struct cdns_dphy *dphy = phy_get_drvdata(phy); > > - struct cdns_dphy_cfg cfg = { 0 }; > > int ret; > > > > - ret = cdns_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg); > > + ret = cdns_dphy_tx_config_from_opts(dphy->phy, &opts->mipi_dphy, &cfg); > > if (ret) > > return ret; > > > > @@ -279,9 +263,18 @@ static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts) > > return 0; > > } > > > > -static int cdns_dphy_power_on(struct phy *phy) > > +static int cdns_dphy_tx_validate(struct cdns_dphy *dphy, enum phy_mode mode, > > + int submode, union phy_configure_opts *opts) > > { > > - struct cdns_dphy *dphy = phy_get_drvdata(phy); > > + struct cdns_dphy_cfg cfg = { 0 }; > > + > > + return cdns_dphy_tx_config_from_opts(dphy->phy, &opts->mipi_dphy, &cfg); > > +} > > + > > +static int cdns_dphy_tx_power_on(struct cdns_dphy *dphy) > > +{ > > + if (!dphy->psm_clk || !dphy->pll_ref_clk) > > + return -EINVAL; > > > > clk_prepare_enable(dphy->psm_clk); > > clk_prepare_enable(dphy->pll_ref_clk); > > @@ -293,16 +286,73 @@ static int cdns_dphy_power_on(struct phy *phy) > > return 0; > > } > > > > -static int cdns_dphy_power_off(struct phy *phy) > > +static int cdns_dphy_tx_power_off(struct cdns_dphy *dphy) > > { > > - struct cdns_dphy *dphy = phy_get_drvdata(phy); > > - > > clk_disable_unprepare(dphy->pll_ref_clk); > > clk_disable_unprepare(dphy->psm_clk); > > > > return 0; > > } > > > > +/* > > + * This is the reference implementation of DPHY hooks. Specific integration of > > + * this IP may have to re-implement some of them depending on how they decided > > + * to wire things in the SoC. > > + */ > > +static const struct cdns_dphy_ops tx_ref_dphy_ops = { > > + .power_on = cdns_dphy_tx_power_on, > > + .power_off = cdns_dphy_tx_power_off, > > + .validate = cdns_dphy_tx_validate, > > + .configure = cdns_dphy_tx_configure, > > + .get_wakeup_time_ns = cdns_dphy_ref_get_wakeup_time_ns, > > + .set_pll_cfg = cdns_dphy_ref_set_pll_cfg, > > + .set_psm_div = cdns_dphy_ref_set_psm_div, > > +}; > > + > > +static int cdns_dphy_validate(struct phy *phy, enum phy_mode mode, int submode, > > + union phy_configure_opts *opts) > > +{ > > + struct cdns_dphy *dphy = phy_get_drvdata(phy); > > + > > + if (mode != PHY_MODE_MIPI_DPHY) > > + return -EINVAL; > > + > > + if (dphy->ops->validate) > > + return dphy->ops->validate(dphy, mode, submode, opts); > > + > > + return 0; > > +} > > + > > +static int cdns_dphy_power_on(struct phy *phy) > > +{ > > + struct cdns_dphy *dphy = phy_get_drvdata(phy); > > + > > + if (dphy->ops->power_on) > > + return dphy->ops->power_on(dphy); > > + > > + return 0; > > +} > > + > > +static int cdns_dphy_power_off(struct phy *phy) > > +{ > > + struct cdns_dphy *dphy = phy_get_drvdata(phy); > > + > > + if (dphy->ops->power_off) > > + return dphy->ops->power_off(dphy); > > + > > + return 0; > > +} > > + > > +static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts) > > +{ > > + struct cdns_dphy *dphy = phy_get_drvdata(phy); > > + > > + if (dphy->ops->configure) > > + return dphy->ops->configure(dphy, opts); > > + > > + return 0; > > +} > > + > > Given that all of these are essentially pass-through operations, how > about getting rid of the indirection ? I would create a new structure: > > struct cdns_dphy_info { > const struct phy_ops *phy_ops; > const struct cdns_dphy_ops *dphy_ops; > }; > > and reference it in cdns_dphy_of_match. The cdns_dphy structure would > then store a pointer to cdns_dphy_info. That way you won't have to > extend cdns_dphy_ops, which could possibly be renamed to > cdns_dphy_tx_ops as you don't use those operations for rx.
Ok, sounds like a good idea.
> > > static const struct phy_ops cdns_dphy_ops = { > > .configure = cdns_dphy_configure, > > .validate = cdns_dphy_validate, > > @@ -320,6 +370,7 @@ static int cdns_dphy_probe(struct platform_device *pdev) > > if (!dphy) > > return -ENOMEM; > > dev_set_drvdata(&pdev->dev, dphy); > > + dphy->dev = &pdev->dev; > > > > dphy->ops = of_device_get_match_data(&pdev->dev); > > if (!dphy->ops) > > @@ -329,11 +380,11 @@ static int cdns_dphy_probe(struct platform_device *pdev) > > if (IS_ERR(dphy->regs)) > > return PTR_ERR(dphy->regs); > > > > - dphy->psm_clk = devm_clk_get(&pdev->dev, "psm"); > > + dphy->psm_clk = devm_clk_get_optional(dphy->dev, "psm"); > > if (IS_ERR(dphy->psm_clk)) > > return PTR_ERR(dphy->psm_clk); > > > > - dphy->pll_ref_clk = devm_clk_get(&pdev->dev, "pll_ref"); > > + dphy->pll_ref_clk = devm_clk_get_optional(dphy->dev, "pll_ref"); > > if (IS_ERR(dphy->pll_ref_clk)) > > return PTR_ERR(dphy->pll_ref_clk); > > > > @@ -369,7 +420,7 @@ static int cdns_dphy_remove(struct platform_device *pdev) > > } > > > > static const struct of_device_id cdns_dphy_of_match[] = { > > - { .compatible = "cdns,dphy", .data = &ref_dphy_ops }, > > + { .compatible = "cdns,dphy", .data = &tx_ref_dphy_ops }, > > { /* sentinel */ }, > > }; > > MODULE_DEVICE_TABLE(of, cdns_dphy_of_match); > > -- > Regards, > > Laurent Pinchart
-- Regards, Pratyush Yadav Texas Instruments Inc.
| |