Hi John,
Looks good, aside from a couple small things. See my inline comments.
Your patch included all spaces instead of tabs; maybe your mail client
mangled it? I've had trouble with Outlook 2007 mail servers converting
all tabs to spaces. That's why I use my linux.intel.com address to send
and receive patches.
You should probably run scripts/checkpatch.pl over this patch too.
You might want to elaborate on what a scatchpad buffer array is in the
commit message for those people that don't have the xHCI spec. It's
basically just an array of 64-bit pointers to chunks of memory that the
xHCI hardware can use "as a scratchpad", correct?
On Mon, Jul 13, 2009 at 02:10:10PM -0700, John Youn wrote:
> Allocation and initialization of scratchpad buffer array.
> XHCI 4.20.
>
> Signed-off-by: John Youn <johnyoun@xxxxxxxxxxxx>
> ---
>
> drivers/usb/host/xhci-mem.c | 93 +++++++++++++++++++++++++++++++++++++++++++
> drivers/usb/host/xhci.h | 11 +++++
> 2 files changed, 104 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index d90baaa..88d5794 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -544,6 +544,93 @@ void xhci_endpoint_zero(struct xhci_hcd *xhci,
> */
> }
>
> +
> +/* Set up the scratchpad buffer array and scratchpad buffers, if needed. */
> +static int scratchpad_alloc(struct xhci_hcd *xhci, gfp_t flags)
> +{
> + int i;
> + struct device *dev = xhci_to_hcd(xhci)->self.controller;
> + int num_sp = HCS_MAX_SCRATCHPAD(xhci->hcs_params2);
> +
> + xhci_dbg(xhci, "Allocating %d scratchpad buffers\n", num_sp);
> +
> + if (!num_sp)
> + return 0;
> +
> + xhci->scratchpad = kzalloc(sizeof(*xhci->scratchpad), flags);
> + if (!xhci->scratchpad)
> + goto fail_sp;
> +
> + xhci->scratchpad->sp_array =
> + pci_alloc_consistent(to_pci_dev(dev),
> + num_sp * sizeof(u64),
> + &xhci->scratchpad->sp_dma);
> + if (!xhci->scratchpad->sp_array)
> + goto fail_sp2;
> +
> + xhci->scratchpad->sp_buffers = kzalloc(sizeof(void *) * num_sp, flags);
> + if (!xhci->scratchpad->sp_buffers)
> + goto fail_sp3;
> +
> + xhci->dcbaa->dev_context_ptrs[0] = xhci->scratchpad->sp_dma;
> + for (i = 0; i < num_sp; i++) {
> + dma_addr_t dma;
> + void *buf = pci_alloc_consistent(to_pci_dev(dev),
> + PAGE_SIZE, &dma);
PAGE_SIZE should be xhci->page_size instead. We want the internal xHCI
page size to always be 4K, but the driver needs to run on systems that
have a bigger page size. (Yes, xHCI implementations can have a bigger
page size, but the driver doesn't support a page size that's bigger than
4KB right now.)
> + if (!buf)
> + goto fail_sp4;
> +
> + xhci->scratchpad->sp_array[i] = dma;
> + xhci->scratchpad->sp_buffers[i] = buf;
> + }
> +
> + return 0;
> +
> + fail_sp4:
> + for (i = i - 1; i >= 0; i --) {
checkpatch error:
ERROR: space prohibited before that '--' (ctx:WxB)
#62: FILE: drivers/usb/host/xhci-mem.c:590:
+ for (i = i - 1; i >= 0; i --) {
^
> + pci_free_consistent(to_pci_dev(dev), PAGE_SIZE,
> + xhci->scratchpad->sp_buffers[i],
> + xhci->scratchpad->sp_array[i]);
Use xhci->page_size.
> + }
> + kfree(xhci->scratchpad->sp_buffers);
> +
> + fail_sp3:
> + pci_free_consistent(to_pci_dev(dev), num_sp * sizeof(u64),
> + xhci->scratchpad->sp_array,
> + xhci->scratchpad->sp_dma);
> +
> + fail_sp2:
> + kfree(xhci->scratchpad);
> + xhci->scratchpad = NULL;
> +
> + fail_sp:
> + return -1;
You should return -ENOMEM here instead of -1. It's not a bug, it's just
common practice.
> +}
> +
> +static void scratchpad_free(struct xhci_hcd *xhci)
> +{
> + int num_sp;
> + int i;
> + struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
> +
> + if (!xhci->scratchpad)
> + return;
> +
> + num_sp = HCS_MAX_SCRATCHPAD(xhci->hcs_params2);
> +
> + for (i = 0; i < num_sp; i++) {
> + pci_free_consistent(pdev, PAGE_SIZE,
Use xhci->page_size.
> + xhci->scratchpad->sp_buffers[i],
> + xhci->scratchpad->sp_array[i]);
So if the xHCI hardware has barfed into the DMA pointers in the
scratchpad context array, you're just going to use those addresses to
free memory? You might want to keep an array of buffer DMA pointers in
struct xhci_scratchpad. It's not pretty, but I'm not sure how much I
trust the hardware yet. ;)
> + }
> + kfree(xhci->scratchpad->sp_buffers);
> + pci_free_consistent(pdev, num_sp * sizeof(u64),
> + xhci->scratchpad->sp_array,
> + xhci->scratchpad->sp_dma);
> + kfree(xhci->scratchpad);
> + xhci->scratchpad = NULL;
> +}
> +
> void xhci_mem_cleanup(struct xhci_hcd *xhci)
> {
> struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller);
> @@ -592,6 +679,7 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>
> xhci->page_size = 0;
> xhci->page_shift = 0;
> + scratchpad_free(xhci);
> }
>
> int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
> @@ -754,7 +842,12 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
> for (i = 0; i < MAX_HC_SLOTS; ++i)
> xhci->devs[i] = 0;
>
> + if (scratchpad_alloc(xhci, flags))
> + goto fail;
> +
> +
> return 0;
> +
> fail:
> xhci_warn(xhci, "Couldn't initialize memory\n");
> xhci_mem_cleanup(xhci);
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 341c907..c37b31a 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -89,6 +89,7 @@ struct xhci_cap_regs {
> #define HCS_ERST_MAX(p) (((p) >> 4) & 0xf)
> /* bit 26 Scratchpad restore - for save/restore HW state - not used yet */
> /* bits 27:31 number of Scratchpad buffers SW must allocate for the HW */
> +#define HCS_MAX_SCRATCHPAD(p) (((p) >> 27) & 0x1f)
>
> /* HCSPARAMS3 - hcs_params3 - bitmasks */
> /* bits 0:7, Max U1 to U0 latency for the roothub ports */
> @@ -968,6 +969,12 @@ struct xhci_erst {
> unsigned int erst_size;
> };
>
> +struct xhci_scratchpad {
> + u64 *sp_array;
> + dma_addr_t sp_dma;
> + void **sp_buffers;
> +};
> +
> /*
> * Each segment table entry is 4*32bits long. 1K seems like an ok size:
> * (1K bytes * 8bytes/bit) / (4*32 bits) = 64 segment entries in the table,
> @@ -1022,6 +1029,9 @@ struct xhci_hcd {
> struct xhci_ring *cmd_ring;
> struct xhci_ring *event_ring;
> struct xhci_erst erst;
> + /* Scratchpad */
> + struct xhci_scratchpad *scratchpad;
> +
> /* slot enabling and address device helpers */
> struct completion addr_dev;
> int slot_id;
> @@ -1032,6 +1042,7 @@ struct xhci_hcd {
> struct dma_pool *device_pool;
> struct dma_pool *segment_pool;
>
> +
Whitespace fixes should go in separate patches.
> #ifdef CONFIG_USB_XHCI_HCD_DEBUGGING
> /* Poll the rings - for debugging */
> struct timer_list event_ring_timer;
> --
> 1.6.2.4
Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
