[U-Boot] [PATCH 5/9] mmc: dw_mmc: add support for 64bit DMA

[Lukasz Majewski <lukma@denx.de>]

On Wed, 21 Nov 2018 09:52:08 +0100
Lukasz Majewski <lukma@denx.de> wrote:

> On Wed, 07 Nov 2018 16:03:08 +0100
> Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> 
> > From: Lukasz Majewski <l.majewski@samsung.com>
> > 
> > DW-MMC module in Samsung Exynos5433 requires 64bit DMA descriptors.
> > This patch adds code for handling them.
> > 
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > [extracted from old sources and adapted to mainline u-boot, minor
> > fixes] Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > ---
> >  drivers/mmc/dw_mmc.c        | 53
> > ++++++++++++++++++++++++++++++------- drivers/mmc/exynos_dw_mmc.c |
> > 6 +++++ include/dwmmc.h             | 25 +++++++++++++++++
> >  3 files changed, 74 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
> > index 3c702b3ed8..f50eb29ac9 100644
> > --- a/drivers/mmc/dw_mmc.c
> > +++ b/drivers/mmc/dw_mmc.c
> > @@ -30,8 +30,8 @@ static int dwmci_wait_reset(struct dwmci_host
> > *host, u32 value) return 0;
> >  }
> >  
> > -static void dwmci_set_idma_desc(struct dwmci_idmac *idmac,
> > -		u32 desc0, u32 desc1, u32 desc2)
> > +static void dwmci_set_idma_desc_32bit(void *idmac,
> > +				      u32 desc0, u32 desc1, u32
> > desc2) {
> >  	struct dwmci_idmac *desc = idmac;
> >  
> > @@ -41,11 +41,27 @@ static void dwmci_set_idma_desc(struct
> > dwmci_idmac *idmac, desc->next_addr = (ulong)desc + sizeof(struct
> > dwmci_idmac); }
> >  
> > +static void dwmci_set_idma_desc_64bit(void *idmac,
> > +				      u32 desc0, u32 desc1, u32
> > desc2) +{
> > +	struct dwmci_idmac_64addr *desc = idmac;
> > +
> > +	desc->flags = desc0;
> > +	desc->_res1 = 0;
> > +	desc->cnt = desc1;
> > +	desc->_res2 = 0;
> > +	desc->addrl = desc2;
> > +	desc->addrh = 0;
> > +	desc->next_addrl = (ulong)desc + sizeof(struct
> > dwmci_idmac_64addr);
> > +	desc->next_addrh = 0;
> > +}
> > +
> >  static void dwmci_prepare_data(struct dwmci_host *host,
> >  			       struct mmc_data *data,
> > -			       struct dwmci_idmac *cur_idmac,
> > +			       struct dwmci_idmac_64addr
> > *cur_idmac64, void *bounce_buffer)  
> 
> This looks strange - why the core dwmci_prepare_data() has this
> change?
> 
> Above, the *idmac has been changed to void* from struct dwmci_idmac
> *idmac to handle 64bit DMA descriptors.
> 
> I think that it shall be done in the same way here.
> 
> >  {
> > +	struct dwmci_idmac *cur_idmac = (struct dwmci_idmac
> > *)cur_idmac64; unsigned long ctrl;
> >  	unsigned int i = 0, flags, cnt, blk_cnt;
> >  	ulong data_start, data_end;
> > @@ -56,7 +72,12 @@ static void dwmci_prepare_data(struct dwmci_host
> > *host, dwmci_wait_reset(host, DWMCI_CTRL_FIFO_RESET);
> >  
> >  	data_start = (ulong)cur_idmac;
> > -	dwmci_writel(host, DWMCI_DBADDR, (ulong)cur_idmac);
> > +
> > +	if (host->dma_64bit_address) {
> > +		dwmci_writel(host, DWMCI_DBADDRU, 0);
> > +		dwmci_writel(host, DWMCI_DBADDRL,
> > (ulong)cur_idmac64);  
> 
> This is even more strange - the upper part of 32 bit address is 0, so
> we pass only 32 bit address. Is this SoC working in armv7 mode (32
> bit) and the dwmmc is expecting 64bit descriptors?
> 
> 
> > +	} else
> > +		dwmci_writel(host, DWMCI_DBADDR, (ulong)cur_idmac);
> >  
> >  	do {
> >  		flags = DWMCI_IDMAC_OWN | DWMCI_IDMAC_CH ;
> > @@ -67,17 +88,27 @@ static void dwmci_prepare_data(struct dwmci_host
> > *host, } else
> >  			cnt = data->blocksize * 8;
> >  
> > -		dwmci_set_idma_desc(cur_idmac, flags, cnt,
> > -				    (ulong)bounce_buffer + (i *
> > PAGE_SIZE));
> > +		if (host->dma_64bit_address)
> > +			dwmci_set_idma_desc_64bit(cur_idmac64,  
> 						  ^^^^^^ here a void
> 						  pointer with a
> static cast would be enough.
> 
> > flags, cnt,
> > +
> > (ulong)bounce_buffer +
> > +						  (i * PAGE_SIZE));
> > +		else
> > +			dwmci_set_idma_desc_32bit(cur_idmac, flags,
> > cnt,
> > +
> > (ulong)bounce_buffer +
> > +						  (i * PAGE_SIZE));
> >  
> >  		if (blk_cnt <= 8)
> >  			break;
> >  		blk_cnt -= 8;
> >  		cur_idmac++;
> > +		cur_idmac64++;  
> 
> This looks like a quick hack - the if (host->dma_64bit_address) / else
> is missing. Otherwise it would break the boards which use this code
> already.
> 
> >  		i++;
> >  	} while(1);
> >  
> > -	data_end = (ulong)cur_idmac;
> > +	if (host->dma_64bit_address)
> > +		data_end = (ulong)cur_idmac64;
> > +	else
> > +		data_end = (ulong)cur_idmac;
> >  	flush_dcache_range(data_start, data_end +
> > ARCH_DMA_MINALIGN); 
> >  	ctrl = dwmci_readl(host, DWMCI_CTRL);
> > @@ -222,7 +253,7 @@ static int dwmci_send_cmd(struct mmc *mmc,
> > struct mmc_cmd *cmd, {
> >  #endif
> >  	struct dwmci_host *host = mmc->priv;
> > -	ALLOC_CACHE_ALIGN_BUFFER(struct dwmci_idmac, cur_idmac,
> > +	ALLOC_CACHE_ALIGN_BUFFER(struct dwmci_idmac_64addr,  
> 
> Isn't this change causing the boards with 32 bits addressing
> malfunctioning? 
> 
> #ifdef would be necessary - as struct dwmci_idmac / dwmci_idmac_64addr
> have different sizes and fields.
> 
> > cur_idmac64, data ? DIV_ROUND_UP(data->blocks, 8) : 0);
> >  	int ret = 0, flags = 0, i;
> >  	unsigned int timeout = 500;
> > @@ -256,7 +287,7 @@ static int dwmci_send_cmd(struct mmc *mmc,
> > struct mmc_cmd *cmd, data->blocksize *
> >  						data->blocks,
> > GEN_BB_READ); }
> > -			dwmci_prepare_data(host, data, cur_idmac,
> > +			dwmci_prepare_data(host, data, cur_idmac64,
> >  					   bbstate.bounce_buffer);  
> 
> The above comment also applies here.
> 
> >  		}
> >  	}
> > @@ -474,7 +505,9 @@ static int dwmci_init(struct mmc *mmc)
> >  
> >  	dwmci_writel(host, DWMCI_TMOUT, 0xFFFFFFFF);
> >  
> > -	dwmci_writel(host, DWMCI_IDINTEN, 0);
> > +	dwmci_writel(host, host->dma_64bit_address ?
> > +			   DWMCI_IDINTEN64 : DWMCI_IDINTEN, 0);
> > +
> >  	dwmci_writel(host, DWMCI_BMOD, 1);
> >  
> >  	if (!host->fifoth_val) {
> > diff --git a/drivers/mmc/exynos_dw_mmc.c
> > b/drivers/mmc/exynos_dw_mmc.c index 435ccac594..3e9d47538c 100644
> > --- a/drivers/mmc/exynos_dw_mmc.c
> > +++ b/drivers/mmc/exynos_dw_mmc.c
> > @@ -98,6 +98,7 @@ static void exynos_dwmci_board_init(struct
> > dwmci_host *host) 
> >  static int exynos_dwmci_core_init(struct dwmci_host *host)
> >  {
> > +	unsigned int addr_config;
> >  	unsigned int div;
> >  	unsigned long freq, sclk;
> >  
> > @@ -122,6 +123,11 @@ static int exynos_dwmci_core_init(struct
> > dwmci_host *host) host->clksel = exynos_dwmci_clksel;
> >  	host->get_mmc_clk = exynos_dwmci_get_clk;
> >  
> > +	addr_config = DWMCI_GET_ADDR_CONFIG(dwmci_readl(host,
> > DWMCI_HCON));
> > +	if (addr_config == 1)
> > +		/* host supports IDMAC in 64-bit address mode */
> > +		host->dma_64bit_address = 1;
> > +
> >  #ifndef CONFIG_DM_MMC
> >  	/* Add the mmc channel to be registered with mmc core */
> >  	if (add_dwmci(host, DWMMC_MAX_FREQ, DWMMC_MIN_FREQ)) {
> > diff --git a/include/dwmmc.h b/include/dwmmc.h
> > index 0f9d51b557..14db03d7d4 100644
> > --- a/include/dwmmc.h
> > +++ b/include/dwmmc.h
> > @@ -48,6 +48,17 @@
> >  #define DWMCI_DSCADDR		0x094
> >  #define DWMCI_BUFADDR		0x098
> >  #define DWMCI_DATA		0x200
> > +/*
> > + * Registers to support idmac 64-bit address mode
> > + */
> > +#define DWMCI_DBADDRL		0x088
> > +#define DWMCI_DBADDRU		0x08c
> > +#define DWMCI_IDSTS64		0x090
> > +#define DWMCI_IDINTEN64		0x094
> > +#define DWMCI_DSCADDRL		0x098
> > +#define DWMCI_DSCADDRU		0x09c
> > +#define DWMCI_BUFADDRL		0x0A0
> > +#define DWMCI_BUFADDRU		0x0A4
> >  
> >  /* Interrupt Mask register */
> >  #define DWMCI_INTMSK_ALL	0xffffffff
> > @@ -132,6 +143,7 @@
> >  /* quirks */
> >  #define DWMCI_QUIRK_DISABLE_SMU		(1 << 0)
> >  
> > +#define DWMCI_GET_ADDR_CONFIG(x) (((x)>>27) & 0x1)
> >  /**
> >   * struct dwmci_host - Information about a designware MMC host
> >   *
> > @@ -145,6 +157,7 @@
> >   * @dev_id:	Arbitrary device ID for use by controller
> >   * @buswidth:	Bus width in bits (8 or 4)
> >   * @fifoth_val:	Value for FIFOTH register (or 0 to leave
> > unset)
> > + * @dma_64bit_address:	True only for devices supporting 64
> > bit DMA
> >   * @mmc:	Pointer to generic MMC structure for this device
> >   * @priv:	Private pointer for use by controller
> >   */
> > @@ -161,6 +174,7 @@ struct dwmci_host {
> >  	int dev_id;
> >  	int buswidth;
> >  	u32 fifoth_val;
> > +	int dma_64bit_address;
> >  	struct mmc *mmc;
> >  	void *priv;
> >  
> > @@ -196,6 +210,17 @@ struct dwmci_idmac {
> >  	u32 next_addr;
> >  } __aligned(ARCH_DMA_MINALIGN);
> >  
> > +struct dwmci_idmac_64addr {
> > +	u32 flags;
> > +	u32 _res1;
> > +	u32 cnt;
> > +	u32 _res2;
> > +	u32 addrl;
> > +	u32 addrh;
> > +	u32 next_addrl;
> > +	u32 next_addrh;
> > +} __aligned(ARCH_DMA_MINALIGN);
> > +
> >  static inline void dwmci_writel(struct dwmci_host *host, int reg,
> > u32 val) {
> >  	writel(val, host->ioaddr + reg);  
> 
> 

And just to mention - for such change I would like to have a tested-by:
tag on the SoC (Odroid XU3/4), which uses the 32 bit DMA descriptors.

> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> lukma at denx.de




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181121/c0679926/attachment.sig>