| Message ID | 1429757950-28789-4-git-send-email-computersforpeace@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday 22 April 2015 19:59:08 Brian Norris wrote: > Pretty straightforward driver, using the nice library-ization of the > generic ahci_platform driver. > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> There is an alternative way to do this, by writing a separate phy driver for drivers/phy and using the generic ahci-platform driver. Have you considered that as well? I don't know which approach works better here, but in general we should try to have the generic driver handle as many chips as possible. > diff --git a/drivers/ata/sata_brcmstb.c b/drivers/ata/sata_brcmstb.c > new file mode 100644 > index 000000000000..ab8d2261fa96 > --- /dev/null > +++ b/drivers/ata/sata_brcmstb.c > @@ -0,0 +1,285 @@ > +/* > + * Broadcom SATA3 AHCI Controller Driver Is this AHCI or not? Most AHCI drivers are called ahci_*.c, not sata_*.c > +#ifdef __BIG_ENDIAN > +#define DATA_ENDIAN 2 /* AHCI->DDR inbound accesses */ > +#define MMIO_ENDIAN 2 /* CPU->AHCI outbound accesses */ > +#else > +#define DATA_ENDIAN 0 > +#define MMIO_ENDIAN 0 > +#endif Is this for MIPS or ARM based chips or both? ARM SoCs should never care about the endianess of the CPU, so I'd expect something like #if defined(__BIG_ENDIAN) && defined(CONFIG_MIPS) /* mips big-endian stuff */ #else /* all other combinations */ #endif > +static void brcm_sata_phy_enable(struct brcm_ahci_priv *priv, int port) > +{ > + void __iomem *phyctrl = priv->top_ctrl + SATA_TOP_CTRL_PHY_CTRL + > + (port * SATA_TOP_CTRL_PHY_OFFS); > + void __iomem *p; > + u32 reg; > + > + /* clear PHY_DEFAULT_POWER_STATE */ > + p = phyctrl + SATA_TOP_CTRL_PHY_CTRL_1; > + reg = __raw_readl(p); > + reg &= ~SATA_TOP_CTRL_1_PHY_DEFAULT_POWER_STATE; > + __raw_writel(reg, p); Similarly, the use of __raw_readl() is broken on ARM here and won't work on big-endian kernels. Better use a driver specific helper function that does the right thing based on the architecture and endianess. You can use the same conditional as above and do #if defined(__BIG_ENDIAN) && defined(CONFIG_MIPS) static inline brcm_sata_phy_read(struct brcm_ahci_phy *priv, int port int reg) { void __iomem *phyctrl = priv->regs; if (IS_ENABLED(CONFIG_MIPS) && IS_ENABLED(__BIG_ENDIAN)) return __raw_readl(regs->reg); return readl_relaxed(regs->reg); } > +static const struct of_device_id ahci_of_match[] = { > + {.compatible = "brcm,sata3-ahci"}, > + {}, This sounds awefully generic. Can you guarantee that no part of Broadcom has produced another ahci-like SATA3 controller in the past, or will have one in the future? If not, please be more specific here, and use the internal specifier for this version of the IP block. If you can't find out what that is, use the identifier for the oldest chip you know that has it. Arnd
Hi Arnd, On Thu, Apr 23, 2015 at 09:43:55AM +0200, Arnd Bergmann wrote: > On Wednesday 22 April 2015 19:59:08 Brian Norris wrote: > > Pretty straightforward driver, using the nice library-ization of the > > generic ahci_platform driver. > > > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > > There is an alternative way to do this, by writing a separate phy driver > for drivers/phy and using the generic ahci-platform driver. Have you > considered that as well? I don't know which approach works better here, > but in general we should try to have the generic driver handle as many > chips as possible. Did you read v1 [1]? There was discussion all over the place, with each person recommending their own favorite Right Way. It was suggested in various ways that code be moved into drivers/ata/ or drivers/phy/ at various points. In the end, I couldn't follow all suggestions and instead came up with this: 1. SATA_TOP_CTRL deals with AHCI-related registers and some high-level PHY bits (power on or off the PHY). 2. There is another discrete bit of hardware that handles analog aspects of the PHY 3. The device tree should describe hardware, not how software wants to use it 4. Software should avoid sharing register ranges So, this suggested we should have two DT nodes; one for #1 and one for #2. It seemed natural that because #1 modifies the AHCI core (e.g., the endianness of it), that it belongs in a 'SATA' driver. So I use libahci_platform instead of using the generic driver. The bits from #2 go in drivers/phy/. So we have a PHY driver, but it doesn't cover everything. Did you want me to write a second PHY driver?? I'm not sure how that'd work... > > diff --git a/drivers/ata/sata_brcmstb.c b/drivers/ata/sata_brcmstb.c > > new file mode 100644 > > index 000000000000..ab8d2261fa96 > > --- /dev/null > > +++ b/drivers/ata/sata_brcmstb.c > > @@ -0,0 +1,285 @@ > > +/* > > + * Broadcom SATA3 AHCI Controller Driver > > Is this AHCI or not? Most AHCI drivers are called ahci_*.c, not sata_*.c It is AHCI. I can rename if necessary... If you're wondering about the comment there, SATA3 is left to indicate the difference between earlier (non-AHCI) SATA cores that only supported up to Gen2 SATA. > > +#ifdef __BIG_ENDIAN > > +#define DATA_ENDIAN 2 /* AHCI->DDR inbound accesses */ > > +#define MMIO_ENDIAN 2 /* CPU->AHCI outbound accesses */ > > +#else > > +#define DATA_ENDIAN 0 > > +#define MMIO_ENDIAN 0 > > +#endif > > Is this for MIPS or ARM based chips or both? Both. (This particular iteration of the driver was only tested on ARM, but this particular code has been the same on MIPS.) > ARM SoCs should never care > about the endianess of the CPU, Why do you say that? What makes ARM different than MIPS here? If somebody were using ARM BE, then they would (just like in MIPS) need to configure our AHCI core to pretend like it's in LE mode, as that's what libahci.c assumes. > so I'd expect something like > > #if defined(__BIG_ENDIAN) && defined(CONFIG_MIPS) > /* mips big-endian stuff */ > #else > /* all other combinations */ > #endif > > > +static void brcm_sata_phy_enable(struct brcm_ahci_priv *priv, int port) > > +{ > > + void __iomem *phyctrl = priv->top_ctrl + SATA_TOP_CTRL_PHY_CTRL + > > + (port * SATA_TOP_CTRL_PHY_OFFS); > > + void __iomem *p; > > + u32 reg; > > + > > + /* clear PHY_DEFAULT_POWER_STATE */ > > + p = phyctrl + SATA_TOP_CTRL_PHY_CTRL_1; > > + reg = __raw_readl(p); > > + reg &= ~SATA_TOP_CTRL_1_PHY_DEFAULT_POWER_STATE; > > + __raw_writel(reg, p); > > Similarly, the use of __raw_readl() is broken on ARM here and won't > work on big-endian kernels. Better use a driver specific helper > function that does the right thing based on the architecture and > endianess. You can use the same conditional as above and do > > #if defined(__BIG_ENDIAN) && defined(CONFIG_MIPS) > > static inline brcm_sata_phy_read(struct brcm_ahci_phy *priv, int port int reg) > { > void __iomem *phyctrl = priv->regs; > > if (IS_ENABLED(CONFIG_MIPS) && IS_ENABLED(__BIG_ENDIAN)) > return __raw_readl(regs->reg); > > return readl_relaxed(regs->reg); > } Huh? I'm fine with a driver-specific helper to abstract this out, but why the MIPS fiddling? I'm fairly confident that in *all* configurations, this piece of the IP is wired up to work in native endianness, so it should never be doing an endian swap. The readl_relaxed() you're adding looks like it would just break the (theoretical) ARM BE case. I understand that you don't like bare __raw_{read,write}l(), but some IP is just wired this way. > > +static const struct of_device_id ahci_of_match[] = { > > + {.compatible = "brcm,sata3-ahci"}, > > + {}, > > This sounds awefully generic. Can you guarantee that no part of Broadcom > has produced another ahci-like SATA3 controller in the past, or will > have one in the future? I know this controller is used by several chips across Broadcom, though I can't guarantee there are not others at the moment. I can look into that. > If not, please be more specific here, and use the internal specifier for > this version of the IP block. If you can't find out what that is, use the > identifier for the oldest chip you know that has it. The binding documentation already notes "brcm,bcm7445-ahci", but we just don't bind against it here yet, since that hasn't been necessary. I suppose I can include the revision number, though; I forgot this block had one. (EDIT: in checking two random chips, I see that two cores both say v0.1 but have slightly different register layouts. Chip guys suck sometimes. So I'll stick to chip-based matching instead.) Brian [1] https://lkml.org/lkml/2015/3/18/940
On Thursday 23 April 2015 09:46:11 Brian Norris wrote: > Hi Arnd, > > On Thu, Apr 23, 2015 at 09:43:55AM +0200, Arnd Bergmann wrote: > > On Wednesday 22 April 2015 19:59:08 Brian Norris wrote: > > > Pretty straightforward driver, using the nice library-ization of the > > > generic ahci_platform driver. > > > > > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > > > > There is an alternative way to do this, by writing a separate phy driver > > for drivers/phy and using the generic ahci-platform driver. Have you > > considered that as well? I don't know which approach works better here, > > but in general we should try to have the generic driver handle as many > > chips as possible. > > Did you read v1 [1]? There was discussion all over the place, with each > person recommending their own favorite Right Way. It was suggested in > various ways that code be moved into drivers/ata/ or drivers/phy/ at > various points. In the end, I couldn't follow all suggestions and > instead came up with this: > > 1. SATA_TOP_CTRL deals with AHCI-related registers and some high-level > PHY bits (power on or off the PHY). > 2. There is another discrete bit of hardware that handles analog aspects > of the PHY > 3. The device tree should describe hardware, not how software wants to > use it > 4. Software should avoid sharing register ranges > > So, this suggested we should have two DT nodes; one for #1 and one for > #2. It seemed natural that because #1 modifies the AHCI core (e.g., the > endianness of it), that it belongs in a 'SATA' driver. So I use > libahci_platform instead of using the generic driver. The bits from #2 > go in drivers/phy/. > > So we have a PHY driver, but it doesn't cover everything. Did you want > me to write a second PHY driver?? I'm not sure how that'd work... I think this is all fine, I mainly wanted to make sure that it had been considered and that a decision was made after comparing the options. What I'd really like to see is that you put this summary into the patch description, because any reviewer will wonder about this. You can use a 'Link: https://lkml.org/lkml/2015/3/18/940' tag behind your signed-off-by to reference the earlier discussion, in addition to the summary. > > > diff --git a/drivers/ata/sata_brcmstb.c b/drivers/ata/sata_brcmstb.c > > > new file mode 100644 > > > index 000000000000..ab8d2261fa96 > > > --- /dev/null > > > +++ b/drivers/ata/sata_brcmstb.c > > > @@ -0,0 +1,285 @@ > > > +/* > > > + * Broadcom SATA3 AHCI Controller Driver > > > > Is this AHCI or not? Most AHCI drivers are called ahci_*.c, not sata_*.c > > It is AHCI. I can rename if necessary... > > If you're wondering about the comment there, SATA3 is left to indicate > the difference between earlier (non-AHCI) SATA cores that only supported > up to Gen2 SATA. Up to the ata maintainer of course, but I'd use ahci_brcmstb in the name just for consistency with the other drivers. > > > +#ifdef __BIG_ENDIAN > > > +#define DATA_ENDIAN 2 /* AHCI->DDR inbound accesses */ > > > +#define MMIO_ENDIAN 2 /* CPU->AHCI outbound accesses */ > > > +#else > > > +#define DATA_ENDIAN 0 > > > +#define MMIO_ENDIAN 0 > > > +#endif > > > > Is this for MIPS or ARM based chips or both? > > Both. (This particular iteration of the driver was only tested on > ARM, but this particular code has been the same on MIPS.) > > > ARM SoCs should never care > > about the endianess of the CPU, > > Why do you say that? What makes ARM different than MIPS here? If > somebody were using ARM BE, then they would (just like in MIPS) need to > configure our AHCI core to pretend like it's in LE mode, as that's what > libahci.c assumes. MIPS is special regarding endianess here, because their CPU cores use a power-on setting that defines a fixed endianess, while others tend to let software decide at runtime. Broadcom usually wires peripherals in a way to match the CPU endianess, but this is not necessarily what other MIPS chips do, and on most architectures that would be considered a mistake in the hardware design, because it breaks device drivers. If you can configure the endianess of the AHCI core through software, it would be best to always set it to little-endian unconditionally, so you can just use readl() or readl_relaxed() all the time. > > > +static void brcm_sata_phy_enable(struct brcm_ahci_priv *priv, int port) > > > +{ > > > + void __iomem *phyctrl = priv->top_ctrl + SATA_TOP_CTRL_PHY_CTRL + > > > + (port * SATA_TOP_CTRL_PHY_OFFS); > > > + void __iomem *p; > > > + u32 reg; > > > + > > > + /* clear PHY_DEFAULT_POWER_STATE */ > > > + p = phyctrl + SATA_TOP_CTRL_PHY_CTRL_1; > > > + reg = __raw_readl(p); > > > + reg &= ~SATA_TOP_CTRL_1_PHY_DEFAULT_POWER_STATE; > > > + __raw_writel(reg, p); > > > > Similarly, the use of __raw_readl() is broken on ARM here and won't > > work on big-endian kernels. Better use a driver specific helper > > function that does the right thing based on the architecture and > > endianess. You can use the same conditional as above and do > > > > #if defined(__BIG_ENDIAN) && defined(CONFIG_MIPS) > > > > static inline brcm_sata_phy_read(struct brcm_ahci_phy *priv, int port int reg) > > { > > void __iomem *phyctrl = priv->regs; > > > > if (IS_ENABLED(CONFIG_MIPS) && IS_ENABLED(__BIG_ENDIAN)) > > return __raw_readl(regs->reg); > > > > return readl_relaxed(regs->reg); > > } > > Huh? I'm fine with a driver-specific helper to abstract this out, but > why the MIPS fiddling? I'm fairly confident that in *all* > configurations, this piece of the IP is wired up to work in native > endianness, so it should never be doing an endian swap. The > readl_relaxed() you're adding looks like it would just break the > (theoretical) ARM BE case. > > I understand that you don't like bare __raw_{read,write}l(), but some IP > is just wired this way. Kevin Cernekee recently introduced the "native-endian" DT property that you can use to do runtime detection if that is necessary (i.e. you can't tell the endianess from the CPU architecture, and you can't set it either). Using __raw_readl() in driver code just means that the driver is not endian-aware at all, and we don't do that any more: new drivers that can be used on ARM must be written in a way that works on both little-endian and big-endian, and it's a good idea to use endian-safe code on other architectures too. It's ok to special-case MIPS here when you know that MIPS is weird. If you don't want that, use run-time detection instead. > > > +static const struct of_device_id ahci_of_match[] = { > > > + {.compatible = "brcm,sata3-ahci"}, > > > + {}, > > > > This sounds awefully generic. Can you guarantee that no part of Broadcom > > has produced another ahci-like SATA3 controller in the past, or will > > have one in the future? > > I know this controller is used by several chips across Broadcom, though > I can't guarantee there are not others at the moment. I can look into > that. Ok. > > If not, please be more specific here, and use the internal specifier for > > this version of the IP block. If you can't find out what that is, use the > > identifier for the oldest chip you know that has it. > > The binding documentation already notes "brcm,bcm7445-ahci", but we just > don't bind against it here yet, since that hasn't been necessary. I > suppose I can include the revision number, though; I forgot this block > had one. (EDIT: in checking two random chips, I see that two cores both > say v0.1 but have slightly different register layouts. Chip guys suck > sometimes. So I'll stick to chip-based matching instead.) Ok, I missed the part in the binding. In principle it's ok like you do then, but using "brcm,bcm7445-ahci" (or whichever one) as the "most generic" string is probably better, because of the PHY registers here that might not be present (or might be incompatible) in a future Broadcom chip. Arnd
On Fri, Apr 24, 2015 at 09:46:01AM +0200, Arnd Bergmann wrote: > On Thursday 23 April 2015 09:46:11 Brian Norris wrote: > > On Thu, Apr 23, 2015 at 09:43:55AM +0200, Arnd Bergmann wrote: > > > On Wednesday 22 April 2015 19:59:08 Brian Norris wrote: [snip] > > So we have a PHY driver, but it doesn't cover everything. Did you want > > me to write a second PHY driver?? I'm not sure how that'd work... > > I think this is all fine, I mainly wanted to make sure that it had been > considered and that a decision was made after comparing the options. > What I'd really like to see is that you put this summary into the > patch description, because any reviewer will wonder about this. > You can use a 'Link: https://lkml.org/lkml/2015/3/18/940' tag behind > your signed-off-by to reference the earlier discussion, in addition to > the summary. I summarized most of the conclusions in the cover letter, but I suppose a plain link to the original thread could do the job too. > > > > diff --git a/drivers/ata/sata_brcmstb.c b/drivers/ata/sata_brcmstb.c > > > > new file mode 100644 > > > > index 000000000000..ab8d2261fa96 > > > > --- /dev/null > > > > +++ b/drivers/ata/sata_brcmstb.c > > > > @@ -0,0 +1,285 @@ > > > > +/* > > > > + * Broadcom SATA3 AHCI Controller Driver > > > > > > Is this AHCI or not? Most AHCI drivers are called ahci_*.c, not sata_*.c > > > > It is AHCI. I can rename if necessary... > > > > If you're wondering about the comment there, SATA3 is left to indicate > > the difference between earlier (non-AHCI) SATA cores that only supported > > up to Gen2 SATA. > > Up to the ata maintainer of course, but I'd use ahci_brcmstb in the name > just for consistency with the other drivers. OK, maybe I'll do that. > > > > +#ifdef __BIG_ENDIAN > > > > +#define DATA_ENDIAN 2 /* AHCI->DDR inbound accesses */ > > > > +#define MMIO_ENDIAN 2 /* CPU->AHCI outbound accesses */ > > > > +#else > > > > +#define DATA_ENDIAN 0 > > > > +#define MMIO_ENDIAN 0 > > > > +#endif > > > > > > Is this for MIPS or ARM based chips or both? > > > > Both. (This particular iteration of the driver was only tested on > > ARM, but this particular code has been the same on MIPS.) > > > > > ARM SoCs should never care > > > about the endianess of the CPU, > > > > Why do you say that? What makes ARM different than MIPS here? If > > somebody were using ARM BE, then they would (just like in MIPS) need to > > configure our AHCI core to pretend like it's in LE mode, as that's what > > libahci.c assumes. > > MIPS is special regarding endianess here, because their CPU cores > use a power-on setting that defines a fixed endianess, while others > tend to let software decide at runtime. I wasn't really considering this. I haven't seen a Broadcom chip where runtime switching was used. But I haven't played with too many big endian systems, and all have been MIPS. > Broadcom usually wires peripherals in a way to match the CPU endianess, > but this is not necessarily what other MIPS chips do, and on most > architectures that would be considered a mistake in the hardware > design, because it breaks device drivers. I understand this. But this has no relevance to Broadcom IP. It's probably a good argument for putting good comments, I suppose. > If you can configure the endianess of the AHCI core through software, > it would be best to always set it to little-endian unconditionally, > so you can just use readl() or readl_relaxed() all the time. We're already doing this in the driver as written, at least for the three configurations that have been tested (MIPS LE, MIPS BE, ARM LE). IIUC, you're focusing on ARM BE? This is not in any support plan for these chips. But to straighten out what you're saying (correct me if I'm wrong), it seems like you're saying that on a (theoretical) BCM7xxx ARM in BE mode, the chip will come up in LE as normal, all busing will be configured for LE mode, and the BE kernel would only later change CPU endianness. This would mean that AHCI should be left as it was -- in LE mode -- whereas this driver submission would actually configure AHCI to do its own internal swapping, effectively making it BE mode again. And that would be wrong. Now I think this has a few problems: 1. ARM BE is not (and won't be) supported on these chips. There has been no plan and no testing, and I'm sure we'd run into more problems than those you're suggesting here. 2. To get ARM BE working properly, I'm not confident we'd do (only) runtime 'setend' endianness switching. We're more likely to get a bus-level endianness switch and be back with a native-endian driver again. But then, I'm only speculating (as you are). > > > > +static void brcm_sata_phy_enable(struct brcm_ahci_priv *priv, int port) > > > > +{ > > > > + void __iomem *phyctrl = priv->top_ctrl + SATA_TOP_CTRL_PHY_CTRL + > > > > + (port * SATA_TOP_CTRL_PHY_OFFS); > > > > + void __iomem *p; > > > > + u32 reg; > > > > + > > > > + /* clear PHY_DEFAULT_POWER_STATE */ > > > > + p = phyctrl + SATA_TOP_CTRL_PHY_CTRL_1; > > > > + reg = __raw_readl(p); > > > > + reg &= ~SATA_TOP_CTRL_1_PHY_DEFAULT_POWER_STATE; > > > > + __raw_writel(reg, p); > > > > > > Similarly, the use of __raw_readl() is broken on ARM here and won't > > > work on big-endian kernels. Better use a driver specific helper > > > function that does the right thing based on the architecture and > > > endianess. You can use the same conditional as above and do > > > > > > #if defined(__BIG_ENDIAN) && defined(CONFIG_MIPS) > > > > > > static inline brcm_sata_phy_read(struct brcm_ahci_phy *priv, int port int reg) > > > { > > > void __iomem *phyctrl = priv->regs; > > > > > > if (IS_ENABLED(CONFIG_MIPS) && IS_ENABLED(__BIG_ENDIAN)) > > > return __raw_readl(regs->reg); > > > > > > return readl_relaxed(regs->reg); > > > } > > > > Huh? I'm fine with a driver-specific helper to abstract this out, but > > why the MIPS fiddling? I'm fairly confident that in *all* > > configurations, this piece of the IP is wired up to work in native > > endianness, so it should never be doing an endian swap. The > > readl_relaxed() you're adding looks like it would just break the > > (theoretical) ARM BE case. > > > > I understand that you don't like bare __raw_{read,write}l(), but some IP > > is just wired this way. > > Kevin Cernekee recently introduced the "native-endian" DT property that > you can use to do runtime detection if that is necessary (i.e. you can't > tell the endianess from the CPU architecture, and you can't set it either). I've seen that series. That works fine for IP that's shared on other systems, and that's what Broadcom has to work with. But I'm not sure that's worth doing on Broadcom-only IP, which is *always* accessed in native endianness. > Using __raw_readl() in driver code just means that the driver is not > endian-aware at all, and we don't do that any more: new drivers that > can be used on ARM must be written in a way that works on both > little-endian and big-endian, and it's a good idea to use endian-safe > code on other architectures too. > > It's ok to special-case MIPS here when you know that MIPS is weird. If > you don't want that, use run-time detection instead. I'm not sure I come to the same conclusions as you. ARM BE is never tested on these chips, so if you're really stuck on "handling" it (without testing), I'm almost more inclined to put in compile dependencies instead. e.g.: #if defined(CONFIG_ARM) && defined(CONFIG_CPU_BIG_ENDIAN) #error ARM BE not supported #endif Or maybe Kconfig deps instead. Or maybe a 'native-endian' DT property, and we just fail to probe for !native-endian. Now, this particular case is pretty trivial, and I'm sure I could just do what you say (since really, I don't care about ARM BE anyway), but I guarantee these same questions will come up over and over again on newly-upstreamed Broadcom drivers, where many of them are intentionally written for native endianness (because that's how the hardware works for all supported use cases) and have never been tested (and likely never will) on big endian ARM. To add extra levels of indirection (either conditional, or function pointers) to something as low-level as a register access, and for reasons that look almost 100% theoretical, seems excessive. If, however, you really have strong arguments for doing the extra work (and not testing it, and almost definitely never using it), then I can try and make sure we don't run across these problems in the future. I won't be too happy about it, but at least I won't have to waste my time on these discussions. Brian
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig index 5f601553b9b0..33d4b3031705 100644 --- a/drivers/ata/Kconfig +++ b/drivers/ata/Kconfig @@ -98,6 +98,15 @@ config SATA_AHCI_PLATFORM If unsure, say N. +config SATA_BRCMSTB + tristate "Broadcom STB AHCI SATA support" + depends on ARCH_BRCMSTB + help + This option enables support for the AHCI SATA3 controller found on + STB SoC's. + + If unsure, say N. + config AHCI_DA850 tristate "DaVinci DA850 AHCI SATA support" depends on ARCH_DAVINCI_DA850 diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile index ae41107afc1f..5d1e6a96bc93 100644 --- a/drivers/ata/Makefile +++ b/drivers/ata/Makefile @@ -5,6 +5,7 @@ obj-$(CONFIG_ATA) += libata.o obj-$(CONFIG_SATA_AHCI) += ahci.o libahci.o obj-$(CONFIG_SATA_ACARD_AHCI) += acard-ahci.o libahci.o obj-$(CONFIG_SATA_AHCI_PLATFORM) += ahci_platform.o libahci.o libahci_platform.o +obj-$(CONFIG_SATA_BRCMSTB) += sata_brcmstb.o libahci.o libahci_platform.o obj-$(CONFIG_SATA_FSL) += sata_fsl.o obj-$(CONFIG_SATA_INIC162X) += sata_inic162x.o obj-$(CONFIG_SATA_SIL24) += sata_sil24.o diff --git a/drivers/ata/sata_brcmstb.c b/drivers/ata/sata_brcmstb.c new file mode 100644 index 000000000000..ab8d2261fa96 --- /dev/null +++ b/drivers/ata/sata_brcmstb.c @@ -0,0 +1,285 @@ +/* + * Broadcom SATA3 AHCI Controller Driver + * + * Copyright © 2009-2015 Broadcom Corporation + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2, or (at your option) + * any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/ahci_platform.h> +#include <linux/compiler.h> +#include <linux/device.h> +#include <linux/init.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/libata.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/string.h> + +#include "ahci.h" + +#define DRV_NAME "brcm-ahci" + +#define SATA_TOP_CTRL_VERSION 0x0 +#define SATA_TOP_CTRL_BUS_CTRL 0x4 +#define SATA_TOP_CTRL_TP_CTRL 0x8 +#define SATA_TOP_CTRL_PHY_CTRL 0xc + #define SATA_TOP_CTRL_PHY_CTRL_1 0x0 + #define SATA_TOP_CTRL_1_PHY_DEFAULT_POWER_STATE BIT(14) + #define SATA_TOP_CTRL_PHY_CTRL_2 0x4 + #define SATA_TOP_CTRL_2_SW_RST_MDIOREG BIT(0) + #define SATA_TOP_CTRL_2_SW_RST_OOB BIT(1) + #define SATA_TOP_CTRL_2_SW_RST_RX BIT(2) + #define SATA_TOP_CTRL_2_SW_RST_TX BIT(3) + #define SATA_TOP_CTRL_2_PHY_GLOBAL_RESET BIT(14) + #define SATA_TOP_CTRL_PHY_OFFS 0x8 + #define SATA_TOP_MAX_PHYS 2 +#define SATA_TOP_CTRL_SATA_TP_OUT 0x1c +#define SATA_TOP_CTRL_CLIENT_INIT_CTRL 0x20 + +#ifdef __BIG_ENDIAN +#define DATA_ENDIAN 2 /* AHCI->DDR inbound accesses */ +#define MMIO_ENDIAN 2 /* CPU->AHCI outbound accesses */ +#else +#define DATA_ENDIAN 0 +#define MMIO_ENDIAN 0 +#endif + +struct brcm_ahci_priv { + struct device *dev; + void __iomem *top_ctrl; + u32 port_mask; +}; + +static const struct ata_port_info ahci_brcm_port_info = { + .flags = AHCI_FLAG_COMMON, + .pio_mask = ATA_PIO4, + .udma_mask = ATA_UDMA6, + .port_ops = &ahci_platform_ops, +}; + +static void brcm_sata_phy_enable(struct brcm_ahci_priv *priv, int port) +{ + void __iomem *phyctrl = priv->top_ctrl + SATA_TOP_CTRL_PHY_CTRL + + (port * SATA_TOP_CTRL_PHY_OFFS); + void __iomem *p; + u32 reg; + + /* clear PHY_DEFAULT_POWER_STATE */ + p = phyctrl + SATA_TOP_CTRL_PHY_CTRL_1; + reg = __raw_readl(p); + reg &= ~SATA_TOP_CTRL_1_PHY_DEFAULT_POWER_STATE; + __raw_writel(reg, p); + + /* reset the PHY digital logic */ + p = phyctrl + SATA_TOP_CTRL_PHY_CTRL_2; + reg = __raw_readl(p); + reg &= ~(SATA_TOP_CTRL_2_SW_RST_MDIOREG | SATA_TOP_CTRL_2_SW_RST_OOB | + SATA_TOP_CTRL_2_SW_RST_RX); + reg |= SATA_TOP_CTRL_2_SW_RST_TX; + __raw_writel(reg, p); + reg = __raw_readl(p); + reg |= SATA_TOP_CTRL_2_PHY_GLOBAL_RESET; + __raw_writel(reg, p); + reg = __raw_readl(p); + reg &= ~SATA_TOP_CTRL_2_PHY_GLOBAL_RESET; + __raw_writel(reg, p); + (void)__raw_readl(p); +} + +static void brcm_sata_phy_disable(struct brcm_ahci_priv *priv, int port) +{ + void __iomem *phyctrl = priv->top_ctrl + SATA_TOP_CTRL_PHY_CTRL + + (port * SATA_TOP_CTRL_PHY_OFFS); + void __iomem *p; + u32 reg; + + /* power-off the PHY digital logic */ + p = phyctrl + SATA_TOP_CTRL_PHY_CTRL_2; + reg = __raw_readl(p); + reg |= (SATA_TOP_CTRL_2_SW_RST_MDIOREG | SATA_TOP_CTRL_2_SW_RST_OOB | + SATA_TOP_CTRL_2_SW_RST_RX | SATA_TOP_CTRL_2_SW_RST_TX | + SATA_TOP_CTRL_2_PHY_GLOBAL_RESET); + __raw_writel(reg, p); + + /* set PHY_DEFAULT_POWER_STATE */ + p = phyctrl + SATA_TOP_CTRL_PHY_CTRL_1; + reg = __raw_readl(p); + reg |= SATA_TOP_CTRL_1_PHY_DEFAULT_POWER_STATE; + __raw_writel(reg, p); +} + +static void brcm_sata_phys_enable(struct brcm_ahci_priv *priv) +{ + int i; + + for (i = 0; i < SATA_TOP_MAX_PHYS; i++) + if (priv->port_mask & BIT(i)) + brcm_sata_phy_enable(priv, i); +} + +static void brcm_sata_phys_disable(struct brcm_ahci_priv *priv) +{ + int i; + + for (i = 0; i < SATA_TOP_MAX_PHYS; i++) + if (priv->port_mask & BIT(i)) + brcm_sata_phy_disable(priv, i); +} + +static u32 brcm_ahci_get_portmask(struct platform_device *pdev, + struct brcm_ahci_priv *priv) +{ + void __iomem *ahci; + struct resource *res; + u32 impl; + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ahci"); + ahci = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(ahci)) + return 0; + + impl = readl(ahci + HOST_PORTS_IMPL); + + if (fls(impl) > SATA_TOP_MAX_PHYS) + dev_warn(priv->dev, "warning: more ports than PHYs (%#x)\n", + impl); + else if (!impl) + dev_info(priv->dev, "no ports found\n"); + + devm_iounmap(&pdev->dev, ahci); + devm_release_mem_region(&pdev->dev, res->start, resource_size(res)); + + return impl; +} + +static void brcm_sata_init(struct brcm_ahci_priv *priv) +{ + /* Configure endianness */ + __raw_writel((DATA_ENDIAN << 4) | (DATA_ENDIAN << 2) | + (MMIO_ENDIAN << 0), + priv->top_ctrl + SATA_TOP_CTRL_BUS_CTRL); +} + +static int brcm_ahci_suspend(struct device *dev) +{ + struct ata_host *host = dev_get_drvdata(dev); + struct ahci_host_priv *hpriv = host->private_data; + struct brcm_ahci_priv *priv = hpriv->plat_data; + int ret; + + ret = ahci_platform_suspend(dev); + brcm_sata_phys_disable(priv); + return ret; +} + +static int brcm_ahci_resume(struct device *dev) +{ + struct ata_host *host = dev_get_drvdata(dev); + struct ahci_host_priv *hpriv = host->private_data; + struct brcm_ahci_priv *priv = hpriv->plat_data; + + brcm_sata_init(priv); + brcm_sata_phys_enable(priv); + return ahci_platform_resume(dev); +} + +static struct scsi_host_template ahci_platform_sht = { + AHCI_SHT(DRV_NAME), +}; + +static int brcm_ahci_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct brcm_ahci_priv *priv; + struct ahci_host_priv *hpriv; + struct resource *res; + int ret; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + priv->dev = dev; + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "top-ctrl"); + priv->top_ctrl = devm_ioremap_resource(dev, res); + if (IS_ERR(priv->top_ctrl)) + return PTR_ERR(priv->top_ctrl); + + brcm_sata_init(priv); + + priv->port_mask = brcm_ahci_get_portmask(pdev, priv); + if (!priv->port_mask) + return -ENODEV; + + brcm_sata_phys_enable(priv); + + hpriv = ahci_platform_get_resources(pdev); + if (IS_ERR(hpriv)) + return PTR_ERR(hpriv); + hpriv->plat_data = priv; + + ret = ahci_platform_enable_resources(hpriv); + if (ret) + return ret; + + ret = ahci_platform_init_host(pdev, hpriv, &ahci_brcm_port_info, + &ahci_platform_sht); + if (ret) + return ret; + + dev_info(dev, "Broadcom AHCI SATA3 registered\n"); + + return 0; +} + +static int brcm_ahci_remove(struct platform_device *pdev) +{ + struct ata_host *host = dev_get_drvdata(&pdev->dev); + struct ahci_host_priv *hpriv = host->private_data; + struct brcm_ahci_priv *priv = hpriv->plat_data; + int ret; + + ret = ata_platform_remove_one(pdev); + if (ret) + return ret; + + brcm_sata_phys_disable(priv); + + return 0; +} + +static const struct of_device_id ahci_of_match[] = { + {.compatible = "brcm,sata3-ahci"}, + {}, +}; +MODULE_DEVICE_TABLE(of, ahci_of_match); + +static SIMPLE_DEV_PM_OPS(ahci_brcm_pm_ops, brcm_ahci_suspend, brcm_ahci_resume); + +static struct platform_driver brcm_ahci_driver = { + .probe = brcm_ahci_probe, + .remove = brcm_ahci_remove, + .driver = { + .name = DRV_NAME, + .of_match_table = ahci_of_match, + .pm = &ahci_brcm_pm_ops, + }, +}; +module_platform_driver(brcm_ahci_driver); + +MODULE_DESCRIPTION("Broadcom SATA3 AHCI Controller Driver"); +MODULE_AUTHOR("Brian Norris"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:sata-brcmstb");
Pretty straightforward driver, using the nice library-ization of the generic ahci_platform driver. Signed-off-by: Brian Norris <computersforpeace@gmail.com> --- v2: - move port enabling into this driver, since the affected registers are in the SATA_TOP_CTRL block. This means we need to check for the implemented port(s) here. - fix up layering issues with using drvdata (libata expects to use drvdata), similar to this issue: http://marc.info/?l=linux-ide&m=142851961920009&w=2 - trivial fixups drivers/ata/Kconfig | 9 ++ drivers/ata/Makefile | 1 + drivers/ata/sata_brcmstb.c | 285 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 295 insertions(+) create mode 100644 drivers/ata/sata_brcmstb.c