* [PATCH 0/9] Enable IPQ5332 USB2
@ 2023-06-07 10:56 Varadarajan Narayanan
2023-06-07 10:56 ` [PATCH 1/9] dt-bindings: usb: dwc3: Add IPQ5332 compatible Varadarajan Narayanan
` (8 more replies)
0 siblings, 9 replies; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-07 10:56 UTC (permalink / raw
To: agross, andersson, konrad.dybcio, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
Cc: Varadarajan Narayanan
This patch series adds the relevant phy and controller
configurations for enabling USB2 on IPQ5332
Depends On: https://lore.kernel.org/linux-arm-msm/cover.1686045347.git.quic_varada@quicinc.com/
Varadarajan Narayanan (9):
dt-bindings: usb: dwc3: Add IPQ5332 compatible
dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy
phy: qcom-m31: Introduce qcom,m31 USB phy driver
clk: qcom: ipq5332: Fix USB related clock defines
phy: qcom-m31: Introduce qcom,m31 USB phy
phy: qcom: Add qcom,m31 USB phy driver
arm64: dts: qcom: ipq5332: Add USB related nodes
arm64: dts: qcom: ipq5332: Enable USB
arm64: defconfig: Enable QCOM M31 USB phy driver
.../devicetree/bindings/phy/qcom,m31.yaml | 69 ++++
.../devicetree/bindings/usb/qcom,dwc3.yaml | 2 +
arch/arm64/boot/dts/qcom/ipq5332-rdp468.dts | 8 +
arch/arm64/boot/dts/qcom/ipq5332.dtsi | 55 ++++
arch/arm64/configs/defconfig | 1 +
drivers/clk/qcom/gcc-ipq5332.c | 34 +-
drivers/phy/qualcomm/Kconfig | 11 +
drivers/phy/qualcomm/Makefile | 1 +
drivers/phy/qualcomm/phy-qcom-m31.c | 360 +++++++++++++++++++++
9 files changed, 530 insertions(+), 11 deletions(-)
create mode 100644 Documentation/devicetree/bindings/phy/qcom,m31.yaml
create mode 100644 drivers/phy/qualcomm/phy-qcom-m31.c
--
2.7.4
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 1/9] dt-bindings: usb: dwc3: Add IPQ5332 compatible
2023-06-07 10:56 [PATCH 0/9] Enable IPQ5332 USB2 Varadarajan Narayanan
@ 2023-06-07 10:56 ` Varadarajan Narayanan
2023-06-07 18:33 ` Krzysztof Kozlowski
2023-06-07 10:56 ` [PATCH 2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy Varadarajan Narayanan
` (7 subsequent siblings)
8 siblings, 1 reply; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-07 10:56 UTC (permalink / raw
To: agross, andersson, konrad.dybcio, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
Cc: Varadarajan Narayanan
Document the IPQ5332 dwc3 compatible
Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
index ae24dac..9c3d6f4 100644
--- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
@@ -14,6 +14,7 @@ properties:
items:
- enum:
- qcom,ipq4019-dwc3
+ - qcom,ipq5332-dwc3
- qcom,ipq6018-dwc3
- qcom,ipq8064-dwc3
- qcom,ipq8074-dwc3
@@ -246,6 +247,7 @@ allOf:
compatible:
contains:
enum:
+ - qcom,ipq5332-dwc3
- qcom,msm8994-dwc3
- qcom,qcs404-dwc3
then:
--
2.7.4
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy
2023-06-07 10:56 [PATCH 0/9] Enable IPQ5332 USB2 Varadarajan Narayanan
2023-06-07 10:56 ` [PATCH 1/9] dt-bindings: usb: dwc3: Add IPQ5332 compatible Varadarajan Narayanan
@ 2023-06-07 10:56 ` Varadarajan Narayanan
2023-06-07 11:37 ` Rob Herring
2023-06-07 18:31 ` Krzysztof Kozlowski
2023-06-07 10:56 ` [PATCH 3/9] phy: qcom-m31: Introduce qcom,m31 USB phy driver Varadarajan Narayanan
` (6 subsequent siblings)
8 siblings, 2 replies; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-07 10:56 UTC (permalink / raw
To: agross, andersson, konrad.dybcio, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
Cc: Varadarajan Narayanan
Document the M31 USB2 phy present in IPQ5332
Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
.../devicetree/bindings/phy/qcom,m31.yaml | 69 ++++++++++++++++++++++
1 file changed, 69 insertions(+)
create mode 100644 Documentation/devicetree/bindings/phy/qcom,m31.yaml
diff --git a/Documentation/devicetree/bindings/phy/qcom,m31.yaml b/Documentation/devicetree/bindings/phy/qcom,m31.yaml
new file mode 100644
index 0000000..8ad4ba4
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/qcom,m31.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/qcom,m31.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm M31 USB PHY
+
+maintainers:
+ - Sricharan Ramabadhran <quic_srichara@quicinc.com>
+ - Varadarajan Narayanan <quic_varada@quicinc.org>
+
+description:
+ This file contains documentation for the USB M31 PHY found in Qualcomm
+ IPQ5018, IPQ5332 SoCs.
+
+properties:
+ compatible:
+ oneOf:
+ - items:
+ - enum:
+ - qcom,m31-usb-hsphy
+ - qcom,ipq5332-m31-usb-hsphy
+
+ reg:
+ description:
+ Offset and length of the M31 PHY register set
+ maxItems: 2
+
+ reg-names:
+ items:
+ - const: m31usb_phy_base
+ - const: qscratch_base
+
+ phy_type:
+ oneOf:
+ - items:
+ - enum:
+ - utmi
+ - ulpi
+
+ resets:
+ maxItems: 1
+ description:
+ List of phandles and reset pairs, one for each entry in reset-names.
+
+ reset-names:
+ items:
+ - const:
+ usb2_phy_reset
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
+ hs_m31phy_0: hs_m31phy@5b00 {
+ compatible = "qcom,m31-usb-hsphy";
+ reg = <0x5b000 0x3000>,
+ <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 = "ok";
+ };
--
2.7.4
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 3/9] phy: qcom-m31: Introduce qcom,m31 USB phy driver
2023-06-07 10:56 [PATCH 0/9] Enable IPQ5332 USB2 Varadarajan Narayanan
2023-06-07 10:56 ` [PATCH 1/9] dt-bindings: usb: dwc3: Add IPQ5332 compatible Varadarajan Narayanan
2023-06-07 10:56 ` [PATCH 2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy Varadarajan Narayanan
@ 2023-06-07 10:56 ` Varadarajan Narayanan
2023-06-07 11:54 ` Konrad Dybcio
2023-06-07 10:56 ` [PATCH 4/9] clk: qcom: ipq5332: Fix USB related clock defines Varadarajan Narayanan
` (5 subsequent siblings)
8 siblings, 1 reply; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-07 10:56 UTC (permalink / raw
To: agross, andersson, konrad.dybcio, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
Cc: Varadarajan Narayanan
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>
+
+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
+
+#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)
+
+#define UTMI_PHY_OVERRIDE_EN BIT(1)
+#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)
+
+static void m31usb_write_readback(void *base, u32 offset,
+ const u32 mask, u32 val);
+
+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 */
+}
+
+static void m31usb_phy_enable_clock(struct m31usb_phy *qphy)
+{
+ /* Enable override ctrl */
+ writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0);
+ /* 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*/
+ 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);
+ /*
+ * 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);
+ 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();
+
+ /* 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);
+
+ /* 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)
+{
+ 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);
+
+ /* 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);
+
+ 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;
+
+ 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);
+ if (IS_ERR(qphy->base))
+ return PTR_ERR(qphy->base);
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+ "qscratch_base");
+ if (res) {
+ 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");
+ 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);
+
+ 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)
+{
+ 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),
+ },
+};
+
+module_platform_driver(m31usb_phy_driver);
+
+MODULE_DESCRIPTION("USB2 Qualcomm M31 HSPHY driver");
+MODULE_LICENSE("GPL");
--
2.7.4
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 4/9] clk: qcom: ipq5332: Fix USB related clock defines
2023-06-07 10:56 [PATCH 0/9] Enable IPQ5332 USB2 Varadarajan Narayanan
` (2 preceding siblings ...)
2023-06-07 10:56 ` [PATCH 3/9] phy: qcom-m31: Introduce qcom,m31 USB phy driver Varadarajan Narayanan
@ 2023-06-07 10:56 ` Varadarajan Narayanan
2023-06-07 11:19 ` Dmitry Baryshkov
2023-06-07 10:56 ` [PATCH 5/9] phy: qcom-m31: Introduce qcom,m31 USB phy Varadarajan Narayanan
` (4 subsequent siblings)
8 siblings, 1 reply; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-07 10:56 UTC (permalink / raw
To: agross, andersson, konrad.dybcio, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
Cc: Varadarajan Narayanan
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" },
+};
+
+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,
},
},
};
@@ -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"
},
.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[] = {
--
2.7.4
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 5/9] phy: qcom-m31: Introduce qcom,m31 USB phy
2023-06-07 10:56 [PATCH 0/9] Enable IPQ5332 USB2 Varadarajan Narayanan
` (3 preceding siblings ...)
2023-06-07 10:56 ` [PATCH 4/9] clk: qcom: ipq5332: Fix USB related clock defines Varadarajan Narayanan
@ 2023-06-07 10:56 ` Varadarajan Narayanan
2023-06-07 11:20 ` Dmitry Baryshkov
2023-06-07 10:56 ` [PATCH 6/9] phy: qcom: Add qcom,m31 USB phy driver Varadarajan Narayanan
` (3 subsequent siblings)
8 siblings, 1 reply; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-07 10:56 UTC (permalink / raw
To: agross, andersson, konrad.dybcio, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
Cc: Varadarajan Narayanan
Introduce CONFIG_PHY_QCOM_M31_USB for including the M31 phy driver
Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
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.
--
2.7.4
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 6/9] phy: qcom: Add qcom,m31 USB phy driver
2023-06-07 10:56 [PATCH 0/9] Enable IPQ5332 USB2 Varadarajan Narayanan
` (4 preceding siblings ...)
2023-06-07 10:56 ` [PATCH 5/9] phy: qcom-m31: Introduce qcom,m31 USB phy Varadarajan Narayanan
@ 2023-06-07 10:56 ` Varadarajan Narayanan
2023-06-07 18:36 ` Krzysztof Kozlowski
2023-06-07 10:56 ` [PATCH 7/9] arm64: dts: qcom: ipq5332: Add USB related nodes Varadarajan Narayanan
` (2 subsequent siblings)
8 siblings, 1 reply; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-07 10:56 UTC (permalink / raw
To: agross, andersson, konrad.dybcio, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
Cc: Varadarajan Narayanan
Include M31 phy driver if CONFIG_PHY_QCOM_M31_USB is enabled
Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
drivers/phy/qualcomm/Makefile | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/phy/qualcomm/Makefile b/drivers/phy/qualcomm/Makefile
index de3dc9c..79a6e75 100644
--- a/drivers/phy/qualcomm/Makefile
+++ b/drivers/phy/qualcomm/Makefile
@@ -21,3 +21,4 @@ obj-$(CONFIG_PHY_QCOM_USB_HS_28NM) += phy-qcom-usb-hs-28nm.o
obj-$(CONFIG_PHY_QCOM_USB_SS) += phy-qcom-usb-ss.o
obj-$(CONFIG_PHY_QCOM_USB_SNPS_FEMTO_V2)+= phy-qcom-snps-femto-v2.o
obj-$(CONFIG_PHY_QCOM_IPQ806X_USB) += phy-qcom-ipq806x-usb.o
+obj-$(CONFIG_PHY_QCOM_M31_USB) += phy-qcom-m31.o
--
2.7.4
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 7/9] arm64: dts: qcom: ipq5332: Add USB related nodes
2023-06-07 10:56 [PATCH 0/9] Enable IPQ5332 USB2 Varadarajan Narayanan
` (5 preceding siblings ...)
2023-06-07 10:56 ` [PATCH 6/9] phy: qcom: Add qcom,m31 USB phy driver Varadarajan Narayanan
@ 2023-06-07 10:56 ` Varadarajan Narayanan
2023-06-07 11:23 ` Dmitry Baryshkov
` (2 more replies)
2023-06-07 10:56 ` [PATCH 8/9] arm64: dts: qcom: ipq5332: Enable USB Varadarajan Narayanan
2023-06-07 10:56 ` [PATCH 9/9] arm64: defconfig: Enable QCOM M31 USB phy driver Varadarajan Narayanan
8 siblings, 3 replies; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-07 10:56 UTC (permalink / raw
To: agross, andersson, konrad.dybcio, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
Cc: Varadarajan Narayanan
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>;
+
+ clock-names = "core",
+ "iface",
+ "sleep",
+ "mock_utmi";
+
+ interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>;
+ 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 {
--
2.7.4
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 8/9] arm64: dts: qcom: ipq5332: Enable USB
2023-06-07 10:56 [PATCH 0/9] Enable IPQ5332 USB2 Varadarajan Narayanan
` (6 preceding siblings ...)
2023-06-07 10:56 ` [PATCH 7/9] arm64: dts: qcom: ipq5332: Add USB related nodes Varadarajan Narayanan
@ 2023-06-07 10:56 ` Varadarajan Narayanan
2023-06-07 10:56 ` [PATCH 9/9] arm64: defconfig: Enable QCOM M31 USB phy driver Varadarajan Narayanan
8 siblings, 0 replies; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-07 10:56 UTC (permalink / raw
To: agross, andersson, konrad.dybcio, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
Cc: Varadarajan Narayanan
Enable USB2 in host mode
Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
arch/arm64/boot/dts/qcom/ipq5332-rdp468.dts | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/ipq5332-rdp468.dts b/arch/arm64/boot/dts/qcom/ipq5332-rdp468.dts
index 3b6a5cb..303e796 100644
--- a/arch/arm64/boot/dts/qcom/ipq5332-rdp468.dts
+++ b/arch/arm64/boot/dts/qcom/ipq5332-rdp468.dts
@@ -101,3 +101,11 @@
bias-pull-up;
};
};
+
+&usb2 {
+ status = "okay";
+};
+
+&usb2_0_dwc {
+ dr_mode = "host";
+};
--
2.7.4
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH 9/9] arm64: defconfig: Enable QCOM M31 USB phy driver
2023-06-07 10:56 [PATCH 0/9] Enable IPQ5332 USB2 Varadarajan Narayanan
` (7 preceding siblings ...)
2023-06-07 10:56 ` [PATCH 8/9] arm64: dts: qcom: ipq5332: Enable USB Varadarajan Narayanan
@ 2023-06-07 10:56 ` Varadarajan Narayanan
2023-06-07 18:36 ` Krzysztof Kozlowski
8 siblings, 1 reply; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-07 10:56 UTC (permalink / raw
To: agross, andersson, konrad.dybcio, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
Cc: Varadarajan Narayanan
Enable QCOM M31 USB phy driver present in IPQ5332
Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
arch/arm64/configs/defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index afb63e3..f1e8669 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1362,6 +1362,7 @@ CONFIG_PHY_QCOM_USB_HS=m
CONFIG_PHY_QCOM_USB_SNPS_FEMTO_V2=m
CONFIG_PHY_QCOM_USB_HS_28NM=m
CONFIG_PHY_QCOM_USB_SS=m
+CONFIG_PHY_QCOM_M31_USB=m
CONFIG_PHY_R8A779F0_ETHERNET_SERDES=y
CONFIG_PHY_RCAR_GEN3_PCIE=y
CONFIG_PHY_RCAR_GEN3_USB2=y
--
2.7.4
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH 4/9] clk: qcom: ipq5332: Fix USB related clock defines
2023-06-07 10:56 ` [PATCH 4/9] clk: qcom: ipq5332: Fix USB related clock defines Varadarajan Narayanan
@ 2023-06-07 11:19 ` Dmitry Baryshkov
2023-06-15 6:02 ` Varadarajan Narayanan
0 siblings, 1 reply; 39+ messages in thread
From: Dmitry Baryshkov @ 2023-06-07 11:19 UTC (permalink / raw
To: Varadarajan Narayanan, agross, andersson, konrad.dybcio, vkoul,
kishon, robh+dt, krzysztof.kozlowski+dt, conor+dt, gregkh,
catalin.marinas, will, mturquette, sboyd, p.zabel, arnd,
geert+renesas, neil.armstrong, nfraprado, broonie, rafal,
quic_srichara, quic_varada, quic_wcheng, linux-arm-msm, linux-phy,
devicetree, linux-kernel, linux-usb, linux-arm-kernel, linux-clk
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[] = {
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/9] phy: qcom-m31: Introduce qcom,m31 USB phy
2023-06-07 10:56 ` [PATCH 5/9] phy: qcom-m31: Introduce qcom,m31 USB phy Varadarajan Narayanan
@ 2023-06-07 11:20 ` Dmitry Baryshkov
2023-06-07 18:37 ` Krzysztof Kozlowski
0 siblings, 1 reply; 39+ messages in thread
From: Dmitry Baryshkov @ 2023-06-07 11:20 UTC (permalink / raw
To: Varadarajan Narayanan, agross, andersson, konrad.dybcio, vkoul,
kishon, robh+dt, krzysztof.kozlowski+dt, conor+dt, gregkh,
catalin.marinas, will, mturquette, sboyd, p.zabel, arnd,
geert+renesas, neil.armstrong, nfraprado, broonie, rafal,
quic_srichara, quic_varada, quic_wcheng, linux-arm-msm, linux-phy,
devicetree, linux-kernel, linux-usb, linux-arm-kernel, linux-clk
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.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 7/9] arm64: dts: qcom: ipq5332: Add USB related nodes
2023-06-07 10:56 ` [PATCH 7/9] arm64: dts: qcom: ipq5332: Add USB related nodes Varadarajan Narayanan
@ 2023-06-07 11:23 ` Dmitry Baryshkov
2023-06-15 6:26 ` Varadarajan Narayanan
2023-06-07 18:24 ` Konrad Dybcio
2023-06-07 18:35 ` Krzysztof Kozlowski
2 siblings, 1 reply; 39+ messages in thread
From: Dmitry Baryshkov @ 2023-06-07 11:23 UTC (permalink / raw
To: Varadarajan Narayanan, agross, andersson, konrad.dybcio, vkoul,
kishon, robh+dt, krzysztof.kozlowski+dt, conor+dt, gregkh,
catalin.marinas, will, mturquette, sboyd, p.zabel, arnd,
geert+renesas, neil.armstrong, nfraprado, broonie, rafal,
quic_srichara, quic_varada, quic_wcheng, linux-arm-msm, linux-phy,
devicetree, linux-kernel, linux-usb, linux-arm-kernel, linux-clk
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 {
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy
2023-06-07 10:56 ` [PATCH 2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy Varadarajan Narayanan
@ 2023-06-07 11:37 ` Rob Herring
2023-06-07 18:31 ` Krzysztof Kozlowski
1 sibling, 0 replies; 39+ messages in thread
From: Rob Herring @ 2023-06-07 11:37 UTC (permalink / raw
To: Varadarajan Narayanan
Cc: linux-phy, neil.armstrong, devicetree, robh+dt, rafal, sboyd,
gregkh, arnd, mturquette, krzysztof.kozlowski+dt, agross, will,
nfraprado, linux-clk, andersson, linux-usb, konrad.dybcio,
p.zabel, linux-kernel, linux-arm-msm, linux-arm-kernel,
quic_srichara, conor+dt, broonie, geert+renesas, quic_wcheng,
kishon, quic_varada, catalin.marinas, vkoul
On Wed, 07 Jun 2023 16:26:06 +0530, Varadarajan Narayanan wrote:
> Document the M31 USB2 phy present in IPQ5332
>
> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
> .../devicetree/bindings/phy/qcom,m31.yaml | 69 ++++++++++++++++++++++
> 1 file changed, 69 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/qcom,m31.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/phy/qcom,m31.example.dtb: hs_m31phy@5b00: status:0: 'ok' is not one of ['okay', 'disabled', 'reserved']
From schema: /usr/local/lib/python3.10/dist-packages/dtschema/schemas/dt-core.yaml
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/14f60578e2935c0844537eab162af3afa52ffe39.1686126439.git.quic_varada@quicinc.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/9] phy: qcom-m31: Introduce qcom,m31 USB phy driver
2023-06-07 10:56 ` [PATCH 3/9] phy: qcom-m31: Introduce qcom,m31 USB phy driver Varadarajan Narayanan
@ 2023-06-07 11:54 ` Konrad Dybcio
2023-06-07 12:29 ` Dmitry Baryshkov
` (2 more replies)
0 siblings, 3 replies; 39+ messages in thread
From: Konrad Dybcio @ 2023-06-07 11:54 UTC (permalink / raw
To: Varadarajan Narayanan, agross, andersson, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
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");
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/9] phy: qcom-m31: Introduce qcom,m31 USB phy driver
2023-06-07 11:54 ` Konrad Dybcio
@ 2023-06-07 12:29 ` Dmitry Baryshkov
2023-06-15 6:01 ` Varadarajan Narayanan
2023-06-15 5:49 ` Varadarajan Narayanan
2023-06-21 11:05 ` Vinod Koul
2 siblings, 1 reply; 39+ messages in thread
From: Dmitry Baryshkov @ 2023-06-07 12:29 UTC (permalink / raw
To: Konrad Dybcio, Varadarajan Narayanan, agross, andersson, vkoul,
kishon, robh+dt, krzysztof.kozlowski+dt, conor+dt, gregkh,
catalin.marinas, will, mturquette, sboyd, p.zabel, arnd,
geert+renesas, neil.armstrong, nfraprado, broonie, rafal,
quic_srichara, quic_varada, quic_wcheng, linux-arm-msm, linux-phy,
devicetree, linux-kernel, linux-usb, linux-arm-kernel, linux-clk
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.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 7/9] arm64: dts: qcom: ipq5332: Add USB related nodes
2023-06-07 10:56 ` [PATCH 7/9] arm64: dts: qcom: ipq5332: Add USB related nodes Varadarajan Narayanan
2023-06-07 11:23 ` Dmitry Baryshkov
@ 2023-06-07 18:24 ` Konrad Dybcio
2023-06-15 6:52 ` Varadarajan Narayanan
2023-06-07 18:35 ` Krzysztof Kozlowski
2 siblings, 1 reply; 39+ messages in thread
From: Konrad Dybcio @ 2023-06-07 18:24 UTC (permalink / raw
To: Varadarajan Narayanan, agross, andersson, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
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 {
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy
2023-06-07 10:56 ` [PATCH 2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy Varadarajan Narayanan
2023-06-07 11:37 ` Rob Herring
@ 2023-06-07 18:31 ` Krzysztof Kozlowski
2023-06-15 5:27 ` Varadarajan Narayanan
1 sibling, 1 reply; 39+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-07 18:31 UTC (permalink / raw
To: Varadarajan Narayanan, agross, andersson, konrad.dybcio, vkoul,
kishon, robh+dt, krzysztof.kozlowski+dt, conor+dt, gregkh,
catalin.marinas, will, mturquette, sboyd, p.zabel, arnd,
geert+renesas, neil.armstrong, nfraprado, broonie, rafal,
quic_srichara, quic_varada, quic_wcheng, linux-arm-msm, linux-phy,
devicetree, linux-kernel, linux-usb, linux-arm-kernel, linux-clk
On 07/06/2023 12:56, Varadarajan Narayanan wrote:
> Document the M31 USB2 phy present in IPQ5332
Full stop.
>
> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
> .../devicetree/bindings/phy/qcom,m31.yaml | 69 ++++++++++++++++++++++
> 1 file changed, 69 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/qcom,m31.yaml
>
> diff --git a/Documentation/devicetree/bindings/phy/qcom,m31.yaml b/Documentation/devicetree/bindings/phy/qcom,m31.yaml
> new file mode 100644
> index 0000000..8ad4ba4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qcom,m31.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +
Drop stray blank lines.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/qcom,m31.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm M31 USB PHY
What is M31?
> +
> +maintainers:
> + - Sricharan Ramabadhran <quic_srichara@quicinc.com>
> + - Varadarajan Narayanan <quic_varada@quicinc.org>
> +
> +description:
> + This file contains documentation for the USB M31 PHY found in Qualcomm
Drop redundant parts ("This file contains documentation for the").
> + IPQ5018, IPQ5332 SoCs.
> +
> +properties:
> + compatible:
> + oneOf:
That's not oneOf.
> + - items:
No items.
> + - enum:
> + - qcom,m31-usb-hsphy
I am confused what's this. If m31 is coming from some IP block provider,
then you are using wrong vendor prefix.
https://www.m31tech.com/download_file/M31_USB.pdf
> + - qcom,ipq5332-m31-usb-hsphy
This confuses me even more. IPQ m31?
> +
> + reg:
> + description:
> + Offset and length of the M31 PHY register set
Drop description, obvious.
> + maxItems: 2
> +
> + reg-names:
> + items:
> + - const: m31usb_phy_base
> + - const: qscratch_base
Drop "_base" from both.
> +
> + phy_type:
> + oneOf:
> + - items:
> + - enum:
> + - utmi
> + - ulpi
This does not belong to phy, but to USB node.
> +
> + resets:
> + maxItems: 1
> + description:
> + List of phandles and reset pairs, one for each entry in reset-names.
Drop useless description.
> +
> + reset-names:
> + items:
> + - const:
> + usb2_phy_reset
Drop entire reset-names.
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
> + hs_m31phy_0: hs_m31phy@5b00 {
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
Also, no underscores in node names.
> + compatible = "qcom,m31-usb-hsphy";
> + reg = <0x5b000 0x3000>,
This is not what your unit address is saying.
> + <0x08af8800 0x400>;
Align it.
> + reg-names = "m31usb_phy_base",
> + "qscratch_base";
Align it.
> + phy_type = "utmi";
> + resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
> + reset-names = "usb2_phy_reset";
> +
> + status = "ok";
Drop.
> + };
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/9] dt-bindings: usb: dwc3: Add IPQ5332 compatible
2023-06-07 10:56 ` [PATCH 1/9] dt-bindings: usb: dwc3: Add IPQ5332 compatible Varadarajan Narayanan
@ 2023-06-07 18:33 ` Krzysztof Kozlowski
2023-06-15 4:15 ` Varadarajan Narayanan
0 siblings, 1 reply; 39+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-07 18:33 UTC (permalink / raw
To: Varadarajan Narayanan, agross, andersson, konrad.dybcio, vkoul,
kishon, robh+dt, krzysztof.kozlowski+dt, conor+dt, gregkh,
catalin.marinas, will, mturquette, sboyd, p.zabel, arnd,
geert+renesas, neil.armstrong, nfraprado, broonie, rafal,
quic_srichara, quic_varada, quic_wcheng, linux-arm-msm, linux-phy,
devicetree, linux-kernel, linux-usb, linux-arm-kernel, linux-clk
On 07/06/2023 12:56, Varadarajan Narayanan wrote:
> Document the IPQ5332 dwc3 compatible
Full stop.
>
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
> Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> index ae24dac..9c3d6f4 100644
> --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> @@ -14,6 +14,7 @@ properties:
> items:
> - enum:
> - qcom,ipq4019-dwc3
> + - qcom,ipq5332-dwc3
> - qcom,ipq6018-dwc3
> - qcom,ipq8064-dwc3
> - qcom,ipq8074-dwc3
> @@ -246,6 +247,7 @@ allOf:
> compatible:
> contains:
> enum:
> + - qcom,ipq5332-dwc3
> - qcom,msm8994-dwc3
> - qcom,qcs404-dwc3
What about interrupts?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 7/9] arm64: dts: qcom: ipq5332: Add USB related nodes
2023-06-07 10:56 ` [PATCH 7/9] arm64: dts: qcom: ipq5332: Add USB related nodes Varadarajan Narayanan
2023-06-07 11:23 ` Dmitry Baryshkov
2023-06-07 18:24 ` Konrad Dybcio
@ 2023-06-07 18:35 ` Krzysztof Kozlowski
2023-06-15 7:17 ` Varadarajan Narayanan
2 siblings, 1 reply; 39+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-07 18:35 UTC (permalink / raw
To: Varadarajan Narayanan, agross, andersson, konrad.dybcio, vkoul,
kishon, robh+dt, krzysztof.kozlowski+dt, conor+dt, gregkh,
catalin.marinas, will, mturquette, sboyd, p.zabel, arnd,
geert+renesas, neil.armstrong, nfraprado, broonie, rafal,
quic_srichara, quic_varada, quic_wcheng, linux-arm-msm, linux-phy,
devicetree, linux-kernel, linux-usb, linux-arm-kernel, linux-clk
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 9/9] arm64: defconfig: Enable QCOM M31 USB phy driver
2023-06-07 10:56 ` [PATCH 9/9] arm64: defconfig: Enable QCOM M31 USB phy driver Varadarajan Narayanan
@ 2023-06-07 18:36 ` Krzysztof Kozlowski
2023-06-15 8:37 ` Varadarajan Narayanan
0 siblings, 1 reply; 39+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-07 18:36 UTC (permalink / raw
To: Varadarajan Narayanan, agross, andersson, konrad.dybcio, vkoul,
kishon, robh+dt, krzysztof.kozlowski+dt, conor+dt, gregkh,
catalin.marinas, will, mturquette, sboyd, p.zabel, arnd,
geert+renesas, neil.armstrong, nfraprado, broonie, rafal,
quic_srichara, quic_varada, quic_wcheng, linux-arm-msm, linux-phy,
devicetree, linux-kernel, linux-usb, linux-arm-kernel, linux-clk
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 6/9] phy: qcom: Add qcom,m31 USB phy driver
2023-06-07 10:56 ` [PATCH 6/9] phy: qcom: Add qcom,m31 USB phy driver Varadarajan Narayanan
@ 2023-06-07 18:36 ` Krzysztof Kozlowski
2023-06-15 6:14 ` Varadarajan Narayanan
0 siblings, 1 reply; 39+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-07 18:36 UTC (permalink / raw
To: Varadarajan Narayanan, agross, andersson, konrad.dybcio, vkoul,
kishon, robh+dt, krzysztof.kozlowski+dt, conor+dt, gregkh,
catalin.marinas, will, mturquette, sboyd, p.zabel, arnd,
geert+renesas, neil.armstrong, nfraprado, broonie, rafal,
quic_srichara, quic_varada, quic_wcheng, linux-arm-msm, linux-phy,
devicetree, linux-kernel, linux-usb, linux-arm-kernel, linux-clk
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/9] phy: qcom-m31: Introduce qcom,m31 USB phy
2023-06-07 11:20 ` Dmitry Baryshkov
@ 2023-06-07 18:37 ` Krzysztof Kozlowski
2023-06-15 6:05 ` Varadarajan Narayanan
0 siblings, 1 reply; 39+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-07 18:37 UTC (permalink / raw
To: Dmitry Baryshkov, Varadarajan Narayanan, agross, andersson,
konrad.dybcio, vkoul, kishon, robh+dt, krzysztof.kozlowski+dt,
conor+dt, gregkh, catalin.marinas, will, mturquette, sboyd,
p.zabel, arnd, geert+renesas, neil.armstrong, nfraprado, broonie,
rafal, quic_srichara, quic_varada, quic_wcheng, linux-arm-msm,
linux-phy, devicetree, linux-kernel, linux-usb, linux-arm-kernel,
linux-clk
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 1/9] dt-bindings: usb: dwc3: Add IPQ5332 compatible
2023-06-07 18:33 ` Krzysztof Kozlowski
@ 2023-06-15 4:15 ` Varadarajan Narayanan
0 siblings, 0 replies; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-15 4:15 UTC (permalink / raw
To: Krzysztof Kozlowski
Cc: agross, andersson, konrad.dybcio, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
On Wed, Jun 07, 2023 at 08:33:26PM +0200, Krzysztof Kozlowski wrote:
> On 07/06/2023 12:56, Varadarajan Narayanan wrote:
> > Document the IPQ5332 dwc3 compatible
>
> Full stop.
Ok.
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
> > Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> > index ae24dac..9c3d6f4 100644
> > --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> > +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> > @@ -14,6 +14,7 @@ properties:
> > items:
> > - enum:
> > - qcom,ipq4019-dwc3
> > + - qcom,ipq5332-dwc3
> > - qcom,ipq6018-dwc3
> > - qcom,ipq8064-dwc3
> > - qcom,ipq8074-dwc3
> > @@ -246,6 +247,7 @@ allOf:
> > compatible:
> > contains:
> > enum:
> > + - qcom,ipq5332-dwc3
> > - qcom,msm8994-dwc3
> > - qcom,qcs404-dwc3
>
> What about interrupts?
Will fix and post next version.
Thanks
Varada
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy
2023-06-07 18:31 ` Krzysztof Kozlowski
@ 2023-06-15 5:27 ` Varadarajan Narayanan
2023-06-17 8:48 ` Krzysztof Kozlowski
0 siblings, 1 reply; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-15 5:27 UTC (permalink / raw
To: Krzysztof Kozlowski
Cc: agross, andersson, konrad.dybcio, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
On Wed, Jun 07, 2023 at 08:31:50PM +0200, Krzysztof Kozlowski wrote:
> On 07/06/2023 12:56, Varadarajan Narayanan wrote:
> > Document the M31 USB2 phy present in IPQ5332
>
> Full stop.
Ok.
> > Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
> > .../devicetree/bindings/phy/qcom,m31.yaml | 69 ++++++++++++++++++++++
> > 1 file changed, 69 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/phy/qcom,m31.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/phy/qcom,m31.yaml b/Documentation/devicetree/bindings/phy/qcom,m31.yaml
> > new file mode 100644
> > index 0000000..8ad4ba4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/qcom,m31.yaml
> > @@ -0,0 +1,69 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +
>
> Drop stray blank lines.
Ok.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/phy/qcom,m31.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm M31 USB PHY
>
> What is M31?
M31 is an external IP integrated into IPQ5332 SoC.
> > +
> > +maintainers:
> > + - Sricharan Ramabadhran <quic_srichara@quicinc.com>
> > + - Varadarajan Narayanan <quic_varada@quicinc.org>
> > +
> > +description:
> > + This file contains documentation for the USB M31 PHY found in Qualcomm
>
> Drop redundant parts ("This file contains documentation for the").
Ok.
> > + IPQ5018, IPQ5332 SoCs.
> > +
> > +properties:
> > + compatible:
> > + oneOf:
>
> That's not oneOf.
Ok.
> > + - items:
>
> No items.
Ok.
> > + - enum:
> > + - qcom,m31-usb-hsphy
>
> I am confused what's this. If m31 is coming from some IP block provider,
> then you are using wrong vendor prefix.
> https://www.m31tech.com/download_file/M31_USB.pdf
>
>
> > + - qcom,ipq5332-m31-usb-hsphy
>
> This confuses me even more. IPQ m31?
Will change this to m31,usb-hsphy and m31,ipq5332-usb-hsphy respectively.
Will that be acceptable?
> > +
> > + reg:
> > + description:
> > + Offset and length of the M31 PHY register set
>
> Drop description, obvious.
Ok.
> > + maxItems: 2
> > +
> > + reg-names:
> > + items:
> > + - const: m31usb_phy_base
> > + - const: qscratch_base
>
> Drop "_base" from both.
Ok. Will drop qscratch_base. This is in the controller space.
Should not come here.
> > +
> > + phy_type:
> > + oneOf:
> > + - items:
> > + - enum:
> > + - utmi
> > + - ulpi
>
> This does not belong to phy, but to USB node.
This is used by the driver to set a bit during phy init. Hence
have it as a replication of the USB node's entry. If this is not
permissible, is there some way to get this from the USB node,
or any other alternative mechanism?
> > +
> > + resets:
> > + maxItems: 1
> > + description:
> > + List of phandles and reset pairs, one for each entry in reset-names.
>
> Drop useless description.
Ok.
> > +
> > + reset-names:
> > + items:
> > + - const:
> > + usb2_phy_reset
>
> Drop entire reset-names.
Ok.
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
> > + hs_m31phy_0: hs_m31phy@5b00 {
>
> 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
>
> Also, no underscores in node names.
Will change this as usbphy0:hs_m31phy@7b000
> > + compatible = "qcom,m31-usb-hsphy";
> > + reg = <0x5b000 0x3000>,
>
> This is not what your unit address is saying.
Will change to 0x0007b000.
> > + <0x08af8800 0x400>;
>
> Align it.
Will drop this. This is in the controller space.
> > + reg-names = "m31usb_phy_base",
> > + "qscratch_base";
>
> Align it.
Ok.
> > + phy_type = "utmi";
> > + resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
> > + reset-names = "usb2_phy_reset";
> > +
> > + status = "ok";
>
> Drop.
Ok. Hopefully will get an alternative for the phy_type.
Thanks
Varada
> > + };
>
> Best regards,
> Krzysztof
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/9] phy: qcom-m31: Introduce qcom,m31 USB phy driver
2023-06-07 11:54 ` Konrad Dybcio
2023-06-07 12:29 ` Dmitry Baryshkov
@ 2023-06-15 5:49 ` Varadarajan Narayanan
2023-06-21 11:05 ` Vinod Koul
2 siblings, 0 replies; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-15 5:49 UTC (permalink / raw
To: Konrad Dybcio
Cc: agross, andersson, vkoul, kishon, robh+dt, krzysztof.kozlowski+dt,
conor+dt, gregkh, catalin.marinas, will, mturquette, sboyd,
p.zabel, arnd, geert+renesas, neil.armstrong, nfraprado, broonie,
rafal, quic_srichara, quic_varada, quic_wcheng, linux-arm-msm,
linux-phy, devicetree, linux-kernel, linux-usb, linux-arm-kernel,
linux-clk
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");
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/9] phy: qcom-m31: Introduce qcom,m31 USB phy driver
2023-06-07 12:29 ` Dmitry Baryshkov
@ 2023-06-15 6:01 ` Varadarajan Narayanan
0 siblings, 0 replies; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-15 6:01 UTC (permalink / raw
To: Dmitry Baryshkov
Cc: Konrad Dybcio, agross, andersson, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
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
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 4/9] clk: qcom: ipq5332: Fix USB related clock defines
2023-06-07 11:19 ` Dmitry Baryshkov
@ 2023-06-15 6:02 ` Varadarajan Narayanan
0 siblings, 0 replies; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-15 6:02 UTC (permalink / raw
To: Dmitry Baryshkov
Cc: agross, andersson, konrad.dybcio, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
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
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 5/9] phy: qcom-m31: Introduce qcom,m31 USB phy
2023-06-07 18:37 ` Krzysztof Kozlowski
@ 2023-06-15 6:05 ` Varadarajan Narayanan
0 siblings, 0 replies; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-15 6:05 UTC (permalink / raw
To: Krzysztof Kozlowski
Cc: Dmitry Baryshkov, agross, andersson, konrad.dybcio, vkoul, kishon,
robh+dt, krzysztof.kozlowski+dt, conor+dt, gregkh,
catalin.marinas, will, mturquette, sboyd, p.zabel, arnd,
geert+renesas, neil.armstrong, nfraprado, broonie, rafal,
quic_srichara, quic_varada, quic_wcheng, linux-arm-msm, linux-phy,
devicetree, linux-kernel, linux-usb, linux-arm-kernel, linux-clk
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 6/9] phy: qcom: Add qcom,m31 USB phy driver
2023-06-07 18:36 ` Krzysztof Kozlowski
@ 2023-06-15 6:14 ` Varadarajan Narayanan
0 siblings, 0 replies; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-15 6:14 UTC (permalink / raw
To: Krzysztof Kozlowski
Cc: agross, andersson, konrad.dybcio, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
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
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 7/9] arm64: dts: qcom: ipq5332: Add USB related nodes
2023-06-07 11:23 ` Dmitry Baryshkov
@ 2023-06-15 6:26 ` Varadarajan Narayanan
0 siblings, 0 replies; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-15 6:26 UTC (permalink / raw
To: Dmitry Baryshkov
Cc: agross, andersson, konrad.dybcio, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
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
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 7/9] arm64: dts: qcom: ipq5332: Add USB related nodes
2023-06-07 18:24 ` Konrad Dybcio
@ 2023-06-15 6:52 ` Varadarajan Narayanan
0 siblings, 0 replies; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-15 6:52 UTC (permalink / raw
To: Konrad Dybcio
Cc: agross, andersson, vkoul, kishon, robh+dt, krzysztof.kozlowski+dt,
conor+dt, gregkh, catalin.marinas, will, mturquette, sboyd,
p.zabel, arnd, geert+renesas, neil.armstrong, nfraprado, broonie,
rafal, quic_srichara, quic_varada, quic_wcheng, linux-arm-msm,
linux-phy, devicetree, linux-kernel, linux-usb, linux-arm-kernel,
linux-clk
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 {
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 7/9] arm64: dts: qcom: ipq5332: Add USB related nodes
2023-06-07 18:35 ` Krzysztof Kozlowski
@ 2023-06-15 7:17 ` Varadarajan Narayanan
0 siblings, 0 replies; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-15 7:17 UTC (permalink / raw
To: Krzysztof Kozlowski
Cc: agross, andersson, konrad.dybcio, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
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
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 9/9] arm64: defconfig: Enable QCOM M31 USB phy driver
2023-06-07 18:36 ` Krzysztof Kozlowski
@ 2023-06-15 8:37 ` Varadarajan Narayanan
0 siblings, 0 replies; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-15 8:37 UTC (permalink / raw
To: Krzysztof Kozlowski
Cc: agross, andersson, konrad.dybcio, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
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
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy
2023-06-15 5:27 ` Varadarajan Narayanan
@ 2023-06-17 8:48 ` Krzysztof Kozlowski
2023-06-20 9:32 ` Varadarajan Narayanan
0 siblings, 1 reply; 39+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-17 8:48 UTC (permalink / raw
To: Varadarajan Narayanan
Cc: agross, andersson, konrad.dybcio, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
On 15/06/2023 07:27, Varadarajan Narayanan wrote:
>>> + - enum:
>>> + - qcom,m31-usb-hsphy
>>
>> I am confused what's this. If m31 is coming from some IP block provider,
>> then you are using wrong vendor prefix.
>> https://www.m31tech.com/download_file/M31_USB.pdf
>>
>>
>>> + - qcom,ipq5332-m31-usb-hsphy
>>
>> This confuses me even more. IPQ m31?
>
> Will change this to m31,usb-hsphy and m31,ipq5332-usb-hsphy respectively.
> Will that be acceptable?
m31,ipq5332 seems wrong, as m31 did not create ipq5332. Does the m31
device have some name/version/model? If it is not really known, then I
would just propose to go with qcom,ipq5332-usb-hsphy.
Skip generic compatible ("usb-hsphy") entirely.
And then we have... existing bindings qcom,usb-hs-phy.yaml. Don't create
something similar with difference in the hyphen. Just use device
specific compatible thus device specific filename.
>
>>> +
>>> + reg:
>>> + description:
>>> + Offset and length of the M31 PHY register set
>>
>> Drop description, obvious.
>
> Ok.
>
>>> + maxItems: 2
>>> +
>>> + reg-names:
>>> + items:
>>> + - const: m31usb_phy_base
>>> + - const: qscratch_base
>>
>> Drop "_base" from both.
>
> Ok. Will drop qscratch_base. This is in the controller space.
> Should not come here.
Then drop reg-names entirely.
>
>>> +
>>> + phy_type:
>>> + oneOf:
>>> + - items:
>>> + - enum:
>>> + - utmi
>>> + - ulpi
>>
>> This does not belong to phy, but to USB node.
>
> This is used by the driver to set a bit during phy init. Hence
> have it as a replication of the USB node's entry. If this is not
> permissible, is there some way to get this from the USB node,
> or any other alternative mechanism?
Shouldn't USB controller choose what type of PHY type it wants?
>
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> + - |
>>> + #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
>>> + hs_m31phy_0: hs_m31phy@5b00 {
>>
>> 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
>>
>> Also, no underscores in node names.
>
> Will change this as usbphy0:hs_m31phy@7b000
This does not solve my comments. I did not write "label" but "node name".
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy
2023-06-17 8:48 ` Krzysztof Kozlowski
@ 2023-06-20 9:32 ` Varadarajan Narayanan
2023-06-21 8:43 ` Krzysztof Kozlowski
0 siblings, 1 reply; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-20 9:32 UTC (permalink / raw
To: Krzysztof Kozlowski
Cc: agross, andersson, konrad.dybcio, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
On Sat, Jun 17, 2023 at 10:48:41AM +0200, Krzysztof Kozlowski wrote:
> On 15/06/2023 07:27, Varadarajan Narayanan wrote:
> >>> + - enum:
> >>> + - qcom,m31-usb-hsphy
> >>
> >> I am confused what's this. If m31 is coming from some IP block provider,
> >> then you are using wrong vendor prefix.
> >> https://www.m31tech.com/download_file/M31_USB.pdf
> >>
> >>
> >>> + - qcom,ipq5332-m31-usb-hsphy
> >>
> >> This confuses me even more. IPQ m31?
> >
> > Will change this to m31,usb-hsphy and m31,ipq5332-usb-hsphy respectively.
> > Will that be acceptable?
>
> m31,ipq5332 seems wrong, as m31 did not create ipq5332. Does the m31
> device have some name/version/model? If it is not really known, then I
> would just propose to go with qcom,ipq5332-usb-hsphy.
>
> Skip generic compatible ("usb-hsphy") entirely.
Ok.
> And then we have... existing bindings qcom,usb-hs-phy.yaml. Don't create
> something similar with difference in the hyphen. Just use device
> specific compatible thus device specific filename.
qcom,usb-hs-phy.yaml seems to be for ULPI mode phy and the
driver we are introducing is for UTMI. We would have to
modify phy-qcom-usb-hs.c to accomodate M31. Will that be
acceptable to phy-qcom-usb-hs.c owners/maintainers?
> >>> +
> >>> + reg:
> >>> + description:
> >>> + Offset and length of the M31 PHY register set
> >>
> >> Drop description, obvious.
> >
> > Ok.
> >
> >>> + maxItems: 2
> >>> +
> >>> + reg-names:
> >>> + items:
> >>> + - const: m31usb_phy_base
> >>> + - const: qscratch_base
> >>
> >> Drop "_base" from both.
> >
> > Ok. Will drop qscratch_base. This is in the controller space.
> > Should not come here.
>
> Then drop reg-names entirely.
Ok.
> >>> +
> >>> + phy_type:
> >>> + oneOf:
> >>> + - items:
> >>> + - enum:
> >>> + - utmi
> >>> + - ulpi
> >>
> >> This does not belong to phy, but to USB node.
> >
> > This is used by the driver to set a bit during phy init. Hence
> > have it as a replication of the USB node's entry. If this is not
> > permissible, is there some way to get this from the USB node,
> > or any other alternative mechanism?
>
> Shouldn't USB controller choose what type of PHY type it wants?
Will remove this. IPQ5332 uses it in UTMI mode only.
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> + - |
> >>> + #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
> >>> + hs_m31phy_0: hs_m31phy@5b00 {
> >>
> >> 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
> >>
> >> Also, no underscores in node names.
> >
> > Will change this as usbphy0:hs_m31phy@7b000
>
> This does not solve my comments. I did not write "label" but "node name".
Sorry. will fix it.
Thanks
Varada
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy
2023-06-20 9:32 ` Varadarajan Narayanan
@ 2023-06-21 8:43 ` Krzysztof Kozlowski
2023-06-21 10:12 ` Varadarajan Narayanan
0 siblings, 1 reply; 39+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-21 8:43 UTC (permalink / raw
To: Varadarajan Narayanan
Cc: agross, andersson, konrad.dybcio, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
On 20/06/2023 11:32, Varadarajan Narayanan wrote:
>
>> And then we have... existing bindings qcom,usb-hs-phy.yaml. Don't create
>> something similar with difference in the hyphen. Just use device
>> specific compatible thus device specific filename.
>
> qcom,usb-hs-phy.yaml seems to be for ULPI mode phy and the
> driver we are introducing is for UTMI. We would have to
> modify phy-qcom-usb-hs.c to accomodate M31. Will that be
> acceptable to phy-qcom-usb-hs.c owners/maintainers?
We don't talk about drivers here but bindings. Why would you need to
modify the driver when introducing new binding for different device?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy
2023-06-21 8:43 ` Krzysztof Kozlowski
@ 2023-06-21 10:12 ` Varadarajan Narayanan
0 siblings, 0 replies; 39+ messages in thread
From: Varadarajan Narayanan @ 2023-06-21 10:12 UTC (permalink / raw
To: Krzysztof Kozlowski
Cc: agross, andersson, konrad.dybcio, vkoul, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
On Wed, Jun 21, 2023 at 10:43:38AM +0200, Krzysztof Kozlowski wrote:
> On 20/06/2023 11:32, Varadarajan Narayanan wrote:
>
> >
> >> And then we have... existing bindings qcom,usb-hs-phy.yaml. Don't create
> >> something similar with difference in the hyphen. Just use device
> >> specific compatible thus device specific filename.
> >
> > qcom,usb-hs-phy.yaml seems to be for ULPI mode phy and the
> > driver we are introducing is for UTMI. We would have to
> > modify phy-qcom-usb-hs.c to accomodate M31. Will that be
> > acceptable to phy-qcom-usb-hs.c owners/maintainers?
>
> We don't talk about drivers here but bindings. Why would you need to
> modify the driver when introducing new binding for different device?
Sorry. I misunderstood your feedback as "use the existing bindings".
Will name the bindings file as qcom,ipq5332-usb-hsphy.yaml and post
the next version.
Thanks
Varada
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 3/9] phy: qcom-m31: Introduce qcom,m31 USB phy driver
2023-06-07 11:54 ` Konrad Dybcio
2023-06-07 12:29 ` Dmitry Baryshkov
2023-06-15 5:49 ` Varadarajan Narayanan
@ 2023-06-21 11:05 ` Vinod Koul
2 siblings, 0 replies; 39+ messages in thread
From: Vinod Koul @ 2023-06-21 11:05 UTC (permalink / raw
To: Konrad Dybcio
Cc: Varadarajan Narayanan, agross, andersson, kishon, robh+dt,
krzysztof.kozlowski+dt, conor+dt, gregkh, catalin.marinas, will,
mturquette, sboyd, p.zabel, arnd, geert+renesas, neil.armstrong,
nfraprado, broonie, rafal, quic_srichara, quic_varada,
quic_wcheng, linux-arm-msm, linux-phy, devicetree, linux-kernel,
linux-usb, linux-arm-kernel, linux-clk
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
--
~Vinod
^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2023-06-21 11:05 UTC | newest]
Thread overview: 39+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-07 10:56 [PATCH 0/9] Enable IPQ5332 USB2 Varadarajan Narayanan
2023-06-07 10:56 ` [PATCH 1/9] dt-bindings: usb: dwc3: Add IPQ5332 compatible Varadarajan Narayanan
2023-06-07 18:33 ` Krzysztof Kozlowski
2023-06-15 4:15 ` Varadarajan Narayanan
2023-06-07 10:56 ` [PATCH 2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy Varadarajan Narayanan
2023-06-07 11:37 ` Rob Herring
2023-06-07 18:31 ` Krzysztof Kozlowski
2023-06-15 5:27 ` Varadarajan Narayanan
2023-06-17 8:48 ` Krzysztof Kozlowski
2023-06-20 9:32 ` Varadarajan Narayanan
2023-06-21 8:43 ` Krzysztof Kozlowski
2023-06-21 10:12 ` Varadarajan Narayanan
2023-06-07 10:56 ` [PATCH 3/9] phy: qcom-m31: Introduce qcom,m31 USB phy driver Varadarajan Narayanan
2023-06-07 11:54 ` Konrad Dybcio
2023-06-07 12:29 ` Dmitry Baryshkov
2023-06-15 6:01 ` Varadarajan Narayanan
2023-06-15 5:49 ` Varadarajan Narayanan
2023-06-21 11:05 ` Vinod Koul
2023-06-07 10:56 ` [PATCH 4/9] clk: qcom: ipq5332: Fix USB related clock defines Varadarajan Narayanan
2023-06-07 11:19 ` Dmitry Baryshkov
2023-06-15 6:02 ` Varadarajan Narayanan
2023-06-07 10:56 ` [PATCH 5/9] phy: qcom-m31: Introduce qcom,m31 USB phy Varadarajan Narayanan
2023-06-07 11:20 ` Dmitry Baryshkov
2023-06-07 18:37 ` Krzysztof Kozlowski
2023-06-15 6:05 ` Varadarajan Narayanan
2023-06-07 10:56 ` [PATCH 6/9] phy: qcom: Add qcom,m31 USB phy driver Varadarajan Narayanan
2023-06-07 18:36 ` Krzysztof Kozlowski
2023-06-15 6:14 ` Varadarajan Narayanan
2023-06-07 10:56 ` [PATCH 7/9] arm64: dts: qcom: ipq5332: Add USB related nodes Varadarajan Narayanan
2023-06-07 11:23 ` Dmitry Baryshkov
2023-06-15 6:26 ` Varadarajan Narayanan
2023-06-07 18:24 ` Konrad Dybcio
2023-06-15 6:52 ` Varadarajan Narayanan
2023-06-07 18:35 ` Krzysztof Kozlowski
2023-06-15 7:17 ` Varadarajan Narayanan
2023-06-07 10:56 ` [PATCH 8/9] arm64: dts: qcom: ipq5332: Enable USB Varadarajan Narayanan
2023-06-07 10:56 ` [PATCH 9/9] arm64: defconfig: Enable QCOM M31 USB phy driver Varadarajan Narayanan
2023-06-07 18:36 ` Krzysztof Kozlowski
2023-06-15 8:37 ` Varadarajan Narayanan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).