All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Daney <ddaney@caviumnetworks.com>
To: Arnd Bergmann <arnd@arndb.de>, Will Deacon <will.deacon@arm.com>,
	Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
Cc: <linux-arm-kernel@lists.infradead.org>,
	Bjorn Helgaas <helgaas@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>, <devicetree@vger.kernel.org>,
	Pawel Moll <pawel.moll@arm.com>,
	Ian Campbell <ijc+devicetree@hellion.org.uk>,
	Marc Zyngier <marc.zyngier@arm.com>, <linux-pci@vger.kernel.org>,
	David Daney <david.daney@cavium.com>,
	<linux-kernel@vger.kernel.org>, Rob Herring <robh+dt@kernel.org>,
	David Daney <ddaney.cavm@gmail.com>,
	Kumar Gala <galak@codeaurora.org>,
	Bjorn Helgaas <bhelgaas@google.com>
Subject: Re: [PATCH v4 4/5] PCI: generic: Correct, and avoid overflow, in bus_max calculation.
Date: Thu, 8 Oct 2015 08:39:58 -0700	[thread overview]
Message-ID: <56168E4E.3070405@caviumnetworks.com> (raw)
In-Reply-To: <5178627.ufUfdomGdT@wuerfel>

On 10/08/2015 08:18 AM, Arnd Bergmann wrote:
> On Thursday 08 October 2015 17:11:32 Arnd Bergmann wrote:
>>>> --- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt
>>>> +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
>>>> @@ -34,7 +34,11 @@ Properties of the host controller node:
>>>>   - #size-cells    : Must be 2.
>>>>
>>>>   - reg            : The Configuration Space base address and size, as accessed
>>>> -                   from the parent bus.
>>>> +                   from the parent bus.  The base address corresponds to
>>>> +                   bus zero, even though the "bus-range" property may specify
>>>> +                   a different starting bus number.  The driver must only map
>>>> +                   or access the portion of the Configuration Space that
>>>> +                   corresponds to the "bus-range"
>>
>> I thought we had reached an agreement that it is a bad idea to have a 'reg'
>> property that lists registers belonging to a different device.
>>
>>
>
> To further clarify: the argument was to make it mirror what ACPI does for
> PCI. However, this is unlike what all other devices do with DT, where you
> have non-overlapping register ranges (start, length) for each device.
> ACPI as far as I understand it does not give a range for a PCIe host, but
> instead provides a way to get the start address of the ECAM register area
> for the domain that the host is part of, and that needs to be the same
> address for each host in the domain.
>

We are agreed that it is a bad thing to do.  There is disagreement about 
if we should do it.

I think there are two schools of thought (I will attribute them to their 
proponents and my apologies if I misrepresent someone's stance):

1) (Arnd) Don't make the the "reg" ranges overlap because it is ugly, 
dangerous and arguably incorrect in general.

2) (Me, Will Deacon, Lorenzo Pieralisi) Overlapping "reg" properties 
should be maintained, as that is the current behavior and seems to agree 
with legacy OF device-tree specifications (although there is some debate 
about this).

Because the driver is broken in this area (thus the patch), it indicates 
that there are probably no users with non-zero starting bus numbers. 
So, we may have some latitude to change it.

I will generate another patch that does it Arnd's way, and if Will is OK 
with it, we might be able to do that instead.  One thing is certain: 
The driver is currently broken for non-zero starting bus numbers.

David Daney

WARNING: multiple messages have this Message-ID (diff)
From: ddaney@caviumnetworks.com (David Daney)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v4 4/5] PCI: generic: Correct, and avoid overflow, in bus_max calculation.
Date: Thu, 8 Oct 2015 08:39:58 -0700	[thread overview]
Message-ID: <56168E4E.3070405@caviumnetworks.com> (raw)
In-Reply-To: <5178627.ufUfdomGdT@wuerfel>

On 10/08/2015 08:18 AM, Arnd Bergmann wrote:
> On Thursday 08 October 2015 17:11:32 Arnd Bergmann wrote:
>>>> --- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt
>>>> +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
>>>> @@ -34,7 +34,11 @@ Properties of the host controller node:
>>>>   - #size-cells    : Must be 2.
>>>>
>>>>   - reg            : The Configuration Space base address and size, as accessed
>>>> -                   from the parent bus.
>>>> +                   from the parent bus.  The base address corresponds to
>>>> +                   bus zero, even though the "bus-range" property may specify
>>>> +                   a different starting bus number.  The driver must only map
>>>> +                   or access the portion of the Configuration Space that
>>>> +                   corresponds to the "bus-range"
>>
>> I thought we had reached an agreement that it is a bad idea to have a 'reg'
>> property that lists registers belonging to a different device.
>>
>>
>
> To further clarify: the argument was to make it mirror what ACPI does for
> PCI. However, this is unlike what all other devices do with DT, where you
> have non-overlapping register ranges (start, length) for each device.
> ACPI as far as I understand it does not give a range for a PCIe host, but
> instead provides a way to get the start address of the ECAM register area
> for the domain that the host is part of, and that needs to be the same
> address for each host in the domain.
>

We are agreed that it is a bad thing to do.  There is disagreement about 
if we should do it.

I think there are two schools of thought (I will attribute them to their 
proponents and my apologies if I misrepresent someone's stance):

1) (Arnd) Don't make the the "reg" ranges overlap because it is ugly, 
dangerous and arguably incorrect in general.

2) (Me, Will Deacon, Lorenzo Pieralisi) Overlapping "reg" properties 
should be maintained, as that is the current behavior and seems to agree 
with legacy OF device-tree specifications (although there is some debate 
about this).

Because the driver is broken in this area (thus the patch), it indicates 
that there are probably no users with non-zero starting bus numbers. 
So, we may have some latitude to change it.

I will generate another patch that does it Arnd's way, and if Will is OK 
with it, we might be able to do that instead.  One thing is certain: 
The driver is currently broken for non-zero starting bus numbers.

David Daney

WARNING: multiple messages have this Message-ID (diff)
From: David Daney <ddaney@caviumnetworks.com>
To: Arnd Bergmann <arnd@arndb.de>, Will Deacon <will.deacon@arm.com>,
	Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
Cc: linux-arm-kernel@lists.infradead.org,
	Bjorn Helgaas <helgaas@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	devicetree@vger.kernel.org, Pawel Moll <pawel.moll@arm.com>,
	Ian Campbell <ijc+devicetree@hellion.org.uk>,
	Marc Zyngier <marc.zyngier@arm.com>,
	linux-pci@vger.kernel.org, David Daney <david.daney@cavium.com>,
	linux-kernel@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
	David Daney <ddaney.cavm@gmail.com>,
	Kumar Gala <galak@codeaurora.org>,
	Bjorn Helgaas <bhelgaas@google.com>
Subject: Re: [PATCH v4 4/5] PCI: generic: Correct, and avoid overflow, in bus_max calculation.
Date: Thu, 8 Oct 2015 08:39:58 -0700	[thread overview]
Message-ID: <56168E4E.3070405@caviumnetworks.com> (raw)
In-Reply-To: <5178627.ufUfdomGdT@wuerfel>

On 10/08/2015 08:18 AM, Arnd Bergmann wrote:
> On Thursday 08 October 2015 17:11:32 Arnd Bergmann wrote:
>>>> --- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt
>>>> +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt
>>>> @@ -34,7 +34,11 @@ Properties of the host controller node:
>>>>   - #size-cells    : Must be 2.
>>>>
>>>>   - reg            : The Configuration Space base address and size, as accessed
>>>> -                   from the parent bus.
>>>> +                   from the parent bus.  The base address corresponds to
>>>> +                   bus zero, even though the "bus-range" property may specify
>>>> +                   a different starting bus number.  The driver must only map
>>>> +                   or access the portion of the Configuration Space that
>>>> +                   corresponds to the "bus-range"
>>
>> I thought we had reached an agreement that it is a bad idea to have a 'reg'
>> property that lists registers belonging to a different device.
>>
>>
>
> To further clarify: the argument was to make it mirror what ACPI does for
> PCI. However, this is unlike what all other devices do with DT, where you
> have non-overlapping register ranges (start, length) for each device.
> ACPI as far as I understand it does not give a range for a PCIe host, but
> instead provides a way to get the start address of the ECAM register area
> for the domain that the host is part of, and that needs to be the same
> address for each host in the domain.
>

We are agreed that it is a bad thing to do.  There is disagreement about 
if we should do it.

I think there are two schools of thought (I will attribute them to their 
proponents and my apologies if I misrepresent someone's stance):

1) (Arnd) Don't make the the "reg" ranges overlap because it is ugly, 
dangerous and arguably incorrect in general.

2) (Me, Will Deacon, Lorenzo Pieralisi) Overlapping "reg" properties 
should be maintained, as that is the current behavior and seems to agree 
with legacy OF device-tree specifications (although there is some debate 
about this).

Because the driver is broken in this area (thus the patch), it indicates 
that there are probably no users with non-zero starting bus numbers. 
So, we may have some latitude to change it.

I will generate another patch that does it Arnd's way, and if Will is OK 
with it, we might be able to do that instead.  One thing is certain: 
The driver is currently broken for non-zero starting bus numbers.

David Daney

  reply	other threads:[~2015-10-08 15:40 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-10-02 18:43 [PATCH v4 0/5] PCI: generic: Misc. bug fixes/enhancements David Daney
2015-10-02 18:43 ` David Daney
2015-10-02 18:43 ` [PATCH v4 1/5] PCI: Add pci_bus_fixup_irqs() David Daney
2015-10-02 18:43   ` David Daney
2015-10-07 19:44   ` Bjorn Helgaas
2015-10-07 19:44     ` Bjorn Helgaas
2015-10-07 20:08     ` David Daney
2015-10-07 20:08       ` David Daney
2015-10-07 20:08       ` David Daney
2015-10-07 23:08       ` Bjorn Helgaas
2015-10-07 23:08         ` Bjorn Helgaas
2015-10-08  2:07         ` Matthew Minter
2015-10-08  2:07           ` Matthew Minter
2015-10-08  2:07           ` Matthew Minter
2015-10-08  9:18           ` Lorenzo Pieralisi
2015-10-08  9:18             ` Lorenzo Pieralisi
2015-10-08  9:18             ` Lorenzo Pieralisi
2015-10-02 18:44 ` [PATCH v4 2/5] PCI: generic: Only fixup irqs for bus we are creating David Daney
2015-10-02 18:44   ` David Daney
2015-10-02 18:44 ` [PATCH v4 3/5] PCI: generic: Quit clobbering our pci_ops David Daney
2015-10-02 18:44   ` David Daney
2015-10-08 15:02   ` Bjorn Helgaas
2015-10-08 15:02     ` Bjorn Helgaas
2015-10-08 15:02     ` Bjorn Helgaas
2015-10-08 15:09     ` Arnd Bergmann
2015-10-08 15:09       ` Arnd Bergmann
2015-10-02 18:44 ` [PATCH v4 4/5] PCI: generic: Correct, and avoid overflow, in bus_max calculation David Daney
2015-10-02 18:44   ` David Daney
2015-10-08 15:02   ` Bjorn Helgaas
2015-10-08 15:02     ` Bjorn Helgaas
2015-10-08 15:11     ` Arnd Bergmann
2015-10-08 15:11       ` Arnd Bergmann
2015-10-08 15:18       ` Arnd Bergmann
2015-10-08 15:18         ` Arnd Bergmann
2015-10-08 15:39         ` David Daney [this message]
2015-10-08 15:39           ` David Daney
2015-10-08 15:39           ` David Daney
2015-10-08 17:27           ` Lorenzo Pieralisi
2015-10-08 17:27             ` Lorenzo Pieralisi
2015-10-08 17:27             ` Lorenzo Pieralisi
2015-10-02 18:44 ` [PATCH v4 5/5] PCI: generic: Pass proper starting bus number to pci_scan_root_bus() David Daney
2015-10-02 18:44   ` David Daney
2015-10-08 15:28 ` [PATCH v4 0/5] PCI: generic: Misc. bug fixes/enhancements Bjorn Helgaas
2015-10-08 15:28   ` Bjorn Helgaas
2015-10-08 15:44   ` David Daney
2015-10-08 15:44     ` David Daney
2015-10-08 15:44     ` David Daney

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=56168E4E.3070405@caviumnetworks.com \
    --to=ddaney@caviumnetworks.com \
    --cc=Lorenzo.Pieralisi@arm.com \
    --cc=arnd@arndb.de \
    --cc=bhelgaas@google.com \
    --cc=david.daney@cavium.com \
    --cc=ddaney.cavm@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=galak@codeaurora.org \
    --cc=helgaas@kernel.org \
    --cc=ijc+devicetree@hellion.org.uk \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=marc.zyngier@arm.com \
    --cc=mark.rutland@arm.com \
    --cc=pawel.moll@arm.com \
    --cc=robh+dt@kernel.org \
    --cc=will.deacon@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.