AHCI and SATA PHY support for Broadcom STB SoCs

Pretty straightforward driver, using the nice library-ization of the generic ahci_platform driver.

Signed-off-by Brian Norris <[email protected]>

Quoted 4 lines · 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 <[email protected]>

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.

drivers/ata/sata_brcmstb.c line 2

Quoted 8 lines

drivers/ata/sata_brcmstb.c new file +2 -0

new file mode 100644
index 000000000000..ab8d2261fa96
@@ -0,0 +1,285 @@
+/*
+ * Broadcom SATA3 AHCI Controller Driver

Is this AHCI or not? Most AHCI drivers are called ahci_*.c, not sata_*.c

drivers/ata/sata_brcmstb.c line 57

Quoted 7 lines

drivers/ata/sata_brcmstb.c new file +7 -0

+#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

drivers/ata/sata_brcmstb.c line 83

Quoted 12 lines

drivers/ata/sata_brcmstb.c new file +12 -0

+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);
}

drivers/ata/sata_brcmstb.c line 265

Quoted 3 lines

drivers/ata/sata_brcmstb.c new file +3 -0

+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.

Hi Arnd,

Quoted 11 lines · Arnd Bergmann

Quoted 4 lines · 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 <[email protected]>

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...

Quoted 10 lines

drivers/ata/sata_brcmstb.c line 2

Quoted 8 lines

drivers/ata/sata_brcmstb.c new file +2 -0

new file mode 100644
index 000000000000..ab8d2261fa96
@@ -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.

Quoted 9 lines

drivers/ata/sata_brcmstb.c line 57

Quoted 7 lines

drivers/ata/sata_brcmstb.c new file +7 -0

+#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.)

Quoted 2 lines

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.

Quoted 21 lines

drivers/ata/sata_brcmstb.c line 83

Quoted 12 lines

drivers/ata/sata_brcmstb.c new file +12 -0

+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)

Quoted 8 lines

{
	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.

Quoted 7 lines

drivers/ata/sata_brcmstb.c line 265

Quoted 3 lines

drivers/ata/sata_brcmstb.c new file +3 -0

+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.

Quoted 3 lines

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.)

Quoted 2 lines · On Thursday 23 April 2015 09:46:11 Brian Norris wrote:

Quoted 1 line · Arnd Bergmann

On Wednesday 22 April 2015 19:59:08 Brian Norris wrote:

Quoted 2 lines

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.

Quoted 11 lines

Quoted 10 lines

drivers/ata/sata_brcmstb.c line 2

Quoted 8 lines

drivers/ata/sata_brcmstb.c new file +2 -0

new file mode 100644
index 000000000000..ab8d2261fa96
@@ -0,0 +1,285 @@
+/*
+ * Broadcom SATA3 AHCI Controller Driver

Is this AHCI or not? Most AHCI drivers are called ahci_*.c, not sata_*.c

Quoted 5 lines

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.

Quoted 16 lines

Quoted 9 lines

drivers/ata/sata_brcmstb.c line 57

Quoted 7 lines

drivers/ata/sata_brcmstb.c new file +7 -0

+#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.)

Quoted 2 lines

ARM SoCs should never care
about the endianess of the CPU,

Quoted 4 lines

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.

Quoted 37 lines

Quoted 21 lines

drivers/ata/sata_brcmstb.c line 83

Quoted 12 lines

drivers/ata/sata_brcmstb.c new file +12 -0

+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)

Quoted 8 lines

{
	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.

Quoted 2 lines

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.

Quoted 3 lines · Arnd Bergmann

Quoted 2 lines · On Thursday 23 April 2015 09:46:11 Brian Norris wrote:

Quoted 1 line · Arnd Bergmann

On Wednesday 22 April 2015 19:59:08 Brian Norris wrote:

[snip]

Quoted 10 lines

Quoted 2 lines

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.

Quoted 3 lines

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).

Quoted 10 lines

Quoted 9 lines

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.

Quoted 3 lines

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.

Quoted 8 lines

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.

Hi,

Here are my updates based on everyone's feedback. I'll try to include most of the changelog info in each patch, but a few summary points for v1 -> v2:

- reworked the PHY DT binding so that we don't need do any custom xlate in the PHY driver

- moved all handling of the 'SATA_TOP_CTRL' block into the SATA driver, instead of sharing it between SATA and PHY drivers. This means we have to do a little extra work in sata_brcmstb.c to decide which ports to power on, but at least this way, we're really describing the hardware, not just how the SW frameworks want to use the hardware.

Enjoy,
Brian

Brian Norris (5):
Documentation: devicetree: add Broadcom SATA binding
Documentation: devicetree: add Broadcom SATA PHY binding
ata: add Broadcom AHCI SATA3 driver for STB chips
phy: add Broadcom SATA3 PHY driver for Broadcom STB SoCs
ARM: dts: brcmstb: add nodes for SATA controller and PHY

  .../devicetree/bindings/ata/brcm,sata-brcmstb.txt  |  35 +++
  .../bindings/phy/brcm,brcmstb-sata-phy.txt         |  40 +++
  arch/arm/boot/dts/bcm7445.dtsi                     |  37 +++
  drivers/ata/Kconfig                                |   9 +
  drivers/ata/Makefile                               |   1 +
  drivers/ata/sata_brcmstb.c                         | 285 +++++++++++++++++++++
  drivers/phy/Kconfig                                |   9 +
  drivers/phy/Makefile                               |   1 +
  drivers/phy/phy-brcmstb-sata.c                     | 216 ++++++++++++++++
  9 files changed, 632 insertions(+)

create mode 100644 Documentation/devicetree/bindings/ata/brcm,sata-brcmstb.txt create mode 100644 Documentation/devicetree/bindings/phy/brcm,brcmstb-sata-phy.txt create mode 100644 drivers/ata/sata_brcmstb.c
create mode 100644 drivers/phy/phy-brcmstb-sata.c

Quoted 6 lines · Brian Norris

Here are my updates based on everyone's feedback. I'll try to include most of the changelog info in each patch, but a few summary points for v1 -> v2:

- reworked the PHY DT binding so that we don't need do any custom xlate in the PHY driver

Quoted 5 lines

- moved all handling of the 'SATA_TOP_CTRL' block into the SATA driver, instead of sharing it between SATA and PHY drivers. This means we have to do a little extra work in sata_brcmstb.c to decide which ports to power on, but at least this way, we're really describing the hardware, not just how the SW frameworks want to use the hardware.

I don't see any problems with the PHY patches. Let me know If I can take this via linux-phy tree.