* Re: [PATCH 12/12] arm64: dts: qcom: Enable Q6v5 WCSS for ipq8074 SoC
From: gokulsri @ 2019-07-18 6:24 UTC (permalink / raw)
To: Stephen Boyd
Cc: agross, bjorn.andersson, david.brown, devicetree, jassisinghbrar,
linux-arm-msm, linux-clk, linux-kernel, linux-remoteproc,
mark.rutland, mturquette, ohad, robh+dt, sricharan
In-Reply-To: <20190717201326.DCEB520880@mail.kernel.org>
On 2019-07-18 01:43, Stephen Boyd wrote:
> Quoting Gokul Sriram Palanisamy (2019-07-11 08:41:08)
>> diff --git a/arch/arm64/boot/dts/qcom/ipq8074.dtsi
>> b/arch/arm64/boot/dts/qcom/ipq8074.dtsi
>> index 6a61a63..c24e3f6 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq8074.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq8074.dtsi
>> @@ -10,6 +10,22 @@
>> model = "Qualcomm Technologies, Inc. IPQ8074";
>> compatible = "qcom,ipq8074";
>>
>> + reserved-memory {
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> + ranges;
>> +
>> + smem_region:smem@4ab00000 {
>
> Put a space between the colon and the node name. Also, just call it
> memory@4ab00000.
ok, will fix.
>
> smem_region: memory@4ab00000 {
>
>> + no-map;
>> + reg = <0x0 0x4ab00000 0x0 0x00100000>;
>> + };
>> +
>> + q6_region: q6@4b000000 {
>
> memory@
ok, will fix.
>
>> + no-map;
>> + reg = <0x0 0x4b000000 0x0 0x05f00000>;
>> + };
>> + };
>> +
>> firmware {
>> scm {
>> compatible = "qcom,scm-ipq8074", "qcom,scm";
>> @@ -431,6 +447,115 @@
>> "axi_m_sticky";
>> status = "disabled";
>> };
>> + apcs: syscon@b111000 {
>
> Add a newline between nodes please.
ok, will fix.
>
>> + compatible = "syscon";
>> + reg = <0x0B111000 0x1000>;
>> + };
>> +
>> + wcss: smp2p-wcss {
>
> This node should be outside the soc node because it doesn't have a reg
> property
ok, will fix.
>
>> + compatible = "qcom,smp2p";
>> + qcom,smem = <435>, <428>;
>> +
>> + interrupt-parent = <&intc>;
>> + interrupts = <0 322 1>;
>> +
>> + qcom,ipc = <&apcs 8 9>;
>> +
>> + qcom,local-pid = <0>;
>> + qcom,remote-pid = <1>;
>> +
>> + wcss_smp2p_out: master-kernel {
>> + qcom,entry-name = "master-kernel";
>> + qcom,smp2p-feature-ssr-ack;
>> + #qcom,smem-state-cells = <1>;
>> + };
>> +
>> + wcss_smp2p_in: slave-kernel {
>> + qcom,entry-name = "slave-kernel";
>> +
>> + interrupt-controller;
>> + #interrupt-cells = <2>;
>> + };
>> + };
>> +
>> + tcsr_q6_block: syscon@1945000 {
>
> Do you really need _block in these aliases?
ok, will fix it to "tcsr_q6"
>
>> + compatible = "syscon";
>> + reg = <0x1945000 0xE000>;
>> + };
>> +
>> + tcsr_mutex_block: syscon@193d000 {
>> + compatible = "syscon";
>> + reg = <0x1905000 0x8000>;
>> + };
>> +
>> + tcsr_mutex: hwlock@193d000 {
>> + compatible = "qcom,tcsr-mutex";
>> + syscon = <&tcsr_mutex_block 0 0x80>;
>> + #hwlock-cells = <1>;
>> + };
>> +
>> + smem: qcom,smem@4AB00000 {
>
> lowercase please. And just 'smem' I guess.
ok, will fix.
>
>> + compatible = "qcom,smem";
>> + memory-region = <&smem_region>;
>> + hwlocks = <&tcsr_mutex 0>;
>> + };
>> +
>> + apcs_glb: mailbox@b111000 {
>> + compatible = "qcom,ipq8074-apcs-apps-global";
>> + reg = <0xb111000 0x1000>;
>
> These addresses should be padded out to 8 digits for the address part
> (not the size).
ok, will fix.
>
>> +
>> + #mbox-cells = <1>;
>> + };
>> +
>> + q6v5_wcss: q6v5_wcss@CD00000 {
>
> lowercase.
ok, will fix.
>
>> + compatible = "qcom,ipq8074-wcss-pil";
>> + reg = <0xCD00000 0x4040>,
>> + <0x4AB000 0x20>;
Regards,
Gokul
^ permalink raw reply
* Re: [PATCH v2] arm64: dts: imx8mm: Init rates and parents configs for clocks
From: Shawn Guo @ 2019-07-18 6:15 UTC (permalink / raw)
To: Abel Vesa
Cc: Rob Herring, Mark Rutland, Leonard Crestez, Jacky Bai,
Sascha Hauer, Daniel Baluta, NXP Linux Team, linux-arm-kernel,
devicetree, Linux Kernel Mailing List
In-Reply-To: <1562155702-29809-1-git-send-email-abel.vesa@nxp.com>
On Wed, Jul 03, 2019 at 03:08:22PM +0300, Abel Vesa wrote:
> Add the initial configuration for clocks that need default parent and rate
> setting. This is based on the vendor tree clock provider parents and rates
> configuration except this is doing the setup in dts rather than using clock
> consumer API in a clock provider driver.
>
> Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH v1 08/50] clk: samsung: change aclk266_isp clocks definitions Exynos5420
From: Lukasz Luba @ 2019-07-18 6:05 UTC (permalink / raw)
To: Chanwoo Choi, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, linux-clk
Cc: mark.rutland, b.zolnierkie, sboyd, mturquette, krzk, a.hajda,
robh+dt, kyungmin.park, kgene, myungjoo.ham, s.nawrocki,
m.szyprowski
In-Reply-To: <5ac878fa-21c8-afcd-9afe-13ed3cd36afb@samsung.com>
Hi Chanwoo,
On 7/16/19 12:22 PM, Chanwoo Choi wrote:
> Hi,
>
> On 19. 7. 15. 오후 9:43, Lukasz Luba wrote:
>> The ACLK266_ISP has different topology in Exynos5420 and 5422/5800. In
>> Exynos5420 this clock does not have dedicated MUX which chooses PLL instead
>> it takes the clock from 2-level mux from ACLK266. In Exynos5422 there is a
>> dedicated clock tree and the PLL can be chosen. The patch adds needed
>> MUXes in the exynos5800_mux_cloks, updates exynos5x_mux_clks and
>> exynos5420_mux_clks properly. It also adds IDs to mange these clocks from
>> DT.
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>> drivers/clk/samsung/clk-exynos5420.c | 17 +++++++++++++++--
>> 1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
>> index 9d54856dbcda..5f251279b4c8 100644
>> --- a/drivers/clk/samsung/clk-exynos5420.c
>> +++ b/drivers/clk/samsung/clk-exynos5420.c
>> @@ -432,6 +432,10 @@ PNAME(mout_group16_5800_p) = { "dout_osc_div", "mout_mau_epll_clk" };
>> PNAME(mout_mx_mspll_ccore_phy_p) = { "sclk_bpll", "mout_sclk_dpll",
>> "mout_sclk_mpll", "ff_dout_spll2",
>> "mout_sclk_spll", "mout_sclk_epll"};
>> +PNAME(mout_group17_5800_p) = { "dout_aclk266_isp", "dout_sclk_sw" };
>> +PNAME(mout_group18_5800_p) = { "dout_osc_div", "mout_sw_aclk266_isp" };
>> +PNAME(mout_group19_5800_p) = { "mout_sclk_cpll", "mout_sclk_dpll",
>> + "mout_sclk_mpll", "mout_sclk_ipll"};
>>
>> /* fixed rate clocks generated outside the soc */
>> static struct samsung_fixed_rate_clock
>> @@ -494,6 +498,8 @@ static const struct samsung_mux_clock exynos5800_mux_clks[] __initconst = {
>> MUX(CLK_SCLK_BPLL, "sclk_bpll", mout_bpll_p, SRC_TOP7, 24, 1),
>> MUX(0, "mout_epll2", mout_epll2_5800_p, SRC_TOP7, 28, 1),
>>
>> + MUX(CLK_MOUT_ACLK266_ISP, "mout_aclk266_isp", mout_group19_5800_p,
>> + SRC_TOP8, 12, 2),
>> MUX(0, "mout_aclk550_cam", mout_group3_5800_p, SRC_TOP8, 16, 3),
>> MUX(0, "mout_aclkfl1_550_cam", mout_group3_5800_p, SRC_TOP8, 20, 3),
>> MUX(0, "mout_aclk432_cam", mout_group6_5800_p, SRC_TOP8, 24, 2),
>> @@ -501,6 +507,8 @@ static const struct samsung_mux_clock exynos5800_mux_clks[] __initconst = {
>>
>> MUX_F(CLK_MOUT_USER_MAU_EPLL, "mout_user_mau_epll", mout_group16_5800_p,
>> SRC_TOP9, 8, 1, CLK_SET_RATE_PARENT, 0),
>> + MUX(CLK_MOUT_USER_ACLK266_ISP, "mout_user_aclk266_isp",
>> + mout_group18_5800_p, SRC_TOP9, 12, 1),
>> MUX(0, "mout_user_aclk550_cam", mout_group15_5800_p,
>> SRC_TOP9, 16, 1),
>> MUX(0, "mout_user_aclkfl1_550_cam", mout_group13_5800_p,
>> @@ -510,6 +518,8 @@ static const struct samsung_mux_clock exynos5800_mux_clks[] __initconst = {
>> MUX(0, "mout_user_aclk432_scaler", mout_group9_5800_p,
>> SRC_TOP9, 28, 1),
>>
>> + MUX(CLK_MOUT_SW_ACLK266_ISP, "mout_sw_aclk266_isp", mout_group17_5800_p,
>> + SRC_TOP13, 12, 1),
>> MUX(0, "mout_sw_aclk550_cam", mout_group14_5800_p, SRC_TOP13, 16, 1),
>> MUX(0, "mout_sw_aclkfl1_550_cam", mout_group12_5800_p,
>> SRC_TOP13, 20, 1),
>> @@ -524,6 +534,8 @@ static const struct samsung_mux_clock exynos5800_mux_clks[] __initconst = {
>> static const struct samsung_div_clock exynos5800_div_clks[] __initconst = {
>> DIV(CLK_DOUT_ACLK400_WCORE, "dout_aclk400_wcore",
>> "mout_aclk400_wcore", DIV_TOP0, 16, 3),
>> + DIV(CLK_DOUT_ACLK266_ISP, "dout_aclk266_isp", "mout_aclk266_isp",
>> + DIV_TOP8, 12, 3),
>> DIV(0, "dout_aclk550_cam", "mout_aclk550_cam",
>> DIV_TOP8, 16, 3),
>> DIV(0, "dout_aclkfl1_550_cam", "mout_aclkfl1_550_cam",
>> @@ -574,6 +586,9 @@ static const struct samsung_mux_clock exynos5420_mux_clks[] __initconst = {
>> MUX(0, "mout_aclk300_disp1", mout_group1_p, SRC_TOP2, 24, 2),
>> MUX(0, "mout_aclk300_gscl", mout_group1_p, SRC_TOP2, 28, 2),
>>
>> + MUX(CLK_MOUT_USER_ACLK266_ISP, "mout_user_aclk266_isp",
>> + mout_user_aclk266_isp_p, SRC_TOP4, 16, 1),
>> +
>> MUX(CLK_MOUT_MX_MSPLL_CCORE, "mout_mx_mspll_ccore",
>> mout_group5_5800_p, SRC_TOP7, 16, 2),
>> MUX_F(0, "mout_mau_epll_clk", mout_mau_epll_clk_p, SRC_TOP7, 20, 2,
>> @@ -641,8 +656,6 @@ static const struct samsung_mux_clock exynos5x_mux_clks[] __initconst = {
>> SRC_TOP4, 8, 1),
>> MUX(0, "mout_user_aclk333_432_isp0", mout_user_aclk333_432_isp0_p,
>> SRC_TOP4, 12, 1),
>> - MUX(0, "mout_user_aclk266_isp", mout_user_aclk266_isp_p,
>> - SRC_TOP4, 16, 1),
>> MUX(0, "mout_user_aclk266", mout_user_aclk266_p, SRC_TOP4, 20, 1),
>> MUX(0, "mout_user_aclk166", mout_user_aclk166_p, SRC_TOP4, 24, 1),
>> MUX(CLK_MOUT_USER_ACLK333, "mout_user_aclk333", mout_user_aclk333_p,
>>
>
> I checked it on Exynos5420/5422 TRM.
>
> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
Thank you!
Regards,
Lukasz
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v1 01/50] clk: samsung: add new IDs for Exynos5420 clocks
From: Lukasz Luba @ 2019-07-18 6:02 UTC (permalink / raw)
To: Chanwoo Choi, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, linux-clk
Cc: mark.rutland, b.zolnierkie, sboyd, mturquette, krzk, a.hajda,
robh+dt, kyungmin.park, kgene, myungjoo.ham, s.nawrocki,
m.szyprowski
In-Reply-To: <f8ff72f9-4a55-40c6-fdb9-bb4cddd00cf9@samsung.com>
On 7/16/19 11:49 AM, Chanwoo Choi wrote:
> Hi,
>
> Usually, when developing the clock controller driver,
> define the same sequence between the definition sequence in dt-bibing
> and clock driver. As I replied, if you squash patches, it is easy.
OK
>
> For example,
> This series add clock id as following.
>
> 569 MUX(CLK_MOUT_ACLK400_ISP, "mout_aclk400_isp", mout_group1_p,
> 570 SRC_TOP0, 0, 2),
> 571 MUX(CLK_MOUT_ACLK400_MSCL, "mout_aclk400_mscl", mout_group1_p,
> 572 SRC_TOP0, 4, 2),
> 573 MUX(CLK_MOUT_ACLK400_WCORE, "mout_aclk400_wcore", mout_group1_p,
> 574 SRC_TOP0, 16, 2),
> 575 MUX(CLK_MOUT_ACLK100_NOC, "mout_aclk100_noc", mout_group1_p,
> 576 SRC_TOP0, 20, 2),
>
> In case of this code, you can define the IDs as following sequentially:
>
> #define CLK_MOUT_ACLK400_ISP ...
> #define CLK_MOUT_ACLK400_MSCL ...
> #define CLK_MOUT_ACLK400_WCORE ...
> #define CLK_MOUT_ACLK100_NOC ...
>
Thank you for this example. I will align them.
Regards,
Lukasz
>
> But, this series define the ID as following:
> #define CLK_MOUT_ACLK400_WCORE 662
> #define CLK_MOUT_SCLK_DPLL 663
> #define CLK_MOUT_ACLK100_NOC 664
> #define CLK_MOUT_ACLK200_FSYS2 665
> #define CLK_MOUT_PCLK200_FSYS 666
> #define CLK_MOUT_ACLK200_FSYS 667
> #define CLK_MOUT_ACLK400_ISP 668
> #define CLK_MOUT_ACLK400_MSCL 669
> #define CLK_MOUT_SCLK_MPLL 700
> #define CLK_MOUT_ACLK266 701
> #define CLK_MOUT_UART0 702
> #define CLK_MOUT_UART1 703
> #define CLK_MOUT_UART2 704
> #define CLK_MOUT_UART3 7
>
> On 19. 7. 15. 오후 9:43, Lukasz Luba wrote:
>> There is a need of new IDs which will be used for modeling proper hierarchy
>> in the Exynos54xx SoCs. Previous implementation rely on bootloader
>> settings, which are not configuring properly some clocks.
>> These IDs provide interface to set proper parents.
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>> include/dt-bindings/clock/exynos5420.h | 27 +++++++++++++++++++++++++-
>> 1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/dt-bindings/clock/exynos5420.h b/include/dt-bindings/clock/exynos5420.h
>> index 02d5ac469a3d..c37a28eeaf7e 100644
>> --- a/include/dt-bindings/clock/exynos5420.h
>> +++ b/include/dt-bindings/clock/exynos5420.h
>> @@ -230,6 +230,30 @@
>> #define CLK_MOUT_USER_MAU_EPLL 659
>> #define CLK_MOUT_SCLK_SPLL 660
>> #define CLK_MOUT_MX_MSPLL_CCORE_PHY 661
>> +#define CLK_MOUT_ACLK400_WCORE 662
>> +#define CLK_MOUT_SCLK_DPLL 663
>> +#define CLK_MOUT_ACLK100_NOC 664
>> +#define CLK_MOUT_ACLK200_FSYS2 665
>> +#define CLK_MOUT_PCLK200_FSYS 666
>> +#define CLK_MOUT_ACLK200_FSYS 667
>> +#define CLK_MOUT_ACLK400_ISP 668
>> +#define CLK_MOUT_ACLK400_MSCL 669
>> +#define CLK_MOUT_SCLK_MPLL 700
>> +#define CLK_MOUT_ACLK266 701
>> +#define CLK_MOUT_UART0 702
>> +#define CLK_MOUT_UART1 703
>> +#define CLK_MOUT_UART2 704
>> +#define CLK_MOUT_UART3 705
>> +#define CLK_MOUT_SCLK_CPLL 706
>> +#define CLK_MOUT_PWM 707
>> +#define CLK_MOUT_ACLK266_G2D 708
>> +#define CLK_MOUT_SW_ACLK400_WCORE 709
>> +#define CLK_MOUT_SW_ACLK400_MSCL 710
>> +#define CLK_MOUT_SW_ACLK400_ISP 711
>> +#define CLK_MOUT_SW_ACLK266_ISP 712
>> +#define CLK_MOUT_USER_ACLK266_ISP 713
>> +#define CLK_MOUT_ACLK266_ISP 714
>> +#define CLK_MOUT_MMC0 715
>>
>> /* divider clocks */
>> #define CLK_DOUT_PIXEL 768
>> @@ -264,8 +288,9 @@
>> #define CLK_FF_DOUT_SPLL2 797
>> #define CLK_DOUT_PCLK_DREX0 798
>> #define CLK_DOUT_PCLK_DREX1 799
>> +#define CLK_DOUT_ACLK266_ISP 800
>>
>> /* must be greater than maximal clock id */
>> -#define CLK_NR_CLKS 800
>> +#define CLK_NR_CLKS 801
>>
>> #endif /* _DT_BINDINGS_CLOCK_EXYNOS_5420_H */
>>
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v1 13/50] clk: samsung: add DPLL rate table in Exynos5420
From: Lukasz Luba @ 2019-07-18 6:01 UTC (permalink / raw)
To: Chanwoo Choi, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, linux-clk
Cc: mturquette, sboyd, b.zolnierkie, krzk, kgene, mark.rutland,
robh+dt, kyungmin.park, a.hajda, m.szyprowski, s.nawrocki,
myungjoo.ham
In-Reply-To: <9095a5be-3002-93c6-9d08-92eb84f5c103@samsung.com>
Hi Chanwoo,
On 7/16/19 11:31 AM, Chanwoo Choi wrote:
> Hi,
>
> Also, you better to merge patch13/patch15/patch16 to one patch
> in order to add the PLL table for DPLL/MPLL/SPLL.
OK
>
> And I have a question. Are there any use-case to change
> the PLL frequency for DPLL/MPLL/SPLL?
Yes, when you set a PLL frequency i.e. DPLL to 1200MHz (from 600MHz)
you can attach a child to it which is running with 400, 300, 150 MHz
and these frequencies are set only on this child clock divider
(/3, /4, /8 - 3bit div) (and there is no set parent rate and
reprogramming the master PLL). While 600MHz cannot satisfy 400MHz
and 300MHz OPPs at the same time.
If the child cannot change the PLL, because it could affect other
children of this PLL and cause misalignment in their frequency after
their dividers, it is better to stick with a rule: pin children to the
PLL with frequency rate which could be divided without a rest and gives
all OPPs rates of all children. This PLL rate could be calculated by
hand, finding least common multiple, keeping in mind clock dividers
width (i.e. 3bit).
That's why I am changing the PLLs' rates and connect children to them
also aligning OPPs to the values which are possible after DIV (3bit).
Regards,
Lukasz
>
> On 19. 7. 15. 오후 9:43, Lukasz Luba wrote:
>> The DPLL has fixed frequency left by the bootloader and it is not possible
>> to change it. With this patch the DPLL gets rate table the same for the
>> whole PLL family (similar as APLL, KPLL according to RM) so the frequency
>> might be changed to one of the values defined there.
>> It is needed for further patches which change the DPLL frequency to feed
>> the clocks with proper base.
>> It also sets CLK_IS_CRITICAL for SCLK_DPLL due to some drivers which could
>> disable master clock, which is then populated higher and tries to disable
>> PLL, which casues system crash. The flag is needed for this kind of use
>> cases.
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>> drivers/clk/samsung/clk-exynos5420.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
>> index 7f8221527633..2395b02ce8c5 100644
>> --- a/drivers/clk/samsung/clk-exynos5420.c
>> +++ b/drivers/clk/samsung/clk-exynos5420.c
>> @@ -694,7 +694,8 @@ static const struct samsung_mux_clock exynos5x_mux_clks[] __initconst = {
>> MUX(0, "mout_sclk_rpll", mout_rpll_p, SRC_TOP6, 16, 1),
>> MUX_F(CLK_MOUT_EPLL, "mout_sclk_epll", mout_epll_p, SRC_TOP6, 20, 1,
>> CLK_SET_RATE_PARENT, 0),
>> - MUX(0, "mout_sclk_dpll", mout_dpll_p, SRC_TOP6, 24, 1),
>> + MUX_F(CLK_MOUT_SCLK_DPLL, "mout_sclk_dpll", mout_dpll_p,
>> + SRC_TOP6, 24, 1, CLK_IS_CRITICAL, 0),
>> MUX(0, "mout_sclk_cpll", mout_cpll_p, SRC_TOP6, 28, 1),
>>
>> MUX(CLK_MOUT_SW_ACLK400_ISP, "mout_sw_aclk400_isp",
>> @@ -1514,6 +1515,7 @@ static void __init exynos5x_clk_init(struct device_node *np,
>>
>> if (_get_rate("fin_pll") == 24 * MHZ) {
>> exynos5x_plls[apll].rate_table = exynos5420_pll2550x_24mhz_tbl;
>> + exynos5x_plls[dpll].rate_table = exynos5420_pll2550x_24mhz_tbl;
>> exynos5x_plls[epll].rate_table = exynos5420_epll_24mhz_tbl;
>> exynos5x_plls[kpll].rate_table = exynos5420_pll2550x_24mhz_tbl;
>> }
>>
>
>
^ permalink raw reply
* Re: [PATCHv8 3/5] arm64: dts: qcom: msm8996: Add Coresight support
From: Sai Prakash Ranjan @ 2019-07-18 5:47 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Greg Kroah-Hartman, Suzuki K Poulose, Leo Yan, Alexander Shishkin,
Mike Leach, Rob Herring, Bjorn Andersson, devicetree, David Brown,
Mark Rutland, Rajendra Nayak, Vivek Gautam, Sibi Sankar,
linux-arm-kernel, linux-kernel, linux-arm-msm, Marc Gonzalez
In-Reply-To: <20190717170050.GB4271@xps15>
Hi Mathieu,
On 7/17/2019 10:30 PM, Mathieu Poirier wrote:
> On Fri, Jul 12, 2019 at 07:46:25PM +0530, Sai Prakash Ranjan wrote:
>> From: Vivek Gautam <vivek.gautam@codeaurora.org>
>>
>> Enable coresight support by adding device nodes for the
>> available source, sinks and channel blocks on msm8996.
>>
>> This also adds coresight cpu debug nodes.
>>
>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
>> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Acked-By: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>> arch/arm64/boot/dts/qcom/msm8996.dtsi | 434 ++++++++++++++++++++++++++
>> 1 file changed, 434 insertions(+)
>>
>
> We've gone trhough 8 iteration of this set and I'm still finding checkpatch
> problems, and I'm not referring to lines over 80 characters.
>
I only get below 2 checkpatch warnings:
If you are talking about the below one, then it was not added manually.
It was taken automatically when I pulled in the v7. Should I be
resending this patch for this?
$ ./scripts/checkpatch.pl -g 2fa725fbc09306f1a95befc62715a708b4c0fad0
WARNING: 'Acked-by:' is the preferred signature form
#14:
Acked-By: Suzuki K Poulose <suzuki.poulose@arm.com>
WARNING: line over 80 characters
#154: FILE: arch/arm64/boot/dts/qcom/msm8996.dtsi:763:
+ compatible = "arm,coresight-dynamic-replicator",
"arm,primecell";
total: 0 errors, 2 warnings, 440 lines checked
-Sai
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* Re: [PATCH v3 0/6] Introduce Bandwidth OPPs for interconnect paths
From: Viresh Kumar @ 2019-07-18 5:37 UTC (permalink / raw)
To: Saravana Kannan
Cc: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, Vincent Guittot,
Sweeney, Sean, daidavid1, Rajendra Nayak, Sibi Sankar,
Bjorn Andersson, Evan Green, Android Kernel Team, Linux PM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML
In-Reply-To: <CAGETcx-tbjVzRKW8D-564zgNOhrA_z-NC1q5U70bhoUDBhp6VA@mail.gmail.com>
I know you have explained lots of things earlier as well, but they are
available over multiple threads and I don't know where to reply now :)
Lets have proper discussion (once again) here and be done with it.
Sorry for the trouble of explaining things again.
On 17-07-19, 13:34, Saravana Kannan wrote:
> On Wed, Jul 17, 2019 at 3:32 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 02-07-19, 18:10, Saravana Kannan wrote:
> > > gpu_cache_opp_table: gpu_cache_opp_table {
> > > compatible = "operating-points-v2";
> > >
> > > gpu_cache_3000: opp-3000 {
> > > opp-peak-KBps = <3000>;
> > > opp-avg-KBps = <1000>;
> > > };
> > > gpu_cache_6000: opp-6000 {
> > > opp-peak-KBps = <6000>;
> > > opp-avg-KBps = <2000>;
> > > };
> > > gpu_cache_9000: opp-9000 {
> > > opp-peak-KBps = <9000>;
> > > opp-avg-KBps = <9000>;
> > > };
> > > };
> > >
> > > gpu_ddr_opp_table: gpu_ddr_opp_table {
> > > compatible = "operating-points-v2";
> > >
> > > gpu_ddr_1525: opp-1525 {
> > > opp-peak-KBps = <1525>;
> > > opp-avg-KBps = <452>;
> > > };
> > > gpu_ddr_3051: opp-3051 {
> > > opp-peak-KBps = <3051>;
> > > opp-avg-KBps = <915>;
> > > };
> > > gpu_ddr_7500: opp-7500 {
> > > opp-peak-KBps = <7500>;
> > > opp-avg-KBps = <3000>;
> > > };
> > > };
> >
> > Who is going to use the above tables and how ?
>
> In this example the GPU driver would use these. It'll go through these
> and then decide what peak and average bw to pick based on whatever
> criteria.
Are you saying that the GPU driver will decide which bandwidth to
choose while running at a particular frequency (say 2 GHz) ? And that
it can choose 1525 or 3051 or 7500 from the ddr path ?
Will it be possible to publicly share how we derive to these decisions
?
The thing is I don't like these separate OPP tables which will not be
used by anyone else, but just GPU (or a single device). I would like
to put this data in the GPU OPP table only. What about putting a
range in the GPU OPP table for the Bandwidth if it can change so much
for the same frequency.
> > These are the maximum
> > BW available over these paths, right ?
>
> I wouldn't call them "maximum" because there can't be multiple
> maximums :) But yes, these are the meaningful bandwidth from the GPU's
> perspective to use over these paths.
>
> >
> > > gpu_opp_table: gpu_opp_table {
> > > compatible = "operating-points-v2";
> > > opp-shared;
> > >
> > > opp-200000000 {
> > > opp-hz = /bits/ 64 <200000000>;
> > > };
> > > opp-400000000 {
> > > opp-hz = /bits/ 64 <400000000>;
> > > };
> > > };
> >
> > Shouldn't this link back to the above tables via required-opp, etc ?
> > How will we know how much BW is required by the GPU device for all the
> > paths ?
>
> If that's what the GPU driver wants to do, then yes. But the GPU
> driver could also choose to scale the bandwidth for these paths based
> on multiple other signals. Eg: bus port busy percentage, measure
> bandwidth, etc.
Lets say that the GPU is running at 2 GHz right now and based on above
inputs it wants to increase the bandwidth to 7500 for ddr path, now
does it make sense to run at 4 GHz instead of 2 so we utilize the
bandwidth to the best of our ability and waste less power ?
If something like that is acceptable, then what about keeping the
bandwidth fixed for frequencies and rather scale the frequency of the
GPU on the inputs your provided (like bus port busy percentage, etc).
The current proposal makes me wonder on why should we try to reuse OPP
tables for providing these bandwidth values as the OPP tables for
interconnect paths isn't really a lot of data, only bandwidth all the
time and there is no linking from the device's OPP table as well.
--
viresh
^ permalink raw reply
* Re: [PATCH v8 07/21] iommu/io-pgtable-arm-v7s: Extend MediaTek 4GB Mode
From: Yong Wu @ 2019-07-18 5:37 UTC (permalink / raw)
To: Will Deacon
Cc: youlin.pei-NuS5LvNUpcJWk0Htik3J/w,
devicetree-u79uwXL29TY76Z2rM5mHXA, Nicolas Boichat,
cui.zhang-NuS5LvNUpcJWk0Htik3J/w,
srv_heupstream-NuS5LvNUpcJWk0Htik3J/w, Tomasz Figa, Will Deacon,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Evan Green,
chao.hao-NuS5LvNUpcJWk0Htik3J/w,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, Rob Herring,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Matthias Brugger,
yingjoe.chen-NuS5LvNUpcJWk0Htik3J/w,
anan.sun-NuS5LvNUpcJWk0Htik3J/w, Robin Murphy, Matthias Kaehlcke,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20190717142339.wltamw6wktwixqqn@willie-the-truck>
On Wed, 2019-07-17 at 15:23 +0100, Will Deacon wrote:
> On Wed, Jul 17, 2019 at 08:44:19PM +0800, Yong Wu wrote:
> > On Mon, 2019-07-15 at 10:51 +0100, Will Deacon wrote:
> > > On Sun, Jul 14, 2019 at 12:41:20PM +0800, Yong Wu wrote:
> > > > @@ -742,7 +763,9 @@ static struct io_pgtable
> > > > *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg,
> > > > {
> > > > struct arm_v7s_io_pgtable *data;
> > > >
> > > > - if (cfg->ias > ARM_V7S_ADDR_BITS || cfg->oas > ARM_V7S_ADDR_BITS)
> > > > + if (cfg->ias > ARM_V7S_ADDR_BITS ||
> > > > + (cfg->oas > ARM_V7S_ADDR_BITS &&
> > > > + !(cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT)))
> > > > return NULL;
> > >
> > > I think you can rework this to do something like:
> > >
> > > if (cfg->ias > ARM_V7S_ADDR_BITS)
> > > return NULL;
> > >
> > > if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT) {
> > > if (!IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT))
> > > cfg->oas = min(cfg->oas, ARM_V7S_ADDR_BITS);
> > > else if (cfg->oas > 34)
> > > return NULL;
> > > } else if (cfg->oas > ARM_V7S_ADDR_BITS) {
> > > return NULL;
> > > }
> > >
> > > so that we clamp the oas when phys_addr_t is 32-bit for you. That should
> > > allow you to remove lots of the checking from iopte_to_paddr() too if you
> > > check against oas in the map() function.
> > >
> > > Does that make sense?
> >
> > Of course I'm ok for this. I'm only afraid that this function has
> > already 3 checking "if (x) return NULL", Here we add a new one and so
> > many lines... Maybe the user should guarantee the right value of oas.
> > How about move it into mtk_iommu.c?
> >
> > About the checking of iopte_to_paddr, I can not remove them. I know it
> > may be a bit special and not readable. Hmm, I guess I should use a MACRO
> > instead of the hard code 33 for the special 4GB mode case.
>
> Why can't you just do something like:
>
> if (!(cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT))
> return paddr;
>
> if (pte & ARM_V7S_ATTR_MTK_PA_BIT33)
> paddr |= BIT_ULL(33);
OK here.
>
> if (pte & ARM_V&S_ATTR_MTK_PA_BIT32)
> paddr |= BIT_ULL(32);
No here, The flow is a bit special for 4GB mode here.
This is the detailed remap relationship for our 4GB mode.
CPU PA -> HW PA
register: 0x0 ~ 0x3fff_ffff
dram 1G:0x4000_0000~0x7fff_ffff ->0x1_4000_0000~0x1_7fff_ffff(Add bit32)
dram 2G:0x8000_0000~0xbfff_ffff ->0x1_8000_0000~0x1_bfff_ffff(Add bit32)
dram 3G:0xc000_0000~0xffff_ffff ->0x1_c000_0000~0x1_ffff_ffff(Add bit32)
dram 4G:0x1_0000_0000~0x1_3fff_ffff->0x1_0000_0000~0x1_3fff_ffff
Thus, in the 4GB mode, we should add always add bit9 in pte(for bit32
PA). But we can not always add bit32 in the iova_to_phys. The valid PA
range should be 0x4000_0000 - 0x1_3fff_ffff. Thus, we can only add bit32
when the PA in pte < 0x4000_0000, keep it as-is if the PA in pte located
from 0x4000_0000 to 0xffff_ffff.
This issue exist all the time after we added 4GB mode for mt8173.
Thus, I have to add a special flow for 4gb mode here:
/* Workaround for MTK 4GB Mode: Add BIT32 only when PA < 0x4000_0000.*/
if (cfg->oas == ARM_V7S_MTK_4GB_OAS && paddr < 0x40000000UL)
paddr |= BIT_ULL(32);
else if (pte & ARM_V7S_ATTR_MTK_PA_BIT32)
paddr |= BIT_ULL(32);
>
> return paddr;
>
> The diff I sent previously sanitises the oas at init time, and then you
> can just enforce it in map().
>
> Will
^ permalink raw reply
* Re: [PATCH v1 02/50] clk: samsung: add IDs for Exynos5420 NoC clocks
From: Lukasz Luba @ 2019-07-18 5:26 UTC (permalink / raw)
To: Chanwoo Choi, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, linux-clk
Cc: mark.rutland, b.zolnierkie, sboyd, mturquette, krzk, a.hajda,
robh+dt, kyungmin.park, kgene, myungjoo.ham, s.nawrocki,
m.szyprowski
In-Reply-To: <b8503015-732f-7ac5-1b25-50e943d27e55@samsung.com>
On 7/16/19 11:26 AM, Chanwoo Choi wrote:
> Hi,
>
> You don't need to make the separate patches according to
> the type of clock just in order to add the ID by handling them
> from devicetree.
>
> Please merge following patches to one patch
> - patch2, patch4~patch7, patch9, patch11, patch12, patch14, patch17
> and separate from patch13, patch15, patch16 for adding the ID
I agree. The patches will be squashed.
Regards,
Lukasz
>
>
> On 19. 7. 15. 오후 9:43, Lukasz Luba wrote:
>> The patch adds NoC WCORE clock IDs needed used for changing parent of the
>> main NoC clock from the DT device.
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>> drivers/clk/samsung/clk-exynos5420.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
>> index 16ad498e3f3f..d353870e7fda 100644
>> --- a/drivers/clk/samsung/clk-exynos5420.c
>> +++ b/drivers/clk/samsung/clk-exynos5420.c
>> @@ -463,7 +463,8 @@ static const struct samsung_fixed_factor_clock
>> static const struct samsung_mux_clock exynos5800_mux_clks[] __initconst = {
>> MUX(0, "mout_aclk400_isp", mout_group3_5800_p, SRC_TOP0, 0, 3),
>> MUX(0, "mout_aclk400_mscl", mout_group3_5800_p, SRC_TOP0, 4, 3),
>> - MUX(0, "mout_aclk400_wcore", mout_group2_5800_p, SRC_TOP0, 16, 3),
>> + MUX(CLK_MOUT_ACLK400_WCORE, "mout_aclk400_wcore", mout_group2_5800_p,
>> + SRC_TOP0, 16, 3),
>> MUX(0, "mout_aclk100_noc", mout_group1_5800_p, SRC_TOP0, 20, 2),
>>
>> MUX(0, "mout_aclk333_432_gscl", mout_group6_5800_p, SRC_TOP1, 0, 2),
>> @@ -548,7 +549,8 @@ static const struct samsung_mux_clock exynos5420_mux_clks[] __initconst = {
>>
>> MUX(0, "mout_aclk400_isp", mout_group1_p, SRC_TOP0, 0, 2),
>> MUX(0, "mout_aclk400_mscl", mout_group1_p, SRC_TOP0, 4, 2),
>> - MUX(0, "mout_aclk400_wcore", mout_group1_p, SRC_TOP0, 16, 2),
>> + MUX(CLK_MOUT_ACLK400_WCORE, "mout_aclk400_wcore", mout_group1_p,
>> + SRC_TOP0, 16, 2),
>> MUX(0, "mout_aclk100_noc", mout_group1_p, SRC_TOP0, 20, 2),
>>
>> MUX(0, "mout_aclk333_432_gscl", mout_group4_p, SRC_TOP1, 0, 2),
>> @@ -674,8 +676,8 @@ static const struct samsung_mux_clock exynos5x_mux_clks[] __initconst = {
>> SRC_TOP10, 8, 1),
>> MUX(0, "mout_sw_aclk200_fsys2", mout_sw_aclk200_fsys2_p,
>> SRC_TOP10, 12, 1),
>> - MUX(0, "mout_sw_aclk400_wcore", mout_sw_aclk400_wcore_p,
>> - SRC_TOP10, 16, 1),
>> + MUX(CLK_MOUT_SW_ACLK400_WCORE, "mout_sw_aclk400_wcore",
>> + mout_sw_aclk400_wcore_p, SRC_TOP10, 16, 1),
>> MUX(0, "mout_sw_aclk100_noc", mout_sw_aclk100_noc_p,
>> SRC_TOP10, 20, 1),
>> MUX(0, "mout_sw_pclk200_fsys", mout_sw_pclk200_fsys_p,
>>
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v1 04/50] clk: samsung: add IDs to manage aclk400_mscl in Exynos5420
From: Lukasz Luba @ 2019-07-18 5:21 UTC (permalink / raw)
To: Chanwoo Choi, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, linux-clk
Cc: mturquette, sboyd, b.zolnierkie, krzk, kgene, mark.rutland,
robh+dt, kyungmin.park, a.hajda, m.szyprowski, s.nawrocki,
myungjoo.ham
In-Reply-To: <ce268a6e-b450-c405-6fd0-4b4bc93def45@samsung.com>
Hi Chanwoo,
On 7/16/19 11:17 AM, Chanwoo Choi wrote:
> Hi,
>
> The patch4~patch7 just add the ID to control the clock from DT.
> You can squash them to one patch instead of splitting out according to
> the type of clock.
Thank you for the review. Yes, you are right it needs to be squashed.
Regards,
Lukasz
>
> On 19. 7. 15. 오후 9:43, Lukasz Luba wrote:
>> Add needed IDs to MUXes which are used from DT to properly set clock
>> hierarchy.
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>> drivers/clk/samsung/clk-exynos5420.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
>> index 361ee53fc9fc..8f1d39cb2f1e 100644
>> --- a/drivers/clk/samsung/clk-exynos5420.c
>> +++ b/drivers/clk/samsung/clk-exynos5420.c
>> @@ -462,7 +462,8 @@ static const struct samsung_fixed_factor_clock
>>
>> static const struct samsung_mux_clock exynos5800_mux_clks[] __initconst = {
>> MUX(0, "mout_aclk400_isp", mout_group3_5800_p, SRC_TOP0, 0, 3),
>> - MUX(0, "mout_aclk400_mscl", mout_group3_5800_p, SRC_TOP0, 4, 3),
>> + MUX(CLK_MOUT_ACLK400_MSCL, "mout_aclk400_mscl", mout_group3_5800_p,
>> + SRC_TOP0, 4, 3),
>> MUX(CLK_MOUT_ACLK400_WCORE, "mout_aclk400_wcore", mout_group2_5800_p,
>> SRC_TOP0, 16, 3),
>> MUX(0, "mout_aclk100_noc", mout_group1_5800_p, SRC_TOP0, 20, 2),
>> @@ -548,7 +549,8 @@ static const struct samsung_mux_clock exynos5420_mux_clks[] __initconst = {
>> TOP_SPARE2, 4, 1),
>>
>> MUX(0, "mout_aclk400_isp", mout_group1_p, SRC_TOP0, 0, 2),
>> - MUX(0, "mout_aclk400_mscl", mout_group1_p, SRC_TOP0, 4, 2),
>> + MUX(CLK_MOUT_ACLK400_MSCL, "mout_aclk400_mscl", mout_group1_p,
>> + SRC_TOP0, 4, 2),
>> MUX(CLK_MOUT_ACLK400_WCORE, "mout_aclk400_wcore", mout_group1_p,
>> SRC_TOP0, 16, 2),
>> MUX(0, "mout_aclk100_noc", mout_group1_p, SRC_TOP0, 20, 2),
>> @@ -670,8 +672,8 @@ static const struct samsung_mux_clock exynos5x_mux_clks[] __initconst = {
>>
>> MUX(0, "mout_sw_aclk400_isp", mout_sw_aclk400_isp_p,
>> SRC_TOP10, 0, 1),
>> - MUX(0, "mout_sw_aclk400_mscl", mout_sw_aclk400_mscl_p,
>> - SRC_TOP10, 4, 1),
>> + MUX(CLK_MOUT_SW_ACLK400_MSCL, "mout_sw_aclk400_mscl",
>> + mout_sw_aclk400_mscl_p, SRC_TOP10, 4, 1),
>> MUX(CLK_MOUT_SW_ACLK200, "mout_sw_aclk200", mout_sw_aclk200_p,
>> SRC_TOP10, 8, 1),
>> MUX(0, "mout_sw_aclk200_fsys2", mout_sw_aclk200_fsys2_p,
>>
>
>
^ permalink raw reply
* Re: [PATCH v1 03/50] clk: samsung: change parent of dout_aclk400_wcore in Exynos5420
From: Lukasz Luba @ 2019-07-18 5:19 UTC (permalink / raw)
To: Chanwoo Choi, devicetree, linux-kernel, linux-arm-kernel,
linux-samsung-soc, linux-clk
Cc: mturquette, sboyd, b.zolnierkie, krzk, kgene, mark.rutland,
robh+dt, kyungmin.park, a.hajda, m.szyprowski, s.nawrocki,
myungjoo.ham
In-Reply-To: <6d3361b5-9e1f-eb1a-aaa9-3ca15ed5ad1d@samsung.com>
Hi Chanwoo,
On 7/16/19 11:13 AM, Chanwoo Choi wrote:
> On 19. 7. 15. 오후 9:43, Lukasz Luba wrote:
>> Change parent of dout_aclk400_wcore to mout_aclk400_wcore which reflects
>> topology described in the RM.
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>> drivers/clk/samsung/clk-exynos5420.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
>> index d353870e7fda..361ee53fc9fc 100644
>> --- a/drivers/clk/samsung/clk-exynos5420.c
>> +++ b/drivers/clk/samsung/clk-exynos5420.c
>> @@ -577,7 +577,7 @@ static const struct samsung_mux_clock exynos5420_mux_clks[] __initconst = {
>>
>> static const struct samsung_div_clock exynos5420_div_clks[] __initconst = {
>> DIV(CLK_DOUT_ACLK400_WCORE, "dout_aclk400_wcore",
>> - "mout_aclk400_wcore_bpll", DIV_TOP0, 16, 3),
>> + "mout_aclk400_wcore", DIV_TOP0, 16, 3),
>> };
>>
>> static const struct samsung_gate_clock exynos5420_gate_clks[] __initconst = {
>>
>
> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
>
> If possible, you better to send it to stable mailing list
> with Fixes information.
Thank you, I will do it when finish the v2.
Regards,
Lukasz
^ permalink raw reply
* Re: [RFC,v3 6/9] media: platform: Add Mediatek ISP P1 V4L2 functions
From: Jungo Lin @ 2019-07-18 4:39 UTC (permalink / raw)
To: Tomasz Figa
Cc: devicetree, sean.cheng, frederic.chen, rynn.wu, srv_heupstream,
robh, ryan.yu, frankie.chiu, hverkuil, ddavenport, sj.huang,
linux-mediatek, laurent.pinchart, matthias.bgg, mchehab,
linux-arm-kernel, linux-media
In-Reply-To: <20190710095429.GA181405@chromium.org>
Hi, Tomasz:
On Wed, 2019-07-10 at 18:54 +0900, Tomasz Figa wrote:
> Hi Jungo,
>
> On Tue, Jun 11, 2019 at 11:53:41AM +0800, Jungo Lin wrote:
> > Implement standard V4L2 video driver that utilizes V4L2
> > and media framework APIs. In this driver, supports one media
> > device, one sub-device and seven video devices during
> > initialization. Moreover, it also connects with sensor and
> > seninf drivers with V4L2 async APIs.
> >
> > (The current metadata interface used in meta input and partial
> > meta nodes is only a temporary solution to kick off the driver
> > development and is not ready to be reviewed yet.)
> >
> > Signed-off-by: Jungo Lin <jungo.lin@mediatek.com>
> > ---
> > This patch depends on "media: support Mediatek sensor interface driver"[1].
> >
> > ISP P1 sub-device communicates with seninf sub-device with CIO.
> >
> > [1]. media: support Mediatek sensor interface driver
> > https://patchwork.kernel.org/cover/10979135/
> > ---
> > .../platform/mtk-isp/isp_50/cam/Makefile | 1 +
> > .../mtk-isp/isp_50/cam/mtk_cam-v4l2-util.c | 1674 +++++++++++++++++
> > .../mtk-isp/isp_50/cam/mtk_cam-v4l2-util.h | 173 ++
> > 3 files changed, 1848 insertions(+)
> > create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-v4l2-util.c
> > create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-v4l2-util.h
> >
>
> Thanks for the patch. Please see my comments inline.
>
> [snip]
>
Appreciate your comments on this patch.
Please check my replied inline.
> > +static void mtk_cam_req_try_isp_queue(struct mtk_cam_dev *cam_dev,
> > + struct media_request *new_req)
> > +{
> > + struct mtk_cam_dev_request *req, *req_safe, *cam_dev_req;
> > + struct device *dev = &cam_dev->pdev->dev;
> > +
> > + dev_dbg(dev, "%s new req:%d", __func__, !new_req);
> > +
> > + if (!cam_dev->streaming) {
> > + cam_dev_req = mtk_cam_req_to_dev_req(new_req);
> > + spin_lock(&cam_dev->req_lock);
> > + list_add_tail(&cam_dev_req->list, &cam_dev->req_list);
> > + spin_unlock(&cam_dev->req_lock);
> > + dev_dbg(dev, "%s: stream off, no ISP enqueue\n", __func__);
> > + return;
> > + }
> > +
> > + /* Normal enqueue flow */
> > + if (new_req) {
> > + mtk_isp_req_enqueue(dev, new_req);
> > + return;
> > + }
> > +
> > + /* Flush all media requests wehen first stream on */
> > + list_for_each_entry_safe(req, req_safe, &cam_dev->req_list, list) {
> > + list_del(&req->list);
> > + mtk_isp_req_enqueue(dev, &req->req);
> > + }
> > +}
>
> This will have to be redone, as per the other suggestions, but generally one
> would have a function that tries to queue as much as possible from a list to
> the hardware and another function that adds a request to the list and calls
> the first function.
>
We revised this function as below.
First to check the en-queue conditions:
a. stream on
b. The composer buffers in SCP are 3, so we only could has 3 jobs
at the same time.
Second, try to en-queue the frames in the pending job if possible and
move them into running job list if possible.
The request has been inserted into pending job in mtk_cam_req_validate
which is used to validate media_request.
void mtk_cam_dev_req_try_queue(struct mtk_cam_dev *cam_dev)
{
struct mtk_cam_dev_request *req, *req_prev;
struct list_head enqueue_job_list;
int buffer_cnt = atomic_read(&cam_dev->running_job_count);
unsigned long flags;
if (!cam_dev->streaming ||
buffer_cnt >= MTK_ISP_MAX_RUNNING_JOBS) {
dev_dbg(cam_dev->dev, "stream off or buffers are full:%d\n",
buffer_cnt);
return;
}
INIT_LIST_HEAD(&enqueue_job_list);
spin_lock(&cam_dev->pending_job_lock);
list_for_each_entry_safe(req, req_prev,
&cam_dev->pending_job_list, list) {
list_del(&req->list);
list_add_tail(&req->list, &enqueue_job_list);
if (atomic_inc_return(&cam_dev->running_job_count) >=
MTK_ISP_MAX_RUNNING_JOBS)
break;
}
spin_unlock(&cam_dev->pending_job_lock);
list_for_each_entry_safe(req, req_prev,
&enqueue_job_list, list) {
list_del(&req->list);
spin_lock_irqsave(&cam_dev->running_job_lock, flags);
list_add_tail(&req->list, &cam_dev->running_job_list);
spin_unlock_irqrestore(&cam_dev->running_job_lock, flags);
mtk_isp_req_enqueue(cam_dev, req);
}
}
> > +
> > +static void mtk_cam_req_queue(struct media_request *req)
> > +{
> > + struct mtk_cam_dev *cam_dev = mtk_cam_mdev_to_dev(req->mdev);
> > +
> > + vb2_request_queue(req);
> > + mtk_cam_req_try_isp_queue(cam_dev, req);
>
> Looks like this driver is suffering from versy similar problems in request
> handling as the DIP driver used to.
>
> I'd prefer to save my time and avoid repeating the same comments, so please
> check my comments for the DIP driver and apply them to this one too:
>
> https://patchwork.kernel.org/patch/10905223/
>
Yes, we will follow the same design of DIP and replace this function by
vb2_request_queue and defined new request structure.
/*
* struct mtk_cam_dev_request - MTK camera device request.
*
* @req: Embedded struct media request.
* @frame_params: The frame info. & address info. of enabled DMA nodes.
* @frame_work: work queue entry for frame transmission to SCP.
* @list: List entry of the object for @struct mtk_cam_dev:
* pending_job_list or running_job_list.
* @buf_count: Buffer count in this request.
*
*/
struct mtk_cam_dev_request {
struct media_request req;
struct mtk_p1_frame_param frame_params;
struct work_struct frame_work;
struct list_head list;
atomic_t buf_count;
};
> > +}
> > +
> > +static struct media_request *mtk_cam_req_alloc(struct media_device *mdev)
> > +{
> > + struct mtk_cam_dev_request *cam_dev_req;
> > +
> > + cam_dev_req = kzalloc(sizeof(*cam_dev_req), GFP_KERNEL);
> > +
> > + return &cam_dev_req->req;
> > +}
> > +
> > +static void mtk_cam_req_free(struct media_request *req)
> > +{
> > + struct mtk_cam_dev_request *cam_dev_req = mtk_cam_req_to_dev_req(req);
> > +
> > + kfree(cam_dev_req);
> > +}
> > +
> > +static __u32 img_get_pixel_byte_by_fmt(__u32 pix_fmt)
>
> Doesn't this function return bits not bytes?
>
Yes, the unit sould be bits, not bytes.
We will rename this function to get_pixel_bits.
static unsigned int get_pixel_bits(unsigned int pix_fmt)
> > +{
> > + switch (pix_fmt) {
> > + case V4L2_PIX_FMT_MTISP_B8:
> > + case V4L2_PIX_FMT_MTISP_F8:
> > + return 8;
> > + case V4L2_PIX_FMT_MTISP_B10:
> > + case V4L2_PIX_FMT_MTISP_F10:
> > + return 10;
> > + case V4L2_PIX_FMT_MTISP_B12:
> > + case V4L2_PIX_FMT_MTISP_F12:
> > + return 12;
> > + case V4L2_PIX_FMT_MTISP_B14:
> > + case V4L2_PIX_FMT_MTISP_F14:
> > + return 14;
> > + default:
> > + return 0;
> > + }
> > +}
> > +
> > +static __u32 img_cal_main_stream_stride(struct device *dev, __u32 width,
> > + __u32 pix_fmt)
> > +{
> > + __u32 stride;
> > + __u32 pixel_byte = img_get_pixel_byte_by_fmt(pix_fmt);
>
> The __ prefixed types should be used only inside UAPI. Please change the
> driver to use the normal ones.
>
Ok, we will fix our usage in our driver source code.
> > +
> > + width = ALIGN(width, 4);
>
> If there is some alignment requirement for width, it should be handled by
> TRY_/S_FMT and here we should already assume everything properly aligned.
>
We will follow your suggestion to move this code login in TRY_/S_FMT
functions.
> > + stride = ALIGN(DIV_ROUND_UP(width * pixel_byte, 8), 2);
> > +
> > + dev_dbg(dev, "main width:%d, stride:%d\n", width, stride);
> > +
> > + return stride;
> > +}
> > +
> > +static __u32 img_cal_packed_out_stride(struct device *dev, __u32 width,
> > + __u32 pix_fmt)
> > +{
> > + __u32 stride;
> > + __u32 pixel_byte = img_get_pixel_byte_by_fmt(pix_fmt);
> > +
> > + width = ALIGN(width, 4);
>
> Ditto.
>
Will fix.
> > + stride = DIV_ROUND_UP(width * 3, 2);
>
> Could we introduce a local variable for this intermediate value, so that its
> name could explain what the value is?
>
> > + stride = DIV_ROUND_UP(stride * pixel_byte, 8);
> > +
> > + if (pix_fmt == V4L2_PIX_FMT_MTISP_F10)
> > + stride = ALIGN(stride, 4);
>
> Is it expected that only the F10 format needs this alignment?
>
yes, if the pixel bits of image format is 10, the byte alignment of bpl
should be 4. Otherwise, it is 8. We will revise this and add more
comments.
/* 4 bytes alignment for 10 bit other are 8 bytes alignment */
if (pixel_bits == 10)
bpl = ALIGN(bpl, 4);
else
bpl = ALIGN(bpl, 8);
> > +
> > + dev_dbg(dev, "packed width:%d, stride:%d\n", width, stride);
> > +
> > + return stride;
> > +}
> > +
> > +static __u32 img_cal_stride(struct device *dev,
> > + int node_id,
> > + __u32 width,
> > + __u32 pix_fmt)
> > +{
> > + __u32 bpl;
> > +
> > + /* Currently, only support one_pixel_mode */
> > + if (node_id == MTK_CAM_P1_MAIN_STREAM_OUT)
> > + bpl = img_cal_main_stream_stride(dev, width, pix_fmt);
> > + else if (node_id == MTK_CAM_P1_PACKED_BIN_OUT)
> > + bpl = img_cal_packed_out_stride(dev, width, pix_fmt);
> > +
> > + /* For DIP HW constrained, it needs 4 byte alignment */
> > + bpl = ALIGN(bpl, 4);
> > +
> > + return bpl;
> > +}
> > +
> > +static const struct v4l2_format *
> > +mtk_cam_dev_find_fmt(struct mtk_cam_dev_node_desc *desc, u32 format)
> > +{
> > + unsigned int i;
> > + const struct v4l2_format *dev_fmt;
> > +
> > + for (i = 0; i < desc->num_fmts; i++) {
> > + dev_fmt = &desc->fmts[i];
> > + if (dev_fmt->fmt.pix_mp.pixelformat == format)
> > + return dev_fmt;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +/* Calcuate mplane pix format */
> > +static void
> > +mtk_cam_dev_cal_mplane_fmt(struct device *dev,
> > + struct v4l2_pix_format_mplane *dest_fmt,
> > + unsigned int node_id)
> > +{
> > + unsigned int i;
> > + __u32 bpl, sizeimage, imagsize;
>
> Perhaps s/sizeimage/plane_size/ and s/imagsize/total_size/?
>
Has fixed it.
Btw we will only support 1 plane, there is no need for plane_size.
> > +
> > + imagsize = 0;
> > + for (i = 0 ; i < dest_fmt->num_planes; ++i) {
> > + bpl = img_cal_stride(dev,
> > + node_id,
> > + dest_fmt->width,
> > + dest_fmt->pixelformat);
> > + sizeimage = bpl * dest_fmt->height;
> > + imagsize += sizeimage;
> > + dest_fmt->plane_fmt[i].bytesperline = bpl;
> > + dest_fmt->plane_fmt[i].sizeimage = sizeimage;
> > + memset(dest_fmt->plane_fmt[i].reserved,
> > + 0, sizeof(dest_fmt->plane_fmt[i].reserved));
>
> This memset is not needed. The core clears the reserved fields
> automatically:
>
> https://elixir.bootlin.com/linux/v5.2/source/drivers/media/v4l2-core/v4l2-ioctl.c#L1559
>
> (We may want to backport the patch that added that to our 4.19 branch.)
>
Ok, we will remove both memset functions in this function.
> > + dev_dbg(dev, "plane:%d,bpl:%d,sizeimage:%u\n",
> > + i, bpl, dest_fmt->plane_fmt[i].sizeimage);
> > + }
> > +
> > + if (dest_fmt->num_planes == 1)
> > + dest_fmt->plane_fmt[0].sizeimage = imagsize;
>
> Hmm, we only seem to support 1 plane raw formats in this driver. Does the
> hardware support any formats with more than 1 plane? If not, all the code
> should be simplified to just assume 1 plane.
>
No, MTK P1 ISP HW only supports raw formats with 1 plane.
We will revise our source codes to support only 1 plane.
> > +}
> > +
> > +static void
> > +mtk_cam_dev_set_img_fmt(struct device *dev,
> > + struct v4l2_pix_format_mplane *dest_fmt,
> > + const struct v4l2_pix_format_mplane *src_fmt,
> > + unsigned int node_id)
> > +{
> > + dest_fmt->width = src_fmt->width;
> > + dest_fmt->height = src_fmt->height;
> > + dest_fmt->pixelformat = src_fmt->pixelformat;
> > + dest_fmt->field = src_fmt->field;
> > + dest_fmt->colorspace = src_fmt->colorspace;
> > + dest_fmt->num_planes = src_fmt->num_planes;
> > + /* Use default */
> > + dest_fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > + dest_fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
> > + dest_fmt->xfer_func =
> > + V4L2_MAP_XFER_FUNC_DEFAULT(dest_fmt->colorspace);
> > + memset(dest_fmt->reserved, 0, sizeof(dest_fmt->reserved));
>
> Given that src_fmt should already be validated and have any fields adjusted
> to match the driver requirements, wouldn't all the lines above be equivalent
> to *dest_fmt = *src_fmt?
>
> We probably want to move setting all the constant fields to
> mtk_cam_vidioc_try_fmt().
>
Ok, we will remove mtk_cam_dev_set_img_fmt function
and use *dest_fmt = *src_fmt directly in the caller.
Moreover, all the constant fields are moved to mtk_cam_vidioc_try_fmt()
function.
> > +
> > + dev_dbg(dev, "%s: Dest Fmt:%c%c%c%c, w*h:%d*%d\n",
> > + __func__,
> > + (dest_fmt->pixelformat & 0xFF),
> > + (dest_fmt->pixelformat >> 8) & 0xFF,
> > + (dest_fmt->pixelformat >> 16) & 0xFF,
> > + (dest_fmt->pixelformat >> 24) & 0xFF,
> > + dest_fmt->width,
> > + dest_fmt->height);
> > +
> > + mtk_cam_dev_cal_mplane_fmt(dev, dest_fmt, node_id);
>
> This should have been called already before this function was called,
> because src_fmt should be already expected to contain valid settings. In
> fact, this is already called in mtk_cam_vidioc_try_fmt().
>
Ok, we will revise this.
> > +}
> > +
> > +/* Get the default format setting */
> > +static void
> > +mtk_cam_dev_load_default_fmt(struct device *dev,
>
> Please don't pass struct device pointer around, but instead just the main
> driver data struct, which should be much more convenient for accessing
> various driver data. Please fix the other functions as well.
>
Ok, we will revise this coding style in our source codes.
> > + struct mtk_cam_dev_node_desc *queue_desc,
> > + struct v4l2_format *dest)
> > +{
> > + const struct v4l2_format *default_fmt =
> > + &queue_desc->fmts[queue_desc->default_fmt_idx];
> > +
> > + dest->type = queue_desc->buf_type;
> > +
> > + /* Configure default format based on node type */
> > + if (queue_desc->image) {
> > + mtk_cam_dev_set_img_fmt(dev,
> > + &dest->fmt.pix_mp,
> > + &default_fmt->fmt.pix_mp,
> > + queue_desc->id);
>
> We should probably just call mtk_cam_vidioc_s_fmt() here, with a dummy
> v4l2_format struct and have any incorrect fields replaced by
> mtk_cam_vidioc_try_fmt(), since it's the same logic, as if setting invalid
> v4l2_format at runtime.
>
Ok, we will revise this.
> > + } else {
> > + dest->fmt.meta.dataformat = default_fmt->fmt.meta.dataformat;
> > + dest->fmt.meta.buffersize = default_fmt->fmt.meta.buffersize;
> > + }
> > +}
> > +
> > +static int mtk_cam_isp_open(struct file *file)
> > +{
> > + struct mtk_cam_dev *cam_dev = video_drvdata(file);
> > + struct device *dev = &cam_dev->pdev->dev;
> > + struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
> > + int ret;
> > +
> > + mutex_lock(&cam_dev->lock);
> > + ret = v4l2_fh_open(file);
> > + if (ret)
> > + goto unlock;
> > +
> > + ret = v4l2_pipeline_pm_use(&node->vdev.entity, 1);
>
> Please don't power on open. Normally applications keep the device nodes open
> all the time, so they would keep everything powered on.
>
> Normally this should be done as late as possible, ideally when starting the
> streaming.
>
Ok, we will remove this function and just call 4l2_fh_open(file)
function.
> > + if (ret)
> > + dev_err(dev, "%s fail:%d", __func__, ret);
> > +
> > +unlock:
> > + mutex_unlock(&cam_dev->lock);
> > +
> > + return ret;
> > +}
> > +
> > +static int mtk_cam_isp_release(struct file *file)
> > +{
> > + struct mtk_cam_dev *cam_dev = video_drvdata(file);
> > + struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
> > +
> > + mutex_lock(&cam_dev->lock);
> > + v4l2_pipeline_pm_use(&node->vdev.entity, 0);
> > + vb2_fop_release(file);
> > + mutex_unlock(&cam_dev->lock);
> > +
> > + return 0;
> > +}
>
> If we remove power handling from open and release, we should be able to just
> use v4l2_fh_open() and vb2_fop_release() directly in the
> v4l2_file_operations struct.
>
Ok, we will fix this.
> > +
> > +static struct v4l2_subdev *
> > +mtk_cam_cio_get_active_sensor(struct mtk_cam_dev *cam_dev)
> > +{
> > + struct media_device *mdev = cam_dev->seninf->entity.graph_obj.mdev;
> > + struct media_entity *entity;
> > + struct device *dev = &cam_dev->pdev->dev;
> > + struct v4l2_subdev *sensor;
>
> This variable would be unitialized if there is no streaming sensor. Was
> there no compiler warning generated for this?
>
No, there is no compiler warning.
But, we will assign sensor to NULL to avoid unnecessary compiler warning
with different compiler options.
> > +
> > + media_device_for_each_entity(entity, mdev) {
> > + dev_dbg(dev, "media entity: %s:0x%x\n",
> > + entity->name, entity->function);
> > + if (entity->function == MEDIA_ENT_F_CAM_SENSOR &&
> > + entity->stream_count) {
> > + sensor = media_entity_to_v4l2_subdev(entity);
> > + dev_dbg(dev, "Sensor found: %s\n", entity->name);
> > + break;
> > + }
> > + }
> > +
> > + if (!sensor)
> > + dev_err(dev, "Sensor is not connected\n");
> > +
> > + return sensor;
> > +}
> > +
> > +static int mtk_cam_cio_stream_on(struct mtk_cam_dev *cam_dev)
> > +{
> > + struct device *dev = &cam_dev->pdev->dev;
> > + int ret;
> > +
> > + /* Align vb2_core_streamon design */
> > + if (cam_dev->streaming) {
> > + dev_warn(dev, "already streaming\n", dev);
> > + return 0;
> > + }
>
> Could we check this in the caller?
>
Ok, we will move this logic check in the mtk_cam_sd_s_stream function.
> > +
> > + if (!cam_dev->seninf) {
> > + dev_err(dev, "no seninf connected:%d\n", ret);
> > + return -EPERM;
>
> I don't think -EPERM is a good error code here. It's about a missing seninf
> device, so perhaps -ENODEV?
>
Fix it in next patch.
> > + }
> > +
> > + /* Get active sensor from graph topology */
> > + cam_dev->sensor = mtk_cam_cio_get_active_sensor(cam_dev);
> > + if (!cam_dev->sensor)
> > + return -EPERM;
>
> > -ENODEV
>
> > +
> > + ret = mtk_isp_config(dev);
> > + if (ret)
> > + return -EPERM;
>
> Maybe just return ret?
>
Fix it in next patch.
> > +
> > + /* Seninf must stream on first */
> > + ret = v4l2_subdev_call(cam_dev->seninf, video, s_stream, 1);
> > + if (ret) {
> > + dev_err(dev, "%s stream on failed:%d\n",
> > + cam_dev->seninf->entity.name, ret);
> > + return -EPERM;
>
> return ret?
>
Fix it in next patch.
> > + }
> > +
> > + ret = v4l2_subdev_call(cam_dev->sensor, video, s_stream, 1);
> > + if (ret) {
> > + dev_err(dev, "%s stream on failed:%d\n",
> > + cam_dev->sensor->entity.name, ret);
> > + goto fail_sensor_on;
> > + }
> > +
> > + cam_dev->streaming = true;
> > + mtk_cam_req_try_isp_queue(cam_dev, NULL);
> > + isp_composer_stream(dev, 1);
> > + dev_dbg(dev, "streamed on Pass 1\n");
> > +
> > + return 0;
> > +
> > +fail_sensor_on:
> > + v4l2_subdev_call(cam_dev->seninf, video, s_stream, 0);
> > +
> > + return -EPERM;
>
> return ret?
>
Fix it in next patch.
> > +}
> > +
> > +static int mtk_cam_cio_stream_off(struct mtk_cam_dev *cam_dev)
> > +{
> > + struct device *dev = &cam_dev->pdev->dev;
> > + int ret;
> > +
> > + if (!cam_dev->streaming) {
> > + dev_warn(dev, "already stream off");
> > + return 0;
> > + }
>
> Could we check this in the caller?
>
Ditto.
> > +
> > + ret = v4l2_subdev_call(cam_dev->sensor, video, s_stream, 0);
> > + if (ret) {
> > + dev_err(dev, "%s stream off failed:%d\n",
> > + cam_dev->sensor->entity.name, ret);
> > + return -EPERM;
> > + }
> > +
> > + ret = v4l2_subdev_call(cam_dev->seninf, video, s_stream, 0);
> > + if (ret) {
> > + dev_err(dev, "%s stream off failed:%d\n",
> > + cam_dev->seninf->entity.name, ret);
> > + return -EPERM;
> > + }
> > +
> > + isp_composer_stream(dev, 0);
>
> Shouldn't we synchronously wait for the streaming to stop here? Otherwise we
> can't guarantee that the hardware releases all the memory that we're going
> to free once this function returns.
>
We will add below functions.
1. Stream off ISP HW
2. Stop ISP HW
3. Clear all pending & running request lists.
cam_dev->streaming = false;
mtk_isp_stream(cam_dev, 0);
mtk_isp_hw_release(cam_dev);
mtk_cam_dev_req_clear(cam_dev);
dev_dbg(dev, "streamed off Pass 1\n");
> > + cam_dev->streaming = false;
> > + dev_dbg(dev, "streamed off Pass 1\n");
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_cam_sd_s_stream(struct v4l2_subdev *sd, int enable)
> > +{
> > + struct mtk_cam_dev *cam_dev = mtk_cam_subdev_to_dev(sd);
> > +
> > + if (enable)
> > + return mtk_cam_cio_stream_on(cam_dev);
> > + else
> > + return mtk_cam_cio_stream_off(cam_dev);
> > +}
> > +
> > +static int mtk_cam_sd_subscribe_event(struct v4l2_subdev *subdev,
> > + struct v4l2_fh *fh,
> > + struct v4l2_event_subscription *sub)
> > +{
> > + switch (sub->type) {
> > + case V4L2_EVENT_FRAME_SYNC:
> > + return v4l2_event_subscribe(fh, sub, 0, NULL);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int mtk_cam_sd_s_power(struct v4l2_subdev *sd, int on)
> > +{
> > + struct mtk_cam_dev *cam_dev = mtk_cam_subdev_to_dev(sd);
> > +
> > + dev_dbg(&cam_dev->pdev->dev, "%s:%d", __func__, on);
> > +
> > + return on ? mtk_isp_power_init(cam_dev) :
> > + mtk_isp_power_release(&cam_dev->pdev->dev);
>
> s_power is a historical thing and we shouldn't be implementing it. Instead,
> we should use runtime PM and call pm_runtime_get_sync(), pm_runtime_put()
> whenever we start and stop streaming respectively.
>
Ok, we will remove this callback function.
> > +}
> > +
> > +static int mtk_cam_media_link_setup(struct media_entity *entity,
> > + const struct media_pad *local,
> > + const struct media_pad *remote, u32 flags)
> > +{
> > + struct mtk_cam_dev *cam_dev =
> > + container_of(entity, struct mtk_cam_dev, subdev.entity);
> > + u32 pad = local->index;
> > +
> > + dev_dbg(&cam_dev->pdev->dev, "%s: %d -> %d flags:0x%x\n",
> > + __func__, pad, remote->index, flags);
> > +
> > + if (pad < MTK_CAM_P1_TOTAL_NODES)
>
> I assume this check is needed, because the pads with higher indexes are not
> video nodes? If so, a comment would be helpful here.
>
Yes, we will new comment as below.
/*
* Check video nodes is enabled by link setup.
* The pad index of video node should be less than
* MTK_CAM_P1_TOTAL_NODES.
*/
if (pad < MTK_CAM_P1_TOTAL_NODES)
cam_dev->vdev_nodes[pad].enabled =
!!(flags & MEDIA_LNK_FL_ENABLED);
> > + cam_dev->vdev_nodes[pad].enabled =
> > + !!(flags & MEDIA_LNK_FL_ENABLED);
> > +
> > + return 0;
> > +}
> > +
> > +static void mtk_cam_vb2_buf_queue(struct vb2_buffer *vb)
> > +{
> > + struct mtk_cam_dev *mtk_cam_dev = vb2_get_drv_priv(vb->vb2_queue);
> > + struct mtk_cam_video_device *node = mtk_cam_vbq_to_vdev(vb->vb2_queue);
> > + struct device *dev = &mtk_cam_dev->pdev->dev;
> > + struct mtk_cam_dev_buffer *buf;
> > +
> > + buf = mtk_cam_vb2_buf_to_dev_buf(vb);
>
> This can be folded into the declaration.
>
Fix it in next patch.
> > +
> > + dev_dbg(dev, "%s: node:%d fd:%d idx:%d\n",
> > + __func__,
> > + node->id,
> > + buf->vbb.request_fd,
> > + buf->vbb.vb2_buf.index);
> > +
> > + /* For request buffers en-queue, handled in mtk_cam_req_try_queue */
> > + if (vb->vb2_queue->uses_requests)
> > + return;
>
> I'd suggest removing non-request support from this driver. Even if we end up
> with a need to provide compatibility for non-request mode, then it should be
> built on top of the requests mode, so that the driver itself doesn't have to
> deal with two modes.
>
The purpose of non-request function in this driver is needed by
our camera middle-ware design. It needs 3A statistics buffers before
image buffers en-queue. So we need to en-queue 3A statistics with
non-request mode in this driver. After MW got the 3A statistics data, it
will en-queue the images, tuning buffer and other meta buffers with
request mode. Based on this requirement, do you have any suggestion?
For upstream driver, should we only consider request mode?
> > +
> > + /* Added the buffer into the tracking list */
> > + spin_lock(&node->slock);
> > + list_add_tail(&buf->list, &node->pending_list);
> > + spin_unlock(&node->slock);
> > +
> > + mtk_isp_enqueue(dev, node->desc.dma_port, buf);
> > +}
> > +
> > +static int mtk_cam_vb2_buf_init(struct vb2_buffer *vb)
> > +{
> > + struct mtk_cam_dev *cam_dev = vb2_get_drv_priv(vb->vb2_queue);
> > + struct device *smem_dev = cam_dev->smem_dev;
> > + struct mtk_cam_video_device *node = mtk_cam_vbq_to_vdev(vb->vb2_queue);
> > + struct mtk_cam_dev_buffer *buf;
> > +
> > + buf = mtk_cam_vb2_buf_to_dev_buf(vb);
> > + buf->node_id = node->id;
> > + buf->daddr = vb2_dma_contig_plane_dma_addr(&buf->vbb.vb2_buf, 0);
> > + buf->scp_addr = 0;
>
> Just a reminder that this will have to be reworked according to my comments
> for the memory allocation patch.
>
Yes, we have revised this implementation according to the review of
below patch set.
https://patchwork.kernel.org/patch/10985833/
> > +
> > + /* scp address is only valid for meta input buffer */
> > + if (node->desc.smem_alloc)
> > + buf->scp_addr = mtk_cam_smem_iova_to_scp_addr(smem_dev,
> > + buf->daddr);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_cam_vb2_buf_prepare(struct vb2_buffer *vb)
> > +{
> > + struct vb2_v4l2_buffer *v4l2_buf = to_vb2_v4l2_buffer(vb);
> > + struct mtk_cam_video_device *node = mtk_cam_vbq_to_vdev(vb->vb2_queue);
> > + const struct v4l2_format *fmt = &node->vdev_fmt;
> > + unsigned int size;
> > +
> > + if (vb->vb2_queue->type == V4L2_BUF_TYPE_META_OUTPUT ||
> > + vb->vb2_queue->type == V4L2_BUF_TYPE_META_CAPTURE)
> > + size = fmt->fmt.meta.buffersize;
> > + else
> > + size = fmt->fmt.pix_mp.plane_fmt[0].sizeimage;
> > +
> > + if (vb2_plane_size(vb, 0) < size)
> > + return -EINVAL;
>
> For OUTPUT buffers we need to check if vb2_get_plane_payload() == size.
> Otherwise we could get not enough or invalid data.
>
Fixed in next patch.
> > +
> > + v4l2_buf->field = V4L2_FIELD_NONE;
> > + vb2_set_plane_payload(vb, 0, size);
>
> This shouldn't be called on OUTPUT buffers.
>
Ditto.
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_cam_vb2_queue_setup(struct vb2_queue *vq,
> > + unsigned int *num_buffers,
> > + unsigned int *num_planes,
> > + unsigned int sizes[],
> > + struct device *alloc_devs[])
> > +{
> > + struct mtk_cam_video_device *node = mtk_cam_vbq_to_vdev(vq);
> > + unsigned int max_buffer_count = node->desc.max_buf_count;
> > + const struct v4l2_format *fmt = &node->vdev_fmt;
> > + unsigned int size;
> > +
> > + /* Check the limitation of buffer size */
> > + if (max_buffer_count)
> > + *num_buffers = clamp_val(*num_buffers, 1, max_buffer_count);
> > +
> > + if (vq->type == V4L2_BUF_TYPE_META_OUTPUT ||
> > + vq->type == V4L2_BUF_TYPE_META_CAPTURE)
> > + size = fmt->fmt.meta.buffersize;
> > + else
> > + size = fmt->fmt.pix_mp.plane_fmt[0].sizeimage;
> > +
> > + /* Add for q.create_bufs with fmt.g_sizeimage(p) / 2 test */
> > + if (*num_planes) {
>
> We should also verify that *num_planes == 1, as we don't support more
> planes in this driver.
>
Ok, here is new check logic.
if (*num_planes) {
if (sizes[0] < size || *num_planes != 1)
return -EINVAL;
} else {
*num_planes = 1;
sizes[0] = size;
}
> > + if (sizes[0] < size)
> > + return -EINVAL;
> > + } else {
> > + *num_planes = 1;
> > + sizes[0] = size;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void mtk_cam_vb2_return_all_buffers(struct mtk_cam_dev *cam_dev,
> > + struct mtk_cam_video_device *node,
> > + enum vb2_buffer_state state)
> > +{
> > + struct mtk_cam_dev_buffer *b, *b0;
> > + struct mtk_cam_dev_request *req, *req0;
> > + struct media_request_object *obj, *obj0;
> > + struct vb2_buffer *vb;
> > +
> > + dev_dbg(&cam_dev->pdev->dev, "%s: node:%s", __func__, node->vdev.name);
> > +
> > + /* Return all buffers */
> > + spin_lock(&node->slock);
> > + list_for_each_entry_safe(b, b0, &node->pending_list, list) {
>
> nit: One would normally call the second argument "prev", or "b_prev".
>
Ok, we will follow this coding convention in our source codes.
> > + vb = &b->vbb.vb2_buf;
> > + if (vb->state == VB2_BUF_STATE_ACTIVE)
>
> We shouldn't need to check the buffer state.
>
Fixed in next patch.
> > + vb2_buffer_done(vb, state);
> > + list_del(&b->list);
> > + }
> > + spin_unlock(&node->slock);
> > +
> > + spin_lock(&cam_dev->req_lock);
> > + list_for_each_entry_safe(req, req0, &cam_dev->req_list, list) {
>
> nit: Ditto.
>
Fixed in next patch.
> > + list_for_each_entry_safe(obj, obj0, &req->req.objects, list) {
>
> Need to check if the object is a buffer.
>
Fixed in next patch.
> > + vb = container_of(obj, struct vb2_buffer, req_obj);
> > + if (vb->state == VB2_BUF_STATE_ACTIVE)
>
> vb->state shouldn't be accessed directly from the drivers.
>
nit: Ditto.
> Generally, the need to check the state here would suggest that there is
> something wrong with how the driver manages the requests. The list that is
> being iterated here shouldn't contain any requests that have buffers that
> aren't active. That will be achieved if my comments for the request handling
> in the DIP driver are applied to this driver as well.
>
> > + vb2_buffer_done(vb, state);
> > + }
> > + list_del(&req->list);
> > + }
> > + spin_unlock(&cam_dev->req_lock);
> > +
> > + if (node->vbq.uses_requests)
> > + mtk_isp_req_flush_buffers(&cam_dev->pdev->dev);
> > +}
> > +
> > +static int mtk_cam_vb2_start_streaming(struct vb2_queue *vq,
> > + unsigned int count)
> > +{
> > + struct mtk_cam_dev *cam_dev = vb2_get_drv_priv(vq);
> > + struct mtk_cam_video_device *node = mtk_cam_vbq_to_vdev(vq);
> > + struct device *dev = &cam_dev->pdev->dev;
> > + unsigned int node_count = cam_dev->subdev.entity.use_count;
> > + int ret;
> > +
> > + if (!node->enabled) {
>
> How is this synchronized with mtk_cam_media_link_setup()?
>
We will follow your suggestion and below is our proposal for this
function.
1. Use !cam_dev->pipeline.streaming_count to decide the first node to
stream-on.
2.a If yes, do the following steps
2.a-1 Call media_pipeline_start function to prevent the link
configuration changes.
2.a-2 Call mtk_cam_dev_init_stream function to calculate how many
video nodes are enabled and save it into cam_dev->enabled_node_count.
2.a-3 Initialize ISP P1 HW in mtk_isp_hw_init function since end
user has called stream-on API
2.b jump step 3.
3. Use cam_dev->streamed_node_count to track how many video nodes are
streamed by user space.
4. Check all enabled video nodes are streamed or not based on
cam_dev->streamed_node_count & cam_dev->enabled_node_count.
5. If yes, call s_stream on for P1 sub-device
Do you think it is reasonable?
> > + dev_err(dev, "Node:%d is not enable\n", node->id);
> > + ret = -ENOLINK;
> > + goto fail_no_link;
> > + }
> > +
> > + dev_dbg(dev, "%s: count info:%d:%d", __func__,
> > + atomic_read(&cam_dev->streamed_node_count), node_count);
> > +
> > + if (atomic_inc_return(&cam_dev->streamed_node_count) < node_count)
> > + return 0;
>
> How do we guarantee that cam_dev->subdev.entity.use_count doesn't change
> between calls to this function on different video nodes?
>
Ditto.
> > +
> > + /* Start streaming of the whole pipeline now */
> > + ret = media_pipeline_start(&node->vdev.entity, &cam_dev->pipeline);
> > + if (ret) {
> > + dev_err(dev, "%s: Node:%d failed\n", __func__, node->id);
> > + goto fail_start_pipeline;
> > + }
> > +
>
> Related to the above comment: If we start the media pipeline when we start
> streaming on the first node, we would naturally prevent the link
> configuration changes until the last node stops streaming (as long as the
> link is not DYNAMIC). Note that it would only mark the entities as
> streaming, but it wouldn't call their s_stream, which I believe is exactly
> what we would need to solve the problem above.
>
Ditto.
> > + /* Stream on sub-devices node */
> > + ret = v4l2_subdev_call(&cam_dev->subdev, video, s_stream, 1);
> > + if (ret) {
> > + dev_err(dev, "Node:%d s_stream on failed:%d\n", node->id, ret);
> > + goto fail_stream_on;
> > + }
> > +
> > + return 0;
> > +
> > +fail_stream_on:
> > + media_pipeline_stop(&node->vdev.entity);
> > +fail_start_pipeline:
> > + atomic_dec(&cam_dev->streamed_node_count);
> > +fail_no_link:
> > + mtk_cam_vb2_return_all_buffers(cam_dev, node, VB2_BUF_STATE_QUEUED);
> > +
> > + return ret;
> > +}
> > +
> > +static void mtk_cam_vb2_stop_streaming(struct vb2_queue *vq)
> > +{
> > + struct mtk_cam_dev *cam_dev = vb2_get_drv_priv(vq);
> > + struct mtk_cam_video_device *node = mtk_cam_vbq_to_vdev(vq);
> > + struct device *dev = &cam_dev->pdev->dev;
> > +
> > + if (!node->enabled)
> > + return;
>
> It shouldn't be possible for this to happen, because nobody could have
> called start_streaming on a disabled node.
>
Will remove in next patch.
> > +
> > + mtk_cam_vb2_return_all_buffers(cam_dev, node, VB2_BUF_STATE_ERROR);
>
> Shouldn't we stop streaming first, so that the hardware operation is
> cancelled and any buffers owned by the hardware are released?
>
For this function, below is the new code flow.
1. Check the first node to stream off based on
cam_dev->streamed_node_count & cam_dev->enabled_node_count.
2. If yes, call all s_stream off for P1 sub-device
3. Call mtk_cam_vb2_return_all_buffers for each node
4. Check the last node to stream off
5. If yes, call media_pipeline_stop to allow user space
to perform link configuration changes, such as disable link.
But, for step 5, is it too late for end user to disable link?
For example, for first node, it has called stream off but
can't call disable link until the last node is stream off?
> > +
> > + dev_dbg(dev, "%s: count info:%d", __func__,
> > + cam_dev->subdev.entity.stream_count);
> > +
> > + /* Check the first node to stream-off */
> > + if (!cam_dev->subdev.entity.stream_count)
> > + return;
> > +
> > + media_pipeline_stop(&node->vdev.entity);
> > +
> > + if (v4l2_subdev_call(&cam_dev->subdev, video, s_stream, 0))
> > + dev_err(dev, "failed to stop streaming\n");
> > +}
> > +
> > +static void mtk_cam_vb2_buf_request_complete(struct vb2_buffer *vb)
> > +{
> > + struct mtk_cam_dev *dev = vb2_get_drv_priv(vb->vb2_queue);
> > +
> > + v4l2_ctrl_request_complete(vb->req_obj.req,
> > + dev->v4l2_dev.ctrl_handler);
>
> This would end up being called multiple times, once for each video node.
> Instead, this should be called explicitly by the driver when it completed
> the request - perhaps in the frame completion handler?
>
> With that, we probably wouldn't even need this callback.
>
First, if we don't implement this callback function, we will receive
kernel warning as below.
https://elixir.bootlin.com/linux/latest/source/drivers/media/common/videobuf2/videobuf2-v4l2.c#L420
Second, this function is only be called in __vb2_queue_cancel function.
Moreover, we will remove cam_dev->v4l2_dev.ctrl_handler in next patch.
So could we just implement dummy empty function?
* @buf_request_complete: a buffer that was never queued to the driver
but is
* associated with a queued request was canceled.
* The driver will have to mark associated objects in the
* request as completed; required if requests are
* supported.
> > +}
> > +
> > +static int mtk_cam_vidioc_querycap(struct file *file, void *fh,
> > + struct v4l2_capability *cap)
> > +{
> > + struct mtk_cam_dev *cam_dev = video_drvdata(file);
> > +
> > + strscpy(cap->driver, MTK_CAM_DEV_P1_NAME, sizeof(cap->driver));
> > + strscpy(cap->card, MTK_CAM_DEV_P1_NAME, sizeof(cap->card));
>
> We could just use dev_driver_name(cam_dev->dev) for both.
>
> > + snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
> > + dev_name(cam_dev->media_dev.dev));
>
> We should just store dev in cam_dev.
>
Will fix in next patch.
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_cam_vidioc_enum_fmt(struct file *file, void *fh,
> > + struct v4l2_fmtdesc *f)
> > +{
> > + struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
> > +
> > + if (f->index >= node->desc.num_fmts)
> > + return -EINVAL;
> > +
> > + f->pixelformat = node->desc.fmts[f->index].fmt.pix_mp.pixelformat;
>
> Is the set of formats available always the same regardless of the sensor
> format?
>
Yes, ISP P1 HW output formats are always available without impact
by sensor formats.
> > + f->flags = 0;
>
> We need f->description too.
>
For this description, do you suggest 1). we fill this field in this
function or 2). v4l_fill_fmtdesc function in v4l2-ioctl?
https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-ioctl.c#L1152
Basically, we prefer method 1.
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_cam_vidioc_g_fmt(struct file *file, void *fh,
> > + struct v4l2_format *f)
> > +{
> > + struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
> > +
> > + if (!node->desc.num_fmts)
> > + return -EINVAL;
>
> When would that condition happen?
>
Will remove this in next patch.
> > +
> > + f->fmt = node->vdev_fmt.fmt;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_cam_vidioc_try_fmt(struct file *file, void *fh,
> > + struct v4l2_format *in_fmt)
> > +{
> > + struct mtk_cam_dev *cam_dev = video_drvdata(file);
> > + struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
> > + const struct v4l2_format *dev_fmt;
> > + __u32 width, height;
>
> Don't use __ types in implementation, they are here for UAPI purposes. There
> is u32, which you could use instead, but for width and height you don't need
> explicit size, so unsigned int should be good.
>
Ok, we will revise this in next patch.
> > +
> > + dev_dbg(&cam_dev->pdev->dev, "%s: fmt:%c%c%c%c, w*h:%u*%u\n",
> > + __func__,
> > + (in_fmt->fmt.pix_mp.pixelformat & 0xFF),
> > + (in_fmt->fmt.pix_mp.pixelformat >> 8) & 0xFF,
> > + (in_fmt->fmt.pix_mp.pixelformat >> 16) & 0xFF,
> > + (in_fmt->fmt.pix_mp.pixelformat >> 24) & 0xFF,
> > + in_fmt->fmt.pix_mp.width, in_fmt->fmt.pix_mp.height);
> > +
> > + width = in_fmt->fmt.pix_mp.width;
> > + height = in_fmt->fmt.pix_mp.height;
> > +
> > + dev_fmt = mtk_cam_dev_find_fmt(&node->desc,
> > + in_fmt->fmt.pix_mp.pixelformat);
> > + if (dev_fmt) {
> > + mtk_cam_dev_set_img_fmt(&cam_dev->pdev->dev,
> > + &in_fmt->fmt.pix_mp,
> > + &dev_fmt->fmt.pix_mp,
> > + node->id);
> > + } else {
> > + mtk_cam_dev_load_default_fmt(&cam_dev->pdev->dev,
> > + &node->desc, in_fmt);
>
> We shouldn't just load a default format. This function should validate all
> the fields one by one and adjust them to something appropriate.
>
For ISP P1 HW, we only cares these fields of v4l2_pix_format_mplane.
a. width
b. height
c. pixelformat
d. plane_fmt
- sizeimage
- bytesperline
e. num_planes
Other fields are consider constant.
So if the user space passes one pixel format with un-supported, we will
apply the default format firstly and adjust width, height, sizeimage,
and bytesperline. We will focus on validate width & height.
Is it ok?
> > + }
>
> CodingStyle: No braces if both if and else bodies have only 1 statement
> each.
>
Will fix coding style in next patch.
> > + in_fmt->fmt.pix_mp.width = clamp_t(u32,
> > + width,
> > + CAM_MIN_WIDTH,
> > + in_fmt->fmt.pix_mp.width);
>
> Shouldn't we clamp this with some maximum value too?
>
Ok, will revise as below:
try_fmt.fmt.pix_mp.width = clamp_val(f->fmt.pix_mp.width,
IMG_MIN_WIDTH, IMG_MAX_WIDTH);
try_fmt.fmt.pix_mp.height = clamp_val(f->fmt.pix_mp.height,
IMG_MAX_HEIGHT, IMG_MIN_HEIGHT);
> > + in_fmt->fmt.pix_mp.height = clamp_t(u32,
> > + height,
> > + CAM_MIN_HEIGHT,
> > + in_fmt->fmt.pix_mp.height);
>
> Ditto.
>
Ditto.
> > + mtk_cam_dev_cal_mplane_fmt(&cam_dev->pdev->dev,
> > + &in_fmt->fmt.pix_mp, node->id);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_cam_vidioc_s_fmt(struct file *file, void *fh,
> > + struct v4l2_format *f)
> > +{
> > + struct mtk_cam_dev *cam_dev = video_drvdata(file);
> > + struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
> > +
> > + if (cam_dev->streaming)
> > + return -EBUSY;
>
> I think this should rather be something like vb2_queue_is_busy(), which
> would prevent format changes if buffers are allocated.
>
Since vb2_queue_is_busy is static function, would we paste its
implementation in this function to check like this?
if (node->vdev.queue->owner &&
node->vdev.queue->owner != file->private_data) {
dev_err(cam_dev->dev, "%s err: buffer allocated\n", __func__);
return -EBUSY;
}
> > +
> > + /* Get the valid format */
> > + mtk_cam_vidioc_try_fmt(file, fh, f);
> > +
> > + /* Configure to video device */
> > + mtk_cam_dev_set_img_fmt(&cam_dev->pdev->dev,
> > + &node->vdev_fmt.fmt.pix_mp,
> > + &f->fmt.pix_mp,
> > + node->id);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_cam_vidioc_enum_input(struct file *file, void *fh,
> > + struct v4l2_input *input)
> > +{
> > + if (input->index)
> > + return -EINVAL;
> > +
> > + strscpy(input->name, "camera", sizeof(input->name));
> > + input->type = V4L2_INPUT_TYPE_CAMERA;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_cam_vidioc_g_input(struct file *file, void *fh,
> > + unsigned int *input)
> > +{
> > + *input = 0;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_cam_vidioc_s_input(struct file *file,
> > + void *fh, unsigned int input)
> > +{
> > + return input == 0 ? 0 : -EINVAL;
> > +}
> > +
> > +static int mtk_cam_vidioc_enum_framesizes(struct file *filp, void *priv,
> > + struct v4l2_frmsizeenum *sizes)
> > +{
> > + struct mtk_cam_video_device *node = file_to_mtk_cam_node(filp);
> > + const struct v4l2_format *dev_fmt;
> > +
> > + dev_fmt = mtk_cam_dev_find_fmt(&node->desc, sizes->pixel_format);
> > + if (!dev_fmt || sizes->index)
> > + return -EINVAL;
> > +
> > + sizes->type = node->desc.frmsizes->type;
> > + memcpy(&sizes->stepwise, &node->desc.frmsizes->stepwise,
> > + sizeof(sizes->stepwise));
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_cam_vidioc_meta_enum_fmt(struct file *file, void *fh,
> > + struct v4l2_fmtdesc *f)
> > +{
> > + struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
> > +
> > + if (f->index)
> > + return -EINVAL;
> > +
> > + strscpy(f->description, node->desc.description,
> > + sizeof(node->desc.description));
> > + f->pixelformat = node->vdev_fmt.fmt.meta.dataformat;
> > + f->flags = 0;
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_cam_vidioc_g_meta_fmt(struct file *file, void *fh,
> > + struct v4l2_format *f)
> > +{
> > + struct mtk_cam_video_device *node = file_to_mtk_cam_node(file);
> > +
> > + f->fmt.meta.dataformat = node->vdev_fmt.fmt.meta.dataformat;
> > + f->fmt.meta.buffersize = node->vdev_fmt.fmt.meta.buffersize;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct v4l2_subdev_core_ops mtk_cam_subdev_core_ops = {
> > + .subscribe_event = mtk_cam_sd_subscribe_event,
> > + .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> > + .s_power = mtk_cam_sd_s_power,
> > +};
> > +
> > +static const struct v4l2_subdev_video_ops mtk_cam_subdev_video_ops = {
> > + .s_stream = mtk_cam_sd_s_stream,
> > +};
> > +
> > +static const struct v4l2_subdev_ops mtk_cam_subdev_ops = {
> > + .core = &mtk_cam_subdev_core_ops,
> > + .video = &mtk_cam_subdev_video_ops,
> > +};
> > +
> > +static const struct media_entity_operations mtk_cam_media_ops = {
>
> nit: mtk_cam_media_entity_ops?
>
Rename in next patch.
> > + .link_setup = mtk_cam_media_link_setup,
> > + .link_validate = v4l2_subdev_link_validate,
> > +};
> > +
> > +static const struct vb2_ops mtk_cam_vb2_ops = {
> > + .queue_setup = mtk_cam_vb2_queue_setup,
> > + .wait_prepare = vb2_ops_wait_prepare,
> > + .wait_finish = vb2_ops_wait_finish,
> > + .buf_init = mtk_cam_vb2_buf_init,
> > + .buf_prepare = mtk_cam_vb2_buf_prepare,
> > + .start_streaming = mtk_cam_vb2_start_streaming,
> > + .stop_streaming = mtk_cam_vb2_stop_streaming,
> > + .buf_queue = mtk_cam_vb2_buf_queue,
> > + .buf_request_complete = mtk_cam_vb2_buf_request_complete,
> > +};
> > +
> > +static const struct v4l2_file_operations mtk_cam_v4l2_fops = {
> > + .unlocked_ioctl = video_ioctl2,
> > + .open = mtk_cam_isp_open,
> > + .release = mtk_cam_isp_release,
> > + .poll = vb2_fop_poll,
> > + .mmap = vb2_fop_mmap,
> > +#ifdef CONFIG_COMPAT
> > + .compat_ioctl32 = v4l2_compat_ioctl32,
> > +#endif
> > +};
> > +
> > +static const struct media_device_ops mtk_cam_media_req_ops = {
>
> nit: Those are media ops, so perhaps just mtk_cam_media_ops?
>
Rename in next patch.
> > + .link_notify = v4l2_pipeline_link_notify,
> > + .req_alloc = mtk_cam_req_alloc,
> > + .req_free = mtk_cam_req_free,
> > + .req_validate = vb2_request_validate,
> > + .req_queue = mtk_cam_req_queue,
> > +};
> > +
> > +static int mtk_cam_media_register(struct device *dev,
> > + struct media_device *media_dev)
> > +{
> > + media_dev->dev = dev;
> > + strscpy(media_dev->model, MTK_CAM_DEV_P1_NAME,
>
> Could we replace any use of this macro with dev_driver_string(dev) and then
> delete the macro? The less name strings in the driver the better, as there
> is less change for confusing the userspace if few different names are used
> at the same time.
>
Ok, revised in next patch.
> > + sizeof(media_dev->model));
> > + snprintf(media_dev->bus_info, sizeof(media_dev->bus_info),
> > + "platform:%s", dev_name(dev));
> > + media_dev->hw_revision = 0;
> > + media_device_init(media_dev);
> > + media_dev->ops = &mtk_cam_media_req_ops;
> > +
> > + return media_device_register(media_dev);
> > +}
> > +
> > +static int mtk_cam_video_register_device(struct mtk_cam_dev *cam_dev, u32 i)
> > +{
> > + struct device *dev = &cam_dev->pdev->dev;
> > + struct mtk_cam_video_device *node = &cam_dev->vdev_nodes[i];
>
> Would it make sense to pass node as an argument to this function instead of
> (or in addition to) i?
>
Ok, revised in next patch.
> > + struct video_device *vdev = &node->vdev;
> > + struct vb2_queue *vbq = &node->vbq;
> > + u32 output = !cam_dev->vdev_nodes[i].desc.capture;
>
> Why not call it capture instead and avoid the inversion?
>
Ok, we will revised as below.
unsigned int output = V4L2_TYPE_IS_OUTPUT(node->desc.buf_type);
> > + u32 link_flags = cam_dev->vdev_nodes[i].desc.link_flags;
> > + int ret;
> > +
> > + cam_dev->subdev_pads[i].flags = output ?
> > + MEDIA_PAD_FL_SINK : MEDIA_PAD_FL_SOURCE;
> > +
> > + /* Initialize media entities */
> > + ret = media_entity_pads_init(&vdev->entity, 1, &node->vdev_pad);
> > + if (ret) {
> > + dev_err(dev, "failed initialize media pad:%d\n", ret);
> > + return ret;
> > + }
> > + node->enabled = false;
>
> Are all the nodes optional? If there is any required node, it should be
> always enabled and have the MEDIA_LNK_FL_IMMUTABLE flag set.
>
Ok, MTK_CAM_P1_MAIN_STREAM_OUT is required node, others are optional.
We will enable TK_CAM_P1_MAIN_STREAM_OUT with MEDIA_LNK_FL_IMMUTABLE |
MEDIA_LNK_FL_ENABLED.
> > + node->id = i;
> > + node->vdev_pad.flags = cam_dev->subdev_pads[i].flags;
>
> Hmm, shouldn't the subdev pads have opposite directions (sink vs source)?
>
Yes, it is wrong and fix with below statement.
node->vdev_pad.flags = output ? MEDIA_PAD_FL_SINK : MEDIA_PAD_FL_SOURCE;
> > + mtk_cam_dev_load_default_fmt(&cam_dev->pdev->dev,
> > + &node->desc,
> > + &node->vdev_fmt);
> > +
> > + /* Initialize vbq */
> > + vbq->type = node->vdev_fmt.type;
> > + if (vbq->type == V4L2_BUF_TYPE_META_OUTPUT)
> > + vbq->io_modes = VB2_MMAP;
> > + else
> > + vbq->io_modes = VB2_MMAP | VB2_DMABUF;
> > +
> > + if (node->desc.smem_alloc) {
> > + vbq->bidirectional = 1;
> > + vbq->dev = cam_dev->smem_dev;
> > + } else {
> > + vbq->dev = &cam_dev->pdev->dev;
> > + }
> > +
> > + if (vbq->type == V4L2_BUF_TYPE_META_CAPTURE)
> > + vdev->entity.function =
> > + MEDIA_ENT_F_PROC_VIDEO_STATISTICS;
>
> This is a video node, so it's just a DMA, not a processing entity. I believe
> all the entities corresponding to video nodes should use MEDIA_ENT_F_IO_V4L.
>
Ok, it is fixed.
> > + vbq->ops = &mtk_cam_vb2_ops;
> > + vbq->mem_ops = &vb2_dma_contig_memops;
> > + vbq->buf_struct_size = sizeof(struct mtk_cam_dev_buffer);
> > + vbq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > + vbq->min_buffers_needed = 0; /* Can streamon w/o buffers */
> > + /* Put the process hub sub device in the vb2 private data */
>
> What is "process hub" and what "sub device" is this about?
>
We will drop this comment.
> > + vbq->drv_priv = cam_dev;
> > + vbq->lock = &node->lock;
> > + vbq->supports_requests = true;
> > +
> > + ret = vb2_queue_init(vbq);
> > + if (ret) {
> > + dev_err(dev, "failed to init. vb2 queue:%d\n", ret);
> > + goto fail_vb2_queue;
> > + }
> > +
> > + /* Initialize vdev */
> > + snprintf(vdev->name, sizeof(vdev->name), "%s %s",
> > + MTK_CAM_DEV_P1_NAME, node->desc.name);
> > + /* set cap/type/ioctl_ops of the video device */
> > + vdev->device_caps = node->desc.cap | V4L2_CAP_STREAMING;
> > + vdev->ioctl_ops = node->desc.ioctl_ops;
> > + vdev->fops = &mtk_cam_v4l2_fops;
> > + vdev->release = video_device_release_empty;
> > + vdev->lock = &node->lock;
> > + vdev->v4l2_dev = &cam_dev->v4l2_dev;
> > + vdev->queue = &node->vbq;
> > + vdev->vfl_dir = output ? VFL_DIR_TX : VFL_DIR_RX;
> > + vdev->entity.ops = NULL;
> > + /* Enable private control for image video devices */
> > + if (node->desc.image) {
> > + mtk_cam_ctrl_init(cam_dev, &node->ctrl_handler);
> > + vdev->ctrl_handler = &node->ctrl_handler;
> > + }
> > + video_set_drvdata(vdev, cam_dev);
> > + dev_dbg(dev, "register vdev:%d:%s\n", i, vdev->name);
> > +
> > + ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
> > + if (ret) {
> > + dev_err(dev, "failed to register vde:%d\n", ret);
> > + goto fail_vdev;
> > + }
> > +
> > + /* Create link between video node and the subdev pad */
> > + if (output) {
> > + ret = media_create_pad_link(&vdev->entity, 0,
> > + &cam_dev->subdev.entity,
> > + i, link_flags);
> > + } else {
> > + ret = media_create_pad_link(&cam_dev->subdev.entity,
> > + i, &vdev->entity, 0,
> > + link_flags);
> > + }
> > + if (ret)
> > + goto fail_link;
> > +
> > + /* Initialize miscellaneous variables */
> > + mutex_init(&node->lock);
> > + spin_lock_init(&node->slock);
> > + INIT_LIST_HEAD(&node->pending_list);
>
> This should be all initialized before registering the video device.
> Otherwise userspace could open the device before these are initialized.
>
Fixed in next patch.
> > +
> > + return 0;
> > +
> > +fail_link:
> > + video_unregister_device(vdev);
> > +fail_vdev:
> > + vb2_queue_release(vbq);
> > +fail_vb2_queue:
> > + media_entity_cleanup(&vdev->entity);
> > +
> > + return ret;
> > +}
> > +
> > +static int mtk_cam_mem2mem2_v4l2_register(struct mtk_cam_dev *cam_dev)
>
> This function doesn't have anything to do with mem2mem. How about
> mtk_cam_v4l2_register()?
>
> Perhaps it would make sense to move any media related code into into
> mtk_cam_media_register(), keep only V4L2 related code here and have the
> caller call the former first and then this one, rather than having such deep
> sequence of nested calls, which makes the driver harder to read.
>
Fixed in next patch.
> > +{
> > + struct device *dev = &cam_dev->pdev->dev;
>
> How about just storing dev, instead of pdev in the struct? Also, calling the
> argument "cam", would make it as short as cam->dev.
>
Ok, we will revise this in next patch.
> > + /* Total pad numbers is video devices + one seninf pad */
> > + unsigned int num_subdev_pads = MTK_CAM_CIO_PAD_SINK + 1;
> > + unsigned int i;
> > + int ret;
> > +
> > + ret = mtk_cam_media_register(dev,
> > + &cam_dev->media_dev);
> > + if (ret) {
> > + dev_err(dev, "failed to register media device:%d\n", ret);
> > + return ret;
> > + }
> > + dev_info(dev, "Register media device: %s, 0x%pK",
> > + MTK_CAM_DEV_P1_NAME, cam_dev->media_dev);
>
> An info message should be useful to the user in some way. Printing kernel
> pointers isn't useful. Something like "registered media0" could be useful to
> let the user know which media device is associated with this driver if there
> is more than one in the system.
>
Here is the new log info.
dev_info(dev, "media%d register",cam->media_dev.devnode->minor);
> > +
> > + /* Set up v4l2 device */
> > + cam_dev->v4l2_dev.mdev = &cam_dev->media_dev;
> > + ret = v4l2_device_register(dev, &cam_dev->v4l2_dev);
> > + if (ret) {
> > + dev_err(dev, "failed to register V4L2 device:%d\n", ret);
> > + goto fail_v4l2_dev;
>
> Please call the labels after the cleanup step that needs to be done. It
> makes it easier to spot any ordering errors.
>
Will fix in next patch.
> > + }
> > + dev_info(dev, "Register v4l2 device: 0x%pK", cam_dev->v4l2_dev);
>
> Same as above.
>
Ditto.
dev_info(dev, "Register v4l2 device: %s", cam->v4l2_dev.name);
> > +
> > + /* Initialize subdev media entity */
> > + cam_dev->subdev_pads = devm_kcalloc(dev, num_subdev_pads,
> > + sizeof(*cam_dev->subdev_pads),
> > + GFP_KERNEL);
> > + if (!cam_dev->subdev_pads) {
> > + ret = -ENOMEM;
> > + goto fail_subdev_pads;
> > + }
> > +
> > + ret = media_entity_pads_init(&cam_dev->subdev.entity,
> > + num_subdev_pads,
> > + cam_dev->subdev_pads);
> > + if (ret) {
> > + dev_err(dev, "failed initialize media pads:%d:\n", ret);
>
> Stray ":" at the end of the message.
>
Fixed in next patch.
> > + goto fail_subdev_pads;
> > + }
> > +
> > + /* Initialize all pads with MEDIA_PAD_FL_SOURCE */
> > + for (i = 0; i < num_subdev_pads; i++)
> > + cam_dev->subdev_pads[i].flags = MEDIA_PAD_FL_SOURCE;
> > +
> > + /* Customize the last one pad as CIO sink pad. */
> > + cam_dev->subdev_pads[MTK_CAM_CIO_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> > +
> > + /* Initialize subdev */
> > + v4l2_subdev_init(&cam_dev->subdev, &mtk_cam_subdev_ops);
> > + cam_dev->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_STATISTICS;
> > + cam_dev->subdev.entity.ops = &mtk_cam_media_ops;
> > + cam_dev->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE |
> > + V4L2_SUBDEV_FL_HAS_EVENTS;
> > + snprintf(cam_dev->subdev.name, sizeof(cam_dev->subdev.name),
> > + "%s", MTK_CAM_DEV_P1_NAME);
> > + v4l2_set_subdevdata(&cam_dev->subdev, cam_dev);
> > +
> > + ret = v4l2_device_register_subdev(&cam_dev->v4l2_dev, &cam_dev->subdev);
> > + if (ret) {
> > + dev_err(dev, "failed initialize subdev:%d\n", ret);
> > + goto fail_subdev;
> > + }
> > + dev_info(dev, "register subdev: %s\n", cam_dev->subdev.name);
> > +
> > + /* Create video nodes and links */
> > + for (i = 0; i < MTK_CAM_P1_TOTAL_NODES; i++) {
> > + ret = mtk_cam_video_register_device(cam_dev, i);
> > + if (ret)
> > + goto fail_video_register;
> > + }
> > +
> > + vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
> > +
> > + return 0;
> > +
> > +fail_video_register:
> > + i--;
>
> This could be moved into the for clause, as the initialization statement.
>
Fixed in next patch.
> > + for (; i >= 0; i--) {
>
> i is unsigned. Did this compile without warnings?
>
> > + video_unregister_device(&cam_dev->vdev_nodes[i].vdev);
> > + media_entity_cleanup(&cam_dev->vdev_nodes[i].vdev.entity);
> > + mutex_destroy(&cam_dev->vdev_nodes[i].lock);
>
> Should we move this into mtk_cam_video_unregister_device() to be consistent
> with registration?
>
Fixed in next patch.
> > + }
> > +fail_subdev:
> > + media_entity_cleanup(&cam_dev->subdev.entity);
> > +fail_subdev_pads:
> > + v4l2_device_unregister(&cam_dev->v4l2_dev);
> > +fail_v4l2_dev:
> > + dev_err(dev, "fail_v4l2_dev mdev: 0x%pK:%d", &cam_dev->media_dev, ret);
>
> Please print errors only where they actually happen, not at the cleanup.
>
Fixed in next patch.
> > + media_device_unregister(&cam_dev->media_dev);
> > + media_device_cleanup(&cam_dev->media_dev);
> > +
> > + return ret;
> > +}
> > +
> > +static int mtk_cam_v4l2_unregister(struct mtk_cam_dev *cam_dev)
> > +{
> > + unsigned int i;
> > + struct mtk_cam_video_device *dev;
>
> nit: Move the declaration inside the for loop, since the variable is only
> used there.
>
Fixed in next patch.
> > +
> > + for (i = 0; i < MTK_CAM_P1_TOTAL_NODES; i++) {
> > + dev = &cam_dev->vdev_nodes[i];
> > + video_unregister_device(&dev->vdev);
> > + media_entity_cleanup(&dev->vdev.entity);
> > + mutex_destroy(&dev->lock);
> > + if (dev->desc.image)
> > + v4l2_ctrl_handler_free(&dev->ctrl_handler);
> > + }
> > +
> > + vb2_dma_contig_clear_max_seg_size(&cam_dev->pdev->dev);
> > +
> > + v4l2_device_unregister_subdev(&cam_dev->subdev);
> > + media_entity_cleanup(&cam_dev->subdev.entity);
> > + kfree(cam_dev->subdev_pads);
>
> This was allocated using devm_kcalloc(), so no need to free it explicitly.
>
Fixed in next patch.
> > +
> > + v4l2_device_unregister(&cam_dev->v4l2_dev);
> > + media_device_unregister(&cam_dev->media_dev);
> > + media_device_cleanup(&cam_dev->media_dev);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_cam_dev_complete(struct v4l2_async_notifier *notifier)
> > +{
> > + struct mtk_cam_dev *cam_dev =
> > + container_of(notifier, struct mtk_cam_dev, notifier);
> > + struct device *dev = &cam_dev->pdev->dev;
> > + int ret;
> > +
> > + ret = media_create_pad_link(&cam_dev->seninf->entity,
> > + MTK_CAM_CIO_PAD_SRC,
> > + &cam_dev->subdev.entity,
> > + MTK_CAM_CIO_PAD_SINK,
> > + 0);
> > + if (ret) {
> > + dev_err(dev, "fail to create pad link %s %s err:%d\n",
> > + cam_dev->seninf->entity.name,
> > + cam_dev->subdev.entity.name,
> > + ret);
> > + return ret;
> > + }
> > +
> > + dev_info(dev, "Complete the v4l2 registration\n");
>
> dev_dbg()
>
Fixed in next patch.
> > +
> > + ret = v4l2_device_register_subdev_nodes(&cam_dev->v4l2_dev);
> > + if (ret) {
> > + dev_err(dev, "failed initialize subdev nodes:%d\n", ret);
> > + return ret;
> > + }
> > +
> > + return ret;
> > +}
>
> Why not just put the contents of this function inside
> mtk_cam_dev_notifier_complete()?
>
Ok, we will mtk_cam_dev_complete() function and move its content into
mtk_cam_dev_notifier_complete().
> > +
> > +static int mtk_cam_dev_notifier_bound(struct v4l2_async_notifier *notifier,
> > + struct v4l2_subdev *sd,
> > + struct v4l2_async_subdev *asd)
> > +{
> > + struct mtk_cam_dev *cam_dev =
> > + container_of(notifier, struct mtk_cam_dev, notifier);
> > +
>
> Should we somehow check that the entity we got is seninf indeed and there
> was no mistake in DT?
>
How about to check the entity function of seninf device?
if (!(sd->entity.function & MEDIA_ENT_F_VID_IF_BRIDGE)) {
dev_dbg(cam->dev, "No MEDIA_ENT_F_VID_IF_BRIDGE function\n");
return -ENODEV;
}
If we need to check DT, may we need to implement this in parse_endpoint
callback function of v4l2_async_notifier_parse_fwnode_endpoints?
> > + cam_dev->seninf = sd;
> > + dev_info(&cam_dev->pdev->dev, "%s is bounded\n", sd->entity.name);
>
> bound
>
> Also please make this dev_dbg().
>
Fixed in next patch.
> > + return 0;
> > +}
> > +
> > +static void mtk_cam_dev_notifier_unbind(struct v4l2_async_notifier *notifier,
> > + struct v4l2_subdev *sd,
> > + struct v4l2_async_subdev *asd)
> > +{
> > + struct mtk_cam_dev *cam_dev =
> > + container_of(notifier, struct mtk_cam_dev, notifier);
> > +
> > + cam_dev->seninf = NULL;
> > + dev_dbg(&cam_dev->pdev->dev, "%s is unbounded\n", sd->entity.name);
>
> unbound
>
Fixed in next patch.
> > +}
> > +
> > +static int mtk_cam_dev_notifier_complete(struct v4l2_async_notifier *notifier)
> > +{
> > + return mtk_cam_dev_complete(notifier);
> > +}
> > +
> > +static const struct v4l2_async_notifier_operations mtk_cam_async_ops = {
> > + .bound = mtk_cam_dev_notifier_bound,
> > + .unbind = mtk_cam_dev_notifier_unbind,
> > + .complete = mtk_cam_dev_notifier_complete,
> > +};
> > +
> > +static int mtk_cam_v4l2_async_register(struct mtk_cam_dev *cam_dev)
> > +{
> > + struct device *dev = &cam_dev->pdev->dev;
> > + int ret;
> > +
> > + ret = v4l2_async_notifier_parse_fwnode_endpoints(dev,
> > + &cam_dev->notifier, sizeof(struct v4l2_async_subdev),
> > + NULL);
> > + if (ret)
> > + return ret;
> > +
> > + if (!cam_dev->notifier.num_subdevs)
> > + return -ENODEV;
>
> Could we print some error messages for the 2 cases above?
>
Fixed in next patch.
> > +
> > + cam_dev->notifier.ops = &mtk_cam_async_ops;
> > + dev_info(&cam_dev->pdev->dev, "mtk_cam v4l2_async_notifier_register\n");
>
> dev_dbg()
>
Fixed in next patch.
> > + ret = v4l2_async_notifier_register(&cam_dev->v4l2_dev,
> > + &cam_dev->notifier);
> > + if (ret) {
> > + dev_err(&cam_dev->pdev->dev,
> > + "failed to register async notifier : %d\n", ret);
> > + v4l2_async_notifier_cleanup(&cam_dev->notifier);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static void mtk_cam_v4l2_async_unregister(struct mtk_cam_dev *cam_dev)
> > +{
> > + v4l2_async_notifier_unregister(&cam_dev->notifier);
> > + v4l2_async_notifier_cleanup(&cam_dev->notifier);
> > +}
> > +
> > +static const struct v4l2_ioctl_ops mtk_cam_v4l2_vcap_ioctl_ops = {
> > + .vidioc_querycap = mtk_cam_vidioc_querycap,
> > + .vidioc_enum_framesizes = mtk_cam_vidioc_enum_framesizes,
> > + .vidioc_enum_fmt_vid_cap_mplane = mtk_cam_vidioc_enum_fmt,
> > + .vidioc_g_fmt_vid_cap_mplane = mtk_cam_vidioc_g_fmt,
> > + .vidioc_s_fmt_vid_cap_mplane = mtk_cam_vidioc_s_fmt,
> > + .vidioc_try_fmt_vid_cap_mplane = mtk_cam_vidioc_try_fmt,
> > + .vidioc_enum_input = mtk_cam_vidioc_enum_input,
> > + .vidioc_g_input = mtk_cam_vidioc_g_input,
> > + .vidioc_s_input = mtk_cam_vidioc_s_input,
>
> I don't think we need vidioc_*_input. At least the current implementation in
> this patch doesn't seem to do anything useful.
>
> > + /* buffer queue management */
>
> Drop this comment, as it's obvious that the callbacks with "buf" in the name
> are related to buffers, there are some non-buffer callbacks below too (e.g.
> vidioc_subscribe_event) and also the other ops structs below don't have such
> comment.
>
Fixed in next patch.
> > + .vidioc_reqbufs = vb2_ioctl_reqbufs,
> > + .vidioc_create_bufs = vb2_ioctl_create_bufs,
> > + .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> > + .vidioc_querybuf = vb2_ioctl_querybuf,
> > + .vidioc_qbuf = vb2_ioctl_qbuf,
> > + .vidioc_dqbuf = vb2_ioctl_dqbuf,
> > + .vidioc_streamon = vb2_ioctl_streamon,
> > + .vidioc_streamoff = vb2_ioctl_streamoff,
> > + .vidioc_expbuf = vb2_ioctl_expbuf,
> > + .vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
> > + .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> > +};
> > +
> > +static const struct v4l2_ioctl_ops mtk_cam_v4l2_meta_cap_ioctl_ops = {
> > + .vidioc_querycap = mtk_cam_vidioc_querycap,
> > + .vidioc_enum_fmt_meta_cap = mtk_cam_vidioc_meta_enum_fmt,
> > + .vidioc_g_fmt_meta_cap = mtk_cam_vidioc_g_meta_fmt,
> > + .vidioc_s_fmt_meta_cap = mtk_cam_vidioc_g_meta_fmt,
> > + .vidioc_try_fmt_meta_cap = mtk_cam_vidioc_g_meta_fmt,
> > + .vidioc_reqbufs = vb2_ioctl_reqbufs,
> > + .vidioc_create_bufs = vb2_ioctl_create_bufs,
> > + .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> > + .vidioc_querybuf = vb2_ioctl_querybuf,
> > + .vidioc_qbuf = vb2_ioctl_qbuf,
> > + .vidioc_dqbuf = vb2_ioctl_dqbuf,
> > + .vidioc_streamon = vb2_ioctl_streamon,
> > + .vidioc_streamoff = vb2_ioctl_streamoff,
> > + .vidioc_expbuf = vb2_ioctl_expbuf,
> > +};
> > +
> > +static const struct v4l2_ioctl_ops mtk_cam_v4l2_meta_out_ioctl_ops = {
> > + .vidioc_querycap = mtk_cam_vidioc_querycap,
> > + .vidioc_enum_fmt_meta_out = mtk_cam_vidioc_meta_enum_fmt,
> > + .vidioc_g_fmt_meta_out = mtk_cam_vidioc_g_meta_fmt,
> > + .vidioc_s_fmt_meta_out = mtk_cam_vidioc_g_meta_fmt,
> > + .vidioc_try_fmt_meta_out = mtk_cam_vidioc_g_meta_fmt,
> > + .vidioc_reqbufs = vb2_ioctl_reqbufs,
> > + .vidioc_create_bufs = vb2_ioctl_create_bufs,
> > + .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> > + .vidioc_querybuf = vb2_ioctl_querybuf,
> > + .vidioc_qbuf = vb2_ioctl_qbuf,
> > + .vidioc_dqbuf = vb2_ioctl_dqbuf,
> > + .vidioc_streamon = vb2_ioctl_streamon,
> > + .vidioc_streamoff = vb2_ioctl_streamoff,
> > + .vidioc_expbuf = vb2_ioctl_expbuf,
> > +};
> > +
> > +static const struct v4l2_format meta_fmts[] = {
> > + {
> > + .fmt.meta = {
> > + .dataformat = V4L2_META_FMT_MTISP_PARAMS,
> > + .buffersize = 128 * PAGE_SIZE,
>
> PAGE_SIZE is a weird unit for specifying generic buffer sizes. How about
> making it 512 * SZ_1K?
>
> That said, it should normally be just sizeof(struct some_struct_used_here).
>
> Same for the other entries below.
>
ok, we will change the size unit from PAGE_SIZE to SZ_1K.
If we finalize the meta structures design, we may change to use sizeof.
> > + },
> > + },
> > + {
> > + .fmt.meta = {
> > + .dataformat = V4L2_META_FMT_MTISP_3A,
> > + .buffersize = 300 * PAGE_SIZE,
> > + },
> > + },
> > + {
> > + .fmt.meta = {
> > + .dataformat = V4L2_META_FMT_MTISP_AF,
> > + .buffersize = 160 * PAGE_SIZE,
> > + },
> > + },
> > + {
> > + .fmt.meta = {
> > + .dataformat = V4L2_META_FMT_MTISP_LCS,
> > + .buffersize = 72 * PAGE_SIZE,
> > + },
> > + },
> > + {
> > + .fmt.meta = {
> > + .dataformat = V4L2_META_FMT_MTISP_LMV,
> > + .buffersize = 256,
> > + },
> > + },
> > +};
> > +
> > +static const struct v4l2_format stream_out_fmts[] = {
> > + {
> > + .fmt.pix_mp = {
> > + .width = IMG_MAX_WIDTH,
> > + .height = IMG_MAX_HEIGHT,
> > + .pixelformat = V4L2_PIX_FMT_MTISP_B8,
> > + .field = V4L2_FIELD_NONE,
> > + .colorspace = V4L2_COLORSPACE_SRGB,
> > + .num_planes = 1,
> > + },
> > + },
> > + {
> > + .fmt.pix_mp = {
> > + .width = IMG_MAX_WIDTH,
> > + .height = IMG_MAX_HEIGHT,
> > + .pixelformat = V4L2_PIX_FMT_MTISP_B10,
> > + .field = V4L2_FIELD_NONE,
> > + .colorspace = V4L2_COLORSPACE_SRGB,
> > + .num_planes = 1,
> > + },
> > + },
> > + {
> > + .fmt.pix_mp = {
> > + .width = IMG_MAX_WIDTH,
> > + .height = IMG_MAX_HEIGHT,
> > + .pixelformat = V4L2_PIX_FMT_MTISP_B12,
> > + .field = V4L2_FIELD_NONE,
> > + .colorspace = V4L2_COLORSPACE_SRGB,
> > + .num_planes = 1,
> > + },
> > + },
> > + {
> > + .fmt.pix_mp = {
> > + .width = IMG_MAX_WIDTH,
> > + .height = IMG_MAX_HEIGHT,
> > + .pixelformat = V4L2_PIX_FMT_MTISP_B14,
> > + .field = V4L2_FIELD_NONE,
> > + .colorspace = V4L2_COLORSPACE_SRGB,
> > + .num_planes = 1,
> > + },
> > + },
> > +};
> > +
> > +static const struct v4l2_format bin_out_fmts[] = {
> > + {
> > + .fmt.pix_mp = {
> > + .width = RRZ_MAX_WIDTH,
> > + .height = RRZ_MAX_HEIGHT,
> > + .pixelformat = V4L2_PIX_FMT_MTISP_F8,
> > + .field = V4L2_FIELD_NONE,
> > + .colorspace = V4L2_COLORSPACE_RAW,
> > + .num_planes = 1,
> > + },
> > + },
> > + {
> > + .fmt.pix_mp = {
> > + .width = RRZ_MAX_WIDTH,
> > + .height = RRZ_MAX_HEIGHT,
> > + .pixelformat = V4L2_PIX_FMT_MTISP_F10,
> > + .field = V4L2_FIELD_NONE,
> > + .colorspace = V4L2_COLORSPACE_RAW,
> > + .num_planes = 1,
> > + },
> > + },
> > + {
> > + .fmt.pix_mp = {
> > + .width = RRZ_MAX_WIDTH,
> > + .height = RRZ_MAX_HEIGHT,
> > + .pixelformat = V4L2_PIX_FMT_MTISP_F12,
> > + .field = V4L2_FIELD_NONE,
> > + .colorspace = V4L2_COLORSPACE_RAW,
> > + .num_planes = 1,
> > + },
> > + },
> > + {
> > + .fmt.pix_mp = {
> > + .width = RRZ_MAX_WIDTH,
> > + .height = RRZ_MAX_HEIGHT,
> > + .pixelformat = V4L2_PIX_FMT_MTISP_F14,
> > + .field = V4L2_FIELD_NONE,
> > + .colorspace = V4L2_COLORSPACE_RAW,
> > + .num_planes = 1,
> > + },
> > + },
> > +};
> > +
> > +static const struct v4l2_frmsizeenum img_frm_size_nums[] = {
> > + {
> > + .index = 0,
> > + .type = V4L2_FRMSIZE_TYPE_CONTINUOUS,
> > + .stepwise = {
> > + .max_width = IMG_MAX_WIDTH,
> > + .min_width = IMG_MIN_WIDTH,
> > + .max_height = IMG_MAX_HEIGHT,
> > + .min_height = IMG_MIN_HEIGHT,
> > + .step_height = 1,
> > + .step_width = 1,
> > + },
> > + },
> > + {
> > + .index = 0,
> > + .type = V4L2_FRMSIZE_TYPE_CONTINUOUS,
> > + .stepwise = {
> > + .max_width = RRZ_MAX_WIDTH,
> > + .min_width = RRZ_MIN_WIDTH,
> > + .max_height = RRZ_MAX_HEIGHT,
> > + .min_height = RRZ_MIN_HEIGHT,
> > + .step_height = 1,
> > + .step_width = 1,
> > + },
> > + },
> > +};
> > +
> > +static const struct
> > +mtk_cam_dev_node_desc output_queues[MTK_CAM_P1_TOTAL_OUTPUT] = {
> > + {
> > + .id = MTK_CAM_P1_META_IN_0,
> > + .name = "meta input",
> > + .description = "ISP tuning parameters",
> > + .cap = V4L2_CAP_META_OUTPUT,
> > + .buf_type = V4L2_BUF_TYPE_META_OUTPUT,
> > + .link_flags = 0,
> > + .capture = false,
> > + .image = false,
> > + .smem_alloc = true,
> > + .fmts = meta_fmts,
> > + .num_fmts = ARRAY_SIZE(meta_fmts),
> > + .default_fmt_idx = 0,
> > + .max_buf_count = 10,
> > + .ioctl_ops = &mtk_cam_v4l2_meta_out_ioctl_ops,
> > + },
> > +};
> > +
> > +static const struct
> > +mtk_cam_dev_node_desc capture_queues[MTK_CAM_P1_TOTAL_CAPTURE] = {
> > + {
> > + .id = MTK_CAM_P1_MAIN_STREAM_OUT,
> > + .name = "main stream",
> > + .cap = V4L2_CAP_VIDEO_CAPTURE_MPLANE,
> > + .buf_type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
> > + .link_flags = 0,
> > + .capture = true,
> > + .image = true,
> > + .smem_alloc = false,
> > + .dma_port = R_IMGO,
> > + .fmts = stream_out_fmts,
> > + .num_fmts = ARRAY_SIZE(stream_out_fmts),
> > + .default_fmt_idx = 1,
>
> Why not just make it always 0 and move the default format to the beginning
> of stream_out_fmts[]?
>
Fixed in next patch.
> > + .ioctl_ops = &mtk_cam_v4l2_vcap_ioctl_ops,
> > + .frmsizes = &img_frm_size_nums[0],
>
> nit: If you only need to point to these data once, you could define them in
> place, as a compound literal, like
>
Fixed in next patch.
> > .frmsizes = &(struct v4l2_framesizeenum) {
> > // initialize here
> > },
>
Ditto
> > + },
> > + {
> > + .id = MTK_CAM_P1_PACKED_BIN_OUT,
> > + .name = "packed out",
> > + .cap = V4L2_CAP_VIDEO_CAPTURE_MPLANE,
> > + .buf_type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
> > + .link_flags = 0,
> > + .capture = true,
> > + .image = true,
> > + .smem_alloc = false,
> > + .dma_port = R_RRZO,
> > + .fmts = bin_out_fmts,
> > + .num_fmts = ARRAY_SIZE(bin_out_fmts),
> > + .default_fmt_idx = 1,
> > + .ioctl_ops = &mtk_cam_v4l2_vcap_ioctl_ops,
> > + .frmsizes = &img_frm_size_nums[1],
> > + },
> > + {
> > + .id = MTK_CAM_P1_META_OUT_0,
> > + .name = "partial meta 0",
> > + .description = "AE/AWB histogram",
> > + .cap = V4L2_CAP_META_CAPTURE,
> > + .buf_type = V4L2_BUF_TYPE_META_CAPTURE,
> > + .link_flags = 0,
> > + .capture = true,
> > + .image = false,
> > + .smem_alloc = false,
> > + .dma_port = R_AAO | R_FLKO | R_PSO,
> > + .fmts = meta_fmts,
> > + .num_fmts = ARRAY_SIZE(meta_fmts),
> > + .default_fmt_idx = 1,
> > + .max_buf_count = 5,
> > + .ioctl_ops = &mtk_cam_v4l2_meta_cap_ioctl_ops,
> > + },
> > + {
> > + .id = MTK_CAM_P1_META_OUT_1,
> > + .name = "partial meta 1",
> > + .description = "AF histogram",
> > + .cap = V4L2_CAP_META_CAPTURE,
> > + .buf_type = V4L2_BUF_TYPE_META_CAPTURE,
> > + .link_flags = 0,
> > + .capture = true,
> > + .image = false,
> > + .smem_alloc = false,
> > + .dma_port = R_AFO,
> > + .fmts = meta_fmts,
> > + .num_fmts = ARRAY_SIZE(meta_fmts),
> > + .default_fmt_idx = 2,
> > + .max_buf_count = 5,
> > + .ioctl_ops = &mtk_cam_v4l2_meta_cap_ioctl_ops,
> > + },
> > + {
> > + .id = MTK_CAM_P1_META_OUT_2,
> > + .name = "partial meta 2",
> > + .description = "Local contrast enhanced statistics",
> > + .cap = V4L2_CAP_META_CAPTURE,
> > + .buf_type = V4L2_BUF_TYPE_META_CAPTURE,
> > + .link_flags = 0,
> > + .capture = true,
> > + .image = false,
> > + .smem_alloc = false,
> > + .dma_port = R_LCSO,
> > + .fmts = meta_fmts,
> > + .num_fmts = ARRAY_SIZE(meta_fmts),
> > + .default_fmt_idx = 3,
> > + .max_buf_count = 10,
> > + .ioctl_ops = &mtk_cam_v4l2_meta_cap_ioctl_ops,
> > + },
> > + {
> > + .id = MTK_CAM_P1_META_OUT_3,
> > + .name = "partial meta 3",
> > + .description = "Local motion vector histogram",
> > + .cap = V4L2_CAP_META_CAPTURE,
> > + .buf_type = V4L2_BUF_TYPE_META_CAPTURE,
> > + .link_flags = 0,
>
> link_flags seems to be 0 for all nodes.
>
Ditto.
> > + .capture = true,
>
> We already know this from .buf_type. We can check using the
> V4L2_TYPE_IS_OUTPUT() macro.
>
Ditto.
> > + .image = false,
> > + .smem_alloc = false,
> > + .dma_port = R_LMVO,
> > + .fmts = meta_fmts,
> > + .num_fmts = ARRAY_SIZE(meta_fmts),
>
> I don't think this is correct. The description suggests one specific format
> (local motion vector histogram), so the queue should probably be hardwired
> to that format?
>
Yes, we will set num_fmts = 1 for meta nodes.
> > + .default_fmt_idx = 4,
> > + .max_buf_count = 10,
>
> Where does this number come from?
>
The default maximum VB2 buffer count is 32.
In order to limit memory usage, we like to limit the maximum buffer
counts in the driver layer. The maximum buffer count is estimated
according to our camera MW.
#define VB2_MAX_FRAME (32)
> > + .ioctl_ops = &mtk_cam_v4l2_meta_cap_ioctl_ops,
> > + },
> > +};
> > +
> > +/* The helper to configure the device context */
> > +static void mtk_cam_dev_queue_setup(struct mtk_cam_dev *cam_dev)
> > +{
> > + unsigned int i, node_idx;
> > +
> > + node_idx = 0;
> > +
> > + /* Setup the output queue */
> > + for (i = 0; i < MTK_CAM_P1_TOTAL_OUTPUT; i++)
>
> < ARRAY_SIZE(output_queues)
>
Fixed in next patch.
> > + cam_dev->vdev_nodes[node_idx++].desc = output_queues[i];
> > +
> > + /* Setup the capture queue */
> > + for (i = 0; i < MTK_CAM_P1_TOTAL_CAPTURE; i++)
>
> < ARRAY_SIZE(capture_queues)
>
Fixed in next patch.
> > + cam_dev->vdev_nodes[node_idx++].desc = capture_queues[i];
> > +}
> > +
> > +int mtk_cam_dev_init(struct platform_device *pdev,
> > + struct mtk_cam_dev *cam_dev)
> > +{
> > + int ret;
> > +
> > + cam_dev->pdev = pdev;
>
> Do we need this additional mtk_cam_dev struct? Couldn't we just use
> mtk_isp_p1 here?
>
We remove pdev field and add dev field in mtk_cam_dev struct.
> > + mtk_cam_dev_queue_setup(cam_dev);
> > + /* v4l2 sub-device registration */
> > +
> > + dev_dbg(&cam_dev->pdev->dev, "mem2mem2.name: %s\n",
> > + MTK_CAM_DEV_P1_NAME);
>
> This debugging message doesn't seem very useful. Please remove.
>
Fixed in next patch.
> > + ret = mtk_cam_mem2mem2_v4l2_register(cam_dev);
> > + if (ret)
> > + return ret;
> > +
> > + ret = mtk_cam_v4l2_async_register(cam_dev);
> > + if (ret) {
> > + mtk_cam_v4l2_unregister(cam_dev);
>
> Please use an error path on the bottom of the function instead. With goto
> labels named after the next clean-up step that needs to be performed.
>
Fixed in next patch.
> > + return ret;
> > + }
> > +
> > + spin_lock_init(&cam_dev->req_lock);
> > + INIT_LIST_HEAD(&cam_dev->req_list);
> > + mutex_init(&cam_dev->lock);
> > +
> > + return 0;
> > +}
> > +
> > +int mtk_cam_dev_release(struct platform_device *pdev,
>
> "release" is normally used for file_operations. How about "cleanup"?
>
Fixed in next patch.
> > + struct mtk_cam_dev *cam_dev)
> > +{
> > + mtk_cam_v4l2_async_unregister(cam_dev);
> > + mtk_cam_v4l2_unregister(cam_dev);
> > +
> > + mutex_destroy(&cam_dev->lock);
> > +
> > + return 0;
> > +}
>
> I'd suggest moving any generic API implementation code (platform_device,
> V4L2, VB2, Media Controller, etc.) to mtk_cam.c and then moving any low
> level hardware/firmware-related code from mtk_cam.c and mtk_cam-scp.c to
> mtk_cam_hw.c.
>
Fixed in next patch.
> > +
> diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-v4l2-util.h b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-v4l2-util.h
> new file mode 100644
> index 000000000000..825cdf20643a
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-v4l2-util.h
> @@ -0,0 +1,173 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2018 MediaTek Inc.
> > + */
> > +
> > +#ifndef __MTK_CAM_DEV_V4L2_H__
> > +#define __MTK_CAM_DEV_V4L2_H__
> > +
> > +#include <linux/device.h>
> > +#include <linux/types.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/videodev2.h>
> > +#include <media/v4l2-device.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-subdev.h>
> > +#include <media/videobuf2-core.h>
> > +#include <media/videobuf2-v4l2.h>
> > +
> > +#define MTK_CAM_DEV_P1_NAME "MTK-ISP-P1-V4L2"
>
> Maybe it's not a critical thing, but generally it's a good practice to just
> explicitly specific this name somewhere, e.g. struct
> platform_driver::driver::name and then just refer to dev_driver_name(). It
> makes it easier to make sure that the name stays the same everywhere.
>
Fixed in next patch.
> > +
> > +#define MTK_CAM_P1_META_IN_0 0
> > +#define MTK_CAM_P1_TOTAL_OUTPUT 1
>
> Since these are just some utility definitions, we can use enum instead of
> assigning the values by hand.
>
We will revise these definitions as below.
/* ID enum value for struct mtk_cam_dev_node_desc:id */
enum {
MTK_CAM_P1_META_IN_0 = 0,
MTK_CAM_P1_MAIN_STREAM_OUT,
MTK_CAM_P1_PACKED_BIN_OUT,
MTK_CAM_P1_META_OUT_0,
MTK_CAM_P1_META_OUT_1,
MTK_CAM_P1_META_OUT_2,
MTK_CAM_P1_META_OUT_3,
MTK_CAM_P1_TOTAL_NODES
};
> > +
> > +#define MTK_CAM_P1_MAIN_STREAM_OUT 1
> > +#define MTK_CAM_P1_PACKED_BIN_OUT 2
> > +#define MTK_CAM_P1_META_OUT_0 3
> > +#define MTK_CAM_P1_META_OUT_1 4
> > +#define MTK_CAM_P1_META_OUT_2 5
> > +#define MTK_CAM_P1_META_OUT_3 6
> > +#define MTK_CAM_P1_TOTAL_CAPTURE 6
>
> Ditto.
>
Ditto.
> > +
> > +#define MTK_CAM_P1_TOTAL_NODES 7
>
> Please just add the two totals together rather than manually specifying the
> value.
>
Ditto.
> > +
> > +struct mtk_cam_dev_request {
> > + struct media_request req;
> > + struct list_head list;
> > +};
> > +
> > +struct mtk_cam_dev_buffer {
> > + struct vb2_v4l2_buffer vbb;
> > + struct list_head list;
> > + /* Intenal part */
> > + dma_addr_t daddr;
> > + dma_addr_t scp_addr;
> > + unsigned int node_id;
> > +};
>
> Could you add kerneldoc comments for the 2 structs?
>
Fixed in next patch.
> > +
> > +/*
> > + * struct mtk_cam_dev_node_desc - node attributes
> > + *
> > + * @id: id of the context queue
> > + * @name: media entity name
> > + * @description: descritpion of node
> > + * @cap: mapped to V4L2 capabilities
> > + * @buf_type: mapped to V4L2 buffer type
> > + * @dma_port: the dma port associated to the buffer
> > + * @link_flags: default media link flags
> > + * @smem_alloc: using the cam_smem_drv as alloc ctx or not
> > + * @capture: true for capture queue (device to user)
> > + * false for output queue (from user to device)
> > + * @image: true for image node, false for meta node
> > + * @num_fmts: the number of supported formats
> > + * @default_fmt_idx: default format of this node
> > + * @max_buf_count: maximum V4L2 buffer count
> > + * @ioctl_ops: mapped to v4l2_ioctl_ops
> > + * @fmts: supported format
> > + * @frmsizes: supported frame size number
> > + *
> > + */
> > +struct mtk_cam_dev_node_desc {
> > + u8 id;
> > + char *name;
> > + char *description;
> > + u32 cap;
> > + u32 buf_type;
> > + u32 dma_port;
> > + u32 link_flags;
> > + u8 smem_alloc:1;
> > + u8 capture:1;
> > + u8 image:1;
> > + u8 num_fmts;
> > + u8 default_fmt_idx;
> > + u8 max_buf_count;
> > + const struct v4l2_ioctl_ops *ioctl_ops;
> > + const struct v4l2_format *fmts;
> > + const struct v4l2_frmsizeenum *frmsizes;
> > +};
> > +
> > +/*
> > + * struct mtk_cam_video_device - Mediatek video device structure.
> > + *
> > + * @id: Id for mtk_cam_dev_node_desc or mem2mem2_nodes array
> > + * @enabled: Indicate the device is enabled or not
> > + * @vdev_fmt: The V4L2 format of video device
> > + * @vdev_apd: The media pad graph object of video device
>
> vdev_pad?
>
> > + * @vbq: A videobuf queue of video device
> > + * @desc: The node attributes of video device
> > + * @ctrl_handler: The control handler of video device
> > + * @pending_list: List for pending buffers before enqueuing into driver
> > + * @lock: Serializes vb2 queue and video device operations.
> > + * @slock: Protect for pending_list.
> > + *
>
> Please fix the order of the documentation to match the order of the struct.
>
Fixed in next patch.
> > + */
> > +struct mtk_cam_video_device {
> > + unsigned int id;
> > + unsigned int enabled;
> > + struct v4l2_format vdev_fmt;
> > + struct mtk_cam_dev_node_desc desc;
> > + struct video_device vdev;
>
> Not documented above.
>
Fixed in next patch.
> > + struct media_pad vdev_pad;
> > + struct vb2_queue vbq;
> > + struct v4l2_ctrl_handler ctrl_handler;
> > + struct list_head pending_list;
> > + /* Used for vbq & vdev */
>
> It's already documented in the kerneldoc comment.
>
Fixed in next patch.
Btw, if we remove this, we will got complain from checkpatch.pl script.
> > + struct mutex lock;
> > + /* protect for pending_list */
>
> It's already documented in the kerneldoc comment.
>
Ditto.
> > + spinlock_t slock;
>
> How about calling it pending_list_lock?
>
We will rename to buf_list to track all en-queue buffers in this video
node.
struct mtk_cam_video_device {
unsigned int id;
unsigned int enabled;
struct v4l2_format vdev_fmt;
struct mtk_cam_dev_node_desc desc;
struct video_device vdev;
struct media_pad vdev_pad;
struct vb2_queue vbq;
struct v4l2_ctrl_handler ctrl_handler;
struct list_head buf_list;
struct mutex lock;
spinlock_t buf_list_lock;
};
> > +};
> > +
> > +/*
> > + * struct mtk_cam_dev - Mediatek camera device structure.
> > + *
> > + * @pdev: Pointer to platform device
> > + * @smem_pdev: Pointer to shared memory platform device
> > + * @pipeline: Media pipeline information
> > + * @media_dev: Media device
> > + * @subdev: The V4L2 sub-device
> > + * @v4l2_dev: The V4L2 device driver
> > + * @notifier: The v4l2_device notifier data
> > + * @subdev_pads: Pointer to the number of media pads of this sub-device
> > + * @ctrl_handler: The control handler
> > + * @vdev_nodes: The array list of mtk_cam_video_device nodes
> > + * @seninf: Pointer to the seninf sub-device
> > + * @sensor: Pointer to the active sensor V4L2 sub-device when streaming on
> > + * @lock: The mutex protecting video device open/release operations
> > + * @streaming: Indicate the overall streaming status is on or off
> > + * @streamed_node_count: The number of V4L2 video device nodes are streaming on
> > + * @req_list: Lins to keep media requests before streaming on
> > + * @req_lock: Protect the req_list data
> > + *
> > + * Below is the graph topology for Camera IO connection.
> > + * sensor 1 (main) --> sensor IF --> P1 sub-device
> > + * sensor 2 (sub) -->
>
> This probably isn't the best place for graph topology description. I think
> we actually want a separate documentation file for this, similar to
> Documentation/media/v4l-drivers/ipu3.rst.
>
Ok, we will drop our graph topology comment & discuss how to come out
another separate document.
> > + *
> > + */
> > +struct mtk_cam_dev {
> > + struct platform_device *pdev;
> > + struct device *smem_dev;
> > + struct media_pipeline pipeline;
> > + struct media_device media_dev;
> > + struct v4l2_subdev subdev;
> > + struct v4l2_device v4l2_dev;
> > + struct v4l2_async_notifier notifier;
> > + struct media_pad *subdev_pads;
> > + struct v4l2_ctrl_handler ctrl_handler;
> > + struct mtk_cam_video_device vdev_nodes[MTK_CAM_P1_TOTAL_NODES];
> > + struct v4l2_subdev *seninf;
> > + struct v4l2_subdev *sensor;
> > + /* protect video device open/release operations */
>
> It's already documented in the kerneldoc comment.
>
Fixed in next patch.
> > + struct mutex lock;
> > + unsigned int streaming:1;
> > + atomic_t streamed_node_count;
> > + struct list_head req_list;
> > + /* protect for req_list */
>
> It's already documented in the kerneldoc comment.
>
Fixed in next patch.
> > + spinlock_t req_lock;
>
> How about calling it req_list_lock?
>
Below is new mtk_cam_dev structure.
We will use job to handle request.
struct mtk_cam_dev {
struct device *dev;
struct device *smem_dev;
struct media_pipeline pipeline;
struct media_device media_dev;
struct v4l2_subdev subdev;
struct v4l2_device v4l2_dev;
struct v4l2_async_notifier notifier;
struct media_pad *subdev_pads;
struct v4l2_ctrl_handler ctrl_handler;
struct mtk_cam_video_device vdev_nodes[MTK_CAM_P1_TOTAL_NODES];
struct v4l2_subdev *seninf;
struct v4l2_subdev *sensor;
struct mutex lock;
unsigned int streaming:1;
unsigned int enabled_dmas;
unsigned int enabled_node_count;
atomic_t streamed_node_count;
struct list_head pending_job_list;
spinlock_t pending_job_lock;
struct list_head running_job_list;
spinlock_t running_job_lock;
atomic_t running_job_count;
};
> Best regards,
> Tomasz
>
Thanks again for your many inputs on this patch.
It is helpful for us.
Best regards,
Jungo
^ permalink raw reply
* Re: [PATCH v3 1/6] dt-bindings: opp: Introduce opp-peak-KBps and opp-avg-KBps bindings
From: Viresh Kumar @ 2019-07-18 4:35 UTC (permalink / raw)
To: Saravana Kannan
Cc: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, Vincent Guittot,
Sweeney, Sean, daidavid1, Rajendra Nayak, Sibi Sankar,
Bjorn Andersson, Evan Green, Android Kernel Team, Linux PM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML
In-Reply-To: <CAGETcx-kUM7MqNYowwNAL1Q0bnFzxPEO6yMg0YTkk16=OnPdmg@mail.gmail.com>
On 17-07-19, 13:29, Saravana Kannan wrote:
> On Wed, Jul 17, 2019 at 12:54 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 02-07-19, 18:10, Saravana Kannan wrote:
> > > Interconnects often quantify their performance points in terms of
> > > bandwidth. So, add opp-peak-KBps (required) and opp-avg-KBps (optional) to
> > > allow specifying Bandwidth OPP tables in DT.
> > >
> > > opp-peak-KBps is a required property that replace opp-hz for Bandwidth OPP
> > > tables.
> > >
> > > opp-avg-KBps is an optional property that can be used in Bandwidth OPP
> > > tables.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > > Documentation/devicetree/bindings/opp/opp.txt | 15 ++++++++++++---
> > > 1 file changed, 12 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
> > > index 76b6c79604a5..c869e87caa2a 100644
> > > --- a/Documentation/devicetree/bindings/opp/opp.txt
> > > +++ b/Documentation/devicetree/bindings/opp/opp.txt
> > > @@ -83,9 +83,14 @@ properties.
> > >
> > > Required properties:
> > > - opp-hz: Frequency in Hz, expressed as a 64-bit big-endian integer. This is a
> > > - required property for all device nodes but devices like power domains. The
> > > - power domain nodes must have another (implementation dependent) property which
> > > - uniquely identifies the OPP nodes.
> > > + required property for all device nodes but for devices like power domains or
> > > + bandwidth opp tables. The power domain nodes must have another (implementation
> > > + dependent) property which uniquely identifies the OPP nodes. The interconnect
> > > + opps are required to have the opp-peak-bw property.
> >
> > ??
>
> Sorry, what's the question? Was this an accidental email?
Too much smartness is too bad sometimes, sorry about that :)
I placed the ?? right below "opp-peak-bw", there is no property like
that. You failed to update it :)
--
viresh
^ permalink raw reply
* Re: [PATCH v2] arm64: dts: fsl: pico-pi: Add a device tree for the PICO-PI-IMX8M
From: Shawn Guo @ 2019-07-18 3:55 UTC (permalink / raw)
To: Andra Danciu
Cc: robh+dt, mark.rutland, leoyang.li, festevam, aisheng.dong,
l.stach, angus, vabhav.sharma, pankaj.bansal, bhaskar.upadhaya,
ping.bai, manivannan.sadhasivam, richard.hu, devicetree,
linux-kernel, matti.vaittinen, linux-imx, daniel.baluta
In-Reply-To: <20190625123407.15888-1-andradanciu1997@gmail.com>
On Tue, Jun 25, 2019 at 03:34:07PM +0300, Andra Danciu wrote:
> From: Richard Hu <richard.hu@technexion.com>
>
> The current level of support yields a working console and is able to boot
> userspace from an initial ramdisk copied via u-boot in RAM.
>
> Additional subsystems that are active :
> - Ethernet
> - USB
>
> Cc: Daniel Baluta <daniel.baluta@nxp.com>
> Signed-off-by: Richard Hu <richard.hu@technexion.com>
> Signed-off-by: Andra Danciu <andradanciu1997@gmail.com>
> ---
> Changes since v1:
> - renamed wandboard-pi-8m.dts to pico-pi-8m.dts
> - removed pinctrl_csi1, pinctrl_wifi_ctrl
> - used generic name for pmic
> - removed gpo node
> - delete regulator-virtuals node
> - remove always-on property from buck1-8 and ldo3-7
> - remove pmic-buck-uses-i2c-dvs property for buck1-4
>
> arch/arm64/boot/dts/freescale/Makefile | 1 +
> arch/arm64/boot/dts/freescale/pico-pi-8m.dts | 458 +++++++++++++++++++++++++++
> 2 files changed, 459 insertions(+)
> create mode 100644 arch/arm64/boot/dts/freescale/pico-pi-8m.dts
>
> diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
> index c043aca66572..538422903e8a 100644
> --- a/arch/arm64/boot/dts/freescale/Makefile
> +++ b/arch/arm64/boot/dts/freescale/Makefile
> @@ -26,3 +26,4 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mq-librem5-devkit.dtb
> dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-rmb3.dtb
> dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-zest.dtb
> dtb-$(CONFIG_ARCH_MXC) += imx8qxp-mek.dtb
> +dtb-$(CONFIG_ARCH_MXC) += pico-pi-8m.dtb
> diff --git a/arch/arm64/boot/dts/freescale/pico-pi-8m.dts b/arch/arm64/boot/dts/freescale/pico-pi-8m.dts
> new file mode 100644
> index 000000000000..23422c8fc43f
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/pico-pi-8m.dts
> @@ -0,0 +1,458 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2018 Wandboard, Org.
> + * Copyright 2017 NXP
> + *
> + * Author: Richard Hu <hakahu@gmail.com>
> + */
> +
> +/dts-v1/;
> +
> +#include "imx8mq.dtsi"
> +
> +/ {
> + model = "PICO-PI-8M";
> + compatible = "wand,imx8mq-pico-pi", "fsl,imx8mq";
The board compatible needs to be documented.
> +
> + chosen {
> + bootargs = "console=ttymxc0,115200 earlycon=ec_imx6q,0x30860000,115200";
The earlycon is only needed for debugging early hang issue. Do you
really want it by default? If no, we can save the 'bootargs' here,
since 'stdout-path' below will get you correct console setup.
> + stdout-path = &uart1;
> + };
> +
> + regulators {
> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <0>;
Drop this container node and put fixed regulator directly under root
node.
> +
> + reg_usb_otg_vbus: usb_otg_vbus {
Please use node name like 'regulator-xxx'.
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_otg_vbus>;
> + compatible = "regulator-fixed";
> + regulator-name = "usb_otg_vbus";
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> + gpio = <&gpio3 14 GPIO_ACTIVE_LOW>;
> + };
> + };
> +};
> +
> +&iomuxc {
> + pinctrl-names = "default";
The 'pinctrl-names' is pointless if there is no 'pinctrl' property.
> +
> + wand-pi-8m {
Drop this container node.
> + pinctrl_otg_vbus: otgvbusgrp {
> + fsl,pins = <
> + MX8MQ_IOMUXC_NAND_DQS_GPIO3_IO14 0x19 /* USB OTG VBUS Enable */
> + >;
> + };
> +
> + pinctrl_enet_3v3: enet3v3grp {
Sort these pinctrl nodes alphabetically.
> + fsl,pins = <
> + MX8MQ_IOMUXC_GPIO1_IO00_GPIO1_IO0 0x19
> + >;
> + };
> +
> + pinctrl_fec1: fec1grp {
> + fsl,pins = <
> + MX8MQ_IOMUXC_ENET_MDC_ENET1_MDC 0x3
> + MX8MQ_IOMUXC_ENET_MDIO_ENET1_MDIO 0x23
> + MX8MQ_IOMUXC_ENET_TD3_ENET1_RGMII_TD3 0x1f
> + MX8MQ_IOMUXC_ENET_TD2_ENET1_RGMII_TD2 0x1f
> + MX8MQ_IOMUXC_ENET_TD1_ENET1_RGMII_TD1 0x1f
> + MX8MQ_IOMUXC_ENET_TD0_ENET1_RGMII_TD0 0x1f
> + MX8MQ_IOMUXC_ENET_RD3_ENET1_RGMII_RD3 0x91
> + MX8MQ_IOMUXC_ENET_RD2_ENET1_RGMII_RD2 0x91
> + MX8MQ_IOMUXC_ENET_RD1_ENET1_RGMII_RD1 0x91
> + MX8MQ_IOMUXC_ENET_RD0_ENET1_RGMII_RD0 0x91
> + MX8MQ_IOMUXC_ENET_TXC_ENET1_RGMII_TXC 0x1f
> + MX8MQ_IOMUXC_ENET_RXC_ENET1_RGMII_RXC 0x91
> + MX8MQ_IOMUXC_ENET_RX_CTL_ENET1_RGMII_RX_CTL 0x91
> + MX8MQ_IOMUXC_ENET_TX_CTL_ENET1_RGMII_TX_CTL 0x1f
> + MX8MQ_IOMUXC_GPIO1_IO09_GPIO1_IO9 0x19
> + >;
> + };
> +
> + pinctrl_i2c1: i2c1grp {
> + fsl,pins = <
> + MX8MQ_IOMUXC_I2C1_SCL_I2C1_SCL 0x4000007f
> + MX8MQ_IOMUXC_I2C1_SDA_I2C1_SDA 0x4000007f
> + >;
> + };
> +
> + pinctrl_i2c2: i2c2grp {
> + fsl,pins = <
> + MX8MQ_IOMUXC_I2C2_SCL_I2C2_SCL 0x4000007f
> + MX8MQ_IOMUXC_I2C2_SDA_I2C2_SDA 0x4000007f
> + >;
> + };
> +
> + pinctrl_uart1: uart1grp {
> + fsl,pins = <
> + MX8MQ_IOMUXC_UART1_RXD_UART1_DCE_RX 0x49
> + MX8MQ_IOMUXC_UART1_TXD_UART1_DCE_TX 0x49
> + >;
> + };
> +
> + pinctrl_uart2: uart2grp {
> + fsl,pins = <
> + MX8MQ_IOMUXC_UART2_RXD_UART2_DCE_RX 0x49
> + MX8MQ_IOMUXC_UART2_TXD_UART2_DCE_TX 0x49
> + MX8MQ_IOMUXC_UART4_RXD_UART2_DCE_CTS_B 0x49
> + MX8MQ_IOMUXC_UART4_TXD_UART2_DCE_RTS_B 0x49
> + >;
> + };
> +
> + pinctrl_usdhc1: usdhc1grp {
> + fsl,pins = <
> + MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x83
> + MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xc3
> + MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0 0xc3
> + MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1 0xc3
> + MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2 0xc3
> + MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3 0xc3
> + MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4 0xc3
> + MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5 0xc3
> + MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6 0xc3
> + MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7 0xc3
> + MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE 0x83
> + >;
> + };
> +
> + pinctrl_usdhc1_100mhz: usdhc1grp100mhz {
> + fsl,pins = <
> + MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x85
> + MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xc5
> + MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0 0xc5
> + MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1 0xc5
> + MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2 0xc5
> + MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3 0xc5
> + MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4 0xc5
> + MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5 0xc5
> + MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6 0xc5
> + MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7 0xc5
> + MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE 0x85
> + >;
> + };
> +
> + pinctrl_usdhc1_200mhz: usdhc1grp200mhz {
> + fsl,pins = <
> + MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x87
> + MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xc7
> + MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0 0xc7
> + MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1 0xc7
> + MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2 0xc7
> + MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3 0xc7
> + MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4 0xc7
> + MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5 0xc7
> + MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6 0xc7
> + MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7 0xc7
> + MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE 0x87
> + >;
> + };
> +
> + pinctrl_usdhc2_gpio: usdhc2grpgpio {
> + fsl,pins = <
> + MX8MQ_IOMUXC_SD2_CD_B_GPIO2_IO12 0x41
> + >;
> + };
> +
> + pinctrl_usdhc2: usdhc2grp {
> + fsl,pins = <
> + MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x83
> + MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xc3
> + MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0 0xc3
> + MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1 0xc3
> + MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2 0xc3
> + MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3 0xc3
> + MX8MQ_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0xc1
> + >;
> + };
> +
> + pinctrl_usdhc2_100mhz: usdhc2grp100mhz {
> + fsl,pins = <
> + MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x85
> + MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xc5
> + MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0 0xc5
> + MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1 0xc5
> + MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2 0xc5
> + MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3 0xc5
> + MX8MQ_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0xc1
> + >;
> + };
> +
> + pinctrl_usdhc2_200mhz: usdhc2grp200mhz {
> + fsl,pins = <
> + MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x87
> + MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xc7
> + MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0 0xc7
> + MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1 0xc7
> + MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2 0xc7
> + MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3 0xc7
> + MX8MQ_IOMUXC_GPIO1_IO04_USDHC2_VSELECT 0xc1
> + >;
> + };
> +
> + pinctrl_wdog: wdoggrp {
> + fsl,pins = <
> + MX8MQ_IOMUXC_GPIO1_IO02_WDOG1_WDOG_B 0xc6
> + >;
> + };
> +
> + pinctrl_pmic: pmicirq {
> + fsl,pins = <
> + MX8MQ_IOMUXC_GPIO1_IO03_GPIO1_IO3 0x41
> + >;
> + };
> +
> + pinctrl_tusb320_irq: tusb320_irqgrp {
Please name the pinctrl node in a more consistent way, i.e. no
underscore.
> + fsl,pins = <
> + MX8MQ_IOMUXC_NAND_DATA00_GPIO3_IO6 0x41
> + >;
> + };
> +
> + pinctrl_typec_ss_sel: typec_ss_selgrp {
> + fsl,pins = <
> + MX8MQ_IOMUXC_NAND_CLE_GPIO3_IO5 0x19
> + >;
> + };
> + };
> +};
> +
> +&fec1 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_fec1 &pinctrl_enet_3v3>;
> + phy-mode = "rgmii-id";
> + pinctrl-assert-gpios = <&gpio1 0 GPIO_ACTIVE_HIGH>;
> + phy-handle = <ðphy0>;
> + fsl,magic-packet;
> + status = "okay";
> +
> + mdio {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ethphy0: ethernet-phy@1 {
> + compatible = "ethernet-phy-ieee802.3-c22";
> + reg = <1>;
> + at803x,led-act-blind-workaround;
> + at803x,eee-disabled;
> + };
> + };
> +};
> +
> +&i2c1 {
> + clock-frequency = <100000>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_i2c1>;
> + status = "okay";
> +
> + typec_tusb320:tusb320@47 {
> + compatible = "ti,tusb320";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_tusb320_irq &pinctrl_typec_ss_sel>;
> + reg = <0x47>;
> + vbus-supply = <®_usb_otg_vbus>;
> + ss-sel-gpios = <&gpio3 5 GPIO_ACTIVE_HIGH>;
> + tusb320,int-gpio = <&gpio3 6 GPIO_ACTIVE_LOW>;
> + tusb320,select-mode = <0>;
> + tusb320,dfp-power = <0>;
> + };
Where is the bindings doc for this device?
> +
> + pmic: pmic@4b {
> + reg = <0x4b>;
> + compatible = "rohm,bd71837";
> + /* PMIC BD71837 PMIC_nINT GPIO1_IO12 */
> + pinctrl-0 = <&pinctrl_pmic>;
> + gpio_intr = <&gpio1 3 GPIO_ACTIVE_LOW>;
Where is the bindings for this property?
> +
> + regulators {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + buck1: BUCK1 {
> + regulator-name = "buck1";
> + regulator-min-microvolt = <700000>;
> + regulator-max-microvolt = <1300000>;
> + regulator-boot-on;
> + regulator-ramp-delay = <1250>;
> + rohm,dvs-run-voltage = <900000>;
> + rohm,dvs-idle-voltage = <850000>;
> + rohm,dvs-suspend-voltage = <800000>;
> + };
> +
> + buck2: BUCK2 {
> + regulator-name = "buck2";
> + regulator-min-microvolt = <700000>;
> + regulator-max-microvolt = <1300000>;
> + regulator-boot-on;
> + regulator-ramp-delay = <1250>;
> + rohm,dvs-run-voltage = <1000000>;
> + rohm,dvs-idle-voltage = <900000>;
> + };
> +
> + buck3: BUCK3 {
> + regulator-name = "buck3";
> + regulator-min-microvolt = <700000>;
> + regulator-max-microvolt = <1300000>;
> + regulator-boot-on;
> + rohm,dvs-run-voltage = <1000000>;
> + };
> +
> + buck4: BUCK4 {
> + regulator-name = "buck4";
> + regulator-min-microvolt = <700000>;
> + regulator-max-microvolt = <1300000>;
> + regulator-boot-on;
> + rohm,dvs-run-voltage = <1000000>;
> + };
> +
> + buck5: BUCK5 {
> + regulator-name = "buck5";
> + regulator-min-microvolt = <700000>;
> + regulator-max-microvolt = <1350000>;
> + regulator-boot-on;
> + };
> +
> + buck6: BUCK6 {
> + regulator-name = "buck6";
> + regulator-min-microvolt = <3000000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-boot-on;
> + };
> +
> + buck7: BUCK7 {
> + regulator-name = "buck7";
> + regulator-min-microvolt = <1605000>;
> + regulator-max-microvolt = <1995000>;
> + regulator-boot-on;
> + };
> +
> + buck8: BUCK8 {
> + regulator-name = "buck8";
> + regulator-min-microvolt = <800000>;
> + regulator-max-microvolt = <1400000>;
> + regulator-boot-on;
> + };
> +
> + ldo1: LDO1 {
> + regulator-name = "ldo1";
> + regulator-min-microvolt = <3000000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + ldo2: LDO2 {
> + regulator-name = "ldo2";
> + regulator-min-microvolt = <900000>;
> + regulator-max-microvolt = <900000>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + ldo3: LDO3 {
> + regulator-name = "ldo3";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-boot-on;
> + };
> +
> + ldo4: LDO4 {
> + regulator-name = "ldo4";
> + regulator-min-microvolt = <900000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-boot-on;
> + };
> +
> + ldo5: LDO5 {
> + regulator-name = "ldo5";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-boot-on;
> + };
> +
> + ldo6: LDO6 {
> + regulator-name = "ldo6";
> + regulator-min-microvolt = <900000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-boot-on;
> + };
> +
> + ldo7: LDO7 {
> + regulator-name = "ldo7";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-boot-on;
> + };
> + };
> + };
> +};
> +
> +&i2c2 {
> + clock-frequency = <100000>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_i2c2>;
> + status = "okay";
> +};
> +
> +&uart1 { /* console */
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_uart1>;
> + status = "okay";
> +};
> +
> +&usdhc1 {
> + pinctrl-names = "default", "state_100mhz", "state_200mhz";
> + pinctrl-0 = <&pinctrl_usdhc1>;
> + pinctrl-1 = <&pinctrl_usdhc1_100mhz>;
> + pinctrl-2 = <&pinctrl_usdhc1_200mhz>;
> + bus-width = <8>;
> + non-removable;
> + status = "okay";
> +};
> +
> +&usdhc2 {
> + pinctrl-names = "default", "state_100mhz", "state_200mhz";
> + pinctrl-0 = <&pinctrl_usdhc2>, <&pinctrl_usdhc2_gpio>;
> + pinctrl-1 = <&pinctrl_usdhc2_100mhz>, <&pinctrl_usdhc2_gpio>;
> + pinctrl-2 = <&pinctrl_usdhc2_200mhz>, <&pinctrl_usdhc2_gpio>;
> + bus-width = <4>;
> + cd-gpios = <&gpio2 12 GPIO_ACTIVE_LOW>;
> + status = "okay";
> +};
> +
> +&usb3_phy0 {
> + status = "okay";
> +};
> +
> +&usb_dwc3_0 {
> + extcon = <&typec_tusb320>;
> + dr_mode = "otg";
> + status = "okay";
> +};
> +
> +&usb3_phy1 {
> + status = "okay";
> +};
> +
> +&usb_dwc3_1 {
> + status = "okay";
> + dr_mode = "host";
We prefer to end the property list with 'status', so please flip the
order here.
Shawn
> +};
> +
> +&wdog1 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_wdog>;
> + fsl,ext-reset-output;
> + status = "okay";
> +};
> +
> +&A53_0 {
> + operating-points = <
> + /* kHz uV */
> + 1500000 1000000
> + 1300000 1000000
> + 1000000 900000
> + 800000 900000
> + >;
> +};
> --
> 2.11.0
>
^ permalink raw reply
* RE: [EXT] Re: [v5,2/2] Documentation: dt: binding: rtc: add binding for ftm alarm driver
From: Biwen Li @ 2019-07-18 3:33 UTC (permalink / raw)
To: Rob Herring
Cc: Alessandro Zummo, Alexandre Belloni, Leo Li,
open list:REAL TIME CLOCK (RTC) SUBSYSTEM,
linux-kernel@vger.kernel.org, Xiaobo Xie, Jiafei Pan, Ran Wang,
Mark Rutland, devicetree@vger.kernel.org
In-Reply-To: <CAL_JsqKdsZQFtn+Bd2rZFKgZN5pgcsgXZz-Cy0qAt=8ycT0VHA@mail.gmail.com>
> >
> > >
> > > Caution: EXT Email
> > >
> > > On Tue, Jul 16, 2019 at 4:26 AM Biwen Li <biwen.li@nxp.com> wrote:
> > > >
> > > > The patch adds binding for ftm alarm driver
> > >
> > > Bindings are for h/w, not drivers...
> > >
> > > 'dt-bindings: rtc: ...' for the subject prefix.
> > I will correct it in v6.
> > >
> > > >
> > > > Signed-off-by: Biwen Li <biwen.li@nxp.com>
> > > > ---
> > > > Change in v5:
> > > > - None
> > > >
> > > > Change in v4:
> > > > - add note about dts and kernel options
> > > > - add aliases in example
> > > >
> > > > Change in v3:
> > > > - remove reg-names property
> > > > - correct cells number
> > > >
> > > > Change in v2:
> > > > - replace ls1043a with ls1088a as example
> > > > - add rcpm node and fsl,rcpm-wakeup property
> > > >
> > > >
> > > > .../bindings/rtc/rtc-fsl-ftm-alarm.txt | 49
> +++++++++++++++++++
> > > > 1 file changed, 49 insertions(+)
> > > > create mode 100644
> > > > Documentation/devicetree/bindings/rtc/rtc-fsl-ftm-alarm.txt
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/rtc/rtc-fsl-ftm-alarm.txt
> > > > b/Documentation/devicetree/bindings/rtc/rtc-fsl-ftm-alarm.txt
> > > > new file mode 100644
> > > > index 000000000000..fb018065406c
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/rtc/rtc-fsl-ftm-alarm.txt
> > > > @@ -0,0 +1,49 @@
> > > > +Freescale FlexTimer Module (FTM) Alarm
> > > > +
> > > > +Note:
> > > > +- The driver depends on RCPM driver
> > > > + to wake up system in sleep.
> > > > +- Need stop using RTC_HCTOSYS or use the DT aliases
> > > > + to ensure the driver is not used as the primary RTC.
> > > > + (Select DT aliases defaultly)
> > >
> > > This is Linux specific and not relevant to the binding.
> > Ok, I will remove all of the Note in v6.
> > >
> > > > +
> > > > +Required properties:
> > > > +- compatible : Should be "fsl,<chip>-ftm-alarm", the
> > > > + supported chips include
> > > > + "fsl,ls1012a-ftm-alarm"
> > > > + "fsl,ls1021a-ftm-alarm"
> > > > + "fsl,ls1028a-ftm-alarm"
> > > > + "fsl,ls1043a-ftm-alarm"
> > > > + "fsl,ls1046a-ftm-alarm"
> > > > + "fsl,ls1088a-ftm-alarm"
> > > > + "fsl,ls208xa-ftm-alarm"
> > > > +- reg : Specifies base physical address and size of the register
> > > > +sets for the
> > > > + FlexTimer Module and base physical address of IP Powerdown
> > > > +Exception Control
> > > > + Register.
> > > > +- interrupts : Should be the FlexTimer Module interrupt.
> > > > +- fsl,rcpm-wakeup property and rcpm node : Please refer
> > > > + Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> > > > +
> > > > +Optional properties:
> > > > +- big-endian: If the host controller is big-endian mode, specify this
> property.
> > > > + The default endian mode is little-endian.
> > > > +
> > > > +Example:
> > > > +aliases {
> > > > + ...
> > > > + rtc1 = ftm_alarm0; /* Use flextimer alarm driver as /dev/rtc1 */
> > > > + ...
> > > > +};
> > >
> > > Drop the aliases part. It's not going to work when this is converted
> > > to DT schema and the comment is Linux specific.
> > Sorry,I can't drop the aliases part. If any user uses the alarm driver
> > as rtc0, the user will get wrong time from /dev/rtc0.It is very important.
>
> I just mean don't put it in the example. You can use it in your dts file.
Thank you so much,I need put it in the example for a note(Please not use it as rtc0).
I use it as a note in the example to avoid that someone use FlexTimer module as rtc0.
^ permalink raw reply
* Re: [PATCH 0/5]Add support for mt2701 JPEG ENC support
From: mtk12025 @ 2019-07-18 3:19 UTC (permalink / raw)
To: Hans Verkuil
Cc: Rob Herring, Matthias Brugger, Rick Chang, linux-media,
devicetree, linux-kernel, linux-arm-kernel, linux-mediatek,
Marek Szyprowski, Tomasz Figa, srv_heupstream
In-Reply-To: <79316488-30fd-7ff3-7598-d29f85f663ab@xs4all.nl>
Dear Hans,
Thank you for your reply.
PATCH v2 has used the latest v4l2-compliance version for test.
PATCH v2 has fixed the compliance test fail, and the driver checked the
buffer size in queue_setup function.
I am sorry for existence of change-id caused by my mistake in the new
patch.
Best Regards,
Xia Jiang
^ permalink raw reply
* RE: [PATCH] clk: imx8: Add DSP related clocks
From: Aisheng Dong @ 2019-07-18 3:11 UTC (permalink / raw)
To: shawnguo@kernel.org
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, S.j. Wang,
sboyd@kernel.org, Daniel Baluta, mturquette@baylibre.com,
linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org,
robh+dt@kernel.org, weiyongjun1@huawei.com, dl-linux-imx,
kernel@pengutronix.de, festevam@gmail.com, s.hauer@pengutronix.de,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190702152007.12190-1-daniel.baluta@nxp.com>
> From: Daniel Baluta <daniel.baluta@nxp.com>
> Sent: Tuesday, July 2, 2019 11:20 PM
>
> i.MX8QXP contains Hifi4 DSP. There are four clocks associated with DSP:
> * dsp_lpcg_core_clk
> * dsp_lpcg_ipg_clk
> * dsp_lpcg_adb_aclk
> * ocram_lpcg_ipg_clk
>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
Regards
Aisheng
^ permalink raw reply
* RE: [PATCH 3/3] arm64: dts: freescale: Add support for i.MX8QXP AI_ML board
From: Aisheng Dong @ 2019-07-18 3:03 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: shawnguo@kernel.org, s.hauer@pengutronix.de, robh+dt@kernel.org,
kernel@pengutronix.de, festevam@gmail.com, dl-linux-imx,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, Darshak Patel, Kinjan Patel,
Prajose John
In-Reply-To: <20190717113435.GA32330@Mani-XPS-13-9360>
> > > +&adma_lpuart1 {
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <&pinctrl_lpuart1>;
> > > + status = "okay";
> > > +};
> > > +
> > > +/* Debug */
> > > +&adma_lpuart2 {
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <&pinctrl_lpuart2>;
> > > + status = "okay";
> > > +};
> > > +
> > > +/* PCI-E */
> >
> > A bit confusing for the comments...
> >
>
> Hmm. How about, "PCI-E UART"?
>
It seems to be related to your board. So up to you.
I'm fine with it.
Regards
Aisheng
^ permalink raw reply
* Re: [PATCH v5 6/6] ARM: dts: imx7ulp: add edma device node
From: Shawn Guo @ 2019-07-18 2:49 UTC (permalink / raw)
To: yibin.gong
Cc: robh, s.hauer, festevam, mark.rutland, vkoul, dan.j.williams,
angelo, linux-imx, linux-arm-kernel, linux-kernel, dmaengine,
devicetree, kernel
In-Reply-To: <20190625094324.19196-7-yibin.gong@nxp.com>
On Tue, Jun 25, 2019 at 05:43:24PM +0800, yibin.gong@nxp.com wrote:
> From: Robin Gong <yibin.gong@nxp.com>
>
> Add edma device node in dts.
>
> Signed-off-by: Robin Gong <yibin.gong@nxp.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH 0/2] arm64: dts: imx8mq: Add DT node for the Mixel MIPI D-PHY
From: Shawn Guo @ 2019-07-18 2:42 UTC (permalink / raw)
To: Guido Günther
Cc: Mark Rutland, devicetree, Pavel Machek, Anson Huang, Carlo Caione,
Andrey Smirnov, Sascha Hauer, Angus Ainslie (Purism),
linux-kernel, Abel Vesa, Rob Herring, NXP Linux Team,
Pengutronix Kernel Team, Fabio Estevam, linux-arm-kernel,
Lucas Stach
In-Reply-To: <cover.1561451144.git.agx@sigxcpu.org>
On Tue, Jun 25, 2019 at 10:27:19AM +0200, Guido Günther wrote:
> Now that the driver is in linux-next as of 20190624 let's have a DT node
> for the i.MX8mq and enable it on the Librem 5 devkit.
>
> Guido Günther (2):
> arm64: dts: imx8mq: Add MIPI D-PHY
> arm64: dts: imx8mq-librem5: Enable MIPI D-PHY
Applied both, thanks.
^ permalink raw reply
* Re: [EXT] Re: [v5,2/2] Documentation: dt: binding: rtc: add binding for ftm alarm driver
From: Rob Herring @ 2019-07-18 1:54 UTC (permalink / raw)
To: Biwen Li
Cc: Alessandro Zummo, Alexandre Belloni, Leo Li,
open list:REAL TIME CLOCK (RTC) SUBSYSTEM,
linux-kernel@vger.kernel.org, Xiaobo Xie, Jiafei Pan, Ran Wang,
Mark Rutland, devicetree@vger.kernel.org
In-Reply-To: <DB7PR04MB44902483232D323AF611D6298FC90@DB7PR04MB4490.eurprd04.prod.outlook.com>
On Wed, Jul 17, 2019 at 4:05 AM Biwen Li <biwen.li@nxp.com> wrote:
>
> >
> > Caution: EXT Email
> >
> > On Tue, Jul 16, 2019 at 4:26 AM Biwen Li <biwen.li@nxp.com> wrote:
> > >
> > > The patch adds binding for ftm alarm driver
> >
> > Bindings are for h/w, not drivers...
> >
> > 'dt-bindings: rtc: ...' for the subject prefix.
> I will correct it in v6.
> >
> > >
> > > Signed-off-by: Biwen Li <biwen.li@nxp.com>
> > > ---
> > > Change in v5:
> > > - None
> > >
> > > Change in v4:
> > > - add note about dts and kernel options
> > > - add aliases in example
> > >
> > > Change in v3:
> > > - remove reg-names property
> > > - correct cells number
> > >
> > > Change in v2:
> > > - replace ls1043a with ls1088a as example
> > > - add rcpm node and fsl,rcpm-wakeup property
> > >
> > >
> > > .../bindings/rtc/rtc-fsl-ftm-alarm.txt | 49 +++++++++++++++++++
> > > 1 file changed, 49 insertions(+)
> > > create mode 100644
> > > Documentation/devicetree/bindings/rtc/rtc-fsl-ftm-alarm.txt
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/rtc/rtc-fsl-ftm-alarm.txt
> > > b/Documentation/devicetree/bindings/rtc/rtc-fsl-ftm-alarm.txt
> > > new file mode 100644
> > > index 000000000000..fb018065406c
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/rtc/rtc-fsl-ftm-alarm.txt
> > > @@ -0,0 +1,49 @@
> > > +Freescale FlexTimer Module (FTM) Alarm
> > > +
> > > +Note:
> > > +- The driver depends on RCPM driver
> > > + to wake up system in sleep.
> > > +- Need stop using RTC_HCTOSYS or use the DT aliases
> > > + to ensure the driver is not used as the primary RTC.
> > > + (Select DT aliases defaultly)
> >
> > This is Linux specific and not relevant to the binding.
> Ok, I will remove all of the Note in v6.
> >
> > > +
> > > +Required properties:
> > > +- compatible : Should be "fsl,<chip>-ftm-alarm", the
> > > + supported chips include
> > > + "fsl,ls1012a-ftm-alarm"
> > > + "fsl,ls1021a-ftm-alarm"
> > > + "fsl,ls1028a-ftm-alarm"
> > > + "fsl,ls1043a-ftm-alarm"
> > > + "fsl,ls1046a-ftm-alarm"
> > > + "fsl,ls1088a-ftm-alarm"
> > > + "fsl,ls208xa-ftm-alarm"
> > > +- reg : Specifies base physical address and size of the register sets
> > > +for the
> > > + FlexTimer Module and base physical address of IP Powerdown
> > > +Exception Control
> > > + Register.
> > > +- interrupts : Should be the FlexTimer Module interrupt.
> > > +- fsl,rcpm-wakeup property and rcpm node : Please refer
> > > + Documentation/devicetree/bindings/soc/fsl/rcpm.txt
> > > +
> > > +Optional properties:
> > > +- big-endian: If the host controller is big-endian mode, specify this property.
> > > + The default endian mode is little-endian.
> > > +
> > > +Example:
> > > +aliases {
> > > + ...
> > > + rtc1 = ftm_alarm0; /* Use flextimer alarm driver as /dev/rtc1 */
> > > + ...
> > > +};
> >
> > Drop the aliases part. It's not going to work when this is converted to DT schema
> > and the comment is Linux specific.
> Sorry,I can't drop the aliases part. If any user uses the alarm driver as rtc0,
> the user will get wrong time from /dev/rtc0.It is very important.
I just mean don't put it in the example. You can use it in your dts file.
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller
From: Andrew Jeffery @ 2019-07-18 1:49 UTC (permalink / raw)
To: Rob Herring
Cc: linux-mmc, Ulf Hansson, Mark Rutland, Joel Stanley, Adrian Hunter,
devicetree,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-aspeed, linux-kernel@vger.kernel.org, Ryan Chen
In-Reply-To: <CAL_JsqKYL5yv8E=aKD1fJwXx1LLdUAs_ZFrc5k1dHsB9u+2ing@mail.gmail.com>
On Wed, 17 Jul 2019, at 23:13, Rob Herring wrote:
> On Tue, Jul 16, 2019 at 9:58 PM Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> >
> >
> > On Wed, 17 Jul 2019, at 00:27, Rob Herring wrote:
> > > On Mon, Jul 15, 2019 at 6:36 PM Andrew Jeffery <andrew@aj.id.au> wrote:
> > > >
> > > >
> > > >
> > > > On Tue, 16 Jul 2019, at 07:47, Rob Herring wrote:
> > > > > On Thu, Jul 11, 2019 at 9:32 PM Andrew Jeffery <andrew@aj.id.au> wrote:
> > > > > >
> > > > > > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the
> > > > > > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit
> > > > > > data bus if only a single slot is enabled.
> > > > > >
> > > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > > > > ---
> > > > > > In v2:
> > > > > >
> > > > > > * Rename to aspeed,sdhci.yaml
> > > > > > * Rename sd-controller compatible
> > > > > > * Add `maxItems: 1` for reg properties
> > > > > > * Move sdhci subnode description to patternProperties
> > > > > > * Drop sdhci compatible requirement
> > > > > > * #address-cells and #size-cells are required
> > > > > > * Prevent additional properties
> > > > > > * Implement explicit ranges in example
> > > > > > * Remove slot property
> > > > > >
> > > > > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++
> > > > > > 1 file changed, 90 insertions(+)
> > > > > > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > > > new file mode 100644
> > > > > > index 000000000000..67a691c3348c
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > > > @@ -0,0 +1,90 @@
> > > > > > +# SPDX-License-Identifier: GPL-2.0-or-later
> > > > > > +%YAML 1.2
> > > > > > +---
> > > > > > +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml#
> > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > +
> > > > > > +title: ASPEED SD/SDIO/eMMC Controller
> > > > > > +
> > > > > > +maintainers:
> > > > > > + - Andrew Jeffery <andrew@aj.id.au>
> > > > > > + - Ryan Chen <ryanchen.aspeed@gmail.com>
> > > > > > +
> > > > > > +description: |+
> > > > > > + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO
> > > > > > + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if
> > > > > > + only a single slot is enabled.
> > > > > > +
> > > > > > + The two slots are supported by a common configuration area. As the SDHCIs for
> > > > > > + the slots are dependent on the common configuration area, they are described
> > > > > > + as child nodes.
> > > > > > +
> > > > > > +properties:
> > > > > > + compatible:
> > > > > > + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ]
> > > > >
> > > > > This is actually a list of 4 strings. Please reformat to 1 per line.
> > > >
> > > > On reflection that's obvious, but also a somewhat subtle interaction with the
> > > > preference for no quotes (the obvious caveat being "except where required").
> > >
> > > It wasn't something I'd run into before. I'm working on a check, but
> > > unfortunately we can only check for quotes not needed and can't check
> > > for missing quotes.
> > >
> > > > Thanks for pointing it out.
> > > >
> > > > I have been running `make dt_binding_check` and `make dtbs_check` over
> > > > these, looks like I need to up my game a bit though. Do you do additional things
> > > > in your workflow?
> > >
> > > That should have thrown the warnings. If you aren't seeing those, do
> > > you have dtschema package installed (see
> > > Documentation/devicetree/writing-schema.md)?
> >
> > I do have it installed, but as mentioned previously there's a fair few
> > warnings emitted currently by the Aspeed devicetrees, so it might have
> > got lost in the noise. I've started to clean that up, though probably need
> > some direction there too.
> >
> > Separately I'm currently trying to track down an issue where I get errors
> > on the Aspeed dts cpu nodes about failing to match the riscv CPU
> > compatibles, it seems dt-validate isn't finding the ARM CPU compatible
> > strings. It feels more annoying to track down that I'd like.
>
> There's a fix in today's linux-next for that and it should be in
> Linus' tree in a few days.
>
Thanks, I'll take a look.
Andrew
^ permalink raw reply
* Re: [PATCH 2/3 v4] dt-bindings: gpio: aspeed: Add SGPIO support
From: Andrew Jeffery @ 2019-07-18 1:48 UTC (permalink / raw)
To: Hongwei Zhang, Joel Stanley, Linus Walleij, devicetree
Cc: Rob Herring, Mark Rutland, Bartosz Golaszewski, linux-aspeed,
linux-kernel, linux-arm-kernel, linux-gpio
In-Reply-To: <1563394325-15941-1-git-send-email-hongweiz@ami.com>
The subject is largely correct, but please see the discussion on the driver patch
about how to clean up the [PATCH ...] prefix.
On Thu, 18 Jul 2019, at 05:42, Hongwei Zhang wrote:
> Add bindings to support SGPIO on AST2400 or AST2500.
>
> Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> ---
> .../devicetree/bindings/gpio/sgpio-aspeed.txt | 55 ++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> new file mode 100644
> index 0000000..2d6305e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> @@ -0,0 +1,55 @@
> +Aspeed SGPIO controller Device Tree Bindings
> +-------------------------------------------
> +
> +This SGPIO controller is for ASPEED AST2500 SoC, it supports up to 80
> full
> +featured Serial GPIOs. Each of the Serial GPIO pins can be programmed
> to
> +support the following options:
> +- Support interrupt option for each input port and various interrupt
> + sensitivity option (level-high, level-low, edge-high, edge-low)
> +- Support reset tolerance option for each output port
> +- Directly connected to APB bus and its shift clock is from APB bus
> clock
> + divided by a programmable value.
> +- Co-work with external signal-chained TTL components (74LV165/74LV595)
> +
> +
> +Required properties:
> +
> +- compatible : Either "aspeed,ast2400-sgpio" or "aspeed,ast2500-sgpio"
> +
> +- #gpio-cells : Should be two
> + - First cell is the GPIO line number
> + - Second cell is used to specify optional
> + parameters (unused)
> +
> +- reg : Address and length of the register set for the device
> +- gpio-controller : Marks the device node as a GPIO controller
> +- interrupts : Interrupt specifier (see interrupt bindings for
> + details)
> +
> +- interrupt-controller : Mark the GPIO controller as an
> interrupt-controller
> +
> +- nr-gpios : number of GPIO pins to serialise.
> + (should be multiple of 8, up to 80 pins)
Please change the property name to "ngpios", as per the generic GPIO
bindings[1].
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/gpio/gpio.txt?h=v5.2#n141
Cheers,
Andrew
> +
> +- clocks : A phandle to the APB clock for SGPM clock
> division
> +
> +- bus-frequency : SGPM CLK frequency
> +
> +
> +The sgpio and interrupt properties are further described in their
> respective bindings documentation:
> +
> +- Documentation/devicetree/bindings/sgpio/gpio.txt
> +- Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> +
> + Example:
> + sgpio: sgpio@1e780200 {
> + #gpio-cells = <2>;
> + compatible = "aspeed,ast2500-sgpio";
> + gpio-controller;
> + interrupts = <40>;
> + reg = <0x1e780200 0x0100>;
> + clocks = <&syscon ASPEED_CLK_APB>;
> + interrupt-controller;
> + nr-gpios = <8>;
> + bus-frequency = <12000000>;
> + };
> --
> 2.7.4
>
>
^ permalink raw reply
* RE: [PATCH v3 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
From: Peng Fan @ 2019-07-18 1:47 UTC (permalink / raw)
To: Sudeep Holla
Cc: robh+dt@kernel.org, mark.rutland@arm.com,
jassisinghbrar@gmail.com, andre.przywara@arm.com,
f.fainelli@gmail.com, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, dl-linux-imx
In-Reply-To: <20190717172801.GG18370@e107155-lin>
Hi Sudeep,
> Subject: Re: [PATCH v3 1/2] dt-bindings: mailbox: add binding doc for the ARM
> SMC/HVC mailbox
>
> This looks much better now.
>
> On Mon, Jul 15, 2019 at 10:10:10AM +0000, Peng Fan wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > The ARM SMC/HVC mailbox binding describes a firmware interface to
> > trigger actions in software layers running in the EL2 or EL3 exception levels.
> > The term "ARM" here relates to the SMC instruction as part of the ARM
> > instruction set, not as a standard endorsed by ARM Ltd.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >
> > V3:
> > Convert to yaml
> > Drop interrupt
> > Introudce transports to indicate mem/reg The func id is still kept
> > as optional, because like SCMI it only cares about message.
> >
> > V2:
> > Introduce interrupts as a property.
> >
> > .../devicetree/bindings/mailbox/arm-smc.yaml | 124
> +++++++++++++++++++++
> > 1 file changed, 124 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > new file mode 100644
> > index 000000000000..da9b1a03bc4e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > @@ -0,0 +1,124 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fschemas%2Fmailbox%2Farm-smc.yaml%23&data=02%7
> C01%7Cp
> >
> +eng.fan%40nxp.com%7Cb5039d50ce8c40928edb08d70adc20f9%7C686ea1
> d3bc2b4c
> >
> +6fa92cd99c5c301635%7C0%7C1%7C636989812923178414&sdata=UT
> 7r2vOLX4a
> > +tv7Yfh750wdSXSh2ZPxeJOXLWl5yACK0%3D&reserved=0
> > +$schema:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=02%7C01%7Cpe
> ng.fan%
> >
> +40nxp.com%7Cb5039d50ce8c40928edb08d70adc20f9%7C686ea1d3bc2b4c
> 6fa92cd9
> >
> +9c5c301635%7C0%7C1%7C636989812923178414&sdata=3Gjn1NQtO
> PbvfTvyN3X
> > +b89%2BvvGO2ff6DpGQUQejGAzU%3D&reserved=0
> > +
> > +title: ARM SMC Mailbox Interface
> > +
> > +maintainers:
> > + - Peng Fan <peng.fan@nxp.com>
> > +
> > +description: |
> > + This mailbox uses the ARM smc (secure monitor call) and hvc
> > +(hypervisor
> > + call) instruction to trigger a mailbox-connected activity in
> > +firmware,
> > + executing on the very same core as the caller. By nature this
> > +operation
> > + is synchronous and this mailbox provides no way for asynchronous
> > +messages
> > + to be delivered the other way round, from firmware to the OS, but
> > + asynchronous notification could also be supported. However the
> > +value of
> > + r0/w0/x0 the firmware returns after the smc call is delivered as a
> > +received
> > + message to the mailbox framework, so a synchronous communication
> > +can be
> > + established, for a asynchronous notification, no value will be returned.
> > + The exact meaning of both the action the mailbox triggers as well
> > +as the
> > + return value is defined by their users and is not subject to this binding.
> > +
> > + One use case of this mailbox is the SCMI interface, which uses
> > + shared memory to transfer commands and parameters, and a mailbox
> to
> > + trigger a function call. This allows SoCs without a separate
> > + management processor (or when such a processor is not available or
> > + used) to use this standardized interface anyway.
> > +
> > + This binding describes no hardware, but establishes a firmware
> interface.
> > + Upon receiving an SMC using one of the described SMC function
> > + identifiers, the firmware is expected to trigger some mailbox connected
> functionality.
> > + The communication follows the ARM SMC calling convention.
> > + Firmware expects an SMC function identifier in r0 or w0. The
> > + supported identifiers are passed from consumers, or listed in the
> > + the arm,func-ids properties as described below. The firmware can
> > + return one value in the first SMC result register, it is expected
> > + to be an error value, which shall be propagated to the mailbox client.
> > +
> > + Any core which supports the SMC or HVC instruction can be used, as
> > + long as a firmware component running in EL3 or EL2 is handling these
> calls.
> > +
> > +properties:
> > + compatible:
> > + const: arm,smc-mbox
> > +
> > + "#mbox-cells":
> > + const: 1
> > +
> > + arm,num-chans:
> > + description: The number of channels supported.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > + method:
> > + items:
> > + - enum:
> > + - smc
> > + - hvc
> > +
> > + transports:
> > + items:
> > + - enum:
> > + - mem
> > + - reg
> > +
> > + arm,func-ids:
> > + description: |
> > + An array of 32-bit values specifying the function IDs used by each
> > + mailbox channel. Those function IDs follow the ARM SMC calling
> > + convention standard [1].
> > +
> > + There is one identifier per channel and the number of supported
> > + channels is determined by the length of this array.
> > + minItems: 0
> > + maxItems: 4096 # Should be enough?
>
> I am new to yaml, is there a way to say the number of entries here must
> match arm,num-chans ? And not sure if min/maxItems matter then ?
I am also new to yaml, Rob might have ideas.
>
> > +
> > +required:
> > + - compatible
> > + - "#mbox-cells"
> > + - arm,num-chans
> > + - transports
> > + - method
> > +
>
> Why is arm,func-ids optional ? Is there any standard arm,func-ids we can
> resort to. Sorry I know you expect ARM Ltd to answer that, but I just want to
> raise the point that we don't have one today and hence it can't be optional. Or
> I am missing something ?
In the v3 patchset, Jassi expected the id be passed from client firmware driver,
but you said scmi is expected a only message protocol. So here I still keep this
as optional, if specified in dts, the mailbox driver will use it. If not specificed
in dts, the mailbox driver expect the client firmware driver pass the func id to
mailbox driver.
Thanks,
Peng.
>
> --
> Regards,
> Sudeep
^ permalink raw reply
* RE: [PATCH v3 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
From: Peng Fan @ 2019-07-18 1:42 UTC (permalink / raw)
To: Rob Herring
Cc: mark.rutland@arm.com, jassisinghbrar@gmail.com,
sudeep.holla@arm.com, andre.przywara@arm.com,
f.fainelli@gmail.com, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, dl-linux-imx
In-Reply-To: <CAL_JsqJkt7pX9F9NggL2EXxS=2oiF07VJCOqVTvF-Zwz=cjmvg@mail.gmail.com>
Hi Rob,
> Subject: Re: [PATCH v3 1/2] dt-bindings: mailbox: add binding doc for the ARM
> SMC/HVC mailbox
>
> On Mon, Jul 15, 2019 at 4:10 AM Peng Fan <peng.fan@nxp.com> wrote:
> >
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > The ARM SMC/HVC mailbox binding describes a firmware interface to
> > trigger actions in software layers running in the EL2 or EL3 exception levels.
> > The term "ARM" here relates to the SMC instruction as part of the ARM
> > instruction set, not as a standard endorsed by ARM Ltd.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >
> > V3:
> > Convert to yaml
> > Drop interrupt
> > Introudce transports to indicate mem/reg The func id is still kept
> > as optional, because like SCMI it only cares about message.
> >
> > V2:
> > Introduce interrupts as a property.
> >
> > .../devicetree/bindings/mailbox/arm-smc.yaml | 124
> +++++++++++++++++++++
> > 1 file changed, 124 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > new file mode 100644
> > index 000000000000..da9b1a03bc4e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> > @@ -0,0 +1,124 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fschemas%2Fmailbox%2Farm-smc.yaml%23&data=02%7
> C01%7Cp
> >
> +eng.fan%40nxp.com%7C424e0d1c19c344406b6008d709465591%7C686ea1
> d3bc2b4c
> >
> +6fa92cd99c5c301635%7C0%7C0%7C636988070002772705&sdata=DV
> stQ%2FhuN
> > +c67%2Bt08yXibQrX7sIeocHziYp3dkkeRoJ4%3D&reserved=0
> > +$schema:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=02%7C01%7Cpe
> ng.fan%
> >
> +40nxp.com%7C424e0d1c19c344406b6008d709465591%7C686ea1d3bc2b4
> c6fa92cd9
> >
> +9c5c301635%7C0%7C0%7C636988070002782698&sdata=D%2Fa2SU
> W%2FCqclJdy
> > +RbFggqqL%2BAEumER0K3rAaisY2bMc%3D&reserved=0
> > +
> > +title: ARM SMC Mailbox Interface
> > +
> > +maintainers:
> > + - Peng Fan <peng.fan@nxp.com>
> > +
> > +description: |
> > + This mailbox uses the ARM smc (secure monitor call) and hvc
> > +(hypervisor
> > + call) instruction to trigger a mailbox-connected activity in
> > +firmware,
> > + executing on the very same core as the caller. By nature this
> > +operation
> > + is synchronous and this mailbox provides no way for asynchronous
> > +messages
> > + to be delivered the other way round, from firmware to the OS, but
> > + asynchronous notification could also be supported. However the
> > +value of
> > + r0/w0/x0 the firmware returns after the smc call is delivered as a
> > +received
> > + message to the mailbox framework, so a synchronous communication
> > +can be
> > + established, for a asynchronous notification, no value will be returned.
> > + The exact meaning of both the action the mailbox triggers as well
> > +as the
> > + return value is defined by their users and is not subject to this binding.
> > +
> > + One use case of this mailbox is the SCMI interface, which uses
> > + shared memory to transfer commands and parameters, and a mailbox
> to
> > + trigger a function call. This allows SoCs without a separate
> > + management processor (or when such a processor is not available or
> > + used) to use this standardized interface anyway.
> > +
> > + This binding describes no hardware, but establishes a firmware
> interface.
> > + Upon receiving an SMC using one of the described SMC function
> > + identifiers, the firmware is expected to trigger some mailbox connected
> functionality.
> > + The communication follows the ARM SMC calling convention.
> > + Firmware expects an SMC function identifier in r0 or w0. The
> > + supported identifiers are passed from consumers, or listed in the
> > + the arm,func-ids properties as described below. The firmware can
> > + return one value in the first SMC result register, it is expected
> > + to be an error value, which shall be propagated to the mailbox client.
> > +
> > + Any core which supports the SMC or HVC instruction can be used, as
> > + long as a firmware component running in EL3 or EL2 is handling these
> calls.
> > +
> > +properties:
> > + compatible:
> > + const: arm,smc-mbox
> > +
> > + "#mbox-cells":
> > + const: 1
> > +
> > + arm,num-chans:
> > + description: The number of channels supported.
> > + $ref: /schemas/types.yaml#/definitions/uint32
>
> Constraints? 0 is valid? 2^32?
0 is not valid. There should be limited channels, but depends on firmware design.
>
> > +
> > + method:
> > + items:
> > + - enum:
> > + - smc
> > + - hvc
> > +
> > + transports:
> > + items:
> > + - enum:
> > + - mem
> > + - reg
>
> What if someone wants to configure this per channel? Perhaps #mbox-cells
> should be 2 and this can be a client parameter.
I need to check. Currently I only use one type. There might be people
want to use different transports for each channels.
>
> Minimally, this needs a 'arm' vendor prefix if it stays.
"arm,transports" in v4.
>
> > +
> > + arm,func-ids:
> > + description: |
> > + An array of 32-bit values specifying the function IDs used by each
> > + mailbox channel. Those function IDs follow the ARM SMC calling
> > + convention standard [1].
>
> What's the default if not specified? Or this should be required?
If not specified, it means the client firmware driver will pass it to mailbox driver.
Thanks,
Peng.
>
> > +
> > + There is one identifier per channel and the number of supported
> > + channels is determined by the length of this array.
> > + minItems: 0
> > + maxItems: 4096 # Should be enough?
> > +
> > +required:
> > + - compatible
> > + - "#mbox-cells"
> > + - arm,num-chans
> > + - transports
> > + - method
> > +
> > +examples:
> > + - |
> > + sram@910000 {
> > + compatible = "mmio-sram";
> > + reg = <0x0 0x93f000 0x0 0x1000>;
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + ranges = <0 0x0 0x93f000 0x1000>;
> > +
> > + cpu_scp_lpri: scp-shmem@0 {
> > + compatible = "arm,scmi-shmem";
> > + reg = <0x0 0x200>;
> > + };
> > +
> > + cpu_scp_hpri: scp-shmem@200 {
> > + compatible = "arm,scmi-shmem";
> > + reg = <0x200 0x200>;
> > + };
> > + };
> > +
> > + firmware {
> > + smc_mbox: mailbox {
> > + #mbox-cells = <1>;
> > + compatible = "arm,smc-mbox";
> > + method = "smc";
> > + arm,num-chans = <0x2>;
> > + transports = "mem";
> > + /* Optional */
> > + arm,func-ids = <0xc20000fe>, <0xc20000ff>;
> > + };
> > +
> > + scmi {
> > + compatible = "arm,scmi";
> > + mboxes = <&mailbox 0 &mailbox 1>;
> > + mbox-names = "tx", "rx";
> > + shmem = <&cpu_scp_lpri &cpu_scp_hpri>;
> > + };
> > + };
> > +
> > +...
> > --
> > 2.16.4
> >
^ 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