Re: [PATCH] mfd: db8500-prcmu: Support U8420-sysclk firmware

From: Stephan Gerhold
Date: Sat Oct 26 2019 - 10:42:57 EST

Next message: Wolfram Sang: "[PULL REQUEST] i2c for 5.4"
Previous message: Dan Carpenter: "Re: [PATCH] net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol()"
In reply to: Linus Walleij: "[PATCH] mfd: db8500-prcmu: Support U8420-sysclk firmware"
Next in thread: Linus Walleij: "Re: [PATCH] mfd: db8500-prcmu: Support U8420-sysclk firmware"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sat, Oct 26, 2019 at 02:27:08PM +0200, Linus Walleij wrote:
> There is a distinct version of the Ux500 U8420 variant
> with "sysclk", as can be seen from the vendor code that
> didn't make it upstream, this firmware lacks the
> ULPPLL (ultra-low power phase locked loop) which in
> effect means that the timer clock is instead wired to
> the 32768 Hz always-on clock.
>
> This has some repercussions when enabling the timer
> clock as the code as it stands will disable the timer
> clock on these platforms (lacking the so-called
> "doze mode") and obtaining the wrong rate of the timer
> clock.
>
> The timer frequency is of course needed very early in
> the boot, and as a consequence, we need to shuffle
> around the early PRCMU init code: whereas in the past
> we did not need to look up the PRCMU firmware version
> in the early init, but now we need to know the version
> before the core system timers are registered so we
> restructure the platform callbacks to the PRCMU so as
> not to take any arguments and instead look up the
> resources it needs directly from the device tree
> when initializing.
>
> As we do not yet support any platforms using this
> firmware it is not a regression, but as PostmarketOS
> is starting to support products with this firmware we
> need to fix this up.
>
> The low rate of 32kHz also makes the MTU timer unsuitable
> as delay timer but this needs to be fixed in a separate
> patch.
>
> Cc: arm@xxxxxxxxxx
> Cc: Lee Jones <lee.jones@xxxxxxxxxx>
> Cc: Stephan Gerhold <stephan@xxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> ARM SoC folks: as this mostly affects the MFD subsystems
> I think it'd be best if Lee can merge it, I do not
> plan any other changes to the ARM core files that the
> patch touches.
> ---
> arch/arm/mach-ux500/cpu-db8500.c | 2 +-
> drivers/mfd/db8500-prcmu.c | 66 ++++++++++++++++++++++----------
> include/linux/mfd/db8500-prcmu.h | 4 +-
> include/linux/mfd/dbx500-prcmu.h | 7 ++--
> 4 files changed, 53 insertions(+), 26 deletions(-)
>
> diff --git a/arch/arm/mach-ux500/cpu-db8500.c b/arch/arm/mach-ux500/cpu-db8500.c
> index 3875027ef8fc..e929aaa744c0 100644
> --- a/arch/arm/mach-ux500/cpu-db8500.c
> +++ b/arch/arm/mach-ux500/cpu-db8500.c
> @@ -84,6 +84,7 @@ static void __init ux500_init_irq(void)
> struct resource r;
>
> irqchip_init();
> + prcmu_early_init();
> np = of_find_compatible_node(NULL, NULL, "stericsson,db8500-prcmu");

Maybe it would make sense to give the struct device_node *np pointer as
parameter to prcmu_early_init() to avoid duplicating the lookup of the
device tree node there? But not sure if this is worth it.

> of_address_to_resource(np, 0, &r);
> of_node_put(np);
> @@ -91,7 +92,6 @@ static void __init ux500_init_irq(void)
> pr_err("could not find PRCMU base resource\n");
> return;
> }
> - prcmu_early_init(r.start, r.end-r.start);
> ux500_pm_init(r.start, r.end-r.start);
>
> /* Unlock before init */
> diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c
> index 9df365a15303..e0cf8bc8eb08 100644
> --- a/drivers/mfd/db8500-prcmu.c
> +++ b/drivers/mfd/db8500-prcmu.c
> @@ -27,6 +27,7 @@
> #include <linux/bitops.h>
> #include <linux/fs.h>
> #include <linux/of.h>
> +#include <linux/of_address.h>
> #include <linux/of_irq.h>
> #include <linux/platform_device.h>
> #include <linux/uaccess.h>
> @@ -668,6 +669,18 @@ struct prcmu_fw_version *prcmu_get_fw_version(void)
> return fw_info.valid ? &fw_info.version : NULL;
> }
>
> +static bool prcmu_is_ulppll_disabled(void)
> +{
> + struct prcmu_fw_version *ver;
> +
> + ver = prcmu_get_fw_version();
> +
> + if (ver)
> + return (ver->project == PRCMU_FW_PROJECT_U8420_SYSCLK);
> + else
> + return false;
> +}
> +

Might be personal preference, but this could be written more concise:

static bool prcmu_is_ulppll_disabled(void)
{
struct prcmu_fw_version *ver = prcmu_get_fw_version();

return ver && ver->project == PRCMU_FW_PROJECT_U8420_SYSCLK;
}

> bool prcmu_has_arm_maxopp(void)
> {
> return (readb(tcdm_base + PRCM_AVS_VARM_MAX_OPP) &
> @@ -1308,10 +1321,23 @@ static int request_sysclk(bool enable)
>
> static int request_timclk(bool enable)
> {
> - u32 val = (PRCM_TCR_DOZE_MODE | PRCM_TCR_TENSEL_MASK);
> + u32 val;
> +
> + /*
> + * On the U8420_CLKSEL firmware, the ULP (Ultra Low Power)
> + * PLL is disabled so we cannot use doze mode, this will
> + * stop the clock on this firmware.
> + */
> + if (prcmu_is_ulppll_disabled())
> + val = 0;
> + else
> + val = (PRCM_TCR_DOZE_MODE | PRCM_TCR_TENSEL_MASK);
>
> if (!enable)
> - val |= PRCM_TCR_STOP_TIMERS;
> + val |= PRCM_TCR_STOP_TIMERS |
> + PRCM_TCR_DOZE_MODE |
> + PRCM_TCR_TENSEL_MASK;
> +
> writel(val, PRCM_TCR);
>
> return 0;
> @@ -1615,7 +1641,8 @@ unsigned long prcmu_clock_rate(u8 clock)
> if (clock < PRCMU_NUM_REG_CLOCKS)
> return clock_rate(clock);
> else if (clock == PRCMU_TIMCLK)
> - return ROOT_CLOCK_RATE / 16;
> + return prcmu_is_ulppll_disabled() ?
> + 32768 : ROOT_CLOCK_RATE / 16;
> else if (clock == PRCMU_SYSCLK)
> return ROOT_CLOCK_RATE;
> else if (clock == PRCMU_PLLSOC0)
> @@ -2646,6 +2673,8 @@ static char *fw_project_name(u32 project)
> return "U8520 MBL";
> case PRCMU_FW_PROJECT_U8420:
> return "U8420";
> + case PRCMU_FW_PROJECT_U8420_SYSCLK:
> + return "U8420-sysclk";
> case PRCMU_FW_PROJECT_U9540:
> return "U9540";
> case PRCMU_FW_PROJECT_A9420:
> @@ -2693,27 +2722,18 @@ static int db8500_irq_init(struct device_node *np)
> return 0;
> }
>
> -static void dbx500_fw_version_init(struct platform_device *pdev,
> - u32 version_offset)
> +static void dbx500_fw_version_init(struct device_node *np)
> {
> - struct resource *res;
> void __iomem *tcpm_base;
> u32 version;
>
> - res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> - "prcmu-tcpm");
> - if (!res) {
> - dev_err(&pdev->dev,
> - "Error: no prcmu tcpm memory region provided\n");
> - return;
> - }
> - tcpm_base = ioremap(res->start, resource_size(res));
> + tcpm_base = of_iomap(np, 1);
> if (!tcpm_base) {
> - dev_err(&pdev->dev, "no prcmu tcpm mem region provided\n");
> + pr_err("no prcmu tcpm mem region provided\n");
> return;
> }
>
> - version = readl(tcpm_base + version_offset);
> + version = readl(tcpm_base + DB8500_PRCMU_FW_VERSION_OFFSET);
> fw_info.version.project = (version & 0xFF);
> fw_info.version.api_version = (version >> 8) & 0xFF;
> fw_info.version.func_version = (version >> 16) & 0xFF;
> @@ -2731,7 +2751,7 @@ static void dbx500_fw_version_init(struct platform_device *pdev,
> iounmap(tcpm_base);
> }
>
> -void __init db8500_prcmu_early_init(u32 phy_base, u32 size)
> +void __init db8500_prcmu_early_init(void)
> {
> /*
> * This is a temporary remap to bring up the clocks. It is
> @@ -2740,9 +2760,16 @@ void __init db8500_prcmu_early_init(u32 phy_base, u32 size)
> * clock driver can probe independently. An early initcall will
> * still be needed, but it can be diverted into drivers/clk/ux500.
> */
> - prcmu_base = ioremap(phy_base, size);
> - if (!prcmu_base)
> + struct device_node *np;
> +
> + np = of_find_compatible_node(NULL, NULL, "stericsson,db8500-prcmu");
> + prcmu_base = of_iomap(np, 0);
> + if (!prcmu_base) {
> + of_node_put(np);
> pr_err("%s: ioremap() of prcmu registers failed!\n", __func__);

There is no return here, so of_node_put(np); is called twice in this case.

> + }
> + dbx500_fw_version_init(np);
> + of_node_put(np);
>
> spin_lock_init(&mb0_transfer.lock);
> spin_lock_init(&mb0_transfer.dbb_irqs_lock);
> @@ -3108,7 +3135,6 @@ static int db8500_prcmu_probe(struct platform_device *pdev)
> return -ENOMEM;
> }
> init_prcm_registers();
> - dbx500_fw_version_init(pdev, DB8500_PRCMU_FW_VERSION_OFFSET);
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "prcmu-tcdm");
> if (!res) {
> dev_err(&pdev->dev, "no prcmu tcdm region provided\n");
> diff --git a/include/linux/mfd/db8500-prcmu.h b/include/linux/mfd/db8500-prcmu.h
> index 813710aa2cfd..1fc75d2b4a38 100644
> --- a/include/linux/mfd/db8500-prcmu.h
> +++ b/include/linux/mfd/db8500-prcmu.h
> @@ -489,7 +489,7 @@ struct prcmu_auto_pm_config {
>
> #ifdef CONFIG_MFD_DB8500_PRCMU
>
> -void db8500_prcmu_early_init(u32 phy_base, u32 size);
> +void db8500_prcmu_early_init(void);
> int prcmu_set_rc_a2p(enum romcode_write);
> enum romcode_read prcmu_get_rc_p2a(void);
> enum ap_pwrst prcmu_get_xp70_current_state(void);
> @@ -546,7 +546,7 @@ void db8500_prcmu_write_masked(unsigned int reg, u32 mask, u32 value);
>
> #else /* !CONFIG_MFD_DB8500_PRCMU */
>
> -static inline void db8500_prcmu_early_init(u32 phy_base, u32 size) {}
> +static inline void db8500_prcmu_early_init(void) {}
>
> static inline int prcmu_set_rc_a2p(enum romcode_write code)
> {
> diff --git a/include/linux/mfd/dbx500-prcmu.h b/include/linux/mfd/dbx500-prcmu.h
> index 238401a50d0b..e2571040c7e8 100644
> --- a/include/linux/mfd/dbx500-prcmu.h
> +++ b/include/linux/mfd/dbx500-prcmu.h
> @@ -190,6 +190,7 @@ enum ddr_pwrst {
> #define PRCMU_FW_PROJECT_U8500_MBL2 12 /* Customer specific */
> #define PRCMU_FW_PROJECT_U8520 13
> #define PRCMU_FW_PROJECT_U8420 14
> +#define PRCMU_FW_PROJECT_U8420_SYSCLK 17
> #define PRCMU_FW_PROJECT_A9420 20
> /* [32..63] 9540 and derivatives */
> #define PRCMU_FW_PROJECT_U9540 32
> @@ -211,9 +212,9 @@ struct prcmu_fw_version {
>
> #if defined(CONFIG_UX500_SOC_DB8500)
>
> -static inline void prcmu_early_init(u32 phy_base, u32 size)
> +static inline void prcmu_early_init(void)
> {
> - return db8500_prcmu_early_init(phy_base, size);
> + return db8500_prcmu_early_init();
> }
>
> static inline int prcmu_set_power_state(u8 state, bool keep_ulp_clk,
> @@ -401,7 +402,7 @@ static inline int prcmu_config_a9wdog(u8 num, bool sleep_auto_off)
> }
> #else
>
> -static inline void prcmu_early_init(u32 phy_base, u32 size) {}
> +static inline void prcmu_early_init(void) {}
>
> static inline int prcmu_set_power_state(u8 state, bool keep_ulp_clk,
> bool keep_ap_pll)
> --
> 2.21.0
>

Next message: Wolfram Sang: "[PULL REQUEST] i2c for 5.4"
Previous message: Dan Carpenter: "Re: [PATCH] net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol()"
In reply to: Linus Walleij: "[PATCH] mfd: db8500-prcmu: Support U8420-sysclk firmware"
Next in thread: Linus Walleij: "Re: [PATCH] mfd: db8500-prcmu: Support U8420-sysclk firmware"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]