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

[Ian Munsie <imunsie@au1.ibm.com>]

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