From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id B7890C352A1 for ; Tue, 6 Dec 2022 10:47:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Vv23Wv9OzRxDmaxuzhuD2p6CE+li7+a3HqzBt6DJkU0=; b=ubpUI9cPu0AxVa VebGiafgQl4zB7yJBmiAGd7k089ocWWIpyxlf8bSDBcj1nu7irN8M4GuNTMjOpE7YQjTtuXv3MHtI yLeAGOoLvzIksvTeb/QmoOT+hkV8+IKIz91Lu0UxDflC54H0onFBoin3LmPMY7n1yrk4Hp3H2BHua mbBDcUv4tz30YN3CZAklWzlsojz7cCqdvqS4e+2cKFIJUwX+d4uXWvdl9RugXnrBN424Q1DbWnB0N n8Ioc32Ey6idRR9t2o6O8jCak7/PbS1XVqVH6J7ygljXz4gZxAxO6/ne3t6mQreoEHBt5BQ/drSZd 4FkitIPTP/zEkg55MfTg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1p2VSi-007Q7x-Oa; Tue, 06 Dec 2022 10:46:08 +0000 Received: from perceval.ideasonboard.com ([213.167.242.64]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1p2VSe-007Pml-3M for linux-arm-kernel@lists.infradead.org; Tue, 06 Dec 2022 10:46:07 +0000 Received: from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi [213.243.189.158]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 50D5C480; Tue, 6 Dec 2022 11:46:00 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1670323560; bh=peUIv9jucIgp0B2+gMC8rxgB7haC/MYgu0HqsykOQGQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=hD6RbrVveR8PcBBwiBtZVeF7fDa7FD816/QvO8GFzoT4kNAV9jD/jX/U1o4arztXi Yk8FmEpE/THzQS1AEnTAYxvxXKNiqkPVMessG9UBD/cry5i/B39KofJSwbxDh+OsuN iuu4x1P96nSrwJ0eiZ6GiOqdrEYB7AyAF71tx5GA= Date: Tue, 6 Dec 2022 12:45:57 +0200 From: Laurent Pinchart To: Dan Scally Cc: Ahmad Fatoum , krzysztof.kozlowski@linaro.org, shawnguo@kernel.org, robh@kernel.org, marcel.ziswiler@toradex.com, leoyang.li@nxp.com, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, s.hauer@pengutronix.de, kieran.bingham@ideasonboard.com, debix-tech@polyhex.net, linux-imx@nxp.com, kernel@pengutronix.de, festevam@gmail.com Subject: Re: [PATCH v4 3/3] arm64: dts: Add device tree for the Debix Model A Board Message-ID: References: <20221017151050.2321919-1-dan.scally@ideasonboard.com> <20221017151050.2321919-4-dan.scally@ideasonboard.com> <23e61494-5567-5701-3a90-3b8105b4c944@pengutronix.de> <2cf061d8-dc5d-ec2d-02a0-10cc59daa7b0@ideasonboard.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <2cf061d8-dc5d-ec2d-02a0-10cc59daa7b0@ideasonboard.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221206_024604_329275_8379944A X-CRM114-Status: GOOD ( 37.00 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Dec 06, 2022 at 10:25:08AM +0000, Dan Scally wrote: > Hi Ahmad - thanks for the review > > On 01/12/2022 17:10, Ahmad Fatoum wrote: > > Hello Daniel, > > > > On 17.10.22 17:10, Daniel Scally wrote: > >> Add a device tree file describing the Debix Model A board from > >> Polyhex Technology Co. > > > > Thanks for your patch. Some minor comments below. > > > >> Changes in v3 (Laurent): > >> > >> - Added IOB copyright notice > >> - Removed the eth node for the connector that's on the separate I/O > >> board > > I'd have left the FEC node in and described the PHY, but left the FEC disabled. > > Only the magnetics are on the expansion board, while the PHY is on the > > base board. > > Fair enough, though there's quite a lot else on the base board which > we've left off simply because we're not currently using it. I'm inclined > to treat this the same for now. Overall I think it's a good strategy, if we haven't tested a peripheral we shouldn't include it yet. Missing pieces can be added later. Of course if there are pieces that are easy to test already, we can include them. > >> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > >> +/* > >> + * Copyright 2019 NXP > >> + * Copyright 2022 Ideas on Board Oy > >> + */ > >> + > >> +/dts-v1/; > >> + > >> +#include > >> +#include > >> +#include > >> + > >> +#include "imx8mp.dtsi" > >> + > >> +/ { > >> + model = "Polyhex Debix Model A i.MX8MPlus board"; > >> + compatible = "polyhex,imx8mp-debix-model-a", "fsl,imx8mp"; > > > > I see that Model A and Model B share the same SoC and PCB. Could you > > add polyhex,imx8mp-debix as a second compatible? That way, bootloader > > may match against that compatible when they support both. > > You'll need to adjust the binding accordingly. > > Sure, makes sense to me > > >> + > >> + chosen { > >> + stdout-path = &uart2; > >> + }; > >> + > >> + gpio-leds { > >> + compatible = "gpio-leds"; > >> + pinctrl-names = "default"; > >> + pinctrl-0 = <&pinctrl_gpio_led>; > >> + > >> + status-led { > >> + function = LED_FUNCTION_POWER; > >> + color = ; > >> + gpios = <&gpio3 16 GPIO_ACTIVE_HIGH>; > >> + default-state = "on"; > >> + }; > >> + }; > >> + > >> + reg_usdhc2_vmmc: regulator-usdhc2 { > >> + compatible = "regulator-fixed"; > >> + pinctrl-names = "default"; > >> + pinctrl-0 = <&pinctrl_reg_usdhc2_vmmc>; > >> + regulator-name = "VSD_3V3"; > >> + regulator-min-microvolt = <3300000>; > >> + regulator-max-microvolt = <3300000>; > >> + gpio = <&gpio2 19 GPIO_ACTIVE_HIGH>; > >> + enable-active-high; > >> + }; > >> +}; > >> + > >> +&A53_0 { > >> + cpu-supply = <&buck2>; > >> +}; > >> + > >> +&A53_1 { > >> + cpu-supply = <&buck2>; > >> +}; > >> + > >> +&A53_2 { > >> + cpu-supply = <&buck2>; > >> +}; > >> + > >> +&A53_3 { > >> + cpu-supply = <&buck2>; > >> +}; > >> + > >> +&eqos { > >> + pinctrl-names = "default"; > >> + pinctrl-0 = <&pinctrl_eqos>; > >> + phy-connection-type = "rgmii-id"; > >> + phy-handle = <ðphy0>; > >> + status = "okay"; > >> + > >> + mdio { > >> + compatible = "snps,dwmac-mdio"; > >> + #address-cells = <1>; > >> + #size-cells = <0>; > >> + > >> + ethphy0: ethernet-phy@0 { > > > > Could you append a /* RTL8211E */ comment here? This can be very useful for others > > who need to bring up the same chip in the future. > > Sure > > >> + compatible = "ethernet-phy-ieee802.3-c22"; > >> + reg = <0>; > > > > Is the PHY really at address 0 or does it just answer at this address > > because it's the broadcast address? > > I confess I'm unsure here, the number here comes from the downstream > .dts, which in this instance I've trusted...is there any way I can check? > > >> +&iomuxc { > >> + pinctrl-names = "default"; > >> + pinctrl-0 = <&pinctrl_hog>; > >> + > >> + pinctrl_hog: hoggrp { > >> + fsl,pins = < > >> + MX8MP_IOMUXC_HDMI_DDC_SCL__HDMIMIX_HDMI_SCL 0x400001c3 > >> + MX8MP_IOMUXC_HDMI_DDC_SDA__HDMIMIX_HDMI_SDA 0x400001c3 > >> + MX8MP_IOMUXC_HDMI_HPD__HDMIMIX_HDMI_HPD 0x40000019 > >> + MX8MP_IOMUXC_HDMI_CEC__HDMIMIX_HDMI_CEC 0x40000019 > > > > Why do you hog these? > > > >> + pinctrl_usb1_vbus: usb1grp { > > > > This is unused. > > Ah both for other elements of the board which aren't included in this > set, as they don't work properly yet. Apologies; I'll remove those. > > >> + pinctrl_usdhc2: usdhc2grp { > >> + fsl,pins = < > >> + MX8MP_IOMUXC_SD2_CLK__USDHC2_CLK 0x190 > >> + MX8MP_IOMUXC_SD2_CMD__USDHC2_CMD 0x1d0 > >> + MX8MP_IOMUXC_SD2_DATA0__USDHC2_DATA0 0x1d0 > >> + MX8MP_IOMUXC_SD2_DATA1__USDHC2_DATA1 0x1d0 > >> + MX8MP_IOMUXC_SD2_DATA2__USDHC2_DATA2 0x1d0 > >> + MX8MP_IOMUXC_SD2_DATA3__USDHC2_DATA3 0x1d0 > >> + MX8MP_IOMUXC_GPIO1_IO04__USDHC2_VSELECT 0xc1 > > > > Just to make sure this doesn't fry SD-Cards by mistake: VSELECT is indeed > > connected to a 1.8V/3.3V switch powering vqmmc? > > > >> +/* SD Card */ > >> +&usdhc2 { > >> + assigned-clocks = <&clk IMX8MP_CLK_USDHC2>; > >> + assigned-clock-rates = <400000000>; > > > > I wonder why this is necessary. Do you see a difference > > in /sys/kernel/debug/mmcX/ios between having this and leaving > > it out? > > I don't actually...it's present in the imx8mp-evk.dts which this is > based on, but in addition to not seeing any difference there the SD card > still seems fine as far as I can tell (same read / write speed in > practice) - I'll take it out, thanks > > >> + status = "okay"; > >> +}; > >> + > >> +/* eMMc */ > > > > eMMC > > Ack > > >> +&usdhc3 { > >> + assigned-clocks = <&clk IMX8MP_CLK_USDHC3>; > >> + assigned-clock-rates = <400000000>; > >> + pinctrl-names = "default", "state_100mhz", "state_200mhz"; > >> + pinctrl-0 = <&pinctrl_usdhc3>; > >> + pinctrl-1 = <&pinctrl_usdhc3_100mhz>; > >> + pinctrl-2 = <&pinctrl_usdhc3_200mhz>; > >> + bus-width = <8>; > >> + non-removable; > >> + status = "okay"; > >> +}; > >> + > >> +&wdog1 { > >> + pinctrl-names = "default"; > >> + pinctrl-0 = <&pinctrl_wdog>; > >> + fsl,ext-reset-output; > >> + status = "okay"; > >> +}; -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel