diff mbox series

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

Message ID 20231212164004.1683589-9-ninad@linux.ibm.com
State Superseded, archived
Headers show
Series Add device tree for IBM system1 BMC | expand

Commit Message

Ninad Palsule Dec. 12, 2023, 4:40 p.m. UTC
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(+)

Comments

Krzysztof Kozlowski Dec. 12, 2023, 8:26 p.m. UTC | #1
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
Ninad Palsule Dec. 13, 2023, 7:02 p.m. UTC | #2
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
>
Krzysztof Kozlowski Dec. 13, 2023, 7:37 p.m. UTC | #3
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
Ninad Palsule Dec. 13, 2023, 7:49 p.m. UTC | #4
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
>
Krzysztof Kozlowski Dec. 14, 2023, 7:24 a.m. UTC | #5
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
Ninad Palsule Dec. 14, 2023, 2:24 p.m. UTC | #6
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
>
Ninad Palsule Dec. 14, 2023, 3:04 p.m. UTC | #7
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
>
diff mbox series

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 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>;
+			};
 		};
 	};
 };