Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Linus Walleij]

On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> No, this is wrong. Platform data should not override DT.
>
> If DT is enabled and passed, it should have highest priority.

Oh is that so.

Rob: do we have a clear consensus on this? Then we should document
it in Documentation/devicetree/usage-model.txt.

(That document isn't part of the binding I believe, so we could define
Linux-specific behaviours in it.)

I always thought it was the other way around, that pdata took priority.

Usecase: hardcoded bootloader passer erroneous DT to a platform.
No way out. What to do? Override with pdata.

Yours,
Linus Walleij

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Lee Jones]

On Mon, Sep 03, 2012 at 01:58:04PM +0200, Linus Walleij wrote:
> On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> 
> > No, this is wrong. Platform data should not override DT.
> >
> > If DT is enabled and passed, it should have highest priority.
> 
> Oh is that so.

:)

> Rob: do we have a clear consensus on this? Then we should document
> it in Documentation/devicetree/usage-model.txt.
> 
> (That document isn't part of the binding I believe, so we could define
> Linux-specific behaviours in it.)
> 
> I always thought it was the other way around, that pdata took priority.
> 
> Usecase: hardcoded bootloader passer erroneous DT to a platform.
> No way out. What to do? Override with pdata.
> 
> Yours,
> Linus Walleij

Hmmm... I see your point, but this won't work.

When booting DT booting take a different path and no platform data
is passed. We can't boot DT AND register devices with platform data
or else we will double probe every device. The only way to pass
pdata when booting with DT is with AUX_DATA() and that's a hack to
get around things we don't have support for yet. Up until now that
has been DMA bindings, clock and pinctrl names and call-backs.

If DT is corrupt or missing the kernel will boot using platform 
data, but np will always be NULL, so we don't have the problem you
were alluding to above.

Let me know if I didn't explain that well enough and I will have
another go.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Linus Walleij]

On Mon, Sep 3, 2012 at 2:34 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> When booting DT booting take a different path and no platform data
> is passed. We can't boot DT AND register devices with platform data
> or else we will double probe every device. The only way to pass
> pdata when booting with DT is with AUX_DATA() and that's a hack to
> get around things we don't have support for yet. Up until now that
> has been DMA bindings, clock and pinctrl names and call-backs.

So if we pass some augmented platform data using AUX_DATA()
that appears as pdata in this case, and gets discarded.

Thus we cannot use AUX_DATA() to override a broken, as in
"the interrupt number is wrong" device tree.

> If DT is corrupt or missing the kernel will boot using platform
> data, but np will always be NULL, so we don't have the problem you
> were alluding to above.

That was not the problem I had in mind.

I had a valid, but incorrect device tree in mind. I.e the device
is there, but with wrong base address, or wrong IRQ number.

If pdata takes precedence, we can use AUX_DATA() to
override such errors from the platform, since drivers/of/platform.c
helpfully pokes in the auxdata as the platform data.
I thought this was one of the reasons why auxdata exist
at all.

Or is the proper solution to runtime-patch the device tree
per se in such cases? How is that actually done then?

Yours,
Linus Walleij

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Lee Jones]

On Mon, Sep 03, 2012 at 03:19:13PM +0200, Linus Walleij wrote:
> On Mon, Sep 3, 2012 at 2:34 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> 
> > When booting DT booting take a different path and no platform data
> > is passed. We can't boot DT AND register devices with platform data
> > or else we will double probe every device. The only way to pass
> > pdata when booting with DT is with AUX_DATA() and that's a hack to
> > get around things we don't have support for yet. Up until now that
> > has been DMA bindings, clock and pinctrl names and call-backs.
> 
> So if we pass some augmented platform data using AUX_DATA()
> that appears as pdata in this case, and gets discarded.
> 
> Thus we cannot use AUX_DATA() to override a broken, as in
> "the interrupt number is wrong" device tree.

No, you cannot to that. You'd have to fix it in DT (which is easier).

> > If DT is corrupt or missing the kernel will boot using platform
> > data, but np will always be NULL, so we don't have the problem you
> > were alluding to above.
> 
> That was not the problem I had in mind.
> 
> I had a valid, but incorrect device tree in mind. I.e the device
> is there, but with wrong base address, or wrong IRQ number.
> 
> If pdata takes precedence, we can use AUX_DATA() to
> override such errors from the platform, since drivers/of/platform.c
> helpfully pokes in the auxdata as the platform data.

Yes, that's what happens.

> I thought this was one of the reasons why auxdata exist
> at all.

I don't think so. I've been told that AUXDATA is just a hack. My
aim is to rid the boardfile of all AUXDATA entries.

> Or is the proper solution to runtime-patch the device tree
> per se in such cases? How is that actually done then?

No, you can't do that either. DT is only read once at boot-time.

The correct solution would be to fix the broken DT.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Stephen Warren]

On 09/03/2012 05:58 AM, Linus Walleij wrote:
> On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones <lee.jones@linaro.org> wrote:
> 
>> No, this is wrong. Platform data should not override DT.
>>
>> If DT is enabled and passed, it should have highest priority.

No, that's wrong. If platform data is specified, it overrides DT, so
that if the DT needs any fixup, it can be provided using platform data.

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Linus Walleij]

On Mon, Sep 3, 2012 at 4:33 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> On 09/03/2012 05:58 AM, Linus Walleij wrote:
>> On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>
>>> No, this is wrong. Platform data should not override DT.
>>>
>>> If DT is enabled and passed, it should have highest priority.
>
> No, that's wrong. If platform data is specified, it overrides DT, so
> that if the DT needs any fixup, it can be provided using platform data.

Thanks Stephen, now there are two of us saying this, Lee please
follow this design pattern.

(Unless Rob/Grant start shouting counter-orders...)

Yours,
Linus Walleij

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Rob Herring]

On 09/03/2012 09:35 AM, Linus Walleij wrote:
> On Mon, Sep 3, 2012 at 4:33 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> On 09/03/2012 05:58 AM, Linus Walleij wrote:
>>> On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>
>>>> No, this is wrong. Platform data should not override DT.
>>>>
>>>> If DT is enabled and passed, it should have highest priority.
>>
>> No, that's wrong. If platform data is specified, it overrides DT, so
>> that if the DT needs any fixup, it can be provided using platform data.
> 
> Thanks Stephen, now there are two of us saying this, Lee please
> follow this design pattern.
> 
> (Unless Rob/Grant start shouting counter-orders...)

Ideally, you only use DT or platform_data and you override DT with a new
DTB. Hopefully we can ultimately remove platform_data or all but parts
that can't be described in DT (i.e. function callouts).

But if you are handling both, then I agree that platform_data should
override DT.

Rob

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Lee Jones]

On Mon, Sep 03, 2012 at 10:09:34AM -0500, Rob Herring wrote:
> On 09/03/2012 09:35 AM, Linus Walleij wrote:
> > On Mon, Sep 3, 2012 at 4:33 PM, Stephen Warren <swarren@nvidia.com> wrote:
> >> On 09/03/2012 05:58 AM, Linus Walleij wrote:
> >>> On Mon, Sep 3, 2012 at 1:32 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >>>
> >>>> No, this is wrong. Platform data should not override DT.
> >>>>
> >>>> If DT is enabled and passed, it should have highest priority.
> >>
> >> No, that's wrong. If platform data is specified, it overrides DT, so
> >> that if the DT needs any fixup, it can be provided using platform data.
> > 
> > Thanks Stephen, now there are two of us saying this, Lee please
> > follow this design pattern.
> > 
> > (Unless Rob/Grant start shouting counter-orders...)
> 
> Ideally, you only use DT or platform_data and you override DT with a new
> DTB. Hopefully we can ultimately remove platform_data or all but parts
> that can't be described in DT (i.e. function callouts).

Exactly. I don't believe that AUX_DATA() should be used as a facility to
override DT settings from platform_data.

> But if you are handling both, then I agree that platform_data should
> override DT.

I do agree with this, but I haven't stumbled over such a use-case yet.
I have only provided; clock names, DMA settings and call-back information
via AUX_DATA() thus far, and those are being removed too when a) the 
correct bindings are mainlined and b) I have the time.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Arnd Bergmann]

On Monday 03 September 2012, Lee Jones wrote:
> > But if you are handling both, then I agree that platform_data should
> > override DT.
> 
> I do agree with this, but I haven't stumbled over such a use-case yet.
> I have only provided; clock names, DMA settings and call-back information
> via AUX_DATA() thus far, and those are being removed too when a) the 
> correct bindings are mainlined and b) I have the time.

I'd prefer if you just disallow the case where pdata and DT have conflicting
information. We don't seem to have a clear rule that is enforced over the
kernel, so I don't think we can rely on either one taking precedence over
the other in general.

In this particular case, we don't have a single board file providing a
struct nmk_i2c_controller definition for platform data, so the best way
to handle this IMHO is to remove the header file with the platform
data definition, and just encode the defaults in the driver.

	Arnd

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Linus Walleij]

On Tue, Sep 4, 2012 at 4:28 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:

> In this particular case, we don't have a single board file providing a
> struct nmk_i2c_controller definition for platform data, so the best way
> to handle this IMHO is to remove the header file with the platform
> data definition, and just encode the defaults in the driver.

Alessandro Rubini is actively working on bridging this (and
other amba_device primecells) to PCI, that is the reason why it
was recently converted to an amba_device. How is he then supposed to
get the proper parameters into the driver? Note that the PCI ID
is no help at all since the parameters depend on what is connected
to the I2C bus, not on what it itself is connected to. Isn't platform data
used in such cases?

Yours,
Linus Walleij

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Alessandro Rubini]

> Alessandro Rubini is actively working on bridging this (and
> other amba_device primecells) to PCI, that is the reason why it
> was recently converted to an amba_device.

Yes, I've been inactive for a while but I'm on it right now.

> How is he then supposed to get the proper parameters into the
> driver? Note that the PCI ID is no help at all since the parameters
> depend on what is connected to the I2C bus, not on what it itself is
> connected to. Isn't platform data used in such cases?

I'm using platform data currently, but Davide Ciminaghi is actively
working to convert the configuration to device-tree: the way we pass
platform data to the pci device (and thus amba) is not considered
acceptable by Peter Anvin.

I'm thus asking Davide if he's happy to remove the platform data
configuration path right now (I personally wouldn't be very happy, but
I acknowledge it should happen, sooner or later).

/alessandro

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Lee Jones]

On Tue, Sep 04, 2012 at 07:27:10PM +0200, Linus Walleij wrote:
> On Tue, Sep 4, 2012 at 4:28 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> 
> > In this particular case, we don't have a single board file providing a
> > struct nmk_i2c_controller definition for platform data, so the best way
> > to handle this IMHO is to remove the header file with the platform
> > data definition, and just encode the defaults in the driver.
> 
> Alessandro Rubini is actively working on bridging this (and
> other amba_device primecells) to PCI, that is the reason why it
> was recently converted to an amba_device. How is he then supposed to
> get the proper parameters into the driver? Note that the PCI ID
> is no help at all since the parameters depend on what is connected
> to the I2C bus, not on what it itself is connected to. Isn't platform data
> used in such cases?

So why can't Alessandro continue to use Platform Data in the normal way?

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/3] i2c: nomadik: Add Device Tree support to the Nomadik I2C driver

[Linus Walleij]

On Wed, Sep 5, 2012 at 8:41 AM, Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Sep 04, 2012 at 07:27:10PM +0200, Linus Walleij wrote:
>> On Tue, Sep 4, 2012 at 4:28 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>>
>> > In this particular case, we don't have a single board file providing a
>> > struct nmk_i2c_controller definition for platform data, so the best way
>> > to handle this IMHO is to remove the header file with the platform
>> > data definition, and just encode the defaults in the driver.
>>
>> Alessandro Rubini is actively working on bridging this (and
>> other amba_device primecells) to PCI, that is the reason why it
>> was recently converted to an amba_device. How is he then supposed to
>> get the proper parameters into the driver? Note that the PCI ID
>> is no help at all since the parameters depend on what is connected
>> to the I2C bus, not on what it itself is connected to. Isn't platform data
>> used in such cases?
>
> So why can't Alessandro continue to use Platform Data in the normal way?

He probably can, this is not an argument about that, what I am worried
about is Arnd's suggestion to delete the platform data header if there
are potential users of it.

Yours,
Linus Walleij