Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20220929175602.19946-1-hal.feng@linux.starfivetech.com>
References: <20220929143225.17907-1-hal.feng@linux.starfivetech.com> <20220929175602.19946-1-hal.feng@linux.starfivetech.com>
Subject: Re: [PATCH v1 15/30] clk: starfive: Use regmap APIs to operate registers
From:   Stephen Boyd <sboyd@kernel.org>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Marc Zyngier <maz@kernel.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Michael Turquette <mturquette@baylibre.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        Emil Renner Berthing <kernel@esmil.dk>,
        Hal Feng <hal.feng@linux.starfivetech.com>,
        linux-kernel@vger.kernel.org
To:     Hal Feng <hal.feng@linux.starfivetech.com>,
        devicetree@vger.kernel.org, linux-clk@vger.kernel.org,
        linux-gpio@vger.kernel.org, linux-riscv@lists.infradead.org
Date:   Fri, 30 Sep 2022 14:48:21 -0700
User-Agent: alot/0.10
Message-Id: <20220930214824.A14ACC433D6@smtp.kernel.org>
Precedence: bulk

Quoting Hal Feng (2022-09-29 10:56:02)
> Clock registers address region is shared with reset controller
> on the new StarFive JH7110 SoC. Change to use regmap framework
> to allow base address sharing and preparation for JH7110 clock
> support.

Do the reset and clk parts share actual registers, where we would need
to lock between rmw? Or is regmap just nice to have because it wraps up
the register APIs with some extra features?

>=20
> Signed-off-by: Hal Feng <hal.feng@linux.starfivetech.com>
> ---
[...]
> diff --git a/drivers/clk/starfive/clk-starfive-jh7100.c b/drivers/clk/sta=
rfive/clk-starfive-jh7100.c
> index 014e36f17595..410aa6e06842 100644
> --- a/drivers/clk/starfive/clk-starfive-jh7100.c
> +++ b/drivers/clk/starfive/clk-starfive-jh7100.c
> @@ -10,6 +10,7 @@
>  #include <linux/clk-provider.h>
>  #include <linux/device.h>
>  #include <linux/init.h>
> +#include <linux/mfd/syscon.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/platform_device.h>
> =20
> @@ -295,11 +296,13 @@ static int __init clk_starfive_jh7100_probe(struct =
platform_device *pdev)
>         if (!priv)
>                 return -ENOMEM;
> =20
> -       spin_lock_init(&priv->rmw_lock);
>         priv->dev =3D &pdev->dev;
> -       priv->base =3D devm_platform_ioremap_resource(pdev, 0);
> -       if (IS_ERR(priv->base))
> -               return PTR_ERR(priv->base);
> +       priv->regmap =3D device_node_to_regmap(priv->dev->of_node);

This is sad. Why do we need to make a syscon? Can we instead use the
auxiliary bus to make a reset device that either gets a regmap made here
in this driver or uses a void __iomem * mapped with ioremap
(priv->base)?

> +       if (IS_ERR(priv->regmap)) {
> +               dev_err(priv->dev, "failed to get regmap (error %ld)\n",
> +                       PTR_ERR(priv->regmap));
> +               return PTR_ERR(priv->regmap);