Re: [PATCH 05/17] mtd: rawnand: cafe: Demistify register fields

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: linux-mtd@lists.infradead.org,
	Tudor Ambarus <tudor.ambarus@microchip.com>,
	David Woodhouse <dwmw2@infradead.org>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Richard Weinberger <richard@nod.at>
Subject: Re: [PATCH 05/17] mtd: rawnand: cafe: Demistify register fields
Date: Mon, 27 Apr 2020 21:42:11 +0200	[thread overview]
Message-ID: <20200427214116.3ea7fa82@xps13> (raw)
In-Reply-To: <20200427082028.394719-6-boris.brezillon@collabora.com>

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 27 Apr
2020 10:20:15 +0200:

> The driver has a bunch of magic values. Let's define proper register
> fields based on the this spec

                  ^^^/^^^^
either one or the other

> http://wiki.laptop.org/images/5/5c/88ALP01_Datasheet_July_2007.pdf and
> use them.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/mtd/nand/raw/cafe_nand.c | 351 ++++++++++++++++++++++++-------
>  1 file changed, 270 insertions(+), 81 deletions(-)
> 

[...]

> +#define CAFE_GLOBAL_IRQ				0x3008
> +#define CAFE_GLOBAL_IRQ_MASK			0x300c
> +#define CAFE_GLOBAL_IRQ_PCI_ERROR		BIT(31)
> +#define CAFE_GLOBAL_IRQ_VPD_TWSI		BIT(26)
> +#define CAFE_GLOBAL_IRQ_CCIC			BIT(2)
> +#define CAFE_GLOBAL_IRQ_SDH			BIT(1)
> +#define CAFE_GLOBAL_IRQ_NAND			BIT(0)
> +
> +#define CAFE_GLOBAL_RESET			0x3034
> +#define CAFE_GLOBAL_RESET_CCIC			BIT(2)
> +#define CAFE_GLOBAL_RESET_SDH			BIT(1)
> +#define CAFE_GLOBAL_RESET_NAND			BIT(0)
> +
> +#define CAFE_FIELD_PREP(reg, field, val)	FIELD_PREP(CAFE_##reg##_##field, val)
> +#define CAFE_FIELD_GET(reg, field, val)		FIELD_GET(CAFE_##reg##_##field, val)

hehe :)

>  
>  struct cafe_priv {
>  	struct nand_chip nand;
> @@ -104,7 +195,8 @@ static const char *part_probes[] = { "cmdlinepart", "RedBoot", NULL };
>  static int cafe_device_ready(struct nand_chip *chip)
>  {
>  	struct cafe_priv *cafe = nand_get_controller_data(chip);
> -	int result = !!(cafe_readl(cafe, NAND_STATUS) & 0x40000000);
> +	int result = !!(cafe_readl(cafe, NAND_STATUS) &
> +			CAFE_NAND_STATUS_FLASH_BUSY);
>  	uint32_t irqs = cafe_readl(cafe, NAND_IRQ);
>  
>  	cafe_writel(cafe, irqs, NAND_IRQ);

[...]

> @@ -318,14 +430,14 @@ static void cafe_select_chip(struct nand_chip *chip, int chipnr)
>  {
>  	struct cafe_priv *cafe = nand_get_controller_data(chip);
>  
> +	if (chipnr < 0 || chipnr > 1)
> +		return;
> +

I think this change should not be part of this patch?

>  	cafe_dev_dbg(&cafe->pdev->dev, "select_chip %d\n", chipnr);
>  
>  	/* Mask the appropriate bit into the stored value of ctl1
>  	   which will be used by cafe_nand_cmdfunc() */
> -	if (chipnr)
> -		cafe->ctl1 |= CTRL1_CHIPSELECT;
> -	else
> -		cafe->ctl1 &= ~CTRL1_CHIPSELECT;
> +	cafe->ctl1 |= CAFE_FIELD_PREP(NAND_CTRL1, CE, chipnr);

I don't master these macros yet, but are you sure CTRL1_CHIPSELECT will
actually get cleared if (!chipnr) ?

>  }
>  
>  static irqreturn_t cafe_nand_interrupt(int irq, void *id)
> @@ -334,7 +446,9 @@ static irqreturn_t cafe_nand_interrupt(int irq, void *id)
>  	struct nand_chip *chip = mtd_to_nand(mtd);
>  	struct cafe_priv *cafe = nand_get_controller_data(chip);
>  	uint32_t irqs = cafe_readl(cafe, NAND_IRQ);
> -	cafe_writel(cafe, irqs & ~0x90000000, NAND_IRQ);
> +	cafe_writel(cafe,
> +		    irqs & ~(CAFE_NAND_IRQ_CMD_DONE | CAFE_NAND_IRQ_DMA_DONE),
> +		    NAND_IRQ);
>  	if (!irqs)
>  		return IRQ_NONE;
>  

Thanks,
Miquèl

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