Re: [PATCH 1/2] mtd: nand: Add Cadence NAND controller driver

[Piotr Sroka]

Hi Boris

The 01/29/2019 19:19, Boris Brezillon wrote:
> > +		NAND_OP_PARSER_PAT_ADDR_ELEM(false, MAX_ADDRESS_CYC)),
> > +	NAND_OP_PARSER_PATTERN(
> > +		cadence_nand_cmd,
> > +		NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, MAX_DATA_SIZE)),
> > +	NAND_OP_PARSER_PATTERN(
> > +		cadence_nand_cmd,
> > +		NAND_OP_PARSER_PAT_DATA_OUT_ELEM(false, MAX_DATA_SIZE)),
> > +	NAND_OP_PARSER_PATTERN(
> > +		cadence_nand_waitrdy,
> > +		NAND_OP_PARSER_PAT_WAITRDY_ELEM(false))
> > +	);
>
> Are you sure you can't pack several instructions into an atomic
> controller operation? I'd be surprised if that was not the case...
All write and reads operations are handled by function pointers. Apart from that I
could handle erase command as a atomic operation. I will add such function to
the parser. 
> > +static int cadence_nand_set_ecc_strength(struct cdns_nand_info *cdns_nand,
> > +					 u8 strength)
> > +{
> > +	u32 reg;
> > +	u8 i, corr_str_idx = 0;
> > +
> > +	if (cadence_nand_wait_for_idle(cdns_nand)) {
> > +		dev_err(cdns_nand->dev, "Error. Controller is busy");
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	for (i = 0; i < BCH_MAX_NUM_CORR_CAPS; i++) {
> > +		if (cdns_nand->ecc_strengths[i] == strength) {
> > +			corr_str_idx = i;
> > +			break;
> > +		}
> > +	}
>
> The index should be retrieved at init time and stored somewhere to avoid
> searching it every time this function is called.
Function is called only once at initilization stage. Do we need to make
a optimalization in succh case?  
> > +
> > +	reg = readl(cdns_nand->reg + ECC_CONFIG_0);
> > +	reg &= ~ECC_CONFIG_0_CORR_STR;
> > +	reg |= FIELD_PREP(ECC_CONFIG_0_CORR_STR, corr_str_idx);
> > +	writel(reg, cdns_nand->reg + ECC_CONFIG_0);
> > +
> > +	return 0;
> > +}
> > +
>
> ...
>
> > +
> > +static int cadence_nand_set_erase_detection(struct cdns_nand_info *cdns_nand,
> > +					    bool enable,
> > +					    u8 bitflips_threshold)
> > +{
> > +	u32 reg;
> > +
> > +	if (cadence_nand_wait_for_idle(cdns_nand)) {
> > +		dev_err(cdns_nand->dev, "Error. Controller is busy");
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	reg = readl(cdns_nand->reg + ECC_CONFIG_0);
> > +
> > +	if (enable)
> > +		reg |= ECC_CONFIG_0_ERASE_DET_EN;
> > +	else
> > +		reg &= ~ECC_CONFIG_0_ERASE_DET_EN;
> > +
> > +	writel(reg, cdns_nand->reg + ECC_CONFIG_0);
> > +
> > +	writel(bitflips_threshold, cdns_nand->reg + ECC_CONFIG_1);
>
> I'm curious, is the threshold defining the max number of bitflips in a
> page or in an ECC-chunk (ecc_step_size)?
Threshold defines number of max bitflips in a sector/ecc_step_size. 
> > +static void
> > +cadence_nand_irq_cleanup(int irqnum, struct cdns_nand_info *cdns_nand)
> > +{
> > +	/* disable interrupts */
> > +	writel(INTR_ENABLE_INTR_EN, cdns_nand->reg + INTR_ENABLE);
> > +	free_irq(irqnum, cdns_nand);
>
> You don't need that if you use devm_request_irq(), do you?
I agree I do not need it I forgot to remove it.
> > +
> > +static int cadence_nand_calc_ecc_bytes_256(int step_size, int strength)
> > +{
> > +	return cadence_nand_calc_ecc_bytes(256, strength);
> > +}
> > +
> > +static int cadence_nand_calc_ecc_bytes_512(int step_size, int strength)
> > +{
> > +	return cadence_nand_calc_ecc_bytes(512, strength);
> > +}
> > +
> > +static int cadence_nand_calc_ecc_bytes_1024(int step_size, int strength)
> > +{
> > +	return cadence_nand_calc_ecc_bytes(1024, strength);
> > +}
> > +
> > +static int cadence_nand_calc_ecc_bytes_2048(int step_size, int strength)
> > +{
> > +	return  cadence_nand_calc_ecc_bytes(2048, strength);
> > +}
> > +
> > +static int cadence_nand_calc_ecc_bytes_4096(int step_size, int strength)
> > +{
> > +	return  cadence_nand_calc_ecc_bytes(4096, strength);
> > +}
>
> And you absolutely don't need those wrappers, just use
> cadence_nand_calc_ecc_bytes() directly.
Unfortunately I need these  wrappers. It is because size of ecc 
does not depend on selected step_size which is on parameter list but it
depends on maximum supported ecc step size which is not avaible in this
function. Lets say that controller supports the following steps sizes
512, 1024, 2048. No matter what step size is set the calculation will be
made always for step size 2048. 
I could use such function directly if it contains nand_chip
parameter. Then I could get this parameter from cdns_nand.  
> I'm stopping here, but I think you got the idea: there's a lot of
> duplicated code in this driver, try to factor this out or simplify the
> logic.
Thans for review. I will try to simplify the rest of code by myself.


Regards,

Piotr Sroka