[v2,5/6] arm64: dts: qcom: apq8016-sbc-d3-camera-mezzanine: Move default ov5640 to a standalone dts
Commit Message
At the moment we define a single ov5640 sensor in the apq8016-sbc and
disable that sensor.
The sensor mezzanine for this is a D3 Engineering Dual ov5640 mezzanine
card. Move the definition from the apq8016-sbc where it shouldn't be to a
standalone dts.
Enables the sensor by default, as we are adding a standalone mezzanine
structure.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
arch/arm64/boot/dts/qcom/Makefile | 1 +
.../qcom/apq8016-sbc-d3-camera-mezzanine.dts | 55 +++++++++++++++++++
arch/arm64/boot/dts/qcom/apq8016-sbc.dts | 49 -----------------
3 files changed, 56 insertions(+), 49 deletions(-)
create mode 100644 arch/arm64/boot/dts/qcom/apq8016-sbc-d3-camera-mezzanine.dts
Comments
Hi,
just some nitpicks. Some of them were present before already but maybe
you can prepend or append another cleanup patch while at it. :)
On Wed, Aug 09, 2023 at 09:23:42PM +0100, Bryan O'Donoghue wrote:
> At the moment we define a single ov5640 sensor in the apq8016-sbc and
> disable that sensor.
>
> The sensor mezzanine for this is a D3 Engineering Dual ov5640 mezzanine
> card. Move the definition from the apq8016-sbc where it shouldn't be to a
> standalone dts.
>
I wonder what would be required to implement this using a DT overlay,
rather than a standalone separate DT? Seems like there are some .dtso
files in upstream Linux.
I'm also fine with the separate DTB personally, though.
> Enables the sensor by default, as we are adding a standalone mezzanine
> structure.
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
> arch/arm64/boot/dts/qcom/Makefile | 1 +
> .../qcom/apq8016-sbc-d3-camera-mezzanine.dts | 55 +++++++++++++++++++
> arch/arm64/boot/dts/qcom/apq8016-sbc.dts | 49 -----------------
> 3 files changed, 56 insertions(+), 49 deletions(-)
> create mode 100644 arch/arm64/boot/dts/qcom/apq8016-sbc-d3-camera-mezzanine.dts
>
> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index f15548dbfa56e..19016765ba4c6 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
> dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc.dtb
> +dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-d3-camera-mezzanine.dtb
> dtb-$(CONFIG_ARCH_QCOM) += apq8039-t2.dtb
> dtb-$(CONFIG_ARCH_QCOM) += apq8094-sony-xperia-kitakami-karin_windy.dtb
> dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb
> diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc-d3-camera-mezzanine.dts b/arch/arm64/boot/dts/qcom/apq8016-sbc-d3-camera-mezzanine.dts
> new file mode 100644
> index 0000000000000..ef0e76e424898
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/apq8016-sbc-d3-camera-mezzanine.dts
> @@ -0,0 +1,55 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright (c) 2023, Linaro Ltd.
> + */
I assume you have permission from the original contributor to relicense
this? apq8016-sbc.dts is GPL.
> +
> +/dts-v1/;
> +
> +#include "apq8016-sbc.dts"
> +
Please also move the fixed regulators here, they're part of the
mezzanine, not the DB410c.
> +&camss {
> + status = "okay";
> +
> + ports {
> + port@0 {
> + reg = <0>;
> + csiphy0_ep: endpoint {
> + data-lanes = <0 2>;
> + remote-endpoint = <&ov5640_ep>;
> + status = "okay";
Should be unneeded since it's not set to disabled anywhere?
> + };
> + };
> + };
> +};
> +
> +&cci {
> + status = "okay";
> +};
> +
> +&cci_i2c0 {
> + camera_rear@3b {
camera@
Thanks,
Stephan
On 10/08/2023 16:11, Stephan Gerhold wrote:
> Hi,
>
> just some nitpicks. Some of them were present before already but maybe
> you can prepend or append another cleanup patch while at it. :)
>
> On Wed, Aug 09, 2023 at 09:23:42PM +0100, Bryan O'Donoghue wrote:
>> At the moment we define a single ov5640 sensor in the apq8016-sbc and
>> disable that sensor.
>>
>> The sensor mezzanine for this is a D3 Engineering Dual ov5640 mezzanine
>> card. Move the definition from the apq8016-sbc where it shouldn't be to a
>> standalone dts.
>>
>
> I wonder what would be required to implement this using a DT overlay,
> rather than a standalone separate DT? Seems like there are some .dtso
> files in upstream Linux.
>
> I'm also fine with the separate DTB personally, though.
So, we've discussed that previously and its a good model, which I like
and which works well for RPI as an example.
AFAIK though the runtime dtbo overlay is still missing at least one
upstream commit and the state of dtbo in qcom bootloaders is variable,
probably good in LK, good in u-boot and then I'd say nothing doing.
I'm hoping to transition the mezzanine dtb files to something "generic"
for boards that support 96 boards interfaces.
Its a bit out of scope for this series as, all I really want to do here
is fixup obvious errors as I find them in camss and its dtbs.
So anyway the idea would be to define labels in the core board dts files
for stuff like "powerdown-gpios = <&tlmm 34 GPIO_ACTIVE_HIGH>;" I'm not
sure that's really feasible until its tried though.
Basically any mezzanine board would ideally only be defined once, with
96boards supporting baseboards providing the necessary additional detail
on pins and regulators for the mezzanine to consume..
Come to think of it though you'd have to #include "myboard.dts" so
maybe, probably, that idea not feasible.
dtbo would be better still but like I say I'm not presupposing a decent
bootloader that can apply the overlay.
I/we will look again at dtbo since its just a neater model really.
>> Enables the sensor by default, as we are adding a standalone mezzanine
>> structure.
>>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>> arch/arm64/boot/dts/qcom/Makefile | 1 +
>> .../qcom/apq8016-sbc-d3-camera-mezzanine.dts | 55 +++++++++++++++++++
>> arch/arm64/boot/dts/qcom/apq8016-sbc.dts | 49 -----------------
>> 3 files changed, 56 insertions(+), 49 deletions(-)
>> create mode 100644 arch/arm64/boot/dts/qcom/apq8016-sbc-d3-camera-mezzanine.dts
>>
>> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
>> index f15548dbfa56e..19016765ba4c6 100644
>> --- a/arch/arm64/boot/dts/qcom/Makefile
>> +++ b/arch/arm64/boot/dts/qcom/Makefile
>> @@ -1,5 +1,6 @@
>> # SPDX-License-Identifier: GPL-2.0
>> dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc.dtb
>> +dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-d3-camera-mezzanine.dtb
>> dtb-$(CONFIG_ARCH_QCOM) += apq8039-t2.dtb
>> dtb-$(CONFIG_ARCH_QCOM) += apq8094-sony-xperia-kitakami-karin_windy.dtb
>> dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb
>> diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc-d3-camera-mezzanine.dts b/arch/arm64/boot/dts/qcom/apq8016-sbc-d3-camera-mezzanine.dts
>> new file mode 100644
>> index 0000000000000..ef0e76e424898
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/apq8016-sbc-d3-camera-mezzanine.dts
>> @@ -0,0 +1,55 @@
>> +// SPDX-License-Identifier: BSD-3-Clause
>> +/*
>> + * Copyright (c) 2023, Linaro Ltd.
>> + */
>
> I assume you have permission from the original contributor to relicense
> this? apq8016-sbc.dts is GPL.
Eh it was Loic @ Linaro but, TBH I just added a boilerplate here ...
No you're right this should be // SPDX-License-Identifier: GPL-2.0-only
>
>> +
>> +/dts-v1/;
>> +
>> +#include "apq8016-sbc.dts"
>> +
>
> Please also move the fixed regulators here, they're part of the
> mezzanine, not the DB410c.
ack, true
>
>> +&camss {
>> + status = "okay";
>> +
>> + ports {
>> + port@0 {
>> + reg = <0>;
>> + csiphy0_ep: endpoint {
>> + data-lanes = <0 2>;
>> + remote-endpoint = <&ov5640_ep>;
>> + status = "okay";
>
> Should be unneeded since it's not set to disabled anywhere?
>> + };
>> + };
>> + };
>> +};
>> +
>> +&cci {
>> + status = "okay";
>> +};
>> +
>> +&cci_i2c0 {
>> + camera_rear@3b {
>
> camera@
sure
---
bod
On 10/08/2023 16:26, Bryan O'Donoghue wrote:
> probably good in LK
lk2nd obvs !
On Thu, Aug 10, 2023 at 04:26:34PM +0100, Bryan O'Donoghue wrote:
> On 10/08/2023 16:11, Stephan Gerhold wrote:
> > Hi,
> >
> > just some nitpicks. Some of them were present before already but maybe
> > you can prepend or append another cleanup patch while at it. :)
> >
> > On Wed, Aug 09, 2023 at 09:23:42PM +0100, Bryan O'Donoghue wrote:
> > > At the moment we define a single ov5640 sensor in the apq8016-sbc and
> > > disable that sensor.
> > >
> > > The sensor mezzanine for this is a D3 Engineering Dual ov5640 mezzanine
> > > card. Move the definition from the apq8016-sbc where it shouldn't be to a
> > > standalone dts.
> > >
> >
> > I wonder what would be required to implement this using a DT overlay,
> > rather than a standalone separate DT? Seems like there are some .dtso
> > files in upstream Linux.
> >
> > I'm also fine with the separate DTB personally, though.
>
> So, we've discussed that previously and its a good model, which I like and
> which works well for RPI as an example.
>
> AFAIK though the runtime dtbo overlay is still missing at least one upstream
> commit and the state of dtbo in qcom bootloaders is variable, probably good
> in LK, good in u-boot and then I'd say nothing doing.
>
AFAIU there is work going on (at Linaro?) to move the qcom RBs to use
U-Boot, so I guess that would be the easiest common base to work with.
There is an U-Boot port for DB410c as well.
> I'm hoping to transition the mezzanine dtb files to something "generic" for
> boards that support 96 boards interfaces.
>
> Its a bit out of scope for this series as, all I really want to do here is
> fixup obvious errors as I find them in camss and its dtbs.
>
Right, yeah I think it's fine to have the separate DTB for now. I was
always confused about the disabled camera parts in apq8016-sbc, having
it in a separate DTB is definitely less confusing. :)
> So anyway the idea would be to define labels in the core board dts files for
> stuff like "powerdown-gpios = <&tlmm 34 GPIO_ACTIVE_HIGH>;" I'm not sure
> that's really feasible until its tried though.
Handling the GPIOs sounds complicated... I think it would be also okay
to have board-specific mezzanine overlays as a first step though. (i.e.
one for DB410c, others for other compatible 96boards).
>
> Basically any mezzanine board would ideally only be defined once, with
> 96boards supporting baseboards providing the necessary additional detail on
> pins and regulators for the mezzanine to consume..
>
> Come to think of it though you'd have to #include "myboard.dts" so maybe,
> probably, that idea not feasible.
>
> dtbo would be better still but like I say I'm not presupposing a decent
> bootloader that can apply the overlay.
>
> I/we will look again at dtbo since its just a neater model really.
>
Thanks, I'm looking forward to seeing what you come up with. :D
Stephan
@@ -1,5 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc.dtb
+dtb-$(CONFIG_ARCH_QCOM) += apq8016-sbc-d3-camera-mezzanine.dtb
dtb-$(CONFIG_ARCH_QCOM) += apq8039-t2.dtb
dtb-$(CONFIG_ARCH_QCOM) += apq8094-sony-xperia-kitakami-karin_windy.dtb
dtb-$(CONFIG_ARCH_QCOM) += apq8096-db820c.dtb
new file mode 100644
@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: BSD-3-Clause
+/*
+ * Copyright (c) 2023, Linaro Ltd.
+ */
+
+/dts-v1/;
+
+#include "apq8016-sbc.dts"
+
+&camss {
+ status = "okay";
+
+ ports {
+ port@0 {
+ reg = <0>;
+ csiphy0_ep: endpoint {
+ data-lanes = <0 2>;
+ remote-endpoint = <&ov5640_ep>;
+ status = "okay";
+ };
+ };
+ };
+};
+
+&cci {
+ status = "okay";
+};
+
+&cci_i2c0 {
+ camera_rear@3b {
+ compatible = "ovti,ov5640";
+ reg = <0x3b>;
+
+ powerdown-gpios = <&tlmm 34 GPIO_ACTIVE_HIGH>;
+ reset-gpios = <&tlmm 35 GPIO_ACTIVE_LOW>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&camera_rear_default>;
+
+ clocks = <&gcc GCC_CAMSS_MCLK0_CLK>;
+ clock-names = "xclk";
+ assigned-clocks = <&gcc GCC_CAMSS_MCLK0_CLK>;
+ assigned-clock-rates = <23880000>;
+
+ DOVDD-supply = <&camera_vdddo_1v8>;
+ AVDD-supply = <&camera_vdda_2v8>;
+ DVDD-supply = <&camera_vddd_1v5>;
+
+ port {
+ ov5640_ep: endpoint {
+ data-lanes = <1 2>;
+ remote-endpoint = <&csiphy0_ep>;
+ };
+ };
+ };
+};
@@ -250,55 +250,6 @@ &blsp_uart2 {
label = "LS-UART1";
};
-&camss {
- status = "okay";
- ports {
- port@0 {
- reg = <0>;
- csiphy0_ep: endpoint {
- data-lanes = <0 2>;
- remote-endpoint = <&ov5640_ep>;
- status = "okay";
- };
- };
- };
-};
-
-&cci {
- status = "okay";
-};
-
-&cci_i2c0 {
- camera_rear@3b {
- compatible = "ovti,ov5640";
- reg = <0x3b>;
-
- powerdown-gpios = <&tlmm 34 GPIO_ACTIVE_HIGH>;
- reset-gpios = <&tlmm 35 GPIO_ACTIVE_LOW>;
- pinctrl-names = "default";
- pinctrl-0 = <&camera_rear_default>;
-
- clocks = <&gcc GCC_CAMSS_MCLK0_CLK>;
- clock-names = "xclk";
- assigned-clocks = <&gcc GCC_CAMSS_MCLK0_CLK>;
- assigned-clock-rates = <23880000>;
-
- DOVDD-supply = <&camera_vdddo_1v8>;
- AVDD-supply = <&camera_vdda_2v8>;
- DVDD-supply = <&camera_vddd_1v5>;
-
- /* No camera mezzanine by default */
- status = "disabled";
-
- port {
- ov5640_ep: endpoint {
- data-lanes = <1 2>;
- remote-endpoint = <&csiphy0_ep>;
- };
- };
- };
-};
-
&lpass {
status = "okay";
};