Re: [PATCH v3 1/4] NAND: FSL-UPM: add multi chip support

Re: [PATCH v3 1/4] NAND: FSL-UPM: add multi chip support

[Grant Likely]

On Wed, Mar 25, 2009 at 7:31 AM, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> On Wed, Mar 25, 2009 at 4:08 AM, Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org> wrote:
>> This patch adds support for multi-chip NAND devices to the FSL-UPM
>> driver. This requires support for multiple GPIOs for the RNB pins.
>> The NAND chips are selected through address lines defined by the
>> FDT property "chip-offset".
>>
>> Signed-off-by: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
>
> Hi Wolfgang,
>
> Can you please send a sample device tree snippit for this and add
> documentation updates to your patch for the extended binding?

Oh, and cc: devicetree-discuss-mnsaURCQ41sdnm+yROfE0A@public.gmane.org in your next posting.

g.

>> ---
>>  arch/powerpc/sysdev/fsl_lbc.c |    2 +-
>>  drivers/mtd/nand/fsl_upm.c    |   95 +++++++++++++++++++++++++++++++---------
>>  2 files changed, 74 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/powerpc/sysdev/fsl_lbc.c b/arch/powerpc/sysdev/fsl_lbc.c
>> index 0494ee5..dceb8d1 100644
>> --- a/arch/powerpc/sysdev/fsl_lbc.c
>> +++ b/arch/powerpc/sysdev/fsl_lbc.c
>> @@ -150,7 +150,7 @@ int fsl_upm_run_pattern(struct fsl_upm *upm, void __iomem *io_base, u32 mar)
>>
>>        spin_lock_irqsave(&fsl_lbc_lock, flags);
>>
>> -       out_be32(&fsl_lbc_regs->mar, mar << (32 - upm->width));
>> +       out_be32(&fsl_lbc_regs->mar, mar);
>>
>>        switch (upm->width) {
>>        case 8:
>> diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c
>> index 7815a40..9b314ce 100644
>> --- a/drivers/mtd/nand/fsl_upm.c
>> +++ b/drivers/mtd/nand/fsl_upm.c
>> @@ -36,8 +36,11 @@ struct fsl_upm_nand {
>>        uint8_t upm_addr_offset;
>>        uint8_t upm_cmd_offset;
>>        void __iomem *io_base;
>> -       int rnb_gpio;
>> +       int rnb_gpio[NAND_MAX_CHIPS];
>>        int chip_delay;
>> +       uint32_t num_chips;
>> +       uint32_t chip_number;
>> +       uint32_t chip_offset;
>>  };
>>
>>  #define to_fsl_upm_nand(mtd) container_of(mtd, struct fsl_upm_nand, mtd)
>> @@ -46,7 +49,7 @@ static int fun_chip_ready(struct mtd_info *mtd)
>>  {
>>        struct fsl_upm_nand *fun = to_fsl_upm_nand(mtd);
>>
>> -       if (gpio_get_value(fun->rnb_gpio))
>> +       if (gpio_get_value(fun->rnb_gpio[fun->chip_number]))
>>                return 1;
>>
>>        dev_vdbg(fun->dev, "busy\n");
>> @@ -55,9 +58,9 @@ static int fun_chip_ready(struct mtd_info *mtd)
>>
>>  static void fun_wait_rnb(struct fsl_upm_nand *fun)
>>  {
>> -       int cnt = 1000000;
>>
>> -       if (fun->rnb_gpio >= 0) {
>> +       if (fun->rnb_gpio[fun->chip_number] >= 0) {
>> +               int cnt = 1000000;
>>                while (--cnt && !fun_chip_ready(&fun->mtd))
>>                        cpu_relax();
>>                if (!cnt)
>> @@ -69,7 +72,9 @@ static void fun_wait_rnb(struct fsl_upm_nand *fun)
>>
>>  static void fun_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>>  {
>> +       struct nand_chip *chip = mtd->priv;
>>        struct fsl_upm_nand *fun = to_fsl_upm_nand(mtd);
>> +       u32 mar;
>>
>>        if (!(ctrl & fun->last_ctrl)) {
>>                fsl_upm_end_pattern(&fun->upm);
>> @@ -87,11 +92,30 @@ static void fun_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>>                        fsl_upm_start_pattern(&fun->upm, fun->upm_cmd_offset);
>>        }
>>
>> -       fsl_upm_run_pattern(&fun->upm, fun->io_base, cmd);
>> +       mar = cmd << (32 - fun->upm.width);
>> +       if (fun->chip_offset && fun->chip_number > 0)
>> +               mar |= fun->chip_number * fun->chip_offset;
>> +       fsl_upm_run_pattern(&fun->upm, chip->IO_ADDR_R, mar);
>>
>>        fun_wait_rnb(fun);
>>  }
>>
>> +static void fun_select_chip(struct mtd_info *mtd, int chip_nr)
>> +{
>> +       struct nand_chip *chip = mtd->priv;
>> +       struct fsl_upm_nand *fun = to_fsl_upm_nand(mtd);
>> +
>> +       if (chip_nr == -1) {
>> +               chip->cmd_ctrl(mtd, NAND_CMD_NONE, 0 | NAND_CTRL_CHANGE);
>> +       } else if (chip_nr >= 0) {
>> +               fun->chip_number = chip_nr;
>> +               chip->IO_ADDR_R = chip->IO_ADDR_W =
>> +                       fun->io_base + chip_nr * fun->chip_offset;
>> +       } else {
>> +               BUG();
>> +       }
>> +}
>> +
>>  static uint8_t fun_read_byte(struct mtd_info *mtd)
>>  {
>>        struct fsl_upm_nand *fun = to_fsl_upm_nand(mtd);
>> @@ -137,8 +161,10 @@ static int __devinit fun_chip_init(struct fsl_upm_nand *fun,
>>        fun->chip.read_buf = fun_read_buf;
>>        fun->chip.write_buf = fun_write_buf;
>>        fun->chip.ecc.mode = NAND_ECC_SOFT;
>> +       if (fun->num_chips > 1)
>> +               fun->chip.select_chip = fun_select_chip;
>>
>> -       if (fun->rnb_gpio >= 0)
>> +       if (fun->rnb_gpio[0] >= 0)
>>                fun->chip.dev_ready = fun_chip_ready;
>>
>>        fun->mtd.priv = &fun->chip;
>> @@ -155,7 +181,7 @@ static int __devinit fun_chip_init(struct fsl_upm_nand *fun,
>>                goto err;
>>        }
>>
>> -       ret = nand_scan(&fun->mtd, 1);
>> +       ret = nand_scan(&fun->mtd, fun->num_chips);
>>        if (ret)
>>                goto err;
>>
>> @@ -187,6 +213,7 @@ static int __devinit fun_probe(struct of_device *ofdev,
>>        const uint32_t *prop;
>>        int ret;
>>        int size;
>> +       int i;
>>
>>        fun = kzalloc(sizeof(*fun), GFP_KERNEL);
>>        if (!fun)
>> @@ -208,7 +235,7 @@ static int __devinit fun_probe(struct of_device *ofdev,
>>        if (!prop || size != sizeof(uint32_t)) {
>>                dev_err(&ofdev->dev, "can't get UPM address offset\n");
>>                ret = -EINVAL;
>> -               goto err2;
>> +               goto err1;
>>        }
>>        fun->upm_addr_offset = *prop;
>>
>> @@ -216,21 +243,36 @@ static int __devinit fun_probe(struct of_device *ofdev,
>>        if (!prop || size != sizeof(uint32_t)) {
>>                dev_err(&ofdev->dev, "can't get UPM command offset\n");
>>                ret = -EINVAL;
>> -               goto err2;
>> +               goto err1;
>>        }
>>        fun->upm_cmd_offset = *prop;
>>
>> -       fun->rnb_gpio = of_get_gpio(ofdev->node, 0);
>> -       if (fun->rnb_gpio >= 0) {
>> -               ret = gpio_request(fun->rnb_gpio, dev_name(&ofdev->dev));
>> -               if (ret) {
>> -                       dev_err(&ofdev->dev, "can't request RNB gpio\n");
>> +       prop = of_get_property(ofdev->node, "num-chips", &size);
>> +       if (prop && size == sizeof(uint32_t)) {
>> +               fun->num_chips = *prop;
>> +               if (fun->num_chips >= NAND_MAX_CHIPS) {
>> +                       dev_err(&ofdev->dev, "too much chips");
>> +                       ret = -EINVAL;
>> +                       goto err1;
>> +               }
>> +       } else {
>> +               fun->num_chips = 1;
>> +       }
>> +
>> +       for (i = 0; i < fun->num_chips; i++) {
>> +               fun->rnb_gpio[i] = of_get_gpio(ofdev->node, i);
>> +               if (fun->rnb_gpio[i] >= 0) {
>> +                       ret = gpio_request(fun->rnb_gpio[i],
>> +                                          dev_name(&ofdev->dev));
>> +                       if (ret) {
>> +                               dev_err(&ofdev->dev, "can't request RNB gpio\n");
>> +                               goto err2;
>> +                       }
>> +                       gpio_direction_input(fun->rnb_gpio[i]);
>> +               } else if (fun->rnb_gpio[i]  == -EINVAL) {
>> +                       dev_err(&ofdev->dev, "specified RNB gpio is invalid\n");
>>                        goto err2;
>>                }
>> -               gpio_direction_input(fun->rnb_gpio);
>> -       } else if (fun->rnb_gpio == -EINVAL) {
>> -               dev_err(&ofdev->dev, "specified RNB gpio is invalid\n");
>> -               goto err2;
>>        }
>>
>>        prop = of_get_property(ofdev->node, "chip-delay", NULL);
>> @@ -239,6 +281,10 @@ static int __devinit fun_probe(struct of_device *ofdev,
>>        else
>>                fun->chip_delay = 50;
>>
>> +       prop = of_get_property(ofdev->node, "chip-offset", &size);
>> +       if (prop && size == sizeof(uint32_t))
>> +               fun->chip_offset = *prop;
>> +
>>        fun->io_base = devm_ioremap_nocache(&ofdev->dev, io_res.start,
>>                                          io_res.end - io_res.start + 1);
>>        if (!fun->io_base) {
>> @@ -257,8 +303,10 @@ static int __devinit fun_probe(struct of_device *ofdev,
>>
>>        return 0;
>>  err2:
>> -       if (fun->rnb_gpio >= 0)
>> -               gpio_free(fun->rnb_gpio);
>> +       for (i = 0; i < fun->num_chips; i++) {
>> +               if (fun->rnb_gpio[i] >= 0)
>> +                       gpio_free(fun->rnb_gpio[i]);
>> +       }
>>  err1:
>>        kfree(fun);
>>
>> @@ -268,12 +316,15 @@ err1:
>>  static int __devexit fun_remove(struct of_device *ofdev)
>>  {
>>        struct fsl_upm_nand *fun = dev_get_drvdata(&ofdev->dev);
>> +       int i;
>>
>>        nand_release(&fun->mtd);
>>        kfree(fun->mtd.name);
>>
>> -       if (fun->rnb_gpio >= 0)
>> -               gpio_free(fun->rnb_gpio);
>> +        for (i = 0; i < fun->num_chips; i++) {
>> +                if (fun->rnb_gpio[i] >= 0)
>> +                        gpio_free(fun->rnb_gpio[i]);
>> +        }
>>
>>        kfree(fun);
>>
>> --
>> 1.6.0.6
>>
>> _______________________________________________
>> Linuxppc-dev mailing list
>> Linuxppc-dev-mnsaURCQ41sdnm+yROfE0A@public.gmane.org
>> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>>
>
>
>
> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v3 1/4] NAND: FSL-UPM: add multi chip support

[Wolfgang Grandegger]

Grant Likely wrote:
> On Wed, Mar 25, 2009 at 7:31 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>> On Wed, Mar 25, 2009 at 4:08 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>>> This patch adds support for multi-chip NAND devices to the FSL-UPM
>>> driver. This requires support for multiple GPIOs for the RNB pins.
>>> The NAND chips are selected through address lines defined by the
>>> FDT property "chip-offset".
>>>
>>> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
>> Hi Wolfgang,
>>
>> Can you please send a sample device tree snippit for this and add
>> documentation updates to your patch for the extended binding?
> 
> Oh, and cc: devicetree-discuss@ozlabs.org in your next posting.

OK, does patch 3/4 not already contain what you are looking for? See:

http://ozlabs.org/pipermail/linuxppc-dev/2009-March/069787.html

I separated it from the NAND patches because they go through the MTD
maintainer(s).

BTW: did you have a chance to look into the following RFC on I2C bus
speed setting?

 http://ozlabs.org/pipermail/linuxppc-dev/2009-March/069489.html

Thanks,

Wolfgang.

Re: [PATCH v3 1/4] NAND: FSL-UPM: add multi chip support

[Grant Likely]

On Wed, Mar 25, 2009 at 7:43 AM, Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org> wrote:
> Grant Likely wrote:
>> On Wed, Mar 25, 2009 at 7:31 AM, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
>>> On Wed, Mar 25, 2009 at 4:08 AM, Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org> wrote:
>>>> This patch adds support for multi-chip NAND devices to the FSL-UPM
>>>> driver. This requires support for multiple GPIOs for the RNB pins.
>>>> The NAND chips are selected through address lines defined by the
>>>> FDT property "chip-offset".
>>>>
>>>> Signed-off-by: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
>>> Hi Wolfgang,
>>>
>>> Can you please send a sample device tree snippit for this and add
>>> documentation updates to your patch for the extended binding?
>>
>> Oh, and cc: devicetree-discuss-mnsaURCQ41sdnm+yROfE0A@public.gmane.org in your next posting.
>
> OK, does patch 3/4 not already contain what you are looking for? See:
>
> http://ozlabs.org/pipermail/linuxppc-dev/2009-March/069787.html

Oops, sorry.  Missed that.

> I separated it from the NAND patches because they go through the MTD
> maintainer(s).
>
> BTW: did you have a chance to look into the following RFC on I2C bus
> speed setting?
>
>  http://ozlabs.org/pipermail/linuxppc-dev/2009-March/069489.html

No.  Looking at it now.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Anton Vorontsov]

On Wed, Mar 25, 2009 at 11:08:20AM +0100, Wolfgang Grandegger wrote:
> This patch adds documentation for the new NAND FSL UPM bindings for:
> 
>  NAND: FSL-UPM: add multi chip support
>  NAND: FSL-UPM: Add wait flags to support board/chip specific delays
> 
> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
> ---

To me it looks good.

Acked-by: Anton Vorontsov <avorontsov@ru.mvista.com>

>  .../powerpc/dts-bindings/fsl/upm-nand.txt          |   39 +++++++++++++++++++-
>  1 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt b/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
> index 84a04d5..0272e70 100644
> --- a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
> +++ b/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
> @@ -5,9 +5,22 @@ Required properties:
>  - reg : should specify localbus chip select and size used for the chip.
>  - fsl,upm-addr-offset : UPM pattern offset for the address latch.
>  - fsl,upm-cmd-offset : UPM pattern offset for the command latch.
> -- gpios : may specify optional GPIO connected to the Ready-Not-Busy pin.
>  
> -Example:
> +Optional properties:
> +- fsl,upm-wait-flags : add chip-dependent short delays after running the
> +  		       UPM pattern (0x1), after writing a data byte (0x2)
> +		       or after writing out a buffer (0x4).
> +- gpios : may specify optional GPIOs connected to the Ready-Not-Busy pins
> +	  (R/B#). For multi-chip devices, "num-chips" GPIO definitions are
> +	  required.
> +- chip-delay : chip dependent delay for transfering data from array to
> +	       read registers (tR). Required if property "gpios" is not
> +	       used (R/B# pins not connected).
> +- num-chips : number of chips per device for multi-chip support.
> +- chip-offset : address offset between chips for multi-chip support. The
> + 		corresponding address lines are used to select the chip.
> +
> +Examples:
>  
>  upm@1,0 {
>  	compatible = "fsl,upm-nand";
> @@ -26,3 +39,25 @@ upm@1,0 {
>  		};
>  	};
>  };
> +
> +upm@3,0 {
> +	compatible = "fsl,upm-nand";
> +	reg = <3 0x0 0x800>;
> +	fsl,upm-addr-offset = <0x10>;
> +	fsl,upm-cmd-offset = <0x08>;
> +	fsl,upm-wait-flags = <0x5>;
> +	/* Multi-chip device */
> +	num-chips = <2>;
> +	chip-offset = <0x200>;
> +	chip-delay = <25>; // in micro-seconds
> +
> +	nand@0 {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +
> +		partition@0 {
> +			    label = "fs";
> +			    reg = <0x00000000 0x10000000>;
> +		};
> +	};
> +};
> -- 
> 1.6.0.6

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Grant Likely]

(cc'ing devicetree-discuss)

On Wed, Mar 25, 2009 at 4:08 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
> This patch adds documentation for the new NAND FSL UPM bindings for:
>
>  NAND: FSL-UPM: add multi chip support
>  NAND: FSL-UPM: Add wait flags to support board/chip specific delays
>
> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>

Mostly looks good to me; but some comments below.

> ---
>  .../powerpc/dts-bindings/fsl/upm-nand.txt          |   39 +++++++++++++++++++-
>  1 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt b/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
> index 84a04d5..0272e70 100644
> --- a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
> +++ b/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
> @@ -5,9 +5,22 @@ Required properties:
>  - reg : should specify localbus chip select and size used for the chip.
>  - fsl,upm-addr-offset : UPM pattern offset for the address latch.
>  - fsl,upm-cmd-offset : UPM pattern offset for the command latch.
> -- gpios : may specify optional GPIO connected to the Ready-Not-Busy pin.
>
> -Example:
> +Optional properties:
> +- fsl,upm-wait-flags : add chip-dependent short delays after running the
> +                      UPM pattern (0x1), after writing a data byte (0x2)
> +                      or after writing out a buffer (0x4).
> +- gpios : may specify optional GPIOs connected to the Ready-Not-Busy pins
> +         (R/B#). For multi-chip devices, "num-chips" GPIO definitions are
> +         required.
> +- chip-delay : chip dependent delay for transfering data from array to
> +              read registers (tR). Required if property "gpios" is not
> +              used (R/B# pins not connected).
> +- num-chips : number of chips per device for multi-chip support.
> +- chip-offset : address offset between chips for multi-chip support. The
> +               corresponding address lines are used to select the chip.

Since these properties (chip-delay, num-chips and chip-offset) are
currently controller specific, it would probably be a good idea to
prefix 'fsl,' onto them.  That way when common NAND controller
properties start getting defined then there won't be any concern about
conflicting with existing meanings.  If you do see these as properties
that other NAND controllers will use, then maybe a 'nand-' prefix is
appropriate (like the SPI binding in booting-without-of).

For the chip offset, it's not clear what the meaning is.  First, does
the UPM controller support access of multiple chips simultaneously?
If so, then can you elaborate in the description on how board design
translates to a chip-offset value.  If it cannot, then it might be
better to have multiple tuples in the 'reg' property for each discrete
chip.  Multiple reg tuples would also remove the need for the
num-chips property.

Cheers,
g.

> +
> +Examples:
>
>  upm@1,0 {
>        compatible = "fsl,upm-nand";
> @@ -26,3 +39,25 @@ upm@1,0 {
>                };
>        };
>  };
> +
> +upm@3,0 {
> +       compatible = "fsl,upm-nand";
> +       reg = <3 0x0 0x800>;
> +       fsl,upm-addr-offset = <0x10>;
> +       fsl,upm-cmd-offset = <0x08>;
> +       fsl,upm-wait-flags = <0x5>;
> +       /* Multi-chip device */
> +       num-chips = <2>;
> +       chip-offset = <0x200>;
> +       chip-delay = <25>; // in micro-seconds
> +
> +       nand@0 {
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +
> +               partition@0 {
> +                           label = "fs";
> +                           reg = <0x00000000 0x10000000>;
> +               };
> +       };
> +};
> --
> 1.6.0.6
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Wolfgang Grandegger]

Grant Likely wrote:
> (cc'ing devicetree-discuss)
> 
> On Wed, Mar 25, 2009 at 4:08 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> This patch adds documentation for the new NAND FSL UPM bindings for:
>>
>>  NAND: FSL-UPM: add multi chip support
>>  NAND: FSL-UPM: Add wait flags to support board/chip specific delays
>>
>> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
> 
> Mostly looks good to me; but some comments below.
> 
>> ---
>>  .../powerpc/dts-bindings/fsl/upm-nand.txt          |   39 +++++++++++++++++++-
>>  1 files changed, 37 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt b/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
>> index 84a04d5..0272e70 100644
>> --- a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
>> +++ b/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
>> @@ -5,9 +5,22 @@ Required properties:
>>  - reg : should specify localbus chip select and size used for the chip.
>>  - fsl,upm-addr-offset : UPM pattern offset for the address latch.
>>  - fsl,upm-cmd-offset : UPM pattern offset for the command latch.
>> -- gpios : may specify optional GPIO connected to the Ready-Not-Busy pin.
>>
>> -Example:
>> +Optional properties:
>> +- fsl,upm-wait-flags : add chip-dependent short delays after running the
>> +                      UPM pattern (0x1), after writing a data byte (0x2)
>> +                      or after writing out a buffer (0x4).
>> +- gpios : may specify optional GPIOs connected to the Ready-Not-Busy pins
>> +         (R/B#). For multi-chip devices, "num-chips" GPIO definitions are
>> +         required.
>> +- chip-delay : chip dependent delay for transfering data from array to
>> +              read registers (tR). Required if property "gpios" is not
>> +              used (R/B# pins not connected).
>> +- num-chips : number of chips per device for multi-chip support.
>> +- chip-offset : address offset between chips for multi-chip support. The
>> +               corresponding address lines are used to select the chip.
> 
> Since these properties (chip-delay, num-chips and chip-offset) are
> currently controller specific, it would probably be a good idea to
> prefix 'fsl,' onto them.  That way when common NAND controller
> properties start getting defined then there won't be any concern about
> conflicting with existing meanings.  If you do see these as properties
> that other NAND controllers will use, then maybe a 'nand-' prefix is
> appropriate (like the SPI binding in booting-without-of).

The chip-delay is NAND device specific. The proper value can be find in
the data sheet. num-chip is also quite generic. A prefix 'nand-' would
be fine for me. But fsl,upm-chip-offset would be more appropriate than
just chip-offset, indeed.

> For the chip offset, it's not clear what the meaning is.  First, does
> the UPM controller support access of multiple chips simultaneously?

The offset drives the corresponding address lines, which are used to
select the chip. That's how it's done on the TQM8548 board. In
principle, the chips could also be selected through dedicated GPIO pins.
Well, I'm not a hardware guy.

> If so, then can you elaborate in the description on how board design
> translates to a chip-offset value.  If it cannot, then it might be
> better to have multiple tuples in the 'reg' property for each discrete
> chip.  Multiple reg tuples would also remove the need for the
> num-chips property.

The node still describes one device mapping all relevant control
registers. How about using fsl,upm-chip-offsets = <0x200 0x400>. It
would be more generic and makes num-chips obsolete as well. And the
property would be reserved for that way of implementing the chip select
in hardware.

Wolfgang.

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Grant Likely]

On Wed, Mar 25, 2009 at 2:48 PM, Wolfgang Grandegger <wg@grandegger.com> wrote:
> Grant Likely wrote:
>> For the chip offset, it's not clear what the meaning is.  First, does
>> the UPM controller support access of multiple chips simultaneously?
>
> The offset drives the corresponding address lines, which are used to
> select the chip. That's how it's done on the TQM8548 board. In
> principle, the chips could also be selected through dedicated GPIO pins.
> Well, I'm not a hardware guy.

Heh.  I mean elaborate in the binding documentation.  :-)

>> If so, then can you elaborate in the description on how board design
>> translates to a chip-offset value.  If it cannot, then it might be
>> better to have multiple tuples in the 'reg' property for each discrete
>> chip.  Multiple reg tuples would also remove the need for the
>> num-chips property.
>
> The node still describes one device mapping all relevant control
> registers. How about using fsl,upm-chip-offsets = <0x200 0x400>. It
> would be more generic and makes num-chips obsolete as well. And the
> property would be reserved for that way of implementing the chip select
> in hardware.

It really sounds like this binding is describing multiple NAND chips
mapped to different base addresses (and looking at the fsm_upm.c
driver appears to confirm it).  So, does this work?  reg = <3 0x200 4
 3 0x400 4>;

It is true that other methods could be used for implementing the chip
select, but that is *not* what the proposed binding describes.  This
proposed binding describes NAND chips selected by address lines
(particular addresses), and in this case I think using reg is the
natural description.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Wolfgang Grandegger]

Grant Likely wrote:
> On Wed, Mar 25, 2009 at 2:48 PM, Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org> wrote:
>> Grant Likely wrote:
>>> For the chip offset, it's not clear what the meaning is.  First, does
>>> the UPM controller support access of multiple chips simultaneously?
>> The offset drives the corresponding address lines, which are used to
>> select the chip. That's how it's done on the TQM8548 board. In
>> principle, the chips could also be selected through dedicated GPIO pins.
>> Well, I'm not a hardware guy.
> 
> Heh.  I mean elaborate in the binding documentation.  :-)
> 
>>> If so, then can you elaborate in the description on how board design
>>> translates to a chip-offset value.  If it cannot, then it might be
>>> better to have multiple tuples in the 'reg' property for each discrete
>>> chip.  Multiple reg tuples would also remove the need for the
>>> num-chips property.
>> The node still describes one device mapping all relevant control
>> registers. How about using fsl,upm-chip-offsets = <0x200 0x400>. It
>> would be more generic and makes num-chips obsolete as well. And the
>> property would be reserved for that way of implementing the chip select
>> in hardware.
> 
> It really sounds like this binding is describing multiple NAND chips
> mapped to different base addresses (and looking at the fsm_upm.c
> driver appears to confirm it).  So, does this work?  reg = <3 0x200 4
>  3 0x400 4>;

The chip-offset, and not the address, needs to be added to the MAR
register as well before running the pattern:

        mar = cmd << (32 - fun->upm.width);

        if (fun->chip_offset && fun->chip_number > 0)

                mar |= fun->chip_number * fun->chip_offset;

        fsl_upm_run_pattern(&fun->upm, chip->IO_ADDR_R, mar);


> It is true that other methods could be used for implementing the chip
> select, but that is *not* what the proposed binding describes.  This
> proposed binding describes NAND chips selected by address lines
> (particular addresses), and in this case I think using reg is the
> natural description.

OK, the chips are selected by accessing a defined address range. Will
prepare a patch using the reg property.

Wolfgang.




> g.
> 

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Grant Likely]

On Thu, Mar 26, 2009 at 1:42 AM, Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org> wrote:
> Grant Likely wrote:
>> On Wed, Mar 25, 2009 at 2:48 PM, Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org> wrote:
>>> Grant Likely wrote:
>>>> For the chip offset, it's not clear what the meaning is.  First, does
>>>> the UPM controller support access of multiple chips simultaneously?
>>> The offset drives the corresponding address lines, which are used to
>>> select the chip. That's how it's done on the TQM8548 board. In
>>> principle, the chips could also be selected through dedicated GPIO pins.
>>> Well, I'm not a hardware guy.
>>
>> Heh.  I mean elaborate in the binding documentation.  :-)
>>
>>>> If so, then can you elaborate in the description on how board design
>>>> translates to a chip-offset value.  If it cannot, then it might be
>>>> better to have multiple tuples in the 'reg' property for each discrete
>>>> chip.  Multiple reg tuples would also remove the need for the
>>>> num-chips property.
>>> The node still describes one device mapping all relevant control
>>> registers. How about using fsl,upm-chip-offsets = <0x200 0x400>. It
>>> would be more generic and makes num-chips obsolete as well. And the
>>> property would be reserved for that way of implementing the chip select
>>> in hardware.
>>
>> It really sounds like this binding is describing multiple NAND chips
>> mapped to different base addresses (and looking at the fsm_upm.c
>> driver appears to confirm it).  So, does this work?  reg = <3 0x200 4
>>  3 0x400 4>;
>
> The chip-offset, and not the address, needs to be added to the MAR
> register as well before running the pattern:
>
>        mar = cmd << (32 - fun->upm.width);
>
>        if (fun->chip_offset && fun->chip_number > 0)
>
>                mar |= fun->chip_number * fun->chip_offset;
>
>        fsl_upm_run_pattern(&fun->upm, chip->IO_ADDR_R, mar);
>
>
>> It is true that other methods could be used for implementing the chip
>> select, but that is *not* what the proposed binding describes.  This
>> proposed binding describes NAND chips selected by address lines
>> (particular addresses), and in this case I think using reg is the
>> natural description.
>
> OK, the chips are selected by accessing a defined address range. Will
> prepare a patch using the reg property.

Hold on a sec.  I'm debating from my experience with device tree
bindings, but I'm fairly ignorant about the implementation of NAND on
UPM.  It *looks* to me like reg is sufficient, but if I'm wrong then
tell me so and why.  Your comment above about fsl_upm_run_pattern()
makes me doubt my position.

Does using the reg property give the driver enough information to
reliably program the MAR for NAND connections that use the address
line chip select scheme?  Related to that, should the binding include
a property that explicitly states that an address line chip select
scheme is being used?

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Wolfgang Grandegger]

Grant Likely wrote:
> On Thu, Mar 26, 2009 at 1:42 AM, Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org> wrote:
>> Grant Likely wrote:
>>> On Wed, Mar 25, 2009 at 2:48 PM, Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org> wrote:
>>>> Grant Likely wrote:
>>>>> For the chip offset, it's not clear what the meaning is.  First, does
>>>>> the UPM controller support access of multiple chips simultaneously?
>>>> The offset drives the corresponding address lines, which are used to
>>>> select the chip. That's how it's done on the TQM8548 board. In
>>>> principle, the chips could also be selected through dedicated GPIO pins.
>>>> Well, I'm not a hardware guy.
>>> Heh.  I mean elaborate in the binding documentation.  :-)
>>>
>>>>> If so, then can you elaborate in the description on how board design
>>>>> translates to a chip-offset value.  If it cannot, then it might be
>>>>> better to have multiple tuples in the 'reg' property for each discrete
>>>>> chip.  Multiple reg tuples would also remove the need for the
>>>>> num-chips property.
>>>> The node still describes one device mapping all relevant control
>>>> registers. How about using fsl,upm-chip-offsets = <0x200 0x400>. It
>>>> would be more generic and makes num-chips obsolete as well. And the
>>>> property would be reserved for that way of implementing the chip select
>>>> in hardware.
>>> It really sounds like this binding is describing multiple NAND chips
>>> mapped to different base addresses (and looking at the fsm_upm.c
>>> driver appears to confirm it).  So, does this work?  reg = <3 0x200 4
>>>  3 0x400 4>;
>> The chip-offset, and not the address, needs to be added to the MAR
>> register as well before running the pattern:
>>
>>        mar = cmd << (32 - fun->upm.width);
>>
>>        if (fun->chip_offset && fun->chip_number > 0)
>>
>>                mar |= fun->chip_number * fun->chip_offset;
>>
>>        fsl_upm_run_pattern(&fun->upm, chip->IO_ADDR_R, mar);
>>
>>
>>> It is true that other methods could be used for implementing the chip
>>> select, but that is *not* what the proposed binding describes.  This
>>> proposed binding describes NAND chips selected by address lines
>>> (particular addresses), and in this case I think using reg is the
>>> natural description.
>> OK, the chips are selected by accessing a defined address range. Will
>> prepare a patch using the reg property.
> 
> Hold on a sec.  I'm debating from my experience with device tree

I already started ;-).

> bindings, but I'm fairly ignorant about the implementation of NAND on
> UPM.  It *looks* to me like reg is sufficient, but if I'm wrong then
> tell me so and why.  Your comment above about fsl_upm_run_pattern()
> makes me doubt my position.

It's not sufficient to just map the related space and access it, at least.

> Does using the reg property give the driver enough information to
> reliably program the MAR for NAND connections that use the address
> line chip select scheme?  Related to that, should the binding include

In principle yes:

  if (i > 0)
      offset[i] = resource[i].start - resource[0].start;

> a property that explicitly states that an address line chip select
> scheme is being used?

That's why I'm still in favor of:

  fsl,upm-multi-chip-offsets = <0x200 0x400>

That would state that the address line chip select scheme is used with
the specified offsets. It also allows for a more elegant solution
(code-wise).

Wolfgang.

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Grant Likely]

On Thu, Mar 26, 2009 at 9:33 AM, Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org> wrote:
> Grant Likely wrote:
>> Does using the reg property give the driver enough information to
>> reliably program the MAR for NAND connections that use the address
>> line chip select scheme?  Related to that, should the binding include
>
> In principle yes:
>
>  if (i > 0)
>      offset[i] = resource[i].start - resource[0].start;

Ewww.  That's ugly.

>> a property that explicitly states that an address line chip select
>> scheme is being used?
>
> That's why I'm still in favor of:
>
>  fsl,upm-multi-chip-offsets = <0x200 0x400>
>
> That would state that the address line chip select scheme is used with
> the specified offsets. It also allows for a more elegant solution
> (code-wise).

Alright.  Then at the very least the property name should reflect that
address lines CS is used to reduce the chance of confusion with
another multi-chip scheme.  Something like
fsl,upm-addr-line-cs-offsets maybe?

Here is another thought.  The binding is describing that address lines
are used to activate CS lines.  Offset for chip access purposes is
derived from the address line, but it doesn't directly describe the
hardware.  The following may be a better description of the hardware.

fsl,upm-addr-line-cs = <9 10>;

g.



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Wolfgang Grandegger]

Grant Likely wrote:
> On Thu, Mar 26, 2009 at 9:33 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> Grant Likely wrote:
>>> Does using the reg property give the driver enough information to
>>> reliably program the MAR for NAND connections that use the address
>>> line chip select scheme?  Related to that, should the binding include
>> In principle yes:
>>
>>  if (i > 0)
>>      offset[i] = resource[i].start - resource[0].start;
> 
> Ewww.  That's ugly.

Yep.

>>> a property that explicitly states that an address line chip select
>>> scheme is being used?
>> That's why I'm still in favor of:
>>
>>  fsl,upm-multi-chip-offsets = <0x200 0x400>
>>
>> That would state that the address line chip select scheme is used with
>> the specified offsets. It also allows for a more elegant solution
>> (code-wise).
> 
> Alright.  Then at the very least the property name should reflect that
> address lines CS is used to reduce the chance of confusion with
> another multi-chip scheme.  Something like
> fsl,upm-addr-line-cs-offsets maybe?


> 
> Here is another thought.  The binding is describing that address lines
> are used to activate CS lines.  Offset for chip access purposes is
> derived from the address line, but it doesn't directly describe the
> hardware.  The following may be a better description of the hardware.
> 
> fsl,upm-addr-line-cs = <9 10>;

The TQM8548 hardware has some logic connected to the two address lines
allowing to select up to 4 chips with two address lines:

 fsl,upm-addr-line-cs-offsets = <0x0 0x200 0x400 0x600>

That's the more general solution.

Wolfgang.

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Grant Likely]

On Thu, Mar 26, 2009 at 10:35 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
> Grant Likely wrote:
>> On Thu, Mar 26, 2009 at 9:33 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>>> Grant Likely wrote:
>>>> Does using the reg property give the driver enough information to
>>>> reliably program the MAR for NAND connections that use the address
>>>> line chip select scheme?  Related to that, should the binding include
>>> In principle yes:
>>>
>>>  if (i > 0)
>>>      offset[i] = resource[i].start - resource[0].start;
>>
>> Ewww.  That's ugly.
>
> Yep.
>
>>>> a property that explicitly states that an address line chip select
>>>> scheme is being used?
>>> That's why I'm still in favor of:
>>>
>>>  fsl,upm-multi-chip-offsets = <0x200 0x400>
>>>
>>> That would state that the address line chip select scheme is used with
>>> the specified offsets. It also allows for a more elegant solution
>>> (code-wise).
>>
>> Alright.  Then at the very least the property name should reflect that
>> address lines CS is used to reduce the chance of confusion with
>> another multi-chip scheme.  Something like
>> fsl,upm-addr-line-cs-offsets maybe?
>
>
>>
>> Here is another thought.  The binding is describing that address lines
>> are used to activate CS lines.  Offset for chip access purposes is
>> derived from the address line, but it doesn't directly describe the
>> hardware.  The following may be a better description of the hardware.
>>
>> fsl,upm-addr-line-cs = <9 10>;
>
> The TQM8548 hardware has some logic connected to the two address lines
> allowing to select up to 4 chips with two address lines:
>
>  fsl,upm-addr-line-cs-offsets = <0x0 0x200 0x400 0x600>

Ah.  I see.  This is board specific then.  I think it is premature to
try and define a generic solution here because it depends on custom
board hardware and different boards could use very different logic.
The next board could end up doing something completely different.  I'd
rather start to see trends in multiple boards implementing the same
scheme before trying to craft a generic scheme.

In other words, this device is not register-level compatible with the
fsl,upm-nand device.  Give the node a new compatible value
(tqc,tqm8548-upm-nand) and add another entry to the of_fun_match table
for the new device.  Use the .data element in the match table to
supply an alternate fun_cmd_ctrl() function for this board (instead of
using a property value do decide which fun_cmd_ctrl() behaviour to
use).  New boards that *do* use the same addressing scheme can claim
compatibility with tqc,tqm8548-upm-nand.

You can still use the property names already discussed, but only
document them as valid for the tqc,tqm8548-upm-nand variant of the
device.

Very little will need to change in your patch to handle it this way.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Anton Vorontsov]

On Thu, Mar 26, 2009 at 11:02:06AM -0600, Grant Likely wrote:
[]
> >> Here is another thought.  The binding is describing that address lines
> >> are used to activate CS lines.  Offset for chip access purposes is
> >> derived from the address line, but it doesn't directly describe the
> >> hardware.  The following may be a better description of the hardware.
> >>
> >> fsl,upm-addr-line-cs = <9 10>;
> >
> > The TQM8548 hardware has some logic connected to the two address lines
> > allowing to select up to 4 chips with two address lines:
> >
> >  fsl,upm-addr-line-cs-offsets = <0x0 0x200 0x400 0x600>
> 
> Ah.  I see.  This is board specific then.  I think it is premature to
> try and define a generic solution here because it depends on custom
> board hardware and different boards could use very different logic.
> The next board could end up doing something completely different.  I'd
> rather start to see trends in multiple boards implementing the same
> scheme before trying to craft a generic scheme.
> 
> In other words, this device is not register-level compatible with the
> fsl,upm-nand device.  Give the node a new compatible value
> (tqc,tqm8548-upm-nand) and add another entry to the of_fun_match table
> for the new device.  Use the .data element in the match table to
> supply an alternate fun_cmd_ctrl() function for this board (instead of
> using a property value do decide which fun_cmd_ctrl() behaviour to
> use).  New boards that *do* use the same addressing scheme can claim
> compatibility with tqc,tqm8548-upm-nand.

I don't like this. :-/

UPM is an universal thing, so there are thousands of ways we can
connect NAND to the UPM. Of which only ~10 would be sane (others are
insane, and nobody would do this. If they do, _then_ we'll fall back
to <board>-upm-nand scheme for a particular board).

I don't see any problem with fsl,upm-addr-line-cs-offsets. It can
describe any scheme in "addr lines are cs" connection, it's a common
setup for multi-chip memory, we shouldn't treat it is as something
extraordinary.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Wolfgang Grandegger]

Anton Vorontsov wrote:
> On Thu, Mar 26, 2009 at 11:02:06AM -0600, Grant Likely wrote:
> []
>>>> Here is another thought.  The binding is describing that address lines
>>>> are used to activate CS lines.  Offset for chip access purposes is
>>>> derived from the address line, but it doesn't directly describe the
>>>> hardware.  The following may be a better description of the hardware.
>>>>
>>>> fsl,upm-addr-line-cs = <9 10>;
>>> The TQM8548 hardware has some logic connected to the two address lines
>>> allowing to select up to 4 chips with two address lines:
>>>
>>>  fsl,upm-addr-line-cs-offsets = <0x0 0x200 0x400 0x600>
>> Ah.  I see.  This is board specific then.  I think it is premature to
>> try and define a generic solution here because it depends on custom
>> board hardware and different boards could use very different logic.
>> The next board could end up doing something completely different.  I'd
>> rather start to see trends in multiple boards implementing the same
>> scheme before trying to craft a generic scheme.
>>
>> In other words, this device is not register-level compatible with the
>> fsl,upm-nand device.  Give the node a new compatible value
>> (tqc,tqm8548-upm-nand) and add another entry to the of_fun_match table
>> for the new device.  Use the .data element in the match table to
>> supply an alternate fun_cmd_ctrl() function for this board (instead of
>> using a property value do decide which fun_cmd_ctrl() behaviour to
>> use).  New boards that *do* use the same addressing scheme can claim
>> compatibility with tqc,tqm8548-upm-nand.
> 
> I don't like this. :-/
> 
> UPM is an universal thing, so there are thousands of ways we can
> connect NAND to the UPM. Of which only ~10 would be sane (others are
> insane, and nobody would do this. If they do, _then_ we'll fall back
> to <board>-upm-nand scheme for a particular board).

Yep.

> I don't see any problem with fsl,upm-addr-line-cs-offsets. It can
> describe any scheme in "addr lines are cs" connection, it's a common
> setup for multi-chip memory, we shouldn't treat it is as something
> extraordinary.

I fully agree. I'm going to provide a patch on monday.

Wolfgang.

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Grant Likely]

On Thu, Mar 26, 2009 at 4:14 PM, Wolfgang Grandegger <wg@grandegger.com> wrote:
> Anton Vorontsov wrote:
>> On Thu, Mar 26, 2009 at 11:02:06AM -0600, Grant Likely wrote:
>>> In other words, this device is not register-level compatible with the
>>> fsl,upm-nand device.  Give the node a new compatible value
>>> (tqc,tqm8548-upm-nand) and add another entry to the of_fun_match table
>>> for the new device.  Use the .data element in the match table to
>>> supply an alternate fun_cmd_ctrl() function for this board (instead of
>>> using a property value do decide which fun_cmd_ctrl() behaviour to
>>> use).  New boards that *do* use the same addressing scheme can claim
>>> compatibility with tqc,tqm8548-upm-nand.
>>
>> I don't like this. :-/
>>
>> UPM is an universal thing, so there are thousands of ways we can
>> connect NAND to the UPM. Of which only ~10 would be sane (others are
>> insane, and nobody would do this. If they do, _then_ we'll fall back
>> to <board>-upm-nand scheme for a particular board).
>
> Yep.
>
>> I don't see any problem with fsl,upm-addr-line-cs-offsets. It can
>> describe any scheme in "addr lines are cs" connection, it's a common
>> setup for multi-chip memory, we shouldn't treat it is as something
>> extraordinary.
>
> I fully agree. I'm going to provide a patch on monday.

Well, I still don't think it is the wisest choice.  My position is
that it is better to be conservative and pedantic now because it is
easy to relax the rules from that point.  If it turns out after some
experience with "fsl,upm-addr-line-cs-offset" that the scheme has a
serious flaw, then the impact is contained.  On the other side, if it
is confirmed and useful and correct, it is a trivial change to make it
available to everything that claims compatibility with fsl,upm-nand.

That said, I won't oppose it if you go this route.  However at the
very least, please change the nand node's compatible list to be:

compatible = "tqc,tqm8548-upm-nand", "fsl,upm-nand";

The custom glue logic makes it something unique, so "tqc,..." should
be at the start of the list to describe it as such, even if the driver
only ever uses "fsl,upm-nand".

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Anton Vorontsov]

On Thu, Mar 26, 2009 at 05:22:36PM -0600, Grant Likely wrote:
[...]
> That said, I won't oppose it if you go this route.  However at the
> very least, please change the nand node's compatible list to be:
> 
> compatible = "tqc,tqm8548-upm-nand", "fsl,upm-nand";

Yeah, that's definitely a good idea.

-- 
Anton Vorontsov
email: cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
irc://irc.freenode.net/bd2

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Wolfgang Grandegger]

Grant Likely wrote:
> On Thu, Mar 26, 2009 at 4:14 PM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> Anton Vorontsov wrote:
>>> On Thu, Mar 26, 2009 at 11:02:06AM -0600, Grant Likely wrote:
>>>> In other words, this device is not register-level compatible with the
>>>> fsl,upm-nand device.  Give the node a new compatible value
>>>> (tqc,tqm8548-upm-nand) and add another entry to the of_fun_match table
>>>> for the new device.  Use the .data element in the match table to
>>>> supply an alternate fun_cmd_ctrl() function for this board (instead of
>>>> using a property value do decide which fun_cmd_ctrl() behaviour to
>>>> use).  New boards that *do* use the same addressing scheme can claim
>>>> compatibility with tqc,tqm8548-upm-nand.
>>> I don't like this. :-/
>>>
>>> UPM is an universal thing, so there are thousands of ways we can
>>> connect NAND to the UPM. Of which only ~10 would be sane (others are
>>> insane, and nobody would do this. If they do, _then_ we'll fall back
>>> to <board>-upm-nand scheme for a particular board).
>> Yep.
>>
>>> I don't see any problem with fsl,upm-addr-line-cs-offsets. It can
>>> describe any scheme in "addr lines are cs" connection, it's a common
>>> setup for multi-chip memory, we shouldn't treat it is as something
>>> extraordinary.
>> I fully agree. I'm going to provide a patch on monday.
> 
> Well, I still don't think it is the wisest choice.  My position is
> that it is better to be conservative and pedantic now because it is
> easy to relax the rules from that point.  If it turns out after some
> experience with "fsl,upm-addr-line-cs-offset" that the scheme has a
> serious flaw, then the impact is contained.  On the other side, if it
> is confirmed and useful and correct, it is a trivial change to make it
> available to everything that claims compatibility with fsl,upm-nand.
> 
> That said, I won't oppose it if you go this route.  However at the
> very least, please change the nand node's compatible list to be:
> 
> compatible = "tqc,tqm8548-upm-nand", "fsl,upm-nand";
> 
> The custom glue logic makes it something unique, so "tqc,..." should
> be at the start of the list to describe it as such, even if the driver
> only ever uses "fsl,upm-nand".

That's a good idea in case we need it lateron. For the time being, I
prefer to make the driver as generic as possible. Currently there are
only two boards using the driver, the MPC8360RTDK and the TQM8548. and
it's unlikely that there will be much more in the future.

Wolfgang.