Re: [PATCH 1/4] mtd: nand: gpio: Determine bus width automatically

Re: [PATCH 1/4] mtd: nand: gpio: Determine bus width automatically

[Brian Norris]

On Tue, Jul 23, 2013 at 5:28 AM, Alexander Shiyan <shc_work@mail.ru> wrote:
> This patch provide automatically determine of bus width. If resource size,
> supplied to the driver more than 1 byte, the NAND_BUSWIDTH_AUTO option
> will be used in the MTD core.

I presume this depends on the bugfix I sent for NAND_BUSWIDTH_AUTO /
nand_set_defaults()? That's worth noting (as I am right now), for
whenever someone gets around to merging this.

> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> ---
>  .../devicetree/bindings/mtd/gpio-control-nand.txt  |  5 ++---

Considering the changes in device-tree bindings, it's good to CC the
device-tree list, I believe.

>  drivers/mtd/nand/gpio.c                            | 23 +++++++++++-----------
>  2 files changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
> index 36ef07d..287b8b8 100644
> --- a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
> @@ -8,15 +8,14 @@ Required properties:
>  - compatible : "gpio-control-nand"
>  - reg : should specify localbus chip select and size used for the chip.  The
>    resource describes the data bus connected to the NAND flash and all accesses
> -  are made in native endianness.
> +  are made in native endianness. Bus width of the device is determined
> +  automatically if size > 1. If size = 1, 8 bit bus width will be used.
>  - #address-cells, #size-cells : Must be present if the device has sub-nodes
>    representing partitions.
>  - gpios : specifies the gpio pins to control the NAND device.  nwp is an
>    optional gpio and may be set to 0 if not present.
>
>  Optional properties:
> -- bank-width : Width (in bytes) of the device.  If not present, the width
> -  defaults to 1 byte.

Do you really want to remove this property entirely? I'm not sure what
the policy is on this. And it may still be useful to leave in the
documentation, since older drivers (and potentially non-Linux OS?) may
still need the property. Maybe you can mark it as optional, and that
it may be ignored entirely.

>  - chip-delay : chip dependent delay for transferring data from array to
>    read registers (tR).  If not present then a default of 20us is used.
>  - gpio-control-nand,io-sync-reg : A 64-bit physical address for a read
> diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
> index 800a1cc..f144b80 100644
> --- a/drivers/mtd/nand/gpio.c
> +++ b/drivers/mtd/nand/gpio.c
> @@ -109,14 +109,9 @@ static int gpio_nand_get_config_of(const struct device *dev,
>         if (!dev->of_node)
>                 return -ENODEV;
>
> -       if (!of_property_read_u32(dev->of_node, "bank-width", &val)) {
> -               if (val == 2) {
> -                       plat->options |= NAND_BUSWIDTH_16;
> -               } else if (val != 1) {
> -                       dev_err(dev, "invalid bank-width %u\n", val);
> -                       return -EINVAL;
> -               }
> -       }
> +       /* Deprecated since 3.11-rc2 */

I doubt this patch will be included in 3.11-rc2 :) This is material
for the next merge window.

> +       if (of_find_property(dev->of_node, "bank-width", NULL))
> +               dev_notice(dev, "Property \"bank-width\" is deprecated");

If you don't totally kill this property (per my comments above), then
you probably don't want this message either. It's probably safe to
just ignore the property, if we can reliably auto-detect it instead.

>         plat->gpio_rdy = of_get_gpio(dev->of_node, 0);
>         plat->gpio_nce = of_get_gpio(dev->of_node, 1);
> @@ -223,6 +218,14 @@ static int gpio_nand_probe(struct platform_device *pdev)
>         if (IS_ERR(chip->IO_ADDR_R))
>                 return PTR_ERR(chip->IO_ADDR_R);
>
> +       ret = gpio_nand_get_config(&pdev->dev, &gpiomtd->plat);
> +       if (ret)
> +               return ret;
> +
> +       gpiomtd->plat.options &= ~(NAND_BUSWIDTH_16 | NAND_BUSWIDTH_AUTO);
> +       if (resource_size(res) > 1)
> +               gpiomtd->plat.options |= NAND_BUSWIDTH_AUTO;
> +
>         res = gpio_nand_get_io_sync(pdev);
>         if (res) {
>                 gpiomtd->io_sync = devm_ioremap_resource(&pdev->dev, res);
> @@ -230,10 +233,6 @@ static int gpio_nand_probe(struct platform_device *pdev)
>                         return PTR_ERR(gpiomtd->io_sync);
>         }
>
> -       ret = gpio_nand_get_config(&pdev->dev, &gpiomtd->plat);
> -       if (ret)
> -               return ret;
> -
>         ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nce, "NAND NCE");
>         if (ret)
>                 return ret;

Brian

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/4] mtd: nand: gpio: Determine bus width automatically

[Brian Norris]

Resending to the new DT mailing list. (Guys, this isn't advertised
very well, and your MAINTAINERS patch hasn't been applied to Linus'
tree yet. Why the rapid transition?)

On Tue, Jul 23, 2013 at 11:42 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Tue, Jul 23, 2013 at 5:28 AM, Alexander Shiyan <shc_work@mail.ru> wrote:
>> This patch provide automatically determine of bus width. If resource size,
>> supplied to the driver more than 1 byte, the NAND_BUSWIDTH_AUTO option
>> will be used in the MTD core.
>
> I presume this depends on the bugfix I sent for NAND_BUSWIDTH_AUTO /
> nand_set_defaults()? That's worth noting (as I am right now), for
> whenever someone gets around to merging this.
>
>> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
>> ---
>>  .../devicetree/bindings/mtd/gpio-control-nand.txt  |  5 ++---
>
> Considering the changes in device-tree bindings, it's good to CC the
> device-tree list, I believe.
>
>>  drivers/mtd/nand/gpio.c                            | 23 +++++++++++-----------
>>  2 files changed, 13 insertions(+), 15 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
>> index 36ef07d..287b8b8 100644
>> --- a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
>> +++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
>> @@ -8,15 +8,14 @@ Required properties:
>>  - compatible : "gpio-control-nand"
>>  - reg : should specify localbus chip select and size used for the chip.  The
>>    resource describes the data bus connected to the NAND flash and all accesses
>> -  are made in native endianness.
>> +  are made in native endianness. Bus width of the device is determined
>> +  automatically if size > 1. If size = 1, 8 bit bus width will be used.
>>  - #address-cells, #size-cells : Must be present if the device has sub-nodes
>>    representing partitions.
>>  - gpios : specifies the gpio pins to control the NAND device.  nwp is an
>>    optional gpio and may be set to 0 if not present.
>>
>>  Optional properties:
>> -- bank-width : Width (in bytes) of the device.  If not present, the width
>> -  defaults to 1 byte.
>
> Do you really want to remove this property entirely? I'm not sure what
> the policy is on this. And it may still be useful to leave in the
> documentation, since older drivers (and potentially non-Linux OS?) may
> still need the property. Maybe you can mark it as optional, and that
> it may be ignored entirely.
>
>>  - chip-delay : chip dependent delay for transferring data from array to
>>    read registers (tR).  If not present then a default of 20us is used.
>>  - gpio-control-nand,io-sync-reg : A 64-bit physical address for a read
>> diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
>> index 800a1cc..f144b80 100644
>> --- a/drivers/mtd/nand/gpio.c
>> +++ b/drivers/mtd/nand/gpio.c
>> @@ -109,14 +109,9 @@ static int gpio_nand_get_config_of(const struct device *dev,
>>         if (!dev->of_node)
>>                 return -ENODEV;
>>
>> -       if (!of_property_read_u32(dev->of_node, "bank-width", &val)) {
>> -               if (val == 2) {
>> -                       plat->options |= NAND_BUSWIDTH_16;
>> -               } else if (val != 1) {
>> -                       dev_err(dev, "invalid bank-width %u\n", val);
>> -                       return -EINVAL;
>> -               }
>> -       }
>> +       /* Deprecated since 3.11-rc2 */
>
> I doubt this patch will be included in 3.11-rc2 :) This is material
> for the next merge window.
>
>> +       if (of_find_property(dev->of_node, "bank-width", NULL))
>> +               dev_notice(dev, "Property \"bank-width\" is deprecated");
>
> If you don't totally kill this property (per my comments above), then
> you probably don't want this message either. It's probably safe to
> just ignore the property, if we can reliably auto-detect it instead.
>
>>         plat->gpio_rdy = of_get_gpio(dev->of_node, 0);
>>         plat->gpio_nce = of_get_gpio(dev->of_node, 1);
>> @@ -223,6 +218,14 @@ static int gpio_nand_probe(struct platform_device *pdev)
>>         if (IS_ERR(chip->IO_ADDR_R))
>>                 return PTR_ERR(chip->IO_ADDR_R);
>>
>> +       ret = gpio_nand_get_config(&pdev->dev, &gpiomtd->plat);
>> +       if (ret)
>> +               return ret;
>> +
>> +       gpiomtd->plat.options &= ~(NAND_BUSWIDTH_16 | NAND_BUSWIDTH_AUTO);
>> +       if (resource_size(res) > 1)
>> +               gpiomtd->plat.options |= NAND_BUSWIDTH_AUTO;
>> +
>>         res = gpio_nand_get_io_sync(pdev);
>>         if (res) {
>>                 gpiomtd->io_sync = devm_ioremap_resource(&pdev->dev, res);
>> @@ -230,10 +233,6 @@ static int gpio_nand_probe(struct platform_device *pdev)
>>                         return PTR_ERR(gpiomtd->io_sync);
>>         }
>>
>> -       ret = gpio_nand_get_config(&pdev->dev, &gpiomtd->plat);
>> -       if (ret)
>> -               return ret;
>> -
>>         ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nce, "NAND NCE");
>>         if (ret)
>>                 return ret;
>
> Brian

Re: [PATCH 1/4] mtd: nand: gpio: Determine bus width automatically

[Brian Norris]

On Tue, Jul 23, 2013 at 11:52 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> and your MAINTAINERS patch hasn't been applied to Linus'
> tree yet

I stand corrected on this point. It was applied a few hours ago.

Regards,
Brian

Re: [PATCH 1/4] mtd: nand: gpio: Determine bus width automatically

[Alexander Shiyan]

On Tue, 23 Jul 2013 23:42:08 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> On Tue, Jul 23, 2013 at 5:28 AM, Alexander Shiyan <shc_work@mail.ru> wrote:
> > This patch provide automatically determine of bus width. If resource size,
> > supplied to the driver more than 1 byte, the NAND_BUSWIDTH_AUTO option
> > will be used in the MTD core.
> 
> I presume this depends on the bugfix I sent for NAND_BUSWIDTH_AUTO /
> nand_set_defaults()? That's worth noting (as I am right now), for
> whenever someone gets around to merging this.

OK.

[...]
> > diff --git a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
> > index 36ef07d..287b8b8 100644
> > --- a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
> > +++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
> > @@ -8,15 +8,14 @@ Required properties:
> >  - compatible : "gpio-control-nand"
> >  - reg : should specify localbus chip select and size used for the chip.  The
> >    resource describes the data bus connected to the NAND flash and all accesses
> > -  are made in native endianness.
> > +  are made in native endianness. Bus width of the device is determined
> > +  automatically if size > 1. If size = 1, 8 bit bus width will be used.
> >  - #address-cells, #size-cells : Must be present if the device has sub-nodes
> >    representing partitions.
> >  - gpios : specifies the gpio pins to control the NAND device.  nwp is an
> >    optional gpio and may be set to 0 if not present.
> >
> >  Optional properties:
> > -- bank-width : Width (in bytes) of the device.  If not present, the width
> > -  defaults to 1 byte.
> 
> Do you really want to remove this property entirely? I'm not sure what
> the policy is on this. And it may still be useful to leave in the
> documentation, since older drivers (and potentially non-Linux OS?) may
> still need the property. Maybe you can mark it as optional, and that
> it may be ignored entirely.

I redid it, I'll do option "bank-width" optional.

[...]
> > +       if (of_find_property(dev->of_node, "bank-width", NULL))
> > +               dev_notice(dev, "Property \"bank-width\" is deprecated");
> 
> If you don't totally kill this property (per my comments above), then
> you probably don't want this message either. It's probably safe to
> just ignore the property, if we can reliably auto-detect it instead.

What do you say about such changes in the patch 4/4?
Thanks.

[...]

-- 
Alexander Shiyan <shc_work@mail.ru>

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/4] mtd: nand: gpio: Determine bus width automatically

[Brian Norris]

On Wed, Jul 24, 2013 at 6:28 AM, Alexander Shiyan <shc_work@mail.ru> wrote:
> On Tue, 23 Jul 2013 23:42:08 -0700
> Brian Norris <computersforpeace@gmail.com> wrote:
>
>> On Tue, Jul 23, 2013 at 5:28 AM, Alexander Shiyan <shc_work@mail.ru> wrote:
>> > diff --git a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
>> > index 36ef07d..287b8b8 100644
>> > --- a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
>> > +++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
>> > @@ -8,15 +8,14 @@ Required properties:
>> >  - compatible : "gpio-control-nand"
>> >  - reg : should specify localbus chip select and size used for the chip.  The
>> >    resource describes the data bus connected to the NAND flash and all accesses
>> > -  are made in native endianness.
>> > +  are made in native endianness. Bus width of the device is determined
>> > +  automatically if size > 1. If size = 1, 8 bit bus width will be used.
>> >  - #address-cells, #size-cells : Must be present if the device has sub-nodes
>> >    representing partitions.
>> >  - gpios : specifies the gpio pins to control the NAND device.  nwp is an
>> >    optional gpio and may be set to 0 if not present.
>> >
>> >  Optional properties:
>> > -- bank-width : Width (in bytes) of the device.  If not present, the width
>> > -  defaults to 1 byte.
>>
>> Do you really want to remove this property entirely? I'm not sure what
>> the policy is on this. And it may still be useful to leave in the
>> documentation, since older drivers (and potentially non-Linux OS?) may
>> still need the property. Maybe you can mark it as optional, and that
>> it may be ignored entirely.
>
> I redid it, I'll do option "bank-width" optional.
>
> [...]
>> > +       if (of_find_property(dev->of_node, "bank-width", NULL))
>> > +               dev_notice(dev, "Property \"bank-width\" is deprecated");
>>
>> If you don't totally kill this property (per my comments above), then
>> you probably don't want this message either. It's probably safe to
>> just ignore the property, if we can reliably auto-detect it instead.
>
> What do you say about such changes in the patch 4/4?
> Thanks.

I guess you're referring to this hunk from patch 4/4?

-       /* Deprecated since 3.11-rc2 */
+       /* Deprecated since 3.11-rc2, should be removed around 3.17 */

This hunk really seems out of place in patch 4. If you even want this
change, it should go in with patch 1. I don't see how it is related to
patch 4.

But the key point is that I don't think you should try to detect (and
print notices about) the bank-width property at all. As long as the
documentation clearly states that it is optional (and that some
systems may autodetect it), I think that is sufficient not to print
any warnings. You should consider that some user may want to retain
this DT binding for some other non-Linux OS (or for some range of new
and old kernel versions) so that they need this bank-width property.

Another way to look at this is that it doesn't hurt Linux at all for
there to be an extra DT property for a device. But it can hurt the
bootloader if it needs to support other OS's/drivers that don't
autodetect bank-width property.

On the other hand, if you're going to include this kind of
schedule-related comment at all, then it should at least be correct. I
can tell you a few things for sure:
(1) This code is not going into any of the 3.11 release candidates
(2) It doesn't really make sense to a user to see references to "-rc2"
when reading Linux source code. That's very much a process-related
detail that should not go in here.

Brian

Re: [PATCH 1/4] mtd: nand: gpio: Determine bus width automatically

[Brian Norris]

(snip conversation)

BTW Alexander, please be sure to CC the appropriate list
(devicetree@vger.kernel.org) for the resend of your DT-related patches
(I initially used the wrong list). They should have better input on
the process of adding/removing/deprecating device properties. And if
they don't have any opinion, then it's probably fine to do this
whatever way you/we see fit :)

Brian

Re: [PATCH 1/4] mtd: nand: gpio: Determine bus width automatically

[Ezequiel Garcia]

On Tue, Jul 23, 2013 at 11:42:08PM -0700, Brian Norris wrote:
> On Tue, Jul 23, 2013 at 5:28 AM, Alexander Shiyan <shc_work@mail.ru> wrote:
> > This patch provide automatically determine of bus width. If resource size,
> > supplied to the driver more than 1 byte, the NAND_BUSWIDTH_AUTO option
> > will be used in the MTD core.
> 
> I presume this depends on the bugfix I sent for NAND_BUSWIDTH_AUTO /
> nand_set_defaults()? That's worth noting (as I am right now), for
> whenever someone gets around to merging this.
> 
> > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > ---
> >  .../devicetree/bindings/mtd/gpio-control-nand.txt  |  5 ++---
> 
> Considering the changes in device-tree bindings, it's good to CC the
> device-tree list, I believe.
> 

Adding the new devicetree list in Cc.

Keep in mind you might want to split the DT binding part of the patch
into a separate patch. For small patches like this one, it's probably
not a big deal.

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com