Re: [PATCH V3 RESEND] phy: bcm-ns-usb3: new driver for USB 3.0 PHY on Northstar

[Scott Branden <scott.branden@broadcom.com>]

Hi Rafal,

some comments in line.

On 16-05-22 03:09 PM, Rafał Miłecki wrote:
 > Northstar is a family of SoCs used in home routers. They have USB 2.0
 > and 3.0 controllers with PHYs that need to be properly initialized.
 > This driver provides PHY init support in a generic way and can be bound
 > with XHCI controller driver.
 > This patch adds 2 defines in bcma header that will be reused in bcma
 > driver. Using common header will allow avoiding code duplication.
 >
 > Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
 > ---
 > V2: Redesign the driver to don't depend on bcma. This will allow 
reusing it on
 >      Northstar+ which doesn't use bcma. This requires providing two 
different
 >      registers ranges in DT which was documented in bindings info.
 > V3: Use readl and writel
 >      Fix MODULE_LICENSE to match header (v2)
 >      Mention C0 series in Documentation
 > RESEND: I can see that my PHY driver for USB 2.0:
 >     [PATCH V4] phy: bcm-ns-usb2: new driver for USB 2.0 PHY on Northstar
 >     sent on 14 Apr 2016 was accepted, however:
 >     [PATCH V3] phy: bcm-ns-usb3: new driver for USB 3.0 PHY on Northstar
 >     sent the very same day wasn't.
 >     I'm sending a simply rebased version hoping it can be accepted or
 >     commented somehow (e.g. what needs to be changed).
 > ---
 >   .../devicetree/bindings/phy/bcm-ns-usb3-phy.txt    |  23 ++
 >   drivers/phy/Kconfig                                |   9 +
 >   drivers/phy/Makefile                               |   1 +
 >   drivers/phy/phy-bcm-ns-usb3.c                      | 267 
+++++++++++++++++++++
 >   include/linux/bcma/bcma_driver_chipcommon.h        |   3 +
 >   5 files changed, 303 insertions(+)
 >   create mode 100644 
Documentation/devicetree/bindings/phy/bcm-ns-usb3-phy.txt
 >   create mode 100644 drivers/phy/phy-bcm-ns-usb3.c
 >
 > diff --git 
a/Documentation/devicetree/bindings/phy/bcm-ns-usb3-phy.txt 
b/Documentation/devicetree/bindings/phy/bcm-ns-usb3-phy.txt
 > new file mode 100644
 > index 0000000..09aeba9
 > --- /dev/null
 > +++ b/Documentation/devicetree/bindings/phy/bcm-ns-usb3-phy.txt
 > @@ -0,0 +1,23 @@
 > +Driver for Broadcom Northstar USB 3.0 PHY
 > +
 > +Required properties:
 > +
 > +- compatible: one of: "brcm,ns-ax-usb3-phy", "brcm,ns-bx-usb3-phy".
 > +- reg: register mappings for DMP (Device Management Plugin) and 
ChipCommon B
 > +       MMI.
 > +- reg-names: "dmp" and "ccb-mii"
 > +
 > +Initialization of USB 3.0 PHY depends on Northstar version. There 
are currently
 > +three known series: Ax, Bx and Cx.
 > +Known A0: BCM4707 rev 0
 > +Known B0: BCM4707 rev 4, BCM53573 rev 2
 > +Known B1: BCM4707 rev 6
 > +Known C0: BCM47094 rev 0
 > +
 > +Example:
 > +    usb3-phy {
 > +        compatible = "brcm,ns-ax-usb3-phy";
 > +        reg = <0x18105000 0x1000>, <0x18003000 0x1000>;
 > +        reg-names = "dmp", "ccb-mii";
 > +        #phy-cells = <0>;
 > +    };
 > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
 > index f2b458f..6967398 100644
 > --- a/drivers/phy/Kconfig
 > +++ b/drivers/phy/Kconfig
 > @@ -24,6 +24,15 @@ config PHY_BCM_NS_USB2
 >         Enable this to support Broadcom USB 2.0 PHY connected to the USB
 >         controller on Northstar family.
 >
 > +config PHY_BCM_NS_USB3
 > +    tristate "Broadcom Northstar USB 3.0 PHY Driver"
 > +    depends on ARCH_BCM_IPROC || COMPILE_TEST
 > +    depends on HAS_IOMEM && OF
 > +    select GENERIC_PHY
 > +    help
 > +      Enable this to support Broadcom USB 3.0 PHY connected to the USB
 > +      controller on Northstar family.
 > +
 >   config PHY_BERLIN_USB
 >       tristate "Marvell Berlin USB PHY Driver"
 >       depends on ARCH_BERLIN && RESET_CONTROLLER && HAS_IOMEM && OF
 > diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
 > index 0de09e1..fa17ae3 100644
 > --- a/drivers/phy/Makefile
 > +++ b/drivers/phy/Makefile
 > @@ -4,6 +4,7 @@
 >
 >   obj-$(CONFIG_GENERIC_PHY)        += phy-core.o
 >   obj-$(CONFIG_PHY_BCM_NS_USB2)        += phy-bcm-ns-usb2.o
 > +obj-$(CONFIG_PHY_BCM_NS_USB3)        += phy-bcm-ns-usb3.o
 >   obj-$(CONFIG_PHY_BERLIN_USB)        += phy-berlin-usb.o
 >   obj-$(CONFIG_PHY_BERLIN_SATA)        += phy-berlin-sata.o
 >   obj-$(CONFIG_PHY_DM816X_USB)        += phy-dm816x-usb.o
 > diff --git a/drivers/phy/phy-bcm-ns-usb3.c 
b/drivers/phy/phy-bcm-ns-usb3.c
 > new file mode 100644
 > index 0000000..869bc20
 > --- /dev/null
 > +++ b/drivers/phy/phy-bcm-ns-usb3.c
 > @@ -0,0 +1,267 @@
 > +/*
 > + * Broadcom Northstar USB 3.0 PHY Driver
 > + *
 > + * Copyright (C) 2016 Rafał Miłecki <zajec5@gmail.com>
My thought is this code must have originated from Broadcom source code. 
  Where is the copyright notice/license from the original code?
 > + *
 > + * This program is free software; you can redistribute it and/or modify
 > + * it under the terms of the GNU General Public License version 2 as
 > + * published by the Free Software Foundation.
 > + *
 > + */
 > +
 > +#include <linux/bcma/bcma.h>
 > +#include <linux/delay.h>
 > +#include <linux/err.h>
 > +#include <linux/module.h>
 > +#include <linux/of_platform.h>
 > +#include <linux/platform_device.h>
 > +#include <linux/phy/phy.h>
 > +#include <linux/slab.h>
 > +
 > +#define BCM_NS_USB3_MII_MNG_TIMEOUT_US    1000    /* usecs */
 > +
 > +enum bcm_ns_family {
 > +    BCM_NS_UNKNOWN,
 > +    BCM_NS_AX,
 > +    BCM_NS_BX,
 > +};
 > +
 > +struct bcm_ns_usb3 {
 > +    struct device *dev;
 > +    enum bcm_ns_family family;
 > +    void __iomem *dmp;
 > +    void __iomem *ccb_mii;
 > +    struct phy *phy;
 > +};
 > +
 > +static const struct of_device_id bcm_ns_usb3_id_table[] = {
 > +    {
 > +        .compatible = "brcm,ns-ax-usb3-phy",
 > +        .data = (int *)BCM_NS_AX,
 > +    },
 > +    {
 > +        .compatible = "brcm,ns-bx-usb3-phy",
 > +        .data = (int *)BCM_NS_BX,
 > +    },
 > +    {},
 > +};
 > +MODULE_DEVICE_TABLE(of, bcm_ns_usb3_id_table);
 > +
 > +static int bcm_ns_usb3_wait_reg(struct bcm_ns_usb3 *usb3, void 
__iomem *addr,
 > +                u32 mask, u32 value, unsigned long timeout)
 > +{
 > +    unsigned long deadline = jiffies + timeout;
 > +    u32 val;
 > +
 > +    do {
 > +        val = readl(addr);
 > +        if ((val & mask) == value)
 > +            return 0;
 > +        cpu_relax();
 > +        udelay(10);
 > +    } while (!time_after_eq(jiffies, deadline));
 > +
 > +    dev_err(usb3->dev, "Timeout waiting for register %p\n", addr);
 > +
 > +    return -EBUSY;
 > +}
 > +
 > +static inline int bcm_ns_usb3_mii_mng_wait_idle(struct bcm_ns_usb3 
*usb3)
 > +{
 > +    return bcm_ns_usb3_wait_reg(usb3, usb3->ccb_mii + 
BCMA_CCB_MII_MNG_CTL,
 > +                    0x0100, 0x0000,
 > +                    usecs_to_jiffies(BCM_NS_USB3_MII_MNG_TIMEOUT_US));
 > +}
 > +
 > +static int bcm_ns_usb3_mii_mng_write32(struct bcm_ns_usb3 *usb3, u32 
value)
 > +{
 > +    int err;
 > +
 > +    err = bcm_ns_usb3_mii_mng_wait_idle(usb3);
 > +    if (err < 0) {
 > +        dev_err(usb3->dev, "Couldn't write 0x%08x value\n", value);
 > +        return err;
 > +    }
 > +
 > +    writel(value, usb3->ccb_mii + BCMA_CCB_MII_MNG_CMD_DATA);
 > +
 > +    return 0;
 > +}
 > +
 > +static int bcm_ns_usb3_phy_init_ns_bx(struct bcm_ns_usb3 *usb3)
 > +{
 > +    int err;
 > +
 > +    /* Enable MDIO. Setting MDCDIV as 26  */
 > +    writel(0x0000009a, usb3->ccb_mii + BCMA_CCB_MII_MNG_CTL);
 > +    udelay(2);
why a value of 2 ?

 > +
 > +    /* USB3 PLL Block */
 > +    err = bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8000);
These hard coded numbers should be replaced with appropriate defines.

 > +    if (err < 0)
 > +        return err;
 > +
 > +    /* Assert Ana_Pllseq start */
 > +    bcm_ns_usb3_mii_mng_write32(usb3, 0x58061000);
 > +
 > +    /* Assert CML Divider ratio to 26 */
 > +    bcm_ns_usb3_mii_mng_write32(usb3, 0x582a6400);
 > +
 > +    /* Asserting PLL Reset */
 > +    bcm_ns_usb3_mii_mng_write32(usb3, 0x582ec000);
 > +
 > +    /* Deaaserting PLL Reset */
 > +    bcm_ns_usb3_mii_mng_write32(usb3, 0x582e8000);
 > +
 > +    /* Waiting MII Mgt interface idle */
 > +    bcm_ns_usb3_mii_mng_wait_idle(usb3);
 > +
 > +    /* Deasserting USB3 system reset */
 > +    writel(0, usb3->dmp + BCMA_RESET_CTL);
 > +
 > +    /* PLL frequency monitor enable */
 > +    bcm_ns_usb3_mii_mng_write32(usb3, 0x58069000);
 > +
 > +    /* PIPE Block */
 > +    bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8060);
 > +
 > +    /* CMPMAX & CMPMINTH setting */
 > +    bcm_ns_usb3_mii_mng_write32(usb3, 0x580af30d);
 > +
 > +    /* DEGLITCH MIN & MAX setting */
 > +    bcm_ns_usb3_mii_mng_write32(usb3, 0x580e6302);
 > +
 > +    /* TXPMD block */
 > +    bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8040);
 > +
 > +    /* Enabling SSC */
 > +    bcm_ns_usb3_mii_mng_write32(usb3, 0x58061003);
 > +
 > +    /* Waiting MII Mgt interface idle */
 > +    bcm_ns_usb3_mii_mng_wait_idle(usb3);
 > +
 > +    return 0;
 > +}
 > +
 > +static int bcm_ns_usb3_phy_init_ns_ax(struct bcm_ns_usb3 *usb3)
 > +{
 > +    int err;
 > +
 > +    /* Enable MDIO. Setting MDCDIV as 26  */
 > +    writel(0x0000009a, usb3->ccb_mii + BCMA_CCB_MII_MNG_CTL);
 > +    udelay(2);
 > +
 > +    /* PLL30 block */
 > +    err = bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8000);
 > +    if (err < 0)
 > +        return err;
 > +
 > +    bcm_ns_usb3_mii_mng_write32(usb3, 0x582a6400);
 > +
 > +    bcm_ns_usb3_mii_mng_write32(usb3, 0x587e80e0);
 > +
 > +    bcm_ns_usb3_mii_mng_write32(usb3, 0x580a009c);
 > +
 > +    /* Enable SSC */
 > +    bcm_ns_usb3_mii_mng_write32(usb3, 0x587e8040);
 > +
 > +    bcm_ns_usb3_mii_mng_write32(usb3, 0x580a21d3);
 > +
 > +    bcm_ns_usb3_mii_mng_write32(usb3, 0x58061003);
 > +
 > +    /* Waiting MII Mgt interface idle */
 > +    bcm_ns_usb3_mii_mng_wait_idle(usb3);
 > +
 > +    /* Deasserting USB3 system reset */
 > +    writel(0, usb3->dmp + BCMA_RESET_CTL);
 > +
 > +    return 0;
 > +}
 > +
 > +static int bcm_ns_usb3_phy_init(struct phy *phy)
 > +{
 > +    struct bcm_ns_usb3 *usb3 = phy_get_drvdata(phy);
 > +    int err;
 > +
 > +    /* Perform USB3 system soft reset */
 > +    writel(BCMA_RESET_CTL_RESET, usb3->dmp + BCMA_RESET_CTL);
 > +
 > +    switch (usb3->family) {
 > +    case BCM_NS_AX:
 > +        err = bcm_ns_usb3_phy_init_ns_ax(usb3);
 > +        break;
 > +    case BCM_NS_BX:
 > +        err = bcm_ns_usb3_phy_init_ns_bx(usb3);
 > +        break;
 > +    default:
 > +        WARN_ON(1);
 > +        err = -ENOTSUPP;
 > +    }
 > +
 > +    return err;
 > +}
 > +
 > +static const struct phy_ops ops = {
 > +    .init        = bcm_ns_usb3_phy_init,
 > +    .owner        = THIS_MODULE,
I don't think .owner is needed.
 > +};
 > +
 > +static int bcm_ns_usb3_probe(struct platform_device *pdev)
 > +{
 > +    struct device *dev = &pdev->dev;
 > +    const struct of_device_id *of_id;
 > +    struct bcm_ns_usb3 *usb3;
 > +    struct resource *res;
 > +    struct phy_provider *phy_provider;
 > +
 > +    usb3 = devm_kzalloc(dev, sizeof(*usb3), GFP_KERNEL);
 > +    if (!usb3)
 > +        return -ENOMEM;
 > +
 > +    usb3->dev = dev;
 > +
 > +    of_id = of_match_device(bcm_ns_usb3_id_table, dev);
 > +    if (!of_id)
 > +        return -EINVAL;
 > +    usb3->family = (enum bcm_ns_family)of_id->data;
 > +
 > +    res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dmp");
 > +    usb3->dmp = devm_ioremap_resource(dev, res);
 > +    if (IS_ERR(usb3->dmp)) {
 > +        dev_err(dev, "Failed to map DMP regs\n");
 > +        return PTR_ERR(usb3->dmp);
 > +    }
 > +
 > +    res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ccb-mii");
 > +    usb3->ccb_mii = devm_ioremap_resource(dev, res);
 > +    if (IS_ERR(usb3->ccb_mii)) {
 > +        dev_err(dev, "Failed to map ChipCommon B MII regs\n");
 > +        return PTR_ERR(usb3->ccb_mii);
 > +    }
 > +
 > +    usb3->phy = devm_phy_create(dev, NULL, &ops);
 > +    if (IS_ERR(usb3->phy)) {
 > +        dev_err(dev, "Failed to create PHY\n");
 > +        return PTR_ERR(usb3->phy);
 > +    }
 > +
 > +    phy_set_drvdata(usb3->phy, usb3);
 > +    platform_set_drvdata(pdev, usb3);
 > +
 > +    phy_provider = devm_of_phy_provider_register(dev, 
of_phy_simple_xlate);
 > +    if (!IS_ERR(phy_provider))
 > +        dev_info(dev, "Registered Broadcom Northstar USB 3.0 PHY 
driver\n");
 > +
 > +    return PTR_ERR_OR_ZERO(phy_provider);
 > +}
 > +
 > +static struct platform_driver bcm_ns_usb3_driver = {
 > +    .probe        = bcm_ns_usb3_probe,
 > +    .driver = {
 > +        .name = "bcm_ns_usb3",
 > +        .of_match_table = bcm_ns_usb3_id_table,
 > +    },
 > +};
 > +module_platform_driver(bcm_ns_usb3_driver);
 > +
 > +MODULE_LICENSE("GPL v2");
 > diff --git a/include/linux/bcma/bcma_driver_chipcommon.h 
b/include/linux/bcma/bcma_driver_chipcommon.h
 > index 846513c..3a86e48 100644
 > --- a/include/linux/bcma/bcma_driver_chipcommon.h
 > +++ b/include/linux/bcma/bcma_driver_chipcommon.h
 > @@ -504,6 +504,9 @@
 >   #define BCMA_CC_PMU1_PLL0_PC2_NDIV_INT_MASK    0x1ff00000
 >   #define BCMA_CC_PMU1_PLL0_PC2_NDIV_INT_SHIFT    20
 >
 > +#define BCMA_CCB_MII_MNG_CTL        0x0000
 > +#define BCMA_CCB_MII_MNG_CMD_DATA    0x0004
These defines should not be tied to a bcma header file.  They need to be 
placed in a new header that is not bcma specific.
 > +
 >   /* BCM4331 ChipControl numbers. */
 >   #define BCMA_CHIPCTL_4331_BT_COEXIST        BIT(0)    /* 0 disable */
 >   #define BCMA_CHIPCTL_4331_SECI            BIT(1)    /* 0 SECI is 
disabled (JATG functional) */
 >

Regards,
Scott