| Message ID | 1467033757-32498-7-git-send-email-riteshh@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27/06/16 16:22, Ritesh Harjani wrote: > From: Subhash Jadavani <subhashj@codeaurora.org> > > SDMA buffer boundary size parameter in block size register should only be > programmed if host controller DMA is operating in SDMA mode otherwise its > better not to set this parameter to avoid any side effect when DMA is > operating in ADMA mode operation. Pedantically, the SDHCI specification does not say the SDMA Buffer Boundary should not be set in ADMA mode. All it says is that ADMA does not use it. In fact it is impossible to avoid writing to it because it is part of the Block Size register. Unfortunately the value 0 does not mean "not used" but instead means the boundary is 4K whereas the value presently being written (7 which is the highest) means 512K. Given that we have always been writing 7 to it, I don't see any reason to change. Murphy's law says if we do change it, someone else's driver will break. However I presume you have hardware where value 7 doesn't work. Can you clarify that? > > Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org> > [venkatg@codeaurora.org: fix trivial merge conflict] > Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org> > --- > drivers/mmc/host/sdhci.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index 0e3d7c0..9f5cdaa 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -742,6 +742,17 @@ static void sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd) > } > } > > +static void sdhci_set_blk_size_reg(struct sdhci_host *host, unsigned int blksz, > + unsigned int sdma_boundary) > +{ > + if (host->flags & SDHCI_USE_ADMA) > + sdhci_writew(host, SDHCI_MAKE_BLKSZ(0, blksz), > + SDHCI_BLOCK_SIZE); > + else > + sdhci_writew(host, SDHCI_MAKE_BLKSZ(sdma_boundary, blksz), > + SDHCI_BLOCK_SIZE); > +} > + > static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd) > { > u8 ctrl; > @@ -874,8 +885,7 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd) > sdhci_set_transfer_irqs(host); > > /* Set the DMA boundary value and block size */ > - sdhci_writew(host, SDHCI_MAKE_BLKSZ(SDHCI_DEFAULT_BOUNDARY_ARG, > - data->blksz), SDHCI_BLOCK_SIZE); > + sdhci_set_blk_size_reg(host, data->blksz, SDHCI_DEFAULT_BOUNDARY_ARG); > sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT); > } > > @@ -1935,14 +1945,11 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode) > */ > if (cmd.opcode == MMC_SEND_TUNING_BLOCK_HS200) { > if (mmc->ios.bus_width == MMC_BUS_WIDTH_8) > - sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, 128), > - SDHCI_BLOCK_SIZE); > + sdhci_set_blk_size_reg(host, 128, 7); > else if (mmc->ios.bus_width == MMC_BUS_WIDTH_4) > - sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, 64), > - SDHCI_BLOCK_SIZE); > + sdhci_set_blk_size_reg(host, 64, 7); > } else { > - sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, 64), > - SDHCI_BLOCK_SIZE); > + sdhci_set_blk_size_reg(host, 64, 7); > } > > /* > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Adrian, On 6/29/2016 4:25 PM, Adrian Hunter wrote: > On 27/06/16 16:22, Ritesh Harjani wrote: >> From: Subhash Jadavani <subhashj@codeaurora.org> >> >> SDMA buffer boundary size parameter in block size register should only be >> programmed if host controller DMA is operating in SDMA mode otherwise its >> better not to set this parameter to avoid any side effect when DMA is >> operating in ADMA mode operation. > > Pedantically, the SDHCI specification does not say the SDMA Buffer Boundary > should not be set in ADMA mode. All it says is that ADMA does not use it. > In fact it is impossible to avoid writing to it because it is part of the > Block Size register. Unfortunately the value 0 does not mean "not used" but > instead means the boundary is 4K whereas the value presently being written > (7 which is the highest) means 512K. Ok. > > Given that we have always been writing 7 to it, I don't see any reason to > change. Murphy's law says if we do change it, someone else's driver will break. > > However I presume you have hardware where value 7 doesn't work. Can you > clarify that? I think this was good to have patch, since SDHCI spec does not explicitly mention about this for ADMA. But agree with what you have mentioned. I will drop this patch then. > >> >> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org> >> [venkatg@codeaurora.org: fix trivial merge conflict] >> Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org> >> --- >> drivers/mmc/host/sdhci.c | 23 +++++++++++++++-------- >> 1 file changed, 15 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >> index 0e3d7c0..9f5cdaa 100644 >> --- a/drivers/mmc/host/sdhci.c >> +++ b/drivers/mmc/host/sdhci.c >> @@ -742,6 +742,17 @@ static void sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd) >> } >> } >> >> +static void sdhci_set_blk_size_reg(struct sdhci_host *host, unsigned int blksz, >> + unsigned int sdma_boundary) >> +{ >> + if (host->flags & SDHCI_USE_ADMA) >> + sdhci_writew(host, SDHCI_MAKE_BLKSZ(0, blksz), >> + SDHCI_BLOCK_SIZE); >> + else >> + sdhci_writew(host, SDHCI_MAKE_BLKSZ(sdma_boundary, blksz), >> + SDHCI_BLOCK_SIZE); >> +} >> + >> static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd) >> { >> u8 ctrl; >> @@ -874,8 +885,7 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd) >> sdhci_set_transfer_irqs(host); >> >> /* Set the DMA boundary value and block size */ >> - sdhci_writew(host, SDHCI_MAKE_BLKSZ(SDHCI_DEFAULT_BOUNDARY_ARG, >> - data->blksz), SDHCI_BLOCK_SIZE); >> + sdhci_set_blk_size_reg(host, data->blksz, SDHCI_DEFAULT_BOUNDARY_ARG); >> sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT); >> } >> >> @@ -1935,14 +1945,11 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode) >> */ >> if (cmd.opcode == MMC_SEND_TUNING_BLOCK_HS200) { >> if (mmc->ios.bus_width == MMC_BUS_WIDTH_8) >> - sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, 128), >> - SDHCI_BLOCK_SIZE); >> + sdhci_set_blk_size_reg(host, 128, 7); >> else if (mmc->ios.bus_width == MMC_BUS_WIDTH_4) >> - sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, 64), >> - SDHCI_BLOCK_SIZE); >> + sdhci_set_blk_size_reg(host, 64, 7); >> } else { >> - sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, 64), >> - SDHCI_BLOCK_SIZE); >> + sdhci_set_blk_size_reg(host, 64, 7); >> } >> >> /* >> > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 0e3d7c0..9f5cdaa 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -742,6 +742,17 @@ static void sdhci_set_timeout(struct sdhci_host *host, struct mmc_command *cmd) } } +static void sdhci_set_blk_size_reg(struct sdhci_host *host, unsigned int blksz, + unsigned int sdma_boundary) +{ + if (host->flags & SDHCI_USE_ADMA) + sdhci_writew(host, SDHCI_MAKE_BLKSZ(0, blksz), + SDHCI_BLOCK_SIZE); + else + sdhci_writew(host, SDHCI_MAKE_BLKSZ(sdma_boundary, blksz), + SDHCI_BLOCK_SIZE); +} + static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd) { u8 ctrl; @@ -874,8 +885,7 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd) sdhci_set_transfer_irqs(host); /* Set the DMA boundary value and block size */ - sdhci_writew(host, SDHCI_MAKE_BLKSZ(SDHCI_DEFAULT_BOUNDARY_ARG, - data->blksz), SDHCI_BLOCK_SIZE); + sdhci_set_blk_size_reg(host, data->blksz, SDHCI_DEFAULT_BOUNDARY_ARG); sdhci_writew(host, data->blocks, SDHCI_BLOCK_COUNT); } @@ -1935,14 +1945,11 @@ static int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode) */ if (cmd.opcode == MMC_SEND_TUNING_BLOCK_HS200) { if (mmc->ios.bus_width == MMC_BUS_WIDTH_8) - sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, 128), - SDHCI_BLOCK_SIZE); + sdhci_set_blk_size_reg(host, 128, 7); else if (mmc->ios.bus_width == MMC_BUS_WIDTH_4) - sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, 64), - SDHCI_BLOCK_SIZE); + sdhci_set_blk_size_reg(host, 64, 7); } else { - sdhci_writew(host, SDHCI_MAKE_BLKSZ(7, 64), - SDHCI_BLOCK_SIZE); + sdhci_set_blk_size_reg(host, 64, 7); } /*