Re: [PATCH v3 1/2] dt-bindings: PCI: intel: Add YAML schemas for the PCIe RC controller
On 9/6/2019 4:31 AM, Martin Blumenstingl wrote:
Hi Dilip,
On Wed, Sep 4, 2019 at 12:11 PM Dilip Kota wrote:
[...]
+properties:
+ compatible:
+const: intel,lgm-pcie
should we add the "snps,dw-pcie" here (and in the example below) as well?
(this is what for example
Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt does)
Thanks for pointing out this. We should add this.
[...]
+ phy-names:
+const: pciephy
the most popular choice in Documentation/devicetree/bindings/pci/ is "pcie-phy"
if Rob is happy with "pciephy" (which is already part of two other
bindings) then I'm happy with "pciephy" as well
Agree.
+ num-lanes:
+description: Number of lanes to use for this port.
are there SoCs with more than 2 lanes?
you can list the allowed values in an enum so "num-lanes = <16>"
causes an error when someone accidentally has this in their .dts (and
runs the dt-bindings validation)
Our SoC(LGM) supports single lane or dual lane. Again this also depends
on the board. I wonder if we should put this into board specific dts.
To make multiple lanes work properly, it also depends on the phy mode.
In my internal version, I put it into board dts.
[...]
+ reset-assert-ms:
maybe add:
$ref: /schemas/types.yaml#/definitions/uint32
Agree
+description: |
+ Device reset interval in ms.
+ Some devices need an interval upto 500ms. By default it is 100ms.
+
+required:
+ - compatible
+ - device_type
+ - reg
+ - reg-names
+ - ranges
+ - resets
+ - clocks
+ - phys
+ - phy-names
+ - reset-gpios
+ - num-lanes
+ - linux,pci-domain
+ - interrupts
+ - interrupt-map
+ - interrupt-map-mask
+
+additionalProperties: false
+
+examples:
+ - |
+pcie10:pcie@d0e0 {
+ compatible = "intel,lgm-pcie";
+ device_type = "pci";
+ #address-cells = <3>;
+ #size-cells = <2>;
+ reg = <
+0xd0e0 0x1000
+0xd200 0x80
+0xd0a41000 0x1000
+>;
+ reg-names = "dbi", "config", "app";
+ linux,pci-domain = <0>;
+ max-link-speed = <4>;
+ bus-range = <0x00 0x08>;
+ interrupt-parent = <>;
+ interrupts = <67 1>;
+ #interrupt-cells = <1>;
+ interrupt-map-mask = <0 0 0 0x7>;
+ interrupt-map = <0 0 0 1 27 1>,
+ <0 0 0 2 28 1>,
+ <0 0 0 3 29 1>,
+ <0 0 0 4 30 1>;
is the "1" in the interrupts and interrupt-map properties IRQ_TYPE_EDGE_RISING?
you can use these macros in this example as well, see
Documentation/devicetree/bindings/iio/accel/adi,adxl372.yaml for
example
No. 1 here means index from arch/x86/devicetree.c
static struct of_ioapic_type of_ioapic_type[] =
{
{
.out_type = IRQ_TYPE_EDGE_RISING,
.trigger = IOAPIC_EDGE,
.polarity = 1,
},
{
.out_type = IRQ_TYPE_LEVEL_LOW,
.trigger = IOAPIC_LEVEL,
.polarity = 0,
},
{
.out_type = IRQ_TYPE_LEVEL_HIGH,
.trigger = IOAPIC_LEVEL,
.polarity = 1,
},
{
.out_type = IRQ_TYPE_EDGE_FALLING,
.trigger = IOAPIC_EDGE,
.polarity = 0,
},
};
static int dt_irqdomain_alloc(struct irq_domain *domain, unsigned int virq,
unsigned int nr_irqs, void *arg)
{
struct irq_fwspec *fwspec = (struct irq_fwspec *)arg;
struct of_ioapic_type *it;
struct irq_alloc_info tmp;
int type_index;
if (WARN_ON(fwspec->param_count < 2))
return -EINVAL;
type_index = fwspec->param[1]; // index.
if (type_index >= ARRAY_SIZE(of_ioapic_type))
return -EINVAL;
I would not see this definition is user-friendly. But it is how x86
handles at the moment.
Martin
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin, On 9/3/2019 6:04 AM, Martin Blumenstingl wrote: Hi, On Mon, Sep 2, 2019 at 11:45 AM Chuan Hua, Lei wrote: Hi Martin, On 9/2/2019 5:38 AM, Martin Blumenstingl wrote: Hi, On Fri, Aug 30, 2019 at 5:02 AM Chuan Hua, Lei wrote: Hi Martin, On 8/30/2019 5:40 AM, Martin Blumenstingl wrote: Hi, On Thu, Aug 29, 2019 at 4:51 AM Chuan Hua, Lei wrote: I'm not surprised that we got some of the IP block layout for the VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW (BSP). with proper documentation (as in a "public datasheet for the SoC") it would be easy to spot these mistakes (at least I assume that the quality of the Infineon / Lantiq datasheets is excellent). back to reset-intel-syscon: assigning only one job to the RCU hardware is a good idea (in my opinion). that brings up a question: why do we need the "syscon" compatible for the RCU node? this is typically used when registers are accessed by another IP block and the other driver has to access these registers as well. does this mean that there's more hidden in the RCU registers? As I mentioned, some other misc registers are put into RCU even they don't belong to reset functions. OK, just be aware that there are also rules for syscon compatible drivers, see for example: [0] if Rob (dt-bindings maintainer) is happy with the documentation in patch 1 then I'm fine with it as well. for my own education I would appreciate if you could describe these "other misc registers" with a few sentences (I assume that this can also help Rob) For LGM, RCU is clean. There would be no MISC register after software's feedback. These misc registers will be moved to chiptop/misc groups(implemented by syscon). For legacy SoC, we do have a lot MISC registers for different SoCs. OK, I think I understand now: chiptop != RCU so RCU really only has one purpose: handling resets while chiptop manages all the random bits does this means we don't need RCU to match "syscon"? If we don't support legacy SoC with the same driver, we don't need syscon, just regmap. Regmap is a must for us since we will use regmap proxy to implement secure rest via secure processor. I think we should drop the syscon compatible for LGM then even for the legacy SoCs the reset controller should not have a syscon compatible: instead it should have a syscon parent (as the current "lantiq,xrx200-reset" binding requires and as suggested by Rob for another IP block: [0]) I am not sure if syscon parent really matches hardware implementation. In all our Networking SoCs, chiptop is kind of misc register collection. Some registers can't belong to any particular group, or they need to work together with other modules(therefore, these misc registers would be accessed by two or more modules). However, chiptop is not a hardware module. keeping regmap is great in my opinion because it's a nice API and gets rid of some boilerplate even better if it makes things easier for accessing the secure processor [...] 4. Code not optimized and intel internal review not assessed. insights from you (like the issue with the reset callback) are very valuable - this shows that we should focus on having one driver. Based on the above findings, I would suggest reset-lantiq.c to move to reset-intel-syscon.c my concern with having two separate drivers is that it will be hard to migrate from reset-lantiq to the "optimized" reset-intel-syscon driver. I don't have access to the datasheets for the any Lantiq/Intel SoC (VRX200 and even older). so debugging issues after switching from one driver to another is tedious because I cannot tell which part of the driver is causing a problem (it's either "all code from driver A" vs "all code from driver B", meaning it's hard to narrow it down). with separate commits/patches that are improving the reset-lantiq driver I can do git bisect to find the cause of a problem on the older SoCs (VRX200 for example) Our internal version supports XRX350/XRX500/PRX300(MIPS based) and latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c should be straight forward. what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do they only use level resets (_assert and _deassert) or are some reset lines using reset pulses (_reset)? when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c we still had to add support for the _reset callback as this is missing in reset-intel-syscon.c currently Yes. We have reset pulse(assert, then check the reset status). only now I realized that the reset-intel-syscon driver does not seem to use the status registers (instead it's looking at the reset registers when checking the status). what happened to the status registers - do they still exist in newer SoCs (like LGM)? why are they not used? Reset status check is there. regmap_read_poll_timeout to check status big. Status register offset &
Re: [PATCH v3 2/2] dwc: PCI: intel: Intel PCIe RC controller driver
Hi Dilip, On 9/4/2019 6:10 PM, Dilip Kota wrote: Add support to PCIe RC controller on Intel Universal Gateway SoC. PCIe controller is based of Synopsys Designware pci core. Signed-off-by: Dilip Kota --- changes on v3: Rename PCIe app logic registers with PCIE_APP prefix. PCIE_IOP_CTRL configuration is not required. Remove respective code. Remove wrapper functions for clk enable/disable APIs. Use platform_get_resource_byname() instead of devm_platform_ioremap_resource() to be similar with DWC framework. Rename phy name to "pciephy". Modify debug message in msi_init() callback to be more specific. Remove map_irq() callback. Enable the INTx interrupts at the time of PCIe initialization. Reduce memory fragmentation by using variable "struct dw_pcie pci" instead of allocating memory. Reduce the delay to 100us during enpoint initialization intel_pcie_ep_rst_init(). Call dw_pcie_host_deinit() during remove() instead of directly calling PCIe core APIs. Rename "intel,rst-interval" to "reset-assert-ms". Remove unused APP logic Interrupt bit macro definitions. Use dwc framework's dw_pcie_setup_rc() for PCIe host specific configuration instead of redefining the same functionality in the driver. Move the whole DT parsing specific code to intel_pcie_get_resources() drivers/pci/controller/dwc/Kconfig | 13 + drivers/pci/controller/dwc/Makefile | 1 + drivers/pci/controller/dwc/pcie-intel-axi.c | 840 3 files changed, 854 insertions(+) create mode 100644 drivers/pci/controller/dwc/pcie-intel-axi.c diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig index 6ea778ae4877..e44b9b6a6390 100644 --- a/drivers/pci/controller/dwc/Kconfig +++ b/drivers/pci/controller/dwc/Kconfig @@ -82,6 +82,19 @@ config PCIE_DW_PLAT_EP order to enable device-specific features PCI_DW_PLAT_EP must be selected. +config PCIE_INTEL_AXI +bool "Intel AHB/AXI PCIe host controller support" +depends on PCI_MSI +depends on PCI +depends on OF +select PCIE_DW_HOST +help + Say 'Y' here to enable support for Intel AHB/AXI PCIe Host + controller driver. + The Intel PCIe controller is based on the Synopsys Designware + pcie core and therefore uses the Designware core functions to + implement the driver. + config PCI_EXYNOS bool "Samsung Exynos PCIe controller" depends on SOC_EXYNOS5440 || COMPILE_TEST diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile index b085dfd4fab7..46e656ebdf90 100644 --- a/drivers/pci/controller/dwc/Makefile +++ b/drivers/pci/controller/dwc/Makefile @@ -3,6 +3,7 @@ obj-$(CONFIG_PCIE_DW) += pcie-designware.o obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o +obj-$(CONFIG_PCIE_INTEL_AXI) += pcie-intel-axi.o obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o obj-$(CONFIG_PCI_IMX6) += pci-imx6.o diff --git a/drivers/pci/controller/dwc/pcie-intel-axi.c b/drivers/pci/controller/dwc/pcie-intel-axi.c new file mode 100644 index ..75607ce03ebf --- /dev/null +++ b/drivers/pci/controller/dwc/pcie-intel-axi.c @@ -0,0 +1,840 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * PCIe host controller driver for Intel AXI PCIe Bridge + * + * Copyright (c) 2019 Intel Corporation. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "../../pci.h" +#include "pcie-designware.h" + +#define PCIE_CCRID 0x8 + +#define PCIE_LCAP 0x7C +#define PCIE_LCAP_MAX_LINK_SPEED GENMASK(3, 0) +#define PCIE_LCAP_MAX_LENGTH_WIDTH GENMASK(9, 4) + +/* Link Control and Status Register */ +#define PCIE_LCTLSTS 0x80 +#define PCIE_LCTLSTS_ASPM_ENABLE GENMASK(1, 0) +#define PCIE_LCTLSTS_RCB128BIT(3) +#define PCIE_LCTLSTS_LINK_DISABLE BIT(4) +#define PCIE_LCTLSTS_COM_CLK_CFG BIT(6) +#define PCIE_LCTLSTS_HW_AW_DIS BIT(9) +#define PCIE_LCTLSTS_LINK_SPEEDGENMASK(19, 16) +#define PCIE_LCTLSTS_NEGOTIATED_LINK_WIDTH GENMASK(25, 20) +#define PCIE_LCTLSTS_SLOT_CLK_CFG BIT(28) + +#define PCIE_LCTLSTS2 0xA0 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED GENMASK(3, 0) +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_25GT 0x1 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_5GT 0x2 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_8GT 0x3 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_16GT 0x4
Re: [PATCH v3 1/2] dt-bindings: PCI: intel: Add YAML schemas for the PCIe RC controller
Hi Dilip,
On 9/4/2019 6:10 PM, Dilip Kota wrote:
The Intel PCIe RC controller is Synopsys Designware
based PCIe core. Add YAML schemas for PCIe in RC mode
present in Intel Universal Gateway soc.
Signed-off-by: Dilip Kota
---
changes on v3:
Add the appropriate License-Identifier
Rename intel,rst-interval to 'reset-assert-us'
rst->interval to reset-assert-ms(should be typo error)
Add additionalProperties: false
Rename phy-names to 'pciephy'
Remove the dtsi node split of SoC and board in the example
Add #interrupt-cells = <1>; or else interrupt parsing will fail
Name yaml file with compatible name
.../devicetree/bindings/pci/intel,lgm-pcie.yaml| 137 +
1 file changed, 137 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml
diff --git a/Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml
b/Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml
new file mode 100644
index ..5e5cc7fd66cd
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml
@@ -0,0 +1,137 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/intel-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intel AXI bus based PCI express root complex
+
+maintainers:
+ - Dilip Kota
+
+properties:
+ compatible:
+const: intel,lgm-pcie
+
+ device_type:
+const: pci
+
+ "#address-cells":
+const: 3
+
+ "#size-cells":
+const: 2
+
+ reg:
+items:
+ - description: Controller control and status registers.
+ - description: PCIe configuration registers.
+ - description: Controller application registers.
+
+ reg-names:
+items:
+ - const: dbi
+ - const: config
+ - const: app
+
+ ranges:
+description: Ranges for the PCI memory and I/O regions.
+
+ resets:
+maxItems: 1
+
+ clocks:
+description: PCIe registers interface clock.
+
+ phys:
+maxItems: 1
+
+ phy-names:
+const: pciephy
+
+ reset-gpios:
+maxItems: 1
+
+ num-lanes:
+description: Number of lanes to use for this port.
+
+ linux,pci-domain:
+$ref: /schemas/types.yaml#/definitions/uint32
+description: PCI domain ID.
+
+ interrupts:
+description: PCIe core integrated miscellaneous interrupt.
+
+ '#interrupt-cells':
+const: 1
+
+ interrupt-map-mask:
+description: Standard PCI IRQ mapping properties.
+
+ interrupt-map:
+description: Standard PCI IRQ mapping properties.
+
+ max-link-speed:
+description: Specify PCI Gen for link capability.
+
+ bus-range:
+description: Range of bus numbers associated with this controller.
+
+ reset-assert-ms:
+description: |
+ Device reset interval in ms.
+ Some devices need an interval upto 500ms. By default it is 100ms.
+
+required:
+ - compatible
+ - device_type
+ - reg
+ - reg-names
+ - ranges
+ - resets
+ - clocks
+ - phys
+ - phy-names
+ - reset-gpios
+ - num-lanes
+ - linux,pci-domain
+ - interrupts
+ - interrupt-map
+ - interrupt-map-mask
+
+additionalProperties: false
+
+examples:
+ - |
+pcie10:pcie@d0e0 {
+ compatible = "intel,lgm-pcie";
+ device_type = "pci";
+ #address-cells = <3>;
+ #size-cells = <2>;
+ reg = <
+0xd0e0 0x1000
+0xd200 0x80
+0xd0a41000 0x1000
+>;
+ reg-names = "dbi", "config", "app";
+ linux,pci-domain = <0>;
+ max-link-speed = <4>;
+ bus-range = <0x00 0x08>;
+ interrupt-parent = <>;
+ interrupts = <67 1>;
+ #interrupt-cells = <1>;
+ interrupt-map-mask = <0 0 0 0x7>;
+ interrupt-map = <0 0 0 1 27 1>,
+ <0 0 0 2 28 1>,
+ <0 0 0 3 29 1>,
+ <0 0 0 4 30 1>;
+ ranges = <0x0200 0 0xd400 0xd400 0 0x0400>;
+ resets = < 0x50 0>;
+ clocks = < LGM_GCLK_PCIE10>;
+ phys = <>;
+ phy-names = "pciephy";
+ status = "okay";
+ reset-assert-ms = <500>;
+ reset-gpios = < 3 GPIO_ACTIVE_LOW>;
+ num-lanes = <2>;
+};
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin,
On 9/2/2019 5:38 AM, Martin Blumenstingl wrote:
Hi,
On Fri, Aug 30, 2019 at 5:02 AM Chuan Hua, Lei
wrote:
Hi Martin,
On 8/30/2019 5:40 AM, Martin Blumenstingl wrote:
Hi,
On Thu, Aug 29, 2019 at 4:51 AM Chuan Hua, Lei
wrote:
I'm not surprised that we got some of the IP block layout for the
VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW
(BSP).
with proper documentation (as in a "public datasheet for the SoC") it
would be easy to spot these mistakes (at least I assume that the
quality of the Infineon / Lantiq datasheets is excellent).
back to reset-intel-syscon:
assigning only one job to the RCU hardware is a good idea (in my opinion).
that brings up a question: why do we need the "syscon" compatible for
the RCU node?
this is typically used when registers are accessed by another IP block
and the other driver has to access these registers as well. does this
mean that there's more hidden in the RCU registers?
As I mentioned, some other misc registers are put into RCU even they
don't belong to reset functions.
OK, just be aware that there are also rules for syscon compatible
drivers, see for example: [0]
if Rob (dt-bindings maintainer) is happy with the documentation in
patch 1 then I'm fine with it as well.
for my own education I would appreciate if you could describe these
"other misc registers" with a few sentences (I assume that this can
also help Rob)
For LGM, RCU is clean. There would be no MISC register after software's
feedback. These misc registers will be moved to chiptop/misc
groups(implemented by syscon). For legacy SoC, we do have a lot MISC
registers for different SoCs.
OK, I think I understand now: chiptop != RCU
so RCU really only has one purpose: handling resets
while chiptop manages all the random bits
does this means we don't need RCU to match "syscon"?
If we don't support legacy SoC with the same driver, we don't need
syscon, just regmap. Regmap is a must for us since we will use regmap
proxy to implement secure rest via secure processor.
[...]
4. Code not optimized and intel internal review not assessed.
insights from you (like the issue with the reset callback) are very
valuable - this shows that we should focus on having one driver.
Based on the above findings, I would suggest reset-lantiq.c to move to
reset-intel-syscon.c
my concern with having two separate drivers is that it will be hard to
migrate from reset-lantiq to the "optimized" reset-intel-syscon
driver.
I don't have access to the datasheets for the any Lantiq/Intel SoC
(VRX200 and even older).
so debugging issues after switching from one driver to another is
tedious because I cannot tell which part of the driver is causing a
problem (it's either "all code from driver A" vs "all code from driver
B", meaning it's hard to narrow it down).
with separate commits/patches that are improving the reset-lantiq
driver I can do git bisect to find the cause of a problem on the older
SoCs (VRX200 for example)
Our internal version supports XRX350/XRX500/PRX300(MIPS based) and
latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c
should be straight forward.
what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do
they only use level resets (_assert and _deassert) or are some reset
lines using reset pulses (_reset)?
when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c
we still had to add support for the _reset callback as this is missing
in reset-intel-syscon.c currently
Yes. We have reset pulse(assert, then check the reset status).
only now I realized that the reset-intel-syscon driver does not seem
to use the status registers (instead it's looking at the reset
registers when checking the status).
what happened to the status registers - do they still exist in newer
SoCs (like LGM)? why are they not used?
Reset status check is there. regmap_read_poll_timeout to check status
big. Status register offset <4) from request register. For legacy, there
is one exception, we can add soc specific data to handle it.
I see, thank you for the explanation
this won't work on VRX200 for example because the status register is
not always at (reset register - 0x4)
As I mentioned, VRX200 and all legacy SoCs (MIPS based) can be solved
with one soc data in the compatible array.
For example(not same as upstream, but idea is similar)
static u32 intel_stat_reg_off(struct intel_reset_data *data, u32 req_off)
{
if (data->soc_data->legacy && req_off == RCU_RST_REQ)
return RCU_RST_STAT;
else
return req_off + 0x4;
}
on VRX200 for example there seem to be some cases where the bits in
the reset and status registers are different (for example: the first
GPHY seems to use reset bit 31 but status bit 30)
this is currently not supported in reset-intel-syscon
This is most tricky and ugly part for VRX200/Danube. Do you h
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin,
On 8/30/2019 5:40 AM, Martin Blumenstingl wrote:
Hi,
On Thu, Aug 29, 2019 at 4:51 AM Chuan Hua, Lei
wrote:
I'm not surprised that we got some of the IP block layout for the
VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW
(BSP).
with proper documentation (as in a "public datasheet for the SoC") it
would be easy to spot these mistakes (at least I assume that the
quality of the Infineon / Lantiq datasheets is excellent).
back to reset-intel-syscon:
assigning only one job to the RCU hardware is a good idea (in my opinion).
that brings up a question: why do we need the "syscon" compatible for
the RCU node?
this is typically used when registers are accessed by another IP block
and the other driver has to access these registers as well. does this
mean that there's more hidden in the RCU registers?
As I mentioned, some other misc registers are put into RCU even they
don't belong to reset functions.
OK, just be aware that there are also rules for syscon compatible
drivers, see for example: [0]
if Rob (dt-bindings maintainer) is happy with the documentation in
patch 1 then I'm fine with it as well.
for my own education I would appreciate if you could describe these
"other misc registers" with a few sentences (I assume that this can
also help Rob)
For LGM, RCU is clean. There would be no MISC register after software's
feedback. These misc registers will be moved to chiptop/misc
groups(implemented by syscon). For legacy SoC, we do have a lot MISC
registers for different SoCs.
[...]
4. Code not optimized and intel internal review not assessed.
insights from you (like the issue with the reset callback) are very
valuable - this shows that we should focus on having one driver.
Based on the above findings, I would suggest reset-lantiq.c to move to
reset-intel-syscon.c
my concern with having two separate drivers is that it will be hard to
migrate from reset-lantiq to the "optimized" reset-intel-syscon
driver.
I don't have access to the datasheets for the any Lantiq/Intel SoC
(VRX200 and even older).
so debugging issues after switching from one driver to another is
tedious because I cannot tell which part of the driver is causing a
problem (it's either "all code from driver A" vs "all code from driver
B", meaning it's hard to narrow it down).
with separate commits/patches that are improving the reset-lantiq
driver I can do git bisect to find the cause of a problem on the older
SoCs (VRX200 for example)
Our internal version supports XRX350/XRX500/PRX300(MIPS based) and
latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c
should be straight forward.
what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - do
they only use level resets (_assert and _deassert) or are some reset
lines using reset pulses (_reset)?
when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c
we still had to add support for the _reset callback as this is missing
in reset-intel-syscon.c currently
Yes. We have reset pulse(assert, then check the reset status).
only now I realized that the reset-intel-syscon driver does not seem
to use the status registers (instead it's looking at the reset
registers when checking the status).
what happened to the status registers - do they still exist in newer
SoCs (like LGM)? why are they not used?
Reset status check is there. regmap_read_poll_timeout to check status
big. Status register offset <4) from request register. For legacy, there
is one exception, we can add soc specific data to handle it.
I see, thank you for the explanation
this won't work on VRX200 for example because the status register is
not always at (reset register - 0x4)
As I mentioned, VRX200 and all legacy SoCs (MIPS based) can be solved
with one soc data in the compatible array.
For example(not same as upstream, but idea is similar)
static u32 intel_stat_reg_off(struct intel_reset_data *data, u32 req_off)
{
if (data->soc_data->legacy && req_off == RCU_RST_REQ)
return RCU_RST_STAT;
else
return req_off + 0x4;
}
on VRX200 for example there seem to be some cases where the bits in
the reset and status registers are different (for example: the first
GPHY seems to use reset bit 31 but status bit 30)
this is currently not supported in reset-intel-syscon
This is most tricky and ugly part for VRX200/Danube. Do you have any
idea to handle this nicely?
with reset-lantiq we have the following register information:
a) reset offset: first reg property
b) status offset: second reg property
c) reset bit: first #reset-cell
d) status bit: second #reset-cell
reset-intel-syscon derives half of this information from the two #reset-cells:
a) reset offset: first #reset-cell
b) status offset: reset offset - 0x4
c) reset bit: second #reset-cell
d) status bit: same as reset bit
I cannot make any suggestion (yet) how to handle VRX200 and LGM in o
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
On 8/29/2019 3:36 AM, Martin Blumenstingl wrote:
On Wed, Aug 28, 2019 at 5:35 AM Chuan Hua, Lei
wrote:
[...]
+static int intel_pcie_ep_rst_init(struct intel_pcie_port *lpp)
+{
+struct device *dev = lpp->pci->dev;
+int ret = 0;
+
+lpp->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+if (IS_ERR(lpp->reset_gpio)) {
+ret = PTR_ERR(lpp->reset_gpio);
+if (ret != -EPROBE_DEFER)
+dev_err(dev, "failed to request PCIe GPIO: %d\n", ret);
+return ret;
+}
+/* Make initial reset last for 100ms */
+msleep(100);
why is there lpp->rst_interval when you hardcode 100ms here?
There are different purpose. rst_interval is purely for asserted reset
pulse.
Here 100ms is to make sure the initial state keeps at least 100ms, then we
can reset.
my interpretation is that it totally depends on the board design or
the bootloader setup.
Partially, you are right. However, we should not add some dependency
here from
bootloader and board. rst_interval is just to make sure the pulse (low
active or high active)
lasts the specified the time.
+Cc Kishon
he recently added support for a GPIO reset line to the
pcie-cadence-host.c [0] and I believe he's also maintaining
pci-keystone.c which are both using a 100uS delay (instead of 100ms).
I don't know the PCIe spec so maybe Kishon can comment on the values
that should be used according to the spec.
if there's then a reason why values other than the ones from the spec
are needed then there should be a comment explaining why different
values are needed (what problem does it solve).
spec doesn't guide this part. It is a board or SoC specific setting.
100us also should work. spec only requirs reset duration should last
100ms. The idea is that before reset assert and deassert, make sure the
default deassert status keeps some time. We take this value from
hardware suggestion long time back. We can reduce this value to 100us,
but we need to test on the board.
OK. I don't know how other PCI controller drivers manage this. if the
PCI maintainers are happy with this then I am as well
maybe it's worth changing the comment to indicate that this delay was
suggested by the hardware team (so it's clear that this is not coming
from the PCI spec)
Dilip will change to 100us delay and run the test. I also need to run
some tests for old boards(XRX350/550/PRX300) to confirm this has no
impact on function.
[...]
+static void __intel_pcie_remove(struct intel_pcie_port *lpp)
+{
+pcie_rc_cfg_wr_mask(lpp, PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER,
+0, PCI_COMMAND);
I expect logic like this to be part of the PCI subsystem in Linux.
why is this needed?
[...]
bind/unbind case we use this. For extreme cases, we use unbind and bind
to reset
PCI instead of rebooting.
OK, but this does not seem Intel/Lantiq specific at all
why isn't this managed by either pcie-designware-host.c or the generic
PCI/PCIe subsystem in Linux?
I doubt if other RC driver will support bind/unbind. We do have this
requirement due to power management from WiFi devices.
pcie-designware-host.c will gain .remove() support in Linux 5.4
I don't understand how .remove() and then .probe() again is different
from .unbind() followed by a .bind()
Good. If this is the case, bind/unbind eventually goes to probe/remove,
so we can remove this.
OK. as far as I understand you need to call dw_pcie_host_deinit from
the .remove() callback (which is missing in this version)
(I'm using drivers/pci/controller/dwc/pcie-tegra194.c as an example,
this driver is in linux-next and thus will appear in Linux 5.4)
Thanks for your information. We should adapt this in next version.
Martin
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
On 8/29/2019 4:01 AM, Martin Blumenstingl wrote:
Hi,
On Wed, Aug 28, 2019 at 3:53 AM Chuan Hua, Lei
wrote:
[...]
1. reset-lantiq.c use index instead of register offset + bit position.
index reset is good for a small system (< 64). However, it will become very
difficult to use if you have > 100 reset. So we use register offset +
bit position
reset-lantiq uses bit bit positions for specifying the reset line.
for example this is from OpenWrt's vr9.dtsi:
reset0: reset-controller@10 {
...
reg = <0x10 4>, <0x14 4>;
#reset-cells = <2>;
};
gphy0: gphy@20 {
...
resets = < 31 30>, < 7 7>;
reset-names = "gphy", "gphy2";
};
in my own words this means:
- all reset0 reset bits are at offset 0x10 (parent is RCU)
- all reset0 status bits are at offset 0x14 (parent is RCU)
- the first reset line uses reset bit 31 and status bit 30
- the second reset line uses reset bit 7 and status bit 7
- there can be multiple reset-controller instances, each taking the
reset and status offsets (OpenWrt's vr9.dtsi specifies the second RCU
reset controller "reset1" with reset offset 0x48 and status offset
0x24)
in reset-lantiq.c, we split each reset request /status pair into one
reset controller.
Each reset controller handles up to 32 resets. It will create up to 9
even more
reset controllers in the new SoCs. In reality, there is only one RCU
controller for all
SoCs. These designs worked but did not follow what hardware implemented.
After checking the existing code and referring to other implementation,
we decided to
use register offset + bit position method. It can support all SoCs with
this methods
without code change(device tree change only).
maybe I have a different interpretation of what "RCU" does.
let me explain it in my own words based on my knowledge about VRX200:
- in my own words it is a multi function device with the following
functionality:
- it contains two reset controllers (reset at 0x10, status 0x14 and
reset at 0x48, status at 0x24)
- it contains two USB2 PHYs (PHY registers at 0x18, ANA cfg at 0x38
and PHY registers at 0x34, ANA cfg at 0x3c)
- it contains the configuration for the two GPHY IP blocks (at 0x20 and 0x68)
- it contains endianness configuration registers (for PCI, PCIe, ...)
- it contains the watchdog boot status (whether the SoC was previously
reset by the WDT)
- maybe more, but I don't know anything else about it
In fact, there is only one reset controller for all SoCs even it doesn't
prevent software from virtualizing multiple reset controllers. Reset
control does include some misc stuff which has been moved to chiptop in
new SoCs so that RCU has a clean job.
just to confirm that I understand this correctly:
even the VRX200 SoC only has one physical reset controller?
instead of a contiguous register area (let's say: 0x10 to 0x1c) it
uses four separate registers:
- 0x10 for asserting/deasserting/pulsing the first 32 reset lines
- 0x14 for the status of the first 32 reset lines
- 0x48 for asserting/deasserting/pulsing the second 32 reset lines
- 0x28 for the status of the second 32 reset lines
Yes, but for VRX200, reset controller registers include some other misc
registers. At that time,
hardware doesn't use chiptop concept, they put some misc registers into
CGU/RCU which makes it quite messy.
We also prefer to have 0x10~0x1c. However, when developing VRX200, 0x18,
0x20 and other address had been used by other registers. system becomes
more complex, need more reset bits for new modules, then hardware just
added them to any available place. From another angle, hardware people
also tried to keep backward compatible with old products like Danube.
I'm not surprised that we got some of the IP block layout for the
VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW
(BSP).
with proper documentation (as in a "public datasheet for the SoC") it
would be easy to spot these mistakes (at least I assume that the
quality of the Infineon / Lantiq datasheets is excellent).
back to reset-intel-syscon:
assigning only one job to the RCU hardware is a good idea (in my opinion).
that brings up a question: why do we need the "syscon" compatible for
the RCU node?
this is typically used when registers are accessed by another IP block
and the other driver has to access these registers as well. does this
mean that there's more hidden in the RCU registers?
As I mentioned, some other misc registers are put into RCU even they
don't belong to reset functions. In MIPS, global software reset handled
in arch/mips/, only recently, this situation changed. This means we have
at least two places to access this module.
2. reset-lantiq.c does not support device restart which is part of the
reset in
old lantiq SoC. It moved this part into arch/mips/lantiq directory.
it was moved to the .dts instead o
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
Hi Martin, Thanks for your comment. On 8/28/2019 4:38 AM, Martin Blumenstingl wrote: Hello, On Tue, Aug 27, 2019 at 5:09 AM Chuan Hua, Lei wrote: Hi Martin, Thanks for your feedback. Please check the comments below. On 8/27/2019 5:15 AM, Martin Blumenstingl wrote: Hello, On Mon, Aug 26, 2019 at 5:31 AM Chuan Hua, Lei wrote: Hi Martin, Thanks for your valuable comments. I reply some of them as below. you're welcome [...] +config PCIE_INTEL_AXI +bool "Intel AHB/AXI PCIe host controller support" I believe that this is mostly the same IP block as it's used in Lantiq (xDSL) VRX200 SoCs (with MIPS cores) which was introduced in 2010 (before Intel acquired Lantiq). This is why I would have personally called the driver PCIE_LANTIQ VRX200 SoC(internally called VR9) was the first PCIe SoC product which was using synopsys controller v3.30a. It only supports PCIe Gen1.1/1.0. The phy is internal phy from infineon. thank you for these details I wasn't aware that the PCIe PHY on these SoCs was developed by Infineon nor is the DWC version documented anywhere VRX200/ARX300 PHY is internal value. There are a lot of hardcode which was from hardware people. From XRX500, we switch to synopsis PHY. However, later comboPHY is coming to the picture. Even though we have one same controller with different versions, we most likely will have three different phy drivers. that is a good argument for using a separate PHY driver and integrating that using the PHY subsystem (which is already the case in this patch revision) Right. CombonPHY(PRX300/Lighting Mountain) and SlimPHY driver(XRX350/550) are in the way to upstream. [...] +#define PCIE_CCRID 0x8 + +#define PCIE_LCAP 0x7C +#define PCIE_LCAP_MAX_LINK_SPEEDGENMASK(3, 0) +#define PCIE_LCAP_MAX_LENGTH_WIDTH GENMASK(9, 4) + +/* Link Control and Status Register */ +#define PCIE_LCTLSTS0x80 +#define PCIE_LCTLSTS_ASPM_ENABLEGENMASK(1, 0) +#define PCIE_LCTLSTS_RCB128 BIT(3) +#define PCIE_LCTLSTS_LINK_DISABLE BIT(4) +#define PCIE_LCTLSTS_COM_CLK_CFGBIT(6) +#define PCIE_LCTLSTS_HW_AW_DIS BIT(9) +#define PCIE_LCTLSTS_LINK_SPEED GENMASK(19, 16) +#define PCIE_LCTLSTS_NEGOTIATED_LINK_WIDTH GENMASK(25, 20) +#define PCIE_LCTLSTS_SLOT_CLK_CFG BIT(28) + +#define PCIE_LCTLSTS2 0xA0 +#define PCIE_LCTLSTS2_TGT_LINK_SPEEDGENMASK(3, 0) +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_25GT 0x1 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_5GT0x2 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_8GT0x3 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_16GT 0x4 +#define PCIE_LCTLSTS2_HW_AUTO_DIS BIT(5) + +/* Ack Frequency Register */ +#define PCIE_AFR0x70C +#define PCIE_AFR_FTS_NUMGENMASK(15, 8) +#define PCIE_AFR_COM_FTS_NUMGENMASK(23, 16) +#define PCIE_AFR_GEN12_FTS_NUM_DFT (SZ_128 - 1) +#define PCIE_AFR_GEN3_FTS_NUM_DFT 180 +#define PCIE_AFR_GEN4_FTS_NUM_DFT 196 + +#define PCIE_PLCR_DLL_LINK_EN BIT(5) +#define PCIE_PORT_LOGIC_FTS GENMASK(7, 0) +#define PCIE_PORT_LOGIC_DFT_FTS_NUM (SZ_128 - 1) + +#define PCIE_MISC_CTRL 0x8BC +#define PCIE_MISC_CTRL_DBI_RO_WR_EN BIT(0) + +#define PCIE_MULTI_LANE_CTRL0x8C0 +#define PCIE_UPCONFIG_SUPPORT BIT(7) +#define PCIE_DIRECT_LINK_WIDTH_CHANGE BIT(6) +#define PCIE_TARGET_LINK_WIDTH GENMASK(5, 0) + +#define PCIE_IOP_CTRL 0x8C4 +#define PCIE_IOP_RX_STANDBY_CTRLGENMASK(6, 0) no need for IOP with "are you sure that you need any of the registers above?" I really meant all registers above (including, but not limited to IOP) [...] As I mentioned, VRX200 was a very old PCIe Gen1.1 product. In our latest SoC Lightning Mountain, we are using synopsys controller 5.20/5.50a. We support Gen2(XRX350/550), Gen3(PRX300) and GEN4(X86 based SoC). We also supported dual lane and single lane. Some of the above registers are needed to control FTS, link width and link speed. only now I noticed that I didn't explain why I was asking whether all these registers are needed my understanding of the DWC PCIe controller driver "library" is that: - all functionality which is provided by the DesignWare PCIe core should be added to drivers/pci/controller/dwc/pcie-designware* - functionality which is built on top/around the DWC PCIe core should be added to the link width and link speed settings (I don't know about "FTS") don't seem Intel/Lantiq controller specific to me so the register setup for these bits should be moved to drivers/pci/controller/dwc/pcie-designware* FTS means fast
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin,
On 8/28/2019 5:15 AM, Martin Blumenstingl wrote:
Hi,
On Tue, Aug 27, 2019 at 4:23 AM Chuan Hua, Lei
wrote:
[...]
1. reset-lantiq.c use index instead of register offset + bit position.
index reset is good for a small system (< 64). However, it will become very
difficult to use if you have > 100 reset. So we use register offset +
bit position
reset-lantiq uses bit bit positions for specifying the reset line.
for example this is from OpenWrt's vr9.dtsi:
reset0: reset-controller@10 {
...
reg = <0x10 4>, <0x14 4>;
#reset-cells = <2>;
};
gphy0: gphy@20 {
...
resets = < 31 30>, < 7 7>;
reset-names = "gphy", "gphy2";
};
in my own words this means:
- all reset0 reset bits are at offset 0x10 (parent is RCU)
- all reset0 status bits are at offset 0x14 (parent is RCU)
- the first reset line uses reset bit 31 and status bit 30
- the second reset line uses reset bit 7 and status bit 7
- there can be multiple reset-controller instances, each taking the
reset and status offsets (OpenWrt's vr9.dtsi specifies the second RCU
reset controller "reset1" with reset offset 0x48 and status offset
0x24)
in reset-lantiq.c, we split each reset request /status pair into one
reset controller.
Each reset controller handles up to 32 resets. It will create up to 9
even more
reset controllers in the new SoCs. In reality, there is only one RCU
controller for all
SoCs. These designs worked but did not follow what hardware implemented.
After checking the existing code and referring to other implementation,
we decided to
use register offset + bit position method. It can support all SoCs with
this methods
without code change(device tree change only).
maybe I have a different interpretation of what "RCU" does.
let me explain it in my own words based on my knowledge about VRX200:
- in my own words it is a multi function device with the following
functionality:
- it contains two reset controllers (reset at 0x10, status 0x14 and
reset at 0x48, status at 0x24)
- it contains two USB2 PHYs (PHY registers at 0x18, ANA cfg at 0x38
and PHY registers at 0x34, ANA cfg at 0x3c)
- it contains the configuration for the two GPHY IP blocks (at 0x20 and 0x68)
- it contains endianness configuration registers (for PCI, PCIe, ...)
- it contains the watchdog boot status (whether the SoC was previously
reset by the WDT)
- maybe more, but I don't know anything else about it
In fact, there is only one reset controller for all SoCs even it doesn't
prevent software from virtualizing multiple reset controllers. Reset
control does include some misc stuff which has been moved to chiptop in
new SoCs so that RCU has a clean job.
we tried our best to document this in
Documentation/devicetree/bindings/mips/lantiq/rcu.txt
I'm not sure about the details of the RCU on the LGM SoCs:
if it contains more than just reset controllers then please let Rob
Herring (dt-bindings maintainer) know about this.
we may only have one chance to do it right, if we start with a
"broken" binding then devices with incompatible bootloaders etc. may
have already shipped
(in general: that is why the devicetree maintainers want to have all
device properties documented in the binding, even if the driver does
not support all of them yet)
2. reset-lantiq.c does not support device restart which is part of the
reset in
old lantiq SoC. It moved this part into arch/mips/lantiq directory.
it was moved to the .dts instead of the arch code. again from
OpenWrt's vr9.dtsi [0]:
reboot {
compatible = "syscon-reboot";
regmap = <>;
offset = <0x10>;
mask = <0xe000>;
};
this sets the reset0 reset bits 31, 30 and 29 at reboot
ok. but not sure why we need to reset bit 31 and 29. global softwre
reset is bit 30.
I don't know either. depending on what the LGM SoCs need you can
change the "mask" property to the value that fits that SoC best
[...]
All SoCs have only one global software reset bit.
- other reset lines only support reset pulses. the _reset function
should be used in this case
- the _reset function should only assert the reset line, then wait
until the hardware automatically de-asserts it (without any further
write)
Yes, this is called hardware reset. We can't control reset duration.
is this the same for all, old and new SoCs?
New SoCs have removed support for hardware reset after software's feedback.
Old SoCs such as VRX200/ARX300 has both software/hardware resets
nice, it's good to see teamwork between hardware and software teams!
[...]
4. Code not optimized and intel internal review not assessed.
insights from you (like the issue with the reset callback) are very
valuable - this shows that we should focus on having one driver.
Based on the above findings, I would suggest reset-lantiq.c to move to
reset-intel-syscon.c
my concern with having two sepa
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
Hi Martin, Thanks for your feedback. Please check the comments below. On 8/27/2019 5:15 AM, Martin Blumenstingl wrote: Hello, On Mon, Aug 26, 2019 at 5:31 AM Chuan Hua, Lei wrote: Hi Martin, Thanks for your valuable comments. I reply some of them as below. you're welcome [...] +config PCIE_INTEL_AXI +bool "Intel AHB/AXI PCIe host controller support" I believe that this is mostly the same IP block as it's used in Lantiq (xDSL) VRX200 SoCs (with MIPS cores) which was introduced in 2010 (before Intel acquired Lantiq). This is why I would have personally called the driver PCIE_LANTIQ VRX200 SoC(internally called VR9) was the first PCIe SoC product which was using synopsys controller v3.30a. It only supports PCIe Gen1.1/1.0. The phy is internal phy from infineon. thank you for these details I wasn't aware that the PCIe PHY on these SoCs was developed by Infineon nor is the DWC version documented anywhere VRX200/ARX300 PHY is internal value. There are a lot of hardcode which was from hardware people. From XRX500, we switch to synopsis PHY. However, later comboPHY is coming to the picture. Even though we have one same controller with different versions, we most likely will have three different phy drivers. [...] +#define PCIE_CCRID 0x8 + +#define PCIE_LCAP 0x7C +#define PCIE_LCAP_MAX_LINK_SPEEDGENMASK(3, 0) +#define PCIE_LCAP_MAX_LENGTH_WIDTH GENMASK(9, 4) + +/* Link Control and Status Register */ +#define PCIE_LCTLSTS0x80 +#define PCIE_LCTLSTS_ASPM_ENABLEGENMASK(1, 0) +#define PCIE_LCTLSTS_RCB128 BIT(3) +#define PCIE_LCTLSTS_LINK_DISABLE BIT(4) +#define PCIE_LCTLSTS_COM_CLK_CFGBIT(6) +#define PCIE_LCTLSTS_HW_AW_DIS BIT(9) +#define PCIE_LCTLSTS_LINK_SPEED GENMASK(19, 16) +#define PCIE_LCTLSTS_NEGOTIATED_LINK_WIDTH GENMASK(25, 20) +#define PCIE_LCTLSTS_SLOT_CLK_CFG BIT(28) + +#define PCIE_LCTLSTS2 0xA0 +#define PCIE_LCTLSTS2_TGT_LINK_SPEEDGENMASK(3, 0) +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_25GT 0x1 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_5GT0x2 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_8GT0x3 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_16GT 0x4 +#define PCIE_LCTLSTS2_HW_AUTO_DIS BIT(5) + +/* Ack Frequency Register */ +#define PCIE_AFR0x70C +#define PCIE_AFR_FTS_NUMGENMASK(15, 8) +#define PCIE_AFR_COM_FTS_NUMGENMASK(23, 16) +#define PCIE_AFR_GEN12_FTS_NUM_DFT (SZ_128 - 1) +#define PCIE_AFR_GEN3_FTS_NUM_DFT 180 +#define PCIE_AFR_GEN4_FTS_NUM_DFT 196 + +#define PCIE_PLCR_DLL_LINK_EN BIT(5) +#define PCIE_PORT_LOGIC_FTS GENMASK(7, 0) +#define PCIE_PORT_LOGIC_DFT_FTS_NUM (SZ_128 - 1) + +#define PCIE_MISC_CTRL 0x8BC +#define PCIE_MISC_CTRL_DBI_RO_WR_EN BIT(0) + +#define PCIE_MULTI_LANE_CTRL0x8C0 +#define PCIE_UPCONFIG_SUPPORT BIT(7) +#define PCIE_DIRECT_LINK_WIDTH_CHANGE BIT(6) +#define PCIE_TARGET_LINK_WIDTH GENMASK(5, 0) + +#define PCIE_IOP_CTRL 0x8C4 +#define PCIE_IOP_RX_STANDBY_CTRLGENMASK(6, 0) no need for IOP with "are you sure that you need any of the registers above?" I really meant all registers above (including, but not limited to IOP) [...] As I mentioned, VRX200 was a very old PCIe Gen1.1 product. In our latest SoC Lightning Mountain, we are using synopsys controller 5.20/5.50a. We support Gen2(XRX350/550), Gen3(PRX300) and GEN4(X86 based SoC). We also supported dual lane and single lane. Some of the above registers are needed to control FTS, link width and link speed. only now I noticed that I didn't explain why I was asking whether all these registers are needed my understanding of the DWC PCIe controller driver "library" is that: - all functionality which is provided by the DesignWare PCIe core should be added to drivers/pci/controller/dwc/pcie-designware* - functionality which is built on top/around the DWC PCIe core should be added to the link width and link speed settings (I don't know about "FTS") don't seem Intel/Lantiq controller specific to me so the register setup for these bits should be moved to drivers/pci/controller/dwc/pcie-designware* FTS means fast training sequence. Different generations will have different FTS. Common DWC drivers have default number for all generations which are not optimized. DWC driver handles link speed and link width during the initialization. Then left link speed change and link width to device (EP) according to PCIe spec. Not sure if other vendors or customers have this kind of requirement. We implemented this due
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin,
Please check the reply below.
On 8/27/2019 5:49 AM, Martin Blumenstingl wrote:
Hi,
On Mon, Aug 26, 2019 at 6:01 AM Chuan Hua, Lei
wrote:
Hi Martin,
Thanks for your comment.
thank you for the quick reply
On 8/25/2019 5:11 AM, Martin Blumenstingl wrote:
Hi Dilip,
Add driver for the reset controller present on Intel
Lightening Mountain (LGM) SoC for performing reset
management of the devices present on the SoC. Driver also
registers a reset handler to peform the entire device reset.
[...]
+static const struct of_device_id intel_reset_match[] = {
+{ .compatible = "intel,rcu-lgm" },
+{}
+};
how is this IP block differnet from the one used in many Lantiq SoCs?
there is already an upstream driver for the RCU IP block on the Lantiq
SoCs: drivers/reset/reset-lantiq.c
some background:
Lantiq was started as a spinoff from Infineon in 2009. Intel then
acquired Lantiq in 2015. source: [0]
Intel is re-using some of the IP blocks from the MIPS Lantiq SoCs
(Intel even has some own MIPS SoCs as part of the Lantiq acquisition,
typically used for PON/GPON/ADSL/VDSL capable network devices).
Thus I think it is likely that the new "Lightening Mountain" SoCs use
an updated version of the Lantiq RCU IP.
I would not say there is a fundamental difference since reset is a
really simple
stuff from all reset drivers. However, it did have some difference
from existing reset-lantiq.c since SoC becomes more and more complex.
OK, let me go through your detailed list
1. reset-lantiq.c use index instead of register offset + bit position.
index reset is good for a small system (< 64). However, it will become very
difficult to use if you have > 100 reset. So we use register offset +
bit position
reset-lantiq uses bit bit positions for specifying the reset line.
for example this is from OpenWrt's vr9.dtsi:
reset0: reset-controller@10 {
...
reg = <0x10 4>, <0x14 4>;
#reset-cells = <2>;
};
gphy0: gphy@20 {
...
resets = < 31 30>, < 7 7>;
reset-names = "gphy", "gphy2";
};
in my own words this means:
- all reset0 reset bits are at offset 0x10 (parent is RCU)
- all reset0 status bits are at offset 0x14 (parent is RCU)
- the first reset line uses reset bit 31 and status bit 30
- the second reset line uses reset bit 7 and status bit 7
- there can be multiple reset-controller instances, each taking the
reset and status offsets (OpenWrt's vr9.dtsi specifies the second RCU
reset controller "reset1" with reset offset 0x48 and status offset
0x24)
in reset-lantiq.c, we split each reset request /status pair into one
reset controller.
Each reset controller handles up to 32 resets. It will create up to 9
even more
reset controllers in the new SoCs. In reality, there is only one RCU
controller for all
SoCs. These designs worked but did not follow what hardware implemented.
After checking the existing code and referring to other implementation,
we decided to
use register offset + bit position method. It can support all SoCs with
this methods
without code change(device tree change only).
2. reset-lantiq.c does not support device restart which is part of the
reset in
old lantiq SoC. It moved this part into arch/mips/lantiq directory.
it was moved to the .dts instead of the arch code. again from
OpenWrt's vr9.dtsi [0]:
reboot {
compatible = "syscon-reboot";
regmap = <>;
offset = <0x10>;
mask = <0xe000>;
};
this sets the reset0 reset bits 31, 30 and 29 at reboot
ok. but not sure why we need to reset bit 31 and 29. global softwre
reset is bit 30.
3. reset-lantiqc reset callback doesn't implement what hardware implemented
function. In old SoCs, some bits in the same register can be hardware
reset clear.
It just call assert + assert. For these SoCs, we should only call
assert, hardware will auto deassert.
I didn't know that. so to confirm I understand this correctly:
- some reset lines must be asserted and de-asserted manually (setting
and clearing the bit manually). the _assert and _deassert functions
should be used in this case
Yes. We call software managed reset. callers call assert, deassert and delay
duration according to their specific requirement.
- other reset lines only support reset pulses. the _reset function
should be used in this case
- the _reset function should only assert the reset line, then wait
until the hardware automatically de-asserts it (without any further
write)
Yes, this is called hardware reset. We can't control reset duration.
is this the same for all, old and new SoCs?
New SoCs have removed support for hardware reset after software's feedback.
Old SoCs such as VRX200/ARX300 has both software/hardware resets
only two mainline Lantiq drivers are using reset lines - both are
using the _assert and _deassert variants:
- drivers/net/dsa/lantiq_gswip.c
- drivers/phy/lanti
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin,
Thanks for your comment.
On 8/25/2019 5:11 AM, Martin Blumenstingl wrote:
Hi Dilip,
Add driver for the reset controller present on Intel
Lightening Mountain (LGM) SoC for performing reset
management of the devices present on the SoC. Driver also
registers a reset handler to peform the entire device reset.
[...]
+static const struct of_device_id intel_reset_match[] = {
+ { .compatible = "intel,rcu-lgm" },
+ {}
+};
how is this IP block differnet from the one used in many Lantiq SoCs?
there is already an upstream driver for the RCU IP block on the Lantiq
SoCs: drivers/reset/reset-lantiq.c
some background:
Lantiq was started as a spinoff from Infineon in 2009. Intel then
acquired Lantiq in 2015. source: [0]
Intel is re-using some of the IP blocks from the MIPS Lantiq SoCs
(Intel even has some own MIPS SoCs as part of the Lantiq acquisition,
typically used for PON/GPON/ADSL/VDSL capable network devices).
Thus I think it is likely that the new "Lightening Mountain" SoCs use
an updated version of the Lantiq RCU IP.
I would not say there is a fundamental difference since reset is a
really simple
stuff from all reset drivers. However, it did have some difference
from existing reset-lantiq.c since SoC becomes more and more complex.
1. reset-lantiq.c use index instead of register offset + bit position.
index reset is good for a small system (< 64). However, it will become very
difficult to use if you have > 100 reset. So we use register offset +
bit position
2. reset-lantiq.c does not support device restart which is part of the
reset in
old lantiq SoC. It moved this part into arch/mips/lantiq directory.
3. reset-lantiqc reset callback doesn't implement what hardware implemented
function. In old SoCs, some bits in the same register can be hardware
reset clear.
It just call assert + assert. For these SoCs, we should only call
assert, hardware
will auto deassert.
4. Code not optimized and intel internal review not assessed.
Based on the above findings, I would suggest reset-lantiq.c to move to
reset-intel-syscon.c
What is your opinion?
Chuanhua
Martin
[0] https://wikidevi.com/wiki/Lantiq
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
Hi Martin, Thanks for your valuable comments. I reply some of them as below. Regards, Chuanhua On 8/25/2019 5:03 AM, Martin Blumenstingl wrote: Hi Dilip, first of all: thank you for submitting this upstream! I hope that we can use this driver to replace the out-of-tree PCIe driver that's used in OpenWrt for the Lantiq VRX200 SoCs. a small disclaimer: I don't have access to any Lantiq, Intel or DesignWare datasheets. so everything I write below is based on my own understanding of the Tegra public datasheet (which describes the DesignWare PCIe registers) as well as the public Lantiq UGW code drops with out-of-tree drivers for an older version of this PCIe IP. thus some of my statements below may be wrong - in this case I would appreciate an explanation of how the hardware really works. please keep me CC'ed on further revisions of this series. I am highly interested in making this driver work on the Lantiq VRX200 SoCs once the initial version has landed in linux-next. +config PCIE_INTEL_AXI +bool "Intel AHB/AXI PCIe host controller support" I believe that this is mostly the same IP block as it's used in Lantiq (xDSL) VRX200 SoCs (with MIPS cores) which was introduced in 2010 (before Intel acquired Lantiq). This is why I would have personally called the driver PCIE_LANTIQ VRX200 SoC(internally called VR9) was the first PCIe SoC product which was using synopsys controller v3.30a. It only supports PCIe Gen1.1/1.0. The phy is internal phy from infineon. After that, we have other PCe 1.1/10. products such as ARX300/390. However, these products are EOL, that is why we don't put effort to support VRX200/ARX300/390. PCIE_LANTIQ was also a name used internally in the product as in linux-3.10.x. [...] +#define PCIE_CCRID 0x8 + +#define PCIE_LCAP 0x7C +#define PCIE_LCAP_MAX_LINK_SPEED GENMASK(3, 0) +#define PCIE_LCAP_MAX_LENGTH_WIDTH GENMASK(9, 4) + +/* Link Control and Status Register */ +#define PCIE_LCTLSTS 0x80 +#define PCIE_LCTLSTS_ASPM_ENABLE GENMASK(1, 0) +#define PCIE_LCTLSTS_RCB128BIT(3) +#define PCIE_LCTLSTS_LINK_DISABLE BIT(4) +#define PCIE_LCTLSTS_COM_CLK_CFG BIT(6) +#define PCIE_LCTLSTS_HW_AW_DIS BIT(9) +#define PCIE_LCTLSTS_LINK_SPEEDGENMASK(19, 16) +#define PCIE_LCTLSTS_NEGOTIATED_LINK_WIDTH GENMASK(25, 20) +#define PCIE_LCTLSTS_SLOT_CLK_CFG BIT(28) + +#define PCIE_LCTLSTS2 0xA0 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED GENMASK(3, 0) +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_25GT 0x1 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_5GT 0x2 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_8GT 0x3 +#define PCIE_LCTLSTS2_TGT_LINK_SPEED_16GT 0x4 +#define PCIE_LCTLSTS2_HW_AUTO_DIS BIT(5) + +/* Ack Frequency Register */ +#define PCIE_AFR 0x70C +#define PCIE_AFR_FTS_NUM GENMASK(15, 8) +#define PCIE_AFR_COM_FTS_NUM GENMASK(23, 16) +#define PCIE_AFR_GEN12_FTS_NUM_DFT (SZ_128 - 1) +#define PCIE_AFR_GEN3_FTS_NUM_DFT 180 +#define PCIE_AFR_GEN4_FTS_NUM_DFT 196 + +#define PCIE_PLCR_DLL_LINK_EN BIT(5) +#define PCIE_PORT_LOGIC_FTSGENMASK(7, 0) +#define PCIE_PORT_LOGIC_DFT_FTS_NUM(SZ_128 - 1) + +#define PCIE_MISC_CTRL 0x8BC +#define PCIE_MISC_CTRL_DBI_RO_WR_ENBIT(0) + +#define PCIE_MULTI_LANE_CTRL 0x8C0 +#define PCIE_UPCONFIG_SUPPORT BIT(7) +#define PCIE_DIRECT_LINK_WIDTH_CHANGE BIT(6) +#define PCIE_TARGET_LINK_WIDTH GENMASK(5, 0) + +#define PCIE_IOP_CTRL 0x8C4 +#define PCIE_IOP_RX_STANDBY_CTRL GENMASK(6, 0) no need for IOP are you sure that you need any of the registers above? as far as I can tell most (all?) of them are part of the DesignWare register set. why doesn't pcie-designware-host initialize these? I don't have any datasheet or register documentation for the DesignWare PCIe core. In my own experiment from a month ago on the Lantiq VRX200 SoC I didn't need any of this: [0] As I mentioned, VRX200 was a very old PCIe Gen1.1 product. In our latest SoC Lightning Mountain, we are using synopsys controller 5.20/5.50a. We support Gen2(XRX350/550), Gen3(PRX300) and GEN4(X86 based SoC). We also supported dual lane and single lane. Some of the above registers are needed to control FTS, link width and link speed. this also makes me wonder if various functions below like intel_pcie_link_setup() and intel_pcie_max_speed_setup() (and probably more) are really needed or if it's just cargo cult / copy paste from an out-of-tree driver). intel_pcie_link_setup is to record maximum speed and and link width. We need these to
Re: [PATCH v15 19/26] x86/tsc: calibrate tsc only once
static unsigned long __init get_loops_per_jiffy(void)
{
unsigned long lpj = tsc_khz * KHZ;
do_div(lpj, HZ);
return lpj;
}
Just tried this with 4.19-rc2 on x86(32bit). lpj return as zero which is not
expected
After disassembling the code,
0xc1239a9e <+199>: imul $0x3e8,0xc12296e4,%edx
0xc1239aa8 <+209>: xor%ecx,%ecx
0xc1239aaa <+211>: test %edx,%edx
0xc1239aac <+213>: mov%eax,%ebx
0xc1239aae <+215>: je 0xc1239abd
0xc1239ab0 <+217>: mov$0x64,%ecx
0xc1239ab5 <+222>: mov%edx,%eax
0xc1239ab7 <+224>: xor%edx,%edx
0xc1239ab9 <+226>: div%ecx
0xc1239abb <+228>: mov%eax,%ecx
0xc1239abd <+230>: mov%ebx,%eax
0xc1239abf <+232>: mov$0x64,%ebx
0xc1239ac4 <+237>: div%ebx
0xc1239ac6 <+239>: mov%ecx,%edx
imul will load the result into %edx, %edx supposed to be high 32 bit which is
not zero,
It should be zero in this case. both lpj and tsc_khz should be u64 to work
properly.
Re: [PATCH v15 19/26] x86/tsc: calibrate tsc only once
static unsigned long __init get_loops_per_jiffy(void)
{
unsigned long lpj = tsc_khz * KHZ;
do_div(lpj, HZ);
return lpj;
}
Just tried this with 4.19-rc2 on x86(32bit). lpj return as zero which is not
expected
After disassembling the code,
0xc1239a9e <+199>: imul $0x3e8,0xc12296e4,%edx
0xc1239aa8 <+209>: xor%ecx,%ecx
0xc1239aaa <+211>: test %edx,%edx
0xc1239aac <+213>: mov%eax,%ebx
0xc1239aae <+215>: je 0xc1239abd
0xc1239ab0 <+217>: mov$0x64,%ecx
0xc1239ab5 <+222>: mov%edx,%eax
0xc1239ab7 <+224>: xor%edx,%edx
0xc1239ab9 <+226>: div%ecx
0xc1239abb <+228>: mov%eax,%ecx
0xc1239abd <+230>: mov%ebx,%eax
0xc1239abf <+232>: mov$0x64,%ebx
0xc1239ac4 <+237>: div%ebx
0xc1239ac6 <+239>: mov%ecx,%edx
imul will load the result into %edx, %edx supposed to be high 32 bit which is
not zero,
It should be zero in this case. both lpj and tsc_khz should be u64 to work
properly.
