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