* [PATCH 3/5] arm: dts: OpenPower Palmetto system can use coprocessor for FSI
From: Rob Herring @ 2018-08-24 0:43 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180724042406.15374-4-benh@kernel.crashing.org>
On Mon, Jul 23, 2018 at 11:25 PM Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> This switches away from userspace bitbanging to kernel FSI
> using the coprocessor.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts | 28 ++++++++++++++-----
> 1 file changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
> index c7084a819dc6..e6cfdf3c1a67 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
> @@ -26,6 +26,11 @@
> no-map;
> reg = <0x5f000000 0x01000000>; /* 16M */
> };
> +
> + coldfire_memory: codefire_memory at 5ee00000 {
typo
> + reg = <0x5ee00000 0x00200000>;
> + no-map;
> + };
> };
>
> leds {
> @@ -44,6 +49,22 @@
> };
> };
>
> + fsi: gpio-fsi {
> + compatible = "aspeed,ast2400-cf-fsi-master", "fsi-master";
> + #address-cells = <2>;
> + #size-cells = <0>;
> +
> + memory-region = <&coldfire_memory>;
> + aspeed,sram = <&sram>;
> + aspeed,cvic = <&cvic>;
> +
> + clock-gpios = <&gpio ASPEED_GPIO(A, 4) GPIO_ACTIVE_HIGH>;
> + data-gpios = <&gpio ASPEED_GPIO(A, 5) GPIO_ACTIVE_HIGH>;
> + mux-gpios = <&gpio ASPEED_GPIO(A, 6) GPIO_ACTIVE_HIGH>;
> + enable-gpios = <&gpio ASPEED_GPIO(D, 0) GPIO_ACTIVE_HIGH>;
> + trans-gpios = <&gpio ASPEED_GPIO(H, 6) GPIO_ACTIVE_HIGH>;
> + };
> +
> gpio-keys {
> compatible = "gpio-keys";
>
> @@ -303,13 +324,6 @@
> line-name = "SYS_PWROK_BMC";
> };
>
> - pin_gpio_h6 {
> - gpio-hog;
> - gpios = <ASPEED_GPIO(H, 6) GPIO_ACTIVE_HIGH>;
> - output-high;
> - line-name = "SCM1_FSI0_DATA_EN";
> - };
> -
> pin_gpio_h7 {
> gpio-hog;
> gpios = <ASPEED_GPIO(H, 7) GPIO_ACTIVE_HIGH>;
> --
> 2.17.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-08-23 22:57 UTC (permalink / raw)
To: linux-aspeed
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 more in multi-master
environment 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>
---
Changes since v5:
- Changed variable names in irq hanlders to represent proper meaning.
- Fixed an error printing message again to make it use the irq_handled variable.
Changes since v4:
- Fixed an error printing message that handlers didn't handle all interrupts.
Changes since v3:
- Fixed typos in a comment.
Changes since v2:
- Changed the name of ASPEED_I2CD_INTR_ERRORS to ASPEED_I2CD_INTR_MASTER_ERRORS
- Removed a member irq_status from the struct aspeed_i2c_bus and changed
master_irq and slave_irq handlers to make them return status_ack.
- Added a comment to explain why it needs to try both irq handlers.
Changes since v1:
- Fixed a grammar issue in commit message.
- Added a missing line feed character into a message printing.
drivers/i2c/busses/i2c-aspeed.c | 131 ++++++++++++++++++--------------
1 file changed, 76 insertions(+), 55 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index a4f956c6d567..c258c4d9a4c0 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_MASTER_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 | \
@@ -227,32 +232,26 @@ 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)
+static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
{
- u32 command, irq_status, status_ack = 0;
+ u32 command, irq_handled = 0;
struct i2c_client *slave = bus->slave;
- bool irq_handled = true;
u8 value;
- if (!slave) {
- irq_handled = false;
- goto out;
- }
+ if (!slave)
+ return 0;
command = readl(bus->base + ASPEED_I2C_CMD_REG);
- irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
/* Slave was requested, restart state machine. */
if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
- status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
+ irq_handled |= 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 irq_handled;
dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
irq_status, command);
@@ -269,31 +268,31 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
bus->slave_state =
ASPEED_I2C_SLAVE_WRITE_REQUESTED;
}
- status_ack |= ASPEED_I2CD_INTR_RX_DONE;
+ irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
}
/* Slave was asked to stop. */
if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
- status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
+ irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
- status_ack |= ASPEED_I2CD_INTR_TX_NAK;
+ irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
+ if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
+ irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
switch (bus->slave_state) {
case ASPEED_I2C_SLAVE_READ_REQUESTED:
if (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))
dev_err(bus->dev,
"Expected ACK after processed read.\n");
@@ -317,13 +316,6 @@ 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;
}
#endif /* CONFIG_I2C_SLAVE */
@@ -380,21 +372,21 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
return 0;
}
-static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
+static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
{
- u32 irq_status, status_ack = 0, command = 0;
+ u32 irq_handled = 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) {
bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
- status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
+ irq_handled |= 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;
}
/*
@@ -403,19 +395,22 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
* INACTIVE state.
*/
ret = aspeed_i2c_is_irq_error(irq_status);
- if (ret < 0) {
+ if (ret) {
dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
irq_status);
bus->cmd_err = ret;
bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
+ irq_handled |= (irq_status & ASPEED_I2CD_INTR_MASTER_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",
+ 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;
}
@@ -428,13 +423,18 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
*/
if (bus->master_state == ASPEED_I2C_MASTER_START) {
if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
+ if (unlikely(!(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\n", msg->addr);
- status_ack |= ASPEED_I2CD_INTR_TX_NAK;
+ irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
bus->cmd_err = -ENXIO;
aspeed_i2c_do_stop(bus);
goto out_no_complete;
}
- status_ack |= ASPEED_I2CD_INTR_TX_ACK;
+ irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
if (msg->len == 0) { /* SMBUS_QUICK */
aspeed_i2c_do_stop(bus);
goto out_no_complete;
@@ -449,13 +449,13 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
case ASPEED_I2C_MASTER_TX:
if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
dev_dbg(bus->dev, "slave NACKed TX\n");
- status_ack |= ASPEED_I2CD_INTR_TX_NAK;
+ irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
goto error_and_stop;
} else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
dev_err(bus->dev, "slave failed to ACK TX\n");
goto error_and_stop;
}
- status_ack |= ASPEED_I2CD_INTR_TX_ACK;
+ irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
/* fallthrough intended */
case ASPEED_I2C_MASTER_TX_FIRST:
if (bus->buf_index < msg->len) {
@@ -478,7 +478,7 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
dev_err(bus->dev, "master failed to RX\n");
goto error_and_stop;
}
- status_ack |= ASPEED_I2CD_INTR_RX_DONE;
+ irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
recv_byte = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
msg->buf[bus->buf_index++] = recv_byte;
@@ -506,11 +506,13 @@ 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");
+ dev_err(bus->dev,
+ "master failed to STOP. irq_status:0x%x\n",
+ irq_status);
bus->cmd_err = -EIO;
/* Do not STOP as we have already tried. */
} else {
- status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
+ irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
}
bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
@@ -540,33 +542,52 @@ 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;
+ return irq_handled;
}
static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
{
struct aspeed_i2c_bus *bus = dev_id;
- bool ret;
+ u32 irq_received, irq_remaining, irq_handled;
spin_lock(&bus->lock);
+ irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+ irq_remaining = 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;
+ /*
+ * In most cases, interrupt bits will be set one by one, although
+ * multiple interrupt bits could be set at the same time. It's also
+ * possible that master interrupt bits could be set along with slave
+ * interrupt bits. Each case needs to be handled using corresponding
+ * handlers depending on the current state.
+ */
+ if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
+ irq_handled = aspeed_i2c_master_irq(bus, irq_remaining);
+ irq_remaining &= ~irq_handled;
+ if (irq_remaining)
+ irq_handled |= aspeed_i2c_slave_irq(bus, irq_remaining);
+ } else {
+ irq_handled = aspeed_i2c_slave_irq(bus, irq_remaining);
+ irq_remaining &= ~irq_handled;
+ if (irq_remaining)
+ irq_handled |= aspeed_i2c_master_irq(bus,
+ irq_remaining);
}
+#else
+ irq_handled = aspeed_i2c_master_irq(bus, irq_remaining);
#endif /* CONFIG_I2C_SLAVE */
- ret = aspeed_i2c_master_irq(bus);
+ irq_remaining &= ~irq_handled;
+ if (irq_remaining)
+ dev_err(bus->dev,
+ "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
+ irq_received, irq_handled);
-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 irq_remaining ? IRQ_NONE : IRQ_HANDLED;
}
static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
--
2.18.0
^ permalink raw reply related
* [PATCH i2c-next v5] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-08-23 21:39 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CAFd5g457FQNwiprMmeGEaem8LTSObXuyifdxtN7LzS6k1PWmdg@mail.gmail.com>
On 8/23/2018 2:18 PM, Brendan Higgins wrote:
> On Thu, Aug 23, 2018 at 1:56 PM Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> 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 more in multi-master
>> environment 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>
>> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
>> ---
>> Changes since v4:
>> - Fixed an error printing message that handlers didn't handle all interrupts.
>>
>> Changes since v3:
>> - Fixed typos in a comment.
>>
>> Changes since v2:
>> - Changed the name of ASPEED_I2CD_INTR_ERRORS to ASPEED_I2CD_INTR_MASTER_ERRORS
>> - Removed a member irq_status from the struct aspeed_i2c_bus and changed
>> master_irq and slave_irq handlers to make them return status_ack.
>> - Added a comment to explain why it needs to try both irq handlers.
>>
>> Changes since v1:
>> - Fixed a grammar issue in commit message.
>> - Added a missing line feed character into a message printing.
>>
>> drivers/i2c/busses/i2c-aspeed.c | 115 +++++++++++++++++++-------------
>> 1 file changed, 70 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index a4f956c6d567..8341e384f4f1 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_MASTER_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 | \
>> @@ -227,20 +232,16 @@ 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)
>> +static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>> {
>> - 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 0;
>>
>> 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) {
>> @@ -249,10 +250,8 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
>> }
>>
>> /* 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 status_ack;
>>
>> dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
>> irq_status, command);
>> @@ -281,19 +280,19 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
>> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>> }
>> + if (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)
>> 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))
>> dev_err(bus->dev,
>> "Expected ACK after processed read.\n");
>> @@ -317,14 +316,7 @@ 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;
>> + return status_ack;
>> }
>> #endif /* CONFIG_I2C_SLAVE */
>>
>> @@ -380,21 +372,21 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
>> return 0;
>> }
>>
>> -static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> +static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>> {
>> - 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) {
>> 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;
>> }
>>
>> /*
>> @@ -403,19 +395,22 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> * INACTIVE state.
>> */
>> ret = aspeed_i2c_is_irq_error(irq_status);
>> - if (ret < 0) {
>> + if (ret) {
>> dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
>> irq_status);
>> bus->cmd_err = ret;
>> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> + status_ack |= (irq_status & ASPEED_I2CD_INTR_MASTER_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",
>> + 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;
>> }
>> @@ -428,6 +423,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> */
>> if (bus->master_state == ASPEED_I2C_MASTER_START) {
>> if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> + if (unlikely(!(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\n", msg->addr);
>> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> bus->cmd_err = -ENXIO;
>> @@ -506,7 +506,9 @@ 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");
>> + dev_err(bus->dev,
>> + "master failed to STOP. irq_status:0x%x\n",
>> + irq_status);
>> bus->cmd_err = -EIO;
>> /* Do not STOP as we have already tried. */
>> } else {
>> @@ -540,33 +542,56 @@ 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;
>> + return status_ack;
>> }
>>
>> static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>> {
>> struct aspeed_i2c_bus *bus = dev_id;
>> - bool ret;
>> + u32 irq_received, irq_status, irq_acked;
>
> Thinking more about how irq_status is used below, I think you can pick
> a better name, maybe `irq_bits_remaining` or `irq_status_remaining`?
>
> Come to think of it, `irq_acked` is a bit misleading too, maybe
> `irq_handled`? This might seem pedantic, but you ack the bits at the
> end of the function, so it does not actually contain acked bits.
>
> It also might make more sense to just have the `irq_handled` and
> `irq_received` fields, and then make the `irq_handled` cumulative.
> That way, anywhere you use the `irq_status` it is immediately obvious
> that you are comparing the received bits with the bits that have been
> handled up to this point. I am not 100% sure, what do you think?
>
I agree with you about naming of the variables. Will also check if it
can use a cumulative variable for the handled bits as you suggested.
Thanks!
>>
>> spin_lock(&bus->lock);
>> + irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> + 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;
>> + /*
>> + * In most cases, interrupt bits will be set one by one, although
>> + * multiple interrupt bits could be set at the same time. It's also
>> + * possible that master interrupt bits could be set along with slave
>> + * interrupt bits. Each case needs to be handled using corresponding
>> + * handlers depending on the current state.
>> + */
>> + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
>> + irq_acked = aspeed_i2c_master_irq(bus, irq_status);
>> + irq_status &= ~irq_acked;
>> + if (irq_status)
>> + irq_acked = aspeed_i2c_slave_irq(bus, irq_status);
>> + } else {
>> + irq_acked = aspeed_i2c_slave_irq(bus, irq_status);
>> + irq_status &= ~irq_acked;
>> + if (irq_status)
>> + irq_acked = aspeed_i2c_master_irq(bus, irq_status);
>> }
>> +#else
>> + irq_acked = aspeed_i2c_master_irq(bus, irq_status);
>> #endif /* CONFIG_I2C_SLAVE */
>>
>> - ret = aspeed_i2c_master_irq(bus);
>> + irq_status &= ~irq_acked;
>> + if (irq_status) {
>> + /*
>> + * 'irq_received ^ irq_status' indicates accumulated bits acked
>> + * by both master and slave irq handlers.
>> + */
>> + dev_err(bus->dev,
>> + "irq acked != irq. expected 0x%08x, but was 0x%08x\n",
>> + irq_received, irq_received ^ irq_status);
>
> My point was that you are still using the `irq_received ^ irq_status`, right?
>
> If you want a cumulative irq_acked, you can either or the first
> instance of irq_acked with the following, or take a not of irq_status.
> Xoring with a mask is not immediately clear.
>
Okay. I'll check your suggestions and will replace the 'irq_received ^
irq_status' use.
>> + }
>>
>> -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 irq_status ? IRQ_NONE : IRQ_HANDLED;
>> }
>>
>> static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
>> --
>> 2.18.0
>>
>
> Cheers
>
Thanks for your review. I'll submit v6 without adding your tag. Please
review and comment the patch then.
Thanks!
^ permalink raw reply
* [PATCH i2c-next v5] i2c: aspeed: Handle master/slave combined irq events properly
From: Brendan Higgins @ 2018-08-23 21:18 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180823205647.12417-1-jae.hyun.yoo@linux.intel.com>
On Thu, Aug 23, 2018 at 1:56 PM Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> 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 more in multi-master
> environment 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>
> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
> ---
> Changes since v4:
> - Fixed an error printing message that handlers didn't handle all interrupts.
>
> Changes since v3:
> - Fixed typos in a comment.
>
> Changes since v2:
> - Changed the name of ASPEED_I2CD_INTR_ERRORS to ASPEED_I2CD_INTR_MASTER_ERRORS
> - Removed a member irq_status from the struct aspeed_i2c_bus and changed
> master_irq and slave_irq handlers to make them return status_ack.
> - Added a comment to explain why it needs to try both irq handlers.
>
> Changes since v1:
> - Fixed a grammar issue in commit message.
> - Added a missing line feed character into a message printing.
>
> drivers/i2c/busses/i2c-aspeed.c | 115 +++++++++++++++++++-------------
> 1 file changed, 70 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index a4f956c6d567..8341e384f4f1 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_MASTER_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 | \
> @@ -227,20 +232,16 @@ 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)
> +static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
> {
> - 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 0;
>
> 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) {
> @@ -249,10 +250,8 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> }
>
> /* 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 status_ack;
>
> dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
> irq_status, command);
> @@ -281,19 +280,19 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> }
> + if (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)
> 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))
> dev_err(bus->dev,
> "Expected ACK after processed read.\n");
> @@ -317,14 +316,7 @@ 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;
> + return status_ack;
> }
> #endif /* CONFIG_I2C_SLAVE */
>
> @@ -380,21 +372,21 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
> return 0;
> }
>
> -static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> +static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
> {
> - 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) {
> 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;
> }
>
> /*
> @@ -403,19 +395,22 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> * INACTIVE state.
> */
> ret = aspeed_i2c_is_irq_error(irq_status);
> - if (ret < 0) {
> + if (ret) {
> dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
> irq_status);
> bus->cmd_err = ret;
> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> + status_ack |= (irq_status & ASPEED_I2CD_INTR_MASTER_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",
> + 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;
> }
> @@ -428,6 +423,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> */
> if (bus->master_state == ASPEED_I2C_MASTER_START) {
> if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> + if (unlikely(!(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\n", msg->addr);
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> bus->cmd_err = -ENXIO;
> @@ -506,7 +506,9 @@ 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");
> + dev_err(bus->dev,
> + "master failed to STOP. irq_status:0x%x\n",
> + irq_status);
> bus->cmd_err = -EIO;
> /* Do not STOP as we have already tried. */
> } else {
> @@ -540,33 +542,56 @@ 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;
> + return status_ack;
> }
>
> static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> {
> struct aspeed_i2c_bus *bus = dev_id;
> - bool ret;
> + u32 irq_received, irq_status, irq_acked;
Thinking more about how irq_status is used below, I think you can pick
a better name, maybe `irq_bits_remaining` or `irq_status_remaining`?
Come to think of it, `irq_acked` is a bit misleading too, maybe
`irq_handled`? This might seem pedantic, but you ack the bits at the
end of the function, so it does not actually contain acked bits.
It also might make more sense to just have the `irq_handled` and
`irq_received` fields, and then make the `irq_handled` cumulative.
That way, anywhere you use the `irq_status` it is immediately obvious
that you are comparing the received bits with the bits that have been
handled up to this point. I am not 100% sure, what do you think?
>
> spin_lock(&bus->lock);
> + irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> + 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;
> + /*
> + * In most cases, interrupt bits will be set one by one, although
> + * multiple interrupt bits could be set at the same time. It's also
> + * possible that master interrupt bits could be set along with slave
> + * interrupt bits. Each case needs to be handled using corresponding
> + * handlers depending on the current state.
> + */
> + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
> + irq_acked = aspeed_i2c_master_irq(bus, irq_status);
> + irq_status &= ~irq_acked;
> + if (irq_status)
> + irq_acked = aspeed_i2c_slave_irq(bus, irq_status);
> + } else {
> + irq_acked = aspeed_i2c_slave_irq(bus, irq_status);
> + irq_status &= ~irq_acked;
> + if (irq_status)
> + irq_acked = aspeed_i2c_master_irq(bus, irq_status);
> }
> +#else
> + irq_acked = aspeed_i2c_master_irq(bus, irq_status);
> #endif /* CONFIG_I2C_SLAVE */
>
> - ret = aspeed_i2c_master_irq(bus);
> + irq_status &= ~irq_acked;
> + if (irq_status) {
> + /*
> + * 'irq_received ^ irq_status' indicates accumulated bits acked
> + * by both master and slave irq handlers.
> + */
> + dev_err(bus->dev,
> + "irq acked != irq. expected 0x%08x, but was 0x%08x\n",
> + irq_received, irq_received ^ irq_status);
My point was that you are still using the `irq_received ^ irq_status`, right?
If you want a cumulative irq_acked, you can either or the first
instance of irq_acked with the following, or take a not of irq_status.
Xoring with a mask is not immediately clear.
> + }
>
> -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 irq_status ? IRQ_NONE : IRQ_HANDLED;
> }
>
> static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
> --
> 2.18.0
>
Cheers
^ permalink raw reply
* [PATCH i2c-next v5] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-08-23 20:56 UTC (permalink / raw)
To: linux-aspeed
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 more in multi-master
environment 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>
Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
---
Changes since v4:
- Fixed an error printing message that handlers didn't handle all interrupts.
Changes since v3:
- Fixed typos in a comment.
Changes since v2:
- Changed the name of ASPEED_I2CD_INTR_ERRORS to ASPEED_I2CD_INTR_MASTER_ERRORS
- Removed a member irq_status from the struct aspeed_i2c_bus and changed
master_irq and slave_irq handlers to make them return status_ack.
- Added a comment to explain why it needs to try both irq handlers.
Changes since v1:
- Fixed a grammar issue in commit message.
- Added a missing line feed character into a message printing.
drivers/i2c/busses/i2c-aspeed.c | 115 +++++++++++++++++++-------------
1 file changed, 70 insertions(+), 45 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index a4f956c6d567..8341e384f4f1 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_MASTER_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 | \
@@ -227,20 +232,16 @@ 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)
+static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
{
- 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 0;
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) {
@@ -249,10 +250,8 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
}
/* 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 status_ack;
dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
irq_status, command);
@@ -281,19 +280,19 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
status_ack |= ASPEED_I2CD_INTR_TX_NAK;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
+ if (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)
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))
dev_err(bus->dev,
"Expected ACK after processed read.\n");
@@ -317,14 +316,7 @@ 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;
+ return status_ack;
}
#endif /* CONFIG_I2C_SLAVE */
@@ -380,21 +372,21 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
return 0;
}
-static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
+static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
{
- 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) {
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;
}
/*
@@ -403,19 +395,22 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
* INACTIVE state.
*/
ret = aspeed_i2c_is_irq_error(irq_status);
- if (ret < 0) {
+ if (ret) {
dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
irq_status);
bus->cmd_err = ret;
bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
+ status_ack |= (irq_status & ASPEED_I2CD_INTR_MASTER_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",
+ 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;
}
@@ -428,6 +423,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
*/
if (bus->master_state == ASPEED_I2C_MASTER_START) {
if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
+ if (unlikely(!(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\n", msg->addr);
status_ack |= ASPEED_I2CD_INTR_TX_NAK;
bus->cmd_err = -ENXIO;
@@ -506,7 +506,9 @@ 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");
+ dev_err(bus->dev,
+ "master failed to STOP. irq_status:0x%x\n",
+ irq_status);
bus->cmd_err = -EIO;
/* Do not STOP as we have already tried. */
} else {
@@ -540,33 +542,56 @@ 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;
+ return status_ack;
}
static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
{
struct aspeed_i2c_bus *bus = dev_id;
- bool ret;
+ u32 irq_received, irq_status, irq_acked;
spin_lock(&bus->lock);
+ irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+ 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;
+ /*
+ * In most cases, interrupt bits will be set one by one, although
+ * multiple interrupt bits could be set at the same time. It's also
+ * possible that master interrupt bits could be set along with slave
+ * interrupt bits. Each case needs to be handled using corresponding
+ * handlers depending on the current state.
+ */
+ if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
+ irq_acked = aspeed_i2c_master_irq(bus, irq_status);
+ irq_status &= ~irq_acked;
+ if (irq_status)
+ irq_acked = aspeed_i2c_slave_irq(bus, irq_status);
+ } else {
+ irq_acked = aspeed_i2c_slave_irq(bus, irq_status);
+ irq_status &= ~irq_acked;
+ if (irq_status)
+ irq_acked = aspeed_i2c_master_irq(bus, irq_status);
}
+#else
+ irq_acked = aspeed_i2c_master_irq(bus, irq_status);
#endif /* CONFIG_I2C_SLAVE */
- ret = aspeed_i2c_master_irq(bus);
+ irq_status &= ~irq_acked;
+ if (irq_status) {
+ /*
+ * 'irq_received ^ irq_status' indicates accumulated bits acked
+ * by both master and slave irq handlers.
+ */
+ dev_err(bus->dev,
+ "irq acked != irq. expected 0x%08x, but was 0x%08x\n",
+ irq_received, irq_received ^ 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 irq_status ? IRQ_NONE : IRQ_HANDLED;
}
static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
--
2.18.0
^ permalink raw reply related
* [PATCH i2c-next v4] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-08-23 18:29 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CAFd5g477PagwugRic6ihC-v3mFaewBbsvGHXRw+kPF5w+-n3iA@mail.gmail.com>
Hi Brendan,
On 8/22/2018 11:46 PM, Brendan Higgins wrote:
> On Mon, Aug 20, 2018 at 1:16 PM Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> 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 more in multi-master
>> environment 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>
>> ---
>> Changes since v3:
>> - Fixed typos in a comment.
>>
>> Changes since v2:
>> - Changed the name of ASPEED_I2CD_INTR_ERRORS to ASPEED_I2CD_INTR_MASTER_ERRORS
>> - Removed a member irq_status from the struct aspeed_i2c_bus and changed
>> master_irq and slave_irq handlers to make them return status_ack.
>> - Added a comment to explain why it needs to try both irq handlers.
>>
>> Changes since v1:
>> - Fixed a grammar issue in commit message.
>> - Added a missing line feed character into a message printing.
>>
>> drivers/i2c/busses/i2c-aspeed.c | 110 +++++++++++++++++++-------------
>> 1 file changed, 65 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index a4f956c6d567..09eb4a9b355c 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_MASTER_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 | \
>> @@ -227,20 +232,16 @@ 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)
>> +static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>> {
>> - 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 0;
>>
>> 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) {
>> @@ -249,10 +250,8 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
>> }
>>
>> /* 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 status_ack;
>>
>> dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
>> irq_status, command);
>> @@ -281,19 +280,19 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
>> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>> }
>> + if (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)
>> 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))
>> dev_err(bus->dev,
>> "Expected ACK after processed read.\n");
>> @@ -317,14 +316,7 @@ 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;
>> + return status_ack;
>> }
>> #endif /* CONFIG_I2C_SLAVE */
>>
>> @@ -380,21 +372,21 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
>> return 0;
>> }
>>
>> -static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> +static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>> {
>> - 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) {
>> 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;
>> }
>>
>> /*
>> @@ -403,19 +395,22 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> * INACTIVE state.
>> */
>> ret = aspeed_i2c_is_irq_error(irq_status);
>> - if (ret < 0) {
>> + if (ret) {
>> dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
>> irq_status);
>> bus->cmd_err = ret;
>> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> + status_ack |= (irq_status & ASPEED_I2CD_INTR_MASTER_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",
>> + 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;
>> }
>> @@ -428,6 +423,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> */
>> if (bus->master_state == ASPEED_I2C_MASTER_START) {
>> if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> + if (unlikely(!(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\n", msg->addr);
>> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> bus->cmd_err = -ENXIO;
>> @@ -506,7 +506,9 @@ 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");
>> + dev_err(bus->dev,
>> + "master failed to STOP irq_status:0x%x\n",
>> + irq_status);
>> bus->cmd_err = -EIO;
>> /* Do not STOP as we have already tried. */
>> } else {
>> @@ -540,33 +542,51 @@ 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;
>> + return status_ack;
>> }
>>
>> static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>> {
>> struct aspeed_i2c_bus *bus = dev_id;
>> - bool ret;
>> + u32 irq_received, irq_status, irq_acked;
>>
>> spin_lock(&bus->lock);
>> + irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> + 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;
>> + /*
>> + * In most cases, interrupt bits will be set one by one, although
>> + * multiple interrupt bits could be set at the same time. It's also
>> + * possible that master interrupt bits could be set along with slave
>> + * interrupt bits. Each case needs to be handled using corresponding
>> + * handlers depending on the current state.
>> + */
>> + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
>> + irq_acked = aspeed_i2c_master_irq(bus, irq_status);
>> + irq_status &= ~irq_acked;
>> + if (irq_status)
>> + irq_acked = aspeed_i2c_slave_irq(bus, irq_status);
>> + } else {
>> + irq_acked = aspeed_i2c_slave_irq(bus, irq_status);
>> + irq_status &= ~irq_acked;
>> + if (irq_status)
>> + irq_acked = aspeed_i2c_master_irq(bus, irq_status);
>> }
>> +#else
>> + irq_acked = aspeed_i2c_master_irq(bus, irq_status);
>> #endif /* CONFIG_I2C_SLAVE */
>>
>> - ret = aspeed_i2c_master_irq(bus);
>> + irq_status &= ~irq_acked;
>> + if (irq_status)
>> + dev_err(bus->dev,
>> + "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>> + irq_received, irq_received ^ irq_status);
>
> I think you want to use `irq_acked` here.
>
Yes, right. I'll fix the message printing. Thanks for your pointing it out.
>>
>> -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 irq_status ? IRQ_NONE : IRQ_HANDLED;
>> }
>>
>> static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
>> --
>> 2.18.0
>>
>
> One minor issue and then I think we are good to go!
>
> Also, sorry about not reviewing your last patch. I missed it apparently.
>
> Feel free to mark the next patch "Reviewed-by: Brendan Higgins
> <brendanhiggins@google.com>"
>
> Cheers
>
Thanks a lot for your review. I really appreciate it. Will submit the v5
patch soon.
Jae
^ permalink raw reply
* [PATCH i2c-next v4] i2c: aspeed: Handle master/slave combined irq events properly
From: Brendan Higgins @ 2018-08-23 6:46 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180820201632.29229-1-jae.hyun.yoo@linux.intel.com>
On Mon, Aug 20, 2018 at 1:16 PM Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> 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 more in multi-master
> environment 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>
> ---
> Changes since v3:
> - Fixed typos in a comment.
>
> Changes since v2:
> - Changed the name of ASPEED_I2CD_INTR_ERRORS to ASPEED_I2CD_INTR_MASTER_ERRORS
> - Removed a member irq_status from the struct aspeed_i2c_bus and changed
> master_irq and slave_irq handlers to make them return status_ack.
> - Added a comment to explain why it needs to try both irq handlers.
>
> Changes since v1:
> - Fixed a grammar issue in commit message.
> - Added a missing line feed character into a message printing.
>
> drivers/i2c/busses/i2c-aspeed.c | 110 +++++++++++++++++++-------------
> 1 file changed, 65 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index a4f956c6d567..09eb4a9b355c 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_MASTER_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 | \
> @@ -227,20 +232,16 @@ 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)
> +static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
> {
> - 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 0;
>
> 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) {
> @@ -249,10 +250,8 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> }
>
> /* 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 status_ack;
>
> dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
> irq_status, command);
> @@ -281,19 +280,19 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> }
> + if (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)
> 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))
> dev_err(bus->dev,
> "Expected ACK after processed read.\n");
> @@ -317,14 +316,7 @@ 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;
> + return status_ack;
> }
> #endif /* CONFIG_I2C_SLAVE */
>
> @@ -380,21 +372,21 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
> return 0;
> }
>
> -static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> +static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
> {
> - 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) {
> 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;
> }
>
> /*
> @@ -403,19 +395,22 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> * INACTIVE state.
> */
> ret = aspeed_i2c_is_irq_error(irq_status);
> - if (ret < 0) {
> + if (ret) {
> dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
> irq_status);
> bus->cmd_err = ret;
> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> + status_ack |= (irq_status & ASPEED_I2CD_INTR_MASTER_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",
> + 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;
> }
> @@ -428,6 +423,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> */
> if (bus->master_state == ASPEED_I2C_MASTER_START) {
> if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> + if (unlikely(!(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\n", msg->addr);
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> bus->cmd_err = -ENXIO;
> @@ -506,7 +506,9 @@ 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");
> + dev_err(bus->dev,
> + "master failed to STOP irq_status:0x%x\n",
> + irq_status);
> bus->cmd_err = -EIO;
> /* Do not STOP as we have already tried. */
> } else {
> @@ -540,33 +542,51 @@ 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;
> + return status_ack;
> }
>
> static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> {
> struct aspeed_i2c_bus *bus = dev_id;
> - bool ret;
> + u32 irq_received, irq_status, irq_acked;
>
> spin_lock(&bus->lock);
> + irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> + 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;
> + /*
> + * In most cases, interrupt bits will be set one by one, although
> + * multiple interrupt bits could be set at the same time. It's also
> + * possible that master interrupt bits could be set along with slave
> + * interrupt bits. Each case needs to be handled using corresponding
> + * handlers depending on the current state.
> + */
> + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
> + irq_acked = aspeed_i2c_master_irq(bus, irq_status);
> + irq_status &= ~irq_acked;
> + if (irq_status)
> + irq_acked = aspeed_i2c_slave_irq(bus, irq_status);
> + } else {
> + irq_acked = aspeed_i2c_slave_irq(bus, irq_status);
> + irq_status &= ~irq_acked;
> + if (irq_status)
> + irq_acked = aspeed_i2c_master_irq(bus, irq_status);
> }
> +#else
> + irq_acked = aspeed_i2c_master_irq(bus, irq_status);
> #endif /* CONFIG_I2C_SLAVE */
>
> - ret = aspeed_i2c_master_irq(bus);
> + irq_status &= ~irq_acked;
> + if (irq_status)
> + dev_err(bus->dev,
> + "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> + irq_received, irq_received ^ irq_status);
I think you want to use `irq_acked` here.
>
> -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 irq_status ? IRQ_NONE : IRQ_HANDLED;
> }
>
> static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
> --
> 2.18.0
>
One minor issue and then I think we are good to go!
Also, sorry about not reviewing your last patch. I missed it apparently.
Feel free to mark the next patch "Reviewed-by: Brendan Higgins
<brendanhiggins@google.com>"
Cheers
^ permalink raw reply
* [PATCH 2/2] ARM: dts: aspeed: quanta-q7l1: Add four PSUs
From: Patrick Venture @ 2018-08-22 17:12 UTC (permalink / raw)
To: linux-aspeed
Enable the four PSUs via generic PMBUS.
Signed-off-by: Patrick Venture <venture@google.com>
---
arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dts | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dts b/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dts
index 586050e1c0e6..c655564e6c6d 100644
--- a/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dts
@@ -318,24 +318,44 @@
#address-cells = <1>;
#size-cells = <0>;
reg = <0>;
+
+ psu at 59 {
+ compatible = "pmbus";
+ reg = <0x59>;
+ };
};
i2c_psu1: i2c at 1 {
#address-cells = <1>;
#size-cells = <0>;
reg = <1>;
+
+ psu at 58 {
+ compatible = "pmbus";
+ reg = <0x58>;
+ };
};
i2c_psu3: i2c at 2 {
#address-cells = <1>;
#size-cells = <0>;
reg = <2>;
+
+ psu at 58 {
+ compatible = "pmbus";
+ reg = <0x58>;
+ };
};
i2c_psu2: i2c at 3 {
#address-cells = <1>;
#size-cells = <0>;
reg = <3>;
+
+ psu at 59 {
+ compatible = "pmbus";
+ reg = <0x59>;
+ };
};
};
--
2.18.0.1017.ga543ac7ca45-goog
^ permalink raw reply related
* [PATCH 1/2] ARM: dts: aspeed: quanta-q71l: add aliases for i2c
From: Patrick Venture @ 2018-08-22 16:30 UTC (permalink / raw)
To: linux-aspeed
Provide aliases to each i2c bus per labels added for
each PCIe slot, etc, that are downstream beyond a mux.
Signed-off-by: Patrick Venture <venture@google.com>
---
arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dts | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dts b/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dts
index 76aa6ea1f988..586050e1c0e6 100644
--- a/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dts
@@ -7,6 +7,25 @@
model = "Quanta Q71L BMC";
compatible = "quanta,q71l-bmc", "aspeed,ast2400";
+ aliases {
+ i2c14 = &i2c_pcie2;
+ i2c15 = &i2c_pcie3;
+ i2c16 = &i2c_pcie6;
+ i2c17 = &i2c_pcie7;
+ i2c18 = &i2c_pcie1;
+ i2c19 = &i2c_pcie4;
+ i2c20 = &i2c_pcie5;
+ i2c21 = &i2c_pcie8;
+ i2c22 = &i2c_pcie9;
+ i2c23 = &i2c_pcie10;
+ i2c24 = &i2c_ssd1;
+ i2c25 = &i2c_ssd2;
+ i2c26 = &i2c_psu4;
+ i2c27 = &i2c_psu1;
+ i2c28 = &i2c_psu3;
+ i2c29 = &i2c_psu2;
+ };
+
chosen {
stdout-path = &uart5;
bootargs = "console=ttyS4,115200 earlyprintk";
--
2.18.0.1017.ga543ac7ca45-goog
^ permalink raw reply related
* [PATCH i2c-next v4] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-08-20 20:16 UTC (permalink / raw)
To: linux-aspeed
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 more in multi-master
environment 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>
---
Changes since v3:
- Fixed typos in a comment.
Changes since v2:
- Changed the name of ASPEED_I2CD_INTR_ERRORS to ASPEED_I2CD_INTR_MASTER_ERRORS
- Removed a member irq_status from the struct aspeed_i2c_bus and changed
master_irq and slave_irq handlers to make them return status_ack.
- Added a comment to explain why it needs to try both irq handlers.
Changes since v1:
- Fixed a grammar issue in commit message.
- Added a missing line feed character into a message printing.
drivers/i2c/busses/i2c-aspeed.c | 110 +++++++++++++++++++-------------
1 file changed, 65 insertions(+), 45 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index a4f956c6d567..09eb4a9b355c 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_MASTER_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 | \
@@ -227,20 +232,16 @@ 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)
+static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
{
- 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 0;
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) {
@@ -249,10 +250,8 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
}
/* 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 status_ack;
dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
irq_status, command);
@@ -281,19 +280,19 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
status_ack |= ASPEED_I2CD_INTR_TX_NAK;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
+ if (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)
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))
dev_err(bus->dev,
"Expected ACK after processed read.\n");
@@ -317,14 +316,7 @@ 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;
+ return status_ack;
}
#endif /* CONFIG_I2C_SLAVE */
@@ -380,21 +372,21 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
return 0;
}
-static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
+static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
{
- 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) {
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;
}
/*
@@ -403,19 +395,22 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
* INACTIVE state.
*/
ret = aspeed_i2c_is_irq_error(irq_status);
- if (ret < 0) {
+ if (ret) {
dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
irq_status);
bus->cmd_err = ret;
bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
+ status_ack |= (irq_status & ASPEED_I2CD_INTR_MASTER_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",
+ 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;
}
@@ -428,6 +423,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
*/
if (bus->master_state == ASPEED_I2C_MASTER_START) {
if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
+ if (unlikely(!(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\n", msg->addr);
status_ack |= ASPEED_I2CD_INTR_TX_NAK;
bus->cmd_err = -ENXIO;
@@ -506,7 +506,9 @@ 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");
+ dev_err(bus->dev,
+ "master failed to STOP irq_status:0x%x\n",
+ irq_status);
bus->cmd_err = -EIO;
/* Do not STOP as we have already tried. */
} else {
@@ -540,33 +542,51 @@ 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;
+ return status_ack;
}
static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
{
struct aspeed_i2c_bus *bus = dev_id;
- bool ret;
+ u32 irq_received, irq_status, irq_acked;
spin_lock(&bus->lock);
+ irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+ 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;
+ /*
+ * In most cases, interrupt bits will be set one by one, although
+ * multiple interrupt bits could be set at the same time. It's also
+ * possible that master interrupt bits could be set along with slave
+ * interrupt bits. Each case needs to be handled using corresponding
+ * handlers depending on the current state.
+ */
+ if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
+ irq_acked = aspeed_i2c_master_irq(bus, irq_status);
+ irq_status &= ~irq_acked;
+ if (irq_status)
+ irq_acked = aspeed_i2c_slave_irq(bus, irq_status);
+ } else {
+ irq_acked = aspeed_i2c_slave_irq(bus, irq_status);
+ irq_status &= ~irq_acked;
+ if (irq_status)
+ irq_acked = aspeed_i2c_master_irq(bus, irq_status);
}
+#else
+ irq_acked = aspeed_i2c_master_irq(bus, irq_status);
#endif /* CONFIG_I2C_SLAVE */
- ret = aspeed_i2c_master_irq(bus);
+ irq_status &= ~irq_acked;
+ if (irq_status)
+ dev_err(bus->dev,
+ "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
+ irq_received, irq_received ^ 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 irq_status ? IRQ_NONE : IRQ_HANDLED;
}
static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
--
2.18.0
^ permalink raw reply related
* [RFC 1/2] dt-bindings: media: Add Aspeed Video Engine binding documentation
From: Eddie James @ 2018-08-16 21:14 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1534448601-74120-2-git-send-email-eajames@linux.vnet.ibm.com>
On 08/16/2018 02:43 PM, Eddie James wrote:
> Document the bindings.
>
> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
> ---
> .../devicetree/bindings/media/aspeed-video.txt | 25 ++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/aspeed-video.txt
>
> diff --git a/Documentation/devicetree/bindings/media/aspeed-video.txt b/Documentation/devicetree/bindings/media/aspeed-video.txt
> new file mode 100644
> index 0000000..91a494e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/aspeed-video.txt
> @@ -0,0 +1,25 @@
> +* Device tree bindings for Aspeed Video Engine
> +
> +The Video Engine (VE) embedded in the Aspeed AST2400 and AST2500 SOCs can
> +capture and compress video data from digital or analog sources.
> +
> +Required properties:
> + - compatible: "aspeed,ast2400-video" or "aspeed,ast2500-video"
> + - reg: contains the offset and length of the VE memory region
> + - clocks: pointers to the the "vclk" and "eclk" of the syscon
> + - clock-names: "vclk-gate", "eclk-gate"
> + - resets: pointer to the VE reset of the syscon
> + - interrupts: the interrupt associated with the VE on this platform
> + - reg-scu: pointer to the syscon itself
Forgot to remove syscon reference in latest, it's not needed.
Thanks,
Eddie
> +
> +Example:
> +
> +video: video at 1e700000 {
> + compatible = "aspeed,ast2500-video";
> + reg = <0x1e700000 0x20000>;
> + clocks = <&syscon ASPEED_CLK_GATE_VCLK>, <&syscon ASPEED_CLK_GATE_ECLK>;
> + clock-names = "vclk-gate", "eclk-gate";
> + resets = <&syscon ASPEED_RESET_VIDEO>;
> + interrupts = <7>;
> + reg-scu = <&syscon>;
> +};
^ permalink raw reply
* [RFC 2/2] media: platform: Add Aspeed Video Engine driver
From: Eddie James @ 2018-08-16 19:43 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1534448601-74120-1-git-send-email-eajames@linux.vnet.ibm.com>
The Video Engine (VE) embedded in the Aspeed AST2400 and AST2500 SOCs
can capture and compress video data from digital or analog sources. With
the Aspeed chip acting a service processor, the Video Engine can capture
the host processor graphics output.
Add a V4L2 driver to capture video data and compress it to JPEG images,
making the data available through a standard read interface.
Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
---
drivers/media/platform/Kconfig | 8 +
drivers/media/platform/Makefile | 1 +
drivers/media/platform/aspeed-video.c | 1307 +++++++++++++++++++++++++++++++++
3 files changed, 1316 insertions(+)
create mode 100644 drivers/media/platform/aspeed-video.c
diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index e307943..1e8e69f 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -32,6 +32,14 @@ source "drivers/media/platform/davinci/Kconfig"
source "drivers/media/platform/omap/Kconfig"
+config VIDEO_ASPEED
+ tristate "Aspeed AST2400 and AST2500 Video Engine driver"
+ depends on VIDEO_V4L2
+ help
+ Support for the Aspeed Video Engine (VE) embedded in the Aspeed
+ AST2400 and AST2500 SOCs. The VE can capture and compress video data
+ from digital or analog sources.
+
config VIDEO_SH_VOU
tristate "SuperH VOU video output driver"
depends on MEDIA_CAMERA_SUPPORT
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 41322ab..205c33a 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -3,6 +3,7 @@
# Makefile for the video capture/playback device drivers.
#
+obj-$(CONFIG_VIDEO_ASPEED) += aspeed-video.o
obj-$(CONFIG_VIDEO_CADENCE) += cadence/
obj-$(CONFIG_VIDEO_VIA_CAMERA) += via-camera.o
obj-$(CONFIG_VIDEO_CAFE_CCIC) += marvell-ccic/
diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
new file mode 100644
index 0000000..4f36a3f
--- /dev/null
+++ b/drivers/media/platform/aspeed-video.c
@@ -0,0 +1,1307 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * ASPEED Video Engine Driver
+ *
+ * Copyright 2018 IBM Corp.
+ *
+ * Eddie James <eajames@linux.vnet.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/atomic.h>
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/interrupt.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/sched.h>
+#include <linux/spinlock.h>
+#include <linux/string.h>
+#include <linux/videodev2.h>
+#include <linux/wait.h>
+#include <linux/workqueue.h>
+#include <media/v4l2-dev.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-ioctl.h>
+
+#define DEVICE_NAME "aspeed-video"
+
+#define ASPEED_VIDEO_JPEG_NUM_QUALITIES 12
+#define ASPEED_VIDEO_JPEG_HEADER_SIZE 10
+#define ASPEED_VIDEO_JPEG_QUANT_SIZE 116
+#define ASPEED_VIDEO_JPEG_DCT_SIZE 34
+
+#define NUM_POLARITY_CHECKS 10
+#define INVALID_RESOLUTION_RETRIES 1
+#define INVALID_RESOLUTION_DELAY msecs_to_jiffies(250)
+#define RESOLUTION_CHANGE_DELAY msecs_to_jiffies(500)
+#define MODE_DETECT_TIMEOUT msecs_to_jiffies(500)
+#define DIRECT_FETCH_THRESHOLD 0x0c0000 /* 1024 * 768, 32bpp */
+
+#define VE_SRC_BUFFER_SIZE 0x900000 /* 1920 * 1200, 32bpp */
+#define VE_COMP_BUFFER_SIZE 0x100000 /* 128K packet * 8 packets */
+#define VE_JPEG_BUFFER_SIZE 0x006000 /* 512 * 12 * 4 */
+
+#define VE_PROTECTION_KEY 0x000
+#define VE_PROTECTION_KEY_UNLOCK 0x1A038AA8
+
+#define VE_SEQ_CTRL 0x004
+#define VE_SEQ_CTRL_TRIG_MODE_DET BIT(0)
+#define VE_SEQ_CTRL_TRIG_CAPTURE BIT(1)
+#define VE_SEQ_CTRL_FORCE_IDLE BIT(2)
+#define VE_SEQ_CTRL_MULT_FRAME BIT(3)
+#define VE_SEQ_CTRL_TRIG_COMP BIT(4)
+#define VE_SEQ_CTRL_AUTO_COMP BIT(5)
+#define VE_SEQ_CTRL_EN_WATCHDOG BIT(7)
+#define VE_SEQ_CTRL_YUV420 BIT(10)
+#define VE_SEQ_CTRL_COMP_FMT GENMASK(11, 10)
+#define VE_SEQ_CTRL_HALT BIT(12)
+#define VE_SEQ_CTRL_EN_WATCHDOG_COMP BIT(14)
+#define VE_SEQ_CTRL_TRIG_JPG BIT(15)
+#define VE_SEQ_CTRL_CAP_BUSY BIT(16)
+#define VE_SEQ_CTRL_COMP_BUSY BIT(18)
+
+#ifdef CONFIG_MACH_ASPEED_G5
+#define VE_SEQ_CTRL_JPEG_MODE BIT(13) /* AST2500 */
+#else
+#define VE_SEQ_CTRL_JPEG_MODE BIT(8) /* AST2400 */
+#endif /* CONFIG_MACH_ASPEED_G5 */
+
+#define VE_CTRL 0x008
+#define VE_CTRL_HSYNC_POL BIT(0)
+#define VE_CTRL_VSYNC_POL BIT(1)
+#define VE_CTRL_SOURCE BIT(2)
+#define VE_CTRL_INT_DE BIT(4)
+#define VE_CTRL_DIRECT_FETCH BIT(5)
+#define VE_CTRL_YUV BIT(6)
+#define VE_CTRL_RGB BIT(7)
+#define VE_CTRL_CAPTURE_FMT GENMASK(7, 6)
+#define VE_CTRL_AUTO_OR_CURSOR BIT(8)
+#define VE_CTRL_CLK_INVERSE BIT(11)
+#define VE_CTRL_CLK_DELAY GENMASK(11, 9)
+#define VE_CTRL_INTERLACE BIT(14)
+#define VE_CTRL_HSYNC_POL_CTRL BIT(15)
+#define VE_CTRL_FRC GENMASK(23, 16)
+
+#define VE_TGS_0 0x00c
+#define VE_TGS_1 0x010
+#define VE_TGS_FIRST GENMASK(28, 16)
+#define VE_TGS_LAST GENMASK(12, 0)
+
+#define VE_SCALING_FACTOR 0x014
+#define VE_SCALING_FILTER0 0x018
+#define VE_SCALING_FILTER1 0x01c
+#define VE_SCALING_FILTER2 0x020
+#define VE_SCALING_FILTER3 0x024
+
+#define VE_CAP_WINDOW 0x030
+#define VE_COMP_WINDOW 0x034
+#define VE_COMP_PROC_OFFSET 0x038
+#define VE_COMP_OFFSET 0x03c
+#define VE_JPEG_ADDR 0x040
+#define VE_SRC0_ADDR 0x044
+#define VE_SRC_SCANLINE_OFFSET 0x048
+#define VE_SRC1_ADDR 0x04c
+#define VE_COMP_ADDR 0x054
+
+#define VE_STREAM_BUF_SIZE 0x058
+#define VE_STREAM_BUF_SIZE_N_PACKETS GENMASK(5, 3)
+#define VE_STREAM_BUF_SIZE_P_SIZE GENMASK(2, 0)
+
+#define VE_COMP_CTRL 0x060
+#define VE_COMP_CTRL_VQ_DCT_ONLY BIT(0)
+#define VE_COMP_CTRL_VQ_4COLOR BIT(1)
+#define VE_COMP_CTRL_QUANTIZE BIT(2)
+#define VE_COMP_CTRL_EN_BQ BIT(4)
+#define VE_COMP_CTRL_EN_CRYPTO BIT(5)
+#define VE_COMP_CTRL_DCT_CHR GENMASK(10, 6)
+#define VE_COMP_CTRL_DCT_LUM GENMASK(15, 11)
+#define VE_COMP_CTRL_EN_HQ BIT(16)
+#define VE_COMP_CTRL_RSVD BIT(19)
+#define VE_COMP_CTRL_ENCODE GENMASK(21, 20)
+#define VE_COMP_CTRL_HQ_DCT_CHR GENMASK(26, 22)
+#define VE_COMP_CTRL_HQ_DCT_LUM GENMASK(31, 27)
+
+#define VE_OFFSET_COMP_STREAM 0x078
+
+#define VE_SRC_LR_EDGE_DET 0x090
+#define VE_SRC_LR_EDGE_DET_LEFT GENMASK(11, 0)
+#define VE_SRC_LR_EDGE_DET_NO_V BIT(12)
+#define VE_SRC_LR_EDGE_DET_NO_H BIT(13)
+#define VE_SRC_LR_EDGE_DET_NO_DISP BIT(14)
+#define VE_SRC_LR_EDGE_DET_NO_CLK BIT(15)
+#define VE_SRC_LR_EDGE_DET_RT_SHF 16
+#define VE_SRC_LR_EDGE_DET_RT GENMASK(27, VE_SRC_LR_EDGE_DET_RT_SHF)
+#define VE_SRC_LR_EDGE_DET_INTERLACE BIT(31)
+
+#define VE_SRC_TB_EDGE_DET 0x094
+#define VE_SRC_TB_EDGE_DET_TOP GENMASK(12, 0)
+#define VE_SRC_TB_EDGE_DET_BOT_SHF 16
+#define VE_SRC_TB_EDGE_DET_BOT GENMASK(28, VE_SRC_TB_EDGE_DET_BOT_SHF)
+
+#define VE_MODE_DETECT_STATUS 0x098
+#define VE_MODE_DETECT_STATUS_VSYNC BIT(28)
+#define VE_MODE_DETECT_STATUS_HSYNC BIT(29)
+
+#define VE_INTERRUPT_CTRL 0x304
+#define VE_INTERRUPT_STATUS 0x308
+#define VE_INTERRUPT_MODE_DETECT_WD BIT(0)
+#define VE_INTERRUPT_CAPTURE_COMPLETE BIT(1)
+#define VE_INTERRUPT_COMP_READY BIT(2)
+#define VE_INTERRUPT_COMP_COMPLETE BIT(3)
+#define VE_INTERRUPT_MODE_DETECT BIT(4)
+#define VE_INTERRUPT_FRAME_COMPLETE BIT(5)
+#define VE_INTERRUPT_DECODE_ERR BIT(6)
+#define VE_INTERRUPT_HALT_READY BIT(8)
+#define VE_INTERRUPT_HANG_WD BIT(9)
+#define VE_INTERRUPT_STREAM_DESC BIT(10)
+#define VE_INTERRUPT_VSYNC_DESC BIT(11)
+
+#define VE_MODE_DETECT 0x30c
+#define VE_MEM_RESTRICT_START 0x310
+#define VE_MEM_RESTRICT_END 0x314
+
+enum {
+ VIDEO_MODE_DETECT_DONE,
+ VIDEO_RES_CHANGE,
+ VIDEO_FRAME_AVAILABLE,
+ VIDEO_FRAME_TRIGGERED,
+};
+
+struct aspeed_video_addr {
+ dma_addr_t dma;
+ void *virt;
+};
+
+struct aspeed_video {
+ void __iomem *base;
+ struct clk *eclk;
+ struct clk *vclk;
+ struct reset_control *rst;
+
+ struct device *dev;
+ struct v4l2_device v4l2_dev;
+ struct video_device vdev;
+ struct mutex video_lock;
+
+ atomic_t clients;
+ wait_queue_head_t wait;
+ struct delayed_work res_work;
+ unsigned long flags;
+
+ int frame_idx;
+ u32 frame_size;
+
+ dma_addr_t max;
+ dma_addr_t min;
+ struct aspeed_video_addr srcs[2];
+ struct aspeed_video_addr comp[2];
+ struct aspeed_video_addr jpeg;
+
+ int frame_rate;
+ int jpeg_quality;
+ struct v4l2_pix_format fmt;
+};
+
+#define to_aspeed_video(x) container_of((x), struct aspeed_video, v4l2_dev)
+
+static const u32 aspeed_video_jpeg_header[ASPEED_VIDEO_JPEG_HEADER_SIZE] = {
+ 0xE0FFD8FF, 0x464A1000, 0x01004649, 0x60000101, 0x00006000, 0x0F00FEFF,
+ 0x00002D05, 0x00000000, 0x00000000, 0x00DBFF00
+};
+
+static const u32 aspeed_video_jpeg_quant[ASPEED_VIDEO_JPEG_QUANT_SIZE] = {
+ 0x081100C0, 0x00000000, 0x00110103, 0x03011102, 0xC4FF0111, 0x00001F00,
+ 0x01010501, 0x01010101, 0x00000000, 0x00000000, 0x04030201, 0x08070605,
+ 0xFF0B0A09, 0x10B500C4, 0x03010200, 0x03040203, 0x04040505, 0x7D010000,
+ 0x00030201, 0x12051104, 0x06413121, 0x07615113, 0x32147122, 0x08A19181,
+ 0xC1B14223, 0xF0D15215, 0x72623324, 0x160A0982, 0x1A191817, 0x28272625,
+ 0x35342A29, 0x39383736, 0x4544433A, 0x49484746, 0x5554534A, 0x59585756,
+ 0x6564635A, 0x69686766, 0x7574736A, 0x79787776, 0x8584837A, 0x89888786,
+ 0x9493928A, 0x98979695, 0xA3A29A99, 0xA7A6A5A4, 0xB2AAA9A8, 0xB6B5B4B3,
+ 0xBAB9B8B7, 0xC5C4C3C2, 0xC9C8C7C6, 0xD4D3D2CA, 0xD8D7D6D5, 0xE2E1DAD9,
+ 0xE6E5E4E3, 0xEAE9E8E7, 0xF4F3F2F1, 0xF8F7F6F5, 0xC4FFFAF9, 0x00011F00,
+ 0x01010103, 0x01010101, 0x00000101, 0x00000000, 0x04030201, 0x08070605,
+ 0xFF0B0A09, 0x11B500C4, 0x02010200, 0x04030404, 0x04040507, 0x77020100,
+ 0x03020100, 0x21050411, 0x41120631, 0x71610751, 0x81322213, 0x91421408,
+ 0x09C1B1A1, 0xF0523323, 0xD1726215, 0x3424160A, 0x17F125E1, 0x261A1918,
+ 0x2A292827, 0x38373635, 0x44433A39, 0x48474645, 0x54534A49, 0x58575655,
+ 0x64635A59, 0x68676665, 0x74736A69, 0x78777675, 0x83827A79, 0x87868584,
+ 0x928A8988, 0x96959493, 0x9A999897, 0xA5A4A3A2, 0xA9A8A7A6, 0xB4B3B2AA,
+ 0xB8B7B6B5, 0xC3C2BAB9, 0xC7C6C5C4, 0xD2CAC9C8, 0xD6D5D4D3, 0xDAD9D8D7,
+ 0xE5E4E3E2, 0xE9E8E7E6, 0xF4F3F2EA, 0xF8F7F6F5, 0xDAFFFAF9, 0x01030C00,
+ 0x03110200, 0x003F0011
+};
+
+static const u32 aspeed_video_jpeg_dct[ASPEED_VIDEO_JPEG_NUM_QUALITIES]
+ [ASPEED_VIDEO_JPEG_DCT_SIZE] = {
+ { 0x0D140043, 0x0C0F110F, 0x11101114, 0x17141516, 0x1E20321E,
+ 0x3D1E1B1B, 0x32242E2B, 0x4B4C3F48, 0x44463F47, 0x61735A50,
+ 0x566C5550, 0x88644644, 0x7A766C65, 0x4D808280, 0x8C978D60,
+ 0x7E73967D, 0xDBFF7B80, 0x1F014300, 0x272D2121, 0x3030582D,
+ 0x697BB958, 0xB8B9B97B, 0xB9B8A6A6, 0xB9B9B9B9, 0xB9B9B9B9,
+ 0xB9B9B9B9, 0xB9B9B9B9, 0xB9B9B9B9, 0xB9B9B9B9, 0xB9B9B9B9,
+ 0xB9B9B9B9, 0xB9B9B9B9, 0xB9B9B9B9, 0xFFB9B9B9 },
+ { 0x0C110043, 0x0A0D0F0D, 0x0F0E0F11, 0x14111213, 0x1A1C2B1A,
+ 0x351A1818, 0x2B1F2826, 0x4142373F, 0x3C3D373E, 0x55644E46,
+ 0x4B5F4A46, 0x77573D3C, 0x6B675F58, 0x43707170, 0x7A847B54,
+ 0x6E64836D, 0xDBFF6C70, 0x1B014300, 0x22271D1D, 0x2A2A4C27,
+ 0x5B6BA04C, 0xA0A0A06B, 0xA0A0A0A0, 0xA0A0A0A0, 0xA0A0A0A0,
+ 0xA0A0A0A0, 0xA0A0A0A0, 0xA0A0A0A0, 0xA0A0A0A0, 0xA0A0A0A0,
+ 0xA0A0A0A0, 0xA0A0A0A0, 0xA0A0A0A0, 0xFFA0A0A0 },
+ { 0x090E0043, 0x090A0C0A, 0x0C0B0C0E, 0x110E0F10, 0x15172415,
+ 0x2C151313, 0x241A211F, 0x36372E34, 0x31322E33, 0x4653413A,
+ 0x3E4E3D3A, 0x62483231, 0x58564E49, 0x385D5E5D, 0x656D6645,
+ 0x5B536C5A, 0xDBFF595D, 0x16014300, 0x1C201818, 0x22223F20,
+ 0x4B58853F, 0x85858558, 0x85858585, 0x85858585, 0x85858585,
+ 0x85858585, 0x85858585, 0x85858585, 0x85858585, 0x85858585,
+ 0x85858585, 0x85858585, 0x85858585, 0xFF858585 },
+ { 0x070B0043, 0x07080A08, 0x0A090A0B, 0x0D0B0C0C, 0x11121C11,
+ 0x23110F0F, 0x1C141A19, 0x2B2B2429, 0x27282428, 0x3842332E,
+ 0x313E302E, 0x4E392827, 0x46443E3A, 0x2C4A4A4A, 0x50565137,
+ 0x48425647, 0xDBFF474A, 0x12014300, 0x161A1313, 0x1C1C331A,
+ 0x3D486C33, 0x6C6C6C48, 0x6C6C6C6C, 0x6C6C6C6C, 0x6C6C6C6C,
+ 0x6C6C6C6C, 0x6C6C6C6C, 0x6C6C6C6C, 0x6C6C6C6C, 0x6C6C6C6C,
+ 0x6C6C6C6C, 0x6C6C6C6C, 0x6C6C6C6C, 0xFF6C6C6C },
+ { 0x06090043, 0x05060706, 0x07070709, 0x0A09090A, 0x0D0E160D,
+ 0x1B0D0C0C, 0x16101413, 0x21221C20, 0x1E1F1C20, 0x2B332824,
+ 0x26302624, 0x3D2D1F1E, 0x3735302D, 0x22393A39, 0x3F443F2B,
+ 0x38334338, 0xDBFF3739, 0x0D014300, 0x11130E0E, 0x15152613,
+ 0x2D355026, 0x50505035, 0x50505050, 0x50505050, 0x50505050,
+ 0x50505050, 0x50505050, 0x50505050, 0x50505050, 0x50505050,
+ 0x50505050, 0x50505050, 0x50505050, 0xFF505050 },
+ { 0x04060043, 0x03040504, 0x05040506, 0x07060606, 0x09090F09,
+ 0x12090808, 0x0F0A0D0D, 0x16161315, 0x14151315, 0x1D221B18,
+ 0x19201918, 0x281E1514, 0x2423201E, 0x17262726, 0x2A2D2A1C,
+ 0x25222D25, 0xDBFF2526, 0x09014300, 0x0B0D0A0A, 0x0E0E1A0D,
+ 0x1F25371A, 0x37373725, 0x37373737, 0x37373737, 0x37373737,
+ 0x37373737, 0x37373737, 0x37373737, 0x37373737, 0x37373737,
+ 0x37373737, 0x37373737, 0x37373737, 0xFF373737 },
+ { 0x02030043, 0x01020202, 0x02020203, 0x03030303, 0x04040704,
+ 0x09040404, 0x07050606, 0x0B0B090A, 0x0A0A090A, 0x0E110D0C,
+ 0x0C100C0C, 0x140F0A0A, 0x1211100F, 0x0B131313, 0x1516150E,
+ 0x12111612, 0xDBFF1213, 0x04014300, 0x05060505, 0x07070D06,
+ 0x0F121B0D, 0x1B1B1B12, 0x1B1B1B1B, 0x1B1B1B1B, 0x1B1B1B1B,
+ 0x1B1B1B1B, 0x1B1B1B1B, 0x1B1B1B1B, 0x1B1B1B1B, 0x1B1B1B1B,
+ 0x1B1B1B1B, 0x1B1B1B1B, 0x1B1B1B1B, 0xFF1B1B1B },
+ { 0x01020043, 0x01010101, 0x01010102, 0x02020202, 0x03030503,
+ 0x06030202, 0x05030404, 0x07070607, 0x06070607, 0x090B0908,
+ 0x080A0808, 0x0D0A0706, 0x0C0B0A0A, 0x070C0D0C, 0x0E0F0E09,
+ 0x0C0B0F0C, 0xDBFF0C0C, 0x03014300, 0x03040303, 0x04040804,
+ 0x0A0C1208, 0x1212120C, 0x12121212, 0x12121212, 0x12121212,
+ 0x12121212, 0x12121212, 0x12121212, 0x12121212, 0x12121212,
+ 0x12121212, 0x12121212, 0x12121212, 0xFF121212 },
+ { 0x01020043, 0x01010101, 0x01010102, 0x02020202, 0x03030503,
+ 0x06030202, 0x05030404, 0x07070607, 0x06070607, 0x090B0908,
+ 0x080A0808, 0x0D0A0706, 0x0C0B0A0A, 0x070C0D0C, 0x0E0F0E09,
+ 0x0C0B0F0C, 0xDBFF0C0C, 0x02014300, 0x03030202, 0x04040703,
+ 0x080A0F07, 0x0F0F0F0A, 0x0F0F0F0F, 0x0F0F0F0F, 0x0F0F0F0F,
+ 0x0F0F0F0F, 0x0F0F0F0F, 0x0F0F0F0F, 0x0F0F0F0F, 0x0F0F0F0F,
+ 0x0F0F0F0F, 0x0F0F0F0F, 0x0F0F0F0F, 0xFF0F0F0F },
+ { 0x01010043, 0x01010101, 0x01010101, 0x01010101, 0x02020302,
+ 0x04020202, 0x03020303, 0x05050405, 0x05050405, 0x07080606,
+ 0x06080606, 0x0A070505, 0x09080807, 0x05090909, 0x0A0B0A07,
+ 0x09080B09, 0xDBFF0909, 0x02014300, 0x02030202, 0x03030503,
+ 0x07080C05, 0x0C0C0C08, 0x0C0C0C0C, 0x0C0C0C0C, 0x0C0C0C0C,
+ 0x0C0C0C0C, 0x0C0C0C0C, 0x0C0C0C0C, 0x0C0C0C0C, 0x0C0C0C0C,
+ 0x0C0C0C0C, 0x0C0C0C0C, 0x0C0C0C0C, 0xFF0C0C0C },
+ { 0x01010043, 0x01010101, 0x01010101, 0x01010101, 0x01010201,
+ 0x03010101, 0x02010202, 0x03030303, 0x03030303, 0x04050404,
+ 0x04050404, 0x06050303, 0x06050505, 0x03060606, 0x07070704,
+ 0x06050706, 0xDBFF0606, 0x01014300, 0x01020101, 0x02020402,
+ 0x05060904, 0x09090906, 0x09090909, 0x09090909, 0x09090909,
+ 0x09090909, 0x09090909, 0x09090909, 0x09090909, 0x09090909,
+ 0x09090909, 0x09090909, 0x09090909, 0xFF090909 },
+ { 0x01010043, 0x01010101, 0x01010101, 0x01010101, 0x01010101,
+ 0x01010101, 0x01010101, 0x01010101, 0x01010101, 0x02020202,
+ 0x02020202, 0x03020101, 0x03020202, 0x01030303, 0x03030302,
+ 0x03020303, 0xDBFF0403, 0x01014300, 0x01010101, 0x01010201,
+ 0x03040602, 0x06060604, 0x06060606, 0x06060606, 0x06060606,
+ 0x06060606, 0x06060606, 0x06060606, 0x06060606, 0x06060606,
+ 0x06060606, 0x06060606, 0x06060606, 0xFF060606 }
+};
+
+static void aspeed_video_init_jpeg_table(u32 *table, bool yuv420)
+{
+ int i;
+ unsigned int base;
+
+ for (i = 0; i < ASPEED_VIDEO_JPEG_NUM_QUALITIES; i++) {
+ int j;
+
+ base = 256 * i; /* AST HW requires this header spacing */
+
+ for (j = 0; j < ASPEED_VIDEO_JPEG_HEADER_SIZE; j++)
+ table[base + j] =
+ le32_to_cpu(aspeed_video_jpeg_header[j]);
+
+ base += ASPEED_VIDEO_JPEG_HEADER_SIZE;
+ for (j = 0; j < ASPEED_VIDEO_JPEG_DCT_SIZE; j++)
+ table[base + j] =
+ le32_to_cpu(aspeed_video_jpeg_dct[i][j]);
+
+ base += ASPEED_VIDEO_JPEG_DCT_SIZE;
+ for (j = 0; j < ASPEED_VIDEO_JPEG_QUANT_SIZE; j++)
+ table[base + j] =
+ le32_to_cpu(aspeed_video_jpeg_quant[j]);
+
+ if (yuv420)
+ table[base + 2] = le32_to_cpu(0x00220103);
+ }
+}
+
+static void aspeed_video_update(struct aspeed_video *video, u32 reg,
+ unsigned long mask, u32 bits)
+{
+ u32 t = readl(video->base + reg);
+ u32 before = t;
+
+ t &= mask;
+ t |= bits;
+ writel(t, video->base + reg);
+ dev_dbg(video->dev, "update %03x[%08x -> %08x]\n", reg, before,
+ readl(video->base + reg));
+}
+
+static u32 aspeed_video_read(struct aspeed_video *video, u32 reg)
+{
+ u32 t = readl(video->base + reg);
+
+ dev_dbg(video->dev, "read %03x[%08x]\n", reg, t);
+ return t;
+}
+
+static void aspeed_video_write(struct aspeed_video *video, u32 reg, u32 val)
+{
+ writel(val, video->base + reg);
+ dev_dbg(video->dev, "write %03x[%08x]\n", reg,
+ readl(video->base + reg));
+}
+
+static bool aspeed_video_engine_busy(struct aspeed_video *video)
+{
+ u32 seq_ctrl = aspeed_video_read(video, VE_SEQ_CTRL);
+
+ if (!(seq_ctrl & VE_SEQ_CTRL_COMP_BUSY) ||
+ !(seq_ctrl & VE_SEQ_CTRL_CAP_BUSY)) {
+ dev_info(video->dev, "video engine busy\n");
+ return true;
+ }
+
+ return false;
+}
+
+static int aspeed_video_start_frame(struct aspeed_video *video)
+{
+ if (aspeed_video_engine_busy(video))
+ return -EBUSY;
+
+ video->frame_idx = (video->frame_idx + 1) % 2;
+
+ aspeed_video_write(video, VE_COMP_PROC_OFFSET, 0);
+ aspeed_video_write(video, VE_COMP_OFFSET, 0);
+ aspeed_video_write(video, VE_COMP_ADDR,
+ video->comp[video->frame_idx].dma);
+
+ set_bit(VIDEO_FRAME_TRIGGERED, &video->flags);
+
+ aspeed_video_update(video, VE_INTERRUPT_CTRL, 0xFFFFFFFF,
+ VE_INTERRUPT_COMP_COMPLETE |
+ VE_INTERRUPT_CAPTURE_COMPLETE);
+
+ aspeed_video_update(video, VE_SEQ_CTRL, 0xFFFFFFFF,
+ VE_SEQ_CTRL_TRIG_CAPTURE | VE_SEQ_CTRL_TRIG_COMP);
+
+ return 0;
+}
+
+static void aspeed_video_start_mode_detect(struct aspeed_video *video)
+{
+ /* Enable mode detect interrupts */
+ aspeed_video_update(video, VE_INTERRUPT_CTRL, 0xFFFFFFFF,
+ VE_INTERRUPT_MODE_DETECT);
+
+ /* Trigger mode detect */
+ aspeed_video_update(video, VE_SEQ_CTRL, 0xFFFFFFFF,
+ VE_SEQ_CTRL_TRIG_MODE_DET);
+}
+
+static void aspeed_video_disable_mode_detect(struct aspeed_video *video)
+{
+ /* Disable mode detect interrupts */
+ aspeed_video_update(video, VE_INTERRUPT_CTRL,
+ ~VE_INTERRUPT_MODE_DETECT, 0);
+
+ /* Disable mode detect */
+ aspeed_video_update(video, VE_SEQ_CTRL, ~VE_SEQ_CTRL_TRIG_MODE_DET, 0);
+}
+
+static void aspeed_video_off(struct aspeed_video *video)
+{
+ /* Reset the engine */
+ reset_control_assert(video->rst);
+ udelay(100);
+ reset_control_deassert(video->rst);
+
+ /* Turn off the relevant clocks */
+ clk_disable_unprepare(video->vclk);
+ clk_disable_unprepare(video->eclk);
+}
+
+static void aspeed_video_on(struct aspeed_video *video)
+{
+ /* Turn on the relevant clocks */
+ clk_prepare_enable(video->eclk);
+ clk_prepare_enable(video->vclk);
+
+ /* Reset the engine */
+ reset_control_assert(video->rst);
+ udelay(100);
+ reset_control_deassert(video->rst);
+}
+
+static irqreturn_t aspeed_video_irq(int irq, void *arg)
+{
+ struct aspeed_video *video = arg;
+ u32 sts = aspeed_video_read(video, VE_INTERRUPT_STATUS);
+
+ if (atomic_read(&video->clients) == 0) {
+ dev_info(video->dev, "irq with no client; disabling irqs\n");
+
+ aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
+ aspeed_video_write(video, VE_INTERRUPT_STATUS, 0xFFFFFFFF);
+ return IRQ_HANDLED;
+ }
+
+ /* Resolution changed; reset entire engine and reinitialize */
+ if (sts & VE_INTERRUPT_MODE_DETECT_WD) {
+ dev_info(video->dev, "resolution changed; resetting\n");
+ set_bit(VIDEO_RES_CHANGE, &video->flags);
+
+ aspeed_video_off(video);
+
+ schedule_delayed_work(&video->res_work,
+ RESOLUTION_CHANGE_DELAY);
+ return IRQ_HANDLED;
+ }
+
+ if (sts & VE_INTERRUPT_MODE_DETECT) {
+ aspeed_video_update(video, VE_INTERRUPT_CTRL,
+ ~VE_INTERRUPT_MODE_DETECT, 0);
+ aspeed_video_write(video, VE_INTERRUPT_STATUS,
+ VE_INTERRUPT_MODE_DETECT);
+
+ set_bit(VIDEO_MODE_DETECT_DONE, &video->flags);
+ wake_up_interruptible_all(&video->wait);
+ }
+
+ if ((sts & VE_INTERRUPT_COMP_COMPLETE) &&
+ (sts & VE_INTERRUPT_CAPTURE_COMPLETE)) {
+ video->frame_size = aspeed_video_read(video,
+ VE_OFFSET_COMP_STREAM);
+
+ aspeed_video_update(video, VE_INTERRUPT_CTRL,
+ ~(VE_INTERRUPT_COMP_COMPLETE |
+ VE_INTERRUPT_CAPTURE_COMPLETE), 0);
+ aspeed_video_write(video, VE_INTERRUPT_STATUS,
+ VE_INTERRUPT_COMP_COMPLETE |
+ VE_INTERRUPT_CAPTURE_COMPLETE);
+ aspeed_video_update(video, VE_SEQ_CTRL,
+ ~(VE_SEQ_CTRL_TRIG_CAPTURE |
+ VE_SEQ_CTRL_FORCE_IDLE |
+ VE_SEQ_CTRL_TRIG_COMP), 0);
+
+ set_bit(VIDEO_FRAME_AVAILABLE, &video->flags);
+ clear_bit(VIDEO_FRAME_TRIGGERED, &video->flags);
+ wake_up_interruptible_all(&video->wait);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static void aspeed_video_check_polarity(struct aspeed_video *video)
+{
+ int i;
+ int hsync_counter = 0;
+ int vsync_counter = 0;
+ u32 sts;
+
+ for (i = 0; i < NUM_POLARITY_CHECKS; ++i) {
+ sts = aspeed_video_read(video, VE_MODE_DETECT_STATUS);
+ if (sts & VE_MODE_DETECT_STATUS_VSYNC)
+ vsync_counter--;
+ else
+ vsync_counter++;
+
+ if (sts & VE_MODE_DETECT_STATUS_HSYNC)
+ hsync_counter--;
+ else
+ hsync_counter++;
+ }
+
+ if (hsync_counter < 0 || vsync_counter < 0) {
+ u32 ctrl;
+
+ if (hsync_counter < 0)
+ ctrl = VE_CTRL_HSYNC_POL;
+
+ if (vsync_counter < 0)
+ ctrl = VE_CTRL_VSYNC_POL;
+
+ aspeed_video_update(video, VE_CTRL, 0xFFFFFFFF, ctrl);
+ }
+}
+
+#define res_check(v) test_and_clear_bit(VIDEO_MODE_DETECT_DONE, &(v)->flags)
+
+static int aspeed_video_get_resolution(struct aspeed_video *video)
+{
+ bool invalid_resolution = true;
+ int rc;
+ int tries = 0;
+ unsigned int bottom;
+ unsigned int left;
+ unsigned int right;
+ unsigned int top;
+ u32 src_lr_edge;
+ u32 src_tb_edge;
+
+ video->fmt.width = 0;
+ video->fmt.height = 0;
+
+ do {
+ if (tries) {
+ set_current_state(TASK_INTERRUPTIBLE);
+ if (schedule_timeout(INVALID_RESOLUTION_DELAY))
+ return -EINTR;
+ }
+
+ aspeed_video_start_mode_detect(video);
+
+ rc = wait_event_interruptible_timeout(video->wait,
+ res_check(video),
+ MODE_DETECT_TIMEOUT);
+ if (!rc) {
+ dev_err(video->dev, "timed out on 1st mode detect\n");
+ aspeed_video_disable_mode_detect(video);
+ return -ETIME;
+ }
+
+ /* Disable mode detect in order to re-trigger */
+ aspeed_video_update(video, VE_SEQ_CTRL,
+ ~VE_SEQ_CTRL_TRIG_MODE_DET, 0);
+
+ aspeed_video_check_polarity(video);
+
+ aspeed_video_start_mode_detect(video);
+
+ rc = wait_event_interruptible_timeout(video->wait,
+ res_check(video),
+ MODE_DETECT_TIMEOUT);
+ if (!rc) {
+ dev_err(video->dev, "timed out on 2nd mode detect\n");
+ aspeed_video_disable_mode_detect(video);
+ return -ETIME;
+ }
+
+ src_lr_edge = aspeed_video_read(video, VE_SRC_LR_EDGE_DET);
+ src_tb_edge = aspeed_video_read(video, VE_SRC_TB_EDGE_DET);
+
+ bottom = (src_tb_edge & VE_SRC_TB_EDGE_DET_BOT) >>
+ VE_SRC_TB_EDGE_DET_BOT_SHF;
+ top = src_tb_edge & VE_SRC_TB_EDGE_DET_TOP;
+ if (top > bottom)
+ continue;
+
+ right = (src_lr_edge & VE_SRC_LR_EDGE_DET_RT) >>
+ VE_SRC_LR_EDGE_DET_RT_SHF;
+ left = src_lr_edge & VE_SRC_LR_EDGE_DET_LEFT;
+ if (left > right)
+ continue;
+
+ invalid_resolution = false;
+ } while (invalid_resolution && (tries++ < INVALID_RESOLUTION_RETRIES));
+
+ if (invalid_resolution) {
+ dev_err(video->dev, "invalid resolution detected\n");
+ return -EMSGSIZE;
+ }
+
+ video->fmt.height = (bottom - top) + 1;
+ video->fmt.width = (right - left) + 1;
+
+ /* Don't use direct mode below 1024 x 768 (irqs don't fire) */
+ if (video->fmt.height * video->fmt.width < DIRECT_FETCH_THRESHOLD) {
+ aspeed_video_write(video, VE_TGS_0,
+ FIELD_PREP(VE_TGS_FIRST, left - 1) |
+ FIELD_PREP(VE_TGS_LAST, right));
+ aspeed_video_write(video, VE_TGS_1,
+ FIELD_PREP(VE_TGS_FIRST, top) |
+ FIELD_PREP(VE_TGS_LAST, bottom + 1));
+ aspeed_video_update(video, VE_CTRL,
+ ~VE_CTRL_DIRECT_FETCH, VE_CTRL_INT_DE);
+ }
+
+ aspeed_video_write(video, VE_CAP_WINDOW,
+ video->fmt.width << 16 | video->fmt.height);
+ aspeed_video_write(video, VE_COMP_WINDOW,
+ video->fmt.width << 16 | video->fmt.height);
+ aspeed_video_write(video, VE_SRC_SCANLINE_OFFSET,
+ video->fmt.width * 4);
+
+ aspeed_video_update(video, VE_INTERRUPT_CTRL, 0xFFFFFFFF,
+ VE_INTERRUPT_MODE_DETECT_WD);
+ aspeed_video_update(video, VE_SEQ_CTRL, 0xFFFFFFFF,
+ VE_SEQ_CTRL_EN_WATCHDOG);
+
+ dev_dbg(video->dev, "got resolution[%dx%d]\n", video->fmt.width,
+ video->fmt.height);
+
+ return 0;
+}
+
+static void aspeed_video_init_regs(struct aspeed_video *video)
+{
+ u32 comp_ctrl = VE_COMP_CTRL_RSVD |
+ FIELD_PREP(VE_COMP_CTRL_DCT_LUM, video->jpeg_quality) |
+ FIELD_PREP(VE_COMP_CTRL_DCT_CHR, video->jpeg_quality | 0x10);
+ u32 ctrl = VE_CTRL_DIRECT_FETCH | VE_CTRL_AUTO_OR_CURSOR;
+ u32 seq_ctrl = VE_SEQ_CTRL_AUTO_COMP | VE_SEQ_CTRL_JPEG_MODE;
+
+ if (video->frame_rate)
+ ctrl |= FIELD_PREP(VE_CTRL_FRC, video->frame_rate);
+
+ if (video->fmt.pixelformat == V4L2_PIX_FMT_YUV420)
+ seq_ctrl |= VE_SEQ_CTRL_YUV420;
+
+ /* Unlock VE registers */
+ aspeed_video_write(video, VE_PROTECTION_KEY, VE_PROTECTION_KEY_UNLOCK);
+
+ /* Disable interrupts */
+ aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
+ aspeed_video_write(video, VE_INTERRUPT_STATUS, 0xFFFFFFFF);
+
+ /* Clear the offset */
+ aspeed_video_write(video, VE_COMP_PROC_OFFSET, 0);
+ aspeed_video_write(video, VE_COMP_OFFSET, 0);
+
+ /* Set memory restrictions */
+ aspeed_video_write(video, VE_MEM_RESTRICT_START, video->min);
+ aspeed_video_write(video, VE_MEM_RESTRICT_END, video->max);
+
+ /* Set buffer addresses */
+ aspeed_video_write(video, VE_SRC0_ADDR, video->srcs[0].dma);
+ aspeed_video_write(video, VE_SRC1_ADDR, video->srcs[1].dma);
+ aspeed_video_write(video, VE_COMP_ADDR, video->comp[0].dma);
+ aspeed_video_write(video, VE_JPEG_ADDR, video->jpeg.dma);
+
+ /* Set control registers */
+ aspeed_video_write(video, VE_SEQ_CTRL, seq_ctrl);
+ aspeed_video_write(video, VE_CTRL, ctrl);
+ aspeed_video_write(video, VE_COMP_CTRL, comp_ctrl);
+
+ /* Compression buffer size; 128K packet * 8 packets */
+ aspeed_video_write(video, VE_STREAM_BUF_SIZE, 0xf);
+
+ /* Don't downscale */
+ aspeed_video_write(video, VE_SCALING_FACTOR, 0x10001000);
+ aspeed_video_write(video, VE_SCALING_FILTER0, 0x00200000);
+ aspeed_video_write(video, VE_SCALING_FILTER1, 0x00200000);
+ aspeed_video_write(video, VE_SCALING_FILTER2, 0x00200000);
+ aspeed_video_write(video, VE_SCALING_FILTER3, 0x00200000);
+
+ /* Set mode detection defaults */
+ aspeed_video_write(video, VE_MODE_DETECT, 0x22666500);
+}
+
+static int aspeed_video_allocate_cma(struct aspeed_video *video)
+{
+ video->srcs[0].virt = dma_alloc_coherent(video->dev,
+ VE_SRC_BUFFER_SIZE,
+ &video->srcs[0].dma,
+ GFP_KERNEL);
+ if (!video->srcs[0].virt) {
+ dev_err(video->dev,
+ "Failed to allocate source buffer 0, size[%x]\n",
+ VE_SRC_BUFFER_SIZE);
+ goto err;
+ }
+
+ video->srcs[1].virt = dma_alloc_coherent(video->dev,
+ VE_SRC_BUFFER_SIZE,
+ &video->srcs[1].dma,
+ GFP_KERNEL);
+ if (!video->srcs[1].virt) {
+ dev_err(video->dev,
+ "Failed to allocate source buffer 1, size[%x]\n",
+ VE_SRC_BUFFER_SIZE);
+ goto free_src0;
+ }
+
+ video->comp[0].virt = dma_alloc_coherent(video->dev,
+ VE_COMP_BUFFER_SIZE,
+ &video->comp[0].dma,
+ GFP_KERNEL);
+ if (!video->comp[0].virt) {
+ dev_err(video->dev,
+ "Failed to allocate compression buffer 0, size[%x]\n",
+ VE_COMP_BUFFER_SIZE);
+ goto free_src1;
+ }
+
+ video->comp[1].virt = dma_alloc_coherent(video->dev,
+ VE_COMP_BUFFER_SIZE,
+ &video->comp[1].dma,
+ GFP_KERNEL);
+ if (!video->comp[0].virt) {
+ dev_err(video->dev,
+ "Failed to allocate compression buffer 1, size[%x]\n",
+ VE_COMP_BUFFER_SIZE);
+ goto free_comp0;
+ }
+
+ video->jpeg.virt = dma_alloc_coherent(video->dev, VE_JPEG_BUFFER_SIZE,
+ &video->jpeg.dma, GFP_KERNEL);
+ if (!video->jpeg.virt) {
+ dev_err(video->dev,
+ "Failed to allocate JPEG buffer, size[%x]\n",
+ VE_JPEG_BUFFER_SIZE);
+ goto free_comp1;
+ }
+
+ if (video->fmt.pixelformat == V4L2_PIX_FMT_YUV420)
+ aspeed_video_init_jpeg_table(video->jpeg.virt, true);
+ else
+ aspeed_video_init_jpeg_table(video->jpeg.virt, false);
+
+ /*
+ * Calculate the memory restrictions. Don't consider the JPEG header
+ * buffer since HW doesn't need to write to it.
+ */
+ video->max = max(video->srcs[0].dma + VE_SRC_BUFFER_SIZE,
+ video->srcs[1].dma + VE_SRC_BUFFER_SIZE);
+ video->max = max(video->max, video->comp[0].dma + VE_COMP_BUFFER_SIZE);
+ video->max = max(video->max, video->comp[1].dma + VE_COMP_BUFFER_SIZE);
+
+ video->min = min(video->srcs[0].dma, video->srcs[1].dma);
+ video->min = min(video->min, video->comp[0].dma);
+ video->min = min(video->min, video->comp[1].dma);
+
+ return 0;
+
+free_comp1:
+ dma_free_coherent(video->dev, VE_COMP_BUFFER_SIZE, video->comp[1].virt,
+ video->comp[1].dma);
+ video->comp[1].dma = 0ULL;
+ video->comp[1].virt = NULL;
+
+free_comp0:
+ dma_free_coherent(video->dev, VE_COMP_BUFFER_SIZE, video->comp[0].virt,
+ video->comp[0].dma);
+ video->comp[0].dma = 0ULL;
+ video->comp[0].virt = NULL;
+
+free_src1:
+ dma_free_coherent(video->dev, VE_SRC_BUFFER_SIZE, video->srcs[1].virt,
+ video->srcs[1].dma);
+ video->srcs[1].dma = 0ULL;
+ video->srcs[1].virt = NULL;
+
+free_src0:
+ dma_free_coherent(video->dev, VE_SRC_BUFFER_SIZE, video->srcs[0].virt,
+ video->srcs[0].dma);
+ video->srcs[0].dma = 0ULL;
+ video->srcs[0].virt = NULL;
+err:
+ return -ENOMEM;
+}
+
+static void aspeed_video_free_cma(struct aspeed_video *video)
+{
+ dma_free_coherent(video->dev, VE_JPEG_BUFFER_SIZE, video->jpeg.virt,
+ video->jpeg.dma);
+ dma_free_coherent(video->dev, VE_COMP_BUFFER_SIZE, video->comp[1].virt,
+ video->comp[1].dma);
+ dma_free_coherent(video->dev, VE_COMP_BUFFER_SIZE, video->comp[0].virt,
+ video->comp[0].dma);
+ dma_free_coherent(video->dev, VE_SRC_BUFFER_SIZE, video->srcs[1].virt,
+ video->srcs[1].dma);
+ dma_free_coherent(video->dev, VE_SRC_BUFFER_SIZE, video->srcs[0].virt,
+ video->srcs[0].dma);
+
+ video->srcs[0].dma = 0ULL;
+ video->srcs[0].virt = NULL;
+ video->srcs[1].dma = 0ULL;
+ video->srcs[1].virt = NULL;
+ video->comp[0].dma = 0ULL;
+ video->comp[0].virt = NULL;
+ video->comp[1].dma = 0ULL;
+ video->comp[1].virt = NULL;
+ video->jpeg.dma = 0ULL;
+ video->jpeg.virt = NULL;
+}
+
+static int aspeed_video_start(struct aspeed_video *video)
+{
+ int rc = aspeed_video_allocate_cma(video);
+
+ if (rc)
+ return rc;
+
+ aspeed_video_on(video);
+
+ aspeed_video_init_regs(video);
+
+ rc = aspeed_video_get_resolution(video);
+ if (rc)
+ aspeed_video_free_cma(video);
+
+ clear_bit(VIDEO_FRAME_TRIGGERED, &video->flags);
+
+ return rc;
+}
+
+static void aspeed_video_stop(struct aspeed_video *video)
+{
+ cancel_delayed_work_sync(&video->res_work);
+
+ aspeed_video_off(video);
+
+ aspeed_video_free_cma(video);
+
+ clear_bit(VIDEO_FRAME_AVAILABLE, &video->flags);
+}
+
+static int aspeed_video_querycap(struct file *file, void *fh,
+ struct v4l2_capability *cap)
+{
+ struct aspeed_video *video = video_drvdata(file);
+
+ strncpy(cap->driver, DEVICE_NAME, sizeof(cap->driver));
+ cap->capabilities = video->vdev.device_caps | V4L2_CAP_DEVICE_CAPS;
+
+ return 0;
+}
+
+static int aspeed_video_get_format(struct file *file, void *fh,
+ struct v4l2_format *f)
+{
+ int rc;
+ struct aspeed_video *video = video_drvdata(file);
+
+ if (test_bit(VIDEO_RES_CHANGE, &video->flags)) {
+ if (file->f_flags & O_NONBLOCK)
+ return -EAGAIN;
+
+ rc = wait_event_interruptible(video->wait,
+ !test_bit(VIDEO_RES_CHANGE,
+ &video->flags));
+ if (rc)
+ return -EINTR;
+ }
+
+ f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+ f->fmt.pix = video->fmt;
+
+ return 0;
+}
+
+static int aspeed_video_set_format(struct file *file, void *fh,
+ struct v4l2_format *f)
+{
+ struct aspeed_video *video = video_drvdata(file);
+
+ if (f->fmt.pix.pixelformat == video->fmt.pixelformat)
+ return 0;
+
+ if (f->fmt.pix.pixelformat == V4L2_PIX_FMT_YUV444) {
+ video->fmt.pixelformat = V4L2_PIX_FMT_YUV444;
+ aspeed_video_init_jpeg_table(video->jpeg.virt, false);
+ aspeed_video_update(video, VE_SEQ_CTRL, ~VE_SEQ_CTRL_YUV420,
+ 0);
+ } else if (f->fmt.pix.pixelformat == V4L2_PIX_FMT_YUV420) {
+ video->fmt.pixelformat = V4L2_PIX_FMT_YUV420;
+ aspeed_video_init_jpeg_table(video->jpeg.virt, true);
+ aspeed_video_update(video, VE_SEQ_CTRL, 0xFFFFFFFF,
+ VE_SEQ_CTRL_YUV420);
+ } else {
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int aspeed_video_get_jpegcomp(struct file *file, void *fh,
+ struct v4l2_jpegcompression *a)
+{
+ struct aspeed_video *video = video_drvdata(file);
+
+ a->quality = video->jpeg_quality;
+
+ return 0;
+}
+
+static int aspeed_video_set_jpegcomp(struct file *file, void *fh,
+ const struct v4l2_jpegcompression *a)
+{
+ u32 comp_ctrl;
+ struct aspeed_video *video = video_drvdata(file);
+
+ if (a->quality < 0 || a->quality > 11)
+ return -EINVAL;
+
+ video->jpeg_quality = a->quality;
+ comp_ctrl = FIELD_PREP(VE_COMP_CTRL_DCT_LUM, video->jpeg_quality) |
+ FIELD_PREP(VE_COMP_CTRL_DCT_CHR, video->jpeg_quality | 0x10);
+
+ aspeed_video_update(video, VE_COMP_CTRL,
+ ~(VE_COMP_CTRL_DCT_LUM | VE_COMP_CTRL_DCT_CHR),
+ comp_ctrl);
+
+ return 0;
+}
+
+static int aspeed_video_get_parm(struct file *file, void *fh,
+ struct v4l2_streamparm *a)
+{
+ struct aspeed_video *video = video_drvdata(file);
+
+ a->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+ a->parm.capture.timeperframe.numerator = 1;
+ a->parm.capture.timeperframe.denominator = video->frame_rate;
+
+ return 0;
+}
+
+static int aspeed_video_set_parm(struct file *file, void *fh,
+ struct v4l2_streamparm *a)
+{
+ int frame_rate;
+ struct aspeed_video *video = video_drvdata(file);
+
+ if (a->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+ return -EINVAL;
+
+ frame_rate = a->parm.capture.timeperframe.denominator /
+ a->parm.capture.timeperframe.numerator;
+
+ if (frame_rate < 0 || frame_rate > 60)
+ return -EINVAL;
+
+ if (video->frame_rate != frame_rate) {
+ video->frame_rate = frame_rate;
+ aspeed_video_update(video, VE_CTRL, ~VE_CTRL_FRC,
+ FIELD_PREP(VE_CTRL_FRC, frame_rate));
+ }
+
+ return 0;
+}
+
+static const struct v4l2_ioctl_ops aspeed_video_ioctls = {
+ .vidioc_querycap = aspeed_video_querycap,
+ .vidioc_g_fmt_vid_cap = aspeed_video_get_format,
+ .vidioc_s_fmt_vid_cap = aspeed_video_set_format,
+ .vidioc_g_jpegcomp = aspeed_video_get_jpegcomp,
+ .vidioc_s_jpegcomp = aspeed_video_set_jpegcomp,
+ .vidioc_g_parm = aspeed_video_get_parm,
+ .vidioc_s_parm = aspeed_video_set_parm,
+};
+
+static void aspeed_video_resolution_work(struct work_struct *work)
+{
+ int rc;
+ struct delayed_work *dwork = to_delayed_work(work);
+ struct aspeed_video *video = container_of(dwork, struct aspeed_video,
+ res_work);
+
+ /* No clients remaining after delay */
+ if (atomic_read(&video->clients) == 0)
+ goto done;
+
+ aspeed_video_on(video);
+
+ aspeed_video_init_regs(video);
+
+ rc = aspeed_video_get_resolution(video);
+ if (rc) {
+ dev_err(video->dev,
+ "resolution changed; couldn't get new resolution\n");
+ } else {
+ video->frame_idx = 0;
+ clear_bit(VIDEO_FRAME_TRIGGERED, &video->flags);
+ }
+
+done:
+ clear_bit(VIDEO_RES_CHANGE, &video->flags);
+ wake_up_interruptible_all(&video->wait);
+}
+
+static bool aspeed_video_frame_available(struct aspeed_video *video)
+{
+ if (!test_and_clear_bit(VIDEO_FRAME_AVAILABLE, &video->flags)) {
+ if (!test_bit(VIDEO_FRAME_TRIGGERED, &video->flags))
+ aspeed_video_start_frame(video);
+
+ return false;
+ }
+
+ return true;
+}
+
+static ssize_t aspeed_video_file_read(struct file *file, char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ int rc;
+ int fidx;
+ size_t size;
+ struct aspeed_video *video = video_drvdata(file);
+
+ if (mutex_lock_interruptible(&video->video_lock))
+ return -EINTR;
+
+ if (file->f_flags & O_NONBLOCK) {
+ if (!aspeed_video_frame_available(video)) {
+ rc = -EAGAIN;
+ goto unlock;
+ } else {
+ goto ready;
+ }
+ }
+
+ rc = wait_event_interruptible(video->wait,
+ aspeed_video_frame_available(video));
+ if (rc) {
+ rc = -EINTR;
+ goto unlock;
+ }
+
+ready:
+ fidx = video->frame_idx;
+ size = min_t(size_t, video->frame_size, count);
+ aspeed_video_start_frame(video);
+
+ if (copy_to_user(buf, video->comp[fidx].virt, size)) {
+ rc = -EFAULT;
+ goto unlock;
+ }
+
+ rc = size;
+
+unlock:
+ mutex_unlock(&video->video_lock);
+ return rc;
+}
+
+static int aspeed_video_open(struct file *file)
+{
+ int rc;
+ struct aspeed_video *video = video_drvdata(file);
+
+ if (atomic_inc_return(&video->clients) == 1) {
+ rc = aspeed_video_start(video);
+ if (rc) {
+ dev_err(video->dev, "Failed to start video engine\n");
+ atomic_dec(&video->clients);
+ return rc;
+ }
+ }
+
+ return v4l2_fh_open(file);
+}
+
+static int aspeed_video_release(struct file *file)
+{
+ int rc;
+ struct aspeed_video *video = video_drvdata(file);
+
+ rc = v4l2_fh_release(file);
+
+ if (atomic_dec_return(&video->clients) == 0)
+ aspeed_video_stop(video);
+
+ return rc;
+}
+
+static const struct v4l2_file_operations aspeed_video_v4l2_fops = {
+ .owner = THIS_MODULE,
+ .read = aspeed_video_file_read,
+ .unlocked_ioctl = video_ioctl2,
+ .open = aspeed_video_open,
+ .release = aspeed_video_release,
+};
+
+static void aspeed_video_device_release(struct video_device *vdev)
+{
+}
+
+static int aspeed_video_setup_video(struct aspeed_video *video)
+{
+ int rc;
+ struct v4l2_device *v4l2_dev = &video->v4l2_dev;
+ struct video_device *vdev = &video->vdev;
+
+ rc = v4l2_device_register(video->dev, v4l2_dev);
+ if (rc) {
+ dev_err(video->dev, "Failed to register v4l2 device\n");
+ return rc;
+ }
+
+ vdev->fops = &aspeed_video_v4l2_fops;
+ vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_READWRITE;
+ vdev->v4l2_dev = v4l2_dev;
+ strncpy(vdev->name, DEVICE_NAME, sizeof(vdev->name));
+ vdev->vfl_type = VFL_TYPE_GRABBER;
+ vdev->vfl_dir = VFL_DIR_RX;
+ vdev->release = aspeed_video_device_release;
+ vdev->ioctl_ops = &aspeed_video_ioctls;
+ vdev->lock = &video->video_lock;
+
+ video_set_drvdata(vdev, video);
+ rc = video_register_device(vdev, VFL_TYPE_GRABBER, 0);
+ if (rc) {
+ v4l2_device_unregister(v4l2_dev);
+ dev_err(video->dev, "Failed to register video device\n");
+ return rc;
+ }
+
+ /* set pixel format defaults */
+ video->fmt.pixelformat = V4L2_PIX_FMT_YUV444;
+ video->fmt.field = V4L2_FIELD_NONE;
+
+ return 0;
+}
+
+static int aspeed_video_init(struct aspeed_video *video)
+{
+ int irq;
+ int rc;
+ struct device *dev = video->dev;
+
+ irq = irq_of_parse_and_map(dev->of_node, 0);
+ if (!irq) {
+ dev_err(dev, "Unable to find IRQ\n");
+ return -ENODEV;
+ }
+
+ rc = devm_request_irq(dev, irq, aspeed_video_irq, IRQF_SHARED,
+ DEVICE_NAME, video);
+ if (rc < 0) {
+ dev_err(dev, "Unable to request IRQ %d\n", irq);
+ return rc;
+ }
+
+ video->eclk = devm_clk_get(dev, "eclk-gate");
+ if (IS_ERR(video->eclk)) {
+ dev_err(dev, "Unable to get ECLK\n");
+ return PTR_ERR(video->eclk);
+ }
+
+ video->vclk = devm_clk_get(dev, "vclk-gate");
+ if (IS_ERR(video->vclk)) {
+ dev_err(dev, "Unable to get VCLK\n");
+ return PTR_ERR(video->vclk);
+ }
+
+ video->rst = devm_reset_control_get_exclusive(dev, NULL);
+ if (IS_ERR(video->rst)) {
+ dev_err(dev, "Unable to get VE reset\n");
+ return PTR_ERR(video->rst);
+ }
+
+ rc = of_reserved_mem_device_init(dev);
+ if (rc) {
+ dev_err(dev, "Unable to reserve memory\n");
+ return rc;
+ }
+
+ rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
+ if (rc) {
+ dev_err(dev, "Failed to set DMA mask\n");
+ of_reserved_mem_device_release(dev);
+ return rc;
+ }
+
+ return 0;
+}
+
+static int aspeed_video_probe(struct platform_device *pdev)
+{
+ int rc;
+ struct resource *res;
+ struct aspeed_video *video = kzalloc(sizeof(*video), GFP_KERNEL);
+
+ if (!video)
+ return -ENOMEM;
+
+ video->frame_rate = 30;
+ video->dev = &pdev->dev;
+ mutex_init(&video->video_lock);
+ init_waitqueue_head(&video->wait);
+ INIT_DELAYED_WORK(&video->res_work, aspeed_video_resolution_work);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+ video->base = devm_ioremap_resource(video->dev, res);
+
+ if (IS_ERR(video->base))
+ return PTR_ERR(video->base);
+
+ rc = aspeed_video_init(video);
+ if (rc)
+ return rc;
+
+ rc = aspeed_video_setup_video(video);
+ if (rc)
+ return rc;
+
+ return 0;
+}
+
+static int aspeed_video_remove(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct v4l2_device *v4l2_dev = dev_get_drvdata(dev);
+ struct aspeed_video *video = to_aspeed_video(v4l2_dev);
+
+ video_unregister_device(&video->vdev);
+
+ v4l2_device_unregister(v4l2_dev);
+
+ of_reserved_mem_device_release(dev);
+
+ return 0;
+}
+
+static const struct of_device_id aspeed_video_of_match[] = {
+ { .compatible = "aspeed,ast2400-video" },
+ { .compatible = "aspeed,ast2500-video" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, aspeed_video_of_match);
+
+static struct platform_driver aspeed_video_driver = {
+ .driver = {
+ .name = DEVICE_NAME,
+ .of_match_table = aspeed_video_of_match,
+ },
+ .probe = aspeed_video_probe,
+ .remove = aspeed_video_remove,
+};
+
+module_platform_driver(aspeed_video_driver);
+
+MODULE_DESCRIPTION("ASPEED Video Engine Driver");
+MODULE_AUTHOR("Eddie James");
+MODULE_LICENSE("GPL v2");
--
1.8.3.1
^ permalink raw reply related
* [RFC 1/2] dt-bindings: media: Add Aspeed Video Engine binding documentation
From: Eddie James @ 2018-08-16 19:43 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1534448601-74120-1-git-send-email-eajames@linux.vnet.ibm.com>
Document the bindings.
Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
---
.../devicetree/bindings/media/aspeed-video.txt | 25 ++++++++++++++++++++++
1 file changed, 25 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/aspeed-video.txt
diff --git a/Documentation/devicetree/bindings/media/aspeed-video.txt b/Documentation/devicetree/bindings/media/aspeed-video.txt
new file mode 100644
index 0000000..91a494e
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/aspeed-video.txt
@@ -0,0 +1,25 @@
+* Device tree bindings for Aspeed Video Engine
+
+The Video Engine (VE) embedded in the Aspeed AST2400 and AST2500 SOCs can
+capture and compress video data from digital or analog sources.
+
+Required properties:
+ - compatible: "aspeed,ast2400-video" or "aspeed,ast2500-video"
+ - reg: contains the offset and length of the VE memory region
+ - clocks: pointers to the the "vclk" and "eclk" of the syscon
+ - clock-names: "vclk-gate", "eclk-gate"
+ - resets: pointer to the VE reset of the syscon
+ - interrupts: the interrupt associated with the VE on this platform
+ - reg-scu: pointer to the syscon itself
+
+Example:
+
+video: video at 1e700000 {
+ compatible = "aspeed,ast2500-video";
+ reg = <0x1e700000 0x20000>;
+ clocks = <&syscon ASPEED_CLK_GATE_VCLK>, <&syscon ASPEED_CLK_GATE_ECLK>;
+ clock-names = "vclk-gate", "eclk-gate";
+ resets = <&syscon ASPEED_RESET_VIDEO>;
+ interrupts = <7>;
+ reg-scu = <&syscon>;
+};
--
1.8.3.1
^ permalink raw reply related
* [RFC 0/2] media: platform: Add Aspeed Video Engine driver
From: Eddie James @ 2018-08-16 19:43 UTC (permalink / raw)
To: linux-aspeed
The Video Engine (VE) embedded in the Aspeed AST2400 and AST2500 SOCs
can capture and compress video data from digital or analog sources. With
the Aspeed chip acting as a service processor, the Video Engine can
capture the host processor graphics output.
This series adds a V4L2 driver for the VE, providing a read() interface
only. The driver triggers the hardware to capture the host graphics output
and compress it to JPEG format.
Testing on an AST2500 determined that the videobuf/streaming/mmap interface
was significantly slower than the simple read() interface, so I have not
included the streaming part.
It's also possible to use an automatic mode for the VE such that
re-triggering the HW every frame isn't necessary. However this wasn't
reliable on the AST2400, and probably used more CPU anyway due to excessive
interrupts. It was approximately 15% faster.
Eddie James (2):
dt-bindings: media: Add Aspeed Video Engine binding documentation
media: platform: Add Aspeed Video Engine driver
.../devicetree/bindings/media/aspeed-video.txt | 25 +
drivers/media/platform/Kconfig | 8 +
drivers/media/platform/Makefile | 1 +
drivers/media/platform/aspeed-video.c | 1307 ++++++++++++++++++++
4 files changed, 1341 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/aspeed-video.txt
create mode 100644 drivers/media/platform/aspeed-video.c
--
1.8.3.1
^ permalink raw reply
* [PATCH i2c-next v2] i2c: aspeed: Add an explicit type casting for *get_clk_reg_val
From: Wolfram Sang @ 2018-08-04 21:13 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180724203615.5770-1-jae.hyun.yoo@linux.intel.com>
On Tue, Jul 24, 2018 at 01:36:15PM -0700, Jae Hyun Yoo wrote:
> This commit fixes this sparse warning:
> drivers/i2c/busses/i2c-aspeed.c:875:38: warning: incorrect type in assignment (different modifiers)
> drivers/i2c/busses/i2c-aspeed.c:875:38: expected unsigned int ( *get_clk_reg_val )( ... )
> drivers/i2c/busses/i2c-aspeed.c:875:38: got void const *const data
>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
Added the 'Reported-tag' and applied to for-next, thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20180804/01f720dc/attachment.sig>
^ permalink raw reply
* [PATCH i2c-next v3] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-08-01 21:44 UTC (permalink / raw)
To: linux-aspeed
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 more in multi-master
environment 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>
---
Changes since v2:
- Changed the name of ASPEED_I2CD_INTR_ERRORS to ASPEED_I2CD_INTR_MASTER_ERRORS
- Removed a member irq_status from the struct aspeed_i2c_bus and changed
master_irq and slave_irq handlers to make them return status_ack.
- Added a comment to explain why it needs to try both irq handlers.
Changes since v1:
- Fixed a grammar issue in commit message.
- Added a missing line feed character into a message printing.
drivers/i2c/busses/i2c-aspeed.c | 110 +++++++++++++++++++-------------
1 file changed, 65 insertions(+), 45 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index efb89422d496..37cee46f1e36 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_MASTER_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 | \
@@ -227,20 +232,16 @@ 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)
+static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
{
- 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 0;
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) {
@@ -249,10 +250,8 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
}
/* 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 status_ack;
dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
irq_status, command);
@@ -281,19 +280,19 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
status_ack |= ASPEED_I2CD_INTR_TX_NAK;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
+ if (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)
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))
dev_err(bus->dev,
"Expected ACK after processed read.\n");
@@ -317,14 +316,7 @@ 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;
+ return status_ack;
}
#endif /* CONFIG_I2C_SLAVE */
@@ -380,21 +372,21 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
return 0;
}
-static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
+static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
{
- 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) {
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;
}
/*
@@ -403,19 +395,22 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
* INACTIVE state.
*/
ret = aspeed_i2c_is_irq_error(irq_status);
- if (ret < 0) {
+ if (ret) {
dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
irq_status);
bus->cmd_err = ret;
bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
+ status_ack |= (irq_status & ASPEED_I2CD_INTR_MASTER_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",
+ 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;
}
@@ -428,6 +423,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
*/
if (bus->master_state == ASPEED_I2C_MASTER_START) {
if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
+ if (unlikely(!(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\n", msg->addr);
status_ack |= ASPEED_I2CD_INTR_TX_NAK;
bus->cmd_err = -ENXIO;
@@ -506,7 +506,9 @@ 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");
+ dev_err(bus->dev,
+ "master failed to STOP irq_status:0x%x\n",
+ irq_status);
bus->cmd_err = -EIO;
/* Do not STOP as we have already tried. */
} else {
@@ -540,33 +542,51 @@ 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;
+ return status_ack;
}
static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
{
struct aspeed_i2c_bus *bus = dev_id;
- bool ret;
+ u32 irq_received, irq_status, irq_acked;
spin_lock(&bus->lock);
+ irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+ 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;
+ /*
+ * In most cases, interrupt bits will be set one by one, although
+ * multiple interrupt bits could be set at the same time. It also
+ * possible that master interrupt bits could be set along with slave
+ * interrupt bits. Each case needs to be handled using corresponding
+ * handlers depending on the current state.
+ */
+ if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
+ irq_acked = aspeed_i2c_master_irq(bus, irq_status);
+ irq_status &= ~irq_acked;
+ if (irq_status)
+ irq_acked = aspeed_i2c_slave_irq(bus, irq_status);
+ } else {
+ irq_acked = aspeed_i2c_slave_irq(bus, irq_status);
+ irq_status &= ~irq_acked;
+ if (irq_status)
+ irq_acked = aspeed_i2c_master_irq(bus, irq_status);
}
+#else
+ irq_acked = aspeed_i2c_master_irq(bus, irq_status);
#endif /* CONFIG_I2C_SLAVE */
- ret = aspeed_i2c_master_irq(bus);
+ irq_status &= ~irq_acked;
+ if (irq_status)
+ dev_err(bus->dev,
+ "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
+ irq_received, irq_received ^ 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 irq_status ? IRQ_NONE : IRQ_HANDLED;
}
static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
--
2.18.0
^ permalink raw reply related
* [PATCH i2c-next v2] i2c: aspeed: Add an explicit type casting for *get_clk_reg_val
From: Jae Hyun Yoo @ 2018-08-01 18:14 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CAFd5g44L+-6wtV4+Kn5-j8zRXA1aNamR2pV0EaFpcoshgQ4bTQ@mail.gmail.com>
On 7/31/2018 11:56 PM, Brendan Higgins wrote:
> On Tue, Jul 24, 2018 at 1:39 PM Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> wrote:
>>
>> This commit fixes this sparse warning:
>> drivers/i2c/busses/i2c-aspeed.c:875:38: warning: incorrect type in assignment (different modifiers)
>> drivers/i2c/busses/i2c-aspeed.c:875:38: expected unsigned int ( *get_clk_reg_val )( ... )
>> drivers/i2c/busses/i2c-aspeed.c:875:38: got void const *const data
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> ---
>> Changes since v1:
>> - Fixed title and added Reported-by tag.
>>
>> drivers/i2c/busses/i2c-aspeed.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index efb89422d496..a4f956c6d567 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -872,7 +872,7 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
>> if (!match)
>> bus->get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val;
>> else
>> - bus->get_clk_reg_val = match->data;
>> + bus->get_clk_reg_val = (u32 (*)(u32))match->data;
>>
>> /* Initialize the I2C adapter */
>> spin_lock_init(&bus->lock);
>> --
>> 2.18.0
>>
>
> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
>
Thanks a lot Brendan for your review!
Hi Wolfram,
I forgot adding 'Reported-by: Wolfram Sang <wsa@the-dreams.de>'. Please
add this tag when you apply this patch, or let me know if I need to
submit v3 for adding the tag.
Thanks,
Jae
^ permalink raw reply
* [PATCH v2] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-08-01 18:08 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CAFd5g46VKJeggUSMdF9Lta42FrpF0qVfrSHi3Bz=gU2qnSrAPA@mail.gmail.com>
Hi Brendan,
Thanks a lot for the review. Please see my inline answers.
On 7/31/2018 11:44 PM, Brendan Higgins wrote:
> On Tue, Jul 24, 2018 at 10:31 AM Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> 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 more in multi-master
>> environment 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.
>>
>> Changes since v1:
>> - Fixed a grammer issue in commit message.
>> - Added a missing line feed character into a message printing.
>
> This should not go in the commit log. Please move this to the comment section.
>
Oops! I'll move it to the comment section. Thanks!
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> ---
>> drivers/i2c/busses/i2c-aspeed.c | 135 ++++++++++++++++++--------------
>> 1 file changed, 75 insertions(+), 60 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index efb89422d496..75431e305073 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)
>
> I think it will be cleaner if both this and aspeed_i2c_master_irq return the
> status_ack; that way you will not have to add the irq_status field to the
> struct.
>
Yes, that would be better. I'll make slave_irq and master_irq return the
status_ack.
>> {
>> - 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;
>
> Here and elsewhere, you are trying to mask out the bits you handled, right?
> Please use `&= ~status_ack;` instead.
>
Right. I'll fix it as you suggested.
>> + 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);
>
> ASPEED_I2CD_INTR_ERRORS only occur in master mode? Please make that more clear
> in the name.
>
Yes, that should be handled in master mode. If the errors detected in a
peer master operation, the peer master has a responsibility to recover
bus. I'll change the name to ASPEED_I2CD_INTR_MASTER_ERRORS.
Actually, Aspeed AST2500 I2C IP provides slave timeout error detection
which can be enabled by setting a bit 15 of the Interrupt Control
Register (I2CD0C), and we can check the error through Interrupt Status
Register (I2CD10) bit 15, so that we can add an error handling logic
into slave_irq. I'll submit an additional patch later for it.
>> 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,7 +425,13 @@ 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))) {
>> + 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\n", msg->addr);
>> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> bus->cmd_err = -ENXIO;
>> @@ -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) {
>
> It might be nice to have a comment here about why you might try both handlers.
>
Okay. Will add a comment here.
>> + 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);
>
> You should probably ACK as soon as you read the bits.
>
In my experiments, doing ACK at the end of irq reduces the possibility
of master/slave combining irq calls that I previously explained.
>> 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,
>> --
>> 2.18.0
>>
>
^ permalink raw reply
* [PATCH 4/4] mtd: spi-nor: aspeed: introduce optimized settings for fast reads
From: Cédric Le Goater @ 2018-08-01 7:43 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CACPK8Xd0KeTpP+hbHXUDnu5-XzywX_ngCx_N-1Qcokavk5cw7w@mail.gmail.com>
On 07/23/2018 02:16 PM, Joel Stanley wrote:
> On 22 June 2018 at 21:44, C?dric Le Goater <clg@kaod.org> wrote:
>> Better settings for fast reads are looked for by implementing a SPI
>> timing calibration sequence described in the Aspeed SoC specification
>> document. The code is based on the OpenPOWER pflash tool and a similar
>> sequence using DMAs can be found in the SDK U-Boot.
>>
>> The SPI calibration performs a loop on different SPI clock rates
>> (dividers of the AHB clock rates) and on different input delay cycles
>> for each SPI clock rates. The successive read results are compared to
>> a golden buffer, read at low speed, to select the safest and fastest
>> read settings for the chip.
>>
>> The "spi-max-frequency" property is used to cap the optimize read
>> algorithm on some devices or controllers for which we want a "really"
>> safe setting, on the FMC controller chips for instance.
>>
>> It can also be deactivated at boot time with a kernel parameter
>> 'optimize_read', but that was never used on the field.
>>
>> Signed-off-by: C?dric Le Goater <clg@kaod.org>
>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
>
> I have also been running these applied to a 4.17 base on ast2400 and
> ast2500 systems for the past few months. This week I gave them a spin
> on top of linux-next too.
>
> They have looked good so far, so I would encourage the series to me
> merged for 4.19 so we can reduce the number of out of tree we use in
> OpenBMC systems.
>
> For the series:
>
> Tested-by: Joel Stanley <joel@jms.id.au>
The first 3 patches should not be too much of a problem. What about
patch 4/4 ? It is not the usual way of setting the freq but the Aspeed
controller has its own mean for tuning it.
Thanks,
C.
^ permalink raw reply
* [PATCH i2c-next v2] i2c: aspeed: Add an explicit type casting for *get_clk_reg_val
From: Brendan Higgins @ 2018-08-01 6:56 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180724203615.5770-1-jae.hyun.yoo@linux.intel.com>
On Tue, Jul 24, 2018 at 1:39 PM Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> wrote:
>
> This commit fixes this sparse warning:
> drivers/i2c/busses/i2c-aspeed.c:875:38: warning: incorrect type in assignment (different modifiers)
> drivers/i2c/busses/i2c-aspeed.c:875:38: expected unsigned int ( *get_clk_reg_val )( ... )
> drivers/i2c/busses/i2c-aspeed.c:875:38: got void const *const data
>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
> Changes since v1:
> - Fixed title and added Reported-by tag.
>
> drivers/i2c/busses/i2c-aspeed.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index efb89422d496..a4f956c6d567 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -872,7 +872,7 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
> if (!match)
> bus->get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val;
> else
> - bus->get_clk_reg_val = match->data;
> + bus->get_clk_reg_val = (u32 (*)(u32))match->data;
>
> /* Initialize the I2C adapter */
> spin_lock_init(&bus->lock);
> --
> 2.18.0
>
Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
^ permalink raw reply
* [PATCH v2] i2c: aspeed: Handle master/slave combined irq events properly
From: Brendan Higgins @ 2018-08-01 6:44 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180724173123.4377-1-jae.hyun.yoo@linux.intel.com>
On Tue, Jul 24, 2018 at 10:31 AM Jae Hyun Yoo
<jae.hyun.yoo@linux.intel.com> 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 more in multi-master
> environment 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.
>
> Changes since v1:
> - Fixed a grammer issue in commit message.
> - Added a missing line feed character into a message printing.
This should not go in the commit log. Please move this to the comment section.
>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
> drivers/i2c/busses/i2c-aspeed.c | 135 ++++++++++++++++++--------------
> 1 file changed, 75 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index efb89422d496..75431e305073 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)
I think it will be cleaner if both this and aspeed_i2c_master_irq return the
status_ack; that way you will not have to add the irq_status field to the
struct.
> {
> - 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;
Here and elsewhere, you are trying to mask out the bits you handled, right?
Please use `&= ~status_ack;` instead.
> + 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);
ASPEED_I2CD_INTR_ERRORS only occur in master mode? Please make that more clear
in the name.
> 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,7 +425,13 @@ 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))) {
> + 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\n", msg->addr);
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> bus->cmd_err = -ENXIO;
> @@ -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) {
It might be nice to have a comment here about why you might try both handlers.
> + 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);
You should probably ACK as soon as you read the bits.
> 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,
> --
> 2.18.0
>
^ permalink raw reply
* [PATCH v7 08/12] mfd: intel-peci-client: Add PECI client MFD driver
From: Jae Hyun Yoo @ 2018-07-31 18:56 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180731070117.GE4662@dell>
Hi Lee,
On 7/31/2018 12:01 AM, Lee Jones wrote:
> On Mon, 30 Jul 2018, Jae Hyun Yoo wrote:
>
>> Hi Rob,
>>
>> On 7/30/2018 3:10 PM, Rob Herring wrote:
>>> On Fri, Jul 27, 2018 at 11:36 AM Jae Hyun Yoo
>>> <jae.hyun.yoo@linux.intel.com> wrote:
>>>>
>>>> Hi Lee,
>>>>
>>>> On 7/27/2018 1:26 AM, Lee Jones wrote:
>>>>> On Mon, 23 Jul 2018, Jae Hyun Yoo wrote:
>>>>>
>>>>>> This commit adds PECI client MFD driver.
>>>>>>
>>>>>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>>>>>> Cc: Lee Jones <lee.jones@linaro.org>
>>>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>>>> Cc: Andrew Jeffery <andrew@aj.id.au>
>>>>>> Cc: James Feist <james.feist@linux.intel.com>
>>>>>> Cc: Jason M Biils <jason.m.bills@linux.intel.com>
>>>>>> Cc: Joel Stanley <joel@jms.id.au>
>>>>>> Cc: Vernon Mauery <vernon.mauery@linux.intel.com>
>>>>>> ---
>>>>>> drivers/mfd/Kconfig | 14 ++
>>>>>> drivers/mfd/Makefile | 1 +
>>>>>> drivers/mfd/intel-peci-client.c | 182 ++++++++++++++++++++++++++
>>>>>> include/linux/mfd/intel-peci-client.h | 81 ++++++++++++
>>>>>> 4 files changed, 278 insertions(+)
>>>>>> create mode 100644 drivers/mfd/intel-peci-client.c
>>>>>> create mode 100644 include/linux/mfd/intel-peci-client.h
>>>>>>
>>>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>>>>> index f3fa516011ec..e38b591479d4 100644
>>>>>> --- a/drivers/mfd/Kconfig
>>>>>> +++ b/drivers/mfd/Kconfig
>>>>>> @@ -595,6 +595,20 @@ config MFD_INTEL_MSIC
>>>>>> Passage) chip. This chip embeds audio, battery, GPIO, etc.
>>>>>> devices used in Intel Medfield platforms.
>>>>>>
>>>>>> +config MFD_INTEL_PECI_CLIENT
>>>>>> + bool "Intel PECI client"
>>>>>> + depends on (PECI || COMPILE_TEST)
>>>>>> + select MFD_CORE
>>>>>> + help
>>>>>> + If you say yes to this option, support will be included for the
>>>>>> + multi-funtional Intel PECI (Platform Environment Control Interface)
>>>>>> + client. PECI is a one-wire bus interface that provides a communication
>>>>>> + channel from PECI clients in Intel processors and chipset components
>>>>>> + to external monitoring or control devices.
>>>>>> +
>>>>>> + Additional drivers must be enabled in order to use the functionality
>>>>>> + of the device.
>>>>>> +
>>>>>> config MFD_IPAQ_MICRO
>>>>>> bool "Atmel Micro ASIC (iPAQ h3100/h3600/h3700) Support"
>>>>>> depends on SA1100_H3100 || SA1100_H3600
>>>>>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>>>>>> index 2852a6042ecf..29e2cacc58bd 100644
>>>>>> --- a/drivers/mfd/Makefile
>>>>>> +++ b/drivers/mfd/Makefile
>>>>>> @@ -203,6 +203,7 @@ obj-$(CONFIG_MFD_INTEL_LPSS) += intel-lpss.o
>>>>>> obj-$(CONFIG_MFD_INTEL_LPSS_PCI) += intel-lpss-pci.o
>>>>>> obj-$(CONFIG_MFD_INTEL_LPSS_ACPI) += intel-lpss-acpi.o
>>>>>> obj-$(CONFIG_MFD_INTEL_MSIC) += intel_msic.o
>>>>>> +obj-$(CONFIG_MFD_INTEL_PECI_CLIENT) += intel-peci-client.o
>>>>>> obj-$(CONFIG_MFD_PALMAS) += palmas.o
>>>>>> obj-$(CONFIG_MFD_VIPERBOARD) += viperboard.o
>>>>>> obj-$(CONFIG_MFD_RC5T583) += rc5t583.o rc5t583-irq.o
>>>>>> diff --git a/drivers/mfd/intel-peci-client.c b/drivers/mfd/intel-peci-client.c
>>>>>> new file mode 100644
>>>>>> index 000000000000..d7702cf1ea50
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/mfd/intel-peci-client.c
>>>>>> @@ -0,0 +1,182 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>> +// Copyright (c) 2018 Intel Corporation
>>>>>> +
>>>>>> +#include <linux/bitfield.h>
>>>>>> +#include <linux/mfd/core.h>
>>>>>> +#include <linux/mfd/intel-peci-client.h>
>>>>>> +#include <linux/module.h>
>>>>>> +#include <linux/peci.h>
>>>>>> +#include <linux/of_device.h>
>>>>>> +
>>>>>> +enum cpu_gens {
>>>>>> + CPU_GEN_HSX = 0, /* Haswell Xeon */
>>>>>> + CPU_GEN_BRX, /* Broadwell Xeon */
>>>>>> + CPU_GEN_SKX, /* Skylake Xeon */
>>>>>> +};
>>>>>> +
>>>>>> +static struct mfd_cell peci_functions[] = {
>>>>>> + {
>>>>>> + .name = "peci-cputemp",
>>>>>> + .of_compatible = "intel,peci-cputemp",
>>>>>> + },
>>>>>> + {
>>>>>> + .name = "peci-dimmtemp",
>>>>>> + .of_compatible = "intel,peci-dimmtemp",
>>>>>> + },
>>>>>> +};
>>>>>
>>>>> The more I look at this driver, the less I think it fits into MFD.
>>>>>
>>>>> What's stopping you from registering these devices directly from DT?
>>>>>
>>>>
>>>> Because DT doesn't allow 2 nodes at the same address so Rob suggested
>>>> MFD for this case.
>>>
>>> Only after I first suggested you create cpu and dimm sensors from a
>>> single hwmon driver. Perhaps you should figure out how to fix the
>>> problem with delayed registration. Perhaps you can register the
>>> sensor, but just delay returning actual data until it is ready.
>>>
>>
>> The actual reason why I divided the single hwmon driver into two is for
>> using recommended hwmon API set which doesn't allow incremental
>> creation of sysfs attributes at run time - using of
>> 'devm_hwmon_device_register_with_info' is suggested by Guenter instead
>> of using other deprecated APIs.
>>
>> The reason why the delayed registration is needed is, BMC kernel cannot
>> know how many DIMM are installed in remote machine before the machine
>> completing memory training and testing in POST. So dimm temp driver
>> cannot create hwmon attributes in advance.
>>
>> My thought is, this way is the best to address these requirements.
>
> As I've previously explained, I do not think this is a good fit for
> MFD. Since this whole PECI piece is a bit, let's say 'niche', perhaps
> it's better to contain the client within the PECI subsystem too. You
> can then use the platform_device_add() API to register devices as and
> when required.
>
If I do like that, I would face the 'DT doesn't allow 2 nodes at the
same address' issue again. Also, as I previously explained, additional
PECI sideband function drivers should be added later through this MFD
driver. For an example, remote CPU crash dump driver which would be an
misc char type driver that are sharing the same unit address also needs
to be added later, and it's definitely not an additional hwmon subsystem
driver. I'm still thinking that an MFD driver is more suitable to
support these mixed-type function drivers that are sharing a single
device unit.
Thanks,
Jae
^ permalink raw reply
* [PATCH v7 08/12] mfd: intel-peci-client: Add PECI client MFD driver
From: Lee Jones @ 2018-07-31 7:01 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <7ab43253-1154-6e2f-b216-69b0d44b470a@linux.intel.com>
On Mon, 30 Jul 2018, Jae Hyun Yoo wrote:
> Hi Rob,
>
> On 7/30/2018 3:10 PM, Rob Herring wrote:
> > On Fri, Jul 27, 2018 at 11:36 AM Jae Hyun Yoo
> > <jae.hyun.yoo@linux.intel.com> wrote:
> > >
> > > Hi Lee,
> > >
> > > On 7/27/2018 1:26 AM, Lee Jones wrote:
> > > > On Mon, 23 Jul 2018, Jae Hyun Yoo wrote:
> > > >
> > > > > This commit adds PECI client MFD driver.
> > > > >
> > > > > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> > > > > Cc: Lee Jones <lee.jones@linaro.org>
> > > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > > Cc: Andrew Jeffery <andrew@aj.id.au>
> > > > > Cc: James Feist <james.feist@linux.intel.com>
> > > > > Cc: Jason M Biils <jason.m.bills@linux.intel.com>
> > > > > Cc: Joel Stanley <joel@jms.id.au>
> > > > > Cc: Vernon Mauery <vernon.mauery@linux.intel.com>
> > > > > ---
> > > > > drivers/mfd/Kconfig | 14 ++
> > > > > drivers/mfd/Makefile | 1 +
> > > > > drivers/mfd/intel-peci-client.c | 182 ++++++++++++++++++++++++++
> > > > > include/linux/mfd/intel-peci-client.h | 81 ++++++++++++
> > > > > 4 files changed, 278 insertions(+)
> > > > > create mode 100644 drivers/mfd/intel-peci-client.c
> > > > > create mode 100644 include/linux/mfd/intel-peci-client.h
> > > > >
> > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > > > index f3fa516011ec..e38b591479d4 100644
> > > > > --- a/drivers/mfd/Kconfig
> > > > > +++ b/drivers/mfd/Kconfig
> > > > > @@ -595,6 +595,20 @@ config MFD_INTEL_MSIC
> > > > > Passage) chip. This chip embeds audio, battery, GPIO, etc.
> > > > > devices used in Intel Medfield platforms.
> > > > >
> > > > > +config MFD_INTEL_PECI_CLIENT
> > > > > + bool "Intel PECI client"
> > > > > + depends on (PECI || COMPILE_TEST)
> > > > > + select MFD_CORE
> > > > > + help
> > > > > + If you say yes to this option, support will be included for the
> > > > > + multi-funtional Intel PECI (Platform Environment Control Interface)
> > > > > + client. PECI is a one-wire bus interface that provides a communication
> > > > > + channel from PECI clients in Intel processors and chipset components
> > > > > + to external monitoring or control devices.
> > > > > +
> > > > > + Additional drivers must be enabled in order to use the functionality
> > > > > + of the device.
> > > > > +
> > > > > config MFD_IPAQ_MICRO
> > > > > bool "Atmel Micro ASIC (iPAQ h3100/h3600/h3700) Support"
> > > > > depends on SA1100_H3100 || SA1100_H3600
> > > > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > > > > index 2852a6042ecf..29e2cacc58bd 100644
> > > > > --- a/drivers/mfd/Makefile
> > > > > +++ b/drivers/mfd/Makefile
> > > > > @@ -203,6 +203,7 @@ obj-$(CONFIG_MFD_INTEL_LPSS) += intel-lpss.o
> > > > > obj-$(CONFIG_MFD_INTEL_LPSS_PCI) += intel-lpss-pci.o
> > > > > obj-$(CONFIG_MFD_INTEL_LPSS_ACPI) += intel-lpss-acpi.o
> > > > > obj-$(CONFIG_MFD_INTEL_MSIC) += intel_msic.o
> > > > > +obj-$(CONFIG_MFD_INTEL_PECI_CLIENT) += intel-peci-client.o
> > > > > obj-$(CONFIG_MFD_PALMAS) += palmas.o
> > > > > obj-$(CONFIG_MFD_VIPERBOARD) += viperboard.o
> > > > > obj-$(CONFIG_MFD_RC5T583) += rc5t583.o rc5t583-irq.o
> > > > > diff --git a/drivers/mfd/intel-peci-client.c b/drivers/mfd/intel-peci-client.c
> > > > > new file mode 100644
> > > > > index 000000000000..d7702cf1ea50
> > > > > --- /dev/null
> > > > > +++ b/drivers/mfd/intel-peci-client.c
> > > > > @@ -0,0 +1,182 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +// Copyright (c) 2018 Intel Corporation
> > > > > +
> > > > > +#include <linux/bitfield.h>
> > > > > +#include <linux/mfd/core.h>
> > > > > +#include <linux/mfd/intel-peci-client.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/peci.h>
> > > > > +#include <linux/of_device.h>
> > > > > +
> > > > > +enum cpu_gens {
> > > > > + CPU_GEN_HSX = 0, /* Haswell Xeon */
> > > > > + CPU_GEN_BRX, /* Broadwell Xeon */
> > > > > + CPU_GEN_SKX, /* Skylake Xeon */
> > > > > +};
> > > > > +
> > > > > +static struct mfd_cell peci_functions[] = {
> > > > > + {
> > > > > + .name = "peci-cputemp",
> > > > > + .of_compatible = "intel,peci-cputemp",
> > > > > + },
> > > > > + {
> > > > > + .name = "peci-dimmtemp",
> > > > > + .of_compatible = "intel,peci-dimmtemp",
> > > > > + },
> > > > > +};
> > > >
> > > > The more I look at this driver, the less I think it fits into MFD.
> > > >
> > > > What's stopping you from registering these devices directly from DT?
> > > >
> > >
> > > Because DT doesn't allow 2 nodes at the same address so Rob suggested
> > > MFD for this case.
> >
> > Only after I first suggested you create cpu and dimm sensors from a
> > single hwmon driver. Perhaps you should figure out how to fix the
> > problem with delayed registration. Perhaps you can register the
> > sensor, but just delay returning actual data until it is ready.
> >
>
> The actual reason why I divided the single hwmon driver into two is for
> using recommended hwmon API set which doesn't allow incremental
> creation of sysfs attributes at run time - using of
> 'devm_hwmon_device_register_with_info' is suggested by Guenter instead
> of using other deprecated APIs.
>
> The reason why the delayed registration is needed is, BMC kernel cannot
> know how many DIMM are installed in remote machine before the machine
> completing memory training and testing in POST. So dimm temp driver
> cannot create hwmon attributes in advance.
>
> My thought is, this way is the best to address these requirements.
As I've previously explained, I do not think this is a good fit for
MFD. Since this whole PECI piece is a bit, let's say 'niche', perhaps
it's better to contain the client within the PECI subsystem too. You
can then use the platform_device_add() API to register devices as and
when required.
--
Lee Jones [???]
Linaro Services Technical Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH v7 08/12] mfd: intel-peci-client: Add PECI client MFD driver
From: Jae Hyun Yoo @ 2018-07-30 23:15 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CAL_JsqL7WkDOaegwmBDqLkRh0JfCo7O2krONVstjWMUsLiJMrg@mail.gmail.com>
Hi Rob,
On 7/30/2018 3:10 PM, Rob Herring wrote:
> On Fri, Jul 27, 2018 at 11:36 AM Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> wrote:
>>
>> Hi Lee,
>>
>> On 7/27/2018 1:26 AM, Lee Jones wrote:
>>> On Mon, 23 Jul 2018, Jae Hyun Yoo wrote:
>>>
>>>> This commit adds PECI client MFD driver.
>>>>
>>>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>>>> Cc: Lee Jones <lee.jones@linaro.org>
>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>> Cc: Andrew Jeffery <andrew@aj.id.au>
>>>> Cc: James Feist <james.feist@linux.intel.com>
>>>> Cc: Jason M Biils <jason.m.bills@linux.intel.com>
>>>> Cc: Joel Stanley <joel@jms.id.au>
>>>> Cc: Vernon Mauery <vernon.mauery@linux.intel.com>
>>>> ---
>>>> drivers/mfd/Kconfig | 14 ++
>>>> drivers/mfd/Makefile | 1 +
>>>> drivers/mfd/intel-peci-client.c | 182 ++++++++++++++++++++++++++
>>>> include/linux/mfd/intel-peci-client.h | 81 ++++++++++++
>>>> 4 files changed, 278 insertions(+)
>>>> create mode 100644 drivers/mfd/intel-peci-client.c
>>>> create mode 100644 include/linux/mfd/intel-peci-client.h
>>>>
>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>>> index f3fa516011ec..e38b591479d4 100644
>>>> --- a/drivers/mfd/Kconfig
>>>> +++ b/drivers/mfd/Kconfig
>>>> @@ -595,6 +595,20 @@ config MFD_INTEL_MSIC
>>>> Passage) chip. This chip embeds audio, battery, GPIO, etc.
>>>> devices used in Intel Medfield platforms.
>>>>
>>>> +config MFD_INTEL_PECI_CLIENT
>>>> + bool "Intel PECI client"
>>>> + depends on (PECI || COMPILE_TEST)
>>>> + select MFD_CORE
>>>> + help
>>>> + If you say yes to this option, support will be included for the
>>>> + multi-funtional Intel PECI (Platform Environment Control Interface)
>>>> + client. PECI is a one-wire bus interface that provides a communication
>>>> + channel from PECI clients in Intel processors and chipset components
>>>> + to external monitoring or control devices.
>>>> +
>>>> + Additional drivers must be enabled in order to use the functionality
>>>> + of the device.
>>>> +
>>>> config MFD_IPAQ_MICRO
>>>> bool "Atmel Micro ASIC (iPAQ h3100/h3600/h3700) Support"
>>>> depends on SA1100_H3100 || SA1100_H3600
>>>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>>>> index 2852a6042ecf..29e2cacc58bd 100644
>>>> --- a/drivers/mfd/Makefile
>>>> +++ b/drivers/mfd/Makefile
>>>> @@ -203,6 +203,7 @@ obj-$(CONFIG_MFD_INTEL_LPSS) += intel-lpss.o
>>>> obj-$(CONFIG_MFD_INTEL_LPSS_PCI) += intel-lpss-pci.o
>>>> obj-$(CONFIG_MFD_INTEL_LPSS_ACPI) += intel-lpss-acpi.o
>>>> obj-$(CONFIG_MFD_INTEL_MSIC) += intel_msic.o
>>>> +obj-$(CONFIG_MFD_INTEL_PECI_CLIENT) += intel-peci-client.o
>>>> obj-$(CONFIG_MFD_PALMAS) += palmas.o
>>>> obj-$(CONFIG_MFD_VIPERBOARD) += viperboard.o
>>>> obj-$(CONFIG_MFD_RC5T583) += rc5t583.o rc5t583-irq.o
>>>> diff --git a/drivers/mfd/intel-peci-client.c b/drivers/mfd/intel-peci-client.c
>>>> new file mode 100644
>>>> index 000000000000..d7702cf1ea50
>>>> --- /dev/null
>>>> +++ b/drivers/mfd/intel-peci-client.c
>>>> @@ -0,0 +1,182 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +// Copyright (c) 2018 Intel Corporation
>>>> +
>>>> +#include <linux/bitfield.h>
>>>> +#include <linux/mfd/core.h>
>>>> +#include <linux/mfd/intel-peci-client.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/peci.h>
>>>> +#include <linux/of_device.h>
>>>> +
>>>> +enum cpu_gens {
>>>> + CPU_GEN_HSX = 0, /* Haswell Xeon */
>>>> + CPU_GEN_BRX, /* Broadwell Xeon */
>>>> + CPU_GEN_SKX, /* Skylake Xeon */
>>>> +};
>>>> +
>>>> +static struct mfd_cell peci_functions[] = {
>>>> + {
>>>> + .name = "peci-cputemp",
>>>> + .of_compatible = "intel,peci-cputemp",
>>>> + },
>>>> + {
>>>> + .name = "peci-dimmtemp",
>>>> + .of_compatible = "intel,peci-dimmtemp",
>>>> + },
>>>> +};
>>>
>>> The more I look at this driver, the less I think it fits into MFD.
>>>
>>> What's stopping you from registering these devices directly from DT?
>>>
>>
>> Because DT doesn't allow 2 nodes at the same address so Rob suggested
>> MFD for this case.
>
> Only after I first suggested you create cpu and dimm sensors from a
> single hwmon driver. Perhaps you should figure out how to fix the
> problem with delayed registration. Perhaps you can register the
> sensor, but just delay returning actual data until it is ready.
>
The actual reason why I divided the single hwmon driver into two is for
using recommended hwmon API set which doesn't allow incremental
creation of sysfs attributes at run time - using of
'devm_hwmon_device_register_with_info' is suggested by Guenter instead
of using other deprecated APIs.
The reason why the delayed registration is needed is, BMC kernel cannot
know how many DIMM are installed in remote machine before the machine
completing memory training and testing in POST. So dimm temp driver
cannot create hwmon attributes in advance.
My thought is, this way is the best to address these requirements.
Thanks,
Jae
> Rob
>
^ permalink raw reply
* [PATCH v7 11/12] hwmon: Add PECI dimmtemp driver
From: Jae Hyun Yoo @ 2018-07-30 22:45 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CAL_JsqK4k=c6k-BJT5fWQH_DCqGhr13fq6M_iZZehaveE-z5Hg@mail.gmail.com>
Hi Rob,
On 7/30/2018 3:06 PM, Rob Herring wrote:
> On Mon, Jul 23, 2018 at 3:49 PM Jae Hyun Yoo
> <jae.hyun.yoo@linux.intel.com> wrote:
>>
>> This commit adds PECI dimmtemp hwmon driver.
>>
>
>> +#if IS_ENABLED(CONFIG_OF)
>> +static const struct of_device_id peci_dimmtemp_of_table[] = {
>> + { .compatible = "intel,peci-dimmtemp" },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(of, peci_dimmtemp_of_table);
>> +#endif
>
> This should be removed. Same for cpu temp driver I'm guessing.
>
Yes, you are right. This isn't needed anymore because MFD driver
loads this module. I'll remove this code from here and from cpu temp
driver. Also, I'll remove 'of_compatible' setting of 'struct mfd_cell
peci_functions' in intel-peci-client.c code.
Thanks a lot for your pointing it out!
Jae
> Rob
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox