* [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
@ 2020-04-20 13:46 ` Neil Armstrong
0 siblings, 0 replies; 26+ messages in thread
From: Neil Armstrong @ 2020-04-20 13:46 UTC (permalink / raw)
To: u-boot
Power on/off the PHYs to enable power to the USB ports, fixing USB support
on Khadas VIM3/VIM3L boards.
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
drivers/usb/dwc3/dwc3-meson-g12a.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
index d4453f8784..8f4a2f3f82 100644
--- a/drivers/usb/dwc3/dwc3-meson-g12a.c
+++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
@@ -408,6 +408,15 @@ static int dwc3_meson_g12a_probe(struct udevice *dev)
goto err_phy_init;
}
+ for (i = 0 ; i < PHY_COUNT ; ++i) {
+ if (!priv->phys[i].dev)
+ continue;
+
+ ret = generic_phy_power_on(&priv->phys[i]);
+ if (ret)
+ goto err_phy_init;
+ }
+
return 0;
err_phy_init:
@@ -430,6 +439,13 @@ static int dwc3_meson_g12a_remove(struct udevice *dev)
clk_release_all(&priv->clk, 1);
+ for (i = 0 ; i < PHY_COUNT ; ++i) {
+ if (!priv->phys[i].dev)
+ continue;
+
+ generic_phy_power_off(&priv->phys[i]);
+ }
+
for (i = 0 ; i < PHY_COUNT ; ++i) {
if (!priv->phys[i].dev)
continue;
--
2.22.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
@ 2020-04-20 13:46 ` Neil Armstrong
0 siblings, 0 replies; 26+ messages in thread
From: Neil Armstrong @ 2020-04-20 13:46 UTC (permalink / raw)
To: u-boot-amlogic, marex; +Cc: u-boot, Neil Armstrong
Power on/off the PHYs to enable power to the USB ports, fixing USB support
on Khadas VIM3/VIM3L boards.
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
drivers/usb/dwc3/dwc3-meson-g12a.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
index d4453f8784..8f4a2f3f82 100644
--- a/drivers/usb/dwc3/dwc3-meson-g12a.c
+++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
@@ -408,6 +408,15 @@ static int dwc3_meson_g12a_probe(struct udevice *dev)
goto err_phy_init;
}
+ for (i = 0 ; i < PHY_COUNT ; ++i) {
+ if (!priv->phys[i].dev)
+ continue;
+
+ ret = generic_phy_power_on(&priv->phys[i]);
+ if (ret)
+ goto err_phy_init;
+ }
+
return 0;
err_phy_init:
@@ -430,6 +439,13 @@ static int dwc3_meson_g12a_remove(struct udevice *dev)
clk_release_all(&priv->clk, 1);
+ for (i = 0 ; i < PHY_COUNT ; ++i) {
+ if (!priv->phys[i].dev)
+ continue;
+
+ generic_phy_power_off(&priv->phys[i]);
+ }
+
for (i = 0 ; i < PHY_COUNT ; ++i) {
if (!priv->phys[i].dev)
continue;
--
2.22.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
2020-04-20 13:46 ` Neil Armstrong
@ 2020-04-20 13:47 ` Marek Vasut
-1 siblings, 0 replies; 26+ messages in thread
From: Marek Vasut @ 2020-04-20 13:47 UTC (permalink / raw)
To: u-boot
On 4/20/20 3:46 PM, Neil Armstrong wrote:
> Power on/off the PHYs to enable power to the USB ports, fixing USB support
> on Khadas VIM3/VIM3L boards.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
> drivers/usb/dwc3/dwc3-meson-g12a.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
> index d4453f8784..8f4a2f3f82 100644
> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
> @@ -408,6 +408,15 @@ static int dwc3_meson_g12a_probe(struct udevice *dev)
> goto err_phy_init;
> }
>
> + for (i = 0 ; i < PHY_COUNT ; ++i) {
Doesn't checkpatch complain about this extra space before semicolon ?
> + if (!priv->phys[i].dev)
> + continue;
> +
> + ret = generic_phy_power_on(&priv->phys[i]);
Do we really need to turn on all the PHYs ?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
@ 2020-04-20 13:47 ` Marek Vasut
0 siblings, 0 replies; 26+ messages in thread
From: Marek Vasut @ 2020-04-20 13:47 UTC (permalink / raw)
To: Neil Armstrong, u-boot-amlogic; +Cc: u-boot
On 4/20/20 3:46 PM, Neil Armstrong wrote:
> Power on/off the PHYs to enable power to the USB ports, fixing USB support
> on Khadas VIM3/VIM3L boards.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
> drivers/usb/dwc3/dwc3-meson-g12a.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
> index d4453f8784..8f4a2f3f82 100644
> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
> @@ -408,6 +408,15 @@ static int dwc3_meson_g12a_probe(struct udevice *dev)
> goto err_phy_init;
> }
>
> + for (i = 0 ; i < PHY_COUNT ; ++i) {
Doesn't checkpatch complain about this extra space before semicolon ?
> + if (!priv->phys[i].dev)
> + continue;
> +
> + ret = generic_phy_power_on(&priv->phys[i]);
Do we really need to turn on all the PHYs ?
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
2020-04-20 13:47 ` Marek Vasut
@ 2020-04-20 13:49 ` Neil Armstrong
-1 siblings, 0 replies; 26+ messages in thread
From: Neil Armstrong @ 2020-04-20 13:49 UTC (permalink / raw)
To: u-boot
On 20/04/2020 15:47, Marek Vasut wrote:
> On 4/20/20 3:46 PM, Neil Armstrong wrote:
>> Power on/off the PHYs to enable power to the USB ports, fixing USB support
>> on Khadas VIM3/VIM3L boards.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>> drivers/usb/dwc3/dwc3-meson-g12a.c | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
>> index d4453f8784..8f4a2f3f82 100644
>> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
>> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
>> @@ -408,6 +408,15 @@ static int dwc3_meson_g12a_probe(struct udevice *dev)
>> goto err_phy_init;
>> }
>>
>> + for (i = 0 ; i < PHY_COUNT ; ++i) {
>
> Doesn't checkpatch complain about this extra space before semicolon ?
Nop, even in --strict
>
>> + if (!priv->phys[i].dev)
>> + continue;
>> +
>> + ret = generic_phy_power_on(&priv->phys[i]);
>
> Do we really need to turn on all the PHYs ?
>
Yes
Neil
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
@ 2020-04-20 13:49 ` Neil Armstrong
0 siblings, 0 replies; 26+ messages in thread
From: Neil Armstrong @ 2020-04-20 13:49 UTC (permalink / raw)
To: Marek Vasut, u-boot-amlogic; +Cc: u-boot
On 20/04/2020 15:47, Marek Vasut wrote:
> On 4/20/20 3:46 PM, Neil Armstrong wrote:
>> Power on/off the PHYs to enable power to the USB ports, fixing USB support
>> on Khadas VIM3/VIM3L boards.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>> drivers/usb/dwc3/dwc3-meson-g12a.c | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
>> index d4453f8784..8f4a2f3f82 100644
>> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
>> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
>> @@ -408,6 +408,15 @@ static int dwc3_meson_g12a_probe(struct udevice *dev)
>> goto err_phy_init;
>> }
>>
>> + for (i = 0 ; i < PHY_COUNT ; ++i) {
>
> Doesn't checkpatch complain about this extra space before semicolon ?
Nop, even in --strict
>
>> + if (!priv->phys[i].dev)
>> + continue;
>> +
>> + ret = generic_phy_power_on(&priv->phys[i]);
>
> Do we really need to turn on all the PHYs ?
>
Yes
Neil
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
2020-04-20 13:49 ` Neil Armstrong
@ 2020-04-20 13:52 ` Marek Vasut
-1 siblings, 0 replies; 26+ messages in thread
From: Marek Vasut @ 2020-04-20 13:52 UTC (permalink / raw)
To: u-boot
On 4/20/20 3:49 PM, Neil Armstrong wrote:
> On 20/04/2020 15:47, Marek Vasut wrote:
>> On 4/20/20 3:46 PM, Neil Armstrong wrote:
>>> Power on/off the PHYs to enable power to the USB ports, fixing USB support
>>> on Khadas VIM3/VIM3L boards.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>> ---
>>> drivers/usb/dwc3/dwc3-meson-g12a.c | 16 ++++++++++++++++
>>> 1 file changed, 16 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
>>> index d4453f8784..8f4a2f3f82 100644
>>> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
>>> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
>>> @@ -408,6 +408,15 @@ static int dwc3_meson_g12a_probe(struct udevice *dev)
>>> goto err_phy_init;
>>> }
>>>
>>> + for (i = 0 ; i < PHY_COUNT ; ++i) {
>>
>> Doesn't checkpatch complain about this extra space before semicolon ?
>
> Nop, even in --strict
Shouldn't it though ? I thought this extra space was forbidden.
>>
>>> + if (!priv->phys[i].dev)
>>> + continue;
>>> +
>>> + ret = generic_phy_power_on(&priv->phys[i]);
>>
>> Do we really need to turn on all the PHYs ?
>>
>
> Yes
Then should we have something like clk_bulk_*(), but for phys ?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
@ 2020-04-20 13:52 ` Marek Vasut
0 siblings, 0 replies; 26+ messages in thread
From: Marek Vasut @ 2020-04-20 13:52 UTC (permalink / raw)
To: Neil Armstrong, u-boot-amlogic; +Cc: u-boot
On 4/20/20 3:49 PM, Neil Armstrong wrote:
> On 20/04/2020 15:47, Marek Vasut wrote:
>> On 4/20/20 3:46 PM, Neil Armstrong wrote:
>>> Power on/off the PHYs to enable power to the USB ports, fixing USB support
>>> on Khadas VIM3/VIM3L boards.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>> ---
>>> drivers/usb/dwc3/dwc3-meson-g12a.c | 16 ++++++++++++++++
>>> 1 file changed, 16 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
>>> index d4453f8784..8f4a2f3f82 100644
>>> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
>>> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
>>> @@ -408,6 +408,15 @@ static int dwc3_meson_g12a_probe(struct udevice *dev)
>>> goto err_phy_init;
>>> }
>>>
>>> + for (i = 0 ; i < PHY_COUNT ; ++i) {
>>
>> Doesn't checkpatch complain about this extra space before semicolon ?
>
> Nop, even in --strict
Shouldn't it though ? I thought this extra space was forbidden.
>>
>>> + if (!priv->phys[i].dev)
>>> + continue;
>>> +
>>> + ret = generic_phy_power_on(&priv->phys[i]);
>>
>> Do we really need to turn on all the PHYs ?
>>
>
> Yes
Then should we have something like clk_bulk_*(), but for phys ?
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
2020-04-20 13:52 ` Marek Vasut
@ 2020-04-20 13:56 ` Neil Armstrong
-1 siblings, 0 replies; 26+ messages in thread
From: Neil Armstrong @ 2020-04-20 13:56 UTC (permalink / raw)
To: u-boot
On 20/04/2020 15:52, Marek Vasut wrote:
> On 4/20/20 3:49 PM, Neil Armstrong wrote:
>> On 20/04/2020 15:47, Marek Vasut wrote:
>>> On 4/20/20 3:46 PM, Neil Armstrong wrote:
>>>> Power on/off the PHYs to enable power to the USB ports, fixing USB support
>>>> on Khadas VIM3/VIM3L boards.
>>>>
>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>>> ---
>>>> drivers/usb/dwc3/dwc3-meson-g12a.c | 16 ++++++++++++++++
>>>> 1 file changed, 16 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
>>>> index d4453f8784..8f4a2f3f82 100644
>>>> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
>>>> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
>>>> @@ -408,6 +408,15 @@ static int dwc3_meson_g12a_probe(struct udevice *dev)
>>>> goto err_phy_init;
>>>> }
>>>>
>>>> + for (i = 0 ; i < PHY_COUNT ; ++i) {
>>>
>>> Doesn't checkpatch complain about this extra space before semicolon ?
>>
>> Nop, even in --strict
>
> Shouldn't it though ? I thought this extra space was forbidden.
No idea, this line is copied from the for loop doing the phy init,
itself copied from the Linux code passing all checkpatch checks.
>
>>>
>>>> + if (!priv->phys[i].dev)
>>>> + continue;
>>>> +
>>>> + ret = generic_phy_power_on(&priv->phys[i]);
>>>
>>> Do we really need to turn on all the PHYs ?
>>>
>>
>> Yes
>
> Then should we have something like clk_bulk_*(), but for phys ?
>
Eventually, yes, but this goes beyond a fix.
Neil
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
@ 2020-04-20 13:56 ` Neil Armstrong
0 siblings, 0 replies; 26+ messages in thread
From: Neil Armstrong @ 2020-04-20 13:56 UTC (permalink / raw)
To: Marek Vasut, u-boot-amlogic; +Cc: u-boot
On 20/04/2020 15:52, Marek Vasut wrote:
> On 4/20/20 3:49 PM, Neil Armstrong wrote:
>> On 20/04/2020 15:47, Marek Vasut wrote:
>>> On 4/20/20 3:46 PM, Neil Armstrong wrote:
>>>> Power on/off the PHYs to enable power to the USB ports, fixing USB support
>>>> on Khadas VIM3/VIM3L boards.
>>>>
>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>>> ---
>>>> drivers/usb/dwc3/dwc3-meson-g12a.c | 16 ++++++++++++++++
>>>> 1 file changed, 16 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
>>>> index d4453f8784..8f4a2f3f82 100644
>>>> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
>>>> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
>>>> @@ -408,6 +408,15 @@ static int dwc3_meson_g12a_probe(struct udevice *dev)
>>>> goto err_phy_init;
>>>> }
>>>>
>>>> + for (i = 0 ; i < PHY_COUNT ; ++i) {
>>>
>>> Doesn't checkpatch complain about this extra space before semicolon ?
>>
>> Nop, even in --strict
>
> Shouldn't it though ? I thought this extra space was forbidden.
No idea, this line is copied from the for loop doing the phy init,
itself copied from the Linux code passing all checkpatch checks.
>
>>>
>>>> + if (!priv->phys[i].dev)
>>>> + continue;
>>>> +
>>>> + ret = generic_phy_power_on(&priv->phys[i]);
>>>
>>> Do we really need to turn on all the PHYs ?
>>>
>>
>> Yes
>
> Then should we have something like clk_bulk_*(), but for phys ?
>
Eventually, yes, but this goes beyond a fix.
Neil
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
2020-04-20 13:56 ` Neil Armstrong
@ 2020-04-20 14:03 ` Marek Vasut
-1 siblings, 0 replies; 26+ messages in thread
From: Marek Vasut @ 2020-04-20 14:03 UTC (permalink / raw)
To: u-boot
On 4/20/20 3:56 PM, Neil Armstrong wrote:
> On 20/04/2020 15:52, Marek Vasut wrote:
>> On 4/20/20 3:49 PM, Neil Armstrong wrote:
>>> On 20/04/2020 15:47, Marek Vasut wrote:
>>>> On 4/20/20 3:46 PM, Neil Armstrong wrote:
>>>>> Power on/off the PHYs to enable power to the USB ports, fixing USB support
>>>>> on Khadas VIM3/VIM3L boards.
>>>>>
>>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>>>> ---
>>>>> drivers/usb/dwc3/dwc3-meson-g12a.c | 16 ++++++++++++++++
>>>>> 1 file changed, 16 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
>>>>> index d4453f8784..8f4a2f3f82 100644
>>>>> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
>>>>> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
>>>>> @@ -408,6 +408,15 @@ static int dwc3_meson_g12a_probe(struct udevice *dev)
>>>>> goto err_phy_init;
>>>>> }
>>>>>
>>>>> + for (i = 0 ; i < PHY_COUNT ; ++i) {
>>>>
>>>> Doesn't checkpatch complain about this extra space before semicolon ?
>>>
>>> Nop, even in --strict
>>
>> Shouldn't it though ? I thought this extra space was forbidden.
>
> No idea, this line is copied from the for loop doing the phy init,
> itself copied from the Linux code passing all checkpatch checks.
Could you change it to for (i = 0; i < PHY_COUNT; i++) please ?
>>>>
>>>>> + if (!priv->phys[i].dev)
>>>>> + continue;
>>>>> +
>>>>> + ret = generic_phy_power_on(&priv->phys[i]);
>>>>
>>>> Do we really need to turn on all the PHYs ?
>>>>
>>>
>>> Yes
>>
>> Then should we have something like clk_bulk_*(), but for phys ?
>>
>
> Eventually, yes, but this goes beyond a fix.
The merge window is open for quite a bit longer, so doing this in a
generic way would be appreciated.
btw while you're at it, shouldn't the PHY_COUNT be replaced with some
PHY count detection by reading the DT ? ( I don't have the hardware and
I don't know the amlogic SoCs though )
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
@ 2020-04-20 14:03 ` Marek Vasut
0 siblings, 0 replies; 26+ messages in thread
From: Marek Vasut @ 2020-04-20 14:03 UTC (permalink / raw)
To: Neil Armstrong, u-boot-amlogic; +Cc: u-boot
On 4/20/20 3:56 PM, Neil Armstrong wrote:
> On 20/04/2020 15:52, Marek Vasut wrote:
>> On 4/20/20 3:49 PM, Neil Armstrong wrote:
>>> On 20/04/2020 15:47, Marek Vasut wrote:
>>>> On 4/20/20 3:46 PM, Neil Armstrong wrote:
>>>>> Power on/off the PHYs to enable power to the USB ports, fixing USB support
>>>>> on Khadas VIM3/VIM3L boards.
>>>>>
>>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>>>> ---
>>>>> drivers/usb/dwc3/dwc3-meson-g12a.c | 16 ++++++++++++++++
>>>>> 1 file changed, 16 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
>>>>> index d4453f8784..8f4a2f3f82 100644
>>>>> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
>>>>> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
>>>>> @@ -408,6 +408,15 @@ static int dwc3_meson_g12a_probe(struct udevice *dev)
>>>>> goto err_phy_init;
>>>>> }
>>>>>
>>>>> + for (i = 0 ; i < PHY_COUNT ; ++i) {
>>>>
>>>> Doesn't checkpatch complain about this extra space before semicolon ?
>>>
>>> Nop, even in --strict
>>
>> Shouldn't it though ? I thought this extra space was forbidden.
>
> No idea, this line is copied from the for loop doing the phy init,
> itself copied from the Linux code passing all checkpatch checks.
Could you change it to for (i = 0; i < PHY_COUNT; i++) please ?
>>>>
>>>>> + if (!priv->phys[i].dev)
>>>>> + continue;
>>>>> +
>>>>> + ret = generic_phy_power_on(&priv->phys[i]);
>>>>
>>>> Do we really need to turn on all the PHYs ?
>>>>
>>>
>>> Yes
>>
>> Then should we have something like clk_bulk_*(), but for phys ?
>>
>
> Eventually, yes, but this goes beyond a fix.
The merge window is open for quite a bit longer, so doing this in a
generic way would be appreciated.
btw while you're at it, shouldn't the PHY_COUNT be replaced with some
PHY count detection by reading the DT ? ( I don't have the hardware and
I don't know the amlogic SoCs though )
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
2020-04-20 14:03 ` Marek Vasut
@ 2020-04-20 14:07 ` Neil Armstrong
-1 siblings, 0 replies; 26+ messages in thread
From: Neil Armstrong @ 2020-04-20 14:07 UTC (permalink / raw)
To: u-boot
On 20/04/2020 16:03, Marek Vasut wrote:
> On 4/20/20 3:56 PM, Neil Armstrong wrote:
>> On 20/04/2020 15:52, Marek Vasut wrote:
>>> On 4/20/20 3:49 PM, Neil Armstrong wrote:
>>>> On 20/04/2020 15:47, Marek Vasut wrote:
>>>>> On 4/20/20 3:46 PM, Neil Armstrong wrote:
>>>>>> Power on/off the PHYs to enable power to the USB ports, fixing USB support
>>>>>> on Khadas VIM3/VIM3L boards.
>>>>>>
>>>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>>>>> ---
>>>>>> drivers/usb/dwc3/dwc3-meson-g12a.c | 16 ++++++++++++++++
>>>>>> 1 file changed, 16 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
>>>>>> index d4453f8784..8f4a2f3f82 100644
>>>>>> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
>>>>>> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
>>>>>> @@ -408,6 +408,15 @@ static int dwc3_meson_g12a_probe(struct udevice *dev)
>>>>>> goto err_phy_init;
>>>>>> }
>>>>>>
>>>>>> + for (i = 0 ; i < PHY_COUNT ; ++i) {
>>>>>
>>>>> Doesn't checkpatch complain about this extra space before semicolon ?
>>>>
>>>> Nop, even in --strict
>>>
>>> Shouldn't it though ? I thought this extra space was forbidden.
>>
>> No idea, this line is copied from the for loop doing the phy init,
>> itself copied from the Linux code passing all checkpatch checks.
>
> Could you change it to for (i = 0; i < PHY_COUNT; i++) please ?
Sure
>
>>>>>
>>>>>> + if (!priv->phys[i].dev)
>>>>>> + continue;
>>>>>> +
>>>>>> + ret = generic_phy_power_on(&priv->phys[i]);
>>>>>
>>>>> Do we really need to turn on all the PHYs ?
>>>>>
>>>>
>>>> Yes
>>>
>>> Then should we have something like clk_bulk_*(), but for phys ?
>>>
>>
>> Eventually, yes, but this goes beyond a fix.
>
> The merge window is open for quite a bit longer, so doing this in a
> generic way would be appreciated.
>
> btw while you're at it, shouldn't the PHY_COUNT be replaced with some
> PHY count detection by reading the DT ? ( I don't have the hardware and
> I don't know the amlogic SoCs though )
>
Hmm, the PHYs list can have some holes (i.e only the second PHY could be used on 3),
and the name of the PHY determines it's type and position (PHY1 is a special OTG phy)
this is why we loop over a fixed array and check if the PHY pointer exists.
This won't match a phy_bulk_code directly, I would still need to map the bulk PHYs to this
array for the OTG and USB3 management.
Neil
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
@ 2020-04-20 14:07 ` Neil Armstrong
0 siblings, 0 replies; 26+ messages in thread
From: Neil Armstrong @ 2020-04-20 14:07 UTC (permalink / raw)
To: Marek Vasut, u-boot-amlogic; +Cc: u-boot
On 20/04/2020 16:03, Marek Vasut wrote:
> On 4/20/20 3:56 PM, Neil Armstrong wrote:
>> On 20/04/2020 15:52, Marek Vasut wrote:
>>> On 4/20/20 3:49 PM, Neil Armstrong wrote:
>>>> On 20/04/2020 15:47, Marek Vasut wrote:
>>>>> On 4/20/20 3:46 PM, Neil Armstrong wrote:
>>>>>> Power on/off the PHYs to enable power to the USB ports, fixing USB support
>>>>>> on Khadas VIM3/VIM3L boards.
>>>>>>
>>>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>>>>> ---
>>>>>> drivers/usb/dwc3/dwc3-meson-g12a.c | 16 ++++++++++++++++
>>>>>> 1 file changed, 16 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
>>>>>> index d4453f8784..8f4a2f3f82 100644
>>>>>> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
>>>>>> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
>>>>>> @@ -408,6 +408,15 @@ static int dwc3_meson_g12a_probe(struct udevice *dev)
>>>>>> goto err_phy_init;
>>>>>> }
>>>>>>
>>>>>> + for (i = 0 ; i < PHY_COUNT ; ++i) {
>>>>>
>>>>> Doesn't checkpatch complain about this extra space before semicolon ?
>>>>
>>>> Nop, even in --strict
>>>
>>> Shouldn't it though ? I thought this extra space was forbidden.
>>
>> No idea, this line is copied from the for loop doing the phy init,
>> itself copied from the Linux code passing all checkpatch checks.
>
> Could you change it to for (i = 0; i < PHY_COUNT; i++) please ?
Sure
>
>>>>>
>>>>>> + if (!priv->phys[i].dev)
>>>>>> + continue;
>>>>>> +
>>>>>> + ret = generic_phy_power_on(&priv->phys[i]);
>>>>>
>>>>> Do we really need to turn on all the PHYs ?
>>>>>
>>>>
>>>> Yes
>>>
>>> Then should we have something like clk_bulk_*(), but for phys ?
>>>
>>
>> Eventually, yes, but this goes beyond a fix.
>
> The merge window is open for quite a bit longer, so doing this in a
> generic way would be appreciated.
>
> btw while you're at it, shouldn't the PHY_COUNT be replaced with some
> PHY count detection by reading the DT ? ( I don't have the hardware and
> I don't know the amlogic SoCs though )
>
Hmm, the PHYs list can have some holes (i.e only the second PHY could be used on 3),
and the name of the PHY determines it's type and position (PHY1 is a special OTG phy)
this is why we loop over a fixed array and check if the PHY pointer exists.
This won't match a phy_bulk_code directly, I would still need to map the bulk PHYs to this
array for the OTG and USB3 management.
Neil
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
2020-04-20 14:07 ` Neil Armstrong
@ 2020-04-20 14:10 ` Marek Vasut
-1 siblings, 0 replies; 26+ messages in thread
From: Marek Vasut @ 2020-04-20 14:10 UTC (permalink / raw)
To: u-boot
On 4/20/20 4:07 PM, Neil Armstrong wrote:
[...]
>>>>>> Doesn't checkpatch complain about this extra space before semicolon ?
>>>>>
>>>>> Nop, even in --strict
>>>>
>>>> Shouldn't it though ? I thought this extra space was forbidden.
>>>
>>> No idea, this line is copied from the for loop doing the phy init,
>>> itself copied from the Linux code passing all checkpatch checks.
>>
>> Could you change it to for (i = 0; i < PHY_COUNT; i++) please ?
>
> Sure
Thank you.
( I'm still surprised checkpatch doesn't warn about it, I was quite sure
it was one of the checks at some point, hmmmmm. )
>>>>>>
>>>>>>> + if (!priv->phys[i].dev)
>>>>>>> + continue;
>>>>>>> +
>>>>>>> + ret = generic_phy_power_on(&priv->phys[i]);
>>>>>>
>>>>>> Do we really need to turn on all the PHYs ?
>>>>>>
>>>>>
>>>>> Yes
>>>>
>>>> Then should we have something like clk_bulk_*(), but for phys ?
>>>>
>>>
>>> Eventually, yes, but this goes beyond a fix.
>>
>> The merge window is open for quite a bit longer, so doing this in a
>> generic way would be appreciated.
>>
>> btw while you're at it, shouldn't the PHY_COUNT be replaced with some
>> PHY count detection by reading the DT ? ( I don't have the hardware and
>> I don't know the amlogic SoCs though )
>>
>
> Hmm, the PHYs list can have some holes (i.e only the second PHY could be used on 3),
> and the name of the PHY determines it's type and position (PHY1 is a special OTG phy)
> this is why we loop over a fixed array and check if the PHY pointer exists.
Um, that sounds like a vaguely familiar problem. Is that an issue with
mapping between controller and a USB PHY ? Is that mapping fixed or is
it configured from DT ? And then, should all PHYs really be powered on,
or should only the PHY matching the currently used controller be powered
on ?
> This won't match a phy_bulk_code directly, I would still need to map the bulk PHYs to this
> array for the OTG and USB3 management.
OK
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
@ 2020-04-20 14:10 ` Marek Vasut
0 siblings, 0 replies; 26+ messages in thread
From: Marek Vasut @ 2020-04-20 14:10 UTC (permalink / raw)
To: Neil Armstrong, u-boot-amlogic; +Cc: u-boot
On 4/20/20 4:07 PM, Neil Armstrong wrote:
[...]
>>>>>> Doesn't checkpatch complain about this extra space before semicolon ?
>>>>>
>>>>> Nop, even in --strict
>>>>
>>>> Shouldn't it though ? I thought this extra space was forbidden.
>>>
>>> No idea, this line is copied from the for loop doing the phy init,
>>> itself copied from the Linux code passing all checkpatch checks.
>>
>> Could you change it to for (i = 0; i < PHY_COUNT; i++) please ?
>
> Sure
Thank you.
( I'm still surprised checkpatch doesn't warn about it, I was quite sure
it was one of the checks at some point, hmmmmm. )
>>>>>>
>>>>>>> + if (!priv->phys[i].dev)
>>>>>>> + continue;
>>>>>>> +
>>>>>>> + ret = generic_phy_power_on(&priv->phys[i]);
>>>>>>
>>>>>> Do we really need to turn on all the PHYs ?
>>>>>>
>>>>>
>>>>> Yes
>>>>
>>>> Then should we have something like clk_bulk_*(), but for phys ?
>>>>
>>>
>>> Eventually, yes, but this goes beyond a fix.
>>
>> The merge window is open for quite a bit longer, so doing this in a
>> generic way would be appreciated.
>>
>> btw while you're at it, shouldn't the PHY_COUNT be replaced with some
>> PHY count detection by reading the DT ? ( I don't have the hardware and
>> I don't know the amlogic SoCs though )
>>
>
> Hmm, the PHYs list can have some holes (i.e only the second PHY could be used on 3),
> and the name of the PHY determines it's type and position (PHY1 is a special OTG phy)
> this is why we loop over a fixed array and check if the PHY pointer exists.
Um, that sounds like a vaguely familiar problem. Is that an issue with
mapping between controller and a USB PHY ? Is that mapping fixed or is
it configured from DT ? And then, should all PHYs really be powered on,
or should only the PHY matching the currently used controller be powered
on ?
> This won't match a phy_bulk_code directly, I would still need to map the bulk PHYs to this
> array for the OTG and USB3 management.
OK
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
2020-04-20 14:10 ` Marek Vasut
@ 2020-04-20 14:47 ` Neil Armstrong
-1 siblings, 0 replies; 26+ messages in thread
From: Neil Armstrong @ 2020-04-20 14:47 UTC (permalink / raw)
To: u-boot
On 20/04/2020 16:10, Marek Vasut wrote:
> On 4/20/20 4:07 PM, Neil Armstrong wrote:
> [...]
>
>>>>>>> Doesn't checkpatch complain about this extra space before semicolon ?
>>>>>>
>>>>>> Nop, even in --strict
>>>>>
>>>>> Shouldn't it though ? I thought this extra space was forbidden.
>>>>
>>>> No idea, this line is copied from the for loop doing the phy init,
>>>> itself copied from the Linux code passing all checkpatch checks.
>>>
>>> Could you change it to for (i = 0; i < PHY_COUNT; i++) please ?
>>
>> Sure
>
> Thank you.
>
> ( I'm still surprised checkpatch doesn't warn about it, I was quite sure
> it was one of the checks at some point, hmmmmm. )
>
>>>>>>>
>>>>>>>> + if (!priv->phys[i].dev)
>>>>>>>> + continue;
>>>>>>>> +
>>>>>>>> + ret = generic_phy_power_on(&priv->phys[i]);
>>>>>>>
>>>>>>> Do we really need to turn on all the PHYs ?
>>>>>>>
>>>>>>
>>>>>> Yes
>>>>>
>>>>> Then should we have something like clk_bulk_*(), but for phys ?
>>>>>
>>>>
>>>> Eventually, yes, but this goes beyond a fix.
>>>
>>> The merge window is open for quite a bit longer, so doing this in a
>>> generic way would be appreciated.
>>>
>>> btw while you're at it, shouldn't the PHY_COUNT be replaced with some
>>> PHY count detection by reading the DT ? ( I don't have the hardware and
>>> I don't know the amlogic SoCs though )
>>>
>>
>> Hmm, the PHYs list can have some holes (i.e only the second PHY could be used on 3),
>> and the name of the PHY determines it's type and position (PHY1 is a special OTG phy)
>> this is why we loop over a fixed array and check if the PHY pointer exists.
>
> Um, that sounds like a vaguely familiar problem. Is that an issue with
> mapping between controller and a USB PHY ? Is that mapping fixed or is
> it configured from DT ?
Mapping is done via DT, all PHYs are optional depending on the board layout.
> And then, should all PHYs really be powered on,
> or should only the PHY matching the currently used controller be powered
> on ?
All PHYs are connected to a glue that muxes 1 PHY between DWC2 and DWC3, and
the other USB2 PHYs (for now 1, but previous SoCs had 2) to DWC3, the USB3 PHY
is muxed between PCIe to dw-pcie and DWC3.
So we power-on only the PHYs specified in DT, thus the:
if (!priv->phys[i].dev)
continue;
I attached the USB complex diagram if you need it.
Neil
>
>> This won't match a phy_bulk_code directly, I would still need to map the bulk PHYs to this
>> array for the OTG and USB3 management.
>
> OK
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: G12A USB.svg
Type: image/svg+xml
Size: 51866 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200420/80913a8d/attachment-0001.svg>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
@ 2020-04-20 14:47 ` Neil Armstrong
0 siblings, 0 replies; 26+ messages in thread
From: Neil Armstrong @ 2020-04-20 14:47 UTC (permalink / raw)
To: Marek Vasut, u-boot-amlogic; +Cc: u-boot
[-- Attachment #1: Type: text/plain, Size: 2486 bytes --]
On 20/04/2020 16:10, Marek Vasut wrote:
> On 4/20/20 4:07 PM, Neil Armstrong wrote:
> [...]
>
>>>>>>> Doesn't checkpatch complain about this extra space before semicolon ?
>>>>>>
>>>>>> Nop, even in --strict
>>>>>
>>>>> Shouldn't it though ? I thought this extra space was forbidden.
>>>>
>>>> No idea, this line is copied from the for loop doing the phy init,
>>>> itself copied from the Linux code passing all checkpatch checks.
>>>
>>> Could you change it to for (i = 0; i < PHY_COUNT; i++) please ?
>>
>> Sure
>
> Thank you.
>
> ( I'm still surprised checkpatch doesn't warn about it, I was quite sure
> it was one of the checks at some point, hmmmmm. )
>
>>>>>>>
>>>>>>>> + if (!priv->phys[i].dev)
>>>>>>>> + continue;
>>>>>>>> +
>>>>>>>> + ret = generic_phy_power_on(&priv->phys[i]);
>>>>>>>
>>>>>>> Do we really need to turn on all the PHYs ?
>>>>>>>
>>>>>>
>>>>>> Yes
>>>>>
>>>>> Then should we have something like clk_bulk_*(), but for phys ?
>>>>>
>>>>
>>>> Eventually, yes, but this goes beyond a fix.
>>>
>>> The merge window is open for quite a bit longer, so doing this in a
>>> generic way would be appreciated.
>>>
>>> btw while you're at it, shouldn't the PHY_COUNT be replaced with some
>>> PHY count detection by reading the DT ? ( I don't have the hardware and
>>> I don't know the amlogic SoCs though )
>>>
>>
>> Hmm, the PHYs list can have some holes (i.e only the second PHY could be used on 3),
>> and the name of the PHY determines it's type and position (PHY1 is a special OTG phy)
>> this is why we loop over a fixed array and check if the PHY pointer exists.
>
> Um, that sounds like a vaguely familiar problem. Is that an issue with
> mapping between controller and a USB PHY ? Is that mapping fixed or is
> it configured from DT ?
Mapping is done via DT, all PHYs are optional depending on the board layout.
> And then, should all PHYs really be powered on,
> or should only the PHY matching the currently used controller be powered
> on ?
All PHYs are connected to a glue that muxes 1 PHY between DWC2 and DWC3, and
the other USB2 PHYs (for now 1, but previous SoCs had 2) to DWC3, the USB3 PHY
is muxed between PCIe to dw-pcie and DWC3.
So we power-on only the PHYs specified in DT, thus the:
if (!priv->phys[i].dev)
continue;
I attached the USB complex diagram if you need it.
Neil
>
>> This won't match a phy_bulk_code directly, I would still need to map the bulk PHYs to this
>> array for the OTG and USB3 management.
>
> OK
>
[-- Attachment #2: G12A USB.svg --]
[-- Type: image/svg+xml, Size: 51866 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
2020-04-20 14:47 ` Neil Armstrong
@ 2020-04-20 15:40 ` Marek Vasut
-1 siblings, 0 replies; 26+ messages in thread
From: Marek Vasut @ 2020-04-20 15:40 UTC (permalink / raw)
To: u-boot
On 4/20/20 4:47 PM, Neil Armstrong wrote:
> On 20/04/2020 16:10, Marek Vasut wrote:
>> On 4/20/20 4:07 PM, Neil Armstrong wrote:
>> [...]
>>
>>>>>>>> Doesn't checkpatch complain about this extra space before semicolon ?
>>>>>>>
>>>>>>> Nop, even in --strict
>>>>>>
>>>>>> Shouldn't it though ? I thought this extra space was forbidden.
>>>>>
>>>>> No idea, this line is copied from the for loop doing the phy init,
>>>>> itself copied from the Linux code passing all checkpatch checks.
>>>>
>>>> Could you change it to for (i = 0; i < PHY_COUNT; i++) please ?
>>>
>>> Sure
>>
>> Thank you.
>>
>> ( I'm still surprised checkpatch doesn't warn about it, I was quite sure
>> it was one of the checks at some point, hmmmmm. )
>>
>>>>>>>>
>>>>>>>>> + if (!priv->phys[i].dev)
>>>>>>>>> + continue;
>>>>>>>>> +
>>>>>>>>> + ret = generic_phy_power_on(&priv->phys[i]);
>>>>>>>>
>>>>>>>> Do we really need to turn on all the PHYs ?
>>>>>>>>
>>>>>>>
>>>>>>> Yes
>>>>>>
>>>>>> Then should we have something like clk_bulk_*(), but for phys ?
>>>>>>
>>>>>
>>>>> Eventually, yes, but this goes beyond a fix.
>>>>
>>>> The merge window is open for quite a bit longer, so doing this in a
>>>> generic way would be appreciated.
>>>>
>>>> btw while you're at it, shouldn't the PHY_COUNT be replaced with some
>>>> PHY count detection by reading the DT ? ( I don't have the hardware and
>>>> I don't know the amlogic SoCs though )
>>>>
>>>
>>> Hmm, the PHYs list can have some holes (i.e only the second PHY could be used on 3),
>>> and the name of the PHY determines it's type and position (PHY1 is a special OTG phy)
>>> this is why we loop over a fixed array and check if the PHY pointer exists.
>>
>> Um, that sounds like a vaguely familiar problem. Is that an issue with
>> mapping between controller and a USB PHY ? Is that mapping fixed or is
>> it configured from DT ?
>
> Mapping is done via DT, all PHYs are optional depending on the board layout.
>
>> And then, should all PHYs really be powered on,
>> or should only the PHY matching the currently used controller be powered
>> on ?
>
> All PHYs are connected to a glue that muxes 1 PHY between DWC2 and DWC3, and
> the other USB2 PHYs (for now 1, but previous SoCs had 2) to DWC3, the USB3 PHY
> is muxed between PCIe to dw-pcie and DWC3.
>
> So we power-on only the PHYs specified in DT, thus the:
> if (!priv->phys[i].dev)
> continue;
>
> I attached the USB complex diagram if you need it.
OK, thanks for clarifying. Now, can you document this in the commit
message, so we have this information stored somewhere for future
reference ? This would be real useful.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
@ 2020-04-20 15:40 ` Marek Vasut
0 siblings, 0 replies; 26+ messages in thread
From: Marek Vasut @ 2020-04-20 15:40 UTC (permalink / raw)
To: Neil Armstrong, u-boot-amlogic; +Cc: u-boot
On 4/20/20 4:47 PM, Neil Armstrong wrote:
> On 20/04/2020 16:10, Marek Vasut wrote:
>> On 4/20/20 4:07 PM, Neil Armstrong wrote:
>> [...]
>>
>>>>>>>> Doesn't checkpatch complain about this extra space before semicolon ?
>>>>>>>
>>>>>>> Nop, even in --strict
>>>>>>
>>>>>> Shouldn't it though ? I thought this extra space was forbidden.
>>>>>
>>>>> No idea, this line is copied from the for loop doing the phy init,
>>>>> itself copied from the Linux code passing all checkpatch checks.
>>>>
>>>> Could you change it to for (i = 0; i < PHY_COUNT; i++) please ?
>>>
>>> Sure
>>
>> Thank you.
>>
>> ( I'm still surprised checkpatch doesn't warn about it, I was quite sure
>> it was one of the checks at some point, hmmmmm. )
>>
>>>>>>>>
>>>>>>>>> + if (!priv->phys[i].dev)
>>>>>>>>> + continue;
>>>>>>>>> +
>>>>>>>>> + ret = generic_phy_power_on(&priv->phys[i]);
>>>>>>>>
>>>>>>>> Do we really need to turn on all the PHYs ?
>>>>>>>>
>>>>>>>
>>>>>>> Yes
>>>>>>
>>>>>> Then should we have something like clk_bulk_*(), but for phys ?
>>>>>>
>>>>>
>>>>> Eventually, yes, but this goes beyond a fix.
>>>>
>>>> The merge window is open for quite a bit longer, so doing this in a
>>>> generic way would be appreciated.
>>>>
>>>> btw while you're at it, shouldn't the PHY_COUNT be replaced with some
>>>> PHY count detection by reading the DT ? ( I don't have the hardware and
>>>> I don't know the amlogic SoCs though )
>>>>
>>>
>>> Hmm, the PHYs list can have some holes (i.e only the second PHY could be used on 3),
>>> and the name of the PHY determines it's type and position (PHY1 is a special OTG phy)
>>> this is why we loop over a fixed array and check if the PHY pointer exists.
>>
>> Um, that sounds like a vaguely familiar problem. Is that an issue with
>> mapping between controller and a USB PHY ? Is that mapping fixed or is
>> it configured from DT ?
>
> Mapping is done via DT, all PHYs are optional depending on the board layout.
>
>> And then, should all PHYs really be powered on,
>> or should only the PHY matching the currently used controller be powered
>> on ?
>
> All PHYs are connected to a glue that muxes 1 PHY between DWC2 and DWC3, and
> the other USB2 PHYs (for now 1, but previous SoCs had 2) to DWC3, the USB3 PHY
> is muxed between PCIe to dw-pcie and DWC3.
>
> So we power-on only the PHYs specified in DT, thus the:
> if (!priv->phys[i].dev)
> continue;
>
> I attached the USB complex diagram if you need it.
OK, thanks for clarifying. Now, can you document this in the commit
message, so we have this information stored somewhere for future
reference ? This would be real useful.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
2020-04-20 15:40 ` Marek Vasut
@ 2020-04-21 8:09 ` Neil Armstrong
-1 siblings, 0 replies; 26+ messages in thread
From: Neil Armstrong @ 2020-04-21 8:09 UTC (permalink / raw)
To: u-boot
On 20/04/2020 17:40, Marek Vasut wrote:
> On 4/20/20 4:47 PM, Neil Armstrong wrote:
>> On 20/04/2020 16:10, Marek Vasut wrote:
>>> On 4/20/20 4:07 PM, Neil Armstrong wrote:
>>> [...]
>>>
>>>>>>>>> Doesn't checkpatch complain about this extra space before semicolon ?
>>>>>>>>
>>>>>>>> Nop, even in --strict
>>>>>>>
>>>>>>> Shouldn't it though ? I thought this extra space was forbidden.
>>>>>>
>>>>>> No idea, this line is copied from the for loop doing the phy init,
>>>>>> itself copied from the Linux code passing all checkpatch checks.
>>>>>
>>>>> Could you change it to for (i = 0; i < PHY_COUNT; i++) please ?
>>>>
>>>> Sure
>>>
>>> Thank you.
>>>
>>> ( I'm still surprised checkpatch doesn't warn about it, I was quite sure
>>> it was one of the checks at some point, hmmmmm. )
>>>
>>>>>>>>>
>>>>>>>>>> + if (!priv->phys[i].dev)
>>>>>>>>>> + continue;
>>>>>>>>>> +
>>>>>>>>>> + ret = generic_phy_power_on(&priv->phys[i]);
>>>>>>>>>
>>>>>>>>> Do we really need to turn on all the PHYs ?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Yes
>>>>>>>
>>>>>>> Then should we have something like clk_bulk_*(), but for phys ?
>>>>>>>
>>>>>>
>>>>>> Eventually, yes, but this goes beyond a fix.
>>>>>
>>>>> The merge window is open for quite a bit longer, so doing this in a
>>>>> generic way would be appreciated.
>>>>>
>>>>> btw while you're at it, shouldn't the PHY_COUNT be replaced with some
>>>>> PHY count detection by reading the DT ? ( I don't have the hardware and
>>>>> I don't know the amlogic SoCs though )
>>>>>
>>>>
>>>> Hmm, the PHYs list can have some holes (i.e only the second PHY could be used on 3),
>>>> and the name of the PHY determines it's type and position (PHY1 is a special OTG phy)
>>>> this is why we loop over a fixed array and check if the PHY pointer exists.
>>>
>>> Um, that sounds like a vaguely familiar problem. Is that an issue with
>>> mapping between controller and a USB PHY ? Is that mapping fixed or is
>>> it configured from DT ?
>>
>> Mapping is done via DT, all PHYs are optional depending on the board layout.
>>
>>> And then, should all PHYs really be powered on,
>>> or should only the PHY matching the currently used controller be powered
>>> on ?
>>
>> All PHYs are connected to a glue that muxes 1 PHY between DWC2 and DWC3, and
>> the other USB2 PHYs (for now 1, but previous SoCs had 2) to DWC3, the USB3 PHY
>> is muxed between PCIe to dw-pcie and DWC3.
>>
>> So we power-on only the PHYs specified in DT, thus the:
>> if (!priv->phys[i].dev)
>> continue;
>>
>> I attached the USB complex diagram if you need it.
>
> OK, thanks for clarifying. Now, can you document this in the commit
> message, so we have this information stored somewhere for future
> reference ? This would be real useful.
>
Yes sure.
Neil
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
@ 2020-04-21 8:09 ` Neil Armstrong
0 siblings, 0 replies; 26+ messages in thread
From: Neil Armstrong @ 2020-04-21 8:09 UTC (permalink / raw)
To: Marek Vasut, u-boot-amlogic; +Cc: u-boot
On 20/04/2020 17:40, Marek Vasut wrote:
> On 4/20/20 4:47 PM, Neil Armstrong wrote:
>> On 20/04/2020 16:10, Marek Vasut wrote:
>>> On 4/20/20 4:07 PM, Neil Armstrong wrote:
>>> [...]
>>>
>>>>>>>>> Doesn't checkpatch complain about this extra space before semicolon ?
>>>>>>>>
>>>>>>>> Nop, even in --strict
>>>>>>>
>>>>>>> Shouldn't it though ? I thought this extra space was forbidden.
>>>>>>
>>>>>> No idea, this line is copied from the for loop doing the phy init,
>>>>>> itself copied from the Linux code passing all checkpatch checks.
>>>>>
>>>>> Could you change it to for (i = 0; i < PHY_COUNT; i++) please ?
>>>>
>>>> Sure
>>>
>>> Thank you.
>>>
>>> ( I'm still surprised checkpatch doesn't warn about it, I was quite sure
>>> it was one of the checks at some point, hmmmmm. )
>>>
>>>>>>>>>
>>>>>>>>>> + if (!priv->phys[i].dev)
>>>>>>>>>> + continue;
>>>>>>>>>> +
>>>>>>>>>> + ret = generic_phy_power_on(&priv->phys[i]);
>>>>>>>>>
>>>>>>>>> Do we really need to turn on all the PHYs ?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Yes
>>>>>>>
>>>>>>> Then should we have something like clk_bulk_*(), but for phys ?
>>>>>>>
>>>>>>
>>>>>> Eventually, yes, but this goes beyond a fix.
>>>>>
>>>>> The merge window is open for quite a bit longer, so doing this in a
>>>>> generic way would be appreciated.
>>>>>
>>>>> btw while you're at it, shouldn't the PHY_COUNT be replaced with some
>>>>> PHY count detection by reading the DT ? ( I don't have the hardware and
>>>>> I don't know the amlogic SoCs though )
>>>>>
>>>>
>>>> Hmm, the PHYs list can have some holes (i.e only the second PHY could be used on 3),
>>>> and the name of the PHY determines it's type and position (PHY1 is a special OTG phy)
>>>> this is why we loop over a fixed array and check if the PHY pointer exists.
>>>
>>> Um, that sounds like a vaguely familiar problem. Is that an issue with
>>> mapping between controller and a USB PHY ? Is that mapping fixed or is
>>> it configured from DT ?
>>
>> Mapping is done via DT, all PHYs are optional depending on the board layout.
>>
>>> And then, should all PHYs really be powered on,
>>> or should only the PHY matching the currently used controller be powered
>>> on ?
>>
>> All PHYs are connected to a glue that muxes 1 PHY between DWC2 and DWC3, and
>> the other USB2 PHYs (for now 1, but previous SoCs had 2) to DWC3, the USB3 PHY
>> is muxed between PCIe to dw-pcie and DWC3.
>>
>> So we power-on only the PHYs specified in DT, thus the:
>> if (!priv->phys[i].dev)
>> continue;
>>
>> I attached the USB complex diagram if you need it.
>
> OK, thanks for clarifying. Now, can you document this in the commit
> message, so we have this information stored somewhere for future
> reference ? This would be real useful.
>
Yes sure.
Neil
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
2020-04-21 8:09 ` Neil Armstrong
@ 2020-04-21 8:10 ` Marek Vasut
-1 siblings, 0 replies; 26+ messages in thread
From: Marek Vasut @ 2020-04-21 8:10 UTC (permalink / raw)
To: u-boot
On 4/21/20 10:09 AM, Neil Armstrong wrote:
> On 20/04/2020 17:40, Marek Vasut wrote:
>> On 4/20/20 4:47 PM, Neil Armstrong wrote:
>>> On 20/04/2020 16:10, Marek Vasut wrote:
>>>> On 4/20/20 4:07 PM, Neil Armstrong wrote:
>>>> [...]
>>>>
>>>>>>>>>> Doesn't checkpatch complain about this extra space before semicolon ?
>>>>>>>>>
>>>>>>>>> Nop, even in --strict
>>>>>>>>
>>>>>>>> Shouldn't it though ? I thought this extra space was forbidden.
>>>>>>>
>>>>>>> No idea, this line is copied from the for loop doing the phy init,
>>>>>>> itself copied from the Linux code passing all checkpatch checks.
>>>>>>
>>>>>> Could you change it to for (i = 0; i < PHY_COUNT; i++) please ?
>>>>>
>>>>> Sure
>>>>
>>>> Thank you.
>>>>
>>>> ( I'm still surprised checkpatch doesn't warn about it, I was quite sure
>>>> it was one of the checks at some point, hmmmmm. )
>>>>
>>>>>>>>>>
>>>>>>>>>>> + if (!priv->phys[i].dev)
>>>>>>>>>>> + continue;
>>>>>>>>>>> +
>>>>>>>>>>> + ret = generic_phy_power_on(&priv->phys[i]);
>>>>>>>>>>
>>>>>>>>>> Do we really need to turn on all the PHYs ?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Yes
>>>>>>>>
>>>>>>>> Then should we have something like clk_bulk_*(), but for phys ?
>>>>>>>>
>>>>>>>
>>>>>>> Eventually, yes, but this goes beyond a fix.
>>>>>>
>>>>>> The merge window is open for quite a bit longer, so doing this in a
>>>>>> generic way would be appreciated.
>>>>>>
>>>>>> btw while you're at it, shouldn't the PHY_COUNT be replaced with some
>>>>>> PHY count detection by reading the DT ? ( I don't have the hardware and
>>>>>> I don't know the amlogic SoCs though )
>>>>>>
>>>>>
>>>>> Hmm, the PHYs list can have some holes (i.e only the second PHY could be used on 3),
>>>>> and the name of the PHY determines it's type and position (PHY1 is a special OTG phy)
>>>>> this is why we loop over a fixed array and check if the PHY pointer exists.
>>>>
>>>> Um, that sounds like a vaguely familiar problem. Is that an issue with
>>>> mapping between controller and a USB PHY ? Is that mapping fixed or is
>>>> it configured from DT ?
>>>
>>> Mapping is done via DT, all PHYs are optional depending on the board layout.
>>>
>>>> And then, should all PHYs really be powered on,
>>>> or should only the PHY matching the currently used controller be powered
>>>> on ?
>>>
>>> All PHYs are connected to a glue that muxes 1 PHY between DWC2 and DWC3, and
>>> the other USB2 PHYs (for now 1, but previous SoCs had 2) to DWC3, the USB3 PHY
>>> is muxed between PCIe to dw-pcie and DWC3.
>>>
>>> So we power-on only the PHYs specified in DT, thus the:
>>> if (!priv->phys[i].dev)
>>> continue;
>>>
>>> I attached the USB complex diagram if you need it.
>>
>> OK, thanks for clarifying. Now, can you document this in the commit
>> message, so we have this information stored somewhere for future
>> reference ? This would be real useful.
>>
>
> Yes sure.
Thanks
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
@ 2020-04-21 8:10 ` Marek Vasut
0 siblings, 0 replies; 26+ messages in thread
From: Marek Vasut @ 2020-04-21 8:10 UTC (permalink / raw)
To: Neil Armstrong, u-boot-amlogic; +Cc: u-boot
On 4/21/20 10:09 AM, Neil Armstrong wrote:
> On 20/04/2020 17:40, Marek Vasut wrote:
>> On 4/20/20 4:47 PM, Neil Armstrong wrote:
>>> On 20/04/2020 16:10, Marek Vasut wrote:
>>>> On 4/20/20 4:07 PM, Neil Armstrong wrote:
>>>> [...]
>>>>
>>>>>>>>>> Doesn't checkpatch complain about this extra space before semicolon ?
>>>>>>>>>
>>>>>>>>> Nop, even in --strict
>>>>>>>>
>>>>>>>> Shouldn't it though ? I thought this extra space was forbidden.
>>>>>>>
>>>>>>> No idea, this line is copied from the for loop doing the phy init,
>>>>>>> itself copied from the Linux code passing all checkpatch checks.
>>>>>>
>>>>>> Could you change it to for (i = 0; i < PHY_COUNT; i++) please ?
>>>>>
>>>>> Sure
>>>>
>>>> Thank you.
>>>>
>>>> ( I'm still surprised checkpatch doesn't warn about it, I was quite sure
>>>> it was one of the checks at some point, hmmmmm. )
>>>>
>>>>>>>>>>
>>>>>>>>>>> + if (!priv->phys[i].dev)
>>>>>>>>>>> + continue;
>>>>>>>>>>> +
>>>>>>>>>>> + ret = generic_phy_power_on(&priv->phys[i]);
>>>>>>>>>>
>>>>>>>>>> Do we really need to turn on all the PHYs ?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Yes
>>>>>>>>
>>>>>>>> Then should we have something like clk_bulk_*(), but for phys ?
>>>>>>>>
>>>>>>>
>>>>>>> Eventually, yes, but this goes beyond a fix.
>>>>>>
>>>>>> The merge window is open for quite a bit longer, so doing this in a
>>>>>> generic way would be appreciated.
>>>>>>
>>>>>> btw while you're at it, shouldn't the PHY_COUNT be replaced with some
>>>>>> PHY count detection by reading the DT ? ( I don't have the hardware and
>>>>>> I don't know the amlogic SoCs though )
>>>>>>
>>>>>
>>>>> Hmm, the PHYs list can have some holes (i.e only the second PHY could be used on 3),
>>>>> and the name of the PHY determines it's type and position (PHY1 is a special OTG phy)
>>>>> this is why we loop over a fixed array and check if the PHY pointer exists.
>>>>
>>>> Um, that sounds like a vaguely familiar problem. Is that an issue with
>>>> mapping between controller and a USB PHY ? Is that mapping fixed or is
>>>> it configured from DT ?
>>>
>>> Mapping is done via DT, all PHYs are optional depending on the board layout.
>>>
>>>> And then, should all PHYs really be powered on,
>>>> or should only the PHY matching the currently used controller be powered
>>>> on ?
>>>
>>> All PHYs are connected to a glue that muxes 1 PHY between DWC2 and DWC3, and
>>> the other USB2 PHYs (for now 1, but previous SoCs had 2) to DWC3, the USB3 PHY
>>> is muxed between PCIe to dw-pcie and DWC3.
>>>
>>> So we power-on only the PHYs specified in DT, thus the:
>>> if (!priv->phys[i].dev)
>>> continue;
>>>
>>> I attached the USB complex diagram if you need it.
>>
>> OK, thanks for clarifying. Now, can you document this in the commit
>> message, so we have this information stored somewhere for future
>> reference ? This would be real useful.
>>
>
> Yes sure.
Thanks
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
[not found] <16078B20C191ED17.15323@groups.io>
@ 2020-04-24 7:18 ` Neil Armstrong
0 siblings, 0 replies; 26+ messages in thread
From: Neil Armstrong @ 2020-04-24 7:18 UTC (permalink / raw)
To: u-boot
On 20/04/2020 15:46, Neil Armstrong via groups.io wrote:
> Power on/off the PHYs to enable power to the USB ports, fixing USB support
> on Khadas VIM3/VIM3L boards.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
> drivers/usb/dwc3/dwc3-meson-g12a.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
> index d4453f8784..8f4a2f3f82 100644
> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
> @@ -408,6 +408,15 @@ static int dwc3_meson_g12a_probe(struct udevice *dev)
> goto err_phy_init;
> }
>
> + for (i = 0 ; i < PHY_COUNT ; ++i) {
> + if (!priv->phys[i].dev)
> + continue;
> +
> + ret = generic_phy_power_on(&priv->phys[i]);
> + if (ret)
> + goto err_phy_init;
> + }
> +
> return 0;
>
> err_phy_init:
> @@ -430,6 +439,13 @@ static int dwc3_meson_g12a_remove(struct udevice *dev)
>
> clk_release_all(&priv->clk, 1);
>
> + for (i = 0 ; i < PHY_COUNT ; ++i) {
> + if (!priv->phys[i].dev)
> + continue;
> +
> + generic_phy_power_off(&priv->phys[i]);
> + }
> +
> for (i = 0 ; i < PHY_COUNT ; ++i) {
> if (!priv->phys[i].dev)
> continue;
>
Applied to u-boot-amlogic
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs
@ 2020-04-24 7:18 ` Neil Armstrong
0 siblings, 0 replies; 26+ messages in thread
From: Neil Armstrong @ 2020-04-24 7:18 UTC (permalink / raw)
To: u-boot-amlogic, marex; +Cc: u-boot
On 20/04/2020 15:46, Neil Armstrong via groups.io wrote:
> Power on/off the PHYs to enable power to the USB ports, fixing USB support
> on Khadas VIM3/VIM3L boards.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
> drivers/usb/dwc3/dwc3-meson-g12a.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
> index d4453f8784..8f4a2f3f82 100644
> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
> @@ -408,6 +408,15 @@ static int dwc3_meson_g12a_probe(struct udevice *dev)
> goto err_phy_init;
> }
>
> + for (i = 0 ; i < PHY_COUNT ; ++i) {
> + if (!priv->phys[i].dev)
> + continue;
> +
> + ret = generic_phy_power_on(&priv->phys[i]);
> + if (ret)
> + goto err_phy_init;
> + }
> +
> return 0;
>
> err_phy_init:
> @@ -430,6 +439,13 @@ static int dwc3_meson_g12a_remove(struct udevice *dev)
>
> clk_release_all(&priv->clk, 1);
>
> + for (i = 0 ; i < PHY_COUNT ; ++i) {
> + if (!priv->phys[i].dev)
> + continue;
> +
> + generic_phy_power_off(&priv->phys[i]);
> + }
> +
> for (i = 0 ; i < PHY_COUNT ; ++i) {
> if (!priv->phys[i].dev)
> continue;
>
Applied to u-boot-amlogic
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2020-04-24 7:18 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-20 13:46 [PATCH] usb: dwc3-meson-g12a: add power-on/off of the PHYs Neil Armstrong
2020-04-20 13:46 ` Neil Armstrong
2020-04-20 13:47 ` Marek Vasut
2020-04-20 13:47 ` Marek Vasut
2020-04-20 13:49 ` Neil Armstrong
2020-04-20 13:49 ` Neil Armstrong
2020-04-20 13:52 ` Marek Vasut
2020-04-20 13:52 ` Marek Vasut
2020-04-20 13:56 ` Neil Armstrong
2020-04-20 13:56 ` Neil Armstrong
2020-04-20 14:03 ` Marek Vasut
2020-04-20 14:03 ` Marek Vasut
2020-04-20 14:07 ` Neil Armstrong
2020-04-20 14:07 ` Neil Armstrong
2020-04-20 14:10 ` Marek Vasut
2020-04-20 14:10 ` Marek Vasut
2020-04-20 14:47 ` Neil Armstrong
2020-04-20 14:47 ` Neil Armstrong
2020-04-20 15:40 ` Marek Vasut
2020-04-20 15:40 ` Marek Vasut
2020-04-21 8:09 ` Neil Armstrong
2020-04-21 8:09 ` Neil Armstrong
2020-04-21 8:10 ` Marek Vasut
2020-04-21 8:10 ` Marek Vasut
[not found] <16078B20C191ED17.15323@groups.io>
2020-04-24 7:18 ` Neil Armstrong
2020-04-24 7:18 ` Neil Armstrong
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.