Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1356e93cd5b101c3d896e35250c66959ed631544.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, 28 Feb 2022 11:30:50 +0100
In-Reply-To: <YgJZbdOlazrde7O/@atomide.com>
References: <20220203140240.973690-1-matthias.schiffer@ew.tq-group.com>
         <20220204143108.653qk2ihnlhsr5aa@prior> <YgDCLaBHA3DDQAUd@atomide.com>
         <5944ba0ce568eaf507917799b1dfd89a3d0ca492.camel@ew.tq-group.com>
         <YgEBml9HvFzSl289@atomide.com>
         <9923df6525212389b86cb635624bcfb5c27a8bc5.camel@ew.tq-group.com>
         <YgJZbdOlazrde7O/@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 Tue, 2022-02-08 at 13:52 +0200, Tony Lindgren wrote:
> * Matthias Schiffer <matthias.schiffer@ew.tq-group.com> [220208
> 10:53]:
> > On Mon, 2022-02-07 at 13:25 +0200, Tony Lindgren wrote:
> > > * Matthias Schiffer <matthias.schiffer@ew.tq-group.com> [220207
> > > 08:45]:
> > > > 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.
> > > 
> > > Not possible, think kexec for example :) How would the previous
> > > kernel
> > > even know what to disable if Linux has no idea about the devices?
> > 
> > Well, optimally, bootloader and all kernels would agree on the
> > devices
> > that are actually available, but I get your point.
> > 
> > > If there are issues you're seeing, it's likely a bug in some of
> > > the
> > > device drivers for not checking for the necessary resources like
> > > pinctrl for i2c lines.
> > 
> > I don't think it's common for individual drivers to care about
> > pinctrl
> > unless switching between different pin settings is required at
> > runtime.
> > Many drivers can be used on different hardware, some of which may
> > require pinmuxing, while others don't.
> 
> Yeah that's true, some configurations only do pin muxing in the
> bootloader. So pins are not a good criteria for devicetree status
> enabled
> for when the device is operational.
> 
> Probably a better criteria for devicetree "operational" status is the
> device can be clocked and configured or idled. Some devices like GPUs
> can render to memory with no external pin configuration for example.
> 

I don't think any properties currently exist that could or should be
used to decide whether a device is operational. Clocks etc. are usually
internal to the SoC and thus already set in the SoC DTSI. Pins and
power supplies may be specific to a mainboard, but can also be
optional. Whether an I2C bus can be operational may solely depend on
whether external pullups are connected to the pins or not.

The idea of an "incomplete" status like you mention below sounds better
to me. I also thought about adding something like a "probe_disabled()"
that is called instead of probe() for "disabled" devices, but I assume
that would cause a boot time penalty on systems that have many
"disabled" devices and don't actually need this...


> Following Linux running on a PC analogy.. If ACPI has some device
> that
> causes driver warnings on Linux boot, do we patch the ACPI table and
> pretend the device does not exist? Or do we patch the device driver
> to
> deal with the random buggy bootloader state for the device? :)
> 
> > Also, what is the expected behavior of a driver that is probed for
> > an
> > unusable device? Wouldn't this require some as-of-yet nonexisting
> > status between "okay" and "disabled" that conveys something like
> > "probe
> > this device, initialize (and disable) PM, but don't register
> > anything",
> > so no unusable devices become visible to userspace (and possibly
> > other
> > kernel drivers)?
> 
> I did some experimental patches several years ago to add devicetree
> status for incomplete, but eventually came to the conclusion that it
> was not really needed. Feel free to revisit that if you have the
> spare cycles :)
> 
> Having the drivers check for the resources like clocks and then just
> idle the device after probe solved the issues I was seeing for
> warnings
> and kexec. In some cases the device may need to be reset or at least
> properly reconfigured in the probe as the state can be unknown from
> the
> bootloader. That's about all there is to it. Sure you could save some
> memory with less instances for some devices, so maybe the status =
> "incomplete" could be used to do the trick for that.

I don't really care about memory usage. What I do care about is that
incorrect userspace usage doesn't cause ugly kernel warnings (for
example timeouts for i2cdetect on unmuxed bus) when we can avoid it,
because such issues always lead to support requests.

Not being able to hide non-operational devices from userspace feels
like a regression from older hardware.

> 
> > > > 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.
> > > 
> > > This approach only works for SoCs that don't need the kernel to
> > > idle
> > > devices for runtime PM.
> > 
> > I'm pretty sure that most modern SoCs I looked at have runtime PM,
> > and
> > it is simply expected that unusable devices are never enabled in
> > the
> > first place, so there is no need for the kernel to know about them.
> 
> Yeah well that assumption is the difference in getting runtime PM to
> work in a sane way across multiple SoCs and devices :)
> 
> Devices tagged with status = "disabled" are completely ignored by the
> kernel. Interconnect and bus related code may not know the details on
> how to reset and idle the child devices. Relying on firmware to do
> the
> reset and idle of unused devices may be too generic, can be buggy,
> and
> probably depends on the firmware revision.

Well, so far it seems like the `status = "disabled"` is just being
pushed from the SoC DTSIs to the board DTSs on TI hardware. For the
AM64 platform (which is fairly similar to the AM65), both mainboards
that currently exist disable unused UARTs, I2C/SPI busses, PWMs, ...
(Some of these might be disabled to make them usable from the R5/M4
cores, but I don't think that the case for all of them - "reserved"
would be more appropriate than "disabled" in these cases anyways)

AFAICT, disabling non-operatational devices in the board DTS instead of
the SoC DTSI is worse than the alternatives in every way:

- Verbose board DTS: You have to think about all the devices that exist
in the SoC, not just the ones you want to use
- Adding new nodes without `status = "disabled" to SoC DTSI can
potentially cause issues on dependent boards
- It doesn't solve the issues that not having `status = "disabled"` in
the DTSI is supposed to solve

Regards,
Matthias


> 
> Regards,
> 
> Tony