Re: Booting bcm47xx (bcma & stuff), sharing code with bcm53xx

[Florian Fainelli <f.fainelli@gmail.com>]

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@hauke-m.de> 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
>