Thank Adrian.
Please see inline, one of help needed item on clocking.
> -----Original Message-----
> From: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> Sent: Wednesday, June 12, 2019 5:06 PM
> To: Udit Kumar <udit.kumar@xxxxxxx>; linux-mmc@xxxxxxxxxxxxxxx;
> ulf.hansson@xxxxxxxxxx
> Cc: Varun Sethi <V.Sethi@xxxxxxx>; Leo Li <leoyang.li@xxxxxxx>; Jimmy Zhao
> <jimmy.zhao@xxxxxxx>; Y.b. Lu <yangbo.lu@xxxxxxx>; Yinbo Zhu
> <yinbo.zhu@xxxxxxx>
> Subject: [EXT] Re: [PATCH] enable acpi support in esdhc driver
>
> Caution: EXT Email
>
> On 3/06/19 8:05 AM, Udit Kumar wrote:
> > This pacth enables acpi support in esdhc driver
>
> pacth -> patch
>
Ah, typo Thanks
> >
> > Signed-off-by: Udit Kumar <udit.kumar@xxxxxxx>
>
> Looks ok, but some minor comments.
>
> > ---
> > drivers/mmc/host/sdhci-of-esdhc.c | 55
> > +++++++++++++++++++++++++--------------
> > 1 file changed, 36 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-esdhc.c
> > b/drivers/mmc/host/sdhci-of-esdhc.c
> > index e20c00f..11d9f48 100644
> > --- a/drivers/mmc/host/sdhci-of-esdhc.c
> > +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> > @@ -3,6 +3,7 @@
> > *
> > * Copyright (c) 2007, 2010, 2012 Freescale Semiconductor, Inc.
> > * Copyright (c) 2009 MontaVista Software, Inc.
> > + * Copyright 2019 NXP
> > *
> > * Authors: Xiaobo Xie <X.Xie@xxxxxxxxxxxxx>
> > * Anton Vorontsov <avorontsov@xxxxxxxxxxxxx>
> > @@ -13,6 +14,7 @@
> > * your option) any later version.
> > */
> >
> > +#include <linux/acpi.h>
> > #include <linux/err.h>
> > #include <linux/io.h>
> > #include <linux/of.h>
> > @@ -75,6 +77,12 @@ struct esdhc_clk_fixup { };
> > MODULE_DEVICE_TABLE(of, sdhci_esdhc_of_match);
> >
> > +static const struct acpi_device_id sdhci_esdhc_ids[] = {
> > + {"NXP0003" },
> > + { /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(acpi, sdhci_esdhc_ids);
> > +
> > struct sdhci_esdhc {
> > u8 vendor_ver;
> > u8 spec_ver;
> > @@ -1038,22 +1046,28 @@ static void esdhc_init(struct platform_device
> *pdev, struct sdhci_host *host)
> > match = of_match_node(sdhci_esdhc_of_match, pdev->dev.of_node);
> > if (match)
> > esdhc->clk_fixup = match->data;
> > - np = pdev->dev.of_node;
> > - clk = of_clk_get(np, 0);
> > - if (!IS_ERR(clk)) {
> > - /*
> > - * esdhc->peripheral_clock would be assigned with a value
> > - * which is eSDHC base clock when use periperal clock.
> > - * For ls1046a, the clock value got by common clk API is
> > - * peripheral clock while the eSDHC base clock is 1/2
> > - * peripheral clock.
> > - */
> > - if (of_device_is_compatible(np, "fsl,ls1046a-esdhc"))
> > - esdhc->peripheral_clock = clk_get_rate(clk) / 2;
> > - else
> > - esdhc->peripheral_clock = clk_get_rate(clk);
> >
> > - clk_put(clk);
> > + np = pdev->dev.of_node;
> > + // in case of device tree, get clock frame work
>
> I prefer to remain consistent with the legacy comment style i.e.
Ok
>
> /* In case of device tree, get clock frame work */
>
> > + if (np) {
> > + clk = of_clk_get(np, 0);
> > + if (!IS_ERR(clk)) {
> > + /*
> > + * esdhc->peripheral_clock would be assigned a value
> > + * which is eSDHC base clock when use periperal clock.
> > + * For ls1046a, the clock value got by common clk API is
> > + * peripheral clock while the eSDHC base clock is 1/2
> > + * peripheral clock.
> > + */
> > + if (of_device_is_compatible(np, "fsl,ls1046a-esdhc"))
> > + esdhc->peripheral_clock = clk_get_rate(clk) / 2;
> > + else
> > + esdhc->peripheral_clock = clk_get_rate(clk);
> > + clk_put(clk);
> > + }
> > + } else {
> > + device_property_read_u32(&pdev->dev, "clock-frequency",
> > +
> > + &esdhc->peripheral_clock);
> > }
>
> Not sure the "if (np) {}" really serves a purpose since of_clk_get(() will return an
> error in that case anyway.
Looks this is something, I need to rework, in internal review I got the feedback
There are two clocks one esdhc->peripheral_clock and another is pltfm_host->clock.
As ACPI does not have clock frame work like of DT.
Therefore I have here 2 option
1) expose a sort of new driver with CLK in acpi tables and have reference here like ^SB0.CLK
2) use fixed clock
Please suggest if you have some preference in either of
> >
> > if (esdhc->peripheral_clock) {
> > @@ -1062,7 +1076,8 @@ static void esdhc_init(struct platform_device *pdev,
> struct sdhci_host *host)
> > val |= ESDHC_PERIPHERAL_CLK_SEL;
> > sdhci_writel(host, val, ESDHC_DMA_SYSCTL);
> > esdhc_clock_enable(host, true);
> > - }
> > + } else
> > + WARN_ON(1);
>
> Please balance braces:
Thanks
In v2 , I plan to remove WARN_ON(1) and may probe to fail if clocks are not mentioned
> } else {
> WARN_ON(1);
> }
>
> > }
> >
> > static int esdhc_hs400_prepare_ddr(struct mmc_host *mmc) @@ -1081,9
> > +1096,10 @@ static int sdhci_esdhc_probe(struct platform_device *pdev)
> >
> > np = pdev->dev.of_node;
> >
> > - if (of_property_read_bool(np, "little-endian"))
> > + if (device_property_read_bool(&pdev->dev, "little-endian")) {
> > host = sdhci_pltfm_init(pdev, &sdhci_esdhc_le_pdata,
> > sizeof(struct sdhci_esdhc));
> > + }
> > else
>
> Also here:
Noted
> } else {
>
> > host = sdhci_pltfm_init(pdev, &sdhci_esdhc_be_pdata,
> > sizeof(struct sdhci_esdhc));
>
> }
>
> > @@ -1143,8 +1159,8 @@ static int sdhci_esdhc_probe(struct platform_device
> *pdev)
> > ret = mmc_of_parse(host->mmc);
> > if (ret)
> > goto err;
> > -
> > - mmc_of_parse_voltage(np, &host->ocr_mask);
> > + if (np)
>
> Again not sure the "if (np)" is really needed.
On one platform this is not needed, but I general I need to add this
V2 will cover this
> > + mmc_of_parse_voltage(np, &host->ocr_mask);
> >
> > ret = sdhci_add_host(host);
> > if (ret)
> > @@ -1160,6 +1176,7 @@ static int sdhci_esdhc_probe(struct platform_device
> *pdev)
> > .driver = {
> > .name = "sdhci-esdhc",
> > .of_match_table = sdhci_esdhc_of_match,
> > + .acpi_match_table = sdhci_esdhc_ids,
> > .pm = &esdhc_of_dev_pm_ops,
> > },
> > .probe = sdhci_esdhc_probe,
> >
