Re: [PATCH v4 4/4] PCI: mediatek-gen3: Add Airoha EN7581 support

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Nov 06, 2024 at 06:00:08PM -0500, Jim Quinlan wrote:
> On Tue, Nov 5, 2024 at 4:33 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Wed, Jul 03, 2024 at 06:12:44PM +0200, Lorenzo Bianconi wrote:
> > > Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3
> > > PCIe controller driver.
> >
> > > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> >
> > > +#define PCIE_EQ_PRESET_01_REG                0x100
> > > +#define PCIE_VAL_LN0_DOWNSTREAM              GENMASK(6, 0)
> > > +#define PCIE_VAL_LN0_UPSTREAM                GENMASK(14, 8)
> > > +#define PCIE_VAL_LN1_DOWNSTREAM              GENMASK(22, 16)
> > > +#define PCIE_VAL_LN1_UPSTREAM                GENMASK(30, 24)
> > > ...
> >
> > > +static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
> > > +{
> > > ...
> >
> > > +     val = FIELD_PREP(PCIE_VAL_LN0_DOWNSTREAM, 0x47) |
> > > +           FIELD_PREP(PCIE_VAL_LN1_DOWNSTREAM, 0x47) |
> > > +           FIELD_PREP(PCIE_VAL_LN0_UPSTREAM, 0x41) |
> > > +           FIELD_PREP(PCIE_VAL_LN1_UPSTREAM, 0x41);
> > > +     writel_relaxed(val, pcie->base + PCIE_EQ_PRESET_01_REG);
> 
> Not sure it is worth the trouble to define fields.  In fact, you are
> already combining fields (rec+trans) so why not go further and just
> write each lane as a u16?
> >
> > This looks like it might be for the Lane Equalization Control
> > registers (PCIe r6.0, sec 7.7.3.4)?
> >
> > I would expect those values (0x47, 0x41) to be related to the platform
> > design, so maybe not completely determined by the SoC itself?  Jim and
> > Krishna have been working on DT schema for the equalization values,
> > which seems like the right place for them:
> >
> > https://lore.kernel.org/linux-pci/20241018182247.41130-2-james.quinlan@broadcom.com/
> > https://lore.kernel.org/r/77d3a1a9-c22d-0fd3-5942-91b9a3d74a43@quicinc.com
> >
> > Maybe that would be applicable here as well?  It would at least be
> > nice to use a common #define for the Lane Equalization Control
> > register offset from the capability base.
> 
> FWIW, these registers are HwInit/RO.  In our (Broadcom) case we have
> to write them using an internal register block that is not visible in
> the config space.  In other words, we do not use the cap offset.

Good point.  It looks like they're a mix of HwInit/RsvdP and
Hwinit/RO.  RsvdP is for writes, so I guess the config space registers
must be write-once and subsequently read-only until reset.  In any
case, mtk is using an internal register block as well, so a cap offset
wouldn't be useful.

Maybe it would still be worthwhile to define the fields themselves in
pci_regs.h so we can someday have common code to parse the DT
properties and assemble them.  Although I suppose there's no
requirement that the registers in the internal block even be laid out
the same as the config space register.

> > Although I see that no such #define exists in pci_regs.h, so I guess
> > there's nothing to do here yet.
> >
> > The only users of equalization settings I could find so far are:
> >
> >   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-tegra194.c?id=v6.11#n832
> >   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-qcom-common.c?id=v6.12-rc1#n11
> >   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pcie-mediatek-gen3.c?id=v6.12-rc1#n909
> >
> > Bjorn
> >