Re: [PATCH] ARM: dts: sun8i: h2+: Enable optional SPI flash on Orange Pi Zero board

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 5553 bytes --]

On Thu, Oct 08, 2020 at 07:40:44PM +0200, Michal Suchánek wrote:
> On Thu, Oct 08, 2020 at 07:14:54PM +0200, Maxime Ripard wrote:
> > On Thu, Oct 08, 2020 at 06:02:19PM +0200, Michal Suchánek wrote:
> > > On Thu, Oct 08, 2020 at 05:13:15PM +0200, Maxime Ripard wrote:
> > > > Hi,
> > > > 
> > > > On Tue, Sep 29, 2020 at 10:30:25AM +0200, Michal Suchanek wrote:
> > > > > The flash is present on all new boards and users went out of their way
> > > > > to add it on the old ones.
> > > > > 
> > > > > Enabling it makes a more reasonable default.
> > > > > 
> > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > > ---
> > > > >  arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> > > > > index f19ed981da9d..061d295bbba7 100644
> > > > > --- a/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> > > > > +++ b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> > > > > @@ -163,8 +163,8 @@ &ohci1 {
> > > > >  };
> > > > >  
> > > > >  &spi0 {
> > > > > -	/* Disable SPI NOR by default: it optional on Orange Pi Zero boards */
> > > > > -	status = "disabled";
> > > > > +	/* Enable optional SPI NOR by default */
> > > > > +	status = "okay";
> > > > >  
> > > > >  	flash@0 {
> > > > >  		#address-cells = <1>;
> > > > 
> > > > Unfortunately, it's optional, so there's really no reason to enable it
> > > > all the time. If it's troublesome to users, then the distros or vendors
> > > > should make the changes necessary to the hardware, bootloader or their
> > > > documentation to make it easier for those users.
> > > 
> > > I don't understand the reasoning. Why must it be disabled when optional?
> > 
> > Think about it the other way around. If we enable everything that is
> > optional, we're going to have a multitude of conflicts everywhere, and
> > without a clear decision as to who is "best" and thus how we should
> > resolve it.
> Conflicts with what?
> 
> The SPI0 bus is routed the the flash memory pads. Either there is the
> flash mounted or there are free pads. Nothing else on the board uses
> these pins. You could possily solder something else there but that's
> definitely not part of the board.
> > 
> > On a separate platform, recently I've been using a VGA bridge for the
> > RaspberryPi that takes the UART pins as well. It's definitely optional,
> > should I enable it by default? At the same time, enabling by default the
> > UART is just as arbitrary and will result in people using the VGA bridge
> > to complain about their regression (rightfully so).
> 
> That's completely different situation. That bridge is probably not even
> part of the board.
> 
> > 
> > So, really, if it's optional, it means that it not always there. If it's
> > not always there, it's meant to be supported by an overlay.
> > 
> > > By the same reasoning there is no reason to disable it all the time.
> > 
> > I'm not sure I follow you here. The least common denominator is that
> > it's not there, so it's not enabled.
> 
> You have two options - have a flash mounted or not. You ask why enable
> flash when it is not always present. By the same logic I can ask why
> disable it when it is not always absent. Enabling is the more useful
> option because it degrades gracefully in the case it is not present. It
> does not work the other way around.
> 
> > 
> > > Also the boards that do not have the flsh are either broken or
> > > obsolete.
> > 
> > Making general statements without arguments doesn't really make it true
> > though. Plenty of boards to have flash and are neither broken nor
> > obsolete.
>
> Cannot parse this.

"Plenty of boards do not have flash and are neither broken nor obsolete"

> > 
> > > So most of the time enabling the flash chip is the right thing.
> > > 
> > > Or do we need two DTBs like sun8i-h2-plus-orangepi-zero.dts and
> > > sun8i-h2-plus-orangepi-zero-no-spi-nor.dts
> > 
> > No, you need sun8i-h2-plus-orangepi-zero plus an overlay for the
> > SPI-NOR.
>
> The flash is part of the board.

Not always though.

> There is no need for an overlay.

Overlays are here to deal with the "not always though" situation...

> And overlays don't exist.

If you want to believe that, please go ahead.

But there's support for it in libfdt, and you can either apply them
directly through the U-Boot command line, or bundle them in a FIT image.

Plenty of support for something that doesn't exist.

> > 
> > > There is no way to change the setting on a runnig system, the pins are
> > > routed to the flash pads anyway so are not usable for anything else. The
> > > only thing that happens on boards that do not have the flash is kernel
> > > probing it and complaining that the ID 00 00 00 is not valid SPI NOR
> > > flash memory ID.
> > 
> > We have people reporting bugs about completely innocuous error messages
> > without any side effects already. An error about a missing or broken
> > storage device will surely raise some eyebrows.
> 
> I am sure poeple who have an old Orange Pi Zero know that it is missing
> the flash memory.

Do you really expect every single user on an Orange Pi Zero to know
exactly the revision it has and whether or not it's been bundled with a
SPI flash if it's the older ones (with the proper definition of older
ones)?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]