| Message ID | 20210109134617.146275-2-angelogioacchino.delregno@somainline.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Clock fixes for MSM8998 GCC, MMCC, GPUCC | expand |
On Sat, Jan 09, 2021 at 02:46:09PM +0100, AngeloGioacchino Del Regno wrote: > This clock enables the GPLL0 output to the multimedia subsystem > clock controller. > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> > --- > drivers/clk/qcom/gcc-msm8998.c | 17 +++++++++++++++++ > include/dt-bindings/clock/qcom,gcc-msm8998.h | 1 + Please put all the dt header changes in their own patch. > 2 files changed, 18 insertions(+) > > diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c > index 9d7016bcd680..d51c556851ca 100644 > --- a/drivers/clk/qcom/gcc-msm8998.c > +++ b/drivers/clk/qcom/gcc-msm8998.c > @@ -1341,6 +1341,22 @@ static struct clk_branch gcc_boot_rom_ahb_clk = { > }, > }; > > +static struct clk_branch gcc_mmss_gpll0_clk = { > + .halt_check = BRANCH_HALT_DELAY, > + .clkr = { > + .enable_reg = 0x5200c, > + .enable_mask = BIT(1), > + .hw.init = &(struct clk_init_data){ > + .name = "gcc_mmss_gpll0_clk", > + .parent_names = (const char *[]){ > + "gpll0_out_main", > + }, > + .num_parents = 1, > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > + > static struct clk_branch gcc_mss_gpll0_div_clk_src = { > .halt_check = BRANCH_HALT_DELAY, > .clkr = { > @@ -2944,6 +2960,7 @@ static struct clk_regmap *gcc_msm8998_clocks[] = { > [GCC_MSS_GPLL0_DIV_CLK_SRC] = &gcc_mss_gpll0_div_clk_src.clkr, > [GCC_MSS_SNOC_AXI_CLK] = &gcc_mss_snoc_axi_clk.clkr, > [GCC_MSS_MNOC_BIMC_AXI_CLK] = &gcc_mss_mnoc_bimc_axi_clk.clkr, > + [GCC_MMSS_GPLL0_CLK] = &gcc_mmss_gpll0_clk.clkr, > }; > > static struct gdsc *gcc_msm8998_gdscs[] = { > diff --git a/include/dt-bindings/clock/qcom,gcc-msm8998.h b/include/dt-bindings/clock/qcom,gcc-msm8998.h > index 6a73a174f049..47ca17df780b 100644 > --- a/include/dt-bindings/clock/qcom,gcc-msm8998.h > +++ b/include/dt-bindings/clock/qcom,gcc-msm8998.h > @@ -184,6 +184,7 @@ > #define GCC_MSS_MNOC_BIMC_AXI_CLK 175 > #define GCC_BIMC_GFX_CLK 176 > #define UFS_UNIPRO_CORE_CLK_SRC 177 > +#define GCC_MMSS_GPLL0_CLK 178 > > #define PCIE_0_GDSC 0 > #define UFS_GDSC 1 > -- > 2.29.2 >
Il 14/01/21 20:07, Rob Herring ha scritto: > On Sat, Jan 09, 2021 at 02:46:09PM +0100, AngeloGioacchino Del Regno wrote: >> This clock enables the GPLL0 output to the multimedia subsystem >> clock controller. >> >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> >> --- >> drivers/clk/qcom/gcc-msm8998.c | 17 +++++++++++++++++ >> include/dt-bindings/clock/qcom,gcc-msm8998.h | 1 + > > Please put all the dt header changes in their own patch. > I thought that this was fine, since I couldn't find *any* patch that is split like this... at least, if you look at dt-bindings/clock/qcom,gcc-{msm8974,msm8994,msm8996,msm8998,qcs404,sdm660,sdm845,sm8150} and qcom,mmcc{apq8084,msm8974,msm8996} ..... and others, at least from what I can see, nobody has split a code addition requiring that header update from the actual code. But if that's a new new new new rule, at this point, I can send a v2 of this series. I don't mean to disrespect, nor to be rude in any way but... are you sure? :)) --Angelo >> 2 files changed, 18 insertions(+) >> >> diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c >> index 9d7016bcd680..d51c556851ca 100644 >> --- a/drivers/clk/qcom/gcc-msm8998.c >> +++ b/drivers/clk/qcom/gcc-msm8998.c >> @@ -1341,6 +1341,22 @@ static struct clk_branch gcc_boot_rom_ahb_clk = { >> }, >> }; >> >> +static struct clk_branch gcc_mmss_gpll0_clk = { >> + .halt_check = BRANCH_HALT_DELAY, >> + .clkr = { >> + .enable_reg = 0x5200c, >> + .enable_mask = BIT(1), >> + .hw.init = &(struct clk_init_data){ >> + .name = "gcc_mmss_gpll0_clk", >> + .parent_names = (const char *[]){ >> + "gpll0_out_main", >> + }, >> + .num_parents = 1, >> + .ops = &clk_branch2_ops, >> + }, >> + }, >> +}; >> + >> static struct clk_branch gcc_mss_gpll0_div_clk_src = { >> .halt_check = BRANCH_HALT_DELAY, >> .clkr = { >> @@ -2944,6 +2960,7 @@ static struct clk_regmap *gcc_msm8998_clocks[] = { >> [GCC_MSS_GPLL0_DIV_CLK_SRC] = &gcc_mss_gpll0_div_clk_src.clkr, >> [GCC_MSS_SNOC_AXI_CLK] = &gcc_mss_snoc_axi_clk.clkr, >> [GCC_MSS_MNOC_BIMC_AXI_CLK] = &gcc_mss_mnoc_bimc_axi_clk.clkr, >> + [GCC_MMSS_GPLL0_CLK] = &gcc_mmss_gpll0_clk.clkr, >> }; >> >> static struct gdsc *gcc_msm8998_gdscs[] = { >> diff --git a/include/dt-bindings/clock/qcom,gcc-msm8998.h b/include/dt-bindings/clock/qcom,gcc-msm8998.h >> index 6a73a174f049..47ca17df780b 100644 >> --- a/include/dt-bindings/clock/qcom,gcc-msm8998.h >> +++ b/include/dt-bindings/clock/qcom,gcc-msm8998.h >> @@ -184,6 +184,7 @@ >> #define GCC_MSS_MNOC_BIMC_AXI_CLK 175 >> #define GCC_BIMC_GFX_CLK 176 >> #define UFS_UNIPRO_CORE_CLK_SRC 177 >> +#define GCC_MMSS_GPLL0_CLK 178 >> >> #define PCIE_0_GDSC 0 >> #define UFS_GDSC 1 >> -- >> 2.29.2 >>
On Sat, Jan 9, 2021 at 6:47 AM AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> wrote: > > This clock enables the GPLL0 output to the multimedia subsystem > clock controller. > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> Any reason why you are not also adding the div_clk?
Il 14/01/21 23:12, Jeffrey Hugo ha scritto: > On Sat, Jan 9, 2021 at 6:47 AM AngeloGioacchino Del Regno > <angelogioacchino.delregno@somainline.org> wrote: >> >> This clock enables the GPLL0 output to the multimedia subsystem >> clock controller. >> >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> > > Any reason why you are not also adding the div_clk? > Yes, just one: I haven't tested it... and my devices worked without. Perhaps we can add it whenever we find out if something really needs it?
On Thu, Jan 14, 2021 at 3:13 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> wrote: > > Il 14/01/21 23:12, Jeffrey Hugo ha scritto: > > On Sat, Jan 9, 2021 at 6:47 AM AngeloGioacchino Del Regno > > <angelogioacchino.delregno@somainline.org> wrote: > >> > >> This clock enables the GPLL0 output to the multimedia subsystem > >> clock controller. > >> > >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> > > > > Any reason why you are not also adding the div_clk? > > > > Yes, just one: I haven't tested it... and my devices worked without. > Perhaps we can add it whenever we find out if something really needs it? I'm mildly surprised you need to turn on the gate to the PLL0 out, but not the div_out. The div_out/div_clk is also fed into every RCG that exists in the MMCC. Per the frequency plan the following RCGs require it - cci cpp fd_core camss_gp[0-1] jpeg0 mclk[0-3] csi[0-2]phytimer dp_gtc maxi axi ahb Also, I'm very interested in all things 8998, and would generally appreciate being added to the to: list.
Il 14/01/21 23:33, Jeffrey Hugo ha scritto: > On Thu, Jan 14, 2021 at 3:13 PM AngeloGioacchino Del Regno > <angelogioacchino.delregno@somainline.org> wrote: >> >> Il 14/01/21 23:12, Jeffrey Hugo ha scritto: >>> On Sat, Jan 9, 2021 at 6:47 AM AngeloGioacchino Del Regno >>> <angelogioacchino.delregno@somainline.org> wrote: >>>> >>>> This clock enables the GPLL0 output to the multimedia subsystem >>>> clock controller. >>>> >>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> >>> >>> Any reason why you are not also adding the div_clk? >>> >> >> Yes, just one: I haven't tested it... and my devices worked without. >> Perhaps we can add it whenever we find out if something really needs it? > > I'm mildly surprised you need to turn on the gate to the PLL0 out, but > not the div_out. The div_out/div_clk is also fed into every RCG that > exists in the MMCC. > > Per the frequency plan the following RCGs require it - > > cci > cpp > fd_core > camss_gp[0-1] > jpeg0 > mclk[0-3] > csi[0-2]phytimer > dp_gtc > maxi > axi > ahb > > Also, I'm very interested in all things 8998, and would generally > appreciate being added to the to: list. > To be honest, I was surprised as well because.. yes, I know that these RCGs seem to need it, but then their clock tables don't contain any reference to the gpll0 divider, hence it's never getting used - and that works great, for now. I am aware of the fact that the clocks that you've mentioned are using the divider to reduce jitter, but I haven't done any camera test on my devices yet: that's definitely in my plans and I really can't wait to do that (as I successfully did for SDM630/660), but... we have more than 100 patches in our trees. We need to get upstream in the same working order as what we have here, so that we don't diverge that much and our work is kept in a maintainable state (avoiding to lose pieces around). I'm sure that I'll send a commit adding the gpll0 divider branch as soon as I will start the camera work: I feel it, it's going to give me issues without, in that field. By the way, noted. I'll make sure to add you in the to/cc for all of the next series regarding 8998 that I'll send. Meanwhile, you may want to check out all the recent patches that I've sent, as like 90% are MSM8998-centric... :)) -- Angelo
On Thu, Jan 14, 2021 at 3:40 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> wrote: > > Il 14/01/21 23:33, Jeffrey Hugo ha scritto: > > On Thu, Jan 14, 2021 at 3:13 PM AngeloGioacchino Del Regno > > <angelogioacchino.delregno@somainline.org> wrote: > >> > >> Il 14/01/21 23:12, Jeffrey Hugo ha scritto: > >>> On Sat, Jan 9, 2021 at 6:47 AM AngeloGioacchino Del Regno > >>> <angelogioacchino.delregno@somainline.org> wrote: > >>>> > >>>> This clock enables the GPLL0 output to the multimedia subsystem > >>>> clock controller. > >>>> > >>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> > >>> > >>> Any reason why you are not also adding the div_clk? > >>> > >> > >> Yes, just one: I haven't tested it... and my devices worked without. > >> Perhaps we can add it whenever we find out if something really needs it? > > > > I'm mildly surprised you need to turn on the gate to the PLL0 out, but > > not the div_out. The div_out/div_clk is also fed into every RCG that > > exists in the MMCC. > > > > Per the frequency plan the following RCGs require it - > > > > cci > > cpp > > fd_core > > camss_gp[0-1] > > jpeg0 > > mclk[0-3] > > csi[0-2]phytimer > > dp_gtc > > maxi > > axi > > ahb > > > > Also, I'm very interested in all things 8998, and would generally > > appreciate being added to the to: list. > > > > To be honest, I was surprised as well because.. yes, I know that these > RCGs seem to need it, but then their clock tables don't contain any > reference to the gpll0 divider, hence it's never getting used - and that > works great, for now. > > I am aware of the fact that the clocks that you've mentioned are using > the divider to reduce jitter, but I haven't done any camera test on my > devices yet: that's definitely in my plans and I really can't wait to do > that (as I successfully did for SDM630/660), but... we have more than > 100 patches in our trees. > We need to get upstream in the same working order as what we have here, > so that we don't diverge that much and our work is kept in a > maintainable state (avoiding to lose pieces around). > > I'm sure that I'll send a commit adding the gpll0 divider branch as soon > as I will start the camera work: I feel it, it's going to give me issues > without, in that field. > > By the way, noted. I'll make sure to add you in the to/cc for all of the > next series regarding 8998 that I'll send. > > Meanwhile, you may want to check out all the recent patches that I've > sent, as like 90% are MSM8998-centric... :)) I noticed, and I'm excited to see additional work since I've had a lack of spare time, although I think you've monopolized my backlog :)
Il 14/01/21 23:44, Jeffrey Hugo ha scritto: > On Thu, Jan 14, 2021 at 3:40 PM AngeloGioacchino Del Regno > <angelogioacchino.delregno@somainline.org> wrote: >> >> Il 14/01/21 23:33, Jeffrey Hugo ha scritto: >>> On Thu, Jan 14, 2021 at 3:13 PM AngeloGioacchino Del Regno >>> <angelogioacchino.delregno@somainline.org> wrote: >>>> >>>> Il 14/01/21 23:12, Jeffrey Hugo ha scritto: >>>>> On Sat, Jan 9, 2021 at 6:47 AM AngeloGioacchino Del Regno >>>>> <angelogioacchino.delregno@somainline.org> wrote: >>>>>> >>>>>> This clock enables the GPLL0 output to the multimedia subsystem >>>>>> clock controller. >>>>>> >>>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> >>>>> >>>>> Any reason why you are not also adding the div_clk? >>>>> >>>> >>>> Yes, just one: I haven't tested it... and my devices worked without. >>>> Perhaps we can add it whenever we find out if something really needs it? >>> >>> I'm mildly surprised you need to turn on the gate to the PLL0 out, but >>> not the div_out. The div_out/div_clk is also fed into every RCG that >>> exists in the MMCC. >>> >>> Per the frequency plan the following RCGs require it - >>> >>> cci >>> cpp >>> fd_core >>> camss_gp[0-1] >>> jpeg0 >>> mclk[0-3] >>> csi[0-2]phytimer >>> dp_gtc >>> maxi >>> axi >>> ahb >>> >>> Also, I'm very interested in all things 8998, and would generally >>> appreciate being added to the to: list. >>> >> >> To be honest, I was surprised as well because.. yes, I know that these >> RCGs seem to need it, but then their clock tables don't contain any >> reference to the gpll0 divider, hence it's never getting used - and that >> works great, for now. >> >> I am aware of the fact that the clocks that you've mentioned are using >> the divider to reduce jitter, but I haven't done any camera test on my >> devices yet: that's definitely in my plans and I really can't wait to do >> that (as I successfully did for SDM630/660), but... we have more than >> 100 patches in our trees. >> We need to get upstream in the same working order as what we have here, >> so that we don't diverge that much and our work is kept in a >> maintainable state (avoiding to lose pieces around). >> >> I'm sure that I'll send a commit adding the gpll0 divider branch as soon >> as I will start the camera work: I feel it, it's going to give me issues >> without, in that field. >> >> By the way, noted. I'll make sure to add you in the to/cc for all of the >> next series regarding 8998 that I'll send. >> >> Meanwhile, you may want to check out all the recent patches that I've >> sent, as like 90% are MSM8998-centric... :)) > > I noticed, and I'm excited to see additional work since I've had a > lack of spare time, although I think you've monopolized my backlog :) > I just... had some time... and passion about it :))) P.S.: I'm not done yet!! :)
diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c index 9d7016bcd680..d51c556851ca 100644 --- a/drivers/clk/qcom/gcc-msm8998.c +++ b/drivers/clk/qcom/gcc-msm8998.c @@ -1341,6 +1341,22 @@ static struct clk_branch gcc_boot_rom_ahb_clk = { }, }; +static struct clk_branch gcc_mmss_gpll0_clk = { + .halt_check = BRANCH_HALT_DELAY, + .clkr = { + .enable_reg = 0x5200c, + .enable_mask = BIT(1), + .hw.init = &(struct clk_init_data){ + .name = "gcc_mmss_gpll0_clk", + .parent_names = (const char *[]){ + "gpll0_out_main", + }, + .num_parents = 1, + .ops = &clk_branch2_ops, + }, + }, +}; + static struct clk_branch gcc_mss_gpll0_div_clk_src = { .halt_check = BRANCH_HALT_DELAY, .clkr = { @@ -2944,6 +2960,7 @@ static struct clk_regmap *gcc_msm8998_clocks[] = { [GCC_MSS_GPLL0_DIV_CLK_SRC] = &gcc_mss_gpll0_div_clk_src.clkr, [GCC_MSS_SNOC_AXI_CLK] = &gcc_mss_snoc_axi_clk.clkr, [GCC_MSS_MNOC_BIMC_AXI_CLK] = &gcc_mss_mnoc_bimc_axi_clk.clkr, + [GCC_MMSS_GPLL0_CLK] = &gcc_mmss_gpll0_clk.clkr, }; static struct gdsc *gcc_msm8998_gdscs[] = { diff --git a/include/dt-bindings/clock/qcom,gcc-msm8998.h b/include/dt-bindings/clock/qcom,gcc-msm8998.h index 6a73a174f049..47ca17df780b 100644 --- a/include/dt-bindings/clock/qcom,gcc-msm8998.h +++ b/include/dt-bindings/clock/qcom,gcc-msm8998.h @@ -184,6 +184,7 @@ #define GCC_MSS_MNOC_BIMC_AXI_CLK 175 #define GCC_BIMC_GFX_CLK 176 #define UFS_UNIPRO_CORE_CLK_SRC 177 +#define GCC_MMSS_GPLL0_CLK 178 #define PCIE_0_GDSC 0 #define UFS_GDSC 1
This clock enables the GPLL0 output to the multimedia subsystem clock controller. Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org> --- drivers/clk/qcom/gcc-msm8998.c | 17 +++++++++++++++++ include/dt-bindings/clock/qcom,gcc-msm8998.h | 1 + 2 files changed, 18 insertions(+)