Re: [PATCH v1] mtd: nand: tango: import driver for tango controller

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Mon, 5 Sep 2016 12:18:26 +0200
Mason <slash.tmp@free.fr> wrote:

> On 05/09/2016 09:14, Boris Brezillon wrote:
> 
> > Marc Gonzalez wrote:
> >   
> >> +/* ERROR_REPORT values */
> >> +#define DECODE_ERR_ON_PKT_0(v)	(~v & BIT(7))
> >> +#define DECODE_ERR_ON_PKT_N(v)	(~v & BIT(15))  
> > 
> > Is this really packet N? I'd say it's packet 1.  
> 
> NB: "packet_0" and "packet_n" are terms used in the controller's
> documentation (which is not publicly available AFAIU).
> 
> I didn't want to change the names, for fear of confusing a
> future (internal) maintainer of the code. But in my mind,
> ERR_ON_FIRST_PKT and ERR_ON_OTHER_PKT are clearer. Perhaps
> I can use these identifiers, with a comment mentioning the
> "internal" names.

Or do it the other way around: keep the datasheet wording and add a
comment explaining what it means in your code.

> 
> DECODE_ERR_ON_PKT_N(v) actually means:
> "decode error on packet N, for N > 0"
> (The HW supports splitting a page in 1 to 16 packets.)
> 
> > #define DECODE_ERR_ON_PKT(pkt, v)	(~(v) & BIT(((pkt) * 8) + 7))  
> 
> Byte 0 is the report for packet 0.
> Byte 1 is the report for other packets.
> 
> >> +#define ERR_COUNT_PKT_0(v)	((v >> 0) & 0x3f)
> >> +#define ERR_COUNT_PKT_N(v)	((v >> 8) & 0x3f)  
> 
> There are only two bytes in the error report.
> ERR_COUNT_PKT_N(v) returns the max error count for N > 1

Okay, then again, explain that in a comment.

> 
> >> +struct tango_chip {
> >> +	struct nand_chip chip;
> >> +	void __iomem *base;  
> > 
> > I think it better to encode the CS id, and calculate the __iomem offset
> > based on that at run-time. Especially if you want to support multi-CS
> > (multi dies) chips, which you don't seem to support here.  
> 
> To support multi-CS chips, I would have to define a
> select_chip callback, where I save the requested CS
> inside the struct tango_chip and use that to compute
> the offset later?

Yes, that's a solution.

> 
> 
> >> +	u32 timing1, timing2, xfer_cfg, pkt_0_cfg, pkt_n_cfg, bb_cfg;  
> > 
> > Please, one field per line in struct definitions.  
> 
> OK.
> 
> 
> >> +#define NFC_BUSY(base) (readl_relaxed(base + NFC_STATUS_REG) & 0xf)  
> > 
> > I'd turn that one into an inline function taking a nand_chip pointer in
> > parameter. Or drop it completely, since you only have one user.  
> 
> OK.
> 
> 
> > And please document why you use this 0xf mask? I guess there's one bit
> > per CS, so masking with 0xf is not necessarily a good idea...  
> 
> There's a 4-bit status code, 0 means idle, non-0 means busy.
> I'll document the mask.
> 
> 
> >> +	res = readl_relaxed(nfc->mem_base + ERROR_REPORT);
> >> +
> >> +	if (DECODE_ERR_ON_PKT_0(res) || DECODE_ERR_ON_PKT_N(res))
> >> +		return -EBADMSG;  
> > 
> > Hm, you're assuming you'll always have 2 packets in your layout, is
> > this true? Don't you have layouts where you only have one packet (2k
> > pages with 2k packets) ?  
> 
> The legacy driver hard-codes packet size to 1024.
> Thus 2 packets for 2k pages, 4 packets for 4k pages.
> Are there recent NAND chips with 1k pages?

Nope. Actually I didn't understand the meaning of PKT_N(). Now that you
clarified the situation (it returns the max value for packet > 0), I
fine with your implementation.

> 
> 
> >> +	writel_relaxed(MODE_MLC, nfc->pbus_base + PBUS_PAD_MODE);  
> > 
> > Not sure I understand what MLC means here? Can you give more detail?  
> 
> I'll rename it to MODE_NFC (for "NAND Flash Controller").
> 
> Basically, either we access NAND chips directly in "raw" mode,
> or we go through the NAND Flash Controller.
> 
> My driver uses "raw" mode for everything except read_page and
> write_page (because they require DMA and HW ECC).

Okay, then yes, please choose a better name.

> 
> 
> >> +	ecc_bits = p->chip.ecc.strength * FIELD_ORDER;  
> > 
> > Are you sure the field order is always 15? I thought the ECC controller
> > was adapting it depending on the packet size (512 bytes packets => 13,
> > 1024 bytes => 14, 2k => 15), but maybe I'm wrong.  
> 
> If I'm reading the doc right, it's always 15.

Okay, so, no matter what packet size you choose, the number of ECC
bytes for a given strength will always be the same, right?

From a storage PoV, that means you'd better choose the biggest packet
size, but I understand that you want to support existing layouts, and
this may also have an impact if you want to support subpage I/Os.

> 
> 
> >> +static int tango_nand_probe(struct platform_device *pdev)
> >> +{
> >> +	int i, kHz;
> >> +	struct resource *res;
> >> +	void __iomem *addr[3];
> >> +	struct tango_nfc *nfc;
> >> +	struct device_node *np;
> >> +
> >> +	struct clk *clk = clk_get(&pdev->dev, NULL);
> >> +	if (IS_ERR(clk))
> >> +		return PTR_ERR(clk);
> >> +
> >> +	nfc = devm_kzalloc(&pdev->dev, sizeof(*nfc), GFP_KERNEL);
> >> +	if (!nfc)
> >> +		return -ENOMEM;
> >> +
> >> +	for (i = 0; i < 3; ++i) {
> >> +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> >> +		addr[i] = devm_ioremap_resource(&pdev->dev, res);
> >> +		if (IS_ERR(addr[i]))
> >> +			return PTR_ERR(addr[i]);
> >> +	}
> >> +
> >> +	platform_set_drvdata(pdev, nfc);
> >> +	nand_hw_control_init(&nfc->hw);
> >> +	kHz = clk_get_rate(clk) / 1000;
> >> +
> >> +	nfc->reg_base	= addr[0];
> >> +	nfc->mem_base	= addr[1];
> >> +	nfc->pbus_base	= addr[2];  
> > 
> > Why not doing
> > 
> > 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > 	nfc->reg_base = devm_ioremap_resource(&pdev->dev, res);
> > 
> > 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

I meant 

	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);

> > 	nfc->mem_base = devm_ioremap_resource(&pdev->dev, res);  
> 
> Do you mean why do I have a loop for the 3 ioremaps?
> IIUC, you'd prefer that I unroll the loop?

Having a loop would be appropriate if you were filling an nfc->iomems[]
array, but here you're just putting the values in a temporary table to
then assign each entry to a different field in your NFC struct.
It's not really useful in my opinion.