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 <[email protected]>
---
.../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);
/*
--
2.7.4
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 <[email protected]>
Patch looks good to me:
Reviewed-by: Hans de Goede <[email protected]>
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);
>
> /*
>
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 <[email protected]>
> ---
> .../devicetree/bindings/ata/ahci-platform.txt | 1 +
Reviewed-by: Rob Herring <[email protected]>
> drivers/ata/ahci.h | 1 +
> drivers/ata/libahci_platform.c | 24 +++++++++++++++++++---
> 3 files changed, 23 insertions(+), 3 deletions(-)
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 <[email protected]>
> ---
> .../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 <[email protected]>
> > ---
> > .../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 <[email protected]> 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 <[email protected]>
> > ---
> > .../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
Hi,
On 05-04-18 13:23, Kunihiko Hayashi wrote:
> Hi Thierry,
>
> On Thu, 5 Apr 2018 11:54:29 +0200
> Thierry Reding <[email protected]> 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 <[email protected]>
>>> ---
>>> .../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
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 <[email protected]>
>> ---
>> .../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
>
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 <[email protected]>
>>> ---
>>> .../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
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 <[email protected]>
> > > > ---
> > > > .../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 <[email protected]>
>>>>> ---
>>>>> .../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
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 <[email protected]>
>>>> ---
>>>> ?? .../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
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 <[email protected]>
>>>>>> ---
>>>>>> ??? .../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
Hi Hans,
On Thu, 5 Apr 2018 16:08:24 +0200
Hans de Goede <[email protected]> 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 <[email protected]>
> >>>>>> ---
> >>>>>> ??? .../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
Hi,
On 06-04-18 06:48, Kunihiko Hayashi wrote:
> Hi Hans,
>
> On Thu, 5 Apr 2018 16:08:24 +0200
> Hans de Goede <[email protected]> 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 <[email protected]>
>>>>>>>> ---
>>>>>>>> ??? .../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
Hi Hans,
On Fri, 6 Apr 2018 10:29:37 +0200
Hans de Goede <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>>>>>>> ---
> >>>>>>>> ??? .../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
Hi,
On 06-04-18 11:36, Kunihiko Hayashi wrote:
> Hi Hans,
>
> On Fri, 6 Apr 2018 10:29:37 +0200
> Hans de Goede <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>>>>>>> ---
>>>>>>>>>> ??? .../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
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 <[email protected]> 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 <[email protected]>
> > > > > > > > > ---
> > > > > > > > > ??? .../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