From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; 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 <dev@dpdk.org>; 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>
<ZMt6F6dPy0t60C4j@bricha3-MOBL.ger.corp.intel.com>
<CAJFAV8wHFUrh9D3XTkzGXXj5Tfi7GVhm5Or37K9W56Uerp2V=w@mail.gmail.com>
In-Reply-To: <CAJFAV8wHFUrh9D3XTkzGXXj5Tfi7GVhm5Or37K9W56Uerp2V=w@mail.gmail.com>
From: David Marchand <david.marchand@redhat.com>
Date: Tue, 8 Aug 2023 11:20:28 +0200
Message-ID: <CAJFAV8yPsubfMSRekgq_JxRaOoZHT165jVEJUPBw+nWO_3BwbA@mail.gmail.com>
Subject: Re: [PATCH 06/14] pci: define some command constants
To: Bruce Richardson <bruce.richardson@intel.com>
Cc: dev@dpdk.org, thomas@monjalon.net, ferruh.yigit@amd.com,
chenbo.xia@intel.com, nipun.gupta@amd.com,
Anatoly Burakov <anatoly.burakov@intel.com>,
Timothy McDaniel <timothy.mcdaniel@intel.com>,
Gaetan Rivet <grive@u256.net>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
On Thu, Aug 3, 2023 at 1:51=E2=80=AFPM David Marchand <david.marchand@redha=
t.com> wrote:
>
> On Thu, Aug 3, 2023 at 11:58=E2=80=AFAM Bruce Richardson
> <bruce.richardson@intel.com> 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 <david.marchand@redhat.com>
> > > ---
> >
> > For the idea, all good, but for the implementation, one comment inline
> > below.
> >
> > With the below reworked:
> >
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> >
> > > 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