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@xxxxxxxxxxxx>
>> 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