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

[Yann Sionneau <ysionneau@kalrayinc.com>]

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 },
>>   	{ }
>>   };
>>