[PATCH v1 0/8] Add device tree for IBM system1 BMC

[PATCH v1 0/8] Add device tree for IBM system1 BMC

[Ninad Palsule]

This patchset adds device tree for IBM system1 bmc board.

Change log:
v1:
 - Added device binding for IBM system1-bmc
 - Added device binding for TIS I2C devices
 - Added device tree for IBM system1 BMC board
 - Added i2c and muxes
 - Added voltage regulators
 - Added GPIO, Fan ctrl, Led
 - Added more compatible strings for tpm_tis_i2c
 - Added power supplies, sensors, EEPROMS, TPM and more

Andrew Geissler (1):
  ARM: dts: aspeed: System1: IBM system1 BMC board

Joel Stanley (1):
  tpm: tis-i2c: Add more compatible strings

Johannes Holland (1):
  dt-bindings: tpm: Add schema for TIS I2C devices

Ninad Palsule (5):
  dt-bindings: arm: aspeed: add IBM system1-bmc
  ARM: dts: aspeed: System1: Add i2c and muxes
  ARM: dts: aspeed: System1: Voltage regulators
  ARM: dts: aspeed: System1: GPIO, Fan ctrl, Led
  ARM: dts: aspeed: System1: PS, sensor and more

 .../bindings/arm/aspeed/aspeed.yaml           |    1 +
 .../bindings/security/tpm/tpm-tis-i2c.yaml    |   50 +
 .../devicetree/bindings/trivial-devices.yaml  |    2 +
 arch/arm/boot/dts/aspeed/Makefile             |    1 +
 .../dts/aspeed/aspeed-bmc-ibm-system1.dts     | 1644 +++++++++++++++++
 drivers/char/tpm/tpm_tis_i2c.c                |    2 +
 6 files changed, 1700 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
 create mode 100644 arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts

-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 1/8] dt-bindings: arm: aspeed: add IBM system1-bmc

[Ninad Palsule]

Document the new compatibles used on IBM system1-bmc

Tested:
    This board is tested using the simics simulator.

Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
 Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml | 1 +
 Documentation/devicetree/bindings/trivial-devices.yaml   | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml b/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml
index 6f7543463d89..ebebe14c42aa 100644
--- a/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml
+++ b/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml
@@ -85,6 +85,7 @@ properties:
               - facebook,yosemite4-bmc
               - ibm,everest-bmc
               - ibm,rainier-bmc
+              - ibm,system1-bmc
               - ibm,tacoma-bmc
               - inventec,starscream-bmc
               - inventec,transformer-bmc
diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 441b55723675..b12a60d2eb0f 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -135,6 +135,8 @@ properties:
           - ibm,cffps1
             # IBM Common Form Factor Power Supply Versions 2
           - ibm,cffps2
+            # Infineon barometric pressure and temperature sensor
+          - infineon,dps310
             # Infineon IR36021 digital POL buck controller
           - infineon,ir36021
             # Infineon IR38060 Voltage Regulator
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/8] dt-bindings: arm: aspeed: add IBM system1-bmc

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1902 bytes --]

On Tue, Dec 12, 2023 at 10:39:57AM -0600, Ninad Palsule wrote:
> Document the new compatibles used on IBM system1-bmc
> 
> Tested:
>     This board is tested using the simics simulator.

I don't see how this is relevant to dt-bindings patches.

> 
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
>  Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml | 1 +
>  Documentation/devicetree/bindings/trivial-devices.yaml   | 2 ++

IMO these should be split into two patches.

Cheers,
Conor.

>  2 files changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml b/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml
> index 6f7543463d89..ebebe14c42aa 100644
> --- a/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml
> +++ b/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml
> @@ -85,6 +85,7 @@ properties:
>                - facebook,yosemite4-bmc
>                - ibm,everest-bmc
>                - ibm,rainier-bmc
> +              - ibm,system1-bmc
>                - ibm,tacoma-bmc
>                - inventec,starscream-bmc
>                - inventec,transformer-bmc
> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> index 441b55723675..b12a60d2eb0f 100644
> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> @@ -135,6 +135,8 @@ properties:
>            - ibm,cffps1
>              # IBM Common Form Factor Power Supply Versions 2
>            - ibm,cffps2
> +            # Infineon barometric pressure and temperature sensor
> +          - infineon,dps310
>              # Infineon IR36021 digital POL buck controller
>            - infineon,ir36021
>              # Infineon IR38060 Voltage Regulator
> -- 
> 2.39.2
> 

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/8] dt-bindings: arm: aspeed: add IBM system1-bmc

[Ninad Palsule]

Hello Conor,

On 12/12/23 11:09, Conor Dooley wrote:
> On Tue, Dec 12, 2023 at 10:39:57AM -0600, Ninad Palsule wrote:
>> Document the new compatibles used on IBM system1-bmc
>>
>> Tested:
>>      This board is tested using the simics simulator.
> I don't see how this is relevant to dt-bindings patches.
Make sense. Removed it.
>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>>   Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml | 1 +
>>   Documentation/devicetree/bindings/trivial-devices.yaml   | 2 ++
> IMO these should be split into two patches.

Make sense. Split it into two patches.

Thank you for the review.

Thanks & Regards,

Ninad

>
> Cheers,
> Conor.
>
>>   2 files changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml b/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml
>> index 6f7543463d89..ebebe14c42aa 100644
>> --- a/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml
>> +++ b/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml
>> @@ -85,6 +85,7 @@ properties:
>>                 - facebook,yosemite4-bmc
>>                 - ibm,everest-bmc
>>                 - ibm,rainier-bmc
>> +              - ibm,system1-bmc
>>                 - ibm,tacoma-bmc
>>                 - inventec,starscream-bmc
>>                 - inventec,transformer-bmc
>> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
>> index 441b55723675..b12a60d2eb0f 100644
>> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
>> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
>> @@ -135,6 +135,8 @@ properties:
>>             - ibm,cffps1
>>               # IBM Common Form Factor Power Supply Versions 2
>>             - ibm,cffps2
>> +            # Infineon barometric pressure and temperature sensor
>> +          - infineon,dps310
>>               # Infineon IR36021 digital POL buck controller
>>             - infineon,ir36021
>>               # Infineon IR38060 Voltage Regulator
>> -- 
>> 2.39.2
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/8] dt-bindings: arm: aspeed: add IBM system1-bmc

[Jarkko Sakkinen]

On Tue Dec 12, 2023 at 6:39 PM EET, Ninad Palsule wrote:
> Document the new compatibles used on IBM system1-bmc
>
> Tested:

this not very useful line :-) (nit)

>     This board is tested using the simics simulator.

Just leave this (w/o indentation)

>
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
>  Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml | 1 +
>  Documentation/devicetree/bindings/trivial-devices.yaml   | 2 ++
>  2 files changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml b/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml
> index 6f7543463d89..ebebe14c42aa 100644
> --- a/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml
> +++ b/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml
> @@ -85,6 +85,7 @@ properties:
>                - facebook,yosemite4-bmc
>                - ibm,everest-bmc
>                - ibm,rainier-bmc
> +              - ibm,system1-bmc
>                - ibm,tacoma-bmc
>                - inventec,starscream-bmc
>                - inventec,transformer-bmc
> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> index 441b55723675..b12a60d2eb0f 100644
> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> @@ -135,6 +135,8 @@ properties:
>            - ibm,cffps1
>              # IBM Common Form Factor Power Supply Versions 2
>            - ibm,cffps2
> +            # Infineon barometric pressure and temperature sensor
> +          - infineon,dps310
>              # Infineon IR36021 digital POL buck controller
>            - infineon,ir36021
>              # Infineon IR38060 Voltage Regulator

BR, Jarkko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/8] dt-bindings: arm: aspeed: add IBM system1-bmc

[Ninad Palsule]

Hello Jarkko,

On 12/13/23 12:18, Jarkko Sakkinen wrote:
> On Tue Dec 12, 2023 at 6:39 PM EET, Ninad Palsule wrote:
>> Document the new compatibles used on IBM system1-bmc
>>
>> Tested:
> this not very useful line :-) (nit)
>
>>      This board is tested using the simics simulator.
> Just leave this (w/o indentation)

Based on Conor's suggestion, I have removed both lines.

Thanks for the review.

Thanks & Regards,

Ninad

>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>>   Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml | 1 +
>>   Documentation/devicetree/bindings/trivial-devices.yaml   | 2 ++
>>   2 files changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml b/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml
>> index 6f7543463d89..ebebe14c42aa 100644
>> --- a/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml
>> +++ b/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml
>> @@ -85,6 +85,7 @@ properties:
>>                 - facebook,yosemite4-bmc
>>                 - ibm,everest-bmc
>>                 - ibm,rainier-bmc
>> +              - ibm,system1-bmc
>>                 - ibm,tacoma-bmc
>>                 - inventec,starscream-bmc
>>                 - inventec,transformer-bmc
>> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
>> index 441b55723675..b12a60d2eb0f 100644
>> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
>> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
>> @@ -135,6 +135,8 @@ properties:
>>             - ibm,cffps1
>>               # IBM Common Form Factor Power Supply Versions 2
>>             - ibm,cffps2
>> +            # Infineon barometric pressure and temperature sensor
>> +          - infineon,dps310
>>               # Infineon IR36021 digital POL buck controller
>>             - infineon,ir36021
>>               # Infineon IR38060 Voltage Regulator
> BR, Jarkko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 2/8] dt-bindings: tpm: Add schema for TIS I2C devices

[Ninad Palsule]

From: Johannes Holland <johannes.holland@infineon.com>

Add a dt schema to support device tree bindings for the generic I2C
physical layer. Refer to the TCG PC Client Platform TPM Profile (PTP)
Specification for TPM 2.0 v1.04 Revision 14.

This includes descriptions for the Nuvoton and Infineon devices.

OpenBMC-Staging-Count: 3
Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
 .../bindings/security/tpm/tpm-tis-i2c.yaml    | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml

diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
new file mode 100644
index 000000000000..de1e34065748
--- /dev/null
+++ b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/security/tpm/tpm-tis-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: I2C PTP based TPM Devices
+
+maintainers:
+  - Johannes Holland <johannes.holland@infineon.com>
+
+description:
+  Device Tree Bindings for I2C based Trusted Platform Module (TPM).
+
+properties:
+  $nodename:
+    pattern: "^tpm(@[0-9a-f]+)?$"
+
+  compatible:
+    oneOf:
+      - description: Infineon's Trusted Platform Module (TPM) (SLB9673).
+        items:
+          - const: infineon,slb9673
+          - const: tcg,tpm-tis-i2c
+      - description: Nuvoton's Trusted Platform Module (TPM) (NPCT75x).
+        items:
+          - const: nuvoton,npct75x
+          - const: tcg,tpm-tis-i2c
+      - const: tcg,tpm-tis-i2c
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      tpm@2e {
+        compatible = "nuvoton,npct75x", "tcg,tpm-tis-i2c";
+        reg = <0x2e>;
+      };
+    };
+...
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/8] dt-bindings: tpm: Add schema for TIS I2C devices

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 2860 bytes --]

Hey,

On Tue, Dec 12, 2023 at 10:39:58AM -0600, Ninad Palsule wrote:
> From: Johannes Holland <johannes.holland@infineon.com>
> 
> Add a dt schema to support device tree bindings

"Add bindings for..."

> for the generic I2C
> physical layer. Refer to the TCG PC Client Platform TPM Profile (PTP)
> Specification for TPM 2.0 v1.04 Revision 14.
> 
> This includes descriptions for the Nuvoton and Infineon devices.
> 

> OpenBMC-Staging-Count: 3

I have no idea what this is, but it needs to be removed from the patch.

> Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
>  .../bindings/security/tpm/tpm-tis-i2c.yaml    | 50 +++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> 
> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> new file mode 100644
> index 000000000000..de1e34065748
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/security/tpm/tpm-tis-i2c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: I2C PTP based TPM Devices
> +
> +maintainers:
> +  - Johannes Holland <johannes.holland@infineon.com>
> +
> +description:
> +  Device Tree Bindings for I2C based Trusted Platform Module (TPM).

s/Device Tree Bindings for //. Doesn't dt_binding_check now complain if
you have this in a title or description?

> +properties:
> +  $nodename:
> +    pattern: "^tpm(@[0-9a-f]+)?$"
> +
> +  compatible:
> +    oneOf:
> +      - description: Infineon's Trusted Platform Module (TPM) (SLB9673).
> +        items:
> +          - const: infineon,slb9673
> +          - const: tcg,tpm-tis-i2c
> +      - description: Nuvoton's Trusted Platform Module (TPM) (NPCT75x).
> +        items:
> +          - const: nuvoton,npct75x
> +          - const: tcg,tpm-tis-i2c

> +      - const: tcg,tpm-tis-i2c

IMO this should be removed and this fallback should only be used in
combination with device specific compatibles, like you have here for the
infineon and nuvoton devices.

Cheers,
Conor.

> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      tpm@2e {
> +        compatible = "nuvoton,npct75x", "tcg,tpm-tis-i2c";
> +        reg = <0x2e>;
> +      };
> +    };
> +...
> -- 
> 2.39.2
> 

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/8] dt-bindings: tpm: Add schema for TIS I2C devices

[Guenter Roeck]

On Tue, Dec 12, 2023 at 05:14:26PM +0000, Conor Dooley wrote:
> Hey,
> 
> On Tue, Dec 12, 2023 at 10:39:58AM -0600, Ninad Palsule wrote:
> > From: Johannes Holland <johannes.holland@infineon.com>
> > 
> > Add a dt schema to support device tree bindings
> 
> "Add bindings for..."
> 
> > for the generic I2C
> > physical layer. Refer to the TCG PC Client Platform TPM Profile (PTP)
> > Specification for TPM 2.0 v1.04 Revision 14.
> > 
> > This includes descriptions for the Nuvoton and Infineon devices.
> > 
> 
> > OpenBMC-Staging-Count: 3
> 
> I have no idea what this is, but it needs to be removed from the patch.
> 
> > Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
> > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> > ---
> >  .../bindings/security/tpm/tpm-tis-i2c.yaml    | 50 +++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> > new file mode 100644
> > index 000000000000..de1e34065748
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> > @@ -0,0 +1,50 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/security/tpm/tpm-tis-i2c.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: I2C PTP based TPM Devices
> > +
> > +maintainers:
> > +  - Johannes Holland <johannes.holland@infineon.com>
> > +
> > +description:
> > +  Device Tree Bindings for I2C based Trusted Platform Module (TPM).
> 
> s/Device Tree Bindings for //. Doesn't dt_binding_check now complain if
> you have this in a title or description?
> 
> > +properties:
> > +  $nodename:
> > +    pattern: "^tpm(@[0-9a-f]+)?$"
> > +
> > +  compatible:
> > +    oneOf:
> > +      - description: Infineon's Trusted Platform Module (TPM) (SLB9673).
> > +        items:
> > +          - const: infineon,slb9673
> > +          - const: tcg,tpm-tis-i2c
> > +      - description: Nuvoton's Trusted Platform Module (TPM) (NPCT75x).
> > +        items:
> > +          - const: nuvoton,npct75x
> > +          - const: tcg,tpm-tis-i2c
> 
> > +      - const: tcg,tpm-tis-i2c
> 
> IMO this should be removed and this fallback should only be used in
> combination with device specific compatibles, like you have here for the
> infineon and nuvoton devices.

As mentioned in my response to the other patch, "only" isn't sufficient
since the tacoma devicetree file only references the generic entry.
It would also make support for chips from other vendors unnecessarily
complex.

Question should in my opinion be if the non-fallback entries are really
needed.

Thanks,
Guenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/8] dt-bindings: tpm: Add schema for TIS I2C devices

[Ninad Palsule]

Hello Guenter,

On 12/12/23 12:20, Guenter Roeck wrote:
> On Tue, Dec 12, 2023 at 05:14:26PM +0000, Conor Dooley wrote:
>> Hey,
>>
>> On Tue, Dec 12, 2023 at 10:39:58AM -0600, Ninad Palsule wrote:
>>> From: Johannes Holland <johannes.holland@infineon.com>
>>>
>>> Add a dt schema to support device tree bindings
>> "Add bindings for..."
>>
>>> for the generic I2C
>>> physical layer. Refer to the TCG PC Client Platform TPM Profile (PTP)
>>> Specification for TPM 2.0 v1.04 Revision 14.
>>>
>>> This includes descriptions for the Nuvoton and Infineon devices.
>>>
>>> OpenBMC-Staging-Count: 3
>> I have no idea what this is, but it needs to be removed from the patch.
>>
>>> Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
>>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>>> ---
>>>   .../bindings/security/tpm/tpm-tis-i2c.yaml    | 50 +++++++++++++++++++
>>>   1 file changed, 50 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
>>> new file mode 100644
>>> index 000000000000..de1e34065748
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
>>> @@ -0,0 +1,50 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/security/tpm/tpm-tis-i2c.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: I2C PTP based TPM Devices
>>> +
>>> +maintainers:
>>> +  - Johannes Holland <johannes.holland@infineon.com>
>>> +
>>> +description:
>>> +  Device Tree Bindings for I2C based Trusted Platform Module (TPM).
>> s/Device Tree Bindings for //. Doesn't dt_binding_check now complain if
>> you have this in a title or description?
>>
>>> +properties:
>>> +  $nodename:
>>> +    pattern: "^tpm(@[0-9a-f]+)?$"
>>> +
>>> +  compatible:
>>> +    oneOf:
>>> +      - description: Infineon's Trusted Platform Module (TPM) (SLB9673).
>>> +        items:
>>> +          - const: infineon,slb9673
>>> +          - const: tcg,tpm-tis-i2c
>>> +      - description: Nuvoton's Trusted Platform Module (TPM) (NPCT75x).
>>> +        items:
>>> +          - const: nuvoton,npct75x
>>> +          - const: tcg,tpm-tis-i2c
>>> +      - const: tcg,tpm-tis-i2c
>> IMO this should be removed and this fallback should only be used in
>> combination with device specific compatibles, like you have here for the
>> infineon and nuvoton devices.
> As mentioned in my response to the other patch, "only" isn't sufficient
> since the tacoma devicetree file only references the generic entry.
> It would also make support for chips from other vendors unnecessarily
> complex.
>
> Question should in my opinion be if the non-fallback entries are really
> needed.

Thanks for the response. I think generic option is in-case we have a 
chip whose specific driver is not available.

Regards,

Ninad

>
> Thanks,
> Guenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/8] dt-bindings: tpm: Add schema for TIS I2C devices

[Ninad Palsule]

Hello Conor,

On 12/12/23 11:14, Conor Dooley wrote:
> Hey,
>
> On Tue, Dec 12, 2023 at 10:39:58AM -0600, Ninad Palsule wrote:
>> From: Johannes Holland <johannes.holland@infineon.com>
>>
>> Add a dt schema to support device tree bindings
> "Add bindings for..."
Fixed.
>
>> for the generic I2C
>> physical layer. Refer to the TCG PC Client Platform TPM Profile (PTP)
>> Specification for TPM 2.0 v1.04 Revision 14.
>>
>> This includes descriptions for the Nuvoton and Infineon devices.
>>
>> OpenBMC-Staging-Count: 3
> I have no idea what this is, but it needs to be removed from the patch.
Removed.
>
>> Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>>   .../bindings/security/tpm/tpm-tis-i2c.yaml    | 50 +++++++++++++++++++
>>   1 file changed, 50 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
>> new file mode 100644
>> index 000000000000..de1e34065748
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
>> @@ -0,0 +1,50 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/security/tpm/tpm-tis-i2c.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: I2C PTP based TPM Devices
>> +
>> +maintainers:
>> +  - Johannes Holland <johannes.holland@infineon.com>
>> +
>> +description:
>> +  Device Tree Bindings for I2C based Trusted Platform Module (TPM).
> s/Device Tree Bindings for //. Doesn't dt_binding_check now complain if
> you have this in a title or description?
Fixed.
>
>> +properties:
>> +  $nodename:
>> +    pattern: "^tpm(@[0-9a-f]+)?$"
>> +
>> +  compatible:
>> +    oneOf:
>> +      - description: Infineon's Trusted Platform Module (TPM) (SLB9673).
>> +        items:
>> +          - const: infineon,slb9673
>> +          - const: tcg,tpm-tis-i2c
>> +      - description: Nuvoton's Trusted Platform Module (TPM) (NPCT75x).
>> +        items:
>> +          - const: nuvoton,npct75x
>> +          - const: tcg,tpm-tis-i2c
>> +      - const: tcg,tpm-tis-i2c
> IMO this should be removed and this fallback should only be used in
> combination with device specific compatibles, like you have here for the
> infineon and nuvoton devices.

As Guenter mentioned I need to keep it as tacoma board is just using 
this string.

Thanks for the review.

Regards,

Ninad

>
> Cheers,
> Conor.
>
>> +  reg:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      tpm@2e {
>> +        compatible = "nuvoton,npct75x", "tcg,tpm-tis-i2c";
>> +        reg = <0x2e>;
>> +      };
>> +    };
>> +...
>> -- 
>> 2.39.2
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/8] dt-bindings: tpm: Add schema for TIS I2C devices

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 3438 bytes --]

On Thu, Dec 14, 2023 at 09:34:39AM -0600, Ninad Palsule wrote:
> Hello Conor,
> 
> On 12/12/23 11:14, Conor Dooley wrote:
> > Hey,
> > 
> > On Tue, Dec 12, 2023 at 10:39:58AM -0600, Ninad Palsule wrote:
> > > From: Johannes Holland <johannes.holland@infineon.com>
> > > 
> > > Add a dt schema to support device tree bindings
> > "Add bindings for..."
> Fixed.
> > 
> > > for the generic I2C
> > > physical layer. Refer to the TCG PC Client Platform TPM Profile (PTP)
> > > Specification for TPM 2.0 v1.04 Revision 14.
> > > 
> > > This includes descriptions for the Nuvoton and Infineon devices.
> > > 
> > > OpenBMC-Staging-Count: 3
> > I have no idea what this is, but it needs to be removed from the patch.
> Removed.
> > 
> > > Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
> > > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > > Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> > > ---
> > >   .../bindings/security/tpm/tpm-tis-i2c.yaml    | 50 +++++++++++++++++++
> > >   1 file changed, 50 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> > > new file mode 100644
> > > index 000000000000..de1e34065748
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> > > @@ -0,0 +1,50 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/security/tpm/tpm-tis-i2c.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: I2C PTP based TPM Devices
> > > +
> > > +maintainers:
> > > +  - Johannes Holland <johannes.holland@infineon.com>
> > > +
> > > +description:
> > > +  Device Tree Bindings for I2C based Trusted Platform Module (TPM).
> > s/Device Tree Bindings for //. Doesn't dt_binding_check now complain if
> > you have this in a title or description?
> Fixed.
> > 
> > > +properties:
> > > +  $nodename:
> > > +    pattern: "^tpm(@[0-9a-f]+)?$"
> > > +
> > > +  compatible:
> > > +    oneOf:
> > > +      - description: Infineon's Trusted Platform Module (TPM) (SLB9673).
> > > +        items:
> > > +          - const: infineon,slb9673
> > > +          - const: tcg,tpm-tis-i2c
> > > +      - description: Nuvoton's Trusted Platform Module (TPM) (NPCT75x).
> > > +        items:
> > > +          - const: nuvoton,npct75x
> > > +          - const: tcg,tpm-tis-i2c

Also, another thought - the bus is not usually encoded in the compatible
string, so it would be good to remove that.

> > > +      - const: tcg,tpm-tis-i2c
> > IMO this should be removed and this fallback should only be used in
> > combination with device specific compatibles, like you have here for the
> > infineon and nuvoton devices.
> 
> As Guenter mentioned I need to keep it as tacoma board is just using this
> string.

No, that does not mean that you have to keep this in the binding. I know
Rob had some comments that might invalidate this binding entirely, but
if that does not happen then I think think that the tacoma devicetree
needs to have a device-specific compatible added for the tpm that it has.
You could of course retain the generic fallback compatible however.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/8] dt-bindings: tpm: Add schema for TIS I2C devices

[Rob Herring]

On Thu, Dec 14, 2023 at 10:35 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, Dec 14, 2023 at 09:34:39AM -0600, Ninad Palsule wrote:
> > Hello Conor,
> >
> > On 12/12/23 11:14, Conor Dooley wrote:
> > > Hey,
> > >
> > > On Tue, Dec 12, 2023 at 10:39:58AM -0600, Ninad Palsule wrote:
> > > > From: Johannes Holland <johannes.holland@infineon.com>
> > > >
> > > > Add a dt schema to support device tree bindings
> > > "Add bindings for..."
> > Fixed.
> > >
> > > > for the generic I2C
> > > > physical layer. Refer to the TCG PC Client Platform TPM Profile (PTP)
> > > > Specification for TPM 2.0 v1.04 Revision 14.
> > > >
> > > > This includes descriptions for the Nuvoton and Infineon devices.
> > > >
> > > > OpenBMC-Staging-Count: 3
> > > I have no idea what this is, but it needs to be removed from the patch.
> > Removed.
> > >
> > > > Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
> > > > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > > > Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> > > > ---
> > > >   .../bindings/security/tpm/tpm-tis-i2c.yaml    | 50 +++++++++++++++++++
> > > >   1 file changed, 50 insertions(+)
> > > >   create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> > > > new file mode 100644
> > > > index 000000000000..de1e34065748
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> > > > @@ -0,0 +1,50 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/security/tpm/tpm-tis-i2c.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: I2C PTP based TPM Devices
> > > > +
> > > > +maintainers:
> > > > +  - Johannes Holland <johannes.holland@infineon.com>
> > > > +
> > > > +description:
> > > > +  Device Tree Bindings for I2C based Trusted Platform Module (TPM).
> > > s/Device Tree Bindings for //. Doesn't dt_binding_check now complain if
> > > you have this in a title or description?
> > Fixed.
> > >
> > > > +properties:
> > > > +  $nodename:
> > > > +    pattern: "^tpm(@[0-9a-f]+)?$"
> > > > +
> > > > +  compatible:
> > > > +    oneOf:
> > > > +      - description: Infineon's Trusted Platform Module (TPM) (SLB9673).
> > > > +        items:
> > > > +          - const: infineon,slb9673
> > > > +          - const: tcg,tpm-tis-i2c
> > > > +      - description: Nuvoton's Trusted Platform Module (TPM) (NPCT75x).
> > > > +        items:
> > > > +          - const: nuvoton,npct75x
> > > > +          - const: tcg,tpm-tis-i2c
>
> Also, another thought - the bus is not usually encoded in the compatible
> string, so it would be good to remove that.

True, but we already have 3 different bus variants in this case. So
that ship has sailed.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/8] dt-bindings: tpm: Add schema for TIS I2C devices

[Rob Herring]

On Tue, Dec 12, 2023 at 10:39:58AM -0600, Ninad Palsule wrote:
> From: Johannes Holland <johannes.holland@infineon.com>
> 
> Add a dt schema to support device tree bindings for the generic I2C
> physical layer. Refer to the TCG PC Client Platform TPM Profile (PTP)
> Specification for TPM 2.0 v1.04 Revision 14.
> 
> This includes descriptions for the Nuvoton and Infineon devices.

This is incomplete and conflicts with this series[1]. Please help 
review and make sure it works for the cases you care about.

Rob

[1] https://lore.kernel.org/all/cover.1701093036.git.lukas@wunner.de/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/8] dt-bindings: tpm: Add schema for TIS I2C devices

[Ninad Palsule]

Hello Rob,

On 12/13/23 10:13, Rob Herring wrote:
> On Tue, Dec 12, 2023 at 10:39:58AM -0600, Ninad Palsule wrote:
>> From: Johannes Holland <johannes.holland@infineon.com>
>>
>> Add a dt schema to support device tree bindings for the generic I2C
>> physical layer. Refer to the TCG PC Client Platform TPM Profile (PTP)
>> Specification for TPM 2.0 v1.04 Revision 14.
>>
>> This includes descriptions for the Nuvoton and Infineon devices.
> This is incomplete and conflicts with this series[1]. Please help
> review and make sure it works for the cases you care about.
>
> Rob
>
> [1] https://lore.kernel.org/all/cover.1701093036.git.lukas@wunner.de/

I will take a look at the patchset. How do you want to handle mine? Do 
you want me to send patch as per the new directory structure?

Thanks for the review.

Regards,

Ninad


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/8] dt-bindings: tpm: Add schema for TIS I2C devices

[Ninad Palsule]

Hi Rob,

On 12/13/23 10:13, Rob Herring wrote:
> On Tue, Dec 12, 2023 at 10:39:58AM -0600, Ninad Palsule wrote:
>> From: Johannes Holland <johannes.holland@infineon.com>
>>
>> Add a dt schema to support device tree bindings for the generic I2C
>> physical layer. Refer to the TCG PC Client Platform TPM Profile (PTP)
>> Specification for TPM 2.0 v1.04 Revision 14.
>>
>> This includes descriptions for the Nuvoton and Infineon devices.
> This is incomplete and conflicts with this series[1]. Please help
> review and make sure it works for the cases you care about.
>
> Rob
>
> [1] https://lore.kernel.org/all/cover.1701093036.git.lukas@wunner.de/

Looks like my patch "dt-bindings: tpm: Add schema for TIS I2C devices" 
is not required. I am removing it.

Thanks for the review.

Regards,

Ninad


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/8] dt-bindings: tpm: Add schema for TIS I2C devices

[Jarkko Sakkinen]

On Tue Dec 12, 2023 at 6:39 PM EET, Ninad Palsule wrote:
> From: Johannes Holland <johannes.holland@infineon.com>
>
> Add a dt schema to support device tree bindings for the generic I2C
> physical layer. Refer to the TCG PC Client Platform TPM Profile (PTP)
> Specification for TPM 2.0 v1.04 Revision 14.
>
> This includes descriptions for the Nuvoton and Infineon devices.
>
> OpenBMC-Staging-Count: 3

Please don't invent your own tags.

> Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
>  .../bindings/security/tpm/tpm-tis-i2c.yaml    | 50 +++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
>
> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> new file mode 100644
> index 000000000000..de1e34065748
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/security/tpm/tpm-tis-i2c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: I2C PTP based TPM Devices
> +
> +maintainers:
> +  - Johannes Holland <johannes.holland@infineon.com>
> +
> +description:
> +  Device Tree Bindings for I2C based Trusted Platform Module (TPM).
> +
> +properties:
> +  $nodename:
> +    pattern: "^tpm(@[0-9a-f]+)?$"
> +
> +  compatible:
> +    oneOf:
> +      - description: Infineon's Trusted Platform Module (TPM) (SLB9673).
> +        items:
> +          - const: infineon,slb9673
> +          - const: tcg,tpm-tis-i2c
> +      - description: Nuvoton's Trusted Platform Module (TPM) (NPCT75x).
> +        items:
> +          - const: nuvoton,npct75x
> +          - const: tcg,tpm-tis-i2c
> +      - const: tcg,tpm-tis-i2c
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      tpm@2e {
> +        compatible = "nuvoton,npct75x", "tcg,tpm-tis-i2c";
> +        reg = <0x2e>;
> +      };
> +    };
> +...


BR, Jarkko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/8] dt-bindings: tpm: Add schema for TIS I2C devices

[Ninad Palsule]

Hello Jarkko,

On 12/13/23 12:20, Jarkko Sakkinen wrote:
> On Tue Dec 12, 2023 at 6:39 PM EET, Ninad Palsule wrote:
>> From: Johannes Holland <johannes.holland@infineon.com>
>>
>> Add a dt schema to support device tree bindings for the generic I2C
>> physical layer. Refer to the TCG PC Client Platform TPM Profile (PTP)
>> Specification for TPM 2.0 v1.04 Revision 14.
>>
>> This includes descriptions for the Nuvoton and Infineon devices.
>>
>> OpenBMC-Staging-Count: 3
> Please don't invent your own tags.

Yes, Sorry. I have cherry-picked this commit from openbmc. Now I have 
removed this line.

Thanks for the review.

Thanks & Regards,

Ninad

>
>> Signed-off-by: Johannes Holland <johannes.holland@infineon.com>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>>   .../bindings/security/tpm/tpm-tis-i2c.yaml    | 50 +++++++++++++++++++
>>   1 file changed, 50 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
>> new file mode 100644
>> index 000000000000..de1e34065748
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/security/tpm/tpm-tis-i2c.yaml
>> @@ -0,0 +1,50 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/security/tpm/tpm-tis-i2c.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: I2C PTP based TPM Devices
>> +
>> +maintainers:
>> +  - Johannes Holland <johannes.holland@infineon.com>
>> +
>> +description:
>> +  Device Tree Bindings for I2C based Trusted Platform Module (TPM).
>> +
>> +properties:
>> +  $nodename:
>> +    pattern: "^tpm(@[0-9a-f]+)?$"
>> +
>> +  compatible:
>> +    oneOf:
>> +      - description: Infineon's Trusted Platform Module (TPM) (SLB9673).
>> +        items:
>> +          - const: infineon,slb9673
>> +          - const: tcg,tpm-tis-i2c
>> +      - description: Nuvoton's Trusted Platform Module (TPM) (NPCT75x).
>> +        items:
>> +          - const: nuvoton,npct75x
>> +          - const: tcg,tpm-tis-i2c
>> +      - const: tcg,tpm-tis-i2c
>> +  reg:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      tpm@2e {
>> +        compatible = "nuvoton,npct75x", "tcg,tpm-tis-i2c";
>> +        reg = <0x2e>;
>> +      };
>> +    };
>> +...
>
> BR, Jarkko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 3/8] ARM: dts: aspeed: System1: IBM system1 BMC board

[Ninad Palsule]

From: Andrew Geissler <geissonator@yahoo.com>

Add a device tree for IBM system1 BMC board. It uses AST2600 SOC.

Tested:
    This board is tested using the simics simulator.

Signed-off-by: Andrew Geissler <geissonator@yahoo.com>
Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
 arch/arm/boot/dts/aspeed/Makefile             |   1 +
 .../dts/aspeed/aspeed-bmc-ibm-system1.dts     | 188 ++++++++++++++++++
 2 files changed, 189 insertions(+)
 create mode 100644 arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts

diff --git a/arch/arm/boot/dts/aspeed/Makefile b/arch/arm/boot/dts/aspeed/Makefile
index 3e3e6b96cb79..6f7ed11978ff 100644
--- a/arch/arm/boot/dts/aspeed/Makefile
+++ b/arch/arm/boot/dts/aspeed/Makefile
@@ -35,6 +35,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
 	aspeed-bmc-ibm-rainier.dtb \
 	aspeed-bmc-ibm-rainier-1s4u.dtb \
 	aspeed-bmc-ibm-rainier-4u.dtb \
+	aspeed-bmc-ibm-system1.dtb \
 	aspeed-bmc-intel-s2600wf.dtb \
 	aspeed-bmc-inspur-fp5280g2.dtb \
 	aspeed-bmc-inspur-nf5280m6.dtb \
diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
new file mode 100644
index 000000000000..01291b407f59
--- /dev/null
+++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
@@ -0,0 +1,188 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright 2023 IBM Corp.
+/dts-v1/;
+
+#include "aspeed-g6.dtsi"
+#include <dt-bindings/gpio/aspeed-gpio.h>
+#include <dt-bindings/i2c/i2c.h>
+#include <dt-bindings/leds/leds-pca955x.h>
+
+/ {
+	model = "System1";
+	compatible = "ibm,system1-bmc", "aspeed,ast2600";
+
+	chosen {
+		stdout-path = &uart5;
+		bootargs = "console=ttyS4,115200n8 earlycon";
+	};
+
+	memory@80000000 {
+		device_type = "memory";
+		reg = <0x80000000 0x40000000>;
+	};
+
+	reserved-memory {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+
+		event_log: tcg_event_log@b3d00000 {
+			no-map;
+			reg = <0xb3d00000 0x100000>;
+		};
+
+		ramoops@b3e00000 {
+			compatible = "ramoops";
+			reg = <0xb3e00000 0x200000>; /* 16 * (4 * 0x8000) */
+			record-size = <0x8000>;
+			console-size = <0x8000>;
+			ftrace-size = <0x8000>;
+			pmsg-size = <0x8000>;
+			max-reason = <3>; /* KMSG_DUMP_EMERG */
+		};
+
+		/* LPC FW cycle bridge region requires natural alignment */
+		flash_memory: region@b4000000 {
+			no-map;
+			reg = <0xb4000000 0x04000000>; /* 64M */
+		};
+
+		/* VGA region is dictated by hardware strapping */
+		vga_memory: region@bf000000 {
+			no-map;
+			compatible = "shared-dma-pool";
+			reg = <0xbf000000 0x01000000>;  /* 16M */
+		};
+	};
+};
+
+&adc1 {
+	status = "okay";
+	aspeed,int-vref-microvolt = <2500000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_adc8_default
+		&pinctrl_adc9_default
+		&pinctrl_adc10_default
+		&pinctrl_adc11_default
+		&pinctrl_adc12_default
+		&pinctrl_adc13_default
+		&pinctrl_adc14_default
+		&pinctrl_adc15_default>;
+};
+
+&ehci1 {
+	status = "okay";
+};
+
+&uhci {
+	status = "okay";
+};
+
+&gpio0 {
+	gpio-line-names =
+	/*A0-A7*/	"","","","","","","","",
+	/*B0-B7*/	"","","","","","","","",
+	/*C0-C7*/	"","","","","","","","",
+	/*D0-D7*/	"","","","","","","","",
+	/*E0-E7*/	"","","","","","","","",
+	/*F0-F7*/	"","","","","","","","",
+	/*G0-G7*/	"","","","","","","","",
+	/*H0-H7*/	"","","","","","","","",
+	/*I0-I7*/	"","","","","","","","",
+	/*J0-J7*/	"","","","","","","","",
+	/*K0-K7*/	"","","","","","","","",
+	/*L0-L7*/	"","","","","","","","",
+	/*M0-M7*/	"","","","","","","","",
+	/*N0-N7*/	"","","","","","","","",
+	/*O0-O7*/	"","","","","","","","",
+	/*P0-P7*/	"","","","","","","","",
+	/*Q0-Q7*/	"","","","","","","","",
+	/*R0-R7*/	"","","","","","","","",
+	/*S0-S7*/	"","","","","","","","",
+	/*T0-T7*/	"","","","","","","","",
+	/*U0-U7*/	"","","","","","","","",
+	/*V0-V7*/	"","","","power-chassis-control","","","","",
+	/*W0-W7*/	"","","","","","","","",
+	/*X0-X7*/	"","power-chassis-good","","","","","","",
+	/*Y0-Y7*/	"","","","","","","","",
+	/*Z0-Z7*/	"","","","","","","","";
+};
+
+&emmc_controller {
+	status = "okay";
+};
+
+&pinctrl_emmc_default {
+	bias-disable;
+};
+
+&emmc {
+	status = "okay";
+	clk-phase-mmc-hs200 = <180>, <180>;
+};
+
+&ibt {
+	status = "okay";
+};
+
+&uart2 {
+	status = "okay";
+};
+
+&vuart1 {
+	status = "okay";
+};
+
+&vuart2 {
+	status = "okay";
+};
+
+&lpc_ctrl {
+	status = "okay";
+	memory-region = <&flash_memory>;
+};
+
+&mac2 {
+	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_rmii3_default>;
+	clocks = <&syscon ASPEED_CLK_GATE_MAC3CLK>,
+		 <&syscon ASPEED_CLK_MAC3RCLK>;
+	clock-names = "MACCLK", "RCLK";
+	use-ncsi;
+};
+
+&mac3 {
+	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_rmii4_default>;
+	clocks = <&syscon ASPEED_CLK_GATE_MAC4CLK>,
+		 <&syscon ASPEED_CLK_MAC4RCLK>;
+	clock-names = "MACCLK", "RCLK";
+	use-ncsi;
+};
+
+&wdt1 {
+	aspeed,reset-type = "none";
+	aspeed,external-signal;
+	aspeed,ext-push-pull;
+	aspeed,ext-active-high;
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_wdtrst1_default>;
+};
+
+&wdt2 {
+	status = "okay";
+};
+
+&kcs2 {
+	status = "okay";
+	aspeed,lpc-io-reg = <0xca8 0xcac>;
+};
+
+&kcs3 {
+	status = "okay";
+	aspeed,lpc-io-reg = <0xca2>;
+	aspeed,lpc-interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
+};
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 3/8] ARM: dts: aspeed: System1: IBM system1 BMC board

[Krzysztof Kozlowski]

On 12/12/2023 17:39, Ninad Palsule wrote:
> From: Andrew Geissler <geissonator@yahoo.com>
> 
> Add a device tree for IBM system1 BMC board. It uses AST2600 SOC.
> 
> Tested:
>     This board is tested using the simics simulator.
> 
> Signed-off-by: Andrew Geissler <geissonator@yahoo.com>
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
>  arch/arm/boot/dts/aspeed/Makefile             |   1 +
>  .../dts/aspeed/aspeed-bmc-ibm-system1.dts     | 188 ++++++++++++++++++
>  2 files changed, 189 insertions(+)
>  create mode 100644 arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
> 
> diff --git a/arch/arm/boot/dts/aspeed/Makefile b/arch/arm/boot/dts/aspeed/Makefile
> index 3e3e6b96cb79..6f7ed11978ff 100644
> --- a/arch/arm/boot/dts/aspeed/Makefile
> +++ b/arch/arm/boot/dts/aspeed/Makefile
> @@ -35,6 +35,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>  	aspeed-bmc-ibm-rainier.dtb \
>  	aspeed-bmc-ibm-rainier-1s4u.dtb \
>  	aspeed-bmc-ibm-rainier-4u.dtb \
> +	aspeed-bmc-ibm-system1.dtb \
>  	aspeed-bmc-intel-s2600wf.dtb \
>  	aspeed-bmc-inspur-fp5280g2.dtb \
>  	aspeed-bmc-inspur-nf5280m6.dtb \
> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
> new file mode 100644
> index 000000000000..01291b407f59
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// Copyright 2023 IBM Corp.
> +/dts-v1/;
> +
> +#include "aspeed-g6.dtsi"
> +#include <dt-bindings/gpio/aspeed-gpio.h>
> +#include <dt-bindings/i2c/i2c.h>
> +#include <dt-bindings/leds/leds-pca955x.h>
> +
> +/ {
> +	model = "System1";
> +	compatible = "ibm,system1-bmc", "aspeed,ast2600";
> +
> +	chosen {
> +		stdout-path = &uart5;
> +		bootargs = "console=ttyS4,115200n8 earlycon";

Drop early con, debugging feature not release. Then use stdout path
alone, so drop console as well.


> +	};
> +
> +	memory@80000000 {
> +		device_type = "memory";
> +		reg = <0x80000000 0x40000000>;
> +	};
> +
> +	reserved-memory {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +
> +		event_log: tcg_event_log@b3d00000 {

No underscores in node names.


Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 3/8] ARM: dts: aspeed: System1: IBM system1 BMC board

[Ninad Palsule]

Hello Krzysztof,


On 12/12/23 14:20, Krzysztof Kozlowski wrote:
> On 12/12/2023 17:39, Ninad Palsule wrote:
>> From: Andrew Geissler <geissonator@yahoo.com>
>>
>> Add a device tree for IBM system1 BMC board. It uses AST2600 SOC.
>>
>> Tested:
>>      This board is tested using the simics simulator.
>>
>> Signed-off-by: Andrew Geissler <geissonator@yahoo.com>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>>   arch/arm/boot/dts/aspeed/Makefile             |   1 +
>>   .../dts/aspeed/aspeed-bmc-ibm-system1.dts     | 188 ++++++++++++++++++
>>   2 files changed, 189 insertions(+)
>>   create mode 100644 arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
>>
>> diff --git a/arch/arm/boot/dts/aspeed/Makefile b/arch/arm/boot/dts/aspeed/Makefile
>> index 3e3e6b96cb79..6f7ed11978ff 100644
>> --- a/arch/arm/boot/dts/aspeed/Makefile
>> +++ b/arch/arm/boot/dts/aspeed/Makefile
>> @@ -35,6 +35,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>>   	aspeed-bmc-ibm-rainier.dtb \
>>   	aspeed-bmc-ibm-rainier-1s4u.dtb \
>>   	aspeed-bmc-ibm-rainier-4u.dtb \
>> +	aspeed-bmc-ibm-system1.dtb \
>>   	aspeed-bmc-intel-s2600wf.dtb \
>>   	aspeed-bmc-inspur-fp5280g2.dtb \
>>   	aspeed-bmc-inspur-nf5280m6.dtb \
>> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
>> new file mode 100644
>> index 000000000000..01291b407f59
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
>> @@ -0,0 +1,188 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +// Copyright 2023 IBM Corp.
>> +/dts-v1/;
>> +
>> +#include "aspeed-g6.dtsi"
>> +#include <dt-bindings/gpio/aspeed-gpio.h>
>> +#include <dt-bindings/i2c/i2c.h>
>> +#include <dt-bindings/leds/leds-pca955x.h>
>> +
>> +/ {
>> +	model = "System1";
>> +	compatible = "ibm,system1-bmc", "aspeed,ast2600";
>> +
>> +	chosen {
>> +		stdout-path = &uart5;
>> +		bootargs = "console=ttyS4,115200n8 earlycon";
> Drop early con, debugging feature not release. Then use stdout path
> alone, so drop console as well.
Fixed as per your suggestion.
>
>> +	};
>> +
>> +	memory@80000000 {
>> +		device_type = "memory";
>> +		reg = <0x80000000 0x40000000>;
>> +	};
>> +
>> +	reserved-memory {
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges;
>> +
>> +		event_log: tcg_event_log@b3d00000 {
> No underscores in node names.

Fixed.

Thanks for the review.

Regards,

Ninad

>
>
> Best regards,
> Krzysztof
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 4/8] ARM: dts: aspeed: System1: Add i2c and muxes

[Ninad Palsule]

This commit adds i2c devices and muxes.

Tested:
    This board is tested using the simics simulator.

Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
 .../dts/aspeed/aspeed-bmc-ibm-system1.dts     | 577 ++++++++++++++++++
 1 file changed, 577 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
index 01291b407f59..0557bff9f36a 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
+++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
@@ -11,6 +11,65 @@ / {
 	model = "System1";
 	compatible = "ibm,system1-bmc", "aspeed,ast2600";
 
+	aliases {
+		i2c16 = &i2c8mux1chn0;
+		i2c17 = &i2c8mux1chn1;
+		i2c18 = &i2c8mux1chn2;
+		i2c19 = &i2c8mux1chn3;
+		i2c20 = &i2c8mux1chn4;
+		i2c21 = &i2c8mux1chn5;
+		i2c22 = &i2c8mux1chn6;
+		i2c23 = &i2c8mux1chn7;
+		i2c24 = &i2c3mux0chn0;
+		i2c25 = &i2c3mux0chn1;
+		i2c26 = &i2c3mux0chn2;
+		i2c27 = &i2c3mux0chn3;
+		i2c28 = &i2c3mux0chn4;
+		i2c29 = &i2c3mux0chn5;
+		i2c30 = &i2c3mux0chn6;
+		i2c31 = &i2c3mux0chn7;
+		i2c32 = &i2c6mux0chn0;
+		i2c33 = &i2c6mux0chn1;
+		i2c34 = &i2c6mux0chn2;
+		i2c35 = &i2c6mux0chn3;
+		i2c36 = &i2c6mux0chn4;
+		i2c37 = &i2c6mux0chn5;
+		i2c38 = &i2c6mux0chn6;
+		i2c39 = &i2c6mux0chn7;
+		i2c40 = &i2c7mux0chn0;
+		i2c41 = &i2c7mux0chn1;
+		i2c42 = &i2c7mux0chn2;
+		i2c43 = &i2c7mux0chn3;
+		i2c44 = &i2c7mux0chn4;
+		i2c45 = &i2c7mux0chn5;
+		i2c46 = &i2c7mux0chn6;
+		i2c47 = &i2c7mux0chn7;
+		i2c48 = &i2c8mux0chn0;
+		i2c49 = &i2c8mux0chn1;
+		i2c50 = &i2c8mux0chn2;
+		i2c51 = &i2c8mux0chn3;
+		i2c52 = &i2c8mux0chn4;
+		i2c53 = &i2c8mux0chn5;
+		i2c54 = &i2c8mux0chn6;
+		i2c55 = &i2c8mux0chn7;
+		i2c56 = &i2c14mux0chn0;
+		i2c57 = &i2c14mux0chn1;
+		i2c58 = &i2c14mux0chn2;
+		i2c59 = &i2c14mux0chn3;
+		i2c60 = &i2c14mux0chn4;
+		i2c61 = &i2c14mux0chn5;
+		i2c62 = &i2c14mux0chn6;
+		i2c63 = &i2c14mux0chn7;
+		i2c64 = &i2c15mux0chn0;
+		i2c65 = &i2c15mux0chn1;
+		i2c66 = &i2c15mux0chn2;
+		i2c67 = &i2c15mux0chn3;
+		i2c68 = &i2c15mux0chn4;
+		i2c69 = &i2c15mux0chn5;
+		i2c70 = &i2c15mux0chn6;
+		i2c71 = &i2c15mux0chn7;
+	};
+
 	chosen {
 		stdout-path = &uart5;
 		bootargs = "console=ttyS4,115200n8 earlycon";
@@ -54,10 +113,12 @@ vga_memory: region@bf000000 {
 			reg = <0xbf000000 0x01000000>;  /* 16M */
 		};
 	};
+
 };
 
 &adc1 {
 	status = "okay";
+
 	aspeed,int-vref-microvolt = <2500000>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_adc8_default
@@ -186,3 +247,519 @@ &kcs3 {
 	aspeed,lpc-io-reg = <0xca2>;
 	aspeed,lpc-interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
 };
+
+&i2c0 {
+	status = "okay";
+};
+
+&i2c1 {
+	status = "okay";
+};
+
+&i2c2 {
+	status = "okay";
+};
+
+&i2c3 {
+	status = "okay";
+
+	i2c-mux@70 {
+		compatible = "nxp,pca9548";
+		reg = <0x70>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		status = "okay";
+		i2c-mux-idle-disconnect;
+
+		i2c3mux0chn0: i2c@0 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0>;
+		};
+
+		i2c3mux0chn1: i2c@1 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <1>;
+		};
+
+		i2c3mux0chn2: i2c@2 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <2>;
+		};
+
+		i2c3mux0chn3: i2c@3 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <3>;
+		};
+
+		i2c3mux0chn4: i2c@4 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <4>;
+		};
+
+		i2c3mux0chn5: i2c@5 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <5>;
+		};
+
+		i2c3mux0chn6: i2c@6 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <6>;
+		};
+
+		i2c3mux0chn7: i2c@7 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <7>;
+		};
+	};
+};
+
+&i2c4 {
+	status = "okay";
+};
+
+&i2c5 {
+	status = "okay";
+};
+
+&i2c6 {
+	status = "okay";
+
+	i2c-mux@70 {
+		compatible = "nxp,pca9548";
+		reg = <0x70>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		status = "okay";
+		i2c-mux-idle-disconnect;
+
+		i2c6mux0chn0: i2c@0 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0>;
+		};
+
+		i2c6mux0chn1: i2c@1 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <1>;
+		};
+
+		i2c6mux0chn2: i2c@2 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <2>;
+		};
+
+		i2c6mux0chn3: i2c@3 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <3>;
+		};
+
+		i2c6mux0chn4: i2c@4 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <4>;
+		};
+
+		i2c6mux0chn5: i2c@5 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <5>;
+		};
+
+		i2c6mux0chn6: i2c@6 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <6>;
+		};
+
+		i2c6mux0chn7: i2c@7 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <7>;
+		};
+	};
+};
+
+&i2c7 {
+	status = "okay";
+
+	i2c-mux@70 {
+		compatible = "nxp,pca9548";
+		reg = <0x70>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		status = "okay";
+		i2c-mux-idle-disconnect;
+
+		i2c7mux0chn0: i2c@0 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0>;
+		};
+
+		i2c7mux0chn1: i2c@1 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <1>;
+		};
+
+		i2c7mux0chn2: i2c@2 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <2>;
+		};
+
+		i2c7mux0chn3: i2c@3 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <3>;
+		};
+
+		i2c7mux0chn4: i2c@4 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <4>;
+		};
+
+		i2c7mux0chn5: i2c@5 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <5>;
+		};
+
+		i2c7mux0chn6: i2c@6 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <6>;
+		};
+
+		i2c7mux0chn7: i2c@7 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <7>;
+		};
+	};
+};
+
+&i2c8 {
+	status = "okay";
+
+	i2c-mux@71 {
+		compatible = "nxp,pca9548";
+		reg = <0x71>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		status = "okay";
+		i2c-mux-idle-disconnect;
+
+		i2c8mux0chn0: i2c@0 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0>;
+		};
+
+		i2c8mux0chn1: i2c@1 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <1>;
+		};
+
+		i2c8mux0chn2: i2c@2 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <2>;
+		};
+
+		i2c8mux0chn3: i2c@3 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <3>;
+		};
+
+		i2c8mux0chn4: i2c@4 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <4>;
+
+			i2c-mux@70 {
+				compatible = "nxp,pca9548";
+				reg = <0x70>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "okay";
+				i2c-mux-idle-disconnect;
+
+				i2c8mux1chn0: i2c@0 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <0>;
+				};
+
+				i2c8mux1chn1: i2c@1 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <1>;
+				};
+
+				i2c8mux1chn2: i2c@2 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <2>;
+				};
+
+				i2c8mux1chn3: i2c@3 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <3>;
+				};
+
+				i2c8mux1chn4: i2c@4 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <4>;
+				};
+
+				i2c8mux1chn5: i2c@5 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <5>;
+				};
+
+				i2c8mux1chn6: i2c@6 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <6>;
+				};
+
+				i2c8mux1chn7: i2c@7 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <7>;
+				};
+			};
+		};
+
+		i2c8mux0chn5: i2c@5 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <5>;
+		};
+
+		i2c8mux0chn6: i2c@6 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <6>;
+		};
+
+		i2c8mux0chn7: i2c@7 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <7>;
+		};
+	};
+};
+
+&i2c9 {
+	status = "okay";
+};
+
+&i2c11 {
+	status = "okay";
+};
+
+&i2c12 {
+	status = "okay";
+};
+
+&i2c13 {
+	status = "okay";
+};
+
+&i2c14 {
+	status = "okay";
+
+	i2c-mux@70 {
+		compatible = "nxp,pca9548";
+		reg = <0x70>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		status = "okay";
+		i2c-mux-idle-disconnect;
+
+		i2c14mux0chn0: i2c@0 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0>;
+		};
+
+		i2c14mux0chn1: i2c@1 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <1>;
+		};
+
+		i2c14mux0chn2: i2c@2 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <2>;
+		};
+
+		i2c14mux0chn3: i2c@3 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <3>;
+		};
+
+		i2c14mux0chn4: i2c@4 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <4>;
+		};
+
+		i2c14mux0chn5: i2c@5 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <5>;
+		};
+
+		i2c14mux0chn6: i2c@6 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <6>;
+		};
+
+		i2c14mux0chn7: i2c@7 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <7>;
+		};
+	};
+};
+
+&i2c15 {
+	status = "okay";
+
+	i2c-mux@71 {
+		compatible = "nxp,pca9548";
+		reg = <0x71>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		status = "okay";
+		i2c-mux-idle-disconnect;
+
+		i2c15mux0chn0: i2c@0 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0>;
+		};
+
+		i2c15mux0chn1: i2c@1 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <1>;
+		};
+
+		i2c15mux0chn2: i2c@2 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <2>;
+		};
+
+		i2c15mux0chn3: i2c@3 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <3>;
+		};
+
+		i2c15mux0chn4: i2c@4 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <4>;
+
+			i2c-mux@70 {
+				compatible = "nxp,pca9548";
+				reg = <0x70>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "okay";
+				i2c-mux-idle-disconnect;
+
+				i2c15mux1chn0: i2c@0 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <0>;
+				};
+
+				i2c15mux1chn1: i2c@1 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <1>;
+				};
+
+				i2c15mux1chn2: i2c@2 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <2>;
+				};
+
+				i2c15mux1chn3: i2c@3 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <3>;
+				};
+
+				i2c15mux1chn4: i2c@4 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <4>;
+				};
+
+				i2c15mux1chn5: i2c@5 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <5>;
+				};
+
+				i2c15mux1chn6: i2c@6 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <6>;
+				};
+
+				i2c15mux1chn7: i2c@7 {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					reg = <7>;
+				};
+			};
+		};
+
+		i2c15mux0chn5: i2c@5 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <5>;
+		};
+
+		i2c15mux0chn6: i2c@6 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <6>;
+		};
+
+		i2c15mux0chn7: i2c@7 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <7>;
+		};
+	};
+};
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 4/8] ARM: dts: aspeed: System1: Add i2c and muxes

[Krzysztof Kozlowski]

On 12/12/2023 17:40, Ninad Palsule wrote:
> This commit adds i2c devices and muxes.
> 
> Tested:
>     This board is tested using the simics simulator.
> 
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
>  .../dts/aspeed/aspeed-bmc-ibm-system1.dts     | 577 ++++++++++++++++++
>  1 file changed, 577 insertions(+)
> 

Why do you split new submission into multiple chunks? The logical
feature here is new board, so all your DTS patches adding new board
should be in one patch.

> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
> index 01291b407f59..0557bff9f36a 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
> @@ -11,6 +11,65 @@ / {
>  	model = "System1";
>  	compatible = "ibm,system1-bmc", "aspeed,ast2600";
>  
> +	aliases {
> +		i2c16 = &i2c8mux1chn0;
> +		i2c17 = &i2c8mux1chn1;
> +		i2c18 = &i2c8mux1chn2;
> +		i2c19 = &i2c8mux1chn3;
> +		i2c20 = &i2c8mux1chn4;
> +		i2c21 = &i2c8mux1chn5;
> +		i2c22 = &i2c8mux1chn6;
> +		i2c23 = &i2c8mux1chn7;
> +		i2c24 = &i2c3mux0chn0;
> +		i2c25 = &i2c3mux0chn1;
> +		i2c26 = &i2c3mux0chn2;
> +		i2c27 = &i2c3mux0chn3;
> +		i2c28 = &i2c3mux0chn4;
> +		i2c29 = &i2c3mux0chn5;
> +		i2c30 = &i2c3mux0chn6;
> +		i2c31 = &i2c3mux0chn7;
> +		i2c32 = &i2c6mux0chn0;
> +		i2c33 = &i2c6mux0chn1;
> +		i2c34 = &i2c6mux0chn2;
> +		i2c35 = &i2c6mux0chn3;
> +		i2c36 = &i2c6mux0chn4;
> +		i2c37 = &i2c6mux0chn5;
> +		i2c38 = &i2c6mux0chn6;
> +		i2c39 = &i2c6mux0chn7;
> +		i2c40 = &i2c7mux0chn0;
> +		i2c41 = &i2c7mux0chn1;
> +		i2c42 = &i2c7mux0chn2;
> +		i2c43 = &i2c7mux0chn3;
> +		i2c44 = &i2c7mux0chn4;
> +		i2c45 = &i2c7mux0chn5;
> +		i2c46 = &i2c7mux0chn6;
> +		i2c47 = &i2c7mux0chn7;
> +		i2c48 = &i2c8mux0chn0;
> +		i2c49 = &i2c8mux0chn1;
> +		i2c50 = &i2c8mux0chn2;
> +		i2c51 = &i2c8mux0chn3;
> +		i2c52 = &i2c8mux0chn4;
> +		i2c53 = &i2c8mux0chn5;
> +		i2c54 = &i2c8mux0chn6;
> +		i2c55 = &i2c8mux0chn7;
> +		i2c56 = &i2c14mux0chn0;
> +		i2c57 = &i2c14mux0chn1;
> +		i2c58 = &i2c14mux0chn2;
> +		i2c59 = &i2c14mux0chn3;
> +		i2c60 = &i2c14mux0chn4;
> +		i2c61 = &i2c14mux0chn5;
> +		i2c62 = &i2c14mux0chn6;
> +		i2c63 = &i2c14mux0chn7;
> +		i2c64 = &i2c15mux0chn0;
> +		i2c65 = &i2c15mux0chn1;
> +		i2c66 = &i2c15mux0chn2;
> +		i2c67 = &i2c15mux0chn3;
> +		i2c68 = &i2c15mux0chn4;
> +		i2c69 = &i2c15mux0chn5;
> +		i2c70 = &i2c15mux0chn6;
> +		i2c71 = &i2c15mux0chn7;
> +	};
> +
>  	chosen {
>  		stdout-path = &uart5;
>  		bootargs = "console=ttyS4,115200n8 earlycon";
> @@ -54,10 +113,12 @@ vga_memory: region@bf000000 {
>  			reg = <0xbf000000 0x01000000>;  /* 16M */
>  		};
>  	};
> +

You need to clean your patchset...


>  };
>  
>  &adc1 {
>  	status = "okay";
> +

Really.

>  	aspeed,int-vref-microvolt = <2500000>;
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_adc8_default
> @@ -186,3 +247,519 @@ &kcs3 {
>  	aspeed,lpc-io-reg = <0xca2>;
>  	aspeed,lpc-interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
>  };
> +
> +&i2c0 {
> +	status = "okay";
> +};
> +
> +&i2c1 {
> +	status = "okay";
> +};
> +
> +&i2c2 {
> +	status = "okay";
> +};
> +
> +&i2c3 {
> +	status = "okay";
> +
> +	i2c-mux@70 {
> +		compatible = "nxp,pca9548";
> +		reg = <0x70>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		status = "okay";

Drop

> +		i2c-mux-idle-disconnect;
> +
> +		i2c3mux0chn0: i2c@0 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <0>;
> +		};
> +
> +		i2c3mux0chn1: i2c@1 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <1>;
> +		};
> +
> +		i2c3mux0chn2: i2c@2 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <2>;
> +		};
> +
> +		i2c3mux0chn3: i2c@3 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <3>;
> +		};
> +
> +		i2c3mux0chn4: i2c@4 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <4>;
> +		};
> +
> +		i2c3mux0chn5: i2c@5 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <5>;
> +		};
> +
> +		i2c3mux0chn6: i2c@6 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <6>;
> +		};
> +
> +		i2c3mux0chn7: i2c@7 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <7>;
> +		};
> +	};
> +};
> +
> +&i2c4 {
> +	status = "okay";
> +};
> +
> +&i2c5 {
> +	status = "okay";
> +};
> +
> +&i2c6 {
> +	status = "okay";
> +
> +	i2c-mux@70 {
> +		compatible = "nxp,pca9548";
> +		reg = <0x70>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		status = "okay";

Drop

Other places as well.


Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 4/8] ARM: dts: aspeed: System1: Add i2c and muxes

[Ninad Palsule]

Hello Krzysztof,

On 12/12/23 14:21, Krzysztof Kozlowski wrote:
> On 12/12/2023 17:40, Ninad Palsule wrote:
>> This commit adds i2c devices and muxes.
>>
>> Tested:
>>      This board is tested using the simics simulator.
>>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>>   .../dts/aspeed/aspeed-bmc-ibm-system1.dts     | 577 ++++++++++++++++++
>>   1 file changed, 577 insertions(+)
>>
> Why do you split new submission into multiple chunks? The logical
> feature here is new board, so all your DTS patches adding new board
> should be in one patch.
Oh,ok. I thought it might be easier to review but now I squashed them 
together.
>
>> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
>> index 01291b407f59..0557bff9f36a 100644
>> --- a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
>> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
>> @@ -11,6 +11,65 @@ / {
>>   	model = "System1";
>>   	compatible = "ibm,system1-bmc", "aspeed,ast2600";
>>   
>> +	aliases {
>> +		i2c16 = &i2c8mux1chn0;
>> +		i2c17 = &i2c8mux1chn1;
>> +		i2c18 = &i2c8mux1chn2;
>> +		i2c19 = &i2c8mux1chn3;
>> +		i2c20 = &i2c8mux1chn4;
>> +		i2c21 = &i2c8mux1chn5;
>> +		i2c22 = &i2c8mux1chn6;
>> +		i2c23 = &i2c8mux1chn7;
>> +		i2c24 = &i2c3mux0chn0;
>> +		i2c25 = &i2c3mux0chn1;
>> +		i2c26 = &i2c3mux0chn2;
>> +		i2c27 = &i2c3mux0chn3;
>> +		i2c28 = &i2c3mux0chn4;
>> +		i2c29 = &i2c3mux0chn5;
>> +		i2c30 = &i2c3mux0chn6;
>> +		i2c31 = &i2c3mux0chn7;
>> +		i2c32 = &i2c6mux0chn0;
>> +		i2c33 = &i2c6mux0chn1;
>> +		i2c34 = &i2c6mux0chn2;
>> +		i2c35 = &i2c6mux0chn3;
>> +		i2c36 = &i2c6mux0chn4;
>> +		i2c37 = &i2c6mux0chn5;
>> +		i2c38 = &i2c6mux0chn6;
>> +		i2c39 = &i2c6mux0chn7;
>> +		i2c40 = &i2c7mux0chn0;
>> +		i2c41 = &i2c7mux0chn1;
>> +		i2c42 = &i2c7mux0chn2;
>> +		i2c43 = &i2c7mux0chn3;
>> +		i2c44 = &i2c7mux0chn4;
>> +		i2c45 = &i2c7mux0chn5;
>> +		i2c46 = &i2c7mux0chn6;
>> +		i2c47 = &i2c7mux0chn7;
>> +		i2c48 = &i2c8mux0chn0;
>> +		i2c49 = &i2c8mux0chn1;
>> +		i2c50 = &i2c8mux0chn2;
>> +		i2c51 = &i2c8mux0chn3;
>> +		i2c52 = &i2c8mux0chn4;
>> +		i2c53 = &i2c8mux0chn5;
>> +		i2c54 = &i2c8mux0chn6;
>> +		i2c55 = &i2c8mux0chn7;
>> +		i2c56 = &i2c14mux0chn0;
>> +		i2c57 = &i2c14mux0chn1;
>> +		i2c58 = &i2c14mux0chn2;
>> +		i2c59 = &i2c14mux0chn3;
>> +		i2c60 = &i2c14mux0chn4;
>> +		i2c61 = &i2c14mux0chn5;
>> +		i2c62 = &i2c14mux0chn6;
>> +		i2c63 = &i2c14mux0chn7;
>> +		i2c64 = &i2c15mux0chn0;
>> +		i2c65 = &i2c15mux0chn1;
>> +		i2c66 = &i2c15mux0chn2;
>> +		i2c67 = &i2c15mux0chn3;
>> +		i2c68 = &i2c15mux0chn4;
>> +		i2c69 = &i2c15mux0chn5;
>> +		i2c70 = &i2c15mux0chn6;
>> +		i2c71 = &i2c15mux0chn7;
>> +	};
>> +
>>   	chosen {
>>   		stdout-path = &uart5;
>>   		bootargs = "console=ttyS4,115200n8 earlycon";
>> @@ -54,10 +113,12 @@ vga_memory: region@bf000000 {
>>   			reg = <0xbf000000 0x01000000>;  /* 16M */
>>   		};
>>   	};
>> +
> You need to clean your patchset...
I made changes about the stdout-path, is that what you mean?
>
>
>>   };
>>   
>>   &adc1 {
>>   	status = "okay";
>> +
> Really.
I need to add "status = okay" otherwise its not instantiated.
>
>>   	aspeed,int-vref-microvolt = <2500000>;
>>   	pinctrl-names = "default";
>>   	pinctrl-0 = <&pinctrl_adc8_default
>> @@ -186,3 +247,519 @@ &kcs3 {
>>   	aspeed,lpc-io-reg = <0xca2>;
>>   	aspeed,lpc-interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
>>   };
>> +
>> +&i2c0 {
>> +	status = "okay";
>> +};
>> +
>> +&i2c1 {
>> +	status = "okay";
>> +};
>> +
>> +&i2c2 {
>> +	status = "okay";
>> +};
>> +
>> +&i2c3 {
>> +	status = "okay";
>> +
>> +	i2c-mux@70 {
>> +		compatible = "nxp,pca9548";
>> +		reg = <0x70>;
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		status = "okay";
> Drop
Done.
>
>> +		i2c-mux-idle-disconnect;
>> +
>> +		i2c3mux0chn0: i2c@0 {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +			reg = <0>;
>> +		};
>> +
>> +		i2c3mux0chn1: i2c@1 {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +			reg = <1>;
>> +		};
>> +
>> +		i2c3mux0chn2: i2c@2 {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +			reg = <2>;
>> +		};
>> +
>> +		i2c3mux0chn3: i2c@3 {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +			reg = <3>;
>> +		};
>> +
>> +		i2c3mux0chn4: i2c@4 {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +			reg = <4>;
>> +		};
>> +
>> +		i2c3mux0chn5: i2c@5 {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +			reg = <5>;
>> +		};
>> +
>> +		i2c3mux0chn6: i2c@6 {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +			reg = <6>;
>> +		};
>> +
>> +		i2c3mux0chn7: i2c@7 {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +			reg = <7>;
>> +		};
>> +	};
>> +};
>> +
>> +&i2c4 {
>> +	status = "okay";
>> +};
>> +
>> +&i2c5 {
>> +	status = "okay";
>> +};
>> +
>> +&i2c6 {
>> +	status = "okay";
>> +
>> +	i2c-mux@70 {
>> +		compatible = "nxp,pca9548";
>> +		reg = <0x70>;
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		status = "okay";
> Drop

Done

Thanks for the review.

Regards,

Ninad

>
> Other places as well.
>
>
> Best regards,
> Krzysztof
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 4/8] ARM: dts: aspeed: System1: Add i2c and muxes

[Krzysztof Kozlowski]

On 14/12/2023 19:34, Ninad Palsule wrote:
>>>   	chosen {
>>>   		stdout-path = &uart5;
>>>   		bootargs = "console=ttyS4,115200n8 earlycon";
>>> @@ -54,10 +113,12 @@ vga_memory: region@bf000000 {
>>>   			reg = <0xbf000000 0x01000000>;  /* 16M */
>>>   		};
>>>   	};
>>> +
>> You need to clean your patchset...
> I made changes about the stdout-path, is that what you mean?

No, I meant you made irrelevant changes in this patch.

>>
>>
>>>   };
>>>   
>>>   &adc1 {
>>>   	status = "okay";
>>> +
>> Really.
> I need to add "status = okay" otherwise its not instantiated.

That was about blank line. How is it related to this patch "Add i2c and
muxes"?

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 5/8] ARM: dts: aspeed: System1: Voltage regulators

[Ninad Palsule]

This commit adds different voltage regulators.

Tested:
    This board is tested using the simics simulator.

Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
 .../dts/aspeed/aspeed-bmc-ibm-system1.dts     | 266 ++++++++++++++++++
 1 file changed, 266 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
index 0557bff9f36a..b8e7e52d4600 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
+++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
@@ -114,10 +114,127 @@ vga_memory: region@bf000000 {
 		};
 	};
 
+	iio-hwmon {
+		compatible = "iio-hwmon";
+		io-channels = <&p12v_vd 0>, <&p5v_aux_vd 0>,
+			<&p5v_bmc_aux_vd 0>, <&p3v3_aux_vd 0>,
+			<&p3v3_bmc_aux_vd 0>, <&p1v8_bmc_aux_vd 0>,
+			<&adc1 4>, <&adc0 2>, <&adc1 0>,
+			<&p2V5_aux_vd 0>, <&adc1 7>;
+	};
+
+	p12v_vd: voltage_divider1 {
+		compatible = "voltage-divider";
+		io-channels = <&adc1 3>;
+		#io-channel-cells = <1>;
+
+		/* Scale the system voltage by 1127/127 to fit the ADC range.
+		 * Use small nominator to prevent integer overflow.
+		 */
+		output-ohms = <15>;
+		full-ohms = <133>;
+	};
+
+	p5v_aux_vd: voltage_divider2 {
+		compatible = "voltage-divider";
+		io-channels = <&adc1 5>;
+		#io-channel-cells = <1>;
+
+		/* Scale the system voltage by 1365/365 to fit the ADC range.
+		 * Use small nominator to prevent integer overflow.
+		 */
+		output-ohms = <50>;
+		full-ohms = <187>;
+	};
+
+	p5v_bmc_aux_vd: voltage_divider3 {
+		compatible = "voltage-divider";
+		io-channels = <&adc0 3>;
+		#io-channel-cells = <1>;
+
+		/* Scale the system voltage by 1365/365 to fit the ADC range.
+		 * Use small nominator to prevent integer overflow.
+		 */
+		output-ohms = <50>;
+		full-ohms = <187>;
+	};
+
+	p3v3_aux_vd: voltage_divider4 {
+		compatible = "voltage-divider";
+		io-channels = <&adc1 2>;
+		#io-channel-cells = <1>;
+
+		/* Scale the system voltage by 1698/698 to fit the ADC range.
+		 * Use small nominator to prevent integer overflow.
+		 */
+		output-ohms = <14>;
+		full-ohms = <34>;
+	};
+
+	p3v3_bmc_aux_vd: voltage_divider5 {
+		compatible = "voltage-divider";
+		io-channels = <&adc0 7>;
+		#io-channel-cells = <1>;
+
+		/* Scale the system voltage by 1698/698 to fit the ADC range.
+		 * Use small nominator to prevent integer overflow.
+		 */
+		output-ohms = <14>;
+		full-ohms = <34>;
+	};
+
+	p1v8_bmc_aux_vd: voltage_divider6 {
+		compatible = "voltage-divider";
+		io-channels = <&adc0 6>;
+		#io-channel-cells = <1>;
+
+		/* Scale the system voltage by 4000/3000 to fit the ADC range.
+		 * Use small nominator to prevent integer overflow.
+		 */
+		output-ohms = <3>;
+		full-ohms = <4>;
+	};
+
+	p2V5_aux_vd: voltage_divider7 {
+		compatible = "voltage-divider";
+		io-channels = <&adc1 1>;
+		#io-channel-cells = <1>;
+
+		/* Scale the system voltage by 2100/1100 to fit the ADC range.
+		 * Use small nominator to prevent integer overflow.
+		 */
+		output-ohms = <11>;
+		full-ohms = <21>;
+	};
+
+	p1v8_bmc_aux: fixedregulator-p1v8_bmc_aux {
+		compatible = "regulator-fixed";
+		regulator-name = "p1v8_bmc_aux";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
+		regulator-always-on;
+	};
+};
+
+&adc0 {
+	status = "okay";
+	vref-supply = <&p1v8_bmc_aux>;
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_adc0_default
+		&pinctrl_adc1_default
+		&pinctrl_adc2_default
+		&pinctrl_adc3_default
+		&pinctrl_adc4_default
+		&pinctrl_adc5_default
+		&pinctrl_adc6_default
+		&pinctrl_adc7_default>;
 };
 
 &adc1 {
 	status = "okay";
+	vref-supply = <&p1v8_bmc_aux>;
+	aspeed,battery-sensing;
 
 	aspeed,int-vref-microvolt = <2500000>;
 	pinctrl-names = "default";
@@ -250,10 +367,32 @@ &kcs3 {
 
 &i2c0 {
 	status = "okay";
+
+	regulator@60 {
+		compatible = "maxim,max8952";
+		reg = <0x60>;
+
+		max8952,default-mode = <0>;
+		max8952,dvs-mode-microvolt = <1250000>, <1200000>,
+						<1050000>, <950000>;
+		max8952,sync-freq = <0>;
+		max8952,ramp-speed = <0>;
+
+		regulator-name = "VR_1.0V";
+		regulator-min-microvolt = <770000>;
+		regulator-max-microvolt = <1400000>;
+		regulator-always-on;
+		regulator-boot-on;
+	};
 };
 
 &i2c1 {
 	status = "okay";
+
+	regulator@42 {
+		compatible = "infineon,ir38263";
+		reg = <0x42>;
+	};
 };
 
 &i2c2 {
@@ -327,6 +466,21 @@ &i2c4 {
 
 &i2c5 {
 	status = "okay";
+
+	regulator@3a {
+		compatible = "maxim,max5970";
+		reg = <0x3a>;
+	};
+
+	regulator@42 {
+		compatible = "infineon,ir38263";
+		reg = <0x42>;
+	};
+
+	regulator@43 {
+		compatible = "infineon,ir38060";
+		reg = <0x43>;
+	};
 };
 
 &i2c6 {
@@ -423,6 +577,11 @@ i2c7mux0chn3: i2c@3 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <3>;
+
+			regulator@58 {
+				compatible = "mps,mp2973";
+				reg = <0x58>;
+			};
 		};
 
 		i2c7mux0chn4: i2c@4 {
@@ -435,6 +594,11 @@ i2c7mux0chn5: i2c@5 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <5>;
+
+			regulator@40 {
+				compatible = "infineon,tda38640";
+				reg = <0x40>;
+			};
 		};
 
 		i2c7mux0chn6: i2c@6 {
@@ -466,12 +630,37 @@ i2c8mux0chn0: i2c@0 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0>;
+
+			regulator@58 {
+				compatible = "mps,mp2971";
+				reg = <0x58>;
+			};
 		};
 
 		i2c8mux0chn1: i2c@1 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <1>;
+
+			regulator@40 {
+				compatible = "infineon,tda38640";
+				reg = <0x40>;
+			};
+
+			regulator@41 {
+				compatible = "infineon,tda38640";
+				reg = <0x41>;
+			};
+
+			regulator@58 {
+				compatible = "mps,mp2971";
+				reg = <0x58>;
+			};
+
+			regulator@5b {
+				compatible = "mps,mp2971";
+				reg = <0x5b>;
+			};
 		};
 
 		i2c8mux0chn2: i2c@2 {
@@ -565,12 +754,44 @@ i2c8mux0chn7: i2c@7 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <7>;
+
+			regulator@40 {
+				compatible = "infineon,ir38060";
+				reg = <0x40>;
+			};
 		};
 	};
 };
 
 &i2c9 {
 	status = "okay";
+
+	regulator@40 {
+		compatible = "infineon,ir38263";
+		reg = <0x40>;
+	};
+
+	regulator@41 {
+		compatible = "infineon,ir38263";
+		reg = <0x41>;
+	};
+
+	regulator@60 {
+		compatible = "maxim,max8952";
+		reg = <0x60>;
+
+		max8952,default-mode = <0>;
+		max8952,dvs-mode-microvolt = <1250000>, <1200000>,
+						<1050000>, <950000>;
+		max8952,sync-freq = <0>;
+		max8952,ramp-speed = <0>;
+
+		regulator-name = "VR_1.2V";
+		regulator-min-microvolt = <770000>;
+		regulator-max-microvolt = <1400000>;
+		regulator-always-on;
+		regulator-boot-on;
+	};
 };
 
 &i2c11 {
@@ -583,6 +804,11 @@ &i2c12 {
 
 &i2c13 {
 	status = "okay";
+
+	regulator@41 {
+		compatible = "infineon,ir38263";
+		reg = <0x41>;
+	};
 };
 
 &i2c14 {
@@ -618,6 +844,11 @@ i2c14mux0chn3: i2c@3 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <3>;
+
+			regulator@58 {
+				compatible = "mps,mp2973";
+				reg = <0x58>;
+			};
 		};
 
 		i2c14mux0chn4: i2c@4 {
@@ -630,6 +861,11 @@ i2c14mux0chn5: i2c@5 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <5>;
+
+			regulator@40 {
+				compatible = "infineon,tda38640";
+				reg = <0x40>;
+			};
 		};
 
 		i2c14mux0chn6: i2c@6 {
@@ -661,12 +897,37 @@ i2c15mux0chn0: i2c@0 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0>;
+
+			regulator@58 {
+				compatible = "mps,mp2971";
+				reg = <0x58>;
+			};
 		};
 
 		i2c15mux0chn1: i2c@1 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <1>;
+
+			regulator@40 {
+				compatible = "infineon,tda38640";
+				reg = <0x40>;
+			};
+
+			regulator@41 {
+				compatible = "infineon,tda38640";
+				reg = <0x41>;
+			};
+
+			regulator@58 {
+				compatible = "mps,mp2971";
+				reg = <0x58>;
+			};
+
+			regulator@5b {
+				compatible = "mps,mp2971";
+				reg = <0x5b>;
+			};
 		};
 
 		i2c15mux0chn2: i2c@2 {
@@ -760,6 +1021,11 @@ i2c15mux0chn7: i2c@7 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <7>;
+
+			regulator@40 {
+				compatible = "infineon,ir38060";
+				reg = <0x40>;
+			};
 		};
 	};
 };
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 5/8] ARM: dts: aspeed: System1: Voltage regulators

[Krzysztof Kozlowski]

On 12/12/2023 17:40, Ninad Palsule wrote:
> This commit adds different voltage regulators.
> 
> Tested:
>     This board is tested using the simics simulator.
> 
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
>  .../dts/aspeed/aspeed-bmc-ibm-system1.dts     | 266 ++++++++++++++++++

Squash it.

>  1 file changed, 266 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
> index 0557bff9f36a..b8e7e52d4600 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
> @@ -114,10 +114,127 @@ vga_memory: region@bf000000 {
>  		};
>  	};
>  
> +	iio-hwmon {
> +		compatible = "iio-hwmon";
> +		io-channels = <&p12v_vd 0>, <&p5v_aux_vd 0>,
> +			<&p5v_bmc_aux_vd 0>, <&p3v3_aux_vd 0>,
> +			<&p3v3_bmc_aux_vd 0>, <&p1v8_bmc_aux_vd 0>,
> +			<&adc1 4>, <&adc0 2>, <&adc1 0>,
> +			<&p2V5_aux_vd 0>, <&adc1 7>;
> +	};
> +
> +	p12v_vd: voltage_divider1 {

Same comments as in other patches.

> +		compatible = "voltage-divider";
> +		io-channels = <&adc1 3>;
> +		#io-channel-cells = <1>;
> +
> +		/* Scale the system voltage by 1127/127 to fit the ADC range.

Use Linux coding style comments. /* and blank line


....

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 5/8] ARM: dts: aspeed: System1: Voltage regulators

[Ninad Palsule]

Hello Krzysztof,

On 12/12/23 14:22, Krzysztof Kozlowski wrote:
> On 12/12/2023 17:40, Ninad Palsule wrote:
>> This commit adds different voltage regulators.
>>
>> Tested:
>>      This board is tested using the simics simulator.
>>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>>   .../dts/aspeed/aspeed-bmc-ibm-system1.dts     | 266 ++++++++++++++++++
> Squash it.
Done.
>
>>   1 file changed, 266 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
>> index 0557bff9f36a..b8e7e52d4600 100644
>> --- a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
>> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
>> @@ -114,10 +114,127 @@ vga_memory: region@bf000000 {
>>   		};
>>   	};
>>   
>> +	iio-hwmon {
>> +		compatible = "iio-hwmon";
>> +		io-channels = <&p12v_vd 0>, <&p5v_aux_vd 0>,
>> +			<&p5v_bmc_aux_vd 0>, <&p3v3_aux_vd 0>,
>> +			<&p3v3_bmc_aux_vd 0>, <&p1v8_bmc_aux_vd 0>,
>> +			<&adc1 4>, <&adc0 2>, <&adc1 0>,
>> +			<&p2V5_aux_vd 0>, <&adc1 7>;
>> +	};
>> +
>> +	p12v_vd: voltage_divider1 {
> Same comments as in other patches.
Fixed.
>
>> +		compatible = "voltage-divider";
>> +		io-channels = <&adc1 3>;
>> +		#io-channel-cells = <1>;
>> +
>> +		/* Scale the system voltage by 1127/127 to fit the ADC range.
> Use Linux coding style comments. /* and blank line

Fixed.

Thanks for the review.

Regards,

Ninad

>
>
> ....
>
> Best regards,
> Krzysztof
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 6/8] ARM: dts: aspeed: System1: GPIO, Fan ctrl, Led

[Ninad Palsule]

This commit adds following devices to the device tree.
- GPIO pin assignements, GPIO expansion devices
- LED brinker devices
- Fan controllers

Tested:
    This board is tested using the simics simulator.

Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
 .../dts/aspeed/aspeed-bmc-ibm-system1.dts     | 547 +++++++++++++++++-
 1 file changed, 542 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
index b8e7e52d4600..75562aa63701 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
+++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
@@ -114,6 +114,99 @@ vga_memory: region@bf000000 {
 		};
 	};
 
+	leds {
+		compatible = "gpio-leds";
+
+		bmc-ready {
+			gpios = <&gpio0 ASPEED_GPIO(L, 7) GPIO_ACTIVE_HIGH>;
+		};
+
+		bmc-hb {
+			gpios = <&gpio0 ASPEED_GPIO(P, 7) GPIO_ACTIVE_HIGH>;
+		};
+
+		rear-enc-fault0 {
+			gpios = <&gpio0 ASPEED_GPIO(S, 6) GPIO_ACTIVE_HIGH>;
+		};
+
+		rear-enc-id0 {
+			gpios = <&gpio0 ASPEED_GPIO(S, 7) GPIO_ACTIVE_HIGH>;
+		};
+
+		fan0-fault-led {
+			gpios = <&pca3 5 GPIO_ACTIVE_LOW>;
+		};
+
+		fan1-fault-led {
+			gpios = <&pca3 6 GPIO_ACTIVE_LOW>;
+		};
+
+		fan2-fault-led {
+			gpios = <&pca3 7 GPIO_ACTIVE_LOW>;
+		};
+
+		fan3-fault-led {
+			gpios = <&pca3 8 GPIO_ACTIVE_LOW>;
+		};
+
+		fan4-fault-led {
+			gpios = <&pca3 9 GPIO_ACTIVE_LOW>;
+		};
+
+		fan5-fault-led {
+			gpios = <&pca3 10 GPIO_ACTIVE_LOW>;
+		};
+
+		fan6-fault-led {
+			gpios = <&pca3 11 GPIO_ACTIVE_LOW>;
+		};
+
+		nvmed0-fault-led {
+			gpios = <&pca4 4 GPIO_ACTIVE_HIGH>;
+		};
+
+		nvmed1-fault-led {
+			gpios = <&pca4 5 GPIO_ACTIVE_HIGH>;
+		};
+
+		nvmed2-fault-led {
+			gpios = <&pca4 6 GPIO_ACTIVE_HIGH>;
+		};
+
+		nvmed3-fault-led {
+			gpios = <&pca4 7 GPIO_ACTIVE_HIGH>;
+		};
+	};
+
+	gpio-keys-polled {
+		compatible = "gpio-keys-polled";
+		poll-interval = <1000>;
+
+		event-nvme0-presence {
+			label = "nvme0-presence";
+			gpios = <&pca4 0 GPIO_ACTIVE_LOW>;
+			linux,code = <0>;
+		};
+
+		event-nvme1-presence {
+			label = "nvme1-presence";
+			gpios = <&pca4 1 GPIO_ACTIVE_LOW>;
+			linux,code = <1>;
+		};
+
+		event-nvme2-presence {
+			label = "nvme2-presence";
+			gpios = <&pca4 2 GPIO_ACTIVE_LOW>;
+			linux,code = <2>;
+		};
+
+		event-nvme3-presence {
+			label = "nvme3-presence";
+			gpios = <&pca4 3 GPIO_ACTIVE_LOW>;
+			linux,code = <3>;
+		};
+	};
+
 	iio-hwmon {
 		compatible = "iio-hwmon";
 		io-channels = <&p12v_vd 0>, <&p5v_aux_vd 0>,
@@ -259,7 +352,7 @@ &uhci {
 &gpio0 {
 	gpio-line-names =
 	/*A0-A7*/	"","","","","","","","",
-	/*B0-B7*/	"","","","","","","","",
+	/*B0-B7*/	"","","","","bmc-tpm-reset","","","",
 	/*C0-C7*/	"","","","","","","","",
 	/*D0-D7*/	"","","","","","","","",
 	/*E0-E7*/	"","","","","","","","",
@@ -269,17 +362,17 @@ &gpio0 {
 	/*I0-I7*/	"","","","","","","","",
 	/*J0-J7*/	"","","","","","","","",
 	/*K0-K7*/	"","","","","","","","",
-	/*L0-L7*/	"","","","","","","","",
+	/*L0-L7*/	"","","","","","","","bmc-ready",
 	/*M0-M7*/	"","","","","","","","",
 	/*N0-N7*/	"","","","","","","","",
 	/*O0-O7*/	"","","","","","","","",
-	/*P0-P7*/	"","","","","","","","",
+	/*P0-P7*/	"","","","","","","","bmc-hb",
 	/*Q0-Q7*/	"","","","","","","","",
 	/*R0-R7*/	"","","","","","","","",
-	/*S0-S7*/	"","","","","","","","",
+	/*S0-S7*/	"","","","","","","rear-enc-fault0","rear-enc-id0",
 	/*T0-T7*/	"","","","","","","","",
 	/*U0-U7*/	"","","","","","","","",
-	/*V0-V7*/	"","","","power-chassis-control","","","","",
+	/*V0-V7*/	"","rtc-battery-voltage-read-enable","","power-chassis-control","","","","",
 	/*W0-W7*/	"","","","","","","","",
 	/*X0-X7*/	"","power-chassis-good","","","","","","",
 	/*Y0-Y7*/	"","","","","","","","",
@@ -393,6 +486,171 @@ regulator@42 {
 		compatible = "infineon,ir38263";
 		reg = <0x42>;
 	};
+
+	led-controller@60 {
+		compatible = "nxp,pca9552";
+		reg = <0x60>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		gpio-controller;
+		#gpio-cells = <2>;
+
+		led@0 {
+			label = "nic1-perst";
+			reg = <0>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@1 {
+			label = "bmc-perst";
+			reg = <1>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@2 {
+			label = "reset-M2-SSD1-2-perst";
+			reg = <2>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@3 {
+			label = "pcie-perst1";
+			reg = <3>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@4 {
+			label = "pcie-perst2";
+			reg = <4>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@5 {
+			label = "pcie-perst3";
+			reg = <5>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@6 {
+			label = "pcie-perst4";
+			reg = <6>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@7 {
+			label = "pcie-perst5";
+			reg = <7>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@8 {
+			label = "pcie-perst6";
+			reg = <8>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@9 {
+			label = "pcie-perst7";
+			reg = <9>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@10 {
+			label = "pcie-perst8";
+			reg = <10>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@11 {
+			label = "PV-cp0-sw1stk4-perst";
+			reg = <11>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@12 {
+			label = "PV-cp0-sw1stk5-perst";
+			reg = <12>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@13 {
+			label = "pe-cp-drv0-perst";
+			reg = <13>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@14 {
+			label = "pe-cp-drv1-perst";
+			reg = <14>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@15 {
+			label = "lom-perst";
+			reg = <15>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+	};
+
+	pca0: pca9539@74 {
+		compatible = "nxp,pca9539";
+		reg = <0x74>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		gpio-controller;
+		#gpio-cells = <2>;
+
+		gpio-line-names =
+			"PLUG_DETECT_PCIE_J101_N",
+			"PLUG_DETECT_PCIE_J102_N",
+			"PLUG_DETECT_PCIE_J103_N",
+			"PLUG_DETECT_PCIE_J104_N",
+			"PLUG_DETECT_PCIE_J105_N",
+			"PLUG_DETECT_PCIE_J106_N",
+			"PLUG_DETECT_PCIE_J107_N",
+			"PLUG_DETECT_PCIE_J108_N",
+			"PLUG_DETECT_M2_SSD1_N",
+			"PLUG_DETECT_NIC1_N",
+			"SEL_SMB_DIMM_CPU0",
+			"presence-ps2",
+			"presence-ps3",
+			"", "",
+			"PWRBRD_PLUG_DETECT2_N";
+	};
 };
 
 &i2c2 {
@@ -486,6 +744,20 @@ regulator@43 {
 &i2c6 {
 	status = "okay";
 
+	fan-controller@52 {
+		compatible = "maxim,max31785a";
+		reg = <0x52>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+	};
+
+	fan-controller@54 {
+		compatible = "maxim,max31785a";
+		reg = <0x54>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+	};
+
 	i2c-mux@70 {
 		compatible = "nxp,pca9548";
 		reg = <0x70>;
@@ -522,6 +794,48 @@ i2c6mux0chn4: i2c@4 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <4>;
+
+			led-controller@60 {
+				compatible = "nxp,pca9551";
+				reg = <0x60>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				gpio-controller;
+				#gpio-cells = <2>;
+
+				led@0 {
+					label = "enclosure-id-led";
+					reg = <0>;
+					retain-state-shutdown;
+					default-state = "keep";
+					type = <PCA955X_TYPE_LED>;
+				};
+
+				led@1 {
+					label = "attention-led";
+					reg = <1>;
+					retain-state-shutdown;
+					default-state = "keep";
+					type = <PCA955X_TYPE_LED>;
+				};
+
+				led@2 {
+					label = "enclosure-fault-rollup-led";
+					reg = <2>;
+					retain-state-shutdown;
+					default-state = "keep";
+					type = <PCA955X_TYPE_LED>;
+				};
+
+				led@3 {
+					label = "power-on-led";
+					reg = <3>;
+					retain-state-shutdown;
+					default-state = "keep";
+					type = <PCA955X_TYPE_LED>;
+				};
+			};
 		};
 
 		i2c6mux0chn5: i2c@5 {
@@ -542,6 +856,44 @@ i2c6mux0chn7: i2c@7 {
 			reg = <7>;
 		};
 	};
+
+	pca3: pca9539@74 {
+		compatible = "nxp,pca9539";
+		reg = <0x74>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		gpio-controller;
+		#gpio-cells = <2>;
+	};
+
+	pca4: pca9539@77 {
+		compatible = "nxp,pca9539";
+		reg = <0x77>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		gpio-controller;
+		#gpio-cells = <2>;
+
+		gpio-line-names =
+			"PE_NVMED0_EXP_PRSNT_N",
+			"PE_NVMED1_EXP_PRSNT_N",
+			"PE_NVMED2_EXP_PRSNT_N",
+			"PE_NVMED3_EXP_PRSNT_N",
+			"LED_FAULT_NVMED0",
+			"LED_FAULT_NVMED1",
+			"LED_FAULT_NVMED2",
+			"LED_FAULT_NVMED3",
+			"FAN0_PRESENCE_R_N",
+			"FAN1_PRESENCE_R_N",
+			"FAN2_PRESENCE_R_N",
+			"FAN3_PRESENCE_R_N",
+			"FAN4_PRESENCE_R_N",
+			"FAN5_PRESENCE_N",
+			"FAN6_PRESENCE_N",
+			"";
+	};
 };
 
 &i2c7 {
@@ -809,6 +1161,191 @@ regulator@41 {
 		compatible = "infineon,ir38263";
 		reg = <0x41>;
 	};
+
+	led-controller@61 {
+		compatible = "nxp,pca9552";
+		reg = <0x61>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		gpio-controller;
+		#gpio-cells = <2>;
+
+		led@0 {
+			label = "efuse-12v-slots";
+			reg = <0>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@1 {
+			label = "efuse-3p3v-slot";
+			reg = <1>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@3 {
+			label = "nic2-pert";
+			reg = <3>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@4 {
+			label = "pcie-perst9";
+			reg = <4>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@5 {
+			label = "pcie-perst10";
+			reg = <5>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@6 {
+			label = "pcie-perst11";
+			reg = <6>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@7 {
+			label = "pcie-perst12";
+			reg = <7>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@8 {
+			label = "pcie-perst13";
+			reg = <8>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@9 {
+			label = "pcie-perst14";
+			reg = <9>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@10 {
+			label = "pcie-perst15";
+			reg = <10>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@11 {
+			label = "pcie-perst16";
+			reg = <11>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@12 {
+			label = "PV-cp1-sw1stk4-perst";
+			reg = <12>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@13 {
+			label = "PV-cp1-sw1stk5-perst";
+			reg = <13>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@14 {
+			label = "pe-cp-drv2-perst";
+			reg = <14>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+
+		led@15 {
+			label = "pe-cp-drv3-perst";
+			reg = <15>;
+			retain-state-shutdown;
+			default-state = "keep";
+			type = <PCA955X_TYPE_LED>;
+		};
+	};
+
+	pca1: pca9539@75 {
+		compatible = "nxp,pca9539";
+		reg = <0x75>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		gpio-controller;
+		#gpio-cells = <2>;
+
+		gpio-line-names =
+			"PLUG_DETECT_PCIE_J109_N",
+			"PLUG_DETECT_PCIE_J110_N",
+			"PLUG_DETECT_PCIE_J111_N",
+			"PLUG_DETECT_PCIE_J112_N",
+			"PLUG_DETECT_PCIE_J113_N",
+			"PLUG_DETECT_PCIE_J114_N",
+			"PLUG_DETECT_PCIE_J115_N",
+			"PLUG_DETECT_PCIE_J116_N",
+			"PLUG_DETECT_M2_SSD2_N",
+			"PLUG_DETECT_NIC2_N",
+			"SEL_SMB_DIMM_CPU1",
+			"presence-ps0",
+			"presence-ps1",
+			"", "",
+			"PWRBRD_PLUG_DETECT1_N";
+	};
+
+	pca2: pca9539@76 {
+		compatible = "nxp,pca9539";
+		reg = <0x76>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		gpio-controller;
+		#gpio-cells = <2>;
+
+		gpio-line-names =
+			"SW1_BOOTRCVRYB1_N",
+			"SW1_BOOTRCVRYB0_N",
+			"SW2_BOOTRCVRYB1_N",
+			"SW2_BOOTRCVRYB0_N",
+			"SW3_4_BOOTRCVRYB1_N",
+			"SW3_4_BOOTRCVRYB0_N",
+			"SW5_BOOTRCVRYB1_N",
+			"SW5_BOOTRCVRYB0_N",
+			"SW6_BOOTRCVRYB1_N",
+			"SW6_BOOTRCVRYB0_N",
+			"SW1_RESET_N",
+			"SW3_RESET_N",
+			"SW4_RESET_N",
+			"SW2_RESET_N",
+			"SW5_RESET_N",
+			"SW6_RESET_N";
+	};
 };
 
 &i2c14 {
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 6/8] ARM: dts: aspeed: System1: GPIO, Fan ctrl, Led

[Krzysztof Kozlowski]

On 12/12/2023 17:40, Ninad Palsule wrote:
> This commit adds following devices to the device tree.
> - GPIO pin assignements, GPIO expansion devices
> - LED brinker devices
> - Fan controllers
> 
> Tested:
>     This board is tested using the simics simulator.
> 
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
>  .../dts/aspeed/aspeed-bmc-ibm-system1.dts     | 547 +++++++++++++++++-

Squash it.

>  1 file changed, 542 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
> index b8e7e52d4600..75562aa63701 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
> @@ -114,6 +114,99 @@ vga_memory: region@bf000000 {
>  		};
>  	};
>  
> +	leds {
> +		compatible = "gpio-leds";
> +
> +		bmc-ready {

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

> +			gpios = <&gpio0 ASPEED_GPIO(L, 7) GPIO_ACTIVE_HIGH>;
> +		};
> +
> +		bmc-hb {

None of these were tested.


>  	/*A0-A7*/	"","","","","","","","",
> -	/*B0-B7*/	"","","","","","","","",
> +	/*B0-B7*/	"","","","","bmc-tpm-reset","","","",

Really? You just added these lines. There is no point in adding a new
line and immediately changing it.

This points how your split is artificial and not helpful.
...


>  &i2c2 {
> @@ -486,6 +744,20 @@ regulator@43 {
>  &i2c6 {
>  	status = "okay";
>  
> +	fan-controller@52 {
> +		compatible = "maxim,max31785a";
> +		reg = <0x52>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;

Why do you need cells?

> +	};
> +
> +	fan-controller@54 {
> +		compatible = "maxim,max31785a";
> +		reg = <0x54>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;

Why do you need cells?

> +	};
> +
>  	i2c-mux@70 {
>  		compatible = "nxp,pca9548";
>  		reg = <0x70>;
> @@ -522,6 +794,48 @@ i2c6mux0chn4: i2c@4 {
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  			reg = <4>;
> +
> +			led-controller@60 {
> +				compatible = "nxp,pca9551";
> +				reg = <0x60>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +
> +				led@0 {
> +					label = "enclosure-id-led";
> +					reg = <0>;
> +					retain-state-shutdown;
> +					default-state = "keep";
> +					type = <PCA955X_TYPE_LED>;
> +				};
> +
> +				led@1 {
> +					label = "attention-led";
> +					reg = <1>;
> +					retain-state-shutdown;
> +					default-state = "keep";
> +					type = <PCA955X_TYPE_LED>;
> +				};
> +
> +				led@2 {
> +					label = "enclosure-fault-rollup-led";
> +					reg = <2>;
> +					retain-state-shutdown;
> +					default-state = "keep";
> +					type = <PCA955X_TYPE_LED>;
> +				};
> +
> +				led@3 {
> +					label = "power-on-led";
> +					reg = <3>;
> +					retain-state-shutdown;
> +					default-state = "keep";
> +					type = <PCA955X_TYPE_LED>;
> +				};
> +			};
>  		};
>  
>  		i2c6mux0chn5: i2c@5 {
> @@ -542,6 +856,44 @@ i2c6mux0chn7: i2c@7 {
>  			reg = <7>;
>  		};
>  	};
> +
> +	pca3: pca9539@74 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +		compatible = "nxp,pca9539";
> +		reg = <0x74>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +	};
> +
> +	pca4: pca9539@77 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +		compatible = "nxp,pca9539";
> +		reg = <0x77>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +
> +		gpio-line-names =
> +			"PE_NVMED0_EXP_PRSNT_N",
> +			"PE_NVMED1_EXP_PRSNT_N",
> +			"PE_NVMED2_EXP_PRSNT_N",
> +			"PE_NVMED3_EXP_PRSNT_N",
> +			"LED_FAULT_NVMED0",
> +			"LED_FAULT_NVMED1",
> +			"LED_FAULT_NVMED2",
> +			"LED_FAULT_NVMED3",
> +			"FAN0_PRESENCE_R_N",
> +			"FAN1_PRESENCE_R_N",
> +			"FAN2_PRESENCE_R_N",
> +			"FAN3_PRESENCE_R_N",
> +			"FAN4_PRESENCE_R_N",
> +			"FAN5_PRESENCE_N",
> +			"FAN6_PRESENCE_N",
> +			"";
> +	};
>  };
>  
>  &i2c7 {
> @@ -809,6 +1161,191 @@ regulator@41 {
>  		compatible = "infineon,ir38263";
>  		reg = <0x41>;
>  	};
> +
> +	led-controller@61 {
> +		compatible = "nxp,pca9552";
> +		reg = <0x61>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +

...

> +		led@15 {
> +			label = "pe-cp-drv3-perst";
> +			reg = <15>;
> +			retain-state-shutdown;
> +			default-state = "keep";
> +			type = <PCA955X_TYPE_LED>;
> +		};
> +	};
> +
> +	pca1: pca9539@75 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation




Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 6/8] ARM: dts: aspeed: System1: GPIO, Fan ctrl, Led

[Ninad Palsule]

Hello Krzysztof,

On 12/12/23 14:25, Krzysztof Kozlowski wrote:
> On 12/12/2023 17:40, Ninad Palsule wrote:
>> This commit adds following devices to the device tree.
>> - GPIO pin assignements, GPIO expansion devices
>> - LED brinker devices
>> - Fan controllers
>>
>> Tested:
>>      This board is tested using the simics simulator.
>>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>>   .../dts/aspeed/aspeed-bmc-ibm-system1.dts     | 547 +++++++++++++++++-
> Squash it.

Yes, I made a single commit for device tree.

>
>>   1 file changed, 542 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
>> index b8e7e52d4600..75562aa63701 100644
>> --- a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
>> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
>> @@ -114,6 +114,99 @@ vga_memory: region@bf000000 {
>>   		};
>>   	};
>>   
>> +	leds {
>> +		compatible = "gpio-leds";
>> +
>> +		bmc-ready {
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check W=1` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).
Fixed names.
>
>> +			gpios = <&gpio0 ASPEED_GPIO(L, 7) GPIO_ACTIVE_HIGH>;
>> +		};
>> +
>> +		bmc-hb {
> None of these were tested.
Fixed names
>
>>   	/*A0-A7*/	"","","","","","","","",
>> -	/*B0-B7*/	"","","","","","","","",
>> +	/*B0-B7*/	"","","","","bmc-tpm-reset","","","",
> Really? You just added these lines. There is no point in adding a new
> line and immediately changing it.
>
> This points how your split is artificial and not helpful.
> ...
Now I have a single commit for device tree.
>
>
>>   &i2c2 {
>> @@ -486,6 +744,20 @@ regulator@43 {
>>   &i2c6 {
>>   	status = "okay";
>>   
>> +	fan-controller@52 {
>> +		compatible = "maxim,max31785a";
>> +		reg = <0x52>;
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
> Why do you need cells?
Removed cells.
>
>> +	};
>> +
>> +	fan-controller@54 {
>> +		compatible = "maxim,max31785a";
>> +		reg = <0x54>;
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
> Why do you need cells?
Removed cells.
>
>> +	};
>> +
>>   	i2c-mux@70 {
>>   		compatible = "nxp,pca9548";
>>   		reg = <0x70>;
>> @@ -522,6 +794,48 @@ i2c6mux0chn4: i2c@4 {
>>   			#address-cells = <1>;
>>   			#size-cells = <0>;
>>   			reg = <4>;
>> +
>> +			led-controller@60 {
>> +				compatible = "nxp,pca9551";
>> +				reg = <0x60>;
>> +				#address-cells = <1>;
>> +				#size-cells = <0>;
>> +
>> +				gpio-controller;
>> +				#gpio-cells = <2>;
>> +
>> +				led@0 {
>> +					label = "enclosure-id-led";
>> +					reg = <0>;
>> +					retain-state-shutdown;
>> +					default-state = "keep";
>> +					type = <PCA955X_TYPE_LED>;
>> +				};
>> +
>> +				led@1 {
>> +					label = "attention-led";
>> +					reg = <1>;
>> +					retain-state-shutdown;
>> +					default-state = "keep";
>> +					type = <PCA955X_TYPE_LED>;
>> +				};
>> +
>> +				led@2 {
>> +					label = "enclosure-fault-rollup-led";
>> +					reg = <2>;
>> +					retain-state-shutdown;
>> +					default-state = "keep";
>> +					type = <PCA955X_TYPE_LED>;
>> +				};
>> +
>> +				led@3 {
>> +					label = "power-on-led";
>> +					reg = <3>;
>> +					retain-state-shutdown;
>> +					default-state = "keep";
>> +					type = <PCA955X_TYPE_LED>;
>> +				};
>> +			};
>>   		};
>>   
>>   		i2c6mux0chn5: i2c@5 {
>> @@ -542,6 +856,44 @@ i2c6mux0chn7: i2c@7 {
>>   			reg = <7>;
>>   		};
>>   	};
>> +
>> +	pca3: pca9539@74 {
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
Fixed node names.
>
>
>> +		compatible = "nxp,pca9539";
>> +		reg = <0x74>;
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		gpio-controller;
>> +		#gpio-cells = <2>;
>> +	};
>> +
>> +	pca4: pca9539@77 {
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
Fixed node names.
>
>
>> +		compatible = "nxp,pca9539";
>> +		reg = <0x77>;
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		gpio-controller;
>> +		#gpio-cells = <2>;
>> +
>> +		gpio-line-names =
>> +			"PE_NVMED0_EXP_PRSNT_N",
>> +			"PE_NVMED1_EXP_PRSNT_N",
>> +			"PE_NVMED2_EXP_PRSNT_N",
>> +			"PE_NVMED3_EXP_PRSNT_N",
>> +			"LED_FAULT_NVMED0",
>> +			"LED_FAULT_NVMED1",
>> +			"LED_FAULT_NVMED2",
>> +			"LED_FAULT_NVMED3",
>> +			"FAN0_PRESENCE_R_N",
>> +			"FAN1_PRESENCE_R_N",
>> +			"FAN2_PRESENCE_R_N",
>> +			"FAN3_PRESENCE_R_N",
>> +			"FAN4_PRESENCE_R_N",
>> +			"FAN5_PRESENCE_N",
>> +			"FAN6_PRESENCE_N",
>> +			"";
>> +	};
>>   };
>>   
>>   &i2c7 {
>> @@ -809,6 +1161,191 @@ regulator@41 {
>>   		compatible = "infineon,ir38263";
>>   		reg = <0x41>;
>>   	};
>> +
>> +	led-controller@61 {
>> +		compatible = "nxp,pca9552";
>> +		reg = <0x61>;
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
> ...
>
>> +		led@15 {
>> +			label = "pe-cp-drv3-perst";
>> +			reg = <15>;
>> +			retain-state-shutdown;
>> +			default-state = "keep";
>> +			type = <PCA955X_TYPE_LED>;
>> +		};
>> +	};
>> +
>> +	pca1: pca9539@75 {
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
Fixed names.

Thanks for the review.

Regards,

Ninad


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 7/8] tpm: tis-i2c: Add more compatible strings

[Ninad Palsule]

From: Joel Stanley <joel@jms.id.au>

The NPCT75x TPM is TIS compatible. It has an I2C and SPI interface.

https://www.nuvoton.com/products/cloud-computing/security/trusted-platform-module-tpm/

Add a compatible string for it, and the generic compatible.

OpenBMC-Staging-Count: 3
Signed-off-by: Joel Stanley <joel@jms.id.au>
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
Link: https://lore.kernel.org/r/20220928043957.2636877-4-joel@jms.id.au
Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
 drivers/char/tpm/tpm_tis_i2c.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
index a897402cc36a..9511c0d50185 100644
--- a/drivers/char/tpm/tpm_tis_i2c.c
+++ b/drivers/char/tpm/tpm_tis_i2c.c
@@ -383,6 +383,8 @@ MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_id);
 #ifdef CONFIG_OF
 static const struct of_device_id of_tis_i2c_match[] = {
 	{ .compatible = "infineon,slb9673", },
+	{ .compatible = "nuvoton,npct75x", },
+	{ .compatible = "tcg,tpm-tis-i2c", },
 	{}
 };
 MODULE_DEVICE_TABLE(of, of_tis_i2c_match);
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 7/8] tpm: tis-i2c: Add more compatible strings

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1437 bytes --]

On Tue, Dec 12, 2023 at 10:40:03AM -0600, Ninad Palsule wrote:
> From: Joel Stanley <joel@jms.id.au>
> 
> The NPCT75x TPM is TIS compatible. It has an I2C and SPI interface.
> 
> https://www.nuvoton.com/products/cloud-computing/security/trusted-platform-module-tpm/
> 
> Add a compatible string for it, and the generic compatible.
> 
> OpenBMC-Staging-Count: 3

Delete this from every patch that it appears from.

> Signed-off-by: Joel Stanley <joel@jms.id.au>
> Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
> Link: https://lore.kernel.org/r/20220928043957.2636877-4-joel@jms.id.au
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
>  drivers/char/tpm/tpm_tis_i2c.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
> index a897402cc36a..9511c0d50185 100644
> --- a/drivers/char/tpm/tpm_tis_i2c.c
> +++ b/drivers/char/tpm/tpm_tis_i2c.c
> @@ -383,6 +383,8 @@ MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_id);
>  #ifdef CONFIG_OF
>  static const struct of_device_id of_tis_i2c_match[] = {
>  	{ .compatible = "infineon,slb9673", },
> +	{ .compatible = "nuvoton,npct75x", },
> +	{ .compatible = "tcg,tpm-tis-i2c", },

What's the point of the generic compatible if you are adding the device
specific ones to the driver anyway?

Thanks,
Conor.

>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, of_tis_i2c_match);
> -- 
> 2.39.2
> 

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 7/8] tpm: tis-i2c: Add more compatible strings

[Guenter Roeck]

On Tue, Dec 12, 2023 at 05:15:51PM +0000, Conor Dooley wrote:
> On Tue, Dec 12, 2023 at 10:40:03AM -0600, Ninad Palsule wrote:
> > From: Joel Stanley <joel@jms.id.au>
> > 
> > The NPCT75x TPM is TIS compatible. It has an I2C and SPI interface.
> > 
> > https://www.nuvoton.com/products/cloud-computing/security/trusted-platform-module-tpm/
> > 
> > Add a compatible string for it, and the generic compatible.
> > 
> > OpenBMC-Staging-Count: 3
> 
> Delete this from every patch that it appears from.
> 
> > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
> > Link: https://lore.kernel.org/r/20220928043957.2636877-4-joel@jms.id.au
> > Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> > ---
> >  drivers/char/tpm/tpm_tis_i2c.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
> > index a897402cc36a..9511c0d50185 100644
> > --- a/drivers/char/tpm/tpm_tis_i2c.c
> > +++ b/drivers/char/tpm/tpm_tis_i2c.c
> > @@ -383,6 +383,8 @@ MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_id);
> >  #ifdef CONFIG_OF
> >  static const struct of_device_id of_tis_i2c_match[] = {
> >  	{ .compatible = "infineon,slb9673", },
> > +	{ .compatible = "nuvoton,npct75x", },
> > +	{ .compatible = "tcg,tpm-tis-i2c", },
> 
> What's the point of the generic compatible if you are adding the device
> specific ones to the driver anyway?
> 

$ git grep infineon,slb9673
Documentation/devicetree/bindings/trivial-devices.yaml:          - infineon,slb9673
drivers/char/tpm/tpm_tis_i2c.c: { .compatible = "infineon,slb9673", },
$ git grep nuvoton,npct75x
arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-bonnell.dts:            compatible = "nuvoton,npct75x", "tcg,tpm-tis-i2c";
arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-everest.dts:            compatible = "nuvoton,npct75x", "tcg,tpm-tis-i2c";
$ git grep tcg,tpm-tis-i2c
arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-bonnell.dts:            compatible = "nuvoton,npct75x", "tcg,tpm-tis-i2c";
arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-everest.dts:            compatible = "nuvoton,npct75x", "tcg,tpm-tis-i2c";
arch/arm/boot/dts/aspeed/aspeed-bmc-opp-tacoma.dts:             compatible = "tcg,tpm-tis-i2c";

It looks like at least the generic entry is needed, given that it is quite
likely that there is hardware out there using it. Other than that, this
makes me wonder: Is there some official guideline describing if and when
to use (only) generic devicetree compatible entries and when specific ones
may / should / have to be used ? I suspect the answer to your question might
simply be "because we did not know better", and it might be helpful to be
able to say "please see XXX for details".

Thanks,
Guenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 7/8] tpm: tis-i2c: Add more compatible strings

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 3766 bytes --]

On Tue, Dec 12, 2023 at 10:00:39AM -0800, Guenter Roeck wrote:
> On Tue, Dec 12, 2023 at 05:15:51PM +0000, Conor Dooley wrote:
> > On Tue, Dec 12, 2023 at 10:40:03AM -0600, Ninad Palsule wrote:
> > > From: Joel Stanley <joel@jms.id.au>
> > > 
> > > The NPCT75x TPM is TIS compatible. It has an I2C and SPI interface.
> > > 
> > > https://www.nuvoton.com/products/cloud-computing/security/trusted-platform-module-tpm/
> > > 
> > > Add a compatible string for it, and the generic compatible.
> > > 
> > > OpenBMC-Staging-Count: 3
> > 
> > Delete this from every patch that it appears from.
> > 
> > > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > > Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > Link: https://lore.kernel.org/r/20220928043957.2636877-4-joel@jms.id.au
> > > Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> > > ---
> > >  drivers/char/tpm/tpm_tis_i2c.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
> > > index a897402cc36a..9511c0d50185 100644
> > > --- a/drivers/char/tpm/tpm_tis_i2c.c
> > > +++ b/drivers/char/tpm/tpm_tis_i2c.c
> > > @@ -383,6 +383,8 @@ MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_id);
> > >  #ifdef CONFIG_OF
> > >  static const struct of_device_id of_tis_i2c_match[] = {
> > >  	{ .compatible = "infineon,slb9673", },
> > > +	{ .compatible = "nuvoton,npct75x", },
> > > +	{ .compatible = "tcg,tpm-tis-i2c", },
> > 
> > What's the point of the generic compatible if you are adding the device
> > specific ones to the driver anyway?
> > 
> 
> $ git grep infineon,slb9673
> Documentation/devicetree/bindings/trivial-devices.yaml:          - infineon,slb9673

Hmm, this would then need to be moved into the new schema, out of
trivial devices.

> drivers/char/tpm/tpm_tis_i2c.c: { .compatible = "infineon,slb9673", },
> $ git grep nuvoton,npct75x
> arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-bonnell.dts:            compatible = "nuvoton,npct75x", "tcg,tpm-tis-i2c";
> arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-everest.dts:            compatible = "nuvoton,npct75x", "tcg,tpm-tis-i2c";
> $ git grep tcg,tpm-tis-i2c
> arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-bonnell.dts:            compatible = "nuvoton,npct75x", "tcg,tpm-tis-i2c";
> arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-everest.dts:            compatible = "nuvoton,npct75x", "tcg,tpm-tis-i2c";
> arch/arm/boot/dts/aspeed/aspeed-bmc-opp-tacoma.dts:             compatible = "tcg,tpm-tis-i2c";

pog, undocumented compatibles.

> It looks like at least the generic entry is needed, given that it is quite
> likely that there is hardware out there using it. Other than that, this
> makes me wonder: Is there some official guideline describing if and when
> to use (only) generic devicetree compatible entries and when specific ones
> may / should / have to be used ? I suspect the answer to your question might
> simply be "because we did not know better", and it might be helpful to be
> able to say "please see XXX for details".

To me using generic compatibles is okay when there is another mechanism
to identify the device. This patch would make more sense if the addition
of nuvoton,npct75x was omitted and the dt-binding had

properties:
  compatible:
    items:
      - enum:
          - infineon,slb9673
          - nuvoton,npct75x
      - const: tcg,tpm-tis-i2c

And whenever new i2c tpms showed up the device specific compatible was
added to the bindings and the driver had only* the generic compatible
static const struct of_device_id of_tis_i2c_match[] = {
	{ .compatible = "infineon,slb9673", },
	{ .compatible = "tcg,tpm-tis-i2c", },
};

* well, and the existing one since that cannot be removed.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 7/8] tpm: tis-i2c: Add more compatible strings

[Guenter Roeck]

On 12/12/23 10:51, Conor Dooley wrote:
> On Tue, Dec 12, 2023 at 10:00:39AM -0800, Guenter Roeck wrote:
>> On Tue, Dec 12, 2023 at 05:15:51PM +0000, Conor Dooley wrote:
>>> On Tue, Dec 12, 2023 at 10:40:03AM -0600, Ninad Palsule wrote:
>>>> From: Joel Stanley <joel@jms.id.au>
>>>>
>>>> The NPCT75x TPM is TIS compatible. It has an I2C and SPI interface.
>>>>
>>>> https://www.nuvoton.com/products/cloud-computing/security/trusted-platform-module-tpm/
>>>>
>>>> Add a compatible string for it, and the generic compatible.
>>>>
>>>> OpenBMC-Staging-Count: 3
>>>
>>> Delete this from every patch that it appears from.
>>>
>>>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>>>> Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
>>>> Link: https://lore.kernel.org/r/20220928043957.2636877-4-joel@jms.id.au
>>>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>>>> ---
>>>>   drivers/char/tpm/tpm_tis_i2c.c | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
>>>> index a897402cc36a..9511c0d50185 100644
>>>> --- a/drivers/char/tpm/tpm_tis_i2c.c
>>>> +++ b/drivers/char/tpm/tpm_tis_i2c.c
>>>> @@ -383,6 +383,8 @@ MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_id);
>>>>   #ifdef CONFIG_OF
>>>>   static const struct of_device_id of_tis_i2c_match[] = {
>>>>   	{ .compatible = "infineon,slb9673", },
>>>> +	{ .compatible = "nuvoton,npct75x", },
>>>> +	{ .compatible = "tcg,tpm-tis-i2c", },
>>>
>>> What's the point of the generic compatible if you are adding the device
>>> specific ones to the driver anyway?
>>>
>>
>> $ git grep infineon,slb9673
>> Documentation/devicetree/bindings/trivial-devices.yaml:          - infineon,slb9673
> 
> Hmm, this would then need to be moved into the new schema, out of
> trivial devices.
> 
>> drivers/char/tpm/tpm_tis_i2c.c: { .compatible = "infineon,slb9673", },
>> $ git grep nuvoton,npct75x
>> arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-bonnell.dts:            compatible = "nuvoton,npct75x", "tcg,tpm-tis-i2c";
>> arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-everest.dts:            compatible = "nuvoton,npct75x", "tcg,tpm-tis-i2c";
>> $ git grep tcg,tpm-tis-i2c
>> arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-bonnell.dts:            compatible = "nuvoton,npct75x", "tcg,tpm-tis-i2c";
>> arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-everest.dts:            compatible = "nuvoton,npct75x", "tcg,tpm-tis-i2c";
>> arch/arm/boot/dts/aspeed/aspeed-bmc-opp-tacoma.dts:             compatible = "tcg,tpm-tis-i2c";
> 
> pog, undocumented compatibles.
> 

Yes, I know, quite annoying. Though, to be fair, a generic "tcg,tpm-tis-i2c"
would make a lot of sense.

Note that Rob had rejected the original addition (into trivial devices)
with the argument that it is not a trivial device
(https://lore.kernel.org/lkml/20220605225610.GA3682221-robh@kernel.org/).

>> It looks like at least the generic entry is needed, given that it is quite
>> likely that there is hardware out there using it. Other than that, this
>> makes me wonder: Is there some official guideline describing if and when
>> to use (only) generic devicetree compatible entries and when specific ones
>> may / should / have to be used ? I suspect the answer to your question might
>> simply be "because we did not know better", and it might be helpful to be
>> able to say "please see XXX for details".
> 
> To me using generic compatibles is okay when there is another mechanism
> to identify the device. This patch would make more sense if the addition
> of nuvoton,npct75x was omitted and the dt-binding had
> 
> properties:
>    compatible:
>      items:
>        - enum:
>            - infineon,slb9673
>            - nuvoton,npct75x
>        - const: tcg,tpm-tis-i2c
> 
> And whenever new i2c tpms showed up the device specific compatible was
> added to the bindings and the driver had only* the generic compatible
> static const struct of_device_id of_tis_i2c_match[] = {
> 	{ .compatible = "infineon,slb9673", },
> 	{ .compatible = "tcg,tpm-tis-i2c", },
> };
> 
> * well, and the existing one since that cannot be removed.

That would be perfectly fine with me. All I personally care about is to get
"tcg,tpm-tis-i2c" added to the kernel source so I can start testing the
code in qemu.

Thanks,
Guenter


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 7/8] tpm: tis-i2c: Add more compatible strings

[Ninad Palsule]

Hello Guenter,

On 12/12/23 13:50, Guenter Roeck wrote:
> On 12/12/23 10:51, Conor Dooley wrote:
>> On Tue, Dec 12, 2023 at 10:00:39AM -0800, Guenter Roeck wrote:
>>> On Tue, Dec 12, 2023 at 05:15:51PM +0000, Conor Dooley wrote:
>>>> On Tue, Dec 12, 2023 at 10:40:03AM -0600, Ninad Palsule wrote:
>>>>> From: Joel Stanley <joel@jms.id.au>
>>>>>
>>>>> The NPCT75x TPM is TIS compatible. It has an I2C and SPI interface.
>>>>>
>>>>> https://www.nuvoton.com/products/cloud-computing/security/trusted-platform-module-tpm/ 
>>>>>
>>>>>
>>>>> Add a compatible string for it, and the generic compatible.
>>>>>
>>>>> OpenBMC-Staging-Count: 3
>>>>
>>>> Delete this from every patch that it appears from.


I have send it as a separate commit. 
https://lore.kernel.org/linux-kernel/20231214144954.3833998-1-ninad@linux.ibm.com/


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 7/8] tpm: tis-i2c: Add more compatible strings

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1210 bytes --]

On Mon, Jan 08, 2024 at 02:05:53PM -0600, Ninad Palsule wrote:
> Hello Guenter,
> 
> On 12/12/23 13:50, Guenter Roeck wrote:
> > On 12/12/23 10:51, Conor Dooley wrote:
> > > On Tue, Dec 12, 2023 at 10:00:39AM -0800, Guenter Roeck wrote:
> > > > On Tue, Dec 12, 2023 at 05:15:51PM +0000, Conor Dooley wrote:
> > > > > On Tue, Dec 12, 2023 at 10:40:03AM -0600, Ninad Palsule wrote:
> > > > > > From: Joel Stanley <joel@jms.id.au>
> > > > > > 
> > > > > > The NPCT75x TPM is TIS compatible. It has an I2C and SPI interface.
> > > > > > 
> > > > > > https://www.nuvoton.com/products/cloud-computing/security/trusted-platform-module-tpm/
> > > > > > 
> > > > > > 
> > > > > > Add a compatible string for it, and the generic compatible.
> > > > > > 
> > > > > > OpenBMC-Staging-Count: 3
> > > > > 
> > > > > Delete this from every patch that it appears from.
> 
> 
> I have send it as a separate commit. https://lore.kernel.org/linux-kernel/20231214144954.3833998-1-ninad@linux.ibm.com/

Why did you do that? It now just adds undocumented compatibles to the
driver. Please, as Rob requested, work with Lukas on his series to make
sure that these devices are documented.

Thanks,
Conor.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 7/8] tpm: tis-i2c: Add more compatible strings

[Ninad Palsule]

Hi Conor,

On 1/9/24 11:14, Conor Dooley wrote:
> On Mon, Jan 08, 2024 at 02:05:53PM -0600, Ninad Palsule wrote:
>> Hello Guenter,
>>
>> On 12/12/23 13:50, Guenter Roeck wrote:
>>> On 12/12/23 10:51, Conor Dooley wrote:
>>>> On Tue, Dec 12, 2023 at 10:00:39AM -0800, Guenter Roeck wrote:
>>>>> On Tue, Dec 12, 2023 at 05:15:51PM +0000, Conor Dooley wrote:
>>>>>> On Tue, Dec 12, 2023 at 10:40:03AM -0600, Ninad Palsule wrote:
>>>>>>> From: Joel Stanley <joel@jms.id.au>
>>>>>>>
>>>>>>> The NPCT75x TPM is TIS compatible. It has an I2C and SPI interface.
>>>>>>>
>>>>>>> https://www.nuvoton.com/products/cloud-computing/security/trusted-platform-module-tpm/
>>>>>>>
>>>>>>>
>>>>>>> Add a compatible string for it, and the generic compatible.
>>>>>>>
>>>>>>> OpenBMC-Staging-Count: 3
>>>>>> Delete this from every patch that it appears from.
>>
>> I have send it as a separate commit. https://lore.kernel.org/linux-kernel/20231214144954.3833998-1-ninad@linux.ibm.com/
> Why did you do that? It now just adds undocumented compatibles to the
> driver. Please, as Rob requested, work with Lukas on his series to make
> sure that these devices are documented.

I think krzysztof kozlowski suggested to send these patches separately: 
https://lore.kernel.org/linux-kernel/1c5ace65-2fd8-4503-b22f-e0f564d1c83f@linaro.org/

Did I misunderstood it? Do you guys want me to include that commit again?

Regards,

Ninad

>
> Thanks,
> Conor.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 7/8] tpm: tis-i2c: Add more compatible strings

[Ninad Palsule]

Hello Conor,

On 1/9/24 11:14, Conor Dooley wrote:
> On Mon, Jan 08, 2024 at 02:05:53PM -0600, Ninad Palsule wrote:
>> Hello Guenter,
>>
>> On 12/12/23 13:50, Guenter Roeck wrote:
>>> On 12/12/23 10:51, Conor Dooley wrote:
>>>> On Tue, Dec 12, 2023 at 10:00:39AM -0800, Guenter Roeck wrote:
>>>>> On Tue, Dec 12, 2023 at 05:15:51PM +0000, Conor Dooley wrote:
>>>>>> On Tue, Dec 12, 2023 at 10:40:03AM -0600, Ninad Palsule wrote:
>>>>>>> From: Joel Stanley <joel@jms.id.au>
>>>>>>>
>>>>>>> The NPCT75x TPM is TIS compatible. It has an I2C and SPI interface.
>>>>>>>
>>>>>>> https://www.nuvoton.com/products/cloud-computing/security/trusted-platform-module-tpm/
>>>>>>>
>>>>>>>
>>>>>>> Add a compatible string for it, and the generic compatible.
>>>>>>>
>>>>>>> OpenBMC-Staging-Count: 3
>>>>>> Delete this from every patch that it appears from.
>>
>> I have send it as a separate commit. https://lore.kernel.org/linux-kernel/20231214144954.3833998-1-ninad@linux.ibm.com/
> Why did you do that? It now just adds undocumented compatibles to the
> driver. Please, as Rob requested, work with Lukas on his series to make
> sure that these devices are documented.

I think krzysztof kozlowski suggested to send these patches separately: 
https://lore.kernel.org/linux-kernel/1c5ace65-2fd8-4503-b22f-e0f564d1c83f@linaro.org/

Did I misunderstood it? Do you guys want me to include that commit again?

Regards,

Ninad



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 7/8] tpm: tis-i2c: Add more compatible strings

[Krzysztof Kozlowski]

On 10/01/2024 00:55, Ninad Palsule wrote:
> Hello Conor,
> 
> On 1/9/24 11:14, Conor Dooley wrote:
>> On Mon, Jan 08, 2024 at 02:05:53PM -0600, Ninad Palsule wrote:
>>> Hello Guenter,
>>>
>>> On 12/12/23 13:50, Guenter Roeck wrote:
>>>> On 12/12/23 10:51, Conor Dooley wrote:
>>>>> On Tue, Dec 12, 2023 at 10:00:39AM -0800, Guenter Roeck wrote:
>>>>>> On Tue, Dec 12, 2023 at 05:15:51PM +0000, Conor Dooley wrote:
>>>>>>> On Tue, Dec 12, 2023 at 10:40:03AM -0600, Ninad Palsule wrote:
>>>>>>>> From: Joel Stanley <joel@jms.id.au>
>>>>>>>>
>>>>>>>> The NPCT75x TPM is TIS compatible. It has an I2C and SPI interface.
>>>>>>>>
>>>>>>>> https://www.nuvoton.com/products/cloud-computing/security/trusted-platform-module-tpm/
>>>>>>>>
>>>>>>>>
>>>>>>>> Add a compatible string for it, and the generic compatible.
>>>>>>>>
>>>>>>>> OpenBMC-Staging-Count: 3
>>>>>>> Delete this from every patch that it appears from.
>>>
>>> I have send it as a separate commit. https://lore.kernel.org/linux-kernel/20231214144954.3833998-1-ninad@linux.ibm.com/
>> Why did you do that? It now just adds undocumented compatibles to the
>> driver. Please, as Rob requested, work with Lukas on his series to make
>> sure that these devices are documented.
> 
> I think krzysztof kozlowski suggested to send these patches separately: 
> https://lore.kernel.org/linux-kernel/1c5ace65-2fd8-4503-b22f-e0f564d1c83f@linaro.org/
> 
> Did I misunderstood it? Do you guys want me to include that commit again?

My comment was in DTS thread under specific DTS patch. How did you
figure out it applies to driver and bindings? This does not make sense.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 7/8] tpm: tis-i2c: Add more compatible strings

[Ninad Palsule]

Hello Krzysztof,



>>>> I have send it as a separate commit. https://lore.kernel.org/linux-kernel/20231214144954.3833998-1-ninad@linux.ibm.com/
>>> Why did you do that? It now just adds undocumented compatibles to the
>>> driver. Please, as Rob requested, work with Lukas on his series to make
>>> sure that these devices are documented.
>> I think krzysztof kozlowski suggested to send these patches separately:
>> https://lore.kernel.org/linux-kernel/1c5ace65-2fd8-4503-b22f-e0f564d1c83f@linaro.org/
>>
>> Did I misunderstood it? Do you guys want me to include that commit again?
> My comment was in DTS thread under specific DTS patch. How did you
> figure out it applies to driver and bindings? This does not make sense.

Sorry for the misunderstanding. Where do you want me to add driver 
patch? Before all DTS patches or after all DTS patches?

tpm: tis-i2c: Add more compatible strings
ARM: dts: aspeed: System1: IBM system1 BMC board
dt-bindings: Add DPS310 as trivial device
dt-bindings: arm: aspeed: add IBM system1-bmc

OR

ARM: dts: aspeed: System1: IBM system1 BMC board
dt-bindings: Add DPS310 as trivial device
dt-bindings: arm: aspeed: add IBM system1-bmc

tpm: tis-i2c: Add more compatible strings

Thanks & Regards,

Ninad Palsule


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 7/8] tpm: tis-i2c: Add more compatible strings

[Krzysztof Kozlowski]

On 10/01/2024 15:31, Ninad Palsule wrote:
> Hello Krzysztof,
> 
> 
> 
>>>>> I have send it as a separate commit. https://lore.kernel.org/linux-kernel/20231214144954.3833998-1-ninad@linux.ibm.com/
>>>> Why did you do that? It now just adds undocumented compatibles to the
>>>> driver. Please, as Rob requested, work with Lukas on his series to make
>>>> sure that these devices are documented.
>>> I think krzysztof kozlowski suggested to send these patches separately:
>>> https://lore.kernel.org/linux-kernel/1c5ace65-2fd8-4503-b22f-e0f564d1c83f@linaro.org/
>>>
>>> Did I misunderstood it? Do you guys want me to include that commit again?
>> My comment was in DTS thread under specific DTS patch. How did you
>> figure out it applies to driver and bindings? This does not make sense.
> 
> Sorry for the misunderstanding. Where do you want me to add driver 
> patch? Before all DTS patches or after all DTS patches?

Does not matter, why do you insist on combining them with DTS? Drivers
and bindings are going together. DTS better separate, although depending
on the case can be together.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 7/8] tpm: tis-i2c: Add more compatible strings

[Ninad Palsule]

Hello Krzysztof,


On 1/10/24 09:37, Krzysztof Kozlowski wrote:
> On 10/01/2024 15:31, Ninad Palsule wrote:
>> Hello Krzysztof,
>>
>>
>>
>>>>>> I have send it as a separate commit. https://lore.kernel.org/linux-kernel/20231214144954.3833998-1-ninad@linux.ibm.com/
>>>>> Why did you do that? It now just adds undocumented compatibles to the
>>>>> driver. Please, as Rob requested, work with Lukas on his series to make
>>>>> sure that these devices are documented.
>>>> I think krzysztof kozlowski suggested to send these patches separately:
>>>> https://lore.kernel.org/linux-kernel/1c5ace65-2fd8-4503-b22f-e0f564d1c83f@linaro.org/
>>>>
>>>> Did I misunderstood it? Do you guys want me to include that commit again?
>>> My comment was in DTS thread under specific DTS patch. How did you
>>> figure out it applies to driver and bindings? This does not make sense.
>> Sorry for the misunderstanding. Where do you want me to add driver
>> patch? Before all DTS patches or after all DTS patches?
> Does not matter, why do you insist on combining them with DTS? Drivers
> and bindings are going together. DTS better separate, although depending
> on the case can be together.
>
I have combined DTS and Driver because DTS was using compatibility 
string which is not upstream yet hence I thought it is logical to send 
it under same patchset.

Conor and Rob, Do you have preference?

Thanks & Regards,

Ninad Palsule


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 7/8] tpm: tis-i2c: Add more compatible strings

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1920 bytes --]

On Wed, Jan 10, 2024 at 09:54:19AM -0600, Ninad Palsule wrote:
> Hello Krzysztof,
> 
> 
> On 1/10/24 09:37, Krzysztof Kozlowski wrote:
> > On 10/01/2024 15:31, Ninad Palsule wrote:
> > > Hello Krzysztof,
> > > 
> > > 
> > > 
> > > > > > > I have send it as a separate commit. https://lore.kernel.org/linux-kernel/20231214144954.3833998-1-ninad@linux.ibm.com/
> > > > > > Why did you do that? It now just adds undocumented compatibles to the
> > > > > > driver. Please, as Rob requested, work with Lukas on his series to make
> > > > > > sure that these devices are documented.
> > > > > I think krzysztof kozlowski suggested to send these patches separately:
> > > > > https://lore.kernel.org/linux-kernel/1c5ace65-2fd8-4503-b22f-e0f564d1c83f@linaro.org/
> > > > > 
> > > > > Did I misunderstood it? Do you guys want me to include that commit again?
> > > > My comment was in DTS thread under specific DTS patch. How did you
> > > > figure out it applies to driver and bindings? This does not make sense.
> > > Sorry for the misunderstanding. Where do you want me to add driver
> > > patch? Before all DTS patches or after all DTS patches?
> > Does not matter, why do you insist on combining them with DTS? Drivers
> > and bindings are going together. DTS better separate, although depending
> > on the case can be together.
> > 
> I have combined DTS and Driver because DTS was using compatibility string
> which is not upstream yet hence I thought it is logical to send it under
> same patchset.
> 
> Conor and Rob, Do you have preference?

I'm not sure what you want from me that Krzysztof hasn't already
provided. dt-bindings and drivers usually go together, and the dts
patches are often in the same series. If you send dts separately, note
under the --- line the patches adding the binding so that the platform
maintainer knows that the compatible has not yet been documented.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 7/8] tpm: tis-i2c: Add more compatible strings

[Krzysztof Kozlowski]

On 10/01/2024 16:54, Ninad Palsule wrote:
> Hello Krzysztof,
> 
> 
> On 1/10/24 09:37, Krzysztof Kozlowski wrote:
>> On 10/01/2024 15:31, Ninad Palsule wrote:
>>> Hello Krzysztof,
>>>
>>>
>>>
>>>>>>> I have send it as a separate commit. https://lore.kernel.org/linux-kernel/20231214144954.3833998-1-ninad@linux.ibm.com/
>>>>>> Why did you do that? It now just adds undocumented compatibles to the
>>>>>> driver. Please, as Rob requested, work with Lukas on his series to make
>>>>>> sure that these devices are documented.
>>>>> I think krzysztof kozlowski suggested to send these patches separately:
>>>>> https://lore.kernel.org/linux-kernel/1c5ace65-2fd8-4503-b22f-e0f564d1c83f@linaro.org/
>>>>>
>>>>> Did I misunderstood it? Do you guys want me to include that commit again?
>>>> My comment was in DTS thread under specific DTS patch. How did you
>>>> figure out it applies to driver and bindings? This does not make sense.
>>> Sorry for the misunderstanding. Where do you want me to add driver
>>> patch? Before all DTS patches or after all DTS patches?
>> Does not matter, why do you insist on combining them with DTS? Drivers
>> and bindings are going together. DTS better separate, although depending
>> on the case can be together.
>>
> I have combined DTS and Driver because DTS was using compatibility 
> string which is not upstream yet hence I thought it is logical to send 
> it under same patchset.

Sometimes yes, sometimes not. DTS must not go via driver subsystem, so
sending it in the same patchset has implications on maintainers applying
it. Some like it, some don't and you will be nagged for combining them.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 7/8] tpm: tis-i2c: Add more compatible strings

[Guenter Roeck]

On 1/10/24 09:54, Krzysztof Kozlowski wrote:
> On 10/01/2024 16:54, Ninad Palsule wrote:
>> Hello Krzysztof,
>>
>>
>> On 1/10/24 09:37, Krzysztof Kozlowski wrote:
>>> On 10/01/2024 15:31, Ninad Palsule wrote:
>>>> Hello Krzysztof,
>>>>
>>>>
>>>>
>>>>>>>> I have send it as a separate commit. https://lore.kernel.org/linux-kernel/20231214144954.3833998-1-ninad@linux.ibm.com/
>>>>>>> Why did you do that? It now just adds undocumented compatibles to the
>>>>>>> driver. Please, as Rob requested, work with Lukas on his series to make
>>>>>>> sure that these devices are documented.
>>>>>> I think krzysztof kozlowski suggested to send these patches separately:
>>>>>> https://lore.kernel.org/linux-kernel/1c5ace65-2fd8-4503-b22f-e0f564d1c83f@linaro.org/
>>>>>>
>>>>>> Did I misunderstood it? Do you guys want me to include that commit again?
>>>>> My comment was in DTS thread under specific DTS patch. How did you
>>>>> figure out it applies to driver and bindings? This does not make sense.
>>>> Sorry for the misunderstanding. Where do you want me to add driver
>>>> patch? Before all DTS patches or after all DTS patches?
>>> Does not matter, why do you insist on combining them with DTS? Drivers
>>> and bindings are going together. DTS better separate, although depending
>>> on the case can be together.
>>>
>> I have combined DTS and Driver because DTS was using compatibility
>> string which is not upstream yet hence I thought it is logical to send
>> it under same patchset.
> 
> Sometimes yes, sometimes not. DTS must not go via driver subsystem, so
> sending it in the same patchset has implications on maintainers applying
> it. Some like it, some don't and you will be nagged for combining them.
> 

"DTS must not go via driver subsystem"

I always thought the guideline was to submit separate _patches_ for dts
and driver changes, but as part of a single series. I didn't know that
there is a rule to submit separate patch _series_. I also didn't know
(and as far as I know no one called me on it) that I am not supposed
to _apply_ dts changes. So far, I typically applied dts changes together
with driver patches after receiving an Acked-by: or Reviewed-by:
from a devicetree maintainer.

This exchange suggests that I did it all wrong. Should I reject devicetree
patches submitted as part of a driver patch series going forward ?
Should I not apply dts patches submitted as part of a patch series ?
If so, it would help to have some documentation I can point to to explain
the rationale to submitters (and myself). Also, in that case, how is the
synchronization between device tree patches and driver patches supposed
to happen ?

FWIW, if dts changes are sent as separate series, I don't know how I would
be able to review driver changes/submissions without being copied on the
associated dts changes.

Guess I am more than a bit confused.

Thanks,
Guenter


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 7/8] tpm: tis-i2c: Add more compatible strings

[Krzysztof Kozlowski]

On 10/01/2024 20:06, Guenter Roeck wrote:
> On 1/10/24 09:54, Krzysztof Kozlowski wrote:
>> On 10/01/2024 16:54, Ninad Palsule wrote:
>>> Hello Krzysztof,
>>>
>>>
>>> On 1/10/24 09:37, Krzysztof Kozlowski wrote:
>>>> On 10/01/2024 15:31, Ninad Palsule wrote:
>>>>> Hello Krzysztof,
>>>>>
>>>>>
>>>>>
>>>>>>>>> I have send it as a separate commit. https://lore.kernel.org/linux-kernel/20231214144954.3833998-1-ninad@linux.ibm.com/
>>>>>>>> Why did you do that? It now just adds undocumented compatibles to the
>>>>>>>> driver. Please, as Rob requested, work with Lukas on his series to make
>>>>>>>> sure that these devices are documented.
>>>>>>> I think krzysztof kozlowski suggested to send these patches separately:
>>>>>>> https://lore.kernel.org/linux-kernel/1c5ace65-2fd8-4503-b22f-e0f564d1c83f@linaro.org/
>>>>>>>
>>>>>>> Did I misunderstood it? Do you guys want me to include that commit again?
>>>>>> My comment was in DTS thread under specific DTS patch. How did you
>>>>>> figure out it applies to driver and bindings? This does not make sense.
>>>>> Sorry for the misunderstanding. Where do you want me to add driver
>>>>> patch? Before all DTS patches or after all DTS patches?
>>>> Does not matter, why do you insist on combining them with DTS? Drivers
>>>> and bindings are going together. DTS better separate, although depending
>>>> on the case can be together.
>>>>
>>> I have combined DTS and Driver because DTS was using compatibility
>>> string which is not upstream yet hence I thought it is logical to send
>>> it under same patchset.
>>
>> Sometimes yes, sometimes not. DTS must not go via driver subsystem, so
>> sending it in the same patchset has implications on maintainers applying
>> it. Some like it, some don't and you will be nagged for combining them.
>>
> 
> "DTS must not go via driver subsystem"
> 
> I always thought the guideline was to submit separate _patches_ for dts
> and driver changes, but as part of a single series. I didn't know that
> there is a rule to submit separate patch _series_. I also didn't know
> (and as far as I know no one called me on it) that I am not supposed
> to _apply_ dts changes. So far, I typically applied dts changes together
> with driver patches after receiving an Acked-by: or Reviewed-by:
> from a devicetree maintainer.

I did not notice you applying them, but such guideline - DTS must go via
respective SoC tree - was always repeated by me and SoC maintainers.
Just like gazillion other things probably was not documented... or even
if it was documented, it would be so deep among hundreds of other rules
nobody would find it. :)

> 
> This exchange suggests that I did it all wrong. Should I reject devicetree
> patches submitted as part of a driver patch series going forward ?

I propose: just ignore them. The SoC maintainer will pick them up.

> Should I not apply dts patches submitted as part of a patch series ?

No, please do not apply them.

> If so, it would help to have some documentation I can point to to explain
> the rationale to submitters (and myself). Also, in that case, how is the

Yes, it would. I can try to create something.

> synchronization between device tree patches and driver patches supposed
> to happen ?

There should not be synchronization. Just to remind: we talk about DTS
(so also DTSI and DTSO), thus everything being in arch/*/boot/dts/. We
do not talk about DT bindings, right? The bindings are obvious (and
documented): preferably go via driver subsystem, with fallback/special
cases via SoC tree and fallback to Rob.

The DTS must be independent from driver changes. If synchronization is
needed, it means it is not independent. It happens from time to time,
kind of expected exception, with proper justifications. In such case,
recommendation is to send DTS for the next kernel release, so after the
driver changes hit rc1.

> 
> FWIW, if dts changes are sent as separate series, I don't know how I would
> be able to review driver changes/submissions without being copied on the
> associated dts changes.

People are also encouraged to provide links between them. One lore link
is required: the DTS patchset, if sent in parallel, should have a link
to the thread with the bindings being reviewed on the list. Of course if
bindings are already accepted (in linux-next), then it's not necessary.
We all know how to use git grep.

Now about your specific question:

As a driver subsystem maintainer you are not expected, as a requirement,
to review DTS code. You can, you are welcomed, but don't you have
already too much stuff to review? Why would like to jump to DTS? If you
do, your help is appreciated, though. However think for a sec how would
it even work? Imagine we send new driver + bindings + DTS for OMAP in
one thread, so you see the DTS for your review. Now 2 months later I
send DTS for Qualcomm using that driver. You would never be Cc-ed on
this second submission. It wouldn't be even possible: get_maintainers.pl
would not print your email. Why would you be expected to be CCed on
first DTS to review and not on all others/further ones? This does not
make sense, really.

BTW, I am not saying here anything new. I was babbling about this every
second month since few years. :)

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 7/8] tpm: tis-i2c: Add more compatible strings

[Krzysztof Kozlowski]

On 10/01/2024 21:34, Krzysztof Kozlowski wrote:
>>>>>>>>>> I have send it as a separate commit. https://lore.kernel.org/linux-kernel/20231214144954.3833998-1-ninad@linux.ibm.com/
>>>>>>>>> Why did you do that? It now just adds undocumented compatibles to the
>>>>>>>>> driver. Please, as Rob requested, work with Lukas on his series to make
>>>>>>>>> sure that these devices are documented.
>>>>>>>> I think krzysztof kozlowski suggested to send these patches separately:
>>>>>>>> https://lore.kernel.org/linux-kernel/1c5ace65-2fd8-4503-b22f-e0f564d1c83f@linaro.org/
>>>>>>>>
>>>>>>>> Did I misunderstood it? Do you guys want me to include that commit again?
>>>>>>> My comment was in DTS thread under specific DTS patch. How did you
>>>>>>> figure out it applies to driver and bindings? This does not make sense.
>>>>>> Sorry for the misunderstanding. Where do you want me to add driver
>>>>>> patch? Before all DTS patches or after all DTS patches?
>>>>> Does not matter, why do you insist on combining them with DTS? Drivers
>>>>> and bindings are going together. DTS better separate, although depending
>>>>> on the case can be together.
>>>>>
>>>> I have combined DTS and Driver because DTS was using compatibility
>>>> string which is not upstream yet hence I thought it is logical to send
>>>> it under same patchset.
>>>
>>> Sometimes yes, sometimes not. DTS must not go via driver subsystem, so
>>> sending it in the same patchset has implications on maintainers applying
>>> it. Some like it, some don't and you will be nagged for combining them.
>>>
>>
>> "DTS must not go via driver subsystem"
>>
>> I always thought the guideline was to submit separate _patches_ for dts
>> and driver changes, but as part of a single series. I didn't know that
>> there is a rule to submit separate patch _series_. I also didn't know
>> (and as far as I know no one called me on it) that I am not supposed
>> to _apply_ dts changes. So far, I typically applied dts changes together
>> with driver patches after receiving an Acked-by: or Reviewed-by:
>> from a devicetree maintainer.
> 
> I did not notice you applying them, but such guideline - DTS must go via
> respective SoC tree - was always repeated by me and SoC maintainers.
> Just like gazillion other things probably was not documented... or even
> if it was documented, it would be so deep among hundreds of other rules
> nobody would find it. :)
> 
>>
>> This exchange suggests that I did it all wrong. Should I reject devicetree
>> patches submitted as part of a driver patch series going forward ?
> 
> I propose: just ignore them. The SoC maintainer will pick them up.
> 
>> Should I not apply dts patches submitted as part of a patch series ?
> 
> No, please do not apply them.

Eh, English can be confusing. Let's make it easier to grasp:
"Please do not apply DTS patches to a driver subsystem."

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 7/8] tpm: tis-i2c: Add more compatible strings

[Guenter Roeck]

On 1/10/24 12:34, Krzysztof Kozlowski wrote:
> On 10/01/2024 20:06, Guenter Roeck wrote:
>> On 1/10/24 09:54, Krzysztof Kozlowski wrote:
>>> On 10/01/2024 16:54, Ninad Palsule wrote:
>>>> Hello Krzysztof,
>>>>
>>>>
>>>> On 1/10/24 09:37, Krzysztof Kozlowski wrote:
>>>>> On 10/01/2024 15:31, Ninad Palsule wrote:
>>>>>> Hello Krzysztof,
>>>>>>
>>>>>>
>>>>>>
>>>>>>>>>> I have send it as a separate commit. https://lore.kernel.org/linux-kernel/20231214144954.3833998-1-ninad@linux.ibm.com/
>>>>>>>>> Why did you do that? It now just adds undocumented compatibles to the
>>>>>>>>> driver. Please, as Rob requested, work with Lukas on his series to make
>>>>>>>>> sure that these devices are documented.
>>>>>>>> I think krzysztof kozlowski suggested to send these patches separately:
>>>>>>>> https://lore.kernel.org/linux-kernel/1c5ace65-2fd8-4503-b22f-e0f564d1c83f@linaro.org/
>>>>>>>>
>>>>>>>> Did I misunderstood it? Do you guys want me to include that commit again?
>>>>>>> My comment was in DTS thread under specific DTS patch. How did you
>>>>>>> figure out it applies to driver and bindings? This does not make sense.
>>>>>> Sorry for the misunderstanding. Where do you want me to add driver
>>>>>> patch? Before all DTS patches or after all DTS patches?
>>>>> Does not matter, why do you insist on combining them with DTS? Drivers
>>>>> and bindings are going together. DTS better separate, although depending
>>>>> on the case can be together.
>>>>>
>>>> I have combined DTS and Driver because DTS was using compatibility
>>>> string which is not upstream yet hence I thought it is logical to send
>>>> it under same patchset.
>>>
>>> Sometimes yes, sometimes not. DTS must not go via driver subsystem, so
>>> sending it in the same patchset has implications on maintainers applying
>>> it. Some like it, some don't and you will be nagged for combining them.
>>>
>>
>> "DTS must not go via driver subsystem"
>>
>> I always thought the guideline was to submit separate _patches_ for dts
>> and driver changes, but as part of a single series. I didn't know that
>> there is a rule to submit separate patch _series_. I also didn't know
>> (and as far as I know no one called me on it) that I am not supposed
>> to _apply_ dts changes. So far, I typically applied dts changes together
>> with driver patches after receiving an Acked-by: or Reviewed-by:
>> from a devicetree maintainer.
> 
> I did not notice you applying them, but such guideline - DTS must go via
> respective SoC tree - was always repeated by me and SoC maintainers.
> Just like gazillion other things probably was not documented... or even
> if it was documented, it would be so deep among hundreds of other rules
> nobody would find it. :)
> 
>>
>> This exchange suggests that I did it all wrong. Should I reject devicetree
>> patches submitted as part of a driver patch series going forward ?
> 
> I propose: just ignore them. The SoC maintainer will pick them up.
> 
>> Should I not apply dts patches submitted as part of a patch series ?
> 
> No, please do not apply them.
> 
>> If so, it would help to have some documentation I can point to to explain
>> the rationale to submitters (and myself). Also, in that case, how is the
> 
> Yes, it would. I can try to create something.
> 
>> synchronization between device tree patches and driver patches supposed
>> to happen ?
> 
> There should not be synchronization. Just to remind: we talk about DTS
> (so also DTSI and DTSO), thus everything being in arch/*/boot/dts/. We
> do not talk about DT bindings, right? The bindings are obvious (and
> documented): preferably go via driver subsystem, with fallback/special
> cases via SoC tree and fallback to Rob.
> 

Sorry, misunderstanding on my side. I do not and never did apply patches
in arch/*/boot/dts/. I referred to patches in Documentation/devicetree/

Sorry, I though you also referred to bindings. My bad.

Guenter


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 7/8] tpm: tis-i2c: Add more compatible strings

[Ninad Palsule]

Hello Conor,

On 12/12/23 11:15, Conor Dooley wrote:
> On Tue, Dec 12, 2023 at 10:40:03AM -0600, Ninad Palsule wrote:
>> From: Joel Stanley <joel@jms.id.au>
>>
>> The NPCT75x TPM is TIS compatible. It has an I2C and SPI interface.
>>
>> https://www.nuvoton.com/products/cloud-computing/security/trusted-platform-module-tpm/
>>
>> Add a compatible string for it, and the generic compatible.
>>
>> OpenBMC-Staging-Count: 3
> Delete this from every patch that it appears from.
Removed.
>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
>> Link: https://lore.kernel.org/r/20220928043957.2636877-4-joel@jms.id.au
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>>   drivers/char/tpm/tpm_tis_i2c.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_i2c.c b/drivers/char/tpm/tpm_tis_i2c.c
>> index a897402cc36a..9511c0d50185 100644
>> --- a/drivers/char/tpm/tpm_tis_i2c.c
>> +++ b/drivers/char/tpm/tpm_tis_i2c.c
>> @@ -383,6 +383,8 @@ MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_id);
>>   #ifdef CONFIG_OF
>>   static const struct of_device_id of_tis_i2c_match[] = {
>>   	{ .compatible = "infineon,slb9673", },
>> +	{ .compatible = "nuvoton,npct75x", },
>> +	{ .compatible = "tcg,tpm-tis-i2c", },
> What's the point of the generic compatible if you are adding the device
> specific ones to the driver anyway?
>
> Thanks,
> Conor.

I have removed this commit from this patchset and send it as a separate 
patch.

Regards,

Ninad


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 8/8] ARM: dts: aspeed: System1: PS, sensor and more

[Ninad Palsule]

This drop adds following devices in the device tree.
- EEPROM/VPD
- Power supplies
- Humidity, pressure and temperature sensors.
- Trusted platform module(TPM) chip

Tested:
    This board is tested using the simics simulator.

Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
 .../dts/aspeed/aspeed-bmc-ibm-system1.dts     | 76 +++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
index 75562aa63701..d960b938fe8d 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
+++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
@@ -461,6 +461,11 @@ &kcs3 {
 &i2c0 {
 	status = "okay";
 
+	eeprom@50 {
+		compatible = "atmel,24c64";
+		reg = <0x50>;
+	};
+
 	regulator@60 {
 		compatible = "maxim,max8952";
 		reg = <0x60>;
@@ -655,6 +660,25 @@ pca0: pca9539@74 {
 
 &i2c2 {
 	status = "okay";
+
+	power-supply@58 {
+		compatible = "ibm,cffps";
+		reg = <0x58>;
+	};
+
+	power-supply@59 {
+		compatible = "ibm,cffps";
+		reg = <0x59>;
+	};
+
+	power-supply@5a {
+		compatible = "ibm,cffps";
+		reg = <0x5a>;
+	};
+	power-supply@5b {
+		compatible = "ibm,cffps";
+		reg = <0x5b>;
+	};
 };
 
 &i2c3 {
@@ -758,6 +782,11 @@ fan-controller@54 {
 		#size-cells = <0>;
 	};
 
+	eeprom@55 {
+		compatible = "atmel,24c64";
+		reg = <0x55>;
+	};
+
 	i2c-mux@70 {
 		compatible = "nxp,pca9548";
 		reg = <0x70>;
@@ -795,6 +824,21 @@ i2c6mux0chn4: i2c@4 {
 			#size-cells = <0>;
 			reg = <4>;
 
+			humidity-sensor@40 {
+				compatible = "ti,hdc1080";
+				reg = <0x40>;
+			};
+
+			temperature-sensor@48 {
+				compatible = "ti,tmp275";
+				reg = <0x48>;
+			};
+
+			eeprom@50 {
+				compatible = "atmel,24c32";
+				reg = <0x50>;
+			};
+
 			led-controller@60 {
 				compatible = "nxp,pca9551";
 				reg = <0x60>;
@@ -836,6 +880,12 @@ led@3 {
 					type = <PCA955X_TYPE_LED>;
 				};
 			};
+
+			temperature-sensor@76 {
+				compatible = "infineon,dps310";
+				reg = <0x76>;
+				#io-channel-cells = <0>;
+			};
 		};
 
 		i2c6mux0chn5: i2c@5 {
@@ -1100,6 +1150,11 @@ i2c8mux0chn6: i2c@6 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <6>;
+
+			temperature-sensor@4c {
+				compatible = "ti,tmp423";
+				reg = <0x4c>;
+			};
 		};
 
 		i2c8mux0chn7: i2c@7 {
@@ -1128,6 +1183,11 @@ regulator@41 {
 		reg = <0x41>;
 	};
 
+	eeprom@50 {
+		compatible = "atmel,24c64";
+		reg = <0x50>;
+	};
+
 	regulator@60 {
 		compatible = "maxim,max8952";
 		reg = <0x60>;
@@ -1148,6 +1208,12 @@ regulator@60 {
 
 &i2c11 {
 	status = "okay";
+
+	tpm@2e {
+		compatible = "nuvoton,npct75x", "tcg,tpm-tis-i2c";
+		reg = <0x2e>;
+		memory-region = <&event_log>;
+	};
 };
 
 &i2c12 {
@@ -1552,6 +1618,11 @@ i2c15mux0chn6: i2c@6 {
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <6>;
+
+			temperature-sensor@4c {
+				compatible = "ti,tmp423";
+				reg = <0x4c>;
+			};
 		};
 
 		i2c15mux0chn7: i2c@7 {
@@ -1563,6 +1634,11 @@ regulator@40 {
 				compatible = "infineon,ir38060";
 				reg = <0x40>;
 			};
+
+			temperature-sensor@4c {
+				compatible = "ti,tmp423";
+				reg = <0x4c>;
+			};
 		};
 	};
 };
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 8/8] ARM: dts: aspeed: System1: PS, sensor and more

[Krzysztof Kozlowski]

On 12/12/2023 17:40, Ninad Palsule wrote:
> This drop adds following devices in the device tree.
> - EEPROM/VPD
> - Power supplies
> - Humidity, pressure and temperature sensors.
> - Trusted platform module(TPM) chip
> 
> Tested:
>     This board is tested using the simics simulator.
> 
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---

Don't mix DTS with drivers. DTS and drivers go via different subsystems
and cannot have dependencies, so why DTS is patch #6, then driver #7 and
now again DTS #7?


>  .../dts/aspeed/aspeed-bmc-ibm-system1.dts     | 76 +++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
> index 75562aa63701..d960b938fe8d 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
> @@ -461,6 +461,11 @@ &kcs3 {
>  &i2c0 {
>  	status = "okay";
>  
> +	eeprom@50 {
> +		compatible = "atmel,24c64";
> +		reg = <0x50>;
> +	};
> +
>  	regulator@60 {
>  		compatible = "maxim,max8952";
>  		reg = <0x60>;
> @@ -655,6 +660,25 @@ pca0: pca9539@74 {
>  
>  &i2c2 {
>  	status = "okay";
> +
> +	power-supply@58 {
> +		compatible = "ibm,cffps";
> +		reg = <0x58>;
> +	};
> +
> +	power-supply@59 {
> +		compatible = "ibm,cffps";
> +		reg = <0x59>;
> +	};
> +
> +	power-supply@5a {
> +		compatible = "ibm,cffps";
> +		reg = <0x5a>;
> +	};

Missing blank line

> +	power-supply@5b {
> +		compatible = "ibm,cffps";
> +		reg = <0x5b>;
> +	};


Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 8/8] ARM: dts: aspeed: System1: PS, sensor and more

[Ninad Palsule]

Hello Krzysztof,

On 12/12/23 14:26, Krzysztof Kozlowski wrote:
> On 12/12/2023 17:40, Ninad Palsule wrote:
>> This drop adds following devices in the device tree.
>> - EEPROM/VPD
>> - Power supplies
>> - Humidity, pressure and temperature sensors.
>> - Trusted platform module(TPM) chip
>>
>> Tested:
>>      This board is tested using the simics simulator.
>>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
> Don't mix DTS with drivers. DTS and drivers go via different subsystems
> and cannot have dependencies, so why DTS is patch #6, then driver #7 and
> now again DTS #7?

There is a dependency on driver code as patch #8 uses the compatibility 
string added in driver patch #7.  I have now moved driver patch at the 
start. Is that ok? OR you are suggesting something else?

v1-0001-tpm-tis-i2c-Add-more-compatible-strings.patch
                         |
v1-0009-ARM-dts-aspeed-System1-PS-sensor-and-more.patch

>
>>   .../dts/aspeed/aspeed-bmc-ibm-system1.dts     | 76 +++++++++++++++++++
>>   1 file changed, 76 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
>> index 75562aa63701..d960b938fe8d 100644
>> --- a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
>> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
>> @@ -461,6 +461,11 @@ &kcs3 {
>>   &i2c0 {
>>   	status = "okay";
>>   
>> +	eeprom@50 {
>> +		compatible = "atmel,24c64";
>> +		reg = <0x50>;
>> +	};
>> +
>>   	regulator@60 {
>>   		compatible = "maxim,max8952";
>>   		reg = <0x60>;
>> @@ -655,6 +660,25 @@ pca0: pca9539@74 {
>>   
>>   &i2c2 {
>>   	status = "okay";
>> +
>> +	power-supply@58 {
>> +		compatible = "ibm,cffps";
>> +		reg = <0x58>;
>> +	};
>> +
>> +	power-supply@59 {
>> +		compatible = "ibm,cffps";
>> +		reg = <0x59>;
>> +	};
>> +
>> +	power-supply@5a {
>> +		compatible = "ibm,cffps";
>> +		reg = <0x5a>;
>> +	};
> Missing blank line

Fixed it.

Thanks for the review.

Regards,

Ninad

>
>> +	power-supply@5b {
>> +		compatible = "ibm,cffps";
>> +		reg = <0x5b>;
>> +	};
>
> Best regards,
> Krzysztof
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 8/8] ARM: dts: aspeed: System1: PS, sensor and more

[Krzysztof Kozlowski]

On 13/12/2023 20:02, Ninad Palsule wrote:
> Hello Krzysztof,
> 
> On 12/12/23 14:26, Krzysztof Kozlowski wrote:
>> On 12/12/2023 17:40, Ninad Palsule wrote:
>>> This drop adds following devices in the device tree.
>>> - EEPROM/VPD
>>> - Power supplies
>>> - Humidity, pressure and temperature sensors.
>>> - Trusted platform module(TPM) chip
>>>
>>> Tested:
>>>      This board is tested using the simics simulator.
>>>
>>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>>> ---
>> Don't mix DTS with drivers. DTS and drivers go via different subsystems
>> and cannot have dependencies, so why DTS is patch #6, then driver #7 and
>> now again DTS #7?
> 
> There is a dependency on driver code as patch #8 uses the compatibility 
> string added in driver patch #7.  I have now moved driver patch at the 
> start. Is that ok? OR you are suggesting something else?

First, there is no dependency. Second, except confusing order anyway DTS
will go via separate trees. Third, again, there is no dependency. If
there is, your patchset is broken and this needs to be fixed. Although I
don't understand how new hardware can depend on driver... it's really odd.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 8/8] ARM: dts: aspeed: System1: PS, sensor and more

[Ninad Palsule]

Hello Krzysztof,

On 12/13/23 13:37, Krzysztof Kozlowski wrote:
> On 13/12/2023 20:02, Ninad Palsule wrote:
>> Hello Krzysztof,
>>
>> On 12/12/23 14:26, Krzysztof Kozlowski wrote:
>>> On 12/12/2023 17:40, Ninad Palsule wrote:
>>>> This drop adds following devices in the device tree.
>>>> - EEPROM/VPD
>>>> - Power supplies
>>>> - Humidity, pressure and temperature sensors.
>>>> - Trusted platform module(TPM) chip
>>>>
>>>> Tested:
>>>>       This board is tested using the simics simulator.
>>>>
>>>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>>>> ---
>>> Don't mix DTS with drivers. DTS and drivers go via different subsystems
>>> and cannot have dependencies, so why DTS is patch #6, then driver #7 and
>>> now again DTS #7?
>> There is a dependency on driver code as patch #8 uses the compatibility
>> string added in driver patch #7.  I have now moved driver patch at the
>> start. Is that ok? OR you are suggesting something else?
> First, there is no dependency. Second, except confusing order anyway DTS
> will go via separate trees. Third, again, there is no dependency. If
> there is, your patchset is broken and this needs to be fixed. Although I
> don't understand how new hardware can depend on driver... it's really odd.

Thanks for the quick response.

This board uses the nuvoton TPM device. The tpm devices uses 
"nuvoton,npct75x" driver hence we added it in the device tree. If the 
driver doesn't have this compatibility string then it won't load. So if 
someone tries to use this board then tpm won't work unless the 
compatibility string is added in the driver. That is the dependency I am 
talking about.

Please let me know.

Regards,

Ninad

>
> Best regards,
> Krzysztof
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 8/8] ARM: dts: aspeed: System1: PS, sensor and more

[Krzysztof Kozlowski]

On 13/12/2023 20:49, Ninad Palsule wrote:
> Hello Krzysztof,
> 
> On 12/13/23 13:37, Krzysztof Kozlowski wrote:
>> On 13/12/2023 20:02, Ninad Palsule wrote:
>>> Hello Krzysztof,
>>>
>>> On 12/12/23 14:26, Krzysztof Kozlowski wrote:
>>>> On 12/12/2023 17:40, Ninad Palsule wrote:
>>>>> This drop adds following devices in the device tree.
>>>>> - EEPROM/VPD
>>>>> - Power supplies
>>>>> - Humidity, pressure and temperature sensors.
>>>>> - Trusted platform module(TPM) chip
>>>>>
>>>>> Tested:
>>>>>       This board is tested using the simics simulator.
>>>>>
>>>>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>>>>> ---
>>>> Don't mix DTS with drivers. DTS and drivers go via different subsystems
>>>> and cannot have dependencies, so why DTS is patch #6, then driver #7 and
>>>> now again DTS #7?
>>> There is a dependency on driver code as patch #8 uses the compatibility
>>> string added in driver patch #7.  I have now moved driver patch at the
>>> start. Is that ok? OR you are suggesting something else?
>> First, there is no dependency. Second, except confusing order anyway DTS
>> will go via separate trees. Third, again, there is no dependency. If
>> there is, your patchset is broken and this needs to be fixed. Although I
>> don't understand how new hardware can depend on driver... it's really odd.
> 
> Thanks for the quick response.
> 
> This board uses the nuvoton TPM device. The tpm devices uses 
> "nuvoton,npct75x" driver hence we added it in the device tree. If the 
> driver doesn't have this compatibility string then it won't load. So if 
> someone tries to use this board then tpm won't work unless the 

... and if there is no board it also fails to load.

> compatibility string is added in the driver. That is the dependency I am 
> talking about.

This is not a dependency! It's unrelated.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 8/8] ARM: dts: aspeed: System1: PS, sensor and more

[Ninad Palsule]

Hello Krzysztof,


On 12/14/23 01:24, Krzysztof Kozlowski wrote:
> On 13/12/2023 20:49, Ninad Palsule wrote:
>> Hello Krzysztof,
>>
>> On 12/13/23 13:37, Krzysztof Kozlowski wrote:
>>> On 13/12/2023 20:02, Ninad Palsule wrote:
>>>> Hello Krzysztof,
>>>>
>>>> On 12/12/23 14:26, Krzysztof Kozlowski wrote:
>>>>> On 12/12/2023 17:40, Ninad Palsule wrote:
>>>>>> This drop adds following devices in the device tree.
>>>>>> - EEPROM/VPD
>>>>>> - Power supplies
>>>>>> - Humidity, pressure and temperature sensors.
>>>>>> - Trusted platform module(TPM) chip
>>>>>>
>>>>>> Tested:
>>>>>>        This board is tested using the simics simulator.
>>>>>>
>>>>>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>>>>>> ---
>>>>> Don't mix DTS with drivers. DTS and drivers go via different subsystems
>>>>> and cannot have dependencies, so why DTS is patch #6, then driver #7 and
>>>>> now again DTS #7?
>>>> There is a dependency on driver code as patch #8 uses the compatibility
>>>> string added in driver patch #7.  I have now moved driver patch at the
>>>> start. Is that ok? OR you are suggesting something else?
>>> First, there is no dependency. Second, except confusing order anyway DTS
>>> will go via separate trees. Third, again, there is no dependency. If
>>> there is, your patchset is broken and this needs to be fixed. Although I
>>> don't understand how new hardware can depend on driver... it's really odd.
>> Thanks for the quick response.
>>
>> This board uses the nuvoton TPM device. The tpm devices uses
>> "nuvoton,npct75x" driver hence we added it in the device tree. If the
>> driver doesn't have this compatibility string then it won't load. So if
>> someone tries to use this board then tpm won't work unless the
> ... and if there is no board it also fails to load.
>
>> compatibility string is added in the driver. That is the dependency I am
>> talking about.
> This is not a dependency! It's unrelated.

ok, I will send it as a separate patch.

Thanks for the prompt reply.

Regards,

Ninad

>
> Best regards,
> Krzysztof
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 8/8] ARM: dts: aspeed: System1: PS, sensor and more

[Ninad Palsule]

Hello Krzysztof,

On 12/12/23 14:26, Krzysztof Kozlowski wrote:
> On 12/12/2023 17:40, Ninad Palsule wrote:
>> This drop adds following devices in the device tree.
>> - EEPROM/VPD
>> - Power supplies
>> - Humidity, pressure and temperature sensors.
>> - Trusted platform module(TPM) chip
>>
>> Tested:
>>      This board is tested using the simics simulator.
>>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
> Don't mix DTS with drivers. DTS and drivers go via different subsystems
> and cannot have dependencies, so why DTS is patch #6, then driver #7 and
> now again DTS #7?
I have sent a driver patch as a separate patchset and removed it from 
this patchset.
>
>
>>   .../dts/aspeed/aspeed-bmc-ibm-system1.dts     | 76 +++++++++++++++++++
>>   1 file changed, 76 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
>> index 75562aa63701..d960b938fe8d 100644
>> --- a/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
>> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-system1.dts
>> @@ -461,6 +461,11 @@ &kcs3 {
>>   &i2c0 {
>>   	status = "okay";
>>   
>> +	eeprom@50 {
>> +		compatible = "atmel,24c64";
>> +		reg = <0x50>;
>> +	};
>> +
>>   	regulator@60 {
>>   		compatible = "maxim,max8952";
>>   		reg = <0x60>;
>> @@ -655,6 +660,25 @@ pca0: pca9539@74 {
>>   
>>   &i2c2 {
>>   	status = "okay";
>> +
>> +	power-supply@58 {
>> +		compatible = "ibm,cffps";
>> +		reg = <0x58>;
>> +	};
>> +
>> +	power-supply@59 {
>> +		compatible = "ibm,cffps";
>> +		reg = <0x59>;
>> +	};
>> +
>> +	power-supply@5a {
>> +		compatible = "ibm,cffps";
>> +		reg = <0x5a>;
>> +	};
> Missing blank line

Fixed it.

Thanks for the review.

Regards,

Ninad

>
>> +	power-supply@5b {
>> +		compatible = "ibm,cffps";
>> +		reg = <0x5b>;
>> +	};
>
> Best regards,
> Krzysztof
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel