Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1542082418-16675-1-git-send-email-srinath.mannam@broadcom.com>
 <1542082418-16675-3-git-send-email-srinath.mannam@broadcom.com>
 <b626bff7-9228-b53f-b16a-7f6fb1f27100@ti.com> <CABe79T4r-20DmQoHX5HXFU0O7KXid=JuhKVtA7+yzkrb4vafkA@mail.gmail.com>
 <46144237-8069-2cf5-a298-14a7ff0bd634@ti.com>
In-Reply-To: <46144237-8069-2cf5-a298-14a7ff0bd634@ti.com>
From:   Srinath Mannam <srinath.mannam@broadcom.com>
Date:   Fri, 30 Nov 2018 11:07:19 +0530
Message-ID: <CABe79T5VpbmgOuOjSceP+W3A028uO+SBD+coHOnSssMKPcJi8w@mail.gmail.com>
Subject: Re: [PATCH 2/3] phy: sr-usb: Add stingray usb phy driver
To:     Kishon Vijay Abraham I <kishon@ti.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>, Tejun Heo <tj@kernel.org>,
        Jayachandran C <jnair@caviumnetworks.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        bcm-kernel-feedback-list@broadcom.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Kishon,

Thank you for more information. Please find my answers below in line.

On Fri, Nov 30, 2018 at 9:56 AM Kishon Vijay Abraham I <kishon@ti.com> wrote:
>
> Hi,
>
> On 29/11/18 6:31 PM, Srinath Mannam wrote:
> > Hi Kishon,
> >
> > Thank you for review.. Please find my answers below in line.
> >
> >
> > On Thu, Nov 29, 2018 at 3:55 PM Kishon Vijay Abraham I <kishon@ti.com> wrote:
> >>
> >> Hi,
> >>
> >> On 13/11/18 9:43 AM, Srinath Mannam wrote:
> >>> This driver supports all versions of stingray SS and HS
> >>> USB phys.
> >>> In version 1 is combo phy contain both SS and HS phys
> >>> in a common IO space.
> >>> In version 2 a single HS phy.
> >>> These phys support both xHCI host driver and
> >>> BDC Broadcom device controller driver.
> >>>
> >>> Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
> >>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> >>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> >>> ---
> >>>  drivers/phy/broadcom/Kconfig          |  11 +
> >>>  drivers/phy/broadcom/Makefile         |   1 +
> >>>  drivers/phy/broadcom/phy-bcm-sr-usb.c | 367 ++++++++++++++++++++++++++++++++++
> >>>  3 files changed, 379 insertions(+)
> >>>  create mode 100644 drivers/phy/broadcom/phy-bcm-sr-usb.c
> >>>
> >>> diff --git a/drivers/phy/broadcom/Kconfig b/drivers/phy/broadcom/Kconfig
> >>> index 8786a96..c1e4dd5 100644
> >>> --- a/drivers/phy/broadcom/Kconfig
> >>> +++ b/drivers/phy/broadcom/Kconfig
> >>> @@ -10,6 +10,17 @@ config PHY_CYGNUS_PCIE
> >>>         Enable this to support the Broadcom Cygnus PCIe PHY.
> >>>         If unsure, say N.
> >>>
> >>> +config PHY_BCM_SR_USB
> >>> +     tristate "Broadcom Stingray USB PHY driver"
> >>> +     depends on OF && (ARCH_BCM_IPROC || COMPILE_TEST)
> >>> +     select GENERIC_PHY
> >>> +     default ARCH_BCM_IPROC
> >>> +     help
> >>> +       Enable this to support the Broadcom Stingray USB PHY
> >>> +       driver. It supports all versions of Superspeed and
> >>> +       Highspeed PHYs.
> >>> +       If unsure, say N.
> >>> +
> >>>  config BCM_KONA_USB2_PHY
> >>>       tristate "Broadcom Kona USB2 PHY Driver"
> >>>       depends on HAS_IOMEM
> >>> diff --git a/drivers/phy/broadcom/Makefile b/drivers/phy/broadcom/Makefile
> >>> index 0f60184..f453c7d 100644
> >>> --- a/drivers/phy/broadcom/Makefile
> >>> +++ b/drivers/phy/broadcom/Makefile
> >>> @@ -11,3 +11,4 @@ obj-$(CONFIG_PHY_BRCM_USB)          += phy-brcm-usb-dvr.o
> >>>  phy-brcm-usb-dvr-objs := phy-brcm-usb.o phy-brcm-usb-init.o
> >>>
> >>>  obj-$(CONFIG_PHY_BCM_SR_PCIE)                += phy-bcm-sr-pcie.o
> >>> +obj-$(CONFIG_PHY_BCM_SR_USB)         += phy-bcm-sr-usb.o
> >>> diff --git a/drivers/phy/broadcom/phy-bcm-sr-usb.c b/drivers/phy/broadcom/phy-bcm-sr-usb.c
> >>> new file mode 100644
> >>> index 0000000..99de49f
> >>> --- /dev/null
> >>> +++ b/drivers/phy/broadcom/phy-bcm-sr-usb.c
> >>> @@ -0,0 +1,367 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +/*
> >>> + * Copyright (C) 2016-2018 Broadcom
> >>> + */
> >>> +
> >>> +#include <linux/delay.h>
> >>> +#include <linux/io.h>
> >>> +#include <linux/module.h>
> >>> +#include <linux/of.h>
> >>> +#include <linux/phy/phy.h>
> >>> +#include <linux/platform_device.h>
> >>> +
> >>> +enum bcm_usb_phy_version {
> >>> +     BCM_USB_PHY_V1,
> >>> +     BCM_USB_PHY_V2,
> >>> +};
> >>> +
> >>> +enum bcm_usb_phy_reg {
> >>> +     PLL_NDIV_FRAC,
> >>> +     PLL_NDIV_INT,
> >>> +     PLL_CTRL,
> >>> +     PHY_CTRL,
> >>> +     PHY_PLL_CTRL,
> >>> +};
> >>> +
> >>> +/* USB PHY registers */
> >>> +
> >>> +static const u8 bcm_usb_u3phy_v1[] = {
> >>> +     [PLL_CTRL]              = 0x18,
> >>> +     [PHY_CTRL]              = 0x14,
> >>> +};
> >>> +
> >>> +static const u8 bcm_usb_u2phy_v1[] = {
> >>> +     [PLL_NDIV_FRAC] = 0x04,
> >>> +     [PLL_NDIV_INT]  = 0x08,
> >>> +     [PLL_CTRL]      = 0x0c,
> >>> +     [PHY_CTRL]      = 0x10,
> >>> +};
> >>> +
> >>> +#define HSPLL_NDIV_INT_VAL   0x13
> >>> +#define HSPLL_NDIV_FRAC_VAL  0x1005
> >>> +
> >>> +static const u8 bcm_usb_u2phy_v2[] = {
> >>> +     [PLL_NDIV_FRAC] = 0x0,
> >>> +     [PLL_NDIV_INT]  = 0x4,
> >>> +     [PLL_CTRL]      = 0x8,
> >>> +     [PHY_CTRL]      = 0xc,
> >>> +};
> >>> +
> >>> +enum pll_ctrl_bits {
> >>> +     PLL_RESETB,
> >>> +     SSPLL_SUSPEND_EN,
> >>> +     PLL_SEQ_START,
> >>> +     PLL_LOCK,
> >>> +     PLL_PDIV,
> >>> +};
> >>> +
> >>> +static const u8 u3pll_ctrl[] = {
> >>> +     [PLL_RESETB]            = 0,
> >>> +     [SSPLL_SUSPEND_EN]      = 1,
> >>> +     [PLL_SEQ_START]         = 2,
> >>> +     [PLL_LOCK]              = 3,
> >>> +};
> >>> +
> >>> +#define HSPLL_PDIV_MASK              0xF
> >>> +#define HSPLL_PDIV_VAL               0x1
> >>> +
> >>> +static const u8 u2pll_ctrl[] = {
> >>> +     [PLL_PDIV]      = 1,
> >>> +     [PLL_RESETB]    = 5,
> >>> +     [PLL_LOCK]      = 6,
> >>> +};
> >>> +
> >>> +enum bcm_usb_phy_ctrl_bits {
> >>> +     CORERDY,
> >>> +     AFE_LDO_PWRDWNB,
> >>> +     AFE_PLL_PWRDWNB,
> >>> +     AFE_BG_PWRDWNB,
> >>> +     PHY_ISO,
> >>> +     PHY_RESETB,
> >>> +     PHY_PCTL,
> >>> +};
> >>> +
> >>> +#define PHY_PCTL_MASK        0xffff
> >>> +/*
> >>> + * 0x0806 of PCTL_VAL has below bits set
> >>> + * BIT-8 : refclk divider 1
> >>> + * BIT-3:2: device mode; mode is not effect
> >>> + * BIT-1: soft reset active low
> >>> + */
> >>> +#define HSPHY_PCTL_VAL       0x0806
> >>> +#define SSPHY_PCTL_VAL       0x0006
> >>> +
> >>> +static const u8 u3phy_ctrl[] = {
> >>> +     [PHY_RESETB]    = 1,
> >>> +     [PHY_PCTL]      = 2,
> >>> +};
> >>> +
> >>> +static const u8 u2phy_ctrl[] = {
> >>> +     [CORERDY]               = 0,
> >>> +     [AFE_LDO_PWRDWNB]       = 1,
> >>> +     [AFE_PLL_PWRDWNB]       = 2,
> >>> +     [AFE_BG_PWRDWNB]        = 3,
> >>> +     [PHY_ISO]               = 4,
> >>> +     [PHY_RESETB]            = 5,
> >>> +     [PHY_PCTL]              = 6,
> >>> +};
> >>> +
> >>> +struct bcm_usb_phy_cfg {
> >>> +     uint32_t type;
> >>> +     uint32_t ver;
> >>> +     void __iomem *regs;
> >>> +     struct phy *phy;
> >>> +     const u8 *offset;
> >>> +};
> >>> +
> >>> +#define PLL_LOCK_RETRY_COUNT 1000
> >>> +
> >>> +enum bcm_usb_phy_type {
> >>> +     USB_HS_PHY,
> >>> +     USB_SS_PHY,
> >>> +};
> >>> +
> >>> +static inline void bcm_usb_reg32_clrbits(void __iomem *addr, uint32_t clear)
> >>> +{
> >>> +     writel(readl(addr) & ~clear, addr);
> >>> +}
> >>> +
> >>> +static inline void bcm_usb_reg32_setbits(void __iomem *addr, uint32_t set)
> >>> +{
> >>> +     writel(readl(addr) | set, addr);
> >>> +}
> >>> +
> >>> +static int bcm_usb_pll_lock_check(void __iomem *addr, u32 bit)
> >>> +{
> >>> +     int retry;
> >>> +     u32 rd_data;
> >>> +
> >>> +     retry = PLL_LOCK_RETRY_COUNT;
> >>> +     do {
> >>> +             rd_data = readl(addr);
> >>> +             if (rd_data & bit)
> >>> +                     return 0;
> >>> +             udelay(1);
> >>> +     } while (--retry > 0);
> >>> +
> >>> +     pr_err("%s: FAIL\n", __func__);
> >>> +     return -ETIMEDOUT;
> >>> +}
> >>> +
> >>> +static int bcm_usb_ss_phy_init(struct bcm_usb_phy_cfg *phy_cfg)
> >>> +{
> >>> +     int ret = 0;
> >>> +     void __iomem *regs = phy_cfg->regs;
> >>> +     const u8 *offset;
> >>> +     u32 rd_data;
> >>> +
> >>> +     offset = phy_cfg->offset;
> >>> +
> >>> +     /* Set pctl with mode and soft reset */
> >>> +     rd_data = readl(regs + offset[PHY_CTRL]);
> >>> +     rd_data &= ~(PHY_PCTL_MASK << u3phy_ctrl[PHY_PCTL]);
> >>> +     rd_data |= (SSPHY_PCTL_VAL << u3phy_ctrl[PHY_PCTL]);
> >>> +     writel(rd_data, regs + offset[PHY_CTRL]);
> >>> +
> >>> +     bcm_usb_reg32_clrbits(regs + offset[PLL_CTRL],
> >>> +                           BIT(u3pll_ctrl[SSPLL_SUSPEND_EN]));
> >>> +     bcm_usb_reg32_setbits(regs + offset[PLL_CTRL],
> >>> +                           BIT(u3pll_ctrl[PLL_SEQ_START]));
> >>> +     bcm_usb_reg32_setbits(regs + offset[PLL_CTRL],
> >>> +                           BIT(u3pll_ctrl[PLL_RESETB]));
> >>> +
> >>> +     msleep(30);
>
> Please add a comment for each of the delay added to mention if it's documented
> in the data manual or was a result of experiment etc..
I will add comments in next patch version.
> >>> +
> >>> +     ret = bcm_usb_pll_lock_check(regs + offset[PLL_CTRL],
> >>> +                                  BIT(u3pll_ctrl[PLL_LOCK]));
> >>> +
> >>> +     return ret;
> >>> +}
> >>> +
> >>> +static int bcm_usb_hs_phy_init(struct bcm_usb_phy_cfg *phy_cfg)
> >>> +{
> >>> +     int ret = 0;
> >>> +     void __iomem *regs = phy_cfg->regs;
> >>> +     const u8 *offset;
> >>> +     u32 rd_data;
> >>> +
> >>> +     offset = phy_cfg->offset;
> >>> +
> >>> +     writel(HSPLL_NDIV_INT_VAL, regs + offset[PLL_NDIV_INT]);
> >>> +     writel(HSPLL_NDIV_FRAC_VAL, regs + offset[PLL_NDIV_FRAC]);
> >>> +
> >>> +     rd_data = readl(regs + offset[PLL_CTRL]);
> >>> +     rd_data &= ~(HSPLL_PDIV_MASK << u2pll_ctrl[PLL_PDIV]);
> >>> +     rd_data |= (HSPLL_PDIV_VAL << u2pll_ctrl[PLL_PDIV]);
> >>> +     writel(rd_data, regs + offset[PLL_CTRL]);
> >>> +
> >>> +     /* Set Core Ready high */
> >>> +     bcm_usb_reg32_setbits(regs + offset[PHY_CTRL],
> >>> +                           BIT(u2phy_ctrl[CORERDY]));
> >>> +
> >>> +     msleep(100);
> >>> +
> >>> +     bcm_usb_reg32_setbits(regs + offset[PLL_CTRL],
> >>> +                           BIT(u2pll_ctrl[PLL_RESETB]));
> >>> +     bcm_usb_reg32_setbits(regs + offset[PHY_CTRL],
> >>> +                           BIT(u2phy_ctrl[PHY_RESETB]));
> >>> +
> >>> +
> >>> +     rd_data = readl(regs + offset[PHY_CTRL]);
> >>> +     rd_data &= ~(PHY_PCTL_MASK << u2phy_ctrl[PHY_PCTL]);
> >>> +     rd_data |= (HSPHY_PCTL_VAL << u2phy_ctrl[PHY_PCTL]);
> >>> +     writel(rd_data, regs + offset[PHY_CTRL]);
> >>> +
> >>> +     ret = bcm_usb_pll_lock_check(regs + offset[PLL_CTRL],
> >>> +                                  BIT(u2pll_ctrl[PLL_LOCK]));
> >>> +
> >>> +     return ret;
> >>> +}
> >>> +
> >>> +static int bcm_usb_phy_reset(struct phy *phy)
> >>> +{
> >>> +     struct bcm_usb_phy_cfg *phy_cfg = phy_get_drvdata(phy);
> >>> +     void __iomem *regs = phy_cfg->regs;
> >>> +     const u8 *offset;
> >>> +
> >>> +     offset = phy_cfg->offset;
> >>> +
> >>> +     if (phy_cfg->type == USB_HS_PHY) {
> >>> +             bcm_usb_reg32_clrbits(regs + offset[PHY_CTRL],
> >>> +                                   BIT(u2phy_ctrl[CORERDY]));
> >>> +             bcm_usb_reg32_setbits(regs + offset[PHY_CTRL],
> >>> +                                   BIT(u2phy_ctrl[CORERDY]));
> >>> +     }
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +static int bcm_usb_phy_init(struct phy *phy)
> >>> +{
> >>> +     struct bcm_usb_phy_cfg *phy_cfg = phy_get_drvdata(phy);
> >>> +
> >>> +     if (phy_cfg->type == USB_SS_PHY)
> >>> +             bcm_usb_ss_phy_init(phy_cfg);
> >>> +     if (phy_cfg->type == USB_HS_PHY)
> >>> +             bcm_usb_hs_phy_init(phy_cfg);
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +static struct phy_ops sr_phy_ops = {
> >>> +     .init           = bcm_usb_phy_init,
> >>> +     .reset          = bcm_usb_phy_reset,
> >>> +     .owner          = THIS_MODULE,
> >>> +};
> >>> +
> >>> +static int bcm_usb_phy_create(struct device *dev, struct device_node *node,
> >>> +                          void __iomem *regs, uint32_t version)
> >>> +{
> >>> +     struct bcm_usb_phy_cfg *phy_cfg;
> >>> +     struct phy_provider *phy_provider;
> >>> +
> >>> +     phy_cfg = devm_kzalloc(dev, sizeof(struct bcm_usb_phy_cfg), GFP_KERNEL);
> >>> +     if (!phy_cfg)
> >>> +             return -ENOMEM;
> >>> +
> >>> +     phy_cfg->regs = regs;
> >>
> >> Are there any registers shared by all the PHYs?
> > No registers shared between phys. All phys have same type of registers
> > but at different address offset.
> > In the same way few bits in registers are same but at different
> > position in registers.
> >>> +     phy_cfg->ver = version;
> >>> +
> >>> +     if (phy_cfg->ver == BCM_USB_PHY_V1) {
> >>> +             unsigned int id;
> >>> +
> >>> +             if (of_property_read_u32(node, "reg", &id)) {
> >>> +                     dev_err(dev, "missing reg property in node %s\n",
> >>> +                             node->name);
> >>> +                     return -EINVAL;
> >>> +             }
> >>> +
> >>> +             if (id == 0) {
> >>> +                     phy_cfg->offset = bcm_usb_u2phy_v1;
> >>
> >> The reg space can come from the dt itself.
> > This driver supports two different phys. one is combo phy which has
> > both USB3 and USB2 phy.
> > other is single phy it has only USB2 phy.
> > So that, from DT we passed address of either combo phy address or
> > single phy address.
> > In combo phy, USB3 phy and USB2 phy registers are at different offsets
> > from the base address passed through DT.
> >>> +                     phy_cfg->type = USB_HS_PHY;
> >>> +             } else if (id == 1) {
> >>> +                     phy_cfg->offset = bcm_usb_u3phy_v1;
> >>> +                     phy_cfg->type = USB_SS_PHY;
> >>> +             } else {
> >>> +                     return -ENODEV;
> >>> +             }
> >>> +     } else if (phy_cfg->ver == BCM_USB_PHY_V2) {
> >>> +             phy_cfg->offset = bcm_usb_u2phy_v2;
> >>> +             phy_cfg->type = USB_HS_PHY;
> >>> +     }
> >>> +
> >>> +     phy_cfg->phy = devm_phy_create(dev, node, &sr_phy_ops);
> >>> +     if (IS_ERR(phy_cfg->phy))
> >>> +             return PTR_ERR(phy_cfg->phy);
> >>> +
> >>> +     phy_set_drvdata(phy_cfg->phy, phy_cfg);
> >>> +     phy_provider = devm_of_phy_provider_register(&phy_cfg->phy->dev,
> >>> +                                                  of_phy_simple_xlate);
> >>> +     if (IS_ERR(phy_provider)) {
> >>> +             dev_err(dev, "Failed to register phy provider\n");
> >>> +             return PTR_ERR(phy_provider);
> >>> +     }
> >>
> >> No need for a separate PHY provider.
> > Could you please provide more details?
> > I thought to get phy device in the upper layer we need to register
> > provider. can we skip this?
> > In this driver, combo phy provides two usb ports to host controller,
> > so that I need to add separate phandlers
> > in host controller DT node and need to create two separate phy
> > devices. also phy_init and reset handlers need
> > to call for both phy devices separately.
>
> A single phy_provider can create multiple PHY's. Here anyways you have subnodes
> for each of the PHY's. So host controller DT node can have a phandle to the
> subnodes directly.
Yes, phandles of subnodes only given in host controller DT node as
shown in below line.
phys = <&usb0_phy1>, <&usb0_phy0>;
I thought phy device is fetched using of_node passed in host
controller DT by fetching
provider attached to it. dev pointer of provide is dev of phy device.
so that I thought it is better to have separate
provider for each phy device. Please correct me if my understanding is
not right.

Thank you very much.

Regards,
Srinath.
>
> Thanks
> Kishon