| Message ID | 20220513201907.36470-3-kdasu.kdev@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mmc: sdhci-brcmstb: Add support for optional sdio_freq clock | expand |
On Fri, 13 May 2022 at 22:19, Kamal Dasu <kdasu.kdev@gmail.com> wrote: > > From: Al Cooper <alcooperx@gmail.com> > > The 72116B0 has improved SDIO controllers that allow the max clock > rate to be increased from a max of 100MHz to a max of 150MHz. The > driver will need to get the clock and increase it's default rate > and override the caps register, that still indicates a max of 100MHz. > The new clock will be named "sdio_freq" in the DT node's "clock-names" > list. The driver will use a DT property, "clock-frequency", to /s/clock-frequency/max-frequency > enable this functionality and will get the actual rate in MHz > from the property to allow various speeds to be requested. > > Signed-off-by: Al Cooper <alcooperx@gmail.com> > Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com> > --- > drivers/mmc/host/sdhci-brcmstb.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c > index 8eb57de48e0c..a1ffdd3f1640 100644 > --- a/drivers/mmc/host/sdhci-brcmstb.c > +++ b/drivers/mmc/host/sdhci-brcmstb.c > @@ -250,6 +250,9 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev) > struct sdhci_pltfm_host *pltfm_host; > const struct of_device_id *match; > struct sdhci_brcmstb_priv *priv; > + struct clk *master_clk; > + u32 base_clock_hz = 0; > + u32 actual_clock_mhz; > struct sdhci_host *host; > struct resource *iomem; > struct clk *clk; > @@ -330,6 +333,33 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev) > if (match_priv->flags & BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT) > host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL; > > + /* Change the base clock frequency if the DT property exists */ > + if (device_property_read_u32(&pdev->dev, "max-frequency", > + &base_clock_hz) != 0) > + goto add_host; The max-frequency DT property is already being parsed by mmc_of_parse() and the value is put in host->mmc->f_max. You could probably use that instead, right? > + > + master_clk = devm_clk_get(&pdev->dev, "sdio_freq"); > + if (IS_ERR(master_clk)) { > + dev_warn(&pdev->dev, "Clock for \"sdio_freq\" not found\n"); > + goto add_host; > + } else { > + res = clk_prepare_enable(master_clk); > + if (res) > + goto err; > + } > + > + /* set improved clock rate */ > + clk_set_rate(master_clk, base_clock_hz); > + actual_clock_mhz = clk_get_rate(master_clk) / 1000000; > + > + host->caps &= ~SDHCI_CLOCK_V3_BASE_MASK; > + host->caps |= (actual_clock_mhz << SDHCI_CLOCK_BASE_SHIFT); > + /* Disable presets because they are now incorrect */ > + host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN; > + dev_dbg(&pdev->dev, "Base Clock Frequency changed to %dMHz\n", > + actual_clock_mhz); > + > +add_host: > res = sdhci_brcmstb_add_host(host, priv); > if (res) > goto err; > -- > 2.17.1 > Kind regards Uffe
This seems confusing and seems to overload the meaning of "max-frequency" which is typically used to limit the clock rate to something slower than what's in the CAPs register as the base clock. Instead we're trying to overclock the controller because the hardware team has verified that it can be run faster than 100MHz which is how the system is configured and is in the CAPs register as the base clock. Al On Tue, May 17, 2022 at 8:50 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Fri, 13 May 2022 at 22:19, Kamal Dasu <kdasu.kdev@gmail.com> wrote: > > > > From: Al Cooper <alcooperx@gmail.com> > > > > The 72116B0 has improved SDIO controllers that allow the max clock > > rate to be increased from a max of 100MHz to a max of 150MHz. The > > driver will need to get the clock and increase it's default rate > > and override the caps register, that still indicates a max of 100MHz. > > The new clock will be named "sdio_freq" in the DT node's "clock-names" > > list. The driver will use a DT property, "clock-frequency", to > > /s/clock-frequency/max-frequency > > > > enable this functionality and will get the actual rate in MHz > > from the property to allow various speeds to be requested. > > > > Signed-off-by: Al Cooper <alcooperx@gmail.com> > > Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com> > > --- > > drivers/mmc/host/sdhci-brcmstb.c | 30 ++++++++++++++++++++++++++++++ > > 1 file changed, 30 insertions(+) > > > > diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c > > index 8eb57de48e0c..a1ffdd3f1640 100644 > > --- a/drivers/mmc/host/sdhci-brcmstb.c > > +++ b/drivers/mmc/host/sdhci-brcmstb.c > > @@ -250,6 +250,9 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev) > > struct sdhci_pltfm_host *pltfm_host; > > const struct of_device_id *match; > > struct sdhci_brcmstb_priv *priv; > > + struct clk *master_clk; > > + u32 base_clock_hz = 0; > > + u32 actual_clock_mhz; > > struct sdhci_host *host; > > struct resource *iomem; > > struct clk *clk; > > @@ -330,6 +333,33 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev) > > if (match_priv->flags & BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT) > > host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL; > > > > + /* Change the base clock frequency if the DT property exists */ > > + if (device_property_read_u32(&pdev->dev, "max-frequency", > > + &base_clock_hz) != 0) > > + goto add_host; > > The max-frequency DT property is already being parsed by > mmc_of_parse() and the value is put in host->mmc->f_max. You could > probably use that instead, right? > > > + > > + master_clk = devm_clk_get(&pdev->dev, "sdio_freq"); > > + if (IS_ERR(master_clk)) { > > + dev_warn(&pdev->dev, "Clock for \"sdio_freq\" not found\n"); > > + goto add_host; > > + } else { > > + res = clk_prepare_enable(master_clk); > > + if (res) > > + goto err; > > + } > > + > > + /* set improved clock rate */ > > + clk_set_rate(master_clk, base_clock_hz); > > + actual_clock_mhz = clk_get_rate(master_clk) / 1000000; > > + > > + host->caps &= ~SDHCI_CLOCK_V3_BASE_MASK; > > + host->caps |= (actual_clock_mhz << SDHCI_CLOCK_BASE_SHIFT); > > + /* Disable presets because they are now incorrect */ > > + host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN; > > + dev_dbg(&pdev->dev, "Base Clock Frequency changed to %dMHz\n", > > + actual_clock_mhz); > > + > > +add_host: > > res = sdhci_brcmstb_add_host(host, priv); > > if (res) > > goto err; > > -- > > 2.17.1 > > > > Kind regards > Uffe
I think I seem to agree now that overloading the meaning of max-frequency for the sdio_freq clock might not be a good idea. If Uffe is Ok I think I will revert back to using the 'clock-frequency' field in the brcmstb,sdhci-brcmstb dt node and implement other suggestions as well Kamal On Thu, May 19, 2022 at 10:16 PM Alan Cooper <alcooperx@gmail.com> wrote: > > This seems confusing and seems to overload the meaning of > "max-frequency" which is typically used to limit the clock rate to > something slower than what's in the CAPs register as the base clock. > Instead we're trying to overclock the controller because the hardware > team has verified that it can be run faster than 100MHz which is how > the system is configured and is in the CAPs register as the base > clock. > > Al > > On Tue, May 17, 2022 at 8:50 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Fri, 13 May 2022 at 22:19, Kamal Dasu <kdasu.kdev@gmail.com> wrote: > > > > > > From: Al Cooper <alcooperx@gmail.com> > > > > > > The 72116B0 has improved SDIO controllers that allow the max clock > > > rate to be increased from a max of 100MHz to a max of 150MHz. The > > > driver will need to get the clock and increase it's default rate > > > and override the caps register, that still indicates a max of 100MHz. > > > The new clock will be named "sdio_freq" in the DT node's "clock-names" > > > list. The driver will use a DT property, "clock-frequency", to > > > > /s/clock-frequency/max-frequency > > > > > > > enable this functionality and will get the actual rate in MHz > > > from the property to allow various speeds to be requested. > > > > > > Signed-off-by: Al Cooper <alcooperx@gmail.com> > > > Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com> > > > --- > > > drivers/mmc/host/sdhci-brcmstb.c | 30 ++++++++++++++++++++++++++++++ > > > 1 file changed, 30 insertions(+) > > > > > > diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c > > > index 8eb57de48e0c..a1ffdd3f1640 100644 > > > --- a/drivers/mmc/host/sdhci-brcmstb.c > > > +++ b/drivers/mmc/host/sdhci-brcmstb.c > > > @@ -250,6 +250,9 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev) > > > struct sdhci_pltfm_host *pltfm_host; > > > const struct of_device_id *match; > > > struct sdhci_brcmstb_priv *priv; > > > + struct clk *master_clk; > > > + u32 base_clock_hz = 0; > > > + u32 actual_clock_mhz; > > > struct sdhci_host *host; > > > struct resource *iomem; > > > struct clk *clk; > > > @@ -330,6 +333,33 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev) > > > if (match_priv->flags & BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT) > > > host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL; > > > > > > + /* Change the base clock frequency if the DT property exists */ > > > + if (device_property_read_u32(&pdev->dev, "max-frequency", > > > + &base_clock_hz) != 0) > > > + goto add_host; > > > > The max-frequency DT property is already being parsed by > > mmc_of_parse() and the value is put in host->mmc->f_max. You could > > probably use that instead, right? > > > > > + > > > + master_clk = devm_clk_get(&pdev->dev, "sdio_freq"); > > > + if (IS_ERR(master_clk)) { > > > + dev_warn(&pdev->dev, "Clock for \"sdio_freq\" not found\n"); > > > + goto add_host; > > > + } else { > > > + res = clk_prepare_enable(master_clk); > > > + if (res) > > > + goto err; > > > + } > > > + > > > + /* set improved clock rate */ > > > + clk_set_rate(master_clk, base_clock_hz); > > > + actual_clock_mhz = clk_get_rate(master_clk) / 1000000; > > > + > > > + host->caps &= ~SDHCI_CLOCK_V3_BASE_MASK; > > > + host->caps |= (actual_clock_mhz << SDHCI_CLOCK_BASE_SHIFT); > > > + /* Disable presets because they are now incorrect */ > > > + host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN; > > > + dev_dbg(&pdev->dev, "Base Clock Frequency changed to %dMHz\n", > > > + actual_clock_mhz); > > > + > > > +add_host: > > > res = sdhci_brcmstb_add_host(host, priv); > > > if (res) > > > goto err; > > > -- > > > 2.17.1 > > > > > > > Kind regards > > Uffe
diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c index 8eb57de48e0c..a1ffdd3f1640 100644 --- a/drivers/mmc/host/sdhci-brcmstb.c +++ b/drivers/mmc/host/sdhci-brcmstb.c @@ -250,6 +250,9 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev) struct sdhci_pltfm_host *pltfm_host; const struct of_device_id *match; struct sdhci_brcmstb_priv *priv; + struct clk *master_clk; + u32 base_clock_hz = 0; + u32 actual_clock_mhz; struct sdhci_host *host; struct resource *iomem; struct clk *clk; @@ -330,6 +333,33 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev) if (match_priv->flags & BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT) host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL; + /* Change the base clock frequency if the DT property exists */ + if (device_property_read_u32(&pdev->dev, "max-frequency", + &base_clock_hz) != 0) + goto add_host; + + master_clk = devm_clk_get(&pdev->dev, "sdio_freq"); + if (IS_ERR(master_clk)) { + dev_warn(&pdev->dev, "Clock for \"sdio_freq\" not found\n"); + goto add_host; + } else { + res = clk_prepare_enable(master_clk); + if (res) + goto err; + } + + /* set improved clock rate */ + clk_set_rate(master_clk, base_clock_hz); + actual_clock_mhz = clk_get_rate(master_clk) / 1000000; + + host->caps &= ~SDHCI_CLOCK_V3_BASE_MASK; + host->caps |= (actual_clock_mhz << SDHCI_CLOCK_BASE_SHIFT); + /* Disable presets because they are now incorrect */ + host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN; + dev_dbg(&pdev->dev, "Base Clock Frequency changed to %dMHz\n", + actual_clock_mhz); + +add_host: res = sdhci_brcmstb_add_host(host, priv); if (res) goto err;