| Message ID | 1370962138-9631-2-git-send-email-florian.vaussard@epfl.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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_1G 0x40000000 > +#define SZ_2G 0x80000000 > + > +#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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 0x40000000 >> +#define SZ_2G 0x80000000 >> + >> +#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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 11 Jun 2013 16:48:56 +0200, Florian Vaussard <florian.vaussard@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.vaussard@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 0000000..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 0000000..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 0x00000001 > +#define SZ_2 0x00000002 > +#define SZ_4 0x00000004 > +#define SZ_8 0x00000008 > +#define SZ_16 0x00000010 > +#define SZ_32 0x00000020 > +#define SZ_64 0x00000040 > +#define SZ_128 0x00000080 > +#define SZ_256 0x00000100 > +#define SZ_512 0x00000200 > + > +#define SZ_1K 0x00000400 > +#define SZ_2K 0x00000800 > +#define SZ_4K 0x00001000 > +#define SZ_8K 0x00002000 > +#define SZ_16K 0x00004000 > +#define SZ_32K 0x00008000 > +#define SZ_64K 0x00010000 > +#define SZ_128K 0x00020000 > +#define SZ_256K 0x00040000 > +#define SZ_512K 0x00080000 > + > +#define SZ_1M 0x00100000 > +#define SZ_2M 0x00200000 > +#define SZ_4M 0x00400000 > +#define SZ_8M 0x00800000 > +#define SZ_16M 0x01000000 > +#define SZ_32M 0x02000000 > +#define SZ_64M 0x04000000 > +#define SZ_128M 0x08000000 > +#define SZ_256M 0x10000000 > +#define SZ_512M 0x20000000 > + > +#define SZ_1G 0x40000000 > +#define SZ_2G 0x80000000 > + > +#endif > + > -- > 1.7.5.4 > > _______________________________________________ > devicetree-discuss mailing list > devicetree-discuss@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/devicetree-discuss
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.vaussard@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 = 0x00000 */ .size = 4 * NAND_BLOCK_SIZE, .mask_flags = MTD_WRITEABLE }, <snip...> { .name = "rootfs", .offset = MTDPART_OFS_APPEND, /* Offset = 0x680000 */ .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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/dt-bindings/mtd/partitions.h b/include/dt-bindings/mtd/partitions.h new file mode 100644 index 0000000..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 0000000..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 0x00000001 +#define SZ_2 0x00000002 +#define SZ_4 0x00000004 +#define SZ_8 0x00000008 +#define SZ_16 0x00000010 +#define SZ_32 0x00000020 +#define SZ_64 0x00000040 +#define SZ_128 0x00000080 +#define SZ_256 0x00000100 +#define SZ_512 0x00000200 + +#define SZ_1K 0x00000400 +#define SZ_2K 0x00000800 +#define SZ_4K 0x00001000 +#define SZ_8K 0x00002000 +#define SZ_16K 0x00004000 +#define SZ_32K 0x00008000 +#define SZ_64K 0x00010000 +#define SZ_128K 0x00020000 +#define SZ_256K 0x00040000 +#define SZ_512K 0x00080000 + +#define SZ_1M 0x00100000 +#define SZ_2M 0x00200000 +#define SZ_4M 0x00400000 +#define SZ_8M 0x00800000 +#define SZ_16M 0x01000000 +#define SZ_32M 0x02000000 +#define SZ_64M 0x04000000 +#define SZ_128M 0x08000000 +#define SZ_256M 0x10000000 +#define SZ_512M 0x20000000 + +#define SZ_1G 0x40000000 +#define SZ_2G 0x80000000 + +#endif +
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.vaussard@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