* Re: [PATCH v5 3/6] Documentation: Add sbs-manager device tree node documentation
From: Phil Reid @ 2016-09-30 7:58 UTC (permalink / raw)
To: Rob Herring
Cc: wsa, mark.rutland, sre, andrea.merello, karl-heinz, arnd,
linux-i2c, devicetree, linux-pm
In-Reply-To: <20160923194434.GA5771@rob-hp-laptop>
G'day Rob,
On 24/09/2016 03:44, Rob Herring wrote:
> On Wed, Sep 21, 2016 at 04:41:11PM +0800, Phil Reid wrote:
>> From: Karl-Heinz Schneider <karl-heinz@schneider-inet.de>
>>
>> This patch adds device tree documentation for the sbs-manager
>>
>> Reviewed-by: Phil Reid <preid@electromag.com.au>
>> Signed-off-by: Karl-Heinz Schneider <karl-heinz@schneider-inet.de>
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>> ---
>> .../bindings/power/supply/sbs,sbs-manager.txt | 62 ++++++++++++++++++++++
>> 1 file changed, 62 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/power/supply/sbs,sbs-manager.txt
>>
>> diff --git a/Documentation/devicetree/bindings/power/supply/sbs,sbs-manager.txt b/Documentation/devicetree/bindings/power/supply/sbs,sbs-manager.txt
>> new file mode 100644
>> index 0000000..2a04b30
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/supply/sbs,sbs-manager.txt
>> @@ -0,0 +1,62 @@
>> +Binding for sbs-manager
>> +
>> +Required properties:
>> +- compatible: should be "lltc,ltc1760" or use "sbs,sbs-manager" as fallback.
>> +- reg: integer, i2c address of the device. Should be <0xa>.
>> +Optional properties:
>> +- gpio-controller: Marks the port as GPIO controller.
>> + See "gpio-specifier" in .../devicetree/bindings/gpio/gpio.txt.
>> +- #gpio-cells: Should be <2>. The first cell is the pin number, the second cell
>> + is used to specify optional parameters:
>> + See "gpio-specifier" in .../devicetree/bindings/gpio/gpio.txt.
>
> Is this leftover from the last version? It doesn't look like you are
> using any GPIOs in the example.
Sorry didn't update the example. I still need the gpio exposed to the battery.
>
>> +
>> +From OS view the device is basically an i2c-mux used to communicate with up to
>> +four smart battery devices at address 0xb. The driver actually implements this
>> +behaviour. So standard i2c-mux nodes can be used to register up to four slave
>> +batteries. See Documentation/devicetree/bindings/i2c/i2c-mux.txt for more
>> +information on i2c-mux nodes. Channels will be numerated starting from 1 to 4.
>> +
>> +Example:
>> +
>> +batman@0a {
>> + compatible = "lltc,ltc1760";
>> + reg = <0x0a>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + gpio-controller;
>> + #gpio-cells = <2>;
>> +
>> + i2c@1 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + reg = <1>;
>> +
>> + battery@b {
>> + compatible = "ti,bq2060", "sbs,sbs-battery";
>> + reg = <0xb>;
should have something like:
sbs,battery-detect-gpios = <&batman 0 GPIO_ACTIVE_HIGH>;
>> + };
>> + };
>> +
>> + i2c@2 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + reg = <2>;
>> +
>> + battery@b {
>> + compatible = "ti,bq2060", "sbs,sbs-battery";
>> + reg = <0xb>;
>> + };
>> + };
>> +
>> + i2c@3 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + reg = <3>;
>> +
>> + battery@b {
>> + compatible = "ti,bq2060", "sbs,sbs-battery";
>> + reg = <0xb>;
>> + };
>> + };
>> +};
>> --
Let me know your thoughts.
--
Regards
Phil Reid
^ permalink raw reply
* Re: [PATCH v5 1/6] I2C: i2c-smbus: add device tree support
From: Phil Reid @ 2016-09-30 7:55 UTC (permalink / raw)
To: Rob Herring
Cc: wsa, mark.rutland, sre, andrea.merello, karl-heinz, arnd,
linux-i2c, devicetree, linux-pm
In-Reply-To: <20160923194213.GA24328@rob-hp-laptop>
G'day Rob,
On 24/09/2016 03:42, Rob Herring wrote:
> On Wed, Sep 21, 2016 at 04:41:09PM +0800, Phil Reid wrote:
>> From: Andrea Merello <andrea.merello@gmail.com>
>>
>> According to Documentation/i2c/smbus-protocol, a smbus controller driver
>> that wants to hook-in smbus extensions support, can call
>> i2c_setup_smbus_alert(). There are very few drivers that are currently
>> doing this.
>>
>> However the i2c-smbus module can also work with any
>> smbus-extensions-unaware I2C controller, as long as we provide an extra
>> IRQ line connected to the I2C bus ALARM signal.
>>
>> This patch makes it possible to go this way via DT. Note that the DT node
>> will indeed describe a (little) piece of HW, that is the routing of the
>> ALARM signal to an IRQ line (so it seems a fair DT use to me, but RFC).
>>
>> Note that AFAICT, by design, i2c-smbus module pretends to be an I2C slave
>> with address 0x0C (that is the alarm response address), and IMHO this is
>> quite consistent with usage in the DT as a I2C child node.
>>
>> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>> ---
>> Documentation/devicetree/bindings/i2c/i2c-smbus.txt | 20 ++++++++++++++++++++
>> drivers/i2c/i2c-smbus.c | 20 +++++++++++++++-----
>> 2 files changed, 35 insertions(+), 5 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-smbus.txt
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-smbus.txt b/Documentation/devicetree/bindings/i2c/i2c-smbus.txt
>> new file mode 100644
>> index 0000000..da83127
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-smbus.txt
>> @@ -0,0 +1,20 @@
>> +* i2c-smbus extensions
>> +
>> +Required Properties:
>> + - compatible: Must contain "smbus_alert"
>> + - interrupts: The irq line for smbus ALERT signal
>> + - reg: I2C slave address. Set to 0x0C (alert response address).
>
> This is not right. The 0xC address is special, not an actual device
> address. The bindings should just have the actual device's compatible
> string and address.
>
>> +
>> +Note: The i2c-smbus module registers itself as a slave I2C device. Whenever
>> +a smbus controller directly support smbus extensions (and its driver supports
>> +this), there is no need to add anything special to the DT. Otherwise, for using
>> +i2c-smbus with any smbus-extensions-unaware I2C controllers, you need to
>> +route the smbus ALARM signal to an extra IRQ line, thus you need to describe
>> +this in the DT.
>
> Now, I guess what you need in the kernel is a common handler for
> SMBALERT# and to know which interrupt line is SMBALERT#.
>
> The drivers should know this. A given h/w device will or will not handle
> the "SMB Alert Response Address". So the drivers should register their
> interrupt with the I2C/SMBus core.
>
> If a controller handles the SMBALERT, then it should make itself an
> interrupt controller and that's what slave devices 'interrupts' property
> will point to.
>
Could a property be added to each i2c bus segment.
Something like the following.
interrupt-parent = <&irqsrc>;
interrupts = <0>;
interrupt-names = "smbalert";
Then the i2c core code could look for this in each segment, either in the parent segment
or a mux segment and create the smbalert common handler for each segment as required.
--
Regards
Phil Reid
^ permalink raw reply
* Re: [PATCH] i2c: designware: Avoid aborted transfers with fast reacting I2C slaves
From: Jukka Laitinen @ 2016-09-30 7:05 UTC (permalink / raw)
To: Jarkko Nikula, linux-i2c; +Cc: Wolfram Sang, Andy Shevchenko, Mika Westerberg
In-Reply-To: <20160929130459.11345-1-jarkko.nikula@linux.intel.com>
On 29.09.2016 16:04, Jarkko Nikula wrote:
> By the specification SDA RX hold time extends incoming SDA low to high
> transition by n * ic_clk cycles but only when SCL is high. However it
> seems to help avoid above faulty arbitration lost error.
>
> Bits 23:16 in IC_SDA_HOLD register define the SDA RX hold time for the
> receiver. Be conservative and enable 1 ic_clk cycle long hold time in
> case boot firmware hasn't set it up.
>
> Reported-by: Jukka Laitinen <jukka.laitinen@intel.com>
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> ---
> drivers/i2c/busses/i2c-designware-core.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index 1fe93c43215c..11e866d05368 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -95,6 +95,9 @@
> #define DW_IC_STATUS_TFE BIT(2)
> #define DW_IC_STATUS_MST_ACTIVITY BIT(5)
>
> +#define DW_IC_SDA_HOLD_RX_SHIFT 16
> +#define DW_IC_SDA_HOLD_RX_MASK GENMASK(23, DW_IC_SDA_HOLD_RX_SHIFT)
> +
> #define DW_IC_ERR_TX_ABRT 0x1
>
> #define DW_IC_TAR_10BITADDR_MASTER BIT(12)
> @@ -420,12 +423,20 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
> /* Configure SDA Hold Time if required */
> reg = dw_readl(dev, DW_IC_COMP_VERSION);
> if (reg >= DW_IC_SDA_HOLD_MIN_VERS) {
> - if (dev->sda_hold_time) {
> - dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
> - } else {
> + if (!dev->sda_hold_time) {
> /* Keep previous hold time setting if no one set it */
> dev->sda_hold_time = dw_readl(dev, DW_IC_SDA_HOLD);
> }
> + /*
> + * Workaround for avoiding TX arbitration lost in case I2C
> + * slave pulls SDA down "too quickly" after falling egde of
> + * SCL by enabling non-zero SDA RX hold. Specification says it
> + * extends incoming SDA low to high transition while SCL is
> + * high but it apprears to help also above issue.
> + */
> + if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK))
> + dev->sda_hold_time |= 1 << DW_IC_SDA_HOLD_RX_SHIFT;
> + dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
> } else {
> dev_warn(dev->dev,
> "Hardware too old to adjust SDA hold time.\n");
>
Tested-by: Jukka Laitinen <jukka.laitinen@intel.com>
Tested with kabylake cpu and Ubuntu 16.04 + kernel 4.8-rc6 + ubuntu
sauce from http://kernel.ubuntu.com/~kernel-ppa/mainline/v4.8-rc6/
Regards,
Jukka Laitinen
^ permalink raw reply
* [PATCH] i2c: designware: Avoid aborted transfers with fast reacting I2C slaves
From: Jarkko Nikula @ 2016-09-29 13:04 UTC (permalink / raw)
To: linux-i2c
Cc: Wolfram Sang, Andy Shevchenko, Mika Westerberg, Jukka Laitinen,
Jarkko Nikula
I2C DesignWare may abort transfer with arbitration lost if I2C slave pulls
SDA down quickly after falling edge of SCL. Reason for this is unknown but
after trial and error it was found this can be avoided by enabling non-zero
SDA RX hold time for the receiver.
By the specification SDA RX hold time extends incoming SDA low to high
transition by n * ic_clk cycles but only when SCL is high. However it
seems to help avoid above faulty arbitration lost error.
Bits 23:16 in IC_SDA_HOLD register define the SDA RX hold time for the
receiver. Be conservative and enable 1 ic_clk cycle long hold time in
case boot firmware hasn't set it up.
Reported-by: Jukka Laitinen <jukka.laitinen@intel.com>
Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
drivers/i2c/busses/i2c-designware-core.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 1fe93c43215c..11e866d05368 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -95,6 +95,9 @@
#define DW_IC_STATUS_TFE BIT(2)
#define DW_IC_STATUS_MST_ACTIVITY BIT(5)
+#define DW_IC_SDA_HOLD_RX_SHIFT 16
+#define DW_IC_SDA_HOLD_RX_MASK GENMASK(23, DW_IC_SDA_HOLD_RX_SHIFT)
+
#define DW_IC_ERR_TX_ABRT 0x1
#define DW_IC_TAR_10BITADDR_MASTER BIT(12)
@@ -420,12 +423,20 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
/* Configure SDA Hold Time if required */
reg = dw_readl(dev, DW_IC_COMP_VERSION);
if (reg >= DW_IC_SDA_HOLD_MIN_VERS) {
- if (dev->sda_hold_time) {
- dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
- } else {
+ if (!dev->sda_hold_time) {
/* Keep previous hold time setting if no one set it */
dev->sda_hold_time = dw_readl(dev, DW_IC_SDA_HOLD);
}
+ /*
+ * Workaround for avoiding TX arbitration lost in case I2C
+ * slave pulls SDA down "too quickly" after falling egde of
+ * SCL by enabling non-zero SDA RX hold. Specification says it
+ * extends incoming SDA low to high transition while SCL is
+ * high but it apprears to help also above issue.
+ */
+ if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK))
+ dev->sda_hold_time |= 1 << DW_IC_SDA_HOLD_RX_SHIFT;
+ dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
} else {
dev_warn(dev->dev,
"Hardware too old to adjust SDA hold time.\n");
--
2.9.3
^ permalink raw reply related
* Re: [PATCH v3 3/7] i2c: bcm2835: Use dev_dbg logging on transfer errors
From: Martin Sperl @ 2016-09-29 11:05 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: <1475085056-5205-4-git-send-email-noralf@tronnes.org>
On 28.09.2016, at 19:50, Noralf Trønnes <noralf@tronnes.org> wrote:
>
> 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.
> Only a handful drivers use dev_err() on transfer error, so switch to
> dev_dbg() instead.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Martin Sperl <kernel@martin.sperl.org>
^ permalink raw reply
* Re: [PATCH v2 1/2] Documentation: tpm: add the IBM Virtual TPM device tree binding documentation
From: Jarkko Sakkinen @ 2016-09-29 11:04 UTC (permalink / raw)
To: Nayna Jain
Cc: devicetree, tpmdd-devel, robh, wsa, pawel.moll, mark.rutland,
ijc+devicetree, galak, linux-i2c, peterhuewe, tpmdd, hellerda,
ltcgcw, cclaudio, honclo
In-Reply-To: <1475051441-23008-1-git-send-email-nayna@linux.vnet.ibm.com>
On Wed, Sep 28, 2016 at 04:30:40AM -0400, Nayna Jain wrote:
> Virtual TPM, which is being used on IBM POWER7+ and POWER8 systems running
> POWERVM, is currently supported by tpm device driver but lacks the
> documentation. This patch adds the missing documentation for the existing
> support.
>
> Suggested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
> ---
> Changelog v2:
>
> - New Patch
>
> .../devicetree/bindings/security/tpm/ibmvtpm.txt | 41 ++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/security/tpm/ibmvtpm.txt
>
> diff --git a/Documentation/devicetree/bindings/security/tpm/ibmvtpm.txt b/Documentation/devicetree/bindings/security/tpm/ibmvtpm.txt
> new file mode 100644
> index 0000000..d89f999
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/security/tpm/ibmvtpm.txt
> @@ -0,0 +1,41 @@
> +* Device Tree Bindings for IBM Virtual Trusted Platform Module(vtpm)
> +
> +Required properties:
> +
> +- compatible : property name that conveys the platform architecture
> + identifiers, as 'IBM,vtpm'
> +- device_type : specifies type of virtual device
A generic device tree question. What is the difference between
these fields? Why the I2C one does have 'device_type'?
/Jarkko
^ permalink raw reply
* Re: [PATCH] gpio: pca953x: add a comment explaining the need for a lockdep subclass
From: Wolfram Sang @ 2016-09-29 8:04 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Linus Walleij, Alexandre Courbot, Andy Shevchenko, Vignesh R,
Yong Li, Geert Uytterhoeven, Peter Zijlstra, Ingo Molnar,
Peter Rosin, linux-i2c, linux-gpio, LKML
In-Reply-To: <CAMpxmJWmzju-gJ54irsqGb86i6THx4BXzqMAa=okqGAi=5bbrA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]
On Thu, Sep 29, 2016 at 09:56:58AM +0200, Bartosz Golaszewski wrote:
> 2016-09-29 8:06 GMT+02:00 Wolfram Sang <wsa@the-dreams.de>:
> > On Mon, Sep 26, 2016 at 12:00:30PM +0200, Wolfram Sang wrote:
> >> On Mon, Sep 26, 2016 at 11:54:15AM +0200, Bartosz Golaszewski wrote:
> >> > This is a follow-up to commit 559b46990e76 ("gpio: pca953x: fix an
> >> > incorrect lockdep warning"). The reason for calling
> >> > lockdep_set_subclass() in pca953x_probe() is not explained in
> >> > the code.
> >> >
> >> > Add a comment describing the problem, partial solution and required
> >> > future extensions.
> >> >
> >> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >>
> >> Acked-by: Wolfram Sang <wsa@the-dreams.de>
> >>
> >> Linus, because of dependencies, I should probably pick it up?
> >
> > Linus, ping!
> >
>
> Hi Wolfram,
>
> this patch will not apply to the gpio tree. If Linus is too busy to
> comment, maybe you could pick it up anyway - it doesn't change
> anything in terms of functionality and it's better to have the code
> commented when it's not obvious what it does.
I want to pick it up and am likely to do so anyway, but to be formally
perfect, I need an ack from Linus.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v3 7/7] i2c: bcm2835: Add support for dynamic clock
From: Martin Sperl @ 2016-09-29 8:02 UTC (permalink / raw)
To: Eric Anholt, Noralf Trønnes, wsa, swarren
Cc: linux-rpi-kernel, linux-kernel, linux-arm-kernel, linux-i2c
In-Reply-To: <87r383g1kb.fsf@eliezer.anholt.net>
On 28.09.2016 23:24, Eric Anholt wrote:
> Noralf Trønnes <noralf@tronnes.org> writes:
>
>> Support a dynamic clock by reading the frequency and setting the
>> divisor in the transfer function instead of during probe.
> Is this fixing some particular case you could note in the commit
> message? As is, it makes me think that we should be using a notifier
> for when the parent clock (that's the one I assume you're talking about
> being dynamic) changes. But maybe you're working around a variable VPU
> clock being set by the firmware, because we don't have a notifier for
> it?
>
> I'm a bit worried because I think this is going to be pretty expensive
> to be doing per transfer.
Well, the clocks are all configured without CLK_GET_RATE_NOCACHE et. al.,
so the value is read from cache, which is not (very) expensive
(see clk_core_get_rate).
This also means that any clock change of the VPU done by the firmware
does not propagate to the linux kernel anyway and the unchanged
cached value is returned.
To make this work it would require a notification mechanism from the
firmware to trigger a re-validation of all the caches. (or some sort of
watchdog
process).
Adding a notifier to each driver (I2C, SPI) instead is - imo - a lot of
unnecessary
code complexity, as any currently running transfer would still be impacted,
because changing the clock-divider in flight is a asking for trouble.
But then changing the vpu-clock speed while a I2s/SPI/... transfer is
running is
also asking for trouble....
The only place where - IMO - a notifier would make sense is with the
auxiliar
UART driver(8250_bcm2835aux.c), as there we only read the clock rates
when setting/changing the baud rate. But - again - this would require some
notification by the firmware in the first place and any reception in the
window of change would go wrong because of unexpected effective baud
rate changes.
So as far as I can tell the change to read the current clock rate in the
transfer function is a reasonable approach and the clock framework should
handle the communication with the firmware about such changes.
(And I remember having had some discussions around this subject
with Phil Elwell or popcornmix some time ago on github where it boiled
down to: what is the "right" interface? - I can not find the reference
right now)
Reviewed-by: Martin Sperl <kernel@martin.sperl.org>
Thanks, Martin
^ permalink raw reply
* Re: [PATCH] gpio: pca953x: add a comment explaining the need for a lockdep subclass
From: Bartosz Golaszewski @ 2016-09-29 7:56 UTC (permalink / raw)
To: Wolfram Sang
Cc: Linus Walleij, Alexandre Courbot, Andy Shevchenko, Vignesh R,
Yong Li, Geert Uytterhoeven, Peter Zijlstra, Ingo Molnar,
Peter Rosin, linux-i2c, linux-gpio, LKML
In-Reply-To: <20160929060638.GB1524@katana>
2016-09-29 8:06 GMT+02:00 Wolfram Sang <wsa@the-dreams.de>:
> On Mon, Sep 26, 2016 at 12:00:30PM +0200, Wolfram Sang wrote:
>> On Mon, Sep 26, 2016 at 11:54:15AM +0200, Bartosz Golaszewski wrote:
>> > This is a follow-up to commit 559b46990e76 ("gpio: pca953x: fix an
>> > incorrect lockdep warning"). The reason for calling
>> > lockdep_set_subclass() in pca953x_probe() is not explained in
>> > the code.
>> >
>> > Add a comment describing the problem, partial solution and required
>> > future extensions.
>> >
>> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>
>> Acked-by: Wolfram Sang <wsa@the-dreams.de>
>>
>> Linus, because of dependencies, I should probably pick it up?
>
> Linus, ping!
>
Hi Wolfram,
this patch will not apply to the gpio tree. If Linus is too busy to
comment, maybe you could pick it up anyway - it doesn't change
anything in terms of functionality and it's better to have the code
commented when it's not obvious what it does.
Thanks,
Bartosz
^ permalink raw reply
* Re: [PATCH] gpio: pca953x: add a comment explaining the need for a lockdep subclass
From: Wolfram Sang @ 2016-09-29 6:06 UTC (permalink / raw)
To: Bartosz Golaszewski, Linus Walleij
Cc: Alexandre Courbot, Andy Shevchenko, Vignesh R, Yong Li,
Geert Uytterhoeven, Peter Zijlstra, Ingo Molnar, Peter Rosin,
linux-i2c, linux-gpio, LKML
In-Reply-To: <20160926100030.GA5606@katana>
[-- Attachment #1: Type: text/plain, Size: 660 bytes --]
On Mon, Sep 26, 2016 at 12:00:30PM +0200, Wolfram Sang wrote:
> On Mon, Sep 26, 2016 at 11:54:15AM +0200, Bartosz Golaszewski wrote:
> > This is a follow-up to commit 559b46990e76 ("gpio: pca953x: fix an
> > incorrect lockdep warning"). The reason for calling
> > lockdep_set_subclass() in pca953x_probe() is not explained in
> > the code.
> >
> > Add a comment describing the problem, partial solution and required
> > future extensions.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Acked-by: Wolfram Sang <wsa@the-dreams.de>
>
> Linus, because of dependencies, I should probably pick it up?
Linus, ping!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts
From: Stefan Wahren @ 2016-09-29 5:37 UTC (permalink / raw)
To: Eric Anholt, wsa, swarren, Noralf Trønnes
Cc: linux-kernel, linux-i2c, linux-rpi-kernel, linux-arm-kernel
In-Reply-To: <d93a0590-1783-b913-9362-007628cb17d6@tronnes.org>
> Noralf Trønnes <noralf@tronnes.org> hat am 29. September 2016 um 00:22
> geschrieben:
>
>
>
> Den 29.09.2016 00:00, skrev Eric Anholt:
> > Noralf Trønnes <noralf@tronnes.org> writes:
> >
> >> If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
> >> the driver has no way to fill/drain the FIFO to stop the interrupts.
> >> In this case the controller has to be disabled and the transfer
> >> completed to avoid hang.
> >>
> >> (CLKT | ERR) and DONE interrupts are completed in their own paths, and
> >> the controller is disabled in the transfer function after completion.
> >> Unite the code paths and do disabling inside the interrupt routine.
> >>
> >> Clear interrupt status bits in the united completion path instead of
> >> trying to do it on every interrupt which isn't necessary.
> >> Only CLKT, ERR and DONE can be cleared that way.
> >>
> >> Add the status value to the error value in case of TXW/RXR errors to
> >> distinguish them from the other S_LEN error.
> > I was surprised that not writing the TXW/RXR bits on handling their
> > interrupts was OK, given that we were doing so before, but it's a level
> > interrupt and those bits are basically ignored on write.
> >
> > This patch and 3, 4, and 6 are:
> >
> > Reviewed-by: Eric Anholt <eric@anholt.net>
> >
> > Patch 5 is:
> >
> > Acked-by: Eric Anholt <eric@anholt.net>
> >
> > Note for future debug: The I2C_C_CLEAR on errors will take some time to
> > resolve -- if you were in non-idle state and I2C_C_READ, it sets an
> > abort_rx flag and runs through the state machine to send a NACK and a
> > STOP, I think. Since we're setting CLEAR without I2CEN, that NACK will
> > be hanging around queued up for next time we start the engine.
>
> Maybe you're able to explain the issues I had with reset:
> https://github.com/raspberrypi/linux/issues/1653
>
> Should we put your note into the commit message?
> It will most likely be lost if it just stays in this email.
I prefer to have this kind of information as a code comment.
>
> Noralf.
>
>
> _______________________________________________
> 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 v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts
From: Noralf Trønnes @ 2016-09-28 22:22 UTC (permalink / raw)
To: Eric Anholt, wsa, swarren
Cc: linux-kernel, linux-i2c, linux-arm-kernel, linux-rpi-kernel
In-Reply-To: <87oa37fzx6.fsf@eliezer.anholt.net>
Den 29.09.2016 00:00, skrev Eric Anholt:
> Noralf Trønnes <noralf@tronnes.org> writes:
>
>> If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
>> the driver has no way to fill/drain the FIFO to stop the interrupts.
>> In this case the controller has to be disabled and the transfer
>> completed to avoid hang.
>>
>> (CLKT | ERR) and DONE interrupts are completed in their own paths, and
>> the controller is disabled in the transfer function after completion.
>> Unite the code paths and do disabling inside the interrupt routine.
>>
>> Clear interrupt status bits in the united completion path instead of
>> trying to do it on every interrupt which isn't necessary.
>> Only CLKT, ERR and DONE can be cleared that way.
>>
>> Add the status value to the error value in case of TXW/RXR errors to
>> distinguish them from the other S_LEN error.
> I was surprised that not writing the TXW/RXR bits on handling their
> interrupts was OK, given that we were doing so before, but it's a level
> interrupt and those bits are basically ignored on write.
>
> This patch and 3, 4, and 6 are:
>
> Reviewed-by: Eric Anholt <eric@anholt.net>
>
> Patch 5 is:
>
> Acked-by: Eric Anholt <eric@anholt.net>
>
> Note for future debug: The I2C_C_CLEAR on errors will take some time to
> resolve -- if you were in non-idle state and I2C_C_READ, it sets an
> abort_rx flag and runs through the state machine to send a NACK and a
> STOP, I think. Since we're setting CLEAR without I2CEN, that NACK will
> be hanging around queued up for next time we start the engine.
Maybe you're able to explain the issues I had with reset:
https://github.com/raspberrypi/linux/issues/1653
Should we put your note into the commit message?
It will most likely be lost if it just stays in this email.
Noralf.
^ permalink raw reply
* Re: [PATCH v3 7/7] i2c: bcm2835: Add support for dynamic clock
From: Noralf Trønnes @ 2016-09-28 22:10 UTC (permalink / raw)
To: Eric Anholt, wsa, swarren
Cc: linux-kernel, linux-i2c, linux-arm-kernel, linux-rpi-kernel
In-Reply-To: <87r383g1kb.fsf@eliezer.anholt.net>
Den 28.09.2016 23:24, skrev Eric Anholt:
> Noralf Trønnes <noralf@tronnes.org> writes:
>
>> Support a dynamic clock by reading the frequency and setting the
>> divisor in the transfer function instead of during probe.
> Is this fixing some particular case you could note in the commit
> message? As is, it makes me think that we should be using a notifier
> for when the parent clock (that's the one I assume you're talking about
> being dynamic) changes. But maybe you're working around a variable VPU
> clock being set by the firmware, because we don't have a notifier for
> it?
>
> I'm a bit worried because I think this is going to be pretty expensive
> to be doing per transfer.
Actually I thought that this was a known problem, but trying to track
it down now, I can't find anything definitive. I don't have any tool
to look at the bus signals, so I couldn't actually test it.
Sorry about this, we can just drop this patch.
Noralf.
^ permalink raw reply
* Re: [PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts
From: Eric Anholt @ 2016-09-28 22:00 UTC (permalink / raw)
To: wsa, swarren
Cc: linux-kernel, linux-i2c, linux-arm-kernel, linux-rpi-kernel,
Noralf Trønnes
In-Reply-To: <1475085056-5205-3-git-send-email-noralf@tronnes.org>
[-- Attachment #1: Type: text/plain, Size: 1594 bytes --]
Noralf Trønnes <noralf@tronnes.org> writes:
> If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
> the driver has no way to fill/drain the FIFO to stop the interrupts.
> In this case the controller has to be disabled and the transfer
> completed to avoid hang.
>
> (CLKT | ERR) and DONE interrupts are completed in their own paths, and
> the controller is disabled in the transfer function after completion.
> Unite the code paths and do disabling inside the interrupt routine.
>
> Clear interrupt status bits in the united completion path instead of
> trying to do it on every interrupt which isn't necessary.
> Only CLKT, ERR and DONE can be cleared that way.
>
> Add the status value to the error value in case of TXW/RXR errors to
> distinguish them from the other S_LEN error.
I was surprised that not writing the TXW/RXR bits on handling their
interrupts was OK, given that we were doing so before, but it's a level
interrupt and those bits are basically ignored on write.
This patch and 3, 4, and 6 are:
Reviewed-by: Eric Anholt <eric@anholt.net>
Patch 5 is:
Acked-by: Eric Anholt <eric@anholt.net>
Note for future debug: The I2C_C_CLEAR on errors will take some time to
resolve -- if you were in non-idle state and I2C_C_READ, it sets an
abort_rx flag and runs through the state machine to send a NACK and a
STOP, I think. Since we're setting CLEAR without I2CEN, that NACK will
be hanging around queued up for next time we start the engine.
Patch 7 I had questions about but probably will send an ack when you
reply.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]
^ permalink raw reply
* Re: [PATCH v3 7/7] i2c: bcm2835: Add support for dynamic clock
From: Eric Anholt @ 2016-09-28 21:24 UTC (permalink / raw)
To: wsa, swarren
Cc: linux-kernel, linux-i2c, linux-arm-kernel, linux-rpi-kernel,
Noralf Trønnes
In-Reply-To: <1475085056-5205-8-git-send-email-noralf@tronnes.org>
[-- Attachment #1: Type: text/plain, Size: 639 bytes --]
Noralf Trønnes <noralf@tronnes.org> writes:
> Support a dynamic clock by reading the frequency and setting the
> divisor in the transfer function instead of during probe.
Is this fixing some particular case you could note in the commit
message? As is, it makes me think that we should be using a notifier
for when the parent clock (that's the one I assume you're talking about
being dynamic) changes. But maybe you're working around a variable VPU
clock being set by the firmware, because we don't have a notifier for
it?
I'm a bit worried because I think this is going to be pretty expensive
to be doing per transfer.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]
^ permalink raw reply
* Courier was unable to deliver the parcel, ID000762941
From: FedEx Standard Overnight @ 2016-09-28 19:36 UTC (permalink / raw)
To: linux-i2c
[-- Attachment #1: Type: text/plain, Size: 176 bytes --]
Dear Customer,
This is to confirm that one or more of your parcels has been shipped.
Shipment Label is attached to email.
Sincerely,
Jeffery Russell,
FedEx Support Manager.
[-- Attachment #2: Delivery_Notification_000762941.zip --]
[-- Type: application/zip, Size: 5616 bytes --]
^ permalink raw reply
* [PATCH v3 5/7] i2c: bcm2835: Add support for Repeated Start Condition
From: Noralf Trønnes @ 2016-09-28 17:50 UTC (permalink / raw)
To: wsa, swarren, eric
Cc: linux-kernel, linux-i2c, linux-arm-kernel, linux-rpi-kernel,
Noralf Trønnes
In-Reply-To: <1475085056-5205-1-git-send-email-noralf@tronnes.org>
Documentation/i2c/i2c-protocol states that Combined transactions should
separate messages with a Start bit and end the whole transaction with a
Stop bit. This patch adds support for issuing only a Start between
messages instead of a Stop followed by a Start.
This implementation differs from downstream i2c-bcm2708 in 2 respects:
- it uses an interrupt to detect that the transfer is active instead
of using polling. There is no interrupt for Transfer Active, but by
not prefilling the FIFO it's possible to use the TXW interrupt.
- when resetting/disabling the controller between transfers it writes
CLEAR to the control register instead of just zero.
Using just zero gave many errors. This might be the reason why
downstream had to disable this feature and make it available with a
module parameter.
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 | 101 ++++++++++++++++++++++++---------------
1 file changed, 63 insertions(+), 38 deletions(-)
diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index 99857f8..d42fee2 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -63,6 +63,7 @@ struct bcm2835_i2c_dev {
struct i2c_adapter adapter;
struct completion completion;
struct i2c_msg *curr_msg;
+ int num_msgs;
u32 msg_err;
u8 *msg_buf;
size_t msg_buf_remaining;
@@ -109,6 +110,45 @@ static void bcm2835_drain_rxfifo(struct bcm2835_i2c_dev *i2c_dev)
}
}
+/*
+ * 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, poll the I2CS.TA flag and wait for it to be set, and then
+ * issue a read.
+ * A comment in https://github.com/raspberrypi/linux/issues/254 shows how the
+ * firmware actually does it using polling and says that it's a workaround for
+ * a problem in the state machine.
+ * It turns out that it is possible to use the TXW interrupt to know when the
+ * transfer is active, provided the FIFO has not been prefilled.
+ */
+
+static void bcm2835_i2c_start_transfer(struct bcm2835_i2c_dev *i2c_dev)
+{
+ u32 c = BCM2835_I2C_C_ST | BCM2835_I2C_C_I2CEN;
+ struct i2c_msg *msg = i2c_dev->curr_msg;
+ bool last_msg = (i2c_dev->num_msgs == 1);
+
+ if (!i2c_dev->num_msgs)
+ return;
+
+ i2c_dev->num_msgs--;
+ i2c_dev->msg_buf = msg->buf;
+ i2c_dev->msg_buf_remaining = msg->len;
+
+ if (msg->flags & I2C_M_RD)
+ c |= BCM2835_I2C_C_READ | BCM2835_I2C_C_INTR;
+ else
+ c |= BCM2835_I2C_C_INTT;
+
+ if (last_msg)
+ c |= BCM2835_I2C_C_INTD;
+
+ 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);
+}
+
static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
{
struct bcm2835_i2c_dev *i2c_dev = data;
@@ -142,6 +182,12 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
}
bcm2835_fill_txfifo(i2c_dev);
+
+ if (i2c_dev->num_msgs && !i2c_dev->msg_buf_remaining) {
+ i2c_dev->curr_msg++;
+ bcm2835_i2c_start_transfer(i2c_dev);
+ }
+
return IRQ_HANDLED;
}
@@ -166,30 +212,25 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
return IRQ_HANDLED;
}
-static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
- struct i2c_msg *msg)
+static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
+ int num)
{
- u32 c;
+ struct bcm2835_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
unsigned long time_left;
+ int i;
- i2c_dev->curr_msg = msg;
- i2c_dev->msg_buf = msg->buf;
- i2c_dev->msg_buf_remaining = msg->len;
- reinit_completion(&i2c_dev->completion);
-
- bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, BCM2835_I2C_C_CLEAR);
+ for (i = 0; i < (num - 1); i++)
+ if (msgs[i].flags & I2C_M_RD) {
+ dev_warn_once(i2c_dev->dev,
+ "only one read message supported, has to be last\n");
+ return -EOPNOTSUPP;
+ }
- if (msg->flags & I2C_M_RD) {
- c = BCM2835_I2C_C_READ | BCM2835_I2C_C_INTR;
- } else {
- c = BCM2835_I2C_C_INTT;
- bcm2835_fill_txfifo(i2c_dev);
- }
- c |= BCM2835_I2C_C_ST | BCM2835_I2C_C_INTD | BCM2835_I2C_C_I2CEN;
+ i2c_dev->curr_msg = msgs;
+ i2c_dev->num_msgs = num;
+ reinit_completion(&i2c_dev->completion);
- 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);
+ bcm2835_i2c_start_transfer(i2c_dev);
time_left = wait_for_completion_timeout(&i2c_dev->completion,
BCM2835_I2C_TIMEOUT);
@@ -200,31 +241,15 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
return -ETIMEDOUT;
}
- if (likely(!i2c_dev->msg_err))
- return 0;
+ if (!i2c_dev->msg_err)
+ return num;
dev_dbg(i2c_dev->dev, "i2c transfer failed: %x\n", i2c_dev->msg_err);
if (i2c_dev->msg_err & BCM2835_I2C_S_ERR)
return -EREMOTEIO;
- else
- return -EIO;
-}
-
-static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
- int num)
-{
- struct bcm2835_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
- int i;
- int ret = 0;
-
- for (i = 0; i < num; i++) {
- ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i]);
- if (ret)
- break;
- }
- return ret ?: i;
+ return -EIO;
}
static u32 bcm2835_i2c_func(struct i2c_adapter *adap)
--
2.8.2
^ permalink raw reply related
* [PATCH v3 7/7] i2c: bcm2835: Add support for dynamic clock
From: Noralf Trønnes @ 2016-09-28 17:50 UTC (permalink / raw)
To: wsa, swarren, eric
Cc: linux-kernel, linux-i2c, linux-arm-kernel, linux-rpi-kernel,
Noralf Trønnes
In-Reply-To: <1475085056-5205-1-git-send-email-noralf@tronnes.org>
Support a dynamic clock by reading the frequency and setting the
divisor in the transfer function instead of during probe.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/i2c/busses/i2c-bcm2835.c | 51 +++++++++++++++++++++++++---------------
1 file changed, 32 insertions(+), 19 deletions(-)
diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index 6a3e753..2b6b682 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -58,6 +58,7 @@ struct bcm2835_i2c_dev {
void __iomem *regs;
struct clk *clk;
int irq;
+ u32 bus_clk_rate;
struct i2c_adapter adapter;
struct completion completion;
struct i2c_msg *curr_msg;
@@ -78,6 +79,30 @@ static inline u32 bcm2835_i2c_readl(struct bcm2835_i2c_dev *i2c_dev, u32 reg)
return readl(i2c_dev->regs + reg);
}
+static int bcm2835_i2c_set_divider(struct bcm2835_i2c_dev *i2c_dev)
+{
+ u32 divider;
+
+ divider = DIV_ROUND_UP(clk_get_rate(i2c_dev->clk),
+ i2c_dev->bus_clk_rate);
+ /*
+ * Per the datasheet, the register is always interpreted as an even
+ * number, by rounding down. In other words, the LSB is ignored. So,
+ * if the LSB is set, increment the divider to avoid any issue.
+ */
+ if (divider & 1)
+ divider++;
+ if ((divider < BCM2835_I2C_CDIV_MIN) ||
+ (divider > BCM2835_I2C_CDIV_MAX)) {
+ dev_err_ratelimited(i2c_dev->dev, "Invalid clock-frequency\n");
+ return -EINVAL;
+ }
+
+ bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DIV, divider);
+
+ return 0;
+}
+
static void bcm2835_fill_txfifo(struct bcm2835_i2c_dev *i2c_dev)
{
u32 val;
@@ -215,7 +240,7 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
{
struct bcm2835_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
unsigned long time_left;
- int i;
+ int i, ret;
for (i = 0; i < (num - 1); i++)
if (msgs[i].flags & I2C_M_RD) {
@@ -224,6 +249,10 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
return -EOPNOTSUPP;
}
+ ret = bcm2835_i2c_set_divider(i2c_dev);
+ if (ret)
+ return ret;
+
i2c_dev->curr_msg = msgs;
i2c_dev->num_msgs = num;
reinit_completion(&i2c_dev->completion);
@@ -273,7 +302,6 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
{
struct bcm2835_i2c_dev *i2c_dev;
struct resource *mem, *irq;
- u32 bus_clk_rate, divider;
int ret;
struct i2c_adapter *adap;
@@ -297,27 +325,12 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
}
ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency",
- &bus_clk_rate);
+ &i2c_dev->bus_clk_rate);
if (ret < 0) {
dev_warn(&pdev->dev,
"Could not read clock-frequency property\n");
- bus_clk_rate = 100000;
- }
-
- divider = DIV_ROUND_UP(clk_get_rate(i2c_dev->clk), bus_clk_rate);
- /*
- * Per the datasheet, the register is always interpreted as an even
- * number, by rounding down. In other words, the LSB is ignored. So,
- * if the LSB is set, increment the divider to avoid any issue.
- */
- if (divider & 1)
- divider++;
- if ((divider < BCM2835_I2C_CDIV_MIN) ||
- (divider > BCM2835_I2C_CDIV_MAX)) {
- dev_err(&pdev->dev, "Invalid clock-frequency\n");
- return -ENODEV;
+ i2c_dev->bus_clk_rate = 100000;
}
- bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DIV, divider);
irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
if (!irq) {
--
2.8.2
^ permalink raw reply related
* [PATCH v3 6/7] i2c: bcm2835: Support i2c-dev ioctl I2C_TIMEOUT
From: Noralf Trønnes @ 2016-09-28 17:50 UTC (permalink / raw)
To: wsa, swarren, eric
Cc: linux-kernel, linux-i2c, linux-arm-kernel, linux-rpi-kernel,
Noralf Trønnes
In-Reply-To: <1475085056-5205-1-git-send-email-noralf@tronnes.org>
Use i2c_adapter->timeout for the completion timeout value. The core
default is 1 second.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/i2c/busses/i2c-bcm2835.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index d42fee2..6a3e753 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -53,8 +53,6 @@
#define BCM2835_I2C_CDIV_MIN 0x0002
#define BCM2835_I2C_CDIV_MAX 0xFFFE
-#define BCM2835_I2C_TIMEOUT (msecs_to_jiffies(1000))
-
struct bcm2835_i2c_dev {
struct device *dev;
void __iomem *regs;
@@ -233,7 +231,7 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
bcm2835_i2c_start_transfer(i2c_dev);
time_left = wait_for_completion_timeout(&i2c_dev->completion,
- BCM2835_I2C_TIMEOUT);
+ adap->timeout);
if (!time_left) {
bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C,
BCM2835_I2C_C_CLEAR);
--
2.8.2
^ permalink raw reply related
* [PATCH v3 4/7] i2c: bcm2835: Can't support I2C_M_IGNORE_NAK
From: Noralf Trønnes @ 2016-09-28 17:50 UTC (permalink / raw)
To: wsa, swarren, eric
Cc: linux-kernel, linux-i2c, linux-arm-kernel, linux-rpi-kernel,
Noralf Trønnes
In-Reply-To: <1475085056-5205-1-git-send-email-noralf@tronnes.org>
The controller can't support this flag, so remove it.
Documentation/i2c/i2c-protocol states that all of the message is sent:
I2C_M_IGNORE_NAK:
Normally message is interrupted immediately if there is [NA] from the
client. Setting this flag treats any [NA] as [A], and all of
message is sent.
>From the BCM2835 ARM Peripherals datasheet:
The ERR field is set when the slave fails to acknowledge either
its address or a data byte written to it.
So when the controller doesn't receive an ack, it sets ERR and raises
an interrupt. In other words, the whole message is not sent.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/i2c/busses/i2c-bcm2835.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index 8cdb139..99857f8 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -203,10 +203,6 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
if (likely(!i2c_dev->msg_err))
return 0;
- if ((i2c_dev->msg_err & BCM2835_I2C_S_ERR) &&
- (msg->flags & I2C_M_IGNORE_NAK))
- return 0;
-
dev_dbg(i2c_dev->dev, "i2c transfer failed: %x\n", i2c_dev->msg_err);
if (i2c_dev->msg_err & BCM2835_I2C_S_ERR)
--
2.8.2
^ permalink raw reply related
* [PATCH v3 3/7] i2c: bcm2835: Use dev_dbg logging on transfer errors
From: Noralf Trønnes @ 2016-09-28 17:50 UTC (permalink / raw)
To: wsa, swarren, eric
Cc: linux-kernel, linux-i2c, linux-arm-kernel, linux-rpi-kernel,
Noralf Trønnes
In-Reply-To: <1475085056-5205-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.
Only a handful drivers use dev_err() on transfer error, so switch to
dev_dbg() instead.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
Changes:
- use dev_dbg instead of dev_err_ratelimited
drivers/i2c/busses/i2c-bcm2835.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index df036ed..8cdb139 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -207,7 +207,7 @@ 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_dbg(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 v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts
From: Noralf Trønnes @ 2016-09-28 17:50 UTC (permalink / raw)
To: wsa, swarren, eric
Cc: linux-kernel, linux-i2c, linux-arm-kernel, linux-rpi-kernel,
Noralf Trønnes
In-Reply-To: <1475085056-5205-1-git-send-email-noralf@tronnes.org>
If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
the driver has no way to fill/drain the FIFO to stop the interrupts.
In this case the controller has to be disabled and the transfer
completed to avoid hang.
(CLKT | ERR) and DONE interrupts are completed in their own paths, and
the controller is disabled in the transfer function after completion.
Unite the code paths and do disabling inside the interrupt routine.
Clear interrupt status bits in the united completion path instead of
trying to do it on every interrupt which isn't necessary.
Only CLKT, ERR and DONE can be cleared that way.
Add the status value to the error value in case of TXW/RXR errors to
distinguish them from the other S_LEN error.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/i2c/busses/i2c-bcm2835.c | 31 ++++++++++++++++++++++---------
1 file changed, 22 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index f283b71..df036ed 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -50,8 +50,6 @@
#define BCM2835_I2C_S_CLKT BIT(9)
#define BCM2835_I2C_S_LEN BIT(10) /* Fake bit for SW error reporting */
-#define BCM2835_I2C_BITMSK_S 0x03FF
-
#define BCM2835_I2C_CDIV_MIN 0x0002
#define BCM2835_I2C_CDIV_MAX 0xFFFE
@@ -117,14 +115,11 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
u32 val, err;
val = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S);
- val &= BCM2835_I2C_BITMSK_S;
- bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_S, val);
err = val & (BCM2835_I2C_S_CLKT | BCM2835_I2C_S_ERR);
if (err) {
i2c_dev->msg_err = err;
- complete(&i2c_dev->completion);
- return IRQ_HANDLED;
+ goto complete;
}
if (val & BCM2835_I2C_S_DONE) {
@@ -137,21 +132,38 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
i2c_dev->msg_err = BCM2835_I2C_S_LEN;
else
i2c_dev->msg_err = 0;
- complete(&i2c_dev->completion);
- return IRQ_HANDLED;
+ goto complete;
}
if (val & BCM2835_I2C_S_TXW) {
+ if (!i2c_dev->msg_buf_remaining) {
+ i2c_dev->msg_err = val | BCM2835_I2C_S_LEN;
+ goto complete;
+ }
+
bcm2835_fill_txfifo(i2c_dev);
return IRQ_HANDLED;
}
if (val & BCM2835_I2C_S_RXR) {
+ if (!i2c_dev->msg_buf_remaining) {
+ i2c_dev->msg_err = val | BCM2835_I2C_S_LEN;
+ goto complete;
+ }
+
bcm2835_drain_rxfifo(i2c_dev);
return IRQ_HANDLED;
}
return IRQ_NONE;
+
+complete:
+ 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);
+ complete(&i2c_dev->completion);
+
+ return IRQ_HANDLED;
}
static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
@@ -181,8 +193,9 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
time_left = wait_for_completion_timeout(&i2c_dev->completion,
BCM2835_I2C_TIMEOUT);
- bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, BCM2835_I2C_C_CLEAR);
if (!time_left) {
+ bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C,
+ BCM2835_I2C_C_CLEAR);
dev_err(i2c_dev->dev, "i2c transfer timed out\n");
return -ETIMEDOUT;
}
--
2.8.2
^ permalink raw reply related
* [PATCH v3 1/7] i2c: bcm2835: Fix hang for writing messages larger than 16 bytes
From: Noralf Trønnes @ 2016-09-28 17:50 UTC (permalink / raw)
To: wsa, swarren, eric
Cc: linux-kernel, linux-i2c, linux-arm-kernel, linux-rpi-kernel,
Noralf Trønnes
In-Reply-To: <1475085056-5205-1-git-send-email-noralf@tronnes.org>
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>
Reviewed-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Martin Sperl <kernel@martin.sperl.org>
---
drivers/i2c/busses/i2c-bcm2835.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index d4f3239..f283b71 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -64,6 +64,7 @@ struct bcm2835_i2c_dev {
int irq;
struct i2c_adapter adapter;
struct completion completion;
+ struct i2c_msg *curr_msg;
u32 msg_err;
u8 *msg_buf;
size_t msg_buf_remaining;
@@ -126,14 +127,13 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
return IRQ_HANDLED;
}
- if (val & BCM2835_I2C_S_RXD) {
- bcm2835_drain_rxfifo(i2c_dev);
- if (!(val & BCM2835_I2C_S_DONE))
- return IRQ_HANDLED;
- }
-
if (val & BCM2835_I2C_S_DONE) {
- if (i2c_dev->msg_buf_remaining)
+ if (i2c_dev->curr_msg->flags & I2C_M_RD) {
+ bcm2835_drain_rxfifo(i2c_dev);
+ val = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S);
+ }
+
+ if ((val & BCM2835_I2C_S_RXD) || i2c_dev->msg_buf_remaining)
i2c_dev->msg_err = BCM2835_I2C_S_LEN;
else
i2c_dev->msg_err = 0;
@@ -141,11 +141,16 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
return IRQ_HANDLED;
}
- if (val & BCM2835_I2C_S_TXD) {
+ if (val & BCM2835_I2C_S_TXW) {
bcm2835_fill_txfifo(i2c_dev);
return IRQ_HANDLED;
}
+ if (val & BCM2835_I2C_S_RXR) {
+ bcm2835_drain_rxfifo(i2c_dev);
+ return IRQ_HANDLED;
+ }
+
return IRQ_NONE;
}
@@ -155,6 +160,7 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
u32 c;
unsigned long time_left;
+ i2c_dev->curr_msg = msg;
i2c_dev->msg_buf = msg->buf;
i2c_dev->msg_buf_remaining = msg->len;
reinit_completion(&i2c_dev->completion);
--
2.8.2
^ permalink raw reply related
* [PATCH v3 0/7] i2c: bcm2835: Bring in changes from downstream
From: Noralf Trønnes @ 2016-09-28 17:50 UTC (permalink / raw)
To: wsa, swarren, eric
Cc: linux-kernel, linux-i2c, linux-arm-kernel, linux-rpi-kernel,
Noralf Trønnes
This patchset tries to bring in the lessons learned in the downstream
driver i2c-bcm2708. The downstream clock stretcing timeout patch has
been left out since clock stretching is broken/unreliable on this
controller, so no point in setting it.
Changes since version 2:
- use dev_dbg instead for transfer errors
- drop i2c2 disabling patch, vc4 uses it.
Noralf.
Noralf Trønnes (7):
i2c: bcm2835: Fix hang for writing messages larger than 16 bytes
i2c: bcm2835: Protect against unexpected TXW/RXR interrupts
i2c: bcm2835: Use dev_dbg logging on transfer errors
i2c: bcm2835: Can't support I2C_M_IGNORE_NAK
i2c: bcm2835: Add support for Repeated Start Condition
i2c: bcm2835: Support i2c-dev ioctl I2C_TIMEOUT
i2c: bcm2835: Add support for dynamic clock
drivers/i2c/busses/i2c-bcm2835.c | 209 ++++++++++++++++++++++++---------------
1 file changed, 130 insertions(+), 79 deletions(-)
--
2.8.2
^ permalink raw reply
* RE: [patch v2] i2c: add master driver for mellanox systems
From: Vadim Pasternak @ 2016-09-28 11:50 UTC (permalink / raw)
To: Vadim Pasternak, wsa@the-dreams.de
Cc: linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
jiri@resnulli.us, Michael Shych
In-Reply-To: <1474297231-36264-1-git-send-email-vadimp@mellanox.com>
Hi,
I'd like to kindly clarify whether the patch I sent could be applied, or I need to modify something?
Best regards,
Vadim.
> -----Original Message-----
> From: vadimp@mellanox.com [mailto:vadimp@mellanox.com]
> Sent: Monday, September 19, 2016 6:01 PM
> To: wsa@the-dreams.de
> Cc: linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org; jiri@resnulli.us;
> Vadim Pasternak <vadimp@mellanox.com>; Michael Shych
> <michaelsh@mellanox.com>
> Subject: [patch v2] i2c: add master driver for mellanox systems
>
> From: Vadim Pasternak <vadimp@mellanox.com>
>
> Device driver for Mellanox I2C controller logic, implemented in Lattice CPLD
> device.
> Device supports:
> - Master mode
> - One physical bus
> - Polling mode
>
> The Kconfig currently controlling compilation of this code is:
> drivers/i2c/busses/Kconfig:config I2C_MLXCPLD
>
> Signed-off-by: Michael Shych <michaelsh@mellanox.com>
> Signed-off-by: Vadim Pasternak <vadimp@mellanox.com>
> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> v1->v2
> Fixes added by Vadim:
> - Put new record in Makefile in alphabetic order;
> - Remove http://www.mellanox.com from MAINTAINERS record;
> ---
> Documentation/i2c/busses/i2c-mlxcpld | 47 +++
> MAINTAINERS | 8 +
> drivers/i2c/busses/Kconfig | 12 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-mlxcpld.c | 597
> +++++++++++++++++++++++++++++++++++
> 5 files changed, 665 insertions(+)
> create mode 100644 Documentation/i2c/busses/i2c-mlxcpld
> create mode 100644 drivers/i2c/busses/i2c-mlxcpld.c
>
> diff --git a/Documentation/i2c/busses/i2c-mlxcpld
> b/Documentation/i2c/busses/i2c-mlxcpld
> new file mode 100644
> index 0000000..0f8678a
> --- /dev/null
> +++ b/Documentation/i2c/busses/i2c-mlxcpld
> @@ -0,0 +1,47 @@
> +Driver i2c-mlxcpld
> +
> +Author: Michael Shych <michaelsh@mellanox.com>
> +
> +This is a for Mellanox I2C controller logic, implemented in Lattice
> +CPLD device.
> +Device supports:
> + - Master mode.
> + - One physical bus.
> + - Polling mode.
> +
> +This controller is equipped within the next Mellanox systems:
> +"msx6710", "msx6720", "msb7700", "msn2700", "msx1410", "msn2410",
> +"msb7800", "msn2740", "msn2100".
> +
> +The next transaction types are supported:
> + - Receive Byte/Block.
> + - Send Byte/Block.
> + - Read Byte/Block.
> + - Write Byte/Block.
> +
> +Registers:
> +CTRL 0x1 - control reg.
> + Resets all the registers.
> +HALF_CYC 0x4 - cycle reg.
> + Configure the width of I2C SCL half clock cycle (in 4
> LPC_CLK
> + units).
> +I2C_HOLD 0x5 - hold reg.
> + OE (output enable) is delayed by value set to this
> register
> + (in LPC_CLK units)
> +CMD 0x6 - command reg.
> + Bit 7(lsb), 0 = write, 1 = read.
> + Bits [6:0] - the 7bit Address of the I2C device.
> + It should be written last as it triggers an I2C transaction.
> +NUM_DATA 0x7 - data size reg.
> + Number of address bytes to write in read transaction
> +NUM_ADDR 0x8 - address reg.
> + Number of address bytes to write in read transaction.
> +STATUS 0x9 - status reg.
> + Bit 0 - transaction is completed.
> + Bit 4 - ACK/NACK.
> +DATAx 0xa - 0x54 - 68 bytes data buffer regs.
> + For write transaction address is specified in four first
> bytes
> + (DATA1 - DATA4), data starting from DATA4.
> + For read transactions address is send in separate
> transaction and
> + specified in four first bytes (DATA0 - DATA3). Data is
> reading
> + starting from DATA0.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6781a3f..dc31231 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7667,6 +7667,14 @@ W: http://www.mellanox.com
> Q: http://patchwork.ozlabs.org/project/netdev/list/
> F: drivers/net/ethernet/mellanox/mlxsw/
>
> +MELLANOX MLXCPLD I2C DRIVER
> +M: Vadim Pasternak <vadimp@mellanox.com>
> +M: Michael Shych <michaelsh@mellanox.com>
> +L: linux-i2c@vger.kernel.org
> +S: Supported
> +F: drivers/i2c/busses/i2c-mlxcpld.c
> +F: Documentation/i2c/busses/i2c-mlxcpld
> +
> SOFT-ROCE DRIVER (rxe)
> M: Moni Shoua <monis@mellanox.com>
> L: linux-rdma@vger.kernel.org
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index
> 5c3993b..1126142a 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1203,4 +1203,16 @@ config I2C_OPAL
> This driver can also be built as a module. If so, the module will be
> called as i2c-opal.
>
> +config I2C_MLXCPLD
> + tristate "Mellanox I2C driver"
> + depends on X86_64
> + default y
> + help
> + This exposes the Mellanox platform I2C busses to the linux I2C layer
> + for X86 based systems.
> + Controller is implemented as CPLD logic.
> +
> + This driver can also be built as a module. If so, the module will be
> + called as i2c-mlxcpld.
> +
> endmenu
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index
> 37f2819..4df3578 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -118,5 +118,6 @@ obj-$(CONFIG_I2C_PCA_ISA) += i2c-pca-isa.o
> obj-$(CONFIG_I2C_SIBYTE) += i2c-sibyte.o
> obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o
> obj-$(CONFIG_SCx200_ACB) += scx200_acb.o
> +obj-$(CONFIG_I2C_MLXCPLD) += i2c-mlxcpld.o
>
> ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG diff --git
> a/drivers/i2c/busses/i2c-mlxcpld.c b/drivers/i2c/busses/i2c-mlxcpld.c
> new file mode 100644
> index 0000000..dd62190
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-mlxcpld.c
> @@ -0,0 +1,597 @@
> +/*
> + * drivers/i2c/busses/i2c-mlxcpld.c
> + * Copyright (c) 2016 Mellanox Technologies. All rights reserved.
> + * Copyright (c) 2016 Michael Shych <michaels@mellanox.com>
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. Neither the names of the copyright holders nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * Alternatively, this software may be distributed under the terms of
> +the
> + * GNU General Public License ("GPL") version 2 as published by the
> +Free
> + * Software Foundation.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> CONTRIBUTORS "AS IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> LIMITED
> +TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> PARTICULAR
> +PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
> +CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> +BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
> WHETHER
> +IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
> +OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
> ADVISED
> +OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +/* General defines */
> +#define MLXPLAT_CPLD_LPC_I2C_BASE_ADRR 0x2000
> +#define MLXCPLD_I2C_DEVICE_NAME "i2c_mlxcpld"
> +#define MLXCPLD_I2C_VALID_FLAG (I2C_M_RECV_LEN |
> I2C_M_RD)
> +#define MLXCPLD_I2C_BUS_NUM 1
> +#define MLXCPLD_I2C_DATA_REG_SZ 36
> +#define MLXCPLD_I2C_MAX_ADDR_LEN 4
> +#define MLXCPLD_I2C_RETR_NUM 2
> +#define MLXCPLD_I2C_XFER_TO 500000 /* msec */
> +#define MLXCPLD_I2C_POLL_TIME 2000 /* msec */
> +
> +/* LPC I2C registers */
> +#define MLXCPLD_LPCI2C_LPF_REG 0x0
> +#define MLXCPLD_LPCI2C_CTRL_REG 0x1
> +#define MLXCPLD_LPCI2C_HALF_CYC_REG 0x4
> +#define MLXCPLD_LPCI2C_I2C_HOLD_REG 0x5
> +#define MLXCPLD_LPCI2C_CMD_REG 0x6
> +#define MLXCPLD_LPCI2C_NUM_DAT_REG 0x7
> +#define MLXCPLD_LPCI2C_NUM_ADDR_REG 0x8
> +#define MLXCPLD_LPCI2C_STATUS_REG 0x9
> +#define MLXCPLD_LPCI2C_DATA_REG 0xa
> +
> +/* LPC I2C masks and parametres */
> +#define MLXCPLD_LPCI2C_RST_SEL_MASK 0x1
> +#define MLXCPLD_LPCI2C_LPF_DFLT 0x2
> +#define MLXCPLD_LPCI2C_HALF_CYC_100 0x1f
> +#define MLXCPLD_LPCI2C_I2C_HOLD_100 0x3c
> +#define MLXCPLD_LPCI2C_TRANS_END 0x1
> +#define MLXCPLD_LPCI2C_STATUS_NACK 0x10
> +#define MLXCPLD_LPCI2C_ERR_IND -1
> +#define MLXCPLD_LPCI2C_NO_IND 0
> +#define MLXCPLD_LPCI2C_ACK_IND 1
> +#define MLXCPLD_LPCI2C_NACK_IND 2
> +
> +/**
> + * mlxcpld_i2c_regs - controller registers:
> + * @half_cyc - half cycle register
> + * @i2c_hold - hold register
> + * @config - config register
> + * @cmd - command register
> + * @cmd - status register
> + * @data - register data
> +**/
> +struct mlxcpld_i2c_regs {
> + u8 half_cyc;
> + u8 i2c_hold;
> + u8 config;
> + u8 cmd;
> + u8 status;
> + u8 data[MLXCPLD_I2C_DATA_REG_SZ];
> +};
> +
> +/**
> + * mlxcpld_i2c_curr_transf - current transaction parameters:
> + * @cmd - command
> + * @addr_width - address width
> + * @data_len - data length
> + * @cmd - command register
> + * @msg_num - message number
> + * @msg - pointer to message buffer
> +**/
> +struct mlxcpld_i2c_curr_transf {
> + u8 cmd;
> + u8 addr_width;
> + u8 data_len;
> + u8 msg_num;
> + struct i2c_msg *msg;
> +};
> +
> +/**
> + * mlxcpld_i2c_priv - private controller data:
> + * @lpc_gen_dec_reg - register space
> + * @adap - i2c adapter
> + * @dev_id - device id
> + * @base_addr - base IO address
> + * @poll_time - polling time
> + * @xfer_to - transfer timeout in microsec (500000)
> + * @retr_num - access retries number (2)
> + * @block_sz - maximum data block size (36),
> + * @lock - bus access lock
> + * @lpc_i2c_res - lpc i2c resourse
> + * @lpc_cpld_res - lpc cpld resource
> + * @xfer - current i2c transfer block
> + * @pdev - platform device
> +**/
> +struct mlxcpld_i2c_priv {
> + struct i2c_adapter adap;
> + u16 dev_id;
> + u16 base_addr;
> + u16 poll_time;
> + int xfer_to;
> + int retr_num;
> + int block_sz;
> + struct mutex lock;
> + struct mlxcpld_i2c_curr_transf xfer;
> + struct platform_device *pdev;
> +};
> +struct platform_device *mlxcpld_i2c_plat_dev;
> +
> +static void mlxcpld_i2c_lpc_write_buf(u8 *data, u8 len, u32 addr) {
> + int i, nbyte, ndword;
> +
> + nbyte = len % 4;
> + ndword = len / 4;
> + for (i = 0; i < ndword; i++)
> + outl(*((u32 *)data + i), addr + i * 4);
> + ndword *= 4;
> + addr += ndword;
> + data += ndword;
> + for (i = 0; i < nbyte; i++)
> + outb(*(data + i), addr + i);
> +}
> +
> +static void mlxcpld_i2c_lpc_read_buf(u8 *data, u8 len, u32 addr) {
> + int i, nbyte, ndword;
> +
> + nbyte = len % 4;
> + ndword = len / 4;
> + for (i = 0; i < ndword; i++)
> + *((u32 *)data + i) = inl(addr + i * 4);
> + ndword *= 4;
> + addr += ndword;
> + data += ndword;
> + for (i = 0; i < nbyte; i++)
> + *(data + i) = inb(addr + i);
> +}
> +
> +static void mlxcpld_i2c_read_comm(struct mlxcpld_i2c_priv *priv, u8 offs,
> + u8 *data, u8 datalen)
> +{
> + u32 addr = priv->base_addr + offs;
> +
> + switch (datalen) {
> + case 1:
> + *(data) = inb(addr);
> + break;
> + case 2:
> + *((u16 *)data) = inw(addr);
> + break;
> + case 3:
> + *((u16 *)data) = inw(addr);
> + *(data + 2) = inb(addr + 2);
> + break;
> + case 4:
> + *((u32 *)data) = inl(addr);
> + break;
> + default:
> + mlxcpld_i2c_lpc_read_buf(data, datalen, addr);
> + break;
> + }
> +}
> +
> +static void mlxcpld_i2c_write_comm(struct mlxcpld_i2c_priv *priv, u8 offs,
> + u8 *data, u8 datalen)
> +{
> + u32 addr = priv->base_addr + offs;
> +
> + switch (datalen) {
> + case 1:
> + outb(*(data), addr);
> + break;
> + case 2:
> + outw(*((u16 *)data), addr);
> + break;
> + case 3:
> + outw(*((u16 *)data), addr);
> + outb(*(data + 2), addr + 2);
> + break;
> + case 4:
> + outl(*((u32 *)data), addr);
> + break;
> + default:
> + mlxcpld_i2c_lpc_write_buf(data, datalen, addr);
> + break;
> + }
> +}
> +
> +/* Check validity of current i2c message and all transfer.
> + * Calculate also coomon length of all i2c messages in transfer.
> + */
> +static int mlxcpld_i2c_invalid_len(struct mlxcpld_i2c_priv *priv,
> + const struct i2c_msg *msg, u8 *comm_len) {
> + u8 max_len = msg->flags == I2C_M_RD ? priv->block_sz -
> + MLXCPLD_I2C_MAX_ADDR_LEN : priv->block_sz;
> +
> + if (msg->len < 0 || msg->len > max_len)
> + return -EINVAL;
> +
> + *comm_len += msg->len;
> + if (*comm_len > priv->block_sz)
> + return -EINVAL;
> + else
> + return 0;
> +}
> +
> +/* Check validity of received i2c messages parameters.
> + * Returns 0 if OK, other - in case of invalid parameters
> + * or common length of data that should be passed to CPLD */ static
> +int mlxcpld_i2c_check_msg_params(struct mlxcpld_i2c_priv *priv,
> + struct i2c_msg *msgs, int num,
> + u8 *comm_len)
> +{
> + int i;
> +
> + if (!num) {
> + dev_err(&priv->pdev->dev, "Incorrect 0 num of messages\n");
> + return -EINVAL;
> + }
> +
> + if (unlikely(msgs[0].addr > 0x7f)) {
> + dev_err(&priv->pdev->dev, "Invalid address 0x%03x\n",
> + msgs[0].addr);
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < num; ++i) {
> + if (unlikely(!msgs[i].buf)) {
> + dev_err(&priv->pdev->dev, "Invalid buf in msg[%d]\n",
> + i);
> + return -EINVAL;
> + }
> + if (unlikely(msgs[0].addr != msgs[i].addr)) {
> + dev_err(&priv->pdev->dev, "Invalid addr in msg[%d]\n",
> + i);
> + return -EINVAL;
> + }
> + if (unlikely(mlxcpld_i2c_invalid_len(priv, &msgs[i],
> + comm_len))) {
> + dev_err(&priv->pdev->dev, "Invalid len %d msg[%d],
> addr 0x%x, lag %u\n",
> + msgs[i].len, i, msgs[i].addr, msgs[i].flags);
> + return -EINVAL;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/* Check if transfer is completed and status of operation.
> + * Returns 0 - transfer completed (both ACK or NACK),
> + * negative - transfer isn't finished.
> + */
> +static int mlxcpld_i2c_check_status(struct mlxcpld_i2c_priv *priv, int
> +*status) {
> + u8 val;
> +
> + mlxcpld_i2c_read_comm(priv, MLXCPLD_LPCI2C_STATUS_REG, &val, 1);
> +
> + if (val & MLXCPLD_LPCI2C_TRANS_END) {
> + if (val & MLXCPLD_LPCI2C_STATUS_NACK)
> + /* The slave is unable to accept the data. No such
> + * slave, command not understood, or unable to accept
> + * any more data.
> + */
> + *status = MLXCPLD_LPCI2C_NACK_IND;
> + else
> + *status = MLXCPLD_LPCI2C_ACK_IND;
> + return 0;
> + }
> + *status = MLXCPLD_LPCI2C_NO_IND;
> +
> + return -EIO;
> +}
> +
> +static void mlxcpld_i2c_set_transf_data(struct mlxcpld_i2c_priv *priv,
> + struct i2c_msg *msgs, int num,
> + u8 comm_len)
> +{
> + priv->xfer.msg = msgs;
> + priv->xfer.msg_num = num;
> +
> + /*
> + * All upper layers currently are never use transfer with more than
> + * 2 messages. Actually, it's also not so relevant in Mellanox systems
> + * because of HW limitation. Max size of transfer is o more than 20B
> + * in current x86 LPCI2C bridge.
> + */
> + priv->xfer.cmd = (msgs[num - 1].flags & I2C_M_RD);
> +
> + if (priv->xfer.cmd == I2C_M_RD) {
> + if (comm_len == msgs[0].len) {
> + /* Special case of addr_width = 0 */
> + priv->xfer.addr_width = 0;
> + priv->xfer.data_len = comm_len;
> + } else {
> + priv->xfer.addr_width = msgs[0].len;
> + priv->xfer.data_len = comm_len - priv-
> >xfer.addr_width;
> + }
> + } else {
> + /* Width (I2C_NUM_ADDR reg) isn't used in write command. */
> + priv->xfer.addr_width = 0;
> + priv->xfer.data_len = comm_len;
> + }
> +}
> +
> +/* Reset CPLD LPCI2C block */
> +static void mlxcpld_i2c_reset(struct mlxcpld_i2c_priv *priv) {
> + u8 val;
> +
> + mutex_lock(&priv->lock);
> + mlxcpld_i2c_read_comm(priv, MLXCPLD_LPCI2C_CTRL_REG, &val, 1);
> + val &= ~MLXCPLD_LPCI2C_RST_SEL_MASK;
> + mlxcpld_i2c_write_comm(priv, MLXCPLD_LPCI2C_CTRL_REG, &val, 1);
> + mutex_unlock(&priv->lock);
> +}
> +
> +/* Make sure the CPLD is ready to start transmitting.
> + * Return 0 if it is, -EBUSY if it is not.
> + */
> +static int mlxcpld_i2c_check_busy(struct mlxcpld_i2c_priv *priv) {
> + u8 val;
> +
> + mlxcpld_i2c_read_comm(priv, MLXCPLD_LPCI2C_STATUS_REG, &val, 1);
> +
> + if (val & MLXCPLD_LPCI2C_TRANS_END)
> + return 0;
> +
> + return -EIO;
> +}
> +
> +static int mlxcpld_i2c_wait_for_free(struct mlxcpld_i2c_priv *priv) {
> + int timeout = 0;
> +
> + do {
> + if (!mlxcpld_i2c_check_busy(priv))
> + break;
> + usleep_range(priv->poll_time/2, priv->poll_time);
> + timeout += priv->poll_time;
> + } while (timeout < priv->xfer_to);
> +
> + if (timeout > priv->xfer_to)
> + return -ETIMEDOUT;
> +
> + return 0;
> +}
> +
> +/*
> + * Wait for master transfer to complete.
> + * It puts current process to sleep until we get interrupt or timeout expires.
> + * Returns the number of transferred or read bytes or error (<0).
> + */
> +static int mlxcpld_i2c_wait_for_tc(struct mlxcpld_i2c_priv *priv) {
> + int status, i = 1, timeout = 0;
> + u8 datalen;
> + int err = 0;
> +
> + do {
> + usleep_range(priv->poll_time / 2, priv->poll_time);
> + if (!mlxcpld_i2c_check_status(priv, &status))
> + break;
> + timeout += priv->poll_time;
> + } while (status == 0 && timeout < priv->xfer_to);
> +
> + switch (status) {
> + case MLXCPLD_LPCI2C_NO_IND:
> + return -ETIMEDOUT;
> + case MLXCPLD_LPCI2C_ACK_IND:
> + if (priv->xfer.cmd == I2C_M_RD) {
> + /*
> + * Actual read data len will be always the same as
> + * requested len. 0xff (line pull-up) will be returned
> + * if slave has no data to return. Thus don't read
> + * MLXCPLD_LPCI2C_NUM_DAT_REG reg from CPLD.
> + */
> + err = datalen = priv->xfer.data_len;
> + if (priv->xfer.msg_num == 1)
> + i = 0;
> +
> + if (!priv->xfer.msg[i].buf)
> + err = -EINVAL;
> + else
> + mlxcpld_i2c_read_comm(priv,
> +
> MLXCPLD_LPCI2C_DATA_REG,
> + priv->xfer.msg[i].buf,
> + datalen);
> + } else {
> + err = priv->xfer.addr_width + priv->xfer.data_len;
> + }
> + break;
> + case MLXCPLD_LPCI2C_NACK_IND:
> + err = -EAGAIN;
> + break;
> + case MLXCPLD_LPCI2C_ERR_IND:
> + err = -EIO;
> + break;
> + default:
> + break;
> + }
> +
> + return err;
> +}
> +
> +static void mlxcpld_i2c_xfer_msg(struct mlxcpld_i2c_priv *priv) {
> + int i, len = 0;
> + u8 cmd;
> +
> + mlxcpld_i2c_write_comm(priv, MLXCPLD_LPCI2C_NUM_DAT_REG,
> + &priv->xfer.data_len, 1);
> + mlxcpld_i2c_write_comm(priv, MLXCPLD_LPCI2C_NUM_ADDR_REG,
> + &priv->xfer.addr_width, 1);
> +
> + for (i = 0; i < priv->xfer.msg_num; i++) {
> + if ((priv->xfer.msg[i].flags & I2C_M_RD) != I2C_M_RD) {
> + /* Don't write to CPLD buffer in read transaction */
> + mlxcpld_i2c_write_comm(priv,
> MLXCPLD_LPCI2C_DATA_REG +
> + len, priv->xfer.msg[i].buf,
> + priv->xfer.msg[i].len);
> + len += priv->xfer.msg[i].len;
> + }
> + }
> +
> + /* Set target slave address with command for master transfer.
> + * It should be latest executed function before CPLD transaction.
> + */
> + cmd = (priv->xfer.msg[0].addr << 1) | priv->xfer.cmd;
> + mlxcpld_i2c_write_comm(priv, MLXCPLD_LPCI2C_CMD_REG, &cmd, 1);
> }
> +
> +/* Generic lpc-i2c transfer.
> + * Returns the number of processed messages or error (<0).
> + */
> +static int mlxcpld_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> + int num)
> +{
> + struct mlxcpld_i2c_priv *priv = i2c_get_adapdata(adap);
> + u8 comm_len = 0;
> + int err;
> +
> + err = mlxcpld_i2c_check_msg_params(priv, msgs, num, &comm_len);
> + if (err) {
> + dev_err(&priv->pdev->dev, "Incorrect message\n");
> + return err;
> + }
> +
> + /* Check bus state */
> + if (mlxcpld_i2c_wait_for_free(priv)) {
> + dev_err(&priv->pdev->dev, "LPCI2C bridge is busy\n");
> +
> + /*
> + * Usually it means something serious has happened.
> + * We can not have unfinished previous transfer
> + * so it doesn't make any sense to try to stop it.
> + * Probably we were not able to recover from the
> + * previous error.
> + * The only reasonable thing - is soft reset.
> + */
> + mlxcpld_i2c_reset(priv);
> + if (mlxcpld_i2c_check_busy(priv)) {
> + dev_err(&priv->pdev->dev, "LPCI2C bridge is busy after
> reset\n");
> + return -EIO;
> + }
> + }
> +
> + mlxcpld_i2c_set_transf_data(priv, msgs, num, comm_len);
> +
> + mutex_lock(&priv->lock);
> + /* Do real transfer. Can't fail */
> + mlxcpld_i2c_xfer_msg(priv);
> + /* Wait for transaction complete */
> + err = mlxcpld_i2c_wait_for_tc(priv);
> + mutex_unlock(&priv->lock);
> +
> + return err < 0 ? err : num;
> +}
> +
> +static u32 mlxcpld_i2c_func(struct i2c_adapter *adap) {
> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
> I2C_FUNC_SMBUS_BLOCK_DATA;
> +}
> +
> +static const struct i2c_algorithm mlxcpld_i2c_algo = {
> + .master_xfer = mlxcpld_i2c_xfer,
> + .functionality = mlxcpld_i2c_func
> +};
> +
> +static struct i2c_adapter mlxcpld_i2c_adapter = {
> + .owner = THIS_MODULE,
> + .name = "i2c-mlxcpld",
> + .class = I2C_CLASS_HWMON | I2C_CLASS_SPD,
> + .algo = &mlxcpld_i2c_algo,
> +};
> +
> +static int mlxcpld_i2c_probe(struct platform_device *pdev) {
> + struct mlxcpld_i2c_priv *priv;
> + int err;
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(struct mlxcpld_i2c_priv),
> + GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + mutex_init(&priv->lock);
> + platform_set_drvdata(pdev, priv);
> + priv->pdev = pdev;
> + priv->xfer_to = MLXCPLD_I2C_XFER_TO;
> + priv->retr_num = MLXCPLD_I2C_RETR_NUM;
> + priv->block_sz = MLXCPLD_I2C_DATA_REG_SZ;
> + priv->poll_time = MLXCPLD_I2C_POLL_TIME;
> + /* Register with i2c layer */
> + priv->adap = mlxcpld_i2c_adapter;
> + priv->adap.dev.parent = &pdev->dev;
> + i2c_set_adapdata(&priv->adap, priv);
> + priv->adap.retries = priv->retr_num;
> + priv->adap.nr = MLXCPLD_I2C_BUS_NUM;
> + priv->adap.timeout = usecs_to_jiffies(priv->xfer_to);
> +
> + err = i2c_add_numbered_adapter(&priv->adap);
> + if (err) {
> + dev_err(&pdev->dev, "Failed to add %s adapter (%d)\n",
> + MLXCPLD_I2C_DEVICE_NAME, err);
> + goto fail_adapter;
> + }
> +
> + priv->base_addr = MLXPLAT_CPLD_LPC_I2C_BASE_ADRR;
> +
> + return 0;
> +
> +fail_adapter:
> + mutex_destroy(&priv->lock);
> + return err;
> +}
> +
> +static int mlxcpld_i2c_remove(struct platform_device *pdev) {
> + struct mlxcpld_i2c_priv *priv = platform_get_drvdata(pdev);
> +
> + i2c_del_adapter(&priv->adap);
> + mutex_destroy(&priv->lock);
> +
> + return 0;
> +}
> +
> +static struct platform_driver mlxcpld_i2c_driver = {
> + .probe = mlxcpld_i2c_probe,
> + .remove = mlxcpld_i2c_remove,
> + .driver = {
> + .name = MLXCPLD_I2C_DEVICE_NAME,
> + },
> +};
> +
> +module_platform_driver(mlxcpld_i2c_driver);
> +
> +MODULE_AUTHOR("Michael Shych (michaels@mellanox.com)");
> +MODULE_DESCRIPTION("Mellanox I2C-CPLD controller driver");
> +MODULE_LICENSE("GPL v2"); MODULE_ALIAS("platform:i2c-mlxcpld");
> --
> 2.1.4
^ permalink raw reply
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