| Message ID | cover.1686126439.git.quic_varada@quicinc.com |
|---|---|
| Headers | show |
| Series | Enable IPQ5332 USB2 | expand |
On 07/06/2023 13:56, Varadarajan Narayanan wrote: > Fix the USB related clock defines and add details > referenced by them > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > --- > drivers/clk/qcom/gcc-ipq5332.c | 34 +++++++++++++++++++++++----------- > 1 file changed, 23 insertions(+), 11 deletions(-) > > diff --git a/drivers/clk/qcom/gcc-ipq5332.c b/drivers/clk/qcom/gcc-ipq5332.c > index a75ab88..2b58558 100644 > --- a/drivers/clk/qcom/gcc-ipq5332.c > +++ b/drivers/clk/qcom/gcc-ipq5332.c > @@ -351,6 +351,16 @@ static const struct freq_tbl ftbl_gcc_adss_pwm_clk_src[] = { > { } > }; > > +static const struct clk_parent_data gcc_usb3phy_0_cc_pipe_clk_xo[] = { > + { .fw_name = "usb3phy_0_cc_pipe_clk" }, > + { .fw_name = "xo" }, gcc-ipq5332 uses DT indices, please don't mix that with .fw_name. > +}; > + > +static const struct parent_map gcc_usb3phy_0_cc_pipe_clk_xo_map[] = { > + { P_USB3PHY_0_PIPE, 0 }, > + { P_XO, 2 }, > +}; > + > static struct clk_rcg2 gcc_adss_pwm_clk_src = { > .cmd_rcgr = 0x1c004, > .mnd_width = 0, > @@ -1101,16 +1111,18 @@ static struct clk_rcg2 gcc_usb0_mock_utmi_clk_src = { > }, > }; > > -static struct clk_regmap_phy_mux gcc_usb0_pipe_clk_src = { > +static struct clk_regmap_mux usb0_pipe_clk_src = { > .reg = 0x2c074, > + .shift = 8, > + .width = 2, > + .parent_map = gcc_usb3phy_0_cc_pipe_clk_xo_map, > .clkr = { > - .hw.init = &(struct clk_init_data) { > - .name = "gcc_usb0_pipe_clk_src", > - .parent_data = &(const struct clk_parent_data) { > - .index = DT_USB_PCIE_WRAPPER_PIPE_CLK, > - }, > - .num_parents = 1, > - .ops = &clk_regmap_phy_mux_ops, > + .hw.init = &(const struct clk_init_data){ > + .name = "usb0phy_0_cc_pipe_clk_src", > + .parent_data = gcc_usb3phy_0_cc_pipe_clk_xo, > + .num_parents = 2, > + .ops = &clk_regmap_mux_closest_ops, > + .flags = CLK_SET_RATE_PARENT, > }, Soo... As you are reverting this. Is USB0 PIPE clock required to be parked to the XO? I was going to write 'before turning USB0_GDSC' off, but then I noticed that gcc-ipq5332 doesn't declare GDSCs. Does this platform have GDSCs? > }, > }; > @@ -3041,8 +3053,8 @@ static struct clk_branch gcc_usb0_pipe_clk = { > .enable_mask = BIT(0), > .hw.init = &(const struct clk_init_data) { > .name = "gcc_usb0_pipe_clk", > - .parent_hws = (const struct clk_hw*[]) { > - &gcc_usb0_pipe_clk_src.clkr.hw, > + .parent_names = (const char *[]){ > + "usb0_pipe_clk_src" complete and definitive NAK. Do not use parent_names, we have just stopped migrating from them. > }, > .num_parents = 1, > .flags = CLK_SET_RATE_PARENT, > @@ -3580,7 +3592,7 @@ static struct clk_regmap *gcc_ipq5332_clocks[] = { > [GCC_PCIE3X2_PIPE_CLK_SRC] = &gcc_pcie3x2_pipe_clk_src.clkr, > [GCC_PCIE3X1_0_PIPE_CLK_SRC] = &gcc_pcie3x1_0_pipe_clk_src.clkr, > [GCC_PCIE3X1_1_PIPE_CLK_SRC] = &gcc_pcie3x1_1_pipe_clk_src.clkr, > - [GCC_USB0_PIPE_CLK_SRC] = &gcc_usb0_pipe_clk_src.clkr, > + [GCC_USB0_PIPE_CLK_SRC] = &usb0_pipe_clk_src.clkr, > }; > > static const struct qcom_reset_map gcc_ipq5332_resets[] = {
On 07/06/2023 13:56, Varadarajan Narayanan wrote: > Introduce CONFIG_PHY_QCOM_M31_USB for including the M31 phy driver > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> Is there any reason to keep Kconfig, Makefile and driver in different commits? > --- > drivers/phy/qualcomm/Kconfig | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig > index 67a45d9..8a363dd 100644 > --- a/drivers/phy/qualcomm/Kconfig > +++ b/drivers/phy/qualcomm/Kconfig > @@ -188,3 +188,14 @@ config PHY_QCOM_IPQ806X_USB > This option enables support for the Synopsis PHYs present inside the > Qualcomm USB3.0 DWC3 controller on ipq806x SoC. This driver supports > both HS and SS PHY controllers. > + > +config PHY_QCOM_M31_USB > + tristate "Qualcomm M31 HS PHY driver support" > + depends on (USB || USB_GADGET) && ARCH_QCOM > + select USB_PHY > + help > + Enable this to support M31 HS PHY transceivers on Qualcomm chips > + with DWC3 USB core. It handles PHY initialization, clock > + management required after resetting the hardware and power > + management. This driver is required even for peripheral only or > + host only mode configurations.
On 07/06/2023 13:56, Varadarajan Narayanan wrote: > Add USB phy and controller nodes > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > --- > arch/arm64/boot/dts/qcom/ipq5332.dtsi | 55 +++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi > index c2d6cc65..3183357 100644 > --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi > +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi > @@ -383,6 +383,61 @@ > status = "disabled"; > }; > }; > + > + usb_0_m31phy: hs_m31phy@7b000 { > + compatible = "qcom,ipq5332-m31-usb-hsphy"; > + reg = <0x0007b000 0x12C>, > + <0x08af8800 0x400>; > + reg-names = "m31usb_phy_base", > + "qscratch_base"; > + phy_type= "utmi"; > + > + resets = <&gcc GCC_QUSB2_0_PHY_BCR>; > + reset-names = "usb2_phy_reset"; > + > + status = "okay"; > + }; > + > + usb2: usb2@8a00000 { > + compatible = "qcom,ipq5332-dwc3", "qcom,dwc3"; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + reg = <0x08af8800 0x100>; > + > + clocks = <&gcc GCC_USB0_MASTER_CLK>, > + <&gcc GCC_SNOC_USB_CLK>, > + <&gcc GCC_USB0_SLEEP_CLK>, > + <&gcc GCC_USB0_MOCK_UTMI_CLK>; Please indent these values. > + > + clock-names = "core", > + "iface", > + "sleep", > + "mock_utmi"; Please indent these values. > + > + interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>; No PHY IRQs? > + interrupt-names = "pwr_event"; > + > + resets = <&gcc GCC_USB_BCR>; > + > + qcom,select-utmi-as-pipe-clk; > + > + usb2_0_dwc: usb@8a00000 { > + compatible = "snps,dwc3"; > + reg = <0x08a00000 0xe000>; > + clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>; > + clock-names = "ref"; > + interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>; > + usb-phy = <&usb_0_m31phy>; > + tx-fifo-resize; > + snps,is-utmi-l1-suspend; > + snps,hird-threshold = /bits/ 8 <0x0>; > + snps,dis_u2_susphy_quirk; > + snps,dis_u3_susphy_quirk; > + snps,ref-clock-period-ns = <21>; > + }; > + }; > }; > > timer {
On 7.06.2023 12:56, Varadarajan Narayanan wrote: > Add the M31 USB2 phy driver > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > --- > drivers/phy/qualcomm/phy-qcom-m31.c | 360 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 360 insertions(+) > create mode 100644 drivers/phy/qualcomm/phy-qcom-m31.c > > diff --git a/drivers/phy/qualcomm/phy-qcom-m31.c b/drivers/phy/qualcomm/phy-qcom-m31.c > new file mode 100644 > index 0000000..d29a91e > --- /dev/null > +++ b/drivers/phy/qualcomm/phy-qcom-m31.c > @@ -0,0 +1,360 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (c) 2014-2016, 2020, The Linux Foundation. All rights reserved. > + */ > + > +#include <linux/module.h> > +#include <linux/kernel.h> > +#include <linux/err.h> > +#include <linux/slab.h> > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/io.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/usb/phy.h> > +#include <linux/reset.h> > +#include <linux/of_device.h> Please sort these > + > +enum clk_reset_action { > + CLK_RESET_DEASSERT = 0, > + CLK_RESET_ASSERT = 1 > +}; > + > +#define USB2PHY_PORT_POWERDOWN 0xA4 > +#define POWER_UP BIT(0) > +#define POWER_DOWN 0 > + > +#define USB2PHY_PORT_UTMI_CTRL1 0x40 > + > +#define USB2PHY_PORT_UTMI_CTRL2 0x44 > +#define UTMI_ULPI_SEL BIT(7) > +#define UTMI_TEST_MUX_SEL BIT(6) > + > +#define HS_PHY_CTRL_REG 0x10 > +#define UTMI_OTG_VBUS_VALID BIT(20) > +#define SW_SESSVLD_SEL BIT(28) > + > +#define USB_PHY_CFG0 0x94 > +#define USB_PHY_UTMI_CTRL5 0x50 > +#define USB_PHY_FSEL_SEL 0xB8 > +#define USB_PHY_HS_PHY_CTRL_COMMON0 0x54 > +#define USB_PHY_REFCLK_CTRL 0xA0 > +#define USB_PHY_HS_PHY_CTRL2 0x64 > +#define USB_PHY_UTMI_CTRL0 0x3c > +#define USB2PHY_USB_PHY_M31_XCFGI_1 0xBC > +#define USB2PHY_USB_PHY_M31_XCFGI_4 0xC8 > +#define USB2PHY_USB_PHY_M31_XCFGI_5 0xCC > +#define USB2PHY_USB_PHY_M31_XCFGI_11 0xE4 Could you sort them address-wise? > + > +#define USB2_0_TX_ENABLE BIT(2) > +#define HSTX_SLEW_RATE_565PS 3 > +#define PLL_CHARGING_PUMP_CURRENT_35UA (3 << 3) > +#define ODT_VALUE_38_02_OHM (3 << 6) > +#define ODT_VALUE_45_02_OHM BIT(2) > +#define HSTX_PRE_EMPHASIS_LEVEL_0_55MA (1) Weird mix of values, bits, bitfields.. perhaps BIT(n) and GENMASK() (+ FIELD_PREP) would be more suitable? > + > +#define UTMI_PHY_OVERRIDE_EN BIT(1) > +#define POR_EN BIT(1) Please associate these with their registers, like #define FOO_REG 0xf00 #define POR_EN BIT(1) > +#define FREQ_SEL BIT(0) > +#define COMMONONN BIT(7) > +#define FSEL BIT(4) > +#define RETENABLEN BIT(3) > +#define USB2_SUSPEND_N_SEL BIT(3) > +#define USB2_SUSPEND_N BIT(2) > +#define USB2_UTMI_CLK_EN BIT(1) > +#define CLKCORE BIT(1) > +#define ATERESET ~BIT(0) > +#define FREQ_24MHZ (5 << 4) > +#define XCFG_COARSE_TUNE_NUM (2 << 0) > +#define XCFG_FINE_TUNE_NUM (1 << 3) same comment > + > +static void m31usb_write_readback(void *base, u32 offset, > + const u32 mask, u32 val); We don't need this forward-definition, just move the function up. > + > +struct m31usb_phy { > + struct usb_phy phy; > + void __iomem *base; > + void __iomem *qscratch_base; > + > + struct reset_control *phy_reset; > + > + bool cable_connected; > + bool suspended; > + bool ulpi_mode; > +}; > + > +static void m31usb_reset(struct m31usb_phy *qphy, u32 action) > +{ > + if (action == CLK_RESET_ASSERT) > + reset_control_assert(qphy->phy_reset); > + else > + reset_control_deassert(qphy->phy_reset); > + wmb(); /* ensure data is written to hw register */ Please move the comment above the call. > +} > + > +static void m31usb_phy_enable_clock(struct m31usb_phy *qphy) > +{ > + /* Enable override ctrl */ > + writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0); Some of the comments are missing a space before '*/' Also, please consider adding some newlines to logically split the actions. > + /* Enable POR*/ > + writel(POR_EN, qphy->base + USB_PHY_UTMI_CTRL5); > + udelay(15); > + /* Configure frequency select value*/ > + writel(FREQ_SEL, qphy->base + USB_PHY_FSEL_SEL); > + /* Configure refclk frequency */ > + writel(COMMONONN | FSEL | RETENABLEN, > + qphy->base + USB_PHY_HS_PHY_CTRL_COMMON0); > + /* select refclk source */ > + writel(CLKCORE, qphy->base + USB_PHY_REFCLK_CTRL); > + /* select ATERESET*/ > + writel(POR_EN & ATERESET, qphy->base + USB_PHY_UTMI_CTRL5); > + writel(USB2_SUSPEND_N_SEL | USB2_SUSPEND_N | USB2_UTMI_CLK_EN, > + qphy->base + USB_PHY_HS_PHY_CTRL2); > + writel(0x0, qphy->base + USB_PHY_UTMI_CTRL5); > + writel(USB2_SUSPEND_N | USB2_UTMI_CLK_EN, > + qphy->base + USB_PHY_HS_PHY_CTRL2); > + /* Disable override ctrl */ > + writel(0x0, qphy->base + USB_PHY_CFG0); > +} > + > +static void ipq5332_m31usb_phy_enable_clock(struct m31usb_phy *qphy) > +{ > + /* Enable override ctrl */ > + writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0); > + /* Enable POR*/ ditto > + writel(POR_EN, qphy->base + USB_PHY_UTMI_CTRL5); > + udelay(15); > + /* Configure frequency select value*/ > + writel(FREQ_SEL, qphy->base + USB_PHY_FSEL_SEL); > + /* Configure refclk frequency */ > + writel(COMMONONN | FREQ_24MHZ | RETENABLEN, > + qphy->base + USB_PHY_HS_PHY_CTRL_COMMON0); > + /* select ATERESET*/ > + writel(POR_EN & ATERESET, qphy->base + USB_PHY_UTMI_CTRL5); > + writel(USB2_SUSPEND_N_SEL | USB2_SUSPEND_N | USB2_UTMI_CLK_EN, > + qphy->base + USB_PHY_HS_PHY_CTRL2); > + writel(XCFG_COARSE_TUNE_NUM | XCFG_FINE_TUNE_NUM, > + qphy->base + USB2PHY_USB_PHY_M31_XCFGI_11); > + /* Adjust HSTX slew rate to 565 ps*/ > + /* Adjust PLL lock Time counter for release clock to 35uA */ > + /* Adjust Manual control ODT value to 38.02 Ohm */ > + writel(HSTX_SLEW_RATE_565PS | PLL_CHARGING_PUMP_CURRENT_35UA | > + ODT_VALUE_38_02_OHM, qphy->base + USB2PHY_USB_PHY_M31_XCFGI_4); These functions seem very similar, with the main difference being that IPQ5332 adds some more writes at the end. Perhaps some commonizing could be done? > + /* > + * Enable to always turn on USB 2.0 TX driver > + * when system is in USB 2.0 HS mode > + */ > + writel(USB2_0_TX_ENABLE, qphy->base + USB2PHY_USB_PHY_M31_XCFGI_1); > + /* Adjust Manual control ODT value to 45.02 Ohm */ > + /* Adjust HSTX Pre-emphasis level to 0.55mA */ > + writel(ODT_VALUE_45_02_OHM | HSTX_PRE_EMPHASIS_LEVEL_0_55MA, > + qphy->base + USB2PHY_USB_PHY_M31_XCFGI_5); > + udelay(4); > + writel(0x0, qphy->base + USB_PHY_UTMI_CTRL5); > + writel(USB2_SUSPEND_N | USB2_UTMI_CLK_EN, > + qphy->base + USB_PHY_HS_PHY_CTRL2); > +} > + > +static int m31usb_phy_init(struct usb_phy *phy) > +{ > + struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy); > + > + /* Perform phy reset */ > + m31usb_reset(qphy, CLK_RESET_ASSERT); > + usleep_range(1, 5); https://www.kernel.org/doc/Documentation/timers/timers-howto.txt this may not work as intended > + m31usb_reset(qphy, CLK_RESET_DEASSERT); > + > + /* configure for ULPI mode if requested */ > + if (qphy->ulpi_mode) > + writel_relaxed(0x0, qphy->base + USB2PHY_PORT_UTMI_CTRL2); > + > + /* Enable the PHY */ > + writel_relaxed(POWER_UP, qphy->base + USB2PHY_PORT_POWERDOWN); > + > + /* Make sure above write completed */ > + wmb(); As you're calling wmb in the reset func, dropping _relaxed from the ULPI and PORT_POWERDOWN writes should work the same > + > + /* Turn on phy ref clock*/ > + if (of_device_is_compatible(phy->dev->of_node, > + "qcom,ipq5332-m31-usb-hsphy")) > + ipq5332_m31usb_phy_enable_clock(qphy); > + else > + m31usb_phy_enable_clock(qphy); This should be done using OF match data. > + > + /* Set OTG VBUS Valid from HSPHY to controller */ > + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG, > + UTMI_OTG_VBUS_VALID, UTMI_OTG_VBUS_VALID); > + /* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */ > + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG, > + SW_SESSVLD_SEL, SW_SESSVLD_SEL); > + > + return 0; > +} > + > +static void m31usb_phy_shutdown(struct usb_phy *phy) > +{ > + struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy); > + > + /* Disable the PHY */ > + writel_relaxed(POWER_DOWN, qphy->base + USB2PHY_PORT_POWERDOWN); > + /* Make sure above write completed */ > + wmb(); > +} > + > +static void m31usb_write_readback(void *base, u32 offset, > + const u32 mask, u32 val) The indentation here makes `const u32..` misaligned. > +{ > + u32 write_val, tmp = readl_relaxed(base + offset); > + > + tmp &= ~mask; /* retain other bits */ > + write_val = tmp | val; > + > + writel_relaxed(write_val, base + offset); > + /* Make sure above write completed */ > + wmb(); > + > + /* Read back to see if val was written */ > + tmp = readl_relaxed(base + offset); > + tmp &= mask; /* clear other bits */ > + > + if (tmp != val) > + pr_err("%s: write: %x to QSCRATCH: %x FAILED\n", > + __func__, val, offset); > +} > + > +static int m31usb_phy_notify_connect(struct usb_phy *phy, > + enum usb_device_speed speed) > +{ > + struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy); > + > + qphy->cable_connected = true; > + > + dev_dbg(phy->dev, " cable_connected=%d\n", qphy->cable_connected); spurious space at the beginning of the string > + > + /* Set OTG VBUS Valid from HSPHY to controller */ > + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG, > + UTMI_OTG_VBUS_VALID, > + UTMI_OTG_VBUS_VALID); Please align the lines with the previous opening bracket > + > + /* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */ > + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG, > + SW_SESSVLD_SEL, SW_SESSVLD_SEL); > + > + dev_dbg(phy->dev, "M31USB2 phy connect notification\n"); > + return 0; > +} > + > +static int m31usb_phy_notify_disconnect(struct usb_phy *phy, > + enum usb_device_speed speed) > +{ > + struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy); > + > + qphy->cable_connected = false; > + > + dev_dbg(phy->dev, " cable_connected=%d\n", qphy->cable_connected); > + > + /* Set OTG VBUS Valid from HSPHY to controller */ > + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG, > + UTMI_OTG_VBUS_VALID, 0); > + > + /* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */ > + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG, > + SW_SESSVLD_SEL, 0); > + > + dev_dbg(phy->dev, "M31USB2 phy disconnect notification\n"); > + return 0; > +} > + > +static int m31usb_phy_probe(struct platform_device *pdev) > +{ > + struct m31usb_phy *qphy; > + struct device *dev = &pdev->dev; > + struct resource *res; > + int ret; > + const char *phy_type; Please sort these in a reverse-Christmas-tree order. > + > + qphy = devm_kzalloc(dev, sizeof(*qphy), GFP_KERNEL); > + if (!qphy) > + return -ENOMEM; > + > + qphy->phy.dev = dev; > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > + "m31usb_phy_base"); > + qphy->base = devm_ioremap_resource(dev, res); devm_platform_get_and_ioremap_resource? > + if (IS_ERR(qphy->base)) > + return PTR_ERR(qphy->base); > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > + "qscratch_base"); > + if (res) { Do we expect platforms without this register space? > + qphy->qscratch_base = devm_ioremap(dev, res->start, > + resource_size(res)); > + if (IS_ERR(qphy->qscratch_base)) { > + dev_dbg(dev, "couldn't ioremap qscratch_base\n"); > + qphy->qscratch_base = NULL; > + } > + } > + > + qphy->phy_reset = devm_reset_control_get(dev, "usb2_phy_reset"); not _exclusive? > + if (IS_ERR(qphy->phy_reset)) > + return PTR_ERR(qphy->phy_reset); > + > + qphy->ulpi_mode = false; > + ret = of_property_read_string(dev->of_node, "phy_type", &phy_type); of_usb_get_phy_mode? > + > + if (!ret) { > + if (!strcasecmp(phy_type, "ulpi")) > + qphy->ulpi_mode = true; > + } else { > + dev_err(dev, "error reading phy_type property\n"); > + return ret; > + } > + > + platform_set_drvdata(pdev, qphy); > + > + qphy->phy.label = "qcom-m31usb-phy"; > + qphy->phy.init = m31usb_phy_init; > + qphy->phy.shutdown = m31usb_phy_shutdown; > + qphy->phy.type = USB_PHY_TYPE_USB2; > + > + if (qphy->qscratch_base) { > + qphy->phy.notify_connect = m31usb_phy_notify_connect; > + qphy->phy.notify_disconnect = m31usb_phy_notify_disconnect; > + } > + > + ret = usb_add_phy_dev(&qphy->phy); > + > + return ret; > +} > + > +static int m31usb_phy_remove(struct platform_device *pdev) Please make this return void and pass it to .remove_new > +{ > + struct m31usb_phy *qphy = platform_get_drvdata(pdev); > + > + usb_remove_phy(&qphy->phy); > + > + return 0; > +} > + > +static const struct of_device_id m31usb_phy_id_table[] = { > + { .compatible = "qcom,m31-usb-hsphy",}, > + { .compatible = "qcom,ipq5332-m31-usb-hsphy",}, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, m31usb_phy_id_table); > + > +static struct platform_driver m31usb_phy_driver = { > + .probe = m31usb_phy_probe, > + .remove = m31usb_phy_remove, > + .driver = { > + .name = "qcom-m31usb-phy", > + .of_match_table = of_match_ptr(m31usb_phy_id_table), of_match_ptr is unnecessary, this driver depends on OF. Konrad > + }, > +}; > + > +module_platform_driver(m31usb_phy_driver); > + > +MODULE_DESCRIPTION("USB2 Qualcomm M31 HSPHY driver"); > +MODULE_LICENSE("GPL");
Two minor nits on top of the review: On 07/06/2023 14:54, Konrad Dybcio wrote: > On 7.06.2023 12:56, Varadarajan Narayanan wrote: >> Add the M31 USB2 phy driver >> >> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> >> --- >> drivers/phy/qualcomm/phy-qcom-m31.c | 360 ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 360 insertions(+) >> create mode 100644 drivers/phy/qualcomm/phy-qcom-m31.c >> >> diff --git a/drivers/phy/qualcomm/phy-qcom-m31.c b/drivers/phy/qualcomm/phy-qcom-m31.c >> new file mode 100644 >> index 0000000..d29a91e >> --- /dev/null >> +++ b/drivers/phy/qualcomm/phy-qcom-m31.c >> @@ -0,0 +1,360 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Copyright (c) 2014-2016, 2020, The Linux Foundation. All rights reserved. >> + */ >> + >> +#include <linux/module.h> >> +#include <linux/kernel.h> >> +#include <linux/err.h> >> +#include <linux/slab.h> >> +#include <linux/clk.h> >> +#include <linux/delay.h> >> +#include <linux/io.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/usb/phy.h> >> +#include <linux/reset.h> >> +#include <linux/of_device.h> > Please sort these > >> + >> +enum clk_reset_action { >> + CLK_RESET_DEASSERT = 0, >> + CLK_RESET_ASSERT = 1 >> +}; >> + >> +#define USB2PHY_PORT_POWERDOWN 0xA4 >> +#define POWER_UP BIT(0) >> +#define POWER_DOWN 0 >> + >> +#define USB2PHY_PORT_UTMI_CTRL1 0x40 >> + >> +#define USB2PHY_PORT_UTMI_CTRL2 0x44 >> +#define UTMI_ULPI_SEL BIT(7) >> +#define UTMI_TEST_MUX_SEL BIT(6) >> + >> +#define HS_PHY_CTRL_REG 0x10 >> +#define UTMI_OTG_VBUS_VALID BIT(20) >> +#define SW_SESSVLD_SEL BIT(28) >> + >> +#define USB_PHY_CFG0 0x94 >> +#define USB_PHY_UTMI_CTRL5 0x50 >> +#define USB_PHY_FSEL_SEL 0xB8 >> +#define USB_PHY_HS_PHY_CTRL_COMMON0 0x54 >> +#define USB_PHY_REFCLK_CTRL 0xA0 >> +#define USB_PHY_HS_PHY_CTRL2 0x64 >> +#define USB_PHY_UTMI_CTRL0 0x3c >> +#define USB2PHY_USB_PHY_M31_XCFGI_1 0xBC >> +#define USB2PHY_USB_PHY_M31_XCFGI_4 0xC8 >> +#define USB2PHY_USB_PHY_M31_XCFGI_5 0xCC >> +#define USB2PHY_USB_PHY_M31_XCFGI_11 0xE4 > Could you sort them address-wise? ... and lowercase the hex values, please. > >> + >> +#define USB2_0_TX_ENABLE BIT(2) >> +#define HSTX_SLEW_RATE_565PS 3 >> +#define PLL_CHARGING_PUMP_CURRENT_35UA (3 << 3) >> +#define ODT_VALUE_38_02_OHM (3 << 6) >> +#define ODT_VALUE_45_02_OHM BIT(2) >> +#define HSTX_PRE_EMPHASIS_LEVEL_0_55MA (1) > Weird mix of values, bits, bitfields.. perhaps BIT(n) and > GENMASK() (+ FIELD_PREP) would be more suitable? > >> + >> +#define UTMI_PHY_OVERRIDE_EN BIT(1) >> +#define POR_EN BIT(1) > Please associate these with their registers, like > > #define FOO_REG 0xf00 > #define POR_EN BIT(1) > >> +#define FREQ_SEL BIT(0) >> +#define COMMONONN BIT(7) >> +#define FSEL BIT(4) >> +#define RETENABLEN BIT(3) >> +#define USB2_SUSPEND_N_SEL BIT(3) >> +#define USB2_SUSPEND_N BIT(2) >> +#define USB2_UTMI_CLK_EN BIT(1) >> +#define CLKCORE BIT(1) >> +#define ATERESET ~BIT(0) >> +#define FREQ_24MHZ (5 << 4) >> +#define XCFG_COARSE_TUNE_NUM (2 << 0) >> +#define XCFG_FINE_TUNE_NUM (1 << 3) > same comment > >> + >> +static void m31usb_write_readback(void *base, u32 offset, >> + const u32 mask, u32 val); > We don't need this forward-definition, just move the function up. > >> + >> +struct m31usb_phy { >> + struct usb_phy phy; >> + void __iomem *base; >> + void __iomem *qscratch_base; >> + >> + struct reset_control *phy_reset; >> + >> + bool cable_connected; >> + bool suspended; >> + bool ulpi_mode; >> +}; >> + >> +static void m31usb_reset(struct m31usb_phy *qphy, u32 action) >> +{ >> + if (action == CLK_RESET_ASSERT) >> + reset_control_assert(qphy->phy_reset); >> + else >> + reset_control_deassert(qphy->phy_reset); >> + wmb(); /* ensure data is written to hw register */ > Please move the comment above the call. > >> +} Or even better just inline the function. I was never a fan of such multiplexers. Also does wmb() make sense here? Doesn't regmap (which is used by reset controller) remove the need for it? >> + >> +static void m31usb_phy_enable_clock(struct m31usb_phy *qphy) >> +{ >> + /* Enable override ctrl */ >> + writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0); > Some of the comments are missing a space before '*/' > > Also, please consider adding some newlines to logically split the > actions.
On 7.06.2023 12:56, Varadarajan Narayanan wrote: > Add USB phy and controller nodes > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > --- > arch/arm64/boot/dts/qcom/ipq5332.dtsi | 55 +++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi > index c2d6cc65..3183357 100644 > --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi > +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi > @@ -383,6 +383,61 @@ > status = "disabled"; > }; > }; > + > + usb_0_m31phy: hs_m31phy@7b000 { > + compatible = "qcom,ipq5332-m31-usb-hsphy"; > + reg = <0x0007b000 0x12C>, random uppercase hex > + <0x08af8800 0x400>; > + reg-names = "m31usb_phy_base", > + "qscratch_base"; > + phy_type= "utmi"; Missing space before '=' > + > + resets = <&gcc GCC_QUSB2_0_PHY_BCR>; > + reset-names = "usb2_phy_reset"; > + > + status = "okay"; If you're only defining the node, it's enabled by default In this case, you'd probably want to disable it by default. > + }; > + > + usb2: usb2@8a00000 { > + compatible = "qcom,ipq5332-dwc3", "qcom,dwc3"; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; Please push these 3 properties to the end And add status = "disabled" below them. > + > + reg = <0x08af8800 0x100>; > + > + clocks = <&gcc GCC_USB0_MASTER_CLK>, > + <&gcc GCC_SNOC_USB_CLK>, > + <&gcc GCC_USB0_SLEEP_CLK>, > + <&gcc GCC_USB0_MOCK_UTMI_CLK>; > + Please remove this newline. > + clock-names = "core", > + "iface", > + "sleep", > + "mock_utmi"; Please align this, and all other similar lists. > + > + interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>; interrupts-extended is unnecessary with just a single interrupt source.. you can make it `interrupts` and drop the GIC reference. It would also be nice to push the interrupt properties below 'reg'. We're working on documenting and automating checking the preferred property order. > + interrupt-names = "pwr_event"; > + > + resets = <&gcc GCC_USB_BCR>; > + > + qcom,select-utmi-as-pipe-clk; > + > + usb2_0_dwc: usb@8a00000 { > + compatible = "snps,dwc3"; > + reg = <0x08a00000 0xe000>; > + clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>; > + clock-names = "ref"; > + interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>; > + usb-phy = <&usb_0_m31phy>; > + tx-fifo-resize; > + snps,is-utmi-l1-suspend; > + snps,hird-threshold = /bits/ 8 <0x0>; > + snps,dis_u2_susphy_quirk; > + snps,dis_u3_susphy_quirk; > + snps,ref-clock-period-ns = <21>; 1/21 is 0.0476.. that doesn't seem to correspond to the ref clk frequency? Konrad > + }; > + }; > }; > > timer {
On 07/06/2023 12:56, Varadarajan Narayanan wrote: > Add USB phy and controller nodes > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > --- > arch/arm64/boot/dts/qcom/ipq5332.dtsi | 55 +++++++++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi > index c2d6cc65..3183357 100644 > --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi > +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi > @@ -383,6 +383,61 @@ > status = "disabled"; > }; > }; > + > + usb_0_m31phy: hs_m31phy@7b000 { Node names should be generic. See also explanation and list of examples in DT specification: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > + compatible = "qcom,ipq5332-m31-usb-hsphy"; > + reg = <0x0007b000 0x12C>, > + <0x08af8800 0x400>; Lowercase hex only. > + reg-names = "m31usb_phy_base", > + "qscratch_base"; > + phy_type= "utmi"; > + > + resets = <&gcc GCC_QUSB2_0_PHY_BCR>; > + reset-names = "usb2_phy_reset"; > + > + status = "okay"; It's by default. Drop. > + }; > + > + usb2: usb2@8a00000 { It does not look like you tested the DTS against bindings. Please run `make dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). Node names should be generic. See also explanation and list of examples in DT specification: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > + compatible = "qcom,ipq5332-dwc3", "qcom,dwc3"; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + reg = <0x08af8800 0x100>; reg is always after compatible. Ranges is third. Then you will spot that address is wrong. > + > + clocks = <&gcc GCC_USB0_MASTER_CLK>, > + <&gcc GCC_SNOC_USB_CLK>, > + <&gcc GCC_USB0_SLEEP_CLK>, > + <&gcc GCC_USB0_MOCK_UTMI_CLK>; Fix alignment. > + > + clock-names = "core", > + "iface", > + "sleep", > + "mock_utmi"; Fix alignment. > + > + interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "pwr_event"; > + Best regards, Krzysztof
On 07/06/2023 12:56, Varadarajan Narayanan wrote:
> Enable QCOM M31 USB phy driver present in IPQ5332
What is "QCOM"? If acronym, extend. IPQ5332 - provide full name, so
"Qualcomm IPQ....".
Best regards,
Krzysztof
On 07/06/2023 12:56, Varadarajan Narayanan wrote: > Include M31 phy driver if CONFIG_PHY_QCOM_M31_USB is enabled > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> That's not a separate commit. Best regards, Krzysztof
On 07/06/2023 13:20, Dmitry Baryshkov wrote: > On 07/06/2023 13:56, Varadarajan Narayanan wrote: >> Introduce CONFIG_PHY_QCOM_M31_USB for including the M31 phy driver >> >> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > > Is there any reason to keep Kconfig, Makefile and driver in different > commits? KPI? Yearly objectives/goals? Best regards, Krzysztof
On Wed, Jun 07, 2023 at 01:54:23PM +0200, Konrad Dybcio wrote: > > > On 7.06.2023 12:56, Varadarajan Narayanan wrote: > > Add the M31 USB2 phy driver > > > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > > --- > > drivers/phy/qualcomm/phy-qcom-m31.c | 360 ++++++++++++++++++++++++++++++++++++ > > 1 file changed, 360 insertions(+) > > create mode 100644 drivers/phy/qualcomm/phy-qcom-m31.c > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-m31.c b/drivers/phy/qualcomm/phy-qcom-m31.c > > new file mode 100644 > > index 0000000..d29a91e > > --- /dev/null > > +++ b/drivers/phy/qualcomm/phy-qcom-m31.c > > @@ -0,0 +1,360 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright (c) 2014-2016, 2020, The Linux Foundation. All rights reserved. > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/kernel.h> > > +#include <linux/err.h> > > +#include <linux/slab.h> > > +#include <linux/clk.h> > > +#include <linux/delay.h> > > +#include <linux/io.h> > > +#include <linux/of.h> > > +#include <linux/platform_device.h> > > +#include <linux/usb/phy.h> > > +#include <linux/reset.h> > > +#include <linux/of_device.h> > Please sort these Ok. > > + > > +enum clk_reset_action { > > + CLK_RESET_DEASSERT = 0, > > + CLK_RESET_ASSERT = 1 > > +}; > > + > > +#define USB2PHY_PORT_POWERDOWN 0xA4 > > +#define POWER_UP BIT(0) > > +#define POWER_DOWN 0 > > + > > +#define USB2PHY_PORT_UTMI_CTRL1 0x40 > > + > > +#define USB2PHY_PORT_UTMI_CTRL2 0x44 > > +#define UTMI_ULPI_SEL BIT(7) > > +#define UTMI_TEST_MUX_SEL BIT(6) > > + > > +#define HS_PHY_CTRL_REG 0x10 > > +#define UTMI_OTG_VBUS_VALID BIT(20) > > +#define SW_SESSVLD_SEL BIT(28) > > + > > +#define USB_PHY_CFG0 0x94 > > +#define USB_PHY_UTMI_CTRL5 0x50 > > +#define USB_PHY_FSEL_SEL 0xB8 > > +#define USB_PHY_HS_PHY_CTRL_COMMON0 0x54 > > +#define USB_PHY_REFCLK_CTRL 0xA0 > > +#define USB_PHY_HS_PHY_CTRL2 0x64 > > +#define USB_PHY_UTMI_CTRL0 0x3c > > +#define USB2PHY_USB_PHY_M31_XCFGI_1 0xBC > > +#define USB2PHY_USB_PHY_M31_XCFGI_4 0xC8 > > +#define USB2PHY_USB_PHY_M31_XCFGI_5 0xCC > > +#define USB2PHY_USB_PHY_M31_XCFGI_11 0xE4 > Could you sort them address-wise? Ok. > > + > > +#define USB2_0_TX_ENABLE BIT(2) > > +#define HSTX_SLEW_RATE_565PS 3 > > +#define PLL_CHARGING_PUMP_CURRENT_35UA (3 << 3) > > +#define ODT_VALUE_38_02_OHM (3 << 6) > > +#define ODT_VALUE_45_02_OHM BIT(2) > > +#define HSTX_PRE_EMPHASIS_LEVEL_0_55MA (1) > Weird mix of values, bits, bitfields.. perhaps BIT(n) and > GENMASK() (+ FIELD_PREP) would be more suitable? Ok. > > + > > +#define UTMI_PHY_OVERRIDE_EN BIT(1) > > +#define POR_EN BIT(1) > Please associate these with their registers, like Ok. > #define FOO_REG 0xf00 > #define POR_EN BIT(1) > > > +#define FREQ_SEL BIT(0) > > +#define COMMONONN BIT(7) > > +#define FSEL BIT(4) > > +#define RETENABLEN BIT(3) > > +#define USB2_SUSPEND_N_SEL BIT(3) > > +#define USB2_SUSPEND_N BIT(2) > > +#define USB2_UTMI_CLK_EN BIT(1) > > +#define CLKCORE BIT(1) > > +#define ATERESET ~BIT(0) > > +#define FREQ_24MHZ (5 << 4) > > +#define XCFG_COARSE_TUNE_NUM (2 << 0) > > +#define XCFG_FINE_TUNE_NUM (1 << 3) > same comment Ok. > > + > > +static void m31usb_write_readback(void *base, u32 offset, > > + const u32 mask, u32 val); > We don't need this forward-definition, just move the function up. > > > + > > +struct m31usb_phy { > > + struct usb_phy phy; > > + void __iomem *base; > > + void __iomem *qscratch_base; > > + > > + struct reset_control *phy_reset; > > + > > + bool cable_connected; > > + bool suspended; > > + bool ulpi_mode; > > +}; > > + > > +static void m31usb_reset(struct m31usb_phy *qphy, u32 action) > > +{ > > + if (action == CLK_RESET_ASSERT) > > + reset_control_assert(qphy->phy_reset); > > + else > > + reset_control_deassert(qphy->phy_reset); > > + wmb(); /* ensure data is written to hw register */ > Please move the comment above the call. This is used only once. Hence, will move it to the calling location itself. > > +} > > + > > +static void m31usb_phy_enable_clock(struct m31usb_phy *qphy) > > +{ > > + /* Enable override ctrl */ > > + writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0); > Some of the comments are missing a space before '*/' > > Also, please consider adding some newlines to logically split the > actions. > > > + /* Enable POR*/ > > + writel(POR_EN, qphy->base + USB_PHY_UTMI_CTRL5); > > + udelay(15); > > + /* Configure frequency select value*/ > > + writel(FREQ_SEL, qphy->base + USB_PHY_FSEL_SEL); > > + /* Configure refclk frequency */ > > + writel(COMMONONN | FSEL | RETENABLEN, > > + qphy->base + USB_PHY_HS_PHY_CTRL_COMMON0); > > + /* select refclk source */ > > + writel(CLKCORE, qphy->base + USB_PHY_REFCLK_CTRL); > > + /* select ATERESET*/ > > + writel(POR_EN & ATERESET, qphy->base + USB_PHY_UTMI_CTRL5); > > + writel(USB2_SUSPEND_N_SEL | USB2_SUSPEND_N | USB2_UTMI_CLK_EN, > > + qphy->base + USB_PHY_HS_PHY_CTRL2); > > + writel(0x0, qphy->base + USB_PHY_UTMI_CTRL5); > > + writel(USB2_SUSPEND_N | USB2_UTMI_CLK_EN, > > + qphy->base + USB_PHY_HS_PHY_CTRL2); > > + /* Disable override ctrl */ > > + writel(0x0, qphy->base + USB_PHY_CFG0); > > +} > > + > > +static void ipq5332_m31usb_phy_enable_clock(struct m31usb_phy *qphy) > > +{ > > + /* Enable override ctrl */ > > + writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0); > > + /* Enable POR*/ > ditto > > > + writel(POR_EN, qphy->base + USB_PHY_UTMI_CTRL5); > > + udelay(15); > > + /* Configure frequency select value*/ > > + writel(FREQ_SEL, qphy->base + USB_PHY_FSEL_SEL); > > + /* Configure refclk frequency */ > > + writel(COMMONONN | FREQ_24MHZ | RETENABLEN, > > + qphy->base + USB_PHY_HS_PHY_CTRL_COMMON0); > > + /* select ATERESET*/ > > + writel(POR_EN & ATERESET, qphy->base + USB_PHY_UTMI_CTRL5); > > + writel(USB2_SUSPEND_N_SEL | USB2_SUSPEND_N | USB2_UTMI_CLK_EN, > > + qphy->base + USB_PHY_HS_PHY_CTRL2); > > + writel(XCFG_COARSE_TUNE_NUM | XCFG_FINE_TUNE_NUM, > > + qphy->base + USB2PHY_USB_PHY_M31_XCFGI_11); > > + /* Adjust HSTX slew rate to 565 ps*/ > > + /* Adjust PLL lock Time counter for release clock to 35uA */ > > + /* Adjust Manual control ODT value to 38.02 Ohm */ > > + writel(HSTX_SLEW_RATE_565PS | PLL_CHARGING_PUMP_CURRENT_35UA | > > + ODT_VALUE_38_02_OHM, qphy->base + USB2PHY_USB_PHY_M31_XCFGI_4); > These functions seem very similar, with the main difference being that > IPQ5332 adds some more writes at the end. Perhaps some commonizing could > be done? > > > + /* > > + * Enable to always turn on USB 2.0 TX driver > > + * when system is in USB 2.0 HS mode > > + */ > > + writel(USB2_0_TX_ENABLE, qphy->base + USB2PHY_USB_PHY_M31_XCFGI_1); > > + /* Adjust Manual control ODT value to 45.02 Ohm */ > > + /* Adjust HSTX Pre-emphasis level to 0.55mA */ > > + writel(ODT_VALUE_45_02_OHM | HSTX_PRE_EMPHASIS_LEVEL_0_55MA, > > + qphy->base + USB2PHY_USB_PHY_M31_XCFGI_5); > > + udelay(4); > > + writel(0x0, qphy->base + USB_PHY_UTMI_CTRL5); > > + writel(USB2_SUSPEND_N | USB2_UTMI_CLK_EN, > > + qphy->base + USB_PHY_HS_PHY_CTRL2); > > +} Will change the above to table based register init, based on compatible/data. > > + > > +static int m31usb_phy_init(struct usb_phy *phy) > > +{ > > + struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy); > > + > > + /* Perform phy reset */ > > + m31usb_reset(qphy, CLK_RESET_ASSERT); > > + usleep_range(1, 5); > https://www.kernel.org/doc/Documentation/timers/timers-howto.txt > > this may not work as intended Will change it to udelay(5). > > + m31usb_reset(qphy, CLK_RESET_DEASSERT); > > + > > + /* configure for ULPI mode if requested */ > > + if (qphy->ulpi_mode) > > + writel_relaxed(0x0, qphy->base + USB2PHY_PORT_UTMI_CTRL2); > > + > > + /* Enable the PHY */ > > + writel_relaxed(POWER_UP, qphy->base + USB2PHY_PORT_POWERDOWN); > > + > > + /* Make sure above write completed */ > > + wmb(); > As you're calling wmb in the reset func, dropping _relaxed from the > ULPI and PORT_POWERDOWN writes should work the same Ok. > > + > > + /* Turn on phy ref clock*/ > > + if (of_device_is_compatible(phy->dev->of_node, > > + "qcom,ipq5332-m31-usb-hsphy")) > > + ipq5332_m31usb_phy_enable_clock(qphy); > > + else > > + m31usb_phy_enable_clock(qphy); > This should be done using OF match data. Ok. > > + > > + /* Set OTG VBUS Valid from HSPHY to controller */ > > + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG, > > + UTMI_OTG_VBUS_VALID, UTMI_OTG_VBUS_VALID); > > + /* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */ > > + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG, > > + SW_SESSVLD_SEL, SW_SESSVLD_SEL); > > + > > + return 0; > > +} > > + > > +static void m31usb_phy_shutdown(struct usb_phy *phy) > > +{ > > + struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy); > > + > > + /* Disable the PHY */ > > + writel_relaxed(POWER_DOWN, qphy->base + USB2PHY_PORT_POWERDOWN); > > + /* Make sure above write completed */ > > + wmb(); > > +} > > + > > +static void m31usb_write_readback(void *base, u32 offset, > > + const u32 mask, u32 val) > The indentation here makes `const u32..` misaligned. > > > +{ > > + u32 write_val, tmp = readl_relaxed(base + offset); > > + > > + tmp &= ~mask; /* retain other bits */ > > + write_val = tmp | val; > > + > > + writel_relaxed(write_val, base + offset); > > + /* Make sure above write completed */ > > + wmb(); > > + > > + /* Read back to see if val was written */ > > + tmp = readl_relaxed(base + offset); > > + tmp &= mask; /* clear other bits */ > > + > > + if (tmp != val) > > + pr_err("%s: write: %x to QSCRATCH: %x FAILED\n", > > + __func__, val, offset); > > +} > > + > > +static int m31usb_phy_notify_connect(struct usb_phy *phy, > > + enum usb_device_speed speed) > > +{ > > + struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy); > > + > > + qphy->cable_connected = true; > > + > > + dev_dbg(phy->dev, " cable_connected=%d\n", qphy->cable_connected); > spurious space at the beginning of the string > > > + > > + /* Set OTG VBUS Valid from HSPHY to controller */ > > + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG, > > + UTMI_OTG_VBUS_VALID, > > + UTMI_OTG_VBUS_VALID); > Please align the lines with the previous opening bracket > > > + > > + /* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */ > > + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG, > > + SW_SESSVLD_SEL, SW_SESSVLD_SEL); > > + > > + dev_dbg(phy->dev, "M31USB2 phy connect notification\n"); > > + return 0; > > +} > > + > > +static int m31usb_phy_notify_disconnect(struct usb_phy *phy, > > + enum usb_device_speed speed) > > +{ > > + struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy); > > + > > + qphy->cable_connected = false; > > + > > + dev_dbg(phy->dev, " cable_connected=%d\n", qphy->cable_connected); > > + > > + /* Set OTG VBUS Valid from HSPHY to controller */ > > + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG, > > + UTMI_OTG_VBUS_VALID, 0); > > + > > + /* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */ > > + m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG, > > + SW_SESSVLD_SEL, 0); > > + > > + dev_dbg(phy->dev, "M31USB2 phy disconnect notification\n"); > > + return 0; > > +} Will remove these functions. They are accessing 'qscratch' area that is part of the controller space. It doesn't belong in this driver. > > +static int m31usb_phy_probe(struct platform_device *pdev) > > +{ > > + struct m31usb_phy *qphy; > > + struct device *dev = &pdev->dev; > > + struct resource *res; > > + int ret; > > + const char *phy_type; > Please sort these in a reverse-Christmas-tree order. ok. > > + > > + qphy = devm_kzalloc(dev, sizeof(*qphy), GFP_KERNEL); > > + if (!qphy) > > + return -ENOMEM; > > + > > + qphy->phy.dev = dev; > > + > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > + "m31usb_phy_base"); > > + qphy->base = devm_ioremap_resource(dev, res); > devm_platform_get_and_ioremap_resource? ok. > > + if (IS_ERR(qphy->base)) > > + return PTR_ERR(qphy->base); > > + > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > + "qscratch_base"); > > + if (res) { > Do we expect platforms without this register space? > > > + qphy->qscratch_base = devm_ioremap(dev, res->start, > > + resource_size(res)); > > + if (IS_ERR(qphy->qscratch_base)) { > > + dev_dbg(dev, "couldn't ioremap qscratch_base\n"); > > + qphy->qscratch_base = NULL; > > + } > > + } Will remove this qscratch code. > > + qphy->phy_reset = devm_reset_control_get(dev, "usb2_phy_reset"); > not _exclusive? Ok. > > + if (IS_ERR(qphy->phy_reset)) > > + return PTR_ERR(qphy->phy_reset); > > + > > + qphy->ulpi_mode = false; > > + ret = of_property_read_string(dev->of_node, "phy_type", &phy_type); > of_usb_get_phy_mode? Ok. > > + > > + if (!ret) { > > + if (!strcasecmp(phy_type, "ulpi")) > > + qphy->ulpi_mode = true; > > + } else { > > + dev_err(dev, "error reading phy_type property\n"); > > + return ret; > > + } > > + > > + platform_set_drvdata(pdev, qphy); > > + > > + qphy->phy.label = "qcom-m31usb-phy"; > > + qphy->phy.init = m31usb_phy_init; > > + qphy->phy.shutdown = m31usb_phy_shutdown; > > + qphy->phy.type = USB_PHY_TYPE_USB2; > > + > > + if (qphy->qscratch_base) { > > + qphy->phy.notify_connect = m31usb_phy_notify_connect; > > + qphy->phy.notify_disconnect = m31usb_phy_notify_disconnect; > > + } > > + > > + ret = usb_add_phy_dev(&qphy->phy); > > + > > + return ret; > > +} > > + > > +static int m31usb_phy_remove(struct platform_device *pdev) > Please make this return void and pass it to .remove_new Ok. > > +{ > > + struct m31usb_phy *qphy = platform_get_drvdata(pdev); > > + > > + usb_remove_phy(&qphy->phy); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id m31usb_phy_id_table[] = { > > + { .compatible = "qcom,m31-usb-hsphy",}, > > + { .compatible = "qcom,ipq5332-m31-usb-hsphy",}, > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(of, m31usb_phy_id_table); > > + > > +static struct platform_driver m31usb_phy_driver = { > > + .probe = m31usb_phy_probe, > > + .remove = m31usb_phy_remove, > > + .driver = { > > + .name = "qcom-m31usb-phy", > > + .of_match_table = of_match_ptr(m31usb_phy_id_table), > of_match_ptr is unnecessary, this driver depends on OF. Ok. Thanks Varada > Konrad > > + }, > > +}; > > + > > +module_platform_driver(m31usb_phy_driver); > > + > > +MODULE_DESCRIPTION("USB2 Qualcomm M31 HSPHY driver"); > > +MODULE_LICENSE("GPL");
On Wed, Jun 07, 2023 at 03:29:18PM +0300, Dmitry Baryshkov wrote: > Two minor nits on top of the review: > > On 07/06/2023 14:54, Konrad Dybcio wrote: > >On 7.06.2023 12:56, Varadarajan Narayanan wrote: > >>Add the M31 USB2 phy driver > >> > >>Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > >>--- > >> drivers/phy/qualcomm/phy-qcom-m31.c | 360 ++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 360 insertions(+) > >> create mode 100644 drivers/phy/qualcomm/phy-qcom-m31.c > >> > >>diff --git a/drivers/phy/qualcomm/phy-qcom-m31.c b/drivers/phy/qualcomm/phy-qcom-m31.c > >>new file mode 100644 > >>index 0000000..d29a91e > >>--- /dev/null > >>+++ b/drivers/phy/qualcomm/phy-qcom-m31.c > >>@@ -0,0 +1,360 @@ > >>+// SPDX-License-Identifier: GPL-2.0+ > >>+/* > >>+ * Copyright (c) 2014-2016, 2020, The Linux Foundation. All rights reserved. > >>+ */ > >>+ > >>+#include <linux/module.h> > >>+#include <linux/kernel.h> > >>+#include <linux/err.h> > >>+#include <linux/slab.h> > >>+#include <linux/clk.h> > >>+#include <linux/delay.h> > >>+#include <linux/io.h> > >>+#include <linux/of.h> > >>+#include <linux/platform_device.h> > >>+#include <linux/usb/phy.h> > >>+#include <linux/reset.h> > >>+#include <linux/of_device.h> > >Please sort these > > > >>+ > >>+enum clk_reset_action { > >>+ CLK_RESET_DEASSERT = 0, > >>+ CLK_RESET_ASSERT = 1 > >>+}; > >>+ > >>+#define USB2PHY_PORT_POWERDOWN 0xA4 > >>+#define POWER_UP BIT(0) > >>+#define POWER_DOWN 0 > >>+ > >>+#define USB2PHY_PORT_UTMI_CTRL1 0x40 > >>+ > >>+#define USB2PHY_PORT_UTMI_CTRL2 0x44 > >>+#define UTMI_ULPI_SEL BIT(7) > >>+#define UTMI_TEST_MUX_SEL BIT(6) > >>+ > >>+#define HS_PHY_CTRL_REG 0x10 > >>+#define UTMI_OTG_VBUS_VALID BIT(20) > >>+#define SW_SESSVLD_SEL BIT(28) > >>+ > >>+#define USB_PHY_CFG0 0x94 > >>+#define USB_PHY_UTMI_CTRL5 0x50 > >>+#define USB_PHY_FSEL_SEL 0xB8 > >>+#define USB_PHY_HS_PHY_CTRL_COMMON0 0x54 > >>+#define USB_PHY_REFCLK_CTRL 0xA0 > >>+#define USB_PHY_HS_PHY_CTRL2 0x64 > >>+#define USB_PHY_UTMI_CTRL0 0x3c > >>+#define USB2PHY_USB_PHY_M31_XCFGI_1 0xBC > >>+#define USB2PHY_USB_PHY_M31_XCFGI_4 0xC8 > >>+#define USB2PHY_USB_PHY_M31_XCFGI_5 0xCC > >>+#define USB2PHY_USB_PHY_M31_XCFGI_11 0xE4 > >Could you sort them address-wise? > > ... and lowercase the hex values, please. Ok. > >>+ > >>+#define USB2_0_TX_ENABLE BIT(2) > >>+#define HSTX_SLEW_RATE_565PS 3 > >>+#define PLL_CHARGING_PUMP_CURRENT_35UA (3 << 3) > >>+#define ODT_VALUE_38_02_OHM (3 << 6) > >>+#define ODT_VALUE_45_02_OHM BIT(2) > >>+#define HSTX_PRE_EMPHASIS_LEVEL_0_55MA (1) > >Weird mix of values, bits, bitfields.. perhaps BIT(n) and > >GENMASK() (+ FIELD_PREP) would be more suitable? > > > >>+ > >>+#define UTMI_PHY_OVERRIDE_EN BIT(1) > >>+#define POR_EN BIT(1) > >Please associate these with their registers, like > > > >#define FOO_REG 0xf00 > > #define POR_EN BIT(1) > > > >>+#define FREQ_SEL BIT(0) > >>+#define COMMONONN BIT(7) > >>+#define FSEL BIT(4) > >>+#define RETENABLEN BIT(3) > >>+#define USB2_SUSPEND_N_SEL BIT(3) > >>+#define USB2_SUSPEND_N BIT(2) > >>+#define USB2_UTMI_CLK_EN BIT(1) > >>+#define CLKCORE BIT(1) > >>+#define ATERESET ~BIT(0) > >>+#define FREQ_24MHZ (5 << 4) > >>+#define XCFG_COARSE_TUNE_NUM (2 << 0) > >>+#define XCFG_FINE_TUNE_NUM (1 << 3) > >same comment > > > >>+ > >>+static void m31usb_write_readback(void *base, u32 offset, > >>+ const u32 mask, u32 val); > >We don't need this forward-definition, just move the function up. > > > >>+ > >>+struct m31usb_phy { > >>+ struct usb_phy phy; > >>+ void __iomem *base; > >>+ void __iomem *qscratch_base; > >>+ > >>+ struct reset_control *phy_reset; > >>+ > >>+ bool cable_connected; > >>+ bool suspended; > >>+ bool ulpi_mode; > >>+}; > >>+ > >>+static void m31usb_reset(struct m31usb_phy *qphy, u32 action) > >>+{ > >>+ if (action == CLK_RESET_ASSERT) > >>+ reset_control_assert(qphy->phy_reset); > >>+ else > >>+ reset_control_deassert(qphy->phy_reset); > >>+ wmb(); /* ensure data is written to hw register */ > >Please move the comment above the call. > > > >>+} > > Or even better just inline the function. I was never a fan of such > multiplexers. > > Also does wmb() make sense here? Doesn't regmap (which is used by reset > controller) remove the need for it? Will inline and remove the wmb. Thanks Varada > >>+ > >>+static void m31usb_phy_enable_clock(struct m31usb_phy *qphy) > >>+{ > >>+ /* Enable override ctrl */ > >>+ writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0); > >Some of the comments are missing a space before '*/' > > > >Also, please consider adding some newlines to logically split the > >actions. > > > -- > With best wishes > Dmitry >
On Wed, Jun 07, 2023 at 02:19:09PM +0300, Dmitry Baryshkov wrote: > On 07/06/2023 13:56, Varadarajan Narayanan wrote: > >Fix the USB related clock defines and add details > >referenced by them > > > >Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > >--- > > drivers/clk/qcom/gcc-ipq5332.c | 34 +++++++++++++++++++++++----------- > > 1 file changed, 23 insertions(+), 11 deletions(-) > > > >diff --git a/drivers/clk/qcom/gcc-ipq5332.c b/drivers/clk/qcom/gcc-ipq5332.c > >index a75ab88..2b58558 100644 > >--- a/drivers/clk/qcom/gcc-ipq5332.c > >+++ b/drivers/clk/qcom/gcc-ipq5332.c > >@@ -351,6 +351,16 @@ static const struct freq_tbl ftbl_gcc_adss_pwm_clk_src[] = { > > { } > > }; > >+static const struct clk_parent_data gcc_usb3phy_0_cc_pipe_clk_xo[] = { > >+ { .fw_name = "usb3phy_0_cc_pipe_clk" }, > >+ { .fw_name = "xo" }, > > gcc-ipq5332 uses DT indices, please don't mix that with .fw_name. > > >+}; > >+ > >+static const struct parent_map gcc_usb3phy_0_cc_pipe_clk_xo_map[] = { > >+ { P_USB3PHY_0_PIPE, 0 }, > >+ { P_XO, 2 }, > >+}; > >+ > > static struct clk_rcg2 gcc_adss_pwm_clk_src = { > > .cmd_rcgr = 0x1c004, > > .mnd_width = 0, > >@@ -1101,16 +1111,18 @@ static struct clk_rcg2 gcc_usb0_mock_utmi_clk_src = { > > }, > > }; > >-static struct clk_regmap_phy_mux gcc_usb0_pipe_clk_src = { > >+static struct clk_regmap_mux usb0_pipe_clk_src = { > > .reg = 0x2c074, > >+ .shift = 8, > >+ .width = 2, > >+ .parent_map = gcc_usb3phy_0_cc_pipe_clk_xo_map, > > .clkr = { > >- .hw.init = &(struct clk_init_data) { > >- .name = "gcc_usb0_pipe_clk_src", > >- .parent_data = &(const struct clk_parent_data) { > >- .index = DT_USB_PCIE_WRAPPER_PIPE_CLK, > >- }, > >- .num_parents = 1, > >- .ops = &clk_regmap_phy_mux_ops, > >+ .hw.init = &(const struct clk_init_data){ > >+ .name = "usb0phy_0_cc_pipe_clk_src", > >+ .parent_data = gcc_usb3phy_0_cc_pipe_clk_xo, > >+ .num_parents = 2, > >+ .ops = &clk_regmap_mux_closest_ops, > >+ .flags = CLK_SET_RATE_PARENT, > > }, > > Soo... As you are reverting this. Is USB0 PIPE clock required to be parked > to the XO? I was going to write 'before turning USB0_GDSC' off, but then I > noticed that gcc-ipq5332 doesn't declare GDSCs. Does this platform have > GDSCs? > > > }, > > }; > >@@ -3041,8 +3053,8 @@ static struct clk_branch gcc_usb0_pipe_clk = { > > .enable_mask = BIT(0), > > .hw.init = &(const struct clk_init_data) { > > .name = "gcc_usb0_pipe_clk", > >- .parent_hws = (const struct clk_hw*[]) { > >- &gcc_usb0_pipe_clk_src.clkr.hw, > >+ .parent_names = (const char *[]){ > >+ "usb0_pipe_clk_src" > > complete and definitive NAK. Do not use parent_names, we have just stopped > migrating from them. Will drop changes to this file and post a new patch. Thanks Varada > > }, > > .num_parents = 1, > > .flags = CLK_SET_RATE_PARENT, > >@@ -3580,7 +3592,7 @@ static struct clk_regmap *gcc_ipq5332_clocks[] = { > > [GCC_PCIE3X2_PIPE_CLK_SRC] = &gcc_pcie3x2_pipe_clk_src.clkr, > > [GCC_PCIE3X1_0_PIPE_CLK_SRC] = &gcc_pcie3x1_0_pipe_clk_src.clkr, > > [GCC_PCIE3X1_1_PIPE_CLK_SRC] = &gcc_pcie3x1_1_pipe_clk_src.clkr, > >- [GCC_USB0_PIPE_CLK_SRC] = &gcc_usb0_pipe_clk_src.clkr, > >+ [GCC_USB0_PIPE_CLK_SRC] = &usb0_pipe_clk_src.clkr, > > }; > > static const struct qcom_reset_map gcc_ipq5332_resets[] = { > > -- > With best wishes > Dmitry >
On Wed, Jun 07, 2023 at 08:37:05PM +0200, Krzysztof Kozlowski wrote: > On 07/06/2023 13:20, Dmitry Baryshkov wrote: > > On 07/06/2023 13:56, Varadarajan Narayanan wrote: > >> Introduce CONFIG_PHY_QCOM_M31_USB for including the M31 phy driver > >> > >> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > > > > Is there any reason to keep Kconfig, Makefile and driver in different > > commits? > > KPI? Yearly objectives/goals? No :-). Was not sure, hence split them. Will combine in the next version. Thanks Varada
On Wed, Jun 07, 2023 at 08:36:45PM +0200, Krzysztof Kozlowski wrote: > On 07/06/2023 12:56, Varadarajan Narayanan wrote: > > Include M31 phy driver if CONFIG_PHY_QCOM_M31_USB is enabled > > > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > > That's not a separate commit. Will combine with the driver and post new version. Thanks Varada > Best regards, > Krzysztof >
On Wed, Jun 07, 2023 at 02:23:20PM +0300, Dmitry Baryshkov wrote: > On 07/06/2023 13:56, Varadarajan Narayanan wrote: > >Add USB phy and controller nodes > > > >Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > >--- > > arch/arm64/boot/dts/qcom/ipq5332.dtsi | 55 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 55 insertions(+) > > > >diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi > >index c2d6cc65..3183357 100644 > >--- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi > >+++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi > >@@ -383,6 +383,61 @@ > > status = "disabled"; > > }; > > }; > >+ > >+ usb_0_m31phy: hs_m31phy@7b000 { > >+ compatible = "qcom,ipq5332-m31-usb-hsphy"; > >+ reg = <0x0007b000 0x12C>, > >+ <0x08af8800 0x400>; > >+ reg-names = "m31usb_phy_base", > >+ "qscratch_base"; > >+ phy_type= "utmi"; > >+ > >+ resets = <&gcc GCC_QUSB2_0_PHY_BCR>; > >+ reset-names = "usb2_phy_reset"; > >+ > >+ status = "okay"; > >+ }; > >+ > >+ usb2: usb2@8a00000 { > >+ compatible = "qcom,ipq5332-dwc3", "qcom,dwc3"; > >+ #address-cells = <1>; > >+ #size-cells = <1>; > >+ ranges; > >+ > >+ reg = <0x08af8800 0x100>; > >+ > >+ clocks = <&gcc GCC_USB0_MASTER_CLK>, > >+ <&gcc GCC_SNOC_USB_CLK>, > >+ <&gcc GCC_USB0_SLEEP_CLK>, > >+ <&gcc GCC_USB0_MOCK_UTMI_CLK>; > > Please indent these values. Ok. > >+ > >+ clock-names = "core", > >+ "iface", > >+ "sleep", > >+ "mock_utmi"; > > Please indent these values. Ok. > >+ > >+ interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>; > > No PHY IRQs? Will add. Thanks Varada > >+ interrupt-names = "pwr_event"; > >+ > >+ resets = <&gcc GCC_USB_BCR>; > >+ > >+ qcom,select-utmi-as-pipe-clk; > >+ > >+ usb2_0_dwc: usb@8a00000 { > >+ compatible = "snps,dwc3"; > >+ reg = <0x08a00000 0xe000>; > >+ clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>; > >+ clock-names = "ref"; > >+ interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>; > >+ usb-phy = <&usb_0_m31phy>; > >+ tx-fifo-resize; > >+ snps,is-utmi-l1-suspend; > >+ snps,hird-threshold = /bits/ 8 <0x0>; > >+ snps,dis_u2_susphy_quirk; > >+ snps,dis_u3_susphy_quirk; > >+ snps,ref-clock-period-ns = <21>; > >+ }; > >+ }; > > }; > > timer { > > -- > With best wishes > Dmitry >
On Wed, Jun 07, 2023 at 08:24:04PM +0200, Konrad Dybcio wrote: > > > On 7.06.2023 12:56, Varadarajan Narayanan wrote: > > Add USB phy and controller nodes > > > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > > --- > > arch/arm64/boot/dts/qcom/ipq5332.dtsi | 55 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 55 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi > > index c2d6cc65..3183357 100644 > > --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi > > +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi > > @@ -383,6 +383,61 @@ > > status = "disabled"; > > }; > > }; > > + > > + usb_0_m31phy: hs_m31phy@7b000 { > > + compatible = "qcom,ipq5332-m31-usb-hsphy"; > > + reg = <0x0007b000 0x12C>, > random uppercase hex Ok. > > + <0x08af8800 0x400>; > > + reg-names = "m31usb_phy_base", > > + "qscratch_base"; > > + phy_type= "utmi"; > Missing space before '=' Ok. > > + > > + resets = <&gcc GCC_QUSB2_0_PHY_BCR>; > > + reset-names = "usb2_phy_reset"; > > + > > + status = "okay"; > If you're only defining the node, it's enabled by default > > In this case, you'd probably want to disable it by default. Ok. > > + }; > > + > > + usb2: usb2@8a00000 { > > + compatible = "qcom,ipq5332-dwc3", "qcom,dwc3"; > > + #address-cells = <1>; > > + #size-cells = <1>; > > + ranges; > Please push these 3 properties to the end > > And add status = "disabled" below them. Ok. > > + > > + reg = <0x08af8800 0x100>; > > + > > + clocks = <&gcc GCC_USB0_MASTER_CLK>, > > + <&gcc GCC_SNOC_USB_CLK>, > > + <&gcc GCC_USB0_SLEEP_CLK>, > > + <&gcc GCC_USB0_MOCK_UTMI_CLK>; > > + > Please remove this newline. > > > + clock-names = "core", > > + "iface", > > + "sleep", > > + "mock_utmi"; > Please align this, and all other similar lists. Ok. > > + > > + interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>; > interrupts-extended is unnecessary with just a single interrupt > source.. you can make it `interrupts` and drop the GIC reference. > > It would also be nice to push the interrupt properties below 'reg'. > We're working on documenting and automating checking the preferred > property order. Ok. > > + interrupt-names = "pwr_event"; > > + > > + resets = <&gcc GCC_USB_BCR>; > > + > > + qcom,select-utmi-as-pipe-clk; > > + > > + usb2_0_dwc: usb@8a00000 { > > + compatible = "snps,dwc3"; > > + reg = <0x08a00000 0xe000>; > > + clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>; > > + clock-names = "ref"; > > + interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>; > > + usb-phy = <&usb_0_m31phy>; > > + tx-fifo-resize; > > + snps,is-utmi-l1-suspend; > > + snps,hird-threshold = /bits/ 8 <0x0>; > > + snps,dis_u2_susphy_quirk; > > + snps,dis_u3_susphy_quirk; > > + snps,ref-clock-period-ns = <21>; > 1/21 is 0.0476.. that doesn't seem to correspond to the ref > clk frequency? dwc3_ref_clk_period() prefers ref clock's rate over ref-clock-period-ns. Since ref clock is available this is not used. Will remove this. Thanks Varada > Konrad > > + }; > > + }; > > }; > > > > timer {
On Wed, Jun 07, 2023 at 08:35:09PM +0200, Krzysztof Kozlowski wrote: > On 07/06/2023 12:56, Varadarajan Narayanan wrote: > > Add USB phy and controller nodes > > > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > > --- > > arch/arm64/boot/dts/qcom/ipq5332.dtsi | 55 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 55 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi > > index c2d6cc65..3183357 100644 > > --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi > > +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi > > @@ -383,6 +383,61 @@ > > status = "disabled"; > > }; > > }; > > + > > + usb_0_m31phy: hs_m31phy@7b000 { > > Node names should be generic. See also explanation and list of examples > in DT specification: > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation Ok. > > + compatible = "qcom,ipq5332-m31-usb-hsphy"; > > + reg = <0x0007b000 0x12C>, > > + <0x08af8800 0x400>; > > Lowercase hex only. Ok. > > + reg-names = "m31usb_phy_base", > > + "qscratch_base"; > > + phy_type= "utmi"; > > + > > + resets = <&gcc GCC_QUSB2_0_PHY_BCR>; > > + reset-names = "usb2_phy_reset"; > > + > > + status = "okay"; > > It's by default. Drop. Ok. > > + }; > > + > > + usb2: usb2@8a00000 { > > It does not look like you tested the DTS against bindings. Please run > `make dtbs_check` (see > Documentation/devicetree/bindings/writing-schema.rst for instructions). > > Node names should be generic. See also explanation and list of examples > in DT specification: > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation Ok. > > + compatible = "qcom,ipq5332-dwc3", "qcom,dwc3"; > > + #address-cells = <1>; > > + #size-cells = <1>; > > + ranges; > > + > > + reg = <0x08af8800 0x100>; > > reg is always after compatible. Ranges is third. Then you will spot that > address is wrong. Ok. > > + > > + clocks = <&gcc GCC_USB0_MASTER_CLK>, > > + <&gcc GCC_SNOC_USB_CLK>, > > + <&gcc GCC_USB0_SLEEP_CLK>, > > + <&gcc GCC_USB0_MOCK_UTMI_CLK>; > > Fix alignment. Ok. > > + > > + clock-names = "core", > > + "iface", > > + "sleep", > > + "mock_utmi"; > > Fix alignment. Ok. > > + > > + interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>; > > + interrupt-names = "pwr_event"; > > + Thanks Varada > Best regards, > Krzysztof >
On Wed, Jun 07, 2023 at 08:36:21PM +0200, Krzysztof Kozlowski wrote: > On 07/06/2023 12:56, Varadarajan Narayanan wrote: > > Enable QCOM M31 USB phy driver present in IPQ5332 > > What is "QCOM"? If acronym, extend. IPQ5332 - provide full name, so > "Qualcomm IPQ....". Will remove 'QCOM'. Thanks Varada > Best regards, > Krzysztof >
On 07-06-23, 13:54, Konrad Dybcio wrote: > > > On 7.06.2023 12:56, Varadarajan Narayanan wrote: > > Add the M31 USB2 phy driver > > > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com> > > --- > > drivers/phy/qualcomm/phy-qcom-m31.c | 360 ++++++++++++++++++++++++++++++++++++ > > 1 file changed, 360 insertions(+) > > create mode 100644 drivers/phy/qualcomm/phy-qcom-m31.c > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-m31.c b/drivers/phy/qualcomm/phy-qcom-m31.c > > new file mode 100644 > > index 0000000..d29a91e > > --- /dev/null > > +++ b/drivers/phy/qualcomm/phy-qcom-m31.c > > @@ -0,0 +1,360 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright (c) 2014-2016, 2020, The Linux Foundation. All rights reserved. > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/kernel.h> > > +#include <linux/err.h> > > +#include <linux/slab.h> > > +#include <linux/clk.h> > > +#include <linux/delay.h> > > +#include <linux/io.h> > > +#include <linux/of.h> > > +#include <linux/platform_device.h> > > +#include <linux/usb/phy.h> > > +#include <linux/reset.h> > > +#include <linux/of_device.h> > Please sort these > > > + > > +enum clk_reset_action { > > + CLK_RESET_DEASSERT = 0, > > + CLK_RESET_ASSERT = 1 > > +}; > > + > > +#define USB2PHY_PORT_POWERDOWN 0xA4 > > +#define POWER_UP BIT(0) > > +#define POWER_DOWN 0 > > + > > +#define USB2PHY_PORT_UTMI_CTRL1 0x40 > > + > > +#define USB2PHY_PORT_UTMI_CTRL2 0x44 > > +#define UTMI_ULPI_SEL BIT(7) > > +#define UTMI_TEST_MUX_SEL BIT(6) > > + > > +#define HS_PHY_CTRL_REG 0x10 > > +#define UTMI_OTG_VBUS_VALID BIT(20) > > +#define SW_SESSVLD_SEL BIT(28) > > + > > +#define USB_PHY_CFG0 0x94 > > +#define USB_PHY_UTMI_CTRL5 0x50 > > +#define USB_PHY_FSEL_SEL 0xB8 > > +#define USB_PHY_HS_PHY_CTRL_COMMON0 0x54 > > +#define USB_PHY_REFCLK_CTRL 0xA0 > > +#define USB_PHY_HS_PHY_CTRL2 0x64 > > +#define USB_PHY_UTMI_CTRL0 0x3c > > +#define USB2PHY_USB_PHY_M31_XCFGI_1 0xBC > > +#define USB2PHY_USB_PHY_M31_XCFGI_4 0xC8 > > +#define USB2PHY_USB_PHY_M31_XCFGI_5 0xCC > > +#define USB2PHY_USB_PHY_M31_XCFGI_11 0xE4 > Could you sort them address-wise? and lower case hex values as well please