Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

2017-05-25 Thread Marc Zyngier 
On 25/05/17 13:41, Mason wrote:
> On 25/05/2017 14:23, Marc Zyngier wrote:
>> On 25/05/17 13:00, Mason wrote:
>>> On 25/05/2017 10:48, Marc Zyngier wrote:
 Please have some defines for these magic values.
>>>
>>> Typical driver do
>>> #define MUX_OFFSET 0x48
>>> and then access the register's value through
>>> readl_relaxed(pcie->base + MUX_OFFSET);
>>>
>>> I can't do that because the registers were shuffled around
>>> between revision 1 and revision 2. Thus, instead of an
>>> explicitly-named macro (MUX_OFFSET), I used an explicitly-
>>> named field (pcie->mux) and access the register's value
>>> through readl_relaxed(pcie->mux);
>>
>> That doesn't prevent you from having a TANGO_V1_MUX_OFFSET define, which
>> you can supplement with a V2 at some point.
>>
>>> This is equivalent to providing the offset definitions in the
>>> init functions, instead of at the top of the file.
>>
>> Sorry, my brain parses text far better than hex number.
> 
> Well, the hex numbers do need to show up somewhere :-)
> 
> IIUC, you're saying that
> #define MUX_OFFSET 0x48
> is clearer than
> pcie->mux = base + 0x48;

yes.

> 
> OK, I can accept that. Maybe our brains have been trained
> to easily recognize and ingest the macro, or maybe it's
> the caps, or maybe the fact that the statement does
> several things (addition and assignment and hex).
> 
> Out of curiosity, how would you feel about
> pcie->MUX_OFFSET = 0x48;
> and then using
> readl_relaxed(pcie->base + pcie->MUX_OFFSET);
> 
> It feels weird to me, I think mostly because it is
> an unusual pattern.

Exactly. Use existing practices help the reviewers quite a lot.

> 
> Anyway, I'll add the macros, if that improves review and
> maintenance.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

2017-05-25 Thread Marc Zyngier 
On 25/05/17 13:41, Mason wrote:
> On 25/05/2017 14:23, Marc Zyngier wrote:
>> On 25/05/17 13:00, Mason wrote:
>>> On 25/05/2017 10:48, Marc Zyngier wrote:
 Please have some defines for these magic values.
>>>
>>> Typical driver do
>>> #define MUX_OFFSET 0x48
>>> and then access the register's value through
>>> readl_relaxed(pcie->base + MUX_OFFSET);
>>>
>>> I can't do that because the registers were shuffled around
>>> between revision 1 and revision 2. Thus, instead of an
>>> explicitly-named macro (MUX_OFFSET), I used an explicitly-
>>> named field (pcie->mux) and access the register's value
>>> through readl_relaxed(pcie->mux);
>>
>> That doesn't prevent you from having a TANGO_V1_MUX_OFFSET define, which
>> you can supplement with a V2 at some point.
>>
>>> This is equivalent to providing the offset definitions in the
>>> init functions, instead of at the top of the file.
>>
>> Sorry, my brain parses text far better than hex number.
> 
> Well, the hex numbers do need to show up somewhere :-)
> 
> IIUC, you're saying that
> #define MUX_OFFSET 0x48
> is clearer than
> pcie->mux = base + 0x48;

yes.

> 
> OK, I can accept that. Maybe our brains have been trained
> to easily recognize and ingest the macro, or maybe it's
> the caps, or maybe the fact that the statement does
> several things (addition and assignment and hex).
> 
> Out of curiosity, how would you feel about
> pcie->MUX_OFFSET = 0x48;
> and then using
> readl_relaxed(pcie->base + pcie->MUX_OFFSET);
> 
> It feels weird to me, I think mostly because it is
> an unusual pattern.

Exactly. Use existing practices help the reviewers quite a lot.

> 
> Anyway, I'll add the macros, if that improves review and
> maintenance.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

2017-05-25 Thread Mason 
On 25/05/2017 14:23, Marc Zyngier wrote:
> On 25/05/17 13:00, Mason wrote:
>> On 25/05/2017 10:48, Marc Zyngier wrote:
>>> Please have some defines for these magic values.
>>
>> Typical driver do
>> #define MUX_OFFSET 0x48
>> and then access the register's value through
>> readl_relaxed(pcie->base + MUX_OFFSET);
>>
>> I can't do that because the registers were shuffled around
>> between revision 1 and revision 2. Thus, instead of an
>> explicitly-named macro (MUX_OFFSET), I used an explicitly-
>> named field (pcie->mux) and access the register's value
>> through readl_relaxed(pcie->mux);
> 
> That doesn't prevent you from having a TANGO_V1_MUX_OFFSET define, which
> you can supplement with a V2 at some point.
> 
>> This is equivalent to providing the offset definitions in the
>> init functions, instead of at the top of the file.
> 
> Sorry, my brain parses text far better than hex number.

Well, the hex numbers do need to show up somewhere :-)

IIUC, you're saying that
#define MUX_OFFSET 0x48
is clearer than
pcie->mux = base + 0x48;

OK, I can accept that. Maybe our brains have been trained
to easily recognize and ingest the macro, or maybe it's
the caps, or maybe the fact that the statement does
several things (addition and assignment and hex).

Out of curiosity, how would you feel about
pcie->MUX_OFFSET = 0x48;
and then using
readl_relaxed(pcie->base + pcie->MUX_OFFSET);

It feels weird to me, I think mostly because it is
an unusual pattern.

Anyway, I'll add the macros, if that improves review and
maintenance.

Regards.

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

2017-05-25 Thread Mason 
On 25/05/2017 14:23, Marc Zyngier wrote:
> On 25/05/17 13:00, Mason wrote:
>> On 25/05/2017 10:48, Marc Zyngier wrote:
>>> Please have some defines for these magic values.
>>
>> Typical driver do
>> #define MUX_OFFSET 0x48
>> and then access the register's value through
>> readl_relaxed(pcie->base + MUX_OFFSET);
>>
>> I can't do that because the registers were shuffled around
>> between revision 1 and revision 2. Thus, instead of an
>> explicitly-named macro (MUX_OFFSET), I used an explicitly-
>> named field (pcie->mux) and access the register's value
>> through readl_relaxed(pcie->mux);
> 
> That doesn't prevent you from having a TANGO_V1_MUX_OFFSET define, which
> you can supplement with a V2 at some point.
> 
>> This is equivalent to providing the offset definitions in the
>> init functions, instead of at the top of the file.
> 
> Sorry, my brain parses text far better than hex number.

Well, the hex numbers do need to show up somewhere :-)

IIUC, you're saying that
#define MUX_OFFSET 0x48
is clearer than
pcie->mux = base + 0x48;

OK, I can accept that. Maybe our brains have been trained
to easily recognize and ingest the macro, or maybe it's
the caps, or maybe the fact that the statement does
several things (addition and assignment and hex).

Out of curiosity, how would you feel about
pcie->MUX_OFFSET = 0x48;
and then using
readl_relaxed(pcie->base + pcie->MUX_OFFSET);

It feels weird to me, I think mostly because it is
an unusual pattern.

Anyway, I'll add the macros, if that improves review and
maintenance.

Regards.

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

2017-05-25 Thread Marc Zyngier 
On 25/05/17 13:00, Mason wrote:
> On 25/05/2017 10:48, Marc Zyngier wrote:
> 
>> On 20/04/17 15:31, Marc Gonzalez wrote:
>>
>>> This driver is required to work around several hardware bugs in the
>>> PCIe controller.
>>>
>>> NB: Revision 1 does not support legacy interrupts, or IO space.
>>>
>>> Signed-off-by: Marc Gonzalez 
>>> ---
>>>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 
>>>  drivers/pci/host/Kconfig |   8 ++
>>>  drivers/pci/host/Makefile|   1 +
>>>  drivers/pci/host/pcie-tango.c| 161 
>>> +++
>>>  include/linux/pci_ids.h  |   2 +
>>>  5 files changed, 204 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/tango-pcie.txt 
>>> b/Documentation/devicetree/bindings/pci/tango-pcie.txt
>>> new file mode 100644
>>> index ..3353b4e77309
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pci/tango-pcie.txt
>>> @@ -0,0 +1,32 @@
>>> +Sigma Designs Tango PCIe controller
>>> +
>>> +Required properties:
>>> +
>>> +- compatible: "sigma,smp8759-pcie"
>>> +- reg: address/size of PCI configuration space, address/size of register 
>>> area
>>> +- device_type: "pci"
>>> +- #size-cells: <2>
>>> +- #address-cells: <3>
>>> +- #interrupt-cells: <1>
>>
>> What is the point of having an #interrupt-cells when this is *not* an
>> interrupt controller (as it doesn't support legacy interrupts)?
> 
> My mistake.
> 
> Thanks for kindly pointing out that the #interrupt-cells property
> is not needed when a controller doesn't support legacy interrupts.
> 
> If a controller does support legacy interrupts, then I see other
> bindings define #interrupt-cells and interrupt-map.
> Is interrupt-controller also required?

Probably.

> Is that redundant with msi-controller?

No.

> (Rev2 will support legacy interrupts.)
> 
> References for my own information:
> http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/pci/altera-pcie.txt
> http://elinux.org/Device_Tree_Usage#Advanced_Interrupt_Mapping
> 
>> As mentioned earlier, this needs to be a separate patch to be reviewed
>> by the Keepers of the Faith (aka the DT maintainers).
> 
> robh already acked v3 two months ago, but can split it up,
> and CC the DT folks for v5.

You didn't add the Acked-by to this patch, making Rob's effort pretty
useless.

>>> +static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
>>> +{
>>> +   pcie->mux   = base + 0x48;
>>> +   pcie->msi_status= base + 0x80;
>>> +   pcie->msi_enable= base + 0xa0;
>>> +   pcie->msi_doorbell  = 0xa000 + 0x2e07c;
>>> +
>>> +   return tango_check_pcie_link(base + 0x74);
>>
>> Please have some defines for these magic values.
> 
> Typical driver do
> #define MUX_OFFSET 0x48
> and then access the register's value through
> readl_relaxed(pcie->base + MUX_OFFSET);
> 
> I can't do that because the registers were shuffled around
> between revision 1 and revision 2. Thus, instead of an
> explicitly-named macro (MUX_OFFSET), I used an explicitly-
> named field (pcie->mux) and access the register's value
> through readl_relaxed(pcie->mux);

That doesn't prevent you from having a TANGO_V1_MUX_OFFSET define, which
you can supplement with a V2 at some point.

> This is equivalent to providing the offset definitions in the
> init functions, instead of at the top of the file.

Sorry, my brain parses text far better than hex number.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

2017-05-25 Thread Marc Zyngier 
On 25/05/17 13:00, Mason wrote:
> On 25/05/2017 10:48, Marc Zyngier wrote:
> 
>> On 20/04/17 15:31, Marc Gonzalez wrote:
>>
>>> This driver is required to work around several hardware bugs in the
>>> PCIe controller.
>>>
>>> NB: Revision 1 does not support legacy interrupts, or IO space.
>>>
>>> Signed-off-by: Marc Gonzalez 
>>> ---
>>>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 
>>>  drivers/pci/host/Kconfig |   8 ++
>>>  drivers/pci/host/Makefile|   1 +
>>>  drivers/pci/host/pcie-tango.c| 161 
>>> +++
>>>  include/linux/pci_ids.h  |   2 +
>>>  5 files changed, 204 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/tango-pcie.txt 
>>> b/Documentation/devicetree/bindings/pci/tango-pcie.txt
>>> new file mode 100644
>>> index ..3353b4e77309
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pci/tango-pcie.txt
>>> @@ -0,0 +1,32 @@
>>> +Sigma Designs Tango PCIe controller
>>> +
>>> +Required properties:
>>> +
>>> +- compatible: "sigma,smp8759-pcie"
>>> +- reg: address/size of PCI configuration space, address/size of register 
>>> area
>>> +- device_type: "pci"
>>> +- #size-cells: <2>
>>> +- #address-cells: <3>
>>> +- #interrupt-cells: <1>
>>
>> What is the point of having an #interrupt-cells when this is *not* an
>> interrupt controller (as it doesn't support legacy interrupts)?
> 
> My mistake.
> 
> Thanks for kindly pointing out that the #interrupt-cells property
> is not needed when a controller doesn't support legacy interrupts.
> 
> If a controller does support legacy interrupts, then I see other
> bindings define #interrupt-cells and interrupt-map.
> Is interrupt-controller also required?

Probably.

> Is that redundant with msi-controller?

No.

> (Rev2 will support legacy interrupts.)
> 
> References for my own information:
> http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/pci/host-generic-pci.txt
> http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/pci/altera-pcie.txt
> http://elinux.org/Device_Tree_Usage#Advanced_Interrupt_Mapping
> 
>> As mentioned earlier, this needs to be a separate patch to be reviewed
>> by the Keepers of the Faith (aka the DT maintainers).
> 
> robh already acked v3 two months ago, but can split it up,
> and CC the DT folks for v5.

You didn't add the Acked-by to this patch, making Rob's effort pretty
useless.

>>> +static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
>>> +{
>>> +   pcie->mux   = base + 0x48;
>>> +   pcie->msi_status= base + 0x80;
>>> +   pcie->msi_enable= base + 0xa0;
>>> +   pcie->msi_doorbell  = 0xa000 + 0x2e07c;
>>> +
>>> +   return tango_check_pcie_link(base + 0x74);
>>
>> Please have some defines for these magic values.
> 
> Typical driver do
> #define MUX_OFFSET 0x48
> and then access the register's value through
> readl_relaxed(pcie->base + MUX_OFFSET);
> 
> I can't do that because the registers were shuffled around
> between revision 1 and revision 2. Thus, instead of an
> explicitly-named macro (MUX_OFFSET), I used an explicitly-
> named field (pcie->mux) and access the register's value
> through readl_relaxed(pcie->mux);

That doesn't prevent you from having a TANGO_V1_MUX_OFFSET define, which
you can supplement with a V2 at some point.

> This is equivalent to providing the offset definitions in the
> init functions, instead of at the top of the file.

Sorry, my brain parses text far better than hex number.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

2017-05-25 Thread Mason 
On 25/05/2017 10:48, Marc Zyngier wrote:

> On 20/04/17 15:31, Marc Gonzalez wrote:
>
>> This driver is required to work around several hardware bugs in the
>> PCIe controller.
>>
>> NB: Revision 1 does not support legacy interrupts, or IO space.
>>
>> Signed-off-by: Marc Gonzalez 
>> ---
>>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 
>>  drivers/pci/host/Kconfig |   8 ++
>>  drivers/pci/host/Makefile|   1 +
>>  drivers/pci/host/pcie-tango.c| 161 
>> +++
>>  include/linux/pci_ids.h  |   2 +
>>  5 files changed, 204 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/tango-pcie.txt 
>> b/Documentation/devicetree/bindings/pci/tango-pcie.txt
>> new file mode 100644
>> index ..3353b4e77309
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/tango-pcie.txt
>> @@ -0,0 +1,32 @@
>> +Sigma Designs Tango PCIe controller
>> +
>> +Required properties:
>> +
>> +- compatible: "sigma,smp8759-pcie"
>> +- reg: address/size of PCI configuration space, address/size of register 
>> area
>> +- device_type: "pci"
>> +- #size-cells: <2>
>> +- #address-cells: <3>
>> +- #interrupt-cells: <1>
> 
> What is the point of having an #interrupt-cells when this is *not* an
> interrupt controller (as it doesn't support legacy interrupts)?

My mistake.

Thanks for kindly pointing out that the #interrupt-cells property
is not needed when a controller doesn't support legacy interrupts.

If a controller does support legacy interrupts, then I see other
bindings define #interrupt-cells and interrupt-map.
Is interrupt-controller also required?
Is that redundant with msi-controller?

(Rev2 will support legacy interrupts.)

References for my own information:
http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/pci/host-generic-pci.txt
http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/pci/altera-pcie.txt
http://elinux.org/Device_Tree_Usage#Advanced_Interrupt_Mapping

> As mentioned earlier, this needs to be a separate patch to be reviewed
> by the Keepers of the Faith (aka the DT maintainers).

robh already acked v3 two months ago, but can split it up,
and CC the DT folks for v5.

>> +static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
>> +{
>> +pcie->mux   = base + 0x48;
>> +pcie->msi_status= base + 0x80;
>> +pcie->msi_enable= base + 0xa0;
>> +pcie->msi_doorbell  = 0xa000 + 0x2e07c;
>> +
>> +return tango_check_pcie_link(base + 0x74);
> 
> Please have some defines for these magic values.

Typical driver do
#define MUX_OFFSET 0x48
and then access the register's value through
readl_relaxed(pcie->base + MUX_OFFSET);

I can't do that because the registers were shuffled around
between revision 1 and revision 2. Thus, instead of an
explicitly-named macro (MUX_OFFSET), I used an explicitly-
named field (pcie->mux) and access the register's value
through readl_relaxed(pcie->mux);

This is equivalent to providing the offset definitions in the
init functions, instead of at the top of the file.

Regards.

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

2017-05-25 Thread Mason 
On 25/05/2017 10:48, Marc Zyngier wrote:

> On 20/04/17 15:31, Marc Gonzalez wrote:
>
>> This driver is required to work around several hardware bugs in the
>> PCIe controller.
>>
>> NB: Revision 1 does not support legacy interrupts, or IO space.
>>
>> Signed-off-by: Marc Gonzalez 
>> ---
>>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 
>>  drivers/pci/host/Kconfig |   8 ++
>>  drivers/pci/host/Makefile|   1 +
>>  drivers/pci/host/pcie-tango.c| 161 
>> +++
>>  include/linux/pci_ids.h  |   2 +
>>  5 files changed, 204 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/tango-pcie.txt 
>> b/Documentation/devicetree/bindings/pci/tango-pcie.txt
>> new file mode 100644
>> index ..3353b4e77309
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/tango-pcie.txt
>> @@ -0,0 +1,32 @@
>> +Sigma Designs Tango PCIe controller
>> +
>> +Required properties:
>> +
>> +- compatible: "sigma,smp8759-pcie"
>> +- reg: address/size of PCI configuration space, address/size of register 
>> area
>> +- device_type: "pci"
>> +- #size-cells: <2>
>> +- #address-cells: <3>
>> +- #interrupt-cells: <1>
> 
> What is the point of having an #interrupt-cells when this is *not* an
> interrupt controller (as it doesn't support legacy interrupts)?

My mistake.

Thanks for kindly pointing out that the #interrupt-cells property
is not needed when a controller doesn't support legacy interrupts.

If a controller does support legacy interrupts, then I see other
bindings define #interrupt-cells and interrupt-map.
Is interrupt-controller also required?
Is that redundant with msi-controller?

(Rev2 will support legacy interrupts.)

References for my own information:
http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/pci/host-generic-pci.txt
http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/pci/altera-pcie.txt
http://elinux.org/Device_Tree_Usage#Advanced_Interrupt_Mapping

> As mentioned earlier, this needs to be a separate patch to be reviewed
> by the Keepers of the Faith (aka the DT maintainers).

robh already acked v3 two months ago, but can split it up,
and CC the DT folks for v5.

>> +static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
>> +{
>> +pcie->mux   = base + 0x48;
>> +pcie->msi_status= base + 0x80;
>> +pcie->msi_enable= base + 0xa0;
>> +pcie->msi_doorbell  = 0xa000 + 0x2e07c;
>> +
>> +return tango_check_pcie_link(base + 0x74);
> 
> Please have some defines for these magic values.

Typical driver do
#define MUX_OFFSET 0x48
and then access the register's value through
readl_relaxed(pcie->base + MUX_OFFSET);

I can't do that because the registers were shuffled around
between revision 1 and revision 2. Thus, instead of an
explicitly-named macro (MUX_OFFSET), I used an explicitly-
named field (pcie->mux) and access the register's value
through readl_relaxed(pcie->mux);

This is equivalent to providing the offset definitions in the
init functions, instead of at the top of the file.

Regards.

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

2017-05-25 Thread Marc Zyngier 
On 20/04/17 15:31, Marc Gonzalez wrote:
> This driver is required to work around several hardware bugs in the
> PCIe controller.
> 
> NB: Revision 1 does not support legacy interrupts, or IO space.
> 
> Signed-off-by: Marc Gonzalez 
> ---
>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 
>  drivers/pci/host/Kconfig |   8 ++
>  drivers/pci/host/Makefile|   1 +
>  drivers/pci/host/pcie-tango.c| 161 
> +++
>  include/linux/pci_ids.h  |   2 +
>  5 files changed, 204 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/tango-pcie.txt 
> b/Documentation/devicetree/bindings/pci/tango-pcie.txt
> new file mode 100644
> index ..3353b4e77309
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/tango-pcie.txt
> @@ -0,0 +1,32 @@
> +Sigma Designs Tango PCIe controller
> +
> +Required properties:
> +
> +- compatible: "sigma,smp8759-pcie"
> +- reg: address/size of PCI configuration space, address/size of register area
> +- device_type: "pci"
> +- #size-cells: <2>
> +- #address-cells: <3>
> +- #interrupt-cells: <1>

What is the point of having an #interrupt-cells when this is *not* an
interrupt controller (as it doesn't support legacy interrupts)?

> +- ranges: translation from system to bus addresses
> +- interrupts: spec for misc interrupts, spec for MSI
> +- msi-controller
> +
> +http://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> +http://elinux.org/Device_Tree_Usage#Advanced_Interrupt_Mapping
> +
> +Example:
> +
> + pcie@2e000 {
> + compatible = "sigma,smp8759-pcie";
> + reg = <0x5000 SZ_4M>, <0x2e000 0x100>;
> + device_type = "pci";
> + #size-cells = <2>;
> + #address-cells = <3>;
> + #interrupt-cells = <1>;
> + ranges = <0x0200 0x0 0x0040  0x5040  0x0 SZ_60M>;
> + msi-controller;
> + interrupts =
> + <54 IRQ_TYPE_LEVEL_HIGH>, /* misc interrupts */
> + <55 IRQ_TYPE_LEVEL_HIGH>; /* MSI */
> + };

As mentioned earlier, this needs to be a separate patch to be reviewed
by the Keepers of the Faith (aka the DT maintainers).

[...]

> +static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
> +{
> + pcie->mux   = base + 0x48;
> + pcie->msi_status= base + 0x80;
> + pcie->msi_enable= base + 0xa0;
> + pcie->msi_doorbell  = 0xa000 + 0x2e07c;
> +
> + return tango_check_pcie_link(base + 0x74);

Please have some defines for these magic values.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

2017-05-25 Thread Marc Zyngier 
On 20/04/17 15:31, Marc Gonzalez wrote:
> This driver is required to work around several hardware bugs in the
> PCIe controller.
> 
> NB: Revision 1 does not support legacy interrupts, or IO space.
> 
> Signed-off-by: Marc Gonzalez 
> ---
>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 
>  drivers/pci/host/Kconfig |   8 ++
>  drivers/pci/host/Makefile|   1 +
>  drivers/pci/host/pcie-tango.c| 161 
> +++
>  include/linux/pci_ids.h  |   2 +
>  5 files changed, 204 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/tango-pcie.txt 
> b/Documentation/devicetree/bindings/pci/tango-pcie.txt
> new file mode 100644
> index ..3353b4e77309
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/tango-pcie.txt
> @@ -0,0 +1,32 @@
> +Sigma Designs Tango PCIe controller
> +
> +Required properties:
> +
> +- compatible: "sigma,smp8759-pcie"
> +- reg: address/size of PCI configuration space, address/size of register area
> +- device_type: "pci"
> +- #size-cells: <2>
> +- #address-cells: <3>
> +- #interrupt-cells: <1>

What is the point of having an #interrupt-cells when this is *not* an
interrupt controller (as it doesn't support legacy interrupts)?

> +- ranges: translation from system to bus addresses
> +- interrupts: spec for misc interrupts, spec for MSI
> +- msi-controller
> +
> +http://elinux.org/Device_Tree_Usage#PCI_Address_Translation
> +http://elinux.org/Device_Tree_Usage#Advanced_Interrupt_Mapping
> +
> +Example:
> +
> + pcie@2e000 {
> + compatible = "sigma,smp8759-pcie";
> + reg = <0x5000 SZ_4M>, <0x2e000 0x100>;
> + device_type = "pci";
> + #size-cells = <2>;
> + #address-cells = <3>;
> + #interrupt-cells = <1>;
> + ranges = <0x0200 0x0 0x0040  0x5040  0x0 SZ_60M>;
> + msi-controller;
> + interrupts =
> + <54 IRQ_TYPE_LEVEL_HIGH>, /* misc interrupts */
> + <55 IRQ_TYPE_LEVEL_HIGH>; /* MSI */
> + };

As mentioned earlier, this needs to be a separate patch to be reviewed
by the Keepers of the Faith (aka the DT maintainers).

[...]

> +static int smp8759_init(struct tango_pcie *pcie, void __iomem *base)
> +{
> + pcie->mux   = base + 0x48;
> + pcie->msi_status= base + 0x80;
> + pcie->msi_enable= base + 0xa0;
> + pcie->msi_doorbell  = 0xa000 + 0x2e07c;
> +
> + return tango_check_pcie_link(base + 0x74);

Please have some defines for these magic values.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

2017-05-23 Thread Mason 
On 23/05/2017 20:35, Bjorn Helgaas wrote:
> On Tue, May 23, 2017 at 07:43:16PM +0200, Mason wrote:
>> On 23/05/2017 19:24, Bjorn Helgaas wrote:
>>> On Thu, Apr 20, 2017 at 04:31:25PM +0200, Marc Gonzalez wrote:
 This driver is required to work around several hardware bugs in the
 PCIe controller.

 NB: Revision 1 does not support legacy interrupts, or IO space.

 Signed-off-by: Marc Gonzalez 
 ---
  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 
  drivers/pci/host/Kconfig |   8 ++
  drivers/pci/host/Makefile|   1 +
  drivers/pci/host/pcie-tango.c| 161 
 +++
  include/linux/pci_ids.h  |   2 +
  5 files changed, 204 insertions(+)
 ...
>>>
 +static int smp8759_config_read(struct pci_bus *bus,
 +  unsigned int devfn, int where, int size, u32 *val)
 +{
 +  int ret;
 +  struct pci_config_window *cfg = bus->sysdata;
 +  struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
 +
 +  /*
 +   * QUIRK #1
 +   * Reads in configuration space outside devfn 0 return garbage.
 +   */
 +  if (devfn != 0)
 +  return PCIBIOS_FUNC_NOT_SUPPORTED;
 +
 +  /*
 +   * QUIRK #2
 +   * Unfortunately, config and mem spaces are muxed.
 +   * Linux does not support such a setting, since drivers are free
 +   * to access mem space directly, at any time.
 +   * Therefore, we can only PRAY that config and mem space accesses
 +   * NEVER occur concurrently.
 +   */
 +  writel_relaxed(1, pcie->mux);
 +  ret = pci_generic_config_read(bus, devfn, where, size, val);
 +  writel_relaxed(0, pcie->mux);
>>>
>>> This is a major issue and possibly even a security problem.
>>> Unprivileged users can cause config accesses via lspci/setpci.
>>
>> Since the host bridge doesn't support hotplug in any way,
>> how about setting a flag once enumeration is complete,
>> to prevent all future config accesses?
> 
> I thought about that, too, but I'm not sure it will work becuase I
> think drivers will need to do at least a few config accesses, e.g.,
> pci_enable_device() may write PCI_COMMAND to set PCI_COMMAND_MEMORY,
> it may read PCI_INTERRUPT_PIN, etc.  And MSI setup requires config
> access, too.
> 
>>> I don't have a good suggestion for addressing it, but if you can't
>>> make this work reliably, you need at least a dev_err() in the probe
>>> function and probably a taint of the kernel (see add_taint()).
>>
>> There is a chance that the issue will get fixed in rev2
>> of the bridge. But obviously, that won't help for rev1
>> already in the wild.
> 
> Hopefully there will be a way to distinguish rev2 from rev1.

rev2 is embedded in a new SoC, so the DT node will specify a
different compatible string. A "preview" of the intent was
given in an older patch:
"[RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge"

Regards.

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

2017-05-23 Thread Mason 
On 23/05/2017 20:35, Bjorn Helgaas wrote:
> On Tue, May 23, 2017 at 07:43:16PM +0200, Mason wrote:
>> On 23/05/2017 19:24, Bjorn Helgaas wrote:
>>> On Thu, Apr 20, 2017 at 04:31:25PM +0200, Marc Gonzalez wrote:
 This driver is required to work around several hardware bugs in the
 PCIe controller.

 NB: Revision 1 does not support legacy interrupts, or IO space.

 Signed-off-by: Marc Gonzalez 
 ---
  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 
  drivers/pci/host/Kconfig |   8 ++
  drivers/pci/host/Makefile|   1 +
  drivers/pci/host/pcie-tango.c| 161 
 +++
  include/linux/pci_ids.h  |   2 +
  5 files changed, 204 insertions(+)
 ...
>>>
 +static int smp8759_config_read(struct pci_bus *bus,
 +  unsigned int devfn, int where, int size, u32 *val)
 +{
 +  int ret;
 +  struct pci_config_window *cfg = bus->sysdata;
 +  struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
 +
 +  /*
 +   * QUIRK #1
 +   * Reads in configuration space outside devfn 0 return garbage.
 +   */
 +  if (devfn != 0)
 +  return PCIBIOS_FUNC_NOT_SUPPORTED;
 +
 +  /*
 +   * QUIRK #2
 +   * Unfortunately, config and mem spaces are muxed.
 +   * Linux does not support such a setting, since drivers are free
 +   * to access mem space directly, at any time.
 +   * Therefore, we can only PRAY that config and mem space accesses
 +   * NEVER occur concurrently.
 +   */
 +  writel_relaxed(1, pcie->mux);
 +  ret = pci_generic_config_read(bus, devfn, where, size, val);
 +  writel_relaxed(0, pcie->mux);
>>>
>>> This is a major issue and possibly even a security problem.
>>> Unprivileged users can cause config accesses via lspci/setpci.
>>
>> Since the host bridge doesn't support hotplug in any way,
>> how about setting a flag once enumeration is complete,
>> to prevent all future config accesses?
> 
> I thought about that, too, but I'm not sure it will work becuase I
> think drivers will need to do at least a few config accesses, e.g.,
> pci_enable_device() may write PCI_COMMAND to set PCI_COMMAND_MEMORY,
> it may read PCI_INTERRUPT_PIN, etc.  And MSI setup requires config
> access, too.
> 
>>> I don't have a good suggestion for addressing it, but if you can't
>>> make this work reliably, you need at least a dev_err() in the probe
>>> function and probably a taint of the kernel (see add_taint()).
>>
>> There is a chance that the issue will get fixed in rev2
>> of the bridge. But obviously, that won't help for rev1
>> already in the wild.
> 
> Hopefully there will be a way to distinguish rev2 from rev1.

rev2 is embedded in a new SoC, so the DT node will specify a
different compatible string. A "preview" of the intent was
given in an older patch:
"[RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge"

Regards.

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

2017-05-23 Thread Bjorn Helgaas 
On Tue, May 23, 2017 at 07:43:16PM +0200, Mason wrote:
> On 23/05/2017 19:24, Bjorn Helgaas wrote:
> > On Thu, Apr 20, 2017 at 04:31:25PM +0200, Marc Gonzalez wrote:
> >> This driver is required to work around several hardware bugs in the
> >> PCIe controller.
> >>
> >> NB: Revision 1 does not support legacy interrupts, or IO space.
> >>
> >> Signed-off-by: Marc Gonzalez 
> >> ---
> >>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 
> >>  drivers/pci/host/Kconfig |   8 ++
> >>  drivers/pci/host/Makefile|   1 +
> >>  drivers/pci/host/pcie-tango.c| 161 
> >> +++
> >>  include/linux/pci_ids.h  |   2 +
> >>  5 files changed, 204 insertions(+)
> >> ...
> > 
> >> +static int smp8759_config_read(struct pci_bus *bus,
> >> +  unsigned int devfn, int where, int size, u32 *val)
> >> +{
> >> +  int ret;
> >> +  struct pci_config_window *cfg = bus->sysdata;
> >> +  struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
> >> +
> >> +  /*
> >> +   * QUIRK #1
> >> +   * Reads in configuration space outside devfn 0 return garbage.
> >> +   */
> >> +  if (devfn != 0)
> >> +  return PCIBIOS_FUNC_NOT_SUPPORTED;
> >> +
> >> +  /*
> >> +   * QUIRK #2
> >> +   * Unfortunately, config and mem spaces are muxed.
> >> +   * Linux does not support such a setting, since drivers are free
> >> +   * to access mem space directly, at any time.
> >> +   * Therefore, we can only PRAY that config and mem space accesses
> >> +   * NEVER occur concurrently.
> >> +   */
> >> +  writel_relaxed(1, pcie->mux);
> >> +  ret = pci_generic_config_read(bus, devfn, where, size, val);
> >> +  writel_relaxed(0, pcie->mux);
> > 
> > This is a major issue and possibly even a security problem.
> > Unprivileged users can cause config accesses via lspci/setpci.
> 
> Since the host bridge doesn't support hotplug in any way,
> how about setting a flag once enumeration is complete,
> to prevent all future config accesses?

I thought about that, too, but I'm not sure it will work becuase I
think drivers will need to do at least a few config accesses, e.g.,
pci_enable_device() may write PCI_COMMAND to set PCI_COMMAND_MEMORY,
it may read PCI_INTERRUPT_PIN, etc.  And MSI setup requires config
access, too.

> > I don't have a good suggestion for addressing it, but if you can't
> > make this work reliably, you need at least a dev_err() in the probe
> > function and probably a taint of the kernel (see add_taint()).
> 
> There is a chance that the issue will get fixed in rev2
> of the bridge. But obviously, that won't help for rev1
> already in the wild.

Hopefully there will be a way to distinguish rev2 from rev1.

Bjorn

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

2017-05-23 Thread Bjorn Helgaas 
On Tue, May 23, 2017 at 07:43:16PM +0200, Mason wrote:
> On 23/05/2017 19:24, Bjorn Helgaas wrote:
> > On Thu, Apr 20, 2017 at 04:31:25PM +0200, Marc Gonzalez wrote:
> >> This driver is required to work around several hardware bugs in the
> >> PCIe controller.
> >>
> >> NB: Revision 1 does not support legacy interrupts, or IO space.
> >>
> >> Signed-off-by: Marc Gonzalez 
> >> ---
> >>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 
> >>  drivers/pci/host/Kconfig |   8 ++
> >>  drivers/pci/host/Makefile|   1 +
> >>  drivers/pci/host/pcie-tango.c| 161 
> >> +++
> >>  include/linux/pci_ids.h  |   2 +
> >>  5 files changed, 204 insertions(+)
> >> ...
> > 
> >> +static int smp8759_config_read(struct pci_bus *bus,
> >> +  unsigned int devfn, int where, int size, u32 *val)
> >> +{
> >> +  int ret;
> >> +  struct pci_config_window *cfg = bus->sysdata;
> >> +  struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
> >> +
> >> +  /*
> >> +   * QUIRK #1
> >> +   * Reads in configuration space outside devfn 0 return garbage.
> >> +   */
> >> +  if (devfn != 0)
> >> +  return PCIBIOS_FUNC_NOT_SUPPORTED;
> >> +
> >> +  /*
> >> +   * QUIRK #2
> >> +   * Unfortunately, config and mem spaces are muxed.
> >> +   * Linux does not support such a setting, since drivers are free
> >> +   * to access mem space directly, at any time.
> >> +   * Therefore, we can only PRAY that config and mem space accesses
> >> +   * NEVER occur concurrently.
> >> +   */
> >> +  writel_relaxed(1, pcie->mux);
> >> +  ret = pci_generic_config_read(bus, devfn, where, size, val);
> >> +  writel_relaxed(0, pcie->mux);
> > 
> > This is a major issue and possibly even a security problem.
> > Unprivileged users can cause config accesses via lspci/setpci.
> 
> Since the host bridge doesn't support hotplug in any way,
> how about setting a flag once enumeration is complete,
> to prevent all future config accesses?

I thought about that, too, but I'm not sure it will work becuase I
think drivers will need to do at least a few config accesses, e.g.,
pci_enable_device() may write PCI_COMMAND to set PCI_COMMAND_MEMORY,
it may read PCI_INTERRUPT_PIN, etc.  And MSI setup requires config
access, too.

> > I don't have a good suggestion for addressing it, but if you can't
> > make this work reliably, you need at least a dev_err() in the probe
> > function and probably a taint of the kernel (see add_taint()).
> 
> There is a chance that the issue will get fixed in rev2
> of the bridge. But obviously, that won't help for rev1
> already in the wild.

Hopefully there will be a way to distinguish rev2 from rev1.

Bjorn

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

2017-05-23 Thread Mason 
On 23/05/2017 19:24, Bjorn Helgaas wrote:
> On Thu, Apr 20, 2017 at 04:31:25PM +0200, Marc Gonzalez wrote:
>> This driver is required to work around several hardware bugs in the
>> PCIe controller.
>>
>> NB: Revision 1 does not support legacy interrupts, or IO space.
>>
>> Signed-off-by: Marc Gonzalez 
>> ---
>>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 
>>  drivers/pci/host/Kconfig |   8 ++
>>  drivers/pci/host/Makefile|   1 +
>>  drivers/pci/host/pcie-tango.c| 161 
>> +++
>>  include/linux/pci_ids.h  |   2 +
>>  5 files changed, 204 insertions(+)
>> ...
> 
>> +static int smp8759_config_read(struct pci_bus *bus,
>> +unsigned int devfn, int where, int size, u32 *val)
>> +{
>> +int ret;
>> +struct pci_config_window *cfg = bus->sysdata;
>> +struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
>> +
>> +/*
>> + * QUIRK #1
>> + * Reads in configuration space outside devfn 0 return garbage.
>> + */
>> +if (devfn != 0)
>> +return PCIBIOS_FUNC_NOT_SUPPORTED;
>> +
>> +/*
>> + * QUIRK #2
>> + * Unfortunately, config and mem spaces are muxed.
>> + * Linux does not support such a setting, since drivers are free
>> + * to access mem space directly, at any time.
>> + * Therefore, we can only PRAY that config and mem space accesses
>> + * NEVER occur concurrently.
>> + */
>> +writel_relaxed(1, pcie->mux);
>> +ret = pci_generic_config_read(bus, devfn, where, size, val);
>> +writel_relaxed(0, pcie->mux);
> 
> This is a major issue and possibly even a security problem.
> Unprivileged users can cause config accesses via lspci/setpci.

Since the host bridge doesn't support hotplug in any way,
how about setting a flag once enumeration is complete,
to prevent all future config accesses?

> I don't have a good suggestion for addressing it, but if you can't
> make this work reliably, you need at least a dev_err() in the probe
> function and probably a taint of the kernel (see add_taint()).

There is a chance that the issue will get fixed in rev2
of the bridge. But obviously, that won't help for rev1
already in the wild.

Regards.

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

2017-05-23 Thread Mason 
On 23/05/2017 19:24, Bjorn Helgaas wrote:
> On Thu, Apr 20, 2017 at 04:31:25PM +0200, Marc Gonzalez wrote:
>> This driver is required to work around several hardware bugs in the
>> PCIe controller.
>>
>> NB: Revision 1 does not support legacy interrupts, or IO space.
>>
>> Signed-off-by: Marc Gonzalez 
>> ---
>>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 
>>  drivers/pci/host/Kconfig |   8 ++
>>  drivers/pci/host/Makefile|   1 +
>>  drivers/pci/host/pcie-tango.c| 161 
>> +++
>>  include/linux/pci_ids.h  |   2 +
>>  5 files changed, 204 insertions(+)
>> ...
> 
>> +static int smp8759_config_read(struct pci_bus *bus,
>> +unsigned int devfn, int where, int size, u32 *val)
>> +{
>> +int ret;
>> +struct pci_config_window *cfg = bus->sysdata;
>> +struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
>> +
>> +/*
>> + * QUIRK #1
>> + * Reads in configuration space outside devfn 0 return garbage.
>> + */
>> +if (devfn != 0)
>> +return PCIBIOS_FUNC_NOT_SUPPORTED;
>> +
>> +/*
>> + * QUIRK #2
>> + * Unfortunately, config and mem spaces are muxed.
>> + * Linux does not support such a setting, since drivers are free
>> + * to access mem space directly, at any time.
>> + * Therefore, we can only PRAY that config and mem space accesses
>> + * NEVER occur concurrently.
>> + */
>> +writel_relaxed(1, pcie->mux);
>> +ret = pci_generic_config_read(bus, devfn, where, size, val);
>> +writel_relaxed(0, pcie->mux);
> 
> This is a major issue and possibly even a security problem.
> Unprivileged users can cause config accesses via lspci/setpci.

Since the host bridge doesn't support hotplug in any way,
how about setting a flag once enumeration is complete,
to prevent all future config accesses?

> I don't have a good suggestion for addressing it, but if you can't
> make this work reliably, you need at least a dev_err() in the probe
> function and probably a taint of the kernel (see add_taint()).

There is a chance that the issue will get fixed in rev2
of the bridge. But obviously, that won't help for rev1
already in the wild.

Regards.

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

2017-05-23 Thread Bjorn Helgaas 
On Thu, Apr 20, 2017 at 04:31:25PM +0200, Marc Gonzalez wrote:
> This driver is required to work around several hardware bugs in the
> PCIe controller.
> 
> NB: Revision 1 does not support legacy interrupts, or IO space.
> 
> Signed-off-by: Marc Gonzalez 
> ---
>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 
>  drivers/pci/host/Kconfig |   8 ++
>  drivers/pci/host/Makefile|   1 +
>  drivers/pci/host/pcie-tango.c| 161 
> +++
>  include/linux/pci_ids.h  |   2 +
>  5 files changed, 204 insertions(+)
> ...

> +static int smp8759_config_read(struct pci_bus *bus,
> + unsigned int devfn, int where, int size, u32 *val)
> +{
> + int ret;
> + struct pci_config_window *cfg = bus->sysdata;
> + struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
> +
> + /*
> +  * QUIRK #1
> +  * Reads in configuration space outside devfn 0 return garbage.
> +  */
> + if (devfn != 0)
> + return PCIBIOS_FUNC_NOT_SUPPORTED;
> +
> + /*
> +  * QUIRK #2
> +  * Unfortunately, config and mem spaces are muxed.
> +  * Linux does not support such a setting, since drivers are free
> +  * to access mem space directly, at any time.
> +  * Therefore, we can only PRAY that config and mem space accesses
> +  * NEVER occur concurrently.
> +  */
> + writel_relaxed(1, pcie->mux);
> + ret = pci_generic_config_read(bus, devfn, where, size, val);
> + writel_relaxed(0, pcie->mux);

This is a major issue and possibly even a security problem.
Unprivileged users can cause config accesses via lspci/setpci.

I don't have a good suggestion for addressing it, but if you can't
make this work reliably, you need at least a dev_err() in the probe
function and probably a taint of the kernel (see add_taint()).

> + return ret;
> +}
> +
> +static int smp8759_config_write(struct pci_bus *bus,
> + unsigned int devfn, int where, int size, u32 val)
> +{
> + int ret;
> + struct pci_config_window *cfg = bus->sysdata;
> + struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
> +
> + writel_relaxed(1, pcie->mux);
> + ret = pci_generic_config_write(bus, devfn, where, size, val);
> + writel_relaxed(0, pcie->mux);
> +
> + return ret;
> +}
> +
> +static struct pci_ecam_ops smp8759_ecam_ops = {
> + .bus_shift  = 20,
> + .pci_ops= {
> + .map_bus= pci_ecam_map_bus,
> + .read   = smp8759_config_read,
> + .write  = smp8759_config_write,
> + }
> +};

Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support

2017-05-23 Thread Bjorn Helgaas 
On Thu, Apr 20, 2017 at 04:31:25PM +0200, Marc Gonzalez wrote:
> This driver is required to work around several hardware bugs in the
> PCIe controller.
> 
> NB: Revision 1 does not support legacy interrupts, or IO space.
> 
> Signed-off-by: Marc Gonzalez 
> ---
>  Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 
>  drivers/pci/host/Kconfig |   8 ++
>  drivers/pci/host/Makefile|   1 +
>  drivers/pci/host/pcie-tango.c| 161 
> +++
>  include/linux/pci_ids.h  |   2 +
>  5 files changed, 204 insertions(+)
> ...

> +static int smp8759_config_read(struct pci_bus *bus,
> + unsigned int devfn, int where, int size, u32 *val)
> +{
> + int ret;
> + struct pci_config_window *cfg = bus->sysdata;
> + struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
> +
> + /*
> +  * QUIRK #1
> +  * Reads in configuration space outside devfn 0 return garbage.
> +  */
> + if (devfn != 0)
> + return PCIBIOS_FUNC_NOT_SUPPORTED;
> +
> + /*
> +  * QUIRK #2
> +  * Unfortunately, config and mem spaces are muxed.
> +  * Linux does not support such a setting, since drivers are free
> +  * to access mem space directly, at any time.
> +  * Therefore, we can only PRAY that config and mem space accesses
> +  * NEVER occur concurrently.
> +  */
> + writel_relaxed(1, pcie->mux);
> + ret = pci_generic_config_read(bus, devfn, where, size, val);
> + writel_relaxed(0, pcie->mux);

This is a major issue and possibly even a security problem.
Unprivileged users can cause config accesses via lspci/setpci.

I don't have a good suggestion for addressing it, but if you can't
make this work reliably, you need at least a dev_err() in the probe
function and probably a taint of the kernel (see add_taint()).

> + return ret;
> +}
> +
> +static int smp8759_config_write(struct pci_bus *bus,
> + unsigned int devfn, int where, int size, u32 val)
> +{
> + int ret;
> + struct pci_config_window *cfg = bus->sysdata;
> + struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
> +
> + writel_relaxed(1, pcie->mux);
> + ret = pci_generic_config_write(bus, devfn, where, size, val);
> + writel_relaxed(0, pcie->mux);
> +
> + return ret;
> +}
> +
> +static struct pci_ecam_ops smp8759_ecam_ops = {
> + .bus_shift  = 20,
> + .pci_ops= {
> + .map_bus= pci_ecam_map_bus,
> + .read   = smp8759_config_read,
> + .write  = smp8759_config_write,
> + }
> +};

[PATCH v4 2/2] PCI: Add tango PCIe host bridge support

2017-04-20 Thread Marc Gonzalez 
This driver is required to work around several hardware bugs in the
PCIe controller.

NB: Revision 1 does not support legacy interrupts, or IO space.

Signed-off-by: Marc Gonzalez 
---
 Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 
 drivers/pci/host/Kconfig |   8 ++
 drivers/pci/host/Makefile|   1 +
 drivers/pci/host/pcie-tango.c| 161 
+++
 include/linux/pci_ids.h  |   2 +
 5 files changed, 204 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/tango-pcie.txt 
b/Documentation/devicetree/bindings/pci/tango-pcie.txt
new file mode 100644
index ..3353b4e77309
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/tango-pcie.txt
@@ -0,0 +1,32 @@
+Sigma Designs Tango PCIe controller
+
+Required properties:
+
+- compatible: "sigma,smp8759-pcie"
+- reg: address/size of PCI configuration space, address/size of register area
+- device_type: "pci"
+- #size-cells: <2>
+- #address-cells: <3>
+- #interrupt-cells: <1>
+- ranges: translation from system to bus addresses
+- interrupts: spec for misc interrupts, spec for MSI
+- msi-controller
+
+http://elinux.org/Device_Tree_Usage#PCI_Address_Translation
+http://elinux.org/Device_Tree_Usage#Advanced_Interrupt_Mapping
+
+Example:
+
+   pcie@2e000 {
+   compatible = "sigma,smp8759-pcie";
+   reg = <0x5000 SZ_4M>, <0x2e000 0x100>;
+   device_type = "pci";
+   #size-cells = <2>;
+   #address-cells = <3>;
+   #interrupt-cells = <1>;
+   ranges = <0x0200 0x0 0x0040  0x5040  0x0 SZ_60M>;
+   msi-controller;
+   interrupts =
+   <54 IRQ_TYPE_LEVEL_HIGH>, /* misc interrupts */
+   <55 IRQ_TYPE_LEVEL_HIGH>; /* MSI */
+   };
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index d7e7c0a827c3..5183d9095c3a 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -285,6 +285,14 @@ config PCIE_ROCKCHIP
  There is 1 internal PCIe port available to support GEN2 with
  4 slots.
 
+config PCIE_TANGO
+   bool "Tango PCIe controller"
+   depends on ARCH_TANGO && PCI_MSI && OF
+   select PCI_HOST_COMMON
+   help
+ Say Y here to enable PCIe controller support for Sigma Designs
+ Tango systems, such as SMP8759 and later chips.
+
 config VMD
depends on PCI_MSI && X86_64
tristate "Intel Volume Management Device Driver"
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 084cb4983645..fc7ea90196f3 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -32,4 +32,5 @@ obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
 obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
 obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
 obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o
+obj-$(CONFIG_PCIE_TANGO) += pcie-tango.o
 obj-$(CONFIG_VMD) += vmd.o
diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
index ada8d35066ab..7eb74e82d325 100644
--- a/drivers/pci/host/pcie-tango.c
+++ b/drivers/pci/host/pcie-tango.c
@@ -230,3 +230,164 @@ static int tango_msi_probe(struct platform_device *pdev, 
struct tango_pcie *pcie
 
return 0;
 }
+
+/*** HOST BRIDGE SUPPORT ***/
+
+static int smp8759_config_read(struct pci_bus *bus,
+   unsigned int devfn, int where, int size, u32 *val)
+{
+   int ret;
+   struct pci_config_window *cfg = bus->sysdata;
+   struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
+
+   /*
+* QUIRK #1
+* Reads in configuration space outside devfn 0 return garbage.
+*/
+   if (devfn != 0)
+   return PCIBIOS_FUNC_NOT_SUPPORTED;
+
+   /*
+* QUIRK #2
+* Unfortunately, config and mem spaces are muxed.
+* Linux does not support such a setting, since drivers are free
+* to access mem space directly, at any time.
+* Therefore, we can only PRAY that config and mem space accesses
+* NEVER occur concurrently.
+*/
+   writel_relaxed(1, pcie->mux);
+   ret = pci_generic_config_read(bus, devfn, where, size, val);
+   writel_relaxed(0, pcie->mux);
+
+   return ret;
+}
+
+static int smp8759_config_write(struct pci_bus *bus,
+   unsigned int devfn, int where, int size, u32 val)
+{
+   int ret;
+   struct pci_config_window *cfg = bus->sysdata;
+   struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
+
+   writel_relaxed(1, pcie->mux);
+   ret = pci_generic_config_write(bus, devfn, where, size, val);
+   writel_relaxed(0, pcie->mux);
+
+   return ret;
+}
+
+static struct pci_ecam_ops smp8759_ecam_ops = {
+   .bus_shift  = 20,
+   .pci_ops= {
+   .map_bus

[PATCH v4 2/2] PCI: Add tango PCIe host bridge support

2017-04-20 Thread Marc Gonzalez 
This driver is required to work around several hardware bugs in the
PCIe controller.

NB: Revision 1 does not support legacy interrupts, or IO space.

Signed-off-by: Marc Gonzalez 
---
 Documentation/devicetree/bindings/pci/tango-pcie.txt |  32 
 drivers/pci/host/Kconfig |   8 ++
 drivers/pci/host/Makefile|   1 +
 drivers/pci/host/pcie-tango.c| 161 
+++
 include/linux/pci_ids.h  |   2 +
 5 files changed, 204 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/tango-pcie.txt 
b/Documentation/devicetree/bindings/pci/tango-pcie.txt
new file mode 100644
index ..3353b4e77309
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/tango-pcie.txt
@@ -0,0 +1,32 @@
+Sigma Designs Tango PCIe controller
+
+Required properties:
+
+- compatible: "sigma,smp8759-pcie"
+- reg: address/size of PCI configuration space, address/size of register area
+- device_type: "pci"
+- #size-cells: <2>
+- #address-cells: <3>
+- #interrupt-cells: <1>
+- ranges: translation from system to bus addresses
+- interrupts: spec for misc interrupts, spec for MSI
+- msi-controller
+
+http://elinux.org/Device_Tree_Usage#PCI_Address_Translation
+http://elinux.org/Device_Tree_Usage#Advanced_Interrupt_Mapping
+
+Example:
+
+   pcie@2e000 {
+   compatible = "sigma,smp8759-pcie";
+   reg = <0x5000 SZ_4M>, <0x2e000 0x100>;
+   device_type = "pci";
+   #size-cells = <2>;
+   #address-cells = <3>;
+   #interrupt-cells = <1>;
+   ranges = <0x0200 0x0 0x0040  0x5040  0x0 SZ_60M>;
+   msi-controller;
+   interrupts =
+   <54 IRQ_TYPE_LEVEL_HIGH>, /* misc interrupts */
+   <55 IRQ_TYPE_LEVEL_HIGH>; /* MSI */
+   };
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index d7e7c0a827c3..5183d9095c3a 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -285,6 +285,14 @@ config PCIE_ROCKCHIP
  There is 1 internal PCIe port available to support GEN2 with
  4 slots.
 
+config PCIE_TANGO
+   bool "Tango PCIe controller"
+   depends on ARCH_TANGO && PCI_MSI && OF
+   select PCI_HOST_COMMON
+   help
+ Say Y here to enable PCIe controller support for Sigma Designs
+ Tango systems, such as SMP8759 and later chips.
+
 config VMD
depends on PCI_MSI && X86_64
tristate "Intel Volume Management Device Driver"
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 084cb4983645..fc7ea90196f3 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -32,4 +32,5 @@ obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
 obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
 obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
 obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o
+obj-$(CONFIG_PCIE_TANGO) += pcie-tango.o
 obj-$(CONFIG_VMD) += vmd.o
diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
index ada8d35066ab..7eb74e82d325 100644
--- a/drivers/pci/host/pcie-tango.c
+++ b/drivers/pci/host/pcie-tango.c
@@ -230,3 +230,164 @@ static int tango_msi_probe(struct platform_device *pdev, 
struct tango_pcie *pcie
 
return 0;
 }
+
+/*** HOST BRIDGE SUPPORT ***/
+
+static int smp8759_config_read(struct pci_bus *bus,
+   unsigned int devfn, int where, int size, u32 *val)
+{
+   int ret;
+   struct pci_config_window *cfg = bus->sysdata;
+   struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
+
+   /*
+* QUIRK #1
+* Reads in configuration space outside devfn 0 return garbage.
+*/
+   if (devfn != 0)
+   return PCIBIOS_FUNC_NOT_SUPPORTED;
+
+   /*
+* QUIRK #2
+* Unfortunately, config and mem spaces are muxed.
+* Linux does not support such a setting, since drivers are free
+* to access mem space directly, at any time.
+* Therefore, we can only PRAY that config and mem space accesses
+* NEVER occur concurrently.
+*/
+   writel_relaxed(1, pcie->mux);
+   ret = pci_generic_config_read(bus, devfn, where, size, val);
+   writel_relaxed(0, pcie->mux);
+
+   return ret;
+}
+
+static int smp8759_config_write(struct pci_bus *bus,
+   unsigned int devfn, int where, int size, u32 val)
+{
+   int ret;
+   struct pci_config_window *cfg = bus->sysdata;
+   struct tango_pcie *pcie = dev_get_drvdata(cfg->parent);
+
+   writel_relaxed(1, pcie->mux);
+   ret = pci_generic_config_write(bus, devfn, where, size, val);
+   writel_relaxed(0, pcie->mux);
+
+   return ret;
+}
+
+static struct pci_ecam_ops smp8759_ecam_ops = {
+   .bus_shift  = 20,
+   .pci_ops= {
+   .map_bus= pci_ecam_map_bus,
+