Re: [PATCH v12 2/2] PCI: hip: Add handling of HiSilicon HIP PCIe controller errors
Hi Bjorn,
Thanks for the comments.
On 2020/7/15 5:10, Bjorn Helgaas wrote:
> [+cc Lorenzo]
>
> On Mon, Jul 13, 2020 at 03:10:19PM +0100, Shiju Jose wrote:
>> From: Yicong Yang
>>
>> The HiSilicon HIP PCIe controller is capable of handling errors
>> on root port and perform port reset separately at each root port.
> s/perform/performing/ (to match "handling")
>
>> The driver placed in the drivers/pci/controller/ because the
>> HIP PCIe controller does not use DWC ip.
> s/ip/IP/
will fix these.
> +#define HISI_PCIE_LOCAL_VALID_ERR_MISC 9
> +
> +static guid_t hisi_pcie_sec_guid =
> + GUID_INIT(0xB2889FC9, 0xE7D7, 0x4F9D,
> + 0xA8, 0x67, 0xAF, 0x42, 0xE9, 0x8B, 0xE7, 0x72);
> +
> +/*
> + * We pass core id and core port id to the ACPI reset method to identify
> + * certain root port to reset, while the firmware reports sockets port
> + * id which occurs an error. Use the macros here to do the conversion
> Maybe:
>
> Firmware reports the socket port ID where the error occurred. These
> macros convert that to the core ID and core port ID required by the
> ACPI reset method.
>
> But even that doesn't quite make sense because you apparently get two
> values (edata->core_id, edata->port_id) from firmware.
will reword the comments.
Actually We have got the socket_id from the firmware, and we use it to find the
correct
error handler device on the same socket in hisi_pcie_notify_error(). As for
port id and core port id, the driver got the port id indexed per socket, but
the firmware
needs the port id indexed per core to locate the right register, so we need
these macros
to do the conversion.
>> + */
>> +#define HISI_PCIE_CORE_ID(v) ((v) >> 3)
>> +#define HISI_PCIE_PORT_ID(core, v) (((v) >> 1) + ((core) << 3))
>> +#define HISI_PCIE_CORE_PORT_ID(v)(((v) & 7) << 1)
> These would make more sense reordered and with HISI_PCIE_PORT_ID()
> rewritten like this:
>
> #define HISI_PCIE_PORT_ID(core, v) (((core) << 3) | ((v) >> 1))
> #define HISI_PCIE_CORE_ID(v) ((v) >> 3)
> #define HISI_PCIE_CORE_PORT_ID(v)(((v) & 7) << 1)
will reorder these.
Regards,
Yicong
>
>> +
>> +struct hisi_pcie_error_data {
>> +u64 val_bits;
>> +u8 version;
>> +u8 soc_id;
>> +u8 socket_id;
>> +u8 nimbus_id;
>> +u8 sub_module_id;
>> +u8 core_id;
>> +u8 port_id;
>> +u8 err_severity;
>> +u16 err_type;
>> +u8 reserv[2];
>> +u32 err_misc[HISI_PCIE_ERR_MISC_REGS];
>> +};
>> +
>> +struct hisi_pcie_error_private {
>> +struct notifier_block nb;
>> +struct device *dev;
>> +};
>> +
>> +enum hisi_pcie_submodule_id {
>> +HISI_PCIE_SUB_MODULE_ID_AP,
>> +HISI_PCIE_SUB_MODULE_ID_TL,
>> +HISI_PCIE_SUB_MODULE_ID_MAC,
>> +HISI_PCIE_SUB_MODULE_ID_DL,
>> +HISI_PCIE_SUB_MODULE_ID_SDI,
>> +};
>> +
>> +static const char * const hisi_pcie_sub_module[] = {
>> +[HISI_PCIE_SUB_MODULE_ID_AP]= "AP Layer",
>> +[HISI_PCIE_SUB_MODULE_ID_TL]= "TL Layer",
>> +[HISI_PCIE_SUB_MODULE_ID_MAC] = "MAC Layer",
>> +[HISI_PCIE_SUB_MODULE_ID_DL]= "DL Layer",
>> +[HISI_PCIE_SUB_MODULE_ID_SDI] = "SDI Layer",
>> +};
>> +
>> +enum hisi_pcie_err_severity {
>> +HISI_PCIE_ERR_SEV_RECOVERABLE,
>> +HISI_PCIE_ERR_SEV_FATAL,
>> +HISI_PCIE_ERR_SEV_CORRECTED,
>> +HISI_PCIE_ERR_SEV_NONE,
>> +};
>> +
>> +static const char * const hisi_pcie_error_sev[] = {
>> +[HISI_PCIE_ERR_SEV_RECOVERABLE] = "recoverable",
>> +[HISI_PCIE_ERR_SEV_FATAL] = "fatal",
>> +[HISI_PCIE_ERR_SEV_CORRECTED] = "corrected",
>> +[HISI_PCIE_ERR_SEV_NONE]= "none",
>> +};
>> +
>> +static const char *hisi_pcie_get_string(const char * const *array,
>> +size_t n, u32 id)
>> +{
>> +u32 index;
>> +
>> +for (index = 0; index < n; index++) {
>> +if (index == id && array[index])
>> +return array[index];
>> +}
>> +
>> +return "unknown";
>> +}
>> +
>> +static int hisi_pcie_port_reset(struct platform_device *pdev,
>> +u32 chip_id, u32 port_id)
>> +{
>> +struct device *dev = >dev;
>> +acpi_handle handle = ACPI_HANDLE(dev);
>> +union acpi_object arg[3];
>> +struct acpi_object_list arg_list;
>> +acpi_status s;
>> +unsigned long long data = 0;
>> +
>> +arg[0].type = ACPI_TYPE_INTEGER;
>> +arg[0].integer.value = chip_id;
>> +arg[1].type = ACPI_TYPE_INTEGER;
>> +arg[1].integer.value = HISI_PCIE_CORE_ID(port_id);
>> +arg[2].type = ACPI_TYPE_INTEGER;
>> +arg[2].integer.value = HISI_PCIE_CORE_PORT_ID(port_id);
>> +
>> +arg_list.count = 3;
>> +arg_list.pointer = arg;
>> +
>> +s = acpi_evaluate_integer(handle, "RST", _list, );
>> +if (ACPI_FAILURE(s)) {
>> +dev_err(dev, "No RST method\n");
>> +return -EIO;
>> +}
>> +
>> +if (data) {
>>
Re: [PATCH v12 2/2] PCI: hip: Add handling of HiSilicon HIP PCIe controller errors
[+cc Lorenzo] On Mon, Jul 13, 2020 at 03:10:19PM +0100, Shiju Jose wrote: > From: Yicong Yang > > The HiSilicon HIP PCIe controller is capable of handling errors > on root port and perform port reset separately at each root port. s/perform/performing/ (to match "handling") > Add error handling driver for HIP PCIe controller to log > and report recoverable errors. Perform root port reset and restore > link status after the recovery. > > Following are some of the PCIe controller's recoverable errors > 1. completion transmission timeout error. > 2. CRS retry counter over the threshold error. > 3. ECC 2 bit errors > 4. AXI bresponse/rresponse errors etc. > > The driver placed in the drivers/pci/controller/ because the > HIP PCIe controller does not use DWC ip. s/ip/IP/ > Signed-off-by: Yicong Yang > Signed-off-by: Shiju Jose OK by me unless Lorenzo has any comments. Acked-by: Bjorn Helgaas Looks like Rafael is willing to merge it? > -- > drivers/pci/controller/Kconfig | 8 + > drivers/pci/controller/Makefile | 1 + > drivers/pci/controller/pcie-hisi-error.c | 336 +++ > 3 files changed, 345 insertions(+) > create mode 100644 drivers/pci/controller/pcie-hisi-error.c > --- > drivers/pci/controller/Kconfig | 8 + > drivers/pci/controller/Makefile | 1 + > drivers/pci/controller/pcie-hisi-error.c | 327 +++ > 3 files changed, 336 insertions(+) > create mode 100644 drivers/pci/controller/pcie-hisi-error.c > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig > index adddf21fa381..b7949b37c029 100644 > --- a/drivers/pci/controller/Kconfig > +++ b/drivers/pci/controller/Kconfig > @@ -286,6 +286,14 @@ config PCI_LOONGSON > Say Y here if you want to enable PCI controller support on > Loongson systems. > > +config PCIE_HISI_ERR > + depends on ARM64 || COMPILE_TEST > + depends on ACPI > + bool "HiSilicon HIP PCIe controller error handling driver" > + help > + Say Y here if you want error handling support > + for the PCIe controller's errors on HiSilicon HIP SoCs > + > source "drivers/pci/controller/dwc/Kconfig" > source "drivers/pci/controller/mobiveil/Kconfig" > source "drivers/pci/controller/cadence/Kconfig" > diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile > index efd9733ead26..90afd865bf6b 100644 > --- a/drivers/pci/controller/Makefile > +++ b/drivers/pci/controller/Makefile > @@ -30,6 +30,7 @@ obj-$(CONFIG_PCIE_TANGO_SMP8759) += pcie-tango.o > obj-$(CONFIG_VMD) += vmd.o > obj-$(CONFIG_PCIE_BRCMSTB) += pcie-brcmstb.o > obj-$(CONFIG_PCI_LOONGSON) += pci-loongson.o > +obj-$(CONFIG_PCIE_HISI_ERR) += pcie-hisi-error.o > # pcie-hisi.o quirks are needed even without CONFIG_PCIE_DW > obj-y+= dwc/ > obj-y+= mobiveil/ > diff --git a/drivers/pci/controller/pcie-hisi-error.c > b/drivers/pci/controller/pcie-hisi-error.c > new file mode 100644 > index ..9bd050cadb31 > --- /dev/null > +++ b/drivers/pci/controller/pcie-hisi-error.c > @@ -0,0 +1,327 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Driver for handling the PCIe controller errors on > + * HiSilicon HIP SoCs. > + * > + * Copyright (c) 2020 HiSilicon Limited. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* HISI PCIe controller error definitions */ > +#define HISI_PCIE_ERR_MISC_REGS 33 > + > +#define HISI_PCIE_LOCAL_VALID_VERSIONBIT(0) > +#define HISI_PCIE_LOCAL_VALID_SOC_ID BIT(1) > +#define HISI_PCIE_LOCAL_VALID_SOCKET_ID BIT(2) > +#define HISI_PCIE_LOCAL_VALID_NIMBUS_ID BIT(3) > +#define HISI_PCIE_LOCAL_VALID_SUB_MODULE_ID BIT(4) > +#define HISI_PCIE_LOCAL_VALID_CORE_IDBIT(5) > +#define HISI_PCIE_LOCAL_VALID_PORT_IDBIT(6) > +#define HISI_PCIE_LOCAL_VALID_ERR_TYPE BIT(7) > +#define HISI_PCIE_LOCAL_VALID_ERR_SEVERITY BIT(8) > +#define HISI_PCIE_LOCAL_VALID_ERR_MISC 9 > + > +static guid_t hisi_pcie_sec_guid = > + GUID_INIT(0xB2889FC9, 0xE7D7, 0x4F9D, > + 0xA8, 0x67, 0xAF, 0x42, 0xE9, 0x8B, 0xE7, 0x72); > + > +/* > + * We pass core id and core port id to the ACPI reset method to identify > + * certain root port to reset, while the firmware reports sockets port > + * id which occurs an error. Use the macros here to do the conversion Maybe: Firmware reports the socket port ID where the error occurred. These macros convert that to the core ID and core port ID required by the ACPI reset method. But even that doesn't quite make sense because you apparently get two values (edata->core_id, edata->port_id) from firmware. > + */ > +#define HISI_PCIE_CORE_ID(v) ((v) >> 3) > +#define HISI_PCIE_PORT_ID(core, v) (((v) >> 1) + ((core) << 3)) >
[PATCH v12 2/2] PCI: hip: Add handling of HiSilicon HIP PCIe controller errors
From: Yicong Yang
The HiSilicon HIP PCIe controller is capable of handling errors
on root port and perform port reset separately at each root port.
Add error handling driver for HIP PCIe controller to log
and report recoverable errors. Perform root port reset and restore
link status after the recovery.
Following are some of the PCIe controller's recoverable errors
1. completion transmission timeout error.
2. CRS retry counter over the threshold error.
3. ECC 2 bit errors
4. AXI bresponse/rresponse errors etc.
The driver placed in the drivers/pci/controller/ because the
HIP PCIe controller does not use DWC ip.
Signed-off-by: Yicong Yang
Signed-off-by: Shiju Jose
--
drivers/pci/controller/Kconfig | 8 +
drivers/pci/controller/Makefile | 1 +
drivers/pci/controller/pcie-hisi-error.c | 336 +++
3 files changed, 345 insertions(+)
create mode 100644 drivers/pci/controller/pcie-hisi-error.c
---
drivers/pci/controller/Kconfig | 8 +
drivers/pci/controller/Makefile | 1 +
drivers/pci/controller/pcie-hisi-error.c | 327 +++
3 files changed, 336 insertions(+)
create mode 100644 drivers/pci/controller/pcie-hisi-error.c
diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index adddf21fa381..b7949b37c029 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -286,6 +286,14 @@ config PCI_LOONGSON
Say Y here if you want to enable PCI controller support on
Loongson systems.
+config PCIE_HISI_ERR
+ depends on ARM64 || COMPILE_TEST
+ depends on ACPI
+ bool "HiSilicon HIP PCIe controller error handling driver"
+ help
+ Say Y here if you want error handling support
+ for the PCIe controller's errors on HiSilicon HIP SoCs
+
source "drivers/pci/controller/dwc/Kconfig"
source "drivers/pci/controller/mobiveil/Kconfig"
source "drivers/pci/controller/cadence/Kconfig"
diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile
index efd9733ead26..90afd865bf6b 100644
--- a/drivers/pci/controller/Makefile
+++ b/drivers/pci/controller/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_PCIE_TANGO_SMP8759) += pcie-tango.o
obj-$(CONFIG_VMD) += vmd.o
obj-$(CONFIG_PCIE_BRCMSTB) += pcie-brcmstb.o
obj-$(CONFIG_PCI_LOONGSON) += pci-loongson.o
+obj-$(CONFIG_PCIE_HISI_ERR) += pcie-hisi-error.o
# pcie-hisi.o quirks are needed even without CONFIG_PCIE_DW
obj-y += dwc/
obj-y += mobiveil/
diff --git a/drivers/pci/controller/pcie-hisi-error.c
b/drivers/pci/controller/pcie-hisi-error.c
new file mode 100644
index ..9bd050cadb31
--- /dev/null
+++ b/drivers/pci/controller/pcie-hisi-error.c
@@ -0,0 +1,327 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for handling the PCIe controller errors on
+ * HiSilicon HIP SoCs.
+ *
+ * Copyright (c) 2020 HiSilicon Limited.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* HISI PCIe controller error definitions */
+#define HISI_PCIE_ERR_MISC_REGS33
+
+#define HISI_PCIE_LOCAL_VALID_VERSION BIT(0)
+#define HISI_PCIE_LOCAL_VALID_SOC_ID BIT(1)
+#define HISI_PCIE_LOCAL_VALID_SOCKET_IDBIT(2)
+#define HISI_PCIE_LOCAL_VALID_NIMBUS_IDBIT(3)
+#define HISI_PCIE_LOCAL_VALID_SUB_MODULE_IDBIT(4)
+#define HISI_PCIE_LOCAL_VALID_CORE_ID BIT(5)
+#define HISI_PCIE_LOCAL_VALID_PORT_ID BIT(6)
+#define HISI_PCIE_LOCAL_VALID_ERR_TYPE BIT(7)
+#define HISI_PCIE_LOCAL_VALID_ERR_SEVERITY BIT(8)
+#define HISI_PCIE_LOCAL_VALID_ERR_MISC 9
+
+static guid_t hisi_pcie_sec_guid =
+ GUID_INIT(0xB2889FC9, 0xE7D7, 0x4F9D,
+ 0xA8, 0x67, 0xAF, 0x42, 0xE9, 0x8B, 0xE7, 0x72);
+
+/*
+ * We pass core id and core port id to the ACPI reset method to identify
+ * certain root port to reset, while the firmware reports sockets port
+ * id which occurs an error. Use the macros here to do the conversion
+ */
+#define HISI_PCIE_CORE_ID(v) ((v) >> 3)
+#define HISI_PCIE_PORT_ID(core, v) (((v) >> 1) + ((core) << 3))
+#define HISI_PCIE_CORE_PORT_ID(v)(((v) & 7) << 1)
+
+struct hisi_pcie_error_data {
+ u64 val_bits;
+ u8 version;
+ u8 soc_id;
+ u8 socket_id;
+ u8 nimbus_id;
+ u8 sub_module_id;
+ u8 core_id;
+ u8 port_id;
+ u8 err_severity;
+ u16 err_type;
+ u8 reserv[2];
+ u32 err_misc[HISI_PCIE_ERR_MISC_REGS];
+};
+
+struct hisi_pcie_error_private {
+ struct notifier_block nb;
+ struct device *dev;
+};
+
+enum hisi_pcie_submodule_id {
+ HISI_PCIE_SUB_MODULE_ID_AP,
+ HISI_PCIE_SUB_MODULE_ID_TL,
+ HISI_PCIE_SUB_MODULE_ID_MAC,
+ HISI_PCIE_SUB_MODULE_ID_DL,
+ HISI_PCIE_SUB_MODULE_ID_SDI,
+};
+
+static const
