[v3] platform/astbmc: Do not delete compatible property

Message ID 20170402090040.18931-1-hegdevasant@linux.vnet.ibm.com
State Accepted
Headers show

Commit Message

Vasant Hegde April 2, 2017, 9 a.m.
P9 onwards OPAL is building device tree for BMC based system using
HDAT. We are populating bmc/compatible node with bmc version. Hence
do not delete this property.

CC: Jeremy Kerr <jk@ozlabs.org>
CC: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
---
 platforms/astbmc/common.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Vasant Hegde May 25, 2017, 6:25 a.m. | #1
On 04/02/2017 02:30 PM, Vasant Hegde wrote:
> P9 onwards OPAL is building device tree for BMC based system using
> HDAT. We are populating bmc/compatible node with bmc version. Hence
> do not delete this property.
>

Stewart,

Can you take a look of this patch?

-Vasant
Jeremy Kerr July 25, 2017, 5:31 a.m. | #2
Hi Vasant,

> P9 onwards OPAL is building device tree for BMC based system using
> HDAT. We are populating bmc/compatible node with bmc version. Hence
> do not delete this property.

ast_bmc_fixup_bmc_sensors() does a few things, but I think I see what
you're getting at here; this change looks good to me.

However, we should probably validate that the generated DT actually
looks OK before we do a release with this patch (and thus releasing
those compatible strings into the wild). To help with that, could you
provide a sample of the generated tree?

Any changes there would be to separate code though, so:

Acked-by: Jeremy Kerr <jk@ozlabs.org>

Cheers,


Jeremy
Vasant Hegde July 25, 2017, 9:39 a.m. | #3
On 07/25/2017 11:01 AM, Jeremy Kerr wrote:
> Hi Vasant,
>
>> P9 onwards OPAL is building device tree for BMC based system using
>> HDAT. We are populating bmc/compatible node with bmc version. Hence
>> do not delete this property.
>
> ast_bmc_fixup_bmc_sensors() does a few things, but I think I see what
> you're getting at here; this change looks good to me.
>
> However, we should probably validate that the generated DT actually
> looks OK before we do a release with this patch (and thus releasing
> those compatible strings into the wild). To help with that, could you
> provide a sample of the generated tree?

Jeremy,

Please find the BMC section of device tree. Let me know if you need full dts output.


	bmc {
		compatible = "ibm,ast2500,openbmc";
		#address-cells = <0x1>;
		#size-cells = <0x0>;
		phandle = <0x2>;

		sensors {
			#address-cells = <0x1>;
			#size-cells = <0x0>;
			phandle = <0x3>;

			sensor@a7 {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x19>;
				reg = <0xa7>;
				ipmi-sensor-type = <0x1>;
			};

			sensor@ae {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x20>;
				reg = <0xae>;
				ipmi-sensor-type = <0xc>;
			};

			sensor@0 {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x14>;
				reg = <0x0>;
				ipmi-sensor-type = <0xcc>;
			};

			sensor@a5 {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x17>;
				reg = <0xa5>;
				ipmi-sensor-type = <0x1>;
			};

			sensor@ac {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x1e>;
				reg = <0xac>;
				ipmi-sensor-type = <0xc>;
			};

			sensor@5a {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x16>;
				reg = <0x5a>;
				ipmi-sensor-type = <0x7>;
			};

			sensor@9 {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x12>;
				reg = <0x9>;
				ipmi-sensor-type = <0xc8>;
			};

			sensor@b3 {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x21>;
				reg = <0xb3>;
				ipmi-sensor-type = <0x1>;
			};

			sensor@aa {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x1c>;
				reg = <0xaa>;
				ipmi-sensor-type = <0xc>;
			};

			sensor@7 {
				compatible = "ibm,ipmi-sensor";
				phandle = <0xf>;
				reg = <0x7>;
				ipmi-sensor-type = <0xc3>;
			};

			sensor@e {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x6>;
				reg = <0xe>;
				ipmi-sensor-type = <0xc7>;
			};

			sensor@5 {
				compatible = "ibm,ipmi-sensor";
				phandle = <0xb>;
				reg = <0x5>;
				ipmi-sensor-type = <0x1f>;
			};

			sensor@c {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x4>;
				reg = <0xc>;
				ipmi-sensor-type = <0xc7>;
			};

			sensor@3 {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x9>;
				reg = <0x3>;
				ipmi-sensor-type = <0xf>;
			};

			sensor@a {
				compatible = "ibm,ipmi-sensor";
				phandle = <0xd>;
				reg = <0xa>;
				ipmi-sensor-type = <0xc1>;
			};

			sensor@a8 {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x1a>;
				reg = <0xa8>;
				ipmi-sensor-type = <0xc>;
			};

			sensor@1 {
				compatible = "ibm,ipmi-sensor";
				phandle = <0xa>;
				reg = <0x1>;
				ipmi-sensor-type = <0x12>;
			};

			sensor@a6 {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x18>;
				reg = <0xa6>;
				ipmi-sensor-type = <0xc>;
			};

			sensor@ad {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x1f>;
				reg = <0xad>;
				ipmi-sensor-type = <0x1>;
			};

			sensor@b4 {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x22>;
				reg = <0xb4>;
				ipmi-sensor-type = <0xc>;
			};

			sensor@ab {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x1d>;
				reg = <0xab>;
				ipmi-sensor-type = <0x1>;
			};

			sensor@8 {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x11>;
				reg = <0x8>;
				ipmi-sensor-type = <0xc6>;
			};

			sensor@f {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x7>;
				reg = <0xf>;
				ipmi-sensor-type = <0xc7>;
			};

			sensor@6 {
				compatible = "ibm,ipmi-sensor";
				phandle = <0xe>;
				reg = <0x6>;
				ipmi-sensor-type = <0xc2>;
			};

			sensor@d {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x5>;
				reg = <0xd>;
				ipmi-sensor-type = <0xc7>;
			};

			sensor@59 {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x15>;
				reg = <0x59>;
				ipmi-sensor-type = <0x1>;
			};

			sensor@10 {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x8>;
				reg = <0x10>;
				ipmi-sensor-type = <0xc7>;
			};

			sensor@4 {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x10>;
				reg = <0x4>;
				ipmi-sensor-type = <0xc4>;
			};

			sensor@b {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x13>;
				reg = <0xb>;
				ipmi-sensor-type = <0xca>;
			};

			sensor@a9 {
				compatible = "ibm,ipmi-sensor";
				phandle = <0x1b>;
				reg = <0xa9>;
				ipmi-sensor-type = <0x1>;
			};

			sensor@2 {
				compatible = "ibm,ipmi-sensor";
				phandle = <0xc>;
				reg = <0x

-Vasant
Jeremy Kerr July 26, 2017, 12:45 a.m. | #4
Hi Vasant,

> Please find the BMC section of device tree. Let me know if you need full
> dts output.

Thanks! What you've provided is sufficient.

>     bmc {
>         compatible = "ibm,ast2500,openbmc";

That looks like a pretty strange compatible value; it should be in the
format <vendor>,<device>...

Do you know who manages those inputs on the HDAT side?

Cheers,


Jeremy
Vasant Hegde July 26, 2017, 10:11 a.m. | #5
On 07/26/2017 06:15 AM, Jeremy Kerr wrote:
> Hi Vasant,
>

Jeremy,

.../...


>
>>     bmc {
>>         compatible = "ibm,ast2500,openbmc";
>
> That looks like a pretty strange compatible value; it should be in the
> format <vendor>,<device>...

They use vendor,hwname,swname format.

>
> Do you know who manages those inputs on the HDAT side?

Venkatesh (in CC) manages these data.

-Vasant
Vasant Hegde Aug. 16, 2017, 4:43 a.m. | #6
On 07/26/2017 06:15 AM, Jeremy Kerr wrote:
> Hi Vasant,
>
>> Please find the BMC section of device tree. Let me know if you need full
>> dts output.
>
> Thanks! What you've provided is sufficient.

Jeremy ,

Can you Ack this patch as compatible property is not related to this patch?


>
>>     bmc {
>>         compatible = "ibm,ast2500,openbmc";
>

If we really want to fix this then we have to ask HDAT folks to split above 
property to something like device and software. Alternatively we can fix it in 
OPAL as well.

-Vasant
Vasant Hegde Nov. 29, 2017, 12:58 p.m. | #7
On 04/02/2017 02:30 PM, Vasant Hegde wrote:
> P9 onwards OPAL is building device tree for BMC based system using
> HDAT. We are populating bmc/compatible node with bmc version. Hence
> do not delete this property.

Stewart,

Looks like this one is not yet merged. Can you please pull this patch?

-Vasant
Stewart Smith May 24, 2018, 11:02 p.m. | #8
Vasant Hegde <hegdevasant@linux.vnet.ibm.com> writes:
> P9 onwards OPAL is building device tree for BMC based system using
> HDAT. We are populating bmc/compatible node with bmc version. Hence
> do not delete this property.
>
> CC: Jeremy Kerr <jk@ozlabs.org>
> CC: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>

This does get us properties like this though:
compatible       "ibm,ast2500,supermicro"
compatible       "ibm,ast2500,openbmc"

which... are certainly wrong. This is, however, what HDAT is giving us,
so arguably this is a HDAT bug.

In fact, this even violates the HDAT spec, which says:

P9 IBM systems with FSP: ibm,fsp2,fips
P9 IBM systems with BMC: ibm,ast2500,openbmc
P9 IBM systems with SuperMicro: supermicro,ast2500,aten

which is not what we get, we seem to always get what I pasted above (for
p9dsu and witherspoon respectively).

Soo... what should we do about that? While the second comma in the
compatible property is odd, it doesn't seem to violate the DT spec.


I'm okay with merging this as-is, as long as we fix up the oddities in a
subsequent patch though. So, it's in master as of
bb0079ea8490cbc138b9d14da30051cc0dabd381
Vasant Hegde May 25, 2018, 5:52 a.m. | #9
On 05/25/2018 04:32 AM, Stewart Smith wrote:
> Vasant Hegde <hegdevasant@linux.vnet.ibm.com> writes:
>> P9 onwards OPAL is building device tree for BMC based system using
>> HDAT. We are populating bmc/compatible node with bmc version. Hence
>> do not delete this property.
>>
>> CC: Jeremy Kerr <jk@ozlabs.org>
>> CC: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
>> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> 
> This does get us properties like this though:
> compatible       "ibm,ast2500,supermicro"
> compatible       "ibm,ast2500,openbmc"
> 
> which... are certainly wrong. This is, however, what HDAT is giving us,
> so arguably this is a HDAT bug.

Yes. Fix should come from HDAT/xml.

> 
> In fact, this even violates the HDAT spec, which says:
> 
> P9 IBM systems with FSP: ibm,fsp2,fips
> P9 IBM systems with BMC: ibm,ast2500,openbmc
> P9 IBM systems with SuperMicro: supermicro,ast2500,aten
Yes. This is wrong.

@Nagendra, Can you fix this one in HDAT/xml?


> 
> which is not what we get, we seem to always get what I pasted above (for
> p9dsu and witherspoon respectively).
> 
> Soo... what should we do about that? While the second comma in the
> compatible property is odd, it doesn't seem to violate the DT spec.

Yeah. But that's what we get.

-Vasant

Patch

diff --git a/platforms/astbmc/common.c b/platforms/astbmc/common.c
index 6f29e61..3fad2d4 100644
--- a/platforms/astbmc/common.c
+++ b/platforms/astbmc/common.c
@@ -336,7 +336,8 @@  static void astbmc_fixup_dt(void)
 	   make sure we have one. */
 	astbmc_fixup_dt_system_id();
 
-	astbmc_fixup_bmc_sensors();
+	if (proc_gen == proc_gen_p8)
+		astbmc_fixup_bmc_sensors();
 }
 
 static void astbmc_fixup_psi_bar(void)