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 X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 236BEC433B4 for ; Mon, 5 Apr 2021 06:00:03 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 99A1461389 for ; Mon, 5 Apr 2021 06:00:02 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 99A1461389 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=marcan.st Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=desiato.20200630; h=Sender:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:Subject:Cc: From:References:To:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=GtehtQgjG8DD9GDugllqrnZraP8FDGrBWn78yrqBzoI=; b=SEyya8LVhUBzOXbrcky3cOBZe IK4kfXfVObWaC8cla499JtYK+sNs3LbdlI2n5P5+8MHb8261ATVCKgjzkDYAvY4vdbaxWXvamvFlQ 9iC9Ou3AnHbtuQlhn9/jQEBqNJwp+qrpa1vzFqJ5YNwS/AH/6NLDOx/uSSQisKGnh50+RT6/7StCR fq3dlDpW+D5Vv58l9BgTmQjqBziWAoiimH/6uOHeHcUUyEnW5UTI3nQ1wbZ8r7fYrXkMADVr9TVLY b4NcJHoQyOMiAm4GgN59WnZvUpLCO2fIwwHqdAiwQv0MnOwuC8+NKEJVxmJQm2OvaDYpL8Xmm9tRv cgD5tM4qQ==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lTIF8-00Gmbl-Md; Mon, 05 Apr 2021 05:57:46 +0000 Received: from marcansoft.com ([2a01:298:fe:f::2] helo=mail.marcansoft.com) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lTIF3-00GmbJ-Qg for linux-arm-kernel@lists.infradead.org; Mon, 05 Apr 2021 05:57:44 +0000 Received: from [127.0.0.1] (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: marcan@marcan.st) by mail.marcansoft.com (Postfix) with ESMTPSA id A8CB6424E8; Mon, 5 Apr 2021 05:50:44 +0000 (UTC) To: Konrad Dybcio , linux-arm-kernel@lists.infradead.org References: <20210304213902.83903-1-marcan@marcan.st> <20210304213902.83903-28-marcan@marcan.st> From: Hector Martin Cc: Arnd Bergmann , Krzysztof Kozlowski , Rob Herring Subject: Re: [RFT PATCH v3 27/27] arm64: apple: Add initial Apple Mac mini (M1, 2020) devicetree Message-ID: Date: Mon, 5 Apr 2021 14:50:42 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: Content-Language: es-ES X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210405_065742_109961_CF27CB06 X-CRM114-Status: GOOD ( 37.73 ) 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi, Sorry, I did not see this e-mail originally as it was sent only to the mailing list, not to me nor CCed to anyone else. I am subscribed to the lists, but I generally rely on my inbox for review feedback. I have added a sieve rule to catch emails sent to the list that are replies to me and do not have me in To: or Cc: and copy them to my inbox in the future. That said, I think it's best to reply-all, to keep everyone else in the loop. CCing a few relevant folks. On 14/03/2021 05.22, Konrad Dybcio wrote: >> +// SPDX-License-Identifier: GPL-2.0+ OR MIT > > It seems to be > > "// SPDX-License-Identifier: (GPL-2.0+ OR MIT)" > > in other DTs (notice the brackets). This was already discussed in [1]. The conclusion was that no braces is the preferred form according to the SPDX spec. [1] https://lore.kernel.org/linux-arm-kernel/20210216073120.qmlaky43t6uelqc4@kozik-lap/ >> +/* >> + * Apple Mac mini (M1, 2020) > > Not sure if it's necessary, you already state it in model="" I find it helpful to have identifying info in the top comment, since it's easier to locate at a glance than visually searching for the model property. Since I also add identifiers that Apple uses to be able to cross reference things here, even if it's somewhat duplicative, I think it makes sense to keep all of it in the top comment. >> + * >> + * target-type: J174 > > Isn't that a typo? Compatible states J*2*74, but I'm curious if that's intentional. That's a typo, it's already fixed in v4 :) >> + * >> + * Copyright The Asahi Linux Contributors > > * Copyright (C) 2021 The Asahi Linux Contributors This was also discussed, see [2] and [3]. Copyright dates in headers are essentially useless because they are always out of date in practice. [2] https://www.linuxfoundation.org/en/blog/copyright-notices-in-open-source-software-projects/ [3] https://lore.kernel.org/linux-arm-kernel/d8454963-3242-699b-4c20-e85d26b19796@marcan.st/ >> + */ >> + >> +/dts-v1/; >> + >> +#include "t8103.dtsi" >> + >> +/ { >> + compatible = "apple,j274", "apple,t8103", "apple,arm-platform"; >> + model = "Apple Mac mini (M1, 2020)"; >> + >> + aliases { >> + serial0 = &serial0; > > Isn't this M1-common? It's common to all existing M1 devices, but in principle there is nothing that says that the stdout path has to be the first UART on any given device (there is more than one UART, we just aren't declaring the others yet in this series). This is a common pattern, see e.g. nvidia/tegra210-smaug.dts. >> + }; >> + >> + chosen { >> + #address-cells = <2>; >> + #size-cells = <2>; >> + ranges; >> + >> + stdout-path = "serial0"; > > Doesn't serial work without this? This is for earlycon to work. See e.g. nvidia/tegra210-smaug.dts for another example. Note that this still requires the earlycon kernel parameter, so if earlycon is not enabled then this does nothing; if the user also isn't setting the serial TTY device as a console, then the serial port is free for use by the user. > Not sure if we want the @0 (unless it's also overwritten by the loader). We need a unit address for the linter, but yes, this is in fact overwritten by the loader with the correct one. >> + compatible = "apple,simple-framebuffer", "simple-framebuffer"; >> + reg = <0 0 0 0>; /* To be filled by loader */ >> + /* Format properties will be added by loader */ >> + status = "disabled"; > > Does it get enabled by the loader? Yup, assuming everything works. The loader leaves it disabled if something goes wrong converting the information from the XNU bootargs (e.g. unsupported CLUT format, which I don't think any M1 mac ever uses). > Also, isn't the framebuffer common for all M1 devices? The framebuffer is allocated by iBoot dynamically, and is at a variable address. The format is also variable, as is the resolution, depending on the device. This is why the loader fills in the info. In principle, the idea that a framebuffer exists is common to all existing devices, but nothing says this has to be the case for future devices: it's not a SoC property, it's a property of devices built using the SoC (and the way they are configured by iBoot). >> + }; >> + }; >> + >> + memory@800000000 { >> + device_type = "memory"; >> + reg = <0x8 0 0x2 0>; /* To be filled by loader */ >> + }; > > Shouldn't this be in the SoC DTSI? It could. The pattern I'm trying to follow here is that stuff filled in by the loader or which varies from device to device goes in the board file; technically here it doesn't matter since of course every SoC has "some" memory and the loader overwrites it anyway, but this still makes logical sense to me. If you look around, you'll see memory is usually defined in board or sub-platform files, not the SoC root (exception: qcom stuff, apparently). >> + compatible = "apple,t8103", "apple,arm-platform"; > > Please remove this line, it's getting overwritten anyway. I think it's helpful to document the expected compatible subset to be inherited by board files. The Tegra example I linked above does this too. >> + }; >> + >> + soc { >> + compatible = "simple-bus"; >> + #address-cells = <2>; >> + #size-cells = <2>; >> + >> + ranges; >> + nonposted-mmio; >> + > > I think > > " > > compatible = "simple-bus"; > > #address-cells = <2>; > > #size-cells = <2>; > > nonposted-mmio; > > ranges; > > " > > would look more coherent to the human eye, but that's rather a nit. The reason for putting nonposted-mmio last is that it is a flag for a bus, so to me it makes logical sense to put it after ranges, which is what marks this as a translatable bus. > Other than this, node order seems to be entirely random. Please sort them by address (where applicable) and by name where not, so that this doesn't become a huge mess as we go forward. Outside of the soc bus I think the order we have makes sense: CPUs first, timer (which is part of the CPU complex) next, then fixed clocks. There are only a few things that are going to go in here, so I think an ad-hoc order like this is okay. Inside the bus we only have two nodes so far, and they are indeed in the wrong order :-). I'll sort them by address. Almost everything else is going to go in here in the future, so that should keep things sane. Thanks! -- Hector Martin (marcan@marcan.st) Public Key: https://mrcn.st/pub _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel