Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Kevin Hilman <khilman@baylibre.com>
To:     Neil Armstrong <narmstrong@baylibre.com>, jbrunet@baylibre.com
Cc:     linux-arm-kernel@lists.infradead.org,
        linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC 04/11] soc: amlogic: Add support for SM1 power controller
In-Reply-To: <98bda35e-1b4c-404c-fdbd-eaef9ecf38a6@baylibre.com>
References: <20190701104705.18271-1-narmstrong@baylibre.com> <20190701104705.18271-5-narmstrong@baylibre.com> <7hftlwvhdk.fsf@baylibre.com> <98bda35e-1b4c-404c-fdbd-eaef9ecf38a6@baylibre.com>
Date:   Tue, 20 Aug 2019 12:19:11 -0700
Message-ID: <7hd0gzejbk.fsf@baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Neil Armstrong <narmstrong@baylibre.com> writes:

> On 20/08/2019 01:56, Kevin Hilman wrote:
>> Neil Armstrong <narmstrong@baylibre.com> writes:
>> 
>>> Add support for the General Purpose Amlogic SM1 Power controller,
>>> dedicated to the PCIe, USB, NNA and GE2D Power Domains.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> 
>> I like this driver in general, but as I look at all the EE power domains
>> for GX, G12 and SM1 they are really very similar.  I had started to
>> generalize the gx-pwrc-vpu driver and it ends up looking just like this.
>
> Yes I developed it to be generic, but when starting to fill up the GXBB/GXL/G12A
> domains, except the VPU, they only need the PD parts.
>
>> 
>> I think this driver could be generalized just a little bit more and then
>> replace the the GX-specific VPU one, and AFAICT, then be used across all
>> the 64-bit SoCs, and be called "meson-pwrc-ee" or something like that...
>> 
>>> ---
>>>  drivers/soc/amlogic/Kconfig          |  11 ++
>>>  drivers/soc/amlogic/Makefile         |   1 +
>>>  drivers/soc/amlogic/meson-sm1-pwrc.c | 245 +++++++++++++++++++++++++++
>>>  3 files changed, 257 insertions(+)
>>>  create mode 100644 drivers/soc/amlogic/meson-sm1-pwrc.c
>>>
>>> diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
>>> index 5501ad5650b2..596f1afef1a7 100644
>>> --- a/drivers/soc/amlogic/Kconfig
>>> +++ b/drivers/soc/amlogic/Kconfig
>>> @@ -36,6 +36,17 @@ config MESON_GX_PM_DOMAINS
>>>  	  Say yes to expose Amlogic Meson GX Power Domains as
>>>  	  Generic Power Domains.
>>>  
>>> +config MESON_SM1_PM_DOMAINS
>>> +	bool "Amlogic Meson SM1 Power Domains driver"
>>> +	depends on ARCH_MESON || COMPILE_TEST
>>> +	depends on PM && OF
>>> +	default ARCH_MESON
>>> +	select PM_GENERIC_DOMAINS
>>> +	select PM_GENERIC_DOMAINS_OF
>>> +	help
>>> +	  Say yes to expose Amlogic Meson SM1 Power Domains as
>>> +	  Generic Power Domains.
>>> +
>>>  config MESON_MX_SOCINFO
>>>  	bool "Amlogic Meson MX SoC Information driver"
>>>  	depends on ARCH_MESON || COMPILE_TEST
>>> diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile
>>> index bf2d109f61e9..f99935499ee6 100644
>>> --- a/drivers/soc/amlogic/Makefile
>>> +++ b/drivers/soc/amlogic/Makefile
>>> @@ -3,3 +3,4 @@ obj-$(CONFIG_MESON_CLK_MEASURE) += meson-clk-measure.o
>>>  obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o
>>>  obj-$(CONFIG_MESON_GX_PM_DOMAINS) += meson-gx-pwrc-vpu.o
>>>  obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o
>>> +obj-$(CONFIG_MESON_SM1_PM_DOMAINS) += meson-sm1-pwrc.o
>>> diff --git a/drivers/soc/amlogic/meson-sm1-pwrc.c b/drivers/soc/amlogic/meson-sm1-pwrc.c
>>> new file mode 100644
>>> index 000000000000..9ece1d06f417
>>> --- /dev/null
>>> +++ b/drivers/soc/amlogic/meson-sm1-pwrc.c
>>> @@ -0,0 +1,245 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * Copyright (c) 2017 BayLibre, SAS
>>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>>> + */
>>> +
>>> +#include <linux/of_address.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/pm_domain.h>
>>> +#include <linux/bitfield.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/of_device.h>
>>> +#include <dt-bindings/power/meson-sm1-power.h>
>>> +
>>> +/* AO Offsets */
>>> +
>>> +#define AO_RTI_GEN_PWR_SLEEP0		(0x3a << 2)
>>> +#define AO_RTI_GEN_PWR_ISO0		(0x3b << 2)
>>> +
>>> +/* HHI Offsets */
>>> +
>>> +#define HHI_MEM_PD_REG0			(0x40 << 2)
>>> +#define HHI_NANOQ_MEM_PD_REG0		(0x46 << 2)
>>> +#define HHI_NANOQ_MEM_PD_REG1		(0x47 << 2)
>>> +
>>> +struct meson_sm1_pwrc;
>>> +
>>> +struct meson_sm1_pwrc_mem_domain {
>>> +	unsigned int reg;
>>> +	unsigned int mask;
>>> +};
>>> +
>>> +struct meson_sm1_pwrc_domain_desc {
>>> +	char *name;
>>> +	unsigned int sleep_reg;
>>> +	unsigned int sleep_bit;
>>> +	unsigned int iso_reg;
>>> +	unsigned int iso_bit;
>>> +	unsigned int mem_pd_count;
>>> +	struct meson_sm1_pwrc_mem_domain *mem_pd;
>>> +};
>> 
>> If you add resets and clocks (using clk bulk like my other proposed
>> patch to gx-pwrc-vpu) then this could be used for VPU also.  We could
>> ignore my clk bulk patch and then just deprecate the old driver and use
>> this one for everything.
>> 
>> We would just need SoC-specific tables selected by compatible-string to
>> select the memory pds, and the clocks and resets could (optionaly) come
>> from the DT.
>
> Could you elaborate ?
>
> Do you mean I should slit out the memory PDs as different compatible ?

You currently create all these SoC-specific `mem_domain` tables.  We'll
need more of those for the other SoCs, so my suggestion was that, in
order to use this across multiple SoCs, you select the set of mem_domain
tables based on compatible string.

That was just my first idea.  If you have a better idea, I'm open to
that too.

> Let me try to fit the VPU stuff in it.

Great, thanks!

Kevin