diff mbox

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

Message ID 20170328051226.21677-2-brendanhiggins@google.com
State Superseded
Headers show

Commit Message

Brendan Higgins March 28, 2017, 5:12 a.m. UTC
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

Comments

Benjamin Herrenschmidt March 28, 2017, 8:49 a.m. UTC | #1
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;
> +};
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Brendan Higgins March 29, 2017, 10:34 a.m. UTC | #2
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Herrenschmidt March 29, 2017, 12:11 p.m. UTC | #3
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Brendan Higgins March 29, 2017, 8:51 p.m. UTC | #4
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Herrenschmidt March 29, 2017, 9:17 p.m. UTC | #5
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring April 3, 2017, 2:16 p.m. UTC | #6
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@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

Acked-by: Rob Herring <robh@kernel.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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