Message-ID:
 <SHXPR01MB086308813C0B311845FDA2C7E629A@SHXPR01MB0863.CHNPR01.prod.partner.outlook.cn>
Date: Thu, 28 Nov 2024 01:31:04 +0000
From: Minda Chen <minda.chen@...rfivetech.com>
To: Emil Renner Berthing <emil.renner.berthing@...onical.com>, E Shattow
	<e@...eshell.de>, 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



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

&pcie0 {
	status = "disabled";
};

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

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

Powered by blists - more mailing lists