Re: [PATCH 1/3] phy: rockchip-emmc: Allow to set drive impedance via DTS.

[Christoph MÃllner]

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

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