| Message ID | 20250612011531.2923701-10-vladimir.zapolskiy@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | media: qcom: camss: add support for csiphy devices | expand |
On 12/06/2025 03:15, Vladimir Zapolskiy wrote: > Following the new device tree bindings for CAMSS IPs introduce csiphy2 > device tree node under SM8250 CAMSS, which allows to perform camera > tests of the model on an RB5 board with an attached vision mezzanine. How the binding allows to perform camera tests? So camera was not working here at all? Then this is a fix, no? > > Note that the optional 'phys' property is deliberately not added. Why? Your commit msg must explain that. > > Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> > --- > For testing only, do not merge. > > arch/arm64/boot/dts/qcom/sm8250.dtsi | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi > index f0d18fd37aaf..401a32679580 100644 > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi > @@ -4613,6 +4613,10 @@ camss: camss@ac6a000 { > "cam_sf_0_mnoc", > "cam_sf_icp_mnoc"; > > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + > ports { > #address-cells = <1>; > #size-cells = <0>; > @@ -4641,6 +4645,16 @@ port@5 { > reg = <5>; > }; > }; > + > + csiphy2: phy@ac6e000 { This will fail checks. You can run them, regardless of "RFT" status. > + compatible = "qcom,csiphy"; > + reg = <0 0x0ac6e000 0 0x1000>; > + clocks = <&camcc CAM_CC_CSIPHY2_CLK>, > + <&camcc CAM_CC_CSI2PHYTIMER_CLK>; > + clock-names = "csiphy", "csiphy_timer"; > + interrupts = <GIC_SPI 479 IRQ_TYPE_EDGE_RISING>; > + #phy-cells = <0>; This is also duplicating existing ports thus you have a mixed MMIO and non-MMIO children which is also issue to fix. > + }; > }; > > camcc: clock-controller@ad00000 { Best regards, Krzysztof
On 6/12/25 9:43 AM, Krzysztof Kozlowski wrote: > On 12/06/2025 03:15, Vladimir Zapolskiy wrote: >> Following the new device tree bindings for CAMSS IPs introduce csiphy2 >> device tree node under SM8250 CAMSS, which allows to perform camera >> tests of the model on an RB5 board with an attached vision mezzanine. [...] >> + compatible = "qcom,csiphy"; >> + reg = <0 0x0ac6e000 0 0x1000>; >> + clocks = <&camcc CAM_CC_CSIPHY2_CLK>, >> + <&camcc CAM_CC_CSI2PHYTIMER_CLK>; >> + clock-names = "csiphy", "csiphy_timer"; >> + interrupts = <GIC_SPI 479 IRQ_TYPE_EDGE_RISING>; >> + #phy-cells = <0>; > > This is also duplicating existing ports thus you have a mixed MMIO and > non-MMIO children which is also issue to fix. Does the CSIPHY work without the camera ahb clk/TOP GDSC enabled? (as far as register access and doing things that don't depend on input from other components) I think moving it outside makes more sense both in a "breaking up the monolith" way and in case the answer to the question is positive, in representing it as an individual device. FWIW I'm generally in favor of what downstream does with regards to camss - *every* instance of every device has its own node, as it represents a physical subblock and has its individual clocks etc. I'm genuinely (not necessarily positively) surprised that we managed to get this far with camss being a single big blob.. Konrad
On 6/12/25 10:43, Krzysztof Kozlowski wrote: > On 12/06/2025 03:15, Vladimir Zapolskiy wrote: >> Following the new device tree bindings for CAMSS IPs introduce csiphy2 >> device tree node under SM8250 CAMSS, which allows to perform camera >> tests of the model on an RB5 board with an attached vision mezzanine. > > How the binding allows to perform camera tests? So camera was not > working here at all? Then this is a fix, no? Here the assumed testing is a regression testing on the selected and well supported legacy platform SM8250/RB5. >> >> Note that the optional 'phys' property is deliberately not added. > > Why? Your commit msg must explain that. > Sure, will add it, just giving a short note here, 'phys' property provides a list of wanted/enabled phys, and this is board specific, thus, and if it is desired to preserve backward ABI compatibility, when this property is just not present, I believe it makes sense to add the 'phys' property to board .dts files only, like it's done in 10/10 patch of this series. >> >> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> >> --- >> For testing only, do not merge. This. >> arch/arm64/boot/dts/qcom/sm8250.dtsi | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi >> index f0d18fd37aaf..401a32679580 100644 >> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi >> @@ -4613,6 +4613,10 @@ camss: camss@ac6a000 { >> "cam_sf_0_mnoc", >> "cam_sf_icp_mnoc"; >> >> + #address-cells = <2>; >> + #size-cells = <2>; >> + ranges; >> + >> ports { >> #address-cells = <1>; >> #size-cells = <0>; >> @@ -4641,6 +4645,16 @@ port@5 { >> reg = <5>; >> }; >> }; >> + >> + csiphy2: phy@ac6e000 { > > This will fail checks. You can run them, regardless of "RFT" status. I ran "dt_binding_check" and "dtbs_check" with no errors reported, something is screwed on my end, because "additionalProperties: false" from qcom,sm8250-camss.yaml shall scream here... Or is it a child device node is not a property?.. >> + compatible = "qcom,csiphy"; >> + reg = <0 0x0ac6e000 0 0x1000>; >> + clocks = <&camcc CAM_CC_CSIPHY2_CLK>, >> + <&camcc CAM_CC_CSI2PHYTIMER_CLK>; >> + clock-names = "csiphy", "csiphy_timer"; >> + interrupts = <GIC_SPI 479 IRQ_TYPE_EDGE_RISING>; >> + #phy-cells = <0>; > > This is also duplicating existing ports thus you have a mixed MMIO and > non-MMIO children which is also issue to fix. That's correct, there are mixed MMIO and non-MMIO children above, there might be children of just any one type of two. It's a valuable review comment and I missed the flaw, thank you, and to be honest so far I don't have a good idea how to overcome the issue in an easy way... >> + }; >> }; >> >> camcc: clock-controller@ad00000 { > > -- Best wishes, Vladimir
diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi index f0d18fd37aaf..401a32679580 100644 --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi @@ -4613,6 +4613,10 @@ camss: camss@ac6a000 { "cam_sf_0_mnoc", "cam_sf_icp_mnoc"; + #address-cells = <2>; + #size-cells = <2>; + ranges; + ports { #address-cells = <1>; #size-cells = <0>; @@ -4641,6 +4645,16 @@ port@5 { reg = <5>; }; }; + + csiphy2: phy@ac6e000 { + compatible = "qcom,csiphy"; + reg = <0 0x0ac6e000 0 0x1000>; + clocks = <&camcc CAM_CC_CSIPHY2_CLK>, + <&camcc CAM_CC_CSI2PHYTIMER_CLK>; + clock-names = "csiphy", "csiphy_timer"; + interrupts = <GIC_SPI 479 IRQ_TYPE_EDGE_RISING>; + #phy-cells = <0>; + }; }; camcc: clock-controller@ad00000 {
Following the new device tree bindings for CAMSS IPs introduce csiphy2 device tree node under SM8250 CAMSS, which allows to perform camera tests of the model on an RB5 board with an attached vision mezzanine. Note that the optional 'phys' property is deliberately not added. Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> --- For testing only, do not merge. arch/arm64/boot/dts/qcom/sm8250.dtsi | 14 ++++++++++++++ 1 file changed, 14 insertions(+)