pci: Add -Tn to SAS adapter's loc-code

Submitted by Gavin Shan on March 2, 2017, 11:54 p.m.

Details

Message ID 1488498882-18269-1-git-send-email-gwshan@linux.vnet.ibm.com
State Changes Requested
Headers show

Commit Message

Gavin Shan March 2, 2017, 11:54 p.m.
This adds -Tn to the device-tree node's loc-code property for SAS
adaptr as we did for ethernet adapter in commit 6558adff01bb ("
Add -Tn to ibm,loc-code for ethernet adaptors"). "n" represents
the function number of the PCI device.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 core/pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Ananth N Mavinakayanahalli March 3, 2017, 2:20 a.m.
On Fri, Mar 03, 2017 at 10:54:42AM +1100, Gavin Shan wrote:
> This adds -Tn to the device-tree node's loc-code property for SAS
> adaptr as we did for ethernet adapter in commit 6558adff01bb ("
> Add -Tn to ibm,loc-code for ethernet adaptors"). "n" represents
> the function number of the PCI device.

This would mean that the "n" would not necessarily be contiguous or
start from 0 -- like what phyp does. Is that OK?

> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
>  core/pci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/core/pci.c b/core/pci.c
> index 9889dbf..6c8f65c 100644
> --- a/core/pci.c
> +++ b/core/pci.c
> @@ -1288,7 +1288,9 @@ static void pci_add_loc_code(struct dt_node *np, struct pci_device *pd)
>  	/* XXX Don't do that on openpower for now, we will need to sort things
>  	 * out later, otherwise the mezzanine slot on Habanero gets weird results
>  	 */
> -	if (class == 0x02 && sub == 0x00 && fsp_present()) {
> +	if (fsp_present() &&
> +	    ((class == 0x02 && sub == 0x00) ||	/* Ethernet */
> +	     (class == 0x01 && sub == 0x07))) {	/* SAS      */
>  		/* There's usually several spaces at the end of the property.
>  		   Test for, but don't rely on, that being the case */
>  		len = strlen(blcode);
> -- 
> 2.7.4
> 
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
Gavin Shan March 3, 2017, 3:19 a.m.
On Fri, Mar 03, 2017 at 07:50:52AM +0530, Ananth N Mavinakayanahalli wrote:
>On Fri, Mar 03, 2017 at 10:54:42AM +1100, Gavin Shan wrote:
>> This adds -Tn to the device-tree node's loc-code property for SAS
>> adaptr as we did for ethernet adapter in commit 6558adff01bb ("
>> Add -Tn to ibm,loc-code for ethernet adaptors"). "n" represents
>> the function number of the PCI device.
>
>This would mean that the "n" would not necessarily be contiguous or
>start from 0 -- like what phyp does. Is that OK?
>

It's not changed by the patch. If it's not correct, what we expect
from here?

>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> ---
>>  core/pci.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/core/pci.c b/core/pci.c
>> index 9889dbf..6c8f65c 100644
>> --- a/core/pci.c
>> +++ b/core/pci.c
>> @@ -1288,7 +1288,9 @@ static void pci_add_loc_code(struct dt_node *np, struct pci_device *pd)
>>  	/* XXX Don't do that on openpower for now, we will need to sort things
>>  	 * out later, otherwise the mezzanine slot on Habanero gets weird results
>>  	 */
>> -	if (class == 0x02 && sub == 0x00 && fsp_present()) {
>> +	if (fsp_present() &&
>> +	    ((class == 0x02 && sub == 0x00) ||	/* Ethernet */
>> +	     (class == 0x01 && sub == 0x07))) {	/* SAS      */
>>  		/* There's usually several spaces at the end of the property.
>>  		   Test for, but don't rely on, that being the case */
>>  		len = strlen(blcode);
>> -- 
>> 2.7.4
>> 
>> _______________________________________________
>> Skiboot mailing list
>> Skiboot@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/skiboot
Ananth N Mavinakayanahalli March 3, 2017, 3:51 a.m.
On Fri, Mar 03, 2017 at 02:19:23PM +1100, Gavin Shan wrote:
> On Fri, Mar 03, 2017 at 07:50:52AM +0530, Ananth N Mavinakayanahalli wrote:
> >On Fri, Mar 03, 2017 at 10:54:42AM +1100, Gavin Shan wrote:
> >> This adds -Tn to the device-tree node's loc-code property for SAS
> >> adaptr as we did for ethernet adapter in commit 6558adff01bb ("
> >> Add -Tn to ibm,loc-code for ethernet adaptors"). "n" represents
> >> the function number of the PCI device.
> >
> >This would mean that the "n" would not necessarily be contiguous or
> >start from 0 -- like what phyp does. Is that OK?
> >
> 
> It's not changed by the patch. If it's not correct, what we expect
> from here?

Brian King would be right person to guide us here...

Ananth
Brian King March 3, 2017, 2:35 p.m.
On 03/02/2017 09:51 PM, Ananth N Mavinakayanahalli wrote:
> On Fri, Mar 03, 2017 at 02:19:23PM +1100, Gavin Shan wrote:
>> On Fri, Mar 03, 2017 at 07:50:52AM +0530, Ananth N Mavinakayanahalli wrote:
>>> On Fri, Mar 03, 2017 at 10:54:42AM +1100, Gavin Shan wrote:
>>>> This adds -Tn to the device-tree node's loc-code property for SAS
>>>> adaptr as we did for ethernet adapter in commit 6558adff01bb ("
>>>> Add -Tn to ibm,loc-code for ethernet adaptors"). "n" represents
>>>> the function number of the PCI device.
>>>
>>> This would mean that the "n" would not necessarily be contiguous or
>>> start from 0 -- like what phyp does. Is that OK?
>>>
>>
>> It's not changed by the patch. If it's not correct, what we expect
>> from here?
> 
> Brian King would be right person to guide us here...

This is the algorithm that PHYP uses with PowerVM:

****************
This description applies to plug-in adapters.  Location codes for integrated devices include data we get from PHYP.


- PFW code keeps track of the base location code (i.e. up to the -C label) on a slot basis.
- For each PCI function associated with the slot, a location code is generated up to the -C label.
- If the function PCI class is not one of (PCI-host-bridge or PCI-PCI-bridge or USB or flash-memory or non-volatile-memory), append a -T label
-- If the location code base does not match the saved location code base
    then save the new location code base and start port numbering with 1; append port number 1
-- If the location code does match the saved location code base
   then increment the port number and append the port number

****************
To which Daniel then replied:
****************
Hi Carolyn,

Thanks for that explanation.

It's quite different to how functions are currently numbered in Skiboot.

 For each device probed:
 - copy the slot location code
 - if it is an ethernet device, add a '-Tn', where n is PCI function number + 1.

As you can see, labelling functions is stateless.

Probing is parallelised per PHB, but it looks like each PHB is done with a regular recursive probe, so we should be able to implement a PowerVM/PHYP style stateful model if necessary. Let's see how the rest of the root cause process goes.

****************

I don't see how we can use the PCI function number here, since these ports don't show up as separate functions but rather separate devices behind a PCI switch on the adapter. From the bugzilla, here is the lspci output:

# lspci | grep SAS
0001:05:00.0 Serial Attached SCSI controller: LSI Logic / Symbios Logic SAS2308 PCI-Express Fusion-MPT SAS-2 (rev 05)
0001:0b:00.0 Serial Attached SCSI controller: LSI Logic / Symbios Logic SAS2308 PCI-Express Fusion-MPT SAS-2 (rev 05)
0001:0c:00.0 RAID bus controller: IBM PCI-E IPR SAS Adapter (ASIC) (rev 02)
0005:03:00.0 Serial Attached SCSI controller: LSI Logic / Symbios Logic SAS2308 PCI-Express Fusion-MPT SAS-2 (rev 05)
0005:09:00.0 Serial Attached SCSI controller: LSI Logic / Symbios Logic SAS2308 PCI-Express Fusion-MPT SAS-2 (rev 05)
0006:03:00.0 Serial Attached SCSI controller: LSI Logic / Symbios Logic SAS2308 PCI-Express Fusion-MPT SAS-2 (rev 05)
0006:09:00.0 Serial Attached SCSI controller: LSI Logic / Symbios Logic SAS2308 PCI-Express Fusion-MPT SAS-2 (rev 05)


-Brian
Gavin Shan March 10, 2017, 12:30 a.m.
On Fri, Mar 03, 2017 at 08:35:40AM -0600, Brian King wrote:
>This is the algorithm that PHYP uses with PowerVM:
>
>****************
>This description applies to plug-in adapters.  Location codes for integrated devices include data we get from PHYP.
>
>
>- PFW code keeps track of the base location code (i.e. up to the -C label) on a slot basis.
>- For each PCI function associated with the slot, a location code is generated up to the -C label.
>- If the function PCI class is not one of (PCI-host-bridge or PCI-PCI-bridge or USB or flash-memory or non-volatile-memory), append a -T label
>-- If the location code base does not match the saved location code base
>    then save the new location code base and start port numbering with 1; append port number 1
>-- If the location code does match the saved location code base
>   then increment the port number and append the port number
>

Thanks, Brian. I'll adopt the code to follow the procedure. So this patch
should be ignore for now. I will post another revision.

Thanks,
Gavin

Patch hide | download patch | download mbox

diff --git a/core/pci.c b/core/pci.c
index 9889dbf..6c8f65c 100644
--- a/core/pci.c
+++ b/core/pci.c
@@ -1288,7 +1288,9 @@  static void pci_add_loc_code(struct dt_node *np, struct pci_device *pd)
 	/* XXX Don't do that on openpower for now, we will need to sort things
 	 * out later, otherwise the mezzanine slot on Habanero gets weird results
 	 */
-	if (class == 0x02 && sub == 0x00 && fsp_present()) {
+	if (fsp_present() &&
+	    ((class == 0x02 && sub == 0x00) ||	/* Ethernet */
+	     (class == 0x01 && sub == 0x07))) {	/* SAS      */
 		/* There's usually several spaces at the end of the property.
 		   Test for, but don't rely on, that being the case */
 		len = strlen(blcode);