| Message ID | 1306505304-9593-3-git-send-email-agust@denx.de |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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
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
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. */
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(-)