* [PATCH i2c-next] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-07-23 18:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <58d31319-83c0-969b-e3fd-4273818929fc@linux.intel.com>
Thanks James for the review. Please see my inline answers.
On 7/23/2018 11:10 AM, James Feist wrote:
> On 07/23/2018 10:48 AM, Jae Hyun Yoo wrote:
>> In most of cases, interrupt bits are set one by one but there are
>> also a lot of other cases that Aspeed I2C IP sends multiple
>> interrupt bits with combining master and slave events using a
>> single interrupt call. It happens much in multi-master environment
>
> much more
>
Thanks! Will fix it.
>> than single-master. For an example, when master is waiting for a
>> NORMAL_STOP interrupt in its MASTER_STOP state, SLAVE_MATCH and
>> RX_DONE interrupts could come along with the NORMAL_STOP in case of
>> an another master immediately sends data just after acquiring the
>> bus. In this case, the NORMAL_STOP interrupt should be handled
>> by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
>> handled by slave_irq. This commit modifies irq hadling logic to
>> handle the master/slave combined events properly.
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> ---
>> ? drivers/i2c/busses/i2c-aspeed.c | 137 ++++++++++++++++++--------------
>> ? 1 file changed, 76 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c
>> b/drivers/i2c/busses/i2c-aspeed.c
>> index efb89422d496..24d43f143a55 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -82,6 +82,11 @@
>> ? #define ASPEED_I2CD_INTR_RX_DONE??????????? BIT(2)
>> ? #define ASPEED_I2CD_INTR_TX_NAK??????????????? BIT(1)
>> ? #define ASPEED_I2CD_INTR_TX_ACK??????????????? BIT(0)
>> +#define ASPEED_I2CD_INTR_ERRORS?????????????????????????????? \
>> +??????? (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |?????????????????? \
>> +???????? ASPEED_I2CD_INTR_SCL_TIMEOUT |?????????????????????? \
>> +???????? ASPEED_I2CD_INTR_ABNORMAL |?????????????????????? \
>> +???????? ASPEED_I2CD_INTR_ARBIT_LOSS)
>> ? #define ASPEED_I2CD_INTR_ALL?????????????????????????????? \
>> ????????? (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |?????????????????? \
>> ?????????? ASPEED_I2CD_INTR_BUS_RECOVER_DONE |?????????????????? \
>> @@ -150,6 +155,7 @@ struct aspeed_i2c_bus {
>> ????? int??????????????? cmd_err;
>> ????? /* Protected only by i2c_lock_bus */
>> ????? int??????????????? master_xfer_result;
>> +??? u32??????????????? irq_status;
>> ? #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> ????? struct i2c_client??????? *slave;
>> ????? enum aspeed_i2c_slave_state??? slave_state;
>> @@ -229,36 +235,30 @@ static int aspeed_i2c_recover_bus(struct
>> aspeed_i2c_bus *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;
>> +??? u32 command, status_ack = 0;
>> ????? struct i2c_client *slave = bus->slave;
>> -??? bool irq_handled = true;
>> ????? u8 value;
>> -??? if (!slave) {
>> -??????? irq_handled = false;
>> -??????? goto out;
>> -??? }
>> +??? if (!slave)
>> +??????? return false;
>> ????? 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) {
>> +??? if (bus->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;
>> -??? }
>> +??? if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
>> +??????? return false;
>> ????? dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
>> -??????? irq_status, command);
>> +??????? bus->irq_status, command);
>> ????? /* Slave was sent something. */
>> -??? if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
>> +??? if (bus->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) {
>> @@ -273,28 +273,29 @@ static bool aspeed_i2c_slave_irq(struct
>> aspeed_i2c_bus *bus)
>> ????? }
>> ????? /* Slave was asked to stop. */
>> -??? if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
>> +??? if (bus->irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
>> ????????? status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
>> ????????? bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>> ????? }
>> -??? if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
>> +??? if (bus->irq_status & ASPEED_I2CD_INTR_TX_NAK) {
>> ????????? status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> ????????? bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>> ????? }
>> +??? if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK) {
>> +??????? status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> +??? }
>> ????? switch (bus->slave_state) {
>> ????? case ASPEED_I2C_SLAVE_READ_REQUESTED:
>> -??????? if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
>> +??????? if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK)
>> ????????????? dev_err(bus->dev, "Unexpected ACK on read request.\n");
>> ????????? bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
>> -
>> ????????? i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
>> ????????? writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
>> ????????? writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
>> ????????? break;
>> ????? case ASPEED_I2C_SLAVE_READ_PROCESSED:
>> -??????? status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> -??????? if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
>> +??????? if (!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))
>> ????????????? dev_err(bus->dev,
>> ????????????????? "Expected ACK after processed read.\n");
>> ????????? i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value);
>> @@ -317,14 +318,8 @@ static bool aspeed_i2c_slave_irq(struct
>> aspeed_i2c_bus *bus)
>> ????????? break;
>> ????? }
>> -??? if (status_ack != irq_status)
>> -??????? dev_err(bus->dev,
>> -??????????? "irq handled != irq. expected %x, but was %x\n",
>> -??????????? irq_status, status_ack);
>> -??? writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
>> -
>> -out:
>> -??? return irq_handled;
>> +??? bus->irq_status ^= status_ack;
>> +??? return !bus->irq_status;
>> ? }
>> ? #endif /* CONFIG_I2C_SLAVE */
>> @@ -382,19 +377,19 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
>> ? static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> ? {
>> -??? u32 irq_status, status_ack = 0, command = 0;
>> +??? u32 status_ack = 0, command = 0;
>> ????? struct i2c_msg *msg;
>> ????? u8 recv_byte;
>> ????? int ret;
>> -??? irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> -??? /* Ack all interrupt bits. */
>> -??? writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
>> -
>> -??? if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
>> +??? if (bus->irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
>> ????????? bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> ????????? status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
>> ????????? goto out_complete;
>> +??? } else {
>> +??????? /* Master is not currently active, irq was for someone else. */
>> +??????? if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE)
>> +??????????? goto out_no_complete;
>> ????? }
>> ????? /*
>> @@ -402,20 +397,23 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ?????? * should clear the command queue effectively taking us back to the
>> ?????? * INACTIVE state.
>> ?????? */
>> -??? ret = aspeed_i2c_is_irq_error(irq_status);
>> -??? if (ret < 0) {
>> +??? ret = aspeed_i2c_is_irq_error(bus->irq_status);
>> +??? if (ret) {
>> ????????? dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
>> -??????????? irq_status);
>> +??????????? bus->irq_status);
>> ????????? bus->cmd_err = ret;
>> ????????? bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> +??????? status_ack |= (bus->irq_status & ASPEED_I2CD_INTR_ERRORS);
>> ????????? goto out_complete;
>> ????? }
>> ????? /* We are in an invalid state; reset bus to a known state. */
>> ????? if (!bus->msgs) {
>> -??????? dev_err(bus->dev, "bus in unknown state\n");
>> +??????? dev_err(bus->dev, "bus in unknown state irq_status: 0x%x\n",
>> +??????????? bus->irq_status);
>> ????????? bus->cmd_err = -EIO;
>> -??????? if (bus->master_state != ASPEED_I2C_MASTER_STOP)
>> +??????? if (bus->master_state != ASPEED_I2C_MASTER_STOP &&
>> +??????????? bus->master_state != ASPEED_I2C_MASTER_INACTIVE)
>> ????????????? aspeed_i2c_do_stop(bus);
>> ????????? goto out_no_complete;
>> ????? }
>> @@ -427,8 +425,14 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ?????? * then update the state and handle the new state below.
>> ?????? */
>> ????? if (bus->master_state == ASPEED_I2C_MASTER_START) {
>> -??????? if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> -??????????? pr_devel("no slave present at %02x\n", msg->addr);
>> +??????? if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> +??????????? if (unlikely(!(bus->irq_status &
>> +???????????????????? ASPEED_I2CD_INTR_TX_NAK))) {
>> +??????????????? bus->cmd_err = -ENXIO;
>> +??????????????? bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> +??????????????? goto out_complete;
>> +??????????? }
>> +??????????? pr_devel("no slave present at %02x", msg->addr);
> Missing line feed character
Thanks for your pointing it out. Will add '\n' at the end of the
message.
>> ????????????? status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> ????????????? bus->cmd_err = -ENXIO;
>> ????????????? aspeed_i2c_do_stop(bus);
>> @@ -447,11 +451,12 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ????? switch (bus->master_state) {
>> ????? case ASPEED_I2C_MASTER_TX:
>> -??????? if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
>> +??????? if (unlikely(bus->irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
>> ????????????? dev_dbg(bus->dev, "slave NACKed TX\n");
>> ????????????? status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> ????????????? goto error_and_stop;
>> -??????? } else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> +??????? } else if (unlikely(!(bus->irq_status &
>> +????????????????????? ASPEED_I2CD_INTR_TX_ACK))) {
>> ????????????? dev_err(bus->dev, "slave failed to ACK TX\n");
>> ????????????? goto error_and_stop;
>> ????????? }
>> @@ -470,11 +475,11 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ????????? goto out_no_complete;
>> ????? case ASPEED_I2C_MASTER_RX_FIRST:
>> ????????? /* RX may not have completed yet (only address cycle) */
>> -??????? if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE))
>> +??????? if (!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))
>> ????????????? goto out_no_complete;
>> ????????? /* fallthrough intended */
>> ????? case ASPEED_I2C_MASTER_RX:
>> -??????? if (unlikely(!(irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
>> +??????? if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
>> ????????????? dev_err(bus->dev, "master failed to RX\n");
>> ????????????? goto error_and_stop;
>> ????????? }
>> @@ -505,8 +510,11 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ????????? }
>> ????????? goto out_no_complete;
>> ????? case ASPEED_I2C_MASTER_STOP:
>> -??????? if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
>> -??????????? dev_err(bus->dev, "master failed to STOP\n");
>> +??????? if (unlikely(!(bus->irq_status &
>> +?????????????????? ASPEED_I2CD_INTR_NORMAL_STOP))) {
>> +??????????? dev_err(bus->dev,
>> +??????????????? "master failed to STOP irq_status:0x%x\n",
>> +??????????????? bus->irq_status);
>> ????????????? bus->cmd_err = -EIO;
>> ????????????? /* Do not STOP as we have already tried. */
>> ????????? } else {
>> @@ -518,7 +526,7 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ????? case ASPEED_I2C_MASTER_INACTIVE:
>> ????????? dev_err(bus->dev,
>> ????????????? "master received interrupt 0x%08x, but is inactive\n",
>> -??????????? irq_status);
>> +??????????? bus->irq_status);
>> ????????? bus->cmd_err = -EIO;
>> ????????? /* Do not STOP as we should be inactive. */
>> ????????? goto out_complete;
>> @@ -540,33 +548,40 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ????????? bus->master_xfer_result = bus->msgs_index + 1;
>> ????? complete(&bus->cmd_complete);
>> ? out_no_complete:
>> -??? if (irq_status != status_ack)
>> -??????? dev_err(bus->dev,
>> -??????????? "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>> -??????????? irq_status, status_ack);
>> -??? return !!irq_status;
>> +??? bus->irq_status ^= status_ack;
>> +??? return !bus->irq_status;
>> ? }
>> ? static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>> ? {
>> ????? struct aspeed_i2c_bus *bus = dev_id;
>> -??? bool ret;
>> +??? u32 irq_received;
>> ????? spin_lock(&bus->lock);
>> +??? irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> +??? bus->irq_status = irq_received;
>> ? #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> -??? if (aspeed_i2c_slave_irq(bus)) {
>> -??????? dev_dbg(bus->dev, "irq handled by slave.\n");
>> -??????? ret = true;
>> -??????? goto out;
>> +??? if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
>> +??????? if (!aspeed_i2c_master_irq(bus))
>> +??????????? aspeed_i2c_slave_irq(bus);
>> +??? } else {
>> +??????? if (!aspeed_i2c_slave_irq(bus))
>> +??????????? aspeed_i2c_master_irq(bus);
>> ????? }
>> +#else
>> +??? aspeed_i2c_master_irq(bus);
>> ? #endif /* CONFIG_I2C_SLAVE */
>> -??? ret = aspeed_i2c_master_irq(bus);
>> +??? if (bus->irq_status)
>> +??????? dev_err(bus->dev,
>> +??????????? "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>> +??????????? irq_received, irq_received ^ bus->irq_status);
>> -out:
>> +??? /* Ack all interrupt bits. */
>> +??? writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>> ????? spin_unlock(&bus->lock);
>> -??? return ret ? IRQ_HANDLED : IRQ_NONE;
>> +??? return bus->irq_status ? IRQ_NONE : IRQ_HANDLED;
>> ? }
>> ? static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
>>
^ permalink raw reply
* Re: [PATCH i2c-next] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-07-23 18:13 UTC (permalink / raw)
To: James Feist, Brendan Higgins, Benjamin Herrenschmidt,
Joel Stanley, Andrew Jeffery, linux-i2c, openbmc,
linux-arm-kernel, linux-aspeed, linux-kernel
Cc: Vernon Mauery, Jarkko Nikula
In-Reply-To: <58d31319-83c0-969b-e3fd-4273818929fc@linux.intel.com>
Thanks James for the review. Please see my inline answers.
On 7/23/2018 11:10 AM, James Feist wrote:
> On 07/23/2018 10:48 AM, Jae Hyun Yoo wrote:
>> In most of cases, interrupt bits are set one by one but there are
>> also a lot of other cases that Aspeed I2C IP sends multiple
>> interrupt bits with combining master and slave events using a
>> single interrupt call. It happens much in multi-master environment
>
> much more
>
Thanks! Will fix it.
>> than single-master. For an example, when master is waiting for a
>> NORMAL_STOP interrupt in its MASTER_STOP state, SLAVE_MATCH and
>> RX_DONE interrupts could come along with the NORMAL_STOP in case of
>> an another master immediately sends data just after acquiring the
>> bus. In this case, the NORMAL_STOP interrupt should be handled
>> by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
>> handled by slave_irq. This commit modifies irq hadling logic to
>> handle the master/slave combined events properly.
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> ---
>> drivers/i2c/busses/i2c-aspeed.c | 137 ++++++++++++++++++--------------
>> 1 file changed, 76 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c
>> b/drivers/i2c/busses/i2c-aspeed.c
>> index efb89422d496..24d43f143a55 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -82,6 +82,11 @@
>> #define ASPEED_I2CD_INTR_RX_DONE BIT(2)
>> #define ASPEED_I2CD_INTR_TX_NAK BIT(1)
>> #define ASPEED_I2CD_INTR_TX_ACK BIT(0)
>> +#define ASPEED_I2CD_INTR_ERRORS \
>> + (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
>> + ASPEED_I2CD_INTR_SCL_TIMEOUT | \
>> + ASPEED_I2CD_INTR_ABNORMAL | \
>> + ASPEED_I2CD_INTR_ARBIT_LOSS)
>> #define ASPEED_I2CD_INTR_ALL \
>> (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
>> ASPEED_I2CD_INTR_BUS_RECOVER_DONE | \
>> @@ -150,6 +155,7 @@ struct aspeed_i2c_bus {
>> int cmd_err;
>> /* Protected only by i2c_lock_bus */
>> int master_xfer_result;
>> + u32 irq_status;
>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> struct i2c_client *slave;
>> enum aspeed_i2c_slave_state slave_state;
>> @@ -229,36 +235,30 @@ static int aspeed_i2c_recover_bus(struct
>> aspeed_i2c_bus *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;
>> + u32 command, status_ack = 0;
>> struct i2c_client *slave = bus->slave;
>> - bool irq_handled = true;
>> u8 value;
>> - if (!slave) {
>> - irq_handled = false;
>> - goto out;
>> - }
>> + if (!slave)
>> + return false;
>> 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) {
>> + if (bus->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;
>> - }
>> + if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
>> + return false;
>> dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
>> - irq_status, command);
>> + bus->irq_status, command);
>> /* Slave was sent something. */
>> - if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
>> + if (bus->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) {
>> @@ -273,28 +273,29 @@ static bool aspeed_i2c_slave_irq(struct
>> aspeed_i2c_bus *bus)
>> }
>> /* Slave was asked to stop. */
>> - if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
>> + if (bus->irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
>> status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
>> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>> }
>> - if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
>> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_NAK) {
>> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>> }
>> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK) {
>> + status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> + }
>> switch (bus->slave_state) {
>> case ASPEED_I2C_SLAVE_READ_REQUESTED:
>> - if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
>> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK)
>> dev_err(bus->dev, "Unexpected ACK on read request.\n");
>> bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
>> -
>> i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
>> writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
>> writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
>> break;
>> case ASPEED_I2C_SLAVE_READ_PROCESSED:
>> - status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> - if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
>> + if (!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))
>> dev_err(bus->dev,
>> "Expected ACK after processed read.\n");
>> i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value);
>> @@ -317,14 +318,8 @@ static bool aspeed_i2c_slave_irq(struct
>> aspeed_i2c_bus *bus)
>> break;
>> }
>> - if (status_ack != irq_status)
>> - dev_err(bus->dev,
>> - "irq handled != irq. expected %x, but was %x\n",
>> - irq_status, status_ack);
>> - writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
>> -
>> -out:
>> - return irq_handled;
>> + bus->irq_status ^= status_ack;
>> + return !bus->irq_status;
>> }
>> #endif /* CONFIG_I2C_SLAVE */
>> @@ -382,19 +377,19 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
>> static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> {
>> - u32 irq_status, status_ack = 0, command = 0;
>> + u32 status_ack = 0, command = 0;
>> struct i2c_msg *msg;
>> u8 recv_byte;
>> int ret;
>> - irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> - /* Ack all interrupt bits. */
>> - writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
>> -
>> - if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
>> + if (bus->irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
>> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
>> goto out_complete;
>> + } else {
>> + /* Master is not currently active, irq was for someone else. */
>> + if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE)
>> + goto out_no_complete;
>> }
>> /*
>> @@ -402,20 +397,23 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> * should clear the command queue effectively taking us back to the
>> * INACTIVE state.
>> */
>> - ret = aspeed_i2c_is_irq_error(irq_status);
>> - if (ret < 0) {
>> + ret = aspeed_i2c_is_irq_error(bus->irq_status);
>> + if (ret) {
>> dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
>> - irq_status);
>> + bus->irq_status);
>> bus->cmd_err = ret;
>> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> + status_ack |= (bus->irq_status & ASPEED_I2CD_INTR_ERRORS);
>> goto out_complete;
>> }
>> /* We are in an invalid state; reset bus to a known state. */
>> if (!bus->msgs) {
>> - dev_err(bus->dev, "bus in unknown state\n");
>> + dev_err(bus->dev, "bus in unknown state irq_status: 0x%x\n",
>> + bus->irq_status);
>> bus->cmd_err = -EIO;
>> - if (bus->master_state != ASPEED_I2C_MASTER_STOP)
>> + if (bus->master_state != ASPEED_I2C_MASTER_STOP &&
>> + bus->master_state != ASPEED_I2C_MASTER_INACTIVE)
>> aspeed_i2c_do_stop(bus);
>> goto out_no_complete;
>> }
>> @@ -427,8 +425,14 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> * then update the state and handle the new state below.
>> */
>> if (bus->master_state == ASPEED_I2C_MASTER_START) {
>> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> - pr_devel("no slave present at %02x\n", msg->addr);
>> + if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> + if (unlikely(!(bus->irq_status &
>> + ASPEED_I2CD_INTR_TX_NAK))) {
>> + bus->cmd_err = -ENXIO;
>> + bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> + goto out_complete;
>> + }
>> + pr_devel("no slave present at %02x", msg->addr);
> Missing line feed character
Thanks for your pointing it out. Will add '\n' at the end of the
message.
>> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> bus->cmd_err = -ENXIO;
>> aspeed_i2c_do_stop(bus);
>> @@ -447,11 +451,12 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> switch (bus->master_state) {
>> case ASPEED_I2C_MASTER_TX:
>> - if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
>> + if (unlikely(bus->irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
>> dev_dbg(bus->dev, "slave NACKed TX\n");
>> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> goto error_and_stop;
>> - } else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> + } else if (unlikely(!(bus->irq_status &
>> + ASPEED_I2CD_INTR_TX_ACK))) {
>> dev_err(bus->dev, "slave failed to ACK TX\n");
>> goto error_and_stop;
>> }
>> @@ -470,11 +475,11 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> goto out_no_complete;
>> case ASPEED_I2C_MASTER_RX_FIRST:
>> /* RX may not have completed yet (only address cycle) */
>> - if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE))
>> + if (!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))
>> goto out_no_complete;
>> /* fallthrough intended */
>> case ASPEED_I2C_MASTER_RX:
>> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
>> + if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
>> dev_err(bus->dev, "master failed to RX\n");
>> goto error_and_stop;
>> }
>> @@ -505,8 +510,11 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> }
>> goto out_no_complete;
>> case ASPEED_I2C_MASTER_STOP:
>> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
>> - dev_err(bus->dev, "master failed to STOP\n");
>> + if (unlikely(!(bus->irq_status &
>> + ASPEED_I2CD_INTR_NORMAL_STOP))) {
>> + dev_err(bus->dev,
>> + "master failed to STOP irq_status:0x%x\n",
>> + bus->irq_status);
>> bus->cmd_err = -EIO;
>> /* Do not STOP as we have already tried. */
>> } else {
>> @@ -518,7 +526,7 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> case ASPEED_I2C_MASTER_INACTIVE:
>> dev_err(bus->dev,
>> "master received interrupt 0x%08x, but is inactive\n",
>> - irq_status);
>> + bus->irq_status);
>> bus->cmd_err = -EIO;
>> /* Do not STOP as we should be inactive. */
>> goto out_complete;
>> @@ -540,33 +548,40 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> bus->master_xfer_result = bus->msgs_index + 1;
>> complete(&bus->cmd_complete);
>> out_no_complete:
>> - if (irq_status != status_ack)
>> - dev_err(bus->dev,
>> - "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>> - irq_status, status_ack);
>> - return !!irq_status;
>> + bus->irq_status ^= status_ack;
>> + return !bus->irq_status;
>> }
>> static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>> {
>> struct aspeed_i2c_bus *bus = dev_id;
>> - bool ret;
>> + u32 irq_received;
>> spin_lock(&bus->lock);
>> + irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> + bus->irq_status = irq_received;
>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> - if (aspeed_i2c_slave_irq(bus)) {
>> - dev_dbg(bus->dev, "irq handled by slave.\n");
>> - ret = true;
>> - goto out;
>> + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
>> + if (!aspeed_i2c_master_irq(bus))
>> + aspeed_i2c_slave_irq(bus);
>> + } else {
>> + if (!aspeed_i2c_slave_irq(bus))
>> + aspeed_i2c_master_irq(bus);
>> }
>> +#else
>> + aspeed_i2c_master_irq(bus);
>> #endif /* CONFIG_I2C_SLAVE */
>> - ret = aspeed_i2c_master_irq(bus);
>> + if (bus->irq_status)
>> + dev_err(bus->dev,
>> + "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>> + irq_received, irq_received ^ bus->irq_status);
>> -out:
>> + /* Ack all interrupt bits. */
>> + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>> spin_unlock(&bus->lock);
>> - return ret ? IRQ_HANDLED : IRQ_NONE;
>> + return bus->irq_status ? IRQ_NONE : IRQ_HANDLED;
>> }
>> static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
>>
^ permalink raw reply
* [PATCH i2c-next] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-07-23 18:13 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <58d31319-83c0-969b-e3fd-4273818929fc@linux.intel.com>
Thanks James for the review. Please see my inline answers.
On 7/23/2018 11:10 AM, James Feist wrote:
> On 07/23/2018 10:48 AM, Jae Hyun Yoo wrote:
>> In most of cases, interrupt bits are set one by one but there are
>> also a lot of other cases that Aspeed I2C IP sends multiple
>> interrupt bits with combining master and slave events using a
>> single interrupt call. It happens much in multi-master environment
>
> much more
>
Thanks! Will fix it.
>> than single-master. For an example, when master is waiting for a
>> NORMAL_STOP interrupt in its MASTER_STOP state, SLAVE_MATCH and
>> RX_DONE interrupts could come along with the NORMAL_STOP in case of
>> an another master immediately sends data just after acquiring the
>> bus. In this case, the NORMAL_STOP interrupt should be handled
>> by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
>> handled by slave_irq. This commit modifies irq hadling logic to
>> handle the master/slave combined events properly.
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> ---
>> ? drivers/i2c/busses/i2c-aspeed.c | 137 ++++++++++++++++++--------------
>> ? 1 file changed, 76 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c
>> b/drivers/i2c/busses/i2c-aspeed.c
>> index efb89422d496..24d43f143a55 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -82,6 +82,11 @@
>> ? #define ASPEED_I2CD_INTR_RX_DONE??????????? BIT(2)
>> ? #define ASPEED_I2CD_INTR_TX_NAK??????????????? BIT(1)
>> ? #define ASPEED_I2CD_INTR_TX_ACK??????????????? BIT(0)
>> +#define ASPEED_I2CD_INTR_ERRORS?????????????????????????????? \
>> +??????? (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |?????????????????? \
>> +???????? ASPEED_I2CD_INTR_SCL_TIMEOUT |?????????????????????? \
>> +???????? ASPEED_I2CD_INTR_ABNORMAL |?????????????????????? \
>> +???????? ASPEED_I2CD_INTR_ARBIT_LOSS)
>> ? #define ASPEED_I2CD_INTR_ALL?????????????????????????????? \
>> ????????? (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |?????????????????? \
>> ?????????? ASPEED_I2CD_INTR_BUS_RECOVER_DONE |?????????????????? \
>> @@ -150,6 +155,7 @@ struct aspeed_i2c_bus {
>> ????? int??????????????? cmd_err;
>> ????? /* Protected only by i2c_lock_bus */
>> ????? int??????????????? master_xfer_result;
>> +??? u32??????????????? irq_status;
>> ? #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> ????? struct i2c_client??????? *slave;
>> ????? enum aspeed_i2c_slave_state??? slave_state;
>> @@ -229,36 +235,30 @@ static int aspeed_i2c_recover_bus(struct
>> aspeed_i2c_bus *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;
>> +??? u32 command, status_ack = 0;
>> ????? struct i2c_client *slave = bus->slave;
>> -??? bool irq_handled = true;
>> ????? u8 value;
>> -??? if (!slave) {
>> -??????? irq_handled = false;
>> -??????? goto out;
>> -??? }
>> +??? if (!slave)
>> +??????? return false;
>> ????? 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) {
>> +??? if (bus->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;
>> -??? }
>> +??? if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
>> +??????? return false;
>> ????? dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
>> -??????? irq_status, command);
>> +??????? bus->irq_status, command);
>> ????? /* Slave was sent something. */
>> -??? if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
>> +??? if (bus->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) {
>> @@ -273,28 +273,29 @@ static bool aspeed_i2c_slave_irq(struct
>> aspeed_i2c_bus *bus)
>> ????? }
>> ????? /* Slave was asked to stop. */
>> -??? if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
>> +??? if (bus->irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
>> ????????? status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
>> ????????? bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>> ????? }
>> -??? if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
>> +??? if (bus->irq_status & ASPEED_I2CD_INTR_TX_NAK) {
>> ????????? status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> ????????? bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>> ????? }
>> +??? if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK) {
>> +??????? status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> +??? }
>> ????? switch (bus->slave_state) {
>> ????? case ASPEED_I2C_SLAVE_READ_REQUESTED:
>> -??????? if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
>> +??????? if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK)
>> ????????????? dev_err(bus->dev, "Unexpected ACK on read request.\n");
>> ????????? bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
>> -
>> ????????? i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
>> ????????? writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
>> ????????? writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
>> ????????? break;
>> ????? case ASPEED_I2C_SLAVE_READ_PROCESSED:
>> -??????? status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> -??????? if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
>> +??????? if (!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))
>> ????????????? dev_err(bus->dev,
>> ????????????????? "Expected ACK after processed read.\n");
>> ????????? i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value);
>> @@ -317,14 +318,8 @@ static bool aspeed_i2c_slave_irq(struct
>> aspeed_i2c_bus *bus)
>> ????????? break;
>> ????? }
>> -??? if (status_ack != irq_status)
>> -??????? dev_err(bus->dev,
>> -??????????? "irq handled != irq. expected %x, but was %x\n",
>> -??????????? irq_status, status_ack);
>> -??? writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
>> -
>> -out:
>> -??? return irq_handled;
>> +??? bus->irq_status ^= status_ack;
>> +??? return !bus->irq_status;
>> ? }
>> ? #endif /* CONFIG_I2C_SLAVE */
>> @@ -382,19 +377,19 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
>> ? static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> ? {
>> -??? u32 irq_status, status_ack = 0, command = 0;
>> +??? u32 status_ack = 0, command = 0;
>> ????? struct i2c_msg *msg;
>> ????? u8 recv_byte;
>> ????? int ret;
>> -??? irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> -??? /* Ack all interrupt bits. */
>> -??? writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
>> -
>> -??? if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
>> +??? if (bus->irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
>> ????????? bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> ????????? status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
>> ????????? goto out_complete;
>> +??? } else {
>> +??????? /* Master is not currently active, irq was for someone else. */
>> +??????? if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE)
>> +??????????? goto out_no_complete;
>> ????? }
>> ????? /*
>> @@ -402,20 +397,23 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ?????? * should clear the command queue effectively taking us back to the
>> ?????? * INACTIVE state.
>> ?????? */
>> -??? ret = aspeed_i2c_is_irq_error(irq_status);
>> -??? if (ret < 0) {
>> +??? ret = aspeed_i2c_is_irq_error(bus->irq_status);
>> +??? if (ret) {
>> ????????? dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
>> -??????????? irq_status);
>> +??????????? bus->irq_status);
>> ????????? bus->cmd_err = ret;
>> ????????? bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> +??????? status_ack |= (bus->irq_status & ASPEED_I2CD_INTR_ERRORS);
>> ????????? goto out_complete;
>> ????? }
>> ????? /* We are in an invalid state; reset bus to a known state. */
>> ????? if (!bus->msgs) {
>> -??????? dev_err(bus->dev, "bus in unknown state\n");
>> +??????? dev_err(bus->dev, "bus in unknown state irq_status: 0x%x\n",
>> +??????????? bus->irq_status);
>> ????????? bus->cmd_err = -EIO;
>> -??????? if (bus->master_state != ASPEED_I2C_MASTER_STOP)
>> +??????? if (bus->master_state != ASPEED_I2C_MASTER_STOP &&
>> +??????????? bus->master_state != ASPEED_I2C_MASTER_INACTIVE)
>> ????????????? aspeed_i2c_do_stop(bus);
>> ????????? goto out_no_complete;
>> ????? }
>> @@ -427,8 +425,14 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ?????? * then update the state and handle the new state below.
>> ?????? */
>> ????? if (bus->master_state == ASPEED_I2C_MASTER_START) {
>> -??????? if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> -??????????? pr_devel("no slave present at %02x\n", msg->addr);
>> +??????? if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> +??????????? if (unlikely(!(bus->irq_status &
>> +???????????????????? ASPEED_I2CD_INTR_TX_NAK))) {
>> +??????????????? bus->cmd_err = -ENXIO;
>> +??????????????? bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> +??????????????? goto out_complete;
>> +??????????? }
>> +??????????? pr_devel("no slave present at %02x", msg->addr);
> Missing line feed character
Thanks for your pointing it out. Will add '\n' at the end of the
message.
>> ????????????? status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> ????????????? bus->cmd_err = -ENXIO;
>> ????????????? aspeed_i2c_do_stop(bus);
>> @@ -447,11 +451,12 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ????? switch (bus->master_state) {
>> ????? case ASPEED_I2C_MASTER_TX:
>> -??????? if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
>> +??????? if (unlikely(bus->irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
>> ????????????? dev_dbg(bus->dev, "slave NACKed TX\n");
>> ????????????? status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> ????????????? goto error_and_stop;
>> -??????? } else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> +??????? } else if (unlikely(!(bus->irq_status &
>> +????????????????????? ASPEED_I2CD_INTR_TX_ACK))) {
>> ????????????? dev_err(bus->dev, "slave failed to ACK TX\n");
>> ????????????? goto error_and_stop;
>> ????????? }
>> @@ -470,11 +475,11 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ????????? goto out_no_complete;
>> ????? case ASPEED_I2C_MASTER_RX_FIRST:
>> ????????? /* RX may not have completed yet (only address cycle) */
>> -??????? if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE))
>> +??????? if (!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))
>> ????????????? goto out_no_complete;
>> ????????? /* fallthrough intended */
>> ????? case ASPEED_I2C_MASTER_RX:
>> -??????? if (unlikely(!(irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
>> +??????? if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
>> ????????????? dev_err(bus->dev, "master failed to RX\n");
>> ????????????? goto error_and_stop;
>> ????????? }
>> @@ -505,8 +510,11 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ????????? }
>> ????????? goto out_no_complete;
>> ????? case ASPEED_I2C_MASTER_STOP:
>> -??????? if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
>> -??????????? dev_err(bus->dev, "master failed to STOP\n");
>> +??????? if (unlikely(!(bus->irq_status &
>> +?????????????????? ASPEED_I2CD_INTR_NORMAL_STOP))) {
>> +??????????? dev_err(bus->dev,
>> +??????????????? "master failed to STOP irq_status:0x%x\n",
>> +??????????????? bus->irq_status);
>> ????????????? bus->cmd_err = -EIO;
>> ????????????? /* Do not STOP as we have already tried. */
>> ????????? } else {
>> @@ -518,7 +526,7 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ????? case ASPEED_I2C_MASTER_INACTIVE:
>> ????????? dev_err(bus->dev,
>> ????????????? "master received interrupt 0x%08x, but is inactive\n",
>> -??????????? irq_status);
>> +??????????? bus->irq_status);
>> ????????? bus->cmd_err = -EIO;
>> ????????? /* Do not STOP as we should be inactive. */
>> ????????? goto out_complete;
>> @@ -540,33 +548,40 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ????????? bus->master_xfer_result = bus->msgs_index + 1;
>> ????? complete(&bus->cmd_complete);
>> ? out_no_complete:
>> -??? if (irq_status != status_ack)
>> -??????? dev_err(bus->dev,
>> -??????????? "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>> -??????????? irq_status, status_ack);
>> -??? return !!irq_status;
>> +??? bus->irq_status ^= status_ack;
>> +??? return !bus->irq_status;
>> ? }
>> ? static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>> ? {
>> ????? struct aspeed_i2c_bus *bus = dev_id;
>> -??? bool ret;
>> +??? u32 irq_received;
>> ????? spin_lock(&bus->lock);
>> +??? irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> +??? bus->irq_status = irq_received;
>> ? #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> -??? if (aspeed_i2c_slave_irq(bus)) {
>> -??????? dev_dbg(bus->dev, "irq handled by slave.\n");
>> -??????? ret = true;
>> -??????? goto out;
>> +??? if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
>> +??????? if (!aspeed_i2c_master_irq(bus))
>> +??????????? aspeed_i2c_slave_irq(bus);
>> +??? } else {
>> +??????? if (!aspeed_i2c_slave_irq(bus))
>> +??????????? aspeed_i2c_master_irq(bus);
>> ????? }
>> +#else
>> +??? aspeed_i2c_master_irq(bus);
>> ? #endif /* CONFIG_I2C_SLAVE */
>> -??? ret = aspeed_i2c_master_irq(bus);
>> +??? if (bus->irq_status)
>> +??????? dev_err(bus->dev,
>> +??????????? "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>> +??????????? irq_received, irq_received ^ bus->irq_status);
>> -out:
>> +??? /* Ack all interrupt bits. */
>> +??? writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>> ????? spin_unlock(&bus->lock);
>> -??? return ret ? IRQ_HANDLED : IRQ_NONE;
>> +??? return bus->irq_status ? IRQ_NONE : IRQ_HANDLED;
>> ? }
>> ? static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
>>
^ permalink raw reply
* Re: [PATCH 1/2] Add sw2_sw4 voltage table to cpcap regulator.
From: Mark Brown @ 2018-07-23 18:13 UTC (permalink / raw)
To: Peter Geis
Cc: lgirdwood, robh+dt, mark.rutland, linux-kernel, devicetree,
linux-tegra
In-Reply-To: <e411faa3-7d42-710f-a18a-a4fdd764b60a@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 505 bytes --]
On Mon, Jul 23, 2018 at 01:58:26PM -0400, Peter Geis wrote:
> SW2 and SW4 use a shared table to provide voltage to the cpu core and
> devices on Tegra hardware.
> Added this table to the cpcap regulator driver as the first step to
> supporting this device on Tegra.
This also doesn't apply against current code (though it does now parse
OK), please check and resend - make sure you don't have other out of
tree changes and are using an up to date kernel (ideally my regulator
for-next branch) as a base.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] t3507: add a testcase showing failure with sparse checkout
From: Junio C Hamano @ 2018-07-23 18:12 UTC (permalink / raw)
To: Ben Peart; +Cc: Elijah Newren, git, benpeart, kewillf
In-Reply-To: <5250481c-f6a1-0784-28e7-f34a9d6d3c93@gmail.com>
Ben Peart <peartben@gmail.com> writes:
> On 7/21/2018 2:34 AM, Elijah Newren wrote:
>> From: Ben Peart <peartben@gmail.com>
>>
>> Signed-off-by: Elijah Newren <newren@gmail.com>
>> ---
>> Testcase provided by Ben, so committing with him as the author. Just need
>> a sign off from him.
>
> Thanks Elijah, consider it
>
> Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
>
Thanks; I'll also tweak the in-body From: line while applying to
match the Sign-off.
^ permalink raw reply
* next-20180723 build: 2 failures 11 warnings (next-20180723)
From: Mark Brown @ 2018-07-23 18:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <E1fheAy-0003tU-Dq@optimist>
On Mon, Jul 23, 2018 at 05:59:12PM +0100, Build bot for Mark Brown wrote:
Today's -next fails to build an arm64 allmodconfig with:
> arm64-allmodconfig
> ERROR: "__sync_icache_dcache" [drivers/xen/xen-privcmd.ko] undefined!
using
aarch64-linux-gnu-gcc (Linaro GCC 5.3-2016.05) 5.3.1 20160412
however it builds perfectly fine with
aarch64-linux-gnu-gcc (Linaro GCC 6.2-2016.11) 6.2.1 20161016
which honestly seems like a sensible and worthwhile upgrade at this
point anyway given that it's a year and a half old so I'm going to do
that for my builder (perhaps even jump on a newer version) but it seemed
worth highlighting in case this is considered undesirable. A similar
issue is hitting on KernelCI, we should probably look at upgrading the
toolchain there too.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180723/f22badc9/attachment-0001.sig>
^ permalink raw reply
* Re: next-20180723 build: 2 failures 11 warnings (next-20180723)
From: Mark Brown @ 2018-07-23 18:11 UTC (permalink / raw)
To: Catalin Marinas, Will Deacon
Cc: linaro-kernel, khilman, kernel-build-reports, linux-next,
matthew.hart, linux-arm-kernel
In-Reply-To: <E1fheAy-0003tU-Dq@optimist>
[-- Attachment #1.1: Type: text/plain, Size: 779 bytes --]
On Mon, Jul 23, 2018 at 05:59:12PM +0100, Build bot for Mark Brown wrote:
Today's -next fails to build an arm64 allmodconfig with:
> arm64-allmodconfig
> ERROR: "__sync_icache_dcache" [drivers/xen/xen-privcmd.ko] undefined!
using
aarch64-linux-gnu-gcc (Linaro GCC 5.3-2016.05) 5.3.1 20160412
however it builds perfectly fine with
aarch64-linux-gnu-gcc (Linaro GCC 6.2-2016.11) 6.2.1 20161016
which honestly seems like a sensible and worthwhile upgrade at this
point anyway given that it's a year and a half old so I'm going to do
that for my builder (perhaps even jump on a newer version) but it seemed
worth highlighting in case this is considered undesirable. A similar
issue is hitting on KernelCI, we should probably look at upgrading the
toolchain there too.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] IB/mlx5: avoid binding a new mpi unit to the same devices repeatedly
From: Daniel Jurgens @ 2018-07-23 18:11 UTC (permalink / raw)
To: Qing Huang, Or Gerlitz, Parav Pandit
Cc: Linux Kernel, RDMA mailing list, Jason Gunthorpe, Doug Ledford,
Leon Romanovsky, gerald.gibson
In-Reply-To: <556984ea-c35f-197d-0e45-16272da3f604@oracle.com>
On 7/23/2018 10:36 AM, Qing Huang wrote:
>
> Hi Daniel/Parav,
>
> Have you got a chance to review this patch? Thanks!
Hi Qing, sorry for the delay, I just got back to the office today. I don't agree with the proposed fix, I provided an alternative suggestion below.
>
>>> Or.
>>>
>>>> Reported-by: Gerald Gibson <gerald.gibson@oracle.com>
>>>> Signed-off-by: Qing Huang <qing.huang@oracle.com>
>>>> ---
>>>> drivers/infiniband/hw/mlx5/main.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
>>>> index b3ba9a2..1ddd1d3 100644
>>>> --- a/drivers/infiniband/hw/mlx5/main.c
>>>> +++ b/drivers/infiniband/hw/mlx5/main.c
>>>> @@ -6068,7 +6068,8 @@ static void *mlx5_ib_add_slave_port(struct mlx5_core_dev *mdev, u8 port_num)
>>>>
>>>> mutex_lock(&mlx5_ib_multiport_mutex);
>>>> list_for_each_entry(dev, &mlx5_ib_dev_list, ib_dev_list) {
>>>> - if (dev->sys_image_guid == mpi->sys_image_guid)
>>>> + if (dev->sys_image_guid == mpi->sys_image_guid &&
>>>> + !dev->port[mlx5_core_native_port_num(mdev) - 1].mp.mpi)
You shouldn't check the mpi field that without holding the lock in the mp structure. Prefer you change the print from a warning in mlx5_ib_bind_slave_port to a debug message.
>>>> bound = mlx5_ib_bind_slave_port(dev, mpi);
>>>>
>>>> if (bound) {
>>>> --
>>>> 2.9.3
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH v2 1/2] common/qat: add sgl header
From: De Lara Guarch, Pablo @ 2018-07-23 18:10 UTC (permalink / raw)
To: Trahe, Fiona, dev@dpdk.org; +Cc: Jozwiak, TomaszX
In-Reply-To: <1532351135-10064-1-git-send-email-fiona.trahe@intel.com>
> -----Original Message-----
> From: Trahe, Fiona
> Sent: Monday, July 23, 2018 2:06 PM
> To: dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; Jozwiak, TomaszX <tomaszx.jozwiak@intel.com>
> Subject: [PATCH v2 1/2] common/qat: add sgl header
>
> This patch refactors the sgl struct so it includes a flexible array of flat buffers as
> sym and compress PMDs can have different size sgls.
>
> Signed-off-by: Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
> Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
Applied to dpdk-next-crypto.
Thanks,
Pablo
^ permalink raw reply
* [PATCH i2c-next] i2c: aspeed: Handle master/slave combined irq events properly
From: James Feist @ 2018-07-23 18:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180723174808.4007-1-jae.hyun.yoo@linux.intel.com>
On 07/23/2018 10:48 AM, Jae Hyun Yoo wrote:
> In most of cases, interrupt bits are set one by one but there are
> also a lot of other cases that Aspeed I2C IP sends multiple
> interrupt bits with combining master and slave events using a
> single interrupt call. It happens much in multi-master environment
much more
> than single-master. For an example, when master is waiting for a
> NORMAL_STOP interrupt in its MASTER_STOP state, SLAVE_MATCH and
> RX_DONE interrupts could come along with the NORMAL_STOP in case of
> an another master immediately sends data just after acquiring the
> bus. In this case, the NORMAL_STOP interrupt should be handled
> by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
> handled by slave_irq. This commit modifies irq hadling logic to
> handle the master/slave combined events properly.
>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
> drivers/i2c/busses/i2c-aspeed.c | 137 ++++++++++++++++++--------------
> 1 file changed, 76 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index efb89422d496..24d43f143a55 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -82,6 +82,11 @@
> #define ASPEED_I2CD_INTR_RX_DONE BIT(2)
> #define ASPEED_I2CD_INTR_TX_NAK BIT(1)
> #define ASPEED_I2CD_INTR_TX_ACK BIT(0)
> +#define ASPEED_I2CD_INTR_ERRORS \
> + (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
> + ASPEED_I2CD_INTR_SCL_TIMEOUT | \
> + ASPEED_I2CD_INTR_ABNORMAL | \
> + ASPEED_I2CD_INTR_ARBIT_LOSS)
> #define ASPEED_I2CD_INTR_ALL \
> (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
> ASPEED_I2CD_INTR_BUS_RECOVER_DONE | \
> @@ -150,6 +155,7 @@ struct aspeed_i2c_bus {
> int cmd_err;
> /* Protected only by i2c_lock_bus */
> int master_xfer_result;
> + u32 irq_status;
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> struct i2c_client *slave;
> enum aspeed_i2c_slave_state slave_state;
> @@ -229,36 +235,30 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *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;
> + u32 command, status_ack = 0;
> struct i2c_client *slave = bus->slave;
> - bool irq_handled = true;
> u8 value;
>
> - if (!slave) {
> - irq_handled = false;
> - goto out;
> - }
> + if (!slave)
> + return false;
>
> 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) {
> + if (bus->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;
> - }
> + if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
> + return false;
>
> dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
> - irq_status, command);
> + bus->irq_status, command);
>
> /* Slave was sent something. */
> - if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
> + if (bus->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) {
> @@ -273,28 +273,29 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> }
>
> /* Slave was asked to stop. */
> - if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
> status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> }
> - if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_NAK) {
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> }
> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK) {
> + status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> + }
>
> switch (bus->slave_state) {
> case ASPEED_I2C_SLAVE_READ_REQUESTED:
> - if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK)
> dev_err(bus->dev, "Unexpected ACK on read request.\n");
> bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
> -
> i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
> writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
> writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
> break;
> case ASPEED_I2C_SLAVE_READ_PROCESSED:
> - status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> - if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
> + if (!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))
> dev_err(bus->dev,
> "Expected ACK after processed read.\n");
> i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value);
> @@ -317,14 +318,8 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> break;
> }
>
> - if (status_ack != irq_status)
> - dev_err(bus->dev,
> - "irq handled != irq. expected %x, but was %x\n",
> - irq_status, status_ack);
> - writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
> -
> -out:
> - return irq_handled;
> + bus->irq_status ^= status_ack;
> + return !bus->irq_status;
> }
> #endif /* CONFIG_I2C_SLAVE */
>
> @@ -382,19 +377,19 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
>
> static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> {
> - u32 irq_status, status_ack = 0, command = 0;
> + u32 status_ack = 0, command = 0;
> struct i2c_msg *msg;
> u8 recv_byte;
> int ret;
>
> - irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> - /* Ack all interrupt bits. */
> - writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
> -
> - if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
> goto out_complete;
> + } else {
> + /* Master is not currently active, irq was for someone else. */
> + if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE)
> + goto out_no_complete;
> }
>
> /*
> @@ -402,20 +397,23 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> * should clear the command queue effectively taking us back to the
> * INACTIVE state.
> */
> - ret = aspeed_i2c_is_irq_error(irq_status);
> - if (ret < 0) {
> + ret = aspeed_i2c_is_irq_error(bus->irq_status);
> + if (ret) {
> dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
> - irq_status);
> + bus->irq_status);
> bus->cmd_err = ret;
> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> + status_ack |= (bus->irq_status & ASPEED_I2CD_INTR_ERRORS);
> goto out_complete;
> }
>
> /* We are in an invalid state; reset bus to a known state. */
> if (!bus->msgs) {
> - dev_err(bus->dev, "bus in unknown state\n");
> + dev_err(bus->dev, "bus in unknown state irq_status: 0x%x\n",
> + bus->irq_status);
> bus->cmd_err = -EIO;
> - if (bus->master_state != ASPEED_I2C_MASTER_STOP)
> + if (bus->master_state != ASPEED_I2C_MASTER_STOP &&
> + bus->master_state != ASPEED_I2C_MASTER_INACTIVE)
> aspeed_i2c_do_stop(bus);
> goto out_no_complete;
> }
> @@ -427,8 +425,14 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> * then update the state and handle the new state below.
> */
> if (bus->master_state == ASPEED_I2C_MASTER_START) {
> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> - pr_devel("no slave present at %02x\n", msg->addr);
> + if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> + if (unlikely(!(bus->irq_status &
> + ASPEED_I2CD_INTR_TX_NAK))) {
> + bus->cmd_err = -ENXIO;
> + bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> + goto out_complete;
> + }
> + pr_devel("no slave present at %02x", msg->addr);
Missing line feed character
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> bus->cmd_err = -ENXIO;
> aspeed_i2c_do_stop(bus);
> @@ -447,11 +451,12 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>
> switch (bus->master_state) {
> case ASPEED_I2C_MASTER_TX:
> - if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
> + if (unlikely(bus->irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
> dev_dbg(bus->dev, "slave NACKed TX\n");
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> goto error_and_stop;
> - } else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> + } else if (unlikely(!(bus->irq_status &
> + ASPEED_I2CD_INTR_TX_ACK))) {
> dev_err(bus->dev, "slave failed to ACK TX\n");
> goto error_and_stop;
> }
> @@ -470,11 +475,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> goto out_no_complete;
> case ASPEED_I2C_MASTER_RX_FIRST:
> /* RX may not have completed yet (only address cycle) */
> - if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE))
> + if (!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))
> goto out_no_complete;
> /* fallthrough intended */
> case ASPEED_I2C_MASTER_RX:
> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
> + if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
> dev_err(bus->dev, "master failed to RX\n");
> goto error_and_stop;
> }
> @@ -505,8 +510,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> }
> goto out_no_complete;
> case ASPEED_I2C_MASTER_STOP:
> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
> - dev_err(bus->dev, "master failed to STOP\n");
> + if (unlikely(!(bus->irq_status &
> + ASPEED_I2CD_INTR_NORMAL_STOP))) {
> + dev_err(bus->dev,
> + "master failed to STOP irq_status:0x%x\n",
> + bus->irq_status);
> bus->cmd_err = -EIO;
> /* Do not STOP as we have already tried. */
> } else {
> @@ -518,7 +526,7 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> case ASPEED_I2C_MASTER_INACTIVE:
> dev_err(bus->dev,
> "master received interrupt 0x%08x, but is inactive\n",
> - irq_status);
> + bus->irq_status);
> bus->cmd_err = -EIO;
> /* Do not STOP as we should be inactive. */
> goto out_complete;
> @@ -540,33 +548,40 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> bus->master_xfer_result = bus->msgs_index + 1;
> complete(&bus->cmd_complete);
> out_no_complete:
> - if (irq_status != status_ack)
> - dev_err(bus->dev,
> - "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> - irq_status, status_ack);
> - return !!irq_status;
> + bus->irq_status ^= status_ack;
> + return !bus->irq_status;
> }
>
> static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> {
> struct aspeed_i2c_bus *bus = dev_id;
> - bool ret;
> + u32 irq_received;
>
> spin_lock(&bus->lock);
> + irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> + bus->irq_status = irq_received;
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> - if (aspeed_i2c_slave_irq(bus)) {
> - dev_dbg(bus->dev, "irq handled by slave.\n");
> - ret = true;
> - goto out;
> + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
> + if (!aspeed_i2c_master_irq(bus))
> + aspeed_i2c_slave_irq(bus);
> + } else {
> + if (!aspeed_i2c_slave_irq(bus))
> + aspeed_i2c_master_irq(bus);
> }
> +#else
> + aspeed_i2c_master_irq(bus);
> #endif /* CONFIG_I2C_SLAVE */
>
> - ret = aspeed_i2c_master_irq(bus);
> + if (bus->irq_status)
> + dev_err(bus->dev,
> + "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> + irq_received, irq_received ^ bus->irq_status);
>
> -out:
> + /* Ack all interrupt bits. */
> + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> spin_unlock(&bus->lock);
> - return ret ? IRQ_HANDLED : IRQ_NONE;
> + return bus->irq_status ? IRQ_NONE : IRQ_HANDLED;
> }
>
> static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
>
^ permalink raw reply
* Re: [PATCH i2c-next] i2c: aspeed: Handle master/slave combined irq events properly
From: James Feist @ 2018-07-23 18:10 UTC (permalink / raw)
To: Jae Hyun Yoo, Brendan Higgins, Benjamin Herrenschmidt,
Joel Stanley, Andrew Jeffery, linux-i2c, openbmc,
linux-arm-kernel, linux-aspeed, linux-kernel
Cc: Vernon Mauery, Jarkko Nikula
In-Reply-To: <20180723174808.4007-1-jae.hyun.yoo@linux.intel.com>
On 07/23/2018 10:48 AM, Jae Hyun Yoo wrote:
> In most of cases, interrupt bits are set one by one but there are
> also a lot of other cases that Aspeed I2C IP sends multiple
> interrupt bits with combining master and slave events using a
> single interrupt call. It happens much in multi-master environment
much more
> than single-master. For an example, when master is waiting for a
> NORMAL_STOP interrupt in its MASTER_STOP state, SLAVE_MATCH and
> RX_DONE interrupts could come along with the NORMAL_STOP in case of
> an another master immediately sends data just after acquiring the
> bus. In this case, the NORMAL_STOP interrupt should be handled
> by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
> handled by slave_irq. This commit modifies irq hadling logic to
> handle the master/slave combined events properly.
>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
> drivers/i2c/busses/i2c-aspeed.c | 137 ++++++++++++++++++--------------
> 1 file changed, 76 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index efb89422d496..24d43f143a55 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -82,6 +82,11 @@
> #define ASPEED_I2CD_INTR_RX_DONE BIT(2)
> #define ASPEED_I2CD_INTR_TX_NAK BIT(1)
> #define ASPEED_I2CD_INTR_TX_ACK BIT(0)
> +#define ASPEED_I2CD_INTR_ERRORS \
> + (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
> + ASPEED_I2CD_INTR_SCL_TIMEOUT | \
> + ASPEED_I2CD_INTR_ABNORMAL | \
> + ASPEED_I2CD_INTR_ARBIT_LOSS)
> #define ASPEED_I2CD_INTR_ALL \
> (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
> ASPEED_I2CD_INTR_BUS_RECOVER_DONE | \
> @@ -150,6 +155,7 @@ struct aspeed_i2c_bus {
> int cmd_err;
> /* Protected only by i2c_lock_bus */
> int master_xfer_result;
> + u32 irq_status;
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> struct i2c_client *slave;
> enum aspeed_i2c_slave_state slave_state;
> @@ -229,36 +235,30 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *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;
> + u32 command, status_ack = 0;
> struct i2c_client *slave = bus->slave;
> - bool irq_handled = true;
> u8 value;
>
> - if (!slave) {
> - irq_handled = false;
> - goto out;
> - }
> + if (!slave)
> + return false;
>
> 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) {
> + if (bus->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;
> - }
> + if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
> + return false;
>
> dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
> - irq_status, command);
> + bus->irq_status, command);
>
> /* Slave was sent something. */
> - if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
> + if (bus->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) {
> @@ -273,28 +273,29 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> }
>
> /* Slave was asked to stop. */
> - if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
> status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> }
> - if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_NAK) {
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> }
> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK) {
> + status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> + }
>
> switch (bus->slave_state) {
> case ASPEED_I2C_SLAVE_READ_REQUESTED:
> - if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK)
> dev_err(bus->dev, "Unexpected ACK on read request.\n");
> bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
> -
> i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
> writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
> writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
> break;
> case ASPEED_I2C_SLAVE_READ_PROCESSED:
> - status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> - if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
> + if (!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))
> dev_err(bus->dev,
> "Expected ACK after processed read.\n");
> i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value);
> @@ -317,14 +318,8 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> break;
> }
>
> - if (status_ack != irq_status)
> - dev_err(bus->dev,
> - "irq handled != irq. expected %x, but was %x\n",
> - irq_status, status_ack);
> - writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
> -
> -out:
> - return irq_handled;
> + bus->irq_status ^= status_ack;
> + return !bus->irq_status;
> }
> #endif /* CONFIG_I2C_SLAVE */
>
> @@ -382,19 +377,19 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
>
> static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> {
> - u32 irq_status, status_ack = 0, command = 0;
> + u32 status_ack = 0, command = 0;
> struct i2c_msg *msg;
> u8 recv_byte;
> int ret;
>
> - irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> - /* Ack all interrupt bits. */
> - writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
> -
> - if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
> goto out_complete;
> + } else {
> + /* Master is not currently active, irq was for someone else. */
> + if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE)
> + goto out_no_complete;
> }
>
> /*
> @@ -402,20 +397,23 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> * should clear the command queue effectively taking us back to the
> * INACTIVE state.
> */
> - ret = aspeed_i2c_is_irq_error(irq_status);
> - if (ret < 0) {
> + ret = aspeed_i2c_is_irq_error(bus->irq_status);
> + if (ret) {
> dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
> - irq_status);
> + bus->irq_status);
> bus->cmd_err = ret;
> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> + status_ack |= (bus->irq_status & ASPEED_I2CD_INTR_ERRORS);
> goto out_complete;
> }
>
> /* We are in an invalid state; reset bus to a known state. */
> if (!bus->msgs) {
> - dev_err(bus->dev, "bus in unknown state\n");
> + dev_err(bus->dev, "bus in unknown state irq_status: 0x%x\n",
> + bus->irq_status);
> bus->cmd_err = -EIO;
> - if (bus->master_state != ASPEED_I2C_MASTER_STOP)
> + if (bus->master_state != ASPEED_I2C_MASTER_STOP &&
> + bus->master_state != ASPEED_I2C_MASTER_INACTIVE)
> aspeed_i2c_do_stop(bus);
> goto out_no_complete;
> }
> @@ -427,8 +425,14 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> * then update the state and handle the new state below.
> */
> if (bus->master_state == ASPEED_I2C_MASTER_START) {
> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> - pr_devel("no slave present at %02x\n", msg->addr);
> + if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> + if (unlikely(!(bus->irq_status &
> + ASPEED_I2CD_INTR_TX_NAK))) {
> + bus->cmd_err = -ENXIO;
> + bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> + goto out_complete;
> + }
> + pr_devel("no slave present at %02x", msg->addr);
Missing line feed character
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> bus->cmd_err = -ENXIO;
> aspeed_i2c_do_stop(bus);
> @@ -447,11 +451,12 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>
> switch (bus->master_state) {
> case ASPEED_I2C_MASTER_TX:
> - if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
> + if (unlikely(bus->irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
> dev_dbg(bus->dev, "slave NACKed TX\n");
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> goto error_and_stop;
> - } else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> + } else if (unlikely(!(bus->irq_status &
> + ASPEED_I2CD_INTR_TX_ACK))) {
> dev_err(bus->dev, "slave failed to ACK TX\n");
> goto error_and_stop;
> }
> @@ -470,11 +475,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> goto out_no_complete;
> case ASPEED_I2C_MASTER_RX_FIRST:
> /* RX may not have completed yet (only address cycle) */
> - if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE))
> + if (!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))
> goto out_no_complete;
> /* fallthrough intended */
> case ASPEED_I2C_MASTER_RX:
> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
> + if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
> dev_err(bus->dev, "master failed to RX\n");
> goto error_and_stop;
> }
> @@ -505,8 +510,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> }
> goto out_no_complete;
> case ASPEED_I2C_MASTER_STOP:
> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
> - dev_err(bus->dev, "master failed to STOP\n");
> + if (unlikely(!(bus->irq_status &
> + ASPEED_I2CD_INTR_NORMAL_STOP))) {
> + dev_err(bus->dev,
> + "master failed to STOP irq_status:0x%x\n",
> + bus->irq_status);
> bus->cmd_err = -EIO;
> /* Do not STOP as we have already tried. */
> } else {
> @@ -518,7 +526,7 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> case ASPEED_I2C_MASTER_INACTIVE:
> dev_err(bus->dev,
> "master received interrupt 0x%08x, but is inactive\n",
> - irq_status);
> + bus->irq_status);
> bus->cmd_err = -EIO;
> /* Do not STOP as we should be inactive. */
> goto out_complete;
> @@ -540,33 +548,40 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> bus->master_xfer_result = bus->msgs_index + 1;
> complete(&bus->cmd_complete);
> out_no_complete:
> - if (irq_status != status_ack)
> - dev_err(bus->dev,
> - "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> - irq_status, status_ack);
> - return !!irq_status;
> + bus->irq_status ^= status_ack;
> + return !bus->irq_status;
> }
>
> static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> {
> struct aspeed_i2c_bus *bus = dev_id;
> - bool ret;
> + u32 irq_received;
>
> spin_lock(&bus->lock);
> + irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> + bus->irq_status = irq_received;
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> - if (aspeed_i2c_slave_irq(bus)) {
> - dev_dbg(bus->dev, "irq handled by slave.\n");
> - ret = true;
> - goto out;
> + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
> + if (!aspeed_i2c_master_irq(bus))
> + aspeed_i2c_slave_irq(bus);
> + } else {
> + if (!aspeed_i2c_slave_irq(bus))
> + aspeed_i2c_master_irq(bus);
> }
> +#else
> + aspeed_i2c_master_irq(bus);
> #endif /* CONFIG_I2C_SLAVE */
>
> - ret = aspeed_i2c_master_irq(bus);
> + if (bus->irq_status)
> + dev_err(bus->dev,
> + "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> + irq_received, irq_received ^ bus->irq_status);
>
> -out:
> + /* Ack all interrupt bits. */
> + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> spin_unlock(&bus->lock);
> - return ret ? IRQ_HANDLED : IRQ_NONE;
> + return bus->irq_status ? IRQ_NONE : IRQ_HANDLED;
> }
>
> static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
>
^ permalink raw reply
* [PATCH i2c-next] i2c: aspeed: Handle master/slave combined irq events properly
From: James Feist @ 2018-07-23 18:10 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180723174808.4007-1-jae.hyun.yoo@linux.intel.com>
On 07/23/2018 10:48 AM, Jae Hyun Yoo wrote:
> In most of cases, interrupt bits are set one by one but there are
> also a lot of other cases that Aspeed I2C IP sends multiple
> interrupt bits with combining master and slave events using a
> single interrupt call. It happens much in multi-master environment
much more
> than single-master. For an example, when master is waiting for a
> NORMAL_STOP interrupt in its MASTER_STOP state, SLAVE_MATCH and
> RX_DONE interrupts could come along with the NORMAL_STOP in case of
> an another master immediately sends data just after acquiring the
> bus. In this case, the NORMAL_STOP interrupt should be handled
> by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
> handled by slave_irq. This commit modifies irq hadling logic to
> handle the master/slave combined events properly.
>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
> drivers/i2c/busses/i2c-aspeed.c | 137 ++++++++++++++++++--------------
> 1 file changed, 76 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index efb89422d496..24d43f143a55 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -82,6 +82,11 @@
> #define ASPEED_I2CD_INTR_RX_DONE BIT(2)
> #define ASPEED_I2CD_INTR_TX_NAK BIT(1)
> #define ASPEED_I2CD_INTR_TX_ACK BIT(0)
> +#define ASPEED_I2CD_INTR_ERRORS \
> + (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
> + ASPEED_I2CD_INTR_SCL_TIMEOUT | \
> + ASPEED_I2CD_INTR_ABNORMAL | \
> + ASPEED_I2CD_INTR_ARBIT_LOSS)
> #define ASPEED_I2CD_INTR_ALL \
> (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
> ASPEED_I2CD_INTR_BUS_RECOVER_DONE | \
> @@ -150,6 +155,7 @@ struct aspeed_i2c_bus {
> int cmd_err;
> /* Protected only by i2c_lock_bus */
> int master_xfer_result;
> + u32 irq_status;
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> struct i2c_client *slave;
> enum aspeed_i2c_slave_state slave_state;
> @@ -229,36 +235,30 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *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;
> + u32 command, status_ack = 0;
> struct i2c_client *slave = bus->slave;
> - bool irq_handled = true;
> u8 value;
>
> - if (!slave) {
> - irq_handled = false;
> - goto out;
> - }
> + if (!slave)
> + return false;
>
> 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) {
> + if (bus->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;
> - }
> + if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
> + return false;
>
> dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
> - irq_status, command);
> + bus->irq_status, command);
>
> /* Slave was sent something. */
> - if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
> + if (bus->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) {
> @@ -273,28 +273,29 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> }
>
> /* Slave was asked to stop. */
> - if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
> status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> }
> - if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_NAK) {
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> }
> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK) {
> + status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> + }
>
> switch (bus->slave_state) {
> case ASPEED_I2C_SLAVE_READ_REQUESTED:
> - if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK)
> dev_err(bus->dev, "Unexpected ACK on read request.\n");
> bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
> -
> i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
> writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
> writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
> break;
> case ASPEED_I2C_SLAVE_READ_PROCESSED:
> - status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> - if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
> + if (!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))
> dev_err(bus->dev,
> "Expected ACK after processed read.\n");
> i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value);
> @@ -317,14 +318,8 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> break;
> }
>
> - if (status_ack != irq_status)
> - dev_err(bus->dev,
> - "irq handled != irq. expected %x, but was %x\n",
> - irq_status, status_ack);
> - writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
> -
> -out:
> - return irq_handled;
> + bus->irq_status ^= status_ack;
> + return !bus->irq_status;
> }
> #endif /* CONFIG_I2C_SLAVE */
>
> @@ -382,19 +377,19 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
>
> static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> {
> - u32 irq_status, status_ack = 0, command = 0;
> + u32 status_ack = 0, command = 0;
> struct i2c_msg *msg;
> u8 recv_byte;
> int ret;
>
> - irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> - /* Ack all interrupt bits. */
> - writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
> -
> - if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
> goto out_complete;
> + } else {
> + /* Master is not currently active, irq was for someone else. */
> + if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE)
> + goto out_no_complete;
> }
>
> /*
> @@ -402,20 +397,23 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> * should clear the command queue effectively taking us back to the
> * INACTIVE state.
> */
> - ret = aspeed_i2c_is_irq_error(irq_status);
> - if (ret < 0) {
> + ret = aspeed_i2c_is_irq_error(bus->irq_status);
> + if (ret) {
> dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
> - irq_status);
> + bus->irq_status);
> bus->cmd_err = ret;
> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> + status_ack |= (bus->irq_status & ASPEED_I2CD_INTR_ERRORS);
> goto out_complete;
> }
>
> /* We are in an invalid state; reset bus to a known state. */
> if (!bus->msgs) {
> - dev_err(bus->dev, "bus in unknown state\n");
> + dev_err(bus->dev, "bus in unknown state irq_status: 0x%x\n",
> + bus->irq_status);
> bus->cmd_err = -EIO;
> - if (bus->master_state != ASPEED_I2C_MASTER_STOP)
> + if (bus->master_state != ASPEED_I2C_MASTER_STOP &&
> + bus->master_state != ASPEED_I2C_MASTER_INACTIVE)
> aspeed_i2c_do_stop(bus);
> goto out_no_complete;
> }
> @@ -427,8 +425,14 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> * then update the state and handle the new state below.
> */
> if (bus->master_state == ASPEED_I2C_MASTER_START) {
> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> - pr_devel("no slave present at %02x\n", msg->addr);
> + if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> + if (unlikely(!(bus->irq_status &
> + ASPEED_I2CD_INTR_TX_NAK))) {
> + bus->cmd_err = -ENXIO;
> + bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> + goto out_complete;
> + }
> + pr_devel("no slave present at %02x", msg->addr);
Missing line feed character
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> bus->cmd_err = -ENXIO;
> aspeed_i2c_do_stop(bus);
> @@ -447,11 +451,12 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>
> switch (bus->master_state) {
> case ASPEED_I2C_MASTER_TX:
> - if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
> + if (unlikely(bus->irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
> dev_dbg(bus->dev, "slave NACKed TX\n");
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> goto error_and_stop;
> - } else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> + } else if (unlikely(!(bus->irq_status &
> + ASPEED_I2CD_INTR_TX_ACK))) {
> dev_err(bus->dev, "slave failed to ACK TX\n");
> goto error_and_stop;
> }
> @@ -470,11 +475,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> goto out_no_complete;
> case ASPEED_I2C_MASTER_RX_FIRST:
> /* RX may not have completed yet (only address cycle) */
> - if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE))
> + if (!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))
> goto out_no_complete;
> /* fallthrough intended */
> case ASPEED_I2C_MASTER_RX:
> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
> + if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
> dev_err(bus->dev, "master failed to RX\n");
> goto error_and_stop;
> }
> @@ -505,8 +510,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> }
> goto out_no_complete;
> case ASPEED_I2C_MASTER_STOP:
> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
> - dev_err(bus->dev, "master failed to STOP\n");
> + if (unlikely(!(bus->irq_status &
> + ASPEED_I2CD_INTR_NORMAL_STOP))) {
> + dev_err(bus->dev,
> + "master failed to STOP irq_status:0x%x\n",
> + bus->irq_status);
> bus->cmd_err = -EIO;
> /* Do not STOP as we have already tried. */
> } else {
> @@ -518,7 +526,7 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> case ASPEED_I2C_MASTER_INACTIVE:
> dev_err(bus->dev,
> "master received interrupt 0x%08x, but is inactive\n",
> - irq_status);
> + bus->irq_status);
> bus->cmd_err = -EIO;
> /* Do not STOP as we should be inactive. */
> goto out_complete;
> @@ -540,33 +548,40 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> bus->master_xfer_result = bus->msgs_index + 1;
> complete(&bus->cmd_complete);
> out_no_complete:
> - if (irq_status != status_ack)
> - dev_err(bus->dev,
> - "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> - irq_status, status_ack);
> - return !!irq_status;
> + bus->irq_status ^= status_ack;
> + return !bus->irq_status;
> }
>
> static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> {
> struct aspeed_i2c_bus *bus = dev_id;
> - bool ret;
> + u32 irq_received;
>
> spin_lock(&bus->lock);
> + irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> + bus->irq_status = irq_received;
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> - if (aspeed_i2c_slave_irq(bus)) {
> - dev_dbg(bus->dev, "irq handled by slave.\n");
> - ret = true;
> - goto out;
> + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
> + if (!aspeed_i2c_master_irq(bus))
> + aspeed_i2c_slave_irq(bus);
> + } else {
> + if (!aspeed_i2c_slave_irq(bus))
> + aspeed_i2c_master_irq(bus);
> }
> +#else
> + aspeed_i2c_master_irq(bus);
> #endif /* CONFIG_I2C_SLAVE */
>
> - ret = aspeed_i2c_master_irq(bus);
> + if (bus->irq_status)
> + dev_err(bus->dev,
> + "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> + irq_received, irq_received ^ bus->irq_status);
>
> -out:
> + /* Ack all interrupt bits. */
> + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> spin_unlock(&bus->lock);
> - return ret ? IRQ_HANDLED : IRQ_NONE;
> + return bus->irq_status ? IRQ_NONE : IRQ_HANDLED;
> }
>
> static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
>
^ permalink raw reply
* Re: [PATCH] btrfs: fix size_t format string
From: Randy Dunlap @ 2018-07-23 17:07 UTC (permalink / raw)
To: dsterba, Arnd Bergmann, Chris Mason, Josef Bacik, David Sterba,
Qu Wenruo, Nikolay Borisov, Su Yue, linux-btrfs, linux-kernel
In-Reply-To: <20180717150400.GA3126@twin.jikos.cz>
On 07/17/2018 08:04 AM, David Sterba wrote:
> On Tue, Jul 17, 2018 at 03:52:27PM +0200, Arnd Bergmann wrote:
>> The newly added check_block_group_item() function causes a build warning
>> on 32-bit architectures:
>>
>> fs/btrfs/tree-checker.c: In function 'check_block_group_item':
>> fs/btrfs/tree-checker.c:404:41: error: format '%lu' expects argument of type 'long unsigned int', but argument 6 has type 'unsigned int' [-Werror=format=]
>>
>> The type of a sizeof() expression is size_t, which is correctly printed
>> using the %zu format string.
>>
>> Fixes: 9dc16aad5660 ("btrfs: tree-checker: Verify block_group_item")
>
> Folded to the commit, thanks.
>
Hi David,
Where did this patch end up? linux-next-20180723 is still showing this
format warning.
thanks,
--
~Randy
^ permalink raw reply
* Re: [PATCH 1/2] t3507: add a testcase showing failure with sparse checkout
From: Eric Sunshine @ 2018-07-23 18:09 UTC (permalink / raw)
To: Ben Peart; +Cc: Elijah Newren, Git List, Junio C Hamano, Ben Peart, kewillf
In-Reply-To: <b56598a1-e543-500a-a39b-cee07fe1e533@gmail.com>
On Mon, Jul 23, 2018 at 9:12 AM Ben Peart <peartben@gmail.com> wrote:
> On 7/21/2018 3:21 AM, Eric Sunshine wrote:
> > On Sat, Jul 21, 2018 at 2:34 AM Elijah Newren <newren@gmail.com> wrote:
> >> + rm .git/info/sparse-checkout
> >
> > Should this cleanup be done by test_when_finished()?
>
> I think trying to use test_when_finished() for this really degrades the
> readability of the test. See below:
>
> test_expect_success 'failed cherry-pick with sparse-checkout' '
> pristine_detach initial &&
> test_config core.sparsecheckout true &&
> echo /unrelated >.git/info/sparse-checkout &&
> git read-tree --reset -u HEAD &&
> test_when_finished "echo \"/*\" >.git/info/sparse-checkout && git
> read-tree --reset -u HEAD && rm .git/info/sparse-checkout" &&
> test_must_fail git cherry-pick -Xours picked>actual &&
> test_i18ngrep ! "Changes not staged for commit:" actual
> '
>
> Given it takes multiple commands, I'd prefer to keep the setup and
> cleanup of the sparse checkout settings symmetrical.
Some observations:
The test_when_finished() ought to be called before the initial
git-read-tree, otherwise you risk leaving a .git/info/sparse-checkout
sitting around if git-read-tree fails.
The tear-down code could be moved to a function, in which case,
test_when_finished() would simply call that function.
Multi-line quoted strings are valid, so you don't need to string out
all the tear-down steps on a single line like that, and instead spread
them across multiple lines to improve readability.
test_when_finished() doesn't expect just a single quoted string as
argument. In fact, it can take many (unquoted) arguments, which also
allows you to spread the tear-down steps over multiple lines to
improve readability.
Multiple test_when_finished() invocations are allowed, so you could
spread out the tear-down commands that way (though they'd have to be
in reverse order, which would be bad for readability in this case,
thus not recommended).
Correctness ought to trump readability, not the other way around.
So, one possibility, which seems pretty readable to me:
test_expect_failure 'failed cherry-pick with sparse-checkout' '
pristine_detach initial &&
test_config core.sparseCheckout true &&
test_when_finished "
echo \"/*\" >.git/info/sparse-checkout
git read-tree --reset -u HEAD
rm .git/info/sparse-checkout" &&
echo /unrelated >.git/info/sparse-checkout &&
git read-tree --reset -u HEAD &&
test_must_fail git cherry-pick -Xours picked>actual &&
test_i18ngrep ! "Changes not staged for commit:" actual &&
'
Notice that I dropped the internal &&-chain in test_when_finish() to
ensure that the final 'rm' is invoked even if the cleanup
git-read-tree fails (though all bets are probably off, anyhow, if it
does fail).
^ permalink raw reply
* Re: [PATCH v2 1/4] i2c: imx: Fix reinit_completion() use
From: Wolfram Sang @ 2018-07-23 18:08 UTC (permalink / raw)
To: Esben Haabendal
Cc: linux-i2c, Wolfram Sang, Uwe Kleine-König, Rob Herring,
Mark Rutland, Yuan Yao, Esben Haabendal, Phil Reid, Lucas Stach,
Linus Walleij, Peter Rosin, Fabio Estevam, linux-kernel
In-Reply-To: <20180709094304.8814-2-esben.haabendal@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 607 bytes --]
On Mon, Jul 09, 2018 at 11:43:01AM +0200, Esben Haabendal wrote:
> From: Esben Haabendal <eha@deif.com>
>
> Make sure to call reinit_completion() before dma is started to avoid race
> condition where reinit_completion() is called after complete() and before
> wait_for_completion_timeout().
>
> Signed-off-by: Esben Haabendal <eha@deif.com>
> Fixes: ce1a78840ff7 ("i2c: imx: add DMA support for freescale i2c driver")
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Applied to for-current and added stable, thanks!
Uwe, do you have comments for the other patches, too?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH net-next] wan/fsl_ucc_hdlc: use IS_ERR_VALUE() to check return value of qe_muram_alloc
From: David Miller @ 2018-07-23 18:07 UTC (permalink / raw)
To: yuehaibing; +Cc: qiang.zhao, linux-kernel, netdev, linuxppc-dev
In-Reply-To: <20180723141233.19948-1-yuehaibing@huawei.com>
From: YueHaibing <yuehaibing@huawei.com>
Date: Mon, 23 Jul 2018 22:12:33 +0800
> qe_muram_alloc return a unsigned long integer,which should not
> compared with zero. check it using IS_ERR_VALUE() to fix this.
>
> Fixes: c19b6d246a35 ("drivers/net: support hdlc function for QE-UCC")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Applied, thanks.
^ permalink raw reply
* [PATCH] media: dw2102: Fix memleak on sequence of probes
From: Anton Vasilyev @ 2018-07-23 17:04 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Anton Vasilyev, Jonathan Corbet, Enrico Mioso, Bhumika Goyal,
Sean Young, linux-media, linux-kernel, ldv-project
Each call to dw2102_probe() allocates memory by kmemdup for structures
p1100, s660, p7500 and s421, but there is no their deallocation.
dvb_usb_device_init() copies the corresponding structure into
dvb_usb_device->props, so there is no use of original structure after
dvb_usb_device_init().
The patch moves structures from global scope to local and adds their
deallocation.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru>
---
drivers/media/usb/dvb-usb/dw2102.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/media/usb/dvb-usb/dw2102.c b/drivers/media/usb/dvb-usb/dw2102.c
index 0d4fdd34a710..9ce8b4d79d1f 100644
--- a/drivers/media/usb/dvb-usb/dw2102.c
+++ b/drivers/media/usb/dvb-usb/dw2102.c
@@ -2101,14 +2101,12 @@ static struct dvb_usb_device_properties s6x0_properties = {
}
};
-static struct dvb_usb_device_properties *p1100;
static const struct dvb_usb_device_description d1100 = {
"Prof 1100 USB ",
{&dw2102_table[PROF_1100], NULL},
{NULL},
};
-static struct dvb_usb_device_properties *s660;
static const struct dvb_usb_device_description d660 = {
"TeVii S660 USB",
{&dw2102_table[TEVII_S660], NULL},
@@ -2127,14 +2125,12 @@ static const struct dvb_usb_device_description d480_2 = {
{NULL},
};
-static struct dvb_usb_device_properties *p7500;
static const struct dvb_usb_device_description d7500 = {
"Prof 7500 USB DVB-S2",
{&dw2102_table[PROF_7500], NULL},
{NULL},
};
-static struct dvb_usb_device_properties *s421;
static const struct dvb_usb_device_description d421 = {
"TeVii S421 PCI",
{&dw2102_table[TEVII_S421], NULL},
@@ -2334,6 +2330,11 @@ static int dw2102_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
int retval = -ENOMEM;
+ struct dvb_usb_device_properties *p1100;
+ struct dvb_usb_device_properties *s660;
+ struct dvb_usb_device_properties *p7500;
+ struct dvb_usb_device_properties *s421;
+
p1100 = kmemdup(&s6x0_properties,
sizeof(struct dvb_usb_device_properties), GFP_KERNEL);
if (!p1100)
@@ -2402,8 +2403,16 @@ static int dw2102_probe(struct usb_interface *intf,
0 == dvb_usb_device_init(intf, &t220_properties,
THIS_MODULE, NULL, adapter_nr) ||
0 == dvb_usb_device_init(intf, &tt_s2_4600_properties,
- THIS_MODULE, NULL, adapter_nr))
+ THIS_MODULE, NULL, adapter_nr)) {
+
+ /* clean up copied properties */
+ kfree(s421);
+ kfree(p7500);
+ kfree(s660);
+ kfree(p1100);
+
return 0;
+ }
retval = -ENODEV;
kfree(s421);
--
2.18.0
^ permalink raw reply related
* [PATCH] i2c: davinci: Avoid zero value of CLKH
From: Wolfram Sang @ 2018-07-23 18:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180713152017.2207-1-alexander.sverdlin@nokia.com>
On Fri, Jul 13, 2018 at 05:20:17PM +0200, Alexander Sverdlin wrote:
> If CLKH is set to 0 I2C clock is not generated at all, so avoid this value
> and stretch the clock in this case.
>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
Applied to for-current, thanks!
I did not add stable because Alexander told me this is very likely not
to be observed on HW out there. But TI people are investigating more.
I suggest they resend this patch to stable if they see fit. D'accord
everyone?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180723/8ea58ced/attachment.sig>
^ permalink raw reply
* Re: [PATCH] i2c: davinci: Avoid zero value of CLKH
From: Wolfram Sang @ 2018-07-23 18:07 UTC (permalink / raw)
To: Alexander Sverdlin; +Cc: Sekhar Nori, linux-i2c, linux-arm-kernel, Kevin Hilman
In-Reply-To: <20180713152017.2207-1-alexander.sverdlin@nokia.com>
[-- Attachment #1.1: Type: text/plain, Size: 521 bytes --]
On Fri, Jul 13, 2018 at 05:20:17PM +0200, Alexander Sverdlin wrote:
> If CLKH is set to 0 I2C clock is not generated at all, so avoid this value
> and stretch the clock in this case.
>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
Applied to for-current, thanks!
I did not add stable because Alexander told me this is very likely not
to be observed on HW out there. But TI people are investigating more.
I suggest they resend this patch to stable if they see fit. D'accord
everyone?
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/2] media: usb: pwc: Don't use coherent DMA buffers for ISO transfer
From: Matwey V. Kornilov @ 2018-07-23 17:04 UTC (permalink / raw)
To: Tomasz Figa
Cc: Alan Stern, Ezequiel Garcia, Hans de Goede, Hans Verkuil,
Mauro Carvalho Chehab, Laurent Pinchart, Steven Rostedt, mingo,
Mike Isely, Bhumika Goyal, Colin King, Linux Media Mailing List,
Linux Kernel Mailing List, Kieran Bingham, keiichiw
In-Reply-To: <CAJs94EaXpiHaJS0M7SxUtmjNAL869sPe9XpC1rorHgJOum-Oog@mail.gmail.com>
2018-07-20 14:57 GMT+03:00 Matwey V. Kornilov <matwey@sai.msu.ru>:
> 2018-07-20 14:33 GMT+03:00 Tomasz Figa <tfiga@chromium.org>:
>> On Fri, Jul 20, 2018 at 8:23 PM Matwey V. Kornilov <matwey@sai.msu.ru> wrote:
>>>
>>> 2018-07-20 13:55 GMT+03:00 Tomasz Figa <tfiga@chromium.org>:
>>> > On Wed, Jul 18, 2018 at 5:51 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>>> >>
>>> >> On Tue, 17 Jul 2018, Ezequiel Garcia wrote:
>>> >>
>>> >> > Hi Matwey,
>>> >> >
>>> >> > First of all, sorry for the delay.
>>> >> >
>>> >> > Adding Alan and Hans. Guys, do you have any feedback here?
>>> >>
>>> >> ...
>>> >>
>>> >> > > > So, what is the benefit of using consistent
>>> >> > > > for these URBs, as opposed to streaming?
>>> >> > >
>>> >> > > I don't know, I think there is no real benefit and all we see is a
>>> >> > > consequence of copy-pasta when some webcam drivers were inspired by
>>> >> > > others and development priparily was going at x86 platforms.
>>> >> >
>>> >> > You are probably right about the copy-pasta.
>>> >> >
>>> >> > > It would
>>> >> > > be great if somebody corrected me here. DMA Coherence is quite strong
>>> >> > > property and I cannot figure out how can it help when streaming video.
>>> >> > > The CPU host always reads from the buffer and never writes to.
>>> >> > > Hardware perepherial always writes to and never reads from. Moreover,
>>> >> > > buffer access is mutually exclusive and separated in time by Interrupt
>>> >> > > fireing and URB starting (when we reuse existing URB for new request).
>>> >> > > Only single one memory barrier is really required here.
>>> >> > >
>>> >> >
>>> >> > Yeah, and not setting URB_NO_TRANSFER_DMA_MAP makes the USB core
>>> >> > create DMA mappings and use the streaming API. Which makes more
>>> >> > sense in hardware without hardware coherency.
>>> >>
>>> >> As far as I know, the _only_ advantage to using coherent DMA in this
>>> >> situation is that you then do not have to pay the overhead of
>>> >> constantly setting up and tearing down the streaming mappings. So it
>>> >> depends very much on the platform: If coherent buffers are cached then
>>> >> it's a slight win and otherwise it's a big lose.
>>> >
>>> > Isn't it about usb_alloc_coherent() being backed by DMA coherent API
>>> > (dma_alloc_coherent/attrs()) and ending up allocating uncached (or
>>> > write-combine) memory for devices with non-coherent DMAs? I'm not sure
>>>
>>> Yes, this is what exactly happens at armv7l platforms.
>>
>> Okay, thanks. So this seems to be exactly the same thing that is
>> happening in the UVC driver. There is quite a bit of random accesses
>> to extract some header fields and then a big memcpy into VB2 buffer to
>> assemble final frame.
>>
>> If we don't want to pay the cost of creating and destroying the
>> streaming mapping, we could map (dma_map_single()) once, set
>> transfer_dma of URB and URB_NO_TRANSFER_DMA_MAP and then just
>> synchronize the caches (dma_sync_single()) before submitting/after
>> completing the URB.
>
> Thank you. Now it is becoming clearer to me. Please allow me some time
> to try to sketch the implementation.
I've tried to strategies:
1) Use dma_unmap and dma_map inside the handler (I suppose this is
similar to how USB core does when there is no URB_NO_TRANSFER_DMA_MAP)
2) Use sync_cpu and sync_device inside the handler (and dma_map only
once at memory allocation)
It is interesting that dma_unmap/dma_map pair leads to the lower
overhead (+1us) than sync_cpu/sync_device (+2us) at x86_64 platform.
At armv7l platform using dma_unmap/dma_map leads to ~50 usec in the
handler, and sync_cpu/sync_device - ~65 usec.
However, I am not sure is it mandatory to call
dma_sync_single_for_device for FROM_DEVICE direction?
>
>>
>> Best regards,
>> Tomasz
>>
>
>
>
> --
> With best regards,
> Matwey V. Kornilov.
> Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
> 119234, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
--
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119234, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
^ permalink raw reply
* Re: [PATCH v2] pack-objects: fix performance issues on packing large deltas
From: Junio C Hamano @ 2018-07-23 18:04 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Elijah Newren, Jeff King
In-Reply-To: <20180722080421.12887-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Access to e->delta_size_ (and by extension
> pack->delta_size[e - pack->objects]) is unprotected as before, the
> thread scheduler in pack-objects must make sure "e" is never updated
> by two different threads.
OK. Do we need to worry about "e" (e.g. "e->delta_size_valid")
being accessed while/before it is set by another thread?
oe_delta_size() makes unprotected accesses to .delta_size_ and
pack->delta_size[e - pack->objects], so we apparently do not, and
oe_set_delta_size() only protects the allocation call and does not
prevent a reader in oe_delta_size() from first reading the _valid
field, noticing that it is 0 as initialized, and goes on to read the
pack->delta_size[] slot for the entry, while the writer is setting
the size to .delta_size_ field and flipping _valid bit, without ever
storing the size in the pack->delta_size[] array.
> @@ -130,6 +131,7 @@ struct packing_data {
> uint32_t index_size;
>
> unsigned int *in_pack_pos;
> + unsigned long *delta_size;
>
> /*
> * Only one of these can be non-NULL and they have different
> @@ -140,10 +142,29 @@ struct packing_data {
> struct packed_git **in_pack_by_idx;
> struct packed_git **in_pack;
>
> +#ifndef NO_PTHREADS
> + pthread_mutex_t lock;
I am wondering if we want the variable to say what data it is
protecting from simultaneous accesses, or leave it as generic so
that any new caller that wants to lock any (new) thing that is
associated with a packing_data structure can grab it for other
purposes. The design of this patch clearly is the latter, which is
OK for now, I think.
> @@ -332,18 +353,34 @@ static inline unsigned long oe_delta_size(struct packing_data *pack,
> {
> if (e->delta_size_valid)
> return e->delta_size_;
> - return oe_size(pack, e);
> +
> + /*
> + * pack->detla_size[] can't be NULL because oe_set_delta_size()
> + * must have been called when a new delta is saved with
> + * oe_set_delta().
> + * If oe_delta() returns NULL (i.e. default state, which means
> + * delta_size_valid is also false), then the caller must never
> + * call oe_delta_size().
> + */
> + return pack->delta_size[e - pack->objects];
> }
>
> static inline void oe_set_delta_size(struct packing_data *pack,
> struct object_entry *e,
> unsigned long size)
> {
> - e->delta_size_ = size;
> - e->delta_size_valid = e->delta_size_ == size;
> - if (!e->delta_size_valid && size != oe_size(pack, e))
> - BUG("this can only happen in check_object() "
> - "where delta size is the same as entry size");
> + if (size < pack->oe_delta_size_limit) {
> + e->delta_size_ = size;
> + e->delta_size_valid = 1;
> + } else {
> + packing_data_lock(pack);
> + if (!pack->delta_size)
> + ALLOC_ARRAY(pack->delta_size, pack->nr_alloc);
> + packing_data_unlock(pack);
> +
> + pack->delta_size[e - pack->objects] = size;
> + e->delta_size_valid = 0;
> + }
> }
^ permalink raw reply
* [Qemu-arm] [PATCH for-3.0?] hw/intc/arm_gicv3: Check correct HCR_EL2 bit when routing IRQ
From: Peter Maydell @ 2018-07-23 18:03 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: patches
In icc_dir_write() we were incorrectly checking HCR_EL2.FMO
when determining whether IRQ should be routed to EL2; this should
be HCR_EL2.IMO (compare the GICv3 pseudocode ICC_DIR_EL1[]).
Use the correct mask.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Cut-n-paste bug we've had since forever. Linux always sets
FMO and IMO to the same thing, so we haven't noticed this.
Probably should go into 3.0, though...
hw/intc/arm_gicv3_cpuif.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 5c89be1af07..2a60568d82c 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -1550,7 +1550,7 @@ static void icc_dir_write(CPUARMState *env, const ARMCPRegInfo *ri,
* tested in cases where we know !IsSecure is true.
*/
route_fiq_to_el2 = env->cp15.hcr_el2 & HCR_FMO;
- route_irq_to_el2 = env->cp15.hcr_el2 & HCR_FMO;
+ route_irq_to_el2 = env->cp15.hcr_el2 & HCR_IMO;
switch (arm_current_el(env)) {
case 3:
--
2.17.1
^ permalink raw reply related
* [Qemu-devel] [PATCH for-3.0?] hw/intc/arm_gicv3: Check correct HCR_EL2 bit when routing IRQ
From: Peter Maydell @ 2018-07-23 18:03 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: patches
In icc_dir_write() we were incorrectly checking HCR_EL2.FMO
when determining whether IRQ should be routed to EL2; this should
be HCR_EL2.IMO (compare the GICv3 pseudocode ICC_DIR_EL1[]).
Use the correct mask.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Cut-n-paste bug we've had since forever. Linux always sets
FMO and IMO to the same thing, so we haven't noticed this.
Probably should go into 3.0, though...
hw/intc/arm_gicv3_cpuif.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 5c89be1af07..2a60568d82c 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -1550,7 +1550,7 @@ static void icc_dir_write(CPUARMState *env, const ARMCPRegInfo *ri,
* tested in cases where we know !IsSecure is true.
*/
route_fiq_to_el2 = env->cp15.hcr_el2 & HCR_FMO;
- route_irq_to_el2 = env->cp15.hcr_el2 & HCR_FMO;
+ route_irq_to_el2 = env->cp15.hcr_el2 & HCR_IMO;
switch (arm_current_el(env)) {
case 3:
--
2.17.1
^ permalink raw reply related
* [PATCH v3] compress/isal: add chained mbuf support
From: Daly, Lee @ 2018-07-23 18:02 UTC (permalink / raw)
To: pablo.de.lara.guarch; +Cc: dev, Daly, Lee
In-Reply-To: <1531312811-177746-1-git-send-email-lee.daly@intel.com>
This patch adds chained mbuf support for input or output buffers
during compression/decompression operations.
Signed-off-by: Lee Daly <lee.daly@intel.com>
---
doc/guides/compressdevs/features/isal.ini | 17 +-
doc/guides/compressdevs/isal.rst | 2 -
drivers/compress/isal/isal_compress_pmd.c | 397 ++++++++++++++++++++------
drivers/compress/isal/isal_compress_pmd_ops.c | 5 +-
4 files changed, 323 insertions(+), 98 deletions(-)
diff --git a/doc/guides/compressdevs/features/isal.ini b/doc/guides/compressdevs/features/isal.ini
index 7183d10..919cf70 100644
--- a/doc/guides/compressdevs/features/isal.ini
+++ b/doc/guides/compressdevs/features/isal.ini
@@ -4,10 +4,13 @@
; Supported features of 'ISA-L' compression driver.
;
[Features]
-CPU SSE = Y
-CPU AVX = Y
-CPU AVX2 = Y
-CPU AVX512 = Y
-Deflate = Y
-Fixed = Y
-Dynamic = Y
+CPU SSE = Y
+CPU AVX = Y
+CPU AVX2 = Y
+CPU AVX512 = Y
+OOP SGL In SGL Out = Y
+OOP SGL In LB Out = Y
+OOP LB In SGL Out = Y
+Deflate = Y
+Fixed = Y
+Dynamic = Y
diff --git a/doc/guides/compressdevs/isal.rst b/doc/guides/compressdevs/isal.rst
index 276ff06..3bc3022 100644
--- a/doc/guides/compressdevs/isal.rst
+++ b/doc/guides/compressdevs/isal.rst
@@ -78,8 +78,6 @@ As a result the level mappings from the API to the PMD are shown below.
Limitations
-----------
-* Chained mbufs will not be supported until a future release, meaning max data size being passed to PMD through a single mbuf is 64K - 1. If data is larger than this it will need to be split up and sent as multiple operations.
-
* Compressdev level 0, no compression, is not supported.
* Checksums will not be supported until future release.
diff --git a/drivers/compress/isal/isal_compress_pmd.c b/drivers/compress/isal/isal_compress_pmd.c
index 5966cc3..a776019 100644
--- a/drivers/compress/isal/isal_compress_pmd.c
+++ b/drivers/compress/isal/isal_compress_pmd.c
@@ -188,6 +188,206 @@ isal_comp_set_priv_xform_parameters(struct isal_priv_xform *priv_xform,
return 0;
}
+/* Compression using chained mbufs for input/output data */
+static int
+chained_mbuf_compression(struct rte_comp_op *op, struct isal_comp_qp *qp)
+{
+ int ret;
+ uint32_t remaining_offset;
+ uint32_t remaining_data = op->src.length;
+ struct rte_mbuf *src = op->m_src;
+ struct rte_mbuf *dst = op->m_dst;
+
+ /* check for offset passing multiple segments
+ * and point compression stream to input/output buffer
+ */
+ if (op->src.offset > src->data_len) {
+ remaining_offset = op->src.offset;
+ while (remaining_offset > src->data_len) {
+ remaining_offset -= src->data_len;
+ src = src->next;
+ }
+
+
+ qp->stream->avail_in = src->data_len - remaining_offset;
+ qp->stream->next_in = rte_pktmbuf_mtod_offset(src, uint8_t *,
+ (op->src.offset % src->data_len));
+ } else {
+ qp->stream->avail_in = src->data_len - op->src.offset;
+ qp->stream->next_in = rte_pktmbuf_mtod_offset(src, uint8_t *,
+ op->src.offset);
+ }
+
+ if (op->dst.offset > dst->data_len) {
+ remaining_offset = op->dst.offset;
+ while (remaining_offset > dst->data_len) {
+ remaining_offset -= dst->data_len;
+ dst = dst->next;
+ }
+
+ qp->stream->avail_out = dst->data_len - remaining_offset;
+ qp->stream->next_out = rte_pktmbuf_mtod_offset(dst, uint8_t *,
+ (op->dst.offset % dst->data_len));
+ } else {
+ qp->stream->avail_out = dst->data_len - op->dst.offset;
+ qp->stream->next_out = rte_pktmbuf_mtod_offset(dst, uint8_t *,
+ op->dst.offset);
+ }
+
+ if (unlikely(!qp->stream->next_in || !qp->stream->next_out)) {
+ ISAL_PMD_LOG(ERR, "Invalid source or destination buffer\n");
+ op->status = RTE_COMP_OP_STATUS_INVALID_ARGS;
+ return -1;
+ }
+
+ while (qp->stream->internal_state.state != ZSTATE_END) {
+ /* Last segment of data */
+ if (remaining_data <= src->data_len)
+ qp->stream->end_of_stream = 1;
+
+ /* Execute compression operation */
+ ret = isal_deflate(qp->stream);
+
+ remaining_data = op->src.length - qp->stream->total_in;
+
+ if (ret != COMP_OK) {
+ ISAL_PMD_LOG(ERR, "Compression operation failed\n");
+ op->status = RTE_COMP_OP_STATUS_ERROR;
+ return ret;
+ }
+
+ if (qp->stream->avail_in == 0 &&
+ qp->stream->total_in != op->src.length) {
+ if (src->next != NULL) {
+ src = src->next;
+ qp->stream->next_in =
+ rte_pktmbuf_mtod(src, uint8_t *);
+ qp->stream->avail_in =
+ RTE_MIN(remaining_data, src->data_len);
+ } else {
+ ISAL_PMD_LOG(ERR,
+ "Not enough input buffer segments\n");
+ op->status = RTE_COMP_OP_STATUS_INVALID_ARGS;
+ return -1;
+ }
+ }
+
+ if (qp->stream->avail_out == 0 &&
+ qp->stream->internal_state.state != ZSTATE_END) {
+ if (dst->next != NULL) {
+ dst = dst->next;
+ qp->stream->next_out =
+ rte_pktmbuf_mtod(dst, uint8_t *);
+ qp->stream->avail_out = dst->data_len;
+ } else {
+ ISAL_PMD_LOG(ERR,
+ "Not enough output buffer segments\n");
+ op->status =
+ RTE_COMP_OP_STATUS_OUT_OF_SPACE_TERMINATED;
+ return -1;
+ }
+ }
+ }
+ op->consumed = qp->stream->total_in;
+ op->produced = qp->stream->total_out;
+
+ return 0;
+}
+
+/* Decompression using chained mbufs for input/output data */
+static int
+chained_mbuf_decompression(struct rte_comp_op *op, struct isal_comp_qp *qp)
+{
+ int ret;
+ uint32_t consumed_data, remaining_offset;
+ uint32_t remaining_data = op->src.length;
+ struct rte_mbuf *src = op->m_src;
+ struct rte_mbuf *dst = op->m_dst;
+
+ /* check for offset passing multiple segments
+ * and point decompression state to input/output buffer
+ */
+ if (op->src.offset > src->data_len) {
+ remaining_offset = op->src.offset;
+ while (remaining_offset > src->data_len) {
+ remaining_offset -= src->data_len;
+ src = src->next;
+ }
+
+ qp->state->avail_in = src->data_len - remaining_offset;
+ qp->state->next_in = rte_pktmbuf_mtod_offset(src,
+ uint8_t *, (op->src.offset % src->data_len));
+ } else {
+ qp->state->avail_in = src->data_len - op->src.offset;
+ qp->state->next_in = rte_pktmbuf_mtod_offset(src, uint8_t *,
+ op->src.offset);
+ }
+ if (op->dst.offset > dst->data_len) {
+ remaining_offset = op->dst.offset;
+ while (remaining_offset > dst->data_len) {
+ remaining_offset -= dst->data_len;
+ dst = dst->next;
+ }
+
+ qp->state->avail_out = dst->data_len - remaining_offset;
+ qp->state->next_out = rte_pktmbuf_mtod_offset(dst, uint8_t *,
+ (op->dst.offset % dst->data_len));
+ } else {
+ qp->state->avail_out = dst->data_len - op->dst.offset;
+ qp->state->next_out = rte_pktmbuf_mtod_offset(dst, uint8_t *,
+ op->dst.offset);
+ }
+ while (qp->state->block_state != ISAL_BLOCK_FINISH) {
+
+ ret = isal_inflate(qp->state);
+
+ if (remaining_data == op->src.length) {
+ consumed_data = src->data_len - qp->state->avail_in -
+ (op->src.offset % src->data_len);
+ } else
+ consumed_data = src->data_len - qp->state->avail_in;
+
+ op->consumed += consumed_data;
+ remaining_data -= consumed_data;
+
+ if (ret != ISAL_DECOMP_OK) {
+ ISAL_PMD_LOG(ERR, "Decompression operation failed\n");
+ op->status = RTE_COMP_OP_STATUS_ERROR;
+ return ret;
+ }
+
+ if (qp->state->avail_in == 0
+ && op->consumed != op->src.length) {
+ if (src->next != NULL) {
+ src = src->next;
+ qp->state->next_in =
+ rte_pktmbuf_mtod(src, uint8_t *);
+ qp->state->avail_in =
+ RTE_MIN(remaining_data, src->data_len);
+ }
+ }
+
+ if (qp->state->avail_out == 0 &&
+ qp->state->block_state != ISAL_BLOCK_FINISH) {
+ if (dst->next != NULL) {
+ dst = dst->next;
+ qp->state->next_out =
+ rte_pktmbuf_mtod(dst, uint8_t *);
+ qp->state->avail_out = dst->data_len;
+ } else {
+ ISAL_PMD_LOG(ERR,
+ "Not enough output buffer segments\n");
+ op->status =
+ RTE_COMP_OP_STATUS_OUT_OF_SPACE_TERMINATED;
+ return -1;
+ }
+ }
+ }
+ op->produced = qp->state->total_out;
+
+ return 0;
+}
+
/* Stateless Compression Function */
static int
process_isal_deflate(struct rte_comp_op *op, struct isal_comp_qp *qp,
@@ -207,7 +407,7 @@ process_isal_deflate(struct rte_comp_op *op, struct isal_comp_qp *qp,
/* Stateless operation, input will be consumed in one go */
qp->stream->flush = NO_FLUSH;
- /* set op level & intermediate level buffer */
+ /* set compression level & intermediate level buffer size */
qp->stream->level = priv_xform->compress.level;
qp->stream->level_buf_size = priv_xform->level_buffer_size;
@@ -225,59 +425,70 @@ process_isal_deflate(struct rte_comp_op *op, struct isal_comp_qp *qp,
isal_deflate_set_hufftables(qp->stream, NULL,
IGZIP_HUFFTABLE_DEFAULT);
- qp->stream->end_of_stream = 1; /* All input consumed in one go */
- if ((op->src.length + op->src.offset) > op->m_src->data_len) {
- ISAL_PMD_LOG(ERR, "Input mbuf not big enough for the length and"
- " offset provided.\n");
- op->status = RTE_COMP_OP_STATUS_INVALID_ARGS;
- return -1;
- }
- /* Point compression stream to input buffer */
- qp->stream->avail_in = op->src.length;
- qp->stream->next_in = rte_pktmbuf_mtod_offset(op->m_src, uint8_t *,
- op->src.offset);
-
- if (op->dst.offset > op->m_dst->data_len) {
- ISAL_PMD_LOG(ERR, "Output mbuf not big enough for the length"
- " and offset provided.\n");
+ if (op->m_src->pkt_len < (op->src.length + op->src.offset)) {
+ ISAL_PMD_LOG(ERR, "Input mbuf(s) not big enough.\n");
op->status = RTE_COMP_OP_STATUS_INVALID_ARGS;
return -1;
}
- /* Point compression stream to output buffer */
- qp->stream->avail_out = op->m_dst->data_len - op->dst.offset;
- qp->stream->next_out = rte_pktmbuf_mtod_offset(op->m_dst, uint8_t *,
- op->dst.offset);
- if (unlikely(!qp->stream->next_in || !qp->stream->next_out)) {
- ISAL_PMD_LOG(ERR, "Invalid source or destination buffers\n");
- op->status = RTE_COMP_OP_STATUS_INVALID_ARGS;
- return -1;
- }
+ /* Chained mbufs */
+ if (op->m_src->nb_segs > 1 || op->m_dst->nb_segs > 1) {
+ ret = chained_mbuf_compression(op, qp);
+ if (ret < 0)
+ return ret;
+ } else {
+ /* Linear buffer */
+ qp->stream->end_of_stream = 1; /* All input consumed in one */
+ /* Point compression stream to input buffer */
+ qp->stream->avail_in = op->src.length;
+ qp->stream->next_in = rte_pktmbuf_mtod_offset(op->m_src,
+ uint8_t *, op->src.offset);
+
+ if (op->dst.offset > op->m_dst->data_len) {
+ ISAL_PMD_LOG(ERR, "Output mbuf not big enough for "
+ "length and offset provided.\n");
+ op->status = RTE_COMP_OP_STATUS_INVALID_ARGS;
+ return -1;
+ }
+ /* Point compression stream to output buffer */
+ qp->stream->avail_out = op->m_dst->data_len - op->dst.offset;
+ qp->stream->next_out = rte_pktmbuf_mtod_offset(op->m_dst,
+ uint8_t *, op->dst.offset);
+
+ if (unlikely(!qp->stream->next_in || !qp->stream->next_out)) {
+ ISAL_PMD_LOG(ERR, "Invalid source or destination"
+ " buffers\n");
+ op->status = RTE_COMP_OP_STATUS_INVALID_ARGS;
+ return -1;
+ }
- /* Execute compression operation */
- ret = isal_deflate_stateless(qp->stream);
+ /* Execute compression operation */
+ ret = isal_deflate_stateless(qp->stream);
- /* Check that output buffer did not run out of space */
- if (ret == STATELESS_OVERFLOW) {
- ISAL_PMD_LOG(ERR, "Output buffer not big enough\n");
- op->status = RTE_COMP_OP_STATUS_OUT_OF_SPACE_TERMINATED;
- return ret;
- }
+ /* Check that output buffer did not run out of space */
+ if (ret == STATELESS_OVERFLOW) {
+ ISAL_PMD_LOG(ERR, "Output buffer not big enough\n");
+ op->status = RTE_COMP_OP_STATUS_OUT_OF_SPACE_TERMINATED;
+ return ret;
+ }
- /* Check that input buffer has been fully consumed */
- if (qp->stream->avail_in != (uint32_t)0) {
- ISAL_PMD_LOG(ERR, "Input buffer could not be read entirely\n");
- op->status = RTE_COMP_OP_STATUS_ERROR;
- return -1;
- }
+ /* Check that input buffer has been fully consumed */
+ if (qp->stream->avail_in != (uint32_t)0) {
+ ISAL_PMD_LOG(ERR, "Input buffer could not be read"
+ " entirely\n");
+ op->status = RTE_COMP_OP_STATUS_ERROR;
+ return -1;
+ }
- if (ret != COMP_OK) {
- op->status = RTE_COMP_OP_STATUS_ERROR;
- return ret;
- }
+ if (ret != COMP_OK) {
+ ISAL_PMD_LOG(ERR, "Compression operation failed\n");
+ op->status = RTE_COMP_OP_STATUS_ERROR;
+ return ret;
+ }
- op->consumed = qp->stream->total_in;
- op->produced = qp->stream->total_out;
+ op->consumed = qp->stream->total_in;
+ op->produced = qp->stream->total_out;
+ }
return ret;
}
@@ -293,59 +504,69 @@ process_isal_inflate(struct rte_comp_op *op, struct isal_comp_qp *qp)
/* Initialize decompression state */
isal_inflate_init(qp->state);
- if ((op->src.length + op->src.offset) > op->m_src->data_len) {
- ISAL_PMD_LOG(ERR, "Input mbuf not big enough for the length and"
- " offset provided.\n");
- op->status = RTE_COMP_OP_STATUS_INVALID_ARGS;
- return -1;
- }
- /* Point decompression state to input buffer */
- qp->state->avail_in = op->src.length;
- qp->state->next_in = rte_pktmbuf_mtod_offset(op->m_src, uint8_t *,
- op->src.offset);
-
- if (op->dst.offset > op->m_dst->data_len) {
- ISAL_PMD_LOG(ERR, "Output mbuf not big enough for the length "
- "and offset provided.\n");
+ if (op->m_src->pkt_len < (op->src.length + op->src.offset)) {
+ ISAL_PMD_LOG(ERR, "Input mbuf(s) not big enough.\n");
op->status = RTE_COMP_OP_STATUS_INVALID_ARGS;
return -1;
}
- /* Point decompression state to output buffer */
- qp->state->avail_out = op->m_dst->data_len - op->dst.offset;
- qp->state->next_out = rte_pktmbuf_mtod_offset(op->m_dst, uint8_t *,
- op->dst.offset);
- if (unlikely(!qp->state->next_in || !qp->state->next_out)) {
- ISAL_PMD_LOG(ERR, "Invalid source or destination buffers\n");
- op->status = RTE_COMP_OP_STATUS_INVALID_ARGS;
- return -1;
- }
+ /* Chained mbufs */
+ if (op->m_src->nb_segs > 1 || op->m_dst->nb_segs > 1) {
+ ret = chained_mbuf_decompression(op, qp);
+ if (ret != 0)
+ return ret;
+ } else {
+ /* Linear buffer */
+ /* Point decompression state to input buffer */
+ qp->state->avail_in = op->src.length;
+ qp->state->next_in = rte_pktmbuf_mtod_offset(op->m_src,
+ uint8_t *, op->src.offset);
+
+ if (op->dst.offset > op->m_dst->data_len) {
+ ISAL_PMD_LOG(ERR, "Output mbuf not big enough for "
+ "length and offset provided.\n");
+ op->status = RTE_COMP_OP_STATUS_INVALID_ARGS;
+ return -1;
+ }
+ /* Point decompression state to output buffer */
+ qp->state->avail_out = op->m_dst->data_len - op->dst.offset;
+ qp->state->next_out = rte_pktmbuf_mtod_offset(op->m_dst,
+ uint8_t *, op->dst.offset);
+
+ if (unlikely(!qp->state->next_in || !qp->state->next_out)) {
+ ISAL_PMD_LOG(ERR, "Invalid source or destination"
+ " buffers\n");
+ op->status = RTE_COMP_OP_STATUS_INVALID_ARGS;
+ return -1;
+ }
- /* Execute decompression operation */
- ret = isal_inflate_stateless(qp->state);
+ /* Execute decompression operation */
+ ret = isal_inflate_stateless(qp->state);
- if (ret == ISAL_OUT_OVERFLOW) {
- ISAL_PMD_LOG(ERR, "Output buffer not big enough\n");
- op->status = RTE_COMP_OP_STATUS_OUT_OF_SPACE_TERMINATED;
- return ret;
- }
+ if (ret == ISAL_OUT_OVERFLOW) {
+ ISAL_PMD_LOG(ERR, "Output buffer not big enough\n");
+ op->status = RTE_COMP_OP_STATUS_OUT_OF_SPACE_TERMINATED;
+ return ret;
+ }
- /* Check that input buffer has been fully consumed */
- if (qp->state->avail_in != (uint32_t)0) {
- ISAL_PMD_LOG(ERR, "Input buffer could not be read entirely\n");
- op->status = RTE_COMP_OP_STATUS_ERROR;
- return -1;
- }
+ /* Check that input buffer has been fully consumed */
+ if (qp->state->avail_in != (uint32_t)0) {
+ ISAL_PMD_LOG(ERR, "Input buffer could not be read"
+ " entirely\n");
+ op->status = RTE_COMP_OP_STATUS_ERROR;
+ return -1;
+ }
- if (ret != ISAL_DECOMP_OK) {
- op->status = RTE_COMP_OP_STATUS_ERROR;
- return ret;
- }
+ if (ret != ISAL_DECOMP_OK) {
+ op->status = RTE_COMP_OP_STATUS_ERROR;
+ return ret;
+ }
- op->consumed = op->src.length - qp->state->avail_in;
- op->produced = qp->state->total_out;
+ op->consumed = op->src.length - qp->state->avail_in;
+ op->produced = qp->state->total_out;
+ }
-return ret;
+ return ret;
}
/* Process compression/decompression operation */
diff --git a/drivers/compress/isal/isal_compress_pmd_ops.c b/drivers/compress/isal/isal_compress_pmd_ops.c
index c61acd4..41cade8 100644
--- a/drivers/compress/isal/isal_compress_pmd_ops.c
+++ b/drivers/compress/isal/isal_compress_pmd_ops.c
@@ -12,7 +12,10 @@
static const struct rte_compressdev_capabilities isal_pmd_capabilities[] = {
{
.algo = RTE_COMP_ALGO_DEFLATE,
- .comp_feature_flags = RTE_COMP_FF_SHAREABLE_PRIV_XFORM |
+ .comp_feature_flags = RTE_COMP_FF_OOP_SGL_IN_SGL_OUT |
+ RTE_COMP_FF_OOP_SGL_IN_LB_OUT |
+ RTE_COMP_FF_OOP_LB_IN_SGL_OUT |
+ RTE_COMP_FF_SHAREABLE_PRIV_XFORM |
RTE_COMP_FF_HUFFMAN_FIXED |
RTE_COMP_FF_HUFFMAN_DYNAMIC,
.window_size = {
--
2.7.4
^ permalink raw reply related
* Re: pull-request: can 2018-07-23
From: David Miller @ 2018-07-23 18:02 UTC (permalink / raw)
To: mkl; +Cc: netdev, linux-can, kernel
In-Reply-To: <20180723125843.391-1-mkl@pengutronix.de>
From: Marc Kleine-Budde <mkl@pengutronix.de>
Date: Mon, 23 Jul 2018 14:58:31 +0200
> this is a pull request of 12 patches for net/master.
>
> The patch by Stephane Grosjean for the peak_canfd CAN driver fixes a problem
> with older firmware. The next patch is by Roman Fietze and fixes the setup of
> the CCCR register in the m_can driver. Nicholas Mc Guire's patch for the
> mpc5xxx_can driver adds missing error checking. The two patches by Faiz Abbas
> fix the runtime resume and clean up the probe function in the m_can driver. The
> last 7 patches by Anssi Hannula fix several problem in the xilinx_can driver.
Pulled, thanks Marc.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.