* Re: [PATCH] i2c: davinci: Avoid zero value of CLKH
From: Wolfram Sang @ 2018-07-23 18:07 UTC (permalink / raw)
To: Alexander Sverdlin; +Cc: Sekhar Nori, linux-i2c, linux-arm-kernel, Kevin Hilman
In-Reply-To: <20180713152017.2207-1-alexander.sverdlin@nokia.com>
[-- Attachment #1.1: Type: text/plain, Size: 521 bytes --]
On Fri, Jul 13, 2018 at 05:20:17PM +0200, Alexander Sverdlin wrote:
> If CLKH is set to 0 I2C clock is not generated at all, so avoid this value
> and stretch the clock in this case.
>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
Applied to for-current, thanks!
I did not add stable because Alexander told me this is very likely not
to be observed on HW out there. But TI people are investigating more.
I suggest they resend this patch to stable if they see fit. D'accord
everyone?
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] i2c: davinci: Avoid zero value of CLKH
From: Wolfram Sang @ 2018-07-23 18:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180713152017.2207-1-alexander.sverdlin@nokia.com>
On Fri, Jul 13, 2018 at 05:20:17PM +0200, Alexander Sverdlin wrote:
> If CLKH is set to 0 I2C clock is not generated at all, so avoid this value
> and stretch the clock in this case.
>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
Applied to for-current, thanks!
I did not add stable because Alexander told me this is very likely not
to be observed on HW out there. But TI people are investigating more.
I suggest they resend this patch to stable if they see fit. D'accord
everyone?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180723/8ea58ced/attachment.sig>
^ permalink raw reply
* [PATCH] media: dw2102: Fix memleak on sequence of probes
From: Anton Vasilyev @ 2018-07-23 17:04 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Anton Vasilyev, Jonathan Corbet, Enrico Mioso, Bhumika Goyal,
Sean Young, linux-media, linux-kernel, ldv-project
Each call to dw2102_probe() allocates memory by kmemdup for structures
p1100, s660, p7500 and s421, but there is no their deallocation.
dvb_usb_device_init() copies the corresponding structure into
dvb_usb_device->props, so there is no use of original structure after
dvb_usb_device_init().
The patch moves structures from global scope to local and adds their
deallocation.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Anton Vasilyev <vasilyev@ispras.ru>
---
drivers/media/usb/dvb-usb/dw2102.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/media/usb/dvb-usb/dw2102.c b/drivers/media/usb/dvb-usb/dw2102.c
index 0d4fdd34a710..9ce8b4d79d1f 100644
--- a/drivers/media/usb/dvb-usb/dw2102.c
+++ b/drivers/media/usb/dvb-usb/dw2102.c
@@ -2101,14 +2101,12 @@ static struct dvb_usb_device_properties s6x0_properties = {
}
};
-static struct dvb_usb_device_properties *p1100;
static const struct dvb_usb_device_description d1100 = {
"Prof 1100 USB ",
{&dw2102_table[PROF_1100], NULL},
{NULL},
};
-static struct dvb_usb_device_properties *s660;
static const struct dvb_usb_device_description d660 = {
"TeVii S660 USB",
{&dw2102_table[TEVII_S660], NULL},
@@ -2127,14 +2125,12 @@ static const struct dvb_usb_device_description d480_2 = {
{NULL},
};
-static struct dvb_usb_device_properties *p7500;
static const struct dvb_usb_device_description d7500 = {
"Prof 7500 USB DVB-S2",
{&dw2102_table[PROF_7500], NULL},
{NULL},
};
-static struct dvb_usb_device_properties *s421;
static const struct dvb_usb_device_description d421 = {
"TeVii S421 PCI",
{&dw2102_table[TEVII_S421], NULL},
@@ -2334,6 +2330,11 @@ static int dw2102_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
int retval = -ENOMEM;
+ struct dvb_usb_device_properties *p1100;
+ struct dvb_usb_device_properties *s660;
+ struct dvb_usb_device_properties *p7500;
+ struct dvb_usb_device_properties *s421;
+
p1100 = kmemdup(&s6x0_properties,
sizeof(struct dvb_usb_device_properties), GFP_KERNEL);
if (!p1100)
@@ -2402,8 +2403,16 @@ static int dw2102_probe(struct usb_interface *intf,
0 == dvb_usb_device_init(intf, &t220_properties,
THIS_MODULE, NULL, adapter_nr) ||
0 == dvb_usb_device_init(intf, &tt_s2_4600_properties,
- THIS_MODULE, NULL, adapter_nr))
+ THIS_MODULE, NULL, adapter_nr)) {
+
+ /* clean up copied properties */
+ kfree(s421);
+ kfree(p7500);
+ kfree(s660);
+ kfree(p1100);
+
return 0;
+ }
retval = -ENODEV;
kfree(s421);
--
2.18.0
^ permalink raw reply related
* Re: [PATCH net-next] wan/fsl_ucc_hdlc: use IS_ERR_VALUE() to check return value of qe_muram_alloc
From: David Miller @ 2018-07-23 18:07 UTC (permalink / raw)
To: yuehaibing; +Cc: qiang.zhao, linux-kernel, netdev, linuxppc-dev
In-Reply-To: <20180723141233.19948-1-yuehaibing@huawei.com>
From: YueHaibing <yuehaibing@huawei.com>
Date: Mon, 23 Jul 2018 22:12:33 +0800
> qe_muram_alloc return a unsigned long integer,which should not
> compared with zero. check it using IS_ERR_VALUE() to fix this.
>
> Fixes: c19b6d246a35 ("drivers/net: support hdlc function for QE-UCC")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH v2 1/4] i2c: imx: Fix reinit_completion() use
From: Wolfram Sang @ 2018-07-23 18:08 UTC (permalink / raw)
To: Esben Haabendal
Cc: linux-i2c, Wolfram Sang, Uwe Kleine-König, Rob Herring,
Mark Rutland, Yuan Yao, Esben Haabendal, Phil Reid, Lucas Stach,
Linus Walleij, Peter Rosin, Fabio Estevam, linux-kernel
In-Reply-To: <20180709094304.8814-2-esben.haabendal@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 607 bytes --]
On Mon, Jul 09, 2018 at 11:43:01AM +0200, Esben Haabendal wrote:
> From: Esben Haabendal <eha@deif.com>
>
> Make sure to call reinit_completion() before dma is started to avoid race
> condition where reinit_completion() is called after complete() and before
> wait_for_completion_timeout().
>
> Signed-off-by: Esben Haabendal <eha@deif.com>
> Fixes: ce1a78840ff7 ("i2c: imx: add DMA support for freescale i2c driver")
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Applied to for-current and added stable, thanks!
Uwe, do you have comments for the other patches, too?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] t3507: add a testcase showing failure with sparse checkout
From: Eric Sunshine @ 2018-07-23 18:09 UTC (permalink / raw)
To: Ben Peart; +Cc: Elijah Newren, Git List, Junio C Hamano, Ben Peart, kewillf
In-Reply-To: <b56598a1-e543-500a-a39b-cee07fe1e533@gmail.com>
On Mon, Jul 23, 2018 at 9:12 AM Ben Peart <peartben@gmail.com> wrote:
> On 7/21/2018 3:21 AM, Eric Sunshine wrote:
> > On Sat, Jul 21, 2018 at 2:34 AM Elijah Newren <newren@gmail.com> wrote:
> >> + rm .git/info/sparse-checkout
> >
> > Should this cleanup be done by test_when_finished()?
>
> I think trying to use test_when_finished() for this really degrades the
> readability of the test. See below:
>
> test_expect_success 'failed cherry-pick with sparse-checkout' '
> pristine_detach initial &&
> test_config core.sparsecheckout true &&
> echo /unrelated >.git/info/sparse-checkout &&
> git read-tree --reset -u HEAD &&
> test_when_finished "echo \"/*\" >.git/info/sparse-checkout && git
> read-tree --reset -u HEAD && rm .git/info/sparse-checkout" &&
> test_must_fail git cherry-pick -Xours picked>actual &&
> test_i18ngrep ! "Changes not staged for commit:" actual
> '
>
> Given it takes multiple commands, I'd prefer to keep the setup and
> cleanup of the sparse checkout settings symmetrical.
Some observations:
The test_when_finished() ought to be called before the initial
git-read-tree, otherwise you risk leaving a .git/info/sparse-checkout
sitting around if git-read-tree fails.
The tear-down code could be moved to a function, in which case,
test_when_finished() would simply call that function.
Multi-line quoted strings are valid, so you don't need to string out
all the tear-down steps on a single line like that, and instead spread
them across multiple lines to improve readability.
test_when_finished() doesn't expect just a single quoted string as
argument. In fact, it can take many (unquoted) arguments, which also
allows you to spread the tear-down steps over multiple lines to
improve readability.
Multiple test_when_finished() invocations are allowed, so you could
spread out the tear-down commands that way (though they'd have to be
in reverse order, which would be bad for readability in this case,
thus not recommended).
Correctness ought to trump readability, not the other way around.
So, one possibility, which seems pretty readable to me:
test_expect_failure 'failed cherry-pick with sparse-checkout' '
pristine_detach initial &&
test_config core.sparseCheckout true &&
test_when_finished "
echo \"/*\" >.git/info/sparse-checkout
git read-tree --reset -u HEAD
rm .git/info/sparse-checkout" &&
echo /unrelated >.git/info/sparse-checkout &&
git read-tree --reset -u HEAD &&
test_must_fail git cherry-pick -Xours picked>actual &&
test_i18ngrep ! "Changes not staged for commit:" actual &&
'
Notice that I dropped the internal &&-chain in test_when_finish() to
ensure that the final 'rm' is invoked even if the cleanup
git-read-tree fails (though all bets are probably off, anyhow, if it
does fail).
^ permalink raw reply
* Re: [PATCH] btrfs: fix size_t format string
From: Randy Dunlap @ 2018-07-23 17:07 UTC (permalink / raw)
To: dsterba, Arnd Bergmann, Chris Mason, Josef Bacik, David Sterba,
Qu Wenruo, Nikolay Borisov, Su Yue, linux-btrfs, linux-kernel
In-Reply-To: <20180717150400.GA3126@twin.jikos.cz>
On 07/17/2018 08:04 AM, David Sterba wrote:
> On Tue, Jul 17, 2018 at 03:52:27PM +0200, Arnd Bergmann wrote:
>> The newly added check_block_group_item() function causes a build warning
>> on 32-bit architectures:
>>
>> fs/btrfs/tree-checker.c: In function 'check_block_group_item':
>> fs/btrfs/tree-checker.c:404:41: error: format '%lu' expects argument of type 'long unsigned int', but argument 6 has type 'unsigned int' [-Werror=format=]
>>
>> The type of a sizeof() expression is size_t, which is correctly printed
>> using the %zu format string.
>>
>> Fixes: 9dc16aad5660 ("btrfs: tree-checker: Verify block_group_item")
>
> Folded to the commit, thanks.
>
Hi David,
Where did this patch end up? linux-next-20180723 is still showing this
format warning.
thanks,
--
~Randy
^ permalink raw reply
* [PATCH i2c-next] i2c: aspeed: Handle master/slave combined irq events properly
From: James Feist @ 2018-07-23 18:10 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180723174808.4007-1-jae.hyun.yoo@linux.intel.com>
On 07/23/2018 10:48 AM, Jae Hyun Yoo wrote:
> In most of cases, interrupt bits are set one by one but there are
> also a lot of other cases that Aspeed I2C IP sends multiple
> interrupt bits with combining master and slave events using a
> single interrupt call. It happens much in multi-master environment
much more
> than single-master. For an example, when master is waiting for a
> NORMAL_STOP interrupt in its MASTER_STOP state, SLAVE_MATCH and
> RX_DONE interrupts could come along with the NORMAL_STOP in case of
> an another master immediately sends data just after acquiring the
> bus. In this case, the NORMAL_STOP interrupt should be handled
> by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
> handled by slave_irq. This commit modifies irq hadling logic to
> handle the master/slave combined events properly.
>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
> drivers/i2c/busses/i2c-aspeed.c | 137 ++++++++++++++++++--------------
> 1 file changed, 76 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index efb89422d496..24d43f143a55 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -82,6 +82,11 @@
> #define ASPEED_I2CD_INTR_RX_DONE BIT(2)
> #define ASPEED_I2CD_INTR_TX_NAK BIT(1)
> #define ASPEED_I2CD_INTR_TX_ACK BIT(0)
> +#define ASPEED_I2CD_INTR_ERRORS \
> + (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
> + ASPEED_I2CD_INTR_SCL_TIMEOUT | \
> + ASPEED_I2CD_INTR_ABNORMAL | \
> + ASPEED_I2CD_INTR_ARBIT_LOSS)
> #define ASPEED_I2CD_INTR_ALL \
> (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
> ASPEED_I2CD_INTR_BUS_RECOVER_DONE | \
> @@ -150,6 +155,7 @@ struct aspeed_i2c_bus {
> int cmd_err;
> /* Protected only by i2c_lock_bus */
> int master_xfer_result;
> + u32 irq_status;
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> struct i2c_client *slave;
> enum aspeed_i2c_slave_state slave_state;
> @@ -229,36 +235,30 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> {
> - u32 command, irq_status, status_ack = 0;
> + u32 command, status_ack = 0;
> struct i2c_client *slave = bus->slave;
> - bool irq_handled = true;
> u8 value;
>
> - if (!slave) {
> - irq_handled = false;
> - goto out;
> - }
> + if (!slave)
> + return false;
>
> command = readl(bus->base + ASPEED_I2C_CMD_REG);
> - irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>
> /* Slave was requested, restart state machine. */
> - if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
> status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
> bus->slave_state = ASPEED_I2C_SLAVE_START;
> }
>
> /* Slave is not currently active, irq was for someone else. */
> - if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
> - irq_handled = false;
> - goto out;
> - }
> + if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
> + return false;
>
> dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
> - irq_status, command);
> + bus->irq_status, command);
>
> /* Slave was sent something. */
> - if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_RX_DONE) {
> value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
> /* Handle address frame. */
> if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
> @@ -273,28 +273,29 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> }
>
> /* Slave was asked to stop. */
> - if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
> status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> }
> - if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_NAK) {
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> }
> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK) {
> + status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> + }
>
> switch (bus->slave_state) {
> case ASPEED_I2C_SLAVE_READ_REQUESTED:
> - if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK)
> dev_err(bus->dev, "Unexpected ACK on read request.\n");
> bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
> -
> i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
> writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
> writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
> break;
> case ASPEED_I2C_SLAVE_READ_PROCESSED:
> - status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> - if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
> + if (!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))
> dev_err(bus->dev,
> "Expected ACK after processed read.\n");
> i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value);
> @@ -317,14 +318,8 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> break;
> }
>
> - if (status_ack != irq_status)
> - dev_err(bus->dev,
> - "irq handled != irq. expected %x, but was %x\n",
> - irq_status, status_ack);
> - writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
> -
> -out:
> - return irq_handled;
> + bus->irq_status ^= status_ack;
> + return !bus->irq_status;
> }
> #endif /* CONFIG_I2C_SLAVE */
>
> @@ -382,19 +377,19 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
>
> static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> {
> - u32 irq_status, status_ack = 0, command = 0;
> + u32 status_ack = 0, command = 0;
> struct i2c_msg *msg;
> u8 recv_byte;
> int ret;
>
> - irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> - /* Ack all interrupt bits. */
> - writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
> -
> - if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
> goto out_complete;
> + } else {
> + /* Master is not currently active, irq was for someone else. */
> + if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE)
> + goto out_no_complete;
> }
>
> /*
> @@ -402,20 +397,23 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> * should clear the command queue effectively taking us back to the
> * INACTIVE state.
> */
> - ret = aspeed_i2c_is_irq_error(irq_status);
> - if (ret < 0) {
> + ret = aspeed_i2c_is_irq_error(bus->irq_status);
> + if (ret) {
> dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
> - irq_status);
> + bus->irq_status);
> bus->cmd_err = ret;
> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> + status_ack |= (bus->irq_status & ASPEED_I2CD_INTR_ERRORS);
> goto out_complete;
> }
>
> /* We are in an invalid state; reset bus to a known state. */
> if (!bus->msgs) {
> - dev_err(bus->dev, "bus in unknown state\n");
> + dev_err(bus->dev, "bus in unknown state irq_status: 0x%x\n",
> + bus->irq_status);
> bus->cmd_err = -EIO;
> - if (bus->master_state != ASPEED_I2C_MASTER_STOP)
> + if (bus->master_state != ASPEED_I2C_MASTER_STOP &&
> + bus->master_state != ASPEED_I2C_MASTER_INACTIVE)
> aspeed_i2c_do_stop(bus);
> goto out_no_complete;
> }
> @@ -427,8 +425,14 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> * then update the state and handle the new state below.
> */
> if (bus->master_state == ASPEED_I2C_MASTER_START) {
> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> - pr_devel("no slave present at %02x\n", msg->addr);
> + if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> + if (unlikely(!(bus->irq_status &
> + ASPEED_I2CD_INTR_TX_NAK))) {
> + bus->cmd_err = -ENXIO;
> + bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> + goto out_complete;
> + }
> + pr_devel("no slave present at %02x", msg->addr);
Missing line feed character
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> bus->cmd_err = -ENXIO;
> aspeed_i2c_do_stop(bus);
> @@ -447,11 +451,12 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>
> switch (bus->master_state) {
> case ASPEED_I2C_MASTER_TX:
> - if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
> + if (unlikely(bus->irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
> dev_dbg(bus->dev, "slave NACKed TX\n");
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> goto error_and_stop;
> - } else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> + } else if (unlikely(!(bus->irq_status &
> + ASPEED_I2CD_INTR_TX_ACK))) {
> dev_err(bus->dev, "slave failed to ACK TX\n");
> goto error_and_stop;
> }
> @@ -470,11 +475,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> goto out_no_complete;
> case ASPEED_I2C_MASTER_RX_FIRST:
> /* RX may not have completed yet (only address cycle) */
> - if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE))
> + if (!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))
> goto out_no_complete;
> /* fallthrough intended */
> case ASPEED_I2C_MASTER_RX:
> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
> + if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
> dev_err(bus->dev, "master failed to RX\n");
> goto error_and_stop;
> }
> @@ -505,8 +510,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> }
> goto out_no_complete;
> case ASPEED_I2C_MASTER_STOP:
> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
> - dev_err(bus->dev, "master failed to STOP\n");
> + if (unlikely(!(bus->irq_status &
> + ASPEED_I2CD_INTR_NORMAL_STOP))) {
> + dev_err(bus->dev,
> + "master failed to STOP irq_status:0x%x\n",
> + bus->irq_status);
> bus->cmd_err = -EIO;
> /* Do not STOP as we have already tried. */
> } else {
> @@ -518,7 +526,7 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> case ASPEED_I2C_MASTER_INACTIVE:
> dev_err(bus->dev,
> "master received interrupt 0x%08x, but is inactive\n",
> - irq_status);
> + bus->irq_status);
> bus->cmd_err = -EIO;
> /* Do not STOP as we should be inactive. */
> goto out_complete;
> @@ -540,33 +548,40 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> bus->master_xfer_result = bus->msgs_index + 1;
> complete(&bus->cmd_complete);
> out_no_complete:
> - if (irq_status != status_ack)
> - dev_err(bus->dev,
> - "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> - irq_status, status_ack);
> - return !!irq_status;
> + bus->irq_status ^= status_ack;
> + return !bus->irq_status;
> }
>
> static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> {
> struct aspeed_i2c_bus *bus = dev_id;
> - bool ret;
> + u32 irq_received;
>
> spin_lock(&bus->lock);
> + irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> + bus->irq_status = irq_received;
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> - if (aspeed_i2c_slave_irq(bus)) {
> - dev_dbg(bus->dev, "irq handled by slave.\n");
> - ret = true;
> - goto out;
> + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
> + if (!aspeed_i2c_master_irq(bus))
> + aspeed_i2c_slave_irq(bus);
> + } else {
> + if (!aspeed_i2c_slave_irq(bus))
> + aspeed_i2c_master_irq(bus);
> }
> +#else
> + aspeed_i2c_master_irq(bus);
> #endif /* CONFIG_I2C_SLAVE */
>
> - ret = aspeed_i2c_master_irq(bus);
> + if (bus->irq_status)
> + dev_err(bus->dev,
> + "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> + irq_received, irq_received ^ bus->irq_status);
>
> -out:
> + /* Ack all interrupt bits. */
> + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> spin_unlock(&bus->lock);
> - return ret ? IRQ_HANDLED : IRQ_NONE;
> + return bus->irq_status ? IRQ_NONE : IRQ_HANDLED;
> }
>
> static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
>
^ permalink raw reply
* Re: [PATCH i2c-next] i2c: aspeed: Handle master/slave combined irq events properly
From: James Feist @ 2018-07-23 18:10 UTC (permalink / raw)
To: Jae Hyun Yoo, Brendan Higgins, Benjamin Herrenschmidt,
Joel Stanley, Andrew Jeffery, linux-i2c, openbmc,
linux-arm-kernel, linux-aspeed, linux-kernel
Cc: Vernon Mauery, Jarkko Nikula
In-Reply-To: <20180723174808.4007-1-jae.hyun.yoo@linux.intel.com>
On 07/23/2018 10:48 AM, Jae Hyun Yoo wrote:
> In most of cases, interrupt bits are set one by one but there are
> also a lot of other cases that Aspeed I2C IP sends multiple
> interrupt bits with combining master and slave events using a
> single interrupt call. It happens much in multi-master environment
much more
> than single-master. For an example, when master is waiting for a
> NORMAL_STOP interrupt in its MASTER_STOP state, SLAVE_MATCH and
> RX_DONE interrupts could come along with the NORMAL_STOP in case of
> an another master immediately sends data just after acquiring the
> bus. In this case, the NORMAL_STOP interrupt should be handled
> by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
> handled by slave_irq. This commit modifies irq hadling logic to
> handle the master/slave combined events properly.
>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
> drivers/i2c/busses/i2c-aspeed.c | 137 ++++++++++++++++++--------------
> 1 file changed, 76 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index efb89422d496..24d43f143a55 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -82,6 +82,11 @@
> #define ASPEED_I2CD_INTR_RX_DONE BIT(2)
> #define ASPEED_I2CD_INTR_TX_NAK BIT(1)
> #define ASPEED_I2CD_INTR_TX_ACK BIT(0)
> +#define ASPEED_I2CD_INTR_ERRORS \
> + (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
> + ASPEED_I2CD_INTR_SCL_TIMEOUT | \
> + ASPEED_I2CD_INTR_ABNORMAL | \
> + ASPEED_I2CD_INTR_ARBIT_LOSS)
> #define ASPEED_I2CD_INTR_ALL \
> (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
> ASPEED_I2CD_INTR_BUS_RECOVER_DONE | \
> @@ -150,6 +155,7 @@ struct aspeed_i2c_bus {
> int cmd_err;
> /* Protected only by i2c_lock_bus */
> int master_xfer_result;
> + u32 irq_status;
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> struct i2c_client *slave;
> enum aspeed_i2c_slave_state slave_state;
> @@ -229,36 +235,30 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> {
> - u32 command, irq_status, status_ack = 0;
> + u32 command, status_ack = 0;
> struct i2c_client *slave = bus->slave;
> - bool irq_handled = true;
> u8 value;
>
> - if (!slave) {
> - irq_handled = false;
> - goto out;
> - }
> + if (!slave)
> + return false;
>
> command = readl(bus->base + ASPEED_I2C_CMD_REG);
> - irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>
> /* Slave was requested, restart state machine. */
> - if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
> status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
> bus->slave_state = ASPEED_I2C_SLAVE_START;
> }
>
> /* Slave is not currently active, irq was for someone else. */
> - if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
> - irq_handled = false;
> - goto out;
> - }
> + if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
> + return false;
>
> dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
> - irq_status, command);
> + bus->irq_status, command);
>
> /* Slave was sent something. */
> - if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_RX_DONE) {
> value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
> /* Handle address frame. */
> if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
> @@ -273,28 +273,29 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> }
>
> /* Slave was asked to stop. */
> - if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
> status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> }
> - if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_NAK) {
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> }
> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK) {
> + status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> + }
>
> switch (bus->slave_state) {
> case ASPEED_I2C_SLAVE_READ_REQUESTED:
> - if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK)
> dev_err(bus->dev, "Unexpected ACK on read request.\n");
> bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
> -
> i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
> writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
> writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
> break;
> case ASPEED_I2C_SLAVE_READ_PROCESSED:
> - status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> - if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
> + if (!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))
> dev_err(bus->dev,
> "Expected ACK after processed read.\n");
> i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value);
> @@ -317,14 +318,8 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> break;
> }
>
> - if (status_ack != irq_status)
> - dev_err(bus->dev,
> - "irq handled != irq. expected %x, but was %x\n",
> - irq_status, status_ack);
> - writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
> -
> -out:
> - return irq_handled;
> + bus->irq_status ^= status_ack;
> + return !bus->irq_status;
> }
> #endif /* CONFIG_I2C_SLAVE */
>
> @@ -382,19 +377,19 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
>
> static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> {
> - u32 irq_status, status_ack = 0, command = 0;
> + u32 status_ack = 0, command = 0;
> struct i2c_msg *msg;
> u8 recv_byte;
> int ret;
>
> - irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> - /* Ack all interrupt bits. */
> - writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
> -
> - if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
> goto out_complete;
> + } else {
> + /* Master is not currently active, irq was for someone else. */
> + if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE)
> + goto out_no_complete;
> }
>
> /*
> @@ -402,20 +397,23 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> * should clear the command queue effectively taking us back to the
> * INACTIVE state.
> */
> - ret = aspeed_i2c_is_irq_error(irq_status);
> - if (ret < 0) {
> + ret = aspeed_i2c_is_irq_error(bus->irq_status);
> + if (ret) {
> dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
> - irq_status);
> + bus->irq_status);
> bus->cmd_err = ret;
> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> + status_ack |= (bus->irq_status & ASPEED_I2CD_INTR_ERRORS);
> goto out_complete;
> }
>
> /* We are in an invalid state; reset bus to a known state. */
> if (!bus->msgs) {
> - dev_err(bus->dev, "bus in unknown state\n");
> + dev_err(bus->dev, "bus in unknown state irq_status: 0x%x\n",
> + bus->irq_status);
> bus->cmd_err = -EIO;
> - if (bus->master_state != ASPEED_I2C_MASTER_STOP)
> + if (bus->master_state != ASPEED_I2C_MASTER_STOP &&
> + bus->master_state != ASPEED_I2C_MASTER_INACTIVE)
> aspeed_i2c_do_stop(bus);
> goto out_no_complete;
> }
> @@ -427,8 +425,14 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> * then update the state and handle the new state below.
> */
> if (bus->master_state == ASPEED_I2C_MASTER_START) {
> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> - pr_devel("no slave present at %02x\n", msg->addr);
> + if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> + if (unlikely(!(bus->irq_status &
> + ASPEED_I2CD_INTR_TX_NAK))) {
> + bus->cmd_err = -ENXIO;
> + bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> + goto out_complete;
> + }
> + pr_devel("no slave present at %02x", msg->addr);
Missing line feed character
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> bus->cmd_err = -ENXIO;
> aspeed_i2c_do_stop(bus);
> @@ -447,11 +451,12 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>
> switch (bus->master_state) {
> case ASPEED_I2C_MASTER_TX:
> - if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
> + if (unlikely(bus->irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
> dev_dbg(bus->dev, "slave NACKed TX\n");
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> goto error_and_stop;
> - } else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> + } else if (unlikely(!(bus->irq_status &
> + ASPEED_I2CD_INTR_TX_ACK))) {
> dev_err(bus->dev, "slave failed to ACK TX\n");
> goto error_and_stop;
> }
> @@ -470,11 +475,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> goto out_no_complete;
> case ASPEED_I2C_MASTER_RX_FIRST:
> /* RX may not have completed yet (only address cycle) */
> - if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE))
> + if (!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))
> goto out_no_complete;
> /* fallthrough intended */
> case ASPEED_I2C_MASTER_RX:
> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
> + if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
> dev_err(bus->dev, "master failed to RX\n");
> goto error_and_stop;
> }
> @@ -505,8 +510,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> }
> goto out_no_complete;
> case ASPEED_I2C_MASTER_STOP:
> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
> - dev_err(bus->dev, "master failed to STOP\n");
> + if (unlikely(!(bus->irq_status &
> + ASPEED_I2CD_INTR_NORMAL_STOP))) {
> + dev_err(bus->dev,
> + "master failed to STOP irq_status:0x%x\n",
> + bus->irq_status);
> bus->cmd_err = -EIO;
> /* Do not STOP as we have already tried. */
> } else {
> @@ -518,7 +526,7 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> case ASPEED_I2C_MASTER_INACTIVE:
> dev_err(bus->dev,
> "master received interrupt 0x%08x, but is inactive\n",
> - irq_status);
> + bus->irq_status);
> bus->cmd_err = -EIO;
> /* Do not STOP as we should be inactive. */
> goto out_complete;
> @@ -540,33 +548,40 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> bus->master_xfer_result = bus->msgs_index + 1;
> complete(&bus->cmd_complete);
> out_no_complete:
> - if (irq_status != status_ack)
> - dev_err(bus->dev,
> - "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> - irq_status, status_ack);
> - return !!irq_status;
> + bus->irq_status ^= status_ack;
> + return !bus->irq_status;
> }
>
> static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> {
> struct aspeed_i2c_bus *bus = dev_id;
> - bool ret;
> + u32 irq_received;
>
> spin_lock(&bus->lock);
> + irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> + bus->irq_status = irq_received;
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> - if (aspeed_i2c_slave_irq(bus)) {
> - dev_dbg(bus->dev, "irq handled by slave.\n");
> - ret = true;
> - goto out;
> + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
> + if (!aspeed_i2c_master_irq(bus))
> + aspeed_i2c_slave_irq(bus);
> + } else {
> + if (!aspeed_i2c_slave_irq(bus))
> + aspeed_i2c_master_irq(bus);
> }
> +#else
> + aspeed_i2c_master_irq(bus);
> #endif /* CONFIG_I2C_SLAVE */
>
> - ret = aspeed_i2c_master_irq(bus);
> + if (bus->irq_status)
> + dev_err(bus->dev,
> + "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> + irq_received, irq_received ^ bus->irq_status);
>
> -out:
> + /* Ack all interrupt bits. */
> + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> spin_unlock(&bus->lock);
> - return ret ? IRQ_HANDLED : IRQ_NONE;
> + return bus->irq_status ? IRQ_NONE : IRQ_HANDLED;
> }
>
> static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
>
^ permalink raw reply
* [PATCH i2c-next] i2c: aspeed: Handle master/slave combined irq events properly
From: James Feist @ 2018-07-23 18:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180723174808.4007-1-jae.hyun.yoo@linux.intel.com>
On 07/23/2018 10:48 AM, Jae Hyun Yoo wrote:
> In most of cases, interrupt bits are set one by one but there are
> also a lot of other cases that Aspeed I2C IP sends multiple
> interrupt bits with combining master and slave events using a
> single interrupt call. It happens much in multi-master environment
much more
> than single-master. For an example, when master is waiting for a
> NORMAL_STOP interrupt in its MASTER_STOP state, SLAVE_MATCH and
> RX_DONE interrupts could come along with the NORMAL_STOP in case of
> an another master immediately sends data just after acquiring the
> bus. In this case, the NORMAL_STOP interrupt should be handled
> by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
> handled by slave_irq. This commit modifies irq hadling logic to
> handle the master/slave combined events properly.
>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
> drivers/i2c/busses/i2c-aspeed.c | 137 ++++++++++++++++++--------------
> 1 file changed, 76 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index efb89422d496..24d43f143a55 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -82,6 +82,11 @@
> #define ASPEED_I2CD_INTR_RX_DONE BIT(2)
> #define ASPEED_I2CD_INTR_TX_NAK BIT(1)
> #define ASPEED_I2CD_INTR_TX_ACK BIT(0)
> +#define ASPEED_I2CD_INTR_ERRORS \
> + (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
> + ASPEED_I2CD_INTR_SCL_TIMEOUT | \
> + ASPEED_I2CD_INTR_ABNORMAL | \
> + ASPEED_I2CD_INTR_ARBIT_LOSS)
> #define ASPEED_I2CD_INTR_ALL \
> (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
> ASPEED_I2CD_INTR_BUS_RECOVER_DONE | \
> @@ -150,6 +155,7 @@ struct aspeed_i2c_bus {
> int cmd_err;
> /* Protected only by i2c_lock_bus */
> int master_xfer_result;
> + u32 irq_status;
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> struct i2c_client *slave;
> enum aspeed_i2c_slave_state slave_state;
> @@ -229,36 +235,30 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> {
> - u32 command, irq_status, status_ack = 0;
> + u32 command, status_ack = 0;
> struct i2c_client *slave = bus->slave;
> - bool irq_handled = true;
> u8 value;
>
> - if (!slave) {
> - irq_handled = false;
> - goto out;
> - }
> + if (!slave)
> + return false;
>
> command = readl(bus->base + ASPEED_I2C_CMD_REG);
> - irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>
> /* Slave was requested, restart state machine. */
> - if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
> status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
> bus->slave_state = ASPEED_I2C_SLAVE_START;
> }
>
> /* Slave is not currently active, irq was for someone else. */
> - if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
> - irq_handled = false;
> - goto out;
> - }
> + if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
> + return false;
>
> dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
> - irq_status, command);
> + bus->irq_status, command);
>
> /* Slave was sent something. */
> - if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_RX_DONE) {
> value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
> /* Handle address frame. */
> if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
> @@ -273,28 +273,29 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> }
>
> /* Slave was asked to stop. */
> - if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
> status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> }
> - if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_NAK) {
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> }
> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK) {
> + status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> + }
>
> switch (bus->slave_state) {
> case ASPEED_I2C_SLAVE_READ_REQUESTED:
> - if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK)
> dev_err(bus->dev, "Unexpected ACK on read request.\n");
> bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
> -
> i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
> writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
> writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
> break;
> case ASPEED_I2C_SLAVE_READ_PROCESSED:
> - status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> - if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
> + if (!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))
> dev_err(bus->dev,
> "Expected ACK after processed read.\n");
> i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value);
> @@ -317,14 +318,8 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> break;
> }
>
> - if (status_ack != irq_status)
> - dev_err(bus->dev,
> - "irq handled != irq. expected %x, but was %x\n",
> - irq_status, status_ack);
> - writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
> -
> -out:
> - return irq_handled;
> + bus->irq_status ^= status_ack;
> + return !bus->irq_status;
> }
> #endif /* CONFIG_I2C_SLAVE */
>
> @@ -382,19 +377,19 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
>
> static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> {
> - u32 irq_status, status_ack = 0, command = 0;
> + u32 status_ack = 0, command = 0;
> struct i2c_msg *msg;
> u8 recv_byte;
> int ret;
>
> - irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> - /* Ack all interrupt bits. */
> - writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
> -
> - if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
> + if (bus->irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
> goto out_complete;
> + } else {
> + /* Master is not currently active, irq was for someone else. */
> + if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE)
> + goto out_no_complete;
> }
>
> /*
> @@ -402,20 +397,23 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> * should clear the command queue effectively taking us back to the
> * INACTIVE state.
> */
> - ret = aspeed_i2c_is_irq_error(irq_status);
> - if (ret < 0) {
> + ret = aspeed_i2c_is_irq_error(bus->irq_status);
> + if (ret) {
> dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
> - irq_status);
> + bus->irq_status);
> bus->cmd_err = ret;
> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> + status_ack |= (bus->irq_status & ASPEED_I2CD_INTR_ERRORS);
> goto out_complete;
> }
>
> /* We are in an invalid state; reset bus to a known state. */
> if (!bus->msgs) {
> - dev_err(bus->dev, "bus in unknown state\n");
> + dev_err(bus->dev, "bus in unknown state irq_status: 0x%x\n",
> + bus->irq_status);
> bus->cmd_err = -EIO;
> - if (bus->master_state != ASPEED_I2C_MASTER_STOP)
> + if (bus->master_state != ASPEED_I2C_MASTER_STOP &&
> + bus->master_state != ASPEED_I2C_MASTER_INACTIVE)
> aspeed_i2c_do_stop(bus);
> goto out_no_complete;
> }
> @@ -427,8 +425,14 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> * then update the state and handle the new state below.
> */
> if (bus->master_state == ASPEED_I2C_MASTER_START) {
> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> - pr_devel("no slave present at %02x\n", msg->addr);
> + if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> + if (unlikely(!(bus->irq_status &
> + ASPEED_I2CD_INTR_TX_NAK))) {
> + bus->cmd_err = -ENXIO;
> + bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> + goto out_complete;
> + }
> + pr_devel("no slave present at %02x", msg->addr);
Missing line feed character
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> bus->cmd_err = -ENXIO;
> aspeed_i2c_do_stop(bus);
> @@ -447,11 +451,12 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>
> switch (bus->master_state) {
> case ASPEED_I2C_MASTER_TX:
> - if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
> + if (unlikely(bus->irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
> dev_dbg(bus->dev, "slave NACKed TX\n");
> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> goto error_and_stop;
> - } else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> + } else if (unlikely(!(bus->irq_status &
> + ASPEED_I2CD_INTR_TX_ACK))) {
> dev_err(bus->dev, "slave failed to ACK TX\n");
> goto error_and_stop;
> }
> @@ -470,11 +475,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> goto out_no_complete;
> case ASPEED_I2C_MASTER_RX_FIRST:
> /* RX may not have completed yet (only address cycle) */
> - if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE))
> + if (!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))
> goto out_no_complete;
> /* fallthrough intended */
> case ASPEED_I2C_MASTER_RX:
> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
> + if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
> dev_err(bus->dev, "master failed to RX\n");
> goto error_and_stop;
> }
> @@ -505,8 +510,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> }
> goto out_no_complete;
> case ASPEED_I2C_MASTER_STOP:
> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
> - dev_err(bus->dev, "master failed to STOP\n");
> + if (unlikely(!(bus->irq_status &
> + ASPEED_I2CD_INTR_NORMAL_STOP))) {
> + dev_err(bus->dev,
> + "master failed to STOP irq_status:0x%x\n",
> + bus->irq_status);
> bus->cmd_err = -EIO;
> /* Do not STOP as we have already tried. */
> } else {
> @@ -518,7 +526,7 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> case ASPEED_I2C_MASTER_INACTIVE:
> dev_err(bus->dev,
> "master received interrupt 0x%08x, but is inactive\n",
> - irq_status);
> + bus->irq_status);
> bus->cmd_err = -EIO;
> /* Do not STOP as we should be inactive. */
> goto out_complete;
> @@ -540,33 +548,40 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> bus->master_xfer_result = bus->msgs_index + 1;
> complete(&bus->cmd_complete);
> out_no_complete:
> - if (irq_status != status_ack)
> - dev_err(bus->dev,
> - "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> - irq_status, status_ack);
> - return !!irq_status;
> + bus->irq_status ^= status_ack;
> + return !bus->irq_status;
> }
>
> static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> {
> struct aspeed_i2c_bus *bus = dev_id;
> - bool ret;
> + u32 irq_received;
>
> spin_lock(&bus->lock);
> + irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> + bus->irq_status = irq_received;
>
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> - if (aspeed_i2c_slave_irq(bus)) {
> - dev_dbg(bus->dev, "irq handled by slave.\n");
> - ret = true;
> - goto out;
> + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
> + if (!aspeed_i2c_master_irq(bus))
> + aspeed_i2c_slave_irq(bus);
> + } else {
> + if (!aspeed_i2c_slave_irq(bus))
> + aspeed_i2c_master_irq(bus);
> }
> +#else
> + aspeed_i2c_master_irq(bus);
> #endif /* CONFIG_I2C_SLAVE */
>
> - ret = aspeed_i2c_master_irq(bus);
> + if (bus->irq_status)
> + dev_err(bus->dev,
> + "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> + irq_received, irq_received ^ bus->irq_status);
>
> -out:
> + /* Ack all interrupt bits. */
> + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> spin_unlock(&bus->lock);
> - return ret ? IRQ_HANDLED : IRQ_NONE;
> + return bus->irq_status ? IRQ_NONE : IRQ_HANDLED;
> }
>
> static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
>
^ permalink raw reply
* Re: [PATCH v2 1/2] common/qat: add sgl header
From: De Lara Guarch, Pablo @ 2018-07-23 18:10 UTC (permalink / raw)
To: Trahe, Fiona, dev@dpdk.org; +Cc: Jozwiak, TomaszX
In-Reply-To: <1532351135-10064-1-git-send-email-fiona.trahe@intel.com>
> -----Original Message-----
> From: Trahe, Fiona
> Sent: Monday, July 23, 2018 2:06 PM
> To: dev@dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; Jozwiak, TomaszX <tomaszx.jozwiak@intel.com>
> Subject: [PATCH v2 1/2] common/qat: add sgl header
>
> This patch refactors the sgl struct so it includes a flexible array of flat buffers as
> sym and compress PMDs can have different size sgls.
>
> Signed-off-by: Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
> Signed-off-by: Fiona Trahe <fiona.trahe@intel.com>
Applied to dpdk-next-crypto.
Thanks,
Pablo
^ permalink raw reply
* Re: [PATCH] IB/mlx5: avoid binding a new mpi unit to the same devices repeatedly
From: Daniel Jurgens @ 2018-07-23 18:11 UTC (permalink / raw)
To: Qing Huang, Or Gerlitz, Parav Pandit
Cc: Linux Kernel, RDMA mailing list, Jason Gunthorpe, Doug Ledford,
Leon Romanovsky, gerald.gibson
In-Reply-To: <556984ea-c35f-197d-0e45-16272da3f604@oracle.com>
On 7/23/2018 10:36 AM, Qing Huang wrote:
>
> Hi Daniel/Parav,
>
> Have you got a chance to review this patch? Thanks!
Hi Qing, sorry for the delay, I just got back to the office today. I don't agree with the proposed fix, I provided an alternative suggestion below.
>
>>> Or.
>>>
>>>> Reported-by: Gerald Gibson <gerald.gibson@oracle.com>
>>>> Signed-off-by: Qing Huang <qing.huang@oracle.com>
>>>> ---
>>>> drivers/infiniband/hw/mlx5/main.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
>>>> index b3ba9a2..1ddd1d3 100644
>>>> --- a/drivers/infiniband/hw/mlx5/main.c
>>>> +++ b/drivers/infiniband/hw/mlx5/main.c
>>>> @@ -6068,7 +6068,8 @@ static void *mlx5_ib_add_slave_port(struct mlx5_core_dev *mdev, u8 port_num)
>>>>
>>>> mutex_lock(&mlx5_ib_multiport_mutex);
>>>> list_for_each_entry(dev, &mlx5_ib_dev_list, ib_dev_list) {
>>>> - if (dev->sys_image_guid == mpi->sys_image_guid)
>>>> + if (dev->sys_image_guid == mpi->sys_image_guid &&
>>>> + !dev->port[mlx5_core_native_port_num(mdev) - 1].mp.mpi)
You shouldn't check the mpi field that without holding the lock in the mp structure. Prefer you change the print from a warning in mlx5_ib_bind_slave_port to a debug message.
>>>> bound = mlx5_ib_bind_slave_port(dev, mpi);
>>>>
>>>> if (bound) {
>>>> --
>>>> 2.9.3
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: next-20180723 build: 2 failures 11 warnings (next-20180723)
From: Mark Brown @ 2018-07-23 18:11 UTC (permalink / raw)
To: Catalin Marinas, Will Deacon
Cc: linaro-kernel, khilman, kernel-build-reports, linux-next,
matthew.hart, linux-arm-kernel
In-Reply-To: <E1fheAy-0003tU-Dq@optimist>
[-- Attachment #1.1: Type: text/plain, Size: 779 bytes --]
On Mon, Jul 23, 2018 at 05:59:12PM +0100, Build bot for Mark Brown wrote:
Today's -next fails to build an arm64 allmodconfig with:
> arm64-allmodconfig
> ERROR: "__sync_icache_dcache" [drivers/xen/xen-privcmd.ko] undefined!
using
aarch64-linux-gnu-gcc (Linaro GCC 5.3-2016.05) 5.3.1 20160412
however it builds perfectly fine with
aarch64-linux-gnu-gcc (Linaro GCC 6.2-2016.11) 6.2.1 20161016
which honestly seems like a sensible and worthwhile upgrade at this
point anyway given that it's a year and a half old so I'm going to do
that for my builder (perhaps even jump on a newer version) but it seemed
worth highlighting in case this is considered undesirable. A similar
issue is hitting on KernelCI, we should probably look at upgrading the
toolchain there too.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* next-20180723 build: 2 failures 11 warnings (next-20180723)
From: Mark Brown @ 2018-07-23 18:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <E1fheAy-0003tU-Dq@optimist>
On Mon, Jul 23, 2018 at 05:59:12PM +0100, Build bot for Mark Brown wrote:
Today's -next fails to build an arm64 allmodconfig with:
> arm64-allmodconfig
> ERROR: "__sync_icache_dcache" [drivers/xen/xen-privcmd.ko] undefined!
using
aarch64-linux-gnu-gcc (Linaro GCC 5.3-2016.05) 5.3.1 20160412
however it builds perfectly fine with
aarch64-linux-gnu-gcc (Linaro GCC 6.2-2016.11) 6.2.1 20161016
which honestly seems like a sensible and worthwhile upgrade at this
point anyway given that it's a year and a half old so I'm going to do
that for my builder (perhaps even jump on a newer version) but it seemed
worth highlighting in case this is considered undesirable. A similar
issue is hitting on KernelCI, we should probably look at upgrading the
toolchain there too.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180723/f22badc9/attachment-0001.sig>
^ permalink raw reply
* Re: [PATCH 1/2] t3507: add a testcase showing failure with sparse checkout
From: Junio C Hamano @ 2018-07-23 18:12 UTC (permalink / raw)
To: Ben Peart; +Cc: Elijah Newren, git, benpeart, kewillf
In-Reply-To: <5250481c-f6a1-0784-28e7-f34a9d6d3c93@gmail.com>
Ben Peart <peartben@gmail.com> writes:
> On 7/21/2018 2:34 AM, Elijah Newren wrote:
>> From: Ben Peart <peartben@gmail.com>
>>
>> Signed-off-by: Elijah Newren <newren@gmail.com>
>> ---
>> Testcase provided by Ben, so committing with him as the author. Just need
>> a sign off from him.
>
> Thanks Elijah, consider it
>
> Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
>
Thanks; I'll also tweak the in-body From: line while applying to
match the Sign-off.
^ permalink raw reply
* Re: [PATCH 1/2] Add sw2_sw4 voltage table to cpcap regulator.
From: Mark Brown @ 2018-07-23 18:13 UTC (permalink / raw)
To: Peter Geis
Cc: lgirdwood, robh+dt, mark.rutland, linux-kernel, devicetree,
linux-tegra
In-Reply-To: <e411faa3-7d42-710f-a18a-a4fdd764b60a@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 505 bytes --]
On Mon, Jul 23, 2018 at 01:58:26PM -0400, Peter Geis wrote:
> SW2 and SW4 use a shared table to provide voltage to the cpu core and
> devices on Tegra hardware.
> Added this table to the cpcap regulator driver as the first step to
> supporting this device on Tegra.
This also doesn't apply against current code (though it does now parse
OK), please check and resend - make sure you don't have other out of
tree changes and are using an up to date kernel (ideally my regulator
for-next branch) as a base.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH i2c-next] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-07-23 18:13 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <58d31319-83c0-969b-e3fd-4273818929fc@linux.intel.com>
Thanks James for the review. Please see my inline answers.
On 7/23/2018 11:10 AM, James Feist wrote:
> On 07/23/2018 10:48 AM, Jae Hyun Yoo wrote:
>> In most of cases, interrupt bits are set one by one but there are
>> also a lot of other cases that Aspeed I2C IP sends multiple
>> interrupt bits with combining master and slave events using a
>> single interrupt call. It happens much in multi-master environment
>
> much more
>
Thanks! Will fix it.
>> than single-master. For an example, when master is waiting for a
>> NORMAL_STOP interrupt in its MASTER_STOP state, SLAVE_MATCH and
>> RX_DONE interrupts could come along with the NORMAL_STOP in case of
>> an another master immediately sends data just after acquiring the
>> bus. In this case, the NORMAL_STOP interrupt should be handled
>> by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
>> handled by slave_irq. This commit modifies irq hadling logic to
>> handle the master/slave combined events properly.
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> ---
>> ? drivers/i2c/busses/i2c-aspeed.c | 137 ++++++++++++++++++--------------
>> ? 1 file changed, 76 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c
>> b/drivers/i2c/busses/i2c-aspeed.c
>> index efb89422d496..24d43f143a55 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -82,6 +82,11 @@
>> ? #define ASPEED_I2CD_INTR_RX_DONE??????????? BIT(2)
>> ? #define ASPEED_I2CD_INTR_TX_NAK??????????????? BIT(1)
>> ? #define ASPEED_I2CD_INTR_TX_ACK??????????????? BIT(0)
>> +#define ASPEED_I2CD_INTR_ERRORS?????????????????????????????? \
>> +??????? (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |?????????????????? \
>> +???????? ASPEED_I2CD_INTR_SCL_TIMEOUT |?????????????????????? \
>> +???????? ASPEED_I2CD_INTR_ABNORMAL |?????????????????????? \
>> +???????? ASPEED_I2CD_INTR_ARBIT_LOSS)
>> ? #define ASPEED_I2CD_INTR_ALL?????????????????????????????? \
>> ????????? (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |?????????????????? \
>> ?????????? ASPEED_I2CD_INTR_BUS_RECOVER_DONE |?????????????????? \
>> @@ -150,6 +155,7 @@ struct aspeed_i2c_bus {
>> ????? int??????????????? cmd_err;
>> ????? /* Protected only by i2c_lock_bus */
>> ????? int??????????????? master_xfer_result;
>> +??? u32??????????????? irq_status;
>> ? #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> ????? struct i2c_client??????? *slave;
>> ????? enum aspeed_i2c_slave_state??? slave_state;
>> @@ -229,36 +235,30 @@ static int aspeed_i2c_recover_bus(struct
>> aspeed_i2c_bus *bus)
>> ? #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> ? static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
>> ? {
>> -??? u32 command, irq_status, status_ack = 0;
>> +??? u32 command, status_ack = 0;
>> ????? struct i2c_client *slave = bus->slave;
>> -??? bool irq_handled = true;
>> ????? u8 value;
>> -??? if (!slave) {
>> -??????? irq_handled = false;
>> -??????? goto out;
>> -??? }
>> +??? if (!slave)
>> +??????? return false;
>> ????? command = readl(bus->base + ASPEED_I2C_CMD_REG);
>> -??? irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> ????? /* Slave was requested, restart state machine. */
>> -??? if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
>> +??? if (bus->irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
>> ????????? status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
>> ????????? bus->slave_state = ASPEED_I2C_SLAVE_START;
>> ????? }
>> ????? /* Slave is not currently active, irq was for someone else. */
>> -??? if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
>> -??????? irq_handled = false;
>> -??????? goto out;
>> -??? }
>> +??? if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
>> +??????? return false;
>> ????? dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
>> -??????? irq_status, command);
>> +??????? bus->irq_status, command);
>> ????? /* Slave was sent something. */
>> -??? if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
>> +??? if (bus->irq_status & ASPEED_I2CD_INTR_RX_DONE) {
>> ????????? value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
>> ????????? /* Handle address frame. */
>> ????????? if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
>> @@ -273,28 +273,29 @@ static bool aspeed_i2c_slave_irq(struct
>> aspeed_i2c_bus *bus)
>> ????? }
>> ????? /* Slave was asked to stop. */
>> -??? if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
>> +??? if (bus->irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
>> ????????? status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
>> ????????? bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>> ????? }
>> -??? if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
>> +??? if (bus->irq_status & ASPEED_I2CD_INTR_TX_NAK) {
>> ????????? status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> ????????? bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>> ????? }
>> +??? if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK) {
>> +??????? status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> +??? }
>> ????? switch (bus->slave_state) {
>> ????? case ASPEED_I2C_SLAVE_READ_REQUESTED:
>> -??????? if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
>> +??????? if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK)
>> ????????????? dev_err(bus->dev, "Unexpected ACK on read request.\n");
>> ????????? bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
>> -
>> ????????? i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
>> ????????? writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
>> ????????? writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
>> ????????? break;
>> ????? case ASPEED_I2C_SLAVE_READ_PROCESSED:
>> -??????? status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> -??????? if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
>> +??????? if (!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))
>> ????????????? dev_err(bus->dev,
>> ????????????????? "Expected ACK after processed read.\n");
>> ????????? i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value);
>> @@ -317,14 +318,8 @@ static bool aspeed_i2c_slave_irq(struct
>> aspeed_i2c_bus *bus)
>> ????????? break;
>> ????? }
>> -??? if (status_ack != irq_status)
>> -??????? dev_err(bus->dev,
>> -??????????? "irq handled != irq. expected %x, but was %x\n",
>> -??????????? irq_status, status_ack);
>> -??? writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
>> -
>> -out:
>> -??? return irq_handled;
>> +??? bus->irq_status ^= status_ack;
>> +??? return !bus->irq_status;
>> ? }
>> ? #endif /* CONFIG_I2C_SLAVE */
>> @@ -382,19 +377,19 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
>> ? static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> ? {
>> -??? u32 irq_status, status_ack = 0, command = 0;
>> +??? u32 status_ack = 0, command = 0;
>> ????? struct i2c_msg *msg;
>> ????? u8 recv_byte;
>> ????? int ret;
>> -??? irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> -??? /* Ack all interrupt bits. */
>> -??? writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
>> -
>> -??? if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
>> +??? if (bus->irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
>> ????????? bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> ????????? status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
>> ????????? goto out_complete;
>> +??? } else {
>> +??????? /* Master is not currently active, irq was for someone else. */
>> +??????? if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE)
>> +??????????? goto out_no_complete;
>> ????? }
>> ????? /*
>> @@ -402,20 +397,23 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ?????? * should clear the command queue effectively taking us back to the
>> ?????? * INACTIVE state.
>> ?????? */
>> -??? ret = aspeed_i2c_is_irq_error(irq_status);
>> -??? if (ret < 0) {
>> +??? ret = aspeed_i2c_is_irq_error(bus->irq_status);
>> +??? if (ret) {
>> ????????? dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
>> -??????????? irq_status);
>> +??????????? bus->irq_status);
>> ????????? bus->cmd_err = ret;
>> ????????? bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> +??????? status_ack |= (bus->irq_status & ASPEED_I2CD_INTR_ERRORS);
>> ????????? goto out_complete;
>> ????? }
>> ????? /* We are in an invalid state; reset bus to a known state. */
>> ????? if (!bus->msgs) {
>> -??????? dev_err(bus->dev, "bus in unknown state\n");
>> +??????? dev_err(bus->dev, "bus in unknown state irq_status: 0x%x\n",
>> +??????????? bus->irq_status);
>> ????????? bus->cmd_err = -EIO;
>> -??????? if (bus->master_state != ASPEED_I2C_MASTER_STOP)
>> +??????? if (bus->master_state != ASPEED_I2C_MASTER_STOP &&
>> +??????????? bus->master_state != ASPEED_I2C_MASTER_INACTIVE)
>> ????????????? aspeed_i2c_do_stop(bus);
>> ????????? goto out_no_complete;
>> ????? }
>> @@ -427,8 +425,14 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ?????? * then update the state and handle the new state below.
>> ?????? */
>> ????? if (bus->master_state == ASPEED_I2C_MASTER_START) {
>> -??????? if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> -??????????? pr_devel("no slave present at %02x\n", msg->addr);
>> +??????? if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> +??????????? if (unlikely(!(bus->irq_status &
>> +???????????????????? ASPEED_I2CD_INTR_TX_NAK))) {
>> +??????????????? bus->cmd_err = -ENXIO;
>> +??????????????? bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> +??????????????? goto out_complete;
>> +??????????? }
>> +??????????? pr_devel("no slave present at %02x", msg->addr);
> Missing line feed character
Thanks for your pointing it out. Will add '\n' at the end of the
message.
>> ????????????? status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> ????????????? bus->cmd_err = -ENXIO;
>> ????????????? aspeed_i2c_do_stop(bus);
>> @@ -447,11 +451,12 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ????? switch (bus->master_state) {
>> ????? case ASPEED_I2C_MASTER_TX:
>> -??????? if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
>> +??????? if (unlikely(bus->irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
>> ????????????? dev_dbg(bus->dev, "slave NACKed TX\n");
>> ????????????? status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> ????????????? goto error_and_stop;
>> -??????? } else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> +??????? } else if (unlikely(!(bus->irq_status &
>> +????????????????????? ASPEED_I2CD_INTR_TX_ACK))) {
>> ????????????? dev_err(bus->dev, "slave failed to ACK TX\n");
>> ????????????? goto error_and_stop;
>> ????????? }
>> @@ -470,11 +475,11 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ????????? goto out_no_complete;
>> ????? case ASPEED_I2C_MASTER_RX_FIRST:
>> ????????? /* RX may not have completed yet (only address cycle) */
>> -??????? if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE))
>> +??????? if (!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))
>> ????????????? goto out_no_complete;
>> ????????? /* fallthrough intended */
>> ????? case ASPEED_I2C_MASTER_RX:
>> -??????? if (unlikely(!(irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
>> +??????? if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
>> ????????????? dev_err(bus->dev, "master failed to RX\n");
>> ????????????? goto error_and_stop;
>> ????????? }
>> @@ -505,8 +510,11 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ????????? }
>> ????????? goto out_no_complete;
>> ????? case ASPEED_I2C_MASTER_STOP:
>> -??????? if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
>> -??????????? dev_err(bus->dev, "master failed to STOP\n");
>> +??????? if (unlikely(!(bus->irq_status &
>> +?????????????????? ASPEED_I2CD_INTR_NORMAL_STOP))) {
>> +??????????? dev_err(bus->dev,
>> +??????????????? "master failed to STOP irq_status:0x%x\n",
>> +??????????????? bus->irq_status);
>> ????????????? bus->cmd_err = -EIO;
>> ????????????? /* Do not STOP as we have already tried. */
>> ????????? } else {
>> @@ -518,7 +526,7 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ????? case ASPEED_I2C_MASTER_INACTIVE:
>> ????????? dev_err(bus->dev,
>> ????????????? "master received interrupt 0x%08x, but is inactive\n",
>> -??????????? irq_status);
>> +??????????? bus->irq_status);
>> ????????? bus->cmd_err = -EIO;
>> ????????? /* Do not STOP as we should be inactive. */
>> ????????? goto out_complete;
>> @@ -540,33 +548,40 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ????????? bus->master_xfer_result = bus->msgs_index + 1;
>> ????? complete(&bus->cmd_complete);
>> ? out_no_complete:
>> -??? if (irq_status != status_ack)
>> -??????? dev_err(bus->dev,
>> -??????????? "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>> -??????????? irq_status, status_ack);
>> -??? return !!irq_status;
>> +??? bus->irq_status ^= status_ack;
>> +??? return !bus->irq_status;
>> ? }
>> ? static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>> ? {
>> ????? struct aspeed_i2c_bus *bus = dev_id;
>> -??? bool ret;
>> +??? u32 irq_received;
>> ????? spin_lock(&bus->lock);
>> +??? irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> +??? bus->irq_status = irq_received;
>> ? #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> -??? if (aspeed_i2c_slave_irq(bus)) {
>> -??????? dev_dbg(bus->dev, "irq handled by slave.\n");
>> -??????? ret = true;
>> -??????? goto out;
>> +??? if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
>> +??????? if (!aspeed_i2c_master_irq(bus))
>> +??????????? aspeed_i2c_slave_irq(bus);
>> +??? } else {
>> +??????? if (!aspeed_i2c_slave_irq(bus))
>> +??????????? aspeed_i2c_master_irq(bus);
>> ????? }
>> +#else
>> +??? aspeed_i2c_master_irq(bus);
>> ? #endif /* CONFIG_I2C_SLAVE */
>> -??? ret = aspeed_i2c_master_irq(bus);
>> +??? if (bus->irq_status)
>> +??????? dev_err(bus->dev,
>> +??????????? "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>> +??????????? irq_received, irq_received ^ bus->irq_status);
>> -out:
>> +??? /* Ack all interrupt bits. */
>> +??? writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>> ????? spin_unlock(&bus->lock);
>> -??? return ret ? IRQ_HANDLED : IRQ_NONE;
>> +??? return bus->irq_status ? IRQ_NONE : IRQ_HANDLED;
>> ? }
>> ? static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
>>
^ permalink raw reply
* Re: [PATCH i2c-next] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-07-23 18:13 UTC (permalink / raw)
To: James Feist, Brendan Higgins, Benjamin Herrenschmidt,
Joel Stanley, Andrew Jeffery, linux-i2c, openbmc,
linux-arm-kernel, linux-aspeed, linux-kernel
Cc: Vernon Mauery, Jarkko Nikula
In-Reply-To: <58d31319-83c0-969b-e3fd-4273818929fc@linux.intel.com>
Thanks James for the review. Please see my inline answers.
On 7/23/2018 11:10 AM, James Feist wrote:
> On 07/23/2018 10:48 AM, Jae Hyun Yoo wrote:
>> In most of cases, interrupt bits are set one by one but there are
>> also a lot of other cases that Aspeed I2C IP sends multiple
>> interrupt bits with combining master and slave events using a
>> single interrupt call. It happens much in multi-master environment
>
> much more
>
Thanks! Will fix it.
>> than single-master. For an example, when master is waiting for a
>> NORMAL_STOP interrupt in its MASTER_STOP state, SLAVE_MATCH and
>> RX_DONE interrupts could come along with the NORMAL_STOP in case of
>> an another master immediately sends data just after acquiring the
>> bus. In this case, the NORMAL_STOP interrupt should be handled
>> by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
>> handled by slave_irq. This commit modifies irq hadling logic to
>> handle the master/slave combined events properly.
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> ---
>> drivers/i2c/busses/i2c-aspeed.c | 137 ++++++++++++++++++--------------
>> 1 file changed, 76 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c
>> b/drivers/i2c/busses/i2c-aspeed.c
>> index efb89422d496..24d43f143a55 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -82,6 +82,11 @@
>> #define ASPEED_I2CD_INTR_RX_DONE BIT(2)
>> #define ASPEED_I2CD_INTR_TX_NAK BIT(1)
>> #define ASPEED_I2CD_INTR_TX_ACK BIT(0)
>> +#define ASPEED_I2CD_INTR_ERRORS \
>> + (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
>> + ASPEED_I2CD_INTR_SCL_TIMEOUT | \
>> + ASPEED_I2CD_INTR_ABNORMAL | \
>> + ASPEED_I2CD_INTR_ARBIT_LOSS)
>> #define ASPEED_I2CD_INTR_ALL \
>> (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
>> ASPEED_I2CD_INTR_BUS_RECOVER_DONE | \
>> @@ -150,6 +155,7 @@ struct aspeed_i2c_bus {
>> int cmd_err;
>> /* Protected only by i2c_lock_bus */
>> int master_xfer_result;
>> + u32 irq_status;
>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> struct i2c_client *slave;
>> enum aspeed_i2c_slave_state slave_state;
>> @@ -229,36 +235,30 @@ static int aspeed_i2c_recover_bus(struct
>> aspeed_i2c_bus *bus)
>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
>> {
>> - u32 command, irq_status, status_ack = 0;
>> + u32 command, status_ack = 0;
>> struct i2c_client *slave = bus->slave;
>> - bool irq_handled = true;
>> u8 value;
>> - if (!slave) {
>> - irq_handled = false;
>> - goto out;
>> - }
>> + if (!slave)
>> + return false;
>> command = readl(bus->base + ASPEED_I2C_CMD_REG);
>> - irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> /* Slave was requested, restart state machine. */
>> - if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
>> + if (bus->irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
>> status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
>> bus->slave_state = ASPEED_I2C_SLAVE_START;
>> }
>> /* Slave is not currently active, irq was for someone else. */
>> - if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
>> - irq_handled = false;
>> - goto out;
>> - }
>> + if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
>> + return false;
>> dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
>> - irq_status, command);
>> + bus->irq_status, command);
>> /* Slave was sent something. */
>> - if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
>> + if (bus->irq_status & ASPEED_I2CD_INTR_RX_DONE) {
>> value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
>> /* Handle address frame. */
>> if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
>> @@ -273,28 +273,29 @@ static bool aspeed_i2c_slave_irq(struct
>> aspeed_i2c_bus *bus)
>> }
>> /* Slave was asked to stop. */
>> - if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
>> + if (bus->irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
>> status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
>> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>> }
>> - if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
>> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_NAK) {
>> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>> }
>> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK) {
>> + status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> + }
>> switch (bus->slave_state) {
>> case ASPEED_I2C_SLAVE_READ_REQUESTED:
>> - if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
>> + if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK)
>> dev_err(bus->dev, "Unexpected ACK on read request.\n");
>> bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
>> -
>> i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
>> writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
>> writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
>> break;
>> case ASPEED_I2C_SLAVE_READ_PROCESSED:
>> - status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> - if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
>> + if (!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))
>> dev_err(bus->dev,
>> "Expected ACK after processed read.\n");
>> i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value);
>> @@ -317,14 +318,8 @@ static bool aspeed_i2c_slave_irq(struct
>> aspeed_i2c_bus *bus)
>> break;
>> }
>> - if (status_ack != irq_status)
>> - dev_err(bus->dev,
>> - "irq handled != irq. expected %x, but was %x\n",
>> - irq_status, status_ack);
>> - writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
>> -
>> -out:
>> - return irq_handled;
>> + bus->irq_status ^= status_ack;
>> + return !bus->irq_status;
>> }
>> #endif /* CONFIG_I2C_SLAVE */
>> @@ -382,19 +377,19 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
>> static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> {
>> - u32 irq_status, status_ack = 0, command = 0;
>> + u32 status_ack = 0, command = 0;
>> struct i2c_msg *msg;
>> u8 recv_byte;
>> int ret;
>> - irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> - /* Ack all interrupt bits. */
>> - writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
>> -
>> - if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
>> + if (bus->irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
>> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
>> goto out_complete;
>> + } else {
>> + /* Master is not currently active, irq was for someone else. */
>> + if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE)
>> + goto out_no_complete;
>> }
>> /*
>> @@ -402,20 +397,23 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> * should clear the command queue effectively taking us back to the
>> * INACTIVE state.
>> */
>> - ret = aspeed_i2c_is_irq_error(irq_status);
>> - if (ret < 0) {
>> + ret = aspeed_i2c_is_irq_error(bus->irq_status);
>> + if (ret) {
>> dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
>> - irq_status);
>> + bus->irq_status);
>> bus->cmd_err = ret;
>> bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> + status_ack |= (bus->irq_status & ASPEED_I2CD_INTR_ERRORS);
>> goto out_complete;
>> }
>> /* We are in an invalid state; reset bus to a known state. */
>> if (!bus->msgs) {
>> - dev_err(bus->dev, "bus in unknown state\n");
>> + dev_err(bus->dev, "bus in unknown state irq_status: 0x%x\n",
>> + bus->irq_status);
>> bus->cmd_err = -EIO;
>> - if (bus->master_state != ASPEED_I2C_MASTER_STOP)
>> + if (bus->master_state != ASPEED_I2C_MASTER_STOP &&
>> + bus->master_state != ASPEED_I2C_MASTER_INACTIVE)
>> aspeed_i2c_do_stop(bus);
>> goto out_no_complete;
>> }
>> @@ -427,8 +425,14 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> * then update the state and handle the new state below.
>> */
>> if (bus->master_state == ASPEED_I2C_MASTER_START) {
>> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> - pr_devel("no slave present at %02x\n", msg->addr);
>> + if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> + if (unlikely(!(bus->irq_status &
>> + ASPEED_I2CD_INTR_TX_NAK))) {
>> + bus->cmd_err = -ENXIO;
>> + bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> + goto out_complete;
>> + }
>> + pr_devel("no slave present at %02x", msg->addr);
> Missing line feed character
Thanks for your pointing it out. Will add '\n' at the end of the
message.
>> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> bus->cmd_err = -ENXIO;
>> aspeed_i2c_do_stop(bus);
>> @@ -447,11 +451,12 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> switch (bus->master_state) {
>> case ASPEED_I2C_MASTER_TX:
>> - if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
>> + if (unlikely(bus->irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
>> dev_dbg(bus->dev, "slave NACKed TX\n");
>> status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> goto error_and_stop;
>> - } else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> + } else if (unlikely(!(bus->irq_status &
>> + ASPEED_I2CD_INTR_TX_ACK))) {
>> dev_err(bus->dev, "slave failed to ACK TX\n");
>> goto error_and_stop;
>> }
>> @@ -470,11 +475,11 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> goto out_no_complete;
>> case ASPEED_I2C_MASTER_RX_FIRST:
>> /* RX may not have completed yet (only address cycle) */
>> - if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE))
>> + if (!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))
>> goto out_no_complete;
>> /* fallthrough intended */
>> case ASPEED_I2C_MASTER_RX:
>> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
>> + if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
>> dev_err(bus->dev, "master failed to RX\n");
>> goto error_and_stop;
>> }
>> @@ -505,8 +510,11 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> }
>> goto out_no_complete;
>> case ASPEED_I2C_MASTER_STOP:
>> - if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
>> - dev_err(bus->dev, "master failed to STOP\n");
>> + if (unlikely(!(bus->irq_status &
>> + ASPEED_I2CD_INTR_NORMAL_STOP))) {
>> + dev_err(bus->dev,
>> + "master failed to STOP irq_status:0x%x\n",
>> + bus->irq_status);
>> bus->cmd_err = -EIO;
>> /* Do not STOP as we have already tried. */
>> } else {
>> @@ -518,7 +526,7 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> case ASPEED_I2C_MASTER_INACTIVE:
>> dev_err(bus->dev,
>> "master received interrupt 0x%08x, but is inactive\n",
>> - irq_status);
>> + bus->irq_status);
>> bus->cmd_err = -EIO;
>> /* Do not STOP as we should be inactive. */
>> goto out_complete;
>> @@ -540,33 +548,40 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> bus->master_xfer_result = bus->msgs_index + 1;
>> complete(&bus->cmd_complete);
>> out_no_complete:
>> - if (irq_status != status_ack)
>> - dev_err(bus->dev,
>> - "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>> - irq_status, status_ack);
>> - return !!irq_status;
>> + bus->irq_status ^= status_ack;
>> + return !bus->irq_status;
>> }
>> static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>> {
>> struct aspeed_i2c_bus *bus = dev_id;
>> - bool ret;
>> + u32 irq_received;
>> spin_lock(&bus->lock);
>> + irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> + bus->irq_status = irq_received;
>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> - if (aspeed_i2c_slave_irq(bus)) {
>> - dev_dbg(bus->dev, "irq handled by slave.\n");
>> - ret = true;
>> - goto out;
>> + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
>> + if (!aspeed_i2c_master_irq(bus))
>> + aspeed_i2c_slave_irq(bus);
>> + } else {
>> + if (!aspeed_i2c_slave_irq(bus))
>> + aspeed_i2c_master_irq(bus);
>> }
>> +#else
>> + aspeed_i2c_master_irq(bus);
>> #endif /* CONFIG_I2C_SLAVE */
>> - ret = aspeed_i2c_master_irq(bus);
>> + if (bus->irq_status)
>> + dev_err(bus->dev,
>> + "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>> + irq_received, irq_received ^ bus->irq_status);
>> -out:
>> + /* Ack all interrupt bits. */
>> + writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>> spin_unlock(&bus->lock);
>> - return ret ? IRQ_HANDLED : IRQ_NONE;
>> + return bus->irq_status ? IRQ_NONE : IRQ_HANDLED;
>> }
>> static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
>>
^ permalink raw reply
* [PATCH i2c-next] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-07-23 18:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <58d31319-83c0-969b-e3fd-4273818929fc@linux.intel.com>
Thanks James for the review. Please see my inline answers.
On 7/23/2018 11:10 AM, James Feist wrote:
> On 07/23/2018 10:48 AM, Jae Hyun Yoo wrote:
>> In most of cases, interrupt bits are set one by one but there are
>> also a lot of other cases that Aspeed I2C IP sends multiple
>> interrupt bits with combining master and slave events using a
>> single interrupt call. It happens much in multi-master environment
>
> much more
>
Thanks! Will fix it.
>> than single-master. For an example, when master is waiting for a
>> NORMAL_STOP interrupt in its MASTER_STOP state, SLAVE_MATCH and
>> RX_DONE interrupts could come along with the NORMAL_STOP in case of
>> an another master immediately sends data just after acquiring the
>> bus. In this case, the NORMAL_STOP interrupt should be handled
>> by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
>> handled by slave_irq. This commit modifies irq hadling logic to
>> handle the master/slave combined events properly.
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> ---
>> ? drivers/i2c/busses/i2c-aspeed.c | 137 ++++++++++++++++++--------------
>> ? 1 file changed, 76 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c
>> b/drivers/i2c/busses/i2c-aspeed.c
>> index efb89422d496..24d43f143a55 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -82,6 +82,11 @@
>> ? #define ASPEED_I2CD_INTR_RX_DONE??????????? BIT(2)
>> ? #define ASPEED_I2CD_INTR_TX_NAK??????????????? BIT(1)
>> ? #define ASPEED_I2CD_INTR_TX_ACK??????????????? BIT(0)
>> +#define ASPEED_I2CD_INTR_ERRORS?????????????????????????????? \
>> +??????? (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |?????????????????? \
>> +???????? ASPEED_I2CD_INTR_SCL_TIMEOUT |?????????????????????? \
>> +???????? ASPEED_I2CD_INTR_ABNORMAL |?????????????????????? \
>> +???????? ASPEED_I2CD_INTR_ARBIT_LOSS)
>> ? #define ASPEED_I2CD_INTR_ALL?????????????????????????????? \
>> ????????? (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |?????????????????? \
>> ?????????? ASPEED_I2CD_INTR_BUS_RECOVER_DONE |?????????????????? \
>> @@ -150,6 +155,7 @@ struct aspeed_i2c_bus {
>> ????? int??????????????? cmd_err;
>> ????? /* Protected only by i2c_lock_bus */
>> ????? int??????????????? master_xfer_result;
>> +??? u32??????????????? irq_status;
>> ? #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> ????? struct i2c_client??????? *slave;
>> ????? enum aspeed_i2c_slave_state??? slave_state;
>> @@ -229,36 +235,30 @@ static int aspeed_i2c_recover_bus(struct
>> aspeed_i2c_bus *bus)
>> ? #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> ? static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
>> ? {
>> -??? u32 command, irq_status, status_ack = 0;
>> +??? u32 command, status_ack = 0;
>> ????? struct i2c_client *slave = bus->slave;
>> -??? bool irq_handled = true;
>> ????? u8 value;
>> -??? if (!slave) {
>> -??????? irq_handled = false;
>> -??????? goto out;
>> -??? }
>> +??? if (!slave)
>> +??????? return false;
>> ????? command = readl(bus->base + ASPEED_I2C_CMD_REG);
>> -??? irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> ????? /* Slave was requested, restart state machine. */
>> -??? if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
>> +??? if (bus->irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
>> ????????? status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
>> ????????? bus->slave_state = ASPEED_I2C_SLAVE_START;
>> ????? }
>> ????? /* Slave is not currently active, irq was for someone else. */
>> -??? if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
>> -??????? irq_handled = false;
>> -??????? goto out;
>> -??? }
>> +??? if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
>> +??????? return false;
>> ????? dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
>> -??????? irq_status, command);
>> +??????? bus->irq_status, command);
>> ????? /* Slave was sent something. */
>> -??? if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
>> +??? if (bus->irq_status & ASPEED_I2CD_INTR_RX_DONE) {
>> ????????? value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
>> ????????? /* Handle address frame. */
>> ????????? if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
>> @@ -273,28 +273,29 @@ static bool aspeed_i2c_slave_irq(struct
>> aspeed_i2c_bus *bus)
>> ????? }
>> ????? /* Slave was asked to stop. */
>> -??? if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
>> +??? if (bus->irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
>> ????????? status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
>> ????????? bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>> ????? }
>> -??? if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
>> +??? if (bus->irq_status & ASPEED_I2CD_INTR_TX_NAK) {
>> ????????? status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> ????????? bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>> ????? }
>> +??? if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK) {
>> +??????? status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> +??? }
>> ????? switch (bus->slave_state) {
>> ????? case ASPEED_I2C_SLAVE_READ_REQUESTED:
>> -??????? if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
>> +??????? if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK)
>> ????????????? dev_err(bus->dev, "Unexpected ACK on read request.\n");
>> ????????? bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
>> -
>> ????????? i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
>> ????????? writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
>> ????????? writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
>> ????????? break;
>> ????? case ASPEED_I2C_SLAVE_READ_PROCESSED:
>> -??????? status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> -??????? if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
>> +??????? if (!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))
>> ????????????? dev_err(bus->dev,
>> ????????????????? "Expected ACK after processed read.\n");
>> ????????? i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value);
>> @@ -317,14 +318,8 @@ static bool aspeed_i2c_slave_irq(struct
>> aspeed_i2c_bus *bus)
>> ????????? break;
>> ????? }
>> -??? if (status_ack != irq_status)
>> -??????? dev_err(bus->dev,
>> -??????????? "irq handled != irq. expected %x, but was %x\n",
>> -??????????? irq_status, status_ack);
>> -??? writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
>> -
>> -out:
>> -??? return irq_handled;
>> +??? bus->irq_status ^= status_ack;
>> +??? return !bus->irq_status;
>> ? }
>> ? #endif /* CONFIG_I2C_SLAVE */
>> @@ -382,19 +377,19 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
>> ? static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> ? {
>> -??? u32 irq_status, status_ack = 0, command = 0;
>> +??? u32 status_ack = 0, command = 0;
>> ????? struct i2c_msg *msg;
>> ????? u8 recv_byte;
>> ????? int ret;
>> -??? irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> -??? /* Ack all interrupt bits. */
>> -??? writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
>> -
>> -??? if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
>> +??? if (bus->irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
>> ????????? bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> ????????? status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
>> ????????? goto out_complete;
>> +??? } else {
>> +??????? /* Master is not currently active, irq was for someone else. */
>> +??????? if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE)
>> +??????????? goto out_no_complete;
>> ????? }
>> ????? /*
>> @@ -402,20 +397,23 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ?????? * should clear the command queue effectively taking us back to the
>> ?????? * INACTIVE state.
>> ?????? */
>> -??? ret = aspeed_i2c_is_irq_error(irq_status);
>> -??? if (ret < 0) {
>> +??? ret = aspeed_i2c_is_irq_error(bus->irq_status);
>> +??? if (ret) {
>> ????????? dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
>> -??????????? irq_status);
>> +??????????? bus->irq_status);
>> ????????? bus->cmd_err = ret;
>> ????????? bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> +??????? status_ack |= (bus->irq_status & ASPEED_I2CD_INTR_ERRORS);
>> ????????? goto out_complete;
>> ????? }
>> ????? /* We are in an invalid state; reset bus to a known state. */
>> ????? if (!bus->msgs) {
>> -??????? dev_err(bus->dev, "bus in unknown state\n");
>> +??????? dev_err(bus->dev, "bus in unknown state irq_status: 0x%x\n",
>> +??????????? bus->irq_status);
>> ????????? bus->cmd_err = -EIO;
>> -??????? if (bus->master_state != ASPEED_I2C_MASTER_STOP)
>> +??????? if (bus->master_state != ASPEED_I2C_MASTER_STOP &&
>> +??????????? bus->master_state != ASPEED_I2C_MASTER_INACTIVE)
>> ????????????? aspeed_i2c_do_stop(bus);
>> ????????? goto out_no_complete;
>> ????? }
>> @@ -427,8 +425,14 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ?????? * then update the state and handle the new state below.
>> ?????? */
>> ????? if (bus->master_state == ASPEED_I2C_MASTER_START) {
>> -??????? if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> -??????????? pr_devel("no slave present at %02x\n", msg->addr);
>> +??????? if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> +??????????? if (unlikely(!(bus->irq_status &
>> +???????????????????? ASPEED_I2CD_INTR_TX_NAK))) {
>> +??????????????? bus->cmd_err = -ENXIO;
>> +??????????????? bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> +??????????????? goto out_complete;
>> +??????????? }
>> +??????????? pr_devel("no slave present at %02x", msg->addr);
> Missing line feed character
Thanks for your pointing it out. Will add '\n' at the end of the
message.
>> ????????????? status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> ????????????? bus->cmd_err = -ENXIO;
>> ????????????? aspeed_i2c_do_stop(bus);
>> @@ -447,11 +451,12 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ????? switch (bus->master_state) {
>> ????? case ASPEED_I2C_MASTER_TX:
>> -??????? if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
>> +??????? if (unlikely(bus->irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
>> ????????????? dev_dbg(bus->dev, "slave NACKed TX\n");
>> ????????????? status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> ????????????? goto error_and_stop;
>> -??????? } else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> +??????? } else if (unlikely(!(bus->irq_status &
>> +????????????????????? ASPEED_I2CD_INTR_TX_ACK))) {
>> ????????????? dev_err(bus->dev, "slave failed to ACK TX\n");
>> ????????????? goto error_and_stop;
>> ????????? }
>> @@ -470,11 +475,11 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ????????? goto out_no_complete;
>> ????? case ASPEED_I2C_MASTER_RX_FIRST:
>> ????????? /* RX may not have completed yet (only address cycle) */
>> -??????? if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE))
>> +??????? if (!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))
>> ????????????? goto out_no_complete;
>> ????????? /* fallthrough intended */
>> ????? case ASPEED_I2C_MASTER_RX:
>> -??????? if (unlikely(!(irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
>> +??????? if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
>> ????????????? dev_err(bus->dev, "master failed to RX\n");
>> ????????????? goto error_and_stop;
>> ????????? }
>> @@ -505,8 +510,11 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ????????? }
>> ????????? goto out_no_complete;
>> ????? case ASPEED_I2C_MASTER_STOP:
>> -??????? if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
>> -??????????? dev_err(bus->dev, "master failed to STOP\n");
>> +??????? if (unlikely(!(bus->irq_status &
>> +?????????????????? ASPEED_I2CD_INTR_NORMAL_STOP))) {
>> +??????????? dev_err(bus->dev,
>> +??????????????? "master failed to STOP irq_status:0x%x\n",
>> +??????????????? bus->irq_status);
>> ????????????? bus->cmd_err = -EIO;
>> ????????????? /* Do not STOP as we have already tried. */
>> ????????? } else {
>> @@ -518,7 +526,7 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ????? case ASPEED_I2C_MASTER_INACTIVE:
>> ????????? dev_err(bus->dev,
>> ????????????? "master received interrupt 0x%08x, but is inactive\n",
>> -??????????? irq_status);
>> +??????????? bus->irq_status);
>> ????????? bus->cmd_err = -EIO;
>> ????????? /* Do not STOP as we should be inactive. */
>> ????????? goto out_complete;
>> @@ -540,33 +548,40 @@ static bool aspeed_i2c_master_irq(struct
>> aspeed_i2c_bus *bus)
>> ????????? bus->master_xfer_result = bus->msgs_index + 1;
>> ????? complete(&bus->cmd_complete);
>> ? out_no_complete:
>> -??? if (irq_status != status_ack)
>> -??????? dev_err(bus->dev,
>> -??????????? "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>> -??????????? irq_status, status_ack);
>> -??? return !!irq_status;
>> +??? bus->irq_status ^= status_ack;
>> +??? return !bus->irq_status;
>> ? }
>> ? static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>> ? {
>> ????? struct aspeed_i2c_bus *bus = dev_id;
>> -??? bool ret;
>> +??? u32 irq_received;
>> ????? spin_lock(&bus->lock);
>> +??? irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> +??? bus->irq_status = irq_received;
>> ? #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> -??? if (aspeed_i2c_slave_irq(bus)) {
>> -??????? dev_dbg(bus->dev, "irq handled by slave.\n");
>> -??????? ret = true;
>> -??????? goto out;
>> +??? if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
>> +??????? if (!aspeed_i2c_master_irq(bus))
>> +??????????? aspeed_i2c_slave_irq(bus);
>> +??? } else {
>> +??????? if (!aspeed_i2c_slave_irq(bus))
>> +??????????? aspeed_i2c_master_irq(bus);
>> ????? }
>> +#else
>> +??? aspeed_i2c_master_irq(bus);
>> ? #endif /* CONFIG_I2C_SLAVE */
>> -??? ret = aspeed_i2c_master_irq(bus);
>> +??? if (bus->irq_status)
>> +??????? dev_err(bus->dev,
>> +??????????? "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>> +??????????? irq_received, irq_received ^ bus->irq_status);
>> -out:
>> +??? /* Ack all interrupt bits. */
>> +??? writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>> ????? spin_unlock(&bus->lock);
>> -??? return ret ? IRQ_HANDLED : IRQ_NONE;
>> +??? return bus->irq_status ? IRQ_NONE : IRQ_HANDLED;
>> ? }
>> ? static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
>>
^ permalink raw reply
* Re: [PATCHv3 2/2] mtd: m25p80: restore the status of SPI flash when exiting
From: Brian Norris @ 2018-07-23 18:13 UTC (permalink / raw)
To: Zhiqiang Hou
Cc: linux-mtd, linux-kernel, dwmw2, boris.brezillon, marek.vasut,
richard, cyrille.pitchen
In-Reply-To: <20171206025342.7266-3-Zhiqiang.Hou@nxp.com>
Hello,
I noticed this got merged, but I wanted to put my 2 cents in here:
On Wed, Dec 06, 2017 at 10:53:42AM +0800, Zhiqiang Hou wrote:
> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>
> Restore the status to be compatible with legacy devices.
> Take Freescale eSPI boot for example, it copies (in 3 Byte
> addressing mode) the RCW and bootloader images from SPI flash
> without firing a reset signal previously, so the reboot command
> will fail without reseting the addressing mode of SPI flash.
> This patch implement .shutdown function to restore the status
> in reboot process, and add the same operation to the .remove
> function.
We have previously rejected this patch multiple times, because the above
comment demonstrates a broken product. You cannot guarantee that all
reboots will invoke the .shutdown() method -- what about crashes? What
about watchdog resets? IIUC, those will hit the same broken behavior,
and have unexepcted behavior in your bootloader.
I suppose one could argue for doing this in remove(), but AIUI you're
just papering over system bugs by introducing the shutdown() function
here. Thus, I'd prefer we drop the shutdown() method to avoid misleading
other users of this driver.
Brian
> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> ---
> V3:
> - Modified the commit to make this patch specific.
>
> drivers/mtd/devices/m25p80.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index dbe6a1de2bb8..a4e18f6aaa33 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -307,10 +307,18 @@ static int m25p_remove(struct spi_device *spi)
> {
> struct m25p *flash = spi_get_drvdata(spi);
>
> + spi_nor_restore(&flash->spi_nor);
> +
> /* Clean up MTD stuff. */
> return mtd_device_unregister(&flash->spi_nor.mtd);
> }
>
> +static void m25p_shutdown(struct spi_device *spi)
> +{
> + struct m25p *flash = spi_get_drvdata(spi);
> +
> + spi_nor_restore(&flash->spi_nor);
> +}
> /*
> * Do NOT add to this array without reading the following:
> *
> @@ -386,6 +394,7 @@ static struct spi_driver m25p80_driver = {
> .id_table = m25p_ids,
> .probe = m25p_probe,
> .remove = m25p_remove,
> + .shutdown = m25p_shutdown,
>
> /* REVISIT: many of these chips have deep power-down modes, which
> * should clearly be entered on suspend() to minimize power use.
> --
> 2.14.1
>
^ permalink raw reply
* Re: [PATCH v6 8/8] fetch-pack: implement ref-in-want
From: Duy Nguyen @ 2018-07-23 18:13 UTC (permalink / raw)
To: Brandon Williams
Cc: Git Mailing List, Jonathan Tan, Junio C Hamano, Stefan Beller,
Jonathan Nieder
In-Reply-To: <20180723175318.GB25435@google.com>
On Mon, Jul 23, 2018 at 7:53 PM Brandon Williams <bmwill@google.com> wrote:
>
> On 07/22, Duy Nguyen wrote:
> > On Thu, Jun 28, 2018 at 12:33 AM Brandon Williams <bmwill@google.com> wrote:
> > > +static void receive_wanted_refs(struct packet_reader *reader, struct ref *refs)
> > > +{
> > > + process_section_header(reader, "wanted-refs", 0);
> > > + while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
> > > + struct object_id oid;
> > > + const char *end;
> > > + struct ref *r = NULL;
> > > +
> > > + if (parse_oid_hex(reader->line, &oid, &end) || *end++ != ' ')
> > > + die("expected wanted-ref, got '%s'", reader->line);
> >
> > Could you do a follow and wrap all these strings in _() since this one
> > is already in 'next'?
>
> What criteria is used to determine if something should be translated?
> To me, this looks like a wire-protocol error which would benefit from
> not being translated because it would be easier to grep for if it
> occurs. That and if a user sees this sort of error I don't think that
> they could really do anything about it anyway.
Devs are users too and for me, even if I can read English just fine, I
prefer fully translated interface, not a mix of non-English and
English. Users can still google around and find out about wanted-ref
(at least linux users 10 years ago did). If they show up here asking
for support, we can ask them to translate back if needed (or look into
.po files). We have the same problem anyway if their bug reports
contain other non-English strings.
Besides drawing the line "benefit from (not) being translated" varies
from one developer to another. I think it's just easier and more
consistent to stick to "if it's not machine-readable (or really meant
for devs, like BUG()), translate it" and leave it to translators to
decide.
--
Duy
^ permalink raw reply
* In-Band Firmware Update
From: Patrick Venture @ 2018-07-23 18:13 UTC (permalink / raw)
To: OpenBMC Maillist
I've started to implement the host-tool outside of google3, and
started splitting up the OEM handler that corresponds with it.
However, firstly, I've submitted the design for the protocol for
review, please let me know if you're interested and I'll add you to
the review. IIRC, there was at least one interested party outside of
us.
Patrick
^ permalink raw reply
* Re: [PATCH] fetch-pack: mark die strings for translation
From: Stefan Beller @ 2018-07-23 18:14 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
In-Reply-To: <20180723175635.31323-1-bmwill@google.com>
On Mon, Jul 23, 2018 at 10:56 AM Brandon Williams <bmwill@google.com> wrote:
>
fetch-pack is listed as a plumbing command, which means its prime consumer
is supposedly a machine; But fetch-pack is also used by git-fetch that
is invoked
by a human, who prefers translations.
.. goes reads code...
This translates protocol v2 messages, and p0 messages are translated already,
so this just aligns the new protocol to the old protocol w.r.t. i18n.
Sounds good,
Thanks,
Stefan
[ This message is a gentle hint for better commit messages. ;-) ]
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
> fetch-pack.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 0b4a9f288f..51abee6181 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1245,13 +1245,13 @@ static int process_section_header(struct packet_reader *reader,
> int ret;
>
> if (packet_reader_peek(reader) != PACKET_READ_NORMAL)
> - die("error reading section header '%s'", section);
> + die(_("error reading section header '%s'"), section);
>
> ret = !strcmp(reader->line, section);
>
> if (!peek) {
> if (!ret)
> - die("expected '%s', received '%s'",
> + die(_("expected '%s', received '%s'"),
> section, reader->line);
> packet_reader_read(reader);
> }
> @@ -1289,12 +1289,12 @@ static int process_acks(struct packet_reader *reader, struct oidset *common)
> continue;
> }
>
> - die("unexpected acknowledgment line: '%s'", reader->line);
> + die(_("unexpected acknowledgment line: '%s'"), reader->line);
> }
>
> if (reader->status != PACKET_READ_FLUSH &&
> reader->status != PACKET_READ_DELIM)
> - die("error processing acks: %d", reader->status);
> + die(_("error processing acks: %d"), reader->status);
>
> /* return 0 if no common, 1 if there are common, or 2 if ready */
> return received_ready ? 2 : (received_ack ? 1 : 0);
> @@ -1331,7 +1331,7 @@ static void receive_shallow_info(struct fetch_pack_args *args,
>
> if (reader->status != PACKET_READ_FLUSH &&
> reader->status != PACKET_READ_DELIM)
> - die("error processing shallow info: %d", reader->status);
> + die(_("error processing shallow info: %d"), reader->status);
>
> setup_alternate_shallow(&shallow_lock, &alternate_shallow_file, NULL);
> args->deepen = 1;
> @@ -1346,7 +1346,7 @@ static void receive_wanted_refs(struct packet_reader *reader, struct ref *refs)
> struct ref *r = NULL;
>
> if (parse_oid_hex(reader->line, &oid, &end) || *end++ != ' ')
> - die("expected wanted-ref, got '%s'", reader->line);
> + die(_("expected wanted-ref, got '%s'"), reader->line);
>
> for (r = refs; r; r = r->next) {
> if (!strcmp(end, r->name)) {
> @@ -1356,11 +1356,11 @@ static void receive_wanted_refs(struct packet_reader *reader, struct ref *refs)
> }
>
> if (!r)
> - die("unexpected wanted-ref: '%s'", reader->line);
> + die(_("unexpected wanted-ref: '%s'"), reader->line);
> }
>
> if (reader->status != PACKET_READ_DELIM)
> - die("error processing wanted refs: %d", reader->status);
> + die(_("error processing wanted refs: %d"), reader->status);
> }
>
> enum fetch_state {
> --
> 2.18.0.233.g985f88cf7e-goog
>
^ permalink raw reply
* [PATCH 3/9] fetch2/git: only use relevant checks for shallow tarball unpack
From: Urs Fässler @ 2018-07-23 15:42 UTC (permalink / raw)
To: bitbake-devel
In-Reply-To: <20180723154259.9076-1-urs.fassler@bbv.ch>
Some checks in need_update do not make sense in the unpack step. The
relevant checks for the unpack check are extracted into
__has_up_to_date_clonedir which is used in the unpack step.
Signed-off-by: Urs Fässler <urs.fassler@bbv.ch>
Signed-off-by: Pascal Bach <pascal.bach@siemens.com>
---
lib/bb/fetch2/git.py | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/lib/bb/fetch2/git.py b/lib/bb/fetch2/git.py
index 612aac43..3364bbf9 100644
--- a/lib/bb/fetch2/git.py
+++ b/lib/bb/fetch2/git.py
@@ -299,17 +299,22 @@ class Git(FetchMethod):
return ud.clonedir
def need_update(self, ud, d):
- if not os.path.exists(ud.clonedir):
+ if not self.__has_up_to_date_clonedir(ud, d):
return True
- for name in ud.names:
- if not self._contains_ref(ud, d, name, ud.clonedir):
- return True
if ud.shallow and ud.write_shallow_tarballs and not os.path.exists(ud.fullshallow):
return True
if ud.write_tarballs and not os.path.exists(ud.fullmirror):
return True
return False
+ def __has_up_to_date_clonedir(self, ud, d):
+ if not os.path.exists(ud.clonedir):
+ return False
+ for name in ud.names:
+ if not self._contains_ref(ud, d, name, ud.clonedir):
+ return False
+ return True
+
def try_premirror(self, ud, d):
# If we don't do this, updating an existing checkout with only premirrors
# is not possible
@@ -472,7 +477,7 @@ class Git(FetchMethod):
if os.path.exists(destdir):
bb.utils.prunedir(destdir)
- if ud.shallow and self.need_update(ud, d):
+ if ud.shallow and not self.__has_up_to_date_clonedir(ud, d):
bb.utils.mkdirhier(destdir)
runfetchcmd("tar -xzf %s" % ud.fullshallow, d, workdir=destdir)
else:
--
2.18.0
^ permalink raw reply related
* [U-Boot] [PULL] Please pull u-boot-imx
From: Tom Rini @ 2018-07-23 18:16 UTC (permalink / raw)
To: u-boot
In-Reply-To: <d239d69a-be35-0b08-1e87-3ad5e0607d14@denx.de>
On Mon, Jul 23, 2018 at 11:44:04AM +0200, Stefano Babic wrote:
> Hi Tom,
>
> please pull from u-boot-imx, thanks !
>
> The following changes since commit 474ecd2c84d97314b8145fbe3a57887f41b2edb3:
>
> env: Simplify Makefile using $(SPL_TPL_) (2018-07-21 12:24:31 -0400)
>
> are available in the git repository at:
>
> git://www.denx.de/git/u-boot-imx.git master
>
> for you to fetch changes up to f97f167107b33fc6596561dae1309571ade39055:
>
> configs: imx6q_logic: Cleanup ramdiskaddr and fdtaddr (2018-07-23
> 11:05:54 +0200)
>
Applied to u-boot/master, thanks!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180723/cc3a1b43/attachment.sig>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.