| Message ID | 20170328051226.21677-2-brendanhiggins@google.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
> 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
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
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 --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; +};
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