On Thu, Aug 15, 2024 at 10:33:55AM +0000, Minda Chen wrote:

quoted

On Tue, Aug 13, 2024 at 07:31:50AM +0200, Jan Kiszka wrote:

quoted

On 12.08.24 17:55, Conor Dooley wrote:

quoted

On Mon, Aug 12, 2024 at 04:15:51PM +0200, Jan Kiszka wrote:

quoted

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

+++++++++++

quoted

quoted

quoted

 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.

quoted

quoted

quoted

+          - description: PHY connect offset of

SYS_SYSCONSAIF__SYSCFG register for USB PHY.

quoted

quoted

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