Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210210101535.47979-1-alexandru.ardelean@analog.com> <YCSryh6kOhA+0xHc@epycbox.lan>
In-Reply-To: <YCSryh6kOhA+0xHc@epycbox.lan>
From:   Alexandru Ardelean <ardeleanalex@gmail.com>
Date:   Thu, 11 Feb 2021 07:32:09 +0200
Message-ID: <CA+U=Dso5uCyjd8v1Xo5iWsUacchfbtuR+6NMgivEAv9TZNezcA@mail.gmail.com>
Subject: Re: [PATCH 1/2] include: fpga: adi-axi-common.h: add definitions for
 supported FPGAs
To:     Moritz Fischer <mdf@kernel.org>
Cc:     Alexandru Ardelean <alexandru.ardelean@analog.com>,
        linux-clk@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        linux-fpga@vger.kernel.org,
        Mircea Caprioru <mircea.caprioru@analog.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Feb 11, 2021 at 6:00 AM Moritz Fischer <mdf@kernel.org> wrote:
>
> Alexandru,
>
> On Wed, Feb 10, 2021 at 12:15:34PM +0200, Alexandru Ardelean wrote:
> > From: Mircea Caprioru <mircea.caprioru@analog.com>
> >
> > All (newer) FPGA IP cores supported by Analog Devices, store informatio=
n in
>
> Nit: extra ',' ?
> > the synthesized designs. This information describes various parameters,
> > including the family of boards on which this is deployed, speed-grade, =
and
> > so on.
> >
> > Currently, some of these definitions are deployed mostly on Xilinx boar=
ds,
> > but they have been considered also for FPGA boards from other vendors.
> Let's add them together with the code that uses them.
> >
> > The register definitions are described at this link:
> >   https://wiki.analog.com/resources/fpga/docs/hdl/regmap
> > (the 'Base (common to all cores)' section).
> >
> > Acked-by: Moritz Fischer <mdf@kernel.org>
> This patchset is very different from the reviewed one earlier. Please
> don't just copy Acked-by's.

Apologies
I think it got some more reviews and I just kept the tag.
Sloppy on my part.

>
> > Signed-off-by: Mircea Caprioru <mircea.caprioru@analog.com>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >
> > This is a continuation of this old set:
> > https://lore.kernel.org/linux-clk/20200929144417.89816-1-alexandru.arde=
lean@analog.com/
> >
> > Particularly patches:
> >   https://lore.kernel.org/linux-clk/20200929144417.89816-15-alexandru.a=
rdelean@analog.com/
> >   https://lore.kernel.org/linux-clk/20200929144417.89816-16-alexandru.a=
rdelean@analog.com/
> >
> > That was v4, but this patchset was split away from it, to resolve
> > discussion on some other patches in that set.
> >
> > The other patches were accepted here:
> >   https://lore.kernel.org/linux-clk/20210201151245.21845-1-alexandru.ar=
delean@analog.com/
> >
> >  include/linux/fpga/adi-axi-common.h | 103 ++++++++++++++++++++++++++++
> >  1 file changed, 103 insertions(+)
> >
> > diff --git a/include/linux/fpga/adi-axi-common.h b/include/linux/fpga/a=
di-axi-common.h
> > index 141ac3f251e6..1a7f18e3a384 100644
> > --- a/include/linux/fpga/adi-axi-common.h
> > +++ b/include/linux/fpga/adi-axi-common.h
> > @@ -13,6 +13,9 @@
> >
> >  #define ADI_AXI_REG_VERSION                  0x0000
> >
> > +#define ADI_AXI_REG_FPGA_INFO                        0x001C
> > +#define ADI_AXI_REG_FPGA_VOLTAGE             0x0140
> > +
> >  #define ADI_AXI_PCORE_VER(major, minor, patch)       \
> >       (((major) << 16) | ((minor) << 8) | (patch))
> >
> > @@ -20,4 +23,104 @@
> >  #define ADI_AXI_PCORE_VER_MINOR(version)     (((version) >> 8) & 0xff)
> >  #define ADI_AXI_PCORE_VER_PATCH(version)     ((version) & 0xff)
> >
> > +#define ADI_AXI_INFO_FPGA_VOLTAGE(val)               ((val) & 0xffff)
> > +
> > +#define ADI_AXI_INFO_FPGA_TECH(info)         (((info) >> 24) & 0xff)
> > +#define ADI_AXI_INFO_FPGA_FAMILY(info)               (((info) >> 16) &=
 0xff)
> > +#define ADI_AXI_INFO_FPGA_SPEED_GRADE(info)  (((info) >> 8) & 0xff)
> > +#define ADI_AXI_INFO_FPGA_DEV_PACKAGE(info)  ((info) & 0xff)
>
> Do we really need all the macros?

No.
I can trim them to a minimum.

> > +
> > +/**
> > + * FPGA Technology definitions
> > + */
> > +#define ADI_AXI_FPGA_TECH_XILINX_UNKNOWN             0
> > +#define ADI_AXI_FPGA_TECH_XILINS_SERIES7             1
> > +#define ADI_AXI_FPGA_TECH_XILINX_ULTRASCALE          2
> > +#define ADI_AXI_FPGA_TECH_XILINX_ULTRASCALE_PLUS     3
> > +
> > +#define ADI_AXI_FPGA_TECH_INTEL_UNKNOWN                      100
> > +#define ADI_AXI_FPGA_TECH_INTEL_CYCLONE_5            101
> > +#define ADI_AXI_FPGA_TECH_INTEL_CYCLONE_10           102
> > +#define ADI_AXI_FPGA_TECH_INTEL_ARRIA_10             103
> > +#define ADI_AXI_FPGA_TECH_INTEL_STRATIX_10           104
> > +
> > +/**
> > + * FPGA Family definitions
> > + */
> > +#define ADI_AXI_FPGA_FAMILY_UNKNOWN                  0
> > +
> > +#define ADI_AXI_FPGA_FAMILY_XILINX_ARTIX             1
> > +#define ADI_AXI_FPGA_FAMILY_XILINX_KINTEX            2
> > +#define ADI_AXI_FPGA_FAMILY_XILINX_VIRTEX            3
> > +#define ADI_AXI_FPGA_FAMILY_XILINX_ZYNQ                      4
> > +
> > +#define ADI_AXI_FPGA_FAMILY_INTEL_SX                 1
> > +#define ADI_AXI_FPGA_FAMILY_INTEL_GX                 2
> > +#define ADI_AXI_FPGA_FAMILY_INTEL_GT                 3
> > +#define ADI_AXI_FPGA_FAMILY_INTEL_GZ                 4
> > +
> > +/**
> > + * FPGA Speed-grade definitions
> > + */
> > +#define ADI_AXI_FPGA_SPEED_GRADE_UNKNOWN             0
> > +
> > +#define ADI_AXI_FPGA_SPEED_GRADE_XILINX_1            10
> > +#define ADI_AXI_FPGA_SPEED_GRADE_XILINX_1L           11
> > +#define ADI_AXI_FPGA_SPEED_GRADE_XILINX_1H           12
> > +#define ADI_AXI_FPGA_SPEED_GRADE_XILINX_1HV          13
> > +#define ADI_AXI_FPGA_SPEED_GRADE_XILINX_1LV          14
> > +#define ADI_AXI_FPGA_SPEED_GRADE_XILINX_2            20
> > +#define ADI_AXI_FPGA_SPEED_GRADE_XILINX_2L           21
> > +#define ADI_AXI_FPGA_SPEED_GRADE_XILINX_2LV          22
> > +#define ADI_AXI_FPGA_SPEED_GRADE_XILINX_3            30
> > +
> > +#define ADI_AXI_FPGA_SPEED_GRADE_INTEL_1             1
> > +#define ADI_AXI_FPGA_SPEED_GRADE_INTEL_2             2
> > +#define ADI_AXI_FPGA_SPEED_GRADE_INTEL_3             3
> > +#define ADI_AXI_FPGA_SPEED_GRADE_INTEL_4             4
> > +#define ADI_AXI_FPGA_SPEED_GRADE_INTEL_5             5
> > +#define ADI_AXI_FPGA_SPEED_GRADE_INTEL_6             6
> > +#define ADI_AXI_FPGA_SPEED_GRADE_INTEL_7             7
> > +#define ADI_AXI_FPGA_SPEED_GRADE_INTEL_8             8
> > +#define ADI_AXI_FPGA_SPEED_GRADE_INTEL_9             9
> > +
> > +/**
> > + * FPGA Device Package definitions
> > + */
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_UNKNOWN             0
> > +
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_XILINX_RF           1
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_XILINX_FL           2
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_XILINX_FF           3
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_XILINX_FB           4
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_XILINX_HC           5
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_XILINX_FH           6
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_XILINX_CS           7
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_XILINX_CP           8
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_XILINX_FT           9
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_XILINX_FG           10
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_XILINX_SB           11
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_XILINX_RB           12
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_XILINX_RS           13
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_XILINX_CL           14
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_XILINX_SF           15
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_XILINX_BA           16
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_XILINX_FA           17
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_XILINX_FS           18
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_XILINX_FI           19
> > +
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_INTEL_BGA           1
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_INTEL_PGA           2
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_INTEL_FBGA          3
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_INTEL_HBGA          4
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_INTEL_PDIP          5
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_INTEL_EQFP          6
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_INTEL_PLCC          7
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_INTEL_PQFP          8
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_INTEL_RQFP          9
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_INTEL_TQFP          10
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_INTEL_UBGA          11
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_INTEL_UFBGA         12
> > +#define ADI_AXI_FPGA_DEV_PACKAGE_INTEL_MBGA          13
>
> What is using those? Do these package impact anything behavioral?

So, these should get defined in ADI FPGA projects.
The initial idea was to be able to read this in drivers and adjust
behavior (usually speed) according to each parameter.
Then some drivers could have used this to do some tunings.
Maybe also pass these in userspace and have the application decide.
I don't think there was some finalization on the idea (yet).

[I know how this sounds, but] The more I try to explain it, the less I
get convinced by this patch.
I pick up some of the patches in our tree and try to make sense of
them through upstreaming.
Sometimes when I pick them, it's not always obvious to me whether
they're worth it or not.
=C2=AF\_(=E3=83=84)_/=C2=AF


> > +
> >  #endif /* ADI_AXI_COMMON_H_ */
> > --
> > 2.17.1
> >
>
> - Moritz
>
> PS: The subject line could use a bit of work, too :)

Will think about it :)