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 phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 72C16C00A5A for ; Wed, 18 Jan 2023 02:52:15 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id F37A0855E6; Wed, 18 Jan 2023 03:52:12 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="SKD9UNEr"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 6A4F5855D6; Wed, 18 Jan 2023 03:52:10 +0100 (CET) Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id CD6BA855EB for ; Wed, 18 Jan 2023 03:52:06 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=robh@kernel.org Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id D7A35615D1 for ; Wed, 18 Jan 2023 02:52:04 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 491AEC433F2 for ; Wed, 18 Jan 2023 02:52:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1674010324; bh=r9J1qbd9BJFv0bgK1q2HE4ghmhMM3YanBch2qaegxb0=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=SKD9UNErQRxy/zwwJ8ngP7IsePEXNmZhq0bREZTNUe6nbzA7mca7INPGI1Y6/Dt35 F3RkFkahduFNmaEcdFrDCsHuYb3ymAeZIZllGWL4Cj7qh9vhOgFe/DrAbmO6p8McI1 AzB57pOW84V9oiZSc8+nZAOaiQlSZCUArT0Y0YvhLSUvQtng4osWLFJg1gAUPLnpb2 NKEXykSNECYctQnBKhgqLcgE6tI3yLqS/+Qd6lurNIroYd07o/CohtOhQi78cQD+qF ORueEciKeE0/pZj4kuuTM3+eUktuSs94VhMkfSsLfwK/8ik/kAtS7VoLZVyJzS34cG EOgaWTfZ/CA0Q== Received: by mail-vk1-f171.google.com with SMTP id 12so4804757vkj.12 for ; Tue, 17 Jan 2023 18:52:04 -0800 (PST) X-Gm-Message-State: AFqh2kow1SXs1MWjpLiPK0w4JjrEtTTewIHtf6DrQUGROqK+GEywX353 DNfc6HHdBrjMOl1zU/zxsq06E+ET3Oh0YD1kYg== X-Google-Smtp-Source: AMrXdXv8LRd5qSt1fXK/srAJ4utXFLPktAsqSIQ6faVJ0xGzq0pry9BUtLZm97gKnDVengLVdGk5aTK6FetsWDo34Ms= X-Received: by 2002:a1f:6e0b:0:b0:3da:f920:c0ef with SMTP id j11-20020a1f6e0b000000b003daf920c0efmr762226vkc.26.1674010323104; Tue, 17 Jan 2023 18:52:03 -0800 (PST) MIME-Version: 1.0 References: <20221107192055.21669-1-abdellatif.elkhlifi@arm.com> <20221122131751.22747-1-abdellatif.elkhlifi@arm.com> <20221122131751.22747-4-abdellatif.elkhlifi@arm.com> <20221124132115.GA393@e121910.cambridge.arm.com> <20221219111251.GA22370@e121910.cambridge.arm.com> In-Reply-To: From: Rob Herring Date: Tue, 17 Jan 2023 20:51:51 -0600 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v8 03/10] arm_ffa: introduce Arm FF-A low-level driver To: Simon Glass Cc: Abdellatif El Khlifi , ilias.apalodimas@linaro.org, jens.wiklander@linaro.org, achin.gupta@arm.com, trini@konsulko.com, xueliang.zhong@arm.com, nd@arm.com, u-boot@lists.denx.de Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean On Thu, Jan 12, 2023 at 5:43 PM Simon Glass wrote: > > Hi Rob, > > On Wed, 11 Jan 2023 at 19:10, Rob Herring wrote: > > > > On Mon, Dec 19, 2022 at 1:21 PM Simon Glass wrote: > > > > > > Hi Abdellatif, > > > > > > On Mon, 19 Dec 2022 at 04:12, Abdellatif El Khlifi > > > wrote: > > > > > > > > On Mon, Dec 05, 2022 at 09:49:30AM -0600, Rob Herring wrote: > > > > > On Sun, Dec 4, 2022 at 1:22 PM Simon Glass wro= te: > > > > > > > > > > > > Hi Rob, > > > > > > > > > > > > On Tue, 29 Nov 2022 at 05:22, Rob Herring wro= te: > > > > > > > > > > > > > > On Fri, Nov 25, 2022 at 3:18 PM Simon Glass wrote: > > > > > > > > > > > > > > > > Hi Abdellatif, > > > > > > > > > > > > > > > > On Thu, 24 Nov 2022 at 06:21, Abdellatif El Khlifi wrote: > > > > > > > > > > > > > > > > > > On Tue, Nov 22, 2022 at 07:09:16PM -0700, Simon Glass wro= te: > > > > > > > > > > should be called 'priov' and should beHi Abdellatif, > > > > > > > > > > > > > > > > > > > > > > > > > > [..] > > > > > > > > > > > > > > > > > > > +/** > > > > > > > > > > > + * ffa_device_get - create, bind and probe the arm_f= fa device > > > > > > > > > > > + * @pdev: the address of a device pointer (to be fil= led when the arm_ffa bus device is created > > > > > > > > > > > + * successfully) > > > > > > > > > > > + * > > > > > > > > > > > + * This function makes sure the arm_ffa device is > > > > > > > > > > > + * created, bound to this driver, probed and ready t= o use. > > > > > > > > > > > + * Arm FF-A transport is implemented through a singl= e U-Boot > > > > > > > > > > > + * device managing the FF-A bus (arm_ffa). > > > > > > > > > > > + * > > > > > > > > > > > + * Return: > > > > > > > > > > > + * > > > > > > > > > > > + * 0 on success. Otherwise, failure > > > > > > > > > > > + */ > > > > > > > > > > > +int ffa_device_get(struct udevice **pdev) > > > > > > > > > > > +{ > > > > > > > > > > > + int ret; > > > > > > > > > > > + struct udevice *dev =3D NULL; > > > > > > > > > > > + > > > > > > > > > > > + ret =3D device_bind(dm_root(), DM_DRIVER_GET(= arm_ffa), FFA_DRV_NAME, NULL, ofnode_null(), > > > > > > > > > > > + &dev); > > > > > > > > > > > > > > > > > > > > Please add a DT binding. Even if only temporary, we nee= d something for this. > > > > > > > > > > > > > > > > > > Thanks for the feedback. I'm happy to address all the com= ments. > > > > > > > > > > > > > > > > > > Regarding DT binding and FF-A discovery. We agreed with L= inaro and Rob Herring > > > > > > > > > about the following: > > > > > > > > > > > > > > > > > > - DT is only for what we failed to make discoverable. For= hardware, we're stuck > > > > > > > > > with it. We shouldn't repeat that for software interfac= es. This approach is > > > > > > > > > already applied in the FF-A kernel driver which comes w= ith no DT support and > > > > > > > > > discovers the bus with bus_register() API [1]. > > > > > > > > > > > > > > > > This may be the UEFI view, but it is not how U-Boot works. = This is not something we are 'stuck' with. It is how we define what is pres= ent on a device. This is how the PCI bus works in U-Boot. It is best practi= ce in U-Boot to use the device tree to make this things visible and configu= rable. Unlike with Linux there is no other way to provide configuration nee= ded by these devices. > > > > > > > > > > > > > > Where do you get UEFI out of this? > > > > > > > > > > > > I assume it was UEFI as there was no discussion about this in U= -Boot. > > > > > > Which firmware project was consulted about this? > > > > > > > > > > > > > > > > > > > > It is the discoverability of hardware that is fixed (and we a= re stuck > > > > > > > with). We can't change hardware. The disoverability may be PC= I > > > > > > > VID/PID, USB device descriptors, or nothing. We only use DT w= hen those > > > > > > > are not sufficient. For a software interface, there is no rea= son to > > > > > > > make them non-discoverable as the interface can be fixed (at = least for > > > > > > > new things like FF-A). > > > > > > > > > > > > Here I am talking about the controller itself, the top-level no= de in > > > > > > the device tree. For PCI this is a device tree node and it shou= ld be > > > > > > the same here. So I am not saying that the devices on the bus n= eed to > > > > > > be in the device tree (that can be optional, but may be useful = in some > > > > > > situations where it is status and known). > > > > > > > > > > Sure, the PCI host bridges are not discoverable, have a bunch of > > > > > resources, and do need to be in DT. The downstream devices only d= o if > > > > > they have extra resources such as when a device is soldered down = on a > > > > > board rather than a standard slot. > > > > > > > > > > > We need something like: > > > > > > > > > > > > ff-a { > > > > > > compatible =3D "something"; > > > > > > }; > > > > > > > > > > > > I don't know what mechanism is actually used to communicate wit= h it, > > > > > > but that will be enough to get the top-level driver started. > > > > > > > > > > There's discovery of FF-A itself and then discovery of FF-A featu= res > > > > > (e.g. partitions). Both of those are discoverable without DT. The > > > > > first is done by checking the SMCCC version, then checking for FF= -A > > > > > presence and features. Putting this into DT is redundant. Worse, = what > > > > > if they disagree? > > > > > > > > Hi Simon, > > > > > > > > Do you agree with Rob, Ilias and myself that it makes more sense > > > > FF-A bus is discovered without a DT node and following the same app= roach as > > > > Linux ? (FF-A bus doesn't have a HW controller and is a purely SW b= us, > > > > no configuration/description needed at DT level). > > > > > > > > Your suggestions are always welcome. > > > > > > I'm sorry I don't agree with that. It does need a compatible string, > > > like PCI has. You can just add it in U-Boot if Linux won't accept the > > > binding. > > > > It's not like PCI as the host side of PCI has non-discoverable resource= s. > > OK I see. It is certainly an edge case. > > > > > This all could have been designed better, but hindsight is 20/20 and > > things evolved step by step. There are a bunch of firmware services > > that are all behind SMCCC. The first (upstream) was PSCI. IIRC, SMCCC > > was invented a bit after that, but generalized PSCI for other > > services. Since then more have been added. More services get added one > > by one and yes we added bindings for them. Because what's one more... > > But that really needs to stop. We're stuck with h/w that's not > > discoverable, there's zero reason to do that with s/w interfaces. If > > we could redo everything, we'd have a node for SMCCC and that's it > > unless there's h/w resources provided to the rest of DT. But we can't, > > so SMCCC is discovered by the presence of PSCI. > > I understand the background here, but if we don't take a stand on > this, this sort of thing will continue. I agree completely (but I think what you mean for 'this' is different). Using DT for s/w features has to stop. Every new SMCCC firmware interface we get a new binding for it. FF-A originally had a bunch of crap in the binding. So did Op-tee. Why does each new firmware feature need a DT node? As software people, can't we design software interfaces which are discoverable on their own rather than using a hardware description for discovering them? The only part we can't discover is whether we have SMCCC or not, but we already have that in DT with the PSCI node because if we have PSCI, we have SMCCC. It is the same problem with that UEFI SPI protocol binding on the list last week. > Just because something works > in Linux does not mean that the binding (or lack of it) is good. > > The reasons to do this are: > - avoids needing to manually call device_bind() > - avoids extra plumbing in U-Boot > - provides visibility into what is in the system, by looking at the > DT, like documentation > - DT is how devices are bound in U-Boot > > You can see the problem if you look at ffa_device_get(). It is called > from ffa_bus_discover() which is a new addition into the board_init > list. We are trying to remove this list and certainly don't want new > things added!! That seems less than ideal. How do you init PSCI? Or u-boot doesn't touch it? The low-level code for both should be shared if not already. > We don't need to change this in the Linux implementation, just add a > top-level DT node for U-Boot. I don't understand why that is such a > big problem? It's a line in the sand. Yeah, 1 compatible is not a big deal, but then it is an invitation to add to the binding and add a compatible for the next SMCCC firmware feature. Rob