From: Mika Westerberg <mika.westerberg@linux.intel.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Tudor Ambarus <tudor.ambarus@microchip.com>,
Mark Brown <broonie@kernel.org>, Lee Jones <lee.jones@linaro.org>,
Michael Walle <michael@walle.cc>, Pratyush Yadav <p.yadav@ti.com>,
Miquel Raynal <miquel.raynal@bootlin.com>,
Richard Weinberger <richard@nod.at>,
Vignesh Raghavendra <vigneshr@ti.com>,
Jonathan Corbet <corbet@lwn.net>,
Mauro Lima <mauro.lima@eclypsium.com>,
Alexander Sverdlin <alexander.sverdlin@nokia.com>,
"open list:MEMORY TECHNOLOGY..." <linux-mtd@lists.infradead.org>,
linux-spi <linux-spi@vger.kernel.org>
Subject: Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM
Date: Tue, 5 Oct 2021 12:41:39 +0300 [thread overview]
Message-ID: <YVwd09KZwOQVvh4g@lahna> (raw)
In-Reply-To: <CAHp75VdtOHn4ED-ixdDngBQw10OnKmbtTv=ydLs6dYbkjyqW4Q@mail.gmail.com>
Hi Andy,
On Mon, Oct 04, 2021 at 05:29:47PM +0300, Andy Shevchenko wrote:
> On Thu, Sep 30, 2021 at 1:08 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > The preferred way to implement SPI-NOR controller drivers is through SPI
> > subsubsystem utilizing the SPI MEM core functions. This converts the
> > Intel SPI flash controller driver over the SPI MEM by moving the driver
> > from SPI-NOR subsystem to SPI subsystem and in one go make it use the
> > SPI MEM functions. The driver name will be changed from intel-spi to
> > spi-intel to match the convention used in the SPI subsystem.
>
> ...
>
> > +config SPI_INTEL_PCI
> > + tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
> > + depends on PCI && (X86 || COMPILE_TEST)
>
> Perhaps two entries, one of which will be the same as for platform case?
Sure will do that in v2.
> > + depends on SPI_MEM
> > + select SPI_INTEL
> > + help
> > + This enables PCI support for the Intel PCH/PCU SPI controller in
> > + master mode. This controller is present in modern Intel hardware
> > + and is used to hold BIOS and other persistent settings. Using
> > + this driver it is possible to upgrade BIOS directly from Linux.
> > +
> > + Say N here unless you know what you are doing. Overwriting the
> > + SPI flash may render the system unbootable.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called spi-intel-pci.
> > +
> > +config SPI_INTEL_PLATFORM
> > + tristate "Intel PCH/PCU SPI flash platform driver (DANGEROUS)"
> > + depends on X86 || COMPILE_TEST
> > + depends on SPI_MEM
> > + select SPI_INTEL
> > + help
> > + This enables platform support for the Intel PCH/PCU SPI
> > + controller in master mode. This controller is present in modern
> > + Intel hardware and is used to hold BIOS and other persistent
> > + settings. Using this driver it is possible to upgrade BIOS
> > + directly from Linux.
> > +
> > + Say N here unless you know what you are doing. Overwriting the
> > + SPI flash may render the system unbootable.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called spi-intel-platform.
>
> ...
>
> + Blank line ?
OK
>
> > #include <linux/mtd/partitions.h>
> > #include <linux/mtd/spi-nor.h>
>
> + Blank line?
OK
> > +#include <linux/spi/flash.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/spi/spi-mem.h>
>
> The rationale is to show that we use two sub(sub)sytems here.
Yup, got it.
> ...
>
> > - dev_err(ispi->dev, "read error: %llx: %#x\n", from,
> > + dev_err(ispi->dev, "read error: %x: %#x\n", from,
> > status);
>
> Now one line?
>
> ...
>
> > - dev_err(ispi->dev, "write error: %llx: %#x\n", to,
> > + dev_err(ispi->dev, "write error: %x: %#x\n", to,
> > status);
>
> Ditto.
Sure for both.
>
> ...
>
> > + ret = intel_spi_sw_cycle(ispi, opcode, 0,
> > + OPTYPE_WRITE_WITH_ADDR);
> > + return ret ? ret : 0;
>
> Why not simply return intel_spi_dw_cycle(...); ?
Good point. Will fix that.
> ...
>
> > + val = readl(ispi->base + HSFSTS_CTL);
> > + val &= ~(HSFSTS_CTL_FDBC_MASK | HSFSTS_CTL_FCYCLE_MASK);
> > + val |= HSFSTS_CTL_AEL | HSFSTS_CTL_FCERR | HSFSTS_CTL_FDONE;
>
> > + val |= cmd;
> > + val |= HSFSTS_CTL_FGO;
>
> Maybe swap these lines to group constants?
OK
> ...
>
> > + status = readl(ispi->base + HSFSTS_CTL);
> > + if (status & HSFSTS_CTL_FCERR)
> > + return -EIO;
>
> > + else if (status & HSFSTS_CTL_AEL)
>
> Redundant 'else'
Right.
> > + return -EACCES;
>
> ...
>
> > +static int intel_spi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
> > +{
> > + struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
> > + size_t nbytes = op->data.nbytes;
> > + u8 opcode = op->cmd.opcode;
> > +
> > + if (op->addr.nbytes) {
> > + if (op->data.dir == SPI_MEM_DATA_IN)
> > + return intel_spi_read(ispi, op->addr.val, nbytes,
> > + op->data.buf.in);
>
> > + else if (op->data.dir == SPI_MEM_DATA_OUT)
>
> Redundant 'else' here and nearby.
Right. I'll fix these in v2.
> > + return intel_spi_write(ispi, op->addr.val, nbytes,
> > + op->data.buf.out);
> > + else if (op->data.dir == SPI_MEM_NO_DATA)
> > + return intel_spi_erase(ispi, opcode, op->addr.val);
> > + } else {
> > + if (op->data.dir == SPI_MEM_DATA_IN)
> > + return intel_spi_read_reg(ispi, opcode, op->data.buf.in,
> > + nbytes);
> > + else if (op->data.dir == SPI_MEM_DATA_OUT)
> > + return intel_spi_write_reg(ispi, opcode, op->data.buf.out,
> > + nbytes);
> > + else if (op->data.dir == SPI_MEM_NO_DATA)
> > + return intel_spi_write_reg(ispi, opcode, NULL, 0);
> > }
>
> > + return -EINVAL;
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
next prev parent reply other threads:[~2021-10-05 9:41 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-30 10:07 [PATCH 0/3] mtd: spi-nor / spi / MFD: Convert intel-spi to SPI MEM Mika Westerberg
2021-09-30 10:07 ` [PATCH 1/3] mtd: spi-nor: intel-spi: Disable write protection only if asked Mika Westerberg
2021-10-01 20:23 ` Mauro Lima
2021-10-04 5:18 ` Mika Westerberg
2021-10-12 18:49 ` Mauro Lima
2021-10-13 9:03 ` Mika Westerberg
2021-10-13 18:22 ` Mauro Lima
2021-09-30 10:07 ` [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM Mika Westerberg
2021-10-04 9:52 ` Pratyush Yadav
2021-10-04 10:07 ` Mika Westerberg
2021-10-07 12:36 ` Pratyush Yadav
2021-10-07 16:46 ` Mika Westerberg
2021-10-07 18:00 ` Pratyush Yadav
2021-10-08 9:02 ` Mika Westerberg
2021-10-08 10:56 ` Pratyush Yadav
2021-10-04 14:29 ` Andy Shevchenko
2021-10-05 9:41 ` Mika Westerberg [this message]
2021-09-30 10:07 ` [PATCH 3/3] Documentation / MTD: Rename the intel-spi driver Mika Westerberg
2021-09-30 15:03 ` Alexander Sverdlin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YVwd09KZwOQVvh4g@lahna \
--to=mika.westerberg@linux.intel.com \
--cc=alexander.sverdlin@nokia.com \
--cc=andy.shevchenko@gmail.com \
--cc=broonie@kernel.org \
--cc=corbet@lwn.net \
--cc=lee.jones@linaro.org \
--cc=linux-mtd@lists.infradead.org \
--cc=linux-spi@vger.kernel.org \
--cc=mauro.lima@eclypsium.com \
--cc=michael@walle.cc \
--cc=miquel.raynal@bootlin.com \
--cc=p.yadav@ti.com \
--cc=richard@nod.at \
--cc=tudor.ambarus@microchip.com \
--cc=vigneshr@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).