* Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning
From: Bartosz Golaszewski @ 2016-09-20 12:32 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Peter Rosin, Peter Zijlstra, Wolfram Sang, Linus Walleij,
Alexandre Courbot, Andy Shevchenko, Vignesh R, Yong Li,
Geert Uytterhoeven, Ingo Molnar, linux-i2c, linux-gpio, LKML
In-Reply-To: <CAMuHMdX6otYLuYm9x4BNm-U3gHn5Y_JwBUm5qagWLOU0ucRmcQ@mail.gmail.com>
2016-09-20 13:30 GMT+02:00 Geert Uytterhoeven <geert@linux-m68k.org>:
> On Tue, Sep 20, 2016 at 12:48 PM, Peter Rosin <peda@axentia.se> wrote:
>> On 2016-09-20 12:07, Bartosz Golaszewski wrote:
>>> I feel like it's just wrong to set an arbitrary limit on the number of
>>> i2c branches - and this is what the result of this approach would be.
>>
>> What arbitrary limit would that be? The number of lockdep classes
>> can't be *that* limited? Or?
>>
>> I mean one lockdep class per root adapter and one subclass within that
>> class per mux level doesn't sound too bad. How many root adapters do
>> we need to design for?
>
> 'git grep -c i2c@ -- "*dts*"' told me exynos7 has 12 i2c interfaces.
> And as long as I have gpios (pcf8574?), I can add more using i2c-gpio.
> Hence the upper limit is infinity.
>
Agreed. If you add to that dynamic i2c adapters like hid-cp2112 or
greybus-i2c then it makes infinity plus some more.
Best regards,
Bartosz Golaszewski
^ permalink raw reply
* Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning
From: Geert Uytterhoeven @ 2016-09-20 11:30 UTC (permalink / raw)
To: Peter Rosin
Cc: Bartosz Golaszewski, Peter Zijlstra, Wolfram Sang, Linus Walleij,
Alexandre Courbot, Andy Shevchenko, Vignesh R, Yong Li,
Geert Uytterhoeven, Ingo Molnar, linux-i2c, linux-gpio, LKML
In-Reply-To: <6e174c17-584f-580d-083a-94dab2f588bd@axentia.se>
On Tue, Sep 20, 2016 at 12:48 PM, Peter Rosin <peda@axentia.se> wrote:
> On 2016-09-20 12:07, Bartosz Golaszewski wrote:
>> I feel like it's just wrong to set an arbitrary limit on the number of
>> i2c branches - and this is what the result of this approach would be.
>
> What arbitrary limit would that be? The number of lockdep classes
> can't be *that* limited? Or?
>
> I mean one lockdep class per root adapter and one subclass within that
> class per mux level doesn't sound too bad. How many root adapters do
> we need to design for?
'git grep -c i2c@ -- "*dts*"' told me exynos7 has 12 i2c interfaces.
And as long as I have gpios (pcf8574?), I can add more using i2c-gpio.
Hence the upper limit is infinity.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer
From: kernel @ 2016-09-20 11:29 UTC (permalink / raw)
To: Noralf Trønnes
Cc: wsa, Stephen Warren, Eric Anholt, linux-rpi-kernel, linux-kernel,
linux-arm-kernel, linux-i2c
In-Reply-To: <8053bf5c-7ddc-b4f7-1961-93f161731b16@tronnes.org>
> On 20.09.2016, at 12:56, Noralf Trønnes <noralf@tronnes.org> wrote:
>
>
> Den 20.09.2016 12:15, skrev Martin Sperl:
>>
>>
>> On 20.09.2016 10:41, Noralf Trønnes wrote:
>>>
>>> Den 20.09.2016 09:19, skrev Martin Sperl:
>>>> Hi Noralf!
>>>>
>>>> On 19.09.2016 17:26, Noralf Trønnes wrote:
>>>>> 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(-)
>>>> ...
>>>>> @@ -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;
>>>>> }
>>>> This does not seem to implement the i2c_msg api correctly.
>>>>
>>>> As per comments in include/uapi/linux/i2c.h on line 58 only the last
>>>> message
>>>> in a group should - by default - send a STOP.
>>>>
>>>
>>> Apparently it's a known problem that the i2c controller doesn't support
>>> Repeated Start. It will always issue a Stop when it has transferred DLEN
>>> bytes.
>>> Refs:
>>> http://www.circuitwizard.de/raspi-i2c-fix/raspi-i2c-fix.html
>>> http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they
>>>
>>>
>>> UNLESS: a Start Transfer (ST) is issued after Transfer Active (TA) is set
>>> and before DONE is set (or the last byte is shifted, I don't know excatly).
>>> Refs:
>>> https://github.com/raspberrypi/linux/issues/254#issuecomment-15254134
>>> https://www.raspberrypi.org/forums/viewtopic.php?p=807834&sid=2b612c7209f2175bf1a266359c72ae6c#p807834
>>>
>>>
>>> I found this answer/report by joan that the downstream combined support
>>> isn't reliable:
>>> http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they
>>>
>>>
>>> My implementation differs from downstream in that I use local_irq_save()
>>> to protect the polling loop. But that only protects from missing the TA
>>> (downstream can miss the TA and issue a Stop).
>>>
>>> So currently in mainline we have a driver that says it support the standard
>>> (I2C_FUNC_I2C), but it really only supports one message transfers since it
>>> can't do ReStart.
>>>
>>> What I have done in this patch is to support ReStart for transfers with
>>> 2 messages: first write, then read. But maybe a better solution is to just
>>> leave this alone if it is flaky and use bitbanging instead. I don't know.
>> I have not said that the approach you have taken is wrong or bad.
>>
>
> I didn't take it as such, I'm just not sure what's the best approach here,
> so I added and looked up some more information
>
>> I was only telling you that the portion inside the bcm2835_i2c_xfer:
>> + /* 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;
>> + }
>> is very specific and maybe could be done in a "generic" manner
>> supporting more cases.
>>
>
> It has to be specific when it comes to number of messages. We can only
> support ReStart after the first message unless we use polling for the
> whole transfer. And in that case we can't disable interrupts for such
> a long period and we will end up sometimes loosing Transfer Active,
> resulting in Stop Condition between the messages.
> So we can only do transfers with 2 messages if we want Restart.
>
> It is possible to support more than 16 bytes for the first message,
> filling the FIFO after polling TA, but I'm not sure that is common.
> Mostly it's 1 or 2 bytes to set a register.
> The write-read restriction isn't absolutely necessary either, but it's the
> most common case I think. So it was about reusing bcm2835_i2c_xfer_msg().
> A less restrictive approach would require a dedicated function I think.
>
>> At least add a dev_warn_once for all num > 1 cases not handled by the
>> code above.
>>
>> This gives people an opportunity to detect such a situation if they
>> find something is not working as expected.
>>
>
> I agree.
>
> After reading joan's report I wonder if it would be best to add a module
> parameter like downstream has, so it can be disabled. What do you think?
>
I guess let us start simple:
* get warning in place about always issuing a stop for num > 1
- instead we may just want to set max_num_msgs = 1 in quirks.
* apply your patch for the write (<=16) then read case.
- maybe by setting quirks I2C_AQ_COMB_WRITE_THEN_READ
plus max_comb_1st_msg_len = 16 and max_num_msgs = 2
If this becomes too restrictive for some specific HW, then someone
may want to add the missing features.
As for the module parameters: no idea if this is acceptable
or sensible.
But that’s just my 2c...
Martin
^ permalink raw reply
* Re: [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer
From: Noralf Trønnes @ 2016-09-20 10:56 UTC (permalink / raw)
To: Martin Sperl, wsa, swarren, eric
Cc: linux-rpi-kernel, linux-kernel, linux-arm-kernel, linux-i2c
In-Reply-To: <4990930f-6d69-89c6-4b23-deae3d6713ed@martin.sperl.org>
Den 20.09.2016 12:15, skrev Martin Sperl:
>
>
> On 20.09.2016 10:41, Noralf Trønnes wrote:
>>
>> Den 20.09.2016 09:19, skrev Martin Sperl:
>>> Hi Noralf!
>>>
>>> On 19.09.2016 17:26, Noralf Trønnes wrote:
>>>> 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(-)
>>> ...
>>>> @@ -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;
>>>> }
>>> This does not seem to implement the i2c_msg api correctly.
>>>
>>> As per comments in include/uapi/linux/i2c.h on line 58 only the last
>>> message
>>> in a group should - by default - send a STOP.
>>>
>>
>> Apparently it's a known problem that the i2c controller doesn't support
>> Repeated Start. It will always issue a Stop when it has transferred DLEN
>> bytes.
>> Refs:
>> http://www.circuitwizard.de/raspi-i2c-fix/raspi-i2c-fix.html
>> http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they
>>
>>
>>
>> UNLESS: a Start Transfer (ST) is issued after Transfer Active (TA) is
>> set
>> and before DONE is set (or the last byte is shifted, I don't know
>> excatly).
>> Refs:
>> https://github.com/raspberrypi/linux/issues/254#issuecomment-15254134
>> https://www.raspberrypi.org/forums/viewtopic.php?p=807834&sid=2b612c7209f2175bf1a266359c72ae6c#p807834
>>
>>
>>
>> I found this answer/report by joan that the downstream combined support
>> isn't reliable:
>> http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they
>>
>>
>>
>> My implementation differs from downstream in that I use local_irq_save()
>> to protect the polling loop. But that only protects from missing the TA
>> (downstream can miss the TA and issue a Stop).
>>
>> So currently in mainline we have a driver that says it support the
>> standard
>> (I2C_FUNC_I2C), but it really only supports one message transfers
>> since it
>> can't do ReStart.
>>
>> What I have done in this patch is to support ReStart for transfers with
>> 2 messages: first write, then read. But maybe a better solution is to
>> just
>> leave this alone if it is flaky and use bitbanging instead. I don't
>> know.
> I have not said that the approach you have taken is wrong or bad.
>
I didn't take it as such, I'm just not sure what's the best approach here,
so I added and looked up some more information
> I was only telling you that the portion inside the bcm2835_i2c_xfer:
> + /* 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;
> + }
> is very specific and maybe could be done in a "generic" manner
> supporting more cases.
>
It has to be specific when it comes to number of messages. We can only
support ReStart after the first message unless we use polling for the
whole transfer. And in that case we can't disable interrupts for such
a long period and we will end up sometimes loosing Transfer Active,
resulting in Stop Condition between the messages.
So we can only do transfers with 2 messages if we want Restart.
It is possible to support more than 16 bytes for the first message,
filling the FIFO after polling TA, but I'm not sure that is common.
Mostly it's 1 or 2 bytes to set a register.
The write-read restriction isn't absolutely necessary either, but it's the
most common case I think. So it was about reusing bcm2835_i2c_xfer_msg().
A less restrictive approach would require a dedicated function I think.
> At least add a dev_warn_once for all num > 1 cases not handled by the
> code above.
>
> This gives people an opportunity to detect such a situation if they
> find something is not working as expected.
>
I agree.
After reading joan's report I wonder if it would be best to add a module
parameter like downstream has, so it can be disabled. What do you think?
Noralf.
^ permalink raw reply
* Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning
From: Peter Rosin @ 2016-09-20 10:48 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Peter Zijlstra, Wolfram Sang, Linus Walleij, Alexandre Courbot,
Andy Shevchenko, Vignesh R, Yong Li, Geert Uytterhoeven,
Ingo Molnar, linux-i2c, linux-gpio, LKML
In-Reply-To: <CAMpxmJVNbsZewcui_WL0aZi3e4Q52=qHxGkm=w5EAr49YAqyaw@mail.gmail.com>
On 2016-09-20 12:07, Bartosz Golaszewski wrote:
> 2016-09-20 10:48 GMT+02:00 Peter Rosin <peda@axentia.se>:
>>
>> One pretty simple problematic case is:
>>
>> .---. .----.
>> | | | |-- i2c2
>> | |-- i2c0 --|mux0| .----.
>> | l | | |-- i2c3 --|gpio|
>> | i | '----' '----'
>> | n | .--------------'
>> | u | .----. .----.
>> | x | | |-- i2c4 --|dev0|
>> | |-- i2c1 --|mux1| '----'
>> | | | |-- i2c5
>> '---' '----'
>>
>> Accesses to dev0 will:
>>
>> 1. lock i2c1:mux_lock (depth 0)
>> 2. switch mux1 to i2c4 using gpio
>> a lock i2c0:mux_lock (depth 0)
>> b switch mux0 to i2c3 using whatever
>> c access gpio
>> d unlock i2c0:mux_lock
>> 3. access dev0
>> 4. unlock i2c1:mux_lock
>>
>> 2a will cause a lockdep splat if i2c0:mux_lock is in the same
>> lockdep class & subclass as i2c1:mux_lock. So, lockdep needs
>> separate lockdep classes depending on the i2c root adapter
>> (subclasses are needed to handle deeper trees, so they are off
>> limits). Great fun. How do I go about creating a new lockdep
>> class for every i2c root adapter instance?
>>
>
> I feel like it's just wrong to set an arbitrary limit on the number of
> i2c branches - and this is what the result of this approach would be.
What arbitrary limit would that be? The number of lockdep classes
can't be *that* limited? Or?
I mean one lockdep class per root adapter and one subclass within that
class per mux level doesn't sound too bad. How many root adapters do
we need to design for?
> One solution that comes to mind is to have a separate, global set of
> lock classes solely for gpio expanders. I think you mentioned earlier
> that it's the only thing that can cause this kind of lockdep false
> positives.
No, that's not true, so if I said that, it was simply wrong. E.g. I
think the new mlxcpld i2c mux can mux another i2c adapter and will
cause the same problem without involving any gpio expander.
[OT: Wolfram, are you still following this? Are you picking up the
mlxcpld mux driver, or did my ack fly by without you noticing?]
Oh, and the there is a pinctrl-based i2c mux that also suffers as
there are pinctrl chips that are i2c clients.
And even though I don't really know the pinctrl demux, it will
probably also cause interesting nesting if an i2c based pinctrl
is used as demuxer. If that's relevant, probably isn't...
> We could potentially have a limited set of lock classes and
> every expander that would need one would request it using some kind of
> API ensuring that every instance gets a separate class. But this
> sounds like a big hack too I'm afraid... And regmap would need to be
> aware of that as well.
>
> Anyways, we're past rc7 already and 4.9 will be the next LTS kernel.
> We have real hardware here that runs on mainline linux and is
> suffering from this issue. Are there any objections against merging
> this series now and continuing the work on improving the solution for
> 4.10?
No, I have no objection. It's not wrong per se, it's just not complete.
Cheers,
Peter
^ permalink raw reply
* Re: [PATCH] i2c / ACPI: Do not touch an I2C device if it belongs to another adapter
From: Mika Westerberg @ 2016-09-20 10:45 UTC (permalink / raw)
To: Nicolai Stange
Cc: Wolfram Sang, Octavian Purdila, Rafael J . Wysocki, Jarkko Nikula,
linux-i2c, linux-kernel
In-Reply-To: <874m5ax3o4.fsf@gmail.com>
On Tue, Sep 20, 2016 at 12:32:11PM +0200, Nicolai Stange wrote:
> Your patch fixes my issue, so feel free to add a
>
> Tested-by: Nicolai Stange <nicstange@gmail.com>
Thanks!
> for this either.
>
> But please see my remark below.
>
>
> Mika Westerberg <mika.westerberg@linux.intel.com> writes:
>
> > When enumerating I2C devices connected to an I2C adapter we scan the whole
> > namespace (as it is possible to have devices anywhere in that namespace,
> > not just below the I2C adapter device) and add each found device to the I2C
> > bus in question.
> >
> > Now after commit 525e6fabeae2 ("i2c / ACPI: add support for ACPI
> > reconfigure notifications") checking of the adapter handle to the one found
> > in the I2cSerialBus() resource was moved to happen after resources of the
> > I2C device has been parsed. This means that if the I2cSerialBus() resource
> > points to an adapter that does not exists in the system we still parse
> > those resources. This is problematic in particular because
> > acpi_dev_resource_interrupt() tries to configure GSI if the device also has
> > an Interrupt() resource. Failing to do that results errrors like this to be
> > printed on the console:
> >
> > [ 10.409490] ERROR: Unable to locate IOAPIC for GSI 37
> >
> > To fix this we pass the I2C adapter to i2c_acpi_get_info() and make sure
> > the handle matches the one in the I2cSerialBus() resource before doing
> > anything else to the device.
> >
> > Reported-by: Nicolai Stange <nicstange@gmail.com>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> > drivers/i2c/i2c-core.c | 12 +++++++-----
> > 1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index c61c961cf8f9..eb32cb783fc8 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -168,6 +168,7 @@ static int i2c_acpi_do_lookup(struct acpi_device *adev,
> >
> > static int i2c_acpi_get_info(struct acpi_device *adev,
> > struct i2c_board_info *info,
> > + struct i2c_adapter *adapter,
> > acpi_handle *adapter_handle)
> > {
> > struct list_head resource_list;
> > @@ -182,6 +183,10 @@ static int i2c_acpi_get_info(struct acpi_device *adev,
> > if (ret)
> > return ret;
> >
> > + /* The adapter must match the one in I2cSerialBus() connector */
> > + if (adapter && ACPI_HANDLE(&adapter->dev) != lookup.adapter_handle)
> > + return -ENODEV;
> > +
>
> Would it be sensible to add the adapter presence check you provided
> earlier, i.e.
>
> + else if (!adapter) {
> + /* The adapter must be present */
> + if (acpi_bus_get_device(lookup.adapter_handle, &adapter_adev))
> + return -ENODEV;
> + if (acpi_bus_get_status(adapter_adev) || !adapter_adev->status.present)
> + return -ENODEV;
> +
> + }
>
>
> here, because we can't know if ...
>
>
> > info->fwnode = acpi_fwnode_handle(adev);
> > *adapter_handle = lookup.adapter_handle;
> >
> > @@ -231,10 +236,7 @@ static acpi_status i2c_acpi_add_device(acpi_handle handle, u32 level,
> > if (acpi_bus_get_device(handle, &adev))
> > return AE_OK;
> >
> > - if (i2c_acpi_get_info(adev, &info, &adapter_handle))
> > - return AE_OK;
> > -
> > - if (adapter_handle != ACPI_HANDLE(&adapter->dev))
> > + if (i2c_acpi_get_info(adev, &info, adapter, &adapter_handle))
> > return AE_OK;
> >
> > i2c_acpi_register_device(adapter, adev, &info);
> > @@ -368,7 +370,7 @@ static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
> >
> > switch (value) {
> > case ACPI_RECONFIG_DEVICE_ADD:
> > - if (i2c_acpi_get_info(adev, &info, &adapter_handle))
> > + if (i2c_acpi_get_info(adev, &info, NULL, &adapter_handle))
> > break;
>
> ... the ACPI device added here is physically existent?
Good point.
>
> >
> > adapter = i2c_acpi_find_adapter_by_handle(adapter_handle);
>
> I suppose that it is always true that adev has been LoadTable()'d from
> some SSDT? Can't this SSDT be just as broken as my DSDT is? Not that
> I've seen such a case in the real world, I'm just asking.
Yes it can be broken and since the adapter reference is just a string in
I2cSerialBus() resource we definitely need to check that it actually
exists.
I'll submit v2 soon.
^ permalink raw reply
* Re: [PATCH] i2c / ACPI: Do not touch an I2C device if it belongs to another adapter
From: Nicolai Stange @ 2016-09-20 10:32 UTC (permalink / raw)
To: Mika Westerberg
Cc: Wolfram Sang, Nicolai Stange, Octavian Purdila,
Rafael J . Wysocki, Jarkko Nikula, linux-i2c, linux-kernel
In-Reply-To: <20160920080027.19523-1-mika.westerberg@linux.intel.com>
Your patch fixes my issue, so feel free to add a
Tested-by: Nicolai Stange <nicstange@gmail.com>
for this either.
But please see my remark below.
Mika Westerberg <mika.westerberg@linux.intel.com> writes:
> When enumerating I2C devices connected to an I2C adapter we scan the whole
> namespace (as it is possible to have devices anywhere in that namespace,
> not just below the I2C adapter device) and add each found device to the I2C
> bus in question.
>
> Now after commit 525e6fabeae2 ("i2c / ACPI: add support for ACPI
> reconfigure notifications") checking of the adapter handle to the one found
> in the I2cSerialBus() resource was moved to happen after resources of the
> I2C device has been parsed. This means that if the I2cSerialBus() resource
> points to an adapter that does not exists in the system we still parse
> those resources. This is problematic in particular because
> acpi_dev_resource_interrupt() tries to configure GSI if the device also has
> an Interrupt() resource. Failing to do that results errrors like this to be
> printed on the console:
>
> [ 10.409490] ERROR: Unable to locate IOAPIC for GSI 37
>
> To fix this we pass the I2C adapter to i2c_acpi_get_info() and make sure
> the handle matches the one in the I2cSerialBus() resource before doing
> anything else to the device.
>
> Reported-by: Nicolai Stange <nicstange@gmail.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> drivers/i2c/i2c-core.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index c61c961cf8f9..eb32cb783fc8 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -168,6 +168,7 @@ static int i2c_acpi_do_lookup(struct acpi_device *adev,
>
> static int i2c_acpi_get_info(struct acpi_device *adev,
> struct i2c_board_info *info,
> + struct i2c_adapter *adapter,
> acpi_handle *adapter_handle)
> {
> struct list_head resource_list;
> @@ -182,6 +183,10 @@ static int i2c_acpi_get_info(struct acpi_device *adev,
> if (ret)
> return ret;
>
> + /* The adapter must match the one in I2cSerialBus() connector */
> + if (adapter && ACPI_HANDLE(&adapter->dev) != lookup.adapter_handle)
> + return -ENODEV;
> +
Would it be sensible to add the adapter presence check you provided
earlier, i.e.
+ else if (!adapter) {
+ /* The adapter must be present */
+ if (acpi_bus_get_device(lookup.adapter_handle, &adapter_adev))
+ return -ENODEV;
+ if (acpi_bus_get_status(adapter_adev) || !adapter_adev->status.present)
+ return -ENODEV;
+
+ }
here, because we can't know if ...
> info->fwnode = acpi_fwnode_handle(adev);
> *adapter_handle = lookup.adapter_handle;
>
> @@ -231,10 +236,7 @@ static acpi_status i2c_acpi_add_device(acpi_handle handle, u32 level,
> if (acpi_bus_get_device(handle, &adev))
> return AE_OK;
>
> - if (i2c_acpi_get_info(adev, &info, &adapter_handle))
> - return AE_OK;
> -
> - if (adapter_handle != ACPI_HANDLE(&adapter->dev))
> + if (i2c_acpi_get_info(adev, &info, adapter, &adapter_handle))
> return AE_OK;
>
> i2c_acpi_register_device(adapter, adev, &info);
> @@ -368,7 +370,7 @@ static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
>
> switch (value) {
> case ACPI_RECONFIG_DEVICE_ADD:
> - if (i2c_acpi_get_info(adev, &info, &adapter_handle))
> + if (i2c_acpi_get_info(adev, &info, NULL, &adapter_handle))
> break;
... the ACPI device added here is physically existent?
>
> adapter = i2c_acpi_find_adapter_by_handle(adapter_handle);
I suppose that it is always true that adev has been LoadTable()'d from
some SSDT? Can't this SSDT be just as broken as my DSDT is? Not that
I've seen such a case in the real world, I'm just asking.
Thanks,
Nicolai
^ permalink raw reply
* Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning
From: Peter Zijlstra @ 2016-09-20 10:28 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Peter Rosin, Wolfram Sang, Linus Walleij, Alexandre Courbot,
Andy Shevchenko, Vignesh R, Yong Li, Geert Uytterhoeven,
Ingo Molnar, linux-i2c, linux-gpio, LKML
In-Reply-To: <CAMpxmJVNbsZewcui_WL0aZi3e4Q52=qHxGkm=w5EAr49YAqyaw@mail.gmail.com>
On Tue, Sep 20, 2016 at 12:07:39PM +0200, Bartosz Golaszewski wrote:
> 2016-09-20 10:48 GMT+02:00 Peter Rosin <peda@axentia.se>:
> >
> > One pretty simple problematic case is:
> >
> > .---. .----.
> > | | | |-- i2c2
> > | |-- i2c0 --|mux0| .----.
> > | l | | |-- i2c3 --|gpio|
> > | i | '----' '----'
> > | n | .--------------'
> > | u | .----. .----.
> > | x | | |-- i2c4 --|dev0|
> > | |-- i2c1 --|mux1| '----'
> > | | | |-- i2c5
> > '---' '----'
Shees, and I suppose this is all external to SoC stuff, so people can
stick on whatever they pretty well please. I mean, its an i2c bus, just
order parts from ebay and stick on a board.
> > Accesses to dev0 will:
> >
> > 1. lock i2c1:mux_lock (depth 0)
> > 2. switch mux1 to i2c4 using gpio
> > a lock i2c0:mux_lock (depth 0)
> > b switch mux0 to i2c3 using whatever
> > c access gpio
> > d unlock i2c0:mux_lock
> > 3. access dev0
> > 4. unlock i2c1:mux_lock
> >
> > 2a will cause a lockdep splat if i2c0:mux_lock is in the same
> > lockdep class & subclass as i2c1:mux_lock. So, lockdep needs
> > separate lockdep classes depending on the i2c root adapter
> > (subclasses are needed to handle deeper trees, so they are off
> > limits). Great fun. How do I go about creating a new lockdep
> > class for every i2c root adapter instance?
> >
>
> I feel like it's just wrong to set an arbitrary limit on the number of
> i2c branches - and this is what the result of this approach would be.
>
> One solution that comes to mind is to have a separate, global set of
> lock classes solely for gpio expanders. I think you mentioned earlier
> that it's the only thing that can cause this kind of lockdep false
> positives. We could potentially have a limited set of lock classes and
> every expander that would need one would request it using some kind of
> API ensuring that every instance gets a separate class. But this
> sounds like a big hack too I'm afraid... And regmap would need to be
> aware of that as well.
>
> Anyways, we're past rc7 already and 4.9 will be the next LTS kernel.
> We have real hardware here that runs on mainline linux and is
> suffering from this issue. Are there any objections against merging
> this series now and continuing the work on improving the solution for
> 4.10?
Seems sensible, I'll also see if I can come up with a better annotation
that can help here.
^ permalink raw reply
* Re: [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer
From: Martin Sperl @ 2016-09-20 10:15 UTC (permalink / raw)
To: Noralf Trønnes, wsa, swarren, eric
Cc: linux-rpi-kernel, linux-kernel, linux-arm-kernel, linux-i2c
In-Reply-To: <fb5e8e93-c6af-0050-1e7d-a69b722feadb@tronnes.org>
On 20.09.2016 10:41, Noralf Trønnes wrote:
>
> Den 20.09.2016 09:19, skrev Martin Sperl:
>> Hi Noralf!
>>
>> On 19.09.2016 17:26, Noralf Trønnes wrote:
>>> 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(-)
>> ...
>>> @@ -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;
>>> }
>> This does not seem to implement the i2c_msg api correctly.
>>
>> As per comments in include/uapi/linux/i2c.h on line 58 only the last
>> message
>> in a group should - by default - send a STOP.
>>
>
> Apparently it's a known problem that the i2c controller doesn't support
> Repeated Start. It will always issue a Stop when it has transferred DLEN
> bytes.
> Refs:
> http://www.circuitwizard.de/raspi-i2c-fix/raspi-i2c-fix.html
> http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they
>
>
> UNLESS: a Start Transfer (ST) is issued after Transfer Active (TA) is set
> and before DONE is set (or the last byte is shifted, I don't know excatly).
> Refs:
> https://github.com/raspberrypi/linux/issues/254#issuecomment-15254134
> https://www.raspberrypi.org/forums/viewtopic.php?p=807834&sid=2b612c7209f2175bf1a266359c72ae6c#p807834
>
>
> I found this answer/report by joan that the downstream combined support
> isn't reliable:
> http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they
>
>
> My implementation differs from downstream in that I use local_irq_save()
> to protect the polling loop. But that only protects from missing the TA
> (downstream can miss the TA and issue a Stop).
>
> So currently in mainline we have a driver that says it support the standard
> (I2C_FUNC_I2C), but it really only supports one message transfers since it
> can't do ReStart.
>
> What I have done in this patch is to support ReStart for transfers with
> 2 messages: first write, then read. But maybe a better solution is to just
> leave this alone if it is flaky and use bitbanging instead. I don't know.
I have not said that the approach you have taken is wrong or bad.
I was only telling you that the portion inside the bcm2835_i2c_xfer:
+ /* 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;
+ }
is very specific and maybe could be done in a "generic" manner
supporting more cases.
At least add a dev_warn_once for all num > 1 cases not handled by the
code above.
This gives people an opportunity to detect such a situation if they
find something is not working as expected.
Martin
^ permalink raw reply
* Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning
From: Bartosz Golaszewski @ 2016-09-20 10:07 UTC (permalink / raw)
To: Peter Rosin
Cc: Peter Zijlstra, Wolfram Sang, Linus Walleij, Alexandre Courbot,
Andy Shevchenko, Vignesh R, Yong Li, Geert Uytterhoeven,
Ingo Molnar, linux-i2c, linux-gpio, LKML
In-Reply-To: <e4b4c894-4913-b00f-0a9d-243258f2699b@axentia.se>
2016-09-20 10:48 GMT+02:00 Peter Rosin <peda@axentia.se>:
>
> One pretty simple problematic case is:
>
> .---. .----.
> | | | |-- i2c2
> | |-- i2c0 --|mux0| .----.
> | l | | |-- i2c3 --|gpio|
> | i | '----' '----'
> | n | .--------------'
> | u | .----. .----.
> | x | | |-- i2c4 --|dev0|
> | |-- i2c1 --|mux1| '----'
> | | | |-- i2c5
> '---' '----'
>
> Accesses to dev0 will:
>
> 1. lock i2c1:mux_lock (depth 0)
> 2. switch mux1 to i2c4 using gpio
> a lock i2c0:mux_lock (depth 0)
> b switch mux0 to i2c3 using whatever
> c access gpio
> d unlock i2c0:mux_lock
> 3. access dev0
> 4. unlock i2c1:mux_lock
>
> 2a will cause a lockdep splat if i2c0:mux_lock is in the same
> lockdep class & subclass as i2c1:mux_lock. So, lockdep needs
> separate lockdep classes depending on the i2c root adapter
> (subclasses are needed to handle deeper trees, so they are off
> limits). Great fun. How do I go about creating a new lockdep
> class for every i2c root adapter instance?
>
I feel like it's just wrong to set an arbitrary limit on the number of
i2c branches - and this is what the result of this approach would be.
One solution that comes to mind is to have a separate, global set of
lock classes solely for gpio expanders. I think you mentioned earlier
that it's the only thing that can cause this kind of lockdep false
positives. We could potentially have a limited set of lock classes and
every expander that would need one would request it using some kind of
API ensuring that every instance gets a separate class. But this
sounds like a big hack too I'm afraid... And regmap would need to be
aware of that as well.
Anyways, we're past rc7 already and 4.9 will be the next LTS kernel.
We have real hardware here that runs on mainline linux and is
suffering from this issue. Are there any objections against merging
this series now and continuing the work on improving the solution for
4.10?
Best regards,
Bartosz Golaszewski
^ permalink raw reply
* Re: [PATCH v2 0/4] gpio: fix an incorrect lockdep warning
From: Peter Rosin @ 2016-09-20 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: <20160919090358.GU5016@twins.programming.kicks-ass.net>
On 2016-09-19 11:03, Peter Zijlstra wrote:
> 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.
So, are we willing to risk a regression and ask any victims of that
fallout to switch to the -RT kernel? -RT is still not completely
mainline, right? Is the part with the mutex->rt_mutex conversion
in mainline? In other words, is it just a config option that we
can ask victims to use, or will they have to defer to patches?
I guess the relatively freshly introduced rt_mutex "mux_lock" in
include/linux/i2c.h can be safely converted to an ordinary mutex since
the only reason I went with rt_mutex was that "bus_lock" was an
rt_mutex and they are grabbed in similar circumstances. I can certainly
send a patch. However, the i2c muxing suffers from the issue we were
originally discussing, potentially causing false positive lockdep
reports when you build complex hierarchies with additional
dependencies between branches coming from client devices in the
hierarchy (such as i2c-muxes controlled by i2c-gpio-expanders).
One pretty simple problematic case is:
.---. .----.
| | | |-- i2c2
| |-- i2c0 --|mux0| .----.
| l | | |-- i2c3 --|gpio|
| i | '----' '----'
| n | .--------------'
| u | .----. .----.
| x | | |-- i2c4 --|dev0|
| |-- i2c1 --|mux1| '----'
| | | |-- i2c5
'---' '----'
Accesses to dev0 will:
1. lock i2c1:mux_lock (depth 0)
2. switch mux1 to i2c4 using gpio
a lock i2c0:mux_lock (depth 0)
b switch mux0 to i2c3 using whatever
c access gpio
d unlock i2c0:mux_lock
3. access dev0
4. unlock i2c1:mux_lock
2a will cause a lockdep splat if i2c0:mux_lock is in the same
lockdep class & subclass as i2c1:mux_lock. So, lockdep needs
separate lockdep classes depending on the i2c root adapter
(subclasses are needed to handle deeper trees, so they are off
limits). Great fun. How do I go about creating a new lockdep
class for every i2c root adapter instance?
Cheers,
Peter
^ permalink raw reply
* Re: [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer
From: Noralf Trønnes @ 2016-09-20 8:41 UTC (permalink / raw)
To: Martin Sperl, wsa, swarren, eric
Cc: linux-i2c, linux-rpi-kernel, linux-arm-kernel, linux-kernel
In-Reply-To: <c47a0a10-4e0b-9025-8d38-af299d5f55da@martin.sperl.org>
Den 20.09.2016 09:19, skrev Martin Sperl:
> Hi Noralf!
>
> On 19.09.2016 17:26, Noralf Trønnes wrote:
>> 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(-)
> ...
>> @@ -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;
>> }
> This does not seem to implement the i2c_msg api correctly.
>
> As per comments in include/uapi/linux/i2c.h on line 58 only the last
> message
> in a group should - by default - send a STOP.
>
Apparently it's a known problem that the i2c controller doesn't support
Repeated Start. It will always issue a Stop when it has transferred DLEN
bytes.
Refs:
http://www.circuitwizard.de/raspi-i2c-fix/raspi-i2c-fix.html
http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they
UNLESS: a Start Transfer (ST) is issued after Transfer Active (TA) is set
and before DONE is set (or the last byte is shifted, I don't know excatly).
Refs:
https://github.com/raspberrypi/linux/issues/254#issuecomment-15254134
https://www.raspberrypi.org/forums/viewtopic.php?p=807834&sid=2b612c7209f2175bf1a266359c72ae6c#p807834
I found this answer/report by joan that the downstream combined support
isn't reliable:
http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they
My implementation differs from downstream in that I use local_irq_save()
to protect the polling loop. But that only protects from missing the TA
(downstream can miss the TA and issue a Stop).
So currently in mainline we have a driver that says it support the standard
(I2C_FUNC_I2C), but it really only supports one message transfers since it
can't do ReStart.
What I have done in this patch is to support ReStart for transfers with
2 messages: first write, then read. But maybe a better solution is to just
leave this alone if it is flaky and use bitbanging instead. I don't know.
Noralf.
> As far as I understand you would need to implement the I2C_M_STOP flag
> (by exposing I2C_FUNC_PROTOCOL_MANGLING in bcm2835_i2c_func)
> to make this work correctly:
>
> for (i = 0; i < num; i++) {
> + bool send_stop = (i == num - 1) ||msgs[i]
> <http://lxr.free-electrons.com/ident?i=msg>->flags
> <http://lxr.free-electrons.com/ident?i=flags> &I2C_M_STOP
> <http://lxr.free-electrons.com/ident?i=I2C_M_STOP>;
> - ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i]);
> + ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i], send_stop);
> if (ret)
> break;
> }
>
> The corresponding device driver (or userspace) will need to set the
> flag correctly.
>
> Martin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] i2c / ACPI: Do not touch an I2C device if it belongs to another adapter
From: Mika Westerberg @ 2016-09-20 8:00 UTC (permalink / raw)
To: Wolfram Sang
Cc: Nicolai Stange, Octavian Purdila, Rafael J . Wysocki,
Jarkko Nikula, Mika Westerberg, linux-i2c, linux-kernel
In-Reply-To: <87fuowf0tf.fsf@gmail.com>
When enumerating I2C devices connected to an I2C adapter we scan the whole
namespace (as it is possible to have devices anywhere in that namespace,
not just below the I2C adapter device) and add each found device to the I2C
bus in question.
Now after commit 525e6fabeae2 ("i2c / ACPI: add support for ACPI
reconfigure notifications") checking of the adapter handle to the one found
in the I2cSerialBus() resource was moved to happen after resources of the
I2C device has been parsed. This means that if the I2cSerialBus() resource
points to an adapter that does not exists in the system we still parse
those resources. This is problematic in particular because
acpi_dev_resource_interrupt() tries to configure GSI if the device also has
an Interrupt() resource. Failing to do that results errrors like this to be
printed on the console:
[ 10.409490] ERROR: Unable to locate IOAPIC for GSI 37
To fix this we pass the I2C adapter to i2c_acpi_get_info() and make sure
the handle matches the one in the I2cSerialBus() resource before doing
anything else to the device.
Reported-by: Nicolai Stange <nicstange@gmail.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
drivers/i2c/i2c-core.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index c61c961cf8f9..eb32cb783fc8 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -168,6 +168,7 @@ static int i2c_acpi_do_lookup(struct acpi_device *adev,
static int i2c_acpi_get_info(struct acpi_device *adev,
struct i2c_board_info *info,
+ struct i2c_adapter *adapter,
acpi_handle *adapter_handle)
{
struct list_head resource_list;
@@ -182,6 +183,10 @@ static int i2c_acpi_get_info(struct acpi_device *adev,
if (ret)
return ret;
+ /* The adapter must match the one in I2cSerialBus() connector */
+ if (adapter && ACPI_HANDLE(&adapter->dev) != lookup.adapter_handle)
+ return -ENODEV;
+
info->fwnode = acpi_fwnode_handle(adev);
*adapter_handle = lookup.adapter_handle;
@@ -231,10 +236,7 @@ static acpi_status i2c_acpi_add_device(acpi_handle handle, u32 level,
if (acpi_bus_get_device(handle, &adev))
return AE_OK;
- if (i2c_acpi_get_info(adev, &info, &adapter_handle))
- return AE_OK;
-
- if (adapter_handle != ACPI_HANDLE(&adapter->dev))
+ if (i2c_acpi_get_info(adev, &info, adapter, &adapter_handle))
return AE_OK;
i2c_acpi_register_device(adapter, adev, &info);
@@ -368,7 +370,7 @@ static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
switch (value) {
case ACPI_RECONFIG_DEVICE_ADD:
- if (i2c_acpi_get_info(adev, &info, &adapter_handle))
+ if (i2c_acpi_get_info(adev, &info, NULL, &adapter_handle))
break;
adapter = i2c_acpi_find_adapter_by_handle(adapter_handle);
--
2.9.3
^ permalink raw reply related
* Re: [REGRESSION? v4.8] i2c-core: acpi_i2c_get_info() touches non-existent devices
From: Mika Westerberg @ 2016-09-20 7:55 UTC (permalink / raw)
To: Nicolai Stange
Cc: Wolfram Sang, Octavian Purdila, Rafael J. Wysocki, linux-i2c,
linux-kernel
In-Reply-To: <87fuowf0tf.fsf@gmail.com>
On Mon, Sep 19, 2016 at 03:58:52PM +0200, Nicolai Stange wrote:
> > 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.
Ah, indeed it needs to happen before we parse resources.
> 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>
Thanks.
> 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...
You are right it should not do that. I'll send you an updated patch
shortly, can you try if it works?
^ permalink raw reply
* Re: [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer
From: Martin Sperl @ 2016-09-20 7:19 UTC (permalink / raw)
To: Noralf Trønnes, wsa, swarren, eric
Cc: linux-rpi-kernel, linux-kernel, linux-arm-kernel, linux-i2c
In-Reply-To: <1474298777-5858-2-git-send-email-noralf@tronnes.org>
Hi Noralf!
On 19.09.2016 17:26, Noralf Trønnes wrote:
> 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(-)
...
> @@ -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;
> }
This does not seem to implement the i2c_msg api correctly.
As per comments in include/uapi/linux/i2c.h on line 58 only the last message
in a group should - by default - send a STOP.
As far as I understand you would need to implement the I2C_M_STOP flag
(by exposing I2C_FUNC_PROTOCOL_MANGLING in bcm2835_i2c_func)
to make this work correctly:
for (i = 0; i < num; i++) {
+ bool send_stop = (i == num - 1) ||msgs[i] <http://lxr.free-electrons.com/ident?i=msg>->flags <http://lxr.free-electrons.com/ident?i=flags> &I2C_M_STOP <http://lxr.free-electrons.com/ident?i=I2C_M_STOP>;
- ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i]);
+ ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i], send_stop);
if (ret)
break;
}
The corresponding device driver (or userspace) will need to set the flag
correctly.
Martin
^ permalink raw reply
* Re: [PATCH 1/3] i2c: bcm2835: Fix hang for writing messages larger than 16 bytes
From: Martin Sperl @ 2016-09-20 6:46 UTC (permalink / raw)
To: Noralf Trønnes, wsa, swarren, eric
Cc: linux-i2c, linux-rpi-kernel, linux-arm-kernel, linux-kernel
In-Reply-To: <1474298777-5858-1-git-send-email-noralf@tronnes.org>
On 19.09.2016 17:26, Noralf Trønnes wrote:
> 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.
I remember having seen similar interrupt issues with the SPI HW-block.
> In this situation wait_for_completion_timeout() apparently is unable to
> stop the madness.
That is because it is a level irq that immediately triggers another
interrupt giving
no CPU no time to do other (threaded) OS activity - this might be slightly
different for multi-core machines...
> 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>
Reviewed-by: Martin Sperl <kernel@martin.sperl.org>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 2/2] i2c: aspeed: added documentation for Aspeed I2C driver
From: Brendan Higgins @ 2016-09-19 23:26 UTC (permalink / raw)
To: Rob Herring
Cc: Wolfram Sang, mark.rutland, linux-i2c, devicetree, linux-kernel,
OpenBMC Maillist, Joel Stanley, Jeremy Kerr
In-Reply-To: <20160919213536.GA19274@rob-hp-laptop>
Addressed in v3.
Thanks!
On Mon, Sep 19, 2016 at 2:35 PM, Rob Herring <robh@kernel.org> wrote:
> On Fri, Sep 09, 2016 at 06:55:51PM -0700, Brendan Higgins wrote:
>> Added device tree binding documentation for Aspeed I2C controller and
>> busses.
>>
>> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
>> ---
>> Changes for v2:
>> - None
>> ---
>> .../devicetree/bindings/i2c/i2c-aspeed.txt | 63 ++++++++++++++++++++++
>> 1 file changed, 63 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
>> new file mode 100644
>> index 0000000..df68f2a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
>> @@ -0,0 +1,63 @@
>> +Device tree configuration for the I2C controller and busses on the AST24XX
>> +and AST25XX SoCs.
>> +
>> +Controller:
>> +
>> + Required Properties:
>> + - #address-cells : should be 1
>> + - #size-cells : should be 1
>> + - #interrupt-cells : should be 1
>> + - compatible : should be "aspeed,ast2400-i2c-controller"
>> + or "aspeed,ast2500-i2c-controller"
>> + - reg : address start and range of controller
>> + - ranges : defines address offset and range for busses
>> + - interrupts : interrupt number
>> + - clocks : root clock of bus, should reference the APB
>> + clock
>> + - clock-ranges : specifies that child busses can inherit clocks
>> + - interrupt-controller : denotes that the controller receives and fires
>> + new interrupts for child busses
>> +
>> +Bus:
>> +
>> + Required Properties:
>> + - #address-cells : should be 1
>> + - #size-cells : should be 0
>> + - reg : address offset and range of bus
>> + - compatible : should be "aspeed,ast2400-i2c-bus"
>> + or "aspeed,ast2500-i2c-bus"
>> + - bus : the bus's number
>
> Don't use indexes. The reg property is enough to id which bus is which.
^ permalink raw reply
* [PATCH v3 2/2] i2c: aspeed: added documentation for Aspeed I2C driver
From: Brendan Higgins @ 2016-09-19 23:23 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland
Cc: linux-i2c, devicetree, linux-kernel, openbmc, joel, jk,
Brendan Higgins
In-Reply-To: <1474327414-651-1-git-send-email-brendanhiggins@google.com>
Added device tree binding documentation for Aspeed I2C controller and
busses.
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
Changes for v2:
- None
Changes for v3:
- Removed reference to "bus" device tree param
---
.../devicetree/bindings/i2c/i2c-aspeed.txt | 61 ++++++++++++++++++++++
1 file changed, 61 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
new file mode 100644
index 0000000..dd11a97
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
@@ -0,0 +1,61 @@
+Device tree configuration for the I2C controller and busses on the AST24XX
+and AST25XX SoCs.
+
+Controller:
+
+ Required Properties:
+ - #address-cells : should be 1
+ - #size-cells : should be 1
+ - #interrupt-cells : should be 1
+ - compatible : should be "aspeed,ast2400-i2c-controller"
+ or "aspeed,ast2500-i2c-controller"
+ - reg : address start and range of controller
+ - ranges : defines address offset and range for busses
+ - interrupts : interrupt number
+ - clocks : root clock of bus, should reference the APB
+ clock
+ - clock-ranges : specifies that child busses can inherit clocks
+ - interrupt-controller : denotes that the controller receives and fires
+ new interrupts for child busses
+
+Bus:
+
+ Required Properties:
+ - #address-cells : should be 1
+ - #size-cells : should be 0
+ - reg : address offset and range of bus
+ - compatible : should be "aspeed,ast2400-i2c-bus"
+ or "aspeed,ast2500-i2c-bus"
+ - interrupts : interrupt number
+
+ Optional Properties:
+ - clock-frequency : frequency of the bus clock in Hz
+ defaults to 100 kHz when not specified
+
+Example:
+
+i2c: i2c@1e78a000 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ #interrupt-cells = <1>;
+
+ compatible = "aspeed,ast2400-i2c-controller";
+ reg = <0x1e78a000 0x40>;
+ ranges = <0 0x1e78a000 0x1000>;
+ interrupts = <12>;
+ clocks = <&clk_apb>;
+ clock-ranges;
+ interrupt-controller;
+
+ i2c0: i2c-bus@40 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x40 0x40>;
+ compatible = "aspeed,ast2400-i2c-bus";
+ clock-frequency = <100000>;
+ status = "disabled";
+ interrupts = <0>;
+ interrupt-parent = <&i2c>;
+ };
+};
+
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v3 1/2] i2c: aspeed: added driver for Aspeed I2C
From: Brendan Higgins @ 2016-09-19 23:23 UTC (permalink / raw)
To: wsa, robh+dt, mark.rutland
Cc: linux-i2c, devicetree, linux-kernel, openbmc, joel, jk,
Brendan Higgins
In-Reply-To: <CAFd5g47W-dGVt6hA+FSpqxCqvd5ZpQP=FCCpYGErahF0iKqTEQ@mail.gmail.com>
Added initial master and slave support for Aspeed I2C controller.
Supports fourteen busses present in ast24xx and ast25xx BMC SoCs by
Aspeed.
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
Changes for v2:
- Added single module_init (multiple was breaking some builds).
Changes for v3:
- Removed "bus" device tree param; now extracted from bus address offset
---
drivers/i2c/busses/Kconfig | 10 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-aspeed.c | 846 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 857 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-aspeed.c
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 5c3993b..0178c6c 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -998,6 +998,16 @@ config I2C_RCAR
This driver can also be built as a module. If so, the module
will be called i2c-rcar.
+config I2C_ASPEED
+ tristate "Aspeed AST2xxx SoC I2C Controller"
+ depends on (ARCH_ASPEED || COMPILE_TEST) && OF
+ help
+ If you say yes to this option, support will be included for the
+ Aspeed AST2xxx SoC I2C controller.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-aspeed.
+
comment "External I2C/SMBus adapter drivers"
config I2C_DIOLAN_U2C
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 37f2819..49631cd 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -96,6 +96,7 @@ obj-$(CONFIG_I2C_XILINX) += i2c-xiic.o
obj-$(CONFIG_I2C_XLR) += i2c-xlr.o
obj-$(CONFIG_I2C_XLP9XX) += i2c-xlp9xx.o
obj-$(CONFIG_I2C_RCAR) += i2c-rcar.o
+obj-$(CONFIG_I2C_ASPEED) += i2c-aspeed.o
# External I2C/SMBus adapter drivers
obj-$(CONFIG_I2C_DIOLAN_U2C) += i2c-diolan-u2c.o
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
new file mode 100644
index 0000000..207533e
--- /dev/null
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -0,0 +1,846 @@
+/*
+ * I2C adapter for the ASPEED I2C bus.
+ *
+ * Copyright (C) 2012-2020 ASPEED Technology Inc.
+ * Copyright 2016 IBM Corporation
+ * Copyright 2016 Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/errno.h>
+#include <linux/interrupt.h>
+#include <linux/completion.h>
+#include <linux/slab.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+
+/* I2C Register */
+#define ASPEED_I2C_FUN_CTRL_REG 0x00
+#define ASPEED_I2C_AC_TIMING_REG1 0x04
+#define ASPEED_I2C_AC_TIMING_REG2 0x08
+#define ASPEED_I2C_INTR_CTRL_REG 0x0c
+#define ASPEED_I2C_INTR_STS_REG 0x10
+#define ASPEED_I2C_CMD_REG 0x14
+#define ASPEED_I2C_DEV_ADDR_REG 0x18
+#define ASPEED_I2C_BYTE_BUF_REG 0x20
+#define ASPEED_I2C_OFFSET_START 0x40
+#define ASPEED_I2C_OFFSET_INCREMENT 0x40
+
+#define ASPEED_I2C_NUM_BUS 14
+
+/* Global Register Definition */
+/* 0x00 : I2C Interrupt Status Register */
+/* 0x08 : I2C Interrupt Target Assignment */
+
+/* Device Register Definition */
+/* 0x00 : I2CD Function Control Register */
+#define ASPEED_I2CD_MULTI_MASTER_DIS BIT(15)
+#define ASPEED_I2CD_SDA_DRIVE_1T_EN BIT(8)
+#define ASPEED_I2CD_M_SDA_DRIVE_1T_EN BIT(7)
+#define ASPEED_I2CD_M_HIGH_SPEED_EN BIT(6)
+#define ASPEED_I2CD_SLAVE_EN BIT(1)
+#define ASPEED_I2CD_MASTER_EN BIT(0)
+
+/* 0x08 : I2CD Clock and AC Timing Control Register #2 */
+#define ASPEED_NO_TIMEOUT_CTRL 0
+
+
+/* 0x0c : I2CD Interrupt Control Register &
+ * 0x10 : I2CD Interrupt Status Register
+ *
+ * These share bit definitions, so use the same values for the enable &
+ * status bits.
+ */
+#define ASPEED_I2CD_INTR_SDA_DL_TIMEOUT BIT(14)
+#define ASPEED_I2CD_INTR_BUS_RECOVER_DONE BIT(13)
+#define ASPEED_I2CD_INTR_SLAVE_MATCH BIT(7)
+#define ASPEED_I2CD_INTR_SCL_TIMEOUT BIT(6)
+#define ASPEED_I2CD_INTR_ABNORMAL BIT(5)
+#define ASPEED_I2CD_INTR_NORMAL_STOP BIT(4)
+#define ASPEED_I2CD_INTR_ARBIT_LOSS BIT(3)
+#define ASPEED_I2CD_INTR_RX_DONE BIT(2)
+#define ASPEED_I2CD_INTR_TX_NAK BIT(1)
+#define ASPEED_I2CD_INTR_TX_ACK BIT(0)
+
+/* 0x14 : I2CD Command/Status Register */
+#define ASPEED_I2CD_SCL_LINE_STS BIT(18)
+#define ASPEED_I2CD_SDA_LINE_STS BIT(17)
+#define ASPEED_I2CD_BUS_BUSY_STS BIT(16)
+#define ASPEED_I2CD_BUS_RECOVER_CMD BIT(11)
+
+/* Command Bit */
+#define ASPEED_I2CD_M_STOP_CMD BIT(5)
+#define ASPEED_I2CD_M_S_RX_CMD_LAST BIT(4)
+#define ASPEED_I2CD_M_RX_CMD BIT(3)
+#define ASPEED_I2CD_S_TX_CMD BIT(2)
+#define ASPEED_I2CD_M_TX_CMD BIT(1)
+#define ASPEED_I2CD_M_START_CMD BIT(0)
+
+/* 0x18 : I2CD Slave Device Address Register */
+#define ASPEED_I2CD_DEV_ADDR_MASK GENMASK(6, 0)
+
+enum aspeed_i2c_slave_state {
+ ASPEED_I2C_SLAVE_START,
+ ASPEED_I2C_SLAVE_READ_REQUESTED,
+ ASPEED_I2C_SLAVE_READ_PROCESSED,
+ ASPEED_I2C_SLAVE_WRITE_REQUESTED,
+ ASPEED_I2C_SLAVE_WRITE_RECEIVED,
+ ASPEED_I2C_SLAVE_STOP,
+};
+
+struct aspeed_i2c_bus {
+ struct i2c_adapter adap;
+ struct device *dev;
+ void __iomem *base;
+ spinlock_t lock;
+ struct completion cmd_complete;
+ int irq;
+ /* Transaction state. */
+ struct i2c_msg *msg;
+ int msg_pos;
+ u32 cmd_err;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+ struct i2c_client *slave;
+ enum aspeed_i2c_slave_state slave_state;
+#endif
+};
+
+struct aspeed_i2c_controller {
+ struct device *dev;
+ void __iomem *base;
+ int irq;
+ struct irq_domain *irq_domain;
+};
+
+static inline void aspeed_i2c_write(struct aspeed_i2c_bus *bus, u32 val,
+ u32 reg)
+{
+ writel(val, bus->base + reg);
+}
+
+static inline u32 aspeed_i2c_read(struct aspeed_i2c_bus *bus, u32 reg)
+{
+ return readl(bus->base + reg);
+}
+
+static u8 aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
+{
+ u32 command;
+ unsigned long time_left;
+ unsigned long flags;
+ int ret = 0;
+
+ spin_lock_irqsave(&bus->lock, flags);
+ command = aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG);
+ /* Bus is idle: no recovery needed. */
+ if ((command & ASPEED_I2CD_SDA_LINE_STS) &&
+ (command & ASPEED_I2CD_SCL_LINE_STS))
+ goto out;
+
+ dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n",
+ command);
+
+ /* Bus held: put bus in stop state. */
+ if ((command & ASPEED_I2CD_SDA_LINE_STS) &&
+ !(command & ASPEED_I2CD_SCL_LINE_STS)) {
+ aspeed_i2c_write(bus, ASPEED_I2CD_M_STOP_CMD,
+ ASPEED_I2C_CMD_REG);
+ reinit_completion(&bus->cmd_complete);
+ spin_unlock_irqrestore(&bus->lock, flags);
+
+ time_left = wait_for_completion_interruptible_timeout(
+ &bus->cmd_complete, bus->adap.timeout * HZ);
+
+ spin_lock_irqsave(&bus->lock, flags);
+ if (time_left == 0)
+ ret = -ETIMEDOUT;
+ else if (bus->cmd_err)
+ ret = -EIO;
+ /* Bus error. */
+ } else if (!(command & ASPEED_I2CD_SDA_LINE_STS)) {
+ aspeed_i2c_write(bus, ASPEED_I2CD_BUS_RECOVER_CMD,
+ ASPEED_I2C_CMD_REG);
+ reinit_completion(&bus->cmd_complete);
+ spin_unlock_irqrestore(&bus->lock, flags);
+
+ time_left = wait_for_completion_interruptible_timeout(
+ &bus->cmd_complete, bus->adap.timeout * HZ);
+
+ spin_lock_irqsave(&bus->lock, flags);
+ if (time_left == 0)
+ ret = -ETIMEDOUT;
+ else if (bus->cmd_err)
+ ret = -EIO;
+ /* Recovery failed. */
+ else if (!(aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG) &
+ ASPEED_I2CD_SDA_LINE_STS))
+ ret = -EIO;
+ }
+
+out:
+ spin_unlock_irqrestore(&bus->lock, flags);
+ return ret;
+}
+
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
+{
+ bool irq_handled = true;
+ u32 command;
+ u32 irq_status;
+ u32 status_ack = 0;
+ u8 value;
+ struct i2c_client *slave = bus->slave;
+
+ spin_lock(&bus->lock);
+ if (!slave) {
+ irq_handled = false;
+ goto out;
+ }
+ command = aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG);
+ irq_status = aspeed_i2c_read(bus, ASPEED_I2C_INTR_STS_REG);
+
+ /* Slave was requested, restart state machine. */
+ if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
+ status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
+ bus->slave_state = ASPEED_I2C_SLAVE_START;
+ }
+ /* Slave is not currently active, irq was for someone else. */
+ if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
+ irq_handled = false;
+ goto out;
+ }
+
+ dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
+ irq_status, command);
+
+ /* Slave was sent something. */
+ if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
+ value = aspeed_i2c_read(bus, ASPEED_I2C_BYTE_BUF_REG) >> 8;
+ /* Handle address frame. */
+ if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
+ if (value & 0x1)
+ bus->slave_state =
+ ASPEED_I2C_SLAVE_READ_REQUESTED;
+ else
+ bus->slave_state =
+ ASPEED_I2C_SLAVE_WRITE_REQUESTED;
+ }
+ status_ack |= ASPEED_I2CD_INTR_RX_DONE;
+ }
+
+ /* Slave was asked to stop. */
+ if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
+ status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
+ bus->slave_state = ASPEED_I2C_SLAVE_STOP;
+ }
+ if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
+ status_ack |= ASPEED_I2CD_INTR_TX_NAK;
+ bus->slave_state = ASPEED_I2C_SLAVE_STOP;
+ }
+
+ if (bus->slave_state == ASPEED_I2C_SLAVE_READ_REQUESTED) {
+ if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
+ dev_err(bus->dev, "Unexpected ACK on read request.\n");
+ bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
+
+ i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
+ aspeed_i2c_write(bus, value, ASPEED_I2C_BYTE_BUF_REG);
+ aspeed_i2c_write(bus, ASPEED_I2CD_S_TX_CMD, ASPEED_I2C_CMD_REG);
+ } else if (bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) {
+ status_ack |= ASPEED_I2CD_INTR_TX_ACK;
+ if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
+ dev_err(bus->dev,
+ "Expected ACK after processed read.\n");
+ i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value);
+ aspeed_i2c_write(bus, value, ASPEED_I2C_BYTE_BUF_REG);
+ aspeed_i2c_write(bus, ASPEED_I2CD_S_TX_CMD, ASPEED_I2C_CMD_REG);
+ } else if (bus->slave_state == ASPEED_I2C_SLAVE_WRITE_REQUESTED) {
+ bus->slave_state = ASPEED_I2C_SLAVE_WRITE_RECEIVED;
+ i2c_slave_event(slave, I2C_SLAVE_WRITE_REQUESTED, &value);
+ } else if (bus->slave_state == ASPEED_I2C_SLAVE_WRITE_RECEIVED) {
+ i2c_slave_event(slave, I2C_SLAVE_WRITE_RECEIVED, &value);
+ } else if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
+ i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
+ }
+
+ if (status_ack != irq_status)
+ dev_err(bus->dev,
+ "irq handled != irq. expected %x, but was %x\n",
+ irq_status, status_ack);
+ aspeed_i2c_write(bus, status_ack, ASPEED_I2C_INTR_STS_REG);
+
+out:
+ spin_unlock(&bus->lock);
+ return irq_handled;
+}
+#endif
+
+static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
+{
+ const u32 errs = ASPEED_I2CD_INTR_ARBIT_LOSS |
+ ASPEED_I2CD_INTR_ABNORMAL |
+ ASPEED_I2CD_INTR_SCL_TIMEOUT |
+ ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |
+ ASPEED_I2CD_INTR_TX_NAK;
+ u32 irq_status;
+
+ spin_lock(&bus->lock);
+ irq_status = aspeed_i2c_read(bus, ASPEED_I2C_INTR_STS_REG);
+ bus->cmd_err = irq_status & errs;
+
+ dev_dbg(bus->dev, "master irq status 0x%08x\n", irq_status);
+
+ /* No message to transfer. */
+ if (bus->cmd_err ||
+ (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) ||
+ (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE)) {
+ complete(&bus->cmd_complete);
+ goto out;
+ } else if (!bus->msg || bus->msg_pos >= bus->msg->len)
+ goto out;
+
+ if ((bus->msg->flags & I2C_M_RD) &&
+ (irq_status & ASPEED_I2CD_INTR_RX_DONE)) {
+ bus->msg->buf[bus->msg_pos++] = aspeed_i2c_read(
+ bus, ASPEED_I2C_BYTE_BUF_REG) >> 8;
+ if (bus->msg_pos + 1 < bus->msg->len)
+ aspeed_i2c_write(bus, ASPEED_I2CD_M_RX_CMD,
+ ASPEED_I2C_CMD_REG);
+ else if (bus->msg_pos < bus->msg->len)
+ aspeed_i2c_write(bus, ASPEED_I2CD_M_RX_CMD |
+ ASPEED_I2CD_M_S_RX_CMD_LAST,
+ ASPEED_I2C_CMD_REG);
+ } else if (!(bus->msg->flags & I2C_M_RD) &&
+ (irq_status & ASPEED_I2CD_INTR_TX_ACK)) {
+ aspeed_i2c_write(bus, bus->msg->buf[bus->msg_pos++],
+ ASPEED_I2C_BYTE_BUF_REG);
+ aspeed_i2c_write(bus, ASPEED_I2CD_M_TX_CMD, ASPEED_I2C_CMD_REG);
+ }
+
+ /* Transmission complete: notify caller. */
+ if (bus->msg_pos >= bus->msg->len)
+ complete(&bus->cmd_complete);
+out:
+ aspeed_i2c_write(bus, irq_status, ASPEED_I2C_INTR_STS_REG);
+ spin_unlock(&bus->lock);
+ return true;
+}
+
+static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
+{
+ struct aspeed_i2c_bus *bus = dev_id;
+
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+ if (aspeed_i2c_slave_irq(bus)) {
+ dev_dbg(bus->dev, "irq handled by slave.\n");
+ return IRQ_HANDLED;
+ }
+#endif
+ if (aspeed_i2c_master_irq(bus)) {
+ dev_dbg(bus->dev, "irq handled by master.\n");
+ return IRQ_HANDLED;
+ }
+ dev_err(bus->dev, "irq not handled properly!\n");
+ return IRQ_HANDLED;
+}
+
+static int aspeed_i2c_master_single_xfer(struct i2c_adapter *adap,
+ struct i2c_msg *msg)
+{
+ struct aspeed_i2c_bus *bus = adap->algo_data;
+ unsigned long flags;
+ u8 slave_addr;
+ u32 command = ASPEED_I2CD_M_START_CMD | ASPEED_I2CD_M_TX_CMD;
+ int ret = msg->len;
+ unsigned long time_left;
+
+ spin_lock_irqsave(&bus->lock, flags);
+ bus->msg = msg;
+ bus->msg_pos = 0;
+ slave_addr = msg->addr << 1;
+ if (msg->flags & I2C_M_RD) {
+ slave_addr |= 1;
+ command |= ASPEED_I2CD_M_RX_CMD;
+ if (msg->len == 1)
+ command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
+ }
+ aspeed_i2c_write(bus, slave_addr, ASPEED_I2C_BYTE_BUF_REG);
+ aspeed_i2c_write(bus, command, ASPEED_I2C_CMD_REG);
+ reinit_completion(&bus->cmd_complete);
+ spin_unlock_irqrestore(&bus->lock, flags);
+
+ time_left = wait_for_completion_interruptible_timeout(
+ &bus->cmd_complete, bus->adap.timeout * HZ * msg->len);
+ if (time_left == 0)
+ return -ETIMEDOUT;
+
+ spin_lock_irqsave(&bus->lock, flags);
+ if (bus->cmd_err)
+ ret = -EIO;
+ bus->msg = NULL;
+ spin_unlock_irqrestore(&bus->lock, flags);
+
+ return ret;
+}
+
+static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
+ struct i2c_msg *msgs, int num)
+{
+ struct aspeed_i2c_bus *bus = adap->algo_data;
+ int ret;
+ int i;
+ unsigned long flags;
+ unsigned long time_left;
+
+ /* If bus is busy, attempt recovery. We assume a single master
+ * environment.
+ */
+ if (aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG) &
+ ASPEED_I2CD_BUS_BUSY_STS) {
+ ret = aspeed_i2c_recover_bus(bus);
+ if (ret)
+ return ret;
+ }
+
+ for (i = 0; i < num; i++) {
+ ret = aspeed_i2c_master_single_xfer(adap, &msgs[i]);
+ if (ret < 0)
+ break;
+ /* TODO: Support other forms of I2C protocol mangling. */
+ if (msgs[i].flags & I2C_M_STOP) {
+ spin_lock_irqsave(&bus->lock, flags);
+ aspeed_i2c_write(bus, ASPEED_I2CD_M_STOP_CMD,
+ ASPEED_I2C_CMD_REG);
+ reinit_completion(&bus->cmd_complete);
+ spin_unlock_irqrestore(&bus->lock, flags);
+
+ time_left = wait_for_completion_interruptible_timeout(
+ &bus->cmd_complete,
+ bus->adap.timeout * HZ);
+ if (time_left == 0)
+ return -ETIMEDOUT;
+ }
+ }
+
+ spin_lock_irqsave(&bus->lock, flags);
+ aspeed_i2c_write(bus, ASPEED_I2CD_M_STOP_CMD, ASPEED_I2C_CMD_REG);
+ reinit_completion(&bus->cmd_complete);
+ spin_unlock_irqrestore(&bus->lock, flags);
+
+ time_left = wait_for_completion_interruptible_timeout(
+ &bus->cmd_complete, bus->adap.timeout * HZ);
+ if (time_left == 0)
+ return -ETIMEDOUT;
+
+ /* If nothing went wrong, return number of messages transferred. */
+ if (ret < 0)
+ return ret;
+ else
+ return i;
+}
+
+static u32 aspeed_i2c_functionality(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA;
+}
+
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+static int aspeed_i2c_reg_slave(struct i2c_client *client)
+{
+ struct aspeed_i2c_bus *bus;
+ unsigned long flags;
+ u32 addr_reg_val;
+ u32 func_ctrl_reg_val;
+
+ bus = client->adapter->algo_data;
+ spin_lock_irqsave(&bus->lock, flags);
+ if (bus->slave) {
+ spin_unlock_irqrestore(&bus->lock, flags);
+ return -EINVAL;
+ }
+
+ /* Set slave addr. */
+ addr_reg_val = aspeed_i2c_read(bus, ASPEED_I2C_DEV_ADDR_REG);
+ addr_reg_val &= ~ASPEED_I2CD_DEV_ADDR_MASK;
+ addr_reg_val |= client->addr & ASPEED_I2CD_DEV_ADDR_MASK;
+ aspeed_i2c_write(bus, addr_reg_val, ASPEED_I2C_DEV_ADDR_REG);
+
+ /* Switch from master mode to slave mode. */
+ func_ctrl_reg_val = aspeed_i2c_read(bus, ASPEED_I2C_FUN_CTRL_REG);
+ func_ctrl_reg_val &= ~ASPEED_I2CD_MASTER_EN;
+ func_ctrl_reg_val |= ASPEED_I2CD_SLAVE_EN;
+ aspeed_i2c_write(bus, func_ctrl_reg_val, ASPEED_I2C_FUN_CTRL_REG);
+
+ bus->slave = client;
+ bus->slave_state = ASPEED_I2C_SLAVE_STOP;
+ spin_unlock_irqrestore(&bus->lock, flags);
+ return 0;
+}
+
+static int aspeed_i2c_unreg_slave(struct i2c_client *client)
+{
+ struct aspeed_i2c_bus *bus = client->adapter->algo_data;
+ unsigned long flags;
+ u32 func_ctrl_reg_val;
+
+ spin_lock_irqsave(&bus->lock, flags);
+ if (!bus->slave) {
+ spin_unlock_irqrestore(&bus->lock, flags);
+ return -EINVAL;
+ }
+
+ /* Switch from slave mode to master mode. */
+ func_ctrl_reg_val = aspeed_i2c_read(bus, ASPEED_I2C_FUN_CTRL_REG);
+ func_ctrl_reg_val &= ~ASPEED_I2CD_SLAVE_EN;
+ func_ctrl_reg_val |= ASPEED_I2CD_MASTER_EN;
+ aspeed_i2c_write(bus, func_ctrl_reg_val, ASPEED_I2C_FUN_CTRL_REG);
+
+ bus->slave = NULL;
+ spin_unlock_irqrestore(&bus->lock, flags);
+ return 0;
+}
+#endif
+
+static const struct i2c_algorithm aspeed_i2c_algo = {
+ .master_xfer = aspeed_i2c_master_xfer,
+ .functionality = aspeed_i2c_functionality,
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+ .reg_slave = aspeed_i2c_reg_slave,
+ .unreg_slave = aspeed_i2c_unreg_slave,
+#endif
+};
+
+static u32 aspeed_i2c_get_clk_reg_val(u32 divider_ratio)
+{
+ unsigned int inc = 0, div;
+ u32 scl_low, scl_high, data;
+
+ for (div = 0; divider_ratio >= 16; div++) {
+ inc |= (divider_ratio & 1);
+ divider_ratio >>= 1;
+ }
+ divider_ratio += inc;
+ scl_low = (divider_ratio >> 1) - 1;
+ scl_high = divider_ratio - scl_low - 2;
+ data = 0x77700300 | (scl_high << 16) | (scl_low << 12) | div;
+ return data;
+}
+
+static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
+ struct platform_device *pdev)
+{
+ struct clk *pclk;
+ u32 clk_freq;
+ u32 divider_ratio;
+ int ret;
+
+ pclk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(pclk)) {
+ dev_err(&pdev->dev, "clk_get failed\n");
+ return PTR_ERR(pclk);
+ }
+ ret = of_property_read_u32(pdev->dev.of_node,
+ "clock-frequency", &clk_freq);
+ if (ret < 0) {
+ dev_err(&pdev->dev,
+ "Could not read clock-frequency property\n");
+ clk_freq = 100000;
+ }
+ divider_ratio = clk_get_rate(pclk) / clk_freq;
+ /* We just need the clock rate, we don't actually use the clk object. */
+ devm_clk_put(&pdev->dev, pclk);
+
+ /* Set AC Timing */
+ if (clk_freq / 1000 > 400) {
+ aspeed_i2c_write(bus, aspeed_i2c_read(bus,
+ ASPEED_I2C_FUN_CTRL_REG) |
+ ASPEED_I2CD_M_HIGH_SPEED_EN |
+ ASPEED_I2CD_M_SDA_DRIVE_1T_EN |
+ ASPEED_I2CD_SDA_DRIVE_1T_EN,
+ ASPEED_I2C_FUN_CTRL_REG);
+
+ aspeed_i2c_write(bus, 0x3, ASPEED_I2C_AC_TIMING_REG2);
+ aspeed_i2c_write(bus, aspeed_i2c_get_clk_reg_val(divider_ratio),
+ ASPEED_I2C_AC_TIMING_REG1);
+ } else {
+ aspeed_i2c_write(bus, aspeed_i2c_get_clk_reg_val(divider_ratio),
+ ASPEED_I2C_AC_TIMING_REG1);
+ aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,
+ ASPEED_I2C_AC_TIMING_REG2);
+ }
+
+ return 0;
+}
+
+static int aspeed_i2c_bus_num(struct device_node *of_node)
+{
+ u32 bus_offset;
+ int ret;
+
+ ret = of_property_read_u32(of_node, "reg", &bus_offset);
+ if (ret < 0)
+ return ret;
+ /* Each bus resides at a fixed increment after the first. */
+ return (bus_offset - ASPEED_I2C_OFFSET_START) /
+ ASPEED_I2C_OFFSET_INCREMENT;
+}
+
+static void noop(struct irq_data *data) { }
+
+static struct irq_chip aspeed_i2c_irqchip = {
+ .name = "ast-i2c",
+ .irq_unmask = noop,
+ .irq_mask = noop,
+};
+
+static int aspeed_i2c_probe_bus(struct platform_device *pdev)
+{
+ struct aspeed_i2c_bus *bus;
+ struct aspeed_i2c_controller *controller =
+ dev_get_drvdata(pdev->dev.parent);
+ struct resource *res;
+ int ret, bus_num, irq;
+
+ bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
+ if (!bus)
+ return -ENOMEM;
+
+ bus_num = aspeed_i2c_bus_num(pdev->dev.of_node);
+ if (bus_num < 0)
+ return -ENXIO;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ bus->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(bus->base))
+ return PTR_ERR(bus->base);
+
+ bus->irq = platform_get_irq(pdev, 0);
+ if (bus->irq < 0) {
+ dev_err(&pdev->dev, "platform_get_irq failed\n");
+ return -ENXIO;
+ }
+
+ irq = irq_create_mapping(controller->irq_domain, bus_num);
+ irq_set_chip_data(irq, controller);
+ irq_set_chip_and_handler(irq, &aspeed_i2c_irqchip, handle_simple_irq);
+ ret = devm_request_irq(&pdev->dev, bus->irq, aspeed_i2c_bus_irq,
+ 0, dev_name(&pdev->dev), bus);
+ if (ret) {
+ dev_err(&pdev->dev, "devm_request_irq failed\n");
+ return -ENXIO;
+ }
+
+ /* Initialize the I2C adapter */
+ spin_lock_init(&bus->lock);
+ init_completion(&bus->cmd_complete);
+ bus->adap.nr = bus_num;
+ bus->adap.owner = THIS_MODULE;
+ bus->adap.retries = 0;
+ bus->adap.timeout = 5;
+ bus->adap.algo = &aspeed_i2c_algo;
+ bus->adap.algo_data = bus;
+ bus->adap.dev.parent = &pdev->dev;
+ bus->adap.dev.of_node = pdev->dev.of_node;
+ snprintf(bus->adap.name, sizeof(bus->adap.name), "Aspeed i2c-%d",
+ bus_num);
+
+ bus->dev = &pdev->dev;
+
+ /* reset device: disable master & slave functions */
+ aspeed_i2c_write(bus, 0, ASPEED_I2C_FUN_CTRL_REG);
+
+ ret = aspeed_i2c_init_clk(bus, pdev);
+ if (ret < 0)
+ return ret;
+
+ /* Enable Master Mode */
+ aspeed_i2c_write(bus, aspeed_i2c_read(bus, ASPEED_I2C_FUN_CTRL_REG) |
+ ASPEED_I2CD_MASTER_EN |
+ ASPEED_I2CD_MULTI_MASTER_DIS, ASPEED_I2C_FUN_CTRL_REG);
+
+ /* Set interrupt generation of I2C controller */
+ aspeed_i2c_write(bus, ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |
+ ASPEED_I2CD_INTR_BUS_RECOVER_DONE |
+ ASPEED_I2CD_INTR_SCL_TIMEOUT |
+ ASPEED_I2CD_INTR_ABNORMAL |
+ ASPEED_I2CD_INTR_NORMAL_STOP |
+ ASPEED_I2CD_INTR_ARBIT_LOSS |
+ ASPEED_I2CD_INTR_RX_DONE |
+ ASPEED_I2CD_INTR_TX_NAK |
+ ASPEED_I2CD_INTR_TX_ACK,
+ ASPEED_I2C_INTR_CTRL_REG);
+
+ ret = i2c_add_numbered_adapter(&bus->adap);
+ if (ret < 0)
+ return -ENXIO;
+
+ platform_set_drvdata(pdev, bus);
+
+ dev_info(bus->dev, "i2c bus %d registered, irq %d\n",
+ bus->adap.nr, bus->irq);
+
+ return 0;
+}
+
+static int aspeed_i2c_remove_bus(struct platform_device *pdev)
+{
+ struct aspeed_i2c_bus *bus = platform_get_drvdata(pdev);
+
+ i2c_del_adapter(&bus->adap);
+ return 0;
+}
+
+static const struct of_device_id aspeed_i2c_bus_of_table[] = {
+ { .compatible = "aspeed,ast2400-i2c-bus", },
+ { .compatible = "aspeed,ast2500-i2c-bus", },
+ { },
+};
+MODULE_DEVICE_TABLE(of, aspeed_i2c_bus_of_table);
+
+static struct platform_driver aspeed_i2c_bus_driver = {
+ .probe = aspeed_i2c_probe_bus,
+ .remove = aspeed_i2c_remove_bus,
+ .driver = {
+ .name = "ast-i2c-bus",
+ .of_match_table = aspeed_i2c_bus_of_table,
+ },
+};
+
+static void aspeed_i2c_controller_irq(struct irq_desc *desc)
+{
+ struct aspeed_i2c_controller *c = irq_desc_get_handler_data(desc);
+ unsigned long p, status;
+ unsigned int bus_irq;
+
+ status = readl(c->base);
+ for_each_set_bit(p, &status, ASPEED_I2C_NUM_BUS) {
+ bus_irq = irq_find_mapping(c->irq_domain, p);
+ generic_handle_irq(bus_irq);
+ }
+}
+
+static int aspeed_i2c_probe_controller(struct platform_device *pdev)
+{
+ struct aspeed_i2c_controller *controller;
+ struct device_node *np;
+ struct resource *res;
+
+ controller = kzalloc(sizeof(*controller), GFP_KERNEL);
+ if (!controller)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ controller->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(controller->base))
+ return PTR_ERR(controller->base);
+
+ controller->irq = platform_get_irq(pdev, 0);
+ if (controller->irq < 0) {
+ dev_err(&pdev->dev, "no platform IRQ\n");
+ return -ENXIO;
+ }
+
+ controller->irq_domain = irq_domain_add_linear(pdev->dev.of_node,
+ ASPEED_I2C_NUM_BUS, &irq_domain_simple_ops, NULL);
+ if (!controller->irq_domain) {
+ dev_err(&pdev->dev, "no IRQ domain\n");
+ return -ENXIO;
+ }
+ controller->irq_domain->name = "ast-i2c-domain";
+
+ irq_set_chained_handler_and_data(controller->irq,
+ aspeed_i2c_controller_irq, controller);
+
+ controller->dev = &pdev->dev;
+
+ platform_set_drvdata(pdev, controller);
+
+ dev_info(controller->dev, "i2c controller registered, irq %d\n",
+ controller->irq);
+
+ for_each_child_of_node(pdev->dev.of_node, np) {
+ int ret;
+ int bus_num;
+ char bus_id[sizeof("i2c-12345")];
+
+ /*
+ * Set a useful name derived from the bus number; the device
+ * tree should provide us with one that corresponds to the
+ * hardware numbering. If the property is missing the
+ * probe would fail so just skip it here.
+ */
+
+ bus_num = aspeed_i2c_bus_num(np);
+ if (bus_num < 0)
+ continue;
+
+ ret = snprintf(bus_id, sizeof(bus_id), "i2c-%d", bus_num);
+ if (ret >= sizeof(bus_id))
+ continue;
+
+ of_platform_device_create(np, bus_id, &pdev->dev);
+ of_node_put(np);
+ }
+
+ return 0;
+}
+
+static int aspeed_i2c_remove_controller(struct platform_device *pdev)
+{
+ struct aspeed_i2c_controller *controller = platform_get_drvdata(pdev);
+
+ irq_domain_remove(controller->irq_domain);
+ return 0;
+}
+
+static const struct of_device_id aspeed_i2c_controller_of_table[] = {
+ { .compatible = "aspeed,ast2400-i2c-controller", },
+ { .compatible = "aspeed,ast2500-i2c-controller", },
+ { },
+};
+MODULE_DEVICE_TABLE(of, aspeed_i2c_controller_of_table);
+
+static struct platform_driver aspeed_i2c_controller_driver = {
+ .probe = aspeed_i2c_probe_controller,
+ .remove = aspeed_i2c_remove_controller,
+ .driver = {
+ .name = "ast-i2c-controller",
+ .of_match_table = aspeed_i2c_controller_of_table,
+ },
+};
+
+static int __init aspeed_i2c_driver_init(void)
+{
+ int ret;
+
+ ret = platform_driver_register(&aspeed_i2c_controller_driver);
+ if (ret < 0)
+ return ret;
+ return platform_driver_register(&aspeed_i2c_bus_driver);
+}
+module_init(aspeed_i2c_driver_init);
+
+static void __exit aspeed_i2c_driver_exit(void)
+{
+ platform_driver_unregister(&aspeed_i2c_bus_driver);
+ platform_driver_unregister(&aspeed_i2c_controller_driver);
+}
+module_exit(aspeed_i2c_driver_exit);
+
+MODULE_AUTHOR("Brendan Higgins <brendanhiggins@google.com>");
+MODULE_DESCRIPTION("Aspeed I2C Bus Driver");
+MODULE_LICENSE("GPL");
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH v2 2/2] i2c: aspeed: added documentation for Aspeed I2C driver
From: Rob Herring @ 2016-09-19 21:35 UTC (permalink / raw)
To: Brendan Higgins
Cc: wsa-z923LK4zBo2bacvFa/9K2g, mark.rutland-5wv7dgnIgG8,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
openbmc-uLR06cmDAlY/bJ5BZ2RsiQ, joel-U3u1mxZcP9KHXe+LvDLADg,
jk-mnsaURCQ41sdnm+yROfE0A
In-Reply-To: <1473472551-11149-2-git-send-email-brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
On Fri, Sep 09, 2016 at 06:55:51PM -0700, Brendan Higgins wrote:
> Added device tree binding documentation for Aspeed I2C controller and
> busses.
>
> Signed-off-by: Brendan Higgins <brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> ---
> Changes for v2:
> - None
> ---
> .../devicetree/bindings/i2c/i2c-aspeed.txt | 63 ++++++++++++++++++++++
> 1 file changed, 63 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
> new file mode 100644
> index 0000000..df68f2a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
> @@ -0,0 +1,63 @@
> +Device tree configuration for the I2C controller and busses on the AST24XX
> +and AST25XX SoCs.
> +
> +Controller:
> +
> + Required Properties:
> + - #address-cells : should be 1
> + - #size-cells : should be 1
> + - #interrupt-cells : should be 1
> + - compatible : should be "aspeed,ast2400-i2c-controller"
> + or "aspeed,ast2500-i2c-controller"
> + - reg : address start and range of controller
> + - ranges : defines address offset and range for busses
> + - interrupts : interrupt number
> + - clocks : root clock of bus, should reference the APB
> + clock
> + - clock-ranges : specifies that child busses can inherit clocks
> + - interrupt-controller : denotes that the controller receives and fires
> + new interrupts for child busses
> +
> +Bus:
> +
> + Required Properties:
> + - #address-cells : should be 1
> + - #size-cells : should be 0
> + - reg : address offset and range of bus
> + - compatible : should be "aspeed,ast2400-i2c-bus"
> + or "aspeed,ast2500-i2c-bus"
> + - bus : the bus's number
Don't use indexes. The reg property is enough to id which bus is which.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 1/2] i2c: aspeed: added driver for Aspeed I2C
From: Brendan Higgins @ 2016-09-19 17:56 UTC (permalink / raw)
To: Wolfram Sang, robh+dt, mark.rutland
Cc: linux-i2c, devicetree, linux-kernel, OpenBMC Maillist,
Joel Stanley, Jeremy Kerr, Brendan Higgins
In-Reply-To: <1473472551-11149-1-git-send-email-brendanhiggins@google.com>
Gentle ping
On Fri, Sep 9, 2016 at 6:55 PM, Brendan Higgins
<brendanhiggins@google.com> wrote:
> Added initial master and slave support for Aspeed I2C controller.
> Supports fourteen busses present in ast24xx and ast25xx BMC SoCs by
> Aspeed.
>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---
> Changes for v2:
> - Added single module_init (multiple was breaking some builds).
> ---
> drivers/i2c/busses/Kconfig | 10 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-aspeed.c | 831 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 842 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-aspeed.c
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 5c3993b..0178c6c 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -998,6 +998,16 @@ config I2C_RCAR
> This driver can also be built as a module. If so, the module
> will be called i2c-rcar.
>
> +config I2C_ASPEED
> + tristate "Aspeed AST2xxx SoC I2C Controller"
> + depends on (ARCH_ASPEED || COMPILE_TEST) && OF
> + help
> + If you say yes to this option, support will be included for the
> + Aspeed AST2xxx SoC I2C controller.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-aspeed.
> +
> comment "External I2C/SMBus adapter drivers"
>
> config I2C_DIOLAN_U2C
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 37f2819..49631cd 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -96,6 +96,7 @@ obj-$(CONFIG_I2C_XILINX) += i2c-xiic.o
> obj-$(CONFIG_I2C_XLR) += i2c-xlr.o
> obj-$(CONFIG_I2C_XLP9XX) += i2c-xlp9xx.o
> obj-$(CONFIG_I2C_RCAR) += i2c-rcar.o
> +obj-$(CONFIG_I2C_ASPEED) += i2c-aspeed.o
>
> # External I2C/SMBus adapter drivers
> obj-$(CONFIG_I2C_DIOLAN_U2C) += i2c-diolan-u2c.o
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> new file mode 100644
> index 0000000..b19f13c
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -0,0 +1,831 @@
> +/*
> + * I2C adapter for the ASPEED I2C bus.
> + *
> + * Copyright (C) 2012-2020 ASPEED Technology Inc.
> + * Copyright 2016 IBM Corporation
> + * Copyright 2016 Google, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/errno.h>
> +#include <linux/interrupt.h>
> +#include <linux/completion.h>
> +#include <linux/slab.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/err.h>
> +#include <linux/clk.h>
> +
> +/* I2C Register */
> +#define ASPEED_I2C_FUN_CTRL_REG 0x00
> +#define ASPEED_I2C_AC_TIMING_REG1 0x04
> +#define ASPEED_I2C_AC_TIMING_REG2 0x08
> +#define ASPEED_I2C_INTR_CTRL_REG 0x0c
> +#define ASPEED_I2C_INTR_STS_REG 0x10
> +#define ASPEED_I2C_CMD_REG 0x14
> +#define ASPEED_I2C_DEV_ADDR_REG 0x18
> +#define ASPEED_I2C_BYTE_BUF_REG 0x20
> +
> +#define ASPEED_I2C_NUM_BUS 14
> +
> +/* Global Register Definition */
> +/* 0x00 : I2C Interrupt Status Register */
> +/* 0x08 : I2C Interrupt Target Assignment */
> +
> +/* Device Register Definition */
> +/* 0x00 : I2CD Function Control Register */
> +#define ASPEED_I2CD_MULTI_MASTER_DIS BIT(15)
> +#define ASPEED_I2CD_SDA_DRIVE_1T_EN BIT(8)
> +#define ASPEED_I2CD_M_SDA_DRIVE_1T_EN BIT(7)
> +#define ASPEED_I2CD_M_HIGH_SPEED_EN BIT(6)
> +#define ASPEED_I2CD_SLAVE_EN BIT(1)
> +#define ASPEED_I2CD_MASTER_EN BIT(0)
> +
> +/* 0x08 : I2CD Clock and AC Timing Control Register #2 */
> +#define ASPEED_NO_TIMEOUT_CTRL 0
> +
> +
> +/* 0x0c : I2CD Interrupt Control Register &
> + * 0x10 : I2CD Interrupt Status Register
> + *
> + * These share bit definitions, so use the same values for the enable &
> + * status bits.
> + */
> +#define ASPEED_I2CD_INTR_SDA_DL_TIMEOUT BIT(14)
> +#define ASPEED_I2CD_INTR_BUS_RECOVER_DONE BIT(13)
> +#define ASPEED_I2CD_INTR_SLAVE_MATCH BIT(7)
> +#define ASPEED_I2CD_INTR_SCL_TIMEOUT BIT(6)
> +#define ASPEED_I2CD_INTR_ABNORMAL BIT(5)
> +#define ASPEED_I2CD_INTR_NORMAL_STOP BIT(4)
> +#define ASPEED_I2CD_INTR_ARBIT_LOSS BIT(3)
> +#define ASPEED_I2CD_INTR_RX_DONE BIT(2)
> +#define ASPEED_I2CD_INTR_TX_NAK BIT(1)
> +#define ASPEED_I2CD_INTR_TX_ACK BIT(0)
> +
> +/* 0x14 : I2CD Command/Status Register */
> +#define ASPEED_I2CD_SCL_LINE_STS BIT(18)
> +#define ASPEED_I2CD_SDA_LINE_STS BIT(17)
> +#define ASPEED_I2CD_BUS_BUSY_STS BIT(16)
> +#define ASPEED_I2CD_BUS_RECOVER_CMD BIT(11)
> +
> +/* Command Bit */
> +#define ASPEED_I2CD_M_STOP_CMD BIT(5)
> +#define ASPEED_I2CD_M_S_RX_CMD_LAST BIT(4)
> +#define ASPEED_I2CD_M_RX_CMD BIT(3)
> +#define ASPEED_I2CD_S_TX_CMD BIT(2)
> +#define ASPEED_I2CD_M_TX_CMD BIT(1)
> +#define ASPEED_I2CD_M_START_CMD BIT(0)
> +
> +/* 0x18 : I2CD Slave Device Address Register */
> +#define ASPEED_I2CD_DEV_ADDR_MASK GENMASK(6, 0)
> +
> +enum aspeed_i2c_slave_state {
> + ASPEED_I2C_SLAVE_START,
> + ASPEED_I2C_SLAVE_READ_REQUESTED,
> + ASPEED_I2C_SLAVE_READ_PROCESSED,
> + ASPEED_I2C_SLAVE_WRITE_REQUESTED,
> + ASPEED_I2C_SLAVE_WRITE_RECEIVED,
> + ASPEED_I2C_SLAVE_STOP,
> +};
> +
> +struct aspeed_i2c_bus {
> + struct i2c_adapter adap;
> + struct device *dev;
> + void __iomem *base;
> + spinlock_t lock;
> + struct completion cmd_complete;
> + int irq;
> + /* Transaction state. */
> + struct i2c_msg *msg;
> + int msg_pos;
> + u32 cmd_err;
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> + struct i2c_client *slave;
> + enum aspeed_i2c_slave_state slave_state;
> +#endif
> +};
> +
> +struct aspeed_i2c_controller {
> + struct device *dev;
> + void __iomem *base;
> + int irq;
> + struct irq_domain *irq_domain;
> +};
> +
> +static inline void aspeed_i2c_write(struct aspeed_i2c_bus *bus, u32 val,
> + u32 reg)
> +{
> + writel(val, bus->base + reg);
> +}
> +
> +static inline u32 aspeed_i2c_read(struct aspeed_i2c_bus *bus, u32 reg)
> +{
> + return readl(bus->base + reg);
> +}
> +
> +static u8 aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
> +{
> + u32 command;
> + unsigned long time_left;
> + unsigned long flags;
> + int ret = 0;
> +
> + spin_lock_irqsave(&bus->lock, flags);
> + command = aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG);
> + /* Bus is idle: no recovery needed. */
> + if ((command & ASPEED_I2CD_SDA_LINE_STS) &&
> + (command & ASPEED_I2CD_SCL_LINE_STS))
> + goto out;
> +
> + dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n",
> + command);
> +
> + /* Bus held: put bus in stop state. */
> + if ((command & ASPEED_I2CD_SDA_LINE_STS) &&
> + !(command & ASPEED_I2CD_SCL_LINE_STS)) {
> + aspeed_i2c_write(bus, ASPEED_I2CD_M_STOP_CMD,
> + ASPEED_I2C_CMD_REG);
> + reinit_completion(&bus->cmd_complete);
> + spin_unlock_irqrestore(&bus->lock, flags);
> +
> + time_left = wait_for_completion_interruptible_timeout(
> + &bus->cmd_complete, bus->adap.timeout * HZ);
> +
> + spin_lock_irqsave(&bus->lock, flags);
> + if (time_left == 0)
> + ret = -ETIMEDOUT;
> + else if (bus->cmd_err)
> + ret = -EIO;
> + /* Bus error. */
> + } else if (!(command & ASPEED_I2CD_SDA_LINE_STS)) {
> + aspeed_i2c_write(bus, ASPEED_I2CD_BUS_RECOVER_CMD,
> + ASPEED_I2C_CMD_REG);
> + reinit_completion(&bus->cmd_complete);
> + spin_unlock_irqrestore(&bus->lock, flags);
> +
> + time_left = wait_for_completion_interruptible_timeout(
> + &bus->cmd_complete, bus->adap.timeout * HZ);
> +
> + spin_lock_irqsave(&bus->lock, flags);
> + if (time_left == 0)
> + ret = -ETIMEDOUT;
> + else if (bus->cmd_err)
> + ret = -EIO;
> + /* Recovery failed. */
> + else if (!(aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG) &
> + ASPEED_I2CD_SDA_LINE_STS))
> + ret = -EIO;
> + }
> +
> +out:
> + spin_unlock_irqrestore(&bus->lock, flags);
> + return ret;
> +}
> +
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> +{
> + bool irq_handled = true;
> + u32 command;
> + u32 irq_status;
> + u32 status_ack = 0;
> + u8 value;
> + struct i2c_client *slave = bus->slave;
> +
> + spin_lock(&bus->lock);
> + if (!slave) {
> + irq_handled = false;
> + goto out;
> + }
> + command = aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG);
> + irq_status = aspeed_i2c_read(bus, ASPEED_I2C_INTR_STS_REG);
> +
> + /* Slave was requested, restart state machine. */
> + if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
> + status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
> + bus->slave_state = ASPEED_I2C_SLAVE_START;
> + }
> + /* Slave is not currently active, irq was for someone else. */
> + if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
> + irq_handled = false;
> + goto out;
> + }
> +
> + dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
> + irq_status, command);
> +
> + /* Slave was sent something. */
> + if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
> + value = aspeed_i2c_read(bus, ASPEED_I2C_BYTE_BUF_REG) >> 8;
> + /* Handle address frame. */
> + if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
> + if (value & 0x1)
> + bus->slave_state =
> + ASPEED_I2C_SLAVE_READ_REQUESTED;
> + else
> + bus->slave_state =
> + ASPEED_I2C_SLAVE_WRITE_REQUESTED;
> + }
> + status_ack |= ASPEED_I2CD_INTR_RX_DONE;
> + }
> +
> + /* Slave was asked to stop. */
> + if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
> + status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
> + bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> + }
> + if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
> + status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> + bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> + }
> +
> + if (bus->slave_state == ASPEED_I2C_SLAVE_READ_REQUESTED) {
> + if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
> + dev_err(bus->dev, "Unexpected ACK on read request.\n");
> + bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
> +
> + i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
> + aspeed_i2c_write(bus, value, ASPEED_I2C_BYTE_BUF_REG);
> + aspeed_i2c_write(bus, ASPEED_I2CD_S_TX_CMD, ASPEED_I2C_CMD_REG);
> + } else if (bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) {
> + status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> + if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
> + dev_err(bus->dev,
> + "Expected ACK after processed read.\n");
> + i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value);
> + aspeed_i2c_write(bus, value, ASPEED_I2C_BYTE_BUF_REG);
> + aspeed_i2c_write(bus, ASPEED_I2CD_S_TX_CMD, ASPEED_I2C_CMD_REG);
> + } else if (bus->slave_state == ASPEED_I2C_SLAVE_WRITE_REQUESTED) {
> + bus->slave_state = ASPEED_I2C_SLAVE_WRITE_RECEIVED;
> + i2c_slave_event(slave, I2C_SLAVE_WRITE_REQUESTED, &value);
> + } else if (bus->slave_state == ASPEED_I2C_SLAVE_WRITE_RECEIVED) {
> + i2c_slave_event(slave, I2C_SLAVE_WRITE_RECEIVED, &value);
> + } else if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
> + i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
> + }
> +
> + if (status_ack != irq_status)
> + dev_err(bus->dev,
> + "irq handled != irq. expected %x, but was %x\n",
> + irq_status, status_ack);
> + aspeed_i2c_write(bus, status_ack, ASPEED_I2C_INTR_STS_REG);
> +
> +out:
> + spin_unlock(&bus->lock);
> + return irq_handled;
> +}
> +#endif
> +
> +static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> +{
> + const u32 errs = ASPEED_I2CD_INTR_ARBIT_LOSS |
> + ASPEED_I2CD_INTR_ABNORMAL |
> + ASPEED_I2CD_INTR_SCL_TIMEOUT |
> + ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |
> + ASPEED_I2CD_INTR_TX_NAK;
> + u32 irq_status;
> +
> + spin_lock(&bus->lock);
> + irq_status = aspeed_i2c_read(bus, ASPEED_I2C_INTR_STS_REG);
> + bus->cmd_err = irq_status & errs;
> +
> + dev_dbg(bus->dev, "master irq status 0x%08x\n", irq_status);
> +
> + /* No message to transfer. */
> + if (bus->cmd_err ||
> + (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) ||
> + (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE)) {
> + complete(&bus->cmd_complete);
> + goto out;
> + } else if (!bus->msg || bus->msg_pos >= bus->msg->len)
> + goto out;
> +
> + if ((bus->msg->flags & I2C_M_RD) &&
> + (irq_status & ASPEED_I2CD_INTR_RX_DONE)) {
> + bus->msg->buf[bus->msg_pos++] = aspeed_i2c_read(
> + bus, ASPEED_I2C_BYTE_BUF_REG) >> 8;
> + if (bus->msg_pos + 1 < bus->msg->len)
> + aspeed_i2c_write(bus, ASPEED_I2CD_M_RX_CMD,
> + ASPEED_I2C_CMD_REG);
> + else if (bus->msg_pos < bus->msg->len)
> + aspeed_i2c_write(bus, ASPEED_I2CD_M_RX_CMD |
> + ASPEED_I2CD_M_S_RX_CMD_LAST,
> + ASPEED_I2C_CMD_REG);
> + } else if (!(bus->msg->flags & I2C_M_RD) &&
> + (irq_status & ASPEED_I2CD_INTR_TX_ACK)) {
> + aspeed_i2c_write(bus, bus->msg->buf[bus->msg_pos++],
> + ASPEED_I2C_BYTE_BUF_REG);
> + aspeed_i2c_write(bus, ASPEED_I2CD_M_TX_CMD, ASPEED_I2C_CMD_REG);
> + }
> +
> + /* Transmission complete: notify caller. */
> + if (bus->msg_pos >= bus->msg->len)
> + complete(&bus->cmd_complete);
> +out:
> + aspeed_i2c_write(bus, irq_status, ASPEED_I2C_INTR_STS_REG);
> + spin_unlock(&bus->lock);
> + return true;
> +}
> +
> +static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> +{
> + struct aspeed_i2c_bus *bus = dev_id;
> +
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> + if (aspeed_i2c_slave_irq(bus)) {
> + dev_dbg(bus->dev, "irq handled by slave.\n");
> + return IRQ_HANDLED;
> + }
> +#endif
> + if (aspeed_i2c_master_irq(bus)) {
> + dev_dbg(bus->dev, "irq handled by master.\n");
> + return IRQ_HANDLED;
> + }
> + dev_err(bus->dev, "irq not handled properly!\n");
> + return IRQ_HANDLED;
> +}
> +
> +static int aspeed_i2c_master_single_xfer(struct i2c_adapter *adap,
> + struct i2c_msg *msg)
> +{
> + struct aspeed_i2c_bus *bus = adap->algo_data;
> + unsigned long flags;
> + u8 slave_addr;
> + u32 command = ASPEED_I2CD_M_START_CMD | ASPEED_I2CD_M_TX_CMD;
> + int ret = msg->len;
> + unsigned long time_left;
> +
> + spin_lock_irqsave(&bus->lock, flags);
> + bus->msg = msg;
> + bus->msg_pos = 0;
> + slave_addr = msg->addr << 1;
> + if (msg->flags & I2C_M_RD) {
> + slave_addr |= 1;
> + command |= ASPEED_I2CD_M_RX_CMD;
> + if (msg->len == 1)
> + command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
> + }
> + aspeed_i2c_write(bus, slave_addr, ASPEED_I2C_BYTE_BUF_REG);
> + aspeed_i2c_write(bus, command, ASPEED_I2C_CMD_REG);
> + reinit_completion(&bus->cmd_complete);
> + spin_unlock_irqrestore(&bus->lock, flags);
> +
> + time_left = wait_for_completion_interruptible_timeout(
> + &bus->cmd_complete, bus->adap.timeout * HZ * msg->len);
> + if (time_left == 0)
> + return -ETIMEDOUT;
> +
> + spin_lock_irqsave(&bus->lock, flags);
> + if (bus->cmd_err)
> + ret = -EIO;
> + bus->msg = NULL;
> + spin_unlock_irqrestore(&bus->lock, flags);
> +
> + return ret;
> +}
> +
> +static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
> + struct i2c_msg *msgs, int num)
> +{
> + struct aspeed_i2c_bus *bus = adap->algo_data;
> + int ret;
> + int i;
> + unsigned long flags;
> + unsigned long time_left;
> +
> + /* If bus is busy, attempt recovery. We assume a single master
> + * environment.
> + */
> + if (aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG) &
> + ASPEED_I2CD_BUS_BUSY_STS) {
> + ret = aspeed_i2c_recover_bus(bus);
> + if (ret)
> + return ret;
> + }
> +
> + for (i = 0; i < num; i++) {
> + ret = aspeed_i2c_master_single_xfer(adap, &msgs[i]);
> + if (ret < 0)
> + break;
> + /* TODO: Support other forms of I2C protocol mangling. */
> + if (msgs[i].flags & I2C_M_STOP) {
> + spin_lock_irqsave(&bus->lock, flags);
> + aspeed_i2c_write(bus, ASPEED_I2CD_M_STOP_CMD,
> + ASPEED_I2C_CMD_REG);
> + reinit_completion(&bus->cmd_complete);
> + spin_unlock_irqrestore(&bus->lock, flags);
> +
> + time_left = wait_for_completion_interruptible_timeout(
> + &bus->cmd_complete,
> + bus->adap.timeout * HZ);
> + if (time_left == 0)
> + return -ETIMEDOUT;
> + }
> + }
> +
> + spin_lock_irqsave(&bus->lock, flags);
> + aspeed_i2c_write(bus, ASPEED_I2CD_M_STOP_CMD, ASPEED_I2C_CMD_REG);
> + reinit_completion(&bus->cmd_complete);
> + spin_unlock_irqrestore(&bus->lock, flags);
> +
> + time_left = wait_for_completion_interruptible_timeout(
> + &bus->cmd_complete, bus->adap.timeout * HZ);
> + if (time_left == 0)
> + return -ETIMEDOUT;
> +
> + /* If nothing went wrong, return number of messages transferred. */
> + if (ret < 0)
> + return ret;
> + else
> + return i;
> +}
> +
> +static u32 aspeed_i2c_functionality(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA;
> +}
> +
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static int aspeed_i2c_reg_slave(struct i2c_client *client)
> +{
> + struct aspeed_i2c_bus *bus;
> + unsigned long flags;
> + u32 addr_reg_val;
> + u32 func_ctrl_reg_val;
> +
> + bus = client->adapter->algo_data;
> + spin_lock_irqsave(&bus->lock, flags);
> + if (bus->slave) {
> + spin_unlock_irqrestore(&bus->lock, flags);
> + return -EINVAL;
> + }
> +
> + /* Set slave addr. */
> + addr_reg_val = aspeed_i2c_read(bus, ASPEED_I2C_DEV_ADDR_REG);
> + addr_reg_val &= ~ASPEED_I2CD_DEV_ADDR_MASK;
> + addr_reg_val |= client->addr & ASPEED_I2CD_DEV_ADDR_MASK;
> + aspeed_i2c_write(bus, addr_reg_val, ASPEED_I2C_DEV_ADDR_REG);
> +
> + /* Switch from master mode to slave mode. */
> + func_ctrl_reg_val = aspeed_i2c_read(bus, ASPEED_I2C_FUN_CTRL_REG);
> + func_ctrl_reg_val &= ~ASPEED_I2CD_MASTER_EN;
> + func_ctrl_reg_val |= ASPEED_I2CD_SLAVE_EN;
> + aspeed_i2c_write(bus, func_ctrl_reg_val, ASPEED_I2C_FUN_CTRL_REG);
> +
> + bus->slave = client;
> + bus->slave_state = ASPEED_I2C_SLAVE_STOP;
> + spin_unlock_irqrestore(&bus->lock, flags);
> + return 0;
> +}
> +
> +static int aspeed_i2c_unreg_slave(struct i2c_client *client)
> +{
> + struct aspeed_i2c_bus *bus = client->adapter->algo_data;
> + unsigned long flags;
> + u32 func_ctrl_reg_val;
> +
> + spin_lock_irqsave(&bus->lock, flags);
> + if (!bus->slave) {
> + spin_unlock_irqrestore(&bus->lock, flags);
> + return -EINVAL;
> + }
> +
> + /* Switch from slave mode to master mode. */
> + func_ctrl_reg_val = aspeed_i2c_read(bus, ASPEED_I2C_FUN_CTRL_REG);
> + func_ctrl_reg_val &= ~ASPEED_I2CD_SLAVE_EN;
> + func_ctrl_reg_val |= ASPEED_I2CD_MASTER_EN;
> + aspeed_i2c_write(bus, func_ctrl_reg_val, ASPEED_I2C_FUN_CTRL_REG);
> +
> + bus->slave = NULL;
> + spin_unlock_irqrestore(&bus->lock, flags);
> + return 0;
> +}
> +#endif
> +
> +static const struct i2c_algorithm aspeed_i2c_algo = {
> + .master_xfer = aspeed_i2c_master_xfer,
> + .functionality = aspeed_i2c_functionality,
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> + .reg_slave = aspeed_i2c_reg_slave,
> + .unreg_slave = aspeed_i2c_unreg_slave,
> +#endif
> +};
> +
> +static u32 aspeed_i2c_get_clk_reg_val(u32 divider_ratio)
> +{
> + unsigned int inc = 0, div;
> + u32 scl_low, scl_high, data;
> +
> + for (div = 0; divider_ratio >= 16; div++) {
> + inc |= (divider_ratio & 1);
> + divider_ratio >>= 1;
> + }
> + divider_ratio += inc;
> + scl_low = (divider_ratio >> 1) - 1;
> + scl_high = divider_ratio - scl_low - 2;
> + data = 0x77700300 | (scl_high << 16) | (scl_low << 12) | div;
> + return data;
> +}
> +
> +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
> + struct platform_device *pdev)
> +{
> + struct clk *pclk;
> + u32 clk_freq;
> + u32 divider_ratio;
> + int ret;
> +
> + pclk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(pclk)) {
> + dev_err(&pdev->dev, "clk_get failed\n");
> + return PTR_ERR(pclk);
> + }
> + ret = of_property_read_u32(pdev->dev.of_node,
> + "clock-frequency", &clk_freq);
> + if (ret < 0) {
> + dev_err(&pdev->dev,
> + "Could not read clock-frequency property\n");
> + clk_freq = 100000;
> + }
> + divider_ratio = clk_get_rate(pclk) / clk_freq;
> + /* We just need the clock rate, we don't actually use the clk object. */
> + devm_clk_put(&pdev->dev, pclk);
> +
> + /* Set AC Timing */
> + if (clk_freq / 1000 > 400) {
> + aspeed_i2c_write(bus, aspeed_i2c_read(bus,
> + ASPEED_I2C_FUN_CTRL_REG) |
> + ASPEED_I2CD_M_HIGH_SPEED_EN |
> + ASPEED_I2CD_M_SDA_DRIVE_1T_EN |
> + ASPEED_I2CD_SDA_DRIVE_1T_EN,
> + ASPEED_I2C_FUN_CTRL_REG);
> +
> + aspeed_i2c_write(bus, 0x3, ASPEED_I2C_AC_TIMING_REG2);
> + aspeed_i2c_write(bus, aspeed_i2c_get_clk_reg_val(divider_ratio),
> + ASPEED_I2C_AC_TIMING_REG1);
> + } else {
> + aspeed_i2c_write(bus, aspeed_i2c_get_clk_reg_val(divider_ratio),
> + ASPEED_I2C_AC_TIMING_REG1);
> + aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,
> + ASPEED_I2C_AC_TIMING_REG2);
> + }
> +
> + return 0;
> +}
> +
> +static void noop(struct irq_data *data) { }
> +
> +static struct irq_chip aspeed_i2c_irqchip = {
> + .name = "ast-i2c",
> + .irq_unmask = noop,
> + .irq_mask = noop,
> +};
> +
> +static int aspeed_i2c_probe_bus(struct platform_device *pdev)
> +{
> + struct aspeed_i2c_bus *bus;
> + struct aspeed_i2c_controller *controller =
> + dev_get_drvdata(pdev->dev.parent);
> + struct resource *res;
> + int ret, bus_num, irq;
> +
> + bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
> + if (!bus)
> + return -ENOMEM;
> +
> + ret = of_property_read_u32(pdev->dev.of_node, "bus", &bus_num);
> + if (ret)
> + return -ENXIO;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + bus->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(bus->base))
> + return PTR_ERR(bus->base);
> +
> + bus->irq = platform_get_irq(pdev, 0);
> + if (bus->irq < 0) {
> + dev_err(&pdev->dev, "platform_get_irq failed\n");
> + return -ENXIO;
> + }
> +
> + irq = irq_create_mapping(controller->irq_domain, bus_num);
> + irq_set_chip_data(irq, controller);
> + irq_set_chip_and_handler(irq, &aspeed_i2c_irqchip, handle_simple_irq);
> + ret = devm_request_irq(&pdev->dev, bus->irq, aspeed_i2c_bus_irq,
> + 0, dev_name(&pdev->dev), bus);
> + if (ret) {
> + dev_err(&pdev->dev, "devm_request_irq failed\n");
> + return -ENXIO;
> + }
> +
> + /* Initialize the I2C adapter */
> + spin_lock_init(&bus->lock);
> + init_completion(&bus->cmd_complete);
> + bus->adap.nr = bus_num;
> + bus->adap.owner = THIS_MODULE;
> + bus->adap.retries = 0;
> + bus->adap.timeout = 5;
> + bus->adap.algo = &aspeed_i2c_algo;
> + bus->adap.algo_data = bus;
> + bus->adap.dev.parent = &pdev->dev;
> + bus->adap.dev.of_node = pdev->dev.of_node;
> + snprintf(bus->adap.name, sizeof(bus->adap.name), "Aspeed i2c-%d",
> + bus_num);
> +
> + bus->dev = &pdev->dev;
> +
> + /* reset device: disable master & slave functions */
> + aspeed_i2c_write(bus, 0, ASPEED_I2C_FUN_CTRL_REG);
> +
> + ret = aspeed_i2c_init_clk(bus, pdev);
> + if (ret < 0)
> + return ret;
> +
> + /* Enable Master Mode */
> + aspeed_i2c_write(bus, aspeed_i2c_read(bus, ASPEED_I2C_FUN_CTRL_REG) |
> + ASPEED_I2CD_MASTER_EN |
> + ASPEED_I2CD_MULTI_MASTER_DIS, ASPEED_I2C_FUN_CTRL_REG);
> +
> + /* Set interrupt generation of I2C controller */
> + aspeed_i2c_write(bus, ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |
> + ASPEED_I2CD_INTR_BUS_RECOVER_DONE |
> + ASPEED_I2CD_INTR_SCL_TIMEOUT |
> + ASPEED_I2CD_INTR_ABNORMAL |
> + ASPEED_I2CD_INTR_NORMAL_STOP |
> + ASPEED_I2CD_INTR_ARBIT_LOSS |
> + ASPEED_I2CD_INTR_RX_DONE |
> + ASPEED_I2CD_INTR_TX_NAK |
> + ASPEED_I2CD_INTR_TX_ACK,
> + ASPEED_I2C_INTR_CTRL_REG);
> +
> + ret = i2c_add_numbered_adapter(&bus->adap);
> + if (ret < 0)
> + return -ENXIO;
> +
> + platform_set_drvdata(pdev, bus);
> +
> + dev_info(bus->dev, "i2c bus %d registered, irq %d\n",
> + bus->adap.nr, bus->irq);
> +
> + return 0;
> +}
> +
> +static int aspeed_i2c_remove_bus(struct platform_device *pdev)
> +{
> + struct aspeed_i2c_bus *bus = platform_get_drvdata(pdev);
> +
> + i2c_del_adapter(&bus->adap);
> + return 0;
> +}
> +
> +static const struct of_device_id aspeed_i2c_bus_of_table[] = {
> + { .compatible = "aspeed,ast2400-i2c-bus", },
> + { .compatible = "aspeed,ast2500-i2c-bus", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_i2c_bus_of_table);
> +
> +static struct platform_driver aspeed_i2c_bus_driver = {
> + .probe = aspeed_i2c_probe_bus,
> + .remove = aspeed_i2c_remove_bus,
> + .driver = {
> + .name = "ast-i2c-bus",
> + .of_match_table = aspeed_i2c_bus_of_table,
> + },
> +};
> +
> +static void aspeed_i2c_controller_irq(struct irq_desc *desc)
> +{
> + struct aspeed_i2c_controller *c = irq_desc_get_handler_data(desc);
> + unsigned long p, status;
> + unsigned int bus_irq;
> +
> + status = readl(c->base);
> + for_each_set_bit(p, &status, ASPEED_I2C_NUM_BUS) {
> + bus_irq = irq_find_mapping(c->irq_domain, p);
> + generic_handle_irq(bus_irq);
> + }
> +}
> +
> +static int aspeed_i2c_probe_controller(struct platform_device *pdev)
> +{
> + struct aspeed_i2c_controller *controller;
> + struct device_node *np;
> + struct resource *res;
> +
> + controller = kzalloc(sizeof(*controller), GFP_KERNEL);
> + if (!controller)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + controller->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(controller->base))
> + return PTR_ERR(controller->base);
> +
> + controller->irq = platform_get_irq(pdev, 0);
> + if (controller->irq < 0) {
> + dev_err(&pdev->dev, "no platform IRQ\n");
> + return -ENXIO;
> + }
> +
> + controller->irq_domain = irq_domain_add_linear(pdev->dev.of_node,
> + ASPEED_I2C_NUM_BUS, &irq_domain_simple_ops, NULL);
> + if (!controller->irq_domain) {
> + dev_err(&pdev->dev, "no IRQ domain\n");
> + return -ENXIO;
> + }
> + controller->irq_domain->name = "ast-i2c-domain";
> +
> + irq_set_chained_handler_and_data(controller->irq,
> + aspeed_i2c_controller_irq, controller);
> +
> + controller->dev = &pdev->dev;
> +
> + platform_set_drvdata(pdev, controller);
> +
> + dev_info(controller->dev, "i2c controller registered, irq %d\n",
> + controller->irq);
> +
> + for_each_child_of_node(pdev->dev.of_node, np) {
> + int ret;
> + u32 bus_num;
> + char bus_id[sizeof("i2c-12345")];
> +
> + /*
> + * Set a useful name derived from the bus number; the device
> + * tree should provide us with one that corresponds to the
> + * hardware numbering. If the property is missing the
> + * probe would fail so just skip it here.
> + */
> +
> + ret = of_property_read_u32(np, "bus", &bus_num);
> + if (ret)
> + continue;
> +
> + ret = snprintf(bus_id, sizeof(bus_id), "i2c-%u", bus_num);
> + if (ret >= sizeof(bus_id))
> + continue;
> +
> + of_platform_device_create(np, bus_id, &pdev->dev);
> + of_node_put(np);
> + }
> +
> + return 0;
> +}
> +
> +static int aspeed_i2c_remove_controller(struct platform_device *pdev)
> +{
> + struct aspeed_i2c_controller *controller = platform_get_drvdata(pdev);
> +
> + irq_domain_remove(controller->irq_domain);
> + return 0;
> +}
> +
> +static const struct of_device_id aspeed_i2c_controller_of_table[] = {
> + { .compatible = "aspeed,ast2400-i2c-controller", },
> + { .compatible = "aspeed,ast2500-i2c-controller", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_i2c_controller_of_table);
> +
> +static struct platform_driver aspeed_i2c_controller_driver = {
> + .probe = aspeed_i2c_probe_controller,
> + .remove = aspeed_i2c_remove_controller,
> + .driver = {
> + .name = "ast-i2c-controller",
> + .of_match_table = aspeed_i2c_controller_of_table,
> + },
> +};
> +
> +static int __init aspeed_i2c_driver_init(void)
> +{
> + int ret;
> +
> + ret = platform_driver_register(&aspeed_i2c_controller_driver);
> + if (ret < 0)
> + return ret;
> + return platform_driver_register(&aspeed_i2c_bus_driver);
> +}
> +module_init(aspeed_i2c_driver_init);
> +
> +static void __exit aspeed_i2c_driver_exit(void)
> +{
> + platform_driver_unregister(&aspeed_i2c_bus_driver);
> + platform_driver_unregister(&aspeed_i2c_controller_driver);
> +}
> +module_exit(aspeed_i2c_driver_exit);
> +
> +MODULE_AUTHOR("Brendan Higgins <brendanhiggins@google.com>");
> +MODULE_DESCRIPTION("Aspeed I2C Bus Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.8.0.rc3.226.g39d4020
>
^ permalink raw reply
* Re: [PATCH 1/3] i2c: bcm2835: Fix hang for writing messages larger than 16 bytes
From: Noralf Trønnes @ 2016-09-19 17:36 UTC (permalink / raw)
To: Eric Anholt, wsa, swarren
Cc: linux-kernel, linux-i2c, linux-arm-kernel, linux-rpi-kernel
In-Reply-To: <87zin3omsa.fsf@eliezer.anholt.net>
Den 19.09.2016 18:51, skrev Eric Anholt:
> Noralf Trønnes <noralf@tronnes.org> writes:
>
>> 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>
> Patches 1, 3 are:
>
> Reviewed-by: Eric Anholt <eric@anholt.net>
>
> For patch 2 I followed some of it, but couldn't quite say it's a review.
> I trust your testing, and that the path has had a lot more testing in
> the downstream tree, so it's:
I don't know how much testing downstream really has had since this feature
is disabled by default. So there is a possibility that this can break some
devices (while at the same time enable others).
I wondered about adding a module parameter so it could be disabled at
runtime, but decided to see what the review would be first.
It is of course possible to add a this module parameter later should it
turn out to break some device.
Noralf.
> Acked-by: Eric Anholt <eric@anholt.net>
>
> Thanks!
^ permalink raw reply
* Re: [PATCH 1/3] i2c: bcm2835: Fix hang for writing messages larger than 16 bytes
From: Eric Anholt @ 2016-09-19 16:51 UTC (permalink / raw)
To: wsa, swarren
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>
[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]
Noralf Trønnes <noralf@tronnes.org> writes:
> 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>
Patches 1, 3 are:
Reviewed-by: Eric Anholt <eric@anholt.net>
For patch 2 I followed some of it, but couldn't quite say it's a review.
I trust your testing, and that the path has had a lot more testing in
the downstream tree, so it's:
Acked-by: Eric Anholt <eric@anholt.net>
Thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]
^ permalink raw reply
* [PATCH 3/3] i2c: bcm2835: Use ratelimited logging on transfer errors
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>
Writing to an AT24C32 generates on average 2x i2c transfer errors per
32-byte page write. Which amounts to a lot for a 4k write. This is due
to the fact that the chip doesn't respond during it's internal write
cycle when the at24 driver tries and retries the next write.
Reduce this flooding of the log by using dev_err_ratelimited().
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/i2c/busses/i2c-bcm2835.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index b3ce565..dc0a5d5 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -274,7 +274,8 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
(msg->flags & I2C_M_IGNORE_NAK))
return 0;
- dev_err(i2c_dev->dev, "i2c transfer failed: %x\n", i2c_dev->msg_err);
+ dev_err_ratelimited(i2c_dev->dev, "i2c transfer failed: %x\n",
+ i2c_dev->msg_err);
if (i2c_dev->msg_err & BCM2835_I2C_S_ERR)
return -EREMOTEIO;
--
2.8.2
^ permalink raw reply related
* [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
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