+ lkml
Hi Rafał,
On Tue, 25 Nov 2014, Rafał Miłecki wrote:
> On 25 November 2014 at 18:50, Paul Walmsley <[email protected]> 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
On 27 November 2014 at 20:56, Paul Walmsley <[email protected]> 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ł
Hi Rafał,
On Thu, 27 Nov 2014, Rafał Miłecki wrote:
> On 27 November 2014 at 20:56, Paul Walmsley <[email protected]> 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 <[email protected]>
Commit: Adrian Bunk <[email protected]>
Commit: Al Viro <[email protected]>
Commit: Andi Kleen <[email protected]>
Commit: Benjamin Herrenschmidt <[email protected]>
Commit: David S. Miller <[email protected]>
Commit: David Woodhouse <[email protected]>
Commit: Dmitry Torokhov <[email protected]>
Commit: Greg Kroah-Hartman <[email protected]>
Commit: Greg Kroah-Hartman <[email protected]>
Commit: H. Peter Anvin <[email protected]>
Commit: H. Peter Anvin <[email protected]>
Commit: Ingo Molnar <[email protected]>
Commit: Ingo Molnar <[email protected]>
Commit: James Bottomley <[email protected]>
Commit: James Bottomley <[email protected]>
Commit: Jean Delvare <[email protected]>
Commit: Jeff Garzik <[email protected]>
Commit: Jeff Garzik <[email protected]>
Commit: Jesse Barnes <[email protected]>
Commit: Jiri Kosina <[email protected]>
Commit: Konrad Rzeszutek Wilk <[email protected]>
Commit: Konrad Rzeszutek Wilk <[email protected]>
Commit: Len Brown <[email protected]>
Commit: Linus Torvalds <[email protected]>
Commit: Linus Torvalds <[email protected]>
Commit: Linus Torvalds <[email protected]>
Commit: Linus Torvalds <[email protected]>
Commit: Linus Torvalds <[email protected]>
Commit: Mark Brown <[email protected]>
Commit: Mark M. Hoffman <[email protected]>
Commit: Matt Fleming <[email protected]>
Commit: Matthew Wilcox <[email protected]>
Commit: Paul Gortmaker <[email protected]>
Commit: Rafael J. Wysocki <[email protected]>
Commit: Russell King <[email protected]>
Commit: Tejun Heo <[email protected]>
Commit: Theodore Ts'o <[email protected]>
Commit: Thomas Gleixner <[email protected]>
Commit: Tony Luck <[email protected]>
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
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
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
On 4 December 2014 at 07:43, Paul Walmsley <[email protected]> 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.