Received: by 2002:ad5:474a:0:0:0:0:0 with SMTP id i10csp4305874imu; Mon, 12 Nov 2018 08:55:05 -0800 (PST) X-Google-Smtp-Source: AJdET5fenjKxPXfKb1/2goxxmTFkKGJP+vUociEgB8HPVJCj6a4izvD653yAVIvNNrk2aftSUlaw X-Received: by 2002:a17:902:4083:: with SMTP id c3-v6mr1649886pld.181.1542041705505; Mon, 12 Nov 2018 08:55:05 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1542041705; cv=none; d=google.com; s=arc-20160816; b=K3UNaI4nP4MuMRpJPjLhtMAshn0EAD8QRFopeYfflOyucO7TloZ+4/WY+pzW5F3NEP 4dJuHyD1jDXrhGr8tHZW7R/NBr1LRddluPCckzGHxeC9/6NuxdytAL1km4Vm7CTYCX9b GoeaYoaaAkDaQR7maRAFE5MWzSZI3VrVmyotYa7q3Oyeeh+JElqYBWC91yujeS/tcrxr rLQlbQLs/qfJL03a0f+kjSzYEjZQjz0AHf9+AJNC7B3k5H3k+57EbcGMmAG+UD9KIaPz rpBlnFGM4UA9EypSpd6ZaTtExHWH8SK2qugSWIY3T64CdD8Ln3wBj2cU11caMylHBeLR 6T6w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :references:in-reply-to:message-id:subject:cc:to:from:date; bh=A5zWwXh2sJv0v7hQeEXeoKFzbs7/2kwtT8WXuTSLc9E=; b=fJsjSj1R/upC4dkXYY/xJKkUwSAHEZCkLbM1z0NqyMRlgSK0xDp/9lxpCib1tPJuta KtWlJzu3VAeMb/L2aa2A1VQClrzfCfuuSON8SvAEo1fcTteyoMuFTfX5VbAjnfEL9z6g d7NbdhL97PSOJfMD2PbVV4fS2RKXq6/aswTyo28/jay3aeYRRNwMY+3m4BpaZe86YlCs K/xfwr+89XAAWTmWq7ttgXLEuwfc2uuPoIcwz7dCnk1iUpehIixlu7trneGbC1NyLXoY j/E81YsqFo8SxTVunDXz6xi9MHLBXW4PBa04KVFGSBw9griqlwX0qgJ4zXPZNXMLMuzr fJEA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id o28si14884617pgm.238.2018.11.12.08.54.49; Mon, 12 Nov 2018 08:55:05 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729786AbeKMCsX (ORCPT + 99 others); Mon, 12 Nov 2018 21:48:23 -0500 Received: from mail.bootlin.com ([62.4.15.54]:53102 "EHLO mail.bootlin.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727530AbeKMCsX (ORCPT ); Mon, 12 Nov 2018 21:48:23 -0500 Received: by mail.bootlin.com (Postfix, from userid 110) id 5067B207BD; Mon, 12 Nov 2018 17:54:17 +0100 (CET) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on mail.bootlin.com X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=ALL_TRUSTED,SHORTCIRCUIT, URIBL_BLOCKED shortcircuit=ham autolearn=disabled version=3.4.2 Received: from bbrezillon (91-160-177-164.subs.proxad.net [91.160.177.164]) by mail.bootlin.com (Postfix) with ESMTPSA id B52AE20701; Mon, 12 Nov 2018 17:54:16 +0100 (CET) Date: Mon, 12 Nov 2018 17:54:16 +0100 From: Boris Brezillon To: Miquel Raynal 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 , Wolfram Sang Subject: Re: [PATCH v6 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller Message-ID: <20181112175416.247e3203@bbrezillon> In-Reply-To: <20181112171351.4ac3506b@xps13> 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> <20181112171351.4ac3506b@xps13> X-Mailer: Claws Mail 3.16.0 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org +Wolfram to give some inputs on the DMA issue. On Mon, 12 Nov 2018 17:13:51 +0100 Miquel Raynal wrote: > 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: > > > > > On 2018/11/6 17:28, Boris Brezillon wrote: > > > > On Tue, 6 Nov 2018 17:08:00 +0800 > > > > Liang Yang wrote: > > > > > > > >> On 2018/11/5 23:53, Boris Brezillon wrote: > > > >>> On Fri, 2 Nov 2018 00:42:21 +0800 > > > >>> Jianxin Pan wrote: > > > >>> > > > >>>> + > > > >>>> +static inline u8 meson_nfc_read_byte(struct mtd_info *mtd) > > > >>>> +{ > > > >>>> + struct nand_chip *nand = mtd_to_nand(mtd); > > > >>>> + struct meson_nfc *nfc = nand_get_controller_data(nand); > > > >>>> + u32 cmd; > > > >>>> + > > > >>>> + cmd = nfc->param.chip_select | NFC_CMD_DRD | 0; > > > >>>> + writel(cmd, nfc->reg_base + NFC_REG_CMD); > > > >>>> + > > > >>>> + meson_nfc_drain_cmd(nfc); > > > >>> > > > >>> You probably don't want to drain the FIFO every time you read a byte 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. > > > >>> > > > >> Register 'NFC_REG_BUF' can holds only 4 bytes, also DRD sends only one > > > >> nand cycle to read one byte and covers the 1st byte every time reading. > > > >> i think nfc controller is faster than nand cycle, but really it is not > > > >> high efficiency when reading so many bytes once. > > > >> Or use dma command here like read_page and read_page_raw. > > > > > > > > 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. > > > > > > > ok, i will try dma here. > > > > We should probably expose the bounce buf handling as generic helpers at > > the rawnand level: > > > > void *nand_op_get_dma_safe_input_buf(struct nand_op_instr *instr) > > { > > void *buf; > > > > if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR)) > > return NULL; > > > > if (virt_addr_valid(instr->data.in) && > > !object_is_on_stack(instr->data.buf.in)) > > return instr->data.buf.in; > > > > return kzalloc(instr->data.len, GFP_KERNEL); > > } > > > > void nand_op_put_dma_safe_input_buf(struct nand_op_instr *instr, > > void *buf) > > { > > if (WARN_ON(instr->type != NAND_OP_DATA_IN_INSTR) || > > WARN_ON(!buf)) > > return; > > > > if (buf == instr->data.buf.in) > > return; > > > > memcpy(instr->data.buf.in, buf, instr->data.len); > > kfree(buf); > > } > > > > const void *nand_op_get_dma_safe_output_buf(struct nand_op_instr *instr) > > { > > void *buf; > > > > if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR)) > > return NULL; > > > > if (virt_addr_valid(instr->data.out) && > > !object_is_on_stack(instr->data.buf.out)) > > return instr->data.buf.out; > > > > return kmemdup(instr->data.buf.out, GFP_KERNEL); > > } > > > > void nand_op_put_dma_safe_output_buf(struct nand_op_instr *instr, > > void *buf) > > { > > if (WARN_ON(instr->type != NAND_OP_DATA_OUT_INSTR) || > > WARN_ON(!buf)) > > return; > > > > if (buf != 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). It's not 100% reliable, but it should cover most cases. Note that the NAND framework already uses virt_addr_valid() to decide when to use its internal bounce buffer, so this should be fixed too if we want a fully reliable solution. > This is my understanding of Wolfram's recent talk > at ELCE [1]. Yes, you're right, but the NAND framework does not provide any guarantee on the buf passed to ->exec_op() simply because the MTD layer does not provide such a guarantee. Reworking that to match how the i2c framework handles it is possible (with a flag set when the buffer is known to be DMA-safe), but it requires rewriting all MTD users if we want to keep decent perfs (the amount of data transfered to a flash is an order of magnitude bigger than what you usually receive/send from/to an I2C device). Also, I'm not even sure the DMA_SAFE flag covers all weird cases like the "DMA engine embedded in the controller is not able to access the whole physical memory range" one. So ideally we should have something that checks if a pointer is DMA-safe at the device level and then at the arch level. A temporary solution would be to add a hook at the nand_controller level: bool (*buf_is_dma_safe)(struct nand_chip *chip, void *buf, size_t len); And then fallback to the default implementation when it's not implemented: static bool nand_buf_is_dma_safe(struct nand_chip *chip, void *buf, size_t len) { if (chip->controller->ops && chip->controller->ops->is_dma_safe) return chip->controller->ops->is_dma_safe(chip, buf, len); return virt_addr_valid(buf) && !object_is_on_stack(buf); } > I suppose using the CONFIG_DMA_API_DEBUG option could help > more reliably to find such issues. Actually, the problem is not only about detecting offenders but being able to detect when a buffer is not DMA-safe at runtime in order to allocate/use a bounce buffer.