[U-Boot] [PATCH 19/23] x86: spi: Support ValleyView in ICH SPI driver

Bin Meng bmeng.cn at gmail.com
Tue Jan 27 15:00:19 CET 2015


Hi Simon,

On Tue, Jan 27, 2015 at 9:23 AM, Simon Glass <sjg at chromium.org> wrote:
> The base address is found in a different way and the protection bit is also
> in a different place. Otherwise it is very similar.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>
>  drivers/spi/ich.c | 56 ++++++++++++++++++++++++++++++++++++++++---------------
>  drivers/spi/ich.h | 11 ++++++-----
>  2 files changed, 47 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
> index fdff158..da85779 100644
> --- a/drivers/spi/ich.c
> +++ b/drivers/spi/ich.c
> @@ -7,6 +7,7 @@
>   */
>
>  #include <common.h>
> +#include <errno.h>
>  #include <malloc.h>
>  #include <spi.h>
>  #include <pci.h>
> @@ -21,6 +22,7 @@
>  struct ich_ctlr {
>         pci_dev_t dev;          /* PCI device number */
>         int ich_version;        /* Controller version, 7 or 9 */
> +       bool use_sbase;         /* Use SBASE instead of RCB */
>         int ichspi_lock;
>         int locked;
>         uint8_t *opmenu;
> @@ -145,7 +147,7 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
>          * ICH 7 SPI controller only supports array read command
>          * and byte program command for SST flash
>          */
> -       if (ctlr.ich_version == 7) {
> +       if (ctlr.ich_version == 7 || ctlr.use_sbase) {
>                 ich->slave.op_mode_rx = SPI_OPM_RX_AS;
>                 ich->slave.op_mode_tx = SPI_OPM_TX_BP;
>         }

So the Valleyview SPI controller (version 9) has the same behavior
that was seen on Tunnel Creek (version 7). I wonder whether the same
behavior was seen on chromebook_link which is also version 9? Or is
this only because the Minnowmax board has the same SST flash mounted
on board, just like the Crown Bay so that we need set these RX/TX
flags. If SPI controller on chromebook_link also has the simliar
behavior, we should consider set these flags for all Intel SPI
controller variants. This way, the 'use_sbase' variable may not be
necessary (see below for more comments).

> @@ -181,7 +183,8 @@ static int get_ich_version(uint16_t device_id)
>         if ((device_id >= PCI_DEVICE_ID_INTEL_COUGARPOINT_LPC_MIN &&
>              device_id <= PCI_DEVICE_ID_INTEL_COUGARPOINT_LPC_MAX) ||
>             (device_id >= PCI_DEVICE_ID_INTEL_PANTHERPOINT_LPC_MIN &&
> -            device_id <= PCI_DEVICE_ID_INTEL_PANTHERPOINT_LPC_MAX))
> +            device_id <= PCI_DEVICE_ID_INTEL_PANTHERPOINT_LPC_MAX) ||
> +           device_id == PCI_DEVICE_ID_INTEL_VALLEYVIEW_LPC)

To avoid bigger and bigger test logic, should we consider using DT to
pass such information?

>                 return 9;
>
>         return 0;
> @@ -204,14 +207,14 @@ static int ich9_can_do_33mhz(pci_dev_t dev)
>         return speed == 1;
>  }
>
> -static int ich_find_spi_controller(pci_dev_t *devp, int *ich_versionp)
> +static int ich_find_spi_controller(struct ich_ctlr *ich)
>  {
>         int last_bus = pci_last_busno();
>         int bus;
>
>         if (last_bus == -1) {
>                 debug("No PCI busses?\n");
> -               return -1;
> +               return -ENODEV;
>         }
>
>         for (bus = 0; bus <= last_bus; bus++) {
> @@ -225,24 +228,33 @@ static int ich_find_spi_controller(pci_dev_t *devp, int *ich_versionp)
>                 device_id = ids >> 16;
>
>                 if (vendor_id == PCI_VENDOR_ID_INTEL) {
> -                       *devp = dev;
> -                       *ich_versionp = get_ich_version(device_id);
> -                       return 0;
> +                       ich->dev = dev;
> +                       ich->ich_version = get_ich_version(device_id);
> +                       if (device_id == PCI_DEVICE_ID_INTEL_VALLEYVIEW_LPC)
> +                               ich->use_sbase = true;
> +                       return ich->ich_version == 0 ? -ENODEV : 0;
>                 }
>         }
>
>         debug("ICH SPI: No ICH found.\n");
> -       return -1;
> +       return -ENODEV;
>  }
>
>  static int ich_init_controller(struct ich_ctlr *ctlr)
>  {
>         uint8_t *rcrb; /* Root Complex Register Block */
>         uint32_t rcba; /* Root Complex Base Address */
> +       uint32_t sbase_addr;
> +       uint8_t *sbase;
>
>         pci_read_config_dword(ctlr->dev, 0xf0, &rcba);
>         /* Bits 31-14 are the base address, 13-1 are reserved, 0 is enable. */
>         rcrb = (uint8_t *)(rcba & 0xffffc000);
> +
> +       /* SBASE is similar */
> +       pci_read_config_dword(ctlr->dev, 0x54, &sbase_addr);
> +       sbase = (uint8_t *)(sbase_addr & 0xfffffe00);
> +

Again, DT can be used to determine the base address.

>         if (ctlr->ich_version == 7) {
>                 struct ich7_spi_regs *ich7_spi;
>
> @@ -262,7 +274,10 @@ static int ich_init_controller(struct ich_ctlr *ctlr)
>         } else if (ctlr->ich_version == 9) {
>                 struct ich9_spi_regs *ich9_spi;
>
> -               ich9_spi = (struct ich9_spi_regs *)(rcrb + 0x3800);
> +               if (ctlr->use_sbase)
> +                       ich9_spi = (struct ich9_spi_regs *)sbase;
> +               else
> +                       ich9_spi = (struct ich9_spi_regs *)(rcrb + 0x3800);

Ditto.

>                 ctlr->ichspi_lock = ich_readw(&ich9_spi->hsfs) & HSFS_FLOCKDN;
>                 ctlr->opmenu = ich9_spi->opmenu;
>                 ctlr->menubytes = sizeof(ich9_spi->opmenu);
> @@ -282,12 +297,13 @@ static int ich_init_controller(struct ich_ctlr *ctlr)
>                       ctlr->ich_version);
>                 return -1;
>         }
> -       debug("ICH SPI: Version %d detected\n", ctlr->ich_version);
>
>         /* Work out the maximum speed we can support */
>         ctlr->max_speed = 20000000;
>         if (ctlr->ich_version == 9 && ich9_can_do_33mhz(ctlr->dev))
>                 ctlr->max_speed = 33000000;
> +       debug("ICH SPI: Version %d detected at %p, speed %ld\n",
> +             ctlr->ich_version, ctlr->base, ctlr->max_speed);
>
>         ich_set_bbar(ctlr, 0);
>
> @@ -298,7 +314,7 @@ void spi_init(void)
>  {
>         uint8_t bios_cntl;
>
> -       if (ich_find_spi_controller(&ctlr.dev, &ctlr.ich_version)) {
> +       if (ich_find_spi_controller(&ctlr)) {
>                 printf("ICH SPI: Cannot find device\n");
>                 return;
>         }
> @@ -312,10 +328,20 @@ void spi_init(void)
>          * Disable the BIOS write protect so write commands are allowed.  On
>          * v9, deassert SMM BIOS Write Protect Disable.
>          */
> -       pci_read_config_byte(ctlr.dev, 0xdc, &bios_cntl);
> -       if (ctlr.ich_version == 9)
> -               bios_cntl &= ~(1 << 5);
> -       pci_write_config_byte(ctlr.dev, 0xdc, bios_cntl | 0x1);
> +       if (ctlr.use_sbase) {
> +               struct ich9_spi_regs *ich9_spi;
> +
> +               ich9_spi = (struct ich9_spi_regs *)ctlr.base;
> +               bios_cntl = ich_readb(&ich9_spi->bcr);
> +               bios_cntl &= ~(1 << 5); /* clear Enable InSMM_STS (EISS) */
> +               bios_cntl |= 1;         /* Write Protect Disable (WPD) */
> +               ich_writeb(bios_cntl, &ich9_spi->bcr);

Basically the register bit layout is the same as the non-sbase
controller, only the register address is differnt (0xFC vs. 0xDC).
Again we can use DT to pass this to the driver.

> +       } else {
> +               pci_read_config_byte(ctlr.dev, 0xdc, &bios_cntl);
> +               if (ctlr.ich_version == 9)
> +                       bios_cntl &= ~(1 << 5);
> +               pci_write_config_byte(ctlr.dev, 0xdc, bios_cntl | 0x1);
> +       }
>  }
>
>  int spi_claim_bus(struct spi_slave *slave)
> diff --git a/drivers/spi/ich.h b/drivers/spi/ich.h
> index d2e4b85..1419b23 100644
> --- a/drivers/spi/ich.h
> +++ b/drivers/spi/ich.h
> @@ -37,18 +37,19 @@ struct ich9_spi_regs {
>         uint8_t opmenu[8];              /* 0x98 */
>         uint32_t bbar;
>         uint8_t _reserved3[12];
> -       uint32_t fdoc;
> +       uint32_t fdoc;                  /* 0xb0 */
>         uint32_t fdod;
>         uint8_t _reserved4[8];
> -       uint32_t afc;
> +       uint32_t afc;                   /* 0xc0 */
>         uint32_t lvscc;
>         uint32_t uvscc;
>         uint8_t _reserved5[4];
> -       uint32_t fpb;
> +       uint32_t fpb;                   /* 0xd0 */
>         uint8_t _reserved6[28];
> -       uint32_t srdl;
> +       uint32_t srdl;                  /* 0xf0 */
>         uint32_t srdc;
> -       uint32_t srd;
> +       uint32_t scs;
> +       uint32_t bcr;
>  } __packed;
>
>  enum {
> --

Regards,
Bin


More information about the U-Boot mailing list