[U-Boot] [PATCH 2/2] config.h: clean unused CONFIG_ENV_SPI_* if using driver model

[york sun]

On 07/27/2016 03:00 AM, Qianyu Gong wrote:
>
> Hi York,
>
>> -----Original Message-----
>> From: york sun
>> Sent: Tuesday, July 26, 2016 12:26 PM
>> To: Qianyu Gong <qianyu.gong at nxp.com>; u-boot at lists.denx.de; Prabhakar
>> Kushwaha <prabhakar.kushwaha at nxp.com>; Mingkai Hu <mingkai.hu at nxp.com>
>> Cc: Shaohui Xie <shaohui.xie at nxp.com>; Zhiqiang Hou <zhiqiang.hou at nxp.com>;
>> Wenbin Song <wenbin.song at nxp.com>
>> Subject: Re: [PATCH 2/2] config.h: clean unused CONFIG_ENV_SPI_* if using driver
>> model
>>
>> On 07/25/2016 09:05 PM, Qianyu Gong wrote:
>>> Hi York,
>>>
>>>
>>> As the drivel model is a trend anyway, I just doubt if it is necessary
>>> to support non-DM for the new platforms.
>>>
>>> In fact, we have discarded configurations for non-DM SPI such as SPI
>>> mode related macros
>>>
>>> when doing LS1043A upstream. So the current configuration of LS1043A
>>> doesn't support non-DM SPI.
>>>
>>>
>>> LS1012A supports both ways but the code doesn't differentiate the
>>> respective macros.
>>>
>>> The CONFIG_ENV_SPI_* are set for FMAN ucode at the beginning but I
>>> just find that LS1012A doesn't have FMAN. So it's dead code if using
>>> DM or just duplicated code that is the same with defines in
>>> common/env_sf.c if using non-DM.
>>
>> Qianyu,
>>
>> If DM_SPI_FLASH should always be set, please select it from Kconfig.
>>
>> York
>>
>>
>
> For LS1043A, DM_SPI_FLASH is still defined in include/configs/ls1043a_common.h.
> So I think it won't be affected by menuconfig. But it should have been moved to defconfig.
>
> As DM_SPI_FLASH doesn't depend on any platforms as per "drivers/mtd/spi/Kconfig",
> I can just focus on solving the issue caused by deselecting DM_SPI_FLASH. I also discussed
> with Yuan Yao.
>
> So how about I adding anything in Fman Kconfig like this?
> "
> config SYS_QE_FW_IN_SPIFLASH
>         depends on (FSL_LAYERSCAPE && DM_SPI_FLASH) || PPC
> "
> But as for the existing code, it may need more efforts.
>

I think you can add "select" for the platforms which always use 
DM_SPI_FLASH, for example TARGET_LS1043AQDS.

Simon,

Please comment if this is a good practice.

York

>
> Regards,
> Qianyu
>
>>>
>>>
>>>
>>> Regards,
>>>
>>> Qianyu
>>>
>>> ----------------------------------------------------------------------
>>> --
>>> *From:* york sun
>>> *Sent:* Tuesday, July 26, 2016 6:15:14 AM
>>> *To:* Qianyu Gong; u-boot at lists.denx.de; Prabhakar Kushwaha; Mingkai
>>> Hu
>>> *Cc:* Shaohui Xie; Zhiqiang Hou; Wenbin Song
>>> *Subject:* Re: [PATCH 2/2] config.h: clean unused CONFIG_ENV_SPI_* if
>>> using driver model
>>>
>>> On 07/20/2016 03:51 AM, Gong Qianyu wrote:
>>>> When using SPI driver model, it will get the values from DT. So there
>>>> is no need to set CONFIG_ENV_SPI_MAX_HZ and CONFIG_ENV_SPI_MODE any
>>>> more.
>>>>
>>>
>>> You indicate these macros are not needed _if_ using driver model. You
>>> presume the driver model is always used. You have CONFIG_DM_SPI_FLASH
>>> in defconfig, but you don't have it selected in Kconfig for those
>>> platforms. This can leave a possible configuration if one runs "make
>>> menuconfig" and deselect DM_SPI_FLASH.
>>>
>>> York
>>>
>>>
>>>> Signed-off-by: Gong Qianyu <Qianyu.Gong at nxp.com>
>>>> ---
>>>>  include/configs/ls1012a_common.h | 2 --
>>>> include/configs/ls1043a_common.h | 2 --
>>>>  2 files changed, 4 deletions(-)
>>>>
>>>> diff --git a/include/configs/ls1012a_common.h
>>>> b/include/configs/ls1012a_common.h
>>>> index fba2fac..1602f09 100644
>>>> --- a/include/configs/ls1012a_common.h
>>>> +++ b/include/configs/ls1012a_common.h
>>>> @@ -52,8 +52,6 @@
>>>>  #define CONFIG_SYS_FMAN_FW_ADDR              0x400d0000
>>>>  #define CONFIG_ENV_SPI_BUS           0
>>>>  #define CONFIG_ENV_SPI_CS            0
>>>> -#define CONFIG_ENV_SPI_MAX_HZ                1000000
>>>> -#define CONFIG_ENV_SPI_MODE          0x03
>>>>  #define CONFIG_SPI_FLASH_SPANSION
>>>>  #define CONFIG_FSL_SPI_INTERFACE
>>>>  #define CONFIG_SF_DATAFLASH
>>>> diff --git a/include/configs/ls1043a_common.h
>>>> b/include/configs/ls1043a_common.h
>>>> index b0d4a8d..028f7d9 100644
>>>> --- a/include/configs/ls1043a_common.h
>>>> +++ b/include/configs/ls1043a_common.h
>>>> @@ -222,8 +222,6 @@
>>>>  #define CONFIG_SYS_FMAN_FW_ADDR              0x400d0000
>>>>  #define CONFIG_ENV_SPI_BUS           0
>>>>  #define CONFIG_ENV_SPI_CS            0
>>>> -#define CONFIG_ENV_SPI_MAX_HZ                1000000
>>>> -#define CONFIG_ENV_SPI_MODE          0x03
>>>>  #else
>>>>  #define CONFIG_SYS_QE_FMAN_FW_IN_NOR
>>>>  /* FMan fireware Pre-load address */
>>>>
>>>
>
>