| Message ID | 20240215-mmc_phase-v1-1-f27644ee13e4@outlook.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: host: replace 1st argument to struct device * for mmc_of_parse_clk_phase() | expand |
On Thu, 15 Feb 2024 at 12:03, Yang Xiwen via B4 Relay <devnull+forbidden405.outlook.com@kernel.org> wrote: > > From: Yang Xiwen <forbidden405@outlook.com> > > Parsing dt usaully happens very early, sometimes even bofore struct > mmc_host is allocated (e.g. dw_mci_probe() and dw_mci_parse_dt() in > dw_mmc.c). Looking at the source of mmc_of_parse_clk_phase(), it's > actually not mandatory to have a initialized mmc_host first, instead we > can pass struct device * to it directly. > > Also fix the only current user (sdhci-of-aspeed.c). Is there a problem? I don't see it, can please elaborate. Kind regards Uffe > > Signed-off-by: Yang Xiwen <forbidden405@outlook.com> > --- > drivers/mmc/core/host.c | 4 +--- > drivers/mmc/host/sdhci-of-aspeed.c | 2 +- > include/linux/mmc/host.h | 2 +- > 3 files changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c > index cf396e8f34e9..8b2844ac5dc5 100644 > --- a/drivers/mmc/core/host.c > +++ b/drivers/mmc/core/host.c > @@ -234,10 +234,8 @@ static void mmc_of_parse_timing_phase(struct device *dev, const char *prop, > } > > void > -mmc_of_parse_clk_phase(struct mmc_host *host, struct mmc_clk_phase_map *map) > +mmc_of_parse_clk_phase(struct device *dev, struct mmc_clk_phase_map *map) > { > - struct device *dev = host->parent; > - > mmc_of_parse_timing_phase(dev, "clk-phase-legacy", > &map->phase[MMC_TIMING_LEGACY]); > mmc_of_parse_timing_phase(dev, "clk-phase-mmc-hs", > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c > index 42d54532cabe..430c1f90037b 100644 > --- a/drivers/mmc/host/sdhci-of-aspeed.c > +++ b/drivers/mmc/host/sdhci-of-aspeed.c > @@ -435,7 +435,7 @@ static int aspeed_sdhci_probe(struct platform_device *pdev) > goto err_sdhci_add; > > if (dev->phase_desc) > - mmc_of_parse_clk_phase(host->mmc, &dev->phase_map); > + mmc_of_parse_clk_phase(&pdev->dev, &dev->phase_map); > > ret = sdhci_add_host(host); > if (ret) > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h > index 2f445c651742..5894bf912f7b 100644 > --- a/include/linux/mmc/host.h > +++ b/include/linux/mmc/host.h > @@ -539,7 +539,7 @@ struct mmc_host *devm_mmc_alloc_host(struct device *dev, int extra); > int mmc_add_host(struct mmc_host *); > void mmc_remove_host(struct mmc_host *); > void mmc_free_host(struct mmc_host *); > -void mmc_of_parse_clk_phase(struct mmc_host *host, > +void mmc_of_parse_clk_phase(struct device *dev, > struct mmc_clk_phase_map *map); > int mmc_of_parse(struct mmc_host *host); > int mmc_of_parse_voltage(struct mmc_host *host, u32 *mask); > > --- > base-commit: 9d64bf433c53cab2f48a3fff7a1f2a696bc5229a > change-id: 20240215-mmc_phase-26e85511285d > > Best regards, > -- > Yang Xiwen <forbidden405@outlook.com> >
On 2/16/2024 12:47 AM, Ulf Hansson wrote: > On Thu, 15 Feb 2024 at 12:03, Yang Xiwen via B4 Relay > <devnull+forbidden405.outlook.com@kernel.org> wrote: >> From: Yang Xiwen <forbidden405@outlook.com> >> >> Parsing dt usaully happens very early, sometimes even bofore struct >> mmc_host is allocated (e.g. dw_mci_probe() and dw_mci_parse_dt() in >> dw_mmc.c). Looking at the source of mmc_of_parse_clk_phase(), it's >> actually not mandatory to have a initialized mmc_host first, instead we >> can pass struct device * to it directly. >> >> Also fix the only current user (sdhci-of-aspeed.c). > Is there a problem? I don't see it, can please elaborate. My pleasure. Just look into dw_mci_parse_dt() in dw_mmc.c, this function calls specific extension's .parse_dt() function. And this function is called very early even before struct mmc_host is allocated. Thus it'll be impossible to retrieve host->slot->dev right now since host->slot is still NULL. As a result, We are unable to call mmc_of_parse_clk_phase() in our own .parse_dt() because it needs an initialized struct mmc_host. But mmc_of_parse_clk_phase is intended to be invoked there since this is where the driver is supposed to parse dt. dw_mci_probe() allocates struct mmc_host and calls mmc_of_parse() very late(in dw_mci_init_slot()) and provides us no chance to insert our own dt parsing extensions after that. The key is that mmc_of_parse_clk_phase() is a very simple function which only extracts a few dt props. It is supposed to be called early in the driver before struct mmc_host is allocated. Unlike mmc_of_parse() which fills in a lot of fields in struct mmc_host, it does not need this struct. > Kind regards > Uffe > >> Signed-off-by: Yang Xiwen <forbidden405@outlook.com> >> --- >> drivers/mmc/core/host.c | 4 +--- >> drivers/mmc/host/sdhci-of-aspeed.c | 2 +- >> include/linux/mmc/host.h | 2 +- >> 3 files changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c >> index cf396e8f34e9..8b2844ac5dc5 100644 >> --- a/drivers/mmc/core/host.c >> +++ b/drivers/mmc/core/host.c >> @@ -234,10 +234,8 @@ static void mmc_of_parse_timing_phase(struct device *dev, const char *prop, >> } >> >> void >> -mmc_of_parse_clk_phase(struct mmc_host *host, struct mmc_clk_phase_map *map) >> +mmc_of_parse_clk_phase(struct device *dev, struct mmc_clk_phase_map *map) >> { >> - struct device *dev = host->parent; >> - >> mmc_of_parse_timing_phase(dev, "clk-phase-legacy", >> &map->phase[MMC_TIMING_LEGACY]); >> mmc_of_parse_timing_phase(dev, "clk-phase-mmc-hs", >> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c >> index 42d54532cabe..430c1f90037b 100644 >> --- a/drivers/mmc/host/sdhci-of-aspeed.c >> +++ b/drivers/mmc/host/sdhci-of-aspeed.c >> @@ -435,7 +435,7 @@ static int aspeed_sdhci_probe(struct platform_device *pdev) >> goto err_sdhci_add; >> >> if (dev->phase_desc) >> - mmc_of_parse_clk_phase(host->mmc, &dev->phase_map); >> + mmc_of_parse_clk_phase(&pdev->dev, &dev->phase_map); >> >> ret = sdhci_add_host(host); >> if (ret) >> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h >> index 2f445c651742..5894bf912f7b 100644 >> --- a/include/linux/mmc/host.h >> +++ b/include/linux/mmc/host.h >> @@ -539,7 +539,7 @@ struct mmc_host *devm_mmc_alloc_host(struct device *dev, int extra); >> int mmc_add_host(struct mmc_host *); >> void mmc_remove_host(struct mmc_host *); >> void mmc_free_host(struct mmc_host *); >> -void mmc_of_parse_clk_phase(struct mmc_host *host, >> +void mmc_of_parse_clk_phase(struct device *dev, >> struct mmc_clk_phase_map *map); >> int mmc_of_parse(struct mmc_host *host); >> int mmc_of_parse_voltage(struct mmc_host *host, u32 *mask); >> >> --- >> base-commit: 9d64bf433c53cab2f48a3fff7a1f2a696bc5229a >> change-id: 20240215-mmc_phase-26e85511285d >> >> Best regards, >> -- >> Yang Xiwen <forbidden405@outlook.com> >>
On Thu, 2024-02-15 at 19:03 +0800, Yang Xiwen via B4 Relay wrote: > From: Yang Xiwen <forbidden405@outlook.com> > > Parsing dt usaully happens very early, sometimes even bofore struct > mmc_host is allocated (e.g. dw_mci_probe() and dw_mci_parse_dt() in > dw_mmc.c). Looking at the source of mmc_of_parse_clk_phase(), it's > actually not mandatory to have a initialized mmc_host first, instead we > can pass struct device * to it directly. > > Also fix the only current user (sdhci-of-aspeed.c). > > Signed-off-by: Yang Xiwen <forbidden405@outlook.com> > --- > drivers/mmc/core/host.c | 4 +--- > drivers/mmc/host/sdhci-of-aspeed.c | 2 +- So I think I wrote the mmc_of_parse_clk_phase() prototype with `struct mmc_host *` simply because mmc_of_parse() took `struct mmc_host *`. I'll defer to Ulf for whether what you're proposing is desirable, but in terms of the impact on the Aspeed driver it seems okay. Acked-by: Andrew Jeffery <andrew@codeconstruct.com.au>
Dear Yang, Thank you very much for your patch. Some nits from my side. For the summary, I suggest: > mmc: host: mmc_of_parse_clk_phase(): Pass struct device * instead of mmc_host * Am 15.02.24 um 12:03 schrieb Yang Xiwen via B4 Relay: > From: Yang Xiwen <forbidden405@outlook.com> > > Parsing dt usaully happens very early, sometimes even bofore struct 1. us*ua*lly 2. b*e*fore > mmc_host is allocated (e.g. dw_mci_probe() and dw_mci_parse_dt() in > dw_mmc.c). Looking at the source of mmc_of_parse_clk_phase(), it's > actually not mandatory to have a initialized mmc_host first, instead we a*n* > can pass struct device * to it directly. > > Also fix the only current user (sdhci-of-aspeed.c). > > Signed-off-by: Yang Xiwen <forbidden405@outlook.com> > --- > drivers/mmc/core/host.c | 4 +--- > drivers/mmc/host/sdhci-of-aspeed.c | 2 +- > include/linux/mmc/host.h | 2 +- > 3 files changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c > index cf396e8f34e9..8b2844ac5dc5 100644 > --- a/drivers/mmc/core/host.c > +++ b/drivers/mmc/core/host.c > @@ -234,10 +234,8 @@ static void mmc_of_parse_timing_phase(struct device *dev, const char *prop, > } > > void > -mmc_of_parse_clk_phase(struct mmc_host *host, struct mmc_clk_phase_map *map) > +mmc_of_parse_clk_phase(struct device *dev, struct mmc_clk_phase_map *map) > { > - struct device *dev = host->parent; > - > mmc_of_parse_timing_phase(dev, "clk-phase-legacy", > &map->phase[MMC_TIMING_LEGACY]); > mmc_of_parse_timing_phase(dev, "clk-phase-mmc-hs", > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c > index 42d54532cabe..430c1f90037b 100644 > --- a/drivers/mmc/host/sdhci-of-aspeed.c > +++ b/drivers/mmc/host/sdhci-of-aspeed.c > @@ -435,7 +435,7 @@ static int aspeed_sdhci_probe(struct platform_device *pdev) > goto err_sdhci_add; > > if (dev->phase_desc) > - mmc_of_parse_clk_phase(host->mmc, &dev->phase_map); > + mmc_of_parse_clk_phase(&pdev->dev, &dev->phase_map); > > ret = sdhci_add_host(host); > if (ret) > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h > index 2f445c651742..5894bf912f7b 100644 > --- a/include/linux/mmc/host.h > +++ b/include/linux/mmc/host.h > @@ -539,7 +539,7 @@ struct mmc_host *devm_mmc_alloc_host(struct device *dev, int extra); > int mmc_add_host(struct mmc_host *); > void mmc_remove_host(struct mmc_host *); > void mmc_free_host(struct mmc_host *); > -void mmc_of_parse_clk_phase(struct mmc_host *host, > +void mmc_of_parse_clk_phase(struct device *dev, > struct mmc_clk_phase_map *map); > int mmc_of_parse(struct mmc_host *host); > int mmc_of_parse_voltage(struct mmc_host *host, u32 *mask); Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de> Kind regards, Paul
On 2/15/2024 7:03 PM, Yang Xiwen via B4 Relay wrote: > From: Yang Xiwen <forbidden405@outlook.com> > > Parsing dt usaully happens very early, sometimes even bofore struct > mmc_host is allocated (e.g. dw_mci_probe() and dw_mci_parse_dt() in > dw_mmc.c). Looking at the source of mmc_of_parse_clk_phase(), it's > actually not mandatory to have a initialized mmc_host first, instead we > can pass struct device * to it directly. > > Also fix the only current user (sdhci-of-aspeed.c). > > Signed-off-by: Yang Xiwen <forbidden405@outlook.com> > --- > drivers/mmc/core/host.c | 4 +--- > drivers/mmc/host/sdhci-of-aspeed.c | 2 +- > include/linux/mmc/host.h | 2 +- > 3 files changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c > index cf396e8f34e9..8b2844ac5dc5 100644 > --- a/drivers/mmc/core/host.c > +++ b/drivers/mmc/core/host.c > @@ -234,10 +234,8 @@ static void mmc_of_parse_timing_phase(struct device *dev, const char *prop, > } > > void > -mmc_of_parse_clk_phase(struct mmc_host *host, struct mmc_clk_phase_map *map) > +mmc_of_parse_clk_phase(struct device *dev, struct mmc_clk_phase_map *map) > { > - struct device *dev = host->parent; > - > mmc_of_parse_timing_phase(dev, "clk-phase-legacy", > &map->phase[MMC_TIMING_LEGACY]); > mmc_of_parse_timing_phase(dev, "clk-phase-mmc-hs", > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c > index 42d54532cabe..430c1f90037b 100644 > --- a/drivers/mmc/host/sdhci-of-aspeed.c > +++ b/drivers/mmc/host/sdhci-of-aspeed.c > @@ -435,7 +435,7 @@ static int aspeed_sdhci_probe(struct platform_device *pdev) > goto err_sdhci_add; > > if (dev->phase_desc) > - mmc_of_parse_clk_phase(host->mmc, &dev->phase_map); > + mmc_of_parse_clk_phase(&pdev->dev, &dev->phase_map); > > ret = sdhci_add_host(host); > if (ret) > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h > index 2f445c651742..5894bf912f7b 100644 > --- a/include/linux/mmc/host.h > +++ b/include/linux/mmc/host.h > @@ -539,7 +539,7 @@ struct mmc_host *devm_mmc_alloc_host(struct device *dev, int extra); > int mmc_add_host(struct mmc_host *); > void mmc_remove_host(struct mmc_host *); > void mmc_free_host(struct mmc_host *); > -void mmc_of_parse_clk_phase(struct mmc_host *host, > +void mmc_of_parse_clk_phase(struct device *dev, > struct mmc_clk_phase_map *map); > int mmc_of_parse(struct mmc_host *host); > int mmc_of_parse_voltage(struct mmc_host *host, u32 *mask); > > --- > base-commit: 9d64bf433c53cab2f48a3fff7a1f2a696bc5229a > change-id: 20240215-mmc_phase-26e85511285d > > Best regards, Dear maintainers, this patch has been absorbed by the patchset here: https://lore.kernel.org/all/20240221-b4-mmc-hi3798mv200-v6-1-bc41bf6a9769@outlook.com/ So do not apply this patch directly anymore.
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c index cf396e8f34e9..8b2844ac5dc5 100644 --- a/drivers/mmc/core/host.c +++ b/drivers/mmc/core/host.c @@ -234,10 +234,8 @@ static void mmc_of_parse_timing_phase(struct device *dev, const char *prop, } void -mmc_of_parse_clk_phase(struct mmc_host *host, struct mmc_clk_phase_map *map) +mmc_of_parse_clk_phase(struct device *dev, struct mmc_clk_phase_map *map) { - struct device *dev = host->parent; - mmc_of_parse_timing_phase(dev, "clk-phase-legacy", &map->phase[MMC_TIMING_LEGACY]); mmc_of_parse_timing_phase(dev, "clk-phase-mmc-hs", diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c index 42d54532cabe..430c1f90037b 100644 --- a/drivers/mmc/host/sdhci-of-aspeed.c +++ b/drivers/mmc/host/sdhci-of-aspeed.c @@ -435,7 +435,7 @@ static int aspeed_sdhci_probe(struct platform_device *pdev) goto err_sdhci_add; if (dev->phase_desc) - mmc_of_parse_clk_phase(host->mmc, &dev->phase_map); + mmc_of_parse_clk_phase(&pdev->dev, &dev->phase_map); ret = sdhci_add_host(host); if (ret) diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index 2f445c651742..5894bf912f7b 100644 --- a/include/linux/mmc/host.h +++ b/include/linux/mmc/host.h @@ -539,7 +539,7 @@ struct mmc_host *devm_mmc_alloc_host(struct device *dev, int extra); int mmc_add_host(struct mmc_host *); void mmc_remove_host(struct mmc_host *); void mmc_free_host(struct mmc_host *); -void mmc_of_parse_clk_phase(struct mmc_host *host, +void mmc_of_parse_clk_phase(struct device *dev, struct mmc_clk_phase_map *map); int mmc_of_parse(struct mmc_host *host); int mmc_of_parse_voltage(struct mmc_host *host, u32 *mask);