Messages in this thread |  | | Subject | Re: [PATCH] ata: ahci-platform: add reset control support | From | Hans de Goede <> | Date | Thu, 5 Apr 2018 13:30:35 +0200 |
| |
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
|  |