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

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

[Rafał Miłecki]

[cross-list: linux-mips@ and linux-wireless@]

We're working on another Broadcom platform, SoC with an ARM CPU,
platform called bcm53xx. It shares many things with the older (MIPS
based) bcm47xx, so we need to figure out how to modify some of the
drivers.

Hauke recently proposed sharing code for NVRAM support as a separated
driver. In his RFC patch it was added as a new platform driver. I
liked this idea (I'd simply prefer to modify existing code instead of
duplicating it), so I played with it a bit today.

My plan was to modify bcm47xx code to make nvram.c a separated driver
and update bcm47xx/bcma to use it. Well, it didn't work out. The
problem is that we need access to the NVRAM pretty early. Please take
a look at my description of bcm47xx booting process (it's simply a
summary of start_kernel and bcm47xx code):

1) prom_init / plat_mem_setup
These two functions are called in pretty much the same phase from the
setup_arch (arch/mips/kernel/setup.c).
Task: detect & register memory
Requires: CPU type, maybe Broadcom chip ID (highmem support)
Available: CPU type
Not available: kmalloc, device_add (kobject)

2) arch_init_irq
Called from the arch specific init_IRQ (arch/mips/kernel/irq.c)
Task: setup bcma's MIPS core
Requires: bcma bus MIPS core
Available: kmalloc
Not available: device_add (kobject)

3) plat_time_init
Called from the arch specific time_init (arch/mips/kernel/time.c)
Task: set frequency
Requires: bcma bus ChipCommon core, nvram
Available: kmalloc
Not available: device_add (kobject)

4) At some point we need to register bcma devices, device_initcall can
be used for that

As you can see, we need access to the NVRAM quite early (step 3,
plat_time_init, or even earlier), but device_add (platform
devices/drivers) is not available then yet. So I'm afraid we won't be
able to use this common way to write NVRAM driver.


So there I want to present my plan for the NVRAM improvements. If you
don't agree with any part of it, or you can see any better solution,
please speak up!

1) I won't make nvram.c a platform driver. Instead I would like to
make it less bcm47xx specific. I don't want to touch bcm47xx_bus in
this file. Instead I want to add a generic function that will accept
address and size of memory where NVRAM should be found. Then I'd like
to move this file out of "mips" arch (drivers/misc/?
drivers/bcma/nvram/?) and allow using it for bcm53xx.

2) I was also thinking about cleaning bcm47xx init. Right now we do a
lot of hacks in plat_mem_setup & bcma to register the bus and scan its
cores. It's so early (before mm_init) that we can't alloc memory!
Doing all this stuff slightly later (e.g. arch_init_irq) would allow
us to simply use "kmalloc" and drop all current hacks in bcma.

3) Above change (point 2) would require some small change in bcma. We
would need 2-stages init: detecting (with kmalloc!) bus cores,
registering cores. This is required, because we can't register cores
too early, device_add (and the underlying kobject) would oops/WARN in
kobject_get.

-- 
Rafał

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

[Hauke Mehrtens]

On 08/26/2014 06:42 PM, Rafał Miłecki wrote:
> [cross-list: linux-mips@ and linux-wireless@]
> 
> We're working on another Broadcom platform, SoC with an ARM CPU,
> platform called bcm53xx. It shares many things with the older (MIPS
> based) bcm47xx, so we need to figure out how to modify some of the
> drivers.
> 
> Hauke recently proposed sharing code for NVRAM support as a separated
> driver. In his RFC patch it was added as a new platform driver. I
> liked this idea (I'd simply prefer to modify existing code instead of
> duplicating it), so I played with it a bit today.

I will also make mips bcm47xx uses that code in the next version of the
patch. (move the code from mips)

> 
> My plan was to modify bcm47xx code to make nvram.c a separated driver
> and update bcm47xx/bcma to use it. Well, it didn't work out. The
> problem is that we need access to the NVRAM pretty early. Please take
> a look at my description of bcm47xx booting process (it's simply a
> summary of start_kernel and bcm47xx code):
> 
> 1) prom_init / plat_mem_setup
> These two functions are called in pretty much the same phase from the
> setup_arch (arch/mips/kernel/setup.c).
> Task: detect & register memory
> Requires: CPU type, maybe Broadcom chip ID (highmem support)
> Available: CPU type
> Not available: kmalloc, device_add (kobject)
> 
> 2) arch_init_irq
> Called from the arch specific init_IRQ (arch/mips/kernel/irq.c)
> Task: setup bcma's MIPS core
> Requires: bcma bus MIPS core
> Available: kmalloc
> Not available: device_add (kobject)
> 
> 3) plat_time_init
> Called from the arch specific time_init (arch/mips/kernel/time.c)
> Task: set frequency
> Requires: bcma bus ChipCommon core, nvram
> Available: kmalloc
> Not available: device_add (kobject)
> 
> 4) At some point we need to register bcma devices, device_initcall can
> be used for that
> 
> As you can see, we need access to the NVRAM quite early (step 3,
> plat_time_init, or even earlier), but device_add (platform
> devices/drivers) is not available then yet. So I'm afraid we won't be
> able to use this common way to write NVRAM driver.
> 
> 
> So there I want to present my plan for the NVRAM improvements. If you
> don't agree with any part of it, or you can see any better solution,
> please speak up!
> 
> 1) I won't make nvram.c a platform driver. Instead I would like to
> make it less bcm47xx specific. I don't want to touch bcm47xx_bus in
> this file. Instead I want to add a generic function that will accept
> address and size of memory where NVRAM should be found. Then I'd like
> to move this file out of "mips" arch (drivers/misc/?
> drivers/bcma/nvram/?) and allow using it for bcm53xx.

I would make this nvram.c a platform driver in addition so it can get
registered to device tree. this part would only get activated when
CONFIG_OF is set which is not on MIPS bcm47xx.

> 2) I was also thinking about cleaning bcm47xx init. Right now we do a
> lot of hacks in plat_mem_setup & bcma to register the bus and scan its
> cores. It's so early (before mm_init) that we can't alloc memory!
> Doing all this stuff slightly later (e.g. arch_init_irq) would allow
> us to simply use "kmalloc" and drop all current hacks in bcma.
> 
> 3) Above change (point 2) would require some small change in bcma. We
> would need 2-stages init: detecting (with kmalloc!) bus cores,
> registering cores. This is required, because we can't register cores
> too early, device_add (and the underlying kobject) would oops/WARN in
> kobject_get.
> 

This sound good to me, but I still have some questions.

Do you also want to change ssb registration?
Is it worth the effort? I think MIPS bcm47xx will be EOL and replaced by
the ARM versions completely in the next years. (I do not have any
private information about Broadcom product politics)

I think this will be reduce the number of hacks a little bit, but you
still need a 2 stage init of bcma for mips SoCs, and I do not know how
to prevent this.

Hauke

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

[Arend van Spriel]

On 08/26/14 22:32, Hauke Mehrtens wrote:
> On 08/26/2014 06:42 PM, Rafał Miłecki wrote:
>> [cross-list: linux-mips@ and linux-wireless@]
>>
>> We're working on another Broadcom platform, SoC with an ARM CPU,
>> platform called bcm53xx. It shares many things with the older (MIPS
>> based) bcm47xx, so we need to figure out how to modify some of the
>> drivers.
>>
>> Hauke recently proposed sharing code for NVRAM support as a separated
>> driver. In his RFC patch it was added as a new platform driver. I
>> liked this idea (I'd simply prefer to modify existing code instead of
>> duplicating it), so I played with it a bit today.
>
> I will also make mips bcm47xx uses that code in the next version of the
> patch. (move the code from mips)
>
>>
>> My plan was to modify bcm47xx code to make nvram.c a separated driver
>> and update bcm47xx/bcma to use it. Well, it didn't work out. The
>> problem is that we need access to the NVRAM pretty early. Please take
>> a look at my description of bcm47xx booting process (it's simply a
>> summary of start_kernel and bcm47xx code):
>>
>> 1) prom_init / plat_mem_setup
>> These two functions are called in pretty much the same phase from the
>> setup_arch (arch/mips/kernel/setup.c).
>> Task: detect&  register memory
>> Requires: CPU type, maybe Broadcom chip ID (highmem support)
>> Available: CPU type
>> Not available: kmalloc, device_add (kobject)
>>
>> 2) arch_init_irq
>> Called from the arch specific init_IRQ (arch/mips/kernel/irq.c)
>> Task: setup bcma's MIPS core
>> Requires: bcma bus MIPS core
>> Available: kmalloc
>> Not available: device_add (kobject)
>>
>> 3) plat_time_init
>> Called from the arch specific time_init (arch/mips/kernel/time.c)
>> Task: set frequency
>> Requires: bcma bus ChipCommon core, nvram
>> Available: kmalloc
>> Not available: device_add (kobject)
>>
>> 4) At some point we need to register bcma devices, device_initcall can
>> be used for that
>>
>> As you can see, we need access to the NVRAM quite early (step 3,
>> plat_time_init, or even earlier), but device_add (platform
>> devices/drivers) is not available then yet. So I'm afraid we won't be
>> able to use this common way to write NVRAM driver.
>>
>>
>> So there I want to present my plan for the NVRAM improvements. If you
>> don't agree with any part of it, or you can see any better solution,
>> please speak up!
>>
>> 1) I won't make nvram.c a platform driver. Instead I would like to
>> make it less bcm47xx specific. I don't want to touch bcm47xx_bus in
>> this file. Instead I want to add a generic function that will accept
>> address and size of memory where NVRAM should be found. Then I'd like
>> to move this file out of "mips" arch (drivers/misc/?
>> drivers/bcma/nvram/?) and allow using it for bcm53xx.
>
> I would make this nvram.c a platform driver in addition so it can get
> registered to device tree. this part would only get activated when
> CONFIG_OF is set which is not on MIPS bcm47xx.
>
>> 2) I was also thinking about cleaning bcm47xx init. Right now we do a
>> lot of hacks in plat_mem_setup&  bcma to register the bus and scan its
>> cores. It's so early (before mm_init) that we can't alloc memory!
>> Doing all this stuff slightly later (e.g. arch_init_irq) would allow
>> us to simply use "kmalloc" and drop all current hacks in bcma.
>>
>> 3) Above change (point 2) would require some small change in bcma. We
>> would need 2-stages init: detecting (with kmalloc!) bus cores,
>> registering cores. This is required, because we can't register cores
>> too early, device_add (and the underlying kobject) would oops/WARN in
>> kobject_get.
>>
>
> This sound good to me, but I still have some questions.
>
> Do you also want to change ssb registration?
> Is it worth the effort? I think MIPS bcm47xx will be EOL and replaced by
> the ARM versions completely in the next years. (I do not have any
> private information about Broadcom product politics)

I am not a (product) politician as well, but I think it is a safe 
assumption.

Regards,
Arend

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

[Rafał Miłecki]

On 26 August 2014 22:32, Hauke Mehrtens <hauke@hauke-m.de> wrote:
> On 08/26/2014 06:42 PM, Rafał Miłecki wrote:
>> 3) Above change (point 2) would require some small change in bcma. We
>> would need 2-stages init: detecting (with kmalloc!) bus cores,
>> registering cores. This is required, because we can't register cores
>> too early, device_add (and the underlying kobject) would oops/WARN in
>> kobject_get.
>>
>
> This sound good to me, but I still have some questions.
>
> Do you also want to change ssb registration?
> Is it worth the effort? I think MIPS bcm47xx will be EOL and replaced by
> the ARM versions completely in the next years. (I do not have any
> private information about Broadcom product politics)

ssb has its own hacks like having "struct device" static (I think it
was a big "no" from Greg when introducing bcma). ssb is already smart
enough to detect early boot phase and don't register devices then. I
think we won't need to modify ssb at all.
On the other hand I care about bcma, as it's used by PCIe devices and
will still be used on ARM SoCs.


> I think this will be reduce the number of hacks a little bit, but you
> still need a 2 stage init of bcma for mips SoCs, and I do not know how
> to prevent this.

I'm OK with two separated calls to the bcma to register it fully. Not
a big deal. We could also think about sth like a ssb_is_early_boot,
not sure about this yet.

-- 
Rafał

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

[Arnd Bergmann]

On Tuesday 26 August 2014 18:42:51 Rafał Miłecki wrote:
> 
> We're working on another Broadcom platform, SoC with an ARM CPU,
> platform called bcm53xx. It shares many things with the older (MIPS
> based) bcm47xx, so we need to figure out how to modify some of the
> drivers.
> 
> Hauke recently proposed sharing code for NVRAM support as a separated
> driver. In his RFC patch it was added as a new platform driver. I
> liked this idea (I'd simply prefer to modify existing code instead of
> duplicating it), so I played with it a bit today.
> 
> My plan was to modify bcm47xx code to make nvram.c a separated driver
> and update bcm47xx/bcma to use it. Well, it didn't work out. The
> problem is that we need access to the NVRAM pretty early. Please take
> a look at my description of bcm47xx booting process (it's simply a
> summary of start_kernel and bcm47xx code):
> 
> 1) prom_init / plat_mem_setup
> These two functions are called in pretty much the same phase from the
> setup_arch (arch/mips/kernel/setup.c).
> Task: detect & register memory
> Requires: CPU type, maybe Broadcom chip ID (highmem support)
> Available: CPU type
> Not available: kmalloc, device_add (kobject)
> 
> 2) arch_init_irq
> Called from the arch specific init_IRQ (arch/mips/kernel/irq.c)
> Task: setup bcma's MIPS core
> Requires: bcma bus MIPS core
> Available: kmalloc
> Not available: device_add (kobject)
> 
> 3) plat_time_init
> Called from the arch specific time_init (arch/mips/kernel/time.c)
> Task: set frequency
> Requires: bcma bus ChipCommon core, nvram
> Available: kmalloc
> Not available: device_add (kobject)

My impression is that all the information that you need in these early
three steps is stuff that is already meant to be part of DT.
This isn't surprising, because the bcm47xx serves a similar purpose
to DT, it's just more specialized.

This duplication is a bit unfortunate, but it seems that just using
the respective information from DT would be easier here.

Is any of the information you need early dynamic? It sounds that
for a given SoC, it should never change and can just be statically
encoded in DT.

> 4) At some point we need to register bcma devices, device_initcall can
> be used for that
> 
> As you can see, we need access to the NVRAM quite early (step 3,
> plat_time_init, or even earlier), but device_add (platform
> devices/drivers) is not available then yet. So I'm afraid we won't be
> able to use this common way to write NVRAM driver.
> 
> 
> So there I want to present my plan for the NVRAM improvements. If you
> don't agree with any part of it, or you can see any better solution,
> please speak up!
> 
> 1) I won't make nvram.c a platform driver. Instead I would like to
> make it less bcm47xx specific. I don't want to touch bcm47xx_bus in
> this file. Instead I want to add a generic function that will accept
> address and size of memory where NVRAM should be found. Then I'd like
> to move this file out of "mips" arch (drivers/misc/?
> drivers/bcma/nvram/?) and allow using it for bcm53xx.

In general, I'd try to avoid adding any platform specific code on ARM
when it needs to run as something other than a device driver.
Moving the code out of arch/mips and making it more generic definitely
sounds good to me, but I'd prefer to have an actual platform_driver
for it.

> 2) I was also thinking about cleaning bcm47xx init. Right now we do a
> lot of hacks in plat_mem_setup & bcma to register the bus and scan its
> cores. It's so early (before mm_init) that we can't alloc memory!
> Doing all this stuff slightly later (e.g. arch_init_irq) would allow
> us to simply use "kmalloc" and drop all current hacks in bcma.

This sounds good

> 3) Above change (point 2) would require some small change in bcma. We
> would need 2-stages init: detecting (with kmalloc!) bus cores,
> registering cores. This is required, because we can't register cores
> too early, device_add (and the underlying kobject) would oops/WARN in
> kobject_get.

Right. Could you do the bcma scan much later, at the time when
device_add works as well? Traditionally PCI has been a problem
since it had to be enabled really early, but that restriction
should be gone now, and we can actually probe it from a loadable
module.

A common exception is the console driver, and we have a number
of other cases where we do a minimal bus scan just to find the
serial console at very early boot, but then probe all other
devices from a regular initcall.

On a global level, there is another choice, which is to do something
similar to the 'pxa-impedence-matcher' and the 'sunxi-babelfish':
These are two projects that implement a last-stage boot loader that
runs before the kernel and translates a platform-specific boot format
into standard DTB format. We could do the same for bcm53xx and
translate any nvram strings we know about into properties of device
nodes we already have, and copy all remaining strings into a
properties of the /chosen node. That way, we don't even need any
nvram driver for ARM, except a trivial one that provides raw
write access to user space for updating it.

	Arnd

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

[Rafał Miłecki]

On 28 August 2014 12:13, Arnd Bergmann <arnd@arndb.de> wrote:
>> 1) prom_init / plat_mem_setup
>> These two functions are called in pretty much the same phase from the
>> setup_arch (arch/mips/kernel/setup.c).
>> Task: detect & register memory
>> Requires: CPU type, maybe Broadcom chip ID (highmem support)
>> Available: CPU type
>> Not available: kmalloc, device_add (kobject)
>>
>> 2) arch_init_irq
>> Called from the arch specific init_IRQ (arch/mips/kernel/irq.c)
>> Task: setup bcma's MIPS core
>> Requires: bcma bus MIPS core
>> Available: kmalloc
>> Not available: device_add (kobject)
>>
>> 3) plat_time_init
>> Called from the arch specific time_init (arch/mips/kernel/time.c)
>> Task: set frequency
>> Requires: bcma bus ChipCommon core, nvram
>> Available: kmalloc
>> Not available: device_add (kobject)
>
> My impression is that all the information that you need in these early
> three steps is stuff that is already meant to be part of DT.
> This isn't surprising, because the bcm47xx serves a similar purpose
> to DT, it's just more specialized.
>
> This duplication is a bit unfortunate, but it seems that just using
> the respective information from DT would be easier here.
>
> Is any of the information you need early dynamic? It sounds that
> for a given SoC, it should never change and can just be statically
> encoded in DT.

I'm not sure which info you exactly are talking about. I believe one
SoC model always use the same CPU, ChipCommon, embedded wireless and
PCIe + USB controllers. However amount of RAM, type of flash (serial
vs. NAND), size of flash, booting method, NVRAM location, etc. all
depend on vendor's choice. I think CPU speed could also depend on
vendor choice.


>> 4) At some point we need to register bcma devices, device_initcall can
>> be used for that
>>
>> As you can see, we need access to the NVRAM quite early (step 3,
>> plat_time_init, or even earlier), but device_add (platform
>> devices/drivers) is not available then yet. So I'm afraid we won't be
>> able to use this common way to write NVRAM driver.
>>
>>
>> So there I want to present my plan for the NVRAM improvements. If you
>> don't agree with any part of it, or you can see any better solution,
>> please speak up!
>>
>> 1) I won't make nvram.c a platform driver. Instead I would like to
>> make it less bcm47xx specific. I don't want to touch bcm47xx_bus in
>> this file. Instead I want to add a generic function that will accept
>> address and size of memory where NVRAM should be found. Then I'd like
>> to move this file out of "mips" arch (drivers/misc/?
>> drivers/bcma/nvram/?) and allow using it for bcm53xx.
>
> In general, I'd try to avoid adding any platform specific code on ARM
> when it needs to run as something other than a device driver.
> Moving the code out of arch/mips and making it more generic definitely
> sounds good to me, but I'd prefer to have an actual platform_driver
> for it.

Sure, I didn't want to add NVRAM driver into arch/arm/ :)

Can you see any solution for making NVRAM support a standard platform
driver on MIPS and ARM? As I said, on MIPS we need access to the NVRAM
really early, before platform devices/drivers can operate.


>> 3) Above change (point 2) would require some small change in bcma. We
>> would need 2-stages init: detecting (with kmalloc!) bus cores,
>> registering cores. This is required, because we can't register cores
>> too early, device_add (and the underlying kobject) would oops/WARN in
>> kobject_get.
>
> Right. Could you do the bcma scan much later, at the time when
> device_add works as well? Traditionally PCI has been a problem
> since it had to be enabled really early, but that restriction
> should be gone now, and we can actually probe it from a loadable
> module.

Take a look at "arch_init_irq" I described above. It needs access to
the MIPS core (bcma bus contains many cores). To get access to this
core (to know it exists and to get it mapped), I need to scan the bus.


> On a global level, there is another choice, which is to do something
> similar to the 'pxa-impedence-matcher' and the 'sunxi-babelfish':
> These are two projects that implement a last-stage boot loader that
> runs before the kernel and translates a platform-specific boot format
> into standard DTB format. We could do the same for bcm53xx and
> translate any nvram strings we know about into properties of device
> nodes we already have, and copy all remaining strings into a
> properties of the /chosen node. That way, we don't even need any
> nvram driver for ARM, except a trivial one that provides raw
> write access to user space for updating it.

I think on bcm53xx early access to the NVRAM is less important, so
this may be not such a big problem at all.

-- 
Rafał

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

[Arnd Bergmann]

On Thursday 28 August 2014 12:47:29 Rafał Miłecki wrote:
> On 28 August 2014 12:13, Arnd Bergmann <arnd@arndb.de> wrote:
> >> 1) prom_init / plat_mem_setup
> >> These two functions are called in pretty much the same phase from the
> >> setup_arch (arch/mips/kernel/setup.c).
> >> Task: detect & register memory
> >> Requires: CPU type, maybe Broadcom chip ID (highmem support)
> >> Available: CPU type
> >> Not available: kmalloc, device_add (kobject)
> >>
> >> 2) arch_init_irq
> >> Called from the arch specific init_IRQ (arch/mips/kernel/irq.c)
> >> Task: setup bcma's MIPS core
> >> Requires: bcma bus MIPS core
> >> Available: kmalloc
> >> Not available: device_add (kobject)
> >>
> >> 3) plat_time_init
> >> Called from the arch specific time_init (arch/mips/kernel/time.c)
> >> Task: set frequency
> >> Requires: bcma bus ChipCommon core, nvram
> >> Available: kmalloc
> >> Not available: device_add (kobject)
> >
> > My impression is that all the information that you need in these early
> > three steps is stuff that is already meant to be part of DT.
> > This isn't surprising, because the bcm47xx serves a similar purpose
> > to DT, it's just more specialized.
> >
> > This duplication is a bit unfortunate, but it seems that just using
> > the respective information from DT would be easier here.
> >
> > Is any of the information you need early dynamic? It sounds that
> > for a given SoC, it should never change and can just be statically
> > encoded in DT.
> 
> I'm not sure which info you exactly are talking about. I believe one
> SoC model always use the same CPU, ChipCommon, embedded wireless and
> PCIe + USB controllers. However amount of RAM, type of flash (serial
> vs. NAND), size of flash, booting method, NVRAM location, etc. all
> depend on vendor's choice. I think CPU speed could also depend on
> vendor choice.

But those would also be present in DT on ARM, right?

> >> 4) At some point we need to register bcma devices, device_initcall can
> >> be used for that
> >>
> >> As you can see, we need access to the NVRAM quite early (step 3,
> >> plat_time_init, or even earlier), but device_add (platform
> >> devices/drivers) is not available then yet. So I'm afraid we won't be
> >> able to use this common way to write NVRAM driver.
> >>
> >>
> >> So there I want to present my plan for the NVRAM improvements. If you
> >> don't agree with any part of it, or you can see any better solution,
> >> please speak up!
> >>
> >> 1) I won't make nvram.c a platform driver. Instead I would like to
> >> make it less bcm47xx specific. I don't want to touch bcm47xx_bus in
> >> this file. Instead I want to add a generic function that will accept
> >> address and size of memory where NVRAM should be found. Then I'd like
> >> to move this file out of "mips" arch (drivers/misc/?
> >> drivers/bcma/nvram/?) and allow using it for bcm53xx.
> >
> > In general, I'd try to avoid adding any platform specific code on ARM
> > when it needs to run as something other than a device driver.
> > Moving the code out of arch/mips and making it more generic definitely
> > sounds good to me, but I'd prefer to have an actual platform_driver
> > for it.
> 
> Sure, I didn't want to add NVRAM driver into arch/arm/ :)

What I meant is that I'd prefer to not even call a probe function
for this driver from arch/arm. I may have misunderstood what you meant
though.

> Can you see any solution for making NVRAM support a standard platform
> driver on MIPS and ARM? As I said, on MIPS we need access to the NVRAM
> really early, before platform devices/drivers can operate.

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.

> >> 3) Above change (point 2) would require some small change in bcma. We
> >> would need 2-stages init: detecting (with kmalloc!) bus cores,
> >> registering cores. This is required, because we can't register cores
> >> too early, device_add (and the underlying kobject) would oops/WARN in
> >> kobject_get.
> >
> > Right. Could you do the bcma scan much later, at the time when
> > device_add works as well? Traditionally PCI has been a problem
> > since it had to be enabled really early, but that restriction
> > should be gone now, and we can actually probe it from a loadable
> > module.
> 
> Take a look at "arch_init_irq" I described above. It needs access to
> the MIPS core (bcma bus contains many cores). To get access to this
> core (to know it exists and to get it mapped), I need to scan the bus.

I see. Still that would fit into the model of only using the
early probe on MIPS, but getting that information out of DT
on ARM, right? I would also expect the ARM version to use a
regular GIC only instead of the bcma irqchip, but I haven't
looked at that.

> > On a global level, there is another choice, which is to do something
> > similar to the 'pxa-impedence-matcher' and the 'sunxi-babelfish':
> > These are two projects that implement a last-stage boot loader that
> > runs before the kernel and translates a platform-specific boot format
> > into standard DTB format. We could do the same for bcm53xx and
> > translate any nvram strings we know about into properties of device
> > nodes we already have, and copy all remaining strings into a
> > properties of the /chosen node. That way, we don't even need any
> > nvram driver for ARM, except a trivial one that provides raw
> > write access to user space for updating it.
> 
> I think on bcm53xx early access to the NVRAM is less important, so
> this may be not such a big problem at all.

Ok.

	Arnd

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

[Rafał Miłecki]

On 28 August 2014 13:02, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 28 August 2014 12:47:29 Rafał Miłecki wrote:
>> On 28 August 2014 12:13, Arnd Bergmann <arnd@arndb.de> wrote:
>> > My impression is that all the information that you need in these early
>> > three steps is stuff that is already meant to be part of DT.
>> > This isn't surprising, because the bcm47xx serves a similar purpose
>> > to DT, it's just more specialized.
>> >
>> > This duplication is a bit unfortunate, but it seems that just using
>> > the respective information from DT would be easier here.
>> >
>> > Is any of the information you need early dynamic? It sounds that
>> > for a given SoC, it should never change and can just be statically
>> > encoded in DT.
>>
>> I'm not sure which info you exactly are talking about. I believe one
>> SoC model always use the same CPU, ChipCommon, embedded wireless and
>> PCIe + USB controllers. However amount of RAM, type of flash (serial
>> vs. NAND), size of flash, booting method, NVRAM location, etc. all
>> depend on vendor's choice. I think CPU speed could also depend on
>> vendor choice.
>
> But those would also be present in DT on ARM, right?

Well, that depends. Hauke was planning to put info about flash in DT.
Me on the other hand suggested reading info about flash from the
board. See my reply:
https://www.mail-archive.com/devicetree@vger.kernel.org/msg39365.html

My plan was to use patch like
[PATCH] bcma: get & store info about flash type SoC booted from
http://www.spinics.net/lists/linux-wireless/msg126163.html
(it would work on both: MIPS and ARM)
and then:

switch (boot_device) {
case BCMA_BOOT_DEV_NAND:
    nvram_address = 0x1000dead;
    break;
case BCMA_BOOT_DEV_SERIAL:
    nvram_address = 0x1000c0de;
    break;
}


>> >> 4) At some point we need to register bcma devices, device_initcall can
>> >> be used for that
>> >>
>> >> As you can see, we need access to the NVRAM quite early (step 3,
>> >> plat_time_init, or even earlier), but device_add (platform
>> >> devices/drivers) is not available then yet. So I'm afraid we won't be
>> >> able to use this common way to write NVRAM driver.
>> >>
>> >>
>> >> So there I want to present my plan for the NVRAM improvements. If you
>> >> don't agree with any part of it, or you can see any better solution,
>> >> please speak up!
>> >>
>> >> 1) I won't make nvram.c a platform driver. Instead I would like to
>> >> make it less bcm47xx specific. I don't want to touch bcm47xx_bus in
>> >> this file. Instead I want to add a generic function that will accept
>> >> address and size of memory where NVRAM should be found. Then I'd like
>> >> to move this file out of "mips" arch (drivers/misc/?
>> >> drivers/bcma/nvram/?) and allow using it for bcm53xx.
>> >
>> > In general, I'd try to avoid adding any platform specific code on ARM
>> > when it needs to run as something other than a device driver.
>> > Moving the code out of arch/mips and making it more generic definitely
>> > sounds good to me, but I'd prefer to have an actual platform_driver
>> > for it.
>>
>> Sure, I didn't want to add NVRAM driver into arch/arm/ :)
>
> What I meant is that I'd prefer to not even call a probe function
> for this driver from arch/arm. I may have misunderstood what you meant
> though.
>
>> Can you see any solution for making NVRAM support a standard platform
>> driver on MIPS and ARM? As I said, on MIPS we need access to the NVRAM
>> really early, before platform devices/drivers can operate.
>
> 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?

-- 
Rafał

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

[Arnd Bergmann]

On Thursday 28 August 2014 13:39:55 Rafał Miłecki wrote:
> On 28 August 2014 13:02, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Thursday 28 August 2014 12:47:29 Rafał Miłecki wrote:
> >> On 28 August 2014 12:13, Arnd Bergmann <arnd@arndb.de> wrote:
> >> > My impression is that all the information that you need in these early
> >> > three steps is stuff that is already meant to be part of DT.
> >> > This isn't surprising, because the bcm47xx serves a similar purpose
> >> > to DT, it's just more specialized.
> >> >
> >> > This duplication is a bit unfortunate, but it seems that just using
> >> > the respective information from DT would be easier here.
> >> >
> >> > Is any of the information you need early dynamic? It sounds that
> >> > for a given SoC, it should never change and can just be statically
> >> > encoded in DT.
> >>
> >> I'm not sure which info you exactly are talking about. I believe one
> >> SoC model always use the same CPU, ChipCommon, embedded wireless and
> >> PCIe + USB controllers. However amount of RAM, type of flash (serial
> >> vs. NAND), size of flash, booting method, NVRAM location, etc. all
> >> depend on vendor's choice. I think CPU speed could also depend on
> >> vendor choice.
> >
> > But those would also be present in DT on ARM, right?
> 
> Well, that depends. Hauke was planning to put info about flash in DT.
> Me on the other hand suggested reading info about flash from the
> board. See my reply:
> https://www.mail-archive.com/devicetree@vger.kernel.org/msg39365.html
> 
> My plan was to use patch like
> [PATCH] bcma: get & store info about flash type SoC booted from
> http://www.spinics.net/lists/linux-wireless/msg126163.html
> (it would work on both: MIPS and ARM)
> and then:
> 
> switch (boot_device) {
> case BCMA_BOOT_DEV_NAND:
>     nvram_address = 0x1000dead;
>     break;
> case BCMA_BOOT_DEV_SERIAL:
>     nvram_address = 0x1000c0de;
>     break;
> }
> 

I don't understand. Why does the nvram address depend on the boot
device?

> >> Can you see any solution for making NVRAM support a standard platform
> >> driver on MIPS and ARM? As I said, on MIPS we need access to the NVRAM
> >> really early, before platform devices/drivers can operate.
> >
> > 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?

	Arnd

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

[Rafał Miłecki]

On 28 August 2014 13:56, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 28 August 2014 13:39:55 Rafał Miłecki wrote:
>> On 28 August 2014 13:02, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Thursday 28 August 2014 12:47:29 Rafał Miłecki wrote:
>> >> On 28 August 2014 12:13, Arnd Bergmann <arnd@arndb.de> wrote:
>> >> > My impression is that all the information that you need in these early
>> >> > three steps is stuff that is already meant to be part of DT.
>> >> > This isn't surprising, because the bcm47xx serves a similar purpose
>> >> > to DT, it's just more specialized.
>> >> >
>> >> > This duplication is a bit unfortunate, but it seems that just using
>> >> > the respective information from DT would be easier here.
>> >> >
>> >> > Is any of the information you need early dynamic? It sounds that
>> >> > for a given SoC, it should never change and can just be statically
>> >> > encoded in DT.
>> >>
>> >> I'm not sure which info you exactly are talking about. I believe one
>> >> SoC model always use the same CPU, ChipCommon, embedded wireless and
>> >> PCIe + USB controllers. However amount of RAM, type of flash (serial
>> >> vs. NAND), size of flash, booting method, NVRAM location, etc. all
>> >> depend on vendor's choice. I think CPU speed could also depend on
>> >> vendor choice.
>> >
>> > But those would also be present in DT on ARM, right?
>>
>> Well, that depends. Hauke was planning to put info about flash in DT.
>> Me on the other hand suggested reading info about flash from the
>> board. See my reply:
>> https://www.mail-archive.com/devicetree@vger.kernel.org/msg39365.html
>>
>> My plan was to use patch like
>> [PATCH] bcma: get & store info about flash type SoC booted from
>> http://www.spinics.net/lists/linux-wireless/msg126163.html
>> (it would work on both: MIPS and ARM)
>> and then:
>>
>> switch (boot_device) {
>> case BCMA_BOOT_DEV_NAND:
>>     nvram_address = 0x1000dead;
>>     break;
>> case BCMA_BOOT_DEV_SERIAL:
>>     nvram_address = 0x1000c0de;
>>     break;
>> }
>>
>
> I don't understand. Why does the nvram address depend on the boot
> device?

NVRAM is basically just a partition on flash, however there are few
tricks applying to it.

To make booting possible, flash content is mapped to the memory. We're
talking about read only access. This mapping allows CPU to get code
(bootloader) and execute it as well as it allows CFE to get NVRAM
content easily. You don't need flash driver (with erasing & writing
support) to read NVRAM.
Depending on the boot flash device, content of flash is mapped at
different offsets:
1) MIPS serial flash: SI_FLASH2 (0x1c000000)
2) MIPS NAND flash: SI_FLASH1 (0x1fc00000)
3) ARM serial flash: SI_NS_NORFLASH (0x1e000000)
4) ARM NAND flash: SI_NS_NANDFLASH (0x1c000000)

So on my ARM device with serial flash (connected over SPI) I was able
to get NVRAM header this way:

void __iomem *iobase = ioremap_nocache(0x1e000000, 0x1000000);
u8 *buf;

buf = (u8 *)(iobase + 0xff0000);
pr_info("[NVRAM] %02X %02X %02X %02X\n", buf[0], buf[1], buf[2], buf[3]);

This resulted in:
[NVRAM] 46 4C 53 48

(I hardcoded 0xff0000 above, normally you would need to try 0x10000,
0x20000, 0x30000 and so on...).

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

[Arnd Bergmann]

On Thursday 28 August 2014 14:37:54 Rafał Miłecki wrote:
> On 28 August 2014 13:56, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Thursday 28 August 2014 13:39:55 Rafał Miłecki wrote:
> >> switch (boot_device) {
> >> case BCMA_BOOT_DEV_NAND:
> >>     nvram_address = 0x1000dead;
> >>     break;
> >> case BCMA_BOOT_DEV_SERIAL:
> >>     nvram_address = 0x1000c0de;
> >>     break;
> >> }
> >>
> >
> > I don't understand. Why does the nvram address depend on the boot
> > device?
> 
> NVRAM is basically just a partition on flash, however there are few
> tricks applying to it.

Ah, that explains a lot! I was thinking of hardware nvram, which I assume
it was on some early hardware.

> To make booting possible, flash content is mapped to the memory. We're
> talking about read only access. This mapping allows CPU to get code
> (bootloader) and execute it as well as it allows CFE to get NVRAM
> content easily. You don't need flash driver (with erasing & writing
> support) to read NVRAM.

Ok. Just out of curiosity, how does the system manage to map NAND
flash into physical address space? Is this a feature of the SoC
of the flash chip?

I guess for writing you'd still use the full MTD driver, right?

> Depending on the boot flash device, content of flash is mapped at
> different offsets:
> 1) MIPS serial flash: SI_FLASH2 (0x1c000000)
> 2) MIPS NAND flash: SI_FLASH1 (0x1fc00000)
> 3) ARM serial flash: SI_NS_NORFLASH (0x1e000000)
> 4) ARM NAND flash: SI_NS_NANDFLASH (0x1c000000)
> 
> So on my ARM device with serial flash (connected over SPI) I was able
> to get NVRAM header this way:
> 
> void __iomem *iobase = ioremap_nocache(0x1e000000, 0x1000000);
> u8 *buf;
> 
> buf = (u8 *)(iobase + 0xff0000);
> pr_info("[NVRAM] %02X %02X %02X %02X\n", buf[0], buf[1], buf[2], buf[3]);
> 
> This resulted in:
> [NVRAM] 46 4C 53 48
> 
> (I hardcoded 0xff0000 above, normally you would need to try 0x10000,
> 0x20000, 0x30000 and so on...).

Does that mean the entire 0x1e000000-0x1f000000 area is mapped to
the flash and you are looking for the nvram in it, or that you don't
know where it is?

	Arnd

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

[Rafał Miłecki]

On 28 August 2014 17:32, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 28 August 2014 14:37:54 Rafał Miłecki wrote:
>> To make booting possible, flash content is mapped to the memory. We're
>> talking about read only access. This mapping allows CPU to get code
>> (bootloader) and execute it as well as it allows CFE to get NVRAM
>> content easily. You don't need flash driver (with erasing & writing
>> support) to read NVRAM.
>
> Ok. Just out of curiosity, how does the system manage to map NAND
> flash into physical address space? Is this a feature of the SoC
> of the flash chip?

I don't know exactly. Many (all?) device with BCM4706 SoC have two
flashes. Serial flash (~2 MiB) with bootloader + nvram and NAND flash
with the firmware. However Netgear WNR3500Lv2 (based on BCM47186B0)
has only a NAND flash.


> I guess for writing you'd still use the full MTD driver, right?

That's right. This is why I wrote about "talking about read only access".


>> Depending on the boot flash device, content of flash is mapped at
>> different offsets:
>> 1) MIPS serial flash: SI_FLASH2 (0x1c000000)
>> 2) MIPS NAND flash: SI_FLASH1 (0x1fc00000)
>> 3) ARM serial flash: SI_NS_NORFLASH (0x1e000000)
>> 4) ARM NAND flash: SI_NS_NANDFLASH (0x1c000000)
>>
>> So on my ARM device with serial flash (connected over SPI) I was able
>> to get NVRAM header this way:
>>
>> void __iomem *iobase = ioremap_nocache(0x1e000000, 0x1000000);
>> u8 *buf;
>>
>> buf = (u8 *)(iobase + 0xff0000);
>> pr_info("[NVRAM] %02X %02X %02X %02X\n", buf[0], buf[1], buf[2], buf[3]);
>>
>> This resulted in:
>> [NVRAM] 46 4C 53 48
>>
>> (I hardcoded 0xff0000 above, normally you would need to try 0x10000,
>> 0x20000, 0x30000 and so on...).
>
> Does that mean the entire 0x1e000000-0x1f000000 area is mapped to
> the flash and you are looking for the nvram in it, or that you don't
> know where it is?

The correct algorithm would be:
for (off = 0; off < SOME_LIMIT; off += 0x10000) {
    buf = (u8 *)(iobase + off);
    if (buf[0] == 0x46 && buf[1] == 0x4C) {
        pr_info("NVRAM found at 0x%X offset\n", off);
        break;
    }
}

-- 
Rafał

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

[Rafał Miłecki]

On 28 August 2014 18:00, Rafał Miłecki <zajec5@gmail.com> wrote:
> On 28 August 2014 17:32, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Thursday 28 August 2014 14:37:54 Rafał Miłecki wrote:
>>> To make booting possible, flash content is mapped to the memory. We're
>>> talking about read only access. This mapping allows CPU to get code
>>> (bootloader) and execute it as well as it allows CFE to get NVRAM
>>> content easily. You don't need flash driver (with erasing & writing
>>> support) to read NVRAM.
>>
>> Ok. Just out of curiosity, how does the system manage to map NAND
>> flash into physical address space? Is this a feature of the SoC
>> of the flash chip?
>
> I don't know exactly. Many (all?) device with BCM4706 SoC have two
> flashes. Serial flash (~2 MiB) with bootloader + nvram and NAND flash
> with the firmware. However Netgear WNR3500Lv2 (based on BCM47186B0)
> has only a NAND flash.

Btw. since NAND flashes tend to be huhe, they can't be fully mapped
into memory. This is where Broadcom's "nfl_boot_size" comes in. This
is a function saying how much of NAND content it mapped into memory.
It returns NFL_BOOT_SIZE (0x200000) or NFL_BIG_BOOT_SIZE (0x800000)
depending on the block size.

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

[Hauke Mehrtens]

On 08/28/2014 01:56 PM, Arnd Bergmann wrote:
> On Thursday 28 August 2014 13:39:55 Rafał Miłecki wrote:
>> On 28 August 2014 13:02, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Thursday 28 August 2014 12:47:29 Rafał Miłecki wrote:
>>>> On 28 August 2014 12:13, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>> My impression is that all the information that you need in these early
>>>>> three steps is stuff that is already meant to be part of DT.
>>>>> This isn't surprising, because the bcm47xx serves a similar purpose
>>>>> to DT, it's just more specialized.
>>>>>
>>>>> This duplication is a bit unfortunate, but it seems that just using
>>>>> the respective information from DT would be easier here.
>>>>>
>>>>> Is any of the information you need early dynamic? It sounds that
>>>>> for a given SoC, it should never change and can just be statically
>>>>> encoded in DT.
>>>>
>>>> I'm not sure which info you exactly are talking about. I believe one
>>>> SoC model always use the same CPU, ChipCommon, embedded wireless and
>>>> PCIe + USB controllers. However amount of RAM, type of flash (serial
>>>> vs. NAND), size of flash, booting method, NVRAM location, etc. all
>>>> depend on vendor's choice. I think CPU speed could also depend on
>>>> vendor choice.
>>>
>>> But those would also be present in DT on ARM, right?
>>
>> Well, that depends. Hauke was planning to put info about flash in DT.
>> Me on the other hand suggested reading info about flash from the
>> board. See my reply:
>> https://www.mail-archive.com/devicetree@vger.kernel.org/msg39365.html
>>
>> My plan was to use patch like
>> [PATCH] bcma: get & store info about flash type SoC booted from
>> http://www.spinics.net/lists/linux-wireless/msg126163.html
>> (it would work on both: MIPS and ARM)
>> and then:
>>
>> switch (boot_device) {
>> case BCMA_BOOT_DEV_NAND:
>>     nvram_address = 0x1000dead;
>>     break;
>> case BCMA_BOOT_DEV_SERIAL:
>>     nvram_address = 0x1000c0de;
>>     break;
>> }
>>
> 
> I don't understand. Why does the nvram address depend on the boot
> device?
> 
>>>> Can you see any solution for making NVRAM support a standard platform
>>>> driver on MIPS and ARM? As I said, on MIPS we need access to the NVRAM
>>>> really early, before platform devices/drivers can operate.
>>>
>>> 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.  I do not think the arm
guys do like some board files containing the gpio numbers of the leds
and buttons found on the board.

For the MIPS version of BCM47xx we are able to automatically detect
mostly everything, just for the gpio configuration we try to guess the
board name based on nvram content and then configure the gpios.

The ARM BCM47xx contains a standard ARM with GIC and other standard arm
things just the flash, Ethernet, PCIe, USB controller and their
interconnection are Braodcom specific.

My plan was to provide a nvram and sprom driver which registers as a
normal platform device and supports device tree, like the one I posted
and it would also be possible to call the function with the address of
the flash directly, this function would be used for MIPS, this way we
can share the code and do not have to change the mips stuff so much.

For ARM BCM47xx we do not need bcma at all to boot the device, so it
should also work when bcma is build as a module, this is different to
MIPS. The ARM BCM47xx code currently in mainline Linux boots for me into
user space with an initramfs, it just misses many parts like Ethernet,
flash PCIe, ...

The address of the console is already hard coded in device tree. It
would also be possible to automatically detect their address based on
some description in the AIX bus (bcma), but I think hard coding the
address in device tree is easier.

Hauke

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

[Arnd Bergmann]

On Thursday 28 August 2014 23:22:35 Hauke Mehrtens wrote:
> 
> 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.  I do not think the arm
> guys do like some board files containing the gpio numbers of the leds
> and buttons found on the board.

Ok. The part I'm not sure about is how to best represent the nvram
in a way that matches the actual hardware.

If the two physical address ranges are just used for the purpose
of showing nvram, that would be fairly straightforward, and we
can jut put both of them in DT, mark them as 'status="disabled"'
by default and let the board specific file enable the one it needs.

However, if these registers really belong into the address range
that is owned by the flash controller device and that is behind
the bcma bus logic, things get a little tricky and we have to decide
whether we want to intentionally put a simplified (and incorrect)
description into the DT to make it easier to use, or to make
the description more correct at the expense of complicating the
code to detect it (thereby negating the intention of this hardware,
which is built to make it easier to boot).

> For the MIPS version of BCM47xx we are able to automatically detect
> mostly everything, just for the gpio configuration we try to guess the
> board name based on nvram content and then configure the gpios.

We could still do something like this with a boot wrapper that fills
the fields in the dtb from nvram data. We are pretty flexible in
the kernel when it comes to how that dtb is created, and there is no
requirement to have each board's dts file be part of the kernel sources
if there is some pre-kernel environment (firmware, boot loader,
wrapper, ...) that can generate it for us.

> The ARM BCM47xx contains a standard ARM with GIC and other standard arm
> things just the flash, Ethernet, PCIe, USB controller and their
> interconnection are Braodcom specific.

Ok.

> My plan was to provide a nvram and sprom driver which registers as a
> normal platform device and supports device tree, like the one I posted
> and it would also be possible to call the function with the address of
> the flash directly, this function would be used for MIPS, this way we
> can share the code and do not have to change the mips stuff so much.

Yes, and none of that should interfere with the cleanup plans for MIPS
that Rafał talked about, right?

> For ARM BCM47xx we do not need bcma at all to boot the device, so it
> should also work when bcma is build as a module, this is different to
> MIPS. The ARM BCM47xx code currently in mainline Linux boots for me into
> user space with an initramfs, it just misses many parts like Ethernet,
> flash PCIe, ...

Ah, good. So to confirm: all the essential devices including irqchip,
clocksource, uart and nvram can be accessed without using the bcma bus,
right?
Does that mean they are actually connected to another bus, or are they
actually bcma bus devices for which you provide an additional probe
method using dt/platform_device?

> The address of the console is already hard coded in device tree. It
> would also be possible to automatically detect their address based on
> some description in the AIX bus (bcma), but I think hard coding the
> address in device tree is easier.

Right. Importantly for the console, there are patches to allow a very
early output by having some minimal dt parsing done before start
accessing any other hardware. I think this is valuable even if it
means we compromise on the accurate DT description. We do the same
thing for consoles on other buses (ISAPnP, PCI, of_platform, ...)
and a lot of serial drivers have a way to retroactively connect that
early console setup to the actual device once it is probed normally.

	Arnd

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

[Rafał Miłecki]

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;
}

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.

-- 
Rafał

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

[Arnd Bergmann]

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;
	};	

	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

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

[Hauke Mehrtens]

On 08/29/2014 10:04 PM, 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.

I think we need a separate device tree description for every board
anyway (to specify the GPIO configuration) and then I do not see a big
problem specifying if this board boots from serial or nand flash.

>>
>> 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;
> 	};	
> 
> 	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".

This looks interesting.

But when we have an own device tree description for every board I do not
see many advantages over using status="disabled". Anyway we can share
all the common device tree description parts and  we can add the device
tree description after building the kernel, we are doing both for now.

Hauke

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

[Rafał Miłecki]

On 29 August 2014 22:04, Arnd Bergmann <arnd@arndb.de> wrote:
> 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;
>         };
>
>         nvram@1c000000 {
>                 compatible = "bcm,bcm4710-nvram";
>                 reg = <0x1e000000 0x1000000>;
>                 bcm,boot-device = BCMA_BOOT_DEV_SERIAL;
>         };

This sounds like a nice consensus for me! Actually it seems to be
similar to what we already do for other hardware parts.

E.g. in bcm4708.dtsi Hauke put registers location of 4 Ethernet cores
(gmac@0, gmac@1, gmac@2, gmac@3). I believe this board is ready for 4
Ethernet cores so DT matches hardware capabilities. Then most vendors
use/activate only one (maybe up to 2?) Ethernet cores. It's up to the
driver to detect if core is activated/used.

AFAIU having two flash mappings (as suggested above) would follow this
logic. It would match hardware capabilities. And then it would be up
to driver to detect which one mapping is really in use for this
particular board.

Does it make sense?

-- 
Rafał

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

[Rafał Miłecki]

On 31 August 2014 11:20, Rafał Miłecki <zajec5@gmail.com> wrote:
> On 29 August 2014 22:04, Arnd Bergmann <arnd@arndb.de> wrote:
>> 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;
>>         };
>>
>>         nvram@1c000000 {
>>                 compatible = "bcm,bcm4710-nvram";
>>                 reg = <0x1e000000 0x1000000>;
>>                 bcm,boot-device = BCMA_BOOT_DEV_SERIAL;
>>         };
>
> This sounds like a nice consensus for me! Actually it seems to be
> similar to what we already do for other hardware parts.
>
> E.g. in bcm4708.dtsi Hauke put registers location of 4 Ethernet cores
> (gmac@0, gmac@1, gmac@2, gmac@3). I believe this board is ready for 4
> Ethernet cores so DT matches hardware capabilities. Then most vendors
> use/activate only one (maybe up to 2?) Ethernet cores. It's up to the
> driver to detect if core is activated/used.
>
> AFAIU having two flash mappings (as suggested above) would follow this
> logic. It would match hardware capabilities. And then it would be up
> to driver to detect which one mapping is really in use for this
> particular board.
>
> Does it make sense?

I've just realized something. When Broadcom's code reads NVRAM content
it uses "hndnand_checkbadb" to skip bad blocks. It seems NVRAM doesn't
lay in 100% reliable flash sectors.

Above function comes from shared/ which (the directory) provides tons
of low level stuff without using any kernel API. OFC it won't be
acceptable for us to implement low level NAND stuff in the nvram
driver (this would mean duplicating NAND driver code). It seems we
won't be able to use NAND flash mapping to the memory (this magic
0x1c000000) at all...

So I think we'll need to change our vision of flash access in
bcm74xx_nvram driver. I guess we will have to:
1) Register NAND core early
2) Initialize NAND driver
3) Use mtd/nand API in bcm47xx_nvram

-- 
Rafał

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

[Arnd Bergmann]

On Monday 01 September 2014 09:48:48 Rafał Miłecki wrote:
> On 31 August 2014 11:20, Rafał Miłecki <zajec5@gmail.com> wrote:
> > On 29 August 2014 22:04, Arnd Bergmann <arnd@arndb.de> wrote:
> >> 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;
> >>         };
> >>
> >>         nvram@1c000000 {
> >>                 compatible = "bcm,bcm4710-nvram";
> >>                 reg = <0x1e000000 0x1000000>;
> >>                 bcm,boot-device = BCMA_BOOT_DEV_SERIAL;
> >>         };
> >
> > This sounds like a nice consensus for me! Actually it seems to be
> > similar to what we already do for other hardware parts.
> >
> > E.g. in bcm4708.dtsi Hauke put registers location of 4 Ethernet cores
> > (gmac@0, gmac@1, gmac@2, gmac@3). I believe this board is ready for 4
> > Ethernet cores so DT matches hardware capabilities. Then most vendors
> > use/activate only one (maybe up to 2?) Ethernet cores. It's up to the
> > driver to detect if core is activated/used.

Actually we normally list in the board-specific dts file which ones
are available on a particular machine.

> > AFAIU having two flash mappings (as suggested above) would follow this
> > logic. It would match hardware capabilities. And then it would be up
> > to driver to detect which one mapping is really in use for this
> > particular board.
> >
> > Does it make sense?

I mainly showed the example above for how it could be done, not saying
it's my preferred way. I think both Hauke and I said it would be betted
to do it explicitly in the dts file using the 'status="disabled"' property
instead of the 'brcm,boot-device = BCMA_BOOT_DEV_NAND' property.

For me this is mainly a question of whether we need a per-board
dts file or not. Hauke said he thinks we do need it, and (without
knowing anything about the platform), I would assume the same. If
we did not need that, and the nvram location was in fact the only
think we need to know, then the autoconfiguration based on the
brcm,boot-device property would become much more attractive.

> I've just realized something. When Broadcom's code reads NVRAM content
> it uses "hndnand_checkbadb" to skip bad blocks. It seems NVRAM doesn't
> lay in 100% reliable flash sectors.
> 
> Above function comes from shared/ which (the directory) provides tons
> of low level stuff without using any kernel API. OFC it won't be
> acceptable for us to implement low level NAND stuff in the nvram
> driver (this would mean duplicating NAND driver code). It seems we
> won't be able to use NAND flash mapping to the memory (this magic
> 0x1c000000) at all...

Hmm

> So I think we'll need to change our vision of flash access in
> bcm74xx_nvram driver. I guess we will have to:
> 1) Register NAND core early
> 2) Initialize NAND driver
> 3) Use mtd/nand API in bcm47xx_nvram

This would mean it's available really late. Is that a problem?

A possible solution for this would be to use the boot wrapper
I mentioned earlier, to put all the data from nvram into DT
properties before the kernel gets started.

	Arnd

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

[Jonas Gorski]

On Mon, Sep 1, 2014 at 4:57 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 01 September 2014 09:48:48 Rafał Miłecki wrote:
>> On 31 August 2014 11:20, Rafał Miłecki <zajec5@gmail.com> wrote:
>> So I think we'll need to change our vision of flash access in
>> bcm74xx_nvram driver. I guess we will have to:
>> 1) Register NAND core early
>> 2) Initialize NAND driver
>> 3) Use mtd/nand API in bcm47xx_nvram
>
> This would mean it's available really late. Is that a problem?

That's probably mostly fine (for MIPS), except for two places:
a) the kernel command line is stored in nvram, and used for finding
out the correct console tty.
b) on one specific chip, the configured system clock rate needs to be
read out from nvram.

Both can be also done through DT, but b) is somewhat important to do
right, as it will cause the time running fast/slow if the value is
wrong.

> A possible solution for this would be to use the boot wrapper
> I mentioned earlier, to put all the data from nvram into DT
> properties before the kernel gets started.

That sounds like quite a bit of effort, and a bit over-engineered for
just 2.5 platforms.


Jonas

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

[Arnd Bergmann]

On Monday 01 September 2014 22:45:25 Jonas Gorski wrote:
> On Mon, Sep 1, 2014 at 4:57 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Monday 01 September 2014 09:48:48 Rafał Miłecki wrote:
> >> On 31 August 2014 11:20, Rafał Miłecki <zajec5@gmail.com> wrote:
> >> So I think we'll need to change our vision of flash access in
> >> bcm74xx_nvram driver. I guess we will have to:
> >> 1) Register NAND core early
> >> 2) Initialize NAND driver
> >> 3) Use mtd/nand API in bcm47xx_nvram
> >
> > This would mean it's available really late. Is that a problem?
> 
> That's probably mostly fine (for MIPS), except for two places:
> a) the kernel command line is stored in nvram, and used for finding
> out the correct console tty.

Is this also the case on ARM? According to the documented boot protocol,
ARM systems are supposed to pass the command line either through the
ATAGS interface or through a DT, and we have code to move it from
the former into the latter one. Of course it wouldn't be the first
system that ignores the boot protocol, but it has fortunately become
rather rare these days.

> b) on one specific chip, the configured system clock rate needs to be
> read out from nvram.
> 
> Both can be also done through DT, but b) is somewhat important to do
> right, as it will cause the time running fast/slow if the value is
> wrong.

Can you have two systems that can use the same DTB with the exception
of the clock rate? This sounds no different than on any other system
that has a variable clock input.

	Arnd

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

[Florian Fainelli]

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
>