Re: [PATCH v3 02/16] soc: mediatek: Add MediaTek SCPSYS power domains

From: Nicolas Boichat <drinkcat@chromium.org>
To: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Cc: lkml <linux-kernel@vger.kernel.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Hsin-Yi Wang <hsinyi@chromium.org>,
	Collabora Kernel ML <kernel@collabora.com>,
	Fabien Parent <fparent@baylibre.com>,
	Weiyi Lu <weiyi.lu@mediatek.com>,
	Matthias Brugger <mbrugger@suse.com>,
	linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
	"moderated list:ARM/Mediatek SoC support" 
	<linux-mediatek@lists.infradead.org>
Subject: Re: [PATCH v3 02/16] soc: mediatek: Add MediaTek SCPSYS power domains
Date: Tue, 27 Oct 2020 08:55:58 +0800	[thread overview]
Message-ID: <CANMq1KCe-QtQWXpmwFiCt4vcy0MG==L-C9cSU=o4W4OC5tyWgA@mail.gmail.com> (raw)
In-Reply-To: <20201026175526.2915399-3-enric.balletbo@collabora.com>

On Tue, Oct 27, 2020 at 1:55 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> The System Control Processor System (SCPSYS) has several power management
> related tasks in the system. This driver implements support to handle
> the different power domains supported in order to meet high performance
> and low power requirements.
>
> Co-developed-by: Matthias Brugger <mbrugger@suse.com>
> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
>
> Changes in v3:
> - Return only a boolean for scpsys_domain_is_on()
> - Use regmap_update_bits API when possible.
> - Add some logic to make sure scpsys->domains[id] == NULL or != NULL
>   when needed.
> - Return the child node for scpsys_add_one_domain() call.
> - Remove unneded zeroing num_clks variable.
> - Move the soc specific data to separate include files.
>
> Changes in v2:
> - Get base address from parent syscon. We have now a scpsys syscon node
>   and a child for the SPM (System Power Manager).
> - Use regmap API to acces de base address.
>
>  drivers/soc/mediatek/Kconfig             |  13 +
>  drivers/soc/mediatek/Makefile            |   1 +
>  drivers/soc/mediatek/mt8173-pm-domains.h |  96 +++++
>  drivers/soc/mediatek/mtk-pm-domains.c    | 455 +++++++++++++++++++++++
>  drivers/soc/mediatek/mtk-pm-domains.h    |  65 ++++
>  5 files changed, 630 insertions(+)
>  create mode 100644 drivers/soc/mediatek/mt8173-pm-domains.h
>  create mode 100644 drivers/soc/mediatek/mtk-pm-domains.c
>  create mode 100644 drivers/soc/mediatek/mtk-pm-domains.h
[snip]
> diff --git a/drivers/soc/mediatek/mtk-pm-domains.c b/drivers/soc/mediatek/mtk-pm-domains.c
> new file mode 100644
> index 000000000000..16503d6db6a8
> --- /dev/null
> +++ b/drivers/soc/mediatek/mtk-pm-domains.c
> @@ -0,0 +1,455 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2020 Collabora Ltd.
> + */
> +#include <linux/clk.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_clk.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/regmap.h>
> +#include <linux/soc/mediatek/infracfg.h>
> +
> +#include "mt8173-pm-domains.h"
> +
> +#define MTK_POLL_DELAY_US              10
> +#define MTK_POLL_TIMEOUT               USEC_PER_SEC
> +
> +#define PWR_RST_B_BIT                  BIT(0)
> +#define PWR_ISO_BIT                    BIT(1)
> +#define PWR_ON_BIT                     BIT(2)
> +#define PWR_ON_2ND_BIT                 BIT(3)
> +#define PWR_CLK_DIS_BIT                        BIT(4)
> +
> +struct scpsys_domain {
> +       struct generic_pm_domain genpd;
> +       const struct scpsys_domain_data *data;
> +       struct scpsys *scpsys;
> +       int num_clks;
> +       struct clk_bulk_data *clks;
> +       struct regmap *infracfg;
> +};
> +
> +struct scpsys {
> +       struct device *dev;
> +       struct regmap *base;
> +       const struct scpsys_soc_data *soc_data;
> +       struct genpd_onecell_data pd_data;
> +       struct generic_pm_domain *domains[];
> +};
> +
> +#define to_scpsys_domain(gpd) container_of(gpd, struct scpsys_domain, genpd)
> +
> +static int scpsys_domain_is_on(struct scpsys_domain *pd)

static bool?

> +{
> +       struct scpsys *scpsys = pd->scpsys;
> +       u32 status, status2;
> +
> +       regmap_read(scpsys->base, scpsys->soc_data->pwr_sta_offs, &status);
> +       status &= pd->data->sta_mask;
> +
> +       regmap_read(scpsys->base, scpsys->soc_data->pwr_sta2nd_offs, &status2);
> +       status2 &= pd->data->sta_mask;
> +
> +       /* A domain is on when both status bits are set. */
> +       return status && status2;
> +}
> +
> +static int scpsys_sram_enable(struct scpsys_domain *pd)
> +{
> +       u32 pdn_ack = pd->data->sram_pdn_ack_bits;
> +       struct scpsys *scpsys = pd->scpsys;
> +       unsigned int tmp;
> +
> +       regmap_update_bits(scpsys->base, pd->data->ctl_offs, pd->data->sram_pdn_bits, 0);
> +
> +       /* Either wait until SRAM_PDN_ACK all 1 or 0 */
> +       return regmap_read_poll_timeout(scpsys->base, pd->data->ctl_offs, tmp,
> +                                       (tmp & pdn_ack) == 0, MTK_POLL_DELAY_US, MTK_POLL_TIMEOUT);
> +}
> +
> +static int scpsys_sram_disable(struct scpsys_domain *pd)
> +{
> +       u32 pdn_ack = pd->data->sram_pdn_ack_bits;
> +       struct scpsys *scpsys = pd->scpsys;
> +       unsigned int tmp;
> +
> +       regmap_update_bits(scpsys->base, pd->data->ctl_offs, pd->data->sram_pdn_bits,
> +                          pd->data->sram_pdn_bits);
> +
> +       /* Either wait until SRAM_PDN_ACK all 1 or 0 */
> +       return regmap_read_poll_timeout(scpsys->base, pd->data->ctl_offs, tmp,
> +                                       (tmp & pdn_ack) == pdn_ack, MTK_POLL_DELAY_US,
> +                                       MTK_POLL_TIMEOUT);
> +}
> +
> +static int scpsys_bus_protect_enable(struct scpsys_domain *pd)
> +{
> +       const struct scpsys_bus_prot_data *bp_data = &pd->data->bp_infracfg;
> +
> +       if (!bp_data->bus_prot_mask)
> +               return 0;
> +
> +       return mtk_infracfg_set_bus_protection(pd->infracfg, bp_data->bus_prot_mask,
> +                                              bp_data->bus_prot_reg_update);
> +}
> +
> +static int scpsys_bus_protect_disable(struct scpsys_domain *pd)
> +{
> +       const struct scpsys_bus_prot_data *bp_data = &pd->data->bp_infracfg;
> +
> +       if (!bp_data->bus_prot_mask)
> +               return 0;
> +
> +       return mtk_infracfg_clear_bus_protection(pd->infracfg, bp_data->bus_prot_mask,
> +                                                bp_data->bus_prot_reg_update);
> +}
> +
> +static int scpsys_power_on(struct generic_pm_domain *genpd)
> +{
> +       struct scpsys_domain *pd = container_of(genpd, struct scpsys_domain, genpd);
> +       struct scpsys *scpsys = pd->scpsys;
> +       unsigned int tmp;

bool tmp?

> +       int ret;
> +
> +       ret = clk_bulk_enable(pd->num_clks, pd->clks);
> +       if (ret)
> +               return ret;
> +
> +       /* subsys power on */
> +       regmap_update_bits(scpsys->base, pd->data->ctl_offs, PWR_ON_BIT, PWR_ON_BIT);
> +       regmap_update_bits(scpsys->base, pd->data->ctl_offs, PWR_ON_2ND_BIT, PWR_ON_2ND_BIT);
> +
> +       /* wait until PWR_ACK = 1 */
> +       ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, tmp > 0, MTK_POLL_DELAY_US,

`tmp > 0` is a little weird now that scpsys_domain_is_on returns a
boolean. Just use `tmp`.

> +                                MTK_POLL_TIMEOUT);
> +       if (ret < 0)
> +               goto err_pwr_ack;
> +
> +       regmap_update_bits(scpsys->base, pd->data->ctl_offs, PWR_CLK_DIS_BIT, 0);
> +       regmap_update_bits(scpsys->base, pd->data->ctl_offs, PWR_ISO_BIT, 0);
> +       regmap_update_bits(scpsys->base, pd->data->ctl_offs, PWR_RST_B_BIT, PWR_RST_B_BIT);
> +
> +       ret = scpsys_sram_enable(pd);
> +       if (ret < 0)
> +               goto err_pwr_ack;
> +
> +       ret = scpsys_bus_protect_disable(pd);
> +       if (ret < 0)
> +               goto err_pwr_ack;

Do we need to disable the SRAM in this error path?

> +
> +       return 0;
> +
> +err_pwr_ack:
> +       clk_bulk_disable(pd->num_clks, pd->clks);
> +       return ret;
> +}
> +
> +static int scpsys_power_off(struct generic_pm_domain *genpd)
> +{
> +       struct scpsys_domain *pd = container_of(genpd, struct scpsys_domain, genpd);
> +       struct scpsys *scpsys = pd->scpsys;
> +       unsigned int tmp;
> +       int ret;
> +
> +       ret = scpsys_bus_protect_enable(pd);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = scpsys_sram_disable(pd);
> +       if (ret < 0)
> +               return ret;
> +
> +       /* subsys power off */
> +       regmap_update_bits(scpsys->base, pd->data->ctl_offs, PWR_RST_B_BIT, 0);
> +       regmap_update_bits(scpsys->base, pd->data->ctl_offs, PWR_ISO_BIT, PWR_ISO_BIT);
> +       regmap_update_bits(scpsys->base, pd->data->ctl_offs, PWR_CLK_DIS_BIT, PWR_CLK_DIS_BIT);
> +       regmap_update_bits(scpsys->base, pd->data->ctl_offs, PWR_ON_2ND_BIT, 0);
> +       regmap_update_bits(scpsys->base, pd->data->ctl_offs, PWR_ON_BIT, 0);
> +
> +       /* wait until PWR_ACK = 0 */
> +       ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, tmp == 0, MTK_POLL_DELAY_US,

`!tmp` as condition.

> +                                MTK_POLL_TIMEOUT);
> +       if (ret < 0)
> +               return ret;
> +
> +       clk_bulk_disable(pd->num_clks, pd->clks);
> +
> +       return 0;
> +}
> +
> +static struct
> +generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_node *node)
> +{
> +       const struct scpsys_domain_data *domain_data;
> +       struct scpsys_domain *pd;
> +       int i, ret;
> +       u32 id;
> +
> +       ret = of_property_read_u32(node, "reg", &id);
> +       if (ret) {
> +               dev_err(scpsys->dev, "%pOF: failed to retrieve domain id from reg: %d\n",
> +                       node, ret);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       if (id >= scpsys->soc_data->num_domains) {
> +               dev_err(scpsys->dev, "%pOF: invalid domain id %d\n", node, id);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       domain_data = &scpsys->soc_data->domains[id];

See my comment on v2. domain_data = scpsys->soc_data->domains[id]; is
probably what you want (unless I got it completely wrong...)

> +       if (!domain_data) {
> +               dev_err(scpsys->dev, "%pOF: undefined domain id %d\n", node, id);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       pd = devm_kzalloc(scpsys->dev, sizeof(*pd), GFP_KERNEL);
> +       if (!pd)
> +               return ERR_PTR(-ENOMEM);
> +
> +       pd->data = domain_data;
> +       pd->scpsys = scpsys;
> +
> +       pd->infracfg = syscon_regmap_lookup_by_phandle(node, "mediatek,infracfg");
> +       if (IS_ERR(pd->infracfg))
> +               pd->infracfg = NULL;
> +
> +       pd->num_clks = of_clk_get_parent_count(node);
> +       if (pd->num_clks > 0) {
> +               pd->clks = devm_kcalloc(scpsys->dev, pd->num_clks, sizeof(*pd->clks), GFP_KERNEL);
> +               if (!pd->clks)
> +                       return ERR_PTR(-ENOMEM);
> +       }
[snip]