| Message ID | 1426728222-8197-4-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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)" ;)
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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");
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