On Fri, Apr 06, 2018 at 10:29:37AM +0200, Hans de Goede wrote:

Hi,

On 06-04-18 06:48, Kunihiko Hayashi wrote:

quoted

Hi Hans,

On Thu, 5 Apr 2018 16:08:24 +0200
Hans de Goede [off-list ref] wrote:

quoted

Hi,

On 05-04-18 16:00, Hans de Goede wrote:

quoted

Hi,

quoted

On 05-04-18 15:54, Thierry Reding wrote:
On Thu, Apr 05, 2018 at 03:27:03PM +0200, Hans de Goede wrote:

quoted

Hi,

On 05-04-18 15:17, Patrice CHOTARD wrote:

quoted

Hi Thierry

On 04/05/2018 11:54 AM, Thierry Reding wrote:

quoted

On Fri, Mar 23, 2018 at 10:30:53AM +0900, Kunihiko Hayashi wrote:

quoted

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.

quoted

?? 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.

quoted

quoted

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)

quoted

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