Re: [PATCH v2 2/4] PCI: Provide common functions for ECAM mapping

Re: [PATCH v2 2/4] PCI: Provide common functions for ECAM mapping

[David Daney]

On 04/11/2016 03:45 PM, Jayachandran C wrote:
> Add config option PCI_GENERIC_ECAM and file drivers/pci/ecam.c to
> provide generic functions for accessing memory mapped PCI config space.
>
> The API is defined in drivers/pci/ecam.h and is written to replace the
> API in drivers/pci/host/pci-host-common.h. The file defines a new
> 'struct pci_config_window' to hold the information related to a PCI
> config area and its mapping. This structure is expected to be used as
> sysdata for controllers that have ECAM based mapping.
>
> Helper functions are provided to setup the mapping, free the mapping
> and to implement the map_bus method in 'struct pci_ops'
>
> Signed-off-by: Jayachandran C <jchandra@broadcom.com>

Tested-by: David Daney <david.daney@cavium.com>


> ---
>   drivers/pci/Kconfig  |   3 ++
>   drivers/pci/Makefile |   2 +
>   drivers/pci/ecam.c   | 130 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   drivers/pci/ecam.h   |  58 +++++++++++++++++++++++

I wonder if these files should go in drivers/pci/host ...  I understand 
that you still have to use them from drivers/pci/acpi though.

I will let others opine on this, but could you put the contents of 
ecam.h into include/linux/pci.h along with the  pci_generic_config_*() 
declarations?

If you did that, the contents of ecam.c could  go into 
drivers/pci/access.c...


>   4 files changed, 193 insertions(+)
>   create mode 100644 drivers/pci/ecam.c
>   create mode 100644 drivers/pci/ecam.h
>
[...]

Re: [PATCH v2 2/4] PCI: Provide common functions for ECAM mapping

[Jon Masters]

Hi David, JC,

On 04/11/2016 08:24 PM, David Daney wrote:

> Tested-by: David Daney <david.daney@cavium.com>

On ThunderX (please let me know the silicon pass specifics off-list)?
I'm planning to give this series a test run also on some other ARMv8
hardware and will prod a few of the other vendors to do so.

>>   drivers/pci/ecam.c   | 130
>>   drivers/pci/ecam.h   |  58 +++++++++++++++++++++++
> 
> I wonder if these files should go in drivers/pci/host ...  I understand
> that you still have to use them from drivers/pci/acpi though.
> 
> I will let others opine on this, but could you put the contents of
> ecam.h into include/linux/pci.h along with the  pci_generic_config_*()
> declarations?
> 
> If you did that, the contents of ecam.c could  go into
> drivers/pci/access.c...

Quoting Bjorn's original reply to the previous series:

> Some of the code that moved to drivers/acpi/pci_mcfg.c is not
> really ACPI-specific, and could potentially be used for non-ACPI
> bridges that support ECAM.  I'd like to see that sort of code
> moved to a new file like drivers/pci/ecam.c.

So my guess is that this is the reasoning behind JC's file layout.

I'm curious what Lorenzo's take on things is currently. I assume this
series is now to be the official coordinated version of this effort for
upstream, following the advice of Bjorn previously, but I would like to
know if everyone is behind this plan. I've (previously) requested a
Linaro LEG meeting this week (part of our bootarch working group) to
specifically discuss the status of PCI upstreaming in order to get the
different vendors together to ensure every single one of them is
tracking the correct latest effort and doing what is needed to test/aid,
hence my ask. If this is now plan A, I'll make sure everyone is aligned
behind it and start pinging people individually for testing.

Jon.

-- 
Computer Architect

Re: [PATCH v2 2/4] PCI: Provide common functions for ECAM mapping

[Lorenzo Pieralisi]

On Tue, Apr 12, 2016 at 12:26:25AM -0400, Jon Masters wrote:

[...]

> Quoting Bjorn's original reply to the previous series:
> 
> > Some of the code that moved to drivers/acpi/pci_mcfg.c is not
> > really ACPI-specific, and could potentially be used for non-ACPI
> > bridges that support ECAM.  I'd like to see that sort of code
> > moved to a new file like drivers/pci/ecam.c.
> 
> So my guess is that this is the reasoning behind JC's file layout.
> 
> I'm curious what Lorenzo's take on things is currently. I assume this
> series is now to be the official coordinated version of this effort for
> upstream, following the advice of Bjorn previously, but I would like to
> know if everyone is behind this plan. I've (previously) requested a
> Linaro LEG meeting this week (part of our bootarch working group) to
> specifically discuss the status of PCI upstreaming in order to get the
> different vendors together to ensure every single one of them is
> tracking the correct latest effort and doing what is needed to test/aid,
> hence my ask. If this is now plan A, I'll make sure everyone is aligned
> behind it and start pinging people individually for testing.

My take is that JC's aim is to get this four patch series reviewed and
merged (which is *not* sufficient to get ACPI PCI to work fully on ARM64
- see cover letter - the remaining patches in his branch are not
fixes, it is code that is required to get things to work, these 4
patches stand alone are not sufficient but I understand he wants to get
them reviewed following feedback on the lists) so that we can make
progress on ACPI PCI on ARM64.

I will comment on the patches as soon as I have time to review
them, I certainly would like to understand what we have to do with the
rest of the code though (provided this series is good to go) see above.

Lorenzo

Re: [PATCH v2 2/4] PCI: Provide common functions for ECAM mapping

[Jon Masters]

On 04/12/2016 12:44 PM, Lorenzo Pieralisi wrote:
> On Tue, Apr 12, 2016 at 12:26:25AM -0400, Jon Masters wrote:
> 
> [...]
> 
>> Quoting Bjorn's original reply to the previous series:
>>
>>> Some of the code that moved to drivers/acpi/pci_mcfg.c is not
>>> really ACPI-specific, and could potentially be used for non-ACPI
>>> bridges that support ECAM.  I'd like to see that sort of code
>>> moved to a new file like drivers/pci/ecam.c.
>>
>> So my guess is that this is the reasoning behind JC's file layout.
>>
>> I'm curious what Lorenzo's take on things is currently. I assume this
>> series is now to be the official coordinated version of this effort for
>> upstream, following the advice of Bjorn previously, but I would like to
>> know if everyone is behind this plan. I've (previously) requested a
>> Linaro LEG meeting this week (part of our bootarch working group) to
>> specifically discuss the status of PCI upstreaming in order to get the
>> different vendors together to ensure every single one of them is
>> tracking the correct latest effort and doing what is needed to test/aid,
>> hence my ask. If this is now plan A, I'll make sure everyone is aligned
>> behind it and start pinging people individually for testing.
> 
> My take is that JC's aim is to get this four patch series reviewed and
> merged

Indeed, I see that's probably the goal, and why not :)

> (which is *not* sufficient to get ACPI PCI to work fully on ARM64
> - see cover letter - the remaining patches in his branch are not
> fixes, it is code that is required to get things to work, these 4
> patches stand alone are not sufficient but I understand he wants to get
> them reviewed following feedback on the lists) so that we can make
> progress on ACPI PCI on ARM64.

Agreed. I went through the branch and the 11 patches there, reacquainted
myself with what's what. So what we have now is 4 patches here plus a
few others that in total replace v5 of your previous mmconfig patches in
functionality. The question is what happens with the rest (of JC's
branch let's say) - do they get sent out now too?

> I will comment on the patches as soon as I have time to review
> them, I certainly would like to understand what we have to do with the
> rest of the code though (provided this series is good to go) see above.

Right. That's my reason for asking. I'd like to know who is driving (I
believe that to be Lorenzo) and what the path forward is, and whether we
need to get additional support from anyone else. There's a multi-vendor
meeting in the morning where I'm going to summarize the current state of
these patches and I would like to know (soon) what the plan is so that
we can get everyone on deck to help out at least with testing (most have
tested the previous set, but we need public acks happening).

Jon.

Re: [PATCH v2 2/4] PCI: Provide common functions for ECAM mapping

[Lorenzo Pieralisi]

On Thu, Apr 14, 2016 at 01:55:32AM -0400, Jon Masters wrote:

[...]

> > I will comment on the patches as soon as I have time to review
> > them, I certainly would like to understand what we have to do with the
> > rest of the code though (provided this series is good to go) see above.
> 
> Right. That's my reason for asking. I'd like to know who is driving (I
> believe that to be Lorenzo) and what the path forward is, and whether we
> need to get additional support from anyone else. There's a multi-vendor
> meeting in the morning where I'm going to summarize the current state of
> these patches and I would like to know (soon) what the plan is so that
> we can get everyone on deck to help out at least with testing (most have
> tested the previous set, but we need public acks happening).

Testing always helps, this code needs reviewing from PCI and ACPI
standpoints, what we can do is review this series and repost the whole
thing when we agree the ECAM refactoring this series is implementing
is the right way to go and it is a step in that direction, with no
generic MCFG support in the kernel ACPI PCI on ARM64 can't be
implemented so I would start from that.

Lorenzo

Re: [PATCH v2 3/4] PCI: generic, thunder: update to use generic ECAM API

[David Daney]

On 04/11/2016 03:45 PM, Jayachandran C wrote:
> Use functions provided by drivers/pci/ecam.h for mapping the config
> space in drivers/pci/host/pci-host-common.c, and update its users to
> use 'struct pci_config_window' and 'struct pci_generic_ecam_ops'
>
> The changes are mostly to use 'struct pci_config_window' in place of
> 'struct gen_pci'. Some of the fields of gen_pci were only used
> temporarily and can be eliminated by using local variables or function
> arguments, these are not carried over to struct pci_config_window.
>
> pci-thunder-ecam.c and pci-thunder-pem.c are the only users of the
> pci_host_common_probe function and the gen_pci structure, these have
> been updated to use the new API as well.
>
> The patch does not introduce any functional changes other than a very
> minor one: with the new code, on 64-bit platforms, we do just a single
> ioremap for the whole config space.
>
> Signed-off-by: Jayachandran C <jchandra@broadcom.com>

Tested-by: David Daney <david.daney@cavium.com>

However, ...

> ---
>   drivers/pci/ecam.h                  |   5 ++
>   drivers/pci/host/Kconfig            |   1 +
>   drivers/pci/host/pci-host-common.c  | 121 ++++++++++++++++--------------------
>   drivers/pci/host/pci-host-common.h  |  47 --------------
>   drivers/pci/host/pci-host-generic.c |  50 +++------------
>   drivers/pci/host/pci-thunder-ecam.c |  37 +++--------
>   drivers/pci/host/pci-thunder-pem.c  |  53 +++++-----------

... The patch conflicts with a bug-fix patch I just sent to 
pci-thunder-pem.c, so there will be a race to see who gets in first.

Also, I don't know if it would make sense to split out the pci-thunder-* 
changes to a separate patch.


>   7 files changed, 93 insertions(+), 221 deletions(-)
>   delete mode 100644 drivers/pci/host/pci-host-common.h
>
[...]

Re: [PATCH v2 3/4] PCI: generic, thunder: update to use generic ECAM API

[Jayachandran C]

Hi David,

On Tue, Apr 12, 2016 at 6:04 AM, David Daney <ddaney@caviumnetworks.com> wrote:
> On 04/11/2016 03:45 PM, Jayachandran C wrote:
>>
>> Use functions provided by drivers/pci/ecam.h for mapping the config
>> space in drivers/pci/host/pci-host-common.c, and update its users to
>> use 'struct pci_config_window' and 'struct pci_generic_ecam_ops'
>>
>> The changes are mostly to use 'struct pci_config_window' in place of
>> 'struct gen_pci'. Some of the fields of gen_pci were only used
>> temporarily and can be eliminated by using local variables or function
>> arguments, these are not carried over to struct pci_config_window.
>>
>> pci-thunder-ecam.c and pci-thunder-pem.c are the only users of the
>> pci_host_common_probe function and the gen_pci structure, these have
>> been updated to use the new API as well.
>>
>> The patch does not introduce any functional changes other than a very
>> minor one: with the new code, on 64-bit platforms, we do just a single
>> ioremap for the whole config space.
>>
>> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
>
>
> Tested-by: David Daney <david.daney@cavium.com>

Thanks.

> However, ...
>
>> ---
>>   drivers/pci/ecam.h                  |   5 ++
>>   drivers/pci/host/Kconfig            |   1 +
>>   drivers/pci/host/pci-host-common.c  | 121
>> ++++++++++++++++--------------------
>>   drivers/pci/host/pci-host-common.h  |  47 --------------
>>   drivers/pci/host/pci-host-generic.c |  50 +++------------
>>   drivers/pci/host/pci-thunder-ecam.c |  37 +++--------
>>   drivers/pci/host/pci-thunder-pem.c  |  53 +++++-----------
>
>
> ... The patch conflicts with a bug-fix patch I just sent to
> pci-thunder-pem.c, so there will be a race to see who gets in first.

I would expect this patchset to take more time than the bugfix.
I will update the patches when the fix is queued to -next.

> Also, I don't know if it would make sense to split out the pci-thunder-*
> changes to a separate patch.

In this case, I think splitting the pci-thunder-* will make it more
complex than just switching everything to ecam.h in one go.

If the patch needs to be split, I can move some of the simplification
that was done to gen_pci structure and pci-host-common to a separate
patch.

JC.

Re: [PATCH v2 4/4] ACPI: PCI: Add generic PCI host controller

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3130 bytes --]

Hi Jayachandran,

[auto build test ERROR on pci/next]
[also build test ERROR on v4.6-rc3 next-20160411]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Jayachandran-C/ACPI-based-PCI-host-driver-with-generic-ECAM/20160412-064853
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: arm64-allyesconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

>> drivers/xen/pci.c:31:25: fatal error: asm/pci_x86.h: No such file or directory
    #include <asm/pci_x86.h>
                            ^
   compilation terminated.

vim +31 drivers/xen/pci.c

e28c31a9 Weidong Han           2010-10-27  15   * Place - Suite 330, Boston, MA 02111-1307 USA.
e28c31a9 Weidong Han           2010-10-27  16   *
e28c31a9 Weidong Han           2010-10-27  17   * Author: Weidong Han <weidong.han@intel.com>
e28c31a9 Weidong Han           2010-10-27  18   */
e28c31a9 Weidong Han           2010-10-27  19  
e28c31a9 Weidong Han           2010-10-27  20  #include <linux/pci.h>
55e901fc Jan Beulich           2011-09-22  21  #include <linux/acpi.h>
0b97b03d Ross Lagerwall        2015-04-09  22  #include <linux/pci-acpi.h>
e28c31a9 Weidong Han           2010-10-27  23  #include <xen/xen.h>
e28c31a9 Weidong Han           2010-10-27  24  #include <xen/interface/physdev.h>
e28c31a9 Weidong Han           2010-10-27  25  #include <xen/interface/xen.h>
e28c31a9 Weidong Han           2010-10-27  26  
e28c31a9 Weidong Han           2010-10-27  27  #include <asm/xen/hypervisor.h>
e28c31a9 Weidong Han           2010-10-27  28  #include <asm/xen/hypercall.h>
e28c31a9 Weidong Han           2010-10-27  29  #include "../pci/pci.h"
b7ef4a6d Ben Hutchings         2013-12-31  30  #ifdef CONFIG_PCI_MMCONFIG
8deb3eb1 Konrad Rzeszutek Wilk 2013-10-25 @31  #include <asm/pci_x86.h>
b7ef4a6d Ben Hutchings         2013-12-31  32  #endif
e28c31a9 Weidong Han           2010-10-27  33  
55e901fc Jan Beulich           2011-09-22  34  static bool __read_mostly pci_seg_supported = true;
55e901fc Jan Beulich           2011-09-22  35  
e28c31a9 Weidong Han           2010-10-27  36  static int xen_add_device(struct device *dev)
e28c31a9 Weidong Han           2010-10-27  37  {
e28c31a9 Weidong Han           2010-10-27  38  	int r;
e28c31a9 Weidong Han           2010-10-27  39  	struct pci_dev *pci_dev = to_pci_dev(dev);

:::::: The code at line 31 was first introduced by commit
:::::: 8deb3eb1461e4cb136c88d03ec5a6729ccf2f933 xen/mcfg: Call PHYSDEVOP_pci_mmcfg_reserved for MCFG areas.

:::::: TO: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
:::::: CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 49553 bytes --]

Re: [PATCH v2 4/4] ACPI: PCI: Add generic PCI host controller

[Sinan Kaya]

On 4/11/2016 6:45 PM, Jayachandran C wrote:
> +/* find the entry in cfgarr which contains range bus_start..bus_end */
> +static int mcfg_lookup(u16 seg, u8 bus_start, u8 bus_end)
> +{
> +	struct pci_config_window *cfg;
> +	int i;
> +
> +	if (!cfgarr)
> +		return -ENOENT;
> +
> +	for (i = 0; cfgarr[i]; i++) {
> +		cfg = cfgarr[i];
> +

I see that you are allocating an array of cfgarr to keep the MCFG table entries.
The above way of checking the number of entries is not correct. 

You should keep track of the number of entries you found out globally instead of 
relying an element being NULL or not. 

If you exceed the array size, you may or may not be lucky to find another entry
in memory.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH v2 4/4] ACPI: PCI: Add generic PCI host controller

[Sinan Kaya]

On 4/14/2016 11:53 AM, Sinan Kaya wrote:
> On 4/11/2016 6:45 PM, Jayachandran C wrote:
>> +/* find the entry in cfgarr which contains range bus_start..bus_end */
>> +static int mcfg_lookup(u16 seg, u8 bus_start, u8 bus_end)
>> +{
>> +	struct pci_config_window *cfg;
>> +	int i;
>> +
>> +	if (!cfgarr)
>> +		return -ENOENT;
>> +
>> +	for (i = 0; cfgarr[i]; i++) {
>> +		cfg = cfgarr[i];
>> +
> 
> I see that you are allocating an array of cfgarr to keep the MCFG table entries.
> The above way of checking the number of entries is not correct. 
> 
> You should keep track of the number of entries you found out globally instead of 
> relying an element being NULL or not. 
> 
> If you exceed the array size, you may or may not be lucky to find another entry
> in memory.
> 

I see now that you are allocating an extra element in memory. Still, looping to find
out the number of elements didn't quite look good to me.

+		for (i = 0; cfgarr[i]; i++)
+			;
+		pr_info(PREFIX " MCFG table at loaded, %d entries\n", i);


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project