Hi Doug, On 28/11/16 18:46, Doug Anderson wrote: > Hi, > > On Mon, Nov 28, 2016 at 6:39 AM, Sebastian Frias <[email protected]> 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.
