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

[Simon Glass]

Hi Tien Fong,

On 28 June 2018 at 01:58, Chee, Tien Fong <tien.fong.chee at intel.com> 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 at 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 at intel.com> wrote:
>> > > > >
>> > > > >
>> > > > >
>> > > > > From: Tien Fong Chee <tien.fong.chee at intel.com>
>> > > > >
>> > > > > Add a document to describe file system firmware loader
>> > > > > binding
>> > > > > information.
>> > > > >
>> > > > > Signed-off-by: Tien Fong Chee <tien.fong.chee at 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"

Do you need this? Can it not be obtained from uclass_get_name() using
the phandle (below)?

>
> 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.

[...]

Regards,
Simon