Re: [PATCH 7/7] mtd: rawnand: sunxi: Precompute the ECC_CTL register value

[Miquel Raynal]

Hi Samuel,

samuel@xxxxxxxxxxxx wrote on Thu, 29 Dec 2022 12:15:26 -0600:

> This removes an unnecessary memory allocation, and avoids recomputing
> the same register value every time ECC is enabled.

I am fine with the "let's not recompute the register value each time"
idea, but I like having a dedicated object reserved for the ECC
engine, that is separated from the main controller structure (both
are two different things, even though they are usually well
integrated).

If it's actually useless you can still get rid of the allocation and in
the structure you can save the ecc_ctrl reg value instead of mode.

The other patches in the series look good to me.

Thanks,
Miquèl

> Signed-off-by: Samuel Holland <samuel@xxxxxxxxxxxx>
> ---
> 
>  drivers/mtd/nand/raw/sunxi_nand.c | 75 ++++++-------------------------
>  1 file changed, 13 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/sunxi_nand.c b/drivers/mtd/nand/raw/sunxi_nand.c
> index a3bc9f7f9e5a..5c5a567d8870 100644
> --- a/drivers/mtd/nand/raw/sunxi_nand.c
> +++ b/drivers/mtd/nand/raw/sunxi_nand.c
> @@ -169,22 +169,13 @@ struct sunxi_nand_chip_sel {
>  	s8 rb;
>  };
>  
> -/**
> - * struct sunxi_nand_hw_ecc - stores information related to HW ECC support
> - *
> - * @mode: the sunxi ECC mode field deduced from ECC requirements
> - */
> -struct sunxi_nand_hw_ecc {
> -	int mode;
> -};
> -
>  /**
>   * struct sunxi_nand_chip - stores NAND chip device related information
>   *
>   * @node: used to store NAND chips into a list
>   * @nand: base NAND chip structure
> - * @ecc: ECC controller structure
>   * @clk_rate: clk_rate required for this NAND chip
> + * @ecc_ctl: ECC_CTL register value for this NAND chip
>   * @timing_cfg: TIMING_CFG register value for this NAND chip
>   * @timing_ctl: TIMING_CTL register value for this NAND chip
>   * @nsels: number of CS lines required by the NAND chip
> @@ -193,8 +184,8 @@ struct sunxi_nand_hw_ecc {
>  struct sunxi_nand_chip {
>  	struct list_head node;
>  	struct nand_chip nand;
> -	struct sunxi_nand_hw_ecc *ecc;
>  	unsigned long clk_rate;
> +	u32 ecc_ctl;
>  	u32 timing_cfg;
>  	u32 timing_ctl;
>  	int nsels;
> @@ -689,26 +680,15 @@ static void sunxi_nfc_hw_ecc_enable(struct nand_chip *nand)
>  {
>  	struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
>  	struct sunxi_nfc *nfc = to_sunxi_nfc(nand->controller);
> -	u32 ecc_ctl;
> -
> -	ecc_ctl = readl(nfc->regs + NFC_REG_ECC_CTL);
> -	ecc_ctl &= ~(NFC_ECC_MODE_MSK | NFC_ECC_PIPELINE |
> -		     NFC_ECC_BLOCK_SIZE_MSK);
> -	ecc_ctl |= NFC_ECC_EN | NFC_ECC_MODE(sunxi_nand->ecc->mode) |
> -		   NFC_ECC_EXCEPTION | NFC_ECC_PIPELINE;
> -
> -	if (nand->ecc.size == 512)
> -		ecc_ctl |= NFC_ECC_BLOCK_512;
>  
> -	writel(ecc_ctl, nfc->regs + NFC_REG_ECC_CTL);
> +	writel(sunxi_nand->ecc_ctl, nfc->regs + NFC_REG_ECC_CTL);
>  }
>  
>  static void sunxi_nfc_hw_ecc_disable(struct nand_chip *nand)
>  {
>  	struct sunxi_nfc *nfc = to_sunxi_nfc(nand->controller);
>  
> -	writel(readl(nfc->regs + NFC_REG_ECC_CTL) & ~NFC_ECC_EN,
> -	       nfc->regs + NFC_REG_ECC_CTL);
> +	writel(0, nfc->regs + NFC_REG_ECC_CTL);
>  }
>  
>  static inline void sunxi_nfc_user_data_to_buf(u32 user_data, u8 *buf)
> @@ -1626,11 +1606,6 @@ static const struct mtd_ooblayout_ops sunxi_nand_ooblayout_ops = {
>  	.free = sunxi_nand_ooblayout_free,
>  };
>  
> -static void sunxi_nand_hw_ecc_ctrl_cleanup(struct sunxi_nand_chip *sunxi_nand)
> -{
> -	kfree(sunxi_nand->ecc);
> -}
> -
>  static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>  				       struct nand_ecc_ctrl *ecc,
>  				       struct device_node *np)
> @@ -1641,7 +1616,6 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>  	struct mtd_info *mtd = nand_to_mtd(nand);
>  	struct nand_device *nanddev = mtd_to_nanddev(mtd);
>  	int nsectors;
> -	int ret;
>  	int i;
>  
>  	if (nanddev->ecc.user_conf.flags & NAND_ECC_MAXIMIZE_STRENGTH) {
> @@ -1675,10 +1649,6 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>  	if (ecc->size != 512 && ecc->size != 1024)
>  		return -EINVAL;
>  
> -	sunxi_nand->ecc = kzalloc(sizeof(*sunxi_nand->ecc), GFP_KERNEL);
> -	if (!sunxi_nand->ecc)
> -		return -ENOMEM;
> -
>  	/* Prefer 1k ECC chunk over 512 ones */
>  	if (ecc->size == 512 && mtd->writesize > 512) {
>  		ecc->size = 1024;
> @@ -1699,12 +1669,9 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>  
>  	if (i >= ARRAY_SIZE(strengths)) {
>  		dev_err(nfc->dev, "unsupported strength\n");
> -		ret = -ENOTSUPP;
> -		goto err;
> +		return -ENOTSUPP;
>  	}
>  
> -	sunxi_nand->ecc->mode = i;
> -
>  	/* HW ECC always request ECC bytes for 1024 bytes blocks */
>  	ecc->bytes = DIV_ROUND_UP(ecc->strength * fls(8 * 1024), 8);
>  
> @@ -1713,10 +1680,14 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>  
>  	nsectors = mtd->writesize / ecc->size;
>  
> -	if (mtd->oobsize < ((ecc->bytes + 4) * nsectors)) {
> -		ret = -EINVAL;
> -		goto err;
> -	}
> +	if (mtd->oobsize < ((ecc->bytes + 4) * nsectors))
> +		return -EINVAL;
> +
> +	sunxi_nand->ecc_ctl = NFC_ECC_MODE(i) | NFC_ECC_EXCEPTION |
> +			      NFC_ECC_PIPELINE | NFC_ECC_EN;
> +
> +	if (ecc->size == 512)
> +		sunxi_nand->ecc_ctl |= NFC_ECC_BLOCK_512;
>  
>  	ecc->read_oob = sunxi_nfc_hw_ecc_read_oob;
>  	ecc->write_oob = sunxi_nfc_hw_ecc_write_oob;
> @@ -1739,25 +1710,6 @@ static int sunxi_nand_hw_ecc_ctrl_init(struct nand_chip *nand,
>  	ecc->write_oob_raw = nand_write_oob_std;
>  
>  	return 0;
> -
> -err:
> -	kfree(sunxi_nand->ecc);
> -
> -	return ret;
> -}
> -
> -static void sunxi_nand_ecc_cleanup(struct sunxi_nand_chip *sunxi_nand)
> -{
> -	struct nand_ecc_ctrl *ecc = &sunxi_nand->nand.ecc;
> -
> -	switch (ecc->engine_type) {
> -	case NAND_ECC_ENGINE_TYPE_ON_HOST:
> -		sunxi_nand_hw_ecc_ctrl_cleanup(sunxi_nand);
> -		break;
> -	case NAND_ECC_ENGINE_TYPE_NONE:
> -	default:
> -		break;
> -	}
>  }
>  
>  static int sunxi_nand_attach_chip(struct nand_chip *nand)
> @@ -1970,7 +1922,6 @@ static void sunxi_nand_chips_cleanup(struct sunxi_nfc *nfc)
>  		ret = mtd_device_unregister(nand_to_mtd(chip));
>  		WARN_ON(ret);
>  		nand_cleanup(chip);
> -		sunxi_nand_ecc_cleanup(sunxi_nand);
>  		list_del(&sunxi_nand->node);
>  	}
>  }