RE: [PATCH] mmc: host: renesas_internal_dmac: add pre_req and post_req support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Wolfram-san,

Thank you for your review!

> From: Wolfram Sang, Sent: Tuesday, December 15, 2020 12:50 AM
> 
> Hi Shimoda-san,
> 
> On Fri, Dec 04, 2020 at 10:17:33PM +0900, Yoshihiro Shimoda wrote:
> > Add pre_req and post_req support to improve performance.
> >
> > Inspired by a patch in the BSP by Masaharu Hayakawa.
> 
> Thank you for upporting this!
> 
> >  /*
> >   * Specification of this driver:
> >   * - host->chan_{rx,tx} will be used as a flag of enabling/disabling the dma
> > @@ -172,6 +178,47 @@ renesas_sdhi_internal_dmac_dataend_dma(struct tmio_mmc_host *host) {
> >  	tasklet_schedule(&priv->dma_priv.dma_complete);
> >  }
> >
> > +/* Should not use host->sg_ptr/sg_len in the following function */
> 
> Maybe a short explanation why we shouldn't use the functions?

I tried to update the comment as below:
/*
 * tmio_mmc_request() only sets host->sg_{ptr,len} and
 * renesas_sdhi_internal_dmac_pre_req() doesn't set host->sg_{ptr,len} so that
 * we should not use the values in the following function.
 */

Hmm... Perhaps, I should modify the code to use host->sg_{ptr,len}
in both paths (.request() and .pre_req()) and remove this comments.
So, I'll try to modify. I guess tmio_mmc_init_sg() is called in pre_req(),
we can use host->sg_{ptr,len}.

> > +static void
> > +renesas_sdhi_internal_dmac_unmap(struct tmio_mmc_host *host,
> > +				 struct mmc_data *data,
> > +				 enum renesas_sdhi_dma_cookie cookie,
> > +				 bool expected_unmatch)
> 
> Can we maybe skip "expected_unmatch"? It is always true for
> COOKIE_UNMAPPED and always false for the COOKIE_*MAPPED values, or?
> 
> > +{
> > +	bool unmap = expected_unmatch ? (data->host_cookie != cookie) :
> > +					(data->host_cookie == cookie);
> 
> Then, we could do:
>  +	bool unmap = cookie == COOKIE_UNMAPPED ? (data->host_cookie != cookie) :
>  +					(data->host_cookie == cookie);

Thank you for your suggestion! You're correct. I'll fix this.

> > +
> > +	if (unmap) {
> > +		dma_unmap_sg(&host->pdev->dev, data->sg, data->sg_len,
> > +			     mmc_get_dma_dir(data));
> > +		data->host_cookie = COOKIE_UNMAPPED;
> > +	}
> 
> Is it maybe worth a warning if the expected condition was not found?

If we could add such a warning, it's helpful. However,
I have no idea how to implement a condition for it because
we cannot use "else" here. For example, when this driver mapped
as PRE_MAPPED and then renesas_sdhi_internal_dmac_complete() is called,
the "unmap" value of renesas_sdhi_internal_dmac_unmap()
from renesas_sdhi_internal_dmac_complete() is false:

	data->host_cookie = PRE_MAPPED,
  and
	cookie = MAPPED

This is because this driver should unmap in .post_req() so that
this is the expected condition in such a case. But, what do you think?

> Rest looks good!

Thanks!

Best regards,
Yoshihiro Shimoda





[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux