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

[Jim Quinlan <jim2101024@gmail.com>]

On Tue, Nov 5, 2024 at 4:33 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Jim, Krishna, Vidya, Shashank]
>
> 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.

Regards,
Jim
Broadcom STB/CM
>
> 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
>