[PATCH 1/2] PCI: uniphier: Set mode register to host mode

[PATCH 1/2] PCI: uniphier: Set mode register to host mode

[Kunihiko Hayashi]

In order to avoid effect of the initial mode depending on SoCs,
this patch sets the mode register to host(RC) mode.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/pci/controller/dwc/pcie-uniphier.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
index 3f30ee4..8fd7bad 100644
--- a/drivers/pci/controller/dwc/pcie-uniphier.c
+++ b/drivers/pci/controller/dwc/pcie-uniphier.c
@@ -33,6 +33,10 @@
 #define PCL_PIPEMON			0x0044
 #define PCL_PCLK_ALIVE			BIT(15)
 
+#define PCL_MODE			0x8000
+#define PCL_MODE_REGEN			BIT(8)
+#define PCL_MODE_REGVAL			BIT(0)
+
 #define PCL_APP_READY_CTRL		0x8008
 #define PCL_APP_LTSSM_ENABLE		BIT(0)
 
@@ -85,6 +89,12 @@ static void uniphier_pcie_init_rc(struct uniphier_pcie_priv *priv)
 {
 	u32 val;
 
+	/* set RC MODE */
+	val = readl(priv->base + PCL_MODE);
+	val |= PCL_MODE_REGEN;
+	val &= ~PCL_MODE_REGVAL;
+	writel(val, priv->base + PCL_MODE);
+
 	/* use auxiliary power detection */
 	val = readl(priv->base + PCL_APP_PM0);
 	val |= PCL_SYS_AUX_PWR_DET;
-- 
2.7.4

[PATCH 2/2] PCI: uniphier: Add checking whether PERST# is deasserted

[Kunihiko Hayashi]

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(+)

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
-- 
2.7.4

Re: [PATCH 2/2] PCI: uniphier: Add checking whether PERST# is deasserted

[Andrew Murray]

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
> 

Re: [PATCH 2/2] PCI: uniphier: Add checking whether PERST# is deasserted

[Kunihiko Hayashi]

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

Re: [PATCH 2/2] PCI: uniphier: Add checking whether PERST# is deasserted

[Andrew Murray]

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
> 

Re: [PATCH 2/2] PCI: uniphier: Add checking whether PERST# is deasserted

[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

Re: [PATCH 2/2] PCI: uniphier: Add checking whether PERST# is deasserted

[Andrew Murray]

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
> 
> 

Re: [PATCH 2/2] PCI: uniphier: Add checking whether PERST# is deasserted

[Lorenzo Pieralisi]

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

Re: [PATCH 2/2] PCI: uniphier: Add checking whether PERST# is deasserted

[Kunihiko Hayashi]

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

Re: [PATCH 2/2] PCI: uniphier: Add checking whether PERST# is deasserted

[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

Re: [PATCH 2/2] PCI: uniphier: Add checking whether PERST# is deasserted

[Kishon Vijay Abraham I]

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

Re: [PATCH 2/2] PCI: uniphier: Add checking whether PERST# is deasserted

[Kunihiko Hayashi]

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

Re: [PATCH 2/2] PCI: uniphier: Add checking whether PERST# is deasserted

[Kishon Vijay Abraham I]

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

Re: [PATCH 2/2] PCI: uniphier: Add checking whether PERST# is deasserted

[Kunihiko Hayashi]

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

Re: [PATCH 1/2] PCI: uniphier: Set mode register to host mode

[Andrew Murray]

On Thu, Nov 07, 2019 at 01:58:14PM +0900, Kunihiko Hayashi wrote:
> In order to avoid effect of the initial mode depending on SoCs,
> this patch sets the mode register to host(RC) mode.
> 
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
>  drivers/pci/controller/dwc/pcie-uniphier.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
> index 3f30ee4..8fd7bad 100644
> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
> @@ -33,6 +33,10 @@
>  #define PCL_PIPEMON			0x0044
>  #define PCL_PCLK_ALIVE			BIT(15)
>  
> +#define PCL_MODE			0x8000
> +#define PCL_MODE_REGEN			BIT(8)
> +#define PCL_MODE_REGVAL			BIT(0)
> +
>  #define PCL_APP_READY_CTRL		0x8008
>  #define PCL_APP_LTSSM_ENABLE		BIT(0)
>  
> @@ -85,6 +89,12 @@ static void uniphier_pcie_init_rc(struct uniphier_pcie_priv *priv)
>  {
>  	u32 val;
>  
> +	/* set RC MODE */
> +	val = readl(priv->base + PCL_MODE);
> +	val |= PCL_MODE_REGEN;
> +	val &= ~PCL_MODE_REGVAL;
> +	writel(val, priv->base + PCL_MODE);
> +

Reviewed-by: Andrew Murray <andrew.murray@arm.com>

>  	/* use auxiliary power detection */
>  	val = readl(priv->base + PCL_APP_PM0);
>  	val |= PCL_SYS_AUX_PWR_DET;
> -- 
> 2.7.4
> 

Re: [PATCH 1/2] PCI: uniphier: Set mode register to host mode

[Lorenzo Pieralisi]

On Thu, Nov 07, 2019 at 01:58:14PM +0900, Kunihiko Hayashi wrote:
> In order to avoid effect of the initial mode depending on SoCs,
> this patch sets the mode register to host(RC) mode.
> 
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
>  drivers/pci/controller/dwc/pcie-uniphier.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Applied to pci/uniphier, thanks.

Lorenzo

> diff --git a/drivers/pci/controller/dwc/pcie-uniphier.c b/drivers/pci/controller/dwc/pcie-uniphier.c
> index 3f30ee4..8fd7bad 100644
> --- a/drivers/pci/controller/dwc/pcie-uniphier.c
> +++ b/drivers/pci/controller/dwc/pcie-uniphier.c
> @@ -33,6 +33,10 @@
>  #define PCL_PIPEMON			0x0044
>  #define PCL_PCLK_ALIVE			BIT(15)
>  
> +#define PCL_MODE			0x8000
> +#define PCL_MODE_REGEN			BIT(8)
> +#define PCL_MODE_REGVAL			BIT(0)
> +
>  #define PCL_APP_READY_CTRL		0x8008
>  #define PCL_APP_LTSSM_ENABLE		BIT(0)
>  
> @@ -85,6 +89,12 @@ static void uniphier_pcie_init_rc(struct uniphier_pcie_priv *priv)
>  {
>  	u32 val;
>  
> +	/* set RC MODE */
> +	val = readl(priv->base + PCL_MODE);
> +	val |= PCL_MODE_REGEN;
> +	val &= ~PCL_MODE_REGVAL;
> +	writel(val, priv->base + PCL_MODE);
> +
>  	/* use auxiliary power detection */
>  	val = readl(priv->base + PCL_APP_PM0);
>  	val |= PCL_SYS_AUX_PWR_DET;
> -- 
> 2.7.4
>