Re: [PATCH v2 4/7] scsi: ufs: core: Add hwq print for debug

[Powen Kao (高伯文)]

Hi Ziqi,

Thanks for ur comments.

This piece of code successfully dump relevent registers on our
platform. As you know, mcq error handling flow is not ready yet so the
insertion point might not seems to be reasonable. 

Maybe drop this patch for now, I will send it later with error handling
patches.


On Thu, 2023-02-23 at 18:14 +0800, Ziqi Chen wrote:
> Hi Po-Wen,
> 
> On 2/22/2023 11:04 AM, Po-Wen Kao wrote:
> > +void ufshcd_mcq_print_hwqs(struct ufs_hba *hba, unsigned long
> > bitmap)
> > +{
> > +	int id, i;
> > +	char prefix[15];
> > +
> > +	if (!is_mcq_enabled(hba))
> > +		return;
> > +
> > +	for_each_set_bit(id, &bitmap, hba->nr_hw_queues) {
> > +		snprintf(prefix, sizeof(prefix), "q%d SQCFG: ", id);
> > +		ufshcd_hex_dump(prefix,
> > +			hba->mcq_base + MCQ_QCFG_SIZE * id,
> > MCQ_QCFG_SQ_SIZE);
> 
> Is your purpose dump per hardware queue registers here?  If yes, why 
> don't use ufsmcq_readl() to save to a buffer and then use
> ufshcd_hex_dump()
> 
> to dump ? Are you sure ufshcd_hex_dump() can dump register directly?
> 
> > +
> > +		snprintf(prefix, sizeof(prefix), "q%d CQCFG: ", id);
> > +		ufshcd_hex_dump(prefix,
> > +			hba->mcq_base + MCQ_QCFG_SIZE * id +
> > MCQ_QCFG_SQ_SIZE, MCQ_QCFG_CQ_SIZE);
> 
> Same to above comment.
> > +
> > +		for (i = 0; i < OPR_MAX ; i++) {
> > +			snprintf(prefix, sizeof(prefix), "q%d OPR%d: ",
> > id, i);
> > +			ufshcd_hex_dump(prefix, mcq_opr_base(hba, i,
> > id), mcq_opr_size[i]);
> 
> Same.
> > +		}
> > +	}
> > +}
> > +
> > 
> >   
> > @@ -574,7 +569,16 @@ void ufshcd_print_trs(struct ufs_hba *hba,
> > unsigned long bitmap, bool pr_prdt)
> >   		if (pr_prdt)
> >   			ufshcd_hex_dump("UPIU PRDT: ", lrbp-
> > >ucd_prdt_ptr,
> >   				ufshcd_sg_entry_size(hba) *
> > prdt_length);
> > +
> > +		if (is_mcq_enabled(hba)) {
> > +			cmd = lrbp->cmd;
> > +			if (!cmd)
> > +				return;
> > +			hwq = ufshcd_mcq_req_to_hwq(hba,
> > scsi_cmd_to_rq(cmd));
> > +			ufshcd_mcq_print_hwqs(hba, 1 << hwq->id);
> 
> Calling registers dump function in ufshcd_print_trs() is not
> reasonable, 
> eg.. for each aborted request, it would print out all hwq registers, 
> it's not make sense.
> 
> I think we should move it out of ufshcd_print_trs().
> 
> > +		}
> >   	}
> > +
> >   }
> 
> 
> Best Regards,
> 
> Ziqi
>