[PATCH] mtd: document linux-specific partition parser DT binding

[PATCH] mtd: document linux-specific partition parser DT binding

[Linus Walleij]

The Linux code in drivers/mtd/maps/physmap_of.c will optionally
look for this binding for hints on a partition type to look for
in the MTD. It was added in commit 9d5da3a9b849
"mtd: extend physmap_of to let the device tree specify the parition probe"
but no corresponding device tree binding was added. Fix this.

Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Cc: Liviu Dudau <Liviu.Dudau-5wv7dgnIgG8@public.gmane.org>
Reported-by: Liviu Dudau <Liviu.Dudau-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 Documentation/devicetree/bindings/mtd/mtd-physmap.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
index 4a0a48bf4ecb..0dee084651da 100644
--- a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
+++ b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
@@ -26,6 +26,9 @@ file systems on embedded devices.
  - linux,mtd-name: allow to specify the mtd name for retro capability with
    physmap-flash drivers as boot loader pass the mtd partition via the old
    device name physmap-flash.
+ - linux,part-probe: a flash partition type to look for, using the
+   Linux-internal partition naming scheme, e.g. "afs" for the ARM
+   Flash footers.
  - use-advanced-sector-protection: boolean to enable support for the
    advanced sector protection (Spansion: PPB - Persistent Protection
    Bits) locking.
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mtd: document linux-specific partition parser DT binding

[Brian Norris]

+ others

Hi Linus,

On Thu, Oct 15, 2015 at 03:19:51PM +0200, Linus Walleij wrote:
> The Linux code in drivers/mtd/maps/physmap_of.c will optionally
> look for this binding for hints on a partition type to look for
> in the MTD. It was added in commit 9d5da3a9b849
> "mtd: extend physmap_of to let the device tree specify the parition probe"
> but no corresponding device tree binding was added. Fix this.
> 
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Cc: Liviu Dudau <Liviu.Dudau-5wv7dgnIgG8@public.gmane.org>
> Reported-by: Liviu Dudau <Liviu.Dudau-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Are you trying to use this binding, or is this just purely a mechanical
documentation issue? I ask, because it seems that binding never really
got reviewed at all, and others have recently tried to extend support
for it generically [1], but a few objections came up [2][3].

Unfortunately I/we dropped the ball a bit on that thread, but we'd
ideally like to address those concerns in a new binding that is
supported for all MTDs, and deprecate the old one. The new one would
probably not directly use the parser name as used by Linux, but define
some list of compatible strings that fit DT conventions better. Also, I
don't want people including things like "cmdlinepart" in DT, but it
should be available as an override if necessary. IOW, DT shouldn't
supersede the kernel command line.

That's not to say we can't document the old one, but I'm curious if
there are real users. I'd also like to encourage new users to avoid the
old one if we can make that feasible.

> ---
>  Documentation/devicetree/bindings/mtd/mtd-physmap.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
> index 4a0a48bf4ecb..0dee084651da 100644
> --- a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
> +++ b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
> @@ -26,6 +26,9 @@ file systems on embedded devices.
>   - linux,mtd-name: allow to specify the mtd name for retro capability with
>     physmap-flash drivers as boot loader pass the mtd partition via the old
>     device name physmap-flash.
> + - linux,part-probe: a flash partition type to look for, using the
> +   Linux-internal partition naming scheme, e.g. "afs" for the ARM
> +   Flash footers.

IIUC, this property actually supports a list of parsers, not just a
single partition type.

Also, if we're really going to support this, we should list exactly what
strings we support. And that's one of the problems with the existing
binding; it supports any old string Linux supports, which doesn't match
how we typically want to add bindings (i.e., via proposal + review).

>   - use-advanced-sector-protection: boolean to enable support for the
>     advanced sector protection (Spansion: PPB - Persistent Protection
>     Bits) locking.

Brian

[1] http://lists.infradead.org/pipermail/linux-mtd/2015-May/059226.html
[2] http://lists.infradead.org/pipermail/linux-mtd/2015-May/059276.html
[3] Tail end of Arnd's comments here:
    http://thread.gmane.org/gmane.linux.drivers.devicetree/122479/focus=122788
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mtd: document linux-specific partition parser DT binding

[Jason Gunthorpe]

On Thu, Oct 15, 2015 at 09:22:10AM -0700, Brian Norris wrote:

> Are you trying to use this binding, or is this just purely a mechanical
> documentation issue? I ask, because it seems that binding never really
> got reviewed at all, and others have recently tried to extend
> support

I wrote it before ARM started down the DT direction and all these
formalities were developed.

> That's not to say we can't document the old one, but I'm curious if
> there are real users. I'd also like to encourage new users to avoid the
> old one if we can make that feasible.

We continue to use it here, there is just no way to autodetect flash
partition layouts. I'm not fussed about having to change to something
else in future.

The reason the original patch exposed the cmdline parser is because
that is how the internal mechanisms work - but realistically, the
cmdline parser is a hack to get around the lack of DT partition
description. If the DT can describe the partitions it should supersede
and obsolete the old command line hack. IMHO

> Also, if we're really going to support this, we should list exactly what
> strings we support. And that's one of the problems with the existing
> binding; it supports any old string Linux supports, which doesn't match
> how we typically want to add bindings (i.e., via proposal + review).

That is why it is prefixed with linux, the review of the part parser name is
the responsibility of the MTD crew, not the DT folks.

bcm47xxpart seems like a terrible name for a partition scheme. Really,
almost any scheme can be used on any SOC, naming a partition parser
after a SOC family makes very little sense to me..

Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mtd: document linux-specific partition parser DT binding

[Brian Norris]

Hi Jason,

On Thu, Oct 15, 2015 at 11:04:28AM -0600, Jason Gunthorpe wrote:
> On Thu, Oct 15, 2015 at 09:22:10AM -0700, Brian Norris wrote:
> 
> > Are you trying to use this binding, or is this just purely a mechanical
> > documentation issue? I ask, because it seems that binding never really
> > got reviewed at all, and others have recently tried to extend
> > support
> 
> I wrote it before ARM started down the DT direction and all these
> formalities were developed.
> 
> > That's not to say we can't document the old one, but I'm curious if
> > there are real users. I'd also like to encourage new users to avoid the
> > old one if we can make that feasible.
> 
> We continue to use it here,

Good to know.

> there is just no way to autodetect flash
> partition layouts.

Right. It'd be lovely if things were more standardized, and we could
just build all parsers in for all MTDs, like how block devices do it,
but sadly that's probably not feasible, given the quirks of various
parsers, and how they often require scanning a large portion of the
device :(

> I'm not fussed about having to change to something
> else in future.

OK, good to know. Recent developments have put maintainers in a tough
position sometimes, since as soon as there's a DT binding used in the
wild, some people like to claim that it is ABI and must never change.
This ties the hands when wanting to deprecate/improve/replace features.
In reality, there are few (my guess: a negligible near-zero-percent) DT
users that actually have DTs locked into some kind of firmware in a way
that they (1) can never update their DT and (2) expect to be able to run
mainline kernels, so this all just becomes a lot of fuss over nothing.

> The reason the original patch exposed the cmdline parser is because
> that is how the internal mechanisms work

Right. And that's why it's best these days not to directly expose
internal mechanisms via DT.

> - but realistically, the
> cmdline parser is a hack to get around the lack of DT partition
> description. If the DT can describe the partitions it should supersede
> and obsolete the old command line hack. IMHO

Hmm, I don't know. I wouldn't expect people should really be using
cmdlineparts as a production solution, but I'd consider it more of a
debugging/development option -- if I want to override the DT (which is
sometimes a bit harder to change) or the on-flash layout (e.g.,
RedBoot), then I can fiddle with the command line. So I wouldn't quite
qualify it as a hack (though many might use it in a hacky way) nor would
I suggest that the DT should override it.

> > Also, if we're really going to support this, we should list exactly what
> > strings we support. And that's one of the problems with the existing
> > binding; it supports any old string Linux supports, which doesn't match
> > how we typically want to add bindings (i.e., via proposal + review).
> 
> That is why it is prefixed with linux, the review of the part parser name is
> the responsibility of the MTD crew, not the DT folks.

OK. That works for now. But I don't want to extend this binding to any
other drivers, if I can help it. And I don't want to encourage it (lest
it *really* becomes ABI).

I don't think it would be too hard to fix this to not be so
Linux-specific. I think any partition layouts that people would want to
specify in DT can be reasonably well-defined to not consider them
Linux-specific. Some probably didn't even have a Linux target in mind
when they were developed. We'd just need to give them better names and a
short description and/or pointers to documentation. e.g. [1][2].

> bcm47xxpart seems like a terrible name for a partition scheme. Really,
> almost any scheme can be used on any SOC, naming a partition parser
> after a SOC family makes very little sense to me..

I believe it was defined purely by Broadcom, for use with their
firmwares mostly seen on that SoC family. And in the spirit of DT
naming, IP often gets named after the first SoC that implements it, even
if later revs aren't the same SoC (or SoC family) but are still
compatible. So I can see the name being reasonable, even if not perfect.

Any better nominations for names?

Brian

[1] http://infocenter.arm.com/help/topic/com.arm.doc.dui0102g/DUI0102F_afs1_4_rg.pdf
[2] https://sourceware.org/redboot/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mtd: document linux-specific partition parser DT binding

[Jason Gunthorpe]

On Thu, Oct 15, 2015 at 10:47:09AM -0700, Brian Norris wrote:
> Hmm, I don't know. I wouldn't expect people should really be using
> cmdlineparts as a production solution, but I'd consider it more of a
> debugging/development option -- if I want to override the DT (which is
> sometimes a bit harder to change) or the on-flash layout (e.g.,
> RedBoot), then I can fiddle with the command line.

We don't have cmdline overrides for very much (if any?) of DT.

The DT is supposed to describe the hardware/boot environment, if
overriding the partition layout is really common, then is the DT
binding really describing the hardware at all?

The partition layout is already very border line to support in DT, it
is really very close to a software configuration which is strongly
discouraged from DT files.

I justify it as the boot loader communicating the partition scheme it
understands to the OS, so the OS can have a hope of setting up the
flash in a way the bootloader understands.

I've never used the cmdline option, so maybe I don't see the use case,
but it seems like a really weird desire to change the partitioning
away from what the bootloader supports.

> Any better nominations for names?

bcrm-part-table-43 ?

Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mtd: document linux-specific partition parser DT binding

[Linus Walleij]

On Thu, Oct 15, 2015 at 6:22 PM, Brian Norris
<computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Are you trying to use this binding, or is this just purely a mechanical
> documentation issue? I ask, because it seems that binding never really
> got reviewed at all, and others have recently tried to extend support
> for it generically [1], but a few objections came up [2][3].

I am using it in this devicetree patch:
http://marc.info/?l=linux-arm-kernel&m=144492610417605&w=2

And this devicetree patch:
http://marc.info/?l=linux-arm-kernel&m=144490455308758&w=2

Otherwise the AFS partition type will not be scanned.

The other option is to add "afs" to this list in
drivers/mtd/maps/physmap_of.c:

diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
index 3e614e9119d5..6233473a55d6 100644
--- a/drivers/mtd/maps/physmap_of.c
+++ b/drivers/mtd/maps/physmap_of.c
@@ -110,7 +110,7 @@ static struct mtd_info *obsolete_probe(struct
platform_device *dev,
    default is use. These take precedence over other device tree
    information. */
 static const char * const part_probe_types_def[] = {
-       "cmdlinepart", "RedBoot", "ofpart", "ofoldpart", NULL };
+       "cmdlinepart", "RedBoot", "ofpart", "ofoldpart", "afs", NULL };

I can send a patch like this if you prefer?

> Unfortunately I/we dropped the ball a bit on that thread, but we'd
> ideally like to address those concerns in a new binding that is
> supported for all MTDs, and deprecate the old one. The new one would
> probably not directly use the parser name as used by Linux, but define
> some list of compatible strings that fit DT conventions better. Also, I
> don't want people including things like "cmdlinepart" in DT, but it
> should be available as an override if necessary. IOW, DT shouldn't
> supersede the kernel command line.

This is not for cmdlinepart, as AFS is an actual on-flash format.

> That's not to say we can't document the old one, but I'm curious if
> there are real users. I'd also like to encourage new users to avoid the
> old one if we can make that feasible.

I'm happy to do either patch, or define a new binding if you
prefer, like simply:

partition-type = <string>;

and then we define this as "arm,arm-flash-structure" or something,
and parse that to "afs" internally in physmap_of.c.

>> + - linux,part-probe: a flash partition type to look for, using the
>> +   Linux-internal partition naming scheme, e.g. "afs" for the ARM
>> +   Flash footers.
>
> IIUC, this property actually supports a list of parsers, not just a
> single partition type.

Ah it does. If we wanna go with this patch I'll fix it.

> Also, if we're really going to support this, we should list exactly what
> strings we support. And that's one of the problems with the existing
> binding; it supports any old string Linux supports, which doesn't match
> how we typically want to add bindings (i.e., via proposal + review).

OK that sounds like a case for "arm,arm-flash-structure" for this
specific binding.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mtd: document linux-specific partition parser DT binding

[Brian Norris]

Hi Linus,

On Fri, Oct 23, 2015 at 11:17:54AM +0200, Linus Walleij wrote:
> On Thu, Oct 15, 2015 at 6:22 PM, Brian Norris
> <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > Are you trying to use this binding, or is this just purely a mechanical
> > documentation issue? I ask, because it seems that binding never really
> > got reviewed at all, and others have recently tried to extend support
> > for it generically [1], but a few objections came up [2][3].
> 
> I am using it in this devicetree patch:
> http://marc.info/?l=linux-arm-kernel&m=144492610417605&w=2
> 
> And this devicetree patch:
> http://marc.info/?l=linux-arm-kernel&m=144490455308758&w=2
> 
> Otherwise the AFS partition type will not be scanned.
> 
> The other option is to add "afs" to this list in
> drivers/mtd/maps/physmap_of.c:
> 
> diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
> index 3e614e9119d5..6233473a55d6 100644
> --- a/drivers/mtd/maps/physmap_of.c
> +++ b/drivers/mtd/maps/physmap_of.c
> @@ -110,7 +110,7 @@ static struct mtd_info *obsolete_probe(struct
> platform_device *dev,
>     default is use. These take precedence over other device tree
>     information. */
>  static const char * const part_probe_types_def[] = {
> -       "cmdlinepart", "RedBoot", "ofpart", "ofoldpart", NULL };
> +       "cmdlinepart", "RedBoot", "ofpart", "ofoldpart", "afs", NULL };
> 
> I can send a patch like this if you prefer?

Hmm, not really. While I'd love to have a good common set of reasonably
generic partition parsers that we could always run on all MTDs (or in
this case, all physmap_of MTDs) similar to how the block subsystem does
it, I don't think that is really workable here, as there is too much
variation in parsers and mamy of them actually scan the whole flash (or
at least, the headers of each block). This can get very expensive, and
potentially leaves room for false matches, especially when you start
layering on all the other potential parsers others might want to use.

IOW, this solution is not scalable IMO. In previous discussions, I think
we've agreed that partition parsing must be informed by platform
knowledge.

> > Unfortunately I/we dropped the ball a bit on that thread, but we'd
> > ideally like to address those concerns in a new binding that is
> > supported for all MTDs, and deprecate the old one. The new one would
> > probably not directly use the parser name as used by Linux, but define
> > some list of compatible strings that fit DT conventions better. Also, I
> > don't want people including things like "cmdlinepart" in DT, but it
> > should be available as an override if necessary. IOW, DT shouldn't
> > supersede the kernel command line.
> 
> This is not for cmdlinepart, as AFS is an actual on-flash format.

Right. I was just mentioning that this binding:

  (1) allows people to specify nonsensical things like "ofpart" and
  "cmdlinepart" in their DT and
  (2) allows people to specify DTs that *don't* allow developer
  overrides like cmdlinepart (e.g., the partition layout is corrupted,
  so we want to boot a recovery kernel with a predetermined
  'mtdparts=...' parameter)

Neither is directly related to your use of AFS, but it's just part of my
thoughts on codifying the use of this binding.

> > That's not to say we can't document the old one, but I'm curious if
> > there are real users. I'd also like to encourage new users to avoid the
> > old one if we can make that feasible.
> 
> I'm happy to do either patch, or define a new binding if you
> prefer, like simply:

I would prefer a new binding. And personally, I'd like to get something
like this extended to all MTDs (there are other users asking for similar
things), but I haven't made the time to write the patch :(

> partition-type = <string>;
> 
> and then we define this as "arm,arm-flash-structure" or something,
> and parse that to "afs" internally in physmap_of.c.

Seems OK to me. Again, I'd prefer this not live in physmap_of.c, but if
that's the expedient solution, I could probably take it from there.

> >> + - linux,part-probe: a flash partition type to look for, using the
> >> +   Linux-internal partition naming scheme, e.g. "afs" for the ARM
> >> +   Flash footers.
> >
> > IIUC, this property actually supports a list of parsers, not just a
> > single partition type.
> 
> Ah it does. If we wanna go with this patch I'll fix it.

I'd rather not take this patch if we can safely pivot to an improved
binding, but I'm not sure if that violates the DT policy party line :)

> > Also, if we're really going to support this, we should list exactly what
> > strings we support. And that's one of the problems with the existing
> > binding; it supports any old string Linux supports, which doesn't match
> > how we typically want to add bindings (i.e., via proposal + review).
> 
> OK that sounds like a case for "arm,arm-flash-structure" for this
> specific binding.

Right.

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html