RE: [PATCH v6 8/8] usb: ehci-exynos: Change to use phy provided by the generic phy framework

[Kamil Debski <k.debski@samsung.com>]

Hi Alan,

Thank you for your review.

> From: Alan Stern [mailto:stern@rowland.harvard.edu]
> Sent: Wednesday, January 29, 2014 9:43 PM
> 
> On Wed, 29 Jan 2014, Kamil Debski wrote:
> 
> > Change the phy provider used from the old one using the USB phy
> > framework to a new one using the Generic phy framework.
> >
> > Signed-off-by: Kamil Debski <k.debski@samsung.com>
> > ---
> >  .../devicetree/bindings/usb/exynos-usb.txt         |   13 +++
> >  drivers/usb/host/ehci-exynos.c                     |   97
> +++++++++++++-------
> >  2 files changed, 76 insertions(+), 34 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt
> > b/Documentation/devicetree/bindings/usb/exynos-usb.txt
> > index d967ba1..25e199a 100644
> > --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
> > +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
> > @@ -12,6 +12,10 @@ Required properties:
> >   - interrupts: interrupt number to the cpu.
> >   - clocks: from common clock binding: handle to usb clock.
> >   - clock-names: from common clock binding: Shall be "usbhost".
> > +  - port: if in the SoC there are EHCI phys, they should be listed
> here.
> > +One phy per port. Each port should have its reg entry with a
> > +consecutive number. Also it should contain phys and phy-names
> entries
> > +specifying the phy used by the port.
> 
> What is the reg entry number used for?  As far as I can see, it isn't
> used for anything.  In which case, why have it at all?

Tomasz Figa already commented this. I agree with him, that this should
be better described in the documentation.

> 
> > @@ -42,10 +42,10 @@
> >  static const char hcd_name[] = "ehci-exynos";  static struct
> > hc_driver __read_mostly exynos_ehci_hc_driver;
> >
> > +#define PHY_NUMBER 3
> >  struct exynos_ehci_hcd {
> >  	struct clk *clk;
> > -	struct usb_phy *phy;
> > -	struct usb_otg *otg;
> 
> You have removed all the OTG stuff from the driver.  This wasn't
> mentioned in the patch description, and it has no connection with the
> PHY work.

Maybe I'll explain more about what are we trying to achieve. The goal
is to replace the old phy driver with the new one. In the old driver it was
difficult to add support to new SoC. It also had issues with having device
and
host working together.

You're right that until the old phy driver is removed support for the it
should
remain. To be able to remove the old driver both new USB2 and new USB3 phy
drivers
have to be ready. The USB3 driver is written by Vivek Gautam and as I see
it, he
is also close to completion.

Regarding the otg part. The old phy driver is the only provider of the otg
structure. It sets the host field of the structure. It is then used by
samsung_usb2phy_init (drivers/usb/phy/phy-samsung-usb2.c) to check which
driver is requesting the phy (is it host or device). In the new driver this
is determined by the entry in device tree. So no need to check the otg
struct
and strstr (!) to check if dev_name is ehci, ohci or other, like the old
driver.

> > +	struct phy *phy[PHY_NUMBER];
> >  };
> >
> >  #define to_exynos_ehci(hcd) (struct exynos_ehci_hcd
> > *)(hcd_to_ehci(hcd)->priv) @@ -69,13 +69,43 @@ static void
> exynos_setup_vbus_gpio(struct platform_device *pdev)
> >  		dev_err(dev, "can't request ehci vbus gpio %d", gpio);  }
> >
> > +static int exynos_phys_on(struct phy *p[]) {
> > +	int i;
> > +	int ret = 0;
> > +
> > +	for (i = 0; ret == 0 && i < PHY_NUMBER; i++)
> > +		if (p[i])
> > +			ret = phy_power_on(p[i]);
> > +	if (ret)
> > +		for (i--; i > 0; i--)
> > +			if (p[i])
> > +				phy_power_off(p[i]);
> 
> This loop runs while i > 0.  Therefore you will never turn off the
> power to p[0].

Ups, my bad. Thank you for spotting this.

> 
> > @@ -102,14 +132,26 @@ static int exynos_ehci_probe(struct
> platform_device *pdev)
> >  					"samsung,exynos5440-ehci"))
> >  		goto skip_phy;
> >
> > -	phy = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2);
> > -	if (IS_ERR(phy)) {
> > -		usb_put_hcd(hcd);
> 
> You omitted this line from the new error returns below.

I see.

> 
> > -		dev_warn(&pdev->dev, "no platform data or transceiver
> defined\n");
> > -		return -EPROBE_DEFER;
> > -	} else {
> > -		exynos_ehci->phy = phy;
> > -		exynos_ehci->otg = phy->otg;
> > +	for_each_available_child_of_node(pdev->dev.of_node, child) {
> > +		err = of_property_read_u32(child, "reg", &phy_number);
> > +		if (err) {
> > +			dev_err(&pdev->dev, "Failed to parse device
tree\n");
> > +			of_node_put(child);
> > +			return err;
> 
> Here, for example.  Wouldn't it be better to goto fail_clk?

Right, I will fix this. Thank you.

Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland