usb/phy: Add Amlogic Meson8b and GXBB USB support

USB0_DDR_BRIDGE and USB1_DDR_BRIDGE1 are needed for the related dwc2 usb controller. USB, USB0 and USB1 are needed for the PHYs. Expose these clocks to DT and comment out in clk driver.

Signed-off-by Jerome Brunet <[email protected]>

Signed-off-by Martin Blumenstingl <[email protected]>

Quoted 6 lines · Martin Blumenstingl

USB0_DDR_BRIDGE and USB1_DDR_BRIDGE1 are needed for the related dwc2 usb controller. USB, USB0 and USB1 are needed for the PHYs. Expose these clocks to DT and comment out in clk driver.

Signed-off-by Jerome Brunet <[email protected]>

Signed-off-by Martin Blumenstingl <[email protected]>

Is authorship correct on this patch? Did Jerome author it instead?

Quoted 8 lines · Stephen Boyd

Quoted 6 lines · Martin Blumenstingl

USB0_DDR_BRIDGE and USB1_DDR_BRIDGE1 are needed for the related dwc2 usb controller. USB, USB0 and USB1 are needed for the PHYs. Expose these clocks to DT and comment out in clk driver.

Signed-off-by Jerome Brunet <[email protected]>

Signed-off-by Martin Blumenstingl <[email protected]>

Quoted 2 lines

Is authorship correct on this patch? Did Jerome author it instead?

We (Jerome and I) have both worked on this patch, that's why you have two signed-off-by's.
Or is this simply about the order (author = from address should be listed first)?

Quoted 9 lines · Martin Blumenstingl

Quoted 8 lines · Stephen Boyd

Quoted 6 lines · Martin Blumenstingl

USB0_DDR_BRIDGE and USB1_DDR_BRIDGE1 are needed for the related dwc2 usb controller. USB, USB0 and USB1 are needed for the PHYs. Expose these clocks to DT and comment out in clk driver.

Signed-off-by Jerome Brunet <[email protected]>

Signed-off-by Martin Blumenstingl <[email protected]>

Quoted 6 lines

Quoted 2 lines

Is authorship correct on this patch? Did Jerome author it instead?

We (Jerome and I) have both worked on this patch, that's why you have two signed-off-by's.
Or is this simply about the order (author = from address should be listed first)?

Yes. Typically author is listed first in the signed-off-by chain.

USB0_DDR_BRIDGE and USB1_DDR_BRIDGE1 are needed for the related dwc2 usb controller. USB, USB0 and USB1 are needed for the PHYs. Expose these clocks to DT and comment out in clk driver.

Signed-off-by Jerome Brunet <[email protected]>

Signed-off-by Martin Blumenstingl <[email protected]>

---

Quoted 4 lines · Martin Blumenstingl

USB0_DDR_BRIDGE and USB1_DDR_BRIDGE1 are needed for the related dwc2 usb controller. USB, USB0 and USB1 are needed for the PHYs. Expose these clocks to DT and comment out in clk driver.

Quoted 3 lines

Signed-off-by Jerome Brunet <[email protected]>

Signed-off-by Martin Blumenstingl <[email protected]>

---

Assuming authorship is resolved:

Acked-by Stephen Boyd <[email protected]>

From: Jerome Brunet <[email protected]>

Add compatible strings for amlogic Meson8b and GXBB SoCs with the corresponding configuration parameters.

Signed-off-by Martin Blumenstingl <[email protected]>

Signed-off-by Jerome Brunet <[email protected]>

---
 Documentation/devicetree/bindings/usb/dwc2.txt |  2 ++
 drivers/usb/dwc2/platform.c                    | 34 ++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

Documentation/devicetree/bindings/usb/dwc2.txt +2 -0

index 20a68bf..2c30a54 100644
@@ -10,6 +10,8 @@ Required properties:
   - "rockchip,rk3288-usb", "rockchip,rk3066-usb", "snps,dwc2": for rk3288 Soc;
   - "lantiq,arx100-usb": The DWC2 USB controller instance in Lantiq ARX SoCs;
   - "lantiq,xrx200-usb": The DWC2 USB controller instance in Lantiq XRX SoCs;
+  - "amlogic,meson8b-usb": The DWC2 USB controller instance in Amlogic Meson8b SoCs;
+  - "amlogic,meson-gxbb-usb": The DWC2 USB controller instance in Amlogic S905 SoCs;
   - snps,dwc2: A generic DWC2 USB controller with default parameters.
 - reg : Should contain 1 register range (address and length)
 - interrupts : Should contain 1 interrupt

drivers/usb/dwc2/platform.c +32 -0

index fc6f525..8f7b34c 100644
@@ -181,6 +181,38 @@ static const struct dwc2_core_params params_ltq = {
 	.hibernation			= -1,
 };
 
+static const struct dwc2_core_params params_amlogic = {
+	.otg_cap			= DWC2_CAP_PARAM_NO_HNP_SRP_CAPABLE,
+	.otg_ver			= -1,
+	.dma_enable			= 1,
+	.dma_desc_enable		= 0,
+	.dma_desc_fs_enable		= 0,
+	.speed				= DWC2_SPEED_PARAM_HIGH,
+	.enable_dynamic_fifo		= 1,
+	.en_multiple_tx_fifo		= -1,
+	.host_rx_fifo_size		= 512,
+	.host_nperio_tx_fifo_size	= 500,
+	.host_perio_tx_fifo_size	= 500,
+	.max_transfer_size		= -1,
+	.max_packet_count		= -1,
+	.host_channels			= 16,
+	.phy_type			= DWC2_PHY_TYPE_PARAM_UTMI,
+	.phy_utmi_width			= -1,
+	.phy_ulpi_ddr			= -1,
+	.phy_ulpi_ext_vbus		= -1,
+	.i2c_enable			= -1,
+	.ulpi_fs_ls			= -1,
+	.host_support_fs_ls_low_power	= -1,
+	.host_ls_low_power_phy_clk	= -1,
+	.ts_dline			= -1,
+	.reload_ctl			= 1,
+	.ahbcfg				= GAHBCFG_HBSTLEN_INCR8 <<
+					  GAHBCFG_HBSTLEN_SHIFT,
+	.uframe_sched			= 0,
+	.external_id_pin_ctl		= -1,
+	.hibernation			= -1,
+};
+
 /*
  * Check the dr_mode against the module configuration and hardware
  * capabilities.

Quoted 5 lines · Martin Blumenstingl

From: Jerome Brunet <[email protected]>

Add compatible strings for amlogic Meson8b and GXBB SoCs with the corresponding configuration parameters.

Quoted 7 lines

Signed-off-by Martin Blumenstingl <[email protected]>

Signed-off-by Jerome Brunet <[email protected]>

---
 Documentation/devicetree/bindings/usb/dwc2.txt |  2 ++
 drivers/usb/dwc2/platform.c                    | 34 ++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

Hi,

I have a patch series in the works that deprecates the usage of these static parameters in favor of devicetree properties. I'll try to push it out soon. I would rather not see any more static params structures in the driver so hopefully we can make it work for your platform.

This is a new driver for the USB PHY found in Meson8b and GXBB SoCs.

Signed-off-by Martin Blumenstingl <[email protected]>

Signed-off-by Jerome Brunet <[email protected]>

drivers/phy/phy-meson-usb2.c new file +35 -0

+static int phy_meson_usb2_probe(struct platform_device *pdev)
+{
+	struct phy_meson_usb2_priv *priv;
+	struct resource *res;
+	struct phy *phy;
+	struct phy_provider *phy_provider;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->regs))
+		return PTR_ERR(priv->regs);
+
+	priv->clk_usb_general = devm_clk_get(&pdev->dev, "usb_general");
+	if (IS_ERR(priv->clk_usb_general))
+		return PTR_ERR(priv->clk_usb_general);
+
+	priv->clk_usb = devm_clk_get(&pdev->dev, "usb");
+	if (IS_ERR(priv->clk_usb))
+		return PTR_ERR(priv->clk_usb);
+
+	priv->dr_mode = of_usb_get_dr_mode_by_phy(pdev->dev.of_node, -1);
+	if (priv->dr_mode == USB_DR_MODE_UNKNOWN) {
+		dev_err(&pdev->dev,
+			"missing dual role configuration of the controller\n");
+		return -EINVAL;
+	}
+
+	phy = devm_phy_create(&pdev->dev, NULL, &phy_meson_usb2_ops);
+	if (IS_ERR(phy)) {
+		dev_err(&pdev->dev, "failed to create PHY\n");

Quoted 4 lines · Martin Blumenstingl

This is a new driver for the USB PHY found in Meson8b and GXBB SoCs.

Signed-off-by Martin Blumenstingl <[email protected]>

Signed-off-by Jerome Brunet <[email protected]>

I tested this on meson-gxbb-p200 with USB host and a mass storage device.

Tested-by Kevin Hilman <[email protected]>

A minor question/comment below, for you and for Kishon...

[...]

Quoted 35 lines

drivers/phy/phy-meson-usb2.c new file +35 -0

+static int phy_meson_usb2_probe(struct platform_device *pdev)
+{
+	struct phy_meson_usb2_priv *priv;
+	struct resource *res;
+	struct phy *phy;
+	struct phy_provider *phy_provider;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->regs))
+		return PTR_ERR(priv->regs);
+
+	priv->clk_usb_general = devm_clk_get(&pdev->dev, "usb_general");
+	if (IS_ERR(priv->clk_usb_general))
+		return PTR_ERR(priv->clk_usb_general);
+
+	priv->clk_usb = devm_clk_get(&pdev->dev, "usb");
+	if (IS_ERR(priv->clk_usb))
+		return PTR_ERR(priv->clk_usb);
+
+	priv->dr_mode = of_usb_get_dr_mode_by_phy(pdev->dev.of_node, -1);
+	if (priv->dr_mode == USB_DR_MODE_UNKNOWN) {
+		dev_err(&pdev->dev,
+			"missing dual role configuration of the controller\n");
+		return -EINVAL;
+	}
+
+	phy = devm_phy_create(&pdev->dev, NULL, &phy_meson_usb2_ops);
+	if (IS_ERR(phy)) {
+		dev_err(&pdev->dev, "failed to create PHY\n");

drivers/phy/phy-meson-usb2.c line 257

Quoted 10 lines

drivers/phy/phy-meson-usb2.c new file +10 -0

+		return PTR_ERR(phy);
+	}
+
+	if (usb_reset_refcnt++ == 0) {
+		ret = device_reset(&pdev->dev);
+		if (ret) {
+			dev_err(&phy->dev, "Failed to reset USB PHY\n");
+			return ret;
+		}
+	}

The ref count + reset here looks like something that could/should be handled in a runtime PM callback.

IOW, there should be a pm_runtime_get_sync() here, and in the ->runtime_resume() callback, the device_reset() would be called. Runtime PM does the refcounting, and only calls ->runtime_resume() on the 0 -> 1 transition.

This isn't a big deal for now, so I'll let Kishon decide, but using runtime PM from the start will help enabling other PM features later.

drivers/phy/phy-meson-usb2.c line 257

Quoted 13 lines

drivers/phy/phy-meson-usb2.c new file +13 -0

+     phy = devm_phy_create(&pdev->dev, NULL, &phy_meson_usb2_ops);
+     if (IS_ERR(phy)) {
+             dev_err(&pdev->dev, "failed to create PHY\n");
+             return PTR_ERR(phy);
+     }
+
+     if (usb_reset_refcnt++ == 0) {
+             ret = device_reset(&pdev->dev);
+             if (ret) {
+                     dev_err(&phy->dev, "Failed to reset USB PHY\n");
+                     return ret;
+             }
+     }

The ref count + reset here looks like something that could/should be handled in a runtime PM callback.

Quoted 14 lines · Kevin Hilman

drivers/phy/phy-meson-usb2.c line 257

Quoted 13 lines

drivers/phy/phy-meson-usb2.c new file +13 -0

+     phy = devm_phy_create(&pdev->dev, NULL, &phy_meson_usb2_ops);
+     if (IS_ERR(phy)) {
+             dev_err(&pdev->dev, "failed to create PHY\n");
+             return PTR_ERR(phy);
+     }
+
+     if (usb_reset_refcnt++ == 0) {
+             ret = device_reset(&pdev->dev);
+             if (ret) {
+                     dev_err(&phy->dev, "Failed to reset USB PHY\n");
+                     return ret;
+             }
+     }

Quoted 2 lines

The ref count + reset here looks like something that could/should be handled in a runtime PM callback.

Unfortunately that doesn't work (as Jerome found out) because both PHYs are sharing the same reset line.
So if the second PHY would call device_reset then it would also reset the first PHY!

There's a comment above the declaration of usb_reset_refcnt which tries to explain this:
"The PHYs are sharing a common reset line -> we are only allowed to reset once for all PHYs."
Maybe I should move this comment to the "if (usb_reset_refcnt++ == 0) {" line to make it easier to see?

Quoted 22 lines · Martin Blumenstingl

Quoted 14 lines · Kevin Hilman

Review

Quoted 13 lines

(quoted diff) +13 -0

+     phy = devm_phy_create(&pdev->dev, NULL, &phy_meson_usb2_ops);
+     if (IS_ERR(phy)) {
+             dev_err(&pdev->dev, "failed to create PHY\n");
+             return PTR_ERR(phy);
+     }
+
+     if (usb_reset_refcnt++ == 0) {
+             ret = device_reset(&pdev->dev);
+             if (ret) {
+                     dev_err(&phy->dev, "Failed to reset USB PHY\n");
+                     return ret;
+             }
+     }

Quoted 2 lines

The ref count + reset here looks like something that could/should be handled in a runtime PM callback.

Unfortunately that doesn't work (as Jerome found out) because both PHYs are sharing the same reset line.
So if the second PHY would call device_reset then it would also reset the first PHY!

Quoted 7 lines

There's a comment above the declaration of usb_reset_refcnt which tries to explain this:
"The PHYs are sharing a common reset line -> we are only allowed to reset once for all PHYs."
Maybe I should move this comment to the "if (usb_reset_refcnt++ == 0) {" line to make it easier to see?

pm-runtime has refcounting in it. When one of the nodes turns on, the pm-runtime will call your driver to say there is a user when this first use turns up.

If all the sub-phys turn off and drop their refcount then the driver is called to say there are no more users and you can go to sleep.

Quoted 31 lines · Ben Dooks

Quoted 22 lines · Martin Blumenstingl

Quoted 14 lines · Kevin Hilman

Review

Quoted 13 lines

(quoted diff) +13 -0

+     phy = devm_phy_create(&pdev->dev, NULL, &phy_meson_usb2_ops);
+     if (IS_ERR(phy)) {
+             dev_err(&pdev->dev, "failed to create PHY\n");
+             return PTR_ERR(phy);
+     }
+
+     if (usb_reset_refcnt++ == 0) {
+             ret = device_reset(&pdev->dev);
+             if (ret) {
+                     dev_err(&phy->dev, "Failed to reset USB PHY\n");
+                     return ret;
+             }
+     }

Quoted 2 lines

The ref count + reset here looks like something that could/should be handled in a runtime PM callback.

Unfortunately that doesn't work (as Jerome found out) because both PHYs are sharing the same reset line.
So if the second PHY would call device_reset then it would also reset the first PHY!

Quoted 7 lines

There's a comment above the declaration of usb_reset_refcnt which tries to explain this:
"The PHYs are sharing a common reset line -> we are only allowed to reset once for all PHYs."
Maybe I should move this comment to the "if (usb_reset_refcnt++ == 0) {" line to make it easier to see?

Quoted 6 lines

pm-runtime has refcounting in it. When one of the nodes turns on, the pm-runtime will call your driver to say there is a user when this first use turns up.

If all the sub-phys turn off and drop their refcount then the driver is called to say there are no more users and you can go to sleep.

After a chat w/Martin on IRC, It turns out runtime PM wont help here.

The reason is because there are physically two PHY devices[1]. Those 2 devices will be treated independely by runtime PM, and have separate use-counting, which means doing what I proposed would cause a reset to happen when either device was probed.

So, I think it's OK as it is.

Quoted 54 lines · Kevin Hilman

Quoted 36 lines · Ben Dooks

Quoted 27 lines · Martin Blumenstingl

Quoted 16 lines · Kevin Hilman

Quoted 4 lines

(quoted diff) +3 -0

+     phy = devm_phy_create(&pdev->dev, NULL, &phy_meson_usb2_ops);
+     if (IS_ERR(phy)) {
+             dev_err(&pdev->dev, "failed to create PHY\n");

Review

Quoted 10 lines

(quoted diff) +10 -0

+             return PTR_ERR(phy);
+     }
+
+     if (usb_reset_refcnt++ == 0) {
+             ret = device_reset(&pdev->dev);
+             if (ret) {
+                     dev_err(&phy->dev, "Failed to reset USB PHY\n");
+                     return ret;
+             }
+     }

Quoted 2 lines

The ref count + reset here looks like something that could/should be handled in a runtime PM callback.

Unfortunately that doesn't work (as Jerome found out) because both PHYs are sharing the same reset line.
So if the second PHY would call device_reset then it would also reset the first PHY!

Quoted 7 lines

There's a comment above the declaration of usb_reset_refcnt which tries to explain this:
"The PHYs are sharing a common reset line -> we are only allowed to reset once for all PHYs."
Maybe I should move this comment to the "if (usb_reset_refcnt++ == 0) {" line to make it easier to see?

Quoted 6 lines

pm-runtime has refcounting in it. When one of the nodes turns on, the pm-runtime will call your driver to say there is a user when this first use turns up.

If all the sub-phys turn off and drop their refcount then the driver is called to say there are no more users and you can go to sleep.

After a chat w/Martin on IRC, It turns out runtime PM wont help here.

The reason is because there are physically two PHY devices[1]. Those 2 devices will be treated independely by runtime PM, and have separate use-counting, which means doing what I proposed would cause a reset to happen when either device was probed.

Quoted 1 line

So, I think it's OK as it is.

Surely you can do pm_runtime_get/put on the phy's parent platform device and do it that way?

Quoted 55 lines · Ben Dooks

Quoted 54 lines · Kevin Hilman

Quoted 36 lines · Ben Dooks

Quoted 27 lines · Martin Blumenstingl

Quoted 16 lines · Kevin Hilman

Quoted 4 lines

(quoted diff) +3 -0

+     phy = devm_phy_create(&pdev->dev, NULL, &phy_meson_usb2_ops);
+     if (IS_ERR(phy)) {
+             dev_err(&pdev->dev, "failed to create PHY\n");

Review

Quoted 10 lines

(quoted diff) +10 -0

+             return PTR_ERR(phy);
+     }
+
+     if (usb_reset_refcnt++ == 0) {
+             ret = device_reset(&pdev->dev);
+             if (ret) {
+                     dev_err(&phy->dev, "Failed to reset USB PHY\n");
+                     return ret;
+             }
+     }

Quoted 2 lines

The ref count + reset here looks like something that could/should be handled in a runtime PM callback.

Unfortunately that doesn't work (as Jerome found out) because both PHYs are sharing the same reset line.
So if the second PHY would call device_reset then it would also reset the first PHY!

Quoted 7 lines

There's a comment above the declaration of usb_reset_refcnt which tries to explain this:
"The PHYs are sharing a common reset line -> we are only allowed to reset once for all PHYs."
Maybe I should move this comment to the "if (usb_reset_refcnt++ == 0) {" line to make it easier to see?

Quoted 6 lines

pm-runtime has refcounting in it. When one of the nodes turns on, the pm-runtime will call your driver to say there is a user when this first use turns up.

If all the sub-phys turn off and drop their refcount then the driver is called to say there are no more users and you can go to sleep.

After a chat w/Martin on IRC, It turns out runtime PM wont help here.

The reason is because there are physically two PHY devices[1]. Those 2 devices will be treated independely by runtime PM, and have separate use-counting, which means doing what I proposed would cause a reset to happen when either device was probed.

Quoted 5 lines

Quoted 1 line

So, I think it's OK as it is.

Surely you can do pm_runtime_get/put on the phy's parent platform device and do it that way?

could you please be more specific with that (do you mean pdev->dev.parent)? so we would use pm_runtime_{get_sync,put} with the parent, while we would still define the runtime_resume in our driver.

However, the problem with all of the solutions proposed (runtime PM ones included) is that we're forcing a board-specific design issue (2 devices sharing a reset line) into a driver that should not have any board-specific assumptions in it.

For example, if this driver is used on another platform where different PHYs have different reset lines, then one of them (the unlucky one who is not probed first) will never get reset. So any form of per-device ref-counting is not a portable solution.

I'm not sure yet how the reset framework is supposed to handle shared reset lines, but that needs some investigation. I quick glance and it seems that reset controllers can have shared lines, so that should be investigated.

Quoted 9 lines · Kevin Hilman

However, the problem with all of the solutions proposed (runtime PM ones included) is that we're forcing a board-specific design issue (2 devices sharing a reset line) into a driver that should not have any board-specific assumptions in it.

For example, if this driver is used on another platform where different PHYs have different reset lines, then one of them (the unlucky one who is not probed first) will never get reset. So any form of per-device ref-counting is not a portable solution.

maybe we should also consider Ben's solution: he played with the USB PHY on his Meson8b board. His approach was to have only one USB PHY driver instance which exposes two PHYs.
The downside of this: the driver would have to know the offset of the PHYs (0x0 for the first PHY, 0x20 for the second), but we could handle the reset using runtime PM without any hacks.

I checked the USB PHY reference driver: it seems that there will be a new USB PHY with the GXL/GXM SoCs.
So maybe we could live with the assumption that the PHYs are at consecutive addresses.

Quoted 9 lines · Kevin Hilman

However, the problem with all of the solutions proposed (runtime PM ones included) is that we're forcing a board-specific design issue (2 devices sharing a reset line) into a driver that should not have any board-specific assumptions in it.

For example, if this driver is used on another platform where different PHYs have different reset lines, then one of them (the unlucky one who is not probed first) will never get reset. So any form of per-device ref-counting is not a portable solution.

maybe we should also consider Ben's solution: he played with the USB PHY on his Meson8b board. His approach was to have only one USB PHY driver instance which exposes two PHYs.
The downside of this: the driver would have to know the offset of the PHYs (0x0 for the first PHY, 0x20 for the second), but we could handle the reset using runtime PM without any hacks.

I checked the USB PHY reference driver: it seems that there will be a new USB PHY with the GXL/GXM SoCs.
So maybe we could live with the assumption that the PHYs are at consecutive addresses.

Quoted 4 lines

I'm not sure yet how the reset framework is supposed to handle shared reset lines, but that needs some investigation. I quick glance and it seems that reset controllers can have shared lines, so that should be investigated.

unfortunately shared resets are not allowed to use reset_control_reset, see [0]

[0] http://lxr.free-electrons.com/source/drivers/reset/core.c#L102

Quoted 34 lines · Martin Blumenstingl

Quoted 10 lines · Kevin Hilman

However, the problem with all of the solutions proposed (runtime PM ones included) is that we're forcing a board-specific design issue (2 devices sharing a reset line) into a driver that should not have any board-specific assumptions in it.

For example, if this driver is used on another platform where different PHYs have different reset lines, then one of them (the unlucky one who is not probed first) will never get reset. So any form of per-device ref-counting is not a portable solution.

maybe we should also consider Ben's solution: he played with the USB PHY on his Meson8b board. His approach was to have only one USB PHY driver instance which exposes two PHYs.
The downside of this: the driver would have to know the offset of the PHYs (0x0 for the first PHY, 0x20 for the second), but we could handle the reset using runtime PM without any hacks.

I checked the USB PHY reference driver: it seems that there will be a new USB PHY with the GXL/GXM SoCs.
So maybe we could live with the assumption that the PHYs are at consecutive addresses.

Quoted 4 lines

I'm not sure yet how the reset framework is supposed to handle shared reset lines, but that needs some investigation. I quick glance and it seems that reset controllers can have shared lines, so that should be investigated.

unfortunately shared resets are not allowed to use reset_control_reset, see [0]

Quoted 1 line

[0] http://lxr.free-electrons.com/source/drivers/reset/core.c#L102

If we didn't have the shared reset, we'd have one of node per phy and not have to have two sub-nodes... I don't think any other bits of the PHY framework are shared.

Quoted 9 lines · Kevin Hilman

However, the problem with all of the solutions proposed (runtime PM ones included) is that we're forcing a board-specific design issue (2 devices sharing a reset line) into a driver that should not have any board-specific assumptions in it.

For example, if this driver is used on another platform where different PHYs have different reset lines, then one of them (the unlucky one who is not probed first) will never get reset. So any form of per-device ref-counting is not a portable solution.

maybe we should also consider Ben's solution: he played with the USB PHY on his Meson8b board. His approach was to have only one USB PHY driver instance which exposes two PHYs.
The downside of this: the driver would have to know the offset of the PHYs (0x0 for the first PHY, 0x20 for the second), but we could handle the reset using runtime PM without any hacks.

I think the offset information can come from the devicetree too. The phy can be modeled something like below.

                usb-phys at c0000000 {
                        compatible = "amlogic,meson-gxbb-usb2-phy";
                        reg = <0x0 0xc0000000 0x0 0x40>;
                        #address-cells = <2>;
                        #size-cells = <2>;
                        ranges = <0x0 0x0 0x0 0xc0000000 0x0 0x40>;
                        resets = <&reset 34>;

                        usb0_phy: usb_phy at 0 {
                                #phy-cells = <0>;
                                reg = <0x0 0x0 0x0 0x20>;
                                clocks = <&clkc CLKID_USB &clkc CLKID_USB0>;
                                clock-names = "usb_general", "usb";
                                status = "disabled";
                        };

                        usb1_phy: usb_phy at 20 {
                                #phy-cells = <0>;
                                reg = <0x0 0x20 0x0 0x20>;
                                clocks = <&clkc CLKID_USB &clkc CLKID_USB1>;
                                clock-names = "usb_general", "usb";
                                status = "disabled";
                        };
                };

This way the driver will be probed only once (the reset can be done during probe). The phy driver should scan the dt node and for every sub-node it invokes phy_create?

Hi,

Quoted 10 lines · Martin Blumenstingl

Quoted 9 lines · Kevin Hilman

However, the problem with all of the solutions proposed (runtime PM ones included) is that we're forcing a board-specific design issue (2 devices sharing a reset line) into a driver that should not have any board-specific assumptions in it.

For example, if this driver is used on another platform where different PHYs have different reset lines, then one of them (the unlucky one who is not probed first) will never get reset. So any form of per-device ref-counting is not a portable solution.

Quoted 6 lines

maybe we should also consider Ben's solution: he played with the USB PHY on his Meson8b board. His approach was to have only one USB PHY driver instance which exposes two PHYs.
The downside of this: the driver would have to know the offset of the PHYs (0x0 for the first PHY, 0x20 for the second), but we could handle the reset using runtime PM without any hacks.

I think the offset information can come from the devicetree too. The phy can be modeled something like below.

                usb-phys at c0000000 {
                        compatible = "amlogic,meson-gxbb-usb2-phy";
                        reg = <0x0 0xc0000000 0x0 0x40>;
                        #address-cells = <2>;
                        #size-cells = <2>;
                        ranges = <0x0 0x0 0x0 0xc0000000 0x0 0x40>;
                        resets = <&reset 34>;

                        usb0_phy: usb_phy at 0 {
                                #phy-cells = <0>;
                                reg = <0x0 0x0 0x0 0x20>;
                                clocks = <&clkc CLKID_USB &clkc CLKID_USB0>;
                                clock-names = "usb_general", "usb";
                                status = "disabled";
                        };

                        usb1_phy: usb_phy at 20 {
                                #phy-cells = <0>;
                                reg = <0x0 0x20 0x0 0x20>;
                                clocks = <&clkc CLKID_USB &clkc CLKID_USB1>;
                                clock-names = "usb_general", "usb";
                                status = "disabled";
                        };
                };

This way the driver will be probed only once (the reset can be done during probe). The phy driver should scan the dt node and for every sub-node it invokes phy_create?

Hi Kishon,

Quoted 48 lines · Kishon Vijay Abraham I

Hi,

Quoted 10 lines · Martin Blumenstingl

Quoted 9 lines · Kevin Hilman

However, the problem with all of the solutions proposed (runtime PM ones included) is that we're forcing a board-specific design issue (2 devices sharing a reset line) into a driver that should not have any board-specific assumptions in it.

For example, if this driver is used on another platform where different PHYs have different reset lines, then one of them (the unlucky one who is not probed first) will never get reset. So any form of per-device ref-counting is not a portable solution.

Quoted 6 lines

maybe we should also consider Ben's solution: he played with the USB PHY on his Meson8b board. His approach was to have only one USB PHY driver instance which exposes two PHYs.
The downside of this: the driver would have to know the offset of the PHYs (0x0 for the first PHY, 0x20 for the second), but we could handle the reset using runtime PM without any hacks.

I think the offset information can come from the devicetree too. The phy can be modeled something like below.

                usb-phys at c0000000 {
                        compatible = "amlogic,meson-gxbb-usb2-phy";
                        reg = <0x0 0xc0000000 0x0 0x40>;
                        #address-cells = <2>;
                        #size-cells = <2>;
                        ranges = <0x0 0x0 0x0 0xc0000000 0x0 0x40>;
                        resets = <&reset 34>;

                        usb0_phy: usb_phy at 0 {
                                #phy-cells = <0>;
                                reg = <0x0 0x0 0x0 0x20>;
                                clocks = <&clkc CLKID_USB &clkc CLKID_USB0>;
                                clock-names = "usb_general", "usb";
                                status = "disabled";
                        };

                        usb1_phy: usb_phy at 20 {
                                #phy-cells = <0>;
                                reg = <0x0 0x20 0x0 0x20>;
                                clocks = <&clkc CLKID_USB &clkc CLKID_USB1>;
                                clock-names = "usb_general", "usb";
                                status = "disabled";
                        };
                };

Quoted 3 lines

This way the driver will be probed only once (the reset can be done during probe). The phy driver should scan the dt node and for every sub-node it invokes phy_create?

I'll recap what we have discussed so far (so you don't have to re-read the whole thread):
The reference driver treats both USB PHYs as separate devices (the datasheet has no information about the PHYs though). The only "special" thing is the shared reset line -> together with Philipp Zabel (the reset framework maintainer) we decided to make reset_control_reset work for shared reset lines.

That means we can keep the two PHYs as separate devices inside the .dts, while keeping everything else separate (just like the reference driver)
Is this fine for you and Arnd?

Regards,
Martin

Quoted 13 lines · Martin Blumenstingl

Quoted 3 lines · Kishon Vijay Abraham I

This way the driver will be probed only once (the reset can be done during probe). The phy driver should scan the dt node and for every sub-node it invokes phy_create?

I'll recap what we have discussed so far (so you don't have to re-read the whole thread):
The reference driver treats both USB PHYs as separate devices (the datasheet has no information about the PHYs though). The only "special" thing is the shared reset line -> together with Philipp Zabel (the reset framework maintainer) we decided to make reset_control_reset work for shared reset lines.

Quoted 4 lines

That means we can keep the two PHYs as separate devices inside the .dts, while keeping everything else separate (just like the reference driver)
Is this fine for you and Arnd?

yeah.. I'm fine with that.

Quoted 56 lines · Martin Blumenstingl

Quoted 55 lines · Ben Dooks

Quoted 54 lines · Kevin Hilman

Quoted 36 lines · Ben Dooks

Quoted 27 lines · Martin Blumenstingl

Quoted 16 lines · Kevin Hilman

Quoted 4 lines

(quoted diff) +3 -0

+     phy = devm_phy_create(&pdev->dev, NULL, &phy_meson_usb2_ops);
+     if (IS_ERR(phy)) {
+             dev_err(&pdev->dev, "failed to create PHY\n");

Review

Quoted 10 lines

(quoted diff) +10 -0

+             return PTR_ERR(phy);
+     }
+
+     if (usb_reset_refcnt++ == 0) {
+             ret = device_reset(&pdev->dev);
+             if (ret) {
+                     dev_err(&phy->dev, "Failed to reset USB PHY\n");
+                     return ret;
+             }
+     }

Quoted 2 lines

The ref count + reset here looks like something that could/should be handled in a runtime PM callback.

Unfortunately that doesn't work (as Jerome found out) because both PHYs are sharing the same reset line.
So if the second PHY would call device_reset then it would also reset the first PHY!

Quoted 7 lines

There's a comment above the declaration of usb_reset_refcnt which tries to explain this:
"The PHYs are sharing a common reset line -> we are only allowed to reset once for all PHYs."
Maybe I should move this comment to the "if (usb_reset_refcnt++ == 0) {" line to make it easier to see?

Quoted 6 lines

pm-runtime has refcounting in it. When one of the nodes turns on, the pm-runtime will call your driver to say there is a user when this first use turns up.

If all the sub-phys turn off and drop their refcount then the driver is called to say there are no more users and you can go to sleep.

After a chat w/Martin on IRC, It turns out runtime PM wont help here.

The reason is because there are physically two PHY devices[1]. Those 2 devices will be treated independely by runtime PM, and have separate use-counting, which means doing what I proposed would cause a reset to happen when either device was probed.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

So, I think it's OK as it is.

Surely you can do pm_runtime_get/put on the phy's parent platform device and do it that way?

could you please be more specific with that (do you mean pdev->dev.parent)? so we would use pm_runtime_{get_sync,put} with the parent, while we would still define the runtime_resume in our driver.

You'd also need to do get/put on the children, but yes, that's what Ben is suggesting.

However, the problem with all of the solutions proposed (runtime PM ones included) is that we're forcing a board-specific design issue (2 devices sharing a reset line) into a driver that should not have any board-specific assumptions in it.

For example, if this driver is used on another platform where different PHYs have different reset lines, then one of them (the unlucky one who is not probed first) will never get reset. So any form of per-device ref-counting is not a portable solution.

I'm not sure yet how the reset framework is supposed to handle shared reset lines, but that needs some investigation. I quick glance and it seems that reset controllers can have shared lines, so that should be investigated.

Quoted 75 lines · Kevin Hilman

Quoted 56 lines · Martin Blumenstingl

Quoted 55 lines · Ben Dooks

Quoted 54 lines · Kevin Hilman

Quoted 36 lines · Ben Dooks

Quoted 27 lines · Martin Blumenstingl

Quoted 16 lines · Kevin Hilman

Quoted 4 lines

(quoted diff) +3 -0

+     phy = devm_phy_create(&pdev->dev, NULL, &phy_meson_usb2_ops);
+     if (IS_ERR(phy)) {
+             dev_err(&pdev->dev, "failed to create PHY\n");

Review

Quoted 10 lines

(quoted diff) +10 -0

+             return PTR_ERR(phy);
+     }
+
+     if (usb_reset_refcnt++ == 0) {
+             ret = device_reset(&pdev->dev);
+             if (ret) {
+                     dev_err(&phy->dev, "Failed to reset USB PHY\n");
+                     return ret;
+             }
+     }

Quoted 2 lines

The ref count + reset here looks like something that could/should be handled in a runtime PM callback.

Unfortunately that doesn't work (as Jerome found out) because both PHYs are sharing the same reset line.
So if the second PHY would call device_reset then it would also reset the first PHY!

Quoted 7 lines

There's a comment above the declaration of usb_reset_refcnt which tries to explain this:
"The PHYs are sharing a common reset line -> we are only allowed to reset once for all PHYs."
Maybe I should move this comment to the "if (usb_reset_refcnt++ == 0) {" line to make it easier to see?

Quoted 6 lines

pm-runtime has refcounting in it. When one of the nodes turns on, the pm-runtime will call your driver to say there is a user when this first use turns up.

If all the sub-phys turn off and drop their refcount then the driver is called to say there are no more users and you can go to sleep.

After a chat w/Martin on IRC, It turns out runtime PM wont help here.

The reason is because there are physically two PHY devices[1]. Those 2 devices will be treated independely by runtime PM, and have separate use-counting, which means doing what I proposed would cause a reset to happen when either device was probed.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

So, I think it's OK as it is.

Surely you can do pm_runtime_get/put on the phy's parent platform device and do it that way?

could you please be more specific with that (do you mean pdev->dev.parent)? so we would use pm_runtime_{get_sync,put} with the parent, while we would still define the runtime_resume in our driver.

You'd also need to do get/put on the children, but yes, that's what Ben is suggesting.

However, the problem with all of the solutions proposed (runtime PM ones included) is that we're forcing a board-specific design issue (2 devices sharing a reset line) into a driver that should not have any board-specific assumptions in it.

Quoted 4 lines

For example, if this driver is used on another platform where different PHYs have different reset lines, then one of them (the unlucky one who is not probed first) will never get reset. So any form of per-device ref-counting is not a portable solution.

indeed, so in simple words we would need something like reset_control_do_once(rstc, RESET/ASSERT/DEASSERT) which would remember internally if any action has already been executed: if not it does a _reset, _assert or _deassert and otherwise it does nothing.

Quoted 80 lines · Martin Blumenstingl

Quoted 79 lines · Kevin Hilman

Quoted 56 lines · Martin Blumenstingl

Quoted 55 lines · Ben Dooks

Quoted 54 lines · Kevin Hilman

Quoted 36 lines · Ben Dooks

Quoted 27 lines · Martin Blumenstingl

Quoted 16 lines · Kevin Hilman

Quoted 4 lines

(quoted diff) +3 -0

+     phy = devm_phy_create(&pdev->dev, NULL, &phy_meson_usb2_ops);
+     if (IS_ERR(phy)) {
+             dev_err(&pdev->dev, "failed to create PHY\n");

Review

Quoted 10 lines

(quoted diff) +10 -0

+             return PTR_ERR(phy);
+     }
+
+     if (usb_reset_refcnt++ == 0) {
+             ret = device_reset(&pdev->dev);
+             if (ret) {
+                     dev_err(&phy->dev, "Failed to reset USB PHY\n");
+                     return ret;
+             }
+     }

Quoted 2 lines

The ref count + reset here looks like something that could/should be handled in a runtime PM callback.

Unfortunately that doesn't work (as Jerome found out) because both PHYs are sharing the same reset line.
So if the second PHY would call device_reset then it would also reset the first PHY!

Quoted 7 lines

There's a comment above the declaration of usb_reset_refcnt which tries to explain this:
"The PHYs are sharing a common reset line -> we are only allowed to reset once for all PHYs."
Maybe I should move this comment to the "if (usb_reset_refcnt++ == 0) {" line to make it easier to see?

Quoted 6 lines

pm-runtime has refcounting in it. When one of the nodes turns on, the pm-runtime will call your driver to say there is a user when this first use turns up.

If all the sub-phys turn off and drop their refcount then the driver is called to say there are no more users and you can go to sleep.

After a chat w/Martin on IRC, It turns out runtime PM wont help here.

The reason is because there are physically two PHY devices[1]. Those 2 devices will be treated independely by runtime PM, and have separate use-counting, which means doing what I proposed would cause a reset to happen when either device was probed.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

So, I think it's OK as it is.

Surely you can do pm_runtime_get/put on the phy's parent platform device and do it that way?

could you please be more specific with that (do you mean pdev->dev.parent)? so we would use pm_runtime_{get_sync,put} with the parent, while we would still define the runtime_resume in our driver.

You'd also need to do get/put on the children, but yes, that's what Ben is suggesting.

However, the problem with all of the solutions proposed (runtime PM ones included) is that we're forcing a board-specific design issue (2 devices sharing a reset line) into a driver that should not have any board-specific assumptions in it.

For example, if this driver is used on another platform where different PHYs have different reset lines, then one of them (the unlucky one who is not probed first) will never get reset. So any form of per-device ref-counting is not a portable solution.

Quoted 4 lines

indeed, so in simple words we would need something like reset_control_do_once(rstc, RESET/ASSERT/DEASSERT) which would remember internally if any action has already been executed: if not it does a _reset, _assert or _deassert and otherwise it does nothing.

for now I've implemented something less hacky: I made the reset optional and only specified it for phy0.

During Jerome's tests the reset was not needed, while on my board it's required to bring both PHYs up.
Additionally the USB PHY reference driver does not have any reset logic for newer SoCs (GXL), so making the reset optional doesn't sound that bad to me.

Quoted 75 lines

Quoted 56 lines · Martin Blumenstingl

Quoted 55 lines · Ben Dooks

Quoted 54 lines · Kevin Hilman

Quoted 36 lines · Ben Dooks

Quoted 27 lines · Martin Blumenstingl

Quoted 16 lines · Kevin Hilman

Quoted 4 lines

(quoted diff) +3 -0

+     phy = devm_phy_create(&pdev->dev, NULL, &phy_meson_usb2_ops);
+     if (IS_ERR(phy)) {
+             dev_err(&pdev->dev, "failed to create PHY\n");

Review

Quoted 10 lines

(quoted diff) +10 -0

+             return PTR_ERR(phy);
+     }
+
+     if (usb_reset_refcnt++ == 0) {
+             ret = device_reset(&pdev->dev);
+             if (ret) {
+                     dev_err(&phy->dev, "Failed to reset USB PHY\n");
+                     return ret;
+             }
+     }

Quoted 2 lines

The ref count + reset here looks like something that could/should be handled in a runtime PM callback.

Unfortunately that doesn't work (as Jerome found out) because both PHYs are sharing the same reset line.
So if the second PHY would call device_reset then it would also reset the first PHY!

Quoted 7 lines

There's a comment above the declaration of usb_reset_refcnt which tries to explain this:
"The PHYs are sharing a common reset line -> we are only allowed to reset once for all PHYs."
Maybe I should move this comment to the "if (usb_reset_refcnt++ == 0) {" line to make it easier to see?

Quoted 6 lines

pm-runtime has refcounting in it. When one of the nodes turns on, the pm-runtime will call your driver to say there is a user when this first use turns up.

If all the sub-phys turn off and drop their refcount then the driver is called to say there are no more users and you can go to sleep.

After a chat w/Martin on IRC, It turns out runtime PM wont help here.

The reason is because there are physically two PHY devices[1]. Those 2 devices will be treated independely by runtime PM, and have separate use-counting, which means doing what I proposed would cause a reset to happen when either device was probed.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

So, I think it's OK as it is.

Surely you can do pm_runtime_get/put on the phy's parent platform device and do it that way?

could you please be more specific with that (do you mean pdev->dev.parent)? so we would use pm_runtime_{get_sync,put} with the parent, while we would still define the runtime_resume in our driver.

You'd also need to do get/put on the children, but yes, that's what Ben is suggesting.

However, the problem with all of the solutions proposed (runtime PM ones included) is that we're forcing a board-specific design issue (2 devices sharing a reset line) into a driver that should not have any board-specific assumptions in it.

Quoted 4 lines

For example, if this driver is used on another platform where different PHYs have different reset lines, then one of them (the unlucky one who is not probed first) will never get reset. So any form of per-device ref-counting is not a portable solution.

indeed, so in simple words we would need something like reset_control_do_once(rstc, RESET/ASSERT/DEASSERT) which would remember internally if any action has already been executed: if not it does a _reset, _assert or _deassert and otherwise it does nothing.

Quoted 4 lines

I'm not sure yet how the reset framework is supposed to handle shared reset lines, but that needs some investigation. I quick glance and it seems that reset controllers can have shared lines, so that should be investigated.

I added Philipp and Hans to this thread - maybe they can comment on this. To sum it up, our problem is:
- there are two separate USB PHYs on Meson GXBB
- both are sharing the same reset line (provided by the reset-meson driver)
- during initialization of the PHYs we must only call reset_control_reset(rstc) once (if we do it for the first *and* second PHY then the first PHY gets confused once the second PHY uses the reset because the first PHY's state is reset as well)

Hi Martin,

Am Freitag, den 09.09.2016, 22:36 +0200 schrieb Martin Blumenstingl:

Quoted 89 lines · Kevin Hilman

Quoted 75 lines

Quoted 56 lines · Martin Blumenstingl

Quoted 55 lines · Ben Dooks

Quoted 54 lines · Kevin Hilman

Quoted 36 lines · Ben Dooks

Quoted 27 lines · Martin Blumenstingl

Quoted 16 lines · Kevin Hilman

Quoted 4 lines

(quoted diff) +3 -0

+     phy = devm_phy_create(&pdev->dev, NULL, &phy_meson_usb2_ops);
+     if (IS_ERR(phy)) {
+             dev_err(&pdev->dev, "failed to create PHY\n");

Review

Quoted 10 lines

(quoted diff) +10 -0

+             return PTR_ERR(phy);
+     }
+
+     if (usb_reset_refcnt++ == 0) {
+             ret = device_reset(&pdev->dev);
+             if (ret) {
+                     dev_err(&phy->dev, "Failed to reset USB PHY\n");
+                     return ret;
+             }
+     }

Quoted 2 lines

The ref count + reset here looks like something that could/should be handled in a runtime PM callback.

Unfortunately that doesn't work (as Jerome found out) because both PHYs are sharing the same reset line.
So if the second PHY would call device_reset then it would also reset the first PHY!

Quoted 7 lines

There's a comment above the declaration of usb_reset_refcnt which tries to explain this:
"The PHYs are sharing a common reset line -> we are only allowed to reset once for all PHYs."
Maybe I should move this comment to the "if (usb_reset_refcnt++ == 0) {" line to make it easier to see?

Quoted 6 lines

pm-runtime has refcounting in it. When one of the nodes turns on, the pm-runtime will call your driver to say there is a user when this first use turns up.

If all the sub-phys turn off and drop their refcount then the driver is called to say there are no more users and you can go to sleep.

After a chat w/Martin on IRC, It turns out runtime PM wont help here.

The reason is because there are physically two PHY devices[1]. Those 2 devices will be treated independely by runtime PM, and have separate use-counting, which means doing what I proposed would cause a reset to happen when either device was probed.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

So, I think it's OK as it is.

Surely you can do pm_runtime_get/put on the phy's parent platform device and do it that way?

could you please be more specific with that (do you mean pdev->dev.parent)? so we would use pm_runtime_{get_sync,put} with the parent, while we would still define the runtime_resume in our driver.

You'd also need to do get/put on the children, but yes, that's what Ben is suggesting.

However, the problem with all of the solutions proposed (runtime PM ones included) is that we're forcing a board-specific design issue (2 devices sharing a reset line) into a driver that should not have any board-specific assumptions in it.

Quoted 4 lines

For example, if this driver is used on another platform where different PHYs have different reset lines, then one of them (the unlucky one who is not probed first) will never get reset. So any form of per-device ref-counting is not a portable solution.

indeed, so in simple words we would need something like reset_control_do_once(rstc, RESET/ASSERT/DEASSERT) which would remember internally if any action has already been executed: if not it does a _reset, _assert or _deassert and otherwise it does nothing.

Quoted 4 lines

I'm not sure yet how the reset framework is supposed to handle shared reset lines, but that needs some investigation. I quick glance and it seems that reset controllers can have shared lines, so that should be investigated.

Quoted 8 lines

I added Philipp and Hans to this thread - maybe they can comment on this. To sum it up, our problem is:
- there are two separate USB PHYs on Meson GXBB
- both are sharing the same reset line (provided by the reset-meson driver)
- during initialization of the PHYs we must only call reset_control_reset(rstc) once (if we do it for the first *and* second PHY then the first PHY gets confused once the second PHY uses the reset because the first PHY's state is reset as well)

If you have an initially asserted reset line and you can enable the first module by deasserting the reset via reset_control_deassert (and reset_control_assert to signal when the module may be disabled again after use), shared resets are for you.

If you need a reset pulse or have no direct control over the reset line, (device_reset), the reset framework currently has no solution for this. The ugly thing about reset_control_once would be that it can't re-reset modules when unloading and reloading driver modules.

A real solution for shared reset lines with reset pulses would have to be some kind of reset request framework where if one module requests a reset, the other module sharing the reset could be notified, and then either veto the reset or, if possible, cease operations, store its state, and prepare to be reset, too, and afterwards restore state. I'd prefer not to think about this too much unless absolutely necessary.

On Tue, Sep 13, 2016 at 5:28 PM, Philipp Zabel <[email protected]> wrote:

Quoted 14 lines

Quoted 8 lines

I added Philipp and Hans to this thread - maybe they can comment on this. To sum it up, our problem is:
- there are two separate USB PHYs on Meson GXBB
- both are sharing the same reset line (provided by the reset-meson driver)
- during initialization of the PHYs we must only call reset_control_reset(rstc) once (if we do it for the first *and* second PHY then the first PHY gets confused once the second PHY uses the reset because the first PHY's state is reset as well)

If you have an initially asserted reset line and you can enable the first module by deasserting the reset via reset_control_deassert (and reset_control_assert to signal when the module may be disabled again after use), shared resets are for you.

Quoted 4 lines

If you need a reset pulse or have no direct control over the reset line, (device_reset), the reset framework currently has no solution for this. The ugly thing about reset_control_once would be that it can't re-reset modules when unloading and reloading driver modules.

The corresponding reset driver in question is reset-meson, which only implements reset (assert/deassert are not implemented). However, I don't know if this is due to hardware design.
I think the hardware implements the latter, but maybe Neil can give more information here (I currently don't have access to my board so I cannot test how the hardware actually behaves).

Quoted 1 line · Martin Blumenstingl

On Tue, Sep 13, 2016 at 5:28 PM, Philipp Zabel <[email protected]> wrote:

[...]

Quoted 19 lines

Quoted 14 lines

Quoted 8 lines

I added Philipp and Hans to this thread - maybe they can comment on this. To sum it up, our problem is:
- there are two separate USB PHYs on Meson GXBB
- both are sharing the same reset line (provided by the reset-meson driver)
- during initialization of the PHYs we must only call reset_control_reset(rstc) once (if we do it for the first *and* second PHY then the first PHY gets confused once the second PHY uses the reset because the first PHY's state is reset as well)

If you have an initially asserted reset line and you can enable the first module by deasserting the reset via reset_control_deassert (and reset_control_assert to signal when the module may be disabled again after use), shared resets are for you.

Quoted 4 lines

If you need a reset pulse or have no direct control over the reset line, (device_reset), the reset framework currently has no solution for this. The ugly thing about reset_control_once would be that it can't re-reset modules when unloading and reloading driver modules.

Quoted 6 lines

The corresponding reset driver in question is reset-meson, which only implements reset (assert/deassert are not implemented). However, I don't know if this is due to hardware design.
I think the hardware implements the latter, but maybe Neil can give more information here (I currently don't have access to my board so I cannot test how the hardware actually behaves).

It's implemented that way because the hardware only supports a reset pulse.

Quoted 1 line · Martin Blumenstingl

On Tue, Sep 13, 2016 at 5:28 PM, Philipp Zabel <[email protected]> wrote:

[...]

Quoted 19 lines

Quoted 14 lines

Quoted 8 lines

I added Philipp and Hans to this thread - maybe they can comment on this. To sum it up, our problem is:
- there are two separate USB PHYs on Meson GXBB
- both are sharing the same reset line (provided by the reset-meson driver)
- during initialization of the PHYs we must only call reset_control_reset(rstc) once (if we do it for the first *and* second PHY then the first PHY gets confused once the second PHY uses the reset because the first PHY's state is reset as well)

If you have an initially asserted reset line and you can enable the first module by deasserting the reset via reset_control_deassert (and reset_control_assert to signal when the module may be disabled again after use), shared resets are for you.

Quoted 4 lines

If you need a reset pulse or have no direct control over the reset line, (device_reset), the reset framework currently has no solution for this. The ugly thing about reset_control_once would be that it can't re-reset modules when unloading and reloading driver modules.

Quoted 6 lines

The corresponding reset driver in question is reset-meson, which only implements reset (assert/deassert are not implemented). However, I don't know if this is due to hardware design.
I think the hardware implements the latter, but maybe Neil can give more information here (I currently don't have access to my board so I cannot test how the hardware actually behaves).

It's implemented that way because the hardware only supports a reset pulse.

Am Dienstag, den 13.09.2016, 17:59 -0700 schrieb Kevin Hilman:

Quoted 32 lines

Quoted 1 line · Martin Blumenstingl

On Tue, Sep 13, 2016 at 5:28 PM, Philipp Zabel <[email protected]> wrote:

[...]

Quoted 25 lines

Quoted 14 lines

Quoted 8 lines

I added Philipp and Hans to this thread - maybe they can comment on this. To sum it up, our problem is:
- there are two separate USB PHYs on Meson GXBB
- both are sharing the same reset line (provided by the reset-meson driver)
- during initialization of the PHYs we must only call reset_control_reset(rstc) once (if we do it for the first *and* second PHY then the first PHY gets confused once the second PHY uses the reset because the first PHY's state is reset as well)

If you have an initially asserted reset line and you can enable the first module by deasserting the reset via reset_control_deassert (and reset_control_assert to signal when the module may be disabled again after use), shared resets are for you.

Quoted 4 lines

If you need a reset pulse or have no direct control over the reset line, (device_reset), the reset framework currently has no solution for this. The ugly thing about reset_control_once would be that it can't re-reset modules when unloading and reloading driver modules.

The corresponding reset driver in question is reset-meson, which only implements reset (assert/deassert are not implemented). However, I don't know if this is due to hardware design.
I think the hardware implements the latter, but maybe Neil can give more information here (I currently don't have access to my board so I cannot test how the hardware actually behaves).

Quoted 2 lines

It's implemented that way because the hardware only supports a reset pulse.

Would it be possible to bring down both PHYs drivers, pull the reset line once, and then bring the drivers back up again?

Hi Philipp,

Quoted 106 lines · Philipp Zabel

Hi Martin,

Am Freitag, den 09.09.2016, 22:36 +0200 schrieb Martin Blumenstingl:

Quoted 89 lines · Kevin Hilman

Quoted 75 lines

Quoted 56 lines · Martin Blumenstingl

Quoted 55 lines · Ben Dooks

Quoted 54 lines · Kevin Hilman

Quoted 36 lines · Ben Dooks

Quoted 27 lines · Martin Blumenstingl

Quoted 16 lines · Kevin Hilman

Quoted 4 lines

(quoted diff) +3 -0

+     phy = devm_phy_create(&pdev->dev, NULL, &phy_meson_usb2_ops);
+     if (IS_ERR(phy)) {
+             dev_err(&pdev->dev, "failed to create PHY\n");

Review

Quoted 10 lines

(quoted diff) +10 -0

+             return PTR_ERR(phy);
+     }
+
+     if (usb_reset_refcnt++ == 0) {
+             ret = device_reset(&pdev->dev);
+             if (ret) {
+                     dev_err(&phy->dev, "Failed to reset USB PHY\n");
+                     return ret;
+             }
+     }

Quoted 2 lines

The ref count + reset here looks like something that could/should be handled in a runtime PM callback.

Unfortunately that doesn't work (as Jerome found out) because both PHYs are sharing the same reset line.
So if the second PHY would call device_reset then it would also reset the first PHY!

Quoted 7 lines

There's a comment above the declaration of usb_reset_refcnt which tries to explain this:
"The PHYs are sharing a common reset line -> we are only allowed to reset once for all PHYs."
Maybe I should move this comment to the "if (usb_reset_refcnt++ == 0) {" line to make it easier to see?

Quoted 6 lines

pm-runtime has refcounting in it. When one of the nodes turns on, the pm-runtime will call your driver to say there is a user when this first use turns up.

If all the sub-phys turn off and drop their refcount then the driver is called to say there are no more users and you can go to sleep.

After a chat w/Martin on IRC, It turns out runtime PM wont help here.

The reason is because there are physically two PHY devices[1]. Those 2 devices will be treated independely by runtime PM, and have separate use-counting, which means doing what I proposed would cause a reset to happen when either device was probed.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

So, I think it's OK as it is.

Surely you can do pm_runtime_get/put on the phy's parent platform device and do it that way?

could you please be more specific with that (do you mean pdev->dev.parent)? so we would use pm_runtime_{get_sync,put} with the parent, while we would still define the runtime_resume in our driver.

You'd also need to do get/put on the children, but yes, that's what Ben is suggesting.

However, the problem with all of the solutions proposed (runtime PM ones included) is that we're forcing a board-specific design issue (2 devices sharing a reset line) into a driver that should not have any board-specific assumptions in it.

Quoted 4 lines

For example, if this driver is used on another platform where different PHYs have different reset lines, then one of them (the unlucky one who is not probed first) will never get reset. So any form of per-device ref-counting is not a portable solution.

indeed, so in simple words we would need something like reset_control_do_once(rstc, RESET/ASSERT/DEASSERT) which would remember internally if any action has already been executed: if not it does a _reset, _assert or _deassert and otherwise it does nothing.

Quoted 4 lines

I'm not sure yet how the reset framework is supposed to handle shared reset lines, but that needs some investigation. I quick glance and it seems that reset controllers can have shared lines, so that should be investigated.

Quoted 8 lines

I added Philipp and Hans to this thread - maybe they can comment on this. To sum it up, our problem is:
- there are two separate USB PHYs on Meson GXBB
- both are sharing the same reset line (provided by the reset-meson driver)
- during initialization of the PHYs we must only call reset_control_reset(rstc) once (if we do it for the first *and* second PHY then the first PHY gets confused once the second PHY uses the reset because the first PHY's state is reset as well)

If you have an initially asserted reset line and you can enable the first module by deasserting the reset via reset_control_deassert (and reset_control_assert to signal when the module may be disabled again after use), shared resets are for you.

Quoted 4 lines

If you need a reset pulse or have no direct control over the reset line, (device_reset), the reset framework currently has no solution for this. The ugly thing about reset_control_once would be that it can't re-reset modules when unloading and reloading driver modules.

The corresponding reset driver in question is reset-meson, which only implements reset (assert/deassert are not implemented). However, I don't know if this is due to hardware design.
I think the hardware implements the latter, but maybe Neil can give more information here (I currently don't have access to my board so I cannot test how the hardware actually behaves).

Quoted 6 lines

A real solution for shared reset lines with reset pulses would have to be some kind of reset request framework where if one module requests a reset, the other module sharing the reset could be notified, and then either veto the reset or, if possible, cease operations, store its state, and prepare to be reset, too, and afterwards restore state. I'd prefer not to think about this too much unless absolutely necessary.

I'm not sure if this would work in our case: one PHY instance would have to know if the other has already triggered the reset or not.

Am Dienstag, den 13.09.2016, 20:38 +0200 schrieb Martin Blumenstingl:

Quoted 126 lines

Hi Philipp,

Quoted 106 lines · Philipp Zabel

Hi Martin,

Am Freitag, den 09.09.2016, 22:36 +0200 schrieb Martin Blumenstingl:

Quoted 89 lines · Kevin Hilman

Quoted 75 lines

Quoted 56 lines · Martin Blumenstingl

Quoted 55 lines · Ben Dooks

Quoted 54 lines · Kevin Hilman

Quoted 36 lines · Ben Dooks

Quoted 27 lines · Martin Blumenstingl

Quoted 16 lines · Kevin Hilman

Quoted 4 lines

(quoted diff) +3 -0

+     phy = devm_phy_create(&pdev->dev, NULL, &phy_meson_usb2_ops);
+     if (IS_ERR(phy)) {
+             dev_err(&pdev->dev, "failed to create PHY\n");

Review

Quoted 10 lines

(quoted diff) +10 -0

+             return PTR_ERR(phy);
+     }
+
+     if (usb_reset_refcnt++ == 0) {
+             ret = device_reset(&pdev->dev);
+             if (ret) {
+                     dev_err(&phy->dev, "Failed to reset USB PHY\n");
+                     return ret;
+             }
+     }

Quoted 2 lines

The ref count + reset here looks like something that could/should be handled in a runtime PM callback.

Unfortunately that doesn't work (as Jerome found out) because both PHYs are sharing the same reset line.
So if the second PHY would call device_reset then it would also reset the first PHY!

Quoted 7 lines

There's a comment above the declaration of usb_reset_refcnt which tries to explain this:
"The PHYs are sharing a common reset line -> we are only allowed to reset once for all PHYs."
Maybe I should move this comment to the "if (usb_reset_refcnt++ == 0) {" line to make it easier to see?

Quoted 6 lines

pm-runtime has refcounting in it. When one of the nodes turns on, the pm-runtime will call your driver to say there is a user when this first use turns up.

If all the sub-phys turn off and drop their refcount then the driver is called to say there are no more users and you can go to sleep.

After a chat w/Martin on IRC, It turns out runtime PM wont help here.

The reason is because there are physically two PHY devices[1]. Those 2 devices will be treated independely by runtime PM, and have separate use-counting, which means doing what I proposed would cause a reset to happen when either device was probed.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

So, I think it's OK as it is.

Surely you can do pm_runtime_get/put on the phy's parent platform device and do it that way?

could you please be more specific with that (do you mean pdev->dev.parent)? so we would use pm_runtime_{get_sync,put} with the parent, while we would still define the runtime_resume in our driver.

You'd also need to do get/put on the children, but yes, that's what Ben is suggesting.

However, the problem with all of the solutions proposed (runtime PM ones included) is that we're forcing a board-specific design issue (2 devices sharing a reset line) into a driver that should not have any board-specific assumptions in it.

Quoted 4 lines

For example, if this driver is used on another platform where different PHYs have different reset lines, then one of them (the unlucky one who is not probed first) will never get reset. So any form of per-device ref-counting is not a portable solution.

indeed, so in simple words we would need something like reset_control_do_once(rstc, RESET/ASSERT/DEASSERT) which would remember internally if any action has already been executed: if not it does a _reset, _assert or _deassert and otherwise it does nothing.

Quoted 4 lines

I'm not sure yet how the reset framework is supposed to handle shared reset lines, but that needs some investigation. I quick glance and it seems that reset controllers can have shared lines, so that should be investigated.

Quoted 8 lines

I added Philipp and Hans to this thread - maybe they can comment on this. To sum it up, our problem is:
- there are two separate USB PHYs on Meson GXBB
- both are sharing the same reset line (provided by the reset-meson driver)
- during initialization of the PHYs we must only call reset_control_reset(rstc) once (if we do it for the first *and* second PHY then the first PHY gets confused once the second PHY uses the reset because the first PHY's state is reset as well)

If you have an initially asserted reset line and you can enable the first module by deasserting the reset via reset_control_deassert (and reset_control_assert to signal when the module may be disabled again after use), shared resets are for you.

Quoted 4 lines

If you need a reset pulse or have no direct control over the reset line, (device_reset), the reset framework currently has no solution for this. The ugly thing about reset_control_once would be that it can't re-reset modules when unloading and reloading driver modules.

The corresponding reset driver in question is reset-meson, which only implements reset (assert/deassert are not implemented). However, I don't know if this is due to hardware design.
I think the hardware implements the latter, but maybe Neil can give more information here (I currently don't have access to my board so I cannot test how the hardware actually behaves).

Quoted 6 lines

A real solution for shared reset lines with reset pulses would have to be some kind of reset request framework where if one module requests a reset, the other module sharing the reset could be notified, and then either veto the reset or, if possible, cease operations, store its state, and prepare to be reset, too, and afterwards restore state. I'd prefer not to think about this too much unless absolutely necessary.

Quoted 2 lines

I'm not sure if this would work in our case: one PHY instance would have to know if the other has already triggered the reset or not.

We could add a triggered flag or a counter to struct reset_control, and have reset_control_reset_once do nothing if it is already set / incremented. Since the reset_control goes away with the last consumer, the shared reset line would get triggered again after unbinding both PHY devices.

Quoted 5 lines

We could add a triggered flag or a counter to struct reset_control, and have reset_control_reset_once do nothing if it is already set / incremented. Since the reset_control goes away with the last consumer, the shared reset line would get triggered again after unbinding both PHY devices.

I guess that'd do the trick for us:
- we could use devm_reset_control_get_optional_shared() during probe
- power_on would then call reset_control_reset()
- the code in reset_control_reset would be changed: the if (WARN_ON(rstc->shared)) would be removed. then we return 0 if (rstc->shared && atomic_read(&rstc->shared_triggered)). otherwise we proceed with the old logic, except that we use atomic_set(&rstc->shared_triggerred, 1) in case of success (if an error was returned we leave it as "not triggered").

Let me know if you want me to (at least try to) implement that and send an RFC.

Add the nodes for the dwc2 USB controller and the related USB PHYs. Currently we force usb0 to host mode because OTG is currently not working in our PHY driver.

Signed-off-by Jerome Brunet <[email protected]>

Signed-off-by Martin Blumenstingl <[email protected]>

---
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 52 +++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi +0 -0

index 2e8a3d9..02dfc54 100644

arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi +4 -0

+			compatible = "amlogic,meson-gxbb-usb", "snps,dwc2";
+			reg = <0x0 0xc9100000 0x0 0x40000>;
+			interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&clkc CLKID_USB1_DDR_BRIDGE>;

Hi Martin,

Am 04.09.2016 um 23:31 schrieb Martin Blumenstingl:

Quoted 15 lines

Add the nodes for the dwc2 USB controller and the related USB PHYs. Currently we force usb0 to host mode because OTG is currently not working in our PHY driver.

Signed-off-by Jerome Brunet <[email protected]>

Signed-off-by Martin Blumenstingl <[email protected]>

---
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 52 +++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi +0 -0

index 2e8a3d9..02dfc54 100644

arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi line 159

Quoted 9 lines

arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi +6 -0

@@ -151,6 +151,34 @@
 		#size-cells = <2>;
 		ranges;
 
+		usb-phys at c0000000 {
+			compatible = "simple-bus";
+			reg = <0x0 0xc0000000 0x0 0x40>;
+			#address-cells = <2>;
+			#size-cells = <2>;
+			ranges = <0x0 0x0 0x0 0xc0000000 0x0 0x40>;

Does this correspond to any physical bus or could we drop this layer?

arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi line 161

Quoted 2 lines

arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi +2 -0

+
+			usb0_phy: usb_phy at 0 {

phy at 0 to avoid the underscore in node name? dash otherwise. If dropping the bus, *phy at c0000000 obviously.

arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi line 166

Quoted 5 lines

arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi +5 -0

+				compatible = "amlogic,meson-gxbb-usb2-phy";
+				#phy-cells = <0>;
+				reg = <0x0 0x0 0x0 0x20>;
+				resets = <&reset 34>;
+				clocks = <&clkc CLKID_USB &clkc CLKID_USB0>;

<&clkc CLKID_USB>, <&clkc CLKID_USB0> please.

arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi line 176

Quoted 10 lines

arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi +10 -0

+				clock-names = "usb_general", "usb";
+				status = "disabled";
+			};
+
+			usb1_phy: usb_phy at 20 {
+				compatible = "amlogic,meson-gxbb-usb2-phy";
+				#phy-cells = <0>;
+				reg = <0x0 0x20 0x0 0x20>;
+				resets = <&reset 34>;
+				clocks = <&clkc CLKID_USB &clkc CLKID_USB1>;

Ditto

arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi line 527

Quoted 13 lines

arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi +6 -0

+				clock-names = "usb_general", "usb";
+				status = "disabled";
+			};
+		};
+
 		cbus: cbus at c1100000 {
 			compatible = "simple-bus";
 			reg = <0x0 0xc1100000 0x0 0x100000>;

@@ -496,6 +524,30 @@
 			};
 		};
 
+		usb0: usb-controller at c9000000 {

usb at c9000000 by convention.

arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi line 539

Quoted 12 lines

arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi +12 -0

+			compatible = "amlogic,meson-gxbb-usb", "snps,dwc2";
+			reg = <0x0 0xc9000000 0x0 0x40000>;
+			interrupts = <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&clkc CLKID_USB0_DDR_BRIDGE>;
+			clock-names = "otg";
+			phys = <&usb0_phy>;
+			phy-names = "usb2-phy";
+			dr_mode = "host";
+			status = "disabled";
+		};
+
+		usb1: usb-controller at c9100000 {

usb at c9100000

Quoted 4 lines

arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi +4 -0

+			compatible = "amlogic,meson-gxbb-usb", "snps,dwc2";
+			reg = <0x0 0xc9100000 0x0 0x40000>;
+			interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&clkc CLKID_USB1_DDR_BRIDGE>;

arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi line 553

Quoted 10 lines

arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi +7 -0

+			clock-names = "otg";
+			phys = <&usb1_phy>;
+			phy-names = "usb2-phy";
+			dr_mode = "host";
+			status = "disabled";
+		};
+
 		ethmac: ethernet at c9410000 {
 			compatible = "amlogic,meson-gxbb-dwmac", "snps,dwmac";
 			reg = <0x0 0xc9410000 0x0 0x10000

Thanks for the huge progress you guys have been making.

This patchset will probably clash with Neil's GXL preparations?

Regards,
Andreas

From: Jerome Brunet <[email protected]>

Add compatible strings for amlogic Meson8b and GXBB SoCs with the corresponding configuration parameters.

Signed-off-by Jerome Brunet <[email protected]>

Signed-off-by Martin Blumenstingl <[email protected]>

---
 Documentation/devicetree/bindings/usb/dwc2.txt |  2 ++
 drivers/usb/dwc2/platform.c                    | 34 ++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

Documentation/devicetree/bindings/usb/dwc2.txt +2 -0

index 20a68bf..2c30a54 100644
@@ -10,6 +10,8 @@ Required properties:
   - "rockchip,rk3288-usb", "rockchip,rk3066-usb", "snps,dwc2": for rk3288 Soc;
   - "lantiq,arx100-usb": The DWC2 USB controller instance in Lantiq ARX SoCs;
   - "lantiq,xrx200-usb": The DWC2 USB controller instance in Lantiq XRX SoCs;
+  - "amlogic,meson8b-usb": The DWC2 USB controller instance in Amlogic Meson8b SoCs;
+  - "amlogic,meson-gxbb-usb": The DWC2 USB controller instance in Amlogic S905 SoCs;
   - snps,dwc2: A generic DWC2 USB controller with default parameters.
 - reg : Should contain 1 register range (address and length)
 - interrupts : Should contain 1 interrupt

drivers/usb/dwc2/platform.c +34 -0

index fc6f525..8f7b34c 100644
@@ -181,6 +181,38 @@ static const struct dwc2_core_params params_ltq = {
 	.hibernation			= -1,
 };
 
+static const struct dwc2_core_params params_amlogic = {
+	.otg_cap			= DWC2_CAP_PARAM_NO_HNP_SRP_CAPABLE,
+	.otg_ver			= -1,
+	.dma_enable			= 1,
+	.dma_desc_enable		= 0,
+	.dma_desc_fs_enable		= 0,
+	.speed				= DWC2_SPEED_PARAM_HIGH,
+	.enable_dynamic_fifo		= 1,
+	.en_multiple_tx_fifo		= -1,
+	.host_rx_fifo_size		= 512,
+	.host_nperio_tx_fifo_size	= 500,
+	.host_perio_tx_fifo_size	= 500,
+	.max_transfer_size		= -1,
+	.max_packet_count		= -1,
+	.host_channels			= 16,
+	.phy_type			= DWC2_PHY_TYPE_PARAM_UTMI,
+	.phy_utmi_width			= -1,
+	.phy_ulpi_ddr			= -1,
+	.phy_ulpi_ext_vbus		= -1,
+	.i2c_enable			= -1,
+	.ulpi_fs_ls			= -1,
+	.host_support_fs_ls_low_power	= -1,
+	.host_ls_low_power_phy_clk	= -1,
+	.ts_dline			= -1,
+	.reload_ctl			= 1,
+	.ahbcfg				= GAHBCFG_HBSTLEN_INCR8 <<
+					  GAHBCFG_HBSTLEN_SHIFT,
+	.uframe_sched			= 0,
+	.external_id_pin_ctl		= -1,
+	.hibernation			= -1,
+};
+
 /*
  * Check the dr_mode against the module configuration and hardware
  * capabilities.

@@ -464,6 +496,8 @@ static const struct of_device_id dwc2_of_match_table[] = {
 	{ .compatible = "lantiq,xrx200-usb", .data = &params_ltq },
 	{ .compatible = "snps,dwc2", .data = NULL },
 	{ .compatible = "samsung,s3c6400-hsotg", .data = NULL},
+	{ .compatible = "amlogic,meson8b-usb", .data = &params_amlogic },
+	{ .compatible = "amlogic,meson-gxbb-usb", .data = &params_amlogic },
 	{},
 };
 MODULE_DEVICE_TABLE(of, dwc2_of_match_table);

Hi John,

Quoted 7 lines · Martin Blumenstingl

From: Jerome Brunet <[email protected]>

Add compatible strings for amlogic Meson8b and GXBB SoCs with the corresponding configuration parameters.

Signed-off-by Jerome Brunet <[email protected]>

Signed-off-by Martin Blumenstingl <[email protected]>

Are you OK with adding this platform for v4.9? I know you mentioned you're working on new bindings to replace the current way, but since that hasn't been posted AFAICT, it would be nice to get this merged now and we can help test the new bindings when you're ready.

If you're OK with that, and with your Ack, I can take merge the driver changes through the arm-soc tree along with the rest of the DT patches.

Kevin

Quoted 70 lines

---
 Documentation/devicetree/bindings/usb/dwc2.txt |  2 ++
 drivers/usb/dwc2/platform.c                    | 34 ++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

Documentation/devicetree/bindings/usb/dwc2.txt +2 -0

index 20a68bf..2c30a54 100644
@@ -10,6 +10,8 @@ Required properties:
   - "rockchip,rk3288-usb", "rockchip,rk3066-usb", "snps,dwc2": for rk3288 Soc;
   - "lantiq,arx100-usb": The DWC2 USB controller instance in Lantiq ARX SoCs;
   - "lantiq,xrx200-usb": The DWC2 USB controller instance in Lantiq XRX SoCs;
+  - "amlogic,meson8b-usb": The DWC2 USB controller instance in Amlogic Meson8b SoCs;
+  - "amlogic,meson-gxbb-usb": The DWC2 USB controller instance in Amlogic S905 SoCs;
   - snps,dwc2: A generic DWC2 USB controller with default parameters.
 - reg : Should contain 1 register range (address and length)
 - interrupts : Should contain 1 interrupt

drivers/usb/dwc2/platform.c +34 -0

index fc6f525..8f7b34c 100644
@@ -181,6 +181,38 @@ static const struct dwc2_core_params params_ltq = {
 	.hibernation			= -1,
 };
 
+static const struct dwc2_core_params params_amlogic = {
+	.otg_cap			= DWC2_CAP_PARAM_NO_HNP_SRP_CAPABLE,
+	.otg_ver			= -1,
+	.dma_enable			= 1,
+	.dma_desc_enable		= 0,
+	.dma_desc_fs_enable		= 0,
+	.speed				= DWC2_SPEED_PARAM_HIGH,
+	.enable_dynamic_fifo		= 1,
+	.en_multiple_tx_fifo		= -1,
+	.host_rx_fifo_size		= 512,
+	.host_nperio_tx_fifo_size	= 500,
+	.host_perio_tx_fifo_size	= 500,
+	.max_transfer_size		= -1,
+	.max_packet_count		= -1,
+	.host_channels			= 16,
+	.phy_type			= DWC2_PHY_TYPE_PARAM_UTMI,
+	.phy_utmi_width			= -1,
+	.phy_ulpi_ddr			= -1,
+	.phy_ulpi_ext_vbus		= -1,
+	.i2c_enable			= -1,
+	.ulpi_fs_ls			= -1,
+	.host_support_fs_ls_low_power	= -1,
+	.host_ls_low_power_phy_clk	= -1,
+	.ts_dline			= -1,
+	.reload_ctl			= 1,
+	.ahbcfg				= GAHBCFG_HBSTLEN_INCR8 <<
+					  GAHBCFG_HBSTLEN_SHIFT,
+	.uframe_sched			= 0,
+	.external_id_pin_ctl		= -1,
+	.hibernation			= -1,
+};
+
 /*
  * Check the dr_mode against the module configuration and hardware
  * capabilities.

@@ -464,6 +496,8 @@ static const struct of_device_id dwc2_of_match_table[] = {
 	{ .compatible = "lantiq,xrx200-usb", .data = &params_ltq },
 	{ .compatible = "snps,dwc2", .data = NULL },
 	{ .compatible = "samsung,s3c6400-hsotg", .data = NULL},
+	{ .compatible = "amlogic,meson8b-usb", .data = &params_amlogic },
+	{ .compatible = "amlogic,meson-gxbb-usb", .data = &params_amlogic },
 	{},
 };
 MODULE_DEVICE_TABLE(of, dwc2_of_match_table);

Quoted 12 lines · Kevin Hilman

Hi John,

Quoted 7 lines · Martin Blumenstingl

From: Jerome Brunet <[email protected]>

Add compatible strings for amlogic Meson8b and GXBB SoCs with the corresponding configuration parameters.

Signed-off-by Jerome Brunet <[email protected]>

Signed-off-by Martin Blumenstingl <[email protected]>

Quoted 8 lines

Are you OK with adding this platform for v4.9? I know you mentioned you're working on new bindings to replace the current way, but since that hasn't been posted AFAICT, it would be nice to get this merged now and we can help test the new bindings when you're ready.

If you're OK with that, and with your Ack, I can take merge the driver changes through the arm-soc tree along with the rest of the DT patches.

Sure. Unfortunately, I wasn't able to complete it for 4.9.

You can add my acked-by on this:

Acked-by John Youn <[email protected]>

Quoted 24 lines · John Youn

Quoted 12 lines · Kevin Hilman

Hi John,

Quoted 7 lines · Martin Blumenstingl

From: Jerome Brunet <[email protected]>

Add compatible strings for amlogic Meson8b and GXBB SoCs with the corresponding configuration parameters.

Signed-off-by Jerome Brunet <[email protected]>

Signed-off-by Martin Blumenstingl <[email protected]>

Quoted 8 lines

Are you OK with adding this platform for v4.9? I know you mentioned you're working on new bindings to replace the current way, but since that hasn't been posted AFAICT, it would be nice to get this merged now and we can help test the new bindings when you're ready.

If you're OK with that, and with your Ack, I can take merge the driver changes through the arm-soc tree along with the rest of the DT patches.

Sure. Unfortunately, I wasn't able to complete it for 4.9.

Quoted 3 lines

You can add my acked-by on this:

Acked-by John Youn <[email protected]>

Great, thanks! I assume you're OK with PATCH 2/2 for the bindings as well (at least for now until you rework them?)

Kevin

Quoted 29 lines · Kevin Hilman

Quoted 27 lines · John Youn

Quoted 12 lines · Kevin Hilman

Hi John,

Quoted 7 lines · Martin Blumenstingl

From: Jerome Brunet <[email protected]>

Add compatible strings for amlogic Meson8b and GXBB SoCs with the corresponding configuration parameters.

Signed-off-by Jerome Brunet <[email protected]>

Signed-off-by Martin Blumenstingl <[email protected]>

Quoted 8 lines

Are you OK with adding this platform for v4.9? I know you mentioned you're working on new bindings to replace the current way, but since that hasn't been posted AFAICT, it would be nice to get this merged now and we can help test the new bindings when you're ready.

If you're OK with that, and with your Ack, I can take merge the driver changes through the arm-soc tree along with the rest of the DT patches.

Sure. Unfortunately, I wasn't able to complete it for 4.9.

You can add my acked-by on this:

Acked-by John Youn <[email protected]>

Quoted 5 lines

Great, thanks! I assume you're OK with PATCH 2/2 for the bindings as well (at least for now until you rework them?)

Kevin

Do you mean PATCH 2/6 from this series?

It doesn't seem to add or remove any bindings in dwc2 so I'm fine with it.

From: Jerome Brunet <[email protected]>

Add compatible strings for amlogic Meson8b and GXBB SoCs with the corresponding configuration parameters.

Signed-off-by Jerome Brunet <[email protected]>

Signed-off-by Martin Blumenstingl <[email protected]>

---
 Documentation/devicetree/bindings/usb/dwc2.txt |  2 ++

 drivers/usb/dwc2/platform.c                    | 34 ++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

Add the documentation for the bindings for the Meson8b and GXBB USB2 PHYs.

Signed-off-by Martin Blumenstingl <[email protected]>

---
 .../devicetree/bindings/phy/meson-usb2-phy.txt     | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/meson-usb2-phy.txt

Documentation/devicetree/bindings/phy/meson-usb2-phy.txt new file +9 -0

new file mode 100644
index 0000000..9da5ea2
@@ -0,0 +1,27 @@
+* Amlogic USB2 PHY
+
+Required properties:
+- compatible:	Depending on the platform this should be one of:
+	"amlogic,meson8b-usb2-phy"
+	"amlogic,meson-gxbb-usb2-phy"
+- reg:		The base address and length of the registers
+- #phys-cells:	should be 0 (see phy-bindings.txt in this directory)
+- clocks:	phandle and clock identifier for the phy clocks

Documentation/devicetree/bindings/phy/meson-usb2-phy.txt new file +8 -0

+	compatible = "amlogic,meson-gxbb-usb2-phy";
+	#phy-cells = <0>;
+	reg = <0x0 0x0 0x0 0x20>;
+	resets = <&reset RESET_USB_OTG>;
+	clocks = <&clkc CLKID_USB>, <&clkc CLKID_USB0>;
+	clock-names = "usb_general", "usb";
+	phy-supply = <&usb_vbus>;
+};

Signature · 1 line

2.9.3

Quoted 24 lines · Martin Blumenstingl

Add the documentation for the bindings for the Meson8b and GXBB USB2 PHYs.

Signed-off-by Martin Blumenstingl <[email protected]>

---
 .../devicetree/bindings/phy/meson-usb2-phy.txt     | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/meson-usb2-phy.txt

Documentation/devicetree/bindings/phy/meson-usb2-phy.txt new file +9 -0

new file mode 100644
index 0000000..9da5ea2
@@ -0,0 +1,27 @@
+* Amlogic USB2 PHY
+
+Required properties:
+- compatible:	Depending on the platform this should be one of:
+	"amlogic,meson8b-usb2-phy"
+	"amlogic,meson-gxbb-usb2-phy"
+- reg:		The base address and length of the registers
+- #phys-cells:	should be 0 (see phy-bindings.txt in this directory)
+- clocks:	phandle and clock identifier for the phy clocks

Documentation/devicetree/bindings/phy/meson-usb2-phy.txt line 19

Quoted 10 lines

Documentation/devicetree/bindings/phy/meson-usb2-phy.txt new file +10 -0

+- clock-names:	"usb_general" and "usb"
+
+Optional properties:
+- resets:	reference to the reset controller
+- phy-supply:	see phy-bindings.txt in this directory
+
+
+Example:
+
+usb0_phy: usb_phy at 0 {

usb-phy at 0

With that,

Acked-by Rob Herring <[email protected]>

This is a new driver for the USB PHY found in Meson8b and GXBB SoCs.

Signed-off-by Martin Blumenstingl <[email protected]>

Signed-off-by Jerome Brunet <[email protected]>

Tested-by Kevin Hilman <[email protected]>

---
 drivers/phy/Kconfig          |  11 ++
 drivers/phy/Makefile         |   1 +
 drivers/phy/phy-meson-usb2.c | 280 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 292 insertions(+)
 create mode 100644 drivers/phy/phy-meson-usb2.c

drivers/phy/Kconfig +11 -0

index 19bff3a..6ad87ec 100644
@@ -453,4 +453,15 @@ config PHY_NS2_PCIE
 	help
 	  Enable this to support the Broadcom Northstar2 PCIe PHY.
 	  If unsure, say N.
+
+config PHY_MESON_USB2
+	tristate "Meson USB2 PHY driver"
+	default ARCH_MESON
+	depends on OF && (ARCH_MESON || COMPILE_TEST)
+	select GENERIC_PHY
+	help
+	  Enable this to support the Meson USB2 PHYs found in Meson8b
+	  and GXBB SoCs.
+	  If unsure, say N.
+
 endmenu

drivers/phy/Makefile +1 -0

index 90ae198..dd507ac 100644
@@ -56,3 +56,4 @@ obj-$(CONFIG_PHY_PISTACHIO_USB)		+= phy-pistachio-usb.o
 obj-$(CONFIG_PHY_CYGNUS_PCIE)		+= phy-bcm-cygnus-pcie.o
 obj-$(CONFIG_ARCH_TEGRA) += tegra/
 obj-$(CONFIG_PHY_NS2_PCIE)		+= phy-bcm-ns2-pcie.o
+obj-$(CONFIG_PHY_MESON_USB2)		+= phy-meson-usb2.o

drivers/phy/phy-meson-usb2.c new file +280 -0

new file mode 100644
index 0000000..eece521
@@ -0,0 +1,280 @@
+/*
+ * Meson USB2 PHY driver
+ *
+ * Copyright (C) 2016 Martin Blumenstingl <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/reset.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/usb/of.h>
+
+#define REG_CONFIG					0x00
+	#define REG_CONFIG_CLK_EN			BIT(0)
+	#define REG_CONFIG_CLK_SEL_MASK			GENMASK(3, 1)
+	#define REG_CONFIG_CLK_DIV_MASK			GENMASK(10, 4)
+	#define REG_CONFIG_CLK_32k_ALTSEL		BIT(15)
+	#define REG_CONFIG_TEST_TRIG			BIT(31)
+
+#define REG_CTRL					0x04
+	#define REG_CTRL_SOFT_PRST			BIT(0)
+	#define REG_CTRL_SOFT_HRESET			BIT(1)
+	#define REG_CTRL_SS_SCALEDOWN_MODE_MASK		GENMASK(3, 2)
+	#define REG_CTRL_CLK_DET_RST			BIT(4)
+	#define REG_CTRL_INTR_SEL			BIT(5)
+	#define REG_CTRL_CLK_DETECTED			BIT(8)
+	#define REG_CTRL_SOF_SENT_RCVD_TGL		BIT(9)
+	#define REG_CTRL_SOF_TOGGLE_OUT			BIT(10)
+	#define REG_CTRL_POWER_ON_RESET			BIT(15)
+	#define REG_CTRL_SLEEPM				BIT(16)
+	#define REG_CTRL_TX_BITSTUFF_ENN_H		BIT(17)
+	#define REG_CTRL_TX_BITSTUFF_ENN		BIT(18)
+	#define REG_CTRL_COMMON_ON			BIT(19)
+	#define REG_CTRL_REF_CLK_SEL_MASK		GENMASK(21, 20)
+	#define REG_CTRL_REF_CLK_SEL_SHIFT		20
+	#define REG_CTRL_FSEL_MASK			GENMASK(24, 22)
+	#define REG_CTRL_FSEL_SHIFT			22
+	#define REG_CTRL_PORT_RESET			BIT(25)
+	#define REG_CTRL_THREAD_ID_MASK			GENMASK(31, 26)
+
+#define REG_ENDP_INTR					0x08
+
+/* bits [31:26], [24:21] and [15:3] seem to be read-only */
+#define REG_ADP_BC					0x0c
+	#define REG_ADP_BC_VBUS_VLD_EXT_SEL		BIT(0)
+	#define REG_ADP_BC_VBUS_VLD_EXT			BIT(1)
+	#define REG_ADP_BC_OTG_DISABLE			BIT(2)
+	#define REG_ADP_BC_ID_PULLUP			BIT(3)
+	#define REG_ADP_BC_DRV_VBUS			BIT(4)
+	#define REG_ADP_BC_ADP_PRB_EN			BIT(5)
+	#define REG_ADP_BC_ADP_DISCHARGE		BIT(6)
+	#define REG_ADP_BC_ADP_CHARGE			BIT(7)
+	#define REG_ADP_BC_SESS_END			BIT(8)
+	#define REG_ADP_BC_DEVICE_SESS_VLD		BIT(9)
+	#define REG_ADP_BC_B_VALID			BIT(10)
+	#define REG_ADP_BC_A_VALID			BIT(11)
+	#define REG_ADP_BC_ID_DIG			BIT(12)
+	#define REG_ADP_BC_VBUS_VALID			BIT(13)
+	#define REG_ADP_BC_ADP_PROBE			BIT(14)
+	#define REG_ADP_BC_ADP_SENSE			BIT(15)
+	#define REG_ADP_BC_ACA_ENABLE			BIT(16)
+	#define REG_ADP_BC_DCD_ENABLE			BIT(17)
+	#define REG_ADP_BC_VDAT_DET_EN_B		BIT(18)
+	#define REG_ADP_BC_VDAT_SRC_EN_B		BIT(19)
+	#define REG_ADP_BC_CHARGE_SEL			BIT(20)
+	#define REG_ADP_BC_CHARGE_DETECT		BIT(21)
+	#define REG_ADP_BC_ACA_PIN_RANGE_C		BIT(22)
+	#define REG_ADP_BC_ACA_PIN_RANGE_B		BIT(23)
+	#define REG_ADP_BC_ACA_PIN_RANGE_A		BIT(24)
+	#define REG_ADP_BC_ACA_PIN_GND			BIT(25)
+	#define REG_ADP_BC_ACA_PIN_FLOAT		BIT(26)
+
+#define REG_DBG_UART					0x14
+
+#define REG_TEST					0x18
+	#define REG_TEST_DATA_IN_MASK			GENMASK(3, 0)
+	#define REG_TEST_EN_MASK			GENMASK(7, 4)
+	#define REG_TEST_ADDR_MASK			GENMASK(11, 8)
+	#define REG_TEST_DATA_OUT_SEL			BIT(12)
+	#define REG_TEST_CLK				BIT(13)
+	#define REG_TEST_VA_TEST_EN_B_MASK		GENMASK(15, 14)
+	#define REG_TEST_DATA_OUT_MASK			GENMASK(19, 16)
+	#define REG_TEST_DISABLE_ID_PULLUP		BIT(20)
+
+#define REG_TUNE					0x1c
+	#define REG_TUNE_TX_RES_TUNE_MASK		GENMASK(1, 0)
+	#define REG_TUNE_TX_HSXV_TUNE_MASK		GENMASK(3, 2)
+	#define REG_TUNE_TX_VREF_TUNE_MASK		GENMASK(7, 4)
+	#define REG_TUNE_TX_RISE_TUNE_MASK		GENMASK(9, 8)
+	#define REG_TUNE_TX_PREEMP_PULSE_TUNE		BIT(10)
+	#define REG_TUNE_TX_PREEMP_AMP_TUNE_MASK	GENMASK(12, 11)
+	#define REG_TUNE_TX_FSLS_TUNE_MASK		GENMASK(16, 13)
+	#define REG_TUNE_SQRX_TUNE_MASK			GENMASK(19, 17)
+	#define REG_TUNE_OTG_TUNE			GENMASK(22, 20)
+	#define REG_TUNE_COMP_DIS_TUNE			GENMASK(25, 23)
+	#define REG_TUNE_HOST_DM_PULLDOWN		BIT(26)
+	#define REG_TUNE_HOST_DP_PULLDOWN		BIT(27)
+
+#define RESET_COMPLETE_TIME				500
+#define ACA_ENABLE_COMPLETE_TIME			50
+
+struct phy_meson_usb2_priv {
+	void __iomem		*regs;
+	enum usb_dr_mode	dr_mode;
+	struct clk		*clk_usb_general;
+	struct clk		*clk_usb;
+};
+
+static u32 phy_meson_usb2_read(struct phy_meson_usb2_priv *phy_priv, u32 reg)
+{
+	return readl(phy_priv->regs + reg);
+}
+
+static void phy_meson_usb2_mask_bits(struct phy_meson_usb2_priv *phy_priv,
+				     u32 reg, u32 mask, u32 value)
+{
+	u32 data;
+
+	data = phy_meson_usb2_read(phy_priv, reg);
+	data &= ~mask;
+	data |= (value & mask);
+
+	writel(data, phy_priv->regs + reg);
+}
+
+static int phy_meson_usb2_power_on(struct phy *phy)
+{
+	struct phy_meson_usb2_priv *priv = phy_get_drvdata(phy);
+	int ret;
+
+	ret = clk_prepare_enable(priv->clk_usb_general);
+	if (ret) {
+		dev_err(&phy->dev, "Failed to enable USB general clock\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(priv->clk_usb);
+	if (ret) {
+		dev_err(&phy->dev, "Failed to enable USB DDR clock\n");
+		return ret;
+	}
+
+	phy_meson_usb2_mask_bits(priv, REG_CONFIG, REG_CONFIG_CLK_32k_ALTSEL,
+				 REG_CONFIG_CLK_32k_ALTSEL);
+
+	phy_meson_usb2_mask_bits(priv, REG_CTRL, REG_CTRL_REF_CLK_SEL_MASK,
+				 0x2 << REG_CTRL_REF_CLK_SEL_SHIFT);
+
+	phy_meson_usb2_mask_bits(priv, REG_CTRL, REG_CTRL_FSEL_MASK,
+				 0x5 << REG_CTRL_FSEL_SHIFT);
+
+	/* reset the PHY */
+	phy_meson_usb2_mask_bits(priv, REG_CTRL, REG_CTRL_POWER_ON_RESET,
+				 REG_CTRL_POWER_ON_RESET);
+	udelay(RESET_COMPLETE_TIME);
+	phy_meson_usb2_mask_bits(priv, REG_CTRL, REG_CTRL_POWER_ON_RESET, 0);
+	udelay(RESET_COMPLETE_TIME);
+
+	phy_meson_usb2_mask_bits(priv, REG_CTRL, REG_CTRL_SOF_TOGGLE_OUT,
+				 REG_CTRL_SOF_TOGGLE_OUT);
+
+	if (priv->dr_mode == USB_DR_MODE_HOST) {
+		phy_meson_usb2_mask_bits(priv, REG_ADP_BC,
+					 REG_ADP_BC_ACA_ENABLE,
+					 REG_ADP_BC_ACA_ENABLE);
+
+		udelay(ACA_ENABLE_COMPLETE_TIME);
+
+		if (phy_meson_usb2_read(priv, REG_ADP_BC) &
+			REG_ADP_BC_ACA_PIN_FLOAT) {
+			dev_warn(&phy->dev, "USB ID detect failed!\n");
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static int phy_meson_usb2_power_off(struct phy *phy)
+{
+	struct phy_meson_usb2_priv *priv = phy_get_drvdata(phy);
+
+	clk_disable_unprepare(priv->clk_usb);
+	clk_disable_unprepare(priv->clk_usb_general);
+
+	return 0;
+}
+
+static const struct phy_ops phy_meson_usb2_ops = {
+	.power_on	= phy_meson_usb2_power_on,
+	.power_off	= phy_meson_usb2_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static int phy_meson_usb2_probe(struct platform_device *pdev)
+{
+	struct phy_meson_usb2_priv *priv;
+	struct resource *res;
+	struct phy *phy;
+	struct phy_provider *phy_provider;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->regs))
+		return PTR_ERR(priv->regs);
+
+	priv->clk_usb_general = devm_clk_get(&pdev->dev, "usb_general");
+	if (IS_ERR(priv->clk_usb_general))
+		return PTR_ERR(priv->clk_usb_general);
+
+	priv->clk_usb = devm_clk_get(&pdev->dev, "usb");
+	if (IS_ERR(priv->clk_usb))
+		return PTR_ERR(priv->clk_usb);
+
+	priv->dr_mode = of_usb_get_dr_mode_by_phy(pdev->dev.of_node, -1);
+	if (priv->dr_mode == USB_DR_MODE_UNKNOWN) {
+		dev_err(&pdev->dev,
+			"missing dual role configuration of the controller\n");
+		return -EINVAL;
+	}
+
+	phy = devm_phy_create(&pdev->dev, NULL, &phy_meson_usb2_ops);
+	if (IS_ERR(phy)) {
+		dev_err(&pdev->dev, "failed to create PHY\n");
+		return PTR_ERR(phy);
+	}
+
+	/*
+	 * No actual error check here because the hardware only has one reset
+	 * line for both PHYs. Using a shared reset is not possible because we
+	 * must call reset_control_reset to trigger the reset (which is not
+	 * allowed for shared resets in the reset framework).
+	 */
+	ret = device_reset_optional(&pdev->dev);
+	if (ret == -EPROBE_DEFER)
+		return ret;
+
+	phy_set_drvdata(phy, priv);
+
+	phy_provider =
+		devm_of_phy_provider_register(&pdev->dev, of_phy_simple_xlate);
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id phy_meson_usb2_of_match[] = {
+	{ .compatible = "amlogic,meson8b-usb2-phy", },
+	{ .compatible = "amlogic,meson-gxbb-usb2-phy", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, phy_meson_usb2_of_match);
+
+static struct platform_driver phy_meson_usb2_driver = {
+	.probe	= phy_meson_usb2_probe,
+	.driver	= {
+		.name		= "phy-meson-usb2",
+		.of_match_table	= phy_meson_usb2_of_match,
+	},
+};
+module_platform_driver(phy_meson_usb2_driver);
+
+MODULE_AUTHOR("Martin Blumenstingl <[email protected]>");
+MODULE_DESCRIPTION("Meson USB2 PHY driver");
+MODULE_LICENSE("GPL");

Kishon,

Quoted 5 lines · Martin Blumenstingl

This is a new driver for the USB PHY found in Meson8b and GXBB SoCs.

Signed-off-by Martin Blumenstingl <[email protected]>

Signed-off-by Jerome Brunet <[email protected]>

Tested-by Kevin Hilman <[email protected]>

Will you be picking this up for v4.9?

Kevin

Quoted 322 lines

---
 drivers/phy/Kconfig          |  11 ++
 drivers/phy/Makefile         |   1 +
 drivers/phy/phy-meson-usb2.c | 280 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 292 insertions(+)
 create mode 100644 drivers/phy/phy-meson-usb2.c

drivers/phy/Kconfig +11 -0

index 19bff3a..6ad87ec 100644
@@ -453,4 +453,15 @@ config PHY_NS2_PCIE
 	help
 	  Enable this to support the Broadcom Northstar2 PCIe PHY.
 	  If unsure, say N.
+
+config PHY_MESON_USB2
+	tristate "Meson USB2 PHY driver"
+	default ARCH_MESON
+	depends on OF && (ARCH_MESON || COMPILE_TEST)
+	select GENERIC_PHY
+	help
+	  Enable this to support the Meson USB2 PHYs found in Meson8b
+	  and GXBB SoCs.
+	  If unsure, say N.
+
 endmenu

drivers/phy/Makefile +1 -0

index 90ae198..dd507ac 100644
@@ -56,3 +56,4 @@ obj-$(CONFIG_PHY_PISTACHIO_USB)		+= phy-pistachio-usb.o
 obj-$(CONFIG_PHY_CYGNUS_PCIE)		+= phy-bcm-cygnus-pcie.o
 obj-$(CONFIG_ARCH_TEGRA) += tegra/
 obj-$(CONFIG_PHY_NS2_PCIE)		+= phy-bcm-ns2-pcie.o
+obj-$(CONFIG_PHY_MESON_USB2)		+= phy-meson-usb2.o

drivers/phy/phy-meson-usb2.c new file +280 -0

new file mode 100644
index 0000000..eece521
@@ -0,0 +1,280 @@
+/*
+ * Meson USB2 PHY driver
+ *
+ * Copyright (C) 2016 Martin Blumenstingl <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/reset.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/usb/of.h>
+
+#define REG_CONFIG					0x00
+	#define REG_CONFIG_CLK_EN			BIT(0)
+	#define REG_CONFIG_CLK_SEL_MASK			GENMASK(3, 1)
+	#define REG_CONFIG_CLK_DIV_MASK			GENMASK(10, 4)
+	#define REG_CONFIG_CLK_32k_ALTSEL		BIT(15)
+	#define REG_CONFIG_TEST_TRIG			BIT(31)
+
+#define REG_CTRL					0x04
+	#define REG_CTRL_SOFT_PRST			BIT(0)
+	#define REG_CTRL_SOFT_HRESET			BIT(1)
+	#define REG_CTRL_SS_SCALEDOWN_MODE_MASK		GENMASK(3, 2)
+	#define REG_CTRL_CLK_DET_RST			BIT(4)
+	#define REG_CTRL_INTR_SEL			BIT(5)
+	#define REG_CTRL_CLK_DETECTED			BIT(8)
+	#define REG_CTRL_SOF_SENT_RCVD_TGL		BIT(9)
+	#define REG_CTRL_SOF_TOGGLE_OUT			BIT(10)
+	#define REG_CTRL_POWER_ON_RESET			BIT(15)
+	#define REG_CTRL_SLEEPM				BIT(16)
+	#define REG_CTRL_TX_BITSTUFF_ENN_H		BIT(17)
+	#define REG_CTRL_TX_BITSTUFF_ENN		BIT(18)
+	#define REG_CTRL_COMMON_ON			BIT(19)
+	#define REG_CTRL_REF_CLK_SEL_MASK		GENMASK(21, 20)
+	#define REG_CTRL_REF_CLK_SEL_SHIFT		20
+	#define REG_CTRL_FSEL_MASK			GENMASK(24, 22)
+	#define REG_CTRL_FSEL_SHIFT			22
+	#define REG_CTRL_PORT_RESET			BIT(25)
+	#define REG_CTRL_THREAD_ID_MASK			GENMASK(31, 26)
+
+#define REG_ENDP_INTR					0x08
+
+/* bits [31:26], [24:21] and [15:3] seem to be read-only */
+#define REG_ADP_BC					0x0c
+	#define REG_ADP_BC_VBUS_VLD_EXT_SEL		BIT(0)
+	#define REG_ADP_BC_VBUS_VLD_EXT			BIT(1)
+	#define REG_ADP_BC_OTG_DISABLE			BIT(2)
+	#define REG_ADP_BC_ID_PULLUP			BIT(3)
+	#define REG_ADP_BC_DRV_VBUS			BIT(4)
+	#define REG_ADP_BC_ADP_PRB_EN			BIT(5)
+	#define REG_ADP_BC_ADP_DISCHARGE		BIT(6)
+	#define REG_ADP_BC_ADP_CHARGE			BIT(7)
+	#define REG_ADP_BC_SESS_END			BIT(8)
+	#define REG_ADP_BC_DEVICE_SESS_VLD		BIT(9)
+	#define REG_ADP_BC_B_VALID			BIT(10)
+	#define REG_ADP_BC_A_VALID			BIT(11)
+	#define REG_ADP_BC_ID_DIG			BIT(12)
+	#define REG_ADP_BC_VBUS_VALID			BIT(13)
+	#define REG_ADP_BC_ADP_PROBE			BIT(14)
+	#define REG_ADP_BC_ADP_SENSE			BIT(15)
+	#define REG_ADP_BC_ACA_ENABLE			BIT(16)
+	#define REG_ADP_BC_DCD_ENABLE			BIT(17)
+	#define REG_ADP_BC_VDAT_DET_EN_B		BIT(18)
+	#define REG_ADP_BC_VDAT_SRC_EN_B		BIT(19)
+	#define REG_ADP_BC_CHARGE_SEL			BIT(20)
+	#define REG_ADP_BC_CHARGE_DETECT		BIT(21)
+	#define REG_ADP_BC_ACA_PIN_RANGE_C		BIT(22)
+	#define REG_ADP_BC_ACA_PIN_RANGE_B		BIT(23)
+	#define REG_ADP_BC_ACA_PIN_RANGE_A		BIT(24)
+	#define REG_ADP_BC_ACA_PIN_GND			BIT(25)
+	#define REG_ADP_BC_ACA_PIN_FLOAT		BIT(26)
+
+#define REG_DBG_UART					0x14
+
+#define REG_TEST					0x18
+	#define REG_TEST_DATA_IN_MASK			GENMASK(3, 0)
+	#define REG_TEST_EN_MASK			GENMASK(7, 4)
+	#define REG_TEST_ADDR_MASK			GENMASK(11, 8)
+	#define REG_TEST_DATA_OUT_SEL			BIT(12)
+	#define REG_TEST_CLK				BIT(13)
+	#define REG_TEST_VA_TEST_EN_B_MASK		GENMASK(15, 14)
+	#define REG_TEST_DATA_OUT_MASK			GENMASK(19, 16)
+	#define REG_TEST_DISABLE_ID_PULLUP		BIT(20)
+
+#define REG_TUNE					0x1c
+	#define REG_TUNE_TX_RES_TUNE_MASK		GENMASK(1, 0)
+	#define REG_TUNE_TX_HSXV_TUNE_MASK		GENMASK(3, 2)
+	#define REG_TUNE_TX_VREF_TUNE_MASK		GENMASK(7, 4)
+	#define REG_TUNE_TX_RISE_TUNE_MASK		GENMASK(9, 8)
+	#define REG_TUNE_TX_PREEMP_PULSE_TUNE		BIT(10)
+	#define REG_TUNE_TX_PREEMP_AMP_TUNE_MASK	GENMASK(12, 11)
+	#define REG_TUNE_TX_FSLS_TUNE_MASK		GENMASK(16, 13)
+	#define REG_TUNE_SQRX_TUNE_MASK			GENMASK(19, 17)
+	#define REG_TUNE_OTG_TUNE			GENMASK(22, 20)
+	#define REG_TUNE_COMP_DIS_TUNE			GENMASK(25, 23)
+	#define REG_TUNE_HOST_DM_PULLDOWN		BIT(26)
+	#define REG_TUNE_HOST_DP_PULLDOWN		BIT(27)
+
+#define RESET_COMPLETE_TIME				500
+#define ACA_ENABLE_COMPLETE_TIME			50
+
+struct phy_meson_usb2_priv {
+	void __iomem		*regs;
+	enum usb_dr_mode	dr_mode;
+	struct clk		*clk_usb_general;
+	struct clk		*clk_usb;
+};
+
+static u32 phy_meson_usb2_read(struct phy_meson_usb2_priv *phy_priv, u32 reg)
+{
+	return readl(phy_priv->regs + reg);
+}
+
+static void phy_meson_usb2_mask_bits(struct phy_meson_usb2_priv *phy_priv,
+				     u32 reg, u32 mask, u32 value)
+{
+	u32 data;
+
+	data = phy_meson_usb2_read(phy_priv, reg);
+	data &= ~mask;
+	data |= (value & mask);
+
+	writel(data, phy_priv->regs + reg);
+}
+
+static int phy_meson_usb2_power_on(struct phy *phy)
+{
+	struct phy_meson_usb2_priv *priv = phy_get_drvdata(phy);
+	int ret;
+
+	ret = clk_prepare_enable(priv->clk_usb_general);
+	if (ret) {
+		dev_err(&phy->dev, "Failed to enable USB general clock\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(priv->clk_usb);
+	if (ret) {
+		dev_err(&phy->dev, "Failed to enable USB DDR clock\n");
+		return ret;
+	}
+
+	phy_meson_usb2_mask_bits(priv, REG_CONFIG, REG_CONFIG_CLK_32k_ALTSEL,
+				 REG_CONFIG_CLK_32k_ALTSEL);
+
+	phy_meson_usb2_mask_bits(priv, REG_CTRL, REG_CTRL_REF_CLK_SEL_MASK,
+				 0x2 << REG_CTRL_REF_CLK_SEL_SHIFT);
+
+	phy_meson_usb2_mask_bits(priv, REG_CTRL, REG_CTRL_FSEL_MASK,
+				 0x5 << REG_CTRL_FSEL_SHIFT);
+
+	/* reset the PHY */
+	phy_meson_usb2_mask_bits(priv, REG_CTRL, REG_CTRL_POWER_ON_RESET,
+				 REG_CTRL_POWER_ON_RESET);
+	udelay(RESET_COMPLETE_TIME);
+	phy_meson_usb2_mask_bits(priv, REG_CTRL, REG_CTRL_POWER_ON_RESET, 0);
+	udelay(RESET_COMPLETE_TIME);
+
+	phy_meson_usb2_mask_bits(priv, REG_CTRL, REG_CTRL_SOF_TOGGLE_OUT,
+				 REG_CTRL_SOF_TOGGLE_OUT);
+
+	if (priv->dr_mode == USB_DR_MODE_HOST) {
+		phy_meson_usb2_mask_bits(priv, REG_ADP_BC,
+					 REG_ADP_BC_ACA_ENABLE,
+					 REG_ADP_BC_ACA_ENABLE);
+
+		udelay(ACA_ENABLE_COMPLETE_TIME);
+
+		if (phy_meson_usb2_read(priv, REG_ADP_BC) &
+			REG_ADP_BC_ACA_PIN_FLOAT) {
+			dev_warn(&phy->dev, "USB ID detect failed!\n");
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static int phy_meson_usb2_power_off(struct phy *phy)
+{
+	struct phy_meson_usb2_priv *priv = phy_get_drvdata(phy);
+
+	clk_disable_unprepare(priv->clk_usb);
+	clk_disable_unprepare(priv->clk_usb_general);
+
+	return 0;
+}
+
+static const struct phy_ops phy_meson_usb2_ops = {
+	.power_on	= phy_meson_usb2_power_on,
+	.power_off	= phy_meson_usb2_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static int phy_meson_usb2_probe(struct platform_device *pdev)
+{
+	struct phy_meson_usb2_priv *priv;
+	struct resource *res;
+	struct phy *phy;
+	struct phy_provider *phy_provider;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->regs))
+		return PTR_ERR(priv->regs);
+
+	priv->clk_usb_general = devm_clk_get(&pdev->dev, "usb_general");
+	if (IS_ERR(priv->clk_usb_general))
+		return PTR_ERR(priv->clk_usb_general);
+
+	priv->clk_usb = devm_clk_get(&pdev->dev, "usb");
+	if (IS_ERR(priv->clk_usb))
+		return PTR_ERR(priv->clk_usb);
+
+	priv->dr_mode = of_usb_get_dr_mode_by_phy(pdev->dev.of_node, -1);
+	if (priv->dr_mode == USB_DR_MODE_UNKNOWN) {
+		dev_err(&pdev->dev,
+			"missing dual role configuration of the controller\n");
+		return -EINVAL;
+	}
+
+	phy = devm_phy_create(&pdev->dev, NULL, &phy_meson_usb2_ops);
+	if (IS_ERR(phy)) {
+		dev_err(&pdev->dev, "failed to create PHY\n");
+		return PTR_ERR(phy);
+	}
+
+	/*
+	 * No actual error check here because the hardware only has one reset
+	 * line for both PHYs. Using a shared reset is not possible because we
+	 * must call reset_control_reset to trigger the reset (which is not
+	 * allowed for shared resets in the reset framework).
+	 */
+	ret = device_reset_optional(&pdev->dev);
+	if (ret == -EPROBE_DEFER)
+		return ret;
+
+	phy_set_drvdata(phy, priv);
+
+	phy_provider =
+		devm_of_phy_provider_register(&pdev->dev, of_phy_simple_xlate);
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id phy_meson_usb2_of_match[] = {
+	{ .compatible = "amlogic,meson8b-usb2-phy", },
+	{ .compatible = "amlogic,meson-gxbb-usb2-phy", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, phy_meson_usb2_of_match);
+
+static struct platform_driver phy_meson_usb2_driver = {
+	.probe	= phy_meson_usb2_probe,
+	.driver	= {
+		.name		= "phy-meson-usb2",
+		.of_match_table	= phy_meson_usb2_of_match,
+	},
+};
+module_platform_driver(phy_meson_usb2_driver);
+
+MODULE_AUTHOR("Martin Blumenstingl <[email protected]>");
+MODULE_DESCRIPTION("Meson USB2 PHY driver");
+MODULE_LICENSE("GPL");

Hi Kevin,

Quoted 6 lines · Kevin Hilman

Kishon,

Quoted 2 lines · Martin Blumenstingl

This is a new driver for the USB PHY found in Meson8b and GXBB SoCs.

Quoted 5 lines

Quoted 3 lines

Signed-off-by Martin Blumenstingl <[email protected]>

Signed-off-by Jerome Brunet <[email protected]>

Tested-by Kevin Hilman <[email protected]>

Will you be picking this up for v4.9?

It's already late for 4.9. Generally send pull request to Greg around -rc6. This can go only in 4.10.

Quoted 15 lines · Kishon Vijay Abraham I

Hi Kevin,

Quoted 11 lines · Kevin Hilman

Kishon,

Quoted 5 lines · Martin Blumenstingl

This is a new driver for the USB PHY found in Meson8b and GXBB SoCs.

Signed-off-by Martin Blumenstingl <[email protected]>

Signed-off-by Jerome Brunet <[email protected]>

Tested-by Kevin Hilman <[email protected]>

Will you be picking this up for v4.9?

Quoted 2 lines

It's already late for 4.9. Generally send pull request to Greg around -rc6. This can go only in 4.10.

That's fine.

Does that mean you have it queued someplace? I don't see it in any of your branches.

This is a new driver for the USB PHY found in Meson8b and GXBB SoCs.

Signed-off-by Martin Blumenstingl <[email protected]>

Signed-off-by Jerome Brunet <[email protected]>

Tested-by Kevin Hilman <[email protected]>

---
 drivers/phy/Kconfig          |  11 ++
 drivers/phy/Makefile         |   1 +
 drivers/phy/phy-meson-usb2.c | 280 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 292 insertions(+)
 create mode 100644 drivers/phy/phy-meson-usb2.c

drivers/phy/Kconfig +0 -0

index 19bff3a..6ad87ec 100644

Add the nodes for the dwc2 USB controller and the related USB PHYs. Currently we force usb0 to host mode because OTG is currently not working in our PHY driver.

Signed-off-by Martin Blumenstingl <[email protected]>

Signed-off-by Jerome Brunet <[email protected]>

From: Jerome Brunet <[email protected]>

Enable both gxbb USB controller and add a 5V regulator for the OTG port VBUS

Signed-off-by Jerome Brunet <[email protected]>

---
 arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi | 29 ++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi +0 -0

index ce105fe..4493bce 100644

Quoted 6 lines · Martin Blumenstingl

From: Jerome Brunet <[email protected]>

Enable both gxbb USB controller and add a 5V regulator for the OTG port VBUS

Signed-off-by Jerome Brunet <[email protected]>

nit: subject should have "ARM64: dts:" prefix.

Quoted 9 lines

---
 arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi | 29 ++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi +0 -0

index ce105fe..4493bce 100644

arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi line 97

Quoted 5 lines

arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi +2 -0

@@ -93,6 +93,18 @@
 		compatible = "mmc-pwrseq-emmc";
 		reset-gpios = <&gpio BOOT_9 GPIO_ACTIVE_LOW>;
 	};
+
+	usb_vbus: regulator-usb0-vbus {

nit: I like to use the signal name from the schematics for the node name (and for regulator-name below). In the schematics, that signal is named USB_PWR.

arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi line 105

Quoted 7 lines

arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi +7 -0

+		compatible = "regulator-fixed";
+
+		regulator-name = "USB0_VBUS";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+
+		gpio = <&gpio GPIODV_24 GPIO_ACTIVE_HIGH>;

Please add a comment above this line with the schematic signal name: USB_PWR_EN.

Enable both gxbb USB controller and add a 5V regulator for the OTG port VBUS

Signed-off-by Martin Blumenstingl <[email protected]>

Signed-off-by Jerome Brunet <[email protected]>