* [PATCH v3 14/15] drivers: clk: qcom: gcc-ipq806x: add CryptoEngine resets
From: Ansuel Smith @ 2022-01-21 21:03 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Michael Turquette, Stephen Boyd,
Rob Herring, Philipp Zabel, Taniya Das, Ansuel Smith,
linux-arm-msm, linux-clk, devicetree, linux-kernel
In-Reply-To: <20220121210340.32362-1-ansuelsmth@gmail.com>
Add missing CryptoEngine resets.
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
drivers/clk/qcom/gcc-ipq806x.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/clk/qcom/gcc-ipq806x.c b/drivers/clk/qcom/gcc-ipq806x.c
index a86d1504a149..8f025cef2ec3 100644
--- a/drivers/clk/qcom/gcc-ipq806x.c
+++ b/drivers/clk/qcom/gcc-ipq806x.c
@@ -3314,6 +3314,11 @@ static const struct qcom_reset_map gcc_ipq806x_resets[] = {
[GMAC_CORE3_RESET] = { 0x3cfc, 0 },
[GMAC_CORE4_RESET] = { 0x3d1c, 0 },
[GMAC_AHB_RESET] = { 0x3e24, 0 },
+ [CRYPTO_ENG1_RESET] = { 0x3e00, 0},
+ [CRYPTO_ENG2_RESET] = { 0x3e04, 0},
+ [CRYPTO_ENG3_RESET] = { 0x3e08, 0},
+ [CRYPTO_ENG4_RESET] = { 0x3e0c, 0},
+ [CRYPTO_AHB_RESET] = { 0x3e10, 0},
[NSS_CH0_RST_RX_CLK_N_RESET] = { 0x3b60, 0 },
[NSS_CH0_RST_TX_CLK_N_RESET] = { 0x3b60, 1 },
[NSS_CH0_RST_RX_125M_N_RESET] = { 0x3b60, 2 },
--
2.33.1
^ permalink raw reply related
* [PATCH v3 13/15] dt-bindings: reset: add ipq8064 ce5 resets
From: Ansuel Smith @ 2022-01-21 21:03 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Michael Turquette, Stephen Boyd,
Rob Herring, Philipp Zabel, Taniya Das, Ansuel Smith,
linux-arm-msm, linux-clk, devicetree, linux-kernel
In-Reply-To: <20220121210340.32362-1-ansuelsmth@gmail.com>
Add ipq8064 ce5 resets needed for CryptoEngine gcc driver.
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
include/dt-bindings/reset/qcom,gcc-ipq806x.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/dt-bindings/reset/qcom,gcc-ipq806x.h b/include/dt-bindings/reset/qcom,gcc-ipq806x.h
index 26b6f9200620..020c9cf18751 100644
--- a/include/dt-bindings/reset/qcom,gcc-ipq806x.h
+++ b/include/dt-bindings/reset/qcom,gcc-ipq806x.h
@@ -163,5 +163,10 @@
#define NSS_CAL_PRBS_RST_N_RESET 154
#define NSS_LCKDT_RST_N_RESET 155
#define NSS_SRDS_N_RESET 156
+#define CRYPTO_ENG1_RESET 157
+#define CRYPTO_ENG2_RESET 158
+#define CRYPTO_ENG3_RESET 159
+#define CRYPTO_ENG4_RESET 160
+#define CRYPTO_AHB_RESET 161
#endif
--
2.33.1
^ permalink raw reply related
* [PATCH v3 09/15] drivers: clk: qcom: gcc-ipq806x: add unusued flag for critical clock
From: Ansuel Smith @ 2022-01-21 21:03 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Michael Turquette, Stephen Boyd,
Rob Herring, Philipp Zabel, Taniya Das, Ansuel Smith,
linux-arm-msm, linux-clk, devicetree, linux-kernel
In-Reply-To: <20220121210340.32362-1-ansuelsmth@gmail.com>
Some clocks are used by other devices present on the SoC. For example
the gsbi4_h_clk is used by RPM and is if disabled cause the RPM to
reject any regulator change command. These clock should never be
disabled.
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
drivers/clk/qcom/gcc-ipq806x.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/qcom/gcc-ipq806x.c b/drivers/clk/qcom/gcc-ipq806x.c
index 53a61860063d..77bc3d94f580 100644
--- a/drivers/clk/qcom/gcc-ipq806x.c
+++ b/drivers/clk/qcom/gcc-ipq806x.c
@@ -798,7 +798,7 @@ static struct clk_rcg gsbi4_qup_src = {
.parent_data = gcc_pxo_pll8,
.num_parents = ARRAY_SIZE(gcc_pxo_pll8),
.ops = &clk_rcg_ops,
- .flags = CLK_SET_PARENT_GATE,
+ .flags = CLK_SET_PARENT_GATE | CLK_IGNORE_UNUSED,
},
},
};
@@ -816,7 +816,7 @@ static struct clk_branch gsbi4_qup_clk = {
},
.num_parents = 1,
.ops = &clk_branch_ops,
- .flags = CLK_SET_RATE_PARENT,
+ .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
},
},
};
@@ -900,7 +900,7 @@ static struct clk_rcg gsbi6_qup_src = {
.parent_data = gcc_pxo_pll8,
.num_parents = ARRAY_SIZE(gcc_pxo_pll8),
.ops = &clk_rcg_ops,
- .flags = CLK_SET_PARENT_GATE,
+ .flags = CLK_SET_PARENT_GATE | CLK_IGNORE_UNUSED,
},
},
};
@@ -969,7 +969,7 @@ static struct clk_branch gsbi7_qup_clk = {
},
.num_parents = 1,
.ops = &clk_branch_ops,
- .flags = CLK_SET_RATE_PARENT,
+ .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
},
},
};
@@ -1015,6 +1015,7 @@ static struct clk_branch gsbi4_h_clk = {
.hw.init = &(struct clk_init_data){
.name = "gsbi4_h_clk",
.ops = &clk_branch_ops,
+ .flags = CLK_IGNORE_UNUSED,
},
},
};
--
2.33.1
^ permalink raw reply related
* [PATCH v3 12/15] drivers: clk: qcom: gcc-ipq806x: add CryptoEngine clocks
From: Ansuel Smith @ 2022-01-21 21:03 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Michael Turquette, Stephen Boyd,
Rob Herring, Philipp Zabel, Taniya Das, Ansuel Smith,
linux-arm-msm, linux-clk, devicetree, linux-kernel
In-Reply-To: <20220121210340.32362-1-ansuelsmth@gmail.com>
Add missing CryptoEngine clocks and pll11 required clock.
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
drivers/clk/qcom/gcc-ipq806x.c | 244 +++++++++++++++++++++++++++++++++
1 file changed, 244 insertions(+)
diff --git a/drivers/clk/qcom/gcc-ipq806x.c b/drivers/clk/qcom/gcc-ipq806x.c
index dbd61e4844b0..a86d1504a149 100644
--- a/drivers/clk/qcom/gcc-ipq806x.c
+++ b/drivers/clk/qcom/gcc-ipq806x.c
@@ -256,6 +256,24 @@ static struct clk_pll pll18 = {
},
};
+static struct clk_pll pll11 = {
+ .l_reg = 0x3184,
+ .m_reg = 0x3188,
+ .n_reg = 0x318c,
+ .config_reg = 0x3194,
+ .mode_reg = 0x3180,
+ .status_reg = 0x3198,
+ .status_bit = 16,
+ .clkr.hw.init = &(struct clk_init_data){
+ .name = "pll11",
+ .parent_data = &(const struct clk_parent_data){
+ .fw_name = "pxo",
+ },
+ .num_parents = 1,
+ .ops = &clk_pll_ops,
+ },
+};
+
enum {
P_PXO,
P_PLL8,
@@ -264,6 +282,7 @@ enum {
P_CXO,
P_PLL14,
P_PLL18,
+ P_PLL11,
};
static const struct parent_map gcc_pxo_pll8_map[] = {
@@ -331,6 +350,44 @@ static const struct clk_parent_data gcc_pxo_pll8_pll14_pll18_pll0[] = {
{ .hw = &pll18.clkr.hw },
};
+static const struct parent_map gcc_pxo_pll8_pll0_pll14_pll18_pll11_map[] = {
+ { P_PXO, 0 },
+ { P_PLL8, 4 },
+ { P_PLL0, 2 },
+ { P_PLL14, 5 },
+ { P_PLL18, 1 },
+ { P_PLL11, 3 },
+};
+
+static const struct clk_parent_data gcc_pxo_pll8_pll0_pll14_pll18_pll11[] = {
+ { .fw_name = "pxo" },
+ { .hw = &pll8_vote.hw },
+ { .hw = &pll0_vote.hw },
+ { .hw = &pll14.clkr.hw },
+ { .hw = &pll18.clkr.hw },
+ { .hw = &pll11.clkr.hw },
+
+};
+
+static const struct parent_map gcc_pxo_pll3_pll0_pll14_pll18_pll11_map[] = {
+ { P_PXO, 0 },
+ { P_PLL3, 6 },
+ { P_PLL0, 2 },
+ { P_PLL14, 5 },
+ { P_PLL18, 1 },
+ { P_PLL11, 3 },
+};
+
+static const struct clk_parent_data gcc_pxo_pll3_pll0_pll14_pll18_pll11[] = {
+ { .fw_name = "pxo" },
+ { .hw = &pll3.clkr.hw },
+ { .hw = &pll0_vote.hw },
+ { .hw = &pll14.clkr.hw },
+ { .hw = &pll18.clkr.hw },
+ { .hw = &pll11.clkr.hw },
+
+};
+
static struct freq_tbl clk_tbl_gsbi_uart[] = {
{ 1843200, P_PLL8, 2, 6, 625 },
{ 3686400, P_PLL8, 2, 12, 625 },
@@ -2818,6 +2875,186 @@ static struct clk_dyn_rcg ubi32_core2_src_clk = {
},
};
+static const struct freq_tbl clk_tbl_ce5_core[] = {
+ { 150000000, P_PLL3, 8, 1, 1 },
+ { 213200000, P_PLL11, 5, 1, 1 },
+ { }
+};
+
+static struct clk_dyn_rcg ce5_core_src = {
+ .ns_reg[0] = 0x36C4,
+ .ns_reg[1] = 0x36C8,
+ .bank_reg = 0x36C0,
+ .s[0] = {
+ .src_sel_shift = 0,
+ .parent_map = gcc_pxo_pll3_pll0_pll14_pll18_pll11_map,
+ },
+ .s[1] = {
+ .src_sel_shift = 0,
+ .parent_map = gcc_pxo_pll3_pll0_pll14_pll18_pll11_map,
+ },
+ .p[0] = {
+ .pre_div_shift = 3,
+ .pre_div_width = 4,
+ },
+ .p[1] = {
+ .pre_div_shift = 3,
+ .pre_div_width = 4,
+ },
+ .mux_sel_bit = 0,
+ .freq_tbl = clk_tbl_ce5_core,
+ .clkr = {
+ .enable_reg = 0x36C0,
+ .enable_mask = BIT(1),
+ .hw.init = &(struct clk_init_data){
+ .name = "ce5_core_src",
+ .parent_data = gcc_pxo_pll3_pll0_pll14_pll18_pll11,
+ .num_parents = ARRAY_SIZE(gcc_pxo_pll3_pll0_pll14_pll18_pll11),
+ .ops = &clk_dyn_rcg_ops,
+ },
+ },
+};
+
+static struct clk_branch ce5_core_clk = {
+ .halt_reg = 0x2FDC,
+ .halt_bit = 5,
+ .hwcg_reg = 0x36CC,
+ .hwcg_bit = 6,
+ .clkr = {
+ .enable_reg = 0x36CC,
+ .enable_mask = BIT(4),
+ .hw.init = &(struct clk_init_data){
+ .name = "ce5_core_clk",
+ .parent_hws = (const struct clk_hw*[]){
+ &ce5_core_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .ops = &clk_branch_ops,
+ .flags = CLK_SET_RATE_PARENT,
+ },
+ },
+};
+
+static const struct freq_tbl clk_tbl_ce5_a_clk[] = {
+ { 160000000, P_PLL0, 5, 1, 1 },
+ { 213200000, P_PLL11, 5, 1, 1 },
+ { }
+};
+
+static struct clk_dyn_rcg ce5_a_clk_src = {
+ .ns_reg[0] = 0x3d84,
+ .ns_reg[1] = 0x3d88,
+ .bank_reg = 0x3d80,
+ .s[0] = {
+ .src_sel_shift = 0,
+ .parent_map = gcc_pxo_pll8_pll0_pll14_pll18_pll11_map,
+ },
+ .s[1] = {
+ .src_sel_shift = 0,
+ .parent_map = gcc_pxo_pll8_pll0_pll14_pll18_pll11_map,
+ },
+ .p[0] = {
+ .pre_div_shift = 3,
+ .pre_div_width = 4,
+ },
+ .p[1] = {
+ .pre_div_shift = 3,
+ .pre_div_width = 4,
+ },
+ .mux_sel_bit = 0,
+ .freq_tbl = clk_tbl_ce5_a_clk,
+ .clkr = {
+ .enable_reg = 0x3d80,
+ .enable_mask = BIT(1),
+ .hw.init = &(struct clk_init_data){
+ .name = "ce5_a_clk_src",
+ .parent_data = gcc_pxo_pll8_pll0_pll14_pll18_pll11,
+ .num_parents = ARRAY_SIZE(gcc_pxo_pll8_pll0_pll14_pll18_pll11),
+ .ops = &clk_dyn_rcg_ops,
+ },
+ },
+};
+
+static struct clk_branch ce5_a_clk = {
+ .halt_reg = 0x3c20,
+ .halt_bit = 12,
+ .hwcg_reg = 0x3d8c,
+ .hwcg_bit = 6,
+ .clkr = {
+ .enable_reg = 0x3d8c,
+ .enable_mask = BIT(4),
+ .hw.init = &(struct clk_init_data){
+ .name = "ce5_a_clk",
+ .parent_hws = (const struct clk_hw*[]){
+ &ce5_a_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .ops = &clk_branch_ops,
+ .flags = CLK_SET_RATE_PARENT,
+ },
+ },
+};
+
+static const struct freq_tbl clk_tbl_ce5_h_clk[] = {
+ { 160000000, P_PLL0, 5, 1, 1 },
+ { 213200000, P_PLL11, 5, 1, 1 },
+ { }
+};
+
+static struct clk_dyn_rcg ce5_h_clk_src = {
+ .ns_reg[0] = 0x3c64,
+ .ns_reg[1] = 0x3c68,
+ .bank_reg = 0x3c60,
+ .s[0] = {
+ .src_sel_shift = 0,
+ .parent_map = gcc_pxo_pll8_pll0_pll14_pll18_pll11_map,
+ },
+ .s[1] = {
+ .src_sel_shift = 0,
+ .parent_map = gcc_pxo_pll8_pll0_pll14_pll18_pll11_map,
+ },
+ .p[0] = {
+ .pre_div_shift = 3,
+ .pre_div_width = 4,
+ },
+ .p[1] = {
+ .pre_div_shift = 3,
+ .pre_div_width = 4,
+ },
+ .mux_sel_bit = 0,
+ .freq_tbl = clk_tbl_ce5_h_clk,
+ .clkr = {
+ .enable_reg = 0x3c60,
+ .enable_mask = BIT(1),
+ .hw.init = &(struct clk_init_data){
+ .name = "ce5_h_clk_src",
+ .parent_data = gcc_pxo_pll8_pll0_pll14_pll18_pll11,
+ .num_parents = ARRAY_SIZE(gcc_pxo_pll8_pll0_pll14_pll18_pll11),
+ .ops = &clk_dyn_rcg_ops,
+ },
+ },
+};
+
+static struct clk_branch ce5_h_clk = {
+ .halt_reg = 0x3c20,
+ .halt_bit = 11,
+ .hwcg_reg = 0x3c6c,
+ .hwcg_bit = 6,
+ .clkr = {
+ .enable_reg = 0x3c6c,
+ .enable_mask = BIT(4),
+ .hw.init = &(struct clk_init_data){
+ .name = "ce5_h_clk",
+ .parent_hws = (const struct clk_hw*[]){
+ &ce5_h_clk_src.clkr.hw,
+ },
+ .num_parents = 1,
+ .ops = &clk_branch_ops,
+ .flags = CLK_SET_RATE_PARENT,
+ },
+ },
+};
+
static struct clk_regmap *gcc_ipq806x_clks[] = {
[PLL0] = &pll0.clkr,
[PLL0_VOTE] = &pll0_vote,
@@ -2825,6 +3062,7 @@ static struct clk_regmap *gcc_ipq806x_clks[] = {
[PLL4_VOTE] = &pll4_vote,
[PLL8] = &pll8.clkr,
[PLL8_VOTE] = &pll8_vote,
+ [PLL11] = &pll11.clkr,
[PLL14] = &pll14.clkr,
[PLL14_VOTE] = &pll14_vote,
[PLL18] = &pll18.clkr,
@@ -2939,6 +3177,12 @@ static struct clk_regmap *gcc_ipq806x_clks[] = {
[PLL9] = &hfpll0.clkr,
[PLL10] = &hfpll1.clkr,
[PLL12] = &hfpll_l2.clkr,
+ [CE5_A_CLK_SRC] = &ce5_a_clk_src.clkr,
+ [CE5_A_CLK] = &ce5_a_clk.clkr,
+ [CE5_H_CLK_SRC] = &ce5_h_clk_src.clkr,
+ [CE5_H_CLK] = &ce5_h_clk.clkr,
+ [CE5_CORE_CLK_SRC] = &ce5_core_src.clkr,
+ [CE5_CORE_CLK] = &ce5_core_clk.clkr,
};
static const struct qcom_reset_map gcc_ipq806x_resets[] = {
--
2.33.1
^ permalink raw reply related
* Re: [PATCH v3 04/16] clk: samsung: fsd: Add initial clock support
From: kernel test robot @ 2022-01-21 21:13 UTC (permalink / raw)
To: Alim Akhtar, linux-arm-kernel, linux-kernel
Cc: kbuild-all, soc, linux-clk, devicetree, olof, arnd, linus.walleij,
catalin.marinas, robh+dt
In-Reply-To: <20220121172840.12121-5-alim.akhtar@samsung.com>
Hi Alim,
I love your patch! Perhaps something to improve:
[auto build test WARNING on next-20220121]
[also build test WARNING on v5.16]
[cannot apply to clk/clk-next robh/for-next pinctrl-samsung/for-next v5.16 v5.16-rc8 v5.16-rc7]
[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]
url: https://github.com/0day-ci/linux/commits/Alim-Akhtar/dt-bindings-add-vendor-prefix-for-Tesla/20220122-022924
base: c94951012a748a0f8ed77cd8fc25640c6fe198f9
config: mips-allmodconfig (https://download.01.org/0day-ci/archive/20220122/202201220550.FSQ6N02X-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 11.2.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/0day-ci/linux/commit/69b6b21ebabb149c1c07d83376e9c08a582c6423
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Alim-Akhtar/dt-bindings-add-vendor-prefix-for-Tesla/20220122-022924
git checkout 69b6b21ebabb149c1c07d83376e9c08a582c6423
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=mips SHELL=/bin/bash drivers/clk/samsung/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/clk/samsung/clk-fsd.c:150:9: warning: this decimal constant is unsigned only in ISO C90
150 | PLL_35XX_RATE(24 * MHZ, 2400000000, 200, 2, 0),
| ^~~~~~~~~~~~~
In file included from include/linux/bits.h:22,
from include/linux/bitops.h:6,
from include/linux/of.h:15,
from include/linux/clk-provider.h:9,
from drivers/clk/samsung/clk-fsd.c:11:
>> include/linux/build_bug.h:16:51: warning: this decimal constant is unsigned only in ISO C90
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
drivers/clk/samsung/clk-pll.h:48:9: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
48 | BUILD_BUG_ON_ZERO(PLL_RATE(_fin, _m, _p, _s, _k, _ks) != (_fout)))
| ^~~~~~~~~~~~~~~~~
drivers/clk/samsung/clk-pll.h:52:33: note: in expansion of macro 'PLL_VALID_RATE'
52 | .rate = PLL_VALID_RATE(_fin, _rate, \
| ^~~~~~~~~~~~~~
drivers/clk/samsung/clk-fsd.c:150:9: note: in expansion of macro 'PLL_35XX_RATE'
150 | PLL_35XX_RATE(24 * MHZ, 2400000000, 200, 2, 0),
| ^~~~~~~~~~~~~
drivers/clk/samsung/clk-fsd.c:154:9: warning: this decimal constant is unsigned only in ISO C90
154 | PLL_35XX_RATE(24 * MHZ, 2400000000, 200, 2, 0),
| ^~~~~~~~~~~~~
In file included from include/linux/bits.h:22,
from include/linux/bitops.h:6,
from include/linux/of.h:15,
from include/linux/clk-provider.h:9,
from drivers/clk/samsung/clk-fsd.c:11:
>> include/linux/build_bug.h:16:51: warning: this decimal constant is unsigned only in ISO C90
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
| ^
drivers/clk/samsung/clk-pll.h:48:9: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
48 | BUILD_BUG_ON_ZERO(PLL_RATE(_fin, _m, _p, _s, _k, _ks) != (_fout)))
| ^~~~~~~~~~~~~~~~~
drivers/clk/samsung/clk-pll.h:52:33: note: in expansion of macro 'PLL_VALID_RATE'
52 | .rate = PLL_VALID_RATE(_fin, _rate, \
| ^~~~~~~~~~~~~~
drivers/clk/samsung/clk-fsd.c:154:9: note: in expansion of macro 'PLL_35XX_RATE'
154 | PLL_35XX_RATE(24 * MHZ, 2400000000, 200, 2, 0),
| ^~~~~~~~~~~~~
vim +150 drivers/clk/samsung/clk-fsd.c
148
149 static const struct samsung_pll_rate_table pll_shared1_rate_table[] __initconst = {
> 150 PLL_35XX_RATE(24 * MHZ, 2400000000, 200, 2, 0),
151 };
152
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply
* Re: [PATCH v11 1/3] dt-binding: mt8183: add Mediatek MDP3 dt-bindings
From: Rob Herring @ 2022-01-21 21:16 UTC (permalink / raw)
To: Moudy Ho
Cc: Mauro Carvalho Chehab, Matthias Brugger, Hans Verkuil,
Jernej Skrabec, Chun-Kuang Hu, Geert Uytterhoeven, Rob Landley,
Laurent Pinchart, linux-media, devicetree, linux-arm-kernel,
linux-mediatek, linux-kernel, Alexandre Courbot, tfiga, drinkcat,
pihsun, hsinyi, AngeloGioacchino Del Regno, Maoguang Meng,
daoyuan huang, Ping-Hsun Wu, menghui.lin, sj.huang,
allen-kh.cheng, randy.wu, jason-jh.lin, roy-cw.yeh, river.cheng,
srv_heupstream
In-Reply-To: <20220105093758.6850-2-moudy.ho@mediatek.com>
On Wed, Jan 05, 2022 at 05:37:56PM +0800, Moudy Ho wrote:
> This patch adds DT binding document for Media Data Path 3 (MDP3)
> a unit in multimedia system used for scaling and color format convert.
>
> Signed-off-by: Moudy Ho <moudy.ho@mediatek.com>
> ---
> .../bindings/media/mediatek,mdp3-rdma.yaml | 193 ++++++++++++++++++
> .../bindings/media/mediatek,mdp3-rsz.yaml | 55 +++++
> .../bindings/media/mediatek,mdp3-wrot.yaml | 57 ++++++
> .../bindings/soc/mediatek/mediatek,ccorr.yaml | 47 +++++
> .../bindings/soc/mediatek/mediatek,wdma.yaml | 58 ++++++
> 5 files changed, 410 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/mediatek,mdp3-rdma.yaml
> create mode 100644 Documentation/devicetree/bindings/media/mediatek,mdp3-rsz.yaml
> create mode 100644 Documentation/devicetree/bindings/media/mediatek,mdp3-wrot.yaml
> create mode 100644 Documentation/devicetree/bindings/soc/mediatek/mediatek,ccorr.yaml
> create mode 100644 Documentation/devicetree/bindings/soc/mediatek/mediatek,wdma.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/mediatek,mdp3-rdma.yaml b/Documentation/devicetree/bindings/media/mediatek,mdp3-rdma.yaml
> new file mode 100644
> index 000000000000..002503383934
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/mediatek,mdp3-rdma.yaml
> @@ -0,0 +1,193 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/mediatek,mdp3-rdma.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek Read Direct Memory Access
> +
> +maintainers:
> + - Matthias Brugger <matthias.bgg@gmail.com>
> +
> +description: |
> + Mediatek Read Direct Memory Access(RDMA) component used to do read DMA.
> + It contains one line buffer to store the sufficient pixel data, and
> + must be siblings to the central MMSYS_CONFIG node.
> + For a description of the MMSYS_CONFIG binding, see
> + Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
> + for details.
> + The 1st RDMA is also used to be a controller node in Media Data Path 3(MDP3)
> + that containing MMSYS, MUTEX, GCE and SCP settings.
> +
> +properties:
> + compatible:
> + oneOf:
> + - items:
> + # MDP3 controller node
> + - const: mediatek,mt8183-mdp3
How is this more specific than this:
> + - const: mediatek,mt8183-mdp3-rdma0
> + - items:
> + # normal RDMA conponent
> + - const: mediatek,mt8183-mdp3-rdma0
> +
> + mediatek,scp:
> + description: The node of system control processor (SCP), using
> + the remoteproc & rpmsg framework.
> + $ref: /schemas/types.yaml#/definitions/phandle
> + maxItems: 1
> +
> + mediatek,mdp3-comps:
> + description: MTK sub-system of direct-link or DIP
This needs a better description. What is DIP? What is direct-link?
> + $ref: /schemas/types.yaml#/definitions/string-array
> + items:
> + - enum:
> + # MDP direct-link input path selection, create a
> + # component for path connectedness of HW pipe control
> + - mediatek,mt8183-mdp3-dl1
> + - enum:
> + - mediatek,mt8183-mdp3-dl2
> + - enum:
> + # MDP direct-link output path selection, create a
> + # component for path connectedness of HW pipe control
> + - mediatek,mt8183-mdp3-path1
> + - enum:
> + - mediatek,mt8183-mdp3-path2
> + - enum:
> + # Input DMA of ISP PASS2 (DIP) module for raw image input
> + - mediatek,mt8183-mdp3-imgi
> + - enum:
> + # Output DMA of ISP PASS2 (DIP) module for YUV image output
> + - mediatek,mt8183-mdp3-exto
There's only 1 possible value for mediatek,mdp3-comps, so why does it
need to be in DT?
> +
> + reg:
> + items:
> + - description: basic RDMA HW address
> + - description: MDP direct-link 1st and 2nd input
> + - description: MDP direct-link 1st output
> + - description: MDP direct-link 2nd output
> + - description: ISP input and output
> +
> + mediatek,gce-client-reg:
> + description: The register of client driver can be configured by gce with
> + 4 arguments defined in this property, such as phandle of gce, subsys id,
> + register offset and size. Each GCE subsys id is mapping to a client
> + defined in the header include/dt-bindings/gce/<chip>-gce.h.
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + items:
> + - description: GCE client for RDMA
> + - description: GCR client for MDP direct-link 1st and 2nd input
> + - description: GCR client for MDP direct-link 1st output
> + - description: GCR client for MDP direct-link 2nd output
> + - description: GCR client for ISP input and output
> +
> + power-domains:
> + maxItems: 1
> +
> + clocks:
> + items:
> + - description: RDMA clock
> + - description: RSZ clock
> + - description: direck-link TX clock in MDP side
> + - description: direck-link RX clock in MDP side
> + - description: direck-link TX clock in ISP side
> + - description: direck-link RX clock in ISP side
> +
> + iommus:
> + maxItems: 1
> +
> + mediatek,mmsys:
> + description: The node of mux(multiplexer) controller for HW connections.
> + $ref: /schemas/types.yaml#/definitions/phandle
> +
> + mediatek,mm-mutex:
Is this some sort of h/w lock? We have a standard binding for that.
> + description: The node of sof(start of frame) signal controller.
> + $ref: /schemas/types.yaml#/definitions/phandle
> + maxItems: 1
> +
> + mediatek,mailbox-gce:
> + description: The node of global command engine (GCE), used to read/write
> + registers with critical time limitation.
> + $ref: /schemas/types.yaml#/definitions/phandle
> +
> + mboxes:
> + items:
> + - description: used for 1st data pipe from RDMA
> + - description: used for 2nd data pipe from RDMA
> + - description: used for 3rd data pipe from Direct-Link
> + - description: used for 4th data pipe from Direct-Link
> +
> + gce-subsys:
> + description: sub-system id corresponding to the global command engine (GCE)
> + register address.
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> +
> +if:
> + properties:
> + compatible:
> + contains:
> + const: mediatek,mt8183-mdp3
> +
> +then:
> + required:
> + - mediatek,scp
> + - mediatek,mmsys
> + - mediatek,mm-mutex
> + - mediatek,mailbox-gce
> + - mboxes
> + - gce-subsys
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - mediatek,gce-client-reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/mt8183-clk.h>
> + #include <dt-bindings/gce/mt8183-gce.h>
> + #include <dt-bindings/power/mt8183-power.h>
> + #include <dt-bindings/memory/mt8183-larb-port.h>
> +
> + mdp3_rdma0: mdp3_rdma0@14001000 {
> + compatible = "mediatek,mt8183-mdp3",
> + "mediatek,mt8183-mdp3-rdma0";
> + mediatek,scp = <&scp>;
> + mediatek,mdp3-comps = "mediatek,mt8183-mdp3-dl1",
> + "mediatek,mt8183-mdp3-dl2",
> + "mediatek,mt8183-mdp3-path1",
> + "mediatek,mt8183-mdp3-path2",
> + "mediatek,mt8183-mdp3-imgi",
> + "mediatek,mt8183-mdp3-exto";
> + reg = <0x14001000 0x1000>,
> + <0x14000000 0x1000>,
> + <0x14005000 0x1000>,
> + <0x14006000 0x1000>,
> + <0x15020000 0x1000>;
> + mediatek,gce-client-reg = <&gce SUBSYS_1400XXXX 0x1000 0x1000>,
> + <&gce SUBSYS_1400XXXX 0 0x1000>,
> + <&gce SUBSYS_1400XXXX 0x5000 0x1000>,
> + <&gce SUBSYS_1400XXXX 0x6000 0x1000>,
> + <&gce SUBSYS_1502XXXX 0 0x1000>;
> + power-domains = <&spm MT8183_POWER_DOMAIN_DISP>;
> + clocks = <&mmsys CLK_MM_MDP_RDMA0>,
> + <&mmsys CLK_MM_MDP_RSZ1>,
> + <&mmsys CLK_MM_MDP_DL_TXCK>,
> + <&mmsys CLK_MM_MDP_DL_RX>,
> + <&mmsys CLK_MM_IPU_DL_TXCK>,
> + <&mmsys CLK_MM_IPU_DL_RX>;
> + iommus = <&iommu>;
> + mediatek,mmsys = <&mmsys>;
> + mediatek,mm-mutex = <&mutex>;
> + mediatek,mailbox-gce = <&gce>;
> + mboxes = <&gce 20 CMDQ_THR_PRIO_LOWEST>,
> + <&gce 21 CMDQ_THR_PRIO_LOWEST>,
> + <&gce 22 CMDQ_THR_PRIO_LOWEST>,
> + <&gce 23 CMDQ_THR_PRIO_LOWEST>;
> + gce-subsys = <&gce 0x14000000 SUBSYS_1400XXXX>,
> + <&gce 0x14010000 SUBSYS_1401XXXX>,
> + <&gce 0x14020000 SUBSYS_1402XXXX>,
> + <&gce 0x15020000 SUBSYS_1502XXXX>;
> + };
> diff --git a/Documentation/devicetree/bindings/media/mediatek,mdp3-rsz.yaml b/Documentation/devicetree/bindings/media/mediatek,mdp3-rsz.yaml
> new file mode 100644
> index 000000000000..cd4cf1531535
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/mediatek,mdp3-rsz.yaml
> @@ -0,0 +1,55 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/mediatek,mdp3-rsz.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek Resizer
> +
> +maintainers:
> + - Matthias Brugger <matthias.bgg@gmail.com>
> +
> +description: |
> + One of Media Data Path 3 (MDP3) components used to do frame resizing.
> +
> +properties:
> + compatible:
> + items:
> + - enum:
> + - mediatek,mt8183-mdp3-rsz0
> + - mediatek,mt8183-mdp3-rsz1
Again, what's the difference between 0 and 1?
I've probably asked that before, but without a sufficient reasoning
here in the schema I'm just going to keep asking the same question.
Rob
^ permalink raw reply
* Re: [PATCH 1/3] dt-binding: mtd: nand: Document the wp-gpios property
From: Rob Herring @ 2022-01-21 21:19 UTC (permalink / raw)
To: Christophe Kerello
Cc: robh+dt, miquel.raynal, vigneshr, richard, chenshumin86,
linux-stm32, devicetree, linux-mtd, linux-kernel,
srinivas.kandagatla
In-Reply-To: <20220105135734.271313-2-christophe.kerello@foss.st.com>
On Wed, 05 Jan 2022 14:57:32 +0100, Christophe Kerello wrote:
> A few drivers use this property to describe the GPIO pin used to protect
> the NAND during program/erase operations.
>
> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
> ---
> Documentation/devicetree/bindings/mtd/nand-controller.yaml | 7 +++++++
> 1 file changed, 7 insertions(+)
>
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v4 1/3] dt-bindings: interconnect: imx8m-noc: Add fsl,icc-id property
From: Rob Herring @ 2022-01-21 21:27 UTC (permalink / raw)
To: Abel Vesa
Cc: Georgi Djakov, Shawn Guo, Sascha Hauer, Fabio Estevam,
Pengutronix Kernel Team, NXP Linux Team, linux-pm,
linux-arm-kernel, Linux Kernel Mailing List, devicetree
In-Reply-To: <20220106164150.3474048-1-abel.vesa@nxp.com>
On Thu, Jan 06, 2022 at 06:41:48PM +0200, Abel Vesa wrote:
> Add documentation for fsl,icc-id property.
>
> Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
> ---
>
> Changes since v3:
> * fixed typo in property description
>
> .../devicetree/bindings/interconnect/fsl,imx8m-noc.yaml | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml b/Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml
> index b8204ed22dd5..dc7f6b6f508a 100644
> --- a/Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml
> +++ b/Documentation/devicetree/bindings/interconnect/fsl,imx8m-noc.yaml
> @@ -47,6 +47,11 @@ properties:
> operating-points-v2: true
> opp-table: true
>
> + fsl,icc-id:
> + $ref: "/schemas/types.yaml#/definitions/uint32"
> + description:
> + unique ID used for linking i.MX bus or ddrc node to interconnect
Where does this unique ID come from and how is it used? Why aren't cells
in 'interconnects' sufficient for this?
> +
> fsl,ddrc:
> $ref: "/schemas/types.yaml#/definitions/phandle"
> description:
> --
> 2.31.1
>
>
^ permalink raw reply
* Re: [PATCH v3 2/3] dt-bindings: pwm: Document clk based PWM controller
From: Sean Anderson @ 2022-01-21 21:34 UTC (permalink / raw)
To: Krzysztof Kozlowski, Nikita Travkin
Cc: thierry.reding, lee.jones, u.kleine-koenig, robh+dt, sboyd,
linus.walleij, masneyb, linux-pwm, devicetree,
linux-kernel@vger.kernel.org, ~postmarketos/upstreaming
In-Reply-To: <CAJKOXPc249vbZZwjXxfg+mEgqQe0P8uhf1GTg8Db9sBeMY3+tA@mail.gmail.com>
On 1/21/22 2:34 AM, Krzysztof Kozlowski wrote:
> On Thu, 20 Jan 2022 at 17:15, Nikita Travkin <nikita@trvn.ru> wrote:
>>
>> Add YAML devicetree binding for clk based PWM controller
>>
>> Signed-off-by: Nikita Travkin <nikita@trvn.ru>
>> --
>> Changes in v2:
>> - fix the file name.
>> ---
>> .../devicetree/bindings/pwm/clk-pwm.yaml | 45 +++++++++++++++++++
>> 1 file changed, 45 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/pwm/clk-pwm.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/clk-pwm.yaml b/Documentation/devicetree/bindings/pwm/clk-pwm.yaml
>> new file mode 100644
>> index 000000000000..4fb2c1baaad4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/clk-pwm.yaml
>> @@ -0,0 +1,45 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pwm/clk-pwm.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Clock based PWM controller
>> +
>> +maintainers:
>> + - Nikita Travkin <nikita@trvn.ru>
>> +
>> +description: |
>> + Some systems have clocks that can be exposed to external devices.
>> + (e.g. by muxing them to GPIO pins)
>> + It's often possible to control duty-cycle of such clocks which makes them
>> + suitable for generating PWM signal.
>> +
>> +allOf:
>> + - $ref: pwm.yaml#
>> +
>> +properties:
>> + compatible:
>> + const: clk-pwm
>> +
>> + clocks:
>> + description: Clock used to generate the signal.
>> + maxItems: 1
>> +
>> + "#pwm-cells":
>> + const: 2
>> +
>> +unevaluatedProperties: false
>> +
>> +required:
>> + - clocks
>> +
>> +examples:
>> + - |
>> + pwm-flash {
>
> Node names should be generic (see devicetree specification), so just "pwm".
And then what will you do if you have two clock-based pwms?
--Sean
^ permalink raw reply
* Re: [PATCH v12 2/2] pwm: Add support for Xilinx AXI Timer
From: Sean Anderson @ 2022-01-21 21:34 UTC (permalink / raw)
To: linux-pwm, devicetree, Thierry Reding
Cc: michal.simek, Mubin Usman Sayyed, linux-kernel,
Uwe Kleine-König, Alvaro Gamez, Lee Jones, linux-arm-kernel
In-Reply-To: <20211217233015.67664-2-sean.anderson@seco.com>
On 12/17/21 6:30 PM, Sean Anderson wrote:
> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
> found on Xilinx FPGAs. At the moment clock control is very basic: we
> just enable the clock during probe and pin the frequency. In the future,
> someone could add support for disabling the clock when not in use.
>
> Some common code has been specially demarcated. While currently only
> used by the PWM driver, it is anticipated that it may be split off in
> the future to be used by the timer driver as well.
>
> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
>
> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
>
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> Acked-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
> Changes in v12:
> - Add a comment to the timer driver about #pwm-cells
> - Combine/expand comments on rounding in xilinx_pwm_apply
>
> Changes in v11:
> - Add comment about why we test for #pwm-cells
> - Clarify comment on generate out signal
> - Rename pwm variables to xilinx_pwm
> - Round like Uwe wants...
> - s/xilinx_timer/xilinx_pwm/ for non-common functions
>
> Changes in v10:
> - Fix compilation error in timer driver
>
> Changes in v9:
> - Refactor "if { return } else if { }" to "if { return } if { }"
> - Remove drivers/clocksource/timer-xilinx-common.c from MAINTAINERS
> - Remove xilinx_timer_common_init and integrate it into xilinx_timer_probe
>
> Changes in v8:
> - Drop new timer driver; it has been deferred for future series
>
> Changes in v7:
> - Add dependency on OF_ADDRESS
> - Fix period_cycles calculation
> - Fix typo in limitations
>
> Changes in v6:
> - Capitalize error messages
> - Don't disable regmap locking to allow inspection of registers via
> debugfs
> - Prevent overflow when calculating period_cycles
> - Remove enabled variable from xilinx_pwm_apply
> - Swap order of period_cycle range comparisons
>
> Changes in v5:
> - Allow non-zero #pwm-cells
> - Correctly set duty_cycle in get_state when TLR0=TLR1
> - Elaborate on limitation section
> - Perform some additional checks/rounding in apply_state
> - Remove xlnx,axi-timer-2.0 compatible string
> - Rework duty-cycle and period calculations with feedback from Uwe
> - Switch to regmap to abstract endianness issues
> - Use more verbose error messages
>
> Changes in v4:
> - Don't use volatile in read/write replacements. Some arches have it and
> some don't.
> - Put common timer properties into their own struct to better reuse
> code.
> - Remove references to properties which are not good enough for Linux.
>
> Changes in v3:
> - Add clockevent and clocksource support
> - Remove old microblaze driver
> - Rewrite probe to only use a device_node, since timers may need to be
> initialized before we have proper devices. This does bloat the code a bit
> since we can no longer rely on helpers such as dev_err_probe. We also
> cannot rely on device resources being free'd on failure, so we must free
> them manually.
> - We now access registers through xilinx_timer_(read|write). This allows us
> to deal with endianness issues, as originally seen in the microblaze
> driver. CAVEAT EMPTOR: I have not tested this on big-endian!
>
> Changes in v2:
> - Add comment describing device
> - Add comment explaining why we depend on !MICROBLAZE
> - Add dependencies on COMMON_CLK and HAS_IOMEM
> - Cast dividends to u64 to avoid overflow
> - Check for over- and underflow when calculating TLR
> - Check range of xlnx,count-width
> - Don't compile this module by default for arm64
> - Don't set pwmchip.base to -1
> - Ensure the clock is always running when the pwm is registered
> - Remove debugfs file :l
> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
> - Report errors with dev_error_probe
> - Set xilinx_pwm_ops.owner
> - Use NSEC_TO_SEC instead of defining our own
> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by Uwe
>
> MAINTAINERS | 6 +
> arch/microblaze/kernel/timer.c | 4 +
> drivers/pwm/Kconfig | 14 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-xilinx.c | 319 +++++++++++++++++++++++++++++
> include/clocksource/timer-xilinx.h | 91 ++++++++
> 6 files changed, 435 insertions(+)
> create mode 100644 drivers/pwm/pwm-xilinx.c
> create mode 100644 include/clocksource/timer-xilinx.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 13f9a84a617e..373757fcc8c6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20925,6 +20925,12 @@ F: drivers/misc/Makefile
> F: drivers/misc/xilinx_sdfec.c
> F: include/uapi/misc/xilinx_sdfec.h
>
> +XILINX PWM DRIVER
> +M: Sean Anderson <sean.anderson@seco.com>
> +S: Maintained
> +F: drivers/pwm/pwm-xilinx.c
> +F: include/clocksource/timer-xilinx.h
> +
> XILINX UARTLITE SERIAL DRIVER
> M: Peter Korsgaard <jacmet@sunsite.dk>
> L: linux-serial@vger.kernel.org
> diff --git a/arch/microblaze/kernel/timer.c b/arch/microblaze/kernel/timer.c
> index f8832cf49384..26c385582c3b 100644
> --- a/arch/microblaze/kernel/timer.c
> +++ b/arch/microblaze/kernel/timer.c
> @@ -251,6 +251,10 @@ static int __init xilinx_timer_init(struct device_node *timer)
> u32 timer_num = 1;
> int ret;
>
> + /* If this property is present, the device is a PWM and not a timer */
> + if (of_property_read_bool(timer, "#pwm-cells"))
> + return 0;
> +
> if (initialized)
> return -EINVAL;
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 21e3b05a5153..cefbf00b4c7e 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -640,4 +640,18 @@ config PWM_VT8500
> To compile this driver as a module, choose M here: the module
> will be called pwm-vt8500.
>
> +config PWM_XILINX
> + tristate "Xilinx AXI Timer PWM support"
> + depends on OF_ADDRESS
> + depends on COMMON_CLK
> + select REGMAP_MMIO
> + help
> + PWM driver for Xilinx LogiCORE IP AXI timers. This timer is
> + typically a soft core which may be present in Xilinx FPGAs.
> + This device may also be present in Microblaze soft processors.
> + If you don't have this IP in your design, choose N.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-xilinx.
> +
> endif
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 708840b7fba8..ea785480359b 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -60,3 +60,4 @@ obj-$(CONFIG_PWM_TWL) += pwm-twl.o
> obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.o
> obj-$(CONFIG_PWM_VISCONTI) += pwm-visconti.o
> obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o
> +obj-$(CONFIG_PWM_XILINX) += pwm-xilinx.o
> diff --git a/drivers/pwm/pwm-xilinx.c b/drivers/pwm/pwm-xilinx.c
> new file mode 100644
> index 000000000000..b4d93e8812c6
> --- /dev/null
> +++ b/drivers/pwm/pwm-xilinx.c
> @@ -0,0 +1,319 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2021 Sean Anderson <sean.anderson@seco.com>
> + *
> + * Limitations:
> + * - When changing both duty cycle and period, we may end up with one cycle
> + * with the old duty cycle and the new period. This is because the counters
> + * may only be reloaded by first stopping them, or by letting them be
> + * automatically reloaded at the end of a cycle. If this automatic reload
> + * happens after we set TLR0 but before we set TLR1 then we will have a
> + * bad cycle. This could probably be fixed by reading TCR0 just before
> + * reprogramming, but I think it would add complexity for little gain.
> + * - Cannot produce 100% duty cycle by configuring the TLRs. This might be
> + * possible by stopping the counters at an appropriate point in the cycle,
> + * but this is not (yet) implemented.
> + * - Only produces "normal" output.
> + * - Always produces low output if disabled.
> + */
> +
> +#include <clocksource/timer-xilinx.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +
> +/*
> + * The following functions are "common" to drivers for this device, and may be
> + * exported at a future date.
> + */
> +u32 xilinx_timer_tlr_cycles(struct xilinx_timer_priv *priv, u32 tcsr,
> + u64 cycles)
> +{
> + WARN_ON(cycles < 2 || cycles - 2 > priv->max);
> +
> + if (tcsr & TCSR_UDT)
> + return cycles - 2;
> + return priv->max - cycles + 2;
> +}
> +
> +unsigned int xilinx_timer_get_period(struct xilinx_timer_priv *priv,
> + u32 tlr, u32 tcsr)
> +{
> + u64 cycles;
> +
> + if (tcsr & TCSR_UDT)
> + cycles = tlr + 2;
> + else
> + cycles = (u64)priv->max - tlr + 2;
> +
> + /* cycles has a max of 2^32 + 2 */
> + return DIV64_U64_ROUND_UP(cycles * NSEC_PER_SEC,
> + clk_get_rate(priv->clk));
> +}
> +
> +/*
> + * The idea here is to capture whether the PWM is actually running (e.g.
> + * because we or the bootloader set it up) and we need to be careful to ensure
> + * we don't cause a glitch. According to the data sheet, to enable the PWM we
> + * need to
> + *
> + * - Set both timers to generate mode (MDT=1)
> + * - Set both timers to PWM mode (PWMA=1)
> + * - Enable the generate out signals (GENT=1)
> + *
> + * In addition,
> + *
> + * - The timer must be running (ENT=1)
> + * - The timer must auto-reload TLR into TCR (ARHT=1)
> + * - We must not be in the process of loading TLR into TCR (LOAD=0)
> + * - Cascade mode must be disabled (CASC=0)
> + *
> + * If any of these differ from usual, then the PWM is either disabled, or is
> + * running in a mode that this driver does not support.
> + */
> +#define TCSR_PWM_SET (TCSR_GENT | TCSR_ARHT | TCSR_ENT | TCSR_PWMA)
> +#define TCSR_PWM_CLEAR (TCSR_MDT | TCSR_LOAD)
> +#define TCSR_PWM_MASK (TCSR_PWM_SET | TCSR_PWM_CLEAR)
> +
> +struct xilinx_pwm_device {
> + struct pwm_chip chip;
> + struct xilinx_timer_priv priv;
> +};
> +
> +static inline struct xilinx_timer_priv
> +*xilinx_pwm_chip_to_priv(struct pwm_chip *chip)
> +{
> + return &container_of(chip, struct xilinx_pwm_device, chip)->priv;
> +}
> +
> +static bool xilinx_timer_pwm_enabled(u32 tcsr0, u32 tcsr1)
> +{
> + return ((TCSR_PWM_MASK | TCSR_CASC) & tcsr0) == TCSR_PWM_SET &&
> + (TCSR_PWM_MASK & tcsr1) == TCSR_PWM_SET;
> +}
> +
> +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
> + const struct pwm_state *state)
> +{
> + struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
> + u32 tlr0, tlr1, tcsr0, tcsr1;
> + u64 period_cycles, duty_cycles;
> + unsigned long rate;
> +
> + if (state->polarity != PWM_POLARITY_NORMAL)
> + return -EINVAL;
> +
> + /*
> + * To be representable by TLR, cycles must be between 2 and
> + * priv->max + 2. To enforce this we can reduce the cycles, but we may
> + * not increase them. Caveat emptor: while this does result in more
> + * predictable rounding, it may also result in a completely different
> + * duty cycle (% high time) than what was requested.
> + */
> + rate = clk_get_rate(priv->clk);
> + /* Avoid overflow */
> + period_cycles = min_t(u64, state->period, ULONG_MAX * NSEC_PER_SEC);
> + period_cycles = mul_u64_u32_div(period_cycles, rate, NSEC_PER_SEC);
> + period_cycles = min_t(u64, period_cycles, priv->max + 2);
> + if (period_cycles < 2)
> + return -ERANGE;
> +
> + /* Same thing for duty cycles */
> + duty_cycles = min_t(u64, state->duty_cycle, ULONG_MAX * NSEC_PER_SEC);
> + duty_cycles = mul_u64_u32_div(duty_cycles, rate, NSEC_PER_SEC);
> + duty_cycles = min_t(u64, duty_cycles, priv->max + 2);
> +
> + /*
> + * If we specify 100% duty cycle, we will get 0% instead, so decrease
> + * the duty cycle count by one.
> + */
> + if (duty_cycles >= period_cycles)
> + duty_cycles = period_cycles - 1;
> +
> + /* Round down to 0% duty cycle for unrepresentable duty cycles */
> + if (duty_cycles < 2)
> + duty_cycles = period_cycles;
> +
> + regmap_read(priv->map, TCSR0, &tcsr0);
> + regmap_read(priv->map, TCSR1, &tcsr1);
> + tlr0 = xilinx_timer_tlr_cycles(priv, tcsr0, period_cycles);
> + tlr1 = xilinx_timer_tlr_cycles(priv, tcsr1, duty_cycles);
> + regmap_write(priv->map, TLR0, tlr0);
> + regmap_write(priv->map, TLR1, tlr1);
> +
> + if (state->enabled) {
> + /*
> + * If the PWM is already running, then the counters will be
> + * reloaded at the end of the current cycle.
> + */
> + if (!xilinx_timer_pwm_enabled(tcsr0, tcsr1)) {
> + /* Load TLR into TCR */
> + regmap_write(priv->map, TCSR0, tcsr0 | TCSR_LOAD);
> + regmap_write(priv->map, TCSR1, tcsr1 | TCSR_LOAD);
> + /* Enable timers all at once with ENALL */
> + tcsr0 = (TCSR_PWM_SET & ~TCSR_ENT) | (tcsr0 & TCSR_UDT);
> + tcsr1 = TCSR_PWM_SET | TCSR_ENALL | (tcsr1 & TCSR_UDT);
> + regmap_write(priv->map, TCSR0, tcsr0);
> + regmap_write(priv->map, TCSR1, tcsr1);
> + }
> + } else {
> + regmap_write(priv->map, TCSR0, 0);
> + regmap_write(priv->map, TCSR1, 0);
> + }
> +
> + return 0;
> +}
> +
> +static void xilinx_pwm_get_state(struct pwm_chip *chip,
> + struct pwm_device *unused,
> + struct pwm_state *state)
> +{
> + struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
> + u32 tlr0, tlr1, tcsr0, tcsr1;
> +
> + regmap_read(priv->map, TLR0, &tlr0);
> + regmap_read(priv->map, TLR1, &tlr1);
> + regmap_read(priv->map, TCSR0, &tcsr0);
> + regmap_read(priv->map, TCSR1, &tcsr1);
> + state->period = xilinx_timer_get_period(priv, tlr0, tcsr0);
> + state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1);
> + state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1);
> + state->polarity = PWM_POLARITY_NORMAL;
> +
> + /* 100% duty cycle results in constant low output */
> + if (state->period == state->duty_cycle)
> + state->duty_cycle = 0;
> +}
> +
> +static const struct pwm_ops xilinx_pwm_ops = {
> + .apply = xilinx_pwm_apply,
> + .get_state = xilinx_pwm_get_state,
> + .owner = THIS_MODULE,
> +};
> +
> +static const struct regmap_config xilinx_pwm_regmap_config = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .val_format_endian = REGMAP_ENDIAN_LITTLE,
> + .max_register = TCR1,
> +};
> +
> +static int xilinx_pwm_probe(struct platform_device *pdev)
> +{
> + int ret;
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + struct xilinx_timer_priv *priv;
> + struct xilinx_pwm_device *xilinx_pwm;
> + u32 pwm_cells, one_timer, width;
> + void __iomem *regs;
> +
> + /* If there are no PWM cells, this binding is for a timer */
> + ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
> + if (ret == -EINVAL)
> + return -ENODEV;
> + if (ret)
> + return dev_err_probe(dev, ret, "could not read #pwm-cells\n");
> +
> + xilinx_pwm = devm_kzalloc(dev, sizeof(*xilinx_pwm), GFP_KERNEL);
> + if (!xilinx_pwm)
> + return -ENOMEM;
> + platform_set_drvdata(pdev, xilinx_pwm);
> + priv = &xilinx_pwm->priv;
> +
> + regs = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(regs))
> + return PTR_ERR(regs);
> +
> + priv->map = devm_regmap_init_mmio(dev, regs,
> + &xilinx_pwm_regmap_config);
> + if (IS_ERR(priv->map))
> + return dev_err_probe(dev, PTR_ERR(priv->map),
> + "Could not create regmap\n");
> +
> + ret = of_property_read_u32(np, "xlnx,one-timer-only", &one_timer);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Could not read xlnx,one-timer-only\n");
> +
> + if (one_timer)
> + return dev_err_probe(dev, -EINVAL,
> + "Two timers required for PWM mode\n");
> +
> +
> + ret = of_property_read_u32(np, "xlnx,count-width", &width);
> + if (ret == -EINVAL)
> + width = 32;
> + else if (ret)
> + return dev_err_probe(dev, ret,
> + "Could not read xlnx,count-width\n");
> +
> + if (width != 8 && width != 16 && width != 32)
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid counter width %d\n", width);
> + priv->max = BIT_ULL(width) - 1;
> +
> + /*
> + * The polarity of the Generate Out signals must be active high for PWM
> + * mode to work. We could determine this from the device tree, but
> + * alas, such properties are not allowed to be used.
> + */
> +
> + priv->clk = devm_clk_get(dev, "s_axi_aclk");
> + if (IS_ERR(priv->clk))
> + return dev_err_probe(dev, PTR_ERR(priv->clk),
> + "Could not get clock\n");
> +
> + ret = clk_prepare_enable(priv->clk);
> + if (ret)
> + return dev_err_probe(dev, ret, "Clock enable failed\n");
> + clk_rate_exclusive_get(priv->clk);
> +
> + xilinx_pwm->chip.dev = dev;
> + xilinx_pwm->chip.ops = &xilinx_pwm_ops;
> + xilinx_pwm->chip.npwm = 1;
> + ret = pwmchip_add(&xilinx_pwm->chip);
> + if (ret) {
> + clk_rate_exclusive_put(priv->clk);
> + clk_disable_unprepare(priv->clk);
> + return dev_err_probe(dev, ret, "Could not register PWM chip\n");
> + }
> +
> + return 0;
> +}
> +
> +static int xilinx_pwm_remove(struct platform_device *pdev)
> +{
> + struct xilinx_pwm_device *xilinx_pwm = platform_get_drvdata(pdev);
> +
> + pwmchip_remove(&xilinx_pwm->chip);
> + clk_rate_exclusive_put(xilinx_pwm->priv.clk);
> + clk_disable_unprepare(xilinx_pwm->priv.clk);
> + return 0;
> +}
> +
> +static const struct of_device_id xilinx_pwm_of_match[] = {
> + { .compatible = "xlnx,xps-timer-1.00.a", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, xilinx_pwm_of_match);
> +
> +static struct platform_driver xilinx_pwm_driver = {
> + .probe = xilinx_pwm_probe,
> + .remove = xilinx_pwm_remove,
> + .driver = {
> + .name = "xilinx-pwm",
> + .of_match_table = of_match_ptr(xilinx_pwm_of_match),
> + },
> +};
> +module_platform_driver(xilinx_pwm_driver);
> +
> +MODULE_ALIAS("platform:xilinx-pwm");
> +MODULE_DESCRIPTION("PWM driver for Xilinx LogiCORE IP AXI Timer");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/clocksource/timer-xilinx.h b/include/clocksource/timer-xilinx.h
> new file mode 100644
> index 000000000000..1f7757b84a5e
> --- /dev/null
> +++ b/include/clocksource/timer-xilinx.h
> @@ -0,0 +1,91 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2021 Sean Anderson <sean.anderson@seco.com>
> + */
> +
> +#ifndef XILINX_TIMER_H
> +#define XILINX_TIMER_H
> +
> +#include <linux/compiler.h>
> +
> +#define TCSR0 0x00
> +#define TLR0 0x04
> +#define TCR0 0x08
> +#define TCSR1 0x10
> +#define TLR1 0x14
> +#define TCR1 0x18
> +
> +#define TCSR_MDT BIT(0)
> +#define TCSR_UDT BIT(1)
> +#define TCSR_GENT BIT(2)
> +#define TCSR_CAPT BIT(3)
> +#define TCSR_ARHT BIT(4)
> +#define TCSR_LOAD BIT(5)
> +#define TCSR_ENIT BIT(6)
> +#define TCSR_ENT BIT(7)
> +#define TCSR_TINT BIT(8)
> +#define TCSR_PWMA BIT(9)
> +#define TCSR_ENALL BIT(10)
> +#define TCSR_CASC BIT(11)
> +
> +struct clk;
> +struct device_node;
> +struct regmap;
> +
> +/**
> + * struct xilinx_timer_priv - Private data for Xilinx AXI timer drivers
> + * @map: Regmap of the device, possibly with an offset
> + * @clk: Parent clock
> + * @max: Maximum value of the counters
> + */
> +struct xilinx_timer_priv {
> + struct regmap *map;
> + struct clk *clk;
> + u32 max;
> +};
> +
> +/**
> + * xilinx_timer_tlr_cycles() - Calculate the TLR for a period specified
> + * in clock cycles
> + * @priv: The timer's private data
> + * @tcsr: The value of the TCSR register for this counter
> + * @cycles: The number of cycles in this period
> + *
> + * Callers of this function MUST ensure that @cycles is representable as
> + * a TLR.
> + *
> + * Return: The calculated value for TLR
> + */
> +u32 xilinx_timer_tlr_cycles(struct xilinx_timer_priv *priv, u32 tcsr,
> + u64 cycles);
> +
> +/**
> + * xilinx_timer_get_period() - Get the current period of a counter
> + * @priv: The timer's private data
> + * @tlr: The value of TLR for this counter
> + * @tcsr: The value of TCSR for this counter
> + *
> + * Return: The period, in ns
> + */
> +unsigned int xilinx_timer_get_period(struct xilinx_timer_priv *priv,
> + u32 tlr, u32 tcsr);
> +
> +/**
> + * xilinx_timer_common_init() - Perform common initialization for Xilinx
> + * AXI timer drivers.
> + * @priv: The timer's private data
> + * @np: The devicetree node for the timer
> + * @one_timer: Set to %1 if there is only one timer
> + *
> + * This performs common initialization, such as detecting endianness,
> + * and parsing devicetree properties. @priv->regs must be initialized
> + * before calling this function. This function initializes @priv->read,
> + * @priv->write, and @priv->width.
> + *
> + * Return: 0, or negative errno
> + */
> +int xilinx_timer_common_init(struct device_node *np,
> + struct xilinx_timer_priv *priv,
> + u32 *one_timer);
> +
> +#endif /* XILINX_TIMER_H */
>
ping?
Uwe/Thierry, can you have a look at this?
--Sean
^ permalink raw reply
* Re: [WIP PATCH] dt-bindings: display: msm: dsi-controller-main: distinguish DSI versions
From: Rob Herring @ 2022-01-21 21:41 UTC (permalink / raw)
To: David Heidelberg
Cc: Rob Clark, Sean Paul, Abhinav Kumar, David Airlie, Daniel Vetter,
Krishna Manikandan, ~okias/devicetree, linux-arm-msm, dri-devel,
freedreno, devicetree, linux-kernel
In-Reply-To: <20220108190059.72583-1-david@ixit.cz>
On Sat, Jan 08, 2022 at 08:00:58PM +0100, David Heidelberg wrote:
> Update documentation compatible and checking to comprehend
> both V2 and 6G version bindings.
>
> Following this commit, there will be update for
> compatible string in chipsets dtsi.
>
> Additional changes:
> - switch to unevaluatedProperties
>
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---
> Rob, I know you mentioned using rather chipset names, but since
> meanwhile I coded this, I'll let you decide if should make sense to
> change it or keep it this way.
It all depends on how many chips per version. I'm guessing only 1 or 2
given how many QCom SoCs I'm aware of.
I think this should probably be split to 2 docs for the v2 and 6g
versions.
Rob
^ permalink raw reply
* Re: [PATCH 4/4] arm64: dts: qcom: sc7280: Add herobrine-r1
From: Doug Anderson @ 2022-01-21 21:44 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Bjorn Andersson, quic_rjendra, Sibi Sankar, kgodara1,
Matthias Kaehlcke, Stephen Boyd, Prasad Malisetty, Andy Gross,
Rob Herring,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-arm-msm, LKML
In-Reply-To: <ab57f88c-8473-2499-afa6-86bcf639ca32@somainline.org>
Hi,
On Fri, Jan 21, 2022 at 10:00 AM Konrad Dybcio
<konrad.dybcio@somainline.org> wrote:
>
>
> Hi!
>
>
> Your DTs look good, but incorporate some weird style decisions..
Funny. I've been working on a DT style guidelines doc. I guess I need
to prioritize it...
> > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r0.dts
> > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r0.dts
> > @@ -26,7 +26,7 @@
> >
> > / {
> > model = "Google Herobrine (rev0)";
> > - compatible = "google,herobrine",
> > + compatible = "google,herobrine-rev0",
> > "qcom,sc7280";
> Why break the line here?
True, this can go on one line. One argument about having the SoC on a
separate line is to make it consistent for boards that have more revs
listed. That's not new for this patch, though. I could go either way,
honestly.
I'll spin for this unless someone wants to defend it being on more
than one line.
> > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts
> > new file mode 100644
> > index 000000000000..c57bd689df23
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts
> > @@ -0,0 +1,314 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * Google Herobrine board device tree source
> > + *
> > + * Copyright 2022 Google LLC.
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "sc7280-herobrine.dtsi"
> > +
> > +/ {
> > + model = "Google Herobrine (rev1+)";
> Are you sure there won't be any changes significant enough in the future
> that will make rev2 or rev7 or rev8192 incompatible with the rev1+ DT?
Definitely not. If there are significant changes, though, then we'll
need a new dts. ...but right now, as coded, this will _match_ against
anything rev1 or newer. This all has to do with now the bootloader
does matching and also about the standard practices we use. Let me try
to explain.
So you can see down below that we list the compatible as
"google,herobrine" and _not_ "google,herobrine-rev1"? This is on
purpose. The bootloader will read the board rev/sku strappings and
then will search for device trees with this priority order (for rev1,
sku0):
1. google,herobrine-rev1-sku0
2. google,herobrine-rev1
3. google,herobrine-sku0
4. google,herobrine
The general rule of thumb is that the newest board for any given
device should _not_ list a revision or a SKU in its compatible but
instead should just advertise itself as the default device tree for a
given board. Basically that means that the current device tree, as
coded, will match everything that's -rev1 or newer (because there is a
more specific dts for -rev0).
Why do we do this? If the board manufacturer makes a new spin of the
board and changes components around or changes routing or whatever, we
want them to populate a new board ID even if the differences are
_supposed_ to be invisible to software. This means that if the issues
turn out _not_ to be invisible to software that we can later work
around them. Usually when the board manufacturer spins the board like
this the want the old software to "just work", so we want the default
to automatically pick things up.
Even when changes _are_ visible to software, it's likely that a new
rev of a board will most closely match the previous version. Thus, if
we don't have a better board to match against, it's better to pick the
newest revision and have some chance to boot than to crash or pick
some other completely random dts file.
> > + compatible = "google,herobrine",
> > + "qcom,sc7280";
> Why break the line here?
See above.
> > +};
> > +
> > +/* ADDITIONS TO NODES DEFINED IN PARENT DEVICE TREE FILES */
> This is superfluous at best.
So in general I try to use titles like this to indicate a change in
the sort ordering. In different sections different sort ordering is
used.
I'll plan to keep these line headings as they are in my next spin
unless someone else wants to tie-break and tells me to get rid of
them.
> > +/*
> > + * The touchscreen connector might come off the Qcard, at least in the case of
> > + * eDP. Like the trackpad, we'll put it in the board device tree file since
> > + * different boards have different touchscreens.
> > + */
> > +ts_i2c: &i2c13 {
> Either sort these by their i2c aliases, or by their new ones.. currently it is
> not alphabetically sorted at all..
They are sorted by the name of what they are overriding. ...it can
certainly get a little awkward at times, but so can any rule. In this
case, the sort ordering is:
ap_spi_fp, i2c0, i2c13, pcie1, ...
I think the key here is that "ts_i2c" is a local label being added,
but the sort order is based on the node name being overridden
("i2c13"). The fact that extra local labels are being added can
confuse the issue for sure, but if you sort by local labels then that
can be confusing in different ways. Specifically the local labels are
optional, but if you later decide to add one you wouldn't want to
change the sort order.
I'll plan to keep my sort ordering for the next spin.
> Looks like some nodes below are just thrown at random places too..
I believe everything has a consistent ordering, you just need to know
what it is. At least I don't see anything mis-sorted.
> > +/* For nvme */
> > +&pcie1 {
> > + status = "okay";
> > +};
> > +
> > +/* For nvme */
> I think this is kind of obvious and there is no need for this to be said twice
> within 10 lines..
I guess? It feels like this is about the scoping of the comment. I
feel like these are comments that apply only to the next block and are
not "section" comments that apply to anything below. Thus while it
might be obvious it's inconsistent to exclude this comment.
Not planning to change this.
> > +/* PINCTRL - BOARD-SPECIFIC */
> This is also kind of obvious, if it wasn't board-specific, it wouldn't be in the
> board DT..
Sure, but it gets into:
1. Section headers indicate a change in sort ordering and explain why
the stuff below isn't sorted inline with the stuff above.
2. In some files there are actually two separate pinctrl sections, one
where we override / fill in details for parent nodes and one where we
have pins that are totally board specific. You can see herobrine.dtsi
where both pinctrl sections are marked.
> > + /*
> > + * AP_FLASH_WP is crossystem ABI. Schematics
> > + * call it BIOS_FLASH_WP_OD.
> > + */
> Is there a need to put this comment on 4 lines instead of a single one?
Probably doesn't need 4 more than one line now that people are more OK
w/ longer lines. This makes it to exactly 100 characters wide if on
one line. I guess it's a matter of opinion, but it actually looks
quite a bit uglier to me on one line. All the other lines are short
and this one really long line sticks out like a sore thumb.
I'll also note that the goal isn't to set 100 characters as the magic
limit. As commit bdc48fa11e46 ("checkpatch/coding-style: deprecate
80-column warning") says, "80 columns is certainly still _preferred_"
but that we should allow longer lines if they make things more
readable / prettier. I disagree that it's prettier or more readable to
put this on one line in this case.
> > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> > @@ -0,0 +1,781 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * Google Herobrine baseboard device tree source
> > + *
> > + * The set of things in this file is a bit loosely defined. It's roughly
> > + * defined as the set of things that the child boards happen to have in
> > + * common. Since all of the child boards started from the same original
> > + * design this is hopefully a large set of things but as more derivatives
> > + * appear things may "bubble down" out of this file. For things that are
> > + * part of the reference design but might not exist on child nodes we will
> > + * follow the lead of the SoC dtsi files and leave their status as "disabled".
> > + *
> > + * Copyright 2022 Google LLC.
> > + */
> > +
> > +#include <dt-bindings/gpio/gpio.h>
> Factoring gpio.h out into the SoC DT is a good idea.
I'm on the fence and I'd love a 2nd opinion. I don't see any usage of
the GPIO_ definitions there. However, I agree that pretty much all
boards should use them, so it probably makes sense...
Unless someone disagrees, I'll spin for this.
> > + /*
> > + * FIXED REGULATORS
> > + *
> > + * Sort order:
> > + * 1. parents above children.
> > + * 2. higher voltage above lower voltage.
> > + * 3. alphabetically by node name.
> Why not just alphabetically? These regulator-fixed nodes shouldn't
> have issues with probe order and their parent-child relations are
> specified in their properties.
Back in the day this used to reduce the amount of -EPROBE_DEFER that
the system would suffer at bootup. ...but aside from that, I grew to
feel that it made sense. For regulators you're often thinking them as
a power tree. Things closer to "mains" or the battery are at the top
and they feed into things that bring the voltage lower (since dropping
a voltage is usually easier than raising it). Thus this ordering made
a reasonable amount of sense for someone trying to understand the
power tree for a board. Sorting things alphabetically, on the other
hand, doesn't add anything to understanding. ...so why not sort by the
order that helps people understand the board better?
> > + */
> > +
> > + /* This is the top level supply and variable voltage */
> Is there a way to read out the voltage somehow, perhaps as a TODO for the future
> if a driver is needed?
I don't believe so. IIRC this is a voltage that can either be provided
by the battery or by "mains". I believe it feeds a bunch of stuff that
can take whatever voltage is thrown at it and then regulate it down to
something sane.
> I think the regulator framework used not to be very happy
> about not specifying a (fixed) voltage range on a fixed regulator, but I may be
> wrong..
It's never been a problem.
> > +
> > + pwmleds {
> > + compatible = "pwm-leds";
> > + status = "disabled";
> If it's disabled and it's not enabled anywhere else, why is it here?
> Is it going to have users in a very near future?
Right, that's the idea. It's actually on the schematics but just not
stuffed for -rev1 boards.
> > +ap_i2c_tpm: &i2c14 {
> > + status = "okay";
> > + clock-frequency = <400000>;
> > +
> > + tpm@50 {
> > + compatible = "google,cr50";
> > + reg = <0x50>;
> > +
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&gsc_ap_int_odl>;
> > +
> > + interrupt-parent = <&tlmm>;
> > + interrupts = <104 IRQ_TYPE_EDGE_RISING>;
> > + };
> > +};
> > +
> > +/* For nvme; not all herobrine boards have; boards set status = "okay" */
> "NVMe drive, enabled on a per-board basis"?
I guess. I'm not really convinced your wording is better but I'm fine
with it. I'll change to your wording.
> > +&tlmm {
> > + /*
> > + * pinctrl settings for pins that have no real owners.
> > + */
> You can make it /* one line */
Sure, I can spin for this.
> Also, the following pins seem to be in random order, not sorted by either their
> name nor by their gpio number..
I think only "hub_en" is mis-sorted, right? Then they're sorted by
node name. I'll spin for that.
> > +
> > +&ipa {
> > + status = "okay";
> > + modem-init;
> > +};
> > +
> > +/* For nvme; boards set status = "okay" */
> This is kind of obvious, no?
Sure, I can remove this comment.
> > +/* For eMMC; not all Qcards have eMMC stuffed; boards set status = "okay" */
> Same here.
Actually, this provides some pretty useful info IMO. I don't think
it'd be terribly obvious that eMMC is on the Qcard PCB (if stuffed)
but not always stuffed.
> > +mos_bt_uart: &uart7 {
> > + status = "okay";
> > +
> > + /delete-property/interrupts;
> I think generally one should put a space after '/'.
OK, I'll spin with this.
> > +/* For mos_bt_uart */
> > +&qup_uart7_cts {
> > + /*
> > + * Configure a pull-down on CTS to match the pull of
> > + * the Bluetooth module.
> My email client doesn't show me the column count, but I think this would
> fit in a single 100 char line..
IMO it doesn't exactly look ugly the way it is, but sure I can make it
one line. Seems to look fine that way too. I'll change it to one line.
> > + ts_rst_conn: ts-rst-conn {
> > + pins = "gpio54";
> > + function = "gpio";
> > + bias-pull-up;
> > + drive-strength = <2>;
> Please be consistent in the order in which you add the same properties throughout
> GPIO nodes.
Sure, I'll spin for this.
-Doug
^ permalink raw reply
* Re: [PATCH v3] dt-bindings: display/msm: hdmi: split and convert to yaml
From: Rob Herring @ 2022-01-21 21:49 UTC (permalink / raw)
To: David Heidelberg
Cc: Rob Clark, Sean Paul, Abhinav Kumar, David Airlie, Daniel Vetter,
Andy Gross, Bjorn Andersson, Kishon Vijay Abraham I, Vinod Koul,
~okias/devicetree, linux-arm-msm, dri-devel, freedreno,
devicetree, linux-kernel, linux-phy
In-Reply-To: <20220109000348.106534-1-david@ixit.cz>
On Sun, Jan 09, 2022 at 01:03:47AM +0100, David Heidelberg wrote:
> Convert Qualcomm HDMI binding into HDMI TX and PHY yaml bindings.
>
> Other changes:
> - fixed reg-names numbering to match 0..3 instead 0,1,3,4
> - phy part moved into phy/ directory
>
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---
> v2:
> - move phy into phy/
> - added maxItems for gpios
> - simplified pinctrl-names
> - dropped some inconsistent quotes
>
> v3:
> - adjusted $id of phy file to the new path from v2
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---
> .../devicetree/bindings/display/msm/hdmi.txt | 99 ---------
> .../bindings/display/msm/qcom,hdmi.yaml | 206 ++++++++++++++++++
> .../bindings/phy/qcom,hdmi-phy.yaml | 119 ++++++++++
> 3 files changed, 325 insertions(+), 99 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/display/msm/hdmi.txt
> create mode 100644 Documentation/devicetree/bindings/display/msm/qcom,hdmi.yaml
> create mode 100644 Documentation/devicetree/bindings/phy/qcom,hdmi-phy.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/msm/hdmi.txt b/Documentation/devicetree/bindings/display/msm/hdmi.txt
> deleted file mode 100644
> index 5f90a40da51b..000000000000
> --- a/Documentation/devicetree/bindings/display/msm/hdmi.txt
> +++ /dev/null
> @@ -1,99 +0,0 @@
> -Qualcomm adreno/snapdragon hdmi output
> -
> -Required properties:
> -- compatible: one of the following
> - * "qcom,hdmi-tx-8996"
> - * "qcom,hdmi-tx-8994"
> - * "qcom,hdmi-tx-8084"
> - * "qcom,hdmi-tx-8974"
> - * "qcom,hdmi-tx-8660"
> - * "qcom,hdmi-tx-8960"
> -- reg: Physical base address and length of the controller's registers
> -- reg-names: "core_physical"
> -- interrupts: The interrupt signal from the hdmi block.
> -- power-domains: Should be <&mmcc MDSS_GDSC>.
> -- clocks: device clocks
> - See ../clocks/clock-bindings.txt for details.
> -- core-vdda-supply: phandle to supply regulator
> -- hdmi-mux-supply: phandle to mux regulator
> -- phys: the phandle for the HDMI PHY device
> -- phy-names: the name of the corresponding PHY device
> -
> -Optional properties:
> -- hpd-gpios: hpd pin
> -- qcom,hdmi-tx-mux-en-gpios: hdmi mux enable pin
> -- qcom,hdmi-tx-mux-sel-gpios: hdmi mux select pin
> -- qcom,hdmi-tx-mux-lpm-gpios: hdmi mux lpm pin
> -- power-domains: reference to the power domain(s), if available.
> -- pinctrl-names: the pin control state names; should contain "default"
> -- pinctrl-0: the default pinctrl state (active)
> -- pinctrl-1: the "sleep" pinctrl state
> -
> -HDMI PHY:
> -Required properties:
> -- compatible: Could be the following
> - * "qcom,hdmi-phy-8660"
> - * "qcom,hdmi-phy-8960"
> - * "qcom,hdmi-phy-8974"
> - * "qcom,hdmi-phy-8084"
> - * "qcom,hdmi-phy-8996"
> -- #phy-cells: Number of cells in a PHY specifier; Should be 0.
> -- reg: Physical base address and length of the registers of the PHY sub blocks.
> -- reg-names: The names of register regions. The following regions are required:
> - * "hdmi_phy"
> - * "hdmi_pll"
> - For HDMI PHY on msm8996, these additional register regions are required:
> - * "hdmi_tx_l0"
> - * "hdmi_tx_l1"
> - * "hdmi_tx_l3"
> - * "hdmi_tx_l4"
> -- power-domains: Should be <&mmcc MDSS_GDSC>.
> -- clocks: device clocks
> - See Documentation/devicetree/bindings/clock/clock-bindings.txt for details.
> -- core-vdda-supply: phandle to vdda regulator device node
> -
> -Example:
> -
> -/ {
> - ...
> -
> - hdmi: hdmi@4a00000 {
> - compatible = "qcom,hdmi-tx-8960";
> - reg-names = "core_physical";
> - reg = <0x04a00000 0x2f0>;
> - interrupts = <GIC_SPI 79 0>;
> - power-domains = <&mmcc MDSS_GDSC>;
> - clock-names =
> - "core",
> - "master_iface",
> - "slave_iface";
> - clocks =
> - <&mmcc HDMI_APP_CLK>,
> - <&mmcc HDMI_M_AHB_CLK>,
> - <&mmcc HDMI_S_AHB_CLK>;
> - qcom,hdmi-tx-ddc-clk = <&msmgpio 70 GPIO_ACTIVE_HIGH>;
> - qcom,hdmi-tx-ddc-data = <&msmgpio 71 GPIO_ACTIVE_HIGH>;
> - qcom,hdmi-tx-hpd = <&msmgpio 72 GPIO_ACTIVE_HIGH>;
> - core-vdda-supply = <&pm8921_hdmi_mvs>;
> - hdmi-mux-supply = <&ext_3p3v>;
> - pinctrl-names = "default", "sleep";
> - pinctrl-0 = <&hpd_active &ddc_active &cec_active>;
> - pinctrl-1 = <&hpd_suspend &ddc_suspend &cec_suspend>;
> -
> - phys = <&hdmi_phy>;
> - phy-names = "hdmi_phy";
> - };
> -
> - hdmi_phy: phy@4a00400 {
> - compatible = "qcom,hdmi-phy-8960";
> - reg-names = "hdmi_phy",
> - "hdmi_pll";
> - reg = <0x4a00400 0x60>,
> - <0x4a00500 0x100>;
> - #phy-cells = <0>;
> - power-domains = <&mmcc MDSS_GDSC>;
> - clock-names = "slave_iface";
> - clocks = <&mmcc HDMI_S_AHB_CLK>;
> - core-vdda-supply = <&pm8921_hdmi_mvs>;
> - };
> -};
> diff --git a/Documentation/devicetree/bindings/display/msm/qcom,hdmi.yaml b/Documentation/devicetree/bindings/display/msm/qcom,hdmi.yaml
> new file mode 100644
> index 000000000000..33ebc879af93
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/msm/qcom,hdmi.yaml
> @@ -0,0 +1,206 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: "http://devicetree.org/schemas/display/msm/qcom,hdmi.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Qualcomm Adreno/Snapdragon HDMI output
> +
> +maintainers:
> + - Rob Clark <robdclark@gmail.com>
> +
> +allOf:
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: qcom,hdmi-tx-8996
> + then:
> + properties:
> + clocks:
> + minItems: 5
> + maxItems: 5
> +
> + clock-names:
> + items:
> + - const: mdp_core
> + - const: iface
> + - const: core
> + - const: alt_iface
> + - const: extp
> + else:
> + properties:
> + clocks:
> + minItems: 1
> + maxItems: 5
> +
> + clock-names:
> + minItems: 1
> + maxItems: 5
So for !8996 we have a 1-5 undefined clocks? Though like all the other
messes I imagine there's 5 different variations.
> +
> +
> +properties:
> + compatible:
> + enum:
> + - qcom,hdmi-tx-8996
> + - qcom,hdmi-tx-8994
> + - qcom,hdmi-tx-8084
> + - qcom,hdmi-tx-8974
> + - qcom,hdmi-tx-8660
> + - qcom,hdmi-tx-8960
> +
> + clocks: true
> +
> + clock-names: true
> +
> + reg:
> + minItems: 1
> + maxItems: 3
> + description: Physical base address and length of the controller's registers
> +
> + reg-names:
> + minItems: 1
> + items:
> + - const: core_physical
> + - const: qfprom_physical
> + - const: hdcp_physical
> +
> + interrupts:
> + maxItems: 1
> + description: The interrupt signal from the hdmi block.
Don't need a generic description.
> +
> + power-domains:
> + description: should be <&mmcc MDSS_GDSC>
Drop. Should be 'maxItems: 1'.
> +
> + core-vdda-supply: true
> +
> + core-vcc-supply: true
> +
> + hdmi-mux-supply:
> + description: phandle to mux regulator
> +
> + phys:
> + description: the phandle for the HDMI PHY device
How many?
> +
> + phy-names:
> + description: the name of the corresponding PHY device
names?
> +
> + hpd-gpios:
> + maxItems: 1
> + description: hpd pin
> +
> + qcom,hdmi-tx-ddc-clk-gpios:
> + maxItems: 1
> + description: HDMI DDC clock
> +
> + qcom,hdmi-tx-ddc-data-gpios:
> + maxItems: 1
> + description: HDMI DDC data
> +
> + qcom,hdmi-tx-mux-en-gpios:
> + maxItems: 1
> + description: HDMI mux enable pin
> +
> + qcom,hdmi-tx-mux-sel-gpios:
> + maxItems: 1
> + description: HDMI mux select pin
> +
> + qcom,hdmi-tx-mux-lpm-gpios:
> + maxItems: 1
> + description: HDMI mux lpm pin
> +
> + pinctrl-0: true
> + pinctrl-1: true
> +
> + pinctrl-names:
> + minItems: 1
> + items:
> + - const: default
> + - const: sleep
> +
> + '#phy-cells':
> + const: 0
The HDMI block is a phy provider?
> +
> + '#sound-dai-cells':
> + const: 1
> +
> + ports:
> + type: object
> + $ref: /schemas/graph.yaml#/properties/ports
> + properties:
> + port@0:
> + $ref: /schemas/graph.yaml#/$defs/port-base
> + unevaluatedProperties: false
> + description: |
> + Input endpoints of the controller.
> + properties:
> + endpoint:
> + $ref: /schemas/media/video-interfaces.yaml#
> + unevaluatedProperties: false
> + properties:
> + data-lanes:
> + maxItems: 4
> + minItems: 4
> + items:
> + enum: [0, 1, 2, 3]
> +
> + port@1:
> + $ref: /schemas/graph.yaml#/$defs/port-base
> + unevaluatedProperties: false
> + description: |
> + Output endpoints of the controller.
> + properties:
> + endpoint:
> + $ref: /schemas/media/video-interfaces.yaml#
> + unevaluatedProperties: false
> + properties:
> + data-lanes:
> + maxItems: 4
> + minItems: 4
> + items:
> + enum: [0, 1, 2, 3]
> +
> + required:
> + - port@0
> +
> +required:
> + - compatible
> + - clocks
> + - clock-names
> + - reg
> + - reg-names
> + - interrupts
> + - phys
> + - phy-names
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + hdmi: hdmi@4a00000 {
> + compatible = "qcom,hdmi-tx-8960";
> + reg-names = "core_physical";
> + reg = <0x04a00000 0x2f0>;
> + interrupts = <0 79 0>;
> + power-domains = <&mmcc 1>;
> + clock-names =
> + "core",
> + "master_iface",
> + "slave_iface";
> + clocks =
> + <&clk 61>,
> + <&clk 72>,
> + <&clk 98>;
> + qcom,hdmi-tx-ddc-clk-gpios = <&msmgpio 70 0>;
> + qcom,hdmi-tx-ddc-data-gpios = <&msmgpio 71 0>;
> + hpd-gpios = <&msmgpio 72 0>;
> + core-vdda-supply = <&pm8921_hdmi_mvs>;
> + hdmi-mux-supply = <&ext_3p3v>;
> + pinctrl-names = "default", "sleep";
> + pinctrl-0 = <&hpd_active &ddc_active &cec_active>;
> + pinctrl-1 = <&hpd_suspend &ddc_suspend &cec_suspend>;
> +
> + phys = <&hdmi_phy>;
> + phy-names = "hdmi_phy";
> + };
> diff --git a/Documentation/devicetree/bindings/phy/qcom,hdmi-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,hdmi-phy.yaml
> new file mode 100644
> index 000000000000..1203b0c6709f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qcom,hdmi-phy.yaml
> @@ -0,0 +1,119 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: "http://devicetree.org/schemas/phy/qcom,hdmi-phy.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Qualcomm Adreno/Snapdragon HDMI phy
> +
> +maintainers:
> + - Rob Clark <robdclark@gmail.com>
> +
> +allOf:
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: qcom,hdmi-phy-8996
> + then:
> + properties:
> + reg:
> + minItems: 6
> + maxItems: 6
> +
> + reg-names:
> + items:
> + - const: hdmi_pll
> + - const: hdmi_tx_l0
> + - const: hdmi_tx_l1
> + - const: hdmi_tx_l2
> + - const: hdmi_tx_l3
> + - const: hdmi_phy
> +
> + else:
> + properties:
> + reg:
> + minItems: 2
> + maxItems: 2
> +
> + reg-names:
> + items:
> + - const: hdmi_phy
> + - const: hdmi_pll
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: qcom,hdmi-phy-8960
> + then:
> + properties:
> + clock-names:
> + const: slave_iface
> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: qcom,hdmi-phy-8996
> + then:
> + properties:
> + clock-names:
> + items:
> + - const: iface
> + - const: ref
> +
> +properties:
> + compatible:
> + contains:
> + enum:
> + - qcom,hdmi-phy-8084
> + - qcom,hdmi-phy-8660
> + - qcom,hdmi-phy-8960
> + - qcom,hdmi-phy-8974
> + - qcom,hdmi-phy-8994
> + - qcom,hdmi-phy-8996
> +
> + reg: true
> +
> + reg-names: true
> +
> + clocks: true
> +
> + clock-names: true
> +
> + power-domains:
> + maxItems: 1
> +
> + core-vdda-supply: true
> +
> + vcca-supply: true
> +
> + vddio-supply: true
> +
> + '#phy-cells':
> + const: 0
> +
> +required:
> + - compatible
> + - clocks
> + - reg
> + - reg-names
> + - '#phy-cells'
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + hdmi_phy: phy@4a00400 {
> + compatible = "qcom,hdmi-phy-8960";
> + reg-names = "hdmi_phy",
> + "hdmi_pll";
> + reg = <0x4a00400 0x60>,
> + <0x4a00500 0x100>;
> + #phy-cells = <0>;
> + power-domains = <&mmcc 1>;
> + clock-names = "slave_iface";
> + clocks = <&clk 21>;
> + core-vdda-supply = <&pm8921_hdmi_mvs>;
> + };
> --
> 2.34.1
>
>
^ permalink raw reply
* Re: [PATCH v5 2/4] dt-bindings: input: Add Cypress TT2100 touchscreen controller
From: Rob Herring @ 2022-01-21 22:08 UTC (permalink / raw)
To: Alistair Francis
Cc: devicetree, linux-kernel, linux-input, linux-arm-kernel,
dmitry.torokhov, rydberg, andreas, linus.walleij, alistair23,
Mylène Josserand
In-Reply-To: <20220109115331.388633-3-alistair@alistair23.me>
On Sun, Jan 09, 2022 at 09:53:29PM +1000, Alistair Francis wrote:
> From: Mylène Josserand <mylene.josserand@free-electrons.com>
>
> Add the Cypress TrueTouch Generation 5 touchscreen device tree bindings
> documentation. It can use I2C or SPI bus.
> This touchscreen can handle some defined zone that are designed and
> sent as button. To be able to customize the keycode sent, the
> "linux,code" property in a "button" sub-node can be used.
>
> Signed-off-by: Mylène Josserand <mylene.josserand@free-electrons.com>
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> ---
> .../input/touchscreen/cypress,tt21000.yaml | 92 +++++++++++++++++++
> 1 file changed, 92 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.yaml
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.yaml b/Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.yaml
> new file mode 100644
> index 000000000000..acd2d9389f8c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/cypress,tt21000.yaml
> @@ -0,0 +1,92 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/touchscreen/cypress,tt21000.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cypress TT2100 touchscreen controller
> +
> +description: The Cypress TT2100 series (also known as "CYTTSP5" after
> + the marketing name Cypress TrueTouch Standard Product series 5).
> +
> +maintainers:
> + - Alistair Francis <alistair@alistair23.me>
> +
> +allOf:
> + - $ref: touchscreen.yaml#
> +
> +properties:
> + compatible:
> + const: cypress,tt21000
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + vdd-supply:
> + description: Regulator for voltage.
> +
> + reset-gpios:
> + maxItems: 1
> +
> + linux,code:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: EV_ABS specific event code generated by the axis.
> +
> +patternProperties:
> + "^button-[0-9]+$":
Where does the numbering come from? I assume it correlates to something
in the h/w. If so, I think you should use 'reg' and a unit-address here.
Generally, we don't want the node names to be important (i.e. used by
the OS).
> + type: object
> + properties:
> + linux,code:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: Keycode to emit
> +
> + required:
> + - linux,code
> +
> + additionalProperties: false
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - vdd-supply
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + #include <dt-bindings/gpio/gpio.h>
> + #include <dt-bindings/input/linux-event-codes.h>
> +
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + touchscreen@24 {
> + compatible = "cypress,tt21000";
> + reg = <0x24>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&tp_reset_ds203>;
> + interrupt-parent = <&pio>;
> + interrupts = <1 5 IRQ_TYPE_LEVEL_LOW>;
> + reset-gpios = <&pio 7 1 GPIO_ACTIVE_LOW>;
> + vdd-supply = <®_touch>;
> +
> + button-0 {
> + linux,code = <KEY_HOMEPAGE>;
> + };
> +
> + button-1 {
> + linux,code = <KEY_MENU>;
> + };
> +
> + button-2 {
> + linux,code = <KEY_BACK>;
> + };
> + };
> + };
> +...
> --
> 2.31.1
>
>
^ permalink raw reply
* Re: [EXT] Re: [PATCH v8 04/15] media:Add v4l2 event codec_error and skip
From: Nicolas Dufresne @ 2022-01-21 22:25 UTC (permalink / raw)
To: Ming Qian, mchehab@kernel.org, shawnguo@kernel.org,
robh+dt@kernel.org, s.hauer@pengutronix.de
Cc: hverkuil-cisco@xs4all.nl, kernel@pengutronix.de,
festevam@gmail.com, dl-linux-imx, linux-media@vger.kernel.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <AM6PR04MB63417E126287421BCA133514E7539@AM6PR04MB6341.eurprd04.prod.outlook.com>
Le jeudi 13 janvier 2022 à 07:18 +0000, Ming Qian a écrit :
> Hi Nicolas,
>
> I have question about skip event or similar concepts.
> If the client control the input frame count, and it won't queue any more frames unless some frame is decoded.
> But after seek, There is no requirement to begin queuing coded data starting exactly from a resume point (e.g. SPS or a keyframe). Any queued OUTPUT buffers will be processed and returned to the client until a suitable resume point is found. While looking for a resume point, the decoder should not produce any decoded frames into CAPTURE buffers.
>
> So client may have queued some frames but without any resume point, in this case the decoder won't produce any decoded frames into CAPTURE buffers, and the client won't queue frames into output buffers. This creates some kind of deadlock.
>
> In our previous solution, we send skip event to client to tell it that some frame is skipped instead of decoded, then the client can continue to queue frames.
> But the skip event is flawed, so we need some solution to resolve it.
> 1. decoder can produce an empty buffer with V4L2_BUF_FLAG_SKIPPED (or V4L2_BUF_FLAG_ERROR) as you advised, but this seems to conflict with the above description in specification.
> 2. Define a notification mechanism to notify the client
>
> Can you give some advice? This constraint of frame depth is common on android
Without going against the spec, you can as of today pop a capture buffer and
mark it done with error. As it has nothing valid in it, I would also set the
payload size to 0.
So I'd say, for every unique input timestamp, that didn't yield a frame
(skipped), pop a capture buffer, copy the timestamp, set the payload size to 0
and set it as done with error.
I'm not sure though if we that we can specify this, as I'm not sure this is
possible with all the existing HW. I must admit, I don't myself had to deal with
that issue as I'm not using a dummy framework. In GStreamer, we take care of
locating the next sync point. So unless there was an error in the framework,
this case does not exist for us.
>
> Ming
>
> > > > > + * - ``V4L2_EVENT_SKIP``
> > > > > + - 8
> > > > > + - This event is triggered when one frame is decoded, but it
> > > > > + won't
> > > > > be
> > > > outputed
> > > > > + to the display. So the application can't get this frame, and
> > > > > + the
> > > > > input
> > > > frame count
> > > > > + is dismatch with the output frame count. And this evevt is
> > > > > + telling
> > > > > the
> > > > client to
> > > > > + handle this case.
> > > >
> > > > Similar to my previous comment, this event is flawed, since
> > > > userspace cannot know were the skip is located in the queued
> > > > buffers. Currently, all decoders are mandated to support
> > > > V4L2_BUF_FLAG_TIMESTAMP_COPY. The timestamp must NOT be
> > interpreted
> > > > by the driver and must be reproduce as-is in the associated CAPTURE
> > > > buffer. It is possible to "garbage" collect skipped frames with this
> > > > method, though tedious.
> > > >
> > > > An alternative, and I think it would be much nicer then this, would
> > > > be to use the v4l2_buffer.sequence counter, and just make it skip 1
> > > > on skips. Though, the down side is that userspace must also know how
> > > > to reorder frames (a driver job for stateless codecs) in order to
> > > > identify which frame was skipped. So this is perhaps not that
> > > > useful, other then knowing something was skipped in the past.
> > > >
> > > > A third option would be to introduce V4L2_BUF_FLAG_SKIPPED. This way
> > > > the driver could return an empty payload (bytesused = 0) buffer with
> > > > this flag set, and the proper timestamp properly copied. This would
> > > > let the driver communicate skipped frames in real-time. Note that
> > > > this could break with existing userspace, so it would need to be
> > > > opted-in somehow (a control or some flags).
> > >
> > > Hi Nicolas,
> > > The problem we meet is that userspace doesn't care which frame is
> > > skipped, it just need to know that there are a frame is skipped, the
> > > driver should promise the input frame count is equals to the output frame
> > count.
> > > Your first method is possible in theory, but we find the timestamp
> > > may be unreliable, we meet many timestamp issues that userspace may
> > > enqueue invalid timestamp or repeated timestamp and so on, so we can't
> > accept this solution.
> >
> > The driver should not interpret the provided timestamp, so it should not be
> > able to say if the timestamp is valid or not, this is not the driver's task.
> >
> > The driver task is to match the timestamp to the CAPTURE buffer (if that buffer
> > was produced), and reproduce it exactly.
> >
> > > I think your second option is better. And there are only 1
> > > question, we find some application prefer to use the V4L2_EVENT_EOS to
> > > check the eos, not checking the empty buffer, if we use this method to
> > > check skipped frame, the
> >
> > Checking the empty buffer is a legacy method, only available in Samsung MFC
> > driver. The spec says that the last buffer should be flagged with _LAST, and any
> > further attempt to poll should unblock and DQBUF return EPIPE.
> >
> > > application should check empty buffer instead of V4L2_EVENT_EOS,
> > > otherwise if the last frame is skipped, the application will miss it.
> > > Of course this is not a problem, it just increases the complexity of
> > > the userspace implementation
> >
> > The EPIPE mechanism covers this issue, which we initially had with the LAST
> > flag.
> >
> > > I don't think your third method is feasible, the reasons are as below
> > > 1. usually the empty payload means eos, and as you say,
> > > it may introduce confusion.
> > > 2. The driver may not have the opportunity to return an empty
> > > payload during decoding, in our driver, driver will pass the capture
> > > buffer to firmware, and when some frame is skipped, the firmware won't
> > > return the buffer, driver may not find an available capture buffer to
> > > return to userspace.
> > >
> > > The requirement is that userspace need to match the input frame
> > > count and output frame count. It doesn't care which frame is skipped,
> > > so the V4L2_EVENT_SKIP is the easiest way for driver and userspace.
> > > If you think this event is really inappropriate, I prefer to adopt
> > > your second option
> >
> > Please, drop SKIP from you driver and this patchset and fix your draining
> > process handling to follow the spec. The Samsung OMX component is
> > irrelevant to mainline submission, the OMX code should be updated to follow
> > the spec.
> >
> > >
^ permalink raw reply
* Re: [PATCH v5 1/4] Input: Add driver for Cypress Generation 5 touchscreen
From: Rob Herring @ 2022-01-21 22:43 UTC (permalink / raw)
To: Peter Geis
Cc: alistair, alistair23, andreas, devicetree, dmitry.torokhov,
linus.walleij, linux-arm-kernel, linux-input, linux-kernel,
maxime.ripard, mylene.josserand, rydberg
In-Reply-To: <20220119012637.1713748-1-pgwipeout@gmail.com>
On Tue, Jan 18, 2022 at 08:26:37PM -0500, Peter Geis wrote:
> Good Evening,
>
> I tried using this with a cypress tma448 and thought there was an issue
> with the driver.
> Further investigation found the tma448 in question had no configuration
> burned into it, thus max size and pressure are zero.
>
> I've added the patch below to extend the driver with device tree
> overrides.
> Unfortunately device_property_read_u16 is broken on arm64 currently, as
> it returns zero.
FYI, I replied to your bug report:
https://lore.kernel.org/all/CAMdYzYquceSBrOsvO8rW9wmJA_RO=HSwv_waVoS=0hsP414T-A@mail.gmail.com/
Rob
^ permalink raw reply
* Re: [RFC PATCH v2 5/7] ARM: dts: bcm2711: Add unicam CSI nodes
From: Laurent Pinchart @ 2022-01-21 22:45 UTC (permalink / raw)
To: Jean-Michel Hautbois
Cc: dave.stevenson, devicetree, kernel-list, linux-arm-kernel,
linux-kernel, linux-media, linux-rpi-kernel, lukasz, mchehab,
naush, robh, tomi.valkeinen
In-Reply-To: <20220121081810.155500-6-jeanmichel.hautbois@ideasonboard.com>
Hi Jean-Michel,
Thank you for the patch.
On Fri, Jan 21, 2022 at 09:18:08AM +0100, Jean-Michel Hautbois wrote:
> Add both MIPI CSI-2 nodes in the core bcm2711 tree. Use the 3-cells
> interrupt declaration, corresponding clocks and default as disabled.
>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
> arch/arm/boot/dts/bcm2711.dtsi | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
> diff --git a/arch/arm/boot/dts/bcm2711.dtsi b/arch/arm/boot/dts/bcm2711.dtsi
> index dff18fc9a906..077141df7024 100644
> --- a/arch/arm/boot/dts/bcm2711.dtsi
> +++ b/arch/arm/boot/dts/bcm2711.dtsi
> @@ -3,6 +3,7 @@
>
> #include <dt-bindings/interrupt-controller/arm-gic.h>
> #include <dt-bindings/soc/bcm2835-pm.h>
> +#include <dt-bindings/power/raspberrypi-power.h>
>
> / {
> compatible = "brcm,bcm2711";
> @@ -293,6 +294,36 @@ hvs: hvs@7e400000 {
> interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
> };
>
> + csi0: csi1@7e800000 {
The node name should be csi@7e800000, not csi1@7e800000. Now, this will
probably cause issues with the firmware that looks for csi1 (and csi0 ?)
to hand over control of the Unicam CSI-2 receiver to the kernel. I
wonder if this is something that could be handled by a firmware update,
to also recognize nodes named "csi" ?
> + compatible = "brcm,bcm2835-unicam";
> + reg = <0x7e800000 0x800>,
> + <0x7e802000 0x4>;
> + interrupts = <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&clocks BCM2835_CLOCK_CAM0>,
> + <&firmware_clocks 4>;
> + clock-names = "lp", "vpu";
> + power-domains = <&power RPI_POWER_DOMAIN_UNICAM0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + #clock-cells = <1>;
Why do you need #address-cells, #size-cells and #clock-cells ? They're
not mentioned in the binding.
> + status="disabled";
Missing spaces around the =.
Same comment for the next node.
> + };
> +
> + csi1: csi1@7e801000 {
> + compatible = "brcm,bcm2835-unicam";
> + reg = <0x7e801000 0x800>,
> + <0x7e802004 0x4>;
> + interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&clocks BCM2835_CLOCK_CAM1>,
> + <&firmware_clocks 4>;
> + clock-names = "lp", "vpu";
> + power-domains = <&power RPI_POWER_DOMAIN_UNICAM1>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + #clock-cells = <1>;
> + status="disabled";
> + };
> +
> pixelvalve3: pixelvalve@7ec12000 {
> compatible = "brcm,bcm2711-pixelvalve3";
> reg = <0x7ec12000 0x100>;
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v3 4/6] dt-bindings: interrupt-controller: realtek,rtl-intc: require parents
From: Rob Herring @ 2022-01-21 22:56 UTC (permalink / raw)
To: Sander Vanheule
Cc: linux-kernel, devicetree, Thomas Gleixner, Marc Zyngier,
Birger Koblitz, Bert Vermeulen, John Crispin
In-Reply-To: <e043a9faa4a8f71efdf8b7849ec7911f16207fb0.1641739718.git.sander@svanheule.net>
On Sun, Jan 09, 2022 at 03:54:35PM +0100, Sander Vanheule wrote:
> The interrupt router has 32 inputs and up to 15 outputs, and the way
> these are mapped to each other is runtime configurable. The outputs of
> this interrupt router on the other hand, are connected to a fixed set of
> parent interrupts. This means that "interrupt-map" is inappropriate, and
> rather a list of parent interrupts should be specified.
I'm not sure why interrupt-map is not appropriate. It is not appropriate
if you have to touch the interrupt router h/w in servicing the
interrupts. If you just need one time configuration of the mapping, then
it should be fine to use I think.
> Two-part compatibles are introduced to be able to require "interrupts"
> for new devicetrees. The relevant descriptions are extended or added to
> more clearly describe the inputs and outputs of this router. The old
> compatible, "interrupt-map" and "#address-cells", is deprecated.
> Interrupt specifiers for new compatibles will require two cells, to
> indicate the output selection.
>
> To prevent spurious changes when more SoCs are added, "allOf" is used
> with one "if", and the compatible enum only has one item.
>
> The example is updated to provide a correct example for RTL8380 SoCs.
>
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> ---
> .../realtek,rtl-intc.yaml | 78 ++++++++++++++-----
> 1 file changed, 58 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
> index 9e76fff20323..aab8d44010af 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
> +++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
> @@ -6,6 +6,10 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
>
> title: Realtek RTL SoC interrupt controller devicetree bindings
>
> +description:
> + Interrupt router for Realtek MIPS SoCs, allowing each SoC interrupt to be
> + routed to one parent interrupt, or left disconnected.
> +
> maintainers:
> - Birger Koblitz <mail@birger-koblitz.de>
> - Bert Vermeulen <bert@biot.com>
> @@ -13,45 +17,79 @@ maintainers:
>
> properties:
> compatible:
> - const: realtek,rtl-intc
> + oneOf:
> + - items:
> + - enum:
> + - realtek,rtl8380-intc
> + - const: realtek,rtl-intc
> + - const: realtek,rtl-intc
> + deprecated: true
>
> - "#interrupt-cells":
> - const: 1
> + "#interrupt-cells": true
>
> reg:
> maxItems: 1
>
> interrupts:
> - maxItems: 1
> + minItems: 1
> + maxItems: 15
> + description:
> + List of parent interrupts, in the order that they are connected to this
> + interrupt router's outputs.
>
> interrupt-controller: true
>
> - "#address-cells":
> - const: 0
> -
> - interrupt-map:
> - description: Describes mapping from SoC interrupts to CPU interrupts
> -
> required:
> - compatible
> - reg
> - "#interrupt-cells"
> - interrupt-controller
> - - "#address-cells"
> - - interrupt-map
> +
> +allOf:
> + - if:
> + properties:
> + compatible:
> + const: realtek,rtl-intc
> + then:
> + properties:
> + "#interrupt-cells":
> + const: 1
> +
> + "#address-cells":
> + const: 0
> +
> + interrupt-map: true
> + required:
> + - "#address-cells"
> + - interrupt-map
> + else:
> + properties:
> + "#interrupt-cells":
> + description:
> + Two cells to specify which line to connect to, and which output it should
> + be routed to. Both cells use a zero-based index.
Picking the index picks the priority? Which is higher priority?
> + const: 2
> + required:
> + - interrupts
>
> additionalProperties: false
>
> examples:
> - |
> intc: interrupt-controller@3000 {
> - compatible = "realtek,rtl-intc";
> - #interrupt-cells = <1>;
> + compatible = "realtek,rtl8380-intc", "realtek,rtl-intc";
> + #interrupt-cells = <2>;
> interrupt-controller;
> - reg = <0x3000 0x20>;
> - #address-cells = <0>;
> - interrupt-map =
> - <31 &cpuintc 2>,
> - <30 &cpuintc 1>,
> - <29 &cpuintc 5>;
> + reg = <0x3000 0x18>;
> +
> + interrupt-parent = <&cpuintc>;
> + interrupts = <2>, <3>, <4>, <5>, <6>;
> + };
> +
> + irq-consumer@0 {
> + reg = <0 4>;
> + interrupt-parent = <&intc>;
> + interrupts =
> + <19 3>, /* IRQ 19, routed to output 3 (cpuintc 5) */
> + <18 4>; /* IRQ 18, routed to output 4 (cpuintc 6) */
> };
> --
> 2.33.1
>
>
^ permalink raw reply
* Re: [PATCH v2] dt-bindings: serial: Convert rda,8810pl-uart to YAML
From: Rob Herring @ 2022-01-21 22:59 UTC (permalink / raw)
To: Stanislav Jakubek
Cc: linux-arm-kernel, Manivannan Sadhasivam, devicetree, Rob Herring,
linux-unisoc
In-Reply-To: <20220109170321.GA12989@standask-GA-A55M-S2HP>
On Sun, 09 Jan 2022 18:03:21 +0100, Stanislav Jakubek wrote:
> Convert RDA Micro UART bindings to DT schema format.
>
> Signed-off-by: Stanislav Jakubek <stano.jakubek@gmail.com>
> ---
> Changes in v2:
> - Fix indentation in example
>
> .../bindings/serial/rda,8810pl-uart.txt | 17 -------
> .../bindings/serial/rda,8810pl-uart.yaml | 46 +++++++++++++++++++
> MAINTAINERS | 2 +-
> 3 files changed, 47 insertions(+), 18 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/serial/rda,8810pl-uart.txt
> create mode 100644 Documentation/devicetree/bindings/serial/rda,8810pl-uart.yaml
>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH] dt-bindings: msm/mdp4: convert to yaml format
From: Rob Herring @ 2022-01-21 23:05 UTC (permalink / raw)
To: David Heidelberg
Cc: Daniel Vetter, Sean Paul, ~okias/devicetree, David Airlie,
Rob Herring, linux-arm-msm, linux-kernel, devicetree, dri-devel,
Rob Clark, Abhinav Kumar, freedreno
In-Reply-To: <20220109171814.16103-1-david@ixit.cz>
On Sun, 09 Jan 2022 18:18:13 +0100, David Heidelberg wrote:
> Convert mdp4 binding into yaml format.
>
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---
> .../devicetree/bindings/display/msm/mdp4.txt | 114 ----------------
> .../devicetree/bindings/display/msm/mdp4.yaml | 124 ++++++++++++++++++
> 2 files changed, 124 insertions(+), 114 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/display/msm/mdp4.txt
> create mode 100644 Documentation/devicetree/bindings/display/msm/mdp4.yaml
>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH 05/10] dt-bindings: soc: qcom: Update devicetree binding document for rpmh-rsc
From: Rob Herring @ 2022-01-21 23:06 UTC (permalink / raw)
To: Maulik Shah
Cc: quic_lsrao, linux-arm-msm, linux-pm, daniel.lezcano, ulf.hansson,
quic_rjendra, devicetree, rafael, linux-kernel, bjorn.andersson
In-Reply-To: <1641749107-31979-6-git-send-email-quic_mkshah@quicinc.com>
On Sun, 09 Jan 2022 22:55:02 +0530, Maulik Shah wrote:
> The change documents power-domains property for RSC device.
> This optional property points to corresponding PM domain node.
>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Maulik Shah <quic_mkshah@quicinc.com>
> ---
> Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt | 6 ++++++
> 1 file changed, 6 insertions(+)
>
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v4 4/9] dt-bindings: pinctrl: Add Nuvoton WPCM450
From: Rob Herring @ 2022-01-21 23:08 UTC (permalink / raw)
To: Jonathan Neuschäfer
Cc: Andy Shevchenko, devicetree, Nancy Yuen, Tomer Maimon, Tali Perry,
Linus Walleij, Rob Herring, Joel Stanley, linux-kernel,
Benjamin Fair, Patrick Venture, linux-gpio, Avi Fishman, openbmc
In-Reply-To: <20220109173000.1242703-5-j.neuschaefer@gmx.net>
On Sun, 09 Jan 2022 18:29:55 +0100, Jonathan Neuschäfer wrote:
> This binding is heavily based on the one for NPCM7xx, because the
> hardware is similar. There are some notable differences, however:
>
> - The addresses of GPIO banks are not physical addresses but simple
> indices (0 to 7), because the GPIO registers are not laid out in
> convenient blocks.
> - Pinmux settings can explicitly specify that the GPIO mode is used.
>
> Certain pins support blink patterns in hardware. This is currently not
> modelled in the DT binding.
>
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
>
> ---
> v4:
> - Small improvements around gpio node addresses, suggested by Rob Herring
>
> v3:
> - https://lore.kernel.org/lkml/20211224200935.93817-5-j.neuschaefer@gmx.net/
> - Make changes suggested by Rob Herring
> - Fix lint errors
> - Simplify child node patterns
> - Remove if/type=object/then trick
> - Reduce interrupts.maxItems to 3: 4 aren't necessary
> - Replace list of gpio0/1/2/etc. with pattern
> - Remove nuvoton,interrupt-map again, to simplify the binding
> - Make tuples clearer
>
> v2:
> - https://lore.kernel.org/lkml/20211207210823.1975632-5-j.neuschaefer@gmx.net/
> - Move GPIO into subnodes
> - Improve use of quotes
> - Remove unnecessary minItems/maxItems lines
> - Remove "phandle: true"
> - Use separate prefixes for pinmux and pincfg nodes
> - Add nuvoton,interrupt-map property
> - Make it possible to set pinmux to GPIO explicitly
>
> v1:
> - https://lore.kernel.org/lkml/20210602120329.2444672-5-j.neuschaefer@gmx.net/
> ---
> .../pinctrl/nuvoton,wpcm450-pinctrl.yaml | 160 ++++++++++++++++++
> 1 file changed, 160 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.yaml
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [RFC PATCH v2 3/7] media: dt-bindings: media: Add bindings for bcm2835-unicam
From: Laurent Pinchart @ 2022-01-21 23:27 UTC (permalink / raw)
To: Jean-Michel Hautbois
Cc: dave.stevenson, devicetree, kernel-list, linux-arm-kernel,
linux-kernel, linux-media, linux-rpi-kernel, lukasz, mchehab,
naush, robh, tomi.valkeinen
In-Reply-To: <20220121081810.155500-4-jeanmichel.hautbois@ideasonboard.com>
Hi Jean-Michel,
Thank you for the patch.
On Fri, Jan 21, 2022 at 09:18:06AM +0100, Jean-Michel Hautbois wrote:
> Introduce the dt-bindinds documentation for bcm2835 CCP2/CSI2 camera
s/bindinds/bindings/
I'd mention "Unicam" somewhere here.
> interface. Also add a MAINTAINERS entry for it.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
> Dave: I assumed you were the maintainer for this file, as I based it on the
> bcm2835-unicam.txt file. Are you happy to be added directly as the
> maintainer, or should this be specified as "Raspberry Pi Kernel
> Maintenance <kernel-list@raspberrypi.com>"
> - in v2: multiple corrections to pass the bot checking as Rob kindly
> told me.
> ---
> .../bindings/media/brcm,bcm2835-unicam.yaml | 103 ++++++++++++++++++
> MAINTAINERS | 6 +
> 2 files changed, 109 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> new file mode 100644
> index 000000000000..1427514142cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> @@ -0,0 +1,103 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/brcm,bcm2835-unicam.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Broadcom BCM283x Camera Interface (Unicam)
> +
> +maintainers:
> + - Dave Stevenson <dave.stevenson@raspberrypi.com>
> +
> +description: |-
> + The Unicam block on BCM283x SoCs is the receiver for either
> + CSI-2 or CCP2 data from image sensors or similar devices.
> +
> + The main platform using this SoC is the Raspberry Pi family of boards.
> + On the Pi the VideoCore firmware can also control this hardware block,
> + and driving it from two different processors will cause issues.
> + To avoid this, the firmware checks the device tree configuration
> + during boot. If it finds device tree nodes called csi0 or csi1 then
> + it will stop the firmware accessing the block, and it can then
> + safely be used via the device tree binding.
As mentioned in the review of the DT integration, the nodes should
ideally be called just "csi", not "csi0" and "csi1" (maybe Rob could
confirm this ?). Dave, is there a way the firmware could be updated to
also hand over control of the Unicam instances to Linux when a "csi"
node is found, not just "csi0" or "csi1" ?
Given that the node names are significant, they should be enforced in
the YAML schema.
> +
> +properties:
> + compatible:
> + const: brcm,bcm2835-unicam
> +
> + reg:
> + description:
> + physical base address and length of the register sets for the device.
This can be dropped.
> + maxItems: 1
There are two items in the example below. How does this validate ?
> +
> + interrupts:
> + description: the IRQ line for this Unicam instance.
This can be dropped.
> + maxItems: 1
> +
> + clocks:
> + description: |-
> + list of clock specifiers, corresponding to entries in clock-names
> + property.
clocks:
items:
- description: The clock for ...
- description: The clock for ...
(with the two descriptions matching the LP and VPU clocks, I don't know
what they are).
> +
> + clock-names:
> + items:
> + - const: lp
> + - const: vpu
> +
> + port:
> + $ref: /schemas/graph.yaml#/properties/port
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
> + - clock-names
> + - port
> +
> +additionalProperties: False
> +
> +examples:
> + - |
> + csi1: csi1@7e801000 {
> + compatible = "brcm,bcm2835-unicam";
> + reg = <0x7e801000 0x800>,
> + <0x7e802004 0x4>;
> + interrupts = <2 7>;
Let's use the Pi 4 device tree as an example, as that's what we're
upstreaming first.
> + clocks = <&clocks BCM2835_CLOCK_CAM1>,
This will fail to compile without a proper #include, did you get this to
pass validation ?
> + <&firmware_clocks 4>;
> + clock-names = "lp", "vpu";
> + port {
> + csi1_ep: endpoint {
> + remote-endpoint = <&tc358743_0>;
> + data-lanes = <1 2>;
> + };
> + };
> + };
> +
> + i2c0: i2c@7e205000 {
> + tc358743: csi-hdmi-bridge@0f {
> + compatible = "toshiba,tc358743";
> + reg = <0x0f>;
> + clocks = <&tc358743_clk>;
> + clock-names = "refclk";
> +
> + tc358743_clk: bridge-clk {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <27000000>;
> + };
> +
> + port {
> + tc358743_0: endpoint {
> + remote-endpoint = <&csi1_ep>;
> + clock-lanes = <0>;
> + data-lanes = <1 2>;
> + clock-noncontinuous;
> + link-frequencies =
> + /bits/ 64 <297000000>;
> + };
> + };
> + };
> + };
I'd drop this node completely.
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 33f75892f98e..07f238fd5ff9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3679,6 +3679,12 @@ F: Documentation/media/v4l-drivers/bcm2835-isp.rst
> F: drivers/staging/vc04_services/bcm2835-isp
> F: include/uapi/linux/bcm2835-isp.h
>
> +BROADCOM BCM2835 CAMERA DRIVER
> +M: Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com>
> +L: linux-media@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> +
> BROADCOM BCM47XX MIPS ARCHITECTURE
> M: Hauke Mehrtens <hauke@hauke-m.de>
> M: Rafał Miłecki <zajec5@gmail.com>
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [RFC PATCH v2 7/7] media: bcm283x: Include the imx219 node
From: Laurent Pinchart @ 2022-01-21 23:30 UTC (permalink / raw)
To: Jean-Michel Hautbois
Cc: dave.stevenson, devicetree, kernel-list, linux-arm-kernel,
linux-kernel, linux-media, linux-rpi-kernel, lukasz, mchehab,
naush, robh, tomi.valkeinen
In-Reply-To: <20220121081810.155500-8-jeanmichel.hautbois@ideasonboard.com>
Hi Jean-Michel,
Thank you for the patch.
On Fri, Jan 21, 2022 at 09:18:10AM +0100, Jean-Michel Hautbois wrote:
> Configure the csi1 endpoint, add the imx219 node and connect it through
> the i2c mux.
This is not meant to be upstreamed, is it ? Please say so very loudly in
the commit message.
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
> MAINTAINERS | 1 +
> arch/arm/boot/dts/bcm2711-rpi-4-b.dts | 1 +
> arch/arm/boot/dts/bcm283x-rpi-imx219.dtsi | 102 ++++++++++++++++++++++
> 3 files changed, 104 insertions(+)
> create mode 100644 arch/arm/boot/dts/bcm283x-rpi-imx219.dtsi
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b17bb533e007..56544ac98d69 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3684,6 +3684,7 @@ M: Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com>
> L: linux-media@vger.kernel.org
> S: Maintained
> F: Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> +F: arch/arm/boot/dts/bcm283x*
> F: drivers/media/platform/bcm2835/
>
> BROADCOM BCM47XX MIPS ARCHITECTURE
> diff --git a/arch/arm/boot/dts/bcm2711-rpi-4-b.dts b/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
> index 4432412044de..f7625b70fe57 100644
> --- a/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
> +++ b/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
> @@ -4,6 +4,7 @@
> #include "bcm2711-rpi.dtsi"
> #include "bcm283x-rpi-usb-peripheral.dtsi"
> #include "bcm283x-rpi-wifi-bt.dtsi"
> +#include "bcm283x-rpi-imx219.dtsi"
Let's use an overlay instead.
>
> / {
> compatible = "raspberrypi,4-model-b", "brcm,bcm2711";
> diff --git a/arch/arm/boot/dts/bcm283x-rpi-imx219.dtsi b/arch/arm/boot/dts/bcm283x-rpi-imx219.dtsi
> new file mode 100644
> index 000000000000..f2c6a85fd731
> --- /dev/null
> +++ b/arch/arm/boot/dts/bcm283x-rpi-imx219.dtsi
> @@ -0,0 +1,102 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <dt-bindings/clock/bcm2835.h>
> +
> +/ {
> + compatible = "brcm,bcm2835";
> +
> + imx219_vdig: fixedregulator@1 {
> + compatible = "regulator-fixed";
> + regulator-name = "imx219_vdig";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + };
> +
> + imx219_vddl: fixedregulator@2 {
> + compatible = "regulator-fixed";
> + regulator-name = "imx219_vddl";
> + regulator-min-microvolt = <1200000>;
> + regulator-max-microvolt = <1200000>;
> + };
> +
> + imx219_clk: imx219_clk {
> + #clock-cells = <0>;
> + compatible = "fixed-clock";
> + clock-frequency = <24000000>;
> + clock-output-names = "24MHz-clock";
> + };
> +
> + cam1_reg: cam1_reg {
> + compatible = "regulator-fixed";
> + regulator-name = "imx219_vana";
> + enable-active-high;
> + status = "okay";
> + gpio = <&expgpio 5 GPIO_ACTIVE_HIGH>;
> + };
This regulator belongs to the board dtsi. Same for the I2C mux below
(but not the imx219 of course).
> +
> + i2c0mux {
> + compatible = "i2c-mux-pinctrl";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + i2c-parent = <&i2c0>;
> +
> + pinctrl-names = "i2c0", "i2c_csi_dsi";
> + pinctrl-0 = <&i2c0_gpio0>;
> + pinctrl-1 = <&i2c0_gpio44>;
> +
> + i2c@0 {
> + reg = <0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };
> +
> + i2c@1 {
> + reg = <1>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + imx219: sensor@10 {
> + compatible = "sony,imx219";
> + reg = <0x10>;
> + status = "okay";
> +
> + clocks = <&imx219_clk>;
> + clock-names = "xclk";
> +
> + VANA-supply = <&cam1_reg>; /* 2.8v */
> + VDIG-supply = <&imx219_vdig>; /* 1.8v */
> + VDDL-supply = <&imx219_vddl>; /* 1.2v */
> +
> + rotation = <0>;
> + orientation = <0>;
> +
> + port {
> + imx219_0: endpoint {
> + remote-endpoint = <&csi1_ep>;
> + clock-lanes = <0>;
> + data-lanes = <1 2>;
> + clock-noncontinuous;
> + link-frequencies = /bits/ 64 <456000000>;
> + };
> + };
> + };
> + };
> + };
> +};
> +
> +&csi1 {
> + status="okay";
> + num-data-lanes = <2>;
> + port {
> + csi1_ep: endpoint {
> + remote-endpoint = <&imx219_0>;
> + data-lanes = <1 2>;
> + clock-lanes = <0>;
> + };
> + };
> +};
> +
> +&i2c0 {
> + /delete-property/ pinctrl-names;
> + /delete-property/ pinctrl-0;
> +};
> +
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [RFC PATCH v2 6/7] media: imx219: Add support for multiplexed streams
From: Laurent Pinchart @ 2022-01-21 23:34 UTC (permalink / raw)
To: Jean-Michel Hautbois
Cc: dave.stevenson, devicetree, kernel-list, linux-arm-kernel,
linux-kernel, linux-media, linux-rpi-kernel, lukasz, mchehab,
naush, robh, tomi.valkeinen
In-Reply-To: <20220121081810.155500-7-jeanmichel.hautbois@ideasonboard.com>
Hi Jean-Michel,
Thank you for the patch.
On Fri, Jan 21, 2022 at 09:18:09AM +0100, Jean-Michel Hautbois wrote:
> As of now, imx219 was not supporting anything more than one stream. Add
> support for embedded data, based on linux-rpi kernel, and make it work
> with multiplexed streams. We have only one source pad with two streams:
> stream 0 is the image, and stream 1 is the embedded data.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>
> ---
> in v2: modified the get_format_pad function as it was not correctly
> propagating the format in case of sensor flips.
> ---
> drivers/media/i2c/imx219.c | 452 ++++++++++++++++++++-----------------
This is too big, it bundles multiple changes together. It should be
split in multiple patches.
> 1 file changed, 241 insertions(+), 211 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index e10af3f74b38..d73fe6b8b2fb 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -118,6 +118,16 @@
> #define IMX219_PIXEL_ARRAY_WIDTH 3280U
> #define IMX219_PIXEL_ARRAY_HEIGHT 2464U
>
> +/* Embedded metadata stream structure */
> +#define IMX219_EMBEDDED_LINE_WIDTH 16384
> +#define IMX219_NUM_EMBEDDED_LINES 1
> +
> +enum pad_types {
> + IMAGE_PAD,
> + METADATA_PAD,
> + NUM_PADS
> +};
No used.
> +
> struct imx219_reg {
> u16 address;
> u8 val;
> @@ -429,7 +439,7 @@ static const char * const imx219_supply_name[] = {
> * - v flip
> * - h&v flips
> */
> -static const u32 codes[] = {
> +static const u32 imx219_mbus_formats[] = {
For instance renaming this array should go in a patch of its own.
> MEDIA_BUS_FMT_SRGGB10_1X10,
> MEDIA_BUS_FMT_SGRBG10_1X10,
> MEDIA_BUS_FMT_SGBRG10_1X10,
> @@ -655,62 +665,17 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
>
> lockdep_assert_held(&imx219->mutex);
>
> - for (i = 0; i < ARRAY_SIZE(codes); i++)
> - if (codes[i] == code)
> + for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
> + if (imx219_mbus_formats[i] == code)
> break;
>
> - if (i >= ARRAY_SIZE(codes))
> + if (i >= ARRAY_SIZE(imx219_mbus_formats))
> i = 0;
>
> i = (i & ~3) | (imx219->vflip->val ? 2 : 0) |
> (imx219->hflip->val ? 1 : 0);
>
> - return codes[i];
> -}
> -
> -static void imx219_set_default_format(struct imx219 *imx219)
> -{
> - struct v4l2_mbus_framefmt *fmt;
> -
> - fmt = &imx219->fmt;
> - fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
> - fmt->colorspace = V4L2_COLORSPACE_SRGB;
> - fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> - fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> - fmt->colorspace,
> - fmt->ycbcr_enc);
> - fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> - fmt->width = supported_modes[0].width;
> - fmt->height = supported_modes[0].height;
> - fmt->field = V4L2_FIELD_NONE;
> -}
> -
> -static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
So should switching from open to init_cfg. Same thing for adding support
for the V4L2 subdev active state (v4l2_subdev_init_finalize() and the
corresponding changes in the get/set format handlers). This is mainline,
not a BSP, patches must be reviewable.
> -{
> - struct imx219 *imx219 = to_imx219(sd);
> - struct v4l2_mbus_framefmt *try_fmt =
> - v4l2_subdev_get_try_format(sd, fh->state, 0);
> - struct v4l2_rect *try_crop;
> -
> - mutex_lock(&imx219->mutex);
> -
> - /* Initialize try_fmt */
> - try_fmt->width = supported_modes[0].width;
> - try_fmt->height = supported_modes[0].height;
> - try_fmt->code = imx219_get_format_code(imx219,
> - MEDIA_BUS_FMT_SRGGB10_1X10);
> - try_fmt->field = V4L2_FIELD_NONE;
> -
> - /* Initialize try_crop rectangle. */
> - try_crop = v4l2_subdev_get_try_crop(sd, fh->state, 0);
> - try_crop->top = IMX219_PIXEL_ARRAY_TOP;
> - try_crop->left = IMX219_PIXEL_ARRAY_LEFT;
> - try_crop->width = IMX219_PIXEL_ARRAY_WIDTH;
> - try_crop->height = IMX219_PIXEL_ARRAY_HEIGHT;
> -
> - mutex_unlock(&imx219->mutex);
> -
> - return 0;
> + return imx219_mbus_formats[i];
> }
>
> static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> @@ -802,98 +767,148 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
> .s_ctrl = imx219_set_ctrl,
> };
>
> -static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
> - struct v4l2_subdev_state *sd_state,
> - struct v4l2_subdev_mbus_code_enum *code)
> +static void imx219_init_formats(struct v4l2_subdev_state *state)
> {
> - struct imx219 *imx219 = to_imx219(sd);
> + struct v4l2_mbus_framefmt *format;
> +
> + format = v4l2_state_get_stream_format(state, 0, 0);
> + format->code = imx219_mbus_formats[0];
> + format->width = supported_modes[0].width;
> + format->height = supported_modes[0].height;
> + format->field = V4L2_FIELD_NONE;
> + format->colorspace = V4L2_COLORSPACE_RAW;
> +
> + if (state->routing.routes[1].flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE) {
> + format = v4l2_state_get_stream_format(state, 0, 1);
> + format->code = MEDIA_BUS_FMT_METADATA_8;
> + format->width = IMX219_EMBEDDED_LINE_WIDTH;
> + format->height = 1;
> + format->field = V4L2_FIELD_NONE;
> + format->colorspace = V4L2_COLORSPACE_DEFAULT;
> + }
> +}
>
> - if (code->index >= (ARRAY_SIZE(codes) / 4))
> - return -EINVAL;
> +static int _imx219_set_routing(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state)
> +{
> + struct v4l2_subdev_route routes[] = {
> + {
> + .source_pad = 0,
> + .source_stream = 0,
> + .flags = V4L2_SUBDEV_ROUTE_FL_IMMUTABLE |
> + V4L2_SUBDEV_ROUTE_FL_SOURCE |
> + V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> + },
> + {
> + .source_pad = 0,
> + .source_stream = 1,
> + .flags = V4L2_SUBDEV_ROUTE_FL_SOURCE |
> + V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> + }
> + };
>
> - mutex_lock(&imx219->mutex);
> - code->code = imx219_get_format_code(imx219, codes[code->index * 4]);
> - mutex_unlock(&imx219->mutex);
> + struct v4l2_subdev_krouting routing = {
> + .num_routes = ARRAY_SIZE(routes),
> + .routes = routes,
> + };
> +
> + int ret;
> +
> + ret = v4l2_subdev_set_routing(sd, state, &routing);
> + if (ret)
> + return ret;
> +
> + imx219_init_formats(state);
>
> return 0;
> }
>
> -static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> - struct v4l2_subdev_state *sd_state,
> - struct v4l2_subdev_frame_size_enum *fse)
> +static int imx219_set_routing(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state,
> + enum v4l2_subdev_format_whence which,
> + struct v4l2_subdev_krouting *routing)
> {
> - struct imx219 *imx219 = to_imx219(sd);
> - u32 code;
> + int ret;
>
> - if (fse->index >= ARRAY_SIZE(supported_modes))
> + if (routing->num_routes == 0 || routing->num_routes > 2)
> return -EINVAL;
>
> - mutex_lock(&imx219->mutex);
> - code = imx219_get_format_code(imx219, fse->code);
> - mutex_unlock(&imx219->mutex);
> - if (fse->code != code)
> - return -EINVAL;
> + v4l2_subdev_lock_state(state);
>
> - fse->min_width = supported_modes[fse->index].width;
> - fse->max_width = fse->min_width;
> - fse->min_height = supported_modes[fse->index].height;
> - fse->max_height = fse->min_height;
> + ret = _imx219_set_routing(sd, state);
>
> - return 0;
> + v4l2_subdev_unlock_state(state);
> +
> + return ret;
> }
>
> -static void imx219_reset_colorspace(struct v4l2_mbus_framefmt *fmt)
> +static int imx219_init_cfg(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state)
> {
> - fmt->colorspace = V4L2_COLORSPACE_SRGB;
> - fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> - fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> - fmt->colorspace,
> - fmt->ycbcr_enc);
> - fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> + int ret;
> +
> + v4l2_subdev_lock_state(state);
> +
> + ret = _imx219_set_routing(sd, state);
> +
> + v4l2_subdev_unlock_state(state);
> +
> + return ret;
> }
>
> -static void imx219_update_pad_format(struct imx219 *imx219,
> - const struct imx219_mode *mode,
> - struct v4l2_subdev_format *fmt)
> +static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_mbus_code_enum *code)
> {
> - fmt->format.width = mode->width;
> - fmt->format.height = mode->height;
> - fmt->format.field = V4L2_FIELD_NONE;
> - imx219_reset_colorspace(&fmt->format);
> + if (code->index >= ARRAY_SIZE(imx219_mbus_formats))
> + return -EINVAL;
> +
> + code->code = imx219_mbus_formats[code->index];
> +
> + return 0;
> }
>
> -static int __imx219_get_pad_format(struct imx219 *imx219,
> - struct v4l2_subdev_state *sd_state,
> - struct v4l2_subdev_format *fmt)
> +static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_frame_size_enum *fse)
> {
> - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> - struct v4l2_mbus_framefmt *try_fmt =
> - v4l2_subdev_get_try_format(&imx219->sd, sd_state,
> - fmt->pad);
> - /* update the code which could change due to vflip or hflip: */
> - try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
> - fmt->format = *try_fmt;
> + unsigned int i;
> +
> + if (fse->stream == 0) {
> + for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); ++i) {
> + if (imx219_mbus_formats[i] == fse->code)
> + break;
> + }
> +
> + if (i == ARRAY_SIZE(imx219_mbus_formats))
> + return -EINVAL;
> +
> + if (fse->index >= ARRAY_SIZE(supported_modes))
> + return -EINVAL;
> +
> + fse->min_width = supported_modes[fse->index].width;
> + fse->max_width = fse->min_width;
> + fse->max_height = supported_modes[fse->index].height;
> + fse->min_height = fse->max_height;
> } else {
> - imx219_update_pad_format(imx219, imx219->mode, fmt);
> - fmt->format.code = imx219_get_format_code(imx219,
> - imx219->fmt.code);
> + if (fse->code != MEDIA_BUS_FMT_METADATA_8)
> + return -EINVAL;
> +
> + fse->min_width = IMX219_EMBEDDED_LINE_WIDTH;
> + fse->max_width = fse->min_width;
> + fse->min_height = IMX219_NUM_EMBEDDED_LINES;
> + fse->max_height = fse->min_height;
> }
>
> return 0;
> }
>
> -static int imx219_get_pad_format(struct v4l2_subdev *sd,
> - struct v4l2_subdev_state *sd_state,
> - struct v4l2_subdev_format *fmt)
> +static void imx219_update_metadata_pad_format(struct v4l2_subdev_format *fmt)
> {
> - struct imx219 *imx219 = to_imx219(sd);
> - int ret;
> -
> - mutex_lock(&imx219->mutex);
> - ret = __imx219_get_pad_format(imx219, sd_state, fmt);
> - mutex_unlock(&imx219->mutex);
> -
> - return ret;
> + fmt->format.width = IMX219_EMBEDDED_LINE_WIDTH;
> + fmt->format.height = IMX219_NUM_EMBEDDED_LINES;
> + fmt->format.code = MEDIA_BUS_FMT_METADATA_8;
> + fmt->format.field = V4L2_FIELD_NONE;
> }
>
> static int imx219_set_pad_format(struct v4l2_subdev *sd,
> @@ -901,82 +916,91 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> struct v4l2_subdev_format *fmt)
> {
> struct imx219 *imx219 = to_imx219(sd);
> + struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> const struct imx219_mode *mode;
> - struct v4l2_mbus_framefmt *framefmt;
> - int exposure_max, exposure_def, hblank;
> + struct v4l2_mbus_framefmt *format;
> unsigned int i;
> + int ret = 0;
> + int exposure_max, exposure_def, hblank;
>
> - mutex_lock(&imx219->mutex);
> + if (fmt->pad != 0) {
> + dev_err(&client->dev, "%s Could not get pad %d\n", __func__,
> + fmt->pad);
> + return -EINVAL;
> + }
>
> - for (i = 0; i < ARRAY_SIZE(codes); i++)
> - if (codes[i] == fmt->format.code)
> + if (fmt->stream == 1) {
> + /* Only one embedded data mode is supported */
> + imx219_update_metadata_pad_format(fmt);
> + return 0;
> + }
> +
> + if (fmt->stream != 0)
> + return -EINVAL;
> +
> + /*
> + * Validate the media bus code, defaulting to the first one if the
> + * requested code isn't supported.
> + */
> + for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); ++i) {
> + if (imx219_mbus_formats[i] == fmt->format.code)
> break;
> - if (i >= ARRAY_SIZE(codes))
> - i = 0;
> + }
>
> - /* Bayer order varies with flips */
> - fmt->format.code = imx219_get_format_code(imx219, codes[i]);
> + if (i >= ARRAY_SIZE(imx219_mbus_formats))
> + i = 0;
>
> mode = v4l2_find_nearest_size(supported_modes,
> ARRAY_SIZE(supported_modes),
> width, height,
> - fmt->format.width, fmt->format.height);
> - imx219_update_pad_format(imx219, mode, fmt);
> - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> - framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
> - *framefmt = fmt->format;
> - } else if (imx219->mode != mode ||
> - imx219->fmt.code != fmt->format.code) {
> - imx219->fmt = fmt->format;
> - imx219->mode = mode;
> - /* Update limits and set FPS to default */
> - __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> - IMX219_VTS_MAX - mode->height, 1,
> - mode->vts_def - mode->height);
> - __v4l2_ctrl_s_ctrl(imx219->vblank,
> - mode->vts_def - mode->height);
> - /* Update max exposure while meeting expected vblanking */
> - exposure_max = mode->vts_def - 4;
> - exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
> - exposure_max : IMX219_EXPOSURE_DEFAULT;
> - __v4l2_ctrl_modify_range(imx219->exposure,
> - imx219->exposure->minimum,
> - exposure_max, imx219->exposure->step,
> - exposure_def);
> - /*
> - * Currently PPL is fixed to IMX219_PPL_DEFAULT, so hblank
> - * depends on mode->width only, and is not changeble in any
> - * way other than changing the mode.
> - */
> - hblank = IMX219_PPL_DEFAULT - mode->width;
> - __v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1,
> - hblank);
> - }
> + fmt->format.width,
> + fmt->format.height);
>
> - mutex_unlock(&imx219->mutex);
> + v4l2_subdev_lock_state(sd_state);
>
> - return 0;
> -}
> + /* Update the stored format and return it. */
> + format = v4l2_state_get_stream_format(sd_state, fmt->pad, fmt->stream);
>
> -static int imx219_set_framefmt(struct imx219 *imx219)
> -{
> - switch (imx219->fmt.code) {
> - case MEDIA_BUS_FMT_SRGGB8_1X8:
> - case MEDIA_BUS_FMT_SGRBG8_1X8:
> - case MEDIA_BUS_FMT_SGBRG8_1X8:
> - case MEDIA_BUS_FMT_SBGGR8_1X8:
> - return imx219_write_regs(imx219, raw8_framefmt_regs,
> - ARRAY_SIZE(raw8_framefmt_regs));
> -
> - case MEDIA_BUS_FMT_SRGGB10_1X10:
> - case MEDIA_BUS_FMT_SGRBG10_1X10:
> - case MEDIA_BUS_FMT_SGBRG10_1X10:
> - case MEDIA_BUS_FMT_SBGGR10_1X10:
> - return imx219_write_regs(imx219, raw10_framefmt_regs,
> - ARRAY_SIZE(raw10_framefmt_regs));
> + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE && imx219->streaming) {
> + ret = -EBUSY;
> + goto done;
> }
>
> - return -EINVAL;
> + /* Bayer order varies with flips */
> + fmt->format.code = imx219_get_format_code(imx219, imx219_mbus_formats[i]);
> + fmt->format = *format;
> +
> + /* Update limits and set FPS to default */
> + __v4l2_ctrl_modify_range(imx219->vblank,
> + IMX219_VBLANK_MIN,
> + IMX219_VTS_MAX - mode->height,
> + 1,
> + mode->vts_def - mode->height);
> + __v4l2_ctrl_s_ctrl(imx219->vblank, mode->vts_def - mode->height);
> + /*
> + * Update max exposure while meeting
> + * expected vblanking
> + */
> + exposure_max = mode->vts_def - 4;
> + exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
> + exposure_max : IMX219_EXPOSURE_DEFAULT;
> + __v4l2_ctrl_modify_range(imx219->exposure,
> + imx219->exposure->minimum,
> + exposure_max,
> + imx219->exposure->step,
> + exposure_def);
> + /*
> + * Currently PPL is fixed to IMX219_PPL_DEFAULT, so
> + * hblank depends on mode->width only, and is not
> + * changeble in any way other than changing the mode.
> + */
> + hblank = IMX219_PPL_DEFAULT - mode->width;
> + __v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1, hblank);
> +
> +done:
> + v4l2_subdev_unlock_state(sd_state);
> +
> + return ret;
> }
>
> static const struct v4l2_rect *
> @@ -1037,9 +1061,11 @@ static int imx219_start_streaming(struct imx219 *imx219)
> const struct imx219_reg_list *reg_list;
> int ret;
>
> - ret = pm_runtime_resume_and_get(&client->dev);
> - if (ret < 0)
> + ret = pm_runtime_get_sync(&client->dev);
> + if (ret < 0) {
> + pm_runtime_put_noidle(&client->dev);
> return ret;
> + }
>
> /* Apply default values of current mode */
> reg_list = &imx219->mode->reg_list;
> @@ -1049,13 +1075,6 @@ static int imx219_start_streaming(struct imx219 *imx219)
> goto err_rpm_put;
> }
>
> - ret = imx219_set_framefmt(imx219);
> - if (ret) {
> - dev_err(&client->dev, "%s failed to set frame format: %d\n",
> - __func__, ret);
> - goto err_rpm_put;
> - }
> -
> /* Apply customized values from user */
> ret = __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
> if (ret)
> @@ -1133,21 +1152,22 @@ static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
> /* Power/clock management functions */
> static int imx219_power_on(struct device *dev)
> {
> - struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> struct imx219 *imx219 = to_imx219(sd);
> int ret;
>
> ret = regulator_bulk_enable(IMX219_NUM_SUPPLIES,
> imx219->supplies);
> if (ret) {
> - dev_err(dev, "%s: failed to enable regulators\n",
> + dev_err(&client->dev, "%s: failed to enable regulators\n",
> __func__);
> return ret;
> }
>
> ret = clk_prepare_enable(imx219->xclk);
> if (ret) {
> - dev_err(dev, "%s: failed to enable clock\n",
> + dev_err(&client->dev, "%s: failed to enable clock\n",
> __func__);
> goto reg_off;
> }
> @@ -1166,7 +1186,8 @@ static int imx219_power_on(struct device *dev)
>
> static int imx219_power_off(struct device *dev)
> {
> - struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> struct imx219 *imx219 = to_imx219(sd);
>
> gpiod_set_value_cansleep(imx219->reset_gpio, 0);
> @@ -1178,7 +1199,8 @@ static int imx219_power_off(struct device *dev)
>
> static int __maybe_unused imx219_suspend(struct device *dev)
> {
> - struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> struct imx219 *imx219 = to_imx219(sd);
>
> if (imx219->streaming)
> @@ -1189,7 +1211,8 @@ static int __maybe_unused imx219_suspend(struct device *dev)
>
> static int __maybe_unused imx219_resume(struct device *dev)
> {
> - struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> struct imx219 *imx219 = to_imx219(sd);
> int ret;
>
> @@ -1255,11 +1278,13 @@ static const struct v4l2_subdev_video_ops imx219_video_ops = {
> };
>
> static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
> - .enum_mbus_code = imx219_enum_mbus_code,
> - .get_fmt = imx219_get_pad_format,
> - .set_fmt = imx219_set_pad_format,
> - .get_selection = imx219_get_selection,
> - .enum_frame_size = imx219_enum_frame_size,
> + .init_cfg = imx219_init_cfg,
> + .enum_mbus_code = imx219_enum_mbus_code,
> + .get_fmt = v4l2_subdev_get_fmt,
> + .set_fmt = imx219_set_pad_format,
> + .get_selection = imx219_get_selection,
> + .set_routing = imx219_set_routing,
> + .enum_frame_size = imx219_enum_frame_size,
> };
>
> static const struct v4l2_subdev_ops imx219_subdev_ops = {
> @@ -1268,10 +1293,6 @@ static const struct v4l2_subdev_ops imx219_subdev_ops = {
> .pad = &imx219_pad_ops,
> };
>
> -static const struct v4l2_subdev_internal_ops imx219_internal_ops = {
> - .open = imx219_open,
> -};
> -
> /* Initialize control handlers */
> static int imx219_init_controls(struct imx219 *imx219)
> {
> @@ -1446,6 +1467,7 @@ static int imx219_check_hwcfg(struct device *dev)
> static int imx219_probe(struct i2c_client *client)
> {
> struct device *dev = &client->dev;
> + struct v4l2_subdev *sd;
> struct imx219 *imx219;
> int ret;
>
> @@ -1453,7 +1475,8 @@ static int imx219_probe(struct i2c_client *client)
> if (!imx219)
> return -ENOMEM;
>
> - v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
> + sd = &imx219->sd;
> + v4l2_i2c_subdev_init(sd, client, &imx219_subdev_ops);
>
> /* Check the hardware configuration in device tree */
> if (imx219_check_hwcfg(dev))
> @@ -1520,27 +1543,29 @@ static int imx219_probe(struct i2c_client *client)
> goto error_power_off;
>
> /* Initialize subdev */
> - imx219->sd.internal_ops = &imx219_internal_ops;
> - imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> - V4L2_SUBDEV_FL_HAS_EVENTS;
> - imx219->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> + V4L2_SUBDEV_FL_HAS_EVENTS |
> + V4L2_SUBDEV_FL_MULTIPLEXED;
>
> - /* Initialize source pad */
> + /* Initialize the media entity. */
> imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
> -
> - /* Initialize default format */
> - imx219_set_default_format(imx219);
> -
> - ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
> + sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
> + ret = media_entity_pads_init(&sd->entity, 1, &imx219->pad);
> if (ret) {
> dev_err(dev, "failed to init entity pads: %d\n", ret);
> goto error_handler_free;
> }
>
> - ret = v4l2_async_register_subdev_sensor(&imx219->sd);
> + ret = v4l2_subdev_init_finalize(sd);
> + if (ret) {
> + dev_err(dev, "failed to finalize sensor init: %d\n", ret);
> + goto error_media_entity;
> + }
> +
> + ret = v4l2_async_register_subdev_sensor(sd);
> if (ret < 0) {
> dev_err(dev, "failed to register sensor sub-device: %d\n", ret);
> - goto error_media_entity;
> + goto error_free_state;
> }
>
> /* Enable runtime PM and turn off the device */
> @@ -1550,6 +1575,8 @@ static int imx219_probe(struct i2c_client *client)
>
> return 0;
>
> +error_free_state:
> + v4l2_subdev_cleanup(sd);
> error_media_entity:
> media_entity_cleanup(&imx219->sd.entity);
>
> @@ -1568,6 +1595,9 @@ static int imx219_remove(struct i2c_client *client)
> struct imx219 *imx219 = to_imx219(sd);
>
> v4l2_async_unregister_subdev(sd);
> +
> + v4l2_subdev_cleanup(sd);
> +
> media_entity_cleanup(&sd->entity);
> imx219_free_controls(imx219);
>
--
Regards,
Laurent Pinchart
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox