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 = <&ioapic1>; + interrupts = <67 1>; + #interrupt-cells = <1>; + interrupt-map-mask = <0 0 0 0x7>; + interrupt-map = <0 0 0 1 &ioapic1 27 1>, + <0 0 0 2 &ioapic1 28 1>, + <0 0 0 3 &ioapic1 29 1>, + <0 0 0 4 &ioapic1 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. reg
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 +#defi
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 = <&ioapic1>; + interrupts = <67 1>; + #interrupt-cells = <1>; + interrupt-map-mask = <0 0 0 0x7>; + interrupt-map = <0 0 0 1 &ioapic1 27 1>, + <0 0 0 2 &ioapic1 28 1>, + <0 0 0 3 &ioapic1 29 1>, + <0 0 0 4 &ioapic1 30 1>; + ranges = <0x0200 0 0xd400 0xd400 0 0x0400>; + resets = <&rcu0 0x50 0>; + clocks = <&cgu0 LGM_GCLK_PCIE10>; + phys = <&cb0phy0>; + phy-names = "pciephy"; + status = "okay"; + reset-assert-ms = <500>; + reset-gpios = <&gpio0 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-sysc
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
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 = <&reset0 31 30>, <&reset1 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
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-des
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 = <&reset0 31 30>, <&reset1 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 = <&rcu0>; 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 res
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 require
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 = <&reset0 31 30>, <&reset1 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 = <&rcu0>; 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
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.