diff mbox series

ARM: dts: am335x: Add rtc node as system-power-controller

Message ID 20211012191311.879838-1-dfustini@baylibre.com (mailing list archive)
State New, archived
Headers show
Series ARM: dts: am335x: Add rtc node as system-power-controller | expand

Commit Message

Drew Fustini Oct. 12, 2021, 7:13 p.m. UTC 
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.

Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
Signed-off-by: Keerthy <j-keerthy@ti.com>
Signed-off-by: Drew Fustini <dfustini@baylibre.com>
---
 arch/arm/boot/dts/am335x-bone-common.dtsi | 1 +
 1 file changed, 1 insertion(+)

Comments

Johan Hovold Oct. 13, 2021, 7:23 a.m. UTC | #1 
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?

> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> Signed-off-by: Drew Fustini <dfustini@baylibre.com>
> ---
>  arch/arm/boot/dts/am335x-bone-common.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/boot/dts/am335x-bone-common.dtsi b/arch/arm/boot/dts/am335x-bone-common.dtsi
> index 0ccdc7cd463b..56ae5095a5b8 100644
> --- a/arch/arm/boot/dts/am335x-bone-common.dtsi
> +++ b/arch/arm/boot/dts/am335x-bone-common.dtsi
> @@ -399,6 +399,7 @@ &sham {
>  &rtc {
>  	clocks = <&clk_32768_ck>, <&clk_24mhz_clkctrl AM3_CLK_24MHZ_CLKDIV32K_CLKCTRL 0>;
>  	clock-names = "ext-clk", "int-clk";
> +	system-power-controller;
>  };
>  
>  &pruss_tm {

Johan
Drew Fustini Oct. 13, 2021, 9:41 p.m. UTC | #2 
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.  

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.

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?

Thank you,
Drew
Johan Hovold Oct. 14, 2021, 10:42 a.m. UTC | #3 
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
Drew Fustini Oct. 16, 2021, 5:04 a.m. UTC | #4 
On Thu, Oct 14, 2021 at 10:12:48AM -0400, Jason Kridner wrote:
> On Thu, Oct 14, 2021 at 6:43 AM Johan Hovold <johan@kernel.org> wrote:
> 
> > 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.
> >
> 
> I believe anything "bone" is going to have the same RTC power
> configuration. I believe this could be inconsistent at the AM335x level.

Thanks for the input Jason and Johan.

These are the dts files that currently include either 
am335x-bone-common.dtsi or am335x-boneblack-common.dtsi:

    am335x-bone.dts  
	am335x-bone-common.dtsi

    am335x-boneblack.dts
        am335x-bone-common.dtsi
	am335x-boneblack-common.dtsi [rtc system-power-controller]

    am335x-boneblack-wireless.dts
	am335x-bone-common.dtsi
	am335x-boneblack-common.dtsi [rtc system-power-controller]

    am335x-bonegreen.dts
	am335x-bone-common.dtsi

    am335x-bonegreen-wireless.dts
	am335x-bone-common.dtsi

    am335x-sancloud-bbe.dts
	am335x-bone-common.dtsi
	am335x-boneblack-common.dtsi [rtc system-power-controller]

    am335x-sancloud-bbe-lite.dts
	am335x-bone-common.dtsi
	am335x-boneblack-common.dtsi [rtc system-power-controller]

am335x-boneblack.dts, am335x-boneblack-wireless.dts,
am335x-sancloud-bbe.dts, and am335x-sancloud-bbe-lite.dts already have
the rtc system-power-controller through am335x-boneblack-common.dtsi.

Moving rtc system-power-controller from am335x-boneblack-common.dtsi to
am335x-bone-common.dtsi would have no change for those boards as they
also include am335x-bone-common.dtsi.

It would add system-power-controller to am335x-bone.dts,
am335x-bonegreen.dts, and am335x-bonegreen-wireless.dts.

The original bone, green and green wireless have PMIC_POWR_EN (ZCZ C6)
connected to PWR_EN on the TPS65217B PMIC. Thus system-power-controller
should be valid for them too.

I will make new patch series that:

  * removes system-power-controller from am335x-boneblack-common.dtsi 
  * adds system-power-controller to am335x-bone-common.dtsi
  * adds system-power-controller to am335x-evm.dts, am335x-icev2.dts,
    am335x-icev2-prueth.dts

However, am335x-evmsk.dts should not have system-power-controller as
PMIC_POWR_EN is not connected in that board design.

Thanks,
Drew
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/am335x-bone-common.dtsi b/arch/arm/boot/dts/am335x-bone-common.dtsi
index 0ccdc7cd463b..56ae5095a5b8 100644
--- a/arch/arm/boot/dts/am335x-bone-common.dtsi
+++ b/arch/arm/boot/dts/am335x-bone-common.dtsi
@@ -399,6 +399,7 @@  &sham {
 &rtc {
 	clocks = <&clk_32768_ck>, <&clk_24mhz_clkctrl AM3_CLK_24MHZ_CLKDIV32K_CLKCTRL 0>;
 	clock-names = "ext-clk", "int-clk";
+	system-power-controller;
 };
 
 &pruss_tm {