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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux