Re: [PATCH] mtd: atmel-quadspi: add suspend/resume hooks
On Wed, 23 May 2018 19:08:48 +0300
Claudiu Beznea wrote:
> Implement suspend/resume hooks.
>
> Signed-off-by: Claudiu Beznea
> ---
> drivers/mtd/spi-nor/atmel-quadspi.c | 23 +++
> 1 file changed, 23 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/atmel-quadspi.c
> b/drivers/mtd/spi-nor/atmel-quadspi.c
> index 6c5708bacad8..85d7610fb920 100644
> --- a/drivers/mtd/spi-nor/atmel-quadspi.c
> +++ b/drivers/mtd/spi-nor/atmel-quadspi.c
> @@ -737,6 +737,28 @@ static int atmel_qspi_remove(struct platform_device
> *pdev)
> return 0;
> }
>
> +#ifdef CONFIG_PM_SLEEP
> +static int atmel_qspi_suspend(struct device *dev)
> +{
> + struct atmel_qspi *aq = dev_get_drvdata(dev);
> +
> + clk_disable_unprepare(aq->clk);
> +
> + return 0;
> +}
> +
> +static int atmel_qspi_resume(struct device *dev)
> +{
> + struct atmel_qspi *aq = dev_get_drvdata(dev);
> +
> + clk_prepare_enable(aq->clk);
> +
> + return atmel_qspi_init(aq);
> +}
> +#endif
You can avoid this #ifdef section if you use the __maybe_unused
specifier:
static __maybe_unused int atmel_qspi_suspend(struct device *dev)
...
> +
> +static SIMPLE_DEV_PM_OPS(atmel_qspi_pm_ops, atmel_qspi_suspend,
> + atmel_qspi_resume);
>
> static const struct of_device_id atmel_qspi_dt_ids[] = {
> { .compatible = "atmel,sama5d2-qspi" },
> @@ -749,6 +771,7 @@ static struct platform_driver atmel_qspi_driver = {
> .driver = {
> .name = "atmel_qspi",
> .of_match_table = atmel_qspi_dt_ids,
> + .pm = &atmel_qspi_pm_ops,
> },
> .probe = atmel_qspi_probe,
> .remove = atmel_qspi_remove,
Re: [PATCH] mtd: nand: raw: atmel: add module param to avoid using dma
On Tue, 29 May 2018 17:46:21 +0200 Boris Brezillon wrote: > On Tue, 29 May 2018 18:21:40 +0300 > Eugen Hristev wrote: > > > On 29.05.2018 18:15, Boris Brezillon wrote: > > > On Tue, 29 May 2018 18:01:40 +0300 > > > Eugen Hristev wrote: > > > > > >> [...] > > >> > > >> > > >>> > > >>> I think you're missing something here. We use the DMA engine in memcpy > > >>> mode (SRAM -> DRAM), not in device mode (dev -> DRAM or DRAM -> dev). > > >>> So there's no dmas prop defined in the DT and there should not be. > > >>> > > >>> Regards, > > >>> > > >>> Boris > > >>> > > >> > > >> Ok, so the memcpy SRAM <-> DRAM will hog the transfer between DRAM and > > >> LCD if my understanding is correct. That's the DMA that Peter wants to > > >> disable with his patch ? > > >> > > >> Then we can then try to force NFC SRAM DMA channels to use just DDR port > > >> 1 or 2 for memcpy ? > > > > > > You mean the dmaengine? According to "14.1.3 Master to Slave Access" > > > that's already the case. > > > > > > Only DMAC0 can access the NFC SRAM and it's done through DMAC0:IF0, > > > then access to DDR is going through port DDR port 1 (DMAC0:IF1) or 2 > > > (DMAC0:IF0). > > > > If we can make NFC use port 1 only, then HLCDC could have two ports as > > master 8 & 9, maybe a better bandwidth. > > Peter, can you try with the following patch? Actually that won't work because all SRAMs are on IF0, and here we use DMA memcpy to copy things from/to the SRAM to/from the DRAM. I have no simple solution to force usage of IF1 when accessing the DRAM but I'm also not sure this will solve Peter's problem since forcing LCDC to use DDR port 3 did not make things better. > > --->8--- > diff --git a/drivers/dma/at_hdmac_regs.h b/drivers/dma/at_hdmac_regs.h > index ef3f227ce3e6..2a48e870f292 100644 > --- a/drivers/dma/at_hdmac_regs.h > +++ b/drivers/dma/at_hdmac_regs.h > @@ -124,8 +124,8 @@ > #defineATC_SIF(i) (0x3 & (i)) /* Src tx done via > AHB-Lite Interface i */ > #defineATC_DIF(i) ((0x3 & (i)) << 4) /* Dst tx > done via AHB-Lite Interface i */ > /* Specify AHB interfaces */ > -#define AT_DMA_MEM_IF 0 /* interface 0 as memory interface */ > -#define AT_DMA_PER_IF 1 /* interface 1 as peripheral interface */ > +#define AT_DMA_MEM_IF 1 /* interface 0 as memory interface */ > +#define AT_DMA_PER_IF 0 /* interface 1 as peripheral interface */ > > #defineATC_SRC_PIP (0x1 << 8) /* Source > Picture-in-Picture enabled */ > #defineATC_DST_PIP (0x1 << 12) /* Destination > Picture-in-Picture enabled */
Re: [PATCH] mtd: nand: raw: atmel: add module param to avoid using dma
On Tue, 29 May 2018 18:21:40 +0300 Eugen Hristev wrote: > On 29.05.2018 18:15, Boris Brezillon wrote: > > On Tue, 29 May 2018 18:01:40 +0300 > > Eugen Hristev wrote: > > > >> [...] > >> > >> > >>> > >>> I think you're missing something here. We use the DMA engine in memcpy > >>> mode (SRAM -> DRAM), not in device mode (dev -> DRAM or DRAM -> dev). > >>> So there's no dmas prop defined in the DT and there should not be. > >>> > >>> Regards, > >>> > >>> Boris > >>> > >> > >> Ok, so the memcpy SRAM <-> DRAM will hog the transfer between DRAM and > >> LCD if my understanding is correct. That's the DMA that Peter wants to > >> disable with his patch ? > >> > >> Then we can then try to force NFC SRAM DMA channels to use just DDR port > >> 1 or 2 for memcpy ? > > > > You mean the dmaengine? According to "14.1.3 Master to Slave Access" > > that's already the case. > > > > Only DMAC0 can access the NFC SRAM and it's done through DMAC0:IF0, > > then access to DDR is going through port DDR port 1 (DMAC0:IF1) or 2 > > (DMAC0:IF0). > > If we can make NFC use port 1 only, then HLCDC could have two ports as > master 8 & 9, maybe a better bandwidth. Peter, can you try with the following patch? --->8--- diff --git a/drivers/dma/at_hdmac_regs.h b/drivers/dma/at_hdmac_regs.h index ef3f227ce3e6..2a48e870f292 100644 --- a/drivers/dma/at_hdmac_regs.h +++ b/drivers/dma/at_hdmac_regs.h @@ -124,8 +124,8 @@ #defineATC_SIF(i) (0x3 & (i)) /* Src tx done via AHB-Lite Interface i */ #defineATC_DIF(i) ((0x3 & (i)) << 4) /* Dst tx done via AHB-Lite Interface i */ /* Specify AHB interfaces */ -#define AT_DMA_MEM_IF 0 /* interface 0 as memory interface */ -#define AT_DMA_PER_IF 1 /* interface 1 as peripheral interface */ +#define AT_DMA_MEM_IF 1 /* interface 0 as memory interface */ +#define AT_DMA_PER_IF 0 /* interface 1 as peripheral interface */ #defineATC_SRC_PIP (0x1 << 8) /* Source Picture-in-Picture enabled */ #defineATC_DST_PIP (0x1 << 12) /* Destination Picture-in-Picture enabled */
Re: [PATCH] mtd: nand: raw: atmel: add module param to avoid using dma
On Tue, 29 May 2018 18:01:40 +0300 Eugen Hristev wrote: > [...] > > > > > > I think you're missing something here. We use the DMA engine in memcpy > > mode (SRAM -> DRAM), not in device mode (dev -> DRAM or DRAM -> dev). > > So there's no dmas prop defined in the DT and there should not be. > > > > Regards, > > > > Boris > > > > Ok, so the memcpy SRAM <-> DRAM will hog the transfer between DRAM and > LCD if my understanding is correct. That's the DMA that Peter wants to > disable with his patch ? > > Then we can then try to force NFC SRAM DMA channels to use just DDR port > 1 or 2 for memcpy ? You mean the dmaengine? According to "14.1.3 Master to Slave Access" that's already the case. Only DMAC0 can access the NFC SRAM and it's done through DMAC0:IF0, then access to DDR is going through port DDR port 1 (DMAC0:IF1) or 2 (DMAC0:IF0). > > I have also received a suggestion to try to increase the porches in > LCDC_LCDCFG3 . Yep, Nicolas suggested something similar. Peter, can you try that?
Re: [PATCH] mtd: nand: raw: atmel: add module param to avoid using dma
Hi Eugen, On Tue, 29 May 2018 09:30:54 +0300 Eugen Hristev wrote: > On 28.05.2018 13:10, Peter Rosin wrote: > > On 2018-05-28 00:11, Peter Rosin wrote: > >> On 2018-05-27 11:18, Peter Rosin wrote: > >>> On 2018-05-25 16:51, Tudor Ambarus wrote: > We think the best way is to keep LCD on DDR Ports 2 and 3 (8th and 9th > slaves), to have maximum bandwidth and to use DMA on DDR port 1 for NAND > (7th slave). > >>> > >>> Exactly how do I accomplish that? > >>> > >>> I can see how I can move the LCD between slave DDR port 2 and 3 by > >>> selecting LCDC DMA master 8 or 9 (but according to the above it should > >>> not matter). The big question is how I control what slave the NAND flash > >>> is going to use? I find nothing in the datasheet, and the code is also > >>> non-transparent enough for me to figure it out by myself without > >>> throwing out this question first... > > >> [...] > > >> and the output is > >> > >> atmel-nand-controller 1000.ebi:nand-controller: using dma0chan5 for > >> DMA transfers > >> > >> So, DMA controller 0 is in use. I still don't know if IF0, IF1 or IF2 is > >> used > >> or how to find out. I guess IF2 is not in use since that does not allow any > >> DDR2 port as slave... > > Hello Peter, > > Thank you for all the information, I will chip in to help a little bit. > The Master/channel is described in the device tree. The channel has a > controller, a mem/periph interface and a periph ID, plus a FIFO > configuration. > > The dma chan number reported in the dmesg is just software. Here is an > example from DT: > dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(1)>, > <&dma0 2 AT91_DMA_CFG_PER_ID(2)>; > > you can match this with the help from > Documentation/devicetree/bindings/dma/atmel-dma.txt: > > 1. A phandle pointing to the DMA controller. > > 2. The memory interface (16 most significant bits), the peripheral > interface > (16 less significant bits). > > 3. Parameters for the at91 DMA configuration register which are device > > dependent: > >- bit 7-0: peripheral identifier for the hardware handshaking > interface. The >identifier can be different for tx and rx. > >- bit 11-8: FIFO configuration. 0 for half FIFO, 1 for ALAP, 2 for ASAP. > > > So , what was Tudor asking for, is your DT for the ebi node (if you are > using ebi), or, your NFC SRAM (Nand Flash Controller SRAM) DMA > devicetree chunk, so, we can figure out which type of DMA are you using. I think you're missing something here. We use the DMA engine in memcpy mode (SRAM -> DRAM), not in device mode (dev -> DRAM or DRAM -> dev). So there's no dmas prop defined in the DT and there should not be. Regards, Boris > > Normally, the ebi should be connected to both DMA0 and DMA1 on those > interfaces specified in DT. Which ones you want to use, depends on your > setup (and contention on the bus/accesses, like in your case, the HLCDC) > > Thats why we have multiple choices, to pick the right one for each case. > In our vanilla DT sama5d3.dtsi we do not have DMA described for ebi > interface. > > Eugen > > >> [...]
Re: [PATCH v2 3/6] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver
On Mon, 28 May 2018 18:41:26 +0200 Benjamin Lindqvist wrote: > Note that it's certainly possible to encode U-Boot and kernel with > RS[4] and still use RS[8] for the rootfs even if the boot rom doesn't > support it. Not if you want to read/write from/to the uboot partition from Linux. Per-partition ECC setup is not supported, so the only solutions we have right now are: 1/ use the same ECC config as the one use by the bootrom 2/ only access uboot partition in raw mode from Linux (that implies generating images containing the ECC bytes so that they can be flashed to the device in raw mode) 3/ never access uboot partitions from Linux (not sure we want that) > This whole 'use-bootable-ecc-only' business seems a bit > overengineered. > Hm, I don't find it over-engineered.
Re: [PATCH] mtd: nand: raw: atmel: add module param to avoid using dma
On Mon, 28 May 2018 17:52:53 +0200 Peter Rosin wrote: > On 2018-05-28 16:27, Boris Brezillon wrote: > > Hi Peter, > > > > On Mon, 28 May 2018 12:10:02 +0200 > > Peter Rosin wrote: > > > >> On 2018-05-28 00:11, Peter Rosin wrote: > >>> On 2018-05-27 11:18, Peter Rosin wrote: > >>>> On 2018-05-25 16:51, Tudor Ambarus wrote: > >>>>> We think the best way is to keep LCD on DDR Ports 2 and 3 (8th and 9th > >>>>> slaves), to have maximum bandwidth and to use DMA on DDR port 1 for NAND > >>>>> (7th slave). > >>>> > >>>> Exactly how do I accomplish that? > >>>> > >>>> I can see how I can move the LCD between slave DDR port 2 and 3 by > >>>> selecting LCDC DMA master 8 or 9 (but according to the above it should > >>>> not matter). The big question is how I control what slave the NAND flash > >>>> is going to use? I find nothing in the datasheet, and the code is also > >>>> non-transparent enough for me to figure it out by myself without > >>>> throwing out this question first... > >>> > >>> I added this: > >>> > >>> diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c > >>> b/drivers/mtd/nand/raw/atmel/nand-controller.c > >>> index e686fe73159e..3b33c63d2ed4 100644 > >>> --- a/drivers/mtd/nand/raw/atmel/nand-controller.c > >>> +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c > >>> @@ -1991,6 +1991,9 @@ static int atmel_nand_controller_init(struct > >>> atmel_nand_controller *nc, > >>> nc->dmac = dma_request_channel(mask, NULL, NULL); > >>> if (!nc->dmac) > >>> dev_err(nc->dev, "Failed to request DMA channel\n"); > >>> + > >>> + dev_info(nc->dev, "using %s for DMA transfers\n", > >>> + dma_chan_name(nc->dmac)); > >>> } > >>> > >>> /* We do not retrieve the SMC syscon when parsing old DTs. */ > >>> > >>> > >>> > >>> and the output is > >>> > >>> atmel-nand-controller 1000.ebi:nand-controller: using dma0chan5 for > >>> DMA transfers > >>> > >>> So, DMA controller 0 is in use. I still don't know if IF0, IF1 or IF2 is > >>> used > >>> or how to find out. I guess IF2 is not in use since that does not allow > >>> any > >>> DDR2 port as slave... > >>> > >>> From the datasheet, DMAC0/IF0 uses DDR2 Port 2, and DMAC0/IF1 uses DDR2 > >>> Port 1. > >>> But, by the looks of the register content in my other mail, it seems as if > >>> DMA0/IF1 can also use DDR2 Port 3. > >>> > >>> So, I think I want either > >>> > >>> A) the NAND controller to use DMAC0/IF0 (i.e. DDR2 port 1, and possibly > >>> 3) and > >>>the LCDC to use master 9 (i.e. DDR2 Port 2) > >>> > >>> or > >>> > >>> B) the NAND controller to use DMAC1/IF1 (i.e. DDR2 port 2) and the LCDC > >>> to use > >>>master 8 (i.e. DDR2 Port 3) > >> > >> Crap, that was not what I meant to express. Sorry for the confusion. This > >> is > >> better. > >> > >> So, I think I want either > >> > >> A) the NAND controller to use master 1 DMAC0/IF0 (i.e. slave 8 DDR2 port > >> 2) and > >>the LCDC to use master 9 (i.e. slave 9 DDR2 Port 3) > >> > >> or > >> > >> B) the NAND controller to use master 2 DMAC0/IF1 (i.e. slave 7 DDR2 port > >> 1, and > >>possibly slave 9 DDR2 port 3 (if my previous findings are relevant) and > >> the > >>LCDC to use master 8 (i.e. slave 8 DDR2 Port 2) > >> > >>> But, again, how do I limit DMAC0 to either of IF0 or IF1 for NAND > >>> accesses? > >> > >> So, I added a horrid patch (attached), which mainly adds printk lines, but > >> additionally does one more thing in atc_prep_dma_memcpy. It changes the > >> DSCR_IF > >> field (from 0) to 1 for DMA-memcpy for dma0chan5 (i.e. the NAND). At least > >> I > >> think it does that? > >> > >> Running with that patch gets me this: > >> > >> # dmesg | grep -i dma > >> at_hdmac e600.dma-controlle
Re: [PATCH 8/8] drm/bridge: cdns: mark PM functions as __maybe_unused
+Thierry
Hi Arnd,
On Fri, 25 May 2018 17:50:15 +0200
Arnd Bergmann wrote:
> These two functions are unused in some configurations, and using
> __maybe_unused
> is the easiest way to shut up the harmless warnings:
>
> drivers/gpu/drm/bridge/cdns-dsi.c:1353:12: error: 'cdns_dsi_suspend' defined
> but not used [-Werror=unused-function]
> static int cdns_dsi_suspend(struct device *dev)
> ^~~~
> drivers/gpu/drm/bridge/cdns-dsi.c:1340:12: error: 'cdns_dsi_resume' defined
> but not used [-Werror=unused-function]
> static int cdns_dsi_resume(struct device *dev)
>
> Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
> Signed-off-by: Arnd Bergmann
Hm, I thought such a patch had already been applied by Thierry [1].
[1]https://www.spinics.net/lists/dri-devel/msg174363.html
> ---
> drivers/gpu/drm/bridge/cdns-dsi.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c
> b/drivers/gpu/drm/bridge/cdns-dsi.c
> index c255fc3e1be5..f2d43f24acfb 100644
> --- a/drivers/gpu/drm/bridge/cdns-dsi.c
> +++ b/drivers/gpu/drm/bridge/cdns-dsi.c
> @@ -1337,7 +1337,7 @@ static const struct mipi_dsi_host_ops cdns_dsi_ops = {
> .transfer = cdns_dsi_transfer,
> };
>
> -static int cdns_dsi_resume(struct device *dev)
> +static int __maybe_unused cdns_dsi_resume(struct device *dev)
> {
> struct cdns_dsi *dsi = dev_get_drvdata(dev);
>
> @@ -1350,7 +1350,7 @@ static int cdns_dsi_resume(struct device *dev)
> return 0;
> }
>
> -static int cdns_dsi_suspend(struct device *dev)
> +static int __maybe_unused cdns_dsi_suspend(struct device *dev)
> {
> struct cdns_dsi *dsi = dev_get_drvdata(dev);
>
Re: [PATCH] mtd: nand: raw: atmel: add module param to avoid using dma
Hi Peter, On Mon, 28 May 2018 12:10:02 +0200 Peter Rosin wrote: > On 2018-05-28 00:11, Peter Rosin wrote: > > On 2018-05-27 11:18, Peter Rosin wrote: > >> On 2018-05-25 16:51, Tudor Ambarus wrote: > >>> We think the best way is to keep LCD on DDR Ports 2 and 3 (8th and 9th > >>> slaves), to have maximum bandwidth and to use DMA on DDR port 1 for NAND > >>> (7th slave). > >> > >> Exactly how do I accomplish that? > >> > >> I can see how I can move the LCD between slave DDR port 2 and 3 by > >> selecting LCDC DMA master 8 or 9 (but according to the above it should > >> not matter). The big question is how I control what slave the NAND flash > >> is going to use? I find nothing in the datasheet, and the code is also > >> non-transparent enough for me to figure it out by myself without > >> throwing out this question first... > > > > I added this: > > > > diff --git a/drivers/mtd/nand/raw/atmel/nand-controller.c > > b/drivers/mtd/nand/raw/atmel/nand-controller.c > > index e686fe73159e..3b33c63d2ed4 100644 > > --- a/drivers/mtd/nand/raw/atmel/nand-controller.c > > +++ b/drivers/mtd/nand/raw/atmel/nand-controller.c > > @@ -1991,6 +1991,9 @@ static int atmel_nand_controller_init(struct > > atmel_nand_controller *nc, > > nc->dmac = dma_request_channel(mask, NULL, NULL); > > if (!nc->dmac) > > dev_err(nc->dev, "Failed to request DMA channel\n"); > > + > > + dev_info(nc->dev, "using %s for DMA transfers\n", > > +dma_chan_name(nc->dmac)); > > } > > > > /* We do not retrieve the SMC syscon when parsing old DTs. */ > > > > > > > > and the output is > > > > atmel-nand-controller 1000.ebi:nand-controller: using dma0chan5 for DMA > > transfers > > > > So, DMA controller 0 is in use. I still don't know if IF0, IF1 or IF2 is > > used > > or how to find out. I guess IF2 is not in use since that does not allow any > > DDR2 port as slave... > > > > From the datasheet, DMAC0/IF0 uses DDR2 Port 2, and DMAC0/IF1 uses DDR2 > > Port 1. > > But, by the looks of the register content in my other mail, it seems as if > > DMA0/IF1 can also use DDR2 Port 3. > > > > So, I think I want either > > > > A) the NAND controller to use DMAC0/IF0 (i.e. DDR2 port 1, and possibly 3) > > and > >the LCDC to use master 9 (i.e. DDR2 Port 2) > > > > or > > > > B) the NAND controller to use DMAC1/IF1 (i.e. DDR2 port 2) and the LCDC to > > use > >master 8 (i.e. DDR2 Port 3) > > Crap, that was not what I meant to express. Sorry for the confusion. This is > better. > > So, I think I want either > > A) the NAND controller to use master 1 DMAC0/IF0 (i.e. slave 8 DDR2 port 2) > and >the LCDC to use master 9 (i.e. slave 9 DDR2 Port 3) > > or > > B) the NAND controller to use master 2 DMAC0/IF1 (i.e. slave 7 DDR2 port 1, > and >possibly slave 9 DDR2 port 3 (if my previous findings are relevant) and the >LCDC to use master 8 (i.e. slave 8 DDR2 Port 2) > > > But, again, how do I limit DMAC0 to either of IF0 or IF1 for NAND accesses? > > > > So, I added a horrid patch (attached), which mainly adds printk lines, but > additionally does one more thing in atc_prep_dma_memcpy. It changes the > DSCR_IF > field (from 0) to 1 for DMA-memcpy for dma0chan5 (i.e. the NAND). At least I > think it does that? > > Running with that patch gets me this: > > # dmesg | grep -i dma > at_hdmac e600.dma-controller: Atmel AHB DMA Controller ( cpy set slave ), > 8 channels > at_hdmac e800.dma-controller: Atmel AHB DMA Controller ( cpy set slave ), > 8 channels > dma dma0chan0: xlate 0 2 > dma dma0chan1: xlate 0 2 > at91_i2c f0014000.i2c: using dma0chan0 (tx) and dma0chan1 (rx) for DMA > transfers > dma dma1chan0: xlate 0 2 > dma dma1chan1: xlate 0 2 > at91_i2c f801c000.i2c: using dma1chan0 (tx) and dma1chan1 (rx) for DMA > transfers > dma dma0chan2: xlate 0 2 > dma dma0chan3: xlate 0 2 > dma dma0chan4: xlate 0 2 > atmel_mci f000.mmc: using dma0chan4 for DMA transfers > dma dma1chan2: xlate 0 2 > dma dma1chan3: xlate 0 2 > atmel_aes f8038000.aes: Atmel AES - Using dma1chan2, dma1chan3 for DMA > transfers > dma dma1chan4: xlate 0 2 > atmel_sha f8034000.sha: using dma1chan4 for DMA transfers > dma dma1chan5: xlate 0 2 > dma dma1chan6: xlate 0 2 > atmel_tdes f803c000.tdes: using dma1chan5, dma1chan6 for DMA transfers > atmel-nand-controller 1000.ebi:nand-controller: using dma0chan5 for DMA > transfers > dma dma0chan5: memcpy: 0 > dma dma0chan5: DSCR_IF: 1 > dma dma0chan5: memcpy: 1 > > So, output is as expected and I believe that the patch makes the NAND DMA > accesses use master 2 DMAC0/IF1 and are thus forced to use slave 7 DDR2 Port 1 > (and possibly 9). The LCDC is using slave 8 DDR2 Port 2. So there should be no > slave conflict? > > But the on-screen crap remains during NAND accesses. > > But pressing on. > > I then changed the priorities for all accesses to 0 in the PRxSy registers, > except > the ones for
Re: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver
On Sun, 27 May 2018 17:54:03 +0200 Miquel Raynal wrote: > Hi Boris, > > On Sun, 27 May 2018 17:13:37 +0200, Boris Brezillon > wrote: > > > On Sun, 27 May 2018 16:18:32 +0200 > > Miquel Raynal wrote: > > > > > Hi Stefan, > > > > > > On Thu, 24 May 2018 14:19:18 +0200, Stefan Agner > > > wrote: > > > > > > > On 24.05.2018 13:53, Boris Brezillon wrote: > > > > > Hi Benjamin, > > > > > > > > > > On Thu, 24 May 2018 13:30:14 +0200 > > > > > Benjamin Lindqvist wrote: > > > > > > > > > >> Hi Stefan, > > > > >> > > > > >> It seems to me that a probe similar to what the BootROM does > > > > >> shouldn't > > > > >> be awfully complicated to implement - just cycle through the switch > > > > >> cases in case of an ECC error. But I guess that's more of an idea for > > > > >> further improvements rather than a comment to the patch set under > > > > >> review. > > > > > > > > > > Nope, not really an option, because you're not guaranteed that the > > > > > NAND > > > > > will be used as a boot media, and the first page or first set of pages > > > > > might just be erased. > > > > > > > > > > > > > Yeah I did not meant probing like the Boot ROM does. > > > > > > > > What I meant was using only the ECC modes which are supported by the > > > > Boot ROM when the driver tries to choose a viable mode. So that would > > > > be: > > > > - RS t=4 > > > > - BCH t=8 > > > > - BCH t=16 > > > > > > > > Maybe we could add a property to enable that behavior: > > > > > > > > tegra,use-bootable-ecc-only; > > > > > > I'm not sure a property is needed. > > > > > > As there is currently no official user of this driver, why not turning > > > mandatory the nand-ecc-xxx properties? > > > > Not a big fan of this solution. We already have a few cases where the > > NAND part was changed on a design and the new NAND had different ECC > > requirements, With your suggestion, that means creating a new .dts file > > for each possible NAND part. > > > > Note that having a solution that picks the best ECC config based on > > chip->ecc_xxx_ds should be the preferred approach. nand-ecc- props are > > mainly here to address the case where you need/want to assign a config > > that does not match the ECC requirements exposed by the chip. > > Ok, that's right it's a problem. > > But then the driver has to choose a default algorithm if none is given. Yep. > In this case, should we select the one that fits best the NAND chip > requirements, or shall we limit to the ones supported by the BootRom? We should limit to the one used by the BootROM only if the NAND is used as a boot medium. > > The underlying question is: will we add a tegra,use-bootable-ecc-only > property? I guess this one is fine, because it's only adding a constraint on the possible ECC modes that can be used, it's not forcing a specific ECC strength. Note that if we want to make this property generic we could name it nand-is-boot-medium.
Re: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver
On Sun, 27 May 2018 16:18:32 +0200 Miquel Raynal wrote: > Hi Stefan, > > On Thu, 24 May 2018 14:19:18 +0200, Stefan Agner > wrote: > > > On 24.05.2018 13:53, Boris Brezillon wrote: > > > Hi Benjamin, > > > > > > On Thu, 24 May 2018 13:30:14 +0200 > > > Benjamin Lindqvist wrote: > > > > > >> Hi Stefan, > > >> > > >> It seems to me that a probe similar to what the BootROM does shouldn't > > >> be awfully complicated to implement - just cycle through the switch > > >> cases in case of an ECC error. But I guess that's more of an idea for > > >> further improvements rather than a comment to the patch set under > > >> review. > > > > > > Nope, not really an option, because you're not guaranteed that the NAND > > > will be used as a boot media, and the first page or first set of pages > > > might just be erased. > > > > > > > Yeah I did not meant probing like the Boot ROM does. > > > > What I meant was using only the ECC modes which are supported by the > > Boot ROM when the driver tries to choose a viable mode. So that would > > be: > > - RS t=4 > > - BCH t=8 > > - BCH t=16 > > > > Maybe we could add a property to enable that behavior: > > > > tegra,use-bootable-ecc-only; > > I'm not sure a property is needed. > > As there is currently no official user of this driver, why not turning > mandatory the nand-ecc-xxx properties? Not a big fan of this solution. We already have a few cases where the NAND part was changed on a design and the new NAND had different ECC requirements, With your suggestion, that means creating a new .dts file for each possible NAND part. Note that having a solution that picks the best ECC config based on chip->ecc_xxx_ds should be the preferred approach. nand-ecc- props are mainly here to address the case where you need/want to assign a config that does not match the ECC requirements exposed by the chip. > In the documentation you can add > a note saying that using other algorithms than the three above is not > supported by the BootROM. > > > > > >> > > >> However, I think that allowing for an override of the oobsize > > >> inference would be a good idea before merging, no? This could just be > > >> a trivial #ifdef (at least temporarily). If you agree but don't feel > > >> like doing it yourself, I'd be happy to pitch in. Let me know. > > > > > > That's why we have nand-ecc-xxx properties in the DT. > > > > > > > Yes, nand-ecc-strength is the first thing I plan to implement, that way > > strength can be defined in dt. > > > > -- > > Stefan > > Thanks, > Miquèl
Re: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver
On Thu, 24 May 2018 14:23:56 +0200
Boris Brezillon wrote:
> On Thu, 24 May 2018 13:09:53 +0200
> Stefan Agner wrote:
>
> > On 24.05.2018 10:56, Boris Brezillon wrote:
> > > On Thu, 24 May 2018 10:46:27 +0200
> > > Stefan Agner wrote:
> > >
> > >> Hi Boris,
> > >>
> > >> Thanks for the initial review! One small question below:
> > >>
> > >> On 23.05.2018 16:18, Boris Brezillon wrote:
> > >> > Hi Stefan,
> > >> >
> > >> > On Tue, 22 May 2018 14:07:06 +0200
> > >> > Stefan Agner wrote:
> > >> >> +
> > >> >> +struct tegra_nand {
> > >> >> + void __iomem *regs;
> > >> >> + struct clk *clk;
> > >> >> + struct gpio_desc *wp_gpio;
> > >> >> +
> > >> >> + struct nand_chip chip;
> > >> >> + struct device *dev;
> > >> >> +
> > >> >> + struct completion command_complete;
> > >> >> + struct completion dma_complete;
> > >> >> + bool last_read_error;
> > >> >> +
> > >> >> + dma_addr_t data_dma;
> > >> >> + void *data_buf;
> > >> >> + dma_addr_t oob_dma;
> > >> >> + void *oob_buf;
> > >> >> +
> > >> >> + int cur_chip;
> > >> >> +};
> > >> >
> > >> > This struct should be split in 2 structures: one representing the NAND
> > >> > controller and one representing the NAND chip:
> > >> >
> > >> > struct tegra_nand_controller {
> > >> >struct nand_hw_control base;
> > >> >void __iomem *regs;
> > >> >struct clk *clk;
> > >> >struct device *dev;
> > >> >struct completion command_complete;
> > >> >struct completion dma_complete;
> > >> >bool last_read_error;
> > >> >int cur_chip;
> > >> > };
> > >> >
> > >> > struct tegra_nand {
> > >> >struct nand_chip base;
> > >> >dma_addr_t data_dma;
> > >> >void *data_buf;
> > >> >dma_addr_t oob_dma;
> > >> >void *oob_buf;
> > >> > };
> > >>
> > >> Is there a particular reason why you would leave DMA buffers in the chip
> > >> structure? It seems that is more a controller thing...
> > >
> > > The size of those buffers is likely to be device dependent, so if you
> > > have several NANDs connected to the controller, you'll either have to
> > > have one buffer at the controller level which is max(all-chip-buf-size)
> > > or a buffer per device.
> > >
> > > Also, do you really need these buffers? The core already provide some
> > > which are suitable for DMA (chip->oob_poi and chip->data_buf).
> > >
> >
> > Good question, I am not sure, that was existing code.
> >
> > Are you sure data_buf it is DMA capable?
> >
> > nand_scan_tail allocates with kmalloc:
> >
> > chip->data_buf = kmalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL);
>
> Yes, kmalloc() allocates DMA-able buffers, so those are DMA-safe.
Hm, that's not exactly true. It depends on the dma_mask attached to the
device.
Re: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver
On Thu, 24 May 2018 13:09:53 +0200
Stefan Agner wrote:
> On 24.05.2018 10:56, Boris Brezillon wrote:
> > On Thu, 24 May 2018 10:46:27 +0200
> > Stefan Agner wrote:
> >
> >> Hi Boris,
> >>
> >> Thanks for the initial review! One small question below:
> >>
> >> On 23.05.2018 16:18, Boris Brezillon wrote:
> >> > Hi Stefan,
> >> >
> >> > On Tue, 22 May 2018 14:07:06 +0200
> >> > Stefan Agner wrote:
> >> >> +
> >> >> +struct tegra_nand {
> >> >> + void __iomem *regs;
> >> >> + struct clk *clk;
> >> >> + struct gpio_desc *wp_gpio;
> >> >> +
> >> >> + struct nand_chip chip;
> >> >> + struct device *dev;
> >> >> +
> >> >> + struct completion command_complete;
> >> >> + struct completion dma_complete;
> >> >> + bool last_read_error;
> >> >> +
> >> >> + dma_addr_t data_dma;
> >> >> + void *data_buf;
> >> >> + dma_addr_t oob_dma;
> >> >> + void *oob_buf;
> >> >> +
> >> >> + int cur_chip;
> >> >> +};
> >> >
> >> > This struct should be split in 2 structures: one representing the NAND
> >> > controller and one representing the NAND chip:
> >> >
> >> > struct tegra_nand_controller {
> >> > struct nand_hw_control base;
> >> > void __iomem *regs;
> >> > struct clk *clk;
> >> > struct device *dev;
> >> > struct completion command_complete;
> >> > struct completion dma_complete;
> >> > bool last_read_error;
> >> > int cur_chip;
> >> > };
> >> >
> >> > struct tegra_nand {
> >> > struct nand_chip base;
> >> > dma_addr_t data_dma;
> >> > void *data_buf;
> >> > dma_addr_t oob_dma;
> >> > void *oob_buf;
> >> > };
> >>
> >> Is there a particular reason why you would leave DMA buffers in the chip
> >> structure? It seems that is more a controller thing...
> >
> > The size of those buffers is likely to be device dependent, so if you
> > have several NANDs connected to the controller, you'll either have to
> > have one buffer at the controller level which is max(all-chip-buf-size)
> > or a buffer per device.
> >
> > Also, do you really need these buffers? The core already provide some
> > which are suitable for DMA (chip->oob_poi and chip->data_buf).
> >
>
> Good question, I am not sure, that was existing code.
>
> Are you sure data_buf it is DMA capable?
>
> nand_scan_tail allocates with kmalloc:
>
> chip->data_buf = kmalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL);
Also, you might want to set the NAND_USE_BOUNCE_BUFFER flag in
chip->options, so that the core always pass DMA-able buffers to your
->read/write_page[_raw] function.
[1]https://elixir.bootlin.com/linux/v4.17-rc6/source/include/linux/mtd/rawnand.h#L202
Re: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver
On Thu, 24 May 2018 13:09:53 +0200
Stefan Agner wrote:
> On 24.05.2018 10:56, Boris Brezillon wrote:
> > On Thu, 24 May 2018 10:46:27 +0200
> > Stefan Agner wrote:
> >
> >> Hi Boris,
> >>
> >> Thanks for the initial review! One small question below:
> >>
> >> On 23.05.2018 16:18, Boris Brezillon wrote:
> >> > Hi Stefan,
> >> >
> >> > On Tue, 22 May 2018 14:07:06 +0200
> >> > Stefan Agner wrote:
> >> >> +
> >> >> +struct tegra_nand {
> >> >> + void __iomem *regs;
> >> >> + struct clk *clk;
> >> >> + struct gpio_desc *wp_gpio;
> >> >> +
> >> >> + struct nand_chip chip;
> >> >> + struct device *dev;
> >> >> +
> >> >> + struct completion command_complete;
> >> >> + struct completion dma_complete;
> >> >> + bool last_read_error;
> >> >> +
> >> >> + dma_addr_t data_dma;
> >> >> + void *data_buf;
> >> >> + dma_addr_t oob_dma;
> >> >> + void *oob_buf;
> >> >> +
> >> >> + int cur_chip;
> >> >> +};
> >> >
> >> > This struct should be split in 2 structures: one representing the NAND
> >> > controller and one representing the NAND chip:
> >> >
> >> > struct tegra_nand_controller {
> >> > struct nand_hw_control base;
> >> > void __iomem *regs;
> >> > struct clk *clk;
> >> > struct device *dev;
> >> > struct completion command_complete;
> >> > struct completion dma_complete;
> >> > bool last_read_error;
> >> > int cur_chip;
> >> > };
> >> >
> >> > struct tegra_nand {
> >> > struct nand_chip base;
> >> > dma_addr_t data_dma;
> >> > void *data_buf;
> >> > dma_addr_t oob_dma;
> >> > void *oob_buf;
> >> > };
> >>
> >> Is there a particular reason why you would leave DMA buffers in the chip
> >> structure? It seems that is more a controller thing...
> >
> > The size of those buffers is likely to be device dependent, so if you
> > have several NANDs connected to the controller, you'll either have to
> > have one buffer at the controller level which is max(all-chip-buf-size)
> > or a buffer per device.
> >
> > Also, do you really need these buffers? The core already provide some
> > which are suitable for DMA (chip->oob_poi and chip->data_buf).
> >
>
> Good question, I am not sure, that was existing code.
>
> Are you sure data_buf it is DMA capable?
>
> nand_scan_tail allocates with kmalloc:
>
> chip->data_buf = kmalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL);
Yes, kmalloc() allocates DMA-able buffers, so those are DMA-safe.
Re: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver
Hi Benjamin, On Thu, 24 May 2018 13:30:14 +0200 Benjamin Lindqvist wrote: > Hi Stefan, > > It seems to me that a probe similar to what the BootROM does shouldn't > be awfully complicated to implement - just cycle through the switch > cases in case of an ECC error. But I guess that's more of an idea for > further improvements rather than a comment to the patch set under > review. Nope, not really an option, because you're not guaranteed that the NAND will be used as a boot media, and the first page or first set of pages might just be erased. > > However, I think that allowing for an override of the oobsize > inference would be a good idea before merging, no? This could just be > a trivial #ifdef (at least temporarily). If you agree but don't feel > like doing it yourself, I'd be happy to pitch in. Let me know. That's why we have nand-ecc-xxx properties in the DT. Regards, Boris
Re: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver
On Thu, 24 May 2018 13:00:41 +0200 Stefan Agner wrote: > > > > > Note that we're in fact using this patch set in Linux today, but > > we had to remove the oobsize inference part. Currently we're > > simply hard coding it to CFG_TVAL_4, but maybe it would be > > cleaner to add ECC algo as a board config instead, e.g. in the > > .dts file or whatever. This seems to be done by other vendors > > already, see for example excerpt of > > Documentation/devicetree/bindings/mtd/gpmc-nand.txt below: > > > > - ti,nand-ecc-opt: A string setting the ECC layout to use. One of: > > "sw" 1-bit Hamming ecc code via software > > "hw" use "ham1" instead > > "hw-romcode" use "ham1" instead > > "ham1" 1-bit Hamming ecc code > > "bch4" 4-bit BCH ecc code > > "bch8" 8-bit BCH ecc code > > "bch16" 16-bit BCH ECC code > > Refer below "How to select correct ECC scheme for your device ?" > > > > It seems as if this method would be equally applicable to Tegra NAND. > > Yeah, ideally we can reuse "nand-ecc-algo". Although, Reed-Solomon is > not yet in the list. So using this property would require to extend > this standard property. I'm okay with that ;-).
Re: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver
On Thu, 24 May 2018 10:46:27 +0200
Stefan Agner wrote:
> Hi Boris,
>
> Thanks for the initial review! One small question below:
>
> On 23.05.2018 16:18, Boris Brezillon wrote:
> > Hi Stefan,
> >
> > On Tue, 22 May 2018 14:07:06 +0200
> > Stefan Agner wrote:
> >> +
> >> +struct tegra_nand {
> >> + void __iomem *regs;
> >> + struct clk *clk;
> >> + struct gpio_desc *wp_gpio;
> >> +
> >> + struct nand_chip chip;
> >> + struct device *dev;
> >> +
> >> + struct completion command_complete;
> >> + struct completion dma_complete;
> >> + bool last_read_error;
> >> +
> >> + dma_addr_t data_dma;
> >> + void *data_buf;
> >> + dma_addr_t oob_dma;
> >> + void *oob_buf;
> >> +
> >> + int cur_chip;
> >> +};
> >
> > This struct should be split in 2 structures: one representing the NAND
> > controller and one representing the NAND chip:
> >
> > struct tegra_nand_controller {
> > struct nand_hw_control base;
> > void __iomem *regs;
> > struct clk *clk;
> > struct device *dev;
> > struct completion command_complete;
> > struct completion dma_complete;
> > bool last_read_error;
> > int cur_chip;
> > };
> >
> > struct tegra_nand {
> > struct nand_chip base;
> > dma_addr_t data_dma;
> > void *data_buf;
> > dma_addr_t oob_dma;
> > void *oob_buf;
> > };
>
> Is there a particular reason why you would leave DMA buffers in the chip
> structure? It seems that is more a controller thing...
The size of those buffers is likely to be device dependent, so if you
have several NANDs connected to the controller, you'll either have to
have one buffer at the controller level which is max(all-chip-buf-size)
or a buffer per device.
Also, do you really need these buffers? The core already provide some
which are suitable for DMA (chip->oob_poi and chip->data_buf).
>
> If I move them, then struct tegra_nand would be basically empty. Can I
> just use struct nand_chip and have no driver specific chip abstraction?
Sure.
Re: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver
Hi Stefan,
On Tue, 22 May 2018 14:07:06 +0200
Stefan Agner wrote:
> +
> +struct tegra_nand {
> + void __iomem *regs;
> + struct clk *clk;
> + struct gpio_desc *wp_gpio;
> +
> + struct nand_chip chip;
> + struct device *dev;
> +
> + struct completion command_complete;
> + struct completion dma_complete;
> + bool last_read_error;
> +
> + dma_addr_t data_dma;
> + void *data_buf;
> + dma_addr_t oob_dma;
> + void *oob_buf;
> +
> + int cur_chip;
> +};
This struct should be split in 2 structures: one representing the NAND
controller and one representing the NAND chip:
struct tegra_nand_controller {
struct nand_hw_control base;
void __iomem *regs;
struct clk *clk;
struct device *dev;
struct completion command_complete;
struct completion dma_complete;
bool last_read_error;
int cur_chip;
};
struct tegra_nand {
struct nand_chip base;
dma_addr_t data_dma;
void *data_buf;
dma_addr_t oob_dma;
void *oob_buf;
};
> +
> +static inline struct tegra_nand *to_tegra_nand(struct mtd_info *mtd)
> +{
> + struct nand_chip *chip = mtd_to_nand(mtd);
> +
> + return nand_get_controller_data(chip);
then you can just do:
return container_of(chip, struct tegra_nand, base);
> +}
> +
> +static int tegra_nand_ooblayout_16_ecc(struct mtd_info *mtd, int section,
> +struct mtd_oob_region *oobregion)
> +{
> + if (section > 0)
> + return -ERANGE;
> +
> + oobregion->offset = 4;
> + oobregion->length = 4;
> +
> + return 0;
> +}
> +
> +static int tegra_nand_ooblayout_16_free(struct mtd_info *mtd, int section,
> + struct mtd_oob_region *oobregion)
> +{
> + if (section > 0)
> + return -ERANGE;
> +
> + oobregion->offset = 8;
> + oobregion->length = 8;
> +
> + return 0;
> +}
...
> +
> +static int tegra_nand_ooblayout_224_ecc(struct mtd_info *mtd, int section,
> +struct mtd_oob_region *oobregion)
> +{
> + if (section > 0)
> + return -ERANGE;
> +
> + oobregion->offset = 4;
> + oobregion->length = 144;
> +
> + return 0;
> +}
> +
> +static int tegra_nand_ooblayout_224_free(struct mtd_info *mtd, int section,
> + struct mtd_oob_region *oobregion)
> +{
> + if (section > 0)
> + return -ERANGE;
> +
> + oobregion->offset = 148;
> + oobregion->length = 76;
> +
> + return 0;
> +}
> +
> +static const struct mtd_ooblayout_ops tegra_nand_oob_224_ops = {
> + .ecc = tegra_nand_ooblayout_224_ecc,
> + .free = tegra_nand_ooblayout_224_free,
> +};
> +
I'm pretty sure we can find a pattern here to avoid defining a new
mtd_ooblayout_ops for each OOB size.
> +static irqreturn_t tegra_nand_irq(int irq, void *data)
> +{
> + struct tegra_nand *nand = data;
> + u32 isr, dma;
> +
> + isr = readl(nand->regs + ISR);
> + dma = readl(nand->regs + DMA_CTRL);
> + dev_dbg(nand->dev, "isr %08x\n", isr);
> +
> + if (!isr && !(dma & DMA_CTRL_IS_DONE))
> + return IRQ_NONE;
> +
> + if (isr & ISR_CORRFAIL_ERR)
> + nand->last_read_error = true;
> +
> + if (isr & ISR_CMD_DONE)
> + complete(&nand->command_complete);
> +
> + if (isr & ISR_UND)
> + dev_dbg(nand->dev, "FIFO underrun\n");
> +
> + if (isr & ISR_OVR)
> + dev_dbg(nand->dev, "FIFO overrun\n");
> +
> + /* handle DMA interrupts */
> + if (dma & DMA_CTRL_IS_DONE) {
> + writel(dma, nand->regs + DMA_CTRL);
> + complete(&nand->dma_complete);
> + }
> +
> + /* clear interrupts */
> + writel(isr, nand->regs + ISR);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int tegra_nand_cmd(struct nand_chip *chip,
> + const struct nand_subop *subop)
> +{
> + const struct nand_op_instr *instr;
> + const struct nand_op_instr *instr_data_in = NULL;
> + struct mtd_info *mtd = nand_to_mtd(chip);
> + struct tegra_nand *nand = to_tegra_nand(mtd);
> + unsigned int op_id = -1, trfr_in_sz = 0, trfr_out_sz = 0, offset = 0;
> + bool first_cmd = true;
> + bool force8bit;
> + u32 cmd = 0;
> + u32 value;
> +
> + for (op_id = 0; op_id < subop->ninstrs; op_id++) {
> + unsigned int naddrs, i;
> + const u8 *addrs;
> + u32 addr1 = 0, addr2 = 0;
> +
> + instr = &subop->instrs[op_id];
> +
> + switch (instr->type) {
> + case NAND_OP_CMD_INSTR:
> + if (first_cmd) {
> + cmd |= CMD_CLE;
> + writel(instr->ctx.cmd.opcode, nand->regs +
> CMD_1);
> + } else {
> + cmd |= CMD_SEC_CMD;
> + writel(instr->ctx.cmd.opcode, nand-
Re: [PATCH] mtd: cmdlinepart: Update comment for introduction of OFFSET_CONTINUOUS
On Tue, 22 May 2018 17:07:53 +0200
Geert Uytterhoeven wrote:
> The comment about offset zero was not updated when changing behavior:
> - Automatic offset calculation is indicated by OFFSET_CONTINUOUS,
> - Zero really means offset zero.
>
> Fixes: b175d03dd2072836 ("[PATCH] mtd cmdlinepart: allow zero offset value")
> Signed-off-by: Geert Uytterhoeven
Applied.
Thanks,
Boris
> ---
> drivers/mtd/cmdlinepart.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
> index fbd5affc0acfe3fe..3ea44cff9b759e3c 100644
> --- a/drivers/mtd/cmdlinepart.c
> +++ b/drivers/mtd/cmdlinepart.c
> @@ -190,7 +190,10 @@ static struct mtd_partition * newpart(char *s,
> extra_mem = (unsigned char *)(parts + *num_parts);
> }
>
> - /* enter this partition (offset will be calculated later if it is zero
> at this point) */
> + /*
> + * enter this partition (offset will be calculated later if it is
> + * OFFSET_CONTINUOUS at this point)
> + */
> parts[this_part].size = size;
> parts[this_part].offset = offset;
> parts[this_part].mask_flags = mask_flags;
Re: [PATCH] mtd: nand: raw: atmel: add module param to avoid using dma
On Tue, 22 May 2018 20:03:41 +0200 Peter Rosin wrote: > On 2018-04-11 17:34, Nicolas Ferre wrote: > > On 11/04/2018 at 16:44, Peter Rosin wrote: > >> Hi Nicolas, > >> > >> Boris asked for your input on this (the datasheet difference appears to > >> have no bearing on the issue) elsewhere in the tree of messages. It's > >> now been a week or so and I'm starting to wonder if you missed this > >> altogether or if you are simply out of office or something? > > > > Hi Peter, > > > > I didn't dig into this issue with matrix datasheet reset values and your > > findings below. I'll try to move forward with your detailed explanation > > and with my contacts within the "product" team internally. > > Hi again, just checking in to see if there is any progress? If not, maybe > it's time to just take the patch as-is, warts and imperfections included, > and even if we all hate it. But what to do... Well, it's not really that the fix is ugly, but I'm pretty sure it's actually not fixing the problem, just make it less likely to happen. And, as soon as you'll have more traffic going through the DRAM controller you'll experience the same problem again.
Re: [PATCH] mtd: cmdlinepart: Update comment for introduction of OFFSET_CONTINUOUS
On Tue, 22 May 2018 17:07:53 +0200
Geert Uytterhoeven wrote:
> The comment about offset zero was not updated when changing behavior:
> - Automatic offset calculation is indicated by OFFSET_CONTINUOUS,
> - Zero really means offset zero.
>
> Fixes: b175d03dd2072836 ("[PATCH] mtd cmdlinepart: allow zero offset value")
Did we switch to 16bytes for the short commit ids (I usually use 12
bytes)?
> Signed-off-by: Geert Uytterhoeven
> ---
> drivers/mtd/cmdlinepart.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
> index fbd5affc0acfe3fe..3ea44cff9b759e3c 100644
> --- a/drivers/mtd/cmdlinepart.c
> +++ b/drivers/mtd/cmdlinepart.c
> @@ -190,7 +190,10 @@ static struct mtd_partition * newpart(char *s,
> extra_mem = (unsigned char *)(parts + *num_parts);
> }
>
> - /* enter this partition (offset will be calculated later if it is zero
> at this point) */
> + /*
> + * enter this partition (offset will be calculated later if it is
> + * OFFSET_CONTINUOUS at this point)
> + */
> parts[this_part].size = size;
> parts[this_part].offset = offset;
> parts[this_part].mask_flags = mask_flags;
Re: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver
Hi Stefan, On Tue, 22 May 2018 16:53:46 +0200 Stefan Agner wrote: > Hi, > > I do have some questions for some areas I wanted to improve in the next > revision. But I would like to make sure that the way I would like to > implement aligns with the MTD subsystem. I won't have time to review the driver this week. Will try to have a look next week. Regards, Boris
Re: [RESEND PATCH 1/5] mtd: rawnand: tegra: add devicetree binding
Hello Stefan,
On Tue, 22 May 2018 14:07:05 +0200
Stefan Agner wrote:
> From: Lucas Stach
>
> This adds the devicetree binding for the Tegra 2 NAND flash
> controller.
>
> Signed-off-by: Lucas Stach
> Signed-off-by: Stefan Agner
> ---
> .../bindings/mtd/nvidia,tegra20-nand.txt | 29 +++
> 1 file changed, 29 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/mtd/nvidia,tegra20-nand.txt
>
> diff --git a/Documentation/devicetree/bindings/mtd/nvidia,tegra20-nand.txt
> b/Documentation/devicetree/bindings/mtd/nvidia,tegra20-nand.txt
> new file mode 100644
> index ..522d442937a9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/nvidia,tegra20-nand.txt
> @@ -0,0 +1,29 @@
> +NVIDIA Tegra NAND Flash controller
> +
> +Required properties:
> +- compatible: Must be one of:
> + - "nvidia,tegra20-nand"
> +- reg: MMIO address range
> +- interrupts: interrupt output of the NFC controller
> +- clocks: Must contain an entry for each entry in clock-names.
> + See ../clocks/clock-bindings.txt for details.
> +- clock-names: Must include the following entries:
> + - nand
> +- resets: Must contain an entry for each entry in reset-names.
> + See ../reset/reset.txt for details.
> +- reset-names: Must include the following entries:
> + - nand
> +
> +Optional properties:
> +- nvidia,wp-gpios: GPIO used to disable write protection of the flash
> +
> + Example:
> + nand@70008000 {
> + compatible = "nvidia,tegra20-nand";
> + reg = <0x70008000 0x100>;
> + interrupts = ;
> + clocks = <&tegra_car TEGRA20_CLK_NDFLASH>;
> + clock-names = "nand";
> + resets = <&tegra_car 13>;
> + reset-names = "nand";
> + };
Can you represent the controller and NAND chip separately as
recommended here [1].
Thanks,
Boris
[1]https://elixir.bootlin.com/linux/v4.17-rc6/source/Documentation/devicetree/bindings/mtd/nand.txt#L56
Re: [PATCH 043/102] mtd: spi-nor: stm32-quadspi: explicitly request exclusive reset control
On Wed, 19 Jul 2017 17:25:47 +0200
Philipp Zabel wrote:
> Commit a53e35db70d1 ("reset: Ensure drivers are explicit when requesting
> reset lines") started to transition the reset control request API calls
> to explicitly state whether the driver needs exclusive or shared reset
> control behavior. Convert all drivers requesting exclusive resets to the
> explicit API call so the temporary transition helpers can be removed.
>
> No functional changes.
>
> Cc: Cyrille Pitchen
> Cc: Marek Vasut
> Cc: David Woodhouse
> Cc: Brian Norris
> Cc: Boris Brezillon
> Cc: Richard Weinberger
> Cc: Maxime Coquelin
> Cc: Alexandre Torgue
> Cc: linux-...@lists.infradead.org
> Signed-off-by: Philipp Zabel
Queued to spi-nor/next.
Thanks,
Boris
> ---
> drivers/mtd/spi-nor/stm32-quadspi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/stm32-quadspi.c
> b/drivers/mtd/spi-nor/stm32-quadspi.c
> index 86c0931543c53..a367c56deb3cc 100644
> --- a/drivers/mtd/spi-nor/stm32-quadspi.c
> +++ b/drivers/mtd/spi-nor/stm32-quadspi.c
> @@ -633,7 +633,7 @@ static int stm32_qspi_probe(struct platform_device *pdev)
> return ret;
> }
>
> - rstc = devm_reset_control_get(dev, NULL);
> + rstc = devm_reset_control_get_exclusive(dev, NULL);
> if (!IS_ERR(rstc)) {
> reset_control_assert(rstc);
> udelay(2);
--
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [PATCH V2 RFC] mtd: spi-nor: intel: provide a range for poll_timout
On Tue, 14 Feb 2017 11:47:24 +0200 Mika Westerberg wrote: > On Mon, Feb 13, 2017 at 09:13:42AM +0100, Nicholas Mc Guire wrote: > > The overall poll time here is INTEL_SPI_TIMEOUT * 1000 which is > > 5000 * 1000 - so 5seconds and it is coded as a tight loop here delay_us > > to readl_poll_timeout() is set to 0. As this is never called in an atomic > > context sleeping should be no issue and there is no reasons for the > > tight-loop here. > > > > Signed-off-by: Nicholas Mc Guire > > Acked-by: Mika Westerberg Queued to spi-nor/next. Thanks, Boris > > __ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ -- Boris Brezillon, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
Re: [PATCH v8 2/3] mtd: spi-nor: add rockchip serial flash controller driver
Hi Andy, Sorry for the late reply. On Thu, 8 Feb 2018 20:18:47 +0800 Andy Yan wrote: > From: Shawn Lin Commit message please. > > Add Rockchip SFC(serial flash controller) driver. > > Signed-off-by: Shawn Lin > Signed-off-by: Andy Yan > Acked-by: Marek Vasut > > --- > > Changes in v8: > - remove unused macro SFC_CMD_TRAN_BYTES_MASK > - set max transfer length to 15.5KB > - remove unnecessary buffer align check > - remove the duplicate logic what spi-nor.c already does for spi_nor_write > - add spi_nor_erase, as the SFC should get the erase address. Would you mind sending a new version addressing the problem reported by kbuild robots and the comments made by Ezequiel and Robin? Also, maybe it's too much work, but it would be good to check if the driver could use the spi_mem interface [1] so that you can move it to drivers/spi/ and possibly get everything ready for SPI NANDs. Thanks, Boris [1]https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git/tree/include/linux/spi/spi-mem.h?h=for-4.18
Re: Some questions about the spi mem framework
On Fri, 18 May 2018 13:50:00 +0800 Xiangsheng Hou wrote: > Hi Boris, > > On Thu, 2018-05-17 at 09:42 +0200, Boris Brezillon wrote: > > On Thu, 17 May 2018 15:35:04 +0800 > > Xiangsheng Hou wrote: > > > > > On Thu, 2018-05-17 at 09:13 +0200, Boris Brezillon wrote: > > > > On Thu, 17 May 2018 14:58:24 +0800 > > > > Xiangsheng Hou wrote: > > > > > > > > > Hi Boris, > > > > > > > > > > On Wed, 2018-05-16 at 14:42 +0200, Boris Brezillon wrote: > > > > > > On Wed, 16 May 2018 20:11:39 +0800 > > > > > > Xiangsheng Hou wrote: > > > > > > > > > > > > > Hi Boris, > > > > > > > > > > > > > > On Tue, 2018-05-15 at 17:25 +0200, Boris Brezillon wrote: > > > > > > > > Hi, > > > > > > > > > > > > > > > > On Tue, 15 May 2018 11:43:20 +0800 > > > > > > > > Xiangsheng Hou wrote: > > > > > > > > > > > > > > > > > Hello Boris, > > > > > > > > > > > > > > > > > > I have seen you are working on extend the framework to > > > > > > > > > generically > > > > > > > > > support spi memory devices. > > > > > > > > > And, I am working on upstream SPI Nand driver of Mediatek SPI > > > > > > > > > NAND > > > > > > > > > controller based on your branch[1]. > > > > > > > > > > > > > > > > Great! > > > > > > > > > > > > > > > > > I have some questions need your comment. > > > > > > > > > > > > > > > > > > 1) There is a difference between different SPI NAND Flash > > > > > > > > > when using the > > > > > > > > > Quad SPI command,for example Macronix,Etron and GigaDevice, > > > > > > > > > Quad SPI commands require the Quad Enable bit in Status > > > > > > > > > Register(B0H) to > > > > > > > > > be set. > > > > > > > > > However, current spi-mem framework does not have this > > > > > > > > > operation, > > > > > > > > > do you have a plan to support it? > > > > > > > > > > > > > > > > I added support for the QE bit in the v7 I sent just a few > > > > > > > > minutes ago > > > > > > > > [1]. > > > > > > > > > > > > > > Ok,I have studied v7. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2) I see that current spi-mem framework doesn't support ECC, > > > > > > > > > But we need ECC, and we use Mediatek controller's HW ECC > > > > > > > > > instead of spi nand on-chip ECC, > > > > > > > > > maybe other companies also have this behavior, > > > > > > > > > So the ECC part must be implemented in controller's driver. > > > > > > > > > Will you abstract ECC interface in future? > > > > > > > > > > > > > > > > Well, I added support for on-die ECC in my v7 since all chips > > > > > > > > seem to > > > > > > > > provide this feature. I was initially planning on abstracting > > > > > > > > ECC > > > > > > > > engines, but I decided to go for a simpler approach and only > > > > > > > > support > > > > > > > > on-die ECC. That does not mean we shouldn't work on this "ECC > > > > > > > > engine > > > > > > > > abstraction", just that I wanted to get something out and > > > > > > > > didn't have > > > > > > > > time to spend on this topic. > > > > > > > > > > > > > > > > I'd be happy if someone else could work on that aspect though. > > > > > > > > BTW, do > > > > > > > > you plan to use this engine [2], or is thi
Re: Some questions about the spi mem framework
On Thu, 17 May 2018 15:35:04 +0800 Xiangsheng Hou wrote: > On Thu, 2018-05-17 at 09:13 +0200, Boris Brezillon wrote: > > On Thu, 17 May 2018 14:58:24 +0800 > > Xiangsheng Hou wrote: > > > > > Hi Boris, > > > > > > On Wed, 2018-05-16 at 14:42 +0200, Boris Brezillon wrote: > > > > On Wed, 16 May 2018 20:11:39 +0800 > > > > Xiangsheng Hou wrote: > > > > > > > > > Hi Boris, > > > > > > > > > > On Tue, 2018-05-15 at 17:25 +0200, Boris Brezillon wrote: > > > > > > Hi, > > > > > > > > > > > > On Tue, 15 May 2018 11:43:20 +0800 > > > > > > Xiangsheng Hou wrote: > > > > > > > > > > > > > Hello Boris, > > > > > > > > > > > > > > I have seen you are working on extend the framework to generically > > > > > > > support spi memory devices. > > > > > > > And, I am working on upstream SPI Nand driver of Mediatek SPI NAND > > > > > > > controller based on your branch[1]. > > > > > > > > > > > > Great! > > > > > > > > > > > > > I have some questions need your comment. > > > > > > > > > > > > > > 1) There is a difference between different SPI NAND Flash when > > > > > > > using the > > > > > > > Quad SPI command,for example Macronix,Etron and GigaDevice, > > > > > > > Quad SPI commands require the Quad Enable bit in Status > > > > > > > Register(B0H) to > > > > > > > be set. > > > > > > > However, current spi-mem framework does not have this operation, > > > > > > > do you have a plan to support it? > > > > > > > > > > > > I added support for the QE bit in the v7 I sent just a few minutes > > > > > > ago > > > > > > [1]. > > > > > > > > > > Ok,I have studied v7. > > > > > > > > > > > > > > > > > > > > > > > > > 2) I see that current spi-mem framework doesn't support ECC, > > > > > > > But we need ECC, and we use Mediatek controller's HW ECC > > > > > > > instead of spi nand on-chip ECC, > > > > > > > maybe other companies also have this behavior, > > > > > > > So the ECC part must be implemented in controller's driver. > > > > > > > Will you abstract ECC interface in future? > > > > > > > > > > > > Well, I added support for on-die ECC in my v7 since all chips seem > > > > > > to > > > > > > provide this feature. I was initially planning on abstracting ECC > > > > > > engines, but I decided to go for a simpler approach and only support > > > > > > on-die ECC. That does not mean we shouldn't work on this "ECC engine > > > > > > abstraction", just that I wanted to get something out and didn't > > > > > > have > > > > > > time to spend on this topic. > > > > > > > > > > > > I'd be happy if someone else could work on that aspect though. BTW, > > > > > > do > > > > > > you plan to use this engine [2], or is this yet another ECC engine? > > > > > > > > > > > > > > > > Yes,I plan to use this ecc engine[2]. > > > > > > > > Cool. That probably means we'll have to move the driver one level up > > > > (in drivers/mtd/nand) and work on this ECC engine interface I was > > > > talking about. > > > > > > > > > > > 3) You know, some nand controller need configure their registers > > > > > > > when > > > > > > > getting some information(page size, spare size) of nand flash, > > > > > > > But the spi-mem framework doesn't has an interface for scanning > > > > > > > NAND > > > > > > > flash, when controller driver initialization. > > > > > > > > > > > > You seem to mix 2 different things: > > > > > > - spi-mem: this is generic interface provided by the SPI framework > > > > > > to > > > > > > send spi_mem_op. There's nothing NOR or NAND specific in there, > > > > > > and > > > > > > I'd like it to stay like that as much as possible > > > > > > - spinand: this the spi-mem driver that is dealing with SPI NAND > > > > > > devices, and this is where all the code related to SPI NAND > > > > > > support > > > > > > should end up. > > > > > > > > > > > > Can you tell me exactly why your SPI controller needs such a > > > > > > detailed > > > > > > description? Is it able to program/read pages or erase blocks on its > > > > > > own? Do you have a spec of this controller publicly available? > > > > > > > > > > For Mediatek SPI Nand controller,I have to configure registers for ECC > > > > > engine,page format and spare format according to nand information just > > > > > like[3] in mtk_nfc_hw_runtime_config() function. > > > > > > > > So it's all related to the NAND controller, nothing specific to the SPI > > > > controller, right? > > > > > > Yes,we use NAND controller rather than SPI controller. > > > > Sorry, I meant ECC engine, not NAND controller. > > It's related to ECC engine and NAND controller. Not sure I understand what you call NAND controller. Is it a SPI NAND controller or the raw NAND controller available in drivers/mtd/nand/raw? And if it's a SPI NAND controller, does that mean MTK implements a NAND dedicated logic on top of its SPI controller?
Re: [PATCH v7] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter
On Wed, 16 May 2018 09:42:27 +0200
Miquel Raynal wrote:
> Hi Chris,
>
> > >>> +static void nand_bit_wise_majority(const void **srcbufs,
> > >>> + unsigned int nsrcbufs,
> > >>> + void *dstbuf,
> > >>> + unsigned int bufsize)
> > >>> +{
> > >>> + int i, j, k;
> > >>> +
> > >>> + for (i = 0; i < bufsize; i++) {
> > >>> + u8 cnt, val;
> > >>> +
> > >>> + val = 0;
> > >>> + for (j = 0; j < 8; j++) {
> > >>> + cnt = 0;
> > >>> + for (k = 0; k < nsrcbufs; k++) {
> > >>> + const u8 *srcbuf = srcbufs[k];
> > >>> +
> > >>> + if (srcbuf[i] & BIT(j))
> > >>> + cnt++;
> > >>> + }
> > >>> + if (cnt > nsrcbufs / 2)
> > >>> + val |= BIT(j);
> > >>> + }
> > >>> + ((u8 *)dstbuf)[i] = val;
> > >>> + }
> > >>> +}
> > >>> +
> > >>> +/*
> > >>> * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0
> > >>> otherwise.
> > >>> */
> > >>>static int nand_flash_detect_onfi(struct nand_chip *chip)
> > >>> @@ -5102,7 +5131,7 @@ static int nand_flash_detect_onfi(struct
> > >>> nand_chip *chip)
> > >>> return 0;
> > >>>>>> /* ONFI chip: allocate a buffer to hold its parameter
> > >>> page */
> > >>> - p = kzalloc(sizeof(*p), GFP_KERNEL);
> > >>> + p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> > >>> if (!p)
> > >>> return -ENOMEM;
> > >>>>>> @@ -5113,21 +5142,32 @@ static int nand_flash_detect_onfi(struct
> > >>> nand_chip *chip)
> > >>> }
> > >>>>>> for (i = 0; i < 3; i++) {
> > >>> - ret = nand_read_data_op(chip, p, sizeof(*p), true);
> > >>> + ret = nand_read_data_op(chip, &p[i], sizeof(*p), true);
> > >>> if (ret) {
> > >>> ret = 0;
> > >>> goto free_onfi_param_page;
> > >>> }
> > >>>>>> -if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p,
> > >>> 254) ==
> > >>> + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) ==
> > >>> le16_to_cpu(p->crc)) {
> > >>> + if (i)
> > >>> + memcpy(p, &p[i], sizeof(*p));
> > >>> break;
> > >>> }
> > >>> }
> > >>>>>> if (i == 3) {
> > >>> - pr_err("Could not find valid ONFI parameter page;
> > >>> aborting\n");
> > >>> - goto free_onfi_param_page;
> > >>> + const void *srcbufs[3] = {p, p + 1, p + 2};
> > >>> +
> > >>> + pr_warn("Could not find a valid ONFI parameter page,
> > >>> trying bit-wise majority to recover it\n");
> > >>> + nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
> > >>> + sizeof(*p));
> > >>> +
> > >>> + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) !=
> > >>> + le16_to_cpu(p->crc)) {
> > >>> + pr_err("ONFI parameter recovery failed,
> > >>> aborting\n");
> > >>> + goto free_onfi_param_page;
> > >>> + }
> > >>> }
> > >>>>>> /* Check version */
> > >> This version is still hard coded for a three sample bitwise majority
> > >> vote.
> > >> So why not use the method which I suggested previously for v2 and which
> > >> I repeat below?
> > > Because I want the nand_bit_wise_majority() function to work with
> > > nsrcbufs > 3 (the ONFI spec says there's at least 3 copy of the param
> > > page, but NAND vendor can decide to put more). Also, if the X copies of
> > > the PARAM are corrupted (which is rather unlikely), that means we
> > > already spent quite a lot of time reading the different copies and
> > > calculating the CRC, so I think we don't care about perf optimizations
> > > when doing bit-wise majority.
> > >
> > >> The three sample bitwise majority can be implemented without bit level
> > >> manipulation using the identity:
> > >> majority3(a, b, c) = (a & b) | (a & c) | (b & c)
> > >> This can be factorized slightly to (a & (b | c)) | (b & c)
> > >> This enables the operation to be performed 8, 16, 32 or even 64 bits at
> > >> a time depending on the hardware.
> > >>
> > >> This method is not only faster and but also more compact.
> > >>
> >
> > I do understand that the ONFI specifications permit more than 3 copies.
> > However elsewhere the proposed code shows no intention of handling other
> > cases.
> > The constant 3 is hard coded in the following lines extracted from the
> > proposed code:
> >
Re: [PATCH v7] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter
On Wed, 16 May 2018 09:32:57 +0200
Chris Moore wrote:
> Hi,
>
> Le 15/05/2018 à 09:34, Boris Brezillon a écrit :
> > On Tue, 15 May 2018 06:45:51 +0200
> > Chris Moore wrote:
> >
> >> Hi,
> >>
> >> Le 13/05/2018 à 06:30, Wan, Jane (Nokia - US/Sunnyvale) a écrit :
> >>> Per ONFI specification (Rev. 4.0), if all parameter pages have invalid
> >>> CRC values, the bit-wise majority may be used to recover the contents of
> >>> the parameter pages from the parameter page copies present.
> >>>
> >>> Signed-off-by: Jane Wan
> >>> ---
> >>> v7: change debug print messages
> >>> v6: support the cases that srcbufs are not contiguous
> >>> v5: make the bit-wise majority functon generic
> >>> v4: move the bit-wise majority code in a separate function
> >>> v3: fix warning message detected by kbuild test robot
> >>> v2: rebase the changes on top of v4.17-rc1
> >>>
> >>> drivers/mtd/nand/raw/nand_base.c | 50
> >>> ++
> >>>1 file changed, 45 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/mtd/nand/raw/nand_base.c
> >>> b/drivers/mtd/nand/raw/nand_base.c
> >>> index 72f3a89..b43b784 100644
> >>> --- a/drivers/mtd/nand/raw/nand_base.c
> >>> +++ b/drivers/mtd/nand/raw/nand_base.c
> >>> @@ -5087,6 +5087,35 @@ static int nand_flash_detect_ext_param_page(struct
> >>> nand_chip *chip,
> >>>}
> >>>
> >>>/*
> >>> + * Recover data with bit-wise majority
> >>> + */
> >>> +static void nand_bit_wise_majority(const void **srcbufs,
> >>> +unsigned int nsrcbufs,
> >>> +void *dstbuf,
> >>> +unsigned int bufsize)
> >>> +{
> >>> + int i, j, k;
> >>> +
> >>> + for (i = 0; i < bufsize; i++) {
> >>> + u8 cnt, val;
> >>> +
> >>> + val = 0;
> >>> + for (j = 0; j < 8; j++) {
> >>> + cnt = 0;
> >>> + for (k = 0; k < nsrcbufs; k++) {
> >>> + const u8 *srcbuf = srcbufs[k];
> >>> +
> >>> + if (srcbuf[i] & BIT(j))
> >>> + cnt++;
> >>> + }
> >>> + if (cnt > nsrcbufs / 2)
> >>> + val |= BIT(j);
> >>> + }
> >>> + ((u8 *)dstbuf)[i] = val;
> >>> + }
> >>> +}
> >>> +
> >>> +/*
> >>> * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0
> >>> otherwise.
> >>> */
> >>>static int nand_flash_detect_onfi(struct nand_chip *chip)
> >>> @@ -5102,7 +5131,7 @@ static int nand_flash_detect_onfi(struct nand_chip
> >>> *chip)
> >>> return 0;
> >>>
> >>> /* ONFI chip: allocate a buffer to hold its parameter page */
> >>> - p = kzalloc(sizeof(*p), GFP_KERNEL);
> >>> + p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> >>> if (!p)
> >>> return -ENOMEM;
> >>>
> >>> @@ -5113,21 +5142,32 @@ static int nand_flash_detect_onfi(struct
> >>> nand_chip *chip)
> >>> }
> >>>
> >>> for (i = 0; i < 3; i++) {
> >>> - ret = nand_read_data_op(chip, p, sizeof(*p), true);
> >>> + ret = nand_read_data_op(chip, &p[i], sizeof(*p), true);
> >>> if (ret) {
> >>> ret = 0;
> >>> goto free_onfi_param_page;
> >>> }
> >>>
> >>> - if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> >>> + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) ==
> >>> le16_to_cpu(p->crc)) {
> >>> + if (i)
> >>> + memcpy(p, &p[i], sizeof(*p));
> >>> break;
> >>> }
> >>> }
> >>>
> >>> if (i == 3) {
> >>>
Re: [PATCH v5 2/2] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter
On Tue, 15 May 2018 23:23:02 +0300
Andy Shevchenko wrote:
> On Tue, May 15, 2018 at 11:03 AM, Boris Brezillon
> wrote:
> > On Tue, 15 May 2018 10:46:00 +0300
> > Andy Shevchenko wrote:
> >
> >> On Tue, May 15, 2018 at 10:35 AM, Boris Brezillon
> >> wrote:
> >> > On Mon, 14 May 2018 20:54:36 +0300
> >> > Andy Shevchenko wrote:
>
> >> >> > for (k = 0; k < nbufs; k++) {
> >> >> > const u8 *srcbuf = srcbufs[j];
> >> >> >
> >> >> > if (srcbuf[i] & BIT(k))
> >> >> > m++;
> >> >> > }
> >> >>
> >> >> ...which is effectively hweightXX().
> >> >
> >> > No it's not.
> >>
> >> I don't see how "not". In the loop everithing except m and k are
> >> invariants. What did I miss?
> >
> > We're not counting the number of bits set in an uXX var, but the number
> > of set bits at the same position in different buffers.
>
> ...on big picture. The excerpt above is hweight() against srcbuf[i].
>
> Let's rewrite it like this:
>
> const u8 *srcbuf = srcbufs[j];
>
> for (k = 0; k < nbufs; k++) {
>if (srcbuf[i] & BIT(k))
I made a mistake in my code sample, it's
if (srcbuf[i] & BIT(j))
If you look at v6, you'll see it's been fixed by Jane.
> m++;
> }
>
> ...and now it looks obvious:
>
> m += hweight...(srcbuf[i])
>
> _If_ nbufs is power of two we may use primitive helper.
>
Re: [PATCH] mtd: nand: Fix return type of __DIVIDE() when called with 32-bit
On Mon, 14 May 2018 14:13:39 +0200
Boris Brezillon wrote:
> On Mon, 14 May 2018 14:00:19 +0200
> Geert Uytterhoeven wrote:
>
> > Hi Boris,
> >
> > On Mon, May 14, 2018 at 1:46 PM, Boris Brezillon
> > wrote:
> > > On Mon, 14 May 2018 13:32:30 +0200
> > > Geert Uytterhoeven wrote:
> > >> On Mon, May 14, 2018 at 1:23 PM, Boris Brezillon
> > >> wrote:
> > >> > On Mon, 14 May 2018 12:49:37 +0200
> > >> > Geert Uytterhoeven wrote:
> > >> >> The __DIVIDE() macro checks whether it is called with a 32-bit or
> > >> >> 64-bit
> > >> >> dividend, to select the appropriate divide-and-round-up routine.
> > >> >> As the check uses the ternary operator, the result will always be
> > >> >> promoted to a type that can hold both results, i.e. unsigned long
> > >> >> long.
> > >> >>
> > >> >> When using this result in a division on a 32-bit system, this may lead
> > >> >> to link errors like:
> > >> >>
> > >> >> ERROR: "__udivdi3" [drivers/mtd/nand/raw/nand.ko] undefined!
> > >> >>
> > >> >> Fix this by casting the result of the 64-bit division to the type of
> > >> >> the
> > >> >> dividend.
> > >> >>
> > >> >> Fixes: 8878b126df769831 ("mtd: nand: add ->exec_op() implementation")
> > >> >> Signed-off-by: Geert Uytterhoeven
> > >> >> ---
> > >> >> This fixes the root cause of the link failure seen with
> > >> >> m68k/allmodconfig since commit 3057fcef385348fe ("mtd: rawnand: Make
> > >> >> sure we wait tWB before polling the STATUS reg").
> > >> >>
> > >> >> An alternative mitigation was posted as "[PATCH] m68k: Implement
> > >> >> ndelay() as an inline function to force type checking/casting"
> > >> >> (https://lkml.org/lkml/2018/5/13/102).
> > >> >> ---
> > >> >> include/linux/mtd/rawnand.h | 2 +-
> > >> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> > >> >>
> > >> >> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> > >> >> index 5dad59b312440a9c..d06dc428ea0102ae 100644
> > >> >> --- a/include/linux/mtd/rawnand.h
> > >> >> +++ b/include/linux/mtd/rawnand.h
> > >> >> @@ -871,7 +871,7 @@ struct nand_op_instr {
> > >> >> #define __DIVIDE(dividend, divisor) ({
> > >> >> \
> > >> >> sizeof(dividend) == sizeof(u32) ?
> > >> >> \
> > >> >> DIV_ROUND_UP(dividend, divisor) :
> > >> >> \
> > >> >> - DIV_ROUND_UP_ULL(dividend, divisor);
> > >> >> \
> > >> >> + (__typeof__(dividend))DIV_ROUND_UP_ULL(dividend,
> > >> >> divisor); \
> > >> >
> > >> > Hm, it's a bit hard to follow when you place the cast here. One could
> > >> > wonder why a cast to (__typeof__(dividend)) is needed since
> > >> > DIV_ROUND_UP_ULL() already returns a (__typeof__(dividend)) type.
> > >>
> > >> DIV_ROUND_UP_ULL() does not return __typeof__(dividend), but
> > >> unsigned long long.
> > >
> > > Except if you entered this branch, that means you passed an unsigned
> > > long long dividend (AKA u64), otherwise you would go in DIV_ROUND_UP().
> > > Am I missing something?
> >
> > Sure, inside that branch, it does.
> > But the compiler considers the whole ternary operator construction, i.e.
> > both branches.
>
> Yes, and that's my point. The (__typeof__(dividend)) when placed like
> you did is ambiguous. It looks like you're doing a useless cast, while
> what you're actually fixing is the case where dividend is an u32 (AKA
> unsigned long), and from the reader PoV, the code you're fixing
> shouldn't even be reached. Hence my suggestion to move the case one
> level up and add a comment ;-).
>
> >
> > >> > How about:
> > >> >
> > >> > /*
> > >> >
Re: [PATCH v5 2/2] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter
On Tue, 15 May 2018 10:46:00 +0300
Andy Shevchenko wrote:
> On Tue, May 15, 2018 at 10:35 AM, Boris Brezillon
> wrote:
> > On Mon, 14 May 2018 20:54:36 +0300
> > Andy Shevchenko wrote:
> >
> >> On Thu, May 10, 2018 at 3:03 PM, Boris Brezillon
> >> wrote:
> >>
> >> >> +#define GET_BIT(bit, val) (((val) >> (bit)) & 0x01)
> >> >
> >> > Not sure we need that macro, see below.
> >>
> >> +1. We have too many nice helpers for bit manipulations
> >> (for_each_set_bit() as an example).
> >>
> >>
> >> > for (k = 0; k < nbufs; k++) {
> >> > const u8 *srcbuf = srcbufs[j];
> >> >
> >> > if (srcbuf[i] & BIT(k))
> >> > m++;
> >> > }
> >>
> >> ...which is effectively hweightXX().
> >
> > No it's not.
>
> I don't see how "not". In the loop everithing except m and k are
> invariants. What did I miss?
We're not counting the number of bits set in an uXX var, but the number
of set bits at the same position in different buffers.
>
> The powerness of two of nbufs is another thing of _existing_
> prototypes of hweightXX().
>
Re: [PATCH v5 2/2] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter
On Mon, 14 May 2018 20:54:36 +0300
Andy Shevchenko wrote:
> On Thu, May 10, 2018 at 3:03 PM, Boris Brezillon
> wrote:
>
> >> +#define GET_BIT(bit, val) (((val) >> (bit)) & 0x01)
> >
> > Not sure we need that macro, see below.
>
> +1. We have too many nice helpers for bit manipulations
> (for_each_set_bit() as an example).
>
>
> > for (k = 0; k < nbufs; k++) {
> > const u8 *srcbuf = srcbufs[j];
> >
> > if (srcbuf[i] & BIT(k))
> > m++;
> > }
>
> ...which is effectively hweightXX().
No it's not.
Re: [PATCH v7] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter
On Tue, 15 May 2018 06:45:51 +0200
Chris Moore wrote:
> Hi,
>
> Le 13/05/2018 à 06:30, Wan, Jane (Nokia - US/Sunnyvale) a écrit :
> > Per ONFI specification (Rev. 4.0), if all parameter pages have invalid CRC
> > values, the bit-wise majority may be used to recover the contents of the
> > parameter pages from the parameter page copies present.
> >
> > Signed-off-by: Jane Wan
> > ---
> > v7: change debug print messages
> > v6: support the cases that srcbufs are not contiguous
> > v5: make the bit-wise majority functon generic
> > v4: move the bit-wise majority code in a separate function
> > v3: fix warning message detected by kbuild test robot
> > v2: rebase the changes on top of v4.17-rc1
> >
> > drivers/mtd/nand/raw/nand_base.c | 50
> > ++
> > 1 file changed, 45 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/nand_base.c
> > b/drivers/mtd/nand/raw/nand_base.c
> > index 72f3a89..b43b784 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -5087,6 +5087,35 @@ static int nand_flash_detect_ext_param_page(struct
> > nand_chip *chip,
> > }
> >
> > /*
> > + * Recover data with bit-wise majority
> > + */
> > +static void nand_bit_wise_majority(const void **srcbufs,
> > + unsigned int nsrcbufs,
> > + void *dstbuf,
> > + unsigned int bufsize)
> > +{
> > + int i, j, k;
> > +
> > + for (i = 0; i < bufsize; i++) {
> > + u8 cnt, val;
> > +
> > + val = 0;
> > + for (j = 0; j < 8; j++) {
> > + cnt = 0;
> > + for (k = 0; k < nsrcbufs; k++) {
> > + const u8 *srcbuf = srcbufs[k];
> > +
> > + if (srcbuf[i] & BIT(j))
> > + cnt++;
> > + }
> > + if (cnt > nsrcbufs / 2)
> > + val |= BIT(j);
> > + }
> > + ((u8 *)dstbuf)[i] = val;
> > + }
> > +}
> > +
> > +/*
> >* Check if the NAND chip is ONFI compliant, returns 1 if it is, 0
> > otherwise.
> >*/
> > static int nand_flash_detect_onfi(struct nand_chip *chip)
> > @@ -5102,7 +5131,7 @@ static int nand_flash_detect_onfi(struct nand_chip
> > *chip)
> > return 0;
> >
> > /* ONFI chip: allocate a buffer to hold its parameter page */
> > - p = kzalloc(sizeof(*p), GFP_KERNEL);
> > + p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> > if (!p)
> > return -ENOMEM;
> >
> > @@ -5113,21 +5142,32 @@ static int nand_flash_detect_onfi(struct nand_chip
> > *chip)
> > }
> >
> > for (i = 0; i < 3; i++) {
> > - ret = nand_read_data_op(chip, p, sizeof(*p), true);
> > + ret = nand_read_data_op(chip, &p[i], sizeof(*p), true);
> > if (ret) {
> > ret = 0;
> > goto free_onfi_param_page;
> > }
> >
> > - if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> > + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) ==
> > le16_to_cpu(p->crc)) {
> > + if (i)
> > + memcpy(p, &p[i], sizeof(*p));
> > break;
> > }
> > }
> >
> > if (i == 3) {
> > - pr_err("Could not find valid ONFI parameter page; aborting\n");
> > - goto free_onfi_param_page;
> > + const void *srcbufs[3] = {p, p + 1, p + 2};
> > +
> > + pr_warn("Could not find a valid ONFI parameter page, trying
> > bit-wise majority to recover it\n");
> > + nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
> > + sizeof(*p));
> > +
> > + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) !=
> > + le16_to_cpu(p->crc)) {
> > + pr_err("ONFI parameter recovery failed, aborting\n");
> > + goto free_onfi_param_page;
> > + }
> > }
> >
> > /* Check version */
>
> This version is still hard coded for a three sample bitwise majority vote.
> So why not use the method which I suggested previously for v2 and which
> I repeat below?
Because I want the nand_bit_wise_majority() function to work with
nsrcbufs > 3 (the ONFI spec says there's at least 3 copy of the param
page, but NAND vendor can decide to put more). Also, if the X copies of
the PARAM are corrupted (which is rather unlikely), that means we
already spent quite a lot of time reading the different copies and
calculating the CRC, so I think we don't care about perf optimizations
when doing bit-wise majority.
>
> The three sample bitwise majority can be implemented without bit level
> manipulation using the identity:
> majority3(a, b, c) = (a & b) | (a & c) | (b & c)
> This can be factorized slightly to (a & (b | c)) | (
Re: [PATCH] mtd: nand: Fix return type of __DIVIDE() when called with 32-bit
On Mon, 14 May 2018 14:00:19 +0200
Geert Uytterhoeven wrote:
> Hi Boris,
>
> On Mon, May 14, 2018 at 1:46 PM, Boris Brezillon
> wrote:
> > On Mon, 14 May 2018 13:32:30 +0200
> > Geert Uytterhoeven wrote:
> >> On Mon, May 14, 2018 at 1:23 PM, Boris Brezillon
> >> wrote:
> >> > On Mon, 14 May 2018 12:49:37 +0200
> >> > Geert Uytterhoeven wrote:
> >> >> The __DIVIDE() macro checks whether it is called with a 32-bit or 64-bit
> >> >> dividend, to select the appropriate divide-and-round-up routine.
> >> >> As the check uses the ternary operator, the result will always be
> >> >> promoted to a type that can hold both results, i.e. unsigned long long.
> >> >>
> >> >> When using this result in a division on a 32-bit system, this may lead
> >> >> to link errors like:
> >> >>
> >> >> ERROR: "__udivdi3" [drivers/mtd/nand/raw/nand.ko] undefined!
> >> >>
> >> >> Fix this by casting the result of the 64-bit division to the type of the
> >> >> dividend.
> >> >>
> >> >> Fixes: 8878b126df769831 ("mtd: nand: add ->exec_op() implementation")
> >> >> Signed-off-by: Geert Uytterhoeven
> >> >> ---
> >> >> This fixes the root cause of the link failure seen with
> >> >> m68k/allmodconfig since commit 3057fcef385348fe ("mtd: rawnand: Make
> >> >> sure we wait tWB before polling the STATUS reg").
> >> >>
> >> >> An alternative mitigation was posted as "[PATCH] m68k: Implement
> >> >> ndelay() as an inline function to force type checking/casting"
> >> >> (https://lkml.org/lkml/2018/5/13/102).
> >> >> ---
> >> >> include/linux/mtd/rawnand.h | 2 +-
> >> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> >> >> index 5dad59b312440a9c..d06dc428ea0102ae 100644
> >> >> --- a/include/linux/mtd/rawnand.h
> >> >> +++ b/include/linux/mtd/rawnand.h
> >> >> @@ -871,7 +871,7 @@ struct nand_op_instr {
> >> >> #define __DIVIDE(dividend, divisor) ({
> >> >> \
> >> >> sizeof(dividend) == sizeof(u32) ? \
> >> >> DIV_ROUND_UP(dividend, divisor) : \
> >> >> - DIV_ROUND_UP_ULL(dividend, divisor);\
> >> >> + (__typeof__(dividend))DIV_ROUND_UP_ULL(dividend,
> >> >> divisor); \
> >> >
> >> > Hm, it's a bit hard to follow when you place the cast here. One could
> >> > wonder why a cast to (__typeof__(dividend)) is needed since
> >> > DIV_ROUND_UP_ULL() already returns a (__typeof__(dividend)) type.
> >>
> >> DIV_ROUND_UP_ULL() does not return __typeof__(dividend), but
> >> unsigned long long.
> >
> > Except if you entered this branch, that means you passed an unsigned
> > long long dividend (AKA u64), otherwise you would go in DIV_ROUND_UP().
> > Am I missing something?
>
> Sure, inside that branch, it does.
> But the compiler considers the whole ternary operator construction, i.e.
> both branches.
Yes, and that's my point. The (__typeof__(dividend)) when placed like
you did is ambiguous. It looks like you're doing a useless cast, while
what you're actually fixing is the case where dividend is an u32 (AKA
unsigned long), and from the reader PoV, the code you're fixing
shouldn't even be reached. Hence my suggestion to move the case one
level up and add a comment ;-).
>
> >> > How about:
> >> >
> >> > /*
> >> > * Cast to type of dividend is needed here to guarantee that the
> >> > * result won't be an unsigned long long when the dividend is an
> >> > * unsigned long, which is what the compiler does when it sees a
> >> >
> >>
> >> s/an unsigned long/32-bit/
> >>
> >> > * ternary operator with 2 different return types.
> >> > */
> >> > (__typeof__(dividend))(sizeof(dividend) == sizeof(u32) ?
> >> > \
> >
> > To be completely safe and handle cases where dividend is an unsigned
> > short or an unsigned, we should p
Re: [PATCH] mtd: nand: Fix return type of __DIVIDE() when called with 32-bit
On Mon, 14 May 2018 13:46:07 +0200
Boris Brezillon wrote:
> On Mon, 14 May 2018 13:32:30 +0200
> Geert Uytterhoeven wrote:
>
> > Hi Boris,
> >
> > On Mon, May 14, 2018 at 1:23 PM, Boris Brezillon
> > wrote:
> > > On Mon, 14 May 2018 12:49:37 +0200
> > > Geert Uytterhoeven wrote:
> > >> The __DIVIDE() macro checks whether it is called with a 32-bit or 64-bit
> > >> dividend, to select the appropriate divide-and-round-up routine.
> > >> As the check uses the ternary operator, the result will always be
> > >> promoted to a type that can hold both results, i.e. unsigned long long.
> > >>
> > >> When using this result in a division on a 32-bit system, this may lead
> > >> to link errors like:
> > >>
> > >> ERROR: "__udivdi3" [drivers/mtd/nand/raw/nand.ko] undefined!
> > >>
> > >> Fix this by casting the result of the 64-bit division to the type of the
> > >> dividend.
> > >>
> > >> Fixes: 8878b126df769831 ("mtd: nand: add ->exec_op() implementation")
> > >> Signed-off-by: Geert Uytterhoeven
> > >> ---
> > >> This fixes the root cause of the link failure seen with
> > >> m68k/allmodconfig since commit 3057fcef385348fe ("mtd: rawnand: Make
> > >> sure we wait tWB before polling the STATUS reg").
> > >>
> > >> An alternative mitigation was posted as "[PATCH] m68k: Implement
> > >> ndelay() as an inline function to force type checking/casting"
> > >> (https://lkml.org/lkml/2018/5/13/102).
> > >> ---
> > >> include/linux/mtd/rawnand.h | 2 +-
> > >> 1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> > >> index 5dad59b312440a9c..d06dc428ea0102ae 100644
> > >> --- a/include/linux/mtd/rawnand.h
> > >> +++ b/include/linux/mtd/rawnand.h
> > >> @@ -871,7 +871,7 @@ struct nand_op_instr {
> > >> #define __DIVIDE(dividend, divisor) ({
> > >> \
> > >> sizeof(dividend) == sizeof(u32) ? \
> > >> DIV_ROUND_UP(dividend, divisor) : \
> > >> - DIV_ROUND_UP_ULL(dividend, divisor);\
> > >> + (__typeof__(dividend))DIV_ROUND_UP_ULL(dividend, divisor);
> > >> \
> > >
> > > Hm, it's a bit hard to follow when you place the cast here. One could
> > > wonder why a cast to (__typeof__(dividend)) is needed since
> > > DIV_ROUND_UP_ULL() already returns a (__typeof__(dividend)) type.
> >
> > DIV_ROUND_UP_ULL() does not return __typeof__(dividend), but
> > unsigned long long.
>
> Except if you entered this branch, that means you passed an unsigned
> long long dividend (AKA u64), otherwise you would go in DIV_ROUND_UP().
> Am I missing something?
>
> >
> > > How about:
> > >
> > > /*
> > > * Cast to type of dividend is needed here to guarantee that the
> > > * result won't be an unsigned long long when the dividend is an
> > > * unsigned long, which is what the compiler does when it sees a
> > >
> >
> > s/an unsigned long/32-bit/
> >
> > > * ternary operator with 2 different return types.
> > > */
> > > (__typeof__(dividend))(sizeof(dividend) == sizeof(u32) ?\
> > >
>
> To be completely safe and handle cases where dividend is an unsigned
> short or an unsigned, we should probably have:
>
> (__typeof__(dividend))(sizeof(dividend) == sizeof(unsigned long long) ?
> \
> DIV_ROUND_UP_ULL(dividend, divisor) :
> DIV_ROUND_UP(dividend, divisor));
>
> > >DIV_ROUND_UP(dividend, divisor) :\
> > >DIV_ROUND_UP_ULL(dividend, divisor));
> >
> > Looks fine to me, too.
> >
> > > Actually, I'm not even sure we care about the truncation that could
> > > happen on an unsigned long long -> unsigned long cast because the
> > > delays we express here will anyway be hundreds of nanosecs/millisecs,
> > > so nothing close to the billions of nanosecs/millisecs you can express
> > > with an unsigned long.
> &
Re: [PATCH] mtd: nand: Fix return type of __DIVIDE() when called with 32-bit
On Mon, 14 May 2018 13:32:30 +0200
Geert Uytterhoeven wrote:
> Hi Boris,
>
> On Mon, May 14, 2018 at 1:23 PM, Boris Brezillon
> wrote:
> > On Mon, 14 May 2018 12:49:37 +0200
> > Geert Uytterhoeven wrote:
> >> The __DIVIDE() macro checks whether it is called with a 32-bit or 64-bit
> >> dividend, to select the appropriate divide-and-round-up routine.
> >> As the check uses the ternary operator, the result will always be
> >> promoted to a type that can hold both results, i.e. unsigned long long.
> >>
> >> When using this result in a division on a 32-bit system, this may lead
> >> to link errors like:
> >>
> >> ERROR: "__udivdi3" [drivers/mtd/nand/raw/nand.ko] undefined!
> >>
> >> Fix this by casting the result of the 64-bit division to the type of the
> >> dividend.
> >>
> >> Fixes: 8878b126df769831 ("mtd: nand: add ->exec_op() implementation")
> >> Signed-off-by: Geert Uytterhoeven
> >> ---
> >> This fixes the root cause of the link failure seen with
> >> m68k/allmodconfig since commit 3057fcef385348fe ("mtd: rawnand: Make
> >> sure we wait tWB before polling the STATUS reg").
> >>
> >> An alternative mitigation was posted as "[PATCH] m68k: Implement
> >> ndelay() as an inline function to force type checking/casting"
> >> (https://lkml.org/lkml/2018/5/13/102).
> >> ---
> >> include/linux/mtd/rawnand.h | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> >> index 5dad59b312440a9c..d06dc428ea0102ae 100644
> >> --- a/include/linux/mtd/rawnand.h
> >> +++ b/include/linux/mtd/rawnand.h
> >> @@ -871,7 +871,7 @@ struct nand_op_instr {
> >> #define __DIVIDE(dividend, divisor) ({
> >>\
> >> sizeof(dividend) == sizeof(u32) ? \
> >> DIV_ROUND_UP(dividend, divisor) : \
> >> - DIV_ROUND_UP_ULL(dividend, divisor);\
> >> + (__typeof__(dividend))DIV_ROUND_UP_ULL(dividend, divisor); \
> >>
> >
> > Hm, it's a bit hard to follow when you place the cast here. One could
> > wonder why a cast to (__typeof__(dividend)) is needed since
> > DIV_ROUND_UP_ULL() already returns a (__typeof__(dividend)) type.
>
> DIV_ROUND_UP_ULL() does not return __typeof__(dividend), but
> unsigned long long.
Except if you entered this branch, that means you passed an unsigned
long long dividend (AKA u64), otherwise you would go in DIV_ROUND_UP().
Am I missing something?
>
> > How about:
> >
> > /*
> > * Cast to type of dividend is needed here to guarantee that the
> > * result won't be an unsigned long long when the dividend is an
> > * unsigned long, which is what the compiler does when it sees a
>
> s/an unsigned long/32-bit/
>
> > * ternary operator with 2 different return types.
> > */
> > (__typeof__(dividend))(sizeof(dividend) == sizeof(u32) ?\
To be completely safe and handle cases where dividend is an unsigned
short or an unsigned, we should probably have:
(__typeof__(dividend))(sizeof(dividend) == sizeof(unsigned long long) ?
\
DIV_ROUND_UP_ULL(dividend, divisor) :
DIV_ROUND_UP(dividend, divisor));
> >DIV_ROUND_UP(dividend, divisor) :\
> >DIV_ROUND_UP_ULL(dividend, divisor));
>
> Looks fine to me, too.
>
> > Actually, I'm not even sure we care about the truncation that could
> > happen on an unsigned long long -> unsigned long cast because the
> > delays we express here will anyway be hundreds of nanosecs/millisecs,
> > so nothing close to the billions of nanosecs/millisecs you can express
> > with an unsigned long.
> >
> > So, maybe we should just do:
> >
> > (unsigned long)(sizeof(dividend) == sizeof(u32) ? \
> > DIV_ROUND_UP(dividend, divisor) : \
> > DIV_ROUND_UP_ULL(dividend, divisor));
> >
> > to make things more readable.
>
> That would break callers who pass a 64-bit dividend, and expect to receive
> a 64-bit quotient back (on 32-bit systems).
> Calling e.g. PSEC_TO_NSEC(1ULL) is valid, passing the
> result to ndelay() isn't ;-)
Well, theoretically, yes it's possible, in practice, we only ever pass
u32 types to PSEC_TO_NSEC() and u64 types to PSEC_TO_MSEC(), so why
bother.
Re: [PATCH] mtd: nand: Fix return type of __DIVIDE() when called with 32-bit
On Mon, 14 May 2018 12:49:37 +0200
Geert Uytterhoeven wrote:
> The __DIVIDE() macro checks whether it is called with a 32-bit or 64-bit
> dividend, to select the appropriate divide-and-round-up routine.
> As the check uses the ternary operator, the result will always be
> promoted to a type that can hold both results, i.e. unsigned long long.
>
> When using this result in a division on a 32-bit system, this may lead
> to link errors like:
>
> ERROR: "__udivdi3" [drivers/mtd/nand/raw/nand.ko] undefined!
>
> Fix this by casting the result of the 64-bit division to the type of the
> dividend.
>
> Fixes: 8878b126df769831 ("mtd: nand: add ->exec_op() implementation")
> Signed-off-by: Geert Uytterhoeven
> ---
> This fixes the root cause of the link failure seen with
> m68k/allmodconfig since commit 3057fcef385348fe ("mtd: rawnand: Make
> sure we wait tWB before polling the STATUS reg").
>
> An alternative mitigation was posted as "[PATCH] m68k: Implement
> ndelay() as an inline function to force type checking/casting"
> (https://lkml.org/lkml/2018/5/13/102).
> ---
> include/linux/mtd/rawnand.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index 5dad59b312440a9c..d06dc428ea0102ae 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -871,7 +871,7 @@ struct nand_op_instr {
> #define __DIVIDE(dividend, divisor) ({
> \
> sizeof(dividend) == sizeof(u32) ? \
> DIV_ROUND_UP(dividend, divisor) : \
> - DIV_ROUND_UP_ULL(dividend, divisor);\
> + (__typeof__(dividend))DIV_ROUND_UP_ULL(dividend, divisor); \
Hm, it's a bit hard to follow when you place the cast here. One could
wonder why a cast to (__typeof__(dividend)) is needed since
DIV_ROUND_UP_ULL() already returns a (__typeof__(dividend)) type.
How about:
/*
* Cast to type of dividend is needed here to guarantee that the
* result won't be an unsigned long long when the dividend is an
* unsigned long, which is what the compiler does when it sees a
* ternary operator with 2 different return types.
*/
(__typeof__(dividend))(sizeof(dividend) == sizeof(u32) ?\
DIV_ROUND_UP(dividend, divisor) :\
DIV_ROUND_UP_ULL(dividend, divisor));
Actually, I'm not even sure we care about the truncation that could
happen on an unsigned long long -> unsigned long cast because the
delays we express here will anyway be hundreds of nanosecs/millisecs,
so nothing close to the billions of nanosecs/millisecs you can express
with an unsigned long.
So, maybe we should just do:
(unsigned long)(sizeof(dividend) == sizeof(u32) ? \
DIV_ROUND_UP(dividend, divisor) : \
DIV_ROUND_UP_ULL(dividend, divisor));
to make things more readable.
> })
> #define PSEC_TO_NSEC(x) __DIVIDE(x, 1000)
> #define PSEC_TO_MSEC(x) __DIVIDE(x, 10)
Re: [PATCH 4.16 41/72] mtd: rawnand: Make sure we wait tWB before polling the STATUS reg
On Mon, 14 May 2018 11:04:22 +0200
Greg Kroah-Hartman wrote:
> On Mon, May 14, 2018 at 09:32:51AM +0200, Geert Uytterhoeven wrote:
> > On Mon, May 14, 2018 at 8:48 AM, Greg Kroah-Hartman
> > wrote:
> > > 4.16-stable review patch. If anyone has any objections, please let me
> > > know.
> > >
> > > --
> > >
> > > From: Boris Brezillon
> > >
> > > commit 3057fcef385348fe85173f1b0c824d89f1176f72 upstream.
> > >
> > > NAND chips require a bit of time to take the NAND operation into
> > > account and set the BUSY bit in the STATUS reg. Make sure we don't poll
> > > the STATUS reg too early in nand_soft_waitrdy().
> > >
> > > Fixes: 8878b126df76 ("mtd: nand: add ->exec_op() implementation")
> > > Cc:
> > > Signed-off-by: Boris Brezillon
> > > Acked-by: Miquel Raynal
> > > Signed-off-by: Greg Kroah-Hartman
> > >
> > > ---
> > > drivers/mtd/nand/nand_base.c |5 +
> > > 1 file changed, 5 insertions(+)
> > >
> > > --- a/drivers/mtd/nand/nand_base.c
> > > +++ b/drivers/mtd/nand/nand_base.c
> > > @@ -707,12 +707,17 @@ static void nand_wait_status_ready(struc
> > > */
> > > int nand_soft_waitrdy(struct nand_chip *chip, unsigned long timeout_ms)
> > > {
> > > + const struct nand_sdr_timings *timings;
> > > u8 status = 0;
> > > int ret;
> > >
> > > if (!chip->exec_op)
> > > return -ENOTSUPP;
> > >
> > > + /* Wait tWB before polling the STATUS reg. */
> > > + timings = nand_get_sdr_timings(&chip->data_interface);
> > > + ndelay(PSEC_TO_NSEC(timings->tWB_max));
> > > +
> > > ret = nand_status_op(chip, NULL);
> > > if (ret)
> > > return ret;
> > >
> > >
> >
> >
> > kbuild test robot via vger.kernel.org
> > Attachments1:36 PM (19 hours ago)
> > to Boris, kbuild-all, linux-kernel
> > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> > master
> > head: ccda3c4b7f66aeb3c531352bb40d59501c59
> > commit: 3057fcef385348fe85173f1b0c824d89f1176f72 mtd: rawnand: Make
> > sure we wait tWB before polling the STATUS reg
> > date: 3 days ago
> > config: m68k-allyesconfig (attached as .config)
> > compiler: m68k-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
> > reproduce:
> > wget
> > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
> > -O ~/bin/make.cross
> > chmod +x ~/bin/make.cross
> > git checkout 3057fcef385348fe85173f1b0c824d89f1176f72
> > # save the attached .config to linux build tree
> > make.cross ARCH=m68k
> >
> > All errors (new ones prefixed by >>):
> >
> >drivers/mtd/nand/raw/nand_base.o: In function `nand_soft_waitrdy':
> > >> nand_base.c:(.text+0x1022): undefined reference to `__udivdi3'
>
> Ugh. Does this also happen in Linus's tree?
Yes it does, unfortunately :-/. I posted a fix here [1].
[1]https://patchwork.kernel.org/patch/10396217/
Re: [Ksummit-discuss] bug-introducing patches
+Fengguang On Mon, 14 May 2018 10:40:10 +0200 Geert Uytterhoeven wrote: > Hi Boris, > > On Mon, May 14, 2018 at 10:34 AM, Boris Brezillon > wrote: > > On Mon, 14 May 2018 10:29:04 +0200 > > Geert Uytterhoeven wrote: > >> On Mon, May 14, 2018 at 10:12 AM, Boris Brezillon > >> wrote: > >> > On Mon, 14 May 2018 10:00:30 +0200 > >> > Geert Uytterhoeven wrote: > >> >> On Tue, May 1, 2018 at 10:00 PM, Sasha Levin > >> >> wrote: > >> >> > On Tue, May 01, 2018 at 03:44:50PM -0400, Theodore Y. Ts'o wrote: > >> >> >>On Tue, May 01, 2018 at 04:38:21PM +, Sasha Levin wrote: > >> >> > What's worse is that that commit is tagged for stable, which means > >> >> > that (given Greg's schedule) it may find it's way to -stable users > >> >> > even before some -next users/bots had a chance to test it out. > >> >> > >> >> I just noticed a case where a commit was picked up for stable, while a > >> >> bot had flagged it as a build regression 18 hours earlier (with a CC to > >> >> lkml). > >> > > >> > Also, this patch has been on a tree that I know is tested by Fengguang's > >> > robots for more than a week (and in linux-next for 2 days, which, I > >> > agree, is probably not enough), and still, I only received the bug > >> > report when the patch reached mainline. Are there tests that are only > >> > run on Linus' tree? > >> > >> Have your received a success report from Fengguang's bot, listing all > >> configs tested (the broken one should be included; it is included in the > >> configs tested on my branches)? > > > > Yes I did (see below). > > > > -->8-- > > From: kbuild test robot > > To: Boris Brezillon > > Subject: [bbrezillon-0day:mtd/fixes] BUILD SUCCESS > > fc3a9e15b492eef707afd56b7478001fdecfe53f > > Date: Mon, 07 May 2018 20:05:52 +0800 > > User-Agent: Heirloom mailx 12.5 6/20/10 > > > > tree/branch: https://github.com/bbrezillon/linux-0day mtd/fixes > > branch HEAD: fc3a9e15b492eef707afd56b7478001fdecfe53f mtd: rawnand: Make > > sure we wait tWB before polling the STATUS reg > > > > elapsed time: 49m > > > > configs tested: 142 > > But the failed config (m68k/allmodconfig) is not listed? Yes, that's my point. It seems that some configs are only rarely (never?) tested on my linux-0day tree (probably because they take longer to build), and I should only take kbuild robot results as an indication not a guarantee.
Re: [Ksummit-discuss] bug-introducing patches
On Mon, 14 May 2018 10:29:04 +0200 Geert Uytterhoeven wrote: > Hi Boris, > > On Mon, May 14, 2018 at 10:12 AM, Boris Brezillon > wrote: > > On Mon, 14 May 2018 10:00:30 +0200 > > Geert Uytterhoeven wrote: > >> On Tue, May 1, 2018 at 10:00 PM, Sasha Levin > >> wrote: > >> > On Tue, May 01, 2018 at 03:44:50PM -0400, Theodore Y. Ts'o wrote: > >> >>On Tue, May 01, 2018 at 04:38:21PM +, Sasha Levin wrote: > >> > What's worse is that that commit is tagged for stable, which means > >> > that (given Greg's schedule) it may find it's way to -stable users > >> > even before some -next users/bots had a chance to test it out. > >> > >> I just noticed a case where a commit was picked up for stable, while a > >> bot had flagged it as a build regression 18 hours earlier (with a CC to > >> lkml). > > > > Also, this patch has been on a tree that I know is tested by Fengguang's > > robots for more than a week (and in linux-next for 2 days, which, I > > agree, is probably not enough), and still, I only received the bug > > report when the patch reached mainline. Are there tests that are only > > run on Linus' tree? > > Have your received a success report from Fengguang's bot, listing all > configs tested (the broken one should be included; it is included in the > configs tested on my branches)? Yes I did (see below). -->8-- From: kbuild test robot To: Boris Brezillon Subject: [bbrezillon-0day:mtd/fixes] BUILD SUCCESS fc3a9e15b492eef707afd56b7478001fdecfe53f Date: Mon, 07 May 2018 20:05:52 +0800 User-Agent: Heirloom mailx 12.5 6/20/10 tree/branch: https://github.com/bbrezillon/linux-0day mtd/fixes branch HEAD: fc3a9e15b492eef707afd56b7478001fdecfe53f mtd: rawnand: Make sure we wait tWB before polling the STATUS reg elapsed time: 49m configs tested: 142 The following configs have been built successfully. More configs may be tested in the coming days. powerpc skiroot_defconfig sh kfr2r09_defconfig x86_64 acpi-redef x86_64 allyesdebian x86_64nfsroot m68k bvme6000_defconfig powerpc ppa8548_defconfig shallnoconfig sh rsk7269_defconfig sh sh7785lcr_32bit_defconfig shtitan_defconfig i386 randconfig-c0-05071338 i386 tinyconfig i386 randconfig-n0-201818 x86_64 randconfig-x002-201818 x86_64 randconfig-x006-201818 x86_64 randconfig-x005-201818 x86_64 randconfig-x001-201818 x86_64 randconfig-x009-201818 x86_64 randconfig-x004-201818 x86_64 randconfig-x003-201818 x86_64 randconfig-x007-201818 x86_64 randconfig-x000-201818 x86_64 randconfig-x008-201818 i386 randconfig-i1-201818 i386 randconfig-i0-201818 alpha defconfig pariscallnoconfig parisc b180_defconfig pariscc3000_defconfig parisc defconfig ia64defconfig mipsdefconfig powerpc allnoconfig powerpc defconfig powerpc ppc64_defconfig s390default_defconfig x86_64 randconfig-g0-05071702 openriscor1ksim_defconfig um i386_defconfig um x86_64_defconfig i386 allmodconfig ia64 alldefconfig ia64 allnoconfig i386 randconfig-a0-201818 i386 randconfig-a1-201818 x86_64 randconfig-s0-05071833 x86_64 randconfig-s1-05071833 x86_64 randconfig-s2-05071833 x86_64 randconfig-s0-05071933 x86_64 randconfig-s1-05071933 x86_64 randconfig-s2-05071933 c6xevmc6678_defconfig h8300h8300h-sim_defconfig nios2 10m50_defconfig xtensa common_defconfig xtensa iss_defconfig i386 alldefconfig i386 allnoconfig i386defconfig x86_64 federa-25 x86_64 rhel x86_64
Re: [Ksummit-discuss] bug-introducing patches
On Mon, 14 May 2018 10:00:30 +0200
Geert Uytterhoeven wrote:
> On Tue, May 1, 2018 at 10:00 PM, Sasha Levin
> wrote:
> > On Tue, May 01, 2018 at 03:44:50PM -0400, Theodore Y. Ts'o wrote:
> >>On Tue, May 01, 2018 at 04:38:21PM +, Sasha Levin wrote:
> >>> - A merge window commit spent 50% more days, on average, in -next than a
> >>> -rc
> >>>commit.
> >>
> >>So it *used* to be the case that after the merge window, I would queue
> >>up bug fixes for the next merge window. Greg K-H pushed for me to
> >>send them to Linus sooner, instead of waiting for the next merge
> >>window. TBH, it's actually easier for me to just wait until the next
> >>merge window, but please understand that there are multiple pressures
> >>on maintainers going on here, and the latest efforts to try to use
> >>AUTOSEL is just the most recent pressure placed on maintainers.
> >>
> >>The other thing is that when there is a regression users who are
> >>testing linux-next want it fixed *fast*. That's considered more
> >>important to them than waiting for one, perfect patch, just to keep
> >>AUTOSEL happy.
> >>
> >>So please understand that when you say that maintainers *need* to do X
> >>or Y, that there you are not the only one standing in line putting
> >>pressures on maintainers saying they *need* to do something. And
> >>quite frankly, I consider keeping people who are nice enough to test
> >>linux-next happy to be **far** more important than AUTOSEL.
> >
> > Ted,
> >
> > I'm not at all asking to wait before adding the patches to your tree,
> > or to -next. I'm asking to hold on to them a bit longer before you
> > push them to Linus because I can show that patches that don't spend
> > enough time in -next are more likely to introduce bugs.
> >
> > Yes, linux-next users want it fixed *now* and I completely agree it
> > should be done that way, but the fix should not be immediately pushed to
> > Linus as well.
> >
> > I've just finished reading an interesting article on LWN about the
> > PostgreSQL fsync issues (https://lwn.net/Articles/752952/). If you
> > look at Willy's commit, he wrote the final version of it about 5 days
> > ago, Jeff merged it in 3 days ago, and Linus merged it in the tree
> > today. Did it spend any time getting -next testing? nope.
> >
> > What's worse is that that commit is tagged for stable, which means
> > that (given Greg's schedule) it may find it's way to -stable users
> > even before some -next users/bots had a chance to test it out.
>
> I just noticed a case where a commit was picked up for stable, while a
> bot had flagged it as a build regression 18 hours earlier (with a CC to
> lkml).
Also, this patch has been on a tree that I know is tested by Fengguang's
robots for more than a week (and in linux-next for 2 days, which, I
agree, is probably not enough), and still, I only received the bug
report when the patch reached mainline. Are there tests that are only
run on Linus' tree?
>
> So it looks like the script for backporting commits should be enhanced to
> check for this (searching for the commit ID in my email archive found the
> bot report).
>
> Thanks!
>
> Gr{oetje,eeting}s,
>
> Geert
>
[PATCH] m68k: Implement ndelay() as an inline function to force type checking/casting
ndelay() is supposed to take an unsigned long, but if you define
ndelay() as a macro and the caller pass an unsigned long long instead
of an unsigned long, the unsigned long long to unsigned long cast is
not done and we end up with an "undefined reference to `__udivdi3'"
error at link time.
Fix that by making ndelay() an inline function and then defining dummy
ndelay() macro that redirects to the ndelay() function (it's how most
archs do to implement ndelay()).
Fixes: c8ee038bd148 ("m68k: Implement ndelay() based on the existing udelay()
logic")
Signed-off-by: Boris Brezillon
---
Hello Geert,
This patch is fixing the bug reported by kbuild test robot here [1].
I could have patched the PSEC_TO_NSEC() macro to cast the result of
the division on a u32, but I thought making m68k consistent with what
other archs do would be preferable.
Let me know if don't like the solution, and I'll patch the ndelay()
caller instead.
Regards,
Boris
[1]https://lkml.org/lkml/2018/5/13/72
---
arch/m68k/include/asm/delay.h | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/arch/m68k/include/asm/delay.h b/arch/m68k/include/asm/delay.h
index 7f474121e4ca..66ba159002a3 100644
--- a/arch/m68k/include/asm/delay.h
+++ b/arch/m68k/include/asm/delay.h
@@ -115,6 +115,13 @@ static inline void __udelay(unsigned long usecs)
*/
#defineHZSCALE (268435456 / (100 / HZ))
-#define ndelay(n) __delay(DIV_ROUND_UP((n) * HZSCALE) >> 11) *
(loops_per_jiffy >> 11)) >> 6), 1000))
+static inline void ndelay(unsigned long nsec)
+{
+ __delay(DIV_ROUND_UP(nsec *
+HZSCALE) >> 11) *
+ (loops_per_jiffy >> 11)) >> 6),
+1000));
+}
+#define ndelay(n) ndelay(n)
#endif /* defined(_M68K_DELAY_H) */
--
2.14.1
Re: [PATCH v7] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter
Applied after fixing a few things (see below).
Changed the subject to "mtd: rawnand: use bit-wise majority to recover
the ONFI param page"
On Sun, 13 May 2018 04:30:02 +
"Wan, Jane (Nokia - US/Sunnyvale)" wrote:
> Per ONFI specification (Rev. 4.0), if all parameter pages have invalid CRC
> values, the bit-wise majority may be used to recover the contents of the
> parameter pages from the parameter page copies present.
Wrapped this line.
>
> Signed-off-by: Jane Wan
> ---
> v7: change debug print messages
> v6: support the cases that srcbufs are not contiguous
> v5: make the bit-wise majority functon generic
> v4: move the bit-wise majority code in a separate function
> v3: fix warning message detected by kbuild test robot
> v2: rebase the changes on top of v4.17-rc1
>
> drivers/mtd/nand/raw/nand_base.c | 50 ++
> 1 file changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/nand_base.c
> b/drivers/mtd/nand/raw/nand_base.c
> index 72f3a89..b43b784 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -5087,6 +5087,35 @@ static int nand_flash_detect_ext_param_page(struct
> nand_chip *chip,
> }
>
> /*
> + * Recover data with bit-wise majority
> + */
> +static void nand_bit_wise_majority(const void **srcbufs,
> +unsigned int nsrcbufs,
> +void *dstbuf,
> +unsigned int bufsize)
> +{
> + int i, j, k;
> +
> + for (i = 0; i < bufsize; i++) {
> + u8 cnt, val;
Moved declaration cnt in the for(j) loop and made it an unsigned int.
> +
> + val = 0;
> + for (j = 0; j < 8; j++) {
> + cnt = 0;
> + for (k = 0; k < nsrcbufs; k++) {
> + const u8 *srcbuf = srcbufs[k];
> +
> + if (srcbuf[i] & BIT(j))
> + cnt++;
> + }
> + if (cnt > nsrcbufs / 2)
> + val |= BIT(j);
> + }
> + ((u8 *)dstbuf)[i] = val;
> + }
> +}
> +
> +/*
> * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
> */
> static int nand_flash_detect_onfi(struct nand_chip *chip)
> @@ -5102,7 +5131,7 @@ static int nand_flash_detect_onfi(struct nand_chip
> *chip)
> return 0;
>
> /* ONFI chip: allocate a buffer to hold its parameter page */
> - p = kzalloc(sizeof(*p), GFP_KERNEL);
> + p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> if (!p)
> return -ENOMEM;
>
> @@ -5113,21 +5142,32 @@ static int nand_flash_detect_onfi(struct nand_chip
> *chip)
> }
>
> for (i = 0; i < 3; i++) {
> - ret = nand_read_data_op(chip, p, sizeof(*p), true);
> + ret = nand_read_data_op(chip, &p[i], sizeof(*p), true);
> if (ret) {
> ret = 0;
> goto free_onfi_param_page;
> }
>
> - if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) ==
> le16_to_cpu(p->crc)) {
> + if (i)
> + memcpy(p, &p[i], sizeof(*p));
> break;
> }
> }
>
> if (i == 3) {
> - pr_err("Could not find valid ONFI parameter page; aborting\n");
> - goto free_onfi_param_page;
> + const void *srcbufs[3] = {p, p + 1, p + 2};
> +
> + pr_warn("Could not find a valid ONFI parameter page, trying
> bit-wise majority to recover it\n");
> + nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
> +sizeof(*p));
> +
> + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) !=
> + le16_to_cpu(p->crc)) {
> + pr_err("ONFI parameter recovery failed, aborting\n");
> + goto free_onfi_param_page;
> + }
> }
>
> /* Check version */
Re: [PATCH] mtd: nand: Add support for reading ooblayout from device tree
On Sat, 12 May 2018 19:42:49 +0200 Paul Cercueil wrote: > >> >> My motivation is to get rid of this (move it to devicetree): > >> >> > >> >> > >> https://elixir.bootlin.com/linux/latest/source/arch/mips/jz4740/board-qi_lb60.c#L93 > >> > >> >> And enable the support of other boards with custom OOB layouts. > >> > > >> > Can you list the different layouts you have? I'm pretty sure > >> there's a > >> > pattern. Maybe we can even deduce the layout from the page size > >> or OOB > >> > size. > >> > >> This is the other layout I have for another ingenic device: > >> > >> http://projects.qi-hardware.com/index.php/p/qi-kernel/source/tree/od-2011-09-18/arch/mips/jz4740/board-a320.c#L125 > >> > >> Page size and OOB size are the same between these two devices. > > > > Indeed. Do you know if there are other kind of layouts in the wild? > > I'm getting a new board in a few weeks, I'll be able to check that out. > > > Note that can be a string, so if each each board is > > defining its own layout, you could specify the board name here. > > Otherwise, if you just have those 2 patterns, you can just name them > > "contiguous" and "interleaved". > > I don't like the idea of adding board-specific data inside the driver... > I'd prefer to use the method I used in this patch, but inside the > jz4740-nand driver, if you're OK with it. Please don't. Encoding such detailed description in the DT has almost always proven to be poor choice. Also, people are likely to get it wrong, and then you'll have to fix all DTs, while, with a single unique ID representing the layout, you can - re-use existing layouts easily without having to describe everything again in the DT - fix the driver without getting in trouble with people who claim that DT is a stable ABI and don't want to update their DT So, please just pick user-friendly IDs and add layout definitions in the driver. If you don't want to leak board info in the driver, then don't name the layout with the board name. BTW, I still hope you'll only have 2 kind of layouts (contiguous and interleaved).
Re: [PATCH] mtd: nand: Add support for reading ooblayout from device tree
On Sat, 12 May 2018 11:38:26 -0300 Paul Cercueil wrote: > Le sam. 12 mai 2018 à 10:42, Boris Brezillon > a écrit : > > On Sat, 12 May 2018 08:55:40 -0300 > > Paul Cercueil wrote: > > > >> Hi Boris, > >> > >> Le 12 mai 2018 02:55, Boris Brezillon > >> a écrit : > >> > > >> > Hi Paul, > >> > > >> > On Fri, 11 May 2018 23:29:12 +0200 > >> > Paul Cercueil wrote: > >> > > >> > > By specifying the properties "mtd-oob-ecc" and "mtd-oob-free", > >> it is > >> > > now possible to specify from devicetree where the ECC data is > >> located > >> > > inside the OOB region. > >> > > >> > Why would we want to do that? I mean, ECC/free regions are ECC > >> > controller dependent (and NAND chip dependent for the OOB size > >> part), > >> > so there's no reason to describe it in the DT. And more > >> importantly, > >> > people are likely to get it wrong. > >> > > >> > I'm curious, why do you need that? > >> > >> Good question. > >> > >> The reason is that some SoCs have no ECC controller. > >> The various boards for these SoCs then all use a different layout. > > > > Okay. Still think defining the layouts in the DT is a bad idea. We > > can add a jz4740 specific property to define the layout id > > (ingenic,nand-oob-layout = ), but not a generic way to > > define custom layouts for all kind of NAND controller. > > Okay. > > >> > >> My motivation is to get rid of this (move it to devicetree): > >> > >> https://elixir.bootlin.com/linux/latest/source/arch/mips/jz4740/board-qi_lb60.c#L93 > >> And enable the support of other boards with custom OOB layouts. > > > > Can you list the different layouts you have? I'm pretty sure there's a > > pattern. Maybe we can even deduce the layout from the page size or OOB > > size. > > This is the other layout I have for another ingenic device: > http://projects.qi-hardware.com/index.php/p/qi-kernel/source/tree/od-2011-09-18/arch/mips/jz4740/board-a320.c#L125 > > Page size and OOB size are the same between these two devices. Indeed. Do you know if there are other kind of layouts in the wild? Note that can be a string, so if each each board is defining its own layout, you could specify the board name here. Otherwise, if you just have those 2 patterns, you can just name them "contiguous" and "interleaved".
Re: [PATCH] mtd: nand: Add support for reading ooblayout from device tree
On Sat, 12 May 2018 08:55:40 -0300 Paul Cercueil wrote: > Hi Boris, > > Le 12 mai 2018 02:55, Boris Brezillon a écrit : > > > > Hi Paul, > > > > On Fri, 11 May 2018 23:29:12 +0200 > > Paul Cercueil wrote: > > > > > By specifying the properties "mtd-oob-ecc" and "mtd-oob-free", it is > > > now possible to specify from devicetree where the ECC data is located > > > inside the OOB region. > > > > Why would we want to do that? I mean, ECC/free regions are ECC > > controller dependent (and NAND chip dependent for the OOB size part), > > so there's no reason to describe it in the DT. And more importantly, > > people are likely to get it wrong. > > > > I'm curious, why do you need that? > > Good question. > > The reason is that some SoCs have no ECC controller. > The various boards for these SoCs then all use a different layout. Okay. Still think defining the layouts in the DT is a bad idea. We can add a jz4740 specific property to define the layout id (ingenic,nand-oob-layout = ), but not a generic way to define custom layouts for all kind of NAND controller. > > My motivation is to get rid of this (move it to devicetree): > https://elixir.bootlin.com/linux/latest/source/arch/mips/jz4740/board-qi_lb60.c#L93 > And enable the support of other boards with custom OOB layouts. Can you list the different layouts you have? I'm pretty sure there's a pattern. Maybe we can even deduce the layout from the page size or OOB size.
Re: [PATCH v6] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter
On Thu, 10 May 2018 14:28:37 -0700
Jane Wan wrote:
> Per ONFI specification (Rev. 4.0), if all parameter pages have invalid
> CRC values, the bit-wise majority may be used to recover the contents of
> the parameter pages from the parameter page copies present.
>
> Signed-off-by: Jane Wan
> ---
> v6: support the cases that srcbufs are not contiguous
> v5: make the bit-wise majority functon generic
> v4: move the bit-wise majority code in a separate function
> v3: fix warning message detected by kbuild test robot
> v2: rebase the changes on top of v4.17-rc1
>
> drivers/mtd/nand/raw/nand_base.c | 52
> ++
> 1 file changed, 47 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/nand_base.c
> b/drivers/mtd/nand/raw/nand_base.c
> index 72f3a89..acf905c 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -5087,6 +5087,35 @@ static int nand_flash_detect_ext_param_page(struct
> nand_chip *chip,
> }
>
> /*
> + * Recover data with bit-wise majority
> + */
> +static void nand_bit_wise_majority(const void **srcbufs,
> +unsigned int nsrcbufs,
> +void *dstbuf,
> +unsigned int bufsize)
> +{
> + int i, j, k;
> +
> + for (i = 0; i < bufsize; i++) {
> + u8 cnt, val;
> +
> + val = 0;
> + for (j = 0; j < 8; j++) {
> + cnt = 0;
> + for (k = 0; k < nsrcbufs; k++) {
> + const u8 *srcbuf = srcbufs[k];
> +
> + if (srcbuf[i] & BIT(j))
> + cnt++;
> + }
> + if (cnt > nsrcbufs / 2)
> + val |= BIT(j);
> + }
> + ((u8 *)dstbuf)[i] = val;
> + }
> +}
> +
> +/*
> * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
> */
> static int nand_flash_detect_onfi(struct nand_chip *chip)
> @@ -5102,7 +5131,7 @@ static int nand_flash_detect_onfi(struct nand_chip
> *chip)
> return 0;
>
> /* ONFI chip: allocate a buffer to hold its parameter page */
> - p = kzalloc(sizeof(*p), GFP_KERNEL);
> + p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> if (!p)
> return -ENOMEM;
>
> @@ -5113,21 +5142,34 @@ static int nand_flash_detect_onfi(struct nand_chip
> *chip)
> }
>
> for (i = 0; i < 3; i++) {
> - ret = nand_read_data_op(chip, p, sizeof(*p), true);
> + ret = nand_read_data_op(chip, &p[i], sizeof(*p), true);
> if (ret) {
> ret = 0;
> goto free_onfi_param_page;
> }
>
> - if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) ==
> le16_to_cpu(p->crc)) {
> + if (i)
> + memcpy(p, &p[i], sizeof(*p));
> break;
> }
> }
>
> if (i == 3) {
> - pr_err("Could not find valid ONFI parameter page; aborting\n");
> - goto free_onfi_param_page;
> + const void *srcbufs[3] = {p, p + 1, p + 2};
> +
> + pr_err("Could not find valid ONFI parameter page\n");
Maybe pr_warn() here
> + pr_info("Recover ONFI params with bit-wise majority\n");
and maybe you can pack the 2 messages:
pr_warn("Could not find a valid ONFI parameter page, trying
bit-wise majority to recover it");
> +
> + nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
> +sizeof(*p));
> +
> + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) !=
> + le16_to_cpu(p->crc)) {
> + pr_err("ONFI parameter recovery failed, aborting\n");
> + goto free_onfi_param_page;
> + }
> }
>
> /* Check version */
Re: [PATCH] mtd: nand: Add support for reading ooblayout from device tree
Hi Paul,
On Fri, 11 May 2018 23:29:12 +0200
Paul Cercueil wrote:
> By specifying the properties "mtd-oob-ecc" and "mtd-oob-free", it is
> now possible to specify from devicetree where the ECC data is located
> inside the OOB region.
Why would we want to do that? I mean, ECC/free regions are ECC
controller dependent (and NAND chip dependent for the OOB size part),
so there's no reason to describe it in the DT. And more importantly,
people are likely to get it wrong.
I'm curious, why do you need that?
Regards,
Boris
>
> Signed-off-by: Paul Cercueil
> ---
> Documentation/devicetree/bindings/mtd/nand.txt | 7 +
> drivers/mtd/nand/raw/nand_base.c | 42
> ++
> 2 files changed, 49 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mtd/nand.txt
> b/Documentation/devicetree/bindings/mtd/nand.txt
> index 8bb11d809429..118ea92787cb 100644
> --- a/Documentation/devicetree/bindings/mtd/nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/nand.txt
> @@ -45,6 +45,13 @@ Optional NAND chip properties:
>as reliable as possible.
> - nand-rb: shall contain the native Ready/Busy ids.
>
> +- nand-oob-ecc: couples of integers, specifying the offset
> + and length of the ECC data in the OOB region. There can be
> more
> + than one couple.
> +- nand-oob-free: couples of integers, specifying the offset
> + and length of a free-to-use area in the OOB region. There
> can be
> + more than one couple.
> +
> The ECC strength and ECC step size properties define the correction
> capability
> of a controller. Together, they say a controller can correct "{strength} bit
> errors per {size} bytes".
> diff --git a/drivers/mtd/nand/raw/nand_base.c
> b/drivers/mtd/nand/raw/nand_base.c
> index 72f3a89da513..c905531effb0 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -213,6 +213,43 @@ static const struct mtd_ooblayout_ops
> nand_ooblayout_lp_hamming_ops = {
> .free = nand_ooblayout_free_lp_hamming,
> };
>
> +static int nand_oob_of(struct device_node *np, int section,
> +struct mtd_oob_region *oobregion, const char *prop)
> +{
> + int ret = of_property_read_u32_index(np, prop,
> + section * 2, &oobregion->offset);
> + if (ret == -EOVERFLOW)
> + return -ERANGE; /* We're done */
> + if (ret)
> + return ret;
> +
> + ret = of_property_read_u32_index(np, prop,
> + section * 2 + 1, &oobregion->length);
> + if (ret == -EOVERFLOW)
> + return -EINVAL; /* We must have an even number of integers */
> +
> + return ret;
> +}
> +
> +static int nand_ooblayout_ecc_of(struct mtd_info *mtd, int section,
> + struct mtd_oob_region *oobregion)
> +{
> + return nand_oob_of(mtd->dev.of_node, section,
> + oobregion, "nand-oob-ecc");
> +}
> +
> +static int nand_ooblayout_free_of(struct mtd_info *mtd, int section,
> + struct mtd_oob_region *oobregion)
> +{
> + return nand_oob_of(mtd->dev.of_node, section,
> + oobregion, "nand-oob-free");
> +}
> +
> +static const struct mtd_ooblayout_ops nand_ooblayout_of_ops = {
> + .ecc = nand_ooblayout_ecc_of,
> + .free = nand_ooblayout_free_of,
> +};
> +
> static int check_offs_len(struct mtd_info *mtd,
> loff_t ofs, uint64_t len)
> {
> @@ -5843,6 +5880,11 @@ static int nand_dt_init(struct nand_chip *chip)
> if (of_property_read_bool(dn, "nand-ecc-maximize"))
> chip->ecc.options |= NAND_ECC_MAXIMIZE;
>
> + if (!chip->mtd.ooblayout &&
> + of_property_read_bool(dn, "nand-oob-ecc") &&
> + of_property_read_bool(dn, "nand-oob-free"))
> + chip->mtd.ooblayout = &nand_ooblayout_of_ops;
> +
> return 0;
> }
>
[GIT PULL] mtd: Fixes for 4.17-rc5
Hello Linus, Here is the MTD fixes PR for 4.17-rc5. Regards, Boris The following changes since commit 6da6c0db5316275015e8cc2959f12a17584aeb64: Linux v4.17-rc3 (2018-04-29 14:17:42 -0700) are available in the git repository at: git://git.infradead.org/linux-mtd.git tags/mtd/fixes-for-4.17-rc5 for you to fetch changes up to 3057fcef385348fe85173f1b0c824d89f1176f72: mtd: rawnand: Make sure we wait tWB before polling the STATUS reg (2018-05-10 08:40:39 +0200) NAND fixes: - Make nand_soft_waitrdy() wait tWB before polling the status REG - Fix BCH write in the the Marvell NAND controller driver - Fix wrong picosec to msec conversion in the Marvell NAND controller driver - Fix DMA handling in the TI OneNAND controllre driver Boris Brezillon (1): mtd: rawnand: Make sure we wait tWB before polling the STATUS reg Chris Packham (1): mtd: rawnand: marvell: pass ms delay to wait_op Ladislav Michl (1): mtd: onenand: omap2: Disable DMA for HIGHMEM buffers Miquel Raynal (1): mtd: rawnand: marvell: fix command xtype in BCH write hook drivers/mtd/nand/onenand/omap2.c| 105 ++--- drivers/mtd/nand/raw/marvell_nand.c | 12 +-- drivers/mtd/nand/raw/nand_base.c| 5 +++ 3 files changed, 52 insertions(+), 70 deletions(-)
Re: [PATCH v5 2/2] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter
Hi Jane,
Subject prefix should be "[PATCH v5] ...", the 2/2 is no longer valid
since you only have one patch here.
On Wed, 9 May 2018 19:46:40 -0700
Jane Wan wrote:
> Per ONFI specification (Rev. 4.0), if all parameter pages have invalid
> CRC values, the bit-wise majority may be used to recover the contents of
> the parameter pages from the parameter page copies present.
>
> Signed-off-by: Jane Wan
> ---
There should be a changelog here describing what has changed in each
version of the patch.
> drivers/mtd/nand/raw/nand_base.c | 46
> +-
> 1 file changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/nand_base.c
> b/drivers/mtd/nand/raw/nand_base.c
> index 72f3a89..a7c2507 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -5086,6 +5086,34 @@ static int nand_flash_detect_ext_param_page(struct
> nand_chip *chip,
> return ret;
> }
>
> +#define GET_BIT(bit, val) (((val) >> (bit)) & 0x01)
Not sure we need that macro, see below.
> +
> +/*
> + * Recover data with bit-wise majority
> + */
> +static void nand_bit_wise_majority(const void **srcbufs,
> +void *dstbuf,
> +unsigned int nbufs,
> +unsigned int bufsize)
I'd prefer to have nbufs just after srcbufs and named nsrcbufs:
static void nand_bit_wise_majority(const void **srcbufs,
unsigned int nsrcbufs,
void *dstbuf,
unsigned int bufsize)
> +{
> + int i, j, k;
> + u8 v, m;
> + u8 *p;
> +
> + p = *(u8 **)srcbufs;
Nope, I'd like to support the cases where srcbufs are not contiguous,
so that does not work.
> + for (i = 0; i < bufsize; i++) {
> + v = 0;
You can declare the v variable here, since its scope is limited to the
for loop. BTW, v, m, can't we pick better names? I guess v is for val,
but I'm not even sure what m stands for.
> + for (j = 0; j < 8; j++) {
> + m = 0;
> + for (k = 0; k < nbufs; k++)
> + m += GET_BIT(j, p[k*bufsize + i]);
for (k = 0; k < nbufs; k++) {
const u8 *srcbuf = srcbufs[j];
if (srcbuf[i] & BIT(k))
m++;
}
> + if (m > nbufs/2)
Space between operands and operators please
if (m > nbufs / 2)
> + v |= BIT(j);
> + }
> + ((u8 *)dstbuf)[i] = v;
> + }
> +}
> +
> /*
> * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
> */
> @@ -5102,7 +5130,7 @@ static int nand_flash_detect_onfi(struct nand_chip
> *chip)
> return 0;
>
> /* ONFI chip: allocate a buffer to hold its parameter page */
> - p = kzalloc(sizeof(*p), GFP_KERNEL);
> + p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> if (!p)
> return -ENOMEM;
>
> @@ -5113,21 +5141,29 @@ static int nand_flash_detect_onfi(struct nand_chip
> *chip)
> }
>
> for (i = 0; i < 3; i++) {
> - ret = nand_read_data_op(chip, p, sizeof(*p), true);
> + ret = nand_read_data_op(chip, &p[i], sizeof(*p), true);
> if (ret) {
> ret = 0;
> goto free_onfi_param_page;
> }
>
> - if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) ==
> le16_to_cpu(p->crc)) {
> + if (i)
> + memcpy(p, &p[i], sizeof(*p));
> break;
> }
> }
>
> if (i == 3) {
const void *srcbufs[3] = {p, p + 1, p + 2};
> - pr_err("Could not find valid ONFI parameter page; aborting\n");
> - goto free_onfi_param_page;
> + pr_err("Could not find valid ONFI parameter page\n");
> + pr_info("Recover ONFI params with bit-wise majority\n");
> + nand_bit_wise_majority((const void **)&p, p, 3, sizeof(*p));
nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
sizeof(*p))
> + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) !=
> + le16_to_cpu(p->crc)) {
> + pr_err("ONFI parameter recovery failed, aborting\n");
> + goto free_onfi_param_page;
> + }
> }
>
> /* Check version */
Thanks,
Boris
Re: [Ksummit-discuss] bug-introducing patches
Hi Stephen, On Wed, 9 May 2018 20:47:27 +1000 Stephen Rothwell wrote: > On Wed, 9 May 2018 18:03:46 +0900 Mark Brown wrote: > > > > On Wed, May 09, 2018 at 10:47:57AM +0200, Daniel Vetter wrote: > > > On Wed, May 9, 2018 at 10:44 AM, Mark Brown wrote: > > > > > > > > > I think this is an excellent idea, copying in Stephen for his input. > > > > I'm currently on holiday but unless someone convinces me it's a terrible > > > > idea I'm willing to at least give it a go on a trial basis once I'm back > > > > home. > > > > > Since Stephen merges all -fixes branches first, before merging all the > > > -next branches, he already generates that as part of linux-next. All > > > he'd need to do is push that intermediate state out to some > > > linux-fixes branch for consumption by test bots. > > Good idea ... I will see what I can do. > > > True. It's currently only those -fixes branches that people have asked > > him to merge separately which isn't as big a proportion of trees as have > > them (perhaps fortunately given people's enthusiasm for fixes branches > > that don't merge cleanly with their development branches) so we'd also > > need to encourage people to add them separately. > > I currently have 44 such fixes branches. More welcome! I see that the nand/fixes and spi-nor/fixes branch are already there [1]. You can add: mtd-fixes git git://git.infradead.org/linux-mtd.git#master You can also remove the mtd entry [2], since mtd-2.6.git is just a sym link to linux-mtd.git, so it will just be a duplicate of the mtd-fixes entry. You can also rename the l2-mtd entry [3] into mtd. Regards, Boris [1]https://kernel.googlesource.com/pub/scm/linux/kernel/git/next/linux-next/+/43cd1f4979998ba0ef1c0b8e1c5d23d2de5ab172/Next/Trees#41 [2]https://kernel.googlesource.com/pub/scm/linux/kernel/git/next/linux-next/+/43cd1f4979998ba0ef1c0b8e1c5d23d2de5ab172/Next/Trees#155 [3]https://kernel.googlesource.com/pub/scm/linux/kernel/git/next/linux-next/+/43cd1f4979998ba0ef1c0b8e1c5d23d2de5ab172/Next/Trees#156
Re: [PATCH 16/18] mtd: rawnand.h: use nested union kernel-doc markups
On Mon, 7 May 2018 06:35:52 -0300
Mauro Carvalho Chehab wrote:
> Gets rid of those warnings and better document the parameters.
>
> ./include/linux/mtd/rawnand.h:752: warning: Function parameter or member
> 'timings.sdr' not described in 'nand_data_interface'
> ./include/linux/mtd/rawnand.h:817: warning: Function parameter or member
> 'buf' not described in 'nand_op_data_instr'
> ./include/linux/mtd/rawnand.h:817: warning: Function parameter or member
> 'buf.in' not described in 'nand_op_data_instr'
> ./include/linux/mtd/rawnand.h:817: warning: Function parameter or member
> 'buf.out' not described in 'nand_op_data_instr'
> ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member
> 'ctx' not described in 'nand_op_instr'
> ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member
> 'ctx.cmd' not described in 'nand_op_instr'
> ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member
> 'ctx.addr' not described in 'nand_op_instr'
> ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member
> 'ctx.data' not described in 'nand_op_instr'
> ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member
> 'ctx.waitrdy' not described in 'nand_op_instr'
> ./include/linux/mtd/rawnand.h:1010: warning: Function parameter or member
> 'ctx' not described in 'nand_op_parser_pattern_elem'
> ./include/linux/mtd/rawnand.h:1010: warning: Function parameter or member
> 'ctx.addr' not described in 'nand_op_parser_pattern_elem'
> ./include/linux/mtd/rawnand.h:1010: warning: Function parameter or member
> 'ctx.data' not described in 'nand_op_parser_pattern_elem'
> ./include/linux/mtd/rawnand.h:1313: warning: Function parameter or member
> 'manufacturer.desc' not described in 'nand_chip'
> ./include/linux/mtd/rawnand.h:1313: warning: Function parameter or member
> 'manufacturer.priv' not described in 'nand_chip'
>
> ./include/linux/mtd/rawnand.h:848: WARNING: Unexpected indentation.
>
> Signed-off-by: Mauro Carvalho Chehab
Applied.
Thanks,
Boris
> ---
> include/linux/mtd/rawnand.h | 26 ++
> 1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index 5dad59b31244..b986f94906df 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -740,8 +740,9 @@ enum nand_data_interface_type {
>
> /**
> * struct nand_data_interface - NAND interface timing
> - * @type:type of the timing
> - * @timings: The timing, type according to @type
> + * @type: type of the timing
> + * @timings: The timing, type according to @type
> + * @timings.sdr: Use it when @type is %NAND_SDR_IFACE.
> */
> struct nand_data_interface {
> enum nand_data_interface_type type;
> @@ -798,8 +799,9 @@ struct nand_op_addr_instr {
> /**
> * struct nand_op_data_instr - Definition of a data instruction
> * @len: number of data bytes to move
> - * @in: buffer to fill when reading from the NAND chip
> - * @out: buffer to read from when writing to the NAND chip
> + * @buf: buffer to fill
> + * @buf.in: buffer to fill when reading from the NAND chip
> + * @buf.out: buffer to read from when writing to the NAND chip
> * @force_8bit: force 8-bit access
> *
> * Please note that "in" and "out" are inverted from the ONFI specification
> @@ -842,9 +844,13 @@ enum nand_op_instr_type {
> /**
> * struct nand_op_instr - Instruction object
> * @type: the instruction type
> - * @cmd/@addr/@data/@waitrdy: extra data associated to the instruction.
> - *You'll have to use the appropriate element
> - *depending on @type
> + * @ctx: extra data associated to the instruction. You'll have to use the
> + *appropriate element depending on @type
> + * @ctx.cmd: use it if @type is %NAND_OP_CMD_INSTR
> + * @ctx.addr: use it if @type is %NAND_OP_ADDR_INSTR
> + * @ctx.data: use it if @type is %NAND_OP_DATA_IN_INSTR
> + * or %NAND_OP_DATA_OUT_INSTR
> + * @ctx.waitrdy: use it if @type is %NAND_OP_WAITRDY_INSTR
> * @delay_ns: delay the controller should apply after the instruction has
> been
> * issued on the bus. Most modern controllers have internal timings
> * control logic, and in this case, the controller driver can ignore
> @@ -997,7 +1003,9 @@ struct nand_op_parser_data_constraints {
> * struct nand_op_parser_pattern_elem - One element of a pattern
> * @type: the instructuction type
> * @optional: whether this element of the pattern is optional or mandatory
> - * @addr/@data: address or data constraint (number of cycles or data length)
> + * @ctx: address or data constraint
> + * @ctx.addr: address constraint (number of cycles)
> + * @ctx.data: data constraint (data length)
> */
> struct nand_op_parser_pattern_elem {
> enum nand_op_instr_type type;
> @@ -1224,6 +1232,8 @@ int nand_op_parser_exec_op(struct nand_chi
Re: [PATCH v4 1/2] mtd: rawnand: fsl_ifc: fix FSL NAND driver to read all ONFI parameter pages
On Tue, 8 May 2018 14:19:53 -0700
Jane Wan wrote:
> Per ONFI specification (Rev. 4.0), if the CRC of the first parameter page
> read is not valid, the host should read redundant parameter page copies.
> Fix FSL NAND driver to read the two redundant copies which are mandatory
> in the specification.
>
> Signed-off-by: Jane Wan
Applied.
Thanks,
Boris
> ---
> drivers/mtd/nand/raw/fsl_ifc_nand.c | 17 ++---
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/fsl_ifc_nand.c
> b/drivers/mtd/nand/raw/fsl_ifc_nand.c
> index 61aae02..98aac1f 100644
> --- a/drivers/mtd/nand/raw/fsl_ifc_nand.c
> +++ b/drivers/mtd/nand/raw/fsl_ifc_nand.c
> @@ -342,9 +342,16 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd,
> unsigned int command,
>
> case NAND_CMD_READID:
> case NAND_CMD_PARAM: {
> + /*
> + * For READID, read 8 bytes that are currently used.
> + * For PARAM, read all 3 copies of 256-bytes pages.
> + */
> + int len = 8;
> int timing = IFC_FIR_OP_RB;
> - if (command == NAND_CMD_PARAM)
> + if (command == NAND_CMD_PARAM) {
> timing = IFC_FIR_OP_RBCD;
> + len = 256 * 3;
> + }
>
> ifc_out32((IFC_FIR_OP_CW0 << IFC_NAND_FIR0_OP0_SHIFT) |
> (IFC_FIR_OP_UA << IFC_NAND_FIR0_OP1_SHIFT) |
> @@ -354,12 +361,8 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd,
> unsigned int command,
> &ifc->ifc_nand.nand_fcr0);
> ifc_out32(column, &ifc->ifc_nand.row3);
>
> - /*
> - * although currently it's 8 bytes for READID, we always read
> - * the maximum 256 bytes(for PARAM)
> - */
> - ifc_out32(256, &ifc->ifc_nand.nand_fbcr);
> - ifc_nand_ctrl->read_bytes = 256;
> + ifc_out32(len, &ifc->ifc_nand.nand_fbcr);
> + ifc_nand_ctrl->read_bytes = len;
>
> set_addr(mtd, 0, 0, 0);
> fsl_ifc_run_command(mtd);
Re: [PATCH v2] mtd: nxp-spifi: release flash_np in nxp_spifi_probe()
On Wed, 9 May 2018 17:56:46 +0300
Alexey Khoroshilov wrote:
> nxp_spifi_probe() increments refcnt of SPI flash device node by
> of_get_next_available_child() and leaves it undecremented on both
> successful and error paths.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Alexey Khoroshilov
> ---
> drivers/mtd/spi-nor/nxp-spifi.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/nxp-spifi.c b/drivers/mtd/spi-nor/nxp-spifi.c
> index 15374216d4d9..7b047951d0a2 100644
> --- a/drivers/mtd/spi-nor/nxp-spifi.c
> +++ b/drivers/mtd/spi-nor/nxp-spifi.c
> @@ -438,11 +438,15 @@ static int nxp_spifi_probe(struct platform_device *pdev)
> ret = nxp_spifi_setup_flash(spifi, flash_np);
Just put the of_node_put() here and that's the only change you'll need.
> if (ret) {
> dev_err(&pdev->dev, "unable to setup flash chip\n");
> - goto dis_clks;
> + goto put_np;
> }
>
> + of_node_put(flash_np);
> +
> return 0;
>
> +put_np:
> + of_node_put(flash_np);
> dis_clks:
> clk_disable_unprepare(spifi->clk_spifi);
> dis_clk_reg:
Re: [PATCH] mtd: nxp-spifi: decrement flash_np refcnt on error paths
On Wed, 9 May 2018 17:35:41 +0300 Alexey Khoroshilov wrote: > On 09.05.2018 12:42, Boris Brezillon wrote: > > On Tue, 8 May 2018 23:47:36 +0300 > > Alexey Khoroshilov wrote: > > > >> nxp_spifi_probe() increments refcnt of SPI flash device node by > >> of_get_next_available_child() and then it passes the node > >> to mtd device in nxp_spifi_setup_flash(). > >> But if a failure happens before mtd_device_register() succeed, > >> the refcnt is left undecremented. > > > > Why not doing that in the error path of the probe function? Also, you > > probably want to call of_node_put() in the ->remove() function. > > > > > You are right. > > I believed that after successful mtd_device_register() > the node is managed by mtd device. I missed that it calls of_node_get() > in add_mtd_device() by itself. > > I will prepare v2. > But I guess there is no need to have of_node_put() in ->remove(), since > probe() finishes its own usage of flash_np, while mtd_device incremented > refcnt by itself and will decrement it in ->remove() in > mtd_device_unregister(&spifi->nor.mtd). So, I would propose > of_node_put() on both successful and error path. Sounds good.
Re: [PATCH v4 2/2] mtd: rawnand: use bit-wise majority to recover the contents of ONFI parameter
On Tue, 8 May 2018 14:19:54 -0700
Jane Wan wrote:
> Per ONFI specification (Rev. 4.0), if all parameter pages have invalid
> CRC values, the bit-wise majority may be used to recover the contents of
> the parameter pages from the parameter page copies present.
>
> Signed-off-by: Jane Wan
> ---
> drivers/mtd/nand/raw/nand_base.c | 49
> ++
> 1 file changed, 44 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/nand_base.c
> b/drivers/mtd/nand/raw/nand_base.c
> index 72f3a89..dfc341c 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -5086,6 +5086,38 @@ static int nand_flash_detect_ext_param_page(struct
> nand_chip *chip,
> return ret;
> }
>
> +#define GET_BIT(bit, val) (((val) >> (bit)) & 0x01)
> +
> +/*
> + * Recover NAND parameter page with bit-wise majority
> + */
> +static int onfi_recover_param(struct nand_onfi_params *p, int pages)
I had something more generic in mind:
static void nand_bit_wise_majority(const void **srcbufs,
void *dstbuf,
unsigned int nbufs,
unsigned int bufsize)
{
...
}
And then you do the crc check in nand_flash_detect_onfi().
The reason I'm asking that is because I'm almost sure we'll re-use this
functions for extended param pages, and also for vendor specific data
(we already have a byte-wise majority check in the hynix driver, so I
wouldn't be surprised if other vendors decided to use a bit-wise
approach for some of their OTP area).
> +{
> + int i, j, k;
> + u8 v, m;
> + u8 *buf;
> +
> + buf = (u8 *)p;
> + for (i = 0; i < sizeof(*p); i++) {
> + v = 0;
> + for (j = 0; j < 8; j++) {
> + m = 0;
> + for (k = 0; k < pages; k++)
> + m += GET_BIT(j, buf[k*sizeof(*p) + i]);
> + if (m > pages/2)
> + v |= BIT(j);
> + }
> + ((u8 *)p)[i] = v;
> + }
> +
> + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) ==
> + le16_to_cpu(p->crc)) {
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> /*
> * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
> */
> @@ -5102,7 +5134,7 @@ static int nand_flash_detect_onfi(struct nand_chip
> *chip)
> return 0;
>
> /* ONFI chip: allocate a buffer to hold its parameter page */
> - p = kzalloc(sizeof(*p), GFP_KERNEL);
> + p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> if (!p)
> return -ENOMEM;
>
> @@ -5113,21 +5145,28 @@ static int nand_flash_detect_onfi(struct nand_chip
> *chip)
> }
>
> for (i = 0; i < 3; i++) {
> - ret = nand_read_data_op(chip, p, sizeof(*p), true);
> + ret = nand_read_data_op(chip, &p[i], sizeof(*p), true);
> if (ret) {
> ret = 0;
> goto free_onfi_param_page;
> }
>
> - if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) ==
> le16_to_cpu(p->crc)) {
> + if (i)
> + memcpy(p, &p[i], sizeof(*p));
> break;
> }
> }
>
> if (i == 3) {
> - pr_err("Could not find valid ONFI parameter page; aborting\n");
> - goto free_onfi_param_page;
> + pr_err("Could not find valid ONFI parameter page\n");
> + pr_info("Recover ONFI params with bit-wise majority\n");
> + ret = onfi_recover_param(p, 3);
> + if (ret == 0) {
> + pr_err("ONFI parameter recovery failed, aborting\n");
> + goto free_onfi_param_page;
> + }
> }
>
> /* Check version */
Re: [PATCH v4 1/2] mtd: rawnand: fsl_ifc: fix FSL NAND driver to read all ONFI parameter pages
On Tue, 8 May 2018 14:19:53 -0700
Jane Wan wrote:
> Per ONFI specification (Rev. 4.0), if the CRC of the first parameter page
> read is not valid, the host should read redundant parameter page copies.
> Fix FSL NAND driver to read the two redundant copies which are mandatory
> in the specification.
>
> Signed-off-by: Jane Wan
I'm gonna take this patch, but I'd like to make it clear: this is the
last time I accept fixes touching fsl_ifc_cmdfunc() for bugs that could
have been addressed by switching to ->exec_op() (note that I had a look
at a freescale datasheet, and I'm now 100% sure this driver can be
converted to ->exec_op()).
> ---
> drivers/mtd/nand/raw/fsl_ifc_nand.c | 17 ++---
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/fsl_ifc_nand.c
> b/drivers/mtd/nand/raw/fsl_ifc_nand.c
> index 61aae02..98aac1f 100644
> --- a/drivers/mtd/nand/raw/fsl_ifc_nand.c
> +++ b/drivers/mtd/nand/raw/fsl_ifc_nand.c
> @@ -342,9 +342,16 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd,
> unsigned int command,
>
> case NAND_CMD_READID:
> case NAND_CMD_PARAM: {
> + /*
> + * For READID, read 8 bytes that are currently used.
> + * For PARAM, read all 3 copies of 256-bytes pages.
> + */
> + int len = 8;
> int timing = IFC_FIR_OP_RB;
> - if (command == NAND_CMD_PARAM)
> + if (command == NAND_CMD_PARAM) {
> timing = IFC_FIR_OP_RBCD;
> + len = 256 * 3;
> + }
>
> ifc_out32((IFC_FIR_OP_CW0 << IFC_NAND_FIR0_OP0_SHIFT) |
> (IFC_FIR_OP_UA << IFC_NAND_FIR0_OP1_SHIFT) |
> @@ -354,12 +361,8 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd,
> unsigned int command,
> &ifc->ifc_nand.nand_fcr0);
> ifc_out32(column, &ifc->ifc_nand.row3);
>
> - /*
> - * although currently it's 8 bytes for READID, we always read
> - * the maximum 256 bytes(for PARAM)
> - */
> - ifc_out32(256, &ifc->ifc_nand.nand_fbcr);
> - ifc_nand_ctrl->read_bytes = 256;
> + ifc_out32(len, &ifc->ifc_nand.nand_fbcr);
> + ifc_nand_ctrl->read_bytes = len;
>
> set_addr(mtd, 0, 0, 0);
> fsl_ifc_run_command(mtd);
Re: [PATCH/RFC] mtd: spi-nor: honour max_message_size for spi-nor writes.
On Fri, 27 Apr 2018 16:18:05 +1000
NeilBrown wrote:
> Hi,
> I've labeled this an RFC because I'm really not sure about removing the
> error path from spi_nor_write() -- maybe that really matters. But on
> my hardware, performing multiple small spi writes to the flash seems
> to work.
>
> The spi driver is drivers/staging/mt7621-spi. Possibly this needs to
> use DMA instead of a FIFO (assuming the hardware can) - or maybe
> drivers/spi/spi-mt65xx.c can be made to work on this hardware, though
> that is for an ARM SOC and mt7621 is a MIPS SOC.
>
> I note that openwrt has similar patches:
>
> target/linux/generic/pending-4.14/450-mtd-spi-nor-allow-NOR-driver-to-write-fewer-bytes-th.patch
>
> They also change the spi driver to do a short write, rather
> than change m25p80 to request a short write.
>
> Is there something horribly wrong with this?
Marek, any opinion on this patch?
>
> Thanks,
> NeilBrown
>
> ---8<
>
> m25p80 honours max_message_size and max_transfer_size
> for reads, but not for writes.
> I have a driver that has a max message size of 36 bytes
> (command, address, 32 bytes data, all places in a FIFO
> in the controller).
> This requires m25p80_write() to honour the size limits.
> For that to work, spi-nor needs to quietly accept partial
> writes.
>
> With this, I can successfully re-flash my device.
>
> Signed-off-by: NeilBrown
> ---
> drivers/mtd/devices/m25p80.c | 3 ++-
> drivers/mtd/spi-nor/spi-nor.c | 7 ---
> 2 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index a4e18f6aaa33..7ded13507604 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -117,7 +117,8 @@ static ssize_t m25p80_write(struct spi_nor *nor, loff_t
> to, size_t len,
>
> t[data_idx].tx_buf = buf;
> t[data_idx].tx_nbits = data_nbits;
> - t[data_idx].len = len;
> + t[data_idx].len = min3(len, spi_max_transfer_size(spi),
> +spi_max_message_size(spi) - cmd_sz);
> spi_message_add_tail(&t[data_idx], &m);
>
> ret = spi_sync(spi, &m);
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 42ae9a1529bb..cfa15f2801ad 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1445,13 +1445,6 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t
> to, size_t len,
> goto write_err;
> *retlen += written;
> i += written;
> - if (written != page_remain) {
> - dev_err(nor->dev,
> - "While writing %zu bytes written %zd bytes\n",
> - page_remain, written);
> - ret = -EIO;
> - goto write_err;
> - }
> }
>
> write_err:
Re: [PATCH 16/18] mtd: rawnand.h: use nested union kernel-doc markups
On Wed, 9 May 2018 09:10:34 -0300 Mauro Carvalho Chehab wrote: > Hi Boris, > > Em Mon, 7 May 2018 08:32:32 -0300 > Mauro Carvalho Chehab escreveu: > > > Hi Boris, > > > > Em Mon, 7 May 2018 11:46:50 +0200 > > Boris Brezillon escreveu: > > > > Is there a simple > > > way we can get rid of the warning we have when not describing timings > > > but all of its fields? > > > > There's no direct way. It won't be hard to add a way to do it, > > like applying the enclosed patch with ends by declaring timings as: > > > > * @timings: -- undescribed -- > > > > IMHO, this is uglier. > > Didn't hear from you. I replied just a few minutes ago ;). > What do you prefer for me to do with regards to > this patch? Will queue the initial patch to nand/next (I can also queue it to the fixes branch if you prefer).
Re: [PATCH 16/18] mtd: rawnand.h: use nested union kernel-doc markups
On Mon, 7 May 2018 08:32:32 -0300
Mauro Carvalho Chehab wrote:
> Hi Boris,
>
> Em Mon, 7 May 2018 11:46:50 +0200
> Boris Brezillon escreveu:
>
> > Hi Mauro,
>
> > > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> > > index 5dad59b31244..b986f94906df 100644
> > > --- a/include/linux/mtd/rawnand.h
> > > +++ b/include/linux/mtd/rawnand.h
> > > @@ -740,8 +740,9 @@ enum nand_data_interface_type {
> > >
> > > /**
> > > * struct nand_data_interface - NAND interface timing
> > > - * @type:type of the timing
> > > - * @timings: The timing, type according to @type
> > > + * @type: type of the timing
> > > + * @timings: The timing, type according to @type
> > > + * @timings.sdr: Use it when @type is %NAND_SDR_IFACE.
> >
> > Hm, really feels weird to do that. I mean, either we describe
> > timings.sdr or timings, but not both. I this case, I agree that
> > describing timings.sdr would make more sense than generically
> > describing any possible entries in the timings union.
>
> This struct is funny, as the union has just one item. I assume
> that the original intend is to be ready to have more timing
> types (or you had it in the past). By the time you have a
> second value there, describing the union would make more
> sense.
>
> > Is there a simple
> > way we can get rid of the warning we have when not describing timings
> > but all of its fields?
>
> There's no direct way. It won't be hard to add a way to do it,
> like applying the enclosed patch with ends by declaring timings as:
>
> * @timings: -- undescribed --
>
> IMHO, this is uglier.
Yep, don't like it either. I'll just take your initial patch.
Thanks,
Boris
Re: [PATCH] mtd: nxp-spifi: decrement flash_np refcnt on error paths
On Tue, 8 May 2018 23:47:36 +0300
Alexey Khoroshilov wrote:
> nxp_spifi_probe() increments refcnt of SPI flash device node by
> of_get_next_available_child() and then it passes the node
> to mtd device in nxp_spifi_setup_flash().
> But if a failure happens before mtd_device_register() succeed,
> the refcnt is left undecremented.
Why not doing that in the error path of the probe function? Also, you
probably want to call of_node_put() in the ->remove() function.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Alexey Khoroshilov
> ---
> drivers/mtd/spi-nor/nxp-spifi.c | 16 +++-
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/nxp-spifi.c b/drivers/mtd/spi-nor/nxp-spifi.c
> index 15374216d4d9..8919e31f2ab8 100644
> --- a/drivers/mtd/spi-nor/nxp-spifi.c
> +++ b/drivers/mtd/spi-nor/nxp-spifi.c
> @@ -294,7 +294,8 @@ static int nxp_spifi_setup_flash(struct nxp_spifi *spifi,
> break;
> default:
> dev_err(spifi->dev, "unsupported rx-bus-width\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto err_node_put;
> }
> }
>
> @@ -328,7 +329,8 @@ static int nxp_spifi_setup_flash(struct nxp_spifi *spifi,
> break;
> default:
> dev_err(spifi->dev, "only mode 0 and 3 supported\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto err_node_put;
> }
>
> writel(ctrl, spifi->io_base + SPIFI_CTRL);
> @@ -356,22 +358,26 @@ static int nxp_spifi_setup_flash(struct nxp_spifi
> *spifi,
> ret = spi_nor_scan(&spifi->nor, NULL, &hwcaps);
> if (ret) {
> dev_err(spifi->dev, "device scan failed\n");
> - return ret;
> + goto err_node_put;
> }
>
> ret = nxp_spifi_setup_memory_cmd(spifi);
> if (ret) {
> dev_err(spifi->dev, "memory command setup failed\n");
> - return ret;
> + goto err_node_put;
> }
>
> ret = mtd_device_register(&spifi->nor.mtd, NULL, 0);
> if (ret) {
> dev_err(spifi->dev, "mtd device parse failed\n");
> - return ret;
> + goto err_node_put;
> }
>
> return 0;
> +
> +err_node_put:
> + of_node_put(np);
> + return ret;
> }
>
> static int nxp_spifi_probe(struct platform_device *pdev)
Re: [PATCH v3 2/2] mtd: rawnand: fsl_ifc: use bit-wise majority to recover the contents of ONFI parameter
On Mon, 7 May 2018 09:34:15 -0700
Jane Wan wrote:
> Per ONFI specification (Rev. 4.0), if all parameter pages have invalid
> CRC values, the bit-wise majority may be used to recover the contents of
> the parameter pages from the parameter page copies present.
>
> Signed-off-by: Jane Wan
I never received patch 1 of this series. When you fix something in a
commit, please resend the whole patchset, even if other patches haven't
changed.
> ---
> drivers/mtd/nand/raw/nand_base.c | 41
> ++
> 1 file changed, 33 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/nand_base.c
> b/drivers/mtd/nand/raw/nand_base.c
> index 72f3a89..48f2dec 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -5086,15 +5086,18 @@ static int nand_flash_detect_ext_param_page(struct
> nand_chip *chip,
> return ret;
> }
>
> +#define GET_BIT(bit, val) (((val) >> (bit)) & 0x01)
> +
> /*
> * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
> */
> static int nand_flash_detect_onfi(struct nand_chip *chip)
> {
> struct mtd_info *mtd = nand_to_mtd(chip);
> - struct nand_onfi_params *p;
> + struct nand_onfi_params *p = NULL;
> char id[4];
> - int i, ret, val;
> + int i, ret, val, pagesize;
> + u8 *buf = NULL;
>
> /* Try ONFI for unknown chip or LP */
> ret = nand_readid_op(chip, 0x20, id, sizeof(id));
> @@ -5102,8 +5105,9 @@ static int nand_flash_detect_onfi(struct nand_chip
> *chip)
> return 0;
>
> /* ONFI chip: allocate a buffer to hold its parameter page */
> - p = kzalloc(sizeof(*p), GFP_KERNEL);
> - if (!p)
> + pagesize = sizeof(*p);
> + buf = kzalloc((pagesize * 3), GFP_KERNEL);
Not sure why you have to add a new buf variable here, and pagesize is
not needed either, just use sizeof(*p) directly.
> + if (!buf)
> return -ENOMEM;
>
> ret = nand_read_param_page_op(chip, 0, NULL, 0);
> @@ -5113,7 +5117,8 @@ static int nand_flash_detect_onfi(struct nand_chip
> *chip)
> }
>
> for (i = 0; i < 3; i++) {
> - ret = nand_read_data_op(chip, p, sizeof(*p), true);
> + p = (struct nand_onfi_params *)&buf[i*pagesize];
> + ret = nand_read_data_op(chip, p, pagesize, true);
> if (ret) {
> ret = 0;
> goto free_onfi_param_page;
> @@ -5126,8 +5131,27 @@ static int nand_flash_detect_onfi(struct nand_chip
> *chip)
> }
>
> if (i == 3) {
> - pr_err("Could not find valid ONFI parameter page; aborting\n");
> - goto free_onfi_param_page;
> + int j, k, l;
> + u8 v, m;
> +
> + pr_err("Could not find valid ONFI parameter page\n");
> + pr_info("Recover ONFI params with bit-wise majority\n");
> + for (j = 0; j < pagesize; j++) {
> + v = 0;
> + for (k = 0; k < 8; k++) {
> + m = 0;
> + for (l = 0; l < 3; l++)
> + m += GET_BIT(k, buf[l*pagesize + j]);
> + if (m > 1)
> + v |= BIT(k);
> + }
> + ((u8 *)p)[j] = v;
> + }
Can you move the bit-wise majority code in a separate function?
> + if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) !=
> + le16_to_cpu(p->crc)) {
> + pr_err("ONFI parameter recovery failed, aborting\n");
> + goto free_onfi_param_page;
> + }
> }
>
> /* Check version */
> @@ -5220,7 +5244,8 @@ static int nand_flash_detect_onfi(struct nand_chip
> *chip)
> sizeof(p->vendor));
>
> free_onfi_param_page:
> - kfree(p);
> + if (buf != NULL)
> + kfree(buf);
kfree() already handles the buf == NULL case, no need to check it here.
> return ret;
> }
>
Re: [PATCH 16/18] mtd: rawnand.h: use nested union kernel-doc markups
Hi Mauro,
On Mon, 7 May 2018 06:35:52 -0300
Mauro Carvalho Chehab wrote:
> Gets rid of those warnings and better document the parameters.
>
> ./include/linux/mtd/rawnand.h:752: warning: Function parameter or member
> 'timings.sdr' not described in 'nand_data_interface'
> ./include/linux/mtd/rawnand.h:817: warning: Function parameter or member
> 'buf' not described in 'nand_op_data_instr'
> ./include/linux/mtd/rawnand.h:817: warning: Function parameter or member
> 'buf.in' not described in 'nand_op_data_instr'
> ./include/linux/mtd/rawnand.h:817: warning: Function parameter or member
> 'buf.out' not described in 'nand_op_data_instr'
> ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member
> 'ctx' not described in 'nand_op_instr'
> ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member
> 'ctx.cmd' not described in 'nand_op_instr'
> ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member
> 'ctx.addr' not described in 'nand_op_instr'
> ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member
> 'ctx.data' not described in 'nand_op_instr'
> ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member
> 'ctx.waitrdy' not described in 'nand_op_instr'
> ./include/linux/mtd/rawnand.h:1010: warning: Function parameter or member
> 'ctx' not described in 'nand_op_parser_pattern_elem'
> ./include/linux/mtd/rawnand.h:1010: warning: Function parameter or member
> 'ctx.addr' not described in 'nand_op_parser_pattern_elem'
> ./include/linux/mtd/rawnand.h:1010: warning: Function parameter or member
> 'ctx.data' not described in 'nand_op_parser_pattern_elem'
> ./include/linux/mtd/rawnand.h:1313: warning: Function parameter or member
> 'manufacturer.desc' not described in 'nand_chip'
> ./include/linux/mtd/rawnand.h:1313: warning: Function parameter or member
> 'manufacturer.priv' not described in 'nand_chip'
>
> ./include/linux/mtd/rawnand.h:848: WARNING: Unexpected indentation.
>
> Signed-off-by: Mauro Carvalho Chehab
> ---
> include/linux/mtd/rawnand.h | 26 ++
> 1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index 5dad59b31244..b986f94906df 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -740,8 +740,9 @@ enum nand_data_interface_type {
>
> /**
> * struct nand_data_interface - NAND interface timing
> - * @type:type of the timing
> - * @timings: The timing, type according to @type
> + * @type: type of the timing
> + * @timings: The timing, type according to @type
> + * @timings.sdr: Use it when @type is %NAND_SDR_IFACE.
Hm, really feels weird to do that. I mean, either we describe
timings.sdr or timings, but not both. I this case, I agree that
describing timings.sdr would make more sense than generically
describing any possible entries in the timings union. Is there a simple
way we can get rid of the warning we have when not describing timings
but all of its fields?
> */
> struct nand_data_interface {
> enum nand_data_interface_type type;
> @@ -798,8 +799,9 @@ struct nand_op_addr_instr {
> /**
> * struct nand_op_data_instr - Definition of a data instruction
> * @len: number of data bytes to move
> - * @in: buffer to fill when reading from the NAND chip
> - * @out: buffer to read from when writing to the NAND chip
> + * @buf: buffer to fill
> + * @buf.in: buffer to fill when reading from the NAND chip
> + * @buf.out: buffer to read from when writing to the NAND chip
Same here. What we care about is @buf.in and @buf.out, the @buf
description is useless...
Regards,
Boris
Re: [PATCH v2 03/14] dt-bindings: qcom_nandc: make nand-ecc-strength optional
On Thu, 3 May 2018 17:50:30 +0530 Abhishek Sahu wrote: > Now, nand-ecc-strength is optional. If specified in DT, then > controller will use this ECC strength otherwise ECC strength will > be calculated according to chip requirement and available OOB size. > > Signed-off-by: Abhishek Sahu > --- > * Changes from v1: > > NEW PATCH > > Documentation/devicetree/bindings/mtd/qcom_nandc.txt | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt > b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt > index 73d336be..f246aa0 100644 > --- a/Documentation/devicetree/bindings/mtd/qcom_nandc.txt > +++ b/Documentation/devicetree/bindings/mtd/qcom_nandc.txt > @@ -45,11 +45,13 @@ Required properties: > number (e.g., 0, 1, 2, etc.) > - #address-cells:see partition.txt > - #size-cells: see partition.txt > -- nand-ecc-strength: see nand.txt > - nand-ecc-step-size:must be 512. see nand.txt for more details. As mentioned in my other review, no need to specify nand-ecc-step-size if you don't have a choice. You can remove the property and say that nand-ecc-strength encodes the ECC strength for 512 bytes chunks. > > Optional properties: > - nand-bus-width:see nand.txt > +- nand-ecc-strength: see nand.txt. If not specified, then ECC strength will > + be used according to chip requirement and available > + OOB size. > > Each nandcs device node may optionally contain a 'partitions' sub-node, which > further contains sub-nodes describing the flash partition mapping. See
Re: [PATCH] spi-nor: Add support for Atmel Dataflash memories
Hi Nicolas, On Mon, 7 May 2018 10:23:56 +0200 Nicolas Ferre wrote: > On 04/05/2018 at 20:38, Boris Brezillon wrote: > > Hi Radu, > > > > Sorry for the late reply. > > > > On Wed, 28 Feb 2018 13:55:01 +0200 > > Radu Pirea wrote: > > > >> This patch add support in spi-nor for allmost all dataflash memories > >> supported by old mtd_dataflash driver. > > > > Those devices clearly use a different instruction set, so I don't think > > they fit in this framework. Can you tell us why you want to move > > dataflash support to the SPI NOR framework. I think I know why, but I'd > > like to get your version. My guess is that some people want to connect > > dataflash chips to the Atmel QSPI controller, and it's not supported > > right now because the Atmel QSPI controller implements the SPI-NOR > > interface and not the generic SPI one, thus preventing anything that > > is not a SPI NOR from being connected to this controller. > > > > If I'm right, then the solution is to convert the QSPI driver to the > > spi-mem interface [1] and move it to drivers/spi/. > > No, I we didn't think about this. Dataflash is not so popular those days > and we don't want to revive it anyway. Our QSPI driver has already a lot > of things to handle in QSPI-related topics to not mix it with oldies ;-) > > The rationale behind this work is to get rid of the very old dataflash > standalone driver and benefit from the whole spi-nor infrastructure like > cache coherency management and DMA handling (which were broken in the > old dataflash driver in recent kernels). Still don't think that's a good move, especially since those flashes are using a completely different instruction set and are not exactly behaving like SPI NORs. If we need some of the spi-nor logic to help handle dataflash chips in a more efficient/safe way, then those bits should be exposed as helpers at the MTD level instead of turning spi-nor into a Frankenstein framework. Regards, Boris
Re: [PATCH v2 04/14] mtd: rawnand: qcom: use the ecc strength from device parameter
On Thu, 3 May 2018 17:50:31 +0530
Abhishek Sahu wrote:
> Currently the driver uses the ECC strength specified in DT.
> The QPIC/EBI2 NAND supports 4 or 8-bit ECC correction. The same
> kind of board can have different NAND parts so use the ECC
> strength from device parameters if it is not specified in DT.
>
> Signed-off-by: Abhishek Sahu
> ---
> * Changes from v1:
>
> 1. Removed the custom logic and used the helper fuction.
>
> drivers/mtd/nand/raw/qcom_nandc.c | 31 ++-
> 1 file changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c
> b/drivers/mtd/nand/raw/qcom_nandc.c
> index b554fb6..a8d71ce 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -2315,13 +2315,21 @@ static int qcom_nand_ooblayout_free(struct mtd_info
> *mtd, int section,
> .free = qcom_nand_ooblayout_free,
> };
>
> +static int
> +qcom_nandc_calc_ecc_bytes(int step_size, int strength)
> +{
> + return strength == 4 ? 12 : 16;
> +}
> +NAND_ECC_CAPS_SINGLE(qcom_nandc_ecc_caps, qcom_nandc_calc_ecc_bytes,
> + NANDC_STEP_SIZE, 4, 8);
> +
> static int qcom_nand_host_setup(struct qcom_nand_host *host)
> {
> struct nand_chip *chip = &host->chip;
> struct mtd_info *mtd = nand_to_mtd(chip);
> struct nand_ecc_ctrl *ecc = &chip->ecc;
> struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> - int cwperpage, bad_block_byte;
> + int cwperpage, bad_block_byte, ret;
> bool wide_bus;
> int ecc_mode = 1;
>
> @@ -2334,8 +2342,20 @@ static int qcom_nand_host_setup(struct qcom_nand_host
> *host)
> return -EINVAL;
> }
>
> + cwperpage = mtd->writesize / ecc->size;
Looks like you're still expecting nand-ecc-step-size to be defined in
the DT, which does not really make sense since you only support one
size: NANDC_STEP_SIZE.
You should remove the
if (ecc->size != NANDC_STEP_SIZE) {
dev_err(nandc->dev, "invalid ecc size\n");
return -EINVAL;
}
block, then do:
cwperpage = mtd->writesize / NANDC_STEP_SIZE;
and finally let the nand_ecc_param_setup() function choose the best
config for you.
> +
> + /*
> + * Each CW has 4 available OOB bytes which will be protected with ECC
> + * so remaining bytes can be used for ECC.
> + */
> + ret = nand_ecc_param_setup(chip, &qcom_nandc_ecc_caps,
> +mtd->oobsize - (cwperpage << 2));
Please stop doing useless optimizations like. That brings nothing and
obfuscates the code a bit more. You say in the comment that each
codeword has 4 protected OOB bytes that can be used by the upper layer,
so just do (cwperpage * 4) and let gcc optimize that for you.
> + if (ret) {
> + dev_err(nandc->dev, "No valid ecc settings possible\n");
> + return ret;
> + }
> +
> wide_bus = chip->options & NAND_BUSWIDTH_16 ? true : false;
> -
> if (ecc->strength >= 8) {
> /* 8 bit ECC defaults to BCH ECC on all platforms */
> host->bch_enabled = true;
> @@ -2403,7 +2423,6 @@ static int qcom_nand_host_setup(struct qcom_nand_host
> *host)
>
> mtd_set_ooblayout(mtd, &qcom_nand_ooblayout_ops);
>
> - cwperpage = mtd->writesize / ecc->size;
> nandc->max_cwperpage = max_t(unsigned int, nandc->max_cwperpage,
>cwperpage);
>
> @@ -2419,12 +2438,6 @@ static int qcom_nand_host_setup(struct qcom_nand_host
> *host)
>* for 8 bit ECC
>*/
> host->cw_size = host->cw_data + ecc->bytes;
> -
> - if (ecc->bytes * (mtd->writesize / ecc->size) > mtd->oobsize) {
> - dev_err(nandc->dev, "ecc data doesn't fit in OOB area\n");
> - return -EINVAL;
> - }
> -
> bad_block_byte = mtd->writesize - host->cw_size * (cwperpage - 1) + 1;
>
> host->cfg0 = (cwperpage - 1) << CW_PER_PAGE
Re: [PATCH v2 01/14] mtd: rawnand: helper function for setting up ECC parameters
On Thu, 3 May 2018 17:50:28 +0530
Abhishek Sahu wrote:
> commit 2c8f8afa7f92 ("mtd: nand: add generic helpers to check,
> match, maximize ECC settings") provides generic helpers which
> drivers can use for setting up ECC parameters.
>
> Since same board can have different ECC strength nand chips so
> following is the logic for setting up ECC strength and ECC step
> size, which can be used by most of the drivers.
>
> 1. If both ECC step size and ECC strength are already set
>(usually by DT) then just check whether this setting
>is supported by NAND controller.
> 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength
>supported by NAND controller.
> 3. Otherwise, try to match the ECC step size and ECC strength closest
>to the chip's requirement. If available OOB size can't fit the chip
>requirement then select maximum ECC strength which can be fit with
>available OOB size with warning.
>
> This patch introduces nand_ecc_param_setup function which calls the
> required helper functions for the above logic. The drivers can use
> this single function instead of calling the 3 helper functions
> individually.
>
> CC: Masahiro Yamada
> Signed-off-by: Abhishek Sahu
> ---
> * Changes from v1:
>
> NEW PATCH
>
> drivers/mtd/nand/raw/nand_base.c | 42
>
> include/linux/mtd/rawnand.h | 3 +++
> 2 files changed, 45 insertions(+)
>
> diff --git a/drivers/mtd/nand/raw/nand_base.c
> b/drivers/mtd/nand/raw/nand_base.c
> index 72f3a89..dd7a984 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -6249,6 +6249,48 @@ int nand_maximize_ecc(struct nand_chip *chip,
> }
> EXPORT_SYMBOL_GPL(nand_maximize_ecc);
>
> +/**
> + * nand_ecc_param_setup - Set the ECC strength and ECC step size
> + * @chip: nand chip info structure
> + * @caps: ECC engine caps info structure
> + * @oobavail: OOB size that the ECC engine can use
> + *
> + * Choose the ECC strength according to following logic
> + *
> + * 1. If both ECC step size and ECC strength are already set (usually by DT)
> + *then check if it is supported by this controller.
> + * 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength.
> + * 3. Otherwise, try to match the ECC step size and ECC strength closest
> + *to the chip's requirement. If available OOB size can't fit the chip
> + *requirement then fallback to the maximum ECC step size and ECC strength
> + *and print the warning.
> + *
> + * On success, the chosen ECC settings are set.
> + */
> +int nand_ecc_param_setup(struct nand_chip *chip,
> + const struct nand_ecc_caps *caps, int oobavail)
Can we rename this function "nand_ecc_choose_conf()".
Re: [PATCH] mtd: rawnand: marvell: pass ms delay to wait_op
On Thu, 3 May 2018 14:21:28 +1200
Chris Packham wrote:
> marvell_nfc_wait_op() expects the delay to be expressed in milliseconds
> but nand_sdr_timings uses picoseconds. Use PSEC_TO_MSEC when passing
> tPROG_max to marvell_nfc_wait_op().
>
> Fixes: 02f26ecf8c772 ("mtd: nand: add reworked Marvell NAND controller
> driver")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Chris Packham
Queued to mtd/fixes.
Thanks,
Boris
> ---
> drivers/mtd/nand/raw/marvell_nand.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/marvell_nand.c
> b/drivers/mtd/nand/raw/marvell_nand.c
> index 1d779a35ac8e..e4b964fd40d8 100644
> --- a/drivers/mtd/nand/raw/marvell_nand.c
> +++ b/drivers/mtd/nand/raw/marvell_nand.c
> @@ -1074,7 +1074,7 @@ static int marvell_nfc_hw_ecc_hmg_do_write_page(struct
> nand_chip *chip,
> return ret;
>
> ret = marvell_nfc_wait_op(chip,
> - chip->data_interface.timings.sdr.tPROG_max);
> +
> PSEC_TO_MSEC(chip->data_interface.timings.sdr.tPROG_max));
> return ret;
> }
>
> @@ -1494,7 +1494,7 @@ static int marvell_nfc_hw_ecc_bch_write_page(struct
> mtd_info *mtd,
> }
>
> ret = marvell_nfc_wait_op(chip,
> - chip->data_interface.timings.sdr.tPROG_max);
> +
> PSEC_TO_MSEC(chip->data_interface.timings.sdr.tPROG_max));
>
> marvell_nfc_disable_hw_ecc(chip);
>
Re: [PATCH v2 01/14] mtd: rawnand: helper function for setting up ECC parameters
On Mon, 7 May 2018 12:40:39 +0900
Masahiro Yamada wrote:
> 2018-05-03 21:20 GMT+09:00 Abhishek Sahu :
> > commit 2c8f8afa7f92 ("mtd: nand: add generic helpers to check,
> > match, maximize ECC settings") provides generic helpers which
> > drivers can use for setting up ECC parameters.
> >
> > Since same board can have different ECC strength nand chips so
> > following is the logic for setting up ECC strength and ECC step
> > size, which can be used by most of the drivers.
> >
> > 1. If both ECC step size and ECC strength are already set
> >(usually by DT) then just check whether this setting
> >is supported by NAND controller.
> > 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength
> >supported by NAND controller.
> > 3. Otherwise, try to match the ECC step size and ECC strength closest
> >to the chip's requirement. If available OOB size can't fit the chip
> >requirement then select maximum ECC strength which can be fit with
> >available OOB size with warning.
> >
> > This patch introduces nand_ecc_param_setup function which calls the
> > required helper functions for the above logic. The drivers can use
> > this single function instead of calling the 3 helper functions
> > individually.
> >
> > CC: Masahiro Yamada
> > Signed-off-by: Abhishek Sahu
> > ---
> > * Changes from v1:
> >
> > NEW PATCH
> >
> > drivers/mtd/nand/raw/nand_base.c | 42
> >
> > include/linux/mtd/rawnand.h | 3 +++
> > 2 files changed, 45 insertions(+)
> >
> > diff --git a/drivers/mtd/nand/raw/nand_base.c
> > b/drivers/mtd/nand/raw/nand_base.c
> > index 72f3a89..dd7a984 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -6249,6 +6249,48 @@ int nand_maximize_ecc(struct nand_chip *chip,
> > }
> > EXPORT_SYMBOL_GPL(nand_maximize_ecc);
> >
> > +/**
> > + * nand_ecc_param_setup - Set the ECC strength and ECC step size
> > + * @chip: nand chip info structure
> > + * @caps: ECC engine caps info structure
> > + * @oobavail: OOB size that the ECC engine can use
> > + *
> > + * Choose the ECC strength according to following logic
> > + *
> > + * 1. If both ECC step size and ECC strength are already set (usually by
> > DT)
> > + *then check if it is supported by this controller.
> > + * 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength.
> > + * 3. Otherwise, try to match the ECC step size and ECC strength closest
> > + *to the chip's requirement. If available OOB size can't fit the chip
> > + *requirement then fallback to the maximum ECC step size and ECC
> > strength
> > + *and print the warning.
> > + *
> > + * On success, the chosen ECC settings are set.
> > + */
> > +int nand_ecc_param_setup(struct nand_chip *chip,
> > +const struct nand_ecc_caps *caps, int oobavail)
> > +{
> > + int ret;
> > +
> > + if (chip->ecc.size && chip->ecc.strength)
> > + return nand_check_ecc_caps(chip, caps, oobavail);
> > +
> > + if (chip->ecc.options & NAND_ECC_MAXIMIZE)
> > + return nand_maximize_ecc(chip, caps, oobavail);
> > +
> > + if (!nand_match_ecc_req(chip, caps, oobavail))
> > + return 0;
> > +
> > + ret = nand_maximize_ecc(chip, caps, oobavail);
>
>
> Why two calls for nand_maximize_ecc()?
As long as the code does the same thing, I don't care that much.
>
> My code is simpler,
and I don't see how your code is simpler. Mainly a matter of taste
AFAICS.
> and does not display
> false-positive warning.
I agree on the false-positive warning though, this should be avoided.
>
>
> > + if (!ret)
> > + pr_warn("ECC (step, strength) = (%d, %d) not supported on
> > this controller. Fallback to (%d, %d)\n",
> > + chip->ecc_step_ds, chip->ecc_strength_ds,
> > + chip->ecc.size, chip->ecc.strength);
>
>
> This is annoying.
>
> {ecc_step_ds, ecc_strength_ds} are not provided by Non-ONFi devices.
>
> So,
> ECC (step, strength) = (0, 0) not supported on this controller.
Well, if you have a chip that requires ECC but exposes 0bits/0bytes
then this should be fixed. 0,0 should only be valid when the chip does
not require ECC at all (so, only really old chips). For all other chips,
including non-ONFI ones, we should have a valid value here.
>
> will be always displayed.
>
>
> The strength will be checked by nand_ecc_strength_good() anyway.
True. So, I agree that the pr_warn() is unneeded, but I still think we
should fix all cases where ECC reqs are missing, so if you have such a
setup, please add some code to nand_.c to initialize
->ecc_xxx_ds properly.
Thanks,
Boris
Re: [PATCH] mtd: spi-nor: add support for Microchip 25LC256
On Fri, 4 May 2018 18:54:04 +0300
Radu Pirea wrote:
> Added geometry description for Microchip 25LC256 memory.
Same as for the dataflash stuff you posted a few weeks ago: I don't
think this device belongs in the SPI NOR framework.
>
> Signed-off-by: Radu Pirea
> ---
> drivers/mtd/devices/m25p80.c | 3 +++
> drivers/mtd/spi-nor/spi-nor.c | 3 +++
> 2 files changed, 6 insertions(+)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index a4e18f6aaa33..1e359d811261 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -372,6 +372,9 @@ static const struct spi_device_id m25p_ids[] = {
> { "mr25h10" }, /* 1 Mib, 40 MHz */
> { "mr25h40" }, /* 4 Mib, 40 MHz */
>
> + /* Microchip */
> + { "25lc256" },
> +
> { },
> };
> MODULE_DEVICE_TABLE(spi, m25p_ids);
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index d445a4d3b770..6341c86be647 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1076,6 +1076,9 @@ static const struct flash_info spi_nor_ids[] = {
> { "mx66l1g45g", INFO(0xc2201b, 0, 64 * 1024, 2048, SECT_4K |
> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> { "mx66l1g55g", INFO(0xc2261b, 0, 64 * 1024, 2048, SPI_NOR_QUAD_READ)
> },
>
> + /* Microchip */
> + { "25lc256", CAT25_INFO(32 * 1024, 1, 64, 2, SPI_NOR_NO_ERASE |
> SPI_NOR_NO_FR) },
> +
> /* Micron */
> { "n25q016a",INFO(0x20bb15, 0, 64 * 1024, 32, SECT_4K |
> SPI_NOR_QUAD_READ) },
> { "n25q032", INFO(0x20ba16, 0, 64 * 1024, 64, SPI_NOR_QUAD_READ)
> },
Re: [PATCH] spi-nor: Add support for Atmel Dataflash memories
Hi Radu, Sorry for the late reply. On Wed, 28 Feb 2018 13:55:01 +0200 Radu Pirea wrote: > This patch add support in spi-nor for allmost all dataflash memories > supported by old mtd_dataflash driver. Those devices clearly use a different instruction set, so I don't think they fit in this framework. Can you tell us why you want to move dataflash support to the SPI NOR framework. I think I know why, but I'd like to get your version. My guess is that some people want to connect dataflash chips to the Atmel QSPI controller, and it's not supported right now because the Atmel QSPI controller implements the SPI-NOR interface and not the generic SPI one, thus preventing anything that is not a SPI NOR from being connected to this controller. If I'm right, then the solution is to convert the QSPI driver to the spi-mem interface [1] and move it to drivers/spi/. Regards, Boris [1]http://patchwork.ozlabs.org/project/linux-mtd/list/?series=41174
Re: [PATCH v2 1/2] mtd: rawnand: fsl_ifc: fix FSL NAND driver to read
Hi Jane,
On Fri, 4 May 2018 01:35:37 +
"Wan, Jane (Nokia - US/Sunnyvale)" wrote:
> Hi Miquel and Boris,
>
> Thank you for your response. The following is the reposting the v2 version
> of the patch (also in the attachment).
No need to attach the patch, and this message should not be placed
here...
>
> Subject: [PATCH v2 1/2] mtd: rawnand: fsl_ifc: fix FSL NAND driver to read
> all ONFI parameter pages
>
> Per ONFI specification (Rev. 4.0), if the CRC of the first parameter page
> read is not valid, the host should read redundant parameter page copies.
> Fix FSL NAND driver to read the two redundant copies which are mandatory
> in the specification.
>
> Signed-off-by: Jane Wan
> ---
... but here.
Also, I'd recommend using git send-email to send patches, so that they
are properly sent and placed in a thread when they are part of a patch
series.
Thanks,
Boris
> drivers/mtd/nand/raw/fsl_ifc_nand.c | 17 ++---
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/fsl_ifc_nand.c
> b/drivers/mtd/nand/raw/fsl_ifc_nand.c
> index 61aae02..98aac1f 100644
> --- a/drivers/mtd/nand/raw/fsl_ifc_nand.c
> +++ b/drivers/mtd/nand/raw/fsl_ifc_nand.c
> @@ -342,9 +342,16 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd,
> unsigned int command,
>
> case NAND_CMD_READID:
> case NAND_CMD_PARAM: {
> + /*
> + * For READID, read 8 bytes that are currently used.
> + * For PARAM, read all 3 copies of 256-bytes pages.
> + */
> + int len = 8;
> int timing = IFC_FIR_OP_RB;
> - if (command == NAND_CMD_PARAM)
> + if (command == NAND_CMD_PARAM) {
> timing = IFC_FIR_OP_RBCD;
> + len = 256 * 3;
> + }
>
> ifc_out32((IFC_FIR_OP_CW0 << IFC_NAND_FIR0_OP0_SHIFT) |
> (IFC_FIR_OP_UA << IFC_NAND_FIR0_OP1_SHIFT) |
> @@ -354,12 +361,8 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd,
> unsigned int command,
> &ifc->ifc_nand.nand_fcr0);
> ifc_out32(column, &ifc->ifc_nand.row3);
>
> - /*
> - * although currently it's 8 bytes for READID, we always read
> - * the maximum 256 bytes(for PARAM)
> - */
> - ifc_out32(256, &ifc->ifc_nand.nand_fbcr);
> - ifc_nand_ctrl->read_bytes = 256;
> + ifc_out32(len, &ifc->ifc_nand.nand_fbcr);
> + ifc_nand_ctrl->read_bytes = len;
>
> set_addr(mtd, 0, 0, 0);
> fsl_ifc_run_command(mtd);
Re: [PATCH v2] mtd: chips: Replace printk() with pr_*()
On Sat, 24 Mar 2018 22:01:45 +0530 Arushi Singhal wrote: > Using pr_() is more concise than printk(KERN_). > This patch: > * Replace printks having a log level with the appropriate > pr_*() macros. > * Indent the code where possible. > * Remove periods from messages. > > Signed-off-by: Arushi Singhal > --- > changes: > Before printk() is changes to dev_*macro(), but it was not good idea > because mtd->dev is only initialized after mtd_device_register() is called. > > drivers/mtd/chips/cfi_cmdset_0001.c | 130 > +++- I see printk(KERN_ ...) in other files inside this directory. This should be fixed too.
Re: [PATCH] mtd: devices: check mtd_device_register() return code
On Sat, 24 Mar 2018 16:48:20 +0530
Arushi Singhal wrote:
Subject prefix should be "mtd: devices: st_spi_fsm: ", and it's really
about checking return code which was already propagated to the lower
layer (platform bus), but it's about making sure we disable the clk we
have previously enabled in case of failure.
BTW, I see that the clk is not disabled in ->remove(), probably
something we should fix too (in a separate patch).
> stfsm_probe() misses error handling of mtd_device_register().
>
> Signed-off-by: Arushi Singhal
> ---
> drivers/mtd/devices/st_spi_fsm.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/devices/st_spi_fsm.c
> b/drivers/mtd/devices/st_spi_fsm.c
> index 7bc29d7..4a99a6a 100644
> --- a/drivers/mtd/devices/st_spi_fsm.c
> +++ b/drivers/mtd/devices/st_spi_fsm.c
> @@ -2125,7 +2125,13 @@ static int stfsm_probe(struct platform_device *pdev)
> (long long)fsm->mtd.size, (long long)(fsm->mtd.size >> 20),
> fsm->mtd.erasesize, (fsm->mtd.erasesize >> 10));
>
> - return mtd_device_register(&fsm->mtd, NULL, 0);
> + ret = mtd_device_register(&fsm->mtd, NULL, 0);
> + if (ret) {
> + pr_err("Failed to register device\n");
> + goto err_clk_unprepare;
> + }
> +
> + return 0;
>
> err_clk_unprepare:
> clk_disable_unprepare(fsm->clk);
Re: [PATCH] mtd: nand: do not initialise statics to 0 or NULL
Hi Arushi, On Sat, 24 Mar 2018 15:47:27 +0530 Arushi Singhal wrote: > This patch fixes the checkpatch.pl error to ams-delta.c. > ERROR: do not initialise statics to 0 or NULL Can you rebase on top of nand/next [1] and use "mtd: rawnand: ams-delta: " for the subject prefix? Thanks, Boris [1]http://git.infradead.org/linux-mtd.git/shortlog/refs/heads/nand/next > > Signed-off-by: Arushi Singhal > --- > drivers/mtd/nand/ams-delta.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/ams-delta.c b/drivers/mtd/nand/ams-delta.c > index 6e7f6e0..8e89452 100644 > --- a/drivers/mtd/nand/ams-delta.c > +++ b/drivers/mtd/nand/ams-delta.c > @@ -35,7 +35,7 @@ > /* > * MTD structure for E3 (Delta) > */ > -static struct mtd_info *ams_delta_mtd = NULL; > +static struct mtd_info *ams_delta_mtd; > > /* > * Define partitions for flash devices
Re: [PATCH v3] mtd: onenand: omap2: Disable DMA for HIGHMEM buffers
On Wed, 2 May 2018 12:41:32 +0200
Ladislav Michl wrote:
> dma_map_single does not work for vmalloc-ed buffers,
> so disable DMA in this case.
>
> Signed-off-by: Ladislav Michl
> Reported-by: "H. Nikolaus Schaller"
> Tested-by: "H. Nikolaus Schaller"
Applied to mtd/master. Will be part of the next fixes PR I'll send
later this week.
Thanks,
Boris
> ---
> Changes:
> -v2: Added Tested-by tag, based on v4.17-rc1 (no change in patch itself)
> -v3: Reworded commit log
>
> drivers/mtd/nand/onenand/omap2.c | 105 +++
> 1 file changed, 38 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/mtd/nand/onenand/omap2.c
> b/drivers/mtd/nand/onenand/omap2.c
> index 9c159f0dd9a6..321137158ff3 100644
> --- a/drivers/mtd/nand/onenand/omap2.c
> +++ b/drivers/mtd/nand/onenand/omap2.c
> @@ -375,56 +375,42 @@ static int omap2_onenand_read_bufferram(struct mtd_info
> *mtd, int area,
> {
> struct omap2_onenand *c = container_of(mtd, struct omap2_onenand, mtd);
> struct onenand_chip *this = mtd->priv;
> - dma_addr_t dma_src, dma_dst;
> - int bram_offset;
> + struct device *dev = &c->pdev->dev;
> void *buf = (void *)buffer;
> + dma_addr_t dma_src, dma_dst;
> + int bram_offset, err;
> size_t xtra;
> - int ret;
>
> bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
> - if (bram_offset & 3 || (size_t)buf & 3 || count < 384)
> - goto out_copy;
> -
> - /* panic_write() may be in an interrupt context */
> - if (in_interrupt() || oops_in_progress)
> + /*
> + * If the buffer address is not DMA-able, len is not long enough to make
> + * DMA transfers profitable or panic_write() may be in an interrupt
> + * context fallback to PIO mode.
> + */
> + if (!virt_addr_valid(buf) || bram_offset & 3 || (size_t)buf & 3 ||
> + count < 384 || in_interrupt() || oops_in_progress )
> goto out_copy;
>
> - if (buf >= high_memory) {
> - struct page *p1;
> -
> - if (((size_t)buf & PAGE_MASK) !=
> - ((size_t)(buf + count - 1) & PAGE_MASK))
> - goto out_copy;
> - p1 = vmalloc_to_page(buf);
> - if (!p1)
> - goto out_copy;
> - buf = page_address(p1) + ((size_t)buf & ~PAGE_MASK);
> - }
> -
> xtra = count & 3;
> if (xtra) {
> count -= xtra;
> memcpy(buf + count, this->base + bram_offset + count, xtra);
> }
>
> + dma_dst = dma_map_single(dev, buf, count, DMA_FROM_DEVICE);
> dma_src = c->phys_base + bram_offset;
> - dma_dst = dma_map_single(&c->pdev->dev, buf, count, DMA_FROM_DEVICE);
> - if (dma_mapping_error(&c->pdev->dev, dma_dst)) {
> - dev_err(&c->pdev->dev,
> - "Couldn't DMA map a %d byte buffer\n",
> - count);
> - goto out_copy;
> - }
>
> - ret = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
> - dma_unmap_single(&c->pdev->dev, dma_dst, count, DMA_FROM_DEVICE);
> -
> - if (ret) {
> - dev_err(&c->pdev->dev, "timeout waiting for DMA\n");
> + if (dma_mapping_error(dev, dma_dst)) {
> + dev_err(dev, "Couldn't DMA map a %d byte buffer\n", count);
> goto out_copy;
> }
>
> - return 0;
> + err = omap2_onenand_dma_transfer(c, dma_src, dma_dst, count);
> + dma_unmap_single(dev, dma_dst, count, DMA_FROM_DEVICE);
> + if (!err)
> + return 0;
> +
> + dev_err(dev, "timeout waiting for DMA\n");
>
> out_copy:
> memcpy(buf, this->base + bram_offset, count);
> @@ -437,49 +423,34 @@ static int omap2_onenand_write_bufferram(struct
> mtd_info *mtd, int area,
> {
> struct omap2_onenand *c = container_of(mtd, struct omap2_onenand, mtd);
> struct onenand_chip *this = mtd->priv;
> - dma_addr_t dma_src, dma_dst;
> - int bram_offset;
> + struct device *dev = &c->pdev->dev;
> void *buf = (void *)buffer;
> - int ret;
> + dma_addr_t dma_src, dma_dst;
> + int bram_offset, err;
>
> bram_offset = omap2_onenand_bufferram_offset(mtd, area) + area + offset;
> - if (bram_offset & 3 || (size_t)buf & 3 || count < 384)
> - goto out_copy;
> -
> - /* panic_write() may be in an interrupt context */
> - if (in_interrupt() || oops_in_progress)
> + /*
> + * If the buffer address is not DMA-able, len is not long enough to make
> + * DMA transfers profitable or panic_write() may be in an interrupt
> + * context fallback to PIO mode.
> + */
> + if (!virt_addr_valid(buf) || bram_offset & 3 || (size_t)buf & 3 ||
> + count < 384 || in_interrupt() || oops_in_progress )
> goto out_copy;
>
> - if (buf >= high_memory) {
> - struct page *p1;
> -
> - if (((size_t)buf & PAGE_MASK) !=
> -
Re: [PATCH] mtd: rawnand: marvell: pass ms delay to wait_op
Hi Chris,
On Thu, 3 May 2018 05:28:32 +
Chris Packham wrote:
> On 03/05/18 14:21, Chris Packham wrote:
> > marvell_nfc_wait_op() expects the delay to be expressed in milliseconds
> > but nand_sdr_timings uses picoseconds. Use PSEC_TO_MSEC when passing
> > tPROG_max to marvell_nfc_wait_op().
> >
> > Fixes: 02f26ecf8c772 ("mtd: nand: add reworked Marvell NAND controller
> > driver")
> > Cc: sta...@vger.kernel.org
> > Signed-off-by: Chris Packham
> > ---
> > drivers/mtd/nand/raw/marvell_nand.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/marvell_nand.c
> > b/drivers/mtd/nand/raw/marvell_nand.c
> > index 1d779a35ac8e..e4b964fd40d8 100644
> > --- a/drivers/mtd/nand/raw/marvell_nand.c
> > +++ b/drivers/mtd/nand/raw/marvell_nand.c
> > @@ -1074,7 +1074,7 @@ static int
> > marvell_nfc_hw_ecc_hmg_do_write_page(struct nand_chip *chip,
> > return ret;
> >
> > ret = marvell_nfc_wait_op(chip,
> > - chip->data_interface.timings.sdr.tPROG_max);
> > +
> > PSEC_TO_MSEC(chip->data_interface.timings.sdr.tPROG_max));
> > return ret;
> > }
> >
> > @@ -1494,7 +1494,7 @@ static int marvell_nfc_hw_ecc_bch_write_page(struct
> > mtd_info *mtd,
> > }
> >
> > ret = marvell_nfc_wait_op(chip,
> > - chip->data_interface.timings.sdr.tPROG_max);
> > +
> > PSEC_TO_MSEC(chip->data_interface.timings.sdr.tPROG_max));
> >
> > marvell_nfc_disable_hw_ecc(chip);
> >
>
> Actually I'm not so sure about this patch. While passing the pico-second
> value for tPROG_max is clearly wrong and leads to seemingly indefinite
> hangs on some systems. Converting the times to micro-seconds leaves us
> with delays that are far too short.
What makes you think they are far too short?
>
> The old pxa3xx driver had hard coded 200ms delays. These delays now work
> out to 1ms which seems every bit as wrong as 6ms.
The old driver was indifferently waiting a huge amount of time no
matter how long the chip is supposed to stay busy for a specific
operation. Here we're using the value exposed by the chip itself, and
it's not a typical value, it's a max value, which means you should
never reach that in real life, and if you do that means your chip is
stuck for some reasons. Now, if you want to take extra safety margin,
you can multiply the value by 2 and that should be enough, but 200ms is
way too long.
Regards,
Boris
Re: [PATCH] drm/vc4: Fix oops dereferencing DPI's connector since panel_bridge.
On Fri, 9 Mar 2018 15:32:56 -0800
Eric Anholt wrote:
> In the cleanup, I didn't notice that we needed to dereference the
> connector for the bus_format. Fix the regression by looking up the
> first (and only) connector attached to us, and assume that its
> bus_format is what we want. Some day it would be good to have that
> part of display_info attached to the bridge, instead.
>
> Signed-off-by: Eric Anholt
> Fixes: 7b1298e05310 ("drm/vc4: Switch DPI to using the panel-bridge helper.")
> Cc: Boris Brezillon
Reviewed-by: Boris Brezillon
> ---
> drivers/gpu/drm/vc4/vc4_dpi.c | 27 +++
> 1 file changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c
> index 72c9dbd81d7f..88783e143cc2 100644
> --- a/drivers/gpu/drm/vc4/vc4_dpi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dpi.c
> @@ -96,7 +96,6 @@ struct vc4_dpi {
> struct platform_device *pdev;
>
> struct drm_encoder *encoder;
> - struct drm_connector *connector;
>
> void __iomem *regs;
>
> @@ -164,14 +163,31 @@ static void vc4_dpi_encoder_disable(struct drm_encoder
> *encoder)
>
> static void vc4_dpi_encoder_enable(struct drm_encoder *encoder)
> {
> + struct drm_device *dev = encoder->dev;
> struct drm_display_mode *mode = &encoder->crtc->mode;
> struct vc4_dpi_encoder *vc4_encoder = to_vc4_dpi_encoder(encoder);
> struct vc4_dpi *dpi = vc4_encoder->dpi;
> + struct drm_connector_list_iter conn_iter;
> + struct drm_connector *connector = NULL, *connector_scan;
> u32 dpi_c = DPI_ENABLE | DPI_OUTPUT_ENABLE_MODE;
> int ret;
>
> - if (dpi->connector->display_info.num_bus_formats) {
> - u32 bus_format = dpi->connector->display_info.bus_formats[0];
> + /* Look up the connector attached to DPI so we can get the
> + * bus_format. Ideally the bridge would tell us the
> + * bus_format we want, but it doesn't yet, so assume that it's
> + * uniform throughout the bridge chain.
> + */
> + drm_connector_list_iter_begin(dev, &conn_iter);
> + drm_for_each_connector_iter(connector_scan, &conn_iter) {
> + if (connector_scan->encoder == encoder) {
> + connector = connector_scan;
> + break;
> + }
> + }
> + drm_connector_list_iter_end(&conn_iter);
> +
> + if (connector && connector->display_info.num_bus_formats) {
> + u32 bus_format = connector->display_info.bus_formats[0];
>
> switch (bus_format) {
> case MEDIA_BUS_FMT_RGB888_1X24:
> @@ -199,6 +215,9 @@ static void vc4_dpi_encoder_enable(struct drm_encoder
> *encoder)
> DRM_ERROR("Unknown media bus format %d\n", bus_format);
> break;
> }
> + } else {
> + /* Default to 24bit if no connector found. */
> + dpi_c |= VC4_SET_FIELD(DPI_FORMAT_24BIT_888_RGB, DPI_FORMAT);
> }
>
> if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> @@ -264,7 +283,7 @@ static int vc4_dpi_init_bridge(struct vc4_dpi *dpi)
> return ret;
> }
>
> - if (panel)
> + if (panel)
> bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_DPI);
>
> return drm_bridge_attach(dpi->encoder, bridge, NULL);
Re: [PATCH 1/2] Fix FSL NAND driver to read all ONFI parameter pages
On Tue, 1 May 2018 05:01:23 + "Wan, Jane (Nokia - US/Sunnyvale)" wrote: > Hi Miquèl and Boris, > > Thank you for your response and feedback. I've modified the fix based on > your comments. > Please see the updated patch file at the end of this message (also in > attachment). > My answers to your comments/questions are inline in the previous message. > > Here is the answer to Boris question in another email thread: > > > What if some NANDs have 4 or more copies of the param page? > [Jane] The ONFI spec defines that the parameter page and its two redundant > copies are mandatory. > The additional redundant pages are optional. Currently, the FSL NAND driver > only reads the first > parameter page. This patch is to fix the driver to meet the mandatory > requirement in the spec. > We got a batch of particularly bad NAND chips recently and we needed these > changes to make them > work reliably over temperature. The patch was verified using these bad chips. And that proves my point. The core is reading 3 param pages [1], but since this driver was trying to guess how many bytes to read from ->cmdfunc() and did not guess correctly you ended up with a partially working implementation (works only if the first PARAM page is valid). Now, you fix it to read 3 PARAM pages, but what if we decide to read more to cope with MLC NANDs where even more copy are needed to have one valid version? You'll have to patch ->cmdfunc() again, just because you're trying to guess something that the core is supposed to tell you. [1]https://elixir.bootlin.com/linux/v4.17-rc3/source/drivers/mtd/nand/raw/nand_base.c#L5115
Re: [PATCH 2/2] Use bit-wise majority to recover the contents of ONFI parameter
On Wed, 2 May 2018 12:25:45 +0200
Boris Brezillon wrote:
> Hi Jane,
>
> On Thu, 26 Apr 2018 17:19:56 -0700
> Jane Wan wrote:
>
> > Signed-off-by: Jane Wan
> > ---
> > drivers/mtd/nand/nand_base.c | 35 +++
> > 1 file changed, 31 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index c2e1232..161b523 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -3153,8 +3153,10 @@ static int nand_flash_detect_onfi(struct mtd_info
> > *mtd, struct nand_chip *chip,
> > int *busw)
> > {
> > struct nand_onfi_params *p = &chip->onfi_params;
> > - int i, j;
> > - int val;
> > + int i, j, k, len, val;
> > + uint8_t copy[3][256], v8;
> > +
> > + len = (sizeof(*p) > 256) ? 256 : sizeof(*p);
> >
> > /* Try ONFI for unknown chip or LP */
> > chip->cmdfunc(mtd, NAND_CMD_READID, 0x20, -1);
> > @@ -3170,11 +3172,36 @@ static int nand_flash_detect_onfi(struct mtd_info
> > *mtd, struct nand_chip *chip,
> > le16_to_cpu(p->crc)) {
> > break;
> > }
> > + pr_err("CRC of parameter page %d is not valid\n", i);
> > + for (j = 0; j < len; j++)
> > + copy[i][j] = ((uint8_t *)p)[j];
> > }
> >
> > if (i == 3) {
> > - pr_err("Could not find valid ONFI parameter page; aborting\n");
> > - return 0;
> > + pr_err("Could not find valid ONFI parameter page\n");
> > + pr_info("Recover ONFI parameters with bit-wise majority\n");
> > + for (j = 0; j < len; j++) {
> > + if (copy[0][j] == copy[1][j] ||
> > + copy[0][j] == copy[2][j]) {
> > + ((uint8_t *)p)[j] = copy[0][j];
> > + } else if (copy[1][j] == copy[2][j]) {
> > + ((uint8_t *)p)[j] = copy[1][j];
> > + } else {
> > + ((uint8_t *)p)[j] = 0;
> > + for (k = 0; k < 8; k++) {
> > + v8 = (copy[0][j] >> k) & 0x1;
> > + v8 += (copy[1][j] >> k) & 0x1;
> > + v8 += (copy[2][j] >> k) & 0x1;
> > + if (v8 > 1)
> > + ((uint8_t *)p)[j] |= (1 << k);
> > + }
> > + }
> > + }
>
> I'd like this bit-wise majority algorithm to be generic and moved to
> nand_base.c, because we might want to do the same in the core and make
> it work for any number of repetitions of the PARAM page.
Never mind, I thought you were implementing that in the FSL IFC driver,
but you're actually modifying the core.
Re: [PATCH 2/2] Use bit-wise majority to recover the contents of ONFI parameter
Hi Jane,
On Thu, 26 Apr 2018 17:19:56 -0700
Jane Wan wrote:
> Signed-off-by: Jane Wan
> ---
> drivers/mtd/nand/nand_base.c | 35 +++
> 1 file changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index c2e1232..161b523 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3153,8 +3153,10 @@ static int nand_flash_detect_onfi(struct mtd_info
> *mtd, struct nand_chip *chip,
> int *busw)
> {
> struct nand_onfi_params *p = &chip->onfi_params;
> - int i, j;
> - int val;
> + int i, j, k, len, val;
> + uint8_t copy[3][256], v8;
> +
> + len = (sizeof(*p) > 256) ? 256 : sizeof(*p);
>
> /* Try ONFI for unknown chip or LP */
> chip->cmdfunc(mtd, NAND_CMD_READID, 0x20, -1);
> @@ -3170,11 +3172,36 @@ static int nand_flash_detect_onfi(struct mtd_info
> *mtd, struct nand_chip *chip,
> le16_to_cpu(p->crc)) {
> break;
> }
> + pr_err("CRC of parameter page %d is not valid\n", i);
> + for (j = 0; j < len; j++)
> + copy[i][j] = ((uint8_t *)p)[j];
> }
>
> if (i == 3) {
> - pr_err("Could not find valid ONFI parameter page; aborting\n");
> - return 0;
> + pr_err("Could not find valid ONFI parameter page\n");
> + pr_info("Recover ONFI parameters with bit-wise majority\n");
> + for (j = 0; j < len; j++) {
> + if (copy[0][j] == copy[1][j] ||
> + copy[0][j] == copy[2][j]) {
> + ((uint8_t *)p)[j] = copy[0][j];
> + } else if (copy[1][j] == copy[2][j]) {
> + ((uint8_t *)p)[j] = copy[1][j];
> + } else {
> + ((uint8_t *)p)[j] = 0;
> + for (k = 0; k < 8; k++) {
> + v8 = (copy[0][j] >> k) & 0x1;
> + v8 += (copy[1][j] >> k) & 0x1;
> + v8 += (copy[2][j] >> k) & 0x1;
> + if (v8 > 1)
> + ((uint8_t *)p)[j] |= (1 << k);
> + }
> + }
> + }
I'd like this bit-wise majority algorithm to be generic and moved to
nand_base.c, because we might want to do the same in the core and make
it work for any number of repetitions of the PARAM page.
> + if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) !=
> + le16_to_cpu(p->crc)) {
> + pr_err("ONFI parameter recovery failed, aborting\n");
> + return 0;
> + }
> }
>
> /* Check version */
Thanks,
Boris
Re: [PATCH v2] mtd: onenand: omap2: Disable DMA for HIGHMEM buffers
On Wed, 2 May 2018 10:06:36 +0200 Ladislav Michl wrote: > Hi Boris, > > (and apologies for delay) > > On Fri, Apr 20, 2018 at 10:01:34PM +0200, Boris Brezillon wrote: > > Hi Ladislav, > > > > On Mon, 16 Apr 2018 08:52:59 +0200 > > Ladislav Michl wrote: > > > > > dma_map_single doesn't get the proper DMA address for vmalloced area, > > > > That's not true, it returns the right DMA (physical) address, it's just > > that: > > To be honest I used log message from commit dcf08227e964 which is dealing > with the same issue. Okay, looks like I was wrong. The problem is caused by the virt_to_page() call done in dma_map_single_attrs() which expects a valid virtual address (one that is present in the identity mapping). If you pass a vmalloc address to it, the conversion is broken and that's probably why you end up with a NULL pointer exception. Maybe you should just say that dma_map_single() does not work for vmalloc-ed buffers instead of saying that it does not get the right DMA address.
Re: [PATCH 00/12] mtd: nand: davinci: stop using pdev->id as chipselect
On Tue, 1 May 2018 16:01:42 +0530 Sekhar Nori wrote: > On Monday 30 April 2018 03:39 PM, Boris Brezillon wrote: > > Hi Bartosz, > > > > On Mon, 30 Apr 2018 10:24:41 +0200 > > Bartosz Golaszewski wrote: > > > >> From: Bartosz Golaszewski > >> > >> We have the 'ti,davinci-chipselect' property in the device tree, but > >> when using platform data the driver silently uses the id field of > >> struct platform_device as the chipselect. This is confusing and we > >> almost broke the nand support again recently after converting the > >> platform to common clock framework (which changed the device id in the > >> clock lookup - the problem is gone now that we no longer acquire the > >> clock in the nand driver. > >> > >> This series adds a new field to davinci-nand platform data, then makes > >> all board use it and finally modifies the two drivers that make use of > >> it. > >> > >> Bartosz Golaszewski (12): > >> mtd: nand: davinci: store the core chipselect number in platform data > > > > Raw NAND related patches (that is, everything that is not onenand or > > SPI NAND) should use the "mtd: rawnand: " prefix now. Other than that, > > I'm fine with the patch series, just let me know how you want to have it > > Hi Boris, should I convert the "I'm fine" to some sort of formal tag? If > yes, what is the tag and for which patches? Yep, you can add Acked-by: Boris Brezillon on patches 1 and 11. Thanks, Boris
Re: [PATCH 00/12] mtd: nand: davinci: stop using pdev->id as chipselect
On Mon, 30 Apr 2018 18:45:06 +0200 Bartosz Golaszewski wrote: > 2018-04-30 12:09 GMT+02:00 Boris Brezillon : > > Hi Bartosz, > > > > On Mon, 30 Apr 2018 10:24:41 +0200 > > Bartosz Golaszewski wrote: > > > >> From: Bartosz Golaszewski > >> > >> We have the 'ti,davinci-chipselect' property in the device tree, but > >> when using platform data the driver silently uses the id field of > >> struct platform_device as the chipselect. This is confusing and we > >> almost broke the nand support again recently after converting the > >> platform to common clock framework (which changed the device id in the > >> clock lookup - the problem is gone now that we no longer acquire the > >> clock in the nand driver. > >> > >> This series adds a new field to davinci-nand platform data, then makes > >> all board use it and finally modifies the two drivers that make use of > >> it. > >> > >> Bartosz Golaszewski (12): > >> mtd: nand: davinci: store the core chipselect number in platform data > > > > Raw NAND related patches (that is, everything that is not onenand or > > SPI NAND) should use the "mtd: rawnand: " prefix now. Other than that, > > I'm fine with the patch series, just let me know how you want to have it > > merged (through the MTD tree or the davinci tree). > > > > Thanks, > > > > Boris > > I think Sekhar could pick all those patches up for 4.18. Okay, then I'd need an immutable branch containing these changes. > > Sekhar - do you want me to resend the series with the commit message > changed as requested by Boris, or can you fix it when applying the > series? > > Best regards, > Bartosz Golaszewski
Re: [PATCH v2] mtd: nftl: Remove VLA usage
On Sun, 29 Apr 2018 08:00:53 -0700
Kees Cook wrote:
> On the quest to remove all stack VLAs from the kernel[1] this changes
> the check_free_sectors() routine to use a kmalloc()ed buffer instead
> of a large VLA stack buffer.
>
> [1]
> https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
>
> Signed-off-by: Kees Cook
Applied to mtd/next.
Thanks,
Boris
> ---
> v2: kmalloc() instead of stack buffer (Boris)
> ---
> drivers/mtd/inftlmount.c | 23 ---
> drivers/mtd/nftlmount.c | 23 ---
> 2 files changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mtd/inftlmount.c b/drivers/mtd/inftlmount.c
> index aab4f68bd36f..2d598412972d 100644
> --- a/drivers/mtd/inftlmount.c
> +++ b/drivers/mtd/inftlmount.c
> @@ -334,28 +334,37 @@ static int memcmpb(void *a, int c, int n)
> static int check_free_sectors(struct INFTLrecord *inftl, unsigned int
> address,
> int len, int check_oob)
> {
> - u8 buf[SECTORSIZE + inftl->mbd.mtd->oobsize];
> struct mtd_info *mtd = inftl->mbd.mtd;
> size_t retlen;
> - int i;
> + int i, ret;
> + u8 *buf;
> +
> + buf = kmalloc(SECTORSIZE + mtd->oobsize, GFP_KERNEL);
> + if (!buf)
> + return -1;
>
> + ret = -1;
> for (i = 0; i < len; i += SECTORSIZE) {
> if (mtd_read(mtd, address, SECTORSIZE, &retlen, buf))
> - return -1;
> + goto out;
> if (memcmpb(buf, 0xff, SECTORSIZE) != 0)
> - return -1;
> + goto out;
>
> if (check_oob) {
> if(inftl_read_oob(mtd, address, mtd->oobsize,
> &retlen, &buf[SECTORSIZE]) < 0)
> - return -1;
> + goto out;
> if (memcmpb(buf + SECTORSIZE, 0xff, mtd->oobsize) != 0)
> - return -1;
> + goto out;
> }
> address += SECTORSIZE;
> }
>
> - return 0;
> + ret = 0;
> +
> +out:
> + kfree(buf);
> + return ret;
> }
>
> /*
> diff --git a/drivers/mtd/nftlmount.c b/drivers/mtd/nftlmount.c
> index a6fbfa4e5799..6281da3dadac 100644
> --- a/drivers/mtd/nftlmount.c
> +++ b/drivers/mtd/nftlmount.c
> @@ -272,28 +272,37 @@ static int memcmpb(void *a, int c, int n)
> static int check_free_sectors(struct NFTLrecord *nftl, unsigned int address,
> int len,
> int check_oob)
> {
> - u8 buf[SECTORSIZE + nftl->mbd.mtd->oobsize];
> struct mtd_info *mtd = nftl->mbd.mtd;
> size_t retlen;
> - int i;
> + int i, ret;
> + u8 *buf;
> +
> + buf = kmalloc(SECTORSIZE + mtd->oobsize, GFP_KERNEL);
> + if (!buf)
> + return -1;
>
> + ret = -1;
> for (i = 0; i < len; i += SECTORSIZE) {
> if (mtd_read(mtd, address, SECTORSIZE, &retlen, buf))
> - return -1;
> + goto out;
> if (memcmpb(buf, 0xff, SECTORSIZE) != 0)
> - return -1;
> + goto out;
>
> if (check_oob) {
> if(nftl_read_oob(mtd, address, mtd->oobsize,
>&retlen, &buf[SECTORSIZE]) < 0)
> - return -1;
> + goto out;
> if (memcmpb(buf + SECTORSIZE, 0xff, mtd->oobsize) != 0)
> - return -1;
> + goto out;
> }
> address += SECTORSIZE;
> }
>
> - return 0;
> + ret = 0;
> +
> +out:
> + kfree(buf);
> + return ret;
> }
>
> /* NFTL_format: format a Erase Unit by erasing ALL Erase Zones in the Erase
> Unit and
Re: [PATCH 00/12] mtd: nand: davinci: stop using pdev->id as chipselect
Hi Bartosz, On Mon, 30 Apr 2018 10:24:41 +0200 Bartosz Golaszewski wrote: > From: Bartosz Golaszewski > > We have the 'ti,davinci-chipselect' property in the device tree, but > when using platform data the driver silently uses the id field of > struct platform_device as the chipselect. This is confusing and we > almost broke the nand support again recently after converting the > platform to common clock framework (which changed the device id in the > clock lookup - the problem is gone now that we no longer acquire the > clock in the nand driver. > > This series adds a new field to davinci-nand platform data, then makes > all board use it and finally modifies the two drivers that make use of > it. > > Bartosz Golaszewski (12): > mtd: nand: davinci: store the core chipselect number in platform data Raw NAND related patches (that is, everything that is not onenand or SPI NAND) should use the "mtd: rawnand: " prefix now. Other than that, I'm fine with the patch series, just let me know how you want to have it merged (through the MTD tree or the davinci tree). Thanks, Boris
Re: [PATCH 1/2] Fix FSL NAND driver to read all ONFI parameter pages
Hi Jane, On Thu, 26 Apr 2018 17:19:55 -0700 Jane Wan wrote: > Signed-off-by: Jane Wan > --- > drivers/mtd/nand/fsl_ifc_nand.c | 10 ++ > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c > index ca36b35..a3cf6ca 100644 > --- a/drivers/mtd/nand/fsl_ifc_nand.c > +++ b/drivers/mtd/nand/fsl_ifc_nand.c > @@ -413,6 +413,7 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, > unsigned int command, > struct fsl_ifc_mtd *priv = chip->priv; > struct fsl_ifc_ctrl *ctrl = priv->ctrl; > struct fsl_ifc_runtime __iomem *ifc = ctrl->rregs; > + int len; > > /* clear the read buffer */ > ifc_nand_ctrl->read_bytes = 0; > @@ -462,11 +463,12 @@ static void fsl_ifc_cmdfunc(struct mtd_info *mtd, > unsigned int command, > ifc_out32(column, &ifc->ifc_nand.row3); > > /* > - * although currently it's 8 bytes for READID, we always read > - * the maximum 256 bytes(for PARAM) > + * For READID, read 8 bytes that are currently used. > + * For PARAM, read all 3 copies of 256-bytes pages. >*/ > - ifc_out32(256, &ifc->ifc_nand.nand_fbcr); > - ifc_nand_ctrl->read_bytes = 256; > + len = (command == NAND_CMD_PARAM) ? (3 * 256) : 8; > + ifc_out32(len, &ifc->ifc_nand.nand_fbcr); > + ifc_nand_ctrl->read_bytes = len; This driver really calls for a clean rework to transition to ->exec_op(). Guessing the amount of data to be read from ->cmdfunc() is broken and your patch series just shows how broken this is. What next? What if some NANDs have 4 or more copies of the param page? I'm still undecided whether I want to apply this patch. I guess having some guarantees that someone will actually work on implementing ->exec_op() in a near future would help me take this decision. Regards, Boris
Re: [PATCH v4 00/10] Add the I3C subsystem
Hi Greg, On Sun, 29 Apr 2018 15:36:42 +0200 Greg Kroah-Hartman wrote: > On Mon, Apr 23, 2018 at 07:56:46PM +0200, Greg Kroah-Hartman wrote: > > On Mon, Apr 23, 2018 at 07:38:14PM +0200, Boris Brezillon wrote: > > > Hi, > > > > > > On Fri, 30 Mar 2018 09:47:41 +0200 > > > Boris Brezillon wrote: > > > > > > > This patch series is a proposal for a new I3C subsystem. > > > > > > This v4 has been sent almost a month ago and I didn't get any feedback > > > so far apart from Rob's R-b. Greg, is there any chance we can get these > > > patches merged in 4.18? If not, could you tell me what should be > > > addressed/improved/reworked? > > > > I'll look over it later this week, thanks. > > At first glance, it would be great to have at least one other > reviewed-by or signed-off-by on the main code here. I don't want to be > the only one to have to review it :) I understand. Arnd, Wolfram, any chance you could spend some time reviewing this patch series? I know Arnd is in vacation for the next few weeks, so I don't expect him to be able to do that immediately. Also, Wolfram stated that he didn't have time to review the series in details when I submitted the v1, I don't know if things have changed since then. Anyway, I'll keep searching for people to review the code. Regards, Boris
Re: [PATCH v4 03/10] i3c: Add sysfs ABI spec
Hi Greg, On Sun, 29 Apr 2018 15:37:00 +0200 Greg Kroah-Hartman wrote: > On Fri, Mar 30, 2018 at 09:47:44AM +0200, Boris Brezillon wrote: > > Document sysfs files/directories/symlinks exposed by the I3C subsystem. > > > > Signed-off-by: Boris Brezillon > > --- > > Changes in v2: > > - new patch > > --- > > Documentation/ABI/testing/sysfs-bus-i3c | 95 > > + > > 1 file changed, 95 insertions(+) > > create mode 100644 Documentation/ABI/testing/sysfs-bus-i3c > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-i3c > > b/Documentation/ABI/testing/sysfs-bus-i3c > > new file mode 100644 > > index ..5e88cc093e0e > > --- /dev/null > > +++ b/Documentation/ABI/testing/sysfs-bus-i3c > > @@ -0,0 +1,95 @@ > > +What: /sys/bus/i3c/devices/i3c- > > +KernelVersion: 4.16 > > Wrong kernel versions :) I'll fix that. Thanks, Boris
Re: [PATCH 2/2] drm/panel: Enable DSI transactions on the RPi panel.
On Tue, 31 Oct 2017 12:32:58 -0700
Eric Anholt wrote:
> It turns out that I had just mistaken what type of write the register
> writes were supposed to be, using DCS instead of generic long writes.
>
> Switching to transactions instead of using the atmel as a bridge also
> seems to resolve the sparkling pixels problem I've had.
>
> Signed-off-by: Eric Anholt
> Fixes: 2f733d6194bd ("drm/panel: Add support for the Raspberry Pi 7"
> Touchscreen.")
Reviewed-by: Boris Brezillon
> ---
> drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c | 14 +-
> 1 file changed, 1 insertion(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> index d964d454e4ae..2c9c9722734f 100644
> --- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> +++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
> @@ -238,12 +238,6 @@ static void rpi_touchscreen_i2c_write(struct
> rpi_touchscreen *ts,
>
> static int rpi_touchscreen_write(struct rpi_touchscreen *ts, u16 reg, u32
> val)
> {
> -#if 0
> - /* The firmware uses LP DSI transactions like this to bring up
> - * the hardware, which should be faster than using I2C to then
> - * pass to the Toshiba. However, I was unable to get it to
> - * work.
> - */
> u8 msg[] = {
> reg,
> reg >> 8,
> @@ -253,13 +247,7 @@ static int rpi_touchscreen_write(struct rpi_touchscreen
> *ts, u16 reg, u32 val)
> val >> 24,
> };
>
> - mipi_dsi_dcs_write_buffer(ts->dsi, msg, sizeof(msg));
> -#else
> - rpi_touchscreen_i2c_write(ts, REG_WR_ADDRH, reg >> 8);
> - rpi_touchscreen_i2c_write(ts, REG_WR_ADDRL, reg);
> - rpi_touchscreen_i2c_write(ts, REG_WRITEH, val >> 8);
> - rpi_touchscreen_i2c_write(ts, REG_WRITEL, val);
> -#endif
> + mipi_dsi_generic_write(ts->dsi, msg, sizeof(msg));
>
> return 0;
> }
--
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [PATCH 1/2] drm/vc4: Skip ULPS latching when we're in that ULPS state already.
On Tue, 31 Oct 2017 12:32:57 -0700 Eric Anholt wrote: > It seems that trying to go from unlatched to unlatched will time out > waiting for STOP, and we can just skip that. > > Signed-off-by: Eric Anholt Reviewed-by: Boris Brezillon > --- > drivers/gpu/drm/vc4/vc4_dsi.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c > index 94085f8bcd68..8aa897835118 100644 > --- a/drivers/gpu/drm/vc4/vc4_dsi.c > +++ b/drivers/gpu/drm/vc4/vc4_dsi.c > @@ -753,6 +753,11 @@ static void vc4_dsi_ulps(struct vc4_dsi *dsi, bool ulps) >(dsi->lanes > 2 ? DSI1_STAT_PHY_D2_STOP : 0) | >(dsi->lanes > 3 ? DSI1_STAT_PHY_D3_STOP : 0)); > int ret; > + bool ulps_currently_enabled = (DSI_PORT_READ(PHY_AFEC0) & > +DSI_PORT_BIT(PHY_AFEC0_LATCH_ULPS)); > + > + if (ulps == ulps_currently_enabled) > + return; > > DSI_PORT_WRITE(STAT, stat_ulps); > DSI_PORT_WRITE(PHYC, DSI_PORT_READ(PHYC) | phyc_ulps); -- Boris Brezillon, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
Re: [PATCH V4] clk: at91: PLL recalc_rate() now using cached MUL and DIV values
On Sun, 29 Apr 2018 15:01:11 -0400
Marcin Ziemianowicz wrote:
> When a USB device is connected to the USB host port on the SAM9N12 then
> you get "-62" error which seems to indicate USB replies from the device
> are timing out. Based on a logic sniffer, I saw the USB bus was running
> at half speed.
>
> The PLL code uses cached MUL and DIV values which get set in set_rate()
> and applied in prepare(), but the recalc_rate() function instead
> queries the hardware instead of using these cached values. Therefore,
> if recalc_rate() is called between a set_rate() and prepare(), the
> wrong frequency is calculated and later the USB clock divider for the
> SAM9N12 SOC will be configured for an incorrect clock.
>
> In my case, the PLL hardware was set to 96 Mhz before the OHCI
> driver loads, and therefore the usb clock divider was being set
> to /2 even though the OHCI driver set the PLL to 48 Mhz.
>
> As an alternative explanation, I noticed this was fixed in the past by
> 87e2ed338f1b ("clk: at91: fix recalc_rate implementation of PLL
> driver") but the bug was later re-introduced by 1bdf02326b71 ("clk:
> at91: make use of syscon/regmap internally").
>
> Fixes: 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally)
> Cc:
> Signed-off-by: Marcin Ziemianowicz
Acked-by: Boris Brezillon
> ---
> Thank you for bearing with me about this Boris.
>
> Changes since V3:
> Fix for double returns found by kbluild test robot
> > Comments by Boris Brezillon about email formatting issues
> Changes since V2:
> Removed all logging/debug messages I added
> > Comment by Boris Brezillon about my fix being wrong addressed
> Changes since V1:
> Added patch set cover letter
> Shortened lines which were over >80 characters long
> > Comment by Greg Kroah-Hartman about "from" field in email addressed
> > Comment by Alan Stern about redundant debug lines addressed
>
> drivers/clk/at91/clk-pll.c | 13 +
> 1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/drivers/clk/at91/clk-pll.c b/drivers/clk/at91/clk-pll.c
> index 7d3223fc..72b6091e 100644
> --- a/drivers/clk/at91/clk-pll.c
> +++ b/drivers/clk/at91/clk-pll.c
> @@ -132,19 +132,8 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw
> *hw,
>unsigned long parent_rate)
> {
> struct clk_pll *pll = to_clk_pll(hw);
> - unsigned int pllr;
> - u16 mul;
> - u8 div;
> -
> - regmap_read(pll->regmap, PLL_REG(pll->id), &pllr);
> -
> - div = PLL_DIV(pllr);
> - mul = PLL_MUL(pllr, pll->layout);
> -
> - if (!div || !mul)
> - return 0;
>
> - return (parent_rate / div) * (mul + 1);
> + return (parent_rate / pll->div) * (pll->mul + 1);
> }
>
> static long clk_pll_get_best_div_mul(struct clk_pll *pll, unsigned long rate,
Re: [PATCH] mtd: nftl: Remove VLA usage
On Sun, 29 Apr 2018 07:31:15 -0700
Kees Cook wrote:
> On Sun, Apr 29, 2018 at 2:16 AM, Boris Brezillon
> wrote:
> > Hi Kees,
> >
> > On Mon, 23 Apr 2018 13:35:00 -0700
> > Kees Cook wrote:
> >
> >> On the quest to remove all VLAs from the kernel[1] this changes the
> >> check_free_sectors() routine to use the same stack buffer for both
> >> data byte checks (SECTORSIZE) and oob byte checks (oobsize). Since
> >> these regions aren't needed at the same time, they don't need to be
> >> consecutively allocated. Additionally, while it's possible for oobsize
> >> to be large, it is unlikely to be larger than the actual SECTORSIZE. As
> >> such, remove the VLA, adjust offsets and add a sanity check to make sure
> >> we never get a pathological oobsize.
> >>
> >> [1]
> >> https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
> >>
> >> Signed-off-by: Kees Cook
> >> ---
> >> drivers/mtd/inftlmount.c | 11 ---
> >> drivers/mtd/nftlmount.c | 11 ---
> >> 2 files changed, 16 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/mtd/inftlmount.c b/drivers/mtd/inftlmount.c
> >> index aab4f68bd36f..9cdae7f0fc2e 100644
> >> --- a/drivers/mtd/inftlmount.c
> >> +++ b/drivers/mtd/inftlmount.c
> >> @@ -334,7 +334,7 @@ static int memcmpb(void *a, int c, int n)
> >> static int check_free_sectors(struct INFTLrecord *inftl, unsigned int
> >> address,
> >> int len, int check_oob)
> >> {
> >> - u8 buf[SECTORSIZE + inftl->mbd.mtd->oobsize];
> >> + u8 buf[SECTORSIZE];
> >
> > Could we instead move to dynamic allocation. I mean, SECTORSIZE is 512
> > bytes, so only with this function we consume 1/16 of the stack. Not to
> > mention that some MTD drivers might want to do DMA on buffer passed by
> > the MTD user, and if the buffer is on the stack they'll have to use a
> > bounce buffer instead.
>
> Sure! I can rework it to do that. Is GFP_KERNEL okay for that, or does
> it need something else?
Just had a quick look, and I think GFP_KERNEL is good.
Re: [PATCH v3] clk: at91: PLL recalc_rate() now using cached MUL+DIV values
On Sun, 29 Apr 2018 15:17:10 +0200
Boris Brezillon wrote:
> Hi Marcin,
>
> On Fri, 27 Apr 2018 13:56:09 -0400
> Marcin Ziemianowicz wrote:
>
> > Stephen Boyd ,
> > linux-...@vger.kernel.org,
> > linux-arm-ker...@lists.infradead.org,
> > linux-kernel@vger.kernel.org
> > Bcc:
> > Subject: [PATCH v3] clk: at91: PLL recalc_rate() now using cached MUL and
> > DIV
> > values
> > Reply-To:
>
> Hm, I don't know how you prepared and sent your patch, but you shouldn't
> have these fields in the body of your email. Please use git format-patch
> to prepare the patch and then git send-email to send it.
>
> >
> > When a USB device is connected to the USB host port on the SAM9N12 then
> > you get "-62" error which seems to indicate USB replies from the device
> > are timing out. Based on a logic sniffer, I saw the USB bus was running
> > at half speed.
> >
> > The PLL code uses cached MUL and DIV values which get set in set_rate()
> > and applied in prepare(), but the recalc_rate() function instead
> > queries the hardware instead of using these cached values. Therefore,
> > if recalc_rate() is called between a set_rate() and prepare(), the
> > wrong frequency is calculated and later the USB clock divider for the
> > SAM9N12 SOC will be configured for an incorrect clock.
> >
> > In my case, the PLL hardware was set to 96 Mhz before the OHCI
> > driver loads, and therefore the usb clock divider was being set
> > to /2 even though the OHCI driver set the PLL to 48 Mhz.
> >
> > As an alternative explanation, I noticed this was fixed in the past:
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283502.html
> > but was later changed back via a large patch (maybe by mistake?):
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1bdf02326b71eae7e9b4b335b881856aaf9d1af6
> >
>
> Yep, probably by mistake. I started this rework long before it has been
> submitted to the ML, so I probably messed something up when rebasing.
>
> Also, prefer commit IDs to links to the ML archive. The above would
> sentence would give:
>
> "
> As an alternative explanation, I noticed this was fixed in the past by
> 87e2ed338f1b ("clk: at91: fix recalc_rate implementation of PLL
> driver") but the bug was later re-introduced by 1bdf02326b71 ("clk:
> at91: make use of syscon/regmap internally").
> "
>
>
> The following comment and the changelog should be placed after the
> '---' line, so that it's not part of the commit message.
>
> > Thank you for bearing with me about this Boris.
> >
> > Changes since V2:
> > Removed all logging/debug messages I added
> > > Comment by Boris Brezillon about my fix being wrong addressed
> > Changes since V1:
> > Added patch set cover letter
> > Shortened lines which were over >80 characters long
> > > Comment by Greg Kroah-Hartman about "from" field in email addressed
> > > Comment by Alan Stern about redundant debug lines addressed
> >
>
> You should add Fixes and Cc-stable tags so that the fix is backported
> to stable branches:
>
> Fixes: 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally)
> Cc:
>
> > Signed-off-by: Marcin Ziemianowicz
> > ---
> > drivers/clk/at91/clk-pll.c | 13 +
> > 1 file changed, 1 insertion(+), 12 deletions(-)
> >
> > diff --git a/drivers/clk/at91/clk-pll.c b/drivers/clk/at91/clk-pll.c
> > index 7d3223fc..cc6e0364 100644
> > --- a/drivers/clk/at91/clk-pll.c
> > +++ b/drivers/clk/at91/clk-pll.c
> > @@ -132,19 +132,8 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw
> > *hw,
> > unsigned long parent_rate)
> > {
> > struct clk_pll *pll = to_clk_pll(hw);
> > - unsigned int pllr;
> > - u16 mul;
> > - u8 div;
> > -
> > - regmap_read(pll->regmap, PLL_REG(pll->id), &pllr);
> > -
> > - div = PLL_DIV(pllr);
> > - mul = PLL_MUL(pllr, pll->layout);
> > -
> > - if (!div || !mul)
> > - return 0;
> >
> > - return (parent_rate / div) * (mul + 1);
> > + return return (parent_rate / pll->div) * (pll->mul + 1);
Oops, one too many return as reported by kbuild test robot.
>
> The fix looks good. Let me know if you struggle with git
> format-patch/send-email and I'll try to help you (or send the patch for
> you if you don't care learning the process, but I think it's better if
> you learn how to submit patches).
>
> Thanks,
>
> Boris
>
> > }
> >
> > static long clk_pll_get_best_div_mul(struct clk_pll *pll, unsigned long
> > rate,
>
Re: [PATCH v3] clk: at91: PLL recalc_rate() now using cached MUL+DIV values
Hi Marcin,
On Fri, 27 Apr 2018 13:56:09 -0400
Marcin Ziemianowicz wrote:
> Stephen Boyd ,
> linux-...@vger.kernel.org,
> linux-arm-ker...@lists.infradead.org,
> linux-kernel@vger.kernel.org
> Bcc:
> Subject: [PATCH v3] clk: at91: PLL recalc_rate() now using cached MUL and DIV
> values
> Reply-To:
Hm, I don't know how you prepared and sent your patch, but you shouldn't
have these fields in the body of your email. Please use git format-patch
to prepare the patch and then git send-email to send it.
>
> When a USB device is connected to the USB host port on the SAM9N12 then
> you get "-62" error which seems to indicate USB replies from the device
> are timing out. Based on a logic sniffer, I saw the USB bus was running
> at half speed.
>
> The PLL code uses cached MUL and DIV values which get set in set_rate()
> and applied in prepare(), but the recalc_rate() function instead
> queries the hardware instead of using these cached values. Therefore,
> if recalc_rate() is called between a set_rate() and prepare(), the
> wrong frequency is calculated and later the USB clock divider for the
> SAM9N12 SOC will be configured for an incorrect clock.
>
> In my case, the PLL hardware was set to 96 Mhz before the OHCI
> driver loads, and therefore the usb clock divider was being set
> to /2 even though the OHCI driver set the PLL to 48 Mhz.
>
> As an alternative explanation, I noticed this was fixed in the past:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/283502.html
> but was later changed back via a large patch (maybe by mistake?):
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1bdf02326b71eae7e9b4b335b881856aaf9d1af6
Yep, probably by mistake. I started this rework long before it has been
submitted to the ML, so I probably messed something up when rebasing.
Also, prefer commit IDs to links to the ML archive. The above would
sentence would give:
"
As an alternative explanation, I noticed this was fixed in the past by
87e2ed338f1b ("clk: at91: fix recalc_rate implementation of PLL
driver") but the bug was later re-introduced by 1bdf02326b71 ("clk:
at91: make use of syscon/regmap internally").
"
The following comment and the changelog should be placed after the
'---' line, so that it's not part of the commit message.
> Thank you for bearing with me about this Boris.
>
> Changes since V2:
> Removed all logging/debug messages I added
> > Comment by Boris Brezillon about my fix being wrong addressed
> Changes since V1:
> Added patch set cover letter
> Shortened lines which were over >80 characters long
> > Comment by Greg Kroah-Hartman about "from" field in email addressed
> > Comment by Alan Stern about redundant debug lines addressed
>
You should add Fixes and Cc-stable tags so that the fix is backported
to stable branches:
Fixes: 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally)
Cc:
> Signed-off-by: Marcin Ziemianowicz
> ---
> drivers/clk/at91/clk-pll.c | 13 +
> 1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/drivers/clk/at91/clk-pll.c b/drivers/clk/at91/clk-pll.c
> index 7d3223fc..cc6e0364 100644
> --- a/drivers/clk/at91/clk-pll.c
> +++ b/drivers/clk/at91/clk-pll.c
> @@ -132,19 +132,8 @@ static unsigned long clk_pll_recalc_rate(struct clk_hw
> *hw,
>unsigned long parent_rate)
> {
> struct clk_pll *pll = to_clk_pll(hw);
> - unsigned int pllr;
> - u16 mul;
> - u8 div;
> -
> - regmap_read(pll->regmap, PLL_REG(pll->id), &pllr);
> -
> - div = PLL_DIV(pllr);
> - mul = PLL_MUL(pllr, pll->layout);
> -
> - if (!div || !mul)
> - return 0;
>
> - return (parent_rate / div) * (mul + 1);
> + return return (parent_rate / pll->div) * (pll->mul + 1);
The fix looks good. Let me know if you struggle with git
format-patch/send-email and I'll try to help you (or send the patch for
you if you don't care learning the process, but I think it's better if
you learn how to submit patches).
Thanks,
Boris
> }
>
> static long clk_pll_get_best_div_mul(struct clk_pll *pll, unsigned long rate,
Re: [PATCH] mtd: nftl: Remove VLA usage
Hi Kees,
On Mon, 23 Apr 2018 13:35:00 -0700
Kees Cook wrote:
> On the quest to remove all VLAs from the kernel[1] this changes the
> check_free_sectors() routine to use the same stack buffer for both
> data byte checks (SECTORSIZE) and oob byte checks (oobsize). Since
> these regions aren't needed at the same time, they don't need to be
> consecutively allocated. Additionally, while it's possible for oobsize
> to be large, it is unlikely to be larger than the actual SECTORSIZE. As
> such, remove the VLA, adjust offsets and add a sanity check to make sure
> we never get a pathological oobsize.
>
> [1]
> https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
>
> Signed-off-by: Kees Cook
> ---
> drivers/mtd/inftlmount.c | 11 ---
> drivers/mtd/nftlmount.c | 11 ---
> 2 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mtd/inftlmount.c b/drivers/mtd/inftlmount.c
> index aab4f68bd36f..9cdae7f0fc2e 100644
> --- a/drivers/mtd/inftlmount.c
> +++ b/drivers/mtd/inftlmount.c
> @@ -334,7 +334,7 @@ static int memcmpb(void *a, int c, int n)
> static int check_free_sectors(struct INFTLrecord *inftl, unsigned int
> address,
> int len, int check_oob)
> {
> - u8 buf[SECTORSIZE + inftl->mbd.mtd->oobsize];
> + u8 buf[SECTORSIZE];
Could we instead move to dynamic allocation. I mean, SECTORSIZE is 512
bytes, so only with this function we consume 1/16 of the stack. Not to
mention that some MTD drivers might want to do DMA on buffer passed by
the MTD user, and if the buffer is on the stack they'll have to use a
bounce buffer instead.
Regards,
Boris
> struct mtd_info *mtd = inftl->mbd.mtd;
> size_t retlen;
> int i;
> @@ -346,10 +346,15 @@ static int check_free_sectors(struct INFTLrecord
> *inftl, unsigned int address,
> return -1;
>
> if (check_oob) {
> + if (mtd->oobsize > sizeof(buf)) {
> + pr_warn("MTD oobsize > SECTORSIZE: %d\n",
> + mtd->oobsize);
> + return -1;
> + }
> if(inftl_read_oob(mtd, address, mtd->oobsize,
> - &retlen, &buf[SECTORSIZE]) < 0)
> + &retlen, buf) < 0)
> return -1;
> - if (memcmpb(buf + SECTORSIZE, 0xff, mtd->oobsize) != 0)
> + if (memcmpb(buf, 0xff, mtd->oobsize) != 0)
> return -1;
> }
> address += SECTORSIZE;
> diff --git a/drivers/mtd/nftlmount.c b/drivers/mtd/nftlmount.c
> index a6fbfa4e5799..e6eba7f3fdf5 100644
> --- a/drivers/mtd/nftlmount.c
> +++ b/drivers/mtd/nftlmount.c
> @@ -272,7 +272,7 @@ static int memcmpb(void *a, int c, int n)
> static int check_free_sectors(struct NFTLrecord *nftl, unsigned int address,
> int len,
> int check_oob)
> {
> - u8 buf[SECTORSIZE + nftl->mbd.mtd->oobsize];
> + u8 buf[SECTORSIZE];
> struct mtd_info *mtd = nftl->mbd.mtd;
> size_t retlen;
> int i;
> @@ -284,10 +284,15 @@ static int check_free_sectors(struct NFTLrecord *nftl,
> unsigned int address, int
> return -1;
>
> if (check_oob) {
> + if (mtd->oobsize > sizeof(buf)) {
> + pr_warn("MTD oobsize > SECTORSIZE: %d\n",
> + mtd->oobsize);
> + return -1;
> + }
> if(nftl_read_oob(mtd, address, mtd->oobsize,
> - &retlen, &buf[SECTORSIZE]) < 0)
> + &retlen, buf) < 0)
> return -1;
> - if (memcmpb(buf + SECTORSIZE, 0xff, mtd->oobsize) != 0)
> + if (memcmpb(buf, 0xff, mtd->oobsize) != 0)
> return -1;
> }
> address += SECTORSIZE;
[GIT PULL] mtd: Fixes for 4.17-rc3
Hello Linus,
Here is the MTD fixes PR for 4.17-rc3.
Regards,
Boris
The following changes since commit 60cc43fc888428bb2f18f08997432d426a243338:
Linux 4.17-rc1 (2018-04-15 18:24:20 -0700)
are available in the git repository at:
git://git.infradead.org/linux-mtd.git tags/mtd/fixes-for-4.17-rc3
for you to fetch changes up to f6997bec6af43396ff530caee79e178d32774a49:
mtd: rawnand: marvell: fix the chip-select DT parsing logic (2018-04-26
19:06:42 +0200)
* Fix nanddev_mtd_erase() function to match the changes done in
e7bfb3fdbde3 ("mtd: Stop updating erase_info->state and calling
mtd_erase_callback()")
* Fix a memory leak in the Tango NAND controller driver
* Fix read/write to a suspended erase block in the CFI driver
* Fix the DT parsing logic in the Marvell NAND controller driver
----
Boris Brezillon (1):
mtd: nand: Fix nanddev_mtd_erase()
Joakim Tjernlund (3):
mtd: cfi: cmdset_0001: Do not allow read/write to suspend erase block.
mtd: cfi: cmdset_0001: Workaround Micron Erase suspend bug.
mtd: cfi: cmdset_0002: Do not allow read/write to suspend erase block.
Marc Gonzalez (1):
mtd: rawnand: tango: Fix struct clk memory leak
Miquel Raynal (1):
mtd: rawnand: marvell: fix the chip-select DT parsing logic
Thor Thayer (1):
mtd: spi-nor: cadence-quadspi: Fix page fault kernel panic
drivers/mtd/chips/cfi_cmdset_0001.c | 33 -
drivers/mtd/chips/cfi_cmdset_0002.c | 9 ++---
drivers/mtd/nand/core.c | 3 ---
drivers/mtd/nand/raw/marvell_nand.c | 25 -
drivers/mtd/nand/raw/tango_nand.c | 2 +-
drivers/mtd/spi-nor/cadence-quadspi.c | 19 +--
include/linux/mtd/flashchip.h | 1 +
7 files changed, 61 insertions(+), 31 deletions(-)
Re: [PATCH] mtd: partitions: Handle add_mtd_device() failures gracefully
On Thu, 26 Apr 2018 19:56:58 +0200
Geert Uytterhoeven wrote:
> Hi Boris,
>
> On Thu, Apr 26, 2018 at 7:53 PM, Boris Brezillon
> wrote:
> > On Tue, 10 Apr 2018 15:26:20 +0200
> > Geert Uytterhoeven wrote:
> >> On Mon, Apr 9, 2018 at 11:59 PM, Marek Vasut
> >> wrote:
> >> > On 04/09/2018 02:25 PM, Geert Uytterhoeven wrote:
> >> >> Currently add_mtd_device() failures are plainly ignored, which may lead
> >> >> to kernel crashes later.
> >>
> >> >> Fix this by ignoring and freeing partitions that failed to add in
> >> >> add_mtd_partitions(). The same issue is present in mtd_add_partition(),
> >> >> so fix that as well.
> >> >>
> >> >> Signed-off-by: Geert Uytterhoeven
> >> >> ---
> >> >> I don't know if it is worthwhile factoring out the common handling.
> >> >>
> >> >> Should allocate_partition() fail instead? There's a comment saying
> >> >> "let's register it anyway to preserve ordering".
> >>
> >> >> --- a/drivers/mtd/mtdpart.c
> >> >> +++ b/drivers/mtd/mtdpart.c
> >>
> >> >> @@ -746,7 +753,15 @@ int add_mtd_partitions(struct mtd_info *master,
> >> >> list_add(&slave->list, &mtd_partitions);
> >> >> mutex_unlock(&mtd_partitions_mutex);
> >> >>
> >> >> - add_mtd_device(&slave->mtd);
> >> >> + ret = add_mtd_device(&slave->mtd);
> >> >> + if (ret) {
> >> >> + mutex_lock(&mtd_partitions_mutex);
> >> >> + list_del(&slave->list);
> >> >> + mutex_unlock(&mtd_partitions_mutex);
> >> >> + free_partition(slave);
> >> >> + continue;
> >> >> + }
> >> >
> >> > Why is the partition even in the list in the first place ? Can we avoid
> >> > adding it rather than adding and removing it ?
> >>
> >> Hence my question "Should allocate_partition() fail instead?".
> >
> > I'd prefer this option too. Can you prepare a new version doing that?
>
> OK, then I have another question ;-)
>
> Should this be a special failure, so all other valid partitions on the
> same FLASH
> are still added, or should it be fatal, so no partitions are added at all?
I guess we can go for the "drop the invalid partitions and print a
warning" approach. Anyway, I'm sure people will notice really quickly
when one of their partition is missing, so it's not a big deal IMO.
