AHCI and SATA PHY support for Broadcom STB SoCs

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

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

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

drivers/ata/ahci_brcmstb.c line 189

Quoted 4 lines · Brian Norris

drivers/ata/ahci_brcmstb.c new file +4 -0

+       res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ahci");
+       ahci = devm_ioremap_resource(&pdev->dev, res);
+       if (IS_ERR(ahci))
+               return 0;

You should propagate 'return PTR_ERR(ahci)' instead.

Quoted 3 lines · Fabio Estevam

On Tue, May 12, 2015 at 8:28 PM, Brian Norris
<[email protected]> wrote:

Quoted 6 lines

drivers/ata/ahci_brcmstb.c line 189

Quoted 4 lines

drivers/ata/ahci_brcmstb.c new file +4 -0

+       res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ahci");
+       ahci = devm_ioremap_resource(&pdev->dev, res);
+       if (IS_ERR(ahci))
+               return 0;

You should propagate 'return PTR_ERR(ahci)' instead.

Brian, can you please update the patch?

Quoted 4 lines · Tejun Heo

Quoted 3 lines · Fabio Estevam

On Tue, May 12, 2015 at 8:28 PM, Brian Norris
<[email protected]> wrote:

Quoted 8 lines

Quoted 6 lines

drivers/ata/ahci_brcmstb.c line 189

Quoted 4 lines

drivers/ata/ahci_brcmstb.c new file +4 -0

+       res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ahci");
+       ahci = devm_ioremap_resource(&pdev->dev, res);
+       if (IS_ERR(ahci))
+               return 0;

You should propagate 'return PTR_ERR(ahci)' instead.

Brian, can you please update the patch?

Note how the function brcm_ahci_get_portmask() is used. In brcm_ahci_probe():

        priv->port_mask = brcm_ahci_get_portmask(pdev, priv);
        if (!priv->port_mask)
                return -ENODEV;

I could try to change the return semantics if really needed. Or I could add some comments on brcm_ahci_get_portmask() to note its return semantics. Or I could just leave the driver as-is. Your call.

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

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

---
v3: no change

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

drivers/phy/Kconfig +9 -0

index a53bd5b52df9..36788b6f0220 100644
@@ -309,4 +309,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

drivers/phy/Makefile +1 -0

index f12625178780..c61f3fdd191e 100644
@@ -40,3 +40,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

drivers/phy/phy-brcmstb-sata.c new file +41 -0

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

drivers/phy/phy-brcmstb-sata.c new file +28 -0

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

drivers/phy/phy-brcmstb-sata.c new file +99 -0

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

Hi,

Quoted 4 lines · Brian Norris

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

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

couple of minor comments below

Quoted 91 lines

---
v3: no change

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

drivers/phy/Kconfig +9 -0

index a53bd5b52df9..36788b6f0220 100644
@@ -309,4 +309,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

drivers/phy/Makefile +1 -0

index f12625178780..c61f3fdd191e 100644
@@ -40,3 +40,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

drivers/phy/phy-brcmstb-sata.c new file +45 -0

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

drivers/phy/phy-brcmstb-sata.c line 51

Quoted 6 lines

drivers/phy/phy-brcmstb-sata.c new file +6 -0

+	void __iomem *phy_base;
+
+	struct brcm_sata_port phys[MAX_PORTS];
+};
+
+enum sata_mdio_phy_regs_28nm {

Why should these defines be in enum?

Quoted 32 lines

drivers/phy/phy-brcmstb-sata.c new file +32 -0

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

drivers/phy/phy-brcmstb-sata.c line 89

Quoted 6 lines

drivers/phy/phy-brcmstb-sata.c new file +6 -0

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

brcm_sata_cfg_ssc_28nm to make it similar to other functions.

Quoted 103 lines

drivers/phy/phy-brcmstb-sata.c new file +103 -0

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

drivers/phy/phy-brcmstb-sata.c line 198

Quoted 6 lines

drivers/phy/phy-brcmstb-sata.c new file +6 -0

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

lets not make the boot noisy. Change to dev_dbg?

Quoted 100 lines · Kishon Vijay Abraham I

Hi,

Quoted 4 lines · Brian Norris

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

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

couple of minor comments below

Quoted 91 lines

---
v3: no change

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

drivers/phy/Kconfig +9 -0

index a53bd5b52df9..36788b6f0220 100644
@@ -309,4 +309,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

drivers/phy/Makefile +1 -0

index f12625178780..c61f3fdd191e 100644
@@ -40,3 +40,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

drivers/phy/phy-brcmstb-sata.c new file +45 -0

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

Quoted 8 lines

drivers/phy/phy-brcmstb-sata.c line 51

Quoted 6 lines

drivers/phy/phy-brcmstb-sata.c new file +6 -0

+	void __iomem *phy_base;
+
+	struct brcm_sata_port phys[MAX_PORTS];
+};
+
+enum sata_mdio_phy_regs_28nm {

Why should these defines be in enum?

Why not? They're logically grouped this way, and IMO, they look nicer. You can see drivers/ata/ahci.h for a similar example.

Quoted 33 lines

Quoted 33 lines

drivers/phy/phy-brcmstb-sata.c new file +32 -0

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

Quoted 8 lines

drivers/phy/phy-brcmstb-sata.c line 89

Quoted 6 lines

drivers/phy/phy-brcmstb-sata.c new file +6 -0

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

brcm_sata_cfg_ssc_28nm to make it similar to other functions.

OK.

Quoted 103 lines

Quoted 103 lines

drivers/phy/phy-brcmstb-sata.c new file +103 -0

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

Quoted 8 lines

drivers/phy/phy-brcmstb-sata.c line 198

Quoted 6 lines

drivers/phy/phy-brcmstb-sata.c new file +6 -0

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

lets not make the boot noisy. Change to dev_dbg?

Why? What's the harm? And I thought we discussed this already. "Noisy" depends on your point of view; IMO, this is important information. If for some reason this driver wasn't loaded or probed, we'd want to know. Besides, dev_dbg() requires you to fiddle with both the log level and the dynamic debug boot parameters just to get these informational messages.

While sending the next version please also add a
MAINTAINERs entry for this driver.

Quoted 2 lines · Kishon Vijay Abraham I

While sending the next version please also add a
MAINTAINERs entry for this driver.

I think this has it covered, right?

https://lkml.org/lkml/2015/3/18/932

Quoted 4 lines · Brian Norris

Quoted 2 lines · Kishon Vijay Abraham I

While sending the next version please also add a
MAINTAINERs entry for this driver.

Quoted 3 lines

I think this has it covered, right?

https://lkml.org/lkml/2015/3/18/932

Kishon, this patch is on its way to arm-soc via a pull request:
http://www.spinics.net/lists/arm-kernel/msg418276.html

this should address your concern. Thanks!

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

Few changes since v2, and the changes are *only* to the SATA driver, not to any of the DT bindings or the PHY driver. I'm resending the whole series for completeness.

I'll list the changelog in each patch, but in summary:

v2 -> v3:
- straighten out endianness for big endian MIPS and ARM in AHCI driver
- rename sata_brcmstb.c to ahci_brcmstb.c
- bind ahci_brcmstb.c against the specific string, "brcm,bcm7445-ahci" instead of the generic one

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

I'll pick "[PATCH v3 2/5] Documentation: devicetree: add Broadcom SATA PHY binding" and
"[PATCH v4] phy: add Broadcom SATA3 PHY driver for Broadcom STB SoCs" in the linux-phy tree.

Hello,

Quoted 4 lines · Kishon Vijay Abraham I

I'll pick "[PATCH v3 2/5] Documentation: devicetree: add Broadcom SATA PHY binding" and
"[PATCH v4] phy: add Broadcom SATA3 PHY driver for Broadcom STB SoCs" in the linux-phy tree.

Applying patch 5 to libata/for-4.2. If more need to go through libata, please let me know.

Quoted 3 lines · Tejun Heo

Quoted 2 lines · Kishon Vijay Abraham I

I'll pick "[PATCH v3 2/5] Documentation: devicetree: add Broadcom SATA PHY binding" and

Quoted 5 lines

Quoted 2 lines

"[PATCH v4] phy: add Broadcom SATA3 PHY driver for Broadcom STB SoCs" in the linux-phy tree.

Applying patch 5 to libata/for-4.2. If more need to go through libata, please let me know.

lol not patch 5, the following one. Sorry.

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

Quoted 1 line

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

Thanks. Also, this one?

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

Quoted 5 lines · Brian Norris

Quoted 1 line

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

Thanks. Also, this one?

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

devicetree patches usually haven't gone through libata tho.

Is this
an exception?

Most subsystems take device tree binding patches for their constituent drivers. And I see you've been merging others:

e35b98849f25 ata: sata_rcar: Add r8a7793 device support 8340bfeb03 ahci: st: Update the ahci_st DT documentation af64dce4cb3a ahci: st: Provide DT bindings for ST's SATA implementation a1a205df6ee2 ahci: add support for Hisilicon sata

All have your sign-off / commit stamp.

Quoted 2 lines

Is this
an exception?

Not that I know of. Where would you like it to go instead?

But the rules have never been clear to me. If the subsystem maintainer is okay with it, I'm happy to take the patches. I'm just kinda curious why this doesn't go through devicetree tree while some other devicetree patches go through there.

Can somebody explain the
overall policy to me? I'm not looking for some absolute rules and exceptions are fine but I do wanna have a general direction.

It's possible you're confusing binding documentation with .dts source files? The DTS files (arch/*/boot/dts/) go through arch trees. For instance, the arm-soc maintainers have a structured process by which sub-architecture maintainers track .dts(i) file updates for their boards/chips and filter them up to Arnd, Olof, etc., via their separate 'dts' branches. That's why Florian took patch 5 to his tree.