Hi David,
All right, I'm finally starting to catch up on my patch queue. :)
Sorry for the delay.
In general this looks like a good patch, thanks for submitting it.
However, there's a couple things I need you to fix.
On Mon, Nov 04, 2013 at 11:36:00AM -0000, David Laight wrote:
> In many cases the constant 1 is used for values that eventually
> get written to the hardware ring. Replace all of these with the
> symbolic constant TRB_CYCLE.
>
> This makes it clear that the ring->cycle_state value is used when
> writing to the actual ring itself.
>
> Always use ^= TRB_CYCLE to invert the bit.
> ---
The first is that you forgot to include a Signed-off-by line here.
Running your patch through scripts/checkpatch.pl will help you catch
these kinds of errors.
The second is that this patch triggers some checkpatch warnings because
some of your changes caused the lines to go over 80 characters:
sarah@xanatos:~/git/kernels/xhci$ git commit -v
WARNING: line over 80 characters
#10: FILE: drivers/usb/host/xhci-mem.c:601:
+ xhci_ring_alloc(xhci, 2, TRB_CYCLE, TYPE_STREAM, mem_flags);
WARNING: line over 80 characters
#19: FILE: drivers/usb/host/xhci-mem.c:909:
+ dev->eps[0].ring = xhci_ring_alloc(xhci, 2, TRB_CYCLE, TYPE_CTRL, flags);
WARNING: line over 80 characters
#37: FILE: drivers/usb/host/xhci-mem.c:2304:
+ xhci->cmd_ring = xhci_ring_alloc(xhci, 1, TRB_CYCLE, TYPE_COMMAND, flags);
WARNING: line over 80 characters
#46: FILE: drivers/usb/host/xhci-mem.c:2348:
+ xhci->event_ring = xhci_ring_alloc(xhci, ERST_NUM_SEGS, TRB_CYCLE, TYPE_EVENT,
total: 0 errors, 4 warnings, 112 lines checked
Your patch has style problems, please review.
Can you please break those lines at 80 characters?
Thanks,
Sarah Sharp
> drivers/usb/host/xhci-mem.c | 10 +++++-----
> drivers/usb/host/xhci-ring.c | 16 ++++++++--------
> drivers/usb/host/xhci.c | 2 +-
> 3 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 9b5d1c3..5b2d835 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -597,7 +597,7 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct xhci_hcd *xhci,
> */
> for (cur_stream = 1; cur_stream < num_streams; cur_stream++) {
> stream_info->stream_rings[cur_stream] =
> - xhci_ring_alloc(xhci, 2, 1, TYPE_STREAM, mem_flags);
> + xhci_ring_alloc(xhci, 2, TRB_CYCLE, TYPE_STREAM, mem_flags);
> cur_ring = stream_info->stream_rings[cur_stream];
> if (!cur_ring)
> goto cleanup_rings;
> @@ -906,7 +906,7 @@ int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
> }
>
> /* Allocate endpoint 0 ring */
> - dev->eps[0].ring = xhci_ring_alloc(xhci, 2, 1, TYPE_CTRL, flags);
> + dev->eps[0].ring = xhci_ring_alloc(xhci, 2, TRB_CYCLE, TYPE_CTRL, flags);
> if (!dev->eps[0].ring)
> goto fail;
>
> @@ -1326,7 +1326,7 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
> type = usb_endpoint_type(&ep->desc);
> /* Set up the endpoint ring */
> virt_dev->eps[ep_index].new_ring =
> - xhci_ring_alloc(xhci, 2, 1, type, mem_flags);
> + xhci_ring_alloc(xhci, 2, TRB_CYCLE, type, mem_flags);
> if (!virt_dev->eps[ep_index].new_ring) {
> /* Attempt to use the ring cache */
> if (virt_dev->num_rings_cached == 0)
> @@ -2311,7 +2311,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
> goto fail;
>
> /* Set up the command ring to have one segments for now. */
> - xhci->cmd_ring = xhci_ring_alloc(xhci, 1, 1, TYPE_COMMAND, flags);
> + xhci->cmd_ring = xhci_ring_alloc(xhci, 1, TRB_CYCLE, TYPE_COMMAND, flags);
> if (!xhci->cmd_ring)
> goto fail;
> xhci_dbg_trace(xhci, trace_xhci_dbg_init,
> @@ -2355,7 +2355,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
> * the event ring segment table (ERST). Section 4.9.3.
> */
> xhci_dbg_trace(xhci, trace_xhci_dbg_init, "// Allocating event ring");
> - xhci->event_ring = xhci_ring_alloc(xhci, ERST_NUM_SEGS, 1, TYPE_EVENT,
> + xhci->event_ring = xhci_ring_alloc(xhci, ERST_NUM_SEGS, TRB_CYCLE, TYPE_EVENT,
> flags);
> if (!xhci->event_ring)
> goto fail;
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index d3f4a9a..408978b 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -178,7 +178,7 @@ static void inc_deq(struct xhci_hcd *xhci, struct xhci_ring *ring)
> if (ring->type == TYPE_EVENT &&
> last_trb_on_last_seg(xhci, ring,
> ring->deq_seg, ring->dequeue)) {
> - ring->cycle_state = (ring->cycle_state ? 0 : 1);
> + ring->cycle_state ^= TRB_CYCLE;
> }
> ring->deq_seg = ring->deq_seg->next;
> ring->dequeue = ring->deq_seg->trbs;
> @@ -257,7 +257,7 @@ static void inc_enq(struct xhci_hcd *xhci, struct xhci_ring *ring,
>
> /* Toggle the cycle bit after the last ring segment. */
> if (last_trb_on_last_seg(xhci, ring, ring->enq_seg, next)) {
> - ring->cycle_state = (ring->cycle_state ? 0 : 1);
> + ring->cycle_state ^= TRB_CYCLE;
> }
> }
> ring->enq_seg = ring->enq_seg->next;
> @@ -475,7 +475,7 @@ static struct xhci_segment *find_trb_seg(
> &cur_seg->trbs[TRBS_PER_SEGMENT - 1] < trb) {
> generic_trb = &cur_seg->trbs[TRBS_PER_SEGMENT - 1].generic;
> if (generic_trb->field[3] & cpu_to_le32(LINK_TOGGLE))
> - *cycle_state ^= 0x1;
> + *cycle_state ^= TRB_CYCLE;
> cur_seg = cur_seg->next;
> if (cur_seg == start_seg)
> /* Looped over the entire list. Oops! */
> @@ -2967,7 +2967,7 @@ static int prepare_ring(struct xhci_hcd *xhci, struct xhci_ring *ep_ring,
>
> /* Toggle the cycle bit after the last ring segment. */
> if (last_trb_on_last_seg(xhci, ring, ring->enq_seg, next)) {
> - ring->cycle_state = (ring->cycle_state ? 0 : 1);
> + ring->cycle_state ^= TRB_CYCLE;
> }
> ring->enq_seg = ring->enq_seg->next;
> ring->enqueue = ring->enq_seg->trbs;
> @@ -3253,7 +3253,7 @@ static int queue_bulk_sg_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> if (first_trb) {
> first_trb = false;
> if (start_cycle == 0)
> - field |= 0x1;
> + field |= TRB_CYCLE;
> } else
> field |= ep_ring->cycle_state;
>
> @@ -3416,7 +3416,7 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> if (first_trb) {
> first_trb = false;
> if (start_cycle == 0)
> - field |= 0x1;
> + field |= TRB_CYCLE;
> } else
> field |= ep_ring->cycle_state;
>
> @@ -3531,7 +3531,7 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> field = 0;
> field |= TRB_IDT | TRB_TYPE(TRB_SETUP);
> if (start_cycle == 0)
> - field |= 0x1;
> + field |= TRB_CYCLE;
>
> /* xHCI 1.0 6.4.1.2.1: Transfer Type field */
> if (xhci->hci_version == 0x100) {
> @@ -3741,7 +3741,7 @@ static int xhci_queue_isoc_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> field |= TRB_SIA;
> if (i == 0) {
> if (start_cycle == 0)
> - field |= 0x1;
> + field |= TRB_CYCLE;
> } else
> field |= ep_ring->cycle_state;
> first_trb = false;
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 0969f74..95729e5 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -825,7 +825,7 @@ static void xhci_clear_command_ring(struct xhci_hcd *xhci)
> * Ring is now zeroed, so the HW should look for change of ownership
> * when the cycle bit is set to 1.
> */
> - ring->cycle_state = 1;
> + ring->cycle_state = TRB_CYCLE;
>
> /*
> * Reset the hardware dequeue pointer.
> --
> 1.8.1.2
>
>
>
--
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
