Re: [PATCH 1/2] mmc: cqhci: replace NUM_SLOTS with cq_host->num_slots

[Alamy Liu]

The IP I'm using has fixed 32 slots for CQ once CQE is enabled in RTL.
There is no other RO register to say the slot number & which slot that
DCMD would use.

I agree that most host controller would have 32 slots for CQ, and use
the last one for DCMD if enabled (easier design & save gates
count/size). (15th for DCMD if there were 16 slots in CQ, ...etc). It
also doesn't make sense to use a slot in the middle of slots for DCMD,
Both the code (both S.W. & H.W.) would become more complicated.
Although I don't see it is defined somewhere, I believe that every
designer would follow.
(H.W.: increasing gate count -> increasing die size -> increase $;
complicated design -> higher chance to have bug/error).
(H.W.: SDHC cq_depth: longer cq_depth -> more memory -> more $. The
decision would be the trade off between $ & performance. Also, the
SDHC is not necessary to be used in Cortex-A/Linux. Thus, 16/8
cq_depth is possible, although I don't know if there is one existed
out there)

Another thinking: If all cq_depth is fixed to 32, the mmc driver core
doesn't even have to detect the cq_depth and eventually choose the
small one between HC & eMMC (queue.c::mmc_init_queue)

Best Regards


On Fri, Feb 8, 2019 at 2:34 AM Asutosh Das (asd)
<asutoshd@xxxxxxxxxxxxxx> wrote:
>
> On 2/8/2019 8:07 AM, Ritesh Harjani wrote:
> > Hi Alamy,
> >
> > On 2/8/2019 1:00 AM, Alamy Liu wrote:
> >> It says in B.2.1 in the JESD84-B51.pdf (I don't have JESD84-B51A.pdf):
> >>
> >>     /The TDL is located in a memory location known to the CQE, and is
> >>     comprised of up to 32 fixed-size slots. Each slot is comprised of
> >>     one Task Descriptor and one Transfer Descriptor./
> >>
> >>
> >> So if the IP has 16 slots, it should still meet the specification.
> >> Then the configuration could be moved to DTS, maybe:
> >>
> >>     cqhci-slotnum = <16>;
> >>
> >>     cqhci-dcmd-slotno = <15>;
> >>
> > Does your IP defines this as 16 slots & DCMD slot to be 15?
> > Because if there is a deviation then IP may also define num_slots by
> > using some reserved registers.
> > Also in JESD84-B51.pdf, specific slot no. is used extensively for
> > defining policies(like in case of DCMD & CQCFG), so in that case
> > defining via DT may not be that helpful.
> > Unless there is no other way in your IP to determine the num_slots
> > except going via DT?
> >
> > Let others also provide an opinion here.
> >
> > Regards
> > Ritesh
> >
> >
> >>
> >> Please comment.
> >>
> >> Regards,
> >> Alamy
> >>
> >>
> >>
> >> On Thu, Jan 31, 2019 at 1:34 AM Adrian Hunter <adrian.hunter@xxxxxxxxx
> >> <mailto:adrian.hunter@xxxxxxxxx>> wrote:
> >>
> >>     On 14/01/19 9:17 PM, Alamy Liu wrote:
> >>     > Prevent to use fixed value (NUM_SLOTS) after it had been determined
> >>     > and saved in a variable (cq_host->num_slots).
> >>
> >>     num_slots is always 32 (i.e. NUM_SLOTS) so why not go the other
> >>     way and get
> >>     rid of num_slots and just use NUM_SLOTS?
> >>
> >>     >
> >>     > Signed-off-by: Alamy Liu <alamy.liu@xxxxxxxxx
> >>     <mailto:alamy.liu@xxxxxxxxx>>
> >>     > ---
> >>     >  drivers/mmc/host/cqhci.c | 2 +-
> >>     >  1 file changed, 1 insertion(+), 1 deletion(-)
> >>     >
> >>     > diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c
> >>     > index 159270e947..26d63594b7 100644
> >>     > --- a/drivers/mmc/host/cqhci.c
> >>     > +++ b/drivers/mmc/host/cqhci.c
> >>     > @@ -699,7 +699,7 @@ static void cqhci_error_irq(struct mmc_host
> >>     *mmc, u32 status, int cmd_error,
> >>     >                * The only way to guarantee forward progress is
> >>     to mark at
> >>     >                * least one task in error, so if none is
> >>     indicated, pick one.
> >>     >                */
> >>     > -             for (tag = 0; tag < NUM_SLOTS; tag++) {
> >>     > +             for (tag = 0; tag < cq_host->num_slots; tag++) {
> >>     >                       slot = &cq_host->slot[tag];
> >>     >                       if (!slot->mrq)
> >>     >                               continue;
> >>     >
> >>
> Is it worth exploring to tie up the TDL memory allocations with the
> queue-depth? Because the queue-depth may vary with vendor; in most host
> controllers the slot size is 32. And since memory allocations are done
> on the basis of host slot size there's unused slots in the case of card
> advertising less than 32 queue-depth.
> The tricky part would be the DCMD handling though.
>
> In the IP in question, what slot is assigned to DCMD?
>
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
> Linux Foundation Collaborative Project