On Tue, Jun 28, 2016 at 8:44 AM, Stefan Roese <s...@denx.de> wrote: > This patch adds support for the SMBus block read/write functionality. > Other protocols like the SMBus quick command need to get added > if this is needed. > > This patch also removed the SMBus related defines from the Ivybridge > pch.h header. As they are integrated in this driver and should be > used from here. This change is added in this patch to avoid compile > breakage to keep the source git bisectable. > > Tested on a congatec BayTrail board to configure the SMSC2513 USB > hub. > > Signed-off-by: Stefan Roese <s...@denx.de> > Cc: Bin Meng <bmeng...@gmail.com> > Cc: Simon Glass <s...@chromium.org> > Cc: Heiko Schocher <h...@denx.de> > --- > Simon, I'm not sure if this change breaks your Ivybridge targets > using the probe part of this driver. Could you please let me > know if this works? Or let me know what needs changes here? > > Thanks, > Stefan > > arch/x86/include/asm/arch-ivybridge/pch.h | 26 --- > drivers/i2c/intel_i2c.c | 290 > ++++++++++++++++++++++++++++-- > 2 files changed, 278 insertions(+), 38 deletions(-) > > diff --git a/arch/x86/include/asm/arch-ivybridge/pch.h > b/arch/x86/include/asm/arch-ivybridge/pch.h > index 4725250..9c51f63 100644 > --- a/arch/x86/include/asm/arch-ivybridge/pch.h > +++ b/arch/x86/include/asm/arch-ivybridge/pch.h > @@ -134,32 +134,6 @@ > #define SATA_IOBP_SP0G3IR 0xea000151 > #define SATA_IOBP_SP1G3IR 0xea000051 > > -/* PCI Configuration Space (D31:F3): SMBus */ > -#define PCH_SMBUS_DEV PCI_BDF(0, 0x1f, 3) > -#define SMB_BASE 0x20 > -#define HOSTC 0x40 > -#define SMB_RCV_SLVA 0x09 > - > -/* HOSTC bits */ > -#define I2C_EN (1 << 2) > -#define SMB_SMI_EN (1 << 1) > -#define HST_EN (1 << 0) > - > -/* SMBus I/O bits. */ > -#define SMBHSTSTAT 0x0 > -#define SMBHSTCTL 0x2 > -#define SMBHSTCMD 0x3 > -#define SMBXMITADD 0x4 > -#define SMBHSTDAT0 0x5 > -#define SMBHSTDAT1 0x6 > -#define SMBBLKDAT 0x7 > -#define SMBTRNSADD 0x9 > -#define SMBSLVDATA 0xa > -#define SMLINK_PIN_CTL 0xe > -#define SMBUS_PIN_CTL 0xf > - > -#define SMBUS_TIMEOUT (10 * 1000 * 100) > - > #define VCH 0x0000 /* 32bit */ > #define VCAP1 0x0004 /* 32bit */ > #define VCAP2 0x0008 /* 32bit */ > diff --git a/drivers/i2c/intel_i2c.c b/drivers/i2c/intel_i2c.c > index 3d777ff..8b63916 100644 > --- a/drivers/i2c/intel_i2c.c > +++ b/drivers/i2c/intel_i2c.c > @@ -2,45 +2,263 @@ > * Copyright (c) 2015 Google, Inc > * Written by Simon Glass <s...@chromium.org> > * > + * SMBus block read/write support added by Stefan Roese: > + * Copyright (C) 2016 Stefan Roese <s...@denx.de> > + * > * SPDX-License-Identifier: GPL-2.0+ > */ > > #include <common.h> > #include <dm.h> > #include <i2c.h> > +#include <pci.h> > #include <asm/io.h> > +#ifdef CONFIG_NORTHBRIDGE_INTEL_IVYBRIDGE > #include <asm/arch/pch.h> > +#endif > + > +/* PCI Configuration Space (D31:F3): SMBus */ > +#define SMB_BASE 0x20 > +#define HOSTC 0x40 > +#define HST_EN (1 << 0) > +#define SMB_RCV_SLVA 0x09 > + > +/* SMBus I/O bits. */ > +#define SMBHSTSTAT 0x0 > +#define SMBHSTCTL 0x2 > +#define SMBHSTCMD 0x3 > +#define SMBXMITADD 0x4 > +#define SMBHSTDAT0 0x5 > +#define SMBHSTDAT1 0x6 > +#define SMBBLKDAT 0x7 > +#define SMBTRNSADD 0x9 > +#define SMBSLVDATA 0xa > +#define SMBAUXCTL 0xd > +#define SMLINK_PIN_CTL 0xe > +#define SMBUS_PIN_CTL 0xf > + > +/* I801 Hosts Status register bits */ > +#define SMBHSTSTS_BYTE_DONE 0x80 > +#define SMBHSTSTS_INUSE_STS 0x40 > +#define SMBHSTSTS_SMBALERT_STS 0x20 > +#define SMBHSTSTS_FAILED 0x10 > +#define SMBHSTSTS_BUS_ERR 0x08 > +#define SMBHSTSTS_DEV_ERR 0x04 > +#define SMBHSTSTS_INTR 0x02 > +#define SMBHSTSTS_HOST_BUSY 0x01 > + > +/* I801 Host Control register bits */ > +#define SMBHSTCNT_INTREN 0x01 > +#define SMBHSTCNT_KILL 0x02 > +#define SMBHSTCNT_LAST_BYTE 0x20 > +#define SMBHSTCNT_START 0x40 > +#define SMBHSTCNT_PEC_EN 0x80 /* ICH3 and later */ > + > +/* Auxiliary control register bits, ICH4+ only */ > +#define SMBAUXCTL_CRC 1 > +#define SMBAUXCTL_E32B 2 > + > +#define SMBUS_TIMEOUT 100 /* 100 ms */ > + > +struct intel_i2c { > + u32 base; > + int running; > +}; > + > +static int smbus_wait_until_ready(u32 base) > +{ > + unsigned long ts; > + u8 byte; > + > + ts = get_timer(0); > + do { > + byte = inb(base + SMBHSTSTAT); > + if (!(byte & 1)) > + return 0; > + } while (get_timer(ts) < SMBUS_TIMEOUT); > + > + return -ETIMEDOUT; > +} > + > +static int smbus_wait_until_done(u32 base) > +{ > + unsigned long ts; > + u8 byte; > + > + ts = get_timer(0); > + do { > + byte = inb(base + SMBHSTSTAT); > + if (!((byte & 1) || (byte & ~((1 << 6) | (1 << 0))) == 0)) > + return 0; > + } while (get_timer(ts) < SMBUS_TIMEOUT); > + > + return -ETIMEDOUT; > +} > + > +static int smbus_block_read(u32 base, u8 dev, u8 *buffer, > + int offset, int len) > +{ > + u8 buf_temp[32]; > + int count; > + int i; > + > + debug("%s (%d): dev=0x%x offs=0x%x len=0x%x\n", > + __func__, __LINE__, dev, offset, len); > + if (smbus_wait_until_ready(base) < 0) > + return -ETIMEDOUT; > + > + /* Setup transaction */ > + > + /* Reset the data buffer index */ > + inb(base + SMBHSTCTL); > + > + /* Set the device I'm talking too */ > + outb(((dev & 0x7f) << 1) | 1, base + SMBXMITADD); > + /* Set the command/address... */ > + outb(offset & 0xff, base + SMBHSTCMD); > + /* Set up for a block read */ > + outb((inb(base + SMBHSTCTL) & (~(0x7) << 2)) | (0x5 << 2), > + (base + SMBHSTCTL)); > + /* Clear any lingering errors, so the transaction will run */ > + outb(inb(base + SMBHSTSTAT), base + SMBHSTSTAT); > + > + /* Start the command */ > + outb((inb(base + SMBHSTCTL) | SMBHSTCNT_START), base + SMBHSTCTL); > + > + /* Poll for transaction completion */ > + if (smbus_wait_until_done(base) < 0) { > + printf("SMBUS read transaction timeout (dev=0x%x)\n", dev); > + return -ETIMEDOUT; > + } > + > + count = inb(base + SMBHSTDAT0);
I think always doing a block read is a bit problematic. I've tested the driver (on Baytrail) with several SMBus devices. This example is a PCA9554 8-Bit I2C AND SMBus I/O Expander (datasheet http://www.ti.com/lit/ds/symlink/pca9554.pdf): i2c md 20 0 5 i2c_xfer: 2 messages smbus_block_read (108): dev=0x20 offs=0x0 len=0x5 smbus_block_read (139): count=127 (len=5) Invalid Opcode (Undefined Opcode) EIP: 0010:[<80000001>] EFLAGS: 00010a92 Original EIP :[<049a0001>] EAX: 00000000 EBX: 7f7ffe7f ECX: 00000000 EDX: 0000b000 ESI: 7f7f7f7f EDI: 7f7f7f7f EBP: 7f7f7f7f ESP: 7b34c3b0 DS: 0018 ES: 0018 FS: 0020 GS: 0018 SS: 0018 CR0: 00000033 CR2: 00000000 CR3: 00000000 CR4: 00000600 DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000 DR6: ffff0ff0 DR7: 00000400 Stack: 0x7b34c3f0 : 0x7b36bd60 0x7b34c3ec : 0x7b7f7f7f 0x7b34c3e8 : 0x7f7f7f7f 0x7b34c3e4 : 0x7f7f7f7f 0x7b34c3e0 : 0x7f7f7f7f 0x7b34c3dc : 0x7f7f7f7f 0x7b34c3d8 : 0x7f7f7f7f 0x7b34c3d4 : 0x7f7f7f7f 0x7b34c3d0 : 0x7f7f7f7f 0x7b34c3cc : 0x7f7f7f7f 0x7b34c3c8 : 0x7f7f7f7f 0x7b34c3c4 : 0x7f7f7f7f 0x7b34c3c0 : 0x7f7f7f7f 0x7b34c3bc : 0x7f7f7f7f 0x7b34c3b8 : 0x7f7f7f7f 0x7b34c3b4 : 0x7f7f7f7f --->0x7b34c3b0 : 0x7f7f7f7f 0x7b34c3ac : 0x00010a92 0x7b34c3a8 : 0x00000010 0x7b34c3a4 : 0x80000001 ### ERROR ### Please RESET the board ### This device always returns the data instead of providing a proper count. In order for this driver to be usable with this part it needs to support a byte read. Any ideas how we could support both byte and block read? > + debug("%s (%d): count=%d (len=%d)\n", __func__, __LINE__, count, len); > + if (count == 0) { > + debug("ERROR: len=0 on read\n"); > + return -EIO; > + } > + > + if (count < len) { > + debug("ERROR: too few bytes read\n"); > + return -EIO; > + } > + > + /* Read all available bytes from buffer */ > + for (i = 0; i < count; i++) > + buf_temp[i] = inb(base + SMBBLKDAT); No overrun check, if count is > 32 buffer is over run. In my case the data returned by the PCA9554 is interpreted as count and an overrun occurs. > + > + memcpy(buffer, buf_temp, len); > + > + /* Return results of transaction */ > + if (!(inb(base + SMBHSTSTAT) & SMBHSTSTS_INTR)) > + return -EIO; > + > + return 0; > +} > + > +static int smbus_block_write(u32 base, u8 dev, u8 *buffer, > + int offset, int len) > +{ > + int i; > > -int intel_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int nmsgs) > + debug("%s (%d): dev=0x%x offs=0x%x len=0x%x\n", > + __func__, __LINE__, dev, offset, len); > + if (smbus_wait_until_ready(base) < 0) > + return -ETIMEDOUT; > + > + /* Setup transaction */ > + /* Set the device I'm talking too */ > + outb(((dev & 0x7f) << 1) & ~0x01, base + SMBXMITADD); > + /* Set the command/address... */ > + outb(offset, base + SMBHSTCMD); > + /* Set up for a block write */ > + outb((inb(base + SMBHSTCTL) & (~(0x7) << 2)) | (0x5 << 2), > + (base + SMBHSTCTL)); > + /* Clear any lingering errors, so the transaction will run */ > + outb(inb(base + SMBHSTSTAT), base + SMBHSTSTAT); > + > + /* Write count in DAT0 register */ > + outb(len, base + SMBHSTDAT0); > + > + /* Write data bytes... */ > + for (i = 0; i < len; i++) > + outb(*buffer++, base + SMBBLKDAT); > + > + /* Start the command */ > + outb((inb(base + SMBHSTCTL) | SMBHSTCNT_START), base + SMBHSTCTL); > + > + /* Poll for transaction completion */ > + if (smbus_wait_until_done(base) < 0) { > + printf("SMBUS write transaction timeout (dev=0x%x)\n", dev); > + return -ETIMEDOUT; > + } > + > + /* Return results of transaction */ > + if (!(inb(base + SMBHSTSTAT) & SMBHSTSTS_INTR)) > + return -EIO; > + > + return 0; > +} > + > +static int intel_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int > nmsgs) > { > - return -ENOSYS; > + struct intel_i2c *i2c = dev_get_priv(bus); > + struct i2c_msg *dmsg, *omsg, dummy; > + > + debug("i2c_xfer: %d messages\n", nmsgs); > + > + memset(&dummy, 0, sizeof(struct i2c_msg)); > + > + /* > + * We expect either two messages (one with an offset and one with the > + * actucal data) or one message (just data) > + */ > + if (nmsgs > 2 || nmsgs == 0) { > + debug("%s: Only one or two messages are supported", __func__); > + return -EIO; > + } > + > + omsg = nmsgs == 1 ? &dummy : msg; > + dmsg = nmsgs == 1 ? msg : msg + 1; > + > + if (dmsg->flags & I2C_M_RD) > + return smbus_block_read(i2c->base, dmsg->addr, &dmsg->buf[0], > + omsg->buf[0], dmsg->len); > + else > + return smbus_block_write(i2c->base, dmsg->addr, &dmsg->buf[1], > + dmsg->buf[0], dmsg->len - 1); > } > > -int intel_i2c_probe_chip(struct udevice *bus, uint chip_addr, uint > chip_flags) > +static int intel_i2c_probe_chip(struct udevice *bus, uint chip_addr, > + uint chip_flags) > { > - return -ENOSYS; > + struct intel_i2c *i2c = dev_get_priv(bus); > + u8 buf[4]; > + > + return smbus_block_read(i2c->base, chip_addr, buf, 0, 1); > } > > -int intel_i2c_set_bus_speed(struct udevice *bus, unsigned int speed) > +static int intel_i2c_set_bus_speed(struct udevice *bus, unsigned int speed) > { > return 0; > } > > static int intel_i2c_probe(struct udevice *dev) > { > + struct intel_i2c *priv = dev_get_priv(dev); > + u32 base; > + > +#ifdef CONFIG_NORTHBRIDGE_INTEL_IVYBRIDGE > + /* Handle ivybridge setup code */ > + priv->base = SMBUS_IO_BASE; > + base = SMBUS_IO_BASE; > + > /* > - * So far this is just setup code for ivybridge SMbus. When we have > - * a full I2C driver this may need to be moved, generalised or made > - * dependant on a particular compatible string. > - * > * Set SMBus I/O base > */ > dm_pci_write_config32(dev, SMB_BASE, > SMBUS_IO_BASE | PCI_BASE_ADDRESS_SPACE_IO); > > - /* Set SMBus enable. */ > - dm_pci_write_config8(dev, HOSTC, HST_EN); > - > /* Set SMBus I/O space enable. */ > dm_pci_write_config16(dev, PCI_COMMAND, PCI_COMMAND_IO); > > @@ -50,6 +268,44 @@ static int intel_i2c_probe(struct udevice *dev) > /* Clear any lingering errors, so transactions can run. */ > outb(inb(SMBUS_IO_BASE + SMBHSTSTAT), SMBUS_IO_BASE + SMBHSTSTAT); > debug("SMBus controller enabled\n"); > +#else > + /* Save base address from PCI BAR */ > + priv->base = (u32)dm_pci_map_bar(dev, PCI_BASE_ADDRESS_4, > + PCI_REGION_IO); > + base = priv->base; > +#endif > + > + /* Set SMBus enable. */ > + dm_pci_write_config8(dev, HOSTC, HST_EN); > + > + /* Disable interrupts */ > + outb(inb(base + SMBHSTCTL) & ~SMBHSTCNT_INTREN, base + SMBHSTCTL); > + > + /* Set 32-byte data buffer mode */ > + outb(inb(base + SMBAUXCTL) | SMBAUXCTL_E32B, base + SMBAUXCTL); > + > + return 0; > +} > + > +static int intel_i2c_bind(struct udevice *dev) > +{ > + static int num_cards; > + char name[20]; > + > + /* Create a unique device name for PCI type devices */ > + if (device_is_on_pci_bus(dev)) { > + /* > + * ToDo: > + * Setting req_seq in the driver is probably not recommended. > + * But without a DT alias the number is not configured. And > + * using this driver is impossible for PCIe I2C devices. > + * This can be removed, once a better (correct) way for this > + * is found and implemented. > + */ > + dev->req_seq = num_cards; > + sprintf(name, "intel_i2c#%u", num_cards++); > + device_set_name(dev, name); > + } > > return 0; > } > @@ -71,5 +327,15 @@ U_BOOT_DRIVER(intel_i2c) = { > .of_match = intel_i2c_ids, > .per_child_auto_alloc_size = sizeof(struct dm_i2c_chip), > .ops = &intel_i2c_ops, > + .priv_auto_alloc_size = sizeof(struct intel_i2c), > + .bind = intel_i2c_bind, > .probe = intel_i2c_probe, > }; > + > +static struct pci_device_id intel_smbus_pci_supported[] = { > + /* Intel BayTrail SMBus on the PCI bus */ > + { PCI_VDEVICE(INTEL, 0x0f12) }, > + {}, > +}; > + > +U_BOOT_PCI_DEVICE(intel_i2c, intel_smbus_pci_supported); > -- > 2.9.0 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot