Sender: kvalo=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.4.1 smtp.codeaurora.org 16162C4338F
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Anilkumar Kolli <akolli@codeaurora.org>
Cc:     ath11k@lists.infradead.org, linux-wireless@vger.kernel.org,
        devicetree@vger.kernel.org, robh@kernel.org
Subject: Re: [PATCH v2 2/2] ath11k: Use reserved host DDR addresses from DT for PCI devices
References: <1637082058-6398-1-git-send-email-akolli@codeaurora.org>
        <1637082058-6398-2-git-send-email-akolli@codeaurora.org>
        <87wnl7dx69.fsf@codeaurora.org>
        <b1617fc74da2d65d663a82e4a4f538e2@codeaurora.org>
Date:   Fri, 19 Nov 2021 15:08:17 +0200
In-Reply-To: <b1617fc74da2d65d663a82e4a4f538e2@codeaurora.org> (Anilkumar
        Kolli's message of "Thu, 18 Nov 2021 16:05:21 +0530")
Message-ID: <878rxkcmym.fsf@codeaurora.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Anilkumar Kolli <akolli@codeaurora.org> writes:

> On 2021-11-17 13:35, Kalle Valo wrote:
>> Anilkumar Kolli <akolli@codeaurora.org> writes:
>>
>>> Host DDR memory (contiguous 45 MB in mode-0 or 15 MB in mode-2)
>>> is reserved through DT entries for firmware usage. Send the base
>>> address and size from DT entries.
>>>
>>> If DT entry is available, PCI devices work with
>>> fixed_mem_region else host allocates multiple segments.
>>>
>>> IPQ8074 on HK10 board supports multiple PCI devices.
>>> IPQ8074 + QCN9074 is tested with this patch.
>>>
>>> Tested-on: QCN9074 hw1.0 PCI
>>> WLAN.HK.2.4.0.1-01838-QCAHKSWPL_SILICONZ-1
>>>
>>> Signed-off-by: Anilkumar Kolli <akolli@codeaurora.org>

[...]

>>> +		goto no_dt_entry;
>>> +	}
>>> +
>>> +	reg_end = reg + len / (aw * sw);
>>> +
>>> +	do {
>>> +		start = of_read_number(reg, aw);
>>> +		reg += aw;
>>> +		size = of_read_number(reg, sw);
>>
>> of_read_number() takes 'const __be32 *cell' but reg is 'u32 *'?
>>
> Yes.

My point here was that doesn't this mixing of __be32 and u32 when cause
a sparse warning?

>
>>> +		reg += sw;
>>> +	} while (reg < reg_end);
>>> +
>>> +no_dt_entry:
>>> +	if (no_dt_entry) {
>>> +		mhi_ctrl->iova_start = 0;
>>> +		mhi_ctrl->iova_stop = 0xFFFFFFFF;
>>> +	} else {
>>> +		mhi_ctrl->iova_start = (dma_addr_t)(start + 0x1000000);
>>> +		mhi_ctrl->iova_stop = (dma_addr_t)(start + size);
>>
>> I don't like casts, they hide bugs like the const issue above. Is there
>> any way to do this without casts?
>>
>
> u64 start, size;
>
>     if (of_property_read_u32_array(np, "reg", reg, 4)) {
>          ath11k_dbg(ab, ATH11K_DBG_QMI,
>                     "qmi fail to get reg from hremote\n");
>          return 0;
>     }
>     start = reg[0] + reg[1];
>     size = reg[2] + reg[3];
>
> I will this code in next patch. still needs typecast form u64 to
> dma_addr_t. no ?

I suspect that a cast is not needed, but not sure. No time to check either.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches