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@xxxxxx>
>>>>> ---
>>>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
