On 08/29/14 13:04, Arnd Bergmann wrote:
On Friday 29 August 2014 17:21:18 Rafał Miłecki wrote:
On 28 August 2014 23:22, Hauke Mehrtens <hauke@xxxxxxxxxx> wrote:
On 08/28/2014 01:56 PM, Arnd Bergmann wrote:
On Thursday 28 August 2014 13:39:55 Rafał Miłecki wrote:
Well, that depends. Hauke was planning to put info about flash in DT.
I think it would make sense to have a common driver that has both
an 'early' init part used by MIPS and a regular init part used by
ARM and potentially also on MIPS if we want. Most of the code can
still be shared.
OK, now it's clear what you meant.
The thing is that we may want to call probe function from
drivers/bcma/main.c. I think we never meant to call it directly from
arch code. This code in drivers/bcma/main.c is used on both: MIPS and
ARM. So I wonder if there is much sense in doing it like
#ifdev MIPS
bcm47xx_nvram_init(nvram_address);
#endif
#ifdef ARM
nvram_device.resource[0].start = nvram_address;
platform_device_register(nvram_device);
#endif
What do you think about this?
I definitely don't want to see any manual platform_device_register()
calls on ARM, any device should be either a platform_device probed
from DT, or a bcma_device that comes from the bcma bus.
I suspect I'm still missing part of the story here. How is the
nvram chip actually connected?
I think we have to provide an own device tree for every board, like it
is done for other arm boards. If we do so I do not see a problem to
specify the nvram address space in device tree.
Alright, I think we should try to answer one main question at this
point: how much data we want to put in DTS? It's still not clear to
me.
What about this flash memory mapping? You added this in your RFC:
reg = <0x1c000000 0x01000000>;
As I described, the first part (address 0x1c000000) could be extracted
on runtime. For that you need my patch:
[PATCH] bcma: get & store info about flash type SoC booted from
http://www.spinics.net/lists/linux-wireless/msg126163.html
And then add some simple "swtich" like:
switch (boot_device) {
case BCMA_BOOT_DEV_NAND:
nvram_address = 0x1c000000;
break;
case BCMA_BOOT_DEV_SERIAL:
nvram_address = 0x1e000000;
break;
}
At the very least, those addresses should come from DT in some form.
We should never hardcode register locations in kernel code, since those
tend to change when a new hardware version comes out. Even if you are
sure that wouldn't happen with bcm53xx, it's still bad style and I
want to avoid having other developers copy code like that into a new
platform or driver.
So... should we handle it on runtime? Or do we really want this in DTS?
I was thinking about doing this on runtime. This would limit amount of
DTS entries and this is what makes more sense to me. The same way
don't hardcode many other hardware details. For example we don't store
flash size, block size, erase size in DTS. We simply use JEDEC and
mtd's spi-nor framework database.
I think the main difference is that for the example of the flash
chip, we can find out that information by looking at the device itself:
The DT describes how to find the device and from there we can do
proper hardware probing.
For the case of the nvram, I don't see how that would be done, since
the presence of the device itself is something your code above tries
to derive from something that from an unrelated setting, so I'd rather
see it done explicit in DT.
You mentioned that the 'boot_device' variable in your code snippet
comes from a hardware register that can be accessed easily, right?
A possible way to handle it would then be to have two DT entries
like
nvram@1c000000 {
compatible = "bcm,bcm4710-nvram";
reg = <0x1c000000 0x1000000>;
bcm,boot-device = BCMA_BOOT_DEV_NAND;
};
Just in case you happen to copy/paste that example as-is, this should be
"brcm" instead of "bcm" ;)
nvram@1c000000 {
compatible = "bcm,bcm4710-nvram";
reg = <0x1e000000 0x1000000>;
bcm,boot-device = BCMA_BOOT_DEV_SERIAL;
};
We would then have two platform device instances and get the
driver's probe function to reject any device whose bcm,boot-device
property doesn't match the contents of the register.
That would correctly describe the hardware while still allowing
automatic probing of the device, but I don't see a value in
the extra complexity compared to just marking one of the two
as status="disabled".
Arnd
