[PATCH v2 0/3] riscv: jh7110: Fix configuration for on-chip USB 2.0 support

[PATCH v2 0/3] riscv: jh7110: Fix configuration for on-chip USB 2.0 support

[Jan Kiszka]

While mainline has support for the USB controller of the JH7110 since 6.5,
this never really worked, even not with latest downstream kernels by Starfive -
unless you were also using an old downstream U-Boot version. The reason for
that was a missing syscon setting that prevented the connection between USB
2.0 PHY and the controller. This series finally fixes the issue.

Changes in v2:
 - fix copy&paste mistake in error patch found by kernel test robot and Dan Carpenter

Jan

CC: Conor Dooley <conor+dt@kernel.org>
CC: Emil Renner Berthing <kernel@esmil.dk>
CC: Krzysztof Kozlowski <krzk+dt@kernel.org>
CC: Rob Herring <robh@kernel.org>

Jan Kiszka (3):
  dt-bindings: phy: jh7110-usb-phy: Add sys-syscon property
  riscv: dts: starfive: jh7110: Add sys-syscon property to usbphy0
  phy: starfive: jh7110-usb: Fix link configuration to controller

 .../bindings/phy/starfive,jh7110-usb-phy.yaml | 11 ++++++++++
 arch/riscv/boot/dts/starfive/jh7110.dtsi      |  1 +
 drivers/phy/starfive/phy-jh7110-usb.c         | 20 +++++++++++++++++++
 3 files changed, 32 insertions(+)

-- 
2.43.0

[PATCH v2 1/3] dt-bindings: phy: jh7110-usb-phy: Add sys-syscon property

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Analogously to the PCI PHY, access to sys_syscon is needed to connect
the USB PHY to its controller.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
CC: Rob Herring <robh@kernel.org>
CC: Krzysztof Kozlowski <krzk+dt@kernel.org>
CC: Conor Dooley <conor+dt@kernel.org>
---
 .../bindings/phy/starfive,jh7110-usb-phy.yaml         | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yaml b/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yaml
index 269e9f9f12b6..eaf0050c6f17 100644
--- a/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yaml
@@ -19,6 +19,16 @@ properties:
   "#phy-cells":
     const: 0
 
+  starfive,sys-syscon:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - items:
+          - description: phandle to System Register Controller sys_syscon node.
+          - description: PHY connect offset of SYS_SYSCONSAIF__SYSCFG register for USB PHY.
+    description:
+      The phandle to System Register Controller syscon node and the PHY connect offset
+      of SYS_SYSCONSAIF__SYSCFG register. Connect PHY to USB controller.
+
   clocks:
     items:
       - description: PHY 125m
@@ -47,4 +57,5 @@ examples:
                  <&stgcrg 6>;
         clock-names = "125m", "app_125m";
         #phy-cells = <0>;
+        starfive,sys-syscon = <&sys_syscon 0x18>;
     };
-- 
2.43.0

[PATCH v2 2/3] riscv: dts: starfive: jh7110: Add sys-syscon property to usbphy0

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Allows the PHY to connect to its USB controller.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
CC: Rob Herring <robh@kernel.org>
CC: Krzysztof Kozlowski <krzk+dt@kernel.org>
CC: Conor Dooley <conor+dt@kernel.org>
---
 arch/riscv/boot/dts/starfive/jh7110.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
index 0d8339357bad..0c0b66a69065 100644
--- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
@@ -606,6 +606,7 @@ usbphy0: phy@10200000 {
 				 <&stgcrg JH7110_STGCLK_USB0_APP_125>;
 			clock-names = "125m", "app_125m";
 			#phy-cells = <0>;
+			starfive,sys-syscon = <&sys_syscon 0x18>;
 		};
 
 		pciephy0: phy@10210000 {
-- 
2.43.0

[PATCH v2 3/3] phy: starfive: jh7110-usb: Fix link configuration to controller

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

In order to connect the USB 2.0 PHY to its controller, we also need to
set "u0_pdrstn_split_sw_usbpipe_plugen" [1]. Some downstream U-Boot
versions did that, but upstream firmware does not, and the kernel must
not rely on such behavior anyway. Failing to set this left the USB
gadget port invisible to connected hosts behind.

Link: https://doc-en.rvspace.org/JH7110/TRM/JH7110_TRM/sys_syscon.html#sys_syscon__section_b3l_fqs_wsb [1]
Fixes: 16d3a71c20cf ("phy: starfive: Add JH7110 USB 2.0 PHY driver")
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Reviewed-by: Minda Chen <minda.chen@starfivetech.com>
---
 drivers/phy/starfive/phy-jh7110-usb.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/phy/starfive/phy-jh7110-usb.c b/drivers/phy/starfive/phy-jh7110-usb.c
index 633912f8a05d..89451e740f77 100644
--- a/drivers/phy/starfive/phy-jh7110-usb.c
+++ b/drivers/phy/starfive/phy-jh7110-usb.c
@@ -10,18 +10,24 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/io.h>
+#include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
+#include <linux/regmap.h>
 #include <linux/usb/of.h>
 
 #define USB_125M_CLK_RATE		125000000
 #define USB_LS_KEEPALIVE_OFF		0x4
 #define USB_LS_KEEPALIVE_ENABLE		BIT(4)
 
+#define USB_PDRSTN_SPLIT		BIT(17)
+
 struct jh7110_usb2_phy {
 	struct phy *phy;
 	void __iomem *regs;
+	struct regmap *sys_syscon;
+	u32 sys_phy_connect;
 	struct clk *usb_125m_clk;
 	struct clk *app_125m;
 	enum phy_mode mode;
@@ -61,6 +67,10 @@ static int usb2_phy_set_mode(struct phy *_phy,
 		usb2_set_ls_keepalive(phy, (mode != PHY_MODE_USB_DEVICE));
 	}
 
+	/* Connect usb 2.0 phy mode */
+	regmap_update_bits(phy->sys_syscon, phy->sys_phy_connect,
+			   USB_PDRSTN_SPLIT, USB_PDRSTN_SPLIT);
+
 	return 0;
 }
 
@@ -101,6 +111,7 @@ static int jh7110_usb_phy_probe(struct platform_device *pdev)
 	struct jh7110_usb2_phy *phy;
 	struct device *dev = &pdev->dev;
 	struct phy_provider *phy_provider;
+	u32 args[1];
 
 	phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL);
 	if (!phy)
@@ -129,6 +140,15 @@ static int jh7110_usb_phy_probe(struct platform_device *pdev)
 	phy_set_drvdata(phy->phy, phy);
 	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
 
+	phy->sys_syscon =
+		syscon_regmap_lookup_by_phandle_args(pdev->dev.of_node,
+						     "starfive,sys-syscon",
+						     1, args);
+	if (IS_ERR(phy->sys_syscon))
+		return dev_err_probe(dev, PTR_ERR(phy->sys_syscon),
+			"Failed to get sys-syscon\n");
+	phy->sys_phy_connect = args[0];
+
 	return PTR_ERR_OR_ZERO(phy_provider);
 }
 
-- 
2.43.0

Re: [PATCH v2 1/3] dt-bindings: phy: jh7110-usb-phy: Add sys-syscon property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1984 bytes --]

On Mon, Aug 12, 2024 at 04:15:51PM +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Analogously to the PCI PHY, access to sys_syscon is needed to connect
> the USB PHY to its controller.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> CC: Rob Herring <robh@kernel.org>
> CC: Krzysztof Kozlowski <krzk+dt@kernel.org>
> CC: Conor Dooley <conor+dt@kernel.org>
> ---
>  .../bindings/phy/starfive,jh7110-usb-phy.yaml         | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yaml b/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yaml
> index 269e9f9f12b6..eaf0050c6f17 100644
> --- a/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yaml
> @@ -19,6 +19,16 @@ properties:
>    "#phy-cells":
>      const: 0
>  
> +  starfive,sys-syscon:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - items:
> +          - description: phandle to System Register Controller sys_syscon node.
> +          - description: PHY connect offset of SYS_SYSCONSAIF__SYSCFG register for USB PHY.

Why is having a new property for this required? The devicetree only has
a single usb phy, so isn't it sufficient to look up the syscon by
compatible, rather than via phandle + offset?

> +    description:
> +      The phandle to System Register Controller syscon node and the PHY connect offset
> +      of SYS_SYSCONSAIF__SYSCFG register. Connect PHY to USB controller.
> +
>    clocks:
>      items:
>        - description: PHY 125m
> @@ -47,4 +57,5 @@ examples:
>                   <&stgcrg 6>;
>          clock-names = "125m", "app_125m";
>          #phy-cells = <0>;
> +        starfive,sys-syscon = <&sys_syscon 0x18>;
>      };
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/3] dt-bindings: phy: jh7110-usb-phy: Add sys-syscon property

[Jan Kiszka]

On 12.08.24 17:55, Conor Dooley wrote:
> On Mon, Aug 12, 2024 at 04:15:51PM +0200, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Analogously to the PCI PHY, access to sys_syscon is needed to connect
>> the USB PHY to its controller.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
>> ---
>> CC: Rob Herring <robh@kernel.org>
>> CC: Krzysztof Kozlowski <krzk+dt@kernel.org>
>> CC: Conor Dooley <conor+dt@kernel.org>
>> ---
>>  .../bindings/phy/starfive,jh7110-usb-phy.yaml         | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yaml b/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yaml
>> index 269e9f9f12b6..eaf0050c6f17 100644
>> --- a/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yaml
>> +++ b/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yaml
>> @@ -19,6 +19,16 @@ properties:
>>    "#phy-cells":
>>      const: 0
>>  
>> +  starfive,sys-syscon:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    items:
>> +      - items:
>> +          - description: phandle to System Register Controller sys_syscon node.
>> +          - description: PHY connect offset of SYS_SYSCONSAIF__SYSCFG register for USB PHY.
> 
> Why is having a new property for this required? The devicetree only has
> a single usb phy, so isn't it sufficient to look up the syscon by
> compatible, rather than via phandle + offset?
> 

I didn't design this, I just copied it from
starfive,jh7110-pcie-phy.yaml. As that already exists, I'm neither sure
we want to change that anymore nor deviate in the pattern here.

Jan

>> +    description:
>> +      The phandle to System Register Controller syscon node and the PHY connect offset
>> +      of SYS_SYSCONSAIF__SYSCFG register. Connect PHY to USB controller.
>> +
>>    clocks:
>>      items:
>>        - description: PHY 125m
>> @@ -47,4 +57,5 @@ examples:
>>                   <&stgcrg 6>;
>>          clock-names = "125m", "app_125m";
>>          #phy-cells = <0>;
>> +        starfive,sys-syscon = <&sys_syscon 0x18>;
>>      };
>> -- 
>> 2.43.0
>>

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH v2 1/3] dt-bindings: phy: jh7110-usb-phy: Add sys-syscon property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3028 bytes --]

On Tue, Aug 13, 2024 at 07:31:50AM +0200, Jan Kiszka wrote:
> On 12.08.24 17:55, Conor Dooley wrote:
> > On Mon, Aug 12, 2024 at 04:15:51PM +0200, Jan Kiszka wrote:
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> Analogously to the PCI PHY, access to sys_syscon is needed to connect
> >> the USB PHY to its controller.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> >> ---
> >> CC: Rob Herring <robh@kernel.org>
> >> CC: Krzysztof Kozlowski <krzk+dt@kernel.org>
> >> CC: Conor Dooley <conor+dt@kernel.org>
> >> ---
> >>  .../bindings/phy/starfive,jh7110-usb-phy.yaml         | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yaml b/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yaml
> >> index 269e9f9f12b6..eaf0050c6f17 100644
> >> --- a/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yaml
> >> +++ b/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yaml
> >> @@ -19,6 +19,16 @@ properties:
> >>    "#phy-cells":
> >>      const: 0
> >>  
> >> +  starfive,sys-syscon:
> >> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> >> +    items:
> >> +      - items:
> >> +          - description: phandle to System Register Controller sys_syscon node.
> >> +          - description: PHY connect offset of SYS_SYSCONSAIF__SYSCFG register for USB PHY.
> > 
> > Why is having a new property for this required? The devicetree only has
> > a single usb phy, so isn't it sufficient to look up the syscon by
> > compatible, rather than via phandle + offset?
> > 
> 
> I didn't design this, I just copied it from
> starfive,jh7110-pcie-phy.yaml. As that already exists, I'm neither sure
> we want to change that anymore nor deviate in the pattern here.

To be honest, I think some of the other users of phandle + offset on
this soc were just copy-pasted without thinking about whether or not they
were required too. This one seems like it should just be a lookup by
compatible in the driver instead of by phandle. As a bonus, it will work
with existing devicetrees - whereas your current implementation will
fail to probe on systems that have the old devicetree, a regression for
systems running with that devicetree and downstream firmware.

Cheers,
Conor.

> Jan
> 
> >> +    description:
> >> +      The phandle to System Register Controller syscon node and the PHY connect offset
> >> +      of SYS_SYSCONSAIF__SYSCFG register. Connect PHY to USB controller.
> >> +
> >>    clocks:
> >>      items:
> >>        - description: PHY 125m
> >> @@ -47,4 +57,5 @@ examples:
> >>                   <&stgcrg 6>;
> >>          clock-names = "125m", "app_125m";
> >>          #phy-cells = <0>;
> >> +        starfive,sys-syscon = <&sys_syscon 0x18>;
> >>      };
> >> -- 
> >> 2.43.0
> >>
> 
> -- 
> Siemens AG, Technology
> Linux Expert Center
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

回复: [PATCH v2 1/3] dt-bindings: phy: jh7110-usb-phy: Add sys-syscon property

[Minda Chen]

> 
> On Tue, Aug 13, 2024 at 07:31:50AM +0200, Jan Kiszka wrote:
> > On 12.08.24 17:55, Conor Dooley wrote:
> > > On Mon, Aug 12, 2024 at 04:15:51PM +0200, Jan Kiszka wrote:
> > >> From: Jan Kiszka <jan.kiszka@siemens.com>
> > >>
> > >> Analogously to the PCI PHY, access to sys_syscon is needed to
> > >> connect the USB PHY to its controller.
> > >>
> > >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > >> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > >> ---
> > >> CC: Rob Herring <robh@kernel.org>
> > >> CC: Krzysztof Kozlowski <krzk+dt@kernel.org>
> > >> CC: Conor Dooley <conor+dt@kernel.org>
> > >> ---
> > >>  .../bindings/phy/starfive,jh7110-usb-phy.yaml         | 11
> +++++++++++
> > >>  1 file changed, 11 insertions(+)
> > >>
> > >> diff --git
> > >> a/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yam
> > >> l
> > >> b/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yam
> > >> l index 269e9f9f12b6..eaf0050c6f17 100644
> > >> ---
> > >> a/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yam
> > >> l
> > >> +++ b/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy
> > >> +++ .yaml
> > >> @@ -19,6 +19,16 @@ properties:
> > >>    "#phy-cells":
> > >>      const: 0
> > >>
> > >> +  starfive,sys-syscon:
> > >> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > >> +    items:
> > >> +      - items:
> > >> +          - description: phandle to System Register Controller
> sys_syscon node.
> > >> +          - description: PHY connect offset of
> SYS_SYSCONSAIF__SYSCFG register for USB PHY.
> > >
> > > Why is having a new property for this required? The devicetree only
> > > has a single usb phy, so isn't it sufficient to look up the syscon
> > > by compatible, rather than via phandle + offset?
> > >
> >
> > I didn't design this, I just copied it from
> > starfive,jh7110-pcie-phy.yaml. As that already exists, I'm neither
> > sure we want to change that anymore nor deviate in the pattern here.
> 
> To be honest, I think some of the other users of phandle + offset on this soc were
> just copy-pasted without thinking about whether or not they were required too.
> This one seems like it should just be a lookup by compatible in the driver instead
> of by phandle. As a bonus, it will work with existing devicetrees - whereas your
> current implementation will fail to probe on systems that have the old
> devicetree, a regression for systems running with that devicetree and
> downstream firmware.
> 
> Cheers,
> Conor.
> 
Hi Conor
I know you would like to put the offset value to the code, Just set syscon in dts.
Just like pcie-starfive.c. right?

> > Jan
> >
> > >> +    description:
> > >> +      The phandle to System Register Controller syscon node and the
> PHY connect offset
> > >> +      of SYS_SYSCONSAIF__SYSCFG register. Connect PHY to USB
> controller.
> > >> +
> > >>    clocks:
> > >>      items:
> > >>        - description: PHY 125m
> > >> @@ -47,4 +57,5 @@ examples:
> > >>                   <&stgcrg 6>;
> > >>          clock-names = "125m", "app_125m";
> > >>          #phy-cells = <0>;
> > >> +        starfive,sys-syscon = <&sys_syscon 0x18>;
> > >>      };
> > >> --
> > >> 2.43.0
> > >>
> >
> > --
> > Siemens AG, Technology
> > Linux Expert Center
> >

Re: 回复: [PATCH v2 1/3] dt-bindings: phy: jh7110-usb-phy: Add sys-syscon property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3039 bytes --]

On Thu, Aug 15, 2024 at 10:33:55AM +0000, Minda Chen wrote:
> 
> 
> > 
> > On Tue, Aug 13, 2024 at 07:31:50AM +0200, Jan Kiszka wrote:
> > > On 12.08.24 17:55, Conor Dooley wrote:
> > > > On Mon, Aug 12, 2024 at 04:15:51PM +0200, Jan Kiszka wrote:
> > > >> From: Jan Kiszka <jan.kiszka@siemens.com>
> > > >>
> > > >> Analogously to the PCI PHY, access to sys_syscon is needed to
> > > >> connect the USB PHY to its controller.
> > > >>
> > > >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > >> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > >> ---
> > > >> CC: Rob Herring <robh@kernel.org>
> > > >> CC: Krzysztof Kozlowski <krzk+dt@kernel.org>
> > > >> CC: Conor Dooley <conor+dt@kernel.org>
> > > >> ---
> > > >>  .../bindings/phy/starfive,jh7110-usb-phy.yaml         | 11
> > +++++++++++
> > > >>  1 file changed, 11 insertions(+)
> > > >>
> > > >> diff --git
> > > >> a/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yam
> > > >> l
> > > >> b/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yam
> > > >> l index 269e9f9f12b6..eaf0050c6f17 100644
> > > >> ---
> > > >> a/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy.yam
> > > >> l
> > > >> +++ b/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy
> > > >> +++ .yaml
> > > >> @@ -19,6 +19,16 @@ properties:
> > > >>    "#phy-cells":
> > > >>      const: 0
> > > >>
> > > >> +  starfive,sys-syscon:
> > > >> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > > >> +    items:
> > > >> +      - items:
> > > >> +          - description: phandle to System Register Controller
> > sys_syscon node.
> > > >> +          - description: PHY connect offset of
> > SYS_SYSCONSAIF__SYSCFG register for USB PHY.
> > > >
> > > > Why is having a new property for this required? The devicetree only
> > > > has a single usb phy, so isn't it sufficient to look up the syscon
> > > > by compatible, rather than via phandle + offset?
> > > >
> > >
> > > I didn't design this, I just copied it from
> > > starfive,jh7110-pcie-phy.yaml. As that already exists, I'm neither
> > > sure we want to change that anymore nor deviate in the pattern here.
> > 
> > To be honest, I think some of the other users of phandle + offset on this soc were
> > just copy-pasted without thinking about whether or not they were required too.
> > This one seems like it should just be a lookup by compatible in the driver instead
> > of by phandle. As a bonus, it will work with existing devicetrees - whereas your
> > current implementation will fail to probe on systems that have the old
> > devicetree, a regression for systems running with that devicetree and
> > downstream firmware.
> > 
> > Cheers,
> > Conor.
> > 
> Hi Conor
> I know you would like to put the offset value to the code, Just set syscon in dts.
> Just like pcie-starfive.c. right?

No, not quite. That still uses a phandle lookup, I was talking about
using syscon_regmap_lookup_by_compatible().

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/3] dt-bindings: phy: jh7110-usb-phy: Add sys-syscon property

[Minda Chen]

> 
> On Thu, Aug 15, 2024 at 10:33:55AM +0000, Minda Chen wrote:
> >
> >
> > >
> > > On Tue, Aug 13, 2024 at 07:31:50AM +0200, Jan Kiszka wrote:
> > > > On 12.08.24 17:55, Conor Dooley wrote:
> > > > > On Mon, Aug 12, 2024 at 04:15:51PM +0200, Jan Kiszka wrote:
> > > > >> From: Jan Kiszka <jan.kiszka@siemens.com>
> > > > >>
> > > > >> Analogously to the PCI PHY, access to sys_syscon is needed to
> > > > >> connect the USB PHY to its controller.
> > > > >>
> > > > >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > > >> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > > >> ---
> > > > >> CC: Rob Herring <robh@kernel.org>
> > > > >> CC: Krzysztof Kozlowski <krzk+dt@kernel.org>
> > > > >> CC: Conor Dooley <conor+dt@kernel.org>
> > > > >> ---
> > > > >>  .../bindings/phy/starfive,jh7110-usb-phy.yaml         | 11
> > > +++++++++++
> > > > >>  1 file changed, 11 insertions(+)
> > > > >>
> > > > >> diff --git
> > > > >> a/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy
> > > > >> .yam
> > > > >> l
> > > > >> b/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy
> > > > >> .yam l index 269e9f9f12b6..eaf0050c6f17 100644
> > > > >> ---
> > > > >> a/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy
> > > > >> .yam
> > > > >> l
> > > > >> +++ b/Documentation/devicetree/bindings/phy/starfive,jh7110-usb
> > > > >> +++ -phy
> > > > >> +++ .yaml
> > > > >> @@ -19,6 +19,16 @@ properties:
> > > > >>    "#phy-cells":
> > > > >>      const: 0
> > > > >>
> > > > >> +  starfive,sys-syscon:
> > > > >> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > > > >> +    items:
> > > > >> +      - items:
> > > > >> +          - description: phandle to System Register Controller
> > > sys_syscon node.
> > > > >> +          - description: PHY connect offset of
> > > SYS_SYSCONSAIF__SYSCFG register for USB PHY.
> > > > >
> > > > > Why is having a new property for this required? The devicetree
> > > > > only has a single usb phy, so isn't it sufficient to look up the
> > > > > syscon by compatible, rather than via phandle + offset?
> > > > >
> > > >
> > > > I didn't design this, I just copied it from
> > > > starfive,jh7110-pcie-phy.yaml. As that already exists, I'm neither
> > > > sure we want to change that anymore nor deviate in the pattern here.
> > >
> > > To be honest, I think some of the other users of phandle + offset on
> > > this soc were just copy-pasted without thinking about whether or not they
> were required too.
> > > This one seems like it should just be a lookup by compatible in the
> > > driver instead of by phandle. As a bonus, it will work with existing
> > > devicetrees - whereas your current implementation will fail to probe
> > > on systems that have the old devicetree, a regression for systems
> > > running with that devicetree and downstream firmware.
> > >
> > > Cheers,
> > > Conor.
> > >
> > Hi Conor
> > I know you would like to put the offset value to the code, Just set syscon in dts.
> > Just like pcie-starfive.c. right?
> 
> No, not quite. That still uses a phandle lookup, I was talking about using
> syscon_regmap_lookup_by_compatible().

Okay. Using syscon_regmap_lookup_by_compatible() can just modify the driver code only.
But syscon_regmap_lookup_by_compatible() is not exist in uboot now. If I want to enable
CONFIG_OF_UPSTREAM in uboot. I have to add this function in u-boot...

Re: [PATCH v2 1/3] dt-bindings: phy: jh7110-usb-phy: Add sys-syscon property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3832 bytes --]

On Mon, Sep 02, 2024 at 09:19:29AM +0000, Minda Chen wrote:
> 
> 
> > 
> > On Thu, Aug 15, 2024 at 10:33:55AM +0000, Minda Chen wrote:
> > >
> > >
> > > >
> > > > On Tue, Aug 13, 2024 at 07:31:50AM +0200, Jan Kiszka wrote:
> > > > > On 12.08.24 17:55, Conor Dooley wrote:
> > > > > > On Mon, Aug 12, 2024 at 04:15:51PM +0200, Jan Kiszka wrote:
> > > > > >> From: Jan Kiszka <jan.kiszka@siemens.com>
> > > > > >>
> > > > > >> Analogously to the PCI PHY, access to sys_syscon is needed to
> > > > > >> connect the USB PHY to its controller.
> > > > > >>
> > > > > >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > > > >> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > >> ---
> > > > > >> CC: Rob Herring <robh@kernel.org>
> > > > > >> CC: Krzysztof Kozlowski <krzk+dt@kernel.org>
> > > > > >> CC: Conor Dooley <conor+dt@kernel.org>
> > > > > >> ---
> > > > > >>  .../bindings/phy/starfive,jh7110-usb-phy.yaml         | 11
> > > > +++++++++++
> > > > > >>  1 file changed, 11 insertions(+)
> > > > > >>
> > > > > >> diff --git
> > > > > >> a/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy
> > > > > >> .yam
> > > > > >> l
> > > > > >> b/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy
> > > > > >> .yam l index 269e9f9f12b6..eaf0050c6f17 100644
> > > > > >> ---
> > > > > >> a/Documentation/devicetree/bindings/phy/starfive,jh7110-usb-phy
> > > > > >> .yam
> > > > > >> l
> > > > > >> +++ b/Documentation/devicetree/bindings/phy/starfive,jh7110-usb
> > > > > >> +++ -phy
> > > > > >> +++ .yaml
> > > > > >> @@ -19,6 +19,16 @@ properties:
> > > > > >>    "#phy-cells":
> > > > > >>      const: 0
> > > > > >>
> > > > > >> +  starfive,sys-syscon:
> > > > > >> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > > > > >> +    items:
> > > > > >> +      - items:
> > > > > >> +          - description: phandle to System Register Controller
> > > > sys_syscon node.
> > > > > >> +          - description: PHY connect offset of
> > > > SYS_SYSCONSAIF__SYSCFG register for USB PHY.
> > > > > >
> > > > > > Why is having a new property for this required? The devicetree
> > > > > > only has a single usb phy, so isn't it sufficient to look up the
> > > > > > syscon by compatible, rather than via phandle + offset?
> > > > > >
> > > > >
> > > > > I didn't design this, I just copied it from
> > > > > starfive,jh7110-pcie-phy.yaml. As that already exists, I'm neither
> > > > > sure we want to change that anymore nor deviate in the pattern here.
> > > >
> > > > To be honest, I think some of the other users of phandle + offset on
> > > > this soc were just copy-pasted without thinking about whether or not they
> > were required too.
> > > > This one seems like it should just be a lookup by compatible in the
> > > > driver instead of by phandle. As a bonus, it will work with existing
> > > > devicetrees - whereas your current implementation will fail to probe
> > > > on systems that have the old devicetree, a regression for systems
> > > > running with that devicetree and downstream firmware.
> > > >
> > > > Cheers,
> > > > Conor.
> > > >
> > > Hi Conor
> > > I know you would like to put the offset value to the code, Just set syscon in dts.
> > > Just like pcie-starfive.c. right?
> > 
> > No, not quite. That still uses a phandle lookup, I was talking about using
> > syscon_regmap_lookup_by_compatible().
> 
> Okay. Using syscon_regmap_lookup_by_compatible() can just modify the driver code only.
> But syscon_regmap_lookup_by_compatible() is not exist in uboot now. If I want to enable
> CONFIG_OF_UPSTREAM in uboot. I have to add this function in u-boot...

You can use
	node = ofnode_by_compatible(ofnode_null(), "foo");
	*regmap = syscon_node_to_regmap(node);
in U-Boot.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]