Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1560237 > unrolled thread

Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2017-01-17 04:40 +0100
    Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Rob Herring <robh@kernel.org> - 2017-01-18 23:30 +0100
      Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2017-01-19 00:50 +0100
        Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Rob Herring <robh@kernel.org> - 2017-01-19 04:00 +0100
          Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-01-19 09:20 +0100
            Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Rob Herring <robh@kernel.org> - 2017-01-19 15:20 +0100
              Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Guochun Mao <guochun.mao@mediatek.com> - 2017-01-22 03:40 +0100
                Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node John Crispin <john@phrozen.org> - 2017-01-24 11:40 +0100
                Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-01-24 11:40 +0100
      Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-01-19 09:00 +0100

#1560237 — Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node

Hello,

(Side note: you guys should learn about stripping irrelevant parts of
an e-mail when replying!)

On Mon, 16 Jan 2017 09:40:32 +0100, Boris Brezillon wrote:

> > Well this is OK I guess, but then you can also use "mediatek,mt8173-nor"
> > as the oldest supported compatible and be done with it, no ? It looks a
> > bit crappy though, I admit that ...
> 
> Let's stop bikeshedding and wait for DT maintainers feedback
> before taking a decision ;-).
> 
> Rob, Mark, any opinion?

I agree that a clarification would be good. There are really two
options:

 1. Have two compatible strings in the DT, the one that matches the
    exact SoC where the IP is found (first compatible string) and the
    one that matches some other SoC where the same IP is found (second
    compatible string). Originally, Linux only supports the second
    compatible string in its device driver, but if it happens that a
    difference is found between two IPs that we thought were the same,
    we can add support for the first compatible string in the driver,
    with a slightly different behavior.

 2. Have a single compatible string in the DT, matching the exact SoC
    where the IP is found. This involves adding immediately this
    compatible string in the corresponding driver.

I've not really been able to figure out which of the two options is the
most future-proof/appropriate.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [next] | [standalone]

#1562228

On Tue, Jan 17, 2017 at 02:36:50PM +1100, Thomas Petazzoni wrote:
> Hello,
> 
> (Side note: you guys should learn about stripping irrelevant parts of
> an e-mail when replying!)
>
> On Mon, 16 Jan 2017 09:40:32 +0100, Boris Brezillon wrote:
> 
> > > Well this is OK I guess, but then you can also use "mediatek,mt8173-nor"
> > > as the oldest supported compatible and be done with it, no ? It looks a
> > > bit crappy though, I admit that ...
> > 
> > Let's stop bikeshedding and wait for DT maintainers feedback
> > before taking a decision ;-).
> > 
> > Rob, Mark, any opinion?
>

Sigh, is how to do compatibles really not yet understood?
 
> I agree that a clarification would be good. There are really two
> options:
> 
>  1. Have two compatible strings in the DT, the one that matches the
>     exact SoC where the IP is found (first compatible string) and the
>     one that matches some other SoC where the same IP is found (second
>     compatible string). Originally, Linux only supports the second
>     compatible string in its device driver, but if it happens that a
>     difference is found between two IPs that we thought were the same,
>     we can add support for the first compatible string in the driver,
>     with a slightly different behavior.

This. And no wildcards in the compatible string. 

>  2. Have a single compatible string in the DT, matching the exact SoC
>     where the IP is found. This involves adding immediately this
>     compatible string in the corresponding driver.

I wouldn't object to this from a DT perspective as I have no clue 
generally if IP blocks are "the same" or not. Subsystem maintainers will 
object though.

> I've not really been able to figure out which of the two options is the
> most future-proof/appropriate.

They are both future-proof. #2 has the disadvantage of requiring a 
kernel update for a new SoC. 

Rob

[toc] | [prev] | [next] | [standalone]

#1562302

Hello,

On Wed, 18 Jan 2017 16:20:10 -0600, Rob Herring wrote:

> > > Rob, Mark, any opinion?  
> >  
> 
> Sigh, is how to do compatibles really not yet understood?

Well, it seems like not everyone necessarily understands what is the
best strategy to adopt (me included).

> > I agree that a clarification would be good. There are really two
> > options:
> > 
> >  1. Have two compatible strings in the DT, the one that matches the
> >     exact SoC where the IP is found (first compatible string) and the
> >     one that matches some other SoC where the same IP is found (second
> >     compatible string). Originally, Linux only supports the second
> >     compatible string in its device driver, but if it happens that a
> >     difference is found between two IPs that we thought were the same,
> >     we can add support for the first compatible string in the driver,
> >     with a slightly different behavior.  
> 
> This. And no wildcards in the compatible string. 

OK. So it means that today we do something like:

	compatible = "baz,foo-12", "baz,foo-00";

and support only baz,foo-00 in the driver. If tomorrow we discover
that there is in fact a difference between the two IP blocks, we can
add support for baz,foo-12 in the driver, and handle the differences.

But then, the DT still contains:

	compatible = "baz,foo-12", "baz,foo-00";

and therefore pretends that the IP block is compatible with
"baz,foo-00" which is in fact *not* the case. It was a mistake to
consider it as compatible. So we keep living with a DT that has
incorrect information.

> 
> >  2. Have a single compatible string in the DT, matching the exact SoC
> >     where the IP is found. This involves adding immediately this
> >     compatible string in the corresponding driver.  
> 
> I wouldn't object to this from a DT perspective as I have no clue 
> generally if IP blocks are "the same" or not. Subsystem maintainers will 
> object though.

Knowing if IP blocks are "the same" is in fact not necessarily trivial.
What appears to be identical IP blocks today might be discovered later
as actually having subtle differences (sometimes not even visible in
the datasheet).

> > I've not really been able to figure out which of the two options is the
> > most future-proof/appropriate.  
> 
> They are both future-proof. #2 has the disadvantage of requiring a 
> kernel update for a new SoC. 

Which is generally anyway needed because a new SoC will almost always
require some new drivers, adjusting pin-muxing or clock drivers, etc.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1562393

On Wed, Jan 18, 2017 at 5:38 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Hello,
>
> On Wed, 18 Jan 2017 16:20:10 -0600, Rob Herring wrote:
>
>> > > Rob, Mark, any opinion?
>> >
>>
>> Sigh, is how to do compatibles really not yet understood?
>
> Well, it seems like not everyone necessarily understands what is the
> best strategy to adopt (me included).
>
>> > I agree that a clarification would be good. There are really two
>> > options:
>> >
>> >  1. Have two compatible strings in the DT, the one that matches the
>> >     exact SoC where the IP is found (first compatible string) and the
>> >     one that matches some other SoC where the same IP is found (second
>> >     compatible string). Originally, Linux only supports the second
>> >     compatible string in its device driver, but if it happens that a
>> >     difference is found between two IPs that we thought were the same,
>> >     we can add support for the first compatible string in the driver,
>> >     with a slightly different behavior.
>>
>> This. And no wildcards in the compatible string.
>
> OK. So it means that today we do something like:
>
>         compatible = "baz,foo-12", "baz,foo-00";
>
> and support only baz,foo-00 in the driver. If tomorrow we discover
> that there is in fact a difference between the two IP blocks, we can
> add support for baz,foo-12 in the driver, and handle the differences.
>
> But then, the DT still contains:
>
>         compatible = "baz,foo-12", "baz,foo-00";
>
> and therefore pretends that the IP block is compatible with
> "baz,foo-00" which is in fact *not* the case. It was a mistake to
> consider it as compatible. So we keep living with a DT that has
> incorrect information.

I wouldn't say it's a mistake necessarily. The old compatible would
probably work to some extent. I'd assume it was tested to some level.
Or it could be other changes exposing a difference.

>> >  2. Have a single compatible string in the DT, matching the exact SoC
>> >     where the IP is found. This involves adding immediately this
>> >     compatible string in the corresponding driver.
>>
>> I wouldn't object to this from a DT perspective as I have no clue
>> generally if IP blocks are "the same" or not. Subsystem maintainers will
>> object though.
>
> Knowing if IP blocks are "the same" is in fact not necessarily trivial.
> What appears to be identical IP blocks today might be discovered later
> as actually having subtle differences (sometimes not even visible in
> the datasheet).

Yes, I know. That's exactly when you should have multiple compatibles.
Trying to guarantee things are the same is just going to get you in
trouble. You only need to figure out if blocks are obviously different
and only drop the old compatible in that case.

>> > I've not really been able to figure out which of the two options is the
>> > most future-proof/appropriate.
>>
>> They are both future-proof. #2 has the disadvantage of requiring a
>> kernel update for a new SoC.
>
> Which is generally anyway needed because a new SoC will almost always
> require some new drivers, adjusting pin-muxing or clock drivers, etc.

Yes, but you don't want to have to update every single driver.

Rob

[toc] | [prev] | [next] | [standalone]

#1562459

Hi Rob,

On Wed, 18 Jan 2017 20:51:08 -0600
Rob Herring <robh@kernel.org> wrote:

> On Wed, Jan 18, 2017 at 5:38 PM, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
> > Hello,
> >
> > On Wed, 18 Jan 2017 16:20:10 -0600, Rob Herring wrote:
> >  
> >> > > Rob, Mark, any opinion?  
> >> >  
> >>
> >> Sigh, is how to do compatibles really not yet understood?  
> >
> > Well, it seems like not everyone necessarily understands what is the
> > best strategy to adopt (me included).
> >  
> >> > I agree that a clarification would be good. There are really two
> >> > options:
> >> >
> >> >  1. Have two compatible strings in the DT, the one that matches the
> >> >     exact SoC where the IP is found (first compatible string) and the
> >> >     one that matches some other SoC where the same IP is found (second
> >> >     compatible string). Originally, Linux only supports the second
> >> >     compatible string in its device driver, but if it happens that a
> >> >     difference is found between two IPs that we thought were the same,
> >> >     we can add support for the first compatible string in the driver,
> >> >     with a slightly different behavior.  
> >>
> >> This. And no wildcards in the compatible string.  
> >
> > OK. So it means that today we do something like:
> >
> >         compatible = "baz,foo-12", "baz,foo-00";
> >
> > and support only baz,foo-00 in the driver. If tomorrow we discover
> > that there is in fact a difference between the two IP blocks, we can
> > add support for baz,foo-12 in the driver, and handle the differences.
> >
> > But then, the DT still contains:
> >
> >         compatible = "baz,foo-12", "baz,foo-00";
> >
> > and therefore pretends that the IP block is compatible with
> > "baz,foo-00" which is in fact *not* the case. It was a mistake to
> > consider it as compatible. So we keep living with a DT that has
> > incorrect information.  
> 
> I wouldn't say it's a mistake necessarily. The old compatible would
> probably work to some extent. I'd assume it was tested to some level.
> Or it could be other changes exposing a difference.

One last question and I'm done: is something like that acceptable?

	compatible = "<vendor>,<old-soc>","<vendor>,<new-soc>";

This can happen when someone adds support for an unsupported feature
on a brand new SoC, and then someone else use the same driver for an
older SoC embedding the same IP but still wants to add a new compatible
just in case these 2 IPs appear to be slightly different.

Here the order of compat strings is no longer following a clear rule
like 'most-specific compatible first' or 'newest IP/SoC version first',
it's completely dependent on the order these IPs were supported in the
OS (Linux). I'm perfectly fine with that BTW, just want to make sure
this is authorized.

Regards,

Boris

[toc] | [prev] | [next] | [standalone]

#1562764

On Thu, Jan 19, 2017 at 2:14 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Hi Rob,
>
> On Wed, 18 Jan 2017 20:51:08 -0600
> Rob Herring <robh@kernel.org> wrote:
>
>> On Wed, Jan 18, 2017 at 5:38 PM, Thomas Petazzoni
>> <thomas.petazzoni@free-electrons.com> wrote:
>> > Hello,
>> >
>> > On Wed, 18 Jan 2017 16:20:10 -0600, Rob Herring wrote:
>> >
>> >> > > Rob, Mark, any opinion?
>> >> >
>> >>
>> >> Sigh, is how to do compatibles really not yet understood?
>> >
>> > Well, it seems like not everyone necessarily understands what is the
>> > best strategy to adopt (me included).
>> >
>> >> > I agree that a clarification would be good. There are really two
>> >> > options:
>> >> >
>> >> >  1. Have two compatible strings in the DT, the one that matches the
>> >> >     exact SoC where the IP is found (first compatible string) and the
>> >> >     one that matches some other SoC where the same IP is found (second
>> >> >     compatible string). Originally, Linux only supports the second
>> >> >     compatible string in its device driver, but if it happens that a
>> >> >     difference is found between two IPs that we thought were the same,
>> >> >     we can add support for the first compatible string in the driver,
>> >> >     with a slightly different behavior.
>> >>
>> >> This. And no wildcards in the compatible string.
>> >
>> > OK. So it means that today we do something like:
>> >
>> >         compatible = "baz,foo-12", "baz,foo-00";
>> >
>> > and support only baz,foo-00 in the driver. If tomorrow we discover
>> > that there is in fact a difference between the two IP blocks, we can
>> > add support for baz,foo-12 in the driver, and handle the differences.
>> >
>> > But then, the DT still contains:
>> >
>> >         compatible = "baz,foo-12", "baz,foo-00";
>> >
>> > and therefore pretends that the IP block is compatible with
>> > "baz,foo-00" which is in fact *not* the case. It was a mistake to
>> > consider it as compatible. So we keep living with a DT that has
>> > incorrect information.
>>
>> I wouldn't say it's a mistake necessarily. The old compatible would
>> probably work to some extent. I'd assume it was tested to some level.
>> Or it could be other changes exposing a difference.
>
> One last question and I'm done: is something like that acceptable?
>
>         compatible = "<vendor>,<old-soc>","<vendor>,<new-soc>";
>
> This can happen when someone adds support for an unsupported feature
> on a brand new SoC, and then someone else use the same driver for an
> older SoC embedding the same IP but still wants to add a new compatible
> just in case these 2 IPs appear to be slightly different.

Yes, it's old and new compatible strings in this case and it's newest
compatible string first.

> Here the order of compat strings is no longer following a clear rule
> like 'most-specific compatible first' or 'newest IP/SoC version first',
> it's completely dependent on the order these IPs were supported in the
> OS (Linux). I'm perfectly fine with that BTW, just want to make sure
> this is authorized.

I guess we should say "newest compatible for IP first" instead. There
are some exceptions where we add fallbacks later on, but that falls
under the most-specific part.

It's order that the bindings are defined, not Linux support really,
but in practice those are the same.

Rob

[toc] | [prev] | [next] | [standalone]

#1564281

Hi,
On Thu, 2017-01-19 at 08:18 -0600, Rob Herring wrote:
> On Thu, Jan 19, 2017 at 2:14 AM, Boris Brezillon
> > One last question and I'm done: is something like that acceptable?
> >
> >         compatible = "<vendor>,<old-soc>","<vendor>,<new-soc>";
> >
> > This can happen when someone adds support for an unsupported feature
> > on a brand new SoC, and then someone else use the same driver for an
> > older SoC embedding the same IP but still wants to add a new compatible
> > just in case these 2 IPs appear to be slightly different.
> 
> Yes, it's old and new compatible strings in this case and it's newest
> compatible string first.
> 
> > Here the order of compat strings is no longer following a clear rule
> > like 'most-specific compatible first' or 'newest IP/SoC version first',
> > it's completely dependent on the order these IPs were supported in the
> > OS (Linux). I'm perfectly fine with that BTW, just want to make sure
> > this is authorized.
> 
> I guess we should say "newest compatible for IP first" instead. There
> are some exceptions where we add fallbacks later on, but that falls
> under the most-specific part.
> 
> It's order that the bindings are defined, not Linux support really,
> but in practice those are the same.
> 
> Rob

Thanks for all your effort for code reviewing.
Our mt2701-nor's hardware is designed base on mt8713-nor,
even so, there would be some slight difference.
If I don't misunderstand your viewpoint in this discussion,
there's no need to drop mt2701-nor compatible.
And if not, is there any other suggestion?

Thanks.

[toc] | [prev] | [next] | [standalone]

#1565778

On 24/01/2017 11:31, Boris Brezillon wrote:
> On Sun, 22 Jan 2017 10:36:40 +0800
> Guochun Mao <guochun.mao@mediatek.com> wrote:
> 
>> Hi,
>> On Thu, 2017-01-19 at 08:18 -0600, Rob Herring wrote:
>>> On Thu, Jan 19, 2017 at 2:14 AM, Boris Brezillon  
>>>> One last question and I'm done: is something like that acceptable?
>>>>
>>>>         compatible = "<vendor>,<old-soc>","<vendor>,<new-soc>";
>>>>
>>>> This can happen when someone adds support for an unsupported feature
>>>> on a brand new SoC, and then someone else use the same driver for an
>>>> older SoC embedding the same IP but still wants to add a new compatible
>>>> just in case these 2 IPs appear to be slightly different.  
>>>
>>> Yes, it's old and new compatible strings in this case and it's newest
>>> compatible string first.
>>>   
>>>> Here the order of compat strings is no longer following a clear rule
>>>> like 'most-specific compatible first' or 'newest IP/SoC version first',
>>>> it's completely dependent on the order these IPs were supported in the
>>>> OS (Linux). I'm perfectly fine with that BTW, just want to make sure
>>>> this is authorized.  
>>>
>>> I guess we should say "newest compatible for IP first" instead. There
>>> are some exceptions where we add fallbacks later on, but that falls
>>> under the most-specific part.
>>>
>>> It's order that the bindings are defined, not Linux support really,
>>> but in practice those are the same.
>>>
>>> Rob  
>>
>> Thanks for all your effort for code reviewing.
>> Our mt2701-nor's hardware is designed base on mt8713-nor,
>> even so, there would be some slight difference.
>> If I don't misunderstand your viewpoint in this discussion,
>> there's no need to drop mt2701-nor compatible.
> 
> No, just update the documentation as suggested by Rob.
> 
>> And if not, is there any other suggestion?
> 
> Nope, and my apologies for being so insistent on something I obviously
> misunderstood.
> 
> Regards,
> 
> Boris


Hi,

could you please add the mt7623 compat string to the documentation
aswell while at it ? mt7623/mt2701 are essentially the same and it'll
safe us time and effort and not cause merge order conflicts. otherwise i
would need to wait till the mt2701 patch is merged before i can send the
mt7623 one

Thanks !
	John

[toc] | [prev] | [next] | [standalone]

#1565780

On Sun, 22 Jan 2017 10:36:40 +0800
Guochun Mao <guochun.mao@mediatek.com> wrote:

> Hi,
> On Thu, 2017-01-19 at 08:18 -0600, Rob Herring wrote:
> > On Thu, Jan 19, 2017 at 2:14 AM, Boris Brezillon  
> > > One last question and I'm done: is something like that acceptable?
> > >
> > >         compatible = "<vendor>,<old-soc>","<vendor>,<new-soc>";
> > >
> > > This can happen when someone adds support for an unsupported feature
> > > on a brand new SoC, and then someone else use the same driver for an
> > > older SoC embedding the same IP but still wants to add a new compatible
> > > just in case these 2 IPs appear to be slightly different.  
> > 
> > Yes, it's old and new compatible strings in this case and it's newest
> > compatible string first.
> >   
> > > Here the order of compat strings is no longer following a clear rule
> > > like 'most-specific compatible first' or 'newest IP/SoC version first',
> > > it's completely dependent on the order these IPs were supported in the
> > > OS (Linux). I'm perfectly fine with that BTW, just want to make sure
> > > this is authorized.  
> > 
> > I guess we should say "newest compatible for IP first" instead. There
> > are some exceptions where we add fallbacks later on, but that falls
> > under the most-specific part.
> > 
> > It's order that the bindings are defined, not Linux support really,
> > but in practice those are the same.
> > 
> > Rob  
> 
> Thanks for all your effort for code reviewing.
> Our mt2701-nor's hardware is designed base on mt8713-nor,
> even so, there would be some slight difference.
> If I don't misunderstand your viewpoint in this discussion,
> there's no need to drop mt2701-nor compatible.

No, just update the documentation as suggested by Rob.

> And if not, is there any other suggestion?

Nope, and my apologies for being so insistent on something I obviously
misunderstood.

Regards,

Boris

[toc] | [prev] | [next] | [standalone]

#1562449

On Wed, 18 Jan 2017 16:20:10 -0600
Rob Herring <robh@kernel.org> wrote:

> On Tue, Jan 17, 2017 at 02:36:50PM +1100, Thomas Petazzoni wrote:
> > Hello,
> > 
> > (Side note: you guys should learn about stripping irrelevant parts of
> > an e-mail when replying!)
> >
> > On Mon, 16 Jan 2017 09:40:32 +0100, Boris Brezillon wrote:
> >   
> > > > Well this is OK I guess, but then you can also use "mediatek,mt8173-nor"
> > > > as the oldest supported compatible and be done with it, no ? It looks a
> > > > bit crappy though, I admit that ...  
> > > 
> > > Let's stop bikeshedding and wait for DT maintainers feedback
> > > before taking a decision ;-).
> > > 
> > > Rob, Mark, any opinion?  
> >  
> 
> Sigh, is how to do compatibles really not yet understood?

Apparently not, and I fear this is not the last misunderstanding on my
side ;-).

>  
> > I agree that a clarification would be good. There are really two
> > options:
> > 
> >  1. Have two compatible strings in the DT, the one that matches the
> >     exact SoC where the IP is found (first compatible string) and the
> >     one that matches some other SoC where the same IP is found (second
> >     compatible string). Originally, Linux only supports the second
> >     compatible string in its device driver, but if it happens that a
> >     difference is found between two IPs that we thought were the same,
> >     we can add support for the first compatible string in the driver,
> >     with a slightly different behavior.  
> 
> This. And no wildcards in the compatible string. 
> 
> >  2. Have a single compatible string in the DT, matching the exact SoC
> >     where the IP is found. This involves adding immediately this
> >     compatible string in the corresponding driver.  
> 
> I wouldn't object to this from a DT perspective as I have no clue 
> generally if IP blocks are "the same" or not. Subsystem maintainers will 
> object though.
> 
> > I've not really been able to figure out which of the two options is the
> > most future-proof/appropriate.  
> 
> They are both future-proof. #2 has the disadvantage of requiring a 
> kernel update for a new SoC. 
> 
> Rob

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web