[PATCH v2] PCI: designware: move remaining rc setup code to dw_pcie_setup_rc()

Hi Bjorn

-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas at kernel.org]
Sent: 21 April 2016 16:49
To: Gabriele Paoloni
Cc: Jisheng Zhang; jingoohan1 at gmail.com; pratyush.anand at gmail.com;
bhelgaas at google.com; linux-pci at vger.kernel.org; linux-
kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH v2] PCI: designware: move remaining rc setup code
to dw_pcie_setup_rc()

On Tue, Apr 12, 2016 at 09:43:32AM +0000, Gabriele Paoloni wrote:

quoted

Hi Bjorn

[...]

quoted

quoted

quoted

What's the hisi plan for resuming after suspend-to-RAM?  How

does

quoted

quoted

the

quoted

quoted

RC get reprogrammed after it loses all its state?

PM is not part of the driver yet. This is planned for near
future release so haven't made such considerations yet

quoted

What would break if hisi did call dw_pcie_setup_rc()?  I know

you

quoted

quoted

said

quoted

quoted

it would overwrite what the bootloader already did, which is

true.

quoted

quoted

quoted

I am try to figure this out now with our HW team.

quoted

But hisi does call dw_pcie_host_init(), so it reads pp->mem

(which

quoted

quoted

quoted

quoted

determines pp->mem_base) and pp->lanes from the DT.  Other

drivers

quoted

quoted

quoted

quoted

then call dw_pcie_setup_rc() which programs the RC based on
pp->mem_base and pp->lanes.  So hisi assumes UEFI programmed

the RC

quoted

quoted

to

quoted

quoted

match the DT, while the other drivers read the DT and program

the

quoted

quoted

RC

quoted

quoted

to match.  The latter seems more robust because it enforces the
consistency rather than relying on it.

Yes I agree with you, however we have preferred to move RC config

to

quoted

quoted

quoted

BIOS to have a single driver to support multiple versions of the
same SoC.

I think there are two reasonable approaches:

  1) A single generic driver that doesn't have any knowledge about

the

quoted

quoted

  chipset registers; it uses run-time firmware interfaces to manage
  the bridge.  The ACPI pci_root.c driver is the best example so

far

quoted

quoted

  and works very well.  It supports basically all x86 and ia64
  chipsets and requires no kernel work for new ones.

  2) Native drivers specific to each chipset.  These may get
  configuration information from DT, but they do their own
  register-level programming of the device without run-time help

from

quoted

quoted

  firmware.

I think hisi is a native driver because it uses hip05/hip06

registers

quoted

quoted

to check link state and perform config operations.  And apparently

you

quoted

quoted

rely on the ATU, BAR, class, and link width programming currently

done

quoted

quoted

in dw_pcie_host_init().  But you want to rely on pre-boot firmware

to

quoted

quoted

set up the link.  That doesn't make sense to me -- if the driver

wants

quoted

quoted

to twiddle the registers, it should know how to do it all.  I don't
see how you can reasonably manage this half-way approach.

quoted

The patch I proposed above does the same job as the original

patch

quoted

quoted

quoted

proposed by Jisheng and also allows hisi driver to call the moved
code.

Do you see anything wrong with it?

Only that it makes the structure more complicated and we haven't
identified a corresponding benefit yet.

Finally I have checked that assigning .host_init function pointer
in our driver to call dw_pcie_setup_rc() will not affect the values
already set by BIOS.

Also I agree with you that a hybrid approach is not ideal.

So I will update the driver to call dw_pcie_setup_rc() from
.host_init and ask the BIOS team to update the firmware for next
releases (the driver will be backward compatible anyway).

Am I right in assuming that the patch currently in my tree:

https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=
pci/host-designware&id=1488aefa37a4033080942c860294d13c613ec829

will work for you?  I'm going to assume so unless I hear otherwise.

Yes you are right.

I thought it was clear by the last conclusion.
Sorry if it was not explicit.

Many Thanks and Regards

Gab

Bjorn