[PATCH v2 0/2] add the Amlogic Meson PCIe controller driver.

[PATCH v2 0/2] add the Amlogic Meson PCIe controller driver.

[Hanjie Lin]

The Amlogic Meson PCIe host controller is based on the Synopsys DesignWare
PCI core. This patchset add the driver and dt-bindings of the controller.

Changes since v1: [0]
 - use gpio lib instead open code
 - move 'apb' and 'port' reset from phy driver
 - format correcting

[0] : https://lkml.org/lkml/2018/8/14/70

Yue Wang (2):
  dt-bindings: PCI: meson: add DT bindings for Amlogic Meson PCIe
    controller
  PCI: meson: add the Amlogic Meson PCIe controller driver

 .../devicetree/bindings/pci/amlogic,meson-pcie.txt |  63 +++
 drivers/pci/controller/dwc/Kconfig                 |  12 +
 drivers/pci/controller/dwc/Makefile                |   1 +
 drivers/pci/controller/dwc/pci-meson.c             | 613 +++++++++++++++++++++
 4 files changed, 689 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
 create mode 100644 drivers/pci/controller/dwc/pci-meson.c

-- 
2.7.4

[PATCH v2 1/2] dt-bindings: PCI: meson: add DT bindings for Amlogic Meson PCIe controller

[Hanjie Lin]

From: Yue Wang <yue.wang@amlogic.com>

The Amlogic Meson PCIe host controller is based on the Synopsys DesignWare
PCI core. This patch adds documentation for the DT bindings in Meson PCIe
controller.

Signed-off-by: Yue Wang <yue.wang@amlogic.com>
Signed-off-by: Hanjie Lin <hanjie.lin@amlogic.com>
---
 .../devicetree/bindings/pci/amlogic,meson-pcie.txt | 63 ++++++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt

diff --git a/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
new file mode 100644
index 0000000..8a831d1
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
@@ -0,0 +1,63 @@
+Amlogic Meson AXG DWC PCIE SoC controller
+
+Amlogic Meson PCIe host controller is based on the Synopsys DesignWare PCI core.
+It shares common functions with the PCIe DesignWare core driver and
+inherits common properties defined in
+Documentation/devicetree/bindings/pci/designware-pci.txt.
+
+Additional properties are described here:
+
+Required properties:
+- compatible:
+	should contain "amlogic,axg-pcie" to identify the core.
+- reg:
+	Should contain the configuration address space.
+- reg-names: Must be
+	- "elbi"	External local bus interface registers
+	- "cfg"		Meson specific registers
+	- "config"	PCIe configuration space
+- reset-gpios: The GPIO to generate PCIe PERST# assert and deassert signal.
+- clocks: Must contain an entry for each entry in clock-names.
+- clock-names: Must include the following entries:
+	- "pclk"       PCIe GEN 100M PLL clock
+	- "port"       PCIe_x(A or B) RC clock gate
+	- "general"    PCIe Phy clock
+	- "mipi"       PCIe_x(A or B) 100M ref clock gate
+- resets: phandle to the reset lines.
+- reset-names: must contain "phy" and "peripheral"
+       - "port" Port A or B reset
+       - "apb" APB reset
+
+Example configuration:
+
+	pcie: pcie@f9800000 {
+			compatible = "amlogic,axg-pcie", "snps,dw-pcie";
+			reg = <0x0 0xf9800000 0x0 0x400000
+					0x0 0xff646000 0x0 0x2000
+					0x0 0xf9f00000 0x0 0x100000>;
+			reg-names = "elbi", "cfg", "config";
+			reset-gpios = <&gpio GPIOX_19 GPIO_ACTIVE_HIGH>;
+			interrupts = <GIC_SPI 177 IRQ_TYPE_EDGE_RISING>;
+			#interrupt-cells = <1>;
+			interrupt-map-mask = <0 0 0 0>;
+			interrupt-map = <0 0 0 0 &gic GIC_SPI 179 IRQ_TYPE_EDGE_RISING>;
+			bus-range = <0x0 0xff>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+			device_type = "pci";
+			phys = <&pcie_phy>;
+			ranges = <0x82000000 0 0 0x0 0xf9c00000 0 0x00300000>;
+
+			clocks = <&clkc CLKID_USB
+					&clkc CLKID_MIPI_ENABLE
+					&clkc CLKID_PCIE_A
+					&clkc CLKID_PCIE_CML_EN0>;
+			clock-names = "general",
+					"mipi",
+					"pclk",
+					"port";
+			resets = <&reset RESET_PCIE_A>,
+				<&reset RESET_PCIE_APB>;
+			reset-names = "port",
+					"apb";
+	};
-- 
2.7.4

Re: [PATCH v2 1/2] dt-bindings: PCI: meson: add DT bindings for Amlogic Meson PCIe controller

[Jerome Brunet]

On Fri, 2018-08-24 at 15:36 +0800, Hanjie Lin wrote:
> From: Yue Wang <yue.wang@amlogic.com>
> 
> The Amlogic Meson PCIe host controller is based on the Synopsys DesignWare
> PCI core. This patch adds documentation for the DT bindings in Meson PCIe
> controller.
> 
> Signed-off-by: Yue Wang <yue.wang@amlogic.com>
> Signed-off-by: Hanjie Lin <hanjie.lin@amlogic.com>
> ---
>  .../devicetree/bindings/pci/amlogic,meson-pcie.txt | 63 ++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
> 
> diff --git a/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
> new file mode 100644
> index 0000000..8a831d1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
> @@ -0,0 +1,63 @@
> +Amlogic Meson AXG DWC PCIE SoC controller
> +
> +Amlogic Meson PCIe host controller is based on the Synopsys DesignWare PCI core.
> +It shares common functions with the PCIe DesignWare core driver and
> +inherits common properties defined in
> +Documentation/devicetree/bindings/pci/designware-pci.txt.
> +
> +Additional properties are described here:
> +
> +Required properties:
> +- compatible:
> +	should contain "amlogic,axg-pcie" to identify the core.
> +- reg:
> +	Should contain the configuration address space.
> +- reg-names: Must be
> +	- "elbi"	External local bus interface registers
> +	- "cfg"		Meson specific registers
> +	- "config"	PCIe configuration space
> +- reset-gpios: The GPIO to generate PCIe PERST# assert and deassert signal.
> +- clocks: Must contain an entry for each entry in clock-names.
> +- clock-names: Must include the following entries:
> +	- "pclk"       PCIe GEN 100M PLL clock
> +	- "port"       PCIe_x(A or B) RC clock gate
> +	- "general"    PCIe Phy clock
> +	- "mipi"       PCIe_x(A or B) 100M ref clock gate
> +- resets: phandle to the reset lines.
> +- reset-names: must contain "phy" and "peripheral"
> +       - "port" Port A or B reset
> +       - "apb" APB reset

The above description is not coherent (phy <=> port)

> +
> +Example configuration:
> +
> +	pcie: pcie@f9800000 {
> +			compatible = "amlogic,axg-pcie", "snps,dw-pcie";
> +			reg = <0x0 0xf9800000 0x0 0x400000
> +					0x0 0xff646000 0x0 0x2000
> +					0x0 0xf9f00000 0x0 0x100000>;
> +			reg-names = "elbi", "cfg", "config";
> +			reset-gpios = <&gpio GPIOX_19 GPIO_ACTIVE_HIGH>;
> +			interrupts = <GIC_SPI 177 IRQ_TYPE_EDGE_RISING>;
> +			#interrupt-cells = <1>;
> +			interrupt-map-mask = <0 0 0 0>;
> +			interrupt-map = <0 0 0 0 &gic GIC_SPI 179 IRQ_TYPE_EDGE_RISING>;
> +			bus-range = <0x0 0xff>;
> +			#address-cells = <3>;
> +			#size-cells = <2>;
> +			device_type = "pci";

Not described above - is it even used ?

> +			phys = <&pcie_phy>;

Not documented and not necessary. Please remove this.

> +			ranges = <0x82000000 0 0 0x0 0xf9c00000 0 0x00300000>;
> +
> +			clocks = <&clkc CLKID_USB
> +					&clkc CLKID_MIPI_ENABLE
> +					&clkc CLKID_PCIE_A
> +					&clkc CLKID_PCIE_CML_EN0>;
> +			clock-names = "general",
> +					"mipi",
> +					"pclk",
> +					"port";
> +			resets = <&reset RESET_PCIE_A>,
> +				<&reset RESET_PCIE_APB>;
> +			reset-names = "port",
> +					"apb";
> +	};

Re: [PATCH v2 1/2] dt-bindings: PCI: meson: add DT bindings for Amlogic Meson PCIe controller

[Hanjie Lin]

On 2018/8/24 16:22, Jerome Brunet wrote:
> On Fri, 2018-08-24 at 15:36 +0800, Hanjie Lin wrote:
>> From: Yue Wang <yue.wang@amlogic.com>
>>
>> The Amlogic Meson PCIe host controller is based on the Synopsys DesignWare
>> PCI core. This patch adds documentation for the DT bindings in Meson PCIe
>> controller.
>>
>> Signed-off-by: Yue Wang <yue.wang@amlogic.com>
>> Signed-off-by: Hanjie Lin <hanjie.lin@amlogic.com>
>> ---
>>  .../devicetree/bindings/pci/amlogic,meson-pcie.txt | 63 ++++++++++++++++++++++
>>  1 file changed, 63 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
>>
>> diff --git a/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
>> new file mode 100644
>> index 0000000..8a831d1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
>> @@ -0,0 +1,63 @@
>> +Amlogic Meson AXG DWC PCIE SoC controller
>> +
>> +Amlogic Meson PCIe host controller is based on the Synopsys DesignWare PCI core.
>> +It shares common functions with the PCIe DesignWare core driver and
>> +inherits common properties defined in
>> +Documentation/devicetree/bindings/pci/designware-pci.txt.
>> +
>> +Additional properties are described here:
>> +
>> +Required properties:
>> +- compatible:
>> +	should contain "amlogic,axg-pcie" to identify the core.
>> +- reg:
>> +	Should contain the configuration address space.
>> +- reg-names: Must be
>> +	- "elbi"	External local bus interface registers
>> +	- "cfg"		Meson specific registers
>> +	- "config"	PCIe configuration space
>> +- reset-gpios: The GPIO to generate PCIe PERST# assert and deassert signal.
>> +- clocks: Must contain an entry for each entry in clock-names.
>> +- clock-names: Must include the following entries:
>> +	- "pclk"       PCIe GEN 100M PLL clock
>> +	- "port"       PCIe_x(A or B) RC clock gate
>> +	- "general"    PCIe Phy clock
>> +	- "mipi"       PCIe_x(A or B) 100M ref clock gate
>> +- resets: phandle to the reset lines.
>> +- reset-names: must contain "phy" and "peripheral"
>> +       - "port" Port A or B reset
>> +       - "apb" APB reset
> 
> The above description is not coherent (phy <=> port)
> 

Yes, this should be port and apb here.
We'll integrate phy driver into ctrl driver, and move phy reset to here also.

>> +
>> +Example configuration:
>> +
>> +	pcie: pcie@f9800000 {
>> +			compatible = "amlogic,axg-pcie", "snps,dw-pcie";
>> +			reg = <0x0 0xf9800000 0x0 0x400000
>> +					0x0 0xff646000 0x0 0x2000
>> +					0x0 0xf9f00000 0x0 0x100000>;
>> +			reg-names = "elbi", "cfg", "config";
>> +			reset-gpios = <&gpio GPIOX_19 GPIO_ACTIVE_HIGH>;
>> +			interrupts = <GIC_SPI 177 IRQ_TYPE_EDGE_RISING>;
>> +			#interrupt-cells = <1>;
>> +			interrupt-map-mask = <0 0 0 0>;
>> +			interrupt-map = <0 0 0 0 &gic GIC_SPI 179 IRQ_TYPE_EDGE_RISING>;
>> +			bus-range = <0x0 0xff>;
>> +			#address-cells = <3>;
>> +			#size-cells = <2>;
>> +			device_type = "pci";
> 
> Not described above - is it even used ?
> 

It's necessary, specified in designware-pcie.txt:
 - device_type:
  Usage: required
  Value type: <string>
  Definition: Should be "pci". 

>> +			phys = <&pcie_phy>;
> 
> Not documented and not necessary. Please remove this.
> 

We'll remove phy driver and this also.

>> +			ranges = <0x82000000 0 0 0x0 0xf9c00000 0 0x00300000>;
>> +
>> +			clocks = <&clkc CLKID_USB
>> +					&clkc CLKID_MIPI_ENABLE
>> +					&clkc CLKID_PCIE_A
>> +					&clkc CLKID_PCIE_CML_EN0>;
>> +			clock-names = "general",
>> +					"mipi",
>> +					"pclk",
>> +					"port";
>> +			resets = <&reset RESET_PCIE_A>,
>> +				<&reset RESET_PCIE_APB>;
>> +			reset-names = "port",
>> +					"apb";
>> +	};
> 
> 
> .
> 

Re: [PATCH v2 1/2] dt-bindings: PCI: meson: add DT bindings for Amlogic Meson PCIe controller

[Rob Herring]

On Mon, Aug 27, 2018 at 04:55:20PM +0800, Hanjie Lin wrote:
> 
> 
> On 2018/8/24 16:22, Jerome Brunet wrote:
> > On Fri, 2018-08-24 at 15:36 +0800, Hanjie Lin wrote:
> >> From: Yue Wang <yue.wang@amlogic.com>
> >>
> >> The Amlogic Meson PCIe host controller is based on the Synopsys DesignWare
> >> PCI core. This patch adds documentation for the DT bindings in Meson PCIe
> >> controller.
> >>
> >> Signed-off-by: Yue Wang <yue.wang@amlogic.com>
> >> Signed-off-by: Hanjie Lin <hanjie.lin@amlogic.com>
> >> ---
> >>  .../devicetree/bindings/pci/amlogic,meson-pcie.txt | 63 ++++++++++++++++++++++
> >>  1 file changed, 63 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
> >> new file mode 100644
> >> index 0000000..8a831d1
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
> >> @@ -0,0 +1,63 @@
> >> +Amlogic Meson AXG DWC PCIE SoC controller
> >> +
> >> +Amlogic Meson PCIe host controller is based on the Synopsys DesignWare PCI core.
> >> +It shares common functions with the PCIe DesignWare core driver and
> >> +inherits common properties defined in
> >> +Documentation/devicetree/bindings/pci/designware-pci.txt.
> >> +
> >> +Additional properties are described here:
> >> +
> >> +Required properties:
> >> +- compatible:
> >> +	should contain "amlogic,axg-pcie" to identify the core.
> >> +- reg:
> >> +	Should contain the configuration address space.
> >> +- reg-names: Must be
> >> +	- "elbi"	External local bus interface registers
> >> +	- "cfg"		Meson specific registers
> >> +	- "config"	PCIe configuration space
> >> +- reset-gpios: The GPIO to generate PCIe PERST# assert and deassert signal.
> >> +- clocks: Must contain an entry for each entry in clock-names.
> >> +- clock-names: Must include the following entries:
> >> +	- "pclk"       PCIe GEN 100M PLL clock
> >> +	- "port"       PCIe_x(A or B) RC clock gate
> >> +	- "general"    PCIe Phy clock
> >> +	- "mipi"       PCIe_x(A or B) 100M ref clock gate
> >> +- resets: phandle to the reset lines.
> >> +- reset-names: must contain "phy" and "peripheral"
> >> +       - "port" Port A or B reset
> >> +       - "apb" APB reset
> > 
> > The above description is not coherent (phy <=> port)
> > 
> 
> Yes, this should be port and apb here.
> We'll integrate phy driver into ctrl driver, and move phy reset to here also.

Why? That's the wrong thing to do if they are separate h/w blocks. You 
can do whatever you like in the drivers, but the DT should reflect the 
h/w.

Rob

Re: [PATCH v2 1/2] dt-bindings: PCI: meson: add DT bindings for Amlogic Meson PCIe controller

[Hanjie Lin]

On 2018/8/29 8:41, Rob Herring wrote:
> On Mon, Aug 27, 2018 at 04:55:20PM +0800, Hanjie Lin wrote:
>>
>>
>> On 2018/8/24 16:22, Jerome Brunet wrote:
>>> On Fri, 2018-08-24 at 15:36 +0800, Hanjie Lin wrote:
>>>> From: Yue Wang <yue.wang@amlogic.com>
>>>>
>>>> The Amlogic Meson PCIe host controller is based on the Synopsys DesignWare
>>>> PCI core. This patch adds documentation for the DT bindings in Meson PCIe
>>>> controller.
>>>>
>>>> Signed-off-by: Yue Wang <yue.wang@amlogic.com>
>>>> Signed-off-by: Hanjie Lin <hanjie.lin@amlogic.com>
>>>> ---
>>>>  .../devicetree/bindings/pci/amlogic,meson-pcie.txt | 63 ++++++++++++++++++++++
>>>>  1 file changed, 63 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
>>>> new file mode 100644
>>>> index 0000000..8a831d1
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
>>>> @@ -0,0 +1,63 @@
>>>> +Amlogic Meson AXG DWC PCIE SoC controller
>>>> +
>>>> +Amlogic Meson PCIe host controller is based on the Synopsys DesignWare PCI core.
>>>> +It shares common functions with the PCIe DesignWare core driver and
>>>> +inherits common properties defined in
>>>> +Documentation/devicetree/bindings/pci/designware-pci.txt.
>>>> +
>>>> +Additional properties are described here:
>>>> +
>>>> +Required properties:
>>>> +- compatible:
>>>> +	should contain "amlogic,axg-pcie" to identify the core.
>>>> +- reg:
>>>> +	Should contain the configuration address space.
>>>> +- reg-names: Must be
>>>> +	- "elbi"	External local bus interface registers
>>>> +	- "cfg"		Meson specific registers
>>>> +	- "config"	PCIe configuration space
>>>> +- reset-gpios: The GPIO to generate PCIe PERST# assert and deassert signal.
>>>> +- clocks: Must contain an entry for each entry in clock-names.
>>>> +- clock-names: Must include the following entries:
>>>> +	- "pclk"       PCIe GEN 100M PLL clock
>>>> +	- "port"       PCIe_x(A or B) RC clock gate
>>>> +	- "general"    PCIe Phy clock
>>>> +	- "mipi"       PCIe_x(A or B) 100M ref clock gate
>>>> +- resets: phandle to the reset lines.
>>>> +- reset-names: must contain "phy" and "peripheral"
>>>> +       - "port" Port A or B reset
>>>> +       - "apb" APB reset
>>>
>>> The above description is not coherent (phy <=> port)
>>>
>>
>> Yes, this should be port and apb here.
>> We'll integrate phy driver into ctrl driver, and move phy reset to here also.
> 
> Why? That's the wrong thing to do if they are separate h/w blocks. You 
> can do whatever you like in the drivers, but the DT should reflect the 
> h/w.
> 
> Rob
> 
> .
> 

We have the dedicated phy driver which only process reset job,
and we consider that it's too overkill to do just these things .
So we will integrate phy reset job into the controller driver int the next version.  

thanks. 

Re: [PATCH v2 1/2] dt-bindings: PCI: meson: add DT bindings for Amlogic Meson PCIe controller

[Jerome Brunet]

On Thu, 2018-08-30 at 15:37 +0800, Hanjie Lin wrote:
> 
> On 2018/8/29 8:41, Rob Herring wrote:
> > On Mon, Aug 27, 2018 at 04:55:20PM +0800, Hanjie Lin wrote:
> > > 
> > > 
> > > On 2018/8/24 16:22, Jerome Brunet wrote:
> > > > On Fri, 2018-08-24 at 15:36 +0800, Hanjie Lin wrote:
> > > > > From: Yue Wang <yue.wang@amlogic.com>
> > > > > 
> > > > > The Amlogic Meson PCIe host controller is based on the Synopsys DesignWare
> > > > > PCI core. This patch adds documentation for the DT bindings in Meson PCIe
> > > > > controller.
> > > > > 
> > > > > Signed-off-by: Yue Wang <yue.wang@amlogic.com>
> > > > > Signed-off-by: Hanjie Lin <hanjie.lin@amlogic.com>
> > > > > ---
> > > > >  .../devicetree/bindings/pci/amlogic,meson-pcie.txt | 63 ++++++++++++++++++++++
> > > > >  1 file changed, 63 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
> > > > > new file mode 100644
> > > > > index 0000000..8a831d1
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
> > > > > @@ -0,0 +1,63 @@
> > > > > +Amlogic Meson AXG DWC PCIE SoC controller
> > > > > +
> > > > > +Amlogic Meson PCIe host controller is based on the Synopsys DesignWare PCI core.
> > > > > +It shares common functions with the PCIe DesignWare core driver and
> > > > > +inherits common properties defined in
> > > > > +Documentation/devicetree/bindings/pci/designware-pci.txt.
> > > > > +
> > > > > +Additional properties are described here:
> > > > > +
> > > > > +Required properties:
> > > > > +- compatible:
> > > > > +	should contain "amlogic,axg-pcie" to identify the core.
> > > > > +- reg:
> > > > > +	Should contain the configuration address space.
> > > > > +- reg-names: Must be
> > > > > +	- "elbi"	External local bus interface registers
> > > > > +	- "cfg"		Meson specific registers
> > > > > +	- "config"	PCIe configuration space
> > > > > +- reset-gpios: The GPIO to generate PCIe PERST# assert and deassert signal.
> > > > > +- clocks: Must contain an entry for each entry in clock-names.
> > > > > +- clock-names: Must include the following entries:
> > > > > +	- "pclk"       PCIe GEN 100M PLL clock
> > > > > +	- "port"       PCIe_x(A or B) RC clock gate
> > > > > +	- "general"    PCIe Phy clock
> > > > > +	- "mipi"       PCIe_x(A or B) 100M ref clock gate
> > > > > +- resets: phandle to the reset lines.
> > > > > +- reset-names: must contain "phy" and "peripheral"
> > > > > +       - "port" Port A or B reset
> > > > > +       - "apb" APB reset
> > > > 
> > > > The above description is not coherent (phy <=> port)
> > > > 
> > > 
> > > Yes, this should be port and apb here.
> > > We'll integrate phy driver into ctrl driver, and move phy reset to here also.
> > 
> > Why? That's the wrong thing to do if they are separate h/w blocks. You 
> > can do whatever you like in the drivers, but the DT should reflect the 
> > h/w.
> > 
> > Rob
> > 
> > .
> > 
> 
> We have the dedicated phy driver which only process reset job,
> and we consider that it's too overkill to do just these things .
> So we will integrate phy reset job into the controller driver int the next version.

Rob has a point there. Even if overkill, it does model the HW as it is.
+ I spotted in your v2 that there is also a register access, so not only the
reset

> 
> thanks. 

Re: [PATCH v2 1/2] dt-bindings: PCI: meson: add DT bindings for Amlogic Meson PCIe controller

[Rob Herring]

On Thu, Aug 30, 2018 at 2:37 AM Hanjie Lin <hanjie.lin@amlogic.com> wrote:
>
>
>
> On 2018/8/29 8:41, Rob Herring wrote:
> > On Mon, Aug 27, 2018 at 04:55:20PM +0800, Hanjie Lin wrote:
> >>
> >>
> >> On 2018/8/24 16:22, Jerome Brunet wrote:
> >>> On Fri, 2018-08-24 at 15:36 +0800, Hanjie Lin wrote:
> >>>> From: Yue Wang <yue.wang@amlogic.com>
> >>>>
> >>>> The Amlogic Meson PCIe host controller is based on the Synopsys DesignWare
> >>>> PCI core. This patch adds documentation for the DT bindings in Meson PCIe
> >>>> controller.
> >>>>
> >>>> Signed-off-by: Yue Wang <yue.wang@amlogic.com>
> >>>> Signed-off-by: Hanjie Lin <hanjie.lin@amlogic.com>
> >>>> ---
> >>>>  .../devicetree/bindings/pci/amlogic,meson-pcie.txt | 63 ++++++++++++++++++++++
> >>>>  1 file changed, 63 insertions(+)
> >>>>  create mode 100644 Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
> >>>> new file mode 100644
> >>>> index 0000000..8a831d1
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
> >>>> @@ -0,0 +1,63 @@
> >>>> +Amlogic Meson AXG DWC PCIE SoC controller
> >>>> +
> >>>> +Amlogic Meson PCIe host controller is based on the Synopsys DesignWare PCI core.
> >>>> +It shares common functions with the PCIe DesignWare core driver and
> >>>> +inherits common properties defined in
> >>>> +Documentation/devicetree/bindings/pci/designware-pci.txt.
> >>>> +
> >>>> +Additional properties are described here:
> >>>> +
> >>>> +Required properties:
> >>>> +- compatible:
> >>>> +  should contain "amlogic,axg-pcie" to identify the core.
> >>>> +- reg:
> >>>> +  Should contain the configuration address space.
> >>>> +- reg-names: Must be
> >>>> +  - "elbi"        External local bus interface registers
> >>>> +  - "cfg"         Meson specific registers
> >>>> +  - "config"      PCIe configuration space
> >>>> +- reset-gpios: The GPIO to generate PCIe PERST# assert and deassert signal.
> >>>> +- clocks: Must contain an entry for each entry in clock-names.
> >>>> +- clock-names: Must include the following entries:
> >>>> +  - "pclk"       PCIe GEN 100M PLL clock
> >>>> +  - "port"       PCIe_x(A or B) RC clock gate
> >>>> +  - "general"    PCIe Phy clock
> >>>> +  - "mipi"       PCIe_x(A or B) 100M ref clock gate
> >>>> +- resets: phandle to the reset lines.
> >>>> +- reset-names: must contain "phy" and "peripheral"
> >>>> +       - "port" Port A or B reset
> >>>> +       - "apb" APB reset
> >>>
> >>> The above description is not coherent (phy <=> port)
> >>>
> >>
> >> Yes, this should be port and apb here.
> >> We'll integrate phy driver into ctrl driver, and move phy reset to here also.
> >
> > Why? That's the wrong thing to do if they are separate h/w blocks. You
> > can do whatever you like in the drivers, but the DT should reflect the
> > h/w.
> >
> > Rob
> >
> > .
> >
>
> We have the dedicated phy driver which only process reset job,
> and we consider that it's too overkill to do just these things .
> So we will integrate phy reset job into the controller driver int the next version.

What's in the separate register space you had for the phy?

Rob

Re: [PATCH v2 1/2] dt-bindings: PCI: meson: add DT bindings for Amlogic Meson PCIe controller

[Hanjie Lin]

On 2018/8/30 21:59, Rob Herring wrote:
> On Thu, Aug 30, 2018 at 2:37 AM Hanjie Lin <hanjie.lin@amlogic.com> wrote:
>>
>>
>>
>> On 2018/8/29 8:41, Rob Herring wrote:
>>> On Mon, Aug 27, 2018 at 04:55:20PM +0800, Hanjie Lin wrote:
>>>>
>>>>
>>>> On 2018/8/24 16:22, Jerome Brunet wrote:
>>>>> On Fri, 2018-08-24 at 15:36 +0800, Hanjie Lin wrote:
>>>>>> From: Yue Wang <yue.wang@amlogic.com>
>>>>>>
>>>>>> The Amlogic Meson PCIe host controller is based on the Synopsys DesignWare
>>>>>> PCI core. This patch adds documentation for the DT bindings in Meson PCIe
>>>>>> controller.
>>>>>>
>>>>>> Signed-off-by: Yue Wang <yue.wang@amlogic.com>
>>>>>> Signed-off-by: Hanjie Lin <hanjie.lin@amlogic.com>
>>>>>> ---
>>>>>>  .../devicetree/bindings/pci/amlogic,meson-pcie.txt | 63 ++++++++++++++++++++++
>>>>>>  1 file changed, 63 insertions(+)
>>>>>>  create mode 100644 Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..8a831d1
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
>>>>>> @@ -0,0 +1,63 @@
>>>>>> +Amlogic Meson AXG DWC PCIE SoC controller
>>>>>> +
>>>>>> +Amlogic Meson PCIe host controller is based on the Synopsys DesignWare PCI core.
>>>>>> +It shares common functions with the PCIe DesignWare core driver and
>>>>>> +inherits common properties defined in
>>>>>> +Documentation/devicetree/bindings/pci/designware-pci.txt.
>>>>>> +
>>>>>> +Additional properties are described here:
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible:
>>>>>> +  should contain "amlogic,axg-pcie" to identify the core.
>>>>>> +- reg:
>>>>>> +  Should contain the configuration address space.
>>>>>> +- reg-names: Must be
>>>>>> +  - "elbi"        External local bus interface registers
>>>>>> +  - "cfg"         Meson specific registers
>>>>>> +  - "config"      PCIe configuration space
>>>>>> +- reset-gpios: The GPIO to generate PCIe PERST# assert and deassert signal.
>>>>>> +- clocks: Must contain an entry for each entry in clock-names.
>>>>>> +- clock-names: Must include the following entries:
>>>>>> +  - "pclk"       PCIe GEN 100M PLL clock
>>>>>> +  - "port"       PCIe_x(A or B) RC clock gate
>>>>>> +  - "general"    PCIe Phy clock
>>>>>> +  - "mipi"       PCIe_x(A or B) 100M ref clock gate
>>>>>> +- resets: phandle to the reset lines.
>>>>>> +- reset-names: must contain "phy" and "peripheral"
>>>>>> +       - "port" Port A or B reset
>>>>>> +       - "apb" APB reset
>>>>>
>>>>> The above description is not coherent (phy <=> port)
>>>>>
>>>>
>>>> Yes, this should be port and apb here.
>>>> We'll integrate phy driver into ctrl driver, and move phy reset to here also.
>>>
>>> Why? That's the wrong thing to do if they are separate h/w blocks. You
>>> can do whatever you like in the drivers, but the DT should reflect the
>>> h/w.
>>>
>>> Rob
>>>
>>> .
>>>
>>
>> We have the dedicated phy driver which only process reset job,
>> and we consider that it's too overkill to do just these things .
>> So we will integrate phy reset job into the controller driver int the next version.
> 
> What's in the separate register space you had for the phy?
> 
> Rob
> 
> .
> 

As described with 'phy' reg in ctrl patch v3 thread [0]

- reg-names: Must be
	- "elbi"	External local bus interface registers
	- "cfg"		Meson specific registers
	- "phy"		Meson PCIE PHY registers
	- "config"	PCIe configuration space

When each controller driver probe, we powerup phy by write phy register like below: 
writel(MESON_PCIE_PHY_POWERUP, mp->mem_res.phy_base);

[0] https://lkml.kernel.org/r/1535616829-167936-1-git-send-email-hanjie.lin@amlogic.com