Received: by 2002:a05:6a10:5bc5:0:0:0:0 with SMTP id os5csp4629114pxb; Tue, 2 Nov 2021 13:01:42 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwIKVGBcqBUwEwcgT/VllyKJ7uxqehFwARPLvK0AByIFqxMdQEJ18f3gSz219qAC8N7QuMk X-Received: by 2002:a05:6402:26c2:: with SMTP id x2mr6635198edd.198.1635883302513; Tue, 02 Nov 2021 13:01:42 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1635883302; cv=none; d=google.com; s=arc-20160816; b=g+89NBjAiF1C5eKAROTWgDYUA8rofGqR3pzOrBYN5Zt2sLKiFwU90D6UzJzgNwq9NT VjUAPVXS1Gr0LM2tgTS+m/Mo1ze4OGjcFQtREObO4n+jn8CZFFEI5UQQbHjt+hOCvytm 2ioMpQNJpJwsLLXc+nY7MaC4iYJwUV55Widhq/MvOaD2iaQMEemhW96BC2itTNmXDST+ lqzY7tlJhOg7AwpLCDEzY7OL5HJfA8obeU7+B3z073FD7Cyp00pkQizzdAhNpn4A8nXj VXCuiwM9dqZHUxsYA2wIVwUnW1QkYWLjo9Q+Hr5iolnLAo9gVSJI5k4gj2hlMzFYzrPx ZPlA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version; bh=RkF10ejv+ZfbbBVLr9JViJZH+YXp2/4h2Ubp9f1gi2c=; b=PNRlxnblZ9+cs88AxzjmZVff43rGenFgES6PPQqvs7if0K2pZgnPZXTM6w01jONnjH zNNCoqLIpXbGyeEVeoLGnvUqdUqV5XMGmeNfEe4Z2ar3brc/57oL3k/ynQfXdeMJnLAB gv6TJtSUsIFp7gIl0IjaYBVOtaijJrKNjMcMOznPlcQLc1Blh4zep0fSzo0bHaD5X3w/ im+4UgVgGBIjKQ96Dv066o/zMBsHFacOsoO7jcN6+X0SmJ5lfBf7YclUt5p+SX+cw0wt HK0BP/plBqrK+BLnnmhCngxxfeZ5FBEQ8CDFLI0yIGU4nrksEti5bn2pBtK696ZunXEs pUBQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id kz1si25767024ejc.7.2021.11.02.13.01.17; Tue, 02 Nov 2021 13:01:42 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230457AbhKBUBo (ORCPT + 99 others); Tue, 2 Nov 2021 16:01:44 -0400 Received: from mail-pl1-f175.google.com ([209.85.214.175]:33296 "EHLO mail-pl1-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229813AbhKBUBn (ORCPT ); Tue, 2 Nov 2021 16:01:43 -0400 Received: by mail-pl1-f175.google.com with SMTP id s24so617319plp.0; Tue, 02 Nov 2021 12:59:08 -0700 (PDT) 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=RkF10ejv+ZfbbBVLr9JViJZH+YXp2/4h2Ubp9f1gi2c=; b=mc7wf9S3vGDNWjNgPTnaD5ghHG98OpxUGSiTEfwtyeU5sls3AGHrhR7gFuK7F4AwdJ tTITkyIRovka5daBa0lqOS3NqA6sHBNwdiGvtBp41KP5xSOo+kLJByf/IFRTOQtmu2fU kHFQQV0pfX1T+dXoZwEY2VJLZPG6GRqtywPHgdRCaAyBwrV2J5aNuP3ahc1+nieeoYpy idck+y2EdfvRsA+iYx8HKTD4ztf6FygMgffKsg8re0h5L9UBZNnJ90ahqgVuN2V4Mdi8 FANMx4ATyYy5Yww90SNtTY8Ye7bFrix/1EG5jZ7olpeZjql5J7QTJNZ/2Z8+X5dt7HHo HcUQ== X-Gm-Message-State: AOAM531k+kF1pOdnmbwhvEJRnLCzYmTHEFE02Kh7MwV257GEThRaEOoA Lovu4RwQjbJctSwJJGThvwRdCIJwO/oYUiTdLuo= X-Received: by 2002:a17:903:11c5:b0:13f:ef40:e319 with SMTP id q5-20020a17090311c500b0013fef40e319mr33828546plh.33.1635883148168; Tue, 02 Nov 2021 12:59:08 -0700 (PDT) MIME-Version: 1.0 References: <20211102161125.1144023-1-kernel@esmil.dk> <20211102161125.1144023-10-kernel@esmil.dk> In-Reply-To: From: Emil Renner Berthing Date: Tue, 2 Nov 2021 20:58:57 +0100 Message-ID: Subject: Re: [PATCH v3 09/16] reset: starfive-jh7100: Add StarFive JH7100 reset driver To: Andy Shevchenko Cc: Yury Norov , linux-riscv , devicetree , linux-clk , "open list:GPIO SUBSYSTEM" , "open list:SERIAL DRIVERS" , Palmer Dabbelt , Paul Walmsley , Rob Herring , Michael Turquette , Stephen Boyd , Thomas Gleixner , Marc Zyngier , Philipp Zabel , Linus Walleij , Greg Kroah-Hartman , Daniel Lezcano , Andy Shevchenko , Jiri Slaby , Maximilian Luz , Sagar Kadam , Drew Fustini , Geert Uytterhoeven , Michael Zhu , Fu Wei , Anup Patel , Atish Patra , Matteo Croce , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2 Nov 2021 at 20:43, Andy Shevchenko wrote: > +Cc: Yury (bitmap expert) > > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing wrote: > > > > Add a driver for the StarFive JH7100 reset controller. > > ... > > > +#define BIT_MASK32(x) BIT((x) % 32) > > Possible namespace collision. > > ... > > > +/* > > + * the registers work like a 32bit bitmap, so writing a 1 to the m'th bit of > > + * the n'th ASSERT register asserts line 32n + m, and writing a 0 deasserts the > > + * same line. > > + * most reset lines have their status inverted so a 0 in the STATUS register > > + * means the line is asserted and a 1 means it's deasserted. a few lines don't > > + * though, so store the expected value of the status registers when all lines > > + * are asserted. > > + */ > > Besides missing capitalization, I'm confused. it was you who wanted all comments to capitalized the same.. 64bi if it sounds like bitmap, use bitmap. > I have checked DT definitions and it seems you don't even need the > BIT_MASK() macro, > > > +static const u32 jh7100_reset_asserted[4] = { > > + /* STATUS0 register */ > > + BIT_MASK32(JH7100_RST_U74) | > > + BIT_MASK32(JH7100_RST_VP6_DRESET) | > > + BIT_MASK32(JH7100_RST_VP6_BRESET), > > + /* STATUS1 register */ > > + BIT_MASK32(JH7100_RST_HIFI4_DRESET) | > > + BIT_MASK32(JH7100_RST_HIFI4_BRESET), > > + /* STATUS2 register */ > > + BIT_MASK32(JH7100_RST_E24), > > + /* STATUS3 register */ > > + 0, > > +}; > > Yury, do we have any clever (clean) way to initialize a bitmap with > particular bits so that it will be a constant from the beginning? If > no, any suggestion what we can provide to such users? The problem is, that even if we could initialize this without the monstrosity in our last conversation a 64bit bitmap would still produce worse code. As it is now it's simply a 32bit load and mask with index and mask already calculated for the registers. In the status callback the mask can even be folded into the register read mask. With a 64bit bitmap you'd need to calculate new 64bit index and masks, and then conditionally shift the bits into position. If this reflection of the 32bit registers bothers you that much we could alternatively do static bool jh7100_reset_inverted(unsigned int idx) { switch (idx) { case JH7100_RST_U74: case JH7100_RST_VP6_DRESET: .. return false; } return true; } It'd still produce worse code, but at least it would be readable. /Emil > ... > > > + dev_dbg(rcdev->dev, "reset(%lu)\n", id); > > These debug messages are useless since one should use ftrace facility instead, > > -- > With Best Regards, > Andy Shevchenko