Re: [PATCH] ARM: dts: am335x: Add rtc node as system-power-controller

From: Johan Hovold <johan@kernel.org>
To: Drew Fustini <dfustini@baylibre.com>
Cc: Benoit Cousson <bcousson@baylibre.com>,
	Dave Gerlach <d-gerlach@ti.com>, Keerthy <j-keerthy@ti.com>,
	Tony Lindgren <tony@atomide.com>,
	Rob Herring <robh+dt@kernel.org>,
	linux-omap@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ARM: dts: am335x: Add rtc node as system-power-controller
Date: Thu, 14 Oct 2021 12:42:26 +0200	[thread overview]
Message-ID: <YWgJkkJ4LuAJhLmk@hovoldconsulting.com> (raw)
In-Reply-To: <20211013214103.GA10628@x1>

On Wed, Oct 13, 2021 at 02:41:03PM -0700, Drew Fustini wrote:
> On Wed, Oct 13, 2021 at 09:23:42AM +0200, Johan Hovold wrote:
> > On Tue, Oct 12, 2021 at 12:13:12PM -0700, Drew Fustini wrote:
> > > From: Keerthy <j-keerthy@ti.com>
> > > 
> > > PMIC_PWR_EN pin of RTC on am335x-evm, bone, and boneblack is connected to
> > > PMIC on board, so flag rtc node as system-power-controller to allow
> > > software to poweroff boards.
> > 
> > The "system-power-controller" property is already set in
> > bone-common.dtsi since
> > 
> > 	2876cc4a773c ("ARM: dts: Move most of am335x-boneblack.dts to am335x-boneblack-common.dtsi")
> > 
> > so this probably only affects am335x-evm and that should be reflected in
> > the commit message.
> > 
> > Also, should you now remove the property from boneblack-common? Or just
> > add it to am335x-evm instead?
> 
> Thank you for reviewing. Yes, I should improve the commit message as the
> BeagleBone Black is already covered for the rtc system-power-controller
> in am335x-boneblack-common.dtsi.  

So is sancloud-bbe apparently.

I only noticed because I added support to BeagleBone Black long ago so
unless there'd been a regression it should already be supported.

> I believe it would be ok to remove system-power-controller from 
> am335x-boneblack-common.dtsi and have it in am335x-bone-common.dtsi.
> 
> These are the files that include am335x-boneblack-common.dtsi:
> arch/arm/boot/dts/am335x-boneblack-wireless.dts
> arch/arm/boot/dts/am335x-boneblack.dts
> arch/arm/boot/dts/am335x-sancloud-bbe-lite.dts
> arch/arm/boot/dts/am335x-sancloud-bbe.dts
> 
> But they all also include am335x-bone-common.dtsi.
> 
> However, I just noticed that am335x-evm.dts does not include either
> am335x-boneblack-common.dtsi or am335x-boneblack-common.dtsi. Thus
> rtc system-power-controller should be directly inserted into
> am335x-evm.dts.

Right.

> I considered just moving system-power-controller to the rtc node in
> am33xx-l4.dtsi but I don't think that would be correct as this would not
> be valid for all am33xx devices.
> 
> Does that seem correct to you?

No, that wouldn't be right.

You're more familiar with the different variants here, but unless all
flavours of Bone Black have the signal wired, it should probably be
pushed down into the dts files again.

Johan