mmc: sdhci: Add 64-bit ADMA support
Adrian Hunter
maintainercover mmc: sdhci: Add 64-bit ADMA support
Hi
Here are patches to add 64-bit ADMA support to the SDHCI driver.
The patchset starts with 3 minor fixes related to SDHCI ADMA, then there are 8 preparatory patches, then 3 main patches, then the mmc_test "Badly aligned" tests are extended slightly.
Adrian Hunter (15):
mmc: sdhci: Fix incorrect ADMA2 descriptor table size
mmc: sdhci: Fix ADMA page boundary warnings
mmc: sdhci: Fix ADMA table size warning
mmc: sdhci: Rename two ADMA-related functions for consistency
mmc: sdhci: Rename adma_desc to adma_table
mmc: sdhci: Add sdhci_adma_mark_end()
mmc: sdhci: Use 'void *' for not 'u8 *' for ADMA data
mmc: sdhci: Parameterize ADMA sizes and alignment
mmc: sdhci: Define maximum segments
mmc: sdhci: Define ADMA constants
mmc: sdhci: Define ADMA descriptor structure
mmc: sdhci: Add 64-bit ADMA support
mmc: sdhci-acpi: Add 64-bit DMA support
mmc: sdhci-pci: Add 64-bit DMA support
mmc: mmc_test: Extend "Badly aligned" tests for 8-byte alignment
drivers/mmc/card/mmc_test.c | 10 +- drivers/mmc/host/sdhci-acpi.c | 41 +++++--- drivers/mmc/host/sdhci-pci.c | 14 ++- drivers/mmc/host/sdhci.c | 226 +++++++++++++++++++++++++++--------------- drivers/mmc/host/sdhci.h | 41 ++++++++ include/linux/mmc/sdhci.h | 14 ++- 6 files changed, 241 insertions(+), 105 deletions(-)
Regards
Adrian
Adrian Hunter
maintainerPATCH 1/15 mmc: sdhci: Fix incorrect ADMA2 descriptor table size
The ADMA2 descriptor table size was being calculated incorrectly Fix it.
Note that it has been wrong for a long time and likely has not caused any problems because of a combination of 1) not needing alignment descriptors for block operations 2) more memory being allocated than was requested 3) the use of SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC which does not use an extra descriptor for the end marker.
--- drivers/mmc/host/sdhci.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/mmc/host/sdhci.c +7 -1
@@ -44,7 +44,13 @@#define MAX_TUNING_LOOP 40 -#define ADMA_SIZE ((128 * 2 + 1) * 4) +/* +* The ADMA2 descriptor table size is calculated as the maximum number of +* segments (128), times 2 to allow for an alignment descriptor for each +* segment, plus 1 for a nop end descriptor, all multipled by the 32-bit +* descriptor size (8). +*/ +#define ADMA_SIZE ((128 * 2 + 1) * 8)static unsigned int debug_quirks =0 ;static unsigned int debug_quirks2;
Signature · 1 line
1.9.1
Adrian Hunter
maintainerPATCH 2/15 mmc: sdhci: Fix ADMA page boundary warnings
In reply to Adrian HunterBytes are being copied from/to a single page. The intent of the warning is to warn if the page boundary is crossed. There are two problems. First, PAGE_MASK is mistaken for (PAGE_SIZE - 1). Secondly, instead of using the number of bytes to copy, the warning is using the maximum that that value could be. Fix both.
--- drivers/mmc/host/sdhci.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci.c +4 -2
@@ -525,7 +525,8 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,if (offset) {if (data->flags &MMC_DATA_WRITE ) { buffer =sdhci_kmap_atomic (sg, &flags); -WARN_ON (((long )buffer &PAGE_MASK ) > (PAGE_SIZE -3 )); +WARN_ON (((long )buffer & (PAGE_SIZE -1 )) > + (PAGE_SIZE - offset));memcpy (align, buffer, offset);sdhci_kunmap_atomic (buffer, &flags); }
@@ -630,7 +631,8 @@ static void sdhci_adma_table_post(struct sdhci_host *host, size =4 - (sg_dma_address (sg) &0x3 ); buffer =sdhci_kmap_atomic (sg, &flags); -WARN_ON (((long )buffer &PAGE_MASK ) > (PAGE_SIZE -3 )); +WARN_ON (((long )buffer & (PAGE_SIZE -1 )) > + (PAGE_SIZE - size));memcpy (buffer, align, size);sdhci_kunmap_atomic (buffer, &flags);
Signature · 1 line
1.9.1
Adrian Hunter
maintainerPATCH 3/15 mmc: sdhci: Fix ADMA table size warning
In reply to Adrian HunterThe intent of the warning is to warn if the ADMA table overflows. However there can be one more 'end' entry so the condition should be adjusted accordingly.
--- drivers/mmc/host/sdhci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mmc/host/sdhci.c +1 -1
@@ -555,7 +555,7 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,* If this triggers then we have a calculation bug * somewhere. :/ */ -WARN_ON ((desc - host->adma_desc) >ADMA_SIZE ); +WARN_ON ((desc - host->adma_desc) >=ADMA_SIZE ); }if (host->quirks &SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC ) {
Signature · 1 line
1.9.1
Adrian Hunter
maintainerPATCH 4/15 mmc: sdhci: Rename two ADMA-related functions for consistency
In reply to Adrian HunterRename sdhci_set_adma_desc to sdhci_adma_write_desc and sdhci_show_adma_error to sdhci_adma_show_error so that all ADMA functions start with sdhci_adma_.
--- drivers/mmc/host/sdhci.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
drivers/mmc/host/sdhci.c +7 -7
@@ -454,7 +454,7 @@ static void sdhci_kunmap_atomic(void *buffer, unsigned long *flags)local_irq_restore (*flags); } -static void sdhci_set_adma_desc (u8 *desc,u32 addr,int len,unsigned cmd) +static void sdhci_adma_write_desc (u8 *desc,u32 addr,int len,unsigned cmd) {__le32 *dataddr = (__le32 __force *)(desc +4 );__le16 *cmdlen = (__le16 __force *)desc;
@@ -532,7 +532,7 @@ static int sdhci_adma_table_pre(struct sdhci_host *host, }/* tran, valid */ -sdhci_set_adma_desc (desc, align_addr, offset,0x21 ); +sdhci_adma_write_desc (desc, align_addr, offset,0x21 );BUG_ON (offset >65536 );
@@ -548,7 +548,7 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,BUG_ON (len >65536 );/* tran, valid */ -sdhci_set_adma_desc (desc, addr, len,0x21 ); +sdhci_adma_write_desc (desc, addr, len,0x21 ); desc +=8 ;/*
@@ -572,7 +572,7 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,*/ /* nop, end, valid */ -sdhci_set_adma_desc (desc,0 ,0 ,0x3 ); +sdhci_adma_write_desc (desc,0 ,0 ,0x3 ); }/*
@@ -2290,7 +2290,7 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask, u32 *mask) }#ifdef CONFIG_MMC_DEBUG -static void sdhci_show_adma_error (struct sdhci_host *host) +static void sdhci_adma_show_error (struct sdhci_host *host) {const char *name =mmc_hostname (host->mmc);u8 *desc = host->adma_desc;
@@ -2315,7 +2315,7 @@ static void sdhci_show_adma_error(struct sdhci_host *host) } }#else -static void sdhci_show_adma_error (struct sdhci_host *host) { } +static void sdhci_adma_show_error (struct sdhci_host *host) { }#endif static void sdhci_data_irq (struct sdhci_host *host,u32 intmask)
@@ -2378,7 +2378,7 @@ static void sdhci_data_irq(struct sdhci_host *host, u32 intmask) host->data->error = -EILSEQ ; else if (intmask & SDHCI_INT_ADMA_ERROR) {pr_err ("%s: ADMA error\n" ,mmc_hostname (host->mmc)); -sdhci_show_adma_error (host); +sdhci_adma_show_error (host); host->data->error = -EIO ;if (host->ops->adma_workaround) host->ops->adma_workaround (host, intmask);
Signature · 1 line
1.9.1
Adrian Hunter
maintainerPATCH 5/15 mmc: sdhci: Rename adma_desc to adma_table
In reply to Adrian HunterIn preparation for 64-bit ADMA, rename adma_desc to adma_table. That is because members will be added for descriptor size and table size, so using adma_desc (which is the table) is confusing.
--- drivers/mmc/host/sdhci.c | 31 ++++++++++++++++--------------- include/linux/mmc/sdhci.h | 2 +- 2 files changed, 17 insertions(+), 16 deletions(-)
drivers/mmc/host/sdhci.c +16 -15
@@ -505,7 +505,7 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,if (host->sg_count ==0 ) goto unmap_align; - desc = host->adma_desc; + desc = host->adma_table; align = host->align_buffer; align_addr = host->align_addr;
@@ -555,14 +555,14 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,* If this triggers then we have a calculation bug * somewhere. :/ */ -WARN_ON ((desc - host->adma_desc) >=ADMA_SIZE ); +WARN_ON ((desc - host->adma_table) >=ADMA_SIZE ); }if (host->quirks &SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC ) {/* * Mark the last descriptor as the terminating descriptor */ -if (desc != host->adma_desc) { +if (desc != host->adma_table) { desc -=8 ; desc[0 ] |=0x2 ;/* end */ }
@@ -2293,7 +2293,7 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask, u32 *mask)static void sdhci_adma_show_error (struct sdhci_host *host) {const char *name =mmc_hostname (host->mmc); -u8 *desc = host->adma_desc; +u8 *desc = host->adma_table;__le32 *dma;__le16 *len;u8 attr;
@@ -2874,27 +2874,28 @@ int sdhci_add_host(struct sdhci_host *host)* (128) and potentially one alignment transfer for * each of those entries. */ - host->adma_desc =dma_alloc_coherent (mmc_dev (mmc), -ADMA_SIZE , &host->adma_addr, -GFP_KERNEL ); + host->adma_table =dma_alloc_coherent (mmc_dev (mmc), +ADMA_SIZE , + &host->adma_addr, +GFP_KERNEL ); host->align_buffer =kmalloc (128 *4 ,GFP_KERNEL ); -if (!host->adma_desc || !host->align_buffer) { +if (!host->adma_table || !host->align_buffer) {dma_free_coherent (mmc_dev (mmc),ADMA_SIZE , - host->adma_desc, host->adma_addr); + host->adma_table, host->adma_addr);kfree (host->align_buffer);pr_warn ("%s: Unable to allocate ADMA buffers - falling back to standard DMA\n" ,mmc_hostname (mmc)); host->flags &= ~SDHCI_USE_ADMA ; - host->adma_desc =NULL ; + host->adma_table =NULL ; host->align_buffer =NULL ; }else if (host->adma_addr &3 ) {pr_warn ("%s: unable to allocate aligned ADMA descriptor\n" ,mmc_hostname (mmc)); host->flags &= ~SDHCI_USE_ADMA ;dma_free_coherent (mmc_dev (mmc),ADMA_SIZE , - host->adma_desc, host->adma_addr); + host->adma_table, host->adma_addr);kfree (host->align_buffer); - host->adma_desc =NULL ; + host->adma_table =NULL ; host->align_buffer =NULL ; } }
@@ -3353,12 +3354,12 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)if (!IS_ERR (mmc->supply.vqmmc))regulator_disable (mmc->supply.vqmmc); -if (host->adma_desc) +if (host->adma_table)dma_free_coherent (mmc_dev (mmc),ADMA_SIZE , - host->adma_desc, host->adma_addr); + host->adma_table, host->adma_addr);kfree (host->align_buffer); - host->adma_desc =NULL ; + host->adma_table =NULL ; host->align_buffer =NULL ; }
include/linux/mmc/sdhci.h +1 -1
@@ -155,7 +155,7 @@ struct sdhci_host {int sg_count;/* Mapped sg entries */ -u8 *adma_desc;/* ADMA descriptor table */ +u8 *adma_table;/* ADMA descriptor table */ u8 *align_buffer;/* Bounce buffer */ dma_addr_t adma_addr;/* Mapped ADMA descr. table */
Signature · 1 line
1.9.1
Adrian Hunter
maintainerPATCH 6/15 mmc: sdhci: Add sdhci_adma_mark_end()
In reply to Adrian HunterIn preparation for 64-bit ADMA, separate out code
that touches the ADMA descriptor by addingsdhci_adma_mark_end().
--- drivers/mmc/host/sdhci.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/mmc/host/sdhci.c +8 -1
@@ -468,6 +468,13 @@ static void sdhci_adma_write_desc(u8 *desc, u32 addr, int len, unsigned cmd) dataddr[0 ] =cpu_to_le32 (addr); } +static void sdhci_adma_mark_end (void *desc) +{ +u8 *dma_desc = desc; + + dma_desc[0 ] |=0x2 ;/* end */ +} +static int sdhci_adma_table_pre (struct sdhci_host *host,struct mmc_data *data) {
@@ -564,7 +571,7 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,*/ if (desc != host->adma_table) { desc -=8 ; - desc[0 ] |=0x2 ;/* end */ +sdhci_adma_mark_end (desc); } }else {/*
Signature · 1 line
1.9.1
Adrian Hunter
maintainerPATCH 7/15 mmc: sdhci: Use 'void *' for not 'u8 *' for ADMA data
In reply to Adrian HunterIt is kernel-style to use 'void *' for anonymous data. This is being applied to the ADMA bounce buffer which contains unaligned bytes, and to the ADMA descriptor table which will contain 32-bit ADMA descriptors or 64-bit ADMA descriptors when support is added.
--- drivers/mmc/host/sdhci.c | 10 +++++----- include/linux/mmc/sdhci.h | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-)
drivers/mmc/host/sdhci.c +5 -5
@@ -454,7 +454,7 @@ static void sdhci_kunmap_atomic(void *buffer, unsigned long *flags)local_irq_restore (*flags); } -static void sdhci_adma_write_desc (u8 *desc,u32 addr,int len,unsigned cmd) +static void sdhci_adma_write_desc (void *desc,u32 addr,int len,unsigned cmd) {__le32 *dataddr = (__le32 __force *)(desc +4 );__le16 *cmdlen = (__le16 __force *)desc;
@@ -480,8 +480,8 @@ static int sdhci_adma_table_pre(struct sdhci_host *host, {int direction; -u8 *desc; -u8 *align; +void *desc; +void *align;dma_addr_t addr;dma_addr_t align_addr;int len, offset;
@@ -606,7 +606,7 @@ static void sdhci_adma_table_post(struct sdhci_host *host,struct scatterlist *sg;int i, size; -u8 *align; +void *align;char *buffer;unsigned long flags;bool has_unaligned;
@@ -2300,7 +2300,7 @@ static void sdhci_cmd_irq(struct sdhci_host *host, u32 intmask, u32 *mask)static void sdhci_adma_show_error (struct sdhci_host *host) {const char *name =mmc_hostname (host->mmc); -u8 *desc = host->adma_table; +void *desc = host->adma_table;__le32 *dma;__le16 *len;u8 attr;
include/linux/mmc/sdhci.h +2 -2
@@ -155,8 +155,8 @@ struct sdhci_host {int sg_count;/* Mapped sg entries */ -u8 *adma_table;/* ADMA descriptor table */ -u8 *align_buffer;/* Bounce buffer */ +void *adma_table;/* ADMA descriptor table */ +void *align_buffer;/* Bounce buffer */ dma_addr_t adma_addr;/* Mapped ADMA descr. table */ dma_addr_t align_addr;/* Mapped bounce buffer */
Signature · 1 line
1.9.1
Adrian Hunter
maintainerPATCH 8/15 mmc: sdhci: Parameterize ADMA sizes and alignment
In reply to Adrian HunterIn preparation for 64-bit ADMA, parameterize ADMA sizes and alignment. 64-bit ADMA has a larger descriptor because it contains a 64-bit address instead of a 32-bit address. Also data must be 8-byte aligned instead of 4-byte aligned. Consequently, sdhci_host members are added for descriptor, table, and buffer sizes and alignment.
--- drivers/mmc/host/sdhci.c | 68 +++++++++++++++++++++++------------------------ include/linux/mmc/sdhci.h | 7 +++++ 2 files changed, 41 insertions(+), 34 deletions(-)
drivers/mmc/host/sdhci.c +34 -34
@@ -44,14 +44,6 @@#define MAX_TUNING_LOOP 40 -/* -* The ADMA2 descriptor table size is calculated as the maximum number of -* segments (128), times 2 to allow for an alignment descriptor for each -* segment, plus 1 for a nop end descriptor, all multipled by the 32-bit -* descriptor size (8). -*/ -#define ADMA_SIZE ((128 * 2 + 1) * 8) -static unsigned int debug_quirks =0 ;static unsigned int debug_quirks2;
@@ -502,10 +494,10 @@ static int sdhci_adma_table_pre(struct sdhci_host *host, direction =DMA_TO_DEVICE ; host->align_addr =dma_map_single (mmc_dev (host->mmc), - host->align_buffer,128 *4 , direction); + host->align_buffer, host->align_buffer_sz, direction);if (dma_mapping_error (mmc_dev (host->mmc), host->align_addr)) goto fail; -BUG_ON (host->align_addr &0x3 ); +BUG_ON (host->align_addr & host->align_mask); host->sg_count =dma_map_sg (mmc_dev (host->mmc), data->sg, data->sg_len, direction);
@@ -528,7 +520,8 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,* the (up to three) bytes that screw up the * alignment. */ - offset = (4 - (addr &0x3 )) &0x3 ; + offset = (host->align_sz - (addr & host->align_mask)) & + host->align_mask;if (offset) {if (data->flags &MMC_DATA_WRITE ) { buffer =sdhci_kmap_atomic (sg, &flags);
@@ -543,10 +536,10 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,BUG_ON (offset >65536 ); - align +=4 ; - align_addr +=4 ; + align += host->align_sz; + align_addr += host->align_sz; - desc +=8 ; + desc += host->desc_sz; addr += offset; len -= offset;
@@ -556,13 +549,13 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,/* tran, valid */ sdhci_adma_write_desc (desc, addr, len,0x21 ); - desc +=8 ; + desc += host->desc_sz;/* * If this triggers then we have a calculation bug * somewhere. :/ */ -WARN_ON ((desc - host->adma_table) >=ADMA_SIZE ); +WARN_ON ((desc - host->adma_table) >= host->adma_table_sz); }if (host->quirks &SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC ) {
@@ -570,7 +563,7 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,* Mark the last descriptor as the terminating descriptor */ if (desc != host->adma_table) { - desc -=8 ; + desc -= host->desc_sz;sdhci_adma_mark_end (desc); } }else {
@@ -587,14 +580,14 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,*/ if (data->flags &MMC_DATA_WRITE ) {dma_sync_single_for_device (mmc_dev (host->mmc), - host->align_addr,128 *4 , direction); + host->align_addr, host->align_buffer_sz, direction); }return 0 ; unmap_align:dma_unmap_single (mmc_dev (host->mmc), host->align_addr, -128 *4 , direction); + host->align_buffer_sz, direction); fail:return -EINVAL ; }
@@ -617,12 +610,12 @@ static void sdhci_adma_table_post(struct sdhci_host *host, direction =DMA_TO_DEVICE ;dma_unmap_single (mmc_dev (host->mmc), host->align_addr, -128 *4 , direction); + host->align_buffer_sz, direction);/* Do a quick scan of the SG list for any unaligned mappings */ has_unaligned = false;for_each_sg (data->sg, sg, host->sg_count, i) -if (sg_dma_address (sg) &3 ) { +if (sg_dma_address (sg) & host->align_mask) { has_unaligned = true;break ; }
@@ -634,8 +627,9 @@ static void sdhci_adma_table_post(struct sdhci_host *host, align = host->align_buffer;for_each_sg (data->sg, sg, host->sg_count, i) { -if (sg_dma_address (sg) &0x3 ) { - size =4 - (sg_dma_address (sg) &0x3 ); +if (sg_dma_address (sg) & host->align_mask) { + size = host->align_sz - + (sg_dma_address (sg) & host->align_mask); buffer =sdhci_kmap_atomic (sg, &flags); WARN_ON(((long)buffer & (PAGE_SIZE - 1)) >
@@ -643,7 +637,7 @@ static void sdhci_adma_table_post(struct sdhci_host *host,memcpy (buffer, align, size);sdhci_kunmap_atomic (buffer, &flags); - align +=4 ; + align += host->align_sz; } } }
@@ -2315,7 +2309,7 @@ static void sdhci_adma_show_error(struct sdhci_host *host)DBG ("%s: %p: DMA 0x%08x, LEN 0x%04x, Attr=0x%02x\n" , name, desc,le32_to_cpu (*dma),le16_to_cpu (*len), attr); - desc +=8 ; + desc += host->desc_sz;if (attr &2 )break ;
@@ -2877,17 +2871,23 @@ int sdhci_add_host(struct sdhci_host *host)if (host->flags &SDHCI_USE_ADMA ) {/* -* We need to allocate descriptors for all sg entries -* (128) and potentially one alignment transfer for -* each of those entries. +* The DMA descriptor table size is calculated as the maximum +* number of segments times 2, to allow for an alignment +* descriptor for each segment, plus 1 for a nop end descriptor, +* all multipled by the descriptor size. */ + host->adma_table_sz = (128 *2 +1 ) *8 ; + host->align_buffer_sz =128 *4 ; + host->desc_sz =8 ; + host->align_sz =4 ; + host->align_mask =3 ; host->adma_table =dma_alloc_coherent (mmc_dev (mmc), -ADMA_SIZE , + host->adma_table_sz, &host->adma_addr,GFP_KERNEL ); - host->align_buffer =kmalloc (128 *4 ,GFP_KERNEL ); + host->align_buffer =kmalloc (host->align_buffer_sz,GFP_KERNEL );if (!host->adma_table || !host->align_buffer) { -dma_free_coherent (mmc_dev (mmc),ADMA_SIZE , +dma_free_coherent (mmc_dev (mmc), host->adma_table_sz, host->adma_table, host->adma_addr);kfree (host->align_buffer); pr_warn("%s: Unable to allocate ADMA buffers - falling back to standard DMA\n",
@@ -2895,11 +2895,11 @@ int sdhci_add_host(struct sdhci_host *host) host->flags &= ~SDHCI_USE_ADMA ; host->adma_table =NULL ; host->align_buffer =NULL ; - }else if (host->adma_addr &3 ) { + }else if (host->adma_addr & host->align_mask) {pr_warn ("%s: unable to allocate aligned ADMA descriptor\n" ,mmc_hostname (mmc)); host->flags &= ~SDHCI_USE_ADMA ; -dma_free_coherent (mmc_dev (mmc),ADMA_SIZE , +dma_free_coherent (mmc_dev (mmc), host->adma_table_sz, host->adma_table, host->adma_addr);kfree (host->align_buffer); host->adma_table =NULL ;
@@ -3362,7 +3362,7 @@ void sdhci_remove_host(struct sdhci_host *host, int dead)regulator_disable (mmc->supply.vqmmc);if (host->adma_table) -dma_free_coherent (mmc_dev (mmc),ADMA_SIZE , +dma_free_coherent (mmc_dev (mmc), host->adma_table_sz, host->adma_table, host->adma_addr);kfree (host->align_buffer);
include/linux/mmc/sdhci.h +7 -0
@@ -158,9 +158,16 @@ struct sdhci_host {void *adma_table;/* ADMA descriptor table */ void *align_buffer;/* Bounce buffer */ +size_t adma_table_sz;/* ADMA descriptor table size */ +size_t align_buffer_sz;/* Bounce buffer size */ +dma_addr_t adma_addr;/* Mapped ADMA descr. table */ dma_addr_t align_addr;/* Mapped bounce buffer */ +unsigned int desc_sz;/* ADMA descriptor size */ +unsigned int align_sz;/* ADMA alignment */ +unsigned int align_mask;/* ADMA alignment mask */ +struct tasklet_struct finish_tasklet;/* Tasklet structures */ struct timer_list timer;/* Timer for timeouts */
Signature · 1 line
1.9.1
Adrian Hunter
maintainerPATCH 9/15 mmc: sdhci: Define maximum segments
In reply to Adrian HunterDefine the maximum number of segments instead of
having the constant 128 appearing in the code in
various places.
--- drivers/mmc/host/sdhci.c | 8 ++++---- drivers/mmc/host/sdhci.h | 6 ++++++ 2 files changed, 10 insertions(+), 4 deletions(-)
drivers/mmc/host/sdhci.c +4 -4
@@ -2876,8 +2876,8 @@ int sdhci_add_host(struct sdhci_host *host)* descriptor for each segment, plus 1 for a nop end descriptor, * all multipled by the descriptor size. */ - host->adma_table_sz = (128 *2 +1 ) *8 ; - host->align_buffer_sz =128 *4 ; + host->adma_table_sz = (SDHCI_MAX_SEGS *2 +1 ) *8 ; + host->align_buffer_sz =SDHCI_MAX_SEGS *4 ; host->desc_sz =8 ; host->align_sz =4 ; host->align_mask =3 ;
@@ -3191,11 +3191,11 @@ int sdhci_add_host(struct sdhci_host *host)* can do scatter/gather or not. */ if (host->flags &SDHCI_USE_ADMA ) - mmc->max_segs =128 ; + mmc->max_segs =SDHCI_MAX_SEGS ;else if (host->flags &SDHCI_USE_SDMA ) mmc->max_segs =1 ;else /* PIO */ - mmc->max_segs =128 ; + mmc->max_segs =SDHCI_MAX_SEGS ;/* * Maximum number of sectors in one transfer. Limited by DMA boundary
drivers/mmc/host/sdhci.h +6 -0
@@ -266,6 +266,12 @@#define SDHCI_DEFAULT_BOUNDARY_SIZE (512 * 1024)#define SDHCI_DEFAULT_BOUNDARY_ARG (ilog2(SDHCI_DEFAULT_BOUNDARY_SIZE) - 12) +/* +* Maximum segments assuming a 512KiB maximum requisition size and a minimum +* 4KiB page size. +*/ +#define SDHCI_MAX_SEGS 128 +struct sdhci_ops {#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS u32 (*read_l)(struct sdhci_host *host,int reg);
Signature · 1 line
1.9.1
Adrian Hunter
maintainerPATCH 10/15 mmc: sdhci: Define ADMA constants
In reply to Adrian HunterDefine all the ADMA constants instead of having numbers scattered throughout the code.
--- drivers/mmc/host/sdhci.c | 23 +++++++++++++---------- drivers/mmc/host/sdhci.h | 10 ++++++++++ 2 files changed, 23 insertions(+), 10 deletions(-)
drivers/mmc/host/sdhci.c +13 -10
@@ -464,7 +464,7 @@ static void sdhci_adma_mark_end(void *desc) {u8 *dma_desc = desc; - dma_desc[0 ] |=0x2 ;/* end */ + dma_desc[0 ] |=ADMA2_END ; } static int sdhci_adma_table_pre(struct sdhci_host *host,
@@ -532,7 +532,8 @@ static int sdhci_adma_table_pre(struct sdhci_host *host, }/* tran, valid */ -sdhci_adma_write_desc (desc, align_addr, offset,0x21 ); +sdhci_adma_write_desc (desc, align_addr, offset, +ADMA2_TRAN_VALID );BUG_ON (offset >65536 );
@@ -548,7 +549,7 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,BUG_ON (len >65536 );/* tran, valid */ -sdhci_adma_write_desc (desc, addr, len,0x21 ); +sdhci_adma_write_desc (desc, addr, len,ADMA2_TRAN_VALID ); desc += host->desc_sz;/*
@@ -572,7 +573,7 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,*/ /* nop, end, valid */ -sdhci_adma_write_desc (desc,0 ,0 ,0x3 ); +sdhci_adma_write_desc (desc,0 ,0 ,ADMA2_NOP_END_VALID ); }/*
@@ -2311,7 +2312,7 @@ static void sdhci_adma_show_error(struct sdhci_host *host) desc += host->desc_sz; -if (attr &2 ) +if (attr &ADMA2_END )break ; } }
@@ -2876,11 +2877,13 @@ int sdhci_add_host(struct sdhci_host *host)* descriptor for each segment, plus 1 for a nop end descriptor, * all multipled by the descriptor size. */ - host->adma_table_sz = (SDHCI_MAX_SEGS *2 +1 ) *8 ; - host->align_buffer_sz =SDHCI_MAX_SEGS *4 ; - host->desc_sz =8 ; - host->align_sz =4 ; - host->align_mask =3 ; + host->adma_table_sz = (SDHCI_MAX_SEGS *2 +1 ) * +SDHCI_ADMA2_32_DESC_SZ ; + host->align_buffer_sz =SDHCI_MAX_SEGS * +SDHCI_ADMA2_32_ALIGN ; + host->desc_sz =SDHCI_ADMA2_32_DESC_SZ ; + host->align_sz =SDHCI_ADMA2_32_ALIGN ; + host->align_mask =SDHCI_ADMA2_32_ALIGN -1 ; host->adma_table = dma_alloc_coherent(mmc_dev(mmc), host->adma_table_sz, &host->adma_addr,
drivers/mmc/host/sdhci.h +10 -0
@@ -266,6 +266,16 @@#define SDHCI_DEFAULT_BOUNDARY_SIZE (512 * 1024)#define SDHCI_DEFAULT_BOUNDARY_ARG (ilog2(SDHCI_DEFAULT_BOUNDARY_SIZE) - 12) +/* ADMA2 32-bit DMA descriptor size */ +#define SDHCI_ADMA2_32_DESC_SZ 8 + +/* ADMA2 32-bit DMA alignment */ +#define SDHCI_ADMA2_32_ALIGN 4 + +#define ADMA2_TRAN_VALID 0x21 +#define ADMA2_NOP_END_VALID 0x3 +#define ADMA2_END 0x2 +/* * Maximum segments assuming a 512KiB maximum requisition size and a minimum * 4KiB page size.
Signature · 1 line
1.9.1
Adrian Hunter
maintainerPATCH 11/15 mmc: sdhci: Define ADMA descriptor structure
In reply to Adrian HunterDefine the ADMA descriptor structure instead of
using manual offsets and casts.
--- drivers/mmc/host/sdhci.c | 30 +++++++++++------------------- drivers/mmc/host/sdhci.h | 7 +++++++ 2 files changed, 18 insertions(+), 19 deletions(-)
drivers/mmc/host/sdhci.c +11 -19
@@ -448,23 +448,18 @@ static void sdhci_kunmap_atomic(void *buffer, unsigned long *flags)static void sdhci_adma_write_desc (void *desc,u32 addr,int len,unsigned cmd) { -__le32 *dataddr = (__le32 __force *)(desc +4 ); -__le16 *cmdlen = (__le16 __force *)desc; +struct sdhci_adma2_32_desc *dma_desc = desc; -/* SDHCI specification says ADMA descriptors should be 4 byte -* aligned, so using 16 or 32bit operations should be safe. */ - - cmdlen[0 ] =cpu_to_le16 (cmd); - cmdlen[1 ] =cpu_to_le16 (len); - - dataddr[0 ] =cpu_to_le32 (addr); + dma_desc->cmd =cpu_to_le16 (cmd); + dma_desc->len =cpu_to_le16 (len); + dma_desc->addr =cpu_to_le32 (addr); }static void sdhci_adma_mark_end (void *desc) { -u8 *dma_desc = desc; +struct sdhci_adma2_32_desc *dma_desc = desc; - dma_desc[0 ] |=ADMA2_END ; + dma_desc->cmd |=cpu_to_le16 (ADMA2_END ); } static int sdhci_adma_table_pre(struct sdhci_host *host,
@@ -2296,23 +2291,20 @@ static void sdhci_adma_show_error(struct sdhci_host *host) {const char *name =mmc_hostname (host->mmc);void *desc = host->adma_table; -__le32 *dma; -__le16 *len; -u8 attr;sdhci_dumpregs (host);while (true) { - dma = (__le32 *)(desc +4 ); - len = (__le16 *)(desc +2 ); - attr = *desc; +struct sdhci_adma2_32_desc *dma_desc = desc;DBG ("%s: %p: DMA 0x%08x, LEN 0x%04x, Attr=0x%02x\n" , - name, desc,le32_to_cpu (*dma),le16_to_cpu (*len), attr); + name, desc,le32_to_cpu (dma_desc->addr), +le16_to_cpu (dma_desc->len), +le16_to_cpu (dma_desc->cmd)); desc += host->desc_sz; -if (attr &ADMA2_END ) +if (dma_desc->cmd &cpu_to_le16 (ADMA2_END ))break ; } }
drivers/mmc/host/sdhci.h +7 -0
@@ -272,6 +272,13 @@/* ADMA2 32-bit DMA alignment */ #define SDHCI_ADMA2_32_ALIGN 4 +/* ADMA2 32-bit descriptor */ +struct sdhci_adma2_32_desc { +__le16 cmd; +__le16 len; +__le32 addr; +} __packed__aligned(SDHCI_ADMA2_32_ALIGN) ; +#define ADMA2_TRAN_VALID 0x21#define ADMA2_NOP_END_VALID 0x3#define ADMA2_END 0x2
Signature · 1 line
1.9.1
Adrian Hunter
maintainerPATCH 12/15 mmc: sdhci: Add 64-bit ADMA support
In reply to Adrian HunterAdd 64-bit ADMA support including:
- add 64-bit ADMA descriptor
- add SDHCI_USE_64_BIT_DMA flag
- set upper 32-bits of DMA addresses
- ability to select 64-bit ADMA
- ability to use 64-bit ADMA sizes and alignment
- display "ADMA 64-bit" when host is added
It is assumed that a 64-bit capable device has set a 64-bit DMA mask and *must* do 64-bit DMA. A driver has the opportunity to change that during the first call to ->enable_dma(). Similarly SDHCI_QUIRK2_BROKEN_64_BIT_DMA must be left to the drivers to implement.
--- drivers/mmc/host/sdhci.c | 109 ++++++++++++++++++++++++++++++++++------------ drivers/mmc/host/sdhci.h | 18 ++++++++ include/linux/mmc/sdhci.h | 3 ++ 3 files changed, 102 insertions(+), 28 deletions(-)
drivers/mmc/host/sdhci.c +81 -28
@@ -117,10 +117,17 @@ static void sdhci_dumpregs(struct sdhci_host *host)pr_debug (DRIVER_NAME ": Host ctl2: 0x%08x\n" ,sdhci_readw (host,SDHCI_HOST_CONTROL2 )); -if (host->flags &SDHCI_USE_ADMA ) -pr_debug (DRIVER_NAME ": ADMA Err: 0x%08x | ADMA Ptr: 0x%08x\n" , -readl (host->ioaddr +SDHCI_ADMA_ERROR ), -readl (host->ioaddr +SDHCI_ADMA_ADDRESS )); +if (host->flags &SDHCI_USE_ADMA ) { +if (host->flags &SDHCI_USE_64_BIT_DMA ) +pr_debug (DRIVER_NAME ": ADMA Err: 0x%08x | ADMA Ptr: 0x%08x%08x\n" , +readl (host->ioaddr +SDHCI_ADMA_ERROR ), +readl (host->ioaddr +SDHCI_ADMA_ADDRESS_HI ), +readl (host->ioaddr +SDHCI_ADMA_ADDRESS )); +else +pr_debug (DRIVER_NAME ": ADMA Err: 0x%08x | ADMA Ptr: 0x%08x\n" , +readl (host->ioaddr +SDHCI_ADMA_ERROR ), +readl (host->ioaddr +SDHCI_ADMA_ADDRESS )); + }pr_debug (DRIVER_NAME ": ===========================================\n" ); }
@@ -446,19 +453,25 @@ static void sdhci_kunmap_atomic(void *buffer, unsigned long *flags)local_irq_restore (*flags); } -static void sdhci_adma_write_desc (void *desc,u32 addr,int len,unsigned cmd) +static void sdhci_adma_write_desc (struct sdhci_host *host,void *desc, +dma_addr_t addr,int len,unsigned cmd) { -struct sdhci_adma2_32_desc *dma_desc = desc; +struct sdhci_adma2_64_desc *dma_desc = desc; +/* 32-bit and 64-bit descriptors have these members in same position */ dma_desc->cmd =cpu_to_le16 (cmd); dma_desc->len =cpu_to_le16 (len); - dma_desc->addr =cpu_to_le32 (addr); + dma_desc->addr_lo =cpu_to_le32 ((u32 )addr); + +if (host->flags &SDHCI_USE_64_BIT_DMA ) + dma_desc->addr_hi =cpu_to_le32 ((u64 )addr >>32 ); }static void sdhci_adma_mark_end (void *desc) { -struct sdhci_adma2_32_desc *dma_desc = desc; +struct sdhci_adma2_64_desc *dma_desc = desc; +/* 32-bit and 64-bit descriptors have 'cmd' in same position */ dma_desc->cmd |=cpu_to_le16 (ADMA2_END ); }
@@ -527,7 +540,7 @@ static int sdhci_adma_table_pre(struct sdhci_host *host, }/* tran, valid */ -sdhci_adma_write_desc (desc, align_addr, offset, +sdhci_adma_write_desc (host, desc, align_addr, offset,ADMA2_TRAN_VALID );BUG_ON (offset >65536 );
@@ -544,7 +557,7 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,BUG_ON (len >65536 );/* tran, valid */ -sdhci_adma_write_desc (desc, addr, len,ADMA2_TRAN_VALID ); +sdhci_adma_write_desc (host, desc, addr, len,ADMA2_TRAN_VALID ); desc += host->desc_sz;/*
@@ -568,7 +581,7 @@ static int sdhci_adma_table_pre(struct sdhci_host *host,*/ /* nop, end, valid */ -sdhci_adma_write_desc (desc,0 ,0 ,ADMA2_NOP_END_VALID ); +sdhci_adma_write_desc (host, desc,0 ,0 ,ADMA2_NOP_END_VALID ); }/*
@@ -827,6 +840,10 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd) }else {sdhci_writel (host, host->adma_addr,SDHCI_ADMA_ADDRESS ); +if (host->flags &SDHCI_USE_64_BIT_DMA ) +sdhci_writel (host, + (u64 )host->adma_addr >>32 , +SDHCI_ADMA_ADDRESS_HI ); } }else {int sg_cnt;
@@ -860,10 +877,14 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_command *cmd) ctrl =sdhci_readb (host,SDHCI_HOST_CONTROL ); ctrl &= ~SDHCI_CTRL_DMA_MASK ;if ((host->flags &SDHCI_REQ_USE_DMA ) && - (host->flags &SDHCI_USE_ADMA )) - ctrl |=SDHCI_CTRL_ADMA32 ; -else + (host->flags &SDHCI_USE_ADMA )) { +if (host->flags &SDHCI_USE_64_BIT_DMA ) + ctrl |=SDHCI_CTRL_ADMA64 ; +else + ctrl |=SDHCI_CTRL_ADMA32 ; + }else { ctrl |=SDHCI_CTRL_SDMA ; + }sdhci_writeb (host, ctrl,SDHCI_HOST_CONTROL ); }
@@ -2295,12 +2316,19 @@ static void sdhci_adma_show_error(struct sdhci_host *host)sdhci_dumpregs (host);while (true) { -struct sdhci_adma2_32_desc *dma_desc = desc; - -DBG ("%s: %p: DMA 0x%08x, LEN 0x%04x, Attr=0x%02x\n" , - name, desc,le32_to_cpu (dma_desc->addr), -le16_to_cpu (dma_desc->len), -le16_to_cpu (dma_desc->cmd)); +struct sdhci_adma2_64_desc *dma_desc = desc; + +if (host->flags &SDHCI_USE_64_BIT_DMA ) +DBG ("%s: %p: DMA 0x%08x%08x, LEN 0x%04x, Attr=0x%02x\n" , + name, desc,le32_to_cpu (dma_desc->addr_hi), +le32_to_cpu (dma_desc->addr_lo), +le16_to_cpu (dma_desc->len), +le16_to_cpu (dma_desc->cmd)); +else +DBG ("%s: %p: DMA 0x%08x, LEN 0x%04x, Attr=0x%02x\n" , + name, desc,le32_to_cpu (dma_desc->addr_lo), +le16_to_cpu (dma_desc->len), +le16_to_cpu (dma_desc->cmd)); desc += host->desc_sz;
@@ -2851,6 +2879,16 @@ int sdhci_add_host(struct sdhci_host *host) host->flags &= ~SDHCI_USE_ADMA ; } +/* +* It is assumed that a 64-bit capable device has set a 64-bit DMA mask +* and *must* do 64-bit DMA. A driver has the opportunity to change +* that during the first call to ->enable_dma(). Similarly +* SDHCI_QUIRK2_BROKEN_64_BIT_DMA must be left to the drivers to +* implement. +*/ +if (sdhci_readl (host,SDHCI_CAPABILITIES ) &SDHCI_CAN_64BIT ) + host->flags |=SDHCI_USE_64_BIT_DMA ; +if (host->flags & (SDHCI_USE_SDMA |SDHCI_USE_ADMA )) {if (host->ops->enable_dma) {if (host->ops->enable_dma (host)) {
@@ -2862,6 +2900,10 @@ int sdhci_add_host(struct sdhci_host *host) } } +/* SDMA does not support 64-bit DMA */ +if (host->flags &SDHCI_USE_64_BIT_DMA ) + host->flags &= ~SDHCI_USE_SDMA ; +if (host->flags &SDHCI_USE_ADMA ) {/* * The DMA descriptor table size is calculated as the maximum
@@ -2869,13 +2911,23 @@ int sdhci_add_host(struct sdhci_host *host)* descriptor for each segment, plus 1 for a nop end descriptor, * all multipled by the descriptor size. */ - host->adma_table_sz = (SDHCI_MAX_SEGS *2 +1 ) * -SDHCI_ADMA2_32_DESC_SZ ; - host->align_buffer_sz =SDHCI_MAX_SEGS * -SDHCI_ADMA2_32_ALIGN ; - host->desc_sz =SDHCI_ADMA2_32_DESC_SZ ; - host->align_sz =SDHCI_ADMA2_32_ALIGN ; - host->align_mask =SDHCI_ADMA2_32_ALIGN -1 ; +if (host->flags &SDHCI_USE_64_BIT_DMA ) { + host->adma_table_sz = (SDHCI_MAX_SEGS *2 +1 ) * +SDHCI_ADMA2_64_DESC_SZ ; + host->align_buffer_sz =SDHCI_MAX_SEGS * +SDHCI_ADMA2_64_ALIGN ; + host->desc_sz =SDHCI_ADMA2_64_DESC_SZ ; + host->align_sz =SDHCI_ADMA2_64_ALIGN ; + host->align_mask =SDHCI_ADMA2_64_ALIGN -1 ; + }else { + host->adma_table_sz = (SDHCI_MAX_SEGS *2 +1 ) * +SDHCI_ADMA2_32_DESC_SZ ; + host->align_buffer_sz =SDHCI_MAX_SEGS * +SDHCI_ADMA2_32_ALIGN ; + host->desc_sz =SDHCI_ADMA2_32_DESC_SZ ; + host->align_sz =SDHCI_ADMA2_32_ALIGN ; + host->align_mask =SDHCI_ADMA2_32_ALIGN -1 ; + } host->adma_table = dma_alloc_coherent(mmc_dev(mmc), host->adma_table_sz, &host->adma_addr,
@@ -3288,7 +3340,8 @@ int sdhci_add_host(struct sdhci_host *host)pr_info ("%s: SDHCI controller on %s [%s] using %s\n" ,mmc_hostname (mmc), host->hw_name,dev_name (mmc_dev (mmc)), - (host->flags &SDHCI_USE_ADMA ) ?"ADMA" : + (host->flags &SDHCI_USE_ADMA ) ? + (host->flags &SDHCI_USE_64_BIT_DMA ) ?"ADMA 64-bit" :"ADMA" : (host->flags &SDHCI_USE_SDMA ) ?"DMA" :"PIO" );sdhci_enable_card_detection (host);
drivers/mmc/host/sdhci.h +18 -0
@@ -227,6 +227,7 @@/* 55-57 reserved */ #define SDHCI_ADMA_ADDRESS 0x58 +#define SDHCI_ADMA_ADDRESS_HI 0x5C/* 60-FB reserved */
@@ -279,6 +280,23 @@ struct sdhci_adma2_32_desc {__le32 addr; } __packed__aligned(SDHCI_ADMA2_32_ALIGN) ; +/* ADMA2 64-bit DMA descriptor size */ +#define SDHCI_ADMA2_64_DESC_SZ 12 + +/* ADMA2 64-bit DMA alignment */ +#define SDHCI_ADMA2_64_ALIGN 8 + +/* +* ADMA2 64-bit descriptor. Note 12-byte descriptor can't always be 8-byte +* aligned. +*/ +struct sdhci_adma2_64_desc { +__le16 cmd; +__le16 len; +__le32 addr_lo; +__le32 addr_hi; +} __packed__aligned(4) ; +#define ADMA2_TRAN_VALID 0x21#define ADMA2_NOP_END_VALID 0x3#define ADMA2_END 0x2
include/linux/mmc/sdhci.h +3 -0
@@ -100,6 +100,8 @@ struct sdhci_host {#define SDHCI_QUIRK2_BROKEN_DDR50 (1<<7)/* Stop command (CMD12) can set Transfer Complete when not using MMC_RSP_BUSY */ #define SDHCI_QUIRK2_STOP_WITH_TC (1<<8) +/* Controller does not support 64-bit DMA */ +#define SDHCI_QUIRK2_BROKEN_64_BIT_DMA (1<<9)int irq;/* Device IRQ */ void __iomem *ioaddr;/* Mapped address */
@@ -130,6 +132,7 @@ struct sdhci_host {#define SDHCI_SDIO_IRQ_ENABLED (1<<9)/* SDIO irq enabled */ #define SDHCI_SDR104_NEEDS_TUNING (1<<10)/* SDR104/HS200 needs tuning */ #define SDHCI_USING_RETUNING_TIMER (1<<11)/* Host is using a retuning timer for the card */ +#define SDHCI_USE_64_BIT_DMA (1<<12)/* Use 64-bit DMA */ unsigned int version;/* SDHCI spec. version */
Signature · 1 line
1.9.1
Adrian Hunter
maintainerPATCH 13/15 mmc: sdhci-acpi: Add 64-bit DMA support
In reply to Adrian HunterSet the DMA mask during the first call to ->enable_dma() to make use of the SDHCI_USE_64_BIT_DMA flag.
--- drivers/mmc/host/sdhci-acpi.c | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-)
drivers/mmc/host/sdhci-acpi.c +25 -16
@@ -76,6 +76,7 @@ struct sdhci_acpi_host {const struct sdhci_acpi_slot *slot;struct platform_device *pdev;bool use_runtime_pm; +bool dma_setup; };static inline bool sdhci_acpi_flag (struct sdhci_acpi_host *c,unsigned int flag)
@@ -85,7 +86,30 @@ static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)static int sdhci_acpi_enable_dma (struct sdhci_host *host) { -return 0 ; +struct sdhci_acpi_host *c =sdhci_priv (host); +struct device *dev = &c->pdev->dev; +int err =-1 ; + +if (c->dma_setup) +return 0 ; + +if (host->flags &SDHCI_USE_64_BIT_DMA ) { +if (host->quirks2 &SDHCI_QUIRK2_BROKEN_64_BIT_DMA ) { + host->flags &= ~SDHCI_USE_64_BIT_DMA ; + }else { + err =dma_coerce_mask_and_coherent (dev, +DMA_BIT_MASK (64 )); +if (err) +dev_warn (dev,"Failed to set 64-bit DMA mask\n" ); + } + } + +if (err) + err =dma_coerce_mask_and_coherent (dev,DMA_BIT_MASK (32 )); + + c->dma_setup = !err; + +return err; }static void sdhci_acpi_int_hw_reset (struct sdhci_host *host)
@@ -305,21 +329,6 @@ static int sdhci_acpi_probe(struct platform_device *pdev) goto err_free; } -if (!dev->dma_mask) { -u64 dma_mask; - -if (sdhci_readl (host,SDHCI_CAPABILITIES ) &SDHCI_CAN_64BIT ) { -/* 64-bit DMA is not supported at present */ - dma_mask =DMA_BIT_MASK (32 ); - }else { - dma_mask =DMA_BIT_MASK (32 ); - } - - err =dma_coerce_mask_and_coherent (dev, dma_mask); -if (err) - goto err_free; - } -if (c->slot) {if (c->slot->probe_slot) { err = c->slot->probe_slot (pdev, hid, uid);
Signature · 1 line
1.9.1
Re: [PATCH 13/15] mmc: sdhci-acpi: Add 64-bit DMA support
Quoted 8 lines · On Tuesday 21 October 2014 12:26:23 Adrian Hunter wrote:
drivers/mmc/host/sdhci-acpi.c +7
-0+return 0 ; + +if (host->flags &SDHCI_USE_64_BIT_DMA ) { +if (host->quirks2 &SDHCI_QUIRK2_BROKEN_64_BIT_DMA ) { + host->flags &= ~SDHCI_USE_64_BIT_DMA ; + }else { + err = dma_coerce_mask_and_coherent(dev,
drivers/mmc/host/sdhci-acpi.c static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag) · line 110
Quoted 10 lines
drivers/mmc/host/sdhci-acpi.c +10
-0+ DMA_BIT_MASK(64)); +if (err) +dev_warn (dev,"Failed to set 64-bit DMA mask\n" ); + } + } + +if (err) + err =dma_coerce_mask_and_coherent (dev,DMA_BIT_MASK (32 )); + + c->dma_setup = !err;
Never use dma_coerce_mask_and_coherent(), it will ignore limitations of the upstream bus. Use dma_set_mask_and_coherent instead. If it fails, I think you need to clear the SDHCI_USE_64_BIT_DMA flag.
Arnd
Adrian Hunter
maintainerRe: [PATCH 13/15] mmc: sdhci-acpi: Add 64-bit DMA support
Quoted 15 lines · Arnd Bergmann
Quoted 14 lines · On Tuesday 21 October 2014 12:26:23 Adrian Hunter wrote:
drivers/mmc/host/sdhci-acpi.c +14
-0+return 0 ; + +if (host->flags &SDHCI_USE_64_BIT_DMA ) { +if (host->quirks2 &SDHCI_QUIRK2_BROKEN_64_BIT_DMA ) { + host->flags &= ~SDHCI_USE_64_BIT_DMA ; + }else { + err =dma_coerce_mask_and_coherent (dev, +DMA_BIT_MASK (64 )); +if (err) +dev_warn (dev,"Failed to set 64-bit DMA mask\n" ); + } + } + +if (err)
Quoted 8 lines
drivers/mmc/host/sdhci-acpi.c static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag) · line 110
Quoted 4 lines
drivers/mmc/host/sdhci-acpi.c +3
-0+ err =dma_coerce_mask_and_coherent (dev,DMA_BIT_MASK (32 )); + + c->dma_setup = !err;Never use
dma_coerce_mask_and_coherent(), it will ignore limitations of the upstream bus. Usedma_set_mask_and_coherentinstead. If it fails, I think you need to clear theSDHCI_USE_64_BIT_DMAflag.
The sdhci-acpi device is a platform devices created by acpi. dev->dma_mask is NULL so dma_set_mask_and_coherent will always fail
unless I add:
if (!dev->dma_mask) dev->dma_mask = &dev->coherent_dma_mask;
Re: [PATCH 13/15] mmc: sdhci-acpi: Add 64-bit DMA support
Quoted 25 lines · On Tuesday 21 October 2014 13:45:20 Adrian Hunter wrote:
Quoted 15 lines · Arnd Bergmann
Quoted 14 lines · On Tuesday 21 October 2014 12:26:23 Adrian Hunter wrote:
drivers/mmc/host/sdhci-acpi.c +14
-0+return 0 ; + +if (host->flags &SDHCI_USE_64_BIT_DMA ) { +if (host->quirks2 &SDHCI_QUIRK2_BROKEN_64_BIT_DMA ) { + host->flags &= ~SDHCI_USE_64_BIT_DMA ; + }else { + err =dma_coerce_mask_and_coherent (dev, +DMA_BIT_MASK (64 )); +if (err) +dev_warn (dev,"Failed to set 64-bit DMA mask\n" ); + } + } + +if (err)Quoted 8 lines
drivers/mmc/host/sdhci-acpi.c static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag) · line 110
Quoted 4 lines
drivers/mmc/host/sdhci-acpi.c +3
-0+ err =dma_coerce_mask_and_coherent (dev,DMA_BIT_MASK (32 )); + + c->dma_setup = !err;Never use
dma_coerce_mask_and_coherent(), it will ignore limitations of the upstream bus. Usedma_set_mask_and_coherentinstead. If it fails, I think you need to clear theSDHCI_USE_64_BIT_DMAflag.
Quoted 7 lines
The sdhci-acpi device is a platform devices created by acpi.
dev->dma_maskis NULL sodma_set_mask_and_coherentwill always fail
unless I add:if (!dev->dma_mask) dev->dma_mask = &dev->coherent_dma_mask;
That's a bug in the ACPI code. Fix that instead of working around it in random drivers.
Arnd
Adrian Hunter
maintainerPATCH v2 1/1 mmc: sdhci-acpi: Add 64-bit DMA support
Set the DMA mask during the first call to ->enable_dma() to make use of the SDHCI_USE_64_BIT_DMA flag.
This patch is dependent on
commit 8a2f38ddfeb526c30b3ec209468172a30a38d996 ("ACPI / platform: provide default DMA mask") which provides the dev->dma_mask pointer without
which dma_set_mask_and_coherent() will always fail.
--- drivers/mmc/host/sdhci-acpi.c | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-)
drivers/mmc/host/sdhci-acpi.c +24 -16
@@ -76,6 +76,7 @@ struct sdhci_acpi_host {const struct sdhci_acpi_slot *slot;struct platform_device *pdev;bool use_runtime_pm; +bool dma_setup; };static inline bool sdhci_acpi_flag (struct sdhci_acpi_host *c,unsigned int flag)
@@ -85,7 +86,29 @@ static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag)static int sdhci_acpi_enable_dma (struct sdhci_host *host) { -return 0 ; +struct sdhci_acpi_host *c =sdhci_priv (host); +struct device *dev = &c->pdev->dev; +int err =-1 ; + +if (c->dma_setup) +return 0 ; + +if (host->flags &SDHCI_USE_64_BIT_DMA ) { +if (host->quirks2 &SDHCI_QUIRK2_BROKEN_64_BIT_DMA ) { + host->flags &= ~SDHCI_USE_64_BIT_DMA ; + }else { + err =dma_set_mask_and_coherent (dev,DMA_BIT_MASK (64 )); +if (err) +dev_warn (dev,"Failed to set 64-bit DMA mask\n" ); + } + } + +if (err) + err =dma_set_mask_and_coherent (dev,DMA_BIT_MASK (32 )); + + c->dma_setup = !err; + +return err; }static void sdhci_acpi_int_hw_reset (struct sdhci_host *host)
@@ -305,21 +328,6 @@ static int sdhci_acpi_probe(struct platform_device *pdev) goto err_free; } -if (!dev->dma_mask) { -u64 dma_mask; - -if (sdhci_readl (host,SDHCI_CAPABILITIES ) &SDHCI_CAN_64BIT ) { -/* 64-bit DMA is not supported at present */ - dma_mask =DMA_BIT_MASK (32 ); - }else { - dma_mask =DMA_BIT_MASK (32 ); - } - - err =dma_coerce_mask_and_coherent (dev, dma_mask); -if (err) - goto err_free; - } -if (c->slot) {if (c->slot->probe_slot) { err = c->slot->probe_slot (pdev, hid, uid);
Signature · 1 line
1.9.1
Re: [PATCH V2] mmc: sdhci-acpi: Add 64-bit DMA support
Quoted 18 lines · On Tuesday 28 October 2014 10:37:20 Adrian Hunter wrote:
static int
sdhci_acpi_enable_dma(struct sdhci_host *host){drivers/mmc/host/sdhci-acpi.c +14
-1-return 0 ; +struct sdhci_acpi_host *c =sdhci_priv (host); +struct device *dev = &c->pdev->dev; +int err =-1 ; + +if (c->dma_setup) +return 0 ; + +if (host->flags &SDHCI_USE_64_BIT_DMA ) { +if (host->quirks2 &SDHCI_QUIRK2_BROKEN_64_BIT_DMA ) { + host->flags &= ~SDHCI_USE_64_BIT_DMA ; + }else { + err =dma_set_mask_and_coherent (dev,DMA_BIT_MASK (64 )); +if (err) +dev_warn (dev,"Failed to set 64-bit DMA mask\n" );
drivers/mmc/host/sdhci-acpi.c static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag) · line 112
Quoted 10 lines
drivers/mmc/host/sdhci-acpi.c +9
-0+ } + } + +if (err) + err =dma_set_mask_and_coherent (dev,DMA_BIT_MASK (32 )); + + c->dma_setup = !err; + +return err; }
I don't think it's worth a dev_warn() message (maybe dev_info), there is nothing wrong in this case, but you probably have to clear the SDHCI_USE_64_BIT_DMA to ensure that the driver won't try to to use the 64-bit DMA if the mask is not set.
Arnd
Adrian Hunter
maintainerRe: [PATCH V2] mmc: sdhci-acpi: Add 64-bit DMA support
Quoted 23 lines · Arnd Bergmann
Quoted 22 lines · On Tuesday 28 October 2014 10:37:20 Adrian Hunter wrote:
static int
sdhci_acpi_enable_dma(struct sdhci_host *host){drivers/mmc/host/sdhci-acpi.c +19
-1-return 0 ; +struct sdhci_acpi_host *c =sdhci_priv (host); +struct device *dev = &c->pdev->dev; +int err =-1 ; + +if (c->dma_setup) +return 0 ; + +if (host->flags &SDHCI_USE_64_BIT_DMA ) { +if (host->quirks2 &SDHCI_QUIRK2_BROKEN_64_BIT_DMA ) { + host->flags &= ~SDHCI_USE_64_BIT_DMA ; + }else { + err =dma_set_mask_and_coherent (dev,DMA_BIT_MASK (64 )); +if (err) +dev_warn (dev,"Failed to set 64-bit DMA mask\n" ); + } + } + +if (err) + err =dma_set_mask_and_coherent (dev,DMA_BIT_MASK (32 ));
Quoted 8 lines
drivers/mmc/host/sdhci-acpi.c static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag) · line 112
Quoted 6 lines
drivers/mmc/host/sdhci-acpi.c +4
-0+ + c->dma_setup = !err; + +return err; }I don't think it's worth a
dev_warn()message (maybedev_info), there is nothing
It is worth a dev_warn because 32-bit DMA can allocate memory for bounce buffers which jeopardizes memory reclaim.
Quoted 2 lines
wrong in this case, but you probably have to clear the
SDHCI_USE_64_BIT_DMAto ensure that the driver won't try to to use the 64-bit DMA if the mask is not set.
64-bit DMA will work with a 32-bit DMA mask so there is no need.
Re: [PATCH V2] mmc: sdhci-acpi: Add 64-bit DMA support
Quoted 33 lines · On Tuesday 28 October 2014 12:05:30 Adrian Hunter wrote:
Quoted 23 lines · Arnd Bergmann
Quoted 22 lines · On Tuesday 28 October 2014 10:37:20 Adrian Hunter wrote:
static int
sdhci_acpi_enable_dma(struct sdhci_host *host){drivers/mmc/host/sdhci-acpi.c +19
-1-return 0 ; +struct sdhci_acpi_host *c =sdhci_priv (host); +struct device *dev = &c->pdev->dev; +int err =-1 ; + +if (c->dma_setup) +return 0 ; + +if (host->flags &SDHCI_USE_64_BIT_DMA ) { +if (host->quirks2 &SDHCI_QUIRK2_BROKEN_64_BIT_DMA ) { + host->flags &= ~SDHCI_USE_64_BIT_DMA ; + }else { + err =dma_set_mask_and_coherent (dev,DMA_BIT_MASK (64 )); +if (err) +dev_warn (dev,"Failed to set 64-bit DMA mask\n" ); + } + } + +if (err) + err =dma_set_mask_and_coherent (dev,DMA_BIT_MASK (32 ));Quoted 8 lines
drivers/mmc/host/sdhci-acpi.c static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag) · line 112
Quoted 6 lines
drivers/mmc/host/sdhci-acpi.c +4
-0+ + c->dma_setup = !err; + +return err; }I don't think it's worth a
dev_warn()message (maybedev_info), there is nothing
Quoted 2 lines
It is worth a
dev_warnbecause 32-bit DMA can allocate memory for bounce buffers which jeopardizes memory reclaim.
Then you should also warn if SDHCI_USE_64_BIT_DMA isn't or if SDHCI_QUIRK2_BROKEN_64_BIT_DMA is set I guess.
Quoted 4 lines
Quoted 2 lines
wrong in this case, but you probably have to clear the
SDHCI_USE_64_BIT_DMAto ensure that the driver won't try to to use the 64-bit DMA if the mask is not set.64-bit DMA will work with a 32-bit DMA mask so there is no need.
Wouldn't that at least be (slightly) less efficient? You end up having to do two expensive MMIO register accesses each time you transfer an address.
Arnd
Adrian Hunter
maintainerRe: [PATCH V2] mmc: sdhci-acpi: Add 64-bit DMA support
Quoted 34 lines · Arnd Bergmann
Quoted 33 lines · On Tuesday 28 October 2014 12:05:30 Adrian Hunter wrote:
Quoted 31 lines · Arnd Bergmann
Quoted 18 lines · On Tuesday 28 October 2014 10:37:20 Adrian Hunter wrote:
static int
sdhci_acpi_enable_dma(struct sdhci_host *host){drivers/mmc/host/sdhci-acpi.c +14
-1-return 0 ; +struct sdhci_acpi_host *c =sdhci_priv (host); +struct device *dev = &c->pdev->dev; +int err =-1 ; + +if (c->dma_setup) +return 0 ; + +if (host->flags &SDHCI_USE_64_BIT_DMA ) { +if (host->quirks2 &SDHCI_QUIRK2_BROKEN_64_BIT_DMA ) { + host->flags &= ~SDHCI_USE_64_BIT_DMA ; + }else { + err =dma_set_mask_and_coherent (dev,DMA_BIT_MASK (64 )); +if (err) +dev_warn (dev,"Failed to set 64-bit DMA mask\n" );drivers/mmc/host/sdhci-acpi.c static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag) · line 112
Quoted 10 lines
drivers/mmc/host/sdhci-acpi.c +9
-0+ } + } + +if (err) + err =dma_set_mask_and_coherent (dev,DMA_BIT_MASK (32 )); + + c->dma_setup = !err; + +return err; }I don't think it's worth a
dev_warn()message (maybedev_info), there is nothing
Quoted 5 lines
Quoted 2 lines
It is worth a
dev_warnbecause 32-bit DMA can allocate memory for bounce buffers which jeopardizes memory reclaim.Then you should also warn if
SDHCI_USE_64_BIT_DMAisn't or ifSDHCI_QUIRK2_BROKEN_64_BIT_DMAis set I guess.
The warning is for when the controller supports 64-bit, not when it doesn't.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
wrong in this case, but you probably have to clear the
SDHCI_USE_64_BIT_DMAto ensure that the driver won't try to to use the 64-bit DMA if the mask is not set.64-bit DMA will work with a 32-bit DMA mask so there is no need.
Wouldn't that at least be (slightly) less efficient? You end up having to do two expensive MMIO register accesses each time you transfer an address.
I doubt it is measureable and I don't want to optimize a degenerate case anyway.
Re: [PATCH V2] mmc: sdhci-acpi: Add 64-bit DMA support
Quoted 35 lines · On Tuesday 28 October 2014 13:41:30 Adrian Hunter wrote:
Quoted 34 lines · Arnd Bergmann
Quoted 33 lines · On Tuesday 28 October 2014 12:05:30 Adrian Hunter wrote:
Quoted 31 lines · Arnd Bergmann
Quoted 18 lines · On Tuesday 28 October 2014 10:37:20 Adrian Hunter wrote:
static int
sdhci_acpi_enable_dma(struct sdhci_host *host){drivers/mmc/host/sdhci-acpi.c +14
-1-return 0 ; +struct sdhci_acpi_host *c =sdhci_priv (host); +struct device *dev = &c->pdev->dev; +int err =-1 ; + +if (c->dma_setup) +return 0 ; + +if (host->flags &SDHCI_USE_64_BIT_DMA ) { +if (host->quirks2 &SDHCI_QUIRK2_BROKEN_64_BIT_DMA ) { + host->flags &= ~SDHCI_USE_64_BIT_DMA ; + }else { + err =dma_set_mask_and_coherent (dev,DMA_BIT_MASK (64 )); +if (err) +dev_warn (dev,"Failed to set 64-bit DMA mask\n" );drivers/mmc/host/sdhci-acpi.c static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag) · line 112
Quoted 10 lines
drivers/mmc/host/sdhci-acpi.c +9
-0+ } + } + +if (err) + err =dma_set_mask_and_coherent (dev,DMA_BIT_MASK (32 )); + + c->dma_setup = !err; + +return err; }I don't think it's worth a
dev_warn()message (maybedev_info), there is nothing
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
It is worth a
dev_warnbecause 32-bit DMA can allocate memory for bounce buffers which jeopardizes memory reclaim.Then you should also warn if
SDHCI_USE_64_BIT_DMAisn't or ifSDHCI_QUIRK2_BROKEN_64_BIT_DMAis set I guess.The warning is for when the controller supports 64-bit, not when it doesn't.
But why warn about a feature of the controller being present? You just said it's a problem for memory reclaim if 64-bit DMA is not supported.
Arnd
Adrian Hunter
maintainerRe: [PATCH V2] mmc: sdhci-acpi: Add 64-bit DMA support
Quoted 39 lines · Arnd Bergmann
Quoted 38 lines · On Tuesday 28 October 2014 13:41:30 Adrian Hunter wrote:
Quoted 37 lines · Arnd Bergmann
Quoted 35 lines · On Tuesday 28 October 2014 12:05:30 Adrian Hunter wrote:
Quoted 23 lines · Arnd Bergmann
Quoted 22 lines · On Tuesday 28 October 2014 10:37:20 Adrian Hunter wrote:
static int sdhci_acpi_enable_dma(struct sdhci_host *host) {drivers/mmc/host/sdhci-acpi.c +19
-1-return 0 ; +struct sdhci_acpi_host *c =sdhci_priv (host); +struct device *dev = &c->pdev->dev; +int err =-1 ; + +if (c->dma_setup) +return 0 ; + +if (host->flags &SDHCI_USE_64_BIT_DMA ) { +if (host->quirks2 &SDHCI_QUIRK2_BROKEN_64_BIT_DMA ) { + host->flags &= ~SDHCI_USE_64_BIT_DMA ; + }else { + err =dma_set_mask_and_coherent (dev,DMA_BIT_MASK (64 )); +if (err) +dev_warn (dev,"Failed to set 64-bit DMA mask\n" ); + } + } + +if (err) + err =dma_set_mask_and_coherent (dev,DMA_BIT_MASK (32 ));Quoted 8 lines
drivers/mmc/host/sdhci-acpi.c static inline bool sdhci_acpi_flag(struct sdhci_acpi_host *c, unsigned int flag) · line 112
Quoted 6 lines
drivers/mmc/host/sdhci-acpi.c +4
-0+ + c->dma_setup = !err; + +return err; }I don't think it's worth a
dev_warn()message (maybedev_info), there is nothingIt is worth a
dev_warnbecause 32-bit DMA can allocate memory for bounce buffers which jeopardizes memory reclaim.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Then you should also warn if
SDHCI_USE_64_BIT_DMAisn't or ifSDHCI_QUIRK2_BROKEN_64_BIT_DMAis set I guess.The warning is for when the controller supports 64-bit, not when it doesn't.
But why warn about a feature of the controller being present? You just said it's a problem for memory reclaim if 64-bit DMA is not supported.
The warning is for when the controller supports 64-bit but it can't get a 64-bit DMA mask, and might therefore need to bounce things.
Re: [PATCH V2] mmc: sdhci-acpi: Add 64-bit DMA support
Quoted 43 lines · On Tuesday 28 October 2014 16:14:46 Adrian Hunter wrote:
Quoted 42 lines · Arnd Bergmann
Quoted 41 lines · On Tuesday 28 October 2014 13:41:30 Adrian Hunter wrote:
Quoted 39 lines · Arnd Bergmann
Quoted 33 lines · On Tuesday 28 October 2014 12:05:30 Adrian Hunter wrote:
Quoted 23 lines · Arnd Bergmann
Quoted 22 lines · On Tuesday 28 October 2014 10:37:20 Adrian Hunter wrote:
static int sdhci_acpi_enable_dma(struct sdhci_host *host) {(quoted diff) +19
-1- return 0; + struct sdhci_acpi_host *c = sdhci_priv(host); + struct device *dev = &c->pdev->dev; + int err = -1; + + if (c->dma_setup) + return 0; + + if (host->flags & SDHCI_USE_64_BIT_DMA) { + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA) { + host->flags &= ~SDHCI_USE_64_BIT_DMA; + } else { + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); + if (err) + dev_warn(dev, "Failed to set 64-bit DMA mask\n"); + } + } + + if (err) + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +4
-0+ + c->dma_setup = !err; + + return err; }I don't think it's worth a
dev_warn()message (maybedev_info), there is nothingQuoted 2 lines
It is worth a
dev_warnbecause 32-bit DMA can allocate memory for bounce buffers which jeopardizes memory reclaim.Then you should also warn if
SDHCI_USE_64_BIT_DMAisn't or ifSDHCI_QUIRK2_BROKEN_64_BIT_DMAis set I guess.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
The warning is for when the controller supports 64-bit, not when it doesn't.
But why warn about a feature of the controller being present? You just said it's a problem for memory reclaim if 64-bit DMA is not supported.
The warning is for when the controller supports 64-bit but it can't get a 64-bit DMA mask, and might therefore need to bounce things.
What does "can't get a 64-bit DMA mask" mean? This is just a different way to say it doesn't support 64-bit for some reason.
Arnd
Adrian Hunter
maintainerRe: [PATCH V2] mmc: sdhci-acpi: Add 64-bit DMA support
Quoted 46 lines · Arnd Bergmann
Quoted 45 lines · On Tuesday 28 October 2014 16:14:46 Adrian Hunter wrote:
Quoted 44 lines · Arnd Bergmann
Quoted 42 lines · On Tuesday 28 October 2014 13:41:30 Adrian Hunter wrote:
Quoted 34 lines · Arnd Bergmann
Quoted 33 lines · On Tuesday 28 October 2014 12:05:30 Adrian Hunter wrote:
Quoted 31 lines · Arnd Bergmann
Quoted 18 lines · On Tuesday 28 October 2014 10:37:20 Adrian Hunter wrote:
static int sdhci_acpi_enable_dma(struct sdhci_host *host) {(quoted diff) +14
-1- return 0; + struct sdhci_acpi_host *c = sdhci_priv(host); + struct device *dev = &c->pdev->dev; + int err = -1; + + if (c->dma_setup) + return 0; + + if (host->flags & SDHCI_USE_64_BIT_DMA) { + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA) { + host->flags &= ~SDHCI_USE_64_BIT_DMA; + } else { + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); + if (err) + dev_warn(dev, "Failed to set 64-bit DMA mask\n");Review
Quoted 10 lines
(quoted diff) +9
-0+ } + } + + if (err) + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); + + c->dma_setup = !err; + + return err; }I don't think it's worth a
dev_warn()message (maybedev_info), there is nothingQuoted 5 lines
Quoted 2 lines
It is worth a
dev_warnbecause 32-bit DMA can allocate memory for bounce buffers which jeopardizes memory reclaim.Then you should also warn if
SDHCI_USE_64_BIT_DMAisn't or ifSDHCI_QUIRK2_BROKEN_64_BIT_DMAis set I guess.The warning is for when the controller supports 64-bit, not when it doesn't.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
But why warn about a feature of the controller being present? You just said it's a problem for memory reclaim if 64-bit DMA is not supported.
The warning is for when the controller supports 64-bit but it can't get a 64-bit DMA mask, and might therefore need to bounce things.
What does "can't get a 64-bit DMA mask" mean? This is just a different way to say it doesn't support 64-bit for some reason.
The host controller advertises whether it is capable of 64-bit DMA. If it is 64-bit capable but the driver cannot get a 64-bit DMA mask it issues a warning.
Re: [PATCH V2] mmc: sdhci-acpi: Add 64-bit DMA support
Quoted 53 lines · On Tuesday 28 October 2014 17:14:25 Adrian Hunter wrote:
Quoted 52 lines · Arnd Bergmann
Quoted 50 lines · On Tuesday 28 October 2014 16:14:46 Adrian Hunter wrote:
Quoted 39 lines · Arnd Bergmann
Quoted 38 lines · On Tuesday 28 October 2014 13:41:30 Adrian Hunter wrote:
Quoted 37 lines · Arnd Bergmann
Quoted 35 lines · On Tuesday 28 October 2014 12:05:30 Adrian Hunter wrote:
Quoted 23 lines · Arnd Bergmann
Quoted 22 lines · On Tuesday 28 October 2014 10:37:20 Adrian Hunter wrote:
static int sdhci_acpi_enable_dma(struct sdhci_host *host) {(quoted diff) +19
-1- return 0; + struct sdhci_acpi_host *c = sdhci_priv(host); + struct device *dev = &c->pdev->dev; + int err = -1; + + if (c->dma_setup) + return 0; + + if (host->flags & SDHCI_USE_64_BIT_DMA) { + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA) { + host->flags &= ~SDHCI_USE_64_BIT_DMA; + } else { + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); + if (err) + dev_warn(dev, "Failed to set 64-bit DMA mask\n"); + } + } + + if (err) + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +4
-0+ + c->dma_setup = !err; + + return err; }I don't think it's worth a
dev_warn()message (maybedev_info), there is nothingIt is worth a
dev_warnbecause 32-bit DMA can allocate memory for bounce buffers which jeopardizes memory reclaim.Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Then you should also warn if
SDHCI_USE_64_BIT_DMAisn't or ifSDHCI_QUIRK2_BROKEN_64_BIT_DMAis set I guess.The warning is for when the controller supports 64-bit, not when it doesn't.
But why warn about a feature of the controller being present? You just said it's a problem for memory reclaim if 64-bit DMA is not supported.
The warning is for when the controller supports 64-bit but it can't get a 64-bit DMA mask, and might therefore need to bounce things.
Quoted 6 lines
Quoted 2 lines
What does "can't get a 64-bit DMA mask" mean? This is just a different way to say it doesn't support 64-bit for some reason.
The host controller advertises whether it is capable of 64-bit DMA. If it is 64-bit capable but the driver cannot get a 64-bit DMA mask it issues a warning.
But the host controller doesn't know if it's 64-bit capable, nor should it know. All the host controller knows is that it has registers to perform 64-bit DMA, but the platform code (ACPI in your case) is the only thing that knows how many of those address lines are connected to an upstream bus.
Arnd
Adrian Hunter
maintainerRe: [PATCH V2] mmc: sdhci-acpi: Add 64-bit DMA support
Quoted 57 lines · Arnd Bergmann
Quoted 56 lines · On Tuesday 28 October 2014 17:14:25 Adrian Hunter wrote:
Quoted 54 lines · Arnd Bergmann
Quoted 43 lines · On Tuesday 28 October 2014 16:14:46 Adrian Hunter wrote:
Quoted 42 lines · Arnd Bergmann
Quoted 41 lines · On Tuesday 28 October 2014 13:41:30 Adrian Hunter wrote:
Quoted 39 lines · Arnd Bergmann
Quoted 33 lines · On Tuesday 28 October 2014 12:05:30 Adrian Hunter wrote:
Quoted 23 lines · Arnd Bergmann
Quoted 22 lines · On Tuesday 28 October 2014 10:37:20 Adrian Hunter wrote:
static int sdhci_acpi_enable_dma(struct sdhci_host *host) {(quoted diff) +19
-1- return 0; + struct sdhci_acpi_host *c = sdhci_priv(host); + struct device *dev = &c->pdev->dev; + int err = -1; + + if (c->dma_setup) + return 0; + + if (host->flags & SDHCI_USE_64_BIT_DMA) { + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_64_BIT_DMA) { + host->flags &= ~SDHCI_USE_64_BIT_DMA; + } else { + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); + if (err) + dev_warn(dev, "Failed to set 64-bit DMA mask\n"); + } + } + + if (err) + err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +4
-0+ + c->dma_setup = !err; + + return err; }I don't think it's worth a
dev_warn()message (maybedev_info), there is nothingQuoted 2 lines
It is worth a
dev_warnbecause 32-bit DMA can allocate memory for bounce buffers which jeopardizes memory reclaim.Then you should also warn if
SDHCI_USE_64_BIT_DMAisn't or ifSDHCI_QUIRK2_BROKEN_64_BIT_DMAis set I guess.Quoted 7 lines
Quoted 4 lines
Quoted 1 line
The warning is for when the controller supports 64-bit, not when it doesn't.
But why warn about a feature of the controller being present? You just said it's a problem for memory reclaim if 64-bit DMA is not supported.
The warning is for when the controller supports 64-bit but it can't get a 64-bit DMA mask, and might therefore need to bounce things.
What does "can't get a 64-bit DMA mask" mean? This is just a different way to say it doesn't support 64-bit for some reason.
Quoted 8 lines
Quoted 3 lines
The host controller advertises whether it is capable of 64-bit DMA. If it is 64-bit capable but the driver cannot get a 64-bit DMA mask it issues a warning.
But the host controller doesn't know if it's 64-bit capable, nor should it know. All the host controller knows is that it has registers to perform 64-bit DMA, but the platform code (ACPI in your case) is the only thing that knows how many of those address lines are connected to an upstream bus.
No, the SDHCI spec. says:
64-bit System Bus Support
Setting 1 to this bit indicates that the Host Controller supports 64-bit address descriptor mode and is connected to 64-bit address system bus.
So the expectation is that it is on a 64-bit bus. The warning is reasonable.
Adrian Hunter
maintainerPATCH 14/15 mmc: sdhci-pci: Add 64-bit DMA support
In reply to Adrian HunterSet a 64-bit DMA mask when using 64-bit DMA.
--- drivers/mmc/host/sdhci-pci.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
drivers/mmc/host/sdhci-pci.c +12 -2
@@ -1064,7 +1064,7 @@ static int sdhci_pci_enable_dma(struct sdhci_host *host) {struct sdhci_pci_slot *slot;struct pci_dev *pdev; -int ret; +int ret =-1 ; slot =sdhci_priv (host); pdev = slot->chip->pdev;
@@ -1076,7 +1076,17 @@ static int sdhci_pci_enable_dma(struct sdhci_host *host) "doesn't fully claim to support it.\n"); } - ret =pci_set_dma_mask (pdev,DMA_BIT_MASK (32 )); +if (host->flags &SDHCI_USE_64_BIT_DMA ) { +if (host->quirks2 &SDHCI_QUIRK2_BROKEN_64_BIT_DMA ) { + host->flags &= ~SDHCI_USE_64_BIT_DMA ; + }else { + ret =pci_set_dma_mask (pdev,DMA_BIT_MASK (64 )); +if (ret) +dev_warn (&pdev->dev,"Failed to set 64-bit DMA mask\n" ); + } + } +if (ret) + ret =pci_set_dma_mask (pdev,DMA_BIT_MASK (32 ));if (ret)return ret;
Signature · 1 line
1.9.1
Adrian Hunter
maintainerPATCH 15/15 mmc: mmc_test: Extend "Badly aligned" tests for 8-byte alignment
In reply to Adrian HunterThe "Badly aligned" tests, test reading/writing with alignments of 1,2 and 3. SDHCI now has 64-bit ADMA which has 8-byte alignment, so extend the tests to test up to 7.
--- drivers/mmc/card/mmc_test.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/mmc/card/mmc_test.c +6 -4
@@ -32,6 +32,8 @@#define BUFFER_ORDER 2#define BUFFER_SIZE (PAGE_SIZE << BUFFER_ORDER) +#define TEST_ALIGN_END 8 +/* * Limit the test area size to the maximum MMC HC erase group size. Note that * the maximum SD allocation unit size is just 4MiB.
@@ -1174,7 +1176,7 @@ static int mmc_test_align_write(struct mmc_test_card *test)int ret, i;struct scatterlist sg; -for (i =1 ;i <4 ;i++) { +for (i =1 ; i <TEST_ALIGN_END ; i++) {sg_init_one (&sg, test->buffer + i,512 ); ret =mmc_test_transfer (test, &sg,1 ,0 ,1 ,512 ,1 );if (ret)
@@ -1189,7 +1191,7 @@ static int mmc_test_align_read(struct mmc_test_card *test)int ret, i;struct scatterlist sg; -for (i =1 ;i <4 ;i++) { +for (i =1 ; i <TEST_ALIGN_END ; i++) {sg_init_one (&sg, test->buffer + i,512 ); ret =mmc_test_transfer (test, &sg,1 ,0 ,1 ,512 ,0 );if (ret)
@@ -1216,7 +1218,7 @@ static int mmc_test_align_multi_write(struct mmc_test_card *test)if (size <1024 )return RESULT_UNSUP_HOST ; -for (i =1 ;i <4 ;i++) { +for (i =1 ; i <TEST_ALIGN_END ; i++) {sg_init_one (&sg, test->buffer + i, size); ret =mmc_test_transfer (test, &sg,1 ,0 , size/512 ,512 ,1 );if (ret)
@@ -1243,7 +1245,7 @@ static int mmc_test_align_multi_read(struct mmc_test_card *test)if (size <1024 )return RESULT_UNSUP_HOST ; -for (i =1 ;i <4 ;i++) { +for (i =1 ; i <TEST_ALIGN_END ; i++) {sg_init_one (&sg, test->buffer + i, size); ret =mmc_test_transfer (test, &sg,1 ,0 , size/512 ,512 ,0 );if (ret)
Signature · 1 line
1.9.1
Adrian Hunter
maintainerQuoted 7 lines · Adrian Hunter
Hi
Here are patches to add 64-bit ADMA support to the SDHCI driver.
The patchset starts with 3 minor fixes related to SDHCI ADMA, then there are 8 preparatory patches, then 3 main patches, then the
mmc_test"Badly aligned" tests are extended slightly.
Hi Ulf
Can you take these? Note that there was a V2 of "mmc: sdhci-acpi: Add 64-bit DMA support". Also that patch is dependent (for functionality not compilation) on a patch in v3.18-rc2 so it is slightly preferable if you pull v3.18-rc2 first.
Regards
Adrian
Quoted 37 lines
Adrian Hunter (15):
mmc: sdhci: Fix incorrect ADMA2 descriptor table size
mmc: sdhci: Fix ADMA page boundary warnings
mmc: sdhci: Fix ADMA table size warning
mmc: sdhci: Rename two ADMA-related functions for consistency
mmc: sdhci: Renameadma_desctoadma_table
mmc: sdhci: Addsdhci_adma_mark_end()
mmc: sdhci: Use 'void *' for not 'u8 *' for ADMA data
mmc: sdhci: Parameterize ADMA sizes and alignment
mmc: sdhci: Define maximum segments
mmc: sdhci: Define ADMA constants
mmc: sdhci: Define ADMA descriptor structure
mmc: sdhci: Add 64-bit ADMA support
mmc: sdhci-acpi: Add 64-bit DMA support
mmc: sdhci-pci: Add 64-bit DMA support
mmc:mmc_test: Extend "Badly aligned" tests for 8-byte alignmentdrivers/mmc/card/mmc_test.c | 10 +- drivers/mmc/host/sdhci-acpi.c | 41 +++++--- drivers/mmc/host/sdhci-pci.c | 14 ++- drivers/mmc/host/sdhci.c | 226 +++++++++++++++++++++++++++--------------- drivers/mmc/host/sdhci.h | 41 ++++++++ include/linux/mmc/sdhci.h | 14 ++- 6 files changed, 241 insertions(+), 105 deletions(-)Regards
Adrian
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 9 lines · On Thursday 30 October 2014 09:25:54 Adrian Hunter wrote:
Quoted 7 lines · Adrian Hunter
Hi
Here are patches to add 64-bit ADMA support to the SDHCI driver.
The patchset starts with 3 minor fixes related to SDHCI ADMA, then there are 8 preparatory patches, then 3 main patches, then the
mmc_test"Badly aligned" tests are extended slightly.
Quoted 7 lines
Hi Ulf
Can you take these? Note that there was a V2 of "mmc: sdhci-acpi: Add 64-bit DMA support". Also that patch is dependent (for functionality not compilation) on a patch in v3.18-rc2 so it is slightly preferable if you pull v3.18-rc2 first.
You still haven't addressed my comments about clearing the SDHCI_USE_64_BIT_DMA flag if the platform finds that hardware has set this bit incorrectly.
Arnd
Adrian Hunter
maintainerQuoted 18 lines · Arnd Bergmann
Quoted 9 lines · On Thursday 30 October 2014 09:25:54 Adrian Hunter wrote:
Quoted 7 lines · Adrian Hunter
Hi
Here are patches to add 64-bit ADMA support to the SDHCI driver.
The patchset starts with 3 minor fixes related to SDHCI ADMA, then there are 8 preparatory patches, then 3 main patches, then the
mmc_test"Badly aligned" tests are extended slightly.Quoted 7 lines
Hi Ulf
Can you take these? Note that there was a V2 of "mmc: sdhci-acpi: Add 64-bit DMA support". Also that patch is dependent (for functionality not compilation) on a patch in v3.18-rc2 so it is slightly preferable if you pull v3.18-rc2 first.
Quoted 3 lines
You still haven't addressed my comments about clearing the
SDHCI_USE_64_BIT_DMAflag if the platform finds that hardware has set this bit incorrectly.
Yes I did. I said there was no need:
http://marc.info/?l=linux-mmc&m=141449082331402&w=2
I would also note that the SDHCI spec does not say explicitly that a 64-bit device supports 32-bit DMA descriptors.
If the hardware really is broken, the SDHCI_QUIRK2_BROKEN_64_BIT_DMA should be used.
Quoted 25 lines · On Thursday 30 October 2014 10:40:12 Adrian Hunter wrote:
Quoted 18 lines · Arnd Bergmann
Quoted 9 lines · On Thursday 30 October 2014 09:25:54 Adrian Hunter wrote:
Quoted 7 lines · Adrian Hunter
Hi
Here are patches to add 64-bit ADMA support to the SDHCI driver.
The patchset starts with 3 minor fixes related to SDHCI ADMA, then there are 8 preparatory patches, then 3 main patches, then the
mmc_test"Badly aligned" tests are extended slightly.Quoted 7 lines
Hi Ulf
Can you take these? Note that there was a V2 of "mmc: sdhci-acpi: Add 64-bit DMA support". Also that patch is dependent (for functionality not compilation) on a patch in v3.18-rc2 so it is slightly preferable if you pull v3.18-rc2 first.
Quoted 3 lines
You still haven't addressed my comments about clearing the
SDHCI_USE_64_BIT_DMAflag if the platform finds that hardware has set this bit incorrectly.Yes I did. I said there was no need:
Quoted 7 lines
http://marc.info/?l=linux-mmc&m=141449082331402&w=2I would also note that the SDHCI spec does not say explicitly that a 64-bit device supports 32-bit DMA descriptors.
If the hardware really is broken, the
SDHCI_QUIRK2_BROKEN_64_BIT_DMAshould be used.
You also said that the flag is defined to mean that 64-bit DMA is working and that you want to show the warning when the hardware and the firmware disagree about this. If you have an (at least) 50% chance that the hardware is lying, you really shouldn't believe it.
Arnd
Adrian Hunter
maintainerQuoted 31 lines · Arnd Bergmann
Quoted 30 lines · On Thursday 30 October 2014 10:40:12 Adrian Hunter wrote:
Quoted 18 lines · Arnd Bergmann
Quoted 9 lines · On Thursday 30 October 2014 09:25:54 Adrian Hunter wrote:
Quoted 7 lines · Adrian Hunter
Hi
Here are patches to add 64-bit ADMA support to the SDHCI driver.
The patchset starts with 3 minor fixes related to SDHCI ADMA, then there are 8 preparatory patches, then 3 main patches, then the
mmc_test"Badly aligned" tests are extended slightly.Quoted 7 lines
Hi Ulf
Can you take these? Note that there was a V2 of "mmc: sdhci-acpi: Add 64-bit DMA support". Also that patch is dependent (for functionality not compilation) on a patch in v3.18-rc2 so it is slightly preferable if you pull v3.18-rc2 first.
Quoted 3 lines
You still haven't addressed my comments about clearing the
SDHCI_USE_64_BIT_DMAflag if the platform finds that hardware has set this bit incorrectly.Yes I did. I said there was no need:
http://marc.info/?l=linux-mmc&m=141449082331402&w=2I would also note that the SDHCI spec does not say explicitly that a 64-bit device supports 32-bit DMA descriptors.
Quoted 8 lines
Quoted 2 lines
If the hardware really is broken, the
SDHCI_QUIRK2_BROKEN_64_BIT_DMAshould be used.You also said that the flag is defined to mean that 64-bit DMA is working and that you want to show the warning when the hardware and the firmware disagree about this. If you have an (at least) 50% chance that the hardware is lying, you really shouldn't believe it.
Just because there are two options does not mean they are each equally likely.
Quoted 11 lines · On Thursday 30 October 2014 12:50:59 Adrian Hunter wrote:
Quoted 6 lines · Arnd Bergmann
You also said that the flag is defined to mean that 64-bit DMA is working and that you want to show the warning when the hardware and the firmware disagree about this. If you have an (at least) 50% chance that the hardware is lying, you really shouldn't believe it.
Just because there are two options does not mean they are each equally likely.
Yes, that's why I said "at least". Presumably when the hardware and firmware disagree about something, it's because the firmware developer found a problem with the hardware and wants to work around that.
Arnd
Adrian Hunter
maintainerQuoted 9 lines · Arnd Bergmann
Quoted 8 lines · On Thursday 30 October 2014 12:50:59 Adrian Hunter wrote:
Quoted 6 lines · Arnd Bergmann
You also said that the flag is defined to mean that 64-bit DMA is working and that you want to show the warning when the hardware and the firmware disagree about this. If you have an (at least) 50% chance that the hardware is lying, you really shouldn't believe it.
Quoted 7 lines
Quoted 3 lines
Just because there are two options does not mean they are each equally likely.
Yes, that's why I said "at least". Presumably when the hardware and firmware disagree about something, it's because the firmware developer found a problem with the hardware and wants to work around that.
No, the capability bit could be set by firmware and the DMA restriction could be broken software, which would make the situation quite different.
sdhci.c will also accept the capabilities bits coming from another source.
Nevertheless, the programming is for non-broken hardware and the SDHCI_QUIRK2_BROKEN_64_BIT_DMA will suffice otherwise.
Adrian Hunter
maintainerQuoted 9 lines · Adrian Hunter
Quoted 7 lines · Adrian Hunter
Hi
Here are patches to add 64-bit ADMA support to the SDHCI driver.
The patchset starts with 3 minor fixes related to SDHCI ADMA, then there are 8 preparatory patches, then 3 main patches, then the
mmc_test"Badly aligned" tests are extended slightly.
Quoted 7 lines
Hi Ulf
Can you take these? Note that there was a V2 of "mmc: sdhci-acpi: Add 64-bit DMA support". Also that patch is dependent (for functionality not compilation) on a patch in v3.18-rc2 so it is slightly preferable if you pull v3.18-rc2 first.
Can you take these now?
Quoted 44 lines
Regards
AdrianQuoted 37 lines
Adrian Hunter (15):
mmc: sdhci: Fix incorrect ADMA2 descriptor table size
mmc: sdhci: Fix ADMA page boundary warnings
mmc: sdhci: Fix ADMA table size warning
mmc: sdhci: Rename two ADMA-related functions for consistency
mmc: sdhci: Renameadma_desctoadma_table
mmc: sdhci: Addsdhci_adma_mark_end()
mmc: sdhci: Use 'void *' for not 'u8 *' for ADMA data
mmc: sdhci: Parameterize ADMA sizes and alignment
mmc: sdhci: Define maximum segments
mmc: sdhci: Define ADMA constants
mmc: sdhci: Define ADMA descriptor structure
mmc: sdhci: Add 64-bit ADMA support
mmc: sdhci-acpi: Add 64-bit DMA support
mmc: sdhci-pci: Add 64-bit DMA support
mmc:mmc_test: Extend "Badly aligned" tests for 8-byte alignmentdrivers/mmc/card/mmc_test.c | 10 +- drivers/mmc/host/sdhci-acpi.c | 41 +++++--- drivers/mmc/host/sdhci-pci.c | 14 ++- drivers/mmc/host/sdhci.c | 226 +++++++++++++++++++++++++++--------------- drivers/mmc/host/sdhci.h | 41 ++++++++ include/linux/mmc/sdhci.h | 14 ++- 6 files changed, 241 insertions(+), 105 deletions(-)Regards
Adrian
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Ulf Hansson
AppliedQuoted 12 lines · Adrian Hunter
Quoted 11 lines · Adrian Hunter
Quoted 7 lines · Adrian Hunter
Hi
Here are patches to add 64-bit ADMA support to the SDHCI driver.
The patchset starts with 3 minor fixes related to SDHCI ADMA, then there are 8 preparatory patches, then 3 main patches, then the
mmc_test"Badly aligned" tests are extended slightly.Hi Ulf
Quoted 7 lines
Quoted 5 lines
Can you take these? Note that there was a V2 of "mmc: sdhci-acpi: Add 64-bit DMA support". Also that patch is dependent (for functionality not compilation) on a patch in v3.18-rc2 so it is slightly preferable if you pull v3.18-rc2 first.
Can you take these now?
Thanks, applied for next!
If Arnd have some more thoughts or would like to provide his reviewed by I may update/drop them.
Kind regards
Uffe