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

[Minda Chen]

> -----邮件原件-----
> 发件人: E Shattow <e@xxxxxxxxxxxx>
> 发送时间: 2024年11月28日 16:01
> 收件人: Minda Chen <minda.chen@xxxxxxxxxxxxxxxx>; Emil Renner Berthing
> <emil.renner.berthing@xxxxxxxxxxxxx>; Jisheng Zhang <jszhang@xxxxxxxxxx>
> 抄送: Emil Renner Berthing <kernel@xxxxxxxx>; Conor Dooley
> <conor@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzk+dt@xxxxxxxxxx>; Paul Walmsley <paul.walmsley@xxxxxxxxxx>; Palmer
> Dabbelt <palmer@xxxxxxxxxxx>; Albert Ou <aou@xxxxxxxxxxxxxxxxx>;
> linux-riscv@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; Hal Feng <hal.feng@xxxxxxxxxxxxxxxx>
> 主题: 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.42
> >>>>> 6
> >>>>> 8-6-minda.chen@xxxxxxxxxxxxxxxx/
> >>>> 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@xxxxxxxxxxxxxx/
> >>> 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?)
> 
Now you can see only JH7110 contain 2 pcie 2.0 combo PHY and 1 USB 2.0 PHY
Only PCIe0 PHY can move to Cadence USB 3.0 Controller.
So if enable Cadence USB 3.0 port. Only PCIe1 can be used.
Two solution
2 PCIe + USB 2.0 eg: VF2/ milk-v mars board.
1 PCIe + USB 3.0/ 2.0 eg: star64/starfive devkits board.

> > &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.
> 
JH7110 syscon/stgcon register offset.
Maybe move this to the PHY code is better.

> 
> 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_sys
> con__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@starfivete
> ch.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)?
> 
The bit mean split pcie PHY from cadence USB. So if use USB 2.0 only. The bit
must be set. But if PHY connect to Cadence USB. The bit must be cleared.
I think the bit should reset to 1 for the PHY default connect to PCIe but the reset
Value is 0. And it is a little issue in JH7110 IC.

> 
> 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_sy
> scon__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_sy
> scon__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 USB 3.0 PHY stg syscon setting. To make Cadence USB 3.0 work. Must set
Some syscon register.  It is belong to JH7110 IC.
You can check this in PCIe PHY code.

> 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"