LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Sricharan R <sricharan@codeaurora.org>
To: Vinod <vkoul@kernel.org>, Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Ohad Ben-Cohen <ohad@wizery.com>,
	Sibi Sankar <sibis@codeaurora.org>,
	Rohit kumar <rohitkr@codeaurora.org>,
	Andy Gross <andy.gross@linaro.org>,
	linux-kernel@vger.kernel.org, linux-remoteproc@vger.kernel.org,
	linux-arm-msm@vger.kernel.org
Subject: Re: [RFC PATCH 5/5] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
Date: Wed, 23 May 2018 20:18:08 +0530	[thread overview]
Message-ID: <fa0af8c1-f635-13e7-e600-4c1f91a48351@codeaurora.org> (raw)
In-Reply-To: <20180523073723.GF20991@vkoul-mobl>

Hi Bjorn,

On 5/23/2018 1:07 PM, Vinod wrote:
> On 22-05-18, 23:58, Bjorn Andersson wrote:
>> On Tue 22 May 23:05 PDT 2018, Vinod wrote:
>>
>>> On 22-05-18, 22:20, Bjorn Andersson wrote:
>>>
>>>> +static int q6v5_wcss_reset(struct q6v5_wcss *wcss)
>>>> +{
>>>> +	int ret;
>>>> +	u32 val;
>>>> +	int i;
>>>> +
>>>> +	/* Assert resets, stop core */
>>>> +	val = readl(wcss->reg_base + QDSP6SS_RESET_REG);
>>>> +	val |= Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE;
>>>> +	writel(val, wcss->reg_base + QDSP6SS_RESET_REG);
>>>> +
>>>> +	/* BHS require xo cbcr to be enabled */
>>>> +	val = readl(wcss->reg_base + QDSP6SS_XO_CBCR);
>>>> +	val |= 0x1;
>>>> +	writel(val, wcss->reg_base + QDSP6SS_XO_CBCR);
>>>
>>> As commented on previous patch, it would help IMO to add a modify() wrapper
>>> here which would perform read, modify and write.
>>>
>>
>> Iirc the code ended up like this because a lot of these operations ended
>> up being line wrapped and harder to read using some modify(reg, mask,
>> val) helper. That said, the function isn't very pretty in it's current
>> state either...
> 
> Agreed :) and i thought modify make help it make better
> 
>> One of the parts of the RFC is that this sequence is a verbatim copy
>> from the qcom_q6v5_pil.c driver for 8996, so if we find this duplication
>> suitable I would prefer that we keep them the same.
>>
>>
>> The alternative to duplicating this function is as Sricharan proposed to
>> have the qcom_q6v5_pil.c be both a driver for both the single-stage
>> remoteproc and the two-stage (load boot loader, then modem firmware).
>>
>>> Looking at the patch, few other comments would be applicable too, so would be
>>> great if you/Sricharan can update this
>>>
>>
>> I agree, the primary purpose of this patch was rather to get feedback on
>> the structure of the drivers, I do expect this to take another round
>> through the editor to get some polishing touches. Sorry if this wasn't
>> clear from the description.
> 
> Since Sricharan replied to comments, I though they would be fixed. Yeah this is
> fine from RFC..
> 

 Thanks for this.

 Tested this on ipq8074 and wcss rproc is up with this.

 Tested-by: Sricharan R <sricharan@codeaurora.org>

 So regarding the cleanup, as i see, this consolidates the code much better.

 so about the point of avoiding duplication for soc specific functions like
 qcv5_wcss_reset common between qcv5_wcss and qcv5_pil drivers as done in my
 series, with a second thought that feels it might be difficult to maintain
 in the longer run. Since the sequences are specific to each soc and for now
 although some part of it is common, for a minor update in one soc, common
 code needs to reworked every time and tested on all boards that share them.

 So feels like having the duplication for hw init sequences is the cleaner way.

 Apart from that for other comments on the q6v5 wcss driver, i can address them
 on this final patch that you have posted and same can be included in your
 next version. Please let me know how you want to go about it.

Regards,
 Sricharan

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

  reply	other threads:[~2018-05-23 14:48 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-23  5:20 [RFC PATCH 0/5] Hexagon remoteproc spring cleaning Bjorn Andersson
2018-05-23  5:20 ` [RFC PATCH 1/5] remoteproc: qcom: mdt_loader: Make the firmware authentication optional Bjorn Andersson
2018-05-23  5:20 ` [RFC PATCH 2/5] remoteproc: q6v5: Extract common resource handling Bjorn Andersson
2018-06-01  6:16   ` Sricharan R
2018-06-01 15:18     ` Sibi S
2018-06-01 16:39       ` Sricharan R
2018-05-23  5:20 ` [RFC PATCH 3/5] remoteproc: qcom: adsp: Use common q6v5 helpers Bjorn Andersson
2018-06-01  6:25   ` Sricharan R
2018-06-01 13:35   ` Rohit Kumar
2018-05-23  5:20 ` [RFC PATCH 4/5] remoteproc: qcom: q6v5-pil: " Bjorn Andersson
2018-06-01  6:42   ` Sricharan R
2018-05-23  5:20 ` [RFC PATCH 5/5] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver Bjorn Andersson
2018-05-23  6:05   ` Vinod
2018-05-23  6:58     ` Bjorn Andersson
2018-05-23  7:37       ` Vinod
2018-05-23 14:48         ` Sricharan R [this message]
2018-05-29  4:07           ` Bjorn Andersson
2018-05-29  8:32             ` Sricharan R
2018-06-01 13:33 ` [RFC PATCH 0/5] Hexagon remoteproc spring cleaning Rohit Kumar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fa0af8c1-f635-13e7-e600-4c1f91a48351@codeaurora.org \
    --to=sricharan@codeaurora.org \
    --cc=andy.gross@linaro.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=ohad@wizery.com \
    --cc=rohitkr@codeaurora.org \
    --cc=sibis@codeaurora.org \
    --cc=vkoul@kernel.org \
    --subject='Re: [RFC PATCH 5/5] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).