[U-Boot] [PATCH 1/5] fsl: serdes: ensure accessing the initialized maps of serdes protocol

[york sun]

On 07/20/2016 02:38 AM, Zhiqiang Hou wrote:
> Hi York,
>
> Thanks for your comments!
>
>> -----Original Message-----
>> From: york sun
>> Sent: 2016年7月19日 23:46
>> To: Zhiqiang Hou <zhiqiang.hou at nxp.com>; u-boot at lists.denx.de;
>> albert.u.boot at aribaud.net; wd at denx.de; Prabhakar Kushwaha
>> <prabhakar.kushwaha at nxp.com>; alison.wang at freescale.com;
>> Mingkai.Hu at freescale.com
>> Cc: yao.yuan at freescale.com; Qianyu.Gong at freescale.com;
>> bmeng.cn at gmail.com; Shengzhou Liu <shengzhou.liu at nxp.com>
>> Subject: Re: [PATCH 1/5] fsl: serdes: ensure accessing the initialized maps of serdes
>> protocol
>>
>> On 07/03/2016 11:39 PM, Zhiqiang Hou wrote:
>>> From: Hou Zhiqiang <Zhiqiang.Hou at nxp.com>
>>>
>>> Up to now, the function is_serdes_configed() doesn't check if the map
>>> of serdes protocol is initialized before accessing it. The function
>>> is_serdes_configed() will get wrong result when it was called before
>>> the serdes protocol maps initialized. As the first eliment of the map
>>
>> s/eliment/element
>
> Will fix my spelling mistakes next version, thanks a lot!
>
>>> isn't used for any device, so use it as the flag to indicate if the
>>> map has been initialized.
>>
>> I am not sure it is safe to presume the first element is always not used. Take
>> LS2080A as an example, the serdes map array is initialized by serdes_init(), which
>> calls serdes_get_prtcl() to get the index of the array. With normal condition, the
>> index shouldn't be zero. Zero is used as an error but it is not checked before setting
>>
>> serdes_prtcl_map[lane_prtcl] = 1;
>>
>
> The first element of enum srds_prtcl 'NONE' is aim to describe a lane isn't assigned to
> any device, and the array serdesn_prtcl_map is used to check if the specified device
> selected by the current serdes protocol, right? Nobody will check if the device 'NONE'
> has been configured, right? So it can be used to indicate if the serdes_prtcl_map has
> been initialized.
> Don't care whether the function serdes_get_prtcl() will return zero, just focus if the
> serdes protocol map has been filled. For example, if the serdes protocol value read from
> RCW doesn't match with any entry of the serdes_cfg_tbl, then all lane will be assigned to
> 'NONE'. It still means the serdes protocol map has been filled, even if there is only the
> serdesn_prtcl_map[NONE] was set to 1.
>
>> If you presumption is correct, and you want to use the first one as a flag, you
>> probably need to check all of them to make sure errors are handled correctly,
>> instead of setting the flag unexpected. Also it is important to document this idea
>> so future platform code follows the same.
>>
>
> Is it necessary to add it to a document, if add a comment to the serdesn_prtcl_map make it?
>

If you don't want to add a document, please at least put some comments, 
in case we need to change some code in the future.

York