Re: binding for nvec mfd device

Re: binding for nvec mfd device

[Stephen Warren]

On 06/27/2013 12:26 PM, Marc Dietrich wrote:
> Hi,

(CC'ing linux-tegra)

> I'm puzzling for some time now to find a binding for a mfd device. So let's 
> start with something I think I have learned already, let's describe the 
> hardware.
> 
> Here the problem starts, because the nvec (nvidia [protocol] compliant 
> embedded controller) isn't a well defined piece of silicon at all, but more a 
> protocol which specifies how to communicate with certain devices (keyboard, 
> mouse, gpio, system state, battery, oem specific hw ...). Even worse, the 
> number of "attached" devices may vary for each implementation and there is no 
> way to sanely probe which which devices exist or not.
> 
> For example, the "hardware" in my case is an ENE KB926 keyboard controller 
> with an additional ps/2 port (for the touchpad) and a number of gpio's for 
> connecting leds, spi bus, a charger chip and some other stuff. Most gpio's 
> cannot be controlled directly, but only through certain ec commands. The i2c 
> protocol used for communication with the host cpu is handled by a good old 
> 8051 type integrated in the keyboard controller.
> 
> Other vendors may implement this in totally different hardware, keeping only 
> the protocol. So I think this is a case where the "describe the hardware" 
> principle just fails.

I think "describe the HW" should work quite well. However, it's a matter
of interpretation what that means:-) I take it to mean describing the
SW-visible aspects of that HW, which in this case is the protocol used
to talk to that HW.

> So I thought ok - forget this principle for now and describe it from a higher 
> protocol point of view. We currently have something like this:
> 
>         nvec@7000c500 {
>                 compatible = "nvidia,nvec";
>                 reg = <0x7000c500 0x100>;
>                 interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH>;
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>                 clock-frequency = <80000>;
>                 request-gpios = <&gpio TEGRA_GPIO(V, 2) GPIO_ACTIVE_HIGH>;
>                 slave-addr = <138>;
>                 clocks = <&tegra_car TEGRA20_CLK_I2C3>,
>                          <&tegra_car TEGRA20_CLK_PLL_P_OUT3>;
>                 clock-names = "div-clk", "fast-clk";
>         };

I would eventually like to do the following:

* Have a node that represents the I2C slave controller HW module in Tegra.

* Have a node (or nodes) (perhaps a child node of that) which represents
the protocol that "binds" to the slave controller. This would have its
own compatible value that describes which protocol to instantiate.

This will separate the driver for the I2C slave HW from the driver for
the protocol, so that you could even run the same protocol with a
different I2C slave device if needed.

> which describes how the host cpu should setup the i2c communication. The 
> devices are currently hard coded in the driver, but I like to make them more 
> flexible through a device tree binding, eg. something like this:
> 
> 		nvec {
> 				...
> 				<properties from above>
> 				...
> 
> 				keyboard {
> 						<add some key bindings here>
> 				};
> 
> 				ps2 {
> 						packet-size = 6; /* for the ac100 tp */
> 				};
... (more child nodes)
> 		};
> 
> One question is now how to instantiate the mfd children from this. I see three 
> different methods used in different drivers:
> 
> 		a) just add a "compatible =" property to the child nodes and a 
> 			"simple-bus" property to the the driver

(or instead of simple-bus, have the top-level node's probe() do the
following at the end:

        of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);

That's what sounds/soc/tegra/tegra30_ahub.c does.

> 		b) search for sub-nodes and instantiate the children "manually" by
> 			"guessing" some driver name from the node name and use 
> 			platform_add_device
> 		c) use an mfd_cell struct hard coded in the driver and "enable" only
> 			the children found in the sub-node list and use mfd_add_devices.
>
> All these methods above have their advantages and disadvantages. But lets 
> ignore them for now and ask what's the preferred way to do this?

I'm not sure if there is a preferred way as a global rule.

I prefer either:

1) The existing method of entirely hard-coding everything in the
top-level device driver, since it is a driver for the particular HW
setup, it knows exactly what is there, and so it's fine to hard-code
everything. The disadvantage here is that any configuration for all the
child nodes gets merged together into the one top-level node though,
which could get messy.

2) Your option a. I believe if we're going to break down the MFD device
into chunks, the way we do it should mirror existing DT techniques as
closely as possible, and using nodes with compatible values to represent
the child devices seems most consistent with existing techniques.

> Another question is how the children will get their platform data. I guess 
> they will just walk the device tree of their parent node and search for it, 
> but maybe there is a better way?

In option (a), there will be a separate struct device for each child
node, instantiated directly from the child DT node. Hence, the driver
(chosen from the compatible value just like any other) would simply
parse dev.of_node just like any other driver. The only difference would
be that the driver would have to go to the parent node, and search for a
driver for it, and ask that driver for any "supporting resources", such
as a device handle that allowed you to all APIs to send/receive bytes
over the protocol. Perhaps the parent device might just pre-create
regmaps for all the child devices if that abstraction works. That would
save the child devices even having to look for the parent device.

Re: binding for nvec mfd device

[Thierry Reding]

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

On Fri, Jun 28, 2013 at 09:13:38AM -0600, Stephen Warren wrote:
> On 06/27/2013 12:26 PM, Marc Dietrich wrote:
[...]
> > Another question is how the children will get their platform data. I guess 
> > they will just walk the device tree of their parent node and search for it, 
> > but maybe there is a better way?
> 
> In option (a), there will be a separate struct device for each child
> node, instantiated directly from the child DT node. Hence, the driver
> (chosen from the compatible value just like any other) would simply
> parse dev.of_node just like any other driver. The only difference would
> be that the driver would have to go to the parent node, and search for a
> driver for it, and ask that driver for any "supporting resources", such
> as a device handle that allowed you to all APIs to send/receive bytes
> over the protocol. Perhaps the parent device might just pre-create
> regmaps for all the child devices if that abstraction works. That would
> save the child devices even having to look for the parent device.

I've often seen a much easier method employed and something similar is
also done in host1x/tegra-drm. Child devices directly pass dev->parent
to some API provided by the parent. The API then takes care of checking
and extracting the driver-specific data of the parent from the struct
device before performing the operation. Or you could provide an API to
cast from a struct device to the parent's type.

So in the specific case of nvec this would translate to either making
the nvec_write_{sync,async}() functions take a struct device * instead
of the struct nvec_chip as first parameter, then do something like:

	struct nvec_chip *nvec = dev_get_drvdata(dev);

Or provide a function such as this:

	struct nvec_chip *nvec_chip_from_device(struct device *dev)
	{
		/* perhaps do some validation */
		return dev_get_drvdata(dev);
	}

which can be called from child drivers before calling nvec_write_sync()
or nvec_write_async().

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: binding for nvec mfd device

[Marc Dietrich]

On Friday 28 June 2013 09:13:38 Stephen Warren wrote:
> On 06/27/2013 12:26 PM, Marc Dietrich wrote:
> > Hi,
> 
> (CC'ing linux-tegra)
> 
> > I'm puzzling for some time now to find a binding for a mfd device. So
> > let's
> > start with something I think I have learned already, let's describe the
> > hardware.
> > 
> > Here the problem starts, because the nvec (nvidia [protocol] compliant
> > embedded controller) isn't a well defined piece of silicon at all, but
> > more a protocol which specifies how to communicate with certain devices
> > (keyboard, mouse, gpio, system state, battery, oem specific hw ...). Even
> > worse, the number of "attached" devices may vary for each implementation
> > and there is no way to sanely probe which which devices exist or not.
> > 
> > For example, the "hardware" in my case is an ENE KB926 keyboard controller
> > with an additional ps/2 port (for the touchpad) and a number of gpio's for
> > connecting leds, spi bus, a charger chip and some other stuff. Most gpio's
> > cannot be controlled directly, but only through certain ec commands. The
> > i2c protocol used for communication with the host cpu is handled by a
> > good old 8051 type integrated in the keyboard controller.
> > 
> > Other vendors may implement this in totally different hardware, keeping
> > only the protocol. So I think this is a case where the "describe the
> > hardware" principle just fails.
> 
> I think "describe the HW" should work quite well. However, it's a matter
> of interpretation what that means:-) I take it to mean describing the
> SW-visible aspects of that HW, which in this case is the protocol used
> to talk to that HW.
> 
> > So I thought ok - forget this principle for now and describe it from a
> > higher> 
> > protocol point of view. We currently have something like this:
> >         nvec@7000c500 {
> >         
> >                 compatible = "nvidia,nvec";
> >                 reg = <0x7000c500 0x100>;
> >                 interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH>;
> >                 #address-cells = <1>;
> >                 #size-cells = <0>;
> >                 clock-frequency = <80000>;
> >                 request-gpios = <&gpio TEGRA_GPIO(V, 2) GPIO_ACTIVE_HIGH>;
> >                 slave-addr = <138>;
> >                 clocks = <&tegra_car TEGRA20_CLK_I2C3>,
> >                 
> >                          <&tegra_car TEGRA20_CLK_PLL_P_OUT3>;
> >                 
> >                 clock-names = "div-clk", "fast-clk";
> >         
> >         };
> 
> I would eventually like to do the following:
> 
> * Have a node that represents the I2C slave controller HW module in Tegra.
> 
> * Have a node (or nodes) (perhaps a child node of that) which represents
> the protocol that "binds" to the slave controller. This would have its
> own compatible value that describes which protocol to instantiate.
> 
> This will separate the driver for the I2C slave HW from the driver for
> the protocol, so that you could even run the same protocol with a
> different I2C slave device if needed.

I agree with you that the slave driver should be separated from the protocol 
(which itself can also be separated into e.g. smbus and nvec layers). But 
that's more like a long term goal which we cannot handle so quickly. For this 
to work, you probably also need a kernel api for i2c slaves which many people 
failed to introduce in the past already. The NVIDIA downstream kernel has 
something like this already (are there any clients?), but unfortunately no 
smbus protocol support yet.

So keeping this in mind, the binding above already looks ok, even if it is 
named "nvec" instead of e.g. "i2c-slave" for now.

> > which describes how the host cpu should setup the i2c communication. The
> > devices are currently hard coded in the driver, but I like to make them
> > more> 
> > flexible through a device tree binding, eg. something like this:
> > 		nvec {
> > 		
> > 				...
> > 				<properties from above>
> > 				...
> > 				
> > 				keyboard {
> > 				
> > 						<add some key bindings here>
> > 				
> > 				};
> > 				
> > 				ps2 {
> > 				
> > 						packet-size = 6; /* for the ac100 tp */
> > 				
> > 				};
> 
> ... (more child nodes)
> 
> > 		};
> > 
> > One question is now how to instantiate the mfd children from this. I see
> > three> 
> > different methods used in different drivers:
> > 		a) just add a "compatible =" property to the child nodes and a
> > 		
> > 			"simple-bus" property to the the driver
> 
> (or instead of simple-bus, have the top-level node's probe() do the
> following at the end:
> 
>         of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
> 
> That's what sounds/soc/tegra/tegra30_ahub.c does.

"simple-bus" and maybe also of_platform_populate are only favored if there is 
a real bus, AFAIU, which isn't the here (and maybe also not for other mfds).

> > 		b) search for sub-nodes and instantiate the children "manually" by
> > 		
> > 			"guessing" some driver name from the node name and use
> > 			platform_add_device
> > 		
> > 		c) use an mfd_cell struct hard coded in the driver and "enable" 
only
> > 		
> > 			the children found in the sub-node list and use 
mfd_add_devices.
> > 
> > All these methods above have their advantages and disadvantages. But lets
> > ignore them for now and ask what's the preferred way to do this?
> 
> I'm not sure if there is a preferred way as a global rule.
> 
> I prefer either:
> 
> 1) The existing method of entirely hard-coding everything in the
> top-level device driver, since it is a driver for the particular HW
> setup, it knows exactly what is there, and so it's fine to hard-code
> everything. The disadvantage here is that any configuration for all the
> child nodes gets merged together into the one top-level node though,
> which could get messy.

also this method is hard to expand to other devices which may only implement a 
subset of the functions (or needs different platform data).

> 2) Your option a. I believe if we're going to break down the MFD device
> into chunks, the way we do it should mirror existing DT techniques as
> closely as possible, and using nodes with compatible values to represent
> the child devices seems most consistent with existing techniques.

Thinking about what you said at the beginning, I think it's best not to use 
any sub-nodes at all, but give all children a node of their own (standalone 
driver) with compatible properties and a link property to the device 
responsible for the communications (nvec or i2c-slave in our case), similar to 
the backlight property for panels:

root {
....
	nvec: nvec {
		compatible = "nvidia,nvec";
		...
	};

	keyboard {
		compatible = "nvidia,nvec-keyboard";
		nvec = <&nvec>;
	};
	....
};

I'm not sure if we can still use the function exports from the nvec or if we 
need to add some "ops" to the nvec struct. Maybe the latter would be the 
better solution. I'm also wondering if we can guaranty that the nvec is loaded 
before any children and that the children are unloaded before the nvec core 
driver.

With the above method, we should have no problems to parse the platform data, 
as each "child" needs to provide its own dt parser function.

Marc


> > Another question is how the children will get their platform data. I guess
> > they will just walk the device tree of their parent node and search for
> > it,
> > but maybe there is a better way?
> 
> In option (a), there will be a separate struct device for each child
> node, instantiated directly from the child DT node. Hence, the driver
> (chosen from the compatible value just like any other) would simply
> parse dev.of_node just like any other driver. The only difference would
> be that the driver would have to go to the parent node, and search for a
> driver for it, and ask that driver for any "supporting resources", such
> as a device handle that allowed you to all APIs to send/receive bytes
> over the protocol. Perhaps the parent device might just pre-create
> regmaps for all the child devices if that abstraction works. That would
> save the child devices even having to look for the parent 
> device.

Re: binding for nvec mfd device

[Stephen Warren]

On 06/29/2013 10:22 AM, Marc Dietrich wrote:
> On Friday 28 June 2013 09:13:38 Stephen Warren wrote:
>> On 06/27/2013 12:26 PM, Marc Dietrich wrote:
...
>>> I'm puzzling for some time now to find a binding for a mfd device. So
>>> let's
>>> start with something I think I have learned already, let's describe the
>>> hardware.
...
>> I would eventually like to do the following:
>>
>> * Have a node that represents the I2C slave controller HW module in Tegra.
>>
>> * Have a node (or nodes) (perhaps a child node of that) which represents
>> the protocol that "binds" to the slave controller. This would have its
>> own compatible value that describes which protocol to instantiate.
>>
>> This will separate the driver for the I2C slave HW from the driver for
>> the protocol, so that you could even run the same protocol with a
>> different I2C slave device if needed.
> 
> I agree with you that the slave driver should be separated from the protocol 
> (which itself can also be separated into e.g. smbus and nvec layers). But 
> that's more like a long term goal which we cannot handle so quickly. For this 
> to work, you probably also need a kernel api for i2c slaves which many people 
> failed to introduce in the past already. The NVIDIA downstream kernel has 
> something like this already (are there any clients?), but unfortunately no 
> smbus protocol support yet.
> 
> So keeping this in mind, the binding above already looks ok, even if it is 
> named "nvec" instead of e.g. "i2c-slave" for now.

Well, the DT binding is supposed to be describing the HW (or at least
the SW-visible aspects of the HW), so we should not be designing the
binding to suit a particular SW/driver structure. Instead, the binding
should describe the HW, and the SW should be written to work with the
binding.

Put another way, the binding is something that shouldn't change even if
the SW/driver structure does.

So, if we're coming up with a final binding to correctly represent nvec,
we should split the I2C slave, protocol, protocol clients all out, since
that's the best representation of the HW and protocol.

...
>>> One question is now how to instantiate the mfd children from this. I see
>>> three> 
>>> different methods used in different drivers:
>>> 		a) just add a "compatible =" property to the child nodes and a
>>> 		
>>> 			"simple-bus" property to the the driver
>>
>> (or instead of simple-bus, have the top-level node's probe() do the
>> following at the end:
>>
>>         of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
>>
>> That's what sounds/soc/tegra/tegra30_ahub.c does.
> 
> "simple-bus" and maybe also of_platform_populate are only favored if there is 
> a real bus, AFAIU, which isn't the here (and maybe also not for other mfds).

I don't think that restriction is generally true. I've certainly seen
seasoned DT experts state that *not* breaking things up into individual
components has almost always turned out to be a mistake.

Besides, the nvec protocol really is a bus, even a physical one in some
ways. It multiplexes access to a variety of endpoints (keyboard, GPIOs)
over a communication channel (nvec protocol, over a physical I2C channel).

>> I prefer either:
...
>> 2) Your option a. I believe if we're going to break down the MFD device
>> into chunks, the way we do it should mirror existing DT techniques as
>> closely as possible, and using nodes with compatible values to represent
>> the child devices seems most consistent with existing techniques.
> 
> Thinking about what you said at the beginning, I think it's best not to use 
> any sub-nodes at all, but give all children a node of their own (standalone 
> driver) with compatible properties and a link property to the device 
> responsible for the communications (nvec or i2c-slave in our case), similar to 
> the backlight property for panels:
> 
> root {
> ....
> 	nvec: nvec {
> 		compatible = "nvidia,nvec";
> 		...
> 	};
> 
> 	keyboard {
> 		compatible = "nvidia,nvec-keyboard";
> 		nvec = <&nvec>;
> 	};
> 	....
> };

So here, the keyboard node is using nvec as the communication transport
for all its data. That *is* a bus, and devices on buses are typically
child nodes of the bus itself; take a look at the I2C or SPI master
bindings. The only exception to this I can think off quickly is the I2C
mux bindings, and that's because the control of the muxing isn't
necessarily part of the data path being muxed.

> I'm not sure if we can still use the function exports from the nvec or if we 
> need to add some "ops" to the nvec struct. Maybe the latter would be the 
> better solution. I'm also wondering if we can guaranty that the nvec is loaded 
> before any children and that the children are unloaded before the nvec core 
> driver.

The driver for the keyboard node above would have to convert the phandle
in the nvec property to some kind of nvec device/driver handle. That
process would need to defer the probe of the keyboard node's
driver/device if the nvec node's driver/device was not yet available.

However, this would be entirely automatic if nvec was a bus, and the
devices behind the bus were children of the nvec node.