| Message ID | 20240814223555.3695-3-detlev.casanova@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add dw_mmc support for rk3576 | expand |
Hi Detlev 在 2024/8/15 6:34, Detlev Casanova 写道: > From: Shawn Lin <shawn.lin@rock-chips.com> > > v2 tuning will inherit pre-stage loader's phase settings for the first > time, and do re-tune if necessary. > Re-tune will still try the rough degrees, for instance, 90, 180, 270, > 360 but continue to do the fine tuning if sample window isn't good > enough. > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com> > --- > drivers/mmc/host/dw_mmc-rockchip.c | 49 ++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c > index b07190ba4b7ac..367633f4e8892 100644 > --- a/drivers/mmc/host/dw_mmc-rockchip.c > +++ b/drivers/mmc/host/dw_mmc-rockchip.c > @@ -24,6 +24,8 @@ struct dw_mci_rockchip_priv_data { > struct clk *sample_clk; > int default_sample_phase; > int num_phases; > + bool use_v2_tuning; > + int last_degree; > }; > > static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios) > @@ -134,6 +136,42 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios) > #define TUNING_ITERATION_TO_PHASE(i, num_phases) \ > (DIV_ROUND_UP((i) * 360, num_phases)) > > +static int dw_mci_v2_execute_tuning(struct dw_mci_slot *slot, u32 opcode) > +{ > + struct dw_mci *host = slot->host; > + struct dw_mci_rockchip_priv_data *priv = host->priv; > + struct mmc_host *mmc = slot->mmc; > + u32 degrees[4] = {90, 180, 270, 360}; > + int i; > + static bool inherit = true; > + > + if (inherit) { > + inherit = false; > + i = clk_get_phase(priv->sample_clk) / 90 - 1; > + goto done; > + } > + > + /* v2 only support 4 degrees in theory */ > + for (i = 0; i < ARRAY_SIZE(degrees); i++) { > + if (degrees[i] == priv->last_degree) > + continue; > + > + clk_set_phase(priv->sample_clk, degrees[i]); > + if (!mmc_send_tuning(mmc, opcode, NULL)) > + break; > + } > + > + if (i == ARRAY_SIZE(degrees)) { > + dev_warn(host->dev, "All phases bad!"); > + return -EIO; > + } > + > +done: > + dev_info(host->dev, "Successfully tuned phase to %d\n", degrees[i]); > + priv->last_degree = degrees[i]; > + return 0; > +} > + > static int dw_mci_rk3288_execute_tuning(struct dw_mci_slot *slot, u32 opcode) > { > struct dw_mci *host = slot->host; > @@ -157,6 +195,13 @@ static int dw_mci_rk3288_execute_tuning(struct dw_mci_slot *slot, u32 opcode) > return -EIO; > } > > + if (priv->use_v2_tuning) { > + ret = dw_mci_v2_execute_tuning(slot, opcode); > + if (!ret) > + return 0; > + /* Otherwise we continue using fine tuning */ > + } > + > ranges = kmalloc_array(priv->num_phases / 2 + 1, > sizeof(*ranges), GFP_KERNEL); > if (!ranges) > @@ -277,6 +322,10 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci *host) > &priv->default_sample_phase)) > priv->default_sample_phase = 0; > > + priv->use_v2_tuning = > + of_device_is_compatible(host->dev->of_node, > + "rockchip,rk3576-dw-mshc"); > + v2 is a kind of software decision instead of hardware dependency. So in theory, any SoC can claim to use it via DT. > priv->drv_clk = devm_clk_get(host->dev, "ciu-drive"); > if (IS_ERR(priv->drv_clk)) > dev_dbg(host->dev, "ciu-drive not available\n");
Am Donnerstag, 15. August 2024, 02:55:37 CEST schrieb Shawn Lin: > Hi Detlev > > 在 2024/8/15 6:34, Detlev Casanova 写道: > > From: Shawn Lin <shawn.lin@rock-chips.com> > > > > v2 tuning will inherit pre-stage loader's phase settings for the first > > time, and do re-tune if necessary. > > Re-tune will still try the rough degrees, for instance, 90, 180, 270, > > 360 but continue to do the fine tuning if sample window isn't good > > enough. > > > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com> > > @@ -277,6 +322,10 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci *host) > > &priv->default_sample_phase)) > > priv->default_sample_phase = 0; > > > > + priv->use_v2_tuning = > > + of_device_is_compatible(host->dev->of_node, > > + "rockchip,rk3576-dw-mshc"); > > + > > v2 is a kind of software decision instead of hardware dependency. > So in theory, any SoC can claim to use it via DT. which actually makes it unsuitable for dt. Devicetree describes hardware-properties and should _not_ be used for software configuration. From the comment above, I assume the rk3576 does not need that feature and can just work with the regular tuning? So there are two routes for the immediate future: (1) rk3576 _needs_ that feature, then going with the compatible is fine (2) rk3576 does not need absolutely need that feature, then I'd expect the basic rk3576 to first come without, as I'd expect a lot more explanation on why it is actually needed, and which cases it does improve. The commit message does not really explain that much about why this is a great/needed feature and which areas it does improve. Heiko
On Wednesday, 14 August 2024 20:55:37 EDT Shawn Lin wrote: > Hi Detlev > > 在 2024/8/15 6:34, Detlev Casanova 写道: > > From: Shawn Lin <shawn.lin@rock-chips.com> > > > > v2 tuning will inherit pre-stage loader's phase settings for the first > > time, and do re-tune if necessary. > > Re-tune will still try the rough degrees, for instance, 90, 180, 270, > > 360 but continue to do the fine tuning if sample window isn't good > > enough. > > > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com> > > --- > > > > drivers/mmc/host/dw_mmc-rockchip.c | 49 ++++++++++++++++++++++++++++++ > > 1 file changed, 49 insertions(+) > > > > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c > > b/drivers/mmc/host/dw_mmc-rockchip.c index b07190ba4b7ac..367633f4e8892 > > 100644 > > --- a/drivers/mmc/host/dw_mmc-rockchip.c > > +++ b/drivers/mmc/host/dw_mmc-rockchip.c [...] > > > > priv->default_sample_phase = 0; > > > > + priv->use_v2_tuning = > > + of_device_is_compatible(host->dev->of_node, > > + "rockchip,rk3576-dw- mshc"); > > + > > v2 is a kind of software decision instead of hardware dependency. > So in theory, any SoC can claim to use it via DT. Yes but from my tests, only rk3576 won't work without it. So it makes sense to only use v2 for this SoC (and other future ones not supported yet) > > > priv->drv_clk = devm_clk_get(host->dev, "ciu-drive"); > > if (IS_ERR(priv->drv_clk)) > > > > dev_dbg(host->dev, "ciu-drive not available\n"); Detlev.
Am Donnerstag, 15. August 2024, 15:23:40 CEST schrieb Detlev Casanova: > On Wednesday, 14 August 2024 20:55:37 EDT Shawn Lin wrote: > > Hi Detlev > > > > 在 2024/8/15 6:34, Detlev Casanova 写道: > > > From: Shawn Lin <shawn.lin@rock-chips.com> > > > > > > v2 tuning will inherit pre-stage loader's phase settings for the first > > > time, and do re-tune if necessary. > > > Re-tune will still try the rough degrees, for instance, 90, 180, 270, > > > 360 but continue to do the fine tuning if sample window isn't good > > > enough. > > > > > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > > > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com> > > > --- > > > > > > drivers/mmc/host/dw_mmc-rockchip.c | 49 ++++++++++++++++++++++++++++++ > > > 1 file changed, 49 insertions(+) > > > > > > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c > > > b/drivers/mmc/host/dw_mmc-rockchip.c index b07190ba4b7ac..367633f4e8892 > > > 100644 > > > --- a/drivers/mmc/host/dw_mmc-rockchip.c > > > +++ b/drivers/mmc/host/dw_mmc-rockchip.c > > [...] > > > > > > > priv->default_sample_phase = 0; > > > > > > + priv->use_v2_tuning = > > > + of_device_is_compatible(host->dev->of_node, > > > + "rockchip,rk3576-dw- > mshc"); > > > + > > > > v2 is a kind of software decision instead of hardware dependency. > > So in theory, any SoC can claim to use it via DT. > > Yes but from my tests, only rk3576 won't work without it. So it makes sense to > only use v2 for this SoC (and other future ones not supported yet) Good know and thanks for testing that scenario. If you go with my suggestion from patch3 and separate the rk3576 from the original rk3288 type, you can even just assume this v2-tuning for all those types (rk3576 only so far) > > > priv->drv_clk = devm_clk_get(host->dev, "ciu-drive"); > > > if (IS_ERR(priv->drv_clk)) > > > > > > dev_dbg(host->dev, "ciu-drive not available\n"); > > > Detlev. > > >
Hi Heiko, 在 2024/8/15 21:17, Heiko Stübner 写道: > Am Donnerstag, 15. August 2024, 02:55:37 CEST schrieb Shawn Lin: >> Hi Detlev >> >> 在 2024/8/15 6:34, Detlev Casanova 写道: >>> From: Shawn Lin <shawn.lin@rock-chips.com> >>> >>> v2 tuning will inherit pre-stage loader's phase settings for the first >>> time, and do re-tune if necessary. >>> Re-tune will still try the rough degrees, for instance, 90, 180, 270, >>> 360 but continue to do the fine tuning if sample window isn't good >>> enough. >>> >>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com> > >>> @@ -277,6 +322,10 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci *host) >>> &priv->default_sample_phase)) >>> priv->default_sample_phase = 0; >>> >>> + priv->use_v2_tuning = >>> + of_device_is_compatible(host->dev->of_node, >>> + "rockchip,rk3576-dw-mshc"); >>> + >> >> v2 is a kind of software decision instead of hardware dependency. >> So in theory, any SoC can claim to use it via DT. > > which actually makes it unsuitable for dt. > Understood. > Devicetree describes hardware-properties and should _not_ be used for > software configuration. > > From the comment above, I assume the rk3576 does not need that feature > and can just work with the regular tuning? Yep, your are right. > > So there are two routes for the immediate future: > (1) rk3576 _needs_ that feature, then going with the compatible is fine > > (2) rk3576 does not need absolutely need that feature, then I'd expect > the basic rk3576 to first come without, as I'd expect a lot more explanation > on why it is actually needed, and which cases it does improve. > The commit message does not really explain that much about why this > is a great/needed feature and which areas it does improve. > Vote for the 2nd. rk3576 just need [PATCH v3 3/3] mmc: dw_mmc-rockchip: Add internal phase support > > Heiko > >
在 2024/8/15 21:23, Detlev Casanova 写道: > On Wednesday, 14 August 2024 20:55:37 EDT Shawn Lin wrote: >> Hi Detlev >> >> 在 2024/8/15 6:34, Detlev Casanova 写道: >>> From: Shawn Lin <shawn.lin@rock-chips.com> >>> >>> v2 tuning will inherit pre-stage loader's phase settings for the first >>> time, and do re-tune if necessary. >>> Re-tune will still try the rough degrees, for instance, 90, 180, 270, >>> 360 but continue to do the fine tuning if sample window isn't good >>> enough. >>> >>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com> >>> --- >>> >>> drivers/mmc/host/dw_mmc-rockchip.c | 49 ++++++++++++++++++++++++++++++ >>> 1 file changed, 49 insertions(+) >>> >>> diff --git a/drivers/mmc/host/dw_mmc-rockchip.c >>> b/drivers/mmc/host/dw_mmc-rockchip.c index b07190ba4b7ac..367633f4e8892 >>> 100644 >>> --- a/drivers/mmc/host/dw_mmc-rockchip.c >>> +++ b/drivers/mmc/host/dw_mmc-rockchip.c > > [...] > >>> >>> priv->default_sample_phase = 0; >>> >>> + priv->use_v2_tuning = >>> + of_device_is_compatible(host->dev->of_node, >>> + "rockchip,rk3576-dw- > mshc"); >>> + >> >> v2 is a kind of software decision instead of hardware dependency. >> So in theory, any SoC can claim to use it via DT. > > Yes but from my tests, only rk3576 won't work without it. So it makes sense to > only use v2 for this SoC (and other future ones not supported yet) > However from both of the IC design POV and the test from my side, we just need internal phase support patch, and rk3576 could work. >> >>> priv->drv_clk = devm_clk_get(host->dev, "ciu-drive"); >>> if (IS_ERR(priv->drv_clk)) >>> >>> dev_dbg(host->dev, "ciu-drive not available\n"); > > > Detlev. > >
diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c index b07190ba4b7ac..367633f4e8892 100644 --- a/drivers/mmc/host/dw_mmc-rockchip.c +++ b/drivers/mmc/host/dw_mmc-rockchip.c @@ -24,6 +24,8 @@ struct dw_mci_rockchip_priv_data { struct clk *sample_clk; int default_sample_phase; int num_phases; + bool use_v2_tuning; + int last_degree; }; static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios) @@ -134,6 +136,42 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios) #define TUNING_ITERATION_TO_PHASE(i, num_phases) \ (DIV_ROUND_UP((i) * 360, num_phases)) +static int dw_mci_v2_execute_tuning(struct dw_mci_slot *slot, u32 opcode) +{ + struct dw_mci *host = slot->host; + struct dw_mci_rockchip_priv_data *priv = host->priv; + struct mmc_host *mmc = slot->mmc; + u32 degrees[4] = {90, 180, 270, 360}; + int i; + static bool inherit = true; + + if (inherit) { + inherit = false; + i = clk_get_phase(priv->sample_clk) / 90 - 1; + goto done; + } + + /* v2 only support 4 degrees in theory */ + for (i = 0; i < ARRAY_SIZE(degrees); i++) { + if (degrees[i] == priv->last_degree) + continue; + + clk_set_phase(priv->sample_clk, degrees[i]); + if (!mmc_send_tuning(mmc, opcode, NULL)) + break; + } + + if (i == ARRAY_SIZE(degrees)) { + dev_warn(host->dev, "All phases bad!"); + return -EIO; + } + +done: + dev_info(host->dev, "Successfully tuned phase to %d\n", degrees[i]); + priv->last_degree = degrees[i]; + return 0; +} + static int dw_mci_rk3288_execute_tuning(struct dw_mci_slot *slot, u32 opcode) { struct dw_mci *host = slot->host; @@ -157,6 +195,13 @@ static int dw_mci_rk3288_execute_tuning(struct dw_mci_slot *slot, u32 opcode) return -EIO; } + if (priv->use_v2_tuning) { + ret = dw_mci_v2_execute_tuning(slot, opcode); + if (!ret) + return 0; + /* Otherwise we continue using fine tuning */ + } + ranges = kmalloc_array(priv->num_phases / 2 + 1, sizeof(*ranges), GFP_KERNEL); if (!ranges) @@ -277,6 +322,10 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci *host) &priv->default_sample_phase)) priv->default_sample_phase = 0; + priv->use_v2_tuning = + of_device_is_compatible(host->dev->of_node, + "rockchip,rk3576-dw-mshc"); + priv->drv_clk = devm_clk_get(host->dev, "ciu-drive"); if (IS_ERR(priv->drv_clk)) dev_dbg(host->dev, "ciu-drive not available\n");