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

[Shawn Guo]

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