Re: [PATCH v9 5/5] i2c: aspeed: added slave support for Aspeed I2C driver

[Brendan Higgins]

On Fri, Jun 2, 2017 at 5:04 AM, Benjamin Herrenschmidt
 wrote:
> On Fri, 2017-06-02 at 01:46 -0700, Brendan Higgins wrote:
>> Added slave support for Aspeed I2C controller. Supports fourteen busses
>> present in AST24XX and AST25XX BMC SoCs by Aspeed.
>
> (Not an issue for merging)
>
> Have you looked at a "mode" by which you implement just enough of the
> slave support to decode smbus notifications ?
>
> This looks like it could be useful when using pmbus devices, as some of
> them use smbus notifications fairly often.
>
> Today only one driver calls i2c_handle_smbus_host_notify() (the Intel
> one of course).
>

Although I have not thought about the exact thing you are mentioning, I
want to implement a number of other features for the slave. The thing I am
most interested in is adding support for smbus alert from the slave side.
Nevertheless, I wanted to actually get the driver in before I start trying to do
interesting things. As it is, the slave code satisfies our current purposes and
I do not know of anyone else who wants this feature at this time.

Cheers

Re: [PATCH v9 5/5] i2c: aspeed: added slave support for Aspeed I2C driver

[Brendan Higgins]

On Fri, Jun 2, 2017 at 5:04 AM, Benjamin Herrenschmidt
 wrote:
> On Fri, 2017-06-02 at 01:46 -0700, Brendan Higgins wrote:
>> Added slave support for Aspeed I2C controller. Supports fourteen busses
>> present in AST24XX and AST25XX BMC SoCs by Aspeed.
>
> (Not an issue for merging)
>
> Have you looked at a "mode" by which you implement just enough of the
> slave support to decode smbus notifications ?
>
> This looks like it could be useful when using pmbus devices, as some of
> them use smbus notifications fairly often.
>
> Today only one driver calls i2c_handle_smbus_host_notify() (the Intel
> one of course).
>

Although I have not thought about the exact thing you are mentioning, I
want to implement a number of other features for the slave. The thing I am
most interested in is adding support for smbus alert from the slave side.
Nevertheless, I wanted to actually get the driver in before I start trying to do
interesting things. As it is, the slave code satisfies our current purposes and
I do not know of anyone else who wants this feature at this time.

Cheers

Re: [PATCH v9 5/5] i2c: aspeed: added slave support for Aspeed I2C driver

[Benjamin Herrenschmidt]

On Fri, 2017-06-02 at 01:46 -0700, Brendan Higgins wrote:
> Added slave support for Aspeed I2C controller. Supports fourteen busses
> present in AST24XX and AST25XX BMC SoCs by Aspeed.

(Not an issue for merging)

Have you looked at a "mode" by which you implement just enough of the
slave support to decode smbus notifications ?

This looks like it could be useful when using pmbus devices, as some of
them use smbus notifications fairly often.

Today only one driver calls i2c_handle_smbus_host_notify() (the Intel
one of course).

Cheers,
Ben.

Re: [PATCH v9 5/5] i2c: aspeed: added slave support for Aspeed I2C driver

[Benjamin Herrenschmidt]

On Fri, 2017-06-02 at 01:46 -0700, Brendan Higgins wrote:
> Added slave support for Aspeed I2C controller. Supports fourteen busses
> present in AST24XX and AST25XX BMC SoCs by Aspeed.

(Not an issue for merging)

Have you looked at a "mode" by which you implement just enough of the
slave support to decode smbus notifications ?

This looks like it could be useful when using pmbus devices, as some of
them use smbus notifications fairly often.

Today only one driver calls i2c_handle_smbus_host_notify() (the Intel
one of course).

Cheers,
Ben.

[PATCH v9 5/5] i2c: aspeed: added slave support for Aspeed I2C driver

[Brendan Higgins]

Added slave support for Aspeed I2C controller. Supports fourteen busses
present in AST24XX and AST25XX BMC SoCs by Aspeed.

Signed-off-by: Brendan Higgins 
---
Added in v6:
  - Pulled slave support out of initial driver commit into its own commit.
  - No longer arbitrarily restrict bus to be slave xor master.
Changes for v7:
  - Added hardware reset function
  - Marked functions that need to be called with the lock held as "unlocked"
  - Did some cleanup
Changes for v8:
  - None
Changes for v9:
  - None
---
 drivers/i2c/busses/i2c-aspeed.c | 202 
 1 file changed, 202 insertions(+)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index a80f89fa0ed5..40671a60ddf0 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -49,6 +49,7 @@
 #define ASPEED_I2CD_SDA_DRIVE_1T_ENBIT(8)
 #define ASPEED_I2CD_M_SDA_DRIVE_1T_EN  BIT(7)
 #define ASPEED_I2CD_M_HIGH_SPEED_ENBIT(6)
+#define ASPEED_I2CD_SLAVE_EN   BIT(1)
 #define ASPEED_I2CD_MASTER_EN  BIT(0)
 
 /* 0x04 : I2CD Clock and AC Timing Control Register #1 */
@@ -69,6 +70,7 @@
  */
 #define ASPEED_I2CD_INTR_SDA_DL_TIMEOUTBIT(14)
 #define ASPEED_I2CD_INTR_BUS_RECOVER_DONE  BIT(13)
+#define ASPEED_I2CD_INTR_SLAVE_MATCH   BIT(7)
 #define ASPEED_I2CD_INTR_SCL_TIMEOUT   BIT(6)
 #define ASPEED_I2CD_INTR_ABNORMAL  BIT(5)
 #define ASPEED_I2CD_INTR_NORMAL_STOP   BIT(4)
@@ -106,6 +108,9 @@
 #define ASPEED_I2CD_M_TX_CMD   BIT(1)
 #define ASPEED_I2CD_M_START_CMDBIT(0)
 
+/* 0x18 : I2CD Slave Device Address Register   */
+#define ASPEED_I2CD_DEV_ADDR_MASK  GENMASK(6, 0)
+
 enum aspeed_i2c_master_state {
ASPEED_I2C_MASTER_START,
ASPEED_I2C_MASTER_TX_FIRST,
@@ -116,6 +121,15 @@ enum aspeed_i2c_master_state {
ASPEED_I2C_MASTER_INACTIVE,
 };
 
+enum aspeed_i2c_slave_state {
+   ASPEED_I2C_SLAVE_START,
+   ASPEED_I2C_SLAVE_READ_REQUESTED,
+   ASPEED_I2C_SLAVE_READ_PROCESSED,
+   ASPEED_I2C_SLAVE_WRITE_REQUESTED,
+   ASPEED_I2C_SLAVE_WRITE_RECEIVED,
+   ASPEED_I2C_SLAVE_STOP,
+};
+
 struct aspeed_i2c_bus {
struct i2c_adapter  adap;
struct device   *dev;
@@ -134,6 +148,10 @@ struct aspeed_i2c_bus {
size_t  msgs_count;
boolsend_stop;
int cmd_err;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+   struct i2c_client   *slave;
+   enum aspeed_i2c_slave_state slave_state;
+#endif /* CONFIG_I2C_SLAVE */
 };
 
 static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus);
@@ -205,6 +223,110 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus 
*bus)
return aspeed_i2c_reset(bus);
 }
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
+{
+   u32 command, irq_status, status_ack = 0;
+   struct i2c_client *slave = bus->slave;
+   bool irq_handled = true;
+   u8 value;
+
+   spin_lock(>lock);
+   if (!slave) {
+   irq_handled = false;
+   goto out;
+   }
+
+   command = readl(bus->base + ASPEED_I2C_CMD_REG);
+   irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+
+   /* Slave was requested, restart state machine. */
+   if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
+   status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
+   bus->slave_state = ASPEED_I2C_SLAVE_START;
+   }
+
+   /* Slave is not currently active, irq was for someone else. */
+   if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
+   irq_handled = false;
+   goto out;
+   }
+
+   dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
+   irq_status, command);
+
+   /* Slave was sent something. */
+   if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
+   value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
+   /* Handle address frame. */
+   if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
+   if (value & 0x1)
+   bus->slave_state =
+   ASPEED_I2C_SLAVE_READ_REQUESTED;
+   else
+   bus->slave_state =
+   
ASPEED_I2C_SLAVE_WRITE_REQUESTED;
+   }
+   status_ack |= ASPEED_I2CD_INTR_RX_DONE;
+   }
+
+   /* Slave was asked to stop. */
+   if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
+   status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
+   

[PATCH v9 5/5] i2c: aspeed: added slave support for Aspeed I2C driver

[Brendan Higgins]

Added slave support for Aspeed I2C controller. Supports fourteen busses
present in AST24XX and AST25XX BMC SoCs by Aspeed.

Signed-off-by: Brendan Higgins 
---
Added in v6:
  - Pulled slave support out of initial driver commit into its own commit.
  - No longer arbitrarily restrict bus to be slave xor master.
Changes for v7:
  - Added hardware reset function
  - Marked functions that need to be called with the lock held as "unlocked"
  - Did some cleanup
Changes for v8:
  - None
Changes for v9:
  - None
---
 drivers/i2c/busses/i2c-aspeed.c | 202 
 1 file changed, 202 insertions(+)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index a80f89fa0ed5..40671a60ddf0 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -49,6 +49,7 @@
 #define ASPEED_I2CD_SDA_DRIVE_1T_ENBIT(8)
 #define ASPEED_I2CD_M_SDA_DRIVE_1T_EN  BIT(7)
 #define ASPEED_I2CD_M_HIGH_SPEED_ENBIT(6)
+#define ASPEED_I2CD_SLAVE_EN   BIT(1)
 #define ASPEED_I2CD_MASTER_EN  BIT(0)
 
 /* 0x04 : I2CD Clock and AC Timing Control Register #1 */
@@ -69,6 +70,7 @@
  */
 #define ASPEED_I2CD_INTR_SDA_DL_TIMEOUTBIT(14)
 #define ASPEED_I2CD_INTR_BUS_RECOVER_DONE  BIT(13)
+#define ASPEED_I2CD_INTR_SLAVE_MATCH   BIT(7)
 #define ASPEED_I2CD_INTR_SCL_TIMEOUT   BIT(6)
 #define ASPEED_I2CD_INTR_ABNORMAL  BIT(5)
 #define ASPEED_I2CD_INTR_NORMAL_STOP   BIT(4)
@@ -106,6 +108,9 @@
 #define ASPEED_I2CD_M_TX_CMD   BIT(1)
 #define ASPEED_I2CD_M_START_CMDBIT(0)
 
+/* 0x18 : I2CD Slave Device Address Register   */
+#define ASPEED_I2CD_DEV_ADDR_MASK  GENMASK(6, 0)
+
 enum aspeed_i2c_master_state {
ASPEED_I2C_MASTER_START,
ASPEED_I2C_MASTER_TX_FIRST,
@@ -116,6 +121,15 @@ enum aspeed_i2c_master_state {
ASPEED_I2C_MASTER_INACTIVE,
 };
 
+enum aspeed_i2c_slave_state {
+   ASPEED_I2C_SLAVE_START,
+   ASPEED_I2C_SLAVE_READ_REQUESTED,
+   ASPEED_I2C_SLAVE_READ_PROCESSED,
+   ASPEED_I2C_SLAVE_WRITE_REQUESTED,
+   ASPEED_I2C_SLAVE_WRITE_RECEIVED,
+   ASPEED_I2C_SLAVE_STOP,
+};
+
 struct aspeed_i2c_bus {
struct i2c_adapter  adap;
struct device   *dev;
@@ -134,6 +148,10 @@ struct aspeed_i2c_bus {
size_t  msgs_count;
boolsend_stop;
int cmd_err;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+   struct i2c_client   *slave;
+   enum aspeed_i2c_slave_state slave_state;
+#endif /* CONFIG_I2C_SLAVE */
 };
 
 static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus);
@@ -205,6 +223,110 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus 
*bus)
return aspeed_i2c_reset(bus);
 }
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
+{
+   u32 command, irq_status, status_ack = 0;
+   struct i2c_client *slave = bus->slave;
+   bool irq_handled = true;
+   u8 value;
+
+   spin_lock(>lock);
+   if (!slave) {
+   irq_handled = false;
+   goto out;
+   }
+
+   command = readl(bus->base + ASPEED_I2C_CMD_REG);
+   irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+
+   /* Slave was requested, restart state machine. */
+   if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
+   status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
+   bus->slave_state = ASPEED_I2C_SLAVE_START;
+   }
+
+   /* Slave is not currently active, irq was for someone else. */
+   if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
+   irq_handled = false;
+   goto out;
+   }
+
+   dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
+   irq_status, command);
+
+   /* Slave was sent something. */
+   if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
+   value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
+   /* Handle address frame. */
+   if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
+   if (value & 0x1)
+   bus->slave_state =
+   ASPEED_I2C_SLAVE_READ_REQUESTED;
+   else
+   bus->slave_state =
+   
ASPEED_I2C_SLAVE_WRITE_REQUESTED;
+   }
+   status_ack |= ASPEED_I2CD_INTR_RX_DONE;
+   }
+
+   /* Slave was asked to stop. */
+   if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
+   status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
+   bus->slave_state =