diff mbox

[2/2] i2c: xgene-slimpro: Add ACPI support by using PCC mailbox

Message ID 1490733977-23760-3-git-send-email-hotran@apm.com
State Superseded
Headers show

Commit Message

Hoan Tran March 28, 2017, 8:46 p.m. UTC 
This patch adds ACPI support by using PCC mailbox communication
interface.

Signed-off-by: Hoan Tran <hotran@apm.com>
---
 drivers/i2c/busses/i2c-xgene-slimpro.c | 179 ++++++++++++++++++++++++++++++---
 1 file changed, 166 insertions(+), 13 deletions(-)

Comments

Wolfram Sang April 20, 2017, 8:05 a.m. UTC | #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
Hoan Tran April 20, 2017, 8:38 p.m. UTC | #2 
Hi Wolfram,

On Thu, Apr 20, 2017 at 1:05 AM, Wolfram Sang <wsa@the-dreams.de> 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
>
--
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
Wolfram Sang April 21, 2017, 6:31 a.m. UTC | #3 
> > 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.
diff mbox

Patch

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;
 }