[PATCH v2 0/5] AHCI and SATA PHY support for Broadcom STB SoCs

[PATCH v2 0/5] AHCI and SATA PHY support for Broadcom STB SoCs

[Brian Norris]

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

-- 
1.9.1

[PATCH v2 1/5] Documentation: devicetree: add Broadcom SATA binding

[Brian Norris]

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
v2: no change

 .../devicetree/bindings/ata/brcm,sata-brcmstb.txt  | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ata/brcm,sata-brcmstb.txt

diff --git a/Documentation/devicetree/bindings/ata/brcm,sata-brcmstb.txt b/Documentation/devicetree/bindings/ata/brcm,sata-brcmstb.txt
new file mode 100644
index 000000000000..afeede13e195
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/brcm,sata-brcmstb.txt
@@ -0,0 +1,35 @@
+* Broadcom SATA3 AHCI Controller for STB
+
+SATA nodes are defined to describe on-chip Serial ATA controllers.
+Each SATA controller should have its own node.
+
+Required properties:
+- compatible         : compatible list, may contain "brcm,bcm7445-ahci" and/or
+                       "brcm,sata3-ahci"
+- reg                : register mappings for AHCI and SATA_TOP_CTRL
+- reg-names          : "ahci" and "top-ctrl"
+- interrupts         : interrupt mapping for SATA IRQ
+
+Also see ahci-platform.txt.
+
+Example:
+
+	sata@f045a000 {
+		compatible = "brcm,bcm7445-ahci", "brcm,sata3-ahci";
+		reg = <0xf045a000 0xa9c>, <0xf0458040 0x24>;
+		reg-names = "ahci", "top-ctrl";
+		interrupts = <0 30 0>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		sata0: sata-port@0 {
+			reg = <0>;
+			phys = <&sata_phy 0>;
+		};
+
+		sata1: sata-port@1 {
+			reg = <1>;
+			phys = <&sata_phy 1>;
+		};
+	};
+
-- 
1.9.1

[PATCH v2 2/5] Documentation: devicetree: add Broadcom SATA PHY binding

[Brian Norris]

For 28nm STB chips, based on BCM7445.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
v2:
  - make each subnode into a provider, so we can use direct phandle references
    to them
  - drop the 'port-ctrl' register range, since this was shared with the SATA
    node

 .../bindings/phy/brcm,brcmstb-sata-phy.txt         | 40 ++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/brcm,brcmstb-sata-phy.txt

diff --git a/Documentation/devicetree/bindings/phy/brcm,brcmstb-sata-phy.txt b/Documentation/devicetree/bindings/phy/brcm,brcmstb-sata-phy.txt
new file mode 100644
index 000000000000..7f81ef90146a
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/brcm,brcmstb-sata-phy.txt
@@ -0,0 +1,40 @@
+* Broadcom SATA3 PHY for STB
+
+Required properties:
+- compatible: should be one or more of
+     "brcm,bcm7445-sata-phy"
+     "brcm,phy-sata3"
+- address-cells: should be 1
+- size-cells: should be 0
+- reg: register range for the PHY PCB interface
+- reg-names: should be "phy"
+
+Sub-nodes:
+  Each port's PHY should be represented as a sub-node.
+
+Sub-nodes required properties:
+- reg: the PHY number
+- phy-cells: generic PHY binding; must be 0
+Optional:
+- brcm,enable-ssc: use spread spectrum clocking (SSC) on this port
+
+
+Example:
+
+	sata-phy@f0458100 {
+		compatible = "brcm,bcm7445-sata-phy", "brcm,phy-sata3";
+		reg = <0xf0458100 0x1e00>, <0xf045804c 0x10>;
+		reg-names = "phy";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		sata-phy@0 {
+			reg = <0>;
+			#phy-cells = <0>;
+		};
+
+		sata-phy@1 {
+			reg = <1>;
+			#phy-cells = <0>;
+		};
+	};
-- 
1.9.1

[PATCH v2 3/5] ata: add Broadcom AHCI SATA3 driver for STB chips

[Brian Norris]

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

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");
-- 
1.9.1

Re: [PATCH v2 3/5] ata: add Broadcom AHCI SATA3 driver for STB chips

[Arnd Bergmann]

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

Re: [PATCH v2 3/5] ata: add Broadcom AHCI SATA3 driver for STB chips

[Brian Norris]

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

Re: [PATCH v2 3/5] ata: add Broadcom AHCI SATA3 driver for STB chips

[Arnd Bergmann]

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

Re: [PATCH v2 3/5] ata: add Broadcom AHCI SATA3 driver for STB chips

[Brian Norris]

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

Re: [PATCH v2 3/5] ata: add Broadcom AHCI SATA3 driver for STB chips

[Arnd Bergmann]

On Monday 27 April 2015 14:55:41 Brian Norris wrote:
> 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.

Ok. I missed the description in the cover letter, that was my mistake.
Having more information in the part before the "---" line in the
patch description would be good here. 

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

I don't care if you want to provide official support for that configuration,
this is just about not writing code that is obviously broken in
configurations that are otherwise valid.

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

Correct, this is how all ARMv7 SoCs work.

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

Yes. I wasn't sure how the register for the swapping is defined though,
whether you set it to 'LE' vs 'BE', or to 'swap' vs 'noswap'.

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

There are usually three problems that happen when someone first tries
out a BE kernel on a new ARM platform:

a) secondary CPUs need to be switched into big-endian mode when they initially
   come out of reset:

diff --git a/arch/arm/mach-bcm/headsmp-brcmstb.S b/arch/arm/mach-bcm/headsmp-brcmstb.S
index 199c1ea58248..d7fe25502f54 100644
--- a/arch/arm/mach-bcm/headsmp-brcmstb.S
+++ b/arch/arm/mach-bcm/headsmp-brcmstb.S
@@ -26,6 +26,7 @@ ENTRY(brcmstb_secondary_startup)
          * Ensure CPU is in a sane state by disabling all IRQs and switching
          * into SVC mode.
          */
+ARM_BE8(setend be)
         setmode	PSR_I_BIT | PSR_F_BIT | SVC_MODE, r0
 
         bl      v7_invalidate_l1

b) drivers that use __raw_readl() instead readl_relaxed()

We already have an army of people that hunt down these drivers to convert them
to use readl_relaxed(). In your case, doing that would have the side-effect
of breaking the MIPS parts, so please do everyone and yourself a favor and
write the driver to work correctly from the start, so we don't need to write
that patch.

c) DMA descriptors that have gone wrong: As the endianess of a DMA master
device is fixed, you have to make sure that any data that is read by the
device is stored in device endianess, usually by defining the DMA
descriptors like 

struct ahci_cmd_hdr {
        __le32                  opts;
        __le32                  status;
        __le32                  tbl_addr;
        __le32                  tbl_addr_hi;
        __le32                  reserved[4];
};

and then using cpu_to_le32 (or cpu_to_be32 for big-endian devices) to store
the descriptor data in memory. Most drivers get this right, and we have
tools available to help you there. Doing a byte swap on the bus does not
work for DMA in general, because that would swap both the descriptors
and the data you want to transfer, which has to remain in address order
as a byte stream.

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

We have a ton of SoCs that just work in BE mode, and none of them were "designed"
to support it. Having a register to switch the bus endianess is considered
a mistake on most CPU architectures, and if there is one, we definitely
won't turn it on.

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

Just get the concept of "native endianess" out of your head when you have
something that is not MIPS, it doesn't exist. ;-) 
In Linux, each device has its own endianess, and the drivers are written
to support it that way.

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

Those would all still be bugs. Don't write code that is known to be
broken when the simple and documented way to write a driver makes it
work.

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

Making it architecture-dependent is fine, as I said. Just special-case
the MIPS-BE configuration here at compile-time, and it will be fine.

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

What I really want you to understand is the importance of writing correct
code that works even in cases you have not tested. We have put a lot of
thought into the MMIO register accessors over the years, trying to make
them foolproof, but you should not try to outsmart them if you don't know
the full story.

Note that a lot of the complexity of readl/writel stems from x86 having
very specific semantics: Hardware is assumed to always have little-endian
registers because that's what x86 has, and we do a lot of cache
and write buffer syncs because x86 is fully ordered on MMIO accesses.
(readl_relaxed avoids a lot of that overhead and you can use that
if you know what you are doing).

	Arnd

[PATCH v2 4/5] phy: add Broadcom SATA3 PHY driver for Broadcom STB SoCs

[Brian Norris]

Supports up to two ports which can each be powered on/off and configured
independently.

Signed-off-by: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
v2:
  - stop sharing SATA_TOP_CTRL registers with SATA driver
  - kill custom xlate function

 drivers/phy/Kconfig            |   9 ++
 drivers/phy/Makefile           |   1 +
 drivers/phy/phy-brcmstb-sata.c | 216 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 226 insertions(+)
 create mode 100644 drivers/phy/phy-brcmstb-sata.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 2962de205ba7..c8b22074bcf6 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -291,4 +291,13 @@ config PHY_QCOM_UFS
 	help
 	  Support for UFS PHY on QCOM chipsets.
 
+config PHY_BRCMSTB_SATA
+	tristate "Broadcom STB SATA PHY driver"
+	depends on ARCH_BRCMSTB
+	depends on OF
+	select GENERIC_PHY
+	help
+	  Enable this to support the SATA3 PHY on 28nm Broadcom STB SoCs.
+	  Likely useful only with CONFIG_SATA_BRCMSTB enabled.
+
 endmenu
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index f080e1bb2a74..28a10804b4f4 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -38,3 +38,4 @@ obj-$(CONFIG_PHY_STIH41X_USB)		+= phy-stih41x-usb.o
 obj-$(CONFIG_PHY_QCOM_UFS) 	+= phy-qcom-ufs.o
 obj-$(CONFIG_PHY_QCOM_UFS) 	+= phy-qcom-ufs-qmp-20nm.o
 obj-$(CONFIG_PHY_QCOM_UFS) 	+= phy-qcom-ufs-qmp-14nm.o
+obj-$(CONFIG_PHY_BRCMSTB_SATA)		+= phy-brcmstb-sata.o
diff --git a/drivers/phy/phy-brcmstb-sata.c b/drivers/phy/phy-brcmstb-sata.c
new file mode 100644
index 000000000000..8387c8cbea8c
--- /dev/null
+++ b/drivers/phy/phy-brcmstb-sata.c
@@ -0,0 +1,216 @@
+/*
+ * Broadcom SATA3 AHCI Controller PHY 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/device.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+
+#define SATA_MDIO_BANK_OFFSET				0x23c
+#define SATA_MDIO_REG_OFFSET(ofs)			((ofs) * 4)
+#define SATA_MDIO_REG_SPACE_SIZE			0x1000
+#define SATA_MDIO_REG_LENGTH				0x1f00
+
+#define MAX_PORTS					2
+
+/* Register offset between PHYs in PCB space */
+#define SATA_MDIO_REG_SPACE_SIZE			0x1000
+
+struct brcm_sata_port {
+	int portnum;
+	struct phy *phy;
+	struct brcm_sata_phy *phy_priv;
+	bool ssc_en;
+};
+
+struct brcm_sata_phy {
+	struct device *dev;
+	void __iomem *phy_base;
+
+	struct brcm_sata_port phys[MAX_PORTS];
+};
+
+enum sata_mdio_phy_regs_28nm {
+	PLL_REG_BANK_0				= 0x50,
+	PLL_REG_BANK_0_PLLCONTROL_0		= 0x81,
+
+	TXPMD_REG_BANK				= 0x1a0,
+	TXPMD_CONTROL1				= 0x81,
+	TXPMD_CONTROL1_TX_SSC_EN_FRC		= BIT(0),
+	TXPMD_CONTROL1_TX_SSC_EN_FRC_VAL	= BIT(1),
+	TXPMD_TX_FREQ_CTRL_CONTROL1		= 0x82,
+	TXPMD_TX_FREQ_CTRL_CONTROL2		= 0x83,
+	TXPMD_TX_FREQ_CTRL_CONTROL2_FMIN_MASK	= 0x3ff,
+	TXPMD_TX_FREQ_CTRL_CONTROL3		= 0x84,
+	TXPMD_TX_FREQ_CTRL_CONTROL3_FMAX_MASK	= 0x3ff,
+};
+
+static inline void __iomem *brcm_sata_phy_base(struct brcm_sata_port *port)
+{
+	struct brcm_sata_phy *priv = port->phy_priv;
+
+	return priv->phy_base + (port->portnum * SATA_MDIO_REG_SPACE_SIZE);
+}
+
+static void brcm_sata_mdio_wr(void __iomem *addr, u32 bank, u32 ofs,
+			      u32 msk, u32 value)
+{
+	u32 tmp;
+
+	writel(bank, addr + SATA_MDIO_BANK_OFFSET);
+	tmp = readl(addr + SATA_MDIO_REG_OFFSET(ofs));
+	tmp = (tmp & msk) | value;
+	writel(tmp, addr + SATA_MDIO_REG_OFFSET(ofs));
+}
+
+/* These defaults were characterized by H/W group */
+#define FMIN_VAL_DEFAULT	0x3df
+#define FMAX_VAL_DEFAULT	0x3df
+#define FMAX_VAL_SSC		0x83
+
+static void cfg_ssc_28nm(struct brcm_sata_port *port)
+{
+	void __iomem *base = brcm_sata_phy_base(port);
+	struct brcm_sata_phy *priv = port->phy_priv;
+	u32 tmp;
+
+	/* override the TX spread spectrum setting */
+	tmp = TXPMD_CONTROL1_TX_SSC_EN_FRC_VAL | TXPMD_CONTROL1_TX_SSC_EN_FRC;
+	brcm_sata_mdio_wr(base, TXPMD_REG_BANK, TXPMD_CONTROL1, ~tmp, tmp);
+
+	/* set fixed min freq */
+	brcm_sata_mdio_wr(base, TXPMD_REG_BANK, TXPMD_TX_FREQ_CTRL_CONTROL2,
+			  ~TXPMD_TX_FREQ_CTRL_CONTROL2_FMIN_MASK,
+			  FMIN_VAL_DEFAULT);
+
+	/* set fixed max freq depending on SSC config */
+	if (port->ssc_en) {
+		dev_info(priv->dev, "enabling SSC on port %d\n", port->portnum);
+		tmp = FMAX_VAL_SSC;
+	} else {
+		tmp = FMAX_VAL_DEFAULT;
+	}
+
+	brcm_sata_mdio_wr(base, TXPMD_REG_BANK, TXPMD_TX_FREQ_CTRL_CONTROL3,
+			  ~TXPMD_TX_FREQ_CTRL_CONTROL3_FMAX_MASK, tmp);
+}
+
+static int brcm_sata_phy_init(struct phy *phy)
+{
+	struct brcm_sata_port *port = phy_get_drvdata(phy);
+
+	cfg_ssc_28nm(port);
+
+	return 0;
+}
+
+static struct phy_ops phy_ops_28nm = {
+	.init		= brcm_sata_phy_init,
+	.owner		= THIS_MODULE,
+};
+
+static const struct of_device_id brcm_sata_phy_of_match[] = {
+	{ .compatible	= "brcm,bcm7445-sata-phy" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, brcm_sata_phy_of_match);
+
+static int brcm_sata_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *dn = dev->of_node, *child;
+	struct brcm_sata_phy *priv;
+	struct resource *res;
+	struct phy_provider *provider;
+	int count = 0;
+
+	if (of_get_child_count(dn) == 0)
+		return -ENODEV;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	dev_set_drvdata(dev, priv);
+	priv->dev = dev;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "phy");
+	priv->phy_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->phy_base))
+		return PTR_ERR(priv->phy_base);
+
+	for_each_available_child_of_node(dn, child) {
+		unsigned int id;
+		struct brcm_sata_port *port;
+
+		if (of_property_read_u32(child, "reg", &id)) {
+			dev_err(dev, "missing reg property in node %s\n",
+					child->name);
+			return -EINVAL;
+		}
+
+		if (id >= MAX_PORTS) {
+			dev_err(dev, "invalid reg: %u\n", id);
+			return -EINVAL;
+		}
+		if (priv->phys[id].phy) {
+			dev_err(dev, "already registered port %u\n", id);
+			return -EINVAL;
+		}
+
+		port = &priv->phys[id];
+		port->portnum = id;
+		port->phy_priv = priv;
+		port->phy = devm_phy_create(dev, child, &phy_ops_28nm);
+		port->ssc_en = of_property_read_bool(child, "brcm,enable-ssc");
+		if (IS_ERR(port->phy)) {
+			dev_err(dev, "failed to create PHY\n");
+			return PTR_ERR(port->phy);
+		}
+
+		phy_set_drvdata(port->phy, port);
+		count++;
+	}
+
+	provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+	if (IS_ERR(provider)) {
+		dev_err(dev, "could not register PHY provider\n");
+		return PTR_ERR(provider);
+	}
+
+	dev_info(dev, "registered %d port(s)\n", count);
+
+	return 0;
+}
+
+static struct platform_driver brcm_sata_phy_driver = {
+	.probe	= brcm_sata_phy_probe,
+	.driver	= {
+		.of_match_table	= brcm_sata_phy_of_match,
+		.name		= "brcmstb-sata-phy",
+	}
+};
+module_platform_driver(brcm_sata_phy_driver);
+
+MODULE_DESCRIPTION("Broadcom STB SATA PHY driver");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Marc Carino");
+MODULE_AUTHOR("Brian Norris");
+MODULE_ALIAS("platform:phy-brcmstb-sata");
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 5/5] ARM: dts: brcmstb: add nodes for SATA controller and PHY

[Brian Norris]

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
v2:
  - fix up some typos
  - account for binding changes in previous patches

 arch/arm/boot/dts/bcm7445.dtsi | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/arch/arm/boot/dts/bcm7445.dtsi b/arch/arm/boot/dts/bcm7445.dtsi
index 9eaeac8dce1b..2abb639723d2 100644
--- a/arch/arm/boot/dts/bcm7445.dtsi
+++ b/arch/arm/boot/dts/bcm7445.dtsi
@@ -108,6 +108,43 @@
 			brcm,int-map-mask = <0x25c>, <0x7000000>;
 			brcm,int-fwd-mask = <0x70000>;
 		};
+
+		sata@45a000 {
+			compatible = "brcm,bcm7445-ahci", "brcm,sata3-ahci";
+			reg-names = "ahci", "top-ctrl";
+			reg = <0x45a000 0xa9c>, <0x458040 0x24>;
+			interrupts = <GIC_SPI 30 0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			sata0: sata-port@0 {
+				reg = <0>;
+				phys = <&sata_phy0>;
+			};
+
+			sata1: sata-port@1 {
+				reg = <1>;
+				phys = <&sata_phy1>;
+			};
+		};
+
+		sata_phy: sata-phy@458100 {
+			compatible = "brcm,bcm7445-sata-phy", "brcm,phy-sata3";
+			reg = <0x458100 0x1f00>;
+			reg-names = "phy";
+			#address-cells = <0x1>;
+			#size-cells = <0x0>;
+
+			sata_phy0: sata-phy@0 {
+				reg = <0>;
+				#phy-cells = <0>;
+			};
+
+			sata_phy1: sata-phy@1 {
+				reg = <1>;
+				#phy-cells = <0>;
+			};
+		};
 	};
 
 	smpboot {
-- 
1.9.1

Re: [PATCH v2 0/5] AHCI and SATA PHY support for Broadcom STB SoCs

[Kishon Vijay Abraham I]

Hi,

On Thursday 23 April 2015 08:29 AM, Brian Norris wrote:
> 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.

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

Cheers
Kishon

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

Re: [PATCH v2 0/5] AHCI and SATA PHY support for Broadcom STB SoCs

[Brian Norris]

Hi Kishon,

On Mon, May 11, 2015 at 08:11:01PM +0530, Kishon Vijay Abraham I wrote:
> On Thursday 23 April 2015 08:29 AM, Brian Norris wrote:
> >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.
> 
> I don't see any problems with the PHY patches. Let me know If I can
> take this via linux-phy tree.

I think the only remaining objections were about endianness.
Incidentally, the PHY driver was using readl()/writel(), so that should
be OK then. It might be a problem for big endian MIPS, but this
particular PHY IP is not available on MIPS. If it ever is used there,
then we may need to patch in something like this:

	if (big endian MIPS)
		iowrite32be(val, addr);
	else
		iowrite32(val, addr);

But for now, I think all is OK, so feel free to take it. I'll just
rework/resend the SATA driver, SATA binding, and ARM/dts updates.

Brian