* [PATCH 0/2] dw_mmc: fix DW MMC cores with 32-bit bus on 64-bit Linux systems @ 2023-03-11 15:23 Sergey Lisov 2023-03-11 15:22 ` [PATCH 1/2] devicetree: synopsys-dw-mshc-common: add "fifo-access-32bit" property Sergey Lisov 2023-03-11 15:22 ` [PATCH 2/2] dw_mmc: add an option to force 32-bit accesses to 64-bit device registers Sergey Lisov 0 siblings, 2 replies; 10+ messages in thread From: Sergey Lisov @ 2023-03-11 15:23 UTC (permalink / raw) To: Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Jaehoon Chung Cc: linux-mmc, devicetree, linux-kernel DesignWare MMC cores have a configurable data bus width of either 16, 32, or 64 bytes. It is possible, and some vendors actually do it, to ship a DW MMC core configured for 32-bit data bus within a 64-bit SoC. In this case the kernel will attempt 64-bit (readq) accesses to certain 64-bit MMIO registers, while the core will expect pairs of 32-bit accesses. It seems that currently the only register for which the kernel performs 64-bit accesses is the FIFO. The symptom is that the DW MMC core never receives a read on the second half of the register, does not register the datum as being read, and thus not advancing its internal FIFO pointer, breaking further reads. It also seems that this FIFO is only used for small (less than 16 bytes) transfers, which probably means that only some SDIO cards are affected. Sergey Lisov (2): devicetree: synopsys-dw-mshc-common: add "fifo-access-32bit" property dw_mmc: add an option to force 32-bit accesses to 64-bit device registers .../bindings/mmc/synopsys-dw-mshc-common.yaml | 6 + drivers/mmc/host/dw_mmc.c | 125 +++++++++++++++++- drivers/mmc/host/dw_mmc.h | 2 + 3 files changed, 131 insertions(+), 2 deletions(-) -- 2.38.3 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] devicetree: synopsys-dw-mshc-common: add "fifo-access-32bit" property 2023-03-11 15:23 [PATCH 0/2] dw_mmc: fix DW MMC cores with 32-bit bus on 64-bit Linux systems Sergey Lisov @ 2023-03-11 15:22 ` Sergey Lisov 2023-03-11 15:50 ` Krzysztof Kozlowski 2023-03-11 15:22 ` [PATCH 2/2] dw_mmc: add an option to force 32-bit accesses to 64-bit device registers Sergey Lisov 1 sibling, 1 reply; 10+ messages in thread From: Sergey Lisov @ 2023-03-11 15:22 UTC (permalink / raw) To: Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Jaehoon Chung Cc: linux-mmc, devicetree, linux-kernel --- .../devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml index 8dfad89c7..2bc5ac528 100644 --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml @@ -57,6 +57,12 @@ properties: force fifo watermark setting accordingly. $ref: /schemas/types.yaml#/definitions/flag + fifo-access-32bit: + description: + Specifies that this device requires accesses to its 64-bit registers + to be done as pairs of 32-bit accesses, even on architectures where + readq is available. + dmas: maxItems: 1 -- 2.38.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] devicetree: synopsys-dw-mshc-common: add "fifo-access-32bit" property 2023-03-11 15:22 ` [PATCH 1/2] devicetree: synopsys-dw-mshc-common: add "fifo-access-32bit" property Sergey Lisov @ 2023-03-11 15:50 ` Krzysztof Kozlowski 2023-03-11 17:40 ` Sergey Lisov 0 siblings, 1 reply; 10+ messages in thread From: Krzysztof Kozlowski @ 2023-03-11 15:50 UTC (permalink / raw) To: Sergey Lisov, Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Jaehoon Chung Cc: linux-mmc, devicetree, linux-kernel On 11/03/2023 16:22, Sergey Lisov wrote: Use subject prefixes matching the subsystem (which you can get for example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory your patch is touching). Missing commit msg, which should answer why you are doing this and give more background. > --- > .../devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml > index 8dfad89c7..2bc5ac528 100644 > --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml > +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml > @@ -57,6 +57,12 @@ properties: > force fifo watermark setting accordingly. > $ref: /schemas/types.yaml#/definitions/flag > > + fifo-access-32bit: Missing type boolean. > + description: > + Specifies that this device requires accesses to its 64-bit registers > + to be done as pairs of 32-bit accesses, even on architectures where > + readq is available. And why the device would require this? If it has 64-bit registers in the first place, they can be accessed in 64-bit. Otherwise these are not 64-bit registers, but just lower/upper 32-bit, right? Also, why this cannot be implied from compatible? Why different boards with same SoC should have different FIFO access? Best regards, Krzysztof ^ permalink raw reply [flat|nested] 10+ messages in thread
* (no subject) 2023-03-11 15:50 ` Krzysztof Kozlowski @ 2023-03-11 17:40 ` Sergey Lisov 2023-03-11 17:57 ` broken subject? Krzysztof Kozlowski 0 siblings, 1 reply; 10+ messages in thread From: Sergey Lisov @ 2023-03-11 17:40 UTC (permalink / raw) To: Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Jaehoon Chung Cc: linux-mmc, devicetree, linux-kernel > > --- > > .../devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml > > index 8dfad89c7..2bc5ac528 100644 > > --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml > > +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml > > @@ -57,6 +57,12 @@ properties: > > force fifo watermark setting accordingly. > > $ref: /schemas/types.yaml#/definitions/flag > > > > + fifo-access-32bit: > > Missing type boolean. Thanks, will add the same $ref as for the entry above. > > + description: > > + Specifies that this device requires accesses to its 64-bit registers > > + to be done as pairs of 32-bit accesses, even on architectures where > > + readq is available. > > And why the device would require this? If it has 64-bit registers in the > first place, they can be accessed in 64-bit. Otherwise these are not > 64-bit registers, but just lower/upper 32-bit, right? > > Also, why this cannot be implied from compatible? Why different boards > with same SoC should have different FIFO access? It probably can be implied, but I am not exactly sure on which boards it affects, so I decided to go for a new devicetree option. Anyway, the same argument applies to the "data-addr" property, which is already in the spec, so I supposed that adding such knobs is fine. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: broken subject? 2023-03-11 17:40 ` Sergey Lisov @ 2023-03-11 17:57 ` Krzysztof Kozlowski 2023-03-11 17:57 ` [PATCH 1/2] devicetree: synopsys-dw-mshc-common: add "fifo-access-32bit" property Sergey Lisov 0 siblings, 1 reply; 10+ messages in thread From: Krzysztof Kozlowski @ 2023-03-11 17:57 UTC (permalink / raw) To: Sergey Lisov, Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Jaehoon Chung Cc: linux-mmc, devicetree, linux-kernel On 11/03/2023 18:40, Sergey Lisov wrote: >>> --- >>> .../devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> Why did you remove the subject? Please keep the mailing process matching mailing lists. It messes with mailboxes, filters and reading process. >>> diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml >>> index 8dfad89c7..2bc5ac528 100644 >>> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml >>> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml >>> @@ -57,6 +57,12 @@ properties: >>> force fifo watermark setting accordingly. >>> $ref: /schemas/types.yaml#/definitions/flag >>> >>> + fifo-access-32bit: >> >> Missing type boolean. > > Thanks, will add the same $ref as for the entry above. > >>> + description: >>> + Specifies that this device requires accesses to its 64-bit registers >>> + to be done as pairs of 32-bit accesses, even on architectures where >>> + readq is available. >> >> And why the device would require this? If it has 64-bit registers in the >> first place, they can be accessed in 64-bit. Otherwise these are not >> 64-bit registers, but just lower/upper 32-bit, right? >> >> Also, why this cannot be implied from compatible? Why different boards >> with same SoC should have different FIFO access? > > It probably can be implied, but I am not exactly sure on which boards it > affects, so I decided to go for a new devicetree option. Anyway, the same > argument applies to the "data-addr" property, which is already in the > spec, so I supposed that adding such knobs is fine. Yeah, Rob acked it so I will let him to judge this. To me it looks like unnecessary fragmentation - this looks like compatible specific, not board. Anyway you need to resend to fix all the mailing mess. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] devicetree: synopsys-dw-mshc-common: add "fifo-access-32bit" property 2023-03-11 17:57 ` broken subject? Krzysztof Kozlowski @ 2023-03-11 17:57 ` Sergey Lisov 0 siblings, 0 replies; 10+ messages in thread From: Sergey Lisov @ 2023-03-11 17:57 UTC (permalink / raw) To: Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Jaehoon Chung Cc: linux-mmc, devicetree, linux-kernel > Why did you remove the subject? Please keep the mailing process matching > mailing lists. It messes with mailboxes, filters and reading process. Sorry about this, I just wanted to get rid of those X-Something headers and accidentially deleted this one. > Yeah, Rob acked it so I will let him to judge this. To me it looks like > unnecessary fragmentation - this looks like compatible specific, not > board. Anyway you need to resend to fix all the mailing mess. > > Best regards, > Krzysztof Should I resend the whole patchset from scratch, as if nothing happened? ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] dw_mmc: add an option to force 32-bit accesses to 64-bit device registers 2023-03-11 15:23 [PATCH 0/2] dw_mmc: fix DW MMC cores with 32-bit bus on 64-bit Linux systems Sergey Lisov 2023-03-11 15:22 ` [PATCH 1/2] devicetree: synopsys-dw-mshc-common: add "fifo-access-32bit" property Sergey Lisov @ 2023-03-11 15:22 ` Sergey Lisov 1 sibling, 0 replies; 10+ messages in thread From: Sergey Lisov @ 2023-03-11 15:22 UTC (permalink / raw) To: Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Jaehoon Chung Cc: linux-mmc, devicetree, linux-kernel --- drivers/mmc/host/dw_mmc.c | 125 +++++++++++++++++++++++++++++++++++++- drivers/mmc/host/dw_mmc.h | 2 + 2 files changed, 125 insertions(+), 2 deletions(-) diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index 581614196..eee430620 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -2575,6 +2575,119 @@ static void dw_mci_pull_data64(struct dw_mci *host, void *buf, int cnt) } } +/* + Some dw_mmc devices have 64-bit FIFOs, but expect them to be + accessed using two 32-bit accesses. If such controller is used + with a 64-bit kernel, this has to be done explicitly. + + XXX: Is this issue specific to Exynos7? +*/ + +static inline uint64_t mci_fifo_readq_32(void __iomem *addr) +{ + uint64_t ans; + uint32_t proxy[2]; + + proxy[0] = mci_fifo_readl(addr); + proxy[1] = mci_fifo_readl(addr+4); + memcpy(&ans, proxy, 8); + return ans; +} + +static inline void mci_fifo_writeq_32(void __iomem *addr, uint64_t value) +{ + uint32_t proxy[2]; + + memcpy(proxy, &value, 8); + mci_fifo_writel(addr, proxy[0]); + mci_fifo_writel(addr+4, proxy[1]); +} + +static void dw_mci_push_data64_32(struct dw_mci *host, void *buf, int cnt) +{ + struct mmc_data *data = host->data; + int init_cnt = cnt; + + /* try and push anything in the part_buf */ + if (unlikely(host->part_buf_count)) { + int len = dw_mci_push_part_bytes(host, buf, cnt); + + buf += len; + cnt -= len; + + if (host->part_buf_count == 8) { + mci_fifo_writeq_32(host->fifo_reg, host->part_buf); + host->part_buf_count = 0; + } + } +#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS + if (unlikely((unsigned long)buf & 0x7)) { + while (cnt >= 8) { + u64 aligned_buf[16]; + int len = min(cnt & -8, (int)sizeof(aligned_buf)); + int items = len >> 3; + int i; + /* memcpy from input buffer into aligned buffer */ + memcpy(aligned_buf, buf, len); + buf += len; + cnt -= len; + /* push data from aligned buffer into fifo */ + for (i = 0; i < items; ++i) + mci_fifo_writeq_32(host->fifo_reg, aligned_buf[i]); + } + } else +#endif + { + u64 *pdata = buf; + + for (; cnt >= 8; cnt -= 8) + mci_fifo_writeq_32(host->fifo_reg, *pdata++); + buf = pdata; + } + /* put anything remaining in the part_buf */ + if (cnt) { + dw_mci_set_part_bytes(host, buf, cnt); + /* Push data if we have reached the expected data length */ + if ((data->bytes_xfered + init_cnt) == + (data->blksz * data->blocks)) + mci_fifo_writeq_32(host->fifo_reg, host->part_buf); + } +} + +static void dw_mci_pull_data64_32(struct dw_mci *host, void *buf, int cnt) +{ +#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS + if (unlikely((unsigned long)buf & 0x7)) { + while (cnt >= 8) { + /* pull data from fifo into aligned buffer */ + u64 aligned_buf[16]; + int len = min(cnt & -8, (int)sizeof(aligned_buf)); + int items = len >> 3; + int i; + + for (i = 0; i < items; ++i) + aligned_buf[i] = mci_fifo_readq_32(host->fifo_reg); + + /* memcpy from aligned buffer into output buffer */ + memcpy(buf, aligned_buf, len); + buf += len; + cnt -= len; + } + } else +#endif + { + u64 *pdata = buf; + + for (; cnt >= 8; cnt -= 8) + *pdata++ = mci_fifo_readq_32(host->fifo_reg); + buf = pdata; + } + if (cnt) { + host->part_buf = mci_fifo_readq_32(host->fifo_reg); + dw_mci_pull_final_bytes(host, buf, cnt); + } +} + static void dw_mci_pull_data(struct dw_mci *host, void *buf, int cnt) { int len; @@ -3239,6 +3352,9 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host) if (device_property_present(dev, "fifo-watermark-aligned")) host->wm_aligned = true; + if (device_property_present(dev, "fifo-access-32bit")) + host->quirks |= DW_MMC_QUIRK_FIFO64_32; + if (!device_property_read_u32(dev, "clock-frequency", &clock_frequency)) pdata->bus_hz = clock_frequency; @@ -3367,8 +3483,13 @@ int dw_mci_probe(struct dw_mci *host) width = 16; host->data_shift = 1; } else if (i == 2) { - host->push_data = dw_mci_push_data64; - host->pull_data = dw_mci_pull_data64; + if ((host->quirks & DW_MMC_QUIRK_FIFO64_32)) { + host->push_data = dw_mci_push_data64_32; + host->pull_data = dw_mci_pull_data64_32; + } else { + host->push_data = dw_mci_push_data64; + host->pull_data = dw_mci_pull_data64; + } width = 64; host->data_shift = 3; } else { diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h index 4ed81f94f..edd642b92 100644 --- a/drivers/mmc/host/dw_mmc.h +++ b/drivers/mmc/host/dw_mmc.h @@ -280,6 +280,8 @@ struct dw_mci_board { /* Support for longer data read timeout */ #define DW_MMC_QUIRK_EXTENDED_TMOUT BIT(0) +/* Force 32-bit access to the FIFO */ +#define DW_MMC_QUIRK_FIFO64_32 BIT(1) #define DW_MMC_240A 0x240a #define DW_MMC_280A 0x280a -- 2.38.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
[parent not found: <640cc323.050a0220.8e83b.0e60@mx.google.com>]
* Re: [PATCH 1/2] devicetree: synopsys-dw-mshc-common: add "fifo-access-32bit" property [not found] <640cc323.050a0220.8e83b.0e60@mx.google.com> @ 2023-03-11 18:08 ` Krzysztof Kozlowski 0 siblings, 0 replies; 10+ messages in thread From: Krzysztof Kozlowski @ 2023-03-11 18:08 UTC (permalink / raw) To: Sergey Lisov, Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Jaehoon Chung Cc: linux-mmc, devicetree, linux-kernel On 11/03/2023 18:57, Sergey Lisov wrote: >> Why did you remove the subject? Please keep the mailing process matching >> mailing lists. It messes with mailboxes, filters and reading process. > > Sorry about this, I just wanted to get rid of those X-Something headers and > accidentially deleted this one. > >> Yeah, Rob acked it so I will let him to judge this. To me it looks like >> unnecessary fragmentation - this looks like compatible specific, not >> board. Anyway you need to resend to fix all the mailing mess. >> >> Best regards, >> Krzysztof > > Should I resend the whole patchset from scratch, as if nothing happened? No, sorry, I was not specific. You need anyway v2 with all the fixes I pointed already. Lack of commit msg is a no-go. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <640c80ff.050a0220.2983.0b8d@mx.google.com>]
* Re: [PATCH 1/2] devicetree: synopsys-dw-mshc-common: add "fifo-access-32bit" property [not found] <640c80ff.050a0220.2983.0b8d@mx.google.com> @ 2023-03-11 15:58 ` Krzysztof Kozlowski 0 siblings, 0 replies; 10+ messages in thread From: Krzysztof Kozlowski @ 2023-03-11 15:58 UTC (permalink / raw) To: Sergey Lisov, Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Jaehoon Chung Cc: linux-mmc, devicetree, linux-kernel On 11/03/2023 13:54, Sergey Lisov wrote: > --- > .../devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml | 6 ++++++ I received your patches twice, so I already responded to one thread. The comments apply also here. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] devicetree: synopsys-dw-mshc-common: add "fifo-access-32bit" property @ 2023-03-11 12:54 Sergey Lisov 0 siblings, 0 replies; 10+ messages in thread From: Sergey Lisov @ 2023-03-11 12:54 UTC (permalink / raw) To: Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Jaehoon Chung Cc: linux-mmc, devicetree, linux-kernel --- .../devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml index 8dfad89c7..2bc5ac528 100644 --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml @@ -57,6 +57,12 @@ properties: force fifo watermark setting accordingly. $ref: /schemas/types.yaml#/definitions/flag + fifo-access-32bit: + description: + Specifies that this device requires accesses to its 64-bit registers + to be done as pairs of 32-bit accesses, even on architectures where + readq is available. + dmas: maxItems: 1 -- 2.38.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-03-11 18:08 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-11 15:23 [PATCH 0/2] dw_mmc: fix DW MMC cores with 32-bit bus on 64-bit Linux systems Sergey Lisov 2023-03-11 15:22 ` [PATCH 1/2] devicetree: synopsys-dw-mshc-common: add "fifo-access-32bit" property Sergey Lisov 2023-03-11 15:50 ` Krzysztof Kozlowski 2023-03-11 17:40 ` Sergey Lisov 2023-03-11 17:57 ` broken subject? Krzysztof Kozlowski 2023-03-11 17:57 ` [PATCH 1/2] devicetree: synopsys-dw-mshc-common: add "fifo-access-32bit" property Sergey Lisov 2023-03-11 15:22 ` [PATCH 2/2] dw_mmc: add an option to force 32-bit accesses to 64-bit device registers Sergey Lisov [not found] <640cc323.050a0220.8e83b.0e60@mx.google.com> 2023-03-11 18:08 ` [PATCH 1/2] devicetree: synopsys-dw-mshc-common: add "fifo-access-32bit" property Krzysztof Kozlowski [not found] <640c80ff.050a0220.2983.0b8d@mx.google.com> 2023-03-11 15:58 ` Krzysztof Kozlowski -- strict thread matches above, loose matches on Subject: below -- 2023-03-11 12:54 Sergey Lisov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).