linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] mmc: mmci: add quirk property to add stm32 transfer mode
@ 2019-02-21 10:10 Ludovic Barre
  2019-02-21 10:10 ` [PATCH 1/2] mmc: mmci: introduce a quirks property into variant struct Ludovic Barre
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Ludovic Barre @ 2019-02-21 10:10 UTC (permalink / raw)
  To: Ulf Hansson, Rob Herring
  Cc: srinivas.kandagatla, Maxime Coquelin, Alexandre Torgue,
	linux-arm-kernel, linux-kernel, devicetree, linux-mmc,
	linux-stm32, Ludovic Barre

From: Ludovic Barre <ludovic.barre@st.com>

This patch series introduces a bitmap of hardware quirks that require
some special action. This should reduce the number of boolean
into variant structure. 
And adds quirk bit to define sdmmc specific transfer modes.

Ludovic Barre (2):
  mmc: mmci: introduce a quirks property into variant struct
  mmc: mmci: add quirk property to add stm32 transfer mode

 drivers/mmc/host/mmci.c | 11 +++++++++++
 drivers/mmc/host/mmci.h |  9 +++++++++
 2 files changed, 20 insertions(+)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] mmc: mmci: introduce a quirks property into variant struct
  2019-02-21 10:10 [PATCH 0/2] mmc: mmci: add quirk property to add stm32 transfer mode Ludovic Barre
@ 2019-02-21 10:10 ` Ludovic Barre
  2019-02-21 10:10 ` [PATCH 2/2] mmc: mmci: add quirk property to add stm32 transfer mode Ludovic Barre
  2019-02-21 10:27 ` [PATCH 0/2] " Russell King - ARM Linux admin
  2 siblings, 0 replies; 9+ messages in thread
From: Ludovic Barre @ 2019-02-21 10:10 UTC (permalink / raw)
  To: Ulf Hansson, Rob Herring
  Cc: srinivas.kandagatla, Maxime Coquelin, Alexandre Torgue,
	linux-arm-kernel, linux-kernel, devicetree, linux-mmc,
	linux-stm32, Ludovic Barre

From: Ludovic Barre <ludovic.barre@st.com>

This patch introduces a bitmap of hardware quirks that require
some special action. This should reduce the number of boolean
into variant structure.

Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
---
 drivers/mmc/host/mmci.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index 14df810..474f4fa 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -307,6 +307,7 @@ struct mmci_host;
  * @opendrain: bitmask identifying the OPENDRAIN bit inside MMCIPOWER register
  * @dma_lli: true if variant has dma link list feature.
  * @stm32_idmabsize_mask: stm32 sdmmc idma buffer size.
+ * @quirks: A bitmap of hardware quirks that require some special action.
  */
 struct variant_data {
 	unsigned int		clkreg;
@@ -352,6 +353,7 @@ struct variant_data {
 	u32			opendrain;
 	u8			dma_lli:1;
 	u32			stm32_idmabsize_mask;
+	u32			quirks;
 	void (*init)(struct mmci_host *host);
 };
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] mmc: mmci: add quirk property to add stm32 transfer mode
  2019-02-21 10:10 [PATCH 0/2] mmc: mmci: add quirk property to add stm32 transfer mode Ludovic Barre
  2019-02-21 10:10 ` [PATCH 1/2] mmc: mmci: introduce a quirks property into variant struct Ludovic Barre
@ 2019-02-21 10:10 ` Ludovic Barre
  2019-02-21 10:27 ` [PATCH 0/2] " Russell King - ARM Linux admin
  2 siblings, 0 replies; 9+ messages in thread
From: Ludovic Barre @ 2019-02-21 10:10 UTC (permalink / raw)
  To: Ulf Hansson, Rob Herring
  Cc: srinivas.kandagatla, Maxime Coquelin, Alexandre Torgue,
	linux-arm-kernel, linux-kernel, devicetree, linux-mmc,
	linux-stm32, Ludovic Barre

From: Ludovic Barre <ludovic.barre@st.com>

This patch adds a quirk bit to define specific stm32 transfer
modes. sdmmc data transfer mode selection could be:
-Block data transfer ending on block count.
-SDIO multibyte data transfer.
-MMC Stream data transfer (not used).
-Block data transfer ending with STOP_TRANSMISSION command.

Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
---
 drivers/mmc/host/mmci.c | 11 +++++++++++
 drivers/mmc/host/mmci.h |  7 +++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 387ff14..44c721a 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -284,6 +284,7 @@ static struct variant_data variant_stm32_sdmmc = {
 	.datactrl_blocksz	= 14,
 	.stm32_idmabsize_mask	= GENMASK(12, 5),
 	.init			= sdmmc_variant_init,
+	.quirks			= MMCI_QUIRK_STM32_DTMODE,
 };
 
 static struct variant_data variant_qcom = {
@@ -1028,6 +1029,16 @@ static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 	else
 		datactrl = variant->datactrl_dpsm_enable | blksz_bits << 4;
 
+	if (variant->quirks & MMCI_QUIRK_STM32_DTMODE) {
+		if (host->mmc->card && mmc_card_sdio(host->mmc->card) &&
+		    data->blocks == 1)
+			datactrl |= MCI_DPSM_STM32_MODE_SDIO;
+		else if (data->stop && !host->mrq->sbc)
+			datactrl |= MCI_DPSM_STM32_MODE_BLOCK_STOP;
+		else
+			datactrl |= MCI_DPSM_STM32_MODE_BLOCK;
+	}
+
 	if (data->flags & MMC_DATA_READ)
 		datactrl |= MCI_DPSM_DIRECTION;
 
diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index 474f4fa..9fbd0a4 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -131,6 +131,11 @@
 /* Control register extensions in the Qualcomm versions */
 #define MCI_DPSM_QCOM_DATA_PEND	BIT(17)
 #define MCI_DPSM_QCOM_RX_DATA_PEND BIT(20)
+/* Control register extensions in STM32 versions */
+#define MCI_DPSM_STM32_MODE_BLOCK	(0 << 2)
+#define MCI_DPSM_STM32_MODE_SDIO	(1 << 2)
+#define MCI_DPSM_STM32_MODE_STREAM	(2 << 2)
+#define MCI_DPSM_STM32_MODE_BLOCK_STOP	(3 << 2)
 
 #define MMCIDATACNT		0x030
 #define MMCISTATUS		0x034
@@ -357,6 +362,8 @@ struct variant_data {
 	void (*init)(struct mmci_host *host);
 };
 
+#define MMCI_QUIRK_STM32_DTMODE	BIT(0)
+
 /* mmci variant callbacks */
 struct mmci_host_ops {
 	int (*validate_data)(struct mmci_host *host, struct mmc_data *data);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] mmc: mmci: add quirk property to add stm32 transfer mode
  2019-02-21 10:10 [PATCH 0/2] mmc: mmci: add quirk property to add stm32 transfer mode Ludovic Barre
  2019-02-21 10:10 ` [PATCH 1/2] mmc: mmci: introduce a quirks property into variant struct Ludovic Barre
  2019-02-21 10:10 ` [PATCH 2/2] mmc: mmci: add quirk property to add stm32 transfer mode Ludovic Barre
@ 2019-02-21 10:27 ` Russell King - ARM Linux admin
  2019-02-21 10:30   ` Russell King - ARM Linux admin
  2 siblings, 1 reply; 9+ messages in thread
From: Russell King - ARM Linux admin @ 2019-02-21 10:27 UTC (permalink / raw)
  To: Ludovic Barre
  Cc: Ulf Hansson, Rob Herring, devicetree, Alexandre Torgue,
	linux-mmc, linux-kernel, srinivas.kandagatla, Maxime Coquelin,
	linux-stm32, linux-arm-kernel

On Thu, Feb 21, 2019 at 11:10:49AM +0100, Ludovic Barre wrote:
> From: Ludovic Barre <ludovic.barre@st.com>
> 
> This patch series introduces a bitmap of hardware quirks that require
> some special action. This should reduce the number of boolean
> into variant structure. 
> And adds quirk bit to define sdmmc specific transfer modes.

Please find some other way to deal with these differences.  As far as
I'm concerned, introducing a quirk bitmask such as what was done in
sdhci is a complete disaster and leads to long-term maintanability
problems.

We already have a way to deal with variants in mmci.

> 
> Ludovic Barre (2):
>   mmc: mmci: introduce a quirks property into variant struct
>   mmc: mmci: add quirk property to add stm32 transfer mode
> 
>  drivers/mmc/host/mmci.c | 11 +++++++++++
>  drivers/mmc/host/mmci.h |  9 +++++++++
>  2 files changed, 20 insertions(+)
> 
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] mmc: mmci: add quirk property to add stm32 transfer mode
  2019-02-21 10:27 ` [PATCH 0/2] " Russell King - ARM Linux admin
@ 2019-02-21 10:30   ` Russell King - ARM Linux admin
  2019-02-21 13:38     ` Ludovic BARRE
  0 siblings, 1 reply; 9+ messages in thread
From: Russell King - ARM Linux admin @ 2019-02-21 10:30 UTC (permalink / raw)
  To: Ludovic Barre
  Cc: devicetree, Ulf Hansson, Alexandre Torgue, linux-mmc,
	linux-kernel, Rob Herring, srinivas.kandagatla, Maxime Coquelin,
	linux-stm32, linux-arm-kernel

On Thu, Feb 21, 2019 at 10:27:39AM +0000, Russell King - ARM Linux admin wrote:
> On Thu, Feb 21, 2019 at 11:10:49AM +0100, Ludovic Barre wrote:
> > From: Ludovic Barre <ludovic.barre@st.com>
> > 
> > This patch series introduces a bitmap of hardware quirks that require
> > some special action. This should reduce the number of boolean
> > into variant structure. 
> > And adds quirk bit to define sdmmc specific transfer modes.
> 
> Please find some other way to deal with these differences.  As far as
> I'm concerned, introducing a quirk bitmask such as what was done in
> sdhci is a complete disaster and leads to long-term maintanability
> problems.
> 
> We already have a way to deal with variants in mmci.

... to finish what I was saying ...

and I think that:

        if (variant->blksz_datactrl16)
                datactrl = variant->datactrl_dpsm_enable | (data->blksz << 16);
        else if (variant->blksz_datactrl4)
                datactrl = variant->datactrl_dpsm_enable | (data->blksz << 4);
        else
                datactrl = variant->datactrl_dpsm_enable | blksz_bits << 4;

ought to become a variant function call which returns the appropriate
datactrl value.  This would shrink the amount of variant testing in this
path, and also means that going forward we aren't facing an endlessly
increasing number of tests here.

> 
> > 
> > Ludovic Barre (2):
> >   mmc: mmci: introduce a quirks property into variant struct
> >   mmc: mmci: add quirk property to add stm32 transfer mode
> > 
> >  drivers/mmc/host/mmci.c | 11 +++++++++++
> >  drivers/mmc/host/mmci.h |  9 +++++++++
> >  2 files changed, 20 insertions(+)
> > 
> > -- 
> > 2.7.4
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] mmc: mmci: add quirk property to add stm32 transfer mode
  2019-02-21 10:30   ` Russell King - ARM Linux admin
@ 2019-02-21 13:38     ` Ludovic BARRE
  2019-02-21 14:03       ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 9+ messages in thread
From: Ludovic BARRE @ 2019-02-21 13:38 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: devicetree, Ulf Hansson, Alexandre Torgue, linux-mmc,
	linux-kernel, Rob Herring, srinivas.kandagatla, Maxime Coquelin,
	linux-stm32, linux-arm-kernel

hi Russell & Ulf

On 2/21/19 11:30 AM, Russell King - ARM Linux admin wrote:
> On Thu, Feb 21, 2019 at 10:27:39AM +0000, Russell King - ARM Linux admin wrote:
>> On Thu, Feb 21, 2019 at 11:10:49AM +0100, Ludovic Barre wrote:
>>> From: Ludovic Barre <ludovic.barre@st.com>
>>>
>>> This patch series introduces a bitmap of hardware quirks that require
>>> some special action. This should reduce the number of boolean
>>> into variant structure.
>>> And adds quirk bit to define sdmmc specific transfer modes.
>>
>> Please find some other way to deal with these differences.  As far as
>> I'm concerned, introducing a quirk bitmask such as what was done in
>> sdhci is a complete disaster and leads to long-term maintanability
>> problems.
>>
>> We already have a way to deal with variants in mmci.
> 
> ... to finish what I was saying ...
> 
> and I think that:
> 
>          if (variant->blksz_datactrl16)
>                  datactrl = variant->datactrl_dpsm_enable | (data->blksz << 16);
>          else if (variant->blksz_datactrl4)
>                  datactrl = variant->datactrl_dpsm_enable | (data->blksz << 4);
>          else
>                  datactrl = variant->datactrl_dpsm_enable | blksz_bits << 4;
> 
> ought to become a variant function call which returns the appropriate
> datactrl value.  This would shrink the amount of variant testing in this
> path, and also means that going forward we aren't facing an endlessly
> increasing number of tests here.

For blksz_datactrl case:
We could create an inline function for datactrl16 and blksz_datactrl4
which returns the appropriate datactrl value (specific for ux500v2 and 
qcom). This function could be register in mmci_host_ops structure.

in mmci_start_data function we could call a common function which call a
hook if defined.

int mmci_dblksz(struct mmci_host *host)
{
	if (host->ops && host->ops->dblksz)
		return host->ops->dblk(host);

	/* default data block size definition */
	blksz_bits = ffs(data->blksz) - 1;
	return blksz_bits << 4;
}

what do you think about it?
After, I'm afraid to multiply callback function in mmci_host_ops.

For stm32 transfer mode:
ditto, a callback function or I keep a boolean?

BR
Ludo

> 
>>
>>>
>>> Ludovic Barre (2):
>>>    mmc: mmci: introduce a quirks property into variant struct
>>>    mmc: mmci: add quirk property to add stm32 transfer mode
>>>
>>>   drivers/mmc/host/mmci.c | 11 +++++++++++
>>>   drivers/mmc/host/mmci.h |  9 +++++++++
>>>   2 files changed, 20 insertions(+)
>>>
>>> -- 
>>> 2.7.4
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>>
>> -- 
>> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
>> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
>> According to speedtest.net: 11.9Mbps down 500kbps up
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] mmc: mmci: add quirk property to add stm32 transfer mode
  2019-02-21 13:38     ` Ludovic BARRE
@ 2019-02-21 14:03       ` Russell King - ARM Linux admin
  2019-02-25 10:48         ` Ludovic BARRE
  0 siblings, 1 reply; 9+ messages in thread
From: Russell King - ARM Linux admin @ 2019-02-21 14:03 UTC (permalink / raw)
  To: Ludovic BARRE
  Cc: devicetree, Ulf Hansson, Alexandre Torgue, linux-mmc,
	linux-kernel, Rob Herring, srinivas.kandagatla, Maxime Coquelin,
	linux-stm32, linux-arm-kernel

On Thu, Feb 21, 2019 at 02:38:36PM +0100, Ludovic BARRE wrote:
> hi Russell & Ulf
> 
> On 2/21/19 11:30 AM, Russell King - ARM Linux admin wrote:
> > On Thu, Feb 21, 2019 at 10:27:39AM +0000, Russell King - ARM Linux admin wrote:
> > > On Thu, Feb 21, 2019 at 11:10:49AM +0100, Ludovic Barre wrote:
> > > > From: Ludovic Barre <ludovic.barre@st.com>
> > > > 
> > > > This patch series introduces a bitmap of hardware quirks that require
> > > > some special action. This should reduce the number of boolean
> > > > into variant structure.
> > > > And adds quirk bit to define sdmmc specific transfer modes.
> > > 
> > > Please find some other way to deal with these differences.  As far as
> > > I'm concerned, introducing a quirk bitmask such as what was done in
> > > sdhci is a complete disaster and leads to long-term maintanability
> > > problems.
> > > 
> > > We already have a way to deal with variants in mmci.
> > 
> > ... to finish what I was saying ...
> > 
> > and I think that:
> > 
> >          if (variant->blksz_datactrl16)
> >                  datactrl = variant->datactrl_dpsm_enable | (data->blksz << 16);
> >          else if (variant->blksz_datactrl4)
> >                  datactrl = variant->datactrl_dpsm_enable | (data->blksz << 4);
> >          else
> >                  datactrl = variant->datactrl_dpsm_enable | blksz_bits << 4;
> > 
> > ought to become a variant function call which returns the appropriate
> > datactrl value.  This would shrink the amount of variant testing in this
> > path, and also means that going forward we aren't facing an endlessly
> > increasing number of tests here.
> 
> For blksz_datactrl case:
> We could create an inline function for datactrl16 and blksz_datactrl4
> which returns the appropriate datactrl value (specific for ux500v2 and
> qcom). This function could be register in mmci_host_ops structure.

Yes, this is what I'm proposing (except for the "inline" bit which
seems meaningless if it's called via the mmci_host_ops structure.)
I'm also proposing that it shouldn't just be the blksz that's
returned but anything that the variant needs to take account of,
including the stm transfer mode.

> in mmci_start_data function we could call a common function which call a
> hook if defined.
> 
> int mmci_dblksz(struct mmci_host *host)

As this is returning a register value, "u32" would be more appropriate
than "int".

> {
> 	if (host->ops && host->ops->dblksz)
> 		return host->ops->dblk(host);
> 
> 	/* default data block size definition */
> 	blksz_bits = ffs(data->blksz) - 1;
> 	return blksz_bits << 4;
> }
> 
> what do you think about it?

I don't see any reason not to make the call unconditional and have every
variant supply an appropriate function pointer.  IMHO that keeps stuff
cleaner.

> After, I'm afraid to multiply callback function in mmci_host_ops.
> 
> For stm32 transfer mode:
> ditto, a callback function or I keep a boolean?
> 
> BR
> Ludo
> 
> > 
> > > 
> > > > 
> > > > Ludovic Barre (2):
> > > >    mmc: mmci: introduce a quirks property into variant struct
> > > >    mmc: mmci: add quirk property to add stm32 transfer mode
> > > > 
> > > >   drivers/mmc/host/mmci.c | 11 +++++++++++
> > > >   drivers/mmc/host/mmci.h |  9 +++++++++
> > > >   2 files changed, 20 insertions(+)
> > > > 
> > > > -- 
> > > > 2.7.4
> > > > 
> > > > 
> > > > _______________________________________________
> > > > linux-arm-kernel mailing list
> > > > linux-arm-kernel@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > > 
> > > 
> > > -- 
> > > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> > > According to speedtest.net: 11.9Mbps down 500kbps up
> > > 
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > 
> > 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] mmc: mmci: add quirk property to add stm32 transfer mode
  2019-02-21 14:03       ` Russell King - ARM Linux admin
@ 2019-02-25 10:48         ` Ludovic BARRE
  2019-02-27  9:11           ` Ulf Hansson
  0 siblings, 1 reply; 9+ messages in thread
From: Ludovic BARRE @ 2019-02-25 10:48 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: devicetree, Ulf Hansson, Alexandre Torgue, linux-mmc,
	linux-kernel, Rob Herring, srinivas.kandagatla, Maxime Coquelin,
	linux-stm32, linux-arm-kernel

hi Russell & Ulf

On 2/21/19 3:03 PM, Russell King - ARM Linux admin wrote:
> On Thu, Feb 21, 2019 at 02:38:36PM +0100, Ludovic BARRE wrote:
>> hi Russell & Ulf
>>
>> On 2/21/19 11:30 AM, Russell King - ARM Linux admin wrote:
>>> On Thu, Feb 21, 2019 at 10:27:39AM +0000, Russell King - ARM Linux admin wrote:
>>>> On Thu, Feb 21, 2019 at 11:10:49AM +0100, Ludovic Barre wrote:
>>>>> From: Ludovic Barre <ludovic.barre@st.com>
>>>>>
>>>>> This patch series introduces a bitmap of hardware quirks that require
>>>>> some special action. This should reduce the number of boolean
>>>>> into variant structure.
>>>>> And adds quirk bit to define sdmmc specific transfer modes.
>>>>
>>>> Please find some other way to deal with these differences.  As far as
>>>> I'm concerned, introducing a quirk bitmask such as what was done in
>>>> sdhci is a complete disaster and leads to long-term maintanability
>>>> problems.
>>>>
>>>> We already have a way to deal with variants in mmci.
>>>
>>> ... to finish what I was saying ...
>>>
>>> and I think that:
>>>
>>>           if (variant->blksz_datactrl16)
>>>                   datactrl = variant->datactrl_dpsm_enable | (data->blksz << 16);
>>>           else if (variant->blksz_datactrl4)
>>>                   datactrl = variant->datactrl_dpsm_enable | (data->blksz << 4);
>>>           else
>>>                   datactrl = variant->datactrl_dpsm_enable | blksz_bits << 4;
>>>
>>> ought to become a variant function call which returns the appropriate
>>> datactrl value.  This would shrink the amount of variant testing in this
>>> path, and also means that going forward we aren't facing an endlessly
>>> increasing number of tests here.
>>
>> For blksz_datactrl case:
>> We could create an inline function for datactrl16 and blksz_datactrl4
>> which returns the appropriate datactrl value (specific for ux500v2 and
>> qcom). This function could be register in mmci_host_ops structure.
> 
> Yes, this is what I'm proposing (except for the "inline" bit which
> seems meaningless if it's called via the mmci_host_ops structure.)
> I'm also proposing that it shouldn't just be the blksz that's
> returned but anything that the variant needs to take account of,
> including the stm transfer mode.

Ulf, are you alright with this callback approach (just to be sure that 
every body is align, before send a patch)?

This mmci_host_ops callback could return datactrl config to
start data (defined by variant).

> 
>> in mmci_start_data function we could call a common function which call a
>> hook if defined.
>>
>> int mmci_dblksz(struct mmci_host *host)
> 
> As this is returning a register value, "u32" would be more appropriate
> than "int".
> 
>> {
>> 	if (host->ops && host->ops->dblksz)
>> 		return host->ops->dblk(host);
>>
>> 	/* default data block size definition */
>> 	blksz_bits = ffs(data->blksz) - 1;
>> 	return blksz_bits << 4;
>> }
>>
>> what do you think about it?
> 
> I don't see any reason not to make the call unconditional and have every
> variant supply an appropriate function pointer.  IMHO that keeps stuff
> cleaner.
> 
>> After, I'm afraid to multiply callback function in mmci_host_ops.
>>
>> For stm32 transfer mode:
>> ditto, a callback function or I keep a boolean?
>>
>> BR
>> Ludo
>>
>>>
>>>>
>>>>>
>>>>> Ludovic Barre (2):
>>>>>     mmc: mmci: introduce a quirks property into variant struct
>>>>>     mmc: mmci: add quirk property to add stm32 transfer mode
>>>>>
>>>>>    drivers/mmc/host/mmci.c | 11 +++++++++++
>>>>>    drivers/mmc/host/mmci.h |  9 +++++++++
>>>>>    2 files changed, 20 insertions(+)
>>>>>
>>>>> -- 
>>>>> 2.7.4
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> linux-arm-kernel mailing list
>>>>> linux-arm-kernel@lists.infradead.org
>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>>>
>>>>
>>>> -- 
>>>> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
>>>> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
>>>> According to speedtest.net: 11.9Mbps down 500kbps up
>>>>
>>>> _______________________________________________
>>>> linux-arm-kernel mailing list
>>>> linux-arm-kernel@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>>
>>>
>>
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] mmc: mmci: add quirk property to add stm32 transfer mode
  2019-02-25 10:48         ` Ludovic BARRE
@ 2019-02-27  9:11           ` Ulf Hansson
  0 siblings, 0 replies; 9+ messages in thread
From: Ulf Hansson @ 2019-02-27  9:11 UTC (permalink / raw)
  To: Ludovic BARRE
  Cc: Russell King - ARM Linux admin, DTML, Alexandre Torgue,
	linux-mmc, Linux Kernel Mailing List, Rob Herring,
	Srinivas Kandagatla, Maxime Coquelin, linux-stm32, Linux ARM

On Mon, 25 Feb 2019 at 11:49, Ludovic BARRE <ludovic.barre@st.com> wrote:
>
> hi Russell & Ulf
>
> On 2/21/19 3:03 PM, Russell King - ARM Linux admin wrote:
> > On Thu, Feb 21, 2019 at 02:38:36PM +0100, Ludovic BARRE wrote:
> >> hi Russell & Ulf
> >>
> >> On 2/21/19 11:30 AM, Russell King - ARM Linux admin wrote:
> >>> On Thu, Feb 21, 2019 at 10:27:39AM +0000, Russell King - ARM Linux admin wrote:
> >>>> On Thu, Feb 21, 2019 at 11:10:49AM +0100, Ludovic Barre wrote:
> >>>>> From: Ludovic Barre <ludovic.barre@st.com>
> >>>>>
> >>>>> This patch series introduces a bitmap of hardware quirks that require
> >>>>> some special action. This should reduce the number of boolean
> >>>>> into variant structure.
> >>>>> And adds quirk bit to define sdmmc specific transfer modes.
> >>>>
> >>>> Please find some other way to deal with these differences.  As far as
> >>>> I'm concerned, introducing a quirk bitmask such as what was done in
> >>>> sdhci is a complete disaster and leads to long-term maintanability
> >>>> problems.
> >>>>
> >>>> We already have a way to deal with variants in mmci.
> >>>
> >>> ... to finish what I was saying ...
> >>>
> >>> and I think that:
> >>>
> >>>           if (variant->blksz_datactrl16)
> >>>                   datactrl = variant->datactrl_dpsm_enable | (data->blksz << 16);
> >>>           else if (variant->blksz_datactrl4)
> >>>                   datactrl = variant->datactrl_dpsm_enable | (data->blksz << 4);
> >>>           else
> >>>                   datactrl = variant->datactrl_dpsm_enable | blksz_bits << 4;
> >>>
> >>> ought to become a variant function call which returns the appropriate
> >>> datactrl value.  This would shrink the amount of variant testing in this
> >>> path, and also means that going forward we aren't facing an endlessly
> >>> increasing number of tests here.
> >>
> >> For blksz_datactrl case:
> >> We could create an inline function for datactrl16 and blksz_datactrl4
> >> which returns the appropriate datactrl value (specific for ux500v2 and
> >> qcom). This function could be register in mmci_host_ops structure.
> >
> > Yes, this is what I'm proposing (except for the "inline" bit which
> > seems meaningless if it's called via the mmci_host_ops structure.)
> > I'm also proposing that it shouldn't just be the blksz that's
> > returned but anything that the variant needs to take account of,
> > including the stm transfer mode.
>
> Ulf, are you alright with this callback approach (just to be sure that
> every body is align, before send a patch)?

Go ahead, let's see how it looks!

>
> This mmci_host_ops callback could return datactrl config to
> start data (defined by variant).

Yes.

[...]

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-02-27  9:11 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-21 10:10 [PATCH 0/2] mmc: mmci: add quirk property to add stm32 transfer mode Ludovic Barre
2019-02-21 10:10 ` [PATCH 1/2] mmc: mmci: introduce a quirks property into variant struct Ludovic Barre
2019-02-21 10:10 ` [PATCH 2/2] mmc: mmci: add quirk property to add stm32 transfer mode Ludovic Barre
2019-02-21 10:27 ` [PATCH 0/2] " Russell King - ARM Linux admin
2019-02-21 10:30   ` Russell King - ARM Linux admin
2019-02-21 13:38     ` Ludovic BARRE
2019-02-21 14:03       ` Russell King - ARM Linux admin
2019-02-25 10:48         ` Ludovic BARRE
2019-02-27  9:11           ` Ulf Hansson

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).