Re: [PATCH v3 2/7] phy: cdns-dphy: Prepare for Rx support

Vinod Koul
2021-08-06T10:18:28Z

On 25-06-21, 00:11, Pratyush Yadav wrote:
> The Rx programming sequence differs from the Tx programming sequence.
> Currently only Tx mode is supported. Move all the Tx related parts into
> a set of Tx-specific hooks that are then called by the main PHY
> framework hooks. This way when Rx support is added all that is needed to
> be done is to plug in the Rx hooks.
>
> 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 power_on()
> hook.
>
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>
> (no changes since v1)
>
> drivers/phy/cadence/cdns-dphy.c | 140 ++++++++++++++++++++++++--------
> 1 file changed, 104 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
> index ba042e39cfaf..8656f2102a91 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,12 +91,18 @@ 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;
> struct phy *phy;
> };
>
> +struct cdns_dphy_driver_data {
> + const struct cdns_dphy_ops *tx;
> + const struct cdns_dphy_ops *rx;
> +};
> +
> static int cdns_dsi_get_dphy_pll_cfg(struct cdns_dphy *dphy,
> struct cdns_dphy_cfg *cfg,
> struct phy_configure_opts_mipi_dphy *opts,
> @@ -199,20 +210,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 +232,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 +268,21 @@ 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 };
> +
> + if (submode != PHY_MIPI_DPHY_SUBMODE_TX)
> + return -EINVAL;
> +
> + 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 +294,77 @@ 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;
> }
>
> +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,
> +};
> +
> +/*
> + * 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_driver_data ref_dphy_ops = {
> + .tx = &tx_ref_dphy_ops,
> +};
> +
> +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;
> +}

why do you need a dphy specific power_on/off only to call internal
ops..? If there is no additional logic lets drop this and continue the
phy ops for this

--
~Vinod