Hi York, Thanks for your comments!
> -----Original Message----- > From: york sun > Sent: 2016年7月21日 5:15 > To: Zhiqiang Hou <zhiqiang....@nxp.com>; u-boot@lists.denx.de; > albert.u.b...@aribaud.net; w...@denx.de; Prabhakar Kushwaha > <prabhakar.kushw...@nxp.com>; alison.w...@freescale.com; > mingkai...@freescale.com > Cc: yao.y...@freescale.com; qianyu.g...@freescale.com; > bmeng...@gmail.com; Shengzhou Liu <shengzhou....@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....@nxp.com>; u-boot@lists.denx.de; > >> albert.u.b...@aribaud.net; w...@denx.de; Prabhakar Kushwaha > >> <prabhakar.kushw...@nxp.com>; alison.w...@freescale.com; > >> mingkai...@freescale.com > >> Cc: yao.y...@freescale.com; qianyu.g...@freescale.com; > >> bmeng...@gmail.com; Shengzhou Liu <shengzhou....@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....@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
