Date:	Thu, 27 Nov 2014 19:56:46 +0000 (UTC)
From:	Paul Walmsley <paul@...an.com>
To:	Rafał Miłecki <zajec5@...il.com>
cc:	"linux-mips@...ux-mips.org" <linux-mips@...ux-mips.org>,
	Ralf Baechle <ralf@...ux-mips.org>,
	Hauke Mehrtens <hauke@...ke-m.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Kumar Gala <galak@...eaurora.org>,
	Olof Johansson <olof@...om.net>, Arnd Bergmann <arnd@...db.de>,
	Sandeep Nair <sandeep_n@...com>, linux-soc@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V3] MIPS: BCM47XX: Move NVRAM driver to the
 drivers/soc/

+ lkml

Hi Rafał,

On Tue, 25 Nov 2014, Rafał Miłecki wrote:

> On 25 November 2014 at 18:50, Paul Walmsley <paul@...an.com> wrote:
> >> I don't think NVRAM can be treated as a standard char device. Also, in
> >> my V1 I tried moving it to the drivers/misc/, but then drivers/soc/
> >> was suggested as a better place, see Arnd's reply:
> >> http://www.linux-mips.org/archives/linux-mips/2014-11/msg00238.html
> >
> > Yeah.  It depends on who is going to merge the patch.  If you can persuade
> > someone else to merge it in drivers/soc, then it doesn't really matter
> > what I think.
> 
> I'm looking for the solution that will satisfy most ppl. 

Wow, that's a pretty high bar ;-)  Better IMHO to try for something that 
seems to make rational sense, then let others convert it into something 
that makes less sense afterwards, if necessary ;-)

> I understand your arguments against drivers/soc/, but on the other hand 
> I have no idea where else this driver could go.

After looking around the tree to find out where similar code is located, 
it looks like drivers/firmware is the right place.  These days, 
drivers/firmware is mainly used for drivers that parse EFI bootloader 
data, DMI data, that sort of thing.  Quite similar to the CFE-provided 
data that the bcm47xx-nvram code deals with.  So, by functional analogy, 
drivers/firmware appears to be the right place to stash this device 
data-probing code.

> I guess DT is older than CFE, but Broadcom decided to invent own
> solution called NVRAM anyway. This is a bit messy, because it actually
> stores hardware details (CPU, RAM, switch) as well as user settings
> (e.g. LEDs behavior). I can't say why Broadcom decided to implement it
> this way.

Yep, based on what the other drivers in drivers/firmware are used for, I 
think drivers/firmware is the right place for the CFE parsing code.

> > It sounds to me like this code is a combination of three
> > pieces:
> >
> > 1. code that autoprobes the size of the "nvram" partition in the Broadcom
> > platform flash, by reading various locations in the MMIO flash aperture,
> > configured by some other system entity
> 
> That's right, on MIPS we simply detect flash type (drivers/ssb &
> driver/bcma) and using that we init NVRAM passing memory offset where
> the flash is mapped.

OK.

So (as a side issue), I would suggest that when you move this code out of 
arch/mips, the MIPS-isms in it should be removed, like KSEG1ADDR(), etc., 
and replaced by the standard ioremap()-type approach.  After all, Broadcom 
could build CFE for ARM, and then we'd want to use this same code to parse 
the CFE-provided data.

Also I would suggest getting rid of the #ifdefs for the flash type, and 
probing it dynamically instead.  The flash setup code under drivers/ssb/ 
and drivers/bcma/ sets up platform_devices for the flash, right?  If so 
then it would be best if this code could run after the bus setup code, 
query the Linux device model for the type of platform flash in use, and 
then extract the appropriate address space to probe from that data.

> > 2. code that shadow copies the device data from the MMIO flash aperture
> > into system RAM
> >
> > 3. code that parses the CPE-generated device data and returns it to other
> > drivers
> >
> > Does that sound accurate?
> 
> Correct (s/CPE/CFE).

Thanks for the correction, I must have been dreaming of telecom 
equipment..

What do you think?  Does this sound reasonable?


- Paul

Powered by blists - more mailing lists