[RFC,v2,2/3] PCI: hisi: Make the HiSilicon PCIe host controller ECAM compliant

Commit Message

Gabriele Paoloni Feb. 8, 2016, 3:55 p.m. UTC

> -----Original Message-----
> From: Arnd Bergmann [mailto:[e-mail]]
> Sent: 08 February 2016 13:48
> To: [e-mail]
> Cc: Gabriele Paoloni; Guohanjun (Hanjun Guo); Wangzhou (B); liudongdong
> (C); Linuxarm; qiujiang; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; xuwei (O); [e-mail];
> [e-mail]; zhangjukuo; Liguozhu (Kenneth)
> Subject: Re: [RFC PATCH v2 2/3] PCI: hisi: Make the HiSilicon PCIe host
> controller ECAM compliant
> 
> On Monday 08 February 2016 12:41:03 Gabriele Paoloni wrote:
> > From: gabriele paoloni <[e-mail]>
> >
> > This patch modifies the current Hip05/Hip06 PCIe host controller
> > driver to make it ECAM compliant. This is needed in preparation
> > for the ACPI based driver to allow both DT and ACPI drivers to
> > use the same BIOS (that configure the Designware iATUs).
> >
> > Signed-off-by: Gabriele Paoloni <[e-mail]>
> > Signed-off-by: Dongdong Liu <[e-mail]>
> >
> 
> Doesn't this break backwards compatibility?

Well Hip05/Hip06 SoCs are used into evaluation boards
For the Estuary project.

https://github.com/hisilicon/estuary

As this new driver gets upstream we'll merge this new driver into estuary
and release a new version of the firmware to support it.

> 
> I think you need to use a new compatible string in the firmware
> if you change the register layout, and then change the driver
> to support both the old and the new layout.

You are right, for some reason in this patchset I missed the Documentation
update that I posted in the previous one, i.e.:

Thanks

Gab 


> 
> 	Arnd

Comments

Arnd Bergmann Feb. 8, 2016, 4:29 p.m. UTC | #1

On Monday 08 February 2016 15:55:35 Gabriele Paoloni wrote:
> > Doesn't this break backwards compatibility?
> 
> Well Hip05/Hip06 SoCs are used into evaluation boards
> For the Estuary project.
> 
> https://github.com/hisilicon/estuary
> 
> As this new driver gets upstream we'll merge this new driver into estuary
> and release a new version of the firmware to support it.

So what happens to folks running the old firmware then?

> > I think you need to use a new compatible string in the firmware
> > if you change the register layout, and then change the driver
> > to support both the old and the new layout.
> 
> You are right, for some reason in this patchset I missed the Documentation
> update that I posted in the previous one, i.e.:
> 
> --- a/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
> @@ -23,8 +23,8 @@ Optional properties:
>  Hip05 Example (note that Hip06 is the same except compatible):
>         pcie@0xb0080000 {
>                 compatible = "hisilicon,hip05-pcie", "snps,dw-pcie";
> -               reg = <0 0xb0080000 0 0x10000>, <0x220 0x00000000 0 0x2000>;
> -               reg-names = "rc_dbi", "config";
> +               reg = <0 0xb0080000 0 0x10000>, <0x220 0x00100000 0 0x0f00000>;
> +               reg-names = "rc_dbi", "ecam-cfg";
> 

That is not the compatible string, it's an undocumented register set.
You can either define the a new compatible string that gives the "config"
registers a new meaning, or you change the binding to allow two either
a "config" or an "ecam-cfg" register set, and let the driver handle both.

	Arnd

Gabriele Paoloni Feb. 8, 2016, 5:21 p.m. UTC | #2

Hi Arnd

> -----Original Message-----
> From: Arnd Bergmann [mailto:[e-mail]]
> Sent: 08 February 2016 16:30
> To: Gabriele Paoloni
> Cc: [e-mail]; Guohanjun (Hanjun Guo);
> Wangzhou (B); liudongdong (C); Linuxarm; qiujiang; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; xuwei (O); [e-mail];
> [e-mail]; zhangjukuo; Liguozhu (Kenneth)
> Subject: Re: [RFC PATCH v2 2/3] PCI: hisi: Make the HiSilicon PCIe host
> controller ECAM compliant
> 
> On Monday 08 February 2016 15:55:35 Gabriele Paoloni wrote:
> > > Doesn't this break backwards compatibility?
> >
> > Well Hip05/Hip06 SoCs are used into evaluation boards
> > For the Estuary project.
> >
> > https://github.com/hisilicon/estuary
> >
> > As this new driver gets upstream we'll merge this new driver into
> estuary
> > and release a new version of the firmware to support it.
> 
> So what happens to folks running the old firmware then?

So far we haven't released the PCIe nodes dtsi upstream and we think
we can handle the firmware upgrade for our current users either
by pointing them to the updated Estuary branch or by internal 
channels  

> 
> > > I think you need to use a new compatible string in the firmware
> > > if you change the register layout, and then change the driver
> > > to support both the old and the new layout.
> >
> > You are right, for some reason in this patchset I missed the
> Documentation
> > update that I posted in the previous one, i.e.:
> >
> > --- a/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
> > +++ b/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
> > @@ -23,8 +23,8 @@ Optional properties:
> >  Hip05 Example (note that Hip06 is the same except compatible):
> >         pcie@0xb0080000 {
> >                 compatible = "hisilicon,hip05-pcie", "snps,dw-pcie";
> > -               reg = <0 0xb0080000 0 0x10000>, <0x220 0x00000000 0
> 0x2000>;
> > -               reg-names = "rc_dbi", "config";
> > +               reg = <0 0xb0080000 0 0x10000>, <0x220 0x00100000 0
> 0x0f00000>;
> > +               reg-names = "rc_dbi", "ecam-cfg";
> >
> 
> That is not the compatible string, it's an undocumented register set.

Sorry, I misunderstood here, got it now :)

> You can either define the a new compatible string that gives the
> "config"
> registers a new meaning, or you change the binding to allow two either
> a "config" or an "ecam-cfg" register set, and let the driver handle
> both.

As per reply above I think that it would be quite easy for us to
let out current users update the BIOS so I don't think there is any
value in maintaining two version of the drivers one of which will
not be used.

What do you think?

Thanks

Gab

> 
> 	Arnd

Arnd Bergmann Feb. 9, 2016, 3:32 p.m. UTC | #3

On Monday 08 February 2016 17:21:27 Gabriele Paoloni wrote:
> Hi Arnd
> 
> > -----Original Message-----
> > From: Arnd Bergmann [mailto:[e-mail]]
> > Sent: 08 February 2016 16:30
> > To: Gabriele Paoloni
> > Cc: [e-mail]; Guohanjun (Hanjun Guo);
> > Wangzhou (B); liudongdong (C); Linuxarm; qiujiang; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; xuwei (O); [e-mail];
> > [e-mail]; zhangjukuo; Liguozhu (Kenneth)
> > Subject: Re: [RFC PATCH v2 2/3] PCI: hisi: Make the HiSilicon PCIe host
> > controller ECAM compliant
> > 
> > On Monday 08 February 2016 15:55:35 Gabriele Paoloni wrote:
> > > > Doesn't this break backwards compatibility?
> > >
> > > Well Hip05/Hip06 SoCs are used into evaluation boards
> > > For the Estuary project.
> > >
> > > https://github.com/hisilicon/estuary
> > >
> > > As this new driver gets upstream we'll merge this new driver into
> > estuary
> > > and release a new version of the firmware to support it.
> > 
> > So what happens to folks running the old firmware then?
> 
> So far we haven't released the PCIe nodes dtsi upstream and we think
> we can handle the firmware upgrade for our current users either
> by pointing them to the updated Estuary branch or by internal 
> channels  

The dts files in the kernel tree are not really that important,
the question is whether you are breaking things for real users.

If the upstream drivers work on some released firmware version,
they should keep working.

> > 
> > > > I think you need to use a new compatible string in the firmware
> > > > if you change the register layout, and then change the driver
> > > > to support both the old and the new layout.
> > >
> > > You are right, for some reason in this patchset I missed the
> > Documentation
> > > update that I posted in the previous one, i.e.:
> > >
> > > --- a/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
> > > +++ b/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
> > > @@ -23,8 +23,8 @@ Optional properties:
> > >  Hip05 Example (note that Hip06 is the same except compatible):
> > >         pcie@0xb0080000 {
> > >                 compatible = "hisilicon,hip05-pcie", "snps,dw-pcie";
> > > -               reg = <0 0xb0080000 0 0x10000>, <0x220 0x00000000 0
> > 0x2000>;
> > > -               reg-names = "rc_dbi", "config";
> > > +               reg = <0 0xb0080000 0 0x10000>, <0x220 0x00100000 0
> > 0x0f00000>;
> > > +               reg-names = "rc_dbi", "ecam-cfg";
> > >
> > 
> > That is not the compatible string, it's an undocumented register set.
> 
> Sorry, I misunderstood here, got it now :)
> 
> > You can either define the a new compatible string that gives the
> > "config"
> > registers a new meaning, or you change the binding to allow two either
> > a "config" or an "ecam-cfg" register set, and let the driver handle
> > both.
> 
> As per reply above I think that it would be quite easy for us to
> let out current users update the BIOS so I don't think there is any
> value in maintaining two version of the drivers one of which will
> not be used.
> 
> What do you think?

Requiring a BIOS update for a kernel update is really bad, even more
so when the same BIOS update breaks older kernels.

	Arnd

Gabriele Paoloni Feb. 9, 2016, 3:56 p.m. UTC | #4

> -----Original Message-----
> From: Arnd Bergmann [mailto:[e-mail]]
> Sent: 09 February 2016 15:32
> To: [e-mail]
> Cc: Gabriele Paoloni; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Linuxarm; xuwei (O); linux-
> [e-mail]; [e-mail]; Wangzhou (B);
> liudongdong (C); Guohanjun (Hanjun Guo); [e-mail];
> zhangjukuo; Liguozhu (Kenneth); qiujiang
> Subject: Re: [RFC PATCH v2 2/3] PCI: hisi: Make the HiSilicon PCIe host
> controller ECAM compliant
> 
> On Monday 08 February 2016 17:21:27 Gabriele Paoloni wrote:
> > Hi Arnd
> >
> > > -----Original Message-----
> > > From: Arnd Bergmann [mailto:[e-mail]]
> > > Sent: 08 February 2016 16:30
> > > To: Gabriele Paoloni
> > > Cc: [e-mail]; Guohanjun (Hanjun Guo);
> > > Wangzhou (B); liudongdong (C); Linuxarm; qiujiang;
> [e-mail];
> > > [e-mail]; [e-mail]; linux-
> [e-mail];
> > > [e-mail]; xuwei (O); [e-mail];
> > > [e-mail]; zhangjukuo; Liguozhu (Kenneth)
> > > Subject: Re: [RFC PATCH v2 2/3] PCI: hisi: Make the HiSilicon PCIe
> host
> > > controller ECAM compliant
> > >
> > > On Monday 08 February 2016 15:55:35 Gabriele Paoloni wrote:
> > > > > Doesn't this break backwards compatibility?
> > > >
> > > > Well Hip05/Hip06 SoCs are used into evaluation boards
> > > > For the Estuary project.
> > > >
> > > > https://github.com/hisilicon/estuary
> > > >
> > > > As this new driver gets upstream we'll merge this new driver into
> > > estuary
> > > > and release a new version of the firmware to support it.
> > >
> > > So what happens to folks running the old firmware then?
> >
> > So far we haven't released the PCIe nodes dtsi upstream and we think
> > we can handle the firmware upgrade for our current users either
> > by pointing them to the updated Estuary branch or by internal
> > channels
> 
> The dts files in the kernel tree are not really that important,
> the question is whether you are breaking things for real users.
> 
> If the upstream drivers work on some released firmware version,
> they should keep working.
> 
> > >
> > > > > I think you need to use a new compatible string in the firmware
> > > > > if you change the register layout, and then change the driver
> > > > > to support both the old and the new layout.
> > > >
> > > > You are right, for some reason in this patchset I missed the
> > > Documentation
> > > > update that I posted in the previous one, i.e.:
> > > >
> > > > --- a/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
> > > > +++ b/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
> > > > @@ -23,8 +23,8 @@ Optional properties:
> > > >  Hip05 Example (note that Hip06 is the same except compatible):
> > > >         pcie@0xb0080000 {
> > > >                 compatible = "hisilicon,hip05-pcie", "snps,dw-
> pcie";
> > > > -               reg = <0 0xb0080000 0 0x10000>, <0x220 0x00000000
> 0
> > > 0x2000>;
> > > > -               reg-names = "rc_dbi", "config";
> > > > +               reg = <0 0xb0080000 0 0x10000>, <0x220 0x00100000
> 0
> > > 0x0f00000>;
> > > > +               reg-names = "rc_dbi", "ecam-cfg";
> > > >
> > >
> > > That is not the compatible string, it's an undocumented register
> set.
> >
> > Sorry, I misunderstood here, got it now :)
> >
> > > You can either define the a new compatible string that gives the
> > > "config"
> > > registers a new meaning, or you change the binding to allow two
> either
> > > a "config" or an "ecam-cfg" register set, and let the driver handle
> > > both.
> >
> > As per reply above I think that it would be quite easy for us to
> > let out current users update the BIOS so I don't think there is any
> > value in maintaining two version of the drivers one of which will
> > not be used.
> >
> > What do you think?
> 
> Requiring a BIOS update for a kernel update is really bad, even more
> so when the same BIOS update breaks older kernels.

Well I see your point.

To be honest I won't expect many users of the old BIOS version since
as I said we plan to send out the BIOS update to pretty much every user,
however on the other side it is not too much effort for me to add code
to support both ECAM/non ECAM, 
so ok I'll fix this in v3,

Many Thanks

Gab

> 
> 	Arnd

Patch

--- a/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
@@ -23,8 +23,8 @@  Optional properties:
 Hip05 Example (note that Hip06 is the same except compatible):
 	pcie@0xb0080000 {
 		compatible = "hisilicon,hip05-pcie", "snps,dw-pcie";
-		reg = <0 0xb0080000 0 0x10000>, <0x220 0x00000000 0 0x2000>;
-		reg-names = "rc_dbi", "config";
+		reg = <0 0xb0080000 0 0x10000>, <0x220 0x00100000 0 0x0f00000>;
+		reg-names = "rc_dbi", "ecam-cfg";

I will fix this in v3