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

[Thierry Reding]

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@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().
> > 
> > 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: signature.asc
Description: PGP signature