Re: [U-Boot] [PATCH] mtd: nand: fsl-ifc: fix support of multiple NAND devices

[Kurt Kanzenbach]

On Thu, Oct 19, 2017 at 09:30:09PM -0500, Scott Wood wrote:
> On Tue, Oct 17, 2017 at 10:00:45AM +0200, Kurt Kanzenbach wrote:
> > Currently the chipselect used to identify the corresponding NAND chip is 
> > stored
> > at the controller and only set during fsl_ifc_chip_init(). This way, only 
> > the
> > last NAND chip is working, as the previous value of cs_nand gets 
> > overwritten.
> >
> > In order to solve this issue the chipselect is moved from the controller to 
> > the
> > NAND chip structure. Thus, the correct chipselect for each NAND chip 
> > operation
> > is used.
> >
> > Tested on hardware with two NAND chips connected to the IFC controller.
> >
> > Signed-off-by: Kurt Kanzenbach 
> > ---
> >  drivers/mtd/nand/fsl_ifc_nand.c | 16 +---
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c 
> > b/drivers/mtd/nand/fsl_ifc_nand.c
> > index bc6bdc9b2c..57737dbe94 100644
> > --- a/drivers/mtd/nand/fsl_ifc_nand.c
> > +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> > @@ -36,6 +36,7 @@ struct fsl_ifc_mtd {
> >
> > struct device *dev;
> > int bank;   /* Chip select bank number*/
> > +   unsigned int cs_nand;   /* On which chipsel NAND is connected */
>
> This is redundant with "bank" -- just do like the Linux driver does
> and write "priv->bank << IFC_NAND_CSEL_SHIFT" directly to the register
> when needed.

Sure, no problem.

>
> > -static int fsl_ifc_sram_init(uint32_t ver)
> > +static int fsl_ifc_sram_init(struct mtd_info *mtd, uint32_t ver)
> >  {
> > +   struct nand_chip *chip = mtd_to_nand(mtd);
> > +   struct fsl_ifc_mtd *priv = nand_get_controller_data(chip);
>
> Could pass in priv instead of mtd to make it more like the Linux driver.
> It's best to avoid gratuitous differences.

Makes sense. I'll send a v2.

Thanks,
Kurt

>
> -Scott
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] mtd: nand: fsl-ifc: fix support of multiple NAND devices

[Scott Wood]

On Tue, Oct 17, 2017 at 10:00:45AM +0200, Kurt Kanzenbach wrote:
> Currently the chipselect used to identify the corresponding NAND chip is 
> stored
> at the controller and only set during fsl_ifc_chip_init(). This way, only the
> last NAND chip is working, as the previous value of cs_nand gets overwritten.
> 
> In order to solve this issue the chipselect is moved from the controller to 
> the
> NAND chip structure. Thus, the correct chipselect for each NAND chip operation
> is used.
> 
> Tested on hardware with two NAND chips connected to the IFC controller.
> 
> Signed-off-by: Kurt Kanzenbach 
> ---
>  drivers/mtd/nand/fsl_ifc_nand.c | 16 +---
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
> index bc6bdc9b2c..57737dbe94 100644
> --- a/drivers/mtd/nand/fsl_ifc_nand.c
> +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> @@ -36,6 +36,7 @@ struct fsl_ifc_mtd {
>  
>   struct device *dev;
>   int bank;   /* Chip select bank number*/
> + unsigned int cs_nand;   /* On which chipsel NAND is connected */

This is redundant with "bank" -- just do like the Linux driver does
and write "priv->bank << IFC_NAND_CSEL_SHIFT" directly to the register
when needed.

> -static int fsl_ifc_sram_init(uint32_t ver)
> +static int fsl_ifc_sram_init(struct mtd_info *mtd, uint32_t ver)
>  {
> + struct nand_chip *chip = mtd_to_nand(mtd);
> + struct fsl_ifc_mtd *priv = nand_get_controller_data(chip);

Could pass in priv instead of mtd to make it more like the Linux driver. 
It's best to avoid gratuitous differences.

-Scott
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot