Re: [PATCH v2 2/7] usb: dwc3: adapt dwc3 core to use Generic PHY Framework

[Roger Quadros <rogerq@ti.com>]

On 10/21/2013 02:55 PM, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Wednesday 16 October 2013 07:53 PM, Roger Quadros wrote:
>> On 10/16/2013 04:52 PM, Kishon Vijay Abraham I wrote:
>>> Hi,
>>>
>>> On Wednesday 16 October 2013 06:48 PM, Roger Quadros wrote:
>>>> Hi,
>>>>
>>>> On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote:
>>>>> Adapted dwc3 core to use the Generic PHY Framework. So for init, exit,
>>>>> power_on and power_off the following APIs are used phy_init(), phy_exit(),
>>>>> phy_power_on() and phy_power_off().
>>>>>
>>>>> However using the old USB phy library wont be removed till the PHYs of all
>>>>> other SoC's using dwc3 core is adapted to the Generic PHY Framework.
>>>>>
>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/usb/dwc3.txt |    6 ++-
>>>>>  drivers/usb/dwc3/Kconfig                       |    1 +
>>>>>  drivers/usb/dwc3/core.c                        |   52 ++++++++++++++++++++++++
>>>>>  drivers/usb/dwc3/core.h                        |    7 ++++
>>>>>  drivers/usb/dwc3/platform_data.h               |    2 +
>>>>>  5 files changed, 66 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> index e807635..471366d 100644
>>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> @@ -6,11 +6,13 @@ Required properties:
>>>>>   - compatible: must be "snps,dwc3"
>>>>>   - reg : Address and length of the register set for the device
>>>>>   - interrupts: Interrupts used by the dwc3 controller.
>>>>> +
>>>>> +Optional properties:
>>>>>   - usb-phy : array of phandle for the PHY device.  The first element
>>>>>     in the array is expected to be a handle to the USB2/HS PHY and
>>>>>     the second element is expected to be a handle to the USB3/SS PHY
>>>>> -
>>>>> -Optional properties:
>>>>> + - phys: from the *Generic PHY* bindings
>>>>> + - phy-names: from the *Generic PHY* bindings
>>>>>   - tx-fifo-resize: determines if the FIFO *has* to be reallocated.
>>>>>  
>>>>>  This is usually a subnode to DWC3 glue to which it is connected.
>>>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>>>>> index 8e385b4..64eed6f 100644
>>>>> --- a/drivers/usb/dwc3/Kconfig
>>>>> +++ b/drivers/usb/dwc3/Kconfig
>>>>> @@ -2,6 +2,7 @@ config USB_DWC3
>>>>>  	tristate "DesignWare USB3 DRD Core Support"
>>>>>  	depends on (USB || USB_GADGET) && HAS_DMA
>>>>>  	select USB_PHY
>>>>> +	select GENERIC_PHY
>>>>>  	select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD
>>>>>  	help
>>>>>  	  Say Y or M here if your system has a Dual Role SuperSpeed
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index cb91d70..28bfa5b 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>> @@ -82,6 +82,12 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc)
>>>>>  
>>>>>  	usb_phy_init(dwc->usb2_phy);
>>>>>  	usb_phy_init(dwc->usb3_phy);
>>>>> +
>>>>> +	if (dwc->usb2_generic_phy)
>>>>> +		phy_init(dwc->usb2_generic_phy);
>>>>> +	if (dwc->usb3_generic_phy)
>>>>> +		phy_init(dwc->usb3_generic_phy);
>>>>> +
>>>>
>>>> dwc3_core_soft_reset() is called from dwc3_core_init() which is called from
>>>> dwc3_probe() but before the PHYs are initialized.
>>>>
>>>> This will cause phy power_on() to be called before phy_init() which is wrong.
>>>>
>>
>> You overlooked the above?
> 
> No. Forgot to comment back.
> Yeah, you are right.. that should be fixed.
>>
>>>>>  	mdelay(100);
>>>>>  
>>>>>  	/* Clear USB3 PHY reset */
>>>>> @@ -343,6 +349,11 @@ static void dwc3_core_exit(struct dwc3 *dwc)
>>>>>  {
>>>>>  	usb_phy_shutdown(dwc->usb2_phy);
>>>>>  	usb_phy_shutdown(dwc->usb3_phy);
>>>>> +
>>>>> +	if (dwc->usb2_generic_phy)
>>>>> +		phy_power_off(dwc->usb2_generic_phy);
>>>>> +	if (dwc->usb3_generic_phy)
>>>>> +		phy_power_off(dwc->usb3_generic_phy);
>>>>>  }
>>>>>  
>>>>>  #define DWC3_ALIGN_MASK		(16 - 1)
>>>>> @@ -439,6 +450,26 @@ static int dwc3_probe(struct platform_device *pdev)
>>>>>  		dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
>>>>>  	}
>>>>>  
>>>>> +	count = of_property_match_string(node, "phy-names", "usb2-phy");
>>>>> +	if (count >= 0 || (pdata && pdata->usb2_generic_phy)) {
>>>>> +		dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
>>>>> +		if (IS_ERR(dwc->usb2_generic_phy)) {
>>>>> +			dev_err(dev, "no usb2 phy configured yet");
>>>>> +			return PTR_ERR(dwc->usb2_generic_phy);
>>>>> +		}
>>>>> +		dwc->usb2_phy = NULL;
>>>>> +	}
>>>>> +
>>>>> +	count = of_property_match_string(node, "phy-names", "usb3-phy");
>>>>> +	if (count >= 0 || (pdata && pdata->usb3_generic_phy)) {
>>>>> +		dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
>>>>> +		if (IS_ERR(dwc->usb3_generic_phy)) {
>>>>> +			dev_err(dev, "no usb3 phy configured yet");
>>>>> +			return PTR_ERR(dwc->usb3_generic_phy);
>>>>> +		}
>>>>> +		dwc->usb3_phy = NULL;
>>>>> +	}
>>>>> +
>>>>
>>>> There are a couple of issues here.
>>>> - The above code must be executed only if it node is valid.
>>>
>>> of_property_match_string will give a error value if the node is not present.
>>> *count >= 0* can take care of this.
>>
>> OK. 
>>
>>>> - We must either get both PHYs via old method or via new method and not support mix matching
>>>> them. e.g. it is possible with this code that usb2_phy is set and usb3_generic_phy is set.
>>>
>>> Why? As long as both the frameworks co-exist (and we support both in dwc3), I
>>> don't see any reason why we shouldn't allow it. We can avoid adding a few more
>>> checks by leaving it the way it is currently.
>>
>> Because it just doesn't seem elegant. Why would you want to even allow both types of PHY
>> to be used simultaneously?
> 
> We actually don't allow both types of PHYs to be used simultaneously.  We set
> usb2_phy/usb3_phy to NULL, if we had got the the generic PHY.
> If you just doesn't even want to get PHY by the second method if we have got
> PHY by the first method, then we might need to add more *if* checks which IMO
> don't make it any elegant either.
> Having a clean dt data wont have both types of PHYs anyway.

OK.

cheers,
-roger