| Message ID | 20191203034655.51561-8-alastair@au1.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for OpenCAPI SCM devices | expand |
Le 03/12/2019 à 04:46, Alastair D'Silva a écrit : > From: Alastair D'Silva <alastair@d-silva.org> > > Add functions to map/unmap LPC memory > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org> > --- > drivers/misc/ocxl/config.c | 4 +++ > drivers/misc/ocxl/core.c | 50 +++++++++++++++++++++++++++++++ > drivers/misc/ocxl/ocxl_internal.h | 3 ++ > include/misc/ocxl.h | 18 +++++++++++ > 4 files changed, 75 insertions(+) > > diff --git a/drivers/misc/ocxl/config.c b/drivers/misc/ocxl/config.c > index c8e19bfb5ef9..fb0c3b6f8312 100644 > --- a/drivers/misc/ocxl/config.c > +++ b/drivers/misc/ocxl/config.c > @@ -568,6 +568,10 @@ static int read_afu_lpc_memory_info(struct pci_dev *dev, > afu->special_purpose_mem_size = > total_mem_size - lpc_mem_size; > } > + > + dev_info(&dev->dev, "Probed LPC memory of %#llx bytes and special purpose memory of %#llx bytes\n", > + afu->lpc_mem_size, afu->special_purpose_mem_size); > + > return 0; > } > > diff --git a/drivers/misc/ocxl/core.c b/drivers/misc/ocxl/core.c > index 2531c6cf19a0..98611faea219 100644 > --- a/drivers/misc/ocxl/core.c > +++ b/drivers/misc/ocxl/core.c > @@ -210,6 +210,55 @@ static void unmap_mmio_areas(struct ocxl_afu *afu) > release_fn_bar(afu->fn, afu->config.global_mmio_bar); > } > > +int ocxl_afu_map_lpc_mem(struct ocxl_afu *afu) > +{ > + struct pci_dev *dev = to_pci_dev(afu->fn->dev.parent); > + > + if ((afu->config.lpc_mem_size + afu->config.special_purpose_mem_size) == 0) > + return 0; > + > + afu->lpc_base_addr = ocxl_link_lpc_map(afu->fn->link, dev); > + if (afu->lpc_base_addr == 0) > + return -EINVAL; > + > + if (afu->config.lpc_mem_size) { > + afu->lpc_res.start = afu->lpc_base_addr + afu->config.lpc_mem_offset; > + afu->lpc_res.end = afu->lpc_res.start + afu->config.lpc_mem_size - 1; > + } > + > + if (afu->config.special_purpose_mem_size) { > + afu->special_purpose_res.start = afu->lpc_base_addr + > + afu->config.special_purpose_mem_offset; > + afu->special_purpose_res.end = afu->special_purpose_res.start + > + afu->config.special_purpose_mem_size - 1; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(ocxl_afu_map_lpc_mem); > + > +struct resource *ocxl_afu_lpc_mem(struct ocxl_afu *afu) > +{ > + return &afu->lpc_res; > +} > +EXPORT_SYMBOL_GPL(ocxl_afu_lpc_mem); > + > +static void unmap_lpc_mem(struct ocxl_afu *afu) > +{ > + struct pci_dev *dev = to_pci_dev(afu->fn->dev.parent); > + > + if (afu->lpc_res.start || afu->special_purpose_res.start) { > + void *link = afu->fn->link; > + > + ocxl_link_lpc_release(link, dev); > + > + afu->lpc_res.start = 0; > + afu->lpc_res.end = 0; > + afu->special_purpose_res.start = 0; > + afu->special_purpose_res.end = 0; > + } > +} > + > static int configure_afu(struct ocxl_afu *afu, u8 afu_idx, struct pci_dev *dev) > { > int rc; > @@ -251,6 +300,7 @@ static int configure_afu(struct ocxl_afu *afu, u8 afu_idx, struct pci_dev *dev) > > static void deconfigure_afu(struct ocxl_afu *afu) > { > + unmap_lpc_mem(afu); > unmap_mmio_areas(afu); > reclaim_afu_pasid(afu); > reclaim_afu_actag(afu); > diff --git a/drivers/misc/ocxl/ocxl_internal.h b/drivers/misc/ocxl/ocxl_internal.h > index 20b417e00949..9f4b47900e62 100644 > --- a/drivers/misc/ocxl/ocxl_internal.h > +++ b/drivers/misc/ocxl/ocxl_internal.h > @@ -52,6 +52,9 @@ struct ocxl_afu { > void __iomem *global_mmio_ptr; > u64 pp_mmio_start; > void *private; > + u64 lpc_base_addr; /* Covers both LPC & special purpose memory */ > + struct resource lpc_res; > + struct resource special_purpose_res; > }; > > enum ocxl_context_status { > diff --git a/include/misc/ocxl.h b/include/misc/ocxl.h > index 06dd5839e438..6f7c02f0d5e3 100644 > --- a/include/misc/ocxl.h > +++ b/include/misc/ocxl.h > @@ -212,6 +212,24 @@ int ocxl_irq_set_handler(struct ocxl_context *ctx, int irq_id, > > // AFU Metadata > > +/** > + * Map the LPC system & special purpose memory for an AFU > + * > + * Do not call this during device discovery, as there may me multiple > + * devices on a link, and the memory is mapped for the whole link, not > + * just one device. It should only be called after all devices have > + * registered their memory on the link. > + * > + * afu: The AFU that has the LPC memory to map > + */ > +extern int ocxl_afu_map_lpc_mem(struct ocxl_afu *afu); > + > +/** > + * Get the physical address range of LPC memory for an AFU > + * afu: The AFU associated with the LPC memory > + */ > +extern struct resource *ocxl_afu_lpc_mem(struct ocxl_afu *afu); > + You can drop the useless 'extern'. Fred > /** > * Get a pointer to the config for an AFU > * >
On Tue, 3 Dec 2019 14:46:35 +1100 Alastair D'Silva <alastair@au1.ibm.com> wrote: > From: Alastair D'Silva <alastair@d-silva.org> > > Add functions to map/unmap LPC memory > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org> > --- > drivers/misc/ocxl/config.c | 4 +++ > drivers/misc/ocxl/core.c | 50 +++++++++++++++++++++++++++++++ > drivers/misc/ocxl/ocxl_internal.h | 3 ++ > include/misc/ocxl.h | 18 +++++++++++ > 4 files changed, 75 insertions(+) > > diff --git a/drivers/misc/ocxl/config.c b/drivers/misc/ocxl/config.c > index c8e19bfb5ef9..fb0c3b6f8312 100644 > --- a/drivers/misc/ocxl/config.c > +++ b/drivers/misc/ocxl/config.c > @@ -568,6 +568,10 @@ static int read_afu_lpc_memory_info(struct pci_dev *dev, > afu->special_purpose_mem_size = > total_mem_size - lpc_mem_size; > } > + > + dev_info(&dev->dev, "Probed LPC memory of %#llx bytes and special purpose memory of %#llx bytes\n", > + afu->lpc_mem_size, afu->special_purpose_mem_size); > + If we are being fussy, this block has nothing todo with the rest of the patch so we should be seeing it here. > return 0; > } > > diff --git a/drivers/misc/ocxl/core.c b/drivers/misc/ocxl/core.c > index 2531c6cf19a0..98611faea219 100644 > --- a/drivers/misc/ocxl/core.c > +++ b/drivers/misc/ocxl/core.c > @@ -210,6 +210,55 @@ static void unmap_mmio_areas(struct ocxl_afu *afu) > release_fn_bar(afu->fn, afu->config.global_mmio_bar); > } > > +int ocxl_afu_map_lpc_mem(struct ocxl_afu *afu) > +{ > + struct pci_dev *dev = to_pci_dev(afu->fn->dev.parent); > + > + if ((afu->config.lpc_mem_size + afu->config.special_purpose_mem_size) == 0) > + return 0; > + > + afu->lpc_base_addr = ocxl_link_lpc_map(afu->fn->link, dev); > + if (afu->lpc_base_addr == 0) > + return -EINVAL; > + > + if (afu->config.lpc_mem_size) { I was happy with the explicit check on 0 above, but we should be consistent. Either we make use of 0 == false, or we don't and explicitly check vs 0. Hence if (afu->config.pc_mem_size != 0) { here or if (!(afu->config.pc_mem_size + afu->config.special_purpose_mem_size)) return 0; above. > + afu->lpc_res.start = afu->lpc_base_addr + afu->config.lpc_mem_offset; > + afu->lpc_res.end = afu->lpc_res.start + afu->config.lpc_mem_size - 1; > + } > + > + if (afu->config.special_purpose_mem_size) { > + afu->special_purpose_res.start = afu->lpc_base_addr + > + afu->config.special_purpose_mem_offset; > + afu->special_purpose_res.end = afu->special_purpose_res.start + > + afu->config.special_purpose_mem_size - 1; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(ocxl_afu_map_lpc_mem); > + > +struct resource *ocxl_afu_lpc_mem(struct ocxl_afu *afu) > +{ > + return &afu->lpc_res; > +} > +EXPORT_SYMBOL_GPL(ocxl_afu_lpc_mem); > + > +static void unmap_lpc_mem(struct ocxl_afu *afu) > +{ > + struct pci_dev *dev = to_pci_dev(afu->fn->dev.parent); > + > + if (afu->lpc_res.start || afu->special_purpose_res.start) { > + void *link = afu->fn->link; > + > + ocxl_link_lpc_release(link, dev); > + > + afu->lpc_res.start = 0; > + afu->lpc_res.end = 0; > + afu->special_purpose_res.start = 0; > + afu->special_purpose_res.end = 0; > + } > +} > + > static int configure_afu(struct ocxl_afu *afu, u8 afu_idx, struct pci_dev *dev) > { > int rc; > @@ -251,6 +300,7 @@ static int configure_afu(struct ocxl_afu *afu, u8 afu_idx, struct pci_dev *dev) > > static void deconfigure_afu(struct ocxl_afu *afu) > { > + unmap_lpc_mem(afu); Hmm. This breaks the existing balance between configure_afu and deconfigure_afu. Given comments below on why we don't do map_lpc_mem in the afu bring up (as it's a shared operation) it seems to me that we should be doing this outside of the afu deconfigure. Perhaps ocxl_function_close is appropriate? I don't know this infrastructure well enough to be sure. If it does need to be here, then a comment to give more info on why would be great! > unmap_mmio_areas(afu); > reclaim_afu_pasid(afu); > reclaim_afu_actag(afu); > diff --git a/drivers/misc/ocxl/ocxl_internal.h b/drivers/misc/ocxl/ocxl_internal.h > index 20b417e00949..9f4b47900e62 100644 > --- a/drivers/misc/ocxl/ocxl_internal.h > +++ b/drivers/misc/ocxl/ocxl_internal.h > @@ -52,6 +52,9 @@ struct ocxl_afu { > void __iomem *global_mmio_ptr; > u64 pp_mmio_start; > void *private; > + u64 lpc_base_addr; /* Covers both LPC & special purpose memory */ > + struct resource lpc_res; > + struct resource special_purpose_res; > }; > > enum ocxl_context_status { > diff --git a/include/misc/ocxl.h b/include/misc/ocxl.h > index 06dd5839e438..6f7c02f0d5e3 100644 > --- a/include/misc/ocxl.h > +++ b/include/misc/ocxl.h > @@ -212,6 +212,24 @@ int ocxl_irq_set_handler(struct ocxl_context *ctx, int irq_id, > > // AFU Metadata > > +/** > + * Map the LPC system & special purpose memory for an AFU > + * > + * Do not call this during device discovery, as there may me multiple > + * devices on a link, and the memory is mapped for the whole link, not > + * just one device. It should only be called after all devices have > + * registered their memory on the link. > + * > + * afu: The AFU that has the LPC memory to map Run kernel-doc over these files and fix all the errors + warnings. @afu: .. and missing function name etc. > + */ > +extern int ocxl_afu_map_lpc_mem(struct ocxl_afu *afu); > + > +/** > + * Get the physical address range of LPC memory for an AFU > + * afu: The AFU associated with the LPC memory > + */ > +extern struct resource *ocxl_afu_lpc_mem(struct ocxl_afu *afu); > + > /** > * Get a pointer to the config for an AFU > *
On Mon, 2020-02-03 at 12:49 +0000, Jonathan Cameron wrote: > On Tue, 3 Dec 2019 14:46:35 +1100 > Alastair D'Silva <alastair@au1.ibm.com> wrote: > > > From: Alastair D'Silva <alastair@d-silva.org> > > > > Add functions to map/unmap LPC memory > > > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org> > > --- > > drivers/misc/ocxl/config.c | 4 +++ > > drivers/misc/ocxl/core.c | 50 > > +++++++++++++++++++++++++++++++ > > drivers/misc/ocxl/ocxl_internal.h | 3 ++ > > include/misc/ocxl.h | 18 +++++++++++ > > 4 files changed, 75 insertions(+) > > > > diff --git a/drivers/misc/ocxl/config.c > > b/drivers/misc/ocxl/config.c > > index c8e19bfb5ef9..fb0c3b6f8312 100644 > > --- a/drivers/misc/ocxl/config.c > > +++ b/drivers/misc/ocxl/config.c > > @@ -568,6 +568,10 @@ static int read_afu_lpc_memory_info(struct > > pci_dev *dev, > > afu->special_purpose_mem_size = > > total_mem_size - lpc_mem_size; > > } > > + > > + dev_info(&dev->dev, "Probed LPC memory of %#llx bytes and > > special purpose memory of %#llx bytes\n", > > + afu->lpc_mem_size, afu->special_purpose_mem_size); > > + > > If we are being fussy, this block has nothing todo with the rest of > the patch > so we should be seeing it here. Agreed > > > return 0; > > } > > > > diff --git a/drivers/misc/ocxl/core.c b/drivers/misc/ocxl/core.c > > index 2531c6cf19a0..98611faea219 100644 > > --- a/drivers/misc/ocxl/core.c > > +++ b/drivers/misc/ocxl/core.c > > @@ -210,6 +210,55 @@ static void unmap_mmio_areas(struct ocxl_afu > > *afu) > > release_fn_bar(afu->fn, afu->config.global_mmio_bar); > > } > > > > +int ocxl_afu_map_lpc_mem(struct ocxl_afu *afu) > > +{ > > + struct pci_dev *dev = to_pci_dev(afu->fn->dev.parent); > > + > > + if ((afu->config.lpc_mem_size + afu- > > >config.special_purpose_mem_size) == 0) > > + return 0; > > + > > + afu->lpc_base_addr = ocxl_link_lpc_map(afu->fn->link, dev); > > + if (afu->lpc_base_addr == 0) > > + return -EINVAL; > > + > > + if (afu->config.lpc_mem_size) { > > I was happy with the explicit check on 0 above, but we should be > consistent. Either > we make use of 0 == false, or we don't and explicitly check vs 0. > > Hence > > if (afu->config.pc_mem_size != 0) { > > here or > > if (!(afu->config.pc_mem_size + afu- > >config.special_purpose_mem_size)) > return 0; > > above. This feels a bit niggly, but sure, changed to a '> 0' check. > > > + afu->lpc_res.start = afu->lpc_base_addr + afu- > > >config.lpc_mem_offset; > > + afu->lpc_res.end = afu->lpc_res.start + afu- > > >config.lpc_mem_size - 1; > > + } > > + > > + if (afu->config.special_purpose_mem_size) { > > + afu->special_purpose_res.start = afu->lpc_base_addr + > > + afu- > > >config.special_purpose_mem_offset; > > + afu->special_purpose_res.end = afu- > > >special_purpose_res.start + > > + afu- > > >config.special_purpose_mem_size - 1; > > + } > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(ocxl_afu_map_lpc_mem); > > + > > +struct resource *ocxl_afu_lpc_mem(struct ocxl_afu *afu) > > +{ > > + return &afu->lpc_res; > > +} > > +EXPORT_SYMBOL_GPL(ocxl_afu_lpc_mem); > > + > > +static void unmap_lpc_mem(struct ocxl_afu *afu) > > +{ > > + struct pci_dev *dev = to_pci_dev(afu->fn->dev.parent); > > + > > + if (afu->lpc_res.start || afu->special_purpose_res.start) { > > + void *link = afu->fn->link; > > + > > + ocxl_link_lpc_release(link, dev); > > + > > + afu->lpc_res.start = 0; > > + afu->lpc_res.end = 0; > > + afu->special_purpose_res.start = 0; > > + afu->special_purpose_res.end = 0; > > + } > > +} > > + > > static int configure_afu(struct ocxl_afu *afu, u8 afu_idx, struct > > pci_dev *dev) > > { > > int rc; > > @@ -251,6 +300,7 @@ static int configure_afu(struct ocxl_afu *afu, > > u8 afu_idx, struct pci_dev *dev) > > > > static void deconfigure_afu(struct ocxl_afu *afu) > > { > > + unmap_lpc_mem(afu); > > Hmm. This breaks the existing balance between configure_afu and > deconfigure_afu. > > Given comments below on why we don't do map_lpc_mem in the afu bring > up > (as it's a shared operation) it seems to me that we should be doing > this > outside of the afu deconfigure. Perhaps ocxl_function_close is > appropriate? > I don't know this infrastructure well enough to be sure. > > If it does need to be here, then a comment to give more info on > why would be great! > Sure, I've added a comment in unmap_lpc_mem explaining that lpc_release only releases the memory on the link when the last consumer calls release. It's in deconfigure_afu as the LPC memory is registered and reported per-AFU (even though it has to be allocated all at once across the link). > > unmap_mmio_areas(afu); > > reclaim_afu_pasid(afu); > > reclaim_afu_actag(afu); > > diff --git a/drivers/misc/ocxl/ocxl_internal.h > > b/drivers/misc/ocxl/ocxl_internal.h > > index 20b417e00949..9f4b47900e62 100644 > > --- a/drivers/misc/ocxl/ocxl_internal.h > > +++ b/drivers/misc/ocxl/ocxl_internal.h > > @@ -52,6 +52,9 @@ struct ocxl_afu { > > void __iomem *global_mmio_ptr; > > u64 pp_mmio_start; > > void *private; > > + u64 lpc_base_addr; /* Covers both LPC & special purpose memory > > */ > > + struct resource lpc_res; > > + struct resource special_purpose_res; > > }; > > > > enum ocxl_context_status { > > diff --git a/include/misc/ocxl.h b/include/misc/ocxl.h > > index 06dd5839e438..6f7c02f0d5e3 100644 > > --- a/include/misc/ocxl.h > > +++ b/include/misc/ocxl.h > > @@ -212,6 +212,24 @@ int ocxl_irq_set_handler(struct ocxl_context > > *ctx, int irq_id, > > > > // AFU Metadata > > > > +/** > > + * Map the LPC system & special purpose memory for an AFU > > + * > > + * Do not call this during device discovery, as there may me > > multiple > > + * devices on a link, and the memory is mapped for the whole link, > > not > > + * just one device. It should only be called after all devices > > have > > + * registered their memory on the link. > > + * > > + * afu: The AFU that has the LPC memory to map > Run kernel-doc over these files and fix all the errors + warnings. > Ok. > @afu: .. > > and missing function name etc. > > > > + */ > > +extern int ocxl_afu_map_lpc_mem(struct ocxl_afu *afu); > > + > > +/** > > + * Get the physical address range of LPC memory for an AFU > > + * afu: The AFU associated with the LPC memory > > + */ > > +extern struct resource *ocxl_afu_lpc_mem(struct ocxl_afu *afu); > > + > > /** > > * Get a pointer to the config for an AFU > > *
diff --git a/drivers/misc/ocxl/config.c b/drivers/misc/ocxl/config.c index c8e19bfb5ef9..fb0c3b6f8312 100644 --- a/drivers/misc/ocxl/config.c +++ b/drivers/misc/ocxl/config.c @@ -568,6 +568,10 @@ static int read_afu_lpc_memory_info(struct pci_dev *dev, afu->special_purpose_mem_size = total_mem_size - lpc_mem_size; } + + dev_info(&dev->dev, "Probed LPC memory of %#llx bytes and special purpose memory of %#llx bytes\n", + afu->lpc_mem_size, afu->special_purpose_mem_size); + return 0; } diff --git a/drivers/misc/ocxl/core.c b/drivers/misc/ocxl/core.c index 2531c6cf19a0..98611faea219 100644 --- a/drivers/misc/ocxl/core.c +++ b/drivers/misc/ocxl/core.c @@ -210,6 +210,55 @@ static void unmap_mmio_areas(struct ocxl_afu *afu) release_fn_bar(afu->fn, afu->config.global_mmio_bar); } +int ocxl_afu_map_lpc_mem(struct ocxl_afu *afu) +{ + struct pci_dev *dev = to_pci_dev(afu->fn->dev.parent); + + if ((afu->config.lpc_mem_size + afu->config.special_purpose_mem_size) == 0) + return 0; + + afu->lpc_base_addr = ocxl_link_lpc_map(afu->fn->link, dev); + if (afu->lpc_base_addr == 0) + return -EINVAL; + + if (afu->config.lpc_mem_size) { + afu->lpc_res.start = afu->lpc_base_addr + afu->config.lpc_mem_offset; + afu->lpc_res.end = afu->lpc_res.start + afu->config.lpc_mem_size - 1; + } + + if (afu->config.special_purpose_mem_size) { + afu->special_purpose_res.start = afu->lpc_base_addr + + afu->config.special_purpose_mem_offset; + afu->special_purpose_res.end = afu->special_purpose_res.start + + afu->config.special_purpose_mem_size - 1; + } + + return 0; +} +EXPORT_SYMBOL_GPL(ocxl_afu_map_lpc_mem); + +struct resource *ocxl_afu_lpc_mem(struct ocxl_afu *afu) +{ + return &afu->lpc_res; +} +EXPORT_SYMBOL_GPL(ocxl_afu_lpc_mem); + +static void unmap_lpc_mem(struct ocxl_afu *afu) +{ + struct pci_dev *dev = to_pci_dev(afu->fn->dev.parent); + + if (afu->lpc_res.start || afu->special_purpose_res.start) { + void *link = afu->fn->link; + + ocxl_link_lpc_release(link, dev); + + afu->lpc_res.start = 0; + afu->lpc_res.end = 0; + afu->special_purpose_res.start = 0; + afu->special_purpose_res.end = 0; + } +} + static int configure_afu(struct ocxl_afu *afu, u8 afu_idx, struct pci_dev *dev) { int rc; @@ -251,6 +300,7 @@ static int configure_afu(struct ocxl_afu *afu, u8 afu_idx, struct pci_dev *dev) static void deconfigure_afu(struct ocxl_afu *afu) { + unmap_lpc_mem(afu); unmap_mmio_areas(afu); reclaim_afu_pasid(afu); reclaim_afu_actag(afu); diff --git a/drivers/misc/ocxl/ocxl_internal.h b/drivers/misc/ocxl/ocxl_internal.h index 20b417e00949..9f4b47900e62 100644 --- a/drivers/misc/ocxl/ocxl_internal.h +++ b/drivers/misc/ocxl/ocxl_internal.h @@ -52,6 +52,9 @@ struct ocxl_afu { void __iomem *global_mmio_ptr; u64 pp_mmio_start; void *private; + u64 lpc_base_addr; /* Covers both LPC & special purpose memory */ + struct resource lpc_res; + struct resource special_purpose_res; }; enum ocxl_context_status { diff --git a/include/misc/ocxl.h b/include/misc/ocxl.h index 06dd5839e438..6f7c02f0d5e3 100644 --- a/include/misc/ocxl.h +++ b/include/misc/ocxl.h @@ -212,6 +212,24 @@ int ocxl_irq_set_handler(struct ocxl_context *ctx, int irq_id, // AFU Metadata +/** + * Map the LPC system & special purpose memory for an AFU + * + * Do not call this during device discovery, as there may me multiple + * devices on a link, and the memory is mapped for the whole link, not + * just one device. It should only be called after all devices have + * registered their memory on the link. + * + * afu: The AFU that has the LPC memory to map + */ +extern int ocxl_afu_map_lpc_mem(struct ocxl_afu *afu); + +/** + * Get the physical address range of LPC memory for an AFU + * afu: The AFU associated with the LPC memory + */ +extern struct resource *ocxl_afu_lpc_mem(struct ocxl_afu *afu); + /** * Get a pointer to the config for an AFU *