| Message ID | 1503556688-15412-5-git-send-email-sukadev@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Suka, Comments inline. Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> writes: > diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c > index 6156fbe..a3a705a 100644 > --- a/arch/powerpc/platforms/powernv/vas-window.c > +++ b/arch/powerpc/platforms/powernv/vas-window.c > @@ -9,9 +9,182 @@ > > #include <linux/types.h> > #include <linux/mutex.h> > +#include <linux/slab.h> > +#include <linux/io.h> > > #include "vas.h" > > +/* > + * Compute the paste address region for the window @window using the > + * ->paste_base_addr and ->paste_win_id_shift we got from device tree. > + */ > +void compute_paste_address(struct vas_window *window, uint64_t *addr, int *len) > +{ > + uint64_t base, shift; Please use the kernel types, so u64 here. > + int winid; > + > + base = window->vinst->paste_base_addr; > + shift = window->vinst->paste_win_id_shift; > + winid = window->winid; > + > + *addr = base + (winid << shift); > + if (len) > + *len = PAGE_SIZE; Having multiple output parameters makes for a pretty awkward API. Is it really necesssary given len is a constant PAGE_SIZE anyway. If you didn't return len, then you could just make the function return the addr, and you wouldn't need any output parameters. One of the callers that passes len is unmap_paste_region(), but that is a bit odd. It would be more natural I think if once a window is mapped it knows its size. Or if the mapping will always just be one page then we can just know that. > + > + pr_debug("Txwin #%d: Paste addr 0x%llx\n", winid, *addr); > +} > + > +static inline void get_hvwc_mmio_bar(struct vas_window *window, > + uint64_t *start, int *len) > +{ > + uint64_t pbaddr; > + > + pbaddr = window->vinst->hvwc_bar_start; > + *start = pbaddr + window->winid * VAS_HVWC_SIZE; > + *len = VAS_HVWC_SIZE; This is: #define VAS_HVWC_SIZE 512 But then we map it, which will round up to a page anyway. So again I don't see the point of having the len returned form this helper. > +} > + > +static inline void get_uwc_mmio_bar(struct vas_window *window, > + uint64_t *start, int *len) > +{ > + uint64_t pbaddr; > + > + pbaddr = window->vinst->uwc_bar_start; > + *start = pbaddr + window->winid * VAS_UWC_SIZE; > + *len = VAS_UWC_SIZE; > +} > + > +/* > + * Map the paste bus address of the given send window into kernel address > + * space. Unlike MMIO regions (map_mmio_region() below), paste region must > + * be mapped cache-able and is only applicable to send windows. > + */ > +void *map_paste_region(struct vas_window *txwin) > +{ > + int rc, len; > + void *map; > + char *name; > + uint64_t start; > + > + rc = -ENOMEM; You don't need that. > + name = kasprintf(GFP_KERNEL, "window-v%d-w%d", txwin->vinst->vas_id, > + txwin->winid); > + if (!name) > + return ERR_PTR(rc); That can goto free_name; > + > + txwin->paste_addr_name = name; > + compute_paste_address(txwin, &start, &len); > + > + if (!request_mem_region(start, len, name)) { > + pr_devel("%s(): request_mem_region(0x%llx, %d) failed\n", > + __func__, start, len); > + goto free_name; > + } > + > + map = ioremap_cache(start, len); > + if (!map) { > + pr_devel("%s(): ioremap_cache(0x%llx, %d) failed\n", __func__, > + start, len); > + goto free_name; > + } > + > + pr_devel("VAS: mapped paste addr 0x%llx to kaddr 0x%p\n", start, map); > + return map; > + > +free_name: > + kfree(name); Because kfree(NULL) is fine. > + return ERR_PTR(rc); And that can just return ERR_PTR(-ENOMEM); > +} cheers
Michael Ellerman [mpe@ellerman.id.au] wrote: > Hi Suka, > > Comments inline. > > Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> writes: > > diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c > > index 6156fbe..a3a705a 100644 > > --- a/arch/powerpc/platforms/powernv/vas-window.c > > +++ b/arch/powerpc/platforms/powernv/vas-window.c > > @@ -9,9 +9,182 @@ > > > > #include <linux/types.h> > > #include <linux/mutex.h> > > +#include <linux/slab.h> > > +#include <linux/io.h> > > > > #include "vas.h" > > > > +/* > > + * Compute the paste address region for the window @window using the > > + * ->paste_base_addr and ->paste_win_id_shift we got from device tree. > > + */ > > +void compute_paste_address(struct vas_window *window, uint64_t *addr, int *len) > > +{ > > + uint64_t base, shift; > > Please use the kernel types, so u64 here. Ok. > > > + int winid; > > + > > + base = window->vinst->paste_base_addr; > > + shift = window->vinst->paste_win_id_shift; > > + winid = window->winid; > > + > > + *addr = base + (winid << shift); > > + if (len) > > + *len = PAGE_SIZE; > > Having multiple output parameters makes for a pretty awkward API. Is it > really necesssary given len is a constant PAGE_SIZE anyway. > > If you didn't return len, then you could just make the function return > the addr, and you wouldn't need any output parameters. I agree, I went back and forth on it. I was trying to avoid callers making assumptions on the size. But since there are just a couple of places, I guess we could have them assume PAGE_SIZE. > > One of the callers that passes len is unmap_paste_region(), but that > is a bit odd. It would be more natural I think if once a window is > mapped it knows its size. Or if the mapping will always just be one page > then we can just know that. Agree, since the len values are constant I was trying to avoid saving them in each of the 64K windows - so the compute during unmap. Will change to assume PAGE_SIZE. Also agree with other comments here.
diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c index 6156fbe..a3a705a 100644 --- a/arch/powerpc/platforms/powernv/vas-window.c +++ b/arch/powerpc/platforms/powernv/vas-window.c @@ -9,9 +9,182 @@ #include <linux/types.h> #include <linux/mutex.h> +#include <linux/slab.h> +#include <linux/io.h> #include "vas.h" +/* + * Compute the paste address region for the window @window using the + * ->paste_base_addr and ->paste_win_id_shift we got from device tree. + */ +void compute_paste_address(struct vas_window *window, uint64_t *addr, int *len) +{ + uint64_t base, shift; + int winid; + + base = window->vinst->paste_base_addr; + shift = window->vinst->paste_win_id_shift; + winid = window->winid; + + *addr = base + (winid << shift); + if (len) + *len = PAGE_SIZE; + + pr_debug("Txwin #%d: Paste addr 0x%llx\n", winid, *addr); +} + +static inline void get_hvwc_mmio_bar(struct vas_window *window, + uint64_t *start, int *len) +{ + uint64_t pbaddr; + + pbaddr = window->vinst->hvwc_bar_start; + *start = pbaddr + window->winid * VAS_HVWC_SIZE; + *len = VAS_HVWC_SIZE; +} + +static inline void get_uwc_mmio_bar(struct vas_window *window, + uint64_t *start, int *len) +{ + uint64_t pbaddr; + + pbaddr = window->vinst->uwc_bar_start; + *start = pbaddr + window->winid * VAS_UWC_SIZE; + *len = VAS_UWC_SIZE; +} + +/* + * Map the paste bus address of the given send window into kernel address + * space. Unlike MMIO regions (map_mmio_region() below), paste region must + * be mapped cache-able and is only applicable to send windows. + */ +void *map_paste_region(struct vas_window *txwin) +{ + int rc, len; + void *map; + char *name; + uint64_t start; + + rc = -ENOMEM; + name = kasprintf(GFP_KERNEL, "window-v%d-w%d", txwin->vinst->vas_id, + txwin->winid); + if (!name) + return ERR_PTR(rc); + + txwin->paste_addr_name = name; + compute_paste_address(txwin, &start, &len); + + if (!request_mem_region(start, len, name)) { + pr_devel("%s(): request_mem_region(0x%llx, %d) failed\n", + __func__, start, len); + goto free_name; + } + + map = ioremap_cache(start, len); + if (!map) { + pr_devel("%s(): ioremap_cache(0x%llx, %d) failed\n", __func__, + start, len); + goto free_name; + } + + pr_devel("VAS: mapped paste addr 0x%llx to kaddr 0x%p\n", start, map); + return map; + +free_name: + kfree(name); + return ERR_PTR(rc); +} + + +static void *map_mmio_region(char *name, uint64_t start, int len) +{ + void *map; + + if (!request_mem_region(start, len, name)) { + pr_devel("%s(): request_mem_region(0x%llx, %d) failed\n", + __func__, start, len); + return NULL; + } + + map = ioremap(start, len); + if (!map) { + pr_devel("%s(): ioremap(0x%llx, %d) failed\n", __func__, start, + len); + return NULL; + } + + return map; +} + +static void unmap_region(void *addr, uint64_t start, int len) +{ + iounmap(addr); + release_mem_region((phys_addr_t)start, len); +} + +/* + * Unmap the paste address region for a window. + */ +void unmap_paste_region(struct vas_window *window) +{ + int len; + uint64_t busaddr_start; + + if (window->paste_kaddr) { + compute_paste_address(window, &busaddr_start, &len); + unmap_region(window->paste_kaddr, busaddr_start, len); + window->paste_kaddr = NULL; + kfree(window->paste_addr_name); + window->paste_addr_name = NULL; + } +} + +/* + * Unmap the MMIO regions for a window. + */ +static void unmap_winctx_mmio_bars(struct vas_window *window) +{ + int len; + uint64_t busaddr_start; + + if (window->hvwc_map) { + get_hvwc_mmio_bar(window, &busaddr_start, &len); + unmap_region(window->hvwc_map, busaddr_start, len); + window->hvwc_map = NULL; + } + + if (window->uwc_map) { + get_uwc_mmio_bar(window, &busaddr_start, &len); + unmap_region(window->uwc_map, busaddr_start, len); + window->uwc_map = NULL; + } +} + +/* + * Find the Hypervisor Window Context (HVWC) MMIO Base Address Region and the + * OS/User Window Context (UWC) MMIO Base Address Region for the given window. + * Map these bus addresses and save the mapped kernel addresses in @window. + */ +int map_winctx_mmio_bars(struct vas_window *window) +{ + int len; + uint64_t start; + + get_hvwc_mmio_bar(window, &start, &len); + window->hvwc_map = map_mmio_region("HVWCM_Window", start, len); + + get_uwc_mmio_bar(window, &start, &len); + window->uwc_map = map_mmio_region("UWCM_Window", start, len); + + if (!window->hvwc_map || !window->uwc_map) { + unmap_winctx_mmio_bars(window); + return -1; + } + + return 0; +} + /* stub for now */ int vas_win_close(struct vas_window *window) {
Define some helper functions to access the MMIO regions. We use these in follow-on patches to read/write VAS hardware registers. They are also used to later issue 'paste' instructions to submit requests to the NX hardware engines. Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> --- Changelog [v6]: - Minor reorg to make setup/cleanup functions more symmetric Changelog [v5]: - [Ben Herrenschmidt]: Need cachable mapping for paste regions and non-cachable mapping for the MMIO regions. So, just use ioremap() for mapping the MMIO regions; use "winctx" instead of "wc" to avoid collision with "write combine". Changelog [v3]: - Minor reorg/cleanup of map/unmap functions Changelog [v2]: - Get HVWC, UWC and paste addresses from window->vinst (i.e DT) rather than kernel macros. --- arch/powerpc/platforms/powernv/vas-window.c | 173 ++++++++++++++++++++++++++++ 1 file changed, 173 insertions(+)