| Message ID | 20220424230640.31735-1-samuel@sholland.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: sunxi-mmc: Correct the maximum segment size | expand |
Hi Samuel, On 4/25/22 01:06, Samuel Holland wrote: > According to the DMA descriptor documentation, the lowest two bits of > the size field are ignored, so the size must be rounded up to a multiple > of 4 bytes. Furthermore, 0 is not a valid buffer size; setting the size > to 0 will cause that DMA descriptor to be ignored. > > Together, these restrictions limit the maximum DMA segment size to 4 > less than the power-of-two width of the size field. I assume that you were seeing some problems where things where not working which caused you to investigate this; and that this patch fixes those problems? If yes then it would be good to also mention the problems + investigative process in the commit message. I'm no longer involved in sunxi development, but still I wonder if the substraction of 4 from the max_seg_size is really necessary? This seems to be based on the notion that as you say "0 is not a valid buffer size" where as the code so far has been operating under the assumption that putting 0 in sunxi_idma_des.buf_size means maximum buf-size. I'm pretty sure that 0 meaning maximum buf-size is correct for at least the older chips where idma_des_size_bits equals 13, which means that only 2 4K pages fit in a single desc, so we almost certainly have been hitting this code path ? Although: drivers/mmc/host/dw_mmc.c which seems to be for similar hw suggests that on designs where idma_des_size_bits == 13 only 4k can be used, which sorta matches what you are doing here except that you limit things to 8k - 4 instead of to just 4k. Anyways I was just wondering about all this... Regards, Hans > > Fixes: 3cbcb16095f9 ("mmc: sunxi: Add driver for SD/MMC hosts found on Allwinner sunxi SoCs") > Signed-off-by: Samuel Holland <samuel@sholland.org> > --- > > drivers/mmc/host/sunxi-mmc.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c > index c62afd212692..4bd5f37b1036 100644 > --- a/drivers/mmc/host/sunxi-mmc.c > +++ b/drivers/mmc/host/sunxi-mmc.c > @@ -214,6 +214,9 @@ > #define SDXC_IDMAC_DES0_CES BIT(30) /* card error summary */ > #define SDXC_IDMAC_DES0_OWN BIT(31) /* 1-idma owns it, 0-host owns it */ > > +/* Buffer size must be a multiple of 4 bytes. */ > +#define SDXC_IDMAC_SIZE_ALIGN 4 > + > #define SDXC_CLK_400K 0 > #define SDXC_CLK_25M 1 > #define SDXC_CLK_50M 2 > @@ -361,17 +364,15 @@ static void sunxi_mmc_init_idma_des(struct sunxi_mmc_host *host, > { > struct sunxi_idma_des *pdes = (struct sunxi_idma_des *)host->sg_cpu; > dma_addr_t next_desc = host->sg_dma; > - int i, max_len = (1 << host->cfg->idma_des_size_bits); > + int i; > > for (i = 0; i < data->sg_len; i++) { > pdes[i].config = cpu_to_le32(SDXC_IDMAC_DES0_CH | > SDXC_IDMAC_DES0_OWN | > SDXC_IDMAC_DES0_DIC); > > - if (data->sg[i].length == max_len) > - pdes[i].buf_size = 0; /* 0 == max_len */ > - else > - pdes[i].buf_size = cpu_to_le32(data->sg[i].length); > + pdes[i].buf_size = cpu_to_le32(ALIGN(data->sg[i].length, > + SDXC_IDMAC_SIZE_ALIGN)); > > next_desc += sizeof(struct sunxi_idma_des); > pdes[i].buf_addr_ptr1 = > @@ -1420,7 +1421,8 @@ static int sunxi_mmc_probe(struct platform_device *pdev) > mmc->max_blk_count = 8192; > mmc->max_blk_size = 4096; > mmc->max_segs = PAGE_SIZE / sizeof(struct sunxi_idma_des); > - mmc->max_seg_size = (1 << host->cfg->idma_des_size_bits); > + mmc->max_seg_size = (1 << host->cfg->idma_des_size_bits) - > + SDXC_IDMAC_SIZE_ALIGN; > mmc->max_req_size = mmc->max_seg_size * mmc->max_segs; > /* 400kHz ~ 52MHz */ > mmc->f_min = 400000;
On 4/25/22 5:40 AM, Hans de Goede wrote: > Hi Samuel, > > On 4/25/22 01:06, Samuel Holland wrote: >> According to the DMA descriptor documentation, the lowest two bits of >> the size field are ignored, so the size must be rounded up to a multiple >> of 4 bytes. Furthermore, 0 is not a valid buffer size; setting the size >> to 0 will cause that DMA descriptor to be ignored. >> >> Together, these restrictions limit the maximum DMA segment size to 4 >> less than the power-of-two width of the size field. > > I assume that you were seeing some problems where things where not working > which caused you to investigate this; and that this patch fixes those > problems? If yes then it would be good to also mention the problems + > investigative process in the commit message. No, this is just based on reading the manual. I was investigating some problems when I originally wrote this patch, but they turned out to be unrelated, and reverting this patch doesn't cause any obvious regression. > I'm no longer involved in sunxi development, but still I wonder if the > subtraction of 4 from the max_seg_size is really necessary? This seems > to be based on the notion that as you say "0 is not a valid buffer size" > where as the code so far has been operating under the assumption that > putting 0 in sunxi_idma_des.buf_size means maximum buf-size. > > I'm pretty sure that 0 meaning maximum buf-size is correct for at least > the older chips where idma_des_size_bits equals 13, which means that > only 2 4K pages fit in a single desc, so we almost certainly have been > hitting this code path ? > > Although: drivers/mmc/host/dw_mmc.c which seems to be for similar > hw suggests that on designs where idma_des_size_bits == 13 only > 4k can be used, which sorta matches what you are doing here except > that you limit things to 8k - 4 instead of to just 4k. > > Anyways I was just wondering about all this... It probably deserves someone testing this specific scenario, so we can either verify the fix is needed, or add a comment explaining that the documentation is wrong. Regards, Samuel > Regards, > > Hans > > > >> >> Fixes: 3cbcb16095f9 ("mmc: sunxi: Add driver for SD/MMC hosts found on Allwinner sunxi SoCs") >> Signed-off-by: Samuel Holland <samuel@sholland.org> >> --- >> >> drivers/mmc/host/sunxi-mmc.c | 14 ++++++++------ >> 1 file changed, 8 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c >> index c62afd212692..4bd5f37b1036 100644 >> --- a/drivers/mmc/host/sunxi-mmc.c >> +++ b/drivers/mmc/host/sunxi-mmc.c >> @@ -214,6 +214,9 @@ >> #define SDXC_IDMAC_DES0_CES BIT(30) /* card error summary */ >> #define SDXC_IDMAC_DES0_OWN BIT(31) /* 1-idma owns it, 0-host owns it */ >> >> +/* Buffer size must be a multiple of 4 bytes. */ >> +#define SDXC_IDMAC_SIZE_ALIGN 4 >> + >> #define SDXC_CLK_400K 0 >> #define SDXC_CLK_25M 1 >> #define SDXC_CLK_50M 2 >> @@ -361,17 +364,15 @@ static void sunxi_mmc_init_idma_des(struct sunxi_mmc_host *host, >> { >> struct sunxi_idma_des *pdes = (struct sunxi_idma_des *)host->sg_cpu; >> dma_addr_t next_desc = host->sg_dma; >> - int i, max_len = (1 << host->cfg->idma_des_size_bits); >> + int i; >> >> for (i = 0; i < data->sg_len; i++) { >> pdes[i].config = cpu_to_le32(SDXC_IDMAC_DES0_CH | >> SDXC_IDMAC_DES0_OWN | >> SDXC_IDMAC_DES0_DIC); >> >> - if (data->sg[i].length == max_len) >> - pdes[i].buf_size = 0; /* 0 == max_len */ >> - else >> - pdes[i].buf_size = cpu_to_le32(data->sg[i].length); >> + pdes[i].buf_size = cpu_to_le32(ALIGN(data->sg[i].length, >> + SDXC_IDMAC_SIZE_ALIGN)); >> >> next_desc += sizeof(struct sunxi_idma_des); >> pdes[i].buf_addr_ptr1 = >> @@ -1420,7 +1421,8 @@ static int sunxi_mmc_probe(struct platform_device *pdev) >> mmc->max_blk_count = 8192; >> mmc->max_blk_size = 4096; >> mmc->max_segs = PAGE_SIZE / sizeof(struct sunxi_idma_des); >> - mmc->max_seg_size = (1 << host->cfg->idma_des_size_bits); >> + mmc->max_seg_size = (1 << host->cfg->idma_des_size_bits) - >> + SDXC_IDMAC_SIZE_ALIGN; >> mmc->max_req_size = mmc->max_seg_size * mmc->max_segs; >> /* 400kHz ~ 52MHz */ >> mmc->f_min = 400000; >
Hi, On 4/25/22 15:40, Samuel Holland wrote: > On 4/25/22 5:40 AM, Hans de Goede wrote: >> Hi Samuel, >> >> On 4/25/22 01:06, Samuel Holland wrote: >>> According to the DMA descriptor documentation, the lowest two bits of >>> the size field are ignored, so the size must be rounded up to a multiple >>> of 4 bytes. Furthermore, 0 is not a valid buffer size; setting the size >>> to 0 will cause that DMA descriptor to be ignored. >>> >>> Together, these restrictions limit the maximum DMA segment size to 4 >>> less than the power-of-two width of the size field. >> >> I assume that you were seeing some problems where things where not working >> which caused you to investigate this; and that this patch fixes those >> problems? If yes then it would be good to also mention the problems + >> investigative process in the commit message. > > No, this is just based on reading the manual. I was investigating some problems > when I originally wrote this patch, but they turned out to be unrelated, and > reverting this patch doesn't cause any obvious regression. I see, I'm not sure if we should proceed with this patch then, since at least on the older boards with 13 idma_des_size_bits the current code is likely correct given all the testing it has seen. >> I'm no longer involved in sunxi development, but still I wonder if the >> subtraction of 4 from the max_seg_size is really necessary? This seems >> to be based on the notion that as you say "0 is not a valid buffer size" >> where as the code so far has been operating under the assumption that >> putting 0 in sunxi_idma_des.buf_size means maximum buf-size. >> >> I'm pretty sure that 0 meaning maximum buf-size is correct for at least >> the older chips where idma_des_size_bits equals 13, which means that >> only 2 4K pages fit in a single desc, so we almost certainly have been >> hitting this code path ? >> >> Although: drivers/mmc/host/dw_mmc.c which seems to be for similar >> hw suggests that on designs where idma_des_size_bits == 13 only >> 4k can be used, which sorta matches what you are doing here except >> that you limit things to 8k - 4 instead of to just 4k. >> >> Anyways I was just wondering about all this... > > It probably deserves someone testing this specific scenario, so we can either > verify the fix is needed, or add a comment explaining that the documentation is > wrong. I agree that this should be verified before merging this patch. As for the documentation, which documentation are you referring too ? At least for the older sunxi SoCs which I'm familiar with the MMC controller bits were not documented at all. Regards, Hans >>> >>> Fixes: 3cbcb16095f9 ("mmc: sunxi: Add driver for SD/MMC hosts found on Allwinner sunxi SoCs") >>> Signed-off-by: Samuel Holland <samuel@sholland.org> >>> --- >>> >>> drivers/mmc/host/sunxi-mmc.c | 14 ++++++++------ >>> 1 file changed, 8 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c >>> index c62afd212692..4bd5f37b1036 100644 >>> --- a/drivers/mmc/host/sunxi-mmc.c >>> +++ b/drivers/mmc/host/sunxi-mmc.c >>> @@ -214,6 +214,9 @@ >>> #define SDXC_IDMAC_DES0_CES BIT(30) /* card error summary */ >>> #define SDXC_IDMAC_DES0_OWN BIT(31) /* 1-idma owns it, 0-host owns it */ >>> >>> +/* Buffer size must be a multiple of 4 bytes. */ >>> +#define SDXC_IDMAC_SIZE_ALIGN 4 >>> + >>> #define SDXC_CLK_400K 0 >>> #define SDXC_CLK_25M 1 >>> #define SDXC_CLK_50M 2 >>> @@ -361,17 +364,15 @@ static void sunxi_mmc_init_idma_des(struct sunxi_mmc_host *host, >>> { >>> struct sunxi_idma_des *pdes = (struct sunxi_idma_des *)host->sg_cpu; >>> dma_addr_t next_desc = host->sg_dma; >>> - int i, max_len = (1 << host->cfg->idma_des_size_bits); >>> + int i; >>> >>> for (i = 0; i < data->sg_len; i++) { >>> pdes[i].config = cpu_to_le32(SDXC_IDMAC_DES0_CH | >>> SDXC_IDMAC_DES0_OWN | >>> SDXC_IDMAC_DES0_DIC); >>> >>> - if (data->sg[i].length == max_len) >>> - pdes[i].buf_size = 0; /* 0 == max_len */ >>> - else >>> - pdes[i].buf_size = cpu_to_le32(data->sg[i].length); >>> + pdes[i].buf_size = cpu_to_le32(ALIGN(data->sg[i].length, >>> + SDXC_IDMAC_SIZE_ALIGN)); >>> >>> next_desc += sizeof(struct sunxi_idma_des); >>> pdes[i].buf_addr_ptr1 = >>> @@ -1420,7 +1421,8 @@ static int sunxi_mmc_probe(struct platform_device *pdev) >>> mmc->max_blk_count = 8192; >>> mmc->max_blk_size = 4096; >>> mmc->max_segs = PAGE_SIZE / sizeof(struct sunxi_idma_des); >>> - mmc->max_seg_size = (1 << host->cfg->idma_des_size_bits); >>> + mmc->max_seg_size = (1 << host->cfg->idma_des_size_bits) - >>> + SDXC_IDMAC_SIZE_ALIGN; >>> mmc->max_req_size = mmc->max_seg_size * mmc->max_segs; >>> /* 400kHz ~ 52MHz */ >>> mmc->f_min = 400000; >> >
diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c index c62afd212692..4bd5f37b1036 100644 --- a/drivers/mmc/host/sunxi-mmc.c +++ b/drivers/mmc/host/sunxi-mmc.c @@ -214,6 +214,9 @@ #define SDXC_IDMAC_DES0_CES BIT(30) /* card error summary */ #define SDXC_IDMAC_DES0_OWN BIT(31) /* 1-idma owns it, 0-host owns it */ +/* Buffer size must be a multiple of 4 bytes. */ +#define SDXC_IDMAC_SIZE_ALIGN 4 + #define SDXC_CLK_400K 0 #define SDXC_CLK_25M 1 #define SDXC_CLK_50M 2 @@ -361,17 +364,15 @@ static void sunxi_mmc_init_idma_des(struct sunxi_mmc_host *host, { struct sunxi_idma_des *pdes = (struct sunxi_idma_des *)host->sg_cpu; dma_addr_t next_desc = host->sg_dma; - int i, max_len = (1 << host->cfg->idma_des_size_bits); + int i; for (i = 0; i < data->sg_len; i++) { pdes[i].config = cpu_to_le32(SDXC_IDMAC_DES0_CH | SDXC_IDMAC_DES0_OWN | SDXC_IDMAC_DES0_DIC); - if (data->sg[i].length == max_len) - pdes[i].buf_size = 0; /* 0 == max_len */ - else - pdes[i].buf_size = cpu_to_le32(data->sg[i].length); + pdes[i].buf_size = cpu_to_le32(ALIGN(data->sg[i].length, + SDXC_IDMAC_SIZE_ALIGN)); next_desc += sizeof(struct sunxi_idma_des); pdes[i].buf_addr_ptr1 = @@ -1420,7 +1421,8 @@ static int sunxi_mmc_probe(struct platform_device *pdev) mmc->max_blk_count = 8192; mmc->max_blk_size = 4096; mmc->max_segs = PAGE_SIZE / sizeof(struct sunxi_idma_des); - mmc->max_seg_size = (1 << host->cfg->idma_des_size_bits); + mmc->max_seg_size = (1 << host->cfg->idma_des_size_bits) - + SDXC_IDMAC_SIZE_ALIGN; mmc->max_req_size = mmc->max_seg_size * mmc->max_segs; /* 400kHz ~ 52MHz */ mmc->f_min = 400000;
According to the DMA descriptor documentation, the lowest two bits of the size field are ignored, so the size must be rounded up to a multiple of 4 bytes. Furthermore, 0 is not a valid buffer size; setting the size to 0 will cause that DMA descriptor to be ignored. Together, these restrictions limit the maximum DMA segment size to 4 less than the power-of-two width of the size field. Fixes: 3cbcb16095f9 ("mmc: sunxi: Add driver for SD/MMC hosts found on Allwinner sunxi SoCs") Signed-off-by: Samuel Holland <samuel@sholland.org> --- drivers/mmc/host/sunxi-mmc.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)