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

[Michal Simek]

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.

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.

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

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.

Thanks,
Michal