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

From: Hans de Goede
Date: Thu Apr 05 2018 - 07:30:46 EST


On 05-04-18 13:23, Kunihiko Hayashi wrote:
Hi Thierry,

On Thu, 5 Apr 2018 11:54:29 +0200
Thierry Reding <thierry.reding@xxxxxxxxx> 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@xxxxxxxxxxxxx>
.../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.