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

Sat May 5 14:47:31 PDT 2018

Hi Rob,

On 29 March 2018 at 08:23, Rafał Miłecki <zajec5 at gmail.com> wrote:
> 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 at 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 at 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 at 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.

Would you find a moment to review that patch again, after extra
explanation I provided, please?