Re: [PATCH v2 2/2] firmware: add exynos acpm driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On 23/10/2024 11:53, Tudor Ambarus wrote:
> 
> 
> On 10/23/24 10:00 AM, Krzysztof Kozlowski wrote:
>>>>>> I also cannot find any piece of code setting several of above, e.g. tx_base
>>>>> I'm not writing any SRAM configuration fields, these fields are used to
>>>>> read/retrive the channel parameters from SRAM.
>>>> I meany tx_base is always 0. Where is this property set? Ever?
>>> It's not zero. My assumption is it is set in the acpm firmware, but I
>> Where is any assignment to this member?
> 
> In probe() you'll see that exynos_acpm->shmem is a pointer in SRAM to a
> struct exynos_acpm_shmem __iomem *shmem;
> 
> Then in:
> 
> static int exynos_acpm_chans_init()
> {
> 	struct exynos_acpm_shmem_chan __iomem *shmem_chans, *shmem_chan;
> 	struct exynos_acpm_shmem __iomem *shmem = exynos_acpm->shmem;
> 	...
> 
> 	shmem_chans = exynos_acpm_get_iomem_addr(exynos_acpm->sram_base,
> 						 &shmem->chans);
> 	...
> }
> 
> shmem->chans is not initialized (or tx_base). I'm using its address in
> SRAM (&shmem->chans) which I then read it with readl_relaxed().
> 
> I guess one can do the same using offsetof:
> shmem_chans = readl_realaxed(shmem + offsetof(struct exynos_acpm_shmem,
> 					      chans));
> 

I see, the code and the naming is confusing. Two exynos_acpm_shmem_chan
variables and one exynos_acpm_shmem. shmem_chans is used as an array,
but nowhere pointed or indicated that it is array of some size.

All this could be clearer if exynos_acpm_shmem_chan was packed, because
then it is obvious it points to defined memory, but maybe packed is not
correct here? Probably splitting all this into logical chunks would be
useful. Like not mixing reading offsets with reading values, because I
really have to spend a lot of time to identify which one is which in
exynos_acpm_chans_init().

Best regards,
Krzysztof