Hi Neil,
On Fri, 1 Feb 2013 14:48:03 -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>
>
> ---
> General note - Seth had mentioned to me that he was unable to get writes to work
> on his system. I'm not sure of the details, but I've done some rudimentary
> read/write testing here on my system, and it seems to work well (for the limited
> testing I can do). Seth if you could please provide details of your testing, or
> give this patch a try, I would appreciate it.
I am still trying to get my hands on a test machine. I am told we have
a few, however they are currently either not setup or reserved.
Until then, a few remaining comments:
> +/**
> + * 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,
> + (long unsigned int)readq(priv->smba + ISMT_GR_SMTICL));
On 32-bit systems, a long unsigned int can only hold 4 bytes, not 8, so
the top 32 bits of the value returned by readq() will be lost. I think
you have to use format 0x%016llX and cast to long long unsigned int.
> + 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,
> + (long unsigned int)readq(priv->smba + ISMT_MSTR_MDBA));
Same here.
> + 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
> + * @priv: ismt_priv struct holding our dma buffer
> + * @size: SMBus transaction type
> + * @read_write flag to indicate if this is a read or write
Missing colon after parameter name.
> + * @dma_size size of the dma transfer
This function has no parameter named dma_size.
> + */
> +static int ismt_process_desc(const struct ismt_desc *desc,
> + union i2c_smbus_data *data,
> + struct ismt_priv *priv, int size,
> + char read_write)
> +{
> + u8 *dma_buffer = priv->dma_buffer;
> +
> + dev_dbg(&priv->pci_dev->dev, "Processing completed descriptor\n");
> + __ismt_desc_dump(&priv->pci_dev->dev, desc);
> +
> + if (desc->status & ISMT_DESC_SCS) {
> + if ((size != I2C_SMBUS_QUICK) &&
> + (read_write == I2C_SMBUS_READ)) {
> + memcpy(&data->block[1], dma_buffer, desc->rxbytes);
> + data->block[0] = desc->rxbytes;
This looks good for block reads. But for receive byte, read byte and
read word transactions, this can't be correct. You have to copy the
contents of dma_buffer to data->byte or data->word for these
transactions.
> + }
> + return 0;
> + }
> +
> + if (likely(desc->status & ISMT_DESC_NAK))
> + return -ENXIO;
> +
> + if (desc->status & ISMT_DESC_CRC)
> + return -EBADMSG;
> +
> + if (desc->status & ISMT_DESC_COL)
> + return -EAGAIN;
> +
> + if (desc->status & ISMT_DESC_LPR)
> + return -EPROTO;
> +
> + if (desc->status & (ISMT_DESC_DLTO | ISMT_DESC_CLTO))
> + return -ETIMEDOUT;
> +
> + return -EIO;
> +}
> (...)
> + /* 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_BYTE)) {
> + memcpy(&priv->dma_buffer[1], &data->block[1], dma_size);
> + priv->dma_buffer[0] = command;
> + }
Again this looks good for block writes, but not for send byte, write
byte and write word transactions. data->block is only relevant for
block transactions, and &data->block[1] is off by one compared to
&data->byte and &data->word.
We did not discuss it yet but you should also pay attention to
I2C_SMBUS_PROC_CALL. It is the only bidirectional transaction, so the
value of read_write is irrelevant there. So I'm reasonably certain your
code doesn't implement it properly. Either fix it or drop support
altogether. I can't remember this transaction type ever being used by
any slave device in practice, and it can always be added back later if
really needed.
> +
> + /* 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;
> + goto out;
> + }
> +
> + /* do any post processing of the descriptor here */
> + ret = ismt_process_desc(desc, data, priv, size, read_write);
> +
> +out:
> + /* Update the ring pointer */
> + priv->head++;
> + priv->head %= ISMT_DESC_ENTRIES;
> +
> + return ret;
> +}
I have backported your code to kernel 3.0 and I'll get back to you when
I finally get a chance to test it.
--
Jean Delvare
--
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
