Message-ID: <f5dcc018-bd60-87a7-798b-efc261e443dd@rock-chips.com>
Date:	Mon, 13 Jun 2016 16:36:59 +0800
From:	Shawn Lin <shawn.lin@...k-chips.com>
To:	Douglas Anderson <dianders@...omium.org>, ulf.hansson@...aro.org,
	kishon@...com, Heiko Stuebner <heiko@...ech.de>, robh+dt@...nel.org
Cc:	shawn.lin@...k-chips.com, xzy.xu@...k-chips.com,
	briannorris@...omium.org, adrian.hunter@...el.com,
	linux-rockchip@...ts.infradead.org, linux-mmc@...r.kernel.org,
	devicetree@...r.kernel.org, michal.simek@...inx.com,
	soren.brinkmann@...inx.com, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 04/11] mmc: sdhci-of-arasan: Properly set
 corecfg_baseclkfreq on rk3399

在 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@...omium.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",
>


-- 
Best Regards
Shawn Lin

Powered by blists - more mailing lists