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

Alamy Liu
2019-02-11T23:04:28Z

Thanks for the clarification.

On Mon, Feb 11, 2019 at 12:57 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 8/02/19 8:41 PM, Alamy Liu wrote:
> > 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)
>
> The cqe_qdepth changes depending on whether DCMD is supported. That is the
> kind of detail that belongs to the CQHCI driver not the mmc core. For
> example, theoretically there could be more than one CQE driver
> implementation, but it is also nicer to keep a separation between CQHCI and
> mmc core.
>
> >
> > Best Regards
> >
> >
> > On Fri, Feb 8, 2019 at 2:34 AM Asutosh Das (asd)
> > <asutoshd@codeaurora.org> 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@intel.com
> >>>> <mailto:adrian.hunter@intel.com>> 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@gmail.com
> >>>> <mailto:alamy.liu@gmail.com>>
> >>>> > ---
> >>>> > 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
> >
>