| Message ID | 1537523181-14578-24-git-send-email-ludovic.Barre@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: mmci: add sdmmc variant for stm32 | expand |
On 21 September 2018 at 11:46, Ludovic Barre <ludovic.Barre@st.com> wrote: > From: Ludovic Barre <ludovic.barre@st.com> > > Some variants could require a reset. > STM32 sdmmc variant needs to reset hardware block > during the power cycle procedure (for re-initialization) > > Signed-off-by: Ludovic Barre <ludovic.barre@st.com> > --- > Documentation/devicetree/bindings/mmc/mmci.txt | 2 ++ > drivers/mmc/host/mmci.c | 9 +++++++++ > drivers/mmc/host/mmci.h | 4 ++++ > 3 files changed, 15 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mmc/mmci.txt b/Documentation/devicetree/bindings/mmc/mmci.txt > index 03796cf..e952707 100644 > --- a/Documentation/devicetree/bindings/mmc/mmci.txt > +++ b/Documentation/devicetree/bindings/mmc/mmci.txt > @@ -11,6 +11,8 @@ Required properties: > - compatible : contains "arm,pl18x", "arm,primecell". > - vmmc-supply : phandle to the regulator device tree node, mentioned > as the VCC/VDD supply in the eMMC/SD specs. > +depend of variant: > +- resets : phandle to internal reset line. > This looks like a thing depending on the SoC, rather than on the variant. Therefore, I suggest you move this to be an optional DT property. Also, please move the DT doc update into a separate patch. > Optional properties: > - arm,primecell-periphid : contains the PrimeCell Peripheral ID, it overrides > diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c > index b1d5bc5..6b3c33f 100644 > --- a/drivers/mmc/host/mmci.c > +++ b/drivers/mmc/host/mmci.c > @@ -37,6 +37,7 @@ > #include <linux/pm_runtime.h> > #include <linux/types.h> > #include <linux/pinctrl/consumer.h> > +#include <linux/reset.h> > > #include <asm/div64.h> > #include <asm/io.h> > @@ -1854,6 +1855,14 @@ static int mmci_probe(struct amba_device *dev, > > dev_dbg(mmc_dev(mmc), "clocking block at %u Hz\n", mmc->f_max); > > + if (variant->reset) { > + host->rst = devm_reset_control_get_exclusive(&dev->dev, NULL); As suggested, let's make this optional and not depending on the variant. > + if (IS_ERR(host->rst)) { > + ret = PTR_ERR(host->rst); > + goto clk_disable; > + } > + } > + > /* Get regulators and the supported OCR mask */ > ret = mmc_regulator_get_supply(mmc); > if (ret) > diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h > index e130689..f0fa185 100644 > --- a/drivers/mmc/host/mmci.h > +++ b/drivers/mmc/host/mmci.h > @@ -247,6 +247,7 @@ struct mmci_host; > * @start_err: bitmask identifying the STARTBITERR bit inside MMCISTATUS > * register. > * @opendrain: bitmask identifying the OPENDRAIN bit inside MMCIPOWER register > + * @reset: true if variant has need reset signal. > */ > struct variant_data { > unsigned int clkreg; > @@ -285,6 +286,7 @@ struct variant_data { > bool qcom_dml; > bool reversed_irq_handling; > bool mmcimask1; > + bool reset; > unsigned int irq_pio_mask; > u32 start_err; > u32 opendrain; > @@ -318,6 +320,8 @@ struct mmci_host { > struct clk *clk; > bool singleirq; > > + struct reset_control *rst; > + > spinlock_t lock; > > unsigned int mclk; > -- > 2.7.4 > Kind regards Uffe
hi Ulf On 10/01/2018 11:30 AM, Ulf Hansson wrote: > On 21 September 2018 at 11:46, Ludovic Barre <ludovic.Barre@st.com> wrote: >> From: Ludovic Barre <ludovic.barre@st.com> >> >> Some variants could require a reset. >> STM32 sdmmc variant needs to reset hardware block >> during the power cycle procedure (for re-initialization) >> >> Signed-off-by: Ludovic Barre <ludovic.barre@st.com> >> --- >> Documentation/devicetree/bindings/mmc/mmci.txt | 2 ++ >> drivers/mmc/host/mmci.c | 9 +++++++++ >> drivers/mmc/host/mmci.h | 4 ++++ >> 3 files changed, 15 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/mmc/mmci.txt b/Documentation/devicetree/bindings/mmc/mmci.txt >> index 03796cf..e952707 100644 >> --- a/Documentation/devicetree/bindings/mmc/mmci.txt >> +++ b/Documentation/devicetree/bindings/mmc/mmci.txt >> @@ -11,6 +11,8 @@ Required properties: >> - compatible : contains "arm,pl18x", "arm,primecell". >> - vmmc-supply : phandle to the regulator device tree node, mentioned >> as the VCC/VDD supply in the eMMC/SD specs. >> +depend of variant: >> +- resets : phandle to internal reset line. >> > > This looks like a thing depending on the SoC, rather than on the > variant. Therefore, I suggest you move this to be an optional DT > property. could be replaced by: Optional properties: - resets : phandle to internal reset line. Should be defined for sdmmc variant. > > Also, please move the DT doc update into a separate patch. OK > >> Optional properties: >> - arm,primecell-periphid : contains the PrimeCell Peripheral ID, it overrides >> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c >> index b1d5bc5..6b3c33f 100644 >> --- a/drivers/mmc/host/mmci.c >> +++ b/drivers/mmc/host/mmci.c >> @@ -37,6 +37,7 @@ >> #include <linux/pm_runtime.h> >> #include <linux/types.h> >> #include <linux/pinctrl/consumer.h> >> +#include <linux/reset.h> >> >> #include <asm/div64.h> >> #include <asm/io.h> >> @@ -1854,6 +1855,14 @@ static int mmci_probe(struct amba_device *dev, >> >> dev_dbg(mmc_dev(mmc), "clocking block at %u Hz\n", mmc->f_max); >> >> + if (variant->reset) { >> + host->rst = devm_reset_control_get_exclusive(&dev->dev, NULL); > > As suggested, let's make this optional and not depending on the variant. I done like that because is required for my variant (if no reset, no power cycle for sdmmc). If you prefer, I could move to optional with "devm_reset_control_get_optional_exclusive" And I add a comment in mmci dt binding to specify that not optional for sdmmc. (see above) > >> + if (IS_ERR(host->rst)) { >> + ret = PTR_ERR(host->rst); >> + goto clk_disable; >> + } >> + } >> + >> /* Get regulators and the supported OCR mask */ >> ret = mmc_regulator_get_supply(mmc); >> if (ret) >> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h >> index e130689..f0fa185 100644 >> --- a/drivers/mmc/host/mmci.h >> +++ b/drivers/mmc/host/mmci.h >> @@ -247,6 +247,7 @@ struct mmci_host; >> * @start_err: bitmask identifying the STARTBITERR bit inside MMCISTATUS >> * register. >> * @opendrain: bitmask identifying the OPENDRAIN bit inside MMCIPOWER register >> + * @reset: true if variant has need reset signal. >> */ >> struct variant_data { >> unsigned int clkreg; >> @@ -285,6 +286,7 @@ struct variant_data { >> bool qcom_dml; >> bool reversed_irq_handling; >> bool mmcimask1; >> + bool reset; >> unsigned int irq_pio_mask; >> u32 start_err; >> u32 opendrain; >> @@ -318,6 +320,8 @@ struct mmci_host { >> struct clk *clk; >> bool singleirq; >> >> + struct reset_control *rst; >> + >> spinlock_t lock; >> >> unsigned int mclk; >> -- >> 2.7.4 >> > > Kind regards > Uffe >
[...] >>> @@ -1854,6 +1855,14 @@ static int mmci_probe(struct amba_device *dev, >>> >>> dev_dbg(mmc_dev(mmc), "clocking block at %u Hz\n", mmc->f_max); >>> >>> + if (variant->reset) { >>> + host->rst = devm_reset_control_get_exclusive(&dev->dev, >>> NULL); >> >> >> As suggested, let's make this optional and not depending on the variant. > > > I done like that because is required for my variant (if no reset, no power > cycle for sdmmc). The point is, I think don't think it's correct to ties this to the variant, but rather I think it depends on the behavior of the SoC. > > If you prefer, I could move to optional with > "devm_reset_control_get_optional_exclusive" > And I add a comment in mmci dt binding to specify that not optional > for sdmmc. (see above) Alright, fair enough. Let' do that. [...] Kind regards Uffe
diff --git a/Documentation/devicetree/bindings/mmc/mmci.txt b/Documentation/devicetree/bindings/mmc/mmci.txt index 03796cf..e952707 100644 --- a/Documentation/devicetree/bindings/mmc/mmci.txt +++ b/Documentation/devicetree/bindings/mmc/mmci.txt @@ -11,6 +11,8 @@ Required properties: - compatible : contains "arm,pl18x", "arm,primecell". - vmmc-supply : phandle to the regulator device tree node, mentioned as the VCC/VDD supply in the eMMC/SD specs. +depend of variant: +- resets : phandle to internal reset line. Optional properties: - arm,primecell-periphid : contains the PrimeCell Peripheral ID, it overrides diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c index b1d5bc5..6b3c33f 100644 --- a/drivers/mmc/host/mmci.c +++ b/drivers/mmc/host/mmci.c @@ -37,6 +37,7 @@ #include <linux/pm_runtime.h> #include <linux/types.h> #include <linux/pinctrl/consumer.h> +#include <linux/reset.h> #include <asm/div64.h> #include <asm/io.h> @@ -1854,6 +1855,14 @@ static int mmci_probe(struct amba_device *dev, dev_dbg(mmc_dev(mmc), "clocking block at %u Hz\n", mmc->f_max); + if (variant->reset) { + host->rst = devm_reset_control_get_exclusive(&dev->dev, NULL); + if (IS_ERR(host->rst)) { + ret = PTR_ERR(host->rst); + goto clk_disable; + } + } + /* Get regulators and the supported OCR mask */ ret = mmc_regulator_get_supply(mmc); if (ret) diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h index e130689..f0fa185 100644 --- a/drivers/mmc/host/mmci.h +++ b/drivers/mmc/host/mmci.h @@ -247,6 +247,7 @@ struct mmci_host; * @start_err: bitmask identifying the STARTBITERR bit inside MMCISTATUS * register. * @opendrain: bitmask identifying the OPENDRAIN bit inside MMCIPOWER register + * @reset: true if variant has need reset signal. */ struct variant_data { unsigned int clkreg; @@ -285,6 +286,7 @@ struct variant_data { bool qcom_dml; bool reversed_irq_handling; bool mmcimask1; + bool reset; unsigned int irq_pio_mask; u32 start_err; u32 opendrain; @@ -318,6 +320,8 @@ struct mmci_host { struct clk *clk; bool singleirq; + struct reset_control *rst; + spinlock_t lock; unsigned int mclk;