Re: [PATCH 1/3] ARM: dts: Add headers with constants for MTD partitions
Hello,
Thank you for the review.
On 06/12/2013 03:05 PM, Grant Likely wrote:
On Tue, 11 Jun 2013 16:48:56 +0200, Florian Vaussard florian.vauss...@epfl.ch
wrote:
These constants can be used to easily declare MTD partitions inside
DTS.
The constants MTDPART_OFS_* are purposely not included. Indeed,
parse_ofpart_partitions() is expecting u64, but a DT cell is u32.
Negative constants, as defined by MTDPART_OFS_*, would be wrongly
The DT binding uses the number of cells defined by #address-cells. It is
not fixed to a u32 or a u64
The message was ill-formatted, sorry. As an address cell is u32, and as
parse_ofpart_partitions() is storing the value inside u64 without checking
for sign extension (as one assumes to have only positive offsets),
passing a negative value would require 2 address cells, making it more
difficult
for the DT user. But as Stephen Warren noticed, it is probably desirable
to specify sizes = 4GB, thus I will think about a way to easily handle
such case.
Anyway, MTDPART_OFS_* would probably face the same objection raised by
you for
MTDPART_SIZ_FULL.
interpreted by parse_ofpart_partitions(). Two cells should be
used to correctly encode the negative constants, but this breaks
current usage.
The binding doesn't even allow for shortcuts like MTDPART_SIZ_FULL. If a
partition fills the whole device, then the reg property should include
the actual size. If the code is allowing '0' to be used to mean
MTDPART_SIZ_FULL, then that is a bug that needs to be fixed.
The root problem is that many System on Module, like the Gumstix Overo, are
shipped with various NAND sizes depending on the version or even the
manufacturing
period. Supporting such a diversity would painfully duplicates lots of DT
code and clutter the arch/arm/boot/dts/ directory with dozens of slightly-
different versions. I believe that determining the NAND size is better done
at probe time, and this is what is currently done in legacy board files:
static struct mtd_partition overo_nand_partitions[] = {
{
.name = xloader,
.offset = 0,/* Offset =
0x0 */
.size = 4 * NAND_BLOCK_SIZE,
.mask_flags = MTD_WRITEABLE
},
snip...
{
.name = rootfs,
.offset = MTDPART_OFS_APPEND, /* Offset =
0x68 */
.size = MTDPART_SIZ_FULL,
},
};
Moreover, I do not see such strict restriction in the OF norm. If I
refer to IEEE
1275-1994 page 174, for the definition of the reg property, it is written:
The interpretation of the size entries is dependent on the parent bus.
Nevertheless, if such an approach is not acceptable, could we think about an
alternative solution? Like a boolean property mtd,append-and-fill that
would
replace the reg property and tell the MTD core to compute the
partition size
at probe time, like what is currently done with board files?
Best regards,
Florian
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] ARM: dts: Add headers with constants for MTD partitions
On Tue, 11 Jun 2013 16:48:56 +0200, Florian Vaussard florian.vauss...@epfl.ch wrote: These constants can be used to easily declare MTD partitions inside DTS. The constants MTDPART_OFS_* are purposely not included. Indeed, parse_ofpart_partitions() is expecting u64, but a DT cell is u32. Negative constants, as defined by MTDPART_OFS_*, would be wrongly The DT binding uses the number of cells defined by #address-cells. It is not fixed to a u32 or a u64 interpreted by parse_ofpart_partitions(). Two cells should be used to correctly encode the negative constants, but this breaks current usage. The binding doesn't even allow for shortcuts like MTDPART_SIZ_FULL. If a partition fills the whole device, then the reg property should include the actual size. If the code is allowing '0' to be used to mean MTDPART_SIZ_FULL, then that is a bug that needs to be fixed. Please drop the mtd/partitions.h hunk from this patch. g. Signed-off-by: Florian Vaussard florian.vauss...@epfl.ch --- include/dt-bindings/mtd/partitions.h | 12 include/dt-bindings/sizes.h | 52 ++ 2 files changed, 64 insertions(+), 0 deletions(-) create mode 100644 include/dt-bindings/mtd/partitions.h create mode 100644 include/dt-bindings/sizes.h diff --git a/include/dt-bindings/mtd/partitions.h b/include/dt-bindings/mtd/partitions.h new file mode 100644 index 000..7dfa676 --- /dev/null +++ b/include/dt-bindings/mtd/partitions.h @@ -0,0 +1,12 @@ +/* + * This header provides constants used with MTD partitions. + */ + +#ifndef _DT_BINDINGS_MTD_PARTITIONS_H +#define _DT_BINDINGS_MTD_PARTITIONS_H + +/* Partition size */ +#define MTDPART_SIZ_FULL 0 + +#endif + diff --git a/include/dt-bindings/sizes.h b/include/dt-bindings/sizes.h new file mode 100644 index 000..995f2de --- /dev/null +++ b/include/dt-bindings/sizes.h @@ -0,0 +1,52 @@ +/* + * This header provides size constants. + * + * Original version: + * include/linux/sizes.h + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef _DT_BINDINGS_SIZES_H +#define _DT_BINDINGS_SIZES_H + +#define SZ_1 0x0001 +#define SZ_2 0x0002 +#define SZ_4 0x0004 +#define SZ_8 0x0008 +#define SZ_160x0010 +#define SZ_320x0020 +#define SZ_640x0040 +#define SZ_128 0x0080 +#define SZ_256 0x0100 +#define SZ_512 0x0200 + +#define SZ_1K0x0400 +#define SZ_2K0x0800 +#define SZ_4K0x1000 +#define SZ_8K0x2000 +#define SZ_16K 0x4000 +#define SZ_32K 0x8000 +#define SZ_64K 0x0001 +#define SZ_128K 0x0002 +#define SZ_256K 0x0004 +#define SZ_512K 0x0008 + +#define SZ_1M0x0010 +#define SZ_2M0x0020 +#define SZ_4M0x0040 +#define SZ_8M0x0080 +#define SZ_16M 0x0100 +#define SZ_32M 0x0200 +#define SZ_64M 0x0400 +#define SZ_128M 0x0800 +#define SZ_256M 0x1000 +#define SZ_512M 0x2000 + +#define SZ_1G0x4000 +#define SZ_2G0x8000 + +#endif + -- 1.7.5.4 ___ devicetree-discuss mailing list devicetree-disc...@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss -- Grant Likely, B.Sc, P.Eng. Secret Lab Technologies, Ltd. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] ARM: dts: Add headers with constants for MTD partitions
These constants can be used to easily declare MTD partitions inside DTS. The constants MTDPART_OFS_* are purposely not included. Indeed, parse_ofpart_partitions() is expecting u64, but a DT cell is u32. Negative constants, as defined by MTDPART_OFS_*, would be wrongly interpreted by parse_ofpart_partitions(). Two cells should be used to correctly encode the negative constants, but this breaks current usage. Signed-off-by: Florian Vaussard florian.vauss...@epfl.ch --- include/dt-bindings/mtd/partitions.h | 12 include/dt-bindings/sizes.h | 52 ++ 2 files changed, 64 insertions(+), 0 deletions(-) create mode 100644 include/dt-bindings/mtd/partitions.h create mode 100644 include/dt-bindings/sizes.h diff --git a/include/dt-bindings/mtd/partitions.h b/include/dt-bindings/mtd/partitions.h new file mode 100644 index 000..7dfa676 --- /dev/null +++ b/include/dt-bindings/mtd/partitions.h @@ -0,0 +1,12 @@ +/* + * This header provides constants used with MTD partitions. + */ + +#ifndef _DT_BINDINGS_MTD_PARTITIONS_H +#define _DT_BINDINGS_MTD_PARTITIONS_H + +/* Partition size */ +#define MTDPART_SIZ_FULL 0 + +#endif + diff --git a/include/dt-bindings/sizes.h b/include/dt-bindings/sizes.h new file mode 100644 index 000..995f2de --- /dev/null +++ b/include/dt-bindings/sizes.h @@ -0,0 +1,52 @@ +/* + * This header provides size constants. + * + * Original version: + * include/linux/sizes.h + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef _DT_BINDINGS_SIZES_H +#define _DT_BINDINGS_SIZES_H + +#define SZ_1 0x0001 +#define SZ_2 0x0002 +#define SZ_4 0x0004 +#define SZ_8 0x0008 +#define SZ_16 0x0010 +#define SZ_32 0x0020 +#define SZ_64 0x0040 +#define SZ_128 0x0080 +#define SZ_256 0x0100 +#define SZ_512 0x0200 + +#define SZ_1K 0x0400 +#define SZ_2K 0x0800 +#define SZ_4K 0x1000 +#define SZ_8K 0x2000 +#define SZ_16K 0x4000 +#define SZ_32K 0x8000 +#define SZ_64K 0x0001 +#define SZ_128K0x0002 +#define SZ_256K0x0004 +#define SZ_512K0x0008 + +#define SZ_1M 0x0010 +#define SZ_2M 0x0020 +#define SZ_4M 0x0040 +#define SZ_8M 0x0080 +#define SZ_16M 0x0100 +#define SZ_32M 0x0200 +#define SZ_64M 0x0400 +#define SZ_128M0x0800 +#define SZ_256M0x1000 +#define SZ_512M0x2000 + +#define SZ_1G 0x4000 +#define SZ_2G 0x8000 + +#endif + -- 1.7.5.4 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] ARM: dts: Add headers with constants for MTD partitions
On 06/11/2013 08:48 AM, Florian Vaussard wrote: These constants can be used to easily declare MTD partitions inside DTS. The constants MTDPART_OFS_* are purposely not included. Indeed, parse_ofpart_partitions() is expecting u64, but a DT cell is u32. Negative constants, as defined by MTDPART_OFS_*, would be wrongly interpreted by parse_ofpart_partitions(). Two cells should be used to correctly encode the negative constants, but this breaks current usage. I think addition of common headers like this needs an ack from Grant/Rob. I CC'd them here. diff --git a/include/dt-bindings/mtd/partitions.h b/include/dt-bindings/mtd/partitions.h + * This header provides constants used with MTD partitions. ... +/* Partition size */ +#define MTDPART_SIZ_FULL 0 Which binding document in Documentation/devicetree/bindings is this definition associated with? The comment above should really mention this. Documentation/devicetree/bindings/mtd/partition.txt doesn't seem to mention this value. diff --git a/include/dt-bindings/sizes.h b/include/dt-bindings/sizes.h ... +#define SZ_1G0x4000 +#define SZ_2G0x8000 + +#endif For MTD partitions specifically, SZ_4G and onwards would be useful in theory, although that would end up putting two cell values into a single macro. and then the values couldn't be added/or'd together. So, I'm not really sure if we want to add those larger values, but food for thought... -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] ARM: dts: Add headers with constants for MTD partitions
Hello Stephen, On 06/11/2013 06:24 PM, Stephen Warren wrote: On 06/11/2013 08:48 AM, Florian Vaussard wrote: These constants can be used to easily declare MTD partitions inside DTS. The constants MTDPART_OFS_* are purposely not included. Indeed, parse_ofpart_partitions() is expecting u64, but a DT cell is u32. Negative constants, as defined by MTDPART_OFS_*, would be wrongly interpreted by parse_ofpart_partitions(). Two cells should be used to correctly encode the negative constants, but this breaks current usage. I think addition of common headers like this needs an ack from Grant/Rob. I CC'd them here. Indeed. Thank you. diff --git a/include/dt-bindings/mtd/partitions.h b/include/dt-bindings/mtd/partitions.h + * This header provides constants used with MTD partitions. ... +/* Partition size */ +#define MTDPART_SIZ_FULL 0 Which binding document in Documentation/devicetree/bindings is this definition associated with? The comment above should really mention this. Documentation/devicetree/bindings/mtd/partition.txt doesn't seem to mention this value. Mmmh I was not seeing this as a DT binding, strictly speaking. It was already used with legacy board files. Otherwise we should also update the binding for constants related to GPIO, IRQ,... But I agree that a line inside the documentation never killed someone. diff --git a/include/dt-bindings/sizes.h b/include/dt-bindings/sizes.h ... +#define SZ_1G 0x4000 +#define SZ_2G 0x8000 + +#endif For MTD partitions specifically, SZ_4G and onwards would be useful in theory, although that would end up putting two cell values into a single macro. and then the values couldn't be added/or'd together. So, I'm not really sure if we want to add those larger values, but food for thought... It is maybe feasible to define a macro splitting the u64 into two u32 cells? But this can be done afterwards when need arises. Best regards, Florian -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
