Re: [PATCH] mmc: sdhci: introduce adma_write_desc() hook to struct sdhci_ops
Hi,
> Some target are failed to build. (e.g, j721e_beagleboneai64_r5)
>
> +drivers/mmc/sdhci-adma.c: In function '__sdhci_adma_write_desc':
> +drivers/mmc/sdhci-adma.c:37:43: error: 'const struct sdhci_ops' has no
> member named 'adma_write_desc'
> + 37 | if (host && host->ops && host->ops->adma_write_desc)
> + | ^~
> +drivers/mmc/sdhci-adma.c:38:26: error: 'const struct sdhci_ops' has no
> member named 'adma_write_desc'
> + 38 | host->ops->adma_write_desc(host, desc, addr, len,
> end);
> + | ^~
> +make[3]: *** [scripts/Makefile.build:257: drivers/mmc/sdhci-adma.o] Error 1
> +make[2]: *** [scripts/Makefile.build:397: drivers/mmc] Error 2
I will test v2 with CI before resubmitting so that this issue is
fixed. It is caused by the change and explanation below:
> > > diff --git a/include/sdhci.h b/include/sdhci.h
> > > index a1b74e3bd7..4bde7db5c7 100644
> > > --- a/include/sdhci.h
> > > +++ b/include/sdhci.h
> > > @@ -291,6 +291,11 @@ struct sdhci_ops {
> > > * Return: 0 if successful, -ve on error
> > > */
> > > int (*set_enhanced_strobe)(struct sdhci_host *host);
> > > +
> > > +#if CONFIG_IS_ENABLED(MMC_SDHCI_ADMA)
> > > + void(*adma_write_desc)(struct sdhci_host *host, void **desc,
> > > + dma_addr_t addr, int len, bool end);
> > > +#endif
> > > };
We got a little too excited about following checkpatch's
recommendations (no #ifdef CONFIG_xyz, prefer #if
CONFIG_IS_ENABLED(xyz)), which breaks down in this case:
CONFIG_IS_ENABLED(xyz) checks if:
- regular build and CONFIG_xyz is enabled (this portion succeeds)
- SPL build and CONFIG_SPL_xyz is enabled (this portion fails)
drivers/mmc/Makefile builds sdhci-adma.o based on
CONFIG_SDHCI_ADMA_HELPERS only.
There is no CONFIG_SPL_SDHCI_ADMA_HELPERS so CONFIG_IS_ENABLED fails
while building the SPL version of sdhci-adma.o as the structure
definition is different. This only appears on platforms which have
CONFIG_SPL_MMC enabled, which our platform did not, so I missed this
interaction earlier. I apologize for this mistake.
This will be fixed in v2 by changing the #if back to #ifdef
CONFIG_MMC_SDHCI_ADMA_HELPERS, which I will submit after CI finishes
running to verify on all platforms.
Thanks,
Greg
--
Greg Malysa
Timesys Corporation
RE: [PATCH] mmc: sdhci: introduce adma_write_desc() hook to struct sdhci_ops
Hi,
> -Original Message-
> From: U-Boot On Behalf Of Jaehoon Chung
> Sent: Wednesday, April 17, 2024 9:26 AM
> To: 'Greg Malysa' ; u-boot@lists.denx.de; 'Peng Fan'
>
> Cc: 'Ian Roberts' ; 'Nathan Barrett-Morrison'
> ;
> 'Jonas Karlman' ; 'Kever Yang' ;
> 'Peter Geis'
> ; 'Sean Anderson' ; 'Simon
> Glass' ;
> 'Tom Rini'
> Subject: RE: [PATCH] mmc: sdhci: introduce adma_write_desc() hook to struct
> sdhci_ops
>
>
>
> > -Original Message-
> > From: Greg Malysa
> > Sent: Tuesday, March 26, 2024 11:18 AM
> > To: u-boot@lists.denx.de; Peng Fan ; Jaehoon Chung
> >
> > Cc: Ian Roberts ; Nathan Barrett-Morrison
> > ;
> Greg
> > Malysa ; Jonas Karlman ; Kever
> > Yang > chips.com>; Peter Geis ; Sean Anderson
> > ; Simon Glass
> > ; Tom Rini
> > Subject: [PATCH] mmc: sdhci: introduce adma_write_desc() hook to struct
> > sdhci_ops
> >
> > From: Ian Roberts
> >
> > Add this hook so that it can be overridden with driver specific
> > implementations. We also let the original sdhci_adma_write_desc()
> > accept so that the function can set its new value. Then export
> > the function so that it could be reused by driver's specific
> > implementations.
> >
> > The above is a port of Linux kernel commit 54552e4948cbf
> >
> > In addition, allow drivers to allocate their own ADMA descriptor
> > tables if additional space is required.
> >
> > Finally, fix the assignment of adma_addr to fix compiler warning
> > on 64-bit platforms that still use 32-bit DMA addressing.
> >
> > Co-developed-by: Nathan Barrett-Morrison
> > Signed-off-by: Nathan Barrett-Morrison
> > Signed-off-by: Greg Malysa
> > Signed-off-by: Ian Roberts
>
> Reviewed-by: Jaehoon Chung
Some target are failed to build. (e.g, j721e_beagleboneai64_r5)
+drivers/mmc/sdhci-adma.c: In function '__sdhci_adma_write_desc':
+drivers/mmc/sdhci-adma.c:37:43: error: 'const struct sdhci_ops' has no member
named 'adma_write_desc'
+ 37 | if (host && host->ops && host->ops->adma_write_desc)
+ | ^~
+drivers/mmc/sdhci-adma.c:38:26: error: 'const struct sdhci_ops' has no member
named 'adma_write_desc'
+ 38 | host->ops->adma_write_desc(host, desc, addr, len, end);
+ | ^~
+make[3]: *** [scripts/Makefile.build:257: drivers/mmc/sdhci-adma.o] Error 1
+make[2]: *** [scripts/Makefile.build:397: drivers/mmc] Error 2
Best Regards,
Jaehoon Chung
>
> Best Regards,
> Jaehoon Chung
>
> >
> > ---
> >
> >
> > ---
> > drivers/mmc/fsl_esdhc.c | 2 +-
> > drivers/mmc/sdhci-adma.c | 41 +++-
> > drivers/mmc/sdhci.c | 8 +---
> > include/sdhci.h | 12 ++--
> > 4 files changed, 44 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> > index d50669..bd0671cc52 100644
> > --- a/drivers/mmc/fsl_esdhc.c
> > +++ b/drivers/mmc/fsl_esdhc.c
> > @@ -252,7 +252,7 @@ static void esdhc_setup_dma(struct fsl_esdhc_priv
> > *priv, struct mmc_data *data)
> > priv->adma_desc_table) {
> > debug("Using ADMA2\n");
> > /* prefer ADMA2 if it is available */
> > - sdhci_prepare_adma_table(priv->adma_desc_table, data,
> > + sdhci_prepare_adma_table(NULL, priv->adma_desc_table, data,
> > priv->dma_addr);
> >
> > adma_addr = virt_to_phys(priv->adma_desc_table);
> > diff --git a/drivers/mmc/sdhci-adma.c b/drivers/mmc/sdhci-adma.c
> > index 8213223d3f..8c38448b6a 100644
> > --- a/drivers/mmc/sdhci-adma.c
> > +++ b/drivers/mmc/sdhci-adma.c
> > @@ -9,9 +9,10 @@
> > #include
> > #include
> >
> > -static void sdhci_adma_desc(struct sdhci_adma_desc *desc,
> > - dma_addr_t addr, u16 len, bool end)
> > +void sdhci_adma_write_desc(struct sdhci_host *host, void **next_desc,
> > + dma_addr_t addr, int len, bool end)
> > {
> > + struct sdhci_adma_desc *desc = *next_desc;
> > u8 attr;
> >
> > attr = ADMA_DESC_ATTR_VALID | ADMA_DESC_TRANSFER_DATA;
> > @@ -19,17 +20,30 @@ static void sdhci_adma_desc(struct sdhci_adma_desc
> > *desc,
> > attr |= ADMA_DESC_ATTR_END;
> >
> > desc->attr = attr;
> > - desc->len = len;
> > + desc->len = len & 0x;
&g
RE: [PATCH] mmc: sdhci: introduce adma_write_desc() hook to struct sdhci_ops
> -Original Message-
> From: Greg Malysa
> Sent: Tuesday, March 26, 2024 11:18 AM
> To: u-boot@lists.denx.de; Peng Fan ; Jaehoon Chung
>
> Cc: Ian Roberts ; Nathan Barrett-Morrison
> ; Greg
> Malysa ; Jonas Karlman ; Kever Yang
> chips.com>; Peter Geis ; Sean Anderson
> ; Simon Glass
> ; Tom Rini
> Subject: [PATCH] mmc: sdhci: introduce adma_write_desc() hook to struct
> sdhci_ops
>
> From: Ian Roberts
>
> Add this hook so that it can be overridden with driver specific
> implementations. We also let the original sdhci_adma_write_desc()
> accept so that the function can set its new value. Then export
> the function so that it could be reused by driver's specific
> implementations.
>
> The above is a port of Linux kernel commit 54552e4948cbf
>
> In addition, allow drivers to allocate their own ADMA descriptor
> tables if additional space is required.
>
> Finally, fix the assignment of adma_addr to fix compiler warning
> on 64-bit platforms that still use 32-bit DMA addressing.
>
> Co-developed-by: Nathan Barrett-Morrison
> Signed-off-by: Nathan Barrett-Morrison
> Signed-off-by: Greg Malysa
> Signed-off-by: Ian Roberts
Reviewed-by: Jaehoon Chung
Best Regards,
Jaehoon Chung
>
> ---
>
>
> ---
> drivers/mmc/fsl_esdhc.c | 2 +-
> drivers/mmc/sdhci-adma.c | 41 +++-
> drivers/mmc/sdhci.c | 8 +---
> include/sdhci.h | 12 ++--
> 4 files changed, 44 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> index d50669..bd0671cc52 100644
> --- a/drivers/mmc/fsl_esdhc.c
> +++ b/drivers/mmc/fsl_esdhc.c
> @@ -252,7 +252,7 @@ static void esdhc_setup_dma(struct fsl_esdhc_priv *priv,
> struct mmc_data *data)
> priv->adma_desc_table) {
> debug("Using ADMA2\n");
> /* prefer ADMA2 if it is available */
> - sdhci_prepare_adma_table(priv->adma_desc_table, data,
> + sdhci_prepare_adma_table(NULL, priv->adma_desc_table, data,
>priv->dma_addr);
>
> adma_addr = virt_to_phys(priv->adma_desc_table);
> diff --git a/drivers/mmc/sdhci-adma.c b/drivers/mmc/sdhci-adma.c
> index 8213223d3f..8c38448b6a 100644
> --- a/drivers/mmc/sdhci-adma.c
> +++ b/drivers/mmc/sdhci-adma.c
> @@ -9,9 +9,10 @@
> #include
> #include
>
> -static void sdhci_adma_desc(struct sdhci_adma_desc *desc,
> - dma_addr_t addr, u16 len, bool end)
> +void sdhci_adma_write_desc(struct sdhci_host *host, void **next_desc,
> +dma_addr_t addr, int len, bool end)
> {
> + struct sdhci_adma_desc *desc = *next_desc;
> u8 attr;
>
> attr = ADMA_DESC_ATTR_VALID | ADMA_DESC_TRANSFER_DATA;
> @@ -19,17 +20,30 @@ static void sdhci_adma_desc(struct sdhci_adma_desc *desc,
> attr |= ADMA_DESC_ATTR_END;
>
> desc->attr = attr;
> - desc->len = len;
> + desc->len = len & 0x;
> desc->reserved = 0;
> desc->addr_lo = lower_32_bits(addr);
> #ifdef CONFIG_DMA_ADDR_T_64BIT
> desc->addr_hi = upper_32_bits(addr);
> #endif
> +
> + *next_desc += ADMA_DESC_LEN;
> +}
> +
> +static inline void __sdhci_adma_write_desc(struct sdhci_host *host,
> +void **desc, dma_addr_t addr,
> +int len, bool end)
> +{
> + if (host && host->ops && host->ops->adma_write_desc)
> + host->ops->adma_write_desc(host, desc, addr, len, end);
> + else
> + sdhci_adma_write_desc(host, desc, addr, len, end);
> }
>
> /**
> * sdhci_prepare_adma_table() - Populate the ADMA table
> *
> + * @host:Pointer to the sdhci_host
> * @table: Pointer to the ADMA table
> * @data:Pointer to MMC data
> * @addr:DMA address to write to or read from
> @@ -39,25 +53,26 @@ static void sdhci_adma_desc(struct sdhci_adma_desc *desc,
> * Please note, that the table size depends on CONFIG_SYS_MMC_MAX_BLK_COUNT
> and
> * we don't have to check for overflow.
> */
> -void sdhci_prepare_adma_table(struct sdhci_adma_desc *table,
> - struct mmc_data *data, dma_addr_t addr)
> +void sdhci_prepare_adma_table(struct sdhci_host *host,
> + struct sdhci_adma_desc *table,
> + struct mmc_data *data, dma_addr_t start_addr)
> {
> + dma_addr_t addr = start_addr;
> uint trans_bytes = data->blocksize * data->blocks;
> - uint desc_count = DIV_ROUND_UP(trans_bytes, ADMA_MAX_LEN);
> - struct sdhci_adma_desc *desc = table;
> - int i = desc_count;
> + void *next_desc = table;
> + int i = DIV_ROUND_UP(trans_bytes, ADMA_MAX_LEN);
>
> while (--i) {
> - sdhci_adma_desc(desc, addr, ADMA_MAX_LEN, false);
> + __sdhci_adma_write_desc(host, _desc, addr,
> + ADMA_MAX_LEN, false);
>
