From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id D34B74300A; Tue, 8 Aug 2023 11:20:53 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6E5D043247; Tue, 8 Aug 2023 11:20:53 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id A899D40A87 for ; Tue, 8 Aug 2023 11:20:51 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1691486448; 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=MHqETY/NxEWooLVSkdaAMFgVKhEm3XBXFWv9EzEkjUg=; b=RTLfgLwuk9/8a7hlHOaDed2dil2wCzc73slAIaCYrIL91QiQkbZXxu0wa4yrbWfdkXBn92 tgvFX+fO4iYFJFX4EXMULfqIIpL0+/UzAqkNPK3tpIsEFOoR7MRaFZIQifQlJD+lDQpfkv +ChmZjTSQFv5u/nzK8VwXjWue/lML9o= Received: from mail-lf1-f70.google.com (mail-lf1-f70.google.com [209.85.167.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-231-Hw5llqWuMCuMo2tVMVlR-w-1; Tue, 08 Aug 2023 05:20:45 -0400 X-MC-Unique: Hw5llqWuMCuMo2tVMVlR-w-1 Received: by mail-lf1-f70.google.com with SMTP id 2adb3069b0e04-4fe157ebea0so5473818e87.1 for ; Tue, 08 Aug 2023 02:20:45 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1691486441; x=1692091241; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=MHqETY/NxEWooLVSkdaAMFgVKhEm3XBXFWv9EzEkjUg=; b=dfyOQJa+RJYoMyARYvnSjaeq7UVe4YAmzB6xbRSEUyuqvWbPXIUj0IBJk4JLpqo4Yg TlyWzVxD8dqYzcHhCgpuPAsRvZJabPXbXOsSLXTE5s3FcgGBJhNt0v3+KN1IZ7qGDQ2x y3xnQwjehCQBDFDt1vWTyk7jR0EtbB9H/b8j0TObgZmqvXRZLT8pRVqMOjdQ/wdhpEE8 hKb+TDLtIVbi9Zd6eVn8Il6pB1lxHupoZsg+b/OpGCX11mu8GfQs/0krkn6D662PBeCR pK3thlgZohMeh20io5tI3x1rrGpQ2NY9HX5FyiHdDRm54OGBpWwwWk0h0/QRq3KXu76k 9X1g== X-Gm-Message-State: AOJu0YyHAoJiMaAPENgfYZ4dQHeUJvib3OrWHln22+S/ten1DM9AH+5E IZEx0gZ7K4EYavTeRhkrrBo2/afQ9W7Dx1Dcf6DmHHTuopTLbWCIR0crMz/lYgLxYt3Dw+4QgNT Wx4qr+rljHQWVs+bfpbo= X-Received: by 2002:a05:6512:3ca6:b0:4fe:279b:7609 with SMTP id h38-20020a0565123ca600b004fe279b7609mr9553492lfv.17.1691486441495; Tue, 08 Aug 2023 02:20:41 -0700 (PDT) X-Google-Smtp-Source: AGHT+IF4YOJa0FBQGI93u6I/YSBPiG+0SJvwCDLNQdkQC+hogX3l61zFK+BbFh1VYCrVQBh1XUy8uuOdJqzsHmePyFU= X-Received: by 2002:a05:6512:3ca6:b0:4fe:279b:7609 with SMTP id h38-20020a0565123ca600b004fe279b7609mr9553472lfv.17.1691486441138; Tue, 08 Aug 2023 02:20:41 -0700 (PDT) MIME-Version: 1.0 References: <20230803075038.307012-1-david.marchand@redhat.com> <20230803075038.307012-7-david.marchand@redhat.com> In-Reply-To: From: David Marchand Date: Tue, 8 Aug 2023 11:20:28 +0200 Message-ID: Subject: Re: [PATCH 06/14] pci: define some command constants To: Bruce Richardson Cc: dev@dpdk.org, thomas@monjalon.net, ferruh.yigit@amd.com, chenbo.xia@intel.com, nipun.gupta@amd.com, Anatoly Burakov , Timothy McDaniel , Gaetan Rivet X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Thu, Aug 3, 2023 at 1:51=E2=80=AFPM David Marchand wrote: > > On Thu, Aug 3, 2023 at 11:58=E2=80=AFAM Bruce Richardson > wrote: > > > > On Thu, Aug 03, 2023 at 09:50:29AM +0200, David Marchand wrote: > > > Define some PCI command constants and use them in existing drivers. > > > > > > Signed-off-by: David Marchand > > > --- > > > > For the idea, all good, but for the implementation, one comment inline > > below. > > > > With the below reworked: > > > > Acked-by: Bruce Richardson > > > > > drivers/bus/pci/linux/pci_vfio.c | 8 ++++---- > > > drivers/event/dlb2/pf/dlb2_main.c | 8 +++----- > > > lib/pci/rte_pci.h | 4 +++- > > > 3 files changed, 10 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux= /pci_vfio.c > > > index 6d13cafdcf..f96b3ce7fb 100644 > > > --- a/drivers/bus/pci/linux/pci_vfio.c > > > +++ b/drivers/bus/pci/linux/pci_vfio.c > > > @@ -156,18 +156,18 @@ pci_vfio_enable_bus_memory(struct rte_pci_devic= e *dev, int dev_fd) > > > return -1; > > > } > > > > > > - ret =3D pread64(dev_fd, &cmd, sizeof(cmd), offset + PCI_COMMAND= ); > > > + ret =3D pread64(dev_fd, &cmd, sizeof(cmd), offset + RTE_PCI_COM= MAND); > > > > > > if (ret !=3D sizeof(cmd)) { > > > RTE_LOG(ERR, EAL, "Cannot read command from PCI config = space!\n"); > > > return -1; > > > } > > > > > > - if (cmd & PCI_COMMAND_MEMORY) > > > + if (cmd & RTE_PCI_COMMAND_MEMORY) > > > return 0; > > > > > > - cmd |=3D PCI_COMMAND_MEMORY; > > > - ret =3D pwrite64(dev_fd, &cmd, sizeof(cmd), offset + PCI_COMMAN= D); > > > + cmd |=3D RTE_PCI_COMMAND_MEMORY; > > > + ret =3D pwrite64(dev_fd, &cmd, sizeof(cmd), offset + RTE_PCI_CO= MMAND); > > > > > > if (ret !=3D sizeof(cmd)) { > > > RTE_LOG(ERR, EAL, "Cannot write command to PCI config s= pace!\n"); > > > diff --git a/drivers/event/dlb2/pf/dlb2_main.c b/drivers/event/dlb2/p= f/dlb2_main.c > > > index c6606a9bee..6dbaa2ff97 100644 > > > --- a/drivers/event/dlb2/pf/dlb2_main.c > > > +++ b/drivers/event/dlb2/pf/dlb2_main.c > > > @@ -33,7 +33,6 @@ > > > #define DLB2_PCI_EXP_DEVCTL2 40 > > > #define DLB2_PCI_LNKCTL2 48 > > > #define DLB2_PCI_SLTCTL2 56 > > > -#define DLB2_PCI_CMD 4 > > > #define DLB2_PCI_EXP_DEVSTA 10 > > > #define DLB2_PCI_EXP_DEVSTA_TRPND 0x20 > > > #define DLB2_PCI_EXP_DEVCTL_BCR_FLR 0x8000 > > > @@ -47,7 +46,6 @@ > > > #define DLB2_PCI_ERR_ROOT_STATUS 0x30 > > > #define DLB2_PCI_ERR_COR_STATUS 0x10 > > > #define DLB2_PCI_ERR_UNCOR_STATUS 0x4 > > > -#define DLB2_PCI_COMMAND_INTX_DISABLE 0x400 > > > #define DLB2_PCI_ACS_CAP 0x4 > > > #define DLB2_PCI_ACS_CTRL 0x6 > > > #define DLB2_PCI_ACS_SV 0x1 > > > @@ -286,7 +284,7 @@ dlb2_pf_reset(struct dlb2_dev *dlb2_dev) > > > > > > /* clear the PCI command register before issuing the FLR */ > > > > > > - off =3D DLB2_PCI_CMD; > > > + off =3D RTE_PCI_COMMAND; > > > cmd =3D 0; > > > if (rte_pci_write_config(pdev, &cmd, 2, off) !=3D 2) { > > > DLB2_LOG_ERR("[%s()] failed to write the pci command\n"= , > > > @@ -468,9 +466,9 @@ dlb2_pf_reset(struct dlb2_dev *dlb2_dev) > > > } > > > } > > > > > > - off =3D DLB2_PCI_CMD; > > > + off =3D RTE_PCI_COMMAND; > > > if (rte_pci_read_config(pdev, &cmd, 2, off) =3D=3D 2) { > > > - cmd &=3D ~DLB2_PCI_COMMAND_INTX_DISABLE; > > > + cmd &=3D ~RTE_PCI_COMMAND_INTX_DISABLE; > > > if (rte_pci_write_config(pdev, &cmd, 2, off) !=3D 2) { > > > DLB2_LOG_ERR("[%s()] failed to write the pci co= mmand\n", > > > __func__); > > > diff --git a/lib/pci/rte_pci.h b/lib/pci/rte_pci.h > > > index a055a28592..bf2b2639f4 100644 > > > --- a/lib/pci/rte_pci.h > > > +++ b/lib/pci/rte_pci.h > > > @@ -32,10 +32,12 @@ extern "C" { > > > > > > #define RTE_PCI_VENDOR_ID 0x00 /* 16 bits */ > > > #define RTE_PCI_DEVICE_ID 0x02 /* 16 bits */ > > > -#define RTE_PCI_COMMAND 0x04 /* 16 bits */ > > > > > > /* PCI Command Register */ > > > +#define RTE_PCI_COMMAND 0x04 /* 16 bits */ > > > +#define RTE_PCI_COMMAND_MEMORY 0x2 /* Enable response in M= emory space */ > > > #define RTE_PCI_COMMAND_MASTER 0x4 /* Bus Master Enable */ > > > +#define RTE_PCI_COMMAND_INTX_DISABLE 0x400 /* INTx Emulation Disable= */ > > > > > > > It's weird here to have two defines in the same block with the same val= ue - > > even if one is 0x04 and the other is 0x4. That implies that the two val= ues > > don't belong in the same block of defines. > > > > Question - would enums make sense for defining any of these? It allows > > the groups to be named? > > That's something I will look into, or at least better organise the define= s.. At least, using enum would be a (minor) change in the API, as macros were used so far and maybe some users were relying on them (#ifdef / #ifndef). I relooked at linux/pci_regs.h which organises those defines a bit as I did, but with some kind of indentation (double space) to separate the first offset of a block and the rest of the defines related to said block of defines/registers. Example: #define PCI_COMMAND 0x04 /* 16 bits */ #define PCI_COMMAND_IO 0x1 /* Enable response in I/O space */ #define PCI_COMMAND_MEMORY 0x2 /* Enable response in Memory space = */ #define PCI_COMMAND_MASTER 0x4 /* Enable bus mastering */ #define PCI_COMMAND_SPECIAL 0x8 /* Enable response to special cycle= s */ --=20 David Marchand