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

Groups > linux.kernel > #1450980 > unrolled thread

Re: [PATCH v11 4/5] powerpc/fsl: move mpc85xx.h to include/linux/fsl

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 v11 4/5] powerpc/fsl: move mpc85xx.h to include/linux/fsl Scott Wood <oss@buserror.net> - 2016-07-27 02:40 +0200
    RE: [PATCH v11 4/5] powerpc/fsl: move mpc85xx.h to include/linux/fsl Yangbo Lu <yangbo.lu@nxp.com> - 2016-08-02 08:00 +0200
      Re: [PATCH v11 4/5] powerpc/fsl: move mpc85xx.h to include/linux/fsl Scott Wood <oss@buserror.net> - 2016-08-03 00:10 +0200
        RE: [PATCH v11 4/5] powerpc/fsl: move mpc85xx.h to include/linux/fsl Yangbo Lu <yangbo.lu@nxp.com> - 2016-08-03 05:40 +0200

#1450980 — Re: [PATCH v11 4/5] powerpc/fsl: move mpc85xx.h to include/linux/fsl

On Mon, 2016-07-25 at 06:12 +0000, Yangbo Lu wrote:
> Hi Scott,
> 
> 
> > 
> > -----Original Message-----
> > From: Scott Wood [mailto:oss@buserror.net]
> > Sent: Friday, July 22, 2016 12:45 AM
> > To: Michael Ellerman; Arnd Bergmann
> > Cc: linux-mmc@vger.kernel.org; devicetree@vger.kernel.org; linuxppc-
> > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Yangbo Lu
> > Subject: Re: [PATCH v11 4/5] powerpc/fsl: move mpc85xx.h to
> > include/linux/fsl
> > 
> > On Thu, 2016-07-21 at 20:26 +1000, Michael Ellerman wrote:
> > > 
> > > Quoting Scott Wood (2016-07-21 04:31:48)
> > > > 
> > > > 
> > > > On Wed, 2016-07-20 at 13:24 +0200, Arnd Bergmann wrote:
> > > > > 
> > > > > 
> > > > > On Saturday, July 16, 2016 9:50:21 PM CEST Scott Wood wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > From: yangbo lu <yangbo.lu@nxp.com>
> > > > > > 
> > > > > > Move mpc85xx.h to include/linux/fsl and rename it to svr.h as a
> > > > > > common header file.  This SVR numberspace is used on some ARM
> > > > > > chips as well as PPC, and even to check for a PPC SVR multi-arch
> > > > > > drivers would otherwise need to ifdef the header inclusion and
> > > > > > all references to the SVR symbols.
> > > > > > 
> > > > > > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > > > > > Acked-by: Wolfram Sang <wsa@the-dreams.de>
> > > > > > Acked-by: Stephen Boyd <sboyd@codeaurora.org>
> > > > > > Acked-by: Joerg Roedel <jroedel@suse.de>
> > > > > > [scottwood: update description]
> > > > > > Signed-off-by: Scott Wood <oss@buserror.net>
> > > > > > 
> > > > > As discussed before, please don't introduce yet another vendor
> > > > > specific way to match a SoC ID from a device driver.
> > > > > 
> > > > > I've posted a patch for an extension to the soc_device
> > > > > infrastructure to allow comparing the running SoC to a table of
> > > > > devices, use that instead.
> > > > As I asked before, in which relevant maintainership capacity are you
> > > > NACKing this?
> > > I'll nack the powerpc part until you guys can agree.
> > OK, I've pulled these patches out.
> > 
> > For the MMC issue I suggest using ifdef CONFIG_PPC and mfspr(SPRN_SVR)
> > like the clock driver does[1] and we can revisit the issue if/when we
> > need to do something similar on an ARM chip.
> [Lu Yangbo-B47093] I remembered that Uffe had opposed us to introduce non-
> generic header files(like '#include <asm/mpc85xx.h>')
> in mmc driver initially. So I think it will not be accepted to use ifdef
> CONFIG_PPC and mfspr(SPRN_SVR)...
> And this method still couldn’t get SVR of ARM chip now.

Right, as I said we'll have to revisit the issue if/when we have the same
problem on an ARM chip.  That also applies if the PPC ifdef is still getting
NACKed from the MMC side.

> Any other suggestion here?

The other option is to try to come up with something that fits into Arnd's
framework while addressing the concerns I raised.  The soc_id string should be
well-structured to avoid mismatches and compatibility problems (especially
since it would get exposed to userspace).  Maybe something like:

svr:<SVR minus E bit>,svre:<full SVR including E bit>,name:<soc name>,die:<soc
die name>,rev:X.Y,<tag1>,<tag2>,<...>,

with the final comma used so that globs can put a colon on either end to be
sure they're matching a full field.  The SoC die name would be the primary
chip for a given die (e.g. p4040 would have a die name of p4080).  The "name"
and "die" fields would never include the trailing "e" indicated by the E bit.

Extra tags could be used for common groupings, such as all chips from a
particular die before a certain revision.  Once a tag is added it can't be
removed or reordered, to maintain userspace compatibility, but new tags could
be appended.

Some examples:

svr:0x82000020,svre:0x82000020,name:p4080,die:p4080,rev:2.0,
svr:0x82000020,svr
e:0x82080020,name:p4080,die:p4080,rev:2.0,
svr:0x82000030,svre:0x82000030,name:
p4080,die:p4080,rev:3.0,
svr:0x82000030,svre:0x82080030,name:p4080,die:p4080,re
v:3.0,
svr:0x82010020,svre:0x82010020,name:p4040,die:p4080,rev:2.0,
svr:0x820100
20,svre:0x82090020,name:p4040,die:p4080,rev:2.0,

svr:0x82010030,svre:0x82010030
,name:p4040,die:p4080,rev:3.0,
svr:0x82010030,svre:0x82090030,name:p4040,die:p4
080,rev:3.0,

Then if you want to apply a workaround on:
- all chips using the p4080 die, match with "*,die:p4080,*"
- all chips using the rev 2.0 p4080 die, match with "*,die:p4080,rev:2.0,*"
- Only p4040, but of any rev, match with "*,name:p4040,*"

Matching via open-coded hex number should be considered a last resort (it's
more error-prone, either for getting the number wrong or for forgetting
variants -- the latter is already a common problem), but preferable to adding
too many tags.

Using wildcards within a tag field would be discouraged.  

-Scott

[toc] | [next] | [standalone]

#1453609

Hi Scott,

> -----Original Message-----
> From: Scott Wood [mailto:oss@buserror.net]
> Sent: Wednesday, July 27, 2016 8:38 AM
> To: Yangbo Lu; Michael Ellerman; Arnd Bergmann; Ulf Hansson
> Cc: linux-mmc@vger.kernel.org; devicetree@vger.kernel.org; linuxppc-
> dev@lists.ozlabs.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v11 4/5] powerpc/fsl: move mpc85xx.h to
> include/linux/fsl
> 
> On Mon, 2016-07-25 at 06:12 +0000, Yangbo Lu wrote:
> > Hi Scott,
> >
> >
> > >
> > > -----Original Message-----
> > > From: Scott Wood [mailto:oss@buserror.net]
> > > Sent: Friday, July 22, 2016 12:45 AM
> > > To: Michael Ellerman; Arnd Bergmann
> > > Cc: linux-mmc@vger.kernel.org; devicetree@vger.kernel.org; linuxppc-
> > > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Yangbo Lu
> > > Subject: Re: [PATCH v11 4/5] powerpc/fsl: move mpc85xx.h to
> > > include/linux/fsl
> > >
> > > On Thu, 2016-07-21 at 20:26 +1000, Michael Ellerman wrote:
> > > >
> > > > Quoting Scott Wood (2016-07-21 04:31:48)
> > > > >
> > > > >
> > > > > On Wed, 2016-07-20 at 13:24 +0200, Arnd Bergmann wrote:
> > > > > >
> > > > > >
> > > > > > On Saturday, July 16, 2016 9:50:21 PM CEST Scott Wood wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > From: yangbo lu <yangbo.lu@nxp.com>
> > > > > > >
> > > > > > > Move mpc85xx.h to include/linux/fsl and rename it to svr.h
> > > > > > > as a common header file.  This SVR numberspace is used on
> > > > > > > some ARM chips as well as PPC, and even to check for a PPC
> > > > > > > SVR multi-arch drivers would otherwise need to ifdef the
> > > > > > > header inclusion and all references to the SVR symbols.
> > > > > > >
> > > > > > > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > > > > > > Acked-by: Wolfram Sang <wsa@the-dreams.de>
> > > > > > > Acked-by: Stephen Boyd <sboyd@codeaurora.org>
> > > > > > > Acked-by: Joerg Roedel <jroedel@suse.de>
> > > > > > > [scottwood: update description]
> > > > > > > Signed-off-by: Scott Wood <oss@buserror.net>
> > > > > > >
> > > > > > As discussed before, please don't introduce yet another vendor
> > > > > > specific way to match a SoC ID from a device driver.
> > > > > >
> > > > > > I've posted a patch for an extension to the soc_device
> > > > > > infrastructure to allow comparing the running SoC to a table
> > > > > > of devices, use that instead.
> > > > > As I asked before, in which relevant maintainership capacity are
> > > > > you NACKing this?
> > > > I'll nack the powerpc part until you guys can agree.
> > > OK, I've pulled these patches out.
> > >
> > > For the MMC issue I suggest using ifdef CONFIG_PPC and
> > > mfspr(SPRN_SVR) like the clock driver does[1] and we can revisit the
> > > issue if/when we need to do something similar on an ARM chip.
> > [Lu Yangbo-B47093] I remembered that Uffe had opposed us to introduce
> > non- generic header files(like '#include <asm/mpc85xx.h>') in mmc
> > driver initially. So I think it will not be accepted to use ifdef
> > CONFIG_PPC and mfspr(SPRN_SVR)...
> > And this method still couldn’t get SVR of ARM chip now.
> 
> Right, as I said we'll have to revisit the issue if/when we have the same
> problem on an ARM chip.  That also applies if the PPC ifdef is still
> getting NACKed from the MMC side.

[Lu Yangbo-B47093] It's not clear for me about your idea :( 
Do you mean we can still use this method, or not ?
I think Uffe had opposed to use ifdef CONFIG_PPC and mfspr(SPRN_SVR).
Is there any solution to resolve ?
:)

> 
> > Any other suggestion here?
> 
> The other option is to try to come up with something that fits into
> Arnd's framework while addressing the concerns I raised.  The soc_id
> string should be well-structured to avoid mismatches and compatibility
> problems (especially since it would get exposed to userspace).  Maybe
> something like:
> 
> svr:<SVR minus E bit>,svre:<full SVR including E bit>,name:<soc
> name>,die:<soc die name>,rev:X.Y,<tag1>,<tag2>,<...>,

[Lu Yangbo-B47093] The soc_device_attribut struct is defined as below.
struct soc_device_attribute {
        const char *machine;
        const char *family;
        const char *revision;
        const char *soc_id;
};

We can put the 'model' in root node of dts as machine, put 'Freescale QorIQ' as family,
and put x.x as revision. Is it ok?
As you suggested, you like to use below string as soc_id. It's easy to get svr, but how does the software know the name and die,
and put them into this string ? It's a large code to define them. 
> svr:<SVR minus E bit>,svre:<full SVR including E bit>,name:<soc
> name>,die:<soc die name>,rev:X.Y,<tag1>,<tag2>,<...>,
Should we remove rev here since there is also a revision member?

Regarding the guts_init, we still call guts_init and then match the soc, or we change to use platform driver?
Or do you know any better place to call guts_init to initialize only once?

Thank you so much :)

> 
> with the final comma used so that globs can put a colon on either end to
> be sure they're matching a full field.  The SoC die name would be the
> primary chip for a given die (e.g. p4040 would have a die name of
> p4080).  The "name"
> and "die" fields would never include the trailing "e" indicated by the E
> bit.
> 
> Extra tags could be used for common groupings, such as all chips from a
> particular die before a certain revision.  Once a tag is added it can't
> be removed or reordered, to maintain userspace compatibility, but new
> tags could be appended.
> 
> Some examples:
> 
> svr:0x82000020,svre:0x82000020,name:p4080,die:p4080,rev:2.0,
> svr:0x82000020,svr
> e:0x82080020,name:p4080,die:p4080,rev:2.0,
> svr:0x82000030,svre:0x82000030,name:
> p4080,die:p4080,rev:3.0,
> svr:0x82000030,svre:0x82080030,name:p4080,die:p4080,re
> v:3.0,
> svr:0x82010020,svre:0x82010020,name:p4040,die:p4080,rev:2.0,
> svr:0x820100
> 20,svre:0x82090020,name:p4040,die:p4080,rev:2.0,
> 
> svr:0x82010030,svre:0x82010030
> ,name:p4040,die:p4080,rev:3.0,
> svr:0x82010030,svre:0x82090030,name:p4040,die:p4
> 080,rev:3.0,
> 
> Then if you want to apply a workaround on:
> - all chips using the p4080 die, match with "*,die:p4080,*"
> - all chips using the rev 2.0 p4080 die, match with
> "*,die:p4080,rev:2.0,*"
> - Only p4040, but of any rev, match with "*,name:p4040,*"
> 
> Matching via open-coded hex number should be considered a last resort
> (it's more error-prone, either for getting the number wrong or for
> forgetting variants -- the latter is already a common problem), but
> preferable to adding too many tags.
> 
> Using wildcards within a tag field would be discouraged.
> 
> -Scott

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

#1455498

On Tue, 2016-08-02 at 05:57 +0000, Yangbo Lu wrote:
> Hi Scott,
> 
> > 
> > -----Original Message-----
> > From: Scott Wood [mailto:oss@buserror.net]
> > Sent: Wednesday, July 27, 2016 8:38 AM
> > To: Yangbo Lu; Michael Ellerman; Arnd Bergmann; Ulf Hansson
> > Cc: linux-mmc@vger.kernel.org; devicetree@vger.kernel.org; linuxppc-
> > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v11 4/5] powerpc/fsl: move mpc85xx.h to
> > include/linux/fsl
> > 
> > On Mon, 2016-07-25 at 06:12 +0000, Yangbo Lu wrote:
> > > 
> > > Hi Scott,
> > > 
> > > 
> > > > 
> > > > 
> > > > -----Original Message-----
> > > > From: Scott Wood [mailto:oss@buserror.net]
> > > > Sent: Friday, July 22, 2016 12:45 AM
> > > > To: Michael Ellerman; Arnd Bergmann
> > > > Cc: linux-mmc@vger.kernel.org; devicetree@vger.kernel.org; linuxppc-
> > > > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Yangbo Lu
> > > > Subject: Re: [PATCH v11 4/5] powerpc/fsl: move mpc85xx.h to
> > > > include/linux/fsl
> > > > 
> > > > On Thu, 2016-07-21 at 20:26 +1000, Michael Ellerman wrote:
> > > > > 
> > > > > 
> > > > > Quoting Scott Wood (2016-07-21 04:31:48)
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Wed, 2016-07-20 at 13:24 +0200, Arnd Bergmann wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On Saturday, July 16, 2016 9:50:21 PM CEST Scott Wood wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > From: yangbo lu <yangbo.lu@nxp.com>
> > > > > > > > 
> > > > > > > > Move mpc85xx.h to include/linux/fsl and rename it to svr.h
> > > > > > > > as a common header file.  This SVR numberspace is used on
> > > > > > > > some ARM chips as well as PPC, and even to check for a PPC
> > > > > > > > SVR multi-arch drivers would otherwise need to ifdef the
> > > > > > > > header inclusion and all references to the SVR symbols.
> > > > > > > > 
> > > > > > > > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > > > > > > > Acked-by: Wolfram Sang <wsa@the-dreams.de>
> > > > > > > > Acked-by: Stephen Boyd <sboyd@codeaurora.org>
> > > > > > > > Acked-by: Joerg Roedel <jroedel@suse.de>
> > > > > > > > [scottwood: update description]
> > > > > > > > Signed-off-by: Scott Wood <oss@buserror.net>
> > > > > > > > 
> > > > > > > As discussed before, please don't introduce yet another vendor
> > > > > > > specific way to match a SoC ID from a device driver.
> > > > > > > 
> > > > > > > I've posted a patch for an extension to the soc_device
> > > > > > > infrastructure to allow comparing the running SoC to a table
> > > > > > > of devices, use that instead.
> > > > > > As I asked before, in which relevant maintainership capacity are
> > > > > > you NACKing this?
> > > > > I'll nack the powerpc part until you guys can agree.
> > > > OK, I've pulled these patches out.
> > > > 
> > > > For the MMC issue I suggest using ifdef CONFIG_PPC and
> > > > mfspr(SPRN_SVR) like the clock driver does[1] and we can revisit the
> > > > issue if/when we need to do something similar on an ARM chip.
> > > [Lu Yangbo-B47093] I remembered that Uffe had opposed us to introduce
> > > non- generic header files(like '#include <asm/mpc85xx.h>') in mmc
> > > driver initially. So I think it will not be accepted to use ifdef
> > > CONFIG_PPC and mfspr(SPRN_SVR)...
> > > And this method still couldn’t get SVR of ARM chip now.
> > Right, as I said we'll have to revisit the issue if/when we have the same
> > problem on an ARM chip.  That also applies if the PPC ifdef is still
> > getting NACKed from the MMC side.
> [Lu Yangbo-B47093] It's not clear for me about your idea :( 
> Do you mean we can still use this method, or not ?
> I think Uffe had opposed to use ifdef CONFIG_PPC and mfspr(SPRN_SVR).
> Is there any solution to resolve ?
> :)

As I said, I'm OK with using the SPR.  It's up to you to find out whether it's
still unacceptable with the MMC maintainers given all the discussion (it would
be the quickest way to get the workaround enabled), or just go with the method
below.

> > > Any other suggestion here?
> > The other option is to try to come up with something that fits into
> > Arnd's framework while addressing the concerns I raised.  The soc_id
> > string should be well-structured to avoid mismatches and compatibility
> > problems (especially since it would get exposed to userspace).  Maybe
> > something like:
> > 
> > svr:<SVR minus E bit>,svre:<full SVR including E bit>,name:<soc
> > name>,die:<soc die name>,rev:X.Y,<tag1>,<tag2>,<...>,
> [Lu Yangbo-B47093] The soc_device_attribut struct is defined as below.
> struct soc_device_attribute {
>         const char *machine;
>         const char *family;
>         const char *revision;
>         const char *soc_id;
> };
> 
> We can put the 'model' in root node of dts as machine, put 'Freescale QorIQ'
> as family,

I'd just put "QorIQ" to avoid the question of whether to use "Freescale" or
"NXP".

> and put x.x as revision. Is it ok?
> As you suggested, you like to use below string as soc_id. It's easy to get
> svr, but how does the software know the name and die,
> and put them into this string ? It's a large code to define them. 

Yes, there would need to be a table in the guts driver for each SVR.  If the
SVR isn't found in the table then the soc_id would only contain the svr: and
svre: fields.

> > 
> > svr:<SVR minus E bit>,svre:<full SVR including E bit>,name:<soc
> > name>,die:<soc die name>,rev:X.Y,<tag1>,<tag2>,<...>,
> Should we remove rev here since there is also a revision member?

Yes, I forgot there was a revision field -- it should go there obviously.

> Regarding the guts_init, we still call guts_init and then match the soc, or
> we change to use platform driver?
> Or do you know any better place to call guts_init to initialize only once?

Use a platform driver for now.  If we ever need to check an ARM SVR in the
clock driver or similar place, then Arnd can explain what he wants us to do
then :-)

-Scott

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

#1455612

Hi Uffe,


> -----Original Message-----
> From: Scott Wood [mailto:oss@buserror.net]
> Sent: Wednesday, August 03, 2016 5:41 AM
> To: Yangbo Lu; Michael Ellerman; Arnd Bergmann; Ulf Hansson
> Cc: linux-mmc@vger.kernel.org; devicetree@vger.kernel.org; linuxppc-
> dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Xiaobo Xie
> Subject: Re: [PATCH v11 4/5] powerpc/fsl: move mpc85xx.h to
> include/linux/fsl
> 
> On Tue, 2016-08-02 at 05:57 +0000, Yangbo Lu wrote:
> > Hi Scott,
> >
> > >
> > > -----Original Message-----
> > > From: Scott Wood [mailto:oss@buserror.net]
> > > Sent: Wednesday, July 27, 2016 8:38 AM
> > > To: Yangbo Lu; Michael Ellerman; Arnd Bergmann; Ulf Hansson
> > > Cc: linux-mmc@vger.kernel.org; devicetree@vger.kernel.org; linuxppc-
> > > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH v11 4/5] powerpc/fsl: move mpc85xx.h to
> > > include/linux/fsl
> > >
> > > On Mon, 2016-07-25 at 06:12 +0000, Yangbo Lu wrote:
> > > >
> > > > Hi Scott,
> > > >
> > > >
> > > > >
> > > > >
> > > > > -----Original Message-----
> > > > > From: Scott Wood [mailto:oss@buserror.net]
> > > > > Sent: Friday, July 22, 2016 12:45 AM
> > > > > To: Michael Ellerman; Arnd Bergmann
> > > > > Cc: linux-mmc@vger.kernel.org; devicetree@vger.kernel.org;
> > > > > linuxppc- dev@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> > > > > Yangbo Lu
> > > > > Subject: Re: [PATCH v11 4/5] powerpc/fsl: move mpc85xx.h to
> > > > > include/linux/fsl
> > > > >
> > > > > On Thu, 2016-07-21 at 20:26 +1000, Michael Ellerman wrote:
> > > > > >
> > > > > >
> > > > > > Quoting Scott Wood (2016-07-21 04:31:48)
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On Wed, 2016-07-20 at 13:24 +0200, Arnd Bergmann wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > On Saturday, July 16, 2016 9:50:21 PM CEST Scott Wood wrote:
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > From: yangbo lu <yangbo.lu@nxp.com>
> > > > > > > > >
> > > > > > > > > Move mpc85xx.h to include/linux/fsl and rename it to
> > > > > > > > > svr.h as a common header file.  This SVR numberspace is
> > > > > > > > > used on some ARM chips as well as PPC, and even to check
> > > > > > > > > for a PPC SVR multi-arch drivers would otherwise need to
> > > > > > > > > ifdef the header inclusion and all references to the SVR
> symbols.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > > > > > > > > Acked-by: Wolfram Sang <wsa@the-dreams.de>
> > > > > > > > > Acked-by: Stephen Boyd <sboyd@codeaurora.org>
> > > > > > > > > Acked-by: Joerg Roedel <jroedel@suse.de>
> > > > > > > > > [scottwood: update description]
> > > > > > > > > Signed-off-by: Scott Wood <oss@buserror.net>
> > > > > > > > >
> > > > > > > > As discussed before, please don't introduce yet another
> > > > > > > > vendor specific way to match a SoC ID from a device driver.
> > > > > > > >
> > > > > > > > I've posted a patch for an extension to the soc_device
> > > > > > > > infrastructure to allow comparing the running SoC to a
> > > > > > > > table of devices, use that instead.
> > > > > > > As I asked before, in which relevant maintainership capacity
> > > > > > > are you NACKing this?
> > > > > > I'll nack the powerpc part until you guys can agree.
> > > > > OK, I've pulled these patches out.
> > > > >
> > > > > For the MMC issue I suggest using ifdef CONFIG_PPC and
> > > > > mfspr(SPRN_SVR) like the clock driver does[1] and we can revisit
> > > > > the issue if/when we need to do something similar on an ARM chip.
> > > > [Lu Yangbo-B47093] I remembered that Uffe had opposed us to
> > > > introduce
> > > > non- generic header files(like '#include <asm/mpc85xx.h>') in mmc
> > > > driver initially. So I think it will not be accepted to use ifdef
> > > > CONFIG_PPC and mfspr(SPRN_SVR)...
> > > > And this method still couldn’t get SVR of ARM chip now.
> > > Right, as I said we'll have to revisit the issue if/when we have the
> > > same problem on an ARM chip.  That also applies if the PPC ifdef is
> > > still getting NACKed from the MMC side.
> > [Lu Yangbo-B47093] It's not clear for me about your idea :( Do you
> > mean we can still use this method, or not ?
> > I think Uffe had opposed to use ifdef CONFIG_PPC and mfspr(SPRN_SVR).
> > Is there any solution to resolve ?
> > :)
> 
> As I said, I'm OK with using the SPR.  It's up to you to find out whether
> it's still unacceptable with the MMC maintainers given all the discussion
> (it would be the quickest way to get the workaround enabled), or just go
> with the method below.

[Lu Yangbo-B47093] As you know, this patchset(as below) has been discussed for more than one year.
What I want is just to add a fix for an specific soc revision.

Yangbo Lu (7):
  Documentation: DT: update Freescale DCFG compatible
  ARM64: dts: ls2080a: add device configuration node
  soc: fsl: add GUTS driver for QorIQ platforms
  dt: move guts devicetree doc out of powerpc directory
  powerpc/fsl: move mpc85xx.h to include/linux/fsl
  MAINTAINERS: add entry for Freescale SoC drivers
  mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0

But we have to abandon it since Arnd strongly disagree our guts driver method to get soc revision.
Now I have to ask you to reconsider the original method to get soc revison since we really have no better idea.
As Scott suggested above, use ifdef CONFIG_PPC and mfspr(SPRN_SVR) like the clock driver does to get SVR.
It's quickest way to resolve our esdhc issue. Could you reconsider to use this?

Although Arnd provided another new method by sending a proof-of-concept patch as below, there were still many controversial points.
I'm worried that would be discussed for a quite long time like the guts driver.
[PATCH 1/4]  base: soc: introduce soc_device_match() interface
[PATCH 2/4] soc: fsl: add GUTS driver for QorIQ platforms
[PATCH 3/4] mmc: sdhci-of-esdhc: fix host version for T4240-R1.0-R2.0
[PATCH 4/4] Revert "powerpc/fsl: Move fsl_guts.h out of arch/powerpc"

Anyway, what I want is just to fix the esdhc issue ASAP. 
Uffe, Could you reconsider whether you could accept the way using ifdef CONFIG_PPC and mfspr(SPRN_SVR)?
Or do you have any suggestion.

I will appreciate your suggestion.
Thanks a lot.

- Yangbo
> 
> > > > Any other suggestion here?
> > > The other option is to try to come up with something that fits into
> > > Arnd's framework while addressing the concerns I raised.  The soc_id
> > > string should be well-structured to avoid mismatches and
> > > compatibility problems (especially since it would get exposed to
> > > userspace).  Maybe something like:
> > >
> > > svr:<SVR minus E bit>,svre:<full SVR including E bit>,name:<soc
> > > name>,die:<soc die name>,rev:X.Y,<tag1>,<tag2>,<...>,
> > [Lu Yangbo-B47093] The soc_device_attribut struct is defined as below.
> > struct soc_device_attribute {
> >         const char *machine;
> >         const char *family;
> >         const char *revision;
> >         const char *soc_id;
> > };
> >
> > We can put the 'model' in root node of dts as machine, put 'Freescale
> QorIQ'
> > as family,
> 
> I'd just put "QorIQ" to avoid the question of whether to use "Freescale"
> or "NXP".
> 
> > and put x.x as revision. Is it ok?
> > As you suggested, you like to use below string as soc_id. It's easy to
> > get svr, but how does the software know the name and die, and put them
> > into this string ? It's a large code to define them.
> 
> Yes, there would need to be a table in the guts driver for each SVR.  If
> the SVR isn't found in the table then the soc_id would only contain the
> svr: and
> svre: fields.
> 
> > >
> > > svr:<SVR minus E bit>,svre:<full SVR including E bit>,name:<soc
> > > name>,die:<soc die name>,rev:X.Y,<tag1>,<tag2>,<...>,
> > Should we remove rev here since there is also a revision member?
> 
> Yes, I forgot there was a revision field -- it should go there obviously.
> 
> > Regarding the guts_init, we still call guts_init and then match the
> > soc, or we change to use platform driver?
> > Or do you know any better place to call guts_init to initialize only
> once?
> 
> Use a platform driver for now.  If we ever need to check an ARM SVR in
> the clock driver or similar place, then Arnd can explain what he wants us
> to do then :-)
> 
> -Scott

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web