Re: [PATCH] ata: ahci-platform: add reset control support

Re: [PATCH] ata: ahci-platform: add reset control support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1469 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ata: ahci-platform: add reset control support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1849 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ata: ahci-platform: add reset control support

[Kunihiko Hayashi]

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

Re: [PATCH] ata: ahci-platform: add reset control support

[Hans de Goede]

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

Re: [PATCH] ata: ahci-platform: add reset control support

[Patrice CHOTARD]

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-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
>> ---
>>   .../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
> 

Re: [PATCH] ata: ahci-platform: add reset control support

[Hans de Goede]

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

Re: [PATCH] ata: ahci-platform: add reset control support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3005 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ata: ahci-platform: add reset control support

[Hans de Goede]

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

Re: [PATCH] ata: ahci-platform: add reset control support

[Hans de Goede]

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

Re: [PATCH] ata: ahci-platform: add reset control support

[Kunihiko Hayashi]

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

Re: [PATCH] ata: ahci-platform: add reset control support

[Hans de Goede]

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

Re: [PATCH] ata: ahci-platform: add reset control support

[Kunihiko Hayashi]

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

Re: [PATCH] ata: ahci-platform: add reset control support

[Hans de Goede]

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

Re: [PATCH] ata: ahci-platform: add reset control support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 7277 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ata: ahci-platform: add reset control support

[Patrice CHOTARD]

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-uWyLwvC0a2jby3iVrkZq2A@public.gmane.org>
>>>> ---
>>>>    .../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