[PATCH v1 0/4] add syscon requirement for mt7988

[PATCH v1 0/4] add syscon requirement for mt7988

[Frank Wunderlich]

From: Frank Wunderlich <frank-w@public-files.de>

Some nodes require the syscon fallback at least in u-boot when using
OF_UPSTREAM.

This is because uboot driver uses syscon_node_to_regmap in mtk_eth.c for
"mediatek,toprgu", "mediatek,xfi_pll" and reset pointing to watchdog-node.

Frank Wunderlich (4):
  dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 syscon requirement
  dt-bindings: clock: mediatek: add syscon requirement for mt7988
    xfi-pll
  dt-bindings: clock: mediatek: add syscon requirement for mt7988
    ethwarp
  arm64: dts: mediatek: mt7988: add syscon for watchdog, xfi-pll and
    ethwarp

 .../devicetree/bindings/clock/mediatek,mt7988-ethwarp.yaml | 6 ++++--
 .../devicetree/bindings/clock/mediatek,mt7988-xfi-pll.yaml | 7 +++++--
 .../devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml     | 5 ++++-
 arch/arm64/boot/dts/mediatek/mt7988a.dtsi                  | 6 +++---
 4 files changed, 16 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH v1 1/4] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 syscon requirement

[Frank Wunderlich]

From: Frank Wunderlich <frank-w@public-files.de>

This is needed by u-boot-driver when using OF_UPSTREAM.

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 .../devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml       | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
index 8d2520241e37..681df2ff9f2e 100644
--- a/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml
@@ -25,7 +25,6 @@ properties:
           - mediatek,mt6735-wdt
           - mediatek,mt6795-wdt
           - mediatek,mt7986-wdt
-          - mediatek,mt7988-wdt
           - mediatek,mt8183-wdt
           - mediatek,mt8186-wdt
           - mediatek,mt8188-wdt
@@ -43,6 +42,10 @@ properties:
               - mediatek,mt8365-wdt
               - mediatek,mt8516-wdt
           - const: mediatek,mt6589-wdt
+      - items:
+          - enum:
+              - mediatek,mt7988-wdt
+          - const: syscon
 
   reg:
     maxItems: 1
-- 
2.43.0

Re: [PATCH v1 1/4] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 syscon requirement

[Krzysztof Kozlowski]

On 09/07/2024 12:13, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> This is needed by u-boot-driver when using OF_UPSTREAM.

Thanks for providing context. What we miss here still, is the
description of the hardware and for what this syscon is used. Why this
become system controller and before was not? Or this is just for regmap
- then it does not work that way... You cannot have whatever driver
poking all around then claim - I have crappy driver poking to every
driver without implementing proper driver model, so I add syscon to
bindings.



Best regards,
Krzysztof

Re: [PATCH v1 1/4] dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 syscon requirement

[Frank Wunderlich]

Am 13. Juli 2024 12:21:46 MESZ schrieb Krzysztof Kozlowski <krzk@kernel.org>:
>On 09/07/2024 12:13, Frank Wunderlich wrote:
>> From: Frank Wunderlich <frank-w@public-files.de>
>> 
>> This is needed by u-boot-driver when using OF_UPSTREAM.
>
>Thanks for providing context. What we miss here still, is the
>description of the hardware and for what this syscon is used. Why this
>become system controller and before was not? Or this is just for regmap
>- then it does not work that way... You cannot have whatever driver
>poking all around then claim - I have crappy driver poking to every
>driver without implementing proper driver model, so I add syscon to
>bindings.
>
>
>
>Best regards,
>Krzysztof
>

The nodes here are linked to the ethernet-node and syscon is only needed to get the regmap...the access to it is done by ethernet driver. It does work,but maybe not the right way...
regards Frank

[PATCH v1 2/4] dt-bindings: clock: mediatek: add syscon requirement for mt7988 xfi-pll

[Frank Wunderlich]

From: Frank Wunderlich <frank-w@public-files.de>

This is needed by u-boot-driver when using OF_UPSTREAM.

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 .../devicetree/bindings/clock/mediatek,mt7988-xfi-pll.yaml | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/mediatek,mt7988-xfi-pll.yaml b/Documentation/devicetree/bindings/clock/mediatek,mt7988-xfi-pll.yaml
index 192f1451f0af..c3686712505a 100644
--- a/Documentation/devicetree/bindings/clock/mediatek,mt7988-xfi-pll.yaml
+++ b/Documentation/devicetree/bindings/clock/mediatek,mt7988-xfi-pll.yaml
@@ -15,7 +15,10 @@ description:
 
 properties:
   compatible:
-    const: mediatek,mt7988-xfi-pll
+    items:
+      - enum:
+          - mediatek,mt7988-xfi-pll
+      - const: syscon
 
   reg:
     maxItems: 1
@@ -40,7 +43,7 @@ examples:
         #address-cells = <2>;
         #size-cells = <2>;
         clock-controller@11f40000 {
-            compatible = "mediatek,mt7988-xfi-pll";
+            compatible = "mediatek,mt7988-xfi-pll", "syscon";
             reg = <0 0x11f40000 0 0x1000>;
             resets = <&watchdog 16>;
             #clock-cells = <1>;
-- 
2.43.0

Re: [PATCH v1 2/4] dt-bindings: clock: mediatek: add syscon requirement for mt7988 xfi-pll

[Krzysztof Kozlowski]

On 09/07/2024 12:13, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> This is needed by u-boot-driver when using OF_UPSTREAM.
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>  .../devicetree/bindings/clock/mediatek,mt7988-xfi-pll.yaml | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/mediatek,mt7988-xfi-pll.yaml b/Documentation/devicetree/bindings/clock/mediatek,mt7988-xfi-pll.yaml
> index 192f1451f0af..c3686712505a 100644
> --- a/Documentation/devicetree/bindings/clock/mediatek,mt7988-xfi-pll.yaml
> +++ b/Documentation/devicetree/bindings/clock/mediatek,mt7988-xfi-pll.yaml
> @@ -15,7 +15,10 @@ description:
>  
>  properties:
>    compatible:
> -    const: mediatek,mt7988-xfi-pll
> +    items:
> +      - enum:
> +          - mediatek,mt7988-xfi-pll
> +      - const: syscon

No.

Crappy drivers which do not implement clock bindings do not get workaround.

Best regards,
Krzysztof

[PATCH v1 3/4] dt-bindings: clock: mediatek: add syscon requirement for mt7988 ethwarp

[Frank Wunderlich]

From: Frank Wunderlich <frank-w@public-files.de>

This is needed by u-boot-driver when using OF_UPSTREAM.

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 .../devicetree/bindings/clock/mediatek,mt7988-ethwarp.yaml  | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/mediatek,mt7988-ethwarp.yaml b/Documentation/devicetree/bindings/clock/mediatek,mt7988-ethwarp.yaml
index e32a0251ff6a..b460a0de8503 100644
--- a/Documentation/devicetree/bindings/clock/mediatek,mt7988-ethwarp.yaml
+++ b/Documentation/devicetree/bindings/clock/mediatek,mt7988-ethwarp.yaml
@@ -17,7 +17,9 @@ description:
 properties:
   compatible:
     items:
-      - const: mediatek,mt7988-ethwarp
+      - enum:
+          - mediatek,mt7988-ethwarp
+      - const: syscon
 
   reg:
     maxItems: 1
@@ -44,7 +46,7 @@ examples:
         #size-cells = <2>;
 
         clock-controller@15031000 {
-            compatible = "mediatek,mt7988-ethwarp";
+            compatible = "mediatek,mt7988-ethwarp", "syscon";
             reg = <0 0x15031000 0 0x1000>;
             #clock-cells = <1>;
             #reset-cells = <1>;
-- 
2.43.0

Re: [PATCH v1 3/4] dt-bindings: clock: mediatek: add syscon requirement for mt7988 ethwarp

[Krzysztof Kozlowski]

On 09/07/2024 12:13, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> This is needed by u-boot-driver when using OF_UPSTREAM.
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>  .../devicetree/bindings/clock/mediatek,mt7988-ethwarp.yaml  | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/mediatek,mt7988-ethwarp.yaml b/Documentation/devicetree/bindings/clock/mediatek,mt7988-ethwarp.yaml
> index e32a0251ff6a..b460a0de8503 100644
> --- a/Documentation/devicetree/bindings/clock/mediatek,mt7988-ethwarp.yaml
> +++ b/Documentation/devicetree/bindings/clock/mediatek,mt7988-ethwarp.yaml
> @@ -17,7 +17,9 @@ description:
>  properties:
>    compatible:
>      items:
> -      - const: mediatek,mt7988-ethwarp
> +      - enum:
> +          - mediatek,mt7988-ethwarp
> +      - const: syscon

No, for the same reason.

Best regards,
Krzysztof

[PATCH v1 4/4] arm64: dts: mediatek: mt7988: add syscon for watchdog, xfi-pll and ethwarp

[Frank Wunderlich]

From: Frank Wunderlich <frank-w@public-files.de>

This is needed by u-boot-driver when using OF_UPSTREAM.

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 arch/arm64/boot/dts/mediatek/mt7988a.dtsi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
index 9ced005b1595..abde2719c34d 100644
--- a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
@@ -93,7 +93,7 @@ topckgen: clock-controller@1001b000 {
 		};
 
 		watchdog: watchdog@1001c000 {
-			compatible = "mediatek,mt7988-wdt";
+			compatible = "mediatek,mt7988-wdt", "syscon";
 			reg = <0 0x1001c000 0 0x1000>;
 			interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
 			#reset-cells = <1>;
@@ -192,7 +192,7 @@ ssusb1: usb@11200000 {
 		};
 
 		xfi_pll: clock-controller@11f40000 {
-			compatible = "mediatek,mt7988-xfi-pll";
+			compatible = "mediatek,mt7988-xfi-pll", "syscon";
 			reg = <0 0x11f40000 0 0x1000>;
 			resets = <&watchdog 16>;
 			#clock-cells = <1>;
@@ -206,7 +206,7 @@ ethsys: clock-controller@15000000 {
 		};
 
 		ethwarp: clock-controller@15031000 {
-			compatible = "mediatek,mt7988-ethwarp";
+			compatible = "mediatek,mt7988-ethwarp", "syscon";
 			reg = <0 0x15031000 0 0x1000>;
 			#clock-cells = <1>;
 			#reset-cells = <1>;
-- 
2.43.0

Re: [PATCH v1 0/4] add syscon requirement for mt7988

[AngeloGioacchino Del Regno]

Il 09/07/24 12:13, Frank Wunderlich ha scritto:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Some nodes require the syscon fallback at least in u-boot when using
> OF_UPSTREAM.
> 
> This is because uboot driver uses syscon_node_to_regmap in mtk_eth.c for
> "mediatek,toprgu", "mediatek,xfi_pll" and reset pointing to watchdog-node.
> 

I wonder what's the major blocker here to modify the u-boot driver to take
the upstream devicetree as-is, instead of using syscon_node_to_regmap?

Regards,
Angelo

> Frank Wunderlich (4):
>    dt-bindings: watchdog: mediatek,mtk-wdt: add MT7988 syscon requirement
>    dt-bindings: clock: mediatek: add syscon requirement for mt7988
>      xfi-pll
>    dt-bindings: clock: mediatek: add syscon requirement for mt7988
>      ethwarp
>    arm64: dts: mediatek: mt7988: add syscon for watchdog, xfi-pll and
>      ethwarp
> 
>   .../devicetree/bindings/clock/mediatek,mt7988-ethwarp.yaml | 6 ++++--
>   .../devicetree/bindings/clock/mediatek,mt7988-xfi-pll.yaml | 7 +++++--
>   .../devicetree/bindings/watchdog/mediatek,mtk-wdt.yaml     | 5 ++++-
>   arch/arm64/boot/dts/mediatek/mt7988a.dtsi                  | 6 +++---
>   4 files changed, 16 insertions(+), 8 deletions(-)
> 

Aw: Re: [PATCH v1 0/4] add syscon requirement for mt7988

[Frank Wunderlich]

Hi

> Gesendet: Mittwoch, 10. Juli 2024 um 12:45 Uhr
> Von: "AngeloGioacchino Del Regno" <angelogioacchino.delregno@collabora.com>
> Betreff: Re: [PATCH v1 0/4] add syscon requirement for mt7988
>
> Il 09/07/24 12:13, Frank Wunderlich ha scritto:
> > From: Frank Wunderlich <frank-w@public-files.de>
> >
> > Some nodes require the syscon fallback at least in u-boot when using
> > OF_UPSTREAM.
> >
> > This is because uboot driver uses syscon_node_to_regmap in mtk_eth.c for
> > "mediatek,toprgu", "mediatek,xfi_pll" and reset pointing to watchdog-node.
> >
>
> I wonder what's the major blocker here to modify the u-boot driver to take
> the upstream devicetree as-is, instead of using syscon_node_to_regmap?

in uboot there is no driver for all syscon and to handle parallel access this is done with the syscon fallback.

The syscon uclass is a small driver which is generic and only handle the regmap in global context.

In theory it could be possible that regmap is aquired twice when used from 2+ other drivers...to prevent this without
adding the syscon fallback each syscon needs a dedicated driver like in linux which does only syscon stuff (code
duplication at its best :) ).

of course i can use regmap_init_mem in the uboot ethernet driver

https://elixir.bootlin.com/u-boot/latest/source/drivers/core/regmap.c#L242

like it's done once for syscon-uclass.

but i will cause issues when a second device tries to access this regmap. So it was be much easier (for me) to add this
fallback and not writing 3 device-drivers in uboot doing the exactly same as syscon.

if you have a better idea how to handle it, let me know :)

regards Frank

> Regards,
> Angelo

Re: Aw: Re: [PATCH v1 0/4] add syscon requirement for mt7988

[AngeloGioacchino Del Regno]

Il 10/07/24 13:34, Frank Wunderlich ha scritto:
> Hi
> 
>> Gesendet: Mittwoch, 10. Juli 2024 um 12:45 Uhr
>> Von: "AngeloGioacchino Del Regno" <angelogioacchino.delregno@collabora.com>
>> Betreff: Re: [PATCH v1 0/4] add syscon requirement for mt7988
>>
>> Il 09/07/24 12:13, Frank Wunderlich ha scritto:
>>> From: Frank Wunderlich <frank-w@public-files.de>
>>>
>>> Some nodes require the syscon fallback at least in u-boot when using
>>> OF_UPSTREAM.
>>>
>>> This is because uboot driver uses syscon_node_to_regmap in mtk_eth.c for
>>> "mediatek,toprgu", "mediatek,xfi_pll" and reset pointing to watchdog-node.
>>>
>>
>> I wonder what's the major blocker here to modify the u-boot driver to take
>> the upstream devicetree as-is, instead of using syscon_node_to_regmap?
> 
> in uboot there is no driver for all syscon and to handle parallel access this is done with the syscon fallback.
> 
> The syscon uclass is a small driver which is generic and only handle the regmap in global context.
> 
> In theory it could be possible that regmap is aquired twice when used from 2+ other drivers...to prevent this without
> adding the syscon fallback each syscon needs a dedicated driver like in linux which does only syscon stuff (code
> duplication at its best :) ).
> 
> of course i can use regmap_init_mem in the uboot ethernet driver
> 
> https://elixir.bootlin.com/u-boot/latest/source/drivers/core/regmap.c#L242
> 
> like it's done once for syscon-uclass.
> 
> but i will cause issues when a second device tries to access this regmap. So it was be much easier (for me) to add this
> fallback and not writing 3 device-drivers in uboot doing the exactly same as syscon.
> 
> if you have a better idea how to handle it, let me know :)
> 

I see. The problem is that, from your description, it looks like u-boot
uses that as a kind of workaround for concurrent access to MMIO...

...looks like a good topic to discuss in the u-boot mailing lists.

Definitely, the TOPRGU and the XFI PLL are not system controllers, so the actual
"syscon" definition would be wrong for these, that's it.

Cheers

> regards Frank
> 
>> Regards,
>> Angelo
> 

Re: Aw: Re: [PATCH v1 0/4] add syscon requirement for mt7988

[Rob Herring]

On Wed, Jul 10, 2024 at 02:50:42PM +0200, AngeloGioacchino Del Regno wrote:
> Il 10/07/24 13:34, Frank Wunderlich ha scritto:
> > Hi
> > 
> > > Gesendet: Mittwoch, 10. Juli 2024 um 12:45 Uhr
> > > Von: "AngeloGioacchino Del Regno" <angelogioacchino.delregno@collabora.com>
> > > Betreff: Re: [PATCH v1 0/4] add syscon requirement for mt7988
> > > 
> > > Il 09/07/24 12:13, Frank Wunderlich ha scritto:
> > > > From: Frank Wunderlich <frank-w@public-files.de>
> > > > 
> > > > Some nodes require the syscon fallback at least in u-boot when using
> > > > OF_UPSTREAM.
> > > > 
> > > > This is because uboot driver uses syscon_node_to_regmap in mtk_eth.c for
> > > > "mediatek,toprgu", "mediatek,xfi_pll" and reset pointing to watchdog-node.
> > > > 
> > > 
> > > I wonder what's the major blocker here to modify the u-boot driver to take
> > > the upstream devicetree as-is, instead of using syscon_node_to_regmap?
> > 
> > in uboot there is no driver for all syscon and to handle parallel 
> > access this is done with the syscon fallback.
> > 
> > The syscon uclass is a small driver which is generic and only 
> > handle the regmap in global context.
> > 
> > In theory it could be possible that regmap is aquired twice when 
> > used from 2+ other drivers...to prevent this without
> > adding the syscon fallback each syscon needs a dedicated driver 
> > like in linux which does only syscon stuff (code
> > duplication at its best :) ).
> > 
> > of course i can use regmap_init_mem in the uboot ethernet driver
> > 
> > https://elixir.bootlin.com/u-boot/latest/source/drivers/core/regmap.c#L242
> > 
> > like it's done once for syscon-uclass.
> > 
> > but i will cause issues when a second device tries to access this 
> > regmap. So it was be much easier (for me) to add this
> > fallback and not writing 3 device-drivers in uboot doing the 
> > exactly same as syscon.
> > 
> > if you have a better idea how to handle it, let me know :)
> > 
> 
> I see. The problem is that, from your description, it looks like u-boot
> uses that as a kind of workaround for concurrent access to MMIO...
> 
> ...looks like a good topic to discuss in the u-boot mailing lists.
> 
> Definitely, the TOPRGU and the XFI PLL are not system controllers, so the actual
> "syscon" definition would be wrong for these, that's it.

While I'd prefer "syscon" never existed in the first place, I don't care 
too much if it gets added here or not. U-boot's reasoning for wanting it 
isn't really much better or worse than Linux's. Though if u-boot has 
multiple drivers using it, seems like an abstraction is missing if Linux 
doesn't need that.

Rob