Re: [PATCH v4 1/6] phy: cdns-dphy: Prepare for Rx support

From: Laurent Pinchart
Date: Sun Aug 22 2021 - 21:25:22 EST

Next message: Brad Larson: "Re: [PATCH v2 03/13] spi: dw: Add support for Pensando Elba SoC SPI"
Previous message: Brad Larson: "Re: [PATCH v2 01/13] gpio: Add Elba SoC gpio driver for spi cs control"
In reply to: Pratyush Yadav: "[PATCH v4 1/6] phy: cdns-dphy: Prepare for Rx support"
Next in thread: Pratyush Yadav: "[PATCH v4 3/6] phy: dt-bindings: Convert Cadence DPHY binding to YAML"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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.

> Signed-off-by: Pratyush Yadav <p.yadav@xxxxxx>
>
> ---
>
> 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.

> 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

Next message: Brad Larson: "Re: [PATCH v2 03/13] spi: dw: Add support for Pensando Elba SoC SPI"
Previous message: Brad Larson: "Re: [PATCH v2 01/13] gpio: Add Elba SoC gpio driver for spi cs control"
In reply to: Pratyush Yadav: "[PATCH v4 1/6] phy: cdns-dphy: Prepare for Rx support"
Next in thread: Pratyush Yadav: "[PATCH v4 3/6] phy: dt-bindings: Convert Cadence DPHY binding to YAML"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]