| Message ID | 20241025112701.303035-2-niklas.neronin@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: xhci: improve HCD page size & IMODI | expand |
On 25.10.2024 14.27, Niklas Neronin wrote: > xHC supports a page size of (2^(n+12)), where 'n' is the Page Size Bit. > The page size of 4096 bytes is common and always supported. Consequently, > the xHCI driver always sets the 'page_size' to 4096 (i.e., (1 << 12)). > > At present, the xHCI driver reads the Page Size register but does not use > the value, except for printing a two useless debug traces. This introduces > unnecessary code into xhci_mem_init(), which is already quite large. > Although the page size is not currently modified, it may be in the future. > > To balance both current and future needs, the page size setting code is > moved to a separate function. This rework ensures that the Page Size > register is not read for the minimum value (4096). However, if a different > value is provided, it will not be ignored, rather clamped between the > valid min and max page size. > > Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com> > --- > drivers/usb/host/xhci-mem.c | 40 ++++++++++++++++++++++--------------- > drivers/usb/host/xhci.h | 9 ++++++--- > 2 files changed, 30 insertions(+), 19 deletions(-) > > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c > index d2900197a49e..8a6b91862cae 100644 > --- a/drivers/usb/host/xhci-mem.c > +++ b/drivers/usb/host/xhci-mem.c > @@ -1959,7 +1959,6 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci) > xhci->interrupters = NULL; > > xhci->page_size = 0; > - xhci->page_shift = 0; > xhci->usb2_rhub.bus_state.bus_suspended = 0; > xhci->usb3_rhub.bus_state.bus_suspended = 0; > } > @@ -2378,6 +2377,27 @@ xhci_create_secondary_interrupter(struct usb_hcd *hcd, unsigned int segs, > } > EXPORT_SYMBOL_GPL(xhci_create_secondary_interrupter); > > +static void xhci_hcd_page_size(struct xhci_hcd *xhci) > +{ > + u32 page_size; > + > + if (xhci->page_size <= HCD_PAGE_MIN) { > + xhci->page_size = HCD_PAGE_MIN; > + } else { > + /* Max page size is 2^(n+12), where 'n' is the first 15:0 bit set */ > + page_size = readl(&xhci->op_regs->page_size) & HCD_PAGE_SIZE_MASK; > + page_size = 1 << (ffs(page_size) + 12); > + > + if (page_size < xhci->page_size) > + xhci->page_size = page_size; > + else > + xhci->page_size = (1 << ffs(xhci->page_size)); > + } probably fls() instead of ffs() The xhci specification is a bit unclear about pagesize register (xhci 5.4.7) I assume it shows currently used pagesize, and only has one bit set. But I guess it's possible that it could list all supported page sizes, or maybe just maximum page size? > + > + xhci_dbg_trace(xhci, trace_xhci_dbg_init, "HCD page size set to %iK", > + xhci->page_size / 1024); > +} > + > int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) > { > struct xhci_interrupter *ir; > @@ -2385,7 +2405,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) > dma_addr_t dma; > unsigned int val, val2; > u64 val_64; > - u32 page_size, temp; > + u32 temp; > int i; > > INIT_LIST_HEAD(&xhci->cmd_list); > @@ -2394,20 +2414,8 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) > INIT_DELAYED_WORK(&xhci->cmd_timer, xhci_handle_command_timeout); > init_completion(&xhci->cmd_ring_stop_completion); > > - page_size = readl(&xhci->op_regs->page_size); > - xhci_dbg_trace(xhci, trace_xhci_dbg_init, > - "Supported page size register = 0x%x", page_size); > - i = ffs(page_size); > - if (i < 16) > - xhci_dbg_trace(xhci, trace_xhci_dbg_init, > - "Supported page size of %iK", (1 << (i+12)) / 1024); > - else > - xhci_warn(xhci, "WARN: no supported page size\n"); > - /* Use 4K pages, since that's common and the minimum the HC supports */ > - xhci->page_shift = 12; > - xhci->page_size = 1 << xhci->page_shift; > - xhci_dbg_trace(xhci, trace_xhci_dbg_init, > - "HCD page size set to %iK", xhci->page_size / 1024); > + /* If 'page_size' is not set, use 4K pages, since that's common and always supported */ > + xhci_hcd_page_size(xhci); > > /* > * Program the Number of Device Slots Enabled field in the CONFIG > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h > index f0fb696d5619..f998df70f80f 100644 > --- a/drivers/usb/host/xhci.h > +++ b/drivers/usb/host/xhci.h > @@ -211,6 +211,11 @@ struct xhci_op_regs { > #define CONFIG_CIE (1 << 9) > /* bits 10:31 - reserved and should be preserved */ > > +/* bits 15:0 - HCD page shift bit */ > +#define HCD_PAGE_SIZE_MASK 0xffff > +/* HCD page size 4KB up to 128MB, Rev 1.2 Section 5.4.3. */ > +#define HCD_PAGE_MIN (1 << 12) > + Probably better to use XHCI_ prefix instead of HCD_ as these are xhci specific. We don't want to confuse these with usb core hcd defines (include/linux/usb/hcd.h) Thanks Mathias
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index d2900197a49e..8a6b91862cae 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -1959,7 +1959,6 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci) xhci->interrupters = NULL; xhci->page_size = 0; - xhci->page_shift = 0; xhci->usb2_rhub.bus_state.bus_suspended = 0; xhci->usb3_rhub.bus_state.bus_suspended = 0; } @@ -2378,6 +2377,27 @@ xhci_create_secondary_interrupter(struct usb_hcd *hcd, unsigned int segs, } EXPORT_SYMBOL_GPL(xhci_create_secondary_interrupter); +static void xhci_hcd_page_size(struct xhci_hcd *xhci) +{ + u32 page_size; + + if (xhci->page_size <= HCD_PAGE_MIN) { + xhci->page_size = HCD_PAGE_MIN; + } else { + /* Max page size is 2^(n+12), where 'n' is the first 15:0 bit set */ + page_size = readl(&xhci->op_regs->page_size) & HCD_PAGE_SIZE_MASK; + page_size = 1 << (ffs(page_size) + 12); + + if (page_size < xhci->page_size) + xhci->page_size = page_size; + else + xhci->page_size = (1 << ffs(xhci->page_size)); + } + + xhci_dbg_trace(xhci, trace_xhci_dbg_init, "HCD page size set to %iK", + xhci->page_size / 1024); +} + int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) { struct xhci_interrupter *ir; @@ -2385,7 +2405,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) dma_addr_t dma; unsigned int val, val2; u64 val_64; - u32 page_size, temp; + u32 temp; int i; INIT_LIST_HEAD(&xhci->cmd_list); @@ -2394,20 +2414,8 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags) INIT_DELAYED_WORK(&xhci->cmd_timer, xhci_handle_command_timeout); init_completion(&xhci->cmd_ring_stop_completion); - page_size = readl(&xhci->op_regs->page_size); - xhci_dbg_trace(xhci, trace_xhci_dbg_init, - "Supported page size register = 0x%x", page_size); - i = ffs(page_size); - if (i < 16) - xhci_dbg_trace(xhci, trace_xhci_dbg_init, - "Supported page size of %iK", (1 << (i+12)) / 1024); - else - xhci_warn(xhci, "WARN: no supported page size\n"); - /* Use 4K pages, since that's common and the minimum the HC supports */ - xhci->page_shift = 12; - xhci->page_size = 1 << xhci->page_shift; - xhci_dbg_trace(xhci, trace_xhci_dbg_init, - "HCD page size set to %iK", xhci->page_size / 1024); + /* If 'page_size' is not set, use 4K pages, since that's common and always supported */ + xhci_hcd_page_size(xhci); /* * Program the Number of Device Slots Enabled field in the CONFIG diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index f0fb696d5619..f998df70f80f 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -211,6 +211,11 @@ struct xhci_op_regs { #define CONFIG_CIE (1 << 9) /* bits 10:31 - reserved and should be preserved */ +/* bits 15:0 - HCD page shift bit */ +#define HCD_PAGE_SIZE_MASK 0xffff +/* HCD page size 4KB up to 128MB, Rev 1.2 Section 5.4.3. */ +#define HCD_PAGE_MIN (1 << 12) + /** * struct xhci_intr_reg - Interrupt Register Set * @irq_pending: IMAN - Interrupt Management Register. Used to enable @@ -1502,10 +1507,8 @@ struct xhci_hcd { u16 max_interrupters; /* imod_interval in ns (I * 250ns) */ u32 imod_interval; - /* 4KB min, 128MB max */ + /* Always set to HCD_PAGE_MIN (4KB) */ int page_size; - /* Valid values are 12 to 20, inclusive */ - int page_shift; /* MSI-X/MSI vectors */ int nvecs; /* optional clocks */
xHC supports a page size of (2^(n+12)), where 'n' is the Page Size Bit. The page size of 4096 bytes is common and always supported. Consequently, the xHCI driver always sets the 'page_size' to 4096 (i.e., (1 << 12)). At present, the xHCI driver reads the Page Size register but does not use the value, except for printing a two useless debug traces. This introduces unnecessary code into xhci_mem_init(), which is already quite large. Although the page size is not currently modified, it may be in the future. To balance both current and future needs, the page size setting code is moved to a separate function. This rework ensures that the Page Size register is not read for the minimum value (4096). However, if a different value is provided, it will not be ignored, rather clamped between the valid min and max page size. Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com> --- drivers/usb/host/xhci-mem.c | 40 ++++++++++++++++++++++--------------- drivers/usb/host/xhci.h | 9 ++++++--- 2 files changed, 30 insertions(+), 19 deletions(-)