| Message ID | 20190301153348.29870-1-christoph.muellner@theobroma-systems.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] phy: rockchip-emmc: Allow to set drive impedance via DTS. | expand |
Hi Christoph, Am Freitag, 1. März 2019, 16:33:43 CET schrieb Christoph Muellner: > 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 ++++++++++++++++++++++++++++++-- looks good on first glance, but is missing an addition to the emmc-phy devicetree binding in Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt Heiko > 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); > + rk_phy->drive_impedance = PHYCTRL_DR_50OHM; > + } else { > + rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val); > + } > + > generic_phy = devm_phy_create(dev, dev->of_node, &ops); > if (IS_ERR(generic_phy)) { > dev_err(dev, "failed to create PHY\n"); >
Hi Heiko, > On 01.03.2019, at 16:59, Heiko Stuebner <heiko@sntech.de> wrote: > > Hi Christoph, > > Am Freitag, 1. März 2019, 16:33:43 CET schrieb Christoph Muellner: >> 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 ++++++++++++++++++++++++++++++-- > > looks good on first glance, but is missing an addition to the emmc-phy > devicetree binding in > Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt you are right. I've just sent that in a separate patch (DT doc changes need to be in a separate commit anyways). Thanks, Christoph > > > Heiko > >> 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); >> + rk_phy->drive_impedance = PHYCTRL_DR_50OHM; >> + } else { >> + rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val); >> + } >> + >> generic_phy = devm_phy_create(dev, dev->of_node, &ops); >> if (IS_ERR(generic_phy)) { >> dev_err(dev, "failed to create PHY\n"); >> > > > >
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: * 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. 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
On 01/03/2019 15:33, Christoph Muellner 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); I think we're aiming to use %pOF for node names now, but here it looks redundant anyway - dev_info() will already show the device name, which for an of_platform device is the same thing. > + rk_phy->drive_impedance = PHYCTRL_DR_50OHM; > + } else { > + rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val); > + } FWIW, if you initialise val to the default (i.e. 50) then you could get rid of the "else" case and just do the conversion unconditionally. That's a fairly common idiom for reading optional properties. Robin. > + > generic_phy = devm_phy_create(dev, dev->of_node, &ops); > if (IS_ERR(generic_phy)) { > dev_err(dev, "failed to create PHY\n"); >
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: > > * 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
Hi Robin, > On 01.03.2019, at 17:51, Robin Murphy <robin.murphy@arm.com> wrote: > > On 01/03/2019 15:33, Christoph Muellner 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); > > I think we're aiming to use %pOF for node names now, but here it looks redundant anyway - dev_info() will already show the device name, which for an of_platform device is the same thing. > >> + rk_phy->drive_impedance = PHYCTRL_DR_50OHM; >> + } else { >> + rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val); >> + } > > FWIW, if you initialise val to the default (i.e. 50) then you could get rid of the "else" case and just do the conversion unconditionally. That's a fairly common idiom for reading optional properties. Ack. For v2 I've removed the else case (not defined value). Thanks, Christoph > > Robin. > >> + >> generic_phy = devm_phy_create(dev, dev->of_node, &ops); >> if (IS_ERR(generic_phy)) { >> dev_err(dev, "failed to create PHY\n");
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 * 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
> 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
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); + rk_phy->drive_impedance = PHYCTRL_DR_50OHM; + } else { + rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val); + } + generic_phy = devm_phy_create(dev, dev->of_node, &ops); if (IS_ERR(generic_phy)) { dev_err(dev, "failed to create PHY\n");