Some clients like adreno gpu driver would like to ensure that its gdsc
is collapsed at hardware during a gpu reset sequence. This is because it
has a votable gdsc which could be ON due to a vote from another subsystem
like tz, hyp etc or due to an internal hardware signal. To allow
this, gpucc driver can expose an interface to the client driver using
reset framework. Using this the client driver can trigger a polling within
the gdsc driver.
This series is rebased on top of linus's master branch.
Related discussion: https://patchwork.freedesktop.org/patch/493144/
Akhil P Oommen (5):
dt-bindings: clk: qcom: Support gpu cx gdsc reset
clk: qcom: Allow custom reset ops
clk: qcom: gpucc-sc7280: Add cx collapse reset support
clk: qcom: gdsc: Add a reset op to poll gdsc collapse
arm64: dts: qcom: sc7280: Add Reset support for gpu
arch/arm64/boot/dts/qcom/sc7280.dtsi | 3 +++
drivers/clk/qcom/gdsc.c | 23 +++++++++++++++++++----
drivers/clk/qcom/gdsc.h | 7 +++++++
drivers/clk/qcom/gpucc-sc7280.c | 6 ++++++
drivers/clk/qcom/reset.c | 6 ++++++
drivers/clk/qcom/reset.h | 2 ++
include/dt-bindings/clock/qcom,gpucc-sc7280.h | 3 +++
7 files changed, 46 insertions(+), 4 deletions(-)
--
2.7.4
Add support to allow soc specific clk drivers to specify a custom reset
operation. A consumer-driver of the reset framework can call
"reset_control_reset()" api to trigger this.
Signed-off-by: Akhil P Oommen <[email protected]>
---
drivers/clk/qcom/reset.c | 6 ++++++
drivers/clk/qcom/reset.h | 2 ++
2 files changed, 8 insertions(+)
diff --git a/drivers/clk/qcom/reset.c b/drivers/clk/qcom/reset.c
index 819d194..4782bf1 100644
--- a/drivers/clk/qcom/reset.c
+++ b/drivers/clk/qcom/reset.c
@@ -13,6 +13,12 @@
static int qcom_reset(struct reset_controller_dev *rcdev, unsigned long id)
{
+ struct qcom_reset_controller *rst = to_qcom_reset_controller(rcdev);
+ const struct qcom_reset_map *map = &rst->reset_map[id];
+
+ if (map->op)
+ return map->op(map);
+
rcdev->ops->assert(rcdev, id);
udelay(1);
rcdev->ops->deassert(rcdev, id);
diff --git a/drivers/clk/qcom/reset.h b/drivers/clk/qcom/reset.h
index 2a08b5e..295deeb 100644
--- a/drivers/clk/qcom/reset.h
+++ b/drivers/clk/qcom/reset.h
@@ -11,6 +11,8 @@
struct qcom_reset_map {
unsigned int reg;
u8 bit;
+ int (*op)(const struct qcom_reset_map *map);
+ void *priv;
};
struct regmap;
--
2.7.4
Hi Akhil,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on clk/clk-next]
[also build test WARNING on robh/for-next drm-misc/drm-misc-next drm-tip/drm-tip linus/master v5.19-rc8 next-20220728]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Akhil-P-Oommen/clk-qcom-Support-gdsc-collapse-polling-using-reset-inteface/20220730-171922
base: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: ia64-randconfig-r031-20220729 (https://download.01.org/0day-ci/archive/20220730/[email protected]/config)
compiler: ia64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/971a03493e9854ff4a227ee4d80b533997959891
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Akhil-P-Oommen/clk-qcom-Support-gdsc-collapse-polling-using-reset-inteface/20220730-171922
git checkout 971a03493e9854ff4a227ee4d80b533997959891
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/clk/qcom/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/clk/qcom/reset.c: In function 'qcom_reset':
>> drivers/clk/qcom/reset.c:17:9: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
17 | const struct qcom_reset_map *map = &rst->reset_map[id];
| ^~~~~
vim +17 drivers/clk/qcom/reset.c
13
14 static int qcom_reset(struct reset_controller_dev *rcdev, unsigned long id)
15 {
16 struct qcom_reset_controller *rst = to_qcom_reset_controller(rcdev);
> 17 const struct qcom_reset_map *map = &rst->reset_map[id];
18
19 if (map->op)
20 return map->op(map);
21
22 rcdev->ops->assert(rcdev, id);
23 udelay(1);
24 rcdev->ops->deassert(rcdev, id);
25 return 0;
26 }
27
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On 7/30/2022 6:40 PM, kernel test robot wrote:
> Hi Akhil,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on clk/clk-next]
> [also build test WARNING on robh/for-next drm-misc/drm-misc-next drm-tip/drm-tip linus/master v5.19-rc8 next-20220728]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Akhil-P-Oommen/clk-qcom-Support-gdsc-collapse-polling-using-reset-inteface/20220730-171922
> base: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
> config: ia64-randconfig-r031-20220729 (https://download.01.org/0day-ci/archive/20220730/[email protected]/config)
> compiler: ia64-linux-gcc (GCC) 12.1.0
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # https://github.com/intel-lab-lkp/linux/commit/971a03493e9854ff4a227ee4d80b533997959891
> git remote add linux-review https://github.com/intel-lab-lkp/linux
> git fetch --no-tags linux-review Akhil-P-Oommen/clk-qcom-Support-gdsc-collapse-polling-using-reset-inteface/20220730-171922
> git checkout 971a03493e9854ff4a227ee4d80b533997959891
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/clk/qcom/
>
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <[email protected]>
>
> All warnings (new ones prefixed by >>):
>
> drivers/clk/qcom/reset.c: In function 'qcom_reset':
>>> drivers/clk/qcom/reset.c:17:9: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
> 17 | const struct qcom_reset_map *map = &rst->reset_map[id];
> | ^~~~~
>
>
> vim +17 drivers/clk/qcom/reset.c
>
> 13
> 14 static int qcom_reset(struct reset_controller_dev *rcdev, unsigned long id)
> 15 {
> 16 struct qcom_reset_controller *rst = to_qcom_reset_controller(rcdev);
> > 17 const struct qcom_reset_map *map = &rst->reset_map[id];
> 18
> 19 if (map->op)
> 20 return map->op(map);
> 21
> 22 rcdev->ops->assert(rcdev, id);
> 23 udelay(1);
> 24 rcdev->ops->deassert(rcdev, id);
> 25 return 0;
> 26 }
> 27
>
Will fix this and send another version of this patch. Please let me know
if there is any feedback to the whole series.
-Akhil.
On 30/07/2022 12:17, Akhil P Oommen wrote:
>
> Some clients like adreno gpu driver would like to ensure that its gdsc
> is collapsed at hardware during a gpu reset sequence. This is because it
> has a votable gdsc which could be ON due to a vote from another subsystem
> like tz, hyp etc or due to an internal hardware signal.
If this is votable, do we have any guarantee that the gdsc will collapse
at all? How can we proceed if it did not collapse?
> To allow
> this, gpucc driver can expose an interface to the client driver using
> reset framework. Using this the client driver can trigger a polling within
> the gdsc driver.
Trigger the polling made me think initially that we will actually
trigger something in the HW. Instead the client uses reset framework to
poll for the gdsc to be reset.
>
> This series is rebased on top of linus's master branch.
>
> Related discussion: https://patchwork.freedesktop.org/patch/493144/
>
>
> Akhil P Oommen (5):
> dt-bindings: clk: qcom: Support gpu cx gdsc reset
> clk: qcom: Allow custom reset ops
> clk: qcom: gpucc-sc7280: Add cx collapse reset support
> clk: qcom: gdsc: Add a reset op to poll gdsc collapse
> arm64: dts: qcom: sc7280: Add Reset support for gpu
>
> arch/arm64/boot/dts/qcom/sc7280.dtsi | 3 +++
> drivers/clk/qcom/gdsc.c | 23 +++++++++++++++++++----
> drivers/clk/qcom/gdsc.h | 7 +++++++
> drivers/clk/qcom/gpucc-sc7280.c | 6 ++++++
> drivers/clk/qcom/reset.c | 6 ++++++
> drivers/clk/qcom/reset.h | 2 ++
> include/dt-bindings/clock/qcom,gpucc-sc7280.h | 3 +++
> 7 files changed, 46 insertions(+), 4 deletions(-)
>
--
With best wishes
Dmitry
On 30/07/2022 12:17, Akhil P Oommen wrote:
> Add support to allow soc specific clk drivers to specify a custom reset
> operation. A consumer-driver of the reset framework can call
> "reset_control_reset()" api to trigger this.
>
> Signed-off-by: Akhil P Oommen <[email protected]>
> ---
>
> drivers/clk/qcom/reset.c | 6 ++++++
> drivers/clk/qcom/reset.h | 2 ++
> 2 files changed, 8 insertions(+)
>
> diff --git a/drivers/clk/qcom/reset.c b/drivers/clk/qcom/reset.c
> index 819d194..4782bf1 100644
> --- a/drivers/clk/qcom/reset.c
> +++ b/drivers/clk/qcom/reset.c
> @@ -13,6 +13,12 @@
>
> static int qcom_reset(struct reset_controller_dev *rcdev, unsigned long id)
> {
> + struct qcom_reset_controller *rst = to_qcom_reset_controller(rcdev);
> + const struct qcom_reset_map *map = &rst->reset_map[id];
> +
> + if (map->op)
> + return map->op(map);
This looks like a hack. For example, assert() and deassert() would still
follow the usual pattern of updating the bits. Please at least make them
return -EOPNOTSUP if map->op is defined.
A slightly better solution would be to make qcom_reset implementation
optional (and depending on desc->num_resets being greater than 0). Then
you can register your own reset controller implementation from the gpucc
driver.
> +
> rcdev->ops->assert(rcdev, id);
> udelay(1);
> rcdev->ops->deassert(rcdev, id);
> diff --git a/drivers/clk/qcom/reset.h b/drivers/clk/qcom/reset.h
> index 2a08b5e..295deeb 100644
> --- a/drivers/clk/qcom/reset.h
> +++ b/drivers/clk/qcom/reset.h
> @@ -11,6 +11,8 @@
> struct qcom_reset_map {
> unsigned int reg;
> u8 bit;
> + int (*op)(const struct qcom_reset_map *map);
> + void *priv;
> };
>
> struct regmap;
--
With best wishes
Dmitry
On Tue, Aug 2, 2022 at 12:02 AM Dmitry Baryshkov
<[email protected]> wrote:
>
> On 30/07/2022 12:17, Akhil P Oommen wrote:
> >
> > Some clients like adreno gpu driver would like to ensure that its gdsc
> > is collapsed at hardware during a gpu reset sequence. This is because it
> > has a votable gdsc which could be ON due to a vote from another subsystem
> > like tz, hyp etc or due to an internal hardware signal.
>
> If this is votable, do we have any guarantee that the gdsc will collapse
> at all? How can we proceed if it did not collapse?
Other potential votes should be transient. But I guess we eventually
need to timeout and give up. At which point we are no worse off than
before.
But hmm, we aren't using RBBM_SW_RESET_CMD for sw reset like we have
on previous generations? That does seem a bit odd. Looks like kgsl
does use it.
BR,
-R
> > To allow
> > this, gpucc driver can expose an interface to the client driver using
> > reset framework. Using this the client driver can trigger a polling within
> > the gdsc driver.
>
> Trigger the polling made me think initially that we will actually
> trigger something in the HW. Instead the client uses reset framework to
> poll for the gdsc to be reset.
>
> >
> > This series is rebased on top of linus's master branch.
> >
> > Related discussion: https://patchwork.freedesktop.org/patch/493144/
> >
> >
> > Akhil P Oommen (5):
> > dt-bindings: clk: qcom: Support gpu cx gdsc reset
> > clk: qcom: Allow custom reset ops
> > clk: qcom: gpucc-sc7280: Add cx collapse reset support
> > clk: qcom: gdsc: Add a reset op to poll gdsc collapse
> > arm64: dts: qcom: sc7280: Add Reset support for gpu
> >
> > arch/arm64/boot/dts/qcom/sc7280.dtsi | 3 +++
> > drivers/clk/qcom/gdsc.c | 23 +++++++++++++++++++----
> > drivers/clk/qcom/gdsc.h | 7 +++++++
> > drivers/clk/qcom/gpucc-sc7280.c | 6 ++++++
> > drivers/clk/qcom/reset.c | 6 ++++++
> > drivers/clk/qcom/reset.h | 2 ++
> > include/dt-bindings/clock/qcom,gpucc-sc7280.h | 3 +++
> > 7 files changed, 46 insertions(+), 4 deletions(-)
> >
>
>
> --
> With best wishes
> Dmitry
On 8/3/2022 12:02 AM, Rob Clark wrote:
> On Tue, Aug 2, 2022 at 12:02 AM Dmitry Baryshkov
> <[email protected]> wrote:
>> On 30/07/2022 12:17, Akhil P Oommen wrote:
>>> Some clients like adreno gpu driver would like to ensure that its gdsc
>>> is collapsed at hardware during a gpu reset sequence. This is because it
>>> has a votable gdsc which could be ON due to a vote from another subsystem
>>> like tz, hyp etc or due to an internal hardware signal.
>> If this is votable, do we have any guarantee that the gdsc will collapse
>> at all? How can we proceed if it did not collapse?
> Other potential votes should be transient. But I guess we eventually
> need to timeout and give up. At which point we are no worse off than
> before.
>
> But hmm, we aren't using RBBM_SW_RESET_CMD for sw reset like we have
> on previous generations? That does seem a bit odd. Looks like kgsl
> does use it.
>
> BR,
> -R
Like Rob mentioned there could be transient votes from other
clients/subsystem. It could be even stuck ON when hardware is in bad
shape in some very rare cases. For the worst case scenario, I have added
a timeout (500msec) in the gdsc reset op.
I have added the Soft reset in [1]. But this resets only the core gpu
blocks, not everything. For eg. GMU.
[1] [PATCH v3 7/8] drm/msm/a6xx: Improve gpu recovery sequence
>
>>> To allow
>>> this, gpucc driver can expose an interface to the client driver using
>>> reset framework. Using this the client driver can trigger a polling within
>>> the gdsc driver.
>> Trigger the polling made me think initially that we will actually
>> trigger something in the HW. Instead the client uses reset framework to
>> poll for the gdsc to be reset.
Yes. I should replace 'trigger' with 'start' here.
-Akhil.
>>
>>> This series is rebased on top of linus's master branch.
>>>
>>> Related discussion: https://patchwork.freedesktop.org/patch/493144/
>>>
>>>
>>> Akhil P Oommen (5):
>>> dt-bindings: clk: qcom: Support gpu cx gdsc reset
>>> clk: qcom: Allow custom reset ops
>>> clk: qcom: gpucc-sc7280: Add cx collapse reset support
>>> clk: qcom: gdsc: Add a reset op to poll gdsc collapse
>>> arm64: dts: qcom: sc7280: Add Reset support for gpu
>>>
>>> arch/arm64/boot/dts/qcom/sc7280.dtsi | 3 +++
>>> drivers/clk/qcom/gdsc.c | 23 +++++++++++++++++++----
>>> drivers/clk/qcom/gdsc.h | 7 +++++++
>>> drivers/clk/qcom/gpucc-sc7280.c | 6 ++++++
>>> drivers/clk/qcom/reset.c | 6 ++++++
>>> drivers/clk/qcom/reset.h | 2 ++
>>> include/dt-bindings/clock/qcom,gpucc-sc7280.h | 3 +++
>>> 7 files changed, 46 insertions(+), 4 deletions(-)
>>>
>>
>> --
>> With best wishes
>> Dmitry
On Sat 30 Jul 04:17 CDT 2022, Akhil P Oommen wrote:
>
> Some clients like adreno gpu driver would like to ensure that its gdsc
> is collapsed at hardware during a gpu reset sequence. This is because it
> has a votable gdsc which could be ON due to a vote from another subsystem
> like tz, hyp etc or due to an internal hardware signal. To allow
> this, gpucc driver can expose an interface to the client driver using
> reset framework. Using this the client driver can trigger a polling within
> the gdsc driver.
>
> This series is rebased on top of linus's master branch.
>
> Related discussion: https://patchwork.freedesktop.org/patch/493144/
>
Forgive me if I'm assuming too much, but isn't this an extension of:
85a3d920d30a ("clk: qcom: Add a dummy enable function for GX gdsc")
With the additional requirement that disable should really ensure that
the GDSC is turned off?
Regards,
Bjorn
>
> Akhil P Oommen (5):
> dt-bindings: clk: qcom: Support gpu cx gdsc reset
> clk: qcom: Allow custom reset ops
> clk: qcom: gpucc-sc7280: Add cx collapse reset support
> clk: qcom: gdsc: Add a reset op to poll gdsc collapse
> arm64: dts: qcom: sc7280: Add Reset support for gpu
>
> arch/arm64/boot/dts/qcom/sc7280.dtsi | 3 +++
> drivers/clk/qcom/gdsc.c | 23 +++++++++++++++++++----
> drivers/clk/qcom/gdsc.h | 7 +++++++
> drivers/clk/qcom/gpucc-sc7280.c | 6 ++++++
> drivers/clk/qcom/reset.c | 6 ++++++
> drivers/clk/qcom/reset.h | 2 ++
> include/dt-bindings/clock/qcom,gpucc-sc7280.h | 3 +++
> 7 files changed, 46 insertions(+), 4 deletions(-)
>
> --
> 2.7.4
>
On 8/10/2022 2:35 AM, Bjorn Andersson wrote:
> On Sat 30 Jul 04:17 CDT 2022, Akhil P Oommen wrote:
>
>> Some clients like adreno gpu driver would like to ensure that its gdsc
>> is collapsed at hardware during a gpu reset sequence. This is because it
>> has a votable gdsc which could be ON due to a vote from another subsystem
>> like tz, hyp etc or due to an internal hardware signal. To allow
>> this, gpucc driver can expose an interface to the client driver using
>> reset framework. Using this the client driver can trigger a polling within
>> the gdsc driver.
>>
>> This series is rebased on top of linus's master branch.
>>
>> Related discussion: https://patchwork.freedesktop.org/patch/493144/
>>
> Forgive me if I'm assuming too much, but isn't this an extension of:
>
> 85a3d920d30a ("clk: qcom: Add a dummy enable function for GX gdsc")
>
> With the additional requirement that disable should really ensure that
> the GDSC is turned off?
Also, gpu driver needs a way to ensure cx gdsc was collapsed at least
once before it goes ahead with re-init.
Btw, the patch you mentioned is about gx gdsc in gpucc which is supposed
to be owned by gmu (except when it is in bad shape). But the current
series is about cx gdsc which is shared with other subsystems/drivers.
-Akhil.
>
> Regards,
> Bjorn
>
>> Akhil P Oommen (5):
>> dt-bindings: clk: qcom: Support gpu cx gdsc reset
>> clk: qcom: Allow custom reset ops
>> clk: qcom: gpucc-sc7280: Add cx collapse reset support
>> clk: qcom: gdsc: Add a reset op to poll gdsc collapse
>> arm64: dts: qcom: sc7280: Add Reset support for gpu
>>
>> arch/arm64/boot/dts/qcom/sc7280.dtsi | 3 +++
>> drivers/clk/qcom/gdsc.c | 23 +++++++++++++++++++----
>> drivers/clk/qcom/gdsc.h | 7 +++++++
>> drivers/clk/qcom/gpucc-sc7280.c | 6 ++++++
>> drivers/clk/qcom/reset.c | 6 ++++++
>> drivers/clk/qcom/reset.h | 2 ++
>> include/dt-bindings/clock/qcom,gpucc-sc7280.h | 3 +++
>> 7 files changed, 46 insertions(+), 4 deletions(-)
>>
>> --
>> 2.7.4
>>