On Tue, Feb 20, 2024 at 4:53 AM Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote: > > On 16/02/2024 23:32, Sam Protsenko wrote: > > The documentation for struct exynos_cpuclk says .ctrl_base field should > > contain the controller base address. But in reality all Exynos clock > > drivers are passing CPU_SRC register address via CPU_CLK() macro, which > > in turn gets assigned to mentioned .ctrl_base field. Because CPU_SRC > > address usually already has 0x200 offset from controller's base, all > > other register offsets in clk-cpu.c (like DIVs and MUXes) are specified > > as offsets from CPU_SRC offset, and not from controller's base. That > > makes things confusing and not consistent with register offsets provided > > in Exynis clock drivers, also breaking the contract for .ctrl_base field > > Typo: Exynos > Will fix in v2. > > as described in struct exynos_cpuclk doc. Rework all register offsets in > > clk-cpu.c to be actual offsets from controller's base, and fix offsets > > provided to CPU_CLK() macro in all Exynos clock drivers. > > Change is fine and makes sense on devices having separate CPU clock > controller. That's not the case for: > 1. Exynos3250: dedicated CPU clock controller space, but we merged it > into one driver/binding. > 2. Exynos4 and 5250: no obvious dedicated CPU clock controller, but > register layout suggests that there is such, just not explicit. > > In all these cases you provide not the correct offset against explicit > or implicit CPU base, but from main clock controller base. > > Mention it briefly in above commit msg. > Thanks, you are right. That also means the .ctrl_base field (and its documentation) is incorrect. I'll rework the commit message and the field naming to the best of my abilities in v2. > > Best regards, > Krzysztof >
