| Message ID | 20241107014240.24669-1-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: sunxi-mmc: Fix A100 compatible description | expand |
On 11/7/24 7:12 AM, Andre Przywara wrote: > It turns out that the Allwinner A100/A133 SoC only supports 8K DMA > blocks (13 bits wide), for both the SD/SDIO and eMMC instances. > And while this alone would make a trivial fix, the H616 falls back to > the A100 compatible string, so we have to now match the H616 compatible > string explicitly against the description advertising 64K DMA blocks. > > As the A100 is now compatible with the D1 description, let the A100 > compatible string point to that block instead, and introduce an explicit > match against the H616 string, pointing to the old description. > Also remove the redundant setting of clk_delays to NULL on the way. > > Fixes: 3536b82e5853 ("mmc: sunxi: add support for A100 mmc controller") > Cc: stable@vger.kernel.org > Signed-off-by: Andre Przywara <andre.przywara@arm.com> Tested working with SD card in mmc0. Tested-by: Parthiban Nallathambi <parthiban@linumiz.com> Thanks, Parthiban > --- > drivers/mmc/host/sunxi-mmc.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c > index d3bd0ac99ec46..e0ab5fd635e6c 100644 > --- a/drivers/mmc/host/sunxi-mmc.c > +++ b/drivers/mmc/host/sunxi-mmc.c > @@ -1191,10 +1191,9 @@ static const struct sunxi_mmc_cfg sun50i_a64_emmc_cfg = { > .needs_new_timings = true, > }; > > -static const struct sunxi_mmc_cfg sun50i_a100_cfg = { > +static const struct sunxi_mmc_cfg sun50i_h616_cfg = { > .idma_des_size_bits = 16, > .idma_des_shift = 2, > - .clk_delays = NULL, > .can_calibrate = true, > .mask_data0 = true, > .needs_new_timings = true, > @@ -1217,8 +1216,9 @@ static const struct of_device_id sunxi_mmc_of_match[] = { > { .compatible = "allwinner,sun20i-d1-mmc", .data = &sun20i_d1_cfg }, > { .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg }, > { .compatible = "allwinner,sun50i-a64-emmc", .data = &sun50i_a64_emmc_cfg }, > - { .compatible = "allwinner,sun50i-a100-mmc", .data = &sun50i_a100_cfg }, > + { .compatible = "allwinner,sun50i-a100-mmc", .data = &sun20i_d1_cfg }, > { .compatible = "allwinner,sun50i-a100-emmc", .data = &sun50i_a100_emmc_cfg }, > + { .compatible = "allwinner,sun50i-h616-mmc", .data = &sun50i_h616_cfg }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
On Thu, Nov 7, 2024 at 9:43 AM Andre Przywara <andre.przywara@arm.com> wrote: > > It turns out that the Allwinner A100/A133 SoC only supports 8K DMA > blocks (13 bits wide), for both the SD/SDIO and eMMC instances. > And while this alone would make a trivial fix, the H616 falls back to > the A100 compatible string, so we have to now match the H616 compatible > string explicitly against the description advertising 64K DMA blocks. Would be nice to know how this was discovered, and how the correct size was determined. As far as I could find, the A133 user manual says its 64K. ChenYu > As the A100 is now compatible with the D1 description, let the A100 > compatible string point to that block instead, and introduce an explicit > match against the H616 string, pointing to the old description. > Also remove the redundant setting of clk_delays to NULL on the way. > > Fixes: 3536b82e5853 ("mmc: sunxi: add support for A100 mmc controller") > Cc: stable@vger.kernel.org > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > drivers/mmc/host/sunxi-mmc.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c > index d3bd0ac99ec46..e0ab5fd635e6c 100644 > --- a/drivers/mmc/host/sunxi-mmc.c > +++ b/drivers/mmc/host/sunxi-mmc.c > @@ -1191,10 +1191,9 @@ static const struct sunxi_mmc_cfg sun50i_a64_emmc_cfg = { > .needs_new_timings = true, > }; > > -static const struct sunxi_mmc_cfg sun50i_a100_cfg = { > +static const struct sunxi_mmc_cfg sun50i_h616_cfg = { > .idma_des_size_bits = 16, > .idma_des_shift = 2, > - .clk_delays = NULL, > .can_calibrate = true, > .mask_data0 = true, > .needs_new_timings = true, > @@ -1217,8 +1216,9 @@ static const struct of_device_id sunxi_mmc_of_match[] = { > { .compatible = "allwinner,sun20i-d1-mmc", .data = &sun20i_d1_cfg }, > { .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg }, > { .compatible = "allwinner,sun50i-a64-emmc", .data = &sun50i_a64_emmc_cfg }, > - { .compatible = "allwinner,sun50i-a100-mmc", .data = &sun50i_a100_cfg }, > + { .compatible = "allwinner,sun50i-a100-mmc", .data = &sun20i_d1_cfg }, > { .compatible = "allwinner,sun50i-a100-emmc", .data = &sun50i_a100_emmc_cfg }, > + { .compatible = "allwinner,sun50i-h616-mmc", .data = &sun50i_h616_cfg }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match); > -- > 2.46.2 >
On Sun, 10 Nov 2024 17:04:08 +0800 Chen-Yu Tsai <wens@csie.org> wrote: Hi, > On Thu, Nov 7, 2024 at 9:43 AM Andre Przywara <andre.przywara@arm.com> wrote: > > > > It turns out that the Allwinner A100/A133 SoC only supports 8K DMA > > blocks (13 bits wide), for both the SD/SDIO and eMMC instances. > > And while this alone would make a trivial fix, the H616 falls back to > > the A100 compatible string, so we have to now match the H616 compatible > > string explicitly against the description advertising 64K DMA blocks. > > Would be nice to know how this was discovered, and how the correct size > was determined. As far as I could find, the A133 user manual says its > 64K. Mmh, my copy (Revision 1.1, Jul.14, 2020) only mentions bits[12:0] in the DES1 DMA descriptor details, unconditional of SMHC0/1/2. And yes, this is in contradiction to the prose section in "5.3.1. Overview", which mentions a "Block size of 1 to 65535 bytes". Also that matches the observation: eMMC was working fine (as it was already limited to 8K), and the SD card was *somewhat* working: I could mount a FAT filesystem, and even list the (rather short) root directory, but any further action (reading file, benchmarking) would hang. Which would make sense given that the first actions probably don't ask for a block larger than 8K. Cheers, Andre > > ChenYu > > > As the A100 is now compatible with the D1 description, let the A100 > > compatible string point to that block instead, and introduce an explicit > > match against the H616 string, pointing to the old description. > > Also remove the redundant setting of clk_delays to NULL on the way. > > > > Fixes: 3536b82e5853 ("mmc: sunxi: add support for A100 mmc controller") > > Cc: stable@vger.kernel.org > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > --- > > drivers/mmc/host/sunxi-mmc.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c > > index d3bd0ac99ec46..e0ab5fd635e6c 100644 > > --- a/drivers/mmc/host/sunxi-mmc.c > > +++ b/drivers/mmc/host/sunxi-mmc.c > > @@ -1191,10 +1191,9 @@ static const struct sunxi_mmc_cfg sun50i_a64_emmc_cfg = { > > .needs_new_timings = true, > > }; > > > > -static const struct sunxi_mmc_cfg sun50i_a100_cfg = { > > +static const struct sunxi_mmc_cfg sun50i_h616_cfg = { > > .idma_des_size_bits = 16, > > .idma_des_shift = 2, > > - .clk_delays = NULL, > > .can_calibrate = true, > > .mask_data0 = true, > > .needs_new_timings = true, > > @@ -1217,8 +1216,9 @@ static const struct of_device_id sunxi_mmc_of_match[] = { > > { .compatible = "allwinner,sun20i-d1-mmc", .data = &sun20i_d1_cfg }, > > { .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg }, > > { .compatible = "allwinner,sun50i-a64-emmc", .data = &sun50i_a64_emmc_cfg }, > > - { .compatible = "allwinner,sun50i-a100-mmc", .data = &sun50i_a100_cfg }, > > + { .compatible = "allwinner,sun50i-a100-mmc", .data = &sun20i_d1_cfg }, > > { .compatible = "allwinner,sun50i-a100-emmc", .data = &sun50i_a100_emmc_cfg }, > > + { .compatible = "allwinner,sun50i-h616-mmc", .data = &sun50i_h616_cfg }, > > { /* sentinel */ } > > }; > > MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match); > > -- > > 2.46.2 > > >
On Sun, Nov 10, 2024 at 6:22 PM Andre Przywara <andre.przywara@arm.com> wrote: > > On Sun, 10 Nov 2024 17:04:08 +0800 > Chen-Yu Tsai <wens@csie.org> wrote: > > Hi, > > > On Thu, Nov 7, 2024 at 9:43 AM Andre Przywara <andre.przywara@arm.com> wrote: > > > > > > It turns out that the Allwinner A100/A133 SoC only supports 8K DMA > > > blocks (13 bits wide), for both the SD/SDIO and eMMC instances. > > > And while this alone would make a trivial fix, the H616 falls back to > > > the A100 compatible string, so we have to now match the H616 compatible > > > string explicitly against the description advertising 64K DMA blocks. > > > > Would be nice to know how this was discovered, and how the correct size > > was determined. As far as I could find, the A133 user manual says its > > 64K. > > Mmh, my copy (Revision 1.1, Jul.14, 2020) only mentions bits[12:0] in > the DES1 DMA descriptor details, unconditional of SMHC0/1/2. And yes, I see. I was looking at SMHC_BLKSIZ, which had 16 bits. > this is in contradiction to the prose section in "5.3.1. Overview", > which mentions a "Block size of 1 to 65535 bytes". > Also that matches the observation: eMMC was working fine (as it was > already limited to 8K), and the SD card was *somewhat* working: I could > mount a FAT filesystem, and even list the (rather short) root > directory, but any further action (reading file, benchmarking) would > hang. Which would make sense given that the first actions probably > don't ask for a block larger than 8K. Thanks for explaining it. Reviewed-by: Chen-Yu Tsai <wens@csie.org> > Cheers, > Andre > > > > > > > ChenYu > > > > > As the A100 is now compatible with the D1 description, let the A100 > > > compatible string point to that block instead, and introduce an explicit > > > match against the H616 string, pointing to the old description. > > > Also remove the redundant setting of clk_delays to NULL on the way. > > > > > > Fixes: 3536b82e5853 ("mmc: sunxi: add support for A100 mmc controller") > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > --- > > > drivers/mmc/host/sunxi-mmc.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c > > > index d3bd0ac99ec46..e0ab5fd635e6c 100644 > > > --- a/drivers/mmc/host/sunxi-mmc.c > > > +++ b/drivers/mmc/host/sunxi-mmc.c > > > @@ -1191,10 +1191,9 @@ static const struct sunxi_mmc_cfg sun50i_a64_emmc_cfg = { > > > .needs_new_timings = true, > > > }; > > > > > > -static const struct sunxi_mmc_cfg sun50i_a100_cfg = { > > > +static const struct sunxi_mmc_cfg sun50i_h616_cfg = { > > > .idma_des_size_bits = 16, > > > .idma_des_shift = 2, > > > - .clk_delays = NULL, > > > .can_calibrate = true, > > > .mask_data0 = true, > > > .needs_new_timings = true, > > > @@ -1217,8 +1216,9 @@ static const struct of_device_id sunxi_mmc_of_match[] = { > > > { .compatible = "allwinner,sun20i-d1-mmc", .data = &sun20i_d1_cfg }, > > > { .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg }, > > > { .compatible = "allwinner,sun50i-a64-emmc", .data = &sun50i_a64_emmc_cfg }, > > > - { .compatible = "allwinner,sun50i-a100-mmc", .data = &sun50i_a100_cfg }, > > > + { .compatible = "allwinner,sun50i-a100-mmc", .data = &sun20i_d1_cfg }, > > > { .compatible = "allwinner,sun50i-a100-emmc", .data = &sun50i_a100_emmc_cfg }, > > > + { .compatible = "allwinner,sun50i-h616-mmc", .data = &sun50i_h616_cfg }, > > > { /* sentinel */ } > > > }; > > > MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match); > > > -- > > > 2.46.2 > > > > > >
On Thu, 7 Nov 2024 at 02:43, Andre Przywara <andre.przywara@arm.com> wrote: > > It turns out that the Allwinner A100/A133 SoC only supports 8K DMA > blocks (13 bits wide), for both the SD/SDIO and eMMC instances. > And while this alone would make a trivial fix, the H616 falls back to > the A100 compatible string, so we have to now match the H616 compatible > string explicitly against the description advertising 64K DMA blocks. > > As the A100 is now compatible with the D1 description, let the A100 > compatible string point to that block instead, and introduce an explicit > match against the H616 string, pointing to the old description. > Also remove the redundant setting of clk_delays to NULL on the way. > > Fixes: 3536b82e5853 ("mmc: sunxi: add support for A100 mmc controller") > Cc: stable@vger.kernel.org > Signed-off-by: Andre Przywara <andre.przywara@arm.com> Applied for fixes, thanks! Kind regards Uffe > --- > drivers/mmc/host/sunxi-mmc.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c > index d3bd0ac99ec46..e0ab5fd635e6c 100644 > --- a/drivers/mmc/host/sunxi-mmc.c > +++ b/drivers/mmc/host/sunxi-mmc.c > @@ -1191,10 +1191,9 @@ static const struct sunxi_mmc_cfg sun50i_a64_emmc_cfg = { > .needs_new_timings = true, > }; > > -static const struct sunxi_mmc_cfg sun50i_a100_cfg = { > +static const struct sunxi_mmc_cfg sun50i_h616_cfg = { > .idma_des_size_bits = 16, > .idma_des_shift = 2, > - .clk_delays = NULL, > .can_calibrate = true, > .mask_data0 = true, > .needs_new_timings = true, > @@ -1217,8 +1216,9 @@ static const struct of_device_id sunxi_mmc_of_match[] = { > { .compatible = "allwinner,sun20i-d1-mmc", .data = &sun20i_d1_cfg }, > { .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg }, > { .compatible = "allwinner,sun50i-a64-emmc", .data = &sun50i_a64_emmc_cfg }, > - { .compatible = "allwinner,sun50i-a100-mmc", .data = &sun50i_a100_cfg }, > + { .compatible = "allwinner,sun50i-a100-mmc", .data = &sun20i_d1_cfg }, > { .compatible = "allwinner,sun50i-a100-emmc", .data = &sun50i_a100_emmc_cfg }, > + { .compatible = "allwinner,sun50i-h616-mmc", .data = &sun50i_h616_cfg }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match); > -- > 2.46.2 >
diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c index d3bd0ac99ec46..e0ab5fd635e6c 100644 --- a/drivers/mmc/host/sunxi-mmc.c +++ b/drivers/mmc/host/sunxi-mmc.c @@ -1191,10 +1191,9 @@ static const struct sunxi_mmc_cfg sun50i_a64_emmc_cfg = { .needs_new_timings = true, }; -static const struct sunxi_mmc_cfg sun50i_a100_cfg = { +static const struct sunxi_mmc_cfg sun50i_h616_cfg = { .idma_des_size_bits = 16, .idma_des_shift = 2, - .clk_delays = NULL, .can_calibrate = true, .mask_data0 = true, .needs_new_timings = true, @@ -1217,8 +1216,9 @@ static const struct of_device_id sunxi_mmc_of_match[] = { { .compatible = "allwinner,sun20i-d1-mmc", .data = &sun20i_d1_cfg }, { .compatible = "allwinner,sun50i-a64-mmc", .data = &sun50i_a64_cfg }, { .compatible = "allwinner,sun50i-a64-emmc", .data = &sun50i_a64_emmc_cfg }, - { .compatible = "allwinner,sun50i-a100-mmc", .data = &sun50i_a100_cfg }, + { .compatible = "allwinner,sun50i-a100-mmc", .data = &sun20i_d1_cfg }, { .compatible = "allwinner,sun50i-a100-emmc", .data = &sun50i_a100_emmc_cfg }, + { .compatible = "allwinner,sun50i-h616-mmc", .data = &sun50i_h616_cfg }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
It turns out that the Allwinner A100/A133 SoC only supports 8K DMA blocks (13 bits wide), for both the SD/SDIO and eMMC instances. And while this alone would make a trivial fix, the H616 falls back to the A100 compatible string, so we have to now match the H616 compatible string explicitly against the description advertising 64K DMA blocks. As the A100 is now compatible with the D1 description, let the A100 compatible string point to that block instead, and introduce an explicit match against the H616 string, pointing to the old description. Also remove the redundant setting of clk_delays to NULL on the way. Fixes: 3536b82e5853 ("mmc: sunxi: add support for A100 mmc controller") Cc: stable@vger.kernel.org Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- drivers/mmc/host/sunxi-mmc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)