| Message ID | 1521768653-11824-1-git-send-email-hayashi.kunihiko@socionext.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | ata: ahci-platform: add reset control support | expand |
Hi, On 23-03-18 02:30, Kunihiko Hayashi wrote: > Add support to get and control a list of resets for the device > as optional and shared. These resets must be kept de-asserted until > the device is enabled. > > This is specified as shared because some SoCs like UniPhier series > have common reset controls with all ahci controller instances. > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> Patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > .../devicetree/bindings/ata/ahci-platform.txt | 1 + > drivers/ata/ahci.h | 1 + > drivers/ata/libahci_platform.c | 24 +++++++++++++++++++--- > 3 files changed, 23 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt > index c760ecb..f4006d3 100644 > --- a/Documentation/devicetree/bindings/ata/ahci-platform.txt > +++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt > @@ -30,6 +30,7 @@ compatible: > Optional properties: > - dma-coherent : Present if dma operations are coherent > - clocks : a list of phandle + clock specifier pairs > +- resets : a list of phandle + reset specifier pairs > - target-supply : regulator for SATA target power > - phys : reference to the SATA PHY node > - phy-names : must be "sata-phy" > diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h > index 749fd94..47ec77a2 100644 > --- a/drivers/ata/ahci.h > +++ b/drivers/ata/ahci.h > @@ -347,6 +347,7 @@ struct ahci_host_priv { > u32 em_msg_type; /* EM message type */ > bool got_runtime_pm; /* Did we do pm_runtime_get? */ > struct clk *clks[AHCI_MAX_CLKS]; /* Optional */ > + struct reset_control *rsts; /* Optional */ > struct regulator **target_pwrs; /* Optional */ > /* > * If platform uses PHYs. There is a 1:1 relation between the port number and > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > index 341d0ef..3130db9 100644 > --- a/drivers/ata/libahci_platform.c > +++ b/drivers/ata/libahci_platform.c > @@ -25,6 +25,7 @@ > #include <linux/phy/phy.h> > #include <linux/pm_runtime.h> > #include <linux/of_platform.h> > +#include <linux/reset.h> > #include "ahci.h" > > static void ahci_host_stop(struct ata_host *host); > @@ -195,7 +196,8 @@ EXPORT_SYMBOL_GPL(ahci_platform_disable_regulators); > * following order: > * 1) Regulator > * 2) Clocks (through ahci_platform_enable_clks) > - * 3) Phys > + * 3) Resets > + * 4) Phys > * > * If resource enabling fails at any point the previous enabled resources > * are disabled in reverse order. > @@ -215,12 +217,19 @@ int ahci_platform_enable_resources(struct ahci_host_priv *hpriv) > if (rc) > goto disable_regulator; > > - rc = ahci_platform_enable_phys(hpriv); > + rc = reset_control_deassert(hpriv->rsts); > if (rc) > goto disable_clks; > > + rc = ahci_platform_enable_phys(hpriv); > + if (rc) > + goto disable_resets; > + > return 0; > > +disable_resets: > + reset_control_assert(hpriv->rsts); > + > disable_clks: > ahci_platform_disable_clks(hpriv); > > @@ -239,12 +248,15 @@ EXPORT_SYMBOL_GPL(ahci_platform_enable_resources); > * following order: > * 1) Phys > * 2) Clocks (through ahci_platform_disable_clks) > - * 3) Regulator > + * 3) Resets > + * 4) Regulator > */ > void ahci_platform_disable_resources(struct ahci_host_priv *hpriv) > { > ahci_platform_disable_phys(hpriv); > > + reset_control_assert(hpriv->rsts); > + > ahci_platform_disable_clks(hpriv); > > ahci_platform_disable_regulators(hpriv); > @@ -393,6 +405,12 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev) > hpriv->clks[i] = clk; > } > > + hpriv->rsts = devm_reset_control_array_get_optional_shared(dev); > + if (IS_ERR(hpriv->rsts)) { > + rc = PTR_ERR(hpriv->rsts); > + goto err_out; > + } > + > hpriv->nports = child_nodes = of_get_child_count(dev->of_node); > > /* > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 23, 2018 at 10:30:53AM +0900, Kunihiko Hayashi wrote: > Add support to get and control a list of resets for the device > as optional and shared. These resets must be kept de-asserted until > the device is enabled. > > This is specified as shared because some SoCs like UniPhier series > have common reset controls with all ahci controller instances. > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > --- > .../devicetree/bindings/ata/ahci-platform.txt | 1 + Reviewed-by: Rob Herring <robh@kernel.org> > drivers/ata/ahci.h | 1 + > drivers/ata/libahci_platform.c | 24 +++++++++++++++++++--- > 3 files changed, 23 insertions(+), 3 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 23, 2018 at 10:30:53AM +0900, Kunihiko Hayashi wrote: > Add support to get and control a list of resets for the device > as optional and shared. These resets must be kept de-asserted until > the device is enabled. > > This is specified as shared because some SoCs like UniPhier series > have common reset controls with all ahci controller instances. > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > --- > .../devicetree/bindings/ata/ahci-platform.txt | 1 + > drivers/ata/ahci.h | 1 + > drivers/ata/libahci_platform.c | 24 +++++++++++++++++++--- > 3 files changed, 23 insertions(+), 3 deletions(-) This causes a regression on Tegra because we explicitly request the resets after the call to ahci_platform_get_resources(). From a quick look, ahci_mtk and ahci_st are in the same boat, adding the corresponding maintainers to Cc. Patrice, Matthias: does SATA still work for you after this patch? This has been in linux-next since next-20180327. Given how this is one of the more hardware-specific bits, perhaps a better way to do this is to move reset handling into a Uniphier driver much like Tegra, Mediatek and ST? That said, I don't see SATA support for any of the Socionext hardware either in the DT bindings or drivers/ata, so perhaps it'd be best to back this out again until we have something that's more well tested? Thierry
On Thu, Apr 05, 2018 at 11:54:29AM +0200, Thierry Reding wrote: > On Fri, Mar 23, 2018 at 10:30:53AM +0900, Kunihiko Hayashi wrote: > > Add support to get and control a list of resets for the device > > as optional and shared. These resets must be kept de-asserted until > > the device is enabled. > > > > This is specified as shared because some SoCs like UniPhier series > > have common reset controls with all ahci controller instances. > > > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > > --- > > .../devicetree/bindings/ata/ahci-platform.txt | 1 + > > drivers/ata/ahci.h | 1 + > > drivers/ata/libahci_platform.c | 24 +++++++++++++++++++--- > > 3 files changed, 23 insertions(+), 3 deletions(-) > > This causes a regression on Tegra because we explicitly request the > resets after the call to ahci_platform_get_resources(). > > From a quick look, ahci_mtk and ahci_st are in the same boat, adding the > corresponding maintainers to Cc. > > Patrice, Matthias: does SATA still work for you after this patch? This > has been in linux-next since next-20180327. > > Given how this is one of the more hardware-specific bits, perhaps a > better way to do this is to move reset handling into a Uniphier driver > much like Tegra, Mediatek and ST? > > That said, I don't see SATA support for any of the Socionext hardware > either in the DT bindings or drivers/ata, so perhaps it'd be best to > back this out again until we have something that's more well tested? Tejun, I just noticed that Linus already pulled this for v4.17, so backing out isn't going to work anymore. Still, I don't think this is tested well enough, and given the lack of users of this I think a revert is the best option at this point. Thierry
Hi Thierry, On Thu, 5 Apr 2018 11:54:29 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Fri, Mar 23, 2018 at 10:30:53AM +0900, Kunihiko Hayashi wrote: > > Add support to get and control a list of resets for the device > > as optional and shared. These resets must be kept de-asserted until > > the device is enabled. > > > > This is specified as shared because some SoCs like UniPhier series > > have common reset controls with all ahci controller instances. > > > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > > --- > > .../devicetree/bindings/ata/ahci-platform.txt | 1 + > > drivers/ata/ahci.h | 1 + > > drivers/ata/libahci_platform.c | 24 +++++++++++++++++++--- > > 3 files changed, 23 insertions(+), 3 deletions(-) > > This causes a regression on Tegra because we explicitly request the > resets after the call to ahci_platform_get_resources(). > > From a quick look, ahci_mtk and ahci_st are in the same boat, adding the > corresponding maintainers to Cc. > > Patrice, Matthias: does SATA still work for you after this patch? This > has been in linux-next since next-20180327. I assume that I use "generic-ahci" driver directly, and this driver has no way to handle resets, so I sent this patch. However, also as far as I look, some hardware-specific drivers handle their own resets, and call ahci_platform_{enable,disable}_resources(). Surely there are paths to call reset control twice in such drivers. Identically, when the driver also handle their own clocks, they have same issue. > Given how this is one of the more hardware-specific bits, perhaps a > better way to do this is to move reset handling into a Uniphier driver > much like Tegra, Mediatek and ST? Since it's difficult to write the resets in general with ahci_platform, I can prepare hardware-specific driver for our SoCs. > That said, I don't see SATA support for any of the Socionext hardware > either in the DT bindings or drivers/ata, so perhaps it'd be best to > back this out again until we have something that's more well tested? I'm about to use the generic driver, and prepare our phy driver and DT bindings for our SoCs, but not yet. Then it's no problem that we can back this out. Thank you, --- Best Regards, Kunihiko Hayashi -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 05-04-18 13:23, Kunihiko Hayashi wrote: > Hi Thierry, > > On Thu, 5 Apr 2018 11:54:29 +0200 > Thierry Reding <thierry.reding@gmail.com> wrote: > >> On Fri, Mar 23, 2018 at 10:30:53AM +0900, Kunihiko Hayashi wrote: >>> Add support to get and control a list of resets for the device >>> as optional and shared. These resets must be kept de-asserted until >>> the device is enabled. >>> >>> This is specified as shared because some SoCs like UniPhier series >>> have common reset controls with all ahci controller instances. >>> >>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> >>> --- >>> .../devicetree/bindings/ata/ahci-platform.txt | 1 + >>> drivers/ata/ahci.h | 1 + >>> drivers/ata/libahci_platform.c | 24 +++++++++++++++++++--- >>> 3 files changed, 23 insertions(+), 3 deletions(-) >> >> This causes a regression on Tegra because we explicitly request the >> resets after the call to ahci_platform_get_resources(). >> >> From a quick look, ahci_mtk and ahci_st are in the same boat, adding the >> corresponding maintainers to Cc. >> >> Patrice, Matthias: does SATA still work for you after this patch? This >> has been in linux-next since next-20180327. > > I assume that I use "generic-ahci" driver directly, and this driver has > no way to handle resets, so I sent this patch. > > However, also as far as I look, some hardware-specific drivers handle their > own resets, and call ahci_platform_{enable,disable}_resources(). > Surely there are paths to call reset control twice in such drivers. > > Identically, when the driver also handle their own clocks, they have same issue. > >> Given how this is one of the more hardware-specific bits, perhaps a >> better way to do this is to move reset handling into a Uniphier driver >> much like Tegra, Mediatek and ST? > > Since it's difficult to write the resets in general with ahci_platform, I can prepare > hardware-specific driver for our SoCs > >> That said, I don't see SATA support for any of the Socionext hardware >> either in the DT bindings or drivers/ata, so perhaps it'd be best to >> back this out again until we have something that's more well tested? > > I'm about to use the generic driver, and prepare our phy driver and > DT bindings for our SoCs, but not yet. If the AHCI controller on your SoC works with the generic driver + a phy-driver using the generic phy framework, then IMHO that is preferred over adding yet another SoC specific AHCI driver. If the only reason to do a SoC specific AHCI driver is the need for resets, then IMHO we should add a flags parameter to ahci_platform_get_resources which specifies which resource-types to get and have the existing drivers call ahci_platform_get_resources() without the flag to also get resets, where as the generic driver would get resets. Thierry that should solve the problem, right ? > Then it's no problem that we can back this out. Yes reverting it for now is probably best, but I would like to see it get re-introduced while at the same time adding a flags parameter to ahci_platform_get_resources() and make the reset handling conditional on the flags. This IMHO is better then introducing another SoC driver. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Thierry On 04/05/2018 11:54 AM, Thierry Reding wrote: > On Fri, Mar 23, 2018 at 10:30:53AM +0900, Kunihiko Hayashi wrote: >> Add support to get and control a list of resets for the device >> as optional and shared. These resets must be kept de-asserted until >> the device is enabled. >> >> This is specified as shared because some SoCs like UniPhier series >> have common reset controls with all ahci controller instances. >> >> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> >> --- >> .../devicetree/bindings/ata/ahci-platform.txt | 1 + >> drivers/ata/ahci.h | 1 + >> drivers/ata/libahci_platform.c | 24 +++++++++++++++++++--- >> 3 files changed, 23 insertions(+), 3 deletions(-) > > This causes a regression on Tegra because we explicitly request the > resets after the call to ahci_platform_get_resources(). I confirm, we got exactly the same behavior on STi platform. > > From a quick look, ahci_mtk and ahci_st are in the same boat, adding the > corresponding maintainers to Cc. > > Patrice, Matthias: does SATA still work for you after this patch? This > has been in linux-next since next-20180327. SATA is still working after this patch, but a kernel warning is triggered due to the fact that resets are both requested by libahci_platform and by ahci_st driver. Patrice > > Given how this is one of the more hardware-specific bits, perhaps a > better way to do this is to move reset handling into a Uniphier driver > much like Tegra, Mediatek and ST? > > That said, I don't see SATA support for any of the Socionext hardware > either in the DT bindings or drivers/ata, so perhaps it'd be best to > back this out again until we have something that's more well tested? > > Thierry > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 05-04-18 15:17, Patrice CHOTARD wrote: > Hi Thierry > > On 04/05/2018 11:54 AM, Thierry Reding wrote: >> On Fri, Mar 23, 2018 at 10:30:53AM +0900, Kunihiko Hayashi wrote: >>> Add support to get and control a list of resets for the device >>> as optional and shared. These resets must be kept de-asserted until >>> the device is enabled. >>> >>> This is specified as shared because some SoCs like UniPhier series >>> have common reset controls with all ahci controller instances. >>> >>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> >>> --- >>> .../devicetree/bindings/ata/ahci-platform.txt | 1 + >>> drivers/ata/ahci.h | 1 + >>> drivers/ata/libahci_platform.c | 24 +++++++++++++++++++--- >>> 3 files changed, 23 insertions(+), 3 deletions(-) >> >> This causes a regression on Tegra because we explicitly request the >> resets after the call to ahci_platform_get_resources(). > > I confirm, we got exactly the same behavior on STi platform. > >> >> From a quick look, ahci_mtk and ahci_st are in the same boat, adding the >> corresponding maintainers to Cc. >> >> Patrice, Matthias: does SATA still work for you after this patch? This >> has been in linux-next since next-20180327. > > SATA is still working after this patch, but a kernel warning is > triggered due to the fact that resets are both requested by > libahci_platform and by ahci_st driver. So in your case you might be able to remove the reset handling from the ahci_st driver and rely on the new libahci_platform handling instead? If that works that seems like a win to me. As said elsewhere in this thread I think it makes sense to keep (or re-add after a revert) the libahci_platform reset code, but make it conditional on a flag passed to ahci_platform_get_resources(). This way we get the shared code for most cases and platforms which need special handling can opt-out. Regards, Hans > > Patrice > >> >> Given how this is one of the more hardware-specific bits, perhaps a >> better way to do this is to move reset handling into a Uniphier driver >> much like Tegra, Mediatek and ST? >> >> That said, I don't see SATA support for any of the Socionext hardware >> either in the DT bindings or drivers/ata, so perhaps it'd be best to >> back this out again until we have something that's more well tested? >> >> Thierry -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 05, 2018 at 03:27:03PM +0200, Hans de Goede wrote: > Hi, > > On 05-04-18 15:17, Patrice CHOTARD wrote: > > Hi Thierry > > > > On 04/05/2018 11:54 AM, Thierry Reding wrote: > > > On Fri, Mar 23, 2018 at 10:30:53AM +0900, Kunihiko Hayashi wrote: > > > > Add support to get and control a list of resets for the device > > > > as optional and shared. These resets must be kept de-asserted until > > > > the device is enabled. > > > > > > > > This is specified as shared because some SoCs like UniPhier series > > > > have common reset controls with all ahci controller instances. > > > > > > > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > > > > --- > > > > .../devicetree/bindings/ata/ahci-platform.txt | 1 + > > > > drivers/ata/ahci.h | 1 + > > > > drivers/ata/libahci_platform.c | 24 +++++++++++++++++++--- > > > > 3 files changed, 23 insertions(+), 3 deletions(-) > > > > > > This causes a regression on Tegra because we explicitly request the > > > resets after the call to ahci_platform_get_resources(). > > > > I confirm, we got exactly the same behavior on STi platform. > > > > > > > > From a quick look, ahci_mtk and ahci_st are in the same boat, adding the > > > corresponding maintainers to Cc. > > > > > > Patrice, Matthias: does SATA still work for you after this patch? This > > > has been in linux-next since next-20180327. > > > > SATA is still working after this patch, but a kernel warning is > > triggered due to the fact that resets are both requested by > > libahci_platform and by ahci_st driver. > > So in your case you might be able to remove the reset handling > from the ahci_st driver and rely on the new libahci_platform > handling instead? If that works that seems like a win to me. > > As said elsewhere in this thread I think it makes sense to keep (or re-add > after a revert) the libahci_platform reset code, but make it conditional > on a flag passed to ahci_platform_get_resources(). This way we get > the shared code for most cases and platforms which need special handling > can opt-out. Agreed, although I prefer such helpers to be opt-in, rather than opt-out. In my experience that tends make the helpers more resilient to this kind of regression. It also simplifies things because instead of drivers saying "I want all the helpers except this one and that one", they can simply say "I want these helpers and that one". In the former case whenever you add some new (opt-out) feature, you have to update all drivers and add the exception. In the latter you only need to extend the drivers that want to make use of the new helper. With that in mind, rather than adding a flag to the ahci_platform_get_resources() function, it might be more flexible to split the helpers into finer-grained functions. That way drivers can pick whatever functionality they want from the helpers. Thierry
Hi, On 05-04-18 15:54, Thierry Reding wrote: > On Thu, Apr 05, 2018 at 03:27:03PM +0200, Hans de Goede wrote: >> Hi, >> >> On 05-04-18 15:17, Patrice CHOTARD wrote: >>> Hi Thierry >>> >>> On 04/05/2018 11:54 AM, Thierry Reding wrote: >>>> On Fri, Mar 23, 2018 at 10:30:53AM +0900, Kunihiko Hayashi wrote: >>>>> Add support to get and control a list of resets for the device >>>>> as optional and shared. These resets must be kept de-asserted until >>>>> the device is enabled. >>>>> >>>>> This is specified as shared because some SoCs like UniPhier series >>>>> have common reset controls with all ahci controller instances. >>>>> >>>>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> >>>>> --- >>>>> .../devicetree/bindings/ata/ahci-platform.txt | 1 + >>>>> drivers/ata/ahci.h | 1 + >>>>> drivers/ata/libahci_platform.c | 24 +++++++++++++++++++--- >>>>> 3 files changed, 23 insertions(+), 3 deletions(-) >>>> >>>> This causes a regression on Tegra because we explicitly request the >>>> resets after the call to ahci_platform_get_resources(). >>> >>> I confirm, we got exactly the same behavior on STi platform. >>> >>>> >>>> From a quick look, ahci_mtk and ahci_st are in the same boat, adding the >>>> corresponding maintainers to Cc. >>>> >>>> Patrice, Matthias: does SATA still work for you after this patch? This >>>> has been in linux-next since next-20180327. >>> >>> SATA is still working after this patch, but a kernel warning is >>> triggered due to the fact that resets are both requested by >>> libahci_platform and by ahci_st driver. >> >> So in your case you might be able to remove the reset handling >> from the ahci_st driver and rely on the new libahci_platform >> handling instead? If that works that seems like a win to me. >> >> As said elsewhere in this thread I think it makes sense to keep (or re-add >> after a revert) the libahci_platform reset code, but make it conditional >> on a flag passed to ahci_platform_get_resources(). This way we get >> the shared code for most cases and platforms which need special handling >> can opt-out. > > Agreed, although I prefer such helpers to be opt-in, rather than > opt-out. In my experience that tends make the helpers more resilient to > this kind of regression. It also simplifies things because instead of > drivers saying "I want all the helpers except this one and that one", > they can simply say "I want these helpers and that one". In the former > case whenever you add some new (opt-out) feature, you have to update all > drivers and add the exception. In the latter you only need to extend the > drivers that want to make use of the new helper. > > With that in mind, rather than adding a flag to the > ahci_platform_get_resources() function, it might be more flexible to > split the helpers into finer-grained functions. That way drivers can > pick whatever functionality they want from the helpers. Good point, so lets: 1) Revert the patch for now 2) Have a new version of the patch which adds a ahci_platform_get_resets() helper 3) Modify the generic drivers/ata/ahci_platform.c driver to call the new ahci_platform_get_resets() between its ahci_platform_get_resources() and ahci_platform_enable_resources() calls. I think that ahci_platform_enable_resources() should still automatically do the right thing wrt resets if ahci_platform_get_resets() was called (otherwise the resets array will be empty and should be skipped) This should make the generic driver usable for the UniPhier SoCs and maybe some other drivers like the ahci_st driver can also switch to the new ahci_platform_get_resets() functionality to reduce their code a bit. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans On 04/05/2018 03:27 PM, Hans de Goede wrote: > Hi, > > On 05-04-18 15:17, Patrice CHOTARD wrote: >> Hi Thierry >> >> On 04/05/2018 11:54 AM, Thierry Reding wrote: >>> On Fri, Mar 23, 2018 at 10:30:53AM +0900, Kunihiko Hayashi wrote: >>>> Add support to get and control a list of resets for the device >>>> as optional and shared. These resets must be kept de-asserted until >>>> the device is enabled. >>>> >>>> This is specified as shared because some SoCs like UniPhier series >>>> have common reset controls with all ahci controller instances. >>>> >>>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> >>>> --- >>>> .../devicetree/bindings/ata/ahci-platform.txt | 1 + >>>> drivers/ata/ahci.h | 1 + >>>> drivers/ata/libahci_platform.c | 24 >>>> +++++++++++++++++++--- >>>> 3 files changed, 23 insertions(+), 3 deletions(-) >>> >>> This causes a regression on Tegra because we explicitly request the >>> resets after the call to ahci_platform_get_resources(). >> >> I confirm, we got exactly the same behavior on STi platform. >> >>> >>> From a quick look, ahci_mtk and ahci_st are in the same boat, >>> adding the >>> corresponding maintainers to Cc. >>> >>> Patrice, Matthias: does SATA still work for you after this patch? This >>> has been in linux-next since next-20180327. >> >> SATA is still working after this patch, but a kernel warning is >> triggered due to the fact that resets are both requested by >> libahci_platform and by ahci_st driver. > > So in your case you might be able to remove the reset handling > from the ahci_st driver and rely on the new libahci_platform > handling instead? If that works that seems like a win to me. Yes, I did a quick test, the reset handling can be removed from our driver. Patrice > > As said elsewhere in this thread I think it makes sense to keep (or re-add > after a revert) the libahci_platform reset code, but make it conditional > on a flag passed to ahci_platform_get_resources(). This way we get > the shared code for most cases and platforms which need special handling > can opt-out. > > Regards, > > Hans > > >> >> Patrice >> >>> >>> Given how this is one of the more hardware-specific bits, perhaps a >>> better way to do this is to move reset handling into a Uniphier driver >>> much like Tegra, Mediatek and ST? >>> >>> That said, I don't see SATA support for any of the Socionext hardware >>> either in the DT bindings or drivers/ata, so perhaps it'd be best to >>> back this out again until we have something that's more well tested? >>> >>> Thierry-- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 05-04-18 16:00, Hans de Goede wrote: > Hi, > > On 05-04-18 15:54, Thierry Reding wrote: >> On Thu, Apr 05, 2018 at 03:27:03PM +0200, Hans de Goede wrote: >>> Hi, >>> >>> On 05-04-18 15:17, Patrice CHOTARD wrote: >>>> Hi Thierry >>>> >>>> On 04/05/2018 11:54 AM, Thierry Reding wrote: >>>>> On Fri, Mar 23, 2018 at 10:30:53AM +0900, Kunihiko Hayashi wrote: >>>>>> Add support to get and control a list of resets for the device >>>>>> as optional and shared. These resets must be kept de-asserted until >>>>>> the device is enabled. >>>>>> >>>>>> This is specified as shared because some SoCs like UniPhier series >>>>>> have common reset controls with all ahci controller instances. >>>>>> >>>>>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> >>>>>> --- >>>>>> .../devicetree/bindings/ata/ahci-platform.txt | 1 + >>>>>> drivers/ata/ahci.h | 1 + >>>>>> drivers/ata/libahci_platform.c | 24 +++++++++++++++++++--- >>>>>> 3 files changed, 23 insertions(+), 3 deletions(-) >>>>> >>>>> This causes a regression on Tegra because we explicitly request the >>>>> resets after the call to ahci_platform_get_resources(). >>>> >>>> I confirm, we got exactly the same behavior on STi platform. >>>> >>>>> >>>>> From a quick look, ahci_mtk and ahci_st are in the same boat, adding the >>>>> corresponding maintainers to Cc. >>>>> >>>>> Patrice, Matthias: does SATA still work for you after this patch? This >>>>> has been in linux-next since next-20180327. >>>> >>>> SATA is still working after this patch, but a kernel warning is >>>> triggered due to the fact that resets are both requested by >>>> libahci_platform and by ahci_st driver. >>> >>> So in your case you might be able to remove the reset handling >>> from the ahci_st driver and rely on the new libahci_platform >>> handling instead? If that works that seems like a win to me. >>> >>> As said elsewhere in this thread I think it makes sense to keep (or re-add >>> after a revert) the libahci_platform reset code, but make it conditional >>> on a flag passed to ahci_platform_get_resources(). This way we get >>> the shared code for most cases and platforms which need special handling >>> can opt-out. >> >> Agreed, although I prefer such helpers to be opt-in, rather than >> opt-out. In my experience that tends make the helpers more resilient to >> this kind of regression. It also simplifies things because instead of >> drivers saying "I want all the helpers except this one and that one", >> they can simply say "I want these helpers and that one". In the former >> case whenever you add some new (opt-out) feature, you have to update all >> drivers and add the exception. In the latter you only need to extend the >> drivers that want to make use of the new helper. Erm, the idea never was to make this opt-out but rather opt in, so we add a flags parameter to ahci_platform_get_resources() and all current users pass in 0 for that to keep the current behavior. And only the generic drivers/ata/ahci_platform.c driver will pass in a the new AHCI_PLATFORM_GET_RESETS flag, which makes ahci_platform_get_resources() (and the other functions) also deal with resets. >> With that in mind, rather than adding a flag to the >> ahci_platform_get_resources() function, it might be more flexible to >> split the helpers into finer-grained functions. That way drivers can >> pick whatever functionality they want from the helpers. > > Good point, so lets: > > 1) Revert the patch for now > 2) Have a new version of the patch which adds a ahci_platform_get_resets() helper > 3) Modify the generic drivers/ata/ahci_platform.c driver to call the new > ahci_platform_get_resets() between its ahci_platform_get_resources() > and ahci_platform_enable_resources() calls. > I think that ahci_platform_enable_resources() should still automatically > do the right thing wrt resets if ahci_platform_get_resets() was called > (otherwise the resets array will be empty and should be skipped) > > This should make the generic driver usable for the UniPhier SoCs and > maybe some other drivers like the ahci_st driver can also switch to the > new ahci_platform_get_resets() functionality to reduce their code a bit. So thinking slightly longer about this, with the opt-in variant (which is what I intended all along) I do think that a flags parameter is better, because the whole idea behind lib_ahci_platform is to avoid having to do err = get_resource_a(), if (err) bail, err = get_resource_b() if (err) bail, etc. in all the ahci (platform) drivers. And having fine grained helpers re-introduces that. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Thu, 5 Apr 2018 16:08:24 +0200 Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 05-04-18 16:00, Hans de Goede wrote: > > Hi, > > > On 05-04-18 15:54, Thierry Reding wrote: > >> On Thu, Apr 05, 2018 at 03:27:03PM +0200, Hans de Goede wrote: > >>> Hi, > >>> > >>> On 05-04-18 15:17, Patrice CHOTARD wrote: > >>>> Hi Thierry > >>>> > >>>> On 04/05/2018 11:54 AM, Thierry Reding wrote: > >>>>> On Fri, Mar 23, 2018 at 10:30:53AM +0900, Kunihiko Hayashi wrote: > >>>>>> Add support to get and control a list of resets for the device > >>>>>> as optional and shared. These resets must be kept de-asserted until > >>>>>> the device is enabled. > >>>>>> > >>>>>> This is specified as shared because some SoCs like UniPhier series > >>>>>> have common reset controls with all ahci controller instances. > >>>>>> > >>>>>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > >>>>>> --- > >>>>>> ??? .../devicetree/bindings/ata/ahci-platform.txt????? |? 1 + > >>>>>> ??? drivers/ata/ahci.h???????????????????????????????? |? 1 + > >>>>>> ??? drivers/ata/libahci_platform.c???????????????????? | 24 +++++++++++++++++++--- > >>>>>> ??? 3 files changed, 23 insertions(+), 3 deletions(-) > >>>>> > >>>>> This causes a regression on Tegra because we explicitly request the > >>>>> resets after the call to ahci_platform_get_resources(). > >>>> > >>>> I confirm, we got exactly the same behavior on STi platform. > >>>> > >>>>> > >>>>> ?? From a quick look, ahci_mtk and ahci_st are in the same boat, adding the > >>>>> corresponding maintainers to Cc. > >>>>> > >>>>> Patrice, Matthias: does SATA still work for you after this patch? This > >>>>> has been in linux-next since next-20180327. > >>>> > >>>> SATA is still working after this patch, but a kernel warning is > >>>> triggered due to the fact that resets are both requested by > >>>> libahci_platform and by ahci_st driver. > >>> > >>> So in your case you might be able to remove the reset handling > >>> from the ahci_st driver and rely on the new libahci_platform > >>> handling instead? If that works that seems like a win to me. > >>> > >>> As said elsewhere in this thread I think it makes sense to keep (or re-add > >>> after a revert) the libahci_platform reset code, but make it conditional > >>> on a flag passed to ahci_platform_get_resources(). This way we get > >>> the shared code for most cases and platforms which need special handling > >>> can opt-out. > >> > >> Agreed, although I prefer such helpers to be opt-in, rather than > >> opt-out. In my experience that tends make the helpers more resilient to > >> this kind of regression. It also simplifies things because instead of > >> drivers saying "I want all the helpers except this one and that one", > >> they can simply say "I want these helpers and that one". In the former > >> case whenever you add some new (opt-out) feature, you have to update all > >> drivers and add the exception. In the latter you only need to extend the > >> drivers that want to make use of the new helper. > > Erm, the idea never was to make this opt-out but rather opt in, so > we add a flags parameter to ahci_platform_get_resources() and all > current users pass in 0 for that to keep the current behavior. > > And only the generic drivers/ata/ahci_platform.c driver will pass > in a the new AHCI_PLATFORM_GET_RESETS flag, which makes > ahci_platform_get_resources() (and the other functions) also deal > with resets. > > >> With that in mind, rather than adding a flag to the > >> ahci_platform_get_resources() function, it might be more flexible to > >> split the helpers into finer-grained functions. That way drivers can > >> pick whatever functionality they want from the helpers. > > > Good point, so lets: > > > 1) Revert the patch for now > > 2) Have a new version of the patch which adds a ahci_platform_get_resets() helper > > 3) Modify the generic drivers/ata/ahci_platform.c driver to call the new > > ?? ahci_platform_get_resets() between its ahci_platform_get_resources() > > ?? and ahci_platform_enable_resources() calls. > > ?? I think that ahci_platform_enable_resources() should still automatically > > ?? do the right thing wrt resets if ahci_platform_get_resets() was called > > ?? (otherwise the resets array will be empty and should be skipped) > > > This should make the generic driver usable for the UniPhier SoCs and > > maybe some other drivers like the ahci_st driver can also switch to the > > new ahci_platform_get_resets() functionality to reduce their code a bit. > > So thinking slightly longer about this, with the opt-in variant > (which is what I intended all along) I do think that a flags parameter > is better, because the whole idea behind lib_ahci_platform is to avoid > having to do err = get_resource_a(), if (err) bail, err = get_resource_b() > if (err) bail, etc. in all the ahci (platform) drivers. And having fine > grained helpers re-introduces that. In case of adding a flag instead of get_resource_a(), for example, we add the flag for use of resets, -struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev) +struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev, + bool use_reset) and for now all the drivers using this function need to add the argument as false to the caller. - hpriv = ahci_platform_get_resources(pdev); + hpriv = ahci_platform_get_resources(pdev, false); Surely this can avoid adding functions such get_resource_a(). If we apply another feature later, we add its flag as one of the arguments instead. Is it right? Thank you, --- Best Regards, Kunihiko Hayashi -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 06-04-18 06:48, Kunihiko Hayashi wrote: > Hi Hans, > > On Thu, 5 Apr 2018 16:08:24 +0200 > Hans de Goede <hdegoede@redhat.com> wrote: > >> Hi, >> >> On 05-04-18 16:00, Hans de Goede wrote: >>> Hi, >>>> On 05-04-18 15:54, Thierry Reding wrote: >>>> On Thu, Apr 05, 2018 at 03:27:03PM +0200, Hans de Goede wrote: >>>>> Hi, >>>>> >>>>> On 05-04-18 15:17, Patrice CHOTARD wrote: >>>>>> Hi Thierry >>>>>> >>>>>> On 04/05/2018 11:54 AM, Thierry Reding wrote: >>>>>>> On Fri, Mar 23, 2018 at 10:30:53AM +0900, Kunihiko Hayashi wrote: >>>>>>>> Add support to get and control a list of resets for the device >>>>>>>> as optional and shared. These resets must be kept de-asserted until >>>>>>>> the device is enabled. >>>>>>>> >>>>>>>> This is specified as shared because some SoCs like UniPhier series >>>>>>>> have common reset controls with all ahci controller instances. >>>>>>>> >>>>>>>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> >>>>>>>> --- >>>>>>>> ??? .../devicetree/bindings/ata/ahci-platform.txt????? |? 1 + >>>>>>>> ??? drivers/ata/ahci.h???????????????????????????????? |? 1 + >>>>>>>> ??? drivers/ata/libahci_platform.c???????????????????? | 24 +++++++++++++++++++--- >>>>>>>> ??? 3 files changed, 23 insertions(+), 3 deletions(-) >>>>>>> >>>>>>> This causes a regression on Tegra because we explicitly request the >>>>>>> resets after the call to ahci_platform_get_resources(). >>>>>> >>>>>> I confirm, we got exactly the same behavior on STi platform. >>>>>> >>>>>>> >>>>>>> ?? From a quick look, ahci_mtk and ahci_st are in the same boat, adding the >>>>>>> corresponding maintainers to Cc. >>>>>>> >>>>>>> Patrice, Matthias: does SATA still work for you after this patch? This >>>>>>> has been in linux-next since next-20180327. >>>>>> >>>>>> SATA is still working after this patch, but a kernel warning is >>>>>> triggered due to the fact that resets are both requested by >>>>>> libahci_platform and by ahci_st driver. >>>>> >>>>> So in your case you might be able to remove the reset handling >>>>> from the ahci_st driver and rely on the new libahci_platform >>>>> handling instead? If that works that seems like a win to me. >>>>> >>>>> As said elsewhere in this thread I think it makes sense to keep (or re-add >>>>> after a revert) the libahci_platform reset code, but make it conditional >>>>> on a flag passed to ahci_platform_get_resources(). This way we get >>>>> the shared code for most cases and platforms which need special handling >>>>> can opt-out. >>>> >>>> Agreed, although I prefer such helpers to be opt-in, rather than >>>> opt-out. In my experience that tends make the helpers more resilient to >>>> this kind of regression. It also simplifies things because instead of >>>> drivers saying "I want all the helpers except this one and that one", >>>> they can simply say "I want these helpers and that one". In the former >>>> case whenever you add some new (opt-out) feature, you have to update all >>>> drivers and add the exception. In the latter you only need to extend the >>>> drivers that want to make use of the new helper. >> >> Erm, the idea never was to make this opt-out but rather opt in, so >> we add a flags parameter to ahci_platform_get_resources() and all >> current users pass in 0 for that to keep the current behavior. >> >> And only the generic drivers/ata/ahci_platform.c driver will pass >> in a the new AHCI_PLATFORM_GET_RESETS flag, which makes >> ahci_platform_get_resources() (and the other functions) also deal >> with resets. >> >>>> With that in mind, rather than adding a flag to the >>>> ahci_platform_get_resources() function, it might be more flexible to >>>> split the helpers into finer-grained functions. That way drivers can >>>> pick whatever functionality they want from the helpers. >>>> Good point, so lets: >>>> 1) Revert the patch for now >>> 2) Have a new version of the patch which adds a ahci_platform_get_resets() helper >>> 3) Modify the generic drivers/ata/ahci_platform.c driver to call the new >>> ?? ahci_platform_get_resets() between its ahci_platform_get_resources() >>> ?? and ahci_platform_enable_resources() calls. >>> ?? I think that ahci_platform_enable_resources() should still automatically >>> ?? do the right thing wrt resets if ahci_platform_get_resets() was called >>> ?? (otherwise the resets array will be empty and should be skipped) >>>> This should make the generic driver usable for the UniPhier SoCs and >>> maybe some other drivers like the ahci_st driver can also switch to the >>> new ahci_platform_get_resets() functionality to reduce their code a bit. >> >> So thinking slightly longer about this, with the opt-in variant >> (which is what I intended all along) I do think that a flags parameter >> is better, because the whole idea behind lib_ahci_platform is to avoid >> having to do err = get_resource_a(), if (err) bail, err = get_resource_b() >> if (err) bail, etc. in all the ahci (platform) drivers. And having fine >> grained helpers re-introduces that. > > In case of adding a flag instead of get_resource_a(), > for example, we add the flag for use of resets, > > -struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev) > +struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev, > + bool use_reset) > > and for now all the drivers using this function need to add the argument as false > to the caller. > > - hpriv = ahci_platform_get_resources(pdev); > + hpriv = ahci_platform_get_resources(pdev, false); > > Surely this can avoid adding functions such get_resource_a(). If we apply another > feature later, we add its flag as one of the arguments instead. Is it right? Yes, that is right, but instead of adding a "bool use_reset" please add an "unsigned int flags" parameter instead and a: #define AHCI_PLATFORM_GET_RESETS 0x01 And update all callers of ahci_platform_get_resources to pass 0 for flags except for drivers/ata/ahci_platform.c. This way we only need to modify all callers once, and if we want to add another optional resource in the future we can add a: #define AHCI_PLATFORM_GET_FOO 0x02 Without needing to change all callers again. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Fri, 6 Apr 2018 10:29:37 +0200 Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 06-04-18 06:48, Kunihiko Hayashi wrote: > > Hi Hans, > > > On Thu, 5 Apr 2018 16:08:24 +0200 > > Hans de Goede <hdegoede@redhat.com> wrote: > > >> Hi, > >> > >> On 05-04-18 16:00, Hans de Goede wrote: > >>> Hi, > >>>> On 05-04-18 15:54, Thierry Reding wrote: > >>>> On Thu, Apr 05, 2018 at 03:27:03PM +0200, Hans de Goede wrote: > >>>>> Hi, > >>>>> > >>>>> On 05-04-18 15:17, Patrice CHOTARD wrote: > >>>>>> Hi Thierry > >>>>>> > >>>>>> On 04/05/2018 11:54 AM, Thierry Reding wrote: > >>>>>>> On Fri, Mar 23, 2018 at 10:30:53AM +0900, Kunihiko Hayashi wrote: > >>>>>>>> Add support to get and control a list of resets for the device > >>>>>>>> as optional and shared. These resets must be kept de-asserted until > >>>>>>>> the device is enabled. > >>>>>>>> > >>>>>>>> This is specified as shared because some SoCs like UniPhier series > >>>>>>>> have common reset controls with all ahci controller instances. > >>>>>>>> > >>>>>>>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > >>>>>>>> --- > >>>>>>>> ??? .../devicetree/bindings/ata/ahci-platform.txt????? |? 1 + > >>>>>>>> ??? drivers/ata/ahci.h???????????????????????????????? |? 1 + > >>>>>>>> ??? drivers/ata/libahci_platform.c???????????????????? | 24 +++++++++++++++++++--- > >>>>>>>> ??? 3 files changed, 23 insertions(+), 3 deletions(-) > >>>>>>> > >>>>>>> This causes a regression on Tegra because we explicitly request the > >>>>>>> resets after the call to ahci_platform_get_resources(). > >>>>>> > >>>>>> I confirm, we got exactly the same behavior on STi platform. > >>>>>> > >>>>>>> > >>>>>>> ?? From a quick look, ahci_mtk and ahci_st are in the same boat, adding the > >>>>>>> corresponding maintainers to Cc. > >>>>>>> > >>>>>>> Patrice, Matthias: does SATA still work for you after this patch? This > >>>>>>> has been in linux-next since next-20180327. > >>>>>> > >>>>>> SATA is still working after this patch, but a kernel warning is > >>>>>> triggered due to the fact that resets are both requested by > >>>>>> libahci_platform and by ahci_st driver. > >>>>> > >>>>> So in your case you might be able to remove the reset handling > >>>>> from the ahci_st driver and rely on the new libahci_platform > >>>>> handling instead? If that works that seems like a win to me. > >>>>> > >>>>> As said elsewhere in this thread I think it makes sense to keep (or re-add > >>>>> after a revert) the libahci_platform reset code, but make it conditional > >>>>> on a flag passed to ahci_platform_get_resources(). This way we get > >>>>> the shared code for most cases and platforms which need special handling > >>>>> can opt-out. > >>>> > >>>> Agreed, although I prefer such helpers to be opt-in, rather than > >>>> opt-out. In my experience that tends make the helpers more resilient to > >>>> this kind of regression. It also simplifies things because instead of > >>>> drivers saying "I want all the helpers except this one and that one", > >>>> they can simply say "I want these helpers and that one". In the former > >>>> case whenever you add some new (opt-out) feature, you have to update all > >>>> drivers and add the exception. In the latter you only need to extend the > >>>> drivers that want to make use of the new helper. > >> > >> Erm, the idea never was to make this opt-out but rather opt in, so > >> we add a flags parameter to ahci_platform_get_resources() and all > >> current users pass in 0 for that to keep the current behavior. > >> > >> And only the generic drivers/ata/ahci_platform.c driver will pass > >> in a the new AHCI_PLATFORM_GET_RESETS flag, which makes > >> ahci_platform_get_resources() (and the other functions) also deal > >> with resets. > >> > >>>> With that in mind, rather than adding a flag to the > >>>> ahci_platform_get_resources() function, it might be more flexible to > >>>> split the helpers into finer-grained functions. That way drivers can > >>>> pick whatever functionality they want from the helpers. > >>>> Good point, so lets: > >>>> 1) Revert the patch for now > >>> 2) Have a new version of the patch which adds a ahci_platform_get_resets() helper > >>> 3) Modify the generic drivers/ata/ahci_platform.c driver to call the new > >>> ?? ahci_platform_get_resets() between its ahci_platform_get_resources() > >>> ?? and ahci_platform_enable_resources() calls. > >>> ?? I think that ahci_platform_enable_resources() should still automatically > >>> ?? do the right thing wrt resets if ahci_platform_get_resets() was called > >>> ?? (otherwise the resets array will be empty and should be skipped) > >>>> This should make the generic driver usable for the UniPhier SoCs and > >>> maybe some other drivers like the ahci_st driver can also switch to the > >>> new ahci_platform_get_resets() functionality to reduce their code a bit. > >> > >> So thinking slightly longer about this, with the opt-in variant > >> (which is what I intended all along) I do think that a flags parameter > >> is better, because the whole idea behind lib_ahci_platform is to avoid > >> having to do err = get_resource_a(), if (err) bail, err = get_resource_b() > >> if (err) bail, etc. in all the ahci (platform) drivers. And having fine > >> grained helpers re-introduces that. > > > In case of adding a flag instead of get_resource_a(), > > for example, we add the flag for use of resets, > > > -struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev) > > +struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev, > > + bool use_reset) > > > and for now all the drivers using this function need to add the argument as false > > to the caller. > > > - hpriv = ahci_platform_get_resources(pdev); > > + hpriv = ahci_platform_get_resources(pdev, false); > > > Surely this can avoid adding functions such get_resource_a(). If we apply another > > feature later, we add its flag as one of the arguments instead. Is it right? > > Yes, that is right, but instead of adding a "bool use_reset" please add > an "unsigned int flags" parameter instead and a: > > #define AHCI_PLATFORM_GET_RESETS 0x01 > > And update all callers of ahci_platform_get_resources to pass 0 for flags > except for drivers/ata/ahci_platform.c. This way we only need to modify > all callers once, and if we want to add another optional resource in > the future we can add a: > > #define AHCI_PLATFORM_GET_FOO 0x02 > > Without needing to change all callers again. Indeed. This is more flexible to add another resources. Although I'm about to prepare the candidate patch to fix this issue, I think we need to revert the previous patch first if some SoCs have issues because of it. Thank you, --- Best Regards, Kunihiko Hayashi -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 06-04-18 11:36, Kunihiko Hayashi wrote: > Hi Hans, > > On Fri, 6 Apr 2018 10:29:37 +0200 > Hans de Goede <hdegoede@redhat.com> wrote: > >> Hi, >> >> On 06-04-18 06:48, Kunihiko Hayashi wrote: >>> Hi Hans, >>>> On Thu, 5 Apr 2018 16:08:24 +0200 >>> Hans de Goede <hdegoede@redhat.com> wrote: >>>>> Hi, >>>> >>>> On 05-04-18 16:00, Hans de Goede wrote: >>>>> Hi, >>>>>> On 05-04-18 15:54, Thierry Reding wrote: >>>>>> On Thu, Apr 05, 2018 at 03:27:03PM +0200, Hans de Goede wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On 05-04-18 15:17, Patrice CHOTARD wrote: >>>>>>>> Hi Thierry >>>>>>>> >>>>>>>> On 04/05/2018 11:54 AM, Thierry Reding wrote: >>>>>>>>> On Fri, Mar 23, 2018 at 10:30:53AM +0900, Kunihiko Hayashi wrote: >>>>>>>>>> Add support to get and control a list of resets for the device >>>>>>>>>> as optional and shared. These resets must be kept de-asserted until >>>>>>>>>> the device is enabled. >>>>>>>>>> >>>>>>>>>> This is specified as shared because some SoCs like UniPhier series >>>>>>>>>> have common reset controls with all ahci controller instances. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> >>>>>>>>>> --- >>>>>>>>>> ??? .../devicetree/bindings/ata/ahci-platform.txt????? |? 1 + >>>>>>>>>> ??? drivers/ata/ahci.h???????????????????????????????? |? 1 + >>>>>>>>>> ??? drivers/ata/libahci_platform.c???????????????????? | 24 +++++++++++++++++++--- >>>>>>>>>> ??? 3 files changed, 23 insertions(+), 3 deletions(-) >>>>>>>>> >>>>>>>>> This causes a regression on Tegra because we explicitly request the >>>>>>>>> resets after the call to ahci_platform_get_resources(). >>>>>>>> >>>>>>>> I confirm, we got exactly the same behavior on STi platform. >>>>>>>> >>>>>>>>> >>>>>>>>> ?? From a quick look, ahci_mtk and ahci_st are in the same boat, adding the >>>>>>>>> corresponding maintainers to Cc. >>>>>>>>> >>>>>>>>> Patrice, Matthias: does SATA still work for you after this patch? This >>>>>>>>> has been in linux-next since next-20180327. >>>>>>>> >>>>>>>> SATA is still working after this patch, but a kernel warning is >>>>>>>> triggered due to the fact that resets are both requested by >>>>>>>> libahci_platform and by ahci_st driver. >>>>>>> >>>>>>> So in your case you might be able to remove the reset handling >>>>>>> from the ahci_st driver and rely on the new libahci_platform >>>>>>> handling instead? If that works that seems like a win to me. >>>>>>> >>>>>>> As said elsewhere in this thread I think it makes sense to keep (or re-add >>>>>>> after a revert) the libahci_platform reset code, but make it conditional >>>>>>> on a flag passed to ahci_platform_get_resources(). This way we get >>>>>>> the shared code for most cases and platforms which need special handling >>>>>>> can opt-out. >>>>>> >>>>>> Agreed, although I prefer such helpers to be opt-in, rather than >>>>>> opt-out. In my experience that tends make the helpers more resilient to >>>>>> this kind of regression. It also simplifies things because instead of >>>>>> drivers saying "I want all the helpers except this one and that one", >>>>>> they can simply say "I want these helpers and that one". In the former >>>>>> case whenever you add some new (opt-out) feature, you have to update all >>>>>> drivers and add the exception. In the latter you only need to extend the >>>>>> drivers that want to make use of the new helper. >>>> >>>> Erm, the idea never was to make this opt-out but rather opt in, so >>>> we add a flags parameter to ahci_platform_get_resources() and all >>>> current users pass in 0 for that to keep the current behavior. >>>> >>>> And only the generic drivers/ata/ahci_platform.c driver will pass >>>> in a the new AHCI_PLATFORM_GET_RESETS flag, which makes >>>> ahci_platform_get_resources() (and the other functions) also deal >>>> with resets. >>>> >>>>>> With that in mind, rather than adding a flag to the >>>>>> ahci_platform_get_resources() function, it might be more flexible to >>>>>> split the helpers into finer-grained functions. That way drivers can >>>>>> pick whatever functionality they want from the helpers. >>>>>> Good point, so lets: >>>>>> 1) Revert the patch for now >>>>> 2) Have a new version of the patch which adds a ahci_platform_get_resets() helper >>>>> 3) Modify the generic drivers/ata/ahci_platform.c driver to call the new >>>>> ?? ahci_platform_get_resets() between its ahci_platform_get_resources() >>>>> ?? and ahci_platform_enable_resources() calls. >>>>> ?? I think that ahci_platform_enable_resources() should still automatically >>>>> ?? do the right thing wrt resets if ahci_platform_get_resets() was called >>>>> ?? (otherwise the resets array will be empty and should be skipped) >>>>>> This should make the generic driver usable for the UniPhier SoCs and >>>>> maybe some other drivers like the ahci_st driver can also switch to the >>>>> new ahci_platform_get_resets() functionality to reduce their code a bit. >>>> >>>> So thinking slightly longer about this, with the opt-in variant >>>> (which is what I intended all along) I do think that a flags parameter >>>> is better, because the whole idea behind lib_ahci_platform is to avoid >>>> having to do err = get_resource_a(), if (err) bail, err = get_resource_b() >>>> if (err) bail, etc. in all the ahci (platform) drivers. And having fine >>>> grained helpers re-introduces that. >>>> In case of adding a flag instead of get_resource_a(), >>> for example, we add the flag for use of resets, >>>> -struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev) >>> +struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev, >>> + bool use_reset) >>>> and for now all the drivers using this function need to add the argument as false >>> to the caller. >>>> - hpriv = ahci_platform_get_resources(pdev); >>> + hpriv = ahci_platform_get_resources(pdev, false); >>>> Surely this can avoid adding functions such get_resource_a(). If we apply another >>> feature later, we add its flag as one of the arguments instead. Is it right? >> >> Yes, that is right, but instead of adding a "bool use_reset" please add >> an "unsigned int flags" parameter instead and a: >> >> #define AHCI_PLATFORM_GET_RESETS 0x01 >> >> And update all callers of ahci_platform_get_resources to pass 0 for flags >> except for drivers/ata/ahci_platform.c. This way we only need to modify >> all callers once, and if we want to add another optional resource in >> the future we can add a: >> >> #define AHCI_PLATFORM_GET_FOO 0x02 >> >> Without needing to change all callers again. > > Indeed. This is more flexible to add another resources. > > Although I'm about to prepare the candidate patch to fix this issue, > I think we need to revert the previous patch first if some SoCs have > issues because of it. Ack. It is probably best if you do a "git revert f0f56716fc3e5d547fd7811eb218a30ed0695605" on a tree with that commit and then send a patch to Tejun for this. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 06, 2018 at 10:29:37AM +0200, Hans de Goede wrote: > Hi, > > On 06-04-18 06:48, Kunihiko Hayashi wrote: > > Hi Hans, > > > > On Thu, 5 Apr 2018 16:08:24 +0200 > > Hans de Goede <hdegoede@redhat.com> wrote: > > > > > Hi, > > > > > > On 05-04-18 16:00, Hans de Goede wrote: > > > > Hi, > > > > > On 05-04-18 15:54, Thierry Reding wrote: > > > > > On Thu, Apr 05, 2018 at 03:27:03PM +0200, Hans de Goede wrote: > > > > > > Hi, > > > > > > > > > > > > On 05-04-18 15:17, Patrice CHOTARD wrote: > > > > > > > Hi Thierry > > > > > > > > > > > > > > On 04/05/2018 11:54 AM, Thierry Reding wrote: > > > > > > > > On Fri, Mar 23, 2018 at 10:30:53AM +0900, Kunihiko Hayashi wrote: > > > > > > > > > Add support to get and control a list of resets for the device > > > > > > > > > as optional and shared. These resets must be kept de-asserted until > > > > > > > > > the device is enabled. > > > > > > > > > > > > > > > > > > This is specified as shared because some SoCs like UniPhier series > > > > > > > > > have common reset controls with all ahci controller instances. > > > > > > > > > > > > > > > > > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > > > > > > > > > --- > > > > > > > > > ??? .../devicetree/bindings/ata/ahci-platform.txt????? |? 1 + > > > > > > > > > ??? drivers/ata/ahci.h???????????????????????????????? |? 1 + > > > > > > > > > ??? drivers/ata/libahci_platform.c???????????????????? | 24 +++++++++++++++++++--- > > > > > > > > > ??? 3 files changed, 23 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > > > This causes a regression on Tegra because we explicitly request the > > > > > > > > resets after the call to ahci_platform_get_resources(). > > > > > > > > > > > > > > I confirm, we got exactly the same behavior on STi platform. > > > > > > > > > > > > > > > > > > > > > > > ?? From a quick look, ahci_mtk and ahci_st are in the same boat, adding the > > > > > > > > corresponding maintainers to Cc. > > > > > > > > > > > > > > > > Patrice, Matthias: does SATA still work for you after this patch? This > > > > > > > > has been in linux-next since next-20180327. > > > > > > > > > > > > > > SATA is still working after this patch, but a kernel warning is > > > > > > > triggered due to the fact that resets are both requested by > > > > > > > libahci_platform and by ahci_st driver. > > > > > > > > > > > > So in your case you might be able to remove the reset handling > > > > > > from the ahci_st driver and rely on the new libahci_platform > > > > > > handling instead? If that works that seems like a win to me. > > > > > > > > > > > > As said elsewhere in this thread I think it makes sense to keep (or re-add > > > > > > after a revert) the libahci_platform reset code, but make it conditional > > > > > > on a flag passed to ahci_platform_get_resources(). This way we get > > > > > > the shared code for most cases and platforms which need special handling > > > > > > can opt-out. > > > > > > > > > > Agreed, although I prefer such helpers to be opt-in, rather than > > > > > opt-out. In my experience that tends make the helpers more resilient to > > > > > this kind of regression. It also simplifies things because instead of > > > > > drivers saying "I want all the helpers except this one and that one", > > > > > they can simply say "I want these helpers and that one". In the former > > > > > case whenever you add some new (opt-out) feature, you have to update all > > > > > drivers and add the exception. In the latter you only need to extend the > > > > > drivers that want to make use of the new helper. > > > > > > Erm, the idea never was to make this opt-out but rather opt in, so > > > we add a flags parameter to ahci_platform_get_resources() and all > > > current users pass in 0 for that to keep the current behavior. > > > > > > And only the generic drivers/ata/ahci_platform.c driver will pass > > > in a the new AHCI_PLATFORM_GET_RESETS flag, which makes > > > ahci_platform_get_resources() (and the other functions) also deal > > > with resets. > > > > > > > > With that in mind, rather than adding a flag to the > > > > > ahci_platform_get_resources() function, it might be more flexible to > > > > > split the helpers into finer-grained functions. That way drivers can > > > > > pick whatever functionality they want from the helpers. > > > > > Good point, so lets: > > > > > 1) Revert the patch for now > > > > 2) Have a new version of the patch which adds a ahci_platform_get_resets() helper > > > > 3) Modify the generic drivers/ata/ahci_platform.c driver to call the new > > > > ?? ahci_platform_get_resets() between its ahci_platform_get_resources() > > > > ?? and ahci_platform_enable_resources() calls. > > > > ?? I think that ahci_platform_enable_resources() should still automatically > > > > ?? do the right thing wrt resets if ahci_platform_get_resets() was called > > > > ?? (otherwise the resets array will be empty and should be skipped) > > > > > This should make the generic driver usable for the UniPhier SoCs and > > > > maybe some other drivers like the ahci_st driver can also switch to the > > > > new ahci_platform_get_resets() functionality to reduce their code a bit. > > > > > > So thinking slightly longer about this, with the opt-in variant > > > (which is what I intended all along) I do think that a flags parameter > > > is better, because the whole idea behind lib_ahci_platform is to avoid > > > having to do err = get_resource_a(), if (err) bail, err = get_resource_b() > > > if (err) bail, etc. in all the ahci (platform) drivers. And having fine > > > grained helpers re-introduces that. > > > > In case of adding a flag instead of get_resource_a(), > > for example, we add the flag for use of resets, > > > > -struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev) > > +struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev, > > + bool use_reset) > > > > and for now all the drivers using this function need to add the argument as false > > to the caller. > > > > - hpriv = ahci_platform_get_resources(pdev); > > + hpriv = ahci_platform_get_resources(pdev, false); > > > > Surely this can avoid adding functions such get_resource_a(). If we apply another > > feature later, we add its flag as one of the arguments instead. Is it right? > > Yes, that is right, but instead of adding a "bool use_reset" please add > an "unsigned int flags" parameter instead and a: > > #define AHCI_PLATFORM_GET_RESETS 0x01 > > And update all callers of ahci_platform_get_resources to pass 0 for flags > except for drivers/ata/ahci_platform.c. This way we only need to modify > all callers once, and if we want to add another optional resource in > the future we can add a: > > #define AHCI_PLATFORM_GET_FOO 0x02 > > Without needing to change all callers again. On a side-note, I also think the symbolic flags make the code easier to read. Having something like: ahci_platform_get_resources(pdev, true, false, true); becomes really difficult to understand. Thierry
diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt index c760ecb..f4006d3 100644 --- a/Documentation/devicetree/bindings/ata/ahci-platform.txt +++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt @@ -30,6 +30,7 @@ compatible: Optional properties: - dma-coherent : Present if dma operations are coherent - clocks : a list of phandle + clock specifier pairs +- resets : a list of phandle + reset specifier pairs - target-supply : regulator for SATA target power - phys : reference to the SATA PHY node - phy-names : must be "sata-phy" diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h index 749fd94..47ec77a2 100644 --- a/drivers/ata/ahci.h +++ b/drivers/ata/ahci.h @@ -347,6 +347,7 @@ struct ahci_host_priv { u32 em_msg_type; /* EM message type */ bool got_runtime_pm; /* Did we do pm_runtime_get? */ struct clk *clks[AHCI_MAX_CLKS]; /* Optional */ + struct reset_control *rsts; /* Optional */ struct regulator **target_pwrs; /* Optional */ /* * If platform uses PHYs. There is a 1:1 relation between the port number and diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c index 341d0ef..3130db9 100644 --- a/drivers/ata/libahci_platform.c +++ b/drivers/ata/libahci_platform.c @@ -25,6 +25,7 @@ #include <linux/phy/phy.h> #include <linux/pm_runtime.h> #include <linux/of_platform.h> +#include <linux/reset.h> #include "ahci.h" static void ahci_host_stop(struct ata_host *host); @@ -195,7 +196,8 @@ EXPORT_SYMBOL_GPL(ahci_platform_disable_regulators); * following order: * 1) Regulator * 2) Clocks (through ahci_platform_enable_clks) - * 3) Phys + * 3) Resets + * 4) Phys * * If resource enabling fails at any point the previous enabled resources * are disabled in reverse order. @@ -215,12 +217,19 @@ int ahci_platform_enable_resources(struct ahci_host_priv *hpriv) if (rc) goto disable_regulator; - rc = ahci_platform_enable_phys(hpriv); + rc = reset_control_deassert(hpriv->rsts); if (rc) goto disable_clks; + rc = ahci_platform_enable_phys(hpriv); + if (rc) + goto disable_resets; + return 0; +disable_resets: + reset_control_assert(hpriv->rsts); + disable_clks: ahci_platform_disable_clks(hpriv); @@ -239,12 +248,15 @@ EXPORT_SYMBOL_GPL(ahci_platform_enable_resources); * following order: * 1) Phys * 2) Clocks (through ahci_platform_disable_clks) - * 3) Regulator + * 3) Resets + * 4) Regulator */ void ahci_platform_disable_resources(struct ahci_host_priv *hpriv) { ahci_platform_disable_phys(hpriv); + reset_control_assert(hpriv->rsts); + ahci_platform_disable_clks(hpriv); ahci_platform_disable_regulators(hpriv); @@ -393,6 +405,12 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev) hpriv->clks[i] = clk; } + hpriv->rsts = devm_reset_control_array_get_optional_shared(dev); + if (IS_ERR(hpriv->rsts)) { + rc = PTR_ERR(hpriv->rsts); + goto err_out; + } + hpriv->nports = child_nodes = of_get_child_count(dev->of_node); /*
Add support to get and control a list of resets for the device as optional and shared. These resets must be kept de-asserted until the device is enabled. This is specified as shared because some SoCs like UniPhier series have common reset controls with all ahci controller instances. Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> --- .../devicetree/bindings/ata/ahci-platform.txt | 1 + drivers/ata/ahci.h | 1 + drivers/ata/libahci_platform.c | 24 +++++++++++++++++++--- 3 files changed, 23 insertions(+), 3 deletions(-)