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

[Michal Simek]

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?

M