| Message ID | 20240822212418.982927-5-detlev.casanova@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add dw_mmc support for rk3576 | expand |
Hello Detlev, Please see a comment below. On 2024-08-22 23:15, Detlev Casanova wrote: > On rk3576 the tunable clocks are inside the controller itself, removing > the need for the "ciu-drive" and "ciu-sample" clocks. > > That makes it a new type of controller that has its own dt_parse > function. > > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com> > --- > drivers/mmc/host/dw_mmc-rockchip.c | 48 ++++++++++++++++++++++++++---- > 1 file changed, 43 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c > b/drivers/mmc/host/dw_mmc-rockchip.c > index 1458cb5fd5c7..7c8ccf5e71bc 100644 > --- a/drivers/mmc/host/dw_mmc-rockchip.c > +++ b/drivers/mmc/host/dw_mmc-rockchip.c > @@ -410,7 +410,7 @@ static int dw_mci_rk3288_execute_tuning(struct > dw_mci_slot *slot, u32 opcode) > return ret; > } > > -static int dw_mci_rk3288_parse_dt(struct dw_mci *host) > +static int dw_mci_common_parse_dt(struct dw_mci *host) > { > struct device_node *np = host->dev->of_node; > struct dw_mci_rockchip_priv_data *priv; > @@ -420,13 +420,29 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci > *host) > return -ENOMEM; > > if (of_property_read_u32(np, "rockchip,desired-num-phases", > - &priv->num_phases)) > + &priv->num_phases)) > priv->num_phases = 360; > > if (of_property_read_u32(np, "rockchip,default-sample-phase", > - &priv->default_sample_phase)) > + &priv->default_sample_phase)) > priv->default_sample_phase = 0; > > + host->priv = priv; > + > + return 0; > +} > + > +static int dw_mci_rk3288_parse_dt(struct dw_mci *host) > +{ > + struct dw_mci_rockchip_priv_data *priv; > + int err; > + > + err = dw_mci_common_parse_dt(host); > + if (err) > + return err; > + > + priv = host->priv; > + > 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"); > @@ -435,13 +451,25 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci > *host) > if (IS_ERR(priv->sample_clk)) > dev_dbg(host->dev, "ciu-sample not available\n"); > > - host->priv = priv; > - > priv->internal_phase = false; > > return 0; > } > > +static int dw_mci_rk3576_parse_dt(struct dw_mci *host) > +{ > + struct dw_mci_rockchip_priv_data *priv; > + int err = dw_mci_common_parse_dt(host); > + if (err) > + return err; > + > + priv = host->priv; > + > + priv->internal_phase = true; Defining priv, assigning it and using it seems rather redundant, when all that's needed is simple "host->priv->internal_phase = true" assignment instead. > + > + return 0; > +} > + > static int dw_mci_rockchip_init(struct dw_mci *host) > { > int ret, i; > @@ -483,11 +511,21 @@ static const struct dw_mci_drv_data > rk3288_drv_data = { > .init = dw_mci_rockchip_init, > }; > > +static const struct dw_mci_drv_data rk3576_drv_data = { > + .common_caps = MMC_CAP_CMD23, > + .set_ios = dw_mci_rk3288_set_ios, > + .execute_tuning = dw_mci_rk3288_execute_tuning, > + .parse_dt = dw_mci_rk3576_parse_dt, > + .init = dw_mci_rockchip_init, > +}; > + > static const struct of_device_id dw_mci_rockchip_match[] = { > { .compatible = "rockchip,rk2928-dw-mshc", > .data = &rk2928_drv_data }, > { .compatible = "rockchip,rk3288-dw-mshc", > .data = &rk3288_drv_data }, > + { .compatible = "rockchip,rk3576-dw-mshc", > + .data = &rk3576_drv_data }, > {}, > }; > MODULE_DEVICE_TABLE(of, dw_mci_rockchip_match);
Hi Dragan, On Friday, 23 August 2024 03:00:57 EDT Dragan Simic wrote: > Hello Detlev, > > Please see a comment below. > > On 2024-08-22 23:15, Detlev Casanova wrote: > > On rk3576 the tunable clocks are inside the controller itself, removing > > the need for the "ciu-drive" and "ciu-sample" clocks. > > > > That makes it a new type of controller that has its own dt_parse > > function. > > > > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com> > > --- > > > > drivers/mmc/host/dw_mmc-rockchip.c | 48 ++++++++++++++++++++++++++---- > > 1 file changed, 43 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c > > b/drivers/mmc/host/dw_mmc-rockchip.c > > index 1458cb5fd5c7..7c8ccf5e71bc 100644 > > --- a/drivers/mmc/host/dw_mmc-rockchip.c > > +++ b/drivers/mmc/host/dw_mmc-rockchip.c [...] > > @@ -435,13 +451,25 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci > > *host) > > > > if (IS_ERR(priv->sample_clk)) > > > > dev_dbg(host->dev, "ciu-sample not available\n"); > > > > - host->priv = priv; > > - > > > > priv->internal_phase = false; > > > > return 0; > > > > } > > > > +static int dw_mci_rk3576_parse_dt(struct dw_mci *host) > > +{ > > + struct dw_mci_rockchip_priv_data *priv; > > + int err = dw_mci_common_parse_dt(host); > > + if (err) > > + return err; > > + > > + priv = host->priv; > > + > > + priv->internal_phase = true; > > Defining priv, assigning it and using it seems rather redundant, > when all that's needed is simple "host->priv->internal_phase = true" > assignment instead. Yes, that's what I did at first, but host->priv is declared as void*, which means it needs to be cast to struct dw_mci_rockchip_priv_data * and I felt that ((struct dw_mci_rockchip_priv_data *)host->priv)->internal_phase = true; is not very pretty and harder to read. > > + > > + return 0; > > +} > > + > > > > static int dw_mci_rockchip_init(struct dw_mci *host) > > { > > > > int ret, i; > > > > @@ -483,11 +511,21 @@ static const struct dw_mci_drv_data > > rk3288_drv_data = { > > > > .init = dw_mci_rockchip_init, > > > > }; > > > > +static const struct dw_mci_drv_data rk3576_drv_data = { > > + .common_caps = MMC_CAP_CMD23, > > + .set_ios = dw_mci_rk3288_set_ios, > > + .execute_tuning = dw_mci_rk3288_execute_tuning, > > + .parse_dt = dw_mci_rk3576_parse_dt, > > + .init = dw_mci_rockchip_init, > > +}; > > + > > > > static const struct of_device_id dw_mci_rockchip_match[] = { > > > > { .compatible = "rockchip,rk2928-dw-mshc", > > > > .data = &rk2928_drv_data }, > > > > { .compatible = "rockchip,rk3288-dw-mshc", > > > > .data = &rk3288_drv_data }, > > > > + { .compatible = "rockchip,rk3576-dw-mshc", > > + .data = &rk3576_drv_data }, > > > > {}, > > > > }; > > MODULE_DEVICE_TABLE(of, dw_mci_rockchip_match);
Hello Detlev, On 2024-08-23 15:20, Detlev Casanova wrote: > On Friday, 23 August 2024 03:00:57 EDT Dragan Simic wrote: >> Hello Detlev, >> >> Please see a comment below. >> >> On 2024-08-22 23:15, Detlev Casanova wrote: >> > On rk3576 the tunable clocks are inside the controller itself, removing >> > the need for the "ciu-drive" and "ciu-sample" clocks. >> > >> > That makes it a new type of controller that has its own dt_parse >> > function. >> > >> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com> >> > --- >> > >> > drivers/mmc/host/dw_mmc-rockchip.c | 48 ++++++++++++++++++++++++++---- >> > 1 file changed, 43 insertions(+), 5 deletions(-) >> > >> > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c >> > b/drivers/mmc/host/dw_mmc-rockchip.c >> > index 1458cb5fd5c7..7c8ccf5e71bc 100644 >> > --- a/drivers/mmc/host/dw_mmc-rockchip.c >> > +++ b/drivers/mmc/host/dw_mmc-rockchip.c > [...] >> > @@ -435,13 +451,25 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci >> > *host) >> > >> > if (IS_ERR(priv->sample_clk)) >> > >> > dev_dbg(host->dev, "ciu-sample not available\n"); >> > >> > - host->priv = priv; >> > - >> > >> > priv->internal_phase = false; >> > >> > return 0; >> > >> > } >> > >> > +static int dw_mci_rk3576_parse_dt(struct dw_mci *host) >> > +{ >> > + struct dw_mci_rockchip_priv_data *priv; >> > + int err = dw_mci_common_parse_dt(host); >> > + if (err) >> > + return err; >> > + >> > + priv = host->priv; >> > + >> > + priv->internal_phase = true; >> >> Defining priv, assigning it and using it seems rather redundant, >> when all that's needed is simple "host->priv->internal_phase = true" >> assignment instead. > > Yes, that's what I did at first, but host->priv is declared as void*, > which > means it needs to be cast to struct dw_mci_rockchip_priv_data * and I > felt > that > > ((struct dw_mci_rockchip_priv_data *)host->priv)->internal_phase = > true; > > is not very pretty and harder to read. Ah, you're right, and I somehow managed to ignore that. I agree with your conclusions, although I'd suggest something like this, for slightly improved readability: +static int dw_mci_rk3576_parse_dt(struct dw_mci *host) +{ + struct dw_mci_rockchip_priv_data *priv = host->priv; + int err; + + err = dw_mci_common_parse_dt(host); + if (err) + return err; + + priv->internal_phase = true; + + return 0; +}
Hi Dragan, On Monday, 26 August 2024 10:07:49 EDT Dragan Simic wrote: > Hello Detlev, > > On 2024-08-23 15:20, Detlev Casanova wrote: > > On Friday, 23 August 2024 03:00:57 EDT Dragan Simic wrote: > >> Hello Detlev, > >> > >> Please see a comment below. > >> > >> On 2024-08-22 23:15, Detlev Casanova wrote: > >> > On rk3576 the tunable clocks are inside the controller itself, removing > >> > the need for the "ciu-drive" and "ciu-sample" clocks. > >> > > >> > That makes it a new type of controller that has its own dt_parse > >> > function. > >> > > >> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com> > >> > --- > >> > > >> > drivers/mmc/host/dw_mmc-rockchip.c | 48 ++++++++++++++++++++++++++---- > >> > 1 file changed, 43 insertions(+), 5 deletions(-) > >> > > >> > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c > >> > b/drivers/mmc/host/dw_mmc-rockchip.c > >> > index 1458cb5fd5c7..7c8ccf5e71bc 100644 > >> > --- a/drivers/mmc/host/dw_mmc-rockchip.c > >> > +++ b/drivers/mmc/host/dw_mmc-rockchip.c > > > > [...] > > > >> > @@ -435,13 +451,25 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci > >> > *host) > >> > > >> > if (IS_ERR(priv->sample_clk)) > >> > > >> > dev_dbg(host->dev, "ciu-sample not available\n"); > >> > > >> > - host->priv = priv; > >> > - > >> > > >> > priv->internal_phase = false; > >> > > >> > return 0; > >> > > >> > } > >> > > >> > +static int dw_mci_rk3576_parse_dt(struct dw_mci *host) > >> > +{ > >> > + struct dw_mci_rockchip_priv_data *priv; > >> > + int err = dw_mci_common_parse_dt(host); > >> > + if (err) > >> > + return err; > >> > + > >> > + priv = host->priv; > >> > + > >> > + priv->internal_phase = true; > >> > >> Defining priv, assigning it and using it seems rather redundant, > >> when all that's needed is simple "host->priv->internal_phase = true" > >> assignment instead. > > > > Yes, that's what I did at first, but host->priv is declared as void*, > > which > > means it needs to be cast to struct dw_mci_rockchip_priv_data * and I > > felt > > that > > > > ((struct dw_mci_rockchip_priv_data *)host->priv)->internal_phase = > > true; > > > > is not very pretty and harder to read. > > Ah, you're right, and I somehow managed to ignore that. > > I agree with your conclusions, although I'd suggest something like > this, for slightly improved readability: > > +static int dw_mci_rk3576_parse_dt(struct dw_mci *host) > +{ > + struct dw_mci_rockchip_priv_data *priv = host->priv; > + int err; > + > + err = dw_mci_common_parse_dt(host); > + if (err) > + return err; > + > + priv->internal_phase = true; > + > + return 0; > +} That won't work either, because host->priv is initialized in dw_mci_common_parse_dt.
diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c index 1458cb5fd5c7..7c8ccf5e71bc 100644 --- a/drivers/mmc/host/dw_mmc-rockchip.c +++ b/drivers/mmc/host/dw_mmc-rockchip.c @@ -410,7 +410,7 @@ static int dw_mci_rk3288_execute_tuning(struct dw_mci_slot *slot, u32 opcode) return ret; } -static int dw_mci_rk3288_parse_dt(struct dw_mci *host) +static int dw_mci_common_parse_dt(struct dw_mci *host) { struct device_node *np = host->dev->of_node; struct dw_mci_rockchip_priv_data *priv; @@ -420,13 +420,29 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci *host) return -ENOMEM; if (of_property_read_u32(np, "rockchip,desired-num-phases", - &priv->num_phases)) + &priv->num_phases)) priv->num_phases = 360; if (of_property_read_u32(np, "rockchip,default-sample-phase", - &priv->default_sample_phase)) + &priv->default_sample_phase)) priv->default_sample_phase = 0; + host->priv = priv; + + return 0; +} + +static int dw_mci_rk3288_parse_dt(struct dw_mci *host) +{ + struct dw_mci_rockchip_priv_data *priv; + int err; + + err = dw_mci_common_parse_dt(host); + if (err) + return err; + + priv = host->priv; + 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"); @@ -435,13 +451,25 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci *host) if (IS_ERR(priv->sample_clk)) dev_dbg(host->dev, "ciu-sample not available\n"); - host->priv = priv; - priv->internal_phase = false; return 0; } +static int dw_mci_rk3576_parse_dt(struct dw_mci *host) +{ + struct dw_mci_rockchip_priv_data *priv; + int err = dw_mci_common_parse_dt(host); + if (err) + return err; + + priv = host->priv; + + priv->internal_phase = true; + + return 0; +} + static int dw_mci_rockchip_init(struct dw_mci *host) { int ret, i; @@ -483,11 +511,21 @@ static const struct dw_mci_drv_data rk3288_drv_data = { .init = dw_mci_rockchip_init, }; +static const struct dw_mci_drv_data rk3576_drv_data = { + .common_caps = MMC_CAP_CMD23, + .set_ios = dw_mci_rk3288_set_ios, + .execute_tuning = dw_mci_rk3288_execute_tuning, + .parse_dt = dw_mci_rk3576_parse_dt, + .init = dw_mci_rockchip_init, +}; + static const struct of_device_id dw_mci_rockchip_match[] = { { .compatible = "rockchip,rk2928-dw-mshc", .data = &rk2928_drv_data }, { .compatible = "rockchip,rk3288-dw-mshc", .data = &rk3288_drv_data }, + { .compatible = "rockchip,rk3576-dw-mshc", + .data = &rk3576_drv_data }, {}, }; MODULE_DEVICE_TABLE(of, dw_mci_rockchip_match);
On rk3576 the tunable clocks are inside the controller itself, removing the need for the "ciu-drive" and "ciu-sample" clocks. That makes it a new type of controller that has its own dt_parse function. Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com> --- drivers/mmc/host/dw_mmc-rockchip.c | 48 ++++++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 5 deletions(-)