Re: [PATCH V2 23/27] mmc: mmci: add variant property to request a reset

From: Ulf Hansson
Date: Mon Oct 01 2018 - 05:32:00 EST


On 21 September 2018 at 11:46, Ludovic Barre <ludovic.Barre@xxxxxx> wrote:
> From: Ludovic Barre <ludovic.barre@xxxxxx>
>
> 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@xxxxxx>
> ---
> 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