| Message ID | 20220128072642.29188-1-a-govindraju@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] phy: cadence: Sierra: Add support for skipping configuration | expand |
On Fri, Jan 28, 2022 at 12:56:41PM +0530, Aswath Govindraju wrote: > In some cases, a single SerDes instance can be shared between two different > processors, each using a separate link. In these cases, the SerDes > configuration is done in an earlier boot stage. Therefore, add support to > skip reconfiguring, if it is was already configured beforehand. > > Signed-off-by: Aswath Govindraju <a-govindraju@ti.com> > --- > > Changes since v1: > - Removed redundant braces > - Corrected the logic for skipping multilink configuration > - Corrected the order in failure path > Thanks! Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com> regards, dan carpenter
On 28-01-22, 12:56, Aswath Govindraju wrote: > In some cases, a single SerDes instance can be shared between two different > processors, each using a separate link. In these cases, the SerDes > configuration is done in an earlier boot stage. Therefore, add support to > skip reconfiguring, if it is was already configured beforehand. > > Signed-off-by: Aswath Govindraju <a-govindraju@ti.com> > --- > > Changes since v1: > - Removed redundant braces > - Corrected the logic for skipping multilink configuration > - Corrected the order in failure path > > drivers/phy/cadence/phy-cadence-sierra.c | 82 ++++++++++++++++-------- > 1 file changed, 57 insertions(+), 25 deletions(-) > > diff --git a/drivers/phy/cadence/phy-cadence-sierra.c b/drivers/phy/cadence/phy-cadence-sierra.c > index e265647e29a2..6b917f7bddbe 100644 > --- a/drivers/phy/cadence/phy-cadence-sierra.c > +++ b/drivers/phy/cadence/phy-cadence-sierra.c > @@ -370,6 +370,7 @@ struct cdns_sierra_phy { > int nsubnodes; > u32 num_lanes; > bool autoconf; > + int already_configured; where is this set and is set based on..? > struct clk_onecell_data clk_data; > struct clk *output_clks[CDNS_SIERRA_OUTPUT_CLOCKS]; > }; > @@ -517,7 +518,7 @@ static int cdns_sierra_phy_init(struct phy *gphy) > int i, j; > > /* Initialise the PHY registers, unless auto configured */ > - if (phy->autoconf || phy->nsubnodes > 1) > + if (phy->autoconf || phy->already_configured || phy->nsubnodes > 1) > return 0; > > clk_set_rate(phy->input_clks[CMN_REFCLK_DIG_DIV], 25000000); > @@ -646,6 +647,18 @@ static const struct phy_ops ops = { > .owner = THIS_MODULE, > }; > > +static int cdns_sierra_noop_phy_on(struct phy *gphy) > +{ > + usleep_range(5000, 10000); > + > + return 0; > +} > + > +static const struct phy_ops noop_ops = { > + .power_on = cdns_sierra_noop_phy_on, > + .owner = THIS_MODULE, > +}; > + > static u8 cdns_sierra_pll_mux_get_parent(struct clk_hw *hw) > { > struct cdns_sierra_pll_mux *mux = to_cdns_sierra_pll_mux(hw); > @@ -1118,13 +1131,6 @@ static int cdns_sierra_phy_get_clocks(struct cdns_sierra_phy *sp, > struct clk *clk; > int ret; > > - clk = devm_clk_get_optional(dev, "phy_clk"); > - if (IS_ERR(clk)) { > - dev_err(dev, "failed to get clock phy_clk\n"); > - return PTR_ERR(clk); > - } > - sp->input_clks[PHY_CLK] = clk; > - > clk = devm_clk_get_optional(dev, "cmn_refclk_dig_div"); > if (IS_ERR(clk)) { > dev_err(dev, "cmn_refclk_dig_div clock not found\n"); > @@ -1160,17 +1166,33 @@ static int cdns_sierra_phy_get_clocks(struct cdns_sierra_phy *sp, > return 0; > } > > -static int cdns_sierra_phy_enable_clocks(struct cdns_sierra_phy *sp) > +static int cdns_sierra_phy_clk(struct cdns_sierra_phy *sp) > { > + struct device *dev = sp->dev; > + struct clk *clk; > int ret; > > + clk = devm_clk_get_optional(dev, "phy_clk"); > + if (IS_ERR(clk)) { > + dev_err(dev, "failed to get clock phy_clk\n"); > + return PTR_ERR(clk); > + } > + sp->input_clks[PHY_CLK] = clk; > + > ret = clk_prepare_enable(sp->input_clks[PHY_CLK]); > if (ret) > return ret; > > + return 0; > +} > + > +static int cdns_sierra_phy_enable_clocks(struct cdns_sierra_phy *sp) > +{ > + int ret; > + > ret = clk_prepare_enable(sp->output_clks[CDNS_SIERRA_PLL_CMNLC]); > if (ret) > - goto err_pll_cmnlc; > + return ret; > > ret = clk_prepare_enable(sp->output_clks[CDNS_SIERRA_PLL_CMNLC1]); > if (ret) > @@ -1181,9 +1203,6 @@ static int cdns_sierra_phy_enable_clocks(struct cdns_sierra_phy *sp) > err_pll_cmnlc1: > clk_disable_unprepare(sp->output_clks[CDNS_SIERRA_PLL_CMNLC]); > > -err_pll_cmnlc: > - clk_disable_unprepare(sp->input_clks[PHY_CLK]); > - > return ret; > } > > @@ -1191,7 +1210,8 @@ static void cdns_sierra_phy_disable_clocks(struct cdns_sierra_phy *sp) > { > clk_disable_unprepare(sp->output_clks[CDNS_SIERRA_PLL_CMNLC1]); > clk_disable_unprepare(sp->output_clks[CDNS_SIERRA_PLL_CMNLC]); > - clk_disable_unprepare(sp->input_clks[PHY_CLK]); > + if (!sp->already_configured) > + clk_disable_unprepare(sp->input_clks[PHY_CLK]); > } > > static int cdns_sierra_phy_get_resets(struct cdns_sierra_phy *sp, > @@ -1382,22 +1402,30 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev) > if (ret) > return ret; > > - ret = cdns_sierra_phy_get_resets(sp, dev); > - if (ret) > - goto unregister_clk; > - > ret = cdns_sierra_phy_enable_clocks(sp); > if (ret) > goto unregister_clk; > > - /* Enable APB */ > - reset_control_deassert(sp->apb_rst); > + regmap_field_read(sp->pma_cmn_ready, &sp->already_configured); > + > + if (!sp->already_configured) { > + ret = cdns_sierra_phy_clk(sp); > + if (ret) > + goto clk_disable; > + > + ret = cdns_sierra_phy_get_resets(sp, dev); > + if (ret) > + goto clk_disable; > + > + /* Enable APB */ > + reset_control_deassert(sp->apb_rst); > + } > > /* Check that PHY is present */ > regmap_field_read(sp->macro_id_type, &id_value); > if (sp->init_data->id_value != id_value) { > ret = -EINVAL; > - goto clk_disable; > + goto ctrl_assert; > } > > sp->autoconf = of_property_read_bool(dn, "cdns,autoconf"); > @@ -1433,8 +1461,10 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev) > > sp->num_lanes += sp->phys[node].num_lanes; > > - gphy = devm_phy_create(dev, child, &ops); > - > + if (!sp->already_configured) > + gphy = devm_phy_create(dev, child, &ops); > + else > + gphy = devm_phy_create(dev, child, &noop_ops); > if (IS_ERR(gphy)) { > ret = PTR_ERR(gphy); > of_node_put(child); > @@ -1455,7 +1485,7 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev) > } > > /* If more than one subnode, configure the PHY as multilink */ > - if (!sp->autoconf && sp->nsubnodes > 1) { > + if (!sp->already_configured && !sp->autoconf && sp->nsubnodes > 1) { > ret = cdns_sierra_phy_configure_multilink(sp); > if (ret) > goto put_control; > @@ -1473,9 +1503,11 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev) > put_control: > while (--node >= 0) > reset_control_put(sp->phys[node].lnk_rst); > +ctrl_assert: > + if (!sp->already_configured) > + reset_control_assert(sp->apb_rst); > clk_disable: > cdns_sierra_phy_disable_clocks(sp); > - reset_control_assert(sp->apb_rst); > unregister_clk: > cdns_sierra_clk_unregister(sp); > return ret; > -- > 2.17.1
Hi Vinod, On 02/02/22 10:25 am, Vinod Koul wrote: > On 28-01-22, 12:56, Aswath Govindraju wrote: >> In some cases, a single SerDes instance can be shared between two different >> processors, each using a separate link. In these cases, the SerDes >> configuration is done in an earlier boot stage. Therefore, add support to >> skip reconfiguring, if it is was already configured beforehand. >> >> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com> >> --- >> >> Changes since v1: >> - Removed redundant braces >> - Corrected the logic for skipping multilink configuration >> - Corrected the order in failure path >> >> drivers/phy/cadence/phy-cadence-sierra.c | 82 ++++++++++++++++-------- >> 1 file changed, 57 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/phy/cadence/phy-cadence-sierra.c b/drivers/phy/cadence/phy-cadence-sierra.c >> index e265647e29a2..6b917f7bddbe 100644 >> --- a/drivers/phy/cadence/phy-cadence-sierra.c >> +++ b/drivers/phy/cadence/phy-cadence-sierra.c >> @@ -370,6 +370,7 @@ struct cdns_sierra_phy { >> int nsubnodes; >> u32 num_lanes; >> bool autoconf; >> + int already_configured; > > where is this set and is set based on..? > It is being set at [1] based on reading phy_cmn_ready bit field. This gets set when the configuration is done and the PLLs are locked. >> struct clk_onecell_data clk_data; >> struct clk *output_clks[CDNS_SIERRA_OUTPUT_CLOCKS]; >> }; >> @@ -517,7 +518,7 @@ static int cdns_sierra_phy_init(struct phy *gphy) >> int i, j; >> >> /* Initialise the PHY registers, unless auto configured */ >> - if (phy->autoconf || phy->nsubnodes > 1) >> + if (phy->autoconf || phy->already_configured || phy->nsubnodes > 1) >> return 0; >> >> clk_set_rate(phy->input_clks[CMN_REFCLK_DIG_DIV], 25000000); >> @@ -646,6 +647,18 @@ static const struct phy_ops ops = { >> .owner = THIS_MODULE, >> }; >> >> +static int cdns_sierra_noop_phy_on(struct phy *gphy) >> +{ >> + usleep_range(5000, 10000); >> + >> + return 0; >> +} >> + >> +static const struct phy_ops noop_ops = { >> + .power_on = cdns_sierra_noop_phy_on, >> + .owner = THIS_MODULE, >> +}; >> + >> static u8 cdns_sierra_pll_mux_get_parent(struct clk_hw *hw) >> { >> struct cdns_sierra_pll_mux *mux = to_cdns_sierra_pll_mux(hw); >> @@ -1118,13 +1131,6 @@ static int cdns_sierra_phy_get_clocks(struct cdns_sierra_phy *sp, >> struct clk *clk; >> int ret; >> >> - clk = devm_clk_get_optional(dev, "phy_clk"); >> - if (IS_ERR(clk)) { >> - dev_err(dev, "failed to get clock phy_clk\n"); >> - return PTR_ERR(clk); >> - } >> - sp->input_clks[PHY_CLK] = clk; >> - >> clk = devm_clk_get_optional(dev, "cmn_refclk_dig_div"); >> if (IS_ERR(clk)) { >> dev_err(dev, "cmn_refclk_dig_div clock not found\n"); >> @@ -1160,17 +1166,33 @@ static int cdns_sierra_phy_get_clocks(struct cdns_sierra_phy *sp, >> return 0; >> } >> >> -static int cdns_sierra_phy_enable_clocks(struct cdns_sierra_phy *sp) >> +static int cdns_sierra_phy_clk(struct cdns_sierra_phy *sp) >> { >> + struct device *dev = sp->dev; >> + struct clk *clk; >> int ret; >> >> + clk = devm_clk_get_optional(dev, "phy_clk"); >> + if (IS_ERR(clk)) { >> + dev_err(dev, "failed to get clock phy_clk\n"); >> + return PTR_ERR(clk); >> + } >> + sp->input_clks[PHY_CLK] = clk; >> + >> ret = clk_prepare_enable(sp->input_clks[PHY_CLK]); >> if (ret) >> return ret; >> >> + return 0; >> +} >> + >> +static int cdns_sierra_phy_enable_clocks(struct cdns_sierra_phy *sp) >> +{ >> + int ret; >> + >> ret = clk_prepare_enable(sp->output_clks[CDNS_SIERRA_PLL_CMNLC]); >> if (ret) >> - goto err_pll_cmnlc; >> + return ret; >> >> ret = clk_prepare_enable(sp->output_clks[CDNS_SIERRA_PLL_CMNLC1]); >> if (ret) >> @@ -1181,9 +1203,6 @@ static int cdns_sierra_phy_enable_clocks(struct cdns_sierra_phy *sp) >> err_pll_cmnlc1: >> clk_disable_unprepare(sp->output_clks[CDNS_SIERRA_PLL_CMNLC]); >> >> -err_pll_cmnlc: >> - clk_disable_unprepare(sp->input_clks[PHY_CLK]); >> - >> return ret; >> } >> >> @@ -1191,7 +1210,8 @@ static void cdns_sierra_phy_disable_clocks(struct cdns_sierra_phy *sp) >> { >> clk_disable_unprepare(sp->output_clks[CDNS_SIERRA_PLL_CMNLC1]); >> clk_disable_unprepare(sp->output_clks[CDNS_SIERRA_PLL_CMNLC]); >> - clk_disable_unprepare(sp->input_clks[PHY_CLK]); >> + if (!sp->already_configured) >> + clk_disable_unprepare(sp->input_clks[PHY_CLK]); >> } >> >> static int cdns_sierra_phy_get_resets(struct cdns_sierra_phy *sp, >> @@ -1382,22 +1402,30 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev) >> if (ret) >> return ret; >> >> - ret = cdns_sierra_phy_get_resets(sp, dev); >> - if (ret) >> - goto unregister_clk; >> - >> ret = cdns_sierra_phy_enable_clocks(sp); >> if (ret) >> goto unregister_clk; >> >> - /* Enable APB */ >> - reset_control_deassert(sp->apb_rst); >> + regmap_field_read(sp->pma_cmn_ready, &sp->already_configured); >> + [1] Thanks, Aswath >> + if (!sp->already_configured) { >> + ret = cdns_sierra_phy_clk(sp); >> + if (ret) >> + goto clk_disable; >> + >> + ret = cdns_sierra_phy_get_resets(sp, dev); >> + if (ret) >> + goto clk_disable; >> + >> + /* Enable APB */ >> + reset_control_deassert(sp->apb_rst); >> + } >> >> /* Check that PHY is present */ >> regmap_field_read(sp->macro_id_type, &id_value); >> if (sp->init_data->id_value != id_value) { >> ret = -EINVAL; >> - goto clk_disable; >> + goto ctrl_assert; >> } >> >> sp->autoconf = of_property_read_bool(dn, "cdns,autoconf"); >> @@ -1433,8 +1461,10 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev) >> >> sp->num_lanes += sp->phys[node].num_lanes; >> >> - gphy = devm_phy_create(dev, child, &ops); >> - >> + if (!sp->already_configured) >> + gphy = devm_phy_create(dev, child, &ops); >> + else >> + gphy = devm_phy_create(dev, child, &noop_ops); >> if (IS_ERR(gphy)) { >> ret = PTR_ERR(gphy); >> of_node_put(child); >> @@ -1455,7 +1485,7 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev) >> } >> >> /* If more than one subnode, configure the PHY as multilink */ >> - if (!sp->autoconf && sp->nsubnodes > 1) { >> + if (!sp->already_configured && !sp->autoconf && sp->nsubnodes > 1) { >> ret = cdns_sierra_phy_configure_multilink(sp); >> if (ret) >> goto put_control; >> @@ -1473,9 +1503,11 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev) >> put_control: >> while (--node >= 0) >> reset_control_put(sp->phys[node].lnk_rst); >> +ctrl_assert: >> + if (!sp->already_configured) >> + reset_control_assert(sp->apb_rst); >> clk_disable: >> cdns_sierra_phy_disable_clocks(sp); >> - reset_control_assert(sp->apb_rst); >> unregister_clk: >> cdns_sierra_clk_unregister(sp); >> return ret; >> -- >> 2.17.1 >
On 28-01-22, 12:56, Aswath Govindraju wrote: > In some cases, a single SerDes instance can be shared between two different > processors, each using a separate link. In these cases, the SerDes > configuration is done in an earlier boot stage. Therefore, add support to > skip reconfiguring, if it is was already configured beforehand. This fails to apply, pls rebase and resend > > Signed-off-by: Aswath Govindraju <a-govindraju@ti.com> > --- > > Changes since v1: > - Removed redundant braces > - Corrected the logic for skipping multilink configuration > - Corrected the order in failure path > > drivers/phy/cadence/phy-cadence-sierra.c | 82 ++++++++++++++++-------- > 1 file changed, 57 insertions(+), 25 deletions(-) > > diff --git a/drivers/phy/cadence/phy-cadence-sierra.c b/drivers/phy/cadence/phy-cadence-sierra.c > index e265647e29a2..6b917f7bddbe 100644 > --- a/drivers/phy/cadence/phy-cadence-sierra.c > +++ b/drivers/phy/cadence/phy-cadence-sierra.c > @@ -370,6 +370,7 @@ struct cdns_sierra_phy { > int nsubnodes; > u32 num_lanes; > bool autoconf; > + int already_configured; > struct clk_onecell_data clk_data; > struct clk *output_clks[CDNS_SIERRA_OUTPUT_CLOCKS]; > }; > @@ -517,7 +518,7 @@ static int cdns_sierra_phy_init(struct phy *gphy) > int i, j; > > /* Initialise the PHY registers, unless auto configured */ > - if (phy->autoconf || phy->nsubnodes > 1) > + if (phy->autoconf || phy->already_configured || phy->nsubnodes > 1) > return 0; > > clk_set_rate(phy->input_clks[CMN_REFCLK_DIG_DIV], 25000000); > @@ -646,6 +647,18 @@ static const struct phy_ops ops = { > .owner = THIS_MODULE, > }; > > +static int cdns_sierra_noop_phy_on(struct phy *gphy) > +{ > + usleep_range(5000, 10000); > + > + return 0; > +} > + > +static const struct phy_ops noop_ops = { > + .power_on = cdns_sierra_noop_phy_on, > + .owner = THIS_MODULE, > +}; > + > static u8 cdns_sierra_pll_mux_get_parent(struct clk_hw *hw) > { > struct cdns_sierra_pll_mux *mux = to_cdns_sierra_pll_mux(hw); > @@ -1118,13 +1131,6 @@ static int cdns_sierra_phy_get_clocks(struct cdns_sierra_phy *sp, > struct clk *clk; > int ret; > > - clk = devm_clk_get_optional(dev, "phy_clk"); > - if (IS_ERR(clk)) { > - dev_err(dev, "failed to get clock phy_clk\n"); > - return PTR_ERR(clk); > - } > - sp->input_clks[PHY_CLK] = clk; > - > clk = devm_clk_get_optional(dev, "cmn_refclk_dig_div"); > if (IS_ERR(clk)) { > dev_err(dev, "cmn_refclk_dig_div clock not found\n"); > @@ -1160,17 +1166,33 @@ static int cdns_sierra_phy_get_clocks(struct cdns_sierra_phy *sp, > return 0; > } > > -static int cdns_sierra_phy_enable_clocks(struct cdns_sierra_phy *sp) > +static int cdns_sierra_phy_clk(struct cdns_sierra_phy *sp) > { > + struct device *dev = sp->dev; > + struct clk *clk; > int ret; > > + clk = devm_clk_get_optional(dev, "phy_clk"); > + if (IS_ERR(clk)) { > + dev_err(dev, "failed to get clock phy_clk\n"); > + return PTR_ERR(clk); > + } > + sp->input_clks[PHY_CLK] = clk; > + > ret = clk_prepare_enable(sp->input_clks[PHY_CLK]); > if (ret) > return ret; > > + return 0; > +} > + > +static int cdns_sierra_phy_enable_clocks(struct cdns_sierra_phy *sp) > +{ > + int ret; > + > ret = clk_prepare_enable(sp->output_clks[CDNS_SIERRA_PLL_CMNLC]); > if (ret) > - goto err_pll_cmnlc; > + return ret; > > ret = clk_prepare_enable(sp->output_clks[CDNS_SIERRA_PLL_CMNLC1]); > if (ret) > @@ -1181,9 +1203,6 @@ static int cdns_sierra_phy_enable_clocks(struct cdns_sierra_phy *sp) > err_pll_cmnlc1: > clk_disable_unprepare(sp->output_clks[CDNS_SIERRA_PLL_CMNLC]); > > -err_pll_cmnlc: > - clk_disable_unprepare(sp->input_clks[PHY_CLK]); > - > return ret; > } > > @@ -1191,7 +1210,8 @@ static void cdns_sierra_phy_disable_clocks(struct cdns_sierra_phy *sp) > { > clk_disable_unprepare(sp->output_clks[CDNS_SIERRA_PLL_CMNLC1]); > clk_disable_unprepare(sp->output_clks[CDNS_SIERRA_PLL_CMNLC]); > - clk_disable_unprepare(sp->input_clks[PHY_CLK]); > + if (!sp->already_configured) > + clk_disable_unprepare(sp->input_clks[PHY_CLK]); > } > > static int cdns_sierra_phy_get_resets(struct cdns_sierra_phy *sp, > @@ -1382,22 +1402,30 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev) > if (ret) > return ret; > > - ret = cdns_sierra_phy_get_resets(sp, dev); > - if (ret) > - goto unregister_clk; > - > ret = cdns_sierra_phy_enable_clocks(sp); > if (ret) > goto unregister_clk; > > - /* Enable APB */ > - reset_control_deassert(sp->apb_rst); > + regmap_field_read(sp->pma_cmn_ready, &sp->already_configured); > + > + if (!sp->already_configured) { > + ret = cdns_sierra_phy_clk(sp); > + if (ret) > + goto clk_disable; > + > + ret = cdns_sierra_phy_get_resets(sp, dev); > + if (ret) > + goto clk_disable; > + > + /* Enable APB */ > + reset_control_deassert(sp->apb_rst); > + } > > /* Check that PHY is present */ > regmap_field_read(sp->macro_id_type, &id_value); > if (sp->init_data->id_value != id_value) { > ret = -EINVAL; > - goto clk_disable; > + goto ctrl_assert; > } > > sp->autoconf = of_property_read_bool(dn, "cdns,autoconf"); > @@ -1433,8 +1461,10 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev) > > sp->num_lanes += sp->phys[node].num_lanes; > > - gphy = devm_phy_create(dev, child, &ops); > - > + if (!sp->already_configured) > + gphy = devm_phy_create(dev, child, &ops); > + else > + gphy = devm_phy_create(dev, child, &noop_ops); > if (IS_ERR(gphy)) { > ret = PTR_ERR(gphy); > of_node_put(child); > @@ -1455,7 +1485,7 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev) > } > > /* If more than one subnode, configure the PHY as multilink */ > - if (!sp->autoconf && sp->nsubnodes > 1) { > + if (!sp->already_configured && !sp->autoconf && sp->nsubnodes > 1) { > ret = cdns_sierra_phy_configure_multilink(sp); > if (ret) > goto put_control; > @@ -1473,9 +1503,11 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev) > put_control: > while (--node >= 0) > reset_control_put(sp->phys[node].lnk_rst); > +ctrl_assert: > + if (!sp->already_configured) > + reset_control_assert(sp->apb_rst); > clk_disable: > cdns_sierra_phy_disable_clocks(sp); > - reset_control_assert(sp->apb_rst); > unregister_clk: > cdns_sierra_clk_unregister(sp); > return ret; > -- > 2.17.1
Hi Vinod, On 02/02/22 7:53 pm, Vinod Koul wrote: > On 28-01-22, 12:56, Aswath Govindraju wrote: >> In some cases, a single SerDes instance can be shared between two different >> processors, each using a separate link. In these cases, the SerDes >> configuration is done in an earlier boot stage. Therefore, add support to >> skip reconfiguring, if it is was already configured beforehand. > > This fails to apply, pls rebase and resend > On rebasing, I am seeing no difference in the patch and I am able to apply it on top of linux-next/master commit 6abab1b81b65. May I know if there is any other branch that I would need to rebase this patch on top of? Thanks, Aswath >> >> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com> >> --- >> >> Changes since v1: >> - Removed redundant braces >> - Corrected the logic for skipping multilink configuration >> - Corrected the order in failure path >> >> drivers/phy/cadence/phy-cadence-sierra.c | 82 ++++++++++++++++-------- >> 1 file changed, 57 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/phy/cadence/phy-cadence-sierra.c b/drivers/phy/cadence/phy-cadence-sierra.c >> index e265647e29a2..6b917f7bddbe 100644 >> --- a/drivers/phy/cadence/phy-cadence-sierra.c >> +++ b/drivers/phy/cadence/phy-cadence-sierra.c >> @@ -370,6 +370,7 @@ struct cdns_sierra_phy { >> int nsubnodes; >> u32 num_lanes; >> bool autoconf; >> + int already_configured; >> struct clk_onecell_data clk_data; >> struct clk *output_clks[CDNS_SIERRA_OUTPUT_CLOCKS]; >> }; >> @@ -517,7 +518,7 @@ static int cdns_sierra_phy_init(struct phy *gphy) >> int i, j; >> >> /* Initialise the PHY registers, unless auto configured */ >> - if (phy->autoconf || phy->nsubnodes > 1) >> + if (phy->autoconf || phy->already_configured || phy->nsubnodes > 1) >> return 0; >> >> clk_set_rate(phy->input_clks[CMN_REFCLK_DIG_DIV], 25000000); >> @@ -646,6 +647,18 @@ static const struct phy_ops ops = { >> .owner = THIS_MODULE, >> }; >> >> +static int cdns_sierra_noop_phy_on(struct phy *gphy) >> +{ >> + usleep_range(5000, 10000); >> + >> + return 0; >> +} >> + >> +static const struct phy_ops noop_ops = { >> + .power_on = cdns_sierra_noop_phy_on, >> + .owner = THIS_MODULE, >> +}; >> + >> static u8 cdns_sierra_pll_mux_get_parent(struct clk_hw *hw) >> { >> struct cdns_sierra_pll_mux *mux = to_cdns_sierra_pll_mux(hw); >> @@ -1118,13 +1131,6 @@ static int cdns_sierra_phy_get_clocks(struct cdns_sierra_phy *sp, >> struct clk *clk; >> int ret; >> >> - clk = devm_clk_get_optional(dev, "phy_clk"); >> - if (IS_ERR(clk)) { >> - dev_err(dev, "failed to get clock phy_clk\n"); >> - return PTR_ERR(clk); >> - } >> - sp->input_clks[PHY_CLK] = clk; >> - >> clk = devm_clk_get_optional(dev, "cmn_refclk_dig_div"); >> if (IS_ERR(clk)) { >> dev_err(dev, "cmn_refclk_dig_div clock not found\n"); >> @@ -1160,17 +1166,33 @@ static int cdns_sierra_phy_get_clocks(struct cdns_sierra_phy *sp, >> return 0; >> } >> >> -static int cdns_sierra_phy_enable_clocks(struct cdns_sierra_phy *sp) >> +static int cdns_sierra_phy_clk(struct cdns_sierra_phy *sp) >> { >> + struct device *dev = sp->dev; >> + struct clk *clk; >> int ret; >> >> + clk = devm_clk_get_optional(dev, "phy_clk"); >> + if (IS_ERR(clk)) { >> + dev_err(dev, "failed to get clock phy_clk\n"); >> + return PTR_ERR(clk); >> + } >> + sp->input_clks[PHY_CLK] = clk; >> + >> ret = clk_prepare_enable(sp->input_clks[PHY_CLK]); >> if (ret) >> return ret; >> >> + return 0; >> +} >> + >> +static int cdns_sierra_phy_enable_clocks(struct cdns_sierra_phy *sp) >> +{ >> + int ret; >> + >> ret = clk_prepare_enable(sp->output_clks[CDNS_SIERRA_PLL_CMNLC]); >> if (ret) >> - goto err_pll_cmnlc; >> + return ret; >> >> ret = clk_prepare_enable(sp->output_clks[CDNS_SIERRA_PLL_CMNLC1]); >> if (ret) >> @@ -1181,9 +1203,6 @@ static int cdns_sierra_phy_enable_clocks(struct cdns_sierra_phy *sp) >> err_pll_cmnlc1: >> clk_disable_unprepare(sp->output_clks[CDNS_SIERRA_PLL_CMNLC]); >> >> -err_pll_cmnlc: >> - clk_disable_unprepare(sp->input_clks[PHY_CLK]); >> - >> return ret; >> } >> >> @@ -1191,7 +1210,8 @@ static void cdns_sierra_phy_disable_clocks(struct cdns_sierra_phy *sp) >> { >> clk_disable_unprepare(sp->output_clks[CDNS_SIERRA_PLL_CMNLC1]); >> clk_disable_unprepare(sp->output_clks[CDNS_SIERRA_PLL_CMNLC]); >> - clk_disable_unprepare(sp->input_clks[PHY_CLK]); >> + if (!sp->already_configured) >> + clk_disable_unprepare(sp->input_clks[PHY_CLK]); >> } >> >> static int cdns_sierra_phy_get_resets(struct cdns_sierra_phy *sp, >> @@ -1382,22 +1402,30 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev) >> if (ret) >> return ret; >> >> - ret = cdns_sierra_phy_get_resets(sp, dev); >> - if (ret) >> - goto unregister_clk; >> - >> ret = cdns_sierra_phy_enable_clocks(sp); >> if (ret) >> goto unregister_clk; >> >> - /* Enable APB */ >> - reset_control_deassert(sp->apb_rst); >> + regmap_field_read(sp->pma_cmn_ready, &sp->already_configured); >> + >> + if (!sp->already_configured) { >> + ret = cdns_sierra_phy_clk(sp); >> + if (ret) >> + goto clk_disable; >> + >> + ret = cdns_sierra_phy_get_resets(sp, dev); >> + if (ret) >> + goto clk_disable; >> + >> + /* Enable APB */ >> + reset_control_deassert(sp->apb_rst); >> + } >> >> /* Check that PHY is present */ >> regmap_field_read(sp->macro_id_type, &id_value); >> if (sp->init_data->id_value != id_value) { >> ret = -EINVAL; >> - goto clk_disable; >> + goto ctrl_assert; >> } >> >> sp->autoconf = of_property_read_bool(dn, "cdns,autoconf"); >> @@ -1433,8 +1461,10 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev) >> >> sp->num_lanes += sp->phys[node].num_lanes; >> >> - gphy = devm_phy_create(dev, child, &ops); >> - >> + if (!sp->already_configured) >> + gphy = devm_phy_create(dev, child, &ops); >> + else >> + gphy = devm_phy_create(dev, child, &noop_ops); >> if (IS_ERR(gphy)) { >> ret = PTR_ERR(gphy); >> of_node_put(child); >> @@ -1455,7 +1485,7 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev) >> } >> >> /* If more than one subnode, configure the PHY as multilink */ >> - if (!sp->autoconf && sp->nsubnodes > 1) { >> + if (!sp->already_configured && !sp->autoconf && sp->nsubnodes > 1) { >> ret = cdns_sierra_phy_configure_multilink(sp); >> if (ret) >> goto put_control; >> @@ -1473,9 +1503,11 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev) >> put_control: >> while (--node >= 0) >> reset_control_put(sp->phys[node].lnk_rst); >> +ctrl_assert: >> + if (!sp->already_configured) >> + reset_control_assert(sp->apb_rst); >> clk_disable: >> cdns_sierra_phy_disable_clocks(sp); >> - reset_control_assert(sp->apb_rst); >> unregister_clk: >> cdns_sierra_clk_unregister(sp); >> return ret; >> -- >> 2.17.1 >
On 02-02-22, 20:14, Aswath Govindraju wrote: > Hi Vinod, > > On 02/02/22 7:53 pm, Vinod Koul wrote: > > On 28-01-22, 12:56, Aswath Govindraju wrote: > >> In some cases, a single SerDes instance can be shared between two different > >> processors, each using a separate link. In these cases, the SerDes > >> configuration is done in an earlier boot stage. Therefore, add support to > >> skip reconfiguring, if it is was already configured beforehand. > > > > This fails to apply, pls rebase and resend > > > > On rebasing, I am seeing no difference in the patch and I am able to > apply it on top of linux-next/master commit 6abab1b81b65. May I know if > there is any other branch that I would need to rebase this patch on top of? It should be based on phy-next which is at 1f1b0c105b19ac0d90975e2569040da1216489b7 now
Hi Vinod, On 03/02/22 5:44 am, Vinod Koul wrote: > On 02-02-22, 20:14, Aswath Govindraju wrote: >> Hi Vinod, >> >> On 02/02/22 7:53 pm, Vinod Koul wrote: >>> On 28-01-22, 12:56, Aswath Govindraju wrote: >>>> In some cases, a single SerDes instance can be shared between two different >>>> processors, each using a separate link. In these cases, the SerDes >>>> configuration is done in an earlier boot stage. Therefore, add support to >>>> skip reconfiguring, if it is was already configured beforehand. >>> >>> This fails to apply, pls rebase and resend >>> >> >> On rebasing, I am seeing no difference in the patch and I am able to >> apply it on top of linux-next/master commit 6abab1b81b65. May I know if >> there is any other branch that I would need to rebase this patch on top of? > > It should be based on phy-next which is at > 1f1b0c105b19ac0d90975e2569040da1216489b7 now > I have posted a respin of this patch after rebasing it on top of phy-next. One aspect that is not clear to me is, phy-next branch does not have the following commit which is present in linux-next master, 29afbd769ca3 phy: cadence: Sierra: fix error handling bugs in probe() When the respin of this patch(v3) is merged with linux-next/master wouldn't it cause merge-conflicts? May I know how would this be handled? Link to v3: - https://patchwork.kernel.org/project/linux-phy/list/?series=610903 Thanks, Aswath
On 03-02-22, 11:25, Aswath Govindraju wrote: > Hi Vinod, > > On 03/02/22 5:44 am, Vinod Koul wrote: > > On 02-02-22, 20:14, Aswath Govindraju wrote: > >> Hi Vinod, > >> > >> On 02/02/22 7:53 pm, Vinod Koul wrote: > >>> On 28-01-22, 12:56, Aswath Govindraju wrote: > >>>> In some cases, a single SerDes instance can be shared between two different > >>>> processors, each using a separate link. In these cases, the SerDes > >>>> configuration is done in an earlier boot stage. Therefore, add support to > >>>> skip reconfiguring, if it is was already configured beforehand. > >>> > >>> This fails to apply, pls rebase and resend > >>> > >> > >> On rebasing, I am seeing no difference in the patch and I am able to > >> apply it on top of linux-next/master commit 6abab1b81b65. May I know if > >> there is any other branch that I would need to rebase this patch on top of? > > > > It should be based on phy-next which is at > > 1f1b0c105b19ac0d90975e2569040da1216489b7 now > > > > I have posted a respin of this patch after rebasing it on top of > phy-next. One aspect that is not clear to me is, phy-next branch does > not have the following commit which is present in linux-next master, > > 29afbd769ca3 phy: cadence: Sierra: fix error handling bugs in probe() This is in fixes > > When the respin of this patch(v3) is merged with linux-next/master > wouldn't it cause merge-conflicts? > > May I know how would this be handled? If need arises (we have a dependency) I would merge fixes into next and then apply patches. Cover letter of the patches should mention that
Hi Vinod, On 04/02/22 11:44 am, Vinod Koul wrote: > On 03-02-22, 11:25, Aswath Govindraju wrote: >> Hi Vinod, >> >> On 03/02/22 5:44 am, Vinod Koul wrote: >>> On 02-02-22, 20:14, Aswath Govindraju wrote: >>>> Hi Vinod, >>>> >>>> On 02/02/22 7:53 pm, Vinod Koul wrote: >>>>> On 28-01-22, 12:56, Aswath Govindraju wrote: >>>>>> In some cases, a single SerDes instance can be shared between two different >>>>>> processors, each using a separate link. In these cases, the SerDes >>>>>> configuration is done in an earlier boot stage. Therefore, add support to >>>>>> skip reconfiguring, if it is was already configured beforehand. >>>>> >>>>> This fails to apply, pls rebase and resend >>>>> >>>> >>>> On rebasing, I am seeing no difference in the patch and I am able to >>>> apply it on top of linux-next/master commit 6abab1b81b65. May I know if >>>> there is any other branch that I would need to rebase this patch on top of? >>> >>> It should be based on phy-next which is at >>> 1f1b0c105b19ac0d90975e2569040da1216489b7 now >>> >> >> I have posted a respin of this patch after rebasing it on top of >> phy-next. One aspect that is not clear to me is, phy-next branch does >> not have the following commit which is present in linux-next master, >> >> 29afbd769ca3 phy: cadence: Sierra: fix error handling bugs in probe() > > This is in fixes >> >> When the respin of this patch(v3) is merged with linux-next/master >> wouldn't it cause merge-conflicts? >> >> May I know how would this be handled? > > If need arises (we have a dependency) I would merge fixes into next and > then apply patches. Cover letter of the patches should mention that > Thank you for the clarification. I will make note of mentioning this from next time. So, just to confirm, if the fixes are merged then v2 of this patch series will apply cleanly. Thanks, Aswath
On 04-02-22, 11:48, Aswath Govindraju wrote: > Thank you for the clarification. I will make note of mentioning this > from next time. So, just to confirm, if the fixes are merged then v2 of > this patch series will apply cleanly. I have merged fixes and applied v2 now
diff --git a/drivers/phy/cadence/phy-cadence-sierra.c b/drivers/phy/cadence/phy-cadence-sierra.c index e265647e29a2..6b917f7bddbe 100644 --- a/drivers/phy/cadence/phy-cadence-sierra.c +++ b/drivers/phy/cadence/phy-cadence-sierra.c @@ -370,6 +370,7 @@ struct cdns_sierra_phy { int nsubnodes; u32 num_lanes; bool autoconf; + int already_configured; struct clk_onecell_data clk_data; struct clk *output_clks[CDNS_SIERRA_OUTPUT_CLOCKS]; }; @@ -517,7 +518,7 @@ static int cdns_sierra_phy_init(struct phy *gphy) int i, j; /* Initialise the PHY registers, unless auto configured */ - if (phy->autoconf || phy->nsubnodes > 1) + if (phy->autoconf || phy->already_configured || phy->nsubnodes > 1) return 0; clk_set_rate(phy->input_clks[CMN_REFCLK_DIG_DIV], 25000000); @@ -646,6 +647,18 @@ static const struct phy_ops ops = { .owner = THIS_MODULE, }; +static int cdns_sierra_noop_phy_on(struct phy *gphy) +{ + usleep_range(5000, 10000); + + return 0; +} + +static const struct phy_ops noop_ops = { + .power_on = cdns_sierra_noop_phy_on, + .owner = THIS_MODULE, +}; + static u8 cdns_sierra_pll_mux_get_parent(struct clk_hw *hw) { struct cdns_sierra_pll_mux *mux = to_cdns_sierra_pll_mux(hw); @@ -1118,13 +1131,6 @@ static int cdns_sierra_phy_get_clocks(struct cdns_sierra_phy *sp, struct clk *clk; int ret; - clk = devm_clk_get_optional(dev, "phy_clk"); - if (IS_ERR(clk)) { - dev_err(dev, "failed to get clock phy_clk\n"); - return PTR_ERR(clk); - } - sp->input_clks[PHY_CLK] = clk; - clk = devm_clk_get_optional(dev, "cmn_refclk_dig_div"); if (IS_ERR(clk)) { dev_err(dev, "cmn_refclk_dig_div clock not found\n"); @@ -1160,17 +1166,33 @@ static int cdns_sierra_phy_get_clocks(struct cdns_sierra_phy *sp, return 0; } -static int cdns_sierra_phy_enable_clocks(struct cdns_sierra_phy *sp) +static int cdns_sierra_phy_clk(struct cdns_sierra_phy *sp) { + struct device *dev = sp->dev; + struct clk *clk; int ret; + clk = devm_clk_get_optional(dev, "phy_clk"); + if (IS_ERR(clk)) { + dev_err(dev, "failed to get clock phy_clk\n"); + return PTR_ERR(clk); + } + sp->input_clks[PHY_CLK] = clk; + ret = clk_prepare_enable(sp->input_clks[PHY_CLK]); if (ret) return ret; + return 0; +} + +static int cdns_sierra_phy_enable_clocks(struct cdns_sierra_phy *sp) +{ + int ret; + ret = clk_prepare_enable(sp->output_clks[CDNS_SIERRA_PLL_CMNLC]); if (ret) - goto err_pll_cmnlc; + return ret; ret = clk_prepare_enable(sp->output_clks[CDNS_SIERRA_PLL_CMNLC1]); if (ret) @@ -1181,9 +1203,6 @@ static int cdns_sierra_phy_enable_clocks(struct cdns_sierra_phy *sp) err_pll_cmnlc1: clk_disable_unprepare(sp->output_clks[CDNS_SIERRA_PLL_CMNLC]); -err_pll_cmnlc: - clk_disable_unprepare(sp->input_clks[PHY_CLK]); - return ret; } @@ -1191,7 +1210,8 @@ static void cdns_sierra_phy_disable_clocks(struct cdns_sierra_phy *sp) { clk_disable_unprepare(sp->output_clks[CDNS_SIERRA_PLL_CMNLC1]); clk_disable_unprepare(sp->output_clks[CDNS_SIERRA_PLL_CMNLC]); - clk_disable_unprepare(sp->input_clks[PHY_CLK]); + if (!sp->already_configured) + clk_disable_unprepare(sp->input_clks[PHY_CLK]); } static int cdns_sierra_phy_get_resets(struct cdns_sierra_phy *sp, @@ -1382,22 +1402,30 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev) if (ret) return ret; - ret = cdns_sierra_phy_get_resets(sp, dev); - if (ret) - goto unregister_clk; - ret = cdns_sierra_phy_enable_clocks(sp); if (ret) goto unregister_clk; - /* Enable APB */ - reset_control_deassert(sp->apb_rst); + regmap_field_read(sp->pma_cmn_ready, &sp->already_configured); + + if (!sp->already_configured) { + ret = cdns_sierra_phy_clk(sp); + if (ret) + goto clk_disable; + + ret = cdns_sierra_phy_get_resets(sp, dev); + if (ret) + goto clk_disable; + + /* Enable APB */ + reset_control_deassert(sp->apb_rst); + } /* Check that PHY is present */ regmap_field_read(sp->macro_id_type, &id_value); if (sp->init_data->id_value != id_value) { ret = -EINVAL; - goto clk_disable; + goto ctrl_assert; } sp->autoconf = of_property_read_bool(dn, "cdns,autoconf"); @@ -1433,8 +1461,10 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev) sp->num_lanes += sp->phys[node].num_lanes; - gphy = devm_phy_create(dev, child, &ops); - + if (!sp->already_configured) + gphy = devm_phy_create(dev, child, &ops); + else + gphy = devm_phy_create(dev, child, &noop_ops); if (IS_ERR(gphy)) { ret = PTR_ERR(gphy); of_node_put(child); @@ -1455,7 +1485,7 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev) } /* If more than one subnode, configure the PHY as multilink */ - if (!sp->autoconf && sp->nsubnodes > 1) { + if (!sp->already_configured && !sp->autoconf && sp->nsubnodes > 1) { ret = cdns_sierra_phy_configure_multilink(sp); if (ret) goto put_control; @@ -1473,9 +1503,11 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev) put_control: while (--node >= 0) reset_control_put(sp->phys[node].lnk_rst); +ctrl_assert: + if (!sp->already_configured) + reset_control_assert(sp->apb_rst); clk_disable: cdns_sierra_phy_disable_clocks(sp); - reset_control_assert(sp->apb_rst); unregister_clk: cdns_sierra_clk_unregister(sp); return ret;
In some cases, a single SerDes instance can be shared between two different processors, each using a separate link. In these cases, the SerDes configuration is done in an earlier boot stage. Therefore, add support to skip reconfiguring, if it is was already configured beforehand. Signed-off-by: Aswath Govindraju <a-govindraju@ti.com> --- Changes since v1: - Removed redundant braces - Corrected the logic for skipping multilink configuration - Corrected the order in failure path drivers/phy/cadence/phy-cadence-sierra.c | 82 ++++++++++++++++-------- 1 file changed, 57 insertions(+), 25 deletions(-)