[U-Boot] [PATCH v4 6/6] common: Generic loader for file system

Tue Jul 31 06:22:10 UTC 2018

On 30.7.2018 18:05, Simon Glass wrote:
> Hi Michal,
> 
> On 30 July 2018 at 07:30, Michal Simek <michal.simek at xilinx.com> wrote:
>> On 30.7.2018 15:26, Simon Glass wrote:
>>> Hi,
>>>
>>> On 27 July 2018 at 02:40, Chee, Tien Fong <tien.fong.chee at intel.com> wrote:
>>>>
>>>> On Thu, 2018-07-26 at 11:03 +0200, Michal Simek wrote:
>>>>> On 25.7.2018 18:03, Tom Rini wrote:
>>>>>>
>>>>>> On Wed, Jul 25, 2018 at 09:47:17AM -0600, Simon Glass wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 25 July 2018 at 03:48, Michal Simek <michal.simek at xilinx.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 25.7.2018 08:31, Chee, Tien Fong wrote:
>>>>>>>>>
>>>>>>>>> On Wed, 2018-07-18 at 16:48 +0200, Michal Simek wrote:
>>>>>>>>>>
>>>>>>>>>> On 6.7.2018 10:28, tien.fong.chee at intel.com wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> From: Tien Fong Chee <tien.fong.chee at intel.com>
>>>>>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Also that DT binding is quite weird and I don't think you
>>>>>>>>>> will get
>>>>>>>>>> ACK
>>>>>>>>>> for this from device tree community at all. I think that
>>>>>>>>>> calling via
>>>>>>>>>> platdata and avoid DT nodes would be better way to go.
>>>>>>>>> Why do you think DT binding is weird? The DT is designed
>>>>>>>>> based on Simon
>>>>>>>>> proposal, and i believe following the rules in DTS spec.
>>>>>>>>> There are some DT benefits with current design, i think
>>>>>>>>> someone may be
>>>>>>>>> maintainer need to made the final decision on the design.
>>>>>>>> It is software configuration in file which should mostly
>>>>>>>> describe
>>>>>>>> hardware and state for hardware configuration.
>>>>>>>>
>>>>>>>> Your fs_loader node is purely describe sw configuration which
>>>>>>>> shouldn't
>>>>>>>> be here.
>>>>>>>> You have there run time configuration via variables. I think
>>>>>>>> using only
>>>>>>>> this way is enough. Default variables will match what you would
>>>>>>>> want to
>>>>>>>> add to DT.
>>>>>>> I think DT makes sense in the U-Boot context.
>>>>>>>
>>>>>>> We don't have a user space to handle policy decisions, and the
>>>>>>> 'chosen' node is a good place to configure these common features.
>>>>>>>
>>>>>>> While you can argue that the partition or filesystem where an
>>>>>>> image
>>>>>>> comes from is a software config, it is something that has to be
>>>>>>> configured. It has impact on hardware too, since the FPGA has to
>>>>>>> get
>>>>>>> its firmware from somewhere. We use the chosen node to specify
>>>>>>> the
>>>>>>> UART to use, and this is no different. Again, we don't have user-
>>>>>>> space
>>>>>>> config files in U-Boot.
>>>>>>>
>>>>>>> This argument comes up from time to time and I'd really like to
>>>>>>> put it
>>>>>>> to bed for U-Boot. I understand that Linux has its own approach
>>>>>>> and
>>>>>>> rules, but in some cases they serve U-Boot poorly.
>>>>>> I want to second this as well.  So long as we're using our prefix
>>>>>> and
>>>>>> we've thought through and discussed what we're trying to do here,
>>>>>> it's
>>>>>> OK to do things that might not be accepted for Linux.
>>>>>>
>>>>> I have not a problem with using chosen node with u-boot prefix
>>>>> properties and my colleague hopefully with finish work about moving
>>>>> u-boot,dm-pre-reloc; to chosen node where it should be (because
>>>>> current
>>>>> solution has also problem with ordering).
>>>>>
>>>>> In this loader case doc is saying that you can rewrite it with
>>>>> variables
>>>>> on the prompt (or via script).
>>>>> For cases that you want to autodetect platform and pass/load correct
>>>>> dtb
>>>>> which setup u-boot this can be problematic and using DT is could be
>>>>> considered as easier for use.
>>>>>
>>>>> In this case this is what was proposed:
>>>>>
>>>>> +     fs_loader0: fs-loader at 0 {
>>>>> +             u-boot,dm-pre-reloc;
>>>>> +             compatible = "u-boot,fs-loader";
>>>>> +             phandlepart = <&mmc 1>;
>>>>> +     };
>>>>>
>>>>> +     fs_loader1: fs-loader at 1 {
>>>>> +             u-boot,dm-pre-reloc;
>>>>> +             compatible = "u-boot,fs-loader";
>>>>> +             mtdpart = "UBI",
>>>>> +             ubivol = "ubi0";
>>>>> +     };
>>>>>
>>>>> u-boot,dm-pre-reloc; requires DM_FLAG_PRE_RELOC which is not setup
>>>>> for
>>>>> this driver - it means this should be here.
>>>> You are right, i missed this one. The intention of design enables user
>>>> to call any loader with default storage through the sequence number  if
>>>> fs loader is not defined in chosen. For example, there is a case where
>>>> system loading the file from SDMMC, NAND and QSPI.
>>>>>
>>>>> compatible = "u-boot,fs-loader"; - bind and probe are empty that's
>>>>> why
>>>>> this is only used for filling platdata but driver has no user that's
>>>>> why
>>>>> this is unused till someone calls that functions.
>>>>>
>>>>> phandlepart/mtdpart/ubivol is just for setup.
>>>> There are some benefits with driver model:
>>>> 1. Saving space, calling when need.
>>>> 2. Handle memory allocation and deallocation automatically.
>>>>>
>>>>> For the first case you can just use in chosen node:
>>>>> u-boot,fs-loader = <&mmc 1>;
>>>>>
>>>>> And for UBIfs. I have never played with that but I expect it
>>>>> shouldn't
>>>>> be big problem to describe it differently too (something like)
>>>>> u-boot,fs-loader = <0 ubi0>;
>>>> Need consider description for UBIFS, using fs-loader seems not working
>>>> for UBIFS, since more arguments such as mtdpartition and mtd volume
>>>> need passing into driver. In order to avoid messing, fs_loader can act
>>>> the pointer to the chosen.
>>>>
>>>> Anyway, i have no strong opinion with driver designed via platdata or
>>>> driver model if we can resolve the problem for UBIFS and maintainers
>>>> agree with it.
>>>>>
>>>>> Then this driver/interface can stay in DT where it should stay. The
>>>>> only
>>>>> thing is how this should be initialized because there is no
>>>>> compatible
>>>>> string. But you can do that via platdata for platforms which want to
>>>>> use
>>>>> this.
>>>
>>> We should add a compatible string then :-)
>>
>> Isn't driver name used in case of platdata initialization?
> 
> If the node is in /chosen and has a compatible string, it will be
> bound automatically. Manually binding a device is really just a
> fallback for particular situations (e.g. buses like PCI where we often
> rely on probing to find out what is on the bus).

up2you guys. I just have different opinion how this should be done.

Thanks,
Michal