[OpenWrt-Devel] [PATCH v2] ipq40xx: Add support for IPQ4019 ap-dk07.1-c1

Ram Chandra Jangir rjangir at codeaurora.org
Fri Sep 14 11:16:05 EDT 2018



On 9/12/2018 4:52 AM, Christian Lamparter wrote:
> On Monday, September 10, 2018 5:50:53 PM CEST Ram Chandra Jangir wrote:
>> On Friday, September 07, 2018 8:36 PM CEST Christian Lamparter wrote:
>>> On Friday, September 7, 2018 2:10:30 PM CEST Ram Chandra Jangir wrote:
>>
>>> 	3) WDOG test
>>> 	4) cpu frequency scaling
>>> Hey, that's great. Glad to hear that it is working.
>>> So, do you know of the upstream status of the cpu frequency scaling 
>>> fix
>> that OpenWrt carries:
>>> <https://patchwork.codeaurora.org/patch/473589/>
>>
>>> Because it would be a real shame if that wasn't fixed upstream too,
> right?
>>
>> yes, I just saw your above patch and as mentioned it is appearing on
> Compex
>> WPJ428 and AVM FritzBox!4040 boards.
>> Currently I don't have these boards to test locally. I may help if 
>> you
> can
>> try to reproduce this problem on any of DK01/DK04 or DK07 board.
> 
> Hah, this is good one. The Compex WPJ428 is a 3rd party dev board and 
> Compex sells it with the all-important "Based on QCA Reference Design 
> AP-DK03"
> bullet point: <http://shop.compex.com.sg/wpj428.html>. So the question
> becomes: How's this AP-DK03 different from the AP-DK01 or AP-DK04? And 
> was this "710MHz CPU Clock" a change made by Compex or Qualcomm?
> 
> I bet it has more to do with Qualcomm: Because there are more 
> alternatives, just in case  Qualcomm has ran out of the AP-DK03 
> reference design boards.
> A list of more potential boards is available over at wikidevi:
> 
> <https://wikidevi.com/w/index.php?title=Special%3AAsk&q=%3Cq%3E[[CPU1+
> mode 
> l%3A%3A~IPQ40*]]%3C%2Fq%3E&po=%3FCPU1+model%3DCPU1%0D%0A%3FCPU1+clock+
> spee
> d>
> 
> Currently, there are 7 listed boards @ 710 MHz. But there are also 
> other odd frequencies: 700 MHz, 600 MHz, 666 MHz, 632 MHz, 638 MHz?...
> What is going on?
> 
> And cpufreq-dt is wondering as well: It complains about the following 
> on
> boot:
> 
> |[    1.399981] cpufreq: cpufreq_online: CPU0: Running at unlisted freq:
> 666000 KHz
> or
>> [    1.275858] cpufreq: cpufreq_online: CPU0: Running at unlisted freq:
> 632000 KHz
> <https://lists.openwrt.org/pipermail/openwrt-devel/2018-May/012126.htm
> l>
> 
> So, there must be a way to source some of the contenders/offenders.
> And after so much time has passed since the discovery, I think it 
> would be high-time for Qualcomm to look what has caused this confusing 
> CPU speed mess and offer a proper fix to improve upon the current 
> 081-clk-fix-apss-cpu-overclocking.patch way.

If you can reproduce the same issue on DK07, please revert back. Since
DK03 is not upstreamed, I would like to defer this discussion until then.

> 
>>> Signed-off-by: Ram Chandra Jangir <rjangir at codeaurora.org> I do have 
>>> a few comments down below.
>>
>>> --- a/target/linux/ipq40xx/image/Makefile
>>> +++ b/target/linux/ipq40xx/image/Makefile
>>> @@ -207,6 +207,19 @@ define Device/qcom_ap-dk04.1-c1  endef 
>>> TARGET_DEVICES += qcom_ap-dk04.1-c1
>>>   
>>> +define Device/qcom_ap-dk07.1-c1
>>> +	$(call Device/FitImage)
>>> +	$(call Device/UbiFit)
>>> +	BOARD_NAME := ap-dk07.1-c1
>>> This is a new device. So, why do you need a legacy BOARD_NAME for it?
>>    legacy ? Normally we refer the device name as AP-DK07.1-C1 
>> internally
> too.
> I think the BOARD_NAME moniker should be left alone. Currently, it 
> serves as a way to support the early/previous installations (For 
> example for people who had the ipq806x-target based FritzBox!4040 
> image).
> But new devices don't need it. You should be fine without it.
> 
>>> +	DEVICE_DTS := qcom-ipq4019-ap.dk07.1-c1
>>> +	KERNEL_INSTALL := 1
>>> +	KERNEL_SIZE := 4096k
>>> +	BLOCKSIZE := 128k
>>> +	PAGESIZE := 2048
>>> +	DEVICE_TITLE := QCA AP-DK07.1-C1
>>> +endef
>>> +TARGET_DEVICES += qcom_ap-dk07.1-c1
>>>
>>> What's wrong with calling the device "qcom_ipq4019-ap.dk07.1-c1"?
>>     I am fine to have the complete compatible string as device name 
>> "qcom_ipq4019-ap-dk07.1-c1"  but not ok to have dts name as device 
>> name "qcom_ipq4019-ap.dk07.1-c1".
>>     Let John also comment on this.  But this may need renaming of
> existing
>> dk01 and dk04 device name too.
> I don't understand why exactly this would be a problem, though.
> Can you please explain in detail why you are "not ok to have dts name 
> as device name" there? Is it something about the "ipq4019", or is it 
> about the extra "."?
> 
>>> By the way:
>>> Also, John is preparing to drop ath10k:
>>>
>>
> <https://git.openwrt.org/?p=openwrt/staging/blogic.git;a=commit;h=f9cc
> c8f6
> 46
>> 70e3e15b160cffb19f1ef1a8519e97>
>>>
>>> | author	John Crispin <john at phrozen.org>	
>>> | Wed, 5 Sep 2018 14:51:44 +0200 (14:51 +0200)
>>> | committer	John Crispin <john at phrozen.org>	
>>> | Thu, 6 Sep 2018 19:00:21 +0200 (19:00 +0200)
>>> | commit	f9ccc8f64670e3e15b160cffb19f1ef1a8519e97
>>> |
>>> |treewide: drop ath10k support
>>> |
>>> |people should use ath10k-ct instead
>>> |
>>> |Signed-off-by: John Crispin <john at phrozen.org>
>>>
>>> So please make sure that the Wi-Fis are indeed working with ath10k-ct.
>>
>> I think john's change is independent from AP-DK07.1-C1 board support 
>> and both changes can be merged independently.
>> I will test with ath10k-ct too once available.
> Depends. If your patch goes through before ath10k-ct is the new 
> default, then "Sure, you are fine". If the "switch to ath10k-ct" is 
> faster: you have been notifed and please verify it's working.
> 
>>> +From: Sricharan R <sricharan at codeaurora.org>
>>> +Date: Fri, 25 May 2018 11:41:17 +0530
>>> +Subject: [PATCH] ARM: dts: ipq4019: Add ipq4019-ap.dk07.1 common 
>>> +data
>>> +
>>> +Add the common data for all dk07 based boards.
>>> +
>>> +Reviewed-by: Abhishek Sahu <absahu at codeaurora.org>
>>> +Signed-off-by: Sricharan R <sricharan at codeaurora.org>
>>> +Signed-off-by: Andy Gross <andy.gross at linaro.org>
>>> +---
>>> + arch/arm/boot/dts/qcom-ipq4019-ap.dk07.1.dtsi | 75
>>> ++++++++++++++++++++++++++++
>>> + 1 file changed, 75 insertions(+)
>>> + create mode 100644 arch/arm/boot/dts/qcom-ipq4019-ap.dk07.1.dtsi
>>> +
>>> +diff --git a/arch/arm/boot/dts/qcom-ipq4019-ap.dk07.1.dtsi
>>> +b/arch/arm/boot/dts/qcom-ipq4019-ap.dk07.1.dtsi
>>> +new file mode 100644
>>> +index 0000000..9f1a5a66
>>> +--- /dev/null
>>> ++++ b/arch/arm/boot/dts/qcom-ipq4019-ap.dk07.1.dtsi
>>
>>> I guess you'll need to push those changes upstream.
>>    It looks like you missed to read my commit message where I have
> clearly
>> mentioned that
>>    "The ap-dk07.1-c1 board is already available in upstream linux v4.17."
> 
>>    Please see below patches for your reference:
>>    https://patchwork.kernel.org/patch/10426423/
>>    https://patchwork.kernel.org/patch/10426401/
> 
> And exactly for these files have some comments and questions... So, I 
> have added inline comments in order to have the necessary context and 
> noted that these should be fixed in linux upstream as well. Ok? Great!
> 
> as for:
>> Please feel free to send your reviews changes to upstream linux, I 
>> will check with my internal team too on incorporating these reviews/changes.
> 
> This "ipq40xx: Add support for IPQ4019 ap-dk07.1-c1" mail is yours, 
> isn't it?
> And this is upstream openwrt, right? Is upstream openwrt somehow not 
> as good or important as upstream linux? So, why is there an extra step 
> involved here? Obviously, you are doing this as an Qualcomm employee 
> and not on your free time, since you are adding a image for a 
> reference board that can't be purchased/aquired like the mass-produced 
> customer products that are "based" on them.
> 
> And from my point of view: Reference boards should be held to a higher 
> standard, since many devs and hobbiest will use them as the "reference".
> So, I really like them to be spotless and free of errors, oddities and 
> something that can cause misunderstandings. Because any "funny stuff"
> there has a higher chance to turn up again and again and again and again.
> 
> With that out of the way: you have my previous mails and you have my 
> permission to go all out and forward it to the correct destination, 
> since you get paid for this work. Now, I tried doing this myself in 
> the past - you can find my patches on linux-msm-arm. - But I have to 
> say that I was mostly met with denial or silence.
> So, you have to understand: It has come to this, that this way/method 
> of bugging you here has become more efficient than the alternatives. 
> If there is a better way that will cause less friction and actually 
> lead to faster and better results: please let us know.
> 
>>> +@@ -0,0 +1,75 @@
>>> ++// SPDX-License-Identifier: GPL-2.0 // Copyright (c) 2018, The 
>>> ++Linux Foundation. All rights reserved.
>>> ++
>>> ++#include "qcom-ipq4019.dtsi"
>>> ++#include <dt-bindings/input/input.h> #include 
>>> ++<dt-bindings/gpio/gpio.h>
>>> I noticed that you included input.h put none of these reference 
>>> boards
>> actually define any gpio-keys.
>>>
>>> So, what's the reason here? Are these DTS actually for real "boards"
> or
>> are they just for show? (Also related: why is there no system/power 
>> LED
>>> defined?)
>>   This patch is just a copy of already submitted patch in upstream 
>> kernel https://patchwork.kernel.org/patch/10426423/  to ensure that 
>> once we Upgrade to linux v4.17 or higher, then it can be dropped smoothly.
>> Please feel free to send your reviews changes to upstream linux, I 
>> will check with my internal team too on incorporating these reviews/changes.
> 
> Yes, please forward it there.
> 
> Thanks.
>   
>>> ++/ {
>>> ++	model = "Qualcomm Technologies, Inc. IPQ4019/AP-DK07.1";
>>> ++
>>> ++	memory {
>>> ++		device_type = "memory";
>>> ++		reg = <0x80000000 0x20000000>; /* 512MB */
>>> ++	};
>>> ++
>>> ++	aliases {
>>> ++		serial0 = &blsp1_uart1;
>>> ++		serial1 = &blsp1_uart2;
> (The alias node and chosen node was removed later in a separate
> patch.)
> 
> and serial1 was never enabled. Can it be removed or not?
> 
>>> ++	};
>>> ++ [...]
>>> ++
>>> ++	soc {
>>> ++		pinctrl at 1000000 {
>>> [...]
>>> ++
>>> ++			nand_pins: nand-pins {
>>> ++				pins = "gpio53", "gpio55", "gpio56",
>>> ++						"gpio57", "gpio58",
> "gpio59",
>>> ++				       "gpio60", "gpio62", "gpio63",
>>> ++						"gpio64", "gpio65",
> "gpio66",
>>> ++				       "gpio67", "gpio68", "gpio69";
>>> ++				function = "qpic";
>>> ++                        };
>>> ++		};
>>> bias-pull-ups and bias-pull-downs are missing here.
>>> Also what happend to gpio52? The "qcom-ipq4019-ap.dk04.1.dtsi" still 
>>> defines it and from what I can tell, there seems to be only one 
>>> valid
> way to
>>> wire up a nand chip with the SoC.
>>
>>     nand pins pull-up/pull-down will be taken care by uboot.
> Do you have the u-boot code somewhere? The copy we got:
> <https://github.com/riptidewave93/meraki-uboot/blob/mr33-20170427/boar
> d/qc om/ipq40xx_cdp/ipq40xx_board_param.h#L186>
> specifies that gpio52 (that is missing above) is somehow needed for 
> MACH_TYPE_IPQ40XX_AP_DK07_1_C1 device there.
> <https://github.com/riptidewave93/meraki-uboot/blob/mr33-20170427/boar
> d/qc om/ipq40xx_cdp/ipq40xx_board_param.h#L1158>
> 
> I don't necessarily trust u-boot there, since we had issues with it in 
> the past with the Cisco Meraki MR33.
> 
>> I have not
>> added this in kernel yet, i will test and add one by one remaining 
>> patches/features too.
> That's good to hear.
> 
>>> ++
>>> ++		dma at 7884000 {
>>> ++			status = "ok";
>>> ++		};
>>> [...]
>>> +diff --git a/arch/arm/boot/dts/qcom-ipq4019-ap.dk07.1-c1.dts
>>> +b/arch/arm/boot/dts/qcom-ipq4019-ap.dk07.1-c1.dts
>>> +new file mode 100644
>>> +index 0000000..8c7ef65
>>> +--- /dev/null
>>> ++++ b/arch/arm/boot/dts/qcom-ipq4019-ap.dk07.1-c1.dts
>>
>>> Same as with the .dtsi: please push those changes upstream.
>>    This is copy of upstream linux patch, please see my above comments.
>>
> As with the qcom-ipq4019-ap.dk07.1.dtsi: See my comment above. Thanks.
> 
>>> +@@ -0,0 +1,64 @@
>>> ++// SPDX-License-Identifier: GPL-2.0 // Copyright (c) 2018, The 
>>> ++Linux Foundation. All rights reserved.
>>> ++
>>> ++#include "qcom-ipq4019-ap.dk07.1.dtsi"
>>> ++
>>> ++/ {
>>> ++	model = "Qualcomm Technologies, Inc. IPQ4019/AP-DK07.1-C1";
>>> ++	compatible = "qcom,ipq4019-ap-dk07.1-c1";
>>> Make that:
>>
>>> compatible = "qcom,ipq4019-ap-dk07.1-c1", "qcom,ipq4019";
>>     As I am saying this is copy of upstream patch,  please refer 
>> below
> patch
>> 905-dts-qcom-ipq4019-ap.dk07.1-enable-nodes.patch
>>     which already does the similar.
> Similar?
> 
> Let's look at 905-dts-qcom-ipq4019-ap.dk07.1-enable-nodes.patch from 
> your previous mail:
> 
> |+++
> b/target/linux/ipq40xx/patches-4.14/905-dts-qcom-ipq4019-ap.dk07.1-ena
> ble-
> nodes.patch
> |@@ -0,0 +1,79 @@
> |+--- a/arch/arm/boot/dts/qcom-ipq4019-ap.dk07.1-c1.dts
> |++++ b/arch/arm/boot/dts/qcom-ipq4019-ap.dk07.1-c1.dts
> |+@@ -5,7 +5,7 @@
> |+
> |+ / {
> |+       model = "Qualcomm Technologies, Inc. IPQ4019/AP-DK07.1-C1";
> |+-      compatible = "qcom,ipq4019-ap-dk07.1-c1";
> |++      compatible = "qcom,ap-dk07.1-c1", "qcom,ipq4019";
> 
> It changed the compatible from the upstream 
> "qcom,ipq4019-ap-dk07.1-c1" to
> 
> "qcom,ap-dk07.1-c1" for no reason. Why exactly was this change made?
> 
> (The change to add the "qcom,ipq4019" is fine though: +1.
> And I understand that it can dropped for the next LTS/v4.19 kernel)
OK

> 
>>> ++
>>> ++	soc {
>>> ++		pci at 40000000 {
>>> ++			status = "ok";
>>> ++			perst-gpio = <&tlmm 38 0x1>;
>>> ++		};
>>> The phandle's 3'rd parameter should be GPIO_ACTIVE_LOW and not 0x1.
> 
> Great, I'll be looking forward to having this changed.
>>> ++
>>> ++		spi at 78b6000 {
>>> [...]
>>> ++
>>> ++			m25p80 at 0 {
>>> ++				#address-cells = <1>;
>>> ++				#size-cells = <1>;
>>> ++				reg = <0>;
>>> ++				compatible = "n25q128a11";
>>>   I think "jedec,spi-nor" is missing here. It must be included for 
>>> any
> SPI
>> NOR flash that supports the JEDE READ ID Opcode.
>>   This is copy of upstream linux patch, please see my above comments.
>>
> In case you are not aware: That "jedec,spi-nor" is a "must" 
> requirement of the jedec,spi-nor.txt binding file in upstream:
> <https://www.kernel.org/doc/Documentation/devicetree/bindings/mtd/jede
> c%2C
> spi-nor.txt>
> |               Must also include "jedec,spi-nor" for any SPI NOR 
> | flash
> that can
> |               be identified by the JEDEC READ ID opcode (0x9F).
> 
> 
> The n25q128a series supports that JEDEC READ ID opcode as per Table 
> 16: Command Set:
> <http://www.micron.com/~/media/documents/products/data-sheet/nor-flash
> /ser
> ial-nor/n25q/n25q_128mb_3v_65nm.pdf>
> 
>>> +--- a/arch/arm/boot/dts/qcom-ipq4019-ap.dk07.1.dtsi
>>> ++++ b/arch/arm/boot/dts/qcom-ipq4019-ap.dk07.1.dtsi
>>> +@@ -67,9 +71,49 @@
>>> + 		};
>>> +
>>> + 		qpic-nand at 79b0000 {
>>> ++			compatible = "qcom,ipq4019-nand", "qcom,msm-nand";
>>> + 			pinctrl-0 = <&nand_pins>;
>>> + 			pinctrl-names = "default";
>>> + 			status = "ok";
>>> ++
>>> ++			nand at 0 {
>>> ++				compatible = "qcom,nandcs";
>>> ++				reg = <0>;
>>> ++				#address-cells = <1>;
>>> ++				#size-cells = <1>;
>>> ++
>>> ++				nand-ecc-strength = <4>;
>>> ++				nand-ecc-step-size = <512>;
>>> ++				nand-bus-width = <8>;
>>> Is u-boot filling in the partitions here?
>>   Yes, u-boot will fill the SMEM partitions details.
> 
> OK.
> 
> (there's also an smem-partition parser upstream.
> I think it's located in drivers/soc/qcom/.
> But if u-boot does its job: this is fine.)
> 
>>> ++			};
>>> ++		};
>>> ++
>>> ++		mdio at 90000 {
>>> ++			status = "okay";
>>> ++		};
>>> ++
>>> ++		ess-switch at c000000 {
>>> ++				status = "okay";
>>> ++				switch_mac_mode = <0x0>; /* mac mode for
>> RGMII RMII */
>>> The comment (RGMII/RMII mode) is either wrong or the switch_mac_mode
> value
>> is misleading.
> 
> No answer?
> 
> Is the RGMII RMII comment wrong? Or is the 0x0 ( = PSGMII) value bad?
> 
>>   [...]
>>
>>> OT: A user reported a kernel panic with the IPQ4019 ap-dk01.1-c1 on
> the
>> forum: 
>> <https://forum.openwrt.org/t/ipq4018-dk01-1-kernel-panic/20060>
>>> I wonder if this affects the  ap-dk04.1-c1 and  ap-dk07.1-c1 as well.
>>> Can you please comment on that.
>>    We can take this issue offline, Still I am not done with 
>> sysupgrade, envtools and other features support/testing yet.
>>    I will look in to and will check what is wrong during sysupgrade 
>> and
> will
>> udate in the actual forum:
>> <https://forum.openwrt.org/t/ipq4018-dk01-1-kernel-panic/20060> .
> 
> I think the combined nand-and-nor sysupgrade.bin will be a real pain 
> to deal with the existing sysupgrade logic. It might be easier to make 
> two separate "devices". One will be the nand-only version while the 
> other will get the nor-only versions of the ipq4019-ap-dk07.1-c1.dts 
> (and of course each a different compatible or model). This should make 
> it easy to hook the two versions into the existing sysupgrade code and 
> it would make the reference board a "good example" of "how to do 
> it"(tm).
> 
I have conveyed to DK01 team and they will look in to it and will update the ticket.
There is no single sysupgrade image for nor & nand. For nor board, it is openwrt-ipq40xx-qcom_ap-dk07.1-c1-squashfs-sysupgrade.bin wheres for nand board it is openwrt-ipq40xx-qcom_ap-dk07.1-c1-squashfs-nand-sysupgrade.bin.

Thanks,
Ram

> (The only sore spot will be the switching between the nand or nor version.
> But this could be done by flashing the initial [nand|nor]-only version 
> with u-boot. From what I can tell, the nor / nand switching feature is 
> only ever relevant for development and prototype boards. And 
> thankfully, the mass-produced  consumer boards will just stick to one 
> of the options for the full production run or at the very atleast a 
> major revision.)
> 
> Regards,
> Christian
> 
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel at lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
> 



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



More information about the openwrt-devel mailing list