| Message ID | 1488498882-18269-1-git-send-email-gwshan@linux.vnet.ibm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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
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
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
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
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);
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(-)