Re: [PATCH v4 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

On Tue, 10 May 2016 10:50:31 -0400
Jorge Ramirez <jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> On 05/10/2016 08:13 AM, Boris Brezillon wrote:
> >> +	if (config->codec == ECC_ENC) {  
> >> >+		/* configure ECC encoder (in bits) */
> >> >+		enc_sz = config->enc_len << 3;
> >> >+
> >> >+		reg = ecc_bit | (config->ecc_mode << ECC_MODE_SHIFT);
> >> >+		reg |= (enc_sz << ECC_MS_SHIFT);
> >> >+		writel(reg, ecc->regs + ECC_ENCCNFG);
> >> >+
> >> >+		if (config->ecc_mode != ECC_NFI_MODE)
> >> >+			writel(lower_32_bits(config->addr),
> >> >+				ecc->regs + ECC_ENCDIADDR);
> >> >+
> >> >+	} else {
> >> >+		/* configure ECC decoder (in bits) */
> >> >+		dec_sz = config->dec_len;
> >> >+
> >> >+		reg = ecc_bit | (config->ecc_mode << ECC_MODE_SHIFT);
> >> >+		reg |= (dec_sz << ECC_MS_SHIFT) | DEC_CNFG_CORRECT;
> >> >+		reg |= DEC_EMPTY_EN;
> >> >+		writel(reg, ecc->regs + ECC_DECCNFG);
> >> >+
> >> >+		if (config->sec_mask)
> >> >+			ecc->sec_mask = 1 << (config->sec_mask - 1);
> >> >+	}  
> > I see that some of the logic could be shared between the ENC and DEC
> > cases.  
> 
> I guess you are referring to
> reg = ecc_bit | (config->ecc_mode << ECC_MODE_SHIFT);
> 
> ok...

and

reg |= (sz << ECC_MS_SHIFT);

Ok, maybe it's not so important.

> 
> > BTW, why do you multiply enc_len by 8 (bits to byte conversion), but
> > don't do that for dec_len?
> >  
> 
> just as needed by the hardware:
> the config is in bits, the encoder register requires bytes, the decoder 
> register requires bits.
> 

Are you sure about that? Cause it seems to me that the NAND controller
drivers put a length in bits in ->dec_len and a length in bytes in
->enc_len, and then you have an extra conversion in the ECC engine
driver code for enc_len to convert it into a value in bits.

I don't care if you decide to store this value in bytes or bits, but it
should be the same unit for both fields (and I even think we should
have a single field for both encoding and decoding mode).

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com