> On 01.03.2019, at 19:09, Christoph Müllner <christoph.muellner@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> Hi Doug,
>
>> On 01.03.2019, at 17:48, Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>>
>> Hi,
>>
>> On Fri, Mar 1, 2019 at 7:37 AM Christoph Muellner
>> <christoph.muellner@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx>
>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@xxxxxxxxxxxxxxxxxxxxx>
>>> ---
>>> 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
