Re: [PATCH v1 1/2] phy: usb: Add USB2.0 phy driver for Sunplus SP7021

[施錕鴻 <vincent.sunplus@gmail.com>]

Vinod Koul <vkoul@kernel.org> 於 2022年4月13日 週三 下午5:49寫道：
>
> On 04-03-22, 19:30, Vincent Shih wrote:
> > Add USB2.0 phy driver for Sunplus SP7021
> >
> > Signed-off-by: Vincent Shih <vincent.sunplus@gmail.com>
> > ---
> >  MAINTAINERS                            |   8 ++
> >  drivers/phy/Kconfig                    |   1 +
> >  drivers/phy/Makefile                   |   1 +
> >  drivers/phy/sunplus/Kconfig            |  12 ++
> >  drivers/phy/sunplus/Makefile           |   2 +
> >  drivers/phy/sunplus/phy-sunplus-usb2.c | 248 +++++++++++++++++++++++++++++++++
> >  6 files changed, 272 insertions(+)
> >  create mode 100644 drivers/phy/sunplus/Kconfig
> >  create mode 100644 drivers/phy/sunplus/Makefile
> >  create mode 100644 drivers/phy/sunplus/phy-sunplus-usb2.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 80eebc1..a3bb35e 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -17947,6 +17947,14 @@ L:   netdev@vger.kernel.org
> >  S:   Maintained
> >  F:   drivers/net/ethernet/dlink/sundance.c
> >
> > +SUNPLUS USB2 PHY DRIVER
> > +M:   Vincent Shih <vincent.sunplus@gmail.com>
> > +L:   linux-usb@vger.kernel.org
> > +S:   Maintained
> > +F:   drivers/phy/sunplus/Kconfig
> > +F:   drivers/phy/sunplus/Makefile
> > +F:   drivers/phy/sunplus/phy-sunplus-usb2.c
> > +
> >  SUPERH
> >  M:   Yoshinori Sato <ysato@users.sourceforge.jp>
> >  M:   Rich Felker <dalias@libc.org>
> > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> > index 82b63e6..d97e22e 100644
> > --- a/drivers/phy/Kconfig
> > +++ b/drivers/phy/Kconfig
> > @@ -90,6 +90,7 @@ source "drivers/phy/rockchip/Kconfig"
> >  source "drivers/phy/samsung/Kconfig"
> >  source "drivers/phy/socionext/Kconfig"
> >  source "drivers/phy/st/Kconfig"
> > +source "drivers/phy/sunplus/Kconfig"
> >  source "drivers/phy/tegra/Kconfig"
> >  source "drivers/phy/ti/Kconfig"
> >  source "drivers/phy/intel/Kconfig"
> > diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> > index 01e9eff..ed88b6f 100644
> > --- a/drivers/phy/Makefile
> > +++ b/drivers/phy/Makefile
> > @@ -31,6 +31,7 @@ obj-y                                       += allwinner/   \
> >                                          samsung/     \
> >                                          socionext/   \
> >                                          st/          \
> > +                                        sunplus/             \
> >                                          tegra/       \
> >                                          ti/          \
> >                                          xilinx/
> > diff --git a/drivers/phy/sunplus/Kconfig b/drivers/phy/sunplus/Kconfig
> > new file mode 100644
> > index 0000000..beb85f4
> > --- /dev/null
> > +++ b/drivers/phy/sunplus/Kconfig
> > @@ -0,0 +1,12 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +
> > +config PHY_SUNPLUS_USB
> > +     tristate "Sunplus USB2 PHY driver"
> > +     depends on OF && (SOC_SP7021 || COMPILE_TEST)
> > +     select GENERIC_PHY
> > +     help
> > +       Enable this to support the USB 2.0 PHY on Sunplus SP7021
> > +       SoC. The USB 2.0 PHY controller supports battery charger
> > +       and synchronous signals, various power down modes including
> > +       operating, partial and suspend modes, and high-speed,
> > +       full-speed and low-speed data transfer.
> > diff --git a/drivers/phy/sunplus/Makefile b/drivers/phy/sunplus/Makefile
> > new file mode 100644
> > index 0000000..71754d5
> > --- /dev/null
> > +++ b/drivers/phy/sunplus/Makefile
> > @@ -0,0 +1,2 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +obj-$(CONFIG_PHY_SUNPLUS_USB)        += phy-sunplus-usb2.o
> > diff --git a/drivers/phy/sunplus/phy-sunplus-usb2.c b/drivers/phy/sunplus/phy-sunplus-usb2.c
> > new file mode 100644
> > index 0000000..a2c17ca
> > --- /dev/null
> > +++ b/drivers/phy/sunplus/phy-sunplus-usb2.c
> > @@ -0,0 +1,248 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * Sunplus SP7021 USB 2.0 phy driver
> > + *
> > + * Copyright (C) 2021 Sunplus Technology Inc., All rights reserved.
>
> 2022
>

I will modify it.

> > + *
> > + * Note 1 : non-posted write command for the registers accesses of
> > + * Sunplus SP7021.
> > + *
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reset.h>
> > +
> > +#define RF_MASK_V(_mask, _val)                       (((_mask) << 16) | (_val))
> > +#define RF_MASK_V_CLR(_mask)                 (((_mask) << 16) | 0)
>
> Please use FIELD_PREP and FIELD_GET macros for these
>

I will modify it.

> > +
> > +#define MASK_BITS                            0xffff
> > +#define OTP_DISC_LEVEL_DEFAULT                       0xd
> > +
> > +// GROUP UPHY
> > +#define CONFIG1                                      0x4
> > +#define J_HS_TX_PWRSAV                               BIT(5)
> > +#define CONFIG3                                      0xc
>
> GENAMSK() ?
>

I will modify it.

>
> > +#define J_FORCE_DISC_ON                              BIT(5)
> > +#define J_DEBUG_CTRL_ADDR_MACRO                      BIT(0)
> > +#define CONFIG7                                      0x1c
> > +#define J_DISC                                       0X1f
> > +#define CONFIG9                                      0x24
> > +#define J_ECO_PATH                           BIT(6)
> > +#define CONFIG16                             0x40
> > +#define J_TBCWAIT_MASK                               GENMASK(6, 5)
> > +#define J_TBCWAIT_1P1_MS                     FIELD_PREP(J_TBCWAIT_MASK, 0)
> > +#define J_TVDM_SRC_DIS_MASK                  GENMASK(4, 3)
> > +#define J_TVDM_SRC_DIS_8P2_MS                        FIELD_PREP(J_TVDM_SRC_DIS_MASK, 3)
> > +#define J_TVDM_SRC_EN_MASK                   GENMASK(2, 1)
> > +#define J_TVDM_SRC_EN_1P6_MS                 FIELD_PREP(J_TVDM_SRC_EN_MASK, 0)
> > +#define J_BC_EN                                      BIT(0)
> > +#define CONFIG17                             0x44
> > +#define IBG_TRIM0_MASK                               GENMASK(7, 5)
> > +#define IBG_TRIM0_SSLVHT                     FIELD_PREP(IBG_TRIM0_MASK, 4)
> > +#define J_VDATREE_TRIM_MASK                  GENMASK(4, 1)
> > +#define J_VDATREE_TRIM_DEFAULT                       FIELD_PREP(J_VDATREE_TRIM_MASK, 9)
> > +#define CONFIG23                             0x5c
> > +#define PROB_MASK                            GENMASK(5, 3)
> > +#define PROB                                 FIELD_PREP(PROB_MASK, 7)
> > +
> > +// GROUP MOON4
>
> wrong comments style
>

I will modify it.

> > +static int sp_uphy_init(struct phy *phy)
> > +{
> > +     struct sp_usbphy *usbphy = phy_get_drvdata(phy);
> > +     struct nvmem_cell *cell;
> > +     char *disc_name = "disc_vol";
> > +     ssize_t otp_l = 0;
> > +     char *otp_v;
> > +     u32 val, set, pll_pwr_on, pll_pwr_off;
> > +
> > +     /* Default value modification */
> > +     writel(RF_MASK_V(MASK_BITS, 0x4002), usbphy->moon4_regs + UPHY_CONTROL0);
> > +     writel(RF_MASK_V(MASK_BITS, 0x8747), usbphy->moon4_regs + UPHY_CONTROL1);
> > +
> > +     /* PLL power off/on twice */
> > +     pll_pwr_off = (readl(usbphy->moon4_regs + UPHY_CONTROL3) & ~MASK_BITS)
> > +                     | MO1_UPHY_PLL_POWER_OFF_SEL | MO1_UPHY_PLL_POWER_OFF;
> > +     pll_pwr_on = (readl(usbphy->moon4_regs + UPHY_CONTROL3) & ~MASK_BITS)
> > +                     | MO1_UPHY_PLL_POWER_OFF_SEL;
> > +
> > +     writel(RF_MASK_V(MASK_BITS, pll_pwr_off), usbphy->moon4_regs + UPHY_CONTROL3);
> > +     mdelay(1);
> > +     writel(RF_MASK_V(MASK_BITS, pll_pwr_on), usbphy->moon4_regs + UPHY_CONTROL3);
> > +     mdelay(1);
> > +     writel(RF_MASK_V(MASK_BITS, pll_pwr_off), usbphy->moon4_regs + UPHY_CONTROL3);
> > +     mdelay(1);
> > +     writel(RF_MASK_V(MASK_BITS, pll_pwr_on), usbphy->moon4_regs + UPHY_CONTROL3);
> > +     mdelay(1);
>
> why delay on each register write?
>

They are for the stabilities of capacitors charge/discharge in the PCB board.

> > +     writel(RF_MASK_V(MASK_BITS, 0x0), usbphy->moon4_regs + UPHY_CONTROL3);
> > +
> > +     /* board uphy 0 internal register modification for tid certification */
> > +     cell = nvmem_cell_get(usbphy->dev, disc_name);
> > +     if (IS_ERR_OR_NULL(cell)) {
> > +             if (PTR_ERR(cell) == -EPROBE_DEFER)
> > +                     return -EPROBE_DEFER;
> > +     }
> > +
> > +     otp_v = nvmem_cell_read(cell, &otp_l);
> > +     nvmem_cell_put(cell);
> > +
> > +     if (otp_v) {
> > +             set = *(otp_v + 1);
> > +             set = (set << (sizeof(char) * 8)) | *otp_v;
> > +             set = (set >> usbphy->disc_vol_addr_off) & J_DISC;
> > +     }
> > +
> > +     if (!otp_v || set == 0)
> > +             set = OTP_DISC_LEVEL_DEFAULT;
> > +
> > +     val = readl(usbphy->phy_regs + CONFIG7);
> > +     val = (val & ~J_DISC) | set;
> > +     writel(val, usbphy->phy_regs + CONFIG7);
>
> maybe add a updatel() macro

I will add it.

>
> > +static const struct phy_ops sp_uphy_ops = {
> > +     .init           = sp_uphy_init,
>
> no power_on/off/exit routines??

I will implement them.

>
> > +static const struct of_device_id sp_uphy_dt_ids[] = {
> > +     {.compatible = "sunplus,sp7021-usb2-phy",
> > +      .data = &sp_uphy_ops,},
>
> why should the ops be in driver data?

I will modify it.

>
> --
> ~Vinod

Thanks for your review.

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy