All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] ad-hoc config error
@ 2016-09-20 21:22 york sun
  2016-09-20 21:36 ` Tom Rini
  0 siblings, 1 reply; 12+ messages in thread
From: york sun @ 2016-09-20 21:22 UTC (permalink / raw)
  To: u-boot

Tom and Simon,

After commit 371244cb19f9804711dd66e4281ff7979915fd2e, all merges with 
new macros defined will have the compiling error. How shall we fix it? 
Some macros can be added to Kconfig. But some are for local use, better 
than magic numbers. Adding them to the white-list doesn't sound right. 
What's your suggestion?

York

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [U-Boot] ad-hoc config error
  2016-09-20 21:22 [U-Boot] ad-hoc config error york sun
@ 2016-09-20 21:36 ` Tom Rini
  2016-09-20 21:40   ` york sun
  0 siblings, 1 reply; 12+ messages in thread
From: Tom Rini @ 2016-09-20 21:36 UTC (permalink / raw)
  To: u-boot

On Tue, Sep 20, 2016 at 09:22:09PM +0000, york sun wrote:

> Tom and Simon,
> 
> After commit 371244cb19f9804711dd66e4281ff7979915fd2e, all merges with 
> new macros defined will have the compiling error. How shall we fix it? 
> Some macros can be added to Kconfig. But some are for local use, better 
> than magic numbers. Adding them to the white-list doesn't sound right. 
> What's your suggestion?

Things that don't belong in Kconfig don't belong in the CONFIG namespace
either, probably.  For example, the cache line stuff is in Kconfig and
select'ed but for another example, various magic numbers used in the TI
secure boot stuff (which can vary from SoC to SoC) are just not in the
CONFIG namespace now.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160920/ecd7f22c/attachment.sig>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [U-Boot] ad-hoc config error
  2016-09-20 21:36 ` Tom Rini
@ 2016-09-20 21:40   ` york sun
  2016-09-20 22:30     ` Tom Rini
  0 siblings, 1 reply; 12+ messages in thread
From: york sun @ 2016-09-20 21:40 UTC (permalink / raw)
  To: u-boot

On 09/20/2016 02:36 PM, Tom Rini wrote:
> On Tue, Sep 20, 2016 at 09:22:09PM +0000, york sun wrote:
>
>> Tom and Simon,
>>
>> After commit 371244cb19f9804711dd66e4281ff7979915fd2e, all merges with
>> new macros defined will have the compiling error. How shall we fix it?
>> Some macros can be added to Kconfig. But some are for local use, better
>> than magic numbers. Adding them to the white-list doesn't sound right.
>> What's your suggestion?
>
> Things that don't belong in Kconfig don't belong in the CONFIG namespace
> either, probably.  For example, the cache line stuff is in Kconfig and
> select'ed but for another example, various magic numbers used in the TI
> secure boot stuff (which can vary from SoC to SoC) are just not in the
> CONFIG namespace now.
>

For those can be put in Kconfig, I can convert.
Can you point me examples to use macros for magic numbers?
What about the white listed macros? Are we supposed to leave them there, 
or slowly convert them to other name space?

York

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [U-Boot] ad-hoc config error
  2016-09-20 21:40   ` york sun
@ 2016-09-20 22:30     ` Tom Rini
  2016-09-21 15:22       ` york sun
  0 siblings, 1 reply; 12+ messages in thread
From: Tom Rini @ 2016-09-20 22:30 UTC (permalink / raw)
  To: u-boot

On Tue, Sep 20, 2016 at 09:40:00PM +0000, york sun wrote:
> On 09/20/2016 02:36 PM, Tom Rini wrote:
> > On Tue, Sep 20, 2016 at 09:22:09PM +0000, york sun wrote:
> >
> >> Tom and Simon,
> >>
> >> After commit 371244cb19f9804711dd66e4281ff7979915fd2e, all merges with
> >> new macros defined will have the compiling error. How shall we fix it?
> >> Some macros can be added to Kconfig. But some are for local use, better
> >> than magic numbers. Adding them to the white-list doesn't sound right.
> >> What's your suggestion?
> >
> > Things that don't belong in Kconfig don't belong in the CONFIG namespace
> > either, probably.  For example, the cache line stuff is in Kconfig and
> > select'ed but for another example, various magic numbers used in the TI
> > secure boot stuff (which can vary from SoC to SoC) are just not in the
> > CONFIG namespace now.
> >
> 
> For those can be put in Kconfig, I can convert.
> Can you point me examples to use macros for magic numbers?
> What about the white listed macros? Are we supposed to leave them there, 
> or slowly convert them to other name space?

Things on the whitelist should be converted, either to Kconfig or moved
out of the namespace.  Can you give me an example of something you
aren't sure how to convert?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160920/c7702535/attachment.sig>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [U-Boot] ad-hoc config error
  2016-09-20 22:30     ` Tom Rini
@ 2016-09-21 15:22       ` york sun
  2016-09-21 15:45         ` Tom Rini
  0 siblings, 1 reply; 12+ messages in thread
From: york sun @ 2016-09-21 15:22 UTC (permalink / raw)
  To: u-boot

On 09/20/2016 03:30 PM, Tom Rini wrote:
> On Tue, Sep 20, 2016 at 09:40:00PM +0000, york sun wrote:
>> On 09/20/2016 02:36 PM, Tom Rini wrote:
>>> On Tue, Sep 20, 2016 at 09:22:09PM +0000, york sun wrote:
>>>
>>>> Tom and Simon,
>>>>
>>>> After commit 371244cb19f9804711dd66e4281ff7979915fd2e, all merges with
>>>> new macros defined will have the compiling error. How shall we fix it?
>>>> Some macros can be added to Kconfig. But some are for local use, better
>>>> than magic numbers. Adding them to the white-list doesn't sound right.
>>>> What's your suggestion?
>>>
>>> Things that don't belong in Kconfig don't belong in the CONFIG namespace
>>> either, probably.  For example, the cache line stuff is in Kconfig and
>>> select'ed but for another example, various magic numbers used in the TI
>>> secure boot stuff (which can vary from SoC to SoC) are just not in the
>>> CONFIG namespace now.
>>>
>>
>> For those can be put in Kconfig, I can convert.
>> Can you point me examples to use macros for magic numbers?
>> What about the white listed macros? Are we supposed to leave them there,
>> or slowly convert them to other name space?
>
> Things on the whitelist should be converted, either to Kconfig or moved
> out of the namespace.  Can you give me an example of something you
> aren't sure how to convert?
>
For example,

CONFIG_SYS_DDR_MODE_1_1000
CONFIG_SYS_DDR_MODE_1_1200
CONFIG_SYS_DDR_MODE_1_1333
CONFIG_SYS_DDR_MODE_1_667
CONFIG_SYS_DDR_MODE_1_800
CONFIG_SYS_DDR_MODE_1_900

Those are DDR parameters defined per board if used. What will be proper 
names to convert to?

York

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [U-Boot] ad-hoc config error
  2016-09-21 15:22       ` york sun
@ 2016-09-21 15:45         ` Tom Rini
  2016-09-21 16:07           ` york sun
  0 siblings, 1 reply; 12+ messages in thread
From: Tom Rini @ 2016-09-21 15:45 UTC (permalink / raw)
  To: u-boot

On Wed, Sep 21, 2016 at 03:22:59PM +0000, york sun wrote:
> On 09/20/2016 03:30 PM, Tom Rini wrote:
> > On Tue, Sep 20, 2016 at 09:40:00PM +0000, york sun wrote:
> >> On 09/20/2016 02:36 PM, Tom Rini wrote:
> >>> On Tue, Sep 20, 2016 at 09:22:09PM +0000, york sun wrote:
> >>>
> >>>> Tom and Simon,
> >>>>
> >>>> After commit 371244cb19f9804711dd66e4281ff7979915fd2e, all merges with
> >>>> new macros defined will have the compiling error. How shall we fix it?
> >>>> Some macros can be added to Kconfig. But some are for local use, better
> >>>> than magic numbers. Adding them to the white-list doesn't sound right.
> >>>> What's your suggestion?
> >>>
> >>> Things that don't belong in Kconfig don't belong in the CONFIG namespace
> >>> either, probably.  For example, the cache line stuff is in Kconfig and
> >>> select'ed but for another example, various magic numbers used in the TI
> >>> secure boot stuff (which can vary from SoC to SoC) are just not in the
> >>> CONFIG namespace now.
> >>>
> >>
> >> For those can be put in Kconfig, I can convert.
> >> Can you point me examples to use macros for magic numbers?
> >> What about the white listed macros? Are we supposed to leave them there,
> >> or slowly convert them to other name space?
> >
> > Things on the whitelist should be converted, either to Kconfig or moved
> > out of the namespace.  Can you give me an example of something you
> > aren't sure how to convert?
> >
> For example,
> 
> CONFIG_SYS_DDR_MODE_1_1000
> CONFIG_SYS_DDR_MODE_1_1200
> CONFIG_SYS_DDR_MODE_1_1333
> CONFIG_SYS_DDR_MODE_1_667
> CONFIG_SYS_DDR_MODE_1_800
> CONFIG_SYS_DDR_MODE_1_900
> 
> Those are DDR parameters defined per board if used. What will be proper 
> names to convert to?

Poking at this a bit more, looking at say
board/freescale/corenet_ds/p4080ds_ddr.c (which I found grepping for
CONFIG_SYS_DDR_MODE_1_1200) reminds me of
arch/arm/cpu/armv7/am33xx/ddr.c and board/ti/am335x/board.c and no, I'm
not convinced that:
        .timing_cfg_0 = CONFIG_SYS_DDR_TIMING_0_800,
is more clear than:
        .timing_cfg_0 = 0xcc330104,

Especially since there's not a call back to a TRM/whatever that
describes the order of the fields for each register.  To me a link like
that is more descriptive.  I further assume that, after a bit more
grepping, these values, like the counterparts for am33xx are DDR chip
specific so I might go so far as to point at
arch/arm/include/asm/arch-omap3/mem.h where we have a series of
#define MEMORYVENDOR_MEMCTRL_FIELD 0xMAGIC
as if you re-use the part found on the ref board on your custom board
you can use (or at least start with) those values.

And yes, naming of memory controller related magic values is a mess and
is inconsistent even over the TI parts.  My first reaction is that the
am33xx stuff, which came later, worked out "better" than the omap3 way
did.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160921/b11e3f71/attachment.sig>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [U-Boot] ad-hoc config error
  2016-09-21 15:45         ` Tom Rini
@ 2016-09-21 16:07           ` york sun
  2016-09-21 19:01             ` Tom Rini
  0 siblings, 1 reply; 12+ messages in thread
From: york sun @ 2016-09-21 16:07 UTC (permalink / raw)
  To: u-boot

On 09/21/2016 08:45 AM, Tom Rini wrote:
> On Wed, Sep 21, 2016 at 03:22:59PM +0000, york sun wrote:
>> On 09/20/2016 03:30 PM, Tom Rini wrote:
>>> On Tue, Sep 20, 2016 at 09:40:00PM +0000, york sun wrote:
>>>> On 09/20/2016 02:36 PM, Tom Rini wrote:
>>>>> On Tue, Sep 20, 2016 at 09:22:09PM +0000, york sun wrote:
>>>>>
>>>>>> Tom and Simon,
>>>>>>
>>>>>> After commit 371244cb19f9804711dd66e4281ff7979915fd2e, all merges with
>>>>>> new macros defined will have the compiling error. How shall we fix it?
>>>>>> Some macros can be added to Kconfig. But some are for local use, better
>>>>>> than magic numbers. Adding them to the white-list doesn't sound right.
>>>>>> What's your suggestion?
>>>>>
>>>>> Things that don't belong in Kconfig don't belong in the CONFIG namespace
>>>>> either, probably.  For example, the cache line stuff is in Kconfig and
>>>>> select'ed but for another example, various magic numbers used in the TI
>>>>> secure boot stuff (which can vary from SoC to SoC) are just not in the
>>>>> CONFIG namespace now.
>>>>>
>>>>
>>>> For those can be put in Kconfig, I can convert.
>>>> Can you point me examples to use macros for magic numbers?
>>>> What about the white listed macros? Are we supposed to leave them there,
>>>> or slowly convert them to other name space?
>>>
>>> Things on the whitelist should be converted, either to Kconfig or moved
>>> out of the namespace.  Can you give me an example of something you
>>> aren't sure how to convert?
>>>
>> For example,
>>
>> CONFIG_SYS_DDR_MODE_1_1000
>> CONFIG_SYS_DDR_MODE_1_1200
>> CONFIG_SYS_DDR_MODE_1_1333
>> CONFIG_SYS_DDR_MODE_1_667
>> CONFIG_SYS_DDR_MODE_1_800
>> CONFIG_SYS_DDR_MODE_1_900
>>
>> Those are DDR parameters defined per board if used. What will be proper
>> names to convert to?
>
> Poking at this a bit more, looking at say
> board/freescale/corenet_ds/p4080ds_ddr.c (which I found grepping for
> CONFIG_SYS_DDR_MODE_1_1200) reminds me of
> arch/arm/cpu/armv7/am33xx/ddr.c and board/ti/am335x/board.c and no, I'm
> not convinced that:
>         .timing_cfg_0 = CONFIG_SYS_DDR_TIMING_0_800,
> is more clear than:
>         .timing_cfg_0 = 0xcc330104,
>
> Especially since there's not a call back to a TRM/whatever that
> describes the order of the fields for each register.  To me a link like
> that is more descriptive.  I further assume that, after a bit more
> grepping, these values, like the counterparts for am33xx are DDR chip
> specific so I might go so far as to point at
> arch/arm/include/asm/arch-omap3/mem.h where we have a series of
> #define MEMORYVENDOR_MEMCTRL_FIELD 0xMAGIC
> as if you re-use the part found on the ref board on your custom board
> you can use (or at least start with) those values.
>
> And yes, naming of memory controller related magic values is a mess and
> is inconsistent even over the TI parts.  My first reaction is that the
> am33xx stuff, which came later, worked out "better" than the omap3 way
> did.
>

Tom,

If the macros are only used locally, we can replace them with the actual 
magic numbers. But even that is different from what we have been 
encouraging developers to avoid using magic numbers. I believe using 
macros makes the code more readable.

On the other hand, if we want to share a driver for multiple boards, the 
macros have advantage. See this patch 
http://patchwork.ozlabs.org/patch/663051/. This is the one I am trying 
to merge. The author abstracted the DDR init sequence and use macros so 
multiple boards can use the same driver. Each board only needs to define 
a set of macros. I think this use should be accepted. Do we simply 
remove the CONFIG_ from the macro names, or put them in a well-defined 
namespace?

York

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [U-Boot] ad-hoc config error
  2016-09-21 16:07           ` york sun
@ 2016-09-21 19:01             ` Tom Rini
  2016-09-21 19:10               ` york sun
  2016-09-23 16:42               ` york sun
  0 siblings, 2 replies; 12+ messages in thread
From: Tom Rini @ 2016-09-21 19:01 UTC (permalink / raw)
  To: u-boot

On Wed, Sep 21, 2016 at 04:07:49PM +0000, york sun wrote:
> On 09/21/2016 08:45 AM, Tom Rini wrote:
> > On Wed, Sep 21, 2016 at 03:22:59PM +0000, york sun wrote:
> >> On 09/20/2016 03:30 PM, Tom Rini wrote:
> >>> On Tue, Sep 20, 2016 at 09:40:00PM +0000, york sun wrote:
> >>>> On 09/20/2016 02:36 PM, Tom Rini wrote:
> >>>>> On Tue, Sep 20, 2016 at 09:22:09PM +0000, york sun wrote:
> >>>>>
> >>>>>> Tom and Simon,
> >>>>>>
> >>>>>> After commit 371244cb19f9804711dd66e4281ff7979915fd2e, all merges with
> >>>>>> new macros defined will have the compiling error. How shall we fix it?
> >>>>>> Some macros can be added to Kconfig. But some are for local use, better
> >>>>>> than magic numbers. Adding them to the white-list doesn't sound right.
> >>>>>> What's your suggestion?
> >>>>>
> >>>>> Things that don't belong in Kconfig don't belong in the CONFIG namespace
> >>>>> either, probably.  For example, the cache line stuff is in Kconfig and
> >>>>> select'ed but for another example, various magic numbers used in the TI
> >>>>> secure boot stuff (which can vary from SoC to SoC) are just not in the
> >>>>> CONFIG namespace now.
> >>>>>
> >>>>
> >>>> For those can be put in Kconfig, I can convert.
> >>>> Can you point me examples to use macros for magic numbers?
> >>>> What about the white listed macros? Are we supposed to leave them there,
> >>>> or slowly convert them to other name space?
> >>>
> >>> Things on the whitelist should be converted, either to Kconfig or moved
> >>> out of the namespace.  Can you give me an example of something you
> >>> aren't sure how to convert?
> >>>
> >> For example,
> >>
> >> CONFIG_SYS_DDR_MODE_1_1000
> >> CONFIG_SYS_DDR_MODE_1_1200
> >> CONFIG_SYS_DDR_MODE_1_1333
> >> CONFIG_SYS_DDR_MODE_1_667
> >> CONFIG_SYS_DDR_MODE_1_800
> >> CONFIG_SYS_DDR_MODE_1_900
> >>
> >> Those are DDR parameters defined per board if used. What will be proper
> >> names to convert to?
> >
> > Poking at this a bit more, looking at say
> > board/freescale/corenet_ds/p4080ds_ddr.c (which I found grepping for
> > CONFIG_SYS_DDR_MODE_1_1200) reminds me of
> > arch/arm/cpu/armv7/am33xx/ddr.c and board/ti/am335x/board.c and no, I'm
> > not convinced that:
> >         .timing_cfg_0 = CONFIG_SYS_DDR_TIMING_0_800,
> > is more clear than:
> >         .timing_cfg_0 = 0xcc330104,
> >
> > Especially since there's not a call back to a TRM/whatever that
> > describes the order of the fields for each register.  To me a link like
> > that is more descriptive.  I further assume that, after a bit more
> > grepping, these values, like the counterparts for am33xx are DDR chip
> > specific so I might go so far as to point at
> > arch/arm/include/asm/arch-omap3/mem.h where we have a series of
> > #define MEMORYVENDOR_MEMCTRL_FIELD 0xMAGIC
> > as if you re-use the part found on the ref board on your custom board
> > you can use (or at least start with) those values.
> >
> > And yes, naming of memory controller related magic values is a mess and
> > is inconsistent even over the TI parts.  My first reaction is that the
> > am33xx stuff, which came later, worked out "better" than the omap3 way
> > did.
> >
> 
> Tom,
> 
> If the macros are only used locally, we can replace them with the actual 
> magic numbers. But even that is different from what we have been 
> encouraging developers to avoid using magic numbers. I believe using 
> macros makes the code more readable.

So that would be the arch/arm/include/asm/arch-omap/mem.h case then.
That actually assembles the magic numbers by saying that for a given
memory chip from $VENDOR you need to set each of the fields thusly, and
then shifts them in to the places the various memory controller
registers want to them.

> On the other hand, if we want to share a driver for multiple boards, the 
> macros have advantage. See this patch 
> http://patchwork.ozlabs.org/patch/663051/. This is the one I am trying 
> to merge. The author abstracted the DDR init sequence and use macros so 
> multiple boards can use the same driver. Each board only needs to define 
> a set of macros. I think this use should be accepted

This would be the arch/arm/cpu/armv7/am33xx/ddr.c case.  A large number
of different boards and DDR chips work, I just pointed out one of them.
Or in NXP terms, arch/arm/cpu/armv7/mx6/ddr.c.

> Do we simply 
> remove the CONFIG_ from the macro names, or put them in a well-defined 
> namespace?

Very roughly, you should model the abstraction a bit differently.
board/freescale/ls1012afrdm/ls1012afrdm.c should call mmdc_init and pass
it a struct that contains the various DDR timing values and instead of:
/* DDR board-specific timing parameters */
It should say:
/*
 * We have a <VENDOR> <CHIP> for DDR.  The timing values are also however
 * board-specific, please see <TRM or tech note or whatever that NXP
 * advises customers to read>
 */
and then define them and put them into the struct.  Or just put them
into the struct.  I think that having drivers/ddr/fsl/fsl_mmdc.c be
compiled with the values is a step in the wrong direction as it makes
supporting multiple platforms with a single binary harder.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160921/d5d61df0/attachment.sig>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [U-Boot] ad-hoc config error
  2016-09-21 19:01             ` Tom Rini
@ 2016-09-21 19:10               ` york sun
  2016-09-23 16:42               ` york sun
  1 sibling, 0 replies; 12+ messages in thread
From: york sun @ 2016-09-21 19:10 UTC (permalink / raw)
  To: u-boot

On 09/21/2016 12:01 PM, Tom Rini wrote:
> On Wed, Sep 21, 2016 at 04:07:49PM +0000, york sun wrote:
>> On 09/21/2016 08:45 AM, Tom Rini wrote:
>>> On Wed, Sep 21, 2016 at 03:22:59PM +0000, york sun wrote:
>>>> On 09/20/2016 03:30 PM, Tom Rini wrote:
>>>>> On Tue, Sep 20, 2016 at 09:40:00PM +0000, york sun wrote:
>>>>>> On 09/20/2016 02:36 PM, Tom Rini wrote:
>>>>>>> On Tue, Sep 20, 2016 at 09:22:09PM +0000, york sun wrote:
>>>>>>>
>>>>>>>> Tom and Simon,
>>>>>>>>
>>>>>>>> After commit 371244cb19f9804711dd66e4281ff7979915fd2e, all merges with
>>>>>>>> new macros defined will have the compiling error. How shall we fix it?
>>>>>>>> Some macros can be added to Kconfig. But some are for local use, better
>>>>>>>> than magic numbers. Adding them to the white-list doesn't sound right.
>>>>>>>> What's your suggestion?
>>>>>>>
>>>>>>> Things that don't belong in Kconfig don't belong in the CONFIG namespace
>>>>>>> either, probably.  For example, the cache line stuff is in Kconfig and
>>>>>>> select'ed but for another example, various magic numbers used in the TI
>>>>>>> secure boot stuff (which can vary from SoC to SoC) are just not in the
>>>>>>> CONFIG namespace now.
>>>>>>>
>>>>>>
>>>>>> For those can be put in Kconfig, I can convert.
>>>>>> Can you point me examples to use macros for magic numbers?
>>>>>> What about the white listed macros? Are we supposed to leave them there,
>>>>>> or slowly convert them to other name space?
>>>>>
>>>>> Things on the whitelist should be converted, either to Kconfig or moved
>>>>> out of the namespace.  Can you give me an example of something you
>>>>> aren't sure how to convert?
>>>>>
>>>> For example,
>>>>
>>>> CONFIG_SYS_DDR_MODE_1_1000
>>>> CONFIG_SYS_DDR_MODE_1_1200
>>>> CONFIG_SYS_DDR_MODE_1_1333
>>>> CONFIG_SYS_DDR_MODE_1_667
>>>> CONFIG_SYS_DDR_MODE_1_800
>>>> CONFIG_SYS_DDR_MODE_1_900
>>>>
>>>> Those are DDR parameters defined per board if used. What will be proper
>>>> names to convert to?
>>>
>>> Poking at this a bit more, looking at say
>>> board/freescale/corenet_ds/p4080ds_ddr.c (which I found grepping for
>>> CONFIG_SYS_DDR_MODE_1_1200) reminds me of
>>> arch/arm/cpu/armv7/am33xx/ddr.c and board/ti/am335x/board.c and no, I'm
>>> not convinced that:
>>>         .timing_cfg_0 = CONFIG_SYS_DDR_TIMING_0_800,
>>> is more clear than:
>>>         .timing_cfg_0 = 0xcc330104,
>>>
>>> Especially since there's not a call back to a TRM/whatever that
>>> describes the order of the fields for each register.  To me a link like
>>> that is more descriptive.  I further assume that, after a bit more
>>> grepping, these values, like the counterparts for am33xx are DDR chip
>>> specific so I might go so far as to point at
>>> arch/arm/include/asm/arch-omap3/mem.h where we have a series of
>>> #define MEMORYVENDOR_MEMCTRL_FIELD 0xMAGIC
>>> as if you re-use the part found on the ref board on your custom board
>>> you can use (or at least start with) those values.
>>>
>>> And yes, naming of memory controller related magic values is a mess and
>>> is inconsistent even over the TI parts.  My first reaction is that the
>>> am33xx stuff, which came later, worked out "better" than the omap3 way
>>> did.
>>>
>>
>> Tom,
>>
>> If the macros are only used locally, we can replace them with the actual
>> magic numbers. But even that is different from what we have been
>> encouraging developers to avoid using magic numbers. I believe using
>> macros makes the code more readable.
>
> So that would be the arch/arm/include/asm/arch-omap/mem.h case then.
> That actually assembles the magic numbers by saying that for a given
> memory chip from $VENDOR you need to set each of the fields thusly, and
> then shifts them in to the places the various memory controller
> registers want to them.
>
>> On the other hand, if we want to share a driver for multiple boards, the
>> macros have advantage. See this patch
>> http://patchwork.ozlabs.org/patch/663051/. This is the one I am trying
>> to merge. The author abstracted the DDR init sequence and use macros so
>> multiple boards can use the same driver. Each board only needs to define
>> a set of macros. I think this use should be accepted
>
> This would be the arch/arm/cpu/armv7/am33xx/ddr.c case.  A large number
> of different boards and DDR chips work, I just pointed out one of them.
> Or in NXP terms, arch/arm/cpu/armv7/mx6/ddr.c.
>
>> Do we simply
>> remove the CONFIG_ from the macro names, or put them in a well-defined
>> namespace?
>
> Very roughly, you should model the abstraction a bit differently.
> board/freescale/ls1012afrdm/ls1012afrdm.c should call mmdc_init and pass
> it a struct that contains the various DDR timing values and instead of:
> /* DDR board-specific timing parameters */
> It should say:
> /*
>  * We have a <VENDOR> <CHIP> for DDR.  The timing values are also however
>  * board-specific, please see <TRM or tech note or whatever that NXP
>  * advises customers to read>
>  */
> and then define them and put them into the struct.  Or just put them
> into the struct.  I think that having drivers/ddr/fsl/fsl_mmdc.c be
> compiled with the values is a step in the wrong direction as it makes
> supporting multiple platforms with a single binary harder.
>

I like the struct idea better.

York

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [U-Boot] ad-hoc config error
  2016-09-21 19:01             ` Tom Rini
  2016-09-21 19:10               ` york sun
@ 2016-09-23 16:42               ` york sun
  2016-09-23 18:23                 ` Tom Rini
  1 sibling, 1 reply; 12+ messages in thread
From: york sun @ 2016-09-23 16:42 UTC (permalink / raw)
  To: u-boot

Tom,

Following this thread, I have different macros to resolve. Please take a 
look at this patch http://patchwork.ozlabs.org/patch/660787/.

It adds CONFIG_SYS_DCSR_RCPM_ADDR and CONFIG_SYS_GIC_ADDR. This kind of 
macros are used across platforms to define the base addresses. All 
existing macros are white-listed. Do we want to make a massive 
conversion to Kconfig, or convert them to a different name space? They 
are not config option, but system implementation. I believe we agreed to 
use CONFIG_SYS_* for this purpose.

I need to convert these two macros before requesting a pull.

York

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [U-Boot] ad-hoc config error
  2016-09-23 16:42               ` york sun
@ 2016-09-23 18:23                 ` Tom Rini
  2016-09-23 18:45                   ` york sun
  0 siblings, 1 reply; 12+ messages in thread
From: Tom Rini @ 2016-09-23 18:23 UTC (permalink / raw)
  To: u-boot

On Fri, Sep 23, 2016 at 04:42:24PM +0000, york sun wrote:
> Tom,
> 
> Following this thread, I have different macros to resolve. Please take a 
> look at this patch http://patchwork.ozlabs.org/patch/660787/.
> 
> It adds CONFIG_SYS_DCSR_RCPM_ADDR and CONFIG_SYS_GIC_ADDR. This kind of 
> macros are used across platforms to define the base addresses. All 
> existing macros are white-listed. Do we want to make a massive 
> conversion to Kconfig, or convert them to a different name space? They 
> are not config option, but system implementation. I believe we agreed to 
> use CONFIG_SYS_* for this purpose.
> 
> I need to convert these two macros before requesting a pull.

At what system level do these values change?  It feels like they should
probably mostly be moved to another namespace with perhaps
CONFIG_SYS_DCSRBAR and CONFIG_SYS_IMMR being expanded a little bit
name-wise and done via Kconfig.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160923/be3a377a/attachment.sig>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [U-Boot] ad-hoc config error
  2016-09-23 18:23                 ` Tom Rini
@ 2016-09-23 18:45                   ` york sun
  0 siblings, 0 replies; 12+ messages in thread
From: york sun @ 2016-09-23 18:45 UTC (permalink / raw)
  To: u-boot

On 09/23/2016 11:23 AM, Tom Rini wrote:
> On Fri, Sep 23, 2016 at 04:42:24PM +0000, york sun wrote:
>> Tom,
>>
>> Following this thread, I have different macros to resolve. Please take a
>> look at this patch http://patchwork.ozlabs.org/patch/660787/.
>>
>> It adds CONFIG_SYS_DCSR_RCPM_ADDR and CONFIG_SYS_GIC_ADDR. This kind of
>> macros are used across platforms to define the base addresses. All
>> existing macros are white-listed. Do we want to make a massive
>> conversion to Kconfig, or convert them to a different name space? They
>> are not config option, but system implementation. I believe we agreed to
>> use CONFIG_SYS_* for this purpose.
>>
>> I need to convert these two macros before requesting a pull.
>
> At what system level do these values change?  It feels like they should
> probably mostly be moved to another namespace with perhaps
> CONFIG_SYS_DCSRBAR and CONFIG_SYS_IMMR being expanded a little bit
> name-wise and done via Kconfig.
>

These two macros are for all current Freescale SoCs. We have a lot 
registers not used by U-Boot. When new features are added or enabled for 
an existing SoC, some registers need to be added. Those register base 
addresses are different on SoCs. Using Kconfig can solve this issue, but 
requires massive conversion on all existing macros. We may go down this 
path eventually. But in the interim, I think we can use a different name 
space to avoid triggering this compiling error.

York

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2016-09-23 18:45 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-20 21:22 [U-Boot] ad-hoc config error york sun
2016-09-20 21:36 ` Tom Rini
2016-09-20 21:40   ` york sun
2016-09-20 22:30     ` Tom Rini
2016-09-21 15:22       ` york sun
2016-09-21 15:45         ` Tom Rini
2016-09-21 16:07           ` york sun
2016-09-21 19:01             ` Tom Rini
2016-09-21 19:10               ` york sun
2016-09-23 16:42               ` york sun
2016-09-23 18:23                 ` Tom Rini
2016-09-23 18:45                   ` york sun

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.