linux-kernel - Re: [PATCH v2] iio: adc: aspeed: Deassert reset in probe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 31 Oct 2017 12:58:25 +0100
From:   Philipp Zabel <philipp.zabel@...il.com>
To:     Joel Stanley <joel@....id.au>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        Rick Altherr <raltherr@...gle.com>,
        Rob Herring <robh+dt@...nel.org>,
        Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] iio: adc: aspeed: Deassert reset in probe

On Tue, Oct 31, 2017 at 3:12 AM, Joel Stanley <joel@....id.au> wrote:
> The ASPEED SoC must deassert a reset in order to use the ADC peripheral.
>
> The device tree bindings are updated to document the resets phandle, and
> the example is updated to match what is expected for both the reset and
> clock phandle. Note that the bindings should have always had the reset
> controller, as the hardware is unusable without it.
>
> Signed-off-by: Joel Stanley <joel@....id.au>

It is unfortunate that this has to break DT (theoretical) backwards
compatibility, but given that the old bindings never worked,
this is better than to pretend a required reset is optional.

Reviewed-by: Philipp Zabel <philipp.zabel@...il.com>

regards
Philipp

> ---
> v2:
>  - Ensure disabling path unwinds in opposite order as the enable path
>  - Note that the bindings were incorrect without the reset phandle,
>  and for the system to be usable we must update them. No one was
>  (successfully) using these bindings/driver before without out of tree
>  hacks in mach-aspeed, as it would not have worked.
>
>  .../devicetree/bindings/iio/adc/aspeed_adc.txt     |  4 +++-
>  drivers/iio/adc/aspeed_adc.c                       | 25 ++++++++++++++++------
>  2 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
> index 674e133b7cd7..034fc2ba100e 100644
> --- a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
> +++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
> @@ -8,6 +8,7 @@ Required properties:
>  - reg: memory window mapping address and length
>  - clocks: Input clock used to derive the sample clock. Expected to be the
>            SoC's APB clock.
> +- resets: Reset controller phandle
>  - #io-channel-cells: Must be set to <1> to indicate channels are selected
>                       by index.
>
> @@ -15,6 +16,7 @@ Example:
>         adc@...e9000 {
>                 compatible = "aspeed,ast2400-adc";
>                 reg = <0x1e6e9000 0xb0>;
> -               clocks = <&clk_apb>;
> +               clocks = <&syscon ASPEED_CLK_APB>;
> +               resets = <&syscon ASPEED_RESET_ADC>;
>                 #io-channel-cells = <1>;
>         };
> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
> index 8a958d5f1905..327a49ba1991 100644
> --- a/drivers/iio/adc/aspeed_adc.c
> +++ b/drivers/iio/adc/aspeed_adc.c
> @@ -17,6 +17,7 @@
>  #include <linux/module.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> +#include <linux/reset.h>
>  #include <linux/spinlock.h>
>  #include <linux/types.h>
>
> @@ -53,11 +54,12 @@ struct aspeed_adc_model_data {
>  };
>
>  struct aspeed_adc_data {
> -       struct device   *dev;
> -       void __iomem    *base;
> -       spinlock_t      clk_lock;
> -       struct clk_hw   *clk_prescaler;
> -       struct clk_hw   *clk_scaler;
> +       struct device           *dev;
> +       void __iomem            *base;
> +       spinlock_t              clk_lock;
> +       struct clk_hw           *clk_prescaler;
> +       struct clk_hw           *clk_scaler;
> +       struct reset_control    *rst;
>  };
>
>  #define ASPEED_CHAN(_idx, _data_reg_addr) {                    \
> @@ -217,6 +219,15 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>                 goto scaler_error;
>         }
>
> +       data->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +       if (IS_ERR(data->rst)) {
> +               dev_err(&pdev->dev,
> +                       "invalid or missing reset controller device tree entry");
> +               ret = PTR_ERR(data->rst);
> +               goto reset_error;
> +       }
> +       reset_control_deassert(data->rst);
> +
>         model_data = of_device_get_match_data(&pdev->dev);
>
>         if (model_data->wait_init_sequence) {
> @@ -263,9 +274,10 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>         writel(ASPEED_OPERATION_MODE_POWER_DOWN,
>                 data->base + ASPEED_REG_ENGINE_CONTROL);
>         clk_disable_unprepare(data->clk_scaler->clk);
> +reset_error:
> +       reset_control_assert(data->rst);
>  clk_enable_error:
>         clk_hw_unregister_divider(data->clk_scaler);
> -
>  scaler_error:
>         clk_hw_unregister_divider(data->clk_prescaler);
>         return ret;
> @@ -280,6 +292,7 @@ static int aspeed_adc_remove(struct platform_device *pdev)
>         writel(ASPEED_OPERATION_MODE_POWER_DOWN,
>                 data->base + ASPEED_REG_ENGINE_CONTROL);
>         clk_disable_unprepare(data->clk_scaler->clk);
> +       reset_control_assert(data->rst);
>         clk_hw_unregister_divider(data->clk_scaler);
>         clk_hw_unregister_divider(data->clk_prescaler);
>
> --
> 2.14.1
>