| Message ID | 1501728117-11672-1-git-send-email-xiaowei.bao@nxp.com |
|---|---|
| State | Superseded |
| Delegated to: | York Sun |
| Headers | show |
On 08/02/2017 07:58 PM, Bao Xiaowei wrote: > modifiy the ls_pcie_link_up function, add the following three judging > mechanisms: > > detect state: return link down status; > L0 state: return link up status; > other state: delay about 100ms retrieve Status Returns the corresponding link > status; Please pay attention to your upper case and lower case in commit message. What does the spec say about the timeout? Is 100ms the right time? If so, please put a comment in the source code. > > Signed-off-by: Bao Xiaowei <xiaowei.bao@nxp.com> > --- > drivers/pci/pcie_layerscape.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/pcie_layerscape.c b/drivers/pci/pcie_layerscape.c > index 7565e2f..4446ac9 100644 > --- a/drivers/pci/pcie_layerscape.c > +++ b/drivers/pci/pcie_layerscape.c > @@ -65,13 +65,22 @@ static int ls_pcie_ltssm(struct ls_pcie *pcie) > > static int ls_pcie_link_up(struct ls_pcie *pcie) > { > - int ltssm; > + int ltssm, i; > > ltssm = ls_pcie_ltssm(pcie); > - if (ltssm < LTSSM_PCIE_L0) > + if ((ltssm == 0) || (ltssm == 1)) > return 0; > - > - return 1; > + else if (ltssm == LTSSM_PCIE_L0) > + return 1; > + else { > + for (i = 0; i < 100; i++) { > + udelay(1000); > + ltssm = ls_pcie_ltssm(pcie); > + if (ltssm == LTSSM_PCIE_L0) > + return 1; > + } > + return 0; > + } A comment to summary the logic would be nice. York
Hi York, I will pay attention to the case of the case in commit message. This patch is for some special reset times for longer pcie devices, in this case, the pcie device may on polling compliance state, the RC considers the pcie device is link up, but the pcie device is not link up, only the L0 state is link up state. So add the link up status judgement mechanisms. About 100ms timeout, the pcie spec does not specify the link up timeout time, and the link up state is determined by a state machine. The state machine implementation is relatively complex, refer to uboot of other platform pcie link up state to determine the realization of the mechanism, we evaluated a timeout, in detect state consider the pcie device is link down, in L0 state consider the pcie device is link up, within 100ms in other states can be restored to the L0 state considers the pcie device is link up . Thanks -----Original Message----- From: York Sun Sent: Tuesday, August 08, 2017 6:56 AM To: Xiaowei Bao <xiaowei.bao@nxp.com>; u-boot@lists.denx.de; Priyanka Jain <priyanka.jain@nxp.com>; Z.q. Hou <zhiqiang.hou@nxp.com>; M.h. Lian <minghuan.lian@nxp.com>; sjg@chromium.org Subject: Re: [PATCH] armv8: layerscape platform pcie link up state judgment strongly On 08/02/2017 07:58 PM, Bao Xiaowei wrote: > modifiy the ls_pcie_link_up function, add the following three judging > mechanisms: > > detect state: return link down status; > L0 state: return link up status; > other state: delay about 100ms retrieve Status Returns the > corresponding link status; Please pay attention to your upper case and lower case in commit message. What does the spec say about the timeout? Is 100ms the right time? If so, please put a comment in the source code. > > Signed-off-by: Bao Xiaowei <xiaowei.bao@nxp.com> > --- > drivers/pci/pcie_layerscape.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/pcie_layerscape.c > b/drivers/pci/pcie_layerscape.c index 7565e2f..4446ac9 100644 > --- a/drivers/pci/pcie_layerscape.c > +++ b/drivers/pci/pcie_layerscape.c > @@ -65,13 +65,22 @@ static int ls_pcie_ltssm(struct ls_pcie *pcie) > > static int ls_pcie_link_up(struct ls_pcie *pcie) > { > - int ltssm; > + int ltssm, i; > > ltssm = ls_pcie_ltssm(pcie); > - if (ltssm < LTSSM_PCIE_L0) > + if ((ltssm == 0) || (ltssm == 1)) > return 0; > - > - return 1; > + else if (ltssm == LTSSM_PCIE_L0) > + return 1; > + else { > + for (i = 0; i < 100; i++) { > + udelay(1000); > + ltssm = ls_pcie_ltssm(pcie); > + if (ltssm == LTSSM_PCIE_L0) > + return 1; > + } > + return 0; > + } A comment to summary the logic would be nice. York
On 08/07/2017 11:56 PM, Xiaowei Bao wrote: > Hi York, > > I will pay attention to the case of the case in commit message. > > This patch is for some special reset times for longer pcie devices, in this case, the pcie device may on polling compliance state, the RC considers the pcie device is link up, but the pcie device is not link up, only the L0 state is link up state. So add the link up status judgement mechanisms. > > About 100ms timeout, the pcie spec does not specify the link up timeout time, and the link up state is determined by a state machine. The state machine implementation is relatively complex, refer to uboot of other platform pcie link up state to determine the realization of the mechanism, we evaluated a timeout, in detect state consider the pcie device is link down, in L0 state consider the pcie device is link up, within 100ms in other states can be restored to the L0 state considers the pcie device is link up . Can you put this information to inline comment? It will help us when we read the code later. Thanks. York
Hi York, I will add the inline comment in the patch, send it to you later. thanks -----Original Message----- From: York Sun Sent: Wednesday, August 09, 2017 12:14 AM To: Xiaowei Bao <xiaowei.bao@nxp.com>; u-boot@lists.denx.de; Priyanka Jain <priyanka.jain@nxp.com>; Z.q. Hou <zhiqiang.hou@nxp.com>; M.h. Lian <minghuan.lian@nxp.com>; sjg@chromium.org Subject: Re: [PATCH] armv8: layerscape platform pcie link up state judgment strongly On 08/07/2017 11:56 PM, Xiaowei Bao wrote: > Hi York, > > I will pay attention to the case of the case in commit message. > > This patch is for some special reset times for longer pcie devices, in this case, the pcie device may on polling compliance state, the RC considers the pcie device is link up, but the pcie device is not link up, only the L0 state is link up state. So add the link up status judgement mechanisms. > > About 100ms timeout, the pcie spec does not specify the link up timeout time, and the link up state is determined by a state machine. The state machine implementation is relatively complex, refer to uboot of other platform pcie link up state to determine the realization of the mechanism, we evaluated a timeout, in detect state consider the pcie device is link down, in L0 state consider the pcie device is link up, within 100ms in other states can be restored to the L0 state considers the pcie device is link up . Can you put this information to inline comment? It will help us when we read the code later. Thanks. York
Hi York, I have add the inline comment in the new patch and have sent to you, please review it. Thanks -----Original Message----- From: Xiaowei Bao Sent: Friday, August 11, 2017 9:39 AM To: York Sun <york.sun@nxp.com>; u-boot@lists.denx.de; Priyanka Jain <priyanka.jain@nxp.com>; Z.q. Hou <zhiqiang.hou@nxp.com>; M.h. Lian <minghuan.lian@nxp.com>; sjg@chromium.org Subject: RE: [PATCH] armv8: layerscape platform pcie link up state judgment strongly Hi York, I will add the inline comment in the patch, send it to you later. thanks -----Original Message----- From: York Sun Sent: Wednesday, August 09, 2017 12:14 AM To: Xiaowei Bao <xiaowei.bao@nxp.com>; u-boot@lists.denx.de; Priyanka Jain <priyanka.jain@nxp.com>; Z.q. Hou <zhiqiang.hou@nxp.com>; M.h. Lian <minghuan.lian@nxp.com>; sjg@chromium.org Subject: Re: [PATCH] armv8: layerscape platform pcie link up state judgment strongly On 08/07/2017 11:56 PM, Xiaowei Bao wrote: > Hi York, > > I will pay attention to the case of the case in commit message. > > This patch is for some special reset times for longer pcie devices, in this case, the pcie device may on polling compliance state, the RC considers the pcie device is link up, but the pcie device is not link up, only the L0 state is link up state. So add the link up status judgement mechanisms. > > About 100ms timeout, the pcie spec does not specify the link up timeout time, and the link up state is determined by a state machine. The state machine implementation is relatively complex, refer to uboot of other platform pcie link up state to determine the realization of the mechanism, we evaluated a timeout, in detect state consider the pcie device is link down, in L0 state consider the pcie device is link up, within 100ms in other states can be restored to the L0 state considers the pcie device is link up . Can you put this information to inline comment? It will help us when we read the code later. Thanks. York
diff --git a/drivers/pci/pcie_layerscape.c b/drivers/pci/pcie_layerscape.c index 7565e2f..4446ac9 100644 --- a/drivers/pci/pcie_layerscape.c +++ b/drivers/pci/pcie_layerscape.c @@ -65,13 +65,22 @@ static int ls_pcie_ltssm(struct ls_pcie *pcie) static int ls_pcie_link_up(struct ls_pcie *pcie) { - int ltssm; + int ltssm, i; ltssm = ls_pcie_ltssm(pcie); - if (ltssm < LTSSM_PCIE_L0) + if ((ltssm == 0) || (ltssm == 1)) return 0; - - return 1; + else if (ltssm == LTSSM_PCIE_L0) + return 1; + else { + for (i = 0; i < 100; i++) { + udelay(1000); + ltssm = ls_pcie_ltssm(pcie); + if (ltssm == LTSSM_PCIE_L0) + return 1; + } + return 0; + } } static void ls_pcie_cfg0_set_busdev(struct ls_pcie *pcie, u32 busdev)
modifiy the ls_pcie_link_up function, add the following three judging mechanisms: detect state: return link down status; L0 state: return link up status; other state: delay about 100ms retrieve Status Returns the corresponding link status; Signed-off-by: Bao Xiaowei <xiaowei.bao@nxp.com> --- drivers/pci/pcie_layerscape.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)