Re: [PATCH V3] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

Re: [PATCH V3] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3537 bytes --]

+ lkml

Hi Rafał,

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

> On 25 November 2014 at 18:50, Paul Walmsley <paul@pwsan.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

Re: [PATCH V3] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

[Rafał Miłecki]

On 27 November 2014 at 20:56, Paul Walmsley <paul@pwsan.com> wrote:
> On Tue, 25 Nov 2014, Rafał Miłecki wrote:
>> 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.

The problem is I can't find MAINTAINER of the drivers/firmware/. Is
there someone responsible for that? Some mailing list maybe? Who could
give us an ACK to move bcm47xx_nvram there?


>> > 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.

I'm pretty sure you look at some old version of arch/bcm47xx/nvram.c.
I wouldn't dare to move such a MIPS-focused driver to some common
place ;)

Please check for the version of nvram.c in Ralf's upstream-sfr tree. I
think you'll like it much more. Hopefully you will even consider it
ready for moving to the drivers/firmware/ or whatever :)

-- 
Rafał

Re: [PATCH V3] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5905 bytes --]

Hi Rafał,

On Thu, 27 Nov 2014, Rafał Miłecki wrote:

> On 27 November 2014 at 20:56, Paul Walmsley <paul@pwsan.com> wrote:
> > On Tue, 25 Nov 2014, Rafał Miłecki wrote:
> >> 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.
> 
> The problem is I can't find MAINTAINER of the drivers/firmware/. Is
> there someone responsible for that? Some mailing list maybe? Who could
> give us an ACK to move bcm47xx_nvram there?

The list of folks who have committed patches that touch drivers/firmware 
is large.  I did this as a first-order approximation:

$ git log --format=fuller drivers/firmware/* | grep Commit: | sort -u
Commit:     Adrian Bunk <bunk@kernel.org>
Commit:     Adrian Bunk <bunk@stusta.de>
Commit:     Al Viro <viro@zeniv.linux.org.uk>
Commit:     Andi Kleen <andi@basil.nowhere.org>
Commit:     Benjamin Herrenschmidt <benh@kernel.crashing.org>
Commit:     David S. Miller <davem@davemloft.net>
Commit:     David Woodhouse <David.Woodhouse@intel.com>
Commit:     Dmitry Torokhov <dmitry.torokhov@gmail.com>
Commit:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Commit:     Greg Kroah-Hartman <gregkh@suse.de>
Commit:     H. Peter Anvin <hpa@linux.intel.com>
Commit:     H. Peter Anvin <hpa@zytor.com>
Commit:     Ingo Molnar <mingo@elte.hu>
Commit:     Ingo Molnar <mingo@kernel.org>
Commit:     James Bottomley <James.Bottomley@suse.de>
Commit:     James Bottomley <JBottomley@Parallels.com>
Commit:     Jean Delvare <khali@linux-fr.org>
Commit:     Jeff Garzik <jeff@garzik.org>
Commit:     Jeff Garzik <jgarzik@redhat.com>
Commit:     Jesse Barnes <jbarnes@virtuousgeek.org>
Commit:     Jiri Kosina <jkosina@suse.cz>
Commit:     Konrad Rzeszutek Wilk <konrad@kernel.org>
Commit:     Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Commit:     Len Brown <len.brown@intel.com>
Commit:     Linus Torvalds <torvalds@g5.osdl.org>
Commit:     Linus Torvalds <torvalds@linux-foundation.org>
Commit:     Linus Torvalds <torvalds@ppc970.osdl.org>
Commit:     Linus Torvalds <torvalds@woody.linux-foundation.org>
Commit:     Linus Torvalds <torvalds@woody.osdl.org>
Commit:     Mark Brown <broonie@opensource.wolfsonmicro.com>
Commit:     Mark M. Hoffman <mhoffman@lightlink.com>
Commit:     Matt Fleming <matt.fleming@intel.com>
Commit:     Matthew Wilcox <willy@linux.intel.com>
Commit:     Paul Gortmaker <paul.gortmaker@windriver.com>
Commit:     Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Commit:     Russell King <rmk+kernel@arm.linux.org.uk>
Commit:     Tejun Heo <tj@kernel.org>
Commit:     Theodore Ts'o <tytso@mit.edu>
Commit:     Thomas Gleixner <tglx@linutronix.de>
Commit:     Tony Luck <tony.luck@intel.com>

If I were in your shoes, I would suggest either

1. asking Ralf to merge your patches that touch drivers/firmware, since 
he'll also presumably be merging the parts that touch arch/mips

or 

2. asking Greg KH to merge those patches

And of course it would not hurt to collect some Reviewed-By:s from other 
folks.  (See below...)

> >> > 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.
> 
> I'm pretty sure you look at some old version of arch/bcm47xx/nvram.c.
> I wouldn't dare to move such a MIPS-focused driver to some common
> place ;)
> 
> Please check for the version of nvram.c in Ralf's upstream-sfr tree. I
> think you'll like it much more. Hopefully you will even consider it
> ready for moving to the drivers/firmware/ or whatever :)

OK I will take a look at this, and will either send comments, or will 
send a Reviewed-By:.


Thanks for all of the good discussion on this :-)

- Paul

Re: [PATCH V3] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

[Ralf Baechle]

On Fri, Nov 28, 2014 at 05:07:12PM +0000, Paul Walmsley wrote:

> If I were in your shoes, I would suggest either
> 
> 1. asking Ralf to merge your patches that touch drivers/firmware, since 
> he'll also presumably be merging the parts that touch arch/mips
> 
> or 
> 
> 2. asking Greg KH to merge those patches
> 
> And of course it would not hurt to collect some Reviewed-By:s from other 
> folks.  (See below...)

I surely can carry this patch.

  Ralf

Re: [PATCH V3] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4179 bytes --]

Hello Rafał,

On Fri, 28 Nov 2014, Paul Walmsley wrote:

> On Thu, 27 Nov 2014, Rafał Miłecki wrote:
> 
> > I'm pretty sure you look at some old version of arch/bcm47xx/nvram.c.
> > I wouldn't dare to move such a MIPS-focused driver to some common
> > place ;)
> > 
> > Please check for the version of nvram.c in Ralf's upstream-sfr tree. I
> > think you'll like it much more. Hopefully you will even consider it
> > ready for moving to the drivers/firmware/ or whatever :)
> 
> OK I will take a look at this, and will either send comments, or will 
> send a Reviewed-By:.

I had the chance to take a brief look at this file, and you are right: I
like your newer version better than the older one :-)

It is too bad that it seems this flash area has to be accessed very early 
in boot.  That certainly makes it more difficult to write something 
particularly elegant.  It is also a pity that it is unknown how to verify 
that the flash MMIO window has been configured before reading from these 
areas of the address space.  But under the circumstances, calling 
bcm47xx_nvram_init_from_mem() with the appropriate addresses from the bus 
code during early init, as you did, seems rather reasonable.  I also like 
the code that you added to read the flash data from MTD later in boot.

Here are a few very minor comments that you are welcome to take or leave 
as you wish.

1. In nvram_find_and_copy(), the flash header copy loop uses:

	for (i = 0; i < sizeof(struct nvram_header); i += 4)
		*dst++ = *src++;

Consider using either __raw_readl() to read from the MMIO window, or 
possibly memcpy_fromio().  In theory, all MMIO accesses should use 
functions like these.


2. In nvram_find_and_copy(), the flash payload copy loop uses:

	for (; i < header->len && i < NVRAM_SPACE && i < size; i += 4)
		*dst++ = le32_to_cpu(*src++);

Consider using readl() instead of the direct MMIO read & endianness 
conversion.  


3. In nvram_find_and_copy(), I don't believe that this is necessary:

memset(dst, 0x0, NVRAM_SPACE - i);

since nvram_buf[] is a file-static variable, and thus should have been
initialized to all zeroes.


4. As with #3 above, in nvram_init(), I don't believe that this is 
necessary:

memset(dst + bytes_read, 0x0, NVRAM_SPACE - bytes_read);


5. In bcm47xx_nvram_getenv(), this multiple assignment exists:

end[0] = end[1] = '\0';

Best to avoid multiple assignments, per Chapter 1 of 
Documentation/CodingStyle.  You might consider running checkpatch.pl on 
the file:

$ scripts/checkpatch.pl -f --strict arch/mips/bcm47xx/nvram.c
CHECK: No space is necessary after a cast
#101: FILE: arch/mips/bcm47xx/nvram.c:101:
+	src = (u32 *) header;

CHECK: No space is necessary after a cast
#102: FILE: arch/mips/bcm47xx/nvram.c:102:
+	dst = (u32 *) nvram_buf;

CHECK: multiple assignments should be avoided
#195: FILE: arch/mips/bcm47xx/nvram.c:195:
+	end[0] = end[1] = '\0';

CHECK: Alignment should match open parenthesis
#202: FILE: arch/mips/bcm47xx/nvram.c:202:
+		if ((eq - var) == strlen(name) &&
+			strncmp(var, name, (eq - var)) == 0) {


6. bcm47xx_nvram_getenv() calls strchr().  Perhaps it would be better to 
use strnchr(), in case the flash data is corrupted or in an invalid 
format?


7. There are a few magic numbers in this code, mostly in 
bcm47xx_nvram_gpio_pin().  It might be worth converting those to macros 
and documenting the expectations there in a comment above the macro.


8.  The way that bcm47xx_nvram_gpio_pin() calls bcm47xx_nvram_getenv() 
seems a bit inefficient.  It might be better to loop over all of the keys 
in the shadowed flash, looking for values that match "name".  Then if the 
key name matches "gpio" plus one or two digits, the code could just return 
the digits.  That way, only one pass is needed, rather than 32 (in the 
worst case).  Well, at least the reads should be coming from cached DRAM, 
rather than flash...

If you fix/address those (or correct my review ;-) ), then you're 
welcome to add my Reviewed-by: to a patch that moves this file out to 
drivers/firmware.


regards, 

- Paul

Re: [PATCH V3] MIPS: BCM47XX: Move NVRAM driver to the drivers/soc/

[Rafał Miłecki]

On 4 December 2014 at 07:43, Paul Walmsley <paul@pwsan.com> wrote:
> Hello Rafał,
>
> On Fri, 28 Nov 2014, Paul Walmsley wrote:
>
>> On Thu, 27 Nov 2014, Rafał Miłecki wrote:
>>
>> > I'm pretty sure you look at some old version of arch/bcm47xx/nvram.c.
>> > I wouldn't dare to move such a MIPS-focused driver to some common
>> > place ;)
>> >
>> > Please check for the version of nvram.c in Ralf's upstream-sfr tree. I
>> > think you'll like it much more. Hopefully you will even consider it
>> > ready for moving to the drivers/firmware/ or whatever :)
>>
>> OK I will take a look at this, and will either send comments, or will
>> send a Reviewed-By:.
>
> I had the chance to take a brief look at this file, and you are right: I
> like your newer version better than the older one :-)
>
> It is too bad that it seems this flash area has to be accessed very early
> in boot.  That certainly makes it more difficult to write something
> particularly elegant.  It is also a pity that it is unknown how to verify
> that the flash MMIO window has been configured before reading from these
> areas of the address space.  But under the circumstances, calling
> bcm47xx_nvram_init_from_mem() with the appropriate addresses from the bus
> code during early init, as you did, seems rather reasonable.  I also like
> the code that you added to read the flash data from MTD later in boot.
>
> Here are a few very minor comments that you are welcome to take or leave
> as you wish.

Thanks for your comments! I'll address them before (trying) moving
driver to the drivers/firmware/. Appreciate your review.