Re: [PATCH v3 RESEND 1/8] phy: hisilicon: add USB physical layer for Kirin 3670

From: Vinod Koul
Date: Wed Sep 16 2020 - 15:05:27 EST

Next message: Alex Dewar: "[PATCH] media: vidtv: fix time conversion error"
Previous message: Vitaly Kuznetsov: "Re: [PATCH] Revert "KVM: Check the allocation of pv cpu mask""
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 11-09-20, 14:16, Mauro Carvalho Chehab wrote:
> From: Yu Chen <chenyu56@xxxxxxxxxx>
>
> Add the Hisilicon Kirin 3670 USB phy driver just after the
> hi3660, using the same namespace.
>
> This driver was imported from Linaro's official Hikey 970
> tree, from the original patch, removing the addition of
> the dwg3-specific parts.
>
> Signed-off-by: Yu Chen <chenyu56@xxxxxxxxxx>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> ---
> .../bindings/phy/phy-hi3670-usb3.txt | 25 +
> drivers/phy/hisilicon/Kconfig | 10 +
> drivers/phy/hisilicon/Makefile | 1 +
> drivers/phy/hisilicon/phy-hi3670-usb3.c | 682 ++++++++++++++++++
> 4 files changed, 718 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/phy-hi3670-usb3.txt
> create mode 100644 drivers/phy/hisilicon/phy-hi3670-usb3.c
>
> diff --git a/Documentation/devicetree/bindings/phy/phy-hi3670-usb3.txt b/Documentation/devicetree/bindings/phy/phy-hi3670-usb3.txt
> new file mode 100644
> index 000000000000..4cb02612ff23
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/phy-hi3670-usb3.txt

Not yaml bindings?

> +#define CTRL0_USB3_VBUSVLD BIT(7)
> +#define CTRL0_USB3_VBUSVLD_SEL BIT(6)
> +
> +#define CTRL3_USB2_VBUSVLDEXT0 BIT(6)
> +#define CTRL3_USB2_VBUSVLDEXTSEL0 BIT(5)
> +
> +#define CTRL5_USB2_SIDDQ BIT(0)
> +
> +#define CTRL7_USB2_REFCLKSEL_MASK (3 << 3)
> +#define CTRL7_USB2_REFCLKSEL_ABB (3 << 3)
> +#define CTRL7_USB2_REFCLKSEL_PAD (2 << 3)

GENMASK() for these please?

> +
> +#define CFG50_USB3_PHY_TEST_POWERDOWN BIT(23)
> +
> +#define CFG54_USB31PHY_CR_ADDR_MASK (0xFFFF)

Here and other places as well

> +#define CFG54_USB31PHY_CR_ADDR_SHIFT (16)

Okay we should get rid of all shift define. See <linux/bitfield.h> it
has FIELD_PREP, xxx_encode_bits() you can use these to program teh bit
fields

> +static int kirin970_phy_cr_clk(struct regmap *usb31misc)
> +{
> + int ret;
> +
> + /* Clock up */
> + ret = regmap_update_bits(usb31misc, USB_MISC_CFG54,
> + CFG54_USB31PHY_CR_CLK, CFG54_USB31PHY_CR_CLK);
> + if (ret)
> + return ret;
> +
> + /* Clock down */
> + ret = regmap_update_bits(usb31misc, USB_MISC_CFG54,
> + CFG54_USB31PHY_CR_CLK, 0);
> +
> + return ret;

return regmap_update_bits()

> +static int kirin970_phy_cr_start(struct regmap *usb31misc, int direction)
> +{
> + int ret;
> +
> + if (direction)
> + ret = regmap_update_bits(usb31misc, USB_MISC_CFG54,
> + CFG54_USB31PHY_CR_WR_EN, CFG54_USB31PHY_CR_WR_EN);
> + else
> + ret = regmap_update_bits(usb31misc, USB_MISC_CFG54,
> + CFG54_USB31PHY_CR_RD_EN, CFG54_USB31PHY_CR_RD_EN);
> +
> + if (ret)
> + return ret;
> +
> + ret = kirin970_phy_cr_clk(usb31misc);
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(usb31misc, USB_MISC_CFG54,
> + CFG54_USB31PHY_CR_RD_EN | CFG54_USB31PHY_CR_WR_EN, 0);
> +
> + return ret;

here as well

> +static int kirin970_phy_cr_wait_ack(struct regmap *usb31misc)
> +{
> + u32 reg;
> + int retry = 100000;
> + int ret;
> +
> + while (retry-- > 0) {
> + ret = regmap_read(usb31misc, USB_MISC_CFG54, &reg);
> + if (ret)
> + return ret;
> + if ((reg & CFG54_USB31PHY_CR_ACK) == CFG54_USB31PHY_CR_ACK)
> + return 0;
> +
> + ret = kirin970_phy_cr_clk(usb31misc);
> + if (ret)
> + return ret;

no delay here while checking?

> +static int kirin970_phy_cr_set_addr(struct regmap *usb31misc, u32 addr)
> +{
> + u32 reg;
> + int ret;
> +
> + ret = regmap_read(usb31misc, USB_MISC_CFG54, &reg);
> + if (ret)
> + return ret;
> +
> + reg &= ~(CFG54_USB31PHY_CR_ADDR_MASK << CFG54_USB31PHY_CR_ADDR_SHIFT);
> + reg |= ((addr & CFG54_USB31PHY_CR_ADDR_MASK) <<
> + CFG54_USB31PHY_CR_ADDR_SHIFT);

u32_replace_bits() looks apt here

> +static int kirin970_phy_cr_read(struct regmap *usb31misc, u32 addr, u32 *val)
> +{
> + int reg;
> + int i;
> + int ret;
> +
> + for (i = 0; i < 100; i++) {
> + ret = kirin970_phy_cr_clk(usb31misc);
> + if (ret)
> + return ret;
> + }
> +
> + ret = kirin970_phy_cr_set_sel(usb31misc);
> + if (ret)
> + return ret;
> +
> + ret = kirin970_phy_cr_set_addr(usb31misc, addr);
> + if (ret)
> + return ret;
> +
> + ret = kirin970_phy_cr_start(usb31misc, 0);
> + if (ret)
> + return ret;
> +
> + ret = kirin970_phy_cr_wait_ack(usb31misc);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(usb31misc, USB_MISC_CFG58, &reg);
> + if (ret)
> + return ret;

Do you really care about each step error check? if not, we can have:
ret |= step_1;
ret |= step_n;

if (ret)
...

> +static int kirin970_phy_init(struct phy *phy)
> +{
> + struct kirin970_priv *priv = phy_get_drvdata(phy);
> + u32 val;
> + int ret;
> +
> + kirin970_phy_exit(phy);
> + dev_info(priv->dev, "%s in\n", __func__);

Lower to debug level?

> + /* assert controller */
> + val = CFGA0_VAUX_RESET | CFGA0_USB31C_RESET;
> + ret = regmap_update_bits(priv->usb31misc, USB_MISC_CFGA0, val, 0);
> + if (ret)
> + goto out;
> +
> + ret = kirin970_config_phy_clock(priv);
> + if (ret)
> + goto out;
> +
> + /* Exit from IDDQ mode */
> + ret = regmap_update_bits(priv->usb31misc, USB3OTG_CTRL5,
> + CTRL5_USB2_SIDDQ, 0);
> + if (ret)
> + goto out;
> +
> + /* Release USB31 PHY out of TestPowerDown mode */
> + ret = regmap_update_bits(priv->usb31misc, USB_MISC_CFG50,
> + CFG50_USB3_PHY_TEST_POWERDOWN, 0);
> + if (ret)
> + goto out;
> +
> + /* Tell the PHY power is stable */
> + val = CFG54_USB3_PHY0_ANA_PWR_EN | CFG54_PHY0_PCS_PWR_STABLE |
> + CFG54_PHY0_PMA_PWR_STABLE;
> + ret = regmap_update_bits(priv->usb31misc, USB_MISC_CFG54,
> + val, val);
> + if (ret)
> + goto out;
> +
> + ret = kirin970_config_tca(priv);
> + if (ret)
> + goto out;
> +
> + /* Enable SSC */
> + ret = regmap_update_bits(priv->usb31misc, USB_MISC_CFG5C,
> + CFG5C_USB3_PHY0_SS_MPLLA_SSC_EN,
> + CFG5C_USB3_PHY0_SS_MPLLA_SSC_EN);
> + if (ret)
> + goto out;
> +
> + /* Deassert phy */
> + val = CFGA0_USB3PHY_RESET | CFGA0_USB2PHY_POR;
> + ret = regmap_update_bits(priv->usb31misc, USB_MISC_CFGA0, val, val);
> + if (ret)
> + goto out;
> +
> + udelay(100);
> +
> + /* Deassert controller */
> + val = CFGA0_VAUX_RESET | CFGA0_USB31C_RESET;
> + ret = regmap_update_bits(priv->usb31misc, USB_MISC_CFGA0, val, val);
> + if (ret)
> + goto out;
> +
> + udelay(100);
> +
> + /* Set fake vbus valid signal */
> + val = CTRL0_USB3_VBUSVLD | CTRL0_USB3_VBUSVLD_SEL;
> + ret = regmap_update_bits(priv->usb31misc, USB3OTG_CTRL0, val, val);
> + if (ret)
> + goto out;
> +
> + val = CTRL3_USB2_VBUSVLDEXT0 | CTRL3_USB2_VBUSVLDEXTSEL0;
> + ret = regmap_update_bits(priv->usb31misc, USB3OTG_CTRL3, val, val);
> + if (ret)
> + goto out;
> +
> + udelay(100);
> +
> + ret = kirin970_phy_set_params(priv);
> + if (ret)
> + goto out;
> +
> + {
> + ret = regmap_read(priv->peri_crg, 0x4c,
> + &val);
> + if (!ret)
> + dev_info(priv->dev, "peri_crg 0x4c %x\n", val);
> + ret = regmap_read(priv->peri_crg, 0x404,
> + &val);
> + if (!ret)
> + dev_info(priv->dev, "peri_crg 0x404 %x\n", val);
> + ret = regmap_read(priv->peri_crg, 0xc,
> + &val);
> + if (!ret)
> + dev_info(priv->dev, "peri_crg 0xc %x\n", val);
> + ret = regmap_read(priv->peri_crg, 0xac,
> + &val);
> + if (!ret)
> + dev_info(priv->dev, "peri_crg 0xac %x\n", val);
> + ret = regmap_read(priv->pctrl, 0x10,
> + &val);
> + if (!ret)
> + dev_info(priv->dev, "pctrl 0x10 %x\n", val);
> + }

Whats with the funny braces and one more level on indentation here ?

--
~Vinod

Next message: Alex Dewar: "[PATCH] media: vidtv: fix time conversion error"
Previous message: Vitaly Kuznetsov: "Re: [PATCH] Revert "KVM: Check the allocation of pv cpu mask""
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]