| Message ID | 1422321801-6743-20-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Simon, On Tue, Jan 27, 2015 at 9:23 AM, Simon Glass <sjg@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@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
Hi Bin, On 27 January 2015 at 07:00, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Tue, Jan 27, 2015 at 9:23 AM, Simon Glass <sjg@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@chromium.org> >> --- >> >> drivers/spi/ich.c | 56 ++++++++++++++++++++++++++++++++++++++++--------------- >> drivers/spi/ich.h | 11 ++++++----- >> 2 files changed, 47 insertions(+), 20 deletions(-) Just a note on this one...for v2 I have done nothing. I think we should move it to device tree as you say but it needs some more thought, as we have register offsets and device types to think about. I will try a few things and test on link also. Once I have this I will send a v3. So this patch will have to sit here until that is done, hopefully only a few days. Thanks for the quick reviews! Regards, Simon
Hi Simon, On Wed, Jan 28, 2015 at 1:17 PM, Simon Glass <sjg@chromium.org> wrote: > Hi Bin, > > On 27 January 2015 at 07:00, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Simon, >> >> On Tue, Jan 27, 2015 at 9:23 AM, Simon Glass <sjg@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@chromium.org> >>> --- >>> >>> drivers/spi/ich.c | 56 ++++++++++++++++++++++++++++++++++++++++--------------- >>> drivers/spi/ich.h | 11 ++++++----- >>> 2 files changed, 47 insertions(+), 20 deletions(-) > > Just a note on this one...for v2 I have done nothing. I think we > should move it to device tree as you say but it needs some more > thought, as we have register offsets and device types to think about. > I will try a few things and test on link also. Once I have this I will > send a v3. So this patch will have to sit here until that is done, > hopefully only a few days. > OK, I think we can just apply this patch for now along with other patches in this series. We can revisit this driver when adding DT and DM support. > Thanks for the quick reviews! > You are welcome! Regards, Bin
Hi Bin, On 27 January 2015 at 23:02, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Wed, Jan 28, 2015 at 1:17 PM, Simon Glass <sjg@chromium.org> wrote: >> Hi Bin, >> >> On 27 January 2015 at 07:00, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Simon, >>> >>> On Tue, Jan 27, 2015 at 9:23 AM, Simon Glass <sjg@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@chromium.org> >>>> --- >>>> >>>> drivers/spi/ich.c | 56 ++++++++++++++++++++++++++++++++++++++++--------------- >>>> drivers/spi/ich.h | 11 ++++++----- >>>> 2 files changed, 47 insertions(+), 20 deletions(-) >> >> Just a note on this one...for v2 I have done nothing. I think we >> should move it to device tree as you say but it needs some more >> thought, as we have register offsets and device types to think about. >> I will try a few things and test on link also. Once I have this I will >> send a v3. So this patch will have to sit here until that is done, >> hopefully only a few days. >> > > OK, I think we can just apply this patch for now along with other > patches in this series. We can revisit this driver when adding DT and > DM support. Oh OK we can do it that way. I actually have some partial work to convert the ICH driver to DM, so I'll go back to that soon. Applied to u-boot-x86. Regards, Simon
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; } @@ -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) 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); + 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); 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); + } 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 {
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@chromium.org> --- drivers/spi/ich.c | 56 ++++++++++++++++++++++++++++++++++++++++--------------- drivers/spi/ich.h | 11 ++++++----- 2 files changed, 47 insertions(+), 20 deletions(-)