[PATCH linux] mtd: spi-nor: aspeed-smc: Use io string accessors to fifo
Andrew Jeffery
andrew at aj.id.au
Fri Feb 12 13:59:30 AEDT 2016
Hi Milton,
Excuse the unnecessary quote markers that might appear through the
patch, my mail client (Evolution) seems to have a mind of its own
sometimes. Starting to shave that yak. Anyway.
As an aside I've tested an earlier version of the code and confirmed
the approach works (i.e. no JFFS2 corruption on re-mount).
On Wed, 2016-02-10 at 21:50 -0600, OpenBMC Patches wrote:
> From: "Milton D. Miller II" <miltonm at us.ibm.com>
>
> Make and use new routines to access the fifo when in user mode.
> Instead of using memcpy_fromio, base the new routines on the io
> port string accessors.
>
> When the smc controllers are configured to transfer in user mode,
> the exact address within the chip select region is ignored and
> all valid data bytes are sent to the device fifo. It has been
> determined the reason _memcpy_fromio, an arm specific byte-by-byte
> variant of memcpy_fromio was required in read_reg, is that the
> later will stutter when coping from memory, discarding some
> bytes read, when switching from words to bytes for the final
> unaligned length.
>
> This change includes a hack to make inw/outw work. The default
> generic IO_SPACE_LIMIT is 0, and twenty bits when PCI is selected.
> While there is a PCMCIA SOC config to select the full 4GB space it
> would bring in the PCMCIA core for no use to the ast2400. We want
> to be part of the generic multi-platform arm kernel and therefore
> mach/io.h is not going to work. However, the limit is under an
> ifndef, so define it in this file an check it was not redefine
> at probe time. To avoid redefinition errors or warnings, guard
> the setting with CONFIG_ARM for cross-arch compile testing and
> with a Kconfig dependency for arm cross-compile tests. While some
> other architecture may not accept unsigne
typo: unsigned
> long addresses as ioport
> cookies, that can be addressed when, if ever, the hardware appears
> on another arch.
>
> While the change to use the designed-for-memory optimised copy
> template, even reusing the bytes of memcpy, was a recent change
> and only for little endian arm, designing and proposing a patch
> will take some time and there could be debate if the routine
> needs to work on changing memory.
>
> Signed-off-by: Milton Miller <miltonm at us.ibm.com>
> ---
> drivers/mtd/spi-nor/Kconfig | 2 +
> drivers/mtd/spi-nor/aspeed-smc.c | 142 ++++++++++++++++++++++++++++++++++++---
> 2 files changed, 135 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index c7554d8..4a963c4 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -44,6 +44,8 @@ config ASPEED_FLASH_SPI
> > > tristate "Aspeed flash controllers in SPI mode"
> > > depends on HAS_IOMEM && OF
> > > depends on ARCH_ASPEED || COMPILE_TEST
> +> > # IO_SPACE_LIMIT must be equivalent to (~0UL)
> +> > depends on !NEED_MACH_IO_H
> > > help
> > > This enables support for the New Static Memory Controller (FMC)
> > > in the AST2400 when attached to SPI nor chips, and support for
> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
> index 9dd4609..74127d8 100644
> --- a/drivers/mtd/spi-nor/aspeed-smc.c
> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
> @@ -8,6 +8,12 @@
> *
> */
>
> +/* See comment by aspeed_smc_from_fifo */
> +#ifdef CONFIG_ARM
> +#define IO_SPACE_LIMIT (~0UL)
> +#endif
> +
> +#include
> #include
> #include
> #include
> @@ -19,6 +25,116 @@
> #include
> #include
>
> +#include
> +
> +static void *ins_align(unsigned long io, void *buf, size_t len)
> +{
> +> > BUILD_BUG_ON(0 & len & ~3);
Should just be BUG_ON()? Given that you disabled it with '0 &', do we
want it at all?
> +
> +> > if (len & 2) {
> +> > > put_unaligned(inw(io), (u16 *)buf);
> +> > > buf += sizeof(u16);
> +> > }
> +> > if (len & 1) {
> +> > > put_unaligned(inb(io), (u8 *)buf);
> +> > > buf += sizeof(u8);
> +> > }
> +> > return buf;
> +}
> +
> +static const void *outs_align(unsigned long io, const void *buf, size_t len)
> +{
> +> > BUILD_BUG_ON(0 & len & ~3);
As above
> + if (len & 2) {
> +> > > outw(get_unaligned((u16 *)buf), io);
> +> > > buf += sizeof(u16);
> +> > }
> +> > if (len & 1) {
> +> > > outb(get_unaligned((u8 *)buf), io);
> +> > > buf += sizeof(u8);
> +> > }
> +> > return buf;
> +}
> +
> +
> +/*
> + * On the arm architecture, as of Linux version 4.3, memcpy_fromio
> + * stutters discarding some of the bytes read if the destination is
> + * unaligned, so we can't use it for reading from a fifo to a buffer
> + * of unknown alignment. Instead use the ins (l, w, b) family
> + * to read from the fifo. However, ARM tries to hide io port
> + * accesses from drivers unless there is a PCMCIA or PCI device, so
> + * we define the limit before all include files. There is a
> + * check in probe to make sure this will work, as long as the
> + * architecture uses an identity iomap.
> + */
> +
> +static void aspeed_smc_from_fifo(void *buf, const void __iomem *iop, size_t len)
> +{
> +> > unsigned long io = (__force unsigned long)iop;
> +
> +> > if (io & 1) {
> +> > > insb(io, buf, len);
> +> > } else if ((unsigned long)io & 2) {
> +> > > size_t l = (2 - (unsigned long)buf) & 1;
> +
> +> > > l = min(l, len);
> +> > > buf = ins_align(io, buf, l);
> +> > > len -= l;
> +
> +> > > if (len >= 2) {
> +> > > > insw(io, buf, len >> 1);
> +> > > > buf += len & ~1;
> +> > > }
> +> > > buf = ins_align(io, buf, len & 1);
> +> > } else {
> +> > > size_t l = (4 - (unsigned long)buf) & 3;
> +
> +> > > l = min(l, len);
> +> > > buf = ins_align(io, buf, l);
> +> > > len -= l;
> +
> +> > > if (len >= 4) {
> +> > > > insl(io, buf, len >> 2);
> +> > > > buf += len & ~3;
> +> > > }
> +> > > buf = ins_align(io, buf, len & 3);
> +> > }
So I've tried deduplicating this function - the code in the latter two
cases is largely the same bar the magic numbers and insw vs insl. I've
come up with the code below (note that I haven't actually probed the
modifications), but I'm not convinced it's much of a win in terms of
clarity:
/* Is this available somewhere else? Rusty doesn't think so.
* Probably needs a better name (first set value) */
#define FS_VAL(x) ((x) & ~((x) - 1))
static void aspeed_smc_from_fifo(void *buf, const void __iomem *iop, size_t len)
{
unsigned long io = (__force unsigned long)iop;
u8 align = FS_VAL((io & 7) | 4); /* align is one of 1, 2 or 4 */
u8 mask = align - 1;
size_t to_align = align - ((unsigned long)buf) & mask);
if (align == 1) {
insb(io, buf, len);
return;
}
to_align = min(to_align, len);
buf = ins_align(io, buf, to_align);
len -= to_align;
if (len >= align) {
size_t nr_blocks = len / align;
if (align == 2) {
insw(io, buf, nr_blocks);
} else {
insl(io, buf, nr_blocks);
}
buf += (nr_blocks * align);
}
buf = ins_align(io, buf, len & mask);
Is it worth it? A similar approach can be taken for
aspeed_smc_to_fifo() if so.
As for whether there's a better approach short of fixing mmiocpy(), I
don't know, so no further suggestions really (i.e. I guess we just live
with the IO_SPACE_LIMIT jiggery-pokery).
> +}
> +
> +static void aspeed_smc_to_fifo(void __iomem *iop, const void *buf, size_t len)
> +{
> +> > unsigned long io = (__force unsigned long)iop;
> +
> +> > if (io & 1) {
> +> > > outsb(io, buf, len);
> +> > } else if (io & 2) {
> +> > > size_t l = (2 - (unsigned long)buf) & 1;
> +
> +> > > len = min(l, len);
> +> > > buf = outs_align(io, buf, l);
> +> > > len -= l;
> +
> +> > > if (len >= 2) {
> +> > > > outsw(io, buf, len >> 1);
> +> > > > buf += len & ~(size_t)1;
> +> > > }
> +> > > buf = outs_align(io, buf, len & 1);
> +> > } else {
> +> > > size_t l = (4 - (unsigned long)buf) & 3;
> +
> +> > > l = min(l, len);
> +> > > buf = outs_align(io, buf, l);
> +> > > len -= l;
> +
> +> > > if (len >= 4) {
> +> > > > outsl(io, buf, len >> 2);
> +> > > > buf += len & ~(size_t)3;
> +> > > }
> +> > > buf = outs_align(io, buf, len & 3);
> +> > }
> +}
> +
> enum smc_flash_type {
> > > smc_type_nor = 0,> > /* controller connected to nor flash */
> > > smc_type_nand = 1,> > /* controller connected to nand flash */
> @@ -165,8 +281,8 @@ static int aspeed_smc_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
> > > struct aspeed_smc_chip *chip = nor->priv;
>
> > > aspeed_smc_start_user(nor);
> -> > writeb(opcode, chip->base);
> -> > _memcpy_fromio(buf, chip->base, len);
> +> > aspeed_smc_to_fifo(chip->base, &opcode, 1);
> +> > aspeed_smc_from_fifo(buf, chip->base, len);
> > > aspeed_smc_stop_user(nor);
>
> > > return 0;
> @@ -178,8 +294,8 @@ static int aspeed_smc_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf,
> > > struct aspeed_smc_chip *chip = nor->priv;
>
> > > aspeed_smc_start_user(nor);
> -> > writeb(opcode, chip->base);
> -> > _memcpy_toio(chip->base, buf, len);
> +> > aspeed_smc_to_fifo(chip->base, &opcode, 1);
> +> > aspeed_smc_to_fifo(chip->base, buf, len);
> > > aspeed_smc_stop_user(nor);
>
> > > return 0;
> @@ -202,12 +318,12 @@ static void aspeed_smc_send_cmd_addr(struct spi_nor *nor, u8 cmd, u32 addr)
> > > > cmdaddr |= (u32)cmd << 24;
>
> > > > temp = cpu_to_be32(cmdaddr);
> -> > > memcpy_toio(chip->base, &temp, 4);
> +> > > aspeed_smc_to_fifo(chip->base, &temp, 4);
> > > > break;
> > > case 4:
> > > > temp = cpu_to_be32(addr);
> -> > > writeb(cmd, chip->base);
> -> > > memcpy_toio(chip->base, &temp, 4);
> +> > > aspeed_smc_to_fifo(chip->base, &cmd, 1);
> +> > > aspeed_smc_to_fifo(chip->base, &temp, 4);
> > > > break;
> > > }
> }
> @@ -219,7 +335,7 @@ static int aspeed_smc_read_user(struct spi_nor *nor, loff_t from, size_t len,
>
> > > aspeed_smc_start_user(nor);
> > > aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
> -> > memcpy_fromio(read_buf, chip->base, len);
> +> > aspeed_smc_from_fifo(read_buf, chip->base, len);
> > > *retlen += len;
> > > aspeed_smc_stop_user(nor);
>
> @@ -233,7 +349,7 @@ static void aspeed_smc_write_user(struct spi_nor *nor, loff_t to, size_t len,
>
> > > aspeed_smc_start_user(nor);
> > > aspeed_smc_send_cmd_addr(nor, nor->program_opcode, to);
> -> > memcpy_toio(chip->base, write_buf, len);
> +> > aspeed_smc_to_fifo(chip->base, write_buf, len);
> > > *retlen += len;
> > > aspeed_smc_stop_user(nor);
> }
> @@ -292,6 +408,14 @@ static int aspeed_smc_probe(struct platform_device *dev)
> > > int err = 0;
> > > unsigned int n;
>
> +> > /*
> +> > * This driver passes ioremap addresses to io port accessors.
> +> > * This works on arm if the IO_SPACE_LIMIT does not truncate
> +> > * the address.
> +> > */
> +> > if (~(unsigned long)IO_SPACE_LIMIT)
> +> > > return -ENODEV;
> +
> > > match = of_match_device(aspeed_smc_matches, &dev->dev);
> > > if (!match || !match->data)
> > > > return -ENODEV;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160212/d536e48d/attachment.sig>
More information about the openbmc
mailing list