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

[Philipp Tomsich]

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

Not necessarily (i.e. thereâs not a bijective relationship, as far as I know):
Higher drive-strenghts allow for faster speeds, but lower drive-strenghts
may have an improved emission profile.  For the RK3399-Q7, emissions
with a 33 Ohm preset are ok and we donât see any reflections.

The chip documentation has the following info:
	Impedance	Relative		Remark
	50 Ohm		x1			Default driver type, supports up to 200MHz operation
	33 Ohm		x1.5			Supports up to 200MHz operation
	66 Ohm		x0.75		The weakest driver that supports up to 200MHz operation.
	100 Ohm		x0.5			For Low noise and low EMI systems, Minimum operating frequency is system dependent.
	40 Ohm		x1.2			Supports up to DDR 200MHz operation.


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

For our board, we have a series resistor in the line (originally a precaution against
emissions and the BIOS_DISABLE circuitry) â do thereâs more dampening on the
signal than on the reference design.

> 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