On Tue, Nov 5, 2024 at 4:33 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> 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@xxxxxxxxxxxx/
> https://lore.kernel.org/r/77d3a1a9-c22d-0fd3-5942-91b9a3d74a43@xxxxxxxxxxx
>
> 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
>
