From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id E0CDD244694 for ; Thu, 30 Apr 2026 22:51:29 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1777589491; cv=none; b=FQvuCSeUgTWKtbdAyaGImkcLJTHpHEdiPtBqgpfPnt6pnssEfW/NsdlKFPMQFOn6JhqcbSgNJxbvcZBKnDN79LfNbx8MNvTFQDtPR6233YL95Ar3tygNoZmjgYDcVOFwSF+znp5gAK6XcwSYzMYTGvH6JRmdq8G8CjKwOhlq31g= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1777589491; c=relaxed/simple; bh=ccySoFKEoNVQ75TK1fIyIDu/J5XEcUCUomg+Rj0oapg=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=kc6Q82hLTka7nKgjIw7L2KCygDPhBrv+UAH0SINtAs6238MbsZT8MM/S9T3rx8T13IpJyHqZWmxLSlTUumGLjq1S/rC6aLyGgh5BXnWAkqKDXpDex8ouEiyj49pk8FUdOOFnJW15Ma57+bJ7t+1VW0WP+kmE+4HIRXoERtPvfSY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=KAjd6ZLn; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b=hTbEVrAs; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="KAjd6ZLn"; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b="hTbEVrAs" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1777589489; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=HdVxrW5qbfv9o4/xQYAtdxN+n/sFjjbbXDDlDPx+0zc=; b=KAjd6ZLnsawFsmiirUyuJUIPCw8UVWjB6Ny1k8biVgKfYk6ikBi6nD8Dbc1x19zTU2RJbe aVajoESGW+Vak5vHOI5ujh0JLKjr2b/c7VFg38Hlt+KhBO3AXJRauDaNZQ/uV8NiaZIOiY VbahPUyuZv8mRZTEy4O4ej7LPa+Rjms= Received: from mail-qt1-f198.google.com (mail-qt1-f198.google.com [209.85.160.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-422-SDXP7naiOB66mruJEWPLPw-1; Thu, 30 Apr 2026 18:51:27 -0400 X-MC-Unique: SDXP7naiOB66mruJEWPLPw-1 X-Mimecast-MFC-AGG-ID: SDXP7naiOB66mruJEWPLPw_1777589487 Received: by mail-qt1-f198.google.com with SMTP id d75a77b69052e-50ea1a7a5d0so30821881cf.3 for ; Thu, 30 Apr 2026 15:51:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=google; t=1777589487; x=1778194287; darn=vger.kernel.org; h=user-agent:in-reply-to:content-transfer-encoding :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=HdVxrW5qbfv9o4/xQYAtdxN+n/sFjjbbXDDlDPx+0zc=; b=hTbEVrAsUsjjGdpfbVWRlj1ctolSZLULfSkcePLvxvDxEH7xGSuu2M6QYH6MH28L7T I48zkN6lYytB2W6sAzy7+5Ta6iguzzPdHrA87Ja8sOeQqQe5IaqT2Ttk7vl7zD3Z5B4f Tmd1EWdRjBT5JUz4mXAHiCZpWpmblOgGmWk5gqEdjBnh1Sr8vQoXJzIw6PHusT9eUhb9 nK4mQPpAkmbVyk0FCLXqSVOidaAKYqCkAQvoyxpo4auUk7y0IfM4PNkEb6j7znV32uXv ZIHfD89ICl065y4RY+lhM9gqNK1IlC4TXiHoLX+VvlEHLka4De4NKV2FuaxMrhihw/nr z6bA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1777589487; x=1778194287; h=user-agent:in-reply-to:content-transfer-encoding :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=HdVxrW5qbfv9o4/xQYAtdxN+n/sFjjbbXDDlDPx+0zc=; b=h55hJ7qWAEWl0KpcdaKOa7RSg2QbifEYOcokU1f+kIT8+oCzxsDYamVZKoQKqTj1D0 q8vd/nm6rb6fb+GlBGEmvkF9oyP+mtWa+IWMEHXjy/EOol9Y9+F9nl4QaC6OR6aVh10G iTELen6nwrbrP66YTCOcAn0B3y/Huz66UCED9/SNwzsyfyJ0SfNA8FVaYe35mju6WH0D mZRJcKri8mq2jOlBieJrd0LPDwJewInJy/U+Dr0B0+3D9Rg/hNgF4f67EohLdL0yGPNn W91zjoaVzQA6hEMUXIsMjWF5rVvVEsFoO/WbARujHX3AakaKP6rRhpNH4wafrOYZ2V3X P1sg== X-Forwarded-Encrypted: i=1; AFNElJ8XTsJHyPWbTnHH2GMZlD3xsgDRHp+ncmntLDfBurvwYZL0NpdZRxirEw6SJTjXY33S7MCmCW3exu4SeHs=@vger.kernel.org X-Gm-Message-State: AOJu0YxfzqABtCpRP+/b6T78hn2bE2sYkzTx/SrgUZlA4xcXB4DqihSv v3BqDZAc8ZfnunXUCn7f6Xkvpb/PHBOXQBkIsQxtszkAxsZAAI0pry1RBGeG2t8pT2duNNIB5bt HZiW1ejx8Oa4GaVF5ZdUhKYQxcgEDEL5+G02tKqoq/1FZRLBdk2T7kO24i8sLIwDVeQ== X-Gm-Gg: AeBDievnb4Q8vHz3YLNcUCN1ogSBoBzP3HimPZjBkMt4AImt8uuW4WnCtgJJJmS4pSn e0dBAN2bxlgWu0YT1gBZibJ/gkAorJDJQHSVtrGqrp8Vw9Pu0vVPd6VJEti1YjADNbGcD7KavwK 16H4g3efDEsQZN76ggTSV4gc/kQwwa20qK+grEY4Q0MJcMPspycnMRBLqJnblaeUOiYYCFD1d8j /jc9+PAb2h4oS/dw4zeqZzTL95TeO37Ug6XBAIfmc9SNQe7nC1d9aA4E3F/YPIKglmHvJv5BgJ6 6Tlvj407NoIH+faatJmQGgP1dT/mE3S3EFa5EsJw6Z+WjfsW5JAeTdXpRtG9UoLG32wZzICdA2B EgOgGFEw6yuypQz8CJ+3ztKlMzoBCni8= X-Received: by 2002:ac8:5ad6:0:b0:509:4406:44e0 with SMTP id d75a77b69052e-5102ab52a6fmr73290331cf.27.1777589486764; Thu, 30 Apr 2026 15:51:26 -0700 (PDT) X-Received: by 2002:ac8:5ad6:0:b0:509:4406:44e0 with SMTP id d75a77b69052e-5102ab52a6fmr73289921cf.27.1777589486342; Thu, 30 Apr 2026 15:51:26 -0700 (PDT) Received: from redhat.com ([2600:382:7701:f83c:fba1:9a93:4c64:db25]) by smtp.gmail.com with ESMTPSA id 6a1803df08f44-8b538b1d761sm4868636d6.9.2026.04.30.15.51.24 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 30 Apr 2026 15:51:25 -0700 (PDT) Date: Thu, 30 Apr 2026 18:51:23 -0400 From: Brian Masney To: Xuyang Dong Cc: Stephen Boyd , mturquette@baylibre.com, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, linux-clk@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, p.zabel@pengutronix.de, huangyifeng@eswincomputing.com, benoit.monin@bootlin.com, ningyu@eswincomputing.com, linmin@eswincomputing.com, pinkesh.vaghela@einfochips.com Subject: Re: Re: Re: Re: Re: [PATCH v3 2/3] clk: eswin: Add eic7700 HSP clock driver Message-ID: References: <20260423091114.2326-1-dongxuyang@eswincomputing.com> <4e5c887.5a31.19dbf179fb6.Coremail.dongxuyang@eswincomputing.com> <177733570840.5403.12558106273673899411@lazor> <7a76d8cb.5bab.19dd3645d4e.Coremail.dongxuyang@eswincomputing.com> <177742748214.5403.15526965667317467444@localhost.localdomain> <4257942f.5c6d.19dd89b06f8.Coremail.dongxuyang@eswincomputing.com> <1f0a2d11.5cd2.19ddcf8114a.Coremail.dongxuyang@eswincomputing.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1f0a2d11.5cd2.19ddcf8114a.Coremail.dongxuyang@eswincomputing.com> User-Agent: Mutt/2.3.1 (2026-03-20) Hi Xuyang, On Thu, Apr 30, 2026 at 01:58:58PM +0800, Xuyang Dong wrote: > > On Wed, Apr 29, 2026 at 05:38:51PM +0800, Xuyang Dong wrote: > > > > > > > > > > The common gate API, the HSP private API, and the reset driver all access  > > > > > the same register space. > > > > > Therefore, they need to be protected by the same data->lock. > > > > > > > > > > > > > If everything is accessing registers through regmap why aren't we using > > > > the builtin lock with struct regmap_config::use_raw_spinlock? I don't > > > > understand why we're rolling our own here. > > > > > > Hi Stephen, > > > > > > In the HSP clock driver and reset driver, there are three components that > > > access the HSP register space: a common gate clock, a custom gate clock  > > > (i.e., 0x800), and a reset. > > > > > > 1. The common gate uses eswin_clk_register_gate() to register a gate clock  > > > via devm_clk_hw_register_gate_parent_data(). It accesses the register  > > > using clk_gate_endisable(). > > > > > > static void clk_gate_endisable(struct clk_hw *hw, int enable) > > > { > > > struct clk_gate *gate = to_clk_gate(hw); > > > unsigned long flags; > > > > > > if (gate->lock) > > > spin_lock_irqsave(gate->lock, flags); > > > else > > > __acquire(gate->lock); > > > ... > > > if (gate->lock) > > > spin_unlock_irqrestore(gate->lock, flags); > > > else > > > __release(gate->lock); > > > } > > > > > > The gate->lock in use is the data->lock passed in from the clock driver. > > > > > > 2. The custom gate uses hsp_clk_register_gate() to register a gate clock. > > > It accesses the register using hsp_clk_gate_endisable(). > > > > > > static void hsp_clk_gate_endisable(struct clk_hw *hw, int enable) > > > { > > > struct eic7700_hsp_clk_gate *gate = to_gate_clk(hw); > > > > > > guard(spinlock_irqsave)(gate->lock); > > > ... > > > } > > > > > > The gate->lock in use is the same data->lock passed in from the clock  > > > driver. > > > > > > 3. The reset uses eic7700_hsp_reset_assert() and  > > > eic7700_hsp_reset_deassert(), which call regmap_assign_bits() to access  > > > the register. > > > > > > All three methods access the same register space; therefore, they must be  > > > protected by the same lock (data->lock). > > > > > > That's why we introduced eic7700_hsp_regmap_lock/unlock for  > > > eic7700_hsp_regmap_config. > > > eic7700_hsp_regmap_config = { > > > .lock = eic7700_hsp_regmap_lock, > > > .unlock = eic7700_hsp_regmap_unlock, > > > .lock_arg = lock_ctx, > > > }; > > > > > > The 'lock_ctx->lock' in eic7700_hsp_regmap_lock/unlock is the 'data->lock'. > > > static void eic7700_hsp_regmap_lock(void *arg) > > > __acquires(lock_ctx->lock) > > > { > > > struct eic7700_hsp_regmap_lock *const lock_ctx = arg; > > > unsigned long flags; > > > > > > spin_lock_irqsave(lock_ctx->lock, flags); > > > lock_ctx->flags = flags; > > > } > > > > > > The similar approach can be found in clk-imx8ulp-sim-lpav.c. > > > > > > The annotations what we mentioned previously is the above  > > > "__acquires(lock_ctx->lock)". > > > > I see what Stephen is saying. Take a look at __regmap_init() in > > drivers/base/regmap/regmap.c. If the lock/unlock ops are not specified, > > then the final else will automatically setup locking. By default, it'll > > use a mutex, but there is the ability to use a spinlock. > > > > So you can drop the lock/unlock ops from the driver, and add to the ops: > > > > fast_io: 1, > > use_raw_spinlock: 1, > > > > Given the critcal nature of clks, I agree with Stephen that a raw > > spinlock should be used here. > > > > Hi Stephen and Brian, > > In the HSP clock driver, hsp_clk_gate_endisable() only accesses the  > registers at 0x800/0x900, and reset accesses the same registers as well,  > which leads to concurrent RMW (read-modify-write) races. > > There are two approaches to solve these races. > > The first method is the current implementation. All three functions  > (clk_gate_endisable(), hsp_clk_gate_endisable(),  > and eic7700_hsp_reset_assert()) use data->lock to prevent concurrent  > RMW races. > > The second method is as Stephen said. If I understand correctly, it is to  > change the register read/write operations in hsp_clk_gate_endisable() to  > use the regmap API and use the same lock (map->raw_spinlock) as reset. > > Is the second approach preferable? Use the same regmap everywhere. Also you don't have to explicitly define the raw spinlock in your driver since the regmap API will create a raw spinlock for you if you use the fast_io / use_raw_spinlock options I described above. Brian