[LEDE-DEV] fixing of image file names

[Piotr Dymacz]

Hi Mathias,

On 01.12.2017 16:12, Mathias Kresin wrote:
> Hey Piotr ,
> 
> thanks a lot for thinking about my proposal.

Sorry for a very late reply.

> 2017-12-01 14:30 GMT+01:00 Piotr Dymacz <pepe2k at gmail.com>:
>> On 29.11.2017 09:33, Mathias Kresin wrote:
>>> 1. fix the compatible strings in the DTS files
>>
>> I think we should start at the same time upstreaming vendor prefixes to [1],
>> to avoid possible incompatibility/inconsistency later.
> 
> I really doubt that upstream is going to accept vendor prefixes if
> they aren't used anywhere. I would prefer to use what already exists,
> add our own prefixes where required and upstream them at the time the
> dts is send upstream.

I don't know but have a look at "opalkelly" [1] (recent example).

>> The assumption about the underscore character was my suggestion but it seems
>> that it's not (fully) valid. At least, after researching this more, I was
>> able to find only one reference in kernel patchwork: [2].
>> I suppose it's only a general convention rather than a documented
>> requirement (I'm might be wrong here, anyone? Maybe we should just ask
>> upstream about this) as there are already some examples in upstream which
>> contain underscores inside compatible strings: [3], [4].
>>
>> Assuming above and the fact that <manufacture> and <model> parts used in
>> compatible string, based on dt specification: [5], might contain any
>> printable characters (which might not be filename and Makefile safe), we
>> shouldn't make any assumptions about that.
> 
> I share your understanding of the devicetree spec.
> 
> So far I have only seen a handful of distortions in compat strings,
> like underlines. Never noticed any (special) charter beside [a-z],
> [0-9] and [-_].

I was able to find also examples of: '+' in [2], '.' in [3] and '/' in 
[4]. What's more, we have '+', '@' and '/' in our tree: [5], [6], [7].

>>> Due to the sync of runtime boardname and the boardname used in the image
>>> filename, it should be possible to guess the used image filename more
>>> reliably as requested.
>>
>>
>> I would like to propose here slightly extended version of compatible string
>> -> boardname part in image filename (and our building code) conversion
>> function:
>>
>> 1. Convert all letters to lowercase.
>> 2. Replace characters different than [a-z], [0-9] and comma with dash.
>> 3. Replace comma with underscore.
> 
> 3. is already done. I would prefer to add code for 1. and 2. at the
> time it is required.

Agree.
Just be aware of above findings.

> Beside of that, ACK to the filename specs.
> 
>> I think that we could even include above function in userspace and expose
>> the value somewhere, maybe in /tmp/sysinfo?
> 
> Same as above. Fine with me, but I prefer to add such code at the time
> it is required.
> 
> So far, all dts compat strings I've touched only consists of [a-z],
> [0-9] and [-], which allows an easy conversion of dts compat string to
> boardname.

Agree here as well.

>>> Any feedback on this approach (and help in migrating exists boards of
>>> course) is highly welcome.
>>
>>
>> I still have here some concerns/thoughts:
>>
>> 1. I don't know how to deal in above approach with region variant images but
>> I'm sure we should _somehow_ separate region code from boardname and other
>> parts of the image filename. IMHO, dash character won't work here.
> 
> It would be illusionary to assume that the compat string to image
> filename matching will work in all cases. I'm fine if it works in most
> cases. For edge cases we can use some kind of mapping file.

Maybe that mapping file isn't a bad idea. Could be useful also for some 
external projects already mentioned before.

>> 2. We have some boards versions like "v2.1" which with my above approach
>> would end up as "v2-1". Maybe we should keep dot as a valid character in
>> version part?
> 
> Yeah why not. As the devicetree spec doesn't prohibit any chars, it
> should be fine. Is anyone aware that such a compatible string was
> rejected upstream?

There are some compatible strings with dots in upstream so I would say 
it's allowed or at least it was at some point before.

>> 3. Some of devices come in different RAM/flash configurations and in case
>> where common image can't support all of them, we include information about
>> RAM/flash variant sometimes only in image name and sometimes in both the
>> image name and boardname/compatible string. A common and defined approach
>> would be required for this as well.
> 
> Shouldn't different devicetree source files be used in that case? I
> mean, they need a different image for a reason. Maybe because the
> memory and/or the partition node in the dts is different.

Yes, I just wanted to point out that most of the manufacturers don't 
care about properly naming different variants of their boards/devices 
and thus we have to deal with that.

> Anyway, we have other examples for the mentioned issue in the lantiq
> tree: dgn3500/dgn3500b and wbmrA/wbmrB. The only difference between
> the boards is the supported ADSL Annex and therefore the bundled adsl
> firmware.
> 
> For the dgn3500 two dts with (only) different compat strings and two
> images exists. For the wbmr the same devicetree source file is used
> for two images. An easy fix would be to bundle both adsl firmware
> versions.

Another example: TP-Link TL-MR6400 v1 and v2. The basic hardware is 
exactly the same, only the bundled miniPCIe 4G modem is different and 
requires different set of packages.

> For now I tend to leave it as it is. Of course, it doesn't improve the
> situation for the mentioned boards, but it doesn't make it worse
> either. Otherwise I would be busy with fixing all the corner cases
> instead of improving the situation for the (majority) of the boards
> where it is already easily possible. Yeah, call me lazy ;-).

You are the last person I would call lazy ;)
Thanks for your hard work on this!

[1] https://patchwork.ozlabs.org/patch/824516/

[2] 
http://elixir.free-electrons.com/linux/v4.15-rc3/source/arch/arm/boot/dts/ste-hrefv60plus.dtsi#L18

[3] 
http://elixir.free-electrons.com/linux/v4.15-rc3/source/arch/arm/boot/dts/xenvm-4.2.dts#L14

[4] 
http://elixir.free-electrons.com/linux/v4.15-rc3/source/arch/arm/boot/dts/imx6dl-hummingboard.dts#L49

[5] 
https://github.com/lede-project/source/blob/master/target/linux/brcm63xx/dts/cpva502plus.dts#L9

[6] 
https://github.com/lede-project/source/blob/master/target/linux/brcm63xx/dts/fast2704v2.dts#L9

[7] 
https://github.com/lede-project/source/blob/master/target/linux/brcm63xx/dts/dva-g3810bn_tl.dts#L9

-- 
Cheers,
Piotr