| Message ID | 1465339484-969-5-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
在 2016/6/8 6:44, Douglas Anderson 写道: > In the the earlier change in this series ("Documentation: mmc: > sdhci-of-arasan: Add soc-ctl-syscon for corecfg regs") we can see the > mechansim for specifying a syscon to properly set corecfg registers in > sdhci-of-arasan. Now let's use this mechanism to properly set > corecfg_baseclkfreq on rk3399. > >>From [1] the corecfg_baseclkfreq is supposed to be set to: > Base Clock Frequency for SD Clock. > This is the frequency of the xin_clk. > > This is a relatively easy thing to do. Note that we assume that xin_clk > is not dynamic and we can check the clock at probe time. If any real > devices have a dynamic xin_clk future patches could register for > notifiers for the clock. > > At the moment, setting corecfg_baseclkfreq is only supported for rk3399 > since we need a specific map for each implementation. The code is > written in a generic way that should make this easy to extend to other > SoCs. Note that a specific compatible string for rk3399 is already in > use and so we add that to the table to match rk3399. > > [1]: https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > drivers/mmc/host/sdhci-of-arasan.c | 189 ++++++++++++++++++++++++++++++++++--- > 1 file changed, 178 insertions(+), 11 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c > index 3ff1711077c2..859ea1b21215 100644 > --- a/drivers/mmc/host/sdhci-of-arasan.c > +++ b/drivers/mmc/host/sdhci-of-arasan.c > @@ -22,6 +22,8 @@ > #include <linux/module.h> > #include <linux/of_device.h> > #include <linux/phy/phy.h> > +#include <linux/regmap.h> > +#include <linux/mfd/syscon.h> alphabetical order > #include "sdhci-pltfm.h" > > #define SDHCI_ARASAN_CLK_CTRL_OFFSET 0x2c > @@ -32,18 +34,115 @@ > #define CLK_CTRL_TIMEOUT_MASK (0xf << CLK_CTRL_TIMEOUT_SHIFT) > #define CLK_CTRL_TIMEOUT_MIN_EXP 13 > > +/* > + * On some SoCs the syscon area has a feature where the upper 16-bits of > + * each 32-bit register act as a write mask for the lower 16-bits. This allows > + * atomic updates of the register without locking. This macro is used on SoCs > + * that have that feature. > + */ > +#define HIWORD_UPDATE(val, mask, shift) \ > + ((val) << (shift) | (mask) << ((shift) + 16)) > + > +/** > + * struct sdhci_arasan_soc_ctl_field - Field used in sdhci_arasan_soc_ctl_map > + * > + * @reg: Offset within the syscon of the register containing this field > + * @width: Number of bits for this field > + * @shift: Bit offset within @reg of this field (or -1 if not avail) > + */ > +struct sdhci_arasan_soc_ctl_field { > + u32 reg; > + u16 width; > + s16 shift; > +}; > + > +/** > + * struct sdhci_arasan_soc_ctl_map - Map in syscon to corecfg registers > + * > + * It's up to the licensee of the Arsan IP block to make these available > + * somewhere if needed. Presumably these will be scattered somewhere that's > + * accessible via the syscon API. > + * > + * @baseclkfreq: Where to find corecfg_baseclkfreq > + * @hiword_update: If true, use HIWORD_UPDATE to access the syscon > + */ > +struct sdhci_arasan_soc_ctl_map { > + struct sdhci_arasan_soc_ctl_field baseclkfreq; > + bool hiword_update; > +}; > + > /** > * struct sdhci_arasan_data > - * @clk_ahb: Pointer to the AHB clock > - * @phy: Pointer to the generic phy > - * @phy_on: True if the PHY is turned on. > + * @clk_ahb: Pointer to the AHB clock > + * @phy: Pointer to the generic phy > + * @phy_on: True if the PHY is turned on. > + * @soc_ctl_base: Pointer to regmap for syscon for soc_ctl registers. > + * @soc_ctl_map: Map to get offsets into soc_ctl registers. > */ > struct sdhci_arasan_data { > struct clk *clk_ahb; > struct phy *phy; > bool phy_on; > + > + struct regmap *soc_ctl_base; > + const struct sdhci_arasan_soc_ctl_map *soc_ctl_map; > +}; > + > +static const struct sdhci_arasan_soc_ctl_map rk3399_soc_ctl_map = { > + .baseclkfreq = { .reg = 0xf000, .width = 8, .shift = 8 }, > + .hiword_update = true, > }; > > +/** > + * sdhci_arasan_syscon_write - Write to a field in soc_ctl registers > + * > + * This function allows writing to fields in sdhci_arasan_soc_ctl_map. > + * Note that if a field is specified as not available (shift < 0) then > + * this function will silently return an error code. It will be noisy > + * and print errors for any other (unexpected) errors. > + * > + * @host: The sdhci_host > + * @fld: The field to write to > + * @val: The value to write > + */ > +static int sdhci_arasan_syscon_write(struct sdhci_host *host, > + const struct sdhci_arasan_soc_ctl_field *fld, > + u32 val) > +{ > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host); > + struct regmap *soc_ctl_base = sdhci_arasan->soc_ctl_base; > + u32 reg = fld->reg; > + u16 width = fld->width; > + s16 shift = fld->shift; > + int ret; > + > + /* > + * Silently return errors for shift < 0 so caller doesn't have > + * to check for fields which are optional. For fields that > + * are required then caller needs to do something special > + * anyway. > + */ > + if (shift < 0) > + return -EINVAL; > + > + if (sdhci_arasan->soc_ctl_map->hiword_update) > + ret = regmap_write(soc_ctl_base, reg, > + HIWORD_UPDATE(val, GENMASK(width, 0), > + shift)); > + else > + ret = regmap_update_bits(soc_ctl_base, reg, > + GENMASK(shift + width, shift), > + val << shift); > + > + /* Yell about (unexpected) regmap errors */ > + if (ret) > + pr_warn("%s: Regmap write fail: %d\n", > + mmc_hostname(host->mmc), ret); > + > + return ret; > +} > + > static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host) > { > u32 div; > @@ -191,9 +290,66 @@ 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_update_baseclkfreq - Set corecfg_baseclkfreq > + * > + * The corecfg_baseclkfreq is supposed to contain the MHz of clk_xin. This > + * function can be used to make that happen. > + * > + * NOTES: > + * - 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. > + * - It's assumed that clk_xin is not dynamic and that we use the SDHCI divider > + * to achieve lower clock rates. That means that this function is called once > + * at probe time and never called again. > + * > + * @host: The sdhci_host > + */ > +static void sdhci_arasan_update_baseclkfreq(struct sdhci_host *host) > +{ > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host); > + const struct sdhci_arasan_soc_ctl_map *soc_ctl_map = > + sdhci_arasan->soc_ctl_map; > + u32 mhz = DIV_ROUND_CLOSEST(clk_get_rate(pltfm_host->clk), 1000000); > + It's broken when reading capabilities reg on RK3399 platform which means you should get it via clk framework. But you should consider the non-broken case. > + /* Having a map is optional */ > + if (!soc_ctl_map) > + return; > + > + /* If we have a map, we expect to have a syscon */ > + if (!sdhci_arasan->soc_ctl_base) { > + pr_warn("%s: Have regmap, but no soc-ctl-syscon\n", > + mmc_hostname(host->mmc)); > + return; > + } > + > + sdhci_arasan_syscon_write(host, &soc_ctl_map->baseclkfreq, mhz); should we check the return value, and if not -EINVAL, we should give another try? > +} > + > static int sdhci_arasan_probe(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; > @@ -207,6 +363,23 @@ static int sdhci_arasan_probe(struct platform_device *pdev) > pltfm_host = sdhci_priv(host); > sdhci_arasan = sdhci_pltfm_priv(pltfm_host); > > + match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node); > + sdhci_arasan->soc_ctl_map = match->data; > + > + node = of_parse_phandle(pdev->dev.of_node, "arasan,soc-ctl-syscon", 0); should it be within the scope of arasan,sdhci-5.1 or rockchip,rk3399-sdhci-5.1? > + if (node) { > + sdhci_arasan->soc_ctl_base = syscon_node_to_regmap(node); > + of_node_put(node); > + > + if (IS_ERR(sdhci_arasan->soc_ctl_base)) { > + ret = PTR_ERR(sdhci_arasan->soc_ctl_base); > + if (ret != -EPROBE_DEFER) > + dev_err(&pdev->dev, "Can't get syscon: %d\n", > + 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"); > @@ -236,6 +409,8 @@ static int sdhci_arasan_probe(struct platform_device *pdev) > sdhci_get_of_property(pdev); > pltfm_host->clk = clk_xin; > > + sdhci_arasan_update_baseclkfreq(host); > + > ret = mmc_of_parse(host->mmc); > if (ret) { > dev_err(&pdev->dev, "parsing dt failed (%u)\n", ret); > @@ -301,14 +476,6 @@ static int sdhci_arasan_remove(struct platform_device *pdev) > return ret; > } > > -static const struct of_device_id sdhci_arasan_of_match[] = { > - { .compatible = "arasan,sdhci-8.9a" }, > - { .compatible = "arasan,sdhci-5.1" }, > - { .compatible = "arasan,sdhci-4.9a" }, > - { } > -}; > -MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match); > - > static struct platform_driver sdhci_arasan_driver = { > .driver = { > .name = "sdhci-arasan", >
Shawn, On Mon, Jun 13, 2016 at 1:36 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote: > 在 2016/6/8 6:44, Douglas Anderson 写道: >> >> In the the earlier change in this series ("Documentation: mmc: >> sdhci-of-arasan: Add soc-ctl-syscon for corecfg regs") we can see the >> mechansim for specifying a syscon to properly set corecfg registers in >> sdhci-of-arasan. Now let's use this mechanism to properly set >> corecfg_baseclkfreq on rk3399. >> >>> From [1] the corecfg_baseclkfreq is supposed to be set to: >> >> Base Clock Frequency for SD Clock. >> This is the frequency of the xin_clk. >> >> This is a relatively easy thing to do. Note that we assume that xin_clk >> is not dynamic and we can check the clock at probe time. If any real >> devices have a dynamic xin_clk future patches could register for >> notifiers for the clock. >> >> At the moment, setting corecfg_baseclkfreq is only supported for rk3399 >> since we need a specific map for each implementation. The code is >> written in a generic way that should make this easy to extend to other >> SoCs. Note that a specific compatible string for rk3399 is already in >> use and so we add that to the table to match rk3399. >> >> [1]: >> https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf >> >> Signed-off-by: Douglas Anderson <dianders@chromium.org> >> --- >> drivers/mmc/host/sdhci-of-arasan.c | 189 >> ++++++++++++++++++++++++++++++++++--- >> 1 file changed, 178 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c >> b/drivers/mmc/host/sdhci-of-arasan.c >> index 3ff1711077c2..859ea1b21215 100644 >> --- a/drivers/mmc/host/sdhci-of-arasan.c >> +++ b/drivers/mmc/host/sdhci-of-arasan.c >> @@ -22,6 +22,8 @@ >> #include <linux/module.h> >> #include <linux/of_device.h> >> #include <linux/phy/phy.h> >> +#include <linux/regmap.h> >> +#include <linux/mfd/syscon.h> > > > alphabetical order It was, but with a different definition of alphabetical order. ;) "syscon" (s) comes after "regmap" (r). ...but your way is better, you're right. >> +/** >> + * sdhci_arasan_update_baseclkfreq - Set corecfg_baseclkfreq >> + * >> + * The corecfg_baseclkfreq is supposed to contain the MHz of clk_xin. >> This >> + * function can be used to make that happen. >> + * >> + * NOTES: >> + * - 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. >> + * - It's assumed that clk_xin is not dynamic and that we use the SDHCI >> divider >> + * to achieve lower clock rates. That means that this function is >> called once >> + * at probe time and never called again. >> + * >> + * @host: The sdhci_host >> + */ >> +static void sdhci_arasan_update_baseclkfreq(struct sdhci_host *host) >> +{ >> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> + struct sdhci_arasan_data *sdhci_arasan = >> sdhci_pltfm_priv(pltfm_host); >> + const struct sdhci_arasan_soc_ctl_map *soc_ctl_map = >> + sdhci_arasan->soc_ctl_map; >> + u32 mhz = DIV_ROUND_CLOSEST(clk_get_rate(pltfm_host->clk), >> 1000000); >> + > > > It's broken when reading capabilities reg on RK3399 platform > which means you should get it via clk framework. But you should consider > the non-broken case. I'm afraid I don't understand. Can you elaborate? Are you saying if things weren't broken then we wouldn't have to ask the common clock framework for this? Where would we get this value? ...or are you suggesting that in other SoCs you wouldn't need to set corecfg_baseclkfreq because it would be somehow automatic? If that's so then those SoCs should just set -1 for "shift" in their map and this function will be a no-op. >> + /* Having a map is optional */ >> + if (!soc_ctl_map) >> + return; >> + >> + /* If we have a map, we expect to have a syscon */ >> + if (!sdhci_arasan->soc_ctl_base) { >> + pr_warn("%s: Have regmap, but no soc-ctl-syscon\n", >> + mmc_hostname(host->mmc)); >> + return; >> + } >> + >> + sdhci_arasan_syscon_write(host, &soc_ctl_map->baseclkfreq, mhz); > > > should we check the return value, and if not -EINVAL, we should give > another try? I don't see a reason to check the return code here. Specifically: * If this is a SoC where you don't need to write corecfg_baseclkfreq then we need do nothing about this error. * If the regmap write failed (which would be terribly unexpected for a memory mapped register) then we've already printed an error (in sdhci_arasan_syscon_write). Best course of action seems to keep going and try anyway. I don't think a retry is likely to help anything. >> +} >> + >> static int sdhci_arasan_probe(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; >> @@ -207,6 +363,23 @@ static int sdhci_arasan_probe(struct platform_device >> *pdev) >> pltfm_host = sdhci_priv(host); >> sdhci_arasan = sdhci_pltfm_priv(pltfm_host); >> >> + match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node); >> + sdhci_arasan->soc_ctl_map = match->data; >> + >> + node = of_parse_phandle(pdev->dev.of_node, >> "arasan,soc-ctl-syscon", 0); > > > should it be within the scope of arasan,sdhci-5.1 or > rockchip,rk3399-sdhci-5.1? IMHO it doesn't hurt to do this for all SoCs. This will help facilitate other SoCs adding their own syscon so that they can properly modify corecfg registers. I can't say that I know anything about the other Arasan PHYs, but presumably they also have corecfg registers. If/when maps are added for SoCs including those PHYs then this would be useful for them. If you want I could change the code to only call of_parse_phandle if "sdhci_arasan->soc_ctl_map" was non-NULL, but that should have no functional change since we'll never actually use the syscon if we have no map. -Doug
在 2016/6/14 7:06, Doug Anderson 写道: > Shawn, > > On Mon, Jun 13, 2016 at 1:36 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> 在 2016/6/8 6:44, Douglas Anderson 写道: >>> >>> In the the earlier change in this series ("Documentation: mmc: >>> sdhci-of-arasan: Add soc-ctl-syscon for corecfg regs") we can see the >>> mechansim for specifying a syscon to properly set corecfg registers in >>> sdhci-of-arasan. Now let's use this mechanism to properly set >>> corecfg_baseclkfreq on rk3399. >>> >>>> From [1] the corecfg_baseclkfreq is supposed to be set to: >>> >>> Base Clock Frequency for SD Clock. >>> This is the frequency of the xin_clk. >>> >>> This is a relatively easy thing to do. Note that we assume that xin_clk >>> is not dynamic and we can check the clock at probe time. If any real >>> devices have a dynamic xin_clk future patches could register for >>> notifiers for the clock. >>> >>> At the moment, setting corecfg_baseclkfreq is only supported for rk3399 >>> since we need a specific map for each implementation. The code is >>> written in a generic way that should make this easy to extend to other >>> SoCs. Note that a specific compatible string for rk3399 is already in >>> use and so we add that to the table to match rk3399. >>> >>> [1]: >>> https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf >>> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >>> --- >>> drivers/mmc/host/sdhci-of-arasan.c | 189 >>> ++++++++++++++++++++++++++++++++++--- >>> 1 file changed, 178 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c >>> b/drivers/mmc/host/sdhci-of-arasan.c >>> index 3ff1711077c2..859ea1b21215 100644 >>> --- a/drivers/mmc/host/sdhci-of-arasan.c >>> +++ b/drivers/mmc/host/sdhci-of-arasan.c >>> @@ -22,6 +22,8 @@ >>> #include <linux/module.h> >>> #include <linux/of_device.h> >>> #include <linux/phy/phy.h> >>> +#include <linux/regmap.h> >>> +#include <linux/mfd/syscon.h> >> >> >> alphabetical order > > It was, but with a different definition of alphabetical order. ;) > "syscon" (s) comes after "regmap" (r). ...but your way is better, > you're right. > > >>> +/** >>> + * sdhci_arasan_update_baseclkfreq - Set corecfg_baseclkfreq >>> + * >>> + * The corecfg_baseclkfreq is supposed to contain the MHz of clk_xin. >>> This >>> + * function can be used to make that happen. >>> + * >>> + * NOTES: >>> + * - 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. >>> + * - It's assumed that clk_xin is not dynamic and that we use the SDHCI >>> divider >>> + * to achieve lower clock rates. That means that this function is >>> called once >>> + * at probe time and never called again. >>> + * >>> + * @host: The sdhci_host >>> + */ >>> +static void sdhci_arasan_update_baseclkfreq(struct sdhci_host *host) >>> +{ >>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>> + struct sdhci_arasan_data *sdhci_arasan = >>> sdhci_pltfm_priv(pltfm_host); >>> + const struct sdhci_arasan_soc_ctl_map *soc_ctl_map = >>> + sdhci_arasan->soc_ctl_map; >>> + u32 mhz = DIV_ROUND_CLOSEST(clk_get_rate(pltfm_host->clk), >>> 1000000); >>> + >> >> >> It's broken when reading capabilities reg on RK3399 platform >> which means you should get it via clk framework. But you should consider >> the non-broken case. > > I'm afraid I don't understand. Can you elaborate? Are you saying if > things weren't broken then we wouldn't have to ask the common clock > framework for this? Where would we get this value? I mean bascially we should get baseclk from capabilities register[15:8] (offset 0x40@sdhci), namely EMMCCORE_CAP on TRM. Only when you get 0x0 from there, can you consider to get it from clock framework. > > ...or are you suggesting that in other SoCs you wouldn't need to set > corecfg_baseclkfreq because it would be somehow automatic? capabilities register[15:8] should reflect the real baseclkfreq automatic if implemented. It's broken for rk3399 which means you should get it via another method just as what TRM said, but not really for other arasan IP I guess. If that's > so then those SoCs should just set -1 for "shift" in their map and > this function will be a no-op. > > >>> + /* Having a map is optional */ >>> + if (!soc_ctl_map) >>> + return; >>> + >>> + /* If we have a map, we expect to have a syscon */ >>> + if (!sdhci_arasan->soc_ctl_base) { >>> + pr_warn("%s: Have regmap, but no soc-ctl-syscon\n", >>> + mmc_hostname(host->mmc)); >>> + return; >>> + } >>> + >>> + sdhci_arasan_syscon_write(host, &soc_ctl_map->baseclkfreq, mhz); >> >> >> should we check the return value, and if not -EINVAL, we should give >> another try? > > I don't see a reason to check the return code here. Specifically: > > * If this is a SoC where you don't need to write corecfg_baseclkfreq > then we need do nothing about this error. > > * If the regmap write failed (which would be terribly unexpected for a > memory mapped register) then we've already printed an error (in > sdhci_arasan_syscon_write). Best course of action seems to keep going > and try anyway. > > > I don't think a retry is likely to help anything. Well, I saw you add a return value for sdhci_arasan_syscon_write, so should we remove it? > > >>> +} >>> + >>> static int sdhci_arasan_probe(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; >>> @@ -207,6 +363,23 @@ static int sdhci_arasan_probe(struct platform_device >>> *pdev) >>> pltfm_host = sdhci_priv(host); >>> sdhci_arasan = sdhci_pltfm_priv(pltfm_host); >>> >>> + match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node); >>> + sdhci_arasan->soc_ctl_map = match->data; >>> + >>> + node = of_parse_phandle(pdev->dev.of_node, >>> "arasan,soc-ctl-syscon", 0); >> >> >> should it be within the scope of arasan,sdhci-5.1 or >> rockchip,rk3399-sdhci-5.1? > > IMHO it doesn't hurt to do this for all SoCs. This will help > facilitate other SoCs adding their own syscon so that they can > properly modify corecfg registers. okay. > > I can't say that I know anything about the other Arasan PHYs, but > presumably they also have corecfg registers. If/when maps are added > for SoCs including those PHYs then this would be useful for them. > > If you want I could change the code to only call of_parse_phandle if > "sdhci_arasan->soc_ctl_map" was non-NULL, but that should have no > functional change since we'll never actually use the syscon if we have > no map. > > > -Doug > > >
Hi, On Mon, Jun 13, 2016 at 5:14 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote: >>> It's broken when reading capabilities reg on RK3399 platform >>> which means you should get it via clk framework. But you should consider >>> the non-broken case. >> >> >> I'm afraid I don't understand. Can you elaborate? Are you saying if >> things weren't broken then we wouldn't have to ask the common clock >> framework for this? Where would we get this value? > > > > I mean bascially we should get baseclk from capabilities register[15:8] > (offset 0x40@sdhci), namely EMMCCORE_CAP on TRM. Only when you get 0x0 > from there, can you consider to get it from clock framework. Ah, got it! I guess I would be super surprised if an SoC implemented register[15:8] but still then required you to manually copy that value to corecfg_baseclkfreq. Presumably nobody would be crazy enough to try to measure the clock rate in the SDHCI driver, so this would probably only be non-zero where the SDHCI clock is totally fixed. ...in that case probably the SoC designer would also put a default value in corecfg_baseclkfreq that matched (and maybe even make corecfg_baseclkfreq read-only?). Even in the case that an SoC designer didn't put a value into corecfg_baseclkfreq that matched register[15:8], it seems very likely that the rate returned from the clk_get_rate() would match. I guess what I'm saying is that, to me, it seems like my patch isn't broken in any real systems. If we ever find a system that needs this behavior in the future, we can add it. Until then, it seems like my patch would be fine. Do you agree? Note: right now we only provide a register map for rk3399, so certainly we can't be breaking any other SoCs with our current method. > I don't see a reason to check the return code here. Specifically: >> >> * If this is a SoC where you don't need to write corecfg_baseclkfreq >> then we need do nothing about this error. >> >> * If the regmap write failed (which would be terribly unexpected for a >> memory mapped register) then we've already printed an error (in >> sdhci_arasan_syscon_write). Best course of action seems to keep going >> and try anyway. >> >> >> I don't think a retry is likely to help anything. > > > Well, I saw you add a return value for sdhci_arasan_syscon_write, so > should we remove it? It was presuming that there might be future callers who might want to write other corecfg registers and might need to know whether the write worked or not. Since having the return value doesn't hurt anything I'd rather leave it in. If you really want me to remove it, though, I will. Just let me know. -Doug
在 2016/6/14 8:43, Doug Anderson 写道: > Hi, > > On Mon, Jun 13, 2016 at 5:14 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote: >>>> It's broken when reading capabilities reg on RK3399 platform >>>> which means you should get it via clk framework. But you should consider >>>> the non-broken case. >>> >>> >>> I'm afraid I don't understand. Can you elaborate? Are you saying if >>> things weren't broken then we wouldn't have to ask the common clock >>> framework for this? Where would we get this value? >> >> >> >> I mean bascially we should get baseclk from capabilities register[15:8] >> (offset 0x40@sdhci), namely EMMCCORE_CAP on TRM. Only when you get 0x0 >> from there, can you consider to get it from clock framework. > > Ah, got it! > > I guess I would be super surprised if an SoC implemented > register[15:8] but still then required you to manually copy that value > to corecfg_baseclkfreq. Presumably nobody would be crazy enough to > try to measure the clock rate in the SDHCI driver, so this would > probably only be non-zero where the SDHCI clock is totally fixed. > ...in that case probably the SoC designer would also put a default > value in corecfg_baseclkfreq that matched (and maybe even make > corecfg_baseclkfreq read-only?). yes, you could see some others similar capabilities case like timeoutclkfreq or preset value, etc. SDHCI hope Soc designer to implement them within the controller but not mandatory. > > Even in the case that an SoC designer didn't put a value into > corecfg_baseclkfreq that matched register[15:8], it seems very likely > that the rate returned from the clk_get_rate() would match. > > I guess what I'm saying is that, to me, it seems like my patch isn't > broken in any real systems. If we ever find a system that needs this > behavior in the future, we can add it. Until then, it seems like my > patch would be fine. Do you agree? I agree. But from the code itself, we should still use SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN to see if we could get it from internal register in case of some platforms don't provide the clk stuff.. Sounds sane? :) > > Note: right now we only provide a register map for rk3399, so > certainly we can't be breaking any other SoCs with our current method. > > >> I don't see a reason to check the return code here. Specifically: >>> >>> * If this is a SoC where you don't need to write corecfg_baseclkfreq >>> then we need do nothing about this error. >>> >>> * If the regmap write failed (which would be terribly unexpected for a >>> memory mapped register) then we've already printed an error (in >>> sdhci_arasan_syscon_write). Best course of action seems to keep going >>> and try anyway. >>> >>> >>> I don't think a retry is likely to help anything. >> >> >> Well, I saw you add a return value for sdhci_arasan_syscon_write, so >> should we remove it? > > It was presuming that there might be future callers who might want to > write other corecfg registers and might need to know whether the write > worked or not. Since having the return value doesn't hurt anything > I'd rather leave it in. If you really want me to remove it, though, I > will. Just let me know. Ahh, it's trivial, so keep it if you want. > > > -Doug > > >
Shawn, On Mon, Jun 13, 2016 at 5:59 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> Even in the case that an SoC designer didn't put a value into >> corecfg_baseclkfreq that matched register[15:8], it seems very likely >> that the rate returned from the clk_get_rate() would match. >> >> I guess what I'm saying is that, to me, it seems like my patch isn't >> broken in any real systems. If we ever find a system that needs this >> behavior in the future, we can add it. Until then, it seems like my >> patch would be fine. Do you agree? > > > I agree. But from the code itself, we should still use > SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN to see if we could get > it from internal register in case of some platforms don't > provide the clk stuff.. Sounds sane? :) Could we wait until there exists a SoC that needs to provide baseclkfreq in its sdhci_arasan_soc_ctl_map table and that needs this value copied from register[15:8]? AKA: A) If you have a SoC where clk_get_rate() is right and software needs to set baseclkfreq manualy, then you should include "baseclkfreq" in your sdhci_arasan_soc_ctl_map table. This is like rk3399. Note that if _both_ clk_get_rate() and register[15:8] are right, that's fine. We can still use clk_get_rate() since it will be exactly the same as register[15:8]. B) If you have a SoC that doesn't even expose corecfg_baseclkfreq to software control, just don't include "baseclkfreq" in your sdhci_arasan_soc_ctl_map table. Easy. This is how my patch treats anyone using the current "generic" bindings, but you could easily just specify an offset of "-1" for baseclkfreq if you didn't want to use the generic bindings but couldn't control baseclkfreq. C) If you have a SoC that provides a valid value in register[15:8] and clk_get_rate() is wrong and software is required to copy the value from register[15:8] to baseclkfreq, technically we should fix clk_get_rate() anyway. It's good when common clock framework provides correct values. NOTE: It seems very unlikely to me that register[15:8] would be right AND that software would be required to copy this value to baseclkfreq, but I suppose there are some pretty crazy hardware designs out there. D) If you have a SoC that provides a valid value in register[15:8] and clk_get_rate() is wrong and can't be fixed (REALLY?) and software is required to copy the value from register[15:8] to baseclkfreq, we will need to add new code. My assertion is that such a SoC doesn't exist and is unlikely to ever exist, so I am hesitant to add extra code to support this SoC. With my patch, A) and B) are certainly handled. I think C) is unlikely to exist, but if it did exist then I'd say we should fix clk_get_rate(). I think D) is VERY unlikely to exist. If I'm shown proof of D) existing, I'm happy to submit a patch for it. Until we see proof of D)'s existence, I don't think we should clutter the code with support for it. -Doug
On 2016/6/14 10:13, Doug Anderson wrote: > Shawn, > > On Mon, Jun 13, 2016 at 5:59 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote: >>> Even in the case that an SoC designer didn't put a value into >>> corecfg_baseclkfreq that matched register[15:8], it seems very likely >>> that the rate returned from the clk_get_rate() would match. >>> >>> I guess what I'm saying is that, to me, it seems like my patch isn't >>> broken in any real systems. If we ever find a system that needs this >>> behavior in the future, we can add it. Until then, it seems like my >>> patch would be fine. Do you agree? >> >> >> I agree. But from the code itself, we should still use >> SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN to see if we could get >> it from internal register in case of some platforms don't >> provide the clk stuff.. Sounds sane? :) > > Could we wait until there exists a SoC that needs to provide > baseclkfreq in its sdhci_arasan_soc_ctl_map table and that needs this > value copied from register[15:8]? yes, I think the base clk got from clk framework shouldn't make any difference with that from register[15:8] if implemented. And we now decide how to get base clk in a certain variant driver which menas we know that this variant would never implement register[15:8], so it looks fine for your patch with only a nit that we should make sure we toggle up the COMMON_CLK. I saw your v2.1 to deal with it, so I think it's okay now to add Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com> > > AKA: > > A) If you have a SoC where clk_get_rate() is right and software needs > to set baseclkfreq manualy, then you should include "baseclkfreq" in > your sdhci_arasan_soc_ctl_map table. This is like rk3399. Note that > if _both_ clk_get_rate() and register[15:8] are right, that's fine. > We can still use clk_get_rate() since it will be exactly the same as > register[15:8]. > > B) If you have a SoC that doesn't even expose corecfg_baseclkfreq to > software control, just don't include "baseclkfreq" in your > sdhci_arasan_soc_ctl_map table. Easy. This is how my patch treats > anyone using the current "generic" bindings, but you could easily just > specify an offset of "-1" for baseclkfreq if you didn't want to use > the generic bindings but couldn't control baseclkfreq. > > C) If you have a SoC that provides a valid value in register[15:8] and > clk_get_rate() is wrong and software is required to copy the value > from register[15:8] to baseclkfreq, technically we should fix > clk_get_rate() anyway. It's good when common clock framework provides > correct values. NOTE: It seems very unlikely to me that > register[15:8] would be right AND that software would be required to > copy this value to baseclkfreq, but I suppose there are some pretty > crazy hardware designs out there. > > D) If you have a SoC that provides a valid value in register[15:8] and > clk_get_rate() is wrong and can't be fixed (REALLY?) and software is > required to copy the value from register[15:8] to baseclkfreq, we will > need to add new code. My assertion is that such a SoC doesn't exist > and is unlikely to ever exist, so I am hesitant to add extra code to > support this SoC. > > > With my patch, A) and B) are certainly handled. I think C) is > unlikely to exist, but if it did exist then I'd say we should fix > clk_get_rate(). I think D) is VERY unlikely to exist. If I'm shown > proof of D) existing, I'm happy to submit a patch for it. Until we > see proof of D)'s existence, I don't think we should clutter the code > with support for it. > > > -Doug > > >
diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c index 3ff1711077c2..859ea1b21215 100644 --- a/drivers/mmc/host/sdhci-of-arasan.c +++ b/drivers/mmc/host/sdhci-of-arasan.c @@ -22,6 +22,8 @@ #include <linux/module.h> #include <linux/of_device.h> #include <linux/phy/phy.h> +#include <linux/regmap.h> +#include <linux/mfd/syscon.h> #include "sdhci-pltfm.h" #define SDHCI_ARASAN_CLK_CTRL_OFFSET 0x2c @@ -32,18 +34,115 @@ #define CLK_CTRL_TIMEOUT_MASK (0xf << CLK_CTRL_TIMEOUT_SHIFT) #define CLK_CTRL_TIMEOUT_MIN_EXP 13 +/* + * On some SoCs the syscon area has a feature where the upper 16-bits of + * each 32-bit register act as a write mask for the lower 16-bits. This allows + * atomic updates of the register without locking. This macro is used on SoCs + * that have that feature. + */ +#define HIWORD_UPDATE(val, mask, shift) \ + ((val) << (shift) | (mask) << ((shift) + 16)) + +/** + * struct sdhci_arasan_soc_ctl_field - Field used in sdhci_arasan_soc_ctl_map + * + * @reg: Offset within the syscon of the register containing this field + * @width: Number of bits for this field + * @shift: Bit offset within @reg of this field (or -1 if not avail) + */ +struct sdhci_arasan_soc_ctl_field { + u32 reg; + u16 width; + s16 shift; +}; + +/** + * struct sdhci_arasan_soc_ctl_map - Map in syscon to corecfg registers + * + * It's up to the licensee of the Arsan IP block to make these available + * somewhere if needed. Presumably these will be scattered somewhere that's + * accessible via the syscon API. + * + * @baseclkfreq: Where to find corecfg_baseclkfreq + * @hiword_update: If true, use HIWORD_UPDATE to access the syscon + */ +struct sdhci_arasan_soc_ctl_map { + struct sdhci_arasan_soc_ctl_field baseclkfreq; + bool hiword_update; +}; + /** * struct sdhci_arasan_data - * @clk_ahb: Pointer to the AHB clock - * @phy: Pointer to the generic phy - * @phy_on: True if the PHY is turned on. + * @clk_ahb: Pointer to the AHB clock + * @phy: Pointer to the generic phy + * @phy_on: True if the PHY is turned on. + * @soc_ctl_base: Pointer to regmap for syscon for soc_ctl registers. + * @soc_ctl_map: Map to get offsets into soc_ctl registers. */ struct sdhci_arasan_data { struct clk *clk_ahb; struct phy *phy; bool phy_on; + + struct regmap *soc_ctl_base; + const struct sdhci_arasan_soc_ctl_map *soc_ctl_map; +}; + +static const struct sdhci_arasan_soc_ctl_map rk3399_soc_ctl_map = { + .baseclkfreq = { .reg = 0xf000, .width = 8, .shift = 8 }, + .hiword_update = true, }; +/** + * sdhci_arasan_syscon_write - Write to a field in soc_ctl registers + * + * This function allows writing to fields in sdhci_arasan_soc_ctl_map. + * Note that if a field is specified as not available (shift < 0) then + * this function will silently return an error code. It will be noisy + * and print errors for any other (unexpected) errors. + * + * @host: The sdhci_host + * @fld: The field to write to + * @val: The value to write + */ +static int sdhci_arasan_syscon_write(struct sdhci_host *host, + const struct sdhci_arasan_soc_ctl_field *fld, + u32 val) +{ + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host); + struct regmap *soc_ctl_base = sdhci_arasan->soc_ctl_base; + u32 reg = fld->reg; + u16 width = fld->width; + s16 shift = fld->shift; + int ret; + + /* + * Silently return errors for shift < 0 so caller doesn't have + * to check for fields which are optional. For fields that + * are required then caller needs to do something special + * anyway. + */ + if (shift < 0) + return -EINVAL; + + if (sdhci_arasan->soc_ctl_map->hiword_update) + ret = regmap_write(soc_ctl_base, reg, + HIWORD_UPDATE(val, GENMASK(width, 0), + shift)); + else + ret = regmap_update_bits(soc_ctl_base, reg, + GENMASK(shift + width, shift), + val << shift); + + /* Yell about (unexpected) regmap errors */ + if (ret) + pr_warn("%s: Regmap write fail: %d\n", + mmc_hostname(host->mmc), ret); + + return ret; +} + static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host) { u32 div; @@ -191,9 +290,66 @@ 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_update_baseclkfreq - Set corecfg_baseclkfreq + * + * The corecfg_baseclkfreq is supposed to contain the MHz of clk_xin. This + * function can be used to make that happen. + * + * NOTES: + * - 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. + * - It's assumed that clk_xin is not dynamic and that we use the SDHCI divider + * to achieve lower clock rates. That means that this function is called once + * at probe time and never called again. + * + * @host: The sdhci_host + */ +static void sdhci_arasan_update_baseclkfreq(struct sdhci_host *host) +{ + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); + struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host); + const struct sdhci_arasan_soc_ctl_map *soc_ctl_map = + sdhci_arasan->soc_ctl_map; + u32 mhz = DIV_ROUND_CLOSEST(clk_get_rate(pltfm_host->clk), 1000000); + + /* Having a map is optional */ + if (!soc_ctl_map) + return; + + /* If we have a map, we expect to have a syscon */ + if (!sdhci_arasan->soc_ctl_base) { + pr_warn("%s: Have regmap, but no soc-ctl-syscon\n", + mmc_hostname(host->mmc)); + return; + } + + sdhci_arasan_syscon_write(host, &soc_ctl_map->baseclkfreq, mhz); +} + static int sdhci_arasan_probe(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; @@ -207,6 +363,23 @@ static int sdhci_arasan_probe(struct platform_device *pdev) pltfm_host = sdhci_priv(host); sdhci_arasan = sdhci_pltfm_priv(pltfm_host); + match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node); + sdhci_arasan->soc_ctl_map = match->data; + + node = of_parse_phandle(pdev->dev.of_node, "arasan,soc-ctl-syscon", 0); + if (node) { + sdhci_arasan->soc_ctl_base = syscon_node_to_regmap(node); + of_node_put(node); + + if (IS_ERR(sdhci_arasan->soc_ctl_base)) { + ret = PTR_ERR(sdhci_arasan->soc_ctl_base); + if (ret != -EPROBE_DEFER) + dev_err(&pdev->dev, "Can't get syscon: %d\n", + 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"); @@ -236,6 +409,8 @@ static int sdhci_arasan_probe(struct platform_device *pdev) sdhci_get_of_property(pdev); pltfm_host->clk = clk_xin; + sdhci_arasan_update_baseclkfreq(host); + ret = mmc_of_parse(host->mmc); if (ret) { dev_err(&pdev->dev, "parsing dt failed (%u)\n", ret); @@ -301,14 +476,6 @@ static int sdhci_arasan_remove(struct platform_device *pdev) return ret; } -static const struct of_device_id sdhci_arasan_of_match[] = { - { .compatible = "arasan,sdhci-8.9a" }, - { .compatible = "arasan,sdhci-5.1" }, - { .compatible = "arasan,sdhci-4.9a" }, - { } -}; -MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match); - static struct platform_driver sdhci_arasan_driver = { .driver = { .name = "sdhci-arasan",
In the the earlier change in this series ("Documentation: mmc: sdhci-of-arasan: Add soc-ctl-syscon for corecfg regs") we can see the mechansim for specifying a syscon to properly set corecfg registers in sdhci-of-arasan. Now let's use this mechanism to properly set corecfg_baseclkfreq on rk3399. From [1] the corecfg_baseclkfreq is supposed to be set to: Base Clock Frequency for SD Clock. This is the frequency of the xin_clk. This is a relatively easy thing to do. Note that we assume that xin_clk is not dynamic and we can check the clock at probe time. If any real devices have a dynamic xin_clk future patches could register for notifiers for the clock. At the moment, setting corecfg_baseclkfreq is only supported for rk3399 since we need a specific map for each implementation. The code is written in a generic way that should make this easy to extend to other SoCs. Note that a specific compatible string for rk3399 is already in use and so we add that to the table to match rk3399. [1]: https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/mmc/host/sdhci-of-arasan.c | 189 ++++++++++++++++++++++++++++++++++--- 1 file changed, 178 insertions(+), 11 deletions(-)