Patchwork [U-Boot,2/5] pci: option for configurable delay between pci reset and pci bus scan

login
register
mail settings
Submitter Anatolij Gustschin
Date May 27, 2011, 2:08 p.m.
Message ID <1306505304-9593-3-git-send-email-agust@denx.de>
Download mbox | patch
Permalink /patch/97685/
State Superseded
Headers show

Comments

Anatolij Gustschin - May 27, 2011, 2:08 p.m.
PCI cards might need some time after reset to respond.
On some boards (mpc5200 or mpc8260 based) the PCI bus reset is
deasserted at pci_board_init() time, so we can not use available
"pcidelay" option for waiting before pci bus scan here. Add an option
to delay bus scan by setting "pci_scan_delay" environment variable.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
---
 drivers/pci/pci.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)
Detlev Zundel - May 27, 2011, 3:26 p.m.
Hi Anatolij,

> PCI cards might need some time after reset to respond.
> On some boards (mpc5200 or mpc8260 based) the PCI bus reset is
> deasserted at pci_board_init() time, so we can not use available
> "pcidelay" option for waiting before pci bus scan here. Add an option
> to delay bus scan by setting "pci_scan_delay" environment variable.

Hm, I'm not sure I understand the situation, so please correct me.  We
have a "pcidelay" variable, which is used to wait before
pci_board_init() (I'm not counting the semantically different usage in
the esd boards).  This does not fit your need, so you define
pci_scan_delay which is used _after_ pci_init_board(), correct?

If this is correct, then why don't you keep your new delay also in the
pci_init() function so that the delays are easily visible on code
inspection?  But wait, if this is only needed for this very board, then
why don't we put the delay into digsys pci_init_board?  Actually I think
this is the best way, as on this board we always need the delay as PCI
is not hotplug.

Apart from that, having two variables "pcidelay" and "pci_scan_delay" we
would need good documentation to explain their usage - the names do not
help (me) much ;)

Cheers
  Detlev
Anatolij Gustschin - May 27, 2011, 4:43 p.m.
Hi Detlev,

On Fri, 27 May 2011 17:26:24 +0200
Detlev Zundel <dzu@denx.de> wrote:
...
> > PCI cards might need some time after reset to respond.
> > On some boards (mpc5200 or mpc8260 based) the PCI bus reset is
> > deasserted at pci_board_init() time, so we can not use available
> > "pcidelay" option for waiting before pci bus scan here. Add an option
> > to delay bus scan by setting "pci_scan_delay" environment variable.
> 
> Hm, I'm not sure I understand the situation, so please correct me.  We
> have a "pcidelay" variable, which is used to wait before
> pci_board_init() (I'm not counting the semantically different usage in
> the esd boards).  This does not fit your need, so you define
> pci_scan_delay which is used _after_ pci_init_board(), correct?

yes, this is correct.

> If this is correct, then why don't you keep your new delay also in the
> pci_init() function so that the delays are easily visible on code
> inspection?  But wait, if this is only needed for this very board, then
> why don't we put the delay into digsys pci_init_board?  Actually I think
> this is the best way, as on this board we always need the delay as PCI
> is not hotplug.

The reason for not keeping new delay in pci_init() is:
pci_init_board() starts scanning the bus (calls pci_hose_scan()), so
when pci_init_board() returns, it is too late, the scanning is
already completed.

digsy's pci_init_board() just calls pci_mpc5xxx_init(), when the latter
returns, the scanning is completed, too. PCI reset is de-asserted in
pci_mpc5xxx_init(), so I thought about putting the delay there, but
similar situation is also on mpc8260 based boards, pci_mpc8250_init()
de-asserts PCI reset and waits on some boards (on MPC8266ADS 1 sec).
So the problem is not only digsy specific. The needed time after reset
before config cycles could be up to 1 sec, depending on the card. The
pci spec 2.2 allows this.

I think that it would be good to run arch specific pci init not from
the pci_board_init(), but from pci_init(). Then we can add delay
code in the board specific way. This would reduce the code duplication,
too. Currently we have the same pci_init_board() for many 5200 boards,
except for mvbc_p and mvsmr boards.

> Apart from that, having two variables "pcidelay" and "pci_scan_delay" we
> would need good documentation to explain their usage - the names do not
> help (me) much ;)

Sure. If there is an agreement to solve the problem as proposed in
the patch, I'll add the documentation in the next patch version.
Maybe someone have a better idea, lets wait a bit for other comments.
Actually I don't like the name of the variable, it is somehow
misleading. Any better name?

Thanks,
Anatolij
Detlev Zundel - May 30, 2011, 7:45 a.m.
Hi Anatolij,

> Hi Detlev,
>
> On Fri, 27 May 2011 17:26:24 +0200
> Detlev Zundel <dzu@denx.de> wrote:
> ...
>> > PCI cards might need some time after reset to respond.
>> > On some boards (mpc5200 or mpc8260 based) the PCI bus reset is
>> > deasserted at pci_board_init() time, so we can not use available
>> > "pcidelay" option for waiting before pci bus scan here. Add an option
>> > to delay bus scan by setting "pci_scan_delay" environment variable.
>> 
>> Hm, I'm not sure I understand the situation, so please correct me.  We
>> have a "pcidelay" variable, which is used to wait before
>> pci_board_init() (I'm not counting the semantically different usage in
>> the esd boards).  This does not fit your need, so you define
>> pci_scan_delay which is used _after_ pci_init_board(), correct?
>
> yes, this is correct.
>
>> If this is correct, then why don't you keep your new delay also in the
>> pci_init() function so that the delays are easily visible on code
>> inspection?  But wait, if this is only needed for this very board, then
>> why don't we put the delay into digsys pci_init_board?  Actually I think
>> this is the best way, as on this board we always need the delay as PCI
>> is not hotplug.
>
> The reason for not keeping new delay in pci_init() is:
> pci_init_board() starts scanning the bus (calls pci_hose_scan()), so
> when pci_init_board() returns, it is too late, the scanning is
> already completed.
>
> digsy's pci_init_board() just calls pci_mpc5xxx_init(), when the latter
> returns, the scanning is completed, too. PCI reset is de-asserted in
> pci_mpc5xxx_init(), so I thought about putting the delay there, but
> similar situation is also on mpc8260 based boards, pci_mpc8250_init()
> de-asserts PCI reset and waits on some boards (on MPC8266ADS 1 sec).
> So the problem is not only digsy specific. The needed time after reset
> before config cycles could be up to 1 sec, depending on the card. The
> pci spec 2.2 allows this.

Ah, thanks for shedding some light on this.  Now I see how you arrived
at the solution you propose.

> I think that it would be good to run arch specific pci init not from
> the pci_board_init(), but from pci_init(). Then we can add delay
> code in the board specific way. This would reduce the code duplication,
> too. Currently we have the same pci_init_board() for many 5200 boards,
> except for mvbc_p and mvsmr boards.

Yes, I have also noticed the massive code duplicatin here.  But as I
obviously didn't even understand the problem I didn't ponder changing
it ;)

>> Apart from that, having two variables "pcidelay" and "pci_scan_delay" we
>> would need good documentation to explain their usage - the names do not
>> help (me) much ;)
>
> Sure. If there is an agreement to solve the problem as proposed in
> the patch, I'll add the documentation in the next patch version.
> Maybe someone have a better idea, lets wait a bit for other comments.
> Actually I don't like the name of the variable, it is somehow
> misleading. Any better name?

Sorry, no idea.  If we are stuck stuck with "pcidelay" (which I think we
are), then it is hard to come up with a differentiating name.  So good
documentation will have to make up for the lack of good names ;)

Cheers
  Detlev
Stefan Roese - May 30, 2011, 2:10 p.m.
Hi Anatolij and Detlev,

On Monday 30 May 2011 09:45:08 Detlev Zundel wrote:
> >> Hm, I'm not sure I understand the situation, so please correct me.  We
> >> have a "pcidelay" variable, which is used to wait before
> >> pci_board_init() (I'm not counting the semantically different usage in
> >> the esd boards).  This does not fit your need, so you define
> >> pci_scan_delay which is used _after_ pci_init_board(), correct?
> > 
> > yes, this is correct.
> > 
> >> If this is correct, then why don't you keep your new delay also in the
> >> pci_init() function so that the delays are easily visible on code
> >> inspection?  But wait, if this is only needed for this very board, then
> >> why don't we put the delay into digsys pci_init_board?  Actually I think
> >> this is the best way, as on this board we always need the delay as PCI
> >> is not hotplug.
> > 
> > The reason for not keeping new delay in pci_init() is:
> > pci_init_board() starts scanning the bus (calls pci_hose_scan()), so
> > when pci_init_board() returns, it is too late, the scanning is
> > already completed.

Right. With this PCI reset "design", the current "pcidelay" option won't work 
for these platforms. Too bad.

But thinking more about it, couldn't your new code location supersede the old 
one before pci_init_board()? If this really is the case (we would need to 
check with users of this "pcidelay" env variable, Mattias?), then we could 
remove the old code in pci_init() and only use your new version. We would need 
to use the old env variable name "pcidelay" though, since there are boards in 
the field already using this version.

Anatolij, what do you think?

Matthias, could you do some tests on some esd boards with the new version when 
available, to make sure that we don't break backwards compatibility?
 
Best regards,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office@denx.de
Anatolij Gustschin - Oct. 11, 2011, 3:16 p.m.
Hi Stefan,

On Mon, 30 May 2011 16:10:34 +0200
Stefan Roese <sr@denx.de> wrote:

> Hi Anatolij and Detlev,
> 
> On Monday 30 May 2011 09:45:08 Detlev Zundel wrote:
> > >> Hm, I'm not sure I understand the situation, so please correct me.  We
> > >> have a "pcidelay" variable, which is used to wait before
> > >> pci_board_init() (I'm not counting the semantically different usage in
> > >> the esd boards).  This does not fit your need, so you define
> > >> pci_scan_delay which is used _after_ pci_init_board(), correct?
> > > 
> > > yes, this is correct.
> > > 
> > >> If this is correct, then why don't you keep your new delay also in the
> > >> pci_init() function so that the delays are easily visible on code
> > >> inspection?  But wait, if this is only needed for this very board, then
> > >> why don't we put the delay into digsys pci_init_board?  Actually I think
> > >> this is the best way, as on this board we always need the delay as PCI
> > >> is not hotplug.
> > > 
> > > The reason for not keeping new delay in pci_init() is:
> > > pci_init_board() starts scanning the bus (calls pci_hose_scan()), so
> > > when pci_init_board() returns, it is too late, the scanning is
> > > already completed.
> 
> Right. With this PCI reset "design", the current "pcidelay" option won't work 
> for these platforms. Too bad.
> 
> But thinking more about it, couldn't your new code location supersede the old 
> one before pci_init_board()? If this really is the case (we would need to 
> check with users of this "pcidelay" env variable, Mattias?), then we could 
> remove the old code in pci_init() and only use your new version. We would need 
> to use the old env variable name "pcidelay" though, since there are boards in 
> the field already using this version.
> 
> Anatolij, what do you think?

This should work, I think. I'll send a patch moving pcidelay code to
new location.

> Matthias, could you do some tests on some esd boards with the new version when 
> available, to make sure that we don't break backwards compatibility?

Would be great if Matthias could test the patch. 

Thanks,
Anatolij

--
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index cdfc4fb..2206c12 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -699,6 +699,20 @@  int pci_hose_scan_bus(struct pci_controller *hose, int bus)
 
 int pci_hose_scan(struct pci_controller *hose)
 {
+#if defined(CONFIG_PCI_SCAN_DELAY)
+	const char *s;
+	int i;
+
+	/* wait "pci_scan_delay" ms, limit to max. 1 s */
+	s = getenv("pci_scan_delay");
+	if (s) {
+		int val = simple_strtoul(s, NULL, 10);
+		if (val > 1000)
+			val = 1000;
+		for (i = 0; i < val; i++)
+			udelay(1000);
+	}
+#endif
 	/* Start scan at current_busno.
 	 * PCIe will start scan at first_busno+1.
 	 */