Re: [PATCH 09/22] scsi: hisi_sas: retrieve SAS address for pci-based controller

[John Garry]

Hi Arnd,

Currently there is no pci device listed in the ACPI tables.

What I am doing is declaring a fake device in the root of the System bus
tree of the ACPI tables, and in the kernel driver finding it by matching the
name. It is not the ACPI companion for the pci device.

So I think that we can define the pci device under the pci bus in the ACPI
tables, and define the ADR and DSD. Then we would have an ACPI companion for
the device, and from that get the SAS address.


Yes, that would be much better, and allow us to use the device properties
interface directly. An additional advantage is that the property definition
can be exactly the same as for the v1/v2 platform_device properties
for anything that might be needed across all versions.  sas-addr already
fits in there, and there might be additional properties you need in the
future.



Hi Arnd,

OK, we'll try this.

As for other properties, phy and queue count are common to all versions.

However, I need to consider more if I should add these - maybe it is ok. 
The reason we did not hardcode these for v1/v2 was that these were 
variable for controllers inter and intra SoC.


hip08 only has 1 controller, and I have been told that each controller 
will have a unique pci device id that for future SoCs, so it's viable to 
hardcode.



An alternative to this ACPI device method is for UEFI to write the SAS
address to a defined free location in device's pci config space, which the
driver can read.


That sounds ok as well, it would be nice to not rely on firmware data here,
but I'd have to see how the implementation ends up: IIRC you should
not just put the data at a fixed location in the config space but instead use
the 'extended capabilities' infrastructure to find the data.
PCI_EXT_CAP_ID_VNDR might be the right one here, but I don't
know enough about this, so please consult with someone who does
(or the PCIe specification).



Right, we'll keep ACPI table description as plan A.


  Arnd



Much appreciated,
John


.

Re: [PATCH 09/22] scsi: hisi_sas: retrieve SAS address for pci-based controller

[Arnd Bergmann]

On Wed, May 17, 2017 at 6:45 PM, John Garry  wrote:
> On 17/05/2017 15:13, Arnd Bergmann wrote:
>> On Wed, May 17, 2017 at 3:37 PM, John Garry  wrote:
>>> On 17/05/2017 13:37, Arnd Bergmann wrote:
>>> As for your suggestion, in theory it could be ok to have a
>>> pci_dev->fwnode,
>>> and this would work for Unified Device Properties Interface (if that is
>>> indeed what you mean). But how to create/match this fwnode?
>>
>>
>> pci_scan_device sets the dev->of_node field for DT based probing, we
>> could add a trivial patch to set the fwnode field as well if that isn't
>> already done elsewhere.
>>
>>> From what I can tell, the ACPI code sets up a 'companion' device for
>>
>> any PCI device that is listed in the ACPI tables, at least it refers to
>> that later on, but I could not figure out where it actually gets set.
>>
>
> Hi Arnd,
>
> Currently there is no pci device listed in the ACPI tables.
>
> What I am doing is declaring a fake device in the root of the System bus
> tree of the ACPI tables, and in the kernel driver finding it by matching the
> name. It is not the ACPI companion for the pci device.
>
> So I think that we can define the pci device under the pci bus in the ACPI
> tables, and define the ADR and DSD. Then we would have an ACPI companion for
> the device, and from that get the SAS address.

Yes, that would be much better, and allow us to use the device properties
interface directly. An additional advantage is that the property definition
can be exactly the same as for the v1/v2 platform_device properties
for anything that might be needed across all versions.  sas-addr already
fits in there, and there might be additional properties you need in the
future.

> An alternative to this ACPI device method is for UEFI to write the SAS
> address to a defined free location in device's pci config space, which the
> driver can read.

That sounds ok as well, it would be nice to not rely on firmware data here,
but I'd have to see how the implementation ends up: IIRC you should
not just put the data at a fixed location in the config space but instead use
the 'extended capabilities' infrastructure to find the data.
PCI_EXT_CAP_ID_VNDR might be the right one here, but I don't
know enough about this, so please consult with someone who does
(or the PCIe specification).

  Arnd

Re: [PATCH 09/22] scsi: hisi_sas: retrieve SAS address for pci-based controller

[John Garry]

On 17/05/2017 15:13, Arnd Bergmann wrote:

On Wed, May 17, 2017 at 3:37 PM, John Garry  wrote:

On 17/05/2017 13:37, Arnd Bergmann wrote:



Since this uses the _DSD information that was introduced for compatibility
between device tree and ACPI based data, why not write the code so that
it can work for both?

Ideally this should just look up the property for pci_dev->fwnode, but I'm
not entirely sure if this works the same way on ACPI and DT today.



Hi Arnd,

There is no relation at all between the pci device and the ACPI DSDT, apart
from the fact that the device node name is known by the driver, that being
"SAS0". I am not 100% comfortable with this, as we are introducing some sort
of device indexing in the driver.

Please note that Cavium did something similar on their thunderx network
driver:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/cavium/thunder/thunder_bgx.c?h=v4.12-rc1#n1162

As for your suggestion, in theory it could be ok to have a pci_dev->fwnode,
and this would work for Unified Device Properties Interface (if that is
indeed what you mean). But how to create/match this fwnode?


pci_scan_device sets the dev->of_node field for DT based probing, we
could add a trivial patch to set the fwnode field as well if that isn't
already done elsewhere.


From what I can tell, the ACPI code sets up a 'companion' device for

any PCI device that is listed in the ACPI tables, at least it refers to
that later on, but I could not figure out where it actually gets set.



Hi Arnd,

Currently there is no pci device listed in the ACPI tables.

What I am doing is declaring a fake device in the root of the System bus 
tree of the ACPI tables, and in the kernel driver finding it by matching 
the name. It is not the ACPI companion for the pci device.


So I think that we can define the pci device under the pci bus in the 
ACPI tables, and define the ADR and DSD. Then we would have an ACPI 
companion for the device, and from that get the SAS address.


An alternative to this ACPI device method is for UEFI to write the SAS 
address to a defined free location in device's pci config space, which 
the driver can read.


Much appreciated,
John


Arnd

.

Re: [PATCH 09/22] scsi: hisi_sas: retrieve SAS address for pci-based controller

[Arnd Bergmann]

On Wed, May 17, 2017 at 3:37 PM, John Garry  wrote:
> On 17/05/2017 13:37, Arnd Bergmann wrote:

>> Since this uses the _DSD information that was introduced for compatibility
>> between device tree and ACPI based data, why not write the code so that
>> it can work for both?
>>
>> Ideally this should just look up the property for pci_dev->fwnode, but I'm
>> not entirely sure if this works the same way on ACPI and DT today.
>>
>
> Hi Arnd,
>
> There is no relation at all between the pci device and the ACPI DSDT, apart
> from the fact that the device node name is known by the driver, that being
> "SAS0". I am not 100% comfortable with this, as we are introducing some sort
> of device indexing in the driver.
>
> Please note that Cavium did something similar on their thunderx network
> driver:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/cavium/thunder/thunder_bgx.c?h=v4.12-rc1#n1162
>
> As for your suggestion, in theory it could be ok to have a pci_dev->fwnode,
> and this would work for Unified Device Properties Interface (if that is
> indeed what you mean). But how to create/match this fwnode?

pci_scan_device sets the dev->of_node field for DT based probing, we
could add a trivial patch to set the fwnode field as well if that isn't
already done elsewhere.

>From what I can tell, the ACPI code sets up a 'companion' device for
any PCI device that is listed in the ACPI tables, at least it refers to
that later on, but I could not figure out where it actually gets set.

Arnd

Re: [PATCH 09/22] scsi: hisi_sas: retrieve SAS address for pci-based controller

[John Garry]

On 17/05/2017 13:37, Arnd Bergmann wrote:

On Wed, May 17, 2017 at 12:49 PM, John Garry  wrote:

> For a pci-based controller, retrieve the SAS address from the
> ACPI tables.
>
> The retrieval is based on the ACPI device node name. Sample is
> as follows:
> Scope(_SB)
> {
> Device(SAS0) {
> Name(_HID, "HISI0163")
> Name (_DSD, Package () {
>   ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>   Package () {
> Package (2) {"sas-addr", Package() {0x50, 
0x01, 0x88, 0x20, 0x16, 00, 00, 0x00}},
>   }
>   })
> }
> }
>
> If the ACPI node or node property is not available, then we fall
> back on a default address.
>
> Signed-off-by: John Garry 

Since this uses the _DSD information that was introduced for compatibility
between device tree and ACPI based data, why not write the code so that
it can work for both?

Ideally this should just look up the property for pci_dev->fwnode, but I'm
not entirely sure if this works the same way on ACPI and DT today.



Hi Arnd,

There is no relation at all between the pci device and the ACPI DSDT, 
apart from the fact that the device node name is known by the driver, 
that being "SAS0". I am not 100% comfortable with this, as we are 
introducing some sort of device indexing in the driver.


Please note that Cavium did something similar on their thunderx network 
driver:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/cavium/thunder/thunder_bgx.c?h=v4.12-rc1#n1162

As for your suggestion, in theory it could be ok to have a 
pci_dev->fwnode, and this would work for Unified Device Properties 
Interface (if that is indeed what you mean). But how to create/match 
this fwnode?


Thanks,
John


 Arnd

Re: [PATCH 09/22] scsi: hisi_sas: retrieve SAS address for pci-based controller

[Arnd Bergmann]

On Wed, May 17, 2017 at 12:49 PM, John Garry  wrote:
> For a pci-based controller, retrieve the SAS address from the
> ACPI tables.
>
> The retrieval is based on the ACPI device node name. Sample is
> as follows:
> Scope(_SB)
> {
> Device(SAS0) {
> Name(_HID, "HISI0163")
> Name (_DSD, Package () {
>   ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>   Package () {
> Package (2) {"sas-addr", Package() {0x50, 
> 0x01, 0x88, 0x20, 0x16, 00, 00, 0x00}},
>   }
>   })
> }
> }
>
> If the ACPI node or node property is not available, then we fall
> back on a default address.
>
> Signed-off-by: John Garry 

Since this uses the _DSD information that was introduced for compatibility
between device tree and ACPI based data, why not write the code so that
it can work for both?

Ideally this should just look up the property for pci_dev->fwnode, but I'm
not entirely sure if this works the same way on ACPI and DT today.

 Arnd

[PATCH 09/22] scsi: hisi_sas: retrieve SAS address for pci-based controller

[John Garry]

For a pci-based controller, retrieve the SAS address from the
ACPI tables.

The retrieval is based on the ACPI device node name. Sample is
as follows:
Scope(_SB)
{
Device(SAS0) {
Name(_HID, "HISI0163")
Name (_DSD, Package () {
  ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
  Package () {
Package (2) {"sas-addr", Package() {0x50, 0x01, 
0x88, 0x20, 0x16, 00, 00, 0x00}},
  }
  })
}
}

If the ACPI node or node property is not available, then we fall
back on a default address.

Signed-off-by: John Garry 
---
 drivers/scsi/hisi_sas/hisi_sas.h  |   2 +
 drivers/scsi/hisi_sas/hisi_sas_pci_init.c | 104 ++
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c|   2 +-
 3 files changed, 107 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas.h b/drivers/scsi/hisi_sas/hisi_sas.h
index 9bc9550..9fd874d 100644
--- a/drivers/scsi/hisi_sas/hisi_sas.h
+++ b/drivers/scsi/hisi_sas/hisi_sas.h
@@ -190,6 +190,7 @@ struct hisi_sas_pci_hw {
int n_phy;
int queue_count;
const struct hisi_sas_hw *hw;
+   const char *fw_name;
 };
 
 struct hisi_hba {
@@ -250,6 +251,7 @@ struct hisi_hba {
struct hisi_sas_slot*slot_info;
unsigned long flags;
const struct hisi_sas_hw *hw;   /* Low level hw interface */
+   const char *fw_name;
unsigned long sata_dev_bitmap[BITS_TO_LONGS(HISI_SAS_MAX_DEVICES)];
struct work_struct rst_work;
 };
diff --git a/drivers/scsi/hisi_sas/hisi_sas_pci_init.c 
b/drivers/scsi/hisi_sas/hisi_sas_pci_init.c
index e00b482..6d92893 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_pci_init.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_pci_init.c
@@ -11,6 +11,107 @@
 #include "hisi_sas.h"
 #define DRV_NAME "hisi_sas_pci"
 
+
+static const u8 default_sas_addr[SAS_ADDR_SIZE] = {
+   0x50, 0x01, 0x88, 0x20, 0x16, 0x0, 0x0, 0x0};
+static void hisi_sas_pci_set_sas_addr(struct hisi_hba *hisi_hba);
+
+static void
+hisi_sas_pci_set_default_sas_addr(struct hisi_hba *hisi_hba)
+{
+   dev_warn(hisi_hba->dev, "using default SAS ADDR\n");
+   memcpy(hisi_hba->sas_addr, default_sas_addr, SAS_ADDR_SIZE);
+}
+
+#if defined(CONFIG_ACPI)
+static int
+acpi_get_sas_address(struct device *dev, struct acpi_device *adev,
+   u8 *dst)
+{
+   u8 addr[SAS_ADDR_SIZE];
+   int rc;
+
+   rc = fwnode_property_read_u8_array(acpi_fwnode_handle(adev),
+   "sas-addr", addr, SAS_ADDR_SIZE);
+   if (rc) {
+   dev_warn(dev, "could not read sas-addr (%d)\n", rc);
+   return rc;
+   }
+
+   dev_info(dev, "SAS address set to: %pM\n", addr);
+
+   memcpy(dst, addr, SAS_ADDR_SIZE);
+
+   return 0;
+}
+
+static acpi_status
+hisi_sas_acpi_match_id(acpi_handle handle, u32 lvl,
+  void *context, void **ret_val)
+{
+   struct acpi_buffer string = { ACPI_ALLOCATE_BUFFER, NULL };
+   struct hisi_hba *hisi_hba = context;
+   struct device *dev = hisi_hba->dev;
+   struct acpi_device *adev;
+   int rc;
+
+   if (ACPI_FAILURE(acpi_get_name(handle, ACPI_SINGLE_NAME, ))) {
+   dev_warn(dev, "Invalid link device\n");
+   return AE_OK;
+   }
+
+   if (strncmp(string.pointer, hisi_hba->fw_name, 4))
+   return AE_OK;
+
+   rc = acpi_bus_get_device(handle, );
+
+   if (rc == 0)
+   rc = acpi_get_sas_address(dev, adev, hisi_hba->sas_addr);
+   else
+   dev_warn(dev, "could not get acpi bus device (%d)\n", rc);
+
+   if (rc == 0)
+   *(acpi_handle *)ret_val = handle;
+
+   kfree(string.pointer);
+   return AE_CTRL_TERMINATE;
+}
+
+static acpi_status
+hisi_sas_acpi_set_sas_addr(struct hisi_hba *hisi_hba)
+{
+   acpi_status status1;
+   acpi_handle *acpi_handle = NULL;
+
+   status1 = acpi_get_devices(NULL, hisi_sas_acpi_match_id,
+  hisi_hba, (void **)_handle);
+
+   if (status1 != AE_OK)
+   return status1;
+
+   if (!acpi_handle)
+   return AE_ERROR;
+
+   return AE_OK;
+}
+#else
+static acpi_status
+hisi_sas_acpi_set_sas_addr(struct hisi_hba *hisi_hba)
+{
+   return AE_ERROR;
+}
+#endif
+
+static void hisi_sas_pci_set_sas_addr(struct hisi_hba *hisi_hba)
+{
+   acpi_status status = AE_ERROR;
+   if (!acpi_disabled)
+   status = hisi_sas_acpi_set_sas_addr(hisi_hba);
+   if (status == AE_OK)
+   return;
+   hisi_sas_pci_set_default_sas_addr(hisi_hba);
+}
+
 static struct Scsi_Host *
 hisi_sas_shost_alloc_pci(struct pci_dev *pdev,
 const struct hisi_sas_pci_hw *hw)
@@ -32,6 +133,9 @@
hisi_hba->shost = shost;
SHOST_TO_SAS_HA(shost) = _hba->sha;
 
+   hisi_hba->fw_name =