From mboxrd@z Thu Jan 1 00:00:00 1970 From: marcel.ziswiler@toradex.com (Marcel Ziswiler) Date: Wed, 6 Jan 2016 09:15:18 +0000 Subject: [PATCH v2 1/2] ARM: dts: imx6: Add support for Toradex Apalis iMX6Q/D SoM In-Reply-To: <20160106080358.GD9030@ibawizard.net> References: <1452011942-11940-1-git-send-email-marcel.ziswiler@toradex.com> <1452011942-11940-2-git-send-email-marcel.ziswiler@toradex.com> <20160106080358.GD9030@ibawizard.net> Message-ID: <1452071716.30372.61.camel@toradex.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Petr On Wed, 2016-01-06 at 09:03 +0100, Petr ?tetiar wrote: > Marcel Ziswiler [2016-01-05 17:39:01]: > > Hi Marcel, > > thanks for taking care of this, I'm quite busy with other tasks :( You're welcome. I know that feeling. > > - integrated review feedback from Lucas > > You've probably missed few of them :) See my nitpicks bellow. I don't think so but we don't necessarily agree with all of Lucas' findings plus the discussion about some of the stuff hasn't really concluded. > > - left and even added some more comments as I don't see why putting > > any > > ? explanatory comments in dts' should be such a bad thing to do > You've marked this as v2 Well, as Stefan pointed out we at Toradex are/were working on this as well and we discussed whether we should just ignore whatever you have done or not done and post our own stuff but decided to rather join efforts now as you already jumped ahead. > so I think, that you should only work on the > feedback. Ideally you shouldn't add any new stuff if it wasn't > requested > otherwise you're wasting reviewer time. I don't think so. What is important is that any changes are clearly declared which we did. > > - completely got rid of the memory node as that is something > > typically filled > > ? in by the boot loader e.g. U-Boot > > If I'm not mistaken, it wasn't requested by the reviewer. Lucas was actually nitpicking about the location thereof and rather than moving it we decided to just get rid of it as it does not only not add any value but is simply wrong on most of the modules featuring different amount of memory. > > - without the regulators simple-bus it no longer boots > > It works for me on 4.4.0-rc3 with following DTS[1]. BTW, this DTS is > my > preparation for v2 patch series. I only tried with -next stuff so it might be something is just broken there I guess. > > - fixed Ethernet PHY reset & interrupt (requires Micrel PHY driver > > to be > > ? enabled) > > Great! Yeah, that one took us a day to figure it all out. At the end we actually stumbled over your PCIe reset patch inverting the meaning of active-low vs. active-high (;-p). > > + * This file is dual-licensed: you can use it either under the > > terms > > + * of the GPL or the X11 license, at your option. Note that this > > dual > > + * licensing only applies to this file, and not this project as a > > + * whole. > > Hope you've some kind of ACK from FSL and Linaro :) Well, all new DTS' are now dual-licensed including several i.MX 6 related ones so we are not doing anything special there really. > > + reg_usb_otg_vbus: usb_otg_vbus { > > + compatible = "regulator-fixed"; > > + pinctrl-names = "default"; > > + pinctrl-0 = > > <&pinctrl_regulator_usbotg_pwr>; > > + regulator-name = "usb_otg_vbus"; > > + regulator-min-microvolt = <5000000>; > > + regulator-max-microvolt = <5000000>; > > + gpio = <&gpio3 22 GPIO_ACTIVE_HIGH>; > > + enable-active-high; > > + status = "disabled"; > > + }; > > I'm not sure, but it seems to me, that we should move this regulator > into the > carrier board DTS. On our custom carrier board we've this regulator > always on > and we use this GPIO for heartbeat LED, so I've this in our carrier > board > DTS[2]: Yes, you are absolutely right and e.g. on Apalis T30 [1] that is exactly how we did it. > reg_usb_otg_vbus: usb_otg_vbus { > /* Regulator is always on, and we use GPIO for > heartbeat LED */ > pinctrl-0 = <>; > gpio = <>; > > compatible = "regulator-fixed"; > regulator-name = "usb_otg_vbus"; > regulator-min-microvolt = <5000000>; > regulator-max-microvolt = <5000000>; > regulator-always-on; > }; > > ... > > usbotg { > /* GPIO3_IO22 is Heartbeat LED */ > pinctrl_regulator_usbotg_pwr: gpio_regulator_usbotg_pwr > { > }; > }; > > > +/* PAD Ctrl values for common settings */ > > +/* > > + * (PAD_CTL_HYS | PAD_CTL_PUS_100K_UP | PAD_CTL_PUE | PAD_CTL_PKE > > | > > + *??PAD_CTL_SPEED_MED | PAD_CTL_DSE_40ohm) > > + */ > > +#define PAD_CTRL_HYS_PU 0x1b0b0 > > This was requested to be reworked. I've simply replaced all the > macros with > hex values. I don't think the reviewers really concluded on the action to be taken on here and just using hex values is probably the most stupid solution. > > + pinctrl_usdhc3_100mhz: usdhc3grp-100mhz { /* > > 100Mhz */ > > + fsl,pins = < > > + MX6QDL_PAD_SD3_CMD__SD3_CMD > > 0x170B9 > > As per review comments, all hex values should be lowercase. Yes, we probably missed that one. > 1. https://github.com/ynezz/linux-2.6/blob/b27de46e67605fe1a8e386b065 > 845a9708e8e792/arch/arm/boot/dts/imx6qdl-apalis.dtsi > 2. https://github.com/ynezz/linux-2.6/blob/ec678e3734d9fff35f7ba96054 > 69a68a3e424020/arch/arm/boot/dts/imx6qdl-apalis-gaben- > flexisbc.dtsi#L143 > > Thanks! Thank you. > -- ynezz [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/tegra30-apalis-eval.dts#n235 Cheers Marcel From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcel Ziswiler Subject: Re: [PATCH v2 1/2] ARM: dts: imx6: Add support for Toradex Apalis iMX6Q/D SoM Date: Wed, 6 Jan 2016 09:15:18 +0000 Message-ID: <1452071716.30372.61.camel@toradex.com> References: <1452011942-11940-1-git-send-email-marcel.ziswiler@toradex.com> <1452011942-11940-2-git-send-email-marcel.ziswiler@toradex.com> <20160106080358.GD9030@ibawizard.net> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20160106080358.GD9030@ibawizard.net> Content-Language: en-US Content-ID: <879E344D5F3ED74282E933D6CCD4D359@eurprd05.prod.outlook.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: "ynezz@true.cz" Cc: "mark.rutland@arm.com" , "devicetree@vger.kernel.org" , "stillcompiling@gmail.com" , "linux@arm.linux.org.uk" , "pawel.moll@arm.com" , "ijc+devicetree@hellion.org.uk" , "shawnguo@kernel.org" , "linux-kernel@vger.kernel.org" , "stefan@agner.ch" , "robh+dt@kernel.org" , "kernel@pengutronix.de" , "galak@codeaurora.org" , "shawn.guo@linaro.org" , "festevam@gmail.com" , "linux-arm-kernel@lists.infradead.org" , "l.stach@pengutronix.de" List-Id: devicetree@vger.kernel.org SGkgUGV0cg0KDQpPbiBXZWQsIDIwMTYtMDEtMDYgYXQgMDk6MDMgKzAxMDAsIFBldHIgxaB0ZXRp YXIgd3JvdGU6DQo+IE1hcmNlbCBaaXN3aWxlciA8bWFyY2VsLnppc3dpbGVyQHRvcmFkZXguY29t PiBbMjAxNi0wMS0wNSAxNzozOTowMV06DQo+IA0KPiBIaSBNYXJjZWwsDQo+IA0KPiB0aGFua3Mg Zm9yIHRha2luZyBjYXJlIG9mIHRoaXMsIEknbSBxdWl0ZSBidXN5IHdpdGggb3RoZXIgdGFza3Mg OigNCg0KWW91J3JlIHdlbGNvbWUuIEkga25vdyB0aGF0IGZlZWxpbmcuDQoNCj4gPiAtIGludGVn cmF0ZWQgcmV2aWV3IGZlZWRiYWNrIGZyb20gTHVjYXMNCj4gDQo+IFlvdSd2ZSBwcm9iYWJseSBt aXNzZWQgZmV3IG9mIHRoZW0gOikgU2VlIG15IG5pdHBpY2tzIGJlbGxvdy4NCg0KSSBkb24ndCB0 aGluayBzbyBidXQgd2UgZG9uJ3QgbmVjZXNzYXJpbHkgYWdyZWUgd2l0aCBhbGwgb2YgTHVjYXMn DQpmaW5kaW5ncyBwbHVzIHRoZSBkaXNjdXNzaW9uIGFib3V0IHNvbWUgb2YgdGhlIHN0dWZmIGhh c24ndCByZWFsbHkNCmNvbmNsdWRlZC4NCg0KPiA+IC0gbGVmdCBhbmQgZXZlbiBhZGRlZCBzb21l IG1vcmUgY29tbWVudHMgYXMgSSBkb24ndCBzZWUgd2h5IHB1dHRpbmcNCj4gPiBhbnkNCj4gPiDC oCBleHBsYW5hdG9yeSBjb21tZW50cyBpbiBkdHMnIHNob3VsZCBiZSBzdWNoIGEgYmFkIHRoaW5n IHRvIGRvDQoNCj4gWW91J3ZlIG1hcmtlZCB0aGlzIGFzIHYyDQoNCldlbGwsIGFzIFN0ZWZhbiBw b2ludGVkIG91dCB3ZSBhdCBUb3JhZGV4IGFyZS93ZXJlIHdvcmtpbmcgb24gdGhpcyBhcw0Kd2Vs bCBhbmQgd2UgZGlzY3Vzc2VkIHdoZXRoZXIgd2Ugc2hvdWxkIGp1c3QgaWdub3JlIHdoYXRldmVy IHlvdSBoYXZlDQpkb25lIG9yIG5vdCBkb25lIGFuZCBwb3N0IG91ciBvd24gc3R1ZmYgYnV0IGRl Y2lkZWQgdG8gcmF0aGVyIGpvaW4NCmVmZm9ydHMgbm93IGFzIHlvdSBhbHJlYWR5IGp1bXBlZCBh aGVhZC4NCg0KPiAgc28gSSB0aGluaywgdGhhdCB5b3Ugc2hvdWxkIG9ubHkgd29yayBvbiB0aGUN Cj4gZmVlZGJhY2suIElkZWFsbHkgeW91IHNob3VsZG4ndCBhZGQgYW55IG5ldyBzdHVmZiBpZiBp dCB3YXNuJ3QNCj4gcmVxdWVzdGVkDQo+IG90aGVyd2lzZSB5b3UncmUgd2FzdGluZyByZXZpZXdl ciB0aW1lLg0KDQpJIGRvbid0IHRoaW5rIHNvLiBXaGF0IGlzIGltcG9ydGFudCBpcyB0aGF0IGFu eSBjaGFuZ2VzIGFyZSBjbGVhcmx5DQpkZWNsYXJlZCB3aGljaCB3ZSBkaWQuDQoNCj4gPiAtIGNv bXBsZXRlbHkgZ290IHJpZCBvZiB0aGUgbWVtb3J5IG5vZGUgYXMgdGhhdCBpcyBzb21ldGhpbmcN Cj4gPiB0eXBpY2FsbHkgZmlsbGVkDQo+ID4gwqAgaW4gYnkgdGhlIGJvb3QgbG9hZGVyIGUuZy4g VS1Cb290DQo+IA0KPiBJZiBJJ20gbm90IG1pc3Rha2VuLCBpdCB3YXNuJ3QgcmVxdWVzdGVkIGJ5 IHRoZSByZXZpZXdlci4NCg0KTHVjYXMgd2FzIGFjdHVhbGx5IG5pdHBpY2tpbmcgYWJvdXQgdGhl IGxvY2F0aW9uIHRoZXJlb2YgYW5kIHJhdGhlcg0KdGhhbiBtb3ZpbmcgaXQgd2UgZGVjaWRlZCB0 byBqdXN0IGdldCByaWQgb2YgaXQgYXMgaXQgZG9lcyBub3Qgb25seSBub3QNCmFkZCBhbnkgdmFs dWUgYnV0IGlzIHNpbXBseSB3cm9uZyBvbiBtb3N0IG9mIHRoZSBtb2R1bGVzIGZlYXR1cmluZw0K ZGlmZmVyZW50IGFtb3VudCBvZiBtZW1vcnkuDQoNCj4gPiAtIHdpdGhvdXQgdGhlIHJlZ3VsYXRv cnMgc2ltcGxlLWJ1cyBpdCBubyBsb25nZXIgYm9vdHMNCj4gDQo+IEl0IHdvcmtzIGZvciBtZSBv biA0LjQuMC1yYzMgd2l0aCBmb2xsb3dpbmcgRFRTWzFdLiBCVFcsIHRoaXMgRFRTIGlzDQo+IG15 DQo+IHByZXBhcmF0aW9uIGZvciB2MiBwYXRjaCBzZXJpZXMuDQoNCkkgb25seSB0cmllZCB3aXRo IC1uZXh0IHN0dWZmIHNvIGl0IG1pZ2h0IGJlIHNvbWV0aGluZyBpcyBqdXN0IGJyb2tlbg0KdGhl cmUgSSBndWVzcy4NCg0KPiA+IC0gZml4ZWQgRXRoZXJuZXQgUEhZIHJlc2V0ICYgaW50ZXJydXB0 IChyZXF1aXJlcyBNaWNyZWwgUEhZIGRyaXZlcg0KPiA+IHRvIGJlDQo+ID4gwqAgZW5hYmxlZCkN Cj4gDQo+IEdyZWF0IQ0KDQpZZWFoLCB0aGF0IG9uZSB0b29rIHVzIGEgZGF5IHRvIGZpZ3VyZSBp dCBhbGwgb3V0LiBBdCB0aGUgZW5kIHdlDQphY3R1YWxseSBzdHVtYmxlZCBvdmVyIHlvdXIgUENJ ZSByZXNldCBwYXRjaCBpbnZlcnRpbmcgdGhlIG1lYW5pbmcgb2YNCmFjdGl2ZS1sb3cgdnMuIGFj dGl2ZS1oaWdoICg7LXApLg0KDQo+ID4gKyAqIFRoaXMgZmlsZSBpcyBkdWFsLWxpY2Vuc2VkOiB5 b3UgY2FuIHVzZSBpdCBlaXRoZXIgdW5kZXIgdGhlDQo+ID4gdGVybXMNCj4gPiArICogb2YgdGhl IEdQTCBvciB0aGUgWDExIGxpY2Vuc2UsIGF0IHlvdXIgb3B0aW9uLiBOb3RlIHRoYXQgdGhpcw0K PiA+IGR1YWwNCj4gPiArICogbGljZW5zaW5nIG9ubHkgYXBwbGllcyB0byB0aGlzIGZpbGUsIGFu ZCBub3QgdGhpcyBwcm9qZWN0IGFzIGENCj4gPiArICogd2hvbGUuDQo+IA0KPiBIb3BlIHlvdSd2 ZSBzb21lIGtpbmQgb2YgQUNLIGZyb20gRlNMIGFuZCBMaW5hcm8gOikNCg0KV2VsbCwgYWxsIG5l dyBEVFMnIGFyZSBub3cgZHVhbC1saWNlbnNlZCBpbmNsdWRpbmcgc2V2ZXJhbCBpLk1YIDYNCnJl bGF0ZWQgb25lcyBzbyB3ZSBhcmUgbm90IGRvaW5nIGFueXRoaW5nIHNwZWNpYWwgdGhlcmUgcmVh bGx5Lg0KDQo+ID4gKwkJcmVnX3VzYl9vdGdfdmJ1czogdXNiX290Z192YnVzIHsNCj4gPiArCQkJ Y29tcGF0aWJsZSA9ICJyZWd1bGF0b3ItZml4ZWQiOw0KPiA+ICsJCQlwaW5jdHJsLW5hbWVzID0g ImRlZmF1bHQiOw0KPiA+ICsJCQlwaW5jdHJsLTAgPQ0KPiA+IDwmcGluY3RybF9yZWd1bGF0b3Jf dXNib3RnX3B3cj47DQo+ID4gKwkJCXJlZ3VsYXRvci1uYW1lID0gInVzYl9vdGdfdmJ1cyI7DQo+ ID4gKwkJCXJlZ3VsYXRvci1taW4tbWljcm92b2x0ID0gPDUwMDAwMDA+Ow0KPiA+ICsJCQlyZWd1 bGF0b3ItbWF4LW1pY3Jvdm9sdCA9IDw1MDAwMDAwPjsNCj4gPiArCQkJZ3BpbyA9IDwmZ3BpbzMg MjIgR1BJT19BQ1RJVkVfSElHSD47DQo+ID4gKwkJCWVuYWJsZS1hY3RpdmUtaGlnaDsNCj4gPiAr CQkJc3RhdHVzID0gImRpc2FibGVkIjsNCj4gPiArCQl9Ow0KPiANCj4gSSdtIG5vdCBzdXJlLCBi dXQgaXQgc2VlbXMgdG8gbWUsIHRoYXQgd2Ugc2hvdWxkIG1vdmUgdGhpcyByZWd1bGF0b3INCj4g aW50byB0aGUNCj4gY2FycmllciBib2FyZCBEVFMuIE9uIG91ciBjdXN0b20gY2FycmllciBib2Fy ZCB3ZSd2ZSB0aGlzIHJlZ3VsYXRvcg0KPiBhbHdheXMgb24NCj4gYW5kIHdlIHVzZSB0aGlzIEdQ SU8gZm9yIGhlYXJ0YmVhdCBMRUQsIHNvIEkndmUgdGhpcyBpbiBvdXIgY2Fycmllcg0KPiBib2Fy ZA0KPiBEVFNbMl06DQoNClllcywgeW91IGFyZSBhYnNvbHV0ZWx5IHJpZ2h0IGFuZCBlLmcuIG9u IEFwYWxpcyBUMzAgWzFdIHRoYXQgaXMNCmV4YWN0bHkgaG93IHdlIGRpZCBpdC4NCg0KPiAJcmVn X3VzYl9vdGdfdmJ1czogdXNiX290Z192YnVzIHsNCj4gCQkvKiBSZWd1bGF0b3IgaXMgYWx3YXlz IG9uLCBhbmQgd2UgdXNlIEdQSU8gZm9yDQo+IGhlYXJ0YmVhdCBMRUQgKi8NCj4gCQlwaW5jdHJs LTAgPSA8PjsNCj4gCQlncGlvID0gPD47DQo+IA0KPiAJCWNvbXBhdGlibGUgPSAicmVndWxhdG9y LWZpeGVkIjsNCj4gCQlyZWd1bGF0b3ItbmFtZSA9ICJ1c2Jfb3RnX3ZidXMiOw0KPiAJCXJlZ3Vs YXRvci1taW4tbWljcm92b2x0ID0gPDUwMDAwMDA+Ow0KPiAJCXJlZ3VsYXRvci1tYXgtbWljcm92 b2x0ID0gPDUwMDAwMDA+Ow0KPiAJCXJlZ3VsYXRvci1hbHdheXMtb247DQo+IAl9Ow0KPiANCj4g CS4uLg0KPiANCj4gCXVzYm90ZyB7DQo+IAkJLyogR1BJTzNfSU8yMiBpcyBIZWFydGJlYXQgTEVE ICovDQo+IAkJcGluY3RybF9yZWd1bGF0b3JfdXNib3RnX3B3cjogZ3Bpb19yZWd1bGF0b3JfdXNi b3RnX3B3cg0KPiB7DQo+IAkJfTsNCj4gCX07DQo+IA0KPiA+ICsvKiBQQUQgQ3RybCB2YWx1ZXMg Zm9yIGNvbW1vbiBzZXR0aW5ncyAqLw0KPiA+ICsvKg0KPiA+ICsgKiAoUEFEX0NUTF9IWVMgfCBQ QURfQ1RMX1BVU18xMDBLX1VQIHwgUEFEX0NUTF9QVUUgfCBQQURfQ1RMX1BLRQ0KPiA+IHwNCj4g PiArICrCoMKgUEFEX0NUTF9TUEVFRF9NRUQgfCBQQURfQ1RMX0RTRV80MG9obSkNCj4gPiArICov DQo+ID4gKyNkZWZpbmUgUEFEX0NUUkxfSFlTX1BVIDB4MWIwYjANCj4gDQo+IFRoaXMgd2FzIHJl cXVlc3RlZCB0byBiZSByZXdvcmtlZC4gSSd2ZSBzaW1wbHkgcmVwbGFjZWQgYWxsIHRoZQ0KPiBt YWNyb3Mgd2l0aA0KPiBoZXggdmFsdWVzLg0KDQpJIGRvbid0IHRoaW5rIHRoZSByZXZpZXdlcnMg cmVhbGx5IGNvbmNsdWRlZCBvbiB0aGUgYWN0aW9uIHRvIGJlIHRha2VuDQpvbiBoZXJlIGFuZCBq dXN0IHVzaW5nIGhleCB2YWx1ZXMgaXMgcHJvYmFibHkgdGhlIG1vc3Qgc3R1cGlkIHNvbHV0aW9u Lg0KDQo+ID4gKwkJcGluY3RybF91c2RoYzNfMTAwbWh6OiB1c2RoYzNncnAtMTAwbWh6IHsgLyoN Cj4gPiAxMDBNaHogKi8NCj4gPiArCQkJZnNsLHBpbnMgPSA8DQo+ID4gKwkJCQlNWDZRRExfUEFE X1NEM19DTURfX1NEM19DTUQNCj4gPiAweDE3MEI5DQo+IA0KPiBBcyBwZXIgcmV2aWV3IGNvbW1l bnRzLCBhbGwgaGV4IHZhbHVlcyBzaG91bGQgYmUgbG93ZXJjYXNlLg0KDQpZZXMsIHdlIHByb2Jh Ymx5IG1pc3NlZCB0aGF0IG9uZS4NCg0KPiAxLiBodHRwczovL2dpdGh1Yi5jb20veW5lenovbGlu dXgtMi42L2Jsb2IvYjI3ZGU0NmU2NzYwNWZlMWE4ZTM4NmIwNjUNCj4gODQ1YTk3MDhlOGU3OTIv YXJjaC9hcm0vYm9vdC9kdHMvaW14NnFkbC1hcGFsaXMuZHRzaQ0KPiAyLiBodHRwczovL2dpdGh1 Yi5jb20veW5lenovbGludXgtMi42L2Jsb2IvZWM2NzhlMzczNGQ5ZmZmMzVmN2JhOTYwNTQNCj4g NjlhNjhhM2U0MjQwMjAvYXJjaC9hcm0vYm9vdC9kdHMvaW14NnFkbC1hcGFsaXMtZ2FiZW4tDQo+ IGZsZXhpc2JjLmR0c2kjTDE0Mw0KPiANCj4gVGhhbmtzIQ0KDQpUaGFuayB5b3UuDQoNCj4gLS0g eW5lenoNCg0KDQpbMV0gaHR0cHM6Ly9naXQua2VybmVsLm9yZy9jZ2l0L2xpbnV4L2tlcm5lbC9n aXQvdG9ydmFsZHMvbGludXguZ2l0L3RyZWUvYXJjaC9hcm0vYm9vdC9kdHMvdGVncmEzMC1hcGFs aXMtZXZhbC5kdHMjbjIzNQ0KDQpDaGVlcnMNCg0KTWFyY2VsDQpfX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fXwpsaW51eC1hcm0ta2VybmVsIG1haWxpbmcgbGlz dApsaW51eC1hcm0ta2VybmVsQGxpc3RzLmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJh ZGVhZC5vcmcvbWFpbG1hbi9saXN0aW5mby9saW51eC1hcm0ta2VybmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754023AbcAFKse (ORCPT ); Wed, 6 Jan 2016 05:48:34 -0500 Received: from mail-db3on0129.outbound.protection.outlook.com ([157.55.234.129]:15904 "EHLO emea01-db3-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752508AbcAFKs3 (ORCPT ); Wed, 6 Jan 2016 05:48:29 -0500 X-Greylist: delayed 5584 seconds by postgrey-1.27 at vger.kernel.org; Wed, 06 Jan 2016 05:48:28 EST From: Marcel Ziswiler To: "ynezz@true.cz" CC: "linux-kernel@vger.kernel.org" , "robh+dt@kernel.org" , "pawel.moll@arm.com" , "stefan@agner.ch" , "shawn.guo@linaro.org" , "devicetree@vger.kernel.org" , "festevam@gmail.com" , "mark.rutland@arm.com" , "stillcompiling@gmail.com" , "ijc+devicetree@hellion.org.uk" , "galak@codeaurora.org" , "shawnguo@kernel.org" , "linux-arm-kernel@lists.infradead.org" , "l.stach@pengutronix.de" , "kernel@pengutronix.de" , "linux@arm.linux.org.uk" Subject: Re: [PATCH v2 1/2] ARM: dts: imx6: Add support for Toradex Apalis iMX6Q/D SoM Thread-Topic: [PATCH v2 1/2] ARM: dts: imx6: Add support for Toradex Apalis iMX6Q/D SoM Thread-Index: AQHRSGLDZtXMzmHeZkKAFSgeydJV9Q== Date: Wed, 6 Jan 2016 09:15:18 +0000 Message-ID: <1452071716.30372.61.camel@toradex.com> References: <1452011942-11940-1-git-send-email-marcel.ziswiler@toradex.com> <1452011942-11940-2-git-send-email-marcel.ziswiler@toradex.com> <20160106080358.GD9030@ibawizard.net> In-Reply-To: <20160106080358.GD9030@ibawizard.net> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=marcel.ziswiler@toradex.com; x-originating-ip: [46.140.72.82] x-microsoft-exchange-diagnostics: 1;VI1PR05MB0974;5:2etmL1OQBw72l/X0ZhBF7Otidgu5B2iM9sq1r2K6TOGWJ9V6Ml0K2w2oiyTzA+KVMyt5pFCKCBpLtre2mUUcc5DLWvSDA6dvdghKWOoGW+LBjwXuzYi1yE4jO1+u7j1IMb3Jc+LG4H82rTucJfAoug==;24:Vpmsxm9QvRsDKm3OmmM6D4tjipDYivgoqQ5t4wKBU7wiJlEzExHsTqnzKqD7QMoMzLzTnnB+3ne2n+o/eRM7/qO3214htzyJfjH0ALhTN3M= x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:VI1PR05MB0974; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:; x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(601004)(2401047)(5005006)(520078)(8121501046)(10201501046)(3002001);SRVR:VI1PR05MB0974;BCL:0;PCL:0;RULEID:;SRVR:VI1PR05MB0974; x-forefront-prvs: 0813C68E65 x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(6009001)(52604005)(24454002)(189002)(377424004)(199003)(5001960100002)(97736004)(4326007)(2501003)(3846002)(2900100001)(54356999)(106356001)(40100003)(36756003)(87936001)(1730700002)(5008740100001)(103116003)(2351001)(50986999)(105586002)(106116001)(5002640100001)(81156007)(1220700001)(66066001)(575784001)(19580405001)(102836003)(189998001)(101416001)(92566002)(76176999)(5004730100002)(86362001)(1096002)(33646002)(586003)(19580395003)(11100500001)(5250100002)(110136002)(15975445007)(2950100001)(6116002)(32563001);DIR:OUT;SFP:1102;SCL:1;SRVR:VI1PR05MB0974;H:VI1PR05MB0974.eurprd05.prod.outlook.com;FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; spamdiagnosticoutput: 1:23 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="utf-8" Content-ID: <879E344D5F3ED74282E933D6CCD4D359@eurprd05.prod.outlook.com> MIME-Version: 1.0 X-OriginatorOrg: toradex.com X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Jan 2016 09:15:18.9931 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: d9995866-0d9b-4251-8315-093f062abab4 X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR05MB0974 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u06AmcFv002295 Hi Petr On Wed, 2016-01-06 at 09:03 +0100, Petr Štetiar wrote: > Marcel Ziswiler [2016-01-05 17:39:01]: > > Hi Marcel, > > thanks for taking care of this, I'm quite busy with other tasks :( You're welcome. I know that feeling. > > - integrated review feedback from Lucas > > You've probably missed few of them :) See my nitpicks bellow. I don't think so but we don't necessarily agree with all of Lucas' findings plus the discussion about some of the stuff hasn't really concluded. > > - left and even added some more comments as I don't see why putting > > any > >   explanatory comments in dts' should be such a bad thing to do > You've marked this as v2 Well, as Stefan pointed out we at Toradex are/were working on this as well and we discussed whether we should just ignore whatever you have done or not done and post our own stuff but decided to rather join efforts now as you already jumped ahead. > so I think, that you should only work on the > feedback. Ideally you shouldn't add any new stuff if it wasn't > requested > otherwise you're wasting reviewer time. I don't think so. What is important is that any changes are clearly declared which we did. > > - completely got rid of the memory node as that is something > > typically filled > >   in by the boot loader e.g. U-Boot > > If I'm not mistaken, it wasn't requested by the reviewer. Lucas was actually nitpicking about the location thereof and rather than moving it we decided to just get rid of it as it does not only not add any value but is simply wrong on most of the modules featuring different amount of memory. > > - without the regulators simple-bus it no longer boots > > It works for me on 4.4.0-rc3 with following DTS[1]. BTW, this DTS is > my > preparation for v2 patch series. I only tried with -next stuff so it might be something is just broken there I guess. > > - fixed Ethernet PHY reset & interrupt (requires Micrel PHY driver > > to be > >   enabled) > > Great! Yeah, that one took us a day to figure it all out. At the end we actually stumbled over your PCIe reset patch inverting the meaning of active-low vs. active-high (;-p). > > + * This file is dual-licensed: you can use it either under the > > terms > > + * of the GPL or the X11 license, at your option. Note that this > > dual > > + * licensing only applies to this file, and not this project as a > > + * whole. > > Hope you've some kind of ACK from FSL and Linaro :) Well, all new DTS' are now dual-licensed including several i.MX 6 related ones so we are not doing anything special there really. > > + reg_usb_otg_vbus: usb_otg_vbus { > > + compatible = "regulator-fixed"; > > + pinctrl-names = "default"; > > + pinctrl-0 = > > <&pinctrl_regulator_usbotg_pwr>; > > + regulator-name = "usb_otg_vbus"; > > + regulator-min-microvolt = <5000000>; > > + regulator-max-microvolt = <5000000>; > > + gpio = <&gpio3 22 GPIO_ACTIVE_HIGH>; > > + enable-active-high; > > + status = "disabled"; > > + }; > > I'm not sure, but it seems to me, that we should move this regulator > into the > carrier board DTS. On our custom carrier board we've this regulator > always on > and we use this GPIO for heartbeat LED, so I've this in our carrier > board > DTS[2]: Yes, you are absolutely right and e.g. on Apalis T30 [1] that is exactly how we did it. > reg_usb_otg_vbus: usb_otg_vbus { > /* Regulator is always on, and we use GPIO for > heartbeat LED */ > pinctrl-0 = <>; > gpio = <>; > > compatible = "regulator-fixed"; > regulator-name = "usb_otg_vbus"; > regulator-min-microvolt = <5000000>; > regulator-max-microvolt = <5000000>; > regulator-always-on; > }; > > ... > > usbotg { > /* GPIO3_IO22 is Heartbeat LED */ > pinctrl_regulator_usbotg_pwr: gpio_regulator_usbotg_pwr > { > }; > }; > > > +/* PAD Ctrl values for common settings */ > > +/* > > + * (PAD_CTL_HYS | PAD_CTL_PUS_100K_UP | PAD_CTL_PUE | PAD_CTL_PKE > > | > > + *  PAD_CTL_SPEED_MED | PAD_CTL_DSE_40ohm) > > + */ > > +#define PAD_CTRL_HYS_PU 0x1b0b0 > > This was requested to be reworked. I've simply replaced all the > macros with > hex values. I don't think the reviewers really concluded on the action to be taken on here and just using hex values is probably the most stupid solution. > > + pinctrl_usdhc3_100mhz: usdhc3grp-100mhz { /* > > 100Mhz */ > > + fsl,pins = < > > + MX6QDL_PAD_SD3_CMD__SD3_CMD > > 0x170B9 > > As per review comments, all hex values should be lowercase. Yes, we probably missed that one. > 1. https://github.com/ynezz/linux-2.6/blob/b27de46e67605fe1a8e386b065 > 845a9708e8e792/arch/arm/boot/dts/imx6qdl-apalis.dtsi > 2. https://github.com/ynezz/linux-2.6/blob/ec678e3734d9fff35f7ba96054 > 69a68a3e424020/arch/arm/boot/dts/imx6qdl-apalis-gaben- > flexisbc.dtsi#L143 > > Thanks! Thank you. > -- ynezz [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/tegra30-apalis-eval.dts#n235 Cheers Marcel {.n++%ݶw{.n+{G{ayʇڙ,jfhz_(階ݢj"mG?&~iOzv^m ?I