Re: [PATCH 2/3] phy: sr-usb: Add stingray usb phy driver
From: Kishon Vijay Abraham I
Date: Thu Nov 29 2018 - 23:27:16 EST
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@xxxxxx> 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@xxxxxxxxxxxx>
>>> Reviewed-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
>>> Reviewed-by: Scott Branden <scott.branden@xxxxxxxxxxxx>
>>> ---
>>> 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