LKML Archive on lore.kernel.org
 help / Atom feed
From: Sibi S <sibis@codeaurora.org>
To: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: ohad@wizery.com, linux-remoteproc@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH v2] remoteproc: Introduce prepare/unprepare ops for rproc coredump
Date: Tue, 29 May 2018 19:36:10 +0530
Message-ID: <58528924-0ca1-fe23-741c-ad3d2c1d4388@codeaurora.org> (raw)
In-Reply-To: <20180529050523.GG2259@tuxbook-pro>

Hi Bjorn,
Thanks for the review.


On 05/29/2018 10:35 AM, Bjorn Andersson wrote:
> On Mon 21 May 11:45 PDT 2018, Sibi Sankar wrote:
> 
>> In some occasions the remoteproc device might need to
>> prepare some hardware before the coredump can be performed
>> and cleanup the state afterwards.
>>
>> Q6V5 modem requires the mba to be loaded before the
>> coredump and some cleanup of the resources afterwards.
>>
> 
> This describes two different changes, so please put it in two+ patches.
>


The second change just describes an example of a remoteproc device
that requires remoteproc coredump prepare and unprepare but sure
with restructuring required in msa it definitely will require 2+
patches.


> [..]
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index dfdaede9139e..010819e01279 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -333,6 +333,8 @@ struct firmware;
>>    * @kick:	kick a virtqueue (virtqueue id given as a parameter)
>>    * @da_to_va:	optional platform hook to perform address translations
>>    * @load_rsc_table:	load resource table from firmware image
>> + * @prepare_coredump:	prepare function, called before coredump
>> + * @unprepare_coredump:	unprepare function, called post coredump
> 
> I believe there will be other cases where we will need driver-specific
> logic to extract the memory content of the segments, e.g. through custom
> hardware sequences or non-mmio reads.
> 
> To support this I think we should extend the struct rproc_dump_segment
> to carry an optional "dump" function that if specified will be used
> instead of the memcpy in rproc_coredump(). Drivers can then for each
> segment specify this function, if needed.
> 

In q6 modem mba needs to unlocked just once for all the segments to
be dumped but as you say other remote processors may need it for each
segment. This logic should be internal to the dump function anyway. So
will use the dump function approach. What about the cleanup path, can we
still reserve it till the coredump is done?



> Through some restructuring in the msa driver and your patch you should
> be able to implement this using such a mechanism instead - and it would
> be useful to these other cases as well.
> 
> 
> PS. I hope we can get away from some of the conditionals in your patch
> through some restructuring of the code.
> 


Thought you might preferred the conditionals with the least amount of
code addition/change but having prepare and unprepare coredump again
calling q6v5_start/stop respectively seemed a little hacky anyway.
Sure will restructure msa as needed :)


> Regards,
> Bjorn
> 

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

      reply index

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-21 18:45 Sibi Sankar
2018-05-22 10:39 ` kbuild test robot
2018-05-29  5:05 ` Bjorn Andersson
2018-05-29 14:06   ` Sibi S [this message]

Reply instructions:

You may reply publically 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=58528924-0ca1-fe23-741c-ad3d2c1d4388@codeaurora.org \
    --to=sibis@codeaurora.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 \
    /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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox