diff mbox series

mtd: rawnand: ams-delta: Drop board specific partition info

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

Commit Message

Janusz Krzysztofik March 19, 2019, 10:37 p.m. UTC
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(-)

Comments

Aaro Koskinen March 20, 2019, 1:16 a.m. UTC | #1
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.
Janusz Krzysztofik March 24, 2019, 4:48 p.m. UTC | #2
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.
>
Aaro Koskinen March 24, 2019, 6:59 p.m. UTC | #3
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.
H. Nikolaus Schaller March 24, 2019, 7:24 p.m. UTC | #4
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.
Janusz Krzysztofik March 24, 2019, 8:30 p.m. UTC | #5
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.
>
Janusz Krzysztofik March 24, 2019, 8:40 p.m. UTC | #6
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 mbox series

Patch

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;