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

[Michal Simek]

On 27.7.2018 10:40, Chee, Tien Fong 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.

The driver is still using DM. It is only about way how driver is binded.
Take a look for example on board/hisilicon/hikey/hikey.c

Thanks,
Michal