From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Mon, 12 Nov 2018 17:13:51 +0100 From: Miquel Raynal To: Boris Brezillon Cc: Liang Yang , Rob Herring , Hanjie Lin , Victor Wan , Jianxin Pan , Neil Armstrong , Martin Blumenstingl , Richard Weinberger , Yixun Lan , linux-kernel@vger.kernel.org, Marek Vasut , Jian Hu , linux-mtd@lists.infradead.org, Kevin Hilman , Carlo Caione , linux-amlogic@lists.infradead.org, Brian Norris , David Woodhouse , linux-arm-kernel@lists.infradead.org, Jerome Brunet Subject: Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller Message-ID: <20181112171351.4ac3506b@xps13> In-Reply-To: <20181106112206.65a70a81@bbrezillon> References: <1541090542-19618-1-git-send-email-jianxin.pan@amlogic.com> <1541090542-19618-3-git-send-email-jianxin.pan@amlogic.com> <20181105165321.7ea2b45f@bbrezillon> <20181106102851.61deb97a@bbrezillon> <20181106112206.65a70a81@bbrezillon> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Hello, Boris Brezillon wrote on Tue, 6 Nov 2018 11:22:06 +0100: > On Tue, 6 Nov 2018 18:00:37 +0800 > Liang Yang wrote: >=20 > > On 2018/11/6 17:28, Boris Brezillon wrote: =20 > > > On Tue, 6 Nov 2018 17:08:00 +0800 > > > Liang Yang wrote: > > > =20 > > >> On 2018/11/5 23:53, Boris Brezillon wrote: =20 > > >>> On Fri, 2 Nov 2018 00:42:21 +0800 > > >>> Jianxin Pan wrote: > > >>> =20 > > >>>> + > > >>>> +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd) > > >>>> +{ > > >>>> + struct nand_chip *nand =3D mtd_to_nand(mtd); > > >>>> + struct meson_nfc *nfc =3D nand_get_controller_data(nand); > > >>>> + u32 cmd; > > >>>> + > > >>>> + cmd =3D nfc->param.chip_select | NFC_CMD_DRD | 0; > > >>>> + writel(cmd, nfc->reg_base + NFC_REG_CMD); > > >>>> + > > >>>> + meson_nfc_drain_cmd(nfc); =20 > > >>> > > >>> You probably don't want to drain the FIFO every time you read a byt= e on > > >>> the bus, and I guess the INPUT FIFO is at least as big as the CMD > > >>> FIFO, right? If that's the case, you should queue as much DRD cmd as > > >>> possible and only sync when the user explicitly requests it or when > > >>> the INPUT/READ FIFO is full. > > >>> =20 > > >> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only o= ne > > >> nand cycle to read one byte and covers the 1st byte every time readi= ng. > > >> i think nfc controller is faster than nand cycle, but really it is n= ot > > >> high efficiency when reading so many bytes once. > > >> Or use dma command here like read_page and read_page_raw. =20 > > >=20 > > > Yep, that's also an alternative, though you'll have to make sure the > > > buffer passed through the nand_op_inst is DMA-safe, and use a bounce > > > buffer when that's not the case. > > > =20 > > ok, i will try dma here. =20 >=20 > We should probably expose the bounce buf handling as generic helpers at > the rawnand level: >=20 > void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr) > { > void *buf; >=20 > if (WARN_ON(instr->type !=3D NAND_OP_DATA_IN_INSTR)) > return NULL; >=20 > if (virt_addr_valid(instr->data.in) && > !object_is_on_stack(instr->data.buf.in)) > return instr->data.buf.in; >=20 > return kzalloc(instr->data.len, GFP_KERNEL); > } >=20 > void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr, > void *buf) > { > if (WARN_ON(instr->type !=3D NAND_OP_DATA_IN_INSTR) || > WARN_ON(!buf)) > return; >=20 > if (buf =3D=3D instr->data.buf.in) > return; >=20 > memcpy(instr->data.buf.in, buf, instr->data.len); > kfree(buf); > } >=20 > const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr) > { > void *buf; >=20 > if (WARN_ON(instr->type !=3D NAND_OP_DATA_OUT_INSTR)) > return NULL; >=20 > if (virt_addr_valid(instr->data.out) && > !object_is_on_stack(instr->data.buf.out)) > return instr->data.buf.out; >=20 > return kmemdup(instr->data.buf.out, GFP_KERNEL); > } >=20 > void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr, > void *buf) > { > if (WARN_ON(instr->type !=3D NAND_OP_DATA_OUT_INSTR) || > WARN_ON(!buf)) > return; >=20 > if (buf !=3D instr->data.buf.out) > kfree(buf); > } Not that I am against such function, but maybe they should come with comments stating that there is no reliable way to find if a buffer is DMA-able at runtime and these are just sanity checks (ie. required, but probably not enough). This is my understanding of Wolfram's recent talk at ELCE [1]. I suppose using the CONFIG_DMA_API_DEBUG option could help more reliably to find such issues. [1] https://www.youtube.com/watch?v=3DJDwaMClvV-s Thanks, Miqu=C3=A8l