[OpenWrt-Devel] [PATCH 3/3] ipq40xx: ipq4019: Add new device Compex WPJ419

[Jeff Kletsky]

On 10/30/19 4:27 AM, Daniel Danzberger wrote:

> This device contains 2 flash devices. One NOR (32M) and one NAND (128M).
> U-boot and caldata are on the NOR, the firmware on the NAND.
>
>      SoC:    IPQ4019
>      CPU:    4x 710MHz ARMv7
>      RAM:    256MB
>      FLASH:  NOR:32MB NAND:128MB
>
> [...]
>
>

  .../arch/arm/boot/dts/qcom-ipq4019-bus.dtsi   | 1142 +++++++++++++++++
  .../include/dt-bindings/msm/msm-bus-ids.h     |  869 +++++++++++++

The sudden appearance of a need the MSM bus and its IDs worries me.

With 25 devices already on the ipq40xx platform without them, it feels
like something is missing if they are needed by this one.


> diff --git a/target/linux/ipq40xx/config-4.19 b/target/linux/ipq40xx/config-4.19
> index 8948b73ff7..3ee921abed 100644
> --- a/target/linux/ipq40xx/config-4.19
> +++ b/target/linux/ipq40xx/config-4.19
> @@ -303,6 +303,9 @@ CONFIG_MTD_NAND_ECC=y
>   CONFIG_MTD_NAND_QCOM=y
>   CONFIG_MTD_SPI_NAND=y
>   CONFIG_MTD_SPI_NOR=y
> +CONFIG_MTD_SPINAND_MT29F=y
> +CONFIG_MTD_SPINAND_GIGADEVICE=y
> +CONFIG_MTD_SPINAND_ONDIEECC=y


The CONFIG_SPINAND_* additions are not required for upstream SPI-NAND


>   CONFIG_MTD_SPLIT_FIRMWARE=y
>   CONFIG_MTD_SPLIT_FIT_FW=y
>   CONFIG_MTD_UBI=y
>
> [...]
>
> diff --git a/target/linux/ipq40xx/files-4.19/arch/arm/boot/dts/qcom-ipq4019-wpj419.dts b/target/linux/ipq40xx/files-4.19/arch/arm/boot/dts/qcom-ipq4019-wpj419.dts
> new file mode 100644
> index 0000000000..5553bbd166
> --- /dev/null
> +++ b/target/linux/ipq40xx/files-4.19/arch/arm/boot/dts/qcom-ipq4019-wpj419.dts
> @@ -0,0 +1,371 @@
> +/* Copyright (c) 2015, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2019, Nguyen Dinh Phi <phi_nguyen at compex.com.sg>
> + *
> + * Permission to use, copy, modify, and/or distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + *
> + */
> +
>
> [...]
>
> +
> +		spi_0: spi at 78b5000 {
> +			pinctrl-0 = <&spi_0_pins>;
> +			pinctrl-names = "default";
> +			status = "okay";
> +			cs-gpios = <&tlmm 12 GPIO_ACTIVE_HIGH>, <&tlmm 41 GPIO_ACTIVE_HIGH>;
> +			num-cs = <2>;
> +
> +			m25p80 at 0 {
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +				reg = <0>;
> +				linux,modalias = "m25p80", "n25q128a11";
> +				compatible = "jedec,spi-nor", "n25q128a11";
> +				spi-max-frequency = <24000000>;


I don't think you need linux,modalias here, nor the chip type in the 
compatible line.
I believe that the following compatible line is sufficient

     compatible = "jedec,spi-nor";


You might also want to consider "flash at 0" or "nor at 0" or "nor_flash at 0",
or the like, rather than a chip-specific name. (I'm not a committer.)


> +
> +				partitions {
> +					compatible = "fixed-partitions";
> +
> +					partition at 0 {
> +						label = "0:SBL1";
> +						reg = <0x000000 0x040000>;
> +						read-only;
> +					};
> +
> +					partition at 40000 {
> +						label = "0:MIBIB";
> +						reg = <0x040000 0x020000>;
> +						read-only;
> +					};
> +
> +					partition at 60000 {
> +						label = "0:QSEE";
> +						reg = <0x060000 0x060000>;
> +						read-only;
> +					};
> +
> +					partition at c0000 {
> +						label = "0:CDT";
> +						reg = <0x0c0000 0x010000>;
> +						read-only;
> +					};


Someone may rip on you for capitalization of labels. (I'm not a committer.)


> +
> +					partition at d0000 {
> +						label = "0:DDRPARAMS";
> +						reg = <0x0d0000 0x010000>;
> +						read-only;
> +					};
> +
> +					partition at e0000 {
> +						label = "u-boot-env";
> +						reg = <0x0e0000 0x010000>;
> +						read-only;
> +					};


U-Boot environment may want/need to be writable


> +
> +					partition at f0000 {
> +						label = "u-boot";
> +						reg = <0x0f0000 0x080000>;
> +						read-only;
> +					};
> +
> +					partition at 170000 {
> +						label = "art";
> +						reg = <0x170000 0x010000>;
> +						read-only;
> +					};
> +				};
> +			};
> +
> +			mt29f at 1 {
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +				reg = <1>;
> +				status = "okay";
> +				compatible = "spinand,mt29f";
> +				spi-max-frequency = <24000000>;


Same comment on "mt29f" vs. something generic and descriptive.


Converting to the upstream SPI-NAND driver here should be as simple as

     compatible = "spi-nand";



> +
> +				partitions {
> +					compatible = "fixed-partitions";
> +
> +					partition at 0 {
> +						label = "ubi";
> +						reg = <0x0000000 0x8000000>;
> +					};
> +				};
> +			};
> +		};
> +
> [...]


_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel