[PATCH 0/11] Changes to support 150 MHz eMMC on rk3399

[PATCH 0/11] Changes to support 150 MHz eMMC on rk3399

[Douglas Anderson]

The theme of this series of patches is to try to allow running the eMMC
at 150 MHz on the rk3399 SoC, though the changes should still be correct
and have merit on their own.  The motivation for running at 150 MHz is
that doing so improves signal integrity and (with some eMMC devices)
doesn't affect throughput.

These patches have been structured to keep things as separate as
possible, but nevertheless there are still some dependencies between
patches.  It probably makes the most sense for all of the non-device
tree patches to go through a single tree.  If others agree, perhaps the
most sane would be to get Acks from PHY maintainers and then to land the
patches in the MMC tree.  Device tree patches should be able to be
landed separately and the worst what would happen is a warning in the
kernel log if you have the code without the device tree.

The code patches are based on Ulf's mmc-next, then 4 patches that are
outstanding / ready to land.  Specifically:
- https://patchwork.kernel.org/patch/9086501/
  phy: rockchip-emmc: give DLL some extra time to be ready
- https://patchwork.kernel.org/patch/9093681/
  phy: rockchip-emmc: configure frequency range and drive impedance
- https://patchwork.kernel.org/patch/9086511/
  phy: rockchip-emmc: configure default output tap delay
- https://patchwork.kernel.org/patch/9086531/
  phy: rockchip-emmc: reindent the register definitions

The device tree patches are based on Heiko's v4.8-armsoc/dts64.

If requested, I could repost my series with the outstanding code patches
or I could try folding those patches into mine.  Since those patches
aren't in 4.7-rc1 presumably they would also make sense to take through
the MMC tree if others agree.


Douglas Anderson (11):
  phy: rockchip-emmc: Increase lock time allowance
  mmc: sdhci-of-arasan: Always power the PHY off/on when clock changes
  Documentation: mmc: sdhci-of-arasan: Add soc-ctl-syscon for corecfg
    regs
  mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399
  arm64: dts: rockchip: Add soc-ctl-syscon to sdhci for rk3399
  Documentation: mmc: sdhci-of-arasan: Add ability to export card clock
  mmc: sdhci-of-arasan: Add ability to export card clock
  Documentation: phy: Let the rockchip eMMC PHY get an exported card
    clock
  phy: rockchip-emmc: Set phyctrl_frqsel based on card clock
  phy: rockchip-emmc: Minor code cleanup in
    rockchip_emmc_phy_power_off()
  arm64: dts: rockchip: Provide emmcclk to PHY for rk3399

 .../devicetree/bindings/mmc/arasan,sdhci.txt       |  35 ++-
 .../devicetree/bindings/phy/rockchip-emmc-phy.txt  |   7 +
 arch/arm64/boot/dts/rockchip/rk3399.dtsi           |   5 +
 drivers/mmc/host/sdhci-of-arasan.c                 | 333 +++++++++++++++++++--
 drivers/phy/phy-rockchip-emmc.c                    | 107 +++++--
 5 files changed, 427 insertions(+), 60 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[PATCH 01/11] phy: rockchip-emmc: Increase lock time allowance

[Douglas Anderson]

Previous PHY code waited a fixed amount of time for the DLL to lock at
power on time.  Unfortunately, the time for the DLL to lock is actually
a bit more dynamic and can be longer if the card clock is slower.

Instead of waiting a fixed 30 us, let's now dynamically wait until the
lock bit gets set.  We'll wait up to 10 ms which should be OK even if
the card clock is at the super slow 100 kHz.

On its own, this change makes the PHY power on code a little more
robust.  Before this change the PHY was relying on the eMMC code to make
sure the PHY was only powered on when the card clock was set to at least
50 MHz before, though this reliance wasn't documented anywhere.

This change will be even more useful in future changes where we actually
need to be able to wait for a DLL lock at slower clock speeds.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/phy/phy-rockchip-emmc.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c
index a69f53630e67..8336053aea5c 100644
--- a/drivers/phy/phy-rockchip-emmc.c
+++ b/drivers/phy/phy-rockchip-emmc.c
@@ -85,6 +85,7 @@ static int rockchip_emmc_phy_power(struct rockchip_emmc_phy *rk_phy,
 {
 	unsigned int caldone;
 	unsigned int dllrdy;
+	unsigned long timeout;
 
 	/*
 	 * Keep phyctrl_pdb and phyctrl_endll low to allow
@@ -137,15 +138,25 @@ static int rockchip_emmc_phy_power(struct rockchip_emmc_phy *rk_phy,
 				   PHYCTRL_ENDLL_MASK,
 				   PHYCTRL_ENDLL_SHIFT));
 	/*
-	 * After enable analog DLL circuits, we need an extra 10.2us
-	 * for dll to be ready for work. But according to testing, we
-	 * find some chips need more than 25us.
+	 * After enabling analog DLL circuits docs say that we need 10.2 us if
+	 * our source clock is at 50 MHz and that lock time scales linearly
+	 * with clock speed.  If we are powering on the PHY and the card clock
+	 * is super slow (like 100 kHZ) this could take as long as 5.1 ms.
+	 * Hopefully we won't be running at 100 kHz, but we should still make
+	 * sure we wait long enough.
 	 */
-	udelay(30);
-	regmap_read(rk_phy->reg_base,
-		    rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
-		    &dllrdy);
-	dllrdy = (dllrdy >> PHYCTRL_DLLRDY_SHIFT) & PHYCTRL_DLLRDY_MASK;
+	timeout = jiffies + msecs_to_jiffies(10);
+	do {
+		udelay(1);
+
+		regmap_read(rk_phy->reg_base,
+			rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
+			&dllrdy);
+		dllrdy = (dllrdy >> PHYCTRL_DLLRDY_SHIFT) & PHYCTRL_DLLRDY_MASK;
+		if (dllrdy == PHYCTRL_DLLRDY_DONE)
+			break;
+	} while (!time_after(jiffies, timeout));
+
 	if (dllrdy != PHYCTRL_DLLRDY_DONE) {
 		pr_err("rockchip_emmc_phy_power: dllrdy timeout.\n");
 		return -ETIMEDOUT;
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 01/11] phy: rockchip-emmc: Increase lock time allowance

[Shawn Lin]

On 2016/6/8 6:44, Douglas Anderson wrote:
> Previous PHY code waited a fixed amount of time for the DLL to lock at
> power on time.  Unfortunately, the time for the DLL to lock is actually
> a bit more dynamic and can be longer if the card clock is slower.
>
> Instead of waiting a fixed 30 us, let's now dynamically wait until the
> lock bit gets set.  We'll wait up to 10 ms which should be OK even if
> the card clock is at the super slow 100 kHz.
>

mmc stack limit the min freq to 200k when initializing the card.
So 5ms is enough, but it's ok to set the max timeout to 10ms as we
can break out if locked.

> On its own, this change makes the PHY power on code a little more
> robust.  Before this change the PHY was relying on the eMMC code to make
> sure the PHY was only powered on when the card clock was set to at least
> 50 MHz before, though this reliance wasn't documented anywhere.
>
> This change will be even more useful in future changes where we actually
> need to be able to wait for a DLL lock at slower clock speeds.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>  drivers/phy/phy-rockchip-emmc.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c
> index a69f53630e67..8336053aea5c 100644
> --- a/drivers/phy/phy-rockchip-emmc.c
> +++ b/drivers/phy/phy-rockchip-emmc.c
> @@ -85,6 +85,7 @@ static int rockchip_emmc_phy_power(struct rockchip_emmc_phy *rk_phy,
>  {
>  	unsigned int caldone;
>  	unsigned int dllrdy;
> +	unsigned long timeout;
>
>  	/*
>  	 * Keep phyctrl_pdb and phyctrl_endll low to allow
> @@ -137,15 +138,25 @@ static int rockchip_emmc_phy_power(struct rockchip_emmc_phy *rk_phy,
>  				   PHYCTRL_ENDLL_MASK,
>  				   PHYCTRL_ENDLL_SHIFT));
>  	/*
> -	 * After enable analog DLL circuits, we need an extra 10.2us
> -	 * for dll to be ready for work. But according to testing, we
> -	 * find some chips need more than 25us.
> +	 * After enabling analog DLL circuits docs say that we need 10.2 us if
> +	 * our source clock is at 50 MHz and that lock time scales linearly
> +	 * with clock speed.  If we are powering on the PHY and the card clock
> +	 * is super slow (like 100 kHZ) this could take as long as 5.1 ms.

5.1ms is by calculation or test?

> +	 * Hopefully we won't be running at 100 kHz, but we should still make
> +	 * sure we wait long enough.
>  	 */
> -	udelay(30);
> -	regmap_read(rk_phy->reg_base,
> -		    rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
> -		    &dllrdy);
> -	dllrdy = (dllrdy >> PHYCTRL_DLLRDY_SHIFT) & PHYCTRL_DLLRDY_MASK;
> +	timeout = jiffies + msecs_to_jiffies(10);
> +	do {
> +		udelay(1);
> +
> +		regmap_read(rk_phy->reg_base,
> +			rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
> +			&dllrdy);
> +		dllrdy = (dllrdy >> PHYCTRL_DLLRDY_SHIFT) & PHYCTRL_DLLRDY_MASK;
> +		if (dllrdy == PHYCTRL_DLLRDY_DONE)
> +			break;
> +	} while (!time_after(jiffies, timeout));
> +
>  	if (dllrdy != PHYCTRL_DLLRDY_DONE) {
>  		pr_err("rockchip_emmc_phy_power: dllrdy timeout.\n");
>  		return -ETIMEDOUT;
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 01/11] phy: rockchip-emmc: Increase lock time allowance

[Doug Anderson]

Shawn,

On Mon, Jun 13, 2016 at 12:58 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> On 2016/6/8 6:44, Douglas Anderson wrote:
>>
>> Previous PHY code waited a fixed amount of time for the DLL to lock at
>> power on time.  Unfortunately, the time for the DLL to lock is actually
>> a bit more dynamic and can be longer if the card clock is slower.
>>
>> Instead of waiting a fixed 30 us, let's now dynamically wait until the
>> lock bit gets set.  We'll wait up to 10 ms which should be OK even if
>> the card clock is at the super slow 100 kHz.
>>
>
> mmc stack limit the min freq to 200k when initializing the card.

Are you certain?  In "drivers/mmc/core/core.c" I see:

static const unsigned freqs[] = { 400000, 300000, 200000, 100000 };

In ID mode if 400kHz, 300kHz, 200kHz all fail then it will try 100kHz.


> So 5ms is enough, but it's ok to set the max timeout to 10ms as we
> can break out if locked.

Right, it's OK to error on the long side since it is really a pretty
serious error if the DLL doesn't lock and delaying tens of
milliseconds in this case is not a huge deal.


>> On its own, this change makes the PHY power on code a little more
>> robust.  Before this change the PHY was relying on the eMMC code to make
>> sure the PHY was only powered on when the card clock was set to at least
>> 50 MHz before, though this reliance wasn't documented anywhere.
>>
>> This change will be even more useful in future changes where we actually
>> need to be able to wait for a DLL lock at slower clock speeds.
>>
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> ---
>>  drivers/phy/phy-rockchip-emmc.c | 27 +++++++++++++++++++--------
>>  1 file changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/phy/phy-rockchip-emmc.c
>> b/drivers/phy/phy-rockchip-emmc.c
>> index a69f53630e67..8336053aea5c 100644
>> --- a/drivers/phy/phy-rockchip-emmc.c
>> +++ b/drivers/phy/phy-rockchip-emmc.c
>> @@ -85,6 +85,7 @@ static int rockchip_emmc_phy_power(struct
>> rockchip_emmc_phy *rk_phy,
>>  {
>>         unsigned int caldone;
>>         unsigned int dllrdy;
>> +       unsigned long timeout;
>>
>>         /*
>>          * Keep phyctrl_pdb and phyctrl_endll low to allow
>> @@ -137,15 +138,25 @@ static int rockchip_emmc_phy_power(struct
>> rockchip_emmc_phy *rk_phy,
>>                                    PHYCTRL_ENDLL_MASK,
>>                                    PHYCTRL_ENDLL_SHIFT));
>>         /*
>> -        * After enable analog DLL circuits, we need an extra 10.2us
>> -        * for dll to be ready for work. But according to testing, we
>> -        * find some chips need more than 25us.
>> +        * After enabling analog DLL circuits docs say that we need 10.2
>> us if
>> +        * our source clock is at 50 MHz and that lock time scales
>> linearly
>> +        * with clock speed.  If we are powering on the PHY and the card
>> clock
>> +        * is super slow (like 100 kHZ) this could take as long as 5.1 ms.
>
>
> 5.1ms is by calculation or test?

By calculation.

>>> 10.2 us * (50000000 Hz / 100000 Hz)
5100.0 us, or 5.1 ms

I'll add clarification to the comment.


-Doug

[PATCH 02/11] mmc: sdhci-of-arasan: Always power the PHY off/on when clock changes

[Douglas Anderson]

In commit 802ac39a5566 ("mmc: sdhci-of-arasan: fix set_clock when a phy
is supported") we added code to power the PHY off and on whenever the
clock was changed but we avoided doing the power cycle code when the
clock was low speed.  Let's now do it always.

Although there may be other reasons for power cycling the PHY when the
clock changes, one of the main reasons is that we need to give the DLL a
chance to re-lock with the new clock.

One of the things that the DLL is for is tuning the Receive Clock in
HS200 mode and STRB in HS400 mode.  Thus it is clear that we should make
sure we power cycle the PHY (and wait for the DLL to lock) when we know
we'll be in one of these two speed modes.  That's what the original code
did, though it used the clock rate rather than the speed mode.  However,
even in speed modes other than HS200,/HS400 the DLL is used for
something since it can be clearly observed that the PHY doesn't function
properly if you leave the DLL off.

Although it appears less important to power cycle the PHY and wait for
the DLL to lock when not in HS200/HS400 modes (no bugs were reported),
it still seems wise to let the locking always happen nevertheless.

Note: as part of this, we make sure that we never try to turn the PHY on
when the clock is off (when the clock rate is 0).  The PHY cannot work
when the clock is off since its DLL can't lock.

This change requires ("phy: rockchip-emmc: Increase lock time
allowance") and will cause problems if picked without that change.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/mmc/host/sdhci-of-arasan.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index 533e2bcb10bc..3ff1711077c2 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -35,11 +35,13 @@
 /**
  * struct sdhci_arasan_data
  * @clk_ahb:	Pointer to the AHB clock
- * @phy: Pointer to the generic phy
+ * @phy:	Pointer to the generic phy
+ * @phy_on:	True if the PHY is turned on.
  */
 struct sdhci_arasan_data {
 	struct clk	*clk_ahb;
 	struct phy	*phy;
+	bool		phy_on;
 };
 
 static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host)
@@ -61,12 +63,10 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
-	bool ctrl_phy = false;
 
-	if (clock > MMC_HIGH_52_MAX_DTR && (!IS_ERR(sdhci_arasan->phy)))
-		ctrl_phy = true;
+	if (sdhci_arasan->phy_on && !IS_ERR(sdhci_arasan->phy)) {
+		sdhci_arasan->phy_on = false;
 
-	if (ctrl_phy) {
 		spin_unlock_irq(&host->lock);
 		phy_power_off(sdhci_arasan->phy);
 		spin_lock_irq(&host->lock);
@@ -74,7 +74,9 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
 
 	sdhci_set_clock(host, clock);
 
-	if (ctrl_phy) {
+	if (host->mmc->actual_clock && !IS_ERR(sdhci_arasan->phy)) {
+		sdhci_arasan->phy_on = true;
+
 		spin_unlock_irq(&host->lock);
 		phy_power_on(sdhci_arasan->phy);
 		spin_lock_irq(&host->lock);
@@ -257,12 +259,6 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 			goto clk_disable_all;
 		}
 
-		ret = phy_power_on(sdhci_arasan->phy);
-		if (ret < 0) {
-			dev_err(&pdev->dev, "phy_power_on err.\n");
-			goto err_phy_power;
-		}
-
 		host->mmc_host_ops.hs400_enhanced_strobe =
 					sdhci_arasan_hs400_enhanced_strobe;
 	}
@@ -275,9 +271,6 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 
 err_add_host:
 	if (!IS_ERR(sdhci_arasan->phy))
-		phy_power_off(sdhci_arasan->phy);
-err_phy_power:
-	if (!IS_ERR(sdhci_arasan->phy))
 		phy_exit(sdhci_arasan->phy);
 clk_disable_all:
 	clk_disable_unprepare(clk_xin);
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 02/11] mmc: sdhci-of-arasan: Always power the PHY off/on when clock changes

[Shawn Lin]

在 2016/6/8 6:44, Douglas Anderson 写道:
> In commit 802ac39a5566 ("mmc: sdhci-of-arasan: fix set_clock when a phy
> is supported") we added code to power the PHY off and on whenever the
> clock was changed but we avoided doing the power cycle code when the
> clock was low speed.  Let's now do it always.
>
> Although there may be other reasons for power cycling the PHY when the
> clock changes, one of the main reasons is that we need to give the DLL a
> chance to re-lock with the new clock.
>
> One of the things that the DLL is for is tuning the Receive Clock in
> HS200 mode and STRB in HS400 mode.  Thus it is clear that we should make
> sure we power cycle the PHY (and wait for the DLL to lock) when we know
> we'll be in one of these two speed modes.  That's what the original code
> did, though it used the clock rate rather than the speed mode.  However,
> even in speed modes other than HS200,/HS400 the DLL is used for
> something since it can be clearly observed that the PHY doesn't function
> properly if you leave the DLL off.
>
> Although it appears less important to power cycle the PHY and wait for
> the DLL to lock when not in HS200/HS400 modes (no bugs were reported),
> it still seems wise to let the locking always happen nevertheless.
>

 From the design doc, there is no need to off/on phy when not in
HS200/400, but maybe someone will limit the clk freq by assigning
max-frequency in DT when in HS200/400, which will make things worse.

So your patch looks sane.

> Note: as part of this, we make sure that we never try to turn the PHY on
> when the clock is off (when the clock rate is 0).  The PHY cannot work
> when the clock is off since its DLL can't lock.
>
> This change requires ("phy: rockchip-emmc: Increase lock time
> allowance") and will cause problems if picked without that change.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>  drivers/mmc/host/sdhci-of-arasan.c | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 533e2bcb10bc..3ff1711077c2 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -35,11 +35,13 @@
>  /**
>   * struct sdhci_arasan_data
>   * @clk_ahb:	Pointer to the AHB clock
> - * @phy: Pointer to the generic phy
> + * @phy:	Pointer to the generic phy
> + * @phy_on:	True if the PHY is turned on.
>   */
>  struct sdhci_arasan_data {
>  	struct clk	*clk_ahb;
>  	struct phy	*phy;
> +	bool		phy_on;
>  };
>
>  static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host *host)
> @@ -61,12 +63,10 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
> -	bool ctrl_phy = false;
>
> -	if (clock > MMC_HIGH_52_MAX_DTR && (!IS_ERR(sdhci_arasan->phy)))
> -		ctrl_phy = true;
> +	if (sdhci_arasan->phy_on && !IS_ERR(sdhci_arasan->phy)) {
> +		sdhci_arasan->phy_on = false;
>
> -	if (ctrl_phy) {
>  		spin_unlock_irq(&host->lock);
>  		phy_power_off(sdhci_arasan->phy);
>  		spin_lock_irq(&host->lock);
> @@ -74,7 +74,9 @@ static void sdhci_arasan_set_clock(struct sdhci_host *host, unsigned int clock)
>
>  	sdhci_set_clock(host, clock);
>
> -	if (ctrl_phy) {
> +	if (host->mmc->actual_clock && !IS_ERR(sdhci_arasan->phy)) {
> +		sdhci_arasan->phy_on = true;
> +
>  		spin_unlock_irq(&host->lock);
>  		phy_power_on(sdhci_arasan->phy);
>  		spin_lock_irq(&host->lock);
> @@ -257,12 +259,6 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>  			goto clk_disable_all;
>  		}
>
> -		ret = phy_power_on(sdhci_arasan->phy);
> -		if (ret < 0) {
> -			dev_err(&pdev->dev, "phy_power_on err.\n");
> -			goto err_phy_power;
> -		}
> -

Because there is too much dependency between phy and controller, so
previous solution is to setup clk by firmware. The same case for
suspend and resume.

Look really good to do it more legit.

>  		host->mmc_host_ops.hs400_enhanced_strobe =
>  					sdhci_arasan_hs400_enhanced_strobe;
>  	}
> @@ -275,9 +271,6 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>
>  err_add_host:
>  	if (!IS_ERR(sdhci_arasan->phy))
> -		phy_power_off(sdhci_arasan->phy);
> -err_phy_power:
> -	if (!IS_ERR(sdhci_arasan->phy))
>  		phy_exit(sdhci_arasan->phy);
>  clk_disable_all:
>  	clk_disable_unprepare(clk_xin);
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 02/11] mmc: sdhci-of-arasan: Always power the PHY off/on when clock changes

[Doug Anderson]

Hi,

On Mon, Jun 13, 2016 at 1:08 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> 在 2016/6/8 6:44, Douglas Anderson 写道:
>>
>> In commit 802ac39a5566 ("mmc: sdhci-of-arasan: fix set_clock when a phy
>> is supported") we added code to power the PHY off and on whenever the
>> clock was changed but we avoided doing the power cycle code when the
>> clock was low speed.  Let's now do it always.
>>
>> Although there may be other reasons for power cycling the PHY when the
>> clock changes, one of the main reasons is that we need to give the DLL a
>> chance to re-lock with the new clock.
>>
>> One of the things that the DLL is for is tuning the Receive Clock in
>> HS200 mode and STRB in HS400 mode.  Thus it is clear that we should make
>> sure we power cycle the PHY (and wait for the DLL to lock) when we know
>> we'll be in one of these two speed modes.  That's what the original code
>> did, though it used the clock rate rather than the speed mode.  However,
>> even in speed modes other than HS200,/HS400 the DLL is used for
>> something since it can be clearly observed that the PHY doesn't function
>> properly if you leave the DLL off.
>>
>> Although it appears less important to power cycle the PHY and wait for
>> the DLL to lock when not in HS200/HS400 modes (no bugs were reported),
>> it still seems wise to let the locking always happen nevertheless.
>>
>
> From the design doc, there is no need to off/on phy when not in
> HS200/400, but maybe someone will limit the clk freq by assigning
> max-frequency in DT when in HS200/400, which will make things worse.
>
> So your patch looks sane.
>
>
>> Note: as part of this, we make sure that we never try to turn the PHY on
>> when the clock is off (when the clock rate is 0).  The PHY cannot work
>> when the clock is off since its DLL can't lock.
>>
>> This change requires ("phy: rockchip-emmc: Increase lock time
>> allowance") and will cause problems if picked without that change.
>>
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> ---
>>  drivers/mmc/host/sdhci-of-arasan.c | 23 ++++++++---------------
>>  1 file changed, 8 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
>> b/drivers/mmc/host/sdhci-of-arasan.c
>> index 533e2bcb10bc..3ff1711077c2 100644
>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>> @@ -35,11 +35,13 @@
>>  /**
>>   * struct sdhci_arasan_data
>>   * @clk_ahb:   Pointer to the AHB clock
>> - * @phy: Pointer to the generic phy
>> + * @phy:       Pointer to the generic phy
>> + * @phy_on:    True if the PHY is turned on.
>>   */
>>  struct sdhci_arasan_data {
>>         struct clk      *clk_ahb;
>>         struct phy      *phy;
>> +       bool            phy_on;
>>  };
>>
>>  static unsigned int sdhci_arasan_get_timeout_clock(struct sdhci_host
>> *host)
>> @@ -61,12 +63,10 @@ static void sdhci_arasan_set_clock(struct sdhci_host
>> *host, unsigned int clock)
>>  {
>>         struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>         struct sdhci_arasan_data *sdhci_arasan =
>> sdhci_pltfm_priv(pltfm_host);
>> -       bool ctrl_phy = false;
>>
>> -       if (clock > MMC_HIGH_52_MAX_DTR && (!IS_ERR(sdhci_arasan->phy)))
>> -               ctrl_phy = true;
>> +       if (sdhci_arasan->phy_on && !IS_ERR(sdhci_arasan->phy)) {
>> +               sdhci_arasan->phy_on = false;
>>
>> -       if (ctrl_phy) {
>>                 spin_unlock_irq(&host->lock);
>>                 phy_power_off(sdhci_arasan->phy);
>>                 spin_lock_irq(&host->lock);
>> @@ -74,7 +74,9 @@ static void sdhci_arasan_set_clock(struct sdhci_host
>> *host, unsigned int clock)
>>
>>         sdhci_set_clock(host, clock);
>>
>> -       if (ctrl_phy) {
>> +       if (host->mmc->actual_clock && !IS_ERR(sdhci_arasan->phy)) {
>> +               sdhci_arasan->phy_on = true;
>> +
>>                 spin_unlock_irq(&host->lock);
>>                 phy_power_on(sdhci_arasan->phy);
>>                 spin_lock_irq(&host->lock);
>> @@ -257,12 +259,6 @@ static int sdhci_arasan_probe(struct platform_device
>> *pdev)
>>                         goto clk_disable_all;
>>                 }
>>
>> -               ret = phy_power_on(sdhci_arasan->phy);
>> -               if (ret < 0) {
>> -                       dev_err(&pdev->dev, "phy_power_on err.\n");
>> -                       goto err_phy_power;
>> -               }
>> -
>
>
> Because there is too much dependency between phy and controller, so
> previous solution is to setup clk by firmware. The same case for
> suspend and resume.
>
> Look really good to do it more legit.

Thanks!  I don't see a Reviewed-by tag so I'm not adding it.  If you'd
like your tag added, please let me know and I will add it if I send
out another version.


-Doug

[PATCH 03/11] Documentation: mmc: sdhci-of-arasan: Add soc-ctl-syscon for corecfg regs

[Douglas Anderson]

As can be seen in Arasan's datasheet [1] there are several "corecfg"
settings in their SDHCI IP Block that are supposed to be controlled by
software.  Although the datasheet referenced is a bit vague about how to
access corecfg, in Figure 5 you can see that for Arasan's PHY (a
separate component than their SDHCI component) they describe the
"phyctrl" registers as being "FROM SOC CTL REG", implying that it's up
to the licensee of the Arasan IP block to implement these registers.  It
seems sane to assume that the "corecfg" registers in their SDHCI IP
block works in a similar way for all licensees of the IP Block.

Device tree has a model that allows a device to get a reference to
random registers located elsewhere in the SoC: sysctl.  Let's leverage
this model and allow adding a sysctl reference to access the control
registers for the Arasan SDHCI PHYs.

Having a reference to the control registers doesn't do much for us on
its own since the Arasan spec doesn't specify how these corecfg values
are laid out in memory.  In the SDHCI driver we'll need a map detailing
where each corecfg can be found in each implementation.  This map can be
found using the primary compatible string of the SDHCI device.  In that
spirit, document that existing rk3399 device trees already have a
specific compatible string, though up to now they've always been relying
on the driver supporting the generic.

Note that since existing devices seem to work fairly well as-is, we'll
list the syscon reference as "optional", but it's likely that we'll run
into much fewer problems if we can actually set the proper values in the
syscon, so it is strongly suggested that any SoCs where we have a map to
set the corecfg also include a reference to the syscon.

[1]: https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 .../devicetree/bindings/mmc/arasan,sdhci.txt       | 27 ++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
index 31b35c3a5e47..b67e623ca1ff 100644
--- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
+++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
@@ -9,8 +9,12 @@ Device Tree Bindings for the Arasan SDHCI Controller
   [4] Documentation/devicetree/bindings/phy/phy-bindings.txt
 
 Required Properties:
-  - compatible: Compatibility string. Must be 'arasan,sdhci-8.9a' or
-                'arasan,sdhci-4.9a' or 'arasan,sdhci-5.1'
+  - compatible: Compatibility string.  One of:
+    - "arasan,sdhci-8.9a"
+    - "arasan,sdhci-4.9a"
+    - "arasan,sdhci-5.1"
+    - "rockchip,rk3399-sdhci-5.1", "arasan,sdhci-5.1": The PHY in rk3399.
+      For this device it is strongly suggested to include arasan,soc-ctl-syscon.
   - reg: From mmc bindings: Register location and length.
   - clocks: From clock bindings: Handles to clock inputs.
   - clock-names: From clock bindings: Tuple including "clk_xin" and "clk_ahb"
@@ -22,6 +26,11 @@ Required Properties for "arasan,sdhci-5.1":
   - phys: From PHY bindings: Phandle for the Generic PHY for arasan.
   - phy-names:  MUST be "phy_arasan".
 
+Optional Properties:
+  - arasan,soc-ctl-syscon: A phandle to a syscon device (see ../mfd/syscon.txt)
+    used to access core corecfg registers.  Offsets of registers in this
+    syscon are determined based on the main compatible string for the device.
+
 Example:
 	sdhci@e0100000 {
 		compatible = "arasan,sdhci-8.9a";
@@ -42,3 +51,17 @@ Example:
 		phys = <&emmc_phy>;
 		phy-names = "phy_arasan";
 	} ;
+
+	sdhci: sdhci@fe330000 {
+		compatible = "rockchip,rk3399-sdhci-5.1", "arasan,sdhci-5.1";
+		reg = <0x0 0xfe330000 0x0 0x10000>;
+		interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru SCLK_EMMC>, <&cru ACLK_EMMC>;
+		clock-names = "clk_xin", "clk_ahb";
+		arasan,soc-ctl-syscon = <&grf>;
+		assigned-clocks = <&cru SCLK_EMMC>;
+		assigned-clock-rates = <200000000>;
+		phys = <&emmc_phy>;
+		phy-names = "phy_arasan";
+		status = "disabled";
+	};
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 03/11] Documentation: mmc: sdhci-of-arasan: Add soc-ctl-syscon for corecfg regs

[Rob Herring]

On Tue, Jun 07, 2016 at 03:44:36PM -0700, Douglas Anderson wrote:
> As can be seen in Arasan's datasheet [1] there are several "corecfg"
> settings in their SDHCI IP Block that are supposed to be controlled by
> software.  Although the datasheet referenced is a bit vague about how to
> access corecfg, in Figure 5 you can see that for Arasan's PHY (a
> separate component than their SDHCI component) they describe the
> "phyctrl" registers as being "FROM SOC CTL REG", implying that it's up
> to the licensee of the Arasan IP block to implement these registers.  It
> seems sane to assume that the "corecfg" registers in their SDHCI IP
> block works in a similar way for all licensees of the IP Block.
> 
> Device tree has a model that allows a device to get a reference to
> random registers located elsewhere in the SoC: sysctl.  Let's leverage
> this model and allow adding a sysctl reference to access the control
> registers for the Arasan SDHCI PHYs.
> 
> Having a reference to the control registers doesn't do much for us on
> its own since the Arasan spec doesn't specify how these corecfg values
> are laid out in memory.  In the SDHCI driver we'll need a map detailing
> where each corecfg can be found in each implementation.  This map can be
> found using the primary compatible string of the SDHCI device.  In that
> spirit, document that existing rk3399 device trees already have a
> specific compatible string, though up to now they've always been relying
> on the driver supporting the generic.
> 
> Note that since existing devices seem to work fairly well as-is, we'll
> list the syscon reference as "optional", but it's likely that we'll run
> into much fewer problems if we can actually set the proper values in the
> syscon, so it is strongly suggested that any SoCs where we have a map to
> set the corecfg also include a reference to the syscon.
> 
> [1]: https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>  .../devicetree/bindings/mmc/arasan,sdhci.txt       | 27 ++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 03/11] Documentation: mmc: sdhci-of-arasan: Add soc-ctl-syscon for corecfg regs

[Shawn Lin]

On 2016/6/8 6:44, Douglas Anderson wrote:
> As can be seen in Arasan's datasheet [1] there are several "corecfg"
> settings in their SDHCI IP Block that are supposed to be controlled by
> software.  Although the datasheet referenced is a bit vague about how to
> access corecfg, in Figure 5 you can see that for Arasan's PHY (a
> separate component than their SDHCI component) they describe the
> "phyctrl" registers as being "FROM SOC CTL REG", implying that it's up
> to the licensee of the Arasan IP block to implement these registers.  It
> seems sane to assume that the "corecfg" registers in their SDHCI IP
> block works in a similar way for all licensees of the IP Block.
>
> Device tree has a model that allows a device to get a reference to
> random registers located elsewhere in the SoC: sysctl.  Let's leverage
> this model and allow adding a sysctl reference to access the control
> registers for the Arasan SDHCI PHYs.
>
> Having a reference to the control registers doesn't do much for us on
> its own since the Arasan spec doesn't specify how these corecfg values
> are laid out in memory.  In the SDHCI driver we'll need a map detailing
> where each corecfg can be found in each implementation.  This map can be
> found using the primary compatible string of the SDHCI device.  In that
> spirit, document that existing rk3399 device trees already have a
> specific compatible string, though up to now they've always been relying
> on the driver supporting the generic.
>
> Note that since existing devices seem to work fairly well as-is, we'll
> list the syscon reference as "optional", but it's likely that we'll run
> into much fewer problems if we can actually set the proper values in the
> syscon, so it is strongly suggested that any SoCs where we have a map to
> set the corecfg also include a reference to the syscon.

yes, the interaction of phy and controller should be more explicitly
now. Why not make it mandatory for arasan,sdhci-5.1.

>
> [1]: https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>  .../devicetree/bindings/mmc/arasan,sdhci.txt       | 27 ++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> index 31b35c3a5e47..b67e623ca1ff 100644
> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> @@ -9,8 +9,12 @@ Device Tree Bindings for the Arasan SDHCI Controller
>    [4] Documentation/devicetree/bindings/phy/phy-bindings.txt
>
>  Required Properties:
> -  - compatible: Compatibility string. Must be 'arasan,sdhci-8.9a' or
> -                'arasan,sdhci-4.9a' or 'arasan,sdhci-5.1'
> +  - compatible: Compatibility string.  One of:
> +    - "arasan,sdhci-8.9a"
> +    - "arasan,sdhci-4.9a"
> +    - "arasan,sdhci-5.1"
> +    - "rockchip,rk3399-sdhci-5.1", "arasan,sdhci-5.1": The PHY in rk3399.

The PHY in rk3399?

> +      For this device it is strongly suggested to include arasan,soc-ctl-syscon.
>    - reg: From mmc bindings: Register location and length.
>    - clocks: From clock bindings: Handles to clock inputs.
>    - clock-names: From clock bindings: Tuple including "clk_xin" and "clk_ahb"
> @@ -22,6 +26,11 @@ Required Properties for "arasan,sdhci-5.1":
>    - phys: From PHY bindings: Phandle for the Generic PHY for arasan.
>    - phy-names:  MUST be "phy_arasan".
>
> +Optional Properties:
> +  - arasan,soc-ctl-syscon: A phandle to a syscon device (see ../mfd/syscon.txt)
> +    used to access core corecfg registers.  Offsets of registers in this
> +    syscon are determined based on the main compatible string for the device.
> +
>  Example:
>  	sdhci@e0100000 {
>  		compatible = "arasan,sdhci-8.9a";
> @@ -42,3 +51,17 @@ Example:
>  		phys = <&emmc_phy>;
>  		phy-names = "phy_arasan";
>  	} ;
> +
> +	sdhci: sdhci@fe330000 {
> +		compatible = "rockchip,rk3399-sdhci-5.1", "arasan,sdhci-5.1";
> +		reg = <0x0 0xfe330000 0x0 0x10000>;
> +		interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru SCLK_EMMC>, <&cru ACLK_EMMC>;
> +		clock-names = "clk_xin", "clk_ahb";
> +		arasan,soc-ctl-syscon = <&grf>;
> +		assigned-clocks = <&cru SCLK_EMMC>;
> +		assigned-clock-rates = <200000000>;
> +		phys = <&emmc_phy>;
> +		phy-names = "phy_arasan";
> +		status = "disabled";
> +	};
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 03/11] Documentation: mmc: sdhci-of-arasan: Add soc-ctl-syscon for corecfg regs

[Heiko Stübner]

Am Montag, 13. Juni 2016, 16:18:24 schrieb Shawn Lin:
> On 2016/6/8 6:44, Douglas Anderson wrote:
> > As can be seen in Arasan's datasheet [1] there are several "corecfg"
> > settings in their SDHCI IP Block that are supposed to be controlled by
> > software.  Although the datasheet referenced is a bit vague about how to
> > access corecfg, in Figure 5 you can see that for Arasan's PHY (a
> > separate component than their SDHCI component) they describe the
> > "phyctrl" registers as being "FROM SOC CTL REG", implying that it's up
> > to the licensee of the Arasan IP block to implement these registers.  It
> > seems sane to assume that the "corecfg" registers in their SDHCI IP
> > block works in a similar way for all licensees of the IP Block.
> > 
> > Device tree has a model that allows a device to get a reference to
> > random registers located elsewhere in the SoC: sysctl.  Let's leverage
> > this model and allow adding a sysctl reference to access the control
> > registers for the Arasan SDHCI PHYs.
> > 
> > Having a reference to the control registers doesn't do much for us on
> > its own since the Arasan spec doesn't specify how these corecfg values
> > are laid out in memory.  In the SDHCI driver we'll need a map detailing
> > where each corecfg can be found in each implementation.  This map can be
> > found using the primary compatible string of the SDHCI device.  In that
> > spirit, document that existing rk3399 device trees already have a
> > specific compatible string, though up to now they've always been relying
> > on the driver supporting the generic.
> > 
> > Note that since existing devices seem to work fairly well as-is, we'll
> > list the syscon reference as "optional", but it's likely that we'll run
> > into much fewer problems if we can actually set the proper values in the
> > syscon, so it is strongly suggested that any SoCs where we have a map to
> > set the corecfg also include a reference to the syscon.
> 
> yes, the interaction of phy and controller should be more explicitly
> now. Why not make it mandatory for arasan,sdhci-5.1.

The omnipresent backwards-compatiblity :-) .

The binding did not require any of this in the past, so the driver should 
always also work with devicetrees created according to this old binding.

And obviously it was working without that before as well, as Doug stated.


Heiko

Re: [PATCH 03/11] Documentation: mmc: sdhci-of-arasan: Add soc-ctl-syscon for corecfg regs

[Doug Anderson]

Hi,

On Mon, Jun 13, 2016 at 1:18 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> On 2016/6/8 6:44, Douglas Anderson wrote:
>>
>> As can be seen in Arasan's datasheet [1] there are several "corecfg"
>> settings in their SDHCI IP Block that are supposed to be controlled by
>> software.  Although the datasheet referenced is a bit vague about how to
>> access corecfg, in Figure 5 you can see that for Arasan's PHY (a
>> separate component than their SDHCI component) they describe the
>> "phyctrl" registers as being "FROM SOC CTL REG", implying that it's up
>> to the licensee of the Arasan IP block to implement these registers.  It
>> seems sane to assume that the "corecfg" registers in their SDHCI IP
>> block works in a similar way for all licensees of the IP Block.
>>
>> Device tree has a model that allows a device to get a reference to
>> random registers located elsewhere in the SoC: sysctl.  Let's leverage
>> this model and allow adding a sysctl reference to access the control
>> registers for the Arasan SDHCI PHYs.
>>
>> Having a reference to the control registers doesn't do much for us on
>> its own since the Arasan spec doesn't specify how these corecfg values
>> are laid out in memory.  In the SDHCI driver we'll need a map detailing
>> where each corecfg can be found in each implementation.  This map can be
>> found using the primary compatible string of the SDHCI device.  In that
>> spirit, document that existing rk3399 device trees already have a
>> specific compatible string, though up to now they've always been relying
>> on the driver supporting the generic.
>>
>> Note that since existing devices seem to work fairly well as-is, we'll
>> list the syscon reference as "optional", but it's likely that we'll run
>> into much fewer problems if we can actually set the proper values in the
>> syscon, so it is strongly suggested that any SoCs where we have a map to
>> set the corecfg also include a reference to the syscon.
>
>
> yes, the interaction of phy and controller should be more explicitly
> now. Why not make it mandatory for arasan,sdhci-5.1.
>
>>
>> [1]:
>> https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf
>>
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> ---
>>  .../devicetree/bindings/mmc/arasan,sdhci.txt       | 27
>> ++++++++++++++++++++--
>>  1 file changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> index 31b35c3a5e47..b67e623ca1ff 100644
>> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
>> @@ -9,8 +9,12 @@ Device Tree Bindings for the Arasan SDHCI Controller
>>    [4] Documentation/devicetree/bindings/phy/phy-bindings.txt
>>
>>  Required Properties:
>> -  - compatible: Compatibility string. Must be 'arasan,sdhci-8.9a' or
>> -                'arasan,sdhci-4.9a' or 'arasan,sdhci-5.1'
>> +  - compatible: Compatibility string.  One of:
>> +    - "arasan,sdhci-8.9a"
>> +    - "arasan,sdhci-4.9a"
>> +    - "arasan,sdhci-5.1"
>> +    - "rockchip,rk3399-sdhci-5.1", "arasan,sdhci-5.1": The PHY in rk3399.
>
>
> The PHY in rk3399?

I presume that you found the above confusing?  I changed it to:

  - compatible: Compatibility string.  One of:
    - "arasan,sdhci-8.9a": generic Arasan SDHCI 8.9a PHY
    - "arasan,sdhci-4.9a": generic Arasan SDHCI 4.9a PHY
    - "arasan,sdhci-5.1": generic Arasan SDHCI 5.1 PHY
    - "rockchip,rk3399-sdhci-5.1", "arasan,sdhci-5.1": rk3399 eMMC PHY
      For this device it is strongly suggested to include arasan,soc-ctl-syscon.

I kept Rob's Ack since those changes didn't seem too major.  I will
remove it if requested.

-Doug

[PATCH 04/11] mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399

[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>
 #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",
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 04/11] mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399

[Shawn Lin]

在 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",
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 04/11] mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399

[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?

...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

Re: [PATCH 04/11] mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399

[Shawn Lin]

在 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
>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 04/11] mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399

[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?).

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

Re: [PATCH 04/11] mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399

[Shawn Lin]

在 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
>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 04/11] mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399

[Doug Anderson]

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

Re: [PATCH 04/11] mmc: sdhci-of-arasan: Properly set corecfg_baseclkfreq on rk3399

[Shawn Lin]

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


-- 
Best Regards
Shawn Lin

[PATCH 05/11] arm64: dts: rockchip: Add soc-ctl-syscon to sdhci for rk3399

[Douglas Anderson]

On rk3399 we'd like to be able to properly set corecfg registers in the
Arasan SDHCI component.  Specify the syscon to enable that.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index a4383f359264..1b57e92e0093 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -220,6 +220,7 @@
 		compatible = "rockchip,rk3399-sdhci-5.1", "arasan,sdhci-5.1";
 		reg = <0x0 0xfe330000 0x0 0x10000>;
 		interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
+		arasan,soc-ctl-syscon = <&grf>;
 		assigned-clocks = <&cru SCLK_EMMC>;
 		assigned-clock-rates = <200000000>;
 		clocks = <&cru SCLK_EMMC>, <&cru ACLK_EMMC>;
-- 
2.8.0.rc3.226.g39d4020

[PATCH 06/11] Documentation: mmc: sdhci-of-arasan: Add ability to export card clock

[Douglas Anderson]

Some SD/eMMC PHYs (like the PHY from Arasan that is designed to work
with arasan,sdhci-5.1) need to know the card clock in order to function
properly.  Let's expose this clock using a standard device tree
mechanism so that the PHY can get access to and query the card clock.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
index b67e623ca1ff..074d03e630ec 100644
--- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
+++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
@@ -30,6 +30,12 @@ Optional Properties:
   - arasan,soc-ctl-syscon: A phandle to a syscon device (see ../mfd/syscon.txt)
     used to access core corecfg registers.  Offsets of registers in this
     syscon are determined based on the main compatible string for the device.
+  - clock-output-names: If specified, this will be the name of the card clock
+    which will be exposed by this device.  Required if #clock-cells is
+    specified.
+  - #clock-cells: If specified this should be the value <0>.  With this property
+    in place we will export a clock representing the Card Clock.  This clock
+    is expected to be consumed by our PHY.  You must also specify
 
 Example:
 	sdhci@e0100000 {
@@ -61,7 +67,9 @@ Example:
 		arasan,soc-ctl-syscon = <&grf>;
 		assigned-clocks = <&cru SCLK_EMMC>;
 		assigned-clock-rates = <200000000>;
+		clock-output-names = "emmc_cardclock";
 		phys = <&emmc_phy>;
 		phy-names = "phy_arasan";
+		#clock-cells = <0>;
 		status = "disabled";
 	};
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 06/11] Documentation: mmc: sdhci-of-arasan: Add ability to export card clock

[Rob Herring]

On Tue, Jun 07, 2016 at 03:44:39PM -0700, Douglas Anderson wrote:
> Some SD/eMMC PHYs (like the PHY from Arasan that is designed to work
> with arasan,sdhci-5.1) need to know the card clock in order to function
> properly.  Let's expose this clock using a standard device tree
> mechanism so that the PHY can get access to and query the card clock.

Need to know the clock freq or need the clock? The former doesn't need 
to be in DT.

> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> index b67e623ca1ff..074d03e630ec 100644
> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> @@ -30,6 +30,12 @@ Optional Properties:
>    - arasan,soc-ctl-syscon: A phandle to a syscon device (see ../mfd/syscon.txt)
>      used to access core corecfg registers.  Offsets of registers in this
>      syscon are determined based on the main compatible string for the device.
> +  - clock-output-names: If specified, this will be the name of the card clock
> +    which will be exposed by this device.  Required if #clock-cells is
> +    specified.
> +  - #clock-cells: If specified this should be the value <0>.  With this property
> +    in place we will export a clock representing the Card Clock.  This clock
> +    is expected to be consumed by our PHY.  You must also specify
>  
>  Example:
>  	sdhci@e0100000 {
> @@ -61,7 +67,9 @@ Example:
>  		arasan,soc-ctl-syscon = <&grf>;
>  		assigned-clocks = <&cru SCLK_EMMC>;
>  		assigned-clock-rates = <200000000>;
> +		clock-output-names = "emmc_cardclock";
>  		phys = <&emmc_phy>;
>  		phy-names = "phy_arasan";
> +		#clock-cells = <0>;
>  		status = "disabled";
>  	};
> -- 
> 2.8.0.rc3.226.g39d4020
> 

Re: [PATCH 06/11] Documentation: mmc: sdhci-of-arasan: Add ability to export card clock

[Doug Anderson]

Rob,

On Wed, Jun 8, 2016 at 1:19 PM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Jun 07, 2016 at 03:44:39PM -0700, Douglas Anderson wrote:
>> Some SD/eMMC PHYs (like the PHY from Arasan that is designed to work
>> with arasan,sdhci-5.1) need to know the card clock in order to function
>> properly.  Let's expose this clock using a standard device tree
>> mechanism so that the PHY can get access to and query the card clock.
>
> Need to know the clock freq or need the clock? The former doesn't need
> to be in DT.

The PHY needs to know the card clock frequency.  This clock clock
frequency is known nowhere else in the system except the SDHCI driver
since the SDHCI component takes its input clock and applies some
dividers to get the card clock.  In silicon, the SDHCI component and
the PHY are separate and there is a physical clock signal going from
the SDHCI component to the PHY component, so modeling this as a clock
in in the device tree seems sane, doesn't it?

-Doug

Re: [PATCH 06/11] Documentation: mmc: sdhci-of-arasan: Add ability to export card clock

[Rob Herring]

On Wed, Jun 08, 2016 at 01:52:20PM -0700, Doug Anderson wrote:
> Rob,
> 
> On Wed, Jun 8, 2016 at 1:19 PM, Rob Herring <robh@kernel.org> wrote:
> > On Tue, Jun 07, 2016 at 03:44:39PM -0700, Douglas Anderson wrote:
> >> Some SD/eMMC PHYs (like the PHY from Arasan that is designed to work
> >> with arasan,sdhci-5.1) need to know the card clock in order to function
> >> properly.  Let's expose this clock using a standard device tree
> >> mechanism so that the PHY can get access to and query the card clock.
> >
> > Need to know the clock freq or need the clock? The former doesn't need
> > to be in DT.
> 
> The PHY needs to know the card clock frequency.  This clock clock
> frequency is known nowhere else in the system except the SDHCI driver
> since the SDHCI component takes its input clock and applies some
> dividers to get the card clock.  In silicon, the SDHCI component and
> the PHY are separate and there is a physical clock signal going from
> the SDHCI component to the PHY component, so modeling this as a clock
> in in the device tree seems sane, doesn't it?

Yes, please just re-word it to make it clear there is a connection. With 
that,

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 06/11] Documentation: mmc: sdhci-of-arasan: Add ability to export card clock

[Doug Anderson]

Rob,

On Fri, Jun 10, 2016 at 6:10 AM, Rob Herring <robh@kernel.org> wrote:
> On Wed, Jun 08, 2016 at 01:52:20PM -0700, Doug Anderson wrote:
>> Rob,
>>
>> On Wed, Jun 8, 2016 at 1:19 PM, Rob Herring <robh@kernel.org> wrote:
>> > On Tue, Jun 07, 2016 at 03:44:39PM -0700, Douglas Anderson wrote:
>> >> Some SD/eMMC PHYs (like the PHY from Arasan that is designed to work
>> >> with arasan,sdhci-5.1) need to know the card clock in order to function
>> >> properly.  Let's expose this clock using a standard device tree
>> >> mechanism so that the PHY can get access to and query the card clock.
>> >
>> > Need to know the clock freq or need the clock? The former doesn't need
>> > to be in DT.
>>
>> The PHY needs to know the card clock frequency.  This clock clock
>> frequency is known nowhere else in the system except the SDHCI driver
>> since the SDHCI component takes its input clock and applies some
>> dividers to get the card clock.  In silicon, the SDHCI component and
>> the PHY are separate and there is a physical clock signal going from
>> the SDHCI component to the PHY component, so modeling this as a clock
>> in in the device tree seems sane, doesn't it?
>
> Yes, please just re-word it to make it clear there is a connection. With
> that,
>
> Acked-by: Rob Herring <robh@kernel.org>

OK, done.  Thanks for your review!

-Doug

[PATCH 07/11] mmc: sdhci-of-arasan: Add ability to export card clock

[Douglas Anderson]

Some SD/eMMC PHYs (like the PHY from Arasan that is designed to work
with arasan,sdhci-5.1) need to know the card clock in order to function
properly.  Let's add the ability to expose this clock.  Any PHY that
needs to know the clock rate can add a reference and query the clock
rate.

At the moment we register a CLK_GET_RATE_NOCACHE clock that simply
allows querying the clock.  This allows us to be less intrusive with
regards to the main SDHCI driver, which has complex logic for adjusting
the SD clock.  Right now we always fully power cycle the PHY when the
clock changes and that gives the PHY a good chance to query our clock.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/mmc/host/sdhci-of-arasan.c | 125 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 122 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index 859ea1b21215..25852e4fd1b6 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -19,6 +19,7 @@
  * your option) any later version.
  */
 
+#include <linux/clk-provider.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/phy/phy.h>
@@ -73,17 +74,24 @@ struct sdhci_arasan_soc_ctl_map {
 
 /**
  * struct sdhci_arasan_data
+ * @host:		Pointer to the main SDHCI host structure.
  * @clk_ahb:		Pointer to the AHB clock
  * @phy:		Pointer to the generic phy
  * @phy_on:		True if the PHY is turned on.
+ * @sdcardclk_hw:	Struct for the clock we might provide to a PHY.
+ * @sdcardclk:		Pointer to normal 'struct clock' for sdcardclk_hw.
  * @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 sdhci_host *host;
 	struct clk	*clk_ahb;
 	struct phy	*phy;
 	bool		phy_on;
 
+	struct clk_hw	sdcardclk_hw;
+	struct clk      *sdcardclk;
+
 	struct regmap	*soc_ctl_base;
 	const struct sdhci_arasan_soc_ctl_map *soc_ctl_map;
 };
@@ -307,6 +315,31 @@ static const struct of_device_id sdhci_arasan_of_match[] = {
 MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
 
 /**
+ * sdhci_arasan_sdcardclk_recalc_rate - Return the card clock rate
+ *
+ * Return the current actual rate of the SD card clock.  This can be used
+ * to communicate with out PHY.
+ *
+ * @hw:			Pointer to the hardware clock structure.
+ * @parent_rate		The parent rate (should be rate of clk_xin).
+ * Returns the card clock rate.
+ */
+static unsigned long sdhci_arasan_sdcardclk_recalc_rate(struct clk_hw *hw,
+						      unsigned long parent_rate)
+
+{
+	struct sdhci_arasan_data *sdhci_arasan =
+		container_of(hw, struct sdhci_arasan_data, sdcardclk_hw);
+	struct sdhci_host *host = sdhci_arasan->host;
+
+	return host->mmc->actual_clock;
+}
+
+static const struct clk_ops arasan_sdcardclk_ops = {
+	.recalc_rate = sdhci_arasan_sdcardclk_recalc_rate,
+};
+
+/**
  * sdhci_arasan_update_baseclkfreq - Set corecfg_baseclkfreq
  *
  * The corecfg_baseclkfreq is supposed to contain the MHz of clk_xin.  This
@@ -345,6 +378,83 @@ static void sdhci_arasan_update_baseclkfreq(struct sdhci_host *host)
 	sdhci_arasan_syscon_write(host, &soc_ctl_map->baseclkfreq, mhz);
 }
 
+/**
+ * sdhci_arasan_register_sdclk - Register the sdclk for a PHY to use
+ *
+ * Some PHY devices need to know what the actual card clock is.  In order for
+ * them to find out, we'll provide a clock through the common clock framework
+ * for them to query.
+ *
+ * Note: without seriously re-architecting SDHCI's clock code and testing on
+ * all platforms, there's no way to create a totally beautiful clock here
+ * with all clock ops implemented.  Instead, we'll just create a clock that can
+ * be queried and set the CLK_GET_RATE_NOCACHE attribute to tell common clock
+ * framework that we're doing things behind its back.  This should be sufficient
+ * to create nice clean device tree bindings and later (if needed) we can try
+ * re-architecting SDHCI if we see some benefit to it.
+ *
+ * @sdhci_arasan:	Our private data structure.
+ * @clk_xin:		Pointer to the functional clock
+ * @dev:		Pointer to our struct device.
+ * Returns 0 on success and error value on error
+ */
+static int sdhci_arasan_register_sdclk(struct sdhci_arasan_data *sdhci_arasan,
+				       struct clk *clk_xin,
+				       struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct clk_init_data sdcardclk_init;
+	const char *parent_clk_name;
+	int ret;
+
+	/* Providing a clock to the PHY is optional; no error if missing */
+	if (!of_find_property(np, "#clock-cells", NULL))
+		return 0;
+
+	ret = of_property_read_string_index(np, "clock-output-names", 0,
+					    &sdcardclk_init.name);
+	if (ret) {
+		dev_err(dev, "DT has #clock-cells but no clock-output-names\n");
+		return ret;
+	}
+
+	parent_clk_name = __clk_get_name(clk_xin);
+	sdcardclk_init.parent_names = &parent_clk_name;
+	sdcardclk_init.num_parents = 1;
+	sdcardclk_init.flags = CLK_GET_RATE_NOCACHE;
+	sdcardclk_init.ops = &arasan_sdcardclk_ops;
+
+	sdhci_arasan->sdcardclk_hw.init = &sdcardclk_init;
+	sdhci_arasan->sdcardclk =
+		devm_clk_register(dev, &sdhci_arasan->sdcardclk_hw);
+	sdhci_arasan->sdcardclk_hw.init = NULL;
+
+	ret = of_clk_add_provider(np, of_clk_src_simple_get,
+				  sdhci_arasan->sdcardclk);
+	if (ret)
+		dev_err(dev, "Failed to add clock provider\n");
+
+	return ret;
+}
+
+/**
+ * sdhci_arasan_unregister_sdclk - Undoes sdhci_arasan_register_sdclk()
+ *
+ * Should be called any time we're exiting and sdhci_arasan_register_sdclk()
+ * returned success.
+ *
+ * @dev:		Pointer to our struct device.
+ */
+static void sdhci_arasan_unregister_sdclk(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+
+	if (!of_find_property(np, "#clock-cells", NULL))
+		return;
+
+	of_clk_del_provider(dev->of_node);
+}
+
 static int sdhci_arasan_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -362,6 +472,7 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 
 	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;
@@ -411,10 +522,14 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 
 	sdhci_arasan_update_baseclkfreq(host);
 
+	ret = sdhci_arasan_register_sdclk(sdhci_arasan, clk_xin, &pdev->dev);
+	if (ret)
+		goto clk_disable_all;
+
 	ret = mmc_of_parse(host->mmc);
 	if (ret) {
 		dev_err(&pdev->dev, "parsing dt failed (%u)\n", ret);
-		goto clk_disable_all;
+		goto unreg_clk;
 	}
 
 	sdhci_arasan->phy = ERR_PTR(-ENODEV);
@@ -425,13 +540,13 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 		if (IS_ERR(sdhci_arasan->phy)) {
 			ret = PTR_ERR(sdhci_arasan->phy);
 			dev_err(&pdev->dev, "No phy for arasan,sdhci-5.1.\n");
-			goto clk_disable_all;
+			goto unreg_clk;
 		}
 
 		ret = phy_init(sdhci_arasan->phy);
 		if (ret < 0) {
 			dev_err(&pdev->dev, "phy_init err.\n");
-			goto clk_disable_all;
+			goto unreg_clk;
 		}
 
 		host->mmc_host_ops.hs400_enhanced_strobe =
@@ -447,6 +562,8 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 err_add_host:
 	if (!IS_ERR(sdhci_arasan->phy))
 		phy_exit(sdhci_arasan->phy);
+unreg_clk:
+	sdhci_arasan_unregister_sdclk(&pdev->dev);
 clk_disable_all:
 	clk_disable_unprepare(clk_xin);
 clk_dis_ahb:
@@ -469,6 +586,8 @@ static int sdhci_arasan_remove(struct platform_device *pdev)
 		phy_exit(sdhci_arasan->phy);
 	}
 
+	sdhci_arasan_unregister_sdclk(&pdev->dev);
+
 	ret = sdhci_pltfm_unregister(pdev);
 
 	clk_disable_unprepare(clk_ahb);
-- 
2.8.0.rc3.226.g39d4020

[PATCH 08/11] Documentation: phy: Let the rockchip eMMC PHY get an exported card clock

[Douglas Anderson]

As of an earlier change in this series ("Documentation: mmc:
sdhci-of-arasan: Add ability to export card clock") the SDHCI driver
used on Rockchip SoCs can now expose its clock.  Let's now specify that
the PHY can use it.

Letting the PHY get access to this clock means it can adjust
phyctrl_frqsel field appropriately.  Although the Rockchip PHY appears
slightly different than the reference Arasan one, you can see that the
Arasan datasheet [1] had it defined as:
  Select the frequency range of DLL operation:
  3b'000 => 200MHz to 170 MHz
  3b'001 => 170MHz to 140 MHz
  3b'010 => 140MHz to 110 MHz
  3b'011 => 110MHz to 80MHz
  3b'100 => 80MHz to 50 MHz
  3b'101 => 275Mhz to 250MHz
  3b'110 => 250MHz to 225MHz
  3b'111 => 225MHz to 200MHz

On the Rockchip version of the PHY we have less granularity but the idea
is the same.

[1]: https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
index 555cb0f40690..fd118b071e5e 100644
--- a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
+++ b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
@@ -7,6 +7,11 @@ Required properties:
  - reg: PHY register address offset and length in "general
    register files"
 
+Optional clocks (see ../clock/clock-bindings.txt), specified by name:
+ - emmcclk: The card clock exported by the SDHCI driver.  Although this is
+	    listed as optional (because most boards can get basic functionality
+	    without having access to it), it is strongly suggested.
+
 Example:
 
 
@@ -20,6 +25,8 @@ grf: syscon@ff770000 {
 	emmcphy: phy@f780 {
 		compatible = "rockchip,rk3399-emmc-phy";
 		reg = <0xf780 0x20>;
+		clocks = <&sdhci>;
+		clock-names = "emmcclk";
 		#phy-cells = <0>;
 	};
 };
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 08/11] Documentation: phy: Let the rockchip eMMC PHY get an exported card clock

[Rob Herring]

On Tue, Jun 07, 2016 at 03:44:41PM -0700, Douglas Anderson wrote:
> As of an earlier change in this series ("Documentation: mmc:
> sdhci-of-arasan: Add ability to export card clock") the SDHCI driver
> used on Rockchip SoCs can now expose its clock.  Let's now specify that
> the PHY can use it.
> 
> Letting the PHY get access to this clock means it can adjust
> phyctrl_frqsel field appropriately.  Although the Rockchip PHY appears
> slightly different than the reference Arasan one, you can see that the
> Arasan datasheet [1] had it defined as:
>   Select the frequency range of DLL operation:
>   3b'000 => 200MHz to 170 MHz
>   3b'001 => 170MHz to 140 MHz
>   3b'010 => 140MHz to 110 MHz
>   3b'011 => 110MHz to 80MHz
>   3b'100 => 80MHz to 50 MHz
>   3b'101 => 275Mhz to 250MHz
>   3b'110 => 250MHz to 225MHz
>   3b'111 => 225MHz to 200MHz
> 
> On the Rockchip version of the PHY we have less granularity but the idea
> is the same.
> 
> [1]: https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>  Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
> index 555cb0f40690..fd118b071e5e 100644
> --- a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
> +++ b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
> @@ -7,6 +7,11 @@ Required properties:
>   - reg: PHY register address offset and length in "general
>     register files"
>  
> +Optional clocks (see ../clock/clock-bindings.txt), specified by name:
> + - emmcclk: The card clock exported by the SDHCI driver.  Although this is

This reads like emmcclk is the property. You need to list out clocks and 
clock-names.

> +	    listed as optional (because most boards can get basic functionality
> +	    without having access to it), it is strongly suggested.
> +
>  Example:
>  
>  
> @@ -20,6 +25,8 @@ grf: syscon@ff770000 {
>  	emmcphy: phy@f780 {
>  		compatible = "rockchip,rk3399-emmc-phy";
>  		reg = <0xf780 0x20>;
> +		clocks = <&sdhci>;
> +		clock-names = "emmcclk";
>  		#phy-cells = <0>;
>  	};
>  };
> -- 
> 2.8.0.rc3.226.g39d4020
> 

Re: [PATCH 08/11] Documentation: phy: Let the rockchip eMMC PHY get an exported card clock

[Doug Anderson]

Rob,

On Fri, Jun 10, 2016 at 6:36 AM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Jun 07, 2016 at 03:44:41PM -0700, Douglas Anderson wrote:
>> As of an earlier change in this series ("Documentation: mmc:
>> sdhci-of-arasan: Add ability to export card clock") the SDHCI driver
>> used on Rockchip SoCs can now expose its clock.  Let's now specify that
>> the PHY can use it.
>>
>> Letting the PHY get access to this clock means it can adjust
>> phyctrl_frqsel field appropriately.  Although the Rockchip PHY appears
>> slightly different than the reference Arasan one, you can see that the
>> Arasan datasheet [1] had it defined as:
>>   Select the frequency range of DLL operation:
>>   3b'000 => 200MHz to 170 MHz
>>   3b'001 => 170MHz to 140 MHz
>>   3b'010 => 140MHz to 110 MHz
>>   3b'011 => 110MHz to 80MHz
>>   3b'100 => 80MHz to 50 MHz
>>   3b'101 => 275Mhz to 250MHz
>>   3b'110 => 250MHz to 225MHz
>>   3b'111 => 225MHz to 200MHz
>>
>> On the Rockchip version of the PHY we have less granularity but the idea
>> is the same.
>>
>> [1]: https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf
>>
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> ---
>>  Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
>> index 555cb0f40690..fd118b071e5e 100644
>> --- a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
>> +++ b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
>> @@ -7,6 +7,11 @@ Required properties:
>>   - reg: PHY register address offset and length in "general
>>     register files"
>>
>> +Optional clocks (see ../clock/clock-bindings.txt), specified by name:
>> + - emmcclk: The card clock exported by the SDHCI driver.  Although this is
>
> This reads like emmcclk is the property. You need to list out clocks and
> clock-names.

Thanks for the feedback.  Done.

-Doug

[PATCH 09/11] phy: rockchip-emmc: Set phyctrl_frqsel based on card clock

[Douglas Anderson]

The "phyctrl_frqsel" is described in the Arasan datasheet [1] as "the
frequency range of DLL operation".  Although the Rockchip variant of
this PHY has different ranges than the reference Arasan PHY it appears
as if the functionality is similar.  We should set this phyctrl field
properly.

Note: as per Rockchip engineers, apparently the "phyctrl_frqsel" is
actually only useful in HS200 / HS400 modes even though the DLL itself
it used for some purposes in all modes.  See the discussion in the
earlier change in this series: ("mmc: sdhci-of-arasan: Always power the
PHY off/on when clock changes").  In any case, it shouldn't hurt to set
this always.

Note that this change should allow boards to run at HS200 / HS400 speed
modes while running at 100 MHz or 150 MHz.  In fact, running HS400 at
150 MHz (giving 300 MB/s) is the main motivation of this series, since
performance is still good but signal integrity problems are less
prevelant at 150 MHz.

[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/phy/phy-rockchip-emmc.c | 74 +++++++++++++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 17 deletions(-)

diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c
index 8336053aea5c..0fce7359d468 100644
--- a/drivers/phy/phy-rockchip-emmc.c
+++ b/drivers/phy/phy-rockchip-emmc.c
@@ -14,6 +14,7 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
@@ -78,16 +79,61 @@
 struct rockchip_emmc_phy {
 	unsigned int	reg_offset;
 	struct regmap	*reg_base;
+	struct clk	*emmcclk;
 };
 
-static int rockchip_emmc_phy_power(struct rockchip_emmc_phy *rk_phy,
-				   bool on_off)
+static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
 {
+	struct rockchip_emmc_phy *rk_phy = phy_get_drvdata(phy);
 	unsigned int caldone;
 	unsigned int dllrdy;
+	unsigned int freqsel = PHYCTRL_FREQSEL_200M;
 	unsigned long timeout;
 
 	/*
+	 * We purposely get the clock here and not in probe to avoid the
+	 * circular dependency problem.  We expect:
+	 * - PHY driver to probe
+	 * - USB driver to start probe
+	 * - USB driver to register it's clock
+	 * - USB driver to get the PHY
+	 * - USB driver to power on the PHY
+	 */
+	if (!rk_phy->emmcclk) {
+		rk_phy->emmcclk = devm_clk_get(&phy->dev, "emmcclk");
+
+		/* Don't expect defer at this point; try next time */
+		if (PTR_ERR(rk_phy->emmcclk) == -EPROBE_DEFER) {
+			dev_warn(&phy->dev, "Unexpected emmcclk defer\n");
+			rk_phy->emmcclk = NULL;
+		}
+	}
+
+	if (!IS_ERR_OR_NULL(rk_phy->emmcclk)) {
+		unsigned long rate = clk_get_rate(rk_phy->emmcclk);
+
+		switch (rate) {
+		case 0 ... 74999999:
+			/* Nominal  50 MHz */
+			freqsel = PHYCTRL_FREQSEL_50M;
+			break;
+		case 75000000 ... 124999999:
+			/* Nominal 100 MHz */
+			freqsel = PHYCTRL_FREQSEL_100M;
+			break;
+		case 125000000 ... 174999999:
+			/* Nominal 150 MHz */
+			freqsel = PHYCTRL_FREQSEL_150M;
+			break;
+		default:
+			if (rate > 200000000)
+				dev_warn(&phy->dev, "Unsupported rate: %lu\n",
+					 rate);
+			break;
+		};
+	}
+
+	/*
 	 * Keep phyctrl_pdb and phyctrl_endll low to allow
 	 * initialization of CALIO state M/C DFFs
 	 */
@@ -132,6 +178,13 @@ static int rockchip_emmc_phy_power(struct rockchip_emmc_phy *rk_phy,
 		return -ETIMEDOUT;
 	}
 
+	/* Set the frequency of the DLL operation */
+	regmap_write(rk_phy->reg_base,
+		     rk_phy->reg_offset + GRF_EMMCPHY_CON0,
+		     HIWORD_UPDATE(freqsel, PHYCTRL_FREQSEL_MASK,
+				   PHYCTRL_FREQSEL_SHIFT));
+
+	/* Turn on the DLL */
 	regmap_write(rk_phy->reg_base,
 		     rk_phy->reg_offset + GRF_EMMCPHY_CON6,
 		     HIWORD_UPDATE(PHYCTRL_ENDLL_ENABLE,
@@ -167,15 +220,8 @@ static int rockchip_emmc_phy_power(struct rockchip_emmc_phy *rk_phy,
 
 static int rockchip_emmc_phy_power_off(struct phy *phy)
 {
-	struct rockchip_emmc_phy *rk_phy = phy_get_drvdata(phy);
-	int ret = 0;
-
 	/* Power down emmc phy analog blocks */
-	ret = rockchip_emmc_phy_power(rk_phy, PHYCTRL_PDB_PWR_OFF);
-	if (ret)
-		return ret;
-
-	return 0;
+	return rockchip_emmc_phy_power(phy, PHYCTRL_PDB_PWR_OFF);
 }
 
 static int rockchip_emmc_phy_power_on(struct phy *phy)
@@ -183,12 +229,6 @@ static int rockchip_emmc_phy_power_on(struct phy *phy)
 	struct rockchip_emmc_phy *rk_phy = phy_get_drvdata(phy);
 	int ret = 0;
 
-	/* DLL operation: 200 MHz */
-	regmap_write(rk_phy->reg_base,
-		     rk_phy->reg_offset + GRF_EMMCPHY_CON0,
-		     HIWORD_UPDATE(PHYCTRL_FREQSEL_200M,
-				   PHYCTRL_FREQSEL_MASK,
-				   PHYCTRL_FREQSEL_SHIFT));
 
 	/* Drive impedance: 50 Ohm */
 	regmap_write(rk_phy->reg_base,
@@ -212,7 +252,7 @@ static int rockchip_emmc_phy_power_on(struct phy *phy)
 				   PHYCTRL_OTAPDLYSEL_SHIFT));
 
 	/* Power up emmc phy analog blocks */
-	ret = rockchip_emmc_phy_power(rk_phy, PHYCTRL_PDB_PWR_ON);
+	ret = rockchip_emmc_phy_power(phy, PHYCTRL_PDB_PWR_ON);
 	if (ret)
 		return ret;
 
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 09/11] phy: rockchip-emmc: Set phyctrl_frqsel based on card clock

[Shawn Lin]

On 2016/6/8 6:44, Douglas Anderson wrote:
> The "phyctrl_frqsel" is described in the Arasan datasheet [1] as "the
> frequency range of DLL operation".  Although the Rockchip variant of
> this PHY has different ranges than the reference Arasan PHY it appears
> as if the functionality is similar.  We should set this phyctrl field
> properly.
>
> Note: as per Rockchip engineers, apparently the "phyctrl_frqsel" is
> actually only useful in HS200 / HS400 modes even though the DLL itself
> it used for some purposes in all modes.  See the discussion in the
> earlier change in this series: ("mmc: sdhci-of-arasan: Always power the
> PHY off/on when clock changes").  In any case, it shouldn't hurt to set
> this always.
>
> Note that this change should allow boards to run at HS200 / HS400 speed
> modes while running at 100 MHz or 150 MHz.  In fact, running HS400 at
> 150 MHz (giving 300 MB/s) is the main motivation of this series, since
> performance is still good but signal integrity problems are less
> prevelant at 150 MHz.

Thanks for doing this, but I think we should limit freq if assigning
max-frequency from DT more explicitly since the PHY could only support
50/100/150/200M for hs200/400? Otherwise I can't say if the PHY could
always work well. i.e if geting 125000000 ... 174999999 , you code make
the phyctrl_frqsel to be 150M, so it will be 15% missing of precision
for tuning delay element. Ideally, the sample point should be in the
middle of window, but I don't know if there is a bad HW	design makes
the window small enough which need special care about it.



>
> [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/phy/phy-rockchip-emmc.c | 74 +++++++++++++++++++++++++++++++----------
>  1 file changed, 57 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c
> index 8336053aea5c..0fce7359d468 100644
> --- a/drivers/phy/phy-rockchip-emmc.c
> +++ b/drivers/phy/phy-rockchip-emmc.c
> @@ -14,6 +14,7 @@
>   * GNU General Public License for more details.
>   */
>
> +#include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/module.h>
> @@ -78,16 +79,61 @@
>  struct rockchip_emmc_phy {
>  	unsigned int	reg_offset;
>  	struct regmap	*reg_base;
> +	struct clk	*emmcclk;
>  };
>
> -static int rockchip_emmc_phy_power(struct rockchip_emmc_phy *rk_phy,
> -				   bool on_off)
> +static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
>  {
> +	struct rockchip_emmc_phy *rk_phy = phy_get_drvdata(phy);
>  	unsigned int caldone;
>  	unsigned int dllrdy;
> +	unsigned int freqsel = PHYCTRL_FREQSEL_200M;
>  	unsigned long timeout;
>
>  	/*
> +	 * We purposely get the clock here and not in probe to avoid the
> +	 * circular dependency problem.  We expect:
> +	 * - PHY driver to probe
> +	 * - USB driver to start probe
> +	 * - USB driver to register it's clock
> +	 * - USB driver to get the PHY
> +	 * - USB driver to power on the PHY

USB?

> +	 */
> +	if (!rk_phy->emmcclk) {
> +		rk_phy->emmcclk = devm_clk_get(&phy->dev, "emmcclk");
> +
> +		/* Don't expect defer at this point; try next time */
> +		if (PTR_ERR(rk_phy->emmcclk) == -EPROBE_DEFER) {
> +			dev_warn(&phy->dev, "Unexpected emmcclk defer\n");
> +			rk_phy->emmcclk = NULL;
> +		}
> +	}
> +
> +	if (!IS_ERR_OR_NULL(rk_phy->emmcclk)) {
> +		unsigned long rate = clk_get_rate(rk_phy->emmcclk);
> +
> +		switch (rate) {
> +		case 0 ... 74999999:
> +			/* Nominal  50 MHz */
> +			freqsel = PHYCTRL_FREQSEL_50M;
> +			break;
> +		case 75000000 ... 124999999:
> +			/* Nominal 100 MHz */
> +			freqsel = PHYCTRL_FREQSEL_100M;
> +			break;
> +		case 125000000 ... 174999999:
> +			/* Nominal 150 MHz */
> +			freqsel = PHYCTRL_FREQSEL_150M;
> +			break;
> +		default:
> +			if (rate > 200000000)
> +				dev_warn(&phy->dev, "Unsupported rate: %lu\n",
> +					 rate);
> +			break;
> +		};
> +	}
> +
> +	/*
>  	 * Keep phyctrl_pdb and phyctrl_endll low to allow
>  	 * initialization of CALIO state M/C DFFs
>  	 */
> @@ -132,6 +178,13 @@ static int rockchip_emmc_phy_power(struct rockchip_emmc_phy *rk_phy,
>  		return -ETIMEDOUT;
>  	}
>
> +	/* Set the frequency of the DLL operation */
> +	regmap_write(rk_phy->reg_base,
> +		     rk_phy->reg_offset + GRF_EMMCPHY_CON0,
> +		     HIWORD_UPDATE(freqsel, PHYCTRL_FREQSEL_MASK,
> +				   PHYCTRL_FREQSEL_SHIFT));
> +
> +	/* Turn on the DLL */
>  	regmap_write(rk_phy->reg_base,
>  		     rk_phy->reg_offset + GRF_EMMCPHY_CON6,
>  		     HIWORD_UPDATE(PHYCTRL_ENDLL_ENABLE,
> @@ -167,15 +220,8 @@ static int rockchip_emmc_phy_power(struct rockchip_emmc_phy *rk_phy,
>
>  static int rockchip_emmc_phy_power_off(struct phy *phy)
>  {
> -	struct rockchip_emmc_phy *rk_phy = phy_get_drvdata(phy);
> -	int ret = 0;
> -
>  	/* Power down emmc phy analog blocks */
> -	ret = rockchip_emmc_phy_power(rk_phy, PHYCTRL_PDB_PWR_OFF);
> -	if (ret)
> -		return ret;
> -
> -	return 0;
> +	return rockchip_emmc_phy_power(phy, PHYCTRL_PDB_PWR_OFF);
>  }
>
>  static int rockchip_emmc_phy_power_on(struct phy *phy)
> @@ -183,12 +229,6 @@ static int rockchip_emmc_phy_power_on(struct phy *phy)
>  	struct rockchip_emmc_phy *rk_phy = phy_get_drvdata(phy);
>  	int ret = 0;
>
> -	/* DLL operation: 200 MHz */
> -	regmap_write(rk_phy->reg_base,
> -		     rk_phy->reg_offset + GRF_EMMCPHY_CON0,
> -		     HIWORD_UPDATE(PHYCTRL_FREQSEL_200M,
> -				   PHYCTRL_FREQSEL_MASK,
> -				   PHYCTRL_FREQSEL_SHIFT));
>
>  	/* Drive impedance: 50 Ohm */
>  	regmap_write(rk_phy->reg_base,
> @@ -212,7 +252,7 @@ static int rockchip_emmc_phy_power_on(struct phy *phy)
>  				   PHYCTRL_OTAPDLYSEL_SHIFT));
>
>  	/* Power up emmc phy analog blocks */
> -	ret = rockchip_emmc_phy_power(rk_phy, PHYCTRL_PDB_PWR_ON);
> +	ret = rockchip_emmc_phy_power(phy, PHYCTRL_PDB_PWR_ON);
>  	if (ret)
>  		return ret;
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 09/11] phy: rockchip-emmc: Set phyctrl_frqsel based on card clock

[Doug Anderson]

Shawn,

On Mon, Jun 13, 2016 at 1:54 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> On 2016/6/8 6:44, Douglas Anderson wrote:
>>
>> The "phyctrl_frqsel" is described in the Arasan datasheet [1] as "the
>> frequency range of DLL operation".  Although the Rockchip variant of
>> this PHY has different ranges than the reference Arasan PHY it appears
>> as if the functionality is similar.  We should set this phyctrl field
>> properly.
>>
>> Note: as per Rockchip engineers, apparently the "phyctrl_frqsel" is
>> actually only useful in HS200 / HS400 modes even though the DLL itself
>> it used for some purposes in all modes.  See the discussion in the
>> earlier change in this series: ("mmc: sdhci-of-arasan: Always power the
>> PHY off/on when clock changes").  In any case, it shouldn't hurt to set
>> this always.
>>
>> Note that this change should allow boards to run at HS200 / HS400 speed
>> modes while running at 100 MHz or 150 MHz.  In fact, running HS400 at
>> 150 MHz (giving 300 MB/s) is the main motivation of this series, since
>> performance is still good but signal integrity problems are less
>> prevelant at 150 MHz.
>
>
> Thanks for doing this, but I think we should limit freq if assigning
> max-frequency from DT more explicitly since the PHY could only support
> 50/100/150/200M for hs200/400? Otherwise I can't say if the PHY could
> always work well. i.e if geting 125000000 ... 174999999 , you code make
> the phyctrl_frqsel to be 150M, so it will be 15% missing of precision
> for tuning delay element. Ideally, the sample point should be in the
> middle of window, but I don't know if there is a bad HW design makes
> the window small enough which need special care about it.

What would you suggest as a valid range, then?

>From the public Arasan datasheet they seem to indicate +/- 15 MHz is
sane.  Does that sound OK?  Presuming that all of your numbers
(50/100/150/200) are centers, that means that we could support clock
rates of:

35 MHz - 65 MHz
85 MHz - 115 MHz
135 MHz - 165 MHz
185 MHz - 200 MHz


So how about if we add a warning for things that are outside of those
ranges?  ...except no warning for < 35 MHz since presumably we're not
using high speed modes when the DLL is that slow and so we're OK.


NOTE: In rk3399 it's actually quite important to handle clocks that
aren't exactly the right MHz.  When you ask for 150 MHz you actually
end up a child of GPLL and actually end up at 148.5 MHz.  This should
be close enough, but it's not exactly 150 MHz.  If we can't handle
148.5 MHz then the 150 MHz setting in the PHY is useless.

Also note that on rk3399 we're fairly limited on the number of rates
we can actually make since they need to be even divisors of 594 MHz or
800 MHz (assuming you don't rejigger all the PLLs in the SoC or
something).  Most of the rates are actually in those ranges...


-Doug

Re: [PATCH 09/11] phy: rockchip-emmc: Set phyctrl_frqsel based on card clock

[Shawn Lin]

在 2016/6/14 7:05, Doug Anderson 写道:
> Shawn,
>
> On Mon, Jun 13, 2016 at 1:54 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>> On 2016/6/8 6:44, Douglas Anderson wrote:
>>>
>>> The "phyctrl_frqsel" is described in the Arasan datasheet [1] as "the
>>> frequency range of DLL operation".  Although the Rockchip variant of
>>> this PHY has different ranges than the reference Arasan PHY it appears
>>> as if the functionality is similar.  We should set this phyctrl field
>>> properly.
>>>
>>> Note: as per Rockchip engineers, apparently the "phyctrl_frqsel" is
>>> actually only useful in HS200 / HS400 modes even though the DLL itself
>>> it used for some purposes in all modes.  See the discussion in the
>>> earlier change in this series: ("mmc: sdhci-of-arasan: Always power the
>>> PHY off/on when clock changes").  In any case, it shouldn't hurt to set
>>> this always.
>>>
>>> Note that this change should allow boards to run at HS200 / HS400 speed
>>> modes while running at 100 MHz or 150 MHz.  In fact, running HS400 at
>>> 150 MHz (giving 300 MB/s) is the main motivation of this series, since
>>> performance is still good but signal integrity problems are less
>>> prevelant at 150 MHz.
>>
>>
>> Thanks for doing this, but I think we should limit freq if assigning
>> max-frequency from DT more explicitly since the PHY could only support
>> 50/100/150/200M for hs200/400? Otherwise I can't say if the PHY could
>> always work well. i.e if geting 125000000 ... 174999999 , you code make
>> the phyctrl_frqsel to be 150M, so it will be 15% missing of precision
>> for tuning delay element. Ideally, the sample point should be in the
>> middle of window, but I don't know if there is a bad HW design makes
>> the window small enough which need special care about it.
>
> What would you suggest as a valid range, then?
>
>>From the public Arasan datasheet they seem to indicate +/- 15 MHz is
> sane.  Does that sound OK?  Presuming that all of your numbers
> (50/100/150/200) are centers, that means that we could support clock
> rates of:
>
> 35 MHz - 65 MHz
> 85 MHz - 115 MHz
> 135 MHz - 165 MHz
> 185 MHz - 200 MHz
>
>
> So how about if we add a warning for things that are outside of those
> ranges?  ...except no warning for < 35 MHz since presumably we're not
> using high speed modes when the DLL is that slow and so we're OK.

a warning should be ok.
If we ask 150M, but PLL only provide 175M maybe, then should we
fallback it to 150M or promote it to 200M when setting?

>
>
> NOTE: In rk3399 it's actually quite important to handle clocks that
> aren't exactly the right MHz.  When you ask for 150 MHz you actually
> end up a child of GPLL and actually end up at 148.5 MHz.  This should
> be close enough, but it's not exactly 150 MHz.  If we can't handle
> 148.5 MHz then the 150 MHz setting in the PHY is useless.
>
> Also note that on rk3399 we're fairly limited on the number of rates
> we can actually make since they need to be even divisors of 594 MHz or
> 800 MHz (assuming you don't rejigger all the PLLs in the SoC or
> something).  Most of the rates are actually in those ranges...

Yes I don't.

>
>
> -Doug
>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 09/11] phy: rockchip-emmc: Set phyctrl_frqsel based on card clock

[Doug Anderson]

Shawn,

On Mon, Jun 13, 2016 at 5:24 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>>> From the public Arasan datasheet they seem to indicate +/- 15 MHz is
>>
>> sane.  Does that sound OK?  Presuming that all of your numbers
>> (50/100/150/200) are centers, that means that we could support clock
>> rates of:
>>
>> 35 MHz - 65 MHz
>> 85 MHz - 115 MHz
>> 135 MHz - 165 MHz
>> 185 MHz - 200 MHz
>>
>>
>> So how about if we add a warning for things that are outside of those
>> ranges?  ...except no warning for < 35 MHz since presumably we're not
>> using high speed modes when the DLL is that slow and so we're OK.
>
>
> a warning should be ok.
> If we ask 150M, but PLL only provide 175M maybe, then should we
> fallback it to 150M or promote it to 200M when setting?

I made it a warning in V2 but still picked the closest reasonable
value.  See what you think.  The PHY really isn't in control of this
clock, so the warning is the best it can do.  Presumably someone
designing a system with this PHY in it would see the warning an
realize that they should make SDHCI run at more reasonable rates...

-Doug

[PATCH 10/11] phy: rockchip-emmc: Minor code cleanup in rockchip_emmc_phy_power_off()

[Douglas Anderson]

There's no reason to store the return value of rockchip_emmc_phy_power()
in a variable nor to check it.  Just return it.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/phy/phy-rockchip-emmc.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c
index 0fce7359d468..188e4c387ba8 100644
--- a/drivers/phy/phy-rockchip-emmc.c
+++ b/drivers/phy/phy-rockchip-emmc.c
@@ -227,8 +227,6 @@ static int rockchip_emmc_phy_power_off(struct phy *phy)
 static int rockchip_emmc_phy_power_on(struct phy *phy)
 {
 	struct rockchip_emmc_phy *rk_phy = phy_get_drvdata(phy);
-	int ret = 0;
-
 
 	/* Drive impedance: 50 Ohm */
 	regmap_write(rk_phy->reg_base,
@@ -252,11 +250,7 @@ static int rockchip_emmc_phy_power_on(struct phy *phy)
 				   PHYCTRL_OTAPDLYSEL_SHIFT));
 
 	/* Power up emmc phy analog blocks */
-	ret = rockchip_emmc_phy_power(phy, PHYCTRL_PDB_PWR_ON);
-	if (ret)
-		return ret;
-
-	return 0;
+	return rockchip_emmc_phy_power(phy, PHYCTRL_PDB_PWR_ON);
 }
 
 static const struct phy_ops ops = {
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 10/11] phy: rockchip-emmc: Minor code cleanup in rockchip_emmc_phy_power_off()

[Shawn Lin]

On 2016/6/8 6:44, Douglas Anderson wrote:
> There's no reason to store the return value of rockchip_emmc_phy_power()
> in a variable nor to check it.  Just return it.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>  drivers/phy/phy-rockchip-emmc.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c
> index 0fce7359d468..188e4c387ba8 100644
> --- a/drivers/phy/phy-rockchip-emmc.c
> +++ b/drivers/phy/phy-rockchip-emmc.c
> @@ -227,8 +227,6 @@ static int rockchip_emmc_phy_power_off(struct phy *phy)
>  static int rockchip_emmc_phy_power_on(struct phy *phy)
>  {

I saw the cleanup for power_off is done on [patch 9/11]

shouldn't be in this patch? :)

>  	struct rockchip_emmc_phy *rk_phy = phy_get_drvdata(phy);
> -	int ret = 0;
> -
>
>  	/* Drive impedance: 50 Ohm */
>  	regmap_write(rk_phy->reg_base,
> @@ -252,11 +250,7 @@ static int rockchip_emmc_phy_power_on(struct phy *phy)
>  				   PHYCTRL_OTAPDLYSEL_SHIFT));
>
>  	/* Power up emmc phy analog blocks */
> -	ret = rockchip_emmc_phy_power(phy, PHYCTRL_PDB_PWR_ON);
> -	if (ret)
> -		return ret;
> -
> -	return 0;
> +	return rockchip_emmc_phy_power(phy, PHYCTRL_PDB_PWR_ON);
>  }
>
>  static const struct phy_ops ops = {
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 10/11] phy: rockchip-emmc: Minor code cleanup in rockchip_emmc_phy_power_off()

[Doug Anderson]

Hi,

On Mon, Jun 13, 2016 at 1:56 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> On 2016/6/8 6:44, Douglas Anderson wrote:
>>
>> There's no reason to store the return value of rockchip_emmc_phy_power()
>> in a variable nor to check it.  Just return it.
>>
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> ---
>>  drivers/phy/phy-rockchip-emmc.c | 8 +-------
>>  1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/drivers/phy/phy-rockchip-emmc.c
>> b/drivers/phy/phy-rockchip-emmc.c
>> index 0fce7359d468..188e4c387ba8 100644
>> --- a/drivers/phy/phy-rockchip-emmc.c
>> +++ b/drivers/phy/phy-rockchip-emmc.c
>> @@ -227,8 +227,6 @@ static int rockchip_emmc_phy_power_off(struct phy
>> *phy)
>>  static int rockchip_emmc_phy_power_on(struct phy *phy)
>>  {
>
>
> I saw the cleanup for power_off is done on [patch 9/11]
>
> shouldn't be in this patch? :)

Yeah, I tried to, but then I realized that I couldn't split that out
too easily because the previous patch touched the same code.

...but actually, reordering things solves all the problems so I've done that.


-Doug

[PATCH 11/11] arm64: dts: rockchip: Provide emmcclk to PHY for rk3399

[Douglas Anderson]

Previous changes in this series allowed exposing the card clock from the
rk3399 SDHCI device and allowed consuming the card clock in the rk3399
eMMC PHY.  Hook things up in the main rk3399 dtsi file.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index 1b57e92e0093..004b599ca285 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -225,8 +225,10 @@
 		assigned-clock-rates = <200000000>;
 		clocks = <&cru SCLK_EMMC>, <&cru ACLK_EMMC>;
 		clock-names = "clk_xin", "clk_ahb";
+		clock-output-names = "emmc_cardclock";
 		phys = <&emmc_phy>;
 		phy-names = "phy_arasan";
+		#clock-cells = <0>;
 		status = "disabled";
 	};
 
@@ -621,6 +623,8 @@
 		emmc_phy: phy@f780 {
 			compatible = "rockchip,rk3399-emmc-phy";
 			reg = <0xf780 0x24>;
+			clocks = <&sdhci>;
+			clock-names = "emmcclk";
 			#phy-cells = <0>;
 			status = "disabled";
 		};
-- 
2.8.0.rc3.226.g39d4020