On 13/12/2024 17:44, Peter Griffin wrote:
> /*
> @@ -325,6 +328,52 @@ struct regmap *exynos_get_pmu_regmap_by_phandle(struct device_node *np,
> }
> EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap_by_phandle);
>
> +/*
> + * CPU_INFORM register hint values which are used by
> + * EL3 firmware (el3mon).
> + */
> +#define CPU_INFORM_CLEAR 0
> +#define CPU_INFORM_C2 1
> +
> +static int cpuhp_pmu_online(unsigned int cpu)
exynos_cpuhp_pmu_online
or gs101_cpuhp_pmu_online
same for offline
> +{
> + void __iomem *base = pmu_context->pmuintrgen_base;
> + u32 reg;
> + u32 mask;
> +
> + /* clear cpu inform hint */
> + regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpu),
> + CPU_INFORM_CLEAR);
> +
> + mask = (1 << cpu);
BIT(cpu)
> +
> + writel(((0 << cpu) & mask), base + GS101_GRP2_INTR_BID_ENABLE);
I am not sure if I follow. You want to zero-out all other bits or enable
all other bits?
> +
> + reg = readl(base + GS101_GRP2_INTR_BID_UPEND) & mask;
> + writel(reg & mask, base + GS101_GRP2_INTR_BID_CLEAR);
reg is &mask twice.
I don't follow this either, are these auto-cleared? It feels like you
wanted to update some bits, but you are updating entire registers in
both cases.
> +
> + return 0;
> +}
> +
> +static int cpuhp_pmu_offline(unsigned int cpu)
> +{
> + void __iomem *base = pmu_context->pmuintrgen_base;
> + u32 reg, mask;
> +
> + /* set cpu inform hint */
> + regmap_write(pmu_context->pmureg, GS101_CPU_INFORM(cpu),
> + CPU_INFORM_C2);
> +
> + writel((1 << cpu), base + GS101_GRP2_INTR_BID_ENABLE);
> +
> + mask = ((1 << cpu) | (1 << (cpu+8)));
What does 8 stands for?
> +
> + reg = readl(base + GS101_GRP1_INTR_BID_UPEND) & mask;
> + writel(reg & mask, base + GS101_GRP1_INTR_BID_CLEAR);
> +
> + return 0;
> +}
> +
> static int exynos_pmu_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -377,6 +426,28 @@ static int exynos_pmu_probe(struct platform_device *pdev)
> pmu_context->pmureg = regmap;
> pmu_context->dev = dev;
>
> + if (pmu_context->pmu_data && pmu_context->pmu_data->pmu_cpuhp) {
> +
Drop blank line
> + pmu_context->pmuintrgen_base =
> + devm_platform_ioremap_resource_byname(pdev, "pmu-intr-gen");
> + /*
> + * To maintain support for older DTs that didn't specify pmu-intr-gen
> + * register region, just issue a warning rather than fail to probe.
> + */
> + if (IS_ERR(pmu_context->pmuintrgen_base)) {
> + dev_warn(&pdev->dev,
> + "failed to map pmu-intr-gen registers\n");
Test old DTS, I think you do write() to the ERR_PTR when
offlining/onlining...
> + } else {
> + cpuhp_setup_state(CPUHP_BP_PREPARE_DYN,
> + "soc/exynos-pmu:prepare",
> + cpuhp_pmu_online, NULL);
> +
> + cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> + "soc/exynos-pmu:online",
> + NULL, cpuhp_pmu_offline);
> + }
> + }
> +
> if (pmu_context->pmu_data && pmu_context->pmu_data->pmu_init)
> pmu_context->pmu_data->pmu_init();
>
Best regards,
Krzysztof
