Re: [PATCH 1/2] dt-bindings: mtd: document Broadcom's BCM47xx partitions

["Rafał Miłecki" <zajec5@gmail.com>]

Hi Rob & sorry for the late reply. My earlier accepted patchset adding
of_match_table matching was reported to cause a regression. It had to be
dropped and I had to focus on fixing that first.

On 01/19/2018 10:44 PM, Rob Herring wrote:
> On Fri, Jan 12, 2018 at 10:33:53AM +0100, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> Broadcom based home router devices use partitions which have to be
>> discovered in a specific non-standard way. As there is no partition
>> table this commit adds and describes a new custom binding.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>>  .../devicetree/bindings/mtd/partition.txt          | 46 +++++++++++++++++++++-
>>  1 file changed, 44 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/partition.txt b/Documentation/devicetree/bindings/mtd/partition.txt
>> index 36f3b769a626..b201d497b618 100644
>> --- a/Documentation/devicetree/bindings/mtd/partition.txt
>> +++ b/Documentation/devicetree/bindings/mtd/partition.txt
>> @@ -14,8 +14,6 @@ method is used for a given flash device. To describe the method there should be
>>  a subnode of the flash device that is named 'partitions'. It must have a
>>  'compatible' property, which is used to identify the method to use.
>>
>> -We currently only document a binding for fixed layouts.
>> -
>>
>>  Fixed Partitions
>>  ================
>> @@ -109,3 +107,47 @@ flash@2 {
>>  		};
>>  	};
>>  };
>> +
>> +
>> +Broadcom BCM47xx Partitions
>> +===========================
>> +
>> +Broadcom is one of hardware manufacturers providing SoCs (BCM47xx) used in
>> +home routers. Their BCM947xx boards using CFE bootloader have several partitions
>> +without any on-flash partition table. On some devices their sizes and/or
>> +meanings can also vary so fixed partitioning can't be used.
>> +
>> +Discovering partitions on these devices is possible thanks to having a special
>> +header and/or magic signature at the beginning of each of them. They are also
>> +block aligned which is important for determinig a size.
>> +
>> +Most of partitions use ASCII text based magic for determining a type. More
>> +complex partitions (like TRX with its HDR0 magic) may include extra header
>> +containing some details, including a length.
>> +
>> +A list of supported partitions includes:
>> +1) Bootloader with Broadcom's CFE (Common Firmware Environment)
>> +2) NVRAM with configuration/calibration data
>> +3) Device manufacturer's data with some default values (e.g. SSIDs)
>> +4) TRX firmware container which can hold up to 4 subpartitions
>> +5) Backup TRX firmware used after failed upgrade
>> +
>> +As mentioned earlier, role of some partitions may depend on extra configuration.
>> +For example both: main firmware and backup firmware use the same TRX format with
>> +the same header. To distinguish currently used firmware a CFE's environment
>> +variable "bootpartition" is used.
>> +
>> +
>> +Devices using Broadcom partitions described above should should have flash node
>> +with a subnode named "partitions" using following properties:
>> +
>> +Required properties:
>> +- compatible : (required) must be "brcm,bcm947xx-cfe-partitions"
>
> So you can't discover you have CFE partitions, but you can discover
> everything else like where partitions start?

I could discover there is a CFE bootloader at the flash beginning and
assume there are CFE-like partitions. It doesn't sound safe however and
I believe the point of using DT + compatible properties is to avoid such
/guessing/. There are too many cases, devices, "formats" to just blindly
try every possible parser. That's why we agreed to use "compatible"
strings.

This was also described by Boris in his patchset 2+ years ago:
[RFC PATCH 0/7] mtd: partitions: add of_match_table support
http://lists.infradead.org/pipermail/linux-mtd/2015-December/064076.html

 >  (2) we can't just scan for all supported parsers (like the block system does), since
 >      there is a wide diversity of "formats" (no standardization), and it is not
 >      always safe or efficient to attempt to do so, particularly since many of
 >      them allow their data structures to be placed anywhere on the flash, and
 >      so require scanning the entire flash device to find them.

I also believe this is solution you acked back then:
[RFC PATCH 3/7] doc: dt: mtd: partition: add on-flash format binding
http://lists.infradead.org/pipermail/linux-mtd/2015-December/064100.html

 > On Fri, Dec 04, 2015 at 09:19:19PM -0800, Brian Norris wrote:
 > > The platform description (such as the type of partition formats used on
 > > a given flash) should be done independently of the flash driver in use.
 > > However, we can't reasonably have *all* partition parsers run on all
 > > flash (until they find a match), so let's overload the 'partitions'
 > > subnode to support specifying which format(s) to try in the device tree.
 > >
 > > (...)
 >
 > Looks good to me. If you had lots of partitions, I'd agree with comments
 > that they should be separate files, but I doubt we'll have many 10s of
 > them. Plus all we really need here is a list of compatible strings.
 >
 > Acked-by: Rob Herring <robh at kernel.org>

So I guess the answer is:
1) I really don't want to blindly look for CFE-like partitions. Too much
    risk of false positives.
2) I don't want parser to be blindly called & use extra magic checks to
    make sure I'm dealing with device using CFE bootloader.
3) Once I know I'm dealing with device using CFE bootloader I can find
    partitions on the flash.


> Why doesn't the kernel just probe for the partition table?

If by a "partition table" you mean a single block with data using a
clear structure: it doesn't exist. That's the whole point with crappy
Broadcom devices.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/