Date:	Wed, 3 Aug 2016 03:33:38 +0000
From:	Yangbo Lu <yangbo.lu@....com>
To:	Ulf Hansson <ulf.hansson@...aro.org>
CC:	"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Xiaobo Xie <xiaobo.xie@....com>, Scott Wood <oss@...error.net>,
	Michael Ellerman <mpe@...erman.id.au>,
	Arnd Bergmann <arnd@...db.de>
Subject: RE: [PATCH v11 4/5] powerpc/fsl: move mpc85xx.h to include/linux/fsl

Hi Uffe,


> -----Original Message-----
> From: Scott Wood [mailto:oss@...error.net]
> Sent: Wednesday, August 03, 2016 5:41 AM
> To: Yangbo Lu; Michael Ellerman; Arnd Bergmann; Ulf Hansson
> Cc: linux-mmc@...r.kernel.org; devicetree@...r.kernel.org; linuxppc-
> dev@...ts.ozlabs.org; linux-kernel@...r.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@...error.net]
> > > Sent: Wednesday, July 27, 2016 8:38 AM
> > > To: Yangbo Lu; Michael Ellerman; Arnd Bergmann; Ulf Hansson
> > > Cc: linux-mmc@...r.kernel.org; devicetree@...r.kernel.org; linuxppc-
> > > dev@...ts.ozlabs.org; linux-kernel@...r.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@...error.net]
> > > > > Sent: Friday, July 22, 2016 12:45 AM
> > > > > To: Michael Ellerman; Arnd Bergmann
> > > > > Cc: linux-mmc@...r.kernel.org; devicetree@...r.kernel.org;
> > > > > linuxppc- dev@...ts.ozlabs.org; linux-kernel@...r.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@....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@....com>
> > > > > > > > > Acked-by: Wolfram Sang <wsa@...-dreams.de>
> > > > > > > > > Acked-by: Stephen Boyd <sboyd@...eaurora.org>
> > > > > > > > > Acked-by: Joerg Roedel <jroedel@...e.de>
> > > > > > > > > [scottwood: update description]
> > > > > > > > > Signed-off-by: Scott Wood <oss@...error.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

Powered by blists - more mailing lists