| Message ID | 20241217140656.965235-5-quic_vikramsa@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: qcom: camss: Add sc7280 support | expand |
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
On 19/12/2024 19:32, Vladimir Zapolskiy wrote: >>>> + 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. I agree for this particular case that rst-pin should be excised. - RST is an active low signal, which is typically _pulsed_ for a period when the sensor is powered to trigger a reset in the state machine of the sensor - What is the use-case of pulling RST down the GPIO in suspend ? I'd remove the output-low though it should make no difference as the sensor regulators will be off. - MCLK I think should have a suspend state specified or at least I can't think of a good reason right now why what I see here is wrong. For the default state this patch disables the GPIO pull down bias, which to me seems logical and correct. TBH I don't have a big concern about the RST pin in reset because the regulators will be off. --- bod
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; + }; + }; +};