This patch set adds ACPI support by using PCC mailbox communication interface.
Hoan Tran (2):
i2c: xgene-slimpro: Use a single function to send command message
i2c: xgene-slimpro: Add ACPI support by using PCC mailbox
drivers/i2c/busses/i2c-xgene-slimpro.c | 246 +++++++++++++++++++++++++--------
1 file changed, 192 insertions(+), 54 deletions(-)
--
1.9.1
This patch adds ACPI support by using PCC mailbox communication
interface.
Signed-off-by: Hoan Tran <[email protected]>
---
drivers/i2c/busses/i2c-xgene-slimpro.c | 179 ++++++++++++++++++++++++++++++---
1 file changed, 166 insertions(+), 13 deletions(-)
diff --git a/drivers/i2c/busses/i2c-xgene-slimpro.c b/drivers/i2c/busses/i2c-xgene-slimpro.c
index 96545aa..a5771c1 100644
--- a/drivers/i2c/busses/i2c-xgene-slimpro.c
+++ b/drivers/i2c/busses/i2c-xgene-slimpro.c
@@ -32,6 +32,8 @@
#include <linux/platform_device.h>
#include <linux/version.h>
+#include <acpi/pcc.h>
+
#define MAILBOX_OP_TIMEOUT 1000 /* Operation time out in ms */
#define MAILBOX_I2C_INDEX 0
#define SLIMPRO_IIC_BUS 1 /* Use I2C bus 1 only */
@@ -89,6 +91,8 @@
((addrlen << SLIMPRO_IIC_ADDRLEN_SHIFT) & SLIMPRO_IIC_ADDRLEN_MASK) | \
((datalen << SLIMPRO_IIC_DATALEN_SHIFT) & SLIMPRO_IIC_DATALEN_MASK))
+#define SLIMPRO_MSG_TYPE(v) (((v) & 0xF0000000) >> 28)
+
/*
* Encode for upper address for block data
*/
@@ -99,19 +103,48 @@
& 0x3FF00000))
#define SLIMPRO_IIC_ENCODE_ADDR(a) ((a) & 0x000FFFFF)
+#define SLIMPRO_IIC_MSG_DWORD_COUNT 3
+
+/* PCC related defines */
+#define PCC_SIGNATURE 0x50424300
+#define PCC_CMD_GENERATE_DB_INT BIT(15)
+#define PCC_STS_CMD_COMPLETE BIT(0)
+#define PCC_STS_SCI_DOORBELL BIT(1)
+#define PCC_STS_ERR BIT(2)
+#define PCC_STS_PLAT_NOTIFY BIT(3)
+
struct slimpro_i2c_dev {
struct i2c_adapter adapter;
struct device *dev;
struct mbox_chan *mbox_chan;
struct mbox_client mbox_client;
+ int mbox_idx;
struct completion rd_complete;
u8 dma_buffer[I2C_SMBUS_BLOCK_MAX + 1]; /* dma_buffer[0] is used for length */
u32 *resp_msg;
+ struct mutex tx_mutex;
+ phys_addr_t comm_base_addr;
+ void *pcc_comm_addr;
};
#define to_slimpro_i2c_dev(cl) \
container_of(cl, struct slimpro_i2c_dev, mbox_client)
+/*
+ * This function tests and clears a bitmask then returns its old value
+ */
+static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask)
+{
+ u16 ret, val;
+
+ val = le16_to_cpu(READ_ONCE(*addr));
+ ret = val & mask;
+ val &= ~mask;
+ WRITE_ONCE(*addr, cpu_to_le16(val));
+
+ return ret;
+}
+
static void slimpro_i2c_rx_cb(struct mbox_client *cl, void *mssg)
{
struct slimpro_i2c_dev *ctx = to_slimpro_i2c_dev(cl);
@@ -129,9 +162,53 @@ static void slimpro_i2c_rx_cb(struct mbox_client *cl, void *mssg)
complete(&ctx->rd_complete);
}
+static void slimpro_i2c_pcc_rx_cb(struct mbox_client *cl, void *msg)
+{
+ struct slimpro_i2c_dev *ctx = to_slimpro_i2c_dev(cl);
+ struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
+
+ /* Check if platform sends interrupt */
+ if (!xgene_word_tst_and_clr(&generic_comm_base->status,
+ PCC_STS_SCI_DOORBELL))
+ return;
+
+ if (xgene_word_tst_and_clr(&generic_comm_base->status,
+ PCC_STS_CMD_COMPLETE)) {
+ msg = generic_comm_base + 1;
+
+ /* Response message msg[1] contains the return value. */
+ if (ctx->resp_msg)
+ *ctx->resp_msg = ((u32 *)msg)[1];
+
+ complete(&ctx->rd_complete);
+ }
+}
+
+static void slimpro_i2c_pcc_tx_prepare(struct slimpro_i2c_dev *ctx, u32 *msg)
+{
+ struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr;
+ u32 *ptr = (void *)(generic_comm_base + 1);
+ u16 status;
+ int i;
+
+ WRITE_ONCE(generic_comm_base->signature,
+ cpu_to_le32(PCC_SIGNATURE | ctx->mbox_idx));
+
+ WRITE_ONCE(generic_comm_base->command,
+ cpu_to_le16(SLIMPRO_MSG_TYPE(msg[0]) | PCC_CMD_GENERATE_DB_INT));
+
+ status = le16_to_cpu(READ_ONCE(generic_comm_base->status));
+ status &= ~PCC_STS_CMD_COMPLETE;
+ WRITE_ONCE(generic_comm_base->status, cpu_to_le16(status));
+
+ /* Copy the message to the PCC comm space */
+ for (i = 0; i < SLIMPRO_IIC_MSG_DWORD_COUNT; i++)
+ WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));
+}
+
static int start_i2c_msg_xfer(struct slimpro_i2c_dev *ctx)
{
- if (ctx->mbox_client.tx_block) {
+ if (ctx->mbox_client.tx_block || !acpi_disabled) {
if (!wait_for_completion_timeout(&ctx->rd_complete,
msecs_to_jiffies(MAILBOX_OP_TIMEOUT)))
return -ETIMEDOUT;
@@ -152,6 +229,12 @@ static int slimpro_i2c_send_msg(struct slimpro_i2c_dev *ctx,
ctx->resp_msg = data;
+ if (!acpi_disabled) {
+ mutex_lock(&ctx->tx_mutex);
+ init_completion(&ctx->rd_complete);
+ slimpro_i2c_pcc_tx_prepare(ctx, msg);
+ }
+
rc = mbox_send_message(ctx->mbox_chan, msg);
if (rc < 0)
goto err;
@@ -159,6 +242,10 @@ static int slimpro_i2c_send_msg(struct slimpro_i2c_dev *ctx,
rc = start_i2c_msg_xfer(ctx);
err:
+ if (!acpi_disabled) {
+ mbox_chan_txdone(ctx->mbox_chan, 0);
+ mutex_unlock(&ctx->tx_mutex);
+ }
ctx->resp_msg = NULL;
return rc;
@@ -375,21 +462,78 @@ static int xgene_slimpro_i2c_probe(struct platform_device *pdev)
ctx->dev = &pdev->dev;
platform_set_drvdata(pdev, ctx);
+ mutex_init(&ctx->tx_mutex);
cl = &ctx->mbox_client;
/* Request mailbox channel */
cl->dev = &pdev->dev;
- cl->rx_callback = slimpro_i2c_rx_cb;
- cl->tx_block = true;
init_completion(&ctx->rd_complete);
cl->tx_tout = MAILBOX_OP_TIMEOUT;
cl->knows_txdone = false;
- ctx->mbox_chan = mbox_request_channel(cl, MAILBOX_I2C_INDEX);
- if (IS_ERR(ctx->mbox_chan)) {
- dev_err(&pdev->dev, "i2c mailbox channel request failed\n");
- return PTR_ERR(ctx->mbox_chan);
- }
+ if (acpi_disabled) {
+ cl->tx_block = true;
+ cl->rx_callback = slimpro_i2c_rx_cb;
+ ctx->mbox_chan = mbox_request_channel(cl, MAILBOX_I2C_INDEX);
+ if (IS_ERR(ctx->mbox_chan)) {
+ dev_err(&pdev->dev, "i2c mailbox channel request failed\n");
+ return PTR_ERR(ctx->mbox_chan);
+ }
+ } else {
+ struct acpi_pcct_hw_reduced *cppc_ss;
+
+ if (device_property_read_u32(&pdev->dev, "pcc-channel",
+ &ctx->mbox_idx))
+ ctx->mbox_idx = MAILBOX_I2C_INDEX;
+
+ cl->tx_block = false;
+ cl->rx_callback = slimpro_i2c_pcc_rx_cb;
+ ctx->mbox_chan = pcc_mbox_request_channel(cl, ctx->mbox_idx);
+ if (IS_ERR(ctx->mbox_chan)) {
+ dev_err(&pdev->dev, "PCC mailbox channel request failed\n");
+ return PTR_ERR(ctx->mbox_chan);
+ }
+
+ /*
+ * The PCC mailbox controller driver should
+ * have parsed the PCCT (global table of all
+ * PCC channels) and stored pointers to the
+ * subspace communication region in con_priv.
+ */
+ cppc_ss = ctx->mbox_chan->con_priv;
+ if (!cppc_ss) {
+ dev_err(&pdev->dev, "PPC subspace not found\n");
+ rc = -ENODEV;
+ goto mbox_err;
+ }
+
+ if (!ctx->mbox_chan->mbox->txdone_irq) {
+ dev_err(&pdev->dev, "PCC IRQ not supported\n");
+ rc = -ENODEV;
+ goto mbox_err;
+ }
+ /*
+ * This is the shared communication region
+ * for the OS and Platform to communicate over.
+ */
+ ctx->comm_base_addr = cppc_ss->base_address;
+ if (ctx->comm_base_addr) {
+ ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
+ cppc_ss->length,
+ MEMREMAP_WB);
+ } else {
+ dev_err(&pdev->dev, "Failed to get PCC comm region\n");
+ rc = -ENODEV;
+ goto mbox_err;
+ }
+
+ if (!ctx->pcc_comm_addr) {
+ dev_err(&pdev->dev,
+ "Failed to memremap PCC comm region\n");
+ rc = -ENOMEM;
+ goto mbox_err;
+ }
+ }
rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
if (rc)
dev_warn(&pdev->dev, "Unable to set dma mask\n");
@@ -403,13 +547,19 @@ static int xgene_slimpro_i2c_probe(struct platform_device *pdev)
adapter->dev.of_node = pdev->dev.of_node;
i2c_set_adapdata(adapter, ctx);
rc = i2c_add_adapter(adapter);
- if (rc) {
- mbox_free_channel(ctx->mbox_chan);
- return rc;
- }
+ if (rc)
+ goto mbox_err;
dev_info(&pdev->dev, "Mailbox I2C Adapter registered\n");
return 0;
+
+mbox_err:
+ if (acpi_disabled)
+ mbox_free_channel(ctx->mbox_chan);
+ else
+ pcc_mbox_free_channel(ctx->mbox_chan);
+
+ return rc;
}
static int xgene_slimpro_i2c_remove(struct platform_device *pdev)
@@ -418,7 +568,10 @@ static int xgene_slimpro_i2c_remove(struct platform_device *pdev)
i2c_del_adapter(&ctx->adapter);
- mbox_free_channel(ctx->mbox_chan);
+ if (acpi_disabled)
+ mbox_free_channel(ctx->mbox_chan);
+ else
+ pcc_mbox_free_channel(ctx->mbox_chan);
return 0;
}
--
1.9.1
This patch refactors the code to use a single message function to
send command message.
Signed-off-by: Hoan Tran <[email protected]>
---
drivers/i2c/busses/i2c-xgene-slimpro.c | 67 +++++++++++++---------------------
1 file changed, 26 insertions(+), 41 deletions(-)
diff --git a/drivers/i2c/busses/i2c-xgene-slimpro.c b/drivers/i2c/busses/i2c-xgene-slimpro.c
index dbe7e44..96545aa 100644
--- a/drivers/i2c/busses/i2c-xgene-slimpro.c
+++ b/drivers/i2c/busses/i2c-xgene-slimpro.c
@@ -144,49 +144,52 @@ static int start_i2c_msg_xfer(struct slimpro_i2c_dev *ctx)
return 0;
}
-static int slimpro_i2c_rd(struct slimpro_i2c_dev *ctx, u32 chip,
- u32 addr, u32 addrlen, u32 protocol,
- u32 readlen, u32 *data)
+static int slimpro_i2c_send_msg(struct slimpro_i2c_dev *ctx,
+ u32 *msg,
+ u32 *data)
{
- u32 msg[3];
int rc;
- 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;
- rc = mbox_send_message(ctx->mbox_chan, &msg);
+
+ 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_rd(struct slimpro_i2c_dev *ctx, u32 chip,
+ u32 addr, u32 addrlen, u32 protocol,
+ u32 readlen, u32 *data)
+{
+ u32 msg[3];
+
+ 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;
+
+ return slimpro_i2c_send_msg(ctx, msg, data);
+}
+
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;
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;
-
- 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;
+ return slimpro_i2c_send_msg(ctx, msg, msg);
}
static int slimpro_i2c_blkrd(struct slimpro_i2c_dev *ctx, u32 chip, u32 addr,
@@ -201,8 +204,7 @@ static int slimpro_i2c_blkrd(struct slimpro_i2c_dev *ctx, u32 chip, u32 addr,
if (dma_mapping_error(ctx->dev, paddr)) {
dev_err(&ctx->adapter.dev, "Error in mapping dma buffer %p\n",
ctx->dma_buffer);
- rc = -ENOMEM;
- goto err;
+ return -ENOMEM;
}
msg[0] = SLIMPRO_IIC_ENCODE_MSG(SLIMPRO_IIC_BUS, chip, SLIMPRO_IIC_READ,
@@ -212,21 +214,13 @@ static int slimpro_i2c_blkrd(struct slimpro_i2c_dev *ctx, u32 chip, u32 addr,
SLIMPRO_IIC_ENCODE_UPPER_BUFADDR(paddr) |
SLIMPRO_IIC_ENCODE_ADDR(addr);
msg[2] = (u32)paddr;
- ctx->resp_msg = msg;
- rc = mbox_send_message(ctx->mbox_chan, &msg);
- if (rc < 0)
- goto err_unmap;
-
- rc = start_i2c_msg_xfer(ctx);
+ rc = slimpro_i2c_send_msg(ctx, msg, msg);
/* 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;
}
@@ -244,8 +238,7 @@ static int slimpro_i2c_blkwr(struct slimpro_i2c_dev *ctx, u32 chip,
if (dma_mapping_error(ctx->dev, paddr)) {
dev_err(&ctx->adapter.dev, "Error in mapping dma buffer %p\n",
ctx->dma_buffer);
- rc = -ENOMEM;
- goto err;
+ return -ENOMEM;
}
msg[0] = SLIMPRO_IIC_ENCODE_MSG(SLIMPRO_IIC_BUS, chip, SLIMPRO_IIC_WRITE,
@@ -254,21 +247,13 @@ static int slimpro_i2c_blkwr(struct slimpro_i2c_dev *ctx, u32 chip,
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)
reinit_completion(&ctx->rd_complete);
- rc = mbox_send_message(ctx->mbox_chan, &msg);
- if (rc < 0)
- goto err_unmap;
-
- rc = start_i2c_msg_xfer(ctx);
+ rc = slimpro_i2c_send_msg(ctx, msg, msg);
-err_unmap:
dma_unmap_single(ctx->dev, paddr, writelen, DMA_TO_DEVICE);
-err:
- ctx->resp_msg = NULL;
return rc;
}
--
1.9.1
Hi All,
Do you have any comments on this patch set?
Thanks
On Tue, Mar 28, 2017 at 1:46 PM, Hoan Tran <[email protected]> wrote:
> This patch set adds ACPI support by using PCC mailbox communication interface.
>
> Hoan Tran (2):
> i2c: xgene-slimpro: Use a single function to send command message
> i2c: xgene-slimpro: Add ACPI support by using PCC mailbox
>
> drivers/i2c/busses/i2c-xgene-slimpro.c | 246 +++++++++++++++++++++++++--------
> 1 file changed, 192 insertions(+), 54 deletions(-)
>
> --
> 1.9.1
>
Hi,
have you tested these patches also without PCC? So, we can be sure there
is no regression?
> diff --git a/drivers/i2c/busses/i2c-xgene-slimpro.c b/drivers/i2c/busses/i2c-xgene-slimpro.c
> index 96545aa..a5771c1 100644
> --- a/drivers/i2c/busses/i2c-xgene-slimpro.c
> +++ b/drivers/i2c/busses/i2c-xgene-slimpro.c
> @@ -32,6 +32,8 @@
> #include <linux/platform_device.h>
> #include <linux/version.h>
>
> +#include <acpi/pcc.h>
> +
Please keep it sorted alphabetically.
> +#define PCC_CMD_GENERATE_DB_INT BIT(15)
> +#define PCC_STS_CMD_COMPLETE BIT(0)
> +#define PCC_STS_SCI_DOORBELL BIT(1)
> +#define PCC_STS_ERR BIT(2)
> +#define PCC_STS_PLAT_NOTIFY BIT(3)
Please keep it sorted by number.
> + if (!acpi_disabled) {
> + mutex_lock(&ctx->tx_mutex);
> + init_completion(&ctx->rd_complete);
reinit_completion?
> + slimpro_i2c_pcc_tx_prepare(ctx, msg);
> + }
> +
> + mutex_init(&ctx->tx_mutex);
Why this mutex, by the way? The i2c-core serializes access to the
adapter. Maybe I am missing something?
> + cppc_ss = ctx->mbox_chan->con_priv;
> + if (!cppc_ss) {
> + dev_err(&pdev->dev, "PPC subspace not found\n");
> + rc = -ENODEV;
> + goto mbox_err;
> + }
> +
> + if (!ctx->mbox_chan->mbox->txdone_irq) {
> + dev_err(&pdev->dev, "PCC IRQ not supported\n");
> + rc = -ENODEV;
> + goto mbox_err;
> + }
>
> + /*
> + * This is the shared communication region
> + * for the OS and Platform to communicate over.
> + */
> + ctx->comm_base_addr = cppc_ss->base_address;
> + if (ctx->comm_base_addr) {
> + ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
> + cppc_ss->length,
> + MEMREMAP_WB);
> + } else {
> + dev_err(&pdev->dev, "Failed to get PCC comm region\n");
> + rc = -ENODEV;
> + goto mbox_err;
> + }
I think it doesn't make sense to print a dev_err and return ENODEV which
is treated by the driver core as a non-error. It means "not present, but
OK". You probably want other error codes here.
Regards,
Wolfram
Hi Wolfram,
On Thu, Apr 20, 2017 at 1:05 AM, Wolfram Sang <[email protected]> wrote:
> Hi,
>
> have you tested these patches also without PCC? So, we can be sure there
> is no regression?
These patches are tested with/without PCC.
>
>> diff --git a/drivers/i2c/busses/i2c-xgene-slimpro.c b/drivers/i2c/busses/i2c-xgene-slimpro.c
>> index 96545aa..a5771c1 100644
>> --- a/drivers/i2c/busses/i2c-xgene-slimpro.c
>> +++ b/drivers/i2c/busses/i2c-xgene-slimpro.c
>> @@ -32,6 +32,8 @@
>> #include <linux/platform_device.h>
>> #include <linux/version.h>
>>
>> +#include <acpi/pcc.h>
>> +
>
> Please keep it sorted alphabetically.
>
>> +#define PCC_CMD_GENERATE_DB_INT BIT(15)
>> +#define PCC_STS_CMD_COMPLETE BIT(0)
>> +#define PCC_STS_SCI_DOORBELL BIT(1)
>> +#define PCC_STS_ERR BIT(2)
>> +#define PCC_STS_PLAT_NOTIFY BIT(3)
>
> Please keep it sorted by number.
>
>> + if (!acpi_disabled) {
>> + mutex_lock(&ctx->tx_mutex);
>> + init_completion(&ctx->rd_complete);
>
> reinit_completion?
OK,
>
>> + slimpro_i2c_pcc_tx_prepare(ctx, msg);
>> + }
>> +
>> + mutex_init(&ctx->tx_mutex);
>
> Why this mutex, by the way? The i2c-core serializes access to the
> adapter. Maybe I am missing something?
That's a good point, let me remove this mutex.
>
>> + cppc_ss = ctx->mbox_chan->con_priv;
>> + if (!cppc_ss) {
>> + dev_err(&pdev->dev, "PPC subspace not found\n");
>> + rc = -ENODEV;
>> + goto mbox_err;
>> + }
>> +
>> + if (!ctx->mbox_chan->mbox->txdone_irq) {
>> + dev_err(&pdev->dev, "PCC IRQ not supported\n");
>> + rc = -ENODEV;
>> + goto mbox_err;
>> + }
>>
>> + /*
>> + * This is the shared communication region
>> + * for the OS and Platform to communicate over.
>> + */
>> + ctx->comm_base_addr = cppc_ss->base_address;
>> + if (ctx->comm_base_addr) {
>> + ctx->pcc_comm_addr = memremap(ctx->comm_base_addr,
>> + cppc_ss->length,
>> + MEMREMAP_WB);
>> + } else {
>> + dev_err(&pdev->dev, "Failed to get PCC comm region\n");
>> + rc = -ENODEV;
>> + goto mbox_err;
>> + }
>
> I think it doesn't make sense to print a dev_err and return ENODEV which
> is treated by the driver core as a non-error. It means "not present, but
> OK". You probably want other error codes here.
How about -EINVAL for these -ENODEV error codes? Do you have any suggestion?
Thank for your comments!
Hoan
>
> Regards,
>
> Wolfram
>
> > I think it doesn't make sense to print a dev_err and return ENODEV which
> > is treated by the driver core as a non-error. It means "not present, but
> > OK". You probably want other error codes here.
>
> How about -EINVAL for these -ENODEV error codes? Do you have any suggestion?
-EINVAL will do, I would go for -ENOENT, probably. But it doesn't matter
much.