[U-Boot] [PATCH v3 2/3] doc: dtbinding: Add file system firmware loader binding document

[Chee, Tien Fong <tien.fong.chee@intel.com>]

On Sun, 2018-07-08 at 20:39 -0600, Simon Glass wrote:
> Hi Tien Fong,
> 
> On 2 July 2018 at 01:26, Chee, Tien Fong <tien.fong.chee@intel.com>
> wrote:
> > 
> > On Fri, 2018-06-29 at 21:19 -0700, Simon Glass wrote:
> > > 
> > > Hi Tien Fong,
> > > 
> > > On 28 June 2018 at 01:58, Chee, Tien Fong <tien.fong.chee@intel.c
> > > om>
> > > wrote:
> > > > 
> > > > 
> > > > On Thu, 2018-06-28 at 01:04 -0700, Chee, Tien Fong wrote:
> > > > > 
> > > > > 
> > > > > On Wed, 2018-06-27 at 14:03 -0700, Simon Glass wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > Hi Tien Fong,
> > > > > > 
> > > > > > On 27 June 2018 at 01:32, Chee, Tien Fong <tien.fong.chee@i
> > > > > > ntel
> > > > > > .com
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On Mon, 2018-06-25 at 21:58 -0600, Simon Glass wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > On 25 June 2018 at 07:28,  <tien.fong.chee@intel.com>
> > > > > > > > wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > > > > > > > > 
> > > > > > > > > Add a document to describe file system firmware
> > > > > > > > > loader
> > > > > > > > > binding
> > > > > > > > > information.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.c
> > > > > > > > > om>
> > > > > > > > > ---
> > > > > > > > >  doc/device-tree-bindings/chosen.txt         | 22
> > > > > > > > > ++++++++++++
> > > > > > > > >  doc/device-tree-bindings/misc/fs_loader.txt | 52
> > > > > > > > > +++++++++++++++++++++++++++++
> > > > > > > > >  2 files changed, 74 insertions(+)
> > > > > > > > >  create mode 100644 doc/device-tree-
> > > > > > > > > bindings/misc/fs_loader.txt
> > > > > > > > > 
> > > > > > > > > diff --git a/doc/device-tree-bindings/chosen.txt
> > > > > > > > > b/doc/device-
> > > > > > > > > tree-
> > > > > > > > > bindings/chosen.txt
> > > > > > > > > index c96b8f7..738673c 100644
> > > > > > > > > --- a/doc/device-tree-bindings/chosen.txt
> > > > > > > > > +++ b/doc/device-tree-bindings/chosen.txt
> > > > > > > > > @@ -73,3 +73,25 @@ Example
> > > > > > > > >                 u-boot,spl-boot-order = "same-as-
> > > > > > > > > spl",
> > > > > > > > > &sdmmc,
> > > > > > > > > "/sd
> > > > > > > > > hci at fe330000";
> > > > > > > > >         };
> > > > > > > > >  };
> > > > > > > > > +
> > > > > > > > > +firmware-loader property
> > > > > > > > > +------------------------
> > > > > > > > > +Multiple file system firmware loader nodes could be
> > > > > > > > > defined
> > > > > > > > > in
> > > > > > > > > device trees for
> > > > > > > > > +multiple storage type and their default partition,
> > > > > > > > > then
> > > > > > > > > a
> > > > > > > > > property
> > > > > > > > > +"firmware-loader" can be used to pass default
> > > > > > > > > firmware
> > > > > > > > > loader
> > > > > > > > > +node(default storage type) to the firmware loader
> > > > > > > > > driver.
> > > > > > > > > +
> > > > > > > > > +Example
> > > > > > > > > +-------
> > > > > > > > > +/ {
> > > > > > > > > +       chosen {
> > > > > > > > > +               firmware-loader = &fs_loader0;
> > > > > > > > > +       };
> > > > > > > > > +
> > > > > > > > > +       fs_loader0: fs_loader at 0 {
> > > > > > > > This should be :
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > +       fs_loader0: fs-loader at 0 {
> > > > > > > > since hyphen is used for node and property names.
> > > > > > > > Underscore is
> > > > > > > > used
> > > > > > > > for phandles (so you have that correct).
> > > > > > > > 
> > > > > > > Okay.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > +               u-boot,dm-pre-reloc;
> > > > > > > > > +               compatible = "fs_loader";
> > > > > > > > How about u-boot,fs-loader since this is U-Boot
> > > > > > > > specific I
> > > > > > > > think.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > +               storage_device = "mmc";
> > > > > > > > storage-device
> > > > > > > > 
> > > > > > > Okay
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > +               devpart = "0:1";
> > > > > > > > I think you should use a phandle to the node containing
> > > > > > > > the
> > > > > > > > mmc
> > > > > > > > device to use.
> > > > > > > > 
> > > > > > > >    storage-device = <&mmc_3 1>;
> > > > > > > > 
> > > > > > > > for example (meaning use that MMC, partition 1.
> > > > > > > > 
> > > > > > > How to get device number based on node of a storage?
> > > > > > > This could make design more complicated because this
> > > > > > > driver
> > > > > > > is
> > > > > > > designed
> > > > > > > to support both fdt and without fdt(U-boot env and hard
> > > > > > > coding in
> > > > > > > driver).
> > > > > > I think it is fine to support the environment as a way of
> > > > > > providing
> > > > > > this info, but there is no need to support non-DM.
> > > > > > Everything
> > > > > > should
> > > > > > be converting to DM now.
> > > > > > 
> > > > > Ok,i would keep the DM and environment support.
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > We have:
> > > > > > 
> > > > > > uclass_get_device_by_phandle_id()
> > > > > > device_get_global_by_of_offset()
> > > > > > 
> > > > > > I think we need to create a function called
> > > > > > 
> > > > > > device_get_global_by_phandle_id()
> > > > > > 
> > > > > > which can be used to obtain the device based on a phandle.
> > > > > > 
> > > > > I means the device number in the struct blk_desc, block
> > > > > device. I
> > > > > don't
> > > > >  know how the device number is built up during driver model
> > > > > init.
> > > > > Is
> > > > > there a function to retrive the device number?
> > > > After roughly checking the blk-uclass.c, i think we can get the
> > > > device
> > > > number for blok device and also mainstain the consistent
> > > > interface
> > > > with
> > > > UBI(non block device), we need two parameters in FDT,
> > > > 1) storage-interface:
> > > > block device -
> > > >         "ide"
> > > >         "scsi"
> > > >         "atapi"
> > > >         "usb"
> > > >         "doc"
> > > >         "mmc"
> > > >         "sd"
> > > >         "sata"
> > > >         "host"
> > > >         "nvme"
> > > >         "efi"
> > > Do you need this? Can it not be obtained from uclass_get_name()
> > > using
> > > the phandle (below)?
> > > 
> > The purpose of above parameter is used to differentiate the block
> > devices and non block devices(like UBI). This function would be
> > called
> > if it is block device.
> Did you see the strings in the uclass drivers? E.g.:
> 
> UCLASS_DRIVER(mmc) = {
>         .id             = UCLASS_MMC,
>         .name           = "mmc",
>         .flags          = DM_UC_FLAG_SEQ_ALIAS,
>         .per_device_auto_alloc_size = sizeof(struct mmc_uclass_priv),
> };
> 
Yeah, but i'm not sure all uclass drivers having same string as above
interface string.

Anyway, i found alternate way using blk structure instead of the
interface string. You can check the v[4] patchset i have submitted from
last week.
> > 
> > 
> > Is the name is standard defined as above block devices string?
> I don't understand this. The same is defined for all uclasses that
> can
> have a block device as a child.
> 
> > 
> > > 
> > > > 
> > > > 
> > > > 
> > > > non block device
> > > >         "ubi"
> > > > 
> > > > 2) phandle for physical storage node(parent node) which contain
> > > > the
> > > > uclass support as listed below for block devices:
> > > >         UCLASS_IDE
> > > >         UCLASS_SCSI
> > > >         UCLASS_INVALID
> > > >         UCLASS_MASS_STORAGE
> > > >         UCLASS_INVALID
> > > >         UCLASS_MMC
> > > >         UCLASS_INVALID
> > > >         UCLASS_AHCI
> > > >         UCLASS_ROOT
> > > >         UCLASS_NVME
> > > >         UCLASS_EFI
> > > > Above nodes must contain the child node UCLASS_BLK(this node
> > > > contains
> > > > device number)
> > > > 
> > > > No node required for UBI interface.
> > > > 
> > > > I can create a new function, which is specific for getting the
> > > > device
> > > > number from the parent node(block device) which contains the
> > > > child
> > > > node
> > > > UCLASS_BLK. May be that function can be put into fs.c.
> > > > 
> > > > I'm not sure would this work as expected, any ideas/thoughts on
> > > > my
> > > > proposal?
> > > Sounds right to me.
> > > 
> > Is that possible one parent device(like MMC) contains multiple
> > child
> > devices in UCLASS_BLK?
> No, only one is supported.
> 
Noted.
> Regards,
> Simon