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
