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

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Nov 07, 2024 at 05:21:45PM +0100, Lorenzo Bianconi wrote:
> On Nov 07, Bjorn Helgaas wrote:
> > On Thu, Nov 07, 2024 at 08:39:43AM +0100, Lorenzo Bianconi wrote:
> > > > On Wed, Nov 06, 2024 at 11:40:28PM +0100, Lorenzo Bianconi 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.
> > > > > > > ...
> > 
> > > > > > Is this where PERST# is asserted?  If so, a comment to that effect
> > > > > > would be helpful.  Where is PERST# deasserted?  Where are the required
> > > > > > delays before deassert done?
> > > > > 
> > > > > I can add a comment in en7581_pci_enable() describing the PERST issue for
> > > > > EN7581. Please note we have a 250ms delay in en7581_pci_enable() after
> > > > > configuring REG_PCI_CONTROL register.
> > > > > 
> > > > > https://github.com/torvalds/linux/blob/master/drivers/clk/clk-en7523.c#L396
> > > > 
> > > > Does that 250ms delay correspond to a PCIe mandatory delay, e.g.,
> > > > something like PCIE_T_PVPERL_MS?  I think it would be nice to have the
> > > > required PCI delays in this driver if possible so it's easy to verify
> > > > that they are all covered.
> > > 
> > > IIRC I just used the delay value used in the vendor sdk. I do not
> > > have a strong opinion about it but I guess if we move it in the
> > > pcie-mediatek-gen3 driver, we will need to add it in each driver
> > > where this clock is used. What do you think?
> > 
> > I don't know what the 250ms delay is for.  If it is for a required PCI
> > delay, we should use the relevant standard #define for it, and it
> > should be in the PCI controller driver.  Otherwise it's impossible to
> > verify that all the drivers are doing the correct delays.
> 
> ack, fine to me. Do you prefer to keep 250ms after clk_bulk_prepare_enable()
> in mtk_pcie_en7581_power_up() or just use PCIE_T_PVPERL_MS (100)?
> I can check if 100ms works properly.

It's not clear to me where the relevant events are for these chips.

Do you have access to the PCIe CEM spec?  The diagram in r6.0, sec
2.2.1, is helpful.  It shows the required timings for Power Stable,
REFCLK Stable, PERST# deassert, etc.

Per sec 2.11.2, PERST# must be asserted for at least 100us (T_PERST),
PERST# must be asserted for at least 100ms after Power Stable
(T_PVPERL), and PERST# must be asserted for at least 100us after
REFCLK Stable.

It would be helpful if we could tell by reading the source where some
of these critical events happen, and that the relevant delays are
there.  For example, if PERST# is asserted/deasserted by
"clk_enable()" or similar, it's not at all obvious from the code, so
we should have a comment to that effect.

Bjorn