[U-Boot] [PATCH 1/5] fsl: serdes: ensure accessing the initialized maps of serdes protocol
From: Hou Zhiqiang 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 isn't used for any device, so use it as the flag to indicate if the map has been initialized. Signed-off-by: Hou Zhiqiang --- Tested on LS1043ARDB, LS1021AQDS and T4240QDS boards. arch/arm/cpu/armv7/ls102xa/fsl_ls1_serdes.c | 9 + arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch2_serdes.c | 6 ++ arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c | 9 + arch/powerpc/cpu/mpc85xx/bsc9132_serdes.c| 6 ++ arch/powerpc/cpu/mpc85xx/c29x_serdes.c | 6 ++ arch/powerpc/cpu/mpc85xx/fsl_corenet2_serdes.c | 15 +++ arch/powerpc/cpu/mpc85xx/fsl_corenet_serdes.c| 6 ++ arch/powerpc/cpu/mpc85xx/mpc8536_serdes.c| 16 +++- arch/powerpc/cpu/mpc85xx/mpc8544_serdes.c| 16 +++- arch/powerpc/cpu/mpc85xx/mpc8548_serdes.c| 6 ++ arch/powerpc/cpu/mpc85xx/mpc8568_serdes.c| 6 ++ arch/powerpc/cpu/mpc85xx/mpc8569_serdes.c| 6 ++ arch/powerpc/cpu/mpc85xx/mpc8572_serdes.c| 6 ++ arch/powerpc/cpu/mpc85xx/p1010_serdes.c | 16 +++- arch/powerpc/cpu/mpc85xx/p1021_serdes.c | 6 ++ arch/powerpc/cpu/mpc85xx/p1022_serdes.c | 16 +++- arch/powerpc/cpu/mpc85xx/p1023_serdes.c | 9 - arch/powerpc/cpu/mpc85xx/p2020_serdes.c | 6 ++ arch/powerpc/cpu/mpc86xx/mpc8610_serdes.c| 16 +++- arch/powerpc/cpu/mpc86xx/mpc8641_serdes.c| 16 +++- 20 files changed, 191 insertions(+), 7 deletions(-) diff --git a/arch/arm/cpu/armv7/ls102xa/fsl_ls1_serdes.c b/arch/arm/cpu/armv7/ls102xa/fsl_ls1_serdes.c index 9b78acb..ffb05cb 100644 --- a/arch/arm/cpu/armv7/ls102xa/fsl_ls1_serdes.c +++ b/arch/arm/cpu/armv7/ls102xa/fsl_ls1_serdes.c @@ -23,9 +23,15 @@ int is_serdes_configured(enum srds_prtcl device) u64 ret = 0; #ifdef CONFIG_SYS_FSL_SRDS_1 + if (!(serdes1_prtcl_map & 1ULL << NONE)) + fsl_serdes_init(); + ret |= (1ULL << device) & serdes1_prtcl_map; #endif #ifdef CONFIG_SYS_FSL_SRDS_2 + if (!(serdes2_prtcl_map & 1ULL << NONE)) + fsl_serdes_init(); + ret |= (1ULL << device) & serdes2_prtcl_map; #endif @@ -87,6 +93,9 @@ u64 serdes_init(u32 sd, u32 sd_addr, u32 sd_prctl_mask, u32 sd_prctl_shift) serdes_prtcl_map |= (1ULL << lane_prtcl); } + /* Set the first bit to indicate serdes has been initialized */ + serdes_prtcl_map |= (1ULL << NONE); + return serdes_prtcl_map; } diff --git a/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch2_serdes.c b/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch2_serdes.c index fe3444a..fff80ef 100644 --- a/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch2_serdes.c +++ b/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch2_serdes.c @@ -19,6 +19,9 @@ int is_serdes_configured(enum srds_prtcl device) int ret = 0; #ifdef CONFIG_SYS_FSL_SRDS_1 + if (!serdes1_prtcl_map[NONE]) + fsl_serdes_init(); + ret |= serdes1_prtcl_map[device]; #endif @@ -103,6 +106,9 @@ void serdes_init(u32 sd, u32 sd_addr, u32 sd_prctl_mask, u32 sd_prctl_shift, else serdes_prtcl_map[lane_prtcl] = 1; } + + /* Set the first eliment to indicate serdes has been initialized */ + serdes_prtcl_map[NONE] = 1; } void fsl_serdes_init(void) diff --git a/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c b/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c index be6acc6..d83a073 100644 --- a/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c +++ b/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c @@ -28,9 +28,15 @@ int is_serdes_configured(enum srds_prtcl device) int ret = 0; #ifdef CONFIG_SYS_FSL_SRDS_1 + if (!serdes1_prtcl_map[NONE]) + fsl_serdes_init(); + ret |= serdes1_prtcl_map[device]; #endif #ifdef CONFIG_SYS_FSL_SRDS_2 + if (!serdes2_prtcl_map[NONE]) + fsl_serdes_init(); + ret |= serdes2_prtcl_map[device]; #endif @@ -136,6 +142,9 @@ void serdes_init(u32 sd, u32 sd_addr, u32 sd_prctl_mask, u32 sd_prctl_shift, #endif } } + + /* Set the first eliment to indicate serdes has been initialized */ + serdes_prtcl_map[NONE] = 1; } void fsl_serdes_init(void) diff --git a/arch/powerpc/cpu/mpc85xx/bsc9132_serdes.c b/arch/powerpc/cpu/mpc85xx/bsc9132_serdes.c index 399b208..414e1ea 100644 --- a/arch/powerpc/cpu/mpc85xx/bsc9132_serdes.c +++ b/arch/powerpc/cpu/mpc85xx/bsc9132_serdes.c @@ -68,6 +68,9 @@ static u8 serde
Re: [U-Boot] [PATCH 1/5] fsl: serdes: ensure accessing the initialized maps of serdes protocol
Hi All, Any comments? > -Original Message- > From: Zhiqiang Hou [mailto:zhiqiang@nxp.com] > Sent: 2016年7月4日 14:28 > To: u-boot@lists.denx.de; albert.u.b...@aribaud.net; york sun > ; w...@denx.de; Prabhakar Kushwaha > ; alison.w...@freescale.com; > mingkai...@freescale.com > Cc: yao.y...@freescale.com; qianyu.g...@freescale.com; > bmeng...@gmail.com; Shengzhou Liu ; Zhiqiang Hou > > Subject: [PATCH 1/5] fsl: serdes: ensure accessing the initialized maps of > serdes > protocol > > From: Hou Zhiqiang > > 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 isn't used for any > device, > so use it as the flag to indicate if the map has been initialized. > > Signed-off-by: Hou Zhiqiang > --- > Tested on LS1043ARDB, LS1021AQDS and T4240QDS boards. > > arch/arm/cpu/armv7/ls102xa/fsl_ls1_serdes.c | 9 + > arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch2_serdes.c | 6 ++ > arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c | 9 + > arch/powerpc/cpu/mpc85xx/bsc9132_serdes.c| 6 ++ > arch/powerpc/cpu/mpc85xx/c29x_serdes.c | 6 ++ > arch/powerpc/cpu/mpc85xx/fsl_corenet2_serdes.c | 15 +++ > arch/powerpc/cpu/mpc85xx/fsl_corenet_serdes.c| 6 ++ > arch/powerpc/cpu/mpc85xx/mpc8536_serdes.c| 16 +++- > arch/powerpc/cpu/mpc85xx/mpc8544_serdes.c| 16 +++- > arch/powerpc/cpu/mpc85xx/mpc8548_serdes.c| 6 ++ > arch/powerpc/cpu/mpc85xx/mpc8568_serdes.c| 6 ++ > arch/powerpc/cpu/mpc85xx/mpc8569_serdes.c| 6 ++ > arch/powerpc/cpu/mpc85xx/mpc8572_serdes.c| 6 ++ > arch/powerpc/cpu/mpc85xx/p1010_serdes.c | 16 +++- > arch/powerpc/cpu/mpc85xx/p1021_serdes.c | 6 ++ > arch/powerpc/cpu/mpc85xx/p1022_serdes.c | 16 +++- > arch/powerpc/cpu/mpc85xx/p1023_serdes.c | 9 - > arch/powerpc/cpu/mpc85xx/p2020_serdes.c | 6 ++ > arch/powerpc/cpu/mpc86xx/mpc8610_serdes.c| 16 +++- > arch/powerpc/cpu/mpc86xx/mpc8641_serdes.c| 16 +++- > 20 files changed, 191 insertions(+), 7 deletions(-) > > diff --git a/arch/arm/cpu/armv7/ls102xa/fsl_ls1_serdes.c > b/arch/arm/cpu/armv7/ls102xa/fsl_ls1_serdes.c > index 9b78acb..ffb05cb 100644 > --- a/arch/arm/cpu/armv7/ls102xa/fsl_ls1_serdes.c > +++ b/arch/arm/cpu/armv7/ls102xa/fsl_ls1_serdes.c > @@ -23,9 +23,15 @@ int is_serdes_configured(enum srds_prtcl device) > u64 ret = 0; > > #ifdef CONFIG_SYS_FSL_SRDS_1 > + if (!(serdes1_prtcl_map & 1ULL << NONE)) > + fsl_serdes_init(); > + > ret |= (1ULL << device) & serdes1_prtcl_map; #endif #ifdef > CONFIG_SYS_FSL_SRDS_2 > + if (!(serdes2_prtcl_map & 1ULL << NONE)) > + fsl_serdes_init(); > + > ret |= (1ULL << device) & serdes2_prtcl_map; #endif > > @@ -87,6 +93,9 @@ u64 serdes_init(u32 sd, u32 sd_addr, u32 sd_prctl_mask, u32 > sd_prctl_shift) > serdes_prtcl_map |= (1ULL << lane_prtcl); > } > > + /* Set the first bit to indicate serdes has been initialized */ > + serdes_prtcl_map |= (1ULL << NONE); > + > return serdes_prtcl_map; > } > > diff --git a/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch2_serdes.c > b/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch2_serdes.c > index fe3444a..fff80ef 100644 > --- a/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch2_serdes.c > +++ b/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch2_serdes.c > @@ -19,6 +19,9 @@ int is_serdes_configured(enum srds_prtcl device) > int ret = 0; > > #ifdef CONFIG_SYS_FSL_SRDS_1 > + if (!serdes1_prtcl_map[NONE]) > + fsl_serdes_init(); > + > ret |= serdes1_prtcl_map[device]; > #endif > > @@ -103,6 +106,9 @@ void serdes_init(u32 sd, u32 sd_addr, u32 sd_prctl_mask, > u32 sd_prctl_shift, > else > serdes_prtcl_map[lane_prtcl] = 1; > } > + > + /* Set the first eliment to indicate serdes has been initialized */ > + serdes_prtcl_map[NONE] = 1; > } > > void fsl_serdes_init(void) > diff --git a/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c > b/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c > index be6acc6..d83a073 100644 > --- a/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c > +++ b/arch/arm/cpu/armv8/fsl-layerscape/fsl_lsch3_serdes.c > @@ -28,9 +28,15 @@ int is_serdes_configured(enum srds_prtcl device) > int ret = 0; > > #ifdef CONFIG_SYS_FSL_SRDS_1 > + if (!serdes1_prtcl_map[NONE]) > + fsl_serdes_init(); > + > ret |= serdes1_prtcl_map[device]; > #endif > #ifdef CONFIG_SYS_FSL_SRDS_2 > + if (!
Re: [U-Boot] [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 > > 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 > 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; 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. York ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/5] fsl: serdes: ensure accessing the initialized maps of serdes protocol
Hi York, Thanks for your comments! > -Original Message- > From: york sun > Sent: 2016年7月19日 23:46 > To: Zhiqiang Hou ; u-boot@lists.denx.de; > albert.u.b...@aribaud.net; w...@denx.de; Prabhakar Kushwaha > ; alison.w...@freescale.com; > mingkai...@freescale.com > Cc: yao.y...@freescale.com; qianyu.g...@freescale.com; > bmeng...@gmail.com; Shengzhou Liu > 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 > > > > 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? Thanks, Zhiqiang ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [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 ; u-boot@lists.denx.de; >> albert.u.b...@aribaud.net; w...@denx.de; Prabhakar Kushwaha >> ; alison.w...@freescale.com; >> mingkai...@freescale.com >> Cc: yao.y...@freescale.com; qianyu.g...@freescale.com; >> bmeng...@gmail.com; Shengzhou Liu >> 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 >>> >>> 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/5] fsl: serdes: ensure accessing the initialized maps of serdes protocol
Hi Zhiqiang, Sorry for late queries. As per description of patch " 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 isn't used for any device, so use it as the flag to indicate if the map has been initialized." I am just wondering the use-case/situation where this can happen. Can you please help me with understanding. fsl_serdes_init is called from arch_early_init_r in board_r.c. As per my understanding all the driver calling is_serdes_configed (SATA, PCIe, SGMII) etc requires DDR. So are we talking about moving any driver in board_f.c. Regards, Prabhakar > -Original Message- > From: york sun > Sent: Thursday, July 21, 2016 2:45 AM > To: Zhiqiang Hou ; u-boot@lists.denx.de; > albert.u.b...@aribaud.net; w...@denx.de; Prabhakar Kushwaha > ; alison.w...@freescale.com; > mingkai...@freescale.com > Cc: yao.y...@freescale.com; qianyu.g...@freescale.com; > bmeng...@gmail.com; Shengzhou Liu > 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 ; u-boot@lists.denx.de; > >> albert.u.b...@aribaud.net; w...@denx.de; Prabhakar Kushwaha > >> ; alison.w...@freescale.com; > >> mingkai...@freescale.com > >> Cc: yao.y...@freescale.com; qianyu.g...@freescale.com; > >> bmeng...@gmail.com; Shengzhou Liu > >> 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 > >>> > >>> 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/5] fsl: serdes: ensure accessing the initialized maps of serdes protocol
Hi Prabhakar, Thanks for your comments! > -Original Message- > From: Prabhakar Kushwaha > Sent: 2016年7月21日 12:28 > To: york sun ; Zhiqiang Hou ; u- > b...@lists.denx.de; albert.u.b...@aribaud.net; w...@denx.de; > alison.w...@freescale.com; mingkai...@freescale.com > Cc: yao.y...@freescale.com; qianyu.g...@freescale.com; > bmeng...@gmail.com; Shengzhou Liu > Subject: RE: [PATCH 1/5] fsl: serdes: ensure accessing the initialized maps > of serdes > protocol > > Hi Zhiqiang, > > Sorry for late queries. > > As per description of patch " 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 isn't used for any > device, > so use it as the flag to indicate if the map has been initialized." > > I am just wondering the use-case/situation where this can happen. > Can you please help me with understanding. > > fsl_serdes_init is called from arch_early_init_r in board_r.c. > As per my understanding all the driver calling is_serdes_configed (SATA, PCIe, > SGMII) etc requires DDR. > So are we talking about moving any driver in board_f.c. > No, there isn't any driver will be moved to board_f.c. There is a pcie errata that need modify the PCIE's field of CSU according to the current serdes protocol, I just want to reuse the existed serdes protocol parse code, but the workaround function must be called before the arch_early_init_r. Thanks, Zhiqiang ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/5] fsl: serdes: ensure accessing the initialized maps of serdes protocol
Hi York, Thanks for your comments! > -Original Message- > From: york sun > Sent: 2016年7月21日 5:15 > To: Zhiqiang Hou ; u-boot@lists.denx.de; > albert.u.b...@aribaud.net; w...@denx.de; Prabhakar Kushwaha > ; alison.w...@freescale.com; > mingkai...@freescale.com > Cc: yao.y...@freescale.com; qianyu.g...@freescale.com; > bmeng...@gmail.com; Shengzhou Liu > 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 ; u-boot@lists.denx.de; > >> albert.u.b...@aribaud.net; w...@denx.de; Prabhakar Kushwaha > >> ; alison.w...@freescale.com; > >> mingkai...@freescale.com > >> Cc: yao.y...@freescale.com; qianyu.g...@freescale.com; > >> bmeng...@gmail.com; Shengzhou Liu > >> 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 > >>> > >>> 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
