Received-SPF: pass (google.com: domain of linux-kernel+bounces-46725-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 31 Jan 2024 09:37:48 -0600
From: Bjorn Helgaas <helgaas@kernel.org>
To: Frank Li <Frank.Li@nxp.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>, Jingoo Han <jingoohan1@gmail.com>,
	Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>, imx@lists.linux.dev,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
	Serge Semin <fancer.lancer@gmail.com>
Subject: Re: [PATCH 1/6] PCI: Add INTx Mechanism Messages macros
Message-ID: <20240131153748.GA586693@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240130-pme_msg-v1-1-d52b0add5c7c@nxp.com>

On Tue, Jan 30, 2024 at 07:45:26PM -0500, Frank Li wrote:
> From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> Add "Message Routing" and "INTx Mechanism Messages" macros to enable
> a PCIe driver to send messages for INTx Interrupt Signaling.
> 
> The "Message Routing" is from Table 2-17, and the "INTx Mechanism
> Messages" is from Table 2-18 on the PCI Express Base Specification,
> Rev. 4.0 Version 1.0.

Please cite a newer spec revision, e.g., PCIe r6.0 or r6.1.

Also, please cite section numbers instead of table numbers because the
table numbers are hard to find (you can't navigate to them from
"Contents") and they change a lot between spec revisions.  "INTx
Mechanism Messages" is Table 2-21 in r6.0, but it's in sec 2.2.8.1 in
both r4.0 and r6.0.

> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>

With these updates:

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  drivers/pci/pci.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 2336a8d1edab2..fe42f5d10b010 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -22,6 +22,24 @@
>   */
>  #define PCIE_PME_TO_L2_TIMEOUT_US	10000
>  
> +/* Message Routing (r[2:0]) */

Add citation to the comment: "PCIe r6.0, sec 2.2.8"

> +#define PCI_MSG_TYPE_R_RC	0

I think I would prefix all these with "PCIE" instead of "PCI", since
they are specific to PCIe and we already use "PCIE" for some of the
PCIe-specific timeouts.

> +#define PCI_MSG_TYPE_R_ADDR	1
> +#define PCI_MSG_TYPE_R_ID	2
> +#define PCI_MSG_TYPE_R_BC	3
> +#define PCI_MSG_TYPE_R_LOCAL	4
> +#define PCI_MSG_TYPE_R_GATHER	5
> +
> +/* INTx Mechanism Messages */

Add "PCIe r6.0, sec 2.2.8.1"

> +#define PCI_MSG_CODE_ASSERT_INTA	0x20
> +#define PCI_MSG_CODE_ASSERT_INTB	0x21
> +#define PCI_MSG_CODE_ASSERT_INTC	0x22
> +#define PCI_MSG_CODE_ASSERT_INTD	0x23
> +#define PCI_MSG_CODE_DEASSERT_INTA	0x24
> +#define PCI_MSG_CODE_DEASSERT_INTB	0x25
> +#define PCI_MSG_CODE_DEASSERT_INTC	0x26
> +#define PCI_MSG_CODE_DEASSERT_INTD	0x27
> +
>  extern const unsigned char pcie_link_speed[];
>  extern bool pci_early_dump;
>  
> 
> -- 
> 2.34.1
>