| Message ID | 20241217140656.965235-5-quic_vikramsa@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: qcom: camss: Add sc7280 support | expand |
On 12/17/24 16:06, Vikram Sharma wrote: > The Vision Mezzanine for the RB3 ships with an imx577 camera sensor. > Enable the IMX577 on the vision mezzanine. > > An example media-ctl pipeline for the imx577 is: > > media-ctl --reset > media-ctl -v -V '"imx577 '19-001a'":0[fmt:SRGGB10/4056x3040 field:none]' > media-ctl -V '"msm_csiphy3":0[fmt:SRGGB10/4056x3040]' > media-ctl -V '"msm_csid0":0[fmt:SRGGB10/4056x3040]' > media-ctl -V '"msm_vfe0_rdi0":0[fmt:SRGGB10/4056x3040]' > media-ctl -l '"msm_csiphy3":1->"msm_csid0":0[1]' > media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]' > > yavta -B capture-mplane -c -I -n 5 -f SRGGB10P -s 4056x3040 -F /dev/video0 > > Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com> > Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com> > Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com> > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > --- > arch/arm64/boot/dts/qcom/Makefile | 4 + > .../qcs6490-rb3gen2-vision-mezzanine.dtso | 109 ++++++++++++++++++ > 2 files changed, 113 insertions(+) > create mode 100644 arch/arm64/boot/dts/qcom/qcs6490-rb3gen2-vision-mezzanine.dtso > > diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile > index 4686f2a8ddd8..a7e88fcabded 100644 > --- a/arch/arm64/boot/dts/qcom/Makefile > +++ b/arch/arm64/boot/dts/qcom/Makefile > @@ -115,6 +115,10 @@ dtb-$(CONFIG_ARCH_QCOM) += qcs404-evb-1000.dtb > dtb-$(CONFIG_ARCH_QCOM) += qcs404-evb-4000.dtb > dtb-$(CONFIG_ARCH_QCOM) += qcs615-ride.dtb > dtb-$(CONFIG_ARCH_QCOM) += qcs6490-rb3gen2.dtb > + > +qcs6490-rb3gen2-vision-mezzanine-dtbs := qcs6490-rb3gen2.dtb qcs6490-rb3gen2-vision-mezzanine.dtbo > + > +dtb-$(CONFIG_ARCH_QCOM) += qcs6490-rb3gen2-vision-mezzanine.dtb > dtb-$(CONFIG_ARCH_QCOM) += qcs8300-ride.dtb > dtb-$(CONFIG_ARCH_QCOM) += qcs8550-aim300-aiot.dtb > dtb-$(CONFIG_ARCH_QCOM) += qcs9100-ride.dtb > diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2-vision-mezzanine.dtso b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2-vision-mezzanine.dtso > new file mode 100644 > index 000000000000..7782c4aee576 > --- /dev/null > +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2-vision-mezzanine.dtso > @@ -0,0 +1,109 @@ > +// SPDX-License-Identifier: BSD-3-Clause > +/* > + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +/* > + * Camera Sensor overlay on top of rb3gen2 core kit. > + */ > + > +#include <dt-bindings/gpio/gpio.h> > +#include <dt-bindings/clock/qcom,camcc-sc7280.h> Please sort the header files alphabetically. > + > +/dts-v1/; > +/plugin/; > + Please put these two lines right after the comments header. > +&camss { > + vdda-phy-supply = <&vreg_l10c_0p88>; > + vdda-pll-supply = <&vreg_l6b_1p2>; > + > + status = "okay"; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + /* The port index denotes CSIPHY id i.e. csiphy3 */ > + port@3 { > + reg = <3>; > + > + csiphy3_ep: endpoint { > + clock-lanes = <7>; > + data-lanes = <0 1 2 3>; > + remote-endpoint = <&imx577_ep>; > + }; > + }; > + }; > +}; > + > +&cci1 { > + status = "okay"; > +}; > + > +&cci1_i2c1 { > + #address-cells = <1>; > + #size-cells = <0>; > + > + camera@1a { > + compatible = "sony,imx577"; > + > + reg = <0x1a>; > + > + reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>; > + pinctrl-names = "default", "suspend"; > + pinctrl-0 = <&cam2_default>; > + pinctrl-1 = <&cam2_suspend>; > + > + clocks = <&camcc CAM_CC_MCLK3_CLK>; > + assigned-clocks = <&camcc CAM_CC_MCLK3_CLK>; > + assigned-clock-rates = <24000000>; > + > + dovdd-supply = <&vreg_l18b_1p8>; Please remove double space before '='. > + avdd-supply = <&vph_pwr>; > + dvdd-supply = <&vph_pwr>; > + > + port { > + imx577_ep: endpoint { > + clock-lanes = <7>; It is an invalid property/value of the sensor, please remove it. > + link-frequencies = /bits/ 64 <600000000>; > + data-lanes = <0 1 2 3>; data-lanes = <1 2 3 4>; > + remote-endpoint = <&csiphy3_ep>; > + }; > + }; > + }; > +}; > + > +&tlmm { > + cam2_default: cam2-default-state { > + mclk-pins { > + pins = "gpio67"; > + function = "cam_mclk"; > + drive-strength = <2>; > + bias-disable; > + }; > + > + rst-pins { > + pins = "gpio78"; > + function = "gpio"; > + drive-strength = <2>; > + bias-disable; > + }; > + }; > + > + cam2_suspend: cam2-suspend-state { > + mclk-pins { > + pins = "gpio67"; > + function = "cam_mclk"; > + drive-strength = <2>; > + bias-pull-down; > + }; > + > + rst-pins { > + pins = "gpio78"; > + function = "gpio"; > + drive-strength = <2>; > + bias-pull-down; > + output-low; > + }; I have doubts that it's proper to embed a reset gpio into driver's pinctrl suspend/resume power management. Konrad, can you please confirm that it's really accepted? I'd rather ask to remove this reset pin control. > + }; > +}; -- Best wishes, Vladimir
On 12/17/2024 8:10 PM, Vladimir Zapolskiy wrote: > On 12/17/24 16:06, Vikram Sharma wrote: >> The Vision Mezzanine for the RB3 ships with an imx577 camera sensor. >> Enable the IMX577 on the vision mezzanine. >> >> An example media-ctl pipeline for the imx577 is: >> >> media-ctl --reset >> media-ctl -v -V '"imx577 '19-001a'":0[fmt:SRGGB10/4056x3040 field:none]' >> media-ctl -V '"msm_csiphy3":0[fmt:SRGGB10/4056x3040]' >> media-ctl -V '"msm_csid0":0[fmt:SRGGB10/4056x3040]' >> media-ctl -V '"msm_vfe0_rdi0":0[fmt:SRGGB10/4056x3040]' >> media-ctl -l '"msm_csiphy3":1->"msm_csid0":0[1]' >> media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]' >> >> yavta -B capture-mplane -c -I -n 5 -f SRGGB10P -s 4056x3040 -F >> /dev/video0 >> >> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com> >> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com> >> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com> >> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> >> --- >> arch/arm64/boot/dts/qcom/Makefile | 4 + >> .../qcs6490-rb3gen2-vision-mezzanine.dtso | 109 ++++++++++++++++++ >> 2 files changed, 113 insertions(+) >> create mode 100644 >> arch/arm64/boot/dts/qcom/qcs6490-rb3gen2-vision-mezzanine.dtso >> >> diff --git a/arch/arm64/boot/dts/qcom/Makefile >> b/arch/arm64/boot/dts/qcom/Makefile >> index 4686f2a8ddd8..a7e88fcabded 100644 >> --- a/arch/arm64/boot/dts/qcom/Makefile >> +++ b/arch/arm64/boot/dts/qcom/Makefile >> @@ -115,6 +115,10 @@ dtb-$(CONFIG_ARCH_QCOM) += qcs404-evb-1000.dtb >> dtb-$(CONFIG_ARCH_QCOM) += qcs404-evb-4000.dtb >> dtb-$(CONFIG_ARCH_QCOM) += qcs615-ride.dtb >> dtb-$(CONFIG_ARCH_QCOM) += qcs6490-rb3gen2.dtb >> + >> +qcs6490-rb3gen2-vision-mezzanine-dtbs := qcs6490-rb3gen2.dtb >> qcs6490-rb3gen2-vision-mezzanine.dtbo >> + >> +dtb-$(CONFIG_ARCH_QCOM) += qcs6490-rb3gen2-vision-mezzanine.dtb >> dtb-$(CONFIG_ARCH_QCOM) += qcs8300-ride.dtb >> dtb-$(CONFIG_ARCH_QCOM) += qcs8550-aim300-aiot.dtb >> dtb-$(CONFIG_ARCH_QCOM) += qcs9100-ride.dtb >> diff --git >> a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2-vision-mezzanine.dtso >> b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2-vision-mezzanine.dtso >> new file mode 100644 >> index 000000000000..7782c4aee576 >> --- /dev/null >> +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2-vision-mezzanine.dtso >> @@ -0,0 +1,109 @@ >> +// SPDX-License-Identifier: BSD-3-Clause >> +/* >> + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights >> reserved. >> + */ >> + >> +/* >> + * Camera Sensor overlay on top of rb3gen2 core kit. >> + */ >> + >> +#include <dt-bindings/gpio/gpio.h> >> +#include <dt-bindings/clock/qcom,camcc-sc7280.h> > > Please sort the header files alphabetically. Thanks for your comments Vladimir. Will take care in next version. > >> + >> +/dts-v1/; >> +/plugin/; >> + > > Please put these two lines right after the comments header. ACK. > >> +&camss { >> + vdda-phy-supply = <&vreg_l10c_0p88>; >> + vdda-pll-supply = <&vreg_l6b_1p2>; >> + >> + status = "okay"; >> + >> + ports { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + /* The port index denotes CSIPHY id i.e. csiphy3 */ >> + port@3 { >> + reg = <3>; >> + >> + csiphy3_ep: endpoint { >> + clock-lanes = <7>; >> + data-lanes = <0 1 2 3>; >> + remote-endpoint = <&imx577_ep>; >> + }; >> + }; >> + }; >> +}; >> + >> +&cci1 { >> + status = "okay"; >> +}; >> + >> +&cci1_i2c1 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + camera@1a { >> + compatible = "sony,imx577"; >> + >> + reg = <0x1a>; >> + >> + reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>; >> + pinctrl-names = "default", "suspend"; >> + pinctrl-0 = <&cam2_default>; >> + pinctrl-1 = <&cam2_suspend>; >> + >> + clocks = <&camcc CAM_CC_MCLK3_CLK>; >> + assigned-clocks = <&camcc CAM_CC_MCLK3_CLK>; >> + assigned-clock-rates = <24000000>; >> + >> + dovdd-supply = <&vreg_l18b_1p8>; > > Please remove double space before '='. Sure. > >> + avdd-supply = <&vph_pwr>; >> + dvdd-supply = <&vph_pwr>; >> + >> + port { >> + imx577_ep: endpoint { >> + clock-lanes = <7>; > > It is an invalid property/value of the sensor, please remove it. Will check more on this internally and respond back. > >> + link-frequencies = /bits/ 64 <600000000>; >> + data-lanes = <0 1 2 3>; > > data-lanes = <1 2 3 4>; Will check more on this internally and respond back > >> + remote-endpoint = <&csiphy3_ep>; >> + }; >> + }; >> + }; >> +}; >> + >> +&tlmm { >> + cam2_default: cam2-default-state { >> + mclk-pins { >> + pins = "gpio67"; >> + function = "cam_mclk"; >> + drive-strength = <2>; >> + bias-disable; >> + }; >> + >> + rst-pins { >> + pins = "gpio78"; >> + function = "gpio"; >> + drive-strength = <2>; >> + bias-disable; >> + }; >> + }; >> + >> + cam2_suspend: cam2-suspend-state { >> + mclk-pins { >> + pins = "gpio67"; >> + function = "cam_mclk"; >> + drive-strength = <2>; >> + bias-pull-down; >> + }; >> + >> + rst-pins { >> + pins = "gpio78"; >> + function = "gpio"; >> + drive-strength = <2>; >> + bias-pull-down; >> + output-low; >> + }; > > I have doubts that it's proper to embed a reset gpio into driver's > pinctrl suspend/resume power management. > > Konrad, can you please confirm that it's really accepted? > > I'd rather ask to remove this reset pin control. Will discuss this with Konrad and respond. >> + }; >> +}; > > -- > Best wishes, > Vladimir Best Regards, Vikram
On 17.12.2024 3:40 PM, Vladimir Zapolskiy wrote: > On 12/17/24 16:06, Vikram Sharma wrote: >> The Vision Mezzanine for the RB3 ships with an imx577 camera sensor. >> Enable the IMX577 on the vision mezzanine. >> >> An example media-ctl pipeline for the imx577 is: >> >> media-ctl --reset >> media-ctl -v -V '"imx577 '19-001a'":0[fmt:SRGGB10/4056x3040 field:none]' >> media-ctl -V '"msm_csiphy3":0[fmt:SRGGB10/4056x3040]' >> media-ctl -V '"msm_csid0":0[fmt:SRGGB10/4056x3040]' >> media-ctl -V '"msm_vfe0_rdi0":0[fmt:SRGGB10/4056x3040]' >> media-ctl -l '"msm_csiphy3":1->"msm_csid0":0[1]' >> media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]' >> >> yavta -B capture-mplane -c -I -n 5 -f SRGGB10P -s 4056x3040 -F /dev/video0 >> >> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com> >> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com> >> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com> >> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> >> --- [...] >> + rst-pins { >> + pins = "gpio78"; >> + function = "gpio"; >> + drive-strength = <2>; >> + bias-pull-down; >> + output-low; >> + }; > > I have doubts that it's proper to embed a reset gpio into driver's > pinctrl suspend/resume power management. > > Konrad, can you please confirm that it's really accepted? > > I'd rather ask to remove this reset pin control. There's certainly some appearances of this in the tree. You could make the argument that it makes sense to prevent misconfiguration (i.e. the bootloader may set the pin in input mode), but then the counter argument is that the (Linux) gpiod APIs request OUT_LOW/HIGH, and we would expect that the driver uses that if the GPIO is requested through e.g. reset-gpios. I'm not particularly sure what to recommend here. Krzysztof? Konrad
On 12/19/24 21:06, Konrad Dybcio wrote: > On 17.12.2024 3:40 PM, Vladimir Zapolskiy wrote: >> On 12/17/24 16:06, Vikram Sharma wrote: >>> The Vision Mezzanine for the RB3 ships with an imx577 camera sensor. >>> Enable the IMX577 on the vision mezzanine. >>> >>> An example media-ctl pipeline for the imx577 is: >>> >>> media-ctl --reset >>> media-ctl -v -V '"imx577 '19-001a'":0[fmt:SRGGB10/4056x3040 field:none]' >>> media-ctl -V '"msm_csiphy3":0[fmt:SRGGB10/4056x3040]' >>> media-ctl -V '"msm_csid0":0[fmt:SRGGB10/4056x3040]' >>> media-ctl -V '"msm_vfe0_rdi0":0[fmt:SRGGB10/4056x3040]' >>> media-ctl -l '"msm_csiphy3":1->"msm_csid0":0[1]' >>> media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]' >>> >>> yavta -B capture-mplane -c -I -n 5 -f SRGGB10P -s 4056x3040 -F /dev/video0 >>> >>> Signed-off-by: Hariram Purushothaman <quic_hariramp@quicinc.com> >>> Signed-off-by: Vikram Sharma <quic_vikramsa@quicinc.com> >>> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@quicinc.com> >>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >>> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> >>> --- > > [...] > >>> + rst-pins { >>> + pins = "gpio78"; >>> + function = "gpio"; >>> + drive-strength = <2>; >>> + bias-pull-down; >>> + output-low; >>> + }; >> >> I have doubts that it's proper to embed a reset gpio into driver's >> pinctrl suspend/resume power management. >> >> Konrad, can you please confirm that it's really accepted? >> >> I'd rather ask to remove this reset pin control. > > There's certainly some appearances of this in the tree. > > You could make the argument that it makes sense to prevent misconfiguration > (i.e. the bootloader may set the pin in input mode), but then the counter > argument is that the (Linux) gpiod APIs request OUT_LOW/HIGH, and we would > expect that the driver uses that if the GPIO is requested through > e.g. reset-gpios. > > I'm not particularly sure what to recommend here. Krzysztof? > I'm worried by a possibility that a device reset/shutdown control GPIO could be turned off by entering the "sleep" pinctrl setup. If a particular GPIO/pin is off, is it still continuously functional as a control GPIO of some device? I believe it is not anymore in general, this is my concern here. -- Best wishes, Vladimir
diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile index 4686f2a8ddd8..a7e88fcabded 100644 --- a/arch/arm64/boot/dts/qcom/Makefile +++ b/arch/arm64/boot/dts/qcom/Makefile @@ -115,6 +115,10 @@ dtb-$(CONFIG_ARCH_QCOM) += qcs404-evb-1000.dtb dtb-$(CONFIG_ARCH_QCOM) += qcs404-evb-4000.dtb dtb-$(CONFIG_ARCH_QCOM) += qcs615-ride.dtb dtb-$(CONFIG_ARCH_QCOM) += qcs6490-rb3gen2.dtb + +qcs6490-rb3gen2-vision-mezzanine-dtbs := qcs6490-rb3gen2.dtb qcs6490-rb3gen2-vision-mezzanine.dtbo + +dtb-$(CONFIG_ARCH_QCOM) += qcs6490-rb3gen2-vision-mezzanine.dtb dtb-$(CONFIG_ARCH_QCOM) += qcs8300-ride.dtb dtb-$(CONFIG_ARCH_QCOM) += qcs8550-aim300-aiot.dtb dtb-$(CONFIG_ARCH_QCOM) += qcs9100-ride.dtb diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2-vision-mezzanine.dtso b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2-vision-mezzanine.dtso new file mode 100644 index 000000000000..7782c4aee576 --- /dev/null +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2-vision-mezzanine.dtso @@ -0,0 +1,109 @@ +// SPDX-License-Identifier: BSD-3-Clause +/* + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved. + */ + +/* + * Camera Sensor overlay on top of rb3gen2 core kit. + */ + +#include <dt-bindings/gpio/gpio.h> +#include <dt-bindings/clock/qcom,camcc-sc7280.h> + +/dts-v1/; +/plugin/; + +&camss { + vdda-phy-supply = <&vreg_l10c_0p88>; + vdda-pll-supply = <&vreg_l6b_1p2>; + + status = "okay"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + /* The port index denotes CSIPHY id i.e. csiphy3 */ + port@3 { + reg = <3>; + + csiphy3_ep: endpoint { + clock-lanes = <7>; + data-lanes = <0 1 2 3>; + remote-endpoint = <&imx577_ep>; + }; + }; + }; +}; + +&cci1 { + status = "okay"; +}; + +&cci1_i2c1 { + #address-cells = <1>; + #size-cells = <0>; + + camera@1a { + compatible = "sony,imx577"; + + reg = <0x1a>; + + reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>; + pinctrl-names = "default", "suspend"; + pinctrl-0 = <&cam2_default>; + pinctrl-1 = <&cam2_suspend>; + + clocks = <&camcc CAM_CC_MCLK3_CLK>; + assigned-clocks = <&camcc CAM_CC_MCLK3_CLK>; + assigned-clock-rates = <24000000>; + + dovdd-supply = <&vreg_l18b_1p8>; + avdd-supply = <&vph_pwr>; + dvdd-supply = <&vph_pwr>; + + port { + imx577_ep: endpoint { + clock-lanes = <7>; + link-frequencies = /bits/ 64 <600000000>; + data-lanes = <0 1 2 3>; + remote-endpoint = <&csiphy3_ep>; + }; + }; + }; +}; + +&tlmm { + cam2_default: cam2-default-state { + mclk-pins { + pins = "gpio67"; + function = "cam_mclk"; + drive-strength = <2>; + bias-disable; + }; + + rst-pins { + pins = "gpio78"; + function = "gpio"; + drive-strength = <2>; + bias-disable; + }; + }; + + cam2_suspend: cam2-suspend-state { + mclk-pins { + pins = "gpio67"; + function = "cam_mclk"; + drive-strength = <2>; + bias-pull-down; + }; + + rst-pins { + pins = "gpio78"; + function = "gpio"; + drive-strength = <2>; + bias-pull-down; + output-low; + }; + }; +};