[prev in list] [next in list] [prev in thread] [next in thread]
List: linux-kernel
Subject: Re: [PATCH V5 4/5] clk: qcom: camcc-sm8550: Add support for qdss, sleep and xo clocks
From: Konrad Dybcio <konrad.dybcio () linaro ! org>
Date: 2023-06-30 23:21:07
Message-ID: 35b32667-fa29-3e00-1cad-e12a3eb7f362 () linaro ! org
[Download RAW message or body]
On 30.06.2023 09:14, Jagadeesh Kona wrote:
>
>
> On 6/26/2023 7:10 PM, Dmitry Baryshkov wrote:
> > On 26/06/2023 14:59, Jagadeesh Kona wrote:
> > >
> > >
> > > On 6/24/2023 5:49 PM, Konrad Dybcio wrote:
> > > > On 23.06.2023 18:46, Jagadeesh Kona wrote:
> > > > > Add support for camera qdss, sleep and xo clocks.
> > > > >
> > > > > Co-developed-by: Taniya Das <quic_tdas@quicinc.com>
> > > > > Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
> > > > > Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> > > > > ---
> > > > > Changes since v4:
> > > > > - No changes.
> > > > > Changes since v3:
> > > > > - No changes.
> > > > > Changes since v2:
> > > > > - No changes.
> > > > > Changes since v1:
> > > > > - Newly added.
> > > > >
> > > > > drivers/clk/qcom/camcc-sm8550.c | 180 ++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 180 insertions(+)
> > > > >
> > > > > diff --git a/drivers/clk/qcom/camcc-sm8550.c b/drivers/clk/qcom/camcc-sm8550.c
> > > > > index 075bea32087c..7b4882444d58 100644
> > > > > --- a/drivers/clk/qcom/camcc-sm8550.c
> > > > > +++ b/drivers/clk/qcom/camcc-sm8550.c
> > > > > @@ -22,6 +22,8 @@
> > > > > enum {
> > > > > DT_IFACE,
> > > > > DT_BI_TCXO,
> > > > > + DT_BI_TCXO_AO,
> > > > > + DT_SLEEP_CLK,
> > > > > };
> > > > > enum {
> > > > > @@ -43,6 +45,7 @@ enum {
> > > > > P_CAM_CC_PLL10_OUT_EVEN,
> > > > > P_CAM_CC_PLL11_OUT_EVEN,
> > > > > P_CAM_CC_PLL12_OUT_EVEN,
> > > > > + P_SLEEP_CLK,
> > > > > };
> > > > > static const struct pll_vco lucid_ole_vco[] = {
> > > > > @@ -881,6 +884,22 @@ static const struct clk_parent_data cam_cc_parent_data_11[] = {
> > > > > { .hw = &cam_cc_pll7_out_even.clkr.hw },
> > > > > };
> > > > > +static const struct parent_map cam_cc_parent_map_12[] = {
> > > > > + { P_SLEEP_CLK, 0 },
> > > > > +};
> > > > > +
> > > > > +static const struct clk_parent_data cam_cc_parent_data_12[] = {
> > > > > + { .index = DT_SLEEP_CLK },
> > > > > +};
> > > > > +
> > > > > +static const struct parent_map cam_cc_parent_map_13[] = {
> > > > > + { P_BI_TCXO, 0 },
> > > > > +};
> > > > > +
> > > > > +static const struct clk_parent_data cam_cc_parent_data_13_ao[] = {
> > > > > + { .index = DT_BI_TCXO_AO },
> > > > > +};
> > > > > +
> > > > > static const struct freq_tbl ftbl_cam_cc_bps_clk_src[] = {
> > > > > F(19200000, P_BI_TCXO, 1, 0, 0),
> > > > > F(200000000, P_CAM_CC_PLL8_OUT_EVEN, 1, 0, 0),
> > > > > @@ -1565,6 +1584,29 @@ static struct clk_rcg2 cam_cc_mclk7_clk_src = {
> > > > > },
> > > > > };
> > > > > +static const struct freq_tbl ftbl_cam_cc_qdss_debug_clk_src[] = {
> > > > > + F(19200000, P_BI_TCXO, 1, 0, 0),
> > > > > + F(75000000, P_CAM_CC_PLL0_OUT_EVEN, 8, 0, 0),
> > > > > + F(150000000, P_CAM_CC_PLL0_OUT_EVEN, 4, 0, 0),
> > > > > + F(300000000, P_CAM_CC_PLL0_OUT_MAIN, 4, 0, 0),
> > > > > + { }
> > > > > +};
> > > > > +
> > > > > +static struct clk_rcg2 cam_cc_qdss_debug_clk_src = {
> > > > > + .cmd_rcgr = 0x13f24,
> > > > > + .mnd_width = 0,
> > > > > + .hid_width = 5,
> > > > > + .parent_map = cam_cc_parent_map_0,
> > > > > + .freq_tbl = ftbl_cam_cc_qdss_debug_clk_src,
> > > > > + .clkr.hw.init = &(const struct clk_init_data) {
> > > > > + .name = "cam_cc_qdss_debug_clk_src",
> > > > > + .parent_data = cam_cc_parent_data_0,
> > > > > + .num_parents = ARRAY_SIZE(cam_cc_parent_data_0),
> > > > > + .flags = CLK_SET_RATE_PARENT,
> > > > > + .ops = &clk_rcg2_shared_ops,
> > > > > + },
> > > > > +};
> > > > > +
> > > > > static const struct freq_tbl ftbl_cam_cc_sfe_0_clk_src[] = {
> > > > > F(466000000, P_CAM_CC_PLL6_OUT_EVEN, 1, 0, 0),
> > > > > F(594000000, P_CAM_CC_PLL6_OUT_EVEN, 1, 0, 0),
> > > > > @@ -1611,6 +1653,26 @@ static struct clk_rcg2 cam_cc_sfe_1_clk_src = {
> > > > > },
> > > > > };
> > > > > +static const struct freq_tbl ftbl_cam_cc_sleep_clk_src[] = {
> > > > > + F(32000, P_SLEEP_CLK, 1, 0, 0),
> > > > > + { }
> > > > > +};
> > > > > +
> > > > > +static struct clk_rcg2 cam_cc_sleep_clk_src = {
> > > > > + .cmd_rcgr = 0x141a0,
> > > > > + .mnd_width = 0,
> > > > > + .hid_width = 5,
> > > > > + .parent_map = cam_cc_parent_map_12,
> > > > > + .freq_tbl = ftbl_cam_cc_sleep_clk_src,
> > > > > + .clkr.hw.init = &(const struct clk_init_data) {
> > > > > + .name = "cam_cc_sleep_clk_src",
> > > > > + .parent_data = cam_cc_parent_data_12,
> > > > > + .num_parents = ARRAY_SIZE(cam_cc_parent_data_12),
> > > > > + .flags = CLK_SET_RATE_PARENT,
> > > > > + .ops = &clk_rcg2_shared_ops,
> > > > > + },
> > > > > +};
> > > > > +
> > > > > static const struct freq_tbl ftbl_cam_cc_slow_ahb_clk_src[] = {
> > > > > F(19200000, P_BI_TCXO, 1, 0, 0),
> > > > > F(80000000, P_CAM_CC_PLL0_OUT_EVEN, 7.5, 0, 0),
> > > > > @@ -1632,6 +1694,26 @@ static struct clk_rcg2 cam_cc_slow_ahb_clk_src = {
> > > > > },
> > > > > };
> > > > > +static const struct freq_tbl ftbl_cam_cc_xo_clk_src[] = {
> > > > > + F(19200000, P_BI_TCXO, 1, 0, 0),
> > > > You're overloading P_BI_TCXO with a different parent clock (XO_A).
> > > >
> > >
> > > This RCG just requires active only voting, hence using XO_A as its parent.
> > >
> > > Both XO and XO_A are same clock in HW (BI_TCXO), hence we can reuse P_BI_TCXO in frequency table \
> > > for XO_A parent as well.
> >
> > Please don't do such things, it complicates understanding the driver. The reviewer could have thought \
> > that here the driver was really referencing to the BI_TCXO rather than BI_TCXO_AO.
>
> The enum in parent list indicates the actual HW clock, and since XO and XO_A are the same HW clock, \
> parent enum needs to be a single one. Only parent_data needs to be updated with AO as we have been \
> doing for all targets.
I see your point, however to Linux, XO and XO_A are two separate clocks
within the CCF.
Konrad
>
> Thanks,
> Jagadeesh
>
> > >
> > > Thanks,
> > > Jagadeesh
> > >
> > > > The rest lgtm
> > > >
> > > > Konrad
> >
> > [skipped the rest]
> >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic