Re: [PATCH 3/4] ath10k: Enable SRRI/DRRI support on ddr for WCN3990

[pillair]

Hi Kalle,

The checkpatch warning has been addressed in v2 for this patchset.

Thanks,
Rakesh Pillai.

On 2018-04-16 18:57, Kalle Valo wrote:

pill...@codeaurora.org writes:


From: Govind Singh 

SRRI/DRRI are not mapped in the HW Shadow block and can lead
to un-clocked access if common subsystem in the target is
powered down due to idle mode.

To mitigate this problem SRRI/DRRI can be read from
DDR instead of doing an actual hardware read.
Host allocates non cached memory on ddr and configures
the physical address of this memory to the CE hardware.
The hardware updates the RRI on this particular location.
Read SRRI/DRRI from DDR location instead of
direct target read.

Enable retention restore on ddr using hw params to enable
in specific targets.

Signed-off-by: Govind Singh 
Signed-off-by: Rakesh Pillai 


[...]


+   for (i = 0; i < CE_COUNT; i++) {
+   ctrl1_regs = ar->hw_ce_regs->ctrl1_regs->addr;
+   ce_base_addr = ath10k_ce_base_address(ar, i);
+   ath10k_ce_write32(ar, ce_base_addr + ctrl1_regs,
+ ath10k_ce_read32(ar,
+ ce_base_addr + ctrl1_regs) |
+ ar->hw_ce_regs->upd->mask);
+   }


This gives a checkpatch warning:

drivers/net/wireless/ath/ath10k/ce.c:1917: Alignment should match open
parenthesis

You could fix that, and make the code a lot more readable, with
something like this:

tmp = ath10k_ce_read32(ar, ce_base_addr + ctrl1_regs);
tmp |= ar->hw_ce_regs->upd->mask;
ath10k_ce_write32(ar, ce_base_addr + ctrl1_regs, tmp);

Usually it's a good practise avoid making clever tricks, simple code is
a lot easier to read.

Re: [PATCH 3/4] ath10k: Enable SRRI/DRRI support on ddr for WCN3990

[Kalle Valo]

pill...@codeaurora.org writes:

> From: Govind Singh 
>
> SRRI/DRRI are not mapped in the HW Shadow block and can lead
> to un-clocked access if common subsystem in the target is
> powered down due to idle mode.
>
> To mitigate this problem SRRI/DRRI can be read from
> DDR instead of doing an actual hardware read.
> Host allocates non cached memory on ddr and configures
> the physical address of this memory to the CE hardware.
> The hardware updates the RRI on this particular location.
> Read SRRI/DRRI from DDR location instead of
> direct target read.
>
> Enable retention restore on ddr using hw params to enable
> in specific targets.
>
> Signed-off-by: Govind Singh 
> Signed-off-by: Rakesh Pillai 

[...]

> + for (i = 0; i < CE_COUNT; i++) {
> + ctrl1_regs = ar->hw_ce_regs->ctrl1_regs->addr;
> + ce_base_addr = ath10k_ce_base_address(ar, i);
> + ath10k_ce_write32(ar, ce_base_addr + ctrl1_regs,
> +   ath10k_ce_read32(ar,
> +   ce_base_addr + ctrl1_regs) |
> +   ar->hw_ce_regs->upd->mask);
> + }

This gives a checkpatch warning:

drivers/net/wireless/ath/ath10k/ce.c:1917: Alignment should match open 
parenthesis

You could fix that, and make the code a lot more readable, with
something like this:

tmp = ath10k_ce_read32(ar, ce_base_addr + ctrl1_regs);
tmp |= ar->hw_ce_regs->upd->mask;
ath10k_ce_write32(ar, ce_base_addr + ctrl1_regs, tmp);

Usually it's a good practise avoid making clever tricks, simple code is
a lot easier to read.

-- 
Kalle Valo