Re: [PATCH v1 5/5] pci: keystone: add pcie driver based on designware core driver

[Arnd Bergmann]

On Monday 19 May 2014 17:10:50 Murali Karicheri wrote:
> On 5/19/2014 8:06 AM, Arnd Bergmann wrote:
> > On Friday 16 May 2014 16:26:51 Murali Karicheri wrote:
> >> On 5/15/2014 2:20 PM, Arnd Bergmann wrote:
> >>> On Thursday 15 May 2014 13:45:08 Murali Karicheri wrote:
> >>>>>> +#ifdef CONFIG_PCI_KEYSTONE
> >>>>>> +/*
> >>>>>> + * The KeyStone PCIe controller has maximum read request size of 256 bytes.
> >>>>>> + */
> >>>>>> +static void quirk_limit_readrequest(struct pci_dev *dev)
> >>>>>> +{
> >>>>>> +    int readrq = pcie_get_readrq(dev);
> >>>>>> +
> >>>>>> +    if (readrq > 256)
> >>>>>> +            pcie_set_readrq(dev, 256);
> >>>>>> +}
> >>>>>> +DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_readrequest);
> >>>>>> +#endif /* CONFIG_PCI_KEYSTONE */
> >>>>> This doesn't work: you can't just limit do this for all devices just based
> >>>>> on PCI_KEYSTONE being enabled, you have to check if you are actually using
> >>>>> this controller.
> >>>>>
> >>>>>         Arnd
> >>>>     I assume, I need to check if PCI controller's vendor ID/ device ID
> >>>> match with the keystone
> >>>>     PCI controller's ID and call pcie_set_readrq() for all of the slave
> >>>> PCI devices and do this fixup.
> >>>> Is this correct understanding?  If you can point me to an example code
> >>>> for this that will be
> >>>> really helpful so that I can avoid re-inventing the wheel.
> >>> I think it would be best to move the quirk into the keystone pci driver
> >>> and compare compare the dev->driver pointer of the PCI controller device.
> >>>
> >>>        Arnd
> >> Arnd,
> >>
> >> I will move this quirk to keystone pci driver. So in that case, I guess
> >> your original comment
> >> is not applicable as  this quirk gets enabled only with PCI keystone
> >> driver enabled. Right?
> > Of course you still have to fix the bug, not just move the code into
> > the driver. Otherwise it's still broken for every machine after the keystone
> > driver is enabled.
> 
> Agree. I have tried following to get this work so that the quirk gets 
> applied only for
> keystone pci controller.
> 
> #define KS_K2HK_PCI_DEVICE_ID    0xb008
> #define KS_K2E_PCI_DEVICE_ID    0xb009
> #define KS_K2L_PCI_DEVICE_ID    0xb00a
> 
> static u16 root_pci_ids[] =
>      {KS_K2HK_PCI_DEVICE_ID, KS_K2E_PCI_DEVICE_ID, KS_K2L_PCI_DEVICE_ID, 
> 0 };
> /*
>   * The KeyStone PCIe controller has maximum read request size of 256 bytes.
>   */
> static void quirk_limit_readrequest(struct pci_dev *dev)
> {
>      struct pci_dev *root_dev;
>      int i = 0;
> 
>      /* Apply quirk only if this bridge device is keystone */
>      while (root_pci_ids[i]) {
>          root_dev = pci_get_device(PCI_VENDOR_ID_TI, root_pci_ids[i], NULL);
>          if ((root_dev->class >> 8) == PCI_CLASS_BRIDGE_PCI) {
>              int readrq;
> 
>              readrq = pcie_get_readrq(dev);
>              if (pcie_get_readrq(dev) > 256) {
>                  pcie_set_readrq(dev, 256);
>                  printk("Applied quirk\n");
>              }
>              break;
>          };
>      }
> }
> DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, quirk_limit_readrequest);

Why not compare the driver pointer as I suggested?

> > I also agree with Jason that changing the PCI core to call
> > pcie_bus_configure_settings() would be a better way of dealing with this
> > if it solves the problem.
> 
> I tried following piece of code added to bios32.c
> 
> void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
> {
>          struct pci_sys_data *sys;
>          LIST_HEAD(head);
> 
>          pci_add_flags(PCI_REASSIGN_ALL_RSRC);
>          if (hw->preinit)
>                  hw->preinit();
> 
>           ........
>           ....
> 
>           list_for_each_entry(sys, &head, node) {
>                  struct pci_bus *bus = sys->bus;
> 
>                  if (!pci_has_flag(PCI_PROBE_ONLY)) {
>                          /*
>                           * Size the bridge windows.
>                           */
>                          pci_bus_size_bridges(bus);
> 
>                          /*
>                           * Assign resources.
>                           */
>                          pci_bus_assign_resources(bus);
>                  }
>                  /*
>                   * Tell drivers about devices found.
>                   */
>                  pci_bus_add_devices(bus);
>          }
> 
> // New code starts here
>          list_for_each_entry(sys, &head, node) {
>                  struct pci_bus *bus = sys->bus;
> 
>                  /* Configure PCI Express settings */
>                  if (bus && !pci_has_flag(PCI_PROBE_ONLY)) {
>                          struct pci_bus *child;
> 
>                          list_for_each_entry(child, &bus->children, node)
>                          pcie_bus_configure_settings(child);
>                  }
>          }
> // New code ends.

This is the ARM specific part of the PCI code. I think it would be
better to do this independent of the architecture.

> This seems to do different things based on the pci bootargs. The key 
> requirement for Keystone PCI controller
> is that the MRRS can't be more than 256 bytes.  The only option that 
> works for keystone PCI controller
> is with pci=pcie_bus_perf.   I see following logs:-
> 
> [    3.382747] pcie_bus_configure_settings, config 2
> [    3.382753] pcie_bus_configure_set
> [    3.382781] pcieport 0000:00:00.0: Max Payload Size set to  256/ 256 
> (was  128), Max Read Rq  256
> [    3.382788] pcie_bus_configure_set
> [    3.382846] pci 0000:01:00.0: Max Payload Size set to  256/ 256 (was  
> 128), Max Read Rq  256
> [    3.382852] pcie_bus_configure_set
> [    3.382909] pci 0000:01:00.1: Max Payload Size set to  256/ 256 (was  
> 128), Max Read Rq  256
> 
> On ARM, by default pci_bus_config seems to be set to 0 
> (PCIE_BUS_TUNE_OFF). So the code doesn't get
> executed for this default. But for PCIE_BUS_SAFE, it doesn't change the 
> mrrs at the EP and is not
> good for our platform w.r.t mrrs settings. For PCIE_BUS_PERFORMANCE, it 
> seems to increase the payload
> size as well and Keystone Payload size is limited to 128 bytes. So it is 
> not safe to increase the payload
> size to 256 based on the log.
> 
> On other platforms, Why the PCI core try to set the payload size equal 
> to mrrs? Is this explained in any
> PCI spec?  Looks like this is done for performance? Let me know if you 
> want me to send a patch for
> review to add the pcie_bus_configure_settings() code to 
> arch/arm/kernel/bios32.c
> 
> For the Keystone PCI driver, I believe. it is safe to have the quirk so 
> that controller can handle the
> read requests properly. Let me know if the quirk code above looks good 
> to go.

I don't know enough about these to give you a definite answer, but
I'd still prefer to handle this in generic code. A quirk seems to
be the wrong answer here. If this isn't something we can do in generic
fashion, can you do it in the add_bus() callback perhaps?

Maybe Jason or Bjorn have a better idea.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/