Hi Christophe,
Sorry for the delay, here are some answers from my previous comments.
Maybe you already addressed them, in this case please ignore them.
Also, please run and correct 'checkpatch.pl --strict' issues (mostly
uses of uint8_t instead of u8 but also a warning about the compatible).
Overall the driver is in a pretty good shape and should enter the next
release. I'll apply the patches after -rc1 once I'll have your v3+ with
everything corrected.
[...]
> >> index c7efc31..863662c 100644
> >> --- a/drivers/mtd/nand/raw/Kconfig
> >> +++ b/drivers/mtd/nand/raw/Kconfig
> >> @@ -541,4 +541,13 @@ config MTD_NAND_TEGRA
> >> is supported. Extra OOB bytes when using HW ECC are currently
> >> not supported.
> >> >> +config MTD_NAND_STM32_FMC2
> >> + tristate "Support for NAND controller on STM32MP SoCs"
> >> + depends on MACH_STM32MP157 || COMPILE_TEST
> >> + help
> >> + Enables support for NAND Flash chips on SoCs containing the FMC2
> >> + NAND controller. This controller is found on STM32MP SoCs.
> >> + The driver supports a maximum 8k page size. HW ECC is enabled and
> >> + supports a maximum 8-bit correction error per sector of 512 bytes.
> > > HW ECC should not be enabled by default. 8-bit/512B of correctability
> > is good but not that high and people might want to use software ECC in
> > conjunction with raw accesses.
>
> Yes, I agree. The driver only supports NAND_ECC_HW mode. NAND_ECC_SOFT mode was not requested by customers and was not implemented. The driver could be improved later to support mode like NAND_ECC_SOFT or NAND_ECC_ON_DIE. Should I remove "HW ECC is enabled" from Kconfig description?
Yes, please.
[...]
> >> +/* Select function */
> >> +static void stm32_fmc2_select_chip(struct nand_chip *chip, int chipnr)
> >> +{
> >> + struct stm32_fmc2 *fmc2 = nand_get_controller_data(chip);
> >> + struct dma_slave_config dma_cfg;
> >> +
> >> + if (chipnr < 0 || chipnr >= fmc2->ncs)
> >> + return;
> >> +
> >> + if (fmc2->cs_used[chipnr] == fmc2->cs_sel)
> >> + return;
> >> +
> >> + fmc2->cs_sel = fmc2->cs_used[chipnr];
> >> +
> >> + if (fmc2->dma_tx_ch && fmc2->dma_rx_ch) {
> >> + memset(&dma_cfg, 0, sizeof(dma_cfg));
> >> + dma_cfg.src_addr = fmc2->data_phys_addr[fmc2->cs_sel];
> >> + dma_cfg.dst_addr = fmc2->data_phys_addr[fmc2->cs_sel];
> >> + dma_cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> >> + dma_cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> >> + dma_cfg.src_maxburst = 32;
> >> + dma_cfg.dst_maxburst = 32;
> >> +
> >> + if (dmaengine_slave_config(fmc2->dma_tx_ch, &dma_cfg))
> >> + dev_warn(fmc2->dev, "tx DMA engine slave config failed\n");
> >> +
> >> + if (dmaengine_slave_config(fmc2->dma_rx_ch, &dma_cfg))
> >> + dev_warn(fmc2->dev, "rx DMA engine slave config failed\n");
> >> + }
> > > What if there are two NAND chips using different timing modes? You
> > should probably reconfigure the timings registers, unless there are
> > already a set of timing registers per CS?
>
> Yes, it's true. In case of 2 NAND chips, timings and pcr registers should have been reconfigured. But, the driver only supports one NAND chip connected to 1 or 2 CS. There was no requirement on our side to support 2 different NAND chips. I do not have a board to test such configuration, so i do not want to deliver this support without being able to test it on my side. The driver will be improved later to support 2 different NAND chips, in case this configuration is requested by customers.
Sure, I'm not requesting you to support 2 NAND chips, I'm just
requesting to write this driver in a manner so that adding support for a
2nd NAND chip would be easy thanks to a better software design. That's
actually something that is done in the marvell_nand.c driver if you
need inspiration.
[...]
> >> +
> >> +void stm32_fmc2_read_data(struct nand_chip *chip, void *buf,
> >> + unsigned int len, bool force_8bit)
> >> +{
> >> + struct stm32_fmc2 *fmc2 = nand_get_controller_data(chip);
> >> + void __iomem *io_addr_r = fmc2->data_base[fmc2->cs_sel];
> >> + u8 *p = buf;
> >> + unsigned int i;
> >> +
> >> + if (force_8bit)
> >> + goto read_8bit;
> >> +
> >> + if (IS_ALIGNED((u32)buf, sizeof(u32)) && IS_ALIGNED(len, sizeof(u32))) {
> > > If you selected BOUNCE_BUFFER in the options, buf is supposedly
> > aligned, or am I missing something?
>
> 2 FMC2 internal modes can be used:
> - Sequencer mode (Patch 2/3): dmas are used and NAND_USE_BOUNCE_BUFFER option is selected.
> - Manual mode (Patch 3/3): no dma channel is used and NAND_USE_BOUNCE_BUFFER is not selected.
> Should i select NAND_USE_BOUNCE_BUFFER for sequencer and manual mode, and remove IS_ALIGNED test on buf?
If it's only useful in manual mode after patch 3/3, then the logic for
it should be in patch 3 also.
Anyway, unless numbers show a significant drop off in the throughput
(but I suppose the sequencer mode is faster anyway?) I think this is a
good idea to always use the bounce buffer and keep the code simple.
[...]
> >> +static int stm32_fmc2_parse_dt(struct device *dev,
> >> + struct stm32_fmc2 *fmc2)
> >> +{
> >> + struct device_node *dn = dev->of_node;
> >> + struct device_node *child;
> >> + int nchips = of_get_child_count(dn);
> >> + int ret = 0;
> >> +
> >> + if (!nchips) {
> >> + dev_err(dev, "NAND chip not defined\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (nchips > 1) {
> > > If you have two CS, can't you have two NAND chips connected?
> >
> No HW board has been designed with 2 NAND chips connected. I am not able to test this configuration. The driver will be improved when i will be able to test such configuration.
>
Ok.
Thanks,
Miquèl
