[PATCH] arm64: dts: marvell: mcbin: Enable PCIe interface

[PATCH] arm64: dts: marvell: mcbin: Enable PCIe interface

[Gregory CLEMENT]

Enable the PCIe interface on the MACCHIATOBin board. It is located on
CON12 and is 4 lanes capable.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
index 4968e731de61..e445c80213ab 100644
--- a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
+++ b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
@@ -136,6 +136,12 @@
 	vqmmc-supply = <&v_3_3>;
 };
 
+&cpm_pcie0 {
+	/* CON 12 */
+	num-lanes = <4>;
+	status = "okay";
+};
+
 &cpm_usb3_0 {
 	/* J38? - USB2.0 only */
 	status = "okay";
-- 
2.13.2

[PATCH] arm64: dts: marvell: mcbin: Enable PCIe interface

[Russell King - ARM Linux]

On Wed, Jul 05, 2017 at 06:13:33PM +0200, Gregory CLEMENT wrote:
> Enable the PCIe interface on the MACCHIATOBin board. It is located on
> CON12 and is 4 lanes capable.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Why do you folk at free-electrons like doing half a job all the friggin
time?

You know I have complete patches for mcbin, but you pointedly won't look
at them at all - except when you have a problem and want to test my tree.
And even then, you ignore my work (despite testing that it works), and
you still recreate my patches.

This is really frustrating and insane behaviour on your part.

Here's what I have:

+&cpm_pcie0 {
+       pinctrl-names = "default";
+       pinctrl-0 = <&cpm_pcie_pins>;
+       num-lanes = <4>;
+       reset-gpio = <&cpm_gpio1 20 GPIO_ACTIVE_LOW>;
+       status = "okay";
+};
+

+       cpm_pcie_pins: pcie-pins {
+               marvell,pins = "mpp52";
+               marvell,function = "gpio";
+       };

Since you have merged GPIO and pinmux support for the v4.13 window,
there's absolutely no reason not to include the GPIO bits.  In fact,
there's no reason not to consider using my bloody patches.

Except your stupid idiotic NiH problem that you seem to have.

I know that your behaviour in regard of this has been discussed within
Marvell, and people are getting unhappy with free electron's attitude
over this.  You need to change, and start working _with_ people instead
of constantly screwing people over.

So, NAK on your patch.

Once v4.13-rc1 is out, I'll update my patch series for the screw-over
free-electrons has already done, and post some patches.  I can't do it
sooner, your work is scattered all over the place which makes it
impossible to build upon, and afaics you've not published a
consolidated tree.

Maybe the whole plan here is to "screw rmk" - that's exactly the message
that I'm getting from you guys.  You've no interest in working with
others, you seem to want to out-right own everything Marvell and sod
everyone else.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] arm64: dts: marvell: mcbin: Enable PCIe interface

[Ard Biesheuvel]

On 5 July 2017 at 18:16, Russell King - ARM Linux <linux@armlinux.org.uk> wrote:
> On Wed, Jul 05, 2017 at 06:13:33PM +0200, Gregory CLEMENT wrote:
>> Enable the PCIe interface on the MACCHIATOBin board. It is located on
>> CON12 and is 4 lanes capable.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>
> Why do you folk at free-electrons like doing half a job all the friggin
> time?
>
> You know I have complete patches for mcbin, but you pointedly won't look
> at them at all - except when you have a problem and want to test my tree.
> And even then, you ignore my work (despite testing that it works), and
> you still recreate my patches.
>
> This is really frustrating and insane behaviour on your part.
>
> Here's what I have:
>
> +&cpm_pcie0 {
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&cpm_pcie_pins>;
> +       num-lanes = <4>;
> +       reset-gpio = <&cpm_gpio1 20 GPIO_ACTIVE_LOW>;
> +       status = "okay";

This needs 'num-viewport = <8>' as well, or the crazy Synopsys DWC
PCIe driver will happily reconfigure the I/O window at runtime to
perform config space accesses, without any locking whatsoever.

[PATCH] arm64: dts: marvell: mcbin: Enable PCIe interface

[Russell King - ARM Linux]

On Wed, Jul 05, 2017 at 06:36:53PM +0100, Ard Biesheuvel wrote:
> On 5 July 2017 at 18:16, Russell King - ARM Linux <linux@armlinux.org.uk> wrote:
> > On Wed, Jul 05, 2017 at 06:13:33PM +0200, Gregory CLEMENT wrote:
> >> Enable the PCIe interface on the MACCHIATOBin board. It is located on
> >> CON12 and is 4 lanes capable.
> >>
> >> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> >
> > Why do you folk at free-electrons like doing half a job all the friggin
> > time?
> >
> > You know I have complete patches for mcbin, but you pointedly won't look
> > at them at all - except when you have a problem and want to test my tree.
> > And even then, you ignore my work (despite testing that it works), and
> > you still recreate my patches.
> >
> > This is really frustrating and insane behaviour on your part.
> >
> > Here's what I have:
> >
> > +&cpm_pcie0 {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&cpm_pcie_pins>;
> > +       num-lanes = <4>;
> > +       reset-gpio = <&cpm_gpio1 20 GPIO_ACTIVE_LOW>;
> > +       status = "okay";
> 
> This needs 'num-viewport = <8>' as well, or the crazy Synopsys DWC
> PCIe driver will happily reconfigure the I/O window at runtime to
> perform config space accesses, without any locking whatsoever.

Thanks for the feedback, I'll integrate that change into my patch, so
it's ready for when I rebase after -rc1.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] arm64: dts: marvell: mcbin: Enable PCIe interface

[Jisheng Zhang]

On Wed, 5 Jul 2017 18:44:03 +0100 Russell King - ARM Linux wrote:

> On Wed, Jul 05, 2017 at 06:36:53PM +0100, Ard Biesheuvel wrote:
> > On 5 July 2017 at 18:16, Russell King - ARM Linux <linux@armlinux.org.uk> wrote:  
> > > On Wed, Jul 05, 2017 at 06:13:33PM +0200, Gregory CLEMENT wrote:  
> > >> Enable the PCIe interface on the MACCHIATOBin board. It is located on
> > >> CON12 and is 4 lanes capable.
> > >>
> > >> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>  
> > >
> > > Why do you folk at free-electrons like doing half a job all the friggin
> > > time?
> > >
> > > You know I have complete patches for mcbin, but you pointedly won't look
> > > at them at all - except when you have a problem and want to test my tree.
> > > And even then, you ignore my work (despite testing that it works), and
> > > you still recreate my patches.
> > >
> > > This is really frustrating and insane behaviour on your part.
> > >
> > > Here's what I have:
> > >
> > > +&cpm_pcie0 {
> > > +       pinctrl-names = "default";
> > > +       pinctrl-0 = <&cpm_pcie_pins>;
> > > +       num-lanes = <4>;
> > > +       reset-gpio = <&cpm_gpio1 20 GPIO_ACTIVE_LOW>;
> > > +       status = "okay";  
> > 
> > This needs 'num-viewport = <8>' as well, or the crazy Synopsys DWC

IMHO, maybe putting this property into dtsi is better.

Thanks,
Jisheng

> > PCIe driver will happily reconfigure the I/O window at runtime to
> > perform config space accesses, without any locking whatsoever.  
> 
> Thanks for the feedback, I'll integrate that change into my patch, so
> it's ready for when I rebase after -rc1.
> 

[PATCH] arm64: dts: marvell: mcbin: Enable PCIe interface

[Gregory CLEMENT]

Hi Russell King,
 
 On mer., juil. 05 2017, Russell King - ARM Linux <linux@armlinux.org.uk> wrote:

> On Wed, Jul 05, 2017 at 06:13:33PM +0200, Gregory CLEMENT wrote:
>> Enable the PCIe interface on the MACCHIATOBin board. It is located on
>> CON12 and is 4 lanes capable.
>> 
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>
> Why do you folk at free-electrons like doing half a job all the friggin
> time?
>
> You know I have complete patches for mcbin, but you pointedly won't look
> at them at all - except when you have a problem and want to test my tree.
> And even then, you ignore my work (despite testing that it works), and
> you still recreate my patches.
>
> This is really frustrating and insane behaviour on your part.

Sorry for this, I wrongly assumed that if you had the PCIe part you
would have already submitted it. Also I didn't expect there was a gpio
dependency with PCIe because until now I never saw the reset-gpio for
PCIe with the controller I use.

>
> Here's what I have:
>
> +&cpm_pcie0 {
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&cpm_pcie_pins>;
> +       num-lanes = <4>;
> +       reset-gpio = <&cpm_gpio1 20 GPIO_ACTIVE_LOW>;
> +       status = "okay";
> +};
> +
>
> +       cpm_pcie_pins: pcie-pins {
> +               marvell,pins = "mpp52";
> +               marvell,function = "gpio";
> +       };
>
> Since you have merged GPIO and pinmux support for the v4.13 window,
> there's absolutely no reason not to include the GPIO bits.  In fact,
> there's no reason not to consider using my bloody patches.
>
> Except your stupid idiotic NiH problem that you seem to have.
>
> I know that your behaviour in regard of this has been discussed within
> Marvell, and people are getting unhappy with free electron's attitude
> over this.  You need to change, and start working _with_ people instead
> of constantly screwing people over.
>
> So, NAK on your patch.
>
> Once v4.13-rc1 is out, I'll update my patch series for the screw-over
> free-electrons has already done, and post some patches.  I can't do it
> sooner, your work is scattered all over the place which makes it

Actually everything is available in the linux-next branch since one or
two weeks. So you can rebase and send your series right now.

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH] arm64: dts: marvell: mcbin: Enable PCIe interface

[Ard Biesheuvel]

On 6 July 2017 at 07:31, Jisheng Zhang <jszhang@marvell.com> wrote:
> On Wed, 5 Jul 2017 18:44:03 +0100 Russell King - ARM Linux wrote:
>
>> On Wed, Jul 05, 2017 at 06:36:53PM +0100, Ard Biesheuvel wrote:
>> > On 5 July 2017 at 18:16, Russell King - ARM Linux <linux@armlinux.org.uk> wrote:
>> > > On Wed, Jul 05, 2017 at 06:13:33PM +0200, Gregory CLEMENT wrote:
>> > >> Enable the PCIe interface on the MACCHIATOBin board. It is located on
>> > >> CON12 and is 4 lanes capable.
>> > >>
>> > >> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> > >
>> > > Why do you folk at free-electrons like doing half a job all the friggin
>> > > time?
>> > >
>> > > You know I have complete patches for mcbin, but you pointedly won't look
>> > > at them at all - except when you have a problem and want to test my tree.
>> > > And even then, you ignore my work (despite testing that it works), and
>> > > you still recreate my patches.
>> > >
>> > > This is really frustrating and insane behaviour on your part.
>> > >
>> > > Here's what I have:
>> > >
>> > > +&cpm_pcie0 {
>> > > +       pinctrl-names = "default";
>> > > +       pinctrl-0 = <&cpm_pcie_pins>;
>> > > +       num-lanes = <4>;
>> > > +       reset-gpio = <&cpm_gpio1 20 GPIO_ACTIVE_LOW>;
>> > > +       status = "okay";
>> >
>> > This needs 'num-viewport = <8>' as well, or the crazy Synopsys DWC
>
> IMHO, maybe putting this property into dtsi is better.
>

Good point. Do all instances of this IP that live on the SoC have 8 viewports?

[PATCH] arm64: dts: marvell: mcbin: Enable PCIe interface

[Russell King - ARM Linux]

On Thu, Jul 06, 2017 at 09:18:25AM +0200, Gregory CLEMENT wrote:
> Hi Russell King,
>
> Actually everything is available in the linux-next branch since one or
> two weeks. So you can rebase and send your series right now.

As there are multiple trees, just specifying a branch is not helpful.
The Xenon SDHCI and ethernet changes were being published through the
MISL tree, but that tree has no for-next branch.

There is no git tree specified in MAINTAINERS either.

So, sorry, I've no idea which tree you're referring to.

And no, I do _not_ want to grab a copy of linux-next to rebase my
changes on and publish back to SolidRun for their use, that would be
insane.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] arm64: dts: marvell: mcbin: Enable PCIe interface

[Gregory CLEMENT]

Hi Russell King,
 
 On jeu., juil. 06 2017, Russell King - ARM Linux <linux@armlinux.org.uk> wrote:

> On Thu, Jul 06, 2017 at 09:18:25AM +0200, Gregory CLEMENT wrote:
>> Hi Russell King,
>>
>> Actually everything is available in the linux-next branch since one or
>> two weeks. So you can rebase and send your series right now.
>
> As there are multiple trees, just specifying a branch is not helpful.
> The Xenon SDHCI and ethernet changes were being published through the
> MISL tree, but that tree has no for-next branch.
>
> There is no git tree specified in MAINTAINERS either.
>
> So, sorry, I've no idea which tree you're referring to.
>
> And no, I do _not_ want to grab a copy of linux-next to rebase my

Well for upstreaming I don't see the problem to base on a linux-net tags
such as next-20170704. We do it often when we submit patches to various
subsystem (and if it is not a next tag we merge various branch by
ourselves).

> changes on and publish back to SolidRun for their use, that would be
> insane.

We also deliver stable branch for various customers and in this case we
just have a different branch or tree.

However, I think we have exactly what meets your requirements:
https://github.com/MISL-EBU-System-SW/mainline-public/tree/4.12-rc6/backports

It is a stable branch which gathers most of the patch submitted for
Armada 37xx, 7K and 8K.

Gregory

>
> -- 
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH] arm64: dts: marvell: mcbin: Enable PCIe interface

[Gregory CLEMENT]

Hi Ard,
 
 On jeu., juil. 06 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> On 6 July 2017 at 07:31, Jisheng Zhang <jszhang@marvell.com> wrote:
>> On Wed, 5 Jul 2017 18:44:03 +0100 Russell King - ARM Linux wrote:
>>
>>> On Wed, Jul 05, 2017 at 06:36:53PM +0100, Ard Biesheuvel wrote:
>>> > On 5 July 2017 at 18:16, Russell King - ARM Linux <linux@armlinux.org.uk> wrote:
>>> > > On Wed, Jul 05, 2017 at 06:13:33PM +0200, Gregory CLEMENT wrote:
>>> > >> Enable the PCIe interface on the MACCHIATOBin board. It is located on
>>> > >> CON12 and is 4 lanes capable.
>>> > >>
>>> > >> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>>> > >
>>> > > Why do you folk at free-electrons like doing half a job all the friggin
>>> > > time?
>>> > >
>>> > > You know I have complete patches for mcbin, but you pointedly won't look
>>> > > at them at all - except when you have a problem and want to test my tree.
>>> > > And even then, you ignore my work (despite testing that it works), and
>>> > > you still recreate my patches.
>>> > >
>>> > > This is really frustrating and insane behaviour on your part.
>>> > >
>>> > > Here's what I have:
>>> > >
>>> > > +&cpm_pcie0 {
>>> > > +       pinctrl-names = "default";
>>> > > +       pinctrl-0 = <&cpm_pcie_pins>;
>>> > > +       num-lanes = <4>;
>>> > > +       reset-gpio = <&cpm_gpio1 20 GPIO_ACTIVE_LOW>;
>>> > > +       status = "okay";
>>> >
>>> > This needs 'num-viewport = <8>' as well, or the crazy Synopsys DWC
>>
>> IMHO, maybe putting this property into dtsi is better.
>>
>
> Good point. Do all instances of this IP that live on the SoC have 8
> viewports?

In the Armada 80x0 datasheet there is no mention of viewport. However 2
days ago you said that "What I do know is that boards like the Marvell
8040 based MacchiatoBin uses this IP in RC mode, and exposes config,
MMIO and IO space windows using only 2 viewports. Note that this is
essentially a bug in the DT description, given that its version of the
IP supports 8 viewports."[1]

So you seem to know that the version of the IP used support the 8
viewports!

What I can also say is that the in the datasheet there is no mention of
difference between the instance. The only thing that can differ is the
number of lane for each PCIe Port.

Gregory


[1]: https://www.spinics.net/lists/arm-kernel/msg592091.html

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH] arm64: dts: marvell: mcbin: Enable PCIe interface

[Ard Biesheuvel]

On 6 July 2017 at 13:10, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
> Hi Ard,
>
>  On jeu., juil. 06 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> On 6 July 2017 at 07:31, Jisheng Zhang <jszhang@marvell.com> wrote:
>>> On Wed, 5 Jul 2017 18:44:03 +0100 Russell King - ARM Linux wrote:
>>>
>>>> On Wed, Jul 05, 2017 at 06:36:53PM +0100, Ard Biesheuvel wrote:
>>>> > On 5 July 2017 at 18:16, Russell King - ARM Linux <linux@armlinux.org.uk> wrote:
>>>> > > On Wed, Jul 05, 2017 at 06:13:33PM +0200, Gregory CLEMENT wrote:
>>>> > >> Enable the PCIe interface on the MACCHIATOBin board. It is located on
>>>> > >> CON12 and is 4 lanes capable.
>>>> > >>
>>>> > >> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>>>> > >
>>>> > > Why do you folk at free-electrons like doing half a job all the friggin
>>>> > > time?
>>>> > >
>>>> > > You know I have complete patches for mcbin, but you pointedly won't look
>>>> > > at them at all - except when you have a problem and want to test my tree.
>>>> > > And even then, you ignore my work (despite testing that it works), and
>>>> > > you still recreate my patches.
>>>> > >
>>>> > > This is really frustrating and insane behaviour on your part.
>>>> > >
>>>> > > Here's what I have:
>>>> > >
>>>> > > +&cpm_pcie0 {
>>>> > > +       pinctrl-names = "default";
>>>> > > +       pinctrl-0 = <&cpm_pcie_pins>;
>>>> > > +       num-lanes = <4>;
>>>> > > +       reset-gpio = <&cpm_gpio1 20 GPIO_ACTIVE_LOW>;
>>>> > > +       status = "okay";
>>>> >
>>>> > This needs 'num-viewport = <8>' as well, or the crazy Synopsys DWC
>>>
>>> IMHO, maybe putting this property into dtsi is better.
>>>
>>
>> Good point. Do all instances of this IP that live on the SoC have 8
>> viewports?
>
> In the Armada 80x0 datasheet there is no mention of viewport. However 2
> days ago you said that "What I do know is that boards like the Marvell
> 8040 based MacchiatoBin uses this IP in RC mode, and exposes config,
> MMIO and IO space windows using only 2 viewports. Note that this is
> essentially a bug in the DT description, given that its version of the
> IP supports 8 viewports."[1]
>
> So you seem to know that the version of the IP used support the 8
> viewports!
>
> What I can also say is that the in the datasheet there is no mention of
> difference between the instance. The only thing that can differ is the
> number of lane for each PCIe Port.
>

I am sure I found it in the datasheet somewhere. It is the number of
ATU windows.

In any case, I implemented ACPI firmware for the MacchiatoBin, which
also involves programming the PCIe RC in a mode that is compatible
with ECAM. This turns out to be impossible (which means you cannot
implement a SBSA compatible platform with this SoC), but in attempting
to do so, I used five different windows, for type 0 config cycles,
type 1 config cycles, IO space, MMIO32 space and MMIO64 space (which
the Synopsys DW PCIe controller does not even implement,
unfortunately)

Note that this also involved reprogramming all the memory translation
windows from the secure firmware, to free up the region 0xc000_0000 -
0xefff_ffff for use by PCIe (config and MMIO32 space), and assigning a
window 0x8_0000_0000 - 0x8_ffff_ffff for 64-bit MMIO BARs.

[PATCH] arm64: dts: marvell: mcbin: Enable PCIe interface

[Gregory CLEMENT]

Hi Ard,
 
 On jeu., juil. 06 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> On 6 July 2017 at 13:10, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>> Hi Ard,
>>
>>  On jeu., juil. 06 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>
>>> On 6 July 2017 at 07:31, Jisheng Zhang <jszhang@marvell.com> wrote:
>>>> On Wed, 5 Jul 2017 18:44:03 +0100 Russell King - ARM Linux wrote:
>>>>
>>>>> On Wed, Jul 05, 2017 at 06:36:53PM +0100, Ard Biesheuvel wrote:
>>>>> > On 5 July 2017 at 18:16, Russell King - ARM Linux <linux@armlinux.org.uk> wrote:
>>>>> > > On Wed, Jul 05, 2017 at 06:13:33PM +0200, Gregory CLEMENT wrote:
>>>>> > >> Enable the PCIe interface on the MACCHIATOBin board. It is located on
>>>>> > >> CON12 and is 4 lanes capable.
>>>>> > >>
>>>>> > >> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>>>>> > >
>>>>> > > Why do you folk at free-electrons like doing half a job all the friggin
>>>>> > > time?
>>>>> > >
>>>>> > > You know I have complete patches for mcbin, but you pointedly won't look
>>>>> > > at them at all - except when you have a problem and want to test my tree.
>>>>> > > And even then, you ignore my work (despite testing that it works), and
>>>>> > > you still recreate my patches.
>>>>> > >
>>>>> > > This is really frustrating and insane behaviour on your part.
>>>>> > >
>>>>> > > Here's what I have:
>>>>> > >
>>>>> > > +&cpm_pcie0 {
>>>>> > > +       pinctrl-names = "default";
>>>>> > > +       pinctrl-0 = <&cpm_pcie_pins>;
>>>>> > > +       num-lanes = <4>;
>>>>> > > +       reset-gpio = <&cpm_gpio1 20 GPIO_ACTIVE_LOW>;
>>>>> > > +       status = "okay";
>>>>> >
>>>>> > This needs 'num-viewport = <8>' as well, or the crazy Synopsys DWC
>>>>
>>>> IMHO, maybe putting this property into dtsi is better.
>>>>
>>>
>>> Good point. Do all instances of this IP that live on the SoC have 8
>>> viewports?
>>
>> In the Armada 80x0 datasheet there is no mention of viewport. However 2
>> days ago you said that "What I do know is that boards like the Marvell
>> 8040 based MacchiatoBin uses this IP in RC mode, and exposes config,
>> MMIO and IO space windows using only 2 viewports. Note that this is
>> essentially a bug in the DT description, given that its version of the
>> IP supports 8 viewports."[1]
>>
>> So you seem to know that the version of the IP used support the 8
>> viewports!
>>
>> What I can also say is that the in the datasheet there is no mention of
>> difference between the instance. The only thing that can differ is the
>> number of lane for each PCIe Port.
>>
>
> I am sure I found it in the datasheet somewhere. It is the number of
> ATU windows.

Under the "Outbound iATU Features" and "Inbound iATU Features" I have
the following point:
"Up to 8 address regions programmable for location and size."

Is that what you think about ?

>
> In any case, I implemented ACPI firmware for the MacchiatoBin, which
> also involves programming the PCIe RC in a mode that is compatible
> with ECAM. This turns out to be impossible (which means you cannot
> implement a SBSA compatible platform with this SoC), but in attempting
> to do so, I used five different windows, for type 0 config cycles,
> type 1 config cycles, IO space, MMIO32 space and MMIO64 space (which
> the Synopsys DW PCIe controller does not even implement,
> unfortunately)
>
> Note that this also involved reprogramming all the memory translation
> windows from the secure firmware, to free up the region 0xc000_0000 -
> 0xefff_ffff for use by PCIe (config and MMIO32 space), and assigning a
> window 0x8_0000_0000 - 0x8_ffff_ffff for 64-bit MMIO BARs.

Currently for the driver the only important information is having a
num-viewport greater than 2. So given all this we are OK if we add the
'num-viewport = <8>' in all the PCIe port node in the dtsi file.

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH] arm64: dts: marvell: mcbin: Enable PCIe interface

[Ard Biesheuvel]

On 6 July 2017 at 13:55, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
> Hi Ard,
>
>  On jeu., juil. 06 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> On 6 July 2017 at 13:10, Gregory CLEMENT
>> <gregory.clement@free-electrons.com> wrote:
>>> Hi Ard,
>>>
>>>  On jeu., juil. 06 2017, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>
>>>> On 6 July 2017 at 07:31, Jisheng Zhang <jszhang@marvell.com> wrote:
>>>>> On Wed, 5 Jul 2017 18:44:03 +0100 Russell King - ARM Linux wrote:
>>>>>
>>>>>> On Wed, Jul 05, 2017 at 06:36:53PM +0100, Ard Biesheuvel wrote:
>>>>>> > On 5 July 2017 at 18:16, Russell King - ARM Linux <linux@armlinux.org.uk> wrote:
>>>>>> > > On Wed, Jul 05, 2017 at 06:13:33PM +0200, Gregory CLEMENT wrote:
>>>>>> > >> Enable the PCIe interface on the MACCHIATOBin board. It is located on
>>>>>> > >> CON12 and is 4 lanes capable.
>>>>>> > >>
>>>>>> > >> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>>>>>> > >
>>>>>> > > Why do you folk at free-electrons like doing half a job all the friggin
>>>>>> > > time?
>>>>>> > >
>>>>>> > > You know I have complete patches for mcbin, but you pointedly won't look
>>>>>> > > at them at all - except when you have a problem and want to test my tree.
>>>>>> > > And even then, you ignore my work (despite testing that it works), and
>>>>>> > > you still recreate my patches.
>>>>>> > >
>>>>>> > > This is really frustrating and insane behaviour on your part.
>>>>>> > >
>>>>>> > > Here's what I have:
>>>>>> > >
>>>>>> > > +&cpm_pcie0 {
>>>>>> > > +       pinctrl-names = "default";
>>>>>> > > +       pinctrl-0 = <&cpm_pcie_pins>;
>>>>>> > > +       num-lanes = <4>;
>>>>>> > > +       reset-gpio = <&cpm_gpio1 20 GPIO_ACTIVE_LOW>;
>>>>>> > > +       status = "okay";
>>>>>> >
>>>>>> > This needs 'num-viewport = <8>' as well, or the crazy Synopsys DWC
>>>>>
>>>>> IMHO, maybe putting this property into dtsi is better.
>>>>>
>>>>
>>>> Good point. Do all instances of this IP that live on the SoC have 8
>>>> viewports?
>>>
>>> In the Armada 80x0 datasheet there is no mention of viewport. However 2
>>> days ago you said that "What I do know is that boards like the Marvell
>>> 8040 based MacchiatoBin uses this IP in RC mode, and exposes config,
>>> MMIO and IO space windows using only 2 viewports. Note that this is
>>> essentially a bug in the DT description, given that its version of the
>>> IP supports 8 viewports."[1]
>>>
>>> So you seem to know that the version of the IP used support the 8
>>> viewports!
>>>
>>> What I can also say is that the in the datasheet there is no mention of
>>> difference between the instance. The only thing that can differ is the
>>> number of lane for each PCIe Port.
>>>
>>
>> I am sure I found it in the datasheet somewhere. It is the number of
>> ATU windows.
>
> Under the "Outbound iATU Features" and "Inbound iATU Features" I have
> the following point:
> "Up to 8 address regions programmable for location and size."
>
> Is that what you think about ?
>

Yes.

>>
>> In any case, I implemented ACPI firmware for the MacchiatoBin, which
>> also involves programming the PCIe RC in a mode that is compatible
>> with ECAM. This turns out to be impossible (which means you cannot
>> implement a SBSA compatible platform with this SoC), but in attempting
>> to do so, I used five different windows, for type 0 config cycles,
>> type 1 config cycles, IO space, MMIO32 space and MMIO64 space (which
>> the Synopsys DW PCIe controller does not even implement,
>> unfortunately)
>>
>> Note that this also involved reprogramming all the memory translation
>> windows from the secure firmware, to free up the region 0xc000_0000 -
>> 0xefff_ffff for use by PCIe (config and MMIO32 space), and assigning a
>> window 0x8_0000_0000 - 0x8_ffff_ffff for 64-bit MMIO BARs.
>
> Currently for the driver the only important information is having a
> num-viewport greater than 2. So given all this we are OK if we add the
> 'num-viewport = <8>' in all the PCIe port node in the dtsi file.
>

OK. If the x4 and the x1 versions of the description in the manual
both contain the sentence about 8 the address regions, then I think it
is safe to include it for all 6 instances of this IP.