| Message ID | 1573102695-7018-2-git-send-email-hayashi.kunihiko@socionext.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] PCI: uniphier: Set mode register to host mode | expand |
On Thu, Nov 07, 2019 at 01:58:15PM +0900, Kunihiko Hayashi wrote: > When PERST# is asserted once, EP configuration will be initialized. I don't quite understand this - does the EP/RC mode depend on how often PERST# is toggled? > If PERST# has been already deasserted, it isn't necessary to assert > here. What is the motativation for this patch? Is it to avoid a delay during boot, or to fix some bug? Isn't it nice to always reset the IP before use anyway? > > This checks whether PERST# is deasserted using PCL_PINMON register, > and adds omit controlling PERST#. Should this read 'and omits controlling PERST#'? > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > --- > drivers/pci/controller/dwc/pcie-uniphier.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c > index 8fd7bad..1ea4220 100644 > --- a/drivers/pci/controller/dwc/pcie-uniphier.c > +++ b/drivers/pci/controller/dwc/pcie-uniphier.c > @@ -22,6 +22,9 @@ > > #include "pcie-designware.h" > > +#define PCL_PINMON 0x0028 > +#define PCL_PINMON_PERST_OUT BIT(16) > + > #define PCL_PINCTRL0 0x002c > #define PCL_PERST_PLDN_REGEN BIT(12) > #define PCL_PERST_NOE_REGEN BIT(11) > @@ -100,6 +103,11 @@ static void uniphier_pcie_init_rc(struct uniphier_pcie_priv *priv) > val |= PCL_SYS_AUX_PWR_DET; > writel(val, priv->base + PCL_APP_PM0); > > + /* return if PERST# is already deasserted */ This comment just describes what the following line does which may be self-explanatory, perhaps a better comment would describe why we avoid a reset. Thanks, Andrew Murray > + val = readl(priv->base + PCL_PINMON); > + if (val & PCL_PINMON_PERST_OUT) > + return; > + > /* assert PERST# */ > val = readl(priv->base + PCL_PINCTRL0); > val &= ~(PCL_PERST_NOE_REGVAL | PCL_PERST_OUT_REGVAL > -- > 2.7.4 >
Hi Andrew, Thank you for your comments. On Thu, 7 Nov 2019 10:02:08 +0000 <andrew.murray@arm.com> wrote: > On Thu, Nov 07, 2019 at 01:58:15PM +0900, Kunihiko Hayashi wrote: > > When PERST# is asserted once, EP configuration will be initialized. > > I don't quite understand this - does the EP/RC mode depend on how often > PERST# is toggled? I think of connecting this RC controller and EP based on `Linux PCI endpoint framework' in another machine. While this RC driver is probing, the EP driver might be also probing and configurating itself using configfs. If PERST# is toggled after the EP has done its configuration, this configuration will be lost. I expect that the EP configurates after RC has toggled PERST#, however, there is no way to synchronize both of them. > > > If PERST# has been already deasserted, it isn't necessary to assert > > here. > > What is the motativation for this patch? Is it to avoid a delay during > boot, or to fix some bug? Isn't it nice to always reset the IP before > use anyway? Since EP device usually works without its configuration after PERST#, deassering PERST# here is no problem. Since bus reset breaks configuration of EP as shown above, bus reset should be done before EP has probed. Maybe boot sequence in host machine will do. > > > > > This checks whether PERST# is deasserted using PCL_PINMON register, > > and adds omit controlling PERST#. > > Should this read 'and omits controlling PERST#'? Yes, I'll fix it. > > > > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > > --- > > drivers/pci/controller/dwc/pcie-uniphier.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c > > index 8fd7bad..1ea4220 100644 > > --- a/drivers/pci/controller/dwc/pcie-uniphier.c > > +++ b/drivers/pci/controller/dwc/pcie-uniphier.c > > @@ -22,6 +22,9 @@ > > > > #include "pcie-designware.h" > > > > +#define PCL_PINMON 0x0028 > > +#define PCL_PINMON_PERST_OUT BIT(16) > > + > > #define PCL_PINCTRL0 0x002c > > #define PCL_PERST_PLDN_REGEN BIT(12) > > #define PCL_PERST_NOE_REGEN BIT(11) > > @@ -100,6 +103,11 @@ static void uniphier_pcie_init_rc(struct uniphier_pcie_priv *priv) > > val |= PCL_SYS_AUX_PWR_DET; > > writel(val, priv->base + PCL_APP_PM0); > > > > + /* return if PERST# is already deasserted */ > > This comment just describes what the following line does which may be > self-explanatory, perhaps a better comment would describe why we avoid > a reset. Indeed. I'll write the reason here. Thank you, --- Best Regards, Kunihiko Hayashi
On Thu, Nov 07, 2019 at 08:52:39PM +0900, Kunihiko Hayashi wrote: > Hi Andrew, > Thank you for your comments. > > On Thu, 7 Nov 2019 10:02:08 +0000 <andrew.murray@arm.com> wrote: > > > On Thu, Nov 07, 2019 at 01:58:15PM +0900, Kunihiko Hayashi wrote: > > > When PERST# is asserted once, EP configuration will be initialized. > > > > I don't quite understand this - does the EP/RC mode depend on how often > > PERST# is toggled? > > I think of connecting this RC controller and EP based on `Linux PCI > endpoint framework' in another machine. > > While this RC driver is probing, the EP driver might be also probing and > configurating itself using configfs. If PERST# is toggled after the EP > has done its configuration, this configuration will be lost. > > I expect that the EP configurates after RC has toggled PERST#, however, > there is no way to synchronize both of them. > OK I understand where you are coming from now. Please ensure the commit message gives this rationale. However, If I understand correctly, doesn't your solution only work some of the time? What happens if you boot both machines at the same time, and PERST# isn't asserted prior to the kernel booting? The only way you can ensure the EP is started after the RC is initialised is to start the EP after the RC is initialised. I'm not sure what the solution is here, but it feels like this approach only partially solves it. > > > > > > If PERST# has been already deasserted, it isn't necessary to assert > > > here. > > > > What is the motativation for this patch? Is it to avoid a delay during > > boot, or to fix some bug? Isn't it nice to always reset the IP before > > use anyway? > > Since EP device usually works without its configuration after PERST#, > deassering PERST# here is no problem. Since bus reset breaks configuration > of EP as shown above, bus reset should be done before EP has probed. > Maybe boot sequence in host machine will do. > > > > > > > > > > This checks whether PERST# is deasserted using PCL_PINMON register, > > > and adds omit controlling PERST#. > > > > Should this read 'and omits controlling PERST#'? > > Yes, I'll fix it. > > > > > > > > > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > > > --- > > > drivers/pci/controller/dwc/pcie-uniphier.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c > > > index 8fd7bad..1ea4220 100644 > > > --- a/drivers/pci/controller/dwc/pcie-uniphier.c > > > +++ b/drivers/pci/controller/dwc/pcie-uniphier.c > > > @@ -22,6 +22,9 @@ > > > > > > #include "pcie-designware.h" > > > > > > +#define PCL_PINMON 0x0028 > > > +#define PCL_PINMON_PERST_OUT BIT(16) > > > + > > > #define PCL_PINCTRL0 0x002c > > > #define PCL_PERST_PLDN_REGEN BIT(12) > > > #define PCL_PERST_NOE_REGEN BIT(11) > > > @@ -100,6 +103,11 @@ static void uniphier_pcie_init_rc(struct uniphier_pcie_priv *priv) > > > val |= PCL_SYS_AUX_PWR_DET; > > > writel(val, priv->base + PCL_APP_PM0); > > > > > > + /* return if PERST# is already deasserted */ > > > > This comment just describes what the following line does which may be > > self-explanatory, perhaps a better comment would describe why we avoid > > a reset. > > Indeed. I'll write the reason here. > Thanks, Andrew Murray > Thank you, > > --- > Best Regards, > Kunihiko Hayashi >
Hi Andrew, +CC Kishon On Thu, 7 Nov 2019 12:46:17 +0000 <andrew.murray@arm.com> wrote: > On Thu, Nov 07, 2019 at 08:52:39PM +0900, Kunihiko Hayashi wrote: > > Hi Andrew, > > Thank you for your comments. > > > > On Thu, 7 Nov 2019 10:02:08 +0000 <andrew.murray@arm.com> wrote: > > > > > On Thu, Nov 07, 2019 at 01:58:15PM +0900, Kunihiko Hayashi wrote: > > > > When PERST# is asserted once, EP configuration will be initialized. > > > > > > I don't quite understand this - does the EP/RC mode depend on how often > > > PERST# is toggled? > > > > I think of connecting this RC controller and EP based on `Linux PCI > > endpoint framework' in another machine. > > > > While this RC driver is probing, the EP driver might be also probing and > > configurating itself using configfs. If PERST# is toggled after the EP > > has done its configuration, this configuration will be lost. > > > > I expect that the EP configurates after RC has toggled PERST#, however, > > there is no way to synchronize both of them. > > > > OK I understand where you are coming from now. Please ensure the commit > message gives this rationale. I'll explain about that in the commit message next. > However, If I understand correctly, doesn't your solution only work some > of the time? What happens if you boot both machines at the same time, > and PERST# isn't asserted prior to the kernel booting? I think it contains an annoying problem. If PERST# isn't toggled prior to the kernel booting, PERST# remains asserted and the RC driver can't access PCI bus. As a result, this patch works and deasserts PERST# (and EP configuration will be lost). So boot sequence needs to include deasserting PERST#. > The only way you can ensure the EP is started after the RC is initialised > is to start the EP after the RC is initialised. Yes, it's the only soution for now. > I'm not sure what the solution is here, but it feels like this approach > only partially solves it. Surely relying on outside of the driver doesn't seem to be a complete solution. If there is the way that `Linux PCI endpoint framework' assumes, I'd like to follow it, however, I can't find the other way. Thank you, --- Best Regards, Kunihiko Hayashi
On Fri, Nov 08, 2019 at 04:30:27PM +0900, Kunihiko Hayashi wrote: > Hi Andrew, > +CC Kishon > > On Thu, 7 Nov 2019 12:46:17 +0000 <andrew.murray@arm.com> wrote: > > > On Thu, Nov 07, 2019 at 08:52:39PM +0900, Kunihiko Hayashi wrote: > > > Hi Andrew, > > > Thank you for your comments. > > > > > > On Thu, 7 Nov 2019 10:02:08 +0000 <andrew.murray@arm.com> wrote: > > > > > > > On Thu, Nov 07, 2019 at 01:58:15PM +0900, Kunihiko Hayashi wrote: > > > > > When PERST# is asserted once, EP configuration will be initialized. > > > > > > > > I don't quite understand this - does the EP/RC mode depend on how often > > > > PERST# is toggled? > > > > > > I think of connecting this RC controller and EP based on `Linux PCI > > > endpoint framework' in another machine. > > > > > > While this RC driver is probing, the EP driver might be also probing and > > > configurating itself using configfs. If PERST# is toggled after the EP > > > has done its configuration, this configuration will be lost. > > > > > > I expect that the EP configurates after RC has toggled PERST#, however, > > > there is no way to synchronize both of them. > > > > > > > OK I understand where you are coming from now. Please ensure the commit > > message gives this rationale. > > I'll explain about that in the commit message next. > > > However, If I understand correctly, doesn't your solution only work some > > of the time? What happens if you boot both machines at the same time, > > and PERST# isn't asserted prior to the kernel booting? > > I think it contains an annoying problem. > > If PERST# isn't toggled prior to the kernel booting, PERST# remains asserted > and the RC driver can't access PCI bus. > > As a result, this patch works and deasserts PERST# (and EP configuration will > be lost). So boot sequence needs to include deasserting PERST#. > > > The only way you can ensure the EP is started after the RC is initialised > > is to start the EP after the RC is initialised. > > Yes, it's the only soution for now. > > > I'm not sure what the solution is here, but it feels like this approach > > only partially solves it. > > Surely relying on outside of the driver doesn't seem to be a complete solution. > > If there is the way that `Linux PCI endpoint framework' assumes, > I'd like to follow it, however, I can't find the other way. Indeed, this must be a common problem - many host controller drivers in the tree toggle perst on start up. Keen for any feedback from Kishon on this. Thanks, Andrew Murray > > Thank you, > > --- > Best Regards, > Kunihiko Hayashi > >
On Fri, Nov 08, 2019 at 04:30:27PM +0900, Kunihiko Hayashi wrote: > > However, If I understand correctly, doesn't your solution only work some > > of the time? What happens if you boot both machines at the same time, > > and PERST# isn't asserted prior to the kernel booting? > > I think it contains an annoying problem. > > If PERST# isn't toggled prior to the kernel booting, PERST# remains asserted > and the RC driver can't access PCI bus. > > As a result, this patch works and deasserts PERST# (and EP configuration will > be lost). So boot sequence needs to include deasserting PERST#. I am sorry but I have lost you. Can you explain to us why checking that PERST# is deasserted guarantees you that: - The EP has bootstrapped - It is safe not to toggle it again (and also skip uniphier_pcie_ltssm_enable()) Please provide details of the HW configuration so that we understand what's actually supposed to happen and why this patch fixes the issue you are facing. Thanks, Lorenzo
Hello Lorenzo, On Thu, 21 Nov 2019 16:47:05 +0000 <lorenzo.pieralisi@arm.com> wrote: > On Fri, Nov 08, 2019 at 04:30:27PM +0900, Kunihiko Hayashi wrote: > > > However, If I understand correctly, doesn't your solution only work some > > > of the time? What happens if you boot both machines at the same time, > > > and PERST# isn't asserted prior to the kernel booting? > > > > I think it contains an annoying problem. > > > > If PERST# isn't toggled prior to the kernel booting, PERST# remains asserted > > and the RC driver can't access PCI bus. > > > > As a result, this patch works and deasserts PERST# (and EP configuration will > > be lost). So boot sequence needs to include deasserting PERST#. > > I am sorry but I have lost you. Can you explain to us why checking > that PERST# is deasserted guarantees you that: > > - The EP has bootstrapped > - It is safe not to toggle it again (and also skip > uniphier_pcie_ltssm_enable()) > > Please provide details of the HW configuration so that we understand > what's actually supposed to happen and why this patch fixes the > issue you are facing. I tried to connect between the following boards, and do pci-epf-test: - "RC board": UniPhier ld20 board that has DWC RC controller - "EP board": UniPhier legacy board that has DWC EP controller This EP has power-on-state configuration, but it's necessary to set class ID, BAR sizes, etc. after starting up. In case of that starting up RC board before EP board, the RC driver can't establish link. So we need to boot EP board first. - EP/RC: power on both boards - EP: start up the kernel on EP board - EP: according to the following guide, configurate pci-epf-test https://www.kernel.org/doc/html/latest/PCI/endpoint/pci-test-howto.html - RC: start up the kernel on RC board At that time, because RC driver toggled PERST#, the EP configuration values are initialized to the power on state. After that, RC can't access EP collectly. I think there is a following solution: - EP/RC: power on both boards - RC: [deassert PERST# by boot firmware] - EP: start up the kernel on EP board - EP: configurate pci-epf-test - RC: start up the kernel on RC board [without toggling PERST# by this patch] Deasserting PERST# before EP configuration avoids the issue, however, this relies on boot firmware, so I think this isn't enough to solve the issue. Thank you, --- Best Regards, Kunihiko Hayashi
On Fri, 22 Nov 2019 20:53:16 +0900 <hayashi.kunihiko@socionext.com> wrote: > Hello Lorenzo, > > On Thu, 21 Nov 2019 16:47:05 +0000 <lorenzo.pieralisi@arm.com> wrote: > > > On Fri, Nov 08, 2019 at 04:30:27PM +0900, Kunihiko Hayashi wrote: > > > > However, If I understand correctly, doesn't your solution only work some > > > > of the time? What happens if you boot both machines at the same time, > > > > and PERST# isn't asserted prior to the kernel booting? > > > > > > I think it contains an annoying problem. > > > > > > If PERST# isn't toggled prior to the kernel booting, PERST# remains asserted > > > and the RC driver can't access PCI bus. > > > > > > As a result, this patch works and deasserts PERST# (and EP configuration will > > > be lost). So boot sequence needs to include deasserting PERST#. > > > > I am sorry but I have lost you. Can you explain to us why checking > > that PERST# is deasserted guarantees you that: > > > > - The EP has bootstrapped > > - It is safe not to toggle it again (and also skip > > uniphier_pcie_ltssm_enable()) > > > > Please provide details of the HW configuration so that we understand > > what's actually supposed to happen and why this patch fixes the > > issue you are facing. > > I tried to connect between the following boards, and do pci-epf-test: > - "RC board": UniPhier ld20 board that has DWC RC controller > - "EP board": UniPhier legacy board that has DWC EP controller > > This EP has power-on-state configuration, but it's necessary to set > class ID, BAR sizes, etc. after starting up. > > In case of that starting up RC board before EP board, the RC driver > can't establish link. So we need to boot EP board first. At that point, I've considered why RC can't establish link, and found that the waitng time was too short. - EP/RC: power on both boards - RC: start up the kernel on RC board - RC: wait for link up (long time enough) - EP: start up the kernel on EP board - EP: configurate pci-epf-test When the endpoint configuration is done and the EP driver enables LTSSM, the RC driver will quit from waiting for link up. Currently DWC RC driver calls dwc_pcie_wait_for_link(), however, the function tries to link up 10 times only, that is defined as LINK_WAIT_MAX_RETRIES in pcie-designware.h, it's too short to configurate the endpoint. Now the patch to bypass PERST# is not necessary. Instead for DWC RC drivers, I think that the number of retries should be changed according to the usage. And the same issue remains with other RC drivers. Thank you, --- Best Regards, Kunihiko Hayashi
Hi, On 04/12/19 3:35 pm, Kunihiko Hayashi wrote: > On Fri, 22 Nov 2019 20:53:16 +0900 <hayashi.kunihiko@socionext.com> wrote: > >> Hello Lorenzo, >> >> On Thu, 21 Nov 2019 16:47:05 +0000 <lorenzo.pieralisi@arm.com> wrote: >> >>> On Fri, Nov 08, 2019 at 04:30:27PM +0900, Kunihiko Hayashi wrote: >>>>> However, If I understand correctly, doesn't your solution only work some >>>>> of the time? What happens if you boot both machines at the same time, >>>>> and PERST# isn't asserted prior to the kernel booting? >>>> >>>> I think it contains an annoying problem. >>>> >>>> If PERST# isn't toggled prior to the kernel booting, PERST# remains asserted >>>> and the RC driver can't access PCI bus. >>>> >>>> As a result, this patch works and deasserts PERST# (and EP configuration will >>>> be lost). So boot sequence needs to include deasserting PERST#. >>> >>> I am sorry but I have lost you. Can you explain to us why checking >>> that PERST# is deasserted guarantees you that: >>> >>> - The EP has bootstrapped >>> - It is safe not to toggle it again (and also skip >>> uniphier_pcie_ltssm_enable()) >>> >>> Please provide details of the HW configuration so that we understand >>> what's actually supposed to happen and why this patch fixes the >>> issue you are facing. >> >> I tried to connect between the following boards, and do pci-epf-test: >> - "RC board": UniPhier ld20 board that has DWC RC controller >> - "EP board": UniPhier legacy board that has DWC EP controller >> >> This EP has power-on-state configuration, but it's necessary to set >> class ID, BAR sizes, etc. after starting up. >> >> In case of that starting up RC board before EP board, the RC driver >> can't establish link. So we need to boot EP board first. > > At that point, I've considered why RC can't establish link, > and found that the waitng time was too short. > > - EP/RC: power on both boards > > - RC: start up the kernel on RC board > > - RC: wait for link up (long time enough) > > - EP: start up the kernel on EP board > > - EP: configurate pci-epf-test > > When the endpoint configuration is done and the EP driver enables LTSSM, > the RC driver will quit from waiting for link up. > > Currently DWC RC driver calls dwc_pcie_wait_for_link(), however, > the function tries to link up 10 times only, that is defined > as LINK_WAIT_MAX_RETRIES in pcie-designware.h, it's too short > to configurate the endpoint. > > Now the patch to bypass PERST# is not necessary. > > Instead for DWC RC drivers, I think that the number of retries > should be changed according to the usage. > And the same issue remains with other RC drivers. If EP is configured using Linux, then PERST# cannot be used as it's difficult to boot linux and initialize EP within the specified time interval. Can't you prevent PERST from being propagated at all? Thanks Kishon
Hi Kishon, On Fri, 6 Dec 2019 12:28:29 +0530 <kishon@ti.com> wrote: > Hi, > > On 04/12/19 3:35 pm, Kunihiko Hayashi wrote: > > On Fri, 22 Nov 2019 20:53:16 +0900 <hayashi.kunihiko@socionext.com> wrote: > > >> Hello Lorenzo, > >> > >> On Thu, 21 Nov 2019 16:47:05 +0000 <lorenzo.pieralisi@arm.com> wrote: > >> > >>> On Fri, Nov 08, 2019 at 04:30:27PM +0900, Kunihiko Hayashi wrote: > >>>>> However, If I understand correctly, doesn't your solution only work some > >>>>> of the time? What happens if you boot both machines at the same time, > >>>>> and PERST# isn't asserted prior to the kernel booting? > >>>> > >>>> I think it contains an annoying problem. > >>>> > >>>> If PERST# isn't toggled prior to the kernel booting, PERST# remains asserted > >>>> and the RC driver can't access PCI bus. > >>>> > >>>> As a result, this patch works and deasserts PERST# (and EP configuration will > >>>> be lost). So boot sequence needs to include deasserting PERST#. > >>> > >>> I am sorry but I have lost you. Can you explain to us why checking > >>> that PERST# is deasserted guarantees you that: > >>> > >>> - The EP has bootstrapped > >>> - It is safe not to toggle it again (and also skip > >>> uniphier_pcie_ltssm_enable()) > >>> > >>> Please provide details of the HW configuration so that we understand > >>> what's actually supposed to happen and why this patch fixes the > >>> issue you are facing. > >> > >> I tried to connect between the following boards, and do pci-epf-test: > >> - "RC board": UniPhier ld20 board that has DWC RC controller > >> - "EP board": UniPhier legacy board that has DWC EP controller > >> > >> This EP has power-on-state configuration, but it's necessary to set > >> class ID, BAR sizes, etc. after starting up. > >> > >> In case of that starting up RC board before EP board, the RC driver > >> can't establish link. So we need to boot EP board first. > > > At that point, I've considered why RC can't establish link, > > and found that the waitng time was too short. > > > - EP/RC: power on both boards > > > - RC: start up the kernel on RC board > > > - RC: wait for link up (long time enough) > > > - EP: start up the kernel on EP board > > > - EP: configurate pci-epf-test > > > When the endpoint configuration is done and the EP driver enables LTSSM, > > the RC driver will quit from waiting for link up. > > > Currently DWC RC driver calls dwc_pcie_wait_for_link(), however, > > the function tries to link up 10 times only, that is defined > > as LINK_WAIT_MAX_RETRIES in pcie-designware.h, it's too short > > to configurate the endpoint. > > > Now the patch to bypass PERST# is not necessary. > > > Instead for DWC RC drivers, I think that the number of retries > > should be changed according to the usage. > > And the same issue remains with other RC drivers. > > If EP is configured using Linux, then PERST# cannot be used as it's difficult to boot linux and initialize EP within the specified time interval. Can't you prevent PERST from being propagated at all? Surely it might be difficult for RC to decide the time to wait for EP. Since RC almost toggles PERST# in boot time, I'd like to think about how to prevent from first PERST# at least. Thank you, --- Best Regards, Kunihiko Hayashi
Hi, On 06/12/19 2:28 pm, Kunihiko Hayashi wrote: > Hi Kishon, > > On Fri, 6 Dec 2019 12:28:29 +0530 <kishon@ti.com> wrote: > >> Hi, >> >> On 04/12/19 3:35 pm, Kunihiko Hayashi wrote: >>> On Fri, 22 Nov 2019 20:53:16 +0900 <hayashi.kunihiko@socionext.com> wrote: >>>>> Hello Lorenzo, >>>> >>>> On Thu, 21 Nov 2019 16:47:05 +0000 <lorenzo.pieralisi@arm.com> wrote: >>>> >>>>> On Fri, Nov 08, 2019 at 04:30:27PM +0900, Kunihiko Hayashi wrote: >>>>>>> However, If I understand correctly, doesn't your solution only work some >>>>>>> of the time? What happens if you boot both machines at the same time, >>>>>>> and PERST# isn't asserted prior to the kernel booting? >>>>>> >>>>>> I think it contains an annoying problem. >>>>>> >>>>>> If PERST# isn't toggled prior to the kernel booting, PERST# remains asserted >>>>>> and the RC driver can't access PCI bus. >>>>>> >>>>>> As a result, this patch works and deasserts PERST# (and EP configuration will >>>>>> be lost). So boot sequence needs to include deasserting PERST#. >>>>> >>>>> I am sorry but I have lost you. Can you explain to us why checking >>>>> that PERST# is deasserted guarantees you that: >>>>> >>>>> - The EP has bootstrapped >>>>> - It is safe not to toggle it again (and also skip >>>>> uniphier_pcie_ltssm_enable()) >>>>> >>>>> Please provide details of the HW configuration so that we understand >>>>> what's actually supposed to happen and why this patch fixes the >>>>> issue you are facing. >>>> >>>> I tried to connect between the following boards, and do pci-epf-test: >>>> - "RC board": UniPhier ld20 board that has DWC RC controller >>>> - "EP board": UniPhier legacy board that has DWC EP controller >>>> >>>> This EP has power-on-state configuration, but it's necessary to set >>>> class ID, BAR sizes, etc. after starting up. >>>> >>>> In case of that starting up RC board before EP board, the RC driver >>>> can't establish link. So we need to boot EP board first. >>>> At that point, I've considered why RC can't establish link, >>> and found that the waitng time was too short. >>>> - EP/RC: power on both boards >>>> - RC: start up the kernel on RC board >>>> - RC: wait for link up (long time enough) >>>> - EP: start up the kernel on EP board >>>> - EP: configurate pci-epf-test >>>> When the endpoint configuration is done and the EP driver enables LTSSM, >>> the RC driver will quit from waiting for link up. >>>> Currently DWC RC driver calls dwc_pcie_wait_for_link(), however, >>> the function tries to link up 10 times only, that is defined >>> as LINK_WAIT_MAX_RETRIES in pcie-designware.h, it's too short >>> to configurate the endpoint. >>>> Now the patch to bypass PERST# is not necessary. >>>> Instead for DWC RC drivers, I think that the number of retries >>> should be changed according to the usage. >>> And the same issue remains with other RC drivers. >> >> If EP is configured using Linux, then PERST# cannot be used as it's difficult to boot linux and initialize EP within the specified time interval. Can't you prevent PERST from being propagated at all? > > Surely it might be difficult for RC to decide the time to wait for EP. > Since RC almost toggles PERST# in boot time, I'd like to think about > how to prevent from first PERST# at least. It can be prevented in the HW (If that's possible). I modify the cable connecting RC and EP to not propagate PERST#. Thanks Kishon
Hi Kishon, On Fri, 6 Dec 2019 14:31:17 +0530 <kishon@ti.com> wrote: > Hi, > > On 06/12/19 2:28 pm, Kunihiko Hayashi wrote: > > Hi Kishon, > > > On Fri, 6 Dec 2019 12:28:29 +0530 <kishon@ti.com> wrote: > > >> Hi, > >> > >> On 04/12/19 3:35 pm, Kunihiko Hayashi wrote: > >>> On Fri, 22 Nov 2019 20:53:16 +0900 <hayashi.kunihiko@socionext.com> wrote: > >>>>> Hello Lorenzo, > >>>> > >>>> On Thu, 21 Nov 2019 16:47:05 +0000 <lorenzo.pieralisi@arm.com> wrote: > >>>> > >>>>> On Fri, Nov 08, 2019 at 04:30:27PM +0900, Kunihiko Hayashi wrote: > >>>>>>> However, If I understand correctly, doesn't your solution only work some > >>>>>>> of the time? What happens if you boot both machines at the same time, > >>>>>>> and PERST# isn't asserted prior to the kernel booting? > >>>>>> > >>>>>> I think it contains an annoying problem. > >>>>>> > >>>>>> If PERST# isn't toggled prior to the kernel booting, PERST# remains asserted > >>>>>> and the RC driver can't access PCI bus. > >>>>>> > >>>>>> As a result, this patch works and deasserts PERST# (and EP configuration will > >>>>>> be lost). So boot sequence needs to include deasserting PERST#. > >>>>> > >>>>> I am sorry but I have lost you. Can you explain to us why checking > >>>>> that PERST# is deasserted guarantees you that: > >>>>> > >>>>> - The EP has bootstrapped > >>>>> - It is safe not to toggle it again (and also skip > >>>>> uniphier_pcie_ltssm_enable()) > >>>>> > >>>>> Please provide details of the HW configuration so that we understand > >>>>> what's actually supposed to happen and why this patch fixes the > >>>>> issue you are facing. > >>>> > >>>> I tried to connect between the following boards, and do pci-epf-test: > >>>> - "RC board": UniPhier ld20 board that has DWC RC controller > >>>> - "EP board": UniPhier legacy board that has DWC EP controller > >>>> > >>>> This EP has power-on-state configuration, but it's necessary to set > >>>> class ID, BAR sizes, etc. after starting up. > >>>> > >>>> In case of that starting up RC board before EP board, the RC driver > >>>> can't establish link. So we need to boot EP board first. > >>>> At that point, I've considered why RC can't establish link, > >>> and found that the waitng time was too short. > >>>> - EP/RC: power on both boards > >>>> - RC: start up the kernel on RC board > >>>> - RC: wait for link up (long time enough) > >>>> - EP: start up the kernel on EP board > >>>> - EP: configurate pci-epf-test > >>>> When the endpoint configuration is done and the EP driver enables LTSSM, > >>> the RC driver will quit from waiting for link up. > >>>> Currently DWC RC driver calls dwc_pcie_wait_for_link(), however, > >>> the function tries to link up 10 times only, that is defined > >>> as LINK_WAIT_MAX_RETRIES in pcie-designware.h, it's too short > >>> to configurate the endpoint. > >>>> Now the patch to bypass PERST# is not necessary. > >>>> Instead for DWC RC drivers, I think that the number of retries > >>> should be changed according to the usage. > >>> And the same issue remains with other RC drivers. > >> > >> If EP is configured using Linux, then PERST# cannot be used as it's difficult to boot linux and initialize EP within the specified time interval. Can't you prevent PERST from being propagated at all? > > > Surely it might be difficult for RC to decide the time to wait for EP. > > Since RC almost toggles PERST# in boot time, I'd like to think about > > how to prevent from first PERST# at least. > > It can be prevented in the HW (If that's possible). I modify the cable connecting RC and EP to not propagate PERST#. I understand. Although it's possible in case of a cable, in case of an edge connector, EP side needs hardware mechanism not to propagate PERST#. Thank you, --- Best Regards, Kunihiko Hayashi
diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c index 8fd7bad..1ea4220 100644 --- a/drivers/pci/controller/dwc/pcie-uniphier.c +++ b/drivers/pci/controller/dwc/pcie-uniphier.c @@ -22,6 +22,9 @@ #include "pcie-designware.h" +#define PCL_PINMON 0x0028 +#define PCL_PINMON_PERST_OUT BIT(16) + #define PCL_PINCTRL0 0x002c #define PCL_PERST_PLDN_REGEN BIT(12) #define PCL_PERST_NOE_REGEN BIT(11) @@ -100,6 +103,11 @@ static void uniphier_pcie_init_rc(struct uniphier_pcie_priv *priv) val |= PCL_SYS_AUX_PWR_DET; writel(val, priv->base + PCL_APP_PM0); + /* return if PERST# is already deasserted */ + val = readl(priv->base + PCL_PINMON); + if (val & PCL_PINMON_PERST_OUT) + return; + /* assert PERST# */ val = readl(priv->base + PCL_PINCTRL0); val &= ~(PCL_PERST_NOE_REGVAL | PCL_PERST_OUT_REGVAL
When PERST# is asserted once, EP configuration will be initialized. If PERST# has been already deasserted, it isn't necessary to assert here. This checks whether PERST# is deasserted using PCL_PINMON register, and adds omit controlling PERST#. Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> --- drivers/pci/controller/dwc/pcie-uniphier.c | 8 ++++++++ 1 file changed, 8 insertions(+)