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

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

[Brian Norris]

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 .../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 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>
---
 .../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..39eddd53b318
--- /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
+- phy-cells: generic PHY binding; must be 1
+- reg: register ranges for the PHY PCB interface, and for the PHY port control
+     registers found in the SATA_TOP_CTRL block (i.e., PHY_CTRL 1/2/3/4)
+- reg-names: should be "phy" and "port-ctrl"
+
+Sub-nodes:
+  Each port's PHY should be represented as a sub-node.
+
+Sub-nodes required properties:
+- reg: the PHY number
+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", "port-ctrl";
+		#phy-cells = <1>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		sata-phy@0 {
+			reg = <0>;
+		};
+
+		sata-phy@1 {
+			reg = <1>;
+		};
+	};
-- 
1.9.1

[PATCH 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>
---
 drivers/ata/Kconfig        |   9 +++
 drivers/ata/Makefile       |   1 +
 drivers/ata/sata_brcmstb.c | 148 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 158 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..2a3aff419332
--- /dev/null
+++ b/drivers/ata/sata_brcmstb.c
@@ -0,0 +1,148 @@
+/*
+ * 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
+
+#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;
+	struct ahci_host_priv *hpriv;
+
+	void __iomem *top_ctrl;
+};
+
+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_sata3_init_config(struct brcm_ahci_priv *priv)
+{
+	/* Configure endianness */
+	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)
+{
+	return ahci_platform_suspend(dev);
+}
+
+static int brcm_ahci_resume(struct device *dev)
+{
+	struct brcm_ahci_priv *priv = dev_get_drvdata(dev);
+
+	brcm_sata3_init_config(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;
+	dev_set_drvdata(dev, priv);
+	priv->dev = dev;
+
+	hpriv = ahci_platform_get_resources(pdev);
+	if (IS_ERR(hpriv))
+		return PTR_ERR(hpriv);
+
+	priv->hpriv = hpriv;
+
+	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_sata3_init_config(priv);
+
+	ret = ahci_platform_enable_resources(hpriv);
+	if (ret)
+		return ret;
+
+	hpriv->plat_data = priv;
+
+	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 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 = ata_platform_remove_one,
+	.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 3/5] ata: add Broadcom AHCI SATA3 driver for STB chips

[Florian Fainelli]

On 18/03/15 18:23, 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>
> ---
>  drivers/ata/Kconfig        |   9 +++
>  drivers/ata/Makefile       |   1 +
>  drivers/ata/sata_brcmstb.c | 148 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 158 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

We would probably want a select PHY_BRCMSTB_SATA here?

[snip]

> +
> +static void brcm_sata3_init_config(struct brcm_ahci_priv *priv)
> +{
> +	/* Configure endianness */
> +	writel((DATA_ENDIAN << 4) | (DATA_ENDIAN << 2) | (MMIO_ENDIAN << 0),
> +		priv->top_ctrl + SATA_TOP_CTRL_BUS_CTRL);

AFAIR, this portion of the initialization must be done in the host-CPU
native endianness, so __raw_writel() would be more appropriate, or we
could use Kevin's conditional I/O accessors and do either ioread32() or
ioread32be() based on the absence/presence of the "big-endian" property?


[snip]

> +
> +static const struct of_device_id ahci_of_match[] = {
> +	{.compatible = "brcm,sata3-ahci"},

The binding specifies brcm,bcm7445-ahci as a valid compatible string,
such that we would probably want to match it here for consistency.
-- 
Florian

[PATCH 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@gmail.com>
---
 drivers/phy/Kconfig            |   9 ++
 drivers/phy/Makefile           |   1 +
 drivers/phy/phy-brcmstb-sata.c | 333 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 343 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..413bc94225ac
--- /dev/null
+++ b/drivers/phy/phy-brcmstb-sata.c
@@ -0,0 +1,333 @@
+/*
+ * 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 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 MAX_PORTS					2
+/* Register offset between PHYs in port-ctrl */
+#define SATA_TOP_CTRL_PHY_CTRL_LEN			0x8
+/* 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 *port_ctrl;
+	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 *sata_phy_get_port_ctrl(struct brcm_sata_port *port)
+{
+	struct brcm_sata_phy *priv = port->phy_priv;
+
+	return priv->port_ctrl + (port->portnum * SATA_TOP_CTRL_PHY_CTRL_LEN);
+}
+static inline void __iomem *sata_phy_get_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 = sata_phy_get_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 void brcm_sata_phy_enable(struct brcm_sata_port *port)
+{
+	void __iomem *port_ctrl = sata_phy_get_port_ctrl(port);
+	void __iomem *p;
+	u32 reg;
+
+	/* clear PHY_DEFAULT_POWER_STATE */
+	p = port_ctrl + SATA_TOP_CTRL_PHY_CTRL_1;
+	reg = readl(p);
+	reg &= ~SATA_TOP_CTRL_1_PHY_DEFAULT_POWER_STATE;
+	writel(reg, p);
+
+	/* reset the PHY digital logic */
+	p = port_ctrl + SATA_TOP_CTRL_PHY_CTRL_2;
+	reg = 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;
+	writel(reg, p);
+	reg = readl(p);
+	reg |= SATA_TOP_CTRL_2_PHY_GLOBAL_RESET;
+	writel(reg, p);
+	reg = readl(p);
+	reg &= ~SATA_TOP_CTRL_2_PHY_GLOBAL_RESET;
+	writel(reg, p);
+	(void)readl(p);
+}
+
+static void brcm_sata_phy_disable(struct brcm_sata_port *port)
+{
+	void __iomem *port_ctrl = sata_phy_get_port_ctrl(port);
+	void __iomem *p;
+	u32 reg;
+
+	/* power-off the PHY digital logic */
+	p = port_ctrl + SATA_TOP_CTRL_PHY_CTRL_2;
+	reg = 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);
+	writel(reg, p);
+
+	/* set PHY_DEFAULT_POWER_STATE */
+	p = port_ctrl + SATA_TOP_CTRL_PHY_CTRL_1;
+	reg = readl(p);
+	reg |= SATA_TOP_CTRL_1_PHY_DEFAULT_POWER_STATE;
+	writel(reg, p);
+}
+
+static int brcmstb_sata_phy_power_on(struct phy *phy)
+{
+	struct brcm_sata_port *port = phy_get_drvdata(phy);
+
+	dev_info(port->phy_priv->dev, "powering on port %d\n", port->portnum);
+
+	brcm_sata_phy_enable(port);
+	cfg_ssc_28nm(port);
+
+	return 0;
+}
+
+static int brcmstb_sata_phy_power_off(struct phy *phy)
+{
+	struct brcm_sata_port *port = phy_get_drvdata(phy);
+
+	dev_info(port->phy_priv->dev, "powering off port %d\n", port->portnum);
+
+	brcm_sata_phy_disable(port);
+
+	return 0;
+}
+
+static struct phy_ops phy_ops_28nm = {
+	.power_on	= brcmstb_sata_phy_power_on,
+	.power_off	= brcmstb_sata_phy_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static struct phy *brcm_sata_phy_xlate(struct device *dev,
+				       struct of_phandle_args *args)
+{
+	struct brcm_sata_phy *priv = dev_get_drvdata(dev);
+	int i = args->args[0];
+
+	if (i >= MAX_PORTS || !priv->phys[i].phy) {
+		dev_err(dev, "invalid phy: %d\n", i);
+		return ERR_PTR(-ENODEV);
+	}
+
+	return priv->phys[i].phy;
+}
+
+static const struct of_device_id brcmstb_sata_phy_of_match[] = {
+	{ .compatible	= "brcm,bcm7445-sata-phy" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, brcmstb_sata_phy_of_match);
+
+static int brcmstb_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, "port-ctrl");
+	if (!res) {
+		dev_err(dev, "couldn't get port-ctrl resource\n");
+		return -EINVAL;
+	}
+	/*
+	 * Don't request region, since it may be within a region owned by the
+	 * SATA driver
+	 */
+	priv->port_ctrl = devm_ioremap(dev, res->start, resource_size(res));
+	if (!priv->port_ctrl) {
+		dev_err(dev, "couldn't remap: %pR\n", res);
+		return -ENOMEM;
+	}
+
+	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, NULL, &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, brcm_sata_phy_xlate);
+	if (IS_ERR(provider)) {
+		dev_err(dev, "could not register PHY provider\n");
+		return PTR_ERR(provider);
+	}
+
+	dev_info(dev, "registered %d ports\n", count);
+
+	return 0;
+}
+
+static int brcmstb_sata_phy_remove(struct platform_device *pdev)
+{
+	return 0;
+}
+
+static struct platform_driver brcmstb_sata_phy_driver = {
+	.probe	= brcmstb_sata_phy_probe,
+	.remove	= brcmstb_sata_phy_remove,
+	.driver	= {
+		.of_match_table	= brcmstb_sata_phy_of_match,
+		.name		= "brcmstb-sata-phy",
+	}
+};
+module_platform_driver(brcmstb_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

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

[Florian Fainelli]

On 18/03/15 18:23, Brian Norris wrote:
> Supports up to two ports which can each be powered on/off and configured
> independently.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---

[snip]

> +
> +static const struct of_device_id brcmstb_sata_phy_of_match[] = {
> +	{ .compatible	= "brcm,bcm7445-sata-phy" },

The binding document specifies "brcm,phy-sata3" as a fallback compatible
string, so we want to match it here.

[snip]

> +
> +	dev_info(dev, "registered %d ports\n", count);

"registered %d port(s)" ;)
-- 
Florian

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

[Hans de Goede]

Hi,

On 21-03-15 00:02, Florian Fainelli wrote:
> On 18/03/15 18:23, Brian Norris wrote:
>> Supports up to two ports which can each be powered on/off and configured
>> independently.
>>
>> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
>> ---
>
> [snip]
>
>> +
>> +static const struct of_device_id brcmstb_sata_phy_of_match[] = {
>> +	{ .compatible	= "brcm,bcm7445-sata-phy" },
>
> The binding document specifies "brcm,phy-sata3" as a fallback compatible
> string, so we want to match it here.

Erm no we do not, what if another sata3 brcm phy comes along which
is not compatible, both would use "brcm,phy-sata3" as fallback / for
informational purposes, but we do not want this driver to be binding
to that other phy, which it would do with your suggestion.

Regards,

Hans

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

[Kishon Vijay Abraham I]

Hi,

On Thursday 19 March 2015 06:53 AM, Brian Norris wrote:
> Supports up to two ports which can each be powered on/off and configured
> independently.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/phy/Kconfig            |   9 ++
>  drivers/phy/Makefile           |   1 +
>  drivers/phy/phy-brcmstb-sata.c | 333 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 343 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..413bc94225ac
> --- /dev/null
> +++ b/drivers/phy/phy-brcmstb-sata.c
> @@ -0,0 +1,333 @@
> +/*
> + * 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 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 MAX_PORTS					2
> +/* Register offset between PHYs in port-ctrl */
> +#define SATA_TOP_CTRL_PHY_CTRL_LEN			0x8
> +/* 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 *port_ctrl;
> +	void __iomem *phy_base;
> +
> +	struct brcm_sata_port phys[MAX_PORTS];

Can't we allocate memory for the PHYs dynamically?
> +};
> +
> +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 *sata_phy_get_port_ctrl(struct brcm_sata_port *port)
> +{
> +	struct brcm_sata_phy *priv = port->phy_priv;
> +
> +	return priv->port_ctrl + (port->portnum * SATA_TOP_CTRL_PHY_CTRL_LEN);
> +}
> +static inline void __iomem *sata_phy_get_phy_base(struct brcm_sata_port *port)

please use consistent names for all functions. Prefix all the functions
with brcmstb.
> +{
> +	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 = sata_phy_get_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 void brcm_sata_phy_enable(struct brcm_sata_port *port)
> +{
> +	void __iomem *port_ctrl = sata_phy_get_port_ctrl(port);
> +	void __iomem *p;
> +	u32 reg;
> +
> +	/* clear PHY_DEFAULT_POWER_STATE */
> +	p = port_ctrl + SATA_TOP_CTRL_PHY_CTRL_1;
> +	reg = readl(p);
> +	reg &= ~SATA_TOP_CTRL_1_PHY_DEFAULT_POWER_STATE;
> +	writel(reg, p);
> +
> +	/* reset the PHY digital logic */
> +	p = port_ctrl + SATA_TOP_CTRL_PHY_CTRL_2;
> +	reg = 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;
> +	writel(reg, p);
> +	reg = readl(p);
> +	reg |= SATA_TOP_CTRL_2_PHY_GLOBAL_RESET;
> +	writel(reg, p);
> +	reg = readl(p);
> +	reg &= ~SATA_TOP_CTRL_2_PHY_GLOBAL_RESET;
> +	writel(reg, p);
> +	(void)readl(p);
> +}
> +
> +static void brcm_sata_phy_disable(struct brcm_sata_port *port)
> +{
> +	void __iomem *port_ctrl = sata_phy_get_port_ctrl(port);
> +	void __iomem *p;
> +	u32 reg;
> +
> +	/* power-off the PHY digital logic */
> +	p = port_ctrl + SATA_TOP_CTRL_PHY_CTRL_2;
> +	reg = 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);
> +	writel(reg, p);
> +
> +	/* set PHY_DEFAULT_POWER_STATE */
> +	p = port_ctrl + SATA_TOP_CTRL_PHY_CTRL_1;
> +	reg = readl(p);
> +	reg |= SATA_TOP_CTRL_1_PHY_DEFAULT_POWER_STATE;
> +	writel(reg, p);
> +}
> +
> +static int brcmstb_sata_phy_power_on(struct phy *phy)
> +{
> +	struct brcm_sata_port *port = phy_get_drvdata(phy);
> +
> +	dev_info(port->phy_priv->dev, "powering on port %d\n", port->portnum);

dev_dbg?
> +
> +	brcm_sata_phy_enable(port);
> +	cfg_ssc_28nm(port);
> +
> +	return 0;
> +}
> +
> +static int brcmstb_sata_phy_power_off(struct phy *phy)
> +{
> +	struct brcm_sata_port *port = phy_get_drvdata(phy);
> +
> +	dev_info(port->phy_priv->dev, "powering off port %d\n", port->portnum);

same here..
> +
> +	brcm_sata_phy_disable(port);
> +
> +	return 0;
> +}
> +
> +static struct phy_ops phy_ops_28nm = {
> +	.power_on	= brcmstb_sata_phy_power_on,
> +	.power_off	= brcmstb_sata_phy_power_off,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static struct phy *brcm_sata_phy_xlate(struct device *dev,
> +				       struct of_phandle_args *args)
> +{
> +	struct brcm_sata_phy *priv = dev_get_drvdata(dev);
> +	int i = args->args[0];
> +
> +	if (i >= MAX_PORTS || !priv->phys[i].phy) {
> +		dev_err(dev, "invalid phy: %d\n", i);
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	return priv->phys[i].phy;
> +}

this xlate is not required at all if the controller device tree node has
phandle to the phy node (sub node) instead of the phy provider device tree
node.
> +
> +static const struct of_device_id brcmstb_sata_phy_of_match[] = {
> +	{ .compatible	= "brcm,bcm7445-sata-phy" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, brcmstb_sata_phy_of_match);
> +
> +static int brcmstb_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, "port-ctrl");
> +	if (!res) {
> +		dev_err(dev, "couldn't get port-ctrl resource\n");
> +		return -EINVAL;
> +	}
> +	/*
> +	 * Don't request region, since it may be within a region owned by the
> +	 * SATA driver

It should be in the SATA driver then. Why is it here?
> +	 */
> +	priv->port_ctrl = devm_ioremap(dev, res->start, resource_size(res));
> +	if (!priv->port_ctrl) {
> +		dev_err(dev, "couldn't remap: %pR\n", res);
> +		return -ENOMEM;
> +	}
> +
> +	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, NULL, &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, brcm_sata_phy_xlate);
> +	if (IS_ERR(provider)) {
> +		dev_err(dev, "could not register PHY provider\n");
> +		return PTR_ERR(provider);
> +	}
> +
> +	dev_info(dev, "registered %d ports\n", count);

lets not make the boot noisy. Make it dev_vdbg if it is required.
> +
> +	return 0;
> +}
> +
> +static int brcmstb_sata_phy_remove(struct platform_device *pdev)
> +{
> +	return 0;
> +}

Remove this function if it doesn't have anything to do.

Thanks
Kishon

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

[Brian Norris]

Hi Kishon,

On Thu, Mar 26, 2015 at 03:29:44AM +0530, Kishon Vijay Abraham I wrote:
> On Thursday 19 March 2015 06:53 AM, Brian Norris wrote:
> > Supports up to two ports which can each be powered on/off and configured
> > independently.
> > 
> > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > ---
> >  drivers/phy/Kconfig            |   9 ++
> >  drivers/phy/Makefile           |   1 +
> >  drivers/phy/phy-brcmstb-sata.c | 333 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 343 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..413bc94225ac
> > --- /dev/null
> > +++ b/drivers/phy/phy-brcmstb-sata.c
> > @@ -0,0 +1,333 @@
> > +/*
> > + * 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 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 MAX_PORTS					2
> > +/* Register offset between PHYs in port-ctrl */
> > +#define SATA_TOP_CTRL_PHY_CTRL_LEN			0x8
> > +/* 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 *port_ctrl;
> > +	void __iomem *phy_base;
> > +
> > +	struct brcm_sata_port phys[MAX_PORTS];
> 
> Can't we allocate memory for the PHYs dynamically?

Yes, but I don't see a lot of point for an IP that supports exactly 2
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 *sata_phy_get_port_ctrl(struct brcm_sata_port *port)
> > +{
> > +	struct brcm_sata_phy *priv = port->phy_priv;
> > +
> > +	return priv->port_ctrl + (port->portnum * SATA_TOP_CTRL_PHY_CTRL_LEN);
> > +}
> > +static inline void __iomem *sata_phy_get_phy_base(struct brcm_sata_port *port)
> 
> please use consistent names for all functions. Prefix all the functions
> with brcmstb.

Will fix.

> > +{
> > +	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 = sata_phy_get_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 void brcm_sata_phy_enable(struct brcm_sata_port *port)
> > +{
> > +	void __iomem *port_ctrl = sata_phy_get_port_ctrl(port);
> > +	void __iomem *p;
> > +	u32 reg;
> > +
> > +	/* clear PHY_DEFAULT_POWER_STATE */
> > +	p = port_ctrl + SATA_TOP_CTRL_PHY_CTRL_1;
> > +	reg = readl(p);
> > +	reg &= ~SATA_TOP_CTRL_1_PHY_DEFAULT_POWER_STATE;
> > +	writel(reg, p);
> > +
> > +	/* reset the PHY digital logic */
> > +	p = port_ctrl + SATA_TOP_CTRL_PHY_CTRL_2;
> > +	reg = 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;
> > +	writel(reg, p);
> > +	reg = readl(p);
> > +	reg |= SATA_TOP_CTRL_2_PHY_GLOBAL_RESET;
> > +	writel(reg, p);
> > +	reg = readl(p);
> > +	reg &= ~SATA_TOP_CTRL_2_PHY_GLOBAL_RESET;
> > +	writel(reg, p);
> > +	(void)readl(p);
> > +}
> > +
> > +static void brcm_sata_phy_disable(struct brcm_sata_port *port)
> > +{
> > +	void __iomem *port_ctrl = sata_phy_get_port_ctrl(port);
> > +	void __iomem *p;
> > +	u32 reg;
> > +
> > +	/* power-off the PHY digital logic */
> > +	p = port_ctrl + SATA_TOP_CTRL_PHY_CTRL_2;
> > +	reg = 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);
> > +	writel(reg, p);
> > +
> > +	/* set PHY_DEFAULT_POWER_STATE */
> > +	p = port_ctrl + SATA_TOP_CTRL_PHY_CTRL_1;
> > +	reg = readl(p);
> > +	reg |= SATA_TOP_CTRL_1_PHY_DEFAULT_POWER_STATE;
> > +	writel(reg, p);
> > +}
> > +
> > +static int brcmstb_sata_phy_power_on(struct phy *phy)
> > +{
> > +	struct brcm_sata_port *port = phy_get_drvdata(phy);
> > +
> > +	dev_info(port->phy_priv->dev, "powering on port %d\n", port->portnum);
> 
> dev_dbg?

See comments below.

> > +
> > +	brcm_sata_phy_enable(port);
> > +	cfg_ssc_28nm(port);
> > +
> > +	return 0;
> > +}
> > +
> > +static int brcmstb_sata_phy_power_off(struct phy *phy)
> > +{
> > +	struct brcm_sata_port *port = phy_get_drvdata(phy);
> > +
> > +	dev_info(port->phy_priv->dev, "powering off port %d\n", port->portnum);
> 
> same here..
> > +
> > +	brcm_sata_phy_disable(port);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct phy_ops phy_ops_28nm = {
> > +	.power_on	= brcmstb_sata_phy_power_on,
> > +	.power_off	= brcmstb_sata_phy_power_off,
> > +	.owner		= THIS_MODULE,
> > +};
> > +
> > +static struct phy *brcm_sata_phy_xlate(struct device *dev,
> > +				       struct of_phandle_args *args)
> > +{
> > +	struct brcm_sata_phy *priv = dev_get_drvdata(dev);
> > +	int i = args->args[0];
> > +
> > +	if (i >= MAX_PORTS || !priv->phys[i].phy) {
> > +		dev_err(dev, "invalid phy: %d\n", i);
> > +		return ERR_PTR(-ENODEV);
> > +	}
> > +
> > +	return priv->phys[i].phy;
> > +}
> 
> this xlate is not required at all if the controller device tree node has
> phandle to the phy node (sub node) instead of the phy provider device tree
> node.

That doesn't match any convention I see in existing SATA phy bindings,
nor do I see how the existing of_phy_simple_xlate() would support this,
unless I instantiate a device for each port's PHY. If I adjust the
device tree as you suggest, and use of_phy_simple_xlate() instead of
this, of_phy_get() can't find the PHY provider, because the provider is
registered to the parent, not the subnode.

Can you elaborate on your suggestion?

> > +
> > +static const struct of_device_id brcmstb_sata_phy_of_match[] = {
> > +	{ .compatible	= "brcm,bcm7445-sata-phy" },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, brcmstb_sata_phy_of_match);
> > +
> > +static int brcmstb_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, "port-ctrl");
> > +	if (!res) {
> > +		dev_err(dev, "couldn't get port-ctrl resource\n");
> > +		return -EINVAL;
> > +	}
> > +	/*
> > +	 * Don't request region, since it may be within a region owned by the
> > +	 * SATA driver
> 
> It should be in the SATA driver then. Why is it here?

Did you read the discussion branching here?

http://article.gmane.org/gmane.linux.drivers.devicetree/114637

I've seen the exact opposite suggestion already (move it to the PHY
driver), and I'm not sure either suggestion is correct. The same
register block has registers for both the PHY and the SATA controller.

> > +	 */
> > +	priv->port_ctrl = devm_ioremap(dev, res->start, resource_size(res));
> > +	if (!priv->port_ctrl) {
> > +		dev_err(dev, "couldn't remap: %pR\n", res);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	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, NULL, &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, brcm_sata_phy_xlate);
> > +	if (IS_ERR(provider)) {
> > +		dev_err(dev, "could not register PHY provider\n");
> > +		return PTR_ERR(provider);
> > +	}
> > +
> > +	dev_info(dev, "registered %d ports\n", count);
> 
> lets not make the boot noisy. Make it dev_vdbg if it is required.

I think it's important to have at least some of the three informational
prints that you're suggesting turn into dbg. It's pretty important to
see that we're powering on one or more PHY ports, for both
power/correctness concerns (trying to power on a port that is
OTP-disabled, for instance) and debugging concerns (the suggestions you
made about the device tree yielded a dead SATA, and it was obvious,
because the "powering on" prints were missing).

I'd kinda like to see the previous power on/off prints above stay as
dev_info(), though the "registered" print might be superfluous, as the
registration info should show up in sysfs.

Related: I don't see any API for monitoring the PHY status from user
space. e.g., there's nothing useful in /sys/class/phy/*/.

> > +
> > +	return 0;
> > +}
> > +
> > +static int brcmstb_sata_phy_remove(struct platform_device *pdev)
> > +{
> > +	return 0;
> > +}
> 
> Remove this function if it doesn't have anything to do.

I was confused; I thought that that the driver model would not allow a
device to be detached if there was no remove function. Will remove now.

Thanks for the review,
Brian

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

[Kishon Vijay Abraham I]

Hi,

On Saturday 28 March 2015 05:58 AM, Brian Norris wrote:
> Hi Kishon,
>
> On Thu, Mar 26, 2015 at 03:29:44AM +0530, Kishon Vijay Abraham I wrote:
>> On Thursday 19 March 2015 06:53 AM, Brian Norris wrote:
>>> Supports up to two ports which can each be powered on/off and configured
>>> independently.
>>>
>>> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
>>> ---
>>>   drivers/phy/Kconfig            |   9 ++
>>>   drivers/phy/Makefile           |   1 +
>>>   drivers/phy/phy-brcmstb-sata.c | 333 +++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 343 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..413bc94225ac
>>> --- /dev/null
>>> +++ b/drivers/phy/phy-brcmstb-sata.c
>>> @@ -0,0 +1,333 @@
>>> +/*
>>> + * 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 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 MAX_PORTS					2
>>> +/* Register offset between PHYs in port-ctrl */
>>> +#define SATA_TOP_CTRL_PHY_CTRL_LEN			0x8
>>> +/* 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 *port_ctrl;
>>> +	void __iomem *phy_base;
>>> +
>>> +	struct brcm_sata_port phys[MAX_PORTS];
>>
>> Can't we allocate memory for the PHYs dynamically?
>
> Yes, but I don't see a lot of point for an IP that supports exactly 2
> 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 *sata_phy_get_port_ctrl(struct brcm_sata_port *port)
>>> +{
>>> +	struct brcm_sata_phy *priv = port->phy_priv;
>>> +
>>> +	return priv->port_ctrl + (port->portnum * SATA_TOP_CTRL_PHY_CTRL_LEN);
>>> +}
>>> +static inline void __iomem *sata_phy_get_phy_base(struct brcm_sata_port *port)
>>
>> please use consistent names for all functions. Prefix all the functions
>> with brcmstb.
>
> Will fix.
>
>>> +{
>>> +	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 = sata_phy_get_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 void brcm_sata_phy_enable(struct brcm_sata_port *port)
>>> +{
>>> +	void __iomem *port_ctrl = sata_phy_get_port_ctrl(port);
>>> +	void __iomem *p;
>>> +	u32 reg;
>>> +
>>> +	/* clear PHY_DEFAULT_POWER_STATE */
>>> +	p = port_ctrl + SATA_TOP_CTRL_PHY_CTRL_1;
>>> +	reg = readl(p);
>>> +	reg &= ~SATA_TOP_CTRL_1_PHY_DEFAULT_POWER_STATE;
>>> +	writel(reg, p);
>>> +
>>> +	/* reset the PHY digital logic */
>>> +	p = port_ctrl + SATA_TOP_CTRL_PHY_CTRL_2;
>>> +	reg = 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;
>>> +	writel(reg, p);
>>> +	reg = readl(p);
>>> +	reg |= SATA_TOP_CTRL_2_PHY_GLOBAL_RESET;
>>> +	writel(reg, p);
>>> +	reg = readl(p);
>>> +	reg &= ~SATA_TOP_CTRL_2_PHY_GLOBAL_RESET;
>>> +	writel(reg, p);
>>> +	(void)readl(p);
>>> +}
>>> +
>>> +static void brcm_sata_phy_disable(struct brcm_sata_port *port)
>>> +{
>>> +	void __iomem *port_ctrl = sata_phy_get_port_ctrl(port);
>>> +	void __iomem *p;
>>> +	u32 reg;
>>> +
>>> +	/* power-off the PHY digital logic */
>>> +	p = port_ctrl + SATA_TOP_CTRL_PHY_CTRL_2;
>>> +	reg = 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);
>>> +	writel(reg, p);
>>> +
>>> +	/* set PHY_DEFAULT_POWER_STATE */
>>> +	p = port_ctrl + SATA_TOP_CTRL_PHY_CTRL_1;
>>> +	reg = readl(p);
>>> +	reg |= SATA_TOP_CTRL_1_PHY_DEFAULT_POWER_STATE;
>>> +	writel(reg, p);
>>> +}
>>> +
>>> +static int brcmstb_sata_phy_power_on(struct phy *phy)
>>> +{
>>> +	struct brcm_sata_port *port = phy_get_drvdata(phy);
>>> +
>>> +	dev_info(port->phy_priv->dev, "powering on port %d\n", port->portnum);
>>
>> dev_dbg?
>
> See comments below.
>
>>> +
>>> +	brcm_sata_phy_enable(port);
>>> +	cfg_ssc_28nm(port);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int brcmstb_sata_phy_power_off(struct phy *phy)
>>> +{
>>> +	struct brcm_sata_port *port = phy_get_drvdata(phy);
>>> +
>>> +	dev_info(port->phy_priv->dev, "powering off port %d\n", port->portnum);
>>
>> same here..
>>> +
>>> +	brcm_sata_phy_disable(port);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct phy_ops phy_ops_28nm = {
>>> +	.power_on	= brcmstb_sata_phy_power_on,
>>> +	.power_off	= brcmstb_sata_phy_power_off,
>>> +	.owner		= THIS_MODULE,
>>> +};
>>> +
>>> +static struct phy *brcm_sata_phy_xlate(struct device *dev,
>>> +				       struct of_phandle_args *args)
>>> +{
>>> +	struct brcm_sata_phy *priv = dev_get_drvdata(dev);
>>> +	int i = args->args[0];
>>> +
>>> +	if (i >= MAX_PORTS || !priv->phys[i].phy) {
>>> +		dev_err(dev, "invalid phy: %d\n", i);
>>> +		return ERR_PTR(-ENODEV);
>>> +	}
>>> +
>>> +	return priv->phys[i].phy;
>>> +}
>>
>> this xlate is not required at all if the controller device tree node has
>> phandle to the phy node (sub node) instead of the phy provider device tree
>> node.
>
> That doesn't match any convention I see in existing SATA phy bindings,
> nor do I see how the existing of_phy_simple_xlate() would support this,
> unless I instantiate a device for each port's PHY. If I adjust the
> device tree as you suggest, and use of_phy_simple_xlate() instead of
> this, of_phy_get() can't find the PHY provider, because the provider is
> registered to the parent, not the subnode.

The phy core should still be able to get the PHY provider.
See this in of_phy_provider_lookup
                 for_each_child_of_node(phy_provider->dev->of_node, child)
                         if (child == node)
                                 return phy_provider;

Can you post your device tree node here?

>
> Can you elaborate on your suggestion?
>
>>> +
>>> +static const struct of_device_id brcmstb_sata_phy_of_match[] = {
>>> +	{ .compatible	= "brcm,bcm7445-sata-phy" },
>>> +	{},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, brcmstb_sata_phy_of_match);
>>> +
>>> +static int brcmstb_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, "port-ctrl");
>>> +	if (!res) {
>>> +		dev_err(dev, "couldn't get port-ctrl resource\n");
>>> +		return -EINVAL;
>>> +	}
>>> +	/*
>>> +	 * Don't request region, since it may be within a region owned by the
>>> +	 * SATA driver
>>
>> It should be in the SATA driver then. Why is it here?
>
> Did you read the discussion branching here?
>
> http://article.gmane.org/gmane.linux.drivers.devicetree/114637
>
> I've seen the exact opposite suggestion already (move it to the PHY
> driver), and I'm not sure either suggestion is correct. The same
> register block has registers for both the PHY and the SATA controller.

IMHO the register space shouldn't be defined based on the programming sequence
but by where the register is actually present in the IP. From that thread it
doesn't look like it is present in the PHY IP.
>
>>> +	 */
>>> +	priv->port_ctrl = devm_ioremap(dev, res->start, resource_size(res));
>>> +	if (!priv->port_ctrl) {
>>> +		dev_err(dev, "couldn't remap: %pR\n", res);
>>> +		return -ENOMEM;
>>> +	}
>>> +
>>> +	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, NULL, &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, brcm_sata_phy_xlate);
>>> +	if (IS_ERR(provider)) {
>>> +		dev_err(dev, "could not register PHY provider\n");
>>> +		return PTR_ERR(provider);
>>> +	}
>>> +
>>> +	dev_info(dev, "registered %d ports\n", count);
>>
>> lets not make the boot noisy. Make it dev_vdbg if it is required.
>
> I think it's important to have at least some of the three informational
> prints that you're suggesting turn into dbg. It's pretty important to
> see that we're powering on one or more PHY ports, for both
> power/correctness concerns (trying to power on a port that is
> OTP-disabled, for instance) and debugging concerns (the suggestions you
> made about the device tree yielded a dead SATA, and it was obvious,
> because the "powering on" prints were missing).

All these are debugging info. Hence it's better to keep in dev_vdbg or dev_dbg.
>
> I'd kinda like to see the previous power on/off prints above stay as
> dev_info(), though the "registered" print might be superfluous, as the
> registration info should show up in sysfs.
>
> Related: I don't see any API for monitoring the PHY status from user
> space. e.g., there's nothing useful in /sys/class/phy/*/.

Do you really need to monitor the PHY status? We should be careful about
exposing anything to the user space since it will become an ABI and we can
never modify it.
>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int brcmstb_sata_phy_remove(struct platform_device *pdev)
>>> +{
>>> +	return 0;
>>> +}
>>
>> Remove this function if it doesn't have anything to do.
>
> I was confused; I thought that that the driver model would not allow a
> device to be detached if there was no remove function. Will remove now.

okay. I'll cross check that again.

Thanks
Kishon

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

[Brian Norris]

On Tue, Mar 31, 2015 at 11:31:40AM +0530, Kishon Vijay Abraham I wrote:
> On Saturday 28 March 2015 05:58 AM, Brian Norris wrote:
> >On Thu, Mar 26, 2015 at 03:29:44AM +0530, Kishon Vijay Abraham I wrote:
> >>On Thursday 19 March 2015 06:53 AM, Brian Norris wrote:
> >>>+static struct phy *brcm_sata_phy_xlate(struct device *dev,
> >>>+				       struct of_phandle_args *args)
> >>>+{
> >>>+	struct brcm_sata_phy *priv = dev_get_drvdata(dev);
> >>>+	int i = args->args[0];
> >>>+
> >>>+	if (i >= MAX_PORTS || !priv->phys[i].phy) {
> >>>+		dev_err(dev, "invalid phy: %d\n", i);
> >>>+		return ERR_PTR(-ENODEV);
> >>>+	}
> >>>+
> >>>+	return priv->phys[i].phy;
> >>>+}
> >>
> >>this xlate is not required at all if the controller device tree node has
> >>phandle to the phy node (sub node) instead of the phy provider device tree
> >>node.
> >
> >That doesn't match any convention I see in existing SATA phy bindings,
> >nor do I see how the existing of_phy_simple_xlate() would support this,
> >unless I instantiate a device for each port's PHY. If I adjust the
> >device tree as you suggest, and use of_phy_simple_xlate() instead of
> >this, of_phy_get() can't find the PHY provider, because the provider is
> >registered to the parent, not the subnode.
> 
> The phy core should still be able to get the PHY provider.
> See this in of_phy_provider_lookup
>                 for_each_child_of_node(phy_provider->dev->of_node, child)
>                         if (child == node)
>                                 return phy_provider;

That just searches for children of the node. It doesn't walk parent
nodes.

> Can you post your device tree node here?

You mean patch 5?

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

> >
> >Can you elaborate on your suggestion?
> >
> >>>+
> >>>+static const struct of_device_id brcmstb_sata_phy_of_match[] = {
> >>>+	{ .compatible	= "brcm,bcm7445-sata-phy" },
> >>>+	{},
> >>>+};
> >>>+MODULE_DEVICE_TABLE(of, brcmstb_sata_phy_of_match);
> >>>+
> >>>+static int brcmstb_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, "port-ctrl");
> >>>+	if (!res) {
> >>>+		dev_err(dev, "couldn't get port-ctrl resource\n");
> >>>+		return -EINVAL;
> >>>+	}
> >>>+	/*
> >>>+	 * Don't request region, since it may be within a region owned by the
> >>>+	 * SATA driver
> >>
> >>It should be in the SATA driver then. Why is it here?
> >
> >Did you read the discussion branching here?
> >
> >http://article.gmane.org/gmane.linux.drivers.devicetree/114637
> >
> >I've seen the exact opposite suggestion already (move it to the PHY
> >driver), and I'm not sure either suggestion is correct. The same
> >register block has registers for both the PHY and the SATA controller.
> 
> IMHO the register space shouldn't be defined based on the programming sequence
> but by where the register is actually present in the IP. From that thread it
> doesn't look like it is present in the PHY IP.

If you say so. But it has plenty of PHY controls packed into those
registers, so are you just recommending handling those bits from the
SATA driver?

...

> >>lets not make the boot noisy. Make it dev_vdbg if it is required.
> >
> >I think it's important to have at least some of the three informational
> >prints that you're suggesting turn into dbg. It's pretty important to
> >see that we're powering on one or more PHY ports, for both
> >power/correctness concerns (trying to power on a port that is
> >OTP-disabled, for instance) and debugging concerns (the suggestions you
> >made about the device tree yielded a dead SATA, and it was obvious,
> >because the "powering on" prints were missing).
> 
> All these are debugging info. Hence it's better to keep in dev_vdbg or dev_dbg.
> >
> >I'd kinda like to see the previous power on/off prints above stay as
> >dev_info(), though the "registered" print might be superfluous, as the
> >registration info should show up in sysfs.
> >
> >Related: I don't see any API for monitoring the PHY status from user
> >space. e.g., there's nothing useful in /sys/class/phy/*/.
> 
> Do you really need to monitor the PHY status? We should be careful about
> exposing anything to the user space since it will become an ABI and we can
> never modify it.

Not really, but the debugging info (which you want me to remove by
default, and which is unretrievable after system boot) is the next
easiest solution. It doesn't provide an ABI, exactly, but it keeps the
info readily available.

> >>>+
> >>>+	return 0;
> >>>+}
> >>>+
> >>>+static int brcmstb_sata_phy_remove(struct platform_device *pdev)
> >>>+{
> >>>+	return 0;
> >>>+}
> >>
> >>Remove this function if it doesn't have anything to do.
> >
> >I was confused; I thought that that the driver model would not allow a
> >device to be detached if there was no remove function. Will remove now.
> 
> okay. I'll cross check that again.

I haven't done an exhaustive search, but it looks like
__device_release_driver() handles a missing .remove just fine.

Brian

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

[Kishon Vijay Abraham I]

On Thursday 02 April 2015 07:58 AM, Brian Norris wrote:
> On Tue, Mar 31, 2015 at 11:31:40AM +0530, Kishon Vijay Abraham I wrote:
>> On Saturday 28 March 2015 05:58 AM, Brian Norris wrote:
>>> On Thu, Mar 26, 2015 at 03:29:44AM +0530, Kishon Vijay Abraham I wrote:
>>>> On Thursday 19 March 2015 06:53 AM, Brian Norris wrote:
>>>>> +static struct phy *brcm_sata_phy_xlate(struct device *dev,
>>>>> +				       struct of_phandle_args *args)
>>>>> +{
>>>>> +	struct brcm_sata_phy *priv = dev_get_drvdata(dev);
>>>>> +	int i = args->args[0];
>>>>> +
>>>>> +	if (i >= MAX_PORTS || !priv->phys[i].phy) {
>>>>> +		dev_err(dev, "invalid phy: %d\n", i);
>>>>> +		return ERR_PTR(-ENODEV);
>>>>> +	}
>>>>> +
>>>>> +	return priv->phys[i].phy;
>>>>> +}
>>>>
>>>> this xlate is not required at all if the controller device tree node has
>>>> phandle to the phy node (sub node) instead of the phy provider device tree
>>>> node.
>>>
>>> That doesn't match any convention I see in existing SATA phy bindings,
>>> nor do I see how the existing of_phy_simple_xlate() would support this,
>>> unless I instantiate a device for each port's PHY. If I adjust the
>>> device tree as you suggest, and use of_phy_simple_xlate() instead of
>>> this, of_phy_get() can't find the PHY provider, because the provider is
>>> registered to the parent, not the subnode.
>>
>> The phy core should still be able to get the PHY provider.
>> See this in of_phy_provider_lookup
>>                  for_each_child_of_node(phy_provider->dev->of_node, child)
>>                          if (child == node)
>>                                  return phy_provider;
>
> That just searches for children of the node. It doesn't walk parent
> nodes.

okay.. in your phy_create pass the np of the PHYs (sub-node pointer to phy 
provider).

>
>> Can you post your device tree node here?
>
> You mean patch 5?
>
> https://lkml.org/lkml/2015/3/18/937
>
>>>
>>> Can you elaborate on your suggestion?

Change the dt node to something like below..

+
+		sata@f045a000 {
+			<snip>
+
+			sata0: sata-port@0 {
+				reg = <0>;
+				phys = <&sata_phy0>;
+			};
+
+			sata1: sata-port@1 {
+				reg = <1>;
+				phys = <&sata_phy1>;
+			};
+		};
+
+		sata_phy: sata-phy@f0458100 {
+			compatible = "brcm,bcm7445-sata-phy", "brcm,phy-sata3";
+			reg = <0x458100 0x1e00>, <0x45804c 0x10>;
+			reg-names = "phy", "port-ctrl";
+			#address-cells = <0x1>;
+			#size-cells = <0x0>;
+
+			sata_phy0: sata-phy@0 {
+				reg = <0>;
+				#phy-cells = <0>;
+			};
+
+			sata_phy1: sata-phy@1 {
+				#phy-cells = <0>;
+			};
+		};
  	};
>>>
>>>>> +
>>>>> +static const struct of_device_id brcmstb_sata_phy_of_match[] = {
>>>>> +	{ .compatible	= "brcm,bcm7445-sata-phy" },
>>>>> +	{},
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(of, brcmstb_sata_phy_of_match);
>>>>> +
>>>>> +static int brcmstb_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, "port-ctrl");
>>>>> +	if (!res) {
>>>>> +		dev_err(dev, "couldn't get port-ctrl resource\n");
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +	/*
>>>>> +	 * Don't request region, since it may be within a region owned by the
>>>>> +	 * SATA driver
>>>>
>>>> It should be in the SATA driver then. Why is it here?
>>>
>>> Did you read the discussion branching here?
>>>
>>> http://article.gmane.org/gmane.linux.drivers.devicetree/114637
>>>
>>> I've seen the exact opposite suggestion already (move it to the PHY
>>> driver), and I'm not sure either suggestion is correct. The same
>>> register block has registers for both the PHY and the SATA controller.
>>
>> IMHO the register space shouldn't be defined based on the programming sequence
>> but by where the register is actually present in the IP. From that thread it
>> doesn't look like it is present in the PHY IP.
>
> If you say so. But it has plenty of PHY controls packed into those
> registers, so are you just recommending handling those bits from the
> SATA driver?

Yes. Let's program only the PHY IP in this driver.
>
> ...
>
>>>> lets not make the boot noisy. Make it dev_vdbg if it is required.
>>>
>>> I think it's important to have at least some of the three informational
>>> prints that you're suggesting turn into dbg. It's pretty important to
>>> see that we're powering on one or more PHY ports, for both
>>> power/correctness concerns (trying to power on a port that is
>>> OTP-disabled, for instance) and debugging concerns (the suggestions you
>>> made about the device tree yielded a dead SATA, and it was obvious,
>>> because the "powering on" prints were missing).
>>
>> All these are debugging info. Hence it's better to keep in dev_vdbg or dev_dbg.
>>>
>>> I'd kinda like to see the previous power on/off prints above stay as
>>> dev_info(), though the "registered" print might be superfluous, as the
>>> registration info should show up in sysfs.
>>>
>>> Related: I don't see any API for monitoring the PHY status from user
>>> space. e.g., there's nothing useful in /sys/class/phy/*/.
>>
>> Do you really need to monitor the PHY status? We should be careful about
>> exposing anything to the user space since it will become an ABI and we can
>> never modify it.
>
> Not really, but the debugging info (which you want me to remove by
> default, and which is unretrievable after system boot) is the next

I didn't ask you to remove it. I just asked you to keep in dev_dbg. If you are 
interested in the debugging info, all you need to do is change the debug log level.

Cheers
Kishon

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

[Brian Norris]

On Tue, Apr 07, 2015 at 11:37:43AM +0530, Kishon Vijay Abraham I wrote:
> On Thursday 02 April 2015 07:58 AM, Brian Norris wrote:
> >On Tue, Mar 31, 2015 at 11:31:40AM +0530, Kishon Vijay Abraham I wrote:
> >>On Saturday 28 March 2015 05:58 AM, Brian Norris wrote:
> >>>On Thu, Mar 26, 2015 at 03:29:44AM +0530, Kishon Vijay Abraham I wrote:
> >>>>On Thursday 19 March 2015 06:53 AM, Brian Norris wrote:
> >>>>>+static struct phy *brcm_sata_phy_xlate(struct device *dev,
> >>>>>+				       struct of_phandle_args *args)
> >>>>>+{
> >>>>>+	struct brcm_sata_phy *priv = dev_get_drvdata(dev);
> >>>>>+	int i = args->args[0];
> >>>>>+
> >>>>>+	if (i >= MAX_PORTS || !priv->phys[i].phy) {
> >>>>>+		dev_err(dev, "invalid phy: %d\n", i);
> >>>>>+		return ERR_PTR(-ENODEV);
> >>>>>+	}
> >>>>>+
> >>>>>+	return priv->phys[i].phy;
> >>>>>+}
> >>>>
> >>>>this xlate is not required at all if the controller device tree node has
> >>>>phandle to the phy node (sub node) instead of the phy provider device tree
> >>>>node.
> >>>
> >>>That doesn't match any convention I see in existing SATA phy bindings,
> >>>nor do I see how the existing of_phy_simple_xlate() would support this,
> >>>unless I instantiate a device for each port's PHY. If I adjust the
> >>>device tree as you suggest, and use of_phy_simple_xlate() instead of
> >>>this, of_phy_get() can't find the PHY provider, because the provider is
> >>>registered to the parent, not the subnode.
> >>
> >>The phy core should still be able to get the PHY provider.
> >>See this in of_phy_provider_lookup
> >>                 for_each_child_of_node(phy_provider->dev->of_node, child)
> >>                         if (child == node)
> >>                                 return phy_provider;
> >
> >That just searches for children of the node. It doesn't walk parent
> >nodes.
> 
> okay.. in your phy_create pass the np of the PHYs (sub-node pointer
> to phy provider).

Ah, I see. I completely passed over the 2nd parameter to phy_create()...
Thanks for the tip.

> >>Can you post your device tree node here?
> >
> >You mean patch 5?
> >
> >https://lkml.org/lkml/2015/3/18/937
> >
> >>>
> >>>Can you elaborate on your suggestion?
> 
> Change the dt node to something like below..

[snip]

Yes, that worked. Thanks.

OK, I'll fix this up and send out v2 shortly.

Brian

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

[Brian Norris]

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
Light dependency on:
  http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/331921.html
for the surrounding text.

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

diff --git a/arch/arm/boot/dts/bcm7445.dtsi b/arch/arm/boot/dts/bcm7445.dtsi
index 9eaeac8dce1b..7a7c4d8c2afe 100644
--- a/arch/arm/boot/dts/bcm7445.dtsi
+++ b/arch/arm/boot/dts/bcm7445.dtsi
@@ -108,6 +108,42 @@
 			brcm,int-map-mask = <0x25c>, <0x7000000>;
 			brcm,int-fwd-mask = <0x70000>;
 		};
+
+		sata@f045a000 {
+			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_phy 0>;
+			};
+
+			sata1: sata-port@1 {
+				reg = <1>;
+				phys = <&sata_phy 1>;
+			};
+		};
+
+		sata_phy: sata-phy@f0458100 {
+			compatible = "brcm,bcm7445-sata-phy", "brcm,phy-sata3";
+			reg = <0x458100 0x1e00>, <0x45804c 0x10>;
+			reg-names = "phy", "port-ctrl";
+			#phy-cells = <1>;
+			#address-cells = <0x1>;
+			#size-cells = <0x0>;
+
+			sata-phy@0 {
+				reg = <0>;
+			};
+
+			sata-phy@1 {
+				reg = <1>;
+			};
+		};
 	};
 
 	smpboot {
-- 
1.9.1

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

[Hans de Goede]

Hi,

On 19-03-15 02:23, Brian Norris wrote:
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
> Light dependency on:
>    http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/331921.html
> for the surrounding text.
>
>   arch/arm/boot/dts/bcm7445.dtsi | 36 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 36 insertions(+)
>
> diff --git a/arch/arm/boot/dts/bcm7445.dtsi b/arch/arm/boot/dts/bcm7445.dtsi
> index 9eaeac8dce1b..7a7c4d8c2afe 100644
> --- a/arch/arm/boot/dts/bcm7445.dtsi
> +++ b/arch/arm/boot/dts/bcm7445.dtsi
> @@ -108,6 +108,42 @@
>   			brcm,int-map-mask = <0x25c>, <0x7000000>;
>   			brcm,int-fwd-mask = <0x70000>;
>   		};
> +
> +		sata@f045a000 {
> +			compatible = "brcm,bcm7445-ahci", "brcm,sata3-ahci";
> +			reg-names = "ahci", "top-ctrl";
> +			reg = <0x45a000 0xa9c>, <0x458040 0x24>;

Why not simply drop the second register range here, and the minimal top-ctrl
poking you need in the phy driver's phy_init function ?

This avoids the weird / ugly register overlap with the phy driver, and I think you
can then just use the ahci_platform driver unmodified.

> +			interrupts = <GIC_SPI 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>;
> +			};
> +		};
> +
> +		sata_phy: sata-phy@f0458100 {
> +			compatible = "brcm,bcm7445-sata-phy", "brcm,phy-sata3";
> +			reg = <0x458100 0x1e00>, <0x45804c 0x10>;

Why not simply use: reg = <0x458000 0x2000>, to me it seems that what you should
really be using here.

> +			reg-names = "phy", "port-ctrl";
> +			#phy-cells = <1>;
> +			#address-cells = <0x1>;
> +			#size-cells = <0x0>;
> +
> +			sata-phy@0 {
> +				reg = <0>;
> +			};
> +
> +			sata-phy@1 {
> +				reg = <1>;
> +			};
> +		};
>   	};
>
>   	smpboot {
>

Regards,

Hans

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

[Brian Norris]

Hi Hans,

Thanks for the review.

On Thu, Mar 19, 2015 at 12:10:25PM +0100, Hans de Goede wrote:
> On 19-03-15 02:23, Brian Norris wrote:
> >Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> >---
> >Light dependency on:
> >   http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/331921.html
> >for the surrounding text.
> >
> >  arch/arm/boot/dts/bcm7445.dtsi | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
> >diff --git a/arch/arm/boot/dts/bcm7445.dtsi b/arch/arm/boot/dts/bcm7445.dtsi
> >index 9eaeac8dce1b..7a7c4d8c2afe 100644
> >--- a/arch/arm/boot/dts/bcm7445.dtsi
> >+++ b/arch/arm/boot/dts/bcm7445.dtsi
> >@@ -108,6 +108,42 @@
> >  			brcm,int-map-mask = <0x25c>, <0x7000000>;
> >  			brcm,int-fwd-mask = <0x70000>;
> >  		};
> >+
> >+		sata@f045a000 {
> >+			compatible = "brcm,bcm7445-ahci", "brcm,sata3-ahci";
> >+			reg-names = "ahci", "top-ctrl";
> >+			reg = <0x45a000 0xa9c>, <0x458040 0x24>;
> 
> Why not simply drop the second register range here, and the minimal top-ctrl
> poking you need in the phy driver's phy_init function ?

I agree it's a little ugly, but your recommended solution will not work.

The 'top-ctrl' register range includes several SATA functionalities,
some of which are required for the PHY and some of which are definitely
required for the SATA driver. We have:

0x00   VERSION
0x04   BUS_CTRL
0x08   TP_CTRL
0x0C   PHY_CTRL_1
0x10   PHY_CTRL_2
0x14   PHY_CTRL_3
0x18   PHY_CTRL_4
0x1C   TP_OUT
0x20   CLIENT_INIT_CTRL

We *definitely* need the BUS_CTRL register in the SATA driver, since it
controls the endianness of the AHCI register set as well as a few other
important bits we may use in the future. So we can't just "drop" the
"minimal poking" and expect things to work, just because that makes the
device tree look nicer :)

The problem is what to do with the PHY_CTRL registers that are kept in
the middle of the block. These really belong as part of the PHY
power-on/power-off control sequence (i.e., PHY driver).

Do you have any better suggestions that don't involve dropping the
BUS_CTRL register from the SATA driver?

> This avoids the weird / ugly register overlap with the phy driver, and I think you
> can then just use the ahci_platform driver unmodified.
> 
> >+			interrupts = <GIC_SPI 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>;
> >+			};
> >+		};
> >+
> >+		sata_phy: sata-phy@f0458100 {
> >+			compatible = "brcm,bcm7445-sata-phy", "brcm,phy-sata3";
> >+			reg = <0x458100 0x1e00>, <0x45804c 0x10>;
> 
> Why not simply use: reg = <0x458000 0x2000>, to me it seems that what you should
> really be using here.

0x458000 to 0x45800f and 0x458080 to 0x4580af belong to a debug
interrupt controller, which handles bus error handling. It's currently
unused, and it definitely doesn't belong here.

The 'phy' register range is actually documented as two sets of identical
registers:

0x458100 - 0x458fff  Port0 SATA PHY registers
0x459100 - 0x459fff  Port1 SATA PHY registers

with a hole between them. I definitely don't want to do the combining
that you suggested, but I could probably split them apart if that really
helps...

(BTW, I realized I have my math wrong, and each port has length 0xf00,
not 0xe00. So the range I posted should actually be <0x458100 0x1f00>.)

> >+			reg-names = "phy", "port-ctrl";
> >+			#phy-cells = <1>;
> >+			#address-cells = <0x1>;
> >+			#size-cells = <0x0>;
> >+
> >+			sata-phy@0 {
> >+				reg = <0>;
> >+			};
> >+
> >+			sata-phy@1 {
> >+				reg = <1>;
> >+			};
> >+		};
> >  	};
> >
> >  	smpboot {
> >

Brian

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

[Hans de Goede]

Hi,

On 19-03-15 16:53, Brian Norris wrote:
> Hi Hans,
>
> Thanks for the review.
>
> On Thu, Mar 19, 2015 at 12:10:25PM +0100, Hans de Goede wrote:
>> On 19-03-15 02:23, Brian Norris wrote:
>>> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
>>> ---
>>> Light dependency on:
>>>    http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/331921.html
>>> for the surrounding text.
>>>
>>>   arch/arm/boot/dts/bcm7445.dtsi | 36 ++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 36 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/bcm7445.dtsi b/arch/arm/boot/dts/bcm7445.dtsi
>>> index 9eaeac8dce1b..7a7c4d8c2afe 100644
>>> --- a/arch/arm/boot/dts/bcm7445.dtsi
>>> +++ b/arch/arm/boot/dts/bcm7445.dtsi
>>> @@ -108,6 +108,42 @@
>>>   			brcm,int-map-mask = <0x25c>, <0x7000000>;
>>>   			brcm,int-fwd-mask = <0x70000>;
>>>   		};
>>> +
>>> +		sata@f045a000 {
>>> +			compatible = "brcm,bcm7445-ahci", "brcm,sata3-ahci";
>>> +			reg-names = "ahci", "top-ctrl";
>>> +			reg = <0x45a000 0xa9c>, <0x458040 0x24>;
>>
>> Why not simply drop the second register range here, and the minimal top-ctrl
>> poking you need in the phy driver's phy_init function ?
>
> I agree it's a little ugly, but your recommended solution will not work.
>
> The 'top-ctrl' register range includes several SATA functionalities,
> some of which are required for the PHY and some of which are definitely
> required for the SATA driver.

I see, but the phy driver is required for the SATA driver anyways,
and since the BUS_CTRL setting seems to be static it might just as
well be set by the phy driver. The phy driver also poking some
common sata glue bits like this busctrl register is not unheard of,
esp. when these glue bits are in the phy register range.

> We have:
>
> 0x00   VERSION
> 0x04   BUS_CTRL
> 0x08   TP_CTRL
> 0x0C   PHY_CTRL_1
> 0x10   PHY_CTRL_2
> 0x14   PHY_CTRL_3
> 0x18   PHY_CTRL_4
> 0x1C   TP_OUT
> 0x20   CLIENT_INIT_CTRL
>
> We *definitely* need the BUS_CTRL register in the SATA driver, since it
> controls the endianness of the AHCI register set as well as a few other
> important bits we may use in the future.

Are these bits non static, e.g. something which you may want to change at
another time then init/shutdown/suspend/resume ? If they are static I still
think this all clearly belongs in the phy driver, since this looks a lot
like it is a single hardware block.

If otoh these other bits may need runtime poking for e.g. sata error recovery,
then I can understand why you want the bus ctrl register in the sata driver,
but in that case it should only be mapped by the sata driver, and the phy driver
register ranges should not cover it.

> So we can't just "drop" the
> "minimal poking" and expect things to work, just because that makes the
> device tree look nicer :)

I was not suggesting to drop it I was suggesting moving the poke to phy_init,
and given the registermap you've shown above I still think that this belongs
more in the phy driver then anywhere else.

> The problem is what to do with the PHY_CTRL registers that are kept in
> the middle of the block. These really belong as part of the PHY
> power-on/power-off control sequence (i.e., PHY driver).
>
> Do you have any better suggestions that don't involve dropping the
> BUS_CTRL register from the SATA driver?

Nope, if you really believe that the bus-ctrl register should be part of
the sata driver, then please just split the register ranges at a per register
level, to remove the overlap + the hack of not claiming the resource on one side.

>> This avoids the weird / ugly register overlap with the phy driver, and I think you
>> can then just use the ahci_platform driver unmodified.
>>
>>> +			interrupts = <GIC_SPI 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>;
>>> +			};
>>> +		};
>>> +
>>> +		sata_phy: sata-phy@f0458100 {
>>> +			compatible = "brcm,bcm7445-sata-phy", "brcm,phy-sata3";
>>> +			reg = <0x458100 0x1e00>, <0x45804c 0x10>;
>>
>> Why not simply use: reg = <0x458000 0x2000>, to me it seems that what you should
>> really be using here.
>
> 0x458000 to 0x45800f and 0x458080 to 0x4580af belong to a debug
> interrupt controller, which handles bus error handling. It's currently
> unused, and it definitely doesn't belong here.
>
> The 'phy' register range is actually documented as two sets of identical
> registers:
>
> 0x458100 - 0x458fff  Port0 SATA PHY registers
> 0x459100 - 0x459fff  Port1 SATA PHY registers
>
> with a hole between them. I definitely don't want to do the combining
> that you suggested, but I could probably split them apart if that really
> helps...

If this is the documented register range, then I think that splitting them makes
sense, and assuming that the init code for both ports is the same it may
even make for cleaner code.

Regards,

Hans

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

[Brian Norris]

On Thu, Mar 19, 2015 at 06:02:16PM +0100, Hans de Goede wrote:
> On 19-03-15 16:53, Brian Norris wrote:
> >On Thu, Mar 19, 2015 at 12:10:25PM +0100, Hans de Goede wrote:
> >>On 19-03-15 02:23, Brian Norris wrote:
> >>>Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> >>>---
> >>>Light dependency on:
> >>>   http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/331921.html
> >>>for the surrounding text.
> >>>
> >>>  arch/arm/boot/dts/bcm7445.dtsi | 36 ++++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 36 insertions(+)
> >>>
> >>>diff --git a/arch/arm/boot/dts/bcm7445.dtsi b/arch/arm/boot/dts/bcm7445.dtsi
> >>>index 9eaeac8dce1b..7a7c4d8c2afe 100644
> >>>--- a/arch/arm/boot/dts/bcm7445.dtsi
> >>>+++ b/arch/arm/boot/dts/bcm7445.dtsi
> >>>@@ -108,6 +108,42 @@
> >>>  			brcm,int-map-mask = <0x25c>, <0x7000000>;
> >>>  			brcm,int-fwd-mask = <0x70000>;
> >>>  		};
> >>>+
> >>>+		sata@f045a000 {
> >>>+			compatible = "brcm,bcm7445-ahci", "brcm,sata3-ahci";
> >>>+			reg-names = "ahci", "top-ctrl";
> >>>+			reg = <0x45a000 0xa9c>, <0x458040 0x24>;
> >>
> >>Why not simply drop the second register range here, and the minimal top-ctrl
> >>poking you need in the phy driver's phy_init function ?
> >
> >I agree it's a little ugly, but your recommended solution will not work.
> >
> >The 'top-ctrl' register range includes several SATA functionalities,
> >some of which are required for the PHY and some of which are definitely
> >required for the SATA driver.
> 
> I see, but the phy driver is required for the SATA driver anyways,
> and since the BUS_CTRL setting seems to be static it might just as
> well be set by the phy driver. The phy driver also poking some
> common sata glue bits like this busctrl register is not unheard of,
> esp. when these glue bits are in the phy register range.
> 
> >We have:
> >
> >0x00   VERSION
> >0x04   BUS_CTRL
> >0x08   TP_CTRL
> >0x0C   PHY_CTRL_1
> >0x10   PHY_CTRL_2
> >0x14   PHY_CTRL_3
> >0x18   PHY_CTRL_4
> >0x1C   TP_OUT
> >0x20   CLIENT_INIT_CTRL
> >
> >We *definitely* need the BUS_CTRL register in the SATA driver, since it
> >controls the endianness of the AHCI register set as well as a few other
> >important bits we may use in the future.
> 
> Are these bits non static, e.g. something which you may want to change at
> another time then init/shutdown/suspend/resume ? If they are static I still
> think this all clearly belongs in the phy driver, since this looks a lot
> like it is a single hardware block.

They are mostly static. I can't see right now anything (outside of the
PHY ctrl registers) that might ever need handled dynamically.

> If otoh these other bits may need runtime poking for e.g. sata error recovery,
> then I can understand why you want the bus ctrl register in the sata driver,
> but in that case it should only be mapped by the sata driver, and the phy driver
> register ranges should not cover it.
> 
> >So we can't just "drop" the
> >"minimal poking" and expect things to work, just because that makes the
> >device tree look nicer :)
> 
> I was not suggesting to drop it I was suggesting moving the poke to phy_init,
> and given the registermap you've shown above I still think that this belongs
> more in the phy driver then anywhere else.

Implicit in my statements so far was the assumption that the AHCI
platform driver would actually use info from the AHCI registers before
bringing up the PHY. For instance, my driver used to do something like
this:

0. configure AHCI register endianness
1. check AHCI HOST_PORTS_IMPL
2. enable phy(s) for port(s) that are available

(This would automatically account for cases where one or more port(s) are
OTP'd out.)

Such a sequence would require that the AHCI registers are prepared for
use before the PHY driver ever comes into play. But I now see that such
a sequence is not done, and that your suggestion would actually work.
The OTP cases can be handled by disabling the PHY subnodes in the device
tree.

BTW, BUS_CTRL also contains a bit for allowing re-configuration of the
AHCI CAPS register, and we'll probably need to use this soon. I see that
this would probably actually be a PHY-dependent operation, since we will
have some SoCs where features are buggy and should be disabled, and
conversely other SoCs where new features are disabled by default, but
can be enabled after silicon verification confirms they are "good."
(Particularly: AHCI link power management.) I'm not sure whether these
would be properties of the SATA driver or of the PHY driver.

All in all, I think I'm leaning toward your suggestion of moving this
all to the phy driver, and just using 'generic-ahci' directly. I would,
of course, still like to define a 'brcm,bcm7445-ahci' compatible string,
in case we need to differentiate from other 'generic' AHCI eventually.

> >The problem is what to do with the PHY_CTRL registers that are kept in
> >the middle of the block. These really belong as part of the PHY
> >power-on/power-off control sequence (i.e., PHY driver).
> >
> >Do you have any better suggestions that don't involve dropping the
> >BUS_CTRL register from the SATA driver?
> 
> Nope, if you really believe that the bus-ctrl register should be part of
> the sata driver, then please just split the register ranges at a per register
> level, to remove the overlap + the hack of not claiming the resource on one side.

I though of that earlier, but that's even uglier IMO :)

Unless my comments earlier in the email change, I think I'll lean toward
your former suggestion, actually.

> >>This avoids the weird / ugly register overlap with the phy driver, and I think you
> >>can then just use the ahci_platform driver unmodified.
> >>
> >>>+			interrupts = <GIC_SPI 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>;
> >>>+			};
> >>>+		};
> >>>+
> >>>+		sata_phy: sata-phy@f0458100 {
> >>>+			compatible = "brcm,bcm7445-sata-phy", "brcm,phy-sata3";
> >>>+			reg = <0x458100 0x1e00>, <0x45804c 0x10>;
> >>
> >>Why not simply use: reg = <0x458000 0x2000>, to me it seems that what you should
> >>really be using here.
> >
> >0x458000 to 0x45800f and 0x458080 to 0x4580af belong to a debug
> >interrupt controller, which handles bus error handling. It's currently
> >unused, and it definitely doesn't belong here.
> >
> >The 'phy' register range is actually documented as two sets of identical
> >registers:
> >
> >0x458100 - 0x458fff  Port0 SATA PHY registers
> >0x459100 - 0x459fff  Port1 SATA PHY registers
> >
> >with a hole between them. I definitely don't want to do the combining
> >that you suggested, but I could probably split them apart if that really
> >helps...
> 
> If this is the documented register range, then I think that splitting them makes
> sense, and assuming that the init code for both ports is the same it may
> even make for cleaner code.

Eh, it doesn't do too much for the code. We'll just now have to grab two
different resources "by name", and then do the register reads/writes by
pulling from two different __iomem regions, instead of a single one plus
an offset of (0x1000 * portnum). Not much difference.

But yes, I can split the regions if that helps make the description
better (TM).

Brian

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

[Brian Norris]

Replying to myself, because I may or may not like having conversations
with myself :)

On Thu, Mar 19, 2015 at 10:36:40AM -0700, Brian Norris wrote:
> On Thu, Mar 19, 2015 at 06:02:16PM +0100, Hans de Goede wrote:
> > On 19-03-15 16:53, Brian Norris wrote:
> > >On Thu, Mar 19, 2015 at 12:10:25PM +0100, Hans de Goede wrote:
> > >>On 19-03-15 02:23, Brian Norris wrote:
> > >>>Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > >>>---
> > >>>Light dependency on:
> > >>>   http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/331921.html
> > >>>for the surrounding text.
> > >>>
> > >>>  arch/arm/boot/dts/bcm7445.dtsi | 36 ++++++++++++++++++++++++++++++++++++
> > >>>  1 file changed, 36 insertions(+)
> > >>>
> > >>>diff --git a/arch/arm/boot/dts/bcm7445.dtsi b/arch/arm/boot/dts/bcm7445.dtsi
> > >>>index 9eaeac8dce1b..7a7c4d8c2afe 100644
> > >>>--- a/arch/arm/boot/dts/bcm7445.dtsi
> > >>>+++ b/arch/arm/boot/dts/bcm7445.dtsi
> > >>>@@ -108,6 +108,42 @@
> > >>>  			brcm,int-map-mask = <0x25c>, <0x7000000>;
> > >>>  			brcm,int-fwd-mask = <0x70000>;
> > >>>  		};
> > >>>+
> > >>>+		sata@f045a000 {
> > >>>+			compatible = "brcm,bcm7445-ahci", "brcm,sata3-ahci";
> > >>>+			reg-names = "ahci", "top-ctrl";
> > >>>+			reg = <0x45a000 0xa9c>, <0x458040 0x24>;
> > >>
> > >>Why not simply drop the second register range here, and the minimal top-ctrl
> > >>poking you need in the phy driver's phy_init function ?
> > >
> > >I agree it's a little ugly, but your recommended solution will not work.
> > >
> > >The 'top-ctrl' register range includes several SATA functionalities,
> > >some of which are required for the PHY and some of which are definitely
> > >required for the SATA driver.
> > 
> > I see, but the phy driver is required for the SATA driver anyways,
> > and since the BUS_CTRL setting seems to be static it might just as
> > well be set by the phy driver. The phy driver also poking some
> > common sata glue bits like this busctrl register is not unheard of,
> > esp. when these glue bits are in the phy register range.

I realized I *do* still have some reservations about moving the
SATA_TOP_CTRL register range under the PHY DT binding; it's because all
arguments for it seem to rest on descriptions of how the software would
*like* to handle it. It's not at all about describing the hardware
correctly.

I still see SATA_TOP_CTRL as a register resource that belongs to the
SATA controller, not to the PHY. It just happens that it has a few
registers in it that are also for use in the PHY.

So, to best describe the *hardware*, it seems we might split top-ctrl
into 3 portions, where the middle gets assigned to a phy description,
and the first and last belong to the SATA controller description.

But to most easily describe how *software* would best handle them, we
might stick all the custom stuff (i.e., all of top-ctrl + phy) into the
PHY description.

I still think that, practically speaking, the latter should work just
fine, and it's only a theoretical concern that suggests the former.

Thoughts?

> > >We have:
> > >
> > >0x00   VERSION
> > >0x04   BUS_CTRL
> > >0x08   TP_CTRL
> > >0x0C   PHY_CTRL_1
> > >0x10   PHY_CTRL_2
> > >0x14   PHY_CTRL_3
> > >0x18   PHY_CTRL_4
> > >0x1C   TP_OUT
> > >0x20   CLIENT_INIT_CTRL

[snip rest]

Brian

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

[Hans de Goede]

Hi,

On 19-03-15 20:11, Brian Norris wrote:
> Replying to myself, because I may or may not like having conversations
> with myself :)
>
> On Thu, Mar 19, 2015 at 10:36:40AM -0700, Brian Norris wrote:
>> On Thu, Mar 19, 2015 at 06:02:16PM +0100, Hans de Goede wrote:
>>> On 19-03-15 16:53, Brian Norris wrote:
>>>> On Thu, Mar 19, 2015 at 12:10:25PM +0100, Hans de Goede wrote:
>>>>> On 19-03-15 02:23, Brian Norris wrote:
>>>>>> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
>>>>>> ---
>>>>>> Light dependency on:
>>>>>>    http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/331921.html
>>>>>> for the surrounding text.
>>>>>>
>>>>>>   arch/arm/boot/dts/bcm7445.dtsi | 36 ++++++++++++++++++++++++++++++++++++
>>>>>>   1 file changed, 36 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm/boot/dts/bcm7445.dtsi b/arch/arm/boot/dts/bcm7445.dtsi
>>>>>> index 9eaeac8dce1b..7a7c4d8c2afe 100644
>>>>>> --- a/arch/arm/boot/dts/bcm7445.dtsi
>>>>>> +++ b/arch/arm/boot/dts/bcm7445.dtsi
>>>>>> @@ -108,6 +108,42 @@
>>>>>>   			brcm,int-map-mask = <0x25c>, <0x7000000>;
>>>>>>   			brcm,int-fwd-mask = <0x70000>;
>>>>>>   		};
>>>>>> +
>>>>>> +		sata@f045a000 {
>>>>>> +			compatible = "brcm,bcm7445-ahci", "brcm,sata3-ahci";
>>>>>> +			reg-names = "ahci", "top-ctrl";
>>>>>> +			reg = <0x45a000 0xa9c>, <0x458040 0x24>;
>>>>>
>>>>> Why not simply drop the second register range here, and the minimal top-ctrl
>>>>> poking you need in the phy driver's phy_init function ?
>>>>
>>>> I agree it's a little ugly, but your recommended solution will not work.
>>>>
>>>> The 'top-ctrl' register range includes several SATA functionalities,
>>>> some of which are required for the PHY and some of which are definitely
>>>> required for the SATA driver.
>>>
>>> I see, but the phy driver is required for the SATA driver anyways,
>>> and since the BUS_CTRL setting seems to be static it might just as
>>> well be set by the phy driver. The phy driver also poking some
>>> common sata glue bits like this busctrl register is not unheard of,
>>> esp. when these glue bits are in the phy register range.
>
> I realized I *do* still have some reservations about moving the
> SATA_TOP_CTRL register range under the PHY DT binding; it's because all
> arguments for it seem to rest on descriptions of how the software would
> *like* to handle it. It's not at all about describing the hardware
> correctly.

I had the same doubts myself when making the suggestion actually :)

If the busctrl register purely influences the ahci functional block and
not the phy functional block, then you are right.

However if you look at the registermap, then doing as I suggest
feels more natural as you get 2 distinct register blocks, one for ahci
one for the phy, but if the one register in the phy range actually is a
ahci register, then it would probably be more accurate to describe
things that way ...

> I still see SATA_TOP_CTRL as a register resource that belongs to the
> SATA controller, not to the PHY. It just happens that it has a few
> registers in it that are also for use in the PHY.
>
> So, to best describe the *hardware*, it seems we might split top-ctrl
> into 3 portions, where the middle gets assigned to a phy description,
> and the first and last belong to the SATA controller description.
>
> But to most easily describe how *software* would best handle them, we
> might stick all the custom stuff (i.e., all of top-ctrl + phy) into the
> PHY description.
>
> I still think that, practically speaking, the latter should work just
> fine, and it's only a theoretical concern that suggests the former.
>
> Thoughts?

I do not like your original proposal with the overlapping / conflicting
resources. I'm fine with either alternative you suggest above. So unless
someone else weighs in you get to chose which one you like best.

Regards,

Hans

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

[Sergei Shtylyov]

Hello.

On 3/19/2015 4:23 AM, Brian Norris wrote:

> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
> Light dependency on:
>    http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/331921.html
> for the surrounding text.

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

> diff --git a/arch/arm/boot/dts/bcm7445.dtsi b/arch/arm/boot/dts/bcm7445.dtsi
> index 9eaeac8dce1b..7a7c4d8c2afe 100644
> --- a/arch/arm/boot/dts/bcm7445.dtsi
> +++ b/arch/arm/boot/dts/bcm7445.dtsi
> @@ -108,6 +108,42 @@
>   			brcm,int-map-mask = <0x25c>, <0x7000000>;
>   			brcm,int-fwd-mask = <0x70000>;
>   		};
> +
> +		sata@f045a000 {

    Hm, why the <unit-address> part of the name doesn't correspond to the 
"reg" prop?

> +			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>;
[...]
> +		sata_phy: sata-phy@f0458100 {

    Same question here...

> +			compatible = "brcm,bcm7445-sata-phy", "brcm,phy-sata3";
> +			reg = <0x458100 0x1e00>, <0x45804c 0x10>;
[...]

WBR, Sergei

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

[Brian Norris]

Hi Sergei,

On Thu, Mar 19, 2015 at 02:33:32PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 3/19/2015 4:23 AM, Brian Norris wrote:
> 
> >Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> >---
> >Light dependency on:
> >   http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/331921.html
> >for the surrounding text.
> 
> >  arch/arm/boot/dts/bcm7445.dtsi | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> 
> >diff --git a/arch/arm/boot/dts/bcm7445.dtsi b/arch/arm/boot/dts/bcm7445.dtsi
> >index 9eaeac8dce1b..7a7c4d8c2afe 100644
> >--- a/arch/arm/boot/dts/bcm7445.dtsi
> >+++ b/arch/arm/boot/dts/bcm7445.dtsi
> >@@ -108,6 +108,42 @@
> >  			brcm,int-map-mask = <0x25c>, <0x7000000>;
> >  			brcm,int-fwd-mask = <0x70000>;
> >  		};
> >+
> >+		sata@f045a000 {
> 
>    Hm, why the <unit-address> part of the name doesn't correspond to the
> "reg" prop?

Whoops. This .dtsi file is confusing, since none of the Broadcom DTS's I deal
with have the 'ranges' property building in an 0xf0000000 offset in the
/rdb node. All our DTS's give absolute register addresses, not relative.
This discrepancy is a copy-and-paste where I fixed the functional aspect
(the 'reg' property) but overlooked the cosmetic (the name /
unit-address).

We considered just reworking the /rdb node so that it drops the
0xf0000000 offset, in order to be more consistent and to avoid
"mistakes" like this. I could either do that (and use 0xf045a000 in both
places) or just fix the unit address you pointed out.

> >+			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>;
> [...]
> >+		sata_phy: sata-phy@f0458100 {
> 
>    Same question here...

Same answer :)

> >+			compatible = "brcm,bcm7445-sata-phy", "brcm,phy-sata3";
> >+			reg = <0x458100 0x1e00>, <0x45804c 0x10>;
> [...]

Thanks for the review.

Brian