[PATCH] mtd/powernv_flash: Enable partition support

[PATCH] mtd/powernv_flash: Enable partition support

[Timothy Pearson]

On certain systems, such as the Talos II, skiboot emits a partition
table for the main PNOR MTD device in the generated device tree.

Allow this partition table to be parsed and the partitions to be
exposed via MTD device partition nodes.

Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com>
---
 drivers/mtd/devices/powernv_flash.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c
index c1312b141ae0..a2c6b30925c4 100644
--- a/drivers/mtd/devices/powernv_flash.c
+++ b/drivers/mtd/devices/powernv_flash.c
@@ -2,6 +2,7 @@
  * OPAL PNOR flash MTD abstraction
  *
  * Copyright IBM 2015
+ * Copyright Raptor Engineering, LLC 2018
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -259,8 +260,10 @@ static int powernv_flash_probe(struct platform_device *pdev)
 	/*
 	 * The current flash that skiboot exposes is one contiguous flash chip
 	 * with an ffs partition at the start, it should prove easier for users
-	 * to deal with partitions or not as they see fit
+	 * to deal with partitions or not as they see fit.  skiboot places this
+	 * on the first MTD partition.
 	 */
+	mtd_set_of_node(&data->mtd, dev->of_node);
 	return mtd_device_register(&data->mtd, NULL, 0);
 }
 
-- 
2.18.0

Re: [PATCH] mtd/powernv_flash: Enable partition support

[Rafał Miłecki]

On Mon, 23 Jul 2018 at 11:02, Timothy Pearson
<tpearson@raptorengineering.com> wrote:
> On certain systems, such as the Talos II, skiboot emits a partition
> table for the main PNOR MTD device in the generated device tree.
>
> Allow this partition table to be parsed and the partitions to be
> exposed via MTD device partition nodes.
>
> Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com>

This already has been handled in the:
[PATCH] mtd: powernv_flash: set of_node in mtd's dev
https://patchwork.ozlabs.org/patch/943327/

I marked you as reporter, I Cc-ed you, Boris replied to it. I didn't
expect you to miss that change.

Re: [PATCH] mtd/powernv_flash: Enable partition support

[Timothy Pearson]

On 07/23/2018 05:14 AM, Rafał Miłecki wrote:
> On Mon, 23 Jul 2018 at 11:02, Timothy Pearson
> <tpearson@raptorengineering.com> wrote:
>> On certain systems, such as the Talos II, skiboot emits a partition
>> table for the main PNOR MTD device in the generated device tree.
>>
>> Allow this partition table to be parsed and the partitions to be
>> exposed via MTD device partition nodes.
>>
>> Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com>
> 
> This already has been handled in the:
> [PATCH] mtd: powernv_flash: set of_node in mtd's dev
> https://patchwork.ozlabs.org/patch/943327/
> 
> I marked you as reporter, I Cc-ed you, Boris replied to it. I didn't
> expect you to miss that change.

Thank you for the information.  I did miss the change, not sure how, so
my apologies.

-- 
Timothy Pearson
Raptor Engineering
+1 (415) 727-8645 (direct line)
+1 (512) 690-0200 (switchboard)
https://www.raptorengineering.com

[PATCH] mtd/powernv_flash: Enable partition support

[Timothy Pearson]

On certain systems, such as the Talos II, skiboot emits a partition
table for the main PNOR MTD device in the generated device tree.

Allow this partition table to be parsed and the partitions to be
exposed via MTD device partition nodes.
---
 drivers/mtd/devices/powernv_flash.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c
index 26f9feaa5d17..f76045f78221 100644
--- a/drivers/mtd/devices/powernv_flash.c
+++ b/drivers/mtd/devices/powernv_flash.c
@@ -2,6 +2,7 @@
  * OPAL PNOR flash MTD abstraction
  *
  * Copyright IBM 2015
+ * Copyright Raptor Engineering, LLC 2018
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -47,6 +48,8 @@ enum flash_op {
 	FLASH_OP_ERASE,
 };
 
+static char const * const part_probes[] = { "ofpart", NULL };
+
 /*
  * Don't return -ERESTARTSYS if we can't get a token, the MTD core
  * might have split up the call from userspace and called into the
@@ -267,9 +270,14 @@ static int powernv_flash_probe(struct platform_device *pdev)
 	/*
 	 * The current flash that skiboot exposes is one contiguous flash chip
 	 * with an ffs partition at the start, it should prove easier for users
-	 * to deal with partitions or not as they see fit
+	 * to deal with partitions or not as they see fit.  skitboot places this
+	 * on the first MTD partition.
+	 *
+	 * Certain partitions may also be exposed to the host, such as the boot
+	 * kernel firmware partition.
 	 */
-	return mtd_device_register(&data->mtd, NULL, 0);
+	mtd_set_of_node(&data->mtd, dev->of_node);
+	return mtd_device_parse_register(&data->mtd, part_probes, NULL, NULL, 0);
 }
 
 /**
-- 
2.16.1

Re: [PATCH] mtd/powernv_flash: Enable partition support

[Rafał Miłecki]

Hi Tomthy,

On 25 March 2018 at 22:05, Timothy Pearson
<tpearson@raptorengineering.com> wrote:
> On certain systems, such as the Talos II, skiboot emits a partition
> table for the main PNOR MTD device in the generated device tree.
>
> Allow this partition table to be parsed and the partitions to be
> exposed via MTD device partition nodes.

Your commit is missing a Signed-off-by


> ---
>  drivers/mtd/devices/powernv_flash.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c
> index 26f9feaa5d17..f76045f78221 100644
> --- a/drivers/mtd/devices/powernv_flash.c
> +++ b/drivers/mtd/devices/powernv_flash.c
> @@ -2,6 +2,7 @@
>   * OPAL PNOR flash MTD abstraction
>   *
>   * Copyright IBM 2015
> + * Copyright Raptor Engineering, LLC 2018
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License as published by
> @@ -47,6 +48,8 @@ enum flash_op {
>         FLASH_OP_ERASE,
>  };
>
> +static char const * const part_probes[] = { "ofpart", NULL };
> +
>  /*
>   * Don't return -ERESTARTSYS if we can't get a token, the MTD core
>   * might have split up the call from userspace and called into the
> @@ -267,9 +270,14 @@ static int powernv_flash_probe(struct platform_device *pdev)
>         /*
>          * The current flash that skiboot exposes is one contiguous flash chip
>          * with an ffs partition at the start, it should prove easier for users
> -        * to deal with partitions or not as they see fit
> +        * to deal with partitions or not as they see fit.  skitboot places this
> +        * on the first MTD partition.
> +        *
> +        * Certain partitions may also be exposed to the host, such as the boot
> +        * kernel firmware partition.
>          */
> -       return mtd_device_register(&data->mtd, NULL, 0);
> +       mtd_set_of_node(&data->mtd, dev->of_node);
> +       return mtd_device_parse_register(&data->mtd, part_probes, NULL, NULL, 0);

It seems the only change introduced by this patch is passing a list of
parsers. That way you override a default list of parsers:
"cmdlinepart", "ofpart", NULL
with a custom one:
"ofpart", NULL

I don't see how it really changes anything/much. Can you explain it
please? It seems that the only purpose of your change is to don't
probe "cmdlinepart" parser which reads "mtdparts=" from the cmd line.

Which parser exactly is supposed to detect partitions on your device?
"fixed-partitions"? "ofoldpart"? Does your DTB contains "compatible"
property for flash node? Please paste the relevant part of your DTB if
applicable.


>  }
>
>  /**

-- 
Rafał

Re: [PATCH] mtd/powernv_flash: Enable partition support

[Timothy Pearson]

Will resend with Signed-off-by.

Before this patch, the driver was not picking up the OF-provided
partition list.  It seemed specifically designed to register only one
large partition covering the entire PNOR; mtd_device_register() does not
run parsing of any type AFAIK.

Example DT:

flash@0 {
	compatible = "ibm,opal-flash";
	ibm,flash-block-size = <0x10000>;
	ibm,opal-id = <0x0>;
	no-erase;
	#address-cells = <0x1>;
	#size-cells = <0x1>;
	phandle = <0x161>;
	reg = <0x0 0x4000000>;

	partitions {
		compatible = "fixed-partitions";
		#address-cells = <0x1>;
		#size-cells = <0x1>;
		phandle = <0x162>;

		partition@0 {
			label = "PNOR";
			phandle = <0x168>;
			reg = <0x0 0x4000000>;
		};

		partition@3389000 {
			read-only;
			label = "IMA_CATALOG";
			phandle = <0x165>;
			reg = <0x3389000 0x40000>;
		};

		partition@1a21000 {
			read-only;
			label = "BOOTKERNEL";
			phandle = <0x163>;
			reg = <0x1a21000 0x1800000>;
		};

		partition@3388000 {
			read-only;
			label = "VERSION";
			phandle = <0x166>;
			reg = <0x3388000 0x1000>;
		};

		partition@3710000 {
			label = "BOOTKERNFW";
			phandle = <0x167>;
			reg = <0x3710000 0x600000>;
		};

		partition@3344000 {
			read-only;
			label = "CAPP";
			phandle = <0x164>;
			reg = <0x3344000 0x24000>;
		};
	};
};

On 03/29/2018 12:29 AM, Rafał Miłecki wrote:
> Hi Tomthy,
> 
> On 25 March 2018 at 22:05, Timothy Pearson
> <tpearson@raptorengineering.com> wrote:
>> On certain systems, such as the Talos II, skiboot emits a partition
>> table for the main PNOR MTD device in the generated device tree.
>>
>> Allow this partition table to be parsed and the partitions to be
>> exposed via MTD device partition nodes.
> 
> Your commit is missing a Signed-off-by
> 
> 
>> ---
>>  drivers/mtd/devices/powernv_flash.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/devices/powernv_flash.c b/drivers/mtd/devices/powernv_flash.c
>> index 26f9feaa5d17..f76045f78221 100644
>> --- a/drivers/mtd/devices/powernv_flash.c
>> +++ b/drivers/mtd/devices/powernv_flash.c
>> @@ -2,6 +2,7 @@
>>   * OPAL PNOR flash MTD abstraction
>>   *
>>   * Copyright IBM 2015
>> + * Copyright Raptor Engineering, LLC 2018
>>   *
>>   * This program is free software; you can redistribute it and/or modify
>>   * it under the terms of the GNU General Public License as published by
>> @@ -47,6 +48,8 @@ enum flash_op {
>>         FLASH_OP_ERASE,
>>  };
>>
>> +static char const * const part_probes[] = { "ofpart", NULL };
>> +
>>  /*
>>   * Don't return -ERESTARTSYS if we can't get a token, the MTD core
>>   * might have split up the call from userspace and called into the
>> @@ -267,9 +270,14 @@ static int powernv_flash_probe(struct platform_device *pdev)
>>         /*
>>          * The current flash that skiboot exposes is one contiguous flash chip
>>          * with an ffs partition at the start, it should prove easier for users
>> -        * to deal with partitions or not as they see fit
>> +        * to deal with partitions or not as they see fit.  skitboot places this
>> +        * on the first MTD partition.
>> +        *
>> +        * Certain partitions may also be exposed to the host, such as the boot
>> +        * kernel firmware partition.
>>          */
>> -       return mtd_device_register(&data->mtd, NULL, 0);
>> +       mtd_set_of_node(&data->mtd, dev->of_node);
>> +       return mtd_device_parse_register(&data->mtd, part_probes, NULL, NULL, 0);
> 
> It seems the only change introduced by this patch is passing a list of
> parsers. That way you override a default list of parsers:
> "cmdlinepart", "ofpart", NULL
> with a custom one:
> "ofpart", NULL
> 
> I don't see how it really changes anything/much. Can you explain it
> please? It seems that the only purpose of your change is to don't
> probe "cmdlinepart" parser which reads "mtdparts=" from the cmd line.
> 
> Which parser exactly is supposed to detect partitions on your device?
> "fixed-partitions"? "ofoldpart"? Does your DTB contains "compatible"
> property for flash node? Please paste the relevant part of your DTB if
> applicable.
> 
> 
>>  }
>>
>>  /**
> 


-- 
Timothy Pearson
Raptor Engineering
+1 (415) 727-8645 (direct line)
+1 (512) 690-0200 (switchboard)
https://www.raptorengineering.com

Re: [PATCH] mtd/powernv_flash: Enable partition support

[Rafał Miłecki]

On 29 March 2018 at 23:43, Timothy Pearson
<tpearson@raptorengineering.com> wrote:
> Will resend with Signed-off-by.
>
> Before this patch, the driver was not picking up the OF-provided
> partition list.  It seemed specifically designed to register only one
> large partition covering the entire PNOR; mtd_device_register() does not
> run parsing of any type AFAIK.

Please don't top post.

This is how mtd_device_register is defined:
#define mtd_device_register(master, parts, nr_parts) \
        mtd_device_parse_register(master, NULL, NULL, parts, nr_parts)

So your patch seems to be replacing
mtd_device_parse_register(&data->mtd, NULL, NULL, NULL, 0);
with
mtd_device_parse_register(&data->mtd, part_probes, NULL, NULL, 0);

I still claim that all it does is replacing list of default parsers:
"cmdlinepart", "ofpart", NULL
with a custom list:
"ofpart", NULL

I don't think you need that. Probably the only change you really need is to add:
mtd_set_of_node(&data->mtd, dev->of_node);

Re: [PATCH] mtd/powernv_flash: Enable partition support

[Timothy Pearson]

On 03/29/2018 05:11 PM, Rafał Miłecki wrote:
> On 29 March 2018 at 23:43, Timothy Pearson
> <tpearson@raptorengineering.com> wrote:
>> Will resend with Signed-off-by.
>>
>> Before this patch, the driver was not picking up the OF-provided
>> partition list.  It seemed specifically designed to register only one
>> large partition covering the entire PNOR; mtd_device_register() does not
>> run parsing of any type AFAIK.
> 
> Please don't top post.

Whoops!  I have to interface with other entities that need top posting
and slipped up here.  Sorry about that!

> This is how mtd_device_register is defined:
> #define mtd_device_register(master, parts, nr_parts) \
>         mtd_device_parse_register(master, NULL, NULL, parts, nr_parts)
> 
> So your patch seems to be replacing
> mtd_device_parse_register(&data->mtd, NULL, NULL, NULL, 0);
> with
> mtd_device_parse_register(&data->mtd, part_probes, NULL, NULL, 0);
> 
> I still claim that all it does is replacing list of default parsers:
> "cmdlinepart", "ofpart", NULL
> with a custom list:
> "ofpart", NULL
> 
> I don't think you need that. Probably the only change you really need is to add:
> mtd_set_of_node(&data->mtd, dev->of_node);

It's possible; I'm not as familiar with the MTD subsystem as I should
be.  That being said, in this application there is no chance of
partition information being passed via kernel command line, so the right
thing to do from a design perspective is lock the parser to ofpart only.

If you need me to remove that part of the patch I will do so.

Thanks!

-- 
Timothy Pearson
Raptor Engineering
+1 (415) 727-8645 (direct line)
+1 (512) 690-0200 (switchboard)
https://www.raptorengineering.com