From: Marek Vasut <marex@denx.de>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH 2/3] sf: Shrink spi_slave {}
Date: Sat, 18 Jan 2014 17:59:57 +0100 [thread overview]
Message-ID: <201401181759.57890.marex@denx.de> (raw)
In-Reply-To: <CAD6G_RSyg1CSHpCN52jo5M3BSca-m3BtjfHm3BqKZ98XZjXXww@mail.gmail.com>
On Friday, January 17, 2014 at 06:12:49 PM, Jagan Teki wrote:
> On Fri, Jan 17, 2014 at 10:01 PM, Marek Vasut <marex@denx.de> wrote:
> > On Friday, January 17, 2014 at 03:41:46 PM, Jagannadha Sutradharudu Teki
wrote:
> >> Combined spi flash stuff as minimum as possible.
> >
> > Squashing the names of the flags only makes it unreadable :-(
> >
> >> Signed-off-by: Jagannadha Sutradharudu Teki <jaganna@xilinx.com>
> >> Cc: Marek Vasut <marex@denx.de>
> >> ---
> >>
> >> drivers/mtd/spi/sf.c | 4 ++--
> >> drivers/mtd/spi/sf_ops.c | 18 +++++++++---------
> >> drivers/mtd/spi/sf_probe.c | 19 ++++++++++++-------
> >> include/spi.h | 45
> >>
> >> +++++++++++++++++++-------------------------- include/spi_flash.h
> >> |
> >>
> >> 6 +++---
> >> 5 files changed, 45 insertions(+), 47 deletions(-)
> >>
> >> diff --git a/drivers/mtd/spi/sf.c b/drivers/mtd/spi/sf.c
> >> index 664e860..fb91ac6 100644
> >> --- a/drivers/mtd/spi/sf.c
> >> +++ b/drivers/mtd/spi/sf.c
> >> @@ -19,8 +19,8 @@ static int spi_flash_read_write(struct spi_slave *spi,
> >>
> >> int ret;
> >>
> >> #ifdef CONFIG_SF_DUAL_FLASH
> >>
> >> - if (spi->flags & SPI_XFER_U_PAGE)
> >> - flags |= SPI_XFER_U_PAGE;
> >> + if (spi->mode_bits & SPI_U_PAGE)
> >> + flags |= SPI_U_PAGE;
> >>
> >> #endif
> >>
> >> if (data_len == 0)
> >>
> >> flags |= SPI_XFER_END;
> >>
> >> diff --git a/drivers/mtd/spi/sf_ops.c b/drivers/mtd/spi/sf_ops.c
> >> index 9650486..0d87e1b 100644
> >> --- a/drivers/mtd/spi/sf_ops.c
> >> +++ b/drivers/mtd/spi/sf_ops.c
> >> @@ -135,15 +135,15 @@ static int spi_flash_bank(struct spi_flash *flash,
> >> u32 offset) static void spi_flash_dual_flash(struct spi_flash *flash,
> >> u32 *addr) {
> >>
> >> switch (flash->dual_flash) {
> >>
> >> - case SF_DUAL_STACKED_FLASH:
> >>
> >> + case SF_STACKED:
> >> if (*addr >= (flash->size >> 1)) {
> >>
> >> *addr -= flash->size >> 1;
> >>
> >> - flash->spi->flags |= SPI_XFER_U_PAGE;
> >> + flash->spi->mode_bits |= SPI_U_PAGE;
> >>
> >> } else {
> >>
> >> - flash->spi->flags &= ~SPI_XFER_U_PAGE;
> >> + flash->spi->mode_bits &= ~SPI_U_PAGE;
> >>
> >> }
> >> break;
> >>
> >> - case SF_DUAL_PARALLEL_FLASH:
> >>
> >> + case SF_PARALLEL:
> >> *addr >>= flash->shift;
> >> break;
> >>
> >> default:
> >> @@ -170,8 +170,8 @@ int spi_flash_cmd_wait_ready(struct spi_flash
> >> *flash, unsigned long timeout) }
> >>
> >> #ifdef CONFIG_SF_DUAL_FLASH
> >>
> >> - if (spi->flags & SPI_XFER_U_PAGE)
> >> - flags |= SPI_XFER_U_PAGE;
> >> + if (spi->mode_bits & SPI_U_PAGE)
> >> + flags |= SPI_U_PAGE;
> >>
> >> #endif
> >>
> >> ret = spi_xfer(spi, 8, &cmd, NULL, flags);
> >> if (ret) {
> >>
> >> @@ -261,7 +261,7 @@ int spi_flash_cmd_erase_ops(struct spi_flash *flash,
> >> u32 offset, size_t len) erase_addr = offset;
> >>
> >> #ifdef CONFIG_SF_DUAL_FLASH
> >>
> >> - if (flash->dual_flash > SF_SINGLE_FLASH)
> >> + if (flash->dual_flash > SF_SINGLE)
> >>
> >> spi_flash_dual_flash(flash, &erase_addr);
> >>
> >> #endif
> >> #ifdef CONFIG_SPI_FLASH_BAR
> >>
> >> @@ -303,7 +303,7 @@ int spi_flash_cmd_write_ops(struct spi_flash *flash,
> >> u32 offset, write_addr = offset;
> >>
> >> #ifdef CONFIG_SF_DUAL_FLASH
> >>
> >> - if (flash->dual_flash > SF_SINGLE_FLASH)
> >> + if (flash->dual_flash > SF_SINGLE)
> >>
> >> spi_flash_dual_flash(flash, &write_addr);
> >>
> >> #endif
> >> #ifdef CONFIG_SPI_FLASH_BAR
> >>
> >> @@ -388,7 +388,7 @@ int spi_flash_cmd_read_ops(struct spi_flash *flash,
> >> u32 offset, read_addr = offset;
> >>
> >> #ifdef CONFIG_SF_DUAL_FLASH
> >>
> >> - if (flash->dual_flash > SF_SINGLE_FLASH)
> >> + if (flash->dual_flash > SF_SINGLE)
> >>
> >> spi_flash_dual_flash(flash, &read_addr);
> >>
> >> #endif
> >> #ifdef CONFIG_SPI_FLASH_BAR
> >>
> >> diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c
> >> index e84ab13..003f635 100644
> >> --- a/drivers/mtd/spi/sf_probe.c
> >> +++ b/drivers/mtd/spi/sf_probe.c
> >> @@ -133,8 +133,13 @@ static struct spi_flash
> >> *spi_flash_validate_params(struct spi_slave *spi, flash->spi = spi;
> >>
> >> flash->name = params->name;
> >> flash->memory_map = spi->memory_map;
> >>
> >> - flash->dual_flash = flash->spi->option;
> >>
> >> +#ifdef CONFIG_SF_DUAL_FLASH
> >
> > Looks new and unrelated.
> >
> >> + if (flash->spi->mode_bits & SPI_SHARED)
> >> + flash->dual_flash = SF_STACKED;
> >> + else if (flash->spi->mode_bits & SPI_SEPARATED)
> >> + flash->dual_flash = SF_PARALLEL;
> >> +#endif
> >>
> >> /* Assign spi_flash ops */
> >> flash->write = spi_flash_cmd_write_ops;
> >>
> >> #ifdef CONFIG_SPI_FLASH_SST
> >>
> >> @@ -145,12 +150,12 @@ static struct spi_flash
> >> *spi_flash_validate_params(struct spi_slave *spi, flash->read =
> >> spi_flash_cmd_read_ops;
> >>
> >> /* Compute the flash size */
> >>
> >> - flash->shift = (flash->dual_flash & SF_DUAL_PARALLEL_FLASH) ? 1 :
> >> 0; + flash->shift = (flash->dual_flash & SF_PARALLEL) ? 1 : 0;
> >>
> >> flash->page_size = ((ext_jedec == 0x4d00) ? 512 : 256) <<
> >> flash->shift; flash->sector_size = params->sector_size <<
> >> flash->shift; flash->size = flash->sector_size *
> >> params->nr_sectors << flash->shift;
> >>
> >> #ifdef CONFIG_SF_DUAL_FLASH
> >>
> >> - if (flash->dual_flash & SF_DUAL_STACKED_FLASH)
> >> + if (flash->dual_flash & SF_STACKED)
> >>
> >> flash->size <<= 1;
> >>
> >> #endif
> >>
> >> @@ -167,7 +172,7 @@ static struct spi_flash
> >> *spi_flash_validate_params(struct spi_slave *spi, }
> >>
> >> /* Look for the fastest read cmd */
> >>
> >> - cmd = fls(params->e_rd_cmd & flash->spi->op_mode_rx);
> >> + cmd = fls(params->e_rd_cmd & flash->spi->rx_mode);
> >>
> >> if (cmd) {
> >>
> >> cmd = spi_read_cmds_array[cmd - 1];
> >> flash->read_cmd = cmd;
> >>
> >> @@ -177,7 +182,7 @@ static struct spi_flash
> >> *spi_flash_validate_params(struct spi_slave *spi, }
> >>
> >> /* Not require to look for fastest only two write cmds yet */
> >>
> >> - if (params->flags & WR_QPP && flash->spi->op_mode_tx &
> >> SPI_OPM_TX_QPP) + if (params->flags & WR_QPP &&
> >> flash->spi->mode_bits & SPI_TX_QPP)
> >>
> >> flash->write_cmd = CMD_QUAD_PAGE_PROGRAM;
> >>
> >> else
> >>
> >> /* Go for default supported write cmd */
> >>
> >> @@ -329,9 +334,9 @@ static struct spi_flash
> >> *spi_flash_probe_slave(struct spi_slave *spi) puts("\n");
> >>
> >> #endif
> >> #ifndef CONFIG_SPI_FLASH_BAR
> >>
> >> - if (((flash->dual_flash == SF_SINGLE_FLASH) &&
> >> + if (((flash->dual_flash == SF_SINGLE) &&
> >>
> >> (flash->size > SPI_FLASH_16MB_BOUN)) ||
> >>
> >> - ((flash->dual_flash > SF_SINGLE_FLASH) &&
> >> + ((flash->dual_flash > SF_SINGLE) &&
> >>
> >> (flash->size > SPI_FLASH_16MB_BOUN << 1))) {
> >>
> >> puts("SF: Warning - Only lower 16MiB accessible,");
> >> puts(" Full access #define CONFIG_SPI_FLASH_BAR\n");
> >>
> >> diff --git a/include/spi.h b/include/spi.h
> >> index ffd6647..07ba4d0 100644
> >> --- a/include/spi.h
> >> +++ b/include/spi.h
> >> @@ -30,29 +30,25 @@
> >>
> >> #define SPI_XFER_MMAP 0x08 /* Memory Mapped start */
> >> #define SPI_XFER_MMAP_END 0x10 /* Memory Mapped End */
> >> #define SPI_XFER_ONCE (SPI_XFER_BEGIN | SPI_XFER_END)
> >>
> >> -#define SPI_XFER_U_PAGE (1 << 5)
> >> -
> >> -/* SPI TX operation modes */
> >> -#define SPI_OPM_TX_QPP 1 << 0
> >> -
> >> -/* SPI RX operation modes */
> >> -#define SPI_OPM_RX_AS 1 << 0
> >> -#define SPI_OPM_RX_DOUT 1 << 1
> >> -#define SPI_OPM_RX_DIO 1 << 2
> >> -#define SPI_OPM_RX_QOF 1 << 3
> >> -#define SPI_OPM_RX_QIOF 1 << 4
> >> -#define SPI_OPM_RX_EXTN SPI_OPM_RX_AS | SPI_OPM_RX_DOUT |
> >> \ - SPI_OPM_RX_DIO | SPI_OPM_RX_QOF | \ -
> >> SPI_OPM_RX_QIOF
> >> -
> >> -/* SPI bus connection options */
> >> -#define SPI_CONN_DUAL_SHARED 1 << 0
> >> -#define SPI_CONN_DUAL_SEPARATED 1 << 1
> >>
> >> /* Header byte that marks the start of the message */
> >> #define SPI_PREAMBLE_END_BYTE 0xec
> >>
> >> +#define SPI_DEFAULT_WORDLEN 8
> >> +
> >> +/* SPI RX operation modes */
> >> +#define SPI_RX_AS 1 << 0
> >> +#define SPI_RX_DOUT 1 << 1
> >> +#define SPI_RX_DIO 1 << 2
> >> +#define SPI_RX_QOF 1 << 3
> >> +#define SPI_RX_QIOF 1 << 4
> >> +#define SPI_RX_EXTN SPI_RX_AS | SPI_RX_DOUT | SPI_RX_DIO | \
> >> + SPI_RX_QOF | SPI_RX_QIOF
> >>
> >> -#define SPI_DEFAULT_WORDLEN 8
> >> +/* SPI mode bits */
> >> +#define SPI_TX_QPP 1 << 0
> >> +#define SPI_SHARED 1 << 1
> >> +#define SPI_SEPARATED 1 << 2
> >> +#define SPI_U_PAGE 1 << 3
> >>
> >> /**
> >>
> >> * struct spi_slave - Representation of a SPI slave
> >>
> >> @@ -61,25 +57,22 @@
> >>
> >> *
> >> * @bus: ID of the bus that the slave is attached to.
> >> * @cs: ID of the chip select connected to the
> >> slave.
> >>
> >> - * @op_mode_rx: SPI RX operation mode.
> >> - * @op_mode_tx: SPI TX operation mode.
> >>
> >> * @wordlen: Size of SPI word in number of bits
> >> * @max_write_size: If non-zero, the maximum number of bytes which can
> >> * be written at once, excluding command bytes.
> >> * @memory_map: Address of read-only SPI flash access.
> >>
> >> - * @option: Varies SPI bus options - separate, shared bus.
> >> + * @rx_mode: SPI RX operation mode.
> >> + * @mode_bits: SPI TX operation modes, bus options and
> >> few
> >
> > flags.
> >
> > This naming change doesn't make sense. rx_mode vs. mode_bits don't have
> > any relationship even if they are related apparently.
>
> rx_mode need to separate here as it involve some calculation to find
> fastest command.
... and this is really starting to become horrible nonsense. Fastest command for
SPI NOR, right ? But this is struct spi_slave {} describing generic SPI slave
here, not an SPI NOR!
> > Anyway, I feel we're sinking deeper and deeper into this sh*t, we should
> > instead take a step back and re-think the whole approach until we break
> > it even more.
>
> Yes - will shrink once we plan for new approach.
> But I'm unclear with new SPI-NOR.
It's pretty clear in it's own right, do not polute generic code with SPI NOR
specific stuff .
next prev parent reply other threads:[~2014-01-18 16:59 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1389969707-3781-1-git-send-email-jaganna@xilinx.com>
2014-01-17 14:41 ` [U-Boot] [PATCH 2/3] sf: Shrink spi_slave {} Jagannadha Sutradharudu Teki
2014-01-17 16:31 ` Marek Vasut
2014-01-17 17:12 ` Jagan Teki
2014-01-17 18:29 ` Gerhard Sittig
2014-01-18 17:03 ` Marek Vasut
2014-01-18 16:59 ` Marek Vasut [this message]
2014-01-17 14:41 ` [U-Boot] [PATCH 3/3] sf: Use shortcut names Jagannadha Sutradharudu Teki
2014-01-17 16:31 ` Marek Vasut
2014-01-20 11:05 ` Detlev Zundel
2014-01-20 11:41 ` Jagan Teki
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=201401181759.57890.marex@denx.de \
--to=marex@denx.de \
--cc=u-boot@lists.denx.de \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.