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

[Zhiqiang Hou]

Hi York,

Thanks for your comments!

> -----Original Message-----
> From: york sun
> Sent: 2016年7月21日 5:15
> 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/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.
> 

Will add comments to describe it.

Thanks,
Zhiqiang