[v1 3/3] imx: ahci: enable ahci sata on imx6q platforms

[Zhu Richard-R65037]

Hi Shawn:
It's sounds right, platform is not right place to handle device/IP stuff.
I would kick off to your option2. Thanks.

Best Regards
Richard Zhu

-----Original Message-----
From: Shawn Guo [mailto:shawn.guo at linaro.org] 
Sent: Tuesday, June 18, 2013 1:59 PM
To: Zhu Richard-R65037
Cc: Richard Zhu; linux-arm-kernel at lists.infradead.org; jgarzik at pobox.com; linux-ide at vger.kernel.org
Subject: Re: [v1 3/3] imx: ahci: enable ahci sata on imx6q platforms

On Tue, Jun 18, 2013 at 05:00:09AM +0000, Zhu Richard-R65037 wrote:
> > +enum {
> > +     HOST_CAP = 0x00,
> > +     HOST_CAP_SSS = (1 << 27), /* Staggered Spin-up */
> > +     HOST_CTL = 0x04, /* global host control */
> > +     HOST_RESET = (1 << 0),  /* reset controller; self-clear */
> > +     HOST_PORTS_IMPL = 0x0c, /* bitmap of implemented ports */
> > +     HOST_TIMER1MS = 0xe0, /* Timer 1-ms */
> > +     HOST_VERSIONR = 0xf8, /* host version register*/
> > +
> > +     PORT_SATA_SR = 0x128, /* Port0 SATA Status */
> > +     PORT_PHY_CTL = 0x178, /* Port0 PHY Control */
> > +     PORT_PHY_CTL_PDDQ_LOC = 0x100000, /* PORT_PHY_CTL bits */ };
> > +
> 
> This is all about IP stuff.  Some of them are just replication of 
> definitions found in drivers/ata/ahci.h. I do not understand why we 
> need them in platform.  It's a sign to me that we are doing something 
> in platform code that should really be done in the device driver, no?
> 
> [Richard] Regarding to the AHCI SPEC, some bits of the ahci sata 
> registers are  specified to be RO, but they are RW in the design of fsl ahci ata in actually.
>  So the configurations of those bits are mandatory required in the  
> platform sata initialization.
> One HBA reset is better finished before start the configuration.
> That's why those registers/bits are re-defined in the platform level.
> 
> For, e.x:
> bit1 of the HOST_POSRTS_IMPL, HOST_CAP_SSS(bit27) of the HOST_CAP, are 
> RO specified in the AHCI SPEC. But they should be configured in the sata platform initialization.
> 
> BTW, HOST_TIMER1MS is defined by FSL, can't be used in the common codes.
> regarding to the AHCI SPEC.
> " Registers at offset A0h to FFh are vendor specific"

It sounds like to me that the generic ahci_platform driver does not perfectly fit imx6q sata device.  You're pushing all those bits that are not covered by the generic ahci_platform driver down to platform code.  This is not the right thing to do, and platform is not the right place to handle device/IP stuff.  I see two options you can go as below.

1. Create an imx6q sata specific compatible string and add code into
   generic ahci_platform driver to implement programming model for imx6q
   type of device.

2. The existing ahci_platform driver is not so useful for imx6q sata
   considering you have so many imx6q custom bits to add.  It might be
   more appropriate to create a custom ahci_platform driver for imx6q
   sata device with forking the generic one, just like what
   sata_highbank does.

I would go for option 2.

Shawn