LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Lina Iyer <ilina@codeaurora.org>
To: Doug Anderson <dianders@chromium.org>
Cc: Andy Gross <andy.gross@linaro.org>,
	David Brown <david.brown@linaro.org>,
	linux-arm-msm@vger.kernel.org,
	"open list:ARM/QUALCOMM SUPPORT" <linux-soc@vger.kernel.org>,
	Rajendra Nayak <rnayak@codeaurora.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Stephen Boyd <sboyd@kernel.org>,
	Evan Green <evgreen@chromium.org>
Subject: Re: [PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions
Date: Mon, 14 May 2018 09:00:08 -0600	[thread overview]
Message-ID: <20180514150008.GA25503@codeaurora.org> (raw)
In-Reply-To: <CAD=FV=XTV0zF1Hmf5CTx3_+dfhK-nWRZwqWznFp5ASe4E_TyUQ@mail.gmail.com>

On Fri, May 11 2018 at 14:14 -0600, Doug Anderson wrote:
>Hi,
>
>On Fri, May 11, 2018 at 8:06 AM, Lina Iyer <ilina@codeaurora.org> wrote:
>>> As I've said I haven't reviewed RPMh in any amount of detail and so
>>> perhaps I don't understand something.
>>>
>>> OK, I dug a little more and coded up something for you.  Basically
>>> you're doing a whole bunch of iteration / extra work here to try to
>>> come up with a way to associate an extra bit of data with each "struct
>>> rsc_drv".  Rather than that, just add an extra field into "struct
>>> rsc_drv".  Problem solved.
>>>
>>> See http://crosreview.com/1054646 for what I mean.
>>>
>> I tried to avoid such pointer references and keep it object oriented
>> with this approach. I agree that we run through a list of 2 (at the max)
>> RSC to get the drv* from the rpmh_ctrlr. It is not going to be
>> expensive.
>
>Even if you wanted to keep things "object oriented" then IMHO your
>code still should change.  Sure, it's not computationally expensive to
>iterate through this list, but it adds an extra level of complexity
>that doesn't seem justified.
>
>If you _really_ needed an abstraction barrier then at least add a
>"void *client_data" to "struct rsc_drv.c".  At least you'd get rid of
>the ugly global list and store your pointer directly in the correct
>structure rather than creating an external entity.  Now it becomes
>100% obvious that there is exactly 1 "struct rpmh_ctrlr" for each
>controller.  ...but IMO there's enough intertwining between "rpmh.c"
>and "rpmh-rsc.c" that it would just be a waste because now you'd need
>to do extra memory allocation and freeing.  ...and if you just
>allocated the pointer in get_rpmh_ctrlr() it would also seem non-ideal
>because this one-time allocation (that affects _all_ RPMh clients)
>happens whenever one client happens to do the first access.  This is
>one-time init so it makes sense to do it at init time.
>
>I say that there's intertwining between "rpmh.c" and "rpmh-rsc.c"
>because both C files call directly into one another and have intimate
>knowledge of how the other works.  They aren't really separate things.
>Specifically I see that "rpmh-rsc" directly calls into "rpmh.c" when
>it calls rpmh_tx_done(), and of coruse "rpmh-rsc.c" directly calls
>into "rpmh.c".
>
>
>OK, so I've been browsing through the source code more so I can be a
>little more informed here.  As far as I can tell "rpmh.c"'s goal is:
>
>1. Put a simpler API atop "rpmh-rsc.c".  ...but why not just put that
>API directly into "rpmh-rsc.c" anyway?  If there was someone that
>needed the more complex API then having a "simpler" wrapper makes
>sense, but that's not the case, is it?
>
>2. Add caching atop "rpmh-rsc"
>
>
>I'll respond to some of your other patches too, but I think that the
>amount of code for caching is not very much.  I don't see the benefit
>of trying to split the code into two files.  Put them into one and
>then delete all the extra code you needed just the try to maintain
>some abstraction.
>
>
>> Another things this helps with is that, if the driver is not a child of
>> the RSC nodes in DT, then the drvdata of the parent would not a RSC node
>> and accessing that would result in a crash. This offers a cleaner exit
>> path for the error case.
>
>Why would the driver not be a child of the RSC nodes?  That's kinda
>like saying "if you try to instantiate an i2c device as a platform
>device then its probe will crash".  Yeah, it will.  Doctor, it hurts
>if I poke myself in my eye with this sharp stick, do you have any
>medicine that can help fix that?
>
>
>>>>> I'll try to dig into this more so I could just be confused, but in
>>>>> general it seems really odd to have a spinlock and something called a
>>>>> "cache" at this level.  If we need some sort of mutual exclusion or
>>>>> caching it seems like it should be stored in memory directly
>>>>> associated with the RPMh device, not some external global.
>>>>>
>>>> The idea behind the locking is not to avoid the race between rpmh.c and
>>>> rpmh-rsc.c. From the DT, the devices that are dependent on the RSCs are
>>>> probed following the probe of the controller. And init is not that we are
>>>> worried about.
>>>> The condition here is to prevent the rpmh_rsc[] from being modified
>>>> concurrently by drivers.
>>>
>>>
>>> OK, I see the point of the locking now, but not the list still.
>>> Sounds like Matthias agrees with me that the list isn't useful.  Seems
>>> like you should squash my patch at http://crosreview.com/1042883 into
>>> yours.
>>>
>> I saw your approach. I am okay with it for your tree,
>
>I'm not okay with it for the Chrome OS tree.  We need to match
>upstream, not fork for style reasons.  I'd rather take a driver that I
>think it overly complex but matches upstream than a private driver.
>
>
>> my approach comes
>> out of experiences in qcom platforms and how things tend to shape up in
>> the future. I would want you to consider my reasoning as well, before we
>> go forward.
>
>I suppose we can get advice from others who have worked in qcom
>platforms and see what they think.  My opinions come out of years of
>experience working with Linux drivers, so I guess we both have some
>experience under our belts that we're trying to leverage.
>
Doug, I am sorry it came out the wrong way. I meant to say, knowing
qualcomm platforms, as it has been in the past, we need this
flexibility. Things change with hardware variants just a wee bit that it
doesn't warrant a new interface, just a new driver or part of it to be
re-written. Keeping code separate out like this helps us maintain the
driver better.

Thanks for your reviews. I will try to address your comments before my
vacation, but I doubt I will get to all of it.

Thanks,
Lina

  reply	other threads:[~2018-05-14 15:00 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-02 19:37 [PATCH v7 00/10] drivers/qcom: add RPMH communication support Lina Iyer
2018-05-02 19:37 ` [PATCH v7 01/10] drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs Lina Iyer
2018-05-02 19:37 ` [PATCH v7 02/10] dt-bindings: introduce RPMH RSC bindings for Qualcomm SoCs Lina Iyer
2018-05-02 20:37   ` Stephen Boyd
2018-05-02 19:37 ` [PATCH v7 03/10] drivers: qcom: rpmh-rsc: log RPMH requests in FTRACE Lina Iyer
2018-05-02 19:45   ` Steven Rostedt
2018-05-02 19:37 ` [PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions Lina Iyer
2018-05-03 20:26   ` Doug Anderson
2018-05-04 20:50     ` Matthias Kaehlcke
2018-05-08 16:05     ` ilina
2018-05-10 22:37       ` Doug Anderson
2018-05-11 15:06         ` Lina Iyer
2018-05-11 20:14           ` Doug Anderson
2018-05-14 15:00             ` Lina Iyer [this message]
2018-05-02 19:37 ` [PATCH v7 05/10] drivers: qcom: rpmh-rsc: write sleep/wake requests to TCS Lina Iyer
2018-05-03 21:35   ` Matthias Kaehlcke
2018-05-08 16:16     ` ilina
2018-05-02 19:37 ` [PATCH v7 06/10] drivers: qcom: rpmh-rsc: allow invalidation of sleep/wake TCS Lina Iyer
2018-05-03 22:06   ` Matthias Kaehlcke
2018-05-08 16:14     ` ilina
2018-05-08 17:25       ` Matthias Kaehlcke
2018-05-02 19:37 ` [PATCH v7 07/10] drivers: qcom: rpmh: cache sleep/wake state requests Lina Iyer
2018-05-04 21:39   ` Matthias Kaehlcke
2018-05-02 19:37 ` [PATCH v7 08/10] drivers: qcom: rpmh: allow requests to be sent asynchronously Lina Iyer
2018-05-02 19:37 ` [PATCH v7 09/10] drivers: qcom: rpmh: add support for batch RPMH request Lina Iyer
2018-05-02 19:37 ` [PATCH v7 10/10] drivers: qcom: rpmh-rsc: allow active requests from wake TCS Lina Iyer

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=20180514150008.GA25503@codeaurora.org \
    --to=ilina@codeaurora.org \
    --cc=andy.gross@linaro.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=david.brown@linaro.org \
    --cc=dianders@chromium.org \
    --cc=evgreen@chromium.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-soc@vger.kernel.org \
    --cc=rnayak@codeaurora.org \
    --cc=sboyd@kernel.org \
    --subject='Re: [PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions' \
    /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).