Adding a .platform_init callback to sdhci_arasan_ops

Adding a .platform_init callback to sdhci_arasan_ops

[Sebastian Frias]

Hi,

When using the Arasan SDHCI HW IP, there is a set of parameters called
"Hardware initialized registers"

(Table 7, Section "Pin Signals", page 56 of Arasan "SD3.0/SDIO3.0/eMMC4.4
AHB Host Controller", revision 6.0 document)

In some platforms those signals are connected to registers that need to
be programmed at some point for proper driver/HW initialisation.

I found that the 'struct sdhci_ops' contains a '.platform_init' callback
that is called from within 'sdhci_pltfm_init', and that seems a good
candidate for a place to program those registers (*).

Do you agree?

Best regards,

Sebastian


(*): This has been prototyped on 4.7 as working properly.
However, upstream commit:

commit 3ea4666e8d429223fbb39c1dccee7599ef7657d5
Author: Douglas Anderson <dianders@chromium.org>
Date:   Mon Jun 20 10:56:47 2016 -0700

    mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399
...

could affect this solution because of the way the 'sdhci_arasan_of_match'
struct is used after that commit.

Re: Adding a .platform_init callback to sdhci_arasan_ops

[Michal Simek]

+Sai for Xilinx perspective.

On 25.11.2016 16:24, Sebastian Frias wrote:
> Hi,
> 
> When using the Arasan SDHCI HW IP, there is a set of parameters called
> "Hardware initialized registers"
> 
> (Table 7, Section "Pin Signals", page 56 of Arasan "SD3.0/SDIO3.0/eMMC4.4
> AHB Host Controller", revision 6.0 document)
> 
> In some platforms those signals are connected to registers that need to
> be programmed at some point for proper driver/HW initialisation.
> 
> I found that the 'struct sdhci_ops' contains a '.platform_init' callback
> that is called from within 'sdhci_pltfm_init', and that seems a good
> candidate for a place to program those registers (*).
> 
> Do you agree?
> 
> Best regards,
> 
> Sebastian
> 
> 
> (*): This has been prototyped on 4.7 as working properly.
> However, upstream commit:
> 
> commit 3ea4666e8d429223fbb39c1dccee7599ef7657d5
> Author: Douglas Anderson <dianders@chromium.org>
> Date:   Mon Jun 20 10:56:47 2016 -0700
> 
>     mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399
> ...
> 
> could affect this solution because of the way the 'sdhci_arasan_of_match'
> struct is used after that commit.
> 

Re: Adding a .platform_init callback to sdhci_arasan_ops

[Adrian Hunter]

On 28/11/16 09:32, Michal Simek wrote:
> +Sai for Xilinx perspective.
> 
> On 25.11.2016 16:24, Sebastian Frias wrote:
>> Hi,
>>
>> When using the Arasan SDHCI HW IP, there is a set of parameters called
>> "Hardware initialized registers"
>>
>> (Table 7, Section "Pin Signals", page 56 of Arasan "SD3.0/SDIO3.0/eMMC4.4
>> AHB Host Controller", revision 6.0 document)
>>
>> In some platforms those signals are connected to registers that need to
>> be programmed at some point for proper driver/HW initialisation.
>>
>> I found that the 'struct sdhci_ops' contains a '.platform_init' callback
>> that is called from within 'sdhci_pltfm_init', and that seems a good
>> candidate for a place to program those registers (*).
>>
>> Do you agree?

We already killed .platform_init

What is wrong with sdhci_arasan_probe()?

>>
>> Best regards,
>>
>> Sebastian
>>
>>
>> (*): This has been prototyped on 4.7 as working properly.
>> However, upstream commit:
>>
>> commit 3ea4666e8d429223fbb39c1dccee7599ef7657d5
>> Author: Douglas Anderson <dianders@chromium.org>
>> Date:   Mon Jun 20 10:56:47 2016 -0700
>>
>>     mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399
>> ...
>>
>> could affect this solution because of the way the 'sdhci_arasan_of_match'
>> struct is used after that commit.
>>
> 
> 

Re: Adding a .platform_init callback to sdhci_arasan_ops

[Sebastian Frias]

Hi Adrian,

On 28/11/16 11:30, Adrian Hunter wrote:
> On 28/11/16 09:32, Michal Simek wrote:
>> +Sai for Xilinx perspective.
>>
>> On 25.11.2016 16:24, Sebastian Frias wrote:
>>> Hi,
>>>
>>> When using the Arasan SDHCI HW IP, there is a set of parameters called
>>> "Hardware initialized registers"
>>>
>>> (Table 7, Section "Pin Signals", page 56 of Arasan "SD3.0/SDIO3.0/eMMC4.4
>>> AHB Host Controller", revision 6.0 document)
>>>
>>> In some platforms those signals are connected to registers that need to
>>> be programmed at some point for proper driver/HW initialisation.
>>>
>>> I found that the 'struct sdhci_ops' contains a '.platform_init' callback
>>> that is called from within 'sdhci_pltfm_init', and that seems a good
>>> candidate for a place to program those registers (*).
>>>
>>> Do you agree?
> 
> We already killed .platform_init

I just saw that, yet it was the perfect place for the HW initialisation I'm
talking about.
Any way we can restore it?

> 
> What is wrong with sdhci_arasan_probe()?

Well, in 4.7 sdhci_arasan_probe() did not call of_match_device(), so I had
put a call to it just before sdhci_pltfm_init(), something like:

+static const struct of_device_id sdhci_arasan_of_match[] = {
+       {
+               .compatible = "arasan,sdhci-8.9a",
+               .data = &sdhci_arasan_ops,
+       },
+       {
+               .compatible = "arasan,sdhci-5.1",
+               .data = &sdhci_arasan_ops,
+       },
+       {
+               .compatible = "arasan,sdhci-4.9a",
+               .data = &sdhci_arasan_ops,
+       },
+       {
+               .compatible = "sigma,smp8734-sdio",
+               .data = &sdhci_arasan_tango4_ops,
+       },
+       { }
+};
+MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);

...

+       const struct of_device_id *match;
+
+       match = of_match_device(sdhci_arasan_of_match, &pdev->dev);
+       if (match)
+               sdhci_arasan_pdata.ops = match->data;

where 'sdhci_arasan_tango4_ops' contained a pointer to a .platform_init
callback.

However, as I stated earlier, an upstream commit:

commit 3ea4666e8d429223fbb39c1dccee7599ef7657d5
Author: Douglas Anderson <dianders@chromium.org>
Date:   Mon Jun 20 10:56:47 2016 -0700

    mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399

changed struct 'sdhci_arasan_of_match' to convey different data, which
means that instead of having a generic way of accessing such data (such
as 'of_match_device()' and ".data" field), one must also check for
specific "compatible" strings to make sense of the ".data" field, such as
"rockchip,rk3399-sdhci-5.1"

With the current code:
- there's no 'of_match_device()' before 'sdhci_pltfm_init()'
- the sdhci_pltfm_init() call is made with a static 'sdhci_arasan_pdata'
struct (so it cannot be made dependent on the "compatible" string).
- since 'sdhci_arasan_pdata' is the same for all compatible devices, even
for those that require special handling, more "compatible" matching code is
required
- leading to spread "compatible" matching code; IMHO it would be cleaner if
the 'sdhci_arasan_probe()' code was generic, with just a generic "compatible"
matching, which then proceeded with specific initialisation and generic
initialisation.

In a nutshell, IMHO it would be better if adding support for more SoCs only
involved changing just 'sdhci_arasan_of_match' without the need to change
'sdhci_arasan_probe()'.
That would clearly separate the generic and "SoC"-specific code, thus allowing
better maintenance.

Does that makes sense to you guys?

Best regards,

Sebastian

> 
>>>
>>> Best regards,
>>>
>>> Sebastian
>>>
>>>
>>> (*): This has been prototyped on 4.7 as working properly.
>>> However, upstream commit:
>>>
>>> commit 3ea4666e8d429223fbb39c1dccee7599ef7657d5
>>> Author: Douglas Anderson <dianders@chromium.org>
>>> Date:   Mon Jun 20 10:56:47 2016 -0700
>>>
>>>     mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399
>>> ...
>>>
>>> could affect this solution because of the way the 'sdhci_arasan_of_match'
>>> struct is used after that commit.
>>>
>>
>>
> 

Re: Adding a .platform_init callback to sdhci_arasan_ops

[Adrian Hunter]

On 28/11/16 13:20, Sebastian Frias wrote:
> Hi Adrian,
> 
> On 28/11/16 11:30, Adrian Hunter wrote:
>> On 28/11/16 09:32, Michal Simek wrote:
>>> +Sai for Xilinx perspective.
>>>
>>> On 25.11.2016 16:24, Sebastian Frias wrote:
>>>> Hi,
>>>>
>>>> When using the Arasan SDHCI HW IP, there is a set of parameters called
>>>> "Hardware initialized registers"
>>>>
>>>> (Table 7, Section "Pin Signals", page 56 of Arasan "SD3.0/SDIO3.0/eMMC4.4
>>>> AHB Host Controller", revision 6.0 document)
>>>>
>>>> In some platforms those signals are connected to registers that need to
>>>> be programmed at some point for proper driver/HW initialisation.
>>>>
>>>> I found that the 'struct sdhci_ops' contains a '.platform_init' callback
>>>> that is called from within 'sdhci_pltfm_init', and that seems a good
>>>> candidate for a place to program those registers (*).
>>>>
>>>> Do you agree?
>>
>> We already killed .platform_init
> 
> I just saw that, yet it was the perfect place for the HW initialisation I'm
> talking about.
> Any way we can restore it?

It doesn't serve any purpose I am aware of.

> 
>>
>> What is wrong with sdhci_arasan_probe()?
> 
> Well, in 4.7 sdhci_arasan_probe() did not call of_match_device(), so I had
> put a call to it just before sdhci_pltfm_init(), something like:
> 
> +static const struct of_device_id sdhci_arasan_of_match[] = {
> +       {
> +               .compatible = "arasan,sdhci-8.9a",
> +               .data = &sdhci_arasan_ops,
> +       },
> +       {
> +               .compatible = "arasan,sdhci-5.1",
> +               .data = &sdhci_arasan_ops,
> +       },
> +       {
> +               .compatible = "arasan,sdhci-4.9a",
> +               .data = &sdhci_arasan_ops,
> +       },
> +       {
> +               .compatible = "sigma,smp8734-sdio",
> +               .data = &sdhci_arasan_tango4_ops,
> +       },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
> 
> ...
> 
> +       const struct of_device_id *match;
> +
> +       match = of_match_device(sdhci_arasan_of_match, &pdev->dev);
> +       if (match)
> +               sdhci_arasan_pdata.ops = match->data;
> 
> where 'sdhci_arasan_tango4_ops' contained a pointer to a .platform_init
> callback.
> 
> However, as I stated earlier, an upstream commit:
> 
> commit 3ea4666e8d429223fbb39c1dccee7599ef7657d5
> Author: Douglas Anderson <dianders@chromium.org>
> Date:   Mon Jun 20 10:56:47 2016 -0700
> 
>     mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399
> 
> changed struct 'sdhci_arasan_of_match' to convey different data, which
> means that instead of having a generic way of accessing such data (such
> as 'of_match_device()' and ".data" field), one must also check for
> specific "compatible" strings to make sense of the ".data" field, such as
> "rockchip,rk3399-sdhci-5.1"
> 
> With the current code:
> - there's no 'of_match_device()' before 'sdhci_pltfm_init()'
> - the sdhci_pltfm_init() call is made with a static 'sdhci_arasan_pdata'
> struct (so it cannot be made dependent on the "compatible" string).
> - since 'sdhci_arasan_pdata' is the same for all compatible devices, even
> for those that require special handling, more "compatible" matching code is
> required
> - leading to spread "compatible" matching code; IMHO it would be cleaner if
> the 'sdhci_arasan_probe()' code was generic, with just a generic "compatible"
> matching, which then proceeded with specific initialisation and generic
> initialisation.
> 
> In a nutshell, IMHO it would be better if adding support for more SoCs only
> involved changing just 'sdhci_arasan_of_match' without the need to change
> 'sdhci_arasan_probe()'.
> That would clearly separate the generic and "SoC"-specific code, thus allowing
> better maintenance.
> 
> Does that makes sense to you guys?

If you want to do that, then why not define your match data with your own
callbacks. e.g. something like

struct sdhci_arasan_of_data {
	struct sdhci_arasan_soc_ctl_map *soc_ctl_map;
	void (*platform_init)(struct sdhci_arasan_data *sdhci_arasan);
};

	struct sdhci_arasan_of_data *data;

	data = match->data;
	sdhci_arasan->soc_ctl_map = data->soc_ctl_map;
	if (data->platform_init)
		platform_init(sdhci_arasan);

> 
> Best regards,
> 
> Sebastian
> 
>>
>>>>
>>>> Best regards,
>>>>
>>>> Sebastian
>>>>
>>>>
>>>> (*): This has been prototyped on 4.7 as working properly.
>>>> However, upstream commit:
>>>>
>>>> commit 3ea4666e8d429223fbb39c1dccee7599ef7657d5
>>>> Author: Douglas Anderson <dianders@chromium.org>
>>>> Date:   Mon Jun 20 10:56:47 2016 -0700
>>>>
>>>>     mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399
>>>> ...
>>>>
>>>> could affect this solution because of the way the 'sdhci_arasan_of_match'
>>>> struct is used after that commit.
>>>>
>>>
>>>
>>
> 
> 

Re: Adding a .platform_init callback to sdhci_arasan_ops

[Sebastian Frias]

On 28/11/16 12:44, Adrian Hunter wrote:
> On 28/11/16 13:20, Sebastian Frias wrote:
>> Hi Adrian,
>>
>> On 28/11/16 11:30, Adrian Hunter wrote:
>>> On 28/11/16 09:32, Michal Simek wrote:
>>>> +Sai for Xilinx perspective.
>>>>
>>>> On 25.11.2016 16:24, Sebastian Frias wrote:
>>>>> Hi,
>>>>>
>>>>> When using the Arasan SDHCI HW IP, there is a set of parameters called
>>>>> "Hardware initialized registers"
>>>>>
>>>>> (Table 7, Section "Pin Signals", page 56 of Arasan "SD3.0/SDIO3.0/eMMC4.4
>>>>> AHB Host Controller", revision 6.0 document)
>>>>>
>>>>> In some platforms those signals are connected to registers that need to
>>>>> be programmed at some point for proper driver/HW initialisation.
>>>>>
>>>>> I found that the 'struct sdhci_ops' contains a '.platform_init' callback
>>>>> that is called from within 'sdhci_pltfm_init', and that seems a good
>>>>> candidate for a place to program those registers (*).
>>>>>
>>>>> Do you agree?
>>>
>>> We already killed .platform_init
>>
>> I just saw that, yet it was the perfect place for the HW initialisation I'm
>> talking about.
>> Any way we can restore it?
> 
> It doesn't serve any purpose I am aware of.

It would serve (for me) if it was there :-)

> 
>>
>>>
>>> What is wrong with sdhci_arasan_probe()?
>>
>> Well, in 4.7 sdhci_arasan_probe() did not call of_match_device(), so I had
>> put a call to it just before sdhci_pltfm_init(), something like:
>>
>> +static const struct of_device_id sdhci_arasan_of_match[] = {
>> +       {
>> +               .compatible = "arasan,sdhci-8.9a",
>> +               .data = &sdhci_arasan_ops,
>> +       },
>> +       {
>> +               .compatible = "arasan,sdhci-5.1",
>> +               .data = &sdhci_arasan_ops,
>> +       },
>> +       {
>> +               .compatible = "arasan,sdhci-4.9a",
>> +               .data = &sdhci_arasan_ops,
>> +       },
>> +       {
>> +               .compatible = "sigma,smp8734-sdio",
>> +               .data = &sdhci_arasan_tango4_ops,
>> +       },
>> +       { }
>> +};
>> +MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
>>
>> ...
>>
>> +       const struct of_device_id *match;
>> +
>> +       match = of_match_device(sdhci_arasan_of_match, &pdev->dev);
>> +       if (match)
>> +               sdhci_arasan_pdata.ops = match->data;
>>
>> where 'sdhci_arasan_tango4_ops' contained a pointer to a .platform_init
>> callback.
>>
>> However, as I stated earlier, an upstream commit:
>>
>> commit 3ea4666e8d429223fbb39c1dccee7599ef7657d5
>> Author: Douglas Anderson <dianders@chromium.org>
>> Date:   Mon Jun 20 10:56:47 2016 -0700
>>
>>     mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399
>>
>> changed struct 'sdhci_arasan_of_match' to convey different data, which
>> means that instead of having a generic way of accessing such data (such
>> as 'of_match_device()' and ".data" field), one must also check for
>> specific "compatible" strings to make sense of the ".data" field, such as
>> "rockchip,rk3399-sdhci-5.1"
>>
>> With the current code:
>> - there's no 'of_match_device()' before 'sdhci_pltfm_init()'
>> - the sdhci_pltfm_init() call is made with a static 'sdhci_arasan_pdata'
>> struct (so it cannot be made dependent on the "compatible" string).
>> - since 'sdhci_arasan_pdata' is the same for all compatible devices, even
>> for those that require special handling, more "compatible" matching code is
>> required
>> - leading to spread "compatible" matching code; IMHO it would be cleaner if
>> the 'sdhci_arasan_probe()' code was generic, with just a generic "compatible"
>> matching, which then proceeded with specific initialisation and generic
>> initialisation.
>>
>> In a nutshell, IMHO it would be better if adding support for more SoCs only
>> involved changing just 'sdhci_arasan_of_match' without the need to change
>> 'sdhci_arasan_probe()'.
>> That would clearly separate the generic and "SoC"-specific code, thus allowing
>> better maintenance.
>>
>> Does that makes sense to you guys?
> 
> If you want to do that, then why not define your match data with your own
> callbacks. e.g. something like
> 
> struct sdhci_arasan_of_data {
> 	struct sdhci_arasan_soc_ctl_map *soc_ctl_map;
> 	void (*platform_init)(struct sdhci_arasan_data *sdhci_arasan);
> };
> 
> 	struct sdhci_arasan_of_data *data;
> 
> 	data = match->data;
> 	sdhci_arasan->soc_ctl_map = data->soc_ctl_map;
> 	if (data->platform_init)
> 		platform_init(sdhci_arasan);

Well, that adds a level in the hierarchy, but here is what it would look like:


diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index 410a55b..1cb3861 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -382,22 +382,6 @@ static int sdhci_arasan_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(sdhci_arasan_dev_pm_ops, sdhci_arasan_suspend,
 			 sdhci_arasan_resume);
 
-static const struct of_device_id sdhci_arasan_of_match[] = {
-	/* SoC-specific compatible strings w/ soc_ctl_map */
-	{
-		.compatible = "rockchip,rk3399-sdhci-5.1",
-		.data = &rk3399_soc_ctl_map,
-	},
-
-	/* Generic compatible below here */
-	{ .compatible = "arasan,sdhci-8.9a" },
-	{ .compatible = "arasan,sdhci-5.1" },
-	{ .compatible = "arasan,sdhci-4.9a" },
-
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
-
 /**
  * sdhci_arasan_sdcardclk_recalc_rate - Return the card clock rate
  *
@@ -578,6 +562,53 @@ static void sdhci_arasan_unregister_sdclk(struct device *dev)
 	of_clk_del_provider(dev->of_node);
 }
 
+static void sdhci_tango4_platform_init(struct sdhci_host *host)
+{
+	printk("%s\n", __func__);
+
+	/*
+	  pad_mode[2:0]=0    must be 0
+	  sel_sdio[3]=1      must be 1 for SDIO
+	  inv_sdwp_pol[4]=0  if set inverts the SD write protect polarity
+	  inv_sdcd_pol[5]=0  if set inverts the SD card present polarity
+	*/
+	sdhci_writel(host, 0x00000008, 0x100 + 0x0);
+}
+
+struct sdhci_arasan_chip_specific_data {
+	const struct sdhci_arasan_soc_ctl_map *soc_ctl_map;
+	void (*platform_init)(struct sdhci_host *host);
+};
+
+static const struct sdhci_arasan_chip_specific_data sdhci_arasan_rockchip = {
+	.soc_ctl_map = &rk3399_soc_ctl_map,
+};
+
+static const struct sdhci_arasan_chip_specific_data sdhci_arasan_sigma = {
+	.platform_init = sdhci_tango4_platform_init,
+};
+
+static const struct of_device_id sdhci_arasan_of_match[] = {
+	/* SoC-specific compatible strings w/ soc_ctl_map */
+	{
+		.compatible = "rockchip,rk3399-sdhci-5.1",
+		.data = &sdhci_arasan_rockchip,
+	},
+	{
+		.compatible = "sigma,sdio-v1",
+		.data = &sdhci_arasan_sigma,
+	},
+
+	/* Generic compatible below here */
+	{ .compatible = "arasan,sdhci-8.9a" },
+	{ .compatible = "arasan,sdhci-5.1" },
+	{ .compatible = "arasan,sdhci-4.9a" },
+
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
+
+
 static int sdhci_arasan_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -587,6 +618,7 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 	struct sdhci_host *host;
 	struct sdhci_pltfm_host *pltfm_host;
 	struct sdhci_arasan_data *sdhci_arasan;
+	struct sdhci_arasan_chip_specific_data *sdhci_arasan_chip_specific;
 	struct device_node *np = pdev->dev.of_node;
 
 	host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata,
@@ -599,7 +631,11 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 	sdhci_arasan->host = host;
 
 	match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node);
-	sdhci_arasan->soc_ctl_map = match->data;
+	sdhci_arasan_chip_specific = (struct sdhci_arasan_chip_specific_data *)match;
+	if (sdhci_arasan_chip_specific->soc_ctl_map)
+		sdhci_arasan->soc_ctl_map = sdhci_arasan_chip_specific->soc_ctl_map;
+	if (sdhci_arasan_chip_specific->platform_init)
+		sdhci_arasan_chip_specific->platform_init(host);
 
 	node = of_parse_phandle(pdev->dev.of_node, "arasan,soc-ctl-syscon", 0);
 	if (node) {


I will try to send another patch with what a different approach

Re: Adding a .platform_init callback to sdhci_arasan_ops

[Sebastian Frias]

On 28/11/16 14:28, Sebastian Frias wrote:
> On 28/11/16 12:44, Adrian Hunter wrote:
>> On 28/11/16 13:20, Sebastian Frias wrote:
>>> Hi Adrian,
>>>
>>> On 28/11/16 11:30, Adrian Hunter wrote:
>>>> On 28/11/16 09:32, Michal Simek wrote:
>>>>> +Sai for Xilinx perspective.
>>>>>
>>>>> On 25.11.2016 16:24, Sebastian Frias wrote:
>>>>>> Hi,
>>>>>>
>>>>>> When using the Arasan SDHCI HW IP, there is a set of parameters called
>>>>>> "Hardware initialized registers"
>>>>>>
>>>>>> (Table 7, Section "Pin Signals", page 56 of Arasan "SD3.0/SDIO3.0/eMMC4.4
>>>>>> AHB Host Controller", revision 6.0 document)
>>>>>>
>>>>>> In some platforms those signals are connected to registers that need to
>>>>>> be programmed at some point for proper driver/HW initialisation.
>>>>>>
>>>>>> I found that the 'struct sdhci_ops' contains a '.platform_init' callback
>>>>>> that is called from within 'sdhci_pltfm_init', and that seems a good
>>>>>> candidate for a place to program those registers (*).
>>>>>>
>>>>>> Do you agree?
>>>>
>>>> We already killed .platform_init
>>>
>>> I just saw that, yet it was the perfect place for the HW initialisation I'm
>>> talking about.
>>> Any way we can restore it?
>>
>> It doesn't serve any purpose I am aware of.
> 
> It would serve (for me) if it was there :-)
> 
>>
>>>
>>>>
>>>> What is wrong with sdhci_arasan_probe()?
>>>
>>> Well, in 4.7 sdhci_arasan_probe() did not call of_match_device(), so I had
>>> put a call to it just before sdhci_pltfm_init(), something like:
>>>
>>> +static const struct of_device_id sdhci_arasan_of_match[] = {
>>> +       {
>>> +               .compatible = "arasan,sdhci-8.9a",
>>> +               .data = &sdhci_arasan_ops,
>>> +       },
>>> +       {
>>> +               .compatible = "arasan,sdhci-5.1",
>>> +               .data = &sdhci_arasan_ops,
>>> +       },
>>> +       {
>>> +               .compatible = "arasan,sdhci-4.9a",
>>> +               .data = &sdhci_arasan_ops,
>>> +       },
>>> +       {
>>> +               .compatible = "sigma,smp8734-sdio",
>>> +               .data = &sdhci_arasan_tango4_ops,
>>> +       },
>>> +       { }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
>>>
>>> ...
>>>
>>> +       const struct of_device_id *match;
>>> +
>>> +       match = of_match_device(sdhci_arasan_of_match, &pdev->dev);
>>> +       if (match)
>>> +               sdhci_arasan_pdata.ops = match->data;
>>>
>>> where 'sdhci_arasan_tango4_ops' contained a pointer to a .platform_init
>>> callback.
>>>
>>> However, as I stated earlier, an upstream commit:
>>>
>>> commit 3ea4666e8d429223fbb39c1dccee7599ef7657d5
>>> Author: Douglas Anderson <dianders@chromium.org>
>>> Date:   Mon Jun 20 10:56:47 2016 -0700
>>>
>>>     mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399
>>>
>>> changed struct 'sdhci_arasan_of_match' to convey different data, which
>>> means that instead of having a generic way of accessing such data (such
>>> as 'of_match_device()' and ".data" field), one must also check for
>>> specific "compatible" strings to make sense of the ".data" field, such as
>>> "rockchip,rk3399-sdhci-5.1"
>>>
>>> With the current code:
>>> - there's no 'of_match_device()' before 'sdhci_pltfm_init()'
>>> - the sdhci_pltfm_init() call is made with a static 'sdhci_arasan_pdata'
>>> struct (so it cannot be made dependent on the "compatible" string).
>>> - since 'sdhci_arasan_pdata' is the same for all compatible devices, even
>>> for those that require special handling, more "compatible" matching code is
>>> required
>>> - leading to spread "compatible" matching code; IMHO it would be cleaner if
>>> the 'sdhci_arasan_probe()' code was generic, with just a generic "compatible"
>>> matching, which then proceeded with specific initialisation and generic
>>> initialisation.
>>>
>>> In a nutshell, IMHO it would be better if adding support for more SoCs only
>>> involved changing just 'sdhci_arasan_of_match' without the need to change
>>> 'sdhci_arasan_probe()'.
>>> That would clearly separate the generic and "SoC"-specific code, thus allowing
>>> better maintenance.
>>>
>>> Does that makes sense to you guys?
>>
>> If you want to do that, then why not define your match data with your own
>> callbacks. e.g. something like
>>
>> struct sdhci_arasan_of_data {
>> 	struct sdhci_arasan_soc_ctl_map *soc_ctl_map;
>> 	void (*platform_init)(struct sdhci_arasan_data *sdhci_arasan);
>> };
>>
>> 	struct sdhci_arasan_of_data *data;
>>
>> 	data = match->data;
>> 	sdhci_arasan->soc_ctl_map = data->soc_ctl_map;
>> 	if (data->platform_init)
>> 		platform_init(sdhci_arasan);
> 
> Well, that adds a level in the hierarchy, but here is what it would look like:
> 
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 410a55b..1cb3861 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -382,22 +382,6 @@ static int sdhci_arasan_resume(struct device *dev)
>  static SIMPLE_DEV_PM_OPS(sdhci_arasan_dev_pm_ops, sdhci_arasan_suspend,
>  			 sdhci_arasan_resume);
>  
> -static const struct of_device_id sdhci_arasan_of_match[] = {
> -	/* SoC-specific compatible strings w/ soc_ctl_map */
> -	{
> -		.compatible = "rockchip,rk3399-sdhci-5.1",
> -		.data = &rk3399_soc_ctl_map,
> -	},
> -
> -	/* Generic compatible below here */
> -	{ .compatible = "arasan,sdhci-8.9a" },
> -	{ .compatible = "arasan,sdhci-5.1" },
> -	{ .compatible = "arasan,sdhci-4.9a" },
> -
> -	{ /* sentinel */ }
> -};
> -MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
> -
>  /**
>   * sdhci_arasan_sdcardclk_recalc_rate - Return the card clock rate
>   *
> @@ -578,6 +562,53 @@ static void sdhci_arasan_unregister_sdclk(struct device *dev)
>  	of_clk_del_provider(dev->of_node);
>  }
>  
> +static void sdhci_tango4_platform_init(struct sdhci_host *host)
> +{
> +	printk("%s\n", __func__);
> +
> +	/*
> +	  pad_mode[2:0]=0    must be 0
> +	  sel_sdio[3]=1      must be 1 for SDIO
> +	  inv_sdwp_pol[4]=0  if set inverts the SD write protect polarity
> +	  inv_sdcd_pol[5]=0  if set inverts the SD card present polarity
> +	*/
> +	sdhci_writel(host, 0x00000008, 0x100 + 0x0);
> +}
> +
> +struct sdhci_arasan_chip_specific_data {
> +	const struct sdhci_arasan_soc_ctl_map *soc_ctl_map;
> +	void (*platform_init)(struct sdhci_host *host);
> +};
> +
> +static const struct sdhci_arasan_chip_specific_data sdhci_arasan_rockchip = {
> +	.soc_ctl_map = &rk3399_soc_ctl_map,
> +};
> +
> +static const struct sdhci_arasan_chip_specific_data sdhci_arasan_sigma = {
> +	.platform_init = sdhci_tango4_platform_init,
> +};
> +
> +static const struct of_device_id sdhci_arasan_of_match[] = {
> +	/* SoC-specific compatible strings w/ soc_ctl_map */
> +	{
> +		.compatible = "rockchip,rk3399-sdhci-5.1",
> +		.data = &sdhci_arasan_rockchip,
> +	},
> +	{
> +		.compatible = "sigma,sdio-v1",
> +		.data = &sdhci_arasan_sigma,
> +	},
> +
> +	/* Generic compatible below here */
> +	{ .compatible = "arasan,sdhci-8.9a" },
> +	{ .compatible = "arasan,sdhci-5.1" },
> +	{ .compatible = "arasan,sdhci-4.9a" },
> +
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
> +
> +
>  static int sdhci_arasan_probe(struct platform_device *pdev)
>  {
>  	int ret;
> @@ -587,6 +618,7 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>  	struct sdhci_host *host;
>  	struct sdhci_pltfm_host *pltfm_host;
>  	struct sdhci_arasan_data *sdhci_arasan;
> +	struct sdhci_arasan_chip_specific_data *sdhci_arasan_chip_specific;
>  	struct device_node *np = pdev->dev.of_node;
>  
>  	host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata,
> @@ -599,7 +631,11 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>  	sdhci_arasan->host = host;
>  
>  	match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node);
> -	sdhci_arasan->soc_ctl_map = match->data;
> +	sdhci_arasan_chip_specific = (struct sdhci_arasan_chip_specific_data *)match;
> +	if (sdhci_arasan_chip_specific->soc_ctl_map)
> +		sdhci_arasan->soc_ctl_map = sdhci_arasan_chip_specific->soc_ctl_map;
> +	if (sdhci_arasan_chip_specific->platform_init)
> +		sdhci_arasan_chip_specific->platform_init(host);
>  
>  	node = of_parse_phandle(pdev->dev.of_node, "arasan,soc-ctl-syscon", 0);
>  	if (node) {
> 
> 
> I will try to send another patch with what a different approach
> 

Here's a different approach (I just tested that it built, because I don't have the
rk3399 platform):

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index 410a55b..5be6e67 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -382,22 +382,6 @@ static int sdhci_arasan_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(sdhci_arasan_dev_pm_ops, sdhci_arasan_suspend,
 			 sdhci_arasan_resume);
 
-static const struct of_device_id sdhci_arasan_of_match[] = {
-	/* SoC-specific compatible strings w/ soc_ctl_map */
-	{
-		.compatible = "rockchip,rk3399-sdhci-5.1",
-		.data = &rk3399_soc_ctl_map,
-	},
-
-	/* Generic compatible below here */
-	{ .compatible = "arasan,sdhci-8.9a" },
-	{ .compatible = "arasan,sdhci-5.1" },
-	{ .compatible = "arasan,sdhci-4.9a" },
-
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
-
 /**
  * sdhci_arasan_sdcardclk_recalc_rate - Return the card clock rate
  *
@@ -578,28 +562,18 @@ static void sdhci_arasan_unregister_sdclk(struct device *dev)
 	of_clk_del_provider(dev->of_node);
 }
 
-static int sdhci_arasan_probe(struct platform_device *pdev)
+static int sdhci_rockchip_platform_init(struct sdhci_host *host,
+					struct platform_device *pdev)
 {
 	int ret;
-	const struct of_device_id *match;
 	struct device_node *node;
-	struct clk *clk_xin;
-	struct sdhci_host *host;
 	struct sdhci_pltfm_host *pltfm_host;
 	struct sdhci_arasan_data *sdhci_arasan;
-	struct device_node *np = pdev->dev.of_node;
-
-	host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata,
-				sizeof(*sdhci_arasan));
-	if (IS_ERR(host))
-		return PTR_ERR(host);
 
 	pltfm_host = sdhci_priv(host);
 	sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
-	sdhci_arasan->host = host;
 
-	match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node);
-	sdhci_arasan->soc_ctl_map = match->data;
+	sdhci_arasan->soc_ctl_map = &rk3399_soc_ctl_map;
 
 	node = of_parse_phandle(pdev->dev.of_node, "arasan,soc-ctl-syscon", 0);
 	if (node) {
@@ -611,10 +585,107 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 			if (ret != -EPROBE_DEFER)
 				dev_err(&pdev->dev, "Can't get syscon: %d\n",
 					ret);
-			goto err_pltfm_free;
+			return -1;
 		}
 	}
 
+	if (of_property_read_bool(pdev->dev.of_node, "xlnx,fails-without-test-cd"))
+		sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_FORCE_CDTEST;
+
+	return 0;
+}
+
+static int sdhci_rockchip_clock_init(struct sdhci_host *host,
+					struct platform_device *pdev)
+{
+	struct sdhci_pltfm_host *pltfm_host;
+	struct sdhci_arasan_data *sdhci_arasan;
+
+	pltfm_host = sdhci_priv(host);
+	sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
+
+	if (of_device_is_compatible(pdev->dev.of_node,
+				    "rockchip,rk3399-sdhci-5.1"))
+		sdhci_arasan_update_clockmultiplier(host, 0x0);
+
+	sdhci_arasan_update_baseclkfreq(host);
+
+	return sdhci_arasan_register_sdclk(sdhci_arasan, pltfm_host->clk, &pdev->dev);
+}
+
+static int sdhci_tango_platform_init(struct sdhci_host *host,
+				     struct platform_device *pdev)
+{
+	return 0;
+}
+
+/* Chip-specific ops */
+struct sdhci_arasan_cs_ops {
+	int (*platform_init)(struct sdhci_host *host,
+			     struct platform_device *pdev);
+	int (*clock_init)(struct sdhci_host *host,
+			  struct platform_device *pdev);
+};
+
+static const struct sdhci_arasan_cs_ops sdhci_rockchip_cs_ops = {
+	.platform_init = sdhci_rockchip_platform_init,
+	.clock_init = sdhci_rockchip_clock_init,
+};
+
+static const struct sdhci_arasan_cs_ops sdhci_tango_cs_ops = {
+	.platform_init = sdhci_tango_platform_init,
+};
+
+static const struct of_device_id sdhci_arasan_of_match[] = {
+	/* SoC-specific compatible strings */
+	{
+		.compatible = "rockchip,rk3399-sdhci-5.1",
+		.data = &sdhci_rockchip_cs_ops,
+	},
+	{
+		.compatible = "sigma,sdio-v1",
+		.data = &sdhci_tango_cs_ops,
+	},
+
+	/* Generic compatible below here */
+	{ .compatible = "arasan,sdhci-8.9a" },
+	{ .compatible = "arasan,sdhci-5.1" },
+	{ .compatible = "arasan,sdhci-4.9a" },
+
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
+
+static int sdhci_arasan_probe(struct platform_device *pdev)
+{
+	int ret;
+	const struct of_device_id *match;
+	struct clk *clk_xin;
+	struct sdhci_host *host;
+	struct sdhci_pltfm_host *pltfm_host;
+	struct sdhci_arasan_data *sdhci_arasan;
+	const struct sdhci_arasan_cs_ops *cs_ops;
+
+	host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata,
+				sizeof(*sdhci_arasan));
+	if (IS_ERR(host))
+		return PTR_ERR(host);
+
+	pltfm_host = sdhci_priv(host);
+	sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
+	sdhci_arasan->host = host;
+
+	match = of_match_device(sdhci_arasan_of_match, &pdev->dev);
+	if (match)
+		cs_ops = match->data;
+
+	/* SoC-specific platform init */
+	if (cs_ops && cs_ops->platform_init) {
+		ret = cs_ops->platform_init(host, pdev);
+		if (ret)
+			goto err_pltfm_free;
+	}
+
 	sdhci_arasan->clk_ahb = devm_clk_get(&pdev->dev, "clk_ahb");
 	if (IS_ERR(sdhci_arasan->clk_ahb)) {
 		dev_err(&pdev->dev, "clk_ahb clock not found.\n");
@@ -642,21 +713,14 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 	}
 
 	sdhci_get_of_property(pdev);
-
-	if (of_property_read_bool(np, "xlnx,fails-without-test-cd"))
-		sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_FORCE_CDTEST;
-
 	pltfm_host->clk = clk_xin;
 
-	if (of_device_is_compatible(pdev->dev.of_node,
-				    "rockchip,rk3399-sdhci-5.1"))
-		sdhci_arasan_update_clockmultiplier(host, 0x0);
-
-	sdhci_arasan_update_baseclkfreq(host);
-
-	ret = sdhci_arasan_register_sdclk(sdhci_arasan, clk_xin, &pdev->dev);
-	if (ret)
-		goto clk_disable_all;
+	/* SoC-specific clock init */
+	if (cs_ops && cs_ops->clock_init) {
+		ret = cs_ops->clock_init(host, pdev);
+		if (ret)
+			goto clk_disable_all;
+	}
 
 	ret = mmc_of_parse(host->mmc);
 	if (ret) {


If you are ok with it I can clean it up to submit it properly.

Re: Adding a .platform_init callback to sdhci_arasan_ops

[Doug Anderson]

Hi,

On Mon, Nov 28, 2016 at 6:39 AM, Sebastian Frias <sf84@laposte.net> wrote:
>> I will try to send another patch with what a different approach
>>
>
> Here's a different approach (I just tested that it built, because I don't have the
> rk3399 platform):
>
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 410a55b..5be6e67 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -382,22 +382,6 @@ static int sdhci_arasan_resume(struct device *dev)
>  static SIMPLE_DEV_PM_OPS(sdhci_arasan_dev_pm_ops, sdhci_arasan_suspend,
>                          sdhci_arasan_resume);
>
> -static const struct of_device_id sdhci_arasan_of_match[] = {
> -       /* SoC-specific compatible strings w/ soc_ctl_map */
> -       {
> -               .compatible = "rockchip,rk3399-sdhci-5.1",
> -               .data = &rk3399_soc_ctl_map,
> -       },
> -
> -       /* Generic compatible below here */
> -       { .compatible = "arasan,sdhci-8.9a" },
> -       { .compatible = "arasan,sdhci-5.1" },
> -       { .compatible = "arasan,sdhci-4.9a" },
> -
> -       { /* sentinel */ }
> -};
> -MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
> -
>  /**
>   * sdhci_arasan_sdcardclk_recalc_rate - Return the card clock rate
>   *
> @@ -578,28 +562,18 @@ static void sdhci_arasan_unregister_sdclk(struct device *dev)
>         of_clk_del_provider(dev->of_node);
>  }
>
> -static int sdhci_arasan_probe(struct platform_device *pdev)
> +static int sdhci_rockchip_platform_init(struct sdhci_host *host,
> +                                       struct platform_device *pdev)
>  {
>         int ret;
> -       const struct of_device_id *match;
>         struct device_node *node;
> -       struct clk *clk_xin;
> -       struct sdhci_host *host;
>         struct sdhci_pltfm_host *pltfm_host;
>         struct sdhci_arasan_data *sdhci_arasan;
> -       struct device_node *np = pdev->dev.of_node;
> -
> -       host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata,
> -                               sizeof(*sdhci_arasan));
> -       if (IS_ERR(host))
> -               return PTR_ERR(host);
>
>         pltfm_host = sdhci_priv(host);
>         sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
> -       sdhci_arasan->host = host;
>
> -       match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node);
> -       sdhci_arasan->soc_ctl_map = match->data;
> +       sdhci_arasan->soc_ctl_map = &rk3399_soc_ctl_map;
>
>         node = of_parse_phandle(pdev->dev.of_node, "arasan,soc-ctl-syscon", 0);
>         if (node) {
> @@ -611,10 +585,107 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>                         if (ret != -EPROBE_DEFER)
>                                 dev_err(&pdev->dev, "Can't get syscon: %d\n",
>                                         ret);
> -                       goto err_pltfm_free;
> +                       return -1;
>                 }
>         }
>
> +       if (of_property_read_bool(pdev->dev.of_node, "xlnx,fails-without-test-cd"))
> +               sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_FORCE_CDTEST;
> +
> +       return 0;
> +}
> +
> +static int sdhci_rockchip_clock_init(struct sdhci_host *host,
> +                                       struct platform_device *pdev)
> +{
> +       struct sdhci_pltfm_host *pltfm_host;
> +       struct sdhci_arasan_data *sdhci_arasan;
> +
> +       pltfm_host = sdhci_priv(host);
> +       sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
> +
> +       if (of_device_is_compatible(pdev->dev.of_node,
> +                                   "rockchip,rk3399-sdhci-5.1"))
> +               sdhci_arasan_update_clockmultiplier(host, 0x0);

This _does_ belong in a Rockchip-specific init function, for now.
Other platforms might want different values for
corecfg_clockmultiplier, I think.

If it becomes common to need to set this, it wouldn't be hard to make
it generic by putting it in the data matched by the device tree, then
generically call sdhci_arasan_update_clockmultiplier() in cases where
it is needed.  sdhci_arasan_update_clockmultiplier() itself should be
generic enough to handle it.


> +
> +       sdhci_arasan_update_baseclkfreq(host);

If you make soc_ctl_map always part of "struct sdhci_arasan_cs_ops"
then other platforms will be able to use it.

As argued in my original patch the field "corecfg_baseclkfreq" is
documented in the generic Arasan document
<https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf>
and thus is unlikely to be Rockchip specific.  It is entirely possible
that not everyone who integrates this IP block will need this register
set, but in that case they can set an offset as "-1" and they'll be
fine.

Said another way: the concept of whether or not to set "baseclkfreq"
doesn't need to be tired to whether or not we're on Rockchip.


> +
> +       return sdhci_arasan_register_sdclk(sdhci_arasan, pltfm_host->clk, &pdev->dev);

This is documented in "bindings/mmc/arasan,sdhci.txt" to be available
to all people using this driver, not just Rockchip.  You should do it
always.  If you don't specify "#clock-cells" in the device tree it
will be a no-op anyway.


> +}
> +
> +static int sdhci_tango_platform_init(struct sdhci_host *host,
> +                                    struct platform_device *pdev)
> +{
> +       return 0;

I'll comment in your other patch where you actually had an
implementation for this.


> +}
> +
> +/* Chip-specific ops */
> +struct sdhci_arasan_cs_ops {
> +       int (*platform_init)(struct sdhci_host *host,
> +                            struct platform_device *pdev);
> +       int (*clock_init)(struct sdhci_host *host,
> +                         struct platform_device *pdev);
> +};

I really think it's a good idea to add the "soc_ctl_map" into this structure.

If nothing else when the next Rockchip SoC comes out that uses this
then we won't have to do a second level of matching for Rockchip SoCs.
I'm also a firm believer that others will need "soc_ctl_map" at some
point in time, but obviously I can't predict the future.


> +
> +static const struct sdhci_arasan_cs_ops sdhci_rockchip_cs_ops = {
> +       .platform_init = sdhci_rockchip_platform_init,
> +       .clock_init = sdhci_rockchip_clock_init,
> +};
> +
> +static const struct sdhci_arasan_cs_ops sdhci_tango_cs_ops = {
> +       .platform_init = sdhci_tango_platform_init,
> +};
> +
> +static const struct of_device_id sdhci_arasan_of_match[] = {
> +       /* SoC-specific compatible strings */
> +       {
> +               .compatible = "rockchip,rk3399-sdhci-5.1",
> +               .data = &sdhci_rockchip_cs_ops,
> +       },
> +       {
> +               .compatible = "sigma,sdio-v1",
> +               .data = &sdhci_tango_cs_ops,
> +       },
> +
> +       /* Generic compatible below here */
> +       { .compatible = "arasan,sdhci-8.9a" },
> +       { .compatible = "arasan,sdhci-5.1" },
> +       { .compatible = "arasan,sdhci-4.9a" },
> +
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
> +
> +static int sdhci_arasan_probe(struct platform_device *pdev)
> +{
> +       int ret;
> +       const struct of_device_id *match;
> +       struct clk *clk_xin;
> +       struct sdhci_host *host;
> +       struct sdhci_pltfm_host *pltfm_host;
> +       struct sdhci_arasan_data *sdhci_arasan;
> +       const struct sdhci_arasan_cs_ops *cs_ops;
> +
> +       host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata,
> +                               sizeof(*sdhci_arasan));
> +       if (IS_ERR(host))
> +               return PTR_ERR(host);
> +
> +       pltfm_host = sdhci_priv(host);
> +       sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
> +       sdhci_arasan->host = host;
> +
> +       match = of_match_device(sdhci_arasan_of_match, &pdev->dev);
> +       if (match)
> +               cs_ops = match->data;
> +
> +       /* SoC-specific platform init */
> +       if (cs_ops && cs_ops->platform_init) {
> +               ret = cs_ops->platform_init(host, pdev);
> +               if (ret)
> +                       goto err_pltfm_free;
> +       }
> +
>         sdhci_arasan->clk_ahb = devm_clk_get(&pdev->dev, "clk_ahb");
>         if (IS_ERR(sdhci_arasan->clk_ahb)) {
>                 dev_err(&pdev->dev, "clk_ahb clock not found.\n");
> @@ -642,21 +713,14 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>         }
>
>         sdhci_get_of_property(pdev);
> -
> -       if (of_property_read_bool(np, "xlnx,fails-without-test-cd"))
> -               sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_FORCE_CDTEST;
> -
>         pltfm_host->clk = clk_xin;
>
> -       if (of_device_is_compatible(pdev->dev.of_node,
> -                                   "rockchip,rk3399-sdhci-5.1"))
> -               sdhci_arasan_update_clockmultiplier(host, 0x0);
> -
> -       sdhci_arasan_update_baseclkfreq(host);
> -
> -       ret = sdhci_arasan_register_sdclk(sdhci_arasan, clk_xin, &pdev->dev);
> -       if (ret)
> -               goto clk_disable_all;
> +       /* SoC-specific clock init */
> +       if (cs_ops && cs_ops->clock_init) {
> +               ret = cs_ops->clock_init(host, pdev);
> +               if (ret)
> +                       goto clk_disable_all;
> +       }
>
>         ret = mmc_of_parse(host->mmc);
>         if (ret) {
>
>
> If you are ok with it I can clean it up to submit it properly.

Re: Adding a .platform_init callback to sdhci_arasan_ops

[Mason]

On 28/11/2016 18:46, Doug Anderson wrote:

> As argued in my original patch the field "corecfg_baseclkfreq" is
> documented in the generic Arasan document
> <https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf>
> and thus is unlikely to be Rockchip specific.

I downloaded the data sheet, which doesn't mention registers,
but "pins" and "signals". Does that mean it is up to every
platform to decide how to group these wires into individual
registers?

corecfg_baseclkfreq[7:0]
Base Clock Frequency for SD Clock.
This is the frequency of the xin_clk.

How can 8 bits encode a frequency?
Is there an implicit LUT? Is it a MHz count?

"For maximum efficiency this should be around 200 MHz for eMMC
or 208MHz (for SD3.0)"

Regards.

Re: Adding a .platform_init callback to sdhci_arasan_ops

[Doug Anderson]

Hi,

On Mon, Nov 28, 2016 at 11:32 AM, Mason <slash.tmp@free.fr> wrote:
> On 28/11/2016 18:46, Doug Anderson wrote:
>
>> As argued in my original patch the field "corecfg_baseclkfreq" is
>> documented in the generic Arasan document
>> <https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf>
>> and thus is unlikely to be Rockchip specific.
>
> I downloaded the data sheet, which doesn't mention registers,
> but "pins" and "signals". Does that mean it is up to every
> platform to decide how to group these wires into individual
> registers?

As I understand it: yes.  ...but remember that I'm not arasan nor even
Rockchip and I don't have any access to that they provide their IP
licensees.

That's why I structured my config the way it did.  It means that no
matter how a licensee stuffs these configs into their register map we
can support it.  Every configuration is individually described by a
register offset, width, and mask.  Technically the width _ought_ to be
the same for all users, so I guess you could try to optimize that out.
...but it doesn't hurt the way it is.


> corecfg_baseclkfreq[7:0]
> Base Clock Frequency for SD Clock.
> This is the frequency of the xin_clk.
>
> How can 8 bits encode a frequency?
> Is there an implicit LUT? Is it a MHz count?

Use the source.  :)

 * The corecfg_baseclkfreq is supposed to contain the MHz of clk_xin.

It also turns out to match the SDHCI spec "Base Clock Frequency for SD
Clock".  See below.

---

It actually turns out that both of the "corecfg" bits we're setting
right now for Rockchip are a bit on the silly side.  All they do is
get mirrored out the other end in the "caps" register.  I found that
out from folks at Rockchip much later after I wrote my patch.

So, for instance, setting corecfg_clockmultiplier (I'm told) does
nothing more than control the bits read here:

  (caps[1] & SDHCI_CLOCK_MUL_MASK) >> SDHCI_CLOCK_MUL_SHIFT;

I can certainly see that they are mirrored, but I have to just take it
on faith that it does nothing else.

>>> w(grf + 0xf02c, 0x00ff0011)
>>> hex((r(sdhci + 0x44) & 0x00ff0000) >> 16)
'0x11L'


...and similar for the speed:

>>> w(grf + 0xf000, 0xff009700)
>>> hex((r(sdhci + 0x40) & 0x0000ff00) >> 8)
'0x97L'


-Doug

Re: Adding a .platform_init callback to sdhci_arasan_ops

[Sebastian Frias]

Hi Doug,

On 28/11/16 18:46, Doug Anderson wrote:
> Hi,
> 
> On Mon, Nov 28, 2016 at 6:39 AM, Sebastian Frias <sf84@laposte.net> wrote:
>>> I will try to send another patch with what a different approach
>>>
>>
>> Here's a different approach (I just tested that it built, because I don't have the
>> rk3399 platform):
>>
>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
>> index 410a55b..5be6e67 100644
>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>> @@ -382,22 +382,6 @@ static int sdhci_arasan_resume(struct device *dev)
>>  static SIMPLE_DEV_PM_OPS(sdhci_arasan_dev_pm_ops, sdhci_arasan_suspend,
>>                          sdhci_arasan_resume);
>>
>> -static const struct of_device_id sdhci_arasan_of_match[] = {
>> -       /* SoC-specific compatible strings w/ soc_ctl_map */
>> -       {
>> -               .compatible = "rockchip,rk3399-sdhci-5.1",
>> -               .data = &rk3399_soc_ctl_map,
>> -       },
>> -
>> -       /* Generic compatible below here */
>> -       { .compatible = "arasan,sdhci-8.9a" },
>> -       { .compatible = "arasan,sdhci-5.1" },
>> -       { .compatible = "arasan,sdhci-4.9a" },
>> -
>> -       { /* sentinel */ }
>> -};
>> -MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
>> -
>>  /**
>>   * sdhci_arasan_sdcardclk_recalc_rate - Return the card clock rate
>>   *
>> @@ -578,28 +562,18 @@ static void sdhci_arasan_unregister_sdclk(struct device *dev)
>>         of_clk_del_provider(dev->of_node);
>>  }
>>
>> -static int sdhci_arasan_probe(struct platform_device *pdev)
>> +static int sdhci_rockchip_platform_init(struct sdhci_host *host,
>> +                                       struct platform_device *pdev)
>>  {
>>         int ret;
>> -       const struct of_device_id *match;
>>         struct device_node *node;
>> -       struct clk *clk_xin;
>> -       struct sdhci_host *host;
>>         struct sdhci_pltfm_host *pltfm_host;
>>         struct sdhci_arasan_data *sdhci_arasan;
>> -       struct device_node *np = pdev->dev.of_node;
>> -
>> -       host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata,
>> -                               sizeof(*sdhci_arasan));
>> -       if (IS_ERR(host))
>> -               return PTR_ERR(host);
>>
>>         pltfm_host = sdhci_priv(host);
>>         sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>> -       sdhci_arasan->host = host;
>>
>> -       match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node);
>> -       sdhci_arasan->soc_ctl_map = match->data;
>> +       sdhci_arasan->soc_ctl_map = &rk3399_soc_ctl_map;
>>
>>         node = of_parse_phandle(pdev->dev.of_node, "arasan,soc-ctl-syscon", 0);
>>         if (node) {
>> @@ -611,10 +585,107 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>>                         if (ret != -EPROBE_DEFER)
>>                                 dev_err(&pdev->dev, "Can't get syscon: %d\n",
>>                                         ret);
>> -                       goto err_pltfm_free;
>> +                       return -1;
>>                 }
>>         }
>>
>> +       if (of_property_read_bool(pdev->dev.of_node, "xlnx,fails-without-test-cd"))
>> +               sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_FORCE_CDTEST;
>> +
>> +       return 0;
>> +}
>> +
>> +static int sdhci_rockchip_clock_init(struct sdhci_host *host,
>> +                                       struct platform_device *pdev)
>> +{
>> +       struct sdhci_pltfm_host *pltfm_host;
>> +       struct sdhci_arasan_data *sdhci_arasan;
>> +
>> +       pltfm_host = sdhci_priv(host);
>> +       sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>> +
>> +       if (of_device_is_compatible(pdev->dev.of_node,
>> +                                   "rockchip,rk3399-sdhci-5.1"))
>> +               sdhci_arasan_update_clockmultiplier(host, 0x0);
> 
> This _does_ belong in a Rockchip-specific init function, for now.

I'm not sure I understood. Are you saying that you agree to put this
into a specific function? Essentially agreeing with the concept the
patch is putting forward?

Note, I'm more interested in the concept (i.e.: init functions) and less
in knowing if my patch (which was a quick and dirty thing) properly moved
the functions into the said init functions. For example, I did not move
the code dealing with "arasan,sdhci-5.1", but it could go into another
callback.

Right now there are no "chip-specific" functions.
Just a code in sdhci_arasan_probe() that by checking various compatible
strings and the presence of other specific properties, acts as a way of
"chip-specific" initialisation, because it calls or not some functions.
(or the functions do nothing if some DT properties are not there).

The proposed patch is an attempt to clean up sdhci_arasan_probe() from
all those checks and move them into separate functions, for clarity and
maintainability reasons.

What are your thoughts in that regard?

>From what I've seen in other drivers, for example drivers/net/ethernet/aurora/nb8800.c
One matches the compatible string to get a (likely) chip-specific set of
callbacks to use in the 'probe' function.

Then, adding support for other chips is just a matter of replacing some
of those callbacks with others adapted to a given platform.

> Other platforms might want different values for
> corecfg_clockmultiplier, I think.
> 
> If it becomes common to need to set this, it wouldn't be hard to make
> it generic by putting it in the data matched by the device tree, then
> generically call sdhci_arasan_update_clockmultiplier() in cases where
> it is needed.  sdhci_arasan_update_clockmultiplier() itself should be
> generic enough to handle it.
> 
> 
>> +
>> +       sdhci_arasan_update_baseclkfreq(host);
> 
> If you make soc_ctl_map always part of "struct sdhci_arasan_cs_ops"
> then other platforms will be able to use it.

I thought that soc_ctl_map was specific and for a given platform.
For what is worth, I don't know which of these calls are or can be made
generic or not.

Indeed, I'm not an expert in this code; However, I think that given the
amount of checks for specific properties, probably part of this is chip-
specific, and as such, it would benefit from some re-factoring so that
the chip-specific parts are clearly separated from the rest, instead of
being mixed together inside the probe function.

> 
> As argued in my original patch the field "corecfg_baseclkfreq" is
> documented in the generic Arasan document
> <https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf>
> and thus is unlikely to be Rockchip specific.  It is entirely possible
> that not everyone who integrates this IP block will need this register
> set, but in that case they can set an offset as "-1" and they'll be
> fine.
> 
> Said another way: the concept of whether or not to set "baseclkfreq"
> doesn't need to be tired to whether or not we're on Rockchip.
> 

I see.
For what is worth, the documentation for 'sdhci_arasan_update_baseclkfreq()'
says something like:

 *   Many existing devices don't seem to do this and work fine.  To keep
 *   compatibility for old hardware where the device tree doesn't provide a
 *   register map, this function is a noop if a soc_ctl_map hasn't been provided
 *   for this platform.

> 
>> +
>> +       return sdhci_arasan_register_sdclk(sdhci_arasan, pltfm_host->clk, &pdev->dev);
> 
> This is documented in "bindings/mmc/arasan,sdhci.txt" to be available
> to all people using this driver, not just Rockchip.  You should do it
> always.  If you don't specify "#clock-cells" in the device tree it
> will be a no-op anyway.

I see, thanks for the explanation.

> 
> 
>> +}
>> +
>> +static int sdhci_tango_platform_init(struct sdhci_host *host,
>> +                                    struct platform_device *pdev)
>> +{
>> +       return 0;
> 
> I'll comment in your other patch where you actually had an
> implementation for this.
> 

Sounds good. I will reply to you there because now I have a more
complete set of register writes.

Best regards,

Sebastian

> 
>> +}
>> +
>> +/* Chip-specific ops */
>> +struct sdhci_arasan_cs_ops {
>> +       int (*platform_init)(struct sdhci_host *host,
>> +                            struct platform_device *pdev);
>> +       int (*clock_init)(struct sdhci_host *host,
>> +                         struct platform_device *pdev);
>> +};
> 
> I really think it's a good idea to add the "soc_ctl_map" into this structure.
> 
> If nothing else when the next Rockchip SoC comes out that uses this
> then we won't have to do a second level of matching for Rockchip SoCs.
> I'm also a firm believer that others will need "soc_ctl_map" at some
> point in time, but obviously I can't predict the future.
> 
> 
>> +
>> +static const struct sdhci_arasan_cs_ops sdhci_rockchip_cs_ops = {
>> +       .platform_init = sdhci_rockchip_platform_init,
>> +       .clock_init = sdhci_rockchip_clock_init,
>> +};
>> +
>> +static const struct sdhci_arasan_cs_ops sdhci_tango_cs_ops = {
>> +       .platform_init = sdhci_tango_platform_init,
>> +};
>> +
>> +static const struct of_device_id sdhci_arasan_of_match[] = {
>> +       /* SoC-specific compatible strings */
>> +       {
>> +               .compatible = "rockchip,rk3399-sdhci-5.1",
>> +               .data = &sdhci_rockchip_cs_ops,
>> +       },
>> +       {
>> +               .compatible = "sigma,sdio-v1",
>> +               .data = &sdhci_tango_cs_ops,
>> +       },
>> +
>> +       /* Generic compatible below here */
>> +       { .compatible = "arasan,sdhci-8.9a" },
>> +       { .compatible = "arasan,sdhci-5.1" },
>> +       { .compatible = "arasan,sdhci-4.9a" },
>> +
>> +       { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
>> +
>> +static int sdhci_arasan_probe(struct platform_device *pdev)
>> +{
>> +       int ret;
>> +       const struct of_device_id *match;
>> +       struct clk *clk_xin;
>> +       struct sdhci_host *host;
>> +       struct sdhci_pltfm_host *pltfm_host;
>> +       struct sdhci_arasan_data *sdhci_arasan;
>> +       const struct sdhci_arasan_cs_ops *cs_ops;
>> +
>> +       host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata,
>> +                               sizeof(*sdhci_arasan));
>> +       if (IS_ERR(host))
>> +               return PTR_ERR(host);
>> +
>> +       pltfm_host = sdhci_priv(host);
>> +       sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>> +       sdhci_arasan->host = host;
>> +
>> +       match = of_match_device(sdhci_arasan_of_match, &pdev->dev);
>> +       if (match)
>> +               cs_ops = match->data;
>> +
>> +       /* SoC-specific platform init */
>> +       if (cs_ops && cs_ops->platform_init) {
>> +               ret = cs_ops->platform_init(host, pdev);
>> +               if (ret)
>> +                       goto err_pltfm_free;
>> +       }
>> +
>>         sdhci_arasan->clk_ahb = devm_clk_get(&pdev->dev, "clk_ahb");
>>         if (IS_ERR(sdhci_arasan->clk_ahb)) {
>>                 dev_err(&pdev->dev, "clk_ahb clock not found.\n");
>> @@ -642,21 +713,14 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>>         }
>>
>>         sdhci_get_of_property(pdev);
>> -
>> -       if (of_property_read_bool(np, "xlnx,fails-without-test-cd"))
>> -               sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_FORCE_CDTEST;
>> -
>>         pltfm_host->clk = clk_xin;
>>
>> -       if (of_device_is_compatible(pdev->dev.of_node,
>> -                                   "rockchip,rk3399-sdhci-5.1"))
>> -               sdhci_arasan_update_clockmultiplier(host, 0x0);
>> -
>> -       sdhci_arasan_update_baseclkfreq(host);
>> -
>> -       ret = sdhci_arasan_register_sdclk(sdhci_arasan, clk_xin, &pdev->dev);
>> -       if (ret)
>> -               goto clk_disable_all;
>> +       /* SoC-specific clock init */
>> +       if (cs_ops && cs_ops->clock_init) {
>> +               ret = cs_ops->clock_init(host, pdev);
>> +               if (ret)
>> +                       goto clk_disable_all;
>> +       }
>>
>>         ret = mmc_of_parse(host->mmc);
>>         if (ret) {
>>
>>
>> If you are ok with it I can clean it up to submit it properly.

Re: Adding a .platform_init callback to sdhci_arasan_ops

[Doug Anderson]

Hi,

On Mon, Dec 5, 2016 at 4:28 AM, Sebastian Frias <sf84@laposte.net> wrote:
> Hi Doug,
>
> On 28/11/16 18:46, Doug Anderson wrote:
>> Hi,
>>
>> On Mon, Nov 28, 2016 at 6:39 AM, Sebastian Frias <sf84@laposte.net> wrote:
>>>> I will try to send another patch with what a different approach
>>>>
>>>
>>> Here's a different approach (I just tested that it built, because I don't have the
>>> rk3399 platform):
>>>
>>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
>>> index 410a55b..5be6e67 100644
>>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>>> @@ -382,22 +382,6 @@ static int sdhci_arasan_resume(struct device *dev)
>>>  static SIMPLE_DEV_PM_OPS(sdhci_arasan_dev_pm_ops, sdhci_arasan_suspend,
>>>                          sdhci_arasan_resume);
>>>
>>> -static const struct of_device_id sdhci_arasan_of_match[] = {
>>> -       /* SoC-specific compatible strings w/ soc_ctl_map */
>>> -       {
>>> -               .compatible = "rockchip,rk3399-sdhci-5.1",
>>> -               .data = &rk3399_soc_ctl_map,
>>> -       },
>>> -
>>> -       /* Generic compatible below here */
>>> -       { .compatible = "arasan,sdhci-8.9a" },
>>> -       { .compatible = "arasan,sdhci-5.1" },
>>> -       { .compatible = "arasan,sdhci-4.9a" },
>>> -
>>> -       { /* sentinel */ }
>>> -};
>>> -MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
>>> -
>>>  /**
>>>   * sdhci_arasan_sdcardclk_recalc_rate - Return the card clock rate
>>>   *
>>> @@ -578,28 +562,18 @@ static void sdhci_arasan_unregister_sdclk(struct device *dev)
>>>         of_clk_del_provider(dev->of_node);
>>>  }
>>>
>>> -static int sdhci_arasan_probe(struct platform_device *pdev)
>>> +static int sdhci_rockchip_platform_init(struct sdhci_host *host,
>>> +                                       struct platform_device *pdev)
>>>  {
>>>         int ret;
>>> -       const struct of_device_id *match;
>>>         struct device_node *node;
>>> -       struct clk *clk_xin;
>>> -       struct sdhci_host *host;
>>>         struct sdhci_pltfm_host *pltfm_host;
>>>         struct sdhci_arasan_data *sdhci_arasan;
>>> -       struct device_node *np = pdev->dev.of_node;
>>> -
>>> -       host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata,
>>> -                               sizeof(*sdhci_arasan));
>>> -       if (IS_ERR(host))
>>> -               return PTR_ERR(host);
>>>
>>>         pltfm_host = sdhci_priv(host);
>>>         sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>>> -       sdhci_arasan->host = host;
>>>
>>> -       match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node);
>>> -       sdhci_arasan->soc_ctl_map = match->data;
>>> +       sdhci_arasan->soc_ctl_map = &rk3399_soc_ctl_map;
>>>
>>>         node = of_parse_phandle(pdev->dev.of_node, "arasan,soc-ctl-syscon", 0);
>>>         if (node) {
>>> @@ -611,10 +585,107 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>>>                         if (ret != -EPROBE_DEFER)
>>>                                 dev_err(&pdev->dev, "Can't get syscon: %d\n",
>>>                                         ret);
>>> -                       goto err_pltfm_free;
>>> +                       return -1;
>>>                 }
>>>         }
>>>
>>> +       if (of_property_read_bool(pdev->dev.of_node, "xlnx,fails-without-test-cd"))
>>> +               sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_FORCE_CDTEST;
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int sdhci_rockchip_clock_init(struct sdhci_host *host,
>>> +                                       struct platform_device *pdev)
>>> +{
>>> +       struct sdhci_pltfm_host *pltfm_host;
>>> +       struct sdhci_arasan_data *sdhci_arasan;
>>> +
>>> +       pltfm_host = sdhci_priv(host);
>>> +       sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>>> +
>>> +       if (of_device_is_compatible(pdev->dev.of_node,
>>> +                                   "rockchip,rk3399-sdhci-5.1"))
>>> +               sdhci_arasan_update_clockmultiplier(host, 0x0);
>>
>> This _does_ belong in a Rockchip-specific init function, for now.
>
> I'm not sure I understood. Are you saying that you agree to put this
> into a specific function? Essentially agreeing with the concept the
> patch is putting forward?
>
> Note, I'm more interested in the concept (i.e.: init functions) and less
> in knowing if my patch (which was a quick and dirty thing) properly moved
> the functions into the said init functions. For example, I did not move
> the code dealing with "arasan,sdhci-5.1", but it could go into another
> callback.
>
> Right now there are no "chip-specific" functions.
> Just a code in sdhci_arasan_probe() that by checking various compatible
> strings and the presence of other specific properties, acts as a way of
> "chip-specific" initialisation, because it calls or not some functions.
> (or the functions do nothing if some DT properties are not there).
>
> The proposed patch is an attempt to clean up sdhci_arasan_probe() from
> all those checks and move them into separate functions, for clarity and
> maintainability reasons.
>
> What are your thoughts in that regard?
>
> From what I've seen in other drivers, for example drivers/net/ethernet/aurora/nb8800.c
> One matches the compatible string to get a (likely) chip-specific set of
> callbacks to use in the 'probe' function.

I have no objections to chip-specific functions if they are needed.
It's really just a cleaner way to avoid doing "if this chip then, else
if this other chip then, else if this third chip them".

The thing I worry about is that too much stuff will be jammed into
chip-specific functions and that we'll end up solving the same thing
more than one way.


> Then, adding support for other chips is just a matter of replacing some
> of those callbacks with others adapted to a given platform.
>
>> Other platforms might want different values for
>> corecfg_clockmultiplier, I think.
>>
>> If it becomes common to need to set this, it wouldn't be hard to make
>> it generic by putting it in the data matched by the device tree, then
>> generically call sdhci_arasan_update_clockmultiplier() in cases where
>> it is needed.  sdhci_arasan_update_clockmultiplier() itself should be
>> generic enough to handle it.
>>
>>
>>> +
>>> +       sdhci_arasan_update_baseclkfreq(host);
>>
>> If you make soc_ctl_map always part of "struct sdhci_arasan_cs_ops"
>> then other platforms will be able to use it.
>
> I thought that soc_ctl_map was specific and for a given platform.

I believe the soc_ctl_map will be used by more than one chip, mostly
because I saw these same fields referenced in the generic
(non-Rockchip) Arasan docs.  Obviously I have a very small view of the
SDHCI-Arasan world though.


> For what is worth, I don't know which of these calls are or can be made
> generic or not.
>
> Indeed, I'm not an expert in this code; However, I think that given the
> amount of checks for specific properties, probably part of this is chip-
> specific, and as such, it would benefit from some re-factoring so that
> the chip-specific parts are clearly separated from the rest, instead of
> being mixed together inside the probe function.

I believe the only chip-specific stuff was the part that is currently
guarded by the "if rk3399" check.  Everything else seems like it ought
to be applicable to other platforms using Arasan's SDHCI IP.


>> As argued in my original patch the field "corecfg_baseclkfreq" is
>> documented in the generic Arasan document
>> <https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf>
>> and thus is unlikely to be Rockchip specific.  It is entirely possible
>> that not everyone who integrates this IP block will need this register
>> set, but in that case they can set an offset as "-1" and they'll be
>> fine.
>>
>> Said another way: the concept of whether or not to set "baseclkfreq"
>> doesn't need to be tired to whether or not we're on Rockchip.
>>
>
> I see.
> For what is worth, the documentation for 'sdhci_arasan_update_baseclkfreq()'
> says something like:
>
>  *   Many existing devices don't seem to do this and work fine.  To keep
>  *   compatibility for old hardware where the device tree doesn't provide a
>  *   register map, this function is a noop if a soc_ctl_map hasn't been provided
>  *   for this platform.

Yup.  I wrote that.  See "git blame".

Re: Adding a .platform_init callback to sdhci_arasan_ops

[Doug Anderson]

Hi,

On Mon, Nov 28, 2016 at 5:28 AM, Sebastian Frias <sf84@laposte.net> wrote:
> +static void sdhci_tango4_platform_init(struct sdhci_host *host)
> +{
> +       printk("%s\n", __func__);
> +
> +       /*
> +         pad_mode[2:0]=0    must be 0
> +         sel_sdio[3]=1      must be 1 for SDIO
> +         inv_sdwp_pol[4]=0  if set inverts the SD write protect polarity
> +         inv_sdcd_pol[5]=0  if set inverts the SD card present polarity
> +       */
> +       sdhci_writel(host, 0x00000008, 0x100 + 0x0);

If I were doing this, I'd probably actually add these fields to the
"sdhci_arasan_soc_ctl_map", then add a 3rd option to
sdhci_arasan_syscon_write().  Right now it has 2 modes: "hiword
update" and "non-hiword update".  You could add a 3rd indicating that
you set config registers by just writing at some offset of the main
SDHCI registers space.

So you'd add 4 fields:

.tango_pad_mode = { .reg = 0x0000, .width = 3, .shift = 0 },
.sel_sdio = { .reg = 0x0000, .width = 1, .shift = 3},
.inv_sdwp_pol = { .reg = 0x0000, .width = 1, .shift = 4},
.inv_sdcd_pol = { .reg = 0x0000, .width = 1, .shift = 5},

In your platform-specific init you're proposing you could set
tango_pad_mode to 0.  That seems tango-specific.

You'd want to hook into "set_ios" for setting sel_sdio or not.  That's
important if anyone ever wants to plug in an external SDIO card to
your slot.  This one good argument for putting this in
sdhci_arasan_soc_ctl_map, since you wouldn't want to do a
compatibility matching every time set_ios is called.

I'd have to look more into the whole SD/WP polarity issue.  There are
already so many levels of inversion for these signals in Linux that
it's confusing.  It seems like you might just want to hardcode them to
"0" and let users use all the existing ways to invert things...  You
could either put that hardcoding into your platform init code or (if
you're using sdhci_arasan_soc_ctl_map) put it in the main init code so
that if anyone else needs to init similar signals then they can take
advantage of it.

--

One random side note is that as currently documented you need to
specify a "shift" of -1 for any sdhci_arasan_soc_ctl_map fields you
aren't using.  That seems stupid--not sure why I did that.  It seems
better to clue off "width = 0" so that you could just freely not init
any fields you don't need.

-Doug

Re: Adding a .platform_init callback to sdhci_arasan_ops

[Sebastian Frias]

Hi Doug,

On 28/11/16 19:02, Doug Anderson wrote:
> Hi,
> 
> On Mon, Nov 28, 2016 at 5:28 AM, Sebastian Frias <sf84@laposte.net> wrote:
>> +static void sdhci_tango4_platform_init(struct sdhci_host *host)
>> +{
>> +       printk("%s\n", __func__);
>> +
>> +       /*
>> +         pad_mode[2:0]=0    must be 0
>> +         sel_sdio[3]=1      must be 1 for SDIO
>> +         inv_sdwp_pol[4]=0  if set inverts the SD write protect polarity
>> +         inv_sdcd_pol[5]=0  if set inverts the SD card present polarity
>> +       */
>> +       sdhci_writel(host, 0x00000008, 0x100 + 0x0);
> 
> If I were doing this, I'd probably actually add these fields to the
> "sdhci_arasan_soc_ctl_map", then add a 3rd option to
> sdhci_arasan_syscon_write().  Right now it has 2 modes: "hiword
> update" and "non-hiword update".  You could add a 3rd indicating that
> you set config registers by just writing at some offset of the main
> SDHCI registers space.
> 
> So you'd add 4 fields:
> 
> .tango_pad_mode = { .reg = 0x0000, .width = 3, .shift = 0 },
> .sel_sdio = { .reg = 0x0000, .width = 1, .shift = 3},
> .inv_sdwp_pol = { .reg = 0x0000, .width = 1, .shift = 4},
> .inv_sdcd_pol = { .reg = 0x0000, .width = 1, .shift = 5},

Right now I'm using something like:

+       val = 0;
+       val |= PAD_MODE(0); /* must be 0 */
+       val |= SEL_SDIO;    /* enable SDIO */
+       sdhci_writel(host, val, 0x100 + 0x0);
+
+       val = 0;
+       val |= TIMEOUT_CLK_UNIT_MHZ;         /* unit: MHz */
+       val |= TIMEOUT_CLK_FREQ(52);         /* timeout clock: 52MHz */
+       val |= BUS_CLK_FREQ_FOR_SD_CLK(200); /* SDIO clock: 200MHz (TODO: get from DT) */
+       val |= MAX_BLOCK_LENGTH(3);          /* max block size: 4096 bytes */
+       val |= EXTENDED_MEDIA_BUS_SUPPORTED; /* DT? */
+       val |= ADMA2_SUPPORTED;              /* DT? */
+       val |= HIGH_SPEED_SUPPORTED;         /* DT? */
+       val |= SDMA_SUPPORTED;               /* DT? */
+       val |= SUSPEND_RESUME_SUPPORTED;     /* DT? */
+       val |= VOLTAGE_3_3_V_SUPPORTED;      /* DT? */
+#if 0
+       val |= VOLTAGE_1_8_V_SUPPORTED;      /* DT? */
+#endif
+       val |= ASYNCHRONOUS_IRQ_SUPPORTED;   /* DT? */
+       val |= SLOT_TYPE_REMOVABLE;          /* DT? */
+       val |= SDR50_SUPPORTED;              /* DT? */
+       sdhci_writel(host, val, 0x100 + 0x40);
+
+       val = 0;
+       val |= TIMER_COUNT_FOR_RETUNING(1);  /* DT? */
+       val |= CLOCK_MULTIPLIER(0);          /* DT? */
+       val |= SPI_MODE_SUPPORTED;           /* DT? */
+       val |= SPI_BLOCK_MODE_SUPPORTED;     /* DT? */
+       sdhci_writel(host, val, 0x100 + 0x44);
+
+       sdhci_writel(host, 0x0004022c, 0x100 + 0x28);
+       sdhci_writel(host, 0x00000002, 0x100 + 0x2c);
+
+       sdhci_writel(host, 0x00600000, 0x100 + 0x50);

which seems much easier to handle (and portable).

At any rate, one thing to note from this is that many of these
bits should probably be initialised based on DT, right?

For example, the DT has a "non-removable" property, which I think
should be used to setup SLOT_TYPE_EMBEDDED (if the property is
present) or SLOT_TYPE_REMOVABLE (if the property is not present)

Looking at Documentation/devicetree/bindings/mmc/mmc.txt we can see
more related properties:

Optional properties:
- bus-width: Number of data lines, can be <1>, <4>, or <8>.  The default
  will be <1> if the property is absent.
- wp-gpios: Specify GPIOs for write protection, see gpio binding
- cd-inverted: when present, polarity on the CD line is inverted. See the note
  below for the case, when a GPIO is used for the CD line
- wp-inverted: when present, polarity on the WP line is inverted. See the note
  below for the case, when a GPIO is used for the WP line
- disable-wp: When set no physical WP line is present. This property should
  only be specified when the controller has a dedicated write-protect
  detection logic. If a GPIO is always used for the write-protect detection
  logic it is sufficient to not specify wp-gpios property in the absence of a WP
  line.
- max-frequency: maximum operating clock frequency
- no-1-8-v: when present, denotes that 1.8v card voltage is not supported on
  this system, even if the controller claims it is.
- cap-sd-highspeed: SD high-speed timing is supported
- cap-mmc-highspeed: MMC high-speed timing is supported
- sd-uhs-sdr12: SD UHS SDR12 speed is supported
- sd-uhs-sdr25: SD UHS SDR25 speed is supported
- sd-uhs-sdr50: SD UHS SDR50 speed is supported
- sd-uhs-sdr104: SD UHS SDR104 speed is supported
- sd-uhs-ddr50: SD UHS DDR50 speed is supported
...

which makes me wonder, what is the recommended way of dealing with this?
- Should I use properties on the DT? If so, then I need to add code to set
up the register properly.
- Should I hardcode these values use a minimal DT? If so, then I need an
init function to put all this.
- Should I hardcode stuff at u-Boot level? If so, nothing is required and
should work without any modifications to the Arasan Linux driver.

It appears that the Linux driver is expecting most of these fields to be
hardcoded and "pre-set" before (maybe by the bootloader) it starts, hence
the lack of any "init" function so far.

> 
> In your platform-specific init you're proposing you could set
> tango_pad_mode to 0.  That seems tango-specific.
> 
> You'd want to hook into "set_ios" for setting sel_sdio or not.  That's
> important if anyone ever wants to plug in an external SDIO card to
> your slot.  This one good argument for putting this in
> sdhci_arasan_soc_ctl_map, since you wouldn't want to do a
> compatibility matching every time set_ios is called.

Thanks for the advice, I will look into that.

> 
> I'd have to look more into the whole SD/WP polarity issue.  There are
> already so many levels of inversion for these signals in Linux that
> it's confusing.  It seems like you might just want to hardcode them to
> "0" and let users use all the existing ways to invert things...  You
> could either put that hardcoding into your platform init code or (if
> you're using sdhci_arasan_soc_ctl_map) put it in the main init code so
> that if anyone else needs to init similar signals then they can take
> advantage of it.

Yes, I think I will leave them to 0.

> 
> --
> 
> One random side note is that as currently documented you need to
> specify a "shift" of -1 for any sdhci_arasan_soc_ctl_map fields you
> aren't using.  That seems stupid--not sure why I did that.  It seems
> better to clue off "width = 0" so that you could just freely not init
> any fields you don't need.

I see.
So far I'm not really convinced about using "soc_ctl_map" because what I
have so far is more portable, and can easily be put as is somewhere else
(i.e.: in different flavors of bootloaders)

Best regards,

Sebastian

Re: Adding a .platform_init callback to sdhci_arasan_ops

[Doug Anderson]

Hi,

On Mon, Dec 5, 2016 at 4:29 AM, Sebastian Frias <sf84@laposte.net> wrote:
> Hi Doug,
>
> On 28/11/16 19:02, Doug Anderson wrote:
>> Hi,
>>
>> On Mon, Nov 28, 2016 at 5:28 AM, Sebastian Frias <sf84@laposte.net> wrote:
>>> +static void sdhci_tango4_platform_init(struct sdhci_host *host)
>>> +{
>>> +       printk("%s\n", __func__);
>>> +
>>> +       /*
>>> +         pad_mode[2:0]=0    must be 0
>>> +         sel_sdio[3]=1      must be 1 for SDIO
>>> +         inv_sdwp_pol[4]=0  if set inverts the SD write protect polarity
>>> +         inv_sdcd_pol[5]=0  if set inverts the SD card present polarity
>>> +       */
>>> +       sdhci_writel(host, 0x00000008, 0x100 + 0x0);
>>
>> If I were doing this, I'd probably actually add these fields to the
>> "sdhci_arasan_soc_ctl_map", then add a 3rd option to
>> sdhci_arasan_syscon_write().  Right now it has 2 modes: "hiword
>> update" and "non-hiword update".  You could add a 3rd indicating that
>> you set config registers by just writing at some offset of the main
>> SDHCI registers space.
>>
>> So you'd add 4 fields:
>>
>> .tango_pad_mode = { .reg = 0x0000, .width = 3, .shift = 0 },
>> .sel_sdio = { .reg = 0x0000, .width = 1, .shift = 3},
>> .inv_sdwp_pol = { .reg = 0x0000, .width = 1, .shift = 4},
>> .inv_sdcd_pol = { .reg = 0x0000, .width = 1, .shift = 5},
>
> Right now I'm using something like:

So taking a very quick gander at
<https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf>
and comparing the "corecfg" things to what you're setting, I find many
matches.  I didn't look very hard, so probably could find matches for
the rest?


> +       val = 0;
> +       val |= PAD_MODE(0); /* must be 0 */
> +       val |= SEL_SDIO;    /* enable SDIO */
> +       sdhci_writel(host, val, 0x100 + 0x0);
> +
> +       val = 0;
> +       val |= TIMEOUT_CLK_UNIT_MHZ;         /* unit: MHz */
> +       val |= TIMEOUT_CLK_FREQ(52);         /* timeout clock: 52MHz */

corecfg_timeoutclkfreq[5:0] ?

> +       val |= BUS_CLK_FREQ_FOR_SD_CLK(200); /* SDIO clock: 200MHz (TODO: get from DT) */
> +       val |= MAX_BLOCK_LENGTH(3);          /* max block size: 4096 bytes */
> +       val |= EXTENDED_MEDIA_BUS_SUPPORTED; /* DT? */
> +       val |= ADMA2_SUPPORTED;              /* DT? */

corecfg_adma2support

> +       val |= HIGH_SPEED_SUPPORTED;         /* DT? */

corecfg_highspeedsupport

> +       val |= SDMA_SUPPORTED;               /* DT? */

corecfg_sdmasupport

> +       val |= SUSPEND_RESUME_SUPPORTED;     /* DT? */

corecfg_suspressupport

> +       val |= VOLTAGE_3_3_V_SUPPORTED;      /* DT? */

corecfg_3p3voltsupport

> +#if 0
> +       val |= VOLTAGE_1_8_V_SUPPORTED;      /* DT? */

corecfg_1p8voltsupport

> +#endif
> +       val |= ASYNCHRONOUS_IRQ_SUPPORTED;   /* DT? */

corecfg_asyncintrsupport

> +       val |= SLOT_TYPE_REMOVABLE;          /* DT? */

corecfg_slottype[1:0]

> +       val |= SDR50_SUPPORTED;              /* DT? */

corecfg_sdr50support

> +       sdhci_writel(host, val, 0x100 + 0x40);
> +
> +       val = 0;
> +       val |= TIMER_COUNT_FOR_RETUNING(1);  /* DT? */

corecfg_retuningtimercnt[3:0]

> +       val |= CLOCK_MULTIPLIER(0);          /* DT? */
> +       val |= SPI_MODE_SUPPORTED;           /* DT? */

corecfg_spisupport

> +       val |= SPI_BLOCK_MODE_SUPPORTED;     /* DT? */

corecfg_spiblkmode

> +       sdhci_writel(host, val, 0x100 + 0x44);
> +
> +       sdhci_writel(host, 0x0004022c, 0x100 + 0x28);
> +       sdhci_writel(host, 0x00000002, 0x100 + 0x2c);
> +
> +       sdhci_writel(host, 0x00600000, 0x100 + 0x50);

AKA: you are setting up various "corecfg" stuff that's documented in
the generic Arasan docs.  Others SDHCI-Arasan implementations might
want to set the same things, but those values may be stored elsewhere
for them.

So if _all_ Arasan implementations need these same values or need the
same logic to figure out these values, then you should do something
that's not chip-specific but something generic.

If you've got a specific weird quirk that's specific to your platform,
then you could do that in a chip-specific init.

Presumably many of the above could just be hardcoded on some
implementations, so they might not be available in a memory-mapped
implementation...


> which seems much easier to handle (and portable).
>
> At any rate, one thing to note from this is that many of these
> bits should probably be initialised based on DT, right?

Probably, or by proving the voltage value of regulations.  Note that I
think DT already gets parsed and sets up caps.  I'm not really an
expert here and I'd let someone who actually knows / maintains SDMMC
comment.  I know for sure that dw_mmc (which I'm way more familiar
with) does things very differently than sdhci (which I'm barely
familiar with).


> For example, the DT has a "non-removable" property, which I think
> should be used to setup SLOT_TYPE_EMBEDDED (if the property is
> present) or SLOT_TYPE_REMOVABLE (if the property is not present)
>
> Looking at Documentation/devicetree/bindings/mmc/mmc.txt we can see
> more related properties:
>
> Optional properties:
> - bus-width: Number of data lines, can be <1>, <4>, or <8>.  The default
>   will be <1> if the property is absent.
> - wp-gpios: Specify GPIOs for write protection, see gpio binding
> - cd-inverted: when present, polarity on the CD line is inverted. See the note
>   below for the case, when a GPIO is used for the CD line
> - wp-inverted: when present, polarity on the WP line is inverted. See the note
>   below for the case, when a GPIO is used for the WP line
> - disable-wp: When set no physical WP line is present. This property should
>   only be specified when the controller has a dedicated write-protect
>   detection logic. If a GPIO is always used for the write-protect detection
>   logic it is sufficient to not specify wp-gpios property in the absence of a WP
>   line.
> - max-frequency: maximum operating clock frequency
> - no-1-8-v: when present, denotes that 1.8v card voltage is not supported on
>   this system, even if the controller claims it is.
> - cap-sd-highspeed: SD high-speed timing is supported
> - cap-mmc-highspeed: MMC high-speed timing is supported
> - sd-uhs-sdr12: SD UHS SDR12 speed is supported
> - sd-uhs-sdr25: SD UHS SDR25 speed is supported
> - sd-uhs-sdr50: SD UHS SDR50 speed is supported
> - sd-uhs-sdr104: SD UHS SDR104 speed is supported
> - sd-uhs-ddr50: SD UHS DDR50 speed is supported
> ...
>
> which makes me wonder, what is the recommended way of dealing with this?
> - Should I use properties on the DT? If so, then I need to add code to set
> up the register properly.
> - Should I hardcode these values use a minimal DT? If so, then I need an
> init function to put all this.
> - Should I hardcode stuff at u-Boot level? If so, nothing is required and
> should work without any modifications to the Arasan Linux driver.
>
> It appears that the Linux driver is expecting most of these fields to be
> hardcoded and "pre-set" before (maybe by the bootloader) it starts, hence
> the lack of any "init" function so far.
>
>>
>> In your platform-specific init you're proposing you could set
>> tango_pad_mode to 0.  That seems tango-specific.
>>
>> You'd want to hook into "set_ios" for setting sel_sdio or not.  That's
>> important if anyone ever wants to plug in an external SDIO card to
>> your slot.  This one good argument for putting this in
>> sdhci_arasan_soc_ctl_map, since you wouldn't want to do a
>> compatibility matching every time set_ios is called.
>
> Thanks for the advice, I will look into that.
>
>>
>> I'd have to look more into the whole SD/WP polarity issue.  There are
>> already so many levels of inversion for these signals in Linux that
>> it's confusing.  It seems like you might just want to hardcode them to
>> "0" and let users use all the existing ways to invert things...  You
>> could either put that hardcoding into your platform init code or (if
>> you're using sdhci_arasan_soc_ctl_map) put it in the main init code so
>> that if anyone else needs to init similar signals then they can take
>> advantage of it.
>
> Yes, I think I will leave them to 0.
>
>>
>> --
>>
>> One random side note is that as currently documented you need to
>> specify a "shift" of -1 for any sdhci_arasan_soc_ctl_map fields you
>> aren't using.  That seems stupid--not sure why I did that.  It seems
>> better to clue off "width = 0" so that you could just freely not init
>> any fields you don't need.
>
> I see.
> So far I'm not really convinced about using "soc_ctl_map" because what I
> have so far is more portable, and can easily be put as is somewhere else
> (i.e.: in different flavors of bootloaders)

Well, most of your parameters are generic corecfg parameters for
Asasan.  Seems like they would fit into the map nicely...

-Doug

Re: Adding a .platform_init callback to sdhci_arasan_ops

[Sebastian Frias]

Hi,

On 05/12/16 17:30, Doug Anderson wrote:
<snip>
> 
> AKA: you are setting up various "corecfg" stuff that's documented in
> the generic Arasan docs.  Others SDHCI-Arasan implementations might
> want to set the same things, but those values may be stored elsewhere
> for them.

Exactly, that is what I'm trying to find out.
To me, one good place to store this would be DT.

> 
> So if _all_ Arasan implementations need these same values or need the
> same logic to figure out these values, then you should do something
> that's not chip-specific but something generic.
> 

It depends on where this needs to be set, and why am I the first to need
to set this up.

> If you've got a specific weird quirk that's specific to your platform,
> then you could do that in a chip-specific init.

Yes, or in the set_ios you talked earlier.

> 
> Presumably many of the above could just be hardcoded on some
> implementations, so they might not be available in a memory-mapped
> implementation...
> 

Exactly.

> 
>> which seems much easier to handle (and portable).
>>
>> At any rate, one thing to note from this is that many of these
>> bits should probably be initialised based on DT, right?
> 
> Probably, or by proving the voltage value of regulations.  Note that I
> think DT already gets parsed and sets up caps.  I'm not really an
> expert here and I'd let someone who actually knows / maintains SDMMC
> comment.  I know for sure that dw_mmc (which I'm way more familiar
> with) does things very differently than sdhci (which I'm barely
> familiar with).

Thanks.
Could somebody else comment please?

Best regards,

Sebastian

> 
> 
>> For example, the DT has a "non-removable" property, which I think
>> should be used to setup SLOT_TYPE_EMBEDDED (if the property is
>> present) or SLOT_TYPE_REMOVABLE (if the property is not present)
>>
>> Looking at Documentation/devicetree/bindings/mmc/mmc.txt we can see
>> more related properties:
>>
>> Optional properties:
>> - bus-width: Number of data lines, can be <1>, <4>, or <8>.  The default
>>   will be <1> if the property is absent.
>> - wp-gpios: Specify GPIOs for write protection, see gpio binding
>> - cd-inverted: when present, polarity on the CD line is inverted. See the note
>>   below for the case, when a GPIO is used for the CD line
>> - wp-inverted: when present, polarity on the WP line is inverted. See the note
>>   below for the case, when a GPIO is used for the WP line
>> - disable-wp: When set no physical WP line is present. This property should
>>   only be specified when the controller has a dedicated write-protect
>>   detection logic. If a GPIO is always used for the write-protect detection
>>   logic it is sufficient to not specify wp-gpios property in the absence of a WP
>>   line.
>> - max-frequency: maximum operating clock frequency
>> - no-1-8-v: when present, denotes that 1.8v card voltage is not supported on
>>   this system, even if the controller claims it is.
>> - cap-sd-highspeed: SD high-speed timing is supported
>> - cap-mmc-highspeed: MMC high-speed timing is supported
>> - sd-uhs-sdr12: SD UHS SDR12 speed is supported
>> - sd-uhs-sdr25: SD UHS SDR25 speed is supported
>> - sd-uhs-sdr50: SD UHS SDR50 speed is supported
>> - sd-uhs-sdr104: SD UHS SDR104 speed is supported
>> - sd-uhs-ddr50: SD UHS DDR50 speed is supported
>> ...
>>
>> which makes me wonder, what is the recommended way of dealing with this?
>> - Should I use properties on the DT? If so, then I need to add code to set
>> up the register properly.
>> - Should I hardcode these values use a minimal DT? If so, then I need an
>> init function to put all this.
>> - Should I hardcode stuff at u-Boot level? If so, nothing is required and
>> should work without any modifications to the Arasan Linux driver.
>>
>> It appears that the Linux driver is expecting most of these fields to be
>> hardcoded and "pre-set" before (maybe by the bootloader) it starts, hence
>> the lack of any "init" function so far.
>>
>>>
>>> In your platform-specific init you're proposing you could set
>>> tango_pad_mode to 0.  That seems tango-specific.
>>>
>>> You'd want to hook into "set_ios" for setting sel_sdio or not.  That's
>>> important if anyone ever wants to plug in an external SDIO card to
>>> your slot.  This one good argument for putting this in
>>> sdhci_arasan_soc_ctl_map, since you wouldn't want to do a
>>> compatibility matching every time set_ios is called.
>>
>> Thanks for the advice, I will look into that.
>>
>>>
>>> I'd have to look more into the whole SD/WP polarity issue.  There are
>>> already so many levels of inversion for these signals in Linux that
>>> it's confusing.  It seems like you might just want to hardcode them to
>>> "0" and let users use all the existing ways to invert things...  You
>>> could either put that hardcoding into your platform init code or (if
>>> you're using sdhci_arasan_soc_ctl_map) put it in the main init code so
>>> that if anyone else needs to init similar signals then they can take
>>> advantage of it.
>>
>> Yes, I think I will leave them to 0.
>>
>>>
>>> --
>>>
>>> One random side note is that as currently documented you need to
>>> specify a "shift" of -1 for any sdhci_arasan_soc_ctl_map fields you
>>> aren't using.  That seems stupid--not sure why I did that.  It seems
>>> better to clue off "width = 0" so that you could just freely not init
>>> any fields you don't need.
>>
>> I see.
>> So far I'm not really convinced about using "soc_ctl_map" because what I
>> have so far is more portable, and can easily be put as is somewhere else
>> (i.e.: in different flavors of bootloaders)
> 
> Well, most of your parameters are generic corecfg parameters for
> Asasan.  Seems like they would fit into the map nicely...
> 
> -Doug
>