[PATCH RFC v3 0/6] phy: mvebu-cp110-utmi: add support for armada-380 utmi phys

[PATCH RFC v3 0/6] phy: mvebu-cp110-utmi: add support for armada-380 utmi phys

[Josua Mayer]

Armada 380 has smilar USB-2.0 PHYs as CP-110 (Armada 8K).
    
Add support for Armada 380 to cp110 utmi phy driver, and enable it for
armada-388-clearfog boards.

Additionally add a small bugfix for armada-388 clearfog:
Enable Clearfog Base M.2 connector for cellular modems with USB-2.0/3.0
interface.
This is not separated out to avoid future merge conflicts.

Signed-off-by: Josua Mayer <josua@solid-run.com>
---
Changes in v3:
- updated bindings with additional comments, tested with dtbs_check:
  used anyOf for the newly-added optional regs
- added fix for clearfog base m.2 connector / enable third usb
- dropped unnecessary syscon node using invalid compatible
  (Reported-by: Krzysztof Kozlowski <krzk@kernel.org>)
- Link to v2: https://lore.kernel.org/r/20240716-a38x-utmi-phy-v2-0-dae3a9c6ca3e@solid-run.com

Changes in v2:
- add support for optional regs / make syscon use optional
- add device-tree changes for armada-388-clearfog
- attempted to fix warning reported by krobot (untested)
- tested on actual hardware
- drafted dt-bindings
- Link to v1: https://lore.kernel.org/r/20240715-a38x-utmi-phy-v1-0-d57250f53cf2@solid-run.com

---
Josua Mayer (6):
      arm: dts: marvell: armada-388-clearfog: enable third usb on m.2/mpcie
      arm: dts: marvell: armada-388-clearfog-base: add rfkill for m.2
      dt-bindings: phy: cp110-utmi-phy: add compatible string for armada-38x
      arm: dts: marvell: armada-38x: add description for usb phys
      phy: mvebu-cp110-utmi: add support for armada-380 utmi phys
      arm: dts: marvell: armada-388-clearfog: add description for usb phys

 .../phy/marvell,armada-cp110-utmi-phy.yaml         |  34 +++-
 .../boot/dts/marvell/armada-388-clearfog-base.dts  |  41 ++++
 arch/arm/boot/dts/marvell/armada-388-clearfog.dts  |   8 +
 arch/arm/boot/dts/marvell/armada-388-clearfog.dtsi |  30 ++-
 arch/arm/boot/dts/marvell/armada-38x.dtsi          |  24 +++
 drivers/phy/marvell/phy-mvebu-cp110-utmi.c         | 209 ++++++++++++++++-----
 6 files changed, 288 insertions(+), 58 deletions(-)
---
base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
change-id: 20240715-a38x-utmi-phy-02e8059afe35

Sincerely,
-- 
Josua Mayer <josua@solid-run.com>

[PATCH RFC v3 1/6] arm: dts: marvell: armada-388-clearfog: enable third usb on m.2/mpcie

[Josua Mayer]

Armada 388 Clearfog Pro has a USB-2.0 capable minipcie connector "CON2".
Clearfog Base has an M.2 connector combining USB-2.0 and USB-3.0 plus
various pins controlled by the host:

- FULL_CARD_POWER_OFF#: When low, M.2 LTE modules are switched off.
  Many modules include pull-down, thus it must be driven high actively.
- RESET#: Puts modules into reset when low. Modules are expected to
  include pull-up.
- GNSS_DISABLE#
- W_DISABLE#

Enable the usb controller node for the first combined usb-2.0/3.0
controller, for both clearfog base and pro.

To Clearfog base add gpio hogs for power-off and reset to ensure
modules are operational by default.

Cc: stable@vger.kernel.org
Signed-off-by: Josua Mayer <josua@solid-run.com>
---
 .../boot/dts/marvell/armada-388-clearfog-base.dts   | 21 +++++++++++++++++++++
 arch/arm/boot/dts/marvell/armada-388-clearfog.dts   |  5 +++++
 2 files changed, 26 insertions(+)

diff --git a/arch/arm/boot/dts/marvell/armada-388-clearfog-base.dts b/arch/arm/boot/dts/marvell/armada-388-clearfog-base.dts
index f7daa3bc707e..03153186c7bb 100644
--- a/arch/arm/boot/dts/marvell/armada-388-clearfog-base.dts
+++ b/arch/arm/boot/dts/marvell/armada-388-clearfog-base.dts
@@ -33,6 +33,22 @@ &eth1 {
 	phy = <&phy1>;
 };
 
+&expander0 {
+	m2-full-card-power-off-hog {
+		gpio-hog;
+		gpios = <2 GPIO_ACTIVE_LOW>;
+		output-low;
+		line-name = "m2-full-card-power-off";
+	};
+
+	m2-reset-hog {
+		gpio-hog;
+		gpios = <10 GPIO_ACTIVE_LOW>;
+		output-low;
+		line-name = "m2-reset";
+	};
+};
+
 &gpio0 {
 	phy1_reset {
 		gpio-hog;
@@ -66,3 +82,8 @@ rear_button_pins: rear-button-pins {
 		marvell,function = "gpio";
 	};
 };
+
+/* SRDS #4 - USB-2.0/3.0 Host, M.2 */
+&usb3_0 {
+	status = "okay";
+};
diff --git a/arch/arm/boot/dts/marvell/armada-388-clearfog.dts b/arch/arm/boot/dts/marvell/armada-388-clearfog.dts
index 09bf2e6d4ed0..d6d7cc885f4d 100644
--- a/arch/arm/boot/dts/marvell/armada-388-clearfog.dts
+++ b/arch/arm/boot/dts/marvell/armada-388-clearfog.dts
@@ -182,3 +182,8 @@ &spi1 {
 	 */
 	pinctrl-0 = <&spi1_pins &clearfog_spi1_cs_pins &mikro_spi_pins>;
 };
+
+/* USB-2.0 Host, CON2 - nearest CPU */
+&usb3_0 {
+	status = "okay";
+};

-- 
2.43.0

[PATCH RFC v3 2/6] arm: dts: marvell: armada-388-clearfog-base: add rfkill for m.2

[Josua Mayer]

Armada 388 Clearfog Base has a USB-3.0 / SATA capable m.2 connector,
with various pins controlled by the host:

- FULL_CARD_POWER_OFF#: When low, M.2 LTE modules are switched off.
  Many modules include pull-down, thus it must be driven high actively.
- RESET#: Puts modules into reset when low. Modules are expected to
  include pull-up.
- GNSS_DISABLE#
- W_DISABLE#

Add rfkill devices for gnss and wwan.

Signed-off-by: Josua Mayer <josua@solid-run.com>
---
 arch/arm/boot/dts/marvell/armada-388-clearfog-base.dts | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm/boot/dts/marvell/armada-388-clearfog-base.dts b/arch/arm/boot/dts/marvell/armada-388-clearfog-base.dts
index 03153186c7bb..308ad9d1c70f 100644
--- a/arch/arm/boot/dts/marvell/armada-388-clearfog-base.dts
+++ b/arch/arm/boot/dts/marvell/armada-388-clearfog-base.dts
@@ -27,6 +27,23 @@ button-0 {
 			linux,code = <BTN_0>;
 		};
 	};
+
+	rfkill-m2-gnss {
+		compatible = "rfkill-gpio";
+		label = "m.2 GNSS";
+		radio-type = "gps";
+		/* rfkill-gpio inverts internally */
+		shutdown-gpios = <&expander0 9 GPIO_ACTIVE_HIGH>;
+	};
+
+	/* M.2 is B-keyed, so w-disable is for WWAN */
+	rfkill-m2-wwan {
+		compatible = "rfkill-gpio";
+		label = "m.2 WWAN";
+		radio-type = "wwan";
+		/* rfkill-gpio inverts internally */
+		shutdown-gpios = <&expander0 8 GPIO_ACTIVE_HIGH>;
+	};
 };
 
 &eth1 {

-- 
2.43.0

[PATCH RFC v3 3/6] dt-bindings: phy: cp110-utmi-phy: add compatible string for armada-38x

[Josua Mayer]

Armada 38x USB-2.0 PHYs are similar to Armada 8K (CP110) and can be
supported by the same driver with small differences.

Add new compatible string for armada-38x variant of utmi phy.
Then add descriptions and names for two additional register definitions
that may be specified instead of a syscon phandle.

Signed-off-by: Josua Mayer <josua@solid-run.com>
---
 .../phy/marvell,armada-cp110-utmi-phy.yaml         | 34 ++++++++++++++++++----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml b/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml
index 9ce7b4c6d208..246e48d51755 100644
--- a/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml
@@ -23,12 +23,36 @@ description:
   UTMI PHY1  --------\
                       1.H----- USB HOST1
 
+  On Armada 380 there is an additional USB-2.0-only controller,
+  and an additional UTMI PHY respectively.
+  The USB device controller can only be connected to a single UTMI PHY port,
+  either UTMI PHY0 or UTMI PHY2.
+
+
+
 properties:
   compatible:
-    const: marvell,cp110-utmi-phy
+    enum:
+      - marvell,a38x-utmi-phy
+      - marvell,cp110-utmi-phy
 
   reg:
-    maxItems: 1
+    anyOf:
+      - items:
+          - description: UTMI registers
+      - items:
+          - description: UTMI registers
+          - description: USB config register
+          - description: UTMI config registers
+
+  reg-names:
+    anyOf:
+      - items:
+          - const: utmi
+      - items:
+          - const: utmi
+          - const: usb-cfg
+          - const: utmi-cfg
 
   "#address-cells":
     const: 1
@@ -38,13 +62,14 @@ properties:
 
   marvell,system-controller:
     description:
-      Phandle to the system controller node
+      Phandle to the system controller node.
+      Optional when usb-cfg and utmi-cfg regs are given.
     $ref: /schemas/types.yaml#/definitions/phandle
 
 # Required child nodes:
 
 patternProperties:
-  "^usb-phy@[0|1]$":
+  "^usb-phy@[0|1|2]$":
     type: object
     description:
       Each UTMI PHY port must be represented as a sub-node.
@@ -68,7 +93,6 @@ required:
   - reg
   - "#address-cells"
   - "#size-cells"
-  - marvell,system-controller
 
 additionalProperties: false
 

-- 
2.43.0

Re: [PATCH RFC v3 3/6] dt-bindings: phy: cp110-utmi-phy: add compatible string for armada-38x

[Krzysztof Kozlowski]

On 20/07/2024 16:19, Josua Mayer wrote:
> Armada 38x USB-2.0 PHYs are similar to Armada 8K (CP110) and can be
> supported by the same driver with small differences.
> 
> Add new compatible string for armada-38x variant of utmi phy.
> Then add descriptions and names for two additional register definitions
> that may be specified instead of a syscon phandle.
> 
> Signed-off-by: Josua Mayer <josua@solid-run.com>
> ---
>  .../phy/marvell,armada-cp110-utmi-phy.yaml         | 34 ++++++++++++++++++----
>  1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml b/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml
> index 9ce7b4c6d208..246e48d51755 100644
> --- a/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml
> @@ -23,12 +23,36 @@ description:
>    UTMI PHY1  --------\
>                        1.H----- USB HOST1
>  
> +  On Armada 380 there is an additional USB-2.0-only controller,
> +  and an additional UTMI PHY respectively.
> +  The USB device controller can only be connected to a single UTMI PHY port,
> +  either UTMI PHY0 or UTMI PHY2.
> +
> +
> +

One blank line is enough.

>  properties:
>    compatible:
> -    const: marvell,cp110-utmi-phy
> +    enum:
> +      - marvell,a38x-utmi-phy
> +      - marvell,cp110-utmi-phy
>  
>    reg:
> -    maxItems: 1
> +    anyOf:

That's oneOf.

> +      - items:
> +          - description: UTMI registers
> +      - items:
> +          - description: UTMI registers
> +          - description: USB config register
> +          - description: UTMI config registers
> +
> +  reg-names:
> +    anyOf:

oneOf

> +      - items:
> +          - const: utmi
> +      - items:
> +          - const: utmi
> +          - const: usb-cfg
> +          - const: utmi-cfg
>  
>    "#address-cells":
>      const: 1
> @@ -38,13 +62,14 @@ properties:
>  
>    marvell,system-controller:
>      description:
> -      Phandle to the system controller node
> +      Phandle to the system controller node.
> +      Optional when usb-cfg and utmi-cfg regs are given.
>      $ref: /schemas/types.yaml#/definitions/phandle
>  
>  # Required child nodes:
>  
>  patternProperties:
> -  "^usb-phy@[0|1]$":
> +  "^usb-phy@[0|1|2]$":

[0-2]

>      type: object
>      description:
>        Each UTMI PHY port must be represented as a sub-node.
> @@ -68,7 +93,6 @@ required:
>    - reg
>    - "#address-cells"
>    - "#size-cells"
> -  - marvell,system-controller

you miss here allOf:if:then: narrowing and marvell,system-controller per
each variant.



Best regards,
Krzysztof

Re: [PATCH RFC v3 3/6] dt-bindings: phy: cp110-utmi-phy: add compatible string for armada-38x

[Josua Mayer]

Am 21.07.24 um 11:31 schrieb Krzysztof Kozlowski:
> On 20/07/2024 16:19, Josua Mayer wrote:
>> Armada 38x USB-2.0 PHYs are similar to Armada 8K (CP110) and can be
>> supported by the same driver with small differences.
>>
>> Add new compatible string for armada-38x variant of utmi phy.
>> Then add descriptions and names for two additional register definitions
>> that may be specified instead of a syscon phandle.
>>
>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>> ---
>>  .../phy/marvell,armada-cp110-utmi-phy.yaml         | 34 ++++++++++++++++++----
>>  1 file changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml b/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml
>> index 9ce7b4c6d208..246e48d51755 100644
>> --- a/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml
>> +++ b/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml
>> @@ -23,12 +23,36 @@ description:
>>    UTMI PHY1  --------\
>>                        1.H----- USB HOST1
>>  
>> +  On Armada 380 there is an additional USB-2.0-only controller,
>> +  and an additional UTMI PHY respectively.
>> +  The USB device controller can only be connected to a single UTMI PHY port,
>> +  either UTMI PHY0 or UTMI PHY2.
>> +
>> +
>> +
> One blank line is enough.
Ack.
>
>>  properties:
>>    compatible:
>> -    const: marvell,cp110-utmi-phy
>> +    enum:
>> +      - marvell,a38x-utmi-phy
>> +      - marvell,cp110-utmi-phy
>>  
>>    reg:
>> -    maxItems: 1
>> +    anyOf:
> That's oneOf.

Acknowledged, thanks!

Today oneOf seems correct to me, too.

>
>> +      - items:
>> +          - description: UTMI registers
>> +      - items:
>> +          - description: UTMI registers
>> +          - description: USB config register
>> +          - description: UTMI config registers
>> +
>> +  reg-names:
>> +    anyOf:
> oneOf
Ack.
>
>> +      - items:
>> +          - const: utmi
>> +      - items:
>> +          - const: utmi
>> +          - const: usb-cfg
>> +          - const: utmi-cfg
>>  
>>    "#address-cells":
>>      const: 1
>> @@ -38,13 +62,14 @@ properties:
>>  
>>    marvell,system-controller:
>>      description:
>> -      Phandle to the system controller node
>> +      Phandle to the system controller node.
>> +      Optional when usb-cfg and utmi-cfg regs are given.
>>      $ref: /schemas/types.yaml#/definitions/phandle
>>  
>>  # Required child nodes:
>>  
>>  patternProperties:
>> -  "^usb-phy@[0|1]$":
>> +  "^usb-phy@[0|1|2]$":
> [0-2]
Ack.
>
>>      type: object
>>      description:
>>        Each UTMI PHY port must be represented as a sub-node.
>> @@ -68,7 +93,6 @@ required:
>>    - reg
>>    - "#address-cells"
>>    - "#size-cells"
>> -  - marvell,system-controller
> you miss here allOf:if:then: narrowing and marvell,system-controller per
> each variant.
Correct.
I will learn how to do that, and included it a non-rfc version.


Thanks!

sincerely
Josua Mayer

Re: [PATCH RFC v3 3/6] dt-bindings: phy: cp110-utmi-phy: add compatible string for armada-38x

[Josua Mayer]

Am 22.07.24 um 17:05 schrieb Josua Mayer:
> Am 21.07.24 um 11:31 schrieb Krzysztof Kozlowski:
>> On 20/07/2024 16:19, Josua Mayer wrote:
>>> Armada 38x USB-2.0 PHYs are similar to Armada 8K (CP110) and can be
>>> supported by the same driver with small differences.
>>>
>>> Add new compatible string for armada-38x variant of utmi phy.
>>> Then add descriptions and names for two additional register definitions
>>> that may be specified instead of a syscon phandle.
>>>
>>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>>> ---
>>>  .../phy/marvell,armada-cp110-utmi-phy.yaml         | 34 ++++++++++++++++++----
>>>  1 file changed, 29 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml b/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml
>>> index 9ce7b4c6d208..246e48d51755 100644
>>> --- a/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml
>>> +++ b/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml
cut
>>> @@ -68,7 +93,6 @@ required:
>>>    - reg
>>>    - "#address-cells"
>>>    - "#size-cells"
>>> -  - marvell,system-controller
>> you miss here allOf:if:then: narrowing and marvell,system-controller per
>> each variant.
I am struggling a bit with the options.

First attempt says: if not both usb-cfg and utmi-cfg reg-names are specified,
then marvell,system-controller is required.

allOf:
  - required:
      - compatible
      - reg
      - "#address-cells"
      - "#size-cells"
  - if:
      not:
        properties:
          reg-names:
            allOf:
              - contains:
                  const: usb-cfg
              - contains:
                  const: utmi-cfg
    then:
      required:
        - marvell,system-controller

This works okay for any combinations of reg-names.

However when device-tree is missing reg-names all together,
marvell,system-controller is not marked required.

Would it be acceptable to make reg-names required?

Re: [PATCH RFC v3 3/6] dt-bindings: phy: cp110-utmi-phy: add compatible string for armada-38x

[Krzysztof Kozlowski]

On 22/07/2024 17:14, Josua Mayer wrote:
> 
> Am 22.07.24 um 17:05 schrieb Josua Mayer:
>> Am 21.07.24 um 11:31 schrieb Krzysztof Kozlowski:
>>> On 20/07/2024 16:19, Josua Mayer wrote:
>>>> Armada 38x USB-2.0 PHYs are similar to Armada 8K (CP110) and can be
>>>> supported by the same driver with small differences.
>>>>
>>>> Add new compatible string for armada-38x variant of utmi phy.
>>>> Then add descriptions and names for two additional register definitions
>>>> that may be specified instead of a syscon phandle.
>>>>
>>>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>>>> ---
>>>>  .../phy/marvell,armada-cp110-utmi-phy.yaml         | 34 ++++++++++++++++++----
>>>>  1 file changed, 29 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml b/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml
>>>> index 9ce7b4c6d208..246e48d51755 100644
>>>> --- a/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml
>>>> +++ b/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml
> cut
>>>> @@ -68,7 +93,6 @@ required:
>>>>    - reg
>>>>    - "#address-cells"
>>>>    - "#size-cells"
>>>> -  - marvell,system-controller
>>> you miss here allOf:if:then: narrowing and marvell,system-controller per
>>> each variant.
> I am struggling a bit with the options.
> 
> First attempt says: if not both usb-cfg and utmi-cfg reg-names are specified,
> then marvell,system-controller is required.
> 
> allOf:
>   - required:

That's not part of allOf.

>       - compatible
>       - reg
>       - "#address-cells"
>       - "#size-cells"
>   - if:
>       not:
>         properties:
>           reg-names:
>             allOf:
>               - contains:
>                   const: usb-cfg
>               - contains:
>                   const: utmi-cfg
>     then:
>       required:
>         - marvell,system-controller
> 
> This works okay for any combinations of reg-names.

??? I expected this to be per variant.

> 
> However when device-tree is missing reg-names all together,
> marvell,system-controller is not marked required.
> 
> Would it be acceptable to make reg-names required?

I don't understand what you want to achieve.

Best regards,
Krzysztof

Re: [PATCH RFC v3 3/6] dt-bindings: phy: cp110-utmi-phy: add compatible string for armada-38x

[Josua Mayer]

Am 22.07.24 um 17:17 schrieb Krzysztof Kozlowski:
> On 22/07/2024 17:14, Josua Mayer wrote:
>> Am 22.07.24 um 17:05 schrieb Josua Mayer:
>>> Am 21.07.24 um 11:31 schrieb Krzysztof Kozlowski:
>>>> On 20/07/2024 16:19, Josua Mayer wrote:
>>>>> Armada 38x USB-2.0 PHYs are similar to Armada 8K (CP110) and can be
>>>>> supported by the same driver with small differences.
>>>>>
>>>>> Add new compatible string for armada-38x variant of utmi phy.
>>>>> Then add descriptions and names for two additional register definitions
>>>>> that may be specified instead of a syscon phandle.
>>>>>
>>>>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>>>>> ---
>>>>>  .../phy/marvell,armada-cp110-utmi-phy.yaml         | 34 ++++++++++++++++++----
>>>>>  1 file changed, 29 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml b/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml
>>>>> index 9ce7b4c6d208..246e48d51755 100644
>>>>> --- a/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml
>>>>> +++ b/Documentation/devicetree/bindings/phy/marvell,armada-cp110-utmi-phy.yaml
>> cut
>>>>> @@ -68,7 +93,6 @@ required:
>>>>>    - reg
>>>>>    - "#address-cells"
>>>>>    - "#size-cells"
>>>>> -  - marvell,system-controller
>>>> you miss here allOf:if:then: narrowing and marvell,system-controller per
>>>> each variant.
>> I am struggling a bit with the options.
>>
>> First attempt says: if not both usb-cfg and utmi-cfg reg-names are specified,
>> then marvell,system-controller is required.
>>
>> allOf:
>>   - required:
> That's not part of allOf.
>
>>       - compatible
>>       - reg
>>       - "#address-cells"
>>       - "#size-cells"
>>   - if:
>>       not:
>>         properties:
>>           reg-names:
>>             allOf:
>>               - contains:
>>                   const: usb-cfg
>>               - contains:
>>                   const: utmi-cfg
>>     then:
>>       required:
>>         - marvell,system-controller
>>
>> This works okay for any combinations of reg-names.
> ??? I expected this to be per variant.
As in by compatible string?
>
>> However when device-tree is missing reg-names all together,
>> marvell,system-controller is not marked required.
>>
>> Would it be acceptable to make reg-names required?
> I don't understand what you want to achieve.
When there are both usb-cfg and utmi-cfg regs,
then marvell,system-controller is optional,

regardless of armada 380 or 8k.

sincerely
Josua Mayer

Re: [PATCH RFC v3 3/6] dt-bindings: phy: cp110-utmi-phy: add compatible string for armada-38x

[Krzysztof Kozlowski]

On 22/07/2024 17:31, Josua Mayer wrote:
>>
>>>       - compatible
>>>       - reg
>>>       - "#address-cells"
>>>       - "#size-cells"
>>>   - if:
>>>       not:
>>>         properties:
>>>           reg-names:
>>>             allOf:
>>>               - contains:
>>>                   const: usb-cfg
>>>               - contains:
>>>                   const: utmi-cfg
>>>     then:
>>>       required:
>>>         - marvell,system-controller
>>>
>>> This works okay for any combinations of reg-names.
>> ??? I expected this to be per variant.
> As in by compatible string?

Yes, each device has fixed properties, at least usually.

>>
>>> However when device-tree is missing reg-names all together,
>>> marvell,system-controller is not marked required.
>>>
>>> Would it be acceptable to make reg-names required?
>> I don't understand what you want to achieve.
> When there are both usb-cfg and utmi-cfg regs,
> then marvell,system-controller is optional,
> 
> regardless of armada 380 or 8k.

Whether the device has additional MMIO address space, depends on type of
the device, not on some other properties. IOW, either you have here
second reg or not. The hardware has or has not.


Best regards,
Krzysztof

Re: [PATCH RFC v3 3/6] dt-bindings: phy: cp110-utmi-phy: add compatible string for armada-38x

[Josua Mayer]

Am 22.07.24 um 17:45 schrieb Krzysztof Kozlowski:
> On 22/07/2024 17:31, Josua Mayer wrote:
>>>>       - compatible
>>>>       - reg
>>>>       - "#address-cells"
>>>>       - "#size-cells"
>>>>   - if:
>>>>       not:
>>>>         properties:
>>>>           reg-names:
>>>>             allOf:
>>>>               - contains:
>>>>                   const: usb-cfg
>>>>               - contains:
>>>>                   const: utmi-cfg
>>>>     then:
>>>>       required:
>>>>         - marvell,system-controller
>>>>
>>>> This works okay for any combinations of reg-names.
>>> ??? I expected this to be per variant.
>> As in by compatible string?
> Yes, each device has fixed properties, at least usually.
>
>>>> However when device-tree is missing reg-names all together,
>>>> marvell,system-controller is not marked required.
>>>>
>>>> Would it be acceptable to make reg-names required?
>>> I don't understand what you want to achieve.
>> When there are both usb-cfg and utmi-cfg regs,
>> then marvell,system-controller is optional,
>>
>> regardless of armada 380 or 8k.
> Whether the device has additional MMIO address space, depends on type of
> the device, not on some other properties. IOW, either you have here
> second reg or not. The hardware has or has not.

At least Armada 8K and Armada 388 both have them (physically).

The difference is one of them uses syscon for access.

Consequently, would it be better then to always require all 3?

(In driver I could use lack of syscon handle to decide,
currently I use presence of regs by name)

sincerely
Josua Mayer

[PATCH RFC v3 4/6] arm: dts: marvell: armada-38x: add description for usb phys

[Josua Mayer]

Armada 38x has 3x USB-2.0 utmi phys. They are almost identical to the 2x
utmi phys on armada 8k.

Add descriptions for all 3 phy ports.

Signed-off-by: Josua Mayer <josua@solid-run.com>
---
 arch/arm/boot/dts/marvell/armada-38x.dtsi | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm/boot/dts/marvell/armada-38x.dtsi b/arch/arm/boot/dts/marvell/armada-38x.dtsi
index 446861b6b17b..9c4b5a9817f4 100644
--- a/arch/arm/boot/dts/marvell/armada-38x.dtsi
+++ b/arch/arm/boot/dts/marvell/armada-38x.dtsi
@@ -580,6 +580,30 @@ ahci0: sata@a8000 {
 				status = "disabled";
 			};
 
+			utmi: utmi@c0000 {
+				compatible = "marvell,a38x-utmi-phy";
+				reg = <0xc0000 0x6000>, <0x18420 4>, <0x18440 12>;
+				reg-names = "utmi", "usb-cfg", "utmi-cfg";
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "disabled";
+
+				utmi0: usb-phy@0 {
+					reg = <0>;
+					#phy-cells = <0>;
+				};
+
+				utmi1: usb-phy@1 {
+					reg = <1>;
+					#phy-cells = <0>;
+				};
+
+				utmi2: usb-phy@2 {
+					reg = <2>;
+					#phy-cells = <0>;
+				};
+			};
+
 			bm: bm@c8000 {
 				compatible = "marvell,armada-380-neta-bm";
 				reg = <0xc8000 0xac>;

-- 
2.43.0

[PATCH RFC v3 5/6] phy: mvebu-cp110-utmi: add support for armada-380 utmi phys

[Josua Mayer]

Armada 380 has similar USB-2.0 PHYs as CP-110. The differences are:
- register base addresses
- gap between port registers
- number of ports: 388 has three, cp110 two
- device-mode mux bit refers to different ports
- syscon register's base address (offsets identical)
- armada-8k uses syscon for various drivers, a38x not

Differentiation uses of_match_data with distinct compatible strings.

Add support for Armada 380 PHYs by partially restructuting the driver:
- Port register pointers are moved to the per-port private data.
- Add armada-38x-specific compatible string and store enum value in
  of_match_data for differentiation.
- Add support for optional regs usb-cfg and utmi-cfg, to be used instead
  of syscon.

Signed-off-by: Josua Mayer <josua@solid-run.com>
---
 drivers/phy/marvell/phy-mvebu-cp110-utmi.c | 209 +++++++++++++++++++++++------
 1 file changed, 166 insertions(+), 43 deletions(-)

diff --git a/drivers/phy/marvell/phy-mvebu-cp110-utmi.c b/drivers/phy/marvell/phy-mvebu-cp110-utmi.c
index 4922a5f3327d..4341923e85bc 100644
--- a/drivers/phy/marvell/phy-mvebu-cp110-utmi.c
+++ b/drivers/phy/marvell/phy-mvebu-cp110-utmi.c
@@ -19,7 +19,7 @@
 #include <linux/usb/of.h>
 #include <linux/usb/otg.h>
 
-#define UTMI_PHY_PORTS				2
+#define UTMI_PHY_PORTS				3
 
 /* CP110 UTMI register macro definetions */
 #define SYSCON_USB_CFG_REG			0x420
@@ -76,32 +76,44 @@
 #define PLL_LOCK_DELAY_US			10000
 #define PLL_LOCK_TIMEOUT_US			1000000
 
-#define PORT_REGS(p)				((p)->priv->regs + (p)->id * 0x1000)
+enum mvebu_cp110_utmi_type {
+	/* 0 is reserved to avoid clashing with NULL */
+	A380_UTMI = 1,
+	CP110_UTMI = 2,
+};
+
+struct mvebu_cp110_utmi_port;
 
 /**
  * struct mvebu_cp110_utmi - PHY driver data
  *
- * @regs: PHY registers
+ * @regs_usb: USB configuration register
  * @syscon: Regmap with system controller registers
  * @dev: device driver handle
  * @ops: phy ops
+ * @ports: phy object for each port
  */
 struct mvebu_cp110_utmi {
-	void __iomem *regs;
+	void __iomem *regs_usb;
 	struct regmap *syscon;
 	struct device *dev;
 	const struct phy_ops *ops;
+	struct mvebu_cp110_utmi_port *ports[UTMI_PHY_PORTS];
 };
 
 /**
  * struct mvebu_cp110_utmi_port - PHY port data
  *
+ * @regs: PHY registers
+ * @regs_cfg: PHY config register
  * @priv: PHY driver data
  * @id: PHY port ID
  * @dr_mode: PHY connection: USB_DR_MODE_HOST or USB_DR_MODE_PERIPHERAL
  */
 struct mvebu_cp110_utmi_port {
 	struct mvebu_cp110_utmi *priv;
+	void __iomem *regs;
+	void __iomem *regs_cfg;
 	u32 id;
 	enum usb_dr_mode dr_mode;
 };
@@ -118,47 +130,47 @@ static void mvebu_cp110_utmi_port_setup(struct mvebu_cp110_utmi_port *port)
 	 * The crystal used for all platform boards is now 25MHz.
 	 * See the functional specification for details.
 	 */
-	reg = readl(PORT_REGS(port) + UTMI_PLL_CTRL_REG);
+	reg = readl(port->regs + UTMI_PLL_CTRL_REG);
 	reg &= ~(PLL_REFDIV_MASK | PLL_FBDIV_MASK | PLL_SEL_LPFR_MASK);
 	reg |= (PLL_REFDIV_VAL << PLL_REFDIV_OFFSET) |
 	       (PLL_FBDIV_VAL << PLL_FBDIV_OFFSET);
-	writel(reg, PORT_REGS(port) + UTMI_PLL_CTRL_REG);
+	writel(reg, port->regs + UTMI_PLL_CTRL_REG);
 
 	/* Impedance Calibration Threshold Setting */
-	reg = readl(PORT_REGS(port) + UTMI_CAL_CTRL_REG);
+	reg = readl(port->regs + UTMI_CAL_CTRL_REG);
 	reg &= ~IMPCAL_VTH_MASK;
 	reg |= IMPCAL_VTH_VAL << IMPCAL_VTH_OFFSET;
-	writel(reg, PORT_REGS(port) + UTMI_CAL_CTRL_REG);
+	writel(reg, port->regs + UTMI_CAL_CTRL_REG);
 
 	/* Set LS TX driver strength coarse control */
-	reg = readl(PORT_REGS(port) + UTMI_TX_CH_CTRL_REG);
+	reg = readl(port->regs + UTMI_TX_CH_CTRL_REG);
 	reg &= ~TX_AMP_MASK;
 	reg |= TX_AMP_VAL << TX_AMP_OFFSET;
-	writel(reg, PORT_REGS(port) + UTMI_TX_CH_CTRL_REG);
+	writel(reg, port->regs + UTMI_TX_CH_CTRL_REG);
 
 	/* Disable SQ and enable analog squelch detect */
-	reg = readl(PORT_REGS(port) + UTMI_RX_CH_CTRL0_REG);
+	reg = readl(port->regs + UTMI_RX_CH_CTRL0_REG);
 	reg &= ~SQ_DET_EN;
 	reg |= SQ_ANA_DTC_SEL;
-	writel(reg, PORT_REGS(port) + UTMI_RX_CH_CTRL0_REG);
+	writel(reg, port->regs + UTMI_RX_CH_CTRL0_REG);
 
 	/*
 	 * Set External squelch calibration number and
 	 * enable the External squelch calibration
 	 */
-	reg = readl(PORT_REGS(port) + UTMI_RX_CH_CTRL1_REG);
+	reg = readl(port->regs + UTMI_RX_CH_CTRL1_REG);
 	reg &= ~SQ_AMP_CAL_MASK;
 	reg |= (SQ_AMP_CAL_VAL << SQ_AMP_CAL_OFFSET) | SQ_AMP_CAL_EN;
-	writel(reg, PORT_REGS(port) + UTMI_RX_CH_CTRL1_REG);
+	writel(reg, port->regs + UTMI_RX_CH_CTRL1_REG);
 
 	/*
 	 * Set Control VDAT Reference Voltage - 0.325V and
 	 * Control VSRC Reference Voltage - 0.6V
 	 */
-	reg = readl(PORT_REGS(port) + UTMI_CHGDTC_CTRL_REG);
+	reg = readl(port->regs + UTMI_CHGDTC_CTRL_REG);
 	reg &= ~(VDAT_MASK | VSRC_MASK);
 	reg |= (VDAT_VAL << VDAT_OFFSET) | (VSRC_VAL << VSRC_OFFSET);
-	writel(reg, PORT_REGS(port) + UTMI_CHGDTC_CTRL_REG);
+	writel(reg, port->regs + UTMI_CHGDTC_CTRL_REG);
 }
 
 static int mvebu_cp110_utmi_phy_power_off(struct phy *phy)
@@ -166,22 +178,38 @@ static int mvebu_cp110_utmi_phy_power_off(struct phy *phy)
 	struct mvebu_cp110_utmi_port *port = phy_get_drvdata(phy);
 	struct mvebu_cp110_utmi *utmi = port->priv;
 	int i;
+	int reg;
 
 	/* Power down UTMI PHY port */
-	regmap_clear_bits(utmi->syscon, SYSCON_UTMI_CFG_REG(port->id),
-			  UTMI_PHY_CFG_PU_MASK);
+	if (!IS_ERR(port->regs_cfg)) {
+		reg = readl(port->regs_cfg);
+		reg &= ~(UTMI_PHY_CFG_PU_MASK);
+		writel(reg, port->regs_cfg);
+	} else
+		regmap_clear_bits(utmi->syscon, SYSCON_UTMI_CFG_REG(port->id),
+				  UTMI_PHY_CFG_PU_MASK);
 
 	for (i = 0; i < UTMI_PHY_PORTS; i++) {
-		int test = regmap_test_bits(utmi->syscon,
-					    SYSCON_UTMI_CFG_REG(i),
-					    UTMI_PHY_CFG_PU_MASK);
+		if (!utmi->ports[i])
+			continue;
+
+		if (!IS_ERR(utmi->ports[i]->regs_cfg))
+			reg = readl(utmi->ports[i]->regs_cfg);
+		else
+			regmap_read(utmi->syscon, SYSCON_UTMI_CFG_REG(i), &reg);
+		int test = reg & UTMI_PHY_CFG_PU_MASK;
 		/* skip PLL shutdown if there are active UTMI PHY ports */
 		if (test != 0)
 			return 0;
 	}
 
 	/* PLL Power down if all UTMI PHYs are down */
-	regmap_clear_bits(utmi->syscon, SYSCON_USB_CFG_REG, USB_CFG_PLL_MASK);
+	if (!IS_ERR(utmi->regs_usb)) {
+		reg = readl(utmi->regs_usb);
+		reg &= ~(USB_CFG_PLL_MASK);
+		writel(reg, utmi->regs_usb);
+	} else
+		regmap_clear_bits(utmi->syscon, SYSCON_USB_CFG_REG, USB_CFG_PLL_MASK);
 
 	return 0;
 }
@@ -191,8 +219,15 @@ static int mvebu_cp110_utmi_phy_power_on(struct phy *phy)
 	struct mvebu_cp110_utmi_port *port = phy_get_drvdata(phy);
 	struct mvebu_cp110_utmi *utmi = port->priv;
 	struct device *dev = &phy->dev;
+	const void *match;
+	enum mvebu_cp110_utmi_type type;
 	int ret;
 	u32 reg;
+	u32 sel;
+
+	match = device_get_match_data(utmi->dev);
+	if (match)
+		type = (enum mvebu_cp110_utmi_type)(uintptr_t)match;
 
 	/* It is necessary to power off UTMI before configuration */
 	ret = mvebu_cp110_utmi_phy_power_off(phy);
@@ -208,16 +243,45 @@ static int mvebu_cp110_utmi_phy_power_on(struct phy *phy)
 	 * to UTMI0 or to UTMI1 PHY port, but not to both.
 	 */
 	if (port->dr_mode == USB_DR_MODE_PERIPHERAL) {
-		regmap_update_bits(utmi->syscon, SYSCON_USB_CFG_REG,
-				   USB_CFG_DEVICE_EN_MASK | USB_CFG_DEVICE_MUX_MASK,
-				   USB_CFG_DEVICE_EN_MASK |
-				   (port->id << USB_CFG_DEVICE_MUX_OFFSET));
+		switch (type) {
+		case A380_UTMI:
+			/*
+			 * A380 muxes between ports 0/2:
+			 * - 0: Device mode on Port 2
+			 * - 1: Device mode on Port 0
+			 */
+			if (port->id == 1)
+				return -EINVAL;
+			sel = !!(port->id == 0);
+			break;
+		case CP110_UTMI:
+			/*
+			 * CP110 muxes between ports 0/1:
+			 * - 0: Device mode on Port 0
+			 * - 1: Device mode on Port 1
+			 */
+			sel = port->id;
+			break;
+		default:
+			return -EINVAL;
+		}
+		if (!IS_ERR(utmi->regs_usb)) {
+			reg = readl(utmi->regs_usb);
+			reg &= ~(USB_CFG_DEVICE_EN_MASK | USB_CFG_DEVICE_MUX_MASK);
+			reg |= USB_CFG_DEVICE_EN_MASK;
+			reg |= (sel << USB_CFG_DEVICE_MUX_OFFSET);
+			writel(reg, utmi->regs_usb);
+		} else
+			regmap_update_bits(utmi->syscon, SYSCON_USB_CFG_REG,
+					   USB_CFG_DEVICE_EN_MASK | USB_CFG_DEVICE_MUX_MASK,
+					   USB_CFG_DEVICE_EN_MASK |
+					   (sel << USB_CFG_DEVICE_MUX_OFFSET));
 	}
 
 	/* Set Test suspendm mode and enable Test UTMI select */
-	reg = readl(PORT_REGS(port) + UTMI_CTRL_STATUS0_REG);
+	reg = readl(port->regs + UTMI_CTRL_STATUS0_REG);
 	reg |= SUSPENDM | TEST_SEL;
-	writel(reg, PORT_REGS(port) + UTMI_CTRL_STATUS0_REG);
+	writel(reg, port->regs + UTMI_CTRL_STATUS0_REG);
 
 	/* Wait for UTMI power down */
 	mdelay(1);
@@ -226,16 +290,21 @@ static int mvebu_cp110_utmi_phy_power_on(struct phy *phy)
 	mvebu_cp110_utmi_port_setup(port);
 
 	/* Power UP UTMI PHY */
-	regmap_set_bits(utmi->syscon, SYSCON_UTMI_CFG_REG(port->id),
-			UTMI_PHY_CFG_PU_MASK);
+	if (!IS_ERR(port->regs_cfg)) {
+		reg = readl(port->regs_cfg);
+		reg |= UTMI_PHY_CFG_PU_MASK;
+		writel(reg, port->regs_cfg);
+	} else
+		regmap_set_bits(utmi->syscon, SYSCON_UTMI_CFG_REG(port->id),
+				UTMI_PHY_CFG_PU_MASK);
 
 	/* Disable Test UTMI select */
-	reg = readl(PORT_REGS(port) + UTMI_CTRL_STATUS0_REG);
+	reg = readl(port->regs + UTMI_CTRL_STATUS0_REG);
 	reg &= ~TEST_SEL;
-	writel(reg, PORT_REGS(port) + UTMI_CTRL_STATUS0_REG);
+	writel(reg, port->regs + UTMI_CTRL_STATUS0_REG);
 
 	/* Wait for impedance calibration */
-	ret = readl_poll_timeout(PORT_REGS(port) + UTMI_CAL_CTRL_REG, reg,
+	ret = readl_poll_timeout(port->regs + UTMI_CAL_CTRL_REG, reg,
 				 reg & IMPCAL_DONE,
 				 PLL_LOCK_DELAY_US, PLL_LOCK_TIMEOUT_US);
 	if (ret) {
@@ -244,7 +313,7 @@ static int mvebu_cp110_utmi_phy_power_on(struct phy *phy)
 	}
 
 	/* Wait for PLL calibration */
-	ret = readl_poll_timeout(PORT_REGS(port) + UTMI_CAL_CTRL_REG, reg,
+	ret = readl_poll_timeout(port->regs + UTMI_CAL_CTRL_REG, reg,
 				 reg & PLLCAL_DONE,
 				 PLL_LOCK_DELAY_US, PLL_LOCK_TIMEOUT_US);
 	if (ret) {
@@ -253,7 +322,7 @@ static int mvebu_cp110_utmi_phy_power_on(struct phy *phy)
 	}
 
 	/* Wait for PLL ready */
-	ret = readl_poll_timeout(PORT_REGS(port) + UTMI_PLL_CTRL_REG, reg,
+	ret = readl_poll_timeout(port->regs + UTMI_PLL_CTRL_REG, reg,
 				 reg & PLL_RDY,
 				 PLL_LOCK_DELAY_US, PLL_LOCK_TIMEOUT_US);
 	if (ret) {
@@ -262,7 +331,12 @@ static int mvebu_cp110_utmi_phy_power_on(struct phy *phy)
 	}
 
 	/* PLL Power up */
-	regmap_set_bits(utmi->syscon, SYSCON_USB_CFG_REG, USB_CFG_PLL_MASK);
+	if (!IS_ERR(utmi->regs_usb)) {
+		reg = readl(utmi->regs_usb);
+		reg |= USB_CFG_PLL_MASK;
+		writel(reg, utmi->regs_usb);
+	} else
+		regmap_set_bits(utmi->syscon, SYSCON_USB_CFG_REG, USB_CFG_PLL_MASK);
 
 	return 0;
 }
@@ -274,7 +348,8 @@ static const struct phy_ops mvebu_cp110_utmi_phy_ops = {
 };
 
 static const struct of_device_id mvebu_cp110_utmi_of_match[] = {
-	{ .compatible = "marvell,cp110-utmi-phy" },
+	{ .compatible = "marvell,a38x-utmi-phy", .data = (void *)A380_UTMI },
+	{ .compatible = "marvell,cp110-utmi-phy", .data = (void *)CP110_UTMI },
 	{},
 };
 MODULE_DEVICE_TABLE(of, mvebu_cp110_utmi_of_match);
@@ -285,6 +360,10 @@ static int mvebu_cp110_utmi_phy_probe(struct platform_device *pdev)
 	struct mvebu_cp110_utmi *utmi;
 	struct phy_provider *provider;
 	struct device_node *child;
+	void __iomem *regs_utmi;
+	void __iomem *regs_utmi_cfg;
+	const void *match;
+	enum mvebu_cp110_utmi_type type;
 	u32 usb_devices = 0;
 
 	utmi = devm_kzalloc(dev, sizeof(*utmi), GFP_KERNEL);
@@ -293,18 +372,44 @@ static int mvebu_cp110_utmi_phy_probe(struct platform_device *pdev)
 
 	utmi->dev = dev;
 
+	match = device_get_match_data(dev);
+	if (match)
+		type = (enum mvebu_cp110_utmi_type)(uintptr_t)match;
+
+	/* Get UTMI memory region */
+	regs_utmi = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(regs_utmi)) {
+		dev_err(dev, "Failed to map utmi regs\n");
+		return PTR_ERR(regs_utmi);
+	}
+
+	/* Get usb config region */
+	utmi->regs_usb = devm_platform_ioremap_resource_byname(pdev, "usb-cfg");
+	if (IS_ERR(utmi->regs_usb) && PTR_ERR(utmi->regs_usb) != -EINVAL) {
+		dev_err(dev, "Failed to map usb config regs\n");
+		return PTR_ERR(utmi->regs_usb);
+	}
+
+	/* Get utmi config region */
+	regs_utmi_cfg = devm_platform_ioremap_resource_byname(pdev, "utmi-cfg");
+	if (IS_ERR(regs_utmi_cfg) && PTR_ERR(regs_utmi_cfg) != -EINVAL) {
+		dev_err(dev, "Failed to map usb config regs\n");
+		return PTR_ERR(regs_utmi_cfg);
+	}
+
 	/* Get system controller region */
 	utmi->syscon = syscon_regmap_lookup_by_phandle(dev->of_node,
 						       "marvell,system-controller");
-	if (IS_ERR(utmi->syscon)) {
-		dev_err(dev, "Missing UTMI system controller\n");
+	if (IS_ERR(utmi->syscon) && PTR_ERR(utmi->syscon) != -ENODEV) {
+		dev_err(dev, "Failed to get system controller\n");
 		return PTR_ERR(utmi->syscon);
 	}
 
-	/* Get UTMI memory region */
-	utmi->regs = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(utmi->regs))
-		return PTR_ERR(utmi->regs);
+	if (IS_ERR(utmi->syscon) &&
+	    (IS_ERR(utmi->regs_usb) || IS_ERR(regs_utmi_cfg))) {
+		dev_err(dev, "Missing utmi system controller or config regs");
+		return -EINVAL;
+	}
 
 	for_each_available_child_of_node(dev->of_node, child) {
 		struct mvebu_cp110_utmi_port *port;
@@ -326,6 +431,24 @@ static int mvebu_cp110_utmi_phy_probe(struct platform_device *pdev)
 			return -ENOMEM;
 		}
 
+		utmi->ports[port_id] = port;
+
+		/* Get port memory region */
+		switch (type) {
+		case A380_UTMI:
+			port->regs = regs_utmi + port_id * 0x1000;
+			break;
+		case CP110_UTMI:
+			port->regs = regs_utmi + port_id * 0x2000;
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		/* assign utmi cfg reg */
+		if (!IS_ERR(regs_utmi_cfg))
+			port->regs_cfg = regs_utmi_cfg + port_id * 4;
+
 		port->dr_mode = of_usb_get_dr_mode_by_phy(child, -1);
 		if ((port->dr_mode != USB_DR_MODE_HOST) &&
 		    (port->dr_mode != USB_DR_MODE_PERIPHERAL)) {

-- 
2.43.0

Re: [PATCH RFC v3 5/6] phy: mvebu-cp110-utmi: add support for armada-380 utmi phys

[Vinod Koul]

On 20-07-24, 16:19, Josua Mayer wrote:
> Armada 380 has similar USB-2.0 PHYs as CP-110. The differences are:
> - register base addresses
> - gap between port registers
> - number of ports: 388 has three, cp110 two
> - device-mode mux bit refers to different ports
> - syscon register's base address (offsets identical)
> - armada-8k uses syscon for various drivers, a38x not
> 
> Differentiation uses of_match_data with distinct compatible strings.
> 
> Add support for Armada 380 PHYs by partially restructuting the driver:
> - Port register pointers are moved to the per-port private data.
> - Add armada-38x-specific compatible string and store enum value in
>   of_match_data for differentiation.
> - Add support for optional regs usb-cfg and utmi-cfg, to be used instead
>   of syscon.
> 
> Signed-off-by: Josua Mayer <josua@solid-run.com>
> ---
>  drivers/phy/marvell/phy-mvebu-cp110-utmi.c | 209 +++++++++++++++++++++++------
>  1 file changed, 166 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/phy/marvell/phy-mvebu-cp110-utmi.c b/drivers/phy/marvell/phy-mvebu-cp110-utmi.c
> index 4922a5f3327d..4341923e85bc 100644
> --- a/drivers/phy/marvell/phy-mvebu-cp110-utmi.c
> +++ b/drivers/phy/marvell/phy-mvebu-cp110-utmi.c
> @@ -19,7 +19,7 @@
>  #include <linux/usb/of.h>
>  #include <linux/usb/otg.h>
>  
> -#define UTMI_PHY_PORTS				2
> +#define UTMI_PHY_PORTS				3
>  
>  /* CP110 UTMI register macro definetions */
>  #define SYSCON_USB_CFG_REG			0x420
> @@ -76,32 +76,44 @@
>  #define PLL_LOCK_DELAY_US			10000
>  #define PLL_LOCK_TIMEOUT_US			1000000
>  
> -#define PORT_REGS(p)				((p)->priv->regs + (p)->id * 0x1000)
> +enum mvebu_cp110_utmi_type {
> +	/* 0 is reserved to avoid clashing with NULL */
> +	A380_UTMI = 1,
> +	CP110_UTMI = 2,
> +};
> +
> +struct mvebu_cp110_utmi_port;

why forward declare and not move the structs instead?

>  
>  /**
>   * struct mvebu_cp110_utmi - PHY driver data
>   *
> - * @regs: PHY registers
> + * @regs_usb: USB configuration register
>   * @syscon: Regmap with system controller registers
>   * @dev: device driver handle
>   * @ops: phy ops
> + * @ports: phy object for each port
>   */
>  struct mvebu_cp110_utmi {
> -	void __iomem *regs;
> +	void __iomem *regs_usb;
>  	struct regmap *syscon;
>  	struct device *dev;
>  	const struct phy_ops *ops;
> +	struct mvebu_cp110_utmi_port *ports[UTMI_PHY_PORTS];
>  };
>  
>  /**
>   * struct mvebu_cp110_utmi_port - PHY port data
>   *
> + * @regs: PHY registers
> + * @regs_cfg: PHY config register
>   * @priv: PHY driver data
>   * @id: PHY port ID
>   * @dr_mode: PHY connection: USB_DR_MODE_HOST or USB_DR_MODE_PERIPHERAL
>   */
>  struct mvebu_cp110_utmi_port {
>  	struct mvebu_cp110_utmi *priv;
> +	void __iomem *regs;
> +	void __iomem *regs_cfg;
>  	u32 id;
>  	enum usb_dr_mode dr_mode;
>  };
> @@ -118,47 +130,47 @@ static void mvebu_cp110_utmi_port_setup(struct mvebu_cp110_utmi_port *port)
>  	 * The crystal used for all platform boards is now 25MHz.
>  	 * See the functional specification for details.
>  	 */
> -	reg = readl(PORT_REGS(port) + UTMI_PLL_CTRL_REG);
> +	reg = readl(port->regs + UTMI_PLL_CTRL_REG);

why not handle this as a preparatory patch for this? Helps in review

>  	reg &= ~(PLL_REFDIV_MASK | PLL_FBDIV_MASK | PLL_SEL_LPFR_MASK);
>  	reg |= (PLL_REFDIV_VAL << PLL_REFDIV_OFFSET) |
>  	       (PLL_FBDIV_VAL << PLL_FBDIV_OFFSET);
> -	writel(reg, PORT_REGS(port) + UTMI_PLL_CTRL_REG);
> +	writel(reg, port->regs + UTMI_PLL_CTRL_REG);
>  
>  	/* Impedance Calibration Threshold Setting */
> -	reg = readl(PORT_REGS(port) + UTMI_CAL_CTRL_REG);
> +	reg = readl(port->regs + UTMI_CAL_CTRL_REG);
>  	reg &= ~IMPCAL_VTH_MASK;
>  	reg |= IMPCAL_VTH_VAL << IMPCAL_VTH_OFFSET;
> -	writel(reg, PORT_REGS(port) + UTMI_CAL_CTRL_REG);
> +	writel(reg, port->regs + UTMI_CAL_CTRL_REG);
>  
>  	/* Set LS TX driver strength coarse control */
> -	reg = readl(PORT_REGS(port) + UTMI_TX_CH_CTRL_REG);
> +	reg = readl(port->regs + UTMI_TX_CH_CTRL_REG);
>  	reg &= ~TX_AMP_MASK;
>  	reg |= TX_AMP_VAL << TX_AMP_OFFSET;
> -	writel(reg, PORT_REGS(port) + UTMI_TX_CH_CTRL_REG);
> +	writel(reg, port->regs + UTMI_TX_CH_CTRL_REG);
>  
>  	/* Disable SQ and enable analog squelch detect */
> -	reg = readl(PORT_REGS(port) + UTMI_RX_CH_CTRL0_REG);
> +	reg = readl(port->regs + UTMI_RX_CH_CTRL0_REG);
>  	reg &= ~SQ_DET_EN;
>  	reg |= SQ_ANA_DTC_SEL;
> -	writel(reg, PORT_REGS(port) + UTMI_RX_CH_CTRL0_REG);
> +	writel(reg, port->regs + UTMI_RX_CH_CTRL0_REG);
>  
>  	/*
>  	 * Set External squelch calibration number and
>  	 * enable the External squelch calibration
>  	 */
> -	reg = readl(PORT_REGS(port) + UTMI_RX_CH_CTRL1_REG);
> +	reg = readl(port->regs + UTMI_RX_CH_CTRL1_REG);
>  	reg &= ~SQ_AMP_CAL_MASK;
>  	reg |= (SQ_AMP_CAL_VAL << SQ_AMP_CAL_OFFSET) | SQ_AMP_CAL_EN;
> -	writel(reg, PORT_REGS(port) + UTMI_RX_CH_CTRL1_REG);
> +	writel(reg, port->regs + UTMI_RX_CH_CTRL1_REG);
>  
>  	/*
>  	 * Set Control VDAT Reference Voltage - 0.325V and
>  	 * Control VSRC Reference Voltage - 0.6V
>  	 */
> -	reg = readl(PORT_REGS(port) + UTMI_CHGDTC_CTRL_REG);
> +	reg = readl(port->regs + UTMI_CHGDTC_CTRL_REG);
>  	reg &= ~(VDAT_MASK | VSRC_MASK);
>  	reg |= (VDAT_VAL << VDAT_OFFSET) | (VSRC_VAL << VSRC_OFFSET);
> -	writel(reg, PORT_REGS(port) + UTMI_CHGDTC_CTRL_REG);
> +	writel(reg, port->regs + UTMI_CHGDTC_CTRL_REG);
>  }
>  
>  static int mvebu_cp110_utmi_phy_power_off(struct phy *phy)
> @@ -166,22 +178,38 @@ static int mvebu_cp110_utmi_phy_power_off(struct phy *phy)
>  	struct mvebu_cp110_utmi_port *port = phy_get_drvdata(phy);
>  	struct mvebu_cp110_utmi *utmi = port->priv;
>  	int i;
> +	int reg;
>  
>  	/* Power down UTMI PHY port */
> -	regmap_clear_bits(utmi->syscon, SYSCON_UTMI_CFG_REG(port->id),
> -			  UTMI_PHY_CFG_PU_MASK);
> +	if (!IS_ERR(port->regs_cfg)) {
> +		reg = readl(port->regs_cfg);
> +		reg &= ~(UTMI_PHY_CFG_PU_MASK);
> +		writel(reg, port->regs_cfg);
> +	} else
> +		regmap_clear_bits(utmi->syscon, SYSCON_UTMI_CFG_REG(port->id),
> +				  UTMI_PHY_CFG_PU_MASK);

why are we doing both raw register read/write and regmap ops... that
does not sound correct to me

>  
>  	for (i = 0; i < UTMI_PHY_PORTS; i++) {
> -		int test = regmap_test_bits(utmi->syscon,
> -					    SYSCON_UTMI_CFG_REG(i),
> -					    UTMI_PHY_CFG_PU_MASK);
> +		if (!utmi->ports[i])
> +			continue;
> +
> +		if (!IS_ERR(utmi->ports[i]->regs_cfg))
> +			reg = readl(utmi->ports[i]->regs_cfg);
> +		else
> +			regmap_read(utmi->syscon, SYSCON_UTMI_CFG_REG(i), &reg);
> +		int test = reg & UTMI_PHY_CFG_PU_MASK;
>  		/* skip PLL shutdown if there are active UTMI PHY ports */
>  		if (test != 0)
>  			return 0;
>  	}
>  
>  	/* PLL Power down if all UTMI PHYs are down */
> -	regmap_clear_bits(utmi->syscon, SYSCON_USB_CFG_REG, USB_CFG_PLL_MASK);
> +	if (!IS_ERR(utmi->regs_usb)) {
> +		reg = readl(utmi->regs_usb);
> +		reg &= ~(USB_CFG_PLL_MASK);
> +		writel(reg, utmi->regs_usb);
> +	} else
> +		regmap_clear_bits(utmi->syscon, SYSCON_USB_CFG_REG, USB_CFG_PLL_MASK);
>  
>  	return 0;
>  }
> @@ -191,8 +219,15 @@ static int mvebu_cp110_utmi_phy_power_on(struct phy *phy)
>  	struct mvebu_cp110_utmi_port *port = phy_get_drvdata(phy);
>  	struct mvebu_cp110_utmi *utmi = port->priv;
>  	struct device *dev = &phy->dev;
> +	const void *match;
> +	enum mvebu_cp110_utmi_type type;
>  	int ret;
>  	u32 reg;
> +	u32 sel;
> +
> +	match = device_get_match_data(utmi->dev);
> +	if (match)
> +		type = (enum mvebu_cp110_utmi_type)(uintptr_t)match;
>  
>  	/* It is necessary to power off UTMI before configuration */
>  	ret = mvebu_cp110_utmi_phy_power_off(phy);
> @@ -208,16 +243,45 @@ static int mvebu_cp110_utmi_phy_power_on(struct phy *phy)
>  	 * to UTMI0 or to UTMI1 PHY port, but not to both.
>  	 */
>  	if (port->dr_mode == USB_DR_MODE_PERIPHERAL) {
> -		regmap_update_bits(utmi->syscon, SYSCON_USB_CFG_REG,
> -				   USB_CFG_DEVICE_EN_MASK | USB_CFG_DEVICE_MUX_MASK,
> -				   USB_CFG_DEVICE_EN_MASK |
> -				   (port->id << USB_CFG_DEVICE_MUX_OFFSET));
> +		switch (type) {
> +		case A380_UTMI:
> +			/*
> +			 * A380 muxes between ports 0/2:
> +			 * - 0: Device mode on Port 2
> +			 * - 1: Device mode on Port 0
> +			 */
> +			if (port->id == 1)
> +				return -EINVAL;
> +			sel = !!(port->id == 0);
> +			break;
> +		case CP110_UTMI:
> +			/*
> +			 * CP110 muxes between ports 0/1:
> +			 * - 0: Device mode on Port 0
> +			 * - 1: Device mode on Port 1
> +			 */
> +			sel = port->id;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		if (!IS_ERR(utmi->regs_usb)) {
> +			reg = readl(utmi->regs_usb);
> +			reg &= ~(USB_CFG_DEVICE_EN_MASK | USB_CFG_DEVICE_MUX_MASK);
> +			reg |= USB_CFG_DEVICE_EN_MASK;
> +			reg |= (sel << USB_CFG_DEVICE_MUX_OFFSET);
> +			writel(reg, utmi->regs_usb);
> +		} else
> +			regmap_update_bits(utmi->syscon, SYSCON_USB_CFG_REG,
> +					   USB_CFG_DEVICE_EN_MASK | USB_CFG_DEVICE_MUX_MASK,
> +					   USB_CFG_DEVICE_EN_MASK |
> +					   (sel << USB_CFG_DEVICE_MUX_OFFSET));
>  	}
>  
>  	/* Set Test suspendm mode and enable Test UTMI select */
> -	reg = readl(PORT_REGS(port) + UTMI_CTRL_STATUS0_REG);
> +	reg = readl(port->regs + UTMI_CTRL_STATUS0_REG);
>  	reg |= SUSPENDM | TEST_SEL;
> -	writel(reg, PORT_REGS(port) + UTMI_CTRL_STATUS0_REG);
> +	writel(reg, port->regs + UTMI_CTRL_STATUS0_REG);
>  
>  	/* Wait for UTMI power down */
>  	mdelay(1);
> @@ -226,16 +290,21 @@ static int mvebu_cp110_utmi_phy_power_on(struct phy *phy)
>  	mvebu_cp110_utmi_port_setup(port);
>  
>  	/* Power UP UTMI PHY */
> -	regmap_set_bits(utmi->syscon, SYSCON_UTMI_CFG_REG(port->id),
> -			UTMI_PHY_CFG_PU_MASK);
> +	if (!IS_ERR(port->regs_cfg)) {
> +		reg = readl(port->regs_cfg);
> +		reg |= UTMI_PHY_CFG_PU_MASK;
> +		writel(reg, port->regs_cfg);
> +	} else
> +		regmap_set_bits(utmi->syscon, SYSCON_UTMI_CFG_REG(port->id),
> +				UTMI_PHY_CFG_PU_MASK);
>  
>  	/* Disable Test UTMI select */
> -	reg = readl(PORT_REGS(port) + UTMI_CTRL_STATUS0_REG);
> +	reg = readl(port->regs + UTMI_CTRL_STATUS0_REG);
>  	reg &= ~TEST_SEL;
> -	writel(reg, PORT_REGS(port) + UTMI_CTRL_STATUS0_REG);
> +	writel(reg, port->regs + UTMI_CTRL_STATUS0_REG);
>  
>  	/* Wait for impedance calibration */
> -	ret = readl_poll_timeout(PORT_REGS(port) + UTMI_CAL_CTRL_REG, reg,
> +	ret = readl_poll_timeout(port->regs + UTMI_CAL_CTRL_REG, reg,
>  				 reg & IMPCAL_DONE,
>  				 PLL_LOCK_DELAY_US, PLL_LOCK_TIMEOUT_US);
>  	if (ret) {
> @@ -244,7 +313,7 @@ static int mvebu_cp110_utmi_phy_power_on(struct phy *phy)
>  	}
>  
>  	/* Wait for PLL calibration */
> -	ret = readl_poll_timeout(PORT_REGS(port) + UTMI_CAL_CTRL_REG, reg,
> +	ret = readl_poll_timeout(port->regs + UTMI_CAL_CTRL_REG, reg,
>  				 reg & PLLCAL_DONE,
>  				 PLL_LOCK_DELAY_US, PLL_LOCK_TIMEOUT_US);
>  	if (ret) {
> @@ -253,7 +322,7 @@ static int mvebu_cp110_utmi_phy_power_on(struct phy *phy)
>  	}
>  
>  	/* Wait for PLL ready */
> -	ret = readl_poll_timeout(PORT_REGS(port) + UTMI_PLL_CTRL_REG, reg,
> +	ret = readl_poll_timeout(port->regs + UTMI_PLL_CTRL_REG, reg,
>  				 reg & PLL_RDY,
>  				 PLL_LOCK_DELAY_US, PLL_LOCK_TIMEOUT_US);
>  	if (ret) {
> @@ -262,7 +331,12 @@ static int mvebu_cp110_utmi_phy_power_on(struct phy *phy)
>  	}
>  
>  	/* PLL Power up */
> -	regmap_set_bits(utmi->syscon, SYSCON_USB_CFG_REG, USB_CFG_PLL_MASK);
> +	if (!IS_ERR(utmi->regs_usb)) {
> +		reg = readl(utmi->regs_usb);
> +		reg |= USB_CFG_PLL_MASK;
> +		writel(reg, utmi->regs_usb);
> +	} else
> +		regmap_set_bits(utmi->syscon, SYSCON_USB_CFG_REG, USB_CFG_PLL_MASK);
>  
>  	return 0;
>  }
> @@ -274,7 +348,8 @@ static const struct phy_ops mvebu_cp110_utmi_phy_ops = {
>  };
>  
>  static const struct of_device_id mvebu_cp110_utmi_of_match[] = {
> -	{ .compatible = "marvell,cp110-utmi-phy" },
> +	{ .compatible = "marvell,a38x-utmi-phy", .data = (void *)A380_UTMI },
> +	{ .compatible = "marvell,cp110-utmi-phy", .data = (void *)CP110_UTMI },

Cast to void * are not required to be done

>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, mvebu_cp110_utmi_of_match);
> @@ -285,6 +360,10 @@ static int mvebu_cp110_utmi_phy_probe(struct platform_device *pdev)
>  	struct mvebu_cp110_utmi *utmi;
>  	struct phy_provider *provider;
>  	struct device_node *child;
> +	void __iomem *regs_utmi;
> +	void __iomem *regs_utmi_cfg;
> +	const void *match;
> +	enum mvebu_cp110_utmi_type type;
>  	u32 usb_devices = 0;
>  
>  	utmi = devm_kzalloc(dev, sizeof(*utmi), GFP_KERNEL);
> @@ -293,18 +372,44 @@ static int mvebu_cp110_utmi_phy_probe(struct platform_device *pdev)
>  
>  	utmi->dev = dev;
>  
> +	match = device_get_match_data(dev);
> +	if (match)
> +		type = (enum mvebu_cp110_utmi_type)(uintptr_t)match;
> +
> +	/* Get UTMI memory region */
> +	regs_utmi = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(regs_utmi)) {
> +		dev_err(dev, "Failed to map utmi regs\n");
> +		return PTR_ERR(regs_utmi);
> +	}
> +
> +	/* Get usb config region */
> +	utmi->regs_usb = devm_platform_ioremap_resource_byname(pdev, "usb-cfg");
> +	if (IS_ERR(utmi->regs_usb) && PTR_ERR(utmi->regs_usb) != -EINVAL) {
> +		dev_err(dev, "Failed to map usb config regs\n");
> +		return PTR_ERR(utmi->regs_usb);
> +	}
> +
> +	/* Get utmi config region */
> +	regs_utmi_cfg = devm_platform_ioremap_resource_byname(pdev, "utmi-cfg");
> +	if (IS_ERR(regs_utmi_cfg) && PTR_ERR(regs_utmi_cfg) != -EINVAL) {
> +		dev_err(dev, "Failed to map usb config regs\n");
> +		return PTR_ERR(regs_utmi_cfg);
> +	}
> +
>  	/* Get system controller region */
>  	utmi->syscon = syscon_regmap_lookup_by_phandle(dev->of_node,
>  						       "marvell,system-controller");
> -	if (IS_ERR(utmi->syscon)) {
> -		dev_err(dev, "Missing UTMI system controller\n");
> +	if (IS_ERR(utmi->syscon) && PTR_ERR(utmi->syscon) != -ENODEV) {
> +		dev_err(dev, "Failed to get system controller\n");
>  		return PTR_ERR(utmi->syscon);
>  	}
>  
> -	/* Get UTMI memory region */
> -	utmi->regs = devm_platform_ioremap_resource(pdev, 0);
> -	if (IS_ERR(utmi->regs))
> -		return PTR_ERR(utmi->regs);
> +	if (IS_ERR(utmi->syscon) &&
> +	    (IS_ERR(utmi->regs_usb) || IS_ERR(regs_utmi_cfg))) {
> +		dev_err(dev, "Missing utmi system controller or config regs");
> +		return -EINVAL;
> +	}
>  
>  	for_each_available_child_of_node(dev->of_node, child) {
>  		struct mvebu_cp110_utmi_port *port;
> @@ -326,6 +431,24 @@ static int mvebu_cp110_utmi_phy_probe(struct platform_device *pdev)
>  			return -ENOMEM;
>  		}
>  
> +		utmi->ports[port_id] = port;
> +
> +		/* Get port memory region */
> +		switch (type) {
> +		case A380_UTMI:
> +			port->regs = regs_utmi + port_id * 0x1000;
> +			break;
> +		case CP110_UTMI:
> +			port->regs = regs_utmi + port_id * 0x2000;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +		/* assign utmi cfg reg */
> +		if (!IS_ERR(regs_utmi_cfg))
> +			port->regs_cfg = regs_utmi_cfg + port_id * 4;
> +
>  		port->dr_mode = of_usb_get_dr_mode_by_phy(child, -1);
>  		if ((port->dr_mode != USB_DR_MODE_HOST) &&
>  		    (port->dr_mode != USB_DR_MODE_PERIPHERAL)) {
> 
> -- 
> 2.43.0

-- 
~Vinod

Re: [PATCH RFC v3 5/6] phy: mvebu-cp110-utmi: add support for armada-380 utmi phys

[Josua Mayer]

Am 25.07.24 um 08:43 schrieb Vinod Koul:
> On 20-07-24, 16:19, Josua Mayer wrote:
>> Armada 380 has similar USB-2.0 PHYs as CP-110. The differences are:
>> - register base addresses
>> - gap between port registers
>> - number of ports: 388 has three, cp110 two
>> - device-mode mux bit refers to different ports
>> - syscon register's base address (offsets identical)
>> - armada-8k uses syscon for various drivers, a38x not
>>
>> Differentiation uses of_match_data with distinct compatible strings.
>>
>> Add support for Armada 380 PHYs by partially restructuting the driver:
>> - Port register pointers are moved to the per-port private data.
>> - Add armada-38x-specific compatible string and store enum value in
>>   of_match_data for differentiation.
>> - Add support for optional regs usb-cfg and utmi-cfg, to be used instead
>>   of syscon.
>>
>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>> ---
>>  drivers/phy/marvell/phy-mvebu-cp110-utmi.c | 209 +++++++++++++++++++++++------
>>  1 file changed, 166 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/phy/marvell/phy-mvebu-cp110-utmi.c b/drivers/phy/marvell/phy-mvebu-cp110-utmi.c
>> index 4922a5f3327d..4341923e85bc 100644
>> --- a/drivers/phy/marvell/phy-mvebu-cp110-utmi.c
>> +++ b/drivers/phy/marvell/phy-mvebu-cp110-utmi.c
>> @@ -19,7 +19,7 @@
>>  #include <linux/usb/of.h>
>>  #include <linux/usb/otg.h>
>>  
>> -#define UTMI_PHY_PORTS				2
>> +#define UTMI_PHY_PORTS				3
>>  
>>  /* CP110 UTMI register macro definetions */
>>  #define SYSCON_USB_CFG_REG			0x420
>> @@ -76,32 +76,44 @@
>>  #define PLL_LOCK_DELAY_US			10000
>>  #define PLL_LOCK_TIMEOUT_US			1000000
>>  
>> -#define PORT_REGS(p)				((p)->priv->regs + (p)->id * 0x1000)
>> +enum mvebu_cp110_utmi_type {
>> +	/* 0 is reserved to avoid clashing with NULL */
>> +	A380_UTMI = 1,
>> +	CP110_UTMI = 2,
>> +};
>> +
>> +struct mvebu_cp110_utmi_port;
> why forward declare and not move the structs instead?
Seemed like the smaller change / more readable as diff.
I can move the struct instead!
>
>>  
>>  /**
>>   * struct mvebu_cp110_utmi - PHY driver data
>>   *
>> - * @regs: PHY registers
>> + * @regs_usb: USB configuration register
>>   * @syscon: Regmap with system controller registers
>>   * @dev: device driver handle
>>   * @ops: phy ops
>> + * @ports: phy object for each port
>>   */
>>  struct mvebu_cp110_utmi {
>> -	void __iomem *regs;
>> +	void __iomem *regs_usb;
>>  	struct regmap *syscon;
>>  	struct device *dev;
>>  	const struct phy_ops *ops;
>> +	struct mvebu_cp110_utmi_port *ports[UTMI_PHY_PORTS];
>>  };
>>  
>>  /**
>>   * struct mvebu_cp110_utmi_port - PHY port data
>>   *
>> + * @regs: PHY registers
>> + * @regs_cfg: PHY config register
>>   * @priv: PHY driver data
>>   * @id: PHY port ID
>>   * @dr_mode: PHY connection: USB_DR_MODE_HOST or USB_DR_MODE_PERIPHERAL
>>   */
>>  struct mvebu_cp110_utmi_port {
>>  	struct mvebu_cp110_utmi *priv;
>> +	void __iomem *regs;
>> +	void __iomem *regs_cfg;
>>  	u32 id;
>>  	enum usb_dr_mode dr_mode;
>>  };
>> @@ -118,47 +130,47 @@ static void mvebu_cp110_utmi_port_setup(struct mvebu_cp110_utmi_port *port)
>>  	 * The crystal used for all platform boards is now 25MHz.
>>  	 * See the functional specification for details.
>>  	 */
>> -	reg = readl(PORT_REGS(port) + UTMI_PLL_CTRL_REG);
>> +	reg = readl(port->regs + UTMI_PLL_CTRL_REG);
> why not handle this as a preparatory patch for this? Helps in review

Will do!

>
>>  	reg &= ~(PLL_REFDIV_MASK | PLL_FBDIV_MASK | PLL_SEL_LPFR_MASK);
>>  	reg |= (PLL_REFDIV_VAL << PLL_REFDIV_OFFSET) |
>>  	       (PLL_FBDIV_VAL << PLL_FBDIV_OFFSET);
>> -	writel(reg, PORT_REGS(port) + UTMI_PLL_CTRL_REG);
>> +	writel(reg, port->regs + UTMI_PLL_CTRL_REG);
>>  
>>  	/* Impedance Calibration Threshold Setting */
>> -	reg = readl(PORT_REGS(port) + UTMI_CAL_CTRL_REG);
>> +	reg = readl(port->regs + UTMI_CAL_CTRL_REG);
>>  	reg &= ~IMPCAL_VTH_MASK;
>>  	reg |= IMPCAL_VTH_VAL << IMPCAL_VTH_OFFSET;
>> -	writel(reg, PORT_REGS(port) + UTMI_CAL_CTRL_REG);
>> +	writel(reg, port->regs + UTMI_CAL_CTRL_REG);
>>  
>>  	/* Set LS TX driver strength coarse control */
>> -	reg = readl(PORT_REGS(port) + UTMI_TX_CH_CTRL_REG);
>> +	reg = readl(port->regs + UTMI_TX_CH_CTRL_REG);
>>  	reg &= ~TX_AMP_MASK;
>>  	reg |= TX_AMP_VAL << TX_AMP_OFFSET;
>> -	writel(reg, PORT_REGS(port) + UTMI_TX_CH_CTRL_REG);
>> +	writel(reg, port->regs + UTMI_TX_CH_CTRL_REG);
>>  
>>  	/* Disable SQ and enable analog squelch detect */
>> -	reg = readl(PORT_REGS(port) + UTMI_RX_CH_CTRL0_REG);
>> +	reg = readl(port->regs + UTMI_RX_CH_CTRL0_REG);
>>  	reg &= ~SQ_DET_EN;
>>  	reg |= SQ_ANA_DTC_SEL;
>> -	writel(reg, PORT_REGS(port) + UTMI_RX_CH_CTRL0_REG);
>> +	writel(reg, port->regs + UTMI_RX_CH_CTRL0_REG);
>>  
>>  	/*
>>  	 * Set External squelch calibration number and
>>  	 * enable the External squelch calibration
>>  	 */
>> -	reg = readl(PORT_REGS(port) + UTMI_RX_CH_CTRL1_REG);
>> +	reg = readl(port->regs + UTMI_RX_CH_CTRL1_REG);
>>  	reg &= ~SQ_AMP_CAL_MASK;
>>  	reg |= (SQ_AMP_CAL_VAL << SQ_AMP_CAL_OFFSET) | SQ_AMP_CAL_EN;
>> -	writel(reg, PORT_REGS(port) + UTMI_RX_CH_CTRL1_REG);
>> +	writel(reg, port->regs + UTMI_RX_CH_CTRL1_REG);
>>  
>>  	/*
>>  	 * Set Control VDAT Reference Voltage - 0.325V and
>>  	 * Control VSRC Reference Voltage - 0.6V
>>  	 */
>> -	reg = readl(PORT_REGS(port) + UTMI_CHGDTC_CTRL_REG);
>> +	reg = readl(port->regs + UTMI_CHGDTC_CTRL_REG);
>>  	reg &= ~(VDAT_MASK | VSRC_MASK);
>>  	reg |= (VDAT_VAL << VDAT_OFFSET) | (VSRC_VAL << VSRC_OFFSET);
>> -	writel(reg, PORT_REGS(port) + UTMI_CHGDTC_CTRL_REG);
>> +	writel(reg, port->regs + UTMI_CHGDTC_CTRL_REG);
>>  }
>>  
>>  static int mvebu_cp110_utmi_phy_power_off(struct phy *phy)
>> @@ -166,22 +178,38 @@ static int mvebu_cp110_utmi_phy_power_off(struct phy *phy)
>>  	struct mvebu_cp110_utmi_port *port = phy_get_drvdata(phy);
>>  	struct mvebu_cp110_utmi *utmi = port->priv;
>>  	int i;
>> +	int reg;
>>  
>>  	/* Power down UTMI PHY port */
>> -	regmap_clear_bits(utmi->syscon, SYSCON_UTMI_CFG_REG(port->id),
>> -			  UTMI_PHY_CFG_PU_MASK);
>> +	if (!IS_ERR(port->regs_cfg)) {
>> +		reg = readl(port->regs_cfg);
>> +		reg &= ~(UTMI_PHY_CFG_PU_MASK);
>> +		writel(reg, port->regs_cfg);
>> +	} else
>> +		regmap_clear_bits(utmi->syscon, SYSCON_UTMI_CFG_REG(port->id),
>> +				  UTMI_PHY_CFG_PU_MASK);
> why are we doing both raw register read/write and regmap ops... that
> does not sound correct to me
Indeed.

The next question would be why for Armada 8K / CP110 syscon was chosen.
From what I could find the registers accessed by utmi driver
are not accessed by other drivers.

I am adding raw register access as an alternative,

to keep supporting old device-tree specifying syscon handle.

I considered writing helper functions for the if-not-error-syscon-else-raw,
but between set_bits, clear_bits, global and per-port regs
would have ended up with too many.

>
>>  
>>  	for (i = 0; i < UTMI_PHY_PORTS; i++) {
>> -		int test = regmap_test_bits(utmi->syscon,
>> -					    SYSCON_UTMI_CFG_REG(i),
>> -					    UTMI_PHY_CFG_PU_MASK);
>> +		if (!utmi->ports[i])
>> +			continue;
>> +
>> +		if (!IS_ERR(utmi->ports[i]->regs_cfg))
>> +			reg = readl(utmi->ports[i]->regs_cfg);
>> +		else
>> +			regmap_read(utmi->syscon, SYSCON_UTMI_CFG_REG(i), &reg);
>> +		int test = reg & UTMI_PHY_CFG_PU_MASK;
>>  		/* skip PLL shutdown if there are active UTMI PHY ports */
>>  		if (test != 0)
>>  			return 0;
>>  	}
>>  
>>  	/* PLL Power down if all UTMI PHYs are down */
>> -	regmap_clear_bits(utmi->syscon, SYSCON_USB_CFG_REG, USB_CFG_PLL_MASK);
>> +	if (!IS_ERR(utmi->regs_usb)) {
>> +		reg = readl(utmi->regs_usb);
>> +		reg &= ~(USB_CFG_PLL_MASK);
>> +		writel(reg, utmi->regs_usb);
>> +	} else
>> +		regmap_clear_bits(utmi->syscon, SYSCON_USB_CFG_REG, USB_CFG_PLL_MASK);
>>  
>>  	return 0;
>>  }
>> @@ -191,8 +219,15 @@ static int mvebu_cp110_utmi_phy_power_on(struct phy *phy)
>>  	struct mvebu_cp110_utmi_port *port = phy_get_drvdata(phy);
>>  	struct mvebu_cp110_utmi *utmi = port->priv;
>>  	struct device *dev = &phy->dev;
>> +	const void *match;
>> +	enum mvebu_cp110_utmi_type type;
>>  	int ret;
>>  	u32 reg;
>> +	u32 sel;
>> +
>> +	match = device_get_match_data(utmi->dev);
>> +	if (match)
>> +		type = (enum mvebu_cp110_utmi_type)(uintptr_t)match;
>>  
>>  	/* It is necessary to power off UTMI before configuration */
>>  	ret = mvebu_cp110_utmi_phy_power_off(phy);
>> @@ -208,16 +243,45 @@ static int mvebu_cp110_utmi_phy_power_on(struct phy *phy)
>>  	 * to UTMI0 or to UTMI1 PHY port, but not to both.
>>  	 */
>>  	if (port->dr_mode == USB_DR_MODE_PERIPHERAL) {
>> -		regmap_update_bits(utmi->syscon, SYSCON_USB_CFG_REG,
>> -				   USB_CFG_DEVICE_EN_MASK | USB_CFG_DEVICE_MUX_MASK,
>> -				   USB_CFG_DEVICE_EN_MASK |
>> -				   (port->id << USB_CFG_DEVICE_MUX_OFFSET));
>> +		switch (type) {
>> +		case A380_UTMI:
>> +			/*
>> +			 * A380 muxes between ports 0/2:
>> +			 * - 0: Device mode on Port 2
>> +			 * - 1: Device mode on Port 0
>> +			 */
>> +			if (port->id == 1)
>> +				return -EINVAL;
>> +			sel = !!(port->id == 0);
>> +			break;
>> +		case CP110_UTMI:
>> +			/*
>> +			 * CP110 muxes between ports 0/1:
>> +			 * - 0: Device mode on Port 0
>> +			 * - 1: Device mode on Port 1
>> +			 */
>> +			sel = port->id;
>> +			break;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +		if (!IS_ERR(utmi->regs_usb)) {
>> +			reg = readl(utmi->regs_usb);
>> +			reg &= ~(USB_CFG_DEVICE_EN_MASK | USB_CFG_DEVICE_MUX_MASK);
>> +			reg |= USB_CFG_DEVICE_EN_MASK;
>> +			reg |= (sel << USB_CFG_DEVICE_MUX_OFFSET);
>> +			writel(reg, utmi->regs_usb);
>> +		} else
>> +			regmap_update_bits(utmi->syscon, SYSCON_USB_CFG_REG,
>> +					   USB_CFG_DEVICE_EN_MASK | USB_CFG_DEVICE_MUX_MASK,
>> +					   USB_CFG_DEVICE_EN_MASK |
>> +					   (sel << USB_CFG_DEVICE_MUX_OFFSET));
>>  	}
>>  
>>  	/* Set Test suspendm mode and enable Test UTMI select */
>> -	reg = readl(PORT_REGS(port) + UTMI_CTRL_STATUS0_REG);
>> +	reg = readl(port->regs + UTMI_CTRL_STATUS0_REG);
>>  	reg |= SUSPENDM | TEST_SEL;
>> -	writel(reg, PORT_REGS(port) + UTMI_CTRL_STATUS0_REG);
>> +	writel(reg, port->regs + UTMI_CTRL_STATUS0_REG);
>>  
>>  	/* Wait for UTMI power down */
>>  	mdelay(1);
>> @@ -226,16 +290,21 @@ static int mvebu_cp110_utmi_phy_power_on(struct phy *phy)
>>  	mvebu_cp110_utmi_port_setup(port);
>>  
>>  	/* Power UP UTMI PHY */
>> -	regmap_set_bits(utmi->syscon, SYSCON_UTMI_CFG_REG(port->id),
>> -			UTMI_PHY_CFG_PU_MASK);
>> +	if (!IS_ERR(port->regs_cfg)) {
>> +		reg = readl(port->regs_cfg);
>> +		reg |= UTMI_PHY_CFG_PU_MASK;
>> +		writel(reg, port->regs_cfg);
>> +	} else
>> +		regmap_set_bits(utmi->syscon, SYSCON_UTMI_CFG_REG(port->id),
>> +				UTMI_PHY_CFG_PU_MASK);
>>  
>>  	/* Disable Test UTMI select */
>> -	reg = readl(PORT_REGS(port) + UTMI_CTRL_STATUS0_REG);
>> +	reg = readl(port->regs + UTMI_CTRL_STATUS0_REG);
>>  	reg &= ~TEST_SEL;
>> -	writel(reg, PORT_REGS(port) + UTMI_CTRL_STATUS0_REG);
>> +	writel(reg, port->regs + UTMI_CTRL_STATUS0_REG);
>>  
>>  	/* Wait for impedance calibration */
>> -	ret = readl_poll_timeout(PORT_REGS(port) + UTMI_CAL_CTRL_REG, reg,
>> +	ret = readl_poll_timeout(port->regs + UTMI_CAL_CTRL_REG, reg,
>>  				 reg & IMPCAL_DONE,
>>  				 PLL_LOCK_DELAY_US, PLL_LOCK_TIMEOUT_US);
>>  	if (ret) {
>> @@ -244,7 +313,7 @@ static int mvebu_cp110_utmi_phy_power_on(struct phy *phy)
>>  	}
>>  
>>  	/* Wait for PLL calibration */
>> -	ret = readl_poll_timeout(PORT_REGS(port) + UTMI_CAL_CTRL_REG, reg,
>> +	ret = readl_poll_timeout(port->regs + UTMI_CAL_CTRL_REG, reg,
>>  				 reg & PLLCAL_DONE,
>>  				 PLL_LOCK_DELAY_US, PLL_LOCK_TIMEOUT_US);
>>  	if (ret) {
>> @@ -253,7 +322,7 @@ static int mvebu_cp110_utmi_phy_power_on(struct phy *phy)
>>  	}
>>  
>>  	/* Wait for PLL ready */
>> -	ret = readl_poll_timeout(PORT_REGS(port) + UTMI_PLL_CTRL_REG, reg,
>> +	ret = readl_poll_timeout(port->regs + UTMI_PLL_CTRL_REG, reg,
>>  				 reg & PLL_RDY,
>>  				 PLL_LOCK_DELAY_US, PLL_LOCK_TIMEOUT_US);
>>  	if (ret) {
>> @@ -262,7 +331,12 @@ static int mvebu_cp110_utmi_phy_power_on(struct phy *phy)
>>  	}
>>  
>>  	/* PLL Power up */
>> -	regmap_set_bits(utmi->syscon, SYSCON_USB_CFG_REG, USB_CFG_PLL_MASK);
>> +	if (!IS_ERR(utmi->regs_usb)) {
>> +		reg = readl(utmi->regs_usb);
>> +		reg |= USB_CFG_PLL_MASK;
>> +		writel(reg, utmi->regs_usb);
>> +	} else
>> +		regmap_set_bits(utmi->syscon, SYSCON_USB_CFG_REG, USB_CFG_PLL_MASK);
>>  
>>  	return 0;
>>  }
>> @@ -274,7 +348,8 @@ static const struct phy_ops mvebu_cp110_utmi_phy_ops = {
>>  };
>>  
>>  static const struct of_device_id mvebu_cp110_utmi_of_match[] = {
>> -	{ .compatible = "marvell,cp110-utmi-phy" },
>> +	{ .compatible = "marvell,a38x-utmi-phy", .data = (void *)A380_UTMI },
>> +	{ .compatible = "marvell,cp110-utmi-phy", .data = (void *)CP110_UTMI },
> Cast to void * are not required to be done
Ack.
>
>>  	{},
>>  };
>>  MODULE_DEVICE_TABLE(of, mvebu_cp110_utmi_of_match);
>> @@ -285,6 +360,10 @@ static int mvebu_cp110_utmi_phy_probe(struct platform_device *pdev)
>>  	struct mvebu_cp110_utmi *utmi;
>>  	struct phy_provider *provider;
>>  	struct device_node *child;
>> +	void __iomem *regs_utmi;
>> +	void __iomem *regs_utmi_cfg;
>> +	const void *match;
>> +	enum mvebu_cp110_utmi_type type;
>>  	u32 usb_devices = 0;
>>  
>>  	utmi = devm_kzalloc(dev, sizeof(*utmi), GFP_KERNEL);
>> @@ -293,18 +372,44 @@ static int mvebu_cp110_utmi_phy_probe(struct platform_device *pdev)
>>  
>>  	utmi->dev = dev;
>>  
>> +	match = device_get_match_data(dev);
>> +	if (match)
>> +		type = (enum mvebu_cp110_utmi_type)(uintptr_t)match;
>> +
>> +	/* Get UTMI memory region */
>> +	regs_utmi = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(regs_utmi)) {
>> +		dev_err(dev, "Failed to map utmi regs\n");
>> +		return PTR_ERR(regs_utmi);
>> +	}
>> +
>> +	/* Get usb config region */
>> +	utmi->regs_usb = devm_platform_ioremap_resource_byname(pdev, "usb-cfg");
>> +	if (IS_ERR(utmi->regs_usb) && PTR_ERR(utmi->regs_usb) != -EINVAL) {
>> +		dev_err(dev, "Failed to map usb config regs\n");
>> +		return PTR_ERR(utmi->regs_usb);
>> +	}
>> +
>> +	/* Get utmi config region */
>> +	regs_utmi_cfg = devm_platform_ioremap_resource_byname(pdev, "utmi-cfg");
>> +	if (IS_ERR(regs_utmi_cfg) && PTR_ERR(regs_utmi_cfg) != -EINVAL) {
>> +		dev_err(dev, "Failed to map usb config regs\n");
>> +		return PTR_ERR(regs_utmi_cfg);
>> +	}
>> +
>>  	/* Get system controller region */
>>  	utmi->syscon = syscon_regmap_lookup_by_phandle(dev->of_node,
>>  						       "marvell,system-controller");
>> -	if (IS_ERR(utmi->syscon)) {
>> -		dev_err(dev, "Missing UTMI system controller\n");
>> +	if (IS_ERR(utmi->syscon) && PTR_ERR(utmi->syscon) != -ENODEV) {
>> +		dev_err(dev, "Failed to get system controller\n");
>>  		return PTR_ERR(utmi->syscon);
>>  	}
>>  
>> -	/* Get UTMI memory region */
>> -	utmi->regs = devm_platform_ioremap_resource(pdev, 0);
>> -	if (IS_ERR(utmi->regs))
>> -		return PTR_ERR(utmi->regs);
>> +	if (IS_ERR(utmi->syscon) &&
>> +	    (IS_ERR(utmi->regs_usb) || IS_ERR(regs_utmi_cfg))) {
>> +		dev_err(dev, "Missing utmi system controller or config regs");
>> +		return -EINVAL;
>> +	}
>>  
>>  	for_each_available_child_of_node(dev->of_node, child) {
>>  		struct mvebu_cp110_utmi_port *port;
>> @@ -326,6 +431,24 @@ static int mvebu_cp110_utmi_phy_probe(struct platform_device *pdev)
>>  			return -ENOMEM;
>>  		}
>>  
>> +		utmi->ports[port_id] = port;
>> +
>> +		/* Get port memory region */
>> +		switch (type) {
>> +		case A380_UTMI:
>> +			port->regs = regs_utmi + port_id * 0x1000;
>> +			break;
>> +		case CP110_UTMI:
>> +			port->regs = regs_utmi + port_id * 0x2000;
>> +			break;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +
>> +		/* assign utmi cfg reg */
>> +		if (!IS_ERR(regs_utmi_cfg))
>> +			port->regs_cfg = regs_utmi_cfg + port_id * 4;
>> +
>>  		port->dr_mode = of_usb_get_dr_mode_by_phy(child, -1);
>>  		if ((port->dr_mode != USB_DR_MODE_HOST) &&
>>  		    (port->dr_mode != USB_DR_MODE_PERIPHERAL)) {
>>
>> -- 
>> 2.43.0

Re: [PATCH RFC v3 5/6] phy: mvebu-cp110-utmi: add support for armada-380 utmi phys

[Vinod Koul]

On 25-07-24, 08:38, Josua Mayer wrote:
> Am 25.07.24 um 08:43 schrieb Vinod Koul:
> > On 20-07-24, 16:19, Josua Mayer wrote:

> >> +struct mvebu_cp110_utmi_port;
> > why forward declare and not move the structs instead?
> Seemed like the smaller change / more readable as diff.
> I can move the struct instead!

Yes and that can be preparatory as well :-)
 
> >
> >>  	reg &= ~(PLL_REFDIV_MASK | PLL_FBDIV_MASK | PLL_SEL_LPFR_MASK);
> >>  	reg |= (PLL_REFDIV_VAL << PLL_REFDIV_OFFSET) |
> >>  	       (PLL_FBDIV_VAL << PLL_FBDIV_OFFSET);
> >> -	writel(reg, PORT_REGS(port) + UTMI_PLL_CTRL_REG);
> >> +	writel(reg, port->regs + UTMI_PLL_CTRL_REG);
> >>  
> >>  	/* Impedance Calibration Threshold Setting */
> >> -	reg = readl(PORT_REGS(port) + UTMI_CAL_CTRL_REG);
> >> +	reg = readl(port->regs + UTMI_CAL_CTRL_REG);
> >>  	reg &= ~IMPCAL_VTH_MASK;
> >>  	reg |= IMPCAL_VTH_VAL << IMPCAL_VTH_OFFSET;
> >> -	writel(reg, PORT_REGS(port) + UTMI_CAL_CTRL_REG);
> >> +	writel(reg, port->regs + UTMI_CAL_CTRL_REG);
> >>  
> >>  	/* Set LS TX driver strength coarse control */
> >> -	reg = readl(PORT_REGS(port) + UTMI_TX_CH_CTRL_REG);
> >> +	reg = readl(port->regs + UTMI_TX_CH_CTRL_REG);
> >>  	reg &= ~TX_AMP_MASK;
> >>  	reg |= TX_AMP_VAL << TX_AMP_OFFSET;
> >> -	writel(reg, PORT_REGS(port) + UTMI_TX_CH_CTRL_REG);
> >> +	writel(reg, port->regs + UTMI_TX_CH_CTRL_REG);
> >>  
> >>  	/* Disable SQ and enable analog squelch detect */
> >> -	reg = readl(PORT_REGS(port) + UTMI_RX_CH_CTRL0_REG);
> >> +	reg = readl(port->regs + UTMI_RX_CH_CTRL0_REG);
> >>  	reg &= ~SQ_DET_EN;
> >>  	reg |= SQ_ANA_DTC_SEL;
> >> -	writel(reg, PORT_REGS(port) + UTMI_RX_CH_CTRL0_REG);
> >> +	writel(reg, port->regs + UTMI_RX_CH_CTRL0_REG);
> >>  
> >>  	/*
> >>  	 * Set External squelch calibration number and
> >>  	 * enable the External squelch calibration
> >>  	 */
> >> -	reg = readl(PORT_REGS(port) + UTMI_RX_CH_CTRL1_REG);
> >> +	reg = readl(port->regs + UTMI_RX_CH_CTRL1_REG);
> >>  	reg &= ~SQ_AMP_CAL_MASK;
> >>  	reg |= (SQ_AMP_CAL_VAL << SQ_AMP_CAL_OFFSET) | SQ_AMP_CAL_EN;
> >> -	writel(reg, PORT_REGS(port) + UTMI_RX_CH_CTRL1_REG);
> >> +	writel(reg, port->regs + UTMI_RX_CH_CTRL1_REG);
> >>  
> >>  	/*
> >>  	 * Set Control VDAT Reference Voltage - 0.325V and
> >>  	 * Control VSRC Reference Voltage - 0.6V
> >>  	 */
> >> -	reg = readl(PORT_REGS(port) + UTMI_CHGDTC_CTRL_REG);
> >> +	reg = readl(port->regs + UTMI_CHGDTC_CTRL_REG);
> >>  	reg &= ~(VDAT_MASK | VSRC_MASK);
> >>  	reg |= (VDAT_VAL << VDAT_OFFSET) | (VSRC_VAL << VSRC_OFFSET);
> >> -	writel(reg, PORT_REGS(port) + UTMI_CHGDTC_CTRL_REG);
> >> +	writel(reg, port->regs + UTMI_CHGDTC_CTRL_REG);
> >>  }
> >>  
> >>  static int mvebu_cp110_utmi_phy_power_off(struct phy *phy)
> >> @@ -166,22 +178,38 @@ static int mvebu_cp110_utmi_phy_power_off(struct phy *phy)
> >>  	struct mvebu_cp110_utmi_port *port = phy_get_drvdata(phy);
> >>  	struct mvebu_cp110_utmi *utmi = port->priv;
> >>  	int i;
> >> +	int reg;
> >>  
> >>  	/* Power down UTMI PHY port */
> >> -	regmap_clear_bits(utmi->syscon, SYSCON_UTMI_CFG_REG(port->id),
> >> -			  UTMI_PHY_CFG_PU_MASK);
> >> +	if (!IS_ERR(port->regs_cfg)) {
> >> +		reg = readl(port->regs_cfg);
> >> +		reg &= ~(UTMI_PHY_CFG_PU_MASK);
> >> +		writel(reg, port->regs_cfg);
> >> +	} else
> >> +		regmap_clear_bits(utmi->syscon, SYSCON_UTMI_CFG_REG(port->id),
> >> +				  UTMI_PHY_CFG_PU_MASK);
> > why are we doing both raw register read/write and regmap ops... that
> > does not sound correct to me
> Indeed.
> 
> The next question would be why for Armada 8K / CP110 syscon was chosen.
> From what I could find the registers accessed by utmi driver
> are not accessed by other drivers.
> 
> I am adding raw register access as an alternative,
> 
> to keep supporting old device-tree specifying syscon handle.
> 
> I considered writing helper functions for the if-not-error-syscon-else-raw,
> but between set_bits, clear_bits, global and per-port regs
> would have ended up with too many.

Aha, please add the comment so we know why it was done like this few
years later

-- 
~Vinod

[PATCH RFC v3 6/6] arm: dts: marvell: armada-388-clearfog: add description for usb phys

[Josua Mayer]

armada-38x.dtsi now has usb phy nodes for all 3 usb-2.0 ports.

Enable, and add references to the usb phys used by clearfog base and pro.
Explicitly set dr_mode to avoid phy driver warning messages during boot.

Finally replace the usb@58000 and usb3@f8000 nodes with references to
labels in armada-38x.dtsi.

Signed-off-by: Josua Mayer <josua@solid-run.com>
---
 .../boot/dts/marvell/armada-388-clearfog-base.dts  |  3 +++
 arch/arm/boot/dts/marvell/armada-388-clearfog.dts  |  3 +++
 arch/arm/boot/dts/marvell/armada-388-clearfog.dtsi | 30 ++++++++++++++--------
 3 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/dts/marvell/armada-388-clearfog-base.dts b/arch/arm/boot/dts/marvell/armada-388-clearfog-base.dts
index 308ad9d1c70f..3edb7988ee2e 100644
--- a/arch/arm/boot/dts/marvell/armada-388-clearfog-base.dts
+++ b/arch/arm/boot/dts/marvell/armada-388-clearfog-base.dts
@@ -102,5 +102,8 @@ rear_button_pins: rear-button-pins {
 
 /* SRDS #4 - USB-2.0/3.0 Host, M.2 */
 &usb3_0 {
+	phys = <&utmi1>;
+	phy-names = "utmi";
+	dr_mode = "host";
 	status = "okay";
 };
diff --git a/arch/arm/boot/dts/marvell/armada-388-clearfog.dts b/arch/arm/boot/dts/marvell/armada-388-clearfog.dts
index d6d7cc885f4d..4f5bb5867f20 100644
--- a/arch/arm/boot/dts/marvell/armada-388-clearfog.dts
+++ b/arch/arm/boot/dts/marvell/armada-388-clearfog.dts
@@ -185,5 +185,8 @@ &spi1 {
 
 /* USB-2.0 Host, CON2 - nearest CPU */
 &usb3_0 {
+	phys = <&utmi1>;
+	phy-names = "utmi";
+	dr_mode = "host";
 	status = "okay";
 };
diff --git a/arch/arm/boot/dts/marvell/armada-388-clearfog.dtsi b/arch/arm/boot/dts/marvell/armada-388-clearfog.dtsi
index f8a06ae4a3c9..0497fe13f56d 100644
--- a/arch/arm/boot/dts/marvell/armada-388-clearfog.dtsi
+++ b/arch/arm/boot/dts/marvell/armada-388-clearfog.dtsi
@@ -51,16 +51,6 @@ sdhci@d8000 {
 				vmmc-supply = <&reg_3p3v>;
 				wp-inverted;
 			};
-
-			usb@58000 {
-				/* CON3, nearest  power. */
-				status = "okay";
-			};
-
-			usb3@f8000 {
-				/* CON7 */
-				status = "okay";
-			};
 		};
 
 		pcie {
@@ -243,3 +233,23 @@ &uart1 {
 	pinctrl-names = "default";
 	status = "okay";
 };
+
+/* USB-2.0 Host, CON3 - nearest power */
+&usb0 {
+	phys = <&utmi0>;
+	phy-names = "utmi";
+	dr_mode = "host";
+	status = "okay";
+};
+
+/* SRDS #3 - USB-2.0/3.0 Host, Type-A connector */
+&usb3_1 {
+	phys = <&utmi2>;
+	phy-names = "utmi";
+	dr_mode = "host";
+	status = "okay";
+};
+
+&utmi {
+	status = "okay";
+};

-- 
2.43.0

Re: [PATCH RFC v3 0/6] phy: mvebu-cp110-utmi: add support for armada-380 utmi phys

[Rob Herring (Arm)]

On Sat, 20 Jul 2024 16:19:17 +0200, Josua Mayer wrote:
> Armada 380 has smilar USB-2.0 PHYs as CP-110 (Armada 8K).
> 
> Add support for Armada 380 to cp110 utmi phy driver, and enable it for
> armada-388-clearfog boards.
> 
> Additionally add a small bugfix for armada-388 clearfog:
> Enable Clearfog Base M.2 connector for cellular modems with USB-2.0/3.0
> interface.
> This is not separated out to avoid future merge conflicts.
> 
> Signed-off-by: Josua Mayer <josua@solid-run.com>
> ---
> Changes in v3:
> - updated bindings with additional comments, tested with dtbs_check:
>   used anyOf for the newly-added optional regs
> - added fix for clearfog base m.2 connector / enable third usb
> - dropped unnecessary syscon node using invalid compatible
>   (Reported-by: Krzysztof Kozlowski <krzk@kernel.org>)
> - Link to v2: https://lore.kernel.org/r/20240716-a38x-utmi-phy-v2-0-dae3a9c6ca3e@solid-run.com
> 
> Changes in v2:
> - add support for optional regs / make syscon use optional
> - add device-tree changes for armada-388-clearfog
> - attempted to fix warning reported by krobot (untested)
> - tested on actual hardware
> - drafted dt-bindings
> - Link to v1: https://lore.kernel.org/r/20240715-a38x-utmi-phy-v1-0-d57250f53cf2@solid-run.com
> 
> ---
> Josua Mayer (6):
>       arm: dts: marvell: armada-388-clearfog: enable third usb on m.2/mpcie
>       arm: dts: marvell: armada-388-clearfog-base: add rfkill for m.2
>       dt-bindings: phy: cp110-utmi-phy: add compatible string for armada-38x
>       arm: dts: marvell: armada-38x: add description for usb phys
>       phy: mvebu-cp110-utmi: add support for armada-380 utmi phys
>       arm: dts: marvell: armada-388-clearfog: add description for usb phys
> 
>  .../phy/marvell,armada-cp110-utmi-phy.yaml         |  34 +++-
>  .../boot/dts/marvell/armada-388-clearfog-base.dts  |  41 ++++
>  arch/arm/boot/dts/marvell/armada-388-clearfog.dts  |   8 +
>  arch/arm/boot/dts/marvell/armada-388-clearfog.dtsi |  30 ++-
>  arch/arm/boot/dts/marvell/armada-38x.dtsi          |  24 +++
>  drivers/phy/marvell/phy-mvebu-cp110-utmi.c         | 209 ++++++++++++++++-----
>  6 files changed, 288 insertions(+), 58 deletions(-)
> ---
> base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
> change-id: 20240715-a38x-utmi-phy-02e8059afe35
> 
> Sincerely,
> --
> Josua Mayer <josua@solid-run.com>
> 
> 
> 


My bot found new DTB warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


New warnings running 'make CHECK_DTBS=y marvell/armada-388-clearfog-base.dtb marvell/armada-388-clearfog.dtb' for 20240720-a38x-utmi-phy-v3-0-4c16f9abdbdc@solid-run.com:

arch/arm/boot/dts/marvell/armada-388-clearfog-base.dtb: usb@58000: phy-names:0: 'usb' was expected
	from schema $id: http://devicetree.org/schemas/usb/generic-ehci.yaml#
arch/arm/boot/dts/marvell/armada-388-clearfog.dtb: usb@58000: phy-names:0: 'usb' was expected
	from schema $id: http://devicetree.org/schemas/usb/generic-ehci.yaml#
arch/arm/boot/dts/marvell/armada-388-clearfog-base.dtb: usb3@f0000: Unevaluated properties are not allowed ('compatible', 'dr_mode', 'reg' were unexpected)
	from schema $id: http://devicetree.org/schemas/usb/generic-xhci.yaml#
arch/arm/boot/dts/marvell/armada-388-clearfog-base.dtb: usb3@f8000: Unevaluated properties are not allowed ('compatible', 'dr_mode', 'reg' were unexpected)
	from schema $id: http://devicetree.org/schemas/usb/generic-xhci.yaml#
arch/arm/boot/dts/marvell/armada-388-clearfog.dtb: usb3@f0000: Unevaluated properties are not allowed ('compatible', 'dr_mode', 'reg' were unexpected)
	from schema $id: http://devicetree.org/schemas/usb/generic-xhci.yaml#
arch/arm/boot/dts/marvell/armada-388-clearfog.dtb: usb3@f8000: Unevaluated properties are not allowed ('compatible', 'dr_mode', 'reg' were unexpected)
	from schema $id: http://devicetree.org/schemas/usb/generic-xhci.yaml#