[PATCH 1/1] powerpc: Add powerpc PCI-E reset API implementation

[PATCH 1/1] powerpc: Add powerpc PCI-E reset API implementation

[Brian King]

Adds the pSeries platform implementation for a new PCI API
which can be used to issue various types of PCI-E reset,
including PCI-E warm reset and PCI-E hot reset. This is needed
for an ipr PCI-E adapter which does not properly implement BIST.
Running BIST on this adapter results in PCI-E errors. The only
reliable reset mechanism that exists on this hardware is PCI
Fundamental reset (warm reset). 

Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---

Paul,

This patch requires a generic PCI layer patch, which is in Greg's
queue for 2.6.22, so do NOT apply this yet. Once the PCI change goes
in, I will resend this patch.

Thanks,

Brian
---

 linux-2.6-bjking1/arch/powerpc/platforms/pseries/eeh.c |   30 +++++++++++++++++
 1 files changed, 30 insertions(+)

diff -puN arch/powerpc/platforms/pseries/eeh.c~powerpc_slot_reset_api3 arch/powerpc/platforms/pseries/eeh.c
--- linux-2.6/arch/powerpc/platforms/pseries/eeh.c~powerpc_slot_reset_api3	2007-04-06 08:38:35.000000000 -0500
+++ linux-2.6-bjking1/arch/powerpc/platforms/pseries/eeh.c	2007-04-06 08:38:35.000000000 -0500
@@ -561,6 +561,36 @@ rtas_pci_slot_reset(struct pci_dn *pdn, 
 }
 
 /**
+ * pcibios_set_pcie_slot_reset - Set PCI-E reset state
+ * @dev:	pci device struct
+ * @state:	reset state to enter
+ *
+ * Return value:
+ * 	0 if success
+ **/
+int pcibios_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state state)
+{
+	struct device_node *dn = pci_device_to_OF_node(dev);
+	struct pci_dn *pdn = PCI_DN(dn);
+
+	switch (state) {
+	case pci_reset_normal:
+		rtas_pci_slot_reset(pdn, 0);
+		break;
+	case pci_reset_pcie_hot_reset:
+		rtas_pci_slot_reset(pdn, 1);
+		break;
+	case pci_reset_pcie_warm_reset:
+		rtas_pci_slot_reset(pdn, 3);
+		break;
+	default:
+		return -EINVAL;
+	};
+
+	return 0;
+}
+
+/**
  * rtas_set_slot_reset -- assert the pci #RST line for 1/4 second
  * @pdn: pci device node to be reset.
  *
_

Re: [PATCH 1/1] powerpc: Add powerpc PCI-E reset API implementation

[Linas Vepstas]

On Fri, Apr 06, 2007 at 09:07:04AM -0500, Brian King wrote:
> 
> This patch requires a generic PCI layer patch, which is in Greg's
> queue for 2.6.22, 

I managed to miss seeing that patch, even though I thought I was 
subscribed to the mailing list. Was there any discusion of it?

> +int pcibios_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state state)

I think you'll need an EXPORT_SMBOL_GPL if you plan to call this from
a module.

> +	switch (state) {
> +	case pci_reset_normal:
> +		rtas_pci_slot_reset(pdn, 0);

I find this naming confusing. rtas_pci_slot_reset(pdn, 0),
for PCI ad PCI-X means "deassert the reset"; it does not 
mean "do a normal reset".

> +		break;
> +	case pci_reset_pcie_hot_reset:
> +		rtas_pci_slot_reset(pdn, 1);
> +		break;
> +	case pci_reset_pcie_warm_reset:
> +		rtas_pci_slot_reset(pdn, 3);

For PCI and PCI-X, rtas_pci_slot_reset(pdn, 1) means "hold the
reset line high, until such time that its de-asserted."

> +	return 0;

I notice that you do no error checking. I recently wrapped 
rtas_set_slot_reset() to wait for slot status to settle down 
before reporting success or failure of the reset. 

Although the PAPR maps 1 to hot reset, and 3 to #PERST, I always
had the impression that they managed to reverse meaing of these two
(i.e. its a poor match to what the PCI-E spec says), and I never 
understood why. I am still thinking that the correct reset seqeunce
on linux is to try "3" first, if its supported, and then try a "1".
I've not taken steps to do this, though.

I could wrap rtas_set_slot_reset()  to try a "3" first, for 
PCI-E slots, and do 1 "1" only if that fails. Would this solve 
the problem that you are having?

--linas

Re: [PATCH 1/1] powerpc: Add powerpc PCI-E reset API implementation

[Brian King]

Linas Vepstas wrote:
> On Fri, Apr 06, 2007 at 09:07:04AM -0500, Brian King wrote:
>> This patch requires a generic PCI layer patch, which is in Greg's
>> queue for 2.6.22, 
> 
> I managed to miss seeing that patch, even though I thought I was 
> subscribed to the mailing list. Was there any discusion of it?

http://marc.info/?t=117035109100003&r=1&w=2

>> +int pcibios_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state state)
> 
> I think you'll need an EXPORT_SMBOL_GPL if you plan to call this from
> a module.

It is only to be called from the generic PCI layer. Anyone else should be
calling pci_set_pcie_reset_state, which is the PCI layer wrapper for it.

>> +	switch (state) {
>> +	case pci_reset_normal:
>> +		rtas_pci_slot_reset(pdn, 0);
> 
> I find this naming confusing. rtas_pci_slot_reset(pdn, 0),
> for PCI ad PCI-X means "deassert the reset"; it does not 
> mean "do a normal reset".

Normal refers to the reset state of the slot, not the type of reset.
I'm happy to change it if that would be less confusing.


>> +		break;
>> +	case pci_reset_pcie_hot_reset:
>> +		rtas_pci_slot_reset(pdn, 1);
>> +		break;
>> +	case pci_reset_pcie_warm_reset:
>> +		rtas_pci_slot_reset(pdn, 3);
> 
> For PCI and PCI-X, rtas_pci_slot_reset(pdn, 1) means "hold the
> reset line high, until such time that its de-asserted."

Correct. This is a PCI-E only API. I don't think we want to be
supporting an API that allows asserting reset on a potentially
shared PCI bus.

>> +	return 0;
> 
> I notice that you do no error checking. I recently wrapped 
> rtas_set_slot_reset() to wait for slot status to settle down 
> before reporting success or failure of the reset. 

I didn't do any error checking because there was no error checking
in __rtas_set_slot_reset either. I can add error checking if needed,
but I'm not sure I would do anything with it in ipr. 

> Although the PAPR maps 1 to hot reset, and 3 to #PERST, I always
> had the impression that they managed to reverse meaing of these two
> (i.e. its a poor match to what the PCI-E spec says), and I never 
> understood why. I am still thinking that the correct reset seqeunce
> on linux is to try "3" first, if its supported, and then try a "1".
> I've not taken steps to do this, though.

Possibly. Its not clear to me how you can tell if a 3 is supported, however.
You can check if the platform supports it, but there is no guarantee
that the adapter itself supports it. It may ignore it, or it may do
bad things.

> I could wrap rtas_set_slot_reset()  to try a "3" first, for 
> PCI-E slots, and do 1 "1" only if that fails. Would this solve 
> the problem that you are having?

rtas_set_slot_reset can sleep, which means the amount of code and
complexity in the ipr driver to use this API would grow quite a bit.

Additionally, as I already mentioned, how would you know that 3 failed?
The device may respond just fine to config cycles after a failed "3", but
still not actually be in a healthy state. As a corollary, with this particular
ipr adapter a hot reset appears to work from a pci config access perspective,
but the adapter firmware ends up in a bad state since the adapter's processor
does not get reset. Its not clear would don't have adapters that have issues
in the other direction.

Brian

-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: [PATCH 1/1] powerpc: Add powerpc PCI-E reset API implementation

[Linas Vepstas]

On Fri, Apr 06, 2007 at 02:19:04PM -0500, Brian King wrote:
> Linas Vepstas wrote:
> > On Fri, Apr 06, 2007 at 09:07:04AM -0500, Brian King wrote:
> 
> >> +	switch (state) {
> >> +	case pci_reset_normal:
> >> +		rtas_pci_slot_reset(pdn, 0);
> > 
> > I find this naming confusing. rtas_pci_slot_reset(pdn, 0),
> > for PCI ad PCI-X means "deassert the reset"; it does not 
> > mean "do a normal reset".
> 
> Normal refers to the reset state of the slot, not the type of reset.
> I'm happy to change it if that would be less confusing.

Hmm. Well, then, can I get you to change it to "pcie_deassert_reset"?

> >> +	case pci_reset_pcie_hot_reset:

and while we're at it, shorten this to "pcie_hot_reset"

> >> +	case pci_reset_pcie_warm_reset:

"pcie_warm_reset"

> Correct. This is a PCI-E only API. 

which is why I suggest the shortened names.

> I don't think we want to be
> supporting an API that allows asserting reset on a potentially
> shared PCI bus.

Actually, we very nearly do so already.  The "pci error recovery" API 
does define a way of coordinating a pending reset to be delivered 
to multiple PCI functions or multiple PCI cards.  What's missing 
is an external, public API for triggering the thing; right now, 
its only triggered by the hardware and handled by static, private 
routines.

> >> +	return 0;
> > 
> > I notice that you do no error checking. I recently wrapped 
> > rtas_set_slot_reset() to wait for slot status to settle down 
> > before reporting success or failure of the reset. 
> 
> I didn't do any error checking because there was no error checking
> in __rtas_set_slot_reset either. 

Yeah, its a new function: "eeh_wait_for_slot_status", its public,
but cleary very arch dependent. Just FYI, I don't think you'd want
to use it.

> > Although the PAPR maps 1 to hot reset, and 3 to #PERST, I always
> > had the impression that they managed to reverse meaing of these two
[...]
> The device may respond just fine to config cycles after a failed "3", but
> still not actually be in a healthy state. As a corollary, with this particular
> ipr adapter a hot reset appears to work from a pci config access perspective,
> but the adapter firmware ends up in a bad state since the adapter's processor
> does not get reset. Its not clear would don't have adapters that have issues
> in the other direction.

Yes... we had this conversation once before, didn't we?  The ugliness of
it all made me scurry away to some other task. 

--linas

[PATCH 1/1] powerpc: Add powerpc PCI-E reset API implementation

[Brian King]

Adds the pSeries platform implementation for a new PCI API
which can be used to issue various types of PCI-E reset,
including PCI-E warm reset and PCI-E hot reset. This is needed
for an ipr PCI-E adapter which does not properly implement BIST.
Running BIST on this adapter results in PCI-E errors. The only
reliable reset mechanism that exists on this hardware is PCI
Fundamental reset (warm reset). 

Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---

Paul,

This patch requires a generic PCI layer patch, which is in Greg's
queue for 2.6.22, so do NOT apply this yet. Once the PCI change goes
in, I will resend this patch.

This is an update in response to the comments Linas Vepstas had.

Thanks,

Brian
---

 linux-2.6-bjking1/arch/powerpc/platforms/pseries/eeh.c |   30 +++++++++++++++++
 1 files changed, 30 insertions(+)

diff -puN arch/powerpc/platforms/pseries/eeh.c~powerpc_slot_reset_api3 arch/powerpc/platforms/pseries/eeh.c
--- linux-2.6/arch/powerpc/platforms/pseries/eeh.c~powerpc_slot_reset_api3	2007-04-06 15:39:12.000000000 -0500
+++ linux-2.6-bjking1/arch/powerpc/platforms/pseries/eeh.c	2007-04-06 15:40:29.000000000 -0500
@@ -561,6 +561,36 @@ rtas_pci_slot_reset(struct pci_dn *pdn, 
 }
 
 /**
+ * pcibios_set_pcie_slot_reset - Set PCI-E reset state
+ * @dev:	pci device struct
+ * @state:	reset state to enter
+ *
+ * Return value:
+ * 	0 if success
+ **/
+int pcibios_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state state)
+{
+	struct device_node *dn = pci_device_to_OF_node(dev);
+	struct pci_dn *pdn = PCI_DN(dn);
+
+	switch (state) {
+	case pcie_deassert_reset:
+		rtas_pci_slot_reset(pdn, 0);
+		break;
+	case pcie_hot_reset:
+		rtas_pci_slot_reset(pdn, 1);
+		break;
+	case pcie_warm_reset:
+		rtas_pci_slot_reset(pdn, 3);
+		break;
+	default:
+		return -EINVAL;
+	};
+
+	return 0;
+}
+
+/**
  * rtas_set_slot_reset -- assert the pci #RST line for 1/4 second
  * @pdn: pci device node to be reset.
  *
_

Re: [PATCH 1/1] powerpc: Add powerpc PCI-E reset API implementation

[Linas Vepstas]

On Fri, Apr 06, 2007 at 04:40:40PM -0500, Brian King wrote:
> 
> Adds the pSeries platform implementation for a new PCI API
> which can be used to issue various types of PCI-E reset,
> including PCI-E warm reset and PCI-E hot reset. This is needed
> for an ipr PCI-E adapter which does not properly implement BIST.
> Running BIST on this adapter results in PCI-E errors. The only
> reliable reset mechanism that exists on this hardware is PCI
> Fundamental reset (warm reset). 
> 
> Signed-off-by: Brian King <brking@linux.vnet.ibm.com>

Acked-by: Linas Vepstas <linas@austin.ibm.com>

[PATCH 1/1] powerpc: Add powerpc PCI-E reset API implementation

[Brian King]

Adds the pSeries platform implementation for a new PCI API
which can be used to issue various types of PCI-E reset,
including PCI-E warm reset and PCI-E hot reset. This is needed
for an ipr PCI-E adapter which does not properly implement BIST.
Running BIST on this adapter results in PCI-E errors. The only
reliable reset mechanism that exists on this hardware is PCI
Fundamental reset (warm reset). 

Acked-by: Linas Vepstas <linas@austin.ibm.com>
Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
---

Paul,

The generic PCI layer patch that this patch depends on is now
in. This patch can now be merged. Hopefully it can still make
2.6.22.

Thanks,

Brian
---

 linux-2.6-bjking1/arch/powerpc/platforms/pseries/eeh.c |   30 +++++++++++++++++
 1 file changed, 30 insertions(+)

diff -puN arch/powerpc/platforms/pseries/eeh.c~powerpc_slot_reset_api3 arch/powerpc/platforms/pseries/eeh.c
--- linux-2.6/arch/powerpc/platforms/pseries/eeh.c~powerpc_slot_reset_api3	2007-05-07 16:52:33.000000000 -0500
+++ linux-2.6-bjking1/arch/powerpc/platforms/pseries/eeh.c	2007-05-07 16:52:33.000000000 -0500
@@ -580,6 +580,36 @@ rtas_pci_slot_reset(struct pci_dn *pdn, 
 }
 
 /**
+ * pcibios_set_pcie_slot_reset - Set PCI-E reset state
+ * @dev:	pci device struct
+ * @state:	reset state to enter
+ *
+ * Return value:
+ * 	0 if success
+ **/
+int pcibios_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state state)
+{
+	struct device_node *dn = pci_device_to_OF_node(dev);
+	struct pci_dn *pdn = PCI_DN(dn);
+
+	switch (state) {
+	case pcie_deassert_reset:
+		rtas_pci_slot_reset(pdn, 0);
+		break;
+	case pcie_hot_reset:
+		rtas_pci_slot_reset(pdn, 1);
+		break;
+	case pcie_warm_reset:
+		rtas_pci_slot_reset(pdn, 3);
+		break;
+	default:
+		return -EINVAL;
+	};
+
+	return 0;
+}
+
+/**
  * rtas_set_slot_reset -- assert the pci #RST line for 1/4 second
  * @pdn: pci device node to be reset.
  *
_