[PATCH v6 0/5] i2c: aspeed: added driver for Aspeed I2C

[PATCH v6 0/5] i2c: aspeed: added driver for Aspeed I2C

[Brendan Higgins]

Sorry for the delay, I went on a long vacation prior to receiving feedback and
got back in the middle of a hardware bring up that consumed all of my attention
for an extended period of time. I will try to plan upstream submissions around
my other responsibilities better in the future.

Addressed comments from:
  - Vladimir in: https://www.spinics.net/lists/linux-i2c/msg27387.html
    and: https://www.spinics.net/lists/linux-i2c/msg27386.html
  - Wolfram in: https://www.spinics.net/lists/linux-i2c/msg27476.html
    and: https://www.spinics.net/lists/linux-i2c/msg27483.html

Changes since previous update:
  - No longer arbitrarily restrict bus to be slave xor master.
  - Pulled out "struct aspeed_i2c_controller" as a interrupt controller.
  - Pulled out slave support into its own commit.
  - Rewrote code that sets clock divider register because the original version
    set it incorrectly.
  - Discovered and fixed issue in implementation that caused certain slave
    devices to misbehave; the cause was that the master IRQ handler would return
    control to the requesting thread after the last RX or TX command was handled
    such that the requesting thread would issue either a repeated start or stop.
    This was incorrect because the time taken to complete the completion was too
    great. I fixed this by rewriting the master IRQ handler so that it now
    manages the entire transaction only returning control to the requesting
    thread once the entire transaction is complete.
  - Rewrote the aspeed_i2c_master_irq handler because the old method of
    completing a completion in between restarts was too slow causing devices to
    misbehave.
  - Added support for I2C_M_RECV_LEN which I had incorrectly said was supported
    before.
  - Addressed other comments from Vladimir.

Changes have been tested on the Aspeed 2500 evaluation board, as before, and now
on a real platform with an Aspeed 2520.
--
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

[PATCH v6 1/5] irqchip/aspeed-i2c-ic: binding docs for Aspeed I2C Interrupt Controller

[Brendan Higgins]

Added device tree binding documentation for Aspeed I2C Interrupt
Controller.

Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
Added in v6:
  - Pulled "aspeed_i2c_controller" out into a interrupt controller since that is
    what it actually does.
---
 .../interrupt-controller/aspeed,ast2400-i2c-ic.txt | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2400-i2c-ic.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2400-i2c-ic.txt b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2400-i2c-ic.txt
new file mode 100644
index 000000000000..033cc82e5684
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2400-i2c-ic.txt
@@ -0,0 +1,25 @@
+Device tree configuration for the I2C Interrupt Controller on the AST24XX and
+AST25XX SoCs.
+
+Required Properties:
+- #address-cells	: should be 1
+- #size-cells 		: should be 1
+- #interrupt-cells 	: should be 1
+- compatible 		: should be "aspeed,ast2400-i2c-ic"
+			  or "aspeed,ast2500-i2c-ic"
+- reg			: address start and range of controller
+- interrupts		: interrupt number
+- interrupt-controller	: denotes that the controller receives and fires
+			  new interrupts for child busses
+
+Example:
+
+i2c_ic: interrupt-controller@0 {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	#interrupt-cells = <1>;
+	compatible = "aspeed,ast2400-i2c-ic";
+	reg = <0x0 0x40>;
+	interrupts = <12>;
+	interrupt-controller;
+};
-- 
2.12.2.564.g063fe858b8-goog

Re: [PATCH v6 1/5] irqchip/aspeed-i2c-ic: binding docs for Aspeed I2C Interrupt Controller

[Benjamin Herrenschmidt]

On Mon, 2017-03-27 at 22:12 -0700, Brendan Higgins wrote:
> Added device tree binding documentation for Aspeed I2C Interrupt
> Controller.

It's a little bit overkill ... It's not so much an interrupt controller
than a single "summary" register that reflects the state of the
interrupts of all the i2c controllers ;-) It can't do anything with
them, no individual masking or acking or similar.

In fact to be honest I wouldn't even have bothered making it an
irq_domain in the first place though it *is* nice I admit to see the
interrupt counts per bus in /proc/interrupts as a result.

Cheers,
Ben.

> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---
> Added in v6:
>   - Pulled "aspeed_i2c_controller" out into a interrupt controller
> since that is
>     what it actually does.
> ---
>  .../interrupt-controller/aspeed,ast2400-i2c-ic.txt | 25
> ++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-
> controller/aspeed,ast2400-i2c-ic.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-
> controller/aspeed,ast2400-i2c-ic.txt
> b/Documentation/devicetree/bindings/interrupt-
> controller/aspeed,ast2400-i2c-ic.txt
> new file mode 100644
> index 000000000000..033cc82e5684
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-
> controller/aspeed,ast2400-i2c-ic.txt
> @@ -0,0 +1,25 @@
> +Device tree configuration for the I2C Interrupt Controller on the
> AST24XX and
> +AST25XX SoCs.
> +
> +Required Properties:
> +- #address-cells	: should be 1
> +- #size-cells 		: should be 1
> +- #interrupt-cells 	: should be 1
> +- compatible 		: should be "aspeed,ast2400-i2c-ic"
> +			  or "aspeed,ast2500-i2c-ic"
> +- reg			: address start and range of controller
> +- interrupts		: interrupt number
> +- interrupt-controller	: denotes that the controller receives
> and fires
> +			  new interrupts for child busses
> +
> +Example:
> +
> +i2c_ic: interrupt-controller@0 {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	#interrupt-cells = <1>;
> +	compatible = "aspeed,ast2400-i2c-ic";
> +	reg = <0x0 0x40>;
> +	interrupts = <12>;
> +	interrupt-controller;
> +};

Re: [PATCH v6 1/5] irqchip/aspeed-i2c-ic: binding docs for Aspeed I2C Interrupt Controller

[Brendan Higgins]

I think I addressed this on the other email with the actual driver.
Anyway, I thought that this is pretty much the dummy irqchip code is
for; I have seen some other drivers do the same thing. It is true that
this is a really basic "interrupt controller;" it cannot mask on its
own, etc; nevertheless, I think you will pretty much end up with the
same code for an "I2C controller;" it just won't use an irq_domain.

Re: [PATCH v6 1/5] irqchip/aspeed-i2c-ic: binding docs for Aspeed I2C Interrupt Controller

[Benjamin Herrenschmidt]

On Wed, 2017-03-29 at 03:34 -0700, Brendan Higgins wrote:
> I think I addressed this on the other email with the actual driver.
> Anyway, I thought that this is pretty much the dummy irqchip code is
> for; I have seen some other drivers do the same thing. It is true
> that
> this is a really basic "interrupt controller;" it cannot mask on its
> own, etc; nevertheless, I think you will pretty much end up with the
> same code for an "I2C controller;" it just won't use an irq_domain.

Don't worry too much about this. As I think I mention it's not a huge
deal at this stage, I just wanted to make sure you were aware of the
compromise(s) involved.

Regarding the other comment about the "fast mode", my main worry here
is that somebody might come up with a 2Mhz capable device, we'll hit
your 1Mhz test, enable fast mode, and shoot it with 3.4Mhz which it
might not be happy at all about...

I think the cut-off for switching to the "fast" mode should basically
be the fast speed mode frequency (which isn't clear from the spec but
seems to be 3.4Mhz). Otherwise people will end up with higher speeds
than what they asked for and that's bad.

Cheers,
Ben.

Re: [PATCH v6 1/5] irqchip/aspeed-i2c-ic: binding docs for Aspeed I2C Interrupt Controller

[Brendan Higgins]

> Regarding the other comment about the "fast mode", my main worry here
> is that somebody might come up with a 2Mhz capable device, we'll hit
> your 1Mhz test, enable fast mode, and shoot it with 3.4Mhz which it
> might not be happy at all about...
>
> I think the cut-off for switching to the "fast" mode should basically
> be the fast speed mode frequency (which isn't clear from the spec but
> seems to be 3.4Mhz). Otherwise people will end up with higher speeds
> than what they asked for and that's bad.

Ah, but see the documentation only says that high speed mode sets the
Base Clock divisor to zero; is does not say anything about tCKHigh or
tCKLow (clk_high and clk_low in my code respectively), which are the
only parameters which are manipulated for speeds greater than or equal
to 1.5MHz since:

# I forgot the "APB_freq /" part in the comment on my
aspeed_i2c_get_clk_reg_val(...)
# My function still does the computation correctly, I just forgot this
in the comment.
SCL_freq = APB_freq / (1 << base_clk) * (clk_high + 1 + clk_low + 1)

so if base_clk = 0, clk_high = 15, clk_low = 15, APB_freq = 50MHz

SCL_freq = APB_freq / (1 << base_clk) * (clk_high + 1 + clk_low + 1)
                = 50000000 / (1 << 0) * (15 + 1 + 15 + 1)
                = 50000000 / 32
                = 1562500Hz
                = ~1.5MHz

so maybe instead of setting a hard limit like I did, maybe the best
thing is to just check and see what the base_clk gets set to and if it
gets set to zero, we turn on high speed mode. What do you think?

Cheers

Re: [PATCH v6 1/5] irqchip/aspeed-i2c-ic: binding docs for Aspeed I2C Interrupt Controller

[Benjamin Herrenschmidt]

On Wed, 2017-03-29 at 13:51 -0700, Brendan Higgins wrote:
> so maybe instead of setting a hard limit like I did, maybe the best
> thing is to just check and see what the base_clk gets set to and if
> it gets set to zero, we turn on high speed mode. What do you think?

Ah maybe. Did you scope it to see if clock_hi/low do indeed apply in
high speed mode ?

I wonder if that bit does other things.. I would be interesting to
check. Ohterwise why have the bit rather than just have the driver
write 0 to the divisor ?

The doc for the high speed mode bit says "high speed mode (3.4Mbps)"
which is why I, maybe incorrectly, assumed it was a fixed frequency.

Anyway, not a huge deal at this point, but something to look into
at some stage.

Cheers,
Ben.

Re: [PATCH v6 1/5] irqchip/aspeed-i2c-ic: binding docs for Aspeed I2C Interrupt Controller

[Rob Herring]

On Mon, Mar 27, 2017 at 10:12:22PM -0700, Brendan Higgins wrote:
> Added device tree binding documentation for Aspeed I2C Interrupt
> Controller.
> 
> Signed-off-by: Brendan Higgins <brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> ---
> Added in v6:
>   - Pulled "aspeed_i2c_controller" out into a interrupt controller since that is
>     what it actually does.
> ---
>  .../interrupt-controller/aspeed,ast2400-i2c-ic.txt | 25 ++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2400-i2c-ic.txt

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
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

[PATCH v6 3/5] i2c: aspeed: added documentation for Aspeed I2C driver

[Brendan Higgins]

Added device tree binding documentation for Aspeed I2C busses.

Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
Changes for v2:
  - None
Changes for v3:
  - Removed reference to "bus" device tree param
Changes for v4:
  - None
Changes for v5:
  - None
Changes for v6:
  - Replaced the controller property with and interrupt controller, leaving only
    the busses in the I2C documentation.
---
 .../devicetree/bindings/i2c/i2c-aspeed.txt         | 49 ++++++++++++++++++++++
 1 file changed, 49 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 000000000000..fbcc501706b1
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
@@ -0,0 +1,49 @@
+Device tree configuration for the I2C busses on the AST24XX and AST25XX SoCs.
+
+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"
+- clocks		: root clock of bus, should reference the APB
+			  clock
+- interrupts		: interrupt number
+- interrupt-parent	: interrupt controller for bus, should reference a
+			  aspeed,ast2400-i2c-ic or aspeed,ast2500-i2c-ic
+			  interrupt controller
+
+Optional Properties:
+- clock-frequency	: frequency of the bus clock in Hz
+			  defaults to 100 kHz when not specified
+
+Example:
+
+i2c {
+	compatible = "simple-bus";
+	#address-cells = <1>;
+	#size-cells = <1>;
+	ranges = <0 0x1e78a000 0x1000>;
+
+	i2c_ic: interrupt-controller@0 {
+		#interrupt-cells = <1>;
+		compatible = "aspeed,ast2400-i2c-ic";
+		reg = <0x0 0x40>;
+		interrupts = <12>;
+		interrupt-controller;
+	};
+
+	i2c0: i2c-bus@40 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		#interrupt-cells = <1>;
+		reg = <0x40 0x40>;
+		compatible = "aspeed,ast2400-i2c-bus";
+		bus = <0>;
+		clocks = <&clk_apb>;
+		clock-frequency = <100000>;
+		status = "disabled";
+		interrupts = <0>;
+		interrupt-parent = <&i2c_ic>;
+	};
+};
-- 
2.12.2.564.g063fe858b8-goog

Re: [PATCH v6 3/5] i2c: aspeed: added documentation for Aspeed I2C driver

[Benjamin Herrenschmidt]

On Mon, 2017-03-27 at 22:12 -0700, Brendan Higgins wrote:
> Added device tree binding documentation for Aspeed I2C busses.
> 
> Signed-off-by: Brendan Higgins <brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

> +	i2c0: i2c-bus@40 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		#interrupt-cells = <1>;
> +		reg = <0x40 0x40>;
> +		compatible = "aspeed,ast2400-i2c-bus";
> +		bus = <0>;
> +		clocks = <&clk_apb>;
> +		clock-frequency = <100000>;

For busses it's more traditional to make this "bus-frequency" but
that's a nit and Linux/fdt has not respected that tradition terribly
well. If you respin, it might be work changing.

The clock-frequency tends to be the frequency of the controller itself.

> +		status = "disabled";
> +		interrupts = <0>;
> +		interrupt-parent = <&i2c_ic>;
> +	};
> +};
--
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

Re: [PATCH v6 3/5] i2c: aspeed: added documentation for Aspeed I2C driver

[Brendan Higgins]

>> +             bus = <0>;
>> +             clocks = <&clk_apb>;
>> +             clock-frequency = <100000>;
>
> For busses it's more traditional to make this "bus-frequency" but
> that's a nit and Linux/fdt has not respected that tradition terribly
> well. If you respin, it might be work changing.
>
> The clock-frequency tends to be the frequency of the controller itself.

Ah, okay. I will change this on the next revision.
--
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

Re: [PATCH v6 3/5] i2c: aspeed: added documentation for Aspeed I2C driver

[Rob Herring]

On Tue, Mar 28, 2017 at 07:54:43PM +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2017-03-27 at 22:12 -0700, Brendan Higgins wrote:
> > Added device tree binding documentation for Aspeed I2C busses.
> > 
> > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> 
> > +	i2c0: i2c-bus@40 {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		#interrupt-cells = <1>;
> > +		reg = <0x40 0x40>;
> > +		compatible = "aspeed,ast2400-i2c-bus";
> > +		bus = <0>;
> > +		clocks = <&clk_apb>;
> > +		clock-frequency = <100000>;
> 
> For busses it's more traditional to make this "bus-frequency" but
> that's a nit and Linux/fdt has not respected that tradition terribly
> well. If you respin, it might be work changing.

Makes sense, but that's news to me. I'm still new to this DT stuff.

> The clock-frequency tends to be the frequency of the controller itself.

The clock binding has largely replaced that.

Rob

Re: [PATCH v6 3/5] i2c: aspeed: added documentation for Aspeed I2C driver

[Rob Herring]

On Mon, Mar 27, 2017 at 10:12:24PM -0700, Brendan Higgins wrote:
> Added device tree binding documentation for Aspeed I2C busses.
> 
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---
> Changes for v2:
>   - None
> Changes for v3:
>   - Removed reference to "bus" device tree param
> Changes for v4:
>   - None
> Changes for v5:
>   - None
> Changes for v6:
>   - Replaced the controller property with and interrupt controller, leaving only
>     the busses in the I2C documentation.
> ---
>  .../devicetree/bindings/i2c/i2c-aspeed.txt         | 49 ++++++++++++++++++++++
>  1 file changed, 49 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 000000000000..fbcc501706b1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
> @@ -0,0 +1,49 @@
> +Device tree configuration for the I2C busses on the AST24XX and AST25XX SoCs.
> +
> +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"
> +- clocks		: root clock of bus, should reference the APB
> +			  clock
> +- interrupts		: interrupt number
> +- interrupt-parent	: interrupt controller for bus, should reference a
> +			  aspeed,ast2400-i2c-ic or aspeed,ast2500-i2c-ic
> +			  interrupt controller
> +
> +Optional Properties:
> +- clock-frequency	: frequency of the bus clock in Hz
> +			  defaults to 100 kHz when not specified
> +
> +Example:
> +
> +i2c {
> +	compatible = "simple-bus";
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	ranges = <0 0x1e78a000 0x1000>;
> +
> +	i2c_ic: interrupt-controller@0 {
> +		#interrupt-cells = <1>;
> +		compatible = "aspeed,ast2400-i2c-ic";
> +		reg = <0x0 0x40>;
> +		interrupts = <12>;
> +		interrupt-controller;
> +	};
> +
> +	i2c0: i2c-bus@40 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		#interrupt-cells = <1>;
> +		reg = <0x40 0x40>;
> +		compatible = "aspeed,ast2400-i2c-bus";
> +		bus = <0>;

Not documented and what's it for?

> +		clocks = <&clk_apb>;
> +		clock-frequency = <100000>;
> +		status = "disabled";

Drop status from examples.

> +		interrupts = <0>;
> +		interrupt-parent = <&i2c_ic>;
> +	};
> +};
> -- 
> 2.12.2.564.g063fe858b8-goog
> 

[PATCH v6 2/5] irqchip/aspeed-i2c-ic: Add I2C IRQ controller for Aspeed

[Brendan Higgins]

The Aspeed 24XX/25XX chips share a single hardware interrupt across 14
separate I2C busses. This adds a dummy irqchip which maps the single
hardware interrupt to software interrupts for each of the busses.

Signed-off-by: Brendan Higgins <brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
Added in v6:
  - Pulled "aspeed_i2c_controller" out into a interrupt controller since that is
    what it actually does.
---
 drivers/irqchip/Makefile            |   2 +-
 drivers/irqchip/irq-aspeed-i2c-ic.c | 102 ++++++++++++++++++++++++++++++++++++
 2 files changed, 103 insertions(+), 1 deletion(-)
 create mode 100644 drivers/irqchip/irq-aspeed-i2c-ic.c

diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 152bc40b6762..c136c2bd1761 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -74,6 +74,6 @@ obj-$(CONFIG_MVEBU_ODMI)		+= irq-mvebu-odmi.o
 obj-$(CONFIG_MVEBU_PIC)			+= irq-mvebu-pic.o
 obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
 obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
-obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
+obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
diff --git a/drivers/irqchip/irq-aspeed-i2c-ic.c b/drivers/irqchip/irq-aspeed-i2c-ic.c
new file mode 100644
index 000000000000..59c50b28dec0
--- /dev/null
+++ b/drivers/irqchip/irq-aspeed-i2c-ic.c
@@ -0,0 +1,102 @@
+/*
+ *  Aspeed 24XX/25XX I2C Interrupt Controller.
+ *
+ *  Copyright (C) 2012-2017 ASPEED Technology Inc.
+ *  Copyright 2017 IBM Corporation
+ *  Copyright 2017 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/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+
+
+#define ASPEED_I2C_IC_NUM_BUS 14
+
+struct aspeed_i2c_ic {
+	void __iomem		*base;
+	int			parent_irq;
+	struct irq_domain	*irq_domain;
+};
+
+/*
+ * The aspeed chip provides a single hardware interrupt for all of the I2C
+ * busses, so we use a dummy interrupt chip to translate this single interrupt
+ * into multiple interrupts, each associated with a single I2C bus.
+ */
+static void aspeed_i2c_ic_irq_handler(struct irq_desc *desc)
+{
+	struct aspeed_i2c_ic *i2c_ic = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned long bit, status;
+	unsigned int bus_irq;
+
+	chained_irq_enter(chip, desc);
+	status = readl(i2c_ic->base);
+	for_each_set_bit(bit, &status, ASPEED_I2C_IC_NUM_BUS) {
+		bus_irq = irq_find_mapping(i2c_ic->irq_domain, bit);
+		generic_handle_irq(bus_irq);
+	}
+	chained_irq_exit(chip, desc);
+}
+
+/*
+ * Set simple handler and mark IRQ as valid. Nothing interesting to do here
+ * since we are using a dummy interrupt chip.
+ */
+static int aspeed_i2c_ic_map_irq_domain(struct irq_domain *domain,
+					unsigned int irq, irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
+	irq_set_chip_data(irq, domain->host_data);
+
+	return 0;
+}
+
+static const struct irq_domain_ops aspeed_i2c_ic_irq_domain_ops = {
+	.map = aspeed_i2c_ic_map_irq_domain,
+};
+
+static int __init aspeed_i2c_ic_of_init(struct device_node *node,
+					struct device_node *parent)
+{
+	struct aspeed_i2c_ic *i2c_ic;
+
+	i2c_ic = kzalloc(sizeof(*i2c_ic), GFP_KERNEL);
+	if (!i2c_ic)
+		return -ENOMEM;
+
+	i2c_ic->base = of_iomap(node, 0);
+	if (IS_ERR(i2c_ic->base))
+		return PTR_ERR(i2c_ic->base);
+
+	i2c_ic->parent_irq = irq_of_parse_and_map(node, 0);
+	if (i2c_ic->parent_irq < 0)
+		return i2c_ic->parent_irq;
+
+	i2c_ic->irq_domain = irq_domain_add_linear(
+			node, ASPEED_I2C_IC_NUM_BUS,
+			&aspeed_i2c_ic_irq_domain_ops, NULL);
+	if (!i2c_ic->irq_domain)
+		return -ENOMEM;
+
+	i2c_ic->irq_domain->name = "ast-i2c-domain";
+
+	irq_set_chained_handler_and_data(i2c_ic->parent_irq,
+					 aspeed_i2c_ic_irq_handler, i2c_ic);
+
+	pr_info("i2c controller registered, irq %d\n", i2c_ic->parent_irq);
+
+	return 0;
+}
+
+IRQCHIP_DECLARE(ast2400_i2c_ic, "aspeed,ast2400-i2c-ic", aspeed_i2c_ic_of_init);
+IRQCHIP_DECLARE(ast2500_i2c_ic, "aspeed,ast2500-i2c-ic", aspeed_i2c_ic_of_init);
-- 
2.12.2.564.g063fe858b8-goog

--
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

Re: [PATCH v6 2/5] irqchip/aspeed-i2c-ic: Add I2C IRQ controller for Aspeed

[Marc Zyngier]

Hi Brendan,

On 28/03/17 06:12, Brendan Higgins wrote:
> The Aspeed 24XX/25XX chips share a single hardware interrupt across 14
> separate I2C busses. This adds a dummy irqchip which maps the single
> hardware interrupt to software interrupts for each of the busses.
> 
> Signed-off-by: Brendan Higgins <brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> ---
> Added in v6:
>   - Pulled "aspeed_i2c_controller" out into a interrupt controller since that is
>     what it actually does.
> ---
>  drivers/irqchip/Makefile            |   2 +-
>  drivers/irqchip/irq-aspeed-i2c-ic.c | 102 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 103 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/irqchip/irq-aspeed-i2c-ic.c
> 
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 152bc40b6762..c136c2bd1761 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -74,6 +74,6 @@ obj-$(CONFIG_MVEBU_ODMI)		+= irq-mvebu-odmi.o
>  obj-$(CONFIG_MVEBU_PIC)			+= irq-mvebu-pic.o
>  obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
>  obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
> -obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
> +obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o
>  obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
>  obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
> diff --git a/drivers/irqchip/irq-aspeed-i2c-ic.c b/drivers/irqchip/irq-aspeed-i2c-ic.c
> new file mode 100644
> index 000000000000..59c50b28dec0
> --- /dev/null
> +++ b/drivers/irqchip/irq-aspeed-i2c-ic.c
> @@ -0,0 +1,102 @@
> +/*
> + *  Aspeed 24XX/25XX I2C Interrupt Controller.
> + *
> + *  Copyright (C) 2012-2017 ASPEED Technology Inc.
> + *  Copyright 2017 IBM Corporation
> + *  Copyright 2017 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/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/io.h>
> +
> +
> +#define ASPEED_I2C_IC_NUM_BUS 14
> +
> +struct aspeed_i2c_ic {
> +	void __iomem		*base;
> +	int			parent_irq;
> +	struct irq_domain	*irq_domain;
> +};
> +
> +/*
> + * The aspeed chip provides a single hardware interrupt for all of the I2C
> + * busses, so we use a dummy interrupt chip to translate this single interrupt
> + * into multiple interrupts, each associated with a single I2C bus.
> + */
> +static void aspeed_i2c_ic_irq_handler(struct irq_desc *desc)
> +{
> +	struct aspeed_i2c_ic *i2c_ic = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned long bit, status;
> +	unsigned int bus_irq;
> +
> +	chained_irq_enter(chip, desc);
> +	status = readl(i2c_ic->base);
> +	for_each_set_bit(bit, &status, ASPEED_I2C_IC_NUM_BUS) {
> +		bus_irq = irq_find_mapping(i2c_ic->irq_domain, bit);
> +		generic_handle_irq(bus_irq);
> +	}
> +	chained_irq_exit(chip, desc);
> +}
> +
> +/*
> + * Set simple handler and mark IRQ as valid. Nothing interesting to do here
> + * since we are using a dummy interrupt chip.
> + */
> +static int aspeed_i2c_ic_map_irq_domain(struct irq_domain *domain,
> +					unsigned int irq, irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
> +	irq_set_chip_data(irq, domain->host_data);
> +
> +	return 0;
> +}

I'm a bit concerned by this. It means that you can't even mask an
interrupt. Is that really what you intend to do? Or all that the HW can
do? If you cannot mask an interrupt, you're at the mercy of a screaming
device...

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
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

Re: [PATCH v6 2/5] irqchip/aspeed-i2c-ic: Add I2C IRQ controller for Aspeed

[Benjamin Herrenschmidt]

On Tue, 2017-03-28 at 09:32 +0100, Marc Zyngier wrote:
> I'm a bit concerned by this. It means that you can't even mask an
> interrupt. Is that really what you intend to do? Or all that the HW can
> do? If you cannot mask an interrupt, you're at the mercy of a screaming
> device...

This is not really an interrupt controller. It's a "summary" register
that reflects the state of the 14 i2c controller interrupts.

This approach does have the advantage of providing separate counters in
/proc/interrupts which is rather nice, but it does have overhead. On
those shittly little ARMv9 400Mhz cores it can be significant.

I would personally have some kind of trick to register a single
interrupt handler that calls directly the handlers of the respective
i2c busses via a simple indirection for speed, maybe adding my custom
sysfs or debugfs statistics. But that's just me trying to suck the last
cycle out of the bloody thing ;-)

Cheers,
Ben.

Re: [PATCH v6 2/5] irqchip/aspeed-i2c-ic: Add I2C IRQ controller for Aspeed

[Marc Zyngier]

On 28/03/17 10:12, Benjamin Herrenschmidt wrote:
> On Tue, 2017-03-28 at 09:32 +0100, Marc Zyngier wrote:
>> I'm a bit concerned by this. It means that you can't even mask an
>> interrupt. Is that really what you intend to do? Or all that the HW can
>> do? If you cannot mask an interrupt, you're at the mercy of a screaming
>> device...
> 
> This is not really an interrupt controller. It's a "summary" register
> that reflects the state of the 14 i2c controller interrupts.
> 
> This approach does have the advantage of providing separate counters in
> /proc/interrupts which is rather nice, but it does have overhead. On
> those shittly little ARMv9 400Mhz cores it can be significant.

<pedantic>
s/ARMv9/ARM9/, as we're still on variations of the ARMv8 architecture ;-)
</pedantic>

A 400MHz ARM9 (which is either ARMv4 or ARMv5) is not too bad (hey, we
still have a couple of Versatile-ABs here...). Caches are pretty small
though.

> I would personally have some kind of trick to register a single
> interrupt handler that calls directly the handlers of the respective
> i2c busses via a simple indirection for speed, maybe adding my custom
> sysfs or debugfs statistics. But that's just me trying to suck the last
> cycle out of the bloody thing ;-)

I'd hope the irqdomain itself to be pretty light (the revmap should help
here), but of course you're going to do more work. Counters also come at
a cost. It'd be interesting to see if Brendan has any overhead data
about this.

Cheers,

	M.
-- 
Jazz is not dead. It just smells funny...
--
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

Re: [PATCH v6 2/5] irqchip/aspeed-i2c-ic: Add I2C IRQ controller for Aspeed

[Benjamin Herrenschmidt]

On Tue, 2017-03-28 at 10:40 +0100, Marc Zyngier wrote:
> On 28/03/17 10:12, Benjamin Herrenschmidt wrote:
> > On Tue, 2017-03-28 at 09:32 +0100, Marc Zyngier wrote:
> > > I'm a bit concerned by this. It means that you can't even mask an
> > > interrupt. Is that really what you intend to do? Or all that the HW can
> > > do? If you cannot mask an interrupt, you're at the mercy of a screaming
> > > device...
> > 
> > This is not really an interrupt controller. It's a "summary" register
> > that reflects the state of the 14 i2c controller interrupts.
> > 
> > This approach does have the advantage of providing separate counters in
> > /proc/interrupts which is rather nice, but it does have overhead. On
> > those shittly little ARMv9 400Mhz cores it can be significant.
> 
> <pedantic>
> s/ARMv9/ARM9/, as we're still on variations of the ARMv8 architecture ;-)
> </pedantic>

It was a typo, I meant ARM9/ARMv5 :-)

The 2 SOC families we are talking about (Aspeed 24xx and 25xx) are
based on a ARM926EJ at 400Mhz and an ARM1176JZFS at 800Mhz
respectively, so cycles do count :-)
 
> A 400MHz ARM9 (which is either ARMv4 or ARMv5) is not too bad (hey, we
> still have a couple of Versatile-ABs here...). Caches are pretty small
> though.

16K/16K, no L2 :) 

> > I would personally have some kind of trick to register a single
> > interrupt handler that calls directly the handlers of the respective
> > i2c busses via a simple indirection for speed, maybe adding my custom
> > sysfs or debugfs statistics. But that's just me trying to suck the last
> > cycle out of the bloody thing ;-)
> 
> I'd hope the irqdomain itself to be pretty light (the revmap should help
> here), but of course you're going to do more work. Counters also come at
> a cost. It'd be interesting to see if Brendan has any overhead data
> about this.

Thankfully, the HW supports buffered sends/receive or even DMA. The
current patch doesn't yet support these but they would be a good way to
alleviate the cost of the interrupts if it becomes a problem.

Cheers,
Ben.

> Cheers,
> 
> 	M.
--
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

Re: [PATCH v6 2/5] irqchip/aspeed-i2c-ic: Add I2C IRQ controller for Aspeed

[Brendan Higgins]

The main reason I took this approach is just because I thought it was
cleaner from the perspective of the busses which are totally
independent (except for the fact that they share a single hardware
interrupt).

I did not make any measurements, so I doubt that I have anything to
add that you don't already know. I saw other usages of chained
interrupts that do the same thing (scan a "status" register and use
them to make software interrupts) and I thought that is basically what
the dummy irq chip code is for. The only thing I thought I was doing
that was novel was actually breaking out the dummy irqchip into its
own driver; it is not my idea, but I do think makes it a lot cleaner.
Nevertheless, it should be cheap in terms of number of instructions;
the most expensive part looks like looking up the mapping. In any
case, I think the low hanging fruit here is supporting buffering or
DMA, like Ben suggested.

To address the comment on being over engineered: outside of the init
function (which would exist regardless of how we do this, if not here
then in the I2C driver); the code is actually pretty small and
generic.

All that being said, it would not be very hard to do this without
using the dummy irqchip code and it would definitely be smaller in
terms of indirection and space used, but I think the code would
actually be more complicated to read. We would be going back to having
an I2C controller along with the I2C busses; where all the I2C
controller does is read the IRQ register and then call the appropriate
bus irq handler, which looks a lot like a dummy irqchip.

Cheers
--
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

Re: [PATCH v6 2/5] irqchip/aspeed-i2c-ic: Add I2C IRQ controller for Aspeed

[Marc Zyngier]

On 29/03/17 10:59, Brendan Higgins wrote:
> The main reason I took this approach is just because I thought it was
> cleaner from the perspective of the busses which are totally
> independent (except for the fact that they share a single hardware
> interrupt).
> 
> I did not make any measurements, so I doubt that I have anything to
> add that you don't already know. I saw other usages of chained
> interrupts that do the same thing (scan a "status" register and use
> them to make software interrupts) and I thought that is basically what
> the dummy irq chip code is for. The only thing I thought I was doing
> that was novel was actually breaking out the dummy irqchip into its
> own driver; it is not my idea, but I do think makes it a lot cleaner.
> Nevertheless, it should be cheap in terms of number of instructions;
> the most expensive part looks like looking up the mapping. In any
> case, I think the low hanging fruit here is supporting buffering or
> DMA, like Ben suggested.
> 
> To address the comment on being over engineered: outside of the init
> function (which would exist regardless of how we do this, if not here
> then in the I2C driver); the code is actually pretty small and
> generic.
> 
> All that being said, it would not be very hard to do this without
> using the dummy irqchip code and it would definitely be smaller in
> terms of indirection and space used, but I think the code would
> actually be more complicated to read. We would be going back to having
> an I2C controller along with the I2C busses; where all the I2C
> controller does is read the IRQ register and then call the appropriate
> bus irq handler, which looks a lot like a dummy irqchip.

As long as you're happy with the performance and the restrictions that
come attached to the HW, I'm happy to take the irqchip patches.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v6 2/5] irqchip/aspeed-i2c-ic: Add I2C IRQ controller for Aspeed

[Benjamin Herrenschmidt]

On Mon, 2017-03-27 at 22:12 -0700, Brendan Higgins wrote:
> The Aspeed 24XX/25XX chips share a single hardware interrupt across
> 14
> separate I2C busses. This adds a dummy irqchip which maps the single
> hardware interrupt to software interrupts for each of the busses.
> 
> Signed-off-by: Brendan Higgins <brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

I do think as I said earlier that is' a tiny bit overkill, I do worry
about the overhead of the added layer of indirections on a 400Mhz ARMv9
(AST2400) core but otherwise:

Acked-by: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>

> ---
> Added in v6:
>   - Pulled "aspeed_i2c_controller" out into a interrupt controller
> since that is
>     what it actually does.
> ---
>  drivers/irqchip/Makefile            |   2 +-
>  drivers/irqchip/irq-aspeed-i2c-ic.c | 102
> ++++++++++++++++++++++++++++++++++++
>  2 files changed, 103 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/irqchip/irq-aspeed-i2c-ic.c
> 
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 152bc40b6762..c136c2bd1761 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -74,6 +74,6 @@ obj-$(CONFIG_MVEBU_ODMI)		+= irq-
> mvebu-odmi.o
>  obj-$(CONFIG_MVEBU_PIC)			+= irq-mvebu-pic.o
>  obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
>  obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
> -obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
> +obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-
> aspeed-i2c-ic.o
>  obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
>  obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-
> combiner.o
> diff --git a/drivers/irqchip/irq-aspeed-i2c-ic.c
> b/drivers/irqchip/irq-aspeed-i2c-ic.c
> new file mode 100644
> index 000000000000..59c50b28dec0
> --- /dev/null
> +++ b/drivers/irqchip/irq-aspeed-i2c-ic.c
> @@ -0,0 +1,102 @@
> +/*
> + *  Aspeed 24XX/25XX I2C Interrupt Controller.
> + *
> + *  Copyright (C) 2012-2017 ASPEED Technology Inc.
> + *  Copyright 2017 IBM Corporation
> + *  Copyright 2017 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/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/io.h>
> +
> +
> +#define ASPEED_I2C_IC_NUM_BUS 14
> +
> +struct aspeed_i2c_ic {
> +	void __iomem		*base;
> +	int			parent_irq;
> +	struct irq_domain	*irq_domain;
> +};
> +
> +/*
> + * The aspeed chip provides a single hardware interrupt for all of
> the I2C
> + * busses, so we use a dummy interrupt chip to translate this single
> interrupt
> + * into multiple interrupts, each associated with a single I2C bus.
> + */
> +static void aspeed_i2c_ic_irq_handler(struct irq_desc *desc)
> +{
> +	struct aspeed_i2c_ic *i2c_ic =
> irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned long bit, status;
> +	unsigned int bus_irq;
> +
> +	chained_irq_enter(chip, desc);
> +	status = readl(i2c_ic->base);
> +	for_each_set_bit(bit, &status, ASPEED_I2C_IC_NUM_BUS) {
> +		bus_irq = irq_find_mapping(i2c_ic->irq_domain, bit);
> +		generic_handle_irq(bus_irq);
> +	}
> +	chained_irq_exit(chip, desc);
> +}
> +
> +/*
> + * Set simple handler and mark IRQ as valid. Nothing interesting to
> do here
> + * since we are using a dummy interrupt chip.
> + */
> +static int aspeed_i2c_ic_map_irq_domain(struct irq_domain *domain,
> +					unsigned int irq,
> irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_and_handler(irq, &dummy_irq_chip,
> handle_simple_irq);
> +	irq_set_chip_data(irq, domain->host_data);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops aspeed_i2c_ic_irq_domain_ops = {
> +	.map = aspeed_i2c_ic_map_irq_domain,
> +};
> +
> +static int __init aspeed_i2c_ic_of_init(struct device_node *node,
> +					struct device_node *parent)
> +{
> +	struct aspeed_i2c_ic *i2c_ic;
> +
> +	i2c_ic = kzalloc(sizeof(*i2c_ic), GFP_KERNEL);
> +	if (!i2c_ic)
> +		return -ENOMEM;
> +
> +	i2c_ic->base = of_iomap(node, 0);
> +	if (IS_ERR(i2c_ic->base))
> +		return PTR_ERR(i2c_ic->base);
> +
> +	i2c_ic->parent_irq = irq_of_parse_and_map(node, 0);
> +	if (i2c_ic->parent_irq < 0)
> +		return i2c_ic->parent_irq;
> +
> +	i2c_ic->irq_domain = irq_domain_add_linear(
> +			node, ASPEED_I2C_IC_NUM_BUS,
> +			&aspeed_i2c_ic_irq_domain_ops, NULL);
> +	if (!i2c_ic->irq_domain)
> +		return -ENOMEM;
> +
> +	i2c_ic->irq_domain->name = "ast-i2c-domain";
> +
> +	irq_set_chained_handler_and_data(i2c_ic->parent_irq,
> +					 aspeed_i2c_ic_irq_handler,
> i2c_ic);
> +
> +	pr_info("i2c controller registered, irq %d\n", i2c_ic-
> >parent_irq);
> +
> +	return 0;
> +}
> +
> +IRQCHIP_DECLARE(ast2400_i2c_ic, "aspeed,ast2400-i2c-ic",
> aspeed_i2c_ic_of_init);
> +IRQCHIP_DECLARE(ast2500_i2c_ic, "aspeed,ast2500-i2c-ic",
> aspeed_i2c_ic_of_init);
--
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

Re: [PATCH v6 2/5] irqchip/aspeed-i2c-ic: Add I2C IRQ controller for Aspeed

[Joel Stanley]

On Tue, Mar 28, 2017 at 3:42 PM, Brendan Higgins
<brendanhiggins@google.com> wrote:
> +static int __init aspeed_i2c_ic_of_init(struct device_node *node,
> +                                       struct device_node *parent)
> +{
> +       struct aspeed_i2c_ic *i2c_ic;
> +
> +       i2c_ic = kzalloc(sizeof(*i2c_ic), GFP_KERNEL);
> +       if (!i2c_ic)
> +               return -ENOMEM;
> +
> +       i2c_ic->base = of_iomap(node, 0);
> +       if (IS_ERR(i2c_ic->base))
> +               return PTR_ERR(i2c_ic->base);
> +
> +       i2c_ic->parent_irq = irq_of_parse_and_map(node, 0);
> +       if (i2c_ic->parent_irq < 0)
> +               return i2c_ic->parent_irq;
> +
> +       i2c_ic->irq_domain = irq_domain_add_linear(
> +                       node, ASPEED_I2C_IC_NUM_BUS,
> +                       &aspeed_i2c_ic_irq_domain_ops, NULL);
> +       if (!i2c_ic->irq_domain)
> +               return -ENOMEM;
> +
> +       i2c_ic->irq_domain->name = "ast-i2c-domain";

Nit: Make this aspeed-i2c-domain to make this consistent with the
other Aspeed drivers in the kernel tree.

Could this irq code be embedded in the i2c driver? We took a similar
approach for the Aspeed GPIO driver, which has a similar IRQ structure
of one hardware IRQ that tells the driver to check status registers
for the precise irq source. The upside being all of the i2c code is in
the same place in the kernel tree.

Cheers,

Joel

Re: [PATCH v6 2/5] irqchip/aspeed-i2c-ic: Add I2C IRQ controller for Aspeed

[Brendan Higgins]

> Nit: Make this aspeed-i2c-domain to make this consistent with the
> other Aspeed drivers in the kernel tree.
>
> Could this irq code be embedded in the i2c driver? We took a similar
> approach for the Aspeed GPIO driver, which has a similar IRQ structure
> of one hardware IRQ that tells the driver to check status registers
> for the precise irq source. The upside being all of the i2c code is in
> the same place in the kernel tree.

In the previous version of the patch, this code was embedded in the
I2C driver as the "struct aspeed_i2c_controller;" I really did not
change anything about it other than rename some stuff and change the
init method to match what irqchip code wants. I pulled it out into a
separate driver because I was asked to by Vladimir; nevertheless, it
does turn the I2C driver into a normal platforms driver, which is
nice. Another benefit: if we put our dummy irqchip code in with the
other irqchips, it makes it easier for the irqchip people to recognize
when we are reusing the same patterns; for example, I would not at all
be surprised if there are other dummy irqchips which have the same
exact map(...) operation (I looked and did not see anything), but it
is quite possibly something that other people want to do. If we put
this stuff in drivers/irqchip, it is more likely that the irqchip
people would recognize this as a common use case. I do not think
either of these reasons I provided are particularly compelling, but I
do not think the reasons to move it out are particularly compelling
either (unless we decide we do not want make our own irq_domain).

Cheers

[PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C

[Brendan Higgins]

Added initial master support for Aspeed I2C controller. Supports
fourteen busses present in AST24XX and AST25XX BMC SoCs by Aspeed.

Signed-off-by: Brendan Higgins <brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
---
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
Changes for v4:
  - I2C adapter number is now generated dynamically unless specified in alias.
Changes for v5:
  - Removed irq_chip used to multiplex IRQ and replaced it with dummy_irq_chip
    along with some other IRQ cleanup.
  - Addressed comments from Cedric, and Vladimir, mostly stylistic things and
    using devm managed resources.
  - Increased max clock frequency before the bus is put in HighSpeed mode, as
    per Kachalov's comment.
Changes for v6:
  - No longer arbitrarily restrict bus to be slave xor master.
  - Pulled out "struct aspeed_i2c_controller" as a interrupt controller.
  - Pulled out slave support into its own commit.
  - Rewrote code that sets clock divider register because the original version
    set it incorrectly.
  - Rewrote the aspeed_i2c_master_irq handler because the old method of
    completing a completion in between restarts was too slow causing devices to
    misbehave.
  - Added support for I2C_M_RECV_LEN which I had incorrectly said was supported
    before.
  - Addressed other comments from Vladimir.
---
 drivers/i2c/busses/Kconfig      |  10 +
 drivers/i2c/busses/Makefile     |   1 +
 drivers/i2c/busses/i2c-aspeed.c | 610 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 621 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-aspeed.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 8adc0f1d7ad0..e5ea5641a874 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -326,6 +326,16 @@ config I2C_POWERMAC
 
 comment "I2C system bus drivers (mostly embedded / system-on-chip)"
 
+config I2C_ASPEED
+	tristate "Aspeed AST2xxx SoC I2C Controller"
+	depends on ARCH_ASPEED
+	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.
+
 config I2C_AT91
 	tristate "Atmel AT91 I2C Two-Wire interface (TWI)"
 	depends on ARCH_AT91
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 30b60855fbcd..e84604b9bf3b 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_I2C_HYDRA)		+= i2c-hydra.o
 obj-$(CONFIG_I2C_POWERMAC)	+= i2c-powermac.o
 
 # Embedded system I2C/SMBus host controller drivers
+obj-$(CONFIG_I2C_ASPEED)	+= i2c-aspeed.o
 obj-$(CONFIG_I2C_AT91)		+= i2c-at91.o
 obj-$(CONFIG_I2C_AU1550)	+= i2c-au1550.o
 obj-$(CONFIG_I2C_AXXIA)		+= i2c-axxia.o
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
new file mode 100644
index 000000000000..04266acc6c46
--- /dev/null
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -0,0 +1,610 @@
+/*
+ *  Aspeed 24XX/25XX I2C Interrupt Controller.
+ *
+ *  Copyright (C) 2012-2017 ASPEED Technology Inc.
+ *  Copyright 2017 IBM Corporation
+ *  Copyright 2017 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/clk.h>
+#include <linux/completion.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.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
+
+/* 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_MASTER_EN				BIT(0)
+
+/* 0x04 : I2CD Clock and AC Timing Control Register #1 */
+#define ASPEED_I2CD_TIME_SCL_HIGH_SHIFT			16
+#define ASPEED_I2CD_TIME_SCL_HIGH_MASK			GENMASK(19, 16)
+#define ASPEED_I2CD_TIME_SCL_LOW_SHIFT			12
+#define ASPEED_I2CD_TIME_SCL_LOW_MASK			GENMASK(15, 12)
+#define ASPEED_I2CD_TIME_BASE_DIVISOR_MASK		GENMASK(3, 0)
+#define ASPEED_I2CD_TIME_SCL_REG_MAX			GENMASK(3, 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_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)
+#define ASPEED_I2CD_INTR_ERROR						       \
+		(ASPEED_I2CD_INTR_ARBIT_LOSS |				       \
+		 ASPEED_I2CD_INTR_ABNORMAL |				       \
+		 ASPEED_I2CD_INTR_SCL_TIMEOUT |				       \
+		 ASPEED_I2CD_INTR_SDA_DL_TIMEOUT)
+#define ASPEED_I2CD_INTR_ALL						       \
+		(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)
+
+/* 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)
+
+enum aspeed_i2c_master_state {
+	ASPEED_I2C_MASTER_START,
+	ASPEED_I2C_MASTER_TX_FIRST,
+	ASPEED_I2C_MASTER_TX,
+	ASPEED_I2C_MASTER_RX,
+	ASPEED_I2C_MASTER_STOP,
+	ASPEED_I2C_MASTER_INACTIVE,
+};
+
+struct aspeed_i2c_bus {
+	struct i2c_adapter		adap;
+	struct device			*dev;
+	void __iomem			*base;
+	/* Synchronizes I/O mem access to base. */
+	spinlock_t			lock;
+	struct completion		cmd_complete;
+	int				irq;
+	/* Transaction state. */
+	enum aspeed_i2c_master_state	master_state;
+	struct i2c_msg			*msgs;
+	size_t				buf_index;
+	size_t				msgs_index;
+	size_t				msgs_size;
+	bool				send_stop;
+	int				cmd_err;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	struct i2c_client		*slave;
+	enum aspeed_i2c_slave_state	slave_state;
+#endif
+};
+
+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 int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
+{
+	unsigned long time_left, flags;
+	int ret = 0;
+	u32 command;
+
+	spin_lock_irqsave(&bus->lock, flags);
+	command = aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG);
+
+	if (command & ASPEED_I2CD_SDA_LINE_STS) {
+		/* Bus is idle: no recovery needed. */
+		if (command & ASPEED_I2CD_SCL_LINE_STS)
+			goto out;
+		dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n",
+			command);
+
+		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_timeout(
+				&bus->cmd_complete, bus->adap.timeout);
+
+		spin_lock_irqsave(&bus->lock, flags);
+		if (time_left == 0)
+			ret = -ETIMEDOUT;
+		else if (bus->cmd_err)
+			ret = -EIO;
+	/* Bus error. */
+	} else {
+		dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n",
+			command);
+
+		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_timeout(
+				&bus->cmd_complete, bus->adap.timeout);
+
+		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;
+}
+
+static void do_start(struct aspeed_i2c_bus *bus)
+{
+	u32 command = ASPEED_I2CD_M_START_CMD | ASPEED_I2CD_M_TX_CMD;
+	struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
+	u8 slave_addr = msg->addr << 1;
+
+	bus->master_state = ASPEED_I2C_MASTER_START;
+	bus->buf_index = 0;
+
+	if (msg->flags & I2C_M_RD) {
+		slave_addr |= 1;
+		command |= ASPEED_I2CD_M_RX_CMD;
+		/* Need to let the hardware know to NACK after RX. */
+		if (msg->len == 1 && !(msg->flags & I2C_M_RECV_LEN))
+			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);
+}
+
+static void do_stop(struct aspeed_i2c_bus *bus)
+{
+	bus->master_state = ASPEED_I2C_MASTER_STOP;
+	aspeed_i2c_write(bus, ASPEED_I2CD_M_STOP_CMD,
+			 ASPEED_I2C_CMD_REG);
+}
+
+static void aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
+{
+	struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
+	u32 irq_status, status_ack = 0, command = 0;
+	u8 recv_byte;
+
+	spin_lock(&bus->lock);
+	irq_status = aspeed_i2c_read(bus, ASPEED_I2C_INTR_STS_REG);
+
+	if (irq_status & ASPEED_I2CD_INTR_ERROR ||
+	    (!bus->msgs && bus->master_state != ASPEED_I2C_MASTER_STOP)) {
+		dev_dbg(bus->dev, "received error interrupt: 0x%08x",
+			irq_status);
+		bus->cmd_err = -EIO;
+		do_stop(bus);
+		goto out_no_complete;
+	}
+
+	if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
+		bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
+		status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
+		goto out_complete;
+	}
+
+	if (bus->master_state == ASPEED_I2C_MASTER_START) {
+		if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK)) {
+			dev_dbg(bus->dev,
+				"no slave present at %02x", msg->addr);
+			status_ack |= ASPEED_I2CD_INTR_TX_NAK;
+			bus->cmd_err = -EIO;
+			do_stop(bus);
+			goto out_no_complete;
+		} else {
+			status_ack |= ASPEED_I2CD_INTR_TX_ACK;
+			if (msg->flags & I2C_M_RD)
+				bus->master_state = ASPEED_I2C_MASTER_RX;
+			else
+				bus->master_state = ASPEED_I2C_MASTER_TX_FIRST;
+		}
+	}
+
+	switch (bus->master_state) {
+	case ASPEED_I2C_MASTER_TX:
+		if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
+			dev_dbg(bus->dev, "slave NACKed TX");
+			status_ack |= ASPEED_I2CD_INTR_TX_NAK;
+			bus->cmd_err = -EIO;
+			do_stop(bus);
+			goto out_no_complete;
+		} else if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK)) {
+			dev_err(bus->dev, "slave failed to ACK TX");
+			goto out_complete;
+		}
+		status_ack |= ASPEED_I2CD_INTR_TX_ACK;
+		/* fallthrough intended */
+	case ASPEED_I2C_MASTER_TX_FIRST:
+		if (bus->buf_index < msg->len) {
+			bus->master_state = ASPEED_I2C_MASTER_TX;
+			aspeed_i2c_write(bus, msg->buf[bus->buf_index++],
+					 ASPEED_I2C_BYTE_BUF_REG);
+			aspeed_i2c_write(bus, ASPEED_I2CD_M_TX_CMD,
+					 ASPEED_I2C_CMD_REG);
+		} else if (bus->msgs_index + 1 < bus->msgs_size) {
+			bus->msgs_index++;
+			do_start(bus);
+		} else {
+			do_stop(bus);
+		}
+		goto out_no_complete;
+	case ASPEED_I2C_MASTER_RX:
+		if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE)) {
+			dev_err(bus->dev, "master failed to RX");
+			goto out_complete;
+		}
+		status_ack |= ASPEED_I2CD_INTR_RX_DONE;
+
+		recv_byte = aspeed_i2c_read(bus, ASPEED_I2C_BYTE_BUF_REG) >> 8;
+		msg->buf[bus->buf_index++] = recv_byte;
+
+		if (msg->flags & I2C_M_RECV_LEN &&
+		    recv_byte <= I2C_SMBUS_BLOCK_MAX) {
+			msg->len = recv_byte +
+					((msg->flags & I2C_CLIENT_PEC) ? 2 : 1);
+			msg->flags &= ~I2C_M_RECV_LEN;
+		}
+
+		if (bus->buf_index < msg->len) {
+			bus->master_state = ASPEED_I2C_MASTER_RX;
+			command = ASPEED_I2CD_M_RX_CMD;
+			if (bus->buf_index + 1 == msg->len)
+				command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
+			aspeed_i2c_write(bus, command, ASPEED_I2C_CMD_REG);
+		} else if (bus->msgs_index + 1 < bus->msgs_size) {
+			bus->msgs_index++;
+			do_start(bus);
+		} else {
+			do_stop(bus);
+		}
+		goto out_no_complete;
+	case ASPEED_I2C_MASTER_STOP:
+		if (!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP)) {
+			dev_err(bus->dev, "master failed to STOP");
+			bus->cmd_err = -EIO;
+		}
+		status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
+
+		bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
+		goto out_complete;
+	case ASPEED_I2C_MASTER_INACTIVE:
+		dev_err(bus->dev,
+			"master received interrupt 0x%08x, but is inactive",
+			irq_status);
+		bus->cmd_err = -EIO;
+		goto out_complete;
+	default:
+		WARN(1, "unknown master state\n");
+		bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
+		bus->cmd_err = -EIO;
+		goto out_complete;
+	}
+
+out_complete:
+	complete(&bus->cmd_complete);
+out_no_complete:
+	if (irq_status != status_ack)
+		dev_err(bus->dev,
+			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
+			irq_status, status_ack);
+	aspeed_i2c_write(bus, irq_status, ASPEED_I2C_INTR_STS_REG);
+	spin_unlock(&bus->lock);
+}
+
+static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
+{
+	struct aspeed_i2c_bus *bus = dev_id;
+
+	aspeed_i2c_master_irq(bus);
+	return IRQ_HANDLED;
+}
+
+static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
+				  struct i2c_msg *msgs, int num)
+{
+	struct aspeed_i2c_bus *bus = adap->algo_data;
+	unsigned long time_left, flags;
+	int ret = 0;
+
+	bus->cmd_err = 0;
+
+	/* 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;
+	}
+
+	spin_lock_irqsave(&bus->lock, flags);
+	bus->msgs = msgs;
+	bus->msgs_index = 0;
+	bus->msgs_size = num;
+
+	do_start(bus);
+	reinit_completion(&bus->cmd_complete);
+	spin_unlock_irqrestore(&bus->lock, flags);
+
+	time_left = wait_for_completion_timeout(&bus->cmd_complete,
+						bus->adap.timeout);
+
+	spin_lock_irqsave(&bus->lock, flags);
+	bus->msgs = NULL;
+	if (time_left == 0)
+		ret = -ETIMEDOUT;
+	else
+		ret = bus->cmd_err;
+	spin_unlock_irqrestore(&bus->lock, flags);
+
+	/* If nothing went wrong, return number of messages transferred. */
+	if (ret >= 0)
+		return bus->msgs_index + 1;
+	else
+		return ret;
+}
+
+static u32 aspeed_i2c_functionality(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA;
+}
+
+static const struct i2c_algorithm aspeed_i2c_algo = {
+	.master_xfer	= aspeed_i2c_master_xfer,
+	.functionality	= aspeed_i2c_functionality,
+};
+
+static u32 aspeed_i2c_get_clk_reg_val(u32 divisor)
+{
+	u32 base_clk, clk_high, clk_low, tmp;
+
+	/*
+	 * The actual clock frequency of SCL is:
+	 *	SCL_freq = base_freq * (SCL_high + SCL_low)
+	 *		 = APB_freq / divisor
+	 * where base_freq is a programmable clock divider; its value is
+	 *	base_freq = 1 << base_clk
+	 * SCL_high is the number of base_freq clock cycles that SCL stays high
+	 * and SCL_low is the number of base_freq clock cycles that SCL stays
+	 * low for a period of SCL.
+	 * The actual register has a minimum SCL_high and SCL_low minimum of 1;
+	 * thus, they start counting at zero. So
+	 *	SCL_high = clk_high + 1
+	 *	SCL_low	 = clk_low + 1
+	 * Thus,
+	 *	SCL_freq = (1 << base_clk) * (clk_high + 1 + clk_low + 1)
+	 * The documentation recommends clk_high >= 8 and clk_low >= 7 when
+	 * possible; this last constraint gives us the following solution:
+	 */
+	base_clk = divisor > 32 ? ilog2(divisor / 16 - 1) : 0;
+	tmp = divisor / (1 << base_clk);
+	clk_high = tmp / 2 + tmp % 2;
+	clk_low = tmp - clk_high;
+
+	clk_high -= 1;
+	clk_low -= 1;
+
+	return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
+		& ASPEED_I2CD_TIME_SCL_HIGH_MASK)
+			| ((clk_low << ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
+			   & ASPEED_I2CD_TIME_SCL_LOW_MASK)
+			| (base_clk & ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
+}
+
+static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
+			       struct platform_device *pdev)
+{
+	u32 clk_freq, divisor;
+	struct clk *pclk;
+	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;
+	}
+	divisor = 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 > 1000) {
+		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(divisor),
+				 ASPEED_I2C_AC_TIMING_REG1);
+	} else {
+		aspeed_i2c_write(bus, aspeed_i2c_get_clk_reg_val(divisor),
+				 ASPEED_I2C_AC_TIMING_REG1);
+		aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,
+				 ASPEED_I2C_AC_TIMING_REG2);
+	}
+
+	return 0;
+}
+
+static int aspeed_i2c_probe_bus(struct platform_device *pdev)
+{
+	struct aspeed_i2c_bus *bus;
+	struct resource *res;
+	int ret;
+
+	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
+	if (!bus)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	bus->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(bus->base)) {
+		dev_err(&pdev->dev, "failed to devm_ioremap_resource\n");
+		return PTR_ERR(bus->base);
+	}
+
+	bus->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
+	ret = devm_request_irq(&pdev->dev, bus->irq, aspeed_i2c_bus_irq,
+			       IRQF_SHARED, dev_name(&pdev->dev), bus);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to request interrupt\n");
+		return ret;
+	}
+
+	/* Initialize the I2C adapter */
+	spin_lock_init(&bus->lock);
+	init_completion(&bus->cmd_complete);
+	bus->adap.owner = THIS_MODULE;
+	bus->adap.retries = 0;
+	bus->adap.timeout = 5 * HZ;
+	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");
+
+	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_ALL, ASPEED_I2C_INTR_CTRL_REG);
+
+	ret = i2c_add_adapter(&bus->adap);
+	if (ret < 0)
+		return ret;
+
+	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,
+	},
+};
+module_platform_driver(aspeed_i2c_bus_driver);
+
+MODULE_AUTHOR("Brendan Higgins <brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>");
+MODULE_DESCRIPTION("Aspeed I2C Bus Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.12.2.564.g063fe858b8-goog

--
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

Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C

[Benjamin Herrenschmidt]

On Mon, 2017-03-27 at 22:12 -0700, Brendan Higgins wrote:
> +/* 0x04 : I2CD Clock and AC Timing Control Register #1 */
> +#define ASPEED_I2CD_TIME_SCL_HIGH_SHIFT                        16
> +#define ASPEED_I2CD_TIME_SCL_HIGH_MASK                 GENMASK(19, 16)
> +#define ASPEED_I2CD_TIME_SCL_LOW_SHIFT                 12
> +#define ASPEED_I2CD_TIME_SCL_LOW_MASK                  GENMASK(15, 12)
> +#define ASPEED_I2CD_TIME_BASE_DIVISOR_MASK             GENMASK(3, 0)
> +#define ASPEED_I2CD_TIME_SCL_REG_MAX                   GENMASK(3, 0)
> +/* 0x08 : I2CD Clock and AC Timing Control Register #2 */
> +#define ASPEED_NO_TIMEOUT_CTRL                         0

Those are slightly different between the 2400 and 2500, allowing
slightly more fine grained settings (faster base clock and thus higher
numbers in high/low counts).

I *think* that using the 2400 values as-is might work ok, at least
it does for 100kHz but I would double check.

I'll review the rest tomorrow.

Cheers,
Ben.

Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C

[Benjamin Herrenschmidt]

On Mon, 2017-03-27 at 22:12 -0700, Brendan Higgins wrote:
> +       /* Set AC Timing */
> +       if (clk_freq / 1000 > 1000) {
> +               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 |

s/ASPEED_I2CD_M_SDA_DRIVE_1T_EN/ASPEED_I2CD_M_SCL_DRIVE_1T_EN/

(and in the definition too)

> +                               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(divisor),
> +                                ASPEED_I2C_AC_TIMING_REG1);
> +       } else {

I don't think that's right. AFAIK ASPEED_I2CD_M_HIGH_SPEED_EN is about
ignoring the timing register completely and going for full speed which
is a few Mhz (I forgot how much). At least from my (possibly incorrect)
reading of the spec and the SDK driver.

Or maybe that's what you intend by the above ? Anything above 1Mhz ?

I think there's a blurb somewhere that says that setting that bit makes
it ignore the timing register completely. The definition is:

<<
Enable High Speed master mode
0 : normal speed mode
1 : high speed mode (3.4Mbps)
High speed mode can only use buffer mode for transfer. And only master
mode supports speed switching capability
>>

The spec of the base clock field of the timing register also says

<<
When switch to High Speed (HS) mode, the divisor will be switch to 0 by
hardware automatically
>>

Note also that we aren't use buffer mode anyway so this can't work as-
is, we're using byte mode.

The other interesting question is what is the frequency threshold for
setting ASPEED_I2CD_M_SCL_DRIVE_1T_EN (and the SDA one) ? 

Those bits are somewhat orthogonal to ASPEED_I2CD_M_HIGH_SPEED_EN. They
make the device drive the signals for a clock when they go up to "speed
up" the rising edge more than a normal pull up would do.

If you have some fast devices, it would be interesting to scope the
signal see from what speed it becomes interesting to set the 1T enable
bits to speed up rising edges.

Cheers,
Ben.

Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C

[Brendan Higgins]

>> +                               ASPEED_I2CD_M_HIGH_SPEED_EN |
>> +                               ASPEED_I2CD_M_SDA_DRIVE_1T_EN |
>
> s/ASPEED_I2CD_M_SDA_DRIVE_1T_EN/ASPEED_I2CD_M_SCL_DRIVE_1T_EN/
>
> (and in the definition too)

Will fix.

>
>> +                               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(divisor),
>> +                                ASPEED_I2C_AC_TIMING_REG1);
>> +       } else {
>
> I don't think that's right. AFAIK ASPEED_I2CD_M_HIGH_SPEED_EN is about
> ignoring the timing register completely and going for full speed which
> is a few Mhz (I forgot how much). At least from my (possibly incorrect)
> reading of the spec and the SDK driver.
>
> Or maybe that's what you intend by the above ? Anything above 1Mhz ?
>
> I think there's a blurb somewhere that says that setting that bit makes
> it ignore the timing register completely. The definition is:
>
> <<
> Enable High Speed master mode
> 0 : normal speed mode
> 1 : high speed mode (3.4Mbps)
> High speed mode can only use buffer mode for transfer. And only master
> mode supports speed switching capability
>>>

Yeah, I was picking an arbitrary cutoff and 1MHz seemed reasonable in
part because in order to get above 1MHz you would set the divisor to 0
(1 << 0) anyway because you will only modify the SCL high and low time
for anything less than that. Also because that was the cutoff for fast
mode (as opposed to high speed).

>
> The spec of the base clock field of the timing register also says
>
> <<
> When switch to High Speed (HS) mode, the divisor will be switch to 0 by
> hardware automatically
>>>
>
> Note also that we aren't use buffer mode anyway so this can't work as-
> is, we're using byte mode.
>

Good catch. Yeah, I did not realize it. I should probably remove this
until that is supported then.

> The other interesting question is what is the frequency threshold for
> setting ASPEED_I2CD_M_SCL_DRIVE_1T_EN (and the SDA one) ?

I would guess that we should make them correspond to the cutoff for
high speed mode, or fast mode plus. Not really sure though, the
documentation is not clear on this (or a lot of other things :-P)

>
> Those bits are somewhat orthogonal to ASPEED_I2CD_M_HIGH_SPEED_EN. They
> make the device drive the signals for a clock when they go up to "speed
> up" the rising edge more than a normal pull up would do.
>
> If you have some fast devices, it would be interesting to scope the
> signal see from what speed it becomes interesting to set the 1T enable
> bits to speed up rising edges.

Agreed.

Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C

[Joel Stanley]

On Tue, Mar 28, 2017 at 3:42 PM, Brendan Higgins
<brendanhiggins@google.com> wrote:
> Added initial master support for Aspeed I2C controller. Supports
> fourteen busses present in AST24XX and AST25XX BMC SoCs by Aspeed.

Mention that the driver supports byte at a time access only at this stage.

> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>

Looking good. I've given this a spin on ast2500 hardware and it worked for me.

I've got a bunch of nits below, and one bigger question about weather
we need internal locking in the driver, or if we can rely on the i2c
core for our locks.

> ---
>  drivers/i2c/busses/Kconfig      |  10 +
>  drivers/i2c/busses/Makefile     |   1 +
>  drivers/i2c/busses/i2c-aspeed.c | 610 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 621 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-aspeed.c
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 8adc0f1d7ad0..e5ea5641a874 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -326,6 +326,16 @@ config I2C_POWERMAC
>
>  comment "I2C system bus drivers (mostly embedded / system-on-chip)"
>
> +config I2C_ASPEED
> +       tristate "Aspeed AST2xxx SoC I2C Controller"

Aspeed I2C Controller

> +       depends on ARCH_ASPEED
> +       help
> +         If you say yes to this option, support will be included for the
> +         Aspeed AST2xxx SoC I2C controller.

And again.

> +
> +         This driver can also be built as a module.  If so, the module
> +         will be called i2c-aspeed.
> +
>  config I2C_AT91
>         tristate "Atmel AT91 I2C Two-Wire interface (TWI)"
>         depends on ARCH_AT91

> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> new file mode 100644
> index 000000000000..04266acc6c46
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-aspeed.c

> +       spin_unlock_irqrestore(&bus->lock, flags);
> +
> +       return ret;
> +}
> +
> +static void do_start(struct aspeed_i2c_bus *bus)

aspeed_i2c_do_start

> +{
> +       u32 command = ASPEED_I2CD_M_START_CMD | ASPEED_I2CD_M_TX_CMD;
> +       struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
> +       u8 slave_addr = msg->addr << 1;
> +
> +       bus->master_state = ASPEED_I2C_MASTER_START;
> +       bus->buf_index = 0;
> +
> +       if (msg->flags & I2C_M_RD) {
> +               slave_addr |= 1;
> +               command |= ASPEED_I2CD_M_RX_CMD;
> +               /* Need to let the hardware know to NACK after RX. */
> +               if (msg->len == 1 && !(msg->flags & I2C_M_RECV_LEN))
> +                       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);
> +}
> +
> +static void do_stop(struct aspeed_i2c_bus *bus)

aspeed_i2c_do_stop

> +{
> +       bus->master_state = ASPEED_I2C_MASTER_STOP;
> +       aspeed_i2c_write(bus, ASPEED_I2CD_M_STOP_CMD,
> +                        ASPEED_I2C_CMD_REG);
> +}

> +static int aspeed_i2c_probe_bus(struct platform_device *pdev)
> +{
> +       struct aspeed_i2c_bus *bus;
> +       struct resource *res;
> +       int ret;
> +
> +       bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
> +       if (!bus)
> +               return -ENOMEM;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       bus->base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(bus->base)) {
> +               dev_err(&pdev->dev, "failed to devm_ioremap_resource\n");

devm_ioremap_resource shows an error for you, please drop the dev_err here.

> +               return PTR_ERR(bus->base);
> +       }
> +
> +       bus->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> +       ret = devm_request_irq(&pdev->dev, bus->irq, aspeed_i2c_bus_irq,
> +                              IRQF_SHARED, dev_name(&pdev->dev), bus);

Is this requesting an IRQ from your i2c-irq-controller? In which case
the IRQ won't be shared with any other driver, so you don't need to
set IRQF_SHARED.

> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "failed to request interrupt\n");
> +               return ret;
> +       }
> +
> +       /* Initialize the I2C adapter */
> +       spin_lock_init(&bus->lock);

Do we need this lock at all?

The i2c core provides locking around operations on the bus. I was
browsing some of the other bus drivers and they do not have locking
inside of the driver (eg. i2c-at91.c). I also did a test of an earlier
version of this driver where I removed the locks, and it performed
correctly in my testing (http://patchwork.ozlabs.org/patch/731899/).

> +       init_completion(&bus->cmd_complete);
> +       bus->adap.owner = THIS_MODULE;
> +       bus->adap.retries = 0;
> +       bus->adap.timeout = 5 * HZ;
> +       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");

> +static struct platform_driver aspeed_i2c_bus_driver = {
> +       .probe          = aspeed_i2c_probe_bus,
> +       .remove         = aspeed_i2c_remove_bus,
> +       .driver         = {
> +               .name           = "ast-i2c-bus",

aspeed-i2c-bus please.

> +               .of_match_table = aspeed_i2c_bus_of_table,
> +       },
> +};
> +module_platform_driver(aspeed_i2c_bus_driver);
> +
> +MODULE_AUTHOR("Brendan Higgins <brendanhiggins@google.com>");
> +MODULE_DESCRIPTION("Aspeed I2C Bus Driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.12.2.564.g063fe858b8-goog
>

Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C

[Benjamin Herrenschmidt]

Allright, I finally found some time for reviewing some of this
after splitting the ftgmac100 patch into 54 smaller ones :)

On Mon, 2017-03-27 at 22:12 -0700, Brendan Higgins wrote:

 .../...

> +struct aspeed_i2c_bus {
> +	struct i2c_adapter		adap;
> +	struct device			*dev;
> +	void __iomem			*base;
> +	/* Synchronizes I/O mem access to base. */
> +	spinlock_t			lock;

I am not entirely convinced we need that lock. The i2c core will
take a mutex protecting all operations on the bus. So we only need
to synchronize between our "xfer" code and our interrupt handler.

This probably be done without a lock if we are careful. Not a huge
deal though as Aspeed SoC are currently not SMP so the lock compiles
down to not much unless you have all the debug crap enabled :-)

> +	struct completion		cmd_complete;
> +	int				irq;
> +	/* Transaction state. */
> +	enum aspeed_i2c_master_state	master_state;
> +	struct i2c_msg			*msgs;
> +	size_t				buf_index;
> +	size_t				msgs_index;
> +	size_t				msgs_size;
> +	bool				send_stop;
> +	int				cmd_err;
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +	struct i2c_client		*slave;
> +	enum aspeed_i2c_slave_state	slave_state;
> +#endif
> +};

Minor nit but the above should probably be in the slave patch no ?

> +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);
> +}

Another very minor nit, I'm not certain those accessors are a big
win in code size and/or readability but keep them if you want.

> +static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
> +{
> +	unsigned long time_left, flags;
> +	int ret = 0;
> +	u32 command;
> +
> +	spin_lock_irqsave(&bus->lock, flags);
> +	command = aspeed_i2c_read(bus, ASPEED_I2C_CMD_REG);
> +
> +	if (command & ASPEED_I2CD_SDA_LINE_STS) {
> +		/* Bus is idle: no recovery needed. */
> +		if (command & ASPEED_I2CD_SCL_LINE_STS)
> +			goto out;
> +		dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n",
> +			command);
> +
> +		aspeed_i2c_write(bus, ASPEED_I2CD_M_STOP_CMD,
> +				 ASPEED_I2C_CMD_REG);
> +		reinit_completion(&bus->cmd_complete);
> +		spin_unlock_irqrestore(&bus->lock, flags);

See my comment further down in master_xfer, do the reinit before sending
the command, even if currently the lock protects you, it's cleaner.

Now, I don't completely get how your interrupt handler deals with these
"message-less" completions. See the review of the interrupt handler.

> +
> +		time_left = wait_for_completion_timeout(
> +				&bus->cmd_complete, bus->adap.timeout);
> +
> +		spin_lock_irqsave(&bus->lock, flags);
> +		if (time_left == 0)
> +			ret = -ETIMEDOUT;
> +		else if (bus->cmd_err)
> +			ret = -EIO;
> +	/* Bus error. */
> +	} else {
> +		dev_dbg(bus->dev, "bus hung (state %x), attempting recovery\n",
> +			command);
> +
> +		aspeed_i2c_write(bus, ASPEED_I2CD_BUS_RECOVER_CMD,
> +				 ASPEED_I2C_CMD_REG);
> +		reinit_completion(&bus->cmd_complete);

Same comments as above.

> +		spin_unlock_irqrestore(&bus->lock, flags);
> +
> +		time_left = wait_for_completion_timeout(
> +				&bus->cmd_complete, bus->adap.timeout);
> +
> +		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;
> +	}

Some of those error states probably also warrant a reset of the controller,
I think aspeed does that in the SDK.

> +out:
> +	spin_unlock_irqrestore(&bus->lock, flags);
> +
> +	return ret;
> +}
> +
> +static void do_start(struct aspeed_i2c_bus *bus)
> +{
> +	u32 command = ASPEED_I2CD_M_START_CMD | ASPEED_I2CD_M_TX_CMD;
> +	struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
> +	u8 slave_addr = msg->addr << 1;
> +
> +	bus->master_state = ASPEED_I2C_MASTER_START;
> +	bus->buf_index = 0;
> +
> +	if (msg->flags & I2C_M_RD) {
> +		slave_addr |= 1;
> +		command |= ASPEED_I2CD_M_RX_CMD;
> +		/* Need to let the hardware know to NACK after RX. */
> +		if (msg->len == 1 && !(msg->flags & I2C_M_RECV_LEN))
> +			command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
> +	}


What about I2C_M_NOSTART ? 

Not that I've ever seen it used... ;-)

> +	aspeed_i2c_write(bus, slave_addr, ASPEED_I2C_BYTE_BUF_REG);
> +	aspeed_i2c_write(bus, command, ASPEED_I2C_CMD_REG);
> +}
> +
> +static void do_stop(struct aspeed_i2c_bus *bus)
> +{
> +	bus->master_state = ASPEED_I2C_MASTER_STOP;
> +	aspeed_i2c_write(bus, ASPEED_I2CD_M_STOP_CMD,
> +			 ASPEED_I2C_CMD_REG);
> +}
> +
> +static void aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> +{
> +	struct i2c_msg *msg = &bus->msgs[bus->msgs_index];
> +	u32 irq_status, status_ack = 0, command = 0;
> +	u8 recv_byte;

If your lock means anything you should probably capture bus->msgs[..]
with the lock held. That said, see my previous comment about the
lock possibly not being terribly useful.

Additionally, if you are doing a bus recovery, won't you be messing
around with a stale or NULL bus->msgs ?

I would at the very least make it

	msg = bus->msgs ? &bus->msgs[bus->msgs_index] : NULL;

That way msg is NULL in the recovery case rather than a random
crap pointer.

> +	spin_lock(&bus->lock);
> +	irq_status = aspeed_i2c_read(bus, ASPEED_I2C_INTR_STS_REG);
>

I would "ack" (write back to INTR_STS_REG) immediately. Otherwise
you have a race between status bits set as a result of what happened
before the interrupt handler vs. as a result of what you did.

For example, take TX. You get the TX bit in irq_status. You start
a new character transmission bcs there's more to send *then* you ack
the TX bit. That's racy. If that new transmission is fast enough,
you'll end up acking the wrong one. Again this is extremely unlikely
but code should be written in a way that is completely fool proof
from such races. They can happen for stupid reasons, such as huge
bus delays caused by a peripheral, FIQ going bonkers etc...

In general, you always ACK all interrupts first. Then you handle
the bits you have harvested.

> +	if (irq_status & ASPEED_I2CD_INTR_ERROR ||
> +	    (!bus->msgs && bus->master_state != ASPEED_I2C_MASTER_STOP)) {

What happen with recovery completion here ?

Won't we hit !bus->msgs && master state != stop ? Especially
if we hit a timeout where we haven't cleaned up any of our state.

> +		dev_dbg(bus->dev, "received error interrupt: 0x%08x",
> +			irq_status);

This is confusing too in the case of master_state != stop ... any
interrupt will trigger that. I think it would be worthwhile either
commenting a bit more here or having clearer messages depending
on the condition.

> +		bus->cmd_err = -EIO;
> +		do_stop(bus);
> +		goto out_no_complete;
> +	}
> +
> +	if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
> +		bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> +		status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
> +		goto out_complete;
> +	}

I would set master_state to "RECOVERY" (new state ?) and ensure
those things are caught if they happen outside of a recovery.

> +	if (bus->master_state == ASPEED_I2C_MASTER_START) {

Here a comment would be handy as to why you do this before the
switch/case. I understand why but it makes reading the code by
somebody else easier.

> +		if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK)) {

Minor nit:

		if (unlikely(error case)) {
			...
			goto out;
		}
		...

Ie, you don't need the "else", and you make it clear that this
is an error case, allowing the compiler to potentially optimize
the likely branch.

In fact, I would have that on all the error cases above too.

I understand now why you have that 'status_ack'. You are trying
to catch the bits that may be set that shouldn't be.

I think you should still "ack early". However, you could have
status_ack called status_handled or something like that and
at the end, still catch "spurrious" bits.

That said, I notice a lot of duplication in your state machine.

You basically have each state starting with

	if (didn't get the bit I wanted) {
		error
	}

You are also not very consistent as to whether you generate a
stop as a result or not.

I would happily simplify that state machine by just completing
with an error and letting master_xfer() do a stop when done but
if you like to keep it the way it is, you could have a common
goto label that handle error + stop.

> +			dev_dbg(bus->dev,
> +				"no slave present at %02x", msg->addr);
> +			status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> +			bus->cmd_err = -EIO;
> +			do_stop(bus);
> +			goto out_no_complete;
> +		} else {
> +			status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> +			if (msg->flags & I2C_M_RD)
> +				bus->master_state = ASPEED_I2C_MASTER_RX;
> +			else
> +				bus->master_state = ASPEED_I2C_MASTER_TX_FIRST;

What about the SMBUS_QUICK case ? (0-len transfer). Do we need
to handle this here ? A quick look at the TX_FIRST case makes
me think we are ok there but I'm not sure about the RX case.

I'm not sure the RX case is tight also. What completion does the
HW give you for the address cycle ? Won't you get that before it
has received the first character ? IE. You fall through to
the read case of the state machine with the read potentially
not complete yet no ?

> +		}
> +	}
> +
> +	switch (bus->master_state) {
> +	case ASPEED_I2C_MASTER_TX:
> +		if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
> +			dev_dbg(bus->dev, "slave NACKed TX");
> +			status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> +			bus->cmd_err = -EIO;
> +			do_stop(bus);
> +			goto out_no_complete;

As I said earlier, I would factor all the error cases. I would also
not worry too much about checking that the status bits meet expectation
in the error path.

> +		} else if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
> {
> +			dev_err(bus->dev, "slave failed to ACK TX");
> +			goto out_complete;

You should still stop.

> +		}
> +		status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> +		/* fallthrough intended */
> +	case ASPEED_I2C_MASTER_TX_FIRST:
> +		if (bus->buf_index < msg->len) {
> +			bus->master_state = ASPEED_I2C_MASTER_TX;
> +			aspeed_i2c_write(bus, msg->buf[bus->buf_index++],
> +					 ASPEED_I2C_BYTE_BUF_REG);
> +			aspeed_i2c_write(bus, ASPEED_I2CD_M_TX_CMD,
> +					 ASPEED_I2C_CMD_REG);
> +		} else if (bus->msgs_index + 1 < bus->msgs_size) {
> +			bus->msgs_index++;
> +			do_start(bus);
> +		} else {
> +			do_stop(bus);
> +		}
> +		goto out_no_complete;
> +	case ASPEED_I2C_MASTER_RX:
> +		if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE)) {
> +			dev_err(bus->dev, "master failed to RX");
> +			goto out_complete;
> +		}

See my comment above for a bog standard i2c_read. Aren't you getting
the completion for the address before the read is even started ?

> +		status_ack |= ASPEED_I2CD_INTR_RX_DONE;
> +
> +		recv_byte = aspeed_i2c_read(bus, ASPEED_I2C_BYTE_BUF_REG) >> 8;
> +		msg->buf[bus->buf_index++] = recv_byte;
> +
> +		if (msg->flags & I2C_M_RECV_LEN &&
> +		    recv_byte <= I2C_SMBUS_BLOCK_MAX) {
> +			msg->len = recv_byte +
> +					((msg->flags & I2C_CLIENT_PEC) ? 2 : 1);
> +			msg->flags &= ~I2C_M_RECV_LEN;
> +		}

You need to error out with -EPROTO if the size is too large.

> +
> +		if (bus->buf_index < msg->len) {
> +			bus->master_state = ASPEED_I2C_MASTER_RX;
> +			command = ASPEED_I2CD_M_RX_CMD;
> +			if (bus->buf_index + 1 == msg->len)
> +				command |= ASPEED_I2CD_M_S_RX_CMD_LAST;
> +			aspeed_i2c_write(bus, command, ASPEED_I2C_CMD_REG);
> +		} else if (bus->msgs_index + 1 < bus->msgs_size) {
> +			bus->msgs_index++;
> +			do_start(bus);
> +		} else {
> +			do_stop(bus);
> +		}

You have some duplication. You could have your "completed message,
switch to the next one" be either a helper or another goto statement.

I would do a little helper that check the index and calls stop or
start.

> +		goto out_no_complete;
> +	case ASPEED_I2C_MASTER_STOP:
> +		if (!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP)) {
> +			dev_err(bus->dev, "master failed to STOP");
> +			bus->cmd_err = -EIO;
> +		}
> +		status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
> +
> +		bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> +		goto out_complete;
> +	case ASPEED_I2C_MASTER_INACTIVE:
> +		dev_err(bus->dev,
> +			"master received interrupt 0x%08x, but is inactive",
> +			irq_status);
> +		bus->cmd_err = -EIO;
> +		goto out_complete;
> +	default:
> +		WARN(1, "unknown master state\n");
> +		bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> +		bus->cmd_err = -EIO;
> +		goto out_complete;
> +	}
> +
> +out_complete:
> +	complete(&bus->cmd_complete);
> +out_no_complete:
> +	if (irq_status != status_ack)
> +		dev_err(bus->dev,
> +			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> +			irq_status, status_ack);
> +	aspeed_i2c_write(bus, irq_status, ASPEED_I2C_INTR_STS_REG);
> +	spin_unlock(&bus->lock);
> +}
> +
> +static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> +{
> +	struct aspeed_i2c_bus *bus = dev_id;
> +
> +	aspeed_i2c_master_irq(bus);
> +	return IRQ_HANDLED;
> +}

In theory you want to only return IRQ_HANDLED if you indeed has at
least one IRQ status bit set... Not a huge deal here but it would
be cleaner.

> +static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
> +				  struct i2c_msg *msgs, int num)
> +{
> +	struct aspeed_i2c_bus *bus = adap->algo_data;
> +	unsigned long time_left, flags;
> +	int ret = 0;
> +
> +	bus->cmd_err = 0;
> +
> +	/* 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;
> +	}
> +
> +	spin_lock_irqsave(&bus->lock, flags);

See previous comment about the lock.

I would also cleanup all the interrupts before we even start a transfer
(ie write all 1's to the interrupt status reg).

> +	bus->msgs = msgs;
> +	bus->msgs_index = 0;
> +	bus->msgs_size = num;

Minor nit: msgs_count rather than size ?

> +	do_start(bus);
> +	reinit_completion(&bus->cmd_complete);

The reinit_completion call should probably be before do_start.

Currently the spinlock avoids this being a real issue but if as I
suggest you take out the lock, then it will be racy (probably
impossible to hit in practice but still .. :-)
 
> +	spin_unlock_irqrestore(&bus->lock, flags);
> +
> +	time_left = wait_for_completion_timeout(&bus->cmd_complete,
> +						bus->adap.timeout);
> +
> +	spin_lock_irqsave(&bus->lock, flags);
> +	bus->msgs = NULL;
> +	if (time_left == 0)
> +		ret = -ETIMEDOUT;
> +	else
> +		ret = bus->cmd_err;

If we timed out we may want to sanitize the HW state. I would suggest
resetting the master. We should also sanitize master_state. I would
suggest adding a reset function that cleans everything up.

> +	spin_unlock_irqrestore(&bus->lock, flags);
> +
> +	/* If nothing went wrong, return number of messages transferred. */
> +	if (ret >= 0)
> +		return bus->msgs_index + 1;
> +	else
> +		return ret;
> +}
> +
> +static u32 aspeed_i2c_functionality(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA;
> +}
> +
> +static const struct i2c_algorithm aspeed_i2c_algo = {
> +	.master_xfer	= aspeed_i2c_master_xfer,
> +	.functionality	= aspeed_i2c_functionality,
> +};
> +
> +static u32 aspeed_i2c_get_clk_reg_val(u32 divisor)
> +{
> +	u32 base_clk, clk_high, clk_low, tmp;
> +
> +	/*
> +	 * The actual clock frequency of SCL is:
> +	 *	SCL_freq = base_freq * (SCL_high + SCL_low)
> +	 *		 = APB_freq / divisor
> +	 * where base_freq is a programmable clock divider; its value is
> +	 *	base_freq = 1 << base_clk
> +	 * SCL_high is the number of base_freq clock cycles that SCL stays high
> +	 * and SCL_low is the number of base_freq clock cycles that SCL stays
> +	 * low for a period of SCL.
> +	 * The actual register has a minimum SCL_high and SCL_low minimum of 1;
> +	 * thus, they start counting at zero. So
> +	 *	SCL_high = clk_high + 1
> +	 *	SCL_low	 = clk_low + 1
> +	 * Thus,
> +	 *	SCL_freq = (1 << base_clk) * (clk_high + 1 + clk_low + 1)
> +	 * The documentation recommends clk_high >= 8 and clk_low >= 7 when
> +	 * possible; this last constraint gives us the following solution:
> +	 */
> +	base_clk = divisor > 32 ? ilog2(divisor / 16 - 1) : 0;
> +	tmp = divisor / (1 << base_clk);
> +	clk_high = tmp / 2 + tmp % 2;
> +	clk_low = tmp - clk_high;
> +
> +	clk_high -= 1;
> +	clk_low -= 1;
> +
> +	return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
> +		& ASPEED_I2CD_TIME_SCL_HIGH_MASK)
> +			| ((clk_low << ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
> +			   & ASPEED_I2CD_TIME_SCL_LOW_MASK)
> +			| (base_clk & ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
> +}

As I think I mentioned earlier, the AST2500 has a slightly different
register layout which support larger values for high and low, thus
allowing a finer granularity.

BTW. In case you haven't, I would suggest you copy/paste the above in
a userspace app and run it for all frequency divisors and see if your
results match the aspeed table :)

> +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
> +			       struct platform_device *pdev)
> +{
> +	u32 clk_freq, divisor;
> +	struct clk *pclk;
> +	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);

See my previous comment about calling that 'bus-frequency' rather
than 'clock-frequency'.

> +	if (ret < 0) {
> +		dev_err(&pdev->dev,
> +			"Could not read clock-frequency property\n");
> +		clk_freq = 100000;
> +	}
> +	divisor = 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 > 1000) {
> +		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(divisor),
> +				 ASPEED_I2C_AC_TIMING_REG1);

I already discussed by doubts about the above. I can try to scope
it with the EVB if you don't get to it. For now I'd rather take the
code out.

We should ask aspeed from what frequency the "1T" stuff is useful.

> +	} else {
> +		aspeed_i2c_write(bus, aspeed_i2c_get_clk_reg_val(divisor),
> +				 ASPEED_I2C_AC_TIMING_REG1);
> +		aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,
> +				 ASPEED_I2C_AC_TIMING_REG2);
> +	}
> +
> +	return 0;
> +}
> +
> +static int aspeed_i2c_probe_bus(struct platform_device *pdev)
> +{
> +	struct aspeed_i2c_bus *bus;
> +	struct resource *res;
> +	int ret;
> +
> +	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	bus->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(bus->base)) {
> +		dev_err(&pdev->dev, "failed to devm_ioremap_resource\n");
> +		return PTR_ERR(bus->base);
> +	}
> +
> +	bus->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> +	ret = devm_request_irq(&pdev->dev, bus->irq, aspeed_i2c_bus_irq,
> +			       IRQF_SHARED, dev_name(&pdev->dev), bus);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to request interrupt\n");
> +		return ret;
> +	}

Again, out of paranoia, make sure the HW is reset and interrupt
off *before* you register the interrupt handler, or a HW left in
a funny state (by uboot for example) might shoot interrupts before
you are ready to take them. I would move the reset you do below
to before devm_request_irq.

> +	/* Initialize the I2C adapter */
> +	spin_lock_init(&bus->lock);
> +	init_completion(&bus->cmd_complete);
> +	bus->adap.owner = THIS_MODULE;
> +	bus->adap.retries = 0;
> +	bus->adap.timeout = 5 * HZ;
> +	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");

Another trivial one, should we put some kind of bus number
in that string ?

> +	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_ALL, ASPEED_I2C_INTR_CTRL_REG);
> +
> +	ret = i2c_add_adapter(&bus->adap);
> +	if (ret < 0)
> +		return ret;
> +
> +	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);

Out of paranoia, should we turn off the function and mask the
interrupts here just in case ?

> +	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,
> +	},
> +};
> +module_platform_driver(aspeed_i2c_bus_driver);
> +
> +MODULE_AUTHOR("Brendan Higgins <brendanhiggins-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>");
> +MODULE_DESCRIPTION("Aspeed I2C Bus Driver");
> +MODULE_LICENSE("GPL v2");
--
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

Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C

[Brendan Higgins]

>> +struct aspeed_i2c_bus {
>> +     struct i2c_adapter              adap;
>> +     struct device                   *dev;
>> +     void __iomem                    *base;
>> +     /* Synchronizes I/O mem access to base. */
>> +     spinlock_t                      lock;
>
> I am not entirely convinced we need that lock. The i2c core will
> take a mutex protecting all operations on the bus. So we only need
> to synchronize between our "xfer" code and our interrupt handler.

You are right if both having slave and master active at the same time
was not possible; however, it is. Imagine the case where the slave is
receiving a request and something in the I2C API gets called. I
suppose we could make the slave IRQ handler lock that lock, but I
think it makes more sense to have a separate lock, since we do not
control that lock making it harder to reason about. Plus, we put
ourselves in a position where an API user has access to a lock that an
interrupt handler needs to acquire, if the user does something dumb,
then we can get interrupt starvation.

>
> This probably be done without a lock if we are careful. Not a huge
> deal though as Aspeed SoC are currently not SMP so the lock compiles
> down to not much unless you have all the debug crap enabled :-)
>
>> +     struct completion               cmd_complete;
>> +     int                             irq;
>> +     /* Transaction state. */
>> +     enum aspeed_i2c_master_state    master_state;
>> +     struct i2c_msg                  *msgs;
>> +     size_t                          buf_index;
>> +     size_t                          msgs_index;
>> +     size_t                          msgs_size;
>> +     bool                            send_stop;
...
>> +             time_left = wait_for_completion_timeout(
>> +                             &bus->cmd_complete, bus->adap.timeout);
>> +
>> +             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;
>> +     }
>
> Some of those error states probably also warrant a reset of the controller,
> I think aspeed does that in the SDK.

For timeout and cmd_err, I do not see any argument against it; it
sounds like we are in a very messed up, very unknown state, so full
reset is probably the best last resort. For SDA staying pulled down, I
think we can say with reasonable confidence that some device on our
bus is behaving very badly and I am not convinced that resetting the
controller is likely to do anything to help; that being said, I really
do not have any good ideas to address that. So maybe praying and
resetting the controller is *the most reasonable thing to do.* I would
like to know what you think we should do in that case.

While I was thinking about this I also realized that the SDA line
check after recovery happens in the else branch, but SCL line check
does not happen after we attempt to STOP if SCL is hung. If we decide
to make special note SDA being hung by a device that won't let go, we
might want to make a special note that SCL is hung by a device that
won't let go. Just a thought.

>
>> +out:
...
> What about I2C_M_NOSTART ?
>
> Not that I've ever seen it used... ;-)

Right now I am not doing any of the protocol mangling options, but I
can add them in if you think it is important for initial support.

>
>> +     aspeed_i2c_write(bus, slave_addr, ASPEED_I2C_BYTE_BUF_REG);
>> +     aspeed_i2c_write(bus, command, ASPEED_I2C_CMD_REG);
>> +}
...
>
>> +     spin_lock(&bus->lock);
>> +     irq_status = aspeed_i2c_read(bus, ASPEED_I2C_INTR_STS_REG);
>>
>
> I would "ack" (write back to INTR_STS_REG) immediately. Otherwise
> you have a race between status bits set as a result of what happened
> before the interrupt handler vs. as a result of what you did.
>
> For example, take TX. You get the TX bit in irq_status. You start
> a new character transmission bcs there's more to send *then* you ack
> the TX bit. That's racy. If that new transmission is fast enough,
> you'll end up acking the wrong one. Again this is extremely unlikely
> but code should be written in a way that is completely fool proof
> from such races. They can happen for stupid reasons, such as huge
> bus delays caused by a peripheral, FIQ going bonkers etc...
>
> In general, you always ACK all interrupts first. Then you handle
> the bits you have harvested.
>

The documentation says to ACK the interrupt after handling in the RX case:

<<<
S/W needs to clear this status bit to allow next data receiving.
>>>

I will double check with Ryan to make sure TX works the same way.

>> +     if (irq_status & ASPEED_I2CD_INTR_ERROR ||
>> +         (!bus->msgs && bus->master_state != ASPEED_I2C_MASTER_STOP)) {
>
...
>
> I would set master_state to "RECOVERY" (new state ?) and ensure
> those things are caught if they happen outside of a recovery.

Let me know if you still think we need a "RECOVERY" state.

>
>> +     if (bus->master_state == ASPEED_I2C_MASTER_START) {
>
...
>
>> +                     dev_dbg(bus->dev,
>> +                             "no slave present at %02x", msg->addr);
>> +                     status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> +                     bus->cmd_err = -EIO;
>> +                     do_stop(bus);
>> +                     goto out_no_complete;
>> +             } else {
>> +                     status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> +                     if (msg->flags & I2C_M_RD)
>> +                             bus->master_state = ASPEED_I2C_MASTER_RX;
>> +                     else
>> +                             bus->master_state = ASPEED_I2C_MASTER_TX_FIRST;
>
> What about the SMBUS_QUICK case ? (0-len transfer). Do we need
> to handle this here ? A quick look at the TX_FIRST case makes
> me think we are ok there but I'm not sure about the RX case.

I did not think that there is an SMBUS_QUICK RX. Could you point me to
an example?

>
> I'm not sure the RX case is tight also. What completion does the
> HW give you for the address cycle ? Won't you get that before it
> has received the first character ? IE. You fall through to
> the read case of the state machine with the read potentially
> not complete yet no ?
...
>> +     case ASPEED_I2C_MASTER_RX:
>> +             if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE)) {
>> +                     dev_err(bus->dev, "master failed to RX");
>> +                     goto out_complete;
>> +             }
>
> See my comment above for a bog standard i2c_read. Aren't you getting
> the completion for the address before the read is even started ?

In practice no, but it is probably best to be safe :-)

>
>> +             status_ack |= ASPEED_I2CD_INTR_RX_DONE;
>> +
>> +             recv_byte = aspeed_i2c_read(bus, ASPEED_I2C_BYTE_BUF_REG) >> 8;
>> +             msg->buf[bus->buf_index++] = recv_byte;
>> +
>> +             if (msg->flags & I2C_M_RECV_LEN &&
>> +                 recv_byte <= I2C_SMBUS_BLOCK_MAX) {
>> +                     msg->len = recv_byte +
>> +                                     ((msg->flags & I2C_CLIENT_PEC) ? 2 : 1);
...
>> +     return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
>> +             & ASPEED_I2CD_TIME_SCL_HIGH_MASK)
>> +                     | ((clk_low << ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
>> +                        & ASPEED_I2CD_TIME_SCL_LOW_MASK)
>> +                     | (base_clk & ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
>> +}
>
> As I think I mentioned earlier, the AST2500 has a slightly different
> register layout which support larger values for high and low, thus
> allowing a finer granularity.

I am developing against the 2500.

> BTW. In case you haven't, I would suggest you copy/paste the above in
> a userspace app and run it for all frequency divisors and see if your
> results match the aspeed table :)

Good call.

>
>> +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
>> +                            struct platform_device *pdev)
>> +{
>> +     u32 clk_freq, divisor;
>> +     struct clk *pclk;
>> +     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);
>
> See my previous comment about calling that 'bus-frequency' rather
> than 'clock-frequency'.
>
>> +     if (ret < 0) {
>> +             dev_err(&pdev->dev,
>> +                     "Could not read clock-frequency property\n");
>> +             clk_freq = 100000;
>> +     }
>> +     divisor = 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 > 1000) {
>> +             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(divisor),
>> +                              ASPEED_I2C_AC_TIMING_REG1);
>
> I already discussed by doubts about the above. I can try to scope
> it with the EVB if you don't get to it. For now I'd rather take the
> code out.
>
> We should ask aspeed from what frequency the "1T" stuff is useful.

Will do, I will try to rope Ryan in on the next review; it will be
good for him to get used to working with upstream anyway.

>
>> +     } else {
>> +             aspeed_i2c_write(bus, aspeed_i2c_get_clk_reg_val(divisor),
>> +                              ASPEED_I2C_AC_TIMING_REG1);
>> +             aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,
>> +                              ASPEED_I2C_AC_TIMING_REG2);
>> +     }
...
>> +     spin_lock_init(&bus->lock);
>> +     init_completion(&bus->cmd_complete);
>> +     bus->adap.owner = THIS_MODULE;
>> +     bus->adap.retries = 0;
>> +     bus->adap.timeout = 5 * HZ;
>> +     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");
>
> Another trivial one, should we put some kind of bus number
> in that string ?

Whoops, looks like I missed this one; I will get to it in the next revision.

>
>> +     bus->dev = &pdev->dev;
>> +
>> +     /* reset device: disable master & slave functions */
>> +     aspeed_i2c_write(bus, 0, ASPEED_I2C_FUN_CTRL_REG);
...
--
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

Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C

[Benjamin Herrenschmidt]

On Mon, 2017-04-24 at 11:56 -0700, Brendan Higgins wrote:
> > > +struct aspeed_i2c_bus {
> > > +     struct i2c_adapter              adap;
> > > +     struct device                   *dev;
> > > +     void __iomem                    *base;
> > > +     /* Synchronizes I/O mem access to base. */
> > > +     spinlock_t                      lock;
> > 
> > I am not entirely convinced we need that lock. The i2c core will
> > take a mutex protecting all operations on the bus. So we only need
> > to synchronize between our "xfer" code and our interrupt handler.
> 
> You are right if both having slave and master active at the same time
> was not possible; however, it is.

Right, I somewhat forgot about the slave case.

  ...

> > Some of those error states probably also warrant a reset of the
> > controller,
> > I think aspeed does that in the SDK.
> 
> For timeout and cmd_err, I do not see any argument against it; it
> sounds like we are in a very messed up, very unknown state, so full
> reset is probably the best last resort.

Yup.

> For SDA staying pulled down, I
> think we can say with reasonable confidence that some device on our
> bus is behaving very badly and I am not convinced that resetting the
> controller is likely to do anything to help;

Right. Hammering with STOPs and pray ...

>  that being said, I really
> do not have any good ideas to address that. So maybe praying and
> resetting the controller is *the most reasonable thing to do.* I
> would like to know what you think we should do in that case.

Well, there's a (small ?) chance that it's a controller bug asserting
the line so ... but there's little we can do if not.

> While I was thinking about this I also realized that the SDA line
> check after recovery happens in the else branch, but SCL line check
> does not happen after we attempt to STOP if SCL is hung. If we decide
> to make special note SDA being hung by a device that won't let go, we
> might want to make a special note that SCL is hung by a device that
> won't let go. Just a thought.

Maybe. Or just "unrecoverable error"... hopefully these don't happen
too often ... We had cases of a TPM misbehaving like that.

> > > +out:
> 
> ...
> > What about I2C_M_NOSTART ?
> > 
> > Not that I've ever seen it used... ;-)
> 
> Right now I am not doing any of the protocol mangling options, but I
> can add them in if you think it is important for initial support.

No, not important, we can add that later if it ever becomes useful.

 ...

> > In general, you always ACK all interrupts first. Then you handle
> > the bits you have harvested.
> > 
> 
> The documentation says to ACK the interrupt after handling in the RX
> case:
> 
> <<<
> S/W needs to clear this status bit to allow next data receiving.
> > > > 
> 
> I will double check with Ryan to make sure TX works the same way.
> 
> > > +     if (irq_status & ASPEED_I2CD_INTR_ERROR ||
> > > +         (!bus->msgs && bus->master_state !=
> > > ASPEED_I2C_MASTER_STOP)) {
> 
> ...
> > 
> > I would set master_state to "RECOVERY" (new state ?) and ensure
> > those things are caught if they happen outside of a recovery.

I replied privately ... as long as we ack before we start a new command
we should be ok but we shouldn't ack after.

Your latest patch still does that. It will do things like start a STOP
command *then* ack the status bits. I'm pretty sure that's bogus.

That way it's a lot simpler to simply move the

	writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);

To either right after the readl of the status reg at the beginning of
aspeed_i2c_master_irq().

I would be very surprised if that didn't work properly and wasn't much
safer than what you are currently doing. 

> Let me know if you still think we need a "RECOVERY" state.

The way you just switch to stop state and store the error for later
should work I think.

> > 
> > > +     if (bus->master_state == ASPEED_I2C_MASTER_START) {
> 
> ...
> > 
> > > +                     dev_dbg(bus->dev,
> > > +                             "no slave present at %02x", msg-
> > > >addr);
> > > +                     status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> > > +                     bus->cmd_err = -EIO;
> > > +                     do_stop(bus);
> > > +                     goto out_no_complete;
> > > +             } else {
> > > +                     status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> > > +                     if (msg->flags & I2C_M_RD)
> > > +                             bus->master_state =
> > > ASPEED_I2C_MASTER_RX;
> > > +                     else
> > > +                             bus->master_state =
> > > ASPEED_I2C_MASTER_TX_FIRST;
> > 
> > What about the SMBUS_QUICK case ? (0-len transfer). Do we need
> > to handle this here ? A quick look at the TX_FIRST case makes
> > me think we are ok there but I'm not sure about the RX case.
> 
> I did not think that there is an SMBUS_QUICK RX. Could you point me
> to an example?

Not so much an RX, it's more like you are sending a 1-bit data in
the place of the Rd/Wr bit. So you have a read with a lenght of 0,
I don't think in that case you should set ASPEED_I2CD_M_RX_CMD in
__aspeed_i2c_do_start

> > I'm not sure the RX case is tight also. What completion does the
> > HW give you for the address cycle ? Won't you get that before it
> > has received the first character ? IE. You fall through to
> > the read case of the state machine with the read potentially
> > not complete yet no ?
> 
> ...
> > > +     case ASPEED_I2C_MASTER_RX:
> > > +             if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE)) {
> > > +                     dev_err(bus->dev, "master failed to RX");
> > > +                     goto out_complete;
> > > +             }
> > 
> > See my comment above for a bog standard i2c_read. Aren't you
> > getting
> > the completion for the address before the read is even started ?
> 
> In practice no, but it is probably best to be safe :-)

Yup :)
> > 
> > > +             status_ack |= ASPEED_I2CD_INTR_RX_DONE;
> > > +
> > > +             recv_byte = aspeed_i2c_read(bus,
> > > ASPEED_I2C_BYTE_BUF_REG) >> 8;
> > > +             msg->buf[bus->buf_index++] = recv_byte;
> > > +
> > > +             if (msg->flags & I2C_M_RECV_LEN &&
> > > +                 recv_byte <= I2C_SMBUS_BLOCK_MAX) {
> > > +                     msg->len = recv_byte +
> > > +                                     ((msg->flags &
> > > I2C_CLIENT_PEC) ? 2 : 1);
> 
> ...
> > > +     return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
> > > +             & ASPEED_I2CD_TIME_SCL_HIGH_MASK)
> > > +                     | ((clk_low <<
> > > ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
> > > +                        & ASPEED_I2CD_TIME_SCL_LOW_MASK)
> > > +                     | (base_clk &
> > > ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
> > > +}
> > 
> > As I think I mentioned earlier, the AST2500 has a slightly
> > different
> > register layout which support larger values for high and low, thus
> > allowing a finer granularity.
> 
> I am developing against the 2500.

Yes but we'd like the driver to work with both :-)

> > BTW. In case you haven't, I would suggest you copy/paste the above
> > in
> > a userspace app and run it for all frequency divisors and see if
> > your
> > results match the aspeed table :)
> 
> Good call.

If you end up doing that, can you shoot it my way ? I can take care
of making sure it's all good for the 2400.

> > > +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
> > > +                            struct platform_device *pdev)
> > > +{
> > > +     u32 clk_freq, divisor;
> > > +     struct clk *pclk;
> > > +     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);
> > 
> > See my previous comment about calling that 'bus-frequency' rather
> > than 'clock-frequency'.
> > 
> > > +     if (ret < 0) {
> > > +             dev_err(&pdev->dev,
> > > +                     "Could not read clock-frequency
> > > property\n");
> > > +             clk_freq = 100000;
> > > +     }
> > > +     divisor = 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 > 1000) {
> > > +             aspeed_i2c_write(bus, aspeed_i2c_read(bus,
> > > +                                                   ASPEED_I2C_FU
> > > N_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(divisor),
> > > +                              ASPEED_I2C_AC_TIMING_REG1);
> > 
> > I already discussed by doubts about the above. I can try to scope
> > it with the EVB if you don't get to it. For now I'd rather take the
> > code out.
> > 
> > We should ask aspeed from what frequency the "1T" stuff is useful.
> 
> Will do, I will try to rope Ryan in on the next review; it will be
> good for him to get used to working with upstream anyway.

Yup. However, for the sake of getting something upstream (and in
OpenBMC 4.10 kernel) asap, I would suggest just dropping support
for those fast speeds for now, we can add them back later.

> > 
> > > +     } else {
> > > +             aspeed_i2c_write(bus,
> > > aspeed_i2c_get_clk_reg_val(divisor),
> > > +                              ASPEED_I2C_AC_TIMING_REG1);
> > > +             aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,
> > > +                              ASPEED_I2C_AC_TIMING_REG2);
> > > +     }
> 
> ...
> > > +     spin_lock_init(&bus->lock);
> > > +     init_completion(&bus->cmd_complete);
> > > +     bus->adap.owner = THIS_MODULE;
> > > +     bus->adap.retries = 0;
> > > +     bus->adap.timeout = 5 * HZ;
> > > +     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");
> > 
> > Another trivial one, should we put some kind of bus number
> > in that string ?
> 
> Whoops, looks like I missed this one; I will get to it in the next
> revision.

Ok. I noticed you missed that in v7, so I assume you mean v8 :-)

> > 
> > > +     bus->dev = &pdev->dev;
> > > +
> > > +     /* reset device: disable master & slave functions */
> > > +     aspeed_i2c_write(bus, 0, ASPEED_I2C_FUN_CTRL_REG);
> 
> ...
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C

[Brendan Higgins]

Adding Ryan.

On Mon, Apr 24, 2017 at 7:19 PM, Benjamin Herrenschmidt
<benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
> On Mon, 2017-04-24 at 11:56 -0700, Brendan Higgins wrote:
>> > > +struct aspeed_i2c_bus {
>> > > +     struct i2c_adapter              adap;
>> > > +     struct device                   *dev;
>> > > +     void __iomem                    *base;
>> > > +     /* Synchronizes I/O mem access to base. */
>> > > +     spinlock_t                      lock;
>> >
>> > I am not entirely convinced we need that lock. The i2c core will
>> > take a mutex protecting all operations on the bus. So we only need
>> > to synchronize between our "xfer" code and our interrupt handler.
>>
>> You are right if both having slave and master active at the same time
>> was not possible; however, it is.
>
> Right, I somewhat forgot about the slave case.
>
>   ...
>
>> > Some of those error states probably also warrant a reset of the
>> > controller,
>> > I think aspeed does that in the SDK.
>>
>> For timeout and cmd_err, I do not see any argument against it; it
>> sounds like we are in a very messed up, very unknown state, so full
>> reset is probably the best last resort.
>
> Yup.
>
>> For SDA staying pulled down, I
>> think we can say with reasonable confidence that some device on our
>> bus is behaving very badly and I am not convinced that resetting the
>> controller is likely to do anything to help;
>
> Right. Hammering with STOPs and pray ...

I think sending recovery mode sends stops as a part of the recovery
algorithm it executes.

>
>>  that being said, I really
>> do not have any good ideas to address that. So maybe praying and
>> resetting the controller is *the most reasonable thing to do.* I
>> would like to know what you think we should do in that case.
>
> Well, there's a (small ?) chance that it's a controller bug asserting
> the line so ... but there's little we can do if not.

True.

>
>> While I was thinking about this I also realized that the SDA line
>> check after recovery happens in the else branch, but SCL line check
>> does not happen after we attempt to STOP if SCL is hung. If we decide
>> to make special note SDA being hung by a device that won't let go, we
>> might want to make a special note that SCL is hung by a device that
>> won't let go. Just a thought.
>
> Maybe. Or just "unrecoverable error"... hopefully these don't happen
> too often ... We had cases of a TPM misbehaving like that.

Yeah, definitely should print something out.

>
>> > > +out:
>>
>> ...
>> > What about I2C_M_NOSTART ?
>> >
>> > Not that I've ever seen it used... ;-)
>>
>> Right now I am not doing any of the protocol mangling options, but I
>> can add them in if you think it is important for initial support.
>
> No, not important, we can add that later if it ever becomes useful.
>
>  ...
>
>> > In general, you always ACK all interrupts first. Then you handle
>> > the bits you have harvested.
>> >
>>
>> The documentation says to ACK the interrupt after handling in the RX
>> case:
>>
>> <<<
>> S/W needs to clear this status bit to allow next data receiving.
>> > > >
>>
>> I will double check with Ryan to make sure TX works the same way.
>>
>> > > +     if (irq_status & ASPEED_I2CD_INTR_ERROR ||
>> > > +         (!bus->msgs && bus->master_state !=
>> > > ASPEED_I2C_MASTER_STOP)) {
>>
>> ...
>> >
>> > I would set master_state to "RECOVERY" (new state ?) and ensure
>> > those things are caught if they happen outside of a recovery.
>
> I replied privately ... as long as we ack before we start a new command
> we should be ok but we shouldn't ack after.
>
> Your latest patch still does that. It will do things like start a STOP
> command *then* ack the status bits. I'm pretty sure that's bogus.
>
> That way it's a lot simpler to simply move the
>
>         writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
>
> To either right after the readl of the status reg at the beginning of
> aspeed_i2c_master_irq().
>
> I would be very surprised if that didn't work properly and wasn't much
> safer than what you are currently doing.

I think I tried your way and it worked. In anycase, Ryan will be able
to clarify for us.

>
>> Let me know if you still think we need a "RECOVERY" state.
>
> The way you just switch to stop state and store the error for later
> should work I think.
>
>> >
>> > > +     if (bus->master_state == ASPEED_I2C_MASTER_START) {
>>
>> ...
>> >
>> > > +                     dev_dbg(bus->dev,
>> > > +                             "no slave present at %02x", msg-
>> > > >addr);
>> > > +                     status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> > > +                     bus->cmd_err = -EIO;
>> > > +                     do_stop(bus);
>> > > +                     goto out_no_complete;
>> > > +             } else {
>> > > +                     status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> > > +                     if (msg->flags & I2C_M_RD)
>> > > +                             bus->master_state =
>> > > ASPEED_I2C_MASTER_RX;
>> > > +                     else
>> > > +                             bus->master_state =
>> > > ASPEED_I2C_MASTER_TX_FIRST;
>> >
>> > What about the SMBUS_QUICK case ? (0-len transfer). Do we need
>> > to handle this here ? A quick look at the TX_FIRST case makes
>> > me think we are ok there but I'm not sure about the RX case.
>>
>> I did not think that there is an SMBUS_QUICK RX. Could you point me
>> to an example?
>
> Not so much an RX, it's more like you are sending a 1-bit data in
> the place of the Rd/Wr bit. So you have a read with a lenght of 0,
> I don't think in that case you should set ASPEED_I2CD_M_RX_CMD in
> __aspeed_i2c_do_start

Forget what I said, I was just not thinking about the fact that SMBus
emulation causes the data bit to be encoded as the R/W flag. I see
what you are saying; you are correct.

>
>> > I'm not sure the RX case is tight also. What completion does the
>> > HW give you for the address cycle ? Won't you get that before it
>> > has received the first character ? IE. You fall through to
>> > the read case of the state machine with the read potentially
>> > not complete yet no ?
>>
>> ...
>> > > +     case ASPEED_I2C_MASTER_RX:
>> > > +             if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE)) {
>> > > +                     dev_err(bus->dev, "master failed to RX");
>> > > +                     goto out_complete;
>> > > +             }
>> >
>> > See my comment above for a bog standard i2c_read. Aren't you
>> > getting
>> > the completion for the address before the read is even started ?
>>
>> In practice no, but it is probably best to be safe :-)
>
> Yup :)
>> >
>> > > +             status_ack |= ASPEED_I2CD_INTR_RX_DONE;
>> > > +
>> > > +             recv_byte = aspeed_i2c_read(bus,
>> > > ASPEED_I2C_BYTE_BUF_REG) >> 8;
>> > > +             msg->buf[bus->buf_index++] = recv_byte;
>> > > +
>> > > +             if (msg->flags & I2C_M_RECV_LEN &&
>> > > +                 recv_byte <= I2C_SMBUS_BLOCK_MAX) {
>> > > +                     msg->len = recv_byte +
>> > > +                                     ((msg->flags &
>> > > I2C_CLIENT_PEC) ? 2 : 1);
>>
>> ...
>> > > +     return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
>> > > +             & ASPEED_I2CD_TIME_SCL_HIGH_MASK)
>> > > +                     | ((clk_low <<
>> > > ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
>> > > +                        & ASPEED_I2CD_TIME_SCL_LOW_MASK)
>> > > +                     | (base_clk &
>> > > ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
>> > > +}
>> >
>> > As I think I mentioned earlier, the AST2500 has a slightly
>> > different
>> > register layout which support larger values for high and low, thus
>> > allowing a finer granularity.
>>
>> I am developing against the 2500.
>
> Yes but we'd like the driver to work with both :-)

Right, I thought you were making an assertion about the 2500, if you
are making an assertion about the 2400, I do not know and do not have
one handy.

>
>> > BTW. In case you haven't, I would suggest you copy/paste the above
>> > in
>> > a userspace app and run it for all frequency divisors and see if
>> > your
>> > results match the aspeed table :)
>>
>> Good call.
>
> If you end up doing that, can you shoot it my way ? I can take care
> of making sure it's all good for the 2400.

Will do.

>
>> > > +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
>> > > +                            struct platform_device *pdev)
>> > > +{
>> > > +     u32 clk_freq, divisor;
>> > > +     struct clk *pclk;
>> > > +     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);
>> >
>> > See my previous comment about calling that 'bus-frequency' rather
>> > than 'clock-frequency'.
>> >
>> > > +     if (ret < 0) {
>> > > +             dev_err(&pdev->dev,
>> > > +                     "Could not read clock-frequency
>> > > property\n");
>> > > +             clk_freq = 100000;
>> > > +     }
>> > > +     divisor = 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 > 1000) {
>> > > +             aspeed_i2c_write(bus, aspeed_i2c_read(bus,
>> > > +                                                   ASPEED_I2C_FU
>> > > N_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(divisor),
>> > > +                              ASPEED_I2C_AC_TIMING_REG1);
>> >
>> > I already discussed by doubts about the above. I can try to scope
>> > it with the EVB if you don't get to it. For now I'd rather take the
>> > code out.
>> >
>> > We should ask aspeed from what frequency the "1T" stuff is useful.
>>
>> Will do, I will try to rope Ryan in on the next review; it will be
>> good for him to get used to working with upstream anyway.
>
> Yup. However, for the sake of getting something upstream (and in
> OpenBMC 4.10 kernel) asap, I would suggest just dropping support
> for those fast speeds for now, we can add them back later.

Alright, that's fine. Still, Ryan, could you provide some context on this?

>
>> >
>> > > +     } else {
>> > > +             aspeed_i2c_write(bus,
>> > > aspeed_i2c_get_clk_reg_val(divisor),
>> > > +                              ASPEED_I2C_AC_TIMING_REG1);
>> > > +             aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,
>> > > +                              ASPEED_I2C_AC_TIMING_REG2);
>> > > +     }
>>
>> ...
>> > > +     spin_lock_init(&bus->lock);
>> > > +     init_completion(&bus->cmd_complete);
>> > > +     bus->adap.owner = THIS_MODULE;
>> > > +     bus->adap.retries = 0;
>> > > +     bus->adap.timeout = 5 * HZ;
>> > > +     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");
>> >
>> > Another trivial one, should we put some kind of bus number
>> > in that string ?
>>
>> Whoops, looks like I missed this one; I will get to it in the next
>> revision.
>
> Ok. I noticed you missed that in v7, so I assume you mean v8 :-)

Yep, I will get it in v8.

>
>> >
>> > > +     bus->dev = &pdev->dev;
>> > > +
>> > > +     /* reset device: disable master & slave functions */
>> > > +     aspeed_i2c_write(bus, 0, ASPEED_I2C_FUN_CTRL_REG);
>>
>> ...
>> --
>> 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
--
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

RE: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C

[Ryan Chen]

Hello All,
		ASPEED_I2CD_M_SDA_DRIVE_1T_EN, ASPEED_I2CD_SDA_DRIVE_1T_EN is specific for some case usage. 
		For example, if i2c bus is use on "high speed" and "single slave and master" and i2c bus is too long. It need drive SDA or SCL less lunacy. It would enable it. 
		Otherwise, don’t enable it. especially in multi-master. It can’t be enable. 

		  
	

Best Regards,
Ryan

信驊科技股份有限公司
ASPEED Technology Inc.
2F,No.15,Industry East Road 4.,Hsinchu Science Park, Hsinchu City 30077, Taiwan
Tel: 886-3-578-9568  #857
Fax: 886-3-578-9586
************* Email Confidentiality Notice ********************
DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.


-----Original Message-----
From: Brendan Higgins [mailto:brendanhiggins@google.com] 
Sent: Tuesday, April 25, 2017 4:32 PM
To: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Wolfram Sang <wsa@the-dreams.de>; Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Thomas Gleixner <tglx@linutronix.de>; Jason Cooper <jason@lakedaemon.net>; Marc Zyngier <marc.zyngier@arm.com>; Joel Stanley <joel@jms.id.au>; Vladimir Zapolskiy <vz@mleia.com>; Kachalov Anton <mouse@mayc.ru>; Cédric Le Goater <clg@kaod.org>; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; OpenBMC Maillist <openbmc@lists.ozlabs.org>; Ryan Chen <ryan_chen@aspeedtech.com>
Subject: Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C

Adding Ryan.

On Mon, Apr 24, 2017 at 7:19 PM, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> On Mon, 2017-04-24 at 11:56 -0700, Brendan Higgins wrote:
>> > > +struct aspeed_i2c_bus {
>> > > +     struct i2c_adapter              adap;
>> > > +     struct device                   *dev;
>> > > +     void __iomem                    *base;
>> > > +     /* Synchronizes I/O mem access to base. */
>> > > +     spinlock_t                      lock;
>> >
>> > I am not entirely convinced we need that lock. The i2c core will 
>> > take a mutex protecting all operations on the bus. So we only need 
>> > to synchronize between our "xfer" code and our interrupt handler.
>>
>> You are right if both having slave and master active at the same time 
>> was not possible; however, it is.
>
> Right, I somewhat forgot about the slave case.
>
>   ...
>
>> > Some of those error states probably also warrant a reset of the 
>> > controller, I think aspeed does that in the SDK.
>>
>> For timeout and cmd_err, I do not see any argument against it; it 
>> sounds like we are in a very messed up, very unknown state, so full 
>> reset is probably the best last resort.
>
> Yup.
>
>> For SDA staying pulled down, I
>> think we can say with reasonable confidence that some device on our 
>> bus is behaving very badly and I am not convinced that resetting the 
>> controller is likely to do anything to help;
>
> Right. Hammering with STOPs and pray ...

I think sending recovery mode sends stops as a part of the recovery algorithm it executes.

>
>>  that being said, I really
>> do not have any good ideas to address that. So maybe praying and 
>> resetting the controller is *the most reasonable thing to do.* I 
>> would like to know what you think we should do in that case.
>
> Well, there's a (small ?) chance that it's a controller bug asserting 
> the line so ... but there's little we can do if not.

True.

>
>> While I was thinking about this I also realized that the SDA line 
>> check after recovery happens in the else branch, but SCL line check 
>> does not happen after we attempt to STOP if SCL is hung. If we decide 
>> to make special note SDA being hung by a device that won't let go, we 
>> might want to make a special note that SCL is hung by a device that 
>> won't let go. Just a thought.
>
> Maybe. Or just "unrecoverable error"... hopefully these don't happen 
> too often ... We had cases of a TPM misbehaving like that.

Yeah, definitely should print something out.

>
>> > > +out:
>>
>> ...
>> > What about I2C_M_NOSTART ?
>> >
>> > Not that I've ever seen it used... ;-)
>>
>> Right now I am not doing any of the protocol mangling options, but I 
>> can add them in if you think it is important for initial support.
>
> No, not important, we can add that later if it ever becomes useful.
>
>  ...
>
>> > In general, you always ACK all interrupts first. Then you handle 
>> > the bits you have harvested.
>> >
>>
>> The documentation says to ACK the interrupt after handling in the RX
>> case:
>>
>> <<<
>> S/W needs to clear this status bit to allow next data receiving.
>> > > >
>>
>> I will double check with Ryan to make sure TX works the same way.
>>
>> > > +     if (irq_status & ASPEED_I2CD_INTR_ERROR ||
>> > > +         (!bus->msgs && bus->master_state !=
>> > > ASPEED_I2C_MASTER_STOP)) {
>>
>> ...
>> >
>> > I would set master_state to "RECOVERY" (new state ?) and ensure 
>> > those things are caught if they happen outside of a recovery.
>
> I replied privately ... as long as we ack before we start a new 
> command we should be ok but we shouldn't ack after.
>
> Your latest patch still does that. It will do things like start a STOP 
> command *then* ack the status bits. I'm pretty sure that's bogus.
>
> That way it's a lot simpler to simply move the
>
>         writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
>
> To either right after the readl of the status reg at the beginning of 
> aspeed_i2c_master_irq().
>
> I would be very surprised if that didn't work properly and wasn't much 
> safer than what you are currently doing.

I think I tried your way and it worked. In anycase, Ryan will be able to clarify for us.

>
>> Let me know if you still think we need a "RECOVERY" state.
>
> The way you just switch to stop state and store the error for later 
> should work I think.
>
>> >
>> > > +     if (bus->master_state == ASPEED_I2C_MASTER_START) {
>>
>> ...
>> >
>> > > +                     dev_dbg(bus->dev,
>> > > +                             "no slave present at %02x", msg-
>> > > >addr);
>> > > +                     status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> > > +                     bus->cmd_err = -EIO;
>> > > +                     do_stop(bus);
>> > > +                     goto out_no_complete;
>> > > +             } else {
>> > > +                     status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> > > +                     if (msg->flags & I2C_M_RD)
>> > > +                             bus->master_state =
>> > > ASPEED_I2C_MASTER_RX;
>> > > +                     else
>> > > +                             bus->master_state =
>> > > ASPEED_I2C_MASTER_TX_FIRST;
>> >
>> > What about the SMBUS_QUICK case ? (0-len transfer). Do we need to 
>> > handle this here ? A quick look at the TX_FIRST case makes me think 
>> > we are ok there but I'm not sure about the RX case.
>>
>> I did not think that there is an SMBUS_QUICK RX. Could you point me 
>> to an example?
>
> Not so much an RX, it's more like you are sending a 1-bit data in the 
> place of the Rd/Wr bit. So you have a read with a lenght of 0, I don't 
> think in that case you should set ASPEED_I2CD_M_RX_CMD in 
> __aspeed_i2c_do_start

Forget what I said, I was just not thinking about the fact that SMBus emulation causes the data bit to be encoded as the R/W flag. I see what you are saying; you are correct.

>
>> > I'm not sure the RX case is tight also. What completion does the HW 
>> > give you for the address cycle ? Won't you get that before it has 
>> > received the first character ? IE. You fall through to the read 
>> > case of the state machine with the read potentially not complete 
>> > yet no ?
>>
>> ...
>> > > +     case ASPEED_I2C_MASTER_RX:
>> > > +             if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE)) {
>> > > +                     dev_err(bus->dev, "master failed to RX");
>> > > +                     goto out_complete;
>> > > +             }
>> >
>> > See my comment above for a bog standard i2c_read. Aren't you 
>> > getting the completion for the address before the read is even 
>> > started ?
>>
>> In practice no, but it is probably best to be safe :-)
>
> Yup :)
>> >
>> > > +             status_ack |= ASPEED_I2CD_INTR_RX_DONE;
>> > > +
>> > > +             recv_byte = aspeed_i2c_read(bus,
>> > > ASPEED_I2C_BYTE_BUF_REG) >> 8;
>> > > +             msg->buf[bus->buf_index++] = recv_byte;
>> > > +
>> > > +             if (msg->flags & I2C_M_RECV_LEN &&
>> > > +                 recv_byte <= I2C_SMBUS_BLOCK_MAX) {
>> > > +                     msg->len = recv_byte +
>> > > +                                     ((msg->flags &
>> > > I2C_CLIENT_PEC) ? 2 : 1);
>>
>> ...
>> > > +     return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
>> > > +             & ASPEED_I2CD_TIME_SCL_HIGH_MASK)
>> > > +                     | ((clk_low <<
>> > > ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
>> > > +                        & ASPEED_I2CD_TIME_SCL_LOW_MASK)
>> > > +                     | (base_clk &
>> > > ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
>> > > +}
>> >
>> > As I think I mentioned earlier, the AST2500 has a slightly 
>> > different register layout which support larger values for high and 
>> > low, thus allowing a finer granularity.
>>
>> I am developing against the 2500.
>
> Yes but we'd like the driver to work with both :-)

Right, I thought you were making an assertion about the 2500, if you are making an assertion about the 2400, I do not know and do not have one handy.

>
>> > BTW. In case you haven't, I would suggest you copy/paste the above 
>> > in a userspace app and run it for all frequency divisors and see if 
>> > your results match the aspeed table :)
>>
>> Good call.
>
> If you end up doing that, can you shoot it my way ? I can take care of 
> making sure it's all good for the 2400.

Will do.

>
>> > > +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
>> > > +                            struct platform_device *pdev) {
>> > > +     u32 clk_freq, divisor;
>> > > +     struct clk *pclk;
>> > > +     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);
>> >
>> > See my previous comment about calling that 'bus-frequency' rather 
>> > than 'clock-frequency'.
>> >
>> > > +     if (ret < 0) {
>> > > +             dev_err(&pdev->dev,
>> > > +                     "Could not read clock-frequency
>> > > property\n");
>> > > +             clk_freq = 100000;
>> > > +     }
>> > > +     divisor = 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 > 1000) {
>> > > +             aspeed_i2c_write(bus, aspeed_i2c_read(bus,
>> > > +                                                   ASPEED_I2C_FU
>> > > N_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(divisor),
>> > > +                              ASPEED_I2C_AC_TIMING_REG1);
>> >
>> > I already discussed by doubts about the above. I can try to scope 
>> > it with the EVB if you don't get to it. For now I'd rather take the 
>> > code out.
>> >
>> > We should ask aspeed from what frequency the "1T" stuff is useful.
>>
>> Will do, I will try to rope Ryan in on the next review; it will be 
>> good for him to get used to working with upstream anyway.
>
> Yup. However, for the sake of getting something upstream (and in 
> OpenBMC 4.10 kernel) asap, I would suggest just dropping support for 
> those fast speeds for now, we can add them back later.

Alright, that's fine. Still, Ryan, could you provide some context on this?

>
>> >
>> > > +     } else {
>> > > +             aspeed_i2c_write(bus,
>> > > aspeed_i2c_get_clk_reg_val(divisor),
>> > > +                              ASPEED_I2C_AC_TIMING_REG1);
>> > > +             aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,
>> > > +                              ASPEED_I2C_AC_TIMING_REG2);
>> > > +     }
>>
>> ...
>> > > +     spin_lock_init(&bus->lock);
>> > > +     init_completion(&bus->cmd_complete);
>> > > +     bus->adap.owner = THIS_MODULE;
>> > > +     bus->adap.retries = 0;
>> > > +     bus->adap.timeout = 5 * HZ;
>> > > +     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");
>> >
>> > Another trivial one, should we put some kind of bus number in that 
>> > string ?
>>
>> Whoops, looks like I missed this one; I will get to it in the next 
>> revision.
>
> Ok. I noticed you missed that in v7, so I assume you mean v8 :-)

Yep, I will get it in v8.

>
>> >
>> > > +     bus->dev = &pdev->dev;
>> > > +
>> > > +     /* reset device: disable master & slave functions */
>> > > +     aspeed_i2c_write(bus, 0, ASPEED_I2C_FUN_CTRL_REG);
>>
>> ...
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree"
>> in
>> the body of a message to majordomo@vger.kernel.org More majordomo 
>> info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C

[Benjamin Herrenschmidt]

On Tue, 2017-04-25 at 08:50 +0000, Ryan Chen wrote:
> Hello All,
> 		ASPEED_I2CD_M_SDA_DRIVE_1T_EN,
> ASPEED_I2CD_SDA_DRIVE_1T_EN is specific for some case usage. 
> 		For example, if i2c bus is use on "high speed" and
> "single slave and master" and i2c bus is too long. It need drive SDA
> or SCL less lunacy. It would enable it. 
> 		Otherwise, don’t enable it. especially in multi-master. 
> It can’t be enable. 

That smells like a specific enough use case that we should probably
cover with a device-tree property, something like an empty
"sda-extra-drive" property (empty properties are typically used
for booleans, their presence means "true").

Thanks Ryan. Can you shed some light on the meaning of the high-speed
bit as well please ? Does it force to a specific speed (ignoring the
divisor) or we can still play with the clock high/low counts ?

Cheers,
Ben.

> 		  
> 	
> 
> Best Regards,
> Ryan
> 
> 信驊科技股份有限公司
> ASPEED Technology Inc.
> 2F,No.15,Industry East Road 4.,Hsinchu Science Park, Hsinchu City
> 30077, Taiwan
> Tel: 886-3-578-9568  #857
> Fax: 886-3-578-9586
> ************* Email Confidentiality Notice ********************
> DISCLAIMER:
> This message (and any attachments) may contain legally privileged
> and/or other confidential information. If you have received it in
> error, please notify the sender by reply e-mail and immediately
> delete the e-mail and any attachments without copying or disclosing
> the contents. Thank you.
> 
> 
> -----Original Message-----
> From: Brendan Higgins [mailto:brendanhiggins@google.com] 
> Sent: Tuesday, April 25, 2017 4:32 PM
> To: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Wolfram Sang <wsa@the-dreams.de>; Rob Herring <robh+dt@kernel.org
> >; Mark Rutland <mark.rutland@arm.com>; Thomas Gleixner <tglx@linutro
> nix.de>; Jason Cooper <jason@lakedaemon.net>; Marc Zyngier <marc.zyng
> ier@arm.com>; Joel Stanley <joel@jms.id.au>; Vladimir Zapolskiy <vz@m
> leia.com>; Kachalov Anton <mouse@mayc.ru>; Cédric Le Goater <clg@kaod
> .org>; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; Linux
> Kernel Mailing List <linux-kernel@vger.kernel.org>; OpenBMC Maillist
> <openbmc@lists.ozlabs.org>; Ryan Chen <ryan_chen@aspeedtech.com>
> Subject: Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C
> 
> Adding Ryan.
> 
> On Mon, Apr 24, 2017 at 7:19 PM, Benjamin Herrenschmidt <benh@kernel.
> crashing.org> wrote:
> > On Mon, 2017-04-24 at 11:56 -0700, Brendan Higgins wrote:
> > > > > +struct aspeed_i2c_bus {
> > > > > +     struct i2c_adapter              adap;
> > > > > +     struct device                   *dev;
> > > > > +     void __iomem                    *base;
> > > > > +     /* Synchronizes I/O mem access to base. */
> > > > > +     spinlock_t                      lock;
> > > > 
> > > > I am not entirely convinced we need that lock. The i2c core
> > > > will 
> > > > take a mutex protecting all operations on the bus. So we only
> > > > need 
> > > > to synchronize between our "xfer" code and our interrupt
> > > > handler.
> > > 
> > > You are right if both having slave and master active at the same
> > > time 
> > > was not possible; however, it is.
> > 
> > Right, I somewhat forgot about the slave case.
> > 
> >   ...
> > 
> > > > Some of those error states probably also warrant a reset of
> > > > the 
> > > > controller, I think aspeed does that in the SDK.
> > > 
> > > For timeout and cmd_err, I do not see any argument against it;
> > > it 
> > > sounds like we are in a very messed up, very unknown state, so
> > > full 
> > > reset is probably the best last resort.
> > 
> > Yup.
> > 
> > > For SDA staying pulled down, I
> > > think we can say with reasonable confidence that some device on
> > > our 
> > > bus is behaving very badly and I am not convinced that resetting
> > > the 
> > > controller is likely to do anything to help;
> > 
> > Right. Hammering with STOPs and pray ...
> 
> I think sending recovery mode sends stops as a part of the recovery
> algorithm it executes.
> 
> > 
> > >  that being said, I really
> > > do not have any good ideas to address that. So maybe praying and 
> > > resetting the controller is *the most reasonable thing to do.* I 
> > > would like to know what you think we should do in that case.
> > 
> > Well, there's a (small ?) chance that it's a controller bug
> > asserting 
> > the line so ... but there's little we can do if not.
> 
> True.
> 
> > 
> > > While I was thinking about this I also realized that the SDA
> > > line 
> > > check after recovery happens in the else branch, but SCL line
> > > check 
> > > does not happen after we attempt to STOP if SCL is hung. If we
> > > decide 
> > > to make special note SDA being hung by a device that won't let
> > > go, we 
> > > might want to make a special note that SCL is hung by a device
> > > that 
> > > won't let go. Just a thought.
> > 
> > Maybe. Or just "unrecoverable error"... hopefully these don't
> > happen 
> > too often ... We had cases of a TPM misbehaving like that.
> 
> Yeah, definitely should print something out.
> 
> > 
> > > > > +out:
> > > 
> > > ...
> > > > What about I2C_M_NOSTART ?
> > > > 
> > > > Not that I've ever seen it used... ;-)
> > > 
> > > Right now I am not doing any of the protocol mangling options,
> > > but I 
> > > can add them in if you think it is important for initial support.
> > 
> > No, not important, we can add that later if it ever becomes useful.
> > 
> >  ...
> > 
> > > > In general, you always ACK all interrupts first. Then you
> > > > handle 
> > > > the bits you have harvested.
> > > > 
> > > 
> > > The documentation says to ACK the interrupt after handling in the
> > > RX
> > > case:
> > > 
> > > <<<
> > > S/W needs to clear this status bit to allow next data receiving.
> > > > > > 
> > > 
> > > I will double check with Ryan to make sure TX works the same way.
> > > 
> > > > > +     if (irq_status & ASPEED_I2CD_INTR_ERROR ||
> > > > > +         (!bus->msgs && bus->master_state !=
> > > > > ASPEED_I2C_MASTER_STOP)) {
> > > 
> > > ...
> > > > 
> > > > I would set master_state to "RECOVERY" (new state ?) and
> > > > ensure 
> > > > those things are caught if they happen outside of a recovery.
> > 
> > I replied privately ... as long as we ack before we start a new 
> > command we should be ok but we shouldn't ack after.
> > 
> > Your latest patch still does that. It will do things like start a
> > STOP 
> > command *then* ack the status bits. I'm pretty sure that's bogus.
> > 
> > That way it's a lot simpler to simply move the
> > 
> >         writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
> > 
> > To either right after the readl of the status reg at the beginning
> > of 
> > aspeed_i2c_master_irq().
> > 
> > I would be very surprised if that didn't work properly and wasn't
> > much 
> > safer than what you are currently doing.
> 
> I think I tried your way and it worked. In anycase, Ryan will be able
> to clarify for us.
> 
> > 
> > > Let me know if you still think we need a "RECOVERY" state.
> > 
> > The way you just switch to stop state and store the error for
> > later 
> > should work I think.
> > 
> > > > 
> > > > > +     if (bus->master_state == ASPEED_I2C_MASTER_START) {
> > > 
> > > ...
> > > > 
> > > > > +                     dev_dbg(bus->dev,
> > > > > +                             "no slave present at %02x",
> > > > > msg-
> > > > > > addr);
> > > > > 
> > > > > +                     status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> > > > > +                     bus->cmd_err = -EIO;
> > > > > +                     do_stop(bus);
> > > > > +                     goto out_no_complete;
> > > > > +             } else {
> > > > > +                     status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> > > > > +                     if (msg->flags & I2C_M_RD)
> > > > > +                             bus->master_state =
> > > > > ASPEED_I2C_MASTER_RX;
> > > > > +                     else
> > > > > +                             bus->master_state =
> > > > > ASPEED_I2C_MASTER_TX_FIRST;
> > > > 
> > > > What about the SMBUS_QUICK case ? (0-len transfer). Do we need
> > > > to 
> > > > handle this here ? A quick look at the TX_FIRST case makes me
> > > > think 
> > > > we are ok there but I'm not sure about the RX case.
> > > 
> > > I did not think that there is an SMBUS_QUICK RX. Could you point
> > > me 
> > > to an example?
> > 
> > Not so much an RX, it's more like you are sending a 1-bit data in
> > the 
> > place of the Rd/Wr bit. So you have a read with a lenght of 0, I
> > don't 
> > think in that case you should set ASPEED_I2CD_M_RX_CMD in 
> > __aspeed_i2c_do_start
> 
> Forget what I said, I was just not thinking about the fact that SMBus
> emulation causes the data bit to be encoded as the R/W flag. I see
> what you are saying; you are correct.
> 
> > 
> > > > I'm not sure the RX case is tight also. What completion does
> > > > the HW 
> > > > give you for the address cycle ? Won't you get that before it
> > > > has 
> > > > received the first character ? IE. You fall through to the
> > > > read 
> > > > case of the state machine with the read potentially not
> > > > complete 
> > > > yet no ?
> > > 
> > > ...
> > > > > +     case ASPEED_I2C_MASTER_RX:
> > > > > +             if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE)) {
> > > > > +                     dev_err(bus->dev, "master failed to
> > > > > RX");
> > > > > +                     goto out_complete;
> > > > > +             }
> > > > 
> > > > See my comment above for a bog standard i2c_read. Aren't you 
> > > > getting the completion for the address before the read is even 
> > > > started ?
> > > 
> > > In practice no, but it is probably best to be safe :-)
> > 
> > Yup :)
> > > > 
> > > > > +             status_ack |= ASPEED_I2CD_INTR_RX_DONE;
> > > > > +
> > > > > +             recv_byte = aspeed_i2c_read(bus,
> > > > > ASPEED_I2C_BYTE_BUF_REG) >> 8;
> > > > > +             msg->buf[bus->buf_index++] = recv_byte;
> > > > > +
> > > > > +             if (msg->flags & I2C_M_RECV_LEN &&
> > > > > +                 recv_byte <= I2C_SMBUS_BLOCK_MAX) {
> > > > > +                     msg->len = recv_byte +
> > > > > +                                     ((msg->flags &
> > > > > I2C_CLIENT_PEC) ? 2 : 1);
> > > 
> > > ...
> > > > > +     return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
> > > > > +             & ASPEED_I2CD_TIME_SCL_HIGH_MASK)
> > > > > +                     | ((clk_low <<
> > > > > ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
> > > > > +                        & ASPEED_I2CD_TIME_SCL_LOW_MASK)
> > > > > +                     | (base_clk &
> > > > > ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
> > > > > +}
> > > > 
> > > > As I think I mentioned earlier, the AST2500 has a slightly 
> > > > different register layout which support larger values for high
> > > > and 
> > > > low, thus allowing a finer granularity.
> > > 
> > > I am developing against the 2500.
> > 
> > Yes but we'd like the driver to work with both :-)
> 
> Right, I thought you were making an assertion about the 2500, if you
> are making an assertion about the 2400, I do not know and do not have
> one handy.
> 
> > 
> > > > BTW. In case you haven't, I would suggest you copy/paste the
> > > > above 
> > > > in a userspace app and run it for all frequency divisors and
> > > > see if 
> > > > your results match the aspeed table :)
> > > 
> > > Good call.
> > 
> > If you end up doing that, can you shoot it my way ? I can take care
> > of 
> > making sure it's all good for the 2400.
> 
> Will do.
> 
> > 
> > > > > +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
> > > > > +                            struct platform_device *pdev) {
> > > > > +     u32 clk_freq, divisor;
> > > > > +     struct clk *pclk;
> > > > > +     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);
> > > > 
> > > > See my previous comment about calling that 'bus-frequency'
> > > > rather 
> > > > than 'clock-frequency'.
> > > > 
> > > > > +     if (ret < 0) {
> > > > > +             dev_err(&pdev->dev,
> > > > > +                     "Could not read clock-frequency
> > > > > property\n");
> > > > > +             clk_freq = 100000;
> > > > > +     }
> > > > > +     divisor = 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 > 1000) {
> > > > > +             aspeed_i2c_write(bus, aspeed_i2c_read(bus,
> > > > > +                                                   ASPEED_I2
> > > > > C_FU
> > > > > N_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(divisor),
> > > > > +                              ASPEED_I2C_AC_TIMING_REG1);
> > > > 
> > > > I already discussed by doubts about the above. I can try to
> > > > scope 
> > > > it with the EVB if you don't get to it. For now I'd rather take
> > > > the 
> > > > code out.
> > > > 
> > > > We should ask aspeed from what frequency the "1T" stuff is
> > > > useful.
> > > 
> > > Will do, I will try to rope Ryan in on the next review; it will
> > > be 
> > > good for him to get used to working with upstream anyway.
> > 
> > Yup. However, for the sake of getting something upstream (and in 
> > OpenBMC 4.10 kernel) asap, I would suggest just dropping support
> > for 
> > those fast speeds for now, we can add them back later.
> 
> Alright, that's fine. Still, Ryan, could you provide some context on
> this?
> 
> > 
> > > > 
> > > > > +     } else {
> > > > > +             aspeed_i2c_write(bus,
> > > > > aspeed_i2c_get_clk_reg_val(divisor),
> > > > > +                              ASPEED_I2C_AC_TIMING_REG1);
> > > > > +             aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,
> > > > > +                              ASPEED_I2C_AC_TIMING_REG2);
> > > > > +     }
> > > 
> > > ...
> > > > > +     spin_lock_init(&bus->lock);
> > > > > +     init_completion(&bus->cmd_complete);
> > > > > +     bus->adap.owner = THIS_MODULE;
> > > > > +     bus->adap.retries = 0;
> > > > > +     bus->adap.timeout = 5 * HZ;
> > > > > +     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");
> > > > 
> > > > Another trivial one, should we put some kind of bus number in
> > > > that 
> > > > string ?
> > > 
> > > Whoops, looks like I missed this one; I will get to it in the
> > > next 
> > > revision.
> > 
> > Ok. I noticed you missed that in v7, so I assume you mean v8 :-)
> 
> Yep, I will get it in v8.
> 
> > 
> > > > 
> > > > > +     bus->dev = &pdev->dev;
> > > > > +
> > > > > +     /* reset device: disable master & slave functions */
> > > > > +     aspeed_i2c_write(bus, 0, ASPEED_I2C_FUN_CTRL_REG);
> > > 
> > > ...
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe
> > > devicetree"
> > > in
> > > the body of a message to majordomo@vger.kernel.org More
> > > majordomo 
> > > info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C

[Ryan Chen]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 18710 bytes --]

Thanks Ryan. Can you shed some light on the meaning of the high-speed bit as well please ?

About ASPEED_I2CD_M_HIGH_SPEED_EN, it is support for I2C specification "High speed transfer". And also device need support it. 
If you just speed up the I2C bus clock, you don’t have to enable ASPEED_I2CD_M_HIGH_SPEED_EN, just change the clock is ok.


-----Original Message-----
From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org] 
Sent: Tuesday, April 25, 2017 5:35 PM
To: Ryan Chen <ryan_chen@aspeedtech.com>; Brendan Higgins <brendanhiggins@google.com>
Cc: Wolfram Sang <wsa@the-dreams.de>; Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Thomas Gleixner <tglx@linutronix.de>; Jason Cooper <jason@lakedaemon.net>; Marc Zyngier <marc.zyngier@arm.com>; Joel Stanley <joel@jms.id.au>; Vladimir Zapolskiy <vz@mleia.com>; Kachalov Anton <mouse@mayc.ru>; Cédric Le Goater <clg@kaod.org>; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; OpenBMC Maillist <openbmc@lists.ozlabs.org>
Subject: Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C

On Tue, 2017-04-25 at 08:50 +0000, Ryan Chen wrote:
> Hello All,
> 		ASPEED_I2CD_M_SDA_DRIVE_1T_EN,
> ASPEED_I2CD_SDA_DRIVE_1T_EN is specific for some case usage. 
> 		For example, if i2c bus is use on "high speed" and "single slave and 
> master" and i2c bus is too long. It need drive SDA or SCL less lunacy. 
> It would enable it.
> 		Otherwise, don’t enable it. especially in multi-master. 
> It can’t be enable.

That smells like a specific enough use case that we should probably cover with a device-tree property, something like an empty "sda-extra-drive" property (empty properties are typically used for booleans, their presence means "true").

Thanks Ryan. Can you shed some light on the meaning of the high-speed bit as well please ? Does it force to a specific speed (ignoring the
divisor) or we can still play with the clock high/low counts ?

Cheers,
Ben.

> 		  
> 	
> 
> Best Regards,
> Ryan
> 
> 信驊科技股份有限公司
> ASPEED Technology Inc.
> 2F,No.15,Industry East Road 4.,Hsinchu Science Park, Hsinchu City 
> 30077, Taiwan
> Tel: 886-3-578-9568  #857
> Fax: 886-3-578-9586
> ************* Email Confidentiality Notice ********************
> DISCLAIMER:
> This message (and any attachments) may contain legally privileged 
> and/or other confidential information. If you have received it in 
> error, please notify the sender by reply e-mail and immediately delete 
> the e-mail and any attachments without copying or disclosing the 
> contents. Thank you.
> 
> 
> -----Original Message-----
> From: Brendan Higgins [mailto:brendanhiggins@google.com]
> Sent: Tuesday, April 25, 2017 4:32 PM
> To: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Wolfram Sang <wsa@the-dreams.de>; Rob Herring <robh+dt@kernel.org
> >; Mark Rutland <mark.rutland@arm.com>; Thomas Gleixner <tglx@linutro
> nix.de>; Jason Cooper <jason@lakedaemon.net>; Marc Zyngier <marc.zyng 
> ier@arm.com>; Joel Stanley <joel@jms.id.au>; Vladimir Zapolskiy <vz@m 
> leia.com>; Kachalov Anton <mouse@mayc.ru>; Cédric Le Goater <clg@kaod 
> .org>; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; Linux 
> Kernel Mailing List <linux-kernel@vger.kernel.org>; OpenBMC Maillist 
> <openbmc@lists.ozlabs.org>; Ryan Chen <ryan_chen@aspeedtech.com>
> Subject: Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C
> 
> Adding Ryan.
> 
> On Mon, Apr 24, 2017 at 7:19 PM, Benjamin Herrenschmidt <benh@kernel.
> crashing.org> wrote:
> > On Mon, 2017-04-24 at 11:56 -0700, Brendan Higgins wrote:
> > > > > +struct aspeed_i2c_bus {
> > > > > +     struct i2c_adapter              adap;
> > > > > +     struct device                   *dev;
> > > > > +     void __iomem                    *base;
> > > > > +     /* Synchronizes I/O mem access to base. */
> > > > > +     spinlock_t                      lock;
> > > > 
> > > > I am not entirely convinced we need that lock. The i2c core will 
> > > > take a mutex protecting all operations on the bus. So we only 
> > > > need to synchronize between our "xfer" code and our interrupt 
> > > > handler.
> > > 
> > > You are right if both having slave and master active at the same 
> > > time was not possible; however, it is.
> > 
> > Right, I somewhat forgot about the slave case.
> > 
> >   ...
> > 
> > > > Some of those error states probably also warrant a reset of the 
> > > > controller, I think aspeed does that in the SDK.
> > > 
> > > For timeout and cmd_err, I do not see any argument against it; it 
> > > sounds like we are in a very messed up, very unknown state, so 
> > > full reset is probably the best last resort.
> > 
> > Yup.
> > 
> > > For SDA staying pulled down, I
> > > think we can say with reasonable confidence that some device on 
> > > our bus is behaving very badly and I am not convinced that 
> > > resetting the controller is likely to do anything to help;
> > 
> > Right. Hammering with STOPs and pray ...
> 
> I think sending recovery mode sends stops as a part of the recovery 
> algorithm it executes.
> 
> > 
> > >  that being said, I really
> > > do not have any good ideas to address that. So maybe praying and 
> > > resetting the controller is *the most reasonable thing to do.* I 
> > > would like to know what you think we should do in that case.
> > 
> > Well, there's a (small ?) chance that it's a controller bug 
> > asserting the line so ... but there's little we can do if not.
> 
> True.
> 
> > 
> > > While I was thinking about this I also realized that the SDA line 
> > > check after recovery happens in the else branch, but SCL line 
> > > check does not happen after we attempt to STOP if SCL is hung. If 
> > > we decide to make special note SDA being hung by a device that 
> > > won't let go, we might want to make a special note that SCL is 
> > > hung by a device that won't let go. Just a thought.
> > 
> > Maybe. Or just "unrecoverable error"... hopefully these don't happen 
> > too often ... We had cases of a TPM misbehaving like that.
> 
> Yeah, definitely should print something out.
> 
> > 
> > > > > +out:
> > > 
> > > ...
> > > > What about I2C_M_NOSTART ?
> > > > 
> > > > Not that I've ever seen it used... ;-)
> > > 
> > > Right now I am not doing any of the protocol mangling options, but 
> > > I can add them in if you think it is important for initial 
> > > support.
> > 
> > No, not important, we can add that later if it ever becomes useful.
> > 
> >  ...
> > 
> > > > In general, you always ACK all interrupts first. Then you handle 
> > > > the bits you have harvested.
> > > > 
> > > 
> > > The documentation says to ACK the interrupt after handling in the 
> > > RX
> > > case:
> > > 
> > > <<<
> > > S/W needs to clear this status bit to allow next data receiving.
> > > > > > 
> > > 
> > > I will double check with Ryan to make sure TX works the same way.
> > > 
> > > > > +     if (irq_status & ASPEED_I2CD_INTR_ERROR ||
> > > > > +         (!bus->msgs && bus->master_state !=
> > > > > ASPEED_I2C_MASTER_STOP)) {
> > > 
> > > ...
> > > > 
> > > > I would set master_state to "RECOVERY" (new state ?) and ensure 
> > > > those things are caught if they happen outside of a recovery.
> > 
> > I replied privately ... as long as we ack before we start a new 
> > command we should be ok but we shouldn't ack after.
> > 
> > Your latest patch still does that. It will do things like start a 
> > STOP command *then* ack the status bits. I'm pretty sure that's 
> > bogus.
> > 
> > That way it's a lot simpler to simply move the
> > 
> >         writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
> > 
> > To either right after the readl of the status reg at the beginning 
> > of aspeed_i2c_master_irq().
> > 
> > I would be very surprised if that didn't work properly and wasn't 
> > much safer than what you are currently doing.
> 
> I think I tried your way and it worked. In anycase, Ryan will be able 
> to clarify for us.
> 
> > 
> > > Let me know if you still think we need a "RECOVERY" state.
> > 
> > The way you just switch to stop state and store the error for later 
> > should work I think.
> > 
> > > > 
> > > > > +     if (bus->master_state == ASPEED_I2C_MASTER_START) {
> > > 
> > > ...
> > > > 
> > > > > +                     dev_dbg(bus->dev,
> > > > > +                             "no slave present at %02x",
> > > > > msg-
> > > > > > addr);
> > > > > 
> > > > > +                     status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> > > > > +                     bus->cmd_err = -EIO;
> > > > > +                     do_stop(bus);
> > > > > +                     goto out_no_complete;
> > > > > +             } else {
> > > > > +                     status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> > > > > +                     if (msg->flags & I2C_M_RD)
> > > > > +                             bus->master_state =
> > > > > ASPEED_I2C_MASTER_RX;
> > > > > +                     else
> > > > > +                             bus->master_state =
> > > > > ASPEED_I2C_MASTER_TX_FIRST;
> > > > 
> > > > What about the SMBUS_QUICK case ? (0-len transfer). Do we need 
> > > > to handle this here ? A quick look at the TX_FIRST case makes me 
> > > > think we are ok there but I'm not sure about the RX case.
> > > 
> > > I did not think that there is an SMBUS_QUICK RX. Could you point 
> > > me to an example?
> > 
> > Not so much an RX, it's more like you are sending a 1-bit data in 
> > the place of the Rd/Wr bit. So you have a read with a lenght of 0, I 
> > don't think in that case you should set ASPEED_I2CD_M_RX_CMD in 
> > __aspeed_i2c_do_start
> 
> Forget what I said, I was just not thinking about the fact that SMBus 
> emulation causes the data bit to be encoded as the R/W flag. I see 
> what you are saying; you are correct.
> 
> > 
> > > > I'm not sure the RX case is tight also. What completion does the 
> > > > HW give you for the address cycle ? Won't you get that before it 
> > > > has received the first character ? IE. You fall through to the 
> > > > read case of the state machine with the read potentially not 
> > > > complete yet no ?
> > > 
> > > ...
> > > > > +     case ASPEED_I2C_MASTER_RX:
> > > > > +             if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE)) {
> > > > > +                     dev_err(bus->dev, "master failed to
> > > > > RX");
> > > > > +                     goto out_complete;
> > > > > +             }
> > > > 
> > > > See my comment above for a bog standard i2c_read. Aren't you 
> > > > getting the completion for the address before the read is even 
> > > > started ?
> > > 
> > > In practice no, but it is probably best to be safe :-)
> > 
> > Yup :)
> > > > 
> > > > > +             status_ack |= ASPEED_I2CD_INTR_RX_DONE;
> > > > > +
> > > > > +             recv_byte = aspeed_i2c_read(bus,
> > > > > ASPEED_I2C_BYTE_BUF_REG) >> 8;
> > > > > +             msg->buf[bus->buf_index++] = recv_byte;
> > > > > +
> > > > > +             if (msg->flags & I2C_M_RECV_LEN &&
> > > > > +                 recv_byte <= I2C_SMBUS_BLOCK_MAX) {
> > > > > +                     msg->len = recv_byte +
> > > > > +                                     ((msg->flags &
> > > > > I2C_CLIENT_PEC) ? 2 : 1);
> > > 
> > > ...
> > > > > +     return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
> > > > > +             & ASPEED_I2CD_TIME_SCL_HIGH_MASK)
> > > > > +                     | ((clk_low <<
> > > > > ASPEED_I2CD_TIME_SCL_LOW_SHIFT)
> > > > > +                        & ASPEED_I2CD_TIME_SCL_LOW_MASK)
> > > > > +                     | (base_clk &
> > > > > ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
> > > > > +}
> > > > 
> > > > As I think I mentioned earlier, the AST2500 has a slightly 
> > > > different register layout which support larger values for high 
> > > > and low, thus allowing a finer granularity.
> > > 
> > > I am developing against the 2500.
> > 
> > Yes but we'd like the driver to work with both :-)
> 
> Right, I thought you were making an assertion about the 2500, if you 
> are making an assertion about the 2400, I do not know and do not have 
> one handy.
> 
> > 
> > > > BTW. In case you haven't, I would suggest you copy/paste the 
> > > > above in a userspace app and run it for all frequency divisors 
> > > > and see if your results match the aspeed table :)
> > > 
> > > Good call.
> > 
> > If you end up doing that, can you shoot it my way ? I can take care 
> > of making sure it's all good for the 2400.
> 
> Will do.
> 
> > 
> > > > > +static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus,
> > > > > +                            struct platform_device *pdev) {
> > > > > +     u32 clk_freq, divisor;
> > > > > +     struct clk *pclk;
> > > > > +     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);
> > > > 
> > > > See my previous comment about calling that 'bus-frequency'
> > > > rather
> > > > than 'clock-frequency'.
> > > > 
> > > > > +     if (ret < 0) {
> > > > > +             dev_err(&pdev->dev,
> > > > > +                     "Could not read clock-frequency
> > > > > property\n");
> > > > > +             clk_freq = 100000;
> > > > > +     }
> > > > > +     divisor = 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 > 1000) {
> > > > > +             aspeed_i2c_write(bus, aspeed_i2c_read(bus,
> > > > > +                                                   ASPEED_I2
> > > > > C_FU
> > > > > N_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(divisor),
> > > > > +                              ASPEED_I2C_AC_TIMING_REG1);
> > > > 
> > > > I already discussed by doubts about the above. I can try to 
> > > > scope it with the EVB if you don't get to it. For now I'd rather 
> > > > take the code out.
> > > > 
> > > > We should ask aspeed from what frequency the "1T" stuff is 
> > > > useful.
> > > 
> > > Will do, I will try to rope Ryan in on the next review; it will be 
> > > good for him to get used to working with upstream anyway.
> > 
> > Yup. However, for the sake of getting something upstream (and in 
> > OpenBMC 4.10 kernel) asap, I would suggest just dropping support for 
> > those fast speeds for now, we can add them back later.
> 
> Alright, that's fine. Still, Ryan, could you provide some context on 
> this?
> 
> > 
> > > > 
> > > > > +     } else {
> > > > > +             aspeed_i2c_write(bus,
> > > > > aspeed_i2c_get_clk_reg_val(divisor),
> > > > > +                              ASPEED_I2C_AC_TIMING_REG1);
> > > > > +             aspeed_i2c_write(bus, ASPEED_NO_TIMEOUT_CTRL,
> > > > > +                              ASPEED_I2C_AC_TIMING_REG2);
> > > > > +     }
> > > 
> > > ...
> > > > > +     spin_lock_init(&bus->lock);
> > > > > +     init_completion(&bus->cmd_complete);
> > > > > +     bus->adap.owner = THIS_MODULE;
> > > > > +     bus->adap.retries = 0;
> > > > > +     bus->adap.timeout = 5 * HZ;
> > > > > +     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");
> > > > 
> > > > Another trivial one, should we put some kind of bus number in 
> > > > that string ?
> > > 
> > > Whoops, looks like I missed this one; I will get to it in the next 
> > > revision.
> > 
> > Ok. I noticed you missed that in v7, so I assume you mean v8 :-)
> 
> Yep, I will get it in v8.
> 
> > 
> > > > 
> > > > > +     bus->dev = &pdev->dev;
> > > > > +
> > > > > +     /* reset device: disable master & slave functions */
> > > > > +     aspeed_i2c_write(bus, 0, ASPEED_I2C_FUN_CTRL_REG);
> > > 
> > > ...
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe 
> > > devicetree"
> > > in
> > > the body of a message to majordomo@vger.kernel.org More majordomo 
> > > info at  http://vger.kernel.org/majordomo-info.html
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i

Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C

[Brendan Higgins]

On Tue, Apr 25, 2017 at 2:47 AM, Ryan Chen <ryan_chen@aspeedtech.com> wrote:
> Thanks Ryan. Can you shed some light on the meaning of the high-speed bit as well please ?
>
> About ASPEED_I2CD_M_HIGH_SPEED_EN, it is support for I2C specification "High speed transfer". And also device need support it.
> If you just speed up the I2C bus clock, you don’t have to enable ASPEED_I2CD_M_HIGH_SPEED_EN, just change the clock is ok.
>

Interesting, I thought that ASPEED_I2CD_M_SDA_DRIVE_1T_EN and its
counterpart would be used for fast mode or fast mode plus and
ASPEED_I2CD_M_HIGH_SPEED_EN would be used for fast mode plus or high
speed mode and that they work by driving the SDA and SCL signals to
improve rise times. It made sense to me because the lowest SCL you can
get with base clock set to zero is about ~1.5MHz which is in between
fast mode plus (1MHz) and high speed mode (3.4MHz).

But from what you are saying, ASPEED_I2CD_M_SDA_DRIVE_1T_EN and its
counterpart are totally orthogonal to the selected speed and
ASPEED_I2CD_M_HIGH_SPEED_EN exists as a matter of convenience to set
all of the divider registers to their smallest possible values. Is my
understanding correct?

>
> -----Original Message-----
> From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org]
> Sent: Tuesday, April 25, 2017 5:35 PM
> To: Ryan Chen <ryan_chen@aspeedtech.com>; Brendan Higgins <brendanhiggins@google.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>; Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Thomas Gleixner <tglx@linutronix.de>; Jason Cooper <jason@lakedaemon.net>; Marc Zyngier <marc.zyngier@arm.com>; Joel Stanley <joel@jms.id.au>; Vladimir Zapolskiy <vz@mleia.com>; Kachalov Anton <mouse@mayc.ru>; Cédric Le Goater <clg@kaod.org>; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; OpenBMC Maillist <openbmc@lists.ozlabs.org>
> Subject: Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C
>
> On Tue, 2017-04-25 at 08:50 +0000, Ryan Chen wrote:
>> Hello All,
>>               ASPEED_I2CD_M_SDA_DRIVE_1T_EN,
>> ASPEED_I2CD_SDA_DRIVE_1T_EN is specific for some case usage.
>>               For example, if i2c bus is use on "high speed" and "single slave and
>> master" and i2c bus is too long. It need drive SDA or SCL less lunacy.
>> It would enable it.
>>               Otherwise, don’t enable it. especially in multi-master.
>> It can’t be enable.
>
> That smells like a specific enough use case that we should probably cover with a device-tree property, something like an empty "sda-extra-drive" property (empty properties are typically used for booleans, their presence means "true").
>
> Thanks Ryan. Can you shed some light on the meaning of the high-speed bit as well please ? Does it force to a specific speed (ignoring the
> divisor) or we can still play with the clock high/low counts ?
>
...
>> > Your latest patch still does that. It will do things like start a
>> > STOP command *then* ack the status bits. I'm pretty sure that's
>> > bogus.
>> >
>> > That way it's a lot simpler to simply move the
>> >
>> >         writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
>> >
>> > To either right after the readl of the status reg at the beginning
>> > of aspeed_i2c_master_irq().
>> >
>> > I would be very surprised if that didn't work properly and wasn't
>> > much safer than what you are currently doing.
>>
>> I think I tried your way and it worked. In anycase, Ryan will be able
>> to clarify for us.

After thinking about this more, I think Ben is right. It would be
unusual for such a common convention to be broken and even if it is, I
do not see how a command could take effect until it is actually
issued. Nevertheless, it would make me feel better if you, Ryan, could
comment on this.

>>
>> >
>> > > Let me know if you still think we need a "RECOVERY" state.
>> >
...
I feel pretty good about this; it does not look like there will be a
lot of changes going into v8; hopefully, that version will be good
enough to get merged.

RE: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C

[Ryan Chen]

> Thanks Ryan. Can you shed some light on the meaning of the high-speed bit as well please ?
>
> About ASPEED_I2CD_M_HIGH_SPEED_EN, it is support for I2C specification "High speed transfer". And also device need support it.
> If you just speed up the I2C bus clock, you don’t have to enable ASPEED_I2CD_M_HIGH_SPEED_EN, just change the clock is ok.
>

>Interesting, I thought that ASPEED_I2CD_M_SDA_DRIVE_1T_EN and its counterpart would be used for fast mode or fast mode plus and ASPEED_I2CD_M_HIGH_SPEED_EN would be used for fast mode plus or high speed mode and that they work by driving the SDA and SCL signals to >improve rise times. It made sense to me because the lowest SCL you can get with base clock set to zero is about ~1.5MHz which is in between fast mode plus (1MHz) and high speed mode (3.4MHz).

>But from what you are saying, ASPEED_I2CD_M_SDA_DRIVE_1T_EN and its counterpart are totally orthogonal to the selected speed and ASPEED_I2CD_M_HIGH_SPEED_EN exists as a matter of convenience to set all of the divider registers to their smallest possible values. Is my >
>understanding correct?


In I2c specification[http://www.csd.uoc.gr/~hy428/reading/i2c_spec.pdf] there have a chapter about high speed transfer. It will start from specific command (00001XXX) and after that can transfer to high speed mode. 
The following is our high speed mode programming guide. That also have description at AST2400 datasheet. 40.7.12



>
> -----Original Message-----
> From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org]
> Sent: Tuesday, April 25, 2017 5:35 PM
> To: Ryan Chen <ryan_chen@aspeedtech.com>; Brendan Higgins 
> <brendanhiggins@google.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>; Rob Herring 
> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Thomas 
> Gleixner <tglx@linutronix.de>; Jason Cooper <jason@lakedaemon.net>; 
> Marc Zyngier <marc.zyngier@arm.com>; Joel Stanley <joel@jms.id.au>; 
> Vladimir Zapolskiy <vz@mleia.com>; Kachalov Anton <mouse@mayc.ru>; 
> Cédric Le Goater <clg@kaod.org>; linux-i2c@vger.kernel.org; 
> devicetree@vger.kernel.org; Linux Kernel Mailing List 
> <linux-kernel@vger.kernel.org>; OpenBMC Maillist 
> <openbmc@lists.ozlabs.org>
> Subject: Re: [PATCH v6 4/5] i2c: aspeed: added driver for Aspeed I2C
>
> On Tue, 2017-04-25 at 08:50 +0000, Ryan Chen wrote:
>> Hello All,
>>               ASPEED_I2CD_M_SDA_DRIVE_1T_EN, 
>> ASPEED_I2CD_SDA_DRIVE_1T_EN is specific for some case usage.
>>               For example, if i2c bus is use on "high speed" and 
>> "single slave and master" and i2c bus is too long. It need drive SDA or SCL less lunacy.
>> It would enable it.
>>               Otherwise, don’t enable it. especially in multi-master.
>> It can’t be enable.
>
> That smells like a specific enough use case that we should probably cover with a device-tree property, something like an empty "sda-extra-drive" property (empty properties are typically used for booleans, their presence means "true").
>
> Thanks Ryan. Can you shed some light on the meaning of the high-speed 
> bit as well please ? Does it force to a specific speed (ignoring the
> divisor) or we can still play with the clock high/low counts ?
>
...
>> > Your latest patch still does that. It will do things like start a 
>> > STOP command *then* ack the status bits. I'm pretty sure that's 
>> > bogus.
>> >
>> > That way it's a lot simpler to simply move the
>> >
>> >         writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
>> >
>> > To either right after the readl of the status reg at the beginning 
>> > of aspeed_i2c_master_irq().
>> >
>> > I would be very surprised if that didn't work properly and wasn't 
>> > much safer than what you are currently doing.
>>
>> I think I tried your way and it worked. In anycase, Ryan will be able 
>> to clarify for us.

After thinking about this more, I think Ben is right. It would be unusual for such a common convention to be broken and even if it is, I do not see how a command could take effect until it is actually issued. Nevertheless, it would make me feel better if you, Ryan, could comment on this.

>>
>> >
>> > > Let me know if you still think we need a "RECOVERY" state.
>> >
...
I feel pretty good about this; it does not look like there will be a lot of changes going into v8; hopefully, that version will be good enough to get merged.

Re: [PATCH v6 0/5] i2c: aspeed: added driver for Aspeed I2C

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2348 bytes --]

On Mon, 2017-03-27 at 22:12 -0700, Brendan Higgins wrote:
> Sorry for the delay, I went on a long vacation prior to receiving feedback and
> got back in the middle of a hardware bring up that consumed all of my attention
> for an extended period of time. I will try to plan upstream submissions around
> my other responsibilities better in the future.
> 
> Addressed comments from:
>   - Vladimir in: https://www.spinics.net/lists/linux-i2c/msg27387.html
>     and: https://www.spinics.net/lists/linux-i2c/msg27386.html
>   - Wolfram in: https://www.spinics.net/lists/linux-i2c/msg27476.html
>     and: https://www.spinics.net/lists/linux-i2c/msg27483.html
> 
> Changes since previous update:
>   - No longer arbitrarily restrict bus to be slave xor master.
>   - Pulled out "struct aspeed_i2c_controller" as a interrupt controller.
>   - Pulled out slave support into its own commit.
>   - Rewrote code that sets clock divider register because the original version
>     set it incorrectly.
>   - Discovered and fixed issue in implementation that caused certain slave
>     devices to misbehave; the cause was that the master IRQ handler would return
>     control to the requesting thread after the last RX or TX command was handled
>     such that the requesting thread would issue either a repeated start or stop.
>     This was incorrect because the time taken to complete the completion was too
>     great. I fixed this by rewriting the master IRQ handler so that it now
>     manages the entire transaction only returning control to the requesting
>     thread once the entire transaction is complete.
>   - Rewrote the aspeed_i2c_master_irq handler because the old method of
>     completing a completion in between restarts was too slow causing devices to
>     misbehave.
>   - Added support for I2C_M_RECV_LEN which I had incorrectly said was supported
>     before.
>   - Addressed other comments from Vladimir.
> 
> Changes have been tested on the Aspeed 2500 evaluation board, as before, and now
> on a real platform with an Aspeed 2520.

Looks like there's going to be another revision of the series, but
regardless, I've applied and tested v6 and had no issues. So:

Tested-by: Andrew Jeffery <andrew-zrmu5oMJ5Fs@public.gmane.org>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH v6 5/5] i2c: aspeed: added slave support for Aspeed I2C driver

[Brendan Higgins]

Added 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>
---
Added in v6:
  - Pulled slave support out of initial driver commit into its own commit.
  - No longer arbitrarily restrict bus to be slave xor master.
---
 drivers/i2c/busses/i2c-aspeed.c | 186 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 186 insertions(+)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 04266acc6c46..a9ee58a2c4e2 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -49,6 +49,7 @@
 #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)
 
 /* 0x04 : I2CD Clock and AC Timing Control Register #1 */
@@ -69,6 +70,7 @@
  */
 #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)
@@ -106,6 +108,9 @@
 #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_master_state {
 	ASPEED_I2C_MASTER_START,
 	ASPEED_I2C_MASTER_TX_FIRST,
@@ -115,6 +120,15 @@ enum aspeed_i2c_master_state {
 	ASPEED_I2C_MASTER_INACTIVE,
 };
 
+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;
@@ -207,6 +221,110 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
 	return ret;
 }
 
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
+{
+	u32 command, irq_status, status_ack = 0;
+	struct i2c_client *slave = bus->slave;
+	bool irq_handled = true;
+	u8 value;
+
+	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;
+	}
+
+	switch (bus->slave_state) {
+	case ASPEED_I2C_SLAVE_READ_REQUESTED:
+		if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
+			dev_err(bus->dev, "Unexpected ACK on read request.\n");
+		bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
+
+		i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
+		aspeed_i2c_write(bus, value, ASPEED_I2C_BYTE_BUF_REG);
+		aspeed_i2c_write(bus, ASPEED_I2CD_S_TX_CMD, ASPEED_I2C_CMD_REG);
+		break;
+	case ASPEED_I2C_SLAVE_READ_PROCESSED:
+		status_ack |= ASPEED_I2CD_INTR_TX_ACK;
+		if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
+			dev_err(bus->dev,
+				"Expected ACK after processed read.\n");
+		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);
+		break;
+	case ASPEED_I2C_SLAVE_WRITE_REQUESTED:
+		bus->slave_state = ASPEED_I2C_SLAVE_WRITE_RECEIVED;
+		i2c_slave_event(slave, I2C_SLAVE_WRITE_REQUESTED, &value);
+		break;
+	case ASPEED_I2C_SLAVE_WRITE_RECEIVED:
+		i2c_slave_event(slave, I2C_SLAVE_WRITE_RECEIVED, &value);
+		break;
+	case ASPEED_I2C_SLAVE_STOP:
+		i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
+		break;
+	default:
+		dev_err(bus->dev, "unhandled slave_state: %d\n",
+			bus->slave_state);
+		break;
+	}
+
+	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 void do_start(struct aspeed_i2c_bus *bus)
 {
 	u32 command = ASPEED_I2CD_M_START_CMD | ASPEED_I2CD_M_TX_CMD;
@@ -371,6 +489,14 @@ 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
+
+	dev_dbg(bus->dev, "irq handled by master.\n");
 	aspeed_i2c_master_irq(bus);
 	return IRQ_HANDLED;
 }
@@ -426,9 +552,69 @@ 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)
+{
+	u32 addr_reg_val, func_ctrl_reg_val;
+	struct aspeed_i2c_bus *bus;
+	unsigned long flags;
+
+	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);
+
+	/* Turn on slave mode. */
+	func_ctrl_reg_val = aspeed_i2c_read(bus, ASPEED_I2C_FUN_CTRL_REG);
+	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;
+	u32 func_ctrl_reg_val;
+	unsigned long flags;
+
+	spin_lock_irqsave(&bus->lock, flags);
+	if (!bus->slave) {
+		spin_unlock_irqrestore(&bus->lock, flags);
+		return -EINVAL;
+	}
+
+	/* Turn off slave mode. */
+	func_ctrl_reg_val = aspeed_i2c_read(bus, ASPEED_I2C_FUN_CTRL_REG);
+	func_ctrl_reg_val &= ~ASPEED_I2CD_SLAVE_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 divisor)
-- 
2.12.2.564.g063fe858b8-goog