[v1] Revise Meta(Facebook) Harma BMC(AST2600)

[PATCH v1 3/4] ARM: dts: aspeed: harma: add sq52206 power monitor device

Posted by Peter Yin 4 months, 3 weeks ago

Add the SQ52206 power monitor device and reorder the sequence.

Signed-off-by: Peter Yin <peteryin.openbmc@gmail.com>
---
 .../dts/aspeed/aspeed-bmc-facebook-harma.dts  | 28 +++++++++++--------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-harma.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-harma.dts
index bcef91e6eb54..fe72d47a7632 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-harma.dts
+++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-harma.dts
@@ -353,14 +353,15 @@ eeprom@52 {
 		reg = <0x52>;
 	};
 
-	power-monitor@69 {
-		compatible = "pmbus";
-		reg = <0x69>;
+	power-monitor@40 {
+		compatible = "infineon,xdp710";
+		reg = <0x40>;
 	};
 
-	temperature-sensor@49 {
-		compatible = "ti,tmp75";
-		reg = <0x49>;
+	power-monitor@41 {
+		compatible = "silergy,sq52206";
+		reg = <0x41>;
+		shunt-resistor = <500>;
 	};
 
 	power-monitor@44 {
@@ -369,16 +370,21 @@ power-monitor@44 {
 		shunt-resistor-micro-ohms = <250>;
 	};
 
-	power-monitor@40 {
-		compatible = "infineon,xdp710";
-		reg = <0x40>;
-	};
-
 	power-monitor@45 {
 		compatible = "ti,ina238";
 		reg = <0x45>;
 		shunt-resistor = <500>;
 	};
+
+	power-monitor@69 {
+		compatible = "pmbus";
+		reg = <0x69>;
+	};
+
+	temperature-sensor@49 {
+		compatible = "ti,tmp75";
+		reg = <0x49>;
+	};
 };
 
 &i2c5 {
-- 
2.43.0

Re: [PATCH v1 3/4] ARM: dts: aspeed: harma: add sq52206 power monitor device

Posted by Andrew Jeffery 4 months, 2 weeks ago

On Wed, 2025-09-17 at 18:18 +0800, Peter Yin wrote:
> Add the SQ52206 power monitor device and reorder the sequence.
> 
> Signed-off-by: Peter Yin <peteryin.openbmc@gmail.com>
> ---
>  .../dts/aspeed/aspeed-bmc-facebook-harma.dts  | 28 +++++++++++------
> --
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-harma.dts
> b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-harma.dts
> index bcef91e6eb54..fe72d47a7632 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-harma.dts
> +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-harma.dts
> @@ -353,14 +353,15 @@ eeprom@52 {
>  		reg = <0x52>;
>  	};
>  
> -	power-monitor@69 {
> -		compatible = "pmbus";
> -		reg = <0x69>;
> +	power-monitor@40 {
> +		compatible = "infineon,xdp710";
> +		reg = <0x40>;
>  	};
>  
> -	temperature-sensor@49 {
> -		compatible = "ti,tmp75";
> -		reg = <0x49>;
> +	power-monitor@41 {
> +		compatible = "silergy,sq52206";
> +		reg = <0x41>;
> +		shunt-resistor = <500>;
>  	};
>  
>  	power-monitor@44 {
> @@ -369,16 +370,21 @@ power-monitor@44 {
>  		shunt-resistor-micro-ohms = <250>;
>  	};
>  
> -	power-monitor@40 {
> -		compatible = "infineon,xdp710";
> -		reg = <0x40>;
> -	};
> -
>  	power-monitor@45 {
>  		compatible = "ti,ina238";
>  		reg = <0x45>;
>  		shunt-resistor = <500>;
>  	};
> +
> +	power-monitor@69 {
> +		compatible = "pmbus";

I realise you're just moving this node, but I'm surprised it hasn't
caused trouble otherwise. This happens to work due to a quirk of I2C
device IDs in the kernel but it's not a documented compatible.

Compatible strings need to represent the physical device. Can you
please split out a patch either dropping this node, or replacing the
compatible string with something appropriate?

Andrew

Re: [PATCH v1 3/4] ARM: dts: aspeed: harma: add sq52206 power monitor device

Posted by Peter Yin 4 months, 2 weeks ago

Andrew Jeffery <andrew@codeconstruct.com.au> 於 2025年9月22日 週一 上午11:36寫道：
>
> On Wed, 2025-09-17 at 18:18 +0800, Peter Yin wrote:
> > Add the SQ52206 power monitor device and reorder the sequence.
> >
> > Signed-off-by: Peter Yin <peteryin.openbmc@gmail.com>
> > ---
> >  .../dts/aspeed/aspeed-bmc-facebook-harma.dts  | 28 +++++++++++------
> > --
> >  1 file changed, 17 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-harma.dts
> > b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-harma.dts
> > index bcef91e6eb54..fe72d47a7632 100644
> > --- a/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-harma.dts
> > +++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-facebook-harma.dts
> > @@ -353,14 +353,15 @@ eeprom@52 {
> >               reg = <0x52>;
> >       };
> >
> > -     power-monitor@69 {
> > -             compatible = "pmbus";
> > -             reg = <0x69>;
> > +     power-monitor@40 {
> > +             compatible = "infineon,xdp710";
> > +             reg = <0x40>;
> >       };
> >
> > -     temperature-sensor@49 {
> > -             compatible = "ti,tmp75";
> > -             reg = <0x49>;
> > +     power-monitor@41 {
> > +             compatible = "silergy,sq52206";
> > +             reg = <0x41>;
> > +             shunt-resistor = <500>;
> >       };
> >
> >       power-monitor@44 {
> > @@ -369,16 +370,21 @@ power-monitor@44 {
> >               shunt-resistor-micro-ohms = <250>;
> >       };
> >
> > -     power-monitor@40 {
> > -             compatible = "infineon,xdp710";
> > -             reg = <0x40>;
> > -     };
> > -
> >       power-monitor@45 {
> >               compatible = "ti,ina238";
> >               reg = <0x45>;
> >               shunt-resistor = <500>;
> >       };
> > +
> > +     power-monitor@69 {
> > +             compatible = "pmbus";
>
> I realise you're just moving this node, but I'm surprised it hasn't
> caused trouble otherwise. This happens to work due to a quirk of I2C
> device IDs in the kernel but it's not a documented compatible.
>
> Compatible strings need to represent the physical device. Can you
> please split out a patch either dropping this node, or replacing the
> compatible string with something appropriate?
>
> Andrew

Ok, but this device BMR350 is not in the pmbus_id[] list.
I will add BMR350 to the pmbus_id[] list, and then fix the DTS
compatible string.