Re: [PATCH v2 1/9] drivers/char: Add support for Xue USB3 debugger

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Jul 14, 2022 at 01:58:25PM +0200, Jan Beulich wrote:
> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> > +static int xue_init_dbc(struct xue *xue)
> > +{
> > +    uint64_t erdp = 0;
> > +    uint64_t out = 0;
> > +    uint64_t in = 0;
> > +    uint64_t mbs = 0;
> > +    struct xue_dbc_reg *reg = xue_find_dbc(xue);
> > +
> > +    if ( !reg )
> > +        return 0;
> > +
> > +    xue->dbc_reg = reg;
> > +    xue_disable_dbc(xue);
> > +
> > +    xue_trb_ring_init(xue, &xue->dbc_ering, 0, XUE_DB_INVAL);
> > +    xue_trb_ring_init(xue, &xue->dbc_oring, 1, XUE_DB_OUT);
> > +    xue_trb_ring_init(xue, &xue->dbc_iring, 1, XUE_DB_IN);
> > +
> > +    erdp = virt_to_maddr(xue->dbc_ering.trb);
> > +    if ( !erdp )
> > +        return 0;
> > +
> > +    memset(xue->dbc_erst, 0, sizeof(*xue->dbc_erst));
> > +    xue->dbc_erst->base = erdp;
> > +    xue->dbc_erst->size = XUE_TRB_RING_CAP;
> > +
> > +    mbs = (reg->ctrl & 0xFF0000) >> 16;
> > +    out = virt_to_maddr(xue->dbc_oring.trb);
> > +    in = virt_to_maddr(xue->dbc_iring.trb);
> > +
> > +    memset(xue->dbc_ctx, 0, sizeof(*xue->dbc_ctx));
> > +    xue_init_strings(xue, xue->dbc_ctx->info);
> > +    xue_init_ep(xue->dbc_ctx->ep_out, mbs, xue_ep_bulk_out, out);
> > +    xue_init_ep(xue->dbc_ctx->ep_in, mbs, xue_ep_bulk_in, in);
> > +
> > +    reg->erstsz = 1;
> > +    reg->erstba = virt_to_maddr(xue->dbc_erst);
> > +    reg->erdp = erdp;
> > +    reg->cp = virt_to_maddr(xue->dbc_ctx);
> 
> The only place this field is read looks to be xue_dump().
> 
> > +static struct xue_trb evt_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
> > +static struct xue_trb out_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
> > +static struct xue_trb in_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
> > +static struct xue_erst_segment erst __aligned(64);
> > +static struct xue_dbc_ctx ctx __aligned(64);
> > +static uint8_t wrk_buf[XUE_WORK_RING_CAP] __aligned(XUE_PAGE_SIZE);
> > +static char str_buf[XUE_PAGE_SIZE] __aligned(64);
> 
> While I think I can see the point of the page-size alignment, can you
> please clarify the need for the three instances of 64-byte alignment?

(Guessing why original author of this code did it this way) At least
ERSTBA (5.5.2.3.2 section of the spec) is required to be 64-byte aligned
by the xHCI spec. Interestingly the DbC version of this register
(DCERSTBA, section 7.6.8.3.2) requires just 16-byte alignment.
ctx seems to require just 16-byte alignment too, and str_buf (in
practice) requires just 2-byte alignment.
I'll try to reduce those alignments and see if that still works...

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature