| Message ID | 20190319223718.26131-1-jmkrzyszt@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mtd: rawnand: ams-delta: Drop board specific partition info | expand |
On Tue, Mar 19, 2019 at 11:37:18PM +0100, Janusz Krzysztofik wrote: > After recent modifications, only a hardcoded partition info makes > the driver device specific. Other than that, the driver uses GPIO > exclusively and can be used on any hardware. > > Drop the partition info and use MTD partition parser with default > list of partition types instead. > > Amstrad Delta users should append the followig partition info to their ^^^^^^^^ Should be "following". > kernel command line, possibly by embedding it in CONFIG_CMDLINE: > mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u-boot_params),\ > 256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved). For their > convenience, select CONFIG_MTD_CMDLINE_PARTS symbol from that board > Kconfig automatically if this NAND driver is also selected. > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> > Cc: Tony Lindgren <tony@atomide.com> Could we move the fixed partition setup to the board file instead? Otherwise this kind of change is not really nice for the users, as it will likely break existing setups. The default partition layout should remain the same. A.
Hi Aaro, Thanks for your review. On Wednesday, March 20, 2019 2:16:30 AM CET Aaro Koskinen wrote: > On Tue, Mar 19, 2019 at 11:37:18PM +0100, Janusz Krzysztofik wrote: > > After recent modifications, only a hardcoded partition info makes > > the driver device specific. Other than that, the driver uses GPIO > > exclusively and can be used on any hardware. > > > > Drop the partition info and use MTD partition parser with default > > list of partition types instead. > > > > Amstrad Delta users should append the followig partition info to their > ^^^^^^^^ > Should be "following". > > > kernel command line, possibly by embedding it in CONFIG_CMDLINE: > > mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u-boot_params),\ > > 256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved). For their > > convenience, select CONFIG_MTD_CMDLINE_PARTS symbol from that board > > Kconfig automatically if this NAND driver is also selected. > > > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> > > Cc: Tony Lindgren <tony@atomide.com> > > Could we move the fixed partition setup to the board file > instead? Otherwise this kind of change is not really nice for the users, > as it will likely break existing setups. The default partition layout > should remain the same. I'm wondering if it would be acceptable to pass partition info from a .dts file. I think that would be a better, more modern approach than adding a new header under include/linux/platform_data. The problem with a device tree based implementation is, I know of no u-boot version supporting both Amstrad Delta and FDT. However, I've already tested two solutions that work for me. One uses CONFIG_ARM_APPENDED_DTB and requires a user to manually append the blob to zImage and (re)generate uImage. I'm not sure how much more user- friendly it looks for you, compared to the command line version I proposed initially. If the above is not acceptable. I can propose still another approach. The blob is automagically built and embedded into the kernel with some assembler glue, then unflattened from the board init_machine(), somehow similar to the way drivers/of/unittest.c does it. Please advise which approach sounds best to you (platform_data, CONFIG_ARM_APPENDED_DTB or unittest like). Thanks, Janusz > > A. >
Hi, On Sun, Mar 24, 2019 at 05:48:22PM +0100, Janusz Krzysztofik wrote: > Hi Aaro, > > Thanks for your review. > > On Wednesday, March 20, 2019 2:16:30 AM CET Aaro Koskinen wrote: > > On Tue, Mar 19, 2019 at 11:37:18PM +0100, Janusz Krzysztofik wrote: > > > After recent modifications, only a hardcoded partition info makes > > > the driver device specific. Other than that, the driver uses GPIO > > > exclusively and can be used on any hardware. > > > > > > Drop the partition info and use MTD partition parser with default > > > list of partition types instead. > > > > > > Amstrad Delta users should append the followig partition info to their > > ^^^^^^^^ > > Should be "following". > > > > > kernel command line, possibly by embedding it in CONFIG_CMDLINE: > > > mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u-boot_params),\ > > > 256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved). For their > > > convenience, select CONFIG_MTD_CMDLINE_PARTS symbol from that board > > > Kconfig automatically if this NAND driver is also selected. > > > > > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> > > > Cc: Tony Lindgren <tony@atomide.com> > > > > Could we move the fixed partition setup to the board file > > instead? Otherwise this kind of change is not really nice for the users, > > as it will likely break existing setups. The default partition layout > > should remain the same. > > I'm wondering if it would be acceptable to pass partition info from a .dts > file. I think that would be a better, more modern approach than adding a new > header under include/linux/platform_data. Hmm, I thought there was some generic way to define partitions without adding any new headers. But if that is not possible, then I guess your CMDLINE proposal is the preferred one.. A.
Hi, > Am 24.03.2019 um 19:59 schrieb Aaro Koskinen <aaro.koskinen@iki.fi>: > > Hi, > > On Sun, Mar 24, 2019 at 05:48:22PM +0100, Janusz Krzysztofik wrote: >> Hi Aaro, >> >> Thanks for your review. >> >> On Wednesday, March 20, 2019 2:16:30 AM CET Aaro Koskinen wrote: >>> On Tue, Mar 19, 2019 at 11:37:18PM +0100, Janusz Krzysztofik wrote: >>>> After recent modifications, only a hardcoded partition info makes >>>> the driver device specific. Other than that, the driver uses GPIO >>>> exclusively and can be used on any hardware. >>>> >>>> Drop the partition info and use MTD partition parser with default >>>> list of partition types instead. >>>> >>>> Amstrad Delta users should append the followig partition info to their >>> ^^^^^^^^ >>> Should be "following". >>> >>>> kernel command line, possibly by embedding it in CONFIG_CMDLINE: >>>> mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u-boot_params),\ >>>> 256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved). For their >>>> convenience, select CONFIG_MTD_CMDLINE_PARTS symbol from that board >>>> Kconfig automatically if this NAND driver is also selected. >>>> >>>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> >>>> Cc: Tony Lindgren <tony@atomide.com> >>> >>> Could we move the fixed partition setup to the board file >>> instead? Otherwise this kind of change is not really nice for the users, >>> as it will likely break existing setups. The default partition layout >>> should remain the same. >> >> I'm wondering if it would be acceptable to pass partition info from a .dts >> file. I think that would be a better, more modern approach than adding a new >> header under include/linux/platform_data. > > Hmm, I thought there was some generic way to define partitions without > adding any new headers. But if that is not possible, then I guess your > CMDLINE proposal is the preferred one.. I am not sure what you exactly need, but partitions can be defined in the DTS as children of some NAND drivers. Example: arch/arm/boot/dts/omap3-beagle.dts So this design pattern could be copied instead of using CMDLINE.
Hi, On Sunday, March 24, 2019 7:59:32 PM CET Aaro Koskinen wrote: > Hi, > > On Sun, Mar 24, 2019 at 05:48:22PM +0100, Janusz Krzysztofik wrote: > > Hi Aaro, > > > > Thanks for your review. > > > > On Wednesday, March 20, 2019 2:16:30 AM CET Aaro Koskinen wrote: > > > On Tue, Mar 19, 2019 at 11:37:18PM +0100, Janusz Krzysztofik wrote: > > > > After recent modifications, only a hardcoded partition info makes > > > > the driver device specific. Other than that, the driver uses GPIO > > > > exclusively and can be used on any hardware. > > > > > > > > Drop the partition info and use MTD partition parser with default > > > > list of partition types instead. > > > > > > > > Amstrad Delta users should append the followig partition info to their > > > ^^^^^^^^ > > > Should be "following". > > > > > > > kernel command line, possibly by embedding it in CONFIG_CMDLINE: > > > > mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u- boot_params),\ > > > > 256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved). For their > > > > convenience, select CONFIG_MTD_CMDLINE_PARTS symbol from that board > > > > Kconfig automatically if this NAND driver is also selected. > > > > > > > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> > > > > Cc: Tony Lindgren <tony@atomide.com> > > > > > > Could we move the fixed partition setup to the board file > > > instead? Otherwise this kind of change is not really nice for the users, > > > as it will likely break existing setups. The default partition layout > > > should remain the same. > > > > I'm wondering if it would be acceptable to pass partition info from a .dts > > file. I think that would be a better, more modern approach than adding a new > > header under include/linux/platform_data. > > Hmm, I thought there was some generic way to define partitions without > adding any new headers. But if that is not possible, then I guess your > CMDLINE proposal is the preferred one.. I could for example reuse (or abuse) struct gpio_nand_platdata defined in include/linux/mtd/nand-gpio.h for use with drivers/mtd/nand/raw/gpio.c, but I'm not sure if hi-jacking a header that belongs to another driver would be an elegant solution. Thanks, Janusz > > A. >
Hi, On Sunday, March 24, 2019 8:24:48 PM CET H. Nikolaus Schaller wrote: > Hi, > > > Am 24.03.2019 um 19:59 schrieb Aaro Koskinen <aaro.koskinen@iki.fi>: > > > > Hi, > > > > On Sun, Mar 24, 2019 at 05:48:22PM +0100, Janusz Krzysztofik wrote: > >> Hi Aaro, > >> > >> Thanks for your review. > >> > >> On Wednesday, March 20, 2019 2:16:30 AM CET Aaro Koskinen wrote: > >>> On Tue, Mar 19, 2019 at 11:37:18PM +0100, Janusz Krzysztofik wrote: > >>>> After recent modifications, only a hardcoded partition info makes > >>>> the driver device specific. Other than that, the driver uses GPIO > >>>> exclusively and can be used on any hardware. > >>>> > >>>> Drop the partition info and use MTD partition parser with default > >>>> list of partition types instead. > >>>> > >>>> Amstrad Delta users should append the followig partition info to their > >>> ^^^^^^^^ > >>> Should be "following". > >>> > >>>> kernel command line, possibly by embedding it in CONFIG_CMDLINE: > >>>> mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u-boot_params), \ > >>>> 256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved). For their > >>>> convenience, select CONFIG_MTD_CMDLINE_PARTS symbol from that board > >>>> Kconfig automatically if this NAND driver is also selected. > >>>> > >>>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> > >>>> Cc: Tony Lindgren <tony@atomide.com> > >>> > >>> Could we move the fixed partition setup to the board file > >>> instead? Otherwise this kind of change is not really nice for the users, > >>> as it will likely break existing setups. The default partition layout > >>> should remain the same. > >> > >> I'm wondering if it would be acceptable to pass partition info from a .dts > >> file. I think that would be a better, more modern approach than adding a new > >> header under include/linux/platform_data. > > > > Hmm, I thought there was some generic way to define partitions without > > adding any new headers. But if that is not possible, then I guess your > > CMDLINE proposal is the preferred one.. > > I am not sure what you exactly need, but partitions can be defined in > the DTS as children of some NAND drivers. Example: > arch/arm/boot/dts/omap3-beagle.dts > So this design pattern could be copied instead of using CMDLINE. The problem is, OMAP1 has no device tree support. Other than that, your proposed approach already works for me locally with some basic support for device tree added to the board file. Thanks, Janusz
diff --git a/arch/arm/mach-omap1/Kconfig b/arch/arm/mach-omap1/Kconfig index c4694f26b5c4..62cf20f22828 100644 --- a/arch/arm/mach-omap1/Kconfig +++ b/arch/arm/mach-omap1/Kconfig @@ -171,6 +171,7 @@ config MACH_AMS_DELTA select LEDS_GPIO_REGISTER select REGULATOR select REGULATOR_FIXED_VOLTAGE + select MTD_CMDLINE_PARTS if MTD_NAND_AMS_DELTA help Support for the Amstrad E3 (codename Delta) videophone. Say Y here if you have such a device. diff --git a/drivers/mtd/nand/raw/ams-delta.c b/drivers/mtd/nand/raw/ams-delta.c index 8312182088c1..2e8e37ea549a 100644 --- a/drivers/mtd/nand/raw/ams-delta.c +++ b/drivers/mtd/nand/raw/ams-delta.c @@ -41,31 +41,6 @@ struct ams_delta_nand { bool data_in; }; -/* - * Define partitions for flash devices - */ - -static const struct mtd_partition partition_info[] = { - { .name = "Kernel", - .offset = 0, - .size = 3 * SZ_1M + SZ_512K }, - { .name = "u-boot", - .offset = 3 * SZ_1M + SZ_512K, - .size = SZ_256K }, - { .name = "u-boot params", - .offset = 3 * SZ_1M + SZ_512K + SZ_256K, - .size = SZ_256K }, - { .name = "Amstrad LDR", - .offset = 4 * SZ_1M, - .size = SZ_256K }, - { .name = "File system", - .offset = 4 * SZ_1M + 1 * SZ_256K, - .size = 27 * SZ_1M }, - { .name = "PBL reserved", - .offset = 32 * SZ_1M - 3 * SZ_256K, - .size = 3 * SZ_256K }, -}; - static void ams_delta_write_commit(struct ams_delta_nand *priv) { gpiod_set_value(priv->gpiod_nwe, 0); @@ -315,8 +290,7 @@ static int ams_delta_init(struct platform_device *pdev) return err; /* Register the partitions */ - err = mtd_device_register(mtd, partition_info, - ARRAY_SIZE(partition_info)); + err = mtd_device_parse_register(mtd, NULL, NULL, NULL, 0); if (err) goto err_nand_cleanup;
After recent modifications, only a hardcoded partition info makes the driver device specific. Other than that, the driver uses GPIO exclusively and can be used on any hardware. Drop the partition info and use MTD partition parser with default list of partition types instead. Amstrad Delta users should append the followig partition info to their kernel command line, possibly by embedding it in CONFIG_CMDLINE: mtdparts=ams-delta-nand:3584k(Kernel),256k(u-boot),256k(u-boot_params),\ 256k(Amstrad_LDR),27m(File_system),768k(PBL_reserved). For their convenience, select CONFIG_MTD_CMDLINE_PARTS symbol from that board Kconfig automatically if this NAND driver is also selected. Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com> Cc: Tony Lindgren <tony@atomide.com> --- arch/arm/mach-omap1/Kconfig | 1 + drivers/mtd/nand/raw/ams-delta.c | 28 +--------------------------- 2 files changed, 2 insertions(+), 27 deletions(-)