| Message ID | 1412726809-7525-5-git-send-email-fkan@apm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 07, 2014 at 05:06:47PM -0700, Feng Kan wrote: > Add SLIMpro I2C device driver on APM X-Gene platform. This I2C > device driver use the SLIMpro Mailbox driver to tunnel message to > the SLIMpro coprocessor to do the work of accessing I2C components. > > Signed-off-by: Feng Kan <fkan@apm.com> > Signed-off-by: Hieu Le <hnle@apm.com> Thank you for this submission! > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > index 2e45ae3..a03042c 100644 > --- a/drivers/i2c/busses/Kconfig > +++ b/drivers/i2c/busses/Kconfig > @@ -1009,6 +1009,15 @@ config I2C_CROS_EC_TUNNEL > connected there. This will work whatever the interface used to > talk to the EC (SPI, I2C or LPC). > > +config I2C_XGENE_SLIMPRO > + tristate "APM X-Gene SoC I2C SLIMpro devices support" > + depends on ARCH_XGENE && XGENE_SLIMPRO_MBOX > + help > + Enable I2C bus access using the APM X-Gene SoC SLIMpro > + co-processor. The I2C device access the I2C bus via the X-Gene > + to SLIMpro (On chip coprocessor) mailbox mechanism. > + If unsure, say N. > + I guess this is a driver for embedded? If so, please sort it properly. > config SCx200_ACB > tristate "Geode ACCESS.bus support" > depends on X86_32 && PCI > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile > index 49bf07e..22b5f0c 100644 > --- a/drivers/i2c/busses/Makefile > +++ b/drivers/i2c/busses/Makefile > @@ -99,6 +99,7 @@ obj-$(CONFIG_I2C_CROS_EC_TUNNEL) += i2c-cros-ec-tunnel.o > obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o > obj-$(CONFIG_I2C_PCA_ISA) += i2c-pca-isa.o > obj-$(CONFIG_I2C_SIBYTE) += i2c-sibyte.o > +obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o Ditto. > obj-$(CONFIG_SCx200_ACB) += scx200_acb.o > > ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG > diff --git a/drivers/i2c/busses/i2c-xgene-slimpro.c b/drivers/i2c/busses/i2c-xgene-slimpro.c > new file mode 100644 > index 0000000..2cf6c33 > --- /dev/null > +++ b/drivers/i2c/busses/i2c-xgene-slimpro.c > @@ -0,0 +1,531 @@ > +/* > + * X-Gene SLIMpro I2C Driver > + * > + * Copyright (c) 2014, Applied Micro Circuits Corporation > + * Author: Feng Kan <fkan@apm.com> > + * Author: Hieu Le <hnle@apm.com> > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation; either version 2 of > + * the License, or (at your option) any later version. The license preamble and MODULE_LICENSE do not match! > +#include <linux/module.h> > +#include <linux/version.h> > +#include <linux/platform_device.h> > +#include <linux/of.h> > +#include <linux/i2c.h> > +#include <linux/mailbox_client.h> > +#include <linux/delay.h> > +#include <linux/spinlock.h> > +#include <linux/interrupt.h> > +#include <linux/dma-mapping.h> Please sort the includes. > + > +#define XGENE_SLIMPRO_I2C "xgene-slimpro-i2c" Only used once, not really needed > + > +#define SLIMPRO_I2C_WAIT_COUNT 10000 > + > +#define SLIMPRO_OP_TO_MS 1000 /* Operation time out in ms */ *_TOUT_* or *_TIMEOUT_*. TO sounds like a conversion, from op to ms :) > +#define SLIMPRO_IIC_BUS 1 What does this '1' mean? > +static int start_i2c_msg_xfer(struct slimpro_i2c_dev *ctx) > +{ > + int count; > + unsigned long flags; > + > + if (ctx->mbox_client.tx_block) { > + if (!wait_for_completion_timeout(&ctx->rd_complete, > + msecs_to_jiffies > + (SLIMPRO_OP_TO_MS))) > + return -EIO; That should be -ETIMEDOUT, no? > + } else { > + spin_lock_irqsave(&ctx->lock, flags); > + ctx->i2c_rx_poll = 1; > + for (count = SLIMPRO_I2C_WAIT_COUNT; count > 0; count--) { > + if (ctx->i2c_rx_poll == 0) > + break; > + udelay(100); > + } > + > + if (count == 0) { > + ctx->i2c_rx_poll = 0; > + ctx->mbox_client.tx_block = true; > + spin_unlock_irqrestore(&ctx->lock, flags); > + return -EIO; ditto. > + } > + ctx->mbox_client.tx_block = true; > + spin_unlock_irqrestore(&ctx->lock, flags); > + } > + > + /* Check of invalid data or no device */ > + if (*ctx->resp_msg == 0xffffffff) > + return -ENODEV; > + > + return 0; > +} > + > +static int slimpro_i2c_blkrd(struct slimpro_i2c_dev *ctx, u32 chip, u32 addr, > + u32 addrlen, u32 protocol, u32 readlen, > + u32 with_data_len, void *data) > +{ > + dma_addr_t paddr; > + u32 msg[3]; > + int rc; > + > + paddr = dma_map_single(ctx->dev, ctx->dma_buffer, readlen, > + DMA_FROM_DEVICE); > + if (dma_mapping_error(ctx->dev, paddr)) { > + dev_err(&ctx->adapter.dev, "Error in mapping dma buffer %p\n", > + ctx->dma_buffer); > + rc = -EIO; Return the err value from dma_mapping_error? > + goto err; > + } > + > + if (irqs_disabled()) > + ctx->mbox_client.tx_block = false; > + > + msg[0] = SLIMPRO_IIC_ENCODE_MSG(SLIMPRO_IIC_BUS, chip, > + SLIMPRO_IIC_READ, protocol, addrlen, > + readlen); > + msg[1] = SLIMPRO_IIC_ENCODE_FLAG_BUFADDR | > + SLIMPRO_IIC_ENCODE_FLAG_WITH_DATA_LEN(with_data_len) | > + SLIMPRO_IIC_ENCODE_UPPER_BUFADDR(paddr) | > + SLIMPRO_IIC_ENCODE_ADDR(addr); > + msg[2] = (u32) paddr; > + ctx->resp_msg = msg; > + > + if (ctx->mbox_client.tx_block) > + init_completion(&ctx->rd_complete); reinit_completion? and init_completion in probe? > + > + rc = mbox_send_message(ctx->mbox_chan, &msg); > + if (rc < 0) > + goto err_unmap; > + > + rc = start_i2c_msg_xfer(ctx); > + > + /* Copy to destination */ > + memcpy(data, ctx->dma_buffer, readlen); > + > +err_unmap: > + dma_unmap_single(ctx->dev, paddr, readlen, DMA_FROM_DEVICE); > +err: > + ctx->resp_msg = NULL; > + return rc; > +} > + > +static int slimpro_i2c_blkwr(struct slimpro_i2c_dev *ctx, u32 chip, > + u32 addr, u32 addrlen, u32 protocol, u32 writelen, > + void *data) > +{ > + dma_addr_t paddr; > + u32 msg[3]; > + int rc; > + > + memcpy(ctx->dma_buffer, data, writelen); > + paddr = dma_map_single(ctx->dev, ctx->dma_buffer, writelen, > + DMA_TO_DEVICE); > + if (dma_mapping_error(ctx->dev, paddr)) { > + dev_err(&ctx->adapter.dev, "Error in mapping dma buffer %p\n", > + ctx->dma_buffer); > + rc = -EIO; same as above > + goto err; > + } > + > + if (irqs_disabled()) > + ctx->mbox_client.tx_block = false; > + > + msg[0] = SLIMPRO_IIC_ENCODE_MSG(SLIMPRO_IIC_BUS, chip, > + SLIMPRO_IIC_WRITE, protocol, addrlen, > + writelen); > + msg[1] = SLIMPRO_IIC_ENCODE_FLAG_BUFADDR | > + SLIMPRO_IIC_ENCODE_UPPER_BUFADDR(paddr) | > + SLIMPRO_IIC_ENCODE_ADDR(addr); > + msg[2] = (u32) paddr; > + ctx->resp_msg = msg; > + > + if (ctx->mbox_client.tx_block) > + init_completion(&ctx->rd_complete); ditto > + > + rc = mbox_send_message(ctx->mbox_chan, &msg); > + if (rc < 0) > + goto err_unmap; > + > + rc = start_i2c_msg_xfer(ctx); > + > +err_unmap: > + dma_unmap_single(ctx->dev, paddr, writelen, DMA_TO_DEVICE); > +err: > + ctx->resp_msg = NULL; > + return rc; > +} > + > +static struct platform_driver xgene_slimpro_i2c_driver = { > + .probe = xgene_slimpro_i2c_probe, > + .remove = xgene_slimpro_i2c_remove, > + .driver = { > + .name = XGENE_SLIMPRO_I2C, > + .owner = THIS_MODULE, Not needed. > + .of_match_table = of_match_ptr(xgene_slimpro_i2c_id) > + }, > +}; > + > +module_platform_driver(xgene_slimpro_i2c_driver); > + > +MODULE_DESCRIPTION("APM X-Gene SLIMpro I2C driver"); > +MODULE_LICENSE("GPL v2"); So, only minor stuff. Looking good already to me. Thanks, Wolfram
On Tuesday 07 October 2014 17:06:47 Feng Kan wrote: > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > index 2e45ae3..a03042c 100644 > --- a/drivers/i2c/busses/Kconfig > +++ b/drivers/i2c/busses/Kconfig > @@ -1009,6 +1009,15 @@ config I2C_CROS_EC_TUNNEL > connected there. This will work whatever the interface used to > talk to the EC (SPI, I2C or LPC). > > +config I2C_XGENE_SLIMPRO > + tristate "APM X-Gene SoC I2C SLIMpro devices support" > + depends on ARCH_XGENE && XGENE_SLIMPRO_MBOX Why this dependency on XGENE_SLIMPRO_MBOX? Better replace it with a dependency on MAILBOX. > + } else { > + spin_lock_irqsave(&ctx->lock, flags); > + ctx->i2c_rx_poll = 1; > + for (count = SLIMPRO_I2C_WAIT_COUNT; count > 0; count--) { > + if (ctx->i2c_rx_poll == 0) > + break; > + udelay(100); > + } No, you can't block the CPU for an extended amount of time with interrupts disabled. Please kill this code. > + ctx->resp_msg = data; > + if (ctx->mbox_client.tx_block) > + init_completion(&ctx->rd_complete); reinit_completion()? > +static int slimpro_i2c_blkrd(struct slimpro_i2c_dev *ctx, u32 chip, u32 addr, > + u32 addrlen, u32 protocol, u32 readlen, > + u32 with_data_len, void *data) > +{ > + dma_addr_t paddr; > + u32 msg[3]; > + int rc; > + > + paddr = dma_map_single(ctx->dev, ctx->dma_buffer, readlen, > + DMA_FROM_DEVICE); ctx->dev is probably the wrong device here. The i2c controller is not DMA capable itself, you need to have a pointer to the device that actually performs the DMA here. > + /* Request mailbox channel */ > + cl->dev = &pdev->dev; > + cl->rx_callback = slimpro_i2c_rx_cb; > + cl->tx_done = slimpro_i2c_tx_done; > + cl->tx_block = true; > + cl->tx_tout = SLIMPRO_OP_TO_MS; > + cl->knows_txdone = false; > + cl->chan_name = "i2c-slimpro"; > + ctx->mbox_chan = mbox_request_channel(cl); This is not the correct interface. > + rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); > + if (rc) > + dev_warn(&pdev->dev, "Unable to set dma mask\n"); Are you sure that this is the correct device to perform the DMA? Moreover, the mask doesn't match the usage: the slimpro_i2c_blkrd function passes in only the lower 32 bit of the address, which would be DMA_BIT_MASK(32). > +#ifdef CONFIG_OF > +static struct of_device_id xgene_slimpro_i2c_id[] = { > + {.compatible = "apm,xgene-slimpro-i2c" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, xgene_slimpro_i2c_dt_ids); > +#endif > + > +static struct platform_driver xgene_slimpro_i2c_driver = { > + .probe = xgene_slimpro_i2c_probe, > + .remove = xgene_slimpro_i2c_remove, > + .driver = { > + .name = XGENE_SLIMPRO_I2C, > + .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(xgene_slimpro_i2c_id) > + }, > +}; The driver only supports DT, so just drop the #ifdef and the of_match_ptr().
On Tue, Nov 11, 2014 at 12:32 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > On Tue, Oct 07, 2014 at 05:06:47PM -0700, Feng Kan wrote: >> Add SLIMpro I2C device driver on APM X-Gene platform. This I2C >> device driver use the SLIMpro Mailbox driver to tunnel message to >> the SLIMpro coprocessor to do the work of accessing I2C components. >> >> Signed-off-by: Feng Kan <fkan@apm.com> >> Signed-off-by: Hieu Le <hnle@apm.com> > > Thank you for this submission! > >> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig >> index 2e45ae3..a03042c 100644 >> --- a/drivers/i2c/busses/Kconfig >> +++ b/drivers/i2c/busses/Kconfig >> @@ -1009,6 +1009,15 @@ config I2C_CROS_EC_TUNNEL >> connected there. This will work whatever the interface used to >> talk to the EC (SPI, I2C or LPC). >> >> +config I2C_XGENE_SLIMPRO >> + tristate "APM X-Gene SoC I2C SLIMpro devices support" >> + depends on ARCH_XGENE && XGENE_SLIMPRO_MBOX >> + help >> + Enable I2C bus access using the APM X-Gene SoC SLIMpro >> + co-processor. The I2C device access the I2C bus via the X-Gene >> + to SLIMpro (On chip coprocessor) mailbox mechanism. >> + If unsure, say N. >> + > > I guess this is a driver for embedded? If so, please sort it properly. This is a SoC chip but not for embedded platform. Also the fact that this is not a traditional I2C driver, but one that tunnels to the helper cpu using mailbox to obtain I2C information. Do you still want me to group it with the embedded drivers? > >> config SCx200_ACB >> tristate "Geode ACCESS.bus support" >> depends on X86_32 && PCI >> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile >> index 49bf07e..22b5f0c 100644 >> --- a/drivers/i2c/busses/Makefile >> +++ b/drivers/i2c/busses/Makefile >> @@ -99,6 +99,7 @@ obj-$(CONFIG_I2C_CROS_EC_TUNNEL) += i2c-cros-ec-tunnel.o >> obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o >> obj-$(CONFIG_I2C_PCA_ISA) += i2c-pca-isa.o >> obj-$(CONFIG_I2C_SIBYTE) += i2c-sibyte.o >> +obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o > > Ditto. > >> obj-$(CONFIG_SCx200_ACB) += scx200_acb.o >> >> ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG >> diff --git a/drivers/i2c/busses/i2c-xgene-slimpro.c b/drivers/i2c/busses/i2c-xgene-slimpro.c >> new file mode 100644 >> index 0000000..2cf6c33 >> --- /dev/null >> +++ b/drivers/i2c/busses/i2c-xgene-slimpro.c >> @@ -0,0 +1,531 @@ >> +/* >> + * X-Gene SLIMpro I2C Driver >> + * >> + * Copyright (c) 2014, Applied Micro Circuits Corporation >> + * Author: Feng Kan <fkan@apm.com> >> + * Author: Hieu Le <hnle@apm.com> >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License as >> + * published by the Free Software Foundation; either version 2 of >> + * the License, or (at your option) any later version. > > The license preamble and MODULE_LICENSE do not match! > >> +#include <linux/module.h> >> +#include <linux/version.h> >> +#include <linux/platform_device.h> >> +#include <linux/of.h> >> +#include <linux/i2c.h> >> +#include <linux/mailbox_client.h> >> +#include <linux/delay.h> >> +#include <linux/spinlock.h> >> +#include <linux/interrupt.h> >> +#include <linux/dma-mapping.h> > > Please sort the includes. > >> + >> +#define XGENE_SLIMPRO_I2C "xgene-slimpro-i2c" > > Only used once, not really needed > >> + >> +#define SLIMPRO_I2C_WAIT_COUNT 10000 >> + >> +#define SLIMPRO_OP_TO_MS 1000 /* Operation time out in ms */ > > *_TOUT_* or *_TIMEOUT_*. TO sounds like a conversion, from op to ms :) > >> +#define SLIMPRO_IIC_BUS 1 > > What does this '1' mean? > >> +static int start_i2c_msg_xfer(struct slimpro_i2c_dev *ctx) >> +{ >> + int count; >> + unsigned long flags; >> + >> + if (ctx->mbox_client.tx_block) { >> + if (!wait_for_completion_timeout(&ctx->rd_complete, >> + msecs_to_jiffies >> + (SLIMPRO_OP_TO_MS))) >> + return -EIO; > > That should be -ETIMEDOUT, no? > >> + } else { >> + spin_lock_irqsave(&ctx->lock, flags); >> + ctx->i2c_rx_poll = 1; >> + for (count = SLIMPRO_I2C_WAIT_COUNT; count > 0; count--) { >> + if (ctx->i2c_rx_poll == 0) >> + break; >> + udelay(100); >> + } >> + >> + if (count == 0) { >> + ctx->i2c_rx_poll = 0; >> + ctx->mbox_client.tx_block = true; >> + spin_unlock_irqrestore(&ctx->lock, flags); >> + return -EIO; > > ditto. > >> + } >> + ctx->mbox_client.tx_block = true; >> + spin_unlock_irqrestore(&ctx->lock, flags); >> + } >> + >> + /* Check of invalid data or no device */ >> + if (*ctx->resp_msg == 0xffffffff) >> + return -ENODEV; >> + >> + return 0; >> +} >> + >> +static int slimpro_i2c_blkrd(struct slimpro_i2c_dev *ctx, u32 chip, u32 addr, >> + u32 addrlen, u32 protocol, u32 readlen, >> + u32 with_data_len, void *data) >> +{ >> + dma_addr_t paddr; >> + u32 msg[3]; >> + int rc; >> + >> + paddr = dma_map_single(ctx->dev, ctx->dma_buffer, readlen, >> + DMA_FROM_DEVICE); >> + if (dma_mapping_error(ctx->dev, paddr)) { >> + dev_err(&ctx->adapter.dev, "Error in mapping dma buffer %p\n", >> + ctx->dma_buffer); >> + rc = -EIO; > > Return the err value from dma_mapping_error? > >> + goto err; >> + } >> + >> + if (irqs_disabled()) >> + ctx->mbox_client.tx_block = false; >> + >> + msg[0] = SLIMPRO_IIC_ENCODE_MSG(SLIMPRO_IIC_BUS, chip, >> + SLIMPRO_IIC_READ, protocol, addrlen, >> + readlen); >> + msg[1] = SLIMPRO_IIC_ENCODE_FLAG_BUFADDR | >> + SLIMPRO_IIC_ENCODE_FLAG_WITH_DATA_LEN(with_data_len) | >> + SLIMPRO_IIC_ENCODE_UPPER_BUFADDR(paddr) | >> + SLIMPRO_IIC_ENCODE_ADDR(addr); >> + msg[2] = (u32) paddr; >> + ctx->resp_msg = msg; >> + >> + if (ctx->mbox_client.tx_block) >> + init_completion(&ctx->rd_complete); > > reinit_completion? and init_completion in probe? > >> + >> + rc = mbox_send_message(ctx->mbox_chan, &msg); >> + if (rc < 0) >> + goto err_unmap; >> + >> + rc = start_i2c_msg_xfer(ctx); >> + >> + /* Copy to destination */ >> + memcpy(data, ctx->dma_buffer, readlen); >> + >> +err_unmap: >> + dma_unmap_single(ctx->dev, paddr, readlen, DMA_FROM_DEVICE); >> +err: >> + ctx->resp_msg = NULL; >> + return rc; >> +} >> + >> +static int slimpro_i2c_blkwr(struct slimpro_i2c_dev *ctx, u32 chip, >> + u32 addr, u32 addrlen, u32 protocol, u32 writelen, >> + void *data) >> +{ >> + dma_addr_t paddr; >> + u32 msg[3]; >> + int rc; >> + >> + memcpy(ctx->dma_buffer, data, writelen); >> + paddr = dma_map_single(ctx->dev, ctx->dma_buffer, writelen, >> + DMA_TO_DEVICE); >> + if (dma_mapping_error(ctx->dev, paddr)) { >> + dev_err(&ctx->adapter.dev, "Error in mapping dma buffer %p\n", >> + ctx->dma_buffer); >> + rc = -EIO; > > same as above > >> + goto err; >> + } >> + >> + if (irqs_disabled()) >> + ctx->mbox_client.tx_block = false; >> + >> + msg[0] = SLIMPRO_IIC_ENCODE_MSG(SLIMPRO_IIC_BUS, chip, >> + SLIMPRO_IIC_WRITE, protocol, addrlen, >> + writelen); >> + msg[1] = SLIMPRO_IIC_ENCODE_FLAG_BUFADDR | >> + SLIMPRO_IIC_ENCODE_UPPER_BUFADDR(paddr) | >> + SLIMPRO_IIC_ENCODE_ADDR(addr); >> + msg[2] = (u32) paddr; >> + ctx->resp_msg = msg; >> + >> + if (ctx->mbox_client.tx_block) >> + init_completion(&ctx->rd_complete); > > ditto > >> + >> + rc = mbox_send_message(ctx->mbox_chan, &msg); >> + if (rc < 0) >> + goto err_unmap; >> + >> + rc = start_i2c_msg_xfer(ctx); >> + >> +err_unmap: >> + dma_unmap_single(ctx->dev, paddr, writelen, DMA_TO_DEVICE); >> +err: >> + ctx->resp_msg = NULL; >> + return rc; >> +} >> + >> +static struct platform_driver xgene_slimpro_i2c_driver = { >> + .probe = xgene_slimpro_i2c_probe, >> + .remove = xgene_slimpro_i2c_remove, >> + .driver = { >> + .name = XGENE_SLIMPRO_I2C, >> + .owner = THIS_MODULE, > > Not needed. > >> + .of_match_table = of_match_ptr(xgene_slimpro_i2c_id) >> + }, >> +}; >> + >> +module_platform_driver(xgene_slimpro_i2c_driver); >> + >> +MODULE_DESCRIPTION("APM X-Gene SLIMpro I2C driver"); >> +MODULE_LICENSE("GPL v2"); > > So, only minor stuff. Looking good already to me. > > Thanks, > > Wolfram >
On Tue, Nov 11, 2014 at 12:32 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > On Tue, Oct 07, 2014 at 05:06:47PM -0700, Feng Kan wrote: >> Add SLIMpro I2C device driver on APM X-Gene platform. This I2C >> device driver use the SLIMpro Mailbox driver to tunnel message to >> the SLIMpro coprocessor to do the work of accessing I2C components. >> >> Signed-off-by: Feng Kan <fkan@apm.com> >> Signed-off-by: Hieu Le <hnle@apm.com> > > Thank you for this submission! > >> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig >> index 2e45ae3..a03042c 100644 >> --- a/drivers/i2c/busses/Kconfig >> +++ b/drivers/i2c/busses/Kconfig >> @@ -1009,6 +1009,15 @@ config I2C_CROS_EC_TUNNEL >> connected there. This will work whatever the interface used to >> talk to the EC (SPI, I2C or LPC). >> >> +config I2C_XGENE_SLIMPRO >> + tristate "APM X-Gene SoC I2C SLIMpro devices support" >> + depends on ARCH_XGENE && XGENE_SLIMPRO_MBOX >> + help >> + Enable I2C bus access using the APM X-Gene SoC SLIMpro >> + co-processor. The I2C device access the I2C bus via the X-Gene >> + to SLIMpro (On chip coprocessor) mailbox mechanism. >> + If unsure, say N. >> + > > I guess this is a driver for embedded? If so, please sort it properly. Sorry, having let this dangle for a while. I will fix up comments and submit another version. I can move this to embedded, however Xgene is suppose to be a server class chip. > >> config SCx200_ACB >> tristate "Geode ACCESS.bus support" >> depends on X86_32 && PCI >> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile >> index 49bf07e..22b5f0c 100644 >> --- a/drivers/i2c/busses/Makefile >> +++ b/drivers/i2c/busses/Makefile >> @@ -99,6 +99,7 @@ obj-$(CONFIG_I2C_CROS_EC_TUNNEL) += i2c-cros-ec-tunnel.o >> obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o >> obj-$(CONFIG_I2C_PCA_ISA) += i2c-pca-isa.o >> obj-$(CONFIG_I2C_SIBYTE) += i2c-sibyte.o >> +obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o > > Ditto. > >> obj-$(CONFIG_SCx200_ACB) += scx200_acb.o >> >> ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG >> diff --git a/drivers/i2c/busses/i2c-xgene-slimpro.c b/drivers/i2c/busses/i2c-xgene-slimpro.c >> new file mode 100644 >> index 0000000..2cf6c33 >> --- /dev/null >> +++ b/drivers/i2c/busses/i2c-xgene-slimpro.c >> @@ -0,0 +1,531 @@ >> +/* >> + * X-Gene SLIMpro I2C Driver >> + * >> + * Copyright (c) 2014, Applied Micro Circuits Corporation >> + * Author: Feng Kan <fkan@apm.com> >> + * Author: Hieu Le <hnle@apm.com> >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License as >> + * published by the Free Software Foundation; either version 2 of >> + * the License, or (at your option) any later version. > > The license preamble and MODULE_LICENSE do not match! > >> +#include <linux/module.h> >> +#include <linux/version.h> >> +#include <linux/platform_device.h> >> +#include <linux/of.h> >> +#include <linux/i2c.h> >> +#include <linux/mailbox_client.h> >> +#include <linux/delay.h> >> +#include <linux/spinlock.h> >> +#include <linux/interrupt.h> >> +#include <linux/dma-mapping.h> > > Please sort the includes. > >> + >> +#define XGENE_SLIMPRO_I2C "xgene-slimpro-i2c" > > Only used once, not really needed > >> + >> +#define SLIMPRO_I2C_WAIT_COUNT 10000 >> + >> +#define SLIMPRO_OP_TO_MS 1000 /* Operation time out in ms */ > > *_TOUT_* or *_TIMEOUT_*. TO sounds like a conversion, from op to ms :) > >> +#define SLIMPRO_IIC_BUS 1 > > What does this '1' mean? > >> +static int start_i2c_msg_xfer(struct slimpro_i2c_dev *ctx) >> +{ >> + int count; >> + unsigned long flags; >> + >> + if (ctx->mbox_client.tx_block) { >> + if (!wait_for_completion_timeout(&ctx->rd_complete, >> + msecs_to_jiffies >> + (SLIMPRO_OP_TO_MS))) >> + return -EIO; > > That should be -ETIMEDOUT, no? > >> + } else { >> + spin_lock_irqsave(&ctx->lock, flags); >> + ctx->i2c_rx_poll = 1; >> + for (count = SLIMPRO_I2C_WAIT_COUNT; count > 0; count--) { >> + if (ctx->i2c_rx_poll == 0) >> + break; >> + udelay(100); >> + } >> + >> + if (count == 0) { >> + ctx->i2c_rx_poll = 0; >> + ctx->mbox_client.tx_block = true; >> + spin_unlock_irqrestore(&ctx->lock, flags); >> + return -EIO; > > ditto. > >> + } >> + ctx->mbox_client.tx_block = true; >> + spin_unlock_irqrestore(&ctx->lock, flags); >> + } >> + >> + /* Check of invalid data or no device */ >> + if (*ctx->resp_msg == 0xffffffff) >> + return -ENODEV; >> + >> + return 0; >> +} >> + >> +static int slimpro_i2c_blkrd(struct slimpro_i2c_dev *ctx, u32 chip, u32 addr, >> + u32 addrlen, u32 protocol, u32 readlen, >> + u32 with_data_len, void *data) >> +{ >> + dma_addr_t paddr; >> + u32 msg[3]; >> + int rc; >> + >> + paddr = dma_map_single(ctx->dev, ctx->dma_buffer, readlen, >> + DMA_FROM_DEVICE); >> + if (dma_mapping_error(ctx->dev, paddr)) { >> + dev_err(&ctx->adapter.dev, "Error in mapping dma buffer %p\n", >> + ctx->dma_buffer); >> + rc = -EIO; > > Return the err value from dma_mapping_error? > >> + goto err; >> + } >> + >> + if (irqs_disabled()) >> + ctx->mbox_client.tx_block = false; >> + >> + msg[0] = SLIMPRO_IIC_ENCODE_MSG(SLIMPRO_IIC_BUS, chip, >> + SLIMPRO_IIC_READ, protocol, addrlen, >> + readlen); >> + msg[1] = SLIMPRO_IIC_ENCODE_FLAG_BUFADDR | >> + SLIMPRO_IIC_ENCODE_FLAG_WITH_DATA_LEN(with_data_len) | >> + SLIMPRO_IIC_ENCODE_UPPER_BUFADDR(paddr) | >> + SLIMPRO_IIC_ENCODE_ADDR(addr); >> + msg[2] = (u32) paddr; >> + ctx->resp_msg = msg; >> + >> + if (ctx->mbox_client.tx_block) >> + init_completion(&ctx->rd_complete); > > reinit_completion? and init_completion in probe? > >> + >> + rc = mbox_send_message(ctx->mbox_chan, &msg); >> + if (rc < 0) >> + goto err_unmap; >> + >> + rc = start_i2c_msg_xfer(ctx); >> + >> + /* Copy to destination */ >> + memcpy(data, ctx->dma_buffer, readlen); >> + >> +err_unmap: >> + dma_unmap_single(ctx->dev, paddr, readlen, DMA_FROM_DEVICE); >> +err: >> + ctx->resp_msg = NULL; >> + return rc; >> +} >> + >> +static int slimpro_i2c_blkwr(struct slimpro_i2c_dev *ctx, u32 chip, >> + u32 addr, u32 addrlen, u32 protocol, u32 writelen, >> + void *data) >> +{ >> + dma_addr_t paddr; >> + u32 msg[3]; >> + int rc; >> + >> + memcpy(ctx->dma_buffer, data, writelen); >> + paddr = dma_map_single(ctx->dev, ctx->dma_buffer, writelen, >> + DMA_TO_DEVICE); >> + if (dma_mapping_error(ctx->dev, paddr)) { >> + dev_err(&ctx->adapter.dev, "Error in mapping dma buffer %p\n", >> + ctx->dma_buffer); >> + rc = -EIO; > > same as above > >> + goto err; >> + } >> + >> + if (irqs_disabled()) >> + ctx->mbox_client.tx_block = false; >> + >> + msg[0] = SLIMPRO_IIC_ENCODE_MSG(SLIMPRO_IIC_BUS, chip, >> + SLIMPRO_IIC_WRITE, protocol, addrlen, >> + writelen); >> + msg[1] = SLIMPRO_IIC_ENCODE_FLAG_BUFADDR | >> + SLIMPRO_IIC_ENCODE_UPPER_BUFADDR(paddr) | >> + SLIMPRO_IIC_ENCODE_ADDR(addr); >> + msg[2] = (u32) paddr; >> + ctx->resp_msg = msg; >> + >> + if (ctx->mbox_client.tx_block) >> + init_completion(&ctx->rd_complete); > > ditto > >> + >> + rc = mbox_send_message(ctx->mbox_chan, &msg); >> + if (rc < 0) >> + goto err_unmap; >> + >> + rc = start_i2c_msg_xfer(ctx); >> + >> +err_unmap: >> + dma_unmap_single(ctx->dev, paddr, writelen, DMA_TO_DEVICE); >> +err: >> + ctx->resp_msg = NULL; >> + return rc; >> +} >> + >> +static struct platform_driver xgene_slimpro_i2c_driver = { >> + .probe = xgene_slimpro_i2c_probe, >> + .remove = xgene_slimpro_i2c_remove, >> + .driver = { >> + .name = XGENE_SLIMPRO_I2C, >> + .owner = THIS_MODULE, > > Not needed. > >> + .of_match_table = of_match_ptr(xgene_slimpro_i2c_id) >> + }, >> +}; >> + >> +module_platform_driver(xgene_slimpro_i2c_driver); >> + >> +MODULE_DESCRIPTION("APM X-Gene SLIMpro I2C driver"); >> +MODULE_LICENSE("GPL v2"); > > So, only minor stuff. Looking good already to me. Will fix the rest of the comments, thanks > > Thanks, > > Wolfram >
On Tue, Nov 11, 2014 at 1:51 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 07 October 2014 17:06:47 Feng Kan wrote: >> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig >> index 2e45ae3..a03042c 100644 >> --- a/drivers/i2c/busses/Kconfig >> +++ b/drivers/i2c/busses/Kconfig >> @@ -1009,6 +1009,15 @@ config I2C_CROS_EC_TUNNEL >> connected there. This will work whatever the interface used to >> talk to the EC (SPI, I2C or LPC). >> >> +config I2C_XGENE_SLIMPRO >> + tristate "APM X-Gene SoC I2C SLIMpro devices support" >> + depends on ARCH_XGENE && XGENE_SLIMPRO_MBOX > > Why this dependency on XGENE_SLIMPRO_MBOX? > > Better replace it with a dependency on MAILBOX. Arnd, just a question. Is this because this possibly help with future compatibility by choosing a more broad dependency? > >> + } else { >> + spin_lock_irqsave(&ctx->lock, flags); >> + ctx->i2c_rx_poll = 1; >> + for (count = SLIMPRO_I2C_WAIT_COUNT; count > 0; count--) { >> + if (ctx->i2c_rx_poll == 0) >> + break; >> + udelay(100); >> + } > > No, you can't block the CPU for an extended amount of time with > interrupts disabled. Please kill this code. > >> + ctx->resp_msg = data; >> + if (ctx->mbox_client.tx_block) >> + init_completion(&ctx->rd_complete); > > reinit_completion()? > >> +static int slimpro_i2c_blkrd(struct slimpro_i2c_dev *ctx, u32 chip, u32 addr, >> + u32 addrlen, u32 protocol, u32 readlen, >> + u32 with_data_len, void *data) >> +{ >> + dma_addr_t paddr; >> + u32 msg[3]; >> + int rc; >> + >> + paddr = dma_map_single(ctx->dev, ctx->dma_buffer, readlen, >> + DMA_FROM_DEVICE); > > ctx->dev is probably the wrong device here. The i2c controller is not > DMA capable itself, you need to have a pointer to the device that actually > performs the DMA here. > > >> + /* Request mailbox channel */ >> + cl->dev = &pdev->dev; >> + cl->rx_callback = slimpro_i2c_rx_cb; >> + cl->tx_done = slimpro_i2c_tx_done; >> + cl->tx_block = true; >> + cl->tx_tout = SLIMPRO_OP_TO_MS; >> + cl->knows_txdone = false; >> + cl->chan_name = "i2c-slimpro"; >> + ctx->mbox_chan = mbox_request_channel(cl); > > This is not the correct interface. > >> + rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); >> + if (rc) >> + dev_warn(&pdev->dev, "Unable to set dma mask\n"); > > Are you sure that this is the correct device to perform the DMA? > > Moreover, the mask doesn't match the usage: the slimpro_i2c_blkrd > function passes in only the lower 32 bit of the address, which would > be DMA_BIT_MASK(32). > >> +#ifdef CONFIG_OF >> +static struct of_device_id xgene_slimpro_i2c_id[] = { >> + {.compatible = "apm,xgene-slimpro-i2c" }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, xgene_slimpro_i2c_dt_ids); >> +#endif >> + >> +static struct platform_driver xgene_slimpro_i2c_driver = { >> + .probe = xgene_slimpro_i2c_probe, >> + .remove = xgene_slimpro_i2c_remove, >> + .driver = { >> + .name = XGENE_SLIMPRO_I2C, >> + .owner = THIS_MODULE, >> + .of_match_table = of_match_ptr(xgene_slimpro_i2c_id) >> + }, >> +}; > > The driver only supports DT, so just drop the #ifdef and the of_match_ptr(). > > -- > To unsubscribe from this list: send the line "unsubscribe linux-i2c" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 09 January 2015 10:56:51 Feng Kan wrote: > On Tue, Nov 11, 2014 at 1:51 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tuesday 07 October 2014 17:06:47 Feng Kan wrote: > >> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > >> index 2e45ae3..a03042c 100644 > >> --- a/drivers/i2c/busses/Kconfig > >> +++ b/drivers/i2c/busses/Kconfig > >> @@ -1009,6 +1009,15 @@ config I2C_CROS_EC_TUNNEL > >> connected there. This will work whatever the interface used to > >> talk to the EC (SPI, I2C or LPC). > >> > >> +config I2C_XGENE_SLIMPRO > >> + tristate "APM X-Gene SoC I2C SLIMpro devices support" > >> + depends on ARCH_XGENE && XGENE_SLIMPRO_MBOX > > > > Why this dependency on XGENE_SLIMPRO_MBOX? > > > > Better replace it with a dependency on MAILBOX. > Arnd, just a question. Is this because this possibly help with future > compatibility by choosing a more broad dependency? The dependency should ideally describe build-time dependencies, to make it possible to build on other architectures for static code analysis purposes. If the driver makes no sense on other platforms you can also use depends on ARCH_XGENE || COMPILE_TEST depends on MAILBOX to cover both the build-time and run-time dependencies. But the dependency on XGENE_SLIMPRO_MBOX just shouldn't be there, the driver will work with any mailbox implementation if someone puts the same hardware into a different SoC. Arnd
> > I guess this is a driver for embedded? If so, please sort it properly. > Sorry, having let this dangle for a while. I will fix up comments and submit > another version. I can move this to embedded, however Xgene is > suppose to be a server class chip. Then simply leave it as is :)
On Tue, Nov 11, 2014 at 1:51 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 07 October 2014 17:06:47 Feng Kan wrote: >> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig >> index 2e45ae3..a03042c 100644 >> --- a/drivers/i2c/busses/Kconfig >> +++ b/drivers/i2c/busses/Kconfig >> @@ -1009,6 +1009,15 @@ config I2C_CROS_EC_TUNNEL >> connected there. This will work whatever the interface used to >> talk to the EC (SPI, I2C or LPC). >> >> +config I2C_XGENE_SLIMPRO >> + tristate "APM X-Gene SoC I2C SLIMpro devices support" >> + depends on ARCH_XGENE && XGENE_SLIMPRO_MBOX > > Why this dependency on XGENE_SLIMPRO_MBOX? >> +static int slimpro_i2c_blkrd(struct slimpro_i2c_dev *ctx, u32 chip, u32 addr, >> + u32 addrlen, u32 protocol, u32 readlen, >> + u32 with_data_len, void *data) >> +{ >> + dma_addr_t paddr; >> + u32 msg[3]; >> + int rc; >> + >> + paddr = dma_map_single(ctx->dev, ctx->dma_buffer, readlen, >> + DMA_FROM_DEVICE); > > ctx->dev is probably the wrong device here. The i2c controller is not > DMA capable itself, you need to have a pointer to the device that actually > performs the DMA here. Arnd, I do agree this may not be the best identification. There is no representation for this on the Linux side. I could put it as NULL. However, there are other i2c bus drivers that seem to do the same thing. Please let me know what you think. > > >> + /* Request mailbox channel */ >> + cl->dev = &pdev->dev; >> + cl->rx_callback = slimpro_i2c_rx_cb; >> + cl->tx_done = slimpro_i2c_tx_done; >> + cl->tx_block = true; >> + cl->tx_tout = SLIMPRO_OP_TO_MS; >> + cl->knows_txdone = false; >> + cl->chan_name = "i2c-slimpro"; >> + ctx->mbox_chan = mbox_request_channel(cl); > > This is not the correct interface. > >> + rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); >> + if (rc) >> + dev_warn(&pdev->dev, "Unable to set dma mask\n"); > > Are you sure that this is the correct device to perform the DMA? > > Moreover, the mask doesn't match the usage: the slimpro_i2c_blkrd > function passes in only the lower 32 bit of the address, which would > be DMA_BIT_MASK(32). The upper part of the address is passed in as part of the msg[1]
> > ctx->dev is probably the wrong device here. The i2c controller is not > > DMA capable itself, you need to have a pointer to the device that actually > > performs the DMA here. > > Arnd, I do agree this may not be the best identification. There is no > representation for this on the Linux side. I could put it as NULL. However, > there are other i2c bus drivers that seem to do the same thing. Please > let me know what you think. For completeness: Those drivers need fixing!
On Thu, Jan 29, 2015 at 10:11 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > >> > ctx->dev is probably the wrong device here. The i2c controller is not >> > DMA capable itself, you need to have a pointer to the device that actually >> > performs the DMA here. >> >> Arnd, I do agree this may not be the best identification. There is no >> representation for this on the Linux side. I could put it as NULL. However, >> there are other i2c bus drivers that seem to do the same thing. Please >> let me know what you think. > > For completeness: Those drivers need fixing! I have some doubts. I could hack up a dts node for the helper processor that is doing the dma. However, the dma master could be anything for that matter. I am not sure how I can write this in a generic manner.
On Mon, Feb 02, 2015 at 02:15:44PM -0800, Feng Kan wrote: > On Thu, Jan 29, 2015 at 10:11 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > > > >> > ctx->dev is probably the wrong device here. The i2c controller is not > >> > DMA capable itself, you need to have a pointer to the device that actually > >> > performs the DMA here. > >> > >> Arnd, I do agree this may not be the best identification. There is no > >> representation for this on the Linux side. I could put it as NULL. However, > >> there are other i2c bus drivers that seem to do the same thing. Please > >> let me know what you think. > > > > For completeness: Those drivers need fixing! > I have some doubts. I could hack up a dts node for the helper > processor that is doing the dma. However, the dma master could be > anything for that matter. I am not sure how I can write this in a > generic manner. All I wanted to say is that, to the best of my knowledge, currently all DMA capable i2c bus drivers which do not pass the DMA capable device when mapping, need a fix like 8cfcae9f0595. So, the argument "other drivers do it, too" is invalid IMO. You might have other, better, more technical arguments for your case. Like above, that could be fine, I'd say. But I leave this to the DMA experts...
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 2e45ae3..a03042c 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -1009,6 +1009,15 @@ config I2C_CROS_EC_TUNNEL connected there. This will work whatever the interface used to talk to the EC (SPI, I2C or LPC). +config I2C_XGENE_SLIMPRO + tristate "APM X-Gene SoC I2C SLIMpro devices support" + depends on ARCH_XGENE && XGENE_SLIMPRO_MBOX + help + Enable I2C bus access using the APM X-Gene SoC SLIMpro + co-processor. The I2C device access the I2C bus via the X-Gene + to SLIMpro (On chip coprocessor) mailbox mechanism. + If unsure, say N. + config SCx200_ACB tristate "Geode ACCESS.bus support" depends on X86_32 && PCI diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 49bf07e..22b5f0c 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -99,6 +99,7 @@ obj-$(CONFIG_I2C_CROS_EC_TUNNEL) += i2c-cros-ec-tunnel.o obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o obj-$(CONFIG_I2C_PCA_ISA) += i2c-pca-isa.o obj-$(CONFIG_I2C_SIBYTE) += i2c-sibyte.o +obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o obj-$(CONFIG_SCx200_ACB) += scx200_acb.o ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG diff --git a/drivers/i2c/busses/i2c-xgene-slimpro.c b/drivers/i2c/busses/i2c-xgene-slimpro.c new file mode 100644 index 0000000..2cf6c33 --- /dev/null +++ b/drivers/i2c/busses/i2c-xgene-slimpro.c @@ -0,0 +1,531 @@ +/* + * X-Gene SLIMpro I2C Driver + * + * Copyright (c) 2014, Applied Micro Circuits Corporation + * Author: Feng Kan <fkan@apm.com> + * Author: Hieu Le <hnle@apm.com> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + * + * This driver provides support for X-Gene SLIMpro I2C device access + * using the APM X-Gene SLIMpro mailbox driver. + * + */ +#include <linux/module.h> +#include <linux/version.h> +#include <linux/platform_device.h> +#include <linux/of.h> +#include <linux/i2c.h> +#include <linux/mailbox_client.h> +#include <linux/delay.h> +#include <linux/spinlock.h> +#include <linux/interrupt.h> +#include <linux/dma-mapping.h> + +#define XGENE_SLIMPRO_I2C "xgene-slimpro-i2c" + +#define SLIMPRO_I2C_WAIT_COUNT 10000 + +#define SLIMPRO_OP_TO_MS 1000 /* Operation time out in ms */ +#define SLIMPRO_IIC_BUS 1 + +#define SMBUS_CMD_LEN 1 +#define BYTE_DATA 1 +#define WORD_DATA 2 +#define BLOCK_DATA 3 + +#define SLIMPRO_IIC_I2C_PROTOCOL 0 +#define SLIMPRO_IIC_SMB_PROTOCOL 1 + +#define SLIMPRO_IIC_READ 0 +#define SLIMPRO_IIC_WRITE 1 + +#define IIC_SMB_WITHOUT_DATA_LEN 0 +#define IIC_SMB_WITH_DATA_LEN 1 + +#define SLIMPRO_DEBUG_MSG 0 +#define SLIMPRO_MSG_TYPE_SHIFT 28 +#define SLIMPRO_DBG_SUBTYPE_I2C1READ 4 +#define SLIMPRO_DBGMSG_TYPE_SHIFT 24 +#define SLIMPRO_DBGMSG_TYPE_MASK 0x0F000000U +#define SLIMPRO_IIC_DEV_SHIFT 23 +#define SLIMPRO_IIC_DEV_MASK 0x00800000U +#define SLIMPRO_IIC_DEVID_SHIFT 13 +#define SLIMPRO_IIC_DEVID_MASK 0x007FE000U +#define SLIMPRO_IIC_RW_SHIFT 12 +#define SLIMPRO_IIC_RW_MASK 0x00001000U +#define SLIMPRO_IIC_PROTO_SHIFT 11 +#define SLIMPRO_IIC_PROTO_MASK 0x00000800U +#define SLIMPRO_IIC_ADDRLEN_SHIFT 8 +#define SLIMPRO_IIC_ADDRLEN_MASK 0x00000700U +#define SLIMPRO_IIC_DATALEN_SHIFT 0 +#define SLIMPRO_IIC_DATALEN_MASK 0x000000FFU + +/* + * SLIMpro I2C message encode + * + * dev - Controller number (0-based) + * chip - I2C chip address + * op - SLIMPRO_IIC_READ or SLIMPRO_IIC_WRITE + * proto - SLIMPRO_IIC_SMB_PROTOCOL or SLIMPRO_IIC_I2C_PROTOCOL + * addrlen - Length of the address field + * datalen - Length of the data field + */ +#define SLIMPRO_IIC_ENCODE_MSG(dev, chip, op, proto, addrlen, datalen) \ + ((SLIMPRO_DEBUG_MSG << SLIMPRO_MSG_TYPE_SHIFT) | \ + ((SLIMPRO_DBG_SUBTYPE_I2C1READ << SLIMPRO_DBGMSG_TYPE_SHIFT) & \ + SLIMPRO_DBGMSG_TYPE_MASK) | \ + ((dev << SLIMPRO_IIC_DEV_SHIFT) & SLIMPRO_IIC_DEV_MASK) | \ + ((chip << SLIMPRO_IIC_DEVID_SHIFT) & SLIMPRO_IIC_DEVID_MASK) | \ + ((op << SLIMPRO_IIC_RW_SHIFT) & SLIMPRO_IIC_RW_MASK) | \ + ((proto << SLIMPRO_IIC_PROTO_SHIFT) & SLIMPRO_IIC_PROTO_MASK) | \ + ((addrlen << SLIMPRO_IIC_ADDRLEN_SHIFT) & SLIMPRO_IIC_ADDRLEN_MASK) | \ + ((datalen << SLIMPRO_IIC_DATALEN_SHIFT) & SLIMPRO_IIC_DATALEN_MASK)) + +/* + * Encode for upper address for block data + */ +#define SLIMPRO_IIC_ENCODE_FLAG_BUFADDR 0x80000000 +#define SLIMPRO_IIC_ENCODE_FLAG_WITH_DATA_LEN(a) ((u32) (((a) << 30) \ + & 0x40000000)) +#define SLIMPRO_IIC_ENCODE_UPPER_BUFADDR(a) ((u32) (((a) >> 12) \ + & 0x3FF00000)) +#define SLIMPRO_IIC_ENCODE_ADDR(a) ((a) & 0x000FFFFF) + +struct slimpro_i2c_dev { + struct i2c_adapter adapter; + struct device *dev; + struct mbox_chan *mbox_chan; + struct mbox_client mbox_client; + struct completion rd_complete; + spinlock_t lock; + int i2c_rx_poll; + int i2c_tx_poll; + u8 dma_buffer[I2C_SMBUS_BLOCK_MAX]; + u32 *resp_msg; +}; + +#define to_slimpro_i2c_dev(cl) \ + container_of(cl, struct slimpro_i2c_dev, mbox_client) + +static void slimpro_i2c_rx_cb(struct mbox_client *cl, void *mssg) +{ + struct slimpro_i2c_dev *ctx = to_slimpro_i2c_dev(cl); + + /* + * Response message format: + * mssg[0] is the return code of the operation + * mssg[1] is the first data word + * mssg[2] is NOT used + */ + if (ctx->resp_msg) + *ctx->resp_msg = ((u32 *) mssg)[1]; + + if (ctx->mbox_client.tx_block) + complete(&ctx->rd_complete); + else + ctx->i2c_rx_poll = 0; +} + +static void slimpro_i2c_tx_done(struct mbox_client *cl, void *mssg, int r) +{ + struct slimpro_i2c_dev *ctx = to_slimpro_i2c_dev(cl); + + if (r == 0) + ctx->i2c_tx_poll = 0; +} + +static int start_i2c_msg_xfer(struct slimpro_i2c_dev *ctx) +{ + int count; + unsigned long flags; + + if (ctx->mbox_client.tx_block) { + if (!wait_for_completion_timeout(&ctx->rd_complete, + msecs_to_jiffies + (SLIMPRO_OP_TO_MS))) + return -EIO; + } else { + spin_lock_irqsave(&ctx->lock, flags); + ctx->i2c_rx_poll = 1; + for (count = SLIMPRO_I2C_WAIT_COUNT; count > 0; count--) { + if (ctx->i2c_rx_poll == 0) + break; + udelay(100); + } + + if (count == 0) { + ctx->i2c_rx_poll = 0; + ctx->mbox_client.tx_block = true; + spin_unlock_irqrestore(&ctx->lock, flags); + return -EIO; + } + ctx->mbox_client.tx_block = true; + spin_unlock_irqrestore(&ctx->lock, flags); + } + + /* Check of invalid data or no device */ + if (*ctx->resp_msg == 0xffffffff) + return -ENODEV; + + return 0; +} + +static int slimpro_i2c_rd(struct slimpro_i2c_dev *ctx, u32 chip, + u32 addr, u32 addrlen, u32 protocol, + u32 readlen, u32 *data) +{ + u32 msg[3]; + int rc; + + if (irqs_disabled()) + ctx->mbox_client.tx_block = false; + + msg[0] = SLIMPRO_IIC_ENCODE_MSG(SLIMPRO_IIC_BUS, chip, + SLIMPRO_IIC_READ, protocol, addrlen, + readlen); + msg[1] = SLIMPRO_IIC_ENCODE_ADDR(addr); + msg[2] = 0; + ctx->resp_msg = data; + if (ctx->mbox_client.tx_block) + init_completion(&ctx->rd_complete); + + rc = mbox_send_message(ctx->mbox_chan, &msg); + if (rc < 0) + goto err; + + rc = start_i2c_msg_xfer(ctx); +err: + ctx->resp_msg = NULL; + return rc; +} + +static int slimpro_i2c_wr(struct slimpro_i2c_dev *ctx, u32 chip, + u32 addr, u32 addrlen, u32 protocol, u32 writelen, + u32 data) +{ + u32 msg[3]; + int rc; + + if (irqs_disabled()) + ctx->mbox_client.tx_block = false; + + msg[0] = SLIMPRO_IIC_ENCODE_MSG(SLIMPRO_IIC_BUS, chip, + SLIMPRO_IIC_WRITE, protocol, addrlen, + writelen); + msg[1] = SLIMPRO_IIC_ENCODE_ADDR(addr); + msg[2] = data; + ctx->resp_msg = msg; + + if (ctx->mbox_client.tx_block) + init_completion(&ctx->rd_complete); + + rc = mbox_send_message(ctx->mbox_chan, &msg); + if (rc < 0) + goto err; + + rc = start_i2c_msg_xfer(ctx); +err: + ctx->resp_msg = NULL; + return rc; +} + +static int slimpro_i2c_blkrd(struct slimpro_i2c_dev *ctx, u32 chip, u32 addr, + u32 addrlen, u32 protocol, u32 readlen, + u32 with_data_len, void *data) +{ + dma_addr_t paddr; + u32 msg[3]; + int rc; + + paddr = dma_map_single(ctx->dev, ctx->dma_buffer, readlen, + DMA_FROM_DEVICE); + if (dma_mapping_error(ctx->dev, paddr)) { + dev_err(&ctx->adapter.dev, "Error in mapping dma buffer %p\n", + ctx->dma_buffer); + rc = -EIO; + goto err; + } + + if (irqs_disabled()) + ctx->mbox_client.tx_block = false; + + msg[0] = SLIMPRO_IIC_ENCODE_MSG(SLIMPRO_IIC_BUS, chip, + SLIMPRO_IIC_READ, protocol, addrlen, + readlen); + msg[1] = SLIMPRO_IIC_ENCODE_FLAG_BUFADDR | + SLIMPRO_IIC_ENCODE_FLAG_WITH_DATA_LEN(with_data_len) | + SLIMPRO_IIC_ENCODE_UPPER_BUFADDR(paddr) | + SLIMPRO_IIC_ENCODE_ADDR(addr); + msg[2] = (u32) paddr; + ctx->resp_msg = msg; + + if (ctx->mbox_client.tx_block) + init_completion(&ctx->rd_complete); + + rc = mbox_send_message(ctx->mbox_chan, &msg); + if (rc < 0) + goto err_unmap; + + rc = start_i2c_msg_xfer(ctx); + + /* Copy to destination */ + memcpy(data, ctx->dma_buffer, readlen); + +err_unmap: + dma_unmap_single(ctx->dev, paddr, readlen, DMA_FROM_DEVICE); +err: + ctx->resp_msg = NULL; + return rc; +} + +static int slimpro_i2c_blkwr(struct slimpro_i2c_dev *ctx, u32 chip, + u32 addr, u32 addrlen, u32 protocol, u32 writelen, + void *data) +{ + dma_addr_t paddr; + u32 msg[3]; + int rc; + + memcpy(ctx->dma_buffer, data, writelen); + paddr = dma_map_single(ctx->dev, ctx->dma_buffer, writelen, + DMA_TO_DEVICE); + if (dma_mapping_error(ctx->dev, paddr)) { + dev_err(&ctx->adapter.dev, "Error in mapping dma buffer %p\n", + ctx->dma_buffer); + rc = -EIO; + goto err; + } + + if (irqs_disabled()) + ctx->mbox_client.tx_block = false; + + msg[0] = SLIMPRO_IIC_ENCODE_MSG(SLIMPRO_IIC_BUS, chip, + SLIMPRO_IIC_WRITE, protocol, addrlen, + writelen); + msg[1] = SLIMPRO_IIC_ENCODE_FLAG_BUFADDR | + SLIMPRO_IIC_ENCODE_UPPER_BUFADDR(paddr) | + SLIMPRO_IIC_ENCODE_ADDR(addr); + msg[2] = (u32) paddr; + ctx->resp_msg = msg; + + if (ctx->mbox_client.tx_block) + init_completion(&ctx->rd_complete); + + rc = mbox_send_message(ctx->mbox_chan, &msg); + if (rc < 0) + goto err_unmap; + + rc = start_i2c_msg_xfer(ctx); + +err_unmap: + dma_unmap_single(ctx->dev, paddr, writelen, DMA_TO_DEVICE); +err: + ctx->resp_msg = NULL; + return rc; +} + +static int xgene_slimpro_i2c_xfer(struct i2c_adapter *adap, u16 addr, + unsigned short flags, char read_write, + u8 command, int size, + union i2c_smbus_data *data) +{ + struct slimpro_i2c_dev *ctx = i2c_get_adapdata(adap); + int ret = -EOPNOTSUPP; + u32 val; + + switch (size) { + case I2C_SMBUS_BYTE: + if (read_write == I2C_SMBUS_READ) { + ret = slimpro_i2c_rd(ctx, addr, 0, 0, + SLIMPRO_IIC_SMB_PROTOCOL, + BYTE_DATA, &val); + data->byte = val; + } else { + ret = slimpro_i2c_wr(ctx, addr, command, SMBUS_CMD_LEN, + SLIMPRO_IIC_SMB_PROTOCOL, + 0, 0); + } + break; + case I2C_SMBUS_BYTE_DATA: + if (read_write == I2C_SMBUS_READ) { + ret = slimpro_i2c_rd(ctx, addr, command, SMBUS_CMD_LEN, + SLIMPRO_IIC_SMB_PROTOCOL, + BYTE_DATA, &val); + data->byte = val; + } else { + val = data->byte; + ret = slimpro_i2c_wr(ctx, addr, command, SMBUS_CMD_LEN, + SLIMPRO_IIC_SMB_PROTOCOL, + BYTE_DATA, val); + } + break; + case I2C_SMBUS_WORD_DATA: + if (read_write == I2C_SMBUS_READ) { + ret = slimpro_i2c_rd(ctx, addr, command, SMBUS_CMD_LEN, + SLIMPRO_IIC_SMB_PROTOCOL, + WORD_DATA, &val); + data->word = val; + } else { + val = data->word; + ret = slimpro_i2c_wr(ctx, addr, command, SMBUS_CMD_LEN, + SLIMPRO_IIC_SMB_PROTOCOL, + WORD_DATA, val); + } + break; + case I2C_SMBUS_BLOCK_DATA: + if (read_write == I2C_SMBUS_READ) { + ret = slimpro_i2c_blkrd(ctx, addr, command, + SMBUS_CMD_LEN, + SLIMPRO_IIC_SMB_PROTOCOL, + I2C_SMBUS_BLOCK_MAX + 1, + IIC_SMB_WITH_DATA_LEN, + &data->block[0]); + + } else { + ret = slimpro_i2c_blkwr(ctx, addr, command, + SMBUS_CMD_LEN, + SLIMPRO_IIC_SMB_PROTOCOL, + data->block[0] + 1, + &data->block[0]); + } + break; + case I2C_SMBUS_I2C_BLOCK_DATA: + if (read_write == I2C_SMBUS_READ) { + ret = slimpro_i2c_blkrd(ctx, addr, + command, + SMBUS_CMD_LEN, + SLIMPRO_IIC_I2C_PROTOCOL, + I2C_SMBUS_BLOCK_MAX, + IIC_SMB_WITHOUT_DATA_LEN, + &data->block[1]); + } else { + ret = slimpro_i2c_blkwr(ctx, addr, command, + SMBUS_CMD_LEN, + SLIMPRO_IIC_I2C_PROTOCOL, + data->block[0], + &data->block[1]); + } + break; + default: + break; + } + return ret; +} + +/* +* Return list of supported functionality. +*/ +static u32 xgene_slimpro_i2c_func(struct i2c_adapter *adapter) +{ + return I2C_FUNC_SMBUS_BYTE | + I2C_FUNC_SMBUS_BYTE_DATA | + I2C_FUNC_SMBUS_WORD_DATA | + I2C_FUNC_SMBUS_BLOCK_DATA | + I2C_FUNC_SMBUS_I2C_BLOCK; +} + +static struct i2c_algorithm xgene_slimpro_i2c_algorithm = { + .smbus_xfer = xgene_slimpro_i2c_xfer, + .functionality = xgene_slimpro_i2c_func, +}; + +static int __init xgene_slimpro_i2c_probe(struct platform_device *pdev) +{ + struct slimpro_i2c_dev *ctx; + struct i2c_adapter *adapter; + struct mbox_client *cl; + int rc; + + ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL); + if (!ctx) + return -ENOMEM; + + ctx->dev = &pdev->dev; + platform_set_drvdata(pdev, ctx); + cl = &ctx->mbox_client; + + /* Request mailbox channel */ + cl->dev = &pdev->dev; + cl->rx_callback = slimpro_i2c_rx_cb; + cl->tx_done = slimpro_i2c_tx_done; + cl->tx_block = true; + cl->tx_tout = SLIMPRO_OP_TO_MS; + cl->knows_txdone = false; + cl->chan_name = "i2c-slimpro"; + ctx->mbox_chan = mbox_request_channel(cl); + if (IS_ERR(ctx->mbox_chan)) { + dev_err(&pdev->dev, "SLIMpro mailbox channel request failed\n"); + return PTR_ERR(ctx->mbox_chan); + } + + /* Initialiation in case of using poll mode */ + ctx->i2c_rx_poll = 0; + ctx->i2c_tx_poll = 0; + spin_lock_init(&ctx->lock); + + /* Setup I2C adapter */ + adapter = &ctx->adapter; + snprintf(adapter->name, sizeof(adapter->name), "X-Gene SLIMpro I2C"); + adapter->algo = &xgene_slimpro_i2c_algorithm; + adapter->class = I2C_CLASS_HWMON; + adapter->dev.parent = &pdev->dev; + i2c_set_adapdata(adapter, ctx); + rc = i2c_add_adapter(adapter); + if (rc) { + dev_err(&pdev->dev, "Adapter registeration failed\n"); + return rc; + } + + rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); + if (rc) + dev_warn(&pdev->dev, "Unable to set dma mask\n"); + + dev_info(&pdev->dev, "APM X-Gene SLIMpro I2C Adapter registered\n"); + return 0; +} + +static int xgene_slimpro_i2c_remove(struct platform_device *pdev) +{ + struct slimpro_i2c_dev *ctx = platform_get_drvdata(pdev); + + i2c_del_adapter(&ctx->adapter); + + mbox_free_channel(ctx->mbox_chan); + + return 0; +} + +#ifdef CONFIG_OF +static struct of_device_id xgene_slimpro_i2c_id[] = { + {.compatible = "apm,xgene-slimpro-i2c" }, + {}, +}; +MODULE_DEVICE_TABLE(of, xgene_slimpro_i2c_dt_ids); +#endif + +static struct platform_driver xgene_slimpro_i2c_driver = { + .probe = xgene_slimpro_i2c_probe, + .remove = xgene_slimpro_i2c_remove, + .driver = { + .name = XGENE_SLIMPRO_I2C, + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(xgene_slimpro_i2c_id) + }, +}; + +module_platform_driver(xgene_slimpro_i2c_driver); + +MODULE_DESCRIPTION("APM X-Gene SLIMpro I2C driver"); +MODULE_LICENSE("GPL v2");