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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EDD11C433F5 for ; Mon, 25 Oct 2021 19:46:40 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id 63A3A61073 for ; Mon, 25 Oct 2021 19:46:40 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 63A3A61073 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id AEE458355F; Mon, 25 Oct 2021 21:46:38 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.b="PA7W/CfU"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id EDDF18355E; Mon, 25 Oct 2021 21:46:36 +0200 (CEST) Received: from mail-ua1-x930.google.com (mail-ua1-x930.google.com [IPv6:2607:f8b0:4864:20::930]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 8F7B183573 for ; Mon, 25 Oct 2021 21:46:31 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=sjg@google.com Received: by mail-ua1-x930.google.com with SMTP id h4so24552384uaw.1 for ; Mon, 25 Oct 2021 12:46:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=BJCtIJXpwwPrJogMXF0nmEEBWtjOYq+L2sOGm+c6cms=; b=PA7W/CfU/FcyluvwK1JJIXOS7fWS/rg/w2zlX/jnpx4VlFbVu7j3r73jJ/LYt2gGzX DwVQ2LKMwoVCroR1cccMi+cxx2tU0KlI9Kt87LqrFX0AgrxbTDYrrttAkNw89QdcHuTu eDGOOnFsv6rLcE7dWgwByLMUcjuYCHyqwo2Y0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=BJCtIJXpwwPrJogMXF0nmEEBWtjOYq+L2sOGm+c6cms=; b=MwcV7ZqHjXa/v6EYxr/M3d3OuOQZlqeLNBlyftehG2IxGyTC2qAtaqfTh4nay9Jtx0 6lhNTb0x74xBhtT8iU+bdk9GO+wBZYBWi1DRQvKOL0erbh4oEJNPjevT9Y55k4Rr0UB5 8OxLbJxpWuA4/9rmd7PuR0E7OiI0ZwrIDpF7KFhG2pigrUd3g5QafqwtSI81mzYD5yJq unYsMnoxd8Crjlk0ORfwAM0xoQ2MBtBx5x9l3Ah/ogoXi8STccPUPeK4ZarPJLTn0fAW vosME/JAQrdnGPCBiJl2OKPEf9KIOvrbXCEjZd+EGqY8CBRsd1mvNEXiayYkcB08MJw9 3J5w== X-Gm-Message-State: AOAM530J2Fr2eGuqLey3GIvSabewch73eEqz+gcL4hgnMDTrRlSPvFFQ 8GL5yqUvmdOPdNuu5iy5S+MM+QHlS65jnRJTTJ6lsw== X-Google-Smtp-Source: ABdhPJxa9OGOx2qbON/m+VPyRGqgBoHazE9R7mqMKcieC2jwwxScOviL75Ec2wQY1jG9I+KaIiCqa4FgvUwFFD99shY= X-Received: by 2002:a05:6102:304d:: with SMTP id w13mr12154892vsa.59.1635191189916; Mon, 25 Oct 2021 12:46:29 -0700 (PDT) MIME-Version: 1.0 References: <20211023140647.7661-1-heinrich.schuchardt@canonical.com> <81777666-a5e1-b32b-b41e-70d894a44b94@canonical.com> <7d082108-7687-2f9a-5a8b-7620573b823f@canonical.com> In-Reply-To: <7d082108-7687-2f9a-5a8b-7620573b823f@canonical.com> From: Simon Glass Date: Mon, 25 Oct 2021 13:46:18 -0600 Message-ID: Subject: Re: [PATCH 1/1] blk: simplify blk_get_devnum_by_typename() To: Heinrich Schuchardt Cc: Patrick Delaunay , Ilias Apalodimas , U-Boot Mailing List , Tom Rini Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 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.2 at phobos.denx.de X-Virus-Status: Clean Hi Heinrich, On Mon, 25 Oct 2021 at 13:37, Heinrich Schuchardt wrote: > > > > On 10/25/21 21:03, Simon Glass wrote: > > Hi Heinrich, > > > > On Mon, 25 Oct 2021 at 12:41, Heinrich Schuchardt > > wrote: > >> > >> > >> > >> On 10/25/21 19:29, Simon Glass wrote: > >>> Hi Heinrich, > >>> > >>> On Mon, 25 Oct 2021 at 09:44, Heinrich Schuchardt > >>> wrote: > >>>> > >>>> On 10/25/21 17:18, Simon Glass wrote: > >>>>> Hi Heinrich, > >>>>> > >>>>> On Mon, 25 Oct 2021 at 02:00, Heinrich Schuchardt > >>>>> wrote: > >>>>>> > >>>>>> On 10/25/21 09:54, Heinrich Schuchardt wrote: > >>>>>>> On 10/24/21 21:54, Simon Glass wrote: > >>>>>>>> Hi Heinrich, > >>>>>>>> > >>>>>>>> On Sat, 23 Oct 2021 at 08:06, Heinrich Schuchardt > >>>>>>>> wrote: > >>>>>>>>> > >>>>>>>>> The block descriptor contains the if_type. There is no need to first > >>>>>>>>> look > >>>>>>>>> up the uclass for the if_type and then to check the parent device's > >>>>>>>>> uclass > >>>>>>>>> to know if the device has the correct if_type. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Heinrich Schuchardt > >>>>>>>>> --- > >>>>>>>>> drivers/block/blk-uclass.c | 35 +---------------------------------- > >>>>>>>>> 1 file changed, 1 insertion(+), 34 deletions(-) > >>>>>>>> > >>>>>>>> This seems to be heading in the wrong direction though. > >>>>>>>> > >>>>>>>> The IF_TYPE should really go away and be replaced with the UCLASS ID, > >>>>>>>> I think. > >>>>>>>> > >>>>>>>> At present we have lots of calls to blk_create_device_f() which > >>>>>>>> specify the type. I think we should drop the IF_TYPE_.. arg to that > >>>>>>>> function and have it figured out from the uclass, in the interim. > >>>>>>>> > >>>>>>>> But why do we need IF_TYPE now? > >>>>>>> > >>>>>>> I admit that this patch is just an intermediate step. > >>>>>>> > >>>>>>> We can replace IF_TYPE by ULASS ID once SPL block devices are always > >>>>>>> using the driver model. Have we reached this point yet? I have not seen > >>>>>>> drivers/block/blk_legacy.c being deleted. > >>>>>> > >>>>>> qemu_malta64, qemu_malta64el, qemu_malta, qemu_maltael are examples of > >>>>>> defconfigs requiring drivers/block/blk_legacy.c > >>>>> > >>>>> Yes, we seem to have BLK on only for MMC and USB, but malta64 (for > >>>>> example) uses IDE. > >>>>> > >>>>> The problem seems to be HAVE_BLOCK_DEVICE which should not be used. > >>>>> > >>>>> +Tom Rini > >>>>> > >>>>> Should we remove HAVE_BLOCK_DEVICE at this point, or make it depend on BROKEN? > >>>>> > >>>>>> > >>>>>> Best regards > >>>>>> > >>>>>> Heinrich > >>>>>> > >>>>>>> > >>>>>>> Removing if_type_uclass_id[] is anyway on the right path. > >>>>> > >>>>> Are you sure? Instead, could we use the uclass ID in more places? > >>>> > >>>> Yes, you want to get rid of if_type. This means the table will become > >>>> obsolete. > >>> > >>> Yes. > >>> > >>>> Once the legacy drivers are removed we can replace all occurrences of > >>>> if_type by uclass_id. That uclass_id we should not take from blk_desc > >>>> but from udevice. > >>> > >>> How do we get from a blk_desc to a udevice, though? > >>> > >>> Could you instead look at moving from using blk_desc to using udevice? > >>> If we had that, I think I can see your point with this patch. At > >>> present, it looks like a backwards step because you are reducing the > >>> usage of the uclass and in fact removing it altogether. > >>> > >>> Can you see what I am getting at? Let's move towards migration > >>> incrementally, not destroy the bridges we have built towards the goal. > >> > >> Once you move from if_type to uclass_id you will simply replace > >> > >> if (desc->if_type != if_type) > >> > >> by > >> > >> if (device_get_uclass_id(dev->uclass->uclass_id) != uclass_id) > >> > >> Except for this line this patch brings you to the final code. > > > > device_get_uclass_id() BTW > > > > But where does uclass_id come from?! You are removing the function > > that produces it! > > In future you will pass uclass_id as parameter instead of if_type > because if_type will be replaced by uclass_id! Perhaps we can discuss this on the contributor call as we are not getting anywhere. You are removing the ability to translate between the old value and the new value, but still using the old value in the function call. I have already suggested how you could contribute to the migration effort if you have time. Rather than increasing reliance on if_type, we should be removing use of it, e.g. dropping it from blk_desc, etc. - Simon > > > > > How about, instead, you update blk_create_devicef() to drop the > > if_type parameter and just use the device's uclass? That would > > actually head forwards towards migration, rather than away from it. > > > > Regards, > > Simon > >