Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1650329 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support Marc Zyngier <marc.zyngier@arm.com> - 2017-05-25 10:50 +0200
    Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support Mason <slash.tmp@free.fr> - 2017-05-25 14:10 +0200
      Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support Marc Zyngier <marc.zyngier@arm.com> - 2017-05-25 14:30 +0200
        Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support Mason <slash.tmp@free.fr> - 2017-05-25 14:50 +0200
          Re: [PATCH v4 2/2] PCI: Add tango PCIe host bridge support Marc Zyngier <marc.zyngier@arm.com> - 2017-05-25 15:30 +0200

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

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 <marc_gonzalez@sigmadesigns.com>
> ---
>  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 000000000000..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 = <0x50000000 SZ_4M>, <0x2e000 0x100>;
> +		device_type = "pci";
> +		#size-cells = <2>;
> +		#address-cells = <3>;
> +		#interrupt-cells = <1>;
> +		ranges = <0x02000000 0x0 0x00400000  0x50400000  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	= 0xa0000000 + 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...

[toc] | [next] | [standalone]

#1650441

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 <marc_gonzalez@sigmadesigns.com>
>> ---
>>  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 000000000000..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	= 0xa0000000 + 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.

[toc] | [prev] | [next] | [standalone]

#1650446

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 <marc_gonzalez@sigmadesigns.com>
>>> ---
>>>  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 000000000000..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	= 0xa0000000 + 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...

[toc] | [prev] | [next] | [standalone]

#1650453

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.

[toc] | [prev] | [next] | [standalone]

#1650503

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...

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web