CPU revision IDs

CPU revision IDs

[jonsmirl-Re5JQEeQqe8AvxtiuMwx3w]

How are CPU revision IDs encoded into the device tree? It is not
reasonable to ask people to manually identify their revision and then
use the right device tree. For example a heat sink may obscure the
markings. The CPU ID is needed for device driver quirks.

I was thinking of doing something like adding code at early boot that
fixes up the root compatible string. The revision is a
property of the CPU.

Code changes this:
 compatible = "cubietech,cubietruck", "allwinner,sun7i-a20"
to:
 compatible = "cubietech,cubietruck",  "allwinner,sun7i-a20a",
"allwinner,sun7i-a20"

Alternatively this could be done in uboot.

Browsing around the kernel code there seems to be multiple solutions
to this problem.  For example this Marvell code modifies the device
driver compatible string.

http://lxr.free-electrons.com/source/arch/arm/mach-mvebu/board-v7.c#L71

On Fri, Jun 13, 2014 at 8:09 AM, jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org <jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Jun 13, 2014 at 6:15 AM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> On Fri, Jun 13, 2014 at 11:54:06AM +0200, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 06/13/2014 10:40 AM, Maxime Ripard wrote:
>>> > On Fri, Jun 13, 2014 at 09:32:20AM +0200, Hans de Goede wrote:
>>> >> Hi,
>>> >>
>>> >> On 06/13/2014 12:48 AM, jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>>> >>> On Thu, Jun 12, 2014 at 6:35 PM, Emilio López <emilio@elopez.com.ar> wrote:
>>> >>>> El 12/06/14 19:11, jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org escribió:
>>> >>>>
>>> >>>>> What has replaced sw_get_ic_ver() on 3.15?
>>> >>>>>
>>> >>>>> Codec code varies on every chip revision A,B,C and A10/20.
>>> >>>>
>>> >>>>
>>> >>>> A10/A20 can be determined by the compatible string. Chip revision is going
>>> >>>> to be trickier though, there is no direct replacement of sw_get_ic_ver()
>>> >>>> that I'm aware of. sw_get_ic_ver() seems to poke a timer register in the
>>> >>>> sun4i case, and SID (for which we do have a driver[1]) on the sun5i case.
>>> >>>>
>>> >>>> [1] drivers/misc/eeprom/sunxi_sid.c
>>> >>>
>>> >>> We may have to reimplement it. Codec driver has stuff like this in it.
>>> >>
>>> >> I think adding some sort of SoC version detection makes sense, so go for it.
>>> >
>>> > It might, and we probably will come to it eventually, but I don't get
>>> > what it would bring here.
>>> >
>>> > Have different compatible strings for the various revisions of the IP
>>> > is much simpler and adds no code at all.
>>>
>>> That assumes that for a single board only a single revision of the SoC has
>>> ever been used. I would not be so sure that that is the case, I'm pretty
>>> sure that there were various rruns of the original mk802 A10 version,
>>> likely with the first runs having A10 Revision A and later runs
>>> revision B. I really don't want to have to do different dts files just
>>> to deal with this, that is not helpful from a maintenance pov, and it
>>> will also only serve to confuse our end users as they will have no idea
>>> which revision of the SoC they have, so solving the differences between
>>> the A10 revision A vs B/C with a compatible string seems counter productive.
>>
>> There's usually two patterns to deal with this:
>>   - Either have two different DT, depending on the revision of the
>>     board
>>
>>   - If the board rev hasn't changed, have the machine code come and
>>     update the DT with the appropriate compatible (see
>>     http://lxr.free-electrons.com/source/arch/arm/mach-mvebu/board-v7.c#L71)
>
> This code fixes up the I2C compatible string based on SOC ID. That
> doesn't seem right to me. Instead I would have put some code in early
> boot that fixes up the root compatible string. The revision is a
> property of the CPU.
>
> Change this:
> compatible = "cubietech,cubietruck", "allwinner,sun7i-a20"
> to:
> compatible = "cubietech,cubietruck",  "allwinner,sun7i-a20a",
> "allwinner,sun7i-a20"
>
> Uboot can fix up device trees. Power PC uses it to add memory size and
> CPU clock speed.  Maybe uboot should fix the compatible string?
>
>
>>
>> The latter is much better, for the reasons you mentionned, but in any
>> case, the driver itself shouldn't have to worry about that kind of
>> things, and only deal with compatibles.
>>
>> Max
>>
>> --
>> Maxime Ripard, Free Electrons
>> Embedded Linux, Kernel and Android engineering
>> http://free-electrons.com
>
>
>
> --
> Jon Smirl
> jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: CPU revision IDs

[Thomas Petazzoni]

Hello,

On Fri, 13 Jun 2014 08:29:41 -0400, jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> How are CPU revision IDs encoded into the device tree? It is not
> reasonable to ask people to manually identify their revision and then
> use the right device tree. For example a heat sink may obscure the
> markings. The CPU ID is needed for device driver quirks.
> 
> I was thinking of doing something like adding code at early boot that
> fixes up the root compatible string. The revision is a
> property of the CPU.
> 
> Code changes this:
>  compatible = "cubietech,cubietruck", "allwinner,sun7i-a20"
> to:
>  compatible = "cubietech,cubietruck",  "allwinner,sun7i-a20a",
> "allwinner,sun7i-a20"
> 
> Alternatively this could be done in uboot.
> 
> Browsing around the kernel code there seems to be multiple solutions
> to this problem.  For example this Marvell code modifies the device
> driver compatible string.
> 
> http://lxr.free-electrons.com/source/arch/arm/mach-mvebu/board-v7.c#L71

Yes, the way we handle that on Marvell EBU platforms is that we runtime
detect the revision of the SoC, and use this information to adjust the
Device Tree information for this or that device that is known to have
issues/differences. For example, early revisions of Armada XP
processors have an issue in the I2C controller which prevents an
optimization feature from being used, so the code in board-v7.c detects
this early revision, and changes the Device Tree compatible string used
to probe the I2C controller, to a different compatible string that lets
the driver know that the feature shouldn't be used.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: CPU revision IDs

[Arnd Bergmann]

On Friday 13 June 2014 14:43:55 Thomas Petazzoni wrote:
> On Fri, 13 Jun 2014 08:29:41 -0400, jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> > How are CPU revision IDs encoded into the device tree? It is not
> > reasonable to ask people to manually identify their revision and then
> > use the right device tree. For example a heat sink may obscure the
> > markings. The CPU ID is needed for device driver quirks.
> > 
> > I was thinking of doing something like adding code at early boot that
> > fixes up the root compatible string. The revision is a
> > property of the CPU.
> > 
> > Code changes this:
> >  compatible = "cubietech,cubietruck", "allwinner,sun7i-a20"
> > to:
> >  compatible = "cubietech,cubietruck",  "allwinner,sun7i-a20a",
> > "allwinner,sun7i-a20"
> > 
> > Alternatively this could be done in uboot.
> > 
> > Browsing around the kernel code there seems to be multiple solutions
> > to this problem.  For example this Marvell code modifies the device
> > driver compatible string.
> > 
> > http://lxr.free-electrons.com/source/arch/arm/mach-mvebu/board-v7.c#L71
> 
> Yes, the way we handle that on Marvell EBU platforms is that we runtime
> detect the revision of the SoC, and use this information to adjust the
> Device Tree information for this or that device that is known to have
> issues/differences. For example, early revisions of Armada XP
> processors have an issue in the I2C controller which prevents an
> optimization feature from being used, so the code in board-v7.c detects
> this early revision, and changes the Device Tree compatible string used
> to probe the I2C controller, to a different compatible string that lets
> the driver know that the feature shouldn't be used.

I think the best approach always depends on the specific situation, in
particular what the differences are between revisions and how easy to
detect the revision is.

Ideally all knowledge about different versions of an on-chip device
are kept inside of the driver for that device. E.g. if they fixed a
bug in the audio component, the audio driver should first try to find
out the revision by looking at its own registers. If that doesn't work,
the compatible strings for that device should be different for each
revision and hardcoded in dts sources per board. This was problematic
for the mvebu i2c controller as well, because one board was known to
come in multiple revisions, so a hack was added in the platform code.

Hanging driver specifics off the top-level compatible string however
doesn't feel right. If the boot loader is going to fix it up, it should
patch both the top-level strings and the devices that are actually
different.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: CPU revision IDs

[jonsmirl-Re5JQEeQqe8AvxtiuMwx3w]

On Fri, Jun 13, 2014 at 8:56 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Friday 13 June 2014 14:43:55 Thomas Petazzoni wrote:
>> On Fri, 13 Jun 2014 08:29:41 -0400, jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> > How are CPU revision IDs encoded into the device tree? It is not
>> > reasonable to ask people to manually identify their revision and then
>> > use the right device tree. For example a heat sink may obscure the
>> > markings. The CPU ID is needed for device driver quirks.
>> >
>> > I was thinking of doing something like adding code at early boot that
>> > fixes up the root compatible string. The revision is a
>> > property of the CPU.
>> >
>> > Code changes this:
>> >  compatible = "cubietech,cubietruck", "allwinner,sun7i-a20"
>> > to:
>> >  compatible = "cubietech,cubietruck",  "allwinner,sun7i-a20a",
>> > "allwinner,sun7i-a20"
>> >
>> > Alternatively this could be done in uboot.
>> >
>> > Browsing around the kernel code there seems to be multiple solutions
>> > to this problem.  For example this Marvell code modifies the device
>> > driver compatible string.
>> >
>> > http://lxr.free-electrons.com/source/arch/arm/mach-mvebu/board-v7.c#L71
>>
>> Yes, the way we handle that on Marvell EBU platforms is that we runtime
>> detect the revision of the SoC, and use this information to adjust the
>> Device Tree information for this or that device that is known to have
>> issues/differences. For example, early revisions of Armada XP
>> processors have an issue in the I2C controller which prevents an
>> optimization feature from being used, so the code in board-v7.c detects
>> this early revision, and changes the Device Tree compatible string used
>> to probe the I2C controller, to a different compatible string that lets
>> the driver know that the feature shouldn't be used.
>
> I think the best approach always depends on the specific situation, in
> particular what the differences are between revisions and how easy to
> detect the revision is.
>
> Ideally all knowledge about different versions of an on-chip device
> are kept inside of the driver for that device. E.g. if they fixed a
> bug in the audio component, the audio driver should first try to find
> out the revision by looking at its own registers. If that doesn't work,
> the compatible strings for that device should be different for each
> revision and hardcoded in dts sources per board. This was problematic
> for the mvebu i2c controller as well, because one board was known to
> come in multiple revisions, so a hack was added in the platform code.
>
> Hanging driver specifics off the top-level compatible string however
> doesn't feel right. If the boot loader is going to fix it up, it should
> patch both the top-level strings and the devices that are actually
> different.

I was adding the CPU revision ID to the top level compatible string,
nothing specific to the device.  By knowing the CPU revision I know
which errata to apply.

If you patch the device strings, then you have to maintain knowledge
of which devices are broken in two places -- the device driver and the
machine file where you are patching the strings.

>
>         Arnd



-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: CPU revision IDs

[Arnd Bergmann]

On Friday 13 June 2014 09:05:14 jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> I was adding the CPU revision ID to the top level compatible string,
> nothing specific to the device.  By knowing the CPU revision I know
> which errata to apply.
> 
> If you patch the device strings, then you have to maintain knowledge
> of which devices are broken in two places -- the device driver and the
> machine file where you are patching the strings.

IMHO, the driver only has to know what device versions exist, it
shouldn't need to know which soc has which version of the device.

Can you describe which kind of quirk you are looking at in the driver,
and how common it is?

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: CPU revision IDs

[jonsmirl-Re5JQEeQqe8AvxtiuMwx3w]

On Fri, Jun 13, 2014 at 9:08 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Friday 13 June 2014 09:05:14 jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> I was adding the CPU revision ID to the top level compatible string,
>> nothing specific to the device.  By knowing the CPU revision I know
>> which errata to apply.
>>
>> If you patch the device strings, then you have to maintain knowledge
>> of which devices are broken in two places -- the device driver and the
>> machine file where you are patching the strings.
>
> IMHO, the driver only has to know what device versions exist, it
> shouldn't need to know which soc has which version of the device.
>
> Can you describe which kind of quirk you are looking at in the driver,
> and how common it is?

Allwinner wired the volume control for their on-chip codec up
backwards on the A revsion of the the A10 chip. In later revisions is
it fixed to work in the right direction.

I was wanting to add code like this to the device driver...

if (of_machine_is_compatible("allwinner,sun7i-a20a")) {
      fix bug
}

Another solution would be for me to detect the chip revision in the
init code of the driver and remember it.

-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: CPU revision IDs

[Maxime Ripard]

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

On Fri, Jun 13, 2014 at 09:14:45AM -0400, jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> On Fri, Jun 13, 2014 at 9:08 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> > On Friday 13 June 2014 09:05:14 jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> >> I was adding the CPU revision ID to the top level compatible string,
> >> nothing specific to the device.  By knowing the CPU revision I know
> >> which errata to apply.
> >>
> >> If you patch the device strings, then you have to maintain knowledge
> >> of which devices are broken in two places -- the device driver and the
> >> machine file where you are patching the strings.
> >
> > IMHO, the driver only has to know what device versions exist, it
> > shouldn't need to know which soc has which version of the device.
> >
> > Can you describe which kind of quirk you are looking at in the driver,
> > and how common it is?
> 
> Allwinner wired the volume control for their on-chip codec up
> backwards on the A revsion of the the A10 chip. In later revisions is
> it fixed to work in the right direction.
> 
> I was wanting to add code like this to the device driver...
> 
> if (of_machine_is_compatible("allwinner,sun7i-a20a")) {
>       fix bug
> }
> 
> Another solution would be for me to detect the chip revision in the
> init code of the driver and remember it.

The point is that you don't have that kind of constructs. You can
associate any number of compatibles to your driver, and data to each
of these compatibles. So, with different compatibles, you will be
probed and retrieved these data directly, without even having this in
your driver.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: CPU revision IDs

[jonsmirl-Re5JQEeQqe8AvxtiuMwx3w]

On Fri, Jun 13, 2014 at 9:20 AM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Fri, Jun 13, 2014 at 09:14:45AM -0400, jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> On Fri, Jun 13, 2014 at 9:08 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>> > On Friday 13 June 2014 09:05:14 jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> >> I was adding the CPU revision ID to the top level compatible string,
>> >> nothing specific to the device.  By knowing the CPU revision I know
>> >> which errata to apply.
>> >>
>> >> If you patch the device strings, then you have to maintain knowledge
>> >> of which devices are broken in two places -- the device driver and the
>> >> machine file where you are patching the strings.
>> >
>> > IMHO, the driver only has to know what device versions exist, it
>> > shouldn't need to know which soc has which version of the device.
>> >
>> > Can you describe which kind of quirk you are looking at in the driver,
>> > and how common it is?
>>
>> Allwinner wired the volume control for their on-chip codec up
>> backwards on the A revsion of the the A10 chip. In later revisions is
>> it fixed to work in the right direction.
>>
>> I was wanting to add code like this to the device driver...
>>
>> if (of_machine_is_compatible("allwinner,sun7i-a20a")) {
>>       fix bug
>> }
>>
>> Another solution would be for me to detect the chip revision in the
>> init code of the driver and remember it.
>
> The point is that you don't have that kind of constructs. You can
> associate any number of compatibles to your driver, and data to each
> of these compatibles. So, with different compatibles, you will be
> probed and retrieved these data directly, without even having this in
> your driver.


Then I end up with code in the machine file like this...

find SOC revision
if revision == A
    fix up codec and i2s driver compatible
if revision == B
    fix up HDMI compatible
if revision == C
    fix up whatever
I've seen chips with revision levels of J

Why do you want this knowledge in the machine file? Why not just add
the test for SOC revision inside the device driver?

I do see the logic for wanting to change the compatible string for the
device, but these aren't really different devices. They are errata
being applied to the same device.


>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com



-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: CPU revision IDs

[Maxime Ripard]

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

On Fri, Jun 13, 2014 at 09:34:05AM -0400, jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> On Fri, Jun 13, 2014 at 9:20 AM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > On Fri, Jun 13, 2014 at 09:14:45AM -0400, jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> >> On Fri, Jun 13, 2014 at 9:08 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> >> > On Friday 13 June 2014 09:05:14 jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> >> >> I was adding the CPU revision ID to the top level compatible string,
> >> >> nothing specific to the device.  By knowing the CPU revision I know
> >> >> which errata to apply.
> >> >>
> >> >> If you patch the device strings, then you have to maintain knowledge
> >> >> of which devices are broken in two places -- the device driver and the
> >> >> machine file where you are patching the strings.
> >> >
> >> > IMHO, the driver only has to know what device versions exist, it
> >> > shouldn't need to know which soc has which version of the device.
> >> >
> >> > Can you describe which kind of quirk you are looking at in the driver,
> >> > and how common it is?
> >>
> >> Allwinner wired the volume control for their on-chip codec up
> >> backwards on the A revsion of the the A10 chip. In later revisions is
> >> it fixed to work in the right direction.
> >>
> >> I was wanting to add code like this to the device driver...
> >>
> >> if (of_machine_is_compatible("allwinner,sun7i-a20a")) {
> >>       fix bug
> >> }
> >>
> >> Another solution would be for me to detect the chip revision in the
> >> init code of the driver and remember it.
> >
> > The point is that you don't have that kind of constructs. You can
> > associate any number of compatibles to your driver, and data to each
> > of these compatibles. So, with different compatibles, you will be
> > probed and retrieved these data directly, without even having this in
> > your driver.
> 
> 
> Then I end up with code in the machine file like this...
> 
> find SOC revision
> if revision == A
>     fix up codec and i2s driver compatible
> if revision == B
>     fix up HDMI compatible
> if revision == C
>     fix up whatever
> I've seen chips with revision levels of J
> 
> Why do you want this knowledge in the machine file? Why not just add
> the test for SOC revision inside the device driver?

Because it's going to be a mess anyway. No matter if it's in the
driver or the machine code. So at least, that way, we're having it
centralized and isolated from the driver's code.

Plus, we're going to need these several compatibles anyway, for boards
that come with a single SoC revision, so why not just use them.

And if it's a single revision of a single SoC for the moment, it's
completely fine. When we'll get overwhelmed, if we ever are, we can
always refactor that code to something smarter.

> I do see the logic for wanting to change the compatible string for the
> device, but these aren't really different devices. They are errata
> being applied to the same device.

No, they're not really different, but they're not the same either, or
at least, for whatever reason, they don't behave in the exact same
way.

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: CPU revision IDs

[jonsmirl-Re5JQEeQqe8AvxtiuMwx3w]

On Fri, Jun 13, 2014 at 9:58 AM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Fri, Jun 13, 2014 at 09:34:05AM -0400, jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> On Fri, Jun 13, 2014 at 9:20 AM, Maxime Ripard
>> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> > On Fri, Jun 13, 2014 at 09:14:45AM -0400, jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> >> On Fri, Jun 13, 2014 at 9:08 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>> >> > On Friday 13 June 2014 09:05:14 jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> >> >> I was adding the CPU revision ID to the top level compatible string,
>> >> >> nothing specific to the device.  By knowing the CPU revision I know
>> >> >> which errata to apply.
>> >> >>
>> >> >> If you patch the device strings, then you have to maintain knowledge
>> >> >> of which devices are broken in two places -- the device driver and the
>> >> >> machine file where you are patching the strings.
>> >> >
>> >> > IMHO, the driver only has to know what device versions exist, it
>> >> > shouldn't need to know which soc has which version of the device.
>> >> >
>> >> > Can you describe which kind of quirk you are looking at in the driver,
>> >> > and how common it is?
>> >>
>> >> Allwinner wired the volume control for their on-chip codec up
>> >> backwards on the A revsion of the the A10 chip. In later revisions is
>> >> it fixed to work in the right direction.
>> >>
>> >> I was wanting to add code like this to the device driver...
>> >>
>> >> if (of_machine_is_compatible("allwinner,sun7i-a20a")) {
>> >>       fix bug
>> >> }
>> >>
>> >> Another solution would be for me to detect the chip revision in the
>> >> init code of the driver and remember it.
>> >
>> > The point is that you don't have that kind of constructs. You can
>> > associate any number of compatibles to your driver, and data to each
>> > of these compatibles. So, with different compatibles, you will be
>> > probed and retrieved these data directly, without even having this in
>> > your driver.
>>
>>
>> Then I end up with code in the machine file like this...
>>
>> find SOC revision
>> if revision == A
>>     fix up codec and i2s driver compatible
>> if revision == B
>>     fix up HDMI compatible
>> if revision == C
>>     fix up whatever
>> I've seen chips with revision levels of J
>>
>> Why do you want this knowledge in the machine file? Why not just add
>> the test for SOC revision inside the device driver?
>
> Because it's going to be a mess anyway. No matter if it's in the
> driver or the machine code. So at least, that way, we're having it
> centralized and isolated from the driver's code.

The mess is always going to be in the driver code. You are just
changing the the string the driver code selects on...

if (of_machine_is_compatible("allwinner,sun7i-codec-rev-a")) {
      fix bug
}

if (of_machine_is_compatible("allwinner,sun7i-a20a")) {
      fix bug
}

The mess is not required in the machine file.  Init time code can
always modify the machine compatible string like this with no side
effects. It would add 'a'-'j' as appropriate for the CPU.

Code changes this:
 compatible = "cubietech,cubietruck", "allwinner,sun7i-a20"
to:
 compatible = "cubietech,cubietruck",  "allwinner,sun7i-a20-a",
"allwinner,sun7i-a20"

There is an existing API in the FDT code for changing compatible strings.

>
> Plus, we're going to need these several compatibles anyway, for boards
> that come with a single SoC revision, so why not just use them.
>
> And if it's a single revision of a single SoC for the moment, it's
> completely fine. When we'll get overwhelmed, if we ever are, we can
> always refactor that code to something smarter.
>
>> I do see the logic for wanting to change the compatible string for the
>> device, but these aren't really different devices. They are errata
>> being applied to the same device.
>
> No, they're not really different, but they're not the same either, or
> at least, for whatever reason, they don't behave in the exact same
> way.
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com



-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: CPU revision IDs

[Maxime Ripard]

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

On Fri, Jun 13, 2014 at 10:37:37AM -0400, jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> >> Then I end up with code in the machine file like this...
> >>
> >> find SOC revision
> >> if revision == A
> >>     fix up codec and i2s driver compatible
> >> if revision == B
> >>     fix up HDMI compatible
> >> if revision == C
> >>     fix up whatever
> >> I've seen chips with revision levels of J
> >>
> >> Why do you want this knowledge in the machine file? Why not just add
> >> the test for SOC revision inside the device driver?
> >
> > Because it's going to be a mess anyway. No matter if it's in the
> > driver or the machine code. So at least, that way, we're having it
> > centralized and isolated from the driver's code.
> 
> The mess is always going to be in the driver code. You are just
> changing the the string the driver code selects on...
> 
> if (of_machine_is_compatible("allwinner,sun7i-codec-rev-a")) {
>       fix bug
> }
> 
> if (of_machine_is_compatible("allwinner,sun7i-a20a")) {
>       fix bug
> }

What? Hmmm, nope. Look at how we're doing this for the i2c driver
(drivers/i2c/busses/i2c-mv64xxx.c).

> The mess is not required in the machine file.  Init time code can
> always modify the machine compatible string like this with no side
> effects. It would add 'a'-'j' as appropriate for the CPU.
> 
> Code changes this:
>  compatible = "cubietech,cubietruck", "allwinner,sun7i-a20"
> to:
>  compatible = "cubietech,cubietruck",  "allwinner,sun7i-a20-a",
> "allwinner,sun7i-a20"

Again, this is the machine compatible, and this is orthogonal to the
issue. You should have a different compatible string for the *codec*.

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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