Re: [U-Boot] [Ac100] [PATCH 3/3] ARM: tegra: paz00: enable nveckeyboardsupport
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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [Ac100] [PATCH 3/3] ARM: tegra: paz00: enable nveckeyboardsupport
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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [Ac100] [PATCH 3/3] ARM: tegra: paz00: enable nveckeyboardsupport
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?
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [Ac100] [PATCH 3/3] ARM: tegra: paz00: enable nveckeyboardsupport
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 = <8>;
> > 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 devicet...@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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
