| Message ID | 1374223663-8576-4-git-send-email-danindrey@mail.ru |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Warren |
| Headers | show |
On 07/19/2013 02:47 AM, Andrey Danin wrote: > Signed-off-by: Andrey Danin <danindrey@mail.ru> (Some patch descriptions would be useful) > diff --git a/board/compal/dts/tegra20-paz00.dts b/board/compal/dts/tegra20-paz00.dts > + nvec { > + compatible = "nvidia,tegra20-nvec"; > + reg = <0x7000c500 0x100>; > + clock-frequency = <80000>; > + request-gpios = <&gpio 170 0>; /* gpio PV2 */ > + slave-addr = <138>; > + }; I would rather not propagate this DT binding. We need to fix the binding to clearly separate the concepts of: a) The I2C slave controller (which should be a standalone driver for the Tegra I2C slave HW). b) The protocol sent over the I2C slave channel (which would be specific to NVEC, implement the GPIO hand-shaking, etc.). c) The devices that communicate over the protocol (keyboard in this case). I suspect we need separate DT nodes/sub-nodes for all of those, and a method of hooking them all together.
On Friday 19 July 2013 13:14:13 Stephen Warren wrote: > On 07/19/2013 02:47 AM, Andrey Danin wrote: > > Signed-off-by: Andrey Danin <danindrey@mail.ru> > > (Some patch descriptions would be useful) > > > diff --git a/board/compal/dts/tegra20-paz00.dts > > b/board/compal/dts/tegra20-paz00.dts > > > > + nvec { > > + compatible = "nvidia,tegra20-nvec"; > > + reg = <0x7000c500 0x100>; > > + clock-frequency = <80000>; > > + request-gpios = <&gpio 170 0>; /* gpio PV2 */ > > + slave-addr = <138>; > > + }; > > I would rather not propagate this DT binding. We need to fix the binding > to clearly separate the concepts of: so here we go again. I think I have to take this on my shoulders since I didn't got it right yet in the kernel. > a) The I2C slave controller (which should be a standalone driver for the > Tegra I2C slave HW). > > b) The protocol sent over the I2C slave channel (which would be specific > to NVEC, implement the GPIO hand-shaking, etc.). > > c) The devices that communicate over the protocol (keyboard in this case). > > I suspect we need separate DT nodes/sub-nodes for all of those, and a > method of hooking them all together. Let's skip how this may actually look like in software. Given the discussions we had in the past, I propose the following binding: i2c-slave@7000c500 { compatible = "nvidia,tegra20-i2c-slave"; reg = <0x7000c500 0x100>; interrupts = <0 92 0x04>; #address-cells = <1>; #size-cells = <0>; clock-frequency = <80000>; slave-addr = <138>; clocks = <&tegra_car 67>, <&tegra_car 124>; clock-names = "div-clk", "fast-clk"; nvec { compatible = "nvidia,nvec", "simple-bus"; protocol = "smbus-request-gpio"; request-gpios = <&gpio 170 0>; /* gpio PV2 */ keyboard { compatible = "nvidia,nvec-keyboard"; }; }; }; Does this looks better? Marc
On 07/20/2013 03:12 AM, Marc Dietrich wrote: > On Friday 19 July 2013 13:14:13 Stephen Warren wrote: ... > Let's skip how this may actually look like in software. Given the discussions > we had in the past, I propose the following binding: > > i2c-slave@7000c500 { > compatible = "nvidia,tegra20-i2c-slave"; > reg = <0x7000c500 0x100>; > interrupts = <0 92 0x04>; > #address-cells = <1>; > #size-cells = <0>; > clock-frequency = <80000>; > slave-addr = <138>; Hex would be more common, but that's a minor issue. > clocks = <&tegra_car 67>, <&tegra_car 124>; > clock-names = "div-clk", "fast-clk"; > > nvec { Above, it says #address-cells=<1>, which means this node needs a reg property. Perhaps slave-addr should be part of the child nodes (and the Tegra I2C controller binding would limit itself to supporting only a single node), so that the same binding style could be applicable to I2C slave devices that support multiple slave addresses. > compatible = "nvidia,nvec", "simple-bus"; > protocol = "smbus-request-gpio"; What is that property for; doesn't compatible="nvidia,nvec" already imply this, or does the NVEC spec define multiple different protocols? > request-gpios = <&gpio 170 0>; /* gpio PV2 */ We should use the C pre-processor to provide named constants there, although I guess U-Boot isn't set up for that yet. The kernel is once this is ported there, and once the 2013.07 release is out, U-Boot should be able to support this very soon too. > keyboard { Simple-bus might require a reg property; I forget. Does the NVEC protocol include any form of "virtual device address" that it would make sense to put into a reg property? > compatible = "nvidia,nvec-keyboard"; > }; > }; > }; > > Does this looks better? Yes, overall much better. New DT bindings should be sent to devicetree@vger.kernel.org for review. Note that's a branch new list (it moved from a different server), so it might be best to wait a few days for people to subscribe before sending mail to it.
Am Samstag, 20. Juli 2013, 21:20:52 schrieb Stephen Warren: > On 07/20/2013 03:12 AM, Marc Dietrich wrote: > > On Friday 19 July 2013 13:14:13 Stephen Warren wrote: > ... > > > Let's skip how this may actually look like in software. Given the > > discussions we had in the past, I propose the following binding: > > > > i2c-slave@7000c500 { > > > > compatible = "nvidia,tegra20-i2c-slave"; > > reg = <0x7000c500 0x100>; > > interrupts = <0 92 0x04>; > > #address-cells = <1>; > > #size-cells = <0>; > > clock-frequency = <80000>; > > slave-addr = <138>; > > Hex would be more common, but that's a minor issue. ok. > > clocks = <&tegra_car 67>, <&tegra_car 124>; > > clock-names = "div-clk", "fast-clk"; > > > > nvec { > > Above, it says #address-cells=<1>, which means this node needs a reg > property. Perhaps slave-addr should be part of the child nodes (and the > Tegra I2C controller binding would limit itself to supporting only a > single node), so that the same binding style could be applicable to I2C > slave devices that support multiple slave addresses. you mean nvec@87 { reg = <0x87>; ... } ? I think that's ok. Didn't know that Tegra can support multiple slave addresses. To make the binding as general as possible, we could support multi- slave for the binding, but only support single slave in the code for now. I guess that also warrants a "simple-bus" compatibility in the i2c-slave node. > > > compatible = "nvidia,nvec", "simple-bus"; > > protocol = "smbus-request-gpio"; > > What is that property for; doesn't compatible="nvidia,nvec" already > imply this, or does the NVEC spec define multiple different protocols? The GPIO is optional, but SMBUS is required. Maybe to support master initiated communications only. To get rid of the ugly protocol property, we could just check if a valid gpio is given. I think that's what the downstream kernel also does. The nvec still needs to tell the slave driver which protocol to use, but that can be hard coded. > > request-gpios = <&gpio 170 0>; /* gpio PV2 */ > > We should use the C pre-processor to provide named constants there, > although I guess U-Boot isn't set up for that yet. The kernel is once > this is ported there, and once the 2013.07 release is out, U-Boot should > be able to support this very soon too. > > > keyboard { > > Simple-bus might require a reg property; I forget. Does the NVEC > protocol include any form of "virtual device address" that it would make > sense to put into a reg property? > > > compatible = "nvidia,nvec-keyboard"; well, we could misuse the nvec function select byte for this (e.g. 5 for keyboard, 6 for mouse, ...), but we may run into trouble with the "system command = 1" which is used by several drivers. > > > > Does this looks better? > > Yes, overall much better. > > New DT bindings should be sent to devicetree@vger.kernel.org for review. > Note that's a branch new list (it moved from a different server), so it > might be best to wait a few days for people to subscribe before sending > mail to it. Ok, I'll send out a new version and cc dtml. Maybe this some more people respond ;-) Thanks for review! Marc
On 07/22/2013 01:09 AM, Marc Dietrich wrote: > Am Samstag, 20. Juli 2013, 21:20:52 schrieb Stephen Warren: >> On 07/20/2013 03:12 AM, Marc Dietrich wrote: >>> On Friday 19 July 2013 13:14:13 Stephen Warren wrote: >> ... >> >>> Let's skip how this may actually look like in software. Given the >>> discussions we had in the past, I propose the following binding: >>> >>> i2c-slave@7000c500 { >>> #address-cells = <1>; >>> #size-cells = <0>; >>> nvec { >> >> Above, it says #address-cells=<1>, which means this node needs a reg >> property. Perhaps slave-addr should be part of the child nodes (and the >> Tegra I2C controller binding would limit itself to supporting only a >> single node), so that the same binding style could be applicable to I2C >> slave devices that support multiple slave addresses. > > you mean > > nvec@87 { > reg = <0x87>; > ... > } ? Yes. > I think that's ok. Didn't know that Tegra can support multiple slave > addresses. To make the binding as general as possible, we could support multi- > slave for the binding, but only support single slave in the code for now. Tegra can't, but that doesn't mean some future Tegra or some other SoC can't/won't. Putting the slave address inside the child node makes sure the same DT schema will work in other situations, and hence be consistent. > I guess that also warrants a "simple-bus" compatibility in the i2c-slave node. > >> >>> compatible = "nvidia,nvec", "simple-bus"; >>> protocol = "smbus-request-gpio"; >> >> What is that property for; doesn't compatible="nvidia,nvec" already >> imply this, or does the NVEC spec define multiple different protocols? > > The GPIO is optional, but SMBUS is required. Maybe to support master initiated > communications only. To get rid of the ugly protocol property, we could just > check if a valid gpio is given. I think that's what the downstream kernel also > does. Yes, I believe nvidia,nvec implies everything about the protocol then, except for the optional GPIO which can be checked directly. > The nvec still needs to tell the slave driver which protocol to use, but > that can be hard coded. I'm not sure what that means. At the controller/HW level, aren't I2C and SMBUS the same; it's just the data within the transactions that may be more defined by one or the other?
On Tuesday 23 July 2013 08:40:42 Stephen Warren wrote: > On 07/22/2013 01:09 AM, Marc Dietrich wrote: [ snip the stuff we agreed upon ] > > The nvec still needs to tell the slave driver which protocol to use, but > > that can be hard coded. > > I'm not sure what that means. At the controller/HW level, aren't I2C and > SMBUS the same; it's just the data within the transactions that may be > more defined by one or the other? at this level yes, but we need to handle the underlying protocol in the ISR, which means that depending on the protocol (smbus or I2C), we need a different interrupt service routine. Currently we are doing most of the smbus protocol in the ISR because of timing reasons and I'm not sure if we can change this. We are already suffering from nvec timeouts and I fear that splitting the protocol out of the ISR would make things even worse. I'm going to submit a new DT binding during the weekend. Meanwhile, Andrey is trying to split the uboot driver in the hw and nvec parts and also adapting it for the new binding. Marc
On 07/24/2013 10:52 AM, Marc Dietrich wrote: > On Tuesday 23 July 2013 08:40:42 Stephen Warren wrote: >> On 07/22/2013 01:09 AM, Marc Dietrich wrote: > > [ snip the stuff we agreed upon ] > >>> The nvec still needs to tell the slave driver which protocol to use, but >>> that can be hard coded. >> >> I'm not sure what that means. At the controller/HW level, aren't I2C and >> SMBUS the same; it's just the data within the transactions that may be >> more defined by one or the other? > > at this level yes, but we need to handle the underlying protocol in the ISR, > which means that depending on the protocol (smbus or I2C), we need a different > interrupt service routine. Currently we are doing most of the smbus protocol > in the ISR because of timing reasons and I'm not sure if we can change this. > We are already suffering from nvec timeouts and I fear that splitting the > protocol out of the ISR would make things even worse. I assume that the I2C slave driver's ISR would simply directly/immediately call a function in the "protocol driver". That should allow sufficient separation of the layers while still maintaining minimal latency, assuming a good design of the callback's function prototype and/or the list of I2C slave controller functions that the callback is allowed to call into.
diff --git a/board/compal/dts/tegra20-paz00.dts b/board/compal/dts/tegra20-paz00.dts index 780203c..2df5446 100644 --- a/board/compal/dts/tegra20-paz00.dts +++ b/board/compal/dts/tegra20-paz00.dts @@ -88,4 +88,12 @@ nvidia,panel-vdd-gpios = <&gpio 4 0>; /* PA4 */ nvidia,panel-timings = <400 4 203 17 15>; }; + + nvec { + compatible = "nvidia,tegra20-nvec"; + reg = <0x7000c500 0x100>; + clock-frequency = <80000>; + request-gpios = <&gpio 170 0>; /* gpio PV2 */ + slave-addr = <138>; + }; }; diff --git a/include/configs/paz00.h b/include/configs/paz00.h index eac1ef9..d711be8 100644 --- a/include/configs/paz00.h +++ b/include/configs/paz00.h @@ -72,6 +72,14 @@ #define CONFIG_SYS_WHITE_ON_BLACK #define CONFIG_CONSOLE_SCROLL_LINES 10 +/* Keyboard support */ +#define CONFIG_KEYBOARD +#define CONFIG_TEGRA_NVEC_KEYBOARD +/* NVEC support */ +#define CONFIG_TEGRA_I2C +#define CONFIG_SYS_I2C_INIT_BOARD +#define CONFIG_TEGRA_NVEC + #include "tegra-common-post.h" #endif /* __CONFIG_H */
Signed-off-by: Andrey Danin <danindrey@mail.ru> --- board/compal/dts/tegra20-paz00.dts | 8 ++++++++ include/configs/paz00.h | 8 ++++++++ 2 files changed, 16 insertions(+), 0 deletions(-)