* [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer
From: Noralf Trønnes @ 2016-09-19 15:26 UTC (permalink / raw)
To: wsa, swarren, eric
Cc: linux-kernel, linux-i2c, linux-arm-kernel, linux-rpi-kernel,
Noralf Trønnes
In-Reply-To: <1474298777-5858-1-git-send-email-noralf@tronnes.org>
Some SMBus protocols use Repeated Start Condition to switch from write
mode to read mode. Devices like MMA8451 won't work without it.
When downstream implemented support for this in i2c-bcm2708, it broke
support for some devices, so a module parameter was added and combined
transfer was disabled by default.
See https://github.com/raspberrypi/linux/issues/599
It doesn't seem to have been any investigation into what the problem
really was. Later there was added a timeout on the polling loop.
One of the devices mentioned to partially stop working was DS1307.
I have run thousands of transfers to a DS1307 (rtc), MMA8451 (accel)
and AT24C32 (eeprom) in parallel without problems.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/i2c/busses/i2c-bcm2835.c | 107 +++++++++++++++++++++++++++++++++++----
1 file changed, 98 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index f283b71..b3ce565 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -154,8 +154,39 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
return IRQ_NONE;
}
+/*
+ * Repeated Start Condition (Sr)
+ * The BCM2835 ARM Peripherals datasheet mentions a way to trigger a Sr when it
+ * talks about reading from a slave with 10 bit address. This is achieved by
+ * issuing a write (without enabling interrupts), poll the I2CS.TA flag and
+ * wait for it to be set, and then issue a read.
+ * https://github.com/raspberrypi/linux/issues/254 shows how the firmware does
+ * it and states that it's a workaround for a problem in the state machine.
+ * This is the comment in the firmware code:
+ *
+ * The I2C peripheral samples the values for rw_bit and xfer_count in the
+ * IDLE state if start is set.
+ *
+ * We want to generate a ReSTART not a STOP at the end of the TX phase. In
+ * order to do that we must ensure the state machine goes
+ * RACK1 -> RACK2 -> SRSTRT1 (not RACK1 -> RACK2 -> SSTOP1).
+ *
+ * So, in the RACK2 state when (TX) xfer_count==0 we must therefore have
+ * already set, ready to be sampled:
+ * READ; rw_bit <= I2CC bit 0 - must be "read"
+ * ST; start <= I2CC bit 7 - must be "Go" in order to not issue STOP
+ * DLEN; xfer_count <= I2CDLEN - must be equal to our read amount
+ *
+ * The plan to do this is:
+ * 1. Start the sub-address write, but don't let it finish (keep
+ * xfer_count > 0)
+ * 2. Populate READ, DLEN and ST in preparation for ReSTART read sequence
+ * 3. Let TX finish (write the rest of the data)
+ * 4. Read back data as it arrives
+ */
+
static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
- struct i2c_msg *msg)
+ struct i2c_msg *msg, struct i2c_msg *msg2)
{
u32 c;
unsigned long time_left;
@@ -167,21 +198,70 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, BCM2835_I2C_C_CLEAR);
- if (msg->flags & I2C_M_RD) {
- c = BCM2835_I2C_C_READ | BCM2835_I2C_C_INTR;
- } else {
- c = BCM2835_I2C_C_INTT;
+ if (!(msg->flags & I2C_M_RD))
bcm2835_fill_txfifo(i2c_dev);
- }
- c |= BCM2835_I2C_C_ST | BCM2835_I2C_C_INTD | BCM2835_I2C_C_I2CEN;
bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_A, msg->addr);
bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DLEN, msg->len);
- bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, c);
+
+ if (!msg2) {
+ if (msg->flags & I2C_M_RD)
+ c = BCM2835_I2C_C_READ | BCM2835_I2C_C_INTR;
+ else
+ c = BCM2835_I2C_C_INTT;
+
+ c |= BCM2835_I2C_C_ST | BCM2835_I2C_C_INTD |
+ BCM2835_I2C_C_I2CEN;
+ bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, c);
+ } else {
+ unsigned long flags;
+ u32 stat, err = 0;
+
+ local_irq_save(flags);
+
+ /* Start write message */
+ c = BCM2835_I2C_C_ST | BCM2835_I2C_C_I2CEN;
+ bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, c);
+
+ /* Wait for the transfer to become active */
+ for (time_left = 100; time_left > 0; time_left--) {
+ stat = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S);
+
+ err = stat & (BCM2835_I2C_S_CLKT | BCM2835_I2C_S_ERR);
+ if (err)
+ break;
+
+ if (stat & BCM2835_I2C_S_TA)
+ break;
+ }
+
+ if (err || !time_left) {
+ i2c_dev->msg_err = err;
+ local_irq_restore(flags);
+ goto error;
+ }
+
+ /* Start read message */
+ i2c_dev->curr_msg = msg2;
+ i2c_dev->msg_buf = msg2->buf;
+ i2c_dev->msg_buf_remaining = msg2->len;
+ bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DLEN, msg2->len);
+
+ c = BCM2835_I2C_C_READ | BCM2835_I2C_C_INTR |
+ BCM2835_I2C_C_INTD | BCM2835_I2C_C_ST |
+ BCM2835_I2C_C_I2CEN;
+
+ bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, c);
+
+ local_irq_restore(flags);
+ }
time_left = wait_for_completion_timeout(&i2c_dev->completion,
BCM2835_I2C_TIMEOUT);
+error:
bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, BCM2835_I2C_C_CLEAR);
+ bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_S, BCM2835_I2C_S_CLKT |
+ BCM2835_I2C_S_ERR | BCM2835_I2C_S_DONE);
if (!time_left) {
dev_err(i2c_dev->dev, "i2c transfer timed out\n");
return -ETIMEDOUT;
@@ -209,8 +289,17 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
int i;
int ret = 0;
+ /* Combined write-read to the same address (smbus) */
+ if (num == 2 && (msgs[0].addr == msgs[1].addr) &&
+ !(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
+ (msgs[0].len <= 16)) {
+ ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[0], &msgs[1]);
+
+ return ret ? ret : 2;
+ }
+
for (i = 0; i < num; i++) {
- ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i]);
+ ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i], NULL);
if (ret)
break;
}
--
2.8.2
^ permalink raw reply related
* [PATCH 1/3] i2c: bcm2835: Fix hang for writing messages larger than 16 bytes
From: Noralf Trønnes @ 2016-09-19 15:26 UTC (permalink / raw)
To: wsa, swarren, eric
Cc: linux-kernel, linux-i2c, linux-arm-kernel, linux-rpi-kernel,
Noralf Trønnes
Writing messages larger than the FIFO size results in a hang, rendering
the machine unusable. This is because the RXD status flag is set on the
first interrupt which results in bcm2835_drain_rxfifo() stealing bytes
from the buffer. The controller continues to trigger interrupts waiting
for the missing bytes, but bcm2835_fill_txfifo() has none to give.
In this situation wait_for_completion_timeout() apparently is unable to
stop the madness.
The BCM2835 ARM Peripherals datasheet has this to say about the flags:
TXD: is set when the FIFO has space for at least one byte of data.
RXD: is set when the FIFO contains at least one byte of data.
TXW: is set during a write transfer and the FIFO is less than full.
RXR: is set during a read transfer and the FIFO is or more full.
Implementing the logic from the downstream i2c-bcm2708 driver solved
the hang problem.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/i2c/busses/i2c-bcm2835.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index d4f3239..f283b71 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -64,6 +64,7 @@ struct bcm2835_i2c_dev {
int irq;
struct i2c_adapter adapter;
struct completion completion;
+ struct i2c_msg *curr_msg;
u32 msg_err;
u8 *msg_buf;
size_t msg_buf_remaining;
@@ -126,14 +127,13 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
return IRQ_HANDLED;
}
- if (val & BCM2835_I2C_S_RXD) {
- bcm2835_drain_rxfifo(i2c_dev);
- if (!(val & BCM2835_I2C_S_DONE))
- return IRQ_HANDLED;
- }
-
if (val & BCM2835_I2C_S_DONE) {
- if (i2c_dev->msg_buf_remaining)
+ if (i2c_dev->curr_msg->flags & I2C_M_RD) {
+ bcm2835_drain_rxfifo(i2c_dev);
+ val = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S);
+ }
+
+ if ((val & BCM2835_I2C_S_RXD) || i2c_dev->msg_buf_remaining)
i2c_dev->msg_err = BCM2835_I2C_S_LEN;
else
i2c_dev->msg_err = 0;
@@ -141,11 +141,16 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
return IRQ_HANDLED;
}
- if (val & BCM2835_I2C_S_TXD) {
+ if (val & BCM2835_I2C_S_TXW) {
bcm2835_fill_txfifo(i2c_dev);
return IRQ_HANDLED;
}
+ if (val & BCM2835_I2C_S_RXR) {
+ bcm2835_drain_rxfifo(i2c_dev);
+ return IRQ_HANDLED;
+ }
+
return IRQ_NONE;
}
@@ -155,6 +160,7 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
u32 c;
unsigned long time_left;
+ i2c_dev->curr_msg = msg;
i2c_dev->msg_buf = msg->buf;
i2c_dev->msg_buf_remaining = msg->len;
reinit_completion(&i2c_dev->completion);
--
2.8.2
^ permalink raw reply related
* Re: [REGRESSION? v4.8] i2c-core: acpi_i2c_get_info() touches non-existent devices
From: Nicolai Stange @ 2016-09-19 13:58 UTC (permalink / raw)
To: Mika Westerberg
Cc: Nicolai Stange, Wolfram Sang, Octavian Purdila, Rafael J. Wysocki,
linux-i2c, linux-kernel
In-Reply-To: <20160919130324.GI1811@lahna.fi.intel.com>
Mika Westerberg <mika.westerberg@linux.intel.com> writes:
> On Mon, Sep 19, 2016 at 11:48:07AM +0300, Mika Westerberg wrote:
>> On Mon, Sep 19, 2016 at 12:30:53AM +0200, Nicolai Stange wrote:
>> > I'm encountering the following:
>> >
>> > [ 10.409490] ERROR: Unable to locate IOAPIC for GSI 37
>> >
>> > Note that the system works fine, so it's a "cosmetic" regression, I think.
>> >
>> >
>> > I added a dump_stack() right below the printk() in question and it reads
>> > as
>> >
>> > [ 10.410290] CPU: 6 PID: 710 Comm: systemd-udevd Not tainted 4.7.0-rc4+ #348
>> > [ 10.410962] Hardware name: Dell Inc. Latitude E6540/0725FP, BIOS A10 06/26/2014
>> > [ 10.411772] 0000000000000286 00000000b9050627 ffff8800c2e5f590 ffffffffa54161e7
>> > [ 10.412569] 0000000000000025 0000000000000001 ffff8800c2e5f5a0 ffffffffa50465df
>> > [ 10.413292] ffff8800c2e5f5d0 ffffffffa5046ffd 0000000000000000 0000000000000025
>> > [ 10.414016] Call Trace:
>> > [ 10.414713] [<ffffffffa54161e7>] dump_stack+0x68/0xa1
>> > [ 10.415406] [<ffffffffa50465df>] mp_find_ioapic+0x4f/0x60
>> > [ 10.416131] [<ffffffffa5046ffd>] mp_map_gsi_to_irq+0x1d/0xc0
>> > [ 10.416806] [<ffffffffa503dbbb>] acpi_register_gsi_ioapic+0x7b/0x170
>> > [ 10.417494] [<ffffffffa503da6f>] acpi_register_gsi+0xf/0x20
>> > [ 10.418217] [<ffffffffa54a14d5>] acpi_dev_get_irqresource.part.3+0xd7/0x11d
>> > [ 10.418871] [<ffffffffa54a139a>] ? acpi_dev_resource_address_space+0x31/0x67
>> > [ 10.419655] [<ffffffffa54a168d>] acpi_dev_resource_interrupt+0x9b/0xab
>> > [ 10.420408] [<ffffffffa54a1848>] acpi_dev_process_resource+0xbc/0xf7
>> > [ 10.421070] [<ffffffffa54a178c>] ? acpi_dev_resource_memory+0x7c/0x7c
>> > [ 10.421732] [<ffffffffa54c3ba2>] acpi_walk_resource_buffer+0x4d/0x85
>> > [ 10.422399] [<ffffffffa54a178c>] ? acpi_dev_resource_memory+0x7c/0x7c
>> > [ 10.423158] [<ffffffffa54c3e89>] acpi_walk_resources+0x83/0xb6
>> > [ 10.423831] [<ffffffffa54a15b1>] acpi_dev_get_resources+0x96/0xd7
>> > [ 10.424505] [<ffffffffa563f7c4>] acpi_i2c_get_info+0xe4/0x1a0
>> > [ 10.425181] [<ffffffffa5642c06>] acpi_i2c_add_device+0x56/0xa0
>> > [ 10.425856] [<ffffffffa54bf2ff>] acpi_ns_walk_namespace+0xe8/0x19d
>> > [ 10.426564] [<ffffffffa5642bb0>] ? acpi_i2c_register_device+0x70/0x70
>> > [ 10.427418] [<ffffffffa5642bb0>] ? acpi_i2c_register_device+0x70/0x70
>> > [ 10.428179] [<ffffffffa54bf83d>] acpi_walk_namespace+0xa0/0xd5
>> > [ 10.428858] [<ffffffffa56437a9>] i2c_register_adapter+0x369/0x500
>> > [ 10.429499] [<ffffffffa564399c>] i2c_add_adapter+0x5c/0x70
>> > [ 10.430125] [<ffffffffc07df7dd>] i801_probe+0x2bd/0x6a0 [i2c_i801]
>> > I bisected this to commit 525e6fabeae2 ("i2c / ACPI: add support for
>> > ACPI reconfigure notifications").
>> >
>> > The reason for the above message seems to be that acpi_i2c_get_info()
>> > configures the IRQs for any ACPI devices that have got some
>> > I2cSerialBus() resource, regardless of the actual adapter those are
>> > attached to. This behaviour is different from before that commit.
>> >
>> > My ACPI DSDT has got a PCI I2C adapter that isn't physically present, it
>> > seems. No clue why.
>> >
>> > That non-existent PCI I2C adapter is in turn I2cSerialBus()-referenced
>> > by some ACPI device that has got exactly this interrupt 37 assigned.
>> >
>> > So it looks like an attempt is made to configure this non-existent,
>> > ACPI-listed I2C slave's IRQs when an actually existing I2C adapter (i801
>> > SMBus) gets probed.
> Can you try if the following patch cures the problem?
Unfortunately not. That patch installs the check after the
acpi_i2c_get_info() invocation which is part of the backtrace above.
I moved your check into i2c_get_info(), right in front of the IRQ
handling and this works.
So,
Tested-by: Nicolai Stange <nicstange@gmail.com>
for this:
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 74e5aea..3f2b3cf 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -141,6 +141,7 @@ static int acpi_i2c_get_info(struct acpi_device *adev,
struct list_head resource_list;
struct resource_entry *entry;
struct acpi_i2c_lookup lookup;
+ struct acpi_device *adapter_adev;
int ret;
if (acpi_bus_get_status(adev) || !adev->status.present ||
@@ -163,6 +164,12 @@ static int acpi_i2c_get_info(struct acpi_device *adev,
if (ret < 0 || !info->addr)
return -EINVAL;
+ /* The adapter must be present */
+ if (acpi_bus_get_device(lookup.adapter_handle, &adapter_adev))
+ return -EINVAL;
+ if (acpi_bus_get_status(adapter_adev) || !adapter_adev->status.present)
+ return -EINVAL;;
+
*adapter_handle = lookup.adapter_handle;
/* Then fill IRQ number if any */
But it is still true that acpi_i2c_register_devices() configures the
interrupts for all ACPI I2C slaves attached to an available adapter,
independent of whether their adapter is the one given as an argument or
not. I can't tell whether this is desired, just a note...
^ permalink raw reply related
* Re: [REGRESSION? v4.8] i2c-core: acpi_i2c_get_info() touches non-existent devices
From: Mika Westerberg @ 2016-09-19 13:03 UTC (permalink / raw)
To: Nicolai Stange
Cc: Wolfram Sang, Octavian Purdila, Rafael J. Wysocki, linux-i2c,
linux-kernel
In-Reply-To: <20160919084807.GC1811@lahna.fi.intel.com>
On Mon, Sep 19, 2016 at 11:48:07AM +0300, Mika Westerberg wrote:
> On Mon, Sep 19, 2016 at 12:30:53AM +0200, Nicolai Stange wrote:
> > Hi,
> >
> > I'm encountering the following:
> >
> > [ 10.409490] ERROR: Unable to locate IOAPIC for GSI 37
> >
> > Note that the system works fine, so it's a "cosmetic" regression, I think.
> >
> >
> > I added a dump_stack() right below the printk() in question and it reads
> > as
> >
> > [ 10.410290] CPU: 6 PID: 710 Comm: systemd-udevd Not tainted 4.7.0-rc4+ #348
> > [ 10.410962] Hardware name: Dell Inc. Latitude E6540/0725FP, BIOS A10 06/26/2014
> > [ 10.411772] 0000000000000286 00000000b9050627 ffff8800c2e5f590 ffffffffa54161e7
> > [ 10.412569] 0000000000000025 0000000000000001 ffff8800c2e5f5a0 ffffffffa50465df
> > [ 10.413292] ffff8800c2e5f5d0 ffffffffa5046ffd 0000000000000000 0000000000000025
> > [ 10.414016] Call Trace:
> > [ 10.414713] [<ffffffffa54161e7>] dump_stack+0x68/0xa1
> > [ 10.415406] [<ffffffffa50465df>] mp_find_ioapic+0x4f/0x60
> > [ 10.416131] [<ffffffffa5046ffd>] mp_map_gsi_to_irq+0x1d/0xc0
> > [ 10.416806] [<ffffffffa503dbbb>] acpi_register_gsi_ioapic+0x7b/0x170
> > [ 10.417494] [<ffffffffa503da6f>] acpi_register_gsi+0xf/0x20
> > [ 10.418217] [<ffffffffa54a14d5>] acpi_dev_get_irqresource.part.3+0xd7/0x11d
> > [ 10.418871] [<ffffffffa54a139a>] ? acpi_dev_resource_address_space+0x31/0x67
> > [ 10.419655] [<ffffffffa54a168d>] acpi_dev_resource_interrupt+0x9b/0xab
> > [ 10.420408] [<ffffffffa54a1848>] acpi_dev_process_resource+0xbc/0xf7
> > [ 10.421070] [<ffffffffa54a178c>] ? acpi_dev_resource_memory+0x7c/0x7c
> > [ 10.421732] [<ffffffffa54c3ba2>] acpi_walk_resource_buffer+0x4d/0x85
> > [ 10.422399] [<ffffffffa54a178c>] ? acpi_dev_resource_memory+0x7c/0x7c
> > [ 10.423158] [<ffffffffa54c3e89>] acpi_walk_resources+0x83/0xb6
> > [ 10.423831] [<ffffffffa54a15b1>] acpi_dev_get_resources+0x96/0xd7
> > [ 10.424505] [<ffffffffa563f7c4>] acpi_i2c_get_info+0xe4/0x1a0
> > [ 10.425181] [<ffffffffa5642c06>] acpi_i2c_add_device+0x56/0xa0
> > [ 10.425856] [<ffffffffa54bf2ff>] acpi_ns_walk_namespace+0xe8/0x19d
> > [ 10.426564] [<ffffffffa5642bb0>] ? acpi_i2c_register_device+0x70/0x70
> > [ 10.427418] [<ffffffffa5642bb0>] ? acpi_i2c_register_device+0x70/0x70
> > [ 10.428179] [<ffffffffa54bf83d>] acpi_walk_namespace+0xa0/0xd5
> > [ 10.428858] [<ffffffffa56437a9>] i2c_register_adapter+0x369/0x500
> > [ 10.429499] [<ffffffffa564399c>] i2c_add_adapter+0x5c/0x70
> > [ 10.430125] [<ffffffffc07df7dd>] i801_probe+0x2bd/0x6a0 [i2c_i801]
> > [ 10.431159] [<ffffffffa50eb8dd>] ? trace_hardirqs_on+0xd/0x10
> > [ 10.432196] [<ffffffffa54682f2>] local_pci_probe+0x42/0xa0
> > [ 10.432826] [<ffffffffa5468e2a>] ? pci_match_device+0xca/0x110
> > [ 10.433460] [<ffffffffa5469243>] pci_device_probe+0x103/0x150
> > [ 10.434083] [<ffffffffa5545acc>] driver_probe_device+0x22c/0x440
> > [ 10.434712] [<ffffffffa5545db5>] __driver_attach+0xd5/0x100
> > [ 10.435341] [<ffffffffa5545ce0>] ? driver_probe_device+0x440/0x440
> > [ 10.435963] [<ffffffffa5543313>] bus_for_each_dev+0x73/0xc0
> > [ 10.436676] [<ffffffffa554519e>] driver_attach+0x1e/0x20
> > [ 10.437356] [<ffffffffa5544bc6>] bus_add_driver+0x1c6/0x290
> > [ 10.437978] [<ffffffffc07e5000>] ? 0xffffffffc07e5000
> > [ 10.438699] [<ffffffffa5546a50>] driver_register+0x60/0xe0
> > [ 10.439502] [<ffffffffc07e5000>] ? 0xffffffffc07e5000
> > [ 10.440310] [<ffffffffa5467e4d>] __pci_register_driver+0x5d/0x60
> > [ 10.440313] [<ffffffffc07e50af>] i2c_i801_init+0xaf/0x1000 [i2c_i801]
> > [ 10.440314] [<ffffffffc07e5000>] ? 0xffffffffc07e5000
> > [ 10.440316] [<ffffffffa5000450>] do_one_initcall+0x50/0x180
> > [ 10.440319] [<ffffffffa5101cc5>] ? rcu_read_lock_sched_held+0x45/0x80
> > [ 10.440322] [<ffffffffa5234e55>] ? kmem_cache_alloc_trace+0x2d5/0x340
> > [ 10.440325] [<ffffffffa51c1252>] do_init_module+0x5f/0x1da
> > [ 10.440329] [<ffffffffa512d615>] load_module+0x2195/0x2950
> > [ 10.440331] [<ffffffffa512a0e0>] ? __symbol_put+0x70/0x70
> > [ 10.440334] [<ffffffffa525dd3b>] ? vfs_read+0x11b/0x130
> > [ 10.440337] [<ffffffffa512e066>] SYSC_finit_module+0xe6/0x120
> > [ 10.440339] [<ffffffffa512e0be>] SyS_finit_module+0xe/0x10
> > [ 10.440340] [<ffffffffa5002fb1>] do_syscall_64+0x61/0x170
> > [ 10.440343] [<ffffffffa581e1da>] entry_SYSCALL64_slow_path+0x25/0x25
> >
> >
> > I bisected this to commit 525e6fabeae2 ("i2c / ACPI: add support for
> > ACPI reconfigure notifications").
> >
> > The reason for the above message seems to be that acpi_i2c_get_info()
> > configures the IRQs for any ACPI devices that have got some
> > I2cSerialBus() resource, regardless of the actual adapter those are
> > attached to. This behaviour is different from before that commit.
> >
> > My ACPI DSDT has got a PCI I2C adapter that isn't physically present, it
> > seems. No clue why.
> >
> > That non-existent PCI I2C adapter is in turn I2cSerialBus()-referenced
> > by some ACPI device that has got exactly this interrupt 37 assigned.
> >
> > So it looks like an attempt is made to configure this non-existent,
> > ACPI-listed I2C slave's IRQs when an actually existing I2C adapter (i801
> > SMBus) gets probed.
> >
> >
> > Let me know if I can provide you with any additional information,
>
> Can you send me acpidump from that that machine?
Can you try if the following patch cures the problem?
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index da3a02ef4a31..4e6c6fde9bdb 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -205,7 +205,7 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
void *data, void **return_value)
{
struct i2c_adapter *adapter = data;
- struct acpi_device *adev;
+ struct acpi_device *adev, *adapter_adev;
acpi_handle adapter_handle;
struct i2c_board_info info;
@@ -218,6 +218,12 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
if (adapter_handle != ACPI_HANDLE(&adapter->dev))
return AE_OK;
+ /* The adapter must be present */
+ if (acpi_bus_get_device(adapter_handle, &adapter_adev))
+ return AE_OK;
+ if (acpi_bus_get_status(adapter_adev) || !adapter_adev->status.present)
+ return AE_OK;
+
acpi_i2c_register_device(adapter, adev, &info);
return AE_OK;
^ permalink raw reply related
* [patch v2] i2c: add master driver for mellanox systems
From: vadimp @ 2016-09-19 15:00 UTC (permalink / raw)
To: wsa; +Cc: linux-i2c, linux-kernel, jiri, Vadim Pasternak, Michael Shych
From: Vadim Pasternak <vadimp@mellanox.com>
Device driver for Mellanox I2C controller logic, implemented in Lattice
CPLD device.
Device supports:
- Master mode
- One physical bus
- Polling mode
The Kconfig currently controlling compilation of this code is:
drivers/i2c/busses/Kconfig:config I2C_MLXCPLD
Signed-off-by: Michael Shych <michaelsh@mellanox.com>
Signed-off-by: Vadim Pasternak <vadimp@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
v1->v2
Fixes added by Vadim:
- Put new record in Makefile in alphabetic order;
- Remove http://www.mellanox.com from MAINTAINERS record;
---
Documentation/i2c/busses/i2c-mlxcpld | 47 +++
MAINTAINERS | 8 +
drivers/i2c/busses/Kconfig | 12 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-mlxcpld.c | 597 +++++++++++++++++++++++++++++++++++
5 files changed, 665 insertions(+)
create mode 100644 Documentation/i2c/busses/i2c-mlxcpld
create mode 100644 drivers/i2c/busses/i2c-mlxcpld.c
diff --git a/Documentation/i2c/busses/i2c-mlxcpld b/Documentation/i2c/busses/i2c-mlxcpld
new file mode 100644
index 0000000..0f8678a
--- /dev/null
+++ b/Documentation/i2c/busses/i2c-mlxcpld
@@ -0,0 +1,47 @@
+Driver i2c-mlxcpld
+
+Author: Michael Shych <michaelsh@mellanox.com>
+
+This is a for Mellanox I2C controller logic, implemented in Lattice CPLD
+device.
+Device supports:
+ - Master mode.
+ - One physical bus.
+ - Polling mode.
+
+This controller is equipped within the next Mellanox systems:
+"msx6710", "msx6720", "msb7700", "msn2700", "msx1410", "msn2410", "msb7800",
+"msn2740", "msn2100".
+
+The next transaction types are supported:
+ - Receive Byte/Block.
+ - Send Byte/Block.
+ - Read Byte/Block.
+ - Write Byte/Block.
+
+Registers:
+CTRL 0x1 - control reg.
+ Resets all the registers.
+HALF_CYC 0x4 - cycle reg.
+ Configure the width of I2C SCL half clock cycle (in 4 LPC_CLK
+ units).
+I2C_HOLD 0x5 - hold reg.
+ OE (output enable) is delayed by value set to this register
+ (in LPC_CLK units)
+CMD 0x6 - command reg.
+ Bit 7(lsb), 0 = write, 1 = read.
+ Bits [6:0] - the 7bit Address of the I2C device.
+ It should be written last as it triggers an I2C transaction.
+NUM_DATA 0x7 - data size reg.
+ Number of address bytes to write in read transaction
+NUM_ADDR 0x8 - address reg.
+ Number of address bytes to write in read transaction.
+STATUS 0x9 - status reg.
+ Bit 0 - transaction is completed.
+ Bit 4 - ACK/NACK.
+DATAx 0xa - 0x54 - 68 bytes data buffer regs.
+ For write transaction address is specified in four first bytes
+ (DATA1 - DATA4), data starting from DATA4.
+ For read transactions address is send in separate transaction and
+ specified in four first bytes (DATA0 - DATA3). Data is reading
+ starting from DATA0.
diff --git a/MAINTAINERS b/MAINTAINERS
index 6781a3f..dc31231 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7667,6 +7667,14 @@ W: http://www.mellanox.com
Q: http://patchwork.ozlabs.org/project/netdev/list/
F: drivers/net/ethernet/mellanox/mlxsw/
+MELLANOX MLXCPLD I2C DRIVER
+M: Vadim Pasternak <vadimp@mellanox.com>
+M: Michael Shych <michaelsh@mellanox.com>
+L: linux-i2c@vger.kernel.org
+S: Supported
+F: drivers/i2c/busses/i2c-mlxcpld.c
+F: Documentation/i2c/busses/i2c-mlxcpld
+
SOFT-ROCE DRIVER (rxe)
M: Moni Shoua <monis@mellanox.com>
L: linux-rdma@vger.kernel.org
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 5c3993b..1126142a 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1203,4 +1203,16 @@ config I2C_OPAL
This driver can also be built as a module. If so, the module will be
called as i2c-opal.
+config I2C_MLXCPLD
+ tristate "Mellanox I2C driver"
+ depends on X86_64
+ default y
+ help
+ This exposes the Mellanox platform I2C busses to the linux I2C layer
+ for X86 based systems.
+ Controller is implemented as CPLD logic.
+
+ This driver can also be built as a module. If so, the module will be
+ called as i2c-mlxcpld.
+
endmenu
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 37f2819..4df3578 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -118,5 +118,6 @@ obj-$(CONFIG_I2C_PCA_ISA) += i2c-pca-isa.o
obj-$(CONFIG_I2C_SIBYTE) += i2c-sibyte.o
obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o
obj-$(CONFIG_SCx200_ACB) += scx200_acb.o
+obj-$(CONFIG_I2C_MLXCPLD) += i2c-mlxcpld.o
ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
diff --git a/drivers/i2c/busses/i2c-mlxcpld.c b/drivers/i2c/busses/i2c-mlxcpld.c
new file mode 100644
index 0000000..dd62190
--- /dev/null
+++ b/drivers/i2c/busses/i2c-mlxcpld.c
@@ -0,0 +1,597 @@
+/*
+ * drivers/i2c/busses/i2c-mlxcpld.c
+ * Copyright (c) 2016 Mellanox Technologies. All rights reserved.
+ * Copyright (c) 2016 Michael Shych <michaels@mellanox.com>
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ * 3. Neither the names of the copyright holders nor the names of its
+ * contributors may be used to endorse or promote products derived from
+ * this software without specific prior written permission.
+ *
+ * Alternatively, this software may be distributed under the terms of the
+ * GNU General Public License ("GPL") version 2 as published by the Free
+ * Software Foundation.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
+ * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+/* General defines */
+#define MLXPLAT_CPLD_LPC_I2C_BASE_ADRR 0x2000
+#define MLXCPLD_I2C_DEVICE_NAME "i2c_mlxcpld"
+#define MLXCPLD_I2C_VALID_FLAG (I2C_M_RECV_LEN | I2C_M_RD)
+#define MLXCPLD_I2C_BUS_NUM 1
+#define MLXCPLD_I2C_DATA_REG_SZ 36
+#define MLXCPLD_I2C_MAX_ADDR_LEN 4
+#define MLXCPLD_I2C_RETR_NUM 2
+#define MLXCPLD_I2C_XFER_TO 500000 /* msec */
+#define MLXCPLD_I2C_POLL_TIME 2000 /* msec */
+
+/* LPC I2C registers */
+#define MLXCPLD_LPCI2C_LPF_REG 0x0
+#define MLXCPLD_LPCI2C_CTRL_REG 0x1
+#define MLXCPLD_LPCI2C_HALF_CYC_REG 0x4
+#define MLXCPLD_LPCI2C_I2C_HOLD_REG 0x5
+#define MLXCPLD_LPCI2C_CMD_REG 0x6
+#define MLXCPLD_LPCI2C_NUM_DAT_REG 0x7
+#define MLXCPLD_LPCI2C_NUM_ADDR_REG 0x8
+#define MLXCPLD_LPCI2C_STATUS_REG 0x9
+#define MLXCPLD_LPCI2C_DATA_REG 0xa
+
+/* LPC I2C masks and parametres */
+#define MLXCPLD_LPCI2C_RST_SEL_MASK 0x1
+#define MLXCPLD_LPCI2C_LPF_DFLT 0x2
+#define MLXCPLD_LPCI2C_HALF_CYC_100 0x1f
+#define MLXCPLD_LPCI2C_I2C_HOLD_100 0x3c
+#define MLXCPLD_LPCI2C_TRANS_END 0x1
+#define MLXCPLD_LPCI2C_STATUS_NACK 0x10
+#define MLXCPLD_LPCI2C_ERR_IND -1
+#define MLXCPLD_LPCI2C_NO_IND 0
+#define MLXCPLD_LPCI2C_ACK_IND 1
+#define MLXCPLD_LPCI2C_NACK_IND 2
+
+/**
+ * mlxcpld_i2c_regs - controller registers:
+ * @half_cyc - half cycle register
+ * @i2c_hold - hold register
+ * @config - config register
+ * @cmd - command register
+ * @cmd - status register
+ * @data - register data
+**/
+struct mlxcpld_i2c_regs {
+ u8 half_cyc;
+ u8 i2c_hold;
+ u8 config;
+ u8 cmd;
+ u8 status;
+ u8 data[MLXCPLD_I2C_DATA_REG_SZ];
+};
+
+/**
+ * mlxcpld_i2c_curr_transf - current transaction parameters:
+ * @cmd - command
+ * @addr_width - address width
+ * @data_len - data length
+ * @cmd - command register
+ * @msg_num - message number
+ * @msg - pointer to message buffer
+**/
+struct mlxcpld_i2c_curr_transf {
+ u8 cmd;
+ u8 addr_width;
+ u8 data_len;
+ u8 msg_num;
+ struct i2c_msg *msg;
+};
+
+/**
+ * mlxcpld_i2c_priv - private controller data:
+ * @lpc_gen_dec_reg - register space
+ * @adap - i2c adapter
+ * @dev_id - device id
+ * @base_addr - base IO address
+ * @poll_time - polling time
+ * @xfer_to - transfer timeout in microsec (500000)
+ * @retr_num - access retries number (2)
+ * @block_sz - maximum data block size (36),
+ * @lock - bus access lock
+ * @lpc_i2c_res - lpc i2c resourse
+ * @lpc_cpld_res - lpc cpld resource
+ * @xfer - current i2c transfer block
+ * @pdev - platform device
+**/
+struct mlxcpld_i2c_priv {
+ struct i2c_adapter adap;
+ u16 dev_id;
+ u16 base_addr;
+ u16 poll_time;
+ int xfer_to;
+ int retr_num;
+ int block_sz;
+ struct mutex lock;
+ struct mlxcpld_i2c_curr_transf xfer;
+ struct platform_device *pdev;
+};
+struct platform_device *mlxcpld_i2c_plat_dev;
+
+static void mlxcpld_i2c_lpc_write_buf(u8 *data, u8 len, u32 addr)
+{
+ int i, nbyte, ndword;
+
+ nbyte = len % 4;
+ ndword = len / 4;
+ for (i = 0; i < ndword; i++)
+ outl(*((u32 *)data + i), addr + i * 4);
+ ndword *= 4;
+ addr += ndword;
+ data += ndword;
+ for (i = 0; i < nbyte; i++)
+ outb(*(data + i), addr + i);
+}
+
+static void mlxcpld_i2c_lpc_read_buf(u8 *data, u8 len, u32 addr)
+{
+ int i, nbyte, ndword;
+
+ nbyte = len % 4;
+ ndword = len / 4;
+ for (i = 0; i < ndword; i++)
+ *((u32 *)data + i) = inl(addr + i * 4);
+ ndword *= 4;
+ addr += ndword;
+ data += ndword;
+ for (i = 0; i < nbyte; i++)
+ *(data + i) = inb(addr + i);
+}
+
+static void mlxcpld_i2c_read_comm(struct mlxcpld_i2c_priv *priv, u8 offs,
+ u8 *data, u8 datalen)
+{
+ u32 addr = priv->base_addr + offs;
+
+ switch (datalen) {
+ case 1:
+ *(data) = inb(addr);
+ break;
+ case 2:
+ *((u16 *)data) = inw(addr);
+ break;
+ case 3:
+ *((u16 *)data) = inw(addr);
+ *(data + 2) = inb(addr + 2);
+ break;
+ case 4:
+ *((u32 *)data) = inl(addr);
+ break;
+ default:
+ mlxcpld_i2c_lpc_read_buf(data, datalen, addr);
+ break;
+ }
+}
+
+static void mlxcpld_i2c_write_comm(struct mlxcpld_i2c_priv *priv, u8 offs,
+ u8 *data, u8 datalen)
+{
+ u32 addr = priv->base_addr + offs;
+
+ switch (datalen) {
+ case 1:
+ outb(*(data), addr);
+ break;
+ case 2:
+ outw(*((u16 *)data), addr);
+ break;
+ case 3:
+ outw(*((u16 *)data), addr);
+ outb(*(data + 2), addr + 2);
+ break;
+ case 4:
+ outl(*((u32 *)data), addr);
+ break;
+ default:
+ mlxcpld_i2c_lpc_write_buf(data, datalen, addr);
+ break;
+ }
+}
+
+/* Check validity of current i2c message and all transfer.
+ * Calculate also coomon length of all i2c messages in transfer.
+ */
+static int mlxcpld_i2c_invalid_len(struct mlxcpld_i2c_priv *priv,
+ const struct i2c_msg *msg, u8 *comm_len)
+{
+ u8 max_len = msg->flags == I2C_M_RD ? priv->block_sz -
+ MLXCPLD_I2C_MAX_ADDR_LEN : priv->block_sz;
+
+ if (msg->len < 0 || msg->len > max_len)
+ return -EINVAL;
+
+ *comm_len += msg->len;
+ if (*comm_len > priv->block_sz)
+ return -EINVAL;
+ else
+ return 0;
+}
+
+/* Check validity of received i2c messages parameters.
+ * Returns 0 if OK, other - in case of invalid parameters
+ * or common length of data that should be passed to CPLD
+ */
+static int mlxcpld_i2c_check_msg_params(struct mlxcpld_i2c_priv *priv,
+ struct i2c_msg *msgs, int num,
+ u8 *comm_len)
+{
+ int i;
+
+ if (!num) {
+ dev_err(&priv->pdev->dev, "Incorrect 0 num of messages\n");
+ return -EINVAL;
+ }
+
+ if (unlikely(msgs[0].addr > 0x7f)) {
+ dev_err(&priv->pdev->dev, "Invalid address 0x%03x\n",
+ msgs[0].addr);
+ return -EINVAL;
+ }
+
+ for (i = 0; i < num; ++i) {
+ if (unlikely(!msgs[i].buf)) {
+ dev_err(&priv->pdev->dev, "Invalid buf in msg[%d]\n",
+ i);
+ return -EINVAL;
+ }
+ if (unlikely(msgs[0].addr != msgs[i].addr)) {
+ dev_err(&priv->pdev->dev, "Invalid addr in msg[%d]\n",
+ i);
+ return -EINVAL;
+ }
+ if (unlikely(mlxcpld_i2c_invalid_len(priv, &msgs[i],
+ comm_len))) {
+ dev_err(&priv->pdev->dev, "Invalid len %d msg[%d], addr 0x%x, lag %u\n",
+ msgs[i].len, i, msgs[i].addr, msgs[i].flags);
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+
+/* Check if transfer is completed and status of operation.
+ * Returns 0 - transfer completed (both ACK or NACK),
+ * negative - transfer isn't finished.
+ */
+static int mlxcpld_i2c_check_status(struct mlxcpld_i2c_priv *priv, int *status)
+{
+ u8 val;
+
+ mlxcpld_i2c_read_comm(priv, MLXCPLD_LPCI2C_STATUS_REG, &val, 1);
+
+ if (val & MLXCPLD_LPCI2C_TRANS_END) {
+ if (val & MLXCPLD_LPCI2C_STATUS_NACK)
+ /* The slave is unable to accept the data. No such
+ * slave, command not understood, or unable to accept
+ * any more data.
+ */
+ *status = MLXCPLD_LPCI2C_NACK_IND;
+ else
+ *status = MLXCPLD_LPCI2C_ACK_IND;
+ return 0;
+ }
+ *status = MLXCPLD_LPCI2C_NO_IND;
+
+ return -EIO;
+}
+
+static void mlxcpld_i2c_set_transf_data(struct mlxcpld_i2c_priv *priv,
+ struct i2c_msg *msgs, int num,
+ u8 comm_len)
+{
+ priv->xfer.msg = msgs;
+ priv->xfer.msg_num = num;
+
+ /*
+ * All upper layers currently are never use transfer with more than
+ * 2 messages. Actually, it's also not so relevant in Mellanox systems
+ * because of HW limitation. Max size of transfer is o more than 20B
+ * in current x86 LPCI2C bridge.
+ */
+ priv->xfer.cmd = (msgs[num - 1].flags & I2C_M_RD);
+
+ if (priv->xfer.cmd == I2C_M_RD) {
+ if (comm_len == msgs[0].len) {
+ /* Special case of addr_width = 0 */
+ priv->xfer.addr_width = 0;
+ priv->xfer.data_len = comm_len;
+ } else {
+ priv->xfer.addr_width = msgs[0].len;
+ priv->xfer.data_len = comm_len - priv->xfer.addr_width;
+ }
+ } else {
+ /* Width (I2C_NUM_ADDR reg) isn't used in write command. */
+ priv->xfer.addr_width = 0;
+ priv->xfer.data_len = comm_len;
+ }
+}
+
+/* Reset CPLD LPCI2C block */
+static void mlxcpld_i2c_reset(struct mlxcpld_i2c_priv *priv)
+{
+ u8 val;
+
+ mutex_lock(&priv->lock);
+ mlxcpld_i2c_read_comm(priv, MLXCPLD_LPCI2C_CTRL_REG, &val, 1);
+ val &= ~MLXCPLD_LPCI2C_RST_SEL_MASK;
+ mlxcpld_i2c_write_comm(priv, MLXCPLD_LPCI2C_CTRL_REG, &val, 1);
+ mutex_unlock(&priv->lock);
+}
+
+/* Make sure the CPLD is ready to start transmitting.
+ * Return 0 if it is, -EBUSY if it is not.
+ */
+static int mlxcpld_i2c_check_busy(struct mlxcpld_i2c_priv *priv)
+{
+ u8 val;
+
+ mlxcpld_i2c_read_comm(priv, MLXCPLD_LPCI2C_STATUS_REG, &val, 1);
+
+ if (val & MLXCPLD_LPCI2C_TRANS_END)
+ return 0;
+
+ return -EIO;
+}
+
+static int mlxcpld_i2c_wait_for_free(struct mlxcpld_i2c_priv *priv)
+{
+ int timeout = 0;
+
+ do {
+ if (!mlxcpld_i2c_check_busy(priv))
+ break;
+ usleep_range(priv->poll_time/2, priv->poll_time);
+ timeout += priv->poll_time;
+ } while (timeout < priv->xfer_to);
+
+ if (timeout > priv->xfer_to)
+ return -ETIMEDOUT;
+
+ return 0;
+}
+
+/*
+ * Wait for master transfer to complete.
+ * It puts current process to sleep until we get interrupt or timeout expires.
+ * Returns the number of transferred or read bytes or error (<0).
+ */
+static int mlxcpld_i2c_wait_for_tc(struct mlxcpld_i2c_priv *priv)
+{
+ int status, i = 1, timeout = 0;
+ u8 datalen;
+ int err = 0;
+
+ do {
+ usleep_range(priv->poll_time / 2, priv->poll_time);
+ if (!mlxcpld_i2c_check_status(priv, &status))
+ break;
+ timeout += priv->poll_time;
+ } while (status == 0 && timeout < priv->xfer_to);
+
+ switch (status) {
+ case MLXCPLD_LPCI2C_NO_IND:
+ return -ETIMEDOUT;
+ case MLXCPLD_LPCI2C_ACK_IND:
+ if (priv->xfer.cmd == I2C_M_RD) {
+ /*
+ * Actual read data len will be always the same as
+ * requested len. 0xff (line pull-up) will be returned
+ * if slave has no data to return. Thus don't read
+ * MLXCPLD_LPCI2C_NUM_DAT_REG reg from CPLD.
+ */
+ err = datalen = priv->xfer.data_len;
+ if (priv->xfer.msg_num == 1)
+ i = 0;
+
+ if (!priv->xfer.msg[i].buf)
+ err = -EINVAL;
+ else
+ mlxcpld_i2c_read_comm(priv,
+ MLXCPLD_LPCI2C_DATA_REG,
+ priv->xfer.msg[i].buf,
+ datalen);
+ } else {
+ err = priv->xfer.addr_width + priv->xfer.data_len;
+ }
+ break;
+ case MLXCPLD_LPCI2C_NACK_IND:
+ err = -EAGAIN;
+ break;
+ case MLXCPLD_LPCI2C_ERR_IND:
+ err = -EIO;
+ break;
+ default:
+ break;
+ }
+
+ return err;
+}
+
+static void mlxcpld_i2c_xfer_msg(struct mlxcpld_i2c_priv *priv)
+{
+ int i, len = 0;
+ u8 cmd;
+
+ mlxcpld_i2c_write_comm(priv, MLXCPLD_LPCI2C_NUM_DAT_REG,
+ &priv->xfer.data_len, 1);
+ mlxcpld_i2c_write_comm(priv, MLXCPLD_LPCI2C_NUM_ADDR_REG,
+ &priv->xfer.addr_width, 1);
+
+ for (i = 0; i < priv->xfer.msg_num; i++) {
+ if ((priv->xfer.msg[i].flags & I2C_M_RD) != I2C_M_RD) {
+ /* Don't write to CPLD buffer in read transaction */
+ mlxcpld_i2c_write_comm(priv, MLXCPLD_LPCI2C_DATA_REG +
+ len, priv->xfer.msg[i].buf,
+ priv->xfer.msg[i].len);
+ len += priv->xfer.msg[i].len;
+ }
+ }
+
+ /* Set target slave address with command for master transfer.
+ * It should be latest executed function before CPLD transaction.
+ */
+ cmd = (priv->xfer.msg[0].addr << 1) | priv->xfer.cmd;
+ mlxcpld_i2c_write_comm(priv, MLXCPLD_LPCI2C_CMD_REG, &cmd, 1);
+}
+
+/* Generic lpc-i2c transfer.
+ * Returns the number of processed messages or error (<0).
+ */
+static int mlxcpld_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
+ int num)
+{
+ struct mlxcpld_i2c_priv *priv = i2c_get_adapdata(adap);
+ u8 comm_len = 0;
+ int err;
+
+ err = mlxcpld_i2c_check_msg_params(priv, msgs, num, &comm_len);
+ if (err) {
+ dev_err(&priv->pdev->dev, "Incorrect message\n");
+ return err;
+ }
+
+ /* Check bus state */
+ if (mlxcpld_i2c_wait_for_free(priv)) {
+ dev_err(&priv->pdev->dev, "LPCI2C bridge is busy\n");
+
+ /*
+ * Usually it means something serious has happened.
+ * We can not have unfinished previous transfer
+ * so it doesn't make any sense to try to stop it.
+ * Probably we were not able to recover from the
+ * previous error.
+ * The only reasonable thing - is soft reset.
+ */
+ mlxcpld_i2c_reset(priv);
+ if (mlxcpld_i2c_check_busy(priv)) {
+ dev_err(&priv->pdev->dev, "LPCI2C bridge is busy after reset\n");
+ return -EIO;
+ }
+ }
+
+ mlxcpld_i2c_set_transf_data(priv, msgs, num, comm_len);
+
+ mutex_lock(&priv->lock);
+ /* Do real transfer. Can't fail */
+ mlxcpld_i2c_xfer_msg(priv);
+ /* Wait for transaction complete */
+ err = mlxcpld_i2c_wait_for_tc(priv);
+ mutex_unlock(&priv->lock);
+
+ return err < 0 ? err : num;
+}
+
+static u32 mlxcpld_i2c_func(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA;
+}
+
+static const struct i2c_algorithm mlxcpld_i2c_algo = {
+ .master_xfer = mlxcpld_i2c_xfer,
+ .functionality = mlxcpld_i2c_func
+};
+
+static struct i2c_adapter mlxcpld_i2c_adapter = {
+ .owner = THIS_MODULE,
+ .name = "i2c-mlxcpld",
+ .class = I2C_CLASS_HWMON | I2C_CLASS_SPD,
+ .algo = &mlxcpld_i2c_algo,
+};
+
+static int mlxcpld_i2c_probe(struct platform_device *pdev)
+{
+ struct mlxcpld_i2c_priv *priv;
+ int err;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(struct mlxcpld_i2c_priv),
+ GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ mutex_init(&priv->lock);
+ platform_set_drvdata(pdev, priv);
+ priv->pdev = pdev;
+ priv->xfer_to = MLXCPLD_I2C_XFER_TO;
+ priv->retr_num = MLXCPLD_I2C_RETR_NUM;
+ priv->block_sz = MLXCPLD_I2C_DATA_REG_SZ;
+ priv->poll_time = MLXCPLD_I2C_POLL_TIME;
+ /* Register with i2c layer */
+ priv->adap = mlxcpld_i2c_adapter;
+ priv->adap.dev.parent = &pdev->dev;
+ i2c_set_adapdata(&priv->adap, priv);
+ priv->adap.retries = priv->retr_num;
+ priv->adap.nr = MLXCPLD_I2C_BUS_NUM;
+ priv->adap.timeout = usecs_to_jiffies(priv->xfer_to);
+
+ err = i2c_add_numbered_adapter(&priv->adap);
+ if (err) {
+ dev_err(&pdev->dev, "Failed to add %s adapter (%d)\n",
+ MLXCPLD_I2C_DEVICE_NAME, err);
+ goto fail_adapter;
+ }
+
+ priv->base_addr = MLXPLAT_CPLD_LPC_I2C_BASE_ADRR;
+
+ return 0;
+
+fail_adapter:
+ mutex_destroy(&priv->lock);
+ return err;
+}
+
+static int mlxcpld_i2c_remove(struct platform_device *pdev)
+{
+ struct mlxcpld_i2c_priv *priv = platform_get_drvdata(pdev);
+
+ i2c_del_adapter(&priv->adap);
+ mutex_destroy(&priv->lock);
+
+ return 0;
+}
+
+static struct platform_driver mlxcpld_i2c_driver = {
+ .probe = mlxcpld_i2c_probe,
+ .remove = mlxcpld_i2c_remove,
+ .driver = {
+ .name = MLXCPLD_I2C_DEVICE_NAME,
+ },
+};
+
+module_platform_driver(mlxcpld_i2c_driver);
+
+MODULE_AUTHOR("Michael Shych (michaels@mellanox.com)");
+MODULE_DESCRIPTION("Mellanox I2C-CPLD controller driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:i2c-mlxcpld");
--
2.1.4
^ permalink raw reply related
* [PATCH resend v3] i2c: hibvt: add Hisilicon BVT I2C controller driver
From: Pan Wen @ 2016-09-19 11:29 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland
Cc: linux-i2c, devicetree, linux-kernel, howell.yang, xuejiancheng,
jalen.hsu, lvkuanliang, suwenping, raojun, kevin.lixu, Pan Wen
add Hisilicon BVT I2C controller driver support.
Signed-off-by: Pan Wen <wenpan@hisilicon.com>
---
.../devicetree/bindings/i2c/i2c-hibvt.txt | 24 +
drivers/i2c/busses/Kconfig | 10 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-hibvt.c | 737 +++++++++++++++++++++
4 files changed, 772 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-hibvt.txt
create mode 100644 drivers/i2c/busses/i2c-hibvt.c
diff --git a/Documentation/devicetree/bindings/i2c/i2c-hibvt.txt b/Documentation/devicetree/bindings/i2c/i2c-hibvt.txt
new file mode 100644
index 0000000..db3d2e2
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-hibvt.txt
@@ -0,0 +1,24 @@
+Hisilicon BVT I2C master controller
+
+Required properties:
+- compatible: should be "hisilicon,hibvt-i2c" and one of the following:
+ "hisilicon,hi3516cv300-i2c"
+- reg: physical base address of the controller and length of memory mapped.
+ region.
+- interrupts: interrupt number to the cpu.
+- clocks: phandles to input clocks.
+
+Optional properties:
+- clock-frequency: Desired I2C bus frequency in Hz, otherwise defaults to 100000.
+
+Other properties:
+see Documentation/devicetree/bindings/i2c/i2c.txt.
+
+Examples:
+i2c_bus0: i2c@12110000 {
+ compatible = "hisilicon,hi3516cv300-i2c", "hisilicon,hibvt-i2c";
+ reg = <0x12110000 0x100>;
+ interrupts = <20>;
+ clocks = <&crg_ctrl HI3516CV300_APB_CLK>;
+ clock-frequency = <100000>;
+};
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 5c3993b..fc1b679 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -555,6 +555,16 @@ config I2C_GPIO
This is a very simple bitbanging I2C driver utilizing the
arch-neutral GPIO API to control the SCL and SDA lines.
+config I2C_HIBVT
+ tristate "Hisilicon BVT I2C Controller"
+ depends on ARCH_HISI
+ help
+ Say Y here to include support for Hisilicon BVT I2C controller in the
+ Hisilicon BVT SoCs.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-hibvt.
+
config I2C_HIGHLANDER
tristate "Highlander FPGA SMBus interface"
depends on SH_HIGHLANDER
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 37f2819..42ef2e0 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_I2C_EG20T) += i2c-eg20t.o
obj-$(CONFIG_I2C_EMEV2) += i2c-emev2.o
obj-$(CONFIG_I2C_EXYNOS5) += i2c-exynos5.o
obj-$(CONFIG_I2C_GPIO) += i2c-gpio.o
+obj-$(CONFIG_I2C_HIBVT) += i2c-hibvt.o
obj-$(CONFIG_I2C_HIGHLANDER) += i2c-highlander.o
obj-$(CONFIG_I2C_HIX5HD2) += i2c-hix5hd2.o
obj-$(CONFIG_I2C_IBM_IIC) += i2c-ibm_iic.o
diff --git a/drivers/i2c/busses/i2c-hibvt.c b/drivers/i2c/busses/i2c-hibvt.c
new file mode 100644
index 0000000..abe8a07
--- /dev/null
+++ b/drivers/i2c/busses/i2c-hibvt.c
@@ -0,0 +1,737 @@
+/*
+ * Hisilicon BVT I2C Controller Driver
+ *
+ * Copyright (c) 2016 HiSilicon Technologies Co., Ltd.
+ *
+ * Authors: wenpan@hisilicon.com
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+/*
+ * I2C Registers offsets
+ */
+#define HIBVT_I2C_GLB 0x0
+#define HIBVT_I2C_SCL_H 0x4
+#define HIBVT_I2C_SCL_L 0x8
+#define HIBVT_I2C_DATA1 0x10
+#define HIBVT_I2C_TXF 0x20
+#define HIBVT_I2C_RXF 0x24
+#define HIBVT_I2C_CMD_BASE 0x30
+#define HIBVT_I2C_LOOP1 0xb0
+#define HIBVT_I2C_DST1 0xb4
+#define HIBVT_I2C_TX_WATER 0xc8
+#define HIBVT_I2C_RX_WATER 0xcc
+#define HIBVT_I2C_CTRL1 0xd0
+#define HIBVT_I2C_STAT 0xd8
+#define HIBVT_I2C_INTR_RAW 0xe0
+#define HIBVT_I2C_INTR_EN 0xe4
+#define HIBVT_I2C_INTR_STAT 0xe8
+
+/*
+ * I2C Global Config Register -- HIBVT_I2C_GLB
+ */
+#define GLB_EN_MASK BIT(0)
+#define GLB_SDA_HOLD_MASK GENMASK(23, 8)
+#define GLB_SDA_HOLD_SHIFT (8)
+
+/*
+ * I2C Timing CMD Register -- HIBVT_I2C_CMD_BASE + n * 4 (n = 0, 1, 2, ... 31)
+ */
+#define CMD_EXIT 0x0
+#define CMD_TX_S 0x1
+#define CMD_TX_D1_2 0x4
+#define CMD_TX_D1_1 0x5
+#define CMD_TX_FIFO 0x9
+#define CMD_RX_FIFO 0x12
+#define CMD_RX_ACK 0x13
+#define CMD_IGN_ACK 0x15
+#define CMD_TX_ACK 0x16
+#define CMD_TX_NACK 0x17
+#define CMD_JMP1 0x18
+#define CMD_UP_TXF 0x1d
+#define CMD_TX_RS 0x1e
+#define CMD_TX_P 0x1f
+
+/*
+ * I2C Control Register 1 -- HIBVT_I2C_CTRL1
+ */
+#define CTRL1_CMD_START_MASK BIT(0)
+
+/*
+ * I2C Status Register -- HIBVT_I2C_STAT
+ */
+#define STAT_RXF_NOE_MASK BIT(16) /* RX FIFO not empty flag */
+#define STAT_TXF_NOF_MASK BIT(19) /* TX FIFO not full flag */
+
+
+/*
+ * I2C Interrupt status and mask Register --
+ * HIBVT_I2C_INTR_RAW, HIBVT_I2C_STAT, HIBVT_I2C_INTR_STAT
+ */
+#define INTR_ABORT_MASK (BIT(0) | BIT(11))
+#define INTR_RX_MASK BIT(2)
+#define INTR_TX_MASK BIT(4)
+#define INTR_CMD_DONE_MASK BIT(12)
+#define INTR_USE_MASK (INTR_ABORT_MASK \
+ |INTR_RX_MASK \
+ | INTR_TX_MASK \
+ | INTR_CMD_DONE_MASK)
+#define INTR_ALL_MASK GENMASK(31, 0)
+
+#define I2C_DEFAULT_FREQUENCY 0x100000
+#define I2C_TXF_DEPTH 64
+#define I2C_RXF_DEPTH 64
+#define I2C_TXF_WATER 32
+#define I2C_RXF_WATER 32
+#define I2C_WAIT_TIMEOUT 0x10000
+#define I2C_IRQ_TIMEOUT (msecs_to_jiffies(1000))
+
+
+struct hibvt_i2c_dev {
+ struct device *dev;
+ struct i2c_adapter adap;
+ void __iomem *base;
+ struct clk *clk;
+ int irq;
+
+ unsigned int freq;
+ struct i2c_msg *msg;
+ unsigned int msg_num;
+ unsigned int msg_idx;
+ unsigned int msg_buf_ptr;
+ struct completion msg_complete;
+
+ spinlock_t lock;
+ int status;
+};
+
+static inline void hibvt_i2c_disable(struct hibvt_i2c_dev *i2c)
+{
+ unsigned int val;
+
+ val = readl(i2c->base + HIBVT_I2C_GLB);
+ val &= ~GLB_EN_MASK;
+ writel(val, i2c->base + HIBVT_I2C_GLB);
+}
+
+static inline void hibvt_i2c_enable(struct hibvt_i2c_dev *i2c)
+{
+ unsigned int val;
+
+ val = readl(i2c->base + HIBVT_I2C_GLB);
+ val |= GLB_EN_MASK;
+ writel(val, i2c->base + HIBVT_I2C_GLB);
+}
+
+static inline void hibvt_i2c_cfg_irq(struct hibvt_i2c_dev *i2c,
+ unsigned int flag)
+{
+ writel(flag, i2c->base + HIBVT_I2C_INTR_EN);
+}
+
+static inline void hibvt_i2c_disable_irq(struct hibvt_i2c_dev *i2c,
+ unsigned int flag)
+{
+ unsigned int val;
+
+ val = readl(i2c->base + HIBVT_I2C_INTR_EN);
+ val &= ~flag;
+ writel(val, i2c->base + HIBVT_I2C_INTR_EN);
+}
+
+static inline unsigned int hibvt_i2c_clr_irq(struct hibvt_i2c_dev *i2c)
+{
+ unsigned int val;
+
+ val = readl(i2c->base + HIBVT_I2C_INTR_STAT);
+ writel(INTR_ALL_MASK, i2c->base + HIBVT_I2C_INTR_RAW);
+
+ return val;
+}
+
+static inline void hibvt_i2c_cmdreg_set(struct hibvt_i2c_dev *i2c,
+ unsigned int cmd, unsigned int *offset)
+{
+ dev_dbg(i2c->dev, "hii2c reg: offset=0x%x, cmd=0x%x...\n",
+ *offset * 4, cmd);
+ writel(cmd, i2c->base + HIBVT_I2C_CMD_BASE + *offset * 4);
+ (*offset)++;
+}
+
+/*
+ * config i2c slave addr
+ */
+static inline void hibvt_i2c_set_addr(struct hibvt_i2c_dev *i2c)
+{
+ struct i2c_msg *msg = i2c->msg;
+ u16 addr;
+
+ if (msg->flags & I2C_M_TEN) {
+ /* First byte is 11110XX0 where XX is upper 2 bits */
+ addr = ((msg->addr & 0x300) << 1) | 0xf000;
+ if (msg->flags & I2C_M_RD)
+ addr |= 1 << 8;
+
+ /* Second byte is the remaining 8 bits */
+ addr |= msg->addr & 0xff;
+ } else {
+ addr = (msg->addr & 0x7f) << 1;
+ if (msg->flags & I2C_M_RD)
+ addr |= 1;
+ }
+
+ writel(addr, i2c->base + HIBVT_I2C_DATA1);
+}
+
+/*
+ * Start command sequence
+ */
+static inline void hibvt_i2c_start_cmd(struct hibvt_i2c_dev *i2c)
+{
+ unsigned int val;
+
+ val = readl(i2c->base + HIBVT_I2C_CTRL1);
+ val |= CTRL1_CMD_START_MASK;
+ writel(val, i2c->base + HIBVT_I2C_CTRL1);
+}
+
+static int hibvt_i2c_wait_rx_noempty(struct hibvt_i2c_dev *i2c)
+{
+ unsigned int time_cnt = 0;
+ unsigned int val;
+
+ do {
+ val = readl(i2c->base + HIBVT_I2C_STAT);
+ if (val & STAT_RXF_NOE_MASK)
+ return 0;
+
+ udelay(50);
+ } while (time_cnt++ < I2C_WAIT_TIMEOUT);
+
+ dev_err(i2c->dev, "wait rx no empty timeout, RIS: 0x%x, SR: 0x%x\n",
+ readl(i2c->base + HIBVT_I2C_INTR_RAW), val);
+ return -EIO;
+}
+
+static int hibvt_i2c_wait_tx_nofull(struct hibvt_i2c_dev *i2c)
+{
+ unsigned int time_cnt = 0;
+ unsigned int val;
+
+ do {
+ val = readl(i2c->base + HIBVT_I2C_STAT);
+ if (val & STAT_TXF_NOF_MASK)
+ return 0;
+
+ udelay(50);
+ } while (time_cnt++ < I2C_WAIT_TIMEOUT);
+
+ dev_err(i2c->dev, "wait rx no empty timeout, RIS: 0x%x, SR: 0x%x\n",
+ readl(i2c->base + HIBVT_I2C_INTR_RAW), val);
+ return -EIO;
+}
+
+static int hibvt_i2c_wait_idle(struct hibvt_i2c_dev *i2c)
+{
+ unsigned int time_cnt = 0;
+ unsigned int val;
+
+ do {
+ val = readl(i2c->base + HIBVT_I2C_INTR_RAW);
+ if (val & (INTR_ABORT_MASK)) {
+ dev_err(i2c->dev, "wait idle abort!, RIS: 0x%x\n",
+ val);
+ return -EIO;
+ }
+
+ if (val & INTR_CMD_DONE_MASK)
+ return 0;
+
+ udelay(50);
+ } while (time_cnt++ < I2C_WAIT_TIMEOUT);
+
+ dev_err(i2c->dev, "wait idle timeout, RIS: 0x%x, SR: 0x%x\n",
+ val, readl(i2c->base + HIBVT_I2C_STAT));
+
+ return -EIO;
+}
+
+static void hibvt_i2c_set_freq(struct hibvt_i2c_dev *i2c)
+{
+ unsigned int max_freq, freq;
+ unsigned int clk_rate;
+ unsigned int val, sda_hold;
+
+ freq = i2c->freq;
+ clk_rate = clk_get_rate(i2c->clk);
+ max_freq = clk_rate >> 1;
+
+ if (freq > max_freq) {
+ i2c->freq = max_freq;
+ freq = i2c->freq;
+ }
+
+ if (freq <= 100000) {
+ val = clk_rate / (freq * 2) - 1;
+ writel(val, i2c->base + HIBVT_I2C_SCL_H);
+ writel(val, i2c->base + HIBVT_I2C_SCL_L);
+ } else {
+ val = (clk_rate * 36) / (freq * 100);
+ writel(val, i2c->base + HIBVT_I2C_SCL_H);
+ val = (clk_rate * 64) / (freq * 100);
+ writel(val, i2c->base + HIBVT_I2C_SCL_L);
+ }
+
+ sda_hold = val * 3 / 10;
+ sda_hold = (sda_hold << GLB_SDA_HOLD_SHIFT) & GLB_SDA_HOLD_MASK;
+ val = readl(i2c->base + HIBVT_I2C_GLB);
+ val &= ~GLB_SDA_HOLD_MASK;
+ val |= sda_hold;
+ writel(val, i2c->base + HIBVT_I2C_GLB);
+}
+
+/*
+ * set i2c controller TX and RX FIFO water
+ */
+static inline void hibvt_i2c_set_water(struct hibvt_i2c_dev *i2c)
+{
+ writel(I2C_TXF_WATER, i2c->base + HIBVT_I2C_TX_WATER);
+ writel(I2C_RXF_WATER, i2c->base + HIBVT_I2C_RX_WATER);
+}
+
+/*
+ * initialise the controller, set i2c bus interface freq
+ */
+static void hibvt_i2c_hw_init(struct hibvt_i2c_dev *i2c)
+{
+ hibvt_i2c_disable(i2c);
+ hibvt_i2c_disable_irq(i2c, INTR_ALL_MASK);
+ hibvt_i2c_set_freq(i2c);
+ hibvt_i2c_set_water(i2c);
+}
+
+/*
+ * hibvt_i2c_cfg_cmd - config i2c controller command sequence
+ *
+ * After all the timing command is configured,
+ * and then start the command, you can i2c communication,
+ * and then only need to read and write i2c fifo.
+ */
+static void hibvt_i2c_cfg_cmd(struct hibvt_i2c_dev *i2c)
+{
+ struct i2c_msg *msg = i2c->msg;
+ int offset = 0;
+
+ if (i2c->msg_idx == 0)
+ hibvt_i2c_cmdreg_set(i2c, CMD_TX_S, &offset);
+ else
+ hibvt_i2c_cmdreg_set(i2c, CMD_TX_RS, &offset);
+
+ if (msg->flags & I2C_M_TEN) {
+ if (i2c->msg_idx == 0) {
+ hibvt_i2c_cmdreg_set(i2c, CMD_TX_D1_2, &offset);
+ hibvt_i2c_cmdreg_set(i2c, CMD_TX_D1_1, &offset);
+ } else {
+ hibvt_i2c_cmdreg_set(i2c, CMD_TX_D1_2, &offset);
+ }
+ } else {
+ hibvt_i2c_cmdreg_set(i2c, CMD_TX_D1_1, &offset);
+ }
+
+ if (msg->flags & I2C_M_IGNORE_NAK)
+ hibvt_i2c_cmdreg_set(i2c, CMD_IGN_ACK, &offset);
+ else
+ hibvt_i2c_cmdreg_set(i2c, CMD_RX_ACK, &offset);
+
+ if (msg->flags & I2C_M_RD) {
+ if (msg->len >= 2) {
+ writel(offset, i2c->base + HIBVT_I2C_DST1);
+ writel(msg->len - 2, i2c->base + HIBVT_I2C_LOOP1);
+ hibvt_i2c_cmdreg_set(i2c, CMD_RX_FIFO, &offset);
+ hibvt_i2c_cmdreg_set(i2c, CMD_TX_ACK, &offset);
+ hibvt_i2c_cmdreg_set(i2c, CMD_JMP1, &offset);
+ }
+ hibvt_i2c_cmdreg_set(i2c, CMD_RX_FIFO, &offset);
+ hibvt_i2c_cmdreg_set(i2c, CMD_TX_NACK, &offset);
+ } else {
+ writel(offset, i2c->base + HIBVT_I2C_DST1);
+ writel(msg->len - 1, i2c->base + HIBVT_I2C_LOOP1);
+ hibvt_i2c_cmdreg_set(i2c, CMD_UP_TXF, &offset);
+ hibvt_i2c_cmdreg_set(i2c, CMD_TX_FIFO, &offset);
+
+ if (msg->flags & I2C_M_IGNORE_NAK)
+ hibvt_i2c_cmdreg_set(i2c, CMD_IGN_ACK, &offset);
+ else
+ hibvt_i2c_cmdreg_set(i2c, CMD_RX_ACK, &offset);
+
+ hibvt_i2c_cmdreg_set(i2c, CMD_JMP1, &offset);
+ }
+
+ if ((i2c->msg_idx == (i2c->msg_num - 1)) || (msg->flags & I2C_M_STOP)) {
+ dev_dbg(i2c->dev, "run to %s %d...TX STOP\n",
+ __func__, __LINE__);
+ hibvt_i2c_cmdreg_set(i2c, CMD_TX_P, &offset);
+ }
+
+ hibvt_i2c_cmdreg_set(i2c, CMD_EXIT, &offset);
+}
+
+static int hibvt_i2c_polling_xfer_one_msg(struct hibvt_i2c_dev *i2c)
+{
+ int status;
+ unsigned int val;
+ struct i2c_msg *msg = i2c->msg;
+
+ dev_dbg(i2c->dev, "[%s,%d]msg->flags=0x%x, len=0x%x\n",
+ __func__, __LINE__, msg->flags, msg->len);
+
+ hibvt_i2c_enable(i2c);
+ hibvt_i2c_clr_irq(i2c);
+ hibvt_i2c_set_addr(i2c);
+ hibvt_i2c_cfg_cmd(i2c);
+ hibvt_i2c_start_cmd(i2c);
+
+ i2c->msg_buf_ptr = 0;
+
+ if (msg->flags & I2C_M_RD) {
+ while (i2c->msg_buf_ptr < msg->len) {
+ status = hibvt_i2c_wait_rx_noempty(i2c);
+ if (status)
+ goto end;
+
+ val = readl(i2c->base + HIBVT_I2C_RXF);
+ msg->buf[i2c->msg_buf_ptr] = val;
+ i2c->msg_buf_ptr++;
+
+ }
+ } else {
+ while (i2c->msg_buf_ptr < msg->len) {
+ status = hibvt_i2c_wait_tx_nofull(i2c);
+ if (status)
+ goto end;
+
+ val = msg->buf[i2c->msg_buf_ptr];
+ writel(val, i2c->base + HIBVT_I2C_TXF);
+ i2c->msg_buf_ptr++;
+ }
+ }
+
+ status = hibvt_i2c_wait_idle(i2c);
+end:
+ hibvt_i2c_disable(i2c);
+
+ return status;
+}
+
+static irqreturn_t hibvt_i2c_isr(int irq, void *dev_id)
+{
+ struct hibvt_i2c_dev *i2c = dev_id;
+ unsigned int irq_status;
+ struct i2c_msg *msg = i2c->msg;
+
+ spin_lock(&i2c->lock);
+
+ irq_status = hibvt_i2c_clr_irq(i2c);
+ dev_dbg(i2c->dev, "%s RIS: 0x%x\n", __func__, irq_status);
+
+ if (!irq_status) {
+ dev_dbg(i2c->dev, "no irq\n");
+ goto end;
+ }
+
+ if (irq_status & INTR_ABORT_MASK) {
+ dev_err(i2c->dev, "irq handle abort, RIS: 0x%x\n",
+ irq_status);
+ i2c->status = -EIO;
+ hibvt_i2c_disable_irq(i2c, INTR_ALL_MASK);
+
+ complete(&i2c->msg_complete);
+ goto end;
+ }
+
+ if (msg->flags & I2C_M_RD) {
+ while ((readl(i2c->base + HIBVT_I2C_STAT) & STAT_RXF_NOE_MASK)
+ && (i2c->msg_buf_ptr < msg->len)) {
+ msg->buf[i2c->msg_buf_ptr] =
+ readl(i2c->base + HIBVT_I2C_RXF);
+ i2c->msg_buf_ptr++;
+ }
+ } else {
+ while ((readl(i2c->base + HIBVT_I2C_STAT) & STAT_TXF_NOF_MASK)
+ && (i2c->msg_buf_ptr < msg->len)) {
+ writel(msg->buf[i2c->msg_buf_ptr],
+ i2c->base + HIBVT_I2C_TXF);
+ i2c->msg_buf_ptr++;
+ }
+ }
+
+ if (i2c->msg_buf_ptr >= msg->len)
+ hibvt_i2c_disable_irq(i2c, INTR_TX_MASK | INTR_RX_MASK);
+
+ if (irq_status & INTR_CMD_DONE_MASK) {
+ dev_dbg(i2c->dev, "cmd done\n");
+ i2c->status = 0;
+ hibvt_i2c_disable_irq(i2c, INTR_ALL_MASK);
+
+ complete(&i2c->msg_complete);
+ }
+
+end:
+ spin_unlock(&i2c->lock);
+
+ return IRQ_HANDLED;
+}
+
+static int hibvt_i2c_interrupt_xfer_one_msg(struct hibvt_i2c_dev *i2c)
+{
+ int status;
+ struct i2c_msg *msg = i2c->msg;
+ unsigned long timeout;
+ unsigned long flags;
+
+ dev_dbg(i2c->dev, "[%s,%d]msg->flags=0x%x, len=0x%x\n",
+ __func__, __LINE__, msg->flags, msg->len);
+
+ reinit_completion(&i2c->msg_complete);
+ i2c->msg_buf_ptr = 0;
+ i2c->status = -EIO;
+
+ spin_lock_irqsave(&i2c->lock, flags);
+ hibvt_i2c_enable(i2c);
+ hibvt_i2c_clr_irq(i2c);
+ if (msg->flags & I2C_M_RD)
+ hibvt_i2c_cfg_irq(i2c, INTR_USE_MASK & ~INTR_TX_MASK);
+ else
+ hibvt_i2c_cfg_irq(i2c, INTR_USE_MASK & ~INTR_RX_MASK);
+
+ hibvt_i2c_set_addr(i2c);
+ hibvt_i2c_cfg_cmd(i2c);
+ hibvt_i2c_start_cmd(i2c);
+ spin_unlock_irqrestore(&i2c->lock, flags);
+
+ timeout = wait_for_completion_timeout(&i2c->msg_complete,
+ I2C_IRQ_TIMEOUT);
+
+ if (timeout == 0) {
+ hibvt_i2c_disable_irq(i2c, INTR_ALL_MASK);
+ status = -EIO;
+ dev_err(i2c->dev, "%s timeout\n",
+ msg->flags & I2C_M_RD ? "rx" : "tx");
+ } else {
+ status = i2c->status;
+ }
+
+ hibvt_i2c_disable(i2c);
+
+ return status;
+}
+
+/*
+ * Master transfer function
+ */
+static int hibvt_i2c_xfer(struct i2c_adapter *adap,
+ struct i2c_msg *msgs, int num)
+{
+ struct hibvt_i2c_dev *i2c = i2c_get_adapdata(adap);
+ int status;
+
+ if (!msgs) {
+ dev_err(i2c->dev, "msgs == NULL\n");
+ return -EIO;
+ }
+
+ i2c->msg = msgs;
+ i2c->msg_num = num;
+ i2c->msg_idx = 0;
+
+ if (i2c->irq >= 0) {
+ while (i2c->msg_idx < i2c->msg_num) {
+ status = hibvt_i2c_interrupt_xfer_one_msg(i2c);
+ if (status)
+ break;
+
+ i2c->msg++;
+ i2c->msg_idx++;
+ }
+ } else {
+ while (i2c->msg_idx < i2c->msg_num) {
+ status = hibvt_i2c_polling_xfer_one_msg(i2c);
+ if (status)
+ break;
+
+ i2c->msg++;
+ i2c->msg_idx++;
+ }
+ }
+
+ if (!status || i2c->msg_idx > 0)
+ status = i2c->msg_idx;
+
+ return status;
+}
+
+static u32 hibvt_i2c_func(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR
+ | I2C_FUNC_PROTOCOL_MANGLING;
+}
+
+static const struct i2c_algorithm hibvt_i2c_algo = {
+ .master_xfer = hibvt_i2c_xfer,
+ .functionality = hibvt_i2c_func,
+};
+
+static int hibvt_i2c_probe(struct platform_device *pdev)
+{
+ int status;
+ struct hibvt_i2c_dev *i2c;
+ struct i2c_adapter *adap;
+ struct resource *res;
+
+ i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
+ if (!i2c)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, i2c);
+ i2c->dev = &pdev->dev;
+ spin_lock_init(&i2c->lock);
+ init_completion(&i2c->msg_complete);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ i2c->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(i2c->base)) {
+ dev_err(i2c->dev, "cannot ioremap resource\n");
+ return -ENOMEM;
+ }
+
+ i2c->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(i2c->clk)) {
+ dev_err(i2c->dev, "cannot get clock\n");
+ return -ENOENT;
+ }
+ clk_prepare_enable(i2c->clk);
+
+ if (of_property_read_u32(pdev->dev.of_node, "clock-frequency",
+ &i2c->freq)) {
+ dev_warn(i2c->dev, "setting default clock-frequency@%dHz\n",
+ I2C_DEFAULT_FREQUENCY);
+ i2c->freq = I2C_DEFAULT_FREQUENCY;
+ }
+
+ /* i2c controller initialization, disable interrupt */
+ hibvt_i2c_hw_init(i2c);
+
+ i2c->irq = platform_get_irq(pdev, 0);
+ status = devm_request_irq(&pdev->dev, i2c->irq, hibvt_i2c_isr,
+ IRQF_SHARED, dev_name(&pdev->dev), i2c);
+ if (status) {
+ dev_dbg(i2c->dev, "falling back to polling mode");
+ i2c->irq = -1;
+ }
+
+ adap = &i2c->adap;
+ i2c_set_adapdata(adap, i2c);
+ adap->owner = THIS_MODULE;
+ strlcpy(adap->name, "hibvt-i2c", sizeof(adap->name));
+ adap->dev.parent = &pdev->dev;
+ adap->dev.of_node = pdev->dev.of_node;
+ adap->algo = &hibvt_i2c_algo;
+
+ /* Add the i2c adapter */
+ status = i2c_add_adapter(adap);
+ if (status) {
+ dev_err(i2c->dev, "failed to add bus to i2c core\n");
+ goto err_add_adapter;
+ }
+
+ dev_info(i2c->dev, "%s%d@%dhz registered\n",
+ adap->name, adap->nr, i2c->freq);
+
+ return 0;
+
+err_add_adapter:
+ clk_disable_unprepare(i2c->clk);
+ return status;
+}
+
+static int hibvt_i2c_remove(struct platform_device *pdev)
+{
+ struct hibvt_i2c_dev *i2c = platform_get_drvdata(pdev);
+
+ clk_disable_unprepare(i2c->clk);
+ i2c_del_adapter(&i2c->adap);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int hibvt_i2c_suspend(struct device *dev)
+{
+ struct hibvt_i2c_dev *i2c = dev_get_drvdata(dev);
+
+ i2c_lock_adapter(&i2c->adap);
+ clk_disable_unprepare(i2c->clk);
+ i2c_unlock_adapter(&i2c->adap);
+
+ return 0;
+}
+
+static int hibvt_i2c_resume(struct device *dev)
+{
+ struct hibvt_i2c_dev *i2c = dev_get_drvdata(dev);
+
+ i2c_lock_adapter(&i2c->adap);
+ clk_prepare_enable(i2c->clk);
+ hibvt_i2c_hw_init(i2c);
+ i2c_unlock_adapter(&i2c->adap);
+
+ return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(hibvt_i2c_dev_pm, hibvt_i2c_suspend,
+ hibvt_i2c_resume);
+
+static const struct of_device_id hibvt_i2c_match[] = {
+ { .compatible = "hisilicon,hibvt-i2c"},
+ { .compatible = "hisilicon,hi3516cv300-i2c"},
+ {},
+};
+MODULE_DEVICE_TABLE(of, hibvt_i2c_match);
+
+static struct platform_driver hibvt_i2c_driver = {
+ .driver = {
+ .name = "hibvt-i2c",
+ .of_match_table = hibvt_i2c_match,
+ .pm = &hibvt_i2c_dev_pm,
+ },
+ .probe = hibvt_i2c_probe,
+ .remove = hibvt_i2c_remove,
+};
+
+module_platform_driver(hibvt_i2c_driver);
+
+MODULE_AUTHOR("Pan Wen, <wenpan@hisilicon.com>");
+MODULE_DESCRIPTION("HISILICON BVT I2C Bus driver");
+MODULE_LICENSE("GPL v2");
--
2.9.3
^ permalink raw reply related
* [PATCH] i2c: fix device_node_continue.cocci warnings
From: Julia Lawall @ 2016-09-19 9:47 UTC (permalink / raw)
To: Joel Stanley
Cc: kbuild-all, Jeremy Kerr, Milton Miller, Wolfram Sang, linux-i2c,
linux-kernel
Device node iterators put the previous value of the index variable, so an
explicit put causes a double put.
Generated by: scripts/coccinelle/iterators/device_node_continue.cocci
Signed-off-by: Julia Lawall <julia.lawall@lip6.fr>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
Please take the patch only if it's a positive warning. Thanks!
I haven't checked anything more that what is shown in the patch.
julia
i2c-aspeed.c | 1 -
1 file changed, 1 deletion(-)
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -862,7 +862,6 @@ static int ast_i2c_probe_controller(stru
continue;
of_platform_device_create(np, bus_id, &pdev->dev);
- of_node_put(np);
}
return 0;
^ permalink raw reply
* Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning
From: Peter Zijlstra @ 2016-09-19 9:03 UTC (permalink / raw)
To: Peter Rosin
Cc: Bartosz Golaszewski, Wolfram Sang, Linus Walleij,
Alexandre Courbot, Andy Shevchenko, Vignesh R, Yong Li,
Geert Uytterhoeven, Ingo Molnar, linux-i2c, linux-gpio, LKML
In-Reply-To: <88940f9a-79bf-6c88-8e1d-f76f32dda04a@axentia.se>
On Mon, Sep 19, 2016 at 10:48:44AM +0200, Peter Rosin wrote:
> On 2016-09-19 10:14, Peter Zijlstra wrote:
> > On Mon, Sep 19, 2016 at 10:01:49AM +0200, Peter Rosin wrote:
> >> Or, do what the i2c-mux code is doing and use an rt_mutex instead
> >> of an ordinary mutex. That way you are very sure to not get any
> >> lockdep splat ... at all. Ok, sorry, that was not a serious
> >> suggestion, but it would be a tad bit simpler to implement...
> >
> > So I find it weird that people use rt_mutex as a locking primitive,
> > since its only that one lock that then does PI and all the other locks
> > that are related still create inversions.
>
> So, someone took the bait :-)
>
> Yes, I too find it weird, and would like to get rid of it. It's just
> odd. It's been some years since the start though, waaay before me
> entering kernel space.
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=194684e596af4b
>
>
> But it's hard to argue with the numbers given in the discussion:
>
> http://linux-i2c.vger.kernel.narkive.com/nokldJcc/patch-1-1-i2c-prevent-priority-inversion-on-top-of-bus-lock
>
> Has anything happened to the regular mutex implementation that might
> have changed the picture? *crosses fingers*
Use the -RT kernel and all locks will end up as rt_mutex. Avoiding
inversion on one specific lock, while there are then a gazillion other
than can equally create inversion doesn't make sense to me.
^ permalink raw reply
* Re: [PATCH] i2c-eg20t: use dynamically registered adapter number
From: Jean Delvare @ 2016-09-19 9:02 UTC (permalink / raw)
To: Wolfram Sang; +Cc: Yadi Hu, linux-i2c
In-Reply-To: <20160918192250.GA1415@katana>
Hi Wolfram,
On Sun, 18 Sep 2016 21:22:50 +0200, Wolfram Sang wrote:
>
> > If not, it may make sense to add a helper function exposing
> > __i2c_first_dynamic_bus_num to drivers (something like
> > i2c_is_dynamic_bus_num().) After all, i2c_add_numbered_adapter() mostly
> > makes sense if static i2c device definitions exist. If not,
> > i2c_add_adapter() is just as good. So something like:
> >
> > if (i2c_is_dynamic_bus_num(i))
> > ret = i2c_add_adapter(pch_adap);
> > else {
> > pch_adap->nr = i;
> > ret = i2c_add_numbered_adapter(pch_adap);
> > }
> >
> > may make sense. Unless someone has a better idea.
>
> PASEMI does:
>
> smbus->adapter.nr = PCI_FUNC(dev->devfn);
>
> I am unsure if there is any guarantee in what order PCI_FUNCs are
> probed, yet I have the feeling we could try a little harder to get the
> numbered adapter. What about this (untested, just to get the idea)?
>
> static inline int i2c_add_adapter_try_numbered(struct i2c_adapter *new_adap)
> {
> int ret;
> struct i2c_adapter *old_adap = i2c_get_adapter(new_adap->nr);
>
> if (old_adap && new_adap->nr >= __i2c_first_dynamic_bus_num) {
Note that __i2c_first_dynamic_bus_num is not currently available to
device driver, so your function can't actually be inlined. This is the
reason why I proposed to introduce i2c_is_dynamic_bus_num(). If you
prefer to expose __i2c_first_dynamic_bus_num directly instead, that's
fine with me, but then you probably want to rename it.
> i2c_put_adapter(old_adap);
> dev_dbg(&new_adap->dev, "Static bus number occupied, trying dynamic number\n");
> ret = i2c_add_adapter(new_adap);
> } else {
> ret = i2c_add_numbered_adapter(new_adap);
You may be leaking a reference to old_adap here (if old_adap is set but
new_adap->nr < __i2c_first_dynamic_bus_num.)
> }
>
> return ret;
> }
I'm a bit confused by the logic, it seems unnecessarily complex to me
(but please keep in mind I am working in a noisy environment these days
so maybe it's just me.)
If old_adap is set, i2c_add_numbered_adapter() has no chance of
working. So why are you additionally comparing with
__i2c_first_dynamic_bus_num? For me you should check either, not both.
My own proposal was checking __i2c_first_dynamic_bus_num. To be honest
I can't see the added value of relying on i2c_get_adapter() instead.
Would the result not be exactly the same? Plus it seems racy, just
because i2c_get_adapter() returns NULL at one point in time doesn't
mean the bus numbers will not have been assigned by the time you call
i2c_add_numbered_adapter().
> I used 'static inline' because I think the drivers needing this should
> carry the extra weight. But no major objection to put sth like this also
> into the core. The documentation for this function should carry a big
> note that this is only a workaround and it should not be used directly.
I don't care much where it goes, to be honest.
If you consider it a workaround, what would be the "real fix" for you?
I was wondering if selecting one of these drivers could set a Kconfig
option to initialize __i2c_first_dynamic_bus_num to a non-zero value.
Unfortunately there does not seem to be a way to set a numeric value to
a Kconfig option using select. We would have to do it indirectly as
with CONFIG_HZ:
choice
default I2C_RESERVED_BUS_NR_0
config I2C_RESERVED_BUS_NR_0
config I2C_RESERVED_BUS_NR_2
endchoice
config I2C_RESERVED_BUS_NR
int
default 0 if I2C_RESERVED_BUS_NR_0
default 2 if I2C_RESERVED_BUS_NR_2
config I2C_EG20T
tristate "Intel EG20T PCH/LAPIS Semicon IOH(ML7213/ML7223/ML7831) I2C"
depends on PCI && (X86_32 || MIPS || COMPILE_TEST)
select I2C_RESERVED_BUS_NR_2
And in i2c-core.c:
__i2c_first_dynamic_bus_num = I2C_RESERVED_BUS_NR;
If that's possible at all... I'm not sure if select works on choice
config options.
Alternatively we could set the default directly based on driver
selection:
config I2C_RESERVED_BUS_NR
int
default 0
default 2 if I2C_EG20T
This is more simple but a little harder to maintain. One possible
problem is if the number of buses isn't known at build time but could
change depending on the hardware, for example. Also I don't know if
more than one such driver can be included in the same kernel.
Or we can make it a user-visible option and leave it on whoever
configures the kernel to get it right. In all cases it would move the
decision to build-time instead of being set dynamically at run-time.
Note that I am not claiming this is necessarily better than my initial
proposal. I just wanted to mention the possibility.
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply
* Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning
From: Peter Rosin @ 2016-09-19 8:48 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Bartosz Golaszewski, Wolfram Sang, Linus Walleij,
Alexandre Courbot, Andy Shevchenko, Vignesh R, Yong Li,
Geert Uytterhoeven, Ingo Molnar, linux-i2c, linux-gpio, LKML
In-Reply-To: <20160919081413.GR5016@twins.programming.kicks-ass.net>
On 2016-09-19 10:14, Peter Zijlstra wrote:
> On Mon, Sep 19, 2016 at 10:01:49AM +0200, Peter Rosin wrote:
>> Or, do what the i2c-mux code is doing and use an rt_mutex instead
>> of an ordinary mutex. That way you are very sure to not get any
>> lockdep splat ... at all. Ok, sorry, that was not a serious
>> suggestion, but it would be a tad bit simpler to implement...
>
> So I find it weird that people use rt_mutex as a locking primitive,
> since its only that one lock that then does PI and all the other locks
> that are related still create inversions.
So, someone took the bait :-)
Yes, I too find it weird, and would like to get rid of it. It's just
odd. It's been some years since the start though, waaay before me
entering kernel space.
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=194684e596af4b
But it's hard to argue with the numbers given in the discussion:
http://linux-i2c.vger.kernel.narkive.com/nokldJcc/patch-1-1-i2c-prevent-priority-inversion-on-top-of-bus-lock
Has anything happened to the regular mutex implementation that might
have changed the picture? *crosses fingers*
> In any case, since people have started doing this, adding lockdep
> support for rt_mutex is on the todo _somewhere_, so don't expect that to
> avoid splats forever.
I was actually looking quite hard to find out how I should declare the
lockdep class for the rt_mutex in order to prevent future splats, before
I realized that it wasn't even possible...
Cheers,
Peter
^ permalink raw reply
* Re: [REGRESSION? v4.8] i2c-core: acpi_i2c_get_info() touches non-existent devices
From: Mika Westerberg @ 2016-09-19 8:48 UTC (permalink / raw)
To: Nicolai Stange
Cc: Wolfram Sang, Octavian Purdila, Rafael J. Wysocki, linux-i2c,
linux-kernel
In-Reply-To: <87a8f4kfhe.fsf@gmail.com>
On Mon, Sep 19, 2016 at 12:30:53AM +0200, Nicolai Stange wrote:
> Hi,
>
> I'm encountering the following:
>
> [ 10.409490] ERROR: Unable to locate IOAPIC for GSI 37
>
> Note that the system works fine, so it's a "cosmetic" regression, I think.
>
>
> I added a dump_stack() right below the printk() in question and it reads
> as
>
> [ 10.410290] CPU: 6 PID: 710 Comm: systemd-udevd Not tainted 4.7.0-rc4+ #348
> [ 10.410962] Hardware name: Dell Inc. Latitude E6540/0725FP, BIOS A10 06/26/2014
> [ 10.411772] 0000000000000286 00000000b9050627 ffff8800c2e5f590 ffffffffa54161e7
> [ 10.412569] 0000000000000025 0000000000000001 ffff8800c2e5f5a0 ffffffffa50465df
> [ 10.413292] ffff8800c2e5f5d0 ffffffffa5046ffd 0000000000000000 0000000000000025
> [ 10.414016] Call Trace:
> [ 10.414713] [<ffffffffa54161e7>] dump_stack+0x68/0xa1
> [ 10.415406] [<ffffffffa50465df>] mp_find_ioapic+0x4f/0x60
> [ 10.416131] [<ffffffffa5046ffd>] mp_map_gsi_to_irq+0x1d/0xc0
> [ 10.416806] [<ffffffffa503dbbb>] acpi_register_gsi_ioapic+0x7b/0x170
> [ 10.417494] [<ffffffffa503da6f>] acpi_register_gsi+0xf/0x20
> [ 10.418217] [<ffffffffa54a14d5>] acpi_dev_get_irqresource.part.3+0xd7/0x11d
> [ 10.418871] [<ffffffffa54a139a>] ? acpi_dev_resource_address_space+0x31/0x67
> [ 10.419655] [<ffffffffa54a168d>] acpi_dev_resource_interrupt+0x9b/0xab
> [ 10.420408] [<ffffffffa54a1848>] acpi_dev_process_resource+0xbc/0xf7
> [ 10.421070] [<ffffffffa54a178c>] ? acpi_dev_resource_memory+0x7c/0x7c
> [ 10.421732] [<ffffffffa54c3ba2>] acpi_walk_resource_buffer+0x4d/0x85
> [ 10.422399] [<ffffffffa54a178c>] ? acpi_dev_resource_memory+0x7c/0x7c
> [ 10.423158] [<ffffffffa54c3e89>] acpi_walk_resources+0x83/0xb6
> [ 10.423831] [<ffffffffa54a15b1>] acpi_dev_get_resources+0x96/0xd7
> [ 10.424505] [<ffffffffa563f7c4>] acpi_i2c_get_info+0xe4/0x1a0
> [ 10.425181] [<ffffffffa5642c06>] acpi_i2c_add_device+0x56/0xa0
> [ 10.425856] [<ffffffffa54bf2ff>] acpi_ns_walk_namespace+0xe8/0x19d
> [ 10.426564] [<ffffffffa5642bb0>] ? acpi_i2c_register_device+0x70/0x70
> [ 10.427418] [<ffffffffa5642bb0>] ? acpi_i2c_register_device+0x70/0x70
> [ 10.428179] [<ffffffffa54bf83d>] acpi_walk_namespace+0xa0/0xd5
> [ 10.428858] [<ffffffffa56437a9>] i2c_register_adapter+0x369/0x500
> [ 10.429499] [<ffffffffa564399c>] i2c_add_adapter+0x5c/0x70
> [ 10.430125] [<ffffffffc07df7dd>] i801_probe+0x2bd/0x6a0 [i2c_i801]
> [ 10.431159] [<ffffffffa50eb8dd>] ? trace_hardirqs_on+0xd/0x10
> [ 10.432196] [<ffffffffa54682f2>] local_pci_probe+0x42/0xa0
> [ 10.432826] [<ffffffffa5468e2a>] ? pci_match_device+0xca/0x110
> [ 10.433460] [<ffffffffa5469243>] pci_device_probe+0x103/0x150
> [ 10.434083] [<ffffffffa5545acc>] driver_probe_device+0x22c/0x440
> [ 10.434712] [<ffffffffa5545db5>] __driver_attach+0xd5/0x100
> [ 10.435341] [<ffffffffa5545ce0>] ? driver_probe_device+0x440/0x440
> [ 10.435963] [<ffffffffa5543313>] bus_for_each_dev+0x73/0xc0
> [ 10.436676] [<ffffffffa554519e>] driver_attach+0x1e/0x20
> [ 10.437356] [<ffffffffa5544bc6>] bus_add_driver+0x1c6/0x290
> [ 10.437978] [<ffffffffc07e5000>] ? 0xffffffffc07e5000
> [ 10.438699] [<ffffffffa5546a50>] driver_register+0x60/0xe0
> [ 10.439502] [<ffffffffc07e5000>] ? 0xffffffffc07e5000
> [ 10.440310] [<ffffffffa5467e4d>] __pci_register_driver+0x5d/0x60
> [ 10.440313] [<ffffffffc07e50af>] i2c_i801_init+0xaf/0x1000 [i2c_i801]
> [ 10.440314] [<ffffffffc07e5000>] ? 0xffffffffc07e5000
> [ 10.440316] [<ffffffffa5000450>] do_one_initcall+0x50/0x180
> [ 10.440319] [<ffffffffa5101cc5>] ? rcu_read_lock_sched_held+0x45/0x80
> [ 10.440322] [<ffffffffa5234e55>] ? kmem_cache_alloc_trace+0x2d5/0x340
> [ 10.440325] [<ffffffffa51c1252>] do_init_module+0x5f/0x1da
> [ 10.440329] [<ffffffffa512d615>] load_module+0x2195/0x2950
> [ 10.440331] [<ffffffffa512a0e0>] ? __symbol_put+0x70/0x70
> [ 10.440334] [<ffffffffa525dd3b>] ? vfs_read+0x11b/0x130
> [ 10.440337] [<ffffffffa512e066>] SYSC_finit_module+0xe6/0x120
> [ 10.440339] [<ffffffffa512e0be>] SyS_finit_module+0xe/0x10
> [ 10.440340] [<ffffffffa5002fb1>] do_syscall_64+0x61/0x170
> [ 10.440343] [<ffffffffa581e1da>] entry_SYSCALL64_slow_path+0x25/0x25
>
>
> I bisected this to commit 525e6fabeae2 ("i2c / ACPI: add support for
> ACPI reconfigure notifications").
>
> The reason for the above message seems to be that acpi_i2c_get_info()
> configures the IRQs for any ACPI devices that have got some
> I2cSerialBus() resource, regardless of the actual adapter those are
> attached to. This behaviour is different from before that commit.
>
> My ACPI DSDT has got a PCI I2C adapter that isn't physically present, it
> seems. No clue why.
>
> That non-existent PCI I2C adapter is in turn I2cSerialBus()-referenced
> by some ACPI device that has got exactly this interrupt 37 assigned.
>
> So it looks like an attempt is made to configure this non-existent,
> ACPI-listed I2C slave's IRQs when an actually existing I2C adapter (i801
> SMBus) gets probed.
>
>
> Let me know if I can provide you with any additional information,
Can you send me acpidump from that that machine?
^ permalink raw reply
* Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning
From: Peter Zijlstra @ 2016-09-19 8:14 UTC (permalink / raw)
To: Peter Rosin
Cc: Bartosz Golaszewski, Wolfram Sang, Linus Walleij,
Alexandre Courbot, Andy Shevchenko, Vignesh R, Yong Li,
Geert Uytterhoeven, Ingo Molnar, linux-i2c, linux-gpio, LKML
In-Reply-To: <2f7d572c-59e0-f584-9748-48a1a45a1a50@axentia.se>
On Mon, Sep 19, 2016 at 10:01:49AM +0200, Peter Rosin wrote:
> Or, do what the i2c-mux code is doing and use an rt_mutex instead
> of an ordinary mutex. That way you are very sure to not get any
> lockdep splat ... at all. Ok, sorry, that was not a serious
> suggestion, but it would be a tad bit simpler to implement...
So I find it weird that people use rt_mutex as a locking primitive,
since its only that one lock that then does PI and all the other locks
that are related still create inversions.
In any case, since people have started doing this, adding lockdep
support for rt_mutex is on the todo _somewhere_, so don't expect that to
avoid splats forever.
^ permalink raw reply
* Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning
From: Peter Rosin @ 2016-09-19 8:01 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Wolfram Sang, Linus Walleij, Alexandre Courbot, Andy Shevchenko,
Vignesh R, Yong Li, Geert Uytterhoeven, Peter Zijlstra,
Ingo Molnar, linux-i2c, linux-gpio, LKML
In-Reply-To: <CAMpxmJX3_ec44Z0UVTNqSDjmNq+Oe3sk+szLE-Nj7X4pztbywQ@mail.gmail.com>
On 2016-09-18 21:45, Bartosz Golaszewski wrote:
> 2016-09-18 21:43 GMT+02:00 Bartosz Golaszewski <bgolaszewski@baylibre.com>:
>> 2016-09-18 10:52 GMT+02:00 Peter Rosin <peda@axentia.se>:
>>> On 2016-09-16 19:58, Wolfram Sang wrote:
>>>>
>>>> Same here. And if it prevents us from false positive lockdep reports, I
>>>> am all for fixing it.
>>>
>>> Except it doesn't, when I think some more about it...
>>>
>>> If you have two gpio-expanders on the same depth but on different i2c
>>> branches you still end up with a splat if one is used to control a mux
>>> to reach the other.
>>>
>>> The only way to solve it for good, that I see, is to have every instance
>>> of the gpio-expander mutex in its own class. That might lead to many
>>> lockdep classes but then again, how many gpio expanders could there be
>>> in a system? A dozen or two seems extreme, so maybe that is the correct
>>> approach anyway?
>>
>> Wouldn't it be enough to have a separate class for every base (as in:
>> not having any parent adapters) i2c adapter?
>>
>
> Eeek -ESENTTOOEARLY
>
> Of course not - since we could have two branches deeper on the tree
> with the same problem.
>
> Nevermind my last e-mail.
Right, but you have a point, we can avoid some lockdep classes if
we work at it.
We could recognize that no gpio-expander can be involved with
muxing for a gpio-expander sitting on a root i2c adapter. That
much is trivially obvious.
That means that we could treat all gpio-expanders sitting
directly on a root i2c-adapter as we always have, just like
v1 (and v2, implicitly) do, but instead of putting all other
gpio-exanders in one lockdep class (as in v1) or in one lockdep
class per depth (as in v2), we put all other gpio-mutexes in
a lockdep class of their own.
Then we do not get any extra lockdep classes unless there are
i2c muxes, which is better.
But only i2c-mux-gpio can cause the recursion.
I think the theoretical minimal number of lockdep classes is to
create a new lockdep class for every i2c-mux-gpio instance.
gpio-expanders needing a mutex could walk up the adapter tree
looking for a i2c-mux-gpio and getting the lockdep class from
there. If the root i2c adapter is reached, use some default
lockdep class.
That would consolidate muxes from different gpio-expander
drivers into the one lockdep class. I don't know if that is
a big no-no? I guess that can be handled too with more code,
but it is starting to get messy...
Or, do what the i2c-mux code is doing and use an rt_mutex instead
of an ordinary mutex. That way you are very sure to not get any
lockdep splat ... at all. Ok, sorry, that was not a serious
suggestion, but it would be a tad bit simpler to implement...
Cheers,
Peter
^ permalink raw reply
* Re: [PATCH] i2c-eg20t: use dynamically registered adapter number
From: Wolfram Sang @ 2016-09-19 6:44 UTC (permalink / raw)
To: Yadi; +Cc: Jean Delvare, linux-i2c
In-Reply-To: <57DF5AC6.4050504@windriver.com>
[-- Attachment #1: Type: text/plain, Size: 496 bytes --]
> For my case, i2c-kempld register dynamically itself by default,
> consequently it would occupied i2c bus 0 and cause to fails for adding new
> i2c bus with i2c_add_number_adpater(0).
This is exactly the PASEMI case as well. A video card got probed before
the pasemi driver and took all bus numbers it wants.
> i2c-kempld.c
...
> how do you like it?
Not much. If we change kempld, then another driver might come along next
time and we have the same issue again. I want a generic solution.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] i2c-eg20t: use dynamically registered adapter number
From: Yadi @ 2016-09-19 3:25 UTC (permalink / raw)
To: Wolfram Sang, Jean Delvare; +Cc: linux-i2c
In-Reply-To: <20160917214912.GA4395@katana>
On 2016年09月18日 05:49, Wolfram Sang wrote:
>>> The eg20t driver uses i2c_add_numbered_adapter() to register adapter:
>>>
>>> pch_adap->nr = i;
>>> ret = i2c_add_numbered_adapter(pch_adap);
>>>
>>> Variable i is assigned to 0, it means that i2c_eg20t is the first adapter
>>> by default. if another adapter registers before eg20t, above code return
>>> error for index conflict:
>>>
>>> i2c_eg20t 0000:05:0c.2: pch_i2c_probe :i2c_add_adapter[ch:0] FAILED
>>> i2c_eg20t: probe of 0000:05:0c.2 failed with error -16
>>>
>>> So, we can replace i2c_add_numbered_adapter() with i2c_add_adapter()
>>> interface.since it dynamically allocates the index number.
>> This does the exact opposite of:
>>
>> commit 07e8a51ff68353e01d795cceafbac9f54c49132b
>> Author: Feng Tang <feng.tang@intel.com>
>> Date: Thu Jan 12 15:38:02 2012 +0800
>>
>> i2c-eg20t: use i2c_add_numbered_adapter to get a fixed bus number
> We have the same problem with i2c-pasemi:
> http://www.spinics.net/lists/linux-i2c/msg25761.html
>
> PCI + i2c_add_numbered_adapter + another device adding i2c busses
>
> And we cannot simply change back to i2c_add_adapter() because it will
> regress on those systems which originally wanted to have
> i2c_add_numbered_adapter().
I think the is one totally different issue with i2c-pasemi, in which
PCI device prior to probed occupied bus number supposed to be used by
i2c-pasemi.
For my case, i2c-kempld register dynamically itself by default,
consequently it would occupied i2c bus 0 and cause to fails for adding
new i2c bus with i2c_add_number_adpater(0).
although i2c-kempld provide variable 'i2c_bus' to control bus
assignment, it isso inconvenient we have to manually modify it to avoid
possible conflict. so the reasonable solution looks like:
i2c-kempld.c
i2c->adap.nr = i2c_bus;
if (i2c_bus >= -1) /* using pci id to register if default=-1
for dynamic assignment*/
i2c->adap.n = pdev->id;
ret = i2c_add_numbered_adapter(&i2c->adap);
how do you like it?
Yadi
>
>> Looking at
>>
>> commit 03bde7c31a360f814ca42101d60563b1b803dca1
>> Author: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> Date: Thu Mar 12 17:17:59 2015 +0100
>>
>> i2c: busses with dynamic ids should start after fixed ids for DT
>>
>> it could be that you need to set some OF attribute to reserve i2c bus
>> numbers <= 1 for static usage. Assuming you use OF. Or is it automatic,
>> Wolfram?
> No, you need to define an alias in the devicetree. I don't think this
> platform is a DT user, though?
>
>> If not, it may make sense to add a helper function exposing
>> __i2c_first_dynamic_bus_num to drivers (something like
>> i2c_is_dynamic_bus_num().) After all, i2c_add_numbered_adapter() mostly
>> makes sense if static i2c device definitions exist. If not,
>> i2c_add_adapter() is just as good. So something like:
>>
>> if (i2c_is_dynamic_bus_num(i))
>> ret = i2c_add_adapter(pch_adap);
>> else {
>> pch_adap->nr = i;
>> ret = i2c_add_numbered_adapter(pch_adap);
>> }
>>
>> may make sense. Unless someone has a better idea.
> Interesting idea. Need to think about it some more. I didn't have a
> proper solution so far...
>
> Thanks,
>
> Wolfram
>
^ permalink raw reply
* Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning
From: Bartosz Golaszewski @ 2016-09-18 19:45 UTC (permalink / raw)
To: Peter Rosin
Cc: Wolfram Sang, Linus Walleij, Alexandre Courbot, Andy Shevchenko,
Vignesh R, Yong Li, Geert Uytterhoeven, Peter Zijlstra,
Ingo Molnar, linux-i2c, linux-gpio, LKML
In-Reply-To: <CAMpxmJUX8wszxhkoOyuqPeC8xgBjLXFRSWDSa0cOrJ92dUYX1g@mail.gmail.com>
2016-09-18 21:43 GMT+02:00 Bartosz Golaszewski <bgolaszewski@baylibre.com>:
> 2016-09-18 10:52 GMT+02:00 Peter Rosin <peda@axentia.se>:
>> On 2016-09-16 19:58, Wolfram Sang wrote:
>>>
>>> Same here. And if it prevents us from false positive lockdep reports, I
>>> am all for fixing it.
>>
>> Except it doesn't, when I think some more about it...
>>
>> If you have two gpio-expanders on the same depth but on different i2c
>> branches you still end up with a splat if one is used to control a mux
>> to reach the other.
>>
>> The only way to solve it for good, that I see, is to have every instance
>> of the gpio-expander mutex in its own class. That might lead to many
>> lockdep classes but then again, how many gpio expanders could there be
>> in a system? A dozen or two seems extreme, so maybe that is the correct
>> approach anyway?
>
> Wouldn't it be enough to have a separate class for every base (as in:
> not having any parent adapters) i2c adapter?
>
Eeek -ESENTTOOEARLY
Of course not - since we could have two branches deeper on the tree
with the same problem.
Nevermind my last e-mail.
Best regards,
Bartosz Golaszewski
^ permalink raw reply
* Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning
From: Bartosz Golaszewski @ 2016-09-18 19:43 UTC (permalink / raw)
To: Peter Rosin
Cc: Wolfram Sang, Linus Walleij, Alexandre Courbot, Andy Shevchenko,
Vignesh R, Yong Li, Geert Uytterhoeven, Peter Zijlstra,
Ingo Molnar, linux-i2c, linux-gpio, LKML
In-Reply-To: <587cdb60-d7fe-3ab2-b635-02c5072e102e@axentia.se>
2016-09-18 10:52 GMT+02:00 Peter Rosin <peda@axentia.se>:
> On 2016-09-16 19:58, Wolfram Sang wrote:
>>
>> Same here. And if it prevents us from false positive lockdep reports, I
>> am all for fixing it.
>
> Except it doesn't, when I think some more about it...
>
> If you have two gpio-expanders on the same depth but on different i2c
> branches you still end up with a splat if one is used to control a mux
> to reach the other.
>
> The only way to solve it for good, that I see, is to have every instance
> of the gpio-expander mutex in its own class. That might lead to many
> lockdep classes but then again, how many gpio expanders could there be
> in a system? A dozen or two seems extreme, so maybe that is the correct
> approach anyway?
Wouldn't it be enough to have a separate class for every base (as in:
not having any parent adapters) i2c adapter?
Best regards,
Bartosz Golaszewski
^ permalink raw reply
* Re: [PATCH] i2c-eg20t: use dynamically registered adapter number
From: Wolfram Sang @ 2016-09-18 19:22 UTC (permalink / raw)
To: Jean Delvare; +Cc: Yadi Hu, linux-i2c
In-Reply-To: <20160826173019.1f939ce5@endymion>
[-- Attachment #1: Type: text/plain, Size: 1592 bytes --]
> If not, it may make sense to add a helper function exposing
> __i2c_first_dynamic_bus_num to drivers (something like
> i2c_is_dynamic_bus_num().) After all, i2c_add_numbered_adapter() mostly
> makes sense if static i2c device definitions exist. If not,
> i2c_add_adapter() is just as good. So something like:
>
> if (i2c_is_dynamic_bus_num(i))
> ret = i2c_add_adapter(pch_adap);
> else {
> pch_adap->nr = i;
> ret = i2c_add_numbered_adapter(pch_adap);
> }
>
> may make sense. Unless someone has a better idea.
PASEMI does:
smbus->adapter.nr = PCI_FUNC(dev->devfn);
I am unsure if there is any guarantee in what order PCI_FUNCs are
probed, yet I have the feeling we could try a little harder to get the
numbered adapter. What about this (untested, just to get the idea)?
static inline int i2c_add_adapter_try_numbered(struct i2c_adapter *new_adap)
{
int ret;
struct i2c_adapter *old_adap = i2c_get_adapter(new_adap->nr);
if (old_adap && new_adap->nr >= __i2c_first_dynamic_bus_num) {
i2c_put_adapter(old_adap);
dev_dbg(&new_adap->dev, "Static bus number occupied, trying dynamic number\n");
ret = i2c_add_adapter(new_adap);
} else {
ret = i2c_add_numbered_adapter(new_adap);
}
return ret;
}
I used 'static inline' because I think the drivers needing this should
carry the extra weight. But no major objection to put sth like this also
into the core. The documentation for this function should carry a big
note that this is only a workaround and it should not be used directly.
Thoughts?
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH] i2c-eg20t: fix race between i2c init and interrupt enable
From: Yadi Hu @ 2016-09-18 10:52 UTC (permalink / raw)
To: yadi.hu, wsa, jdelvare; +Cc: linux-i2c
In-Reply-To: <1474195951-7238-1-git-send-email-yadi.hu@windriver.com>
From: "Yadi.hu" <yadi.hu@windriver.com>
the eg20t driver call request_irq() function before the pch_base_address,
base address of i2c controller's register, is assigned an effective value.
there is one possible scenario that an interrupt which isn't inside eg20t
arrives immediately after request_irq() is executed when i2c controller
shares an interrupt number with others. since the interrupt handler
pch_i2c_handler() has already active as shared action, it will be called
and read its own register to determine if this interrupt is from itself.
At that moment, since base address of i2c registers is not remapped
in kernel space yet,so the INT handler will access an illegal address
and then a error occurs.
Signed-off-by: Yadi.hu <yadi.hu@windriver.com>
---
drivers/i2c/busses/i2c-eg20t.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c
index 137125b..5ce71ce 100644
--- a/drivers/i2c/busses/i2c-eg20t.c
+++ b/drivers/i2c/busses/i2c-eg20t.c
@@ -773,13 +773,6 @@ static int pch_i2c_probe(struct pci_dev *pdev,
/* Set the number of I2C channel instance */
adap_info->ch_num = id->driver_data;
- ret = request_irq(pdev->irq, pch_i2c_handler, IRQF_SHARED,
- KBUILD_MODNAME, adap_info);
- if (ret) {
- pch_pci_err(pdev, "request_irq FAILED\n");
- goto err_request_irq;
- }
-
for (i = 0; i < adap_info->ch_num; i++) {
pch_adap = &adap_info->pch_data[i].pch_adapter;
adap_info->pch_i2c_suspended = false;
@@ -797,6 +790,17 @@ static int pch_i2c_probe(struct pci_dev *pdev,
pch_adap->dev.of_node = pdev->dev.of_node;
pch_adap->dev.parent = &pdev->dev;
+ }
+
+ ret = request_irq(pdev->irq, pch_i2c_handler, IRQF_SHARED,
+ KBUILD_MODNAME, adap_info);
+ if (ret) {
+ pch_pci_err(pdev, "request_irq FAILED\n");
+ goto err_request_irq;
+ }
+
+ for (i = 0; i < adap_info->ch_num; i++) {
+ pch_adap = &adap_info->pch_data[i].pch_adapter;
pch_i2c_init(&adap_info->pch_data[i]);
--
2.9.3
^ permalink raw reply related
* RESEND: V3 i2c-eg20t:fix race between i2c init and interrupt enable
From: Yadi Hu @ 2016-09-18 10:52 UTC (permalink / raw)
To: yadi.hu, wsa, jdelvare; +Cc: linux-i2c
the eg20t driver call request_irq() function before the pch_base_address,
base address of i2c controller's register, isassigned an effective value.
there is one possible scenario that an interrupt which isn't inside eg20t
arrives immediately after request_irq() is executed when i2c controller
shares an interrupt number with others. since the interrupt handler
pch_i2c_handler() has already active as shared action, it will be called
and read its own register to determine if this interrupt is from itself.
At that moment, since base address of i2c registers is not remapped
in kernel space yet,so the INT handler will access an illegal address
and then a error occurs.
Bad IO access at port 0x18 (return inl(port))
Call Trace:
[<c102c733>] warn_slowpath_common+0x73/0xa0
[<c13109f5>] ? bad_io_access+0x45/0x50
[<c13109f5>] ? bad_io_access+0x45/0x50
[<c102c804>] warn_slowpath_fmt+0x34/0x40
[<c13109f5>] bad_io_access+0x45/0x50
[<c1310b62>] ioread32+0x22/0x40
[<c14c60db>] pch_i2c_handler+0x3b/0x120
[<c1092f34>] handle_irq_event_percpu+0x64/0x330
[<c109323b>] handle_irq_event+0x3b/0x60
[<c1095b50>] ? unmask_irq+0x30/0x30
[<c1095ba0>] handle_fasteoi_irq+0x50/0xe0
<IRQ> [<c16e7e78>] ? do_IRQ+0x48/0xc0
[<c16e7f6f>] ? smp_apic_timer_interrupt+0x7f/0x17a
[<c16e7da9>] ? common_interrupt+0x29/0x30
[<c1008ebb>] ? mwait_idle+0x8b/0x2c0
[<c1009e8e>] ? cpu_idle+0x9e/0xc0
[<c16be408>] ? rest_init+0x6c/0x74
[<c19f770a>] ? start_kernel+0x311/0x318
[<c19f7231>] ? repair_env_string+0x51/0x51
[<c19f7078>] ? i386_start_kernel+0x78/0x7d
---[ end trace eb3a1028f468a140 ]---
i2c_eg20t 0000:05:0c.2: pch_i2c_handler :I2C-3 mode(0) is not supported
Yadi Hu (1)
i2c-eg20t: fix race between i2c init and interrupt enable
drivers/i2c/busses/i2c-eg20t.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
^ permalink raw reply
* Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning
From: Peter Rosin @ 2016-09-18 8:52 UTC (permalink / raw)
To: Wolfram Sang
Cc: Bartosz Golaszewski, Linus Walleij, Alexandre Courbot,
Andy Shevchenko, Vignesh R, Yong Li, Geert Uytterhoeven,
Peter Zijlstra, Ingo Molnar, linux-i2c, linux-gpio, LKML
In-Reply-To: <20160916175842.GD1426@katana>
On 2016-09-16 19:58, Wolfram Sang wrote:
>
>>> Looks good from my POV, but will wait for Peter to comment.
>>>
>>> If accepted, I'd think this should go via my I2C tree and I would like
>>> to ask Linus to ack patch 4. D'accord, everyone?
>>
>> Since it is not clear if "Peter" is me or PeterZ (I suspect PeterZ...),
>
> Nope, I meant you :) I really value your input, it especially helps me
> on topics like locking, nesting, muxing... etc. Much appreciated, thanks
> a lot for doing that!
>
>> I'm just adding that it all looks fine by me as well, just to prevent
>> this from being held up by a misunderstanding.
>
> OK. I read this as Acked-by.
>
>> It does unconditionally add a new function to i2c-core that is only
>> ever used if lockdep is enabled, but it is tiny and I'm not bothered
>> by that memory waste.
>
> Same here. And if it prevents us from false positive lockdep reports, I
> am all for fixing it.
Except it doesn't, when I think some more about it...
If you have two gpio-expanders on the same depth but on different i2c
branches you still end up with a splat if one is used to control a mux
to reach the other.
The only way to solve it for good, that I see, is to have every instance
of the gpio-expander mutex in its own class. That might lead to many
lockdep classes but then again, how many gpio expanders could there be
in a system? A dozen or two seems extreme, so maybe that is the correct
approach anyway?
Cheers,
Peter
^ permalink raw reply
* Re: RESEND:i2c-eg20t: fix race between i2c init and interrupt enable
From: Yadi @ 2016-09-18 8:51 UTC (permalink / raw)
To: Wolfram Sang; +Cc: jdelvare, linux-i2c
In-Reply-To: <20160918075326.GA1412@katana>
On 2016年09月18日 15:53, Wolfram Sang wrote:
>> it seems to me so uncomfortable. how do you like it?
>>
>> for (all_channels)
>> do_initialization
>>
>> request_irq()
>>
>> for (all_channels) {
>> pch_i2c_init /* Enable interrupts at end of function*/
>> register_adapter
>> }
> Thanks for the correction. This would be still my preferred solution.
>
ok, I have no objection and will resend patch V3 per your solution.
Yadi
^ permalink raw reply
* Re: RESEND:i2c-eg20t: fix race between i2c init and interrupt enable
From: Wolfram Sang @ 2016-09-18 7:53 UTC (permalink / raw)
To: Yadi; +Cc: jdelvare, linux-i2c
In-Reply-To: <738c387b-2a7e-19fb-d9d9-034426ce3b49@windriver.com>
[-- Attachment #1: Type: text/plain, Size: 318 bytes --]
> it seems to me so uncomfortable. how do you like it?
>
> for (all_channels)
> do_initialization
>
> request_irq()
>
> for (all_channels) {
> pch_i2c_init /* Enable interrupts at end of function*/
> register_adapter
> }
Thanks for the correction. This would be still my preferred solution.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: RESEND:i2c-eg20t: fix race between i2c init and interrupt enable
From: Yadi @ 2016-09-18 2:55 UTC (permalink / raw)
To: Wolfram Sang; +Cc: jdelvare, linux-i2c
In-Reply-To: <20160916200456.GA2216@katana>
在 2016/9/17 4:04, Wolfram Sang 写道:
>> after testing last patch, replacing request_irq() location is not a
>> good idea,it might produce a little time windows ,in which the interrupts
>> occurring inside will be omitted.
>>
>> the new patch adds a check point on interrupt handler in case field
>> 'pch_base_address' has not been initialed.
> What about using two for-loops in probe like this pseudo code?
>
> for (all_channels)
> do_initialization
>
> request_irq()
>
> for (all_channels)
> register_adapter
>
>
> This seems to me the correct order and most readable solution.
if we use two for-loops, we would have to put pch_i2c_init on the
second loop in order to avoid littel interval, in which the interrupts
occuring inside will be omitted.
it seems to me so uncomfortable. how do you like it?
for (all_channels)
do_initialization
request_irq()
for (all_channels) {
pch_i2c_init /* Enable interrupts at end of function*/
register_adapter
}
Yadi
^ permalink raw reply
* Re: [PATCH] i2c-eg20t: use dynamically registered adapter number
From: Wolfram Sang @ 2016-09-17 21:49 UTC (permalink / raw)
To: Jean Delvare; +Cc: Yadi Hu, linux-i2c
In-Reply-To: <20160826173019.1f939ce5@endymion>
[-- Attachment #1: Type: text/plain, Size: 2412 bytes --]
> > The eg20t driver uses i2c_add_numbered_adapter() to register adapter:
> >
> > pch_adap->nr = i;
> > ret = i2c_add_numbered_adapter(pch_adap);
> >
> > Variable i is assigned to 0, it means that i2c_eg20t is the first adapter
> > by default. if another adapter registers before eg20t, above code return
> > error for index conflict:
> >
> > i2c_eg20t 0000:05:0c.2: pch_i2c_probe :i2c_add_adapter[ch:0] FAILED
> > i2c_eg20t: probe of 0000:05:0c.2 failed with error -16
> >
> > So, we can replace i2c_add_numbered_adapter() with i2c_add_adapter()
> > interface.since it dynamically allocates the index number.
>
> This does the exact opposite of:
>
> commit 07e8a51ff68353e01d795cceafbac9f54c49132b
> Author: Feng Tang <feng.tang@intel.com>
> Date: Thu Jan 12 15:38:02 2012 +0800
>
> i2c-eg20t: use i2c_add_numbered_adapter to get a fixed bus number
We have the same problem with i2c-pasemi:
http://www.spinics.net/lists/linux-i2c/msg25761.html
PCI + i2c_add_numbered_adapter + another device adding i2c busses
And we cannot simply change back to i2c_add_adapter() because it will
regress on those systems which originally wanted to have
i2c_add_numbered_adapter().
> Looking at
>
> commit 03bde7c31a360f814ca42101d60563b1b803dca1
> Author: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Date: Thu Mar 12 17:17:59 2015 +0100
>
> i2c: busses with dynamic ids should start after fixed ids for DT
>
> it could be that you need to set some OF attribute to reserve i2c bus
> numbers <= 1 for static usage. Assuming you use OF. Or is it automatic,
> Wolfram?
No, you need to define an alias in the devicetree. I don't think this
platform is a DT user, though?
> If not, it may make sense to add a helper function exposing
> __i2c_first_dynamic_bus_num to drivers (something like
> i2c_is_dynamic_bus_num().) After all, i2c_add_numbered_adapter() mostly
> makes sense if static i2c device definitions exist. If not,
> i2c_add_adapter() is just as good. So something like:
>
> if (i2c_is_dynamic_bus_num(i))
> ret = i2c_add_adapter(pch_adap);
> else {
> pch_adap->nr = i;
> ret = i2c_add_numbered_adapter(pch_adap);
> }
>
> may make sense. Unless someone has a better idea.
Interesting idea. Need to think about it some more. I didn't have a
proper solution so far...
Thanks,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox