[PATCH 1/3] CXL: Add image control to sysfs

Add reset_loads_image and reset_image_select to sysfs.

reset_image_select identifies which image will be loaded to the card on the
next PERST.  Valid entries are: "user" and "factory".

reset_loads_image defines functionality on a PERST.  Value of 0 means PERST
will not cause image load.  A power cycle is required to load the image.  Value
of 1 means PERST will cause image load.

sysfs updates the cxl struct in the driver then calls cxl_update_image_control
to write the vals in the VSEC.

Signed-off-by: Ryan Grimm <redacted>
---
 Documentation/ABI/testing/sysfs-class-cxl | 15 ++++++++
 drivers/misc/cxl/cxl.h                    |  1 +
 drivers/misc/cxl/pci.c                    | 35 ++++++++++++++++++
 drivers/misc/cxl/sysfs.c                  | 60 +++++++++++++++++++++++++++++++
 4 files changed, 111 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-cxl b/Documentation/ABI/testing/sysfs-class-cxl
index 554405e..134cfaf 100644
--- a/Documentation/ABI/testing/sysfs-class-cxl
+++ b/Documentation/ABI/testing/sysfs-class-cxl

@@ -127,3 +127,18 @@ Contact:        linuxppc-dev@lists.ozlabs.org
 Description:    read only
                 Will return "user" or "factory" depending on the image loaded
                 onto the card.
+
+What:           /sys/class/cxl/<card>/reset_image_select
+Date:           December 2014
+Contact:        linuxppc-dev@lists.ozlabs.org
+Description:    read/write
+                Identifies which image will be loaded to the card on the next
+                PERST.  Valid entries are: "user" and "factory".
+
+What:           /sys/class/cxl/<card>/reset_loads_image
+Date:           December 2014
+Contact:        linuxppc-dev@lists.ozlabs.org
+Description:    read/write
+                Value of 0 means PERST will not cause image load.  A power
+                cycle is required to load the image.  Value of 1 means PERST
+                will cause image load.

diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
index 0df0438..518c4c6 100644
--- a/drivers/misc/cxl/cxl.h
+++ b/drivers/misc/cxl/cxl.h

@@ -488,6 +488,7 @@ void cxl_release_one_irq(struct cxl *adapter, int hwirq);
 int cxl_alloc_irq_ranges(struct cxl_irq_ranges *irqs, struct cxl *adapter, unsigned int num);
 void cxl_release_irq_ranges(struct cxl_irq_ranges *irqs, struct cxl *adapter);
 int cxl_setup_irq(struct cxl *adapter, unsigned int hwirq, unsigned int virq);
+int cxl_update_image_control(struct cxl *adapter);
 
 /* common == phyp + powernv */
 struct cxl_process_element_common {

diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index f801c28..26cacc1 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c

@@ -362,6 +362,41 @@ int cxl_setup_irq(struct cxl *adapter, unsigned int hwirq,
 	return pnv_cxl_ioda_msi_setup(dev, hwirq, virq);
 }
 
+int cxl_update_image_control(struct cxl *adapter)
+{
+	struct pci_dev *dev = to_pci_dev(adapter->dev.parent);
+	int rc;
+	int vsec;
+	u8 image_state;
+
+	if (!(vsec = find_cxl_vsec(dev))) {
+		dev_err(&dev->dev, "ABORTING: CXL VSEC not found!\n");
+		return -ENODEV;
+	}
+
+	if ((rc = CXL_READ_VSEC_IMAGE_STATE(dev, vsec, &image_state))) {
+		dev_err(&dev->dev, "failed to read image state: %i\n", rc);
+		return rc;
+	}
+
+	if (adapter->perst_loads_image)
+		image_state |= CXL_VSEC_PERST_LOADS_IMAGE;
+	else
+		image_state &= ~CXL_VSEC_PERST_LOADS_IMAGE;
+
+	if (adapter->perst_select_user)
+		image_state |= CXL_VSEC_PERST_SELECT_USER;
+	else
+		image_state &= ~CXL_VSEC_PERST_SELECT_USER;
+
+	if ((rc = CXL_WRITE_VSEC_IMAGE_STATE(dev, vsec, image_state))) {
+		dev_err(&dev->dev, "failed to update image control: %i\n", rc);
+		return rc;
+	}
+
+	return 0;
+}
+
 int cxl_alloc_one_irq(struct cxl *adapter)
 {
 	struct pci_dev *dev = to_pci_dev(adapter->dev.parent);

diff --git a/drivers/misc/cxl/sysfs.c b/drivers/misc/cxl/sysfs.c
index 461bdbd..06f554b 100644
--- a/drivers/misc/cxl/sysfs.c
+++ b/drivers/misc/cxl/sysfs.c

@@ -56,11 +56,71 @@ static ssize_t image_loaded_show(struct device *device,
 	return scnprintf(buf, PAGE_SIZE, "factory\n");
 }
 
+static ssize_t reset_loads_image_show(struct device *device,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct cxl *adapter = to_cxl_adapter(device);
+	return sprintf(buf, "%d\n", adapter->perst_loads_image);
+}
+
+static ssize_t reset_loads_image_store(struct device *device,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct cxl *adapter = to_cxl_adapter(device);
+	unsigned long val;
+	int rc;
+
+        if (kstrtoul(buf, 0, &val) < 0)
+                return -EINVAL;
+
+        adapter->perst_loads_image = !!val;
+	if ((rc = cxl_update_image_control(adapter)))
+		return rc;
+
+	return count;
+}
+
+static ssize_t reset_image_select_show(struct device *device,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct cxl *adapter = to_cxl_adapter(device);
+
+	if (adapter->perst_select_user)
+		return scnprintf(buf, PAGE_SIZE, "user\n");
+
+	return scnprintf(buf, PAGE_SIZE, "factory\n");
+}
+
+static ssize_t reset_image_select_store(struct device *device,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct cxl *adapter = to_cxl_adapter(device);
+	int rc;
+
+	if (!strncmp(buf, "user", 4))
+		adapter->perst_select_user = true;
+	else if (!strncmp(buf, "factory", 7))
+		adapter->perst_select_user = false;
+	else
+                return -EINVAL;
+
+	if ((rc = cxl_update_image_control(adapter)))
+		return rc;
+
+	return count;
+}
+
 static struct device_attribute adapter_attrs[] = {
 	__ATTR_RO(caia_version),
 	__ATTR_RO(psl_revision),
 	__ATTR_RO(base_image),
 	__ATTR_RO(image_loaded),
+	__ATTR_RW(reset_loads_image),
+	__ATTR_RW(reset_image_select),
 };
 
 

-- 
1.9.1

Add mode to opal call.  SNOOP control turns CAPP unit snooping on/off.  This is
needed for the following reset patch, which turns snoops off in the CAPP
recovery path.

Signed-off-by: Ryan Grimm <redacted>
---
 arch/powerpc/include/asm/opal.h           | 12 ++++++++++++
 arch/powerpc/include/asm/pnv-pci.h        |  2 +-
 arch/powerpc/platforms/powernv/pci-ioda.c |  4 ++--
 drivers/misc/cxl/pci.c                    |  6 +++++-
 4 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 5d073e5..965e232 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h

@@ -676,6 +676,18 @@ enum {
 	OPAL_PHB3_NUM_PEST_REGS = 256
 };
 
+/* CAPI modes for PHB */
+enum {
+        OPAL_PHB_CAPI_MODE_PCIE         = 0,
+        OPAL_PHB_CAPI_MODE_CAPI         = 1,
+        OPAL_PHB_CAPI_MODE_SNOOP_OFF    = 2,
+        OPAL_PHB_CAPI_MODE_SNOOP_ON     = 3,
+};
+
+/* CAPI feature flags (in device-tree) */
+#define OPAL_PHB_CAPI_FLAG_SNOOP_CONTROL        0x00000001
+#define OPAL_PHB_CAPI_FLAG_REVERT_TO_PCIE       0x00000002
+
 struct OpalIoPhbErrorCommon {
 	__be32 version;
 	__be32 ioType;

diff --git a/arch/powerpc/include/asm/pnv-pci.h b/arch/powerpc/include/asm/pnv-pci.h
index f09a22f..e18082c 100644
--- a/arch/powerpc/include/asm/pnv-pci.h
+++ b/arch/powerpc/include/asm/pnv-pci.h

@@ -13,7 +13,7 @@
 #include <linux/pci.h>
 #include <misc/cxl.h>
 
-int pnv_phb_to_cxl(struct pci_dev *dev);
+int pnv_phb_to_cxl(struct pci_dev *dev, uint64_t mode);
 int pnv_cxl_ioda_msi_setup(struct pci_dev *dev, unsigned int hwirq,
 			   unsigned int virq);
 int pnv_cxl_alloc_hwirqs(struct pci_dev *dev, int num);

diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 468a0f2..acacc1b 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c

@@ -1363,7 +1363,7 @@ struct device_node *pnv_pci_to_phb_node(struct pci_dev *dev)
 }
 EXPORT_SYMBOL(pnv_pci_to_phb_node);
 
-int pnv_phb_to_cxl(struct pci_dev *dev)
+int pnv_phb_to_cxl(struct pci_dev *dev, uint64_t mode)
 {
 	struct pci_controller *hose = pci_bus_to_host(dev->bus);
 	struct pnv_phb *phb = hose->private_data;

@@ -1376,7 +1376,7 @@ int pnv_phb_to_cxl(struct pci_dev *dev)
 
 	pe_info(pe, "Switching PHB to CXL\n");
 
-	rc = opal_pci_set_phb_cxl_mode(phb->opal_id, 1, pe->pe_number);
+	rc = opal_pci_set_phb_cxl_mode(phb->opal_id, mode, pe->pe_number);
 	if (rc)
 		dev_err(&dev->dev, "opal_pci_set_phb_cxl_mode failed: %i\n", rc);
 

diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 26cacc1..9aa95f9 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c

@@ -924,7 +924,7 @@ static struct cxl *cxl_init_adapter(struct pci_dev *dev)
 	if ((rc = init_implementation_adapter_regs(adapter, dev)))
 		goto err3;
 
-	if ((rc = pnv_phb_to_cxl(dev)))
+	if ((rc = pnv_phb_to_cxl(dev, OPAL_PHB_CAPI_MODE_CAPI)))
 		goto err3;
 
 	if ((rc = cxl_register_psl_err_irq(adapter)))

@@ -1009,6 +1009,10 @@ static int cxl_probe(struct pci_dev *dev, const struct pci_device_id *id)
 			dev_err(&dev->dev, "AFU %i failed to initialise: %i\n", slice, rc);
 	}
 
+	if ((rc = pnv_phb_to_cxl(dev, OPAL_PHB_CAPI_MODE_SNOOP_ON))) {
+		dev_err(&dev->dev, "enable capp snoops: %i\n", rc);
+	}
+
 	return 0;
 }
 

-- 
1.9.1

Excerpts from Ryan Grimm's message of 2015-01-15 13:56:40 +1100:

Add mode to opal call.  SNOOP control turns CAPP unit snooping on/off.  This is
needed for the following reset patch, which turns snoops off in the CAPP
recovery path.

Looking at patch 3 in this series I think this description needs to be
updated, as it doesn't seem to turn off snoops?

+/* CAPI modes for PHB */
+enum {
+        OPAL_PHB_CAPI_MODE_PCIE         = 0,
+        OPAL_PHB_CAPI_MODE_CAPI         = 1,
+        OPAL_PHB_CAPI_MODE_SNOOP_OFF    = 2,
+        OPAL_PHB_CAPI_MODE_SNOOP_ON     = 3,
+};

Spaces have been used for indention here

+/* CAPI feature flags (in device-tree) */
+#define OPAL_PHB_CAPI_FLAG_SNOOP_CONTROL        0x00000001
+#define OPAL_PHB_CAPI_FLAG_REVERT_TO_PCIE       0x00000002

It doesn't look like these are used?

-int pnv_phb_to_cxl(struct pci_dev *dev)
+int pnv_phb_to_cxl(struct pci_dev *dev, uint64_t mode)

Should we rename this function since it no longer just sets the PHB to
CXL mode? Maybe something like pnv_phb_set_cxl_mode?

+    if ((rc = pnv_phb_to_cxl(dev, OPAL_PHB_CAPI_MODE_SNOOP_ON))) {
+        dev_err(&dev->dev, "enable capp snoops: %i\n", rc);
+    }

Ok, we turn on snooping here, but I don't see where we turned it off -
has patch 3 changed so that never happens?

Also - why this late in in the init sequence? Not saying it's wrong, just
wondering if this has to happen after all the AFUs have been initialised, or if
it can happen earlier in the adapter initialisation, like when we set
the PHB to capi mode?


Cheers,
-Ian

On 01/15/2015 12:16 AM, Ian Munsie wrote:

Excerpts from Ryan Grimm's message of 2015-01-15 13:56:40 +1100:

quoted

Add mode to opal call.  SNOOP control turns CAPP unit snooping on/off.  This is
needed for the following reset patch, which turns snoops off in the CAPP
recovery path.

Looking at patch 3 in this series I think this description needs to be
updated, as it doesn't seem to turn off snoops?

OK, will make commit message more clear.

Maybe I'll rename the first line to CXL: Enable CAPP recovery, since 
that's what it does.

quoted

+/* CAPI modes for PHB */
+enum {
+        OPAL_PHB_CAPI_MODE_PCIE         = 0,
+        OPAL_PHB_CAPI_MODE_CAPI         = 1,
+        OPAL_PHB_CAPI_MODE_SNOOP_OFF    = 2,
+        OPAL_PHB_CAPI_MODE_SNOOP_ON     = 3,
+};

Spaces have been used for indention here

Fixed.

quoted

+/* CAPI feature flags (in device-tree) */
+#define OPAL_PHB_CAPI_FLAG_SNOOP_CONTROL        0x00000001
+#define OPAL_PHB_CAPI_FLAG_REVERT_TO_PCIE       0x00000002

It doesn't look like these are used?

Yeah.

quoted

-int pnv_phb_to_cxl(struct pci_dev *dev)
+int pnv_phb_to_cxl(struct pci_dev *dev, uint64_t mode)

Should we rename this function since it no longer just sets the PHB to
CXL mode? Maybe something like pnv_phb_set_cxl_mode?

Good call.  The names are updated in the skiboot code base...missed this 
one.

quoted

+    if ((rc = pnv_phb_to_cxl(dev, OPAL_PHB_CAPI_MODE_SNOOP_ON))) {
+        dev_err(&dev->dev, "enable capp snoops: %i\n", rc);
+    }

Ok, we turn on snooping here, but I don't see where we turned it off -
has patch 3 changed so that never happens?

snoops are disabled by Sapphire in the CAPP recovery path.

Also - why this late in in the init sequence? Not saying it's wrong, just
wondering if this has to happen after all the AFUs have been initialised, or if
it can happen earlier in the adapter initialisation, like when we set
the PHB to capi mode?

It has no effect when the driver is bound initially since Sapphire 
already turns on snoops as part of the phb to cxl procedure.

I did it this way to handle CAPP recovery.  The driver doesn't know that 
capp recovery happened...EEH unbinds the driver, Sapphire does the CAPP 
recovery procedure, then EEH rebinds the driver.

The last step in capp recovery is to turn on snoops.  I'll put a comment 
to explain that better...but, sure it could be earlier in the sequence.

-Ryan

Cheers,
-Ian

This allows an image to be downloaded to the flash without rebooting the
machine.  The driver perform a PERST, which results in FPGA image downloaded to
flash and the CAPP unit enters recovery.  CAPP recovery triggers an HMI, which
is handled by EEH in Linux.  EEH removes the driver, calls into Sapphire to
reinitialize the PHB, and then loads the driver.

reset_image_select must be set to "user" and reset_load_image set to 1.  The
driver writes "user" to the vsec if a user image was loaded.  It writes 1 to
reset_load_image on initialization by default.  Other values could be used by
hand for debugging purposes.

Signed-off-by: Ryan Grimm <redacted>
---
 Documentation/ABI/testing/sysfs-class-cxl |  6 +++++
 drivers/misc/cxl/cxl.h                    |  1 +
 drivers/misc/cxl/pci.c                    | 38 +++++++++++++++++++++++++++++--
 drivers/misc/cxl/sysfs.c                  | 13 +++++++++++
 4 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-cxl b/Documentation/ABI/testing/sysfs-class-cxl
index 134cfaf..389cf24 100644
--- a/Documentation/ABI/testing/sysfs-class-cxl
+++ b/Documentation/ABI/testing/sysfs-class-cxl

@@ -142,3 +142,9 @@ Description:    read/write
                 Value of 0 means PERST will not cause image load.  A power
                 cycle is required to load the image.  Value of 1 means PERST
                 will cause image load.
+
+What:           /sys/class/cxl/<card>/reset
+Date:           October 2014
+Contact:        linuxppc-dev@lists.ozlabs.org
+Description:    write only
+                Writing 1 here will issue a PERST to card.

diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
index 518c4c6..6a6a487 100644
--- a/drivers/misc/cxl/cxl.h
+++ b/drivers/misc/cxl/cxl.h

@@ -489,6 +489,7 @@ int cxl_alloc_irq_ranges(struct cxl_irq_ranges *irqs, struct cxl *adapter, unsig
 void cxl_release_irq_ranges(struct cxl_irq_ranges *irqs, struct cxl *adapter);
 int cxl_setup_irq(struct cxl *adapter, unsigned int hwirq, unsigned int virq);
 int cxl_update_image_control(struct cxl *adapter);
+int cxl_reset(struct cxl *adapter);
 
 /* common == phyp + powernv */
 struct cxl_process_element_common {

diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 9aa95f9..a93daa0 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c

@@ -21,6 +21,7 @@
 #include <asm/msi_bitmap.h>
 #include <asm/pci-bridge.h> /* for struct pci_controller */
 #include <asm/pnv-pci.h>
+#include <asm/io.h>
 
 #include "cxl.h"
 

@@ -742,6 +743,36 @@ static void cxl_remove_afu(struct cxl_afu *afu)
 	device_unregister(&afu->dev);
 }
 
+int cxl_reset(struct cxl *adapter)
+{
+	struct pci_dev *dev = to_pci_dev(adapter->dev.parent);
+	int rc;
+	int i;
+	u32 val;
+
+	dev_info(&dev->dev, "CXL reset\n");
+
+	for (i = 0; i < adapter->slices; i++)
+		cxl_remove_afu(adapter->afu[i]);
+
+	if ((rc = pci_set_pcie_reset_state(dev, pcie_warm_reset))) {
+		dev_err(&dev->dev, "cxl: pcie_warm_reset failed\n");
+		return rc;
+	}
+
+	/* Do mmio read to trigger EEH.  Retry for a few seconds. */
+	i = 0;
+        while ((val = mmio_read32be(adapter->p1_mmio) != 0xffffffff) &&
+		(i < 5)) {
+                msleep(500);
+		i++;
+        }
+
+        if (val != 0xffffffff)
+                dev_err(&dev->dev, "cxl: PERST failed to trigger EEH\n");
+
+	return rc;
+}
 
 static int cxl_map_adapter_regs(struct cxl *adapter, struct pci_dev *dev)
 {

@@ -806,8 +837,8 @@ static int cxl_read_vsec(struct cxl *adapter, struct pci_dev *dev)
 	CXL_READ_VSEC_BASE_IMAGE(dev, vsec, &adapter->base_image);
 	CXL_READ_VSEC_IMAGE_STATE(dev, vsec, &image_state);
 	adapter->user_image_loaded = !!(image_state & CXL_VSEC_USER_IMAGE_LOADED);
-	adapter->perst_loads_image = !!(image_state & CXL_VSEC_PERST_LOADS_IMAGE);
-	adapter->perst_select_user = !!(image_state & CXL_VSEC_PERST_SELECT_USER);
+	adapter->perst_loads_image = true;
+	adapter->perst_select_user = !!(image_state & CXL_VSEC_USER_IMAGE_LOADED);
 
 	CXL_READ_VSEC_NAFUS(dev, vsec, &adapter->slices);
 	CXL_READ_VSEC_AFU_DESC_OFF(dev, vsec, &afu_desc_off);

@@ -915,6 +946,9 @@ static struct cxl *cxl_init_adapter(struct pci_dev *dev)
 	if ((rc = cxl_vsec_looks_ok(adapter, dev)))
 		goto err2;
 
+	if ((rc = cxl_update_image_control(adapter)))
+		goto err2;
+
 	if ((rc = cxl_map_adapter_regs(adapter, dev)))
 		goto err2;
 

diff --git a/drivers/misc/cxl/sysfs.c b/drivers/misc/cxl/sysfs.c
index 06f554b..7ebd7e3 100644
--- a/drivers/misc/cxl/sysfs.c
+++ b/drivers/misc/cxl/sysfs.c

@@ -56,6 +56,18 @@ static ssize_t image_loaded_show(struct device *device,
 	return scnprintf(buf, PAGE_SIZE, "factory\n");
 }
 
+static ssize_t reset_adapter_store(struct device *device,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	struct cxl *adapter = to_cxl_adapter(device);
+	int rc;
+
+	if ((rc = cxl_reset(adapter)))
+		return rc;
+	return count;
+}
+
 static ssize_t reset_loads_image_show(struct device *device,
 				 struct device_attribute *attr,
 				 char *buf)

@@ -121,6 +133,7 @@ static struct device_attribute adapter_attrs[] = {
 	__ATTR_RO(image_loaded),
 	__ATTR_RW(reset_loads_image),
 	__ATTR_RW(reset_image_select),
+	__ATTR(reset, S_IWUSR, NULL, reset_adapter_store),
 };
 
 

-- 
1.9.1

Excerpts from Ryan Grimm's message of 2015-01-15 13:56:41 +1100:

This allows an image to be downloaded to the flash without rebooting the
machine.  The driver perform a PERST, which results in FPGA image downloaded to
flash and the CAPP unit enters recovery.  CAPP recovery triggers an HMI, which
is handled by EEH in Linux.  EEH removes the driver, calls into Sapphire to
reinitialize the PHB, and then loads the driver.

reset_image_select must be set to "user" and reset_load_image set to 1.  The
driver writes "user" to the vsec if a user image was loaded.  It writes 1 to
reset_load_image on initialization by default.  Other values could be used by
hand for debugging purposes.

That last paragraph will need to be updated if we merge those two sysfs
files into one. Might as well mention an example of why someone might do
a reset with no image selected for reload, e.g. the PSL trace arrays are
preserved, which can be read out through debugfs after the card comes
back up.

+What:           /sys/class/cxl/<card>/reset
+Date:           October 2014
+Contact:        linuxppc-dev@lists.ozlabs.org
+Description:    write only
+                Writing 1 here will issue a PERST to card.

"..., which may cause the card to reload the FPGA image depending on the
settings of reset_image_select."

+    if ((rc = pci_set_pcie_reset_state(dev, pcie_warm_reset))) {

Can you add a comment here to explain why we first do a warm reset?

+        dev_err(&dev->dev, "cxl: pcie_warm_reset failed\n");
+        return rc;
+    }
+
+    /* Do mmio read to trigger EEH.  Retry for a few seconds. */

This seems a little unusual - can you expand this comment a little to
explain *why* we are using this method to trigger an EEH and reset the
card?

+    i = 0;
+        while ((val = mmio_read32be(adapter->p1_mmio) != 0xffffffff) &&
+        (i < 5)) {
+                msleep(500);
+        i++;
+        }
+
+        if (val != 0xffffffff)
+                dev_err(&dev->dev, "cxl: PERST failed to trigger EEH\n");
+
+    return rc;

Some of the indentation here is a bit funky - some lines are using tabs,
others are using spaces.

...

+    if ((rc = cxl_update_image_control(adapter)))
+        goto err2;

Thanks - that seems like a better default than what we had before,
should make things more stable :)



Cheers,
-Ian

quoted

@@ -806,8 +837,8 @@ static int cxl_read_vsec(struct cxl *adapter, struct pci_dev *dev)
     CXL_READ_VSEC_BASE_IMAGE(dev, vsec, &adapter->base_image);
     CXL_READ_VSEC_IMAGE_STATE(dev, vsec, &image_state);
     adapter->user_image_loaded = !!(image_state & CXL_VSEC_USER_IMAGE_LOADED);
-    adapter->perst_loads_image = !!(image_state & CXL_VSEC_PERST_LOADS_IMAGE);
-    adapter->perst_select_user = !!(image_state & CXL_VSEC_PERST_SELECT_USER);
+    adapter->perst_loads_image = true;
+    adapter->perst_select_user = !!(image_state & CXL_VSEC_USER_IMAGE_LOADED);

...

quoted

+    if ((rc = cxl_update_image_control(adapter)))
+        goto err2;

Thanks - that seems like a better default than what we had before,
should make things more stable :)

In fact, would you mind pulling this part out into a separate patch? It
seems like a serious contender to go to stable as it might help with
cards that get into a funny state and don't come back up properly after
a reboot (symptoms are that the adapter wide tlbia / slbia times out and
the driver aborts initialisation).

Cheers,
-Ian

On 01/15/2015 12:42 AM, Ian Munsie wrote:

Excerpts from Ryan Grimm's message of 2015-01-15 13:56:41 +1100:

quoted

This allows an image to be downloaded to the flash without rebooting the
machine.  The driver perform a PERST, which results in FPGA image downloaded to
flash and the CAPP unit enters recovery.  CAPP recovery triggers an HMI, which
is handled by EEH in Linux.  EEH removes the driver, calls into Sapphire to
reinitialize the PHB, and then loads the driver.

reset_image_select must be set to "user" and reset_load_image set to 1.  The
driver writes "user" to the vsec if a user image was loaded.  It writes 1 to
reset_load_image on initialization by default.  Other values could be used by
hand for debugging purposes.

That last paragraph will need to be updated if we merge those two sysfs
files into one. Might as well mention an example of why someone might do
a reset with no image selected for reload, e.g. the PSL trace arrays are
preserved, which can be read out through debugfs after the card comes
back up.

OK, fixed that up a bit.  Let me know if the commit logs and 
documentations make sense.  There's a bit of overlap and hopefully it's 
clear now.

quoted

+What:           /sys/class/cxl/<card>/reset
+Date:           October 2014
+Contact:        linuxppc-dev@lists.ozlabs.org
+Description:    write only
+                Writing 1 here will issue a PERST to card.

"..., which may cause the card to reload the FPGA image depending on the
settings of reset_image_select."

Sure, can be explicit about that.

quoted

+    if ((rc = pci_set_pcie_reset_state(dev, pcie_warm_reset))) {

Can you add a comment here to explain why we first do a warm reset?

quoted

+        dev_err(&dev->dev, "cxl: pcie_warm_reset failed\n");
+        return rc;
+    }
+
+    /* Do mmio read to trigger EEH.  Retry for a few seconds. */

This seems a little unusual - can you expand this comment a little to
explain *why* we are using this method to trigger an EEH and reset the
card?

Added better commenting to both above.

quoted

+    i = 0;
+        while ((val = mmio_read32be(adapter->p1_mmio) != 0xffffffff) &&
+        (i < 5)) {
+                msleep(500);
+        i++;
+        }
+
+        if (val != 0xffffffff)
+                dev_err(&dev->dev, "cxl: PERST failed to trigger EEH\n");
+
+    return rc;

Some of the indentation here is a bit funky - some lines are using tabs,
others are using spaces.

Ouch, yep, fixed.

quoted

@@ -806,8 +837,8 @@ static int cxl_read_vsec(struct cxl *adapter, struct pci_dev *dev)
      CXL_READ_VSEC_BASE_IMAGE(dev, vsec, &adapter->base_image);
      CXL_READ_VSEC_IMAGE_STATE(dev, vsec, &image_state);
      adapter->user_image_loaded = !!(image_state & CXL_VSEC_USER_IMAGE_LOADED);
-    adapter->perst_loads_image = !!(image_state & CXL_VSEC_PERST_LOADS_IMAGE);
-    adapter->perst_select_user = !!(image_state & CXL_VSEC_PERST_SELECT_USER);
+    adapter->perst_loads_image = true;
+    adapter->perst_select_user = !!(image_state & CXL_VSEC_USER_IMAGE_LOADED);

...

quoted

+    if ((rc = cxl_update_image_control(adapter)))
+        goto err2;

Thanks - that seems like a better default than what we had before,
should make things more stable :)

Yeah for sure.

-Ryan

Cheers,
-Ian

Excerpts from Ryan Grimm's message of 2015-01-15 13:56:41 +1100:

+What:           /sys/class/cxl/<card>/reset
+Date:           October 2014
+Contact:        linuxppc-dev@lists.ozlabs.org
+Description:    write only
+                Writing 1 here will issue a PERST to card.

...

+static ssize_t reset_adapter_store(struct device *device,
+                   struct device_attribute *attr,
+                   const char *buf, size_t count)
+{
+    struct cxl *adapter = to_cxl_adapter(device);
+    int rc;
+
+    if ((rc = cxl_reset(adapter)))
+        return rc;
+    return count;
+}

Looks like we reset the card no matter what is written to that file?

I like the description better - add a test here to match what it says.

Cheers,
-Ian

Excerpts from Ryan Grimm's message of 2015-01-15 13:56:39 +1100:

Add reset_loads_image and reset_image_select to sysfs.

reset_image_select identifies which image will be loaded to the card on the
next PERST.  Valid entries are: "user" and "factory".

reset_loads_image defines functionality on a PERST.  Value of 0 means PERST
will not cause image load.  A power cycle is required to load the image.  Value
of 1 means PERST will cause image load.

sysfs updates the cxl struct in the driver then calls cxl_update_image_control
to write the vals in the VSEC.

Let's combine both of these into a single sysfs file, with "none",
"user" and "factory" options and have the show & read functions handle
mapping those three options to the two bits in the register.

Of the two names I'd probably go with reset_image_select.

+What:           /sys/class/cxl/<card>/reset_loads_image
+Date:           December 2014
+Contact:        linuxppc-dev@lists.ozlabs.org
+Description:    read/write
+                Value of 0 means PERST will not cause image load.  A power
+                cycle is required to load the image.  Value of 1 means PERST
+                will cause image load.

It also seems to be that having this disabled also means that PERST
doesn't fully reset the card. Might want to clarify that somewhat and
recommend it only be disabled for debugging purposes (e.g. to retain
the contents of the PSL trace arrays across a reset), and to always
enable it for production.

At the moment we don't set it at boot - we just go with whatever the
card is already set to do. I'm thinking it might be a good idea to
always set this bit on boot so the only time it's disabled is if a user
has explicitly gone and disabled it.

+static ssize_t reset_loads_image_show(struct device *device,
+                 struct device_attribute *attr,
+                 char *buf)
+{
+    struct cxl *adapter = to_cxl_adapter(device);
+    return sprintf(buf, "%d\n", adapter->perst_loads_image);

We've used scnprintf for the other sysfs reads in this file, why sprintf
here?

+static ssize_t reset_loads_image_store(struct device *device,
+                 struct device_attribute *attr,
+                 const char *buf, size_t count)
+{
+    struct cxl *adapter = to_cxl_adapter(device);
+    unsigned long val;
+    int rc;
+
+        if (kstrtoul(buf, 0, &val) < 0)
+                return -EINVAL;
+
+        adapter->perst_loads_image = !!val;
+    if ((rc = cxl_update_image_control(adapter)))
+        return rc;

Seems to be some indentation mismatches here - some lines are using
spaces other are using tabs. Please use tabs for everything.

+static ssize_t reset_image_select_store(struct device *device,
+                 struct device_attribute *attr,
+                 const char *buf, size_t count)
+{
+    struct cxl *adapter = to_cxl_adapter(device);
+    int rc;
+
+    if (!strncmp(buf, "user", 4))
+        adapter->perst_select_user = true;
+    else if (!strncmp(buf, "factory", 7))
+        adapter->perst_select_user = false;
+    else
+                return -EINVAL;

More indentation mismatches here.


Cheers,
-Ian

Excerpts from Ian Munsie's message of 2015-01-15 15:41:24 +1100:

At the moment we don't set it at boot - we just go with whatever the
card is already set to do. I'm thinking it might be a good idea to
always set this bit on boot so the only time it's disabled is if a user
has explicitly gone and disabled it.

While I think of it - if we change this on boot we should also change
reset_image_select to match the currently loaded image. e.g. if
reset_loads_image has defaulted to off and reset_image_select has
defaulted to factory, but the user image has been loaded - that way we
avoid unexpectedly switching to factory if the card gets reset.

Cheers,
-Ian

While I think of it - if we change this on boot we should also change
reset_image_select to match the currently loaded image. e.g. if
reset_loads_image has defaulted to off and reset_image_select has
defaulted to factory, but the user image has been loaded - that way we
avoid unexpectedly switching to factory if the card gets reset.

Nevermind - I see you have done exactly this in patch 3 :-)

-Ian

On Thu, 2015-01-15 at 15:41 +1100, Ian Munsie wrote:

Excerpts from Ryan Grimm's message of 2015-01-15 13:56:39 +1100:

quoted

Add reset_loads_image and reset_image_select to sysfs.

reset_image_select identifies which image will be loaded to the card on the
next PERST.  Valid entries are: "user" and "factory".

reset_loads_image defines functionality on a PERST.  Value of 0 means PERST
will not cause image load.  A power cycle is required to load the image.  Value
of 1 means PERST will cause image load.

sysfs updates the cxl struct in the driver then calls cxl_update_image_control
to write the vals in the VSEC.

Let's combine both of these into a single sysfs file, with "none",
"user" and "factory" options and have the show & read functions handle
mapping those three options to the two bits in the register.

Of the two names I'd probably go with reset_image_select.

Three words, all can be verbs, two can be nouns, it's not too clear.

Maybe "load_image_on_perst" ?

cheers

Excerpts from Michael Ellerman's message of 2015-01-15 16:07:17 +1100:

quoted

Of the two names I'd probably go with reset_image_select.

Three words, all can be verbs, two can be nouns, it's not too clear.

Maybe "load_image_on_perst" ?

Works for me :)

Cheers,
-Ian

Ian,

Thanks for reviewing!

On 01/14/2015 11:41 PM, Ian Munsie wrote:

Excerpts from Ryan Grimm's message of 2015-01-15 13:56:39 +1100:

quoted

Add reset_loads_image and reset_image_select to sysfs.

reset_image_select identifies which image will be loaded to the card on the
next PERST.  Valid entries are: "user" and "factory".

reset_loads_image defines functionality on a PERST.  Value of 0 means PERST
will not cause image load.  A power cycle is required to load the image.  Value
of 1 means PERST will cause image load.

sysfs updates the cxl struct in the driver then calls cxl_update_image_control
to write the vals in the VSEC.

Let's combine both of these into a single sysfs file, with "none",
"user" and "factory" options and have the show & read functions handle
mapping those three options to the two bits in the register.

I like that idea!

Of the two names I'd probably go with reset_image_select.

quoted

+What:           /sys/class/cxl/<card>/reset_loads_image
+Date:           December 2014
+Contact:        linuxppc-dev@lists.ozlabs.org
+Description:    read/write
+                Value of 0 means PERST will not cause image load.  A power
+                cycle is required to load the image.  Value of 1 means PERST
+                will cause image load.

It also seems to be that having this disabled also means that PERST
doesn't fully reset the card. Might want to clarify that somewhat and
recommend it only be disabled for debugging purposes (e.g. to retain
the contents of the PSL trace arrays across a reset), and to always
enable it for production.

Yeah, that is the main reason you'd disable it.  Will add that info to 
the doc.

At the moment we don't set it at boot - we just go with whatever the
card is already set to do. I'm thinking it might be a good idea to
always set this bit on boot so the only time it's disabled is if a user
has explicitly gone and disabled it.

quoted

+static ssize_t reset_loads_image_show(struct device *device,
+                 struct device_attribute *attr,
+                 char *buf)
+{
+    struct cxl *adapter = to_cxl_adapter(device);
+    return sprintf(buf, "%d\n", adapter->perst_loads_image);

We've used scnprintf for the other sysfs reads in this file, why sprintf
here?

No reason, scnprintf is safer so fixed.

quoted

+static ssize_t reset_loads_image_store(struct device *device,
+                 struct device_attribute *attr,
+                 const char *buf, size_t count)
+{
+    struct cxl *adapter = to_cxl_adapter(device);
+    unsigned long val;
+    int rc;
+
+        if (kstrtoul(buf, 0, &val) < 0)
+                return -EINVAL;
+
+        adapter->perst_loads_image = !!val;
+    if ((rc = cxl_update_image_control(adapter)))
+        return rc;

Seems to be some indentation mismatches here - some lines are using
spaces other are using tabs. Please use tabs for everything.

Fixed.

quoted

+static ssize_t reset_image_select_store(struct device *device,
+                 struct device_attribute *attr,
+                 const char *buf, size_t count)
+{
+    struct cxl *adapter = to_cxl_adapter(device);
+    int rc;
+
+    if (!strncmp(buf, "user", 4))
+        adapter->perst_select_user = true;
+    else if (!strncmp(buf, "factory", 7))
+        adapter->perst_select_user = false;
+    else
+                return -EINVAL;

More indentation mismatches here.

Fixed.

-Ryan

Cheers,
-Ian