| Message ID | 20240807123333.2056518-1-quic_depengs@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | arm64: dts: qcom: sm8550: camss: Add CAMSS block definition | expand |
On 07/08/2024 14:33, Depeng Shao wrote: > Add CAMSS block definition for sm8550. > > This drop contains definitions for the following components on sm8550: 1. Subject: there is no prefix camss. There is no such file, directory or module. 2. You already sent this, so this should be v2 or v3 or vX. Provide changelog under ---. If there is going to be resend, please fix above. 3. If this was tested on aim300, I am surprised this being not enabled on aim300. Best regards, Krzysztof
On 07/08/2024 14:39, Krzysztof Kozlowski wrote: > On 07/08/2024 14:33, Depeng Shao wrote: >> Add CAMSS block definition for sm8550. >> >> This drop contains definitions for the following components on sm8550: > > 1. Subject: there is no prefix camss. There is no such file, directory > or module. > > 2. You already sent this, so this should be v2 or v3 or vX. Provide > changelog under ---. > > If there is going to be resend, please fix above. > > 3. If this was tested on aim300, I am surprised this being not enabled > on aim300. One more thing, bindings were not accepted, thus this patch should not go in. There were no new bindings, so I assume patchset is using rejected ones. It's fine to send it to get some comments, although would be nice to mention to maintainer that this cannot be picked up as is. :( Best regards, Krzysztof
Hi Krzysztof, On 8/7/2024 8:43 PM, Krzysztof Kozlowski wrote: > On 07/08/2024 14:39, Krzysztof Kozlowski wrote: >> On 07/08/2024 14:33, Depeng Shao wrote: >>> Add CAMSS block definition for sm8550. >>> >>> This drop contains definitions for the following components on sm8550: >> >> 1. Subject: there is no prefix camss. There is no such file, directory >> or module. >> Thanks for the comment, will remove this. >> 2. You already sent this, so this should be v2 or v3 or vX. Provide >> changelog under ---. >> >> If there is going to be resend, please fix above. >> Sure, I thought it might be a new series, so I didn't add v*, will add v1, and v2 change log in new version series. >> 3. If this was tested on aim300, I am surprised this being not enabled >> on aim300. > It was tested long times ago, but the patches wasn't sent out for reviewing early due to the team's internal schedule. > One more thing, bindings were not accepted, thus this patch should not > go in. There were no new bindings, so I assume patchset is using > rejected ones. > > It's fine to send it to get some comments, although would be nice to > mention to maintainer that this cannot be picked up as is. :( > Sure, I will resend the dtsi patch until the bindings are accepted, send this patches because you posted the comments in other series. https://lore.kernel.org/all/0324e8e8-2ad4-4ce6-9616-3038b8e02ff9@quicinc.com/ Thanks, Depeng
On 07/08/2024 13:53, Depeng Shao wrote: > Hi Krzysztof, > > On 8/7/2024 8:43 PM, Krzysztof Kozlowski wrote: >> On 07/08/2024 14:39, Krzysztof Kozlowski wrote: >>> On 07/08/2024 14:33, Depeng Shao wrote: >>>> Add CAMSS block definition for sm8550. >>>> >>>> This drop contains definitions for the following components on sm8550: >>> >>> 1. Subject: there is no prefix camss. There is no such file, directory >>> or module. >>> > > Thanks for the comment, will remove this. > >>> 2. You already sent this, so this should be v2 or v3 or vX. Provide >>> changelog under ---. >>> >>> If there is going to be resend, please fix above. >>> > > Sure, I thought it might be a new series, so I didn't add v*, will add > v1, and v2 change log in new version series. > >>> 3. If this was tested on aim300, I am surprised this being not enabled >>> on aim300. >> > > It was tested long times ago, but the patches wasn't sent out for > reviewing early due to the team's internal schedule. > >> One more thing, bindings were not accepted, thus this patch should not >> go in. There were no new bindings, so I assume patchset is using >> rejected ones. >> >> It's fine to send it to get some comments, although would be nice to >> mention to maintainer that this cannot be picked up as is. :( >> > > Sure, I will resend the dtsi patch until the bindings are accepted, send > this patches because you posted the comments in other series. > > https://lore.kernel.org/all/0324e8e8-2ad4-4ce6-9616-3038b8e02ff9@quicinc.com/ > > Thanks, > Depeng > > Recommend 1. Send out your yaml and dts in one series 2. Driver series can be posted in parallel 3. Once #1 and #2 get merged send our your platform dtsi Make clear in the cover letter with links to previous series such as https://lore.kernel.org/all/0324e8e8-2ad4-4ce6-9616-3038b8e02ff9@quicinc.com/ that you are breaking the series up for easier/better merging and ensure in the cover letters you explain what you've done to address previous comments. One nice way to give someone like Krzysztof an overview is to post a complete series to codelinaro or github showing all of your patches stacked on top of each other. The merge order then would be 1 -> 2 -> 3, yaml/dts -> driver -> dtsi That way you never have missing compat/dts/yaml splats, your driver code gets reviewed/tested/merged and only after all of that you "switch it on" for your target platform. The point of making a public tree containing everything is you can reasonably point to and endpoint that lets people know whats coming and that indeed a target platform intends to be brought in so that we don't end up doing a bunch of review/merge work for a platform/dtsi that just lives in downstream tree forever. The ordering of patches is 100% up to you but, I find the 1 -> 2 -> 3 sequencing easiest. --- bod
On 07/08/2024 16:17, Bryan O'Donoghue wrote: > On 07/08/2024 13:53, Depeng Shao wrote: >> Hi Krzysztof, >> >> On 8/7/2024 8:43 PM, Krzysztof Kozlowski wrote: >>> On 07/08/2024 14:39, Krzysztof Kozlowski wrote: >>>> On 07/08/2024 14:33, Depeng Shao wrote: >>>>> Add CAMSS block definition for sm8550. >>>>> >>>>> This drop contains definitions for the following components on sm8550: >>>> >>>> 1. Subject: there is no prefix camss. There is no such file, directory >>>> or module. >>>> >> >> Thanks for the comment, will remove this. >> >>>> 2. You already sent this, so this should be v2 or v3 or vX. Provide >>>> changelog under ---. >>>> >>>> If there is going to be resend, please fix above. >>>> >> >> Sure, I thought it might be a new series, so I didn't add v*, will add >> v1, and v2 change log in new version series. >> >>>> 3. If this was tested on aim300, I am surprised this being not enabled >>>> on aim300. >>> >> >> It was tested long times ago, but the patches wasn't sent out for >> reviewing early due to the team's internal schedule. >> >>> One more thing, bindings were not accepted, thus this patch should not >>> go in. There were no new bindings, so I assume patchset is using >>> rejected ones. >>> >>> It's fine to send it to get some comments, although would be nice to >>> mention to maintainer that this cannot be picked up as is. :( >>> >> >> Sure, I will resend the dtsi patch until the bindings are accepted, send >> this patches because you posted the comments in other series. >> >> https://lore.kernel.org/all/0324e8e8-2ad4-4ce6-9616-3038b8e02ff9@quicinc.com/ >> >> Thanks, >> Depeng >> >> > > Recommend > > 1. Send out your yaml and dts in one series > > 2. Driver series can be posted in parallel The binding should go with the driver. Also usually discussion about driver brings comments, thus changes, to the bindings. Sorry, DTSI and DTS should wait till bindings got accepted to media subsystem. Best regards, Krzysztof
On 07/08/2024 15:51, Krzysztof Kozlowski wrote: > On 07/08/2024 16:17, Bryan O'Donoghue wrote: >> On 07/08/2024 13:53, Depeng Shao wrote: >>> Hi Krzysztof, >>> >>> On 8/7/2024 8:43 PM, Krzysztof Kozlowski wrote: >>>> On 07/08/2024 14:39, Krzysztof Kozlowski wrote: >>>>> On 07/08/2024 14:33, Depeng Shao wrote: >>>>>> Add CAMSS block definition for sm8550. >>>>>> >>>>>> This drop contains definitions for the following components on sm8550: >>>>> >>>>> 1. Subject: there is no prefix camss. There is no such file, directory >>>>> or module. >>>>> >>> >>> Thanks for the comment, will remove this. >>> >>>>> 2. You already sent this, so this should be v2 or v3 or vX. Provide >>>>> changelog under ---. >>>>> >>>>> If there is going to be resend, please fix above. >>>>> >>> >>> Sure, I thought it might be a new series, so I didn't add v*, will add >>> v1, and v2 change log in new version series. >>> >>>>> 3. If this was tested on aim300, I am surprised this being not enabled >>>>> on aim300. >>>> >>> >>> It was tested long times ago, but the patches wasn't sent out for >>> reviewing early due to the team's internal schedule. >>> >>>> One more thing, bindings were not accepted, thus this patch should not >>>> go in. There were no new bindings, so I assume patchset is using >>>> rejected ones. >>>> >>>> It's fine to send it to get some comments, although would be nice to >>>> mention to maintainer that this cannot be picked up as is. :( >>>> >>> >>> Sure, I will resend the dtsi patch until the bindings are accepted, send >>> this patches because you posted the comments in other series. >>> >>> https://lore.kernel.org/all/0324e8e8-2ad4-4ce6-9616-3038b8e02ff9@quicinc.com/ >>> >>> Thanks, >>> Depeng >>> >>> >> >> Recommend >> >> 1. Send out your yaml and dts in one series >> >> 2. Driver series can be posted in parallel > > The binding should go with the driver. Also usually discussion about > driver brings comments, thus changes, to the bindings. > > Sorry, DTSI and DTS should wait till bindings got accepted to media > subsystem. Yes you're right 1. Yaml - bindings 2. dts + driver 3. dtsi In this case @Depeng remember to 1. Link back to the older series in your cover letters 2. Suggested by recommended - publish a complete tree somewhere and link to that tree in your cover letters Its fine IMO to restart the version number of your series when breaking up into smaller series, so long as you remember to link to the previous series and explain in the cover letter whats going on. --- bod
Hi Kryysztof, Bryan, On 8/7/2024 10:58 PM, Bryan O'Donoghue wrote: > On 07/08/2024 15:51, Krzysztof Kozlowski wrote: >> On 07/08/2024 16:17, Bryan O'Donoghue wrote: >>> On 07/08/2024 13:53, Depeng Shao wrote: >>>> Hi Krzysztof, >>>> >>>> On 8/7/2024 8:43 PM, Krzysztof Kozlowski wrote: >>>>> On 07/08/2024 14:39, Krzysztof Kozlowski wrote: >>>>>> On 07/08/2024 14:33, Depeng Shao wrote: >>>>>>> Add CAMSS block definition for sm8550. >>>>>>> >>>>>>> This drop contains definitions for the following components on >>>>>>> sm8550: >>>>>> >>>>>> 1. Subject: there is no prefix camss. There is no such file, >>>>>> directory >>>>>> or module. >>>>>> >>>> >>>> Thanks for the comment, will remove this. >>>> >>>>>> 2. You already sent this, so this should be v2 or v3 or vX. Provide >>>>>> changelog under ---. >>>>>> >>>>>> If there is going to be resend, please fix above. >>>>>> >>>> >>>> Sure, I thought it might be a new series, so I didn't add v*, will add >>>> v1, and v2 change log in new version series. >>>> >>>>>> 3. If this was tested on aim300, I am surprised this being not >>>>>> enabled >>>>>> on aim300. >>>>> >>>> >>>> It was tested long times ago, but the patches wasn't sent out for >>>> reviewing early due to the team's internal schedule. >>>> >>>>> One more thing, bindings were not accepted, thus this patch should not >>>>> go in. There were no new bindings, so I assume patchset is using >>>>> rejected ones. >>>>> >>>>> It's fine to send it to get some comments, although would be nice to >>>>> mention to maintainer that this cannot be picked up as is. :( >>>>> >>>> >>>> Sure, I will resend the dtsi patch until the bindings are accepted, >>>> send >>>> this patches because you posted the comments in other series. >>>> >>>> https://lore.kernel.org/ >>>> all/0324e8e8-2ad4-4ce6-9616-3038b8e02ff9@quicinc.com/ >>>> >>>> Thanks, >>>> Depeng >>>> >>>> >>> >>> Recommend >>> >>> 1. Send out your yaml and dts in one series >>> >>> 2. Driver series can be posted in parallel >> >> The binding should go with the driver. Also usually discussion about >> driver brings comments, thus changes, to the bindings. >> >> Sorry, DTSI and DTS should wait till bindings got accepted to media >> subsystem. > > Yes you're right > > 1. Yaml - bindings > 2. dts + driver > 3. dtsi > > In this case @Depeng remember to > > 1. Link back to the older series in your cover letters > 2. Suggested by recommended - publish a complete tree somewhere and > link to that tree in your cover letters > > Its fine IMO to restart the version number of your series when breaking > up into smaller series, so long as you remember to link to the previous > series and explain in the cover letter whats going on. > Thanks for the guidance, I will follow them, and resend the DTS patch wait till bindings got accepted. Thanks, Depeng
diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi index 4c9820adcf52..58fc8792953d 100644 --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi @@ -2747,6 +2747,205 @@ videocc: clock-controller@aaf0000 { #power-domain-cells = <1>; }; + camss: camss@acb7000 { + compatible = "qcom,sm8550-camss"; + + reg = <0 0x0acb7000 0 0xd00>, + <0 0x0acb9000 0 0xd00>, + <0 0x0acbb000 0 0xd00>, + <0 0x0acca000 0 0xa00>, + <0 0x0acce000 0 0xa00>, + <0 0x0acb6000 0 0x1000>, + <0 0x0ace4000 0 0x2000>, + <0 0x0ace6000 0 0x2000>, + <0 0x0ace8000 0 0x2000>, + <0 0x0acea000 0 0x2000>, + <0 0x0acec000 0 0x2000>, + <0 0x0acee000 0 0x2000>, + <0 0x0acf0000 0 0x2000>, + <0 0x0acf2000 0 0x2000>, + <0 0x0ac62000 0 0xf000>, + <0 0x0ac71000 0 0xf000>, + <0 0x0ac80000 0 0xf000>, + <0 0x0accb000 0 0x1800>, + <0 0x0accf000 0 0x1800>; + reg-names = "csid0", + "csid1", + "csid2", + "csid_lite0", + "csid_lite1", + "csid_top", + "csiphy0", + "csiphy1", + "csiphy2", + "csiphy3", + "csiphy4", + "csiphy5", + "csiphy6", + "csiphy7", + "vfe0", + "vfe1", + "vfe2", + "vfe_lite0", + "vfe_lite1"; + + clocks = <&camcc CAM_CC_CAMNOC_AXI_CLK>, + <&camcc CAM_CC_CPAS_AHB_CLK>, + <&camcc CAM_CC_CPAS_FAST_AHB_CLK>, + <&camcc CAM_CC_CPAS_IFE_LITE_CLK>, + <&camcc CAM_CC_CPAS_IFE_0_CLK>, + <&camcc CAM_CC_CPAS_IFE_1_CLK>, + <&camcc CAM_CC_CPAS_IFE_2_CLK>, + <&camcc CAM_CC_CSID_CLK>, + <&camcc CAM_CC_CSIPHY0_CLK>, + <&camcc CAM_CC_CSI0PHYTIMER_CLK>, + <&camcc CAM_CC_CSIPHY1_CLK>, + <&camcc CAM_CC_CSI1PHYTIMER_CLK>, + <&camcc CAM_CC_CSIPHY2_CLK>, + <&camcc CAM_CC_CSI2PHYTIMER_CLK>, + <&camcc CAM_CC_CSIPHY3_CLK>, + <&camcc CAM_CC_CSI3PHYTIMER_CLK>, + <&camcc CAM_CC_CSIPHY4_CLK>, + <&camcc CAM_CC_CSI4PHYTIMER_CLK>, + <&camcc CAM_CC_CSIPHY5_CLK>, + <&camcc CAM_CC_CSI5PHYTIMER_CLK>, + <&camcc CAM_CC_CSIPHY6_CLK>, + <&camcc CAM_CC_CSI6PHYTIMER_CLK>, + <&camcc CAM_CC_CSIPHY7_CLK>, + <&camcc CAM_CC_CSI7PHYTIMER_CLK>, + <&camcc CAM_CC_CSID_CSIPHY_RX_CLK>, + <&camcc CAM_CC_IFE_0_CLK>, + <&camcc CAM_CC_IFE_0_FAST_AHB_CLK>, + <&camcc CAM_CC_IFE_1_CLK>, + <&camcc CAM_CC_IFE_1_FAST_AHB_CLK>, + <&camcc CAM_CC_IFE_2_CLK>, + <&camcc CAM_CC_IFE_2_FAST_AHB_CLK>, + <&camcc CAM_CC_IFE_LITE_CLK>, + <&camcc CAM_CC_IFE_LITE_AHB_CLK>, + <&camcc CAM_CC_IFE_LITE_CPHY_RX_CLK>, + <&camcc CAM_CC_IFE_LITE_CSID_CLK>, + <&gcc GCC_CAMERA_HF_AXI_CLK>; + + clock-names = "camnoc_axi", + "cpas_ahb", + "cpas_fast_ahb_clk", + "cpas_ife_lite", + "cpas_vfe0", + "cpas_vfe1", + "cpas_vfe2", + "csid", + "csiphy0", + "csiphy0_timer", + "csiphy1", + "csiphy1_timer", + "csiphy2", + "csiphy2_timer", + "csiphy3", + "csiphy3_timer", + "csiphy4", + "csiphy4_timer", + "csiphy5", + "csiphy5_timer", + "csiphy6", + "csiphy6_timer", + "csiphy7", + "csiphy7_timer", + "csiphy_rx", + "vfe0", + "vfe0_fast_ahb", + "vfe1", + "vfe1_fast_ahb", + "vfe2", + "vfe2_fast_ahb", + "vfe_lite", + "vfe_lite_ahb", + "vfe_lite_cphy_rx", + "vfe_lite_csid", + "gcc_axi_hf"; + + interconnects = <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_CAMERA_CFG 0>, + <&mmss_noc MASTER_CAMNOC_HF 0 &mc_virt SLAVE_EBI1 0>, + <&mmss_noc MASTER_CAMNOC_ICP 0 &mc_virt SLAVE_EBI1 0>, + <&mmss_noc MASTER_CAMNOC_SF 0 &mc_virt SLAVE_EBI1 0>; + interconnect-names = "ahb", + "hf_0_mnoc", + "icp_mnoc", + "sf_0_mnoc"; + + interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 603 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 431 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 605 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 376 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 477 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 479 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 278 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 277 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 688 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 606 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 377 IRQ_TYPE_LEVEL_HIGH>; + + interrupt-names = "csid0", + "csid1", + "csid2", + "csid_lite0", + "csid_lite1", + "csiphy0", + "csiphy1", + "csiphy2", + "csiphy3", + "csiphy4", + "csiphy5", + "csiphy6", + "csiphy7", + "vfe0", + "vfe1", + "vfe2", + "vfe_lite0", + "vfe_lite1"; + + iommus = <&apps_smmu 0x800 0x20>; + + power-domains = <&camcc CAM_CC_IFE_0_GDSC>, + <&camcc CAM_CC_IFE_1_GDSC>, + <&camcc CAM_CC_IFE_2_GDSC>, + <&camcc CAM_CC_TITAN_TOP_GDSC>; + + power-domain-names = "ife0", + "ife1", + "ife2", + "top"; + + status = "disabled"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + }; + + port@1 { + reg = <1>; + }; + + port@2 { + reg = <2>; + }; + + port@3 { + reg = <3>; + }; + }; + }; + camcc: clock-controller@ade0000 { compatible = "qcom,sm8550-camcc"; reg = <0 0x0ade0000 0 0x20000>;