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

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

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@intel.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.com>
> > > > > ---
> > > > >  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"

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?
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > +       };
> > > > > +};
> > > > > diff --git a/doc/device-tree-bindings/misc/fs_loader.txt
> > > > > b/doc/device-tree-bindings/misc/fs_loader.txt
> > > > > new file mode 100644
> > > > > index 0000000..78bea66
> > > > > --- /dev/null
> > > > > +++ b/doc/device-tree-bindings/misc/fs_loader.txt
> > > > > @@ -0,0 +1,52 @@
> > > > > +* File system firmware loader
> > > > > +
> > > > > +Required properties:
> > > > > +--------------------
> > > > > +
> > > > > +- compatible: should contain "fs_loader"
> > > > > +- storage_device: which storage device loading from, could
> > > > > be:
> > > > > +                 - mmc, usb, sata, and ubi.
> > > > > +- devpart: which storage device and partition the image
> > > > > loading
> > > > > from,
> > > > > +          this property is required for mmc, usb and sata.
> > > > > +- mdtpart: which partition of ubi the image loading from,
> > > > > this
> > > > > property is
> > > > > +          required for ubi.
> > > > > +- ubivol: which volume of ubi the image loading from, this
> > > > > proprety is required
> > > > > +         for ubi.
> > > > > +
> > > > > +Example of storage device and partition search set for mmc,
> > > > > usb,
> > > > > sata and
> > > > > +ubi in device tree source as shown in below:
> > > > > +
> > > > > +       Example of storage type and device partition search
> > > > > set
> > > > > for
> > > > > mmc, usb,
> > > > > +       sata and ubi as shown in below:
> > > > > +       Example for mmc:
> > > > > +       fs_loader0: fs_loader at 0 {
> > > > > +               u-boot,dm-pre-reloc;
> > > > > +               compatible = "fs_loader";
> > > > > +               storage_device = "mmc";
> > > > > +               devpart = "0:1";
> > > > > +       };
> > > > > +
> > > > > +       Example for usb:
> > > > > +       fs_loader1: fs_loader at 1 {
> > > > > +               u-boot,dm-pre-reloc;
> > > > > +               compatible = "fs_loader";
> > > > > +               storage_device = "usb";
> > > > > +               devpart = "0:1";
> > > > > +       };
> > > > > +
> > > > > +       Example for sata:
> > > > > +       fs_loader2: fs_loader at 2 {
> > > > > +               u-boot,dm-pre-reloc;
> > > > > +               compatible = "fs_loader";
> > > > > +               storage_device = "sata";
> > > > > +               devpart = "0:1";
> > > > > +       };
> > > > > +
> > > > > +       Example for ubi:
> > > > > +       fs_loader3: fs_loader at 3 {
> > > > > +               u-boot,dm-pre-reloc;
> > > > > +               compatible = "fs_loader";
> > > > > +               storage_device = "ubi";
> > > > > +               mtdpart = "UBI",
> > > > > +               ubivol = "ubi0";
> > > > I'm not sure about ubi. It needs to refer to a particular
> > > > device
> > > > I
> > > > suppose. How do we know the mapping from ubi to device?
> > > > 
> > > Actually this design is based on file system implementation which
> > > is
> > > abstract from physical device, storage_device is used as
> > > interface
> > > of
> > > file system, so nand and qspi may having the same ubi interface.
> > > 
> > > May be i can change the storage_device into storage_interface to
> > > avoid
> > > confusing.
> > OK I see. Well I suppose this is OK then, at least for now. I'm not
> > have any great ideas on how to specify which ubi storage device to
> > use. Let's assume we only have one for now.
> > 
> > Regards,
> > Simon