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;
Subject: Re: [PATCH 2/3] phy: sr-usb: Add stingray usb phy driver
To:     Srinath Mannam <srinath.mannam@broadcom.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>
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>
From:   Kishon Vijay Abraham I <kishon@ti.com>
Message-ID: <46144237-8069-2cf5-a298-14a7ff0bd634@ti.com>
Date:   Fri, 30 Nov 2018 09:56:48 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: <CABe79T4r-20DmQoHX5HXFU0O7KXid=JuhKVtA7+yzkrb4vafkA@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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..
>>> +
>>> +     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.

Thanks
Kishon