[U-Boot] [linux-sunxi] Re: [PATCH v2 3/3] ahci: provide sunxi SATA driver using AHCI platform framework

[Ian Campbell]

On Thu, 2014-02-20 at 14:06 -0600, Rob Herring wrote:
> On Thu, Feb 20, 2014 at 10:06 AM, Ian Campbell <ijc at hellion.org.uk> wrote:
> > On Thu, 2014-02-20 at 09:24 -0600, Rob Herring wrote:
> >> > +#define AHCI_PHYCS0R 0x00c0
> >> > +#define AHCI_PHYCS1R 0x00c4
> >> > +#define AHCI_PHYCS2R 0x00c8
> > [...]
> >> > +#define AHCI_RWCR 0x00fc
> >
> >> These registers are not sunxi specific, but part of a certain vendor's
> >> IP found in several SOCs. I can't tell you who, but it shouldn't be
> >> too hard to figure out.
> >
> > Actually, only the 4 above are used here and if I'm guessing which
> > certain vendor you mean correctly then the code for those has these in
> > its register map as reserved and doesn't touch them (this is true in
> > both of the similar drivers I looked at).
> >
> > The rest of the registers in that list did look a lot the DW part
> > (judging from the existing u-boot drivers) though.
> 
> There may be others that do this setup in firmware.

Someone pointed me to http://linux-sunxi.org/Used_IP_cores which
suggests that while the SATA block is DW the PHY might be Allwinner's
own -- that would fit with having found a bunch of unusual registers in
a gap in the DW address map...

In v3 I'll drop all the unused #defines, which apart from being the sane
thing to do ought to make it look less like I'm duplicating DW stuff
here.

> >> > +#define BIT(x) (1<<x)
> >> > +static u32 sunxi_getbits(u8 *reg, u8 mask, u8 shift)
> >> > +{
> >> > +       return (readl(reg) >> shift) & mask;
> >> > +}
> >> > +
> >> > +static int sunxi_ahci_phy_init(u32 base)
> >> > +{
> >> [...magic...]
> >> > +
> >>
> >> I would guess this code or something very similar already exists in u-boot.
> >
> > I've had a look in the most obvious files in drivers/block/ and I don't
> > see anything. Perhaps I should look harder.
> >
> > FWIW I also couldn't find anything similar in linux/drivers/ata.
> 
> I thought iMX needed something like this, but it doesn't look like it
> now. Perhaps they figured out the bootrom is doing all this and it is
> not really necessary to redo.
> 
> I don't really have any concrete suggestions here. I'm just
> highlighting potential duplication.

Thanks for doing so, I don't want to be responsible for YASD (Yet
Another SATA Driver...)

>  We already have 2 AHCI drivers in
> u-boot. I think dwc_ahsata.c is the cleaner implementation, but ahci.c
> is probably more well tested now. The Chromium folks have done various
> fixes as has Calxeda. I think dwc_ahsata.c is only used for i.MX and
> SATA is not the primary storage interface for most i.MX designs.

It looked to me like the ahci.c one was a better bet going forward,
since it seems more generic etc and it supports the "platform ahci"
model, which was a good fit. Also I knew ahci.c was good from my use on
the Calxeda platforms.

I could try switching to dwc_ahsata.c if people strongly prefer.

Ian.