From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f52.google.com (mail-wm1-f52.google.com [209.85.128.52]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 147EC26E6E1 for ; Wed, 26 Nov 2025 17:22:13 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.52 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764177736; cv=none; b=iBi4ww79VojYJ2P+fK8svwfes+Oh1g80EPDf8DdX7W1mWBSh/nIMinOteVgwryIwrWtux2PAhQdmI4k71lfJLychO8VodPmlk41Y8n4dL6WUsubHREpbj08Blv5wrQ96zYMyEopsYyiF1GGfJ8N5E/FPxsi/sVdxuagrK9qpJ38= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1764177736; c=relaxed/simple; bh=wOW9mr8lkvb1dXWEhI4QVzanyGbtM+zavyrY1/ZkNe4=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=p2/rBJ/lsZzYSjhpgBBF0nPF1odU1aCfTO8jfSJyFyXiiGeFOoY6O1qfUFT5UU8wiFlT/R9KDgbc9NamIqhju9rGiMgko15LiDaKSs3azdfRGBCxEMoNIAwkmH6xLm0tik1wGonJYF96Q3TGp48zeTL5Wa66HJDsKsUFQ/SY+tM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=tuxon.dev; spf=pass smtp.mailfrom=tuxon.dev; dkim=pass (2048-bit key) header.d=tuxon.dev header.i=@tuxon.dev header.b=NAsy67xM; arc=none smtp.client-ip=209.85.128.52 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=tuxon.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=tuxon.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=tuxon.dev header.i=@tuxon.dev header.b="NAsy67xM" Received: by mail-wm1-f52.google.com with SMTP id 5b1f17b1804b1-477ba2c1ca2so73961115e9.2 for ; Wed, 26 Nov 2025 09:22:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tuxon.dev; s=google; t=1764177732; x=1764782532; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:content-language:from :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=iMbAlPYdoISwPLiWW5sRnyteylPcxj4Xm9aY2it0S+E=; b=NAsy67xMd2XMwexj7wKUsyD+WCvDYM8wrNYzOK8dManO7VtZc9mgyN/9kqEWqrYS41 yKGDmy0l7D9HI2R1zAqYhIamolDIajtSjz3HSZWMflJwDlU+CSqxhAq3+1MPdbqSWvdJ C6ZmG7GMVtDdcO+8e6BjDZpw41m5dPpcwMFTVIfytpaLAArPNc+zqMWZshGfkMcDzwn8 GeDJ5RMKOJKrS40wYMsj1/ThQpvzRNBMFKl4zqZQweMxmRYBEiCM4OnfNzwhbQSYEbtY uZxEkcYyT0Mujod8s6QewbQprGSU0H2uwTsObNH6HEvYx9e0yCbU1S52tEORUWjwJ29w a4bw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1764177732; x=1764782532; h=content-transfer-encoding:in-reply-to:content-language:from :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=iMbAlPYdoISwPLiWW5sRnyteylPcxj4Xm9aY2it0S+E=; b=eB5WphXpKoAgNzWbOtqDrSUTc4ctSoP3KRPlWiesYx5zm+rcOAy3d3r7fWNiJdSt1S aQ2xPDIaTgmeB19xOZ9UvjF+6LjSKNlMtARj+w7dox6oh8moh4KusI+SP74u0YWJPt3H hhNQDsCgSnHl3mIYfzDlBew3XXI0oiAIWWJVDZVcGZZpPiDRZM3+doR13LSusqDsUdqg DfYRruu5AcK659NjaYmhZrlUl0ObQUw5JKtKOiadscQcZvBXYw1+K2+OicBajaNpRUYz /CukrqAphjAaBzvBuLDyuHqJSbr9VJ8RSpImwU4FKhYoUl3FNW7fo6Lj4O9IcPT7HUOF 0I/w== X-Forwarded-Encrypted: i=1; AJvYcCXSe9q/1bjdpWFtHLenSbWCDWdNyeiB/Cvshp7rKFZXky2UEh+7AxqqLj3oCWfbReuGutVClrdvQdre@vger.kernel.org X-Gm-Message-State: AOJu0Yz2RGD1/ep9hTmq6sptxFWsPV3hxMezSLHzv7728B1xgGvvhmW/ cWY9djsarZw6ji5wTHNad1RCJZ58F/FVlxC2bt9GGn6XIEIqC11XEEY/KZlPAhZxvJM= X-Gm-Gg: ASbGncvXxHjzGhCGb5PtLSOsCkEGbmEOsO7NG7zROx+UTBxt86HWOjt73A+MbznbFLx lSSf50++Kqddz6520rXb/iHfU3zDIhIk8rJrJKAQHily9BTKv4B1BgDxMxm+82toYggEJtysOZE w5e2Z3/8ksqKOt2Tb5H07XiNP3MdE5NP7mW8YyLG2NEWKq2jrBgDP8sO0dejv+FigDcLrID/ze5 20hkkZ+czWiZXrd/wcCAvLo5CiLfAUQdApn3W+0V0X5l5JzYAKwyNp9F4Y3aLR8Z8UhqGHSvzIG lZQi4VJnVAjUfGlcjxCFY1a916UmaV9EjZjMIAUC5rgGVBYZwmRoNmrWp2hEVnc8ud9QyjL22zK dVeaiNCRlZtJaSbvW+GZon5hydqI3bCpc8mCn/E7iQhHrAtyl2jPKZSkFJ3/DEpaH7K8KRWgbPT egB+YGt6upTIECSYRdWPQ= X-Google-Smtp-Source: AGHT+IFveJWow1MlKZhvBvDFWET9HTrn614Fkdf0VqG8AOTAuNxSS7l7a/cBAeHUgCNj8XRXHc1RFQ== X-Received: by 2002:a05:600c:1f0f:b0:477:1af2:f40a with SMTP id 5b1f17b1804b1-47904b1b27bmr75298265e9.17.1764177731802; Wed, 26 Nov 2025 09:22:11 -0800 (PST) Received: from [192.168.50.4] ([82.78.167.134]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4790b0c44dcsm52202665e9.11.2025.11.26.09.22.10 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 26 Nov 2025 09:22:11 -0800 (PST) Message-ID: <2e7ecbc0-6ce5-403a-b794-93aaff1ddf39@tuxon.dev> Date: Wed, 26 Nov 2025 19:22:09 +0200 Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v8 2/6] PCI: rzg3s-host: Add Renesas RZ/G3S SoC host driver To: Bjorn Helgaas Cc: bhelgaas@google.com, lpieralisi@kernel.org, kwilczynski@kernel.org, mani@kernel.org, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, geert+renesas@glider.be, magnus.damm@gmail.com, p.zabel@pengutronix.de, linux-pci@vger.kernel.org, linux-renesas-soc@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Claudiu Beznea , Wolfram Sang References: <20251125183754.GA2755815@bhelgaas> From: Claudiu Beznea Content-Language: en-US In-Reply-To: <20251125183754.GA2755815@bhelgaas> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hi, Bjorn, On 11/25/25 20:37, Bjorn Helgaas wrote: > On Wed, Nov 19, 2025 at 04:35:19PM +0200, Claudiu wrote: >> From: Claudiu Beznea >> >> The Renesas RZ/G3S features a PCIe IP that complies with the PCI Express >> Base Specification 4.0 and supports speeds of up to 5 GT/s. It functions >> only as a root complex, with a single-lane (x1) configuration. The >> controller includes Type 1 configuration registers, as well as IP >> specific registers (called AXI registers) required for various adjustments. > >> +/* Serialization is provided by 'pci_lock' in drivers/pci/access.c */ >> +static int rzg3s_pcie_root_write(struct pci_bus *bus, unsigned int devfn, >> + int where, int size, u32 val) >> +{ >> + struct rzg3s_pcie_host *host = bus->sysdata; >> + int ret; >> + >> + /* Enable access control to the CFGU */ >> + writel_relaxed(RZG3S_PCI_PERM_CFG_HWINIT_EN, >> + host->axi + RZG3S_PCI_PERM); > > I suppose this has been asked and answered already, but it's curious > that you need this for config writes but not for reads. Obviously it > must *work*, but it's unusual and might warrant a comment. "Access > control" must be a hint, but only means something to experts. After initialization, some PCI registers are read only. To enable write access to these registers after initialization, the access control need to be enabled. This is the quote from the HW manual: "Some registers with the RO attribute stated in the PCI Express Base Specification are writable at the time of initialization. When writing to these registers, CFG_HWINIT_EN (Permission Register (offset: H’300) bit[2]) must be set to 1b." > >> + ret = pci_generic_config_write(bus, devfn, where, size, val); >> + >> + /* Disable access control to the CFGU */ >> + writel_relaxed(0, host->axi + RZG3S_PCI_PERM); >> + >> + return ret; >> +} > >> +static irqreturn_t rzg3s_pcie_msi_irq(int irq, void *data) >> +{ >> + u8 regs = RZG3S_PCI_MSI_INT_NR / RZG3S_PCI_MSI_INT_PER_REG; >> + DECLARE_BITMAP(bitmap, RZG3S_PCI_MSI_INT_NR); >> + struct rzg3s_pcie_host *host = data; >> + struct rzg3s_pcie_msi *msi = &host->msi; >> + unsigned long bit; >> + u32 status; >> + >> + status = readl_relaxed(host->axi + RZG3S_PCI_PINTRCVIS); >> + if (!(status & RZG3S_PCI_PINTRCVIS_MSI)) >> + return IRQ_NONE; >> + >> + /* Clear the MSI */ >> + rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_PINTRCVIS, >> + RZG3S_PCI_PINTRCVIS_MSI, >> + RZG3S_PCI_PINTRCVIS_MSI); > > Other writes to RZG3S_PCI_PINTRCVIS are guarded by host->hw_lock. Is this > one safe without it? It should be safe as RZG3S_PCI_PINTRCVIS is a R/W1C type of register. HW manual describes R/W1C registers for PCIe as "Write-1-to-clear status . It can be cleared to 0b by writing 1b with a readable register. Writing 0b does not change anything." With this, it should be safe to drop the guard from rzg3s_pcie_intx_irq_ack(). > >> + rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_MSGRCVIS, >> + RZG3S_PCI_MSGRCVIS_MRI, RZG3S_PCI_MSGRCVIS_MRI); >> + >> + for (u8 reg_id = 0; reg_id < regs; reg_id++) { >> + status = readl_relaxed(host->axi + RZG3S_PCI_MSIRS(reg_id)); >> + bitmap_write(bitmap, status, reg_id * RZG3S_PCI_MSI_INT_PER_REG, >> + RZG3S_PCI_MSI_INT_PER_REG); >> + } >> + >> + for_each_set_bit(bit, bitmap, RZG3S_PCI_MSI_INT_NR) { >> + int ret; >> + >> + ret = generic_handle_domain_irq(msi->domain, bit); >> + if (ret) { >> + u8 reg_bit = bit % RZG3S_PCI_MSI_INT_PER_REG; >> + u8 reg_id = bit / RZG3S_PCI_MSI_INT_PER_REG; >> + >> + /* Unknown MSI, just clear it */ >> + writel_relaxed(BIT(reg_bit), >> + host->axi + RZG3S_PCI_MSIRS(reg_id)); > > Other writes to RZG3S_PCI_MSIRS are guarded by host->hw_lock. Is this > one safe without it? RZG3S_PCI_MSIRS is also a R/W1C type of register. With it, it should be safe to drop the guard from rzg3s_pcie_msi_irq_ack() as well. I'm going to prepare a follow up patch to drop the guard on rzg3s_pcie_intx_irq_ack() and rzg3s_pcie_msi_irq_ack(). Please let me know if you have something against. I can also prepare a patch to detail in a comment the "enable access control to the CFGU" operation in rzg3s_pcie_root_write(), if you prefer. Thank you for your review, Claudiu