Message-ID: <971ae8a4-1add-45ed-8daf-951aef013e64@freeshell.de>
Date: Thu, 28 Nov 2024 00:01:04 -0800
From: E Shattow <e@...eshell.de>
To: Minda Chen <minda.chen@...rfivetech.com>,
 Emil Renner Berthing <emil.renner.berthing@...onical.com>,
 Jisheng Zhang <jszhang@...nel.org>
Cc: Emil Renner Berthing <kernel@...il.dk>, Conor Dooley <conor@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt
 <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
 "linux-riscv@...ts.infradead.org" <linux-riscv@...ts.infradead.org>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 Hal Feng <hal.feng@...rfivetech.com>
Subject: Re: [PATCH v5 0/1] riscv: dts: starfive: jh7110-milkv-mars: enable
 usb0 host function

Re-sending my reply plain text not HTML format. -E

On 11/27/24 17:31, Minda Chen wrote:
>> 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.426
>>>>> 8-6-minda.chen@...rfivetech.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@...l.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
>>
Hi Minda, thanks for taking time to review and explain the multiple USB 
configurations.

> Thanks. E Shadow
>
> The USB and PCIe0 setting in some JH7110 boards.
>                   PCIe0                                      cadence USB controller
> VF2:       connect with VL805 USB3.0 ( 4 USB 3.0 type A ports)       (USB 2.0)Type C port(role: peripheral)
> Milkv mars  connect with VL805 USB3.0 ( 3 USB 3.0 type A ports)       (USB 2.0)1 Type A port (role:host)
> Milkv cm    PCIe slot											(USB 2.0)micro B port (role:host)
> Star64       disabled（pcie PHY0 connect to cadence USB）         1 usb 3.0 port + USB 2.0 hub (role:host)
>
> H E Shadow
> I see you also add star64 USB configuration. I hope also add this to star64 board dts base on
> V6.13-rc1. Thanks.

Yes, on Star64 board I see there is one "blue colour" USB-A port and 
three "black colour" USB-A port. The product webpage 
https://wiki.pine64.org/wiki/STAR64 does say "1× USB3.0 Dedicated Host 
port / 3× USB2.0 Host port".

> &pcie0 {
> 	status = "disabled";
> };

For JH7110 is it allowed to have PCIe0 "okay" PCIe1 "okay" and also have 
USB 2.0?  We disable this PCIe0 here because of JH7110 USB3.0 design on 
Mars and on Star64?  Can JH7110 have USB 3.0 and USB 2.0 together (does 
Star64 do this, or is it USB 2.0 hub attach to JH7110 USB 3.0?)

> &pciephy0 {
> 	starfive,sys-syscon = <&sys_syscon 0x18>;
> 	starfive,stg-syscon = <&stg_syscon 0x148 0x1f4>;
> 	status = "okay";
> };

Minda, I need your help to make a good description of these numbers 0x18 
0x148 0x1f4, for the patch to Linux.


First is 'starfive,sys-sycon':

JH7110 Technical Reference Manual, Table 7. "SYS SYSCONSAIF SYSCFG 24 
Register Description"

https://doc-en.rvspace.org/JH7110/TRM/JH7110_TRM/sys_syscon.html#sys_syscon__table_c3l_fqs_wsb

Register SYS SYSCONSAIF SYSCFG 24 at offset 0x18 why is this needed?  
Your patch series for Linux 6.12 "Fix configuration for on-chip USB 2.0 
support"

https://lore.kernel.org/lkml/20241015070444.20972-1-minda.chen@starfivetech.com/

changes u0_pdrstn_split_sw_usbpipe_plugen in the driver and why do we 
need this now in the Star64 USB 3.0 dts for Linux 6.13?  Is this needed 
for USB function only u0_pdrstn_split_sw_usbpipe_plugen or is more from 
this register important here ?  And, can you make an explanation so I 
can learn what is the purpose of "u0_pdrstn_split_sw_usbpipe_plugen" or 
any other part of this register we need here (for USB)?


Next is 'stg-syscon':

JH7110 Technical Reference Manual, Table 82. "STG SYSCONSAIF SYSCFG 328 
Register Description"

https://doc-en.rvspace.org/JH7110/TRM/JH7110_TRM/stg_syscon.html#stg_syscon__table_mc4_n5s_wsb

Register STG SYSCONSAIF SYSCFG 328 at offset 0x148 what is this? 
Something PCIe?  Does this have to do with wires on the board going from 
USB-A port to PCIe? What is the explanation why we need this for USB?


JH7110 Technical Reference Manual, Table 125. "STG SYSCONSAIF SYSCFG 500 
Register Description"

https://doc-en.rvspace.org/JH7110/TRM/JH7110_TRM/stg_syscon.html#stg_syscon__table_njd_45s_wsb

Register STG SYSCONSAIF SYSCFG 500 at offset 0x1f4 is more usb3 bitfield 
names, what is a general description why this is needed? Is this "all 
the Cadence IP USB3 things" ?

The documentation devicetree/bindings/phy/starfive,jh7110-pcie-phy.yaml 
is not enough information to make a good description. "Total 2 registers 
offset" is a description of the place where the numbers exist for the 
schema but does not say if there is an ordering first,last and the 
expected register names in JH7110 TRM.  Is there more than one 
possibility? or else, will it 100% be exactly STG SYSCONSAIF SYSCFG 
registers 328 and 500 at offsets 0x148 and 0x1f4?


> &usb0 {
> 	pinctrl-names = "default";
> 	pinctrl-0 = <&usb_pins>;
> 	dr_mode = "host";
> 	status = "okay";
> };
>
> &usb_cdns3 {
> 	phys = <&usbphy0>, <&pciephy0>;
> 	phy-names = "cdns3,usb2-phy", "cdns3,usb3-phy";
> };
>
>>> 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
>>>

For Emil, my patches of Star64 and Mars are limited in scope to do the 
work of turning up host mode and adding the necessary vbus GPIO 
assignment;  so it's not wrong, and given the advice from Minda it's 
also not the complete support for all USB-A ports that I would like. I 
can extend this series to also include this adjacent USB 3.0 
configuration, or follow up in another series that is specific in topic 
for USB 3.0? Where I'm at on this is the host mode and GPIO 
configuration clearly belongs in dts but I have questions about this 
additional (USB 3.0 specific) configuration. And if we do, what do you 
think about the addition of syscon register offset values in devicetree, 
should they be #define'd somewhere? When it is the vbus GPIO there's 
literal sequential number of GPIO's so I did not find it was any problem 
to use the value. But, when I read "0x1f4" I think, what is that?


Best regards,

-E Shattow "Shadow"

Powered by blists - more mailing lists