Messages in this thread | | | Date | Wed, 22 Jul 2020 19:19:20 +0200 | From | Paul Cercueil <> | Subject | Re: [PATCH v4 2/3] USB: PHY: JZ4770: Add support for new Ingenic SoCs. |
| |
Hi Zhou,
Le mer. 22 juil. 2020 à 14:33, 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> a écrit : > Add support for probing the phy-jz4770 driver on the JZ4780 SoC, > the X1000 SoC and the X1830 SoC from Ingenic. > > Tested-by: 周正 (Zhou Zheng) <sernia.zhou@foxmail.com> > Co-developed-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com> > Signed-off-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com> > Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> > --- > > Notes: > v1->v2: > Add bindings for the JZ4780 SoC. > > v2->v3: > Use "of_device_get_match_data" instead "of_match_device" > to get version information. > > v3->v4: > Fix typos. > > drivers/usb/phy/Kconfig | 4 +- > drivers/usb/phy/phy-jz4770.c | 174 > ++++++++++++++++++++++++++++++++----------- > 2 files changed, 133 insertions(+), 45 deletions(-) > > diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig > index 4b3fa78995cf..775f0dd7b5f5 100644 > --- a/drivers/usb/phy/Kconfig > +++ b/drivers/usb/phy/Kconfig > @@ -185,11 +185,11 @@ config USB_ULPI_VIEWPORT > controllers with a viewport register (e.g. Chipidea/ARC > controllers). > > config JZ4770_PHY > - tristate "Ingenic JZ4770 Transceiver Driver" > + tristate "Ingenic SoCs Transceiver Driver" > depends on MIPS || COMPILE_TEST > select USB_PHY > help > This driver provides PHY support for the USB controller found > - on the JZ4770 SoC from Ingenic. > + on the JZ4770/JZ4780/X1000/X1830 SoC from Ingenic.
'JZ-series / X-series SoCs' would be more future-proof :)
> > endmenu > diff --git a/drivers/usb/phy/phy-jz4770.c > b/drivers/usb/phy/phy-jz4770.c > index 8f62dc2a90ff..cd49b32b4c13 100644 > --- a/drivers/usb/phy/phy-jz4770.c > +++ b/drivers/usb/phy/phy-jz4770.c > @@ -1,27 +1,30 @@ > // SPDX-License-Identifier: GPL-2.0 > /* > - * Ingenic JZ4770 USB PHY driver > + * Ingenic SoCs USB PHY driver > * Copyright (c) Paul Cercueil <paul@crapouillou.net> > + * Copyright (c) 漆鹏振 (Qi Pengzhen) <aric.pzqi@ingenic.com> > + * Copyright (c) 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> > */ > > #include <linux/clk.h> > #include <linux/io.h> > #include <linux/module.h> > +#include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/regulator/consumer.h> > #include <linux/usb/otg.h> > #include <linux/usb/phy.h> > > +/* OTGPHY register offsets */ > #define REG_USBPCR_OFFSET 0x00 > #define REG_USBRDT_OFFSET 0x04 > #define REG_USBVBFIL_OFFSET 0x08 > #define REG_USBPCR1_OFFSET 0x0c > > -/* USBPCR */ > +/* bits within the USBPCR register */ > #define USBPCR_USB_MODE BIT(31) > #define USBPCR_AVLD_REG BIT(30) > -#define USBPCR_INCRM BIT(27) > -#define USBPCR_CLK12_EN BIT(26) > +#define USBPCR_INCR_MASK BIT(27)
Please don't rename macros, it adds a lot of noise to the patch.
> #define USBPCR_COMMONONN BIT(25) > #define USBPCR_VBUSVLDEXT BIT(24) > #define USBPCR_VBUSVLDEXTSEL BIT(23) > @@ -32,46 +35,80 @@ > > #define USBPCR_IDPULLUP_LSB 28 > #define USBPCR_IDPULLUP_MASK GENMASK(29, USBPCR_IDPULLUP_LSB) > -#define USBPCR_IDPULLUP_ALWAYS (3 << USBPCR_IDPULLUP_LSB) > -#define USBPCR_IDPULLUP_SUSPEND (1 << USBPCR_IDPULLUP_LSB) > -#define USBPCR_IDPULLUP_OTG (0 << USBPCR_IDPULLUP_LSB) > +#define USBPCR_IDPULLUP_ALWAYS (0x2 << USBPCR_IDPULLUP_LSB) > +#define USBPCR_IDPULLUP_SUSPEND (0x1 << USBPCR_IDPULLUP_LSB) > +#define USBPCR_IDPULLUP_OTG (0x0 << USBPCR_IDPULLUP_LSB)
a '3' turned into a '2' here...
> > #define USBPCR_COMPDISTUNE_LSB 17 > #define USBPCR_COMPDISTUNE_MASK GENMASK(19, USBPCR_COMPDISTUNE_LSB) > -#define USBPCR_COMPDISTUNE_DFT 4 > +#define USBPCR_COMPDISTUNE_DFT (0x4 << USBPCR_COMPDISTUNE_LSB)
Why? Even if you have a valid reason to do that, it does not belong in this patch.
> > #define USBPCR_OTGTUNE_LSB 14 > #define USBPCR_OTGTUNE_MASK GENMASK(16, USBPCR_OTGTUNE_LSB) > -#define USBPCR_OTGTUNE_DFT 4 > +#define USBPCR_OTGTUNE_DFT (0x4 << USBPCR_OTGTUNE_LSB) > > #define USBPCR_SQRXTUNE_LSB 11 > #define USBPCR_SQRXTUNE_MASK GENMASK(13, USBPCR_SQRXTUNE_LSB) > -#define USBPCR_SQRXTUNE_DFT 3 > +#define USBPCR_SQRXTUNE_DCR_20PCT (0x7 << USBPCR_SQRXTUNE_LSB) > +#define USBPCR_SQRXTUNE_DFT (0x3 << USBPCR_SQRXTUNE_LSB) > > #define USBPCR_TXFSLSTUNE_LSB 7 > #define USBPCR_TXFSLSTUNE_MASK GENMASK(10, USBPCR_TXFSLSTUNE_LSB) > -#define USBPCR_TXFSLSTUNE_DFT 3 > +#define USBPCR_TXFSLSTUNE_DCR_50PPT (0xf << USBPCR_TXFSLSTUNE_LSB) > +#define USBPCR_TXFSLSTUNE_DCR_25PPT (0x7 << USBPCR_TXFSLSTUNE_LSB) > +#define USBPCR_TXFSLSTUNE_DFT (0x3 << USBPCR_TXFSLSTUNE_LSB) > +#define USBPCR_TXFSLSTUNE_INC_25PPT (0x1 << USBPCR_TXFSLSTUNE_LSB) > +#define USBPCR_TXFSLSTUNE_INC_50PPT (0x0 << USBPCR_TXFSLSTUNE_LSB) > + > +#define USBPCR_TXHSXVTUNE_LSB 4 > +#define USBPCR_TXHSXVTUNE_MASK GENMASK(5, USBPCR_TXHSXVTUNE_LSB) > +#define USBPCR_TXHSXVTUNE_DFT (0x3 << USBPCR_TXHSXVTUNE_LSB) > +#define USBPCR_TXHSXVTUNE_DCR_15MV (0x1 << USBPCR_TXHSXVTUNE_LSB) > > #define USBPCR_TXRISETUNE_LSB 4 > #define USBPCR_TXRISETUNE_MASK GENMASK(5, USBPCR_TXRISETUNE_LSB) > -#define USBPCR_TXRISETUNE_DFT 3 > +#define USBPCR_TXRISETUNE_DFT (0x3 << USBPCR_TXRISETUNE_LSB) > > #define USBPCR_TXVREFTUNE_LSB 0 > #define USBPCR_TXVREFTUNE_MASK GENMASK(3, USBPCR_TXVREFTUNE_LSB) > -#define USBPCR_TXVREFTUNE_DFT 5 > +#define USBPCR_TXVREFTUNE_INC_25PPT (0x7 << USBPCR_TXVREFTUNE_LSB) > +#define USBPCR_TXVREFTUNE_DFT (0x5 << USBPCR_TXVREFTUNE_LSB) > > -/* USBRDT */ > +/* bits within the USBRDTR register */ > +#define USBRDT_UTMI_RST BIT(27) > +#define USBRDT_HB_MASK BIT(26) > #define USBRDT_VBFIL_LD_EN BIT(25) > #define USBRDT_IDDIG_EN BIT(24) > #define USBRDT_IDDIG_REG BIT(23) > - > -#define USBRDT_USBRDT_LSB 0 > -#define USBRDT_USBRDT_MASK GENMASK(22, USBRDT_USBRDT_LSB) > - > -/* USBPCR1 */ > -#define USBPCR1_UHC_POWON BIT(5) > +#define USBRDT_VBFIL_EN BIT(2) > + > +/* bits within the USBPCR1 register */ > +#define USBPCR1_BVLD_REG BIT(31) > +#define USBPCR1_DPPD BIT(29) > +#define USBPCR1_DMPD BIT(28) > +#define USBPCR1_USB_SEL BIT(28) > +#define USBPCR1_WORD_IF_16BIT BIT(19) > + > +#define USBPCR1_REFCLKSEL_LSB 26 > +#define USBPCR1_REFCLKSEL_MASK GENMASK(27, USBPCR1_REFCLKDIV_LSB) > +#define USBPCR1_REFCLKSEL_CLKCORE (0x3 << USBPCR1_REFCLKSEL_LSB) > + > +#define USBPCR1_REFCLKDIV_LSB 24 > +#define USBPCR1_REFCLKDIV_MASK GENMASK(25, USBPCR1_REFCLKDIV_LSB) > +#define USBPCR1_REFCLKDIV_48M (0x2 << USBPCR1_REFCLKDIV_LSB) > +#define USBPCR1_REFCLKDIV_24M (0x1 << USBPCR1_REFCLKDIV_LSB) > +#define USBPCR1_REFCLKDIV_12M (0x0 << USBPCR1_REFCLKDIV_LSB) > + > +enum ingenic_usb_phy_version { > + ID_JZ4770, > + ID_JZ4780, > + ID_X1000, > + ID_X1830, > +}; > > struct jz4770_phy { > + enum ingenic_usb_phy_version version; > + > struct usb_phy phy; > struct usb_otg otg; > struct device *dev; > @@ -96,6 +133,12 @@ static int jz4770_phy_set_peripheral(struct > usb_otg *otg, > struct jz4770_phy *priv = otg_to_jz4770_phy(otg); > u32 reg; > > + if (priv->version >= ID_X1000) { > + reg = readl(priv->base + REG_USBPCR1_OFFSET); > + reg |= USBPCR1_BVLD_REG; > + writel(reg, priv->base + REG_USBPCR1_OFFSET); > + } > + > reg = readl(priv->base + REG_USBPCR_OFFSET); > reg &= ~USBPCR_USB_MODE; > reg |= USBPCR_VBUSVLDEXT | USBPCR_VBUSVLDEXTSEL | > USBPCR_OTG_DISABLE; > @@ -135,17 +178,59 @@ static int jz4770_phy_init(struct usb_phy *phy) > return err; > } > > - reg = USBPCR_AVLD_REG | USBPCR_COMMONONN | USBPCR_IDPULLUP_ALWAYS | > - (USBPCR_COMPDISTUNE_DFT << USBPCR_COMPDISTUNE_LSB) | > - (USBPCR_OTGTUNE_DFT << USBPCR_OTGTUNE_LSB) | > - (USBPCR_SQRXTUNE_DFT << USBPCR_SQRXTUNE_LSB) | > - (USBPCR_TXFSLSTUNE_DFT << USBPCR_TXFSLSTUNE_LSB) | > - (USBPCR_TXRISETUNE_DFT << USBPCR_TXRISETUNE_LSB) | > - (USBPCR_TXVREFTUNE_DFT << USBPCR_TXVREFTUNE_LSB) | > - USBPCR_POR; > + if (priv->version >= ID_X1830) { > + /* rdt */ > + writel(USBRDT_VBFIL_EN | USBRDT_UTMI_RST, priv->base + > REG_USBRDT_OFFSET); > + > + reg = readl(priv->base + REG_USBPCR1_OFFSET) | > USBPCR1_WORD_IF_16BIT | > + USBPCR1_DMPD | USBPCR1_DPPD; > + writel(reg, priv->base + REG_USBPCR1_OFFSET); > + > + reg = USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT > | USBPCR_TXPREEMPHTUNE; > + } else if (priv->version >= ID_X1000) { > + reg = readl(priv->base + REG_USBPCR1_OFFSET) | > USBPCR1_WORD_IF_16BIT; > + writel(reg, priv->base + REG_USBPCR1_OFFSET); > + > + reg = USBPCR_SQRXTUNE_DCR_20PCT | USBPCR_TXPREEMPHTUNE | > + USBPCR_TXHSXVTUNE_DCR_15MV | USBPCR_TXVREFTUNE_INC_25PPT; > + } else if (priv->version >= ID_JZ4780) { > + reg = readl(priv->base + REG_USBPCR1_OFFSET) | USBPCR1_USB_SEL | > + USBPCR1_WORD_IF_16BIT; > + writel(reg, priv->base + REG_USBPCR1_OFFSET); > + > + reg = USBPCR_TXPREEMPHTUNE; > + } else { > + reg = USBPCR_AVLD_REG | USBPCR_IDPULLUP_ALWAYS | > + USBPCR_COMPDISTUNE_DFT | USBPCR_OTGTUNE_DFT | > + USBPCR_SQRXTUNE_DFT | USBPCR_TXFSLSTUNE_DFT | > + USBPCR_TXRISETUNE_DFT | USBPCR_TXVREFTUNE_DFT; > + } > + > + reg |= USBPCR_COMMONONN | USBPCR_POR; > writel(reg, priv->base + REG_USBPCR_OFFSET); > > - /* Wait for PHY to reset */ > + /* > + * Power-On Reset(POR) > + * Function:This customer-specific signal resets all test registers > + * and state machines in the USB 2.0 nanoPHY. > + * The POR signal must be asserted for a minimum of 10 μs. > + * For POR timing information: > + * > + * T0: Power-on reset (POR) is initiated. 0 (reference). > + * T1: T1 indicates when POR can be set to 1’b0. (To provide > examples, > + * values for T2 and T3 are also shown where T1 = T0 + 30 μs.); > + * In general, T1 must be ≥ T0 + 10 μs. T0 + 10 μs ≤ T1. > + * T2: T2 indicates when PHYCLOCK, CLK48MOHCI, and CLK12MOHCI are > + * available at the macro output, based on the USB 2.0 nanoPHY > + * reference clock source. > + * Crystal: > + * • When T1 = T0 + 10 μs: > + * T2 < T1 + 805 μs = T0 + 815 μs > + * • When T1 = T0 + 30 μs: > + * T2 < T1 + 805 μs = T0 + 835 μs > + * see "Reset and Power-Saving Signals" on page 60 an “Powering Up > + * and Powering Down the USB 2.0 nanoPHY” on page 73. > + */
I don't think this comment is very interesting. The code already says that the hardware needs at least 30 µs to reset, all the low-level details are in the programming manual for those who want to learn more. Besides, since you don't actually modify the reset code, why update the comment in this patch?
> usleep_range(30, 300); > writel(reg & ~USBPCR_POR, priv->base + REG_USBPCR_OFFSET); > usleep_range(300, 1000); > @@ -166,6 +251,15 @@ static void jz4770_phy_remove(void *phy) > usb_remove_phy(phy); > } > > +static const struct of_device_id ingenic_usb_phy_of_matches[] = { > + { .compatible = "ingenic,jz4770-phy", .data = (void *) ID_JZ4770 }, > + { .compatible = "ingenic,jz4780-phy", .data = (void *) ID_JZ4780 }, > + { .compatible = "ingenic,x1000-phy", .data = (void *) ID_X1000 }, > + { .compatible = "ingenic,x1830-phy", .data = (void *) ID_X1830 },
I'm not a fan of having ID_* as platform data. Create 'soc_info' structures, one per supported SoC, and pass them here. Then you can add a 'version' field of type 'enum ingenic_usb_phy_version'. And add a '.phy_init' field as a function pointer, that would be called within jz4770_phy_init(). That would make the jz4770_phy_init() function much cleaner (no if/else/else/else/...).
> + { } > +}; > +MODULE_DEVICE_TABLE(of, ingenic_usb_phy_of_matches); > + > static int jz4770_phy_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -176,11 +270,13 @@ static int jz4770_phy_probe(struct > platform_device *pdev) > if (!priv) > return -ENOMEM; > > + priv->version = (enum > ingenic_usb_phy_version)of_device_get_match_data(dev); > + > platform_set_drvdata(pdev, priv); > priv->dev = dev; > priv->phy.dev = dev; > priv->phy.otg = &priv->otg; > - priv->phy.label = "jz4770-phy"; > + priv->phy.label = "ingenic-usb-phy"; > priv->phy.init = jz4770_phy_init; > priv->phy.shutdown = jz4770_phy_shutdown; > > @@ -221,23 +317,15 @@ static int jz4770_phy_probe(struct > platform_device *pdev) > return devm_add_action_or_reset(dev, jz4770_phy_remove, &priv->phy); > } > > -#ifdef CONFIG_OF > -static const struct of_device_id jz4770_phy_of_matches[] = { > - { .compatible = "ingenic,jz4770-phy" }, > - { } > -}; > -MODULE_DEVICE_TABLE(of, jz4770_phy_of_matches); > -#endif > - > -static struct platform_driver jz4770_phy_driver = { > +static struct platform_driver ingenic_usb_phy_driver = { > .probe = jz4770_phy_probe, > .driver = { > - .name = "jz4770-phy", > - .of_match_table = of_match_ptr(jz4770_phy_of_matches), > + .name = "ingenic-usb-phy", > + .of_match_table = of_match_ptr(ingenic_usb_phy_of_matches),
It's preferred not to rename the functions unless you have a good reason, and you definitely shouldn't rename the module. It's OK to have a driver called 'jz4770-phy' even if it supports more SoCs. Renaming it breaks userspace ABI.
Cheers, -Paul
> }, > }; > -module_platform_driver(jz4770_phy_driver); > +module_platform_driver(ingenic_usb_phy_driver); > > MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>"); > -MODULE_DESCRIPTION("Ingenic JZ4770 USB PHY driver"); > +MODULE_DESCRIPTION("Ingenic SoCs USB PHY driver"); > MODULE_LICENSE("GPL"); > -- > 2.11.0 >
| |