From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_PASS, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1586AC1B0F7 for ; Fri, 18 Jan 2019 22:07:54 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id BFAB320883 for ; Fri, 18 Jan 2019 22:07:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1547849273; bh=QavRa01grMcH9fYsFll0lQKbGayuEhRmqDkBMEKaDsQ=; h=Date:From:To:Cc:Subject:In-Reply-To:References:List-ID:From; b=MRz0OVJxXcfot3BOCX99R30FzmZqoY/XA1C0cFWOfVdcSz/LhDEmxXVgbsLS9IGFG exc0eSlguQVVohrXWTZSerDn38ejoPB+vA0wgXP0bCp9vMrog3+xJq0F/FIOfDZrhw 4eRW98MWKb4yfmzo+mlc7PVVHnKHAaomSWSIrGhM= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729960AbfARWHw (ORCPT ); Fri, 18 Jan 2019 17:07:52 -0500 Received: from mail.kernel.org ([198.145.29.99]:44168 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729621AbfARWHv (ORCPT ); Fri, 18 Jan 2019 17:07:51 -0500 Received: from bbrezillon (unknown [91.160.177.164]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id DD2282087E; Fri, 18 Jan 2019 22:07:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1547849270; bh=QavRa01grMcH9fYsFll0lQKbGayuEhRmqDkBMEKaDsQ=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=KeLxHQGUMNHcueutB6/abyrMaV0Sd6xaEWmjcTjncWtAahtPTKhELQCBEI3Kw9xVN jN2YTTrByedZcKaMjbHSVlIF2ZEG68VgTha4NmUtJBynW7YynRpKypzIcUhymMOBeM r/LMI33EWcAQ99uxjLOB1oRuYdQT2lYW+P/hHMH0= Date: Fri, 18 Jan 2019 23:07:40 +0100 From: Boris Brezillon To: Geert Uytterhoeven Cc: Arnd Bergmann , Greg Kroah-Hartman , linux-mtd@lists.infradead.org, Nguyen An Hoan , linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org Subject: Re: [PATCH v2 0/2] eeprom: at25: SPI transfer improvements Message-ID: <20190118230740.44239fcf@bbrezillon> In-Reply-To: <20190118140525.29189-1-geert+renesas@glider.be> References: <20190118140525.29189-1-geert+renesas@glider.be> 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 Hi Geert, On Fri, 18 Jan 2019 15:05:23 +0100 Geert Uytterhoeven wrote: > Hi all, > > This patch series contains two improvements for the AT25 SPI EEPROM > driver, related to SPI transfers. > > Changes compared to v1: > - Merge "off" and "offset" into a single variable instead of just > killing the cast, as suggested by Arnd, > - Add Acked-by, > - Dropped "[PATCH 3/3] eeprom: at25: Split writes in two SPI transfers > to optimize DMA", as this is better implemented in the SPI > controller driver (cfr. e.g. "[v2 PATCH 2/2] spi: sh-msiof: Use DMA > if possible", > https://lore.kernel.org/linux-renesas-soc/1547803771-9564-2-git-send-email-na-hoan@jinso.co.jp/) > > Tested on a Renesas Ebisu development board with R-Car E3 using MSIOF > and a 25LC040 EEPROM. Did you consider converting this driver to spimem? Looks like the protocol used to communicate with the memory resembles the one used on SPI NANDs/NORs and fits pretty well in the spi_mem_op representation. By doing this conversion you'd allow people to connect an AT25 EEPROM to an advanced SPI controller that does not support regular SPI transfers and you wouldn't have to forge SPI messages manually. Here is a patch (only compile tested) doing that. The diffstat is not in favor of this conversion, but I find the resulting code cleaner and more future proof. Regards, Boris drivers/misc/eeprom/at25.c | 302 ++++++++++++++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 198 insertions(+), 104 deletions(-) --->8--- diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c index 99de6939cd5a..b9697a27e69b 100644 --- a/drivers/misc/eeprom/at25.c +++ b/drivers/misc/eeprom/at25.c @@ -17,7 +17,7 @@ #include #include -#include +#include #include #include @@ -29,12 +29,17 @@ */ struct at25_data { - struct spi_device *spi; + struct spi_mem *spimem; struct mutex lock; struct spi_eeprom chip; unsigned addrlen; struct nvmem_config nvmem_config; struct nvmem_device *nvmem; + void *scratchbuf; + struct { + struct spi_mem_dirmap_desc *rdesc[2]; + struct spi_mem_dirmap_desc *wdesc[2]; + } dirmap; }; #define AT25_WREN 0x06 /* latch the write enable */ @@ -63,17 +68,117 @@ struct at25_data { #define io_limit PAGE_SIZE /* bytes */ +static void at25_destroy_dirmaps(struct at25_data *at25) +{ + if (at25->dirmap.rdesc[0]) + spi_mem_dirmap_destroy(at25->dirmap.rdesc[0]); + if (at25->dirmap.rdesc[1]) + spi_mem_dirmap_destroy(at25->dirmap.rdesc[1]); + if (at25->dirmap.wdesc[0]) + spi_mem_dirmap_destroy(at25->dirmap.wdesc[0]); + if (at25->dirmap.wdesc[1]) + spi_mem_dirmap_destroy(at25->dirmap.wdesc[1]); +} + +static int at25_create_dirmaps(struct at25_data *at25) +{ + struct spi_mem_dirmap_info info = { + .op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(AT25_READ, 1), + SPI_MEM_OP_ADDR(at25->addrlen, 0, 1), + SPI_MEM_OP_NO_DUMMY, + SPI_MEM_OP_DATA_IN(0, NULL, 1)), + .offset = 0, + .length = at25->chip.byte_len, + }; + struct spi_mem_dirmap_desc *desc; + int ret; + + if (info.length > 1 << at25->addrlen) + info.length = at25->chip.byte_len; + + desc = spi_mem_dirmap_create(at25->spimem, &info); + if (IS_ERR(desc)) + return PTR_ERR(desc); + + at25->dirmap.rdesc[0] = desc; + + info.op_tmpl.cmd.opcode = AT25_WRITE; + info.op_tmpl.data.dir = SPI_MEM_DATA_OUT; + + desc = spi_mem_dirmap_create(at25->spimem, &info); + if (IS_ERR(desc)) { + ret = PTR_ERR(desc); + goto err_destroy_dirmaps; + } + + at25->dirmap.wdesc[0] = desc; + + if (!(at25->chip.flags & EE_INSTR_BIT3_IS_ADDR)) + return 0; + + info.offset = 256; + info.length = at25->chip.byte_len - 256; + info.op_tmpl.cmd.opcode = AT25_READ | AT25_INSTR_BIT3; + info.op_tmpl.data.dir = SPI_MEM_DATA_IN; + + desc = spi_mem_dirmap_create(at25->spimem, &info); + if (IS_ERR(desc)) { + ret = PTR_ERR(desc); + goto err_destroy_dirmaps; + } + + at25->dirmap.rdesc[1] = desc; + + info.op_tmpl.cmd.opcode = AT25_WRITE | AT25_INSTR_BIT3; + info.op_tmpl.data.dir = SPI_MEM_DATA_OUT; + + desc = spi_mem_dirmap_create(at25->spimem, &info); + if (IS_ERR(desc)) { + ret = PTR_ERR(desc); + goto err_destroy_dirmaps; + } + + at25->dirmap.wdesc[1] = desc; + + return 0; + +err_destroy_dirmaps: + at25_destroy_dirmaps(at25); + + return ret; +} + +static int at25_rdsr(struct at25_data *at25) +{ + struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(AT25_RDSR, 1), + SPI_MEM_OP_NO_ADDR, + SPI_MEM_OP_NO_DUMMY, + SPI_MEM_OP_DATA_IN(1, at25->scratchbuf, 1)); + int ret; + + ret = spi_mem_exec_op(at25->spimem, &op); + if (ret) + return ret; + + return *((u8 *)at25->scratchbuf); +} + +static int at25_wren(struct at25_data *at25) +{ + struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(AT25_WREN, 1), + SPI_MEM_OP_NO_ADDR, + SPI_MEM_OP_NO_DUMMY, + SPI_MEM_OP_NO_DATA); + + return spi_mem_exec_op(at25->spimem, &op); +} + static int at25_ee_read(void *priv, unsigned int offset, void *val, size_t count) { + struct spi_mem_dirmap_desc *desc; struct at25_data *at25 = priv; - char *buf = val; - u8 command[EE_MAXADDRLEN + 1]; - u8 *cp; ssize_t status; - struct spi_transfer t[2]; - struct spi_message m; - u8 instr; if (unlikely(offset >= at25->chip.byte_len)) return -EINVAL; @@ -82,37 +187,10 @@ static int at25_ee_read(void *priv, unsigned int offset, if (unlikely(!count)) return -EINVAL; - cp = command; - - instr = AT25_READ; - if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR) - if (offset >= (1U << (at25->addrlen * 8))) - instr |= AT25_INSTR_BIT3; - *cp++ = instr; - - /* 8/16/24-bit address is written MSB first */ - switch (at25->addrlen) { - default: /* case 3 */ - *cp++ = offset >> 16; - /* fall through */ - case 2: - *cp++ = offset >> 8; - /* fall through */ - case 1: - case 0: /* can't happen: for better codegen */ - *cp++ = offset >> 0; - } - - spi_message_init(&m); - memset(t, 0, sizeof(t)); - - t[0].tx_buf = command; - t[0].len = at25->addrlen + 1; - spi_message_add_tail(&t[0], &m); - - t[1].rx_buf = buf; - t[1].len = count; - spi_message_add_tail(&t[1], &m); + if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR && offset > 255) + desc = at25->dirmap.rdesc[1]; + else + desc = at25->dirmap.rdesc[0]; mutex_lock(&at25->lock); @@ -122,8 +200,8 @@ static int at25_ee_read(void *priv, unsigned int offset, * other devices on the bus need to be accessed regularly or * this chip is clocked very slowly */ - status = spi_sync(at25->spi, &m); - dev_dbg(&at25->spi->dev, "read %zu bytes at %d --> %zd\n", + status = spi_mem_dirmap_read(desc, offset, count, val); + dev_dbg(&at25->spimem->spi->dev, "read %zu bytes at %d --> %zd\n", count, offset, status); mutex_unlock(&at25->lock); @@ -149,7 +227,7 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count) buf_size = at25->chip.page_size; if (buf_size > io_limit) buf_size = io_limit; - bounce = kmalloc(buf_size + at25->addrlen + 1, GFP_KERNEL); + bounce = kmalloc(buf_size, GFP_KERNEL); if (!bounce) return -ENOMEM; @@ -158,47 +236,32 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count) */ mutex_lock(&at25->lock); do { + struct spi_mem_dirmap_desc *desc; unsigned long timeout, retries; unsigned segment; unsigned offset = (unsigned) off; - u8 *cp = bounce; int sr; - u8 instr; - *cp = AT25_WREN; - status = spi_write(at25->spi, cp, 1); + status = at25_wren(at25); if (status < 0) { - dev_dbg(&at25->spi->dev, "WREN --> %d\n", status); + dev_dbg(&at25->spimem->spi->dev, "WREN --> %d\n", + status); break; } - instr = AT25_WRITE; - if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR) - if (offset >= (1U << (at25->addrlen * 8))) - instr |= AT25_INSTR_BIT3; - *cp++ = instr; - - /* 8/16/24-bit address is written MSB first */ - switch (at25->addrlen) { - default: /* case 3 */ - *cp++ = offset >> 16; - /* fall through */ - case 2: - *cp++ = offset >> 8; - /* fall through */ - case 1: - case 0: /* can't happen: for better codegen */ - *cp++ = offset >> 0; - } + if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR && offset > 255) + desc = at25->dirmap.wdesc[1]; + else + desc = at25->dirmap.wdesc[0]; /* Write as much of a page as we can */ segment = buf_size - (offset % buf_size); if (segment > count) segment = count; - memcpy(cp, buf, segment); - status = spi_write(at25->spi, bounce, - segment + at25->addrlen + 1); - dev_dbg(&at25->spi->dev, "write %u bytes at %u --> %d\n", + memcpy(bounce, buf, segment); + status = spi_mem_dirmap_write(desc, offset, segment, bounce); + dev_dbg(&at25->spimem->spi->dev, + "write %u bytes at %u --> %d\n", segment, offset, status); if (status < 0) break; @@ -211,10 +274,9 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count) timeout = jiffies + msecs_to_jiffies(EE_TIMEOUT); retries = 0; do { - - sr = spi_w8r8(at25->spi, AT25_RDSR); + sr = at25_rdsr(at25); if (sr < 0 || (sr & AT25_SR_nRDY)) { - dev_dbg(&at25->spi->dev, + dev_dbg(&at25->spimem->spi->dev, "rdsr --> %d (%02x)\n", sr, sr); /* at HZ=100, this is sloooow */ msleep(1); @@ -225,7 +287,7 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count) } while (retries++ < 3 || time_before_eq(jiffies, timeout)); if ((sr < 0) || (sr & AT25_SR_nRDY)) { - dev_err(&at25->spi->dev, + dev_err(&at25->spimem->spi->dev, "write %u bytes offset %u, timeout after %u msecs\n", segment, offset, jiffies_to_msecs(jiffies - @@ -304,7 +366,7 @@ static int at25_fw_to_chip(struct device *dev, struct spi_eeprom *chip) return 0; } -static int at25_probe(struct spi_device *spi) +static int at25_probe(struct spi_mem *spimem) { struct at25_data *at25 = NULL; struct spi_eeprom chip; @@ -313,12 +375,12 @@ static int at25_probe(struct spi_device *spi) int addrlen; /* Chip description */ - if (!spi->dev.platform_data) { - err = at25_fw_to_chip(&spi->dev, &chip); + if (!spimem->spi->dev.platform_data) { + err = at25_fw_to_chip(&spimem->spi->dev, &chip); if (err) return err; } else - chip = *(struct spi_eeprom *)spi->dev.platform_data; + chip = *(struct spi_eeprom *)spimem->spi->dev.platform_data; /* For now we only support 8/16/24 bit addressing */ if (chip.flags & EE_ADDR1) @@ -328,37 +390,48 @@ static int at25_probe(struct spi_device *spi) else if (chip.flags & EE_ADDR3) addrlen = 3; else { - dev_dbg(&spi->dev, "unsupported address type\n"); + dev_dbg(&spimem->spi->dev, "unsupported address type\n"); return -EINVAL; } - /* Ping the chip ... the status register is pretty portable, - * unlike probing manufacturer IDs. We do expect that system - * firmware didn't write it in the past few milliseconds! - */ - sr = spi_w8r8(spi, AT25_RDSR); - if (sr < 0 || sr & AT25_SR_nRDY) { - dev_dbg(&spi->dev, "rdsr --> %d (%02x)\n", sr, sr); - return -ENXIO; - } - - at25 = devm_kzalloc(&spi->dev, sizeof(struct at25_data), GFP_KERNEL); + at25 = devm_kzalloc(&spimem->spi->dev, sizeof(struct at25_data), GFP_KERNEL); if (!at25) return -ENOMEM; mutex_init(&at25->lock); at25->chip = chip; - at25->spi = spi; - spi_set_drvdata(spi, at25); + at25->spimem = spimem; + spi_mem_set_drvdata(spimem, at25); at25->addrlen = addrlen; - at25->nvmem_config.name = dev_name(&spi->dev); - at25->nvmem_config.dev = &spi->dev; + /* + * Can't be allocated with devm_kmalloc() because we need a DMA-safe + * buffer. + */ + at25->scratchbuf = kmalloc(1, GFP_KERNEL); + + /* Ping the chip ... the status register is pretty portable, + * unlike probing manufacturer IDs. We do expect that system + * firmware didn't write it in the past few milliseconds! + */ + sr = at25_rdsr(at25); + if (sr < 0 || sr & AT25_SR_nRDY) { + dev_dbg(&spimem->spi->dev, "rdsr --> %d (%02x)\n", sr, sr); + err = -ENXIO; + goto err_free_scratchbuf; + } + + err = at25_create_dirmaps(at25); + if (err) + goto err_free_scratchbuf; + + at25->nvmem_config.name = dev_name(&spimem->spi->dev); + at25->nvmem_config.dev = &spimem->spi->dev; at25->nvmem_config.read_only = chip.flags & EE_READONLY; at25->nvmem_config.root_only = true; at25->nvmem_config.owner = THIS_MODULE; at25->nvmem_config.compat = true; - at25->nvmem_config.base_dev = &spi->dev; + at25->nvmem_config.base_dev = &spimem->spi->dev; at25->nvmem_config.reg_read = at25_ee_read; at25->nvmem_config.reg_write = at25_ee_write; at25->nvmem_config.priv = at25; @@ -366,17 +439,38 @@ static int at25_probe(struct spi_device *spi) at25->nvmem_config.word_size = 1; at25->nvmem_config.size = chip.byte_len; - at25->nvmem = devm_nvmem_register(&spi->dev, &at25->nvmem_config); - if (IS_ERR(at25->nvmem)) - return PTR_ERR(at25->nvmem); + at25->nvmem = devm_nvmem_register(&spimem->spi->dev, + &at25->nvmem_config); + if (IS_ERR(at25->nvmem)) { + err = PTR_ERR(at25->nvmem); + goto err_destroy_dirmaps; + } - dev_info(&spi->dev, "%d %s %s eeprom%s, pagesize %u\n", + dev_info(&spimem->spi->dev, "%d %s %s eeprom%s, pagesize %u\n", (chip.byte_len < 1024) ? chip.byte_len : (chip.byte_len / 1024), (chip.byte_len < 1024) ? "Byte" : "KByte", at25->chip.name, (chip.flags & EE_READONLY) ? " (readonly)" : "", at25->chip.page_size); return 0; + +err_destroy_dirmaps: + at25_destroy_dirmaps(at25); + +err_free_scratchbuf: + kfree(at25->scratchbuf); + + return err; +} + +static int at25_remove(struct spi_mem *spimem) +{ + struct at25_data *at25 = spi_mem_get_drvdata(spimem); + + at25_destroy_dirmaps(at25); + kfree(at25->scratchbuf); + + return 0; } /*-------------------------------------------------------------------------*/ @@ -387,15 +481,15 @@ static const struct of_device_id at25_of_match[] = { }; MODULE_DEVICE_TABLE(of, at25_of_match); -static struct spi_driver at25_driver = { - .driver = { - .name = "at25", +static struct spi_mem_driver at25_driver = { + .spidrv.driver = { + .name = "at25", .of_match_table = at25_of_match, }, - .probe = at25_probe, + .probe = at25_probe, + .remove = at25_remove, }; - -module_spi_driver(at25_driver); +module_spi_mem_driver(at25_driver); MODULE_DESCRIPTION("Driver for most SPI EEPROMs"); MODULE_AUTHOR("David Brownell"); From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.1 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DE258C1B0FD for ; Fri, 18 Jan 2019 22:08:02 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id A489A2087E for ; Fri, 18 Jan 2019 22:08:02 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="pOLguNZe"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="KeLxHQGU" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A489A2087E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-mtd-bounces+linux-mtd=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=1HSf2CenKUMpnq5cLxdW34r6JNXpgEquQ6oKDg8BpYA=; b=pOLguNZe2gE1tS NwNVXbUhdA8fiWPt6jd/GIzdCAbXd7Ev6ghaVt2tHpTueIrRE6qIlArOzmMOJAnnXW7H+5UwFsQyk J1wFLef4u1CRYCInRe12PWai9xSjn4tzxOKzTnU5c493xqJK9huAvL8e7pTWfYCm7jBdVPmzcBgCs mP/vVCQLtMn/FV0LUCAHaQp2Rm7cFN/s2C0LZ5lO15obBOzMk6Tts2+fN+a77ng1AH+IQR3uUOrws NqOKOUSc6skxy5eMaiWRwhNOAoVGgigzz2DE4qIt95tu9EQH3buEu77zrcYhvzhFEqQQ8LO3ezrWU iCAaUAvFA1/5NDxNs8dg==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1gkcIs-0001uu-Fk; Fri, 18 Jan 2019 22:07:54 +0000 Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1gkcIo-0001ua-Qt for linux-mtd@lists.infradead.org; Fri, 18 Jan 2019 22:07:53 +0000 Received: from bbrezillon (unknown [91.160.177.164]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id DD2282087E; Fri, 18 Jan 2019 22:07:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1547849270; bh=QavRa01grMcH9fYsFll0lQKbGayuEhRmqDkBMEKaDsQ=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=KeLxHQGUMNHcueutB6/abyrMaV0Sd6xaEWmjcTjncWtAahtPTKhELQCBEI3Kw9xVN jN2YTTrByedZcKaMjbHSVlIF2ZEG68VgTha4NmUtJBynW7YynRpKypzIcUhymMOBeM r/LMI33EWcAQ99uxjLOB1oRuYdQT2lYW+P/hHMH0= Date: Fri, 18 Jan 2019 23:07:40 +0100 From: Boris Brezillon To: Geert Uytterhoeven Subject: Re: [PATCH v2 0/2] eeprom: at25: SPI transfer improvements Message-ID: <20190118230740.44239fcf@bbrezillon> In-Reply-To: <20190118140525.29189-1-geert+renesas@glider.be> References: <20190118140525.29189-1-geert+renesas@glider.be> X-Mailer: Claws Mail 3.16.0 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190118_140750_919740_5A189DA2 X-CRM114-Status: GOOD ( 28.00 ) X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Arnd Bergmann , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org, linux-mtd@lists.infradead.org, Nguyen An Hoan Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-mtd" Errors-To: linux-mtd-bounces+linux-mtd=archiver.kernel.org@lists.infradead.org Hi Geert, On Fri, 18 Jan 2019 15:05:23 +0100 Geert Uytterhoeven wrote: > Hi all, > > This patch series contains two improvements for the AT25 SPI EEPROM > driver, related to SPI transfers. > > Changes compared to v1: > - Merge "off" and "offset" into a single variable instead of just > killing the cast, as suggested by Arnd, > - Add Acked-by, > - Dropped "[PATCH 3/3] eeprom: at25: Split writes in two SPI transfers > to optimize DMA", as this is better implemented in the SPI > controller driver (cfr. e.g. "[v2 PATCH 2/2] spi: sh-msiof: Use DMA > if possible", > https://lore.kernel.org/linux-renesas-soc/1547803771-9564-2-git-send-email-na-hoan@jinso.co.jp/) > > Tested on a Renesas Ebisu development board with R-Car E3 using MSIOF > and a 25LC040 EEPROM. Did you consider converting this driver to spimem? Looks like the protocol used to communicate with the memory resembles the one used on SPI NANDs/NORs and fits pretty well in the spi_mem_op representation. By doing this conversion you'd allow people to connect an AT25 EEPROM to an advanced SPI controller that does not support regular SPI transfers and you wouldn't have to forge SPI messages manually. Here is a patch (only compile tested) doing that. The diffstat is not in favor of this conversion, but I find the resulting code cleaner and more future proof. Regards, Boris drivers/misc/eeprom/at25.c | 302 ++++++++++++++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 198 insertions(+), 104 deletions(-) --->8--- diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c index 99de6939cd5a..b9697a27e69b 100644 --- a/drivers/misc/eeprom/at25.c +++ b/drivers/misc/eeprom/at25.c @@ -17,7 +17,7 @@ #include #include -#include +#include #include #include @@ -29,12 +29,17 @@ */ struct at25_data { - struct spi_device *spi; + struct spi_mem *spimem; struct mutex lock; struct spi_eeprom chip; unsigned addrlen; struct nvmem_config nvmem_config; struct nvmem_device *nvmem; + void *scratchbuf; + struct { + struct spi_mem_dirmap_desc *rdesc[2]; + struct spi_mem_dirmap_desc *wdesc[2]; + } dirmap; }; #define AT25_WREN 0x06 /* latch the write enable */ @@ -63,17 +68,117 @@ struct at25_data { #define io_limit PAGE_SIZE /* bytes */ +static void at25_destroy_dirmaps(struct at25_data *at25) +{ + if (at25->dirmap.rdesc[0]) + spi_mem_dirmap_destroy(at25->dirmap.rdesc[0]); + if (at25->dirmap.rdesc[1]) + spi_mem_dirmap_destroy(at25->dirmap.rdesc[1]); + if (at25->dirmap.wdesc[0]) + spi_mem_dirmap_destroy(at25->dirmap.wdesc[0]); + if (at25->dirmap.wdesc[1]) + spi_mem_dirmap_destroy(at25->dirmap.wdesc[1]); +} + +static int at25_create_dirmaps(struct at25_data *at25) +{ + struct spi_mem_dirmap_info info = { + .op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(AT25_READ, 1), + SPI_MEM_OP_ADDR(at25->addrlen, 0, 1), + SPI_MEM_OP_NO_DUMMY, + SPI_MEM_OP_DATA_IN(0, NULL, 1)), + .offset = 0, + .length = at25->chip.byte_len, + }; + struct spi_mem_dirmap_desc *desc; + int ret; + + if (info.length > 1 << at25->addrlen) + info.length = at25->chip.byte_len; + + desc = spi_mem_dirmap_create(at25->spimem, &info); + if (IS_ERR(desc)) + return PTR_ERR(desc); + + at25->dirmap.rdesc[0] = desc; + + info.op_tmpl.cmd.opcode = AT25_WRITE; + info.op_tmpl.data.dir = SPI_MEM_DATA_OUT; + + desc = spi_mem_dirmap_create(at25->spimem, &info); + if (IS_ERR(desc)) { + ret = PTR_ERR(desc); + goto err_destroy_dirmaps; + } + + at25->dirmap.wdesc[0] = desc; + + if (!(at25->chip.flags & EE_INSTR_BIT3_IS_ADDR)) + return 0; + + info.offset = 256; + info.length = at25->chip.byte_len - 256; + info.op_tmpl.cmd.opcode = AT25_READ | AT25_INSTR_BIT3; + info.op_tmpl.data.dir = SPI_MEM_DATA_IN; + + desc = spi_mem_dirmap_create(at25->spimem, &info); + if (IS_ERR(desc)) { + ret = PTR_ERR(desc); + goto err_destroy_dirmaps; + } + + at25->dirmap.rdesc[1] = desc; + + info.op_tmpl.cmd.opcode = AT25_WRITE | AT25_INSTR_BIT3; + info.op_tmpl.data.dir = SPI_MEM_DATA_OUT; + + desc = spi_mem_dirmap_create(at25->spimem, &info); + if (IS_ERR(desc)) { + ret = PTR_ERR(desc); + goto err_destroy_dirmaps; + } + + at25->dirmap.wdesc[1] = desc; + + return 0; + +err_destroy_dirmaps: + at25_destroy_dirmaps(at25); + + return ret; +} + +static int at25_rdsr(struct at25_data *at25) +{ + struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(AT25_RDSR, 1), + SPI_MEM_OP_NO_ADDR, + SPI_MEM_OP_NO_DUMMY, + SPI_MEM_OP_DATA_IN(1, at25->scratchbuf, 1)); + int ret; + + ret = spi_mem_exec_op(at25->spimem, &op); + if (ret) + return ret; + + return *((u8 *)at25->scratchbuf); +} + +static int at25_wren(struct at25_data *at25) +{ + struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(AT25_WREN, 1), + SPI_MEM_OP_NO_ADDR, + SPI_MEM_OP_NO_DUMMY, + SPI_MEM_OP_NO_DATA); + + return spi_mem_exec_op(at25->spimem, &op); +} + static int at25_ee_read(void *priv, unsigned int offset, void *val, size_t count) { + struct spi_mem_dirmap_desc *desc; struct at25_data *at25 = priv; - char *buf = val; - u8 command[EE_MAXADDRLEN + 1]; - u8 *cp; ssize_t status; - struct spi_transfer t[2]; - struct spi_message m; - u8 instr; if (unlikely(offset >= at25->chip.byte_len)) return -EINVAL; @@ -82,37 +187,10 @@ static int at25_ee_read(void *priv, unsigned int offset, if (unlikely(!count)) return -EINVAL; - cp = command; - - instr = AT25_READ; - if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR) - if (offset >= (1U << (at25->addrlen * 8))) - instr |= AT25_INSTR_BIT3; - *cp++ = instr; - - /* 8/16/24-bit address is written MSB first */ - switch (at25->addrlen) { - default: /* case 3 */ - *cp++ = offset >> 16; - /* fall through */ - case 2: - *cp++ = offset >> 8; - /* fall through */ - case 1: - case 0: /* can't happen: for better codegen */ - *cp++ = offset >> 0; - } - - spi_message_init(&m); - memset(t, 0, sizeof(t)); - - t[0].tx_buf = command; - t[0].len = at25->addrlen + 1; - spi_message_add_tail(&t[0], &m); - - t[1].rx_buf = buf; - t[1].len = count; - spi_message_add_tail(&t[1], &m); + if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR && offset > 255) + desc = at25->dirmap.rdesc[1]; + else + desc = at25->dirmap.rdesc[0]; mutex_lock(&at25->lock); @@ -122,8 +200,8 @@ static int at25_ee_read(void *priv, unsigned int offset, * other devices on the bus need to be accessed regularly or * this chip is clocked very slowly */ - status = spi_sync(at25->spi, &m); - dev_dbg(&at25->spi->dev, "read %zu bytes at %d --> %zd\n", + status = spi_mem_dirmap_read(desc, offset, count, val); + dev_dbg(&at25->spimem->spi->dev, "read %zu bytes at %d --> %zd\n", count, offset, status); mutex_unlock(&at25->lock); @@ -149,7 +227,7 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count) buf_size = at25->chip.page_size; if (buf_size > io_limit) buf_size = io_limit; - bounce = kmalloc(buf_size + at25->addrlen + 1, GFP_KERNEL); + bounce = kmalloc(buf_size, GFP_KERNEL); if (!bounce) return -ENOMEM; @@ -158,47 +236,32 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count) */ mutex_lock(&at25->lock); do { + struct spi_mem_dirmap_desc *desc; unsigned long timeout, retries; unsigned segment; unsigned offset = (unsigned) off; - u8 *cp = bounce; int sr; - u8 instr; - *cp = AT25_WREN; - status = spi_write(at25->spi, cp, 1); + status = at25_wren(at25); if (status < 0) { - dev_dbg(&at25->spi->dev, "WREN --> %d\n", status); + dev_dbg(&at25->spimem->spi->dev, "WREN --> %d\n", + status); break; } - instr = AT25_WRITE; - if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR) - if (offset >= (1U << (at25->addrlen * 8))) - instr |= AT25_INSTR_BIT3; - *cp++ = instr; - - /* 8/16/24-bit address is written MSB first */ - switch (at25->addrlen) { - default: /* case 3 */ - *cp++ = offset >> 16; - /* fall through */ - case 2: - *cp++ = offset >> 8; - /* fall through */ - case 1: - case 0: /* can't happen: for better codegen */ - *cp++ = offset >> 0; - } + if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR && offset > 255) + desc = at25->dirmap.wdesc[1]; + else + desc = at25->dirmap.wdesc[0]; /* Write as much of a page as we can */ segment = buf_size - (offset % buf_size); if (segment > count) segment = count; - memcpy(cp, buf, segment); - status = spi_write(at25->spi, bounce, - segment + at25->addrlen + 1); - dev_dbg(&at25->spi->dev, "write %u bytes at %u --> %d\n", + memcpy(bounce, buf, segment); + status = spi_mem_dirmap_write(desc, offset, segment, bounce); + dev_dbg(&at25->spimem->spi->dev, + "write %u bytes at %u --> %d\n", segment, offset, status); if (status < 0) break; @@ -211,10 +274,9 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count) timeout = jiffies + msecs_to_jiffies(EE_TIMEOUT); retries = 0; do { - - sr = spi_w8r8(at25->spi, AT25_RDSR); + sr = at25_rdsr(at25); if (sr < 0 || (sr & AT25_SR_nRDY)) { - dev_dbg(&at25->spi->dev, + dev_dbg(&at25->spimem->spi->dev, "rdsr --> %d (%02x)\n", sr, sr); /* at HZ=100, this is sloooow */ msleep(1); @@ -225,7 +287,7 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count) } while (retries++ < 3 || time_before_eq(jiffies, timeout)); if ((sr < 0) || (sr & AT25_SR_nRDY)) { - dev_err(&at25->spi->dev, + dev_err(&at25->spimem->spi->dev, "write %u bytes offset %u, timeout after %u msecs\n", segment, offset, jiffies_to_msecs(jiffies - @@ -304,7 +366,7 @@ static int at25_fw_to_chip(struct device *dev, struct spi_eeprom *chip) return 0; } -static int at25_probe(struct spi_device *spi) +static int at25_probe(struct spi_mem *spimem) { struct at25_data *at25 = NULL; struct spi_eeprom chip; @@ -313,12 +375,12 @@ static int at25_probe(struct spi_device *spi) int addrlen; /* Chip description */ - if (!spi->dev.platform_data) { - err = at25_fw_to_chip(&spi->dev, &chip); + if (!spimem->spi->dev.platform_data) { + err = at25_fw_to_chip(&spimem->spi->dev, &chip); if (err) return err; } else - chip = *(struct spi_eeprom *)spi->dev.platform_data; + chip = *(struct spi_eeprom *)spimem->spi->dev.platform_data; /* For now we only support 8/16/24 bit addressing */ if (chip.flags & EE_ADDR1) @@ -328,37 +390,48 @@ static int at25_probe(struct spi_device *spi) else if (chip.flags & EE_ADDR3) addrlen = 3; else { - dev_dbg(&spi->dev, "unsupported address type\n"); + dev_dbg(&spimem->spi->dev, "unsupported address type\n"); return -EINVAL; } - /* Ping the chip ... the status register is pretty portable, - * unlike probing manufacturer IDs. We do expect that system - * firmware didn't write it in the past few milliseconds! - */ - sr = spi_w8r8(spi, AT25_RDSR); - if (sr < 0 || sr & AT25_SR_nRDY) { - dev_dbg(&spi->dev, "rdsr --> %d (%02x)\n", sr, sr); - return -ENXIO; - } - - at25 = devm_kzalloc(&spi->dev, sizeof(struct at25_data), GFP_KERNEL); + at25 = devm_kzalloc(&spimem->spi->dev, sizeof(struct at25_data), GFP_KERNEL); if (!at25) return -ENOMEM; mutex_init(&at25->lock); at25->chip = chip; - at25->spi = spi; - spi_set_drvdata(spi, at25); + at25->spimem = spimem; + spi_mem_set_drvdata(spimem, at25); at25->addrlen = addrlen; - at25->nvmem_config.name = dev_name(&spi->dev); - at25->nvmem_config.dev = &spi->dev; + /* + * Can't be allocated with devm_kmalloc() because we need a DMA-safe + * buffer. + */ + at25->scratchbuf = kmalloc(1, GFP_KERNEL); + + /* Ping the chip ... the status register is pretty portable, + * unlike probing manufacturer IDs. We do expect that system + * firmware didn't write it in the past few milliseconds! + */ + sr = at25_rdsr(at25); + if (sr < 0 || sr & AT25_SR_nRDY) { + dev_dbg(&spimem->spi->dev, "rdsr --> %d (%02x)\n", sr, sr); + err = -ENXIO; + goto err_free_scratchbuf; + } + + err = at25_create_dirmaps(at25); + if (err) + goto err_free_scratchbuf; + + at25->nvmem_config.name = dev_name(&spimem->spi->dev); + at25->nvmem_config.dev = &spimem->spi->dev; at25->nvmem_config.read_only = chip.flags & EE_READONLY; at25->nvmem_config.root_only = true; at25->nvmem_config.owner = THIS_MODULE; at25->nvmem_config.compat = true; - at25->nvmem_config.base_dev = &spi->dev; + at25->nvmem_config.base_dev = &spimem->spi->dev; at25->nvmem_config.reg_read = at25_ee_read; at25->nvmem_config.reg_write = at25_ee_write; at25->nvmem_config.priv = at25; @@ -366,17 +439,38 @@ static int at25_probe(struct spi_device *spi) at25->nvmem_config.word_size = 1; at25->nvmem_config.size = chip.byte_len; - at25->nvmem = devm_nvmem_register(&spi->dev, &at25->nvmem_config); - if (IS_ERR(at25->nvmem)) - return PTR_ERR(at25->nvmem); + at25->nvmem = devm_nvmem_register(&spimem->spi->dev, + &at25->nvmem_config); + if (IS_ERR(at25->nvmem)) { + err = PTR_ERR(at25->nvmem); + goto err_destroy_dirmaps; + } - dev_info(&spi->dev, "%d %s %s eeprom%s, pagesize %u\n", + dev_info(&spimem->spi->dev, "%d %s %s eeprom%s, pagesize %u\n", (chip.byte_len < 1024) ? chip.byte_len : (chip.byte_len / 1024), (chip.byte_len < 1024) ? "Byte" : "KByte", at25->chip.name, (chip.flags & EE_READONLY) ? " (readonly)" : "", at25->chip.page_size); return 0; + +err_destroy_dirmaps: + at25_destroy_dirmaps(at25); + +err_free_scratchbuf: + kfree(at25->scratchbuf); + + return err; +} + +static int at25_remove(struct spi_mem *spimem) +{ + struct at25_data *at25 = spi_mem_get_drvdata(spimem); + + at25_destroy_dirmaps(at25); + kfree(at25->scratchbuf); + + return 0; } /*-------------------------------------------------------------------------*/ @@ -387,15 +481,15 @@ static const struct of_device_id at25_of_match[] = { }; MODULE_DEVICE_TABLE(of, at25_of_match); -static struct spi_driver at25_driver = { - .driver = { - .name = "at25", +static struct spi_mem_driver at25_driver = { + .spidrv.driver = { + .name = "at25", .of_match_table = at25_of_match, }, - .probe = at25_probe, + .probe = at25_probe, + .remove = at25_remove, }; - -module_spi_driver(at25_driver); +module_spi_mem_driver(at25_driver); MODULE_DESCRIPTION("Driver for most SPI EEPROMs"); MODULE_AUTHOR("David Brownell"); ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/