Hi Rafał, On Mon, 24 Nov 2014, Rafał Miłecki wrote: > On 24 November 2014 at 11:02, Paul Walmsley <paul@xxxxxxxxx> wrote: > > Hi Rafał. > > > > On Sun, 23 Nov 2014, Rafał Miłecki wrote: > > > >> After Broadcom switched from MIPS to ARM for their home routers we need > >> to have NVRAM driver in some common place (not arch/mips/). > > > > So this is a driver for a chunk of NVRAM that's embedded in the SoC, > > hanging off an I/O bus? > > > > Is this actually some kind of RAM, or is it flash, or something else? > > I think you missed my description (help section) in Kconfig file :) That's correct. I only looked at the patch description in the E-mail, and the code in arch/mips/bcm47xx/nvram.c. > As it says, NVRAM is a special partition on flash. OK, thanks for the clarification. > >> We were thinking about putting it in bus directory, however there are > >> two possible buses for MIPS: drivers/ssb/ and drivers/bcma/. So this > >> won't fit there neither. > >> This is why I would like to move this driver to the drivers/soc/ > > > > Can't speak for anyone else, but I personally consider drivers/soc to be > > primarily intended for so-called "integration" IP blocks. Those are used > > for SoC control functions. > > > > So low-level NVRAM drivers would probably go somewhere else. Here are > > some likely possibilities, where it can live with others of its kind > > (assuming it is something similar to RAM): > > > > 1. the PC "CMOS memory" NVRAM driver appears to be under drivers/char, as > > drivers/char/nvram.c. > > > > 2. there's a generic SRAM driver, drivers/misc/sram.c > > > > 3. while people have put some of the eFuse code under drivers/soc, in my > > opinion, the low-level fuse access code should really go under > > drivers/misc/fuse. > > 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. To my mind, drivers/soc was intended to fill a very narrow and specific need: for low-level, hardware IP block drivers for so-called 'integration' IP blocks. These IP blocks generally control some combination of clocks, external muxing (pin mux), internal muxing, reset, IO cells/bricks, debug observability, etc., and tend to be very specific to particular SoC families. While it's certainly understandable that folks would like a place to move arbitrary code out of arch/*, drivers/soc was not intended to be that. > > Looking at arch/mips/bcm47xx/nvram.c: if the low-level NVRAM probe code > > were moved elsewhere, the higher-level NVRAM "interpretation" functions > > still remain: bcm47xx_nvram_getenv() and bcm47xx_nvram_gpio_pin(). Those > > seem to be intended to parse device configuration data, yes? And this > > device configuration data is organized this way by platform software > > convention -- there's no hardware requirement to store data this way in > > the NVRAM, right? > > This bcm47xx_nvram driver has two ways of reading NVRAM content: > 1) Using a standard MTD API to access "nvram" partition. In such case > flash driver is a low-level thing if you call it so. OK, so just to confirm, you are referring to these drivers: drivers/mtd/nand/bcm47xxnflash/* drivers/mtd/devices/bcm47xxsflash.c drivers/ssb/driver_mipscore.c (for pflash) drivers/bcma/driver_mips.c (for pflash) ? Just out of curiosity, the nvram.c code seems to contain some code to work around cases where the flash size is larger than the MMIO aperture, and to truncate the copy. Is there some way to program the flash controller to point the aperture at a different section of the flash? > 2) Using memory-mapped flash access window. In such case there isn't > any extra low-level driver. Could you point me at the software entity that configures the memory-mapped flash access window and programs the flash controller to use one of {p,n,s}flash? Or is that done by some code external to the kernel, like the bootloader? > The "nvram" partition is required by the bootloader (CFE). It contains > some important info like CPU clock, RAM configuration, switch ports > layout. Then it's used by system drivers (like Ethernet driver bgmac) > to get info about some particular hardware parts (like PHY address, > MAC, etc.). So it's not used by the on-chip boot-ROM? Sounds like it's just software convention, then? In other words, if one used a different bootloader, like u-boot, and passed a DT or something like that to the kernel, this wouldn't be needed. The presence and format of this flash is part of a specific hardware/software platform, external to the SoC hardware itself, that Broadcom suggests. It's analogous to ARM ATAGS - arch/arm/kernel/atags_parse.c. Does all this match your understanding? (I'm not advocating using a different bootloader or device data format - I think it's important to preserve compatibility with CPE - I am just trying to understand how this area of flash is used.) > > So if the answer to the above two questions is "yes," meaning that this > > NVRAM is used to store device probing data by software convention, then it > > would seem to me that proposing some place like > > drivers/devicedata/bcm47xx-nvram is a better bet (where "bcm47xx-nvram" is > > just some kind of opaque token). Looking at those two functions, it > > doesn't seem like there's really anything MIPS or Broadcom or NVRAM or > > even SoC-specific about key-value pair parsing and GPIO device data? Or > > am I missing something? > > > > Actually, considering that bcm47xx_nvram_gpio_pin() isn't even used, can > > we just drop it and save the hassle? :-) > > We need this function to easily find GPIO responsible for "something". > For example during switch initialization we need to reset it toggling > GPIO. NVRAM contains info which GPIO should be toggled, e.g.: > gpio4=robo_reset > (robo means roboswitch) > > bcm47xx_nvram_gpio_pin is needed by out-of-tree "b53" driver that some > tried to push upstream, but was rejected because of API. > > I'll also send a patch to USB host driver soon that will require > bcm47xx_nvram_gpio_pin. OK thanks for the context. 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 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? - Paul
