Re: [PATCH v6 11/11] arm64: dts: mt7986: add BPI-R3 nand/nor overlays

[Rob Herring]

On Sat, Nov 19, 2022 at 08:19:52AM +0100, Frank Wunderlich wrote:
> Am 8. November 2022 15:45:49 MEZ schrieb Rob Herring <robh+dt@xxxxxxxxxx>:
> >On Fri, Nov 18, 2022 at 4:05 PM Frank Wunderlich
> ><frank-w@xxxxxxxxxxxxxxx> wrote:
> >>
> >> Am 18. November 2022 22:39:52 MEZ schrieb Rob Herring <robh+dt@xxxxxxxxxx>:
> >> >On Fri, Nov 18, 2022 at 1:01 PM Frank Wunderlich <linux@xxxxxxxxx> wrote:
> >> >>
> >> >> From: Frank Wunderlich <frank-w@xxxxxxxxxxxxxxx>
> >> >>
> >> >> Add devicetree overlays for using nand and nor on BPI-R3.
> >> >
> >> >Can you not tell at runtime which one you booted from? If not, how
> >> >does one choose which overlay to apply? If you can, why not populate
> >> >both nodes and enable the right one? IMO, if all h/w is present, it
> >> >should all be in the DT. Selecting what h/w to use is a separate
> >> >problem and overlays aren't a great solution for that.
> >>
> >> It is not the decision about bootdevice,more available devices.
> >>
> >> Only 1 spi device (nand OR nor) is available
> >> at boottime as they share same spi bus and
> >> chipselect is set via hw jumper.
> >> Both nodes have reg 0,which is imho not
> >> supported in linux.
> >
> >As long as one is set to disabled, it should be fine.
> >
> >
> >> I choosed overlays to add the right spi
> >> device on the right mmc device where
> >> similar selection happens (see patch 10).
> >> Either sd OR emmc can be used (1 mmc
> >> controller,first 4bits from bus switched by
> >> hardware jumper).But for mmc i use it as
> >> base fdt because i see mmc as primary
> >> device which holds rootfs too. Nand/nor is
> >> imho helping device for accessing emmc or
> >> like rescue system (only uboot).
> >
> >No way to read the jumper state or know what you booted from I gues?
> >
> >> I probe in uboot if emmc is available (mmc
> >>  partconf) and choose emmc else sd. For
> >>  spi i try with sf command to check for nor,if
> >>  this does not work i apply nand overlay.
> >
> >Instead of applying overlays, wouldn't just changing 'status' be easier?
> 
> It will be easier,but requires dts for all
>  combinations,we have have sd/emmc
>  combination twice (once for nand
>  enabling,once for nor) and we have then 4
>  full dts instead of smaller overlays in fit.

No, I mean can't you have 1 dtb with everything, but nand, nor, emmc, 
and sd are all disabled. Then at boot change 'status' for what's 
enabled.


> So i should add spi subnodes both disabled
>  in base dtsi and create 4 dts (sd-nand,sd-nor,emmc-nand,emmc-nor) with
>  mmc node and enabling the right spi node?
> >>
> >> >> Signed-off-by: Frank Wunderlich <frank-w@xxxxxxxxxxxxxxx>
> >> >> ---
> >> >> maybe rename to dtso?
> >> >>
> >> >> "kbuild: Allow DTB overlays to built from .dtso named source files"
> >> >> https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/commit/?h=dt/next&id=363547d2191cbc32ca954ba75d72908712398ff2
> >>
> >> Should i do this?
> >
> >Yes. .dts -> .dtbo is going to be removed.
> 
> Do this if still using overlays,will test new way.
> 
> Maybe we can apply parts 1-9 first?

Sure.

> 
> >> >> more comments about the dt overlay-support:
> >> >>
> >> >> https://patchwork.kernel.org/comment/25092116/
> >> >> https://patchwork.kernel.org/comment/25085681/
> >>
> >> Daniel suggest define sd/emmc as overlay too...with way you mention below we could create 4 full fdt without applying overlays in uboot.
> >
> >Yes, but if you are going to do that, then you can just do all this
> >with includes.
> 
> This is a third way if i understand correctly
> 
> Make all of them as overlay (dtso?) but
>  build dtb by combining them in makefile.
> 
> This looks the best way because it avoids
>  redundand code for mmc node and allows
>  my current spi config (not the status way
>  which may break due to same unit address).
> 
> I guess my base dtsi is then a dts too?

Yes, it can be.

> 
> Or should these overlays only duplicated and either include sd dts or emmc dts (but this creates again redundant code)?
> >> >> --- a/arch/arm64/boot/dts/mediatek/Makefile
> >> >> +++ b/arch/arm64/boot/dts/mediatek/Makefile
> >> >> @@ -8,6 +8,8 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt6797-x20-dev.dtb
> >> >>  dtb-$(CONFIG_ARCH_MEDIATEK) += mt7622-rfb1.dtb
> >> >>  dtb-$(CONFIG_ARCH_MEDIATEK) += mt7622-bananapi-bpi-r64.dtb
> >> >>  dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-emmc.dtb
> >> >> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-nand.dtbo
> >> >> +dtb-$(CONFIG_ARCH_MEDIATEK) += mt7986a-bananapi-bpi-r3-nor.dtbo
> >> >
> >> >These need rules to apply them to the base dtb(s). You just need:
> >> >
> >> >full.dtb := base.dtb overlay.dtb
> >> >dtb-y += full.dtb
> >>
> >> I would prefer to do this in bootloader to allow all 4 possible configurations:
> >>
> >> Sd+nand
> >> Sd+nor
> >> Emmc+nand
> >> Emmc+nor
> >
> >That's fine. The purpose here is to document what the overlays apply
> >to, check that they actually apply, and validate them when applied
> >(unless someone wants to figure out all the issues with validating
> >just an overlay and make that work). You for example have an
> >undocumented compatible in yours (denx,fit).
> 
> Oh,need to check,copied partitions from my
>  uboot dts...maybe there is a linux version
>  for marking it as fit partition,else i drop
>  completely.

You just need to document it. But the first thing I'm going to say, is 
'u-boot' is the vendor, not 'denx'. So 'u-boot,fit'.

Rob