Re: [PATCH 3/3] CXL: Add reset to sysfs

[Ryan Grimm <grimm@linux.vnet.ibm.com>]

On 01/15/2015 12:42 AM, Ian Munsie wrote:
> 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.
>

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.

>> +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.

>
>> +    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?
>

Added better commenting to both above.

>> +    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.

>
>> @@ -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);
> ...
>> +    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
>