[v3,0/5] Refactor to support multiple download mode

Intention of this series to support multiple download mode and
only modify the required bits during setting tcsr register.

Other download modes are minidump, bothdump (full dump + minidump).

Minidump kernel driver patches has been sent here
https://lore.kernel.org/lkml/1676978713-7394-1-git-send-email-quic_mojha@quicinc.com/

Changes in v3:
 - Removed [1] from the series and sent as a separate patch[2], although this series
   should be applied on top [2].
  [1] https://lore.kernel.org/lkml/1677664555-30191-2-git-send-email-quic_mojha@quicinc.com/
  [2] https://lore.kernel.org/lkml/1678979666-551-1-git-send-email-quic_mojha@quicinc.com/
 - Introduce new exported symbol on suggestion from [srinivas.kandagatla]
 - Use the symbol from drivers/pinctrl/qcom/pinctrl-msm.c.
 - Addressed comment given by [dmitry.baryshkov]
 - Converted non-standard Originally-by to Signed-off-by.

Changes in v2: https://lore.kernel.org/lkml/1677664555-30191-1-git-send-email-quic_mojha@quicinc.com/
 - Addressed comment made by [bjorn]
 - Added download mask.
 - Passed download mode as parameter
 - Accept human accepatable download mode string.
 - enable = !!dload_mode
 - Shifted module param callback to somewhere down in
   the file so that it no longer need to know the
   prototype of qcom_scm_set_download_mode()
 - updated commit text.


Mukesh Ojha (5):
  firmware: qcom_scm: provide a read-modify-write function
  pinctrl: qcom: Use qcom_scm_io_update_field()
  firmware: scm: Modify only the download bits in TCSR register
  firmware: qcom_scm: Refactor code to support multiple download mode
  firmware: qcom_scm: Add multiple download mode support

 drivers/firmware/Kconfig               | 11 -----
 drivers/firmware/qcom_scm.c            | 88 ++++++++++++++++++++++++++++++----
 drivers/pinctrl/qcom/pinctrl-msm.c     | 15 +++---
 include/linux/firmware/qcom/qcom_scm.h |  2 +
 4 files changed, 89 insertions(+), 27 deletions(-)

On Fri, Mar 17, 2023 at 5:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote:

> It was released by Srinivas K. that there is a need of
> read-modify-write scm exported function so that it can
> be used by multiple clients.
>
> Let's introduce qcom_scm_io_update_field() which masks
> out the bits and write the passed value to that
> bit-offset. Subsequent patch will use this function.
>
> Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>

This is starting to reimplement regmap.
In this case regmap_update_bits().

What about just using regmap as accessor for these
registers instead?

Yours,
Linus Walleij

On Fri, Mar 17, 2023 at 09:56:59PM +0100, Linus Walleij wrote:
> On Fri, Mar 17, 2023 at 5:28 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote:
> 
> > It was released by Srinivas K. that there is a need of
> > read-modify-write scm exported function so that it can
> > be used by multiple clients.
> >
> > Let's introduce qcom_scm_io_update_field() which masks
> > out the bits and write the passed value to that
> > bit-offset. Subsequent patch will use this function.
> >
> > Suggested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> 
> This is starting to reimplement regmap.
> In this case regmap_update_bits().
> 
> What about just using regmap as accessor for these
> registers instead?
> 

I'm not sure it would be beneficial...

The regmap interface provides a standardized representation of a block
of registers, with the suitable accessors backing it. But in both cases
touched upon in this series, the addressed registers are part of regions
already handled by the kernel.

So it wouldn't be suitable to create a regmap-abstraction for "a block
of secure registers", at best that would give us two kinds of regmaps
abstracting the same register block.


Instead I believe we'd need to extend the struct regmap_config to
introduce a new table telling a new secure-or-unsecure-mmio-regmap which
accessor (secure or unsecure read/write) shoudl be used, and then have
e.g. pinctrl-msm register such regmap, passing the information about
which registers in its memory region is secure.

We'd still need qcom_scm_io_readl() and qcom_scm_io_writel() exported to
implement the new custom regmap implementation - and the struct
regmap_config needed in just pinctrl-msm alone would be larger than the
one function it replaces.

But please let me know if I'm missing something?

Regards,
Bjorn