[PATCH v3 05/10] dt-bindings: timer: Add HPE GXP Timer Binding

[PATCH v3 05/10] dt-bindings: timer: Add HPE GXP Timer Binding

[nick.hawkins]

From: Nick Hawkins <nick.hawkins@hpe.com>

Creating binding for gxp timer in device tree hpe,gxp-timer
Although there are multiple times on the SoC we are only
enabling one at this time.

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>

----

v2:
 *Removed maintainer change from patch
 *Verified there was no compilation errors
 *Added reference code in separate patch of patchset
---
 .../bindings/timer/hpe,gxp-timer.yaml         | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml

diff --git a/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml b/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
new file mode 100644
index 000000000000..1f4e345c5fb8
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/timer/hpe,gxp-timer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HPE GXP TIMER
+
+maintainers:
+  - Nick Hawkins <nick.hawkins@hpe.com>
+  - Jean-Marie Verdun <verdun@hpe.com>
+
+properties:
+  compatible:
+    const: hpe,gxp-timer
+
+  reg:
+    items:
+      - description: T0CNT register
+      - description: T0CS register
+      - description: TIMELO register
+
+  interrupts:
+    maxItems: 1
+
+  clock-frequency:
+    description: The frequency of the clock that drives the counter, in Hz.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clock-frequency
+
+additionalProperties: false
+
+examples:
+  - |
+    timer@10003000 {
+        compatible = "hpe,gxp-timer";
+        reg = <0xc0000080 0x1>, <0xc0000094 0x01>, <0xc0000088 0x08>;
+        interrupts = <0>;
+        interrupt-parent = <&vic0>;
+        clock-frequency = <400000000>;
+    };
-- 
2.17.1

Re: [PATCH v3 05/10] dt-bindings: timer: Add HPE GXP Timer Binding

[Krzysztof Kozlowski]

On 10/03/2022 20:52, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> Creating binding for gxp timer in device tree hpe,gxp-timer
> Although there are multiple times on the SoC we are only
> enabling one at this time.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> 
> ----
> 
> v2:
>  *Removed maintainer change from patch
>  *Verified there was no compilation errors
>  *Added reference code in separate patch of patchset
> ---
>  .../bindings/timer/hpe,gxp-timer.yaml         | 45 +++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
> 
> diff --git a/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml b/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
> new file mode 100644
> index 000000000000..1f4e345c5fb8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
> @@ -0,0 +1,45 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/timer/hpe,gxp-timer.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HPE GXP TIMER
> +
> +maintainers:
> +  - Nick Hawkins <nick.hawkins@hpe.com>
> +  - Jean-Marie Verdun <verdun@hpe.com>
> +
> +properties:
> +  compatible:
> +    const: hpe,gxp-timer
> +
> +  reg:
> +    items:
> +      - description: T0CNT register
> +      - description: T0CS register
> +      - description: TIMELO register
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clock-frequency:
> +    description: The frequency of the clock that drives the counter, in Hz.

Which clock is it? Generated inside the timer? If outside, why driver
does not take the reference to it and uses clk_get_rate()?

Best regards,
Krzysztof

Re: [PATCH v3 05/10] dt-bindings: timer: Add HPE GXP Timer Binding

[Rob Herring]

On Thu, Mar 10, 2022 at 01:52:24PM -0600, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> Creating binding for gxp timer in device tree hpe,gxp-timer
> Although there are multiple times on the SoC we are only
> enabling one at this time.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> 
> ----
> 
> v2:
>  *Removed maintainer change from patch
>  *Verified there was no compilation errors
>  *Added reference code in separate patch of patchset
> ---
>  .../bindings/timer/hpe,gxp-timer.yaml         | 45 +++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
> 
> diff --git a/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml b/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
> new file mode 100644
> index 000000000000..1f4e345c5fb8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
> @@ -0,0 +1,45 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/timer/hpe,gxp-timer.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HPE GXP TIMER
> +
> +maintainers:
> +  - Nick Hawkins <nick.hawkins@hpe.com>
> +  - Jean-Marie Verdun <verdun@hpe.com>
> +
> +properties:
> +  compatible:
> +    const: hpe,gxp-timer
> +
> +  reg:
> +    items:
> +      - description: T0CNT register
> +      - description: T0CS register
> +      - description: TIMELO register

Is the spec public to know what T0CNT, T0CS, and TIMELO are?

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clock-frequency:
> +    description: The frequency of the clock that drives the counter, in Hz.
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clock-frequency
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    timer@10003000 {
> +        compatible = "hpe,gxp-timer";
> +        reg = <0xc0000080 0x1>, <0xc0000094 0x01>, <0xc0000088 0x08>;

Based on the driver these are 4 bytes, 1 byte, 4 bytes in size.

Are there other registers in 0x80-0x95 range or do these offsets change 
in other chips? If not, just 1 entry covering the whole thing would be 
better.

> +        interrupts = <0>;
> +        interrupt-parent = <&vic0>;
> +        clock-frequency = <400000000>;
> +    };
> -- 
> 2.17.1
> 
> 

RE: [PATCH v3 05/10] dt-bindings: timer: Add HPE GXP Timer Binding

[Hawkins, Nick]

On Thu, Mar 10, 2022 at 01:52:24PM -0600, nick.hawkins@hpe.com wrote:
>> From: Nick Hawkins <nick.hawkins@hpe.com>>
>> 
>> Creating binding for gxp timer in device tree hpe,gxp-timer Although 
>> there are multiple times on the SoC we are only enabling one at this 
>> time.
>> 
>> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>>
>> 
>> ----
>> 
>> v2:
>>  *Removed maintainer change from patch  *Verified there was no 
>> compilation errors  *Added reference code in separate patch of 
>> patchset
>> ---
>>  .../bindings/timer/hpe,gxp-timer.yaml         | 45 +++++++++++++++++++
>>  1 file changed, 45 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml 
>> b/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
>> new file mode 100644
>> index 000000000000..1f4e345c5fb8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
>> @@ -0,0 +1,45 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
>> +---
>> +$id: 
>> +INVALID URI REMOVED
>> +xp-timer.yaml*__;Iw!!NpxR!yaItMPvjqEf3fKyp1xDQAzawRQDd8uDGTNKMlVPpn5Y
>> +56IUABMYbali7jonBl20K$
>> +$schema: 
>> +INVALID URI REMOVED
>> +aml*__;Iw!!NpxR!yaItMPvjqEf3fKyp1xDQAzawRQDd8uDGTNKMlVPpn5Y56IUABMYba
>> +li7jmX565-G$
>> +
>> +title: HPE GXP TIMER
>> +
>> +maintainers:
>> +  - Nick Hawkins <nick.hawkins@hpe.com>>
>> +  - Jean-Marie Verdun <verdun@hpe.com>>
>> +
>> +properties:
>> +  compatible:
>> +    const: hpe,gxp-timer
>> +
>> +  reg:
>> +    items:
>> +      - description: T0CNT register
>> +      - description: T0CS register
>> +      - description: TIMELO register

> Is the spec public to know what T0CNT, T0CS, and TIMELO are?
No it is not, should I not mention the register descriptions at all?

>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clock-frequency:
>> +    description: The frequency of the clock that drives the counter, in Hz.
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clock-frequency
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    timer@10003000 {
>> +        compatible = "hpe,gxp-timer";
>> +        reg = <0xc0000080 0x1>>, <0xc0000094 0x01>>, <0xc0000088 0x08>>;

> Based on the driver these are 4 bytes, 1 byte, 4 bytes in size.

> Are there other registers in 0x80-0x95 range or do these offsets change in other chips? If not, just 1 entry covering the whole thing would be better.
There are other registers in this range that cover different timers/clocks, for the most part between chip generations the offsets remain the same unless there is an architectural issue.
Can you provide a quick example of what one entry would be?

>> +        interrupts = <0>>;
>> +        interrupt-parent = <&vic0>>;
>> +        clock-frequency = <400000000>>;
>> +    };
>> --
>> 2.17.1
>> 
>> 

Regards,

Nick Hawkins 

Re: [PATCH v3 05/10] dt-bindings: timer: Add HPE GXP Timer Binding

[Krzysztof Kozlowski]

On 11/03/2022 17:22, Hawkins, Nick wrote:
> On Thu, Mar 10, 2022 at 01:52:24PM -0600, nick.hawkins@hpe.com wrote:
>>> From: Nick Hawkins <nick.hawkins@hpe.com>>
>>>
>>> Creating binding for gxp timer in device tree hpe,gxp-timer Although 
>>> there are multiple times on the SoC we are only enabling one at this 
>>> time.
>>>
>>> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>>
>>>
>>> ----
>>>
>>> v2:
>>>  *Removed maintainer change from patch  *Verified there was no 
>>> compilation errors  *Added reference code in separate patch of 
>>> patchset
>>> ---
>>>  .../bindings/timer/hpe,gxp-timer.yaml         | 45 +++++++++++++++++++
>>>  1 file changed, 45 insertions(+)
>>>  create mode 100644 
>>> Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml 
>>> b/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
>>> new file mode 100644
>>> index 000000000000..1f4e345c5fb8
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/timer/hpe,gxp-timer.yaml
>>> @@ -0,0 +1,45 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
>>> +---
>>> +$id: 
>>> +INVALID URI REMOVED
>>> +xp-timer.yaml*__;Iw!!NpxR!yaItMPvjqEf3fKyp1xDQAzawRQDd8uDGTNKMlVPpn5Y
>>> +56IUABMYbali7jonBl20K$
>>> +$schema: 
>>> +INVALID URI REMOVED
>>> +aml*__;Iw!!NpxR!yaItMPvjqEf3fKyp1xDQAzawRQDd8uDGTNKMlVPpn5Y56IUABMYba
>>> +li7jmX565-G$
>>> +
>>> +title: HPE GXP TIMER
>>> +
>>> +maintainers:
>>> +  - Nick Hawkins <nick.hawkins@hpe.com>>
>>> +  - Jean-Marie Verdun <verdun@hpe.com>>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: hpe,gxp-timer
>>> +
>>> +  reg:
>>> +    items:
>>> +      - description: T0CNT register
>>> +      - description: T0CS register
>>> +      - description: TIMELO register
> 
>> Is the spec public to know what T0CNT, T0CS, and TIMELO are?
> No it is not, should I not mention the register descriptions at all?
> 
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  clock-frequency:
>>> +    description: The frequency of the clock that drives the counter, in Hz.
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupts
>>> +  - clock-frequency
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    timer@10003000 {
>>> +        compatible = "hpe,gxp-timer";
>>> +        reg = <0xc0000080 0x1>>, <0xc0000094 0x01>>, <0xc0000088 0x08>>;
> 
>> Based on the driver these are 4 bytes, 1 byte, 4 bytes in size.
> 
>> Are there other registers in 0x80-0x95 range or do these offsets change in other chips? If not, just 1 entry covering the whole thing would be better.
> There are other registers in this range that cover different timers/clocks, for the most part between chip generations the offsets remain the same unless there is an architectural issue.
> Can you provide a quick example of what one entry would be?

arch/arm/boot/dts/versatile-ab.dts

and actually 90% of DTS... it's rather a challange to find such
fine-grained iomap.

Best regards,
Krzysztof

[PATCH v3 06/10] dt-bindings: watchdog: Add HPE GXP Watchdog timer binding

[nick.hawkins]

From: Nick Hawkins <nick.hawkins@hpe.com>

Add the hpe gxp watchdog timer binding hpe,gxp-wdt.
This will enable support for the HPE GXP Watchdog

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
---
 .../bindings/watchdog/hpe,gxp-wdt.yaml        | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml

diff --git a/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml b/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
new file mode 100644
index 000000000000..6044496b4968
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
@@ -0,0 +1,37 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/hpe,gxp-wdt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HPE GXP Controlled Watchdog
+
+allOf:
+  - $ref: "watchdog.yaml#"
+
+maintainers:
+  - Nick Hawkins <nick.hawkins@hpe.com>
+  - Jean-Marie Verdun <verdun@hpe.com>
+
+properties:
+  compatible:
+    const: hpe,gxp-wdt
+
+  reg:
+    items:
+      - description: WDGRST register
+      - description: WDGCS register
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    watchdog0:  watchdog@c0000090 {
+      compatible = "hpe,gxp-wdt";
+      reg = <0xc0000090 0x02>, <0xc0000096 0x01>;
+    };
+
-- 
2.17.1

Re: [PATCH v3 06/10] dt-bindings: watchdog: Add HPE GXP Watchdog timer binding

[Krzysztof Kozlowski]

On 10/03/2022 20:52, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> Add the hpe gxp watchdog timer binding hpe,gxp-wdt.
> This will enable support for the HPE GXP Watchdog
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  .../bindings/watchdog/hpe,gxp-wdt.yaml        | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml b/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
> new file mode 100644
> index 000000000000..6044496b4968
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/hpe,gxp-wdt.yaml
> @@ -0,0 +1,37 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/watchdog/hpe,gxp-wdt.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HPE GXP Controlled Watchdog
> +
> +allOf:
> +  - $ref: "watchdog.yaml#"
> +
> +maintainers:
> +  - Nick Hawkins <nick.hawkins@hpe.com>
> +  - Jean-Marie Verdun <verdun@hpe.com>
> +
> +properties:
> +  compatible:
> +    const: hpe,gxp-wdt
> +
> +  reg:
> +    items:
> +      - description: WDGRST register
> +      - description: WDGCS register

Why are you mapping each register instead of entire address space? It
won't scale if your new design comes with 10 registers...

> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    watchdog0:  watchdog@c0000090 {

Skip the label. You also have there double space between label and node
name.

Best regards,
Krzysztof

[PATCH v3 07/10] dt-bindings: arm: Add HPE GXP Binding

[nick.hawkins]

From: Nick Hawkins <nick.hawkins@hpe.com>

This adds support for the hpe,gxp binding. The GXP is based on
the cortex a9 processor and supports arm7.

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
---
 .../devicetree/bindings/arm/gxp.yaml          | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/gxp.yaml

diff --git a/Documentation/devicetree/bindings/arm/gxp.yaml b/Documentation/devicetree/bindings/arm/gxp.yaml
new file mode 100644
index 000000000000..edfd331c493e
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/gxp.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/gxp.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HPE bmc GXP SoC driver
+
+maintainers:
+  - Nick Hawkins <nick.hawkins@hpe.com>
+  - Jean-Marie Verdun <verdun@hpe.com>
+
+properties:
+  compatible:
+    const: hpe,gxp
+
+  "#address-cells":
+    const: 1
+
+required:
+  - compatible
+
+additionalProperties: true
+
+examples:
+  - |
+      / {
+          model = "Hewlett Packard Enterprise GXP BMC";
+          compatible = "hpe,gxp";
+          #address-cells = <1>;
+          #size-cells = <1>;
+          cpus {
+              #address-cells = <1>;
+              #size-cells = <0>;
+
+              CPU0: cpu@0 {
+                  compatible = "arm,cortex-a9";
+                  device_type = "cpu";
+                  reg = <0>;
+              };
+          };
+
+          memory@40000000 {
+            device_type = "memory";
+            reg = <0x40000000 0x20000000>;
+          };
+
+          ahb {
+
+          };
+      };
+
+...
-- 
2.17.1

Re: [PATCH v3 07/10] dt-bindings: arm: Add HPE GXP Binding

[Krzysztof Kozlowski]

On 10/03/2022 20:52, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> This adds support for the hpe,gxp binding. The GXP is based on
> the cortex a9 processor and supports arm7.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  .../devicetree/bindings/arm/gxp.yaml          | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/gxp.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/gxp.yaml b/Documentation/devicetree/bindings/arm/gxp.yaml
> new file mode 100644
> index 000000000000..edfd331c493e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/gxp.yaml

Missing vendor prefix in file name, so "hpe,gxp.yaml"

> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/gxp.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HPE bmc GXP SoC driver

Driver does not fit here. I think you are adding HPE GXP platforms? If yes

> +
> +maintainers:
> +  - Nick Hawkins <nick.hawkins@hpe.com>
> +  - Jean-Marie Verdun <verdun@hpe.com>
> +
> +properties:
> +  compatible:
> +    const: hpe,gxp

This is not proper SoC/boards description. Look at intel,keembay.yaml
for example.

You also miss here other compatibles - for board(s).

> +
> +  "#address-cells":
> +    const: 1
> +
> +required:
> +  - compatible
> +
> +additionalProperties: true
> +
> +examples:
> +  - |

Skip the example, platform bindings do not have them.

Best regards,
Krzysztof

[PATCH v3 08/10] dt-bindings: arm: Add HPE GXP CPU Init

[nick.hawkins]

From: Nick Hawkins <nick.hawkins@hpe.com>

This adds support for the hpe,gxp-cpu-init binding.
With the HPE GXP initialization it is necessary to have access to this
register for it to boot.

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
---
 .../cpu-enable-method/hpe,gxp-cpu-init.yaml   | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/cpu-enable-method/hpe,gxp-cpu-init.yaml

diff --git a/Documentation/devicetree/bindings/arm/cpu-enable-method/hpe,gxp-cpu-init.yaml b/Documentation/devicetree/bindings/arm/cpu-enable-method/hpe,gxp-cpu-init.yaml
new file mode 100644
index 000000000000..a17c3fcc5421
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/cpu-enable-method/hpe,gxp-cpu-init.yaml
@@ -0,0 +1,31 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/cpu-enable-method/hpe,gxp-cpu-init.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Initialize GXP CPU
+
+maintainers:
+  - Nick Hawkins <nick.hawkins@hpe.com>
+  - Jean-Marie Verdun <verdun@hpe.com>
+
+properties:
+  compatible:
+    const: hpe,gxp-cpu-init
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    gxp-init@cefe0010 {
+      compatible = "hpe,gxp-cpu-init";
+      reg = <0xcefe0010 0x04>;
+    };
-- 
2.17.1

Re: [PATCH v3 08/10] dt-bindings: arm: Add HPE GXP CPU Init

[Krzysztof Kozlowski]

On 10/03/2022 20:52, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> This adds support for the hpe,gxp-cpu-init binding.
> With the HPE GXP initialization it is necessary to have access to this
> register for it to boot.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  .../cpu-enable-method/hpe,gxp-cpu-init.yaml   | 31 +++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/cpu-enable-method/hpe,gxp-cpu-init.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/cpu-enable-method/hpe,gxp-cpu-init.yaml b/Documentation/devicetree/bindings/arm/cpu-enable-method/hpe,gxp-cpu-init.yaml
> new file mode 100644
> index 000000000000..a17c3fcc5421
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/cpu-enable-method/hpe,gxp-cpu-init.yaml
> @@ -0,0 +1,31 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/cpu-enable-method/hpe,gxp-cpu-init.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Initialize GXP CPU

Please explain what's this. The bindings describe the hardware and I
cannot get what is this here about. Is it a CPU enable method (like in
cpus.yaml)? Is it some power management block?


Best regards,
Krzysztof

RE: [PATCH v3 08/10] dt-bindings: arm: Add HPE GXP CPU Init

[Hawkins, Nick]

-----Original Message-----
From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@canonical.com] 
Sent: Friday, March 11, 2022 4:22 AM
To: Hawkins, Nick <nick.hawkins@hpe.com>>; Verdun, Jean-Marie <verdun@hpe.com>>
Cc: Rob Herring <robh+dt@kernel.org>>; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 08/10] dt-bindings: arm: Add HPE GXP CPU Init

On 10/03/2022 20:52, nick.hawkins@hpe.com wrote:
>> From: Nick Hawkins <nick.hawkins@hpe.com>>
>> 
>> This adds support for the hpe,gxp-cpu-init binding.
>> With the HPE GXP initialization it is necessary to have access to this 
>> register for it to boot.
>> 
>> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>>
>> ---
>>  .../cpu-enable-method/hpe,gxp-cpu-init.yaml   | 31 +++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/arm/cpu-enable-method/hpe,gxp-cpu-in
>> it.yaml
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/arm/cpu-enable-method/hpe,gxp-cpu-
>> init.yaml 
>> b/Documentation/devicetree/bindings/arm/cpu-enable-method/hpe,gxp-cpu-
>> init.yaml
>> new file mode 100644
>> index 000000000000..a17c3fcc5421
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/cpu-enable-method/hpe,gxp-
>> +++ cpu-init.yaml
>> @@ -0,0 +1,31 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
>> +---
>> +$id: 
>> +INVALID URI REMOVED
>> +ble-method/hpe,gxp-cpu-init.yaml*__;Iw!!NpxR!110gfGDYqJKEHiiJCXcAxkJJ
>> +uv62JykkON1xXN-Gdv6p4hn6fm7y8WHCy6w2GSFt$
>> +$schema: 
>> +INVALID URI REMOVED
>> +aml*__;Iw!!NpxR!110gfGDYqJKEHiiJCXcAxkJJuv62JykkON1xXN-Gdv6p4hn6fm7y8
>> +WHCy25gpjJ0$
>> +
>> +title: Initialize GXP CPU

> Please explain what's this. The bindings describe the hardware and I cannot get what is this here about. Is it a CPU enable method (like in cpus.yaml)? Is it some power management block?

This was intended to be a binding that would provide the necessary register to boot the asic. I have now moved this functionality into our bootloader. Thus this binding is no longer needed and will be removed from subsequent versions of this patch series.

Thanks,

-Nick

[PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[nick.hawkins]

From: Nick Hawkins <nick.hawkins@hpe.com>

The HPE SoC is new to linux. This patch
creates the basic device tree layout with minimum required
for linux to boot. This includes timer and watchdog
support.

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
---
 arch/arm/boot/dts/Makefile               |   2 +
 arch/arm/boot/dts/hpe-bmc-dl360gen10.dts |  27 +++++
 arch/arm/boot/dts/hpe-gxp.dtsi           | 148 +++++++++++++++++++++++
 3 files changed, 177 insertions(+)
 create mode 100644 arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
 create mode 100644 arch/arm/boot/dts/hpe-gxp.dtsi

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index e41eca79c950..2823b359d373 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -1550,3 +1550,5 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
 	aspeed-bmc-vegman-n110.dtb \
 	aspeed-bmc-vegman-rx20.dtb \
 	aspeed-bmc-vegman-sx20.dtb
+dtb-$(CONFIG_ARCH_HPE_GXP) += \
+	hpe-bmc-dl360gen10.dtb
diff --git a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
new file mode 100644
index 000000000000..da5eac1213a8
--- /dev/null
+++ b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device Tree file for HPE DL360Gen10
+ */
+
+/include/ "hpe-gxp.dtsi"
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	compatible = "hpe,gxp";
+	model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
+
+	chosen {
+		bootargs = "earlyprintk console=ttyS2,115200";
+	};
+
+	memory@40000000 {
+		device_type = "memory";
+		reg = <0x40000000 0x20000000>;
+	};
+
+	ahb {
+
+	};
+
+};
diff --git a/arch/arm/boot/dts/hpe-gxp.dtsi b/arch/arm/boot/dts/hpe-gxp.dtsi
new file mode 100644
index 000000000000..dfaf8df829fe
--- /dev/null
+++ b/arch/arm/boot/dts/hpe-gxp.dtsi
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device Tree file for HPE GXP
+ */
+
+/dts-v1/;
+/ {
+	model = "Hewlett Packard Enterprise GXP BMC";
+	compatible = "hpe,gxp";
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu@0 {
+			compatible = "arm,cortex-a9";
+			device_type = "cpu";
+			reg = <0>;
+		};
+	};
+
+	gxp-init@cefe0010 {
+		compatible = "hpe,gxp-cpu-init";
+		reg = <0xcefe0010 0x04>;
+	};
+
+	memory@40000000 {
+		device_type = "memory";
+		reg = <0x40000000 0x20000000>;
+	};
+
+	ahb {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		device_type = "soc";
+		ranges;
+
+		vic0: interrupt-controller@ceff0000 {
+			compatible = "arm,pl192-vic";
+			interrupt-controller;
+			reg = <0xceff0000 0x1000>;
+			#interrupt-cells = <1>;
+		};
+
+		vic1: interrupt-controller@80f00000 {
+			compatible = "arm,pl192-vic";
+			interrupt-controller;
+			reg = <0x80f00000 0x1000>;
+			#interrupt-cells = <1>;
+		};
+
+		timer0: timer@c0000080 {
+			compatible = "hpe,gxp-timer";
+			reg = <0xc0000080 0x1>, <0xc0000094 0x01>, <0xc0000088 0x08>;
+			interrupts = <0>;
+			interrupt-parent = <&vic0>;
+			clock-frequency = <400000000>;
+		};
+
+		uarta: serial@c00000e0 {
+			compatible = "ns16550a";
+			reg = <0xc00000e0 0x8>;
+			interrupts = <17>;
+			interrupt-parent = <&vic0>;
+			clock-frequency = <1846153>;
+			reg-shift = <0>;
+		};
+
+		uartb: serial@c00000e8 {
+			compatible = "ns16550a";
+			reg = <0xc00000e8 0x8>;
+			interrupts = <18>;
+			interrupt-parent = <&vic0>;
+			clock-frequency = <1846153>;
+			reg-shift = <0>;
+		};
+
+		uartc: serial@c00000f0 {
+			compatible = "ns16550a";
+			reg = <0xc00000f0 0x8>;
+			interrupts = <19>;
+			interrupt-parent = <&vic0>;
+			clock-frequency = <1846153>;
+			reg-shift = <0>;
+		};
+
+		usb0: usb@cefe0000 {
+			compatible = "generic-ehci";
+			reg = <0xcefe0000 0x100>;
+			interrupts = <7>;
+			interrupt-parent = <&vic0>;
+		};
+
+		usb1: usb@cefe0100 {
+			compatible = "generic-ohci";
+			reg = <0xcefe0100 0x110>;
+			interrupts = <6>;
+			interrupt-parent = <&vic0>;
+		};
+
+		vrom@58000000 {
+			compatible = "mtd-ram";
+			bank-width = <4>;
+			reg = <0x58000000 0x4000000>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			partition@0 {
+				label = "vrom-prime";
+				reg = <0x0 0x2000000>;
+			};
+			partition@2000000 {
+				label = "vrom-second";
+				reg = <0x2000000 0x2000000>;
+			};
+		};
+
+		i2cg: syscon@c00000f8 {
+			compatible = "simple-mfd", "syscon";
+			reg = <0xc00000f8 0x08>;
+		};
+	};
+
+	clocks {
+		osc: osc {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-output-names = "osc";
+			clock-frequency = <33333333>;
+		};
+
+		iopclk: iopclk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-output-names = "iopclk";
+			clock-frequency = <400000000>;
+		};
+
+		memclk: memclk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-output-names = "memclk";
+			clock-frequency = <800000000>;
+		};
+	};
+};
-- 
2.17.1

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Arnd Bergmann]

On Thu, Mar 10, 2022 at 8:52 PM <nick.hawkins@hpe.com> wrote:
>
> From: Nick Hawkins <nick.hawkins@hpe.com>
>
> The HPE SoC is new to linux. This patch
> creates the basic device tree layout with minimum required
> for linux to boot. This includes timer and watchdog
> support.
>
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> +/ {
> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +       compatible = "hpe,gxp";
> +       model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
> +
> +       chosen {
> +               bootargs = "earlyprintk console=ttyS2,115200";
> +       };

Please drop the bootargs here, you definitely should not have 'earlyprintk'
in the bootargs because that is incompatible with cross-platform kernels.

Instead of passing the console in the bootargs, use the "stdout-path"
property.

The "compatible" property should be a list that contains at least the specific
SoC variant and an identifier for the board. "hpe,gxp" is way too generic
to be the only property here.
> +       gxp-init@cefe0010 {
> +               compatible = "hpe,gxp-cpu-init";
> +               reg = <0xcefe0010 0x04>;
> +       };
> +
> +       memory@40000000 {
> +               device_type = "memory";
> +               reg = <0x40000000 0x20000000>;
> +       };
> +
> +       ahb {
> +               compatible = "simple-bus";
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               device_type = "soc";
> +               ranges;
> +
> +               vic0: interrupt-controller@ceff0000 {
> +                       compatible = "arm,pl192-vic";
> +                       interrupt-controller;
> +                       reg = <0xceff0000 0x1000>;
> +                       #interrupt-cells = <1>;
> +               };

I don't think this represents the actual hierarchy of the devices:
the register range of the "vic0" and the "gxp-init" is very close
together, which usually indicates that they are also on the same
bus.



> +               vic1: interrupt-controller@80f00000 {
> +                       compatible = "arm,pl192-vic";
> +                       interrupt-controller;
> +                       reg = <0x80f00000 0x1000>;
> +                       #interrupt-cells = <1>;
> +               };
> +
> +               timer0: timer@c0000080 {
> +                       compatible = "hpe,gxp-timer";
> +                       reg = <0xc0000080 0x1>, <0xc0000094 0x01>, <0xc0000088 0x08>;
> +                       interrupts = <0>;
> +                       interrupt-parent = <&vic0>;
> +                       clock-frequency = <400000000>;
> +               };
> +
> +               uarta: serial@c00000e0 {
> +                       compatible = "ns16550a";
> +                       reg = <0xc00000e0 0x8>;
> +                       interrupts = <17>;
> +                       interrupt-parent = <&vic0>;
> +                       clock-frequency = <1846153>;
> +                       reg-shift = <0>;
> +               };

In turn, you seem to have a lot of other devices on low addresses
of the 0xc0000000 range, which would be an indication that these
are on a different bus from the others.

Please see if you can find an internal datasheet that describes the
actual device hierarchy, and then try to model this in the device tree.

Use non-empty "ranges" properties to describe the address ranges
and how they get translated between these buses, and add
"dma-ranges" for any high-speed buses that have DMA master
capable devices like USB on them.

> +               i2cg: syscon@c00000f8 {
> +                       compatible = "simple-mfd", "syscon";
> +                       reg = <0xc00000f8 0x08>;
> +               };
> +       };

It's odd to have a "syscon" device that only has 8 bytes worth of registers.

What are the contents of this? Is it possible to have a proper abstraction
for it as something that has a specific purpose rather than being a
random collection of individual bits?

       Arnd

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Krzysztof Kozlowski]

On 10/03/2022 20:52, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> The HPE SoC is new to linux. This patch
> creates the basic device tree layout with minimum required
> for linux to boot. This includes timer and watchdog
> support.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  arch/arm/boot/dts/Makefile               |   2 +
>  arch/arm/boot/dts/hpe-bmc-dl360gen10.dts |  27 +++++
>  arch/arm/boot/dts/hpe-gxp.dtsi           | 148 +++++++++++++++++++++++
>  3 files changed, 177 insertions(+)
>  create mode 100644 arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>  create mode 100644 arch/arm/boot/dts/hpe-gxp.dtsi
> 
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index e41eca79c950..2823b359d373 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1550,3 +1550,5 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>  	aspeed-bmc-vegman-n110.dtb \
>  	aspeed-bmc-vegman-rx20.dtb \
>  	aspeed-bmc-vegman-sx20.dtb
> +dtb-$(CONFIG_ARCH_HPE_GXP) += \
> +	hpe-bmc-dl360gen10.dtb

Alphabetically, also in respect to other architectures, so before
CONFIG_ARCH_INTEGRATOR.

> diff --git a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
> new file mode 100644
> index 000000000000..da5eac1213a8
> --- /dev/null
> +++ b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
> @@ -0,0 +1,27 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree file for HPE DL360Gen10
> + */
> +
> +/include/ "hpe-gxp.dtsi"
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	compatible = "hpe,gxp";

Missing board compatible.

> +	model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
> +
> +	chosen {
> +		bootargs = "earlyprintk console=ttyS2,115200";

I have impression we talked about it...

> +	};
> +
> +	memory@40000000 {
> +		device_type = "memory";
> +		reg = <0x40000000 0x20000000>;
> +	};
> +
> +	ahb {

Why do you need empty node?

> +
> +	};
> +
> +};
> diff --git a/arch/arm/boot/dts/hpe-gxp.dtsi b/arch/arm/boot/dts/hpe-gxp.dtsi
> new file mode 100644
> index 000000000000..dfaf8df829fe
> --- /dev/null
> +++ b/arch/arm/boot/dts/hpe-gxp.dtsi
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree file for HPE GXP
> + */
> +
> +/dts-v1/;
> +/ {
> +	model = "Hewlett Packard Enterprise GXP BMC";
> +	compatible = "hpe,gxp";
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu@0 {
> +			compatible = "arm,cortex-a9";
> +			device_type = "cpu";
> +			reg = <0>;
> +		};
> +	};
> +
> +	gxp-init@cefe0010 {

Need a generic node name. gpx-init is specific.

> +		compatible = "hpe,gxp-cpu-init";
> +		reg = <0xcefe0010 0x04>;
> +	};
> +
> +	memory@40000000 {
> +		device_type = "memory";
> +		reg = <0x40000000 0x20000000>;
> +	};
> +
> +	ahb {

By convention we call it soc.

> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		device_type = "soc";
> +		ranges;
> +
> +		vic0: interrupt-controller@ceff0000 {
> +			compatible = "arm,pl192-vic";
> +			interrupt-controller;
> +			reg = <0xceff0000 0x1000>;

Please put reg after compatible, everywhere.

> +			#interrupt-cells = <1>;
> +		};
> +
> +		vic1: interrupt-controller@80f00000 {
> +			compatible = "arm,pl192-vic";
> +			interrupt-controller;
> +			reg = <0x80f00000 0x1000>;
> +			#interrupt-cells = <1>;
> +		};
> +
> +		timer0: timer@c0000080 {
> +			compatible = "hpe,gxp-timer";
> +			reg = <0xc0000080 0x1>, <0xc0000094 0x01>, <0xc0000088 0x08>;
> +			interrupts = <0>;
> +			interrupt-parent = <&vic0>;
> +			clock-frequency = <400000000>;
> +		};
> +
> +		uarta: serial@c00000e0 {
> +			compatible = "ns16550a";
> +			reg = <0xc00000e0 0x8>;
> +			interrupts = <17>;
> +			interrupt-parent = <&vic0>;
> +			clock-frequency = <1846153>;
> +			reg-shift = <0>;
> +		};
> +
> +		uartb: serial@c00000e8 {
> +			compatible = "ns16550a";
> +			reg = <0xc00000e8 0x8>;
> +			interrupts = <18>;
> +			interrupt-parent = <&vic0>;
> +			clock-frequency = <1846153>;
> +			reg-shift = <0>;
> +		};
> +
> +		uartc: serial@c00000f0 {
> +			compatible = "ns16550a";
> +			reg = <0xc00000f0 0x8>;
> +			interrupts = <19>;
> +			interrupt-parent = <&vic0>;
> +			clock-frequency = <1846153>;
> +			reg-shift = <0>;
> +		};
> +
> +		usb0: usb@cefe0000 {
> +			compatible = "generic-ehci";

I think one of previous comments was that you cannot have "generic-ehci"
only, right?

> +			reg = <0xcefe0000 0x100>;
> +			interrupts = <7>;
> +			interrupt-parent = <&vic0>;
> +		};
> +
> +		usb1: usb@cefe0100 {
> +			compatible = "generic-ohci";
> +			reg = <0xcefe0100 0x110>;
> +			interrupts = <6>;
> +			interrupt-parent = <&vic0>;
> +		};
> +
> +		vrom@58000000 {
> +			compatible = "mtd-ram";
> +			bank-width = <4>;
> +			reg = <0x58000000 0x4000000>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			partition@0 {
> +				label = "vrom-prime";
> +				reg = <0x0 0x2000000>;
> +			};
> +			partition@2000000 {
> +				label = "vrom-second";
> +				reg = <0x2000000 0x2000000>;
> +			};
> +		};
> +
> +		i2cg: syscon@c00000f8 {


> +			compatible = "simple-mfd", "syscon";
> +			reg = <0xc00000f8 0x08>;
> +		};
> +	};
> +
> +	clocks {
> +		osc: osc {

Keep node naming consistent, so just "clk"... but it's also very generic
comparing to others, so I wonder what is this clock?

> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-output-names = "osc";
> +			clock-frequency = <33333333>;
> +		};
> +
> +		iopclk: iopclk {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-output-names = "iopclk";
> +			clock-frequency = <400000000>;
> +		};
> +
> +		memclk: memclk {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-output-names = "memclk";
> +			clock-frequency = <800000000>;
> +		};

What are these clocks? If external to the SoC, then where are they? On
the board?

> +	};
> +};


Best regards,
Krzysztof

RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

-----Original Message-----
From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@canonical.com]
Sent: Friday, March 11, 2022 4:30 AM
To: Hawkins, Nick <nick.hawkins@hpe.com>>; Verdun, Jean-Marie <verdun@hpe.com>>
Cc: Arnd Bergmann <arnd@arndb.de>>; Olof Johansson <olof@lixom.net>>; soc@kernel.org; Rob Herring <robh+dt@kernel.org>>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

On 10/03/2022 20:52, nick.hawkins@hpe.com wrote:
>> From: Nick Hawkins <nick.hawkins@hpe.com>>
>> 
>> The HPE SoC is new to linux. This patch creates the basic device tree 
>> layout with minimum required for linux to boot. This includes timer 
>> and watchdog support.
>> 
>> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>>
>> ---
>>  arch/arm/boot/dts/Makefile               |   2 +
>>  arch/arm/boot/dts/hpe-bmc-dl360gen10.dts |  27 +++++
>>  arch/arm/boot/dts/hpe-gxp.dtsi           | 148 +++++++++++++++++++++++
>>  3 files changed, 177 insertions(+)
>>  create mode 100644 arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>  create mode 100644 arch/arm/boot/dts/hpe-gxp.dtsi
>> 
>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile 
>> index e41eca79c950..2823b359d373 100644
>> --- a/arch/arm/boot/dts/Makefile
>> +++ b/arch/arm/boot/dts/Makefile
>> @@ -1550,3 +1550,5 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>>  	aspeed-bmc-vegman-n110.dtb \
>>  	aspeed-bmc-vegman-rx20.dtb \
>>  	aspeed-bmc-vegman-sx20.dtb
>> +dtb-$(CONFIG_ARCH_HPE_GXP) += \
>> +	hpe-bmc-dl360gen10.dtb

> Alphabetically, also in respect to other architectures, so before CONFIG_ARCH_INTEGRATOR.

Done

>> diff --git a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>> b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>> new file mode 100644
>> index 000000000000..da5eac1213a8
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>> @@ -0,0 +1,27 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Device Tree file for HPE DL360Gen10  */
>> +
>> +/include/ "hpe-gxp.dtsi"
>> +
>> +/ {
>> +	#address-cells = <1>>;
>> +	#size-cells = <1>>;
>> +	compatible = "hpe,gxp";

> Missing board compatible.

Will become compatible =  "hpe,gxp","hpe,bmc-dl360gen10"; If that seems okay to you.

>> +	model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
>> +
>> +	chosen {
>> +		bootargs = "earlyprintk console=ttyS2,115200";

> I have impression we talked about it...

Removed!

>> +	};
>> +
>> +	memory@40000000 {
>> +		device_type = "memory";
>> +		reg = <0x40000000 0x20000000>>;
>> +	};
>> +
>> +	ahb {

> Why do you need empty node?

I do not, this was a placeholder for after the initial checkin where I would put all the board specific stuff. This has been removed.

>> +
>> +	};
>> +
>> +};
>> diff --git a/arch/arm/boot/dts/hpe-gxp.dtsi 
>> b/arch/arm/boot/dts/hpe-gxp.dtsi new file mode 100644 index 
>> 000000000000..dfaf8df829fe
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/hpe-gxp.dtsi
>> @@ -0,0 +1,148 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Device Tree file for HPE GXP
>> + */
>> +
>> +/dts-v1/;
>> +/ {
>> +	model = "Hewlett Packard Enterprise GXP BMC";
>> +	compatible = "hpe,gxp";
>> +	#address-cells = <1>>;
>> +	#size-cells = <1>>;
>> +
>> +	cpus {
>> +		#address-cells = <1>>;
>> +		#size-cells = <0>>;
>> +
>> +		cpu@0 {
>> +			compatible = "arm,cortex-a9";
>> +			device_type = "cpu";
>> +			reg = <0>>;
>> +		};
>> +	};
>> +
>> +	gxp-init@cefe0010 {

> Need a generic node name. gpx-init is specific.

Will do. But more than likely this is going to get removed as I try to push this option into the bootloader.

>> +		compatible = "hpe,gxp-cpu-init";
>> +		reg = <0xcefe0010 0x04>>;
>> +	};
>> +
>> +	memory@40000000 {
>> +		device_type = "memory";
>> +		reg = <0x40000000 0x20000000>>;
>> +	};
>> +
>> +	ahb {

> By convention we call it soc.

>> +		compatible = "simple-bus";
>> +		#address-cells = <1>>;
>> +		#size-cells = <1>>;
>> +		device_type = "soc";
>> +		ranges;
>> +
>> +		vic0: interrupt-controller@ceff0000 {
>> +			compatible = "arm,pl192-vic";
>> +			interrupt-controller;
>> +			reg = <0xceff0000 0x1000>>;

> Please put reg after compatible, everywhere.

Done

>> +			#interrupt-cells = <1>>;
>> +		};
>> +
>> +		vic1: interrupt-controller@80f00000 {
>> +			compatible = "arm,pl192-vic";
>> +			interrupt-controller;
>> +			reg = <0x80f00000 0x1000>>;
>> +			#interrupt-cells = <1>>;
>> +		};
>> +
>> +		timer0: timer@c0000080 {
>> +			compatible = "hpe,gxp-timer";
>> +			reg = <0xc0000080 0x1>>, <0xc0000094 0x01>>, <0xc0000088 0x08>>;
>> +			interrupts = <0>>;
>> +			interrupt-parent = <&vic0>>;
>> +			clock-frequency = <400000000>>;
>> +		};
>> +
>> +		uarta: serial@c00000e0 {
>> +			compatible = "ns16550a";
>> +			reg = <0xc00000e0 0x8>>;
>> +			interrupts = <17>>;
>> +			interrupt-parent = <&vic0>>;
>> +			clock-frequency = <1846153>>;
>> +			reg-shift = <0>>;
>> +		};
>> +
>> +		uartb: serial@c00000e8 {
>> +			compatible = "ns16550a";
>> +			reg = <0xc00000e8 0x8>>;
>> +			interrupts = <18>>;
>> +			interrupt-parent = <&vic0>>;
>> +			clock-frequency = <1846153>>;
>> +			reg-shift = <0>>;
>> +		};
>> +
>> +		uartc: serial@c00000f0 {
>> +			compatible = "ns16550a";
>> +			reg = <0xc00000f0 0x8>>;
>> +			interrupts = <19>>;
>> +			interrupt-parent = <&vic0>>;
>> +			clock-frequency = <1846153>>;
>> +			reg-shift = <0>>;
>> +		};
>> +
>> +		usb0: usb@cefe0000 {
>> +			compatible = "generic-ehci";

> I think one of previous comments was that you cannot have "generic-ehci"
> only, right?

Yes there was, I removed the usb0: ehci@cefe0000. I see now that this is in reference to the compatible. This is our ehci controller. What would be a more appropriate compatible? Do we need hpe,gxp-ehci perhaps?

>> +			reg = <0xcefe0000 0x100>>;
>> +			interrupts = <7>>;
>> +			interrupt-parent = <&vic0>>;
>> +		};
>> +
>> +		usb1: usb@cefe0100 {
>> +			compatible = "generic-ohci";
>> +			reg = <0xcefe0100 0x110>>;
>> +			interrupts = <6>>;
>> +			interrupt-parent = <&vic0>>;
>> +		};
>> +
>> +		vrom@58000000 {
>> +			compatible = "mtd-ram";
>> +			bank-width = <4>>;
>> +			reg = <0x58000000 0x4000000>>;
>> +			#address-cells = <1>>;
>> +			#size-cells = <1>>;
>> +			partition@0 {
>> +				label = "vrom-prime";
>> +				reg = <0x0 0x2000000>>;
>> +			};
>> +			partition@2000000 {
>> +				label = "vrom-second";
>> +				reg = <0x2000000 0x2000000>>;
>> +			};
>> +		};
>> +
>> +		i2cg: syscon@c00000f8 {


>> +			compatible = "simple-mfd", "syscon";
>> +			reg = <0xc00000f8 0x08>>;
>> +		};
>> +	};
>> +
>> +	clocks {
>> +		osc: osc {

> Keep node naming consistent, so just "clk"... but it's also very generic comparing to others, so I wonder what is this clock?

We are in the process of redoing the clocks. This was the oscillator but no longer needed for the minimum boot config.

>> +			compatible = "fixed-clock";
>> +			#clock-cells = <0>>;
>> +			clock-output-names = "osc";
>> +			clock-frequency = <33333333>>;
>> +		};
>> +
>> +		iopclk: iopclk {
>> +			compatible = "fixed-clock";
>> +			#clock-cells = <0>>;
>> +			clock-output-names = "iopclk";
>> +			clock-frequency = <400000000>>;
>> +		};
>> +
>> +		memclk: memclk {
>> +			compatible = "fixed-clock";
>> +			#clock-cells = <0>>;
>> +			clock-output-names = "memclk";
>> +			clock-frequency = <800000000>>;
>> +		};

> What are these clocks? If external to the SoC, then where are they? On the board?

This was the internal iopclk and memclk they were both internal to the chip.
For now I am removing osc and memclk and will just have an iopclk that Gxp-timer will refer to.

>> +	};
>> +};

Thanks,

-Nick

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Krzysztof Kozlowski]

On 16/03/2022 16:41, Hawkins, Nick wrote:
> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@canonical.com]
> Sent: Friday, March 11, 2022 4:30 AM
> To: Hawkins, Nick <nick.hawkins@hpe.com>>; Verdun, Jean-Marie <verdun@hpe.com>>
> Cc: Arnd Bergmann <arnd@arndb.de>>; Olof Johansson <olof@lixom.net>>; soc@kernel.org; Rob Herring <robh+dt@kernel.org>>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree
> 
> On 10/03/2022 20:52, nick.hawkins@hpe.com wrote:
>>> From: Nick Hawkins <nick.hawkins@hpe.com>>
>>>
>>> The HPE SoC is new to linux. This patch creates the basic device tree 
>>> layout with minimum required for linux to boot. This includes timer 
>>> and watchdog support.
>>>
>>> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>>
>>> ---
>>>  arch/arm/boot/dts/Makefile               |   2 +
>>>  arch/arm/boot/dts/hpe-bmc-dl360gen10.dts |  27 +++++
>>>  arch/arm/boot/dts/hpe-gxp.dtsi           | 148 +++++++++++++++++++++++
>>>  3 files changed, 177 insertions(+)
>>>  create mode 100644 arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>>  create mode 100644 arch/arm/boot/dts/hpe-gxp.dtsi
>>>
>>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile 
>>> index e41eca79c950..2823b359d373 100644
>>> --- a/arch/arm/boot/dts/Makefile
>>> +++ b/arch/arm/boot/dts/Makefile
>>> @@ -1550,3 +1550,5 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>>>  	aspeed-bmc-vegman-n110.dtb \
>>>  	aspeed-bmc-vegman-rx20.dtb \
>>>  	aspeed-bmc-vegman-sx20.dtb
>>> +dtb-$(CONFIG_ARCH_HPE_GXP) += \
>>> +	hpe-bmc-dl360gen10.dtb
> 
>> Alphabetically, also in respect to other architectures, so before CONFIG_ARCH_INTEGRATOR.
> 
> Done
> 
>>> diff --git a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>> b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>> new file mode 100644
>>> index 000000000000..da5eac1213a8
>>> --- /dev/null
>>> +++ b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>> @@ -0,0 +1,27 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Device Tree file for HPE DL360Gen10  */
>>> +
>>> +/include/ "hpe-gxp.dtsi"
>>> +
>>> +/ {
>>> +	#address-cells = <1>>;
>>> +	#size-cells = <1>>;
>>> +	compatible = "hpe,gxp";
> 
>> Missing board compatible.
> 
> Will become compatible =  "hpe,gxp","hpe,bmc-dl360gen10"; If that seems okay to you.

Yes, except hpe,gxp goes at the end.

> 
>>> +	model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
>>> +

(...)

>>> +
>>> +		usb0: usb@cefe0000 {
>>> +			compatible = "generic-ehci";
> 
>> I think one of previous comments was that you cannot have "generic-ehci"
>> only, right?
> 
> Yes there was, I removed the usb0: ehci@cefe0000. I see now that this is in reference to the compatible. This is our ehci controller. What would be a more appropriate compatible? Do we need hpe,gxp-ehci perhaps?

Yes,, see other cases in generic-ehci.yaml bindings. Your current choice
would be pointed out by dtbs_check, that it's invalid according to
current bindings.

> 
>>> +			reg = <0xcefe0000 0x100>>;
>>> +			interrupts = <7>>;
>>> +			interrupt-parent = <&vic0>>;
>>> +		};
>>> +
>>> +		usb1: usb@cefe0100 {
>>> +			compatible = "generic-ohci";
>>> +			reg = <0xcefe0100 0x110>>;
>>> +			interrupts = <6>>;
>>> +			interrupt-parent = <&vic0>>;
>>> +		};
>>> +
>>> +		vrom@58000000 {
>>> +			compatible = "mtd-ram";
>>> +			bank-width = <4>>;
>>> +			reg = <0x58000000 0x4000000>>;
>>> +			#address-cells = <1>>;
>>> +			#size-cells = <1>>;
>>> +			partition@0 {
>>> +				label = "vrom-prime";
>>> +				reg = <0x0 0x2000000>>;
>>> +			};
>>> +			partition@2000000 {
>>> +				label = "vrom-second";
>>> +				reg = <0x2000000 0x2000000>>;
>>> +			};
>>> +		};
>>> +
>>> +		i2cg: syscon@c00000f8 {
> 
> 
>>> +			compatible = "simple-mfd", "syscon";
>>> +			reg = <0xc00000f8 0x08>>;
>>> +		};
>>> +	};
>>> +
>>> +	clocks {
>>> +		osc: osc {
> 
>> Keep node naming consistent, so just "clk"... but it's also very generic comparing to others, so I wonder what is this clock?
> 
> We are in the process of redoing the clocks. This was the oscillator but no longer needed for the minimum boot config.
> 
>>> +			compatible = "fixed-clock";
>>> +			#clock-cells = <0>>;
>>> +			clock-output-names = "osc";
>>> +			clock-frequency = <33333333>>;
>>> +		};
>>> +
>>> +		iopclk: iopclk {
>>> +			compatible = "fixed-clock";
>>> +			#clock-cells = <0>>;
>>> +			clock-output-names = "iopclk";
>>> +			clock-frequency = <400000000>>;
>>> +		};
>>> +
>>> +		memclk: memclk {
>>> +			compatible = "fixed-clock";
>>> +			#clock-cells = <0>>;
>>> +			clock-output-names = "memclk";
>>> +			clock-frequency = <800000000>>;
>>> +		};
> 
>> What are these clocks? If external to the SoC, then where are they? On the board?
> 
> This was the internal iopclk and memclk they were both internal to the chip.
> For now I am removing osc and memclk and will just have an iopclk that Gxp-timer will refer to.

You should rather have a clock controller driver which defines this (and
many others). They can stay as a temporary work-around, if you really
need them for some other nodes.

Best regards,
Krzysztof

RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

>> -----Original Message-----
>> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@canonical.com]
>> Sent: Friday, March 11, 2022 4:30 AM
>> To: Hawkins, Nick <nick.hawkins@hpe.com>>>>; Verdun, Jean-Marie 
>> <verdun@hpe.com>>>>
>> Cc: Arnd Bergmann <arnd@arndb.de>>>>; Olof Johansson <olof@lixom.net>>>>; 
>> soc@kernel.org; Rob Herring <robh+dt@kernel.org>>>>; 
>> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; 
>> linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP 
>> Device tree
>> 
>> On 10/03/2022 20:52, nick.hawkins@hpe.com wrote:
>>>>>> From: Nick Hawkins <nick.hawkins@hpe.com>>>>
>>>>>>
>>>>>> The HPE SoC is new to linux. This patch creates the basic device 
>>>>>> tree layout with minimum required for linux to boot. This includes 
>>>>>> timer and watchdog support.
>>>>>>
>>>>>> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>>>>
>>>>>> ---
>>>>>>  arch/arm/boot/dts/Makefile               |   2 +
>>>>>>  arch/arm/boot/dts/hpe-bmc-dl360gen10.dts |  27 +++++
>>>>>>  arch/arm/boot/dts/hpe-gxp.dtsi           | 148 +++++++++++++++++++++++
>>>>>>  3 files changed, 177 insertions(+)
>>>>>>  create mode 100644 arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>>>>>  create mode 100644 arch/arm/boot/dts/hpe-gxp.dtsi
>>>>>>
>>>>>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile 
>>>>>> index e41eca79c950..2823b359d373 100644
>>>>>> --- a/arch/arm/boot/dts/Makefile
>>>>>> +++ b/arch/arm/boot/dts/Makefile
>>>>>> @@ -1550,3 +1550,5 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>>>>>>  	aspeed-bmc-vegman-n110.dtb \
>>>>>>  	aspeed-bmc-vegman-rx20.dtb \
>>>>>>  	aspeed-bmc-vegman-sx20.dtb
>>>>>> +dtb-$(CONFIG_ARCH_HPE_GXP) += \
>>>>>> +	hpe-bmc-dl360gen10.dtb
>> 
>>>> Alphabetically, also in respect to other architectures, so before CONFIG_ARCH_INTEGRATOR.
>> 
>> Done
>> 
>>>>>> diff --git a/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>>>>> b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>>>>> new file mode 100644
>>>>>> index 000000000000..da5eac1213a8
>>>>>> --- /dev/null
>>>>>> +++ b/arch/arm/boot/dts/hpe-bmc-dl360gen10.dts
>>>>>> @@ -0,0 +1,27 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>> +/*
>>>>>> + * Device Tree file for HPE DL360Gen10  */
>>>>>> +
>>>>>> +/include/ "hpe-gxp.dtsi"
>>>>>> +
>>>>>> +/ {
>>>>>> +	#address-cells = <1>>>>;
>>>>>> +	#size-cells = <1>>>>;
>>>>>> +	compatible = "hpe,gxp";
>> 
>>>> Missing board compatible.
>> 
>> Will become compatible =  "hpe,gxp","hpe,bmc-dl360gen10"; If that seems okay to you.

> Yes, except hpe,gxp goes at the end.

Done

>> 
>>>>>> +	model = "Hewlett Packard Enterprise ProLiant dl360 Gen10";
>>>>>> +

(...)

>>>>>> +
>>>>>> +		usb0: usb@cefe0000 {
>>>>>> +			compatible = "generic-ehci";
>> 
>>>> I think one of previous comments was that you cannot have "generic-ehci"
>>>> only, right?
>> 
>> Yes there was, I removed the usb0: ehci@cefe0000. I see now that this is in reference to the compatible. This is our ehci controller. What would be a more appropriate compatible? Do we need hpe,gxp-ehci perhaps?

> Yes,, see other cases in generic-ehci.yaml bindings. Your current choice would be pointed out by dtbs_check, that it's invalid according to current bindings.

For some reason when I compile I am not seeing a warning for that file. I have been using "make dtbs_check" and "make dtbs W=1". Perhaps I am missing an important flag?

In the case of creating a hpe,gxp-ehci binding would I need to add that to the generic-ehci.yaml?


>> 
>>>>>> +			reg = <0xcefe0000 0x100>>>>;
>>>>>> +			interrupts = <7>>>>;
>>>>>> +			interrupt-parent = <&vic0>>>>;
>>>>>> +		};
>>>>>> +
>>>>>> +		usb1: usb@cefe0100 {
>>>>>> +			compatible = "generic-ohci";
>>>>>> +			reg = <0xcefe0100 0x110>>>>;
>>>>>> +			interrupts = <6>>>>;
>>>>>> +			interrupt-parent = <&vic0>>>>;
>>>>>> +		};
>>>>>> +
>>>>>> +		vrom@58000000 {
>>>>>> +			compatible = "mtd-ram";
>>>>>> +			bank-width = <4>>>>;
>>>>>> +			reg = <0x58000000 0x4000000>>>>;
>>>>>> +			#address-cells = <1>>>>;
>>>>>> +			#size-cells = <1>>>>;
>>>>>> +			partition@0 {
>>>>>> +				label = "vrom-prime";
>>>>>> +				reg = <0x0 0x2000000>>>>;
>>>>>> +			};
>>>>>> +			partition@2000000 {
>>>>>> +				label = "vrom-second";
>>>>>> +				reg = <0x2000000 0x2000000>>>>;
>>>>>> +			};
>>>>>> +		};
>>>>>> +
>>>>>> +		i2cg: syscon@c00000f8 {
>> 
>> 
>>>>>> +			compatible = "simple-mfd", "syscon";
>>>>>> +			reg = <0xc00000f8 0x08>>>>;
>>>>>> +		};
>>>>>> +	};
>>>>>> +
>>>>>> +	clocks {
>>>>>> +		osc: osc {
>> 
>>>> Keep node naming consistent, so just "clk"... but it's also very generic comparing to others, so I wonder what is this clock?
>> 
>> We are in the process of redoing the clocks. This was the oscillator but no longer needed for the minimum boot config.
>> 
>>>>>> +			compatible = "fixed-clock";
>>>>>> +			#clock-cells = <0>>>>;
>>>>>> +			clock-output-names = "osc";
>>>>>> +			clock-frequency = <33333333>>>>;
>>>>>> +		};
>>>>>> +
>>>>>> +		iopclk: iopclk {
>>>>>> +			compatible = "fixed-clock";
>>>>>> +			#clock-cells = <0>>>>;
>>>>>> +			clock-output-names = "iopclk";
>>>>>> +			clock-frequency = <400000000>>>>;
>>>>>> +		};
>>>>>> +
>>>>>> +		memclk: memclk {
>>>>>> +			compatible = "fixed-clock";
>>>>>> +			#clock-cells = <0>>>>;
>>>>>> +			clock-output-names = "memclk";
>>>>>> +			clock-frequency = <800000000>>>>;
>>>>>> +		};
>> 
>>>> What are these clocks? If external to the SoC, then where are they? On the board?
>> 
>> This was the internal iopclk and memclk they were both internal to the chip.
>> For now I am removing osc and memclk and will just have an iopclk that Gxp-timer will refer to.

> You should rather have a clock controller driver which defines this (and many others). They can stay as a temporary work-around, if you really need them for some other nodes.

I am trying to picture what you are saying but I am unsure, I know that on a separate review you mentioned that the gxp-timer needed to have clocks, and clock-names inside the node. Would it be improper for the gxp-timer to reference iopclk?

Thanks!

-Nick Hawkins

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Krzysztof Kozlowski]

On 16/03/2022 21:10, Hawkins, Nick wrote:

(...)

>>>>> I think one of previous comments was that you cannot have "generic-ehci"
>>>>> only, right?
>>>
>>> Yes there was, I removed the usb0: ehci@cefe0000. I see now that this is in reference to the compatible. This is our ehci controller. What would be a more appropriate compatible? Do we need hpe,gxp-ehci perhaps?
> 
>> Yes,, see other cases in generic-ehci.yaml bindings. Your current choice would be pointed out by dtbs_check, that it's invalid according to current bindings.
> 
> For some reason when I compile I am not seeing a warning for that file. I have been using "make dtbs_check" and "make dtbs W=1". Perhaps I am missing an important flag?

My bad, I misread the generic-ehci binding, so your compatible is
actually correct from bindings point of view. Still common practice is
to add own compatible which allows later customization.

> In the case of creating a hpe,gxp-ehci binding would I need to add that to the generic-ehci.yaml?

Yes, add your compatible to that big enum with list of many implementations.

(...)

>>>>>>> +
>>>>>>> +		memclk: memclk {
>>>>>>> +			compatible = "fixed-clock";
>>>>>>> +			#clock-cells = <0>>>>;
>>>>>>> +			clock-output-names = "memclk";
>>>>>>> +			clock-frequency = <800000000>>>>;
>>>>>>> +		};
>>>
>>>>> What are these clocks? If external to the SoC, then where are they? On the board?
>>>
>>> This was the internal iopclk and memclk they were both internal to the chip.
>>> For now I am removing osc and memclk and will just have an iopclk that Gxp-timer will refer to.
> 
>> You should rather have a clock controller driver which defines this (and many others). They can stay as a temporary work-around, if you really need them for some other nodes.
> 
> I am trying to picture what you are saying but I am unsure, I know that on a separate review you mentioned that the gxp-timer needed to have clocks, and clock-names inside the node. Would it be improper for the gxp-timer to reference iopclk?

It all depends on the architecture of your SoC. I could imagine such one:
1. Few external (from SoC point of view) oscillators, usually provided
on the board. It could be 24 MHz, could be 32767 Hz for Bluetooth etc.

2. One or several clock controllers inside the SoC which take as input
these external clocks. The clock controller provides clocks for several
other components/blocks. Allows also gating clocks, reparenting and
changing dividers (rate).

3. Other blocks within your SoC, e.g. gxp-timer, use these clocks.

The true question is where is that memclk or iopclk generated? Is it
controllable (gate/mux/rate)? Even some fixed-frequency clocks, coming
out of that clock controller (point 2.), are defined in the clock
controller because that's the logical place for them.


Best regards,
Krzysztof

RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

-----Original Message-----
From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@canonical.com] 
Sent: Thursday, March 17, 2022 3:37 AM
To: Hawkins, Nick <nick.hawkins@hpe.com>; Verdun, Jean-Marie <verdun@hpe.com>
Cc: Arnd Bergmann <arnd@arndb.de>; Olof Johansson <olof@lixom.net>; soc@kernel.org; Rob Herring <robh+dt@kernel.org>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

On 16/03/2022 21:10, Hawkins, Nick wrote:

(...)

>>>>>> I think one of previous comments was that you cannot have "generic-ehci"
>>>>>> only, right?
>>>
>>>> Yes there was, I removed the usb0: ehci@cefe0000. I see now that this is in reference to the compatible. This is our ehci controller. What would be a more appropriate compatible? Do we need hpe,gxp-ehci perhaps?
>> 
>>> Yes,, see other cases in generic-ehci.yaml bindings. Your current choice would be pointed out by dtbs_check, that it's invalid according to current bindings.
>> 
>> For some reason when I compile I am not seeing a warning for that file. I have been using "make dtbs_check" and "make dtbs W=1". Perhaps I am missing an important flag?

> My bad, I misread the generic-ehci binding, so your compatible is actually correct from bindings point of view. Still common practice is to add own compatible which allows later customization.

>> In the case of creating a hpe,gxp-ehci binding would I need to add that to the generic-ehci.yaml?

> Yes, add your compatible to that big enum with list of many implementations.

Done.

> (...)

>>>>>>>> +
>>>>>>>> +		memclk: memclk {
>>>>>>>> +			compatible = "fixed-clock";
>>>>>>>> +			#clock-cells = <0>>>>;
>>>>>>>> +			clock-output-names = "memclk";
>>>>>>>> +			clock-frequency = <800000000>>>>;
>>>>>>>> +		};
>>>
>>>>>> What are these clocks? If external to the SoC, then where are they? On the board?
>>>
>>>> This was the internal iopclk and memclk they were both internal to the chip.
>>>> For now I am removing osc and memclk and will just have an iopclk that Gxp-timer will refer to.
>> 
>>> You should rather have a clock controller driver which defines this (and many others). They can stay as a temporary work-around, if you really need them for some other nodes.
>> 
>> I am trying to picture what you are saying but I am unsure, I know that on a separate review you mentioned that the gxp-timer needed to have clocks, and clock-names inside the node. Would it be improper for the gxp-timer to reference iopclk?

> It all depends on the architecture of your SoC. I could imagine such one:
> 1. Few external (from SoC point of view) oscillators, usually provided on the board. It could be 24 MHz, could be 32767 Hz for Bluetooth etc.

> 2. One or several clock controllers inside the SoC which take as input these external clocks. The clock controller provides clocks for several other components/blocks. Allows also gating clocks, reparenting and changing dividers (rate).

> 3. Other blocks within your SoC, e.g. gxp-timer, use these clocks.

> The true question is where is that memclk or iopclk generated? Is it controllable (gate/mux/rate)? Even some fixed-frequency clocks, coming out of that clock controller (point 2.), are defined in the clock controller because that's the logical place for >them.

From the perspective of our SoC there is a PPUCLK that generates the clk signals for our watchdog and timer. This happens inside the SoC. I am trying to represent this below.

axi {
		compatible = "simple-bus";
		#address-cells = <1>;
		#size-cells = <1>;
		ranges;
		dma-ranges;

		ahb@c0000000 {
			compatible = "simple-bus";
			#address-cells = <1>;
			#size-cells = <1>;
			ranges = <0x0 0xc0000000 0x30000000>;

			...

			timer0: timer@80 {
				compatible = "hpe,gxp-timer";
				reg = <0x80 0x1>, <0x94 0x01>, <0x88 0x08>;
				interrupts = <0>;
				interrupt-parent = <&vic0>;
				clocks = <&ppuclk>;
				clock-names = "ppuclk";
				clock-frequency = <400000000>;
			};

			watchdog0: watchdog@90 {
				compatible = "hpe,gxp-wdt";
				reg = <0x90 0x02>, <0x96 0x01>;
			};

			...
	};

	clocks {
		ppuclk: ppuclk {
			compatible = "fixed-clock";
			#clock-cells = <0>;
			clock-output-names = "ppuclk";
			clock-frequency = <400000000>;
		};
	};

I am in the process of rewriting the timer driver for Linux but have hit a dilemma and I am looking for some direction. The registers that represent the watchdog timer, and timer all lay in the same register region and they are spread out to the point where there are other controls  in the same area.

For instance with our watchdog controls we have:

@90 the countdown value
@96 the configuration

And for our timer we have:
@80 the countdown value
@94 the configuration
@88 this is actually our timestamp register but is being included in with the timer driver currently to call clocksource_mmio_init.

What would be your recommendation for this? I was considering creating a gxp-clock that specifically points at the timestamp register but I still have the issue with gxp-timer and gxp-wdt being spread across the same area of registers. 

 Thanks,

-Nick Hawkins

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Arnd Bergmann]

On Tue, Mar 29, 2022 at 9:38 PM Hawkins, Nick <nick.hawkins@hpe.com> wrote:

> I am in the process of rewriting the timer driver for Linux but have hit a dilemma and I am looking for some direction. The registers that represent the watchdog timer, and timer all lay in the same register region and they are spread out to the point where there are other controls  in the same area.
>
> For instance with our watchdog controls we have:
>
> @90 the countdown value
> @96 the configuration
>
> And for our timer we have:
> @80 the countdown value
> @94 the configuration
> @88 this is actually our timestamp register but is being included in with the timer driver currently to call clocksource_mmio_init.
>
> What would be your recommendation for this? I was considering creating a gxp-clock that specifically points at the timestamp register but I still have the issue with gxp-timer and gxp-wdt being spread across the same area of registers.

I think this is most commonly done using a 'syscon' node, have a look at the
files listed by


$ git grep syscon drivers/watchdog/ drivers/clocksource/

You may also want to look at those drivers to find if any of them can
be directly reused,
this is perhaps a licensed IP block that others are using as well.

        Arnd

RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

-----Original Message-----
From: Arnd Bergmann [mailto:arnd@arndb.de] 
Sent: Tuesday, March 29, 2022 4:13 PM
To: Hawkins, Nick <nick.hawkins@hpe.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>; Verdun, Jean-Marie <verdun@hpe.com>; Arnd Bergmann <arnd@arndb.de>; Olof Johansson <olof@lixom.net>; soc@kernel.org; Rob Herring <robh+dt@kernel.org>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

On Tue, Mar 29, 2022 at 9:38 PM Hawkins, Nick <nick.hawkins@hpe.com>> wrote:

>> I am in the process of rewriting the timer driver for Linux but have hit a dilemma and I am looking for some direction. The registers that represent the watchdog timer, and timer all lay in the same register region and they are spread out to the point where there are other controls  in the same area.
>
>> For instance with our watchdog controls we have:
>
>> @90 the countdown value
>> @96 the configuration
>
>> And for our timer we have:
>> @80 the countdown value
>> @94 the configuration
>> @88 this is actually our timestamp register but is being included in with the timer driver currently to call clocksource_mmio_init.
>
>> What would be your recommendation for this? I was considering creating a gxp-clock that specifically points at the timestamp register but I still have the issue with gxp-timer and gxp-wdt being spread across the same area of registers.

> I think this is most commonly done using a 'syscon' node, have a look at the files listed by


> $ git grep syscon drivers/watchdog/ drivers/clocksource/

Is this a good example of what you were thinking of? I found this in arch/arm64/boot/dts/socionext/uniphier-ld11.dtsi

sysctrl@61840000 {
                        compatible = "socionext,uniphier-ld11-sysctrl",
                                     "simple-mfd", "syscon";
                        reg = <0x61840000 0x10000>;

                        sys_clk: clock {
                                compatible = "socionext,uniphier-ld11-clock";
                                #clock-cells = <1>;
                        };

                        sys_rst: reset {
                                compatible = "socionext,uniphier-ld11-reset";
                                #reset-cells = <1>;
                        };

                        watchdog {
                                compatible = "socionext,uniphier-wdt";
                        };
 };

If that is the case..

timectrl@80 {
	compatible = "hpe,gxp-timectrl","syscon";
	reg = <0x80 0x16>;

	watchdog0: watchdog {
		compatible = "hpe,gxp-wdt";
	}

	timer0: timer {
		compatible = "hpe,gxp-timer";
	}
}
	

> You may also want to look at those drivers to find if any of them can be directly reused, this is perhaps a licensed IP block that others are using as well.

I will look and see if any of them have the same register sets and functionality.

Thanks,

-Nick Hawkins

RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

-----Original Message-----
From: Arnd Bergmann [mailto:arnd@arndb.de] 
Sent: Tuesday, March 29, 2022 4:13 PM
To: Hawkins, Nick <nick.hawkins@hpe.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>; Verdun, Jean-Marie <verdun@hpe.com>; Arnd Bergmann <arnd@arndb.de>; Olof Johansson <olof@lixom.net>; soc@kernel.org; Rob Herring <robh+dt@kernel.org>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

On Tue, Mar 29, 2022 at 9:38 PM Hawkins, Nick <nick.hawkins@hpe.com>> wrote:

>> I am in the process of rewriting the timer driver for Linux but have hit a dilemma and I am looking for some direction. The registers that represent the watchdog timer, and timer all lay in the same register region and they are spread out to the point where there are other controls  in the same area.
>
>> For instance with our watchdog controls we have:
>
>> @90 the countdown value
>> @96 the configuration
>
>> And for our timer we have:
>> @80 the countdown value
>> @94 the configuration
>> @88 this is actually our timestamp register but is being included in with the timer driver currently to call clocksource_mmio_init.
>
>> What would be your recommendation for this? I was considering creating a gxp-clock that specifically points at the timestamp register but I still have the issue with gxp-timer and gxp-wdt being spread across the same area of registers.

> I think this is most commonly done using a 'syscon' node, have a look at the files listed by

I found an example and copied it although I have a couple questions when it comes to actually coding it. Can that be here or should I post these questions in the patch that actually concern the file?

st: timer@80 {
	compatible = "hpe,gxp-timer","syscon","simple-mfd";
	reg = <0x80 0x16>;
	interrupts = <0>;
	interrupt-parent = <&vic0>;
	clocks = <&ppuclk>;
	clock-names = "ppuclk";
	clock-frequency = <400000000>;

	watchdog {
		compatible = "hpe,gxp-wdt";
	};
 };

Thanks,

-Nick Hawkins

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Arnd Bergmann]

On Thu, Mar 31, 2022 at 12:27 AM Hawkins, Nick <nick.hawkins@hpe.com> wrote:
> On Tue, Mar 29, 2022 at 9:38 PM Hawkins, Nick <nick.hawkins@hpe.com>> wrote:
>
> >> I am in the process of rewriting the timer driver for Linux but have hit a dilemma and I am looking for some direction. The registers that represent the watchdog timer, and timer all lay in the same register region and they are spread out to the point where there are other controls  in the same area.
> >
> >> For instance with our watchdog controls we have:
> >
> >> @90 the countdown value
> >> @96 the configuration
> >
> >> And for our timer we have:
> >> @80 the countdown value
> >> @94 the configuration
> >> @88 this is actually our timestamp register but is being included in with the timer driver currently to call clocksource_mmio_init.
> >
> >> What would be your recommendation for this? I was considering creating a gxp-clock that specifically points at the timestamp register but I still have the issue with gxp-timer and gxp-wdt being spread across the same area of registers.
>
> > I think this is most commonly done using a 'syscon' node, have a look at the files listed by
>
> I found an example and copied it although I have a couple questions when it comes to actually coding it. Can that be here or should I post these questions in the patch that actually concern the file?
>
> st: timer@80 {
>         compatible = "hpe,gxp-timer","syscon","simple-mfd";
>         reg = <0x80 0x16>;
>         interrupts = <0>;
>         interrupt-parent = <&vic0>;
>         clocks = <&ppuclk>;
>         clock-names = "ppuclk";
>         clock-frequency = <400000000>;
>
>         watchdog {
>                 compatible = "hpe,gxp-wdt";
>         };
>  };

I'd have to study the other examples myself to see what is most common.

My feeling would be that it's better to either have a "hpe,gxp-timer" parent
device with a watchdog child but no syscon, or to have a syscon/simple-mfd
parent with both the timer and the watchdog as children.

       Arnd

RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

-----Original Message-----
From: Arnd Bergmann [mailto:arnd@arndb.de] 
Sent: Thursday, March 31, 2022 4:30 AM
To: Hawkins, Nick <nick.hawkins@hpe.com>
Cc: Arnd Bergmann <arnd@arndb.de>; Verdun, Jean-Marie <verdun@hpe.com>; Olof Johansson <olof@lixom.net>; soc@kernel.org; Rob Herring <robh+dt@kernel.org>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

On Thu, Mar 31, 2022 at 12:27 AM Hawkins, Nick <nick.hawkins@hpe.com> wrote:
> On Tue, Mar 29, 2022 at 9:38 PM Hawkins, Nick <nick.hawkins@hpe.com>> wrote:
>
> >> I am in the process of rewriting the timer driver for Linux but have hit a dilemma and I am looking for some direction. The registers that represent the watchdog timer, and timer all lay in the same register region and they are spread out to the point where there are other controls  in the same area.
> >
> >> For instance with our watchdog controls we have:
> >
> >> @90 the countdown value
> >> @96 the configuration
> >
> >> And for our timer we have:
> >> @80 the countdown value
> >> @94 the configuration
> >> @88 this is actually our timestamp register but is being included in with the timer driver currently to call clocksource_mmio_init.
> >
> >> What would be your recommendation for this? I was considering creating a gxp-clock that specifically points at the timestamp register but I still have the issue with gxp-timer and gxp-wdt being spread across the same area of registers.
>>
> > I think this is most commonly done using a 'syscon' node, have a 
> > look at the files listed by
>>
>> I found an example and copied it although I have a couple questions when it comes to actually coding it. Can that be here or should I post these questions in the patch that actually concern the file?
>>
>> st: timer@80 {
>>         compatible = "hpe,gxp-timer","syscon","simple-mfd";
>>         reg = <0x80 0x16>;
>>         interrupts = <0>;
>>         interrupt-parent = <&vic0>;
>>         clocks = <&ppuclk>;
>>         clock-names = "ppuclk";
>>         clock-frequency = <400000000>;
>>
>>         watchdog {
>>                 compatible = "hpe,gxp-wdt";
>>         };
>>  };

> I'd have to study the other examples myself to see what is most common.

> My feeling would be that it's better to either have a "hpe,gxp-timer" parent device with a watchdog child but no syscon, or to have a syscon/simple-mfd parent with both the timer and the watchdog as children.

Arnd, thanks for the feedback. I am trying to use the approach you recommend where you have a syscon/simple-mfd parent with watchdog and timer as children.

st: chip-controller@80 {
                                compatible = "hpe,gxp-ctrl-st","syscon","simple-mfd";
                                reg = <0x80 0x16>;

                                timer0: timer {
                                        compatible = "hpe,gxp-timer";
                                        interrupts = <0>;
                                        interrupt-parent = <&vic0>;
                                        clocks = <&ppuclk>;
                                        clock-names = "ppuclk";
                                };

                                watchdog {
                                        compatible = "hpe,gxp-wdt";
                                };
};

This compiles without any errors but I do have some questions about accessing the regmap in both drivers, specifically the timer code. How do you use a regmap with clocksource_mmio_init? I tried searching through the codebase and could not find the answer. I assume I am missing some vital step.

Thanks,

-Nick

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Arnd Bergmann]

On Thu, Mar 31, 2022 at 11:09 PM Hawkins, Nick <nick.hawkins@hpe.com> wrote:
> On Thu, Mar 31, 2022 at 12:27 AM Hawkins, Nick <nick.hawkins@hpe.com> wrote:
> > On Tue, Mar 29, 2022 at 9:38 PM Hawkins, Nick <nick.hawkins@hpe.com>> wrote:
> > I'd have to study the other examples myself to see what is most common.
>
> > My feeling would be that it's better to either have a "hpe,gxp-timer" parent device with a watchdog child but no syscon, or to have a syscon/simple-mfd parent with both the timer and the watchdog as children.
>
> Arnd, thanks for the feedback. I am trying to use the approach you recommend where you have a syscon/simple-mfd parent with watchdog and timer as children.
>
> st: chip-controller@80 {
>                                 compatible = "hpe,gxp-ctrl-st","syscon","simple-mfd";
>                                 reg = <0x80 0x16>;
>
>                                 timer0: timer {
>                                         compatible = "hpe,gxp-timer";
>                                         interrupts = <0>;
>                                         interrupt-parent = <&vic0>;
>                                         clocks = <&ppuclk>;
>                                         clock-names = "ppuclk";
>                                 };
>
>                                 watchdog {
>                                         compatible = "hpe,gxp-wdt";
>                                 };
> };
>
> This compiles without any errors but I do have some questions about accessing the regmap in both drivers, specifically the timer code. How do you use a regmap with clocksource_mmio_init? I tried searching through the codebase and could not find the answer. I assume I am missing some vital step.

I don't think you can do this, if you are using the syscon regmap, you
go through the
regmap indirection rather than accessing the mmio register by virtual address,
and this may result in some extra code in your driver, and a little
runtime overhead.

If you prefer to avoid that, you can go back to having the timer node as the
parent, but without being a syscon. In this case, the watchdog would be handled
in one of these ways:

a) a child device gets created from the clocksource driver and bound to the
    watchdog driver, which then uses a private interface between the clocksource
    and the watchdog to access the registers

b) the clocksource driver itself registers as a watchdog driver, without
    having a separate driver module

One thing to consider is whether the register range here contains any
registers that may be used in another driver, e.g. a second timer,
a PWM, or a clk controller. If not, you are fairly free to pick any of these
approaches.

        Arnd

RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

-----Original Message-----
From: Arnd Bergmann [mailto:arnd@arndb.de] 
Sent: Thursday, March 31, 2022 4:53 PM
To: Hawkins, Nick <nick.hawkins@hpe.com>
Cc: Arnd Bergmann <arnd@arndb.de>; Verdun, Jean-Marie <verdun@hpe.com>; Olof Johansson <olof@lixom.net>; soc@kernel.org; Rob Herring <robh+dt@kernel.org>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree


> I don't think you can do this, if you are using the syscon regmap, you go through the regmap indirection rather than accessing the mmio register by virtual address, and this may result in some extra code in your driver, and a little runtime overhead.

> If you prefer to avoid that, you can go back to having the timer node as the parent, but without being a syscon. In this case, the watchdog would be handled in one of these ways:

> a) a child device gets created from the clocksource driver and bound to the
    watchdog driver, which then uses a private interface between the clocksource
    and the watchdog to access the registers

> b) the clocksource driver itself registers as a watchdog driver, without
    having a separate driver module

> One thing to consider is whether the register range here contains any registers that may be used in another driver, e.g. a second timer, a PWM, or a clk controller. If not, you are fairly free to pick any of these approaches.

I will try to use the b) approach everything in that range is timer or watchdog related. There is a second timer however there are no plans on using that. Should the combined code still live inside the driver/timer directory or should it be moved to mfd?

Thanks,

-Nick

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Arnd Bergmann]

On Fri, Apr 1, 2022 at 6:05 PM Hawkins, Nick <nick.hawkins@hpe.com> wrote:
> > I don't think you can do this, if you are using the syscon regmap, you go through the regmap indirection rather than accessing the mmio register by virtual address, and this may result in some extra code in your driver, and a little runtime overhead.
>
> > If you prefer to avoid that, you can go back to having the timer node as the parent, but without being a syscon. In this case, the watchdog would be handled in one of these ways:
>
> > a) a child device gets created from the clocksource driver and bound to the
>     watchdog driver, which then uses a private interface between the clocksource
>     and the watchdog to access the registers
>
> > b) the clocksource driver itself registers as a watchdog driver, without
>     having a separate driver module
>
> > One thing to consider is whether the register range here contains any registers that may be used in another driver, e.g. a second timer, a PWM, or a clk controller. If not, you are fairly free to pick any of these approaches.
>
> I will try to use the b) approach everything in that range is timer or watchdog related. There is a second timer however there are no plans on using that. Should the combined code still live inside the driver/timer directory or should it be moved to mfd?

I would put it into drivers/clocksource/, I don't think drivers/mtd
would be any better,
but there is a chance that the clocksource maintainers don't want to
have the watchdog
code in their tree.

        Arnd

RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

-----Original Message-----
From: Arnd Bergmann [mailto:arnd@arndb.de] 
Sent: Friday, April 1, 2022 11:30 AM
To: Hawkins, Nick <nick.hawkins@hpe.com>
Cc: Arnd Bergmann <arnd@arndb.de>; Verdun, Jean-Marie <verdun@hpe.com>; Olof Johansson <olof@lixom.net>; soc@kernel.org; Rob Herring <robh+dt@kernel.org>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

On Fri, Apr 1, 2022 at 6:05 PM Hawkins, Nick <nick.hawkins@hpe.com> wrote:
> > > I don't think you can do this, if you are using the syscon regmap, you go through the regmap indirection rather than accessing the mmio register by virtual address, and this may result in some extra code in your driver, and a little runtime overhead.
> >
> > > If you prefer to avoid that, you can go back to having the timer node as the parent, but without being a syscon. In this case, the watchdog would be handled in one of these ways:
> >
> > > a) a child device gets created from the clocksource driver and bound 
> > > to the
> >     watchdog driver, which then uses a private interface between the clocksource
> >     and the watchdog to access the registers
> >
> > > b) the clocksource driver itself registers as a watchdog driver, 
> > > without
> >     having a separate driver module
> >
> > > One thing to consider is whether the register range here contains any registers that may be used in another driver, e.g. a second timer, a PWM, or a clk controller. If not, you are fairly free to pick any of these approaches.
> >
> > I will try to use the b) approach everything in that range is timer or watchdog related. There is a second timer however there are no plans on using that. Should the combined code still live inside the driver/timer directory or should it be moved to mfd?

> > I would put it into drivers/clocksource/, I don't think drivers/mtd would be any better, but there is a chance that the clocksource maintainers don't want to have the watchdog code in their tree.

While trying to discover how to creating two devices in one driver I ran across an interesting .dtsi and I was wondering if this would be a valid approach for my situation as well. The pertinent files are:
1) drivers/clocksource/timer-digicolor.c
2) arch/arm/boot/dts/cx92755.dtsi
3) drivers/watchdog/digicolor_wdt.c

Here they are just sharing the same register area:

timer@f0000fc0 {
	compatible = "cnxt,cx92755-timer";
	reg = <0xf0000fc0 0x40>;
	interrupts = <19>, <31>, <34>, <35>, <52>, <53>, <54>, <55>;
	clocks = <&main_clk>;
};

rtc@f0000c30 {
	compatible = "cnxt,cx92755-rtc";
	reg = <0xf0000c30 0x18>;
	interrupts = <25>;
};

watchdog@f0000fc0 {
	compatible = "cnxt,cx92755-wdt";
	reg = <0xf0000fc0 0x8>;
	clocks = <&main_clk>;
	timeout-sec = <15>;
};

Thanks,

-Nick Hawkins

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Arnd Bergmann]

On Mon, Apr 4, 2022 at 10:22 PM Hawkins, Nick <nick.hawkins@hpe.com> wrote:
> > > I would put it into drivers/clocksource/, I don't think drivers/mtd would be any better, but there is a chance that the clocksource maintainers don't want to have the watchdog code in their tree.
>
> While trying to discover how to creating two devices in one driver I ran across an interesting .dtsi and I was wondering if this would be a valid approach for my situation as well. The pertinent files are:
> 1) drivers/clocksource/timer-digicolor.c
> 2) arch/arm/boot/dts/cx92755.dtsi
> 3) drivers/watchdog/digicolor_wdt.c
>
> Here they are just sharing the same register area:
>
> timer@f0000fc0 {
>         compatible = "cnxt,cx92755-timer";
>         reg = <0xf0000fc0 0x40>;
>         interrupts = <19>, <31>, <34>, <35>, <52>, <53>, <54>, <55>;
>         clocks = <&main_clk>;
> };
>
> rtc@f0000c30 {
>         compatible = "cnxt,cx92755-rtc";
>         reg = <0xf0000c30 0x18>;
>         interrupts = <25>;
> };
>
> watchdog@f0000fc0 {
>         compatible = "cnxt,cx92755-wdt";
>         reg = <0xf0000fc0 0x8>;
>         clocks = <&main_clk>;
>         timeout-sec = <15>;
> };

Right, it is possible to make this work, but it's not recommended, and you have
to work around the sanity checks in the code that try to keep you from doing it
wrong, as well as any tooling that tries to check for these in the DT.

         Arnd

RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

-----Original Message-----
From: Arnd Bergmann [mailto:arnd@arndb.de] 
Sent: Monday, April 4, 2022 5:02 PM
To: Hawkins, Nick <nick.hawkins@hpe.com>>
Cc: Arnd Bergmann <arnd@arndb.de>>; Verdun, Jean-Marie <verdun@hpe.com>>; Olof Johansson <olof@lixom.net>>; soc@kernel.org; Rob Herring <robh+dt@kernel.org>>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

On Mon, Apr 4, 2022 at 10:22 PM Hawkins, Nick <nick.hawkins@hpe.com>> wrote:
>>>> I would put it into drivers/clocksource/, I don't think drivers/mtd would be any better, but there is a chance that the clocksource maintainers don't want to have the watchdog code in their tree.
>>
>> While trying to discover how to creating two devices in one driver I ran across an interesting .dtsi and I was wondering if this would be a valid approach for my situation as well. The pertinent files are:
>> 1) drivers/clocksource/timer-digicolor.c
>> 2) arch/arm/boot/dts/cx92755.dtsi
>> 3) drivers/watchdog/digicolor_wdt.c
>>
>> Here they are just sharing the same register area:
>>
>> timer@f0000fc0 {
>>         compatible = "cnxt,cx92755-timer";
>>         reg = <0xf0000fc0 0x40>>;
>>         interrupts = <19>>, <31>>, <34>>, <35>>, <52>>, <53>>, <54>>, <55>>;
>>         clocks = <&main_clk>>;
>> };
>>
>> rtc@f0000c30 {
>>         compatible = "cnxt,cx92755-rtc";
>>         reg = <0xf0000c30 0x18>>;
>>         interrupts = <25>>;
>> };
>>
>> watchdog@f0000fc0 {
>>         compatible = "cnxt,cx92755-wdt";
>>         reg = <0xf0000fc0 0x8>>;
>>         clocks = <&main_clk>>;
>>         timeout-sec = <15>>;
>> };

> Right, it is possible to make this work, but it's not recommended, and you have to work around the sanity checks in the code that try to keep you from doing it wrong, as well as any tooling that tries to check for these in the DT.

I found an example in the kernel where the timer creates a child watchdog device and passes it the base address when creating it. I used this to model the gxp-timer and gxp-wdt. The following files were what I have referenced:
drivers/watchdog/ixp4xx_wdt.c
drivers/clocksource/timer-ixp4xx.c

This seems very similar to what you suggested previously except I do not see a private interface in there between the parent and the child device. Is it mandatory to have the private interface between the two? If it is, what would you recommend that interface be? So far without the private interface I am not seeing any issues accessing the registers.

Thanks,

-Nick

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Arnd Bergmann]

On Tue, Apr 5, 2022 at 11:21 PM Hawkins, Nick <nick.hawkins@hpe.com> wrote:
>
> > Right, it is possible to make this work, but it's not recommended, and you have to work around the sanity checks in the code that try to keep you from doing it wrong, as well as any tooling that tries to check for these in the DT.
>
> I found an example in the kernel where the timer creates a child watchdog device and passes it the base address when creating it. I used this to model the gxp-timer and gxp-wdt. The following files were what I have referenced:
> drivers/watchdog/ixp4xx_wdt.c
> drivers/clocksource/timer-ixp4xx.c

Yes, I think that is a good example.

> This seems very similar to what you suggested previously except I do not see a private interface in there between the parent and the child device. Is it mandatory to have the private interface between the two? If it is, what would you recommend that interface be? So far without the private interface I am not seeing any issues accessing the registers.

I would count passing a register address to the child device as a
private interface.
It's a minimalistic one, but that is not a bad thing here.

         Arnd

RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

-----Original Message-----
From: Arnd Bergmann [mailto:arnd@arndb.de] 
Sent: Wednesday, April 6, 2022 2:25 AM
To: Hawkins, Nick <nick.hawkins@hpe.com>
Cc: Arnd Bergmann <arnd@arndb.de>; Verdun, Jean-Marie <verdun@hpe.com>; Olof Johansson <olof@lixom.net>; soc@kernel.org; Rob Herring <robh+dt@kernel.org>; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

> I would count passing a register address to the child device as a private interface.
> It's a minimalistic one, but that is not a bad thing here.

Thank you. Now that the parent/child issue with timer/watchdog is resolved I am now having an issue trying to access "iopclk" from "gxp-timer". I have tried use of of_clk_get_by_name and of_clk_get which both fail to find the clock.
Is it because clocks is outside of the axi -> ahb hierarchy?

        clocks {
                pll: pll {
                        compatible = "fixed-clock";
                        #clock-cells = <0>;
                        clock-frequency = <1600000000>;
                };

                iopclk: iopclk {
                        compatible = "fixed-factor-clock";
                        #clock-cells = <0>;
                        clock-div = <4>;
                        clock-mult = <4>;
                        clocks = <&pll>;
                };
        };

        axi {
                compatible = "simple-bus";
                #address-cells = <1>;
                #size-cells = <1>;
                ranges;
                dma-ranges;

                ahb@c0000000 {
                        compatible = "simple-bus";
                        #address-cells = <1>;
                        #size-cells = <1>;
                        ranges = <0x0 0xc0000000 0x30000000>;

                        ...

                        st: timer@80 {
                                compatible = "hpe,gxp-timer","simple-mfd";
                                reg = <0x80 0x16>;
                                interrupts = <0>;
                                interrupt-parent = <&vic0>;
                                clocks = <&iopclk>;
                                clock-names = "iopclk";

                                watchdog {
                                        compatible = "hpe,gxp-wdt";
                                };
                        };
                };
        };

Thanks,

-Nick Hawkins

Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Arnd Bergmann]

On Wed, Apr 13, 2022 at 6:48 PM Hawkins, Nick <nick.hawkins@hpe.com> wrote:
> > I would count passing a register address to the child device as a private interface.
> > It's a minimalistic one, but that is not a bad thing here.
>
> Thank you. Now that the parent/child issue with timer/watchdog is resolved I am now
> having an issue trying to access "iopclk" from "gxp-timer". I have tried use of
>  of_clk_get_by_name and of_clk_get which both fail to find the clock.
> Is it because clocks is outside of the axi -> ahb hierarchy?

No, that should work, but you have to pass the right device node, which would
correspond to the parent device. You should also be able to pass the parent
device pointer (i.e. linux device, not device_node) and use clk_get().

        Arnd