On Tue, Jan 29, 2013 at 03:05:51PM +0100, Jean Delvare wrote:
> Hi Neil,
>
> On Mon, 28 Jan 2013 14:43:52 -0500, Neil Horman wrote:
> > The iSMT (Intel SMBus Message Transport) supports multi-master I2C/SMBus,
> > as well as IPMI. It's operation is DMA-based and utilizes descriptors to
> > initiate transactions on the bus.
> >
> > The iSMT hardware can act as both a master and a target, although this
> > driver only supports being a master.
> >
> > Signed-off-by: Neil Horman <nhorman@xxxxxxxxxxxxx>
> > Signed-off-by: Bill Brown <bill.e.brown@xxxxxxxxx>
> > Tested-by: Seth Heasley <seth.heasley@xxxxxxxxx>
> > CC: Seth Heasley <seth.heasley@xxxxxxxxx>
> > CC: Jean Delvare <khali@xxxxxxxxxxxx>
> >
> > ---
> > Forgive the latency in this reply, Jean, Bill has gone on sabbatical, so I
> > finished this up on behalf of Intel.
>
> No problem, thanks for stepping in and sending this updated version.
> I'm afraid we'll need one more round though (partly my fault, sorry
> about that), see my comments in-line.
>
Understood, and no problem, I'll square it up.
> BTW, when I asked Bill several month ago for a datasheet for this part,
> he told me it wasn't available publicly yet. Has the situation changed
> meanwhile? There is an issue with SMBus block reads, as you'll see
> below. Without a datasheet I can't verify how it can be solved, so
> you'll have to do it.
>
I'm afraid not (at least not that I know of), the only reason that I was able to
post this was because most of the changes were fairly trivial. I'll see if I
can't push to get a datasheet released however, either publically, or to myself,
so that I can verify the issue.
> > +
> > +Module Parameters
> > +-----------------
> > +
> > +* bus_speed (unsigned int)
> > +Allows changing of the bus speed. Normally, the bus speed is set by the BIOS
> > +and never needs to be changed. However, some SMBus analyzers are too slow for
> > +monitoring the bus during debug, thus the need for this module parameter.
> > +Specify the bus speed in units of KHz.
>
> "in kHz" should do.
>
No problem.
> > +Available bus frequency settings:
> > + 0 no change
> > + 80 kHz
> > + 100 kHz
> > + 400 kHz
> > + 1000 KHz
>
> Should still be a lowercase "k" even if it's a big number ;)
>
Check.
> > +
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/pci.h>
> > +#include <linux/kernel.h>
> > +#include <linux/stddef.h>
> > +#include <linux/completion.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/i2c.h>
> > +#include <linux/acpi.h>
> > +#include <linux/interrupt.h>
> > +
> > +#include <asm-generic/io-64-nonatomic-lo-hi.h>
>
> This should fix the build issue on X86_32. This also means that you
> should now be able to write register value ISMT_MSTR_MDBA in
> ismt_hw_init() using writeq(), as was done originally, instead of two
> writel(). This would be both more efficient and more consistent.
>
Understood, I'll put that back.
> There are two build warnings left on 32-bit, see below.
>
> > (...)
> > +/* Bus speed control bits for slow debuggers - refer to the docs for usage */
> > +static unsigned int bus_speed;
> > +module_param(bus_speed, uint, S_IRUGO);
> > +MODULE_PARM_DESC(bus_speed, "Bus Speed in KHz (0 = BIOS default)");
>
> Proper casing is "kHz".
>
yup
> > (...)
> > +/**
> > + * ismt_gen_reg_dump() - dump the iSMT General Registers
> > + * @priv: iSMT private data
> > + */
> > +static void ismt_gen_reg_dump(struct ismt_priv *priv)
> > +{
> > + struct device *dev = &priv->pci_dev->dev;
> > +
> > + dev_dbg(dev, "Dump of the iSMT General Registers\n");
> > + dev_dbg(dev, " GCTRL.... : (0x%p)=0x%X\n",
> > + priv->smba + ISMT_GR_GCTRL,
> > + readl(priv->smba + ISMT_GR_GCTRL));
> > + dev_dbg(dev, " SMTICL... : (0x%p)=0x%016lX\n",
> > + priv->smba + ISMT_GR_SMTICL,
> > + readq(priv->smba + ISMT_GR_SMTICL));
>
> drivers/i2c/busses/i2c-ismt.c: In function ‘ismt_gen_reg_dump’:
> drivers/i2c/busses/i2c-ismt.c:232: warning: format ‘%016lX’ expects type ‘long unsigned int’, but argument 5 has type ‘__u64’
>
Doh! My bad, I'll fix that up.
> > + dev_dbg(dev, " ERRINTMSK : (0x%p)=0x%X\n",
> > + priv->smba + ISMT_GR_ERRINTMSK,
> > + readl(priv->smba + ISMT_GR_ERRINTMSK));
> > + dev_dbg(dev, " ERRAERMSK : (0x%p)=0x%X\n",
> > + priv->smba + ISMT_GR_ERRAERMSK,
> > + readl(priv->smba + ISMT_GR_ERRAERMSK));
> > + dev_dbg(dev, " ERRSTS... : (0x%p)=0x%X\n",
> > + priv->smba + ISMT_GR_ERRSTS,
> > + readl(priv->smba + ISMT_GR_ERRSTS));
> > + dev_dbg(dev, " ERRINFO.. : (0x%p)=0x%X\n",
> > + priv->smba + ISMT_GR_ERRINFO,
> > + readl(priv->smba + ISMT_GR_ERRINFO));
> > +}
> > +
> > +/**
> > + * ismt_mstr_reg_dump() - dump the iSMT Master Registers
> > + * @priv: iSMT private data
> > + */
> > +static void ismt_mstr_reg_dump(struct ismt_priv *priv)
> > +{
> > + struct device *dev = &priv->pci_dev->dev;
> > +
> > + dev_dbg(dev, "Dump of the iSMT Master Registers\n");
> > + dev_dbg(dev, " MDBA..... : (0x%p)=0x%016lX\n",
> > + priv->smba + ISMT_MSTR_MDBA,
> > + readq(priv->smba + ISMT_MSTR_MDBA));
>
> drivers/i2c/busses/i2c-ismt.c: In function ‘ismt_mstr_reg_dump’:
> drivers/i2c/busses/i2c-ismt.c:258: warning: format ‘%016lX’ expects type ‘long unsigned int’, but argument 5 has type ‘__u64’
>
Ditto.
> > + dev_dbg(dev, " MCTRL.... : (0x%p)=0x%X\n",
> > + priv->smba + ISMT_MSTR_MCTRL,
> > + readl(priv->smba + ISMT_MSTR_MCTRL));
> > + dev_dbg(dev, " MSTS..... : (0x%p)=0x%X\n",
> > + priv->smba + ISMT_MSTR_MSTS,
> > + readl(priv->smba + ISMT_MSTR_MSTS));
> > + dev_dbg(dev, " MDS...... : (0x%p)=0x%X\n",
> > + priv->smba + ISMT_MSTR_MDS,
> > + readl(priv->smba + ISMT_MSTR_MDS));
> > + dev_dbg(dev, " RPOLICY.. : (0x%p)=0x%X\n",
> > + priv->smba + ISMT_MSTR_RPOLICY,
> > + readl(priv->smba + ISMT_MSTR_RPOLICY));
> > + dev_dbg(dev, " SPGT..... : (0x%p)=0x%X\n",
> > + priv->smba + ISMT_SPGT,
> > + readl(priv->smba + ISMT_SPGT));
> > +}
> > +
> > (...)
> > +/**
> > + * ismt_process_desc() - handle the completion of the descriptor
> > + * @desc: the iSMT hardware descriptor
> > + * @data: data buffer from the upper layer
> > + * @dma_buffer: temporary buffer for the DMA engine
> > + * @size: SMBus transaction type
> > + */
> > +static int ismt_process_desc(const struct ismt_desc *desc,
> > + union i2c_smbus_data *data,
> > + u8 *dma_buffer, int size,
> > + char read_write)
> > +{
> > + if ((read_write == I2C_SMBUS_READ) &&
> > + (desc->status & ISMT_DESC_SCS)) {
>
> You added the "read_write == I2C_SMBUS_READ" test in the wrong place.
> You should add it below, to limit the memcpy to read transactions.
> Putting it here instead will cause all write transactions to be
> reported as having failed. Which brings a question... you did not test
> the updated driver, did you?
>
I passed it over to intel for testing, and Seth, who is copied, gave it a thumbs
up. Seth, can you elaborate?
> > + if (size != I2C_SMBUS_QUICK)
> > + memcpy(data, dma_buffer,
> > + min(sizeof(*data), (size_t)I2C_SMBUS_BLOCK_MAX));
>
> This computation looks wrong. We already know that sizeof(*data) ==
> sizeof(union i2c_smbus_data) > I2C_SMBUS_BLOCK_MAX, so the min() above
> will always return I2C_SMBUS_BLOCK_MAX. Which may be insufficient as
> the first byte holds the length, and at the same time it will be more
> than needed in most cases.
>
Gah! Sorry, you're right, I wasn't paying attention and though that data was
just an allocated buffer, I need to fix that.
> My original suggestion to limit the size of the copy was meant as a
> run-time optimization, i.e. copying exactly the number of bytes that
> were received from the slave. That would be 1 or 2 for non-block reads,
> and the block length for block reads. Unfortunately I'm not sure is the
> block length is available. See my comment about this below in
> ismt_access().
>
We don't currently have the dma_size, no, but the only calling function computes
it (either from the block header or sets it appropriately for non-block
transfers). We can pass that into the process_desc function and fix this.
> > +
> > +/**
> > + * ismt_access() - process an SMBus command
> > + * @adap: the i2c host adapter
> > + * @addr: address of the i2c/SMBus target
> > + * @flags: command options
> > + * @read_write: read from or write to device
> > + * @command: the i2c/SMBus command to issue
> > + * @size: SMBus transaction type
> > + * @data: read/write data buffer
> > + */
> > +static int ismt_access(struct i2c_adapter *adap, u16 addr,
> > + unsigned short flags, char read_write, u8 command,
> > + int size, union i2c_smbus_data *data)
> > +{
> > + int ret;
> > + dma_addr_t dma_addr = 0; /* address of the data buffer */
> > + u8 dma_size = 0;
> > + enum dma_data_direction dma_direction = 0;
> > + struct ismt_desc *desc;
> > + struct ismt_priv *priv = i2c_get_adapdata(adap);
> > + struct device *dev = &priv->pci_dev->dev;
> > +
> > + desc = &priv->hw[priv->head];
> > +
> > + /* Initialize the descriptor */
> > + memset(desc, 0, sizeof(struct ismt_desc));
> > + desc->tgtaddr_rw = ISMT_DESC_ADDR_RW(addr, read_write);
> > +
> > + /* Create a temporary buffer for the DMA transaction */
> > + /* and insert the command at the beginning of the buffer */
> > + if ((read_write == I2C_SMBUS_WRITE) &&
> > + (size != I2C_SMBUS_QUICK)) {
>
> If I read the code below properly, this can be skipped for size ==
> I2C_SMBUS_BYTE too, right?
I don't think so. The command field contains the data, and that is right below
the memcpy in that if clause. We can certainly optimize this however, given
that I need to fix the memcpy sizes anyway.
>
> > + memcpy(priv->dma_buffer + 1, data,
> > + min(sizeof(*data), (size_t)I2C_SMBUS_BLOCK_MAX));
>
> Thinking about it some more, this is wrong for block transactions.
> Sorry for not spotting it before. For block transactions,
> data->block[0] is used for the length, so you can't just copy the union
> i2c_smbus_data blindly to dma_buffer. You have to start copying from
> data->block[1], and data->block[0] could be used to limit the length of
> the memcpy. For non-block writes, length can only be 1 or 2 so you
> could limit it to 2.
>
understood.
> > + priv->dma_buffer[0] = command;
> > + }
> > +
> > + /* Initialize common control bits */
> > + if (likely(priv->using_msi))
> > + desc->control = ISMT_DESC_INT | ISMT_DESC_FAIR;
> > + else
> > + desc->control = ISMT_DESC_FAIR;
> > +
> > + if ((flags & I2C_CLIENT_PEC) && (size != I2C_SMBUS_QUICK)
> > + && (size != I2C_SMBUS_I2C_BLOCK_DATA))
> > + desc->control |= ISMT_DESC_PEC;
> > +
> > + switch (size) {
> > + case I2C_SMBUS_QUICK:
> > + dev_dbg(dev, "I2C_SMBUS_QUICK\n");
> > + break;
> > +
> > + case I2C_SMBUS_BYTE:
> > + if (read_write == I2C_SMBUS_WRITE) {
> > + /*
> > + * Send Byte
> > + * The command field contains the write data
> > + */
> > + dev_dbg(dev, "I2C_SMBUS_BYTE: WRITE\n");
> > + desc->control |= ISMT_DESC_CWRL;
> > + desc->wr_len_cmd = command;
> > + } else {
> > + /* Receive Byte */
> > + dev_dbg(dev, "I2C_SMBUS_BYTE: READ\n");
> > + dma_size = 1;
> > + dma_direction = DMA_FROM_DEVICE;
> > + desc->rd_len = 1;
> > + }
> > +
>
> This blank line could go away. There are more occurrences of this in
> this function.
>
Ok
> > +
> > + case I2C_SMBUS_BLOCK_DATA:
> > + if (read_write == I2C_SMBUS_WRITE) {
> > + /* Block Write */
> > + dev_dbg(dev, "I2C_SMBUS_BLOCK_DATA: WRITE\n");
> > + dma_size = data->block[0] + 1;
> > + dma_direction = DMA_TO_DEVICE;
> > + desc->wr_len_cmd = dma_size;
> > + desc->control |= ISMT_DESC_BLK;
> > + } else {
> > + /* Block Read */
> > + dev_dbg(dev, "I2C_SMBUS_BLOCK_DATA: READ\n");
> > + dma_size = I2C_SMBUS_BLOCK_MAX + 1;
>
> I am the one who asked for the + 1 here, now I realize it might not be
> correct. It really depends on what the hardware is returning on SMBus
> block reads.
>
> What you are supposed to return to the caller, per Linux' i2c API, is
> the block length in data->block[0] and that many bytes in
> data->block[1..n]. Question is, what do you get from the hardware in
> dma_buffer? If you get the block length followed by the data then
> "dma_size = I2C_SMBUS_BLOCK_MAX + 1" is correct and the code in
> ismt_process_desc() is almost correct as well (you only have to get the
> memcpy size right.)
>
> OTOH if the hardware gives your only the data bytes in dma_buffer, then
> the "+ 1" above is wrong, and we also face an issue with retrieving the
> block length. Where is the hardware making it available to us? I
> certainly hope it does.
>
It better, I'll speak with Intel and see if I can't get the datasheet for this
device, and make the appropriate fix here.
> > + dma_direction = DMA_FROM_DEVICE;
> > + desc->rd_len = dma_size;
> > + desc->wr_len_cmd = command;
> > + desc->control |= (ISMT_DESC_BLK | ISMT_DESC_CWRL);
> > + }
> > +
> > + break;
> > +
> > + default:
> > + dev_err(dev, "Unsupported transaction %d\n",
> > + size);
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + /* map the data buffer */
> > + if (dma_size != 0) {
> > + dev_dbg(dev, " dev=%p\n", dev);
> > + dev_dbg(dev, " data=%p\n", data);
> > + dev_dbg(dev, " dma_buffer=%p\n", priv->dma_buffer);
> > + dev_dbg(dev, " dma_size=%d\n", dma_size);
> > + dev_dbg(dev, " dma_direction=%d\n", dma_direction);
> > +
> > + dma_addr = dma_map_single(dev,
> > + priv->dma_buffer,
> > + dma_size,
> > + dma_direction);
> > +
> > + if (dma_mapping_error(dev, dma_addr)) {
> > + dev_err(dev, "Error in mapping dma buffer %p\n", priv->dma_buffer);
> > + return -EIO;
> > + }
> > +
> > + dev_dbg(dev, " dma_addr = 0x%016llX\n",
> > + dma_addr);
> > +
> > + desc->dptr_low = lower_32_bits(dma_addr);
> > + desc->dptr_high = upper_32_bits(dma_addr);
> > + }
> > +
> > + INIT_COMPLETION(priv->cmp);
> > +
> > + /* Add the descriptor */
> > + ismt_submit_desc(priv);
> > +
> > + /* Now we wait for interrupt completion, 1s */
> > + ret = wait_for_completion_timeout(&priv->cmp, HZ*1);
> > +
> > + /* unmap the data buffer */
> > + if (dma_size != 0)
> > + dma_unmap_single(&adap->dev, dma_addr, dma_size, dma_direction);
> > +
> > + if (unlikely(!ret)) {
> > + dev_err(dev, "completion wait timed out\n");
> > + ret = -ETIMEDOUT;
> > + }
> > +
> > + /* do any post processing of the descriptor here */
> > + ret = ismt_process_desc(desc, data, priv->dma_buffer, size, read_write);
>
> There's something wrong here, the "ret = -ETIMEDOUT" above is
> overwritten by this call so the error code is lost. Bill's code did not
> have the problem, not sure why you changed it.
>
I changed it because you asked for this to be modified to the uninterruptible
version of the completion api. I see what you mean though, I didn't handle the
ETIMEDOUT error case properly.
> > +
> > + /* Update the ring pointer */
> > + priv->head++;
> > + priv->head %= ISMT_DESC_ENTRIES;
> > +
> > + return ret;
> > +}
> > +
> > (...)
> > +/**
> > + * ismt_hw_init() - initialize the iSMT hardware
> > + * @priv: iSMT private data
> > + */
> > +static void ismt_hw_init(struct ismt_priv *priv)
> > +{
> > + u32 val;
> > + struct device *dev = &priv->pci_dev->dev;
> > +
> > + /* initialize the Master Descriptor Base Address (MDBA) */
> > + writel(priv->io_rng_dma, priv->smba + ISMT_MSTR_MDBA);
> > + writel(priv->io_rng_dma >> 32, priv->smba + ISMT_MSTR_MDBA + 4);
> > +
> > + /* initialize the Master Control Register (MCTRL) */
> > + writel(ISMT_MCTRL_MEIE, priv->smba + ISMT_MSTR_MCTRL);
> > +
> > + /* initialize the Master Status Register (MSTS) */
> > + writel(0, priv->smba + ISMT_MSTR_MSTS);
> > +
> > + /* initialize the Master Descriptor Size (MDS) */
> > + val = readl(priv->smba + ISMT_MSTR_MDS);
> > + writel((val & ~ISMT_MDS_MASK) | (ISMT_DESC_ENTRIES - 1),
> > + priv->smba + ISMT_MSTR_MDS);
> > +
> > + /*
> > + * Set the SMBus speed (could use this for slow HW debuggers)
> > + */
> > +
> > + val = readl(priv->smba + ISMT_SPGT);
> > +
> > + switch (bus_speed) {
> > + case 0:
> > + break;
> > +
> > + case 80:
> > + dev_dbg(dev, "Setting SMBus clock to 80 kHz\n");
>
> You did add a space here between "80" and "kHz" as I asked, but...
>
> > + writel(((val & ~ISMT_SPGT_SPD_MASK) | ISMT_SPGT_SPD_80K),
> > + priv->smba + ISMT_SPGT);
> > + break;
> > +
> > + case 100:
> > + dev_dbg(dev, "Setting SMBus clock to 100kHz\n");
>
> ... not here...
>
> > + writel(((val & ~ISMT_SPGT_SPD_MASK) | ISMT_SPGT_SPD_100K),
> > + priv->smba + ISMT_SPGT);
> > + break;
> > +
> > + case 400:
> > + dev_dbg(dev, "Setting SMBus clock to 400kHz\n");
>
> ... nor here...
>
> > + writel(((val & ~ISMT_SPGT_SPD_MASK) | ISMT_SPGT_SPD_400K),
> > + priv->smba + ISMT_SPGT);
> > + break;
> > +
> > + case 1000:
> > + dev_dbg(dev, "Setting SMBus clock to 1MHz\n");
>
> ... nor here.
>
> > + writel(((val & ~ISMT_SPGT_SPD_MASK) | ISMT_SPGT_SPD_1M),
> > + priv->smba + ISMT_SPGT);
> > + break;
> > +
> > + default:
> > + dev_dbg(dev, "Invalid SMBus clock speed, only 1-4 are valid\n");
>
> Valid values are now 80, 100, 400 and 1000. This is more than a debug
> message BTW, this should be a warning IMHO.
>
Ok
> > + break;
> > + }
> > +
> > + switch (val & ISMT_SPGT_SPD_MASK) {
> > + case ISMT_SPGT_SPD_80K:
> > + bus_speed = 80;
> > + break;
> > + case ISMT_SPGT_SPD_100K:
> > + bus_speed = 100;
> > + break;
> > + case ISMT_SPGT_SPD_400K:
> > + bus_speed = 400;
> > + break;
> > + case ISMT_SPGT_SPD_1M:
> > + bus_speed = 1000;
> > + break;
> > + }
> > + dev_dbg(dev, "SMBus clock is running at %d KHz\n", bus_speed);
>
> This is incorrect. You changed the value of register ISMT_SPGT based on
> the value of bus_speed, however you did not update "val" accordingly.
> So if the user forced a specific speed, the value you are printing is
> the speed at which the bus _was_ running before.
>
> So you must update val prior to writing the new value to the register.
>
Ah, you're right, thanks.
> Also note that the proper casing is "kHz".
>
>
Give me a day or two, and I'll have these fixed up.
Neil
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
