[U-Boot] unused-const-variable warnings in FSL DDR driver

>>
>>> On 02/09/2017 02:32 AM, Thomas Schaefer wrote:
>>>> Hi York,
>>>>
>>>>
>>>>
>>>> When compiling latest u-boot with gcc 6.3 compiler, I get several 
>>>> 'unused-const-variable' warnings in options.c file of FSL DDR driver.
>>>> Affected variables are for (DIMM_SLOTS_PER_CTLR==2) configuration (e.g.
>>>> dual_0S[4]) and warnings could be fixed with the patch applied.
>>>>
>>>>
>>>>
>>>> What do you think?
>>>
>>> Thomas,
>>>
>>> Thanks for bringing it to my attention. I understand GCC 6 may have 
>>> more warnings. The proposed patch is OK in logic but it increases the 
>>> size of code unnecessarily. Can you come up with a different fix?
>>>
>>> I can come back to check after I finish my work on hand.
>>>
>>> York
>>
>> Hi York,
>>
>> not sure if I understand this correctly, but why is code size 
>> increased when these variables are not defined? I think code size is 
>> decreased instead as these variables are no longer defined if not needed.
>>
>> I also don't see a way to achieve this in a different way as the 
>> variables are defined differently for DDR2, DDR3 and DDR4.
>>
>>

>Wait a minute, did you generate the patch backward? Your patch shows
>removing "#if CONFIG_DIMM_SLOTS_PER_CTLR==2" which doesn't exist in
>current code. If the logic is reversed, then yes, you are reducing the size. Can 
>GCC 6 optimize out the unused data? If yes, maybe we can use __maybe_unused
>to get rid of the warning?
>
>York

Oops, you are right, sorry for the confusion. Here's the corrected version:



Best regards,
Thomas

On 02/09/2017 09:46 AM, Thomas Schaefer wrote:
>>>
>>>> On 02/09/2017 02:32 AM, Thomas Schaefer wrote:
>>>>> Hi York,
>>>>>
>>>>>
>>>>>
>>>>> When compiling latest u-boot with gcc 6.3 compiler, I get several
>>>>> 'unused-const-variable' warnings in options.c file of FSL DDR driver.
>>>>> Affected variables are for (DIMM_SLOTS_PER_CTLR==2) configuration (e.g.
>>>>> dual_0S[4]) and warnings could be fixed with the patch applied.
>>>>>
>>>>>
>>>>>
>>>>> What do you think?
>>>>
>>>> Thomas,
>>>>
>>>> Thanks for bringing it to my attention. I understand GCC 6 may have
>>>> more warnings. The proposed patch is OK in logic but it increases the
>>>> size of code unnecessarily. Can you come up with a different fix?
>>>>
>>>> I can come back to check after I finish my work on hand.
>>>>
>>>> York
>>>
>>> Hi York,
>>>
>>> not sure if I understand this correctly, but why is code size
>>> increased when these variables are not defined? I think code size is
>>> decreased instead as these variables are no longer defined if not needed.
>>>
>>> I also don't see a way to achieve this in a different way as the
>>> variables are defined differently for DDR2, DDR3 and DDR4.
>>>
>>>
>
>> Wait a minute, did you generate the patch backward? Your patch shows
>> removing "#if CONFIG_DIMM_SLOTS_PER_CTLR==2" which doesn't exist in
>> current code. If the logic is reversed, then yes, you are reducing the size. Can
>> GCC 6 optimize out the unused data? If yes, maybe we can use __maybe_unused
>> to get rid of the warning?
>>
>> York
>
> Oops, you are right, sorry for the confusion. Here's the corrected version:
>
> diff --git a/drivers/ddr/fsl/options.c b/drivers/ddr/fsl/options.c
> index d6a8fcb216..d90ed0b6cc 100644
> --- a/drivers/ddr/fsl/options.c
> +++ b/drivers/ddr/fsl/options.c
> @@ -89,6 +89,7 @@ static const struct dynamic_odt single_S[4] = {
>         {0, 0, 0, 0},
>  };
>
> +#if (CONFIG_DIMM_SLOTS_PER_CTLR==2)
>  static const struct dynamic_odt dual_DD[4] = {
>         {       /* cs0 */
>                 FSL_DDR_ODT_NEVER,
> @@ -235,6 +236,7 @@ static const struct dynamic_odt dual_0S[4] = {
>         {0, 0, 0, 0}
>
>  };
> +#endif
>
>  static const struct dynamic_odt odt_unknown[4] = {
>         {       /* cs0 */
> @@ -319,6 +321,7 @@ static const struct dynamic_odt single_S[4] = {
>         {0, 0, 0, 0},
>  };
>
> +#if (CONFIG_DIMM_SLOTS_PER_CTLR==2)
>  static const struct dynamic_odt dual_DD[4] = {
>         {       /* cs0 */
>                 FSL_DDR_ODT_NEVER,
> @@ -465,6 +468,7 @@ static const struct dynamic_odt dual_0S[4] = {
>         {0, 0, 0, 0}
>
>  };
> +#endif
>
>  static const struct dynamic_odt odt_unknown[4] = {
>         {       /* cs0 */
> @@ -529,6 +533,7 @@ static const struct dynamic_odt single_S[4] = {
>         {0, 0, 0, 0},
>  };
>
> +#if (CONFIG_DIMM_SLOTS_PER_CTLR==2)
>  static const struct dynamic_odt dual_DD[4] = {
>         {       /* cs0 */
>                 FSL_DDR_ODT_OTHER_DIMM,
> @@ -676,6 +681,7 @@ static const struct dynamic_odt dual_0S[4] = {
>         {0, 0, 0, 0}
>
>  };
> +#endif
>
>  static const struct dynamic_odt odt_unknown[4] = {
>         {       /* cs0 */
>
>

This looks better. Can you check the size before and after this change? 
I wonder if GCC 6 can optimize out unused const. If it can, we may be 
able to get away by using __maybe_used and save a lot of #if.

By the way, please put space around "==" if you want to go this way.

York

On Thu, Feb 09, 2017 at 05:51:36PM +0000, york sun wrote:
> On 02/09/2017 09:46 AM, Thomas Schaefer wrote:
> >>>
> >>>> On 02/09/2017 02:32 AM, Thomas Schaefer wrote:
> >>>>> Hi York,
> >>>>>
> >>>>>
> >>>>>
> >>>>> When compiling latest u-boot with gcc 6.3 compiler, I get several
> >>>>> 'unused-const-variable' warnings in options.c file of FSL DDR driver.
> >>>>> Affected variables are for (DIMM_SLOTS_PER_CTLR==2) configuration (e.g.
> >>>>> dual_0S[4]) and warnings could be fixed with the patch applied.
> >>>>>
> >>>>>
> >>>>>
> >>>>> What do you think?
> >>>>
> >>>> Thomas,
> >>>>
> >>>> Thanks for bringing it to my attention. I understand GCC 6 may have
> >>>> more warnings. The proposed patch is OK in logic but it increases the
> >>>> size of code unnecessarily. Can you come up with a different fix?
> >>>>
> >>>> I can come back to check after I finish my work on hand.
> >>>>
> >>>> York
> >>>
> >>> Hi York,
> >>>
> >>> not sure if I understand this correctly, but why is code size
> >>> increased when these variables are not defined? I think code size is
> >>> decreased instead as these variables are no longer defined if not needed.
> >>>
> >>> I also don't see a way to achieve this in a different way as the
> >>> variables are defined differently for DDR2, DDR3 and DDR4.
> >>>
> >>>
> >
> >> Wait a minute, did you generate the patch backward? Your patch shows
> >> removing "#if CONFIG_DIMM_SLOTS_PER_CTLR==2" which doesn't exist in
> >> current code. If the logic is reversed, then yes, you are reducing the size. Can
> >> GCC 6 optimize out the unused data? If yes, maybe we can use __maybe_unused
> >> to get rid of the warning?
> >>
> >> York
> >
> > Oops, you are right, sorry for the confusion. Here's the corrected version:
> >
> > diff --git a/drivers/ddr/fsl/options.c b/drivers/ddr/fsl/options.c
> > index d6a8fcb216..d90ed0b6cc 100644
> > --- a/drivers/ddr/fsl/options.c
> > +++ b/drivers/ddr/fsl/options.c
> > @@ -89,6 +89,7 @@ static const struct dynamic_odt single_S[4] = {
> >         {0, 0, 0, 0},
> >  };
> >
> > +#if (CONFIG_DIMM_SLOTS_PER_CTLR==2)
> >  static const struct dynamic_odt dual_DD[4] = {
> >         {       /* cs0 */
> >                 FSL_DDR_ODT_NEVER,
> > @@ -235,6 +236,7 @@ static const struct dynamic_odt dual_0S[4] = {
> >         {0, 0, 0, 0}
> >
> >  };
> > +#endif
> >
> >  static const struct dynamic_odt odt_unknown[4] = {
> >         {       /* cs0 */
> > @@ -319,6 +321,7 @@ static const struct dynamic_odt single_S[4] = {
> >         {0, 0, 0, 0},
> >  };
> >
> > +#if (CONFIG_DIMM_SLOTS_PER_CTLR==2)
> >  static const struct dynamic_odt dual_DD[4] = {
> >         {       /* cs0 */
> >                 FSL_DDR_ODT_NEVER,
> > @@ -465,6 +468,7 @@ static const struct dynamic_odt dual_0S[4] = {
> >         {0, 0, 0, 0}
> >
> >  };
> > +#endif
> >
> >  static const struct dynamic_odt odt_unknown[4] = {
> >         {       /* cs0 */
> > @@ -529,6 +533,7 @@ static const struct dynamic_odt single_S[4] = {
> >         {0, 0, 0, 0},
> >  };
> >
> > +#if (CONFIG_DIMM_SLOTS_PER_CTLR==2)
> >  static const struct dynamic_odt dual_DD[4] = {
> >         {       /* cs0 */
> >                 FSL_DDR_ODT_OTHER_DIMM,
> > @@ -676,6 +681,7 @@ static const struct dynamic_odt dual_0S[4] = {
> >         {0, 0, 0, 0}
> >
> >  };
> > +#endif
> >
> >  static const struct dynamic_odt odt_unknown[4] = {
> >         {       /* cs0 */
> >
> >
> 
> This looks better. Can you check the size before and after this change? 
> I wonder if GCC 6 can optimize out unused const. If it can, we may be 
> able to get away by using __maybe_used and save a lot of #if.
> 
> By the way, please put space around "==" if you want to go this way.

The above isn't quite enough for all cases.  I'm testing a different and
larger patch currently.