| Message ID | 20200904030958.13325-1-jonathan@marek.ca |
|---|---|
| Headers | show |
| Series | SM8150 and SM8250 videocc drivers | expand |
On Thu, 03 Sep 2020 23:09:51 -0400, Jonathan Marek wrote: > Add device tree bindings for video clock controller for SM8150 SoCs. > > Signed-off-by: Jonathan Marek <jonathan@marek.ca> > --- > .../bindings/clock/qcom,videocc.yaml | 4 ++- > .../dt-bindings/clock/qcom,videocc-sm8150.h | 25 +++++++++++++++++++ > 2 files changed, 28 insertions(+), 1 deletion(-) > create mode 100644 include/dt-bindings/clock/qcom,videocc-sm8150.h > Reviewed-by: Rob Herring <robh@kernel.org>
On Thu, 03 Sep 2020 23:09:52 -0400, Jonathan Marek wrote: > Add device tree bindings for video clock controller for SM8250 SoCs. > > Signed-off-by: Jonathan Marek <jonathan@marek.ca> > --- > .../bindings/clock/qcom,videocc.yaml | 8 +++- > .../dt-bindings/clock/qcom,videocc-sm8250.h | 42 +++++++++++++++++++ > 2 files changed, 49 insertions(+), 1 deletion(-) > create mode 100644 include/dt-bindings/clock/qcom,videocc-sm8250.h > Reviewed-by: Rob Herring <robh@kernel.org>
Quoting Jonathan Marek (2020-09-03 20:09:54) > Add support for the video clock controller found on SM8250 based devices. > > Derived from the downstream driver. > > Signed-off-by: Jonathan Marek <jonathan@marek.ca> > --- > drivers/clk/qcom/Kconfig | 9 + > drivers/clk/qcom/Makefile | 1 + > drivers/clk/qcom/videocc-sm8250.c | 518 ++++++++++++++++++++++++++++++ > 3 files changed, 528 insertions(+) > create mode 100644 drivers/clk/qcom/videocc-sm8250.c > > diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig > index 40d7ee9886c9..95efa38211d5 100644 > --- a/drivers/clk/qcom/Kconfig > +++ b/drivers/clk/qcom/Kconfig > @@ -453,6 +453,15 @@ config SM_VIDEOCC_8150 > Say Y if you want to support video devices and functionality such as > video encode and decode. > > +config SM_VIDEOCC_8250 > + tristate "SM8250 Video Clock Controller" > + select SDM_GCC_8250 > + select QCOM_GDSC > + help > + Support for the video clock controller on SM8250 devices. > + Say Y if you want to support video devices and functionality such as > + video encode and decode. > + > config SPMI_PMIC_CLKDIV > tristate "SPMI PMIC clkdiv Support" > depends on SPMI || COMPILE_TEST > diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile > index 6f4c580d2728..55fb20800b66 100644 > --- a/drivers/clk/qcom/Makefile > +++ b/drivers/clk/qcom/Makefile > @@ -69,6 +69,7 @@ obj-$(CONFIG_SM_GCC_8250) += gcc-sm8250.o > obj-$(CONFIG_SM_GPUCC_8150) += gpucc-sm8150.o > obj-$(CONFIG_SM_GPUCC_8250) += gpucc-sm8250.o > obj-$(CONFIG_SM_VIDEOCC_8150) += videocc-sm8150.o > +obj-$(CONFIG_SM_VIDEOCC_8250) += videocc-sm8250.o > obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o > obj-$(CONFIG_KPSS_XCC) += kpss-xcc.o > obj-$(CONFIG_QCOM_HFPLL) += hfpll.o > diff --git a/drivers/clk/qcom/videocc-sm8250.c b/drivers/clk/qcom/videocc-sm8250.c > new file mode 100644 > index 000000000000..a814d10945c4 > --- /dev/null > +++ b/drivers/clk/qcom/videocc-sm8250.c > @@ -0,0 +1,518 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved. > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +#include <dt-bindings/clock/qcom,videocc-sm8250.h> > + [...] > +static struct clk_rcg2 video_cc_ahb_clk_src = { > + .cmd_rcgr = 0xbd4, > + .mnd_width = 0, > + .hid_width = 5, > + .parent_map = video_cc_parent_map_0, > + .freq_tbl = ftbl_video_cc_ahb_clk_src, > + .clkr.hw.init = &(struct clk_init_data){ > + .name = "video_cc_ahb_clk_src", > + .parent_data = video_cc_parent_data_0, > + .num_parents = ARRAY_SIZE(video_cc_parent_data_0), > + .flags = CLK_SET_RATE_PARENT, > + .ops = &clk_rcg2_shared_ops, > + }, > +}; > + > +static struct clk_rcg2 video_cc_xo_clk_src = { > + .cmd_rcgr = 0xecc, > + .mnd_width = 0, > + .hid_width = 5, > + .parent_map = video_cc_parent_map_0, > + .freq_tbl = ftbl_video_cc_ahb_clk_src, > + .clkr.hw.init = &(struct clk_init_data){ > + .name = "video_cc_xo_clk_src", > + .parent_data = video_cc_parent_data_0, > + .num_parents = ARRAY_SIZE(video_cc_parent_data_0), > + .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT, Similar critical clk comment, see below. > + .ops = &clk_rcg2_ops, > + }, > +}; > + > +static struct clk_branch video_cc_ahb_clk = { > + .halt_reg = 0xe58, > + .halt_check = BRANCH_HALT, > + .clkr = { > + .enable_reg = 0xe58, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "video_cc_ahb_clk", > + .parent_data = &(const struct clk_parent_data){ > + .hw = &video_cc_ahb_clk_src.clkr.hw, > + }, > + .num_parents = 1, > + .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT, Similar critical clk comment, see below. > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > + > +static struct clk_branch video_cc_mvs0_clk = { > + .halt_reg = 0xd34, > + .halt_check = BRANCH_HALT_SKIP, /* TODO: hw gated ? */ Is this resolved? > + .clkr = { > + .enable_reg = 0xd34, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "video_cc_mvs0_clk", > + .parent_data = &(const struct clk_parent_data){ > + .hw = &video_cc_mvs0_div_clk_src.clkr.hw, > + }, > + .num_parents = 1, > + .flags = CLK_SET_RATE_PARENT, > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > + [...] > + > +static struct clk_branch video_cc_xo_clk = { > + .halt_reg = 0xeec, > + .halt_check = BRANCH_HALT, > + .clkr = { > + .enable_reg = 0xeec, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "video_cc_xo_clk", > + .parent_data = &(const struct clk_parent_data){ > + .hw = &video_cc_xo_clk_src.clkr.hw, > + }, > + .num_parents = 1, > + .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT, Please add a coment why it's critical. If no consumer of this clk exists it's preferred to move the enabling to probe and not waste memory modeling it in software. > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > +
Quoting Jonathan Marek (2020-09-03 20:09:51) > Add device tree bindings for video clock controller for SM8150 SoCs. > > Signed-off-by: Jonathan Marek <jonathan@marek.ca> > --- This one should be fine after sorting the lists in the first patch.
Quoting Jonathan Marek (2020-09-03 20:09:52) > diff --git a/Documentation/devicetree/bindings/clock/qcom,videocc.yaml b/Documentation/devicetree/bindings/clock/qcom,videocc.yaml > index d04f5bd28dde..66a6066ae353 100644 > --- a/Documentation/devicetree/bindings/clock/qcom,videocc.yaml > +++ b/Documentation/devicetree/bindings/clock/qcom,videocc.yaml > @@ -24,14 +25,19 @@ properties: > - qcom,sdm845-videocc > - qcom,sc7180-videocc > - qcom,sm8150-videocc > + - qcom,sm8250-videocc > > clocks: > + minItems: 1 > items: > - description: Board XO source > + - description: Board XO source, always-on (required by sm8250 only) I'd expect every SoC to need it. Can you leave it out for now and we can add it when it becomes required?
On 9/22/20 2:46 PM, Stephen Boyd wrote: > Quoting Jonathan Marek (2020-09-03 20:09:54) >> Add support for the video clock controller found on SM8250 based devices. >> >> Derived from the downstream driver. >> >> Signed-off-by: Jonathan Marek <jonathan@marek.ca> >> --- >> drivers/clk/qcom/Kconfig | 9 + >> drivers/clk/qcom/Makefile | 1 + >> drivers/clk/qcom/videocc-sm8250.c | 518 ++++++++++++++++++++++++++++++ >> 3 files changed, 528 insertions(+) >> create mode 100644 drivers/clk/qcom/videocc-sm8250.c >> >> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig >> index 40d7ee9886c9..95efa38211d5 100644 >> --- a/drivers/clk/qcom/Kconfig >> +++ b/drivers/clk/qcom/Kconfig >> @@ -453,6 +453,15 @@ config SM_VIDEOCC_8150 >> Say Y if you want to support video devices and functionality such as >> video encode and decode. >> >> +config SM_VIDEOCC_8250 >> + tristate "SM8250 Video Clock Controller" >> + select SDM_GCC_8250 >> + select QCOM_GDSC >> + help >> + Support for the video clock controller on SM8250 devices. >> + Say Y if you want to support video devices and functionality such as >> + video encode and decode. >> + >> config SPMI_PMIC_CLKDIV >> tristate "SPMI PMIC clkdiv Support" >> depends on SPMI || COMPILE_TEST >> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile >> index 6f4c580d2728..55fb20800b66 100644 >> --- a/drivers/clk/qcom/Makefile >> +++ b/drivers/clk/qcom/Makefile >> @@ -69,6 +69,7 @@ obj-$(CONFIG_SM_GCC_8250) += gcc-sm8250.o >> obj-$(CONFIG_SM_GPUCC_8150) += gpucc-sm8150.o >> obj-$(CONFIG_SM_GPUCC_8250) += gpucc-sm8250.o >> obj-$(CONFIG_SM_VIDEOCC_8150) += videocc-sm8150.o >> +obj-$(CONFIG_SM_VIDEOCC_8250) += videocc-sm8250.o >> obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o >> obj-$(CONFIG_KPSS_XCC) += kpss-xcc.o >> obj-$(CONFIG_QCOM_HFPLL) += hfpll.o >> diff --git a/drivers/clk/qcom/videocc-sm8250.c b/drivers/clk/qcom/videocc-sm8250.c >> new file mode 100644 >> index 000000000000..a814d10945c4 >> --- /dev/null >> +++ b/drivers/clk/qcom/videocc-sm8250.c >> @@ -0,0 +1,518 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (c) 2018-2020, The Linux Foundation. All rights reserved. >> + */ >> + >> +#include <linux/clk-provider.h> >> +#include <linux/module.h> >> +#include <linux/platform_device.h> >> +#include <linux/regmap.h> >> + >> +#include <dt-bindings/clock/qcom,videocc-sm8250.h> >> + > [...] >> +static struct clk_rcg2 video_cc_ahb_clk_src = { >> + .cmd_rcgr = 0xbd4, >> + .mnd_width = 0, >> + .hid_width = 5, >> + .parent_map = video_cc_parent_map_0, >> + .freq_tbl = ftbl_video_cc_ahb_clk_src, >> + .clkr.hw.init = &(struct clk_init_data){ >> + .name = "video_cc_ahb_clk_src", >> + .parent_data = video_cc_parent_data_0, >> + .num_parents = ARRAY_SIZE(video_cc_parent_data_0), >> + .flags = CLK_SET_RATE_PARENT, >> + .ops = &clk_rcg2_shared_ops, >> + }, >> +}; >> + >> +static struct clk_rcg2 video_cc_xo_clk_src = { >> + .cmd_rcgr = 0xecc, >> + .mnd_width = 0, >> + .hid_width = 5, >> + .parent_map = video_cc_parent_map_0, >> + .freq_tbl = ftbl_video_cc_ahb_clk_src, >> + .clkr.hw.init = &(struct clk_init_data){ >> + .name = "video_cc_xo_clk_src", >> + .parent_data = video_cc_parent_data_0, >> + .num_parents = ARRAY_SIZE(video_cc_parent_data_0), >> + .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT, > > Similar critical clk comment, see below. > >> + .ops = &clk_rcg2_ops, >> + }, >> +}; >> + >> +static struct clk_branch video_cc_ahb_clk = { >> + .halt_reg = 0xe58, >> + .halt_check = BRANCH_HALT, >> + .clkr = { >> + .enable_reg = 0xe58, >> + .enable_mask = BIT(0), >> + .hw.init = &(struct clk_init_data){ >> + .name = "video_cc_ahb_clk", >> + .parent_data = &(const struct clk_parent_data){ >> + .hw = &video_cc_ahb_clk_src.clkr.hw, >> + }, >> + .num_parents = 1, >> + .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT, > > Similar critical clk comment, see below. > >> + .ops = &clk_branch2_ops, >> + }, >> + }, >> +}; >> + >> +static struct clk_branch video_cc_mvs0_clk = { >> + .halt_reg = 0xd34, >> + .halt_check = BRANCH_HALT_SKIP, /* TODO: hw gated ? */ > > Is this resolved? > Downstream has this clock as BRANCH_HALT_VOTED, but with the upstream venus driver (with patches to enable sm8250), that results in a "video_cc_mvs0_clk status stuck at 'off" error. AFAIK venus enables/disables this clock on its own (venus still works without touching this clock), but I didn't want to remove this in case it might be needed. I removed these clocks in the v3 I just sent. >> + .clkr = { >> + .enable_reg = 0xd34, >> + .enable_mask = BIT(0), >> + .hw.init = &(struct clk_init_data){ >> + .name = "video_cc_mvs0_clk", >> + .parent_data = &(const struct clk_parent_data){ >> + .hw = &video_cc_mvs0_div_clk_src.clkr.hw, >> + }, >> + .num_parents = 1, >> + .flags = CLK_SET_RATE_PARENT, >> + .ops = &clk_branch2_ops, >> + }, >> + }, >> +}; >> + > [...] >> + >> +static struct clk_branch video_cc_xo_clk = { >> + .halt_reg = 0xeec, >> + .halt_check = BRANCH_HALT, >> + .clkr = { >> + .enable_reg = 0xeec, >> + .enable_mask = BIT(0), >> + .hw.init = &(struct clk_init_data){ >> + .name = "video_cc_xo_clk", >> + .parent_data = &(const struct clk_parent_data){ >> + .hw = &video_cc_xo_clk_src.clkr.hw, >> + }, >> + .num_parents = 1, >> + .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT, > > Please add a coment why it's critical. If no consumer of this clk exists > it's preferred to move the enabling to probe and not waste memory > modeling it in software. > >> + .ops = &clk_branch2_ops, >> + }, >> + }, >> +}; >> +
Quoting Jonathan Marek (2020-09-23 09:07:16) > On 9/22/20 2:46 PM, Stephen Boyd wrote: > > Quoting Jonathan Marek (2020-09-03 20:09:54) > > > >> + .ops = &clk_branch2_ops, > >> + }, > >> + }, > >> +}; > >> + > >> +static struct clk_branch video_cc_mvs0_clk = { > >> + .halt_reg = 0xd34, > >> + .halt_check = BRANCH_HALT_SKIP, /* TODO: hw gated ? */ > > > > Is this resolved? > > > > Downstream has this clock as BRANCH_HALT_VOTED, but with the upstream > venus driver (with patches to enable sm8250), that results in a > "video_cc_mvs0_clk status stuck at 'off" error. AFAIK venus > enables/disables this clock on its own (venus still works without > touching this clock), but I didn't want to remove this in case it might > be needed. I removed these clocks in the v3 I just sent. > Hmm. Does downstream use these clks? There have been some clk stuck problems with venus recently that were attributed to improperly enabling clks before enabling interconnects and power domains. Maybe it's the same problem.
On 9/23/20 7:30 PM, Stephen Boyd wrote: > Quoting Jonathan Marek (2020-09-23 09:07:16) >> On 9/22/20 2:46 PM, Stephen Boyd wrote: >>> Quoting Jonathan Marek (2020-09-03 20:09:54) >>> >>>> + .ops = &clk_branch2_ops, >>>> + }, >>>> + }, >>>> +}; >>>> + >>>> +static struct clk_branch video_cc_mvs0_clk = { >>>> + .halt_reg = 0xd34, >>>> + .halt_check = BRANCH_HALT_SKIP, /* TODO: hw gated ? */ >>> >>> Is this resolved? >>> >> >> Downstream has this clock as BRANCH_HALT_VOTED, but with the upstream >> venus driver (with patches to enable sm8250), that results in a >> "video_cc_mvs0_clk status stuck at 'off" error. AFAIK venus >> enables/disables this clock on its own (venus still works without >> touching this clock), but I didn't want to remove this in case it might >> be needed. I removed these clocks in the v3 I just sent. >> > > Hmm. Does downstream use these clks? There have been some clk stuck > problems with venus recently that were attributed to improperly enabling > clks before enabling interconnects and power domains. Maybe it's the > same problem. > Yes, downstream uses these clks. The "stuck" problem still happens if GSDCS/interconnects are always on, and like I mentioned, venus works even with these clocks completely removed. I think venus controls these clocks (and downstream just happens to try enabling it at a point where venus has already enabled it?). I'm not too sure about this, it might have something to do with the GDSC having the HW_CTRL flag too..
Quoting Jonathan Marek (2020-09-23 17:54:59) > On 9/23/20 7:30 PM, Stephen Boyd wrote: > > Quoting Jonathan Marek (2020-09-23 09:07:16) > >> On 9/22/20 2:46 PM, Stephen Boyd wrote: > >>> Quoting Jonathan Marek (2020-09-03 20:09:54) > >>> > >>>> + .ops = &clk_branch2_ops, > >>>> + }, > >>>> + }, > >>>> +}; > >>>> + > >>>> +static struct clk_branch video_cc_mvs0_clk = { > >>>> + .halt_reg = 0xd34, > >>>> + .halt_check = BRANCH_HALT_SKIP, /* TODO: hw gated ? */ > >>> > >>> Is this resolved? > >>> > >> > >> Downstream has this clock as BRANCH_HALT_VOTED, but with the upstream > >> venus driver (with patches to enable sm8250), that results in a > >> "video_cc_mvs0_clk status stuck at 'off" error. AFAIK venus > >> enables/disables this clock on its own (venus still works without > >> touching this clock), but I didn't want to remove this in case it might > >> be needed. I removed these clocks in the v3 I just sent. > >> > > > > Hmm. Does downstream use these clks? There have been some clk stuck > > problems with venus recently that were attributed to improperly enabling > > clks before enabling interconnects and power domains. Maybe it's the > > same problem. > > > > Yes, downstream uses these clks. > > The "stuck" problem still happens if GSDCS/interconnects are always on, > and like I mentioned, venus works even with these clocks completely > removed. > > I think venus controls these clocks (and downstream just happens to try > enabling it at a point where venus has already enabled it?). I'm not too > sure about this, it might have something to do with the GDSC having the > HW_CTRL flag too.. Ok. Maybe Taniya has an idea.
Add videocc drivers for SM8150/SM8250 required to boot and use venus. v2: - fixed dt_binding_check/checkpatch warnings in SM8250 bindings - added 19.2Mhz in SM8250 freq tbls for consistency with other videocc Jonathan Marek (5): dt-bindings: clock: combine qcom,sdm845-videocc and qcom,sc7180-videocc dt-bindings: clock: add SM8150 QCOM video clock bindings dt-bindings: clock: add SM8250 QCOM video clock bindings clk: qcom: add video clock controller driver for SM8150 clk: qcom: add video clock controller driver for SM8250 .../bindings/clock/qcom,sc7180-videocc.yaml | 65 --- ...,sdm845-videocc.yaml => qcom,videocc.yaml} | 22 +- drivers/clk/qcom/Kconfig | 18 + drivers/clk/qcom/Makefile | 2 + drivers/clk/qcom/videocc-sm8150.c | 276 ++++++++++ drivers/clk/qcom/videocc-sm8250.c | 518 ++++++++++++++++++ .../dt-bindings/clock/qcom,videocc-sm8150.h | 25 + .../dt-bindings/clock/qcom,videocc-sm8250.h | 42 ++ 8 files changed, 898 insertions(+), 70 deletions(-) delete mode 100644 Documentation/devicetree/bindings/clock/qcom,sc7180-videocc.yaml rename Documentation/devicetree/bindings/clock/{qcom,sdm845-videocc.yaml => qcom,videocc.yaml} (62%) create mode 100644 drivers/clk/qcom/videocc-sm8150.c create mode 100644 drivers/clk/qcom/videocc-sm8250.c create mode 100644 include/dt-bindings/clock/qcom,videocc-sm8150.h create mode 100644 include/dt-bindings/clock/qcom,videocc-sm8250.h