[07/13] dt-bindings: media: camss: Add qcom,sm8550-camss binding
Commit Message
Add bindings for qcom,sm8550-camss in order to support the camera
subsystem for sm8550.
Co-developed-by: Yongsheng Li <quic_yon@quicinc.com>
Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
---
.../bindings/media/qcom,sm8550-camss.yaml | 517 ++++++++++++++++++
1 file changed, 517 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/qcom,sm8550-camss.yaml
Comments
On 12/08/2024 16:41, Depeng Shao wrote:
> Add bindings for qcom,sm8550-camss in order to support the camera
> subsystem for sm8550.
>
> Co-developed-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
...
> +
> +required:
> + - compatible
> + - clocks
> + - clock-names
> + - interconnects
> + - interconnect-names
> + - interrupts
> + - interrupt-names
> + - iommus
> + - power-domains
> + - power-domain-names
> + - reg
> + - reg-names
> + - vdda-phy-supply
> + - vdda-pll-supply
Order is still not as expected. I already commented on this - keep the
same order as in "properties:" block.
With the order fixed:
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
Hi Krzysztof,
On 8/16/2024 3:01 PM, Krzysztof Kozlowski wrote:
>> +required:
>> + - compatible
>> + - clocks
>> + - clock-names
>> + - interconnects
>> + - interconnect-names
>> + - interrupts
>> + - interrupt-names
>> + - iommus
>> + - power-domains
>> + - power-domain-names
>> + - reg
>> + - reg-names
>> + - vdda-phy-supply
>> + - vdda-pll-supply
>
> Order is still not as expected. I already commented on this - keep the
> same order as in "properties:" block.
>
> With the order fixed:
>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
Thanks for catching this, the order was correct in my local build, then
Vladimir posted a new comment, so I updated it again and forgot to
update the required item, I will correct the order in next version series.
Vladimir: "I would suggest to put 'compatible', 'reg' and 'reg-names'
properties as the first ones. 'clock-names' should follow 'clocks'
property in the list."
Thanks,
Depeng
Hello Depeng.
On 8/12/24 17:41, Depeng Shao wrote:
> Add bindings for qcom,sm8550-camss in order to support the camera
> subsystem for sm8550.
>
> Co-developed-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
<snip>
> +
> + 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>;
Please change all interrupt types to IRQ_TYPE_EDGE_RISING, this
will match the type set by the camss driver itself, and I believe
a rising edge interrupt here is correct.
A similar change would be needed in the dts file change.
> +
> + interrupt-names = "csid0",
> + "csid1",
> + "csid2",
> + "csid_lite0",
> + "csid_lite1",
> + "csiphy0",
> + "csiphy1",
> + "csiphy2",
> + "csiphy3",
> + "csiphy4",
> + "csiphy5",
> + "csiphy6",
> + "csiphy7",
> + "vfe0",
> + "vfe1",
> + "vfe2",
> + "vfe_lite0",
> + "vfe_lite1";
--
Best wishes,
Vladimir
Hi Vladimir,
On 9/5/2024 11:20 PM, Vladimir Zapolskiy wrote:
> Hello Depeng.
>
> On 8/12/24 17:41, Depeng Shao wrote:
>> Add bindings for qcom,sm8550-camss in order to support the camera
>> subsystem for sm8550.
>>
>> Co-developed-by: Yongsheng Li <quic_yon@quicinc.com>
>> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
>> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
>
> <snip>
>
>> +
>> + 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>;
>
> Please change all interrupt types to IRQ_TYPE_EDGE_RISING, this
> will match the type set by the camss driver itself, and I believe
> a rising edge interrupt here is correct.
>
> A similar change would be needed in the dts file change.
>
Sure, I will try this change. Thanks for the comments.
>> +
>> + interrupt-names = "csid0",
>> + "csid1",
>> + "csid2",
>> + "csid_lite0",
>> + "csid_lite1",
>> + "csiphy0",
>> + "csiphy1",
>> + "csiphy2",
>> + "csiphy3",
>> + "csiphy4",
>> + "csiphy5",
>> + "csiphy6",
>> + "csiphy7",
>> + "vfe0",
>> + "vfe1",
>> + "vfe2",
>> + "vfe_lite0",
>> + "vfe_lite1";
>
> --
> Best wishes,
> Vladimir
Thanks,
Depeng
Hi Depeng,
On 8/12/24 17:41, Depeng Shao wrote:
> Add bindings for qcom,sm8550-camss in order to support the camera
> subsystem for sm8550.
>
> Co-developed-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
there are a few more things, which I noticed.
> +
> + soc {
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + camss: camss@ace4000 {
> + 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 0x2800>,
> + <0 0x0accf000 0 0x2800>;
Please sort the list above in numerical order, this will change positions
of "vfe_lite0", "vfe_lite1" etc.
Another note, since it's not possible to map less than a page, so I believe
it might make sense to align all sizes to 0x1000.
> + 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>;
Could you please put the &gcc provided clock as the first one in the list?
> +
> + 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";
Just a note for myself, interconnect names lost "cam_" prefix, and it should
be fine.
> +
> + 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";
> +
> + vdda-phy-supply = <&vreg_l1e_0p88>;
> + vdda-pll-supply = <&vreg_l3e_1p2>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
In case of a single child node #address-cells/#size-cells could be omitted,
if I'm not mistaken about it...
> + port@0 {
> + reg = <0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
Same as above.
> +
> + csiphy_ep0: endpoint@0 {
> + reg = <0>;
> + clock-lanes = <7>;
> + data-lanes = <0 1>;
> + remote-endpoint = <&sensor_ep>;
> + };
> + };
> + };
> + };
> + };
--
Best wishes,
Vladimir
Hi Depeng,
I do have one more ask for a change.
On 8/12/24 17:41, Depeng Shao wrote:
> Add bindings for qcom,sm8550-camss in order to support the camera
> subsystem for sm8550.
>
> Co-developed-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Yongsheng Li <quic_yon@quicinc.com>
> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
<snip>
> +
> + vdda-phy-supply:
> + description:
> + Phandle to a regulator supply to PHY core block.
> +
> + vdda-pll-supply:
> + description:
> + Phandle to 1.2V regulator supply to PHY refclk pll block.
> +
Here the supplies should be split into ones, which are specific to CSI blocks,
and I believe they shall be set as optional.
The proposed names are:
vdda-phy-01-supply
vdda-pll-01-supply
vdda-phy-23-supply
vdda-pll-23-supply
vdda-phy-46-supply
vdda-pll-46-supply
vdda-phy-57-supply
vdda-pll-57-supply
I understand that what I ask is much more clumsy, and it could be seen even as
unneeded, however it'll be the right set of properties to describe the CAMSS IP
in this respect.
--
Best wishes,
Vladimir
On 12/09/2024 09:22, Vladimir Zapolskiy wrote:
>> +
>> + vdda-phy-supply:
>> + description:
>> + Phandle to a regulator supply to PHY core block.
>> +
>> + vdda-pll-supply:
>> + description:
>> + Phandle to 1.2V regulator supply to PHY refclk pll block.
>> +
>
> Here the supplies should be split into ones, which are specific to CSI
> blocks,
> and I believe they shall be set as optional.
In principle I agree with that, each CSIPHY should have its own vdda-phy
and vdda-pll regulator specified.
In practice though I don't believe its necessary, below.
> The proposed names are:
>
> vdda-phy-01-supply
> vdda-pll-01-supply
> vdda-phy-23-supply
> vdda-pll-23-supply
> vdda-phy-46-supply
> vdda-pll-46-supply
> vdda-phy-57-supply
> vdda-pll-57-supply
In principle, you're right, we need to expand the name set here.
> I understand that what I ask is much more clumsy, and it could be seen
> even as
> unneeded, however it'll be the right set of properties to describe the
> CAMSS IP
> in this respect
I think the following naming would be better as it matches the
power-grid naming in the docs.
csiphyX-vdda-phy-supply
csiphyX-vdda-pll-supply
=>
// voltage domain = vdd_a_csi_01_09 = regulator l1e
csiphy0-vdda-phy-supply = <&vreg_l1e_0p9>;
// voltage domain = vdd_a_csi_01_1p2 = regulator l3e
csiphy0-vdda-pll-supply = <&vreg_l3e_1p2>;
//
csiphy1-vdda-phy-supply = <&vreg_l1e_0p9>;
csiphy1-vdda-pll-supply = <&vreg_l3e_1p2>;
Where X indicates the CSIPHY number.
So in fact, in practice we don't need to differentiate these entries.
Checking x1e80100 ...
csiphy0
vdda-phy-supply = <&vreg_l2c_0p9>;
vdda-pll-supply = <&vreg_l1c_1p2>;
This is also the case for csiphy 1/2/4
So, I _don't_ believe this is work we need to do, since its the same
regulator for each PHY.
---
bod
Hi Bryan.
On 9/12/24 14:41, Bryan O'Donoghue wrote:
> On 12/09/2024 09:22, Vladimir Zapolskiy wrote:
>>> +
>>> + vdda-phy-supply:
>>> + description:
>>> + Phandle to a regulator supply to PHY core block.
>>> +
>>> + vdda-pll-supply:
>>> + description:
>>> + Phandle to 1.2V regulator supply to PHY refclk pll block.
>>> +
>>
>> Here the supplies should be split into ones, which are specific to CSI
>> blocks,
>> and I believe they shall be set as optional.
>
> In principle I agree with that, each CSIPHY should have its own vdda-phy
> and vdda-pll regulator specified.
>
> In practice though I don't believe its necessary, below.
>
>> The proposed names are:
>>
>> vdda-phy-01-supply
>> vdda-pll-01-supply
>> vdda-phy-23-supply
>> vdda-pll-23-supply
>> vdda-phy-46-supply
>> vdda-pll-46-supply
>> vdda-phy-57-supply
>> vdda-pll-57-supply
>
> In principle, you're right, we need to expand the name set here.
>
>> I understand that what I ask is much more clumsy, and it could be seen
>> even as
>> unneeded, however it'll be the right set of properties to describe the
>> CAMSS IP
>> in this respect
> I think the following naming would be better as it matches the
> power-grid naming in the docs.
>
> csiphyX-vdda-phy-supply
> csiphyX-vdda-pll-supply
I have no opinion about the names, any reason for name selection is
good for me.
> =>
>
> // voltage domain = vdd_a_csi_01_09 = regulator l1e
> csiphy0-vdda-phy-supply = <&vreg_l1e_0p9>;
>
> // voltage domain = vdd_a_csi_01_1p2 = regulator l3e
> csiphy0-vdda-pll-supply = <&vreg_l3e_1p2>;
>
> //
> csiphy1-vdda-phy-supply = <&vreg_l1e_0p9>;
> csiphy1-vdda-pll-supply = <&vreg_l3e_1p2>;
>
> Where X indicates the CSIPHY number.
>
> So in fact, in practice we don't need to differentiate these entries.
>
> Checking x1e80100 ...
Checking some particular board, right?
> csiphy0
>
> vdda-phy-supply = <&vreg_l2c_0p9>;
> vdda-pll-supply = <&vreg_l1c_1p2>;
>
> This is also the case for csiphy 1/2/4
>
> So, I _don't_ believe this is work we need to do, since its the same
> regulator for each PHY.
This is board specific, and even if the separation is not needed on the boards
you have just checked, still it may be needed on some boards, which are not yet
checked/not yet known.
It's needed to make the best predictions about all possible usage of hardware,
fortunately it's easy in this particular case, and it's trivial to correct it now
than on some day later on.
--
Best wishes,
Vladimir
On 12/09/2024 13:41, Bryan O'Donoghue wrote:
> On 12/09/2024 09:22, Vladimir Zapolskiy wrote:
>>> +
>>> + vdda-phy-supply:
>>> + description:
>>> + Phandle to a regulator supply to PHY core block.
>>> +
>>> + vdda-pll-supply:
>>> + description:
>>> + Phandle to 1.2V regulator supply to PHY refclk pll block.
>>> +
>>
>> Here the supplies should be split into ones, which are specific to CSI blocks,
>> and I believe they shall be set as optional.
>
> In principle I agree with that, each CSIPHY should have its own vdda-phy and vdda-pll regulator specified.
>
> In practice though I don't believe its necessary, below.
>
>> The proposed names are:
>>
>> vdda-phy-01-supply
>> vdda-pll-01-supply
>> vdda-phy-23-supply
>> vdda-pll-23-supply
>> vdda-phy-46-supply
>> vdda-pll-46-supply
>> vdda-phy-57-supply
>> vdda-pll-57-supply
>
> In principle, you're right, we need to expand the name set here.
>
>> I understand that what I ask is much more clumsy, and it could be seen even as
>> unneeded, however it'll be the right set of properties to describe the CAMSS IP
>> in this respect
> I think the following naming would be better as it matches the power-grid naming in the docs.
>
> csiphyX-vdda-phy-supply
> csiphyX-vdda-pll-supply
>
> =>
>
> // voltage domain = vdd_a_csi_01_09 = regulator l1e
> csiphy0-vdda-phy-supply = <&vreg_l1e_0p9>;
>
> // voltage domain = vdd_a_csi_01_1p2 = regulator l3e
> csiphy0-vdda-pll-supply = <&vreg_l3e_1p2>;
>
> //
> csiphy1-vdda-phy-supply = <&vreg_l1e_0p9>;
> csiphy1-vdda-pll-supply = <&vreg_l3e_1p2>;
>
> Where X indicates the CSIPHY number.
>
> So in fact, in practice we don't need to differentiate these entries.
>
> Checking x1e80100 ...
>
> csiphy0
>
> vdda-phy-supply = <&vreg_l2c_0p9>;
> vdda-pll-supply = <&vreg_l1c_1p2>;
>
> This is also the case for csiphy 1/2/4
>
> So, I _don't_ believe this is work we need to do, since its the same regulator for each PHY.
Except when it's not the case, like on the SM8650 HDK:
VDD_A_CSI_01_0P9 => VREG_L2I_0P88
VDD_A_CSI_01_1P2 => VREG_L3I_1P2
VDD_A_CSI_24_0P9 => VREG_L1I_0P88
VDD_A_CSI_24_1P2 => VREG_L3I_1P2
VDD_A_CSI_35_0P9 => VREG_L2I_0P88
VDD_A_CSI_35_1P2 => VREG_L3I_1P2
the 1P2 all uses VREG_L3I_1P2, while the 0P9 are using VREG_L2I_0P88 or VREG_L1I_0P88
Not declaring the exact supplies is pure lazyness, it will bounce back at our faces at some point.
Neil
>
> ---
> bod
On 12/09/2024 13:44, Vladimir Zapolskiy wrote:
>> csiphy0
>>
>> vdda-phy-supply = <&vreg_l2c_0p9>;
>> vdda-pll-supply = <&vreg_l1c_1p2>;
>>
>> This is also the case for csiphy 1/2/4
>>
>> So, I _don't_ believe this is work we need to do, since its the same
>> regulator for each PHY.
>
> This is board specific, and even if the separation is not needed on the
> boards
> you have just checked, still it may be needed on some boards, which are
> not yet
> checked/not yet known.
There is a Power Grid Analysis document which specifies these rails @
the SoC level and assumes you've used the Qcom PMIC to power, moreover
the PGA re-uses the same regulator over and over again.
You _could_ provide that power from your own PMIC which provides the
same voltage range as the Qcom PMIC you haven't used. Even if you did
provide that from your own PMIC you'd have to provide _separate_ rails
for the various CSIPHYs before it would be required to have a per PHY
rail requirement on this SoC.
Are people really powering these SoCs with their own PMICs ?
No probably not.
Should we add the support for it anyway ?
Maybe.
So to reiterate:
1. csiphyX-vdda-phy-supply
csiphyX-vdda-pll-supply
In the dts and yaml
=> The names should be generic from the perspective of the driver
2. camss.c::csiphy_res_sm8550
[0].regulators = { "csiphy0-vdda-phy-supply",
"csiphy0-vdda-pll-supply" }
...
[N].regulators = { "csiphyN-vdda-phy-supply",
"csiphyN-vdda-pll-supply" }
=> The regulators for the PHY should be defined in the
PHY resources description
3. Required not optional in the yaml
=> You can't use the PHY without its regulators
---
bod
Hi Bryan,
On 9/12/24 18:11, Bryan O'Donoghue wrote:
> On 12/09/2024 13:44, Vladimir Zapolskiy wrote:
>>> csiphy0
>>>
>>> vdda-phy-supply = <&vreg_l2c_0p9>;
>>> vdda-pll-supply = <&vreg_l1c_1p2>;
>>>
>>> This is also the case for csiphy 1/2/4
>>>
>>> So, I _don't_ believe this is work we need to do, since its the same
>>> regulator for each PHY.
>>
>> This is board specific, and even if the separation is not needed on the
>> boards
>> you have just checked, still it may be needed on some boards, which are
>> not yet
>> checked/not yet known.
>
> There is a Power Grid Analysis document which specifies these rails @
> the SoC level and assumes you've used the Qcom PMIC to power, moreover
> the PGA re-uses the same regulator over and over again.
>
> You _could_ provide that power from your own PMIC which provides the
> same voltage range as the Qcom PMIC you haven't used. Even if you did
> provide that from your own PMIC you'd have to provide _separate_ rails
> for the various CSIPHYs before it would be required to have a per PHY
> rail requirement on this SoC.
>
> Are people really powering these SoCs with their own PMICs ?
> No probably not.
>
> Should we add the support for it anyway ?
> Maybe.
To have a set of regulators is a matter of proper IC/IP description, actually
here I see very little option for a divergence or disagreement.
> So to reiterate:
>
> 1. csiphyX-vdda-phy-supply
> csiphyX-vdda-pll-supply
>
> In the dts and yaml
>
> => The names should be generic from the perspective of the driver
As for me I don't care about the particular names, somebody else can care.
> 2. camss.c::csiphy_res_sm8550
> [0].regulators = { "csiphy0-vdda-phy-supply",
> "csiphy0-vdda-pll-supply" }
> ...
>
> [N].regulators = { "csiphyN-vdda-phy-supply",
> "csiphyN-vdda-pll-supply" }
>
> => The regulators for the PHY should be defined in the
> PHY resources description
This is obvious.
> 3. Required not optional in the yaml
>
> => You can't use the PHY without its regulators
No, the supplies shall be optional, since it's absolutely possible to have
such a board, where supplies are merely not connected to the SoC.
Hence there shall be no requirement to describe any non-present supplies,
which is a legit case, if there is no connection and usage of the
correspondent non-supplied PHY.
--
Best wishes,
Vladimir
On 12/09/2024 21:57, Vladimir Zapolskiy wrote:
>> 3. Required not optional in the yaml
>>
>> => You can't use the PHY without its regulators
>
> No, the supplies shall be optional, since it's absolutely possible to have
> such a board, where supplies are merely not connected to the SoC.
For any _used_ PHY both supplies are certainly required.
That's what the yaml/dts check for this should achieve.
---
bod
On Thu, Sep 12, 2024 at 04:11:58PM GMT, Bryan O'Donoghue wrote:
> On 12/09/2024 13:44, Vladimir Zapolskiy wrote:
> > > csiphy0
> > >
> > > vdda-phy-supply = <&vreg_l2c_0p9>;
> > > vdda-pll-supply = <&vreg_l1c_1p2>;
> > >
> > > This is also the case for csiphy 1/2/4
> > >
> > > So, I _don't_ believe this is work we need to do, since its the same
> > > regulator for each PHY.
> >
> > This is board specific, and even if the separation is not needed on the
> > boards
> > you have just checked, still it may be needed on some boards, which are
> > not yet
> > checked/not yet known.
>
> There is a Power Grid Analysis document which specifies these rails @ the
> SoC level and assumes you've used the Qcom PMIC to power, moreover the PGA
> re-uses the same regulator over and over again.
>
> You _could_ provide that power from your own PMIC which provides the same
> voltage range as the Qcom PMIC you haven't used. Even if you did provide
> that from your own PMIC you'd have to provide _separate_ rails for the
> various CSIPHYs before it would be required to have a per PHY rail
> requirement on this SoC.
>
> Are people really powering these SoCs with their own PMICs ?
> No probably not.
Yes, they are.
On 9/13/24 01:41, Bryan O'Donoghue wrote:
> On 12/09/2024 21:57, Vladimir Zapolskiy wrote:
>>> 3. Required not optional in the yaml
>>>
>>> => You can't use the PHY without its regulators
>>
>> No, the supplies shall be optional, since it's absolutely possible to have
>> such a board, where supplies are merely not connected to the SoC.
>
> For any _used_ PHY both supplies are certainly required.
>
> That's what the yaml/dts check for this should achieve.
I believe it is technically possible by writing an enormously complex
scheme, when all possible "port" cases and combinations are listed.
Do you see any simpler way? Do you insist that it is utterly needed?
In any case, there are optional and required device tree properties,
the CAMSS supplies shall be split into multiple ones and become optional.
That's exactly the point of my first message in the discussion, so far
nothing has been added or changed.
--
Best wishes,
Vladimir
On 13/09/2024 06:06, Vladimir Zapolskiy wrote:
> On 9/13/24 01:41, Bryan O'Donoghue wrote:
>> On 12/09/2024 21:57, Vladimir Zapolskiy wrote:
>>>> 3. Required not optional in the yaml
>>>>
>>>> => You can't use the PHY without its regulators
>>>
>>> No, the supplies shall be optional, since it's absolutely possible to
>>> have
>>> such a board, where supplies are merely not connected to the SoC.
>>
>> For any _used_ PHY both supplies are certainly required.
>>
>> That's what the yaml/dts check for this should achieve.
>
> I believe it is technically possible by writing an enormously complex
> scheme, when all possible "port" cases and combinations are listed.
>
> Do you see any simpler way? Do you insist that it is utterly needed?
I asked Krzysztof about this offline.
He said something like
Quote:
This is possible, but I think not between child nodes.
https://elixir.bootlin.com/linux/v6.11-rc7/source/Documentation/devicetree/bindings/example-schema.yaml#L194
You could require something in children, but not in parent node. For
children something around:
https://elixir.bootlin.com/linux/v6.4-rc7/source/Documentation/devicetree/bindings/net/qcom,ipa.yaml#L174
allOf:
- if:
required:
- something-in-parent
then:
properties:
child-node:
required:
- something-in-child
I will see if I can turn that into a workable proposal/patch.
---
bod
Hi Bryan,
On 9/18/24 01:40, Bryan O'Donoghue wrote:
> On 13/09/2024 06:06, Vladimir Zapolskiy wrote:
>> On 9/13/24 01:41, Bryan O'Donoghue wrote:
>>> On 12/09/2024 21:57, Vladimir Zapolskiy wrote:
>>>>> 3. Required not optional in the yaml
>>>>>
>>>>> => You can't use the PHY without its regulators
>>>>
>>>> No, the supplies shall be optional, since it's absolutely possible to
>>>> have
>>>> such a board, where supplies are merely not connected to the SoC.
>>>
>>> For any _used_ PHY both supplies are certainly required.
>>>
>>> That's what the yaml/dts check for this should achieve.
>>
>> I believe it is technically possible by writing an enormously complex
>> scheme, when all possible "port" cases and combinations are listed.
>>
>> Do you see any simpler way? Do you insist that it is utterly needed?
>
> I asked Krzysztof about this offline.
>
> He said something like
>
> Quote:
> This is possible, but I think not between child nodes.
> https://elixir.bootlin.com/linux/v6.11-rc7/source/Documentation/devicetree/bindings/example-schema.yaml#L194
>
> You could require something in children, but not in parent node. For
> children something around:
> https://elixir.bootlin.com/linux/v6.4-rc7/source/Documentation/devicetree/bindings/net/qcom,ipa.yaml#L174
>
> allOf:
> - if:
> required:
> - something-in-parent
> then:
> properties:
> child-node:
> required:
> - something-in-child
>
> I will see if I can turn that into a workable proposal/patch.
>
thank you for pushing my review request forward.
Overall I believe making supply properties as optional ones is sufficient,
technically straightforward and merely good enough, thus please let me
ask you to ponder on this particular variant one more time.
--
Best wishes,
Vladimir
Hi Vladimir,
On 9/6/2024 11:56 PM, Vladimir Zapolskiy wrote:
>> + 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 0x2800>,
>> + <0 0x0accf000 0 0x2800>;
>
> Please sort the list above in numerical order, this will change positions
> of "vfe_lite0", "vfe_lite1" etc.
>
> Another note, since it's not possible to map less than a page, so I believe
> it might make sense to align all sizes to 0x1000.
>
Sure, I previously sorted by the alphabetical order of reg_name.
I will update it based on your suggestion. And will also make sure the
align all sizes to 0x1000.
>> + <&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>;
>
> Could you please put the &gcc provided clock as the first one in the list?
>
Sure, will do.
>> +
>> + 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";
>
> Just a note for myself, interconnect names lost "cam_" prefix, and it
> should
> be fine.
Krzysztof posted a comment in a SC7280 camss change and asked to remove
the "cam_" prefix.
https://lore.kernel.org/all/087e7f29-1fa8-4bc2-bb3d-acb941432381@kernel.org/
>> + ports {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>
> In case of a single child node #address-cells/#size-cells could be omitted,
> if I'm not mistaken about it...
>
I tried it, but dt_binding_check report below warning.
Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dts:221.29-39:
Warning (reg_format):
/example-0/soc/camss@ace4000/ports/port@0/endpoint@0:reg: property has
invalid length (4 bytes) (#address-cells == 2, #size-cells == 1)
Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dtb:
Warning (pci_device_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dtb:
Warning (pci_device_bus_num): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dtb:
Warning (simple_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dtb:
Warning (i2c_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dtb:
Warning (spi_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dts:220.48-225.27:
Warning (avoid_default_addr_size):
/example-0/soc/camss@ace4000/ports/port@0/endpoint@0: Relying on default
#address-cells value
Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dts:220.48-225.27:
Warning (avoid_default_addr_size):
/example-0/soc/camss@ace4000/ports/port@0/endpoint@0: Relying on default
#size-cells value
Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dtb:
Warning (unique_unit_address_if_enabled): Failed prerequisite
'avoid_default_addr_size'
Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dts:220.48-225.27:
Warning (graph_endpoint):
/example-0/soc/camss@ace4000/ports/port@0/endpoint@0: graph node
'#address-cells' is -1, must be 1
Documentation/devicetree/bindings/media/qcom,sm8550-camss.example.dts:220.48-225.27:
Warning (graph_endpoint):
/example-0/soc/camss@ace4000/ports/port@0/endpoint@0: graph node
'#size-cells' is -1, must be 0
>> + port@0 {
>> + reg = <0>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>
> Same as above.
>
Same warning..
>> +
>> + csiphy_ep0: endpoint@0 {
>> + reg = <0>;
>> + clock-lanes = <7>;
>> + data-lanes = <0 1>;
>> + remote-endpoint = <&sensor_ep>;
>> + };
>> + };
>> + };
>> + };
>> + };
>
---
Thanks,
Depeng
Hi Vladimir, Bryan,
On 9/18/2024 7:16 AM, Vladimir Zapolskiy wrote:
> Hi Bryan,
>
> On 9/18/24 01:40, Bryan O'Donoghue wrote:
>> On 13/09/2024 06:06, Vladimir Zapolskiy wrote:
>>> On 9/13/24 01:41, Bryan O'Donoghue wrote:
>>>> On 12/09/2024 21:57, Vladimir Zapolskiy wrote:
>>>>>> 3. Required not optional in the yaml
>>>>>>
>>>>>> => You can't use the PHY without its regulators
>>>>>
>>>>> No, the supplies shall be optional, since it's absolutely possible to
>>>>> have
>>>>> such a board, where supplies are merely not connected to the SoC.
>>>>
>>>> For any _used_ PHY both supplies are certainly required.
>>>>
>>>> That's what the yaml/dts check for this should achieve.
>>>
>>> I believe it is technically possible by writing an enormously complex
>>> scheme, when all possible "port" cases and combinations are listed.
>>>
>>> Do you see any simpler way? Do you insist that it is utterly needed?
>>
>> I asked Krzysztof about this offline.
>>
>> He said something like
>>
>> Quote:
>> This is possible, but I think not between child nodes.
>> https://elixir.bootlin.com/linux/v6.11-rc7/source/Documentation/
>> devicetree/bindings/example-schema.yaml#L194
>>
>> You could require something in children, but not in parent node. For
>> children something around:
>> https://elixir.bootlin.com/linux/v6.4-rc7/source/Documentation/
>> devicetree/bindings/net/qcom,ipa.yaml#L174
>>
>> allOf:
>> - if:
>> required:
>> - something-in-parent
>> then:
>> properties:
>> child-node:
>> required:
>> - something-in-child
>>
>> I will see if I can turn that into a workable proposal/patch.
>>
>
> thank you for pushing my review request forward.
>
> Overall I believe making supply properties as optional ones is sufficient,
> technically straightforward and merely good enough, thus please let me
> ask you to ponder on this particular variant one more time.
>
So, we are discussing two things.
1# Use separate supplies for each CSI block, looks like there is no
doubt about it anymore. So, I will update it just like based on suggestion.
csiphyX-vdda-phy-supply
csiphyX-vdda-pll-supply
Then I will need below items in the required list if they are required.
required:
- csiphy0-vdda-phy-supply
- csiphy0-vdda-pll-supply
- csiphy1-vdda-phy-supply
- csiphy1-vdda-pll-supply
...
- csiphy7-vdda-phy-supply
- csiphy7-vdda-pll-supply
2# Regarding the CSI supplies, if they need to be making as optional?
Looks like there is no conclusion now.
@Bryan, do you agree with this?
Thanks,
Depeng
On 25/09/2024 17:13, Depeng Shao wrote:
> Hi Vladimir,
>
> On 9/6/2024 11:56 PM, Vladimir Zapolskiy wrote:
>
>>> + 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 0x2800>,
>>> + <0 0x0accf000 0 0x2800>;
>>
>> Please sort the list above in numerical order, this will change positions
>> of "vfe_lite0", "vfe_lite1" etc.
>>
>> Another note, since it's not possible to map less than a page, so I believe
>> it might make sense to align all sizes to 0x1000.
>>
>
> Sure, I previously sorted by the alphabetical order of reg_name.
> I will update it based on your suggestion. And will also make sure the
> align all sizes to 0x1000.
If I understood correctly, you want to change the order from existing
devices, so no. You are supposed to keep the same order, as much as
possible.
Best regards,
Krzysztof
On 16/08/2024 09:45, Depeng Shao wrote:
> Hi Krzysztof,
>
> On 8/16/2024 3:01 PM, Krzysztof Kozlowski wrote:
>
>>> +required:
>>> + - compatible
>>> + - clocks
>>> + - clock-names
>>> + - interconnects
>>> + - interconnect-names
>>> + - interrupts
>>> + - interrupt-names
>>> + - iommus
>>> + - power-domains
>>> + - power-domain-names
>>> + - reg
>>> + - reg-names
>>> + - vdda-phy-supply
>>> + - vdda-pll-supply
>>
>> Order is still not as expected. I already commented on this - keep the
>> same order as in "properties:" block.
>>
>> With the order fixed:
>>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
The review tag was given to above code with above changes. If you are
going to implement some more changes, including changing of orders of
some lists or adding ports, then drop this tag and explicitly mention in
patch changelog that tag was not added because of something.
Best regards,
Krzysztof
On 25/09/2024 17:13, Depeng Shao wrote:
> Hi Vladimir,
>
> On 9/6/2024 11:56 PM, Vladimir Zapolskiy wrote:
>
>>> + 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 0x2800>,
>>> + <0 0x0accf000 0 0x2800>;
>>
>> Please sort the list above in numerical order, this will change positions
>> of "vfe_lite0", "vfe_lite1" etc.
>>
>> Another note, since it's not possible to map less than a page, so I believe
>> it might make sense to align all sizes to 0x1000.
And if Linux behavior changes then are you going to rewrite all the DTS
for new size?
No, the sizes reflect hardware register layout, not concept of pages.
I don't think that we should be coming with more nitpicky ideas, one
month after the patch was sent and reviewed.
Best regards,
Krzysztof
On 9/30/24 10:26, Krzysztof Kozlowski wrote:
> On 25/09/2024 17:13, Depeng Shao wrote:
>> Hi Vladimir,
>>
>> On 9/6/2024 11:56 PM, Vladimir Zapolskiy wrote:
>>
>>>> + 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 0x2800>,
>>>> + <0 0x0accf000 0 0x2800>;
>>>
>>> Please sort the list above in numerical order, this will change positions
>>> of "vfe_lite0", "vfe_lite1" etc.
>>>
>>> Another note, since it's not possible to map less than a page, so I believe
>>> it might make sense to align all sizes to 0x1000.
>
> And if Linux behavior changes then are you going to rewrite all the DTS
> for new size?
If Linux behaves properly with page size alignments today, then the selected
page size alignment for AMBA device IO memory regions is correct, hence any
future change from the correct IP device description to another one will be
invalid or noop.
There is nothing to worry about, I believe.
> No, the sizes reflect hardware register layout, not concept of pages.
>
Absolutely they do. It might be a coincidence that both are aligned in
this particular case or another one.
> I don't think that we should be coming with more nitpicky ideas, one
> month after the patch was sent and reviewed.
The change is not yet ready to be accepted from the technical perspective.
--
Best wishes,
Vladimir
Hello Krzysztof,
On 9/30/24 10:16, Krzysztof Kozlowski wrote:
> On 25/09/2024 17:13, Depeng Shao wrote:
>> Hi Vladimir,
>>
>> On 9/6/2024 11:56 PM, Vladimir Zapolskiy wrote:
>>
>>>> + 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 0x2800>,
>>>> + <0 0x0accf000 0 0x2800>;
>>>
>>> Please sort the list above in numerical order, this will change positions
>>> of "vfe_lite0", "vfe_lite1" etc.
>>>
>>> Another note, since it's not possible to map less than a page, so I believe
>>> it might make sense to align all sizes to 0x1000.
>>>
>>
>> Sure, I previously sorted by the alphabetical order of reg_name.
>> I will update it based on your suggestion. And will also make sure the
>> align all sizes to 0x1000.
>
> If I understood correctly, you want to change the order from existing
> devices, so no. You are supposed to keep the same order, as much as
> possible.
Please elaborate, what do you mean here by the "existing evices"?
The list is not sorted by reg values, I ask to sort the list by reg values.
--
Best wishes,
Vladimir
On 30/09/2024 09:46, Vladimir Zapolskiy wrote:
> Hello Krzysztof,
>
> On 9/30/24 10:16, Krzysztof Kozlowski wrote:
>> On 25/09/2024 17:13, Depeng Shao wrote:
>>> Hi Vladimir,
>>>
>>> On 9/6/2024 11:56 PM, Vladimir Zapolskiy wrote:
>>>
>>>>> + 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 0x2800>,
>>>>> + <0 0x0accf000 0 0x2800>;
>>>>
>>>> Please sort the list above in numerical order, this will change
>>>> positions
>>>> of "vfe_lite0", "vfe_lite1" etc.
>>>>
>>>> Another note, since it's not possible to map less than a page, so I
>>>> believe
>>>> it might make sense to align all sizes to 0x1000.
>>>>
>>>
>>> Sure, I previously sorted by the alphabetical order of reg_name.
>>> I will update it based on your suggestion. And will also make sure the
>>> align all sizes to 0x1000.
>>
>> If I understood correctly, you want to change the order from existing
>> devices, so no. You are supposed to keep the same order, as much as
>> possible.
>
> Please elaborate, what do you mean here by the "existing evices"?
>
> The list is not sorted by reg values, I ask to sort the list by reg values.
>
> --
> Best wishes,
> Vladimir
We always sort by address:
camss: camss@ac5a000 {
compatible = "qcom,sc8280xp-camss";
reg = <0 0x0ac5a000 0 0x2000>,
<0 0x0ac5c000 0 0x2000>,
<0 0x0ac65000 0 0x2000>,
<0 0x0ac67000 0 0x2000>,
<0 0x0acaf000 0 0x4000>,
<0 0x0acb3000 0 0x1000>,
<0 0x0acb6000 0 0x4000>,
<0 0x0acba000 0 0x1000>,
<0 0x0acbd000 0 0x4000>,
<0 0x0acc1000 0 0x1000>,
<0 0x0acc4000 0 0x4000>,
<0 0x0acc8000 0 0x1000>,
<0 0x0accb000 0 0x4000>,
<0 0x0accf000 0 0x1000>,
<0 0x0acd2000 0 0x4000>,
<0 0x0acd6000 0 0x1000>,
<0 0x0acd9000 0 0x4000>,
<0 0x0acdd000 0 0x1000>,
<0 0x0ace0000 0 0x4000>,
<0 0x0ace4000 0 0x1000>;
reg-names = "csiphy2",
"csiphy3",
"csiphy0",
"csiphy1",
"vfe0",
"csid0",
"vfe1",
"csid1",
"vfe2",
"csid2",
"vfe_lite0",
"csid0_lite",
"vfe_lite1",
"csid1_lite",
"vfe_lite2",
"csid2_lite",
"vfe_lite3",
"csid3_lite",
"vfe3",
"csid3";
This is the way.
---
bod
On 30/09/2024 08:26, Krzysztof Kozlowski wrote:
>>> Please sort the list above in numerical order, this will change positions
>>> of "vfe_lite0", "vfe_lite1" etc.
>>>
>>> Another note, since it's not possible to map less than a page, so I believe
>>> it might make sense to align all sizes to 0x1000.
> And if Linux behavior changes then are you going to rewrite all the DTS
> for new size?
>
> No, the sizes reflect hardware register layout, not concept of pages.
>
> I don't think that we should be coming with more nitpicky ideas, one
> month after the patch was sent and reviewed.
Agree.
1. My understanding has always been:
- Map the entire register bank extent
- The main reason for that is today you might only use
1/4 of the registers in a given bank but tomorrow you might
add in new functionality - like the HardISP in which case
you'd want the full set of registers not just the 1/4
or the 4k aligned version of that bank.
2. Pages can be all sorts of sizes so aligning to a page
makes no sense. 4k isn't special.
https://en.wikipedia.org/wiki/Page_(computer_memory)#Multiple_page_sizes
---
bod
On 9/30/24 11:55, Bryan O'Donoghue wrote:
> On 30/09/2024 09:46, Vladimir Zapolskiy wrote:
>> Hello Krzysztof,
>>
>> On 9/30/24 10:16, Krzysztof Kozlowski wrote:
>>> On 25/09/2024 17:13, Depeng Shao wrote:
>>>> Hi Vladimir,
>>>>
>>>> On 9/6/2024 11:56 PM, Vladimir Zapolskiy wrote:
>>>>
>>>>>> + 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 0x2800>,
>>>>>> + <0 0x0accf000 0 0x2800>;
>>>>>
>>>>> Please sort the list above in numerical order, this will change
>>>>> positions
>>>>> of "vfe_lite0", "vfe_lite1" etc.
>>>>>
>>>>> Another note, since it's not possible to map less than a page, so I
>>>>> believe
>>>>> it might make sense to align all sizes to 0x1000.
>>>>>
>>>>
>>>> Sure, I previously sorted by the alphabetical order of reg_name.
>>>> I will update it based on your suggestion. And will also make sure the
>>>> align all sizes to 0x1000.
>>>
>>> If I understood correctly, you want to change the order from existing
>>> devices, so no. You are supposed to keep the same order, as much as
>>> possible.
>>
>> Please elaborate, what do you mean here by the "existing evices"?
>>
>> The list is not sorted by reg values, I ask to sort the list by reg values.
>>
>> --
>> Best wishes,
>> Vladimir
>
> We always sort by address:
>
Thank you for the given confirmation that there is a need to make
the change requested by me.
--
Best wishes,
Vladimir
Hi Bryan,
On 9/25/2024 11:40 PM, Depeng Shao wrote:
> Hi Vladimir, Bryan,
>
> On 9/18/2024 7:16 AM, Vladimir Zapolskiy wrote:
>> Hi Bryan,
>>
>> On 9/18/24 01:40, Bryan O'Donoghue wrote:
>>> On 13/09/2024 06:06, Vladimir Zapolskiy wrote:
>>>> On 9/13/24 01:41, Bryan O'Donoghue wrote:
>>>>> On 12/09/2024 21:57, Vladimir Zapolskiy wrote:
>>>>>>> 3. Required not optional in the yaml
>>>>>>>
>>>>>>> => You can't use the PHY without its regulators
>>>>>>
>>>>>> No, the supplies shall be optional, since it's absolutely possible to
>>>>>> have
>>>>>> such a board, where supplies are merely not connected to the SoC.
>>>>>
>>>>> For any _used_ PHY both supplies are certainly required.
>>>>>
>>>>> That's what the yaml/dts check for this should achieve.
>>>>
>>>> I believe it is technically possible by writing an enormously complex
>>>> scheme, when all possible "port" cases and combinations are listed.
>>>>
>>>> Do you see any simpler way? Do you insist that it is utterly needed?
>>>
>>> I asked Krzysztof about this offline.
>>>
>>> He said something like
>>>
>>> Quote:
>>> This is possible, but I think not between child nodes.
>>> https://elixir.bootlin.com/linux/v6.11-rc7/source/Documentation/
>>> devicetree/bindings/example-schema.yaml#L194
>>>
>>> You could require something in children, but not in parent node. For
>>> children something around:
>>> https://elixir.bootlin.com/linux/v6.4-rc7/source/Documentation/
>>> devicetree/bindings/net/qcom,ipa.yaml#L174
>>>
>>> allOf:
>>> - if:
>>> required:
>>> - something-in-parent
>>> then:
>>> properties:
>>> child-node:
>>> required:
>>> - something-in-child
>>>
>>> I will see if I can turn that into a workable proposal/patch.
>>>
>>
>> thank you for pushing my review request forward.
>>
>> Overall I believe making supply properties as optional ones is
>> sufficient,
>> technically straightforward and merely good enough, thus please let me
>> ask you to ponder on this particular variant one more time.
>>
>
> So, we are discussing two things.
>
> 1# Use separate supplies for each CSI block, looks like there is no
> doubt about it anymore. So, I will update it just like based on suggestion.
>
> csiphyX-vdda-phy-supply
> csiphyX-vdda-pll-supply
>
> Then I will need below items in the required list if they are required.
> required:
> - csiphy0-vdda-phy-supply
> - csiphy0-vdda-pll-supply
> - csiphy1-vdda-phy-supply
> - csiphy1-vdda-pll-supply
> ...
> - csiphy7-vdda-phy-supply
> - csiphy7-vdda-pll-supply
>
> 2# Regarding the CSI supplies, if they need to be making as optional?
> Looks like there is no conclusion now.
>
> @Bryan, do you agree with this?
>
I'm preparing the new version patches, and will send out for reviewing
in few days. I will follow Vladimir's comments if you have no response,
it means making supply properties as optional one, so they won't be
added to the required list.
Thanks,
Depeng
On 25/09/2024 16:40, Depeng Shao wrote:
>
> 2# Regarding the CSI supplies, if they need to be making as optional?
> Looks like there is no conclusion now.
>
> @Bryan, do you agree with this?
It doesn't make sense to have those supplies optional. If you
instantiate a csiphy for your board you need a power supply for it.
I believe I said I would _try_ to come up with a proposal for that. I
should be able to get x1e80100 first pass patches out this week -
including such a proposed fix.
---
bod
Hi Depeng.
On 9/30/24 12:26, Depeng Shao wrote:
> Hi Bryan,
>
> On 9/25/2024 11:40 PM, Depeng Shao wrote:
>> Hi Vladimir, Bryan,
>>
>> On 9/18/2024 7:16 AM, Vladimir Zapolskiy wrote:
>>> Hi Bryan,
>>>
>>> On 9/18/24 01:40, Bryan O'Donoghue wrote:
>>>> On 13/09/2024 06:06, Vladimir Zapolskiy wrote:
>>>>> On 9/13/24 01:41, Bryan O'Donoghue wrote:
>>>>>> On 12/09/2024 21:57, Vladimir Zapolskiy wrote:
>>>>>>>> 3. Required not optional in the yaml
>>>>>>>>
>>>>>>>> => You can't use the PHY without its regulators
>>>>>>>
>>>>>>> No, the supplies shall be optional, since it's absolutely possible to
>>>>>>> have
>>>>>>> such a board, where supplies are merely not connected to the SoC.
>>>>>>
>>>>>> For any _used_ PHY both supplies are certainly required.
>>>>>>
>>>>>> That's what the yaml/dts check for this should achieve.
>>>>>
>>>>> I believe it is technically possible by writing an enormously complex
>>>>> scheme, when all possible "port" cases and combinations are listed.
>>>>>
>>>>> Do you see any simpler way? Do you insist that it is utterly needed?
>>>>
>>>> I asked Krzysztof about this offline.
>>>>
>>>> He said something like
>>>>
>>>> Quote:
>>>> This is possible, but I think not between child nodes.
>>>> https://elixir.bootlin.com/linux/v6.11-rc7/source/Documentation/
>>>> devicetree/bindings/example-schema.yaml#L194
>>>>
>>>> You could require something in children, but not in parent node. For
>>>> children something around:
>>>> https://elixir.bootlin.com/linux/v6.4-rc7/source/Documentation/
>>>> devicetree/bindings/net/qcom,ipa.yaml#L174
>>>>
>>>> allOf:
>>>> - if:
>>>> required:
>>>> - something-in-parent
>>>> then:
>>>> properties:
>>>> child-node:
>>>> required:
>>>> - something-in-child
>>>>
>>>> I will see if I can turn that into a workable proposal/patch.
>>>>
>>>
>>> thank you for pushing my review request forward.
>>>
>>> Overall I believe making supply properties as optional ones is
>>> sufficient,
>>> technically straightforward and merely good enough, thus please let me
>>> ask you to ponder on this particular variant one more time.
>>>
>>
>> So, we are discussing two things.
>>
>> 1# Use separate supplies for each CSI block, looks like there is no
>> doubt about it anymore. So, I will update it just like based on suggestion.
>>
>> csiphyX-vdda-phy-supply
>> csiphyX-vdda-pll-supply
>>
>> Then I will need below items in the required list if they are required.
>> required:
>> - csiphy0-vdda-phy-supply
>> - csiphy0-vdda-pll-supply
>> - csiphy1-vdda-phy-supply
>> - csiphy1-vdda-pll-supply
>> ...
>> - csiphy7-vdda-phy-supply
>> - csiphy7-vdda-pll-supply
>>
>> 2# Regarding the CSI supplies, if they need to be making as optional?
>> Looks like there is no conclusion now.
>>
>> @Bryan, do you agree with this?
>>
>
> I'm preparing the new version patches, and will send out for reviewing
> in few days. I will follow Vladimir's comments if you have no response,
> it means making supply properties as optional one, so they won't be
> added to the required list.
>
Recently I published the change, which moves regulator supplies from CSID
to CSIPHY, I believe it makes sense to base the SM8550 change and regulators
under discussion on top of the series:
https://lore.kernel.org/all/20240926211957.4108692-1-vladimir.zapolskiy@linaro.org/
Note, that SM8250 regulators are not changed, however their names are wrong,
the correction shall be a separate change later on...
Next, I developed my opinion regarding the supply regulator property names:
1) voltage supply regulator property names match the pattern "*v*-supply",
and the most common name is "vdd*-supply", the match to the pattern shall
be preserved,
2) also it would be much better and it will exclude any confusion, if SoC pin
names are put into the name, like it is done in a multitude of similar
cases.
So, in my opinion for SM8550 CAMSS a proposed set of voltage supply regulator
names should be this one:
- vdda-csi01-0p9-supply
- vdda-csi01-1p2-supply
- vdda-csi23-0p9-supply
- vdda-csi23-1p2-supply
- vdda-csi46-0p9-supply
- vdda-csi46-1p2-supply
- vdda-csi57-0p9-supply
- vdda-csi57-1p2-supply
Comments, corrections and objections are always welcome.
--
Best wishes,
Vladimir
On 08/10/2024 14:50, Vladimir Zapolskiy wrote:
> Hi Depeng.
>
> On 9/30/24 12:26, Depeng Shao wrote:
>> Hi Bryan,
>>
>> On 9/25/2024 11:40 PM, Depeng Shao wrote:
>>> Hi Vladimir, Bryan,
>>>
>>> On 9/18/2024 7:16 AM, Vladimir Zapolskiy wrote:
>>>> Hi Bryan,
>>>>
>>>> On 9/18/24 01:40, Bryan O'Donoghue wrote:
>>>>> On 13/09/2024 06:06, Vladimir Zapolskiy wrote:
>>>>>> On 9/13/24 01:41, Bryan O'Donoghue wrote:
>>>>>>> On 12/09/2024 21:57, Vladimir Zapolskiy wrote:
>>>>>>>>> 3. Required not optional in the yaml
>>>>>>>>>
>>>>>>>>> => You can't use the PHY without its regulators
>>>>>>>>
>>>>>>>> No, the supplies shall be optional, since it's absolutely
>>>>>>>> possible to
>>>>>>>> have
>>>>>>>> such a board, where supplies are merely not connected to the SoC.
>>>>>>>
>>>>>>> For any _used_ PHY both supplies are certainly required.
>>>>>>>
>>>>>>> That's what the yaml/dts check for this should achieve.
>>>>>>
>>>>>> I believe it is technically possible by writing an enormously complex
>>>>>> scheme, when all possible "port" cases and combinations are listed.
>>>>>>
>>>>>> Do you see any simpler way? Do you insist that it is utterly needed?
>>>>>
>>>>> I asked Krzysztof about this offline.
>>>>>
>>>>> He said something like
>>>>>
>>>>> Quote:
>>>>> This is possible, but I think not between child nodes.
>>>>> https://elixir.bootlin.com/linux/v6.11-rc7/source/Documentation/
>>>>> devicetree/bindings/example-schema.yaml#L194
>>>>>
>>>>> You could require something in children, but not in parent node. For
>>>>> children something around:
>>>>> https://elixir.bootlin.com/linux/v6.4-rc7/source/Documentation/
>>>>> devicetree/bindings/net/qcom,ipa.yaml#L174
>>>>>
>>>>> allOf:
>>>>> - if:
>>>>> required:
>>>>> - something-in-parent
>>>>> then:
>>>>> properties:
>>>>> child-node:
>>>>> required:
>>>>> - something-in-child
>>>>>
>>>>> I will see if I can turn that into a workable proposal/patch.
>>>>>
>>>>
>>>> thank you for pushing my review request forward.
>>>>
>>>> Overall I believe making supply properties as optional ones is
>>>> sufficient,
>>>> technically straightforward and merely good enough, thus please let me
>>>> ask you to ponder on this particular variant one more time.
>>>>
>>>
>>> So, we are discussing two things.
>>>
>>> 1# Use separate supplies for each CSI block, looks like there is no
>>> doubt about it anymore. So, I will update it just like based on
>>> suggestion.
>>>
>>> csiphyX-vdda-phy-supply
>>> csiphyX-vdda-pll-supply
>>>
>>> Then I will need below items in the required list if they are required.
>>> required:
>>> - csiphy0-vdda-phy-supply
>>> - csiphy0-vdda-pll-supply
>>> - csiphy1-vdda-phy-supply
>>> - csiphy1-vdda-pll-supply
>>> ...
>>> - csiphy7-vdda-phy-supply
>>> - csiphy7-vdda-pll-supply
>>>
>>> 2# Regarding the CSI supplies, if they need to be making as optional?
>>> Looks like there is no conclusion now.
>>>
>>> @Bryan, do you agree with this?
>>>
>>
>> I'm preparing the new version patches, and will send out for reviewing
>> in few days. I will follow Vladimir's comments if you have no response,
>> it means making supply properties as optional one, so they won't be
>> added to the required list.
>>
>
> Recently I published the change, which moves regulator supplies from CSID
> to CSIPHY, I believe it makes sense to base the SM8550 change and
> regulators
> under discussion on top of the series:
>
> https://lore.kernel.org/all/20240926211957.4108692-1-
> vladimir.zapolskiy@linaro.org/
>
> Note, that SM8250 regulators are not changed, however their names are
> wrong,
> the correction shall be a separate change later on...
>
> Next, I developed my opinion regarding the supply regulator property names:
>
> 1) voltage supply regulator property names match the pattern "*v*-supply",
> and the most common name is "vdd*-supply", the match to the pattern
> shall
> be preserved,
> 2) also it would be much better and it will exclude any confusion, if
> SoC pin
> names are put into the name, like it is done in a multitude of similar
> cases.
>
> So, in my opinion for SM8550 CAMSS a proposed set of voltage supply
> regulator
> names should be this one:
>
> - vdda-csi01-0p9-supply
> - vdda-csi01-1p2-supply
> - vdda-csi23-0p9-supply
> - vdda-csi23-1p2-supply
> - vdda-csi46-0p9-supply
> - vdda-csi46-1p2-supply
> - vdda-csi57-0p9-supply
> - vdda-csi57-1p2-supply
So I communicated to Depeng to take the patch for the regulators but, I
still don't think the above is the right way to do this.
I will take a pass at constructing something in the schema to capture
the case where a regulator is required if and only if it is instantiated.
May not be possible with our current syntax/tools but is 100% how the
hardware works so IMO is the right thing to try to do.
---
bod
Hi Vladimir,
On 10/8/2024 10:06 PM, Bryan O'Donoghue wrote:
> On 08/10/2024 14:50, Vladimir Zapolskiy wrote:
>> Hi Depeng.
>>
>> On 9/30/24 12:26, Depeng Shao wrote:
>>> Hi Bryan,
>>>
>>> On 9/25/2024 11:40 PM, Depeng Shao wrote:
>>>> Hi Vladimir, Bryan,
>>>>
>>>> On 9/18/2024 7:16 AM, Vladimir Zapolskiy wrote:
>>>>> Hi Bryan,
>>>>>
>>>>> On 9/18/24 01:40, Bryan O'Donoghue wrote:
>>>>>> On 13/09/2024 06:06, Vladimir Zapolskiy wrote:
>>>>>>> On 9/13/24 01:41, Bryan O'Donoghue wrote:
>>>>>>>> On 12/09/2024 21:57, Vladimir Zapolskiy wrote:
>>>>>>>>>> 3. Required not optional in the yaml
>>>>>>>>>>
>>>>>>>>>> => You can't use the PHY without its regulators
>>>>>>>>>
>>>>>>>>> No, the supplies shall be optional, since it's absolutely
>>>>>>>>> possible to
>>>>>>>>> have
>>>>>>>>> such a board, where supplies are merely not connected to the SoC.
>>>>>>>>
>>>>>>>> For any _used_ PHY both supplies are certainly required.
>>>>>>>>
>>>>>>>> That's what the yaml/dts check for this should achieve.
>>>>>>>
>>>>>>> I believe it is technically possible by writing an enormously
>>>>>>> complex
>>>>>>> scheme, when all possible "port" cases and combinations are listed.
>>>>>>>
>>>>>>> Do you see any simpler way? Do you insist that it is utterly needed?
>>>>>>
>>>>>> I asked Krzysztof about this offline.
>>>>>>
>>>>>> He said something like
>>>>>>
>>>>>> Quote:
>>>>>> This is possible, but I think not between child nodes.
>>>>>> https://elixir.bootlin.com/linux/v6.11-rc7/source/Documentation/
>>>>>> devicetree/bindings/example-schema.yaml#L194
>>>>>>
>>>>>> You could require something in children, but not in parent node. For
>>>>>> children something around:
>>>>>> https://elixir.bootlin.com/linux/v6.4-rc7/source/Documentation/
>>>>>> devicetree/bindings/net/qcom,ipa.yaml#L174
>>>>>>
>>>>>> allOf:
>>>>>> - if:
>>>>>> required:
>>>>>> - something-in-parent
>>>>>> then:
>>>>>> properties:
>>>>>> child-node:
>>>>>> required:
>>>>>> - something-in-child
>>>>>>
>>>>>> I will see if I can turn that into a workable proposal/patch.
>>>>>>
>>>>>
>>>>> thank you for pushing my review request forward.
>>>>>
>>>>> Overall I believe making supply properties as optional ones is
>>>>> sufficient,
>>>>> technically straightforward and merely good enough, thus please let me
>>>>> ask you to ponder on this particular variant one more time.
>>>>>
>>>>
>>>> So, we are discussing two things.
>>>>
>>>> 1# Use separate supplies for each CSI block, looks like there is no
>>>> doubt about it anymore. So, I will update it just like based on
>>>> suggestion.
>>>>
>>>> csiphyX-vdda-phy-supply
>>>> csiphyX-vdda-pll-supply
>>>>
>>>> Then I will need below items in the required list if they are required.
>>>> required:
>>>> - csiphy0-vdda-phy-supply
>>>> - csiphy0-vdda-pll-supply
>>>> - csiphy1-vdda-phy-supply
>>>> - csiphy1-vdda-pll-supply
>>>> ...
>>>> - csiphy7-vdda-phy-supply
>>>> - csiphy7-vdda-pll-supply
>>>>
>>>> 2# Regarding the CSI supplies, if they need to be making as optional?
>>>> Looks like there is no conclusion now.
>>>>
>>>> @Bryan, do you agree with this?
>>>>
>>>
>>> I'm preparing the new version patches, and will send out for reviewing
>>> in few days. I will follow Vladimir's comments if you have no response,
>>> it means making supply properties as optional one, so they won't be
>>> added to the required list.
>>>
>>
>> Recently I published the change, which moves regulator supplies from CSID
>> to CSIPHY, I believe it makes sense to base the SM8550 change and
>> regulators
>> under discussion on top of the series:
>>
>> https://lore.kernel.org/all/20240926211957.4108692-1-
>> vladimir.zapolskiy@linaro.org/
>>
>> Note, that SM8250 regulators are not changed, however their names are
>> wrong,
>> the correction shall be a separate change later on...
>>
>> Next, I developed my opinion regarding the supply regulator property
>> names:
>>
>> 1) voltage supply regulator property names match the pattern "*v*-
>> supply",
>> and the most common name is "vdd*-supply", the match to the
>> pattern shall
>> be preserved,
>> 2) also it would be much better and it will exclude any confusion, if
>> SoC pin
>> names are put into the name, like it is done in a multitude of
>> similar
>> cases.
>>
>> So, in my opinion for SM8550 CAMSS a proposed set of voltage supply
>> regulator
>> names should be this one:
>>
>> - vdda-csi01-0p9-supply
>> - vdda-csi01-1p2-supply
>> - vdda-csi23-0p9-supply
>> - vdda-csi23-1p2-supply
>> - vdda-csi46-0p9-supply
>> - vdda-csi46-1p2-supply
>> - vdda-csi57-0p9-supply
>> - vdda-csi57-1p2-supply
>
> So I communicated to Depeng to take the patch for the regulators but, I
> still don't think the above is the right way to do this.
>
> I will take a pass at constructing something in the schema to capture
> the case where a regulator is required if and only if it is instantiated.
>
> May not be possible with our current syntax/tools but is 100% how the
> hardware works so IMO is the right thing to try to do.
>
Yes, I have picked your patch and rebased the SM8550 change based on
your patch. I also verified them and it works good.
But I don't understand why the names are csi01, csi23, csi46, csi57.
Could you please elaborate more?
I'm using csiphyX-vdda-phy-supply and csiphyX-vdda-pll-supply now.
Thanks,
Depeng
new file mode 100644
@@ -0,0 +1,517 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/qcom,sm8550-camss.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm SM8550 Camera Subsystem (CAMSS)
+
+maintainers:
+ - Depeng Shao <quic_depengs@quicinc.com>
+
+description:
+ The CAMSS IP is a CSI decoder and ISP present on Qualcomm platforms.
+
+properties:
+ compatible:
+ const: qcom,sm8550-camss
+
+ reg:
+ maxItems: 19
+
+ reg-names:
+ items:
+ - const: csid0
+ - const: csid1
+ - const: csid2
+ - const: csid_lite0
+ - const: csid_lite1
+ - const: csid_top
+ - const: csiphy0
+ - const: csiphy1
+ - const: csiphy2
+ - const: csiphy3
+ - const: csiphy4
+ - const: csiphy5
+ - const: csiphy6
+ - const: csiphy7
+ - const: vfe0
+ - const: vfe1
+ - const: vfe2
+ - const: vfe_lite0
+ - const: vfe_lite1
+
+ clocks:
+ maxItems: 36
+
+ clock-names:
+ items:
+ - const: camnoc_axi
+ - const: cpas_ahb
+ - const: cpas_fast_ahb_clk
+ - const: cpas_ife_lite
+ - const: cpas_vfe0
+ - const: cpas_vfe1
+ - const: cpas_vfe2
+ - const: csid
+ - const: csiphy0
+ - const: csiphy0_timer
+ - const: csiphy1
+ - const: csiphy1_timer
+ - const: csiphy2
+ - const: csiphy2_timer
+ - const: csiphy3
+ - const: csiphy3_timer
+ - const: csiphy4
+ - const: csiphy4_timer
+ - const: csiphy5
+ - const: csiphy5_timer
+ - const: csiphy6
+ - const: csiphy6_timer
+ - const: csiphy7
+ - const: csiphy7_timer
+ - const: csiphy_rx
+ - const: vfe0
+ - const: vfe0_fast_ahb
+ - const: vfe1
+ - const: vfe1_fast_ahb
+ - const: vfe2
+ - const: vfe2_fast_ahb
+ - const: vfe_lite
+ - const: vfe_lite_ahb
+ - const: vfe_lite_cphy_rx
+ - const: vfe_lite_csid
+ - const: gcc_axi_hf
+
+ interconnects:
+ maxItems: 4
+
+ interconnect-names:
+ items:
+ - const: ahb
+ - const: hf_0_mnoc
+ - const: icp_mnoc
+ - const: sf_0_mnoc
+
+ interrupts:
+ maxItems: 18
+
+ interrupt-names:
+ items:
+ - const: csid0
+ - const: csid1
+ - const: csid2
+ - const: csid_lite0
+ - const: csid_lite1
+ - const: csiphy0
+ - const: csiphy1
+ - const: csiphy2
+ - const: csiphy3
+ - const: csiphy4
+ - const: csiphy5
+ - const: csiphy6
+ - const: csiphy7
+ - const: vfe0
+ - const: vfe1
+ - const: vfe2
+ - const: vfe_lite0
+ - const: vfe_lite1
+
+ iommus:
+ maxItems: 1
+
+ power-domains:
+ items:
+ - description: IFE0 GDSC - Image Front End, Global Distributed Switch Controller.
+ - description: IFE1 GDSC - Image Front End, Global Distributed Switch Controller.
+ - description: IFE2 GDSC - Image Front End, Global Distributed Switch Controller.
+ - description: Titan GDSC - Titan ISP Block, Global Distributed Switch Controller.
+
+ power-domain-names:
+ items:
+ - const: ife0
+ - const: ife1
+ - const: ife2
+ - const: top
+
+ vdda-phy-supply:
+ description:
+ Phandle to a regulator supply to PHY core block.
+
+ vdda-pll-supply:
+ description:
+ Phandle to 1.2V regulator supply to PHY refclk pll block.
+
+ ports:
+ $ref: /schemas/graph.yaml#/properties/ports
+
+ description:
+ CSI input ports.
+
+ properties:
+ port@0:
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ unevaluatedProperties: false
+ description:
+ Input port for receiving CSI data.
+
+ properties:
+ endpoint:
+ $ref: video-interfaces.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ clock-lanes:
+ maxItems: 1
+
+ data-lanes:
+ minItems: 1
+ maxItems: 4
+
+ required:
+ - clock-lanes
+ - data-lanes
+
+ port@1:
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ unevaluatedProperties: false
+ description:
+ Input port for receiving CSI data.
+
+ properties:
+ endpoint:
+ $ref: video-interfaces.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ clock-lanes:
+ maxItems: 1
+
+ data-lanes:
+ minItems: 1
+ maxItems: 4
+
+ required:
+ - clock-lanes
+ - data-lanes
+
+ port@2:
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ unevaluatedProperties: false
+ description:
+ Input port for receiving CSI data.
+
+ properties:
+ endpoint:
+ $ref: video-interfaces.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ clock-lanes:
+ maxItems: 1
+
+ data-lanes:
+ minItems: 1
+ maxItems: 4
+
+ required:
+ - clock-lanes
+ - data-lanes
+
+ port@3:
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ unevaluatedProperties: false
+ description:
+ Input port for receiving CSI data.
+
+ properties:
+ endpoint:
+ $ref: video-interfaces.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ clock-lanes:
+ maxItems: 1
+
+ data-lanes:
+ minItems: 1
+ maxItems: 4
+
+ required:
+ - clock-lanes
+ - data-lanes
+
+ port@4:
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ unevaluatedProperties: false
+ description:
+ Input port for receiving CSI data.
+
+ properties:
+ endpoint:
+ $ref: video-interfaces.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ clock-lanes:
+ maxItems: 1
+
+ data-lanes:
+ minItems: 1
+ maxItems: 4
+
+ required:
+ - clock-lanes
+ - data-lanes
+
+ port@5:
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ unevaluatedProperties: false
+ description:
+ Input port for receiving CSI data.
+
+ properties:
+ endpoint:
+ $ref: video-interfaces.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ clock-lanes:
+ maxItems: 1
+
+ data-lanes:
+ minItems: 1
+ maxItems: 4
+
+ required:
+ - clock-lanes
+ - data-lanes
+
+required:
+ - compatible
+ - clocks
+ - clock-names
+ - interconnects
+ - interconnect-names
+ - interrupts
+ - interrupt-names
+ - iommus
+ - power-domains
+ - power-domain-names
+ - reg
+ - reg-names
+ - vdda-phy-supply
+ - vdda-pll-supply
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/qcom,rpmh.h>
+ #include <dt-bindings/clock/qcom,sm8550-camcc.h>
+ #include <dt-bindings/clock/qcom,sm8550-gcc.h>
+ #include <dt-bindings/interconnect/qcom,sm8550-rpmh.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/power/qcom-rpmpd.h>
+
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ camss: camss@ace4000 {
+ 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 0x2800>,
+ <0 0x0accf000 0 0x2800>;
+ 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";
+
+ vdda-phy-supply = <&vreg_l1e_0p88>;
+ vdda-pll-supply = <&vreg_l3e_1p2>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ csiphy_ep0: endpoint@0 {
+ reg = <0>;
+ clock-lanes = <7>;
+ data-lanes = <0 1>;
+ remote-endpoint = <&sensor_ep>;
+ };
+ };
+ };
+ };
+ };