[PATCH 0/3] mmc: mmci: stm32: updates for SDIO

[PATCH 0/3] mmc: mmci: stm32: updates for SDIO

[Yann Gautier]

To properly manage SDIO on STM32 variant of PL18x, we cannot have DMA
Linked Lists enable. As it has to be disabled during probe sequence,
we cannot wait until we have communicated with the SDIO chip. This
should then be done through a DT property.
The MMC_PM_KEEP_POWER should also be disabled on STM32MP157x-DK2 board
as the clock used for wifi is not always-on.

Yann Gautier (3):
  dt-bindings: mmc: mmci: add a property to disable DMA LLI
  mmc: mmci: stm32: manage st,disable-dma-lli property
  mmc: mmci: manage MMC_PM_KEEP_POWER per variant config

 Documentation/devicetree/bindings/mmc/arm,pl18x.yaml |  5 +++++
 drivers/mmc/host/mmci.c                              |  5 ++++-
 drivers/mmc/host/mmci.h                              |  1 +
 drivers/mmc/host/mmci_stm32_sdmmc.c                  | 10 ++++++++--
 4 files changed, 18 insertions(+), 3 deletions(-)

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] dt-bindings: mmc: mmci: add a property to disable DMA LLI

[Yann Gautier]

On STMicroelectronics variant of PL18x, the DMA Linked Lists are supported
starting from revision v2 of the peripheral. But it has limitations,
as all the buffers should be aligned on block size (except the last one).
But this cannot be guaranteed with SDIO. We should then have a property
to disable the support of LLI.

Signed-off-by: Yann Gautier <yann.gautier@foss.st.com>
---
 Documentation/devicetree/bindings/mmc/arm,pl18x.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/arm,pl18x.yaml b/Documentation/devicetree/bindings/mmc/arm,pl18x.yaml
index 1e69a5a42439..309a2c0426e5 100644
--- a/Documentation/devicetree/bindings/mmc/arm,pl18x.yaml
+++ b/Documentation/devicetree/bindings/mmc/arm,pl18x.yaml
@@ -145,6 +145,11 @@ properties:
       driver to sample the receive data (for example with a voltage switch
       transceiver).
 
+  st,disable-dma-lli:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: ST Micro-specific property, disable DMA linked lists.
+      It is used for SDIO.
+
   st,cmd-gpios:
     maxItems: 1
     description:
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] mmc: mmci: stm32: manage st,disable-dma-lli property

[Yann Gautier]

This property is used to disable DMA LLI for an SDMMC instance. We cannot
directly modify the variant struct as it will affect all MMC instances.
The parameter is then copied in the struct sdmmc_idma, and force to 0
if the st,disable-dma-lli property is set in DT.

Signed-off-by: Yann Gautier <yann.gautier@foss.st.com>
---
 drivers/mmc/host/mmci_stm32_sdmmc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/mmci_stm32_sdmmc.c b/drivers/mmc/host/mmci_stm32_sdmmc.c
index 9c13f2c31365..02cb0929c465 100644
--- a/drivers/mmc/host/mmci_stm32_sdmmc.c
+++ b/drivers/mmc/host/mmci_stm32_sdmmc.c
@@ -43,6 +43,7 @@ struct sdmmc_lli_desc {
 struct sdmmc_idma {
 	dma_addr_t sg_dma;
 	void *sg_cpu;
+	u8 dma_lli:1;
 };
 
 struct sdmmc_dlyb {
@@ -118,6 +119,7 @@ static void sdmmc_idma_unprep_data(struct mmci_host *host,
 
 static int sdmmc_idma_setup(struct mmci_host *host)
 {
+	struct device_node *np = host->mmc->parent->of_node;
 	struct sdmmc_idma *idma;
 	struct device *dev = mmc_dev(host->mmc);
 
@@ -125,9 +127,13 @@ static int sdmmc_idma_setup(struct mmci_host *host)
 	if (!idma)
 		return -ENOMEM;
 
+	idma->dma_lli = host->variant->dma_lli;
+	if (of_get_property(np, "st,disable-dma-lli", NULL))
+		idma->dma_lli = 0;
+
 	host->dma_priv = idma;
 
-	if (host->variant->dma_lli) {
+	if (idma->dma_lli) {
 		idma->sg_cpu = dmam_alloc_coherent(dev, SDMMC_LLI_BUF_LEN,
 						   &idma->sg_dma, GFP_KERNEL);
 		if (!idma->sg_cpu) {
@@ -154,7 +160,7 @@ static int sdmmc_idma_start(struct mmci_host *host, unsigned int *datactrl)
 	struct scatterlist *sg;
 	int i;
 
-	if (!host->variant->dma_lli || data->sg_len == 1) {
+	if (!idma->dma_lli || data->sg_len == 1) {
 		writel_relaxed(sg_dma_address(data->sg),
 			       host->base + MMCI_STM32_IDMABASE0R);
 		writel_relaxed(MMCI_STM32_IDMAEN,
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] mmc: mmci: manage MMC_PM_KEEP_POWER per variant config

[Yann Gautier]

Add a disable_keep_power field in variant_data struct. The
MMC_PM_KEEP_POWER flag will be enabled if disable_keep_power is not set.
It is only set to true for stm32_sdmmc variants.

The issue was seen on STM32MP157C-DK2 board, which embeds a wifi chip.
It doesn't correctly support low power, and we need to unbind the wifi
driver before a suspend sequence. But the wifi chip firmware is then
lost, and the communication with SDIO fails if MMC_PM_KEEP_POWER is
enabled.
The flag can still be enabled through DT property: keep-power-in-suspend.

Signed-off-by: Yann Gautier <yann.gautier@foss.st.com>
---
 drivers/mmc/host/mmci.c | 5 ++++-
 drivers/mmc/host/mmci.h | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 45b8608c935c..0e2f2f5d6a52 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -274,6 +274,7 @@ static struct variant_data variant_stm32_sdmmc = {
 	.busy_detect		= true,
 	.busy_detect_flag	= MCI_STM32_BUSYD0,
 	.busy_detect_mask	= MCI_STM32_BUSYD0ENDMASK,
+	.disable_keep_power	= true,
 	.init			= sdmmc_variant_init,
 };
 
@@ -301,6 +302,7 @@ static struct variant_data variant_stm32_sdmmcv2 = {
 	.busy_detect		= true,
 	.busy_detect_flag	= MCI_STM32_BUSYD0,
 	.busy_detect_mask	= MCI_STM32_BUSYD0ENDMASK,
+	.disable_keep_power	= true,
 	.init			= sdmmc_variant_init,
 };
 
@@ -2172,7 +2174,8 @@ static int mmci_probe(struct amba_device *dev,
 	host->stop_abort.flags = MMC_RSP_R1B | MMC_CMD_AC;
 
 	/* We support these PM capabilities. */
-	mmc->pm_caps |= MMC_PM_KEEP_POWER;
+	if (!variant->disable_keep_power)
+		mmc->pm_caps |= MMC_PM_KEEP_POWER;
 
 	/*
 	 * We can do SGIO
diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index e1a9b96a3396..2cad1ef9766a 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -361,6 +361,7 @@ struct variant_data {
 	u32			opendrain;
 	u8			dma_lli:1;
 	u32			stm32_idmabsize_mask;
+	u8			disable_keep_power:1;
 	void (*init)(struct mmci_host *host);
 };
 
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] dt-bindings: mmc: mmci: add a property to disable DMA LLI

[Ulf Hansson]

Hi Yann,

On Fri, 4 Mar 2022 at 14:52, Yann Gautier <yann.gautier@foss.st.com> wrote:
>
> On STMicroelectronics variant of PL18x, the DMA Linked Lists are supported
> starting from revision v2 of the peripheral. But it has limitations,
> as all the buffers should be aligned on block size (except the last one).
> But this cannot be guaranteed with SDIO. We should then have a property
> to disable the support of LLI.

Indeed, the buffer handling with SDIO is somewhat special, which also
has been discussed several times on LKML before. In principle, we need
the SDIO func drivers to respect buffer limitations that should be
specified by the mmc host drivers. Quite similar to what we already
have for block devices, like ->max_seg_size, ->max_seg, etc, that is
set per mmc host.

I realize that implementing something like the above requires bigger
changes, which is why mmc host drivers instead validates the sglists
and the elements. In some cases that means returning an error code and
in others it could mean falling back to a non-DMA based I/O mode.

For the stm32_sdmmc variant, it looks like the sglist validation is
being managed in sdmmc_idma_validate_data() already. Can it be
extended to cover this case too, rather than using a DT property?

Kind regards
Uffe

>
> Signed-off-by: Yann Gautier <yann.gautier@foss.st.com>
> ---
>  Documentation/devicetree/bindings/mmc/arm,pl18x.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/arm,pl18x.yaml b/Documentation/devicetree/bindings/mmc/arm,pl18x.yaml
> index 1e69a5a42439..309a2c0426e5 100644
> --- a/Documentation/devicetree/bindings/mmc/arm,pl18x.yaml
> +++ b/Documentation/devicetree/bindings/mmc/arm,pl18x.yaml
> @@ -145,6 +145,11 @@ properties:
>        driver to sample the receive data (for example with a voltage switch
>        transceiver).
>
> +  st,disable-dma-lli:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: ST Micro-specific property, disable DMA linked lists.
> +      It is used for SDIO.
> +
>    st,cmd-gpios:
>      maxItems: 1
>      description:
> --
> 2.25.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] dt-bindings: mmc: mmci: add a property to disable DMA LLI

[Yann Gautier]

On 3/8/22 11:57, Ulf Hansson wrote:
> Hi Yann,
> 
> On Fri, 4 Mar 2022 at 14:52, Yann Gautier <yann.gautier@foss.st.com> wrote:
>>
>> On STMicroelectronics variant of PL18x, the DMA Linked Lists are supported
>> starting from revision v2 of the peripheral. But it has limitations,
>> as all the buffers should be aligned on block size (except the last one).
>> But this cannot be guaranteed with SDIO. We should then have a property
>> to disable the support of LLI.
> 
> Indeed, the buffer handling with SDIO is somewhat special, which also
> has been discussed several times on LKML before. In principle, we need
> the SDIO func drivers to respect buffer limitations that should be
> specified by the mmc host drivers. Quite similar to what we already
> have for block devices, like ->max_seg_size, ->max_seg, etc, that is
> set per mmc host.
> 
> I realize that implementing something like the above requires bigger
> changes, which is why mmc host drivers instead validates the sglists
> and the elements. In some cases that means returning an error code and
> in others it could mean falling back to a non-DMA based I/O mode.
> 
> For the stm32_sdmmc variant, it looks like the sglist validation is
> being managed in sdmmc_idma_validate_data() already. Can it be
> extended to cover this case too, rather than using a DT property?
> 
> Kind regards
> Uffe

Hi Ulf,

OK, I'll check what can be done for this. Patches 1 and 2 can be 
dropped, they will be reworked.
But patch 3 of this series could be taken, as not linked to LLI 
management. Should I push it again alone, or could you review it directly?

Best regards,
Yann

> 
>>
>> Signed-off-by: Yann Gautier <yann.gautier@foss.st.com>
>> ---
>>   Documentation/devicetree/bindings/mmc/arm,pl18x.yaml | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/arm,pl18x.yaml b/Documentation/devicetree/bindings/mmc/arm,pl18x.yaml
>> index 1e69a5a42439..309a2c0426e5 100644
>> --- a/Documentation/devicetree/bindings/mmc/arm,pl18x.yaml
>> +++ b/Documentation/devicetree/bindings/mmc/arm,pl18x.yaml
>> @@ -145,6 +145,11 @@ properties:
>>         driver to sample the receive data (for example with a voltage switch
>>         transceiver).
>>
>> +  st,disable-dma-lli:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description: ST Micro-specific property, disable DMA linked lists.
>> +      It is used for SDIO.
>> +
>>     st,cmd-gpios:
>>       maxItems: 1
>>       description:
>> --
>> 2.25.1
>>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] dt-bindings: mmc: mmci: add a property to disable DMA LLI

[Ulf Hansson]

On Thu, 10 Mar 2022 at 17:00, Yann Gautier <yann.gautier@foss.st.com> wrote:
>
> On 3/8/22 11:57, Ulf Hansson wrote:
> > Hi Yann,
> >
> > On Fri, 4 Mar 2022 at 14:52, Yann Gautier <yann.gautier@foss.st.com> wrote:
> >>
> >> On STMicroelectronics variant of PL18x, the DMA Linked Lists are supported
> >> starting from revision v2 of the peripheral. But it has limitations,
> >> as all the buffers should be aligned on block size (except the last one).
> >> But this cannot be guaranteed with SDIO. We should then have a property
> >> to disable the support of LLI.
> >
> > Indeed, the buffer handling with SDIO is somewhat special, which also
> > has been discussed several times on LKML before. In principle, we need
> > the SDIO func drivers to respect buffer limitations that should be
> > specified by the mmc host drivers. Quite similar to what we already
> > have for block devices, like ->max_seg_size, ->max_seg, etc, that is
> > set per mmc host.
> >
> > I realize that implementing something like the above requires bigger
> > changes, which is why mmc host drivers instead validates the sglists
> > and the elements. In some cases that means returning an error code and
> > in others it could mean falling back to a non-DMA based I/O mode.
> >
> > For the stm32_sdmmc variant, it looks like the sglist validation is
> > being managed in sdmmc_idma_validate_data() already. Can it be
> > extended to cover this case too, rather than using a DT property?
> >
> > Kind regards
> > Uffe
>
> Hi Ulf,
>
> OK, I'll check what can be done for this. Patches 1 and 2 can be
> dropped, they will be reworked.

Okay.

> But patch 3 of this series could be taken, as not linked to LLI
> management. Should I push it again alone, or could you review it directly?

I have some comments/questions on it, but perhaps it makes it easier
for people to follow the discussion if it is done separately. So
please push it alone, then I will review it.

[...]

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] dt-bindings: mmc: mmci: add a property to disable DMA LLI

[Linus Walleij]

"On Fri, Mar 4, 2022 at 2:52 PM Yann Gautier <yann.gautier@foss.st.com> wrote:

> On STMicroelectronics variant of PL18x, the DMA Linked Lists are supported
> starting from revision v2 of the peripheral. But it has limitations,
> as all the buffers should be aligned on block size (except the last one).
> But this cannot be guaranteed with SDIO. We should then have a property
> to disable the support of LLI.
>
> Signed-off-by: Yann Gautier <yann.gautier@foss.st.com>

Actually I think this is present also on the ux500 variants. See:
commit 2253ed4b36dc876d1598c4dab5587e537ec68c34
"mmc: mmci: Support any block sizes for ux500v2 and qcom variant"

Spot the variant data "dma_power_of_2".

So whatever property you add
to the variant data (not in the device tree please) should
be added to the ux500 variants as well, it will *VERY* likely
have a problem with LLI elements not being a power of 2
as it is the ancestor of later STMicro variants.

It might actually be the reason for some annoying WiFi error
messages I have seen :/

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] dt-bindings: mmc: mmci: add a property to disable DMA LLI

[Yann Gautier]

On 3/14/22 23:43, Linus Walleij wrote:
> "On Fri, Mar 4, 2022 at 2:52 PM Yann Gautier <yann.gautier@foss.st.com> wrote:
> 
>> On STMicroelectronics variant of PL18x, the DMA Linked Lists are supported
>> starting from revision v2 of the peripheral. But it has limitations,
>> as all the buffers should be aligned on block size (except the last one).
>> But this cannot be guaranteed with SDIO. We should then have a property
>> to disable the support of LLI.
>>
>> Signed-off-by: Yann Gautier <yann.gautier@foss.st.com>
> 
> Actually I think this is present also on the ux500 variants. See:
> commit 2253ed4b36dc876d1598c4dab5587e537ec68c34
> "mmc: mmci: Support any block sizes for ux500v2 and qcom variant"
> 
> Spot the variant data "dma_power_of_2".
> 
> So whatever property you add
> to the variant data (not in the device tree please) should
> be added to the ux500 variants as well, it will *VERY* likely
> have a problem with LLI elements not being a power of 2
> as it is the ancestor of later STMicro variants.
> 
> It might actually be the reason for some annoying WiFi error
> messages I have seen :/
> 
> Yours,
> Linus Walleij

Hi Linus,

The STM32 variant uses an internal DMA, and the DMA functions are in its 
dedicated file. So I was planning to do the same as what is done in 
meson-gx-mmc.c: using a bounce buffer to copy from/to in case DMA 
constraints are not fulfilled. Not sure it can help for Ux500.

Ulf, before I send my new series (although it is not ready yet), would 
you be OK with the bounce buffer idea?


Best regards,
Yann

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] dt-bindings: mmc: mmci: add a property to disable DMA LLI

[Ulf Hansson]

On Tue, 15 Mar 2022 at 09:26, Yann Gautier <yann.gautier@foss.st.com> wrote:
>
> On 3/14/22 23:43, Linus Walleij wrote:
> > "On Fri, Mar 4, 2022 at 2:52 PM Yann Gautier <yann.gautier@foss.st.com> wrote:
> >
> >> On STMicroelectronics variant of PL18x, the DMA Linked Lists are supported
> >> starting from revision v2 of the peripheral. But it has limitations,
> >> as all the buffers should be aligned on block size (except the last one).
> >> But this cannot be guaranteed with SDIO. We should then have a property
> >> to disable the support of LLI.
> >>
> >> Signed-off-by: Yann Gautier <yann.gautier@foss.st.com>
> >
> > Actually I think this is present also on the ux500 variants. See:
> > commit 2253ed4b36dc876d1598c4dab5587e537ec68c34
> > "mmc: mmci: Support any block sizes for ux500v2 and qcom variant"
> >
> > Spot the variant data "dma_power_of_2".
> >
> > So whatever property you add
> > to the variant data (not in the device tree please) should
> > be added to the ux500 variants as well, it will *VERY* likely
> > have a problem with LLI elements not being a power of 2
> > as it is the ancestor of later STMicro variants.
> >
> > It might actually be the reason for some annoying WiFi error
> > messages I have seen :/
> >
> > Yours,
> > Linus Walleij
>
> Hi Linus,
>
> The STM32 variant uses an internal DMA, and the DMA functions are in its
> dedicated file. So I was planning to do the same as what is done in
> meson-gx-mmc.c: using a bounce buffer to copy from/to in case DMA
> constraints are not fulfilled. Not sure it can help for Ux500.

We already have a bounce buffer in mmci_pio_read(), but we need one in
mmc_pio_write() too, which hasn't been implemented yet.

>
> Ulf, before I send my new series (although it is not ready yet), would
> you be OK with the bounce buffer idea?

Yes, that works for me.

On the other hand, it would be even better if we could specify the
buffer limitations per mmc host instance, so upper layers (SDIO func
drivers) could conform to these - and use better buffers, to achieve a
better performance.

>
>
> Best regards,
> Yann

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] dt-bindings: mmc: mmci: add a property to disable DMA LLI

[Yann Gautier]

On 3/17/22 11:00, Ulf Hansson wrote:
> On Tue, 15 Mar 2022 at 09:26, Yann Gautier <yann.gautier@foss.st.com> wrote:
>>
>> On 3/14/22 23:43, Linus Walleij wrote:
>>> "On Fri, Mar 4, 2022 at 2:52 PM Yann Gautier <yann.gautier@foss.st.com> wrote:
>>>
>>>> On STMicroelectronics variant of PL18x, the DMA Linked Lists are supported
>>>> starting from revision v2 of the peripheral. But it has limitations,
>>>> as all the buffers should be aligned on block size (except the last one).
>>>> But this cannot be guaranteed with SDIO. We should then have a property
>>>> to disable the support of LLI.
>>>>
>>>> Signed-off-by: Yann Gautier <yann.gautier@foss.st.com>
>>>
>>> Actually I think this is present also on the ux500 variants. See:
>>> commit 2253ed4b36dc876d1598c4dab5587e537ec68c34
>>> "mmc: mmci: Support any block sizes for ux500v2 and qcom variant"
>>>
>>> Spot the variant data "dma_power_of_2".
>>>
>>> So whatever property you add
>>> to the variant data (not in the device tree please) should
>>> be added to the ux500 variants as well, it will *VERY* likely
>>> have a problem with LLI elements not being a power of 2
>>> as it is the ancestor of later STMicro variants.
>>>
>>> It might actually be the reason for some annoying WiFi error
>>> messages I have seen :/
>>>
>>> Yours,
>>> Linus Walleij
>>
>> Hi Linus,
>>
>> The STM32 variant uses an internal DMA, and the DMA functions are in its
>> dedicated file. So I was planning to do the same as what is done in
>> meson-gx-mmc.c: using a bounce buffer to copy from/to in case DMA
>> constraints are not fulfilled. Not sure it can help for Ux500.
> 

Hi Ulf,

> We already have a bounce buffer in mmci_pio_read(), but we need one in
> mmc_pio_write() too, which hasn't been implemented yet.

The idea is to keep using our internal DMA, and not switch to pio mode.

> 
>>
>> Ulf, before I send my new series (although it is not ready yet), would
>> you be OK with the bounce buffer idea?
> 
> Yes, that works for me.
I have patches almost ready, I'll send that soon.

> 
> On the other hand, it would be even better if we could specify the
> buffer limitations per mmc host instance, so upper layers (SDIO func
> drivers) could conform to these - and use better buffers, to achieve a
> better performance.

We've seen things that could be improved in the wifi driver. I'll check 
what could be done.

Best regards,
Yann
> 
>>
>>
>> Best regards,
>> Yann
> 
> Kind regards
> Uffe


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] dt-bindings: mmc: mmci: add a property to disable DMA LLI

[Ulf Hansson]

On Thu, 17 Mar 2022 at 11:18, Yann Gautier <yann.gautier@foss.st.com> wrote:
>
> On 3/17/22 11:00, Ulf Hansson wrote:
> > On Tue, 15 Mar 2022 at 09:26, Yann Gautier <yann.gautier@foss.st.com> wrote:
> >>
> >> On 3/14/22 23:43, Linus Walleij wrote:
> >>> "On Fri, Mar 4, 2022 at 2:52 PM Yann Gautier <yann.gautier@foss.st.com> wrote:
> >>>
> >>>> On STMicroelectronics variant of PL18x, the DMA Linked Lists are supported
> >>>> starting from revision v2 of the peripheral. But it has limitations,
> >>>> as all the buffers should be aligned on block size (except the last one).
> >>>> But this cannot be guaranteed with SDIO. We should then have a property
> >>>> to disable the support of LLI.
> >>>>
> >>>> Signed-off-by: Yann Gautier <yann.gautier@foss.st.com>
> >>>
> >>> Actually I think this is present also on the ux500 variants. See:
> >>> commit 2253ed4b36dc876d1598c4dab5587e537ec68c34
> >>> "mmc: mmci: Support any block sizes for ux500v2 and qcom variant"
> >>>
> >>> Spot the variant data "dma_power_of_2".
> >>>
> >>> So whatever property you add
> >>> to the variant data (not in the device tree please) should
> >>> be added to the ux500 variants as well, it will *VERY* likely
> >>> have a problem with LLI elements not being a power of 2
> >>> as it is the ancestor of later STMicro variants.
> >>>
> >>> It might actually be the reason for some annoying WiFi error
> >>> messages I have seen :/
> >>>
> >>> Yours,
> >>> Linus Walleij
> >>
> >> Hi Linus,
> >>
> >> The STM32 variant uses an internal DMA, and the DMA functions are in its
> >> dedicated file. So I was planning to do the same as what is done in
> >> meson-gx-mmc.c: using a bounce buffer to copy from/to in case DMA
> >> constraints are not fulfilled. Not sure it can help for Ux500.
> >
>
> Hi Ulf,
>
> > We already have a bounce buffer in mmci_pio_read(), but we need one in
> > mmc_pio_write() too, which hasn't been implemented yet.
>
> The idea is to keep using our internal DMA, and not switch to pio mode.

Okay, but doesn't that become awfully inefficient, especially if we
end up with several smaller sg-list-elements.

The switch to pio would only be temporary for the particular request
that has "bogus" buffers.

>
> >
> >>
> >> Ulf, before I send my new series (although it is not ready yet), would
> >> you be OK with the bounce buffer idea?
> >
> > Yes, that works for me.
> I have patches almost ready, I'll send that soon.

Okay!

>
> >
> > On the other hand, it would be even better if we could specify the
> > buffer limitations per mmc host instance, so upper layers (SDIO func
> > drivers) could conform to these - and use better buffers, to achieve a
> > better performance.
>
> We've seen things that could be improved in the wifi driver. I'll check
> what could be done.

Great!

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] dt-bindings: mmc: mmci: add a property to disable DMA LLI

[Linus Walleij]

On Tue, Mar 15, 2022 at 9:26 AM Yann Gautier <yann.gautier@foss.st.com> wrote:

> The STM32 variant uses an internal DMA, and the DMA functions are in its
> dedicated file. So I was planning to do the same as what is done in
> meson-gx-mmc.c: using a bounce buffer to copy from/to in case DMA
> constraints are not fulfilled. Not sure it can help for Ux500.
>
> Ulf, before I send my new series (although it is not ready yet), would
> you be OK with the bounce buffer idea?

Would it not be better if the bounce buffer is something available
for all MMCI variants and not restricted to the STM32 DMA add-on?

What I'm thinking is that this is a problem with the MMCI hardware
rather than with the DMA hardware, so the problem kind of gets
fixed in the wrong place if the bounce buffer is in the DMA add-on
code.

Maybe this is how you fixed it in later patches, I'll take a look.

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] dt-bindings: mmc: mmci: add a property to disable DMA LLI

[Yann Gautier]

On 3/25/22 21:12, Linus Walleij wrote:
> On Tue, Mar 15, 2022 at 9:26 AM Yann Gautier <yann.gautier@foss.st.com> wrote:
> 
>> The STM32 variant uses an internal DMA, and the DMA functions are in its
>> dedicated file. So I was planning to do the same as what is done in
>> meson-gx-mmc.c: using a bounce buffer to copy from/to in case DMA
>> constraints are not fulfilled. Not sure it can help for Ux500.
>>
>> Ulf, before I send my new series (although it is not ready yet), would
>> you be OK with the bounce buffer idea?
> 
Hi Linus,

> Would it not be better if the bounce buffer is something available
> for all MMCI variants and not restricted to the STM32 DMA add-on?

The issue was seen with the internal DMA of the STM32 variant of PL180.

> 
> What I'm thinking is that this is a problem with the MMCI hardware
> rather than with the DMA hardware, so the problem kind of gets
> fixed in the wrong place if the bounce buffer is in the DMA add-on
> code.
> 
> Maybe this is how you fixed it in later patches, I'll take a look.

I've pushed a new version there to correct latest remarks from Ulf:
https://lore.kernel.org/all/20220328145114.334577-1-yann.gautier@foss.st.com/


> 
> Yours,
> Linus Walleij

Best regards,
Yann


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] dt-bindings: mmc: mmci: add a property to disable DMA LLI

[Manivannan Sadhasivam]

On Tue, Mar 15, 2022 at 09:26:01AM +0100, Yann Gautier wrote:
> On 3/14/22 23:43, Linus Walleij wrote:
> > "On Fri, Mar 4, 2022 at 2:52 PM Yann Gautier <yann.gautier@foss.st.com> wrote:
> > 
> > > On STMicroelectronics variant of PL18x, the DMA Linked Lists are supported
> > > starting from revision v2 of the peripheral. But it has limitations,
> > > as all the buffers should be aligned on block size (except the last one).
> > > But this cannot be guaranteed with SDIO. We should then have a property
> > > to disable the support of LLI.
> > > 
> > > Signed-off-by: Yann Gautier <yann.gautier@foss.st.com>
> > 
> > Actually I think this is present also on the ux500 variants. See:
> > commit 2253ed4b36dc876d1598c4dab5587e537ec68c34
> > "mmc: mmci: Support any block sizes for ux500v2 and qcom variant"
> > 
> > Spot the variant data "dma_power_of_2".
> > 
> > So whatever property you add
> > to the variant data (not in the device tree please) should
> > be added to the ux500 variants as well, it will *VERY* likely
> > have a problem with LLI elements not being a power of 2
> > as it is the ancestor of later STMicro variants.
> > 
> > It might actually be the reason for some annoying WiFi error
> > messages I have seen :/
> > 
> > Yours,
> > Linus Walleij
> 
> Hi Linus,
> 
> The STM32 variant uses an internal DMA, and the DMA functions are in its
> dedicated file. So I was planning to do the same as what is done in
> meson-gx-mmc.c: using a bounce buffer to copy from/to in case DMA
> constraints are not fulfilled. Not sure it can help for Ux500.
> 

Irrelevant to this patch: May I know why the internal DMA cannot be represented
as a dmaengine driver? We started seeing these internal DMA implementations in
the other subsystems as well with pointers towards MMC core [1].

Thanks,
Mani

[1] https://lore.kernel.org/all/CA+V-a8tfUgvzPyMe_FHuz=8mmC6dPHP7E=e+nCzOey04vCcAkg@mail.gmail.com/

> Ulf, before I send my new series (although it is not ready yet), would you
> be OK with the bounce buffer idea?
> 
> 
> Best regards,
> Yann

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] dt-bindings: mmc: mmci: add a property to disable DMA LLI

[Yann Gautier]

On 3/29/22 17:31, Manivannan Sadhasivam wrote:
> On Tue, Mar 15, 2022 at 09:26:01AM +0100, Yann Gautier wrote:
>> On 3/14/22 23:43, Linus Walleij wrote:
>>> "On Fri, Mar 4, 2022 at 2:52 PM Yann Gautier <yann.gautier@foss.st.com> wrote:
>>>
>>>> On STMicroelectronics variant of PL18x, the DMA Linked Lists are supported
>>>> starting from revision v2 of the peripheral. But it has limitations,
>>>> as all the buffers should be aligned on block size (except the last one).
>>>> But this cannot be guaranteed with SDIO. We should then have a property
>>>> to disable the support of LLI.
>>>>
>>>> Signed-off-by: Yann Gautier <yann.gautier@foss.st.com>
>>>
>>> Actually I think this is present also on the ux500 variants. See:
>>> commit 2253ed4b36dc876d1598c4dab5587e537ec68c34
>>> "mmc: mmci: Support any block sizes for ux500v2 and qcom variant"
>>>
>>> Spot the variant data "dma_power_of_2".
>>>
>>> So whatever property you add
>>> to the variant data (not in the device tree please) should
>>> be added to the ux500 variants as well, it will *VERY* likely
>>> have a problem with LLI elements not being a power of 2
>>> as it is the ancestor of later STMicro variants.
>>>
>>> It might actually be the reason for some annoying WiFi error
>>> messages I have seen :/
>>>
>>> Yours,
>>> Linus Walleij
>>
>> Hi Linus,
>>
>> The STM32 variant uses an internal DMA, and the DMA functions are in its
>> dedicated file. So I was planning to do the same as what is done in
>> meson-gx-mmc.c: using a bounce buffer to copy from/to in case DMA
>> constraints are not fulfilled. Not sure it can help for Ux500.
>>

Hi Mani,

> 
> Irrelevant to this patch: May I know why the internal DMA cannot be represented
> as a dmaengine driver? We started seeing these internal DMA implementations in
> the other subsystems as well with pointers towards MMC core [1].

As for Prabhakar's answer, the IDMA here is inside our IP, and not used 
in any other IP. So I'm not sure it is really relevant to move that to 
another dmaengine driver.

> 
> Thanks,
> Mani

Best regards,
Yann

> 
> [1] https://lore.kernel.org/all/CA+V-a8tfUgvzPyMe_FHuz=8mmC6dPHP7E=e+nCzOey04vCcAkg@mail.gmail.com/
> 
>> Ulf, before I send my new series (although it is not ready yet), would you
>> be OK with the bounce buffer idea?
>>
>>
>> Best regards,
>> Yann


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] dt-bindings: mmc: mmci: add a property to disable DMA LLI

[Manivannan Sadhasivam]

On Tue, Mar 29, 2022 at 06:00:26PM +0200, Yann Gautier wrote:
> On 3/29/22 17:31, Manivannan Sadhasivam wrote:
> > On Tue, Mar 15, 2022 at 09:26:01AM +0100, Yann Gautier wrote:
> > > On 3/14/22 23:43, Linus Walleij wrote:
> > > > "On Fri, Mar 4, 2022 at 2:52 PM Yann Gautier <yann.gautier@foss.st.com> wrote:
> > > > 
> > > > > On STMicroelectronics variant of PL18x, the DMA Linked Lists are supported
> > > > > starting from revision v2 of the peripheral. But it has limitations,
> > > > > as all the buffers should be aligned on block size (except the last one).
> > > > > But this cannot be guaranteed with SDIO. We should then have a property
> > > > > to disable the support of LLI.
> > > > > 
> > > > > Signed-off-by: Yann Gautier <yann.gautier@foss.st.com>
> > > > 
> > > > Actually I think this is present also on the ux500 variants. See:
> > > > commit 2253ed4b36dc876d1598c4dab5587e537ec68c34
> > > > "mmc: mmci: Support any block sizes for ux500v2 and qcom variant"
> > > > 
> > > > Spot the variant data "dma_power_of_2".
> > > > 
> > > > So whatever property you add
> > > > to the variant data (not in the device tree please) should
> > > > be added to the ux500 variants as well, it will *VERY* likely
> > > > have a problem with LLI elements not being a power of 2
> > > > as it is the ancestor of later STMicro variants.
> > > > 
> > > > It might actually be the reason for some annoying WiFi error
> > > > messages I have seen :/
> > > > 
> > > > Yours,
> > > > Linus Walleij
> > > 
> > > Hi Linus,
> > > 
> > > The STM32 variant uses an internal DMA, and the DMA functions are in its
> > > dedicated file. So I was planning to do the same as what is done in
> > > meson-gx-mmc.c: using a bounce buffer to copy from/to in case DMA
> > > constraints are not fulfilled. Not sure it can help for Ux500.
> > > 
> 
> Hi Mani,
> 
> > 
> > Irrelevant to this patch: May I know why the internal DMA cannot be represented
> > as a dmaengine driver? We started seeing these internal DMA implementations in
> > the other subsystems as well with pointers towards MMC core [1].
> 
> As for Prabhakar's answer, the IDMA here is inside our IP, and not used in
> any other IP. So I'm not sure it is really relevant to move that to another
> dmaengine driver.
> 

Okay, I think this justification makes sense. I was worried of DMA IPs that get
sandwiched into many peripherals like the one on Renesas platforms. It turned
out that each subsystem has to add internal DMA support for it :/

Ulf, your thoughts?

Thanks,
Mani

> > 
> > Thanks,
> > Mani
> 
> Best regards,
> Yann
> 
> > 
> > [1] https://lore.kernel.org/all/CA+V-a8tfUgvzPyMe_FHuz=8mmC6dPHP7E=e+nCzOey04vCcAkg@mail.gmail.com/
> > 
> > > Ulf, before I send my new series (although it is not ready yet), would you
> > > be OK with the bounce buffer idea?
> > > 
> > > 
> > > Best regards,
> > > Yann
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] dt-bindings: mmc: mmci: add a property to disable DMA LLI

[Linus Walleij]

On Tue, Mar 29, 2022 at 7:33 PM Manivannan Sadhasivam <mani@kernel.org> wrote:
> [Yann]
> > As for Prabhakar's answer, the IDMA here is inside our IP, and not used in
> > any other IP. So I'm not sure it is really relevant to move that to another
> > dmaengine driver.
>
> Okay, I think this justification makes sense. I was worried of DMA IPs that get
> sandwiched into many peripherals like the one on Renesas platforms. It turned
> out that each subsystem has to add internal DMA support for it :/

That is a justified worry.

Qualcomm has "BAM DMA" (I think it is called?) which is added to each IP
that needs DMA. drivers/mmc/host/mmci_qcom_dml.c
It's for older Qualcomm platforms but I *think* it is actually not just used
for the MMCI, just noone ever got around to adding it to any other
peripheral? Srini do you know?

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] dt-bindings: mmc: mmci: add a property to disable DMA LLI

[Srinivas Kandagatla]

On 29/03/2022 22:16, Linus Walleij wrote:
> On Tue, Mar 29, 2022 at 7:33 PM Manivannan Sadhasivam <mani@kernel.org> wrote:
>> [Yann]
>>> As for Prabhakar's answer, the IDMA here is inside our IP, and not used in
>>> any other IP. So I'm not sure it is really relevant to move that to another
>>> dmaengine driver.
>>
>> Okay, I think this justification makes sense. I was worried of DMA IPs that get
>> sandwiched into many peripherals like the one on Renesas platforms. It turned
>> out that each subsystem has to add internal DMA support for it :/
> 
> That is a justified worry.
> 
> Qualcomm has "BAM DMA" (I think it is called?) which is added to each IP
> that needs DMA. drivers/mmc/host/mmci_qcom_dml.c
> It's for older Qualcomm platforms but I *think* it is actually not just used
> for the MMCI, just noone ever got around to adding it to any other
> peripheral? Srini do you know?

There are multiple instances of BAM (Bus access manager) on Qcom SoC, 
some of these instances are dedicated for each peripheral instance.
In this particular case we have 4 instances of BAM each of which are 
dedicated to 4 instances of SD Controllers.

BAM dmaengine is used across many Qualcomm peripheral drivers.

--srini

> 
> Yours,
> Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel