[PATCH v2 0/2] soc: amlogic: ee-pwrc: cleanup init state

[PATCH v2 0/2] soc: amlogic: ee-pwrc: cleanup init state

[Kevin Hilman]

From: Kevin Hilman <khilman@baylibre.com>

Cleanup the PM domain init state and ensure that the driver state
matches the HW state for all domains.

Tested on meson-g12a-sei510 and meson-sm1-sei610 and verified that fb
console still working (VPU power domain.)

Changes since v1:
- always call 'on' 

Kevin Hilman (2):
  soc: amlogic: ee-pwrc: rename get_power
  soc: amlogic: ee-pwrc: ensure driver state maches HW state

 drivers/soc/amlogic/meson-ee-pwrc.c | 57 +++++++++++------------------
 1 file changed, 22 insertions(+), 35 deletions(-)

-- 
2.22.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/2] soc: amlogic: ee-pwrc: rename get_power

[Kevin Hilman]

From: Kevin Hilman <khilman@baylibre.com>

The function named _get_power() is misleading since it returns true
if the power is off.  Rename to _is_off() for better readability.

Signed-off-by: Kevin Hilman <khilman@baylibre.com>
---
 drivers/soc/amlogic/meson-ee-pwrc.c | 34 ++++++++++++++---------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/soc/amlogic/meson-ee-pwrc.c b/drivers/soc/amlogic/meson-ee-pwrc.c
index 5823f5b67d16..dcce8e694a07 100644
--- a/drivers/soc/amlogic/meson-ee-pwrc.c
+++ b/drivers/soc/amlogic/meson-ee-pwrc.c
@@ -56,7 +56,7 @@ struct meson_ee_pwrc_domain_desc {
 	struct meson_ee_pwrc_top_domain *top_pd;
 	unsigned int mem_pd_count;
 	struct meson_ee_pwrc_mem_domain *mem_pd;
-	bool (*get_power)(struct meson_ee_pwrc_domain *pwrc_domain);
+	bool (*is_off)(struct meson_ee_pwrc_domain *pwrc_domain);
 };
 
 struct meson_ee_pwrc_domain_data {
@@ -173,7 +173,7 @@ static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_audio[] = {
 	{ HHI_AUDIO_MEM_PD_REG0, GENMASK(27, 26) },
 };
 
-#define VPU_PD(__name, __top_pd, __mem, __get_power, __resets, __clks)	\
+#define VPU_PD(__name, __top_pd, __mem, __is_off, __resets, __clks)	\
 	{								\
 		.name = __name,						\
 		.reset_names_count = __resets,				\
@@ -181,40 +181,40 @@ static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_audio[] = {
 		.top_pd = __top_pd,					\
 		.mem_pd_count = ARRAY_SIZE(__mem),			\
 		.mem_pd = __mem,					\
-		.get_power = __get_power,				\
+		.is_off = __is_off,				\
 	}
 
-#define TOP_PD(__name, __top_pd, __mem, __get_power)			\
+#define TOP_PD(__name, __top_pd, __mem, __is_off)			\
 	{								\
 		.name = __name,						\
 		.top_pd = __top_pd,					\
 		.mem_pd_count = ARRAY_SIZE(__mem),			\
 		.mem_pd = __mem,					\
-		.get_power = __get_power,				\
+		.is_off = __is_off,				\
 	}
 
 #define MEM_PD(__name, __mem)						\
 	TOP_PD(__name, NULL, __mem, NULL)
 
-static bool pwrc_ee_get_power(struct meson_ee_pwrc_domain *pwrc_domain);
+static bool pwrc_ee_is_off(struct meson_ee_pwrc_domain *pwrc_domain);
 
 static struct meson_ee_pwrc_domain_desc g12a_pwrc_domains[] = {
 	[PWRC_G12A_VPU_ID]  = VPU_PD("VPU", &g12a_pwrc_vpu, g12a_pwrc_mem_vpu,
-				     pwrc_ee_get_power, 11, 2),
+				     pwrc_ee_is_off, 11, 2),
 	[PWRC_G12A_ETH_ID] = MEM_PD("ETH", g12a_pwrc_mem_eth),
 };
 
 static struct meson_ee_pwrc_domain_desc sm1_pwrc_domains[] = {
 	[PWRC_SM1_VPU_ID]  = VPU_PD("VPU", &sm1_pwrc_vpu, sm1_pwrc_mem_vpu,
-				    pwrc_ee_get_power, 11, 2),
+				    pwrc_ee_is_off, 11, 2),
 	[PWRC_SM1_NNA_ID]  = TOP_PD("NNA", &sm1_pwrc_nna, sm1_pwrc_mem_nna,
-				    pwrc_ee_get_power),
+				    pwrc_ee_is_off),
 	[PWRC_SM1_USB_ID]  = TOP_PD("USB", &sm1_pwrc_usb, sm1_pwrc_mem_usb,
-				    pwrc_ee_get_power),
+				    pwrc_ee_is_off),
 	[PWRC_SM1_PCIE_ID] = TOP_PD("PCI", &sm1_pwrc_pci, sm1_pwrc_mem_pcie,
-				    pwrc_ee_get_power),
+				    pwrc_ee_is_off),
 	[PWRC_SM1_GE2D_ID] = TOP_PD("GE2D", &sm1_pwrc_ge2d, sm1_pwrc_mem_ge2d,
-				    pwrc_ee_get_power),
+				    pwrc_ee_is_off),
 	[PWRC_SM1_AUDIO_ID] = MEM_PD("AUDIO", sm1_pwrc_mem_audio),
 	[PWRC_SM1_ETH_ID] = MEM_PD("ETH", g12a_pwrc_mem_eth),
 };
@@ -237,7 +237,7 @@ struct meson_ee_pwrc {
 	struct genpd_onecell_data xlate;
 };
 
-static bool pwrc_ee_get_power(struct meson_ee_pwrc_domain *pwrc_domain)
+static bool pwrc_ee_is_off(struct meson_ee_pwrc_domain *pwrc_domain)
 {
 	u32 reg;
 
@@ -367,7 +367,7 @@ static int meson_ee_pwrc_init_domain(struct platform_device *pdev,
          * we need to power the domain off, otherwise the internal clocks
          * prepare/enable counters won't be in sync.
          */
-	if (dom->num_clks && dom->desc.get_power && !dom->desc.get_power(dom)) {
+	if (dom->num_clks && dom->desc.is_off && !dom->desc.is_off(dom)) {
 		int ret = clk_bulk_prepare_enable(dom->num_clks, dom->clks);
 		if (ret)
 			return ret;
@@ -375,8 +375,8 @@ static int meson_ee_pwrc_init_domain(struct platform_device *pdev,
 		pm_genpd_init(&dom->base, &pm_domain_always_on_gov, false);
 	} else
 		pm_genpd_init(&dom->base, NULL,
-			      (dom->desc.get_power ?
-			       dom->desc.get_power(dom) : true));
+			      (dom->desc.is_off ?
+			       dom->desc.is_off(dom) : true));
 
 	return 0;
 }
@@ -454,7 +454,7 @@ static void meson_ee_pwrc_shutdown(struct platform_device *pdev)
 	for (i = 0 ; i < pwrc->xlate.num_domains ; ++i) {
 		struct meson_ee_pwrc_domain *dom = &pwrc->domains[i];
 
-		if (dom->desc.get_power && !dom->desc.get_power(dom))
+		if (dom->desc.is_off && !dom->desc.is_off(dom))
 			meson_ee_pwrc_off(&dom->base);
 	}
 }
-- 
2.22.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/2] soc: amlogic: ee-pwrc: ensure driver state maches HW state

[Kevin Hilman]

From: Kevin Hilman <khilman@baylibre.com>

During init, ensure that the driver on/off state as well as clock and
reset state matches the hardware state.  Do this by always calling the
drivers 'on' function, and then callling the 'off' function if the
HW state was initially detected as off.

Signed-off-by: Kevin Hilman <khilman@baylibre.com>
---
 drivers/soc/amlogic/meson-ee-pwrc.c | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/drivers/soc/amlogic/meson-ee-pwrc.c b/drivers/soc/amlogic/meson-ee-pwrc.c
index dcce8e694a07..2e8eee0dc166 100644
--- a/drivers/soc/amlogic/meson-ee-pwrc.c
+++ b/drivers/soc/amlogic/meson-ee-pwrc.c
@@ -323,6 +323,8 @@ static int meson_ee_pwrc_init_domain(struct platform_device *pdev,
 				     struct meson_ee_pwrc *pwrc,
 				     struct meson_ee_pwrc_domain *dom)
 {
+	bool is_off;
+
 	dom->pwrc = pwrc;
 	dom->num_rstc = dom->desc.reset_names_count;
 	dom->num_clks = dom->desc.clk_names_count;
@@ -356,27 +358,12 @@ static int meson_ee_pwrc_init_domain(struct platform_device *pdev,
 	dom->base.power_on = meson_ee_pwrc_on;
 	dom->base.power_off = meson_ee_pwrc_off;
 
-	/*
-         * TOFIX: This is a special case for the VPU power domain, which can
-	 * be enabled previously by the bootloader. In this case the VPU
-         * pipeline may be functional but no driver maybe never attach
-         * to this power domain, and if the domain is disabled it could
-         * cause system errors. This is why the pm_domain_always_on_gov
-         * is used here.
-         * For the same reason, the clocks should be enabled in case
-         * we need to power the domain off, otherwise the internal clocks
-         * prepare/enable counters won't be in sync.
-         */
-	if (dom->num_clks && dom->desc.is_off && !dom->desc.is_off(dom)) {
-		int ret = clk_bulk_prepare_enable(dom->num_clks, dom->clks);
-		if (ret)
-			return ret;
-
-		pm_genpd_init(&dom->base, &pm_domain_always_on_gov, false);
-	} else
-		pm_genpd_init(&dom->base, NULL,
-			      (dom->desc.is_off ?
-			       dom->desc.is_off(dom) : true));
+	/* Ensure that driver state matches HW state */
+	is_off = dom->desc.is_off ? dom->desc.is_off(dom) : true;
+	meson_ee_pwrc_on(&dom->base);
+	if (is_off)
+		meson_ee_pwrc_off(&dom->base);
+	pm_genpd_init(&dom->base, NULL, is_off);
 
 	return 0;
 }
-- 
2.22.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/2] soc: amlogic: ee-pwrc: ensure driver state maches HW state

[Neil Armstrong]

On 25/09/2019 23:35, Kevin Hilman wrote:
> From: Kevin Hilman <khilman@baylibre.com>
> 
> During init, ensure that the driver on/off state as well as clock and
> reset state matches the hardware state.  Do this by always calling the
> drivers 'on' function, and then callling the 'off' function if the
> HW state was initially detected as off.
> 
> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
> ---
>  drivers/soc/amlogic/meson-ee-pwrc.c | 29 ++++++++---------------------
>  1 file changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/soc/amlogic/meson-ee-pwrc.c b/drivers/soc/amlogic/meson-ee-pwrc.c
> index dcce8e694a07..2e8eee0dc166 100644
> --- a/drivers/soc/amlogic/meson-ee-pwrc.c
> +++ b/drivers/soc/amlogic/meson-ee-pwrc.c
> @@ -323,6 +323,8 @@ static int meson_ee_pwrc_init_domain(struct platform_device *pdev,
>  				     struct meson_ee_pwrc *pwrc,
>  				     struct meson_ee_pwrc_domain *dom)
>  {
> +	bool is_off;
> +
>  	dom->pwrc = pwrc;
>  	dom->num_rstc = dom->desc.reset_names_count;
>  	dom->num_clks = dom->desc.clk_names_count;
> @@ -356,27 +358,12 @@ static int meson_ee_pwrc_init_domain(struct platform_device *pdev,
>  	dom->base.power_on = meson_ee_pwrc_on;
>  	dom->base.power_off = meson_ee_pwrc_off;
>  
> -	/*
> -         * TOFIX: This is a special case for the VPU power domain, which can
> -	 * be enabled previously by the bootloader. In this case the VPU
> -         * pipeline may be functional but no driver maybe never attach
> -         * to this power domain, and if the domain is disabled it could
> -         * cause system errors. This is why the pm_domain_always_on_gov
> -         * is used here.
> -         * For the same reason, the clocks should be enabled in case
> -         * we need to power the domain off, otherwise the internal clocks
> -         * prepare/enable counters won't be in sync.
> -         */
> -	if (dom->num_clks && dom->desc.is_off && !dom->desc.is_off(dom)) {
> -		int ret = clk_bulk_prepare_enable(dom->num_clks, dom->clks);
> -		if (ret)
> -			return ret;
> -
> -		pm_genpd_init(&dom->base, &pm_domain_always_on_gov, false);
> -	} else
> -		pm_genpd_init(&dom->base, NULL,
> -			      (dom->desc.is_off ?
> -			       dom->desc.is_off(dom) : true));
> +	/* Ensure that driver state matches HW state */
> +	is_off = dom->desc.is_off ? dom->desc.is_off(dom) : true;
> +	meson_ee_pwrc_on(&dom->base);
> +	if (is_off)
> +		meson_ee_pwrc_off(&dom->base);
> +	pm_genpd_init(&dom->base, NULL, is_off);
>  
>  	return 0;
>  }
> 

I don't see what you are trying to solve except simplifying the code.

And the case is more that "matching the clock state" here, the
pm_domain_always_on_gov was is a real case when booting from the Amlogic
U-boot.

The display power domain is complex and as been half solved by using
"simple-framebuffer" on gx and is missing on g12a/g12b/sm1.

For example, Debian installer runs without the modules, but will use
the EFIfb set by U-Boot, but in this precise case :
- the DRM driver isn't loaded
- we can't hook this power domain with EFIfb

When *not* in EFIfb, we use simple-framebuffer on GX, using this
power domain, but it hasn't been copied to G12A.

Personally I'll leave this code until we really tested and checked all
uses cases, not only on the sei510/sei610 using mainline u-boot.

Neil

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/2] soc: amlogic: ee-pwrc: ensure driver state maches HW state

[Kevin Hilman]

Neil Armstrong <narmstrong@baylibre.com> writes:

> On 25/09/2019 23:35, Kevin Hilman wrote:
>> From: Kevin Hilman <khilman@baylibre.com>
>> 
>> During init, ensure that the driver on/off state as well as clock and
>> reset state matches the hardware state.  Do this by always calling the
>> drivers 'on' function, and then callling the 'off' function if the
>> HW state was initially detected as off.

[...]

> I don't see what you are trying to solve except simplifying the code.

Simplifying the code is a worthwhile goal on its own, but that's not the
only thing I'm tring to accomplish.

Part of the goal is make the init less VPU specific and thus make it
more understandable/maintainable.  But the more important part is to
ensure that the driver state and HW state is consistent for all the
domains (not just VPU.)  I've had to debug lots of power-domain issues,
and inconsistiences between HW state and driver state tend to be the
primary cause of problems.

Also I'm generally not a fan of the "don't mess with bootloader state"
approach as that leads to subtle dependencies on specific bootloader
versions that are also difficult to debug.

Antother equally important goal is to actually be able to power-down the
VPU when it's not used.  Right now, we'll never power off the VPU if the
bootloader enabled it, and that seems a bit extreme so I'm looking to
improve that and be able to power off the VPU when not used.

> And the case is more that "matching the clock state" here, the
> pm_domain_always_on_gov was is a real case when booting from the Amlogic
> U-boot.

I'm not understanding what you mean about vendor uboot.  I've done
testing with vendor uboot too:

I tried on g12a-u200, g12a-x96-max, and sm1-khadas-vim3l all of which
have vendor uboot, and I tried before and after $SUBJECT patch.

In all cases, I see the vendor uboot splash screen, and I see the
framebuffer console from linux after kernel boot.  I see the same
behavior before and after my patch.

I also tried on g12b-odroid-n2 (vendor uboot), and there is _no_ uboot
spash screen, but I see the linux framebuffer console come up both
before and after my patch.

What's the specific case you're worried about with vendor uboot?

Also...  note something interesting I noticed on sm1-khadas-vim3l:
before my patch, the framebuffer console appears, but the background is
a bluish/green color.  After my patch, the background is black (as
expected.)  

> The display power domain is complex and as been half solved by using
> "simple-framebuffer" on gx and is missing on g12a/g12b/sm1.
>
> For example, Debian installer runs without the modules, but will use
> the EFIfb set by U-Boot, but in this precise case :
> - the DRM driver isn't loaded
> - we can't hook this power domain with EFIfb

OK, so I agree that this case where we want the display to continue to
work but linux DRM drivers never loaded is a special case.

Is there a way to check if efifb is enabled?  Is the the linux driver
(drivers/video/fbdev/efifb.c) or something else only done by uboot?

If we can detect efifb from the kernel (not just whether the domain is
already on), then a simple addition to my patch like this:

 	if (is_off)
 		meson_ee_pwrc_off(&dom->base);
+	else if (efifb_is_enabled)
+		dom->base.flags |= GENPD_FLAG_ALWAYS_ON;

would force the domain always-on in the case of efifb.  

In fact, now that I think of it, simply adding:

 	if (is_off)
 		meson_ee_pwrc_off(&dom->base);
+	else
+		dom->base.flags |= GENPD_FLAG_ALWAYS_ON;

to my current patch would get back to the same behavior of the existing
driver.  But I still don't like the idea that we can *never* power off
the VPU if the bootloader enables it. :( I'd rather see patches
conditionally adding that flag with detailed explanations as to why it's
needed.

> When *not* in EFIfb, we use simple-framebuffer on GX, using this
> power domain, but it hasn't been copied to G12A.

I don't quite understand what problem simple-framebuffer is
solving. Could you explain that in more detail.

Assuming it is solving something, why can't it be used on g12[ab]/sm1 ?

> Personally I'll leave this code until we really tested and checked all
> uses cases, 

Right, I don't want to break anything on purpose, but I think the
current state of this driver is fragile and difficult to
understand/maintain, so I would be grateful for any help in
understanding the corner cases better, as well as testing them (or
explaining to me how to reproduce them.)

Generally, with long-term maintenance in mind, if I'm forced to choose
between understandable/maintainable code and "covers 100% of corner
cases" I will most often chose the former.

> not only on the sei510/sei610 using mainline u-boot.

See above about all the other boards with vendor uboots also tested.

Kevin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/2] soc: amlogic: ee-pwrc: ensure driver state maches HW state

[Neil Armstrong]

On 26/09/2019 21:08, Kevin Hilman wrote:
> Neil Armstrong <narmstrong@baylibre.com> writes:
> 
>> On 25/09/2019 23:35, Kevin Hilman wrote:
>>> From: Kevin Hilman <khilman@baylibre.com>
>>>
>>> During init, ensure that the driver on/off state as well as clock and
>>> reset state matches the hardware state.  Do this by always calling the
>>> drivers 'on' function, and then callling the 'off' function if the
>>> HW state was initially detected as off.
> 
> [...]
> 
>> I don't see what you are trying to solve except simplifying the code.
> 
> Simplifying the code is a worthwhile goal on its own, but that's not the
> only thing I'm tring to accomplish.

I still find it ugly to power_on a domain to power it off right afterwards.
The issue is with the CCF enable handling which is not in sync with the
HW, if you boot with an already enabled clock, it won't be marked enabled
in CCF, and it's clearly bad when you want to have a fine-tuned gate state
handling.

> 
> Part of the goal is make the init less VPU specific and thus make it
> more understandable/maintainable.  But the more important part is to
> ensure that the driver state and HW state is consistent for all the
> domains (not just VPU.)  I've had to debug lots of power-domain issues,
> and inconsistiences between HW state and driver state tend to be the
> primary cause of problems.
> 
> Also I'm generally not a fan of the "don't mess with bootloader state"
> approach as that leads to subtle dependencies on specific bootloader
> versions that are also difficult to debug.
> 
> Antother equally important goal is to actually be able to power-down the
> VPU when it's not used.  Right now, we'll never power off the VPU if the
> bootloader enabled it, and that seems a bit extreme so I'm looking to
> improve that and be able to power off the VPU when not used.
> 
>> And the case is more that "matching the clock state" here, the
>> pm_domain_always_on_gov was is a real case when booting from the Amlogic
>> U-boot.
> 
> I'm not understanding what you mean about vendor uboot.  I've done
> testing with vendor uboot too:
> 
> I tried on g12a-u200, g12a-x96-max, and sm1-khadas-vim3l all of which
> have vendor uboot, and I tried before and after $SUBJECT patch.
> 
> In all cases, I see the vendor uboot splash screen, and I see the
> framebuffer console from linux after kernel boot.  I see the same
> behavior before and after my patch.
> 
> I also tried on g12b-odroid-n2 (vendor uboot), and there is _no_ uboot
> spash screen, but I see the linux framebuffer console come up both
> before and after my patch.

Thanks for testing all these cases

> 
> What's the specific case you're worried about with vendor uboot?

It's an issue I got when bringing up mainline uboot and the vpu power controller
driver, I had regressions on GXBB and GXL boards.
But it seems it's no more the case on G12A/G12B, we'll see this when
GX support will be added in this driver.

> 
> Also...  note something interesting I noticed on sm1-khadas-vim3l:
> before my patch, the framebuffer console appears, but the background is
> a bluish/green color.  After my patch, the background is black (as
> expected.)  

Yes it's an issue I have on my infinite todo list, it's a different
init done by the latest vendor uboot. I have a fix but it seems it
breaks display when booting other boards.

> 
>> The display power domain is complex and as been half solved by using
>> "simple-framebuffer" on gx and is missing on g12a/g12b/sm1.
>>
>> For example, Debian installer runs without the modules, but will use
>> the EFIfb set by U-Boot, but in this precise case :
>> - the DRM driver isn't loaded
>> - we can't hook this power domain with EFIfb
> 
> OK, so I agree that this case where we want the display to continue to
> work but linux DRM drivers never loaded is a special case.
> 
> Is there a way to check if efifb is enabled?  Is the the linux driver
> (drivers/video/fbdev/efifb.c) or something else only done by uboot?
> 
> If we can detect efifb from the kernel (not just whether the domain is
> already on), then a simple addition to my patch like this:
> 
>  	if (is_off)
>  		meson_ee_pwrc_off(&dom->base);
> +	else if (efifb_is_enabled)
> +		dom->base.flags |= GENPD_FLAG_ALWAYS_ON;
> 
> would force the domain always-on in the case of efifb.  
> 
> In fact, now that I think of it, simply adding:
> 
>  	if (is_off)
>  		meson_ee_pwrc_off(&dom->base);
> +	else
> +		dom->base.flags |= GENPD_FLAG_ALWAYS_ON;
> 
> to my current patch would get back to the same behavior of the existing
> driver.  But I still don't like the idea that we can *never* power off
> the VPU if the bootloader enables it. :( I'd rather see patches
> conditionally adding that flag with detailed explanations as to why it's
> needed.
> 
>> When *not* in EFIfb, we use simple-framebuffer on GX, using this
>> power domain, but it hasn't been copied to G12A.
> 
> I don't quite understand what problem simple-framebuffer is
> solving. Could you explain that in more detail.

simple-framebuffer has the power domain hooked in it's node, so
when u-boot will boot linux with HDMI enabled it will enable
this node and until the DRM driver loads the simple-framebuffer
will live and keep the power domain enabled.

> 
> Assuming it is solving something, why can't it be used on g12[ab]/sm1 ?

It will, but I need to push the patches.

> 
>> Personally I'll leave this code until we really tested and checked all
>> uses cases, 
> 
> Right, I don't want to break anything on purpose, but I think the
> current state of this driver is fragile and difficult to
> understand/maintain, so I would be grateful for any help in
> understanding the corner cases better, as well as testing them (or
> explaining to me how to reproduce them.)
> 
> Generally, with long-term maintenance in mind, if I'm forced to choose
> between understandable/maintainable code and "covers 100% of corner
> cases" I will most often chose the former.
> 
>> not only on the sei510/sei610 using mainline u-boot.
> 
> See above about all the other boards with vendor uboots also tested.

Let's leave apart the vendor uboot issue for g12.

Since display support for G12A will land soon in mainline U-boot, let me
push the DT patches for simple-framebuffer to we have a fallback in
case the DRM driver isn't loaded/built.

We can consider that in case of EFIfb, the simple-framebuffer node would have
been enabled and the power domain hooked and enabled.

Neil

> 
> Kevin
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/2] soc: amlogic: ee-pwrc: ensure driver state maches HW state

[Kevin Hilman]

Neil Armstrong <narmstrong@baylibre.com> writes:

> On 26/09/2019 21:08, Kevin Hilman wrote:
>> Neil Armstrong <narmstrong@baylibre.com> writes:
>> 
>>> On 25/09/2019 23:35, Kevin Hilman wrote:
>>>> From: Kevin Hilman <khilman@baylibre.com>
>>>>
>>>> During init, ensure that the driver on/off state as well as clock and
>>>> reset state matches the hardware state.  Do this by always calling the
>>>> drivers 'on' function, and then callling the 'off' function if the
>>>> HW state was initially detected as off.
>> 
>> [...]
>> 
>>> I don't see what you are trying to solve except simplifying the code.
>> 
>> Simplifying the code is a worthwhile goal on its own, but that's not the
>> only thing I'm tring to accomplish.
>
> I still find it ugly to power_on a domain to power it off right afterwards.
> The issue is with the CCF enable handling which is not in sync with the
> HW, if you boot with an already enabled clock, it won't be marked enabled
> in CCF, and it's clearly bad when you want to have a fine-tuned gate state
> handling.

It's not just the clocks.  The only thing we actually know is the HW
state of the sleep bit (because we read it.)  We don't know the state of
all the mem_pd bits or the iso_reg, nor do we know the state of the
reset lines.  Calling 'on' ensures everything is where we expect, and
we're not relying on the bootloader to do it.

>> Part of the goal is make the init less VPU specific and thus make it
>> more understandable/maintainable.  But the more important part is to
>> ensure that the driver state and HW state is consistent for all the
>> domains (not just VPU.)  I've had to debug lots of power-domain issues,
>> and inconsistiences between HW state and driver state tend to be the
>> primary cause of problems.
>> 
>> Also I'm generally not a fan of the "don't mess with bootloader state"
>> approach as that leads to subtle dependencies on specific bootloader
>> versions that are also difficult to debug.
>> 
>> Antother equally important goal is to actually be able to power-down the
>> VPU when it's not used.  Right now, we'll never power off the VPU if the
>> bootloader enabled it, and that seems a bit extreme so I'm looking to
>> improve that and be able to power off the VPU when not used.
>> 
>>> And the case is more that "matching the clock state" here, the
>>> pm_domain_always_on_gov was is a real case when booting from the Amlogic
>>> U-boot.
>> 
>> I'm not understanding what you mean about vendor uboot.  I've done
>> testing with vendor uboot too:
>> 
>> I tried on g12a-u200, g12a-x96-max, and sm1-khadas-vim3l all of which
>> have vendor uboot, and I tried before and after $SUBJECT patch.
>> 
>> In all cases, I see the vendor uboot splash screen, and I see the
>> framebuffer console from linux after kernel boot.  I see the same
>> behavior before and after my patch.
>> 
>> I also tried on g12b-odroid-n2 (vendor uboot), and there is _no_ uboot
>> spash screen, but I see the linux framebuffer console come up both
>> before and after my patch.
>
> Thanks for testing all these cases
>
>> 
>> What's the specific case you're worried about with vendor uboot?
>
> It's an issue I got when bringing up mainline uboot and the vpu power controller
> driver, I had regressions on GXBB and GXL boards.
> But it seems it's no more the case on G12A/G12B, we'll see this when
> GX support will be added in this driver.
>
>> Also...  note something interesting I noticed on sm1-khadas-vim3l:
>> before my patch, the framebuffer console appears, but the background is
>> a bluish/green color.  After my patch, the background is black (as
>> expected.)  
>
> Yes it's an issue I have on my infinite todo list, it's a different
> init done by the latest vendor uboot. I have a fix but it seems it
> breaks display when booting other boards.
>
>> 
>>> The display power domain is complex and as been half solved by using
>>> "simple-framebuffer" on gx and is missing on g12a/g12b/sm1.
>>>
>>> For example, Debian installer runs without the modules, but will use
>>> the EFIfb set by U-Boot, but in this precise case :
>>> - the DRM driver isn't loaded
>>> - we can't hook this power domain with EFIfb
>> 
>> OK, so I agree that this case where we want the display to continue to
>> work but linux DRM drivers never loaded is a special case.
>> 
>> Is there a way to check if efifb is enabled?  Is the the linux driver
>> (drivers/video/fbdev/efifb.c) or something else only done by uboot?
>> 
>> If we can detect efifb from the kernel (not just whether the domain is
>> already on), then a simple addition to my patch like this:
>> 
>>  	if (is_off)
>>  		meson_ee_pwrc_off(&dom->base);
>> +	else if (efifb_is_enabled)
>> +		dom->base.flags |= GENPD_FLAG_ALWAYS_ON;
>> 
>> would force the domain always-on in the case of efifb.  
>> 
>> In fact, now that I think of it, simply adding:
>> 
>>  	if (is_off)
>>  		meson_ee_pwrc_off(&dom->base);
>> +	else
>> +		dom->base.flags |= GENPD_FLAG_ALWAYS_ON;
>> 
>> to my current patch would get back to the same behavior of the existing
>> driver.  But I still don't like the idea that we can *never* power off
>> the VPU if the bootloader enables it. :( I'd rather see patches
>> conditionally adding that flag with detailed explanations as to why it's
>> needed.
>> 
>>> When *not* in EFIfb, we use simple-framebuffer on GX, using this
>>> power domain, but it hasn't been copied to G12A.
>> 
>> I don't quite understand what problem simple-framebuffer is
>> solving. Could you explain that in more detail.
>
> simple-framebuffer has the power domain hooked in it's node, so
> when u-boot will boot linux with HDMI enabled it will enable
> this node and until the DRM driver loads the simple-framebuffer
> will live and keep the power domain enabled.

Ah, thanks for the explanation.  I didn't realize it was u-boot that was
adding/enabling the simplefb node in the DT.

>> 
>> Assuming it is solving something, why can't it be used on g12[ab]/sm1 ?
>
> It will, but I need to push the patches.
>

OK.

>>> Personally I'll leave this code until we really tested and checked all
>>> uses cases, 
>> 
>> Right, I don't want to break anything on purpose, but I think the
>> current state of this driver is fragile and difficult to
>> understand/maintain, so I would be grateful for any help in
>> understanding the corner cases better, as well as testing them (or
>> explaining to me how to reproduce them.)
>> 
>> Generally, with long-term maintenance in mind, if I'm forced to choose
>> between understandable/maintainable code and "covers 100% of corner
>> cases" I will most often chose the former.
>> 
>>> not only on the sei510/sei610 using mainline u-boot.
>> 
>> See above about all the other boards with vendor uboots also tested.
>
> Let's leave apart the vendor uboot issue for g12.
>
> Since display support for G12A will land soon in mainline U-boot, let me
> push the DT patches for simple-framebuffer to we have a fallback in
> case the DRM driver isn't loaded/built.
>
> We can consider that in case of EFIfb, the simple-framebuffer node would have
> been enabled and the power domain hooked and enabled.

OK, that makes sense to me.

So can I consider this an 'ack' from you for this patch, as long as I
wait to apply it until the simplefb DT patches are also merged?

Thanks,

Kevin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/2] soc: amlogic: ee-pwrc: ensure driver state maches HW state

[Jerome Brunet]

On Fri 27 Sep 2019 at 08:37, Neil Armstrong <narmstrong@baylibre.com> wrote:

> On 26/09/2019 21:08, Kevin Hilman wrote:
>> Neil Armstrong <narmstrong@baylibre.com> writes:
>> 
>>> On 25/09/2019 23:35, Kevin Hilman wrote:
>>>> From: Kevin Hilman <khilman@baylibre.com>
>>>>
>>>> During init, ensure that the driver on/off state as well as clock and
>>>> reset state matches the hardware state.  Do this by always calling the
>>>> drivers 'on' function, and then callling the 'off' function if the
>>>> HW state was initially detected as off.
>> 
>> [...]
>> 
>>> I don't see what you are trying to solve except simplifying the code.
>> 
>> Simplifying the code is a worthwhile goal on its own, but that's not the
>> only thing I'm tring to accomplish.
>
> I still find it ugly to power_on a domain to power it off right afterwards.
> The issue is with the CCF enable handling which is not in sync with the
> HW, if you boot with an already enabled clock, it won't be marked enabled
> in CCF, and it's clearly bad when you want to have a fine-tuned gate state
> handling.
>

CCF should disable unused clock so, in theory, you should not have to
call enable() then disable() to get things in sync.

I suppose the clock in question has the flag CLK_IGNORE_UNUSED (one of
the gates) ?

If the CLK_INGORE_UNUSED is becoming a problem, it would be better to
fix the clock tree rather than adding quirks in consumers.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/2] soc: amlogic: ee-pwrc: ensure driver state maches HW state

[Kevin Hilman]

Jerome Brunet <jbrunet@baylibre.com> writes:

> On Fri 27 Sep 2019 at 08:37, Neil Armstrong <narmstrong@baylibre.com> wrote:
>
>> On 26/09/2019 21:08, Kevin Hilman wrote:
>>> Neil Armstrong <narmstrong@baylibre.com> writes:
>>> 
>>>> On 25/09/2019 23:35, Kevin Hilman wrote:
>>>>> From: Kevin Hilman <khilman@baylibre.com>
>>>>>
>>>>> During init, ensure that the driver on/off state as well as clock and
>>>>> reset state matches the hardware state.  Do this by always calling the
>>>>> drivers 'on' function, and then callling the 'off' function if the
>>>>> HW state was initially detected as off.
>>> 
>>> [...]
>>> 
>>>> I don't see what you are trying to solve except simplifying the code.
>>> 
>>> Simplifying the code is a worthwhile goal on its own, but that's not the
>>> only thing I'm tring to accomplish.
>>
>> I still find it ugly to power_on a domain to power it off right afterwards.
>> The issue is with the CCF enable handling which is not in sync with the
>> HW, if you boot with an already enabled clock, it won't be marked enabled
>> in CCF, and it's clearly bad when you want to have a fine-tuned gate state
>> handling.
>>
>
> CCF should disable unused clock so, in theory, you should not have to
> call enable() then disable() to get things in sync.

But CCF won't disabled unused clocks until late(ish) in the boot
process, which is also when the unused PM domains will be disabled, so I
think there's still a potential for race between the late "disable
unused" features of clocks and pm-domains.

Kevin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel