The Rockchip ciu clock cannot be set as low as the dw-mmc hardware
supports. This leads to a situation during card initialization where the
ciu clock is set lower than the clock driver can support. The
dw-mmc-rockchip driver spews errors when this happens.
For normal operation this only happens a few times during boot, but when
cd-broken is enabled (in cases such as the SoQuartz module) this fires
multiple times each poll cycle.
Fix this by testing the minimum frequency the clock driver can support
that is within the mmc specification, then divide that by the internal
clock divider. Set the f_min frequency to this value, or if it fails,
set f_min to the downstream driver's default.
Fixes: f629ba2c04c9 ("mmc: dw_mmc: add support for RK3288")
Signed-off-by: Peter Geis <[email protected]>
---
drivers/mmc/host/dw_mmc-rockchip.c | 31 ++++++++++++++++++++++++++----
1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
index 95d0ec0f5f3a..c198590cd74a 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -15,7 +15,9 @@
#include "dw_mmc.h"
#include "dw_mmc-pltfm.h"
-#define RK3288_CLKGEN_DIV 2
+#define RK3288_CLKGEN_DIV 2
+#define RK3288_MIN_INIT_FREQ 375000
+#define MMC_MAX_INIT_FREQ 400000
struct dw_mci_rockchip_priv_data {
struct clk *drv_clk;
@@ -27,6 +29,7 @@ struct dw_mci_rockchip_priv_data {
static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
{
struct dw_mci_rockchip_priv_data *priv = host->priv;
+ struct mmc_host *mmc = mmc_from_priv(host);
int ret;
unsigned int cclkin;
u32 bus_hz;
@@ -34,6 +37,10 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
if (ios->clock == 0)
return;
+ /* the clock will fail if below the f_min rate */
+ if (ios->clock < mmc->f_min)
+ ios->clock = mmc->f_min;
+
/*
* cclkin: source clock of mmc controller
* bus_hz: card interface clock generated by CLKGEN
@@ -51,7 +58,7 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
ret = clk_set_rate(host->ciu_clk, cclkin);
if (ret)
- dev_warn(host->dev, "failed to set rate %uHz\n", ios->clock);
+ dev_warn(host->dev, "failed to set rate %uHz err: %d\n", cclkin, ret);
bus_hz = clk_get_rate(host->ciu_clk) / RK3288_CLKGEN_DIV;
if (bus_hz != host->bus_hz) {
@@ -290,13 +297,29 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci *host)
static int dw_mci_rockchip_init(struct dw_mci *host)
{
+ struct mmc_host *mmc = mmc_from_priv(host);
+ int ret;
+
/* It is slot 8 on Rockchip SoCs */
host->sdio_id0 = 8;
- if (of_device_is_compatible(host->dev->of_node,
- "rockchip,rk3288-dw-mshc"))
+ if (of_device_is_compatible(host->dev->of_node, "rockchip,rk3288-dw-mshc")) {
host->bus_hz /= RK3288_CLKGEN_DIV;
+ /* clock driver will fail if the clock is less than the lowest source clock
+ * divided by the internal clock divider. Test for the lowest available
+ * clock and set the f_min freq to clock / clock divider. If we fail, set
+ * it to the downstream hardcoded value.
+ */
+ ret = clk_round_rate(host->ciu_clk, MMC_MAX_INIT_FREQ * RK3288_CLKGEN_DIV);
+ if (ret < 0) {
+ dev_warn(host->dev, "mmc safe rate failed: %d\n", ret);
+ mmc->f_min = RK3288_MIN_INIT_FREQ;
+ } else {
+ mmc->f_min = ret / RK3288_CLKGEN_DIV;
+ }
+ }
+
return 0;
}
--
2.25.1
On Thu, 3 Mar 2022 at 02:52, Peter Geis <[email protected]> wrote:
>
> The Rockchip ciu clock cannot be set as low as the dw-mmc hardware
> supports. This leads to a situation during card initialization where the
> ciu clock is set lower than the clock driver can support. The
> dw-mmc-rockchip driver spews errors when this happens.
> For normal operation this only happens a few times during boot, but when
> cd-broken is enabled (in cases such as the SoQuartz module) this fires
> multiple times each poll cycle.
>
> Fix this by testing the minimum frequency the clock driver can support
> that is within the mmc specification, then divide that by the internal
> clock divider. Set the f_min frequency to this value, or if it fails,
> set f_min to the downstream driver's default.
>
> Fixes: f629ba2c04c9 ("mmc: dw_mmc: add support for RK3288")
>
> Signed-off-by: Peter Geis <[email protected]>
> ---
> drivers/mmc/host/dw_mmc-rockchip.c | 31 ++++++++++++++++++++++++++----
> 1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
> index 95d0ec0f5f3a..c198590cd74a 100644
> --- a/drivers/mmc/host/dw_mmc-rockchip.c
> +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> @@ -15,7 +15,9 @@
> #include "dw_mmc.h"
> #include "dw_mmc-pltfm.h"
>
> -#define RK3288_CLKGEN_DIV 2
> +#define RK3288_CLKGEN_DIV 2
> +#define RK3288_MIN_INIT_FREQ 375000
> +#define MMC_MAX_INIT_FREQ 400000
>
> struct dw_mci_rockchip_priv_data {
> struct clk *drv_clk;
> @@ -27,6 +29,7 @@ struct dw_mci_rockchip_priv_data {
> static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> {
> struct dw_mci_rockchip_priv_data *priv = host->priv;
> + struct mmc_host *mmc = mmc_from_priv(host);
> int ret;
> unsigned int cclkin;
> u32 bus_hz;
> @@ -34,6 +37,10 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> if (ios->clock == 0)
> return;
>
> + /* the clock will fail if below the f_min rate */
> + if (ios->clock < mmc->f_min)
> + ios->clock = mmc->f_min;
> +
You shouldn't need this. The mmc core should manage this already.
> /*
> * cclkin: source clock of mmc controller
> * bus_hz: card interface clock generated by CLKGEN
> @@ -51,7 +58,7 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>
> ret = clk_set_rate(host->ciu_clk, cclkin);
> if (ret)
> - dev_warn(host->dev, "failed to set rate %uHz\n", ios->clock);
> + dev_warn(host->dev, "failed to set rate %uHz err: %d\n", cclkin, ret);
>
> bus_hz = clk_get_rate(host->ciu_clk) / RK3288_CLKGEN_DIV;
> if (bus_hz != host->bus_hz) {
> @@ -290,13 +297,29 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci *host)
>
> static int dw_mci_rockchip_init(struct dw_mci *host)
> {
> + struct mmc_host *mmc = mmc_from_priv(host);
> + int ret;
> +
> /* It is slot 8 on Rockchip SoCs */
> host->sdio_id0 = 8;
>
> - if (of_device_is_compatible(host->dev->of_node,
> - "rockchip,rk3288-dw-mshc"))
> + if (of_device_is_compatible(host->dev->of_node, "rockchip,rk3288-dw-mshc")) {
> host->bus_hz /= RK3288_CLKGEN_DIV;
>
> + /* clock driver will fail if the clock is less than the lowest source clock
> + * divided by the internal clock divider. Test for the lowest available
> + * clock and set the f_min freq to clock / clock divider. If we fail, set
> + * it to the downstream hardcoded value.
> + */
> + ret = clk_round_rate(host->ciu_clk, MMC_MAX_INIT_FREQ * RK3288_CLKGEN_DIV);
> + if (ret < 0) {
> + dev_warn(host->dev, "mmc safe rate failed: %d\n", ret);
> + mmc->f_min = RK3288_MIN_INIT_FREQ;
> + } else {
> + mmc->f_min = ret / RK3288_CLKGEN_DIV;
> + }
> + }
> +
> return 0;
> }
>
Kind regards
Uffe
On Thu, Mar 3, 2022 at 2:53 AM Ulf Hansson <[email protected]> wrote:
>
> On Thu, 3 Mar 2022 at 02:52, Peter Geis <[email protected]> wrote:
> >
> > The Rockchip ciu clock cannot be set as low as the dw-mmc hardware
> > supports. This leads to a situation during card initialization where the
> > ciu clock is set lower than the clock driver can support. The
> > dw-mmc-rockchip driver spews errors when this happens.
> > For normal operation this only happens a few times during boot, but when
> > cd-broken is enabled (in cases such as the SoQuartz module) this fires
> > multiple times each poll cycle.
> >
> > Fix this by testing the minimum frequency the clock driver can support
> > that is within the mmc specification, then divide that by the internal
> > clock divider. Set the f_min frequency to this value, or if it fails,
> > set f_min to the downstream driver's default.
> >
> > Fixes: f629ba2c04c9 ("mmc: dw_mmc: add support for RK3288")
> >
> > Signed-off-by: Peter Geis <[email protected]>
> > ---
> > drivers/mmc/host/dw_mmc-rockchip.c | 31 ++++++++++++++++++++++++++----
> > 1 file changed, 27 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
> > index 95d0ec0f5f3a..c198590cd74a 100644
> > --- a/drivers/mmc/host/dw_mmc-rockchip.c
> > +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> > @@ -15,7 +15,9 @@
> > #include "dw_mmc.h"
> > #include "dw_mmc-pltfm.h"
> >
> > -#define RK3288_CLKGEN_DIV 2
> > +#define RK3288_CLKGEN_DIV 2
> > +#define RK3288_MIN_INIT_FREQ 375000
> > +#define MMC_MAX_INIT_FREQ 400000
> >
> > struct dw_mci_rockchip_priv_data {
> > struct clk *drv_clk;
> > @@ -27,6 +29,7 @@ struct dw_mci_rockchip_priv_data {
> > static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> > {
> > struct dw_mci_rockchip_priv_data *priv = host->priv;
> > + struct mmc_host *mmc = mmc_from_priv(host);
> > int ret;
> > unsigned int cclkin;
> > u32 bus_hz;
> > @@ -34,6 +37,10 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> > if (ios->clock == 0)
> > return;
> >
> > + /* the clock will fail if below the f_min rate */
> > + if (ios->clock < mmc->f_min)
> > + ios->clock = mmc->f_min;
> > +
>
> You shouldn't need this. The mmc core should manage this already.
I thought so too, but while setting f_min did reduce the number of
errors, it didn't stop them completely.
Each tick I was getting three failures, it turns out mmc core tries
anyways with 300000, 200000, and 100000.
Clamping it here was necessary to stop these.
>
> > /*
> > * cclkin: source clock of mmc controller
> > * bus_hz: card interface clock generated by CLKGEN
> > @@ -51,7 +58,7 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> >
> > ret = clk_set_rate(host->ciu_clk, cclkin);
> > if (ret)
> > - dev_warn(host->dev, "failed to set rate %uHz\n", ios->clock);
> > + dev_warn(host->dev, "failed to set rate %uHz err: %d\n", cclkin, ret);
> >
> > bus_hz = clk_get_rate(host->ciu_clk) / RK3288_CLKGEN_DIV;
> > if (bus_hz != host->bus_hz) {
> > @@ -290,13 +297,29 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci *host)
> >
> > static int dw_mci_rockchip_init(struct dw_mci *host)
> > {
> > + struct mmc_host *mmc = mmc_from_priv(host);
> > + int ret;
> > +
> > /* It is slot 8 on Rockchip SoCs */
> > host->sdio_id0 = 8;
> >
> > - if (of_device_is_compatible(host->dev->of_node,
> > - "rockchip,rk3288-dw-mshc"))
> > + if (of_device_is_compatible(host->dev->of_node, "rockchip,rk3288-dw-mshc")) {
> > host->bus_hz /= RK3288_CLKGEN_DIV;
> >
> > + /* clock driver will fail if the clock is less than the lowest source clock
> > + * divided by the internal clock divider. Test for the lowest available
> > + * clock and set the f_min freq to clock / clock divider. If we fail, set
> > + * it to the downstream hardcoded value.
> > + */
> > + ret = clk_round_rate(host->ciu_clk, MMC_MAX_INIT_FREQ * RK3288_CLKGEN_DIV);
> > + if (ret < 0) {
> > + dev_warn(host->dev, "mmc safe rate failed: %d\n", ret);
> > + mmc->f_min = RK3288_MIN_INIT_FREQ;
> > + } else {
> > + mmc->f_min = ret / RK3288_CLKGEN_DIV;
> > + }
> > + }
> > +
> > return 0;
> > }
> >
>
> Kind regards
> Uffe
On Thu, 3 Mar 2022 at 10:49, Peter Geis <[email protected]> wrote:
>
> On Thu, Mar 3, 2022 at 2:53 AM Ulf Hansson <[email protected]> wrote:
> >
> > On Thu, 3 Mar 2022 at 02:52, Peter Geis <[email protected]> wrote:
> > >
> > > The Rockchip ciu clock cannot be set as low as the dw-mmc hardware
> > > supports. This leads to a situation during card initialization where the
> > > ciu clock is set lower than the clock driver can support. The
> > > dw-mmc-rockchip driver spews errors when this happens.
> > > For normal operation this only happens a few times during boot, but when
> > > cd-broken is enabled (in cases such as the SoQuartz module) this fires
> > > multiple times each poll cycle.
> > >
> > > Fix this by testing the minimum frequency the clock driver can support
> > > that is within the mmc specification, then divide that by the internal
> > > clock divider. Set the f_min frequency to this value, or if it fails,
> > > set f_min to the downstream driver's default.
> > >
> > > Fixes: f629ba2c04c9 ("mmc: dw_mmc: add support for RK3288")
> > >
> > > Signed-off-by: Peter Geis <[email protected]>
> > > ---
> > > drivers/mmc/host/dw_mmc-rockchip.c | 31 ++++++++++++++++++++++++++----
> > > 1 file changed, 27 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
> > > index 95d0ec0f5f3a..c198590cd74a 100644
> > > --- a/drivers/mmc/host/dw_mmc-rockchip.c
> > > +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> > > @@ -15,7 +15,9 @@
> > > #include "dw_mmc.h"
> > > #include "dw_mmc-pltfm.h"
> > >
> > > -#define RK3288_CLKGEN_DIV 2
> > > +#define RK3288_CLKGEN_DIV 2
> > > +#define RK3288_MIN_INIT_FREQ 375000
> > > +#define MMC_MAX_INIT_FREQ 400000
> > >
> > > struct dw_mci_rockchip_priv_data {
> > > struct clk *drv_clk;
> > > @@ -27,6 +29,7 @@ struct dw_mci_rockchip_priv_data {
> > > static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> > > {
> > > struct dw_mci_rockchip_priv_data *priv = host->priv;
> > > + struct mmc_host *mmc = mmc_from_priv(host);
> > > int ret;
> > > unsigned int cclkin;
> > > u32 bus_hz;
> > > @@ -34,6 +37,10 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> > > if (ios->clock == 0)
> > > return;
> > >
> > > + /* the clock will fail if below the f_min rate */
> > > + if (ios->clock < mmc->f_min)
> > > + ios->clock = mmc->f_min;
> > > +
> >
> > You shouldn't need this. The mmc core should manage this already.
>
> I thought so too, but while setting f_min did reduce the number of
> errors, it didn't stop them completely.
> Each tick I was getting three failures, it turns out mmc core tries
> anyways with 300000, 200000, and 100000.
> Clamping it here was necessary to stop these.
Ohh, that was certainly a surprise to me. Unless the dw_mmc driver
invokes this path on it's own in some odd way, that means the mmc core
has a bug that we need to fix.
Would you mind taking a stack trace or debug this so we understand in
what case the mmc core doesn't respect f_min? It really should.
[...]
Kind regards
Uffe
On 2022-03-03 01:51, Peter Geis wrote:
> The Rockchip ciu clock cannot be set as low as the dw-mmc hardware
> supports.
Isn't that specific to RK3568, per the downstream fix? Certainly my
RK3399 has no problem if I simulate polling by plugging in an empty adapter:
[499969.392530] mmc_host mmc1: Bus speed (slot 0) = 400000Hz (slot req
400000Hz, actual 400000HZ div = 0)
[499969.467709] mmc_host mmc1: Bus speed (slot 0) = 300000Hz (slot req
300000Hz, actual 300000HZ div = 0)
[499969.545975] mmc_host mmc1: Bus speed (slot 0) = 200000Hz (slot req
200000Hz, actual 200000HZ div = 0)
[499969.629824] mmc_host mmc1: Bus speed (slot 0) = 100000Hz (slot req
100000Hz, actual 100000HZ div = 0)
Even though every card within arm's reach of my desk does seem happy to
identify at 400KHz, there are apparently some that only work towards the
lower end of the range, so it's less than ideal to impose this
limitation where it isn't necessary.
Robin.
> This leads to a situation during card initialization where the
> ciu clock is set lower than the clock driver can support. The
> dw-mmc-rockchip driver spews errors when this happens.
> For normal operation this only happens a few times during boot, but when
> cd-broken is enabled (in cases such as the SoQuartz module) this fires
> multiple times each poll cycle.
>
> Fix this by testing the minimum frequency the clock driver can support
> that is within the mmc specification, then divide that by the internal
> clock divider. Set the f_min frequency to this value, or if it fails,
> set f_min to the downstream driver's default.
>
> Fixes: f629ba2c04c9 ("mmc: dw_mmc: add support for RK3288")
>
> Signed-off-by: Peter Geis <[email protected]>
> ---
> drivers/mmc/host/dw_mmc-rockchip.c | 31 ++++++++++++++++++++++++++----
> 1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
> index 95d0ec0f5f3a..c198590cd74a 100644
> --- a/drivers/mmc/host/dw_mmc-rockchip.c
> +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> @@ -15,7 +15,9 @@
> #include "dw_mmc.h"
> #include "dw_mmc-pltfm.h"
>
> -#define RK3288_CLKGEN_DIV 2
> +#define RK3288_CLKGEN_DIV 2
> +#define RK3288_MIN_INIT_FREQ 375000
> +#define MMC_MAX_INIT_FREQ 400000
>
> struct dw_mci_rockchip_priv_data {
> struct clk *drv_clk;
> @@ -27,6 +29,7 @@ struct dw_mci_rockchip_priv_data {
> static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> {
> struct dw_mci_rockchip_priv_data *priv = host->priv;
> + struct mmc_host *mmc = mmc_from_priv(host);
> int ret;
> unsigned int cclkin;
> u32 bus_hz;
> @@ -34,6 +37,10 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> if (ios->clock == 0)
> return;
>
> + /* the clock will fail if below the f_min rate */
> + if (ios->clock < mmc->f_min)
> + ios->clock = mmc->f_min;
> +
> /*
> * cclkin: source clock of mmc controller
> * bus_hz: card interface clock generated by CLKGEN
> @@ -51,7 +58,7 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>
> ret = clk_set_rate(host->ciu_clk, cclkin);
> if (ret)
> - dev_warn(host->dev, "failed to set rate %uHz\n", ios->clock);
> + dev_warn(host->dev, "failed to set rate %uHz err: %d\n", cclkin, ret);
>
> bus_hz = clk_get_rate(host->ciu_clk) / RK3288_CLKGEN_DIV;
> if (bus_hz != host->bus_hz) {
> @@ -290,13 +297,29 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci *host)
>
> static int dw_mci_rockchip_init(struct dw_mci *host)
> {
> + struct mmc_host *mmc = mmc_from_priv(host);
> + int ret;
> +
> /* It is slot 8 on Rockchip SoCs */
> host->sdio_id0 = 8;
>
> - if (of_device_is_compatible(host->dev->of_node,
> - "rockchip,rk3288-dw-mshc"))
> + if (of_device_is_compatible(host->dev->of_node, "rockchip,rk3288-dw-mshc")) {
> host->bus_hz /= RK3288_CLKGEN_DIV;
>
> + /* clock driver will fail if the clock is less than the lowest source clock
> + * divided by the internal clock divider. Test for the lowest available
> + * clock and set the f_min freq to clock / clock divider. If we fail, set
> + * it to the downstream hardcoded value.
> + */
> + ret = clk_round_rate(host->ciu_clk, MMC_MAX_INIT_FREQ * RK3288_CLKGEN_DIV);
> + if (ret < 0) {
> + dev_warn(host->dev, "mmc safe rate failed: %d\n", ret);
> + mmc->f_min = RK3288_MIN_INIT_FREQ;
> + } else {
> + mmc->f_min = ret / RK3288_CLKGEN_DIV;
> + }
> + }
> +
> return 0;
> }
>
On 2022-03-03 10:21, Ulf Hansson wrote:
> On Thu, 3 Mar 2022 at 10:49, Peter Geis <[email protected]> wrote:
>>
>> On Thu, Mar 3, 2022 at 2:53 AM Ulf Hansson <[email protected]> wrote:
>>>
>>> On Thu, 3 Mar 2022 at 02:52, Peter Geis <[email protected]> wrote:
>>>>
>>>> The Rockchip ciu clock cannot be set as low as the dw-mmc hardware
>>>> supports. This leads to a situation during card initialization where the
>>>> ciu clock is set lower than the clock driver can support. The
>>>> dw-mmc-rockchip driver spews errors when this happens.
>>>> For normal operation this only happens a few times during boot, but when
>>>> cd-broken is enabled (in cases such as the SoQuartz module) this fires
>>>> multiple times each poll cycle.
>>>>
>>>> Fix this by testing the minimum frequency the clock driver can support
>>>> that is within the mmc specification, then divide that by the internal
>>>> clock divider. Set the f_min frequency to this value, or if it fails,
>>>> set f_min to the downstream driver's default.
>>>>
>>>> Fixes: f629ba2c04c9 ("mmc: dw_mmc: add support for RK3288")
>>>>
>>>> Signed-off-by: Peter Geis <[email protected]>
>>>> ---
>>>> drivers/mmc/host/dw_mmc-rockchip.c | 31 ++++++++++++++++++++++++++----
>>>> 1 file changed, 27 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
>>>> index 95d0ec0f5f3a..c198590cd74a 100644
>>>> --- a/drivers/mmc/host/dw_mmc-rockchip.c
>>>> +++ b/drivers/mmc/host/dw_mmc-rockchip.c
>>>> @@ -15,7 +15,9 @@
>>>> #include "dw_mmc.h"
>>>> #include "dw_mmc-pltfm.h"
>>>>
>>>> -#define RK3288_CLKGEN_DIV 2
>>>> +#define RK3288_CLKGEN_DIV 2
>>>> +#define RK3288_MIN_INIT_FREQ 375000
>>>> +#define MMC_MAX_INIT_FREQ 400000
>>>>
>>>> struct dw_mci_rockchip_priv_data {
>>>> struct clk *drv_clk;
>>>> @@ -27,6 +29,7 @@ struct dw_mci_rockchip_priv_data {
>>>> static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>>>> {
>>>> struct dw_mci_rockchip_priv_data *priv = host->priv;
>>>> + struct mmc_host *mmc = mmc_from_priv(host);
>>>> int ret;
>>>> unsigned int cclkin;
>>>> u32 bus_hz;
>>>> @@ -34,6 +37,10 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>>>> if (ios->clock == 0)
>>>> return;
>>>>
>>>> + /* the clock will fail if below the f_min rate */
>>>> + if (ios->clock < mmc->f_min)
>>>> + ios->clock = mmc->f_min;
>>>> +
>>>
>>> You shouldn't need this. The mmc core should manage this already.
>>
>> I thought so too, but while setting f_min did reduce the number of
>> errors, it didn't stop them completely.
>> Each tick I was getting three failures, it turns out mmc core tries
>> anyways with 300000, 200000, and 100000.
>> Clamping it here was necessary to stop these.
>
> Ohh, that was certainly a surprise to me. Unless the dw_mmc driver
> invokes this path on it's own in some odd way, that means the mmc core
> has a bug that we need to fix.
>
> Would you mind taking a stack trace or debug this so we understand in
> what case the mmc core doesn't respect f_min? It really should.
I'm only armed with grep and a hunch, but is dw_mci_init_slot_caps()
stomping on the same f_min that we've set in the platform init hook?
Robin.
On Thu, Mar 3, 2022 at 10:19 AM Robin Murphy <[email protected]> wrote:
>
> On 2022-03-03 10:21, Ulf Hansson wrote:
> > On Thu, 3 Mar 2022 at 10:49, Peter Geis <[email protected]> wrote:
> >>
> >> On Thu, Mar 3, 2022 at 2:53 AM Ulf Hansson <[email protected]> wrote:
> >>>
> >>> On Thu, 3 Mar 2022 at 02:52, Peter Geis <[email protected]> wrote:
> >>>>
> >>>> The Rockchip ciu clock cannot be set as low as the dw-mmc hardware
> >>>> supports. This leads to a situation during card initialization where the
> >>>> ciu clock is set lower than the clock driver can support. The
> >>>> dw-mmc-rockchip driver spews errors when this happens.
> >>>> For normal operation this only happens a few times during boot, but when
> >>>> cd-broken is enabled (in cases such as the SoQuartz module) this fires
> >>>> multiple times each poll cycle.
> >>>>
> >>>> Fix this by testing the minimum frequency the clock driver can support
> >>>> that is within the mmc specification, then divide that by the internal
> >>>> clock divider. Set the f_min frequency to this value, or if it fails,
> >>>> set f_min to the downstream driver's default.
> >>>>
> >>>> Fixes: f629ba2c04c9 ("mmc: dw_mmc: add support for RK3288")
> >>>>
> >>>> Signed-off-by: Peter Geis <[email protected]>
> >>>> ---
> >>>> drivers/mmc/host/dw_mmc-rockchip.c | 31 ++++++++++++++++++++++++++----
> >>>> 1 file changed, 27 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
> >>>> index 95d0ec0f5f3a..c198590cd74a 100644
> >>>> --- a/drivers/mmc/host/dw_mmc-rockchip.c
> >>>> +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> >>>> @@ -15,7 +15,9 @@
> >>>> #include "dw_mmc.h"
> >>>> #include "dw_mmc-pltfm.h"
> >>>>
> >>>> -#define RK3288_CLKGEN_DIV 2
> >>>> +#define RK3288_CLKGEN_DIV 2
> >>>> +#define RK3288_MIN_INIT_FREQ 375000
> >>>> +#define MMC_MAX_INIT_FREQ 400000
> >>>>
> >>>> struct dw_mci_rockchip_priv_data {
> >>>> struct clk *drv_clk;
> >>>> @@ -27,6 +29,7 @@ struct dw_mci_rockchip_priv_data {
> >>>> static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> >>>> {
> >>>> struct dw_mci_rockchip_priv_data *priv = host->priv;
> >>>> + struct mmc_host *mmc = mmc_from_priv(host);
> >>>> int ret;
> >>>> unsigned int cclkin;
> >>>> u32 bus_hz;
> >>>> @@ -34,6 +37,10 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> >>>> if (ios->clock == 0)
> >>>> return;
> >>>>
> >>>> + /* the clock will fail if below the f_min rate */
> >>>> + if (ios->clock < mmc->f_min)
> >>>> + ios->clock = mmc->f_min;
> >>>> +
> >>>
> >>> You shouldn't need this. The mmc core should manage this already.
> >>
> >> I thought so too, but while setting f_min did reduce the number of
> >> errors, it didn't stop them completely.
> >> Each tick I was getting three failures, it turns out mmc core tries
> >> anyways with 300000, 200000, and 100000.
> >> Clamping it here was necessary to stop these.
> >
> > Ohh, that was certainly a surprise to me. Unless the dw_mmc driver
> > invokes this path on it's own in some odd way, that means the mmc core
> > has a bug that we need to fix.
> >
> > Would you mind taking a stack trace or debug this so we understand in
> > what case the mmc core doesn't respect f_min? It really should.
>
> I'm only armed with grep and a hunch, but is dw_mci_init_slot_caps()
> stomping on the same f_min that we've set in the platform init hook?
I suspected this originally, but no, f_min remains intact, so it's
something else.
Also, this clamp wouldn't work if f_min got clobbered.
>
> Robin.
On Thu, Mar 3, 2022 at 6:53 AM Robin Murphy <[email protected]> wrote:
>
> On 2022-03-03 01:51, Peter Geis wrote:
> > The Rockchip ciu clock cannot be set as low as the dw-mmc hardware
> > supports.
>
> Isn't that specific to RK3568, per the downstream fix? Certainly my
> RK3399 has no problem if I simulate polling by plugging in an empty adapter:
>
> [499969.392530] mmc_host mmc1: Bus speed (slot 0) = 400000Hz (slot req
> 400000Hz, actual 400000HZ div = 0)
> [499969.467709] mmc_host mmc1: Bus speed (slot 0) = 300000Hz (slot req
> 300000Hz, actual 300000HZ div = 0)
> [499969.545975] mmc_host mmc1: Bus speed (slot 0) = 200000Hz (slot req
> 200000Hz, actual 200000HZ div = 0)
> [499969.629824] mmc_host mmc1: Bus speed (slot 0) = 100000Hz (slot req
> 100000Hz, actual 100000HZ div = 0)
>
> Even though every card within arm's reach of my desk does seem happy to
> identify at 400KHz, there are apparently some that only work towards the
> lower end of the range, so it's less than ideal to impose this
> limitation where it isn't necessary.
Thanks for the insight!
In the V2 I'll change this to use the lowest supported frequency.
I'm also going to explore what's different in the rk3399 clock tree
from the rk356x clock tree.
>
> Robin.
>
> > This leads to a situation during card initialization where the
> > ciu clock is set lower than the clock driver can support. The
> > dw-mmc-rockchip driver spews errors when this happens.
> > For normal operation this only happens a few times during boot, but when
> > cd-broken is enabled (in cases such as the SoQuartz module) this fires
> > multiple times each poll cycle.
> >
> > Fix this by testing the minimum frequency the clock driver can support
> > that is within the mmc specification, then divide that by the internal
> > clock divider. Set the f_min frequency to this value, or if it fails,
> > set f_min to the downstream driver's default.
> >
> > Fixes: f629ba2c04c9 ("mmc: dw_mmc: add support for RK3288")
> >
> > Signed-off-by: Peter Geis <[email protected]>
> > ---
> > drivers/mmc/host/dw_mmc-rockchip.c | 31 ++++++++++++++++++++++++++----
> > 1 file changed, 27 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
> > index 95d0ec0f5f3a..c198590cd74a 100644
> > --- a/drivers/mmc/host/dw_mmc-rockchip.c
> > +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> > @@ -15,7 +15,9 @@
> > #include "dw_mmc.h"
> > #include "dw_mmc-pltfm.h"
> >
> > -#define RK3288_CLKGEN_DIV 2
> > +#define RK3288_CLKGEN_DIV 2
> > +#define RK3288_MIN_INIT_FREQ 375000
> > +#define MMC_MAX_INIT_FREQ 400000
> >
> > struct dw_mci_rockchip_priv_data {
> > struct clk *drv_clk;
> > @@ -27,6 +29,7 @@ struct dw_mci_rockchip_priv_data {
> > static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> > {
> > struct dw_mci_rockchip_priv_data *priv = host->priv;
> > + struct mmc_host *mmc = mmc_from_priv(host);
> > int ret;
> > unsigned int cclkin;
> > u32 bus_hz;
> > @@ -34,6 +37,10 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> > if (ios->clock == 0)
> > return;
> >
> > + /* the clock will fail if below the f_min rate */
> > + if (ios->clock < mmc->f_min)
> > + ios->clock = mmc->f_min;
> > +
> > /*
> > * cclkin: source clock of mmc controller
> > * bus_hz: card interface clock generated by CLKGEN
> > @@ -51,7 +58,7 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> >
> > ret = clk_set_rate(host->ciu_clk, cclkin);
> > if (ret)
> > - dev_warn(host->dev, "failed to set rate %uHz\n", ios->clock);
> > + dev_warn(host->dev, "failed to set rate %uHz err: %d\n", cclkin, ret);
> >
> > bus_hz = clk_get_rate(host->ciu_clk) / RK3288_CLKGEN_DIV;
> > if (bus_hz != host->bus_hz) {
> > @@ -290,13 +297,29 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci *host)
> >
> > static int dw_mci_rockchip_init(struct dw_mci *host)
> > {
> > + struct mmc_host *mmc = mmc_from_priv(host);
> > + int ret;
> > +
> > /* It is slot 8 on Rockchip SoCs */
> > host->sdio_id0 = 8;
> >
> > - if (of_device_is_compatible(host->dev->of_node,
> > - "rockchip,rk3288-dw-mshc"))
> > + if (of_device_is_compatible(host->dev->of_node, "rockchip,rk3288-dw-mshc")) {
> > host->bus_hz /= RK3288_CLKGEN_DIV;
> >
> > + /* clock driver will fail if the clock is less than the lowest source clock
> > + * divided by the internal clock divider. Test for the lowest available
> > + * clock and set the f_min freq to clock / clock divider. If we fail, set
> > + * it to the downstream hardcoded value.
> > + */
> > + ret = clk_round_rate(host->ciu_clk, MMC_MAX_INIT_FREQ * RK3288_CLKGEN_DIV);
> > + if (ret < 0) {
> > + dev_warn(host->dev, "mmc safe rate failed: %d\n", ret);
> > + mmc->f_min = RK3288_MIN_INIT_FREQ;
> > + } else {
> > + mmc->f_min = ret / RK3288_CLKGEN_DIV;
> > + }
> > + }
> > +
> > return 0;
> > }
> >
On Thu, Mar 3, 2022 at 4:28 PM Peter Geis <[email protected]> wrote:
>
> On Thu, Mar 3, 2022 at 5:21 AM Ulf Hansson <[email protected]> wrote:
> >
> > On Thu, 3 Mar 2022 at 10:49, Peter Geis <[email protected]> wrote:
> > >
> > > On Thu, Mar 3, 2022 at 2:53 AM Ulf Hansson <[email protected]> wrote:
> > > >
> > > > On Thu, 3 Mar 2022 at 02:52, Peter Geis <[email protected]> wrote:
> > > > >
> > > > > The Rockchip ciu clock cannot be set as low as the dw-mmc hardware
> > > > > supports. This leads to a situation during card initialization where the
> > > > > ciu clock is set lower than the clock driver can support. The
> > > > > dw-mmc-rockchip driver spews errors when this happens.
> > > > > For normal operation this only happens a few times during boot, but when
> > > > > cd-broken is enabled (in cases such as the SoQuartz module) this fires
> > > > > multiple times each poll cycle.
> > > > >
> > > > > Fix this by testing the minimum frequency the clock driver can support
> > > > > that is within the mmc specification, then divide that by the internal
> > > > > clock divider. Set the f_min frequency to this value, or if it fails,
> > > > > set f_min to the downstream driver's default.
> > > > >
> > > > > Fixes: f629ba2c04c9 ("mmc: dw_mmc: add support for RK3288")
> > > > >
> > > > > Signed-off-by: Peter Geis <[email protected]>
> > > > > ---
> > > > > drivers/mmc/host/dw_mmc-rockchip.c | 31 ++++++++++++++++++++++++++----
> > > > > 1 file changed, 27 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
> > > > > index 95d0ec0f5f3a..c198590cd74a 100644
> > > > > --- a/drivers/mmc/host/dw_mmc-rockchip.c
> > > > > +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> > > > > @@ -15,7 +15,9 @@
> > > > > #include "dw_mmc.h"
> > > > > #include "dw_mmc-pltfm.h"
> > > > >
> > > > > -#define RK3288_CLKGEN_DIV 2
> > > > > +#define RK3288_CLKGEN_DIV 2
> > > > > +#define RK3288_MIN_INIT_FREQ 375000
> > > > > +#define MMC_MAX_INIT_FREQ 400000
> > > > >
> > > > > struct dw_mci_rockchip_priv_data {
> > > > > struct clk *drv_clk;
> > > > > @@ -27,6 +29,7 @@ struct dw_mci_rockchip_priv_data {
> > > > > static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> > > > > {
> > > > > struct dw_mci_rockchip_priv_data *priv = host->priv;
> > > > > + struct mmc_host *mmc = mmc_from_priv(host);
> > > > > int ret;
> > > > > unsigned int cclkin;
> > > > > u32 bus_hz;
> > > > > @@ -34,6 +37,10 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> > > > > if (ios->clock == 0)
> > > > > return;
> > > > >
> > > > > + /* the clock will fail if below the f_min rate */
> > > > > + if (ios->clock < mmc->f_min)
> > > > > + ios->clock = mmc->f_min;
> > > > > +
> > > >
> > > > You shouldn't need this. The mmc core should manage this already.
> > >
> > > I thought so too, but while setting f_min did reduce the number of
> > > errors, it didn't stop them completely.
> > > Each tick I was getting three failures, it turns out mmc core tries
> > > anyways with 300000, 200000, and 100000.
> > > Clamping it here was necessary to stop these.
> >
> > Ohh, that was certainly a surprise to me. Unless the dw_mmc driver
> > invokes this path on it's own in some odd way, that means the mmc core
> > has a bug that we need to fix.
> >
> > Would you mind taking a stack trace or debug this so we understand in
> > what case the mmc core doesn't respect f_min? It really should.
>
> I thought it was odd too, will check into where it's happening.
> Thanks!
[ 11.376608] Hardware name: Pine64 RK3566 Quartz64-A Board (DT)
[ 11.377127] Workqueue: events_freezable mmc_rescan
[ 11.377567] Call trace:
[ 11.377788] dump_backtrace.part.0+0xd8/0xe4
[ 11.378177] show_stack+0x24/0x80
[ 11.378479] dump_stack_lvl+0x68/0x84
[ 11.378812] dump_stack+0x1c/0x38
[ 11.379111] dw_mci_rk3288_set_ios+0x128/0x150
[ 11.379512] dw_mci_set_ios+0xb0/0x280
[ 11.379849] mmc_power_up.part.0+0xd0/0x17c
[ 11.380225] mmc_rescan+0x184/0x2f0
[ 11.380538] process_one_work+0x1e0/0x48c
[ 11.380901] worker_thread+0x148/0x46c
[ 11.381238] kthread+0x100/0x110
[ 11.381530] ret_from_fork+0x10/0x20
Seems to be happening here:
https://elixir.bootlin.com/linux/latest/source/drivers/mmc/core/core.c#L2233
But it should be guarded.
I'm continuing to dig into it.
>
> >
> > [...]
> >
> > Kind regards
> > Uffe
On Thu, Mar 3, 2022 at 5:21 AM Ulf Hansson <[email protected]> wrote:
>
> On Thu, 3 Mar 2022 at 10:49, Peter Geis <[email protected]> wrote:
> >
> > On Thu, Mar 3, 2022 at 2:53 AM Ulf Hansson <[email protected]> wrote:
> > >
> > > On Thu, 3 Mar 2022 at 02:52, Peter Geis <[email protected]> wrote:
> > > >
> > > > The Rockchip ciu clock cannot be set as low as the dw-mmc hardware
> > > > supports. This leads to a situation during card initialization where the
> > > > ciu clock is set lower than the clock driver can support. The
> > > > dw-mmc-rockchip driver spews errors when this happens.
> > > > For normal operation this only happens a few times during boot, but when
> > > > cd-broken is enabled (in cases such as the SoQuartz module) this fires
> > > > multiple times each poll cycle.
> > > >
> > > > Fix this by testing the minimum frequency the clock driver can support
> > > > that is within the mmc specification, then divide that by the internal
> > > > clock divider. Set the f_min frequency to this value, or if it fails,
> > > > set f_min to the downstream driver's default.
> > > >
> > > > Fixes: f629ba2c04c9 ("mmc: dw_mmc: add support for RK3288")
> > > >
> > > > Signed-off-by: Peter Geis <[email protected]>
> > > > ---
> > > > drivers/mmc/host/dw_mmc-rockchip.c | 31 ++++++++++++++++++++++++++----
> > > > 1 file changed, 27 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
> > > > index 95d0ec0f5f3a..c198590cd74a 100644
> > > > --- a/drivers/mmc/host/dw_mmc-rockchip.c
> > > > +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> > > > @@ -15,7 +15,9 @@
> > > > #include "dw_mmc.h"
> > > > #include "dw_mmc-pltfm.h"
> > > >
> > > > -#define RK3288_CLKGEN_DIV 2
> > > > +#define RK3288_CLKGEN_DIV 2
> > > > +#define RK3288_MIN_INIT_FREQ 375000
> > > > +#define MMC_MAX_INIT_FREQ 400000
> > > >
> > > > struct dw_mci_rockchip_priv_data {
> > > > struct clk *drv_clk;
> > > > @@ -27,6 +29,7 @@ struct dw_mci_rockchip_priv_data {
> > > > static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> > > > {
> > > > struct dw_mci_rockchip_priv_data *priv = host->priv;
> > > > + struct mmc_host *mmc = mmc_from_priv(host);
> > > > int ret;
> > > > unsigned int cclkin;
> > > > u32 bus_hz;
> > > > @@ -34,6 +37,10 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> > > > if (ios->clock == 0)
> > > > return;
> > > >
> > > > + /* the clock will fail if below the f_min rate */
> > > > + if (ios->clock < mmc->f_min)
> > > > + ios->clock = mmc->f_min;
> > > > +
> > >
> > > You shouldn't need this. The mmc core should manage this already.
> >
> > I thought so too, but while setting f_min did reduce the number of
> > errors, it didn't stop them completely.
> > Each tick I was getting three failures, it turns out mmc core tries
> > anyways with 300000, 200000, and 100000.
> > Clamping it here was necessary to stop these.
>
> Ohh, that was certainly a surprise to me. Unless the dw_mmc driver
> invokes this path on it's own in some odd way, that means the mmc core
> has a bug that we need to fix.
>
> Would you mind taking a stack trace or debug this so we understand in
> what case the mmc core doesn't respect f_min? It really should.
I thought it was odd too, will check into where it's happening.
Thanks!
>
> [...]
>
> Kind regards
> Uffe
Hi,
On Wed, Mar 2, 2022 at 5:52 PM Peter Geis <[email protected]> wrote:
>
> The Rockchip ciu clock cannot be set as low as the dw-mmc hardware
> supports. This leads to a situation during card initialization where the
> ciu clock is set lower than the clock driver can support. The
> dw-mmc-rockchip driver spews errors when this happens.
> For normal operation this only happens a few times during boot, but when
> cd-broken is enabled (in cases such as the SoQuartz module) this fires
> multiple times each poll cycle.
>
> Fix this by testing the minimum frequency the clock driver can support
> that is within the mmc specification, then divide that by the internal
> clock divider. Set the f_min frequency to this value, or if it fails,
> set f_min to the downstream driver's default.
>
> Fixes: f629ba2c04c9 ("mmc: dw_mmc: add support for RK3288")
I don't spend tons of time either Rockchip or dw-mmc these days, but
your email tickled a memory in my mind and I swore that I remember
this whole 400 kHz minimum thing, though I never dug into it myself.
It actually looks like the 400 kHz minimum disappeared sometime in
2016! See commit 6a8883d614c7 ("ARM: dts: rockchip: replace to
"max-frequency" instead of "clock-freq-min-max"") which only accounted
for the high end, not the low end?
I'm pretty sure I've tested on veyron since then, though and I didn't
see any errors, but perhaps this is because I was never using
cd-broken and the 400 kHz always worked?
-Doug
On 2022-03-03 01:51, Peter Geis wrote:
> The Rockchip ciu clock cannot be set as low as the dw-mmc hardware
> supports. This leads to a situation during card initialization where the
> ciu clock is set lower than the clock driver can support. The
> dw-mmc-rockchip driver spews errors when this happens.
> For normal operation this only happens a few times during boot, but when
> cd-broken is enabled (in cases such as the SoQuartz module) this fires
> multiple times each poll cycle.
>
> Fix this by testing the minimum frequency the clock driver can support
> that is within the mmc specification, then divide that by the internal
> clock divider. Set the f_min frequency to this value, or if it fails,
> set f_min to the downstream driver's default.
>
> Fixes: f629ba2c04c9 ("mmc: dw_mmc: add support for RK3288")
>
> Signed-off-by: Peter Geis <[email protected]>
> ---
> drivers/mmc/host/dw_mmc-rockchip.c | 31 ++++++++++++++++++++++++++----
> 1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
> index 95d0ec0f5f3a..c198590cd74a 100644
> --- a/drivers/mmc/host/dw_mmc-rockchip.c
> +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> @@ -15,7 +15,9 @@
> #include "dw_mmc.h"
> #include "dw_mmc-pltfm.h"
>
> -#define RK3288_CLKGEN_DIV 2
> +#define RK3288_CLKGEN_DIV 2
> +#define RK3288_MIN_INIT_FREQ 375000
> +#define MMC_MAX_INIT_FREQ 400000
>
> struct dw_mci_rockchip_priv_data {
> struct clk *drv_clk;
> @@ -27,6 +29,7 @@ struct dw_mci_rockchip_priv_data {
> static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> {
> struct dw_mci_rockchip_priv_data *priv = host->priv;
> + struct mmc_host *mmc = mmc_from_priv(host);
> int ret;
> unsigned int cclkin;
> u32 bus_hz;
> @@ -34,6 +37,10 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
> if (ios->clock == 0)
> return;
>
> + /* the clock will fail if below the f_min rate */
> + if (ios->clock < mmc->f_min)
> + ios->clock = mmc->f_min;
> +
> /*
> * cclkin: source clock of mmc controller
> * bus_hz: card interface clock generated by CLKGEN
> @@ -51,7 +58,7 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>
> ret = clk_set_rate(host->ciu_clk, cclkin);
> if (ret)
> - dev_warn(host->dev, "failed to set rate %uHz\n", ios->clock);
> + dev_warn(host->dev, "failed to set rate %uHz err: %d\n", cclkin, ret);
>
> bus_hz = clk_get_rate(host->ciu_clk) / RK3288_CLKGEN_DIV;
> if (bus_hz != host->bus_hz) {
> @@ -290,13 +297,29 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci *host)
>
> static int dw_mci_rockchip_init(struct dw_mci *host)
> {
> + struct mmc_host *mmc = mmc_from_priv(host);
Hang on, "host" here is a struct dw_mci allocated directly by
dw_mci_pltfm_register(), not as private data via mmc_alloc_host(), so
surely this is bogus?
If I've followed things through correctly, I think it's host->slot->mmc
that we need to propagate a non-default f_min to, except that that
hasn't been allocated yet at this point.
Having multiple different types of "host", "slot" and "mmc" variables
certainly does no favours to making sense of this stuff :(
Robin.
> + int ret;
> +
> /* It is slot 8 on Rockchip SoCs */
> host->sdio_id0 = 8;
>
> - if (of_device_is_compatible(host->dev->of_node,
> - "rockchip,rk3288-dw-mshc"))
> + if (of_device_is_compatible(host->dev->of_node, "rockchip,rk3288-dw-mshc")) {
> host->bus_hz /= RK3288_CLKGEN_DIV;
>
> + /* clock driver will fail if the clock is less than the lowest source clock
> + * divided by the internal clock divider. Test for the lowest available
> + * clock and set the f_min freq to clock / clock divider. If we fail, set
> + * it to the downstream hardcoded value.
> + */
> + ret = clk_round_rate(host->ciu_clk, MMC_MAX_INIT_FREQ * RK3288_CLKGEN_DIV);
> + if (ret < 0) {
> + dev_warn(host->dev, "mmc safe rate failed: %d\n", ret);
> + mmc->f_min = RK3288_MIN_INIT_FREQ;
> + } else {
> + mmc->f_min = ret / RK3288_CLKGEN_DIV;
> + }
> + }
> +
> return 0;
> }
>