Hi Paul, 在 2020/7/23 上午1:19, Paul Cercueil 写道:
Hi Zhou,Le mer. 22 juil. 2020 à 14:33, 周琰杰 (Zhou Yanjie) <zhouyanjie@xxxxxxxxxxxxxx> 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@xxxxxxxxxxx> Co-developed-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@xxxxxxxxxxx> Signed-off-by: 漆鹏振 (Qi Pengzhen) <aric.pzqi@xxxxxxxxxxx> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@xxxxxxxxxxxxxx> --- 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_VIEWPORTcontrollers 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 :)
Sure.
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@xxxxxxxxxxxxxxx> + * Copyright (c) 漆鹏振 (Qi Pengzhen) <aric.pzqi@xxxxxxxxxxx> + * Copyright (c) 周琰杰 (Zhou Yanjie) <zhouyanjie@xxxxxxxxxxxxxx> */ #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.
Sure.
#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...
From the information in the programming manual, it can be 3 or 2.I think using 2 with 1 and 0 below seems more in line with reading habits :)
#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.
USBPCR_COMPDISTUNE_DFT (and below) is using in jz4770_phy_init(), to do this can simplify jz4770_phy_init(), do I need separate it as a separate patch in the next version?
#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?
Okay, I will drop it in the next version.
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/...).
Sure, I will do it in the next version.
+ { } +}; +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.
Sure. Thanks and best regards!
Cheers, -Paul}, }; -module_platform_driver(jz4770_phy_driver); +module_platform_driver(ingenic_usb_phy_driver); MODULE_AUTHOR("Paul Cercueil <paul@xxxxxxxxxxxxxxx>"); -MODULE_DESCRIPTION("Ingenic JZ4770 USB PHY driver"); +MODULE_DESCRIPTION("Ingenic SoCs USB PHY driver"); MODULE_LICENSE("GPL"); -- 2.11.0
