[PATCH] [MTD] ofpart: Partitions at same address cannot have the same name v3

[PATCH] [MTD] ofpart: Partitions at same address cannot have the same name v3

[Ricardo Ribalda Delgado]

Sometimes, an special partition is included in the device tree including all the
partitions. Like in:

partition@ff000000 {
       reg = < 0x000000 0x800000 >;
       label = "Root File System";
};
partition@ff800000 {
       reg = < 0x800000 0x1a0000 >;
       label = "Bitstream";
};
...
full@ff000000 {
       compatible = "partition";
       reg = < 0x000000 0x1000000 >;
       label = "Full FLASH";
};

Because two nodes of a device tree cannot have the same name, but all the
partitions must be named "partition", this special partition is invalid.

This patch makes ofpart.c accept spetial partitions compatible with
"partition" but not named partition.

These spetial partitions are very useful for flashing the full firmware
of a device from linux
---
This v3 includes feedback from Scott Wood, Peter Korsgaard & Benjamin Kril

v3: Use the compatible propierty
v2: buggy implementation, strlen-1 instead of strlen
v1: Just check the firt part of the name


 drivers/mtd/ofpart.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
index 3e164f0..59c1e4a 100644
--- a/drivers/mtd/ofpart.c
+++ b/drivers/mtd/ofpart.c
@@ -48,7 +48,9 @@ int __devinit of_mtd_parse_partitions(struct device *dev,
 
 		/* check if this is a partition node */
 		partname = of_get_property(pp, "name", &len);
-		if (strcmp(partname, "partition") != 0) {
+		if ((strcmp(partname, "partition") != 0) &&
+			(of_device_is_compatible(pp, "partition") != 1))
+		{
 			nr_parts--;
 			continue;
 		}
-- 
1.6.2.4

Re: [PATCH] [MTD] ofpart: Partitions at same address cannot have the same name v3

[Benjamin Krill]

>--- a/drivers/mtd/ofpart.c
>+++ b/drivers/mtd/ofpart.c
>@@ -48,7 +48,9 @@ int __devinit of_mtd_parse_partitions(struct device *dev,
> 
> 		/* check if this is a partition node */
> 		partname = of_get_property(pp, "name", &len);
>-		if (strcmp(partname, "partition") != 0) {
>+		if ((strcmp(partname, "partition") != 0) &&
>+			(of_device_is_compatible(pp, "partition") != 1))
>+		{
> 			nr_parts--;
> 			continue;
> 		}

If this is the way, how to go, you get my ack.

Acked-by: Benjamin Krill <ben@codiert.org>

Re: [PATCH] [MTD] ofpart: Partitions at same address cannot have the same name v3

[Segher Boessenkool]

> Sometimes, an special partition is included in the device tree  
> including all the
> partitions. Like in:
>
> partition@ff000000 {
>        reg = < 0x000000 0x800000 >;
>        label = "Root File System";
> };
> partition@ff800000 {
>        reg = < 0x800000 0x1a0000 >;
>        label = "Bitstream";
> };
> ...
> full@ff000000 {
>        compatible = "partition";
>        reg = < 0x000000 0x1000000 >;
>        label = "Full FLASH";
> };

Your special "partition" isn't really a partition then, is it.
Because of that, device nodes to represent your partitions doesn't
work very well.

You really want to use something else, a partition table on the
flash itself for example.  Or maybe the (platform? MTD?) code
should create a Linux device for the "full" device.

> Because two nodes of a device tree cannot have the same name,

This isn't true.

> but all the
> partitions must be named "partition",

Bad binding, no cookie for you!

> -		if (strcmp(partname, "partition") != 0) {
> +		if ((strcmp(partname, "partition") != 0) &&
> +			(of_device_is_compatible(pp, "partition") != 1))
> +		{

You cannot claim a name as generic as "partition" for this.  Pick
something else if you really must do things this way.


Segher

Re: [PATCH] [MTD] ofpart: Partitions at same address cannot have thesame name v3

[Ricardo Ribalda Delgado]

Hello Segher

  Thanks for your comments.

>
> Your special "partition" isn't really a partition then, is it.
> Because of that, device nodes to represent your partitions doesn't
> work very well.

I think that they work pretty well. Unfortunately, since
4b08e149c0e02e97ec49c2a31d14a0d3a02f8074 all the partiton must be
named "partition"

>
> You really want to use something else, a partition table on the
> flash itself for example. =A0Or maybe the (platform? MTD?) code
> should create a Linux device for the "full" device.

This wont be very flexible. With the device tree aproach we can define
partitions not only for the full flash, but also for two partitions
merged, a partition inside a partition....

>
>> Because two nodes of a device tree cannot have the same name,
>
> This isn't true.

Are you sure? This is what I get if I try to compile a device tree
with two partitions with the same name starting at the same address.

$ arch/powerpc/boot/dtc -O dtb -b 0 -p 1024
arch/powerpc/boot/dts/q5-avnet.dts -o /tmp/kk
DTC: dts->dtb  on file "arch/powerpc/boot/dts/q5-avnet.dts"
ERROR (duplicate_node_names): Duplicate node name
/plb@0/flash@ff000000/partition@ff000000
ERROR: Input tree has errors, aborting (use -f to force output)



>
>> but all the
>> partitions must be named "partition",
>
> Bad binding, no cookie for you!

Sorry, I dont understand want you want to say here.


> You cannot claim a name as generic as "partition" for this. =A0Pick
> something else if you really must do things this way.


You choose :)


         Best regards


--=20
Ricardo Ribalda
http://www.eps.uam.es/~rribalda/

Re: [PATCH] [MTD] ofpart: Partitions at same address cannot have thesame name v3

[Segher Boessenkool]

>> Your special "partition" isn't really a partition then, is it.
>> Because of that, device nodes to represent your partitions doesn't
>> work very well.
>
> I think that they work pretty well.

With "real" devices you don't normally have two devices sitting at
the same address.  Since you _do_ have that with your partitions,
it doesn't fit the device node abstraction very well.

> Unfortunately, since
> 4b08e149c0e02e97ec49c2a31d14a0d3a02f8074 all the partiton must be
> named "partition"

Ah!  After writing my original reply, I read the documentation,
and this had me confused.

4b08e14 looks wrong to me.  It also didn't update the documentation.

>> You really want to use something else, a partition table on the
>> flash itself for example.  Or maybe the (platform? MTD?) code
>> should create a Linux device for the "full" device.
>
> This wont be very flexible. With the device tree aproach we can define
> partitions not only for the full flash, but also for two partitions
> merged, a partition inside a partition....

Well, except you have problems doing just that :-P

>>> Because two nodes of a device tree cannot have the same name,
>>
>> This isn't true.
>
> Are you sure?

Yes.

> This is what I get if I try to compile a device tree
> with two partitions with the same name starting at the same address.
>
> $ arch/powerpc/boot/dtc -O dtb -b 0 -p 1024
> arch/powerpc/boot/dts/q5-avnet.dts -o /tmp/kk
> DTC: dts->dtb  on file "arch/powerpc/boot/dts/q5-avnet.dts"
> ERROR (duplicate_node_names): Duplicate node name
> /plb@0/flash@ff000000/partition@ff000000
> ERROR: Input tree has errors, aborting (use -f to force output)

I think this error is trying to say both the names and the unit
addresses are identical (and the latter exist), which isn't
technically incorrect, but not very useful either.

>>> but all the
>>> partitions must be named "partition",
>>
>> Bad binding, no cookie for you!
>
> Sorry, I dont understand want you want to say here.

I was trying to say that if the "partition" binding says the "name"
of every partition node should be "partition", then it is a very
bad binding.  The binding does _not_ say that though, the code is
wrong.

>> You cannot claim a name as generic as "partition" for this.  Pick
>> something else if you really must do things this way.
>
> You choose :)

Picking good names is hard work (more than 50% of all programming, heh).
You choose :-)  [Hint: something with a comma, maybe  
"linux,mtd,partition"
or similar -- I don't really care.  OTOH, it seems you won't need this
at all].


Segher

Re: [PATCH] [MTD] ofpart: Partitions at same address cannot have thesame name v3

[Ricardo Ribalda Delgado]

Hello

>
> Ah! =A0After writing my original reply, I read the documentation,
> and this had me confused.
>
> 4b08e14 looks wrong to me. =A0It also didn't update the documentation.


So, what shall we do, revert  4b08e14 or apply this patch changing the
name to "linux,mtd,partition" or similar.... ?

  Regards


--=20
Ricardo Ribalda
http://www.eps.uam.es/~rribalda/