| Message ID | 20230605184455.34832-2-athierry@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Clock fixes for qcom-snps-femto-v2 PHY driver | expand |
On 5.06.2023 20:44, Adrien Thierry wrote: > The driver is not enabling the ref clock, which thus gets disabled by > the clk_disable_unused initcall. This leads to the dwc3 controller > failing to initialize if probed after clk_disable_unused is called, for > instance when the driver is built as a module. > > To fix this, switch to the clk_bulk API to handle both cfg_ahb and ref > clocks at the proper places. > > Note that the cfg_ahb clock is currently not used by any device tree > instantiation of the PHY. Work needs to be done separately to fix this. > > Link: https://lore.kernel.org/linux-arm-msm/ZEqvy+khHeTkC2hf@fedora/ > Fixes: 51e8114f80d0 ("phy: qcom-snps: Add SNPS USB PHY driver for QCOM based SOCs") > Signed-off-by: Adrien Thierry <athierry@redhat.com> > --- > drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 67 ++++++++++++++----- > 1 file changed, 49 insertions(+), 18 deletions(-) > > diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > index 6c237f3cc66d..ce1d2f8b568a 100644 > --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > @@ -110,11 +110,13 @@ struct phy_override_seq { > /** > * struct qcom_snps_hsphy - snps hs phy attributes > * > + * @dev: device structure > + * > * @phy: generic phy > * @base: iomapped memory space for snps hs phy > * > - * @cfg_ahb_clk: AHB2PHY interface clock > - * @ref_clk: phy reference clock > + * @num_clks: number of clocks > + * @clks: array of clocks > * @phy_reset: phy reset control > * @vregs: regulator supplies bulk data > * @phy_initialized: if PHY has been initialized correctly > @@ -122,11 +124,13 @@ struct phy_override_seq { > * @update_seq_cfg: tuning parameters for phy init > */ > struct qcom_snps_hsphy { > + struct device *dev; > + > struct phy *phy; > void __iomem *base; > > - struct clk *cfg_ahb_clk; > - struct clk *ref_clk; > + int num_clks; > + struct clk_bulk_data *clks; > struct reset_control *phy_reset; > struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS]; > > @@ -135,6 +139,32 @@ struct qcom_snps_hsphy { > struct phy_override_seq update_seq_cfg[NUM_HSPHY_TUNING_PARAMS]; > }; > > +static int qcom_snps_hsphy_clk_init(struct qcom_snps_hsphy *hsphy) > +{ > + struct device *dev = hsphy->dev; > + > + hsphy->num_clks = 2; > + hsphy->clks = devm_kcalloc(dev, hsphy->num_clks, sizeof(*hsphy->clks), GFP_KERNEL); > + if (!hsphy->clks) > + return -ENOMEM; > + > + /* > + * HACK: For cfg_ahb clock, use devm_clk_get_optional() because currently no device > + * tree instantiation of the PHY is using the clock. This needs to be fixed in order > + * for this code to be able to use devm_clk_bulk_get(). > + */ > + hsphy->clks[0].id = "cfg_ahb"; > + hsphy->clks[0].clk = devm_clk_get_optional(dev, "cfg_ahb"); Hm, maybe you could first check if we can get this clock properly (!IS_ERR_OR_NULL) and then allocate the second slot.. > + > + hsphy->clks[1].id = "ref"; > + hsphy->clks[1].clk = devm_clk_get(dev, "ref"); > + if (IS_ERR(hsphy->clks[1].clk)) > + return dev_err_probe(dev, PTR_ERR(hsphy->clks[1].clk), > + "failed to get ref clk\n"); > + > + return 0; > +} > + > static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 offset, > u32 mask, u32 val) > { > @@ -165,7 +195,7 @@ static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy) > 0, USB2_AUTO_RESUME); > } > > - clk_disable_unprepare(hsphy->cfg_ahb_clk); > + clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks); > return 0; > } > > @@ -175,9 +205,9 @@ static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy) > > dev_dbg(&hsphy->phy->dev, "Resume QCOM SNPS PHY, mode\n"); > > - ret = clk_prepare_enable(hsphy->cfg_ahb_clk); > + ret = clk_bulk_prepare_enable(hsphy->num_clks, hsphy->clks); Aren't you dereferencing NULL if the optional clock is absent? Konrad > if (ret) { > - dev_err(&hsphy->phy->dev, "failed to enable cfg ahb clock\n"); > + dev_err(&hsphy->phy->dev, "failed to enable clocks\n"); > return ret; > } > > @@ -374,16 +404,16 @@ static int qcom_snps_hsphy_init(struct phy *phy) > if (ret) > return ret; > > - ret = clk_prepare_enable(hsphy->cfg_ahb_clk); > + ret = clk_bulk_prepare_enable(hsphy->num_clks, hsphy->clks); > if (ret) { > - dev_err(&phy->dev, "failed to enable cfg ahb clock, %d\n", ret); > + dev_err(&phy->dev, "failed to enable clocks, %d\n", ret); > goto poweroff_phy; > } > > ret = reset_control_assert(hsphy->phy_reset); > if (ret) { > dev_err(&phy->dev, "failed to assert phy_reset, %d\n", ret); > - goto disable_ahb_clk; > + goto disable_clks; > } > > usleep_range(100, 150); > @@ -391,7 +421,7 @@ static int qcom_snps_hsphy_init(struct phy *phy) > ret = reset_control_deassert(hsphy->phy_reset); > if (ret) { > dev_err(&phy->dev, "failed to de-assert phy_reset, %d\n", ret); > - goto disable_ahb_clk; > + goto disable_clks; > } > > qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_CFG0, > @@ -448,8 +478,8 @@ static int qcom_snps_hsphy_init(struct phy *phy) > > return 0; > > -disable_ahb_clk: > - clk_disable_unprepare(hsphy->cfg_ahb_clk); > +disable_clks: > + clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks); > poweroff_phy: > regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs); > > @@ -461,7 +491,7 @@ static int qcom_snps_hsphy_exit(struct phy *phy) > struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy); > > reset_control_assert(hsphy->phy_reset); > - clk_disable_unprepare(hsphy->cfg_ahb_clk); > + clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks); > regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs); > hsphy->phy_initialized = false; > > @@ -554,14 +584,15 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev) > if (!hsphy) > return -ENOMEM; > > + hsphy->dev = dev; > + > hsphy->base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(hsphy->base)) > return PTR_ERR(hsphy->base); > > - hsphy->ref_clk = devm_clk_get(dev, "ref"); > - if (IS_ERR(hsphy->ref_clk)) > - return dev_err_probe(dev, PTR_ERR(hsphy->ref_clk), > - "failed to get ref clk\n"); > + ret = qcom_snps_hsphy_clk_init(hsphy); > + if (ret) > + return dev_err_probe(dev, ret, "failed to initialize clocks\n"); > > hsphy->phy_reset = devm_reset_control_get_exclusive(&pdev->dev, NULL); > if (IS_ERR(hsphy->phy_reset)) {
On Tue, Jun 06, 2023 at 01:14:00AM +0200, Konrad Dybcio wrote: > > > On 5.06.2023 20:44, Adrien Thierry wrote: > > The driver is not enabling the ref clock, which thus gets disabled by > > the clk_disable_unused initcall. This leads to the dwc3 controller > > failing to initialize if probed after clk_disable_unused is called, for > > instance when the driver is built as a module. > > > > To fix this, switch to the clk_bulk API to handle both cfg_ahb and ref > > clocks at the proper places. > > > > Note that the cfg_ahb clock is currently not used by any device tree > > instantiation of the PHY. Work needs to be done separately to fix this. > > > > Link: https://lore.kernel.org/linux-arm-msm/ZEqvy+khHeTkC2hf@fedora/ > > Fixes: 51e8114f80d0 ("phy: qcom-snps: Add SNPS USB PHY driver for QCOM based SOCs") > > Signed-off-by: Adrien Thierry <athierry@redhat.com> > > --- > > drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 67 ++++++++++++++----- > > 1 file changed, 49 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > > index 6c237f3cc66d..ce1d2f8b568a 100644 > > --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > > +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > > @@ -110,11 +110,13 @@ struct phy_override_seq { > > /** > > * struct qcom_snps_hsphy - snps hs phy attributes > > * > > + * @dev: device structure > > + * > > * @phy: generic phy > > * @base: iomapped memory space for snps hs phy > > * > > - * @cfg_ahb_clk: AHB2PHY interface clock > > - * @ref_clk: phy reference clock > > + * @num_clks: number of clocks > > + * @clks: array of clocks > > * @phy_reset: phy reset control > > * @vregs: regulator supplies bulk data > > * @phy_initialized: if PHY has been initialized correctly > > @@ -122,11 +124,13 @@ struct phy_override_seq { > > * @update_seq_cfg: tuning parameters for phy init > > */ > > struct qcom_snps_hsphy { > > + struct device *dev; > > + > > struct phy *phy; > > void __iomem *base; > > > > - struct clk *cfg_ahb_clk; > > - struct clk *ref_clk; > > + int num_clks; > > + struct clk_bulk_data *clks; > > struct reset_control *phy_reset; > > struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS]; > > > > @@ -135,6 +139,32 @@ struct qcom_snps_hsphy { > > struct phy_override_seq update_seq_cfg[NUM_HSPHY_TUNING_PARAMS]; > > }; > > > > +static int qcom_snps_hsphy_clk_init(struct qcom_snps_hsphy *hsphy) > > +{ > > + struct device *dev = hsphy->dev; > > + > > + hsphy->num_clks = 2; > > + hsphy->clks = devm_kcalloc(dev, hsphy->num_clks, sizeof(*hsphy->clks), GFP_KERNEL); > > + if (!hsphy->clks) > > + return -ENOMEM; > > + > > + /* > > + * HACK: For cfg_ahb clock, use devm_clk_get_optional() because currently no device > > + * tree instantiation of the PHY is using the clock. This needs to be fixed in order > > + * for this code to be able to use devm_clk_bulk_get(). > > + */ > > + hsphy->clks[0].id = "cfg_ahb"; > > + hsphy->clks[0].clk = devm_clk_get_optional(dev, "cfg_ahb"); > Hm, maybe you could first check if we can get this clock > properly (!IS_ERR_OR_NULL) and then allocate the second > slot.. > The bulk clk api handles NULL clks without issue if I am reading right, so personally if we're going to use the bulk api I say we carry the extra slot unconditionally. No expert on this stuff but that seems more straightforward. Honestly I wouldn't mind using the bulk optional API, then checking the "non optional ref clock" manually. That's closer to the ideal flow to me. Super opinionated though, don't take my word as right. > > + > > + hsphy->clks[1].id = "ref"; > > + hsphy->clks[1].clk = devm_clk_get(dev, "ref"); > > + if (IS_ERR(hsphy->clks[1].clk)) > > + return dev_err_probe(dev, PTR_ERR(hsphy->clks[1].clk), > > + "failed to get ref clk\n"); > > + > > + return 0; > > +} > > + > > static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 offset, > > u32 mask, u32 val) > > { > > @@ -165,7 +195,7 @@ static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy) > > 0, USB2_AUTO_RESUME); > > } > > > > - clk_disable_unprepare(hsphy->cfg_ahb_clk); > > + clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks); > > return 0; > > } > > > > @@ -175,9 +205,9 @@ static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy) > > > > dev_dbg(&hsphy->phy->dev, "Resume QCOM SNPS PHY, mode\n"); > > > > - ret = clk_prepare_enable(hsphy->cfg_ahb_clk); > > + ret = clk_bulk_prepare_enable(hsphy->num_clks, hsphy->clks); > Aren't you dereferencing NULL if the optional clock is absent? > Similar to above, the bulk api seems to handle NULL clks gracefully. Thanks, Andrew
Thanks for your feedback Konrad and Andrew! On Tue, Jun 06, 2023 at 08:55:16AM -0500, Andrew Halaney wrote: > On Tue, Jun 06, 2023 at 01:14:00AM +0200, Konrad Dybcio wrote: > > > > > > On 5.06.2023 20:44, Adrien Thierry wrote: > > > The driver is not enabling the ref clock, which thus gets disabled by > > > the clk_disable_unused initcall. This leads to the dwc3 controller > > > failing to initialize if probed after clk_disable_unused is called, for > > > instance when the driver is built as a module. > > > > > > To fix this, switch to the clk_bulk API to handle both cfg_ahb and ref > > > clocks at the proper places. > > > > > > Note that the cfg_ahb clock is currently not used by any device tree > > > instantiation of the PHY. Work needs to be done separately to fix this. > > > > > > Link: https://lore.kernel.org/linux-arm-msm/ZEqvy+khHeTkC2hf@fedora/ > > > Fixes: 51e8114f80d0 ("phy: qcom-snps: Add SNPS USB PHY driver for QCOM based SOCs") > > > Signed-off-by: Adrien Thierry <athierry@redhat.com> > > > --- > > > drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 67 ++++++++++++++----- > > > 1 file changed, 49 insertions(+), 18 deletions(-) > > > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > > > index 6c237f3cc66d..ce1d2f8b568a 100644 > > > --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > > > +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c > > > @@ -110,11 +110,13 @@ struct phy_override_seq { > > > /** > > > * struct qcom_snps_hsphy - snps hs phy attributes > > > * > > > + * @dev: device structure > > > + * > > > * @phy: generic phy > > > * @base: iomapped memory space for snps hs phy > > > * > > > - * @cfg_ahb_clk: AHB2PHY interface clock > > > - * @ref_clk: phy reference clock > > > + * @num_clks: number of clocks > > > + * @clks: array of clocks > > > * @phy_reset: phy reset control > > > * @vregs: regulator supplies bulk data > > > * @phy_initialized: if PHY has been initialized correctly > > > @@ -122,11 +124,13 @@ struct phy_override_seq { > > > * @update_seq_cfg: tuning parameters for phy init > > > */ > > > struct qcom_snps_hsphy { > > > + struct device *dev; > > > + > > > struct phy *phy; > > > void __iomem *base; > > > > > > - struct clk *cfg_ahb_clk; > > > - struct clk *ref_clk; > > > + int num_clks; > > > + struct clk_bulk_data *clks; > > > struct reset_control *phy_reset; > > > struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS]; > > > > > > @@ -135,6 +139,32 @@ struct qcom_snps_hsphy { > > > struct phy_override_seq update_seq_cfg[NUM_HSPHY_TUNING_PARAMS]; > > > }; > > > > > > +static int qcom_snps_hsphy_clk_init(struct qcom_snps_hsphy *hsphy) > > > +{ > > > + struct device *dev = hsphy->dev; > > > + > > > + hsphy->num_clks = 2; > > > + hsphy->clks = devm_kcalloc(dev, hsphy->num_clks, sizeof(*hsphy->clks), GFP_KERNEL); > > > + if (!hsphy->clks) > > > + return -ENOMEM; > > > + > > > + /* > > > + * HACK: For cfg_ahb clock, use devm_clk_get_optional() because currently no device > > > + * tree instantiation of the PHY is using the clock. This needs to be fixed in order > > > + * for this code to be able to use devm_clk_bulk_get(). > > > + */ > > > + hsphy->clks[0].id = "cfg_ahb"; > > > + hsphy->clks[0].clk = devm_clk_get_optional(dev, "cfg_ahb"); > > Hm, maybe you could first check if we can get this clock > > properly (!IS_ERR_OR_NULL) and then allocate the second > > slot.. > > > > The bulk clk api handles NULL clks without issue if I am reading right, > so personally if we're going to use the bulk api I say we carry the extra > slot unconditionally. No expert on this stuff but that seems more > straightforward. Honestly I wouldn't mind using the bulk optional API, > then checking the "non optional ref clock" manually. That's closer to > the ideal flow to me. Super opinionated though, don't take my word as > right. > Agree with Andrew. Since cfg_ahb is always NULL, I'm certainly "wasting" an array cell here but I think it also better highlights the fact that it's a hack and that cfg_ahb needs to be properly wired in the DTs. As for using the bulk optional API, I'm fine with both! > > > + > > > + hsphy->clks[1].id = "ref"; > > > + hsphy->clks[1].clk = devm_clk_get(dev, "ref"); > > > + if (IS_ERR(hsphy->clks[1].clk)) > > > + return dev_err_probe(dev, PTR_ERR(hsphy->clks[1].clk), > > > + "failed to get ref clk\n"); > > > + > > > + return 0; > > > +} > > > + > > > static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 offset, > > > u32 mask, u32 val) > > > { > > > @@ -165,7 +195,7 @@ static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy) > > > 0, USB2_AUTO_RESUME); > > > } > > > > > > - clk_disable_unprepare(hsphy->cfg_ahb_clk); > > > + clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks); > > > return 0; > > > } > > > > > > @@ -175,9 +205,9 @@ static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy) > > > > > > dev_dbg(&hsphy->phy->dev, "Resume QCOM SNPS PHY, mode\n"); > > > > > > - ret = clk_prepare_enable(hsphy->cfg_ahb_clk); > > > + ret = clk_bulk_prepare_enable(hsphy->num_clks, hsphy->clks); > > Aren't you dereferencing NULL if the optional clock is absent? > > > > Similar to above, the bulk api seems to handle NULL clks gracefully. > devm_clk_get_optional() will return NULL for cfg_ahb, but AFAIU NULL serves as a dummy clock [1] with which the clock API deals gracefully. The various functions like clk_prepare(), clk_enable() check if the clock is NULL and return 0 immediately if that's the case (see for instance [2]). [1] https://elixir.bootlin.com/linux/v6.4-rc5/source/include/linux/clk.h#L514 [2] https://elixir.bootlin.com/linux/v6.4-rc5/source/drivers/clk/clk.c#L1045 Best, Adrien > Thanks, > Andrew >
On 6.06.2023 16:35, Adrien Thierry wrote: > Thanks for your feedback Konrad and Andrew! > > On Tue, Jun 06, 2023 at 08:55:16AM -0500, Andrew Halaney wrote: >> On Tue, Jun 06, 2023 at 01:14:00AM +0200, Konrad Dybcio wrote: >>> >>> >>> On 5.06.2023 20:44, Adrien Thierry wrote: >>>> The driver is not enabling the ref clock, which thus gets disabled by >>>> the clk_disable_unused initcall. This leads to the dwc3 controller >>>> failing to initialize if probed after clk_disable_unused is called, for >>>> instance when the driver is built as a module. >>>> >>>> To fix this, switch to the clk_bulk API to handle both cfg_ahb and ref >>>> clocks at the proper places. >>>> >>>> Note that the cfg_ahb clock is currently not used by any device tree >>>> instantiation of the PHY. Work needs to be done separately to fix this. >>>> >>>> Link: https://lore.kernel.org/linux-arm-msm/ZEqvy+khHeTkC2hf@fedora/ >>>> Fixes: 51e8114f80d0 ("phy: qcom-snps: Add SNPS USB PHY driver for QCOM based SOCs") >>>> Signed-off-by: Adrien Thierry <athierry@redhat.com> >>>> --- >>>> drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 67 ++++++++++++++----- >>>> 1 file changed, 49 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c >>>> index 6c237f3cc66d..ce1d2f8b568a 100644 >>>> --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c >>>> +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c >>>> @@ -110,11 +110,13 @@ struct phy_override_seq { >>>> /** >>>> * struct qcom_snps_hsphy - snps hs phy attributes >>>> * >>>> + * @dev: device structure >>>> + * >>>> * @phy: generic phy >>>> * @base: iomapped memory space for snps hs phy >>>> * >>>> - * @cfg_ahb_clk: AHB2PHY interface clock >>>> - * @ref_clk: phy reference clock >>>> + * @num_clks: number of clocks >>>> + * @clks: array of clocks >>>> * @phy_reset: phy reset control >>>> * @vregs: regulator supplies bulk data >>>> * @phy_initialized: if PHY has been initialized correctly >>>> @@ -122,11 +124,13 @@ struct phy_override_seq { >>>> * @update_seq_cfg: tuning parameters for phy init >>>> */ >>>> struct qcom_snps_hsphy { >>>> + struct device *dev; >>>> + >>>> struct phy *phy; >>>> void __iomem *base; >>>> >>>> - struct clk *cfg_ahb_clk; >>>> - struct clk *ref_clk; >>>> + int num_clks; >>>> + struct clk_bulk_data *clks; >>>> struct reset_control *phy_reset; >>>> struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS]; >>>> >>>> @@ -135,6 +139,32 @@ struct qcom_snps_hsphy { >>>> struct phy_override_seq update_seq_cfg[NUM_HSPHY_TUNING_PARAMS]; >>>> }; >>>> >>>> +static int qcom_snps_hsphy_clk_init(struct qcom_snps_hsphy *hsphy) >>>> +{ >>>> + struct device *dev = hsphy->dev; >>>> + >>>> + hsphy->num_clks = 2; >>>> + hsphy->clks = devm_kcalloc(dev, hsphy->num_clks, sizeof(*hsphy->clks), GFP_KERNEL); >>>> + if (!hsphy->clks) >>>> + return -ENOMEM; >>>> + >>>> + /* >>>> + * HACK: For cfg_ahb clock, use devm_clk_get_optional() because currently no device >>>> + * tree instantiation of the PHY is using the clock. This needs to be fixed in order >>>> + * for this code to be able to use devm_clk_bulk_get(). >>>> + */ >>>> + hsphy->clks[0].id = "cfg_ahb"; >>>> + hsphy->clks[0].clk = devm_clk_get_optional(dev, "cfg_ahb"); >>> Hm, maybe you could first check if we can get this clock >>> properly (!IS_ERR_OR_NULL) and then allocate the second >>> slot.. >>> >> >> The bulk clk api handles NULL clks without issue if I am reading right, >> so personally if we're going to use the bulk api I say we carry the extra >> slot unconditionally. No expert on this stuff but that seems more >> straightforward. Honestly I wouldn't mind using the bulk optional API, >> then checking the "non optional ref clock" manually. That's closer to >> the ideal flow to me. Super opinionated though, don't take my word as >> right. >> > > Agree with Andrew. Since cfg_ahb is always NULL, I'm certainly "wasting" > an array cell here but I think it also better highlights the fact that > it's a hack and that cfg_ahb needs to be properly wired in the DTs. As for > using the bulk optional API, I'm fine with both! Hm right, let's keep it as-is so that the other clock which is actually necessary never gets skipped accidentally.. Konrad > >>>> + >>>> + hsphy->clks[1].id = "ref"; >>>> + hsphy->clks[1].clk = devm_clk_get(dev, "ref"); >>>> + if (IS_ERR(hsphy->clks[1].clk)) >>>> + return dev_err_probe(dev, PTR_ERR(hsphy->clks[1].clk), >>>> + "failed to get ref clk\n"); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 offset, >>>> u32 mask, u32 val) >>>> { >>>> @@ -165,7 +195,7 @@ static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy) >>>> 0, USB2_AUTO_RESUME); >>>> } >>>> >>>> - clk_disable_unprepare(hsphy->cfg_ahb_clk); >>>> + clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks); >>>> return 0; >>>> } >>>> >>>> @@ -175,9 +205,9 @@ static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy) >>>> >>>> dev_dbg(&hsphy->phy->dev, "Resume QCOM SNPS PHY, mode\n"); >>>> >>>> - ret = clk_prepare_enable(hsphy->cfg_ahb_clk); >>>> + ret = clk_bulk_prepare_enable(hsphy->num_clks, hsphy->clks); >>> Aren't you dereferencing NULL if the optional clock is absent? >>> >> >> Similar to above, the bulk api seems to handle NULL clks gracefully. >> > > devm_clk_get_optional() will return NULL for cfg_ahb, but AFAIU NULL > serves as a dummy clock [1] with which the clock API deals gracefully. The > various functions like clk_prepare(), clk_enable() check if the clock is > NULL and return 0 immediately if that's the case (see for instance [2]). > > [1] https://elixir.bootlin.com/linux/v6.4-rc5/source/include/linux/clk.h#L514 > [2] https://elixir.bootlin.com/linux/v6.4-rc5/source/drivers/clk/clk.c#L1045 > > Best, > Adrien > >> Thanks, >> Andrew >> >
On Mon, Jun 05, 2023 at 02:44:53PM -0400, Adrien Thierry wrote: > The driver is not enabling the ref clock, which thus gets disabled by > the clk_disable_unused initcall. This leads to the dwc3 controller > failing to initialize if probed after clk_disable_unused is called, for > instance when the driver is built as a module. > > To fix this, switch to the clk_bulk API to handle both cfg_ahb and ref > clocks at the proper places. > > Note that the cfg_ahb clock is currently not used by any device tree > instantiation of the PHY. Work needs to be done separately to fix this. > > Link: https://lore.kernel.org/linux-arm-msm/ZEqvy+khHeTkC2hf@fedora/ > Fixes: 51e8114f80d0 ("phy: qcom-snps: Add SNPS USB PHY driver for QCOM based SOCs") > Signed-off-by: Adrien Thierry <athierry@redhat.com> Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
diff --git a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c index 6c237f3cc66d..ce1d2f8b568a 100644 --- a/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c +++ b/drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c @@ -110,11 +110,13 @@ struct phy_override_seq { /** * struct qcom_snps_hsphy - snps hs phy attributes * + * @dev: device structure + * * @phy: generic phy * @base: iomapped memory space for snps hs phy * - * @cfg_ahb_clk: AHB2PHY interface clock - * @ref_clk: phy reference clock + * @num_clks: number of clocks + * @clks: array of clocks * @phy_reset: phy reset control * @vregs: regulator supplies bulk data * @phy_initialized: if PHY has been initialized correctly @@ -122,11 +124,13 @@ struct phy_override_seq { * @update_seq_cfg: tuning parameters for phy init */ struct qcom_snps_hsphy { + struct device *dev; + struct phy *phy; void __iomem *base; - struct clk *cfg_ahb_clk; - struct clk *ref_clk; + int num_clks; + struct clk_bulk_data *clks; struct reset_control *phy_reset; struct regulator_bulk_data vregs[SNPS_HS_NUM_VREGS]; @@ -135,6 +139,32 @@ struct qcom_snps_hsphy { struct phy_override_seq update_seq_cfg[NUM_HSPHY_TUNING_PARAMS]; }; +static int qcom_snps_hsphy_clk_init(struct qcom_snps_hsphy *hsphy) +{ + struct device *dev = hsphy->dev; + + hsphy->num_clks = 2; + hsphy->clks = devm_kcalloc(dev, hsphy->num_clks, sizeof(*hsphy->clks), GFP_KERNEL); + if (!hsphy->clks) + return -ENOMEM; + + /* + * HACK: For cfg_ahb clock, use devm_clk_get_optional() because currently no device + * tree instantiation of the PHY is using the clock. This needs to be fixed in order + * for this code to be able to use devm_clk_bulk_get(). + */ + hsphy->clks[0].id = "cfg_ahb"; + hsphy->clks[0].clk = devm_clk_get_optional(dev, "cfg_ahb"); + + hsphy->clks[1].id = "ref"; + hsphy->clks[1].clk = devm_clk_get(dev, "ref"); + if (IS_ERR(hsphy->clks[1].clk)) + return dev_err_probe(dev, PTR_ERR(hsphy->clks[1].clk), + "failed to get ref clk\n"); + + return 0; +} + static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 offset, u32 mask, u32 val) { @@ -165,7 +195,7 @@ static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy) 0, USB2_AUTO_RESUME); } - clk_disable_unprepare(hsphy->cfg_ahb_clk); + clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks); return 0; } @@ -175,9 +205,9 @@ static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy) dev_dbg(&hsphy->phy->dev, "Resume QCOM SNPS PHY, mode\n"); - ret = clk_prepare_enable(hsphy->cfg_ahb_clk); + ret = clk_bulk_prepare_enable(hsphy->num_clks, hsphy->clks); if (ret) { - dev_err(&hsphy->phy->dev, "failed to enable cfg ahb clock\n"); + dev_err(&hsphy->phy->dev, "failed to enable clocks\n"); return ret; } @@ -374,16 +404,16 @@ static int qcom_snps_hsphy_init(struct phy *phy) if (ret) return ret; - ret = clk_prepare_enable(hsphy->cfg_ahb_clk); + ret = clk_bulk_prepare_enable(hsphy->num_clks, hsphy->clks); if (ret) { - dev_err(&phy->dev, "failed to enable cfg ahb clock, %d\n", ret); + dev_err(&phy->dev, "failed to enable clocks, %d\n", ret); goto poweroff_phy; } ret = reset_control_assert(hsphy->phy_reset); if (ret) { dev_err(&phy->dev, "failed to assert phy_reset, %d\n", ret); - goto disable_ahb_clk; + goto disable_clks; } usleep_range(100, 150); @@ -391,7 +421,7 @@ static int qcom_snps_hsphy_init(struct phy *phy) ret = reset_control_deassert(hsphy->phy_reset); if (ret) { dev_err(&phy->dev, "failed to de-assert phy_reset, %d\n", ret); - goto disable_ahb_clk; + goto disable_clks; } qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_CFG0, @@ -448,8 +478,8 @@ static int qcom_snps_hsphy_init(struct phy *phy) return 0; -disable_ahb_clk: - clk_disable_unprepare(hsphy->cfg_ahb_clk); +disable_clks: + clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks); poweroff_phy: regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs); @@ -461,7 +491,7 @@ static int qcom_snps_hsphy_exit(struct phy *phy) struct qcom_snps_hsphy *hsphy = phy_get_drvdata(phy); reset_control_assert(hsphy->phy_reset); - clk_disable_unprepare(hsphy->cfg_ahb_clk); + clk_bulk_disable_unprepare(hsphy->num_clks, hsphy->clks); regulator_bulk_disable(ARRAY_SIZE(hsphy->vregs), hsphy->vregs); hsphy->phy_initialized = false; @@ -554,14 +584,15 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev) if (!hsphy) return -ENOMEM; + hsphy->dev = dev; + hsphy->base = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(hsphy->base)) return PTR_ERR(hsphy->base); - hsphy->ref_clk = devm_clk_get(dev, "ref"); - if (IS_ERR(hsphy->ref_clk)) - return dev_err_probe(dev, PTR_ERR(hsphy->ref_clk), - "failed to get ref clk\n"); + ret = qcom_snps_hsphy_clk_init(hsphy); + if (ret) + return dev_err_probe(dev, ret, "failed to initialize clocks\n"); hsphy->phy_reset = devm_reset_control_get_exclusive(&pdev->dev, NULL); if (IS_ERR(hsphy->phy_reset)) {
The driver is not enabling the ref clock, which thus gets disabled by the clk_disable_unused initcall. This leads to the dwc3 controller failing to initialize if probed after clk_disable_unused is called, for instance when the driver is built as a module. To fix this, switch to the clk_bulk API to handle both cfg_ahb and ref clocks at the proper places. Note that the cfg_ahb clock is currently not used by any device tree instantiation of the PHY. Work needs to be done separately to fix this. Link: https://lore.kernel.org/linux-arm-msm/ZEqvy+khHeTkC2hf@fedora/ Fixes: 51e8114f80d0 ("phy: qcom-snps: Add SNPS USB PHY driver for QCOM based SOCs") Signed-off-by: Adrien Thierry <athierry@redhat.com> --- drivers/phy/qualcomm/phy-qcom-snps-femto-v2.c | 67 ++++++++++++++----- 1 file changed, 49 insertions(+), 18 deletions(-)