[v7,3/5] usb: dwc2: add generic PHY framework support for dwc2 usb controler platform driver.

Commit Message

Paul Zimmerman Jan. 13, 2015, 12:54 a.m. UTC

> From: Paul Zimmerman

> Sent: Saturday, January 10, 2015 3:52 PM

> 

> > From: Yunzhi Li [mailto:[e-mail]]

> > Sent: Saturday, January 10, 2015 8:07 AM

> >

> > ? 2015/1/9 10:15, Paul Zimmerman ??:

> > >> [...]

> > >>   	/*

> > >> -	 * Attempt to find a generic PHY, then look for an old style

> > >> -	 * USB PHY, finally fall back to pdata

> > >> +	 * If platform probe couldn't find a generic PHY or an old style

> > >> +	 * USB PHY, fall back to pdata

> > >>   	 */

> > >> -	phy = devm_phy_get(dev, "usb2-phy");

> > >> -	if (IS_ERR(phy)) {

> > >> -		uphy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);

> > >> -		if (IS_ERR(uphy)) {

> > >> -			/* Fallback for pdata */

> > >> -			plat = dev_get_platdata(dev);

> > >> -			if (!plat) {

> > >> -				dev_err(dev,

> > >> -				"no platform data or transceiver defined\n");

> > >> -				return -EPROBE_DEFER;

> > >> -			}

> > >> -			hsotg->plat = plat;

> > >> -		} else

> > >> -			hsotg->uphy = uphy;

> > >> -	} else {

> > >> -		hsotg->phy = phy;

> > >> +	if (IS_ERR_OR_NULL(hsotg->phy) && IS_ERR_OR_NULL(hsotg->uphy)) {

> > >> +		plat = dev_get_platdata(dev);

> > >> +		if (!plat) {

> > >> +			dev_err(dev,

> > >> +			"no platform data or transceiver defined\n");

> > >> +			return -EPROBE_DEFER;

> > > Hi Yunzhi,

> > >

> > > Testing Felipe's testing/next branch on an Altera SOCFPGA platform,

> > > the driver never loads because it always returns -EPROBE_DEFER here.

> > > Apparently the SOCFPGA platform does not have any platform data

> > > defined, because dev_get_platdata() always returns NULL.

> > >

> > > If I remove the -EPROBE_DEFER return and have it continue on, the

> > > driver works. Reverting the patch also makes it work.

> > 

> > When I debug this problem, I checked socfpga.dtsi, there is a

> > usbphy node defined for each

> > dwc2 controller, so I think when running dwc2_driver_probe() uphy =

> > devm_usb_get_phy()

> > should get a valid usbphy pointer and hsotg->uphy will not be NULL or

> > ERROR, then in dwc2_gadget_init()

> > it will not return -EPROBE_DEFER. I have no idea about why you meet

> > -EPROBE_DEFER, could you please tell

> > me what's the return value of devm_usb_get_phy() on your socfpga board ?

> 

> I'm away from the hardware right now, but I just found this in a saved

> boot log:

> 

> [    1.097268] usb_phy_generic soc:usbphy@0: Error requesting RESET GPIO

> [    1.097285] usb_phy_generic: probe of soc:usbphy@0 failed with error -2

> 

> So that probably explains it. I'll dig into this some more on Monday.


The patch below fixes it. And it seems like the right thing to me,
since GPIOs should be optional for a generic phy, I would think. But
my device tree foo is very weak, so I'm not sure.

CCing Robert, who touched the generic phy code last. Robert, what do
you think?

-- 
Paul


--

Comments

Robert Jarzmik Jan. 13, 2015, 9:35 a.m. UTC | #1

Paul Zimmerman <[e-mail]> writes:
> The patch below fixes it. And it seems like the right thing to me,
> since GPIOs should be optional for a generic phy, I would think. But
> my device tree foo is very weak, so I'm not sure.
>
> CCing Robert, who touched the generic phy code last. Robert, what do
> you think?
I think your patch in [1] is correct, because
"Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt" states that
reset-gpios is optional, and because Felipe told me gpios for phy_generic are
optional.

Now the original code was written by Felipe, so better ask him first. The
original code was :
af9f51c55 phy-generic.c nop->gpio_reset = of_get_named_gpio_flags(node, "reset-gpios",
af9f51c55 phy-generic.c                                                 0, &flags);
af9f51c55 phy-generic.c if (nop->gpio_reset == -EPROBE_DEFER)
af9f51c55 phy-generic.c         return -EPROBE_DEFER;

Cheers.

--
Robert

[1]
> diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
> index dd05254..9a826ff 100644
> --- a/drivers/usb/phy/phy-generic.c
> +++ b/drivers/usb/phy/phy-generic.c
> @@ -218,10 +218,10 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop,
>  			clk_rate = 0;
>  
>  		needs_vcc = of_property_read_bool(node, "vcc-supply");
> -		nop->gpiod_reset = devm_gpiod_get(dev, "reset-gpios");
> +		nop->gpiod_reset = devm_gpiod_get_optional(dev, "reset-gpios");
>  		err = PTR_ERR(nop->gpiod_reset);
>  		if (!err) {
> -			nop->gpiod_vbus = devm_gpiod_get(dev,
> +			nop->gpiod_vbus = devm_gpiod_get_optional(dev,
>  							 "vbus-detect-gpio");
>  			err = PTR_ERR(nop->gpiod_vbus);
>  		}

Felipe Balbi Jan. 13, 2015, 3:30 p.m. UTC | #2

On Tue, Jan 13, 2015 at 10:35:52AM +0100, Robert Jarzmik wrote:
> Paul Zimmerman <[e-mail]> writes:
> > The patch below fixes it. And it seems like the right thing to me,
> > since GPIOs should be optional for a generic phy, I would think. But
> > my device tree foo is very weak, so I'm not sure.
> >
> > CCing Robert, who touched the generic phy code last. Robert, what do
> > you think?
> I think your patch in [1] is correct, because
> "Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt" states that
> reset-gpios is optional, and because Felipe told me gpios for phy_generic are
> optional.
> 
> Now the original code was written by Felipe, so better ask him first. The
> original code was :
> af9f51c55 phy-generic.c nop->gpio_reset = of_get_named_gpio_flags(node, "reset-gpios",
> af9f51c55 phy-generic.c                                                 0, &flags);
> af9f51c55 phy-generic.c if (nop->gpio_reset == -EPROBE_DEFER)
> af9f51c55 phy-generic.c         return -EPROBE_DEFER;

yeah, at the time that was written, there was no _optional() varianto of
get_gpio, if there is one now, we should use it, indeed.

I can rather easily test here on BBB to make sure there are no
regressions.

Patch

diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
index dd05254..9a826ff 100644
--- a/drivers/usb/phy/phy-generic.c
+++ b/drivers/usb/phy/phy-generic.c
@@ -218,10 +218,10 @@  int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop,
 			clk_rate = 0;
 
 		needs_vcc = of_property_read_bool(node, "vcc-supply");
-		nop->gpiod_reset = devm_gpiod_get(dev, "reset-gpios");
+		nop->gpiod_reset = devm_gpiod_get_optional(dev, "reset-gpios");
 		err = PTR_ERR(nop->gpiod_reset);
 		if (!err) {
-			nop->gpiod_vbus = devm_gpiod_get(dev,
+			nop->gpiod_vbus = devm_gpiod_get_optional(dev,
 							 "vbus-detect-gpio");
 			err = PTR_ERR(nop->gpiod_vbus);
 		}