Re: [PATCH 2/3] CXL: Snoop control

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

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

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.

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

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

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

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