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

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

[Alamy Liu]

Prevent to use fixed value (NUM_SLOTS) after it had been determined
and saved in a variable (cq_host->num_slots).

Signed-off-by: Alamy Liu <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;
-- 
2.17.1

[PATCH 2/2] mmc: cqhci: replace DCMD_SLOT with cq_host->dcmd_slot

[Alamy Liu]

Prevent to use fixed value (DCMD_SLOT) after it had been determined
and saved in a variable (cq_host->dcmd).

Signed-off-by: Alamy Liu <alamy.liu@gmail.com>
---
 drivers/mmc/host/cqhci.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c
index 26d63594b7..4cc7863c13 100644
--- a/drivers/mmc/host/cqhci.c
+++ b/drivers/mmc/host/cqhci.c
@@ -76,7 +76,8 @@ static void setup_trans_desc(struct cqhci_host *cq_host, u8 tag)
 	if (cq_host->link_desc_len > 8)
 		*(link_temp + 8) = 0;
 
-	if (tag == DCMD_SLOT && (cq_host->mmc->caps2 & MMC_CAP2_CQE_DCMD)) {
+	if ((tag == cq_host->dcmd_slot)
+	    && (cq_host->mmc->caps2 & MMC_CAP2_CQE_DCMD)) {
 		*link_temp = CQHCI_VALID(0) | CQHCI_ACT(0) | CQHCI_END(1);
 		return;
 	}
@@ -548,9 +549,9 @@ static void cqhci_post_req(struct mmc_host *host, struct mmc_request *mrq)
 	}
 }
 
-static inline int cqhci_tag(struct mmc_request *mrq)
+static inline int cqhci_tag(struct mmc_request *mrq, struct cqhci_host *cq_host)
 {
-	return mrq->cmd ? DCMD_SLOT : mrq->tag;
+	return mrq->cmd ? cq_host->dcmd_slot : mrq->tag;
 }
 
 static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
@@ -558,8 +559,8 @@ static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 	int err = 0;
 	u64 data = 0;
 	u64 *task_desc = NULL;
-	int tag = cqhci_tag(mrq);
 	struct cqhci_host *cq_host = mmc->cqe_private;
+	int tag = cqhci_tag(mrq, cq_host);
 	unsigned long flags;
 
 	if (!cq_host->enabled) {
@@ -824,7 +825,7 @@ static bool cqhci_timeout(struct mmc_host *mmc, struct mmc_request *mrq,
 			  bool *recovery_needed)
 {
 	struct cqhci_host *cq_host = mmc->cqe_private;
-	int tag = cqhci_tag(mrq);
+	int tag = cqhci_tag(mrq, cq_host);
 	struct cqhci_slot *slot = &cq_host->slot[tag];
 	unsigned long flags;
 	bool timed_out;
-- 
2.17.1

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

[Ulf Hansson]

+ Adrian (the SDHCI maintainer)

On Mon, 14 Jan 2019 at 20:17, Alamy Liu <alamy.liu@gmail.com> wrote:
>
> Prevent to use fixed value (NUM_SLOTS) after it had been determined
> and saved in a variable (cq_host->num_slots).
>
> Signed-off-by: Alamy Liu <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;
> --
> 2.17.1
>

Alamy, I realized that you haven't included Adrian when posted your
recent sdhci patches. Please re-post all of them so we can get
Adrian's input.

Kind regards
Uffe

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

[Adrian Hunter]

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>
> ---
>  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;
> 

Re: [PATCH 2/2] mmc: cqhci: replace DCMD_SLOT with cq_host->dcmd_slot

[Adrian Hunter]

On 14/01/19 9:17 PM, Alamy Liu wrote:
> Prevent to use fixed value (DCMD_SLOT) after it had been determined
> and saved in a variable (cq_host->dcmd).

dcmd_slot is always 31 (i.e. DCMD_SLOT) so why not go the other way and get
rid of dcmd_slot and just use DCMD_SLOT?

> 
> Signed-off-by: Alamy Liu <alamy.liu@gmail.com>
> ---
>  drivers/mmc/host/cqhci.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c
> index 26d63594b7..4cc7863c13 100644
> --- a/drivers/mmc/host/cqhci.c
> +++ b/drivers/mmc/host/cqhci.c
> @@ -76,7 +76,8 @@ static void setup_trans_desc(struct cqhci_host *cq_host, u8 tag)
>  	if (cq_host->link_desc_len > 8)
>  		*(link_temp + 8) = 0;
>  
> -	if (tag == DCMD_SLOT && (cq_host->mmc->caps2 & MMC_CAP2_CQE_DCMD)) {
> +	if ((tag == cq_host->dcmd_slot)
> +	    && (cq_host->mmc->caps2 & MMC_CAP2_CQE_DCMD)) {
>  		*link_temp = CQHCI_VALID(0) | CQHCI_ACT(0) | CQHCI_END(1);
>  		return;
>  	}
> @@ -548,9 +549,9 @@ static void cqhci_post_req(struct mmc_host *host, struct mmc_request *mrq)
>  	}
>  }
>  
> -static inline int cqhci_tag(struct mmc_request *mrq)
> +static inline int cqhci_tag(struct mmc_request *mrq, struct cqhci_host *cq_host)
>  {
> -	return mrq->cmd ? DCMD_SLOT : mrq->tag;
> +	return mrq->cmd ? cq_host->dcmd_slot : mrq->tag;
>  }
>  
>  static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
> @@ -558,8 +559,8 @@ static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  	int err = 0;
>  	u64 data = 0;
>  	u64 *task_desc = NULL;
> -	int tag = cqhci_tag(mrq);
>  	struct cqhci_host *cq_host = mmc->cqe_private;
> +	int tag = cqhci_tag(mrq, cq_host);
>  	unsigned long flags;
>  
>  	if (!cq_host->enabled) {
> @@ -824,7 +825,7 @@ static bool cqhci_timeout(struct mmc_host *mmc, struct mmc_request *mrq,
>  			  bool *recovery_needed)
>  {
>  	struct cqhci_host *cq_host = mmc->cqe_private;
> -	int tag = cqhci_tag(mrq);
> +	int tag = cqhci_tag(mrq, cq_host);
>  	struct cqhci_slot *slot = &cq_host->slot[tag];
>  	unsigned long flags;
>  	bool timed_out;
> 

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

[Asutosh Das (asd)]

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

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@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

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

[Adrian Hunter]

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
> 

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

[Alamy Liu]

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
> >
>