Re: [RESEND PATCH 17/19] mmc: sunxi: add support for A100 mmc controller

["André Przywara" <andre.przywara@arm.com>]

On 28/11/2020 19:56, André Przywara wrote:
> On 10/11/2020 06:46, Frank Lee wrote:

Hi,

one more thing below ...

>> From: Yangtao Li <frank@allwinnertech.com>
>>
>> This patch adds support for A100 MMC controller, which use word address
>> for internal dma.
>>
>> Signed-off-by: Yangtao Li <frank@allwinnertech.com>
>> ---
>>  drivers/mmc/host/sunxi-mmc.c | 28 +++++++++++++++++++++++++---
>>  1 file changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
>> index fc62773602ec..1518b64112b7 100644
>> --- a/drivers/mmc/host/sunxi-mmc.c
>> +++ b/drivers/mmc/host/sunxi-mmc.c
>> @@ -244,6 +244,7 @@ struct sunxi_idma_des {
>>  
>>  struct sunxi_mmc_cfg {
>>  	u32 idma_des_size_bits;
>> +	u32 idma_des_shift;
>>  	const struct sunxi_mmc_clk_delay *clk_delays;
>>  
>>  	/* does the IP block support autocalibration? */
>> @@ -343,7 +344,7 @@ static int sunxi_mmc_init_host(struct sunxi_mmc_host *host)
>>  	/* Enable CEATA support */
>>  	mmc_writel(host, REG_FUNS, SDXC_CEATA_ON);
>>  	/* Set DMA descriptor list base address */
>> -	mmc_writel(host, REG_DLBA, host->sg_dma);
>> +	mmc_writel(host, REG_DLBA, host->sg_dma >> host->cfg->idma_des_shift);
>>  
>>  	rval = mmc_readl(host, REG_GCTRL);
>>  	rval |= SDXC_INTERRUPT_ENABLE_BIT;
>> @@ -373,8 +374,10 @@ static void sunxi_mmc_init_idma_des(struct sunxi_mmc_host *host,
>>  
>>  		next_desc += sizeof(struct sunxi_idma_des);
>>  		pdes[i].buf_addr_ptr1 =
>> -			cpu_to_le32(sg_dma_address(&data->sg[i]));
>> -		pdes[i].buf_addr_ptr2 = cpu_to_le32((u32)next_desc);
>> +			cpu_to_le32(sg_dma_address(&data->sg[i]) >>
>> +				    host->cfg->idma_des_shift);
>> +		pdes[i].buf_addr_ptr2 = cpu_to_le32((u32)next_desc >>
>> +						    host->cfg->idma_des_shift);
> 
> I think you should cast after the shift, otherwise you lose the ability
> to run above 4 GB. This won't be a problem at the moment, since we still
> use the default 32-bit DMA mask, but might bite us later.
> 
> Otherwise this patch looks fine, and works on the H616 as well.
> 
> Cheers,
> Andre
> 
>>  	}
>>  
>>  	pdes[0].config |= cpu_to_le32(SDXC_IDMAC_DES0_FD);
>> @@ -1178,6 +1181,23 @@ static const struct sunxi_mmc_cfg sun50i_a64_emmc_cfg = {
>>  	.needs_new_timings = true,
>>  };
>>  
>> +static const struct sunxi_mmc_cfg sun50i_a100_cfg = {
>> +	.idma_des_size_bits = 16,
>> +	.idma_des_shift = 2,
>> +	.clk_delays = NULL,
>> +	.can_calibrate = true,
>> +	.mask_data0 = true,
>> +	.needs_new_timings = true,
>> +};
>> +
>> +static const struct sunxi_mmc_cfg sun50i_a100_emmc_cfg = {
>> +	.idma_des_size_bits = 13,
>> +	.idma_des_shift = 2,

Is that actually true? Don't know about the A100, but the H616 manual
mentions that "SMHC2" deals with byte addresses, in contrast to the
other two ones. So MMC2 would be compatible with the a64_emmc_cfg?

Cheers,
Andre

>> +	.clk_delays = NULL,
>> +	.can_calibrate = true,
>> +	.needs_new_timings = true,
>> +};
>> +
>>  static const struct of_device_id sunxi_mmc_of_match[] = {
>>  	{ .compatible = "allwinner,sun4i-a10-mmc", .data = &sun4i_a10_cfg },
>>  	{ .compatible = "allwinner,sun5i-a13-mmc", .data = &sun5i_a13_cfg },
>> @@ -1186,6 +1206,8 @@ static const struct of_device_id sunxi_mmc_of_match[] = {
>>  	{ .compatible = "allwinner,sun9i-a80-mmc", .data = &sun9i_a80_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-emmc", .data = &sun50i_a100_emmc_cfg },
>>  	{ /* sentinel */ }
>>  };
>>  MODULE_DEVICE_TABLE(of, sunxi_mmc_of_match);
>>
>