[PATCH v6 08/17] dt-bindings: usb: Add H616 compatible string

[PATCH v6 08/17] dt-bindings: usb: Add H616 compatible string

[Andre Przywara]

The H616 has four PHYs as the H3, along with their respective clock
gates and resets, so the property description is identical.

However the PHYs itself need some special bits, so we need a new
compatible string for it.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../devicetree/bindings/phy/allwinner,sun8i-h3-usb-phy.yaml   | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/phy/allwinner,sun8i-h3-usb-phy.yaml b/Documentation/devicetree/bindings/phy/allwinner,sun8i-h3-usb-phy.yaml
index f80431060803..e288450e0844 100644
--- a/Documentation/devicetree/bindings/phy/allwinner,sun8i-h3-usb-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/allwinner,sun8i-h3-usb-phy.yaml
@@ -15,7 +15,9 @@ properties:
     const: 1
 
   compatible:
-    const: allwinner,sun8i-h3-usb-phy
+    enum:
+      - allwinner,sun8i-h3-usb-phy
+      - allwinner,sun50i-h616-usb-phy
 
   reg:
     items:
-- 
2.17.5

[PATCH v6 09/17] dt-bindings: usb: sunxi-musb: Add H616 compatible string

[Andre Przywara]

The H616 MUSB peripheral is compatible to the H3 one (8 endpoints).

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Acked-by: Maxime Ripard <mripard@kernel.org>
---
 .../devicetree/bindings/usb/allwinner,sun4i-a10-musb.yaml      | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/allwinner,sun4i-a10-musb.yaml b/Documentation/devicetree/bindings/usb/allwinner,sun4i-a10-musb.yaml
index 0f520f17735e..933fa356d2ce 100644
--- a/Documentation/devicetree/bindings/usb/allwinner,sun4i-a10-musb.yaml
+++ b/Documentation/devicetree/bindings/usb/allwinner,sun4i-a10-musb.yaml
@@ -22,6 +22,9 @@ properties:
               - allwinner,sun8i-a83t-musb
               - allwinner,sun50i-h6-musb
           - const: allwinner,sun8i-a33-musb
+      - items:
+          - const: allwinner,sun50i-h616-musb
+          - const: allwinner,sun8i-h3-musb
 
   reg:
     maxItems: 1
-- 
2.17.5

[PATCH v6 10/17] phy: sun4i-usb: Rework HCI PHY (aka. "pmu_unk1") handling

[Andre Przywara]

As Icenowy pointed out, newer manuals (starting with H6) actually
document the register block at offset 0x800 as "HCI controller and PHY
interface", also describe the bits in our "PMU_UNK1" register.
Let's put proper names to those "unknown" variables and symbols.

While we are at it, generalise the existing code by allowing a bitmap
of bits to clear and set, to cover newer SoCs: The A100 and H616 use a
different bit for the SIDDQ control.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/phy/allwinner/phy-sun4i-usb.c | 30 ++++++++++++---------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
index 788dd5cdbb7d..142f4cafdc78 100644
--- a/drivers/phy/allwinner/phy-sun4i-usb.c
+++ b/drivers/phy/allwinner/phy-sun4i-usb.c
@@ -43,7 +43,7 @@
 #define REG_PHYCTL_A33			0x10
 #define REG_PHY_OTGCTL			0x20
 
-#define REG_PMU_UNK1			0x10
+#define REG_HCI_PHY_CTL			0x10
 
 #define PHYCTL_DATA			BIT(7)
 
@@ -82,6 +82,7 @@
 /* A83T specific control bits for PHY0 */
 #define PHY_CTL_VBUSVLDEXT		BIT(5)
 #define PHY_CTL_SIDDQ			BIT(3)
+#define PHY_CTL_H3_SIDDQ		BIT(1)
 
 /* A83T specific control bits for PHY2 HSIC */
 #define SUNXI_EHCI_HS_FORCE		BIT(20)
@@ -115,9 +116,9 @@ struct sun4i_usb_phy_cfg {
 	int hsic_index;
 	enum sun4i_usb_phy_type type;
 	u32 disc_thresh;
+	u32 hci_phy_ctl_clear;
 	u8 phyctl_offset;
 	bool dedicated_clocks;
-	bool enable_pmu_unk1;
 	bool phy0_dual_route;
 	int missing_phys;
 };
@@ -288,6 +289,12 @@ static int sun4i_usb_phy_init(struct phy *_phy)
 		return ret;
 	}
 
+	if (phy->pmu && data->cfg->hci_phy_ctl_clear) {
+		val = readl(phy->pmu + REG_HCI_PHY_CTL);
+		val &= ~data->cfg->hci_phy_ctl_clear;
+		writel(val, phy->pmu + REG_HCI_PHY_CTL);
+	}
+
 	if (data->cfg->type == sun8i_a83t_phy ||
 	    data->cfg->type == sun50i_h6_phy) {
 		if (phy->index == 0) {
@@ -297,11 +304,6 @@ static int sun4i_usb_phy_init(struct phy *_phy)
 			writel(val, data->base + data->cfg->phyctl_offset);
 		}
 	} else {
-		if (phy->pmu && data->cfg->enable_pmu_unk1) {
-			val = readl(phy->pmu + REG_PMU_UNK1);
-			writel(val & ~2, phy->pmu + REG_PMU_UNK1);
-		}
-
 		/* Enable USB 45 Ohm resistor calibration */
 		if (phy->index == 0)
 			sun4i_usb_phy_write(phy, PHY_RES45_CAL_EN, 0x01, 1);
@@ -863,7 +865,6 @@ static const struct sun4i_usb_phy_cfg sun4i_a10_cfg = {
 	.disc_thresh = 3,
 	.phyctl_offset = REG_PHYCTL_A10,
 	.dedicated_clocks = false,
-	.enable_pmu_unk1 = false,
 };
 
 static const struct sun4i_usb_phy_cfg sun5i_a13_cfg = {
@@ -872,7 +873,6 @@ static const struct sun4i_usb_phy_cfg sun5i_a13_cfg = {
 	.disc_thresh = 2,
 	.phyctl_offset = REG_PHYCTL_A10,
 	.dedicated_clocks = false,
-	.enable_pmu_unk1 = false,
 };
 
 static const struct sun4i_usb_phy_cfg sun6i_a31_cfg = {
@@ -881,7 +881,6 @@ static const struct sun4i_usb_phy_cfg sun6i_a31_cfg = {
 	.disc_thresh = 3,
 	.phyctl_offset = REG_PHYCTL_A10,
 	.dedicated_clocks = true,
-	.enable_pmu_unk1 = false,
 };
 
 static const struct sun4i_usb_phy_cfg sun7i_a20_cfg = {
@@ -890,7 +889,6 @@ static const struct sun4i_usb_phy_cfg sun7i_a20_cfg = {
 	.disc_thresh = 2,
 	.phyctl_offset = REG_PHYCTL_A10,
 	.dedicated_clocks = false,
-	.enable_pmu_unk1 = false,
 };
 
 static const struct sun4i_usb_phy_cfg sun8i_a23_cfg = {
@@ -899,7 +897,6 @@ static const struct sun4i_usb_phy_cfg sun8i_a23_cfg = {
 	.disc_thresh = 3,
 	.phyctl_offset = REG_PHYCTL_A10,
 	.dedicated_clocks = true,
-	.enable_pmu_unk1 = false,
 };
 
 static const struct sun4i_usb_phy_cfg sun8i_a33_cfg = {
@@ -908,7 +905,6 @@ static const struct sun4i_usb_phy_cfg sun8i_a33_cfg = {
 	.disc_thresh = 3,
 	.phyctl_offset = REG_PHYCTL_A33,
 	.dedicated_clocks = true,
-	.enable_pmu_unk1 = false,
 };
 
 static const struct sun4i_usb_phy_cfg sun8i_a83t_cfg = {
@@ -925,7 +921,7 @@ static const struct sun4i_usb_phy_cfg sun8i_h3_cfg = {
 	.disc_thresh = 3,
 	.phyctl_offset = REG_PHYCTL_A33,
 	.dedicated_clocks = true,
-	.enable_pmu_unk1 = true,
+	.hci_phy_ctl_clear = PHY_CTL_H3_SIDDQ,
 	.phy0_dual_route = true,
 };
 
@@ -935,7 +931,7 @@ static const struct sun4i_usb_phy_cfg sun8i_r40_cfg = {
 	.disc_thresh = 3,
 	.phyctl_offset = REG_PHYCTL_A33,
 	.dedicated_clocks = true,
-	.enable_pmu_unk1 = true,
+	.hci_phy_ctl_clear = PHY_CTL_H3_SIDDQ,
 	.phy0_dual_route = true,
 };
 
@@ -945,7 +941,7 @@ static const struct sun4i_usb_phy_cfg sun8i_v3s_cfg = {
 	.disc_thresh = 3,
 	.phyctl_offset = REG_PHYCTL_A33,
 	.dedicated_clocks = true,
-	.enable_pmu_unk1 = true,
+	.hci_phy_ctl_clear = PHY_CTL_H3_SIDDQ,
 	.phy0_dual_route = true,
 };
 
@@ -955,7 +951,7 @@ static const struct sun4i_usb_phy_cfg sun50i_a64_cfg = {
 	.disc_thresh = 3,
 	.phyctl_offset = REG_PHYCTL_A33,
 	.dedicated_clocks = true,
-	.enable_pmu_unk1 = true,
+	.hci_phy_ctl_clear = PHY_CTL_H3_SIDDQ,
 	.phy0_dual_route = true,
 };
 
-- 
2.17.5

[PATCH v6 11/17] phy: sun4i-usb: Allow reset line to be shared

[Andre Przywara]

The USB HCIs (and PHYs?) in Allwinner's newer generation SoCs (H616)
rely on the reset line of USB PHY 2 to be de-asserted, even when only
one of the other PHYs is actually in use.

To make those ports work, we include this reset line in the HCIs' resets
property, which requires this line to be shareable.

Change the call to allocate the reset line to mark it as shared, to
enable the other ports on those SoCs.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/phy/allwinner/phy-sun4i-usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
index 142f4cafdc78..126ef74d013c 100644
--- a/drivers/phy/allwinner/phy-sun4i-usb.c
+++ b/drivers/phy/allwinner/phy-sun4i-usb.c
@@ -788,7 +788,7 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
 		}
 
 		snprintf(name, sizeof(name), "usb%d_reset", i);
-		phy->reset = devm_reset_control_get(dev, name);
+		phy->reset = devm_reset_control_get_shared(dev, name);
 		if (IS_ERR(phy->reset)) {
 			dev_err(dev, "failed to get reset %s\n", name);
 			return PTR_ERR(phy->reset);
-- 
2.17.5

[PATCH v6 12/17] phy: sun4i-usb: Introduce port2 SIDDQ quirk

[Andre Przywara]

At least the Allwinner H616 SoC requires a weird quirk to make most
USB PHYs work: Only port2 works out of the box, but all other ports
need some help from this port2 to work correctly: The CLK_BUS_PHY2 and
RST_USB_PHY2 clock and reset need to be enabled, and the SIDDQ bit in
the PMU PHY control register needs to be cleared. For this register to
be accessible, CLK_BUS_ECHI2 needs to be ungated. Don't ask ....

Instead of disguising this as some generic feature, do exactly that
in our PHY init:
If the quirk bit is set, and we initialise a PHY other than PHY2, ungate
this one special clock, and clear the SIDDQ bit. We can pull in the
other required clocks via the DT.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/phy/allwinner/phy-sun4i-usb.c | 29 +++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
index 126ef74d013c..ed7b9cc5a424 100644
--- a/drivers/phy/allwinner/phy-sun4i-usb.c
+++ b/drivers/phy/allwinner/phy-sun4i-usb.c
@@ -120,6 +120,7 @@ struct sun4i_usb_phy_cfg {
 	u8 phyctl_offset;
 	bool dedicated_clocks;
 	bool phy0_dual_route;
+	bool needs_phy2_siddq;
 	int missing_phys;
 };
 
@@ -331,6 +332,27 @@ static int sun4i_usb_phy_init(struct phy *_phy)
 		queue_delayed_work(system_wq, &data->detect, 0);
 	}
 
+	/* Some PHYs on some SoCs need the help of PHY2 to work. */
+	if (data->cfg->needs_phy2_siddq && phy->index != 2) {
+		struct sun4i_usb_phy *phy2 = &data->phys[2];
+
+		/*
+		 * This extra clock is just needed to access the
+		 * REG_HCI_PHY_CTL PMU register for PHY2.
+		 */
+		ret = clk_prepare_enable(phy2->clk2);
+		if (ret)
+			return ret;
+
+		if (phy2->pmu && data->cfg->hci_phy_ctl_clear) {
+			val = readl(phy2->pmu + REG_HCI_PHY_CTL);
+			val &= ~data->cfg->hci_phy_ctl_clear;
+			writel(val, phy2->pmu + REG_HCI_PHY_CTL);
+		}
+
+		clk_disable_unprepare(phy->clk2);
+	}
+
 	return 0;
 }
 
@@ -785,6 +807,13 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
 				dev_err(dev, "failed to get clock %s\n", name);
 				return PTR_ERR(phy->clk2);
 			}
+		} else {
+			snprintf(name, sizeof(name), "pmu%d_clk", i);
+			phy->clk2 = devm_clk_get_optional(dev, name);
+			if (IS_ERR(phy->clk2)) {
+				dev_err(dev, "failed to get clock %s\n", name);
+				return PTR_ERR(phy->clk2);
+			}
 		}
 
 		snprintf(name, sizeof(name), "usb%d_reset", i);
-- 
2.17.5

[PATCH v6 13/17] phy: sun4i-usb: Add support for the H616 USB PHY

[Andre Przywara]

The USB PHY used in the Allwinner H616 SoC inherits some traits from its
various predecessors: it has four full PHYs like the H3, needs some
extra bits to be set like the H6, and puts SIDDQ on a different bit like
the A100. Plus it needs this weird PHY2 quirk.

Name all those properties in a new config struct and assign a new
compatible name to it.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/phy/allwinner/phy-sun4i-usb.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
index ed7b9cc5a424..a55765ad7bad 100644
--- a/drivers/phy/allwinner/phy-sun4i-usb.c
+++ b/drivers/phy/allwinner/phy-sun4i-usb.c
@@ -994,6 +994,17 @@ static const struct sun4i_usb_phy_cfg sun50i_h6_cfg = {
 	.missing_phys = BIT(1) | BIT(2),
 };
 
+static const struct sun4i_usb_phy_cfg sun50i_h616_cfg = {
+	.num_phys = 4,
+	.type = sun50i_h6_phy,
+	.disc_thresh = 3,
+	.phyctl_offset = REG_PHYCTL_A33,
+	.dedicated_clocks = true,
+	.phy0_dual_route = true,
+	.hci_phy_ctl_clear = PHY_CTL_SIDDQ,
+	.needs_phy2_siddq = true,
+};
+
 static const struct of_device_id sun4i_usb_phy_of_match[] = {
 	{ .compatible = "allwinner,sun4i-a10-usb-phy", .data = &sun4i_a10_cfg },
 	{ .compatible = "allwinner,sun5i-a13-usb-phy", .data = &sun5i_a13_cfg },
@@ -1008,6 +1019,7 @@ static const struct of_device_id sun4i_usb_phy_of_match[] = {
 	{ .compatible = "allwinner,sun50i-a64-usb-phy",
 	  .data = &sun50i_a64_cfg},
 	{ .compatible = "allwinner,sun50i-h6-usb-phy", .data = &sun50i_h6_cfg },
+	{ .compatible = "allwinner,sun50i-h616-usb-phy", .data = &sun50i_h616_cfg },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, sun4i_usb_phy_of_match);
-- 
2.17.5

Re: [PATCH v6 08/17] dt-bindings: usb: Add H616 compatible string

[Rob Herring]

On Wed, 19 May 2021 11:41:43 +0100, Andre Przywara wrote:
> The H616 has four PHYs as the H3, along with their respective clock
> gates and resets, so the property description is identical.
> 
> However the PHYs itself need some special bits, so we need a new
> compatible string for it.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../devicetree/bindings/phy/allwinner,sun8i-h3-usb-phy.yaml   | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v6 09/17] dt-bindings: usb: sunxi-musb: Add H616 compatible string

[Rob Herring]

On Wed, 19 May 2021 11:41:44 +0100, Andre Przywara wrote:
> The H616 MUSB peripheral is compatible to the H3 one (8 endpoints).
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Acked-by: Maxime Ripard <mripard@kernel.org>
> ---
>  .../devicetree/bindings/usb/allwinner,sun4i-a10-musb.yaml      | 3 +++
>  1 file changed, 3 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v6 12/17] phy: sun4i-usb: Introduce port2 SIDDQ quirk

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 970 bytes --]

Hi

On Wed, May 19, 2021 at 11:41:47AM +0100, Andre Przywara wrote:
> At least the Allwinner H616 SoC requires a weird quirk to make most
> USB PHYs work: Only port2 works out of the box, but all other ports
> need some help from this port2 to work correctly: The CLK_BUS_PHY2 and
> RST_USB_PHY2 clock and reset need to be enabled, and the SIDDQ bit in
> the PMU PHY control register needs to be cleared. For this register to
> be accessible, CLK_BUS_ECHI2 needs to be ungated. Don't ask ....
> 
> Instead of disguising this as some generic feature, do exactly that
> in our PHY init:
> If the quirk bit is set, and we initialise a PHY other than PHY2, ungate
> this one special clock, and clear the SIDDQ bit. We can pull in the
> other required clocks via the DT.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

What is this SIDDQ bit doing exactly?

I guess we could also expose this using a power-domain if it's relevant?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 12/17] phy: sun4i-usb: Introduce port2 SIDDQ quirk

[Jernej Škrabec]

Dne ponedeljek, 24. maj 2021 ob 13:59:46 CEST je Maxime Ripard napisal(a):
> Hi
> 
> On Wed, May 19, 2021 at 11:41:47AM +0100, Andre Przywara wrote:
> > At least the Allwinner H616 SoC requires a weird quirk to make most
> > USB PHYs work: Only port2 works out of the box, but all other ports
> > need some help from this port2 to work correctly: The CLK_BUS_PHY2 and
> > RST_USB_PHY2 clock and reset need to be enabled, and the SIDDQ bit in
> > the PMU PHY control register needs to be cleared. For this register to
> > be accessible, CLK_BUS_ECHI2 needs to be ungated. Don't ask ....
> > 
> > Instead of disguising this as some generic feature, do exactly that
> > in our PHY init:
> > If the quirk bit is set, and we initialise a PHY other than PHY2, ungate
> > this one special clock, and clear the SIDDQ bit. We can pull in the
> > other required clocks via the DT.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> 
> What is this SIDDQ bit doing exactly?

If this is similar to Rockchip USB PHY, then this bit takes care for powering 
up/down analog parts of USB PHY:
https://elixir.bootlin.com/linux/latest/source/drivers/phy/rockchip/phy-rockchip-usb.c#L83

Best regards,
Jernej

> 
> I guess we could also expose this using a power-domain if it's relevant?
> 
> Maxime

Re: [PATCH v6 12/17] phy: sun4i-usb: Introduce port2 SIDDQ quirk

[Andre Przywara]

On Mon, 24 May 2021 13:59:46 +0200
Maxime Ripard <maxime@cerno.tech> wrote:

Hi Maxime,

> On Wed, May 19, 2021 at 11:41:47AM +0100, Andre Przywara wrote:
> > At least the Allwinner H616 SoC requires a weird quirk to make most
> > USB PHYs work: Only port2 works out of the box, but all other ports
> > need some help from this port2 to work correctly: The CLK_BUS_PHY2 and
> > RST_USB_PHY2 clock and reset need to be enabled, and the SIDDQ bit in
> > the PMU PHY control register needs to be cleared. For this register to
> > be accessible, CLK_BUS_ECHI2 needs to be ungated. Don't ask ....
> > 
> > Instead of disguising this as some generic feature, do exactly that
> > in our PHY init:
> > If the quirk bit is set, and we initialise a PHY other than PHY2, ungate
> > this one special clock, and clear the SIDDQ bit. We can pull in the
> > other required clocks via the DT.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>  
> 
> What is this SIDDQ bit doing exactly?

I probably know as much as you do, but as Jernej pointed out, in some
Rockchip code it's indeed documented as some analogue PHY supply switch:
($ git grep -i siddq drivers/phy/rockchip)

In fact we had this pin/bit for ages, it was just hidden as BIT(1) in
our infamous PMU_UNK1 register. Patch 10/17 drags that into the light.

> I guess we could also expose this using a power-domain if it's relevant?

Mmmh, interesting idea. So are you thinking about registering a genpd
provider in sun4i_usb_phy_probe(), then having a power-domains property
in the ehci/ohci nodes, pointing to the PHY node? And if yes, should
the provider be a subnode of the USB PHY node, with a separate
compatible? That sounds a bit more involved, but would have the
advantage of allowing us to specify the resets and clocks from PHY2
there, and would look a bit cleaner than hacking them into the
other EHCI/OHCI nodes.

I would not touch the existing SoCs (even though it seems to apply to
them as well, just not in the exact same way), but I can give it a
try for the H616. It seems like the other SIDDQ bits (in the other
PHYs) are still needed for operation, but the PD provide could actually
take care of this as well.

Does that make sense or is this a bit over the top for just clearing an
extra bit?

Cheers,
Andre

Re: [PATCH v6 12/17] phy: sun4i-usb: Introduce port2 SIDDQ quirk

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2862 bytes --]

Hi,

On Tue, May 25, 2021 at 12:29:01PM +0100, Andre Przywara wrote:
> On Mon, 24 May 2021 13:59:46 +0200
> Maxime Ripard <maxime@cerno.tech> wrote:
> 
> Hi Maxime,
> 
> > On Wed, May 19, 2021 at 11:41:47AM +0100, Andre Przywara wrote:
> > > At least the Allwinner H616 SoC requires a weird quirk to make most
> > > USB PHYs work: Only port2 works out of the box, but all other ports
> > > need some help from this port2 to work correctly: The CLK_BUS_PHY2 and
> > > RST_USB_PHY2 clock and reset need to be enabled, and the SIDDQ bit in
> > > the PMU PHY control register needs to be cleared. For this register to
> > > be accessible, CLK_BUS_ECHI2 needs to be ungated. Don't ask ....
> > > 
> > > Instead of disguising this as some generic feature, do exactly that
> > > in our PHY init:
> > > If the quirk bit is set, and we initialise a PHY other than PHY2, ungate
> > > this one special clock, and clear the SIDDQ bit. We can pull in the
> > > other required clocks via the DT.
> > > 
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>  
> > 
> > What is this SIDDQ bit doing exactly?
> 
> I probably know as much as you do, but as Jernej pointed out, in some
> Rockchip code it's indeed documented as some analogue PHY supply switch:
> ($ git grep -i siddq drivers/phy/rockchip)
> 
> In fact we had this pin/bit for ages, it was just hidden as BIT(1) in
> our infamous PMU_UNK1 register. Patch 10/17 drags that into the light.

Ok

> > I guess we could also expose this using a power-domain if it's relevant?
> 
> Mmmh, interesting idea. So are you thinking about registering a genpd
> provider in sun4i_usb_phy_probe(), then having a power-domains property
> in the ehci/ohci nodes, pointing to the PHY node? And if yes, should
> the provider be a subnode of the USB PHY node, with a separate
> compatible? That sounds a bit more involved, but would have the
> advantage of allowing us to specify the resets and clocks from PHY2
> there, and would look a bit cleaner than hacking them into the
> other EHCI/OHCI nodes.

I'm not sure we need a separate device node, we could just register the
phy driver as a genpd provider, and then with an arg (so with
of_genpd_add_provider_onecell?) the index of the USB controller we want
to power up.

> I would not touch the existing SoCs (even though it seems to apply to
> them as well, just not in the exact same way), but I can give it a
> try for the H616. It seems like the other SIDDQ bits (in the other
> PHYs) are still needed for operation, but the PD provide could actually
> take care of this as well.
> 
> Does that make sense or is this a bit over the top for just clearing an
> extra bit?

Using what I described above should be fairly simple, so if we can fit
in an available and relevant abstraction, yeah, I guess :)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 12/17] phy: sun4i-usb: Introduce port2 SIDDQ quirk

[Andre Przywara]

On Mon, 7 Jun 2021 15:22:55 +0200
Maxime Ripard <maxime@cerno.tech> wrote:

Hi Maxime,

> On Tue, May 25, 2021 at 12:29:01PM +0100, Andre Przywara wrote:
> > On Mon, 24 May 2021 13:59:46 +0200
> > Maxime Ripard <maxime@cerno.tech> wrote:
> > 
> > Hi Maxime,
> >   
> > > On Wed, May 19, 2021 at 11:41:47AM +0100, Andre Przywara wrote:  
> > > > At least the Allwinner H616 SoC requires a weird quirk to make most
> > > > USB PHYs work: Only port2 works out of the box, but all other ports
> > > > need some help from this port2 to work correctly: The CLK_BUS_PHY2 and
> > > > RST_USB_PHY2 clock and reset need to be enabled, and the SIDDQ bit in
> > > > the PMU PHY control register needs to be cleared. For this register to
> > > > be accessible, CLK_BUS_ECHI2 needs to be ungated. Don't ask ....
> > > > 
> > > > Instead of disguising this as some generic feature, do exactly that
> > > > in our PHY init:
> > > > If the quirk bit is set, and we initialise a PHY other than PHY2, ungate
> > > > this one special clock, and clear the SIDDQ bit. We can pull in the
> > > > other required clocks via the DT.
> > > > 
> > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>    
> > > 
> > > What is this SIDDQ bit doing exactly?  
> > 
> > I probably know as much as you do, but as Jernej pointed out, in some
> > Rockchip code it's indeed documented as some analogue PHY supply switch:
> > ($ git grep -i siddq drivers/phy/rockchip)
> > 
> > In fact we had this pin/bit for ages, it was just hidden as BIT(1) in
> > our infamous PMU_UNK1 register. Patch 10/17 drags that into the light.  
> 
> Ok
> 
> > > I guess we could also expose this using a power-domain if it's relevant?  
> > 
> > Mmmh, interesting idea. So are you thinking about registering a genpd
> > provider in sun4i_usb_phy_probe(), then having a power-domains property
> > in the ehci/ohci nodes, pointing to the PHY node? And if yes, should
> > the provider be a subnode of the USB PHY node, with a separate
> > compatible? That sounds a bit more involved, but would have the
> > advantage of allowing us to specify the resets and clocks from PHY2
> > there, and would look a bit cleaner than hacking them into the
> > other EHCI/OHCI nodes.  
> 
> I'm not sure we need a separate device node, we could just register the
> phy driver as a genpd provider, and then with an arg (so with
> of_genpd_add_provider_onecell?) the index of the USB controller we want
> to power up.

Yeah, I figured that myself meanwhile ;-) I now have a fairly nice
implementation, which does away with the extra clocks and resets from
the EHCI/OHCI nodes, and just adds one extra clock to the PHY node. The
rest is power domains properties.

> > I would not touch the existing SoCs (even though it seems to apply to
> > them as well, just not in the exact same way), but I can give it a
> > try for the H616. It seems like the other SIDDQ bits (in the other
> > PHYs) are still needed for operation, but the PD provide could actually
> > take care of this as well.
> > 
> > Does that make sense or is this a bit over the top for just clearing an
> > extra bit?  
> 
> Using what I described above should be fairly simple, so if we can fit
> in an available and relevant abstraction, yeah, I guess :)

Thanks!
I will post what I have, just need to find some solution for the RTC
clock bits.

Cheers,
Andre

Re: [linux-sunxi] Re: [PATCH v6 12/17] phy: sun4i-usb: Introduce port2 SIDDQ quirk

[Chen-Yu Tsai]

Hi,

On Mon, Jun 7, 2021 at 10:17 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Mon, 7 Jun 2021 15:22:55 +0200
> Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi Maxime,
>
> > On Tue, May 25, 2021 at 12:29:01PM +0100, Andre Przywara wrote:
> > > On Mon, 24 May 2021 13:59:46 +0200
> > > Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > Hi Maxime,
> > >
> > > > On Wed, May 19, 2021 at 11:41:47AM +0100, Andre Przywara wrote:
> > > > > At least the Allwinner H616 SoC requires a weird quirk to make most
> > > > > USB PHYs work: Only port2 works out of the box, but all other ports
> > > > > need some help from this port2 to work correctly: The CLK_BUS_PHY2 and
> > > > > RST_USB_PHY2 clock and reset need to be enabled, and the SIDDQ bit in
> > > > > the PMU PHY control register needs to be cleared. For this register to
> > > > > be accessible, CLK_BUS_ECHI2 needs to be ungated. Don't ask ....
> > > > >
> > > > > Instead of disguising this as some generic feature, do exactly that
> > > > > in our PHY init:
> > > > > If the quirk bit is set, and we initialise a PHY other than PHY2, ungate
> > > > > this one special clock, and clear the SIDDQ bit. We can pull in the
> > > > > other required clocks via the DT.
> > > > >
> > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > >
> > > > What is this SIDDQ bit doing exactly?
> > >
> > > I probably know as much as you do, but as Jernej pointed out, in some
> > > Rockchip code it's indeed documented as some analogue PHY supply switch:
> > > ($ git grep -i siddq drivers/phy/rockchip)
> > >
> > > In fact we had this pin/bit for ages, it was just hidden as BIT(1) in
> > > our infamous PMU_UNK1 register. Patch 10/17 drags that into the light.
> >
> > Ok
> >
> > > > I guess we could also expose this using a power-domain if it's relevant?
> > >
> > > Mmmh, interesting idea. So are you thinking about registering a genpd
> > > provider in sun4i_usb_phy_probe(), then having a power-domains property
> > > in the ehci/ohci nodes, pointing to the PHY node? And if yes, should
> > > the provider be a subnode of the USB PHY node, with a separate
> > > compatible? That sounds a bit more involved, but would have the
> > > advantage of allowing us to specify the resets and clocks from PHY2
> > > there, and would look a bit cleaner than hacking them into the
> > > other EHCI/OHCI nodes.
> >
> > I'm not sure we need a separate device node, we could just register the
> > phy driver as a genpd provider, and then with an arg (so with
> > of_genpd_add_provider_onecell?) the index of the USB controller we want
> > to power up.
>
> Yeah, I figured that myself meanwhile ;-) I now have a fairly nice
> implementation, which does away with the extra clocks and resets from
> the EHCI/OHCI nodes, and just adds one extra clock to the PHY node. The
> rest is power domains properties.

I'm wondering, since SIDDQ refers to the analog power for USB, it shouldn't
really affect the HCIs, only the PHYs. Is there any way to model it like
this and test it?

ChenYu

> > > I would not touch the existing SoCs (even though it seems to apply to
> > > them as well, just not in the exact same way), but I can give it a
> > > try for the H616. It seems like the other SIDDQ bits (in the other
> > > PHYs) are still needed for operation, but the PD provide could actually
> > > take care of this as well.
> > >
> > > Does that make sense or is this a bit over the top for just clearing an
> > > extra bit?
> >
> > Using what I described above should be fairly simple, so if we can fit
> > in an available and relevant abstraction, yeah, I guess :)
>
> Thanks!
> I will post what I have, just need to find some solution for the RTC
> clock bits.
>
> Cheers,
> Andre
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20210607151742.2f05ff95%40slackpad.fritz.box.

Re: [linux-sunxi] Re: [PATCH v6 12/17] phy: sun4i-usb: Introduce port2 SIDDQ quirk

[Andre Przywara]

On Mon, 7 Jun 2021 22:26:02 +0800
Chen-Yu Tsai <wens@csie.org> wrote:

Hi Chen-Yu,

> On Mon, Jun 7, 2021 at 10:17 PM Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > On Mon, 7 Jun 2021 15:22:55 +0200
> > Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Hi Maxime,
> >  
> > > On Tue, May 25, 2021 at 12:29:01PM +0100, Andre Przywara wrote:  
> > > > On Mon, 24 May 2021 13:59:46 +0200
> > > > Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > Hi Maxime,
> > > >  
> > > > > On Wed, May 19, 2021 at 11:41:47AM +0100, Andre Przywara wrote:  
> > > > > > At least the Allwinner H616 SoC requires a weird quirk to make most
> > > > > > USB PHYs work: Only port2 works out of the box, but all other ports
> > > > > > need some help from this port2 to work correctly: The CLK_BUS_PHY2 and
> > > > > > RST_USB_PHY2 clock and reset need to be enabled, and the SIDDQ bit in
> > > > > > the PMU PHY control register needs to be cleared. For this register to
> > > > > > be accessible, CLK_BUS_ECHI2 needs to be ungated. Don't ask ....
> > > > > >
> > > > > > Instead of disguising this as some generic feature, do exactly that
> > > > > > in our PHY init:
> > > > > > If the quirk bit is set, and we initialise a PHY other than PHY2, ungate
> > > > > > this one special clock, and clear the SIDDQ bit. We can pull in the
> > > > > > other required clocks via the DT.
> > > > > >
> > > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>  
> > > > >
> > > > > What is this SIDDQ bit doing exactly?  
> > > >
> > > > I probably know as much as you do, but as Jernej pointed out, in some
> > > > Rockchip code it's indeed documented as some analogue PHY supply switch:
> > > > ($ git grep -i siddq drivers/phy/rockchip)
> > > >
> > > > In fact we had this pin/bit for ages, it was just hidden as BIT(1) in
> > > > our infamous PMU_UNK1 register. Patch 10/17 drags that into the light.  
> > >
> > > Ok
> > >  
> > > > > I guess we could also expose this using a power-domain if it's relevant?  
> > > >
> > > > Mmmh, interesting idea. So are you thinking about registering a genpd
> > > > provider in sun4i_usb_phy_probe(), then having a power-domains property
> > > > in the ehci/ohci nodes, pointing to the PHY node? And if yes, should
> > > > the provider be a subnode of the USB PHY node, with a separate
> > > > compatible? That sounds a bit more involved, but would have the
> > > > advantage of allowing us to specify the resets and clocks from PHY2
> > > > there, and would look a bit cleaner than hacking them into the
> > > > other EHCI/OHCI nodes.  
> > >
> > > I'm not sure we need a separate device node, we could just register the
> > > phy driver as a genpd provider, and then with an arg (so with
> > > of_genpd_add_provider_onecell?) the index of the USB controller we want
> > > to power up.  
> >
> > Yeah, I figured that myself meanwhile ;-) I now have a fairly nice
> > implementation, which does away with the extra clocks and resets from
> > the EHCI/OHCI nodes, and just adds one extra clock to the PHY node. The
> > rest is power domains properties.  
> 
> I'm wondering, since SIDDQ refers to the analog power for USB, it shouldn't
> really affect the HCIs, only the PHYs. Is there any way to model it like
> this and test it?

That is actually a good point: it is indeed solely a PHY property. The
HCIs already connect to their PHYs, among other reasons to power them
on, so it should really be an internal PHY affair.
Which is actually what this patch here does.
So I can combine the best of both approaches: we already have the
PHY2 clock and reset references in the PHY node, so can just reach out
and enable them as well, alongside the actually associated PHY clock.
This allows us to get rid of the bogus PHY2 clock references from all
HCIs.
So all of this H616 quirk can be fully contained within the PHY driver,
with no impact on the HCI parts and no extra DT properties
(power-domains, clocks) needed there.

Seems to work on a quick test. I will send a version ASAP.

Thanks!
Andre

> 
> ChenYu
> 
> > > > I would not touch the existing SoCs (even though it seems to apply to
> > > > them as well, just not in the exact same way), but I can give it a
> > > > try for the H616. It seems like the other SIDDQ bits (in the other
> > > > PHYs) are still needed for operation, but the PD provide could actually
> > > > take care of this as well.
> > > >
> > > > Does that make sense or is this a bit over the top for just clearing an
> > > > extra bit?  
> > >
> > > Using what I described above should be fairly simple, so if we can fit
> > > in an available and relevant abstraction, yeah, I guess :)  
> >
> > Thanks!
> > I will post what I have, just need to find some solution for the RTC
> > clock bits.
> >
> > Cheers,
> > Andre
> >