On 8/29/2019 3:36 AM, Martin Blumenstingl wrote:
On Wed, Aug 28, 2019 at 5:35 AM Chuan Hua, Lei
<chuanhua.lei@xxxxxxxxxxxxxxx> wrote:
[...]
+static int intel_pcie_ep_rst_init(struct intel_pcie_port *lpp)
+{
+ struct device *dev = lpp->pci->dev;
+ int ret = 0;
+
+ lpp->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(lpp->reset_gpio)) {
+ ret = PTR_ERR(lpp->reset_gpio);
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "failed to request PCIe GPIO: %d\n", ret);
+ return ret;
+ }
+ /* Make initial reset last for 100ms */
+ msleep(100);
why is there lpp->rst_interval when you hardcode 100ms here?
There are different purpose. rst_interval is purely for asserted reset
pulse.
Here 100ms is to make sure the initial state keeps at least 100ms, then we
can reset.
my interpretation is that it totally depends on the board design or
the bootloader setup.
Partially, you are right. However, we should not add some dependency
here from
bootloader and board. rst_interval is just to make sure the pulse (low
active or high active)
lasts the specified the time.
+Cc Kishon
he recently added support for a GPIO reset line to the
pcie-cadence-host.c [0] and I believe he's also maintaining
pci-keystone.c which are both using a 100uS delay (instead of 100ms).
I don't know the PCIe spec so maybe Kishon can comment on the values
that should be used according to the spec.
if there's then a reason why values other than the ones from the spec
are needed then there should be a comment explaining why different
values are needed (what problem does it solve).
spec doesn't guide this part. It is a board or SoC specific setting.
100us also should work. spec only requirs reset duration should last
100ms. The idea is that before reset assert and deassert, make sure the
default deassert status keeps some time. We take this value from
hardware suggestion long time back. We can reduce this value to 100us,
but we need to test on the board.
OK. I don't know how other PCI controller drivers manage this. if the
PCI maintainers are happy with this then I am as well
maybe it's worth changing the comment to indicate that this delay was
suggested by the hardware team (so it's clear that this is not coming
from the PCI spec)
Dilip will change to 100us delay and run the test. I also need to run
some tests for old boards(XRX350/550/PRX300) to confirm this has no
impact on function.
[...]
+static void __intel_pcie_remove(struct intel_pcie_port *lpp)
+{
+ pcie_rc_cfg_wr_mask(lpp, PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER,
+ 0, PCI_COMMAND);
I expect logic like this to be part of the PCI subsystem in Linux.
why is this needed?
[...]
bind/unbind case we use this. For extreme cases, we use unbind and bind
to reset
PCI instead of rebooting.
OK, but this does not seem Intel/Lantiq specific at all
why isn't this managed by either pcie-designware-host.c or the generic
PCI/PCIe subsystem in Linux?
I doubt if other RC driver will support bind/unbind. We do have this
requirement due to power management from WiFi devices.
pcie-designware-host.c will gain .remove() support in Linux 5.4
I don't understand how .remove() and then .probe() again is different
from .unbind() followed by a .bind()
Good. If this is the case, bind/unbind eventually goes to probe/remove,
so we can remove this.
OK. as far as I understand you need to call dw_pcie_host_deinit from
the .remove() callback (which is missing in this version)
(I'm using drivers/pci/controller/dwc/pcie-tegra194.c as an example,
this driver is in linux-next and thus will appear in Linux 5.4)
Thanks for your information. We should adapt this in next version.
Martin
