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

[Samuel Holland]

Hi Miquèl,

On 1/2/23 03:30, Miquel Raynal wrote:
> 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.

OK, I will do this, and split it into two patches: one for replacing
mode with ecc_ctrl, and the second to keep the structure but get rid of
the allocation.

Regards,
Samuel

> 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);
>>  	}
>>  }