| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200916130553.GO2968@vkoul-mobl>
Date: Wed, 16 Sep 2020 18:35:53 +0530
From: Vinod Koul <vkoul@...nel.org>
To: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc: linuxarm@...wei.com, mauro.chehab@...wei.com,
Yu Chen <chenyu56@...wei.com>,
John Stultz <john.stultz@...aro.org>,
Manivannan Sadhasivam <mani@...nel.org>,
Kishon Vijay Abraham I <kishon@...com>,
Krzysztof Kozlowski <krzk@...nel.org>,
Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 RESEND 1/8] phy: hisilicon: add USB physical layer for
Kirin 3670
On 11-09-20, 14:16, Mauro Carvalho Chehab wrote:
> From: Yu Chen <chenyu56@...wei.com>
>
> 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@...wei.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
> ---
> .../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, ®);
> + 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, ®);
> + 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, ®);
> + 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
Powered by blists - more mailing lists