diff mbox series

[U-Boot,RESEND,1/5] usb: host: dwc2: add phy support

Message ID 20191014080025.11245-2-patrick.delaunay@st.com
State Superseded
Delegated to: Marek Vasut
Headers show
Series usb: host: dwc2: use driver model for PHY and CLOCK | expand

Commit Message

Patrick Delaunay Oct. 14, 2019, 8 a.m. UTC
Use generic phy to initialize the PHY associated to the
DWC2 device and available in the device tree.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 drivers/usb/host/dwc2.c | 61 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

Comments

Marek Vasut Oct. 14, 2019, 11:26 p.m. UTC | #1
On 10/14/19 10:00 AM, Patrick Delaunay wrote:
> Use generic phy to initialize the PHY associated to the

PHY and USB are abbreviations, should be in capitals.

> DWC2 device and available in the device tree.

[...]

General question -- is the PHY subsystem a mandatory dependency of this
driver now or will it work without the PHY subsystem still ?

> +static int dwc2_setup_phy(struct udevice *dev)
> +{
> +	struct dwc2_priv *priv = dev_get_priv(dev);
> +	int ret;
> +
> +	ret = generic_phy_get_by_index(dev, 0, &priv->phy);
> +	if (ret) {
> +		if (ret != -ENOENT) {
> +			dev_err(dev, "failed to get usb phy\n");

Sentence starts with capital letter, USB and PHY are in capitals. Fix
globally please.

It would be useful to print the $ret value too.

> +			return ret;
> +		}
> +		return 0;
> +	}
> +
> +	ret = generic_phy_init(&priv->phy);
> +	if (ret) {
> +		dev_err(dev, "failed to init usb phy\n");
> +		return ret;
> +	}
> +
> +	ret = generic_phy_power_on(&priv->phy);
> +	if (ret) {
> +		dev_err(dev, "failed to power on usb phy\n");
> +		return generic_phy_exit(&priv->phy);
> +	}
> +
> +	return 0;
> +}
> +
> +static int dwc2_shutdown_phy(struct udevice *dev)
> +{
> +	struct dwc2_priv *priv = dev_get_priv(dev);
> +	int ret;
> +
> +	if (!generic_phy_valid(&priv->phy))
> +		return 0;
> +
> +	ret = generic_phy_power_off(&priv->phy);
> +	if (ret) {
> +		dev_err(dev, "failed to power off usb phy\n");
> +		return ret;
> +	}
> +
> +	ret = generic_phy_exit(&priv->phy);
> +	if (ret) {
> +		dev_err(dev, "failed to power off usb phy\n");

Shouldn't all those error prints be produced by the PHY subsystem ?

> +		return ret;

[...]

> @@ -1339,6 +1398,8 @@ static int dwc2_usb_remove(struct udevice *dev)
>  	if (ret)
>  		return ret;
>  
> +	dwc2_shutdown_phy(dev);

This function returns a return value, but it's ignored here ?

>  	dwc2_uninit_common(priv->regs);
>  
>  	reset_release_bulk(&priv->resets);
> 

[...]
Patrick Delaunay Nov. 6, 2019, 5:40 p.m. UTC | #2
Hi Marek,

> From: Marek Vasut <marex@denx.de>
> Sent: mardi 15 octobre 2019 01:27

First sorry for my late answer....
 
> On 10/14/19 10:00 AM, Patrick Delaunay wrote:
> > Use generic phy to initialize the PHY associated to the
> 
> PHY and USB are abbreviations, should be in capitals.
> 
> > DWC2 device and available in the device tree.
> 
> [...]
> 
> General question -- is the PHY subsystem a mandatory dependency of this driver
> now or will it work without the PHY subsystem still ?

Normally it is working as all the generic_phy_XXX fucntions
are stubbed in include/generic-phy.h

- generic_phy_get_by_index() return 0 and phy->dev = NULL
- all other function return 0
- generic_phy_valid return FALSE (phy->dev = NULL)

 
> > +static int dwc2_setup_phy(struct udevice *dev) {
> > +	struct dwc2_priv *priv = dev_get_priv(dev);
> > +	int ret;
> > +
> > +	ret = generic_phy_get_by_index(dev, 0, &priv->phy);
> > +	if (ret) {
> > +		if (ret != -ENOENT) {
> > +			dev_err(dev, "failed to get usb phy\n");
> 
> Sentence starts with capital letter, USB and PHY are in capitals. Fix globally
> please.
> It would be useful to print the $ret value too.

Yes, in V2

> 
> > +			return ret;
> > +		}
> > +		return 0;
> > +	}
> > +
> > +	ret = generic_phy_init(&priv->phy);
> > +	if (ret) {
> > +		dev_err(dev, "failed to init usb phy\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = generic_phy_power_on(&priv->phy);
> > +	if (ret) {
> > +		dev_err(dev, "failed to power on usb phy\n");
> > +		return generic_phy_exit(&priv->phy);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int dwc2_shutdown_phy(struct udevice *dev) {
> > +	struct dwc2_priv *priv = dev_get_priv(dev);
> > +	int ret;
> > +
> > +	if (!generic_phy_valid(&priv->phy))
> > +		return 0;
> > +
> > +	ret = generic_phy_power_off(&priv->phy);
> > +	if (ret) {
> > +		dev_err(dev, "failed to power off usb phy\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = generic_phy_exit(&priv->phy);
> > +	if (ret) {
> > +		dev_err(dev, "failed to power off usb phy\n");
> 
> Shouldn't all those error prints be produced by the PHY subsystem ?

Perhaps... but it is not done today in phy u-class (only call ops).

I make the same level of trace than ./drivers/usb/dwc3/core.c
as copy initially the phy support from this driver.

> > +		return ret;
> 
> [...]
> 
> > @@ -1339,6 +1398,8 @@ static int dwc2_usb_remove(struct udevice *dev)
> >  	if (ret)
> >  		return ret;
> >
> > +	dwc2_shutdown_phy(dev);
> 
> This function returns a return value, but it's ignored here ?

Yes, even if the shutdown of the USB PHY failed, the USB dwc2
 driver continues the procedure to release other ressources...

And the driver expects that the USB PHY will be available for next
request/probe (recovery with phy reset for example).

I use the same logic than dwc3 driver in :
source/drivers/usb/dwc3/dwc3-generic.c::dwc3_generic_remove()
drivers/usb/host/xhci-dwc3.c::xhci_dwc3_remove()

> 
> >  	dwc2_uninit_common(priv->regs);
> >
> >  	reset_release_bulk(&priv->resets);
> >
> 
> [...]

Regards
Patrick
Marek Vasut Nov. 6, 2019, 9:55 p.m. UTC | #3
On 11/6/19 6:40 PM, Patrick DELAUNAY wrote:
> Hi Marek,

Hi,

[...]

>>> +static int dwc2_shutdown_phy(struct udevice *dev) {
>>> +	struct dwc2_priv *priv = dev_get_priv(dev);
>>> +	int ret;
>>> +
>>> +	if (!generic_phy_valid(&priv->phy))
>>> +		return 0;
>>> +
>>> +	ret = generic_phy_power_off(&priv->phy);
>>> +	if (ret) {
>>> +		dev_err(dev, "failed to power off usb phy\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = generic_phy_exit(&priv->phy);
>>> +	if (ret) {
>>> +		dev_err(dev, "failed to power off usb phy\n");
>>
>> Shouldn't all those error prints be produced by the PHY subsystem ?
> 
> Perhaps... but it is not done today in phy u-class (only call ops).
> 
> I make the same level of trace than ./drivers/usb/dwc3/core.c
> as copy initially the phy support from this driver.

So this starts the duplication. Can you move it to the PHY subsystem
instead ?

>>> +		return ret;
>>
>> [...]
>>
>>> @@ -1339,6 +1398,8 @@ static int dwc2_usb_remove(struct udevice *dev)
>>>  	if (ret)
>>>  		return ret;
>>>
>>> +	dwc2_shutdown_phy(dev);
>>
>> This function returns a return value, but it's ignored here ?
> 
> Yes, even if the shutdown of the USB PHY failed, the USB dwc2
>  driver continues the procedure to release other ressources...

How can you safely release the rest of the resources if the PHY driver
didn't shut down? I suspect this might lead to some resource corruption, no?

> And the driver expects that the USB PHY will be available for next
> request/probe (recovery with phy reset for example).
> 
> I use the same logic than dwc3 driver in :
> source/drivers/usb/dwc3/dwc3-generic.c::dwc3_generic_remove()
> drivers/usb/host/xhci-dwc3.c::xhci_dwc3_remove()

dwc3_shutdown_phy() only ever returns 0 though.
Patrick Delaunay Nov. 8, 2019, 1:25 p.m. UTC | #4
Hi,

> From: Marek Vasut <marex@denx.de>
> 
> On 11/6/19 6:40 PM, Patrick DELAUNAY wrote:
> > Hi Marek,
> 
> Hi,
> 
> [...]
> 
> >>> +static int dwc2_shutdown_phy(struct udevice *dev) {
> >>> +	struct dwc2_priv *priv = dev_get_priv(dev);
> >>> +	int ret;
> >>> +
> >>> +	if (!generic_phy_valid(&priv->phy))
> >>> +		return 0;
> >>> +
> >>> +	ret = generic_phy_power_off(&priv->phy);
> >>> +	if (ret) {
> >>> +		dev_err(dev, "failed to power off usb phy\n");
> >>> +		return ret;
> >>> +	}
> >>> +
> >>> +	ret = generic_phy_exit(&priv->phy);
> >>> +	if (ret) {
> >>> +		dev_err(dev, "failed to power off usb phy\n");
> >>
> >> Shouldn't all those error prints be produced by the PHY subsystem ?
> >
> > Perhaps... but it is not done today in phy u-class (only call ops).
> >
> > I make the same level of trace than ./drivers/usb/dwc3/core.c as copy
> > initially the phy support from this driver.
> 
> So this starts the duplication. Can you move it to the PHY subsystem instead ?

Yes I can, in v2 I will change dev_err to dev_dbg

And I will sent a other serie to change the generic phy (add printf or dev_err) 
and also remove the dev_err for all the caller to avoid duplicated trace.

This generic error is already done in some U-Boot uclass,
- clock (clk_enable)

But sometime only the caller, the driver,  knows if it is a error or a warning,
and it is not done for others uclass, for example:

- Reset: reset_assert/ reset_deassert reset_assert_bulk/ reset_deassert_bulk
- Regulator: regulator_set_enable

> >>> +		return ret;
> >>
> >> [...]
> >>
> >>> @@ -1339,6 +1398,8 @@ static int dwc2_usb_remove(struct udevice *dev)
> >>>  	if (ret)
> >>>  		return ret;
> >>>
> >>> +	dwc2_shutdown_phy(dev);
> >>
> >> This function returns a return value, but it's ignored here ?
> >
> > Yes, even if the shutdown of the USB PHY failed, the USB dwc2  driver
> > continues the procedure to release other ressources...
> 
> How can you safely release the rest of the resources if the PHY driver didn't shut
> down? I suspect this might lead to some resource corruption, no?

Yes...and that depends of the PHY driver.

What it is better stategy:
- try to continue to release the resources after the first error and the next probe could works / the error is masked
Or
- stopped the release procedure => the next procedure could failed (resource not available)

> > And the driver expects that the USB PHY will be available for next
> > request/probe (recovery with phy reset for example).
> >
> > I use the same logic than dwc3 driver in :
> > source/drivers/usb/dwc3/dwc3-generic.c::dwc3_generic_remove()
> > drivers/usb/host/xhci-dwc3.c::xhci_dwc3_remove()
> 
> dwc3_shutdown_phy() only ever returns 0 though.

Yes, but in dwc3_shutdown_phy, the phy operation can have errors
and the "remove" procedure continue (even if ret is never retruned)

ret = generic_phy_power_off(&usb_phys[i]);
ret |= generic_phy_exit(&usb_phys[i]);
if (ret) {
	pr_err("Can't shutdown USB PHY%d for %s\n", i, dev->name);
}

Anyway I will treat error in v2, it should be more clear in dw2c code.

+	ret= dwc2_shutdown_phy(dev);
+	if (ret) {
+		dev_dbg(dev, "Failed to shutdown USB PHY: %d.\n": ret);
+		return ret;
+	}

> --
> Best regards,
> Marek Vasut

Regards

Patrick
Marek Vasut Nov. 8, 2019, 3:41 p.m. UTC | #5
On 11/8/19 2:25 PM, Patrick DELAUNAY wrote:
Hi,

[...]
>>>>> +static int dwc2_shutdown_phy(struct udevice *dev) {
>>>>> +	struct dwc2_priv *priv = dev_get_priv(dev);
>>>>> +	int ret;
>>>>> +
>>>>> +	if (!generic_phy_valid(&priv->phy))
>>>>> +		return 0;
>>>>> +
>>>>> +	ret = generic_phy_power_off(&priv->phy);
>>>>> +	if (ret) {
>>>>> +		dev_err(dev, "failed to power off usb phy\n");
>>>>> +		return ret;
>>>>> +	}
>>>>> +
>>>>> +	ret = generic_phy_exit(&priv->phy);
>>>>> +	if (ret) {
>>>>> +		dev_err(dev, "failed to power off usb phy\n");
>>>>
>>>> Shouldn't all those error prints be produced by the PHY subsystem ?
>>>
>>> Perhaps... but it is not done today in phy u-class (only call ops).
>>>
>>> I make the same level of trace than ./drivers/usb/dwc3/core.c as copy
>>> initially the phy support from this driver.
>>
>> So this starts the duplication. Can you move it to the PHY subsystem instead ?
> 
> Yes I can, in v2 I will change dev_err to dev_dbg
> 
> And I will sent a other serie to change the generic phy (add printf or dev_err) 
> and also remove the dev_err for all the caller to avoid duplicated trace.

Thanks

[...]

>>>>> @@ -1339,6 +1398,8 @@ static int dwc2_usb_remove(struct udevice *dev)
>>>>>  	if (ret)
>>>>>  		return ret;
>>>>>
>>>>> +	dwc2_shutdown_phy(dev);
>>>>
>>>> This function returns a return value, but it's ignored here ?
>>>
>>> Yes, even if the shutdown of the USB PHY failed, the USB dwc2  driver
>>> continues the procedure to release other ressources...
>>
>> How can you safely release the rest of the resources if the PHY driver didn't shut
>> down? I suspect this might lead to some resource corruption, no?
> 
> Yes...and that depends of the PHY driver.

Does it ?

> What it is better stategy:
> - try to continue to release the resources after the first error and the next probe could works / the error is masked
> Or
> - stopped the release procedure => the next procedure could failed (resource not available)

Stop the release procedure, because then at least the dynamically
allocated data are still tracked by allocator and if the driver so
decided to use them, they will still exist. If you were to release them,
then the driver could either misbehave and/or cause memory corruption.

>>> And the driver expects that the USB PHY will be available for next
>>> request/probe (recovery with phy reset for example).
>>>
>>> I use the same logic than dwc3 driver in :
>>> source/drivers/usb/dwc3/dwc3-generic.c::dwc3_generic_remove()
>>> drivers/usb/host/xhci-dwc3.c::xhci_dwc3_remove()
>>
>> dwc3_shutdown_phy() only ever returns 0 though.
> 
> Yes, but in dwc3_shutdown_phy, the phy operation can have errors
> and the "remove" procedure continue (even if ret is never retruned)
> 
> ret = generic_phy_power_off(&usb_phys[i]);
> ret |= generic_phy_exit(&usb_phys[i]);
> if (ret) {
> 	pr_err("Can't shutdown USB PHY%d for %s\n", i, dev->name);
> }
> 
> Anyway I will treat error in v2, it should be more clear in dw2c code.
That doesn't sound right.

[...]
diff mbox series

Patch

diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index 350d820a6e..eb1026effc 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -7,6 +7,7 @@ 
 #include <common.h>
 #include <dm.h>
 #include <errno.h>
+#include <generic-phy.h>
 #include <usb.h>
 #include <malloc.h>
 #include <memalign.h>
@@ -35,6 +36,7 @@  struct dwc2_priv {
 #ifdef CONFIG_DM_REGULATOR
 	struct udevice *vbus_supply;
 #endif
+	struct phy phy;
 #else
 	uint8_t *aligned_buffer;
 	uint8_t *status_buffer;
@@ -1320,13 +1322,70 @@  static int dwc2_usb_ofdata_to_platdata(struct udevice *dev)
 	return 0;
 }
 
+static int dwc2_setup_phy(struct udevice *dev)
+{
+	struct dwc2_priv *priv = dev_get_priv(dev);
+	int ret;
+
+	ret = generic_phy_get_by_index(dev, 0, &priv->phy);
+	if (ret) {
+		if (ret != -ENOENT) {
+			dev_err(dev, "failed to get usb phy\n");
+			return ret;
+		}
+		return 0;
+	}
+
+	ret = generic_phy_init(&priv->phy);
+	if (ret) {
+		dev_err(dev, "failed to init usb phy\n");
+		return ret;
+	}
+
+	ret = generic_phy_power_on(&priv->phy);
+	if (ret) {
+		dev_err(dev, "failed to power on usb phy\n");
+		return generic_phy_exit(&priv->phy);
+	}
+
+	return 0;
+}
+
+static int dwc2_shutdown_phy(struct udevice *dev)
+{
+	struct dwc2_priv *priv = dev_get_priv(dev);
+	int ret;
+
+	if (!generic_phy_valid(&priv->phy))
+		return 0;
+
+	ret = generic_phy_power_off(&priv->phy);
+	if (ret) {
+		dev_err(dev, "failed to power off usb phy\n");
+		return ret;
+	}
+
+	ret = generic_phy_exit(&priv->phy);
+	if (ret) {
+		dev_err(dev, "failed to power off usb phy\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static int dwc2_usb_probe(struct udevice *dev)
 {
 	struct dwc2_priv *priv = dev_get_priv(dev);
 	struct usb_bus_priv *bus_priv = dev_get_uclass_priv(dev);
+	int ret;
 
 	bus_priv->desc_before_addr = true;
 
+	ret = dwc2_setup_phy(dev);
+	if (ret)
+		return ret;
+
 	return dwc2_init_common(dev, priv);
 }
 
@@ -1339,6 +1398,8 @@  static int dwc2_usb_remove(struct udevice *dev)
 	if (ret)
 		return ret;
 
+	dwc2_shutdown_phy(dev);
+
 	dwc2_uninit_common(priv->regs);
 
 	reset_release_bulk(&priv->resets);