* [RFC PATCH 0/2] clk: qcom: Quad SPI (qspi) clock support for sdm845 @ 2018-07-18 18:04 Douglas Anderson 2018-07-18 18:04 ` [RFC PATCH 1/2] clk: qcom: Add qspi (Quad SPI) clock defines for sdm845 to header Douglas Anderson 2018-07-18 18:04 ` [RFC PATCH 2/2] clk: qcom: Add qspi (Quad SPI) clocks for sdm845 Douglas Anderson 0 siblings, 2 replies; 8+ messages in thread From: Douglas Anderson @ 2018-07-18 18:04 UTC (permalink / raw) To: sboyd, andy.gross Cc: tdas, girishm, anischal, bjorn.andersson, Douglas Anderson, devicetree, Michael Turquette, linux-arm-msm, linux-kernel, David Brown, Rob Herring, Mark Rutland, linux-soc, linux-clk This two-series patch adds the needed clock bits to use the Quad SPI (qspi) part on sdm845. It's expected that the bindings part of this patch could land in the clock tree with an immutable git hash and then be pulled into the Qualcomm tree so it could be used by dts files. NOTE: though I seem to have some register defines for these clocks, I don't actually have a clock plan that includes them. I also didn't see any use of Quad SPI in the sdm845 Android tree I looked at. Thus the frequency table here is totally made up. Specifically: - I know that GPLL0 is running at 600 MHz and that I can have parents of GPLL0, GPLL0_EVEN (AKA 300 MHz), and TCXO (AKA 19.2 MHz). - It seemed sane to add an entry in the table to come straight from BI_TCXO. - From probing lines, it appears that the Quad SPI block has a divide by 4 somewhere inside it (probably so it can oversample the lines, or possibly so it can generate phase-offset clocks). Thus we need the core to go 4 times faster than we'd expect to run the SPI bus. - At least one SPI flash part I looked at supported a clock frequency of 104 MHz, so it semeed nice to add clocks up to ~400 MHz. This is the first time I've tried to map Qualcomm register definition into the clock driver, so it'd be nice if someone could double-check and make sure I mapped all the numbers correctly. If nothing else it does appear to work though. Douglas Anderson (2): clk: qcom: Add qspi (Quad SPI) clock defines for sdm845 to header clk: qcom: Add qspi (Quad SPI) clocks for sdm845 drivers/clk/qcom/gcc-sdm845.c | 73 +++++++++++++++++++++ include/dt-bindings/clock/qcom,gcc-sdm845.h | 3 + 2 files changed, 76 insertions(+) -- 2.18.0.233.g985f88cf7e-goog ^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC PATCH 1/2] clk: qcom: Add qspi (Quad SPI) clock defines for sdm845 to header 2018-07-18 18:04 [RFC PATCH 0/2] clk: qcom: Quad SPI (qspi) clock support for sdm845 Douglas Anderson @ 2018-07-18 18:04 ` Douglas Anderson 2018-07-18 18:04 ` [RFC PATCH 2/2] clk: qcom: Add qspi (Quad SPI) clocks for sdm845 Douglas Anderson 1 sibling, 0 replies; 8+ messages in thread From: Douglas Anderson @ 2018-07-18 18:04 UTC (permalink / raw) To: sboyd, andy.gross Cc: tdas, girishm, anischal, bjorn.andersson, Douglas Anderson, devicetree, linux-kernel, Rob Herring, Mark Rutland These clocks will need to be defined in the clock driver and referenced in device tree files. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- include/dt-bindings/clock/qcom,gcc-sdm845.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/dt-bindings/clock/qcom,gcc-sdm845.h b/include/dt-bindings/clock/qcom,gcc-sdm845.h index f96fc2dbf60e..b8eae5a76503 100644 --- a/include/dt-bindings/clock/qcom,gcc-sdm845.h +++ b/include/dt-bindings/clock/qcom,gcc-sdm845.h @@ -194,6 +194,9 @@ #define GPLL4 184 #define GCC_CPUSS_DVM_BUS_CLK 185 #define GCC_CPUSS_GNOC_CLK 186 +#define GCC_QSPI_CORE_CLK_SRC 187 +#define GCC_QSPI_CORE_CLK 188 +#define GCC_QSPI_CNOC_PERIPH_AHB_CLK 189 /* GCC Resets */ #define GCC_MMSS_BCR 0 -- 2.18.0.233.g985f88cf7e-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [RFC PATCH 2/2] clk: qcom: Add qspi (Quad SPI) clocks for sdm845 2018-07-18 18:04 [RFC PATCH 0/2] clk: qcom: Quad SPI (qspi) clock support for sdm845 Douglas Anderson 2018-07-18 18:04 ` [RFC PATCH 1/2] clk: qcom: Add qspi (Quad SPI) clock defines for sdm845 to header Douglas Anderson @ 2018-07-18 18:04 ` Douglas Anderson 2018-07-19 0:19 ` grahamr 2018-07-19 11:04 ` Taniya Das 1 sibling, 2 replies; 8+ messages in thread From: Douglas Anderson @ 2018-07-18 18:04 UTC (permalink / raw) To: sboyd, andy.gross Cc: tdas, girishm, anischal, bjorn.andersson, Douglas Anderson, Michael Turquette, linux-arm-msm, linux-kernel, David Brown, linux-soc, linux-clk Add both the interface and core clock. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/clk/qcom/gcc-sdm845.c | 73 +++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c index 0f694ed4238a..2ee96f9bc217 100644 --- a/drivers/clk/qcom/gcc-sdm845.c +++ b/drivers/clk/qcom/gcc-sdm845.c @@ -162,6 +162,20 @@ static const char * const gcc_parent_names_10[] = { "core_bi_pll_test_se", }; +static const struct parent_map gcc_parent_map_9[] = { + { P_BI_TCXO, 0 }, + { P_GPLL0_OUT_MAIN, 1 }, + { P_GPLL0_OUT_EVEN, 6 }, + { P_SLEEP_CLK, 7 }, +}; + +static const char * const gcc_parent_names_9[] = { + "bi_tcxo", + "gpll0", + "gpll0_out_even", + "core_pi_sleep_clk", +}; + static struct clk_alpha_pll gpll0 = { .offset = 0x0, .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA], @@ -358,6 +372,31 @@ static struct clk_rcg2 gcc_pcie_phy_refgen_clk_src = { }, }; +static const struct freq_tbl ftbl_gcc_qspi_core_clk_src[] = { + F(19200000, P_BI_TCXO, 1, 0, 0), + F(50000000, P_GPLL0_OUT_EVEN, 6, 0, 0), + F(75000000, P_GPLL0_OUT_EVEN, 4, 0, 0), + F(100000000, P_GPLL0_OUT_EVEN, 3, 0, 0), + F(150000000, P_GPLL0_OUT_EVEN, 2, 0, 0), + F(300000000, P_GPLL0_OUT_EVEN, 1, 0, 0), + F(400000000, P_GPLL0_OUT_MAIN, 1.5, 0, 0), + { } +}; + +static struct clk_rcg2 gcc_qspi_core_clk_src = { + .cmd_rcgr = 0x4b008, + .mnd_width = 0, + .hid_width = 5, + .parent_map = gcc_parent_map_9, + .freq_tbl = ftbl_gcc_qspi_core_clk_src, + .clkr.hw.init = &(struct clk_init_data){ + .name = "gcc_qspi_core_clk_src", + .parent_names = gcc_parent_names_9, + .num_parents = 4, + .ops = &clk_rcg2_floor_ops, + }, +}; + static const struct freq_tbl ftbl_gcc_pdm2_clk_src[] = { F(9600000, P_BI_TCXO, 2, 0, 0), F(19200000, P_BI_TCXO, 1, 0, 0), @@ -1935,6 +1974,37 @@ static struct clk_branch gcc_qmip_video_ahb_clk = { }, }; +static struct clk_branch gcc_qspi_cnoc_periph_ahb_clk = { + .halt_reg = 0x4b000, + .halt_check = BRANCH_HALT, + .clkr = { + .enable_reg = 0x4b000, + .enable_mask = BIT(0), + .hw.init = &(struct clk_init_data){ + .name = "gcc_qspi_cnoc_periph_ahb_clk", + .ops = &clk_branch2_ops, + }, + }, +}; + +static struct clk_branch gcc_qspi_core_clk = { + .halt_reg = 0x4b004, + .halt_check = BRANCH_HALT, + .clkr = { + .enable_reg = 0x4b004, + .enable_mask = BIT(0), + .hw.init = &(struct clk_init_data){ + .name = "gcc_qspi_core_clk", + .parent_names = (const char *[]){ + "gcc_qspi_core_clk_src", + }, + .num_parents = 1, + .flags = CLK_SET_RATE_PARENT, + .ops = &clk_branch2_ops, + }, + }, +}; + static struct clk_branch gcc_qupv3_wrap0_s0_clk = { .halt_reg = 0x17030, .halt_check = BRANCH_HALT_VOTED, @@ -3383,6 +3453,9 @@ static struct clk_regmap *gcc_sdm845_clocks[] = { [GPLL4] = &gpll4.clkr, [GCC_CPUSS_DVM_BUS_CLK] = &gcc_cpuss_dvm_bus_clk.clkr, [GCC_CPUSS_GNOC_CLK] = &gcc_cpuss_gnoc_clk.clkr, + [GCC_QSPI_CORE_CLK_SRC] = &gcc_qspi_core_clk_src.clkr, + [GCC_QSPI_CORE_CLK] = &gcc_qspi_core_clk.clkr, + [GCC_QSPI_CNOC_PERIPH_AHB_CLK] = &gcc_qspi_cnoc_periph_ahb_clk.clkr, }; static const struct qcom_reset_map gcc_sdm845_resets[] = { -- 2.18.0.233.g985f88cf7e-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH 2/2] clk: qcom: Add qspi (Quad SPI) clocks for sdm845 2018-07-18 18:04 ` [RFC PATCH 2/2] clk: qcom: Add qspi (Quad SPI) clocks for sdm845 Douglas Anderson @ 2018-07-19 0:19 ` grahamr 2018-07-19 11:04 ` Taniya Das 1 sibling, 0 replies; 8+ messages in thread From: grahamr @ 2018-07-19 0:19 UTC (permalink / raw) To: Douglas Anderson Cc: sboyd, andy.gross, tdas, girishm, anischal, bjorn.andersson, Michael Turquette, linux-arm-msm, linux-kernel, David Brown, linux-soc, linux-clk, linux-clk-owner On 2018-07-18 11:04, Douglas Anderson wrote: > Add both the interface and core clock. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/clk/qcom/gcc-sdm845.c | 73 +++++++++++++++++++++++++++++++++++ > 1 file changed, 73 insertions(+) > > diff --git a/drivers/clk/qcom/gcc-sdm845.c > b/drivers/clk/qcom/gcc-sdm845.c > index 0f694ed4238a..2ee96f9bc217 100644 > --- a/drivers/clk/qcom/gcc-sdm845.c > +++ b/drivers/clk/qcom/gcc-sdm845.c > @@ -162,6 +162,20 @@ static const char * const gcc_parent_names_10[] = > { > "core_bi_pll_test_se", > }; > > +static const struct parent_map gcc_parent_map_9[] = { > + { P_BI_TCXO, 0 }, > + { P_GPLL0_OUT_MAIN, 1 }, > + { P_GPLL0_OUT_EVEN, 6 }, > + { P_SLEEP_CLK, 7 }, > +}; > + > +static const char * const gcc_parent_names_9[] = { > + "bi_tcxo", > + "gpll0", > + "gpll0_out_even", > + "core_pi_sleep_clk", > +}; > + > static struct clk_alpha_pll gpll0 = { > .offset = 0x0, > .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA], > @@ -358,6 +372,31 @@ static struct clk_rcg2 gcc_pcie_phy_refgen_clk_src > = { > }, > }; > > +static const struct freq_tbl ftbl_gcc_qspi_core_clk_src[] = { > + F(19200000, P_BI_TCXO, 1, 0, 0), > + F(50000000, P_GPLL0_OUT_EVEN, 6, 0, 0), > + F(75000000, P_GPLL0_OUT_EVEN, 4, 0, 0), > + F(100000000, P_GPLL0_OUT_EVEN, 3, 0, 0), > + F(150000000, P_GPLL0_OUT_EVEN, 2, 0, 0), > + F(300000000, P_GPLL0_OUT_EVEN, 1, 0, 0), > + F(400000000, P_GPLL0_OUT_MAIN, 1.5, 0, 0), > + { } > +}; Doug, 400 MHz is beyond the maximum frequency for the QSPI core, which is 300 MHz. The complete frequency table with required voltage corner is: MinSVS@19.2 LowSVS@75 SVS@150 Nominal@300 You can run at intermediate frequencies less than 300 if required by the QSPI driver - i.e. the other entries you have are fine. FYI, Qualcomm does not support QSPI internally on SDM845 which is why this code does not exist. Regards, Graham Roff > + > +static struct clk_rcg2 gcc_qspi_core_clk_src = { > + .cmd_rcgr = 0x4b008, > + .mnd_width = 0, > + .hid_width = 5, > + .parent_map = gcc_parent_map_9, > + .freq_tbl = ftbl_gcc_qspi_core_clk_src, > + .clkr.hw.init = &(struct clk_init_data){ > + .name = "gcc_qspi_core_clk_src", > + .parent_names = gcc_parent_names_9, > + .num_parents = 4, > + .ops = &clk_rcg2_floor_ops, > + }, > +}; > + > static const struct freq_tbl ftbl_gcc_pdm2_clk_src[] = { > F(9600000, P_BI_TCXO, 2, 0, 0), > F(19200000, P_BI_TCXO, 1, 0, 0), > @@ -1935,6 +1974,37 @@ static struct clk_branch gcc_qmip_video_ahb_clk > = { > }, > }; > > +static struct clk_branch gcc_qspi_cnoc_periph_ahb_clk = { > + .halt_reg = 0x4b000, > + .halt_check = BRANCH_HALT, > + .clkr = { > + .enable_reg = 0x4b000, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "gcc_qspi_cnoc_periph_ahb_clk", > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > + > +static struct clk_branch gcc_qspi_core_clk = { > + .halt_reg = 0x4b004, > + .halt_check = BRANCH_HALT, > + .clkr = { > + .enable_reg = 0x4b004, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "gcc_qspi_core_clk", > + .parent_names = (const char *[]){ > + "gcc_qspi_core_clk_src", > + }, > + .num_parents = 1, > + .flags = CLK_SET_RATE_PARENT, > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > + > static struct clk_branch gcc_qupv3_wrap0_s0_clk = { > .halt_reg = 0x17030, > .halt_check = BRANCH_HALT_VOTED, > @@ -3383,6 +3453,9 @@ static struct clk_regmap *gcc_sdm845_clocks[] = { > [GPLL4] = &gpll4.clkr, > [GCC_CPUSS_DVM_BUS_CLK] = &gcc_cpuss_dvm_bus_clk.clkr, > [GCC_CPUSS_GNOC_CLK] = &gcc_cpuss_gnoc_clk.clkr, > + [GCC_QSPI_CORE_CLK_SRC] = &gcc_qspi_core_clk_src.clkr, > + [GCC_QSPI_CORE_CLK] = &gcc_qspi_core_clk.clkr, > + [GCC_QSPI_CNOC_PERIPH_AHB_CLK] = &gcc_qspi_cnoc_periph_ahb_clk.clkr, > }; > > static const struct qcom_reset_map gcc_sdm845_resets[] = { ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH 2/2] clk: qcom: Add qspi (Quad SPI) clocks for sdm845 2018-07-18 18:04 ` [RFC PATCH 2/2] clk: qcom: Add qspi (Quad SPI) clocks for sdm845 Douglas Anderson 2018-07-19 0:19 ` grahamr @ 2018-07-19 11:04 ` Taniya Das 2018-07-19 17:55 ` Doug Anderson 1 sibling, 1 reply; 8+ messages in thread From: Taniya Das @ 2018-07-19 11:04 UTC (permalink / raw) To: Douglas Anderson, sboyd, andy.gross Cc: girishm, anischal, bjorn.andersson, Michael Turquette, linux-arm-msm, linux-kernel, David Brown, linux-soc, linux-clk Hi Doug, Please find my comments inline. On 7/18/2018 11:34 PM, Douglas Anderson wrote: > Add both the interface and core clock. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/clk/qcom/gcc-sdm845.c | 73 +++++++++++++++++++++++++++++++++++ > 1 file changed, 73 insertions(+) > > diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c > index 0f694ed4238a..2ee96f9bc217 100644 > --- a/drivers/clk/qcom/gcc-sdm845.c > +++ b/drivers/clk/qcom/gcc-sdm845.c > @@ -162,6 +162,20 @@ static const char * const gcc_parent_names_10[] = { > "core_bi_pll_test_se", > }; > > +static const struct parent_map gcc_parent_map_9[] = { > + { P_BI_TCXO, 0 }, > + { P_GPLL0_OUT_MAIN, 1 }, > + { P_GPLL0_OUT_EVEN, 6 }, > + { P_SLEEP_CLK, 7 }, SRC 7 has 'core_bi_pll_test_se' and not 'sleep_clk'. Please use the 'gcc_parent_map_0' > +}; > + > +static const char * const gcc_parent_names_9[] = { > + "bi_tcxo", > + "gpll0", > + "gpll0_out_even", > + "core_pi_sleep_clk", > +}; > + > static struct clk_alpha_pll gpll0 = { > .offset = 0x0, > .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA], > @@ -358,6 +372,31 @@ static struct clk_rcg2 gcc_pcie_phy_refgen_clk_src = { > }, > }; > > +static const struct freq_tbl ftbl_gcc_qspi_core_clk_src[] = { > + F(19200000, P_BI_TCXO, 1, 0, 0), > + F(50000000, P_GPLL0_OUT_EVEN, 6, 0, 0), > + F(75000000, P_GPLL0_OUT_EVEN, 4, 0, 0), > + F(100000000, P_GPLL0_OUT_EVEN, 3, 0, 0), Is SW planning to use this frequency? > + F(150000000, P_GPLL0_OUT_EVEN, 2, 0, 0), F(150000000, P_GPLL0_OUT_MAIN, 4, 0, 0), > + F(300000000, P_GPLL0_OUT_EVEN, 1, 0, 0), F(300000000, P_GPLL0_OUT_MAIN, 2, 0, 0), > + F(400000000, P_GPLL0_OUT_MAIN, 1.5, 0, 0), Please remove this, the Max supported frequency is 300MHz. > + { } > +}; > + > +static struct clk_rcg2 gcc_qspi_core_clk_src = { > + .cmd_rcgr = 0x4b008, > + .mnd_width = 0, > + .hid_width = 5, > + .parent_map = gcc_parent_map_9, > + .freq_tbl = ftbl_gcc_qspi_core_clk_src, > + .clkr.hw.init = &(struct clk_init_data){ > + .name = "gcc_qspi_core_clk_src", > + .parent_names = gcc_parent_names_9, > + .num_parents = 4, > + .ops = &clk_rcg2_floor_ops, Could we use the rcg2_ops instead? > + }, > +}; > + > static const struct freq_tbl ftbl_gcc_pdm2_clk_src[] = { > F(9600000, P_BI_TCXO, 2, 0, 0), > F(19200000, P_BI_TCXO, 1, 0, 0), > @@ -1935,6 +1974,37 @@ static struct clk_branch gcc_qmip_video_ahb_clk = { > }, > }; > > +static struct clk_branch gcc_qspi_cnoc_periph_ahb_clk = { > + .halt_reg = 0x4b000, > + .halt_check = BRANCH_HALT, > + .clkr = { > + .enable_reg = 0x4b000, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "gcc_qspi_cnoc_periph_ahb_clk", > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > + > +static struct clk_branch gcc_qspi_core_clk = { > + .halt_reg = 0x4b004, > + .halt_check = BRANCH_HALT, > + .clkr = { > + .enable_reg = 0x4b004, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "gcc_qspi_core_clk", > + .parent_names = (const char *[]){ > + "gcc_qspi_core_clk_src", > + }, > + .num_parents = 1, > + .flags = CLK_SET_RATE_PARENT, > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > + > static struct clk_branch gcc_qupv3_wrap0_s0_clk = { > .halt_reg = 0x17030, > .halt_check = BRANCH_HALT_VOTED, > @@ -3383,6 +3453,9 @@ static struct clk_regmap *gcc_sdm845_clocks[] = { > [GPLL4] = &gpll4.clkr, > [GCC_CPUSS_DVM_BUS_CLK] = &gcc_cpuss_dvm_bus_clk.clkr, > [GCC_CPUSS_GNOC_CLK] = &gcc_cpuss_gnoc_clk.clkr, > + [GCC_QSPI_CORE_CLK_SRC] = &gcc_qspi_core_clk_src.clkr, > + [GCC_QSPI_CORE_CLK] = &gcc_qspi_core_clk.clkr, > + [GCC_QSPI_CNOC_PERIPH_AHB_CLK] = &gcc_qspi_cnoc_periph_ahb_clk.clkr, > }; > > static const struct qcom_reset_map gcc_sdm845_resets[] = { > -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation. -- ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH 2/2] clk: qcom: Add qspi (Quad SPI) clocks for sdm845 2018-07-19 11:04 ` Taniya Das @ 2018-07-19 17:55 ` Doug Anderson 2018-07-19 19:02 ` Taniya Das 0 siblings, 1 reply; 8+ messages in thread From: Doug Anderson @ 2018-07-19 17:55 UTC (permalink / raw) To: Taniya Das Cc: Stephen Boyd, Andy Gross, Girish Mahadevan, Amit Nischal, Bjorn Andersson, Michael Turquette, linux-arm-msm, LKML, David Brown, open list:ARM/QUALCOMM SUPPORT, linux-clk Hi, On Thu, Jul 19, 2018 at 4:04 AM, Taniya Das <tdas@codeaurora.org> wrote: > Hi Doug, > > Please find my comments inline. > > On 7/18/2018 11:34 PM, Douglas Anderson wrote: >> >> Add both the interface and core clock. >> >> Signed-off-by: Douglas Anderson <dianders@chromium.org> >> --- >> >> drivers/clk/qcom/gcc-sdm845.c | 73 +++++++++++++++++++++++++++++++++++ >> 1 file changed, 73 insertions(+) >> >> diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c >> index 0f694ed4238a..2ee96f9bc217 100644 >> --- a/drivers/clk/qcom/gcc-sdm845.c >> +++ b/drivers/clk/qcom/gcc-sdm845.c >> @@ -162,6 +162,20 @@ static const char * const gcc_parent_names_10[] = { >> "core_bi_pll_test_se", >> }; >> +static const struct parent_map gcc_parent_map_9[] = { >> + { P_BI_TCXO, 0 }, >> + { P_GPLL0_OUT_MAIN, 1 }, >> + { P_GPLL0_OUT_EVEN, 6 }, >> + { P_SLEEP_CLK, 7 }, > > > SRC 7 has 'core_bi_pll_test_se' and not 'sleep_clk'. > > Please use the 'gcc_parent_map_0' Are you sure? I'm looking at a doc showing the bitfields of GCC_QSPI_CORE_CFG_RCGR. It says: 0x0: bi_tcxo. 0x1: gpll0_out_main. 0x6: gpll0_out_even. 0x7: sleep_clk. This contrasts with other clocks using 'gcc_parent_map_0' (for instance "gcc_qupv3_wrap0_s0_clk_src") where 0x7 is simply not listed in my doc. ...so either my doc is wrong or yours is. Any way to resolve that? >> +}; >> + >> +static const char * const gcc_parent_names_9[] = { >> + "bi_tcxo", >> + "gpll0", >> + "gpll0_out_even", >> + "core_pi_sleep_clk", >> +}; >> + >> static struct clk_alpha_pll gpll0 = { >> .offset = 0x0, >> .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA], >> @@ -358,6 +372,31 @@ static struct clk_rcg2 gcc_pcie_phy_refgen_clk_src = >> { >> }, >> }; >> +static const struct freq_tbl ftbl_gcc_qspi_core_clk_src[] = { >> + F(19200000, P_BI_TCXO, 1, 0, 0), >> + F(50000000, P_GPLL0_OUT_EVEN, 6, 0, 0), >> + F(75000000, P_GPLL0_OUT_EVEN, 4, 0, 0), >> + F(100000000, P_GPLL0_OUT_EVEN, 3, 0, 0), > > > Is SW planning to use this frequency? At the moment I actually am using it. I haven't done signal analysis, but in quick testing I couldn't properly read the SPI at faster than a 25 MHz SPI bus which translates to 100 MHz here. Interestingly, it seems like folks in your boot team came to the same conclusion since I see them setting this bus to 100 MHz at <https://review.coreboot.org/#/c/coreboot/+/25392/25/src/soc/qualcomm/sdm845/bootblock.c> too. >> + F(150000000, P_GPLL0_OUT_EVEN, 2, 0, 0), > > > F(150000000, P_GPLL0_OUT_MAIN, 4, 0, 0), Sure. For my edification, is there a reason to use main vs. even? >> + F(300000000, P_GPLL0_OUT_EVEN, 1, 0, 0), > > > F(300000000, P_GPLL0_OUT_MAIN, 2, 0, 0), No problem. >> + F(400000000, P_GPLL0_OUT_MAIN, 1.5, 0, 0), No problem. > Please remove this, the Max supported frequency is 300MHz. >> >> + { } >> +}; >> + >> +static struct clk_rcg2 gcc_qspi_core_clk_src = { >> + .cmd_rcgr = 0x4b008, >> + .mnd_width = 0, >> + .hid_width = 5, >> + .parent_map = gcc_parent_map_9, >> + .freq_tbl = ftbl_gcc_qspi_core_clk_src, >> + .clkr.hw.init = &(struct clk_init_data){ >> + .name = "gcc_qspi_core_clk_src", >> + .parent_names = gcc_parent_names_9, >> + .num_parents = 4, >> + .ops = &clk_rcg2_floor_ops, > > > Could we use the rcg2_ops instead? I'd rather not. Any reason why you think that'd be a good idea? Specifically imagine that we have a SPI flash chip that's rated to run at a max of 20 MHz. In the device tree we'd ideally want to specify: spi-max-frequency = <20000000>; It appears that we need to run the SPI core as 4 times the rate of the SPI bus, so we'd try to set this clock to 80 MHz. If we round up we'll end up at 100 MHz or 150 MHz for the SPI core and have a SPI bus rate of 25 MHz or 37.5 MHz. That would violate the whole idea of "spi-max-frequency". It's much better to round down to 75 MHz. In general I've always seen that for safety it's always better the round clocks down and round voltage up, so I was actually confused by the fact that most of the clocks in this file used rcg2_ops instead of clk_rcg2_floor_ops... I'd be curious if we should we change more of them to clk_rcg2_floor_ops. As a random example I'll take "gcc_sdcc2_apps_clk_src". If someone happened to have a full sized SD slot and put an MMC card in then you'd be in trouble. Why? For MMC a valid rate to request is 52000000. When the SD card core requests this you'll round up to 100 MHz. Oops. That makes the card not work. I just happen to have a micro to full size adapter at my desk and a 2GB MMC card and I can confirm that's a true bug that prevents this card from enumerating. Changing this to a clk_rcg2_floor_ops fixes it. True that it's unlikely anyone will really plug a MMC card into this slot, but I fail to see the advantage of rounding up when rounding down is safer. -Doug ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH 2/2] clk: qcom: Add qspi (Quad SPI) clocks for sdm845 2018-07-19 17:55 ` Doug Anderson @ 2018-07-19 19:02 ` Taniya Das 2018-07-19 19:37 ` Doug Anderson 0 siblings, 1 reply; 8+ messages in thread From: Taniya Das @ 2018-07-19 19:02 UTC (permalink / raw) To: Doug Anderson, Graham Roff Cc: Stephen Boyd, Andy Gross, Girish Mahadevan, Amit Nischal, Bjorn Andersson, Michael Turquette, linux-arm-msm, LKML, David Brown, open list:ARM/QUALCOMM SUPPORT, linux-clk On 7/19/2018 11:25 PM, Doug Anderson wrote: > Hi, > > On Thu, Jul 19, 2018 at 4:04 AM, Taniya Das <tdas@codeaurora.org> wrote: >> Hi Doug, >> >> Please find my comments inline. >> >> On 7/18/2018 11:34 PM, Douglas Anderson wrote: >>> >>> Add both the interface and core clock. >>> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >>> --- >>> >>> drivers/clk/qcom/gcc-sdm845.c | 73 +++++++++++++++++++++++++++++++++++ >>> 1 file changed, 73 insertions(+) >>> >>> diff --git a/drivers/clk/qcom/gcc-sdm845.c b/drivers/clk/qcom/gcc-sdm845.c >>> index 0f694ed4238a..2ee96f9bc217 100644 >>> --- a/drivers/clk/qcom/gcc-sdm845.c >>> +++ b/drivers/clk/qcom/gcc-sdm845.c >>> @@ -162,6 +162,20 @@ static const char * const gcc_parent_names_10[] = { >>> "core_bi_pll_test_se", >>> }; >>> +static const struct parent_map gcc_parent_map_9[] = { >>> + { P_BI_TCXO, 0 }, >>> + { P_GPLL0_OUT_MAIN, 1 }, >>> + { P_GPLL0_OUT_EVEN, 6 }, >>> + { P_SLEEP_CLK, 7 }, >> >> >> SRC 7 has 'core_bi_pll_test_se' and not 'sleep_clk'. >> >> Please use the 'gcc_parent_map_0' > > Are you sure? I'm looking at a doc showing the bitfields of > GCC_QSPI_CORE_CFG_RCGR. It says: > > 0x0: bi_tcxo. > 0x1: gpll0_out_main. > 0x6: gpll0_out_even. > 0x7: sleep_clk. > > This contrasts with other clocks using 'gcc_parent_map_0' (for > instance "gcc_qupv3_wrap0_s0_clk_src") where 0x7 is simply not listed > in my doc. > > ...so either my doc is wrong or yours is. Any way to resolve that? > I am not sure of the document you are referring, but the connectivity details I have shared are from the design side and they are ones which we have to follow. > >>> +}; >>> + >>> +static const char * const gcc_parent_names_9[] = { >>> + "bi_tcxo", >>> + "gpll0", >>> + "gpll0_out_even", >>> + "core_pi_sleep_clk", >>> +}; >>> + >>> static struct clk_alpha_pll gpll0 = { >>> .offset = 0x0, >>> .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA], >>> @@ -358,6 +372,31 @@ static struct clk_rcg2 gcc_pcie_phy_refgen_clk_src = >>> { >>> }, >>> }; >>> +static const struct freq_tbl ftbl_gcc_qspi_core_clk_src[] = { >>> + F(19200000, P_BI_TCXO, 1, 0, 0), >>> + F(50000000, P_GPLL0_OUT_EVEN, 6, 0, 0), >>> + F(75000000, P_GPLL0_OUT_EVEN, 4, 0, 0), >>> + F(100000000, P_GPLL0_OUT_EVEN, 3, 0, 0), >> >> >> Is SW planning to use this frequency? > > At the moment I actually am using it. I haven't done signal analysis, > but in quick testing I couldn't properly read the SPI at faster than a > 25 MHz SPI bus which translates to 100 MHz here. Interestingly, it > seems like folks in your boot team came to the same conclusion since I > see them setting this bus to 100 MHz at > <https://review.coreboot.org/#/c/coreboot/+/25392/25/src/soc/qualcomm/sdm845/bootblock.c> > too. > > >>> + F(150000000, P_GPLL0_OUT_EVEN, 2, 0, 0), >> >> >> F(150000000, P_GPLL0_OUT_MAIN, 4, 0, 0), > > Sure. For my edification, is there a reason to use main vs. even? > The frequencies to be generated are also as per design data. These source usage depends on timing closure for certain max frequencies. > >>> + F(300000000, P_GPLL0_OUT_EVEN, 1, 0, 0), >> >> >> F(300000000, P_GPLL0_OUT_MAIN, 2, 0, 0), > > No problem. > > >>> + F(400000000, P_GPLL0_OUT_MAIN, 1.5, 0, 0), > > No problem. > > >> Please remove this, the Max supported frequency is 300MHz. >>> >>> + { } >>> +}; >>> + >>> +static struct clk_rcg2 gcc_qspi_core_clk_src = { >>> + .cmd_rcgr = 0x4b008, >>> + .mnd_width = 0, >>> + .hid_width = 5, >>> + .parent_map = gcc_parent_map_9, >>> + .freq_tbl = ftbl_gcc_qspi_core_clk_src, >>> + .clkr.hw.init = &(struct clk_init_data){ >>> + .name = "gcc_qspi_core_clk_src", >>> + .parent_names = gcc_parent_names_9, >>> + .num_parents = 4, >>> + .ops = &clk_rcg2_floor_ops, >> >> >> Could we use the rcg2_ops instead? > > I'd rather not. Any reason why you think that'd be a good idea? > > Specifically imagine that we have a SPI flash chip that's rated to run > at a max of 20 MHz. In the device tree we'd ideally want to specify: > > spi-max-frequency = <20000000>; > > It appears that we need to run the SPI core as 4 times the rate of the > SPI bus, so we'd try to set this clock to 80 MHz. If we round up > we'll end up at 100 MHz or 150 MHz for the SPI core and have a SPI bus > rate of 25 MHz or 37.5 MHz. That would violate the whole idea of > "spi-max-frequency". It's much better to round down to 75 MHz. > > > In general I've always seen that for safety it's always better the > round clocks down and round voltage up, so I was actually confused by > the fact that most of the clocks in this file used rcg2_ops instead of > clk_rcg2_floor_ops... I'd be curious if we should we change more of > them to clk_rcg2_floor_ops. As a random example I'll take > "gcc_sdcc2_apps_clk_src". If someone happened to have a full sized SD > slot and put an MMC card in then you'd be in trouble. Why? > > For MMC a valid rate to request is 52000000. When the SD card core > requests this you'll round up to 100 MHz. Oops. That makes the card > not work. > > I just happen to have a micro to full size adapter at my desk and a > 2GB MMC card and I can confirm that's a true bug that prevents this > card from enumerating. Changing this to a clk_rcg2_floor_ops fixes > it. True that it's unlikely anyone will really plug a MMC card into > this slot, but I fail to see the advantage of rounding up when > rounding down is safer. > > These ops were mostly for SDCC/MMC usage only as this was a requirement of rounding the frequency, as the clock framework didn't provide any such API. The QSPI should be safe to use the normal rcg ops as the frequency requests should be from the table itself. Thus request is to move to use the rcg2_ops. > -Doug > -- > To unsubscribe from this list: send the line "unsubscribe linux-clk" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation. -- ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH 2/2] clk: qcom: Add qspi (Quad SPI) clocks for sdm845 2018-07-19 19:02 ` Taniya Das @ 2018-07-19 19:37 ` Doug Anderson 0 siblings, 0 replies; 8+ messages in thread From: Doug Anderson @ 2018-07-19 19:37 UTC (permalink / raw) To: Taniya Das Cc: Graham Roff, Stephen Boyd, Andy Gross, Girish Mahadevan, Amit Nischal, Bjorn Andersson, Michael Turquette, linux-arm-msm, LKML, David Brown, open list:ARM/QUALCOMM SUPPORT, linux-clk Hi, On Thu, Jul 19, 2018 at 12:02 PM, Taniya Das <tdas@codeaurora.org> wrote: > > > On 7/19/2018 11:25 PM, Doug Anderson wrote: >> >> Hi, >> >> On Thu, Jul 19, 2018 at 4:04 AM, Taniya Das <tdas@codeaurora.org> wrote: >>> >>> Hi Doug, >>> >>> Please find my comments inline. >>> >>> On 7/18/2018 11:34 PM, Douglas Anderson wrote: >>>> >>>> >>>> Add both the interface and core clock. >>>> >>>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >>>> --- >>>> >>>> drivers/clk/qcom/gcc-sdm845.c | 73 >>>> +++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 73 insertions(+) >>>> >>>> diff --git a/drivers/clk/qcom/gcc-sdm845.c >>>> b/drivers/clk/qcom/gcc-sdm845.c >>>> index 0f694ed4238a..2ee96f9bc217 100644 >>>> --- a/drivers/clk/qcom/gcc-sdm845.c >>>> +++ b/drivers/clk/qcom/gcc-sdm845.c >>>> @@ -162,6 +162,20 @@ static const char * const gcc_parent_names_10[] = { >>>> "core_bi_pll_test_se", >>>> }; >>>> +static const struct parent_map gcc_parent_map_9[] = { >>>> + { P_BI_TCXO, 0 }, >>>> + { P_GPLL0_OUT_MAIN, 1 }, >>>> + { P_GPLL0_OUT_EVEN, 6 }, >>>> + { P_SLEEP_CLK, 7 }, >>> >>> >>> >>> SRC 7 has 'core_bi_pll_test_se' and not 'sleep_clk'. >>> >>> Please use the 'gcc_parent_map_0' >> >> >> Are you sure? I'm looking at a doc showing the bitfields of >> GCC_QSPI_CORE_CFG_RCGR. It says: >> >> 0x0: bi_tcxo. >> 0x1: gpll0_out_main. >> 0x6: gpll0_out_even. >> 0x7: sleep_clk. >> >> This contrasts with other clocks using 'gcc_parent_map_0' (for >> instance "gcc_qupv3_wrap0_s0_clk_src") where 0x7 is simply not listed >> in my doc. >> >> ...so either my doc is wrong or yours is. Any way to resolve that? >> > > I am not sure of the document you are referring, but the connectivity > details I have shared are from the design side and they are ones which we > have to follow. I'll start up a separate thread about this w/ you and the people I received the doc from. It's concerning if the doc we received is wrong. >>> F(150000000, P_GPLL0_OUT_MAIN, 4, 0, 0), >> >> >> Sure. For my edification, is there a reason to use main vs. even? >> > > The frequencies to be generated are also as per design data. These source > usage depends on timing closure for certain max frequencies. Sounds a bit like a non-answer, but I guess it's not critical to understand the mysteries here. >>>> +static struct clk_rcg2 gcc_qspi_core_clk_src = { >>>> + .cmd_rcgr = 0x4b008, >>>> + .mnd_width = 0, >>>> + .hid_width = 5, >>>> + .parent_map = gcc_parent_map_9, >>>> + .freq_tbl = ftbl_gcc_qspi_core_clk_src, >>>> + .clkr.hw.init = &(struct clk_init_data){ >>>> + .name = "gcc_qspi_core_clk_src", >>>> + .parent_names = gcc_parent_names_9, >>>> + .num_parents = 4, >>>> + .ops = &clk_rcg2_floor_ops, >>> >>> >>> >>> Could we use the rcg2_ops instead? >> >> >> I'd rather not. Any reason why you think that'd be a good idea? >> >> Specifically imagine that we have a SPI flash chip that's rated to run >> at a max of 20 MHz. In the device tree we'd ideally want to specify: >> >> spi-max-frequency = <20000000>; >> >> It appears that we need to run the SPI core as 4 times the rate of the >> SPI bus, so we'd try to set this clock to 80 MHz. If we round up >> we'll end up at 100 MHz or 150 MHz for the SPI core and have a SPI bus >> rate of 25 MHz or 37.5 MHz. That would violate the whole idea of >> "spi-max-frequency". It's much better to round down to 75 MHz. >> >> >> In general I've always seen that for safety it's always better the >> round clocks down and round voltage up, so I was actually confused by >> the fact that most of the clocks in this file used rcg2_ops instead of >> clk_rcg2_floor_ops... I'd be curious if we should we change more of >> them to clk_rcg2_floor_ops. As a random example I'll take >> "gcc_sdcc2_apps_clk_src". If someone happened to have a full sized SD >> slot and put an MMC card in then you'd be in trouble. Why? >> >> For MMC a valid rate to request is 52000000. When the SD card core >> requests this you'll round up to 100 MHz. Oops. That makes the card >> not work. >> >> I just happen to have a micro to full size adapter at my desk and a >> 2GB MMC card and I can confirm that's a true bug that prevents this >> card from enumerating. Changing this to a clk_rcg2_floor_ops fixes >> it. True that it's unlikely anyone will really plug a MMC card into >> this slot, but I fail to see the advantage of rounding up when >> rounding down is safer. >> >> > > These ops were mostly for SDCC/MMC usage only as this was a requirement of > rounding the frequency, as the clock framework didn't provide any such API. > The QSPI should be safe to use the normal rcg ops as the frequency requests > should be from the table itself. Thus request is to move to use the > rcg2_ops. I'm not totally sure I understand what you say here. I'll try to break my confusion down: 1. I think you're saying that "clk_rcg2_floor_ops" was introduced originally for use by SD/MMC. Is that right? ...but my example above shows specifically that SD/MMC is _not_ using "clk_rcg2_floor_ops". 2. I'm confused about you say that the QSPI driver will only be requesting rates from the table itself. Does that mean we need to duplicate the table here in the QSPI driver so the QSPI driver will only ask for exact rates that it knows the clock driver will be able to provide? ...or are you saying that we should always specify SPI transfer rates in the device tree that we know will be achievable by the clock driver? Both of these ideas seem fragile. I'm still confused about what benefit you think we get for rounding up for this clock. -Doug ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-07-19 19:37 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-18 18:04 [RFC PATCH 0/2] clk: qcom: Quad SPI (qspi) clock support for sdm845 Douglas Anderson 2018-07-18 18:04 ` [RFC PATCH 1/2] clk: qcom: Add qspi (Quad SPI) clock defines for sdm845 to header Douglas Anderson 2018-07-18 18:04 ` [RFC PATCH 2/2] clk: qcom: Add qspi (Quad SPI) clocks for sdm845 Douglas Anderson 2018-07-19 0:19 ` grahamr 2018-07-19 11:04 ` Taniya Das 2018-07-19 17:55 ` Doug Anderson 2018-07-19 19:02 ` Taniya Das 2018-07-19 19:37 ` Doug Anderson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).