[PATCH v5 0/1] riscv: dts: starfive: jh7110-milkv-mars: enable usb0 host function

E Shattow posted 1 patch 1 year, 2 months ago
There is a newer version of this series
.../boot/dts/starfive/jh7110-milkv-mars.dts    | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
[PATCH v5 0/1] riscv: dts: starfive: jh7110-milkv-mars: enable usb0 host function
Posted by E Shattow 1 year, 2 months ago
Enable host mode JH7110 on-chip USB for Milk-V Mars by setting host mode
and connect vbus pinctrl.

This functionality depends on setting the USB over-current register to 
disable at bootloader phase, for example U-Boot:
https://patchwork.ozlabs.org/project/uboot/patch/20241012031328.4268-6-minda.chen@starfivetech.com/

If the over-current register is not prepared for us then the result is no 
change in functional outcome with this patch applied; there is an error 
visible to the user and this additional usb configuration fails (same as
it is now). On Milk-V Mars with four USB-A ports this applies to one of the 
ports and the remaining three VL805-connected ports via PCIe are not changed.

Changes since v4:
 - Rebase on latest master

Changes since v3:
 - Rebase on linux-next/master
 - use tabs for code indent

Changes since v2:
 - Rebase on 6.12

E Shattow (1):
  riscv: dts: starfive: jh7110-milkv-mars: enable usb0 host function

 .../boot/dts/starfive/jh7110-milkv-mars.dts    | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

-- 
2.45.2
Re: [PATCH v5 0/1] riscv: dts: starfive: jh7110-milkv-mars: enable usb0 host function
Posted by Emil Renner Berthing 1 year, 2 months ago
E Shattow wrote:
> Enable host mode JH7110 on-chip USB for Milk-V Mars by setting host mode
> and connect vbus pinctrl.
>
> This functionality depends on setting the USB over-current register to
> disable at bootloader phase, for example U-Boot:
> https://patchwork.ozlabs.org/project/uboot/patch/20241012031328.4268-6-minda.chen@starfivetech.com/

Hi E,

Ideally the JH7110 pinctrl driver would be updated, so Linux can do this itself
and doesn't need to rely on u-boot doing it. I already asked for this here:

https://lore.kernel.org/all/CAJM55Z-+Cxdebcn4MLXfQdOVhx4c2SQ+zMH8cjn-Yq35xO8g0A@mail.gmail.com/

>
> If the over-current register is not prepared for us then the result is no
> change in functional outcome with this patch applied; there is an error
> visible to the user and this additional usb configuration fails (same as
> it is now). On Milk-V Mars with four USB-A ports this applies to one of the
> ports and the remaining three VL805-connected ports via PCIe are not changed.

Thanks for the patches. I don't quite understand when you write "no change in
functional outcome with this patch applied". The USB-C port is already
configured as a peripheral, and I just tried setting up an ethernet gadget on
my VF2 running 6.12 and that works quite well. Does it not work on the Milk-V
Mars board? If it does then these patches would break that functionality.

Here is the script I used for that:
https://paste.c-net.org/BravoLonely

At the very least you'll need to explain in the commit message itself why
changing the USB-C port from peripheral mode to host mode is OK. But ideally
maybe you could make it work in OTG mode, so userspace can choose how they want
to use the port. The same goes for the PINE64 board too.

/Emil

>
> Changes since v4:
>  - Rebase on latest master
>
> Changes since v3:
>  - Rebase on linux-next/master
>  - use tabs for code indent
>
> Changes since v2:
>  - Rebase on 6.12
>
> E Shattow (1):
>   riscv: dts: starfive: jh7110-milkv-mars: enable usb0 host function
>
>  .../boot/dts/starfive/jh7110-milkv-mars.dts    | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> --
> 2.45.2
>
Re: [PATCH v5 0/1] riscv: dts: starfive: jh7110-milkv-mars: enable usb0 host function
Posted by E Shattow 1 year, 2 months ago
Hi Emil, thanks for taking time to review!

Added CC: Minda Chen, Hal Feng

Please Minda and Hal can you follow-up on Emil's comments as well?

On 11/27/24 03:00, Emil Renner Berthing wrote:
> E Shattow wrote:
>> Enable host mode JH7110 on-chip USB for Milk-V Mars by setting host mode
>> and connect vbus pinctrl.
>>
>> This functionality depends on setting the USB over-current register to
>> disable at bootloader phase, for example U-Boot:
>> https://patchwork.ozlabs.org/project/uboot/patch/20241012031328.4268-6-minda.chen@starfivetech.com/
> Hi E,
>
> Ideally the JH7110 pinctrl driver would be updated, so Linux can do this itself
> and doesn't need to rely on u-boot doing it. I already asked for this here:
>
> https://lore.kernel.org/all/CAJM55Z-+Cxdebcn4MLXfQdOVhx4c2SQ+zMH8cjn-Yq35xO8g0A@mail.gmail.com/

Yes, I agree, and Linux is not the only consumer of devicetree. I would 
like USB function to work for users of Linux and U-Boot on these boards 
by copying what the vendor Board Support Package does what is shipped 
with the products. If it is more in-depth than this I will defer to Hal 
or Minda.


For some wider context, upstream U-Boot is about to adopt the 
dt-rebasing via Hal's OF_UPSTREAM for JH7110 boards series and then also 
there is a patch set from Minda Chen to add the on-chip JH7110 USB 
support to U-Boot, and so then and there it will depend on these dts 
changes. If you have Milk-V Mars then already there are three of four 
USB-A receptacle ports which are functional on PCIe-connected VL805 USB 
chipset.

>
>> If the over-current register is not prepared for us then the result is no
>> change in functional outcome with this patch applied; there is an error
>> visible to the user and this additional usb configuration fails (same as
>> it is now). On Milk-V Mars with four USB-A ports this applies to one of the
>> ports and the remaining three VL805-connected ports via PCIe are not changed.
> Thanks for the patches. I don't quite understand when you write "no change in
> functional outcome with this patch applied". The USB-C port is already
> configured as a peripheral, and I just tried setting up an ethernet gadget on
> my VF2 running 6.12 and that works quite well. Does it not work on the Milk-V
> Mars board? If it does then these patches would break that functionality.
>
> Here is the script I used for that:
> https://paste.c-net.org/BravoLonely
>
> At the very least you'll need to explain in the commit message itself why
> changing the USB-C port from peripheral mode to host mode is OK. But ideally
> maybe you could make it work in OTG mode, so userspace can choose how they want
> to use the port. The same goes for the PINE64 board too.
>
> /Emil

USB-C port on Mars is not wired for data here, that is only true for 
VisionFive2. If the user wants to use their USB-A receptacle as OTG port 
I will not object to a future improvement, but here we want the basic 
expectations of users covered that they should have four working USB-A 
receptacle ports in U-Boot (and possibly in Linux, depending on the 
overcurrent register wherever it is set). This is what I am meaning, 
there may be somebody using a male-male USB-A USB-A cable for OTG but 
more likely is that people just want to plug in USB peripherals to host 
ports and use their mouse / keyboard / flash memory, I think.


There is no USB-C port on Star64.

>
>> Changes since v4:
>>   - Rebase on latest master
>>
>> Changes since v3:
>>   - Rebase on linux-next/master
>>   - use tabs for code indent
>>
>> Changes since v2:
>>   - Rebase on 6.12
>>
>> E Shattow (1):
>>    riscv: dts: starfive: jh7110-milkv-mars: enable usb0 host function
>>
>>   .../boot/dts/starfive/jh7110-milkv-mars.dts    | 18 +++++++++++++++++-
>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> --
>> 2.45.2
>>
Thanks again Emil.  -E

Re: [PATCH v5 0/1] riscv: dts: starfive: jh7110-milkv-mars: enable usb0 host function
Posted by Emil Renner Berthing 1 year, 2 months ago
E Shattow wrote:
> Hi Emil, thanks for taking time to review!
>
> Added CC: Minda Chen, Hal Feng
>
> Please Minda and Hal can you follow-up on Emil's comments as well?
>
> On 11/27/24 03:00, Emil Renner Berthing wrote:
> > E Shattow wrote:
> >> Enable host mode JH7110 on-chip USB for Milk-V Mars by setting host mode
> >> and connect vbus pinctrl.
> >>
> >> This functionality depends on setting the USB over-current register to
> >> disable at bootloader phase, for example U-Boot:
> >> https://patchwork.ozlabs.org/project/uboot/patch/20241012031328.4268-6-minda.chen@starfivetech.com/
> > Hi E,
> >
> > Ideally the JH7110 pinctrl driver would be updated, so Linux can do this itself
> > and doesn't need to rely on u-boot doing it. I already asked for this here:
> >
> > https://lore.kernel.org/all/CAJM55Z-+Cxdebcn4MLXfQdOVhx4c2SQ+zMH8cjn-Yq35xO8g0A@mail.gmail.com/
>
> Yes, I agree, and Linux is not the only consumer of devicetree. I would
> like USB function to work for users of Linux and U-Boot on these boards
> by copying what the vendor Board Support Package does what is shipped
> with the products. If it is more in-depth than this I will defer to Hal
> or Minda.
>
>
> For some wider context, upstream U-Boot is about to adopt the
> dt-rebasing via Hal's OF_UPSTREAM for JH7110 boards series and then also
> there is a patch set from Minda Chen to add the on-chip JH7110 USB
> support to U-Boot, and so then and there it will depend on these dts
> changes. If you have Milk-V Mars then already there are three of four
> USB-A receptacle ports which are functional on PCIe-connected VL805 USB
> chipset.
>
> >
> >> If the over-current register is not prepared for us then the result is no
> >> change in functional outcome with this patch applied; there is an error
> >> visible to the user and this additional usb configuration fails (same as
> >> it is now). On Milk-V Mars with four USB-A ports this applies to one of the
> >> ports and the remaining three VL805-connected ports via PCIe are not changed.
> > Thanks for the patches. I don't quite understand when you write "no change in
> > functional outcome with this patch applied". The USB-C port is already
> > configured as a peripheral, and I just tried setting up an ethernet gadget on
> > my VF2 running 6.12 and that works quite well. Does it not work on the Milk-V
> > Mars board? If it does then these patches would break that functionality.
> >
> > Here is the script I used for that:
> > https://paste.c-net.org/BravoLonely
> >
> > At the very least you'll need to explain in the commit message itself why
> > changing the USB-C port from peripheral mode to host mode is OK. But ideally
> > maybe you could make it work in OTG mode, so userspace can choose how they want
> > to use the port. The same goes for the PINE64 board too.
> >
> > /Emil
>
> USB-C port on Mars is not wired for data here, that is only true for
> VisionFive2. If the user wants to use their USB-A receptacle as OTG port
> I will not object to a future improvement, but here we want the basic
> expectations of users covered that they should have four working USB-A
> receptacle ports in U-Boot (and possibly in Linux, depending on the
> overcurrent register wherever it is set). This is what I am meaning,
> there may be somebody using a male-male USB-A USB-A cable for OTG but
> more likely is that people just want to plug in USB peripherals to host
> ports and use their mouse / keyboard / flash memory, I think.

You're right, sorry. I'm so used to the JH7110 boards being similar, but this
is actually one of the few differences between the Mars and VF2 that was not
caught when the Mars dts was first upstreamed.

Yes, with 4 similar USB-A ports you'd definitely expect all of them to work in
host mode. With an explanation like the above in the commit message I (now)
think your changes makes sense.

Thanks!
/Emil

>
>
> There is no USB-C port on Star64.
>
> >
> >> Changes since v4:
> >>   - Rebase on latest master
> >>
> >> Changes since v3:
> >>   - Rebase on linux-next/master
> >>   - use tabs for code indent
> >>
> >> Changes since v2:
> >>   - Rebase on 6.12
> >>
> >> E Shattow (1):
> >>    riscv: dts: starfive: jh7110-milkv-mars: enable usb0 host function
> >>
> >>   .../boot/dts/starfive/jh7110-milkv-mars.dts    | 18 +++++++++++++++++-
> >>   1 file changed, 17 insertions(+), 1 deletion(-)
> >>
> >> --
> >> 2.45.2
> >>
> Thanks again Emil.  -E
>