Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver

[Chuan Hua, Lei]

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