From: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
To: "Christoph Müllner" <christoph.muellner@theobroma-systems.com>
Cc: "Doug Anderson" <dianders@chromium.org>,
"Rob Herring" <robh+dt@kernel.org>,
"Mark Rutland" <mark.rutland@arm.com>,
"Heiko Stübner" <heiko@sntech.de>,
"Shawn Lin" <shawn.lin@rock-chips.com>,
"Kishon Vijay Abraham I" <kishon@ti.com>,
"Enric Balletbo i Serra" <enric.balletbo@collabora.com>,
"Klaus Goger" <klaus.goger@theobroma-systems.com>,
"Viresh Kumar" <viresh.kumar@linaro.org>,
"Matthias Brugger" <mbrugger@suse.com>,
"Emil Renner Berthing" <kernel@esmil.dk>,
"Tony Xie" <tony.xie@rock-chips.com>,
"Randy Li" <ayaka@soulik.info>,
"Vicente Bergas" <vicencb@gmail.com>,
"Ezequiel Garcia" <ezequiel@collabora.com>,
devicetree@vger.kernel.org,
"Linux ARM" <linux-arm-kernel@lists.infradead.org>,
"open list:ARM/Rockchip SoC..."
<linux-rockchip@lists.infradead.org>
Subject: Re: [PATCH 1/3] phy: rockchip-emmc: Allow to set drive impedance via DTS.
Date: Fri, 1 Mar 2019 19:41:03 +0100 [thread overview]
Message-ID: <72859695-2E77-4EFF-9555-54907830849F@theobroma-systems.com> (raw)
In-Reply-To: <CB9331AE-2FEC-4646-BD8E-BDF9F4E48F74@theobroma-systems.com>
> On 01.03.2019, at 19:09, Christoph Müllner <christoph.muellner@theobroma-systems.com> wrote:
>
> Hi Doug,
>
>> On 01.03.2019, at 17:48, Doug Anderson <dianders@chromium.org> wrote:
>>
>> Hi,
>>
>> On Fri, Mar 1, 2019 at 7:37 AM Christoph Muellner
>> <christoph.muellner@theobroma-systems.com> wrote:
>>>
>>> The rockchip-emmc PHY can be configured with different
>>> drive impedance values. Currenlty a value of 50 Ohm is
>>> hard coded into the driver.
>>>
>>> This patch introduces the DTS property 'drive-impedance-ohm'
>>> for the rockchip-emmc phy node, which uses the value from the DTS
>>> to setup the drive impedance accordingly.
>>>
>>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>> ---
>>> drivers/phy/rockchip/phy-rockchip-emmc.c | 38 ++++++++++++++++++++++++++++++--
>>> 1 file changed, 36 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
>>> index 19bf84f0bc67..5413fa73dd45 100644
>>> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
>>> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
>>> @@ -87,6 +87,7 @@ struct rockchip_emmc_phy {
>>> unsigned int reg_offset;
>>> struct regmap *reg_base;
>>> struct clk *emmcclk;
>>> + unsigned int drive_impedance;
>>> };
>>>
>>> static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
>>> @@ -281,10 +282,10 @@ static int rockchip_emmc_phy_power_on(struct phy *phy)
>>> {
>>> struct rockchip_emmc_phy *rk_phy = phy_get_drvdata(phy);
>>>
>>> - /* Drive impedance: 50 Ohm */
>>> + /* Drive impedance: from DTS */
>>> regmap_write(rk_phy->reg_base,
>>> rk_phy->reg_offset + GRF_EMMCPHY_CON6,
>>> - HIWORD_UPDATE(PHYCTRL_DR_50OHM,
>>> + HIWORD_UPDATE(rk_phy->drive_impedance,
>>> PHYCTRL_DR_MASK,
>>> PHYCTRL_DR_SHIFT));
>>>
>>> @@ -314,6 +315,28 @@ static const struct phy_ops ops = {
>>> .owner = THIS_MODULE,
>>> };
>>>
>>> +static u32 convert_drive_impedance_ohm(struct platform_device *pdev, u32 dr_ohm)
>>> +{
>>> + switch (dr_ohm) {
>>> + case 100:
>>> + return PHYCTRL_DR_100OHM;
>>> + case 66:
>>> + return PHYCTRL_DR_66OHM;
>>> + case 50:
>>> + return PHYCTRL_DR_50OHM;
>>> + case 40:
>>> + return PHYCTRL_DR_40OHM;
>>> + case 33:
>>> + return PHYCTRL_DR_33OHM;
>>> + }
>>> +
>>> + dev_warn(&pdev->dev,
>>> + "Invalid value %u for drive-impedance-ohm. "
>>> + "Falling back to 50 Ohm.\n",
>>> + dr_ohm);
>>> + return PHYCTRL_DR_50OHM;
>>> +}
>>> +
>>> static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>>> {
>>> struct device *dev = &pdev->dev;
>>> @@ -322,6 +345,7 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>>> struct phy_provider *phy_provider;
>>> struct regmap *grf;
>>> unsigned int reg_offset;
>>> + u32 val;
>>>
>>> if (!dev->parent || !dev->parent->of_node)
>>> return -ENODEV;
>>> @@ -345,6 +369,16 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>>> rk_phy->reg_offset = reg_offset;
>>> rk_phy->reg_base = grf;
>>>
>>> + if (of_property_read_u32(dev->of_node, "drive-impedance-ohm", &val)) {
>>> + dev_info(dev,
>>> + "Missing drive-impedance-ohm property in node %s "
>>> + "Falling back to 50 Ohm.\n",
>>> + dev->of_node->name);
>>
>> This is awfully noisy for something that pretty much all existing
>> boards will run. debug level instead of info level? Also:
>
> Removed for v2 as suggested by Robin.
>
>>
>> * Don't split strings
>> across lines
>>
>> * There's a magic % thing to get the name of an OF node. Use that.
>>
>>
>>> + rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
>>> + } else {
>>> + rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
>>> + }
>>
>> It's been a long time since I looked at this, but I could have sworn
>> that it was more complicated than that. Specifically I though you
>> were supposed to query the eMMC card for what it supported and then
>> resolve that with what the host could support.
>>
>> Assuming that this is supposed to be queried from the card (which is
>> how I remember it) then you definitely don't want it in the device
>> tree since you want to be able to stuff various different eMMC parts
>> and we should be able to figure out the impedance at runtime.
>>
>>
>> NOTE: IIRC the old value of 50 ohms is required to be supported by all
>> hosts and cards and is specified to be the default.
>>
>
> When using 50 ohms on our board in HS-400 mode (200 MHz), then
> we see communication errors. These are cause by additional components
> on the clock line, which damp the 200 Mhz signal too much.
>
> We could do the following:
>
> * sdhci.c provides a default implementation to set the drive impedance (in HOST_CONTROL2,
> like it is done already now as part of sdhci_set_ios())
> * sdhci-of-arasan.c installs an alternative implementation in case of Rockchip's eMMC controller
> * the alternative implementation uses a callback to the Rockchip's eMMC phy driver
Generally, I would not make this either-or (I may have been less than clear
in my comments to the internal ticket), but rather teach the sdhci-driver to
always notify the eMMC PHY driver (if there is one) of the change.
For the RK3399’s sdhci implementation, the bits in the HOST_CONTROL2
register are ‘reserved’ and marked R/O, so the current implementation will
not work anyway and the PHY driver needs to be notified of the change in
requested drive-strength.
> * the Rockchip eMMC phy driver sets the drive impedance accordingly
>
> But still we would need a mechanism to override this for our board.
> And exactly this override mechanism is provided by the patch series.
The PHY would then need to determine the proper operation point (or an
mapping from a table) to achieve the requested line characteristic for any
given board. In other words: the DTS for puma would still contain an entry
to allow us to override to 33 Ohm when switching to HS-400.
> Thanks,
> Christoph
>
>>
>> I do see at least the summary of what I thought of this before at
>> <https://patchwork.kernel.org/patch/9086541/>
>>
>>
>> (Sorry if the above is wrong and feel free to correct me--I don't have
>> time at the moment to do all the full research but hopefully you can
>> dig more based on my pointers. Feel free to call me on my BS)
>>
>>
>> -Doug
next prev parent reply other threads:[~2019-03-01 18:41 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-01 15:33 [PATCH 1/3] phy: rockchip-emmc: Allow to set drive impedance via DTS Christoph Muellner
2019-03-01 15:33 ` [PATCH 2/3] arm64: dts: rockchip: Define drive-impedance-ohm for RK3399's emmc-phy Christoph Muellner
2019-11-11 9:51 ` arm64: dts: rockchip: Disable HS400 for mmc on rk3399-roc-pc Markus Reichl
2019-11-14 13:10 ` Heiko Stuebner
2019-11-15 10:37 ` Markus Reichl
2019-11-15 11:19 ` Heiko Stübner
2019-11-18 16:08 ` Doug Anderson
2019-11-18 19:05 ` Markus Reichl
2019-11-18 1:01 ` Heiko Stuebner
2019-03-01 15:33 ` [PATCH 3/3] arm64: dts: rockchip: Decrease emmc-phy's drive impedance on rk3399-puma Christoph Muellner
2019-03-01 15:59 ` [PATCH 1/3] phy: rockchip-emmc: Allow to set drive impedance via DTS Heiko Stuebner
2019-03-01 16:21 ` Christoph Müllner
2019-03-01 16:48 ` Doug Anderson
2019-03-01 16:59 ` Philipp Tomsich
[not found] ` <CAD=FV=UUuQYwh2DMgB5FLuiE1BwbTMTuc9rAte1J5L_JP5qk1Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2019-03-01 18:09 ` Christoph Müllner
2019-03-01 18:41 ` Philipp Tomsich [this message]
2019-03-01 16:51 ` Robin Murphy
2019-03-01 17:38 ` Christoph Müllner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=72859695-2E77-4EFF-9555-54907830849F@theobroma-systems.com \
--to=philipp.tomsich@theobroma-systems.com \
--cc=ayaka@soulik.info \
--cc=christoph.muellner@theobroma-systems.com \
--cc=devicetree@vger.kernel.org \
--cc=dianders@chromium.org \
--cc=enric.balletbo@collabora.com \
--cc=ezequiel@collabora.com \
--cc=heiko@sntech.de \
--cc=kernel@esmil.dk \
--cc=kishon@ti.com \
--cc=klaus.goger@theobroma-systems.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=mark.rutland@arm.com \
--cc=mbrugger@suse.com \
--cc=robh+dt@kernel.org \
--cc=shawn.lin@rock-chips.com \
--cc=tony.xie@rock-chips.com \
--cc=vicencb@gmail.com \
--cc=viresh.kumar@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).