[PATCH 1/4] mci: dwcmshc-sdhci: Remove superfluous call to sdhci_enable_v4_mode

[PATCH 1/4] mci: dwcmshc-sdhci: Remove superfluous call to sdhci_enable_v4_mode

[Yann Sionneau]

sdhci_enable_v4_mode is already called from dwcmshc_mci_init which is
always called before using the controller from mci-core.c

Signed-off-by: Yann Sionneau <ysionneau@kalrayinc.com>

---
 drivers/mci/dwcmshc-sdhci.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/mci/dwcmshc-sdhci.c b/drivers/mci/dwcmshc-sdhci.c
index 6ccf195eac..7cc6fe3f18 100644
--- a/drivers/mci/dwcmshc-sdhci.c
+++ b/drivers/mci/dwcmshc-sdhci.c
@@ -318,8 +318,6 @@ static int dwcmshc_probe(struct device *dev)
 
 	mci_of_parse(&host->mci);
 
-	/* Enable host_version4 */
-	sdhci_enable_v4_mode(&host->sdhci);
 	sdhci_setup_host(&host->sdhci);
 
 	mci->max_req_size = 0x8000;
-- 
2.43.0

[PATCH 2/4] mci: dwcmshc-sdhci: Fix f_max computation

[Yann Sionneau]

f_max was possibly set from max-frequency DT property
by mci_of_parse() and then overwritten few lines later
with the clock frequency, making it impossible to
reduce the clock frequency from DT.

Move the call to mci_of_parse() after the first assignment
to f_max.

Signed-off-by: Yann Sionneau <ysionneau@kalrayinc.com>

---
 drivers/mci/dwcmshc-sdhci.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/mci/dwcmshc-sdhci.c b/drivers/mci/dwcmshc-sdhci.c
index 7cc6fe3f18..d9c51752db 100644
--- a/drivers/mci/dwcmshc-sdhci.c
+++ b/drivers/mci/dwcmshc-sdhci.c
@@ -316,12 +316,17 @@ static int dwcmshc_probe(struct device *dev)
 	mci->send_cmd = dwcmshc_mci_send_cmd;
 	mci->card_present = dwcmshc_mci_card_present;
 
-	mci_of_parse(&host->mci);
-
 	sdhci_setup_host(&host->sdhci);
 
 	mci->max_req_size = 0x8000;
+	/*
+	 * Let's first initialize f_max to the DT clock freq
+	 * Then mci_of_parse can override if with the content
+	 * of the 'max-frequency' DT property if it is present.
+	 * Then we can finish by computing the f_min.
+	 */
 	mci->f_max = clk_get_rate(clk);
+	mci_of_parse(&host->mci);
 	mci->f_min = mci->f_max / SDHCI_MAX_DIV_SPEC_300;
 
 	dev->priv = host;
-- 
2.43.0

Re: [PATCH 2/4] mci: dwcmshc-sdhci: Fix f_max computation

[Ahmad Fatoum]

On 29.02.24 16:56, Yann Sionneau wrote:
> f_max was possibly set from max-frequency DT property
> by mci_of_parse() and then overwritten few lines later
> with the clock frequency, making it impossible to
> reduce the clock frequency from DT.
> 
> Move the call to mci_of_parse() after the first assignment
> to f_max.
> 
> Signed-off-by: Yann Sionneau <ysionneau@kalrayinc.com>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> 
> ---
>  drivers/mci/dwcmshc-sdhci.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mci/dwcmshc-sdhci.c b/drivers/mci/dwcmshc-sdhci.c
> index 7cc6fe3f18..d9c51752db 100644
> --- a/drivers/mci/dwcmshc-sdhci.c
> +++ b/drivers/mci/dwcmshc-sdhci.c
> @@ -316,12 +316,17 @@ static int dwcmshc_probe(struct device *dev)
>  	mci->send_cmd = dwcmshc_mci_send_cmd;
>  	mci->card_present = dwcmshc_mci_card_present;
>  
> -	mci_of_parse(&host->mci);
> -
>  	sdhci_setup_host(&host->sdhci);
>  
>  	mci->max_req_size = 0x8000;
> +	/*
> +	 * Let's first initialize f_max to the DT clock freq
> +	 * Then mci_of_parse can override if with the content
> +	 * of the 'max-frequency' DT property if it is present.
> +	 * Then we can finish by computing the f_min.
> +	 */
>  	mci->f_max = clk_get_rate(clk);
> +	mci_of_parse(&host->mci);
>  	mci->f_min = mci->f_max / SDHCI_MAX_DIV_SPEC_300;
>  
>  	dev->priv = host;

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 3/4] mci: sdhci: add register define for P_VENDOR_SPECIFIC_AREA

[Yann Sionneau]

This register is standardized by SDHCI (See SD Host Controller
Simplified Specification v4.20 "1.2 Register Map" and
"2.3.14 Pointer Registers to mFFh-100h Area").
 
It is necessary to access some vendor specific registers for
some controllers.

Signed-off-by: Yann Sionneau <ysionneau@kalrayinc.com>
---
 drivers/mci/sdhci.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mci/sdhci.h b/drivers/mci/sdhci.h
index 4291d8d04a..d0ef54bb45 100644
--- a/drivers/mci/sdhci.h
+++ b/drivers/mci/sdhci.h
@@ -168,6 +168,8 @@
 #define SDHCI_PRESET_CLKGEN_SEL		BIT(10)
 #define SDHCI_PRESET_SDCLK_FREQ_MASK	GENMASK(9, 0)
 
+#define SDHCI_P_VENDOR_SPEC_AREA	0xE8
+#define SDHCI_P_VENDOR_SPEC_AREA_MASK	GENMASK(11, 0)
 #define SDHCI_HOST_VERSION	0xFE
 #define  SDHCI_VENDOR_VER_MASK	0xFF00
 #define  SDHCI_VENDOR_VER_SHIFT	8
-- 
2.43.0

[PATCH 4/4] mci: dwcmshc-sdhci: add support for Kalray Coolidge v2 SoC eMMC controller

[Yann Sionneau]

Kalray Coolidge v2 SoC eMMC controller needs static tx delay tuning even
for basic standard or high speed modes.

This patch also adds possibility to do some vendor specific tuning
in set_ios().
This will be needed for Coolidge v2 for >50 MHz speeds and HS200/HS400
modes.

Signed-off-by: Yann Sionneau <ysionneau@kalrayinc.com>
---
 drivers/mci/dwcmshc-sdhci.c | 40 +++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/mci/dwcmshc-sdhci.c b/drivers/mci/dwcmshc-sdhci.c
index d9c51752db..5017a1f56e 100644
--- a/drivers/mci/dwcmshc-sdhci.c
+++ b/drivers/mci/dwcmshc-sdhci.c
@@ -9,11 +9,16 @@
 #include <dma.h>
 #include <malloc.h>
 #include <mci.h>
+#include <of_device.h>
 #include <linux/err.h>
 #include <linux/clk.h>
 
 #include "sdhci.h"
 
+#define tx_delay_static_cfg(delay)      (delay << 5)
+#define tx_tuning_clk_sel(delay)        (delay)
+
+#define DWCMSHC_GPIO_OUT  0x34 /* offset from vendor specific area */
 #define CARD_STATUS_MASK (0x1e00)
 #define CARD_STATUS_TRAN (4 << 9)
 
@@ -22,6 +27,13 @@ static int do_send_cmd(struct mci_host *mci, struct mci_cmd *cmd, struct mci_dat
 struct dwcmshc_host {
 	struct mci_host mci;
 	struct sdhci sdhci;
+	int vendor_specific_area;
+	const struct dwcmshc_callbacks *cb;
+};
+
+struct dwcmshc_callbacks {
+	void (*init)(struct mci_host *mci, struct device *dev);
+	void (*set_ios)(struct mci_host *mci, struct mci_ios *ios);
 };
 
 static inline struct dwcmshc_host *priv_from_mci_host(struct mci_host *h)
@@ -222,6 +234,9 @@ static void dwcmshc_mci_set_ios(struct mci_host *mci, struct mci_ios *ios)
 		return;
 	}
 
+	if (host->cb && host->cb->set_ios)
+		host->cb->set_ios(mci, ios);
+
 	/* enable bus clock */
 	sdhci_write16(&host->sdhci, SDHCI_CLOCK_CONTROL, val | SDHCI_CLOCK_CARD_EN);
 }
@@ -253,6 +268,9 @@ static int dwcmshc_mci_init(struct mci_host *mci, struct device *dev)
 	dev_dbg(host->mci.hw_dev, "host version4: %s\n",
 		ctrl2 & SDHCI_CTRL_V4_MODE ? "enabled" : "disabled");
 
+	if (host->cb && host->cb->init)
+		host->cb->init(mci, dev);
+
 	return 0;
 }
 
@@ -284,6 +302,8 @@ static void dwcmshc_set_dma_mask(struct device *dev)
 
 static int dwcmshc_probe(struct device *dev)
 {
+	const struct dwcmshc_callbacks *dwcmshc_cb =
+			of_device_get_match_data(dev);
 	struct dwcmshc_host *host;
 	struct resource *iores;
 	struct mci_host *mci;
@@ -309,6 +329,7 @@ static int dwcmshc_probe(struct device *dev)
 	host->sdhci.base = IOMEM(iores->start);
 	host->sdhci.mci = mci;
 	host->sdhci.max_clk = clk_get_rate(clk);
+	host->cb = dwcmshc_cb;
 
 	mci->hw_dev = dev;
 	mci->init = dwcmshc_mci_init;
@@ -337,6 +358,10 @@ static int dwcmshc_probe(struct device *dev)
 	dev_dbg(host->mci.hw_dev, "host controller version: %u\n",
 		host->sdhci.version);
 
+	host->vendor_specific_area = sdhci_read32(&host->sdhci,
+						   SDHCI_P_VENDOR_SPEC_AREA);
+	host->vendor_specific_area &= SDHCI_P_VENDOR_SPEC_AREA_MASK;
+
 	ret = mci_register(&host->mci);
 	if (ret)
 		goto err_register;
@@ -354,8 +379,23 @@ static int dwcmshc_probe(struct device *dev)
 	return ret;
 }
 
+static void dwcmshc_coolidgev2_init(struct mci_host *mci, struct device *dev)
+{
+	struct dwcmshc_host *host = priv_from_mci_host(mci);
+
+	// configure TX delay to set correct setup/hold for Coolidge V2
+	sdhci_write32(&host->sdhci,
+		      host->vendor_specific_area + DWCMSHC_GPIO_OUT,
+		      tx_delay_static_cfg(0xf) | tx_tuning_clk_sel(4));
+}
+
+struct dwcmshc_callbacks kalray_coolidgev2_callbacks = {
+	.init = dwcmshc_coolidgev2_init,
+};
+
 static struct of_device_id dwcmshc_dt_ids[] = {
 	{ .compatible = "snps,dwcmshc-sdhci", },
+	{ .compatible = "kalray,coolidge-v2-dwcmshc-sdhci", .data = &kalray_coolidgev2_callbacks },
 	{ }
 };
 
-- 
2.43.0

Re: [PATCH 4/4] mci: dwcmshc-sdhci: add support for Kalray Coolidge v2 SoC eMMC controller

[Ahmad Fatoum]

Hello Yann,

On 29.02.24 16:57, Yann Sionneau wrote:
> Kalray Coolidge v2 SoC eMMC controller needs static tx delay tuning even
> for basic standard or high speed modes.
> 
> This patch also adds possibility to do some vendor specific tuning
> in set_ios().
> This will be needed for Coolidge v2 for >50 MHz speeds and HS200/HS400
> modes.
> 
> Signed-off-by: Yann Sionneau <ysionneau@kalrayinc.com>
> ---
>  drivers/mci/dwcmshc-sdhci.c | 40 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/drivers/mci/dwcmshc-sdhci.c b/drivers/mci/dwcmshc-sdhci.c
> index d9c51752db..5017a1f56e 100644
> --- a/drivers/mci/dwcmshc-sdhci.c
> +++ b/drivers/mci/dwcmshc-sdhci.c
> @@ -9,11 +9,16 @@
>  #include <dma.h>
>  #include <malloc.h>
>  #include <mci.h>
> +#include <of_device.h>
>  #include <linux/err.h>
>  #include <linux/clk.h>
>  
>  #include "sdhci.h"
>  
> +#define tx_delay_static_cfg(delay)      (delay << 5)
> +#define tx_tuning_clk_sel(delay)        (delay)
> +
> +#define DWCMSHC_GPIO_OUT  0x34 /* offset from vendor specific area */
>  #define CARD_STATUS_MASK (0x1e00)
>  #define CARD_STATUS_TRAN (4 << 9)
>  
> @@ -22,6 +27,13 @@ static int do_send_cmd(struct mci_host *mci, struct mci_cmd *cmd, struct mci_dat
>  struct dwcmshc_host {
>  	struct mci_host mci;
>  	struct sdhci sdhci;
> +	int vendor_specific_area;
> +	const struct dwcmshc_callbacks *cb;
> +};
> +
> +struct dwcmshc_callbacks {
> +	void (*init)(struct mci_host *mci, struct device *dev);

Why is dev needed? There's already mci->hw_dev and mci->mci->dev

> +	void (*set_ios)(struct mci_host *mci, struct mci_ios *ios);

You don't actually use set_ios. But I assume you intend to add in future?
Maybe add it when it's actually needed?

> +static void dwcmshc_coolidgev2_init(struct mci_host *mci, struct device *dev)
> +{
> +	struct dwcmshc_host *host = priv_from_mci_host(mci);
> +
> +	// configure TX delay to set correct setup/hold for Coolidge V2

Nite: If you are going to send v2, please change into /* */ comments
for uniformity.


Cheers,
Ahmad

> +	sdhci_write32(&host->sdhci,
> +		      host->vendor_specific_area + DWCMSHC_GPIO_OUT,
> +		      tx_delay_static_cfg(0xf) | tx_tuning_clk_sel(4));
> +}
> +
> +struct dwcmshc_callbacks kalray_coolidgev2_callbacks = {
> +	.init = dwcmshc_coolidgev2_init,
> +};
> +
>  static struct of_device_id dwcmshc_dt_ids[] = {
>  	{ .compatible = "snps,dwcmshc-sdhci", },
> +	{ .compatible = "kalray,coolidge-v2-dwcmshc-sdhci", .data = &kalray_coolidgev2_callbacks },
>  	{ }
>  };
>  

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 4/4] mci: dwcmshc-sdhci: add support for Kalray Coolidge v2 SoC eMMC controller

[Yann Sionneau]

Hello Ahmad,

On 2/29/24 17:19, Ahmad Fatoum wrote:
> Hello Yann,
>
> On 29.02.24 16:57, Yann Sionneau wrote:
>> Kalray Coolidge v2 SoC eMMC controller needs static tx delay tuning even
>> for basic standard or high speed modes.
>>
>> This patch also adds possibility to do some vendor specific tuning
>> in set_ios().
>> This will be needed for Coolidge v2 for >50 MHz speeds and HS200/HS400
>> modes.
>>
>> Signed-off-by: Yann Sionneau <ysionneau@kalrayinc.com>
>> ---
>>   drivers/mci/dwcmshc-sdhci.c | 40 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>
>> diff --git a/drivers/mci/dwcmshc-sdhci.c b/drivers/mci/dwcmshc-sdhci.c
>> index d9c51752db..5017a1f56e 100644
>> --- a/drivers/mci/dwcmshc-sdhci.c
>> +++ b/drivers/mci/dwcmshc-sdhci.c
>> @@ -9,11 +9,16 @@
>>   #include <dma.h>
>>   #include <malloc.h>
>>   #include <mci.h>
>> +#include <of_device.h>
>>   #include <linux/err.h>
>>   #include <linux/clk.h>
>>   
>>   #include "sdhci.h"
>>   
>> +#define tx_delay_static_cfg(delay)      (delay << 5)
>> +#define tx_tuning_clk_sel(delay)        (delay)
>> +
>> +#define DWCMSHC_GPIO_OUT  0x34 /* offset from vendor specific area */
>>   #define CARD_STATUS_MASK (0x1e00)
>>   #define CARD_STATUS_TRAN (4 << 9)
>>   
>> @@ -22,6 +27,13 @@ static int do_send_cmd(struct mci_host *mci, struct mci_cmd *cmd, struct mci_dat
>>   struct dwcmshc_host {
>>   	struct mci_host mci;
>>   	struct sdhci sdhci;
>> +	int vendor_specific_area;
>> +	const struct dwcmshc_callbacks *cb;
>> +};
>> +
>> +struct dwcmshc_callbacks {
>> +	void (*init)(struct mci_host *mci, struct device *dev);
> Why is dev needed? There's already mci->hw_dev and mci->mci->dev

dev is not needed per se, but I thought that since the callback is used 
to extend dwcmshc_mci_init() called from generic code:

https://elixir.bootlin.com/barebox/latest/source/drivers/mci/mci-core.c#L2015

Then it would be a good idea to provide the same arguments.

But I can remove it.

>
>> +	void (*set_ios)(struct mci_host *mci, struct mci_ios *ios);
> You don't actually use set_ios. But I assume you intend to add in future?
> Maybe add it when it's actually needed?

Ok indeed it's for the future, in the future to support HS200/HS400 high 
speed modes

vendor specific tuning (tx/rx delay lines tuning) would take place in 
such callback.

>
>> +static void dwcmshc_coolidgev2_init(struct mci_host *mci, struct device *dev)
>> +{
>> +	struct dwcmshc_host *host = priv_from_mci_host(mci);
>> +
>> +	// configure TX delay to set correct setup/hold for Coolidge V2
> Nite: If you are going to send v2, please change into /* */ comments
> for uniformity.
Ok!
>
>
> Cheers,
> Ahmad
>
>> +	sdhci_write32(&host->sdhci,
>> +		      host->vendor_specific_area + DWCMSHC_GPIO_OUT,
>> +		      tx_delay_static_cfg(0xf) | tx_tuning_clk_sel(4));
>> +}
>> +
>> +struct dwcmshc_callbacks kalray_coolidgev2_callbacks = {
>> +	.init = dwcmshc_coolidgev2_init,
>> +};
>> +
>>   static struct of_device_id dwcmshc_dt_ids[] = {
>>   	{ .compatible = "snps,dwcmshc-sdhci", },
>> +	{ .compatible = "kalray,coolidge-v2-dwcmshc-sdhci", .data = &kalray_coolidgev2_callbacks },
>>   	{ }
>>   };
>>   

Re: [PATCH 4/4] mci: dwcmshc-sdhci: add support for Kalray Coolidge v2 SoC eMMC controller

[Ahmad Fatoum]

Hello Yann,

On 29.02.24 17:31, Yann Sionneau wrote:
> On 2/29/24 17:19, Ahmad Fatoum wrote:
>> On 29.02.24 16:57, Yann Sionneau wrote:
>>> Kalray Coolidge v2 SoC eMMC controller needs static tx delay tuning even
>>> for basic standard or high speed modes.
>>> +struct dwcmshc_callbacks {
>>> +    void (*init)(struct mci_host *mci, struct device *dev);
>> Why is dev needed? There's already mci->hw_dev and mci->mci->dev
> 
> dev is not needed per se, but I thought that since the callback is used to extend dwcmshc_mci_init() called from generic code:
> 
> https://elixir.bootlin.com/barebox/latest/source/drivers/mci/mci-core.c#L2015
> 
> Then it would be a good idea to provide the same arguments.
> 
> But I can remove it.

No, I agree with your reasoning. No need to change it.

>>> +    void (*set_ios)(struct mci_host *mci, struct mci_ios *ios);
>> You don't actually use set_ios. But I assume you intend to add in future?
>> Maybe add it when it's actually needed?
> 
> Ok indeed it's for the future, in the future to support HS200/HS400 high speed modes
> 
> vendor specific tuning (tx/rx delay lines tuning) would take place in such callback.

I'd prefer you add it when it's needed.

> 
>>
>>> +static void dwcmshc_coolidgev2_init(struct mci_host *mci, struct device *dev)
>>> +{
>>> +    struct dwcmshc_host *host = priv_from_mci_host(mci);
>>> +
>>> +    // configure TX delay to set correct setup/hold for Coolidge V2
>> Nite: If you are going to send v2, please change into /* */ comments
>> for uniformity.
> Ok!

Thanks,
Ahmad

>>
>>
>> Cheers,
>> Ahmad
>>
>>> +    sdhci_write32(&host->sdhci,
>>> +              host->vendor_specific_area + DWCMSHC_GPIO_OUT,
>>> +              tx_delay_static_cfg(0xf) | tx_tuning_clk_sel(4));
>>> +}
>>> +
>>> +struct dwcmshc_callbacks kalray_coolidgev2_callbacks = {
>>> +    .init = dwcmshc_coolidgev2_init,
>>> +};
>>> +
>>>   static struct of_device_id dwcmshc_dt_ids[] = {
>>>       { .compatible = "snps,dwcmshc-sdhci", },
>>> +    { .compatible = "kalray,coolidge-v2-dwcmshc-sdhci", .data = &kalray_coolidgev2_callbacks },
>>>       { }
>>>   };
>>>   
> 
> 
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: (subset) [PATCH 1/4] mci: dwcmshc-sdhci: Remove superfluous call to sdhci_enable_v4_mode

[Sascha Hauer]

On Thu, 29 Feb 2024 16:56:57 +0100, Yann Sionneau wrote:
> sdhci_enable_v4_mode is already called from dwcmshc_mci_init which is
> always called before using the controller from mci-core.c
> 
> 

Applied, thanks!

[1/4] mci: dwcmshc-sdhci: Remove superfluous call to sdhci_enable_v4_mode
      https://git.pengutronix.de/cgit/barebox/commit/?id=932b51f4306b (link may not be stable)
[2/4] mci: dwcmshc-sdhci: Fix f_max computation
      https://git.pengutronix.de/cgit/barebox/commit/?id=c1d93168325e (link may not be stable)
[3/4] mci: sdhci: add register define for P_VENDOR_SPECIFIC_AREA
      https://git.pengutronix.de/cgit/barebox/commit/?id=8cada69e81be (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>