Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <5944ba0ce568eaf507917799b1dfd89a3d0ca492.camel@ew.tq-group.com>
Subject: Re: [PATCH v2 1/2] arm64: dts: ti: k3-am65: disable optional
 peripherals by default
From:   Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
To:     Tony Lindgren <tony@atomide.com>
Cc:     Rob Herring <robh+dt@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
        Olof Johansson <olof@lixom.net>, soc@kernel.org,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Tero Kristo <kristo@kernel.org>, jan.kiszka@siemens.com,
        linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, Nishanth Menon <nm@ti.com>
Date:   Mon, 07 Feb 2022 09:45:29 +0100
In-Reply-To: <YgDCLaBHA3DDQAUd@atomide.com>
References: <20220203140240.973690-1-matthias.schiffer@ew.tq-group.com>
         <20220204143108.653qk2ihnlhsr5aa@prior> <YgDCLaBHA3DDQAUd@atomide.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5-0ubuntu1 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 2022-02-07 at 08:54 +0200, Tony Lindgren wrote:
> Hi,
> 
> * Nishanth Menon <nm@ti.com> [220204 14:30]:
> > Rob, Tony, Arnd, SoC maintainers,
> > 
> > On 15:02-20220203, Matthias Schiffer wrote:
> > > All peripherals that require pinmuxing or other configuration to
> > > work
> > > should be disabled by default. Dependent DTS are adjusted
> > > accordingly.
> 
> Disabling SoC internal devices by default is not a good policy. The
> devices are available even if not pinned out. Disabling device by
> default
> causes runtime PM to not work as the kernel will completely ignore
> the
> disabled devices. And this means you add a dependency to some certain
> version of a bootloader for PM to work.
> 
> Additionally tagging devices as disabled by default (and then again
> re-enabling them in the board specific dts files) is just pointless
> churn and bloat. See for example commit 12afc0cf8121 ("ARM: dts: Drop
> pointless status changing for am3 musb") :)
> 
> If you really want to disable some devices for memory usage or other
> reasons, do it in the board specific dts files.

Attempting to use unmuxed peripherals often leads to ugly errors - I2C
without pullups appearing busy, UARTs in endless break condition, ...
Such errors are often seen as defects in hardware or software by people
who aren't familiar with the internals.

I can see the issue with bootloaders leaving peripherals in an unknown
state, but I'm not happy with keeping such devices enabled in the
kernel either.

Generally I think that it's a bootloader's responsiblity to disable
unneeded devices - the kernel may not even have a driver for some
peripherals, leading to the same behaviour as a "disabled" status. For
this reason I believe that it should always be okay to set unneeded
devices to "disabled", and it should be considered a safe default.

I'm not sure what the consensus on these issues is. I'm more familiar
with NXP's i.MX and Layerscape SoCs, where it's common to have all
muxable peripherals set to "disabled" in the base DTSI, and a quick
grep through a few dts directories gives me the impression that this is
the case for most other vendors as well.

Regards,
Matthias


> 
> > https://lore.kernel.org/linux-arm-kernel/20201112183538.6805-1-nm@ti.com/
> > reversal all over again.
> > 
> > Is there a specific pattern we are intending to use here? Because,
> > if we
> > are going down this path (which would be a major churn across
> > multiple
> > downstream trees as well) - I'd rather have this as a documented
> > standard and not just a TI approach and will need to be done across
> > all
> > K3 devices.
> > 
> > Are you aware of such a documented guideline, rather than "word of
> > mouth"? Maybe I have'nt looked deep enough, but checking..
> 
> For SoCs that don't implement runtime PM the policy can be different
> without causing any harm. But for any SoCs implementing runtime PM,
> an
> unknown state from the bootloader is not going to work.
> 
> Regards,
> 
> Tony