Re: [PATCH] dt-bindings: PCI: intel,lgm-pcie: Fix matching on all snps,dw-pcie instances
On 8/20/2020 6:20 AM, Rob Herring wrote: The intel,lgm-pcie binding is matching on all snps,dw-pcie instances which is wrong. Add a custom 'select' entry to fix this. Fixes: e54ea45a4955 ("dt-bindings: PCI: intel: Add YAML schemas for the PCIe RC controller") Cc: Bjorn Helgaas Cc: Dilip Kota Cc: linux-...@vger.kernel.org Signed-off-by: Rob Herring --- I'll take this via the DT tree. Rob Documentation/devicetree/bindings/pci/intel-gw-pcie.yaml | 8 1 file changed, 8 insertions(+) diff --git a/Documentation/devicetree/bindings/pci/intel-gw-pcie.yaml b/Documentation/devicetree/bindings/pci/intel-gw-pcie.yaml index 64b2c64ca806..a1e2be737eec 100644 --- a/Documentation/devicetree/bindings/pci/intel-gw-pcie.yaml +++ b/Documentation/devicetree/bindings/pci/intel-gw-pcie.yaml @@ -9,6 +9,14 @@ title: PCIe RC controller on Intel Gateway SoCs maintainers: - Dilip Kota +select: + properties: +compatible: + contains: +const: intel,lgm-pcie + required: +- compatible + properties: compatible: items: Reviewed-by: Dilip Kota Regards, Dilip
[tip: x86/urgent] x86/tsr: Fix tsc frequency enumeration bug on Lightning Mountain SoC
The following commit has been merged into the x86/urgent branch of tip: Commit-ID: 7d98585860d845e36ee612832a5ff021f201dbaf Gitweb: https://git.kernel.org/tip/7d98585860d845e36ee612832a5ff021f201dbaf Author:Dilip Kota AuthorDate:Mon, 03 Aug 2020 15:56:36 +08:00 Committer: Ingo Molnar CommitterDate: Fri, 07 Aug 2020 01:32:00 +02:00 x86/tsr: Fix tsc frequency enumeration bug on Lightning Mountain SoC Frequency descriptor of Lightning Mountain SoC doesn't have all the frequency entries so resulting in the below failure causing a kernel hang: Error MSR_FSB_FREQ index 15 is unknown tsc: Fast TSC calibration failed So, add all the frequency entries in the Lightning Mountain SoC frequency descriptor. Fixes: 0cc5359d8fd45 ("x86/cpu: Update init data for new Airmont CPU model") Fixes: 812c2d7506fd ("x86/tsc_msr: Use named struct initializers") Signed-off-by: Dilip Kota Signed-off-by: Ingo Molnar Reviewed-by: Andy Shevchenko Link: https://lore.kernel.org/r/211c643ae217604b46cbec43a2c0423946dc7d2d.1596440057.git.eswara.k...@linux.intel.com --- arch/x86/kernel/tsc_msr.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/arch/x86/kernel/tsc_msr.c b/arch/x86/kernel/tsc_msr.c index 4fec6f3..a654a9b 100644 --- a/arch/x86/kernel/tsc_msr.c +++ b/arch/x86/kernel/tsc_msr.c @@ -133,10 +133,15 @@ static const struct freq_desc freq_desc_ann = { .mask = 0x0f, }; -/* 24 MHz crystal? : 24 * 13 / 4 = 78 MHz */ +/* + * 24 MHz crystal? : 24 * 13 / 4 = 78 MHz + * Frequency step for Lightning Mountain SoC is fixed to 78 MHz, + * so all the frequency entries are 78000. + */ static const struct freq_desc freq_desc_lgm = { .use_msr_plat = true, - .freqs = { 78000, 78000, 78000, 78000, 78000, 78000, 78000, 78000 }, + .freqs = { 78000, 78000, 78000, 78000, 78000, 78000, 78000, 78000, + 78000, 78000, 78000, 78000, 78000, 78000, 78000, 78000 }, .mask = 0x0f, };
[tip: x86/urgent] x86/tsr: Fix tsc frequency enumeration bug on Lightning Mountain SoC
The following commit has been merged into the x86/urgent branch of tip: Commit-ID: 287bad1f2b30253443e61ff6d5597a76787f736a Gitweb: https://git.kernel.org/tip/287bad1f2b30253443e61ff6d5597a76787f736a Author:Dilip Kota AuthorDate:Mon, 03 Aug 2020 15:56:36 +08:00 Committer: Ingo Molnar CommitterDate: Thu, 06 Aug 2020 15:27:31 +02:00 x86/tsr: Fix tsc frequency enumeration bug on Lightning Mountain SoC Frequency descriptor of Lightning Mountain SoC doesn't have all the frequency entries so resulting in the below failure causing a kernel hang: Error MSR_FSB_FREQ index 15 is unknown tsc: Fast TSC calibration failed So, add all the frequency entries in the Lightning Mountain SoC frequency descriptor. Fixes: 0cc5359d8fd45 ("x86/cpu: Update init data for new Airmont CPU model") Fixes: 812c2d7506fd ("x86/tsc_msr: Use named struct initializers") Signed-off-by: Dilip Kota Signed-off-by: Ingo Molnar Reviewed-by: Andy Shevchenko Link: https://lore.kernel.org/r/211c643ae217604b46cbec43a2c0423946dc7d2d.1596440057.git.eswara.k...@linux.intel.com --- arch/x86/kernel/tsc_msr.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/arch/x86/kernel/tsc_msr.c b/arch/x86/kernel/tsc_msr.c index 4fec6f3..a654a9b 100644 --- a/arch/x86/kernel/tsc_msr.c +++ b/arch/x86/kernel/tsc_msr.c @@ -133,10 +133,15 @@ static const struct freq_desc freq_desc_ann = { .mask = 0x0f, }; -/* 24 MHz crystal? : 24 * 13 / 4 = 78 MHz */ +/* + * 24 MHz crystal? : 24 * 13 / 4 = 78 MHz + * Frequency step for Lightning Mountain SoC is fixed to 78 MHz, + * so all the frequency entries are 78000. + */ static const struct freq_desc freq_desc_lgm = { .use_msr_plat = true, - .freqs = { 78000, 78000, 78000, 78000, 78000, 78000, 78000, 78000 }, + .freqs = { 78000, 78000, 78000, 78000, 78000, 78000, 78000, 78000, + 78000, 78000, 78000, 78000, 78000, 78000, 78000, 78000 }, .mask = 0x0f, };
[PATCH v3 1/1] x86/tsr: Fix tsc frequency enumeration failure on Lightning Mountain SoC
Frequency descriptor of Lightning Mountain SoC doesn't have all the frequency entries so resulting in the below failure causing kernel hang. [0.00] Error MSR_FSB_FREQ index 15 is unknown [0.00] tsc: Fast TSC calibration failed So, add all the frequency entries in the Lightning Mountain SoC frequency descriptor. Fixes: 0cc5359d8fd45 ("x86/cpu: Update init data for new Airmont CPU model") Fixes: 812c2d7506fd ("x86/tsc_msr: Use named struct initializers") Signed-off-by: Dilip Kota Reviewed-by: Andy Shevchenko --- Changes on v3: Fix the nit pick pointed by Andy Add Reviewed-by: Andy Shevchenko Changes on v2: Add description in the comments explaining about frequency entries. arch/x86/kernel/tsc_msr.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/arch/x86/kernel/tsc_msr.c b/arch/x86/kernel/tsc_msr.c index 4fec6f3a1858b2..a654a9b4b77c07 100644 --- a/arch/x86/kernel/tsc_msr.c +++ b/arch/x86/kernel/tsc_msr.c @@ -133,10 +133,15 @@ static const struct freq_desc freq_desc_ann = { .mask = 0x0f, }; -/* 24 MHz crystal? : 24 * 13 / 4 = 78 MHz */ +/* + * 24 MHz crystal? : 24 * 13 / 4 = 78 MHz + * Frequency step for Lightning Mountain SoC is fixed to 78 MHz, + * so all the frequency entries are 78000. + */ static const struct freq_desc freq_desc_lgm = { .use_msr_plat = true, - .freqs = { 78000, 78000, 78000, 78000, 78000, 78000, 78000, 78000 }, + .freqs = { 78000, 78000, 78000, 78000, 78000, 78000, 78000, 78000, + 78000, 78000, 78000, 78000, 78000, 78000, 78000, 78000 }, .mask = 0x0f, }; -- 2.11.0
[PATCH v2 1/1] x86/tsr: Fix tsc frequency enumeration failure on Lightning Mountain SoC
Frequency descriptor of Lightning Mountain SoC doesn't have all the frequency entries so resulting in the below failure causing kernel hang. [0.00] Error MSR_FSB_FREQ index 15 is unknown [0.00] tsc: Fast TSC calibration failed So, add all the frequency entries in the Lightning Mountain SoC frequency descriptor. Fixes: 0cc5359d8fd45 ("x86/cpu: Update init data for new Airmont CPU model") Fixes: 812c2d7506fd ("x86/tsc_msr: Use named struct initializers") Signed-off-by: Dilip Kota --- Changes on v2: Add description in the comments explaining about frequency entries. arch/x86/kernel/tsc_msr.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/arch/x86/kernel/tsc_msr.c b/arch/x86/kernel/tsc_msr.c index 4fec6f3a1858b2..83b54c65aad2f3 100644 --- a/arch/x86/kernel/tsc_msr.c +++ b/arch/x86/kernel/tsc_msr.c @@ -133,10 +133,15 @@ static const struct freq_desc freq_desc_ann = { .mask = 0x0f, }; -/* 24 MHz crystal? : 24 * 13 / 4 = 78 MHz */ +/* + * 24 MHz crystal? : 24 * 13 / 4 = 78 MHz + * Frequency step for Lightning Mountain SoC is fixed to 78 MHz, + * so all the frequency entries are 78000. + */ static const struct freq_desc freq_desc_lgm = { .use_msr_plat = true, - .freqs = { 78000, 78000, 78000, 78000, 78000, 78000, 78000, 78000 }, + .freqs = { 78000, 78000, 78000, 78000, 78000, 78000, 78000, 78000, 78000, + 78000, 78000, 78000, 78000, 78000, 78000, 78000 }, .mask = 0x0f, }; -- 2.11.0
Re: [PATCH 1/1] x86/tsr: Fix tsc frequency enumeration failure on lightning mountain SoC
On 7/30/2020 3:57 PM, Andy Shevchenko wrote: While at this, can you confirm (with maybe good description and documentation reference) that the numbers in that array are all correct? Sure, i will add the description. Regards, Dilip
[PATCH 1/1] x86/tsr: Fix tsc frequency enumeration failure on lightning mountain SoC
Frequency descriptor of Lightning Mountain SoC doesn't have all the frequency entries so resulting in the below failure causing kernel hang. [0.00] Error MSR_FSB_FREQ index 15 is unknown [0.00] tsc: Fast TSC calibration failed So, add all the frequency entries in the Lightning Mountain SoC frequency descriptor. Fixes: 0cc5359d8fd45 ("x86/cpu: Update init data for new Airmont CPU model") Fixes: 812c2d7506fd ("x86/tsc_msr: Use named struct initializers") Signed-off-by: Dilip Kota --- arch/x86/kernel/tsc_msr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/x86/kernel/tsc_msr.c b/arch/x86/kernel/tsc_msr.c index 4fec6f3a1858b2..c255e10e914aa5 100644 --- a/arch/x86/kernel/tsc_msr.c +++ b/arch/x86/kernel/tsc_msr.c @@ -136,7 +136,8 @@ static const struct freq_desc freq_desc_ann = { /* 24 MHz crystal? : 24 * 13 / 4 = 78 MHz */ static const struct freq_desc freq_desc_lgm = { .use_msr_plat = true, - .freqs = { 78000, 78000, 78000, 78000, 78000, 78000, 78000, 78000 }, + .freqs = { 78000, 78000, 78000, 78000, 78000, 78000, 78000, 78000, 78000, + 78000, 78000, 78000, 78000, 78000, 78000, 78000 }, .mask = 0x0f, }; -- 2.11.0
[PATCH v2 4/8] spi: lantiq: Add support to acknowledge interrupt
On newer chipsets interrupt need to be acknowledged as they use different interrupt controller which does not acknowledge the interrupts automatically. Signed-off-by: Dilip Kota --- drivers/spi/spi-lantiq-ssc.c | 13 + 1 file changed, 13 insertions(+) diff --git a/drivers/spi/spi-lantiq-ssc.c b/drivers/spi/spi-lantiq-ssc.c index 98e1c5d807597..cbe019f995999 100644 --- a/drivers/spi/spi-lantiq-ssc.c +++ b/drivers/spi/spi-lantiq-ssc.c @@ -161,6 +161,7 @@ struct lantiq_ssc_hwcfg { unsigned intirnen_t; unsigned intirncr; unsigned intirnicr; + boolirq_ack; }; struct lantiq_ssc_spi { @@ -623,9 +624,14 @@ static void rx_request(struct lantiq_ssc_spi *spi) static irqreturn_t lantiq_ssc_xmit_interrupt(int irq, void *data) { struct lantiq_ssc_spi *spi = data; + const struct lantiq_ssc_hwcfg *hwcfg = spi->hwcfg; + u32 val = lantiq_ssc_readl(spi, hwcfg->irncr); unsigned long flags; spin_lock_irqsave(>lock, flags); + if (hwcfg->irq_ack) + lantiq_ssc_writel(spi, val, hwcfg->irncr); + if (spi->tx) { if (spi->rx && spi->rx_todo) rx_fifo_read_full_duplex(spi); @@ -660,13 +666,18 @@ static irqreturn_t lantiq_ssc_xmit_interrupt(int irq, void *data) static irqreturn_t lantiq_ssc_err_interrupt(int irq, void *data) { struct lantiq_ssc_spi *spi = data; + const struct lantiq_ssc_hwcfg *hwcfg = spi->hwcfg; u32 stat = lantiq_ssc_readl(spi, LTQ_SPI_STAT); + u32 val = lantiq_ssc_readl(spi, hwcfg->irncr); unsigned long flags; if (!(stat & LTQ_SPI_STAT_ERRORS)) return IRQ_NONE; spin_lock_irqsave(>lock, flags); + if (hwcfg->irq_ack) + lantiq_ssc_writel(spi, val, hwcfg->irncr); + if (stat & LTQ_SPI_STAT_RUE) dev_err(spi->dev, "receive underflow error\n"); if (stat & LTQ_SPI_STAT_TUE) @@ -797,6 +808,7 @@ static const struct lantiq_ssc_hwcfg lantiq_ssc_xway = { .irnen_t= LTQ_SPI_IRNEN_T_XWAY, .irnicr = 0xF8, .irncr = 0xFC, + .irq_ack= false, }; static const struct lantiq_ssc_hwcfg lantiq_ssc_xrx = { @@ -804,6 +816,7 @@ static const struct lantiq_ssc_hwcfg lantiq_ssc_xrx = { .irnen_t= LTQ_SPI_IRNEN_T_XRX, .irnicr = 0xF8, .irncr = 0xFC, + .irq_ack= false, }; static const struct of_device_id lantiq_ssc_match[] = { -- 2.11.0
[PATCH v2 2/8] spi: lantiq: Add SMP support
Existing driver supports only single core SoC. New multicore platforms uses the same driver/IP so SMP support is required. This patch adds multicore support in the driver. Signed-off-by: Dilip Kota --- drivers/spi/spi-lantiq-ssc.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/spi/spi-lantiq-ssc.c b/drivers/spi/spi-lantiq-ssc.c index 44600fb71c484..1073a70a4beba 100644 --- a/drivers/spi/spi-lantiq-ssc.c +++ b/drivers/spi/spi-lantiq-ssc.c @@ -623,7 +623,9 @@ static void rx_request(struct lantiq_ssc_spi *spi) static irqreturn_t lantiq_ssc_xmit_interrupt(int irq, void *data) { struct lantiq_ssc_spi *spi = data; + unsigned long flags; + spin_lock_irqsave(>lock, flags); if (spi->tx) { if (spi->rx && spi->rx_todo) rx_fifo_read_full_duplex(spi); @@ -645,10 +647,12 @@ static irqreturn_t lantiq_ssc_xmit_interrupt(int irq, void *data) } } + spin_unlock_irqrestore(>lock, flags); return IRQ_HANDLED; completed: queue_work(spi->wq, >work); + spin_unlock_irqrestore(>lock, flags); return IRQ_HANDLED; } @@ -657,10 +661,12 @@ static irqreturn_t lantiq_ssc_err_interrupt(int irq, void *data) { struct lantiq_ssc_spi *spi = data; u32 stat = lantiq_ssc_readl(spi, LTQ_SPI_STAT); + unsigned long flags; if (!(stat & LTQ_SPI_STAT_ERRORS)) return IRQ_NONE; + spin_lock_irqsave(>lock, flags); if (stat & LTQ_SPI_STAT_RUE) dev_err(spi->dev, "receive underflow error\n"); if (stat & LTQ_SPI_STAT_TUE) @@ -681,6 +687,7 @@ static irqreturn_t lantiq_ssc_err_interrupt(int irq, void *data) if (spi->master->cur_msg) spi->master->cur_msg->status = -EIO; queue_work(spi->wq, >work); + spin_unlock_irqrestore(>lock, flags); return IRQ_HANDLED; } -- 2.11.0
[PATCH v2 1/8] spi: lantiq: fix: Rx overflow error in full duplex mode
In full duplex mode, rx overflow error is observed. To overcome the error, wait until the complete data got received and proceed further. Fixes: 17f84b793c01 ("spi: lantiq-ssc: add support for Lantiq SSC SPI controller") Signed-off-by: Dilip Kota --- drivers/spi/spi-lantiq-ssc.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/drivers/spi/spi-lantiq-ssc.c b/drivers/spi/spi-lantiq-ssc.c index 1fd7ee53d4510..44600fb71c484 100644 --- a/drivers/spi/spi-lantiq-ssc.c +++ b/drivers/spi/spi-lantiq-ssc.c @@ -184,6 +184,7 @@ struct lantiq_ssc_spi { unsigned inttx_fifo_size; unsigned intrx_fifo_size; unsigned intbase_cs; + unsigned intfdx_tx_level; }; static u32 lantiq_ssc_readl(const struct lantiq_ssc_spi *spi, u32 reg) @@ -481,6 +482,7 @@ static void tx_fifo_write(struct lantiq_ssc_spi *spi) u32 data; unsigned int tx_free = tx_fifo_free(spi); + spi->fdx_tx_level = 0; while (spi->tx_todo && tx_free) { switch (spi->bits_per_word) { case 2 ... 8: @@ -509,6 +511,7 @@ static void tx_fifo_write(struct lantiq_ssc_spi *spi) lantiq_ssc_writel(spi, data, LTQ_SPI_TB); tx_free--; + spi->fdx_tx_level++; } } @@ -520,6 +523,13 @@ static void rx_fifo_read_full_duplex(struct lantiq_ssc_spi *spi) u32 data; unsigned int rx_fill = rx_fifo_level(spi); + /* +* Wait until all expected data to be shifted in. +* Otherwise, rx overrun may occur. +*/ + while (rx_fill != spi->fdx_tx_level) + rx_fill = rx_fifo_level(spi); + while (rx_fill) { data = lantiq_ssc_readl(spi, LTQ_SPI_RB); -- 2.11.0
[PATCH v2 7/8] dt-bindings: spi: Add support to Lightning Mountain SoC
Add support to SPI controller on Intel Atom based Lightning Mountain SoC which reuses the Lantiq SPI controller IP. Signed-off-by: Dilip Kota Reviewed-by: Rob Herring --- .../devicetree/bindings/spi/spi-lantiq-ssc.txt | 21 +++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/spi/spi-lantiq-ssc.txt b/Documentation/devicetree/bindings/spi/spi-lantiq-ssc.txt index ce3230c8e28dc..76a3dd35f7960 100644 --- a/Documentation/devicetree/bindings/spi/spi-lantiq-ssc.txt +++ b/Documentation/devicetree/bindings/spi/spi-lantiq-ssc.txt @@ -1,11 +1,17 @@ Lantiq Synchronous Serial Controller (SSC) SPI master driver Required properties: -- compatible: "lantiq,ase-spi", "lantiq,falcon-spi", "lantiq,xrx100-spi" +- compatible: "lantiq,ase-spi", "lantiq,falcon-spi", "lantiq,xrx100-spi", + "intel,lgm-spi" - #address-cells: see spi-bus.txt - #size-cells: see spi-bus.txt - reg: address and length of the spi master registers -- interrupts: should contain the "spi_rx", "spi_tx" and "spi_err" interrupt. +- interrupts: + For compatible "intel,lgm-ssc" - the common interrupt number for + all of tx rx & err interrupts. + or + For rest of the compatibles, should contain the "spi_rx", "spi_tx" and + "spi_err" interrupt. Optional properties: @@ -27,3 +33,14 @@ spi: spi@e100800 { num-cs = <6>; base-cs = <1>; }; + +ssc0: spi@e080 { + compatible = "intel,lgm-spi"; + reg = <0xe080 0x400>; + interrupt-parent = <>; + interrupts = <35 1>; + #address-cells = <1>; + #size-cells = <0>; + clocks = < LGM_CLK_NGI>, < LGM_GCLK_SSC0>; + clock-names = "freq", "gate"; +}; -- 2.11.0
[PATCH v2 3/8] spi: lantiq: Move interrupt control register offesets to SoC specific data structure
Address of Interrupt control registers are different on new chipsets. So move them to SoC specific data structure. Signed-off-by: Dilip Kota --- drivers/spi/spi-lantiq-ssc.c | 20 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/drivers/spi/spi-lantiq-ssc.c b/drivers/spi/spi-lantiq-ssc.c index 1073a70a4beba..98e1c5d807597 100644 --- a/drivers/spi/spi-lantiq-ssc.c +++ b/drivers/spi/spi-lantiq-ssc.c @@ -50,8 +50,6 @@ #define LTQ_SPI_RXCNT 0x84 #define LTQ_SPI_DMACON 0xec #define LTQ_SPI_IRNEN 0xf4 -#define LTQ_SPI_IRNICR 0xf8 -#define LTQ_SPI_IRNCR 0xfc #define LTQ_SPI_CLC_SMC_S 16 /* Clock divider for sleep mode */ #define LTQ_SPI_CLC_SMC_M (0xFF << LTQ_SPI_CLC_SMC_S) @@ -159,8 +157,10 @@ #define LTQ_SPI_IRNEN_ALL 0x1F struct lantiq_ssc_hwcfg { - unsigned int irnen_r; - unsigned int irnen_t; + unsigned intirnen_r; + unsigned intirnen_t; + unsigned intirncr; + unsigned intirnicr; }; struct lantiq_ssc_spi { @@ -793,13 +793,17 @@ static int lantiq_ssc_transfer_one(struct spi_master *master, } static const struct lantiq_ssc_hwcfg lantiq_ssc_xway = { - .irnen_r = LTQ_SPI_IRNEN_R_XWAY, - .irnen_t = LTQ_SPI_IRNEN_T_XWAY, + .irnen_r= LTQ_SPI_IRNEN_R_XWAY, + .irnen_t= LTQ_SPI_IRNEN_T_XWAY, + .irnicr = 0xF8, + .irncr = 0xFC, }; static const struct lantiq_ssc_hwcfg lantiq_ssc_xrx = { - .irnen_r = LTQ_SPI_IRNEN_R_XRX, - .irnen_t = LTQ_SPI_IRNEN_T_XRX, + .irnen_r= LTQ_SPI_IRNEN_R_XRX, + .irnen_t= LTQ_SPI_IRNEN_T_XRX, + .irnicr = 0xF8, + .irncr = 0xFC, }; static const struct of_device_id lantiq_ssc_match[] = { -- 2.11.0
[PATCH v2 6/8] spi: lantiq: Move interrupt configuration to SoC specific data structure
Moving interrupt configuration to SoC specific data structure helps to add support for newer SoCs on which SPI controller with lesser interrupt lines compared to existing chipsets. Signed-off-by: Dilip Kota --- drivers/spi/spi-lantiq-ssc.c | 64 +++- 1 file changed, 39 insertions(+), 25 deletions(-) diff --git a/drivers/spi/spi-lantiq-ssc.c b/drivers/spi/spi-lantiq-ssc.c index a0d1f82d309f9..2a433d9b5d8fe 100644 --- a/drivers/spi/spi-lantiq-ssc.c +++ b/drivers/spi/spi-lantiq-ssc.c @@ -150,7 +150,10 @@ #define LTQ_SPI_IRNEN_T_XRXBIT(0) /* Receive end interrupt request */ #define LTQ_SPI_IRNEN_ALL 0x1F +struct lantiq_ssc_spi; + struct lantiq_ssc_hwcfg { + int (*cfg_irq)(struct platform_device *pdev, struct lantiq_ssc_spi *spi); unsigned intirnen_r; unsigned intirnen_t; unsigned intirncr; @@ -800,7 +803,40 @@ static int lantiq_ssc_transfer_one(struct spi_master *master, return transfer_start(spi, spidev, t); } +static int lantiq_cfg_irq(struct platform_device *pdev, struct lantiq_ssc_spi *spi) +{ + int irq, err; + + irq = platform_get_irq_byname(pdev, LTQ_SPI_RX_IRQ_NAME); + if (irq < 0) + return irq; + + err = devm_request_irq(>dev, irq, lantiq_ssc_xmit_interrupt, + 0, LTQ_SPI_RX_IRQ_NAME, spi); + if (err) + return err; + + irq = platform_get_irq_byname(pdev, LTQ_SPI_TX_IRQ_NAME); + if (irq < 0) + return irq; + + err = devm_request_irq(>dev, irq, lantiq_ssc_xmit_interrupt, + 0, LTQ_SPI_TX_IRQ_NAME, spi); + + if (err) + return err; + + irq = platform_get_irq_byname(pdev, LTQ_SPI_ERR_IRQ_NAME); + if (irq < 0) + return irq; + + err = devm_request_irq(>dev, irq, lantiq_ssc_err_interrupt, + 0, LTQ_SPI_ERR_IRQ_NAME, spi); + return err; +} + static const struct lantiq_ssc_hwcfg lantiq_ssc_xway = { + .cfg_irq= lantiq_cfg_irq, .irnen_r= LTQ_SPI_IRNEN_R_XWAY, .irnen_t= LTQ_SPI_IRNEN_T_XWAY, .irnicr = 0xF8, @@ -810,6 +846,7 @@ static const struct lantiq_ssc_hwcfg lantiq_ssc_xway = { }; static const struct lantiq_ssc_hwcfg lantiq_ssc_xrx = { + .cfg_irq= lantiq_cfg_irq, .irnen_r= LTQ_SPI_IRNEN_R_XRX, .irnen_t= LTQ_SPI_IRNEN_T_XRX, .irnicr = 0xF8, @@ -833,9 +870,9 @@ static int lantiq_ssc_probe(struct platform_device *pdev) struct lantiq_ssc_spi *spi; const struct lantiq_ssc_hwcfg *hwcfg; const struct of_device_id *match; - int err, rx_irq, tx_irq, err_irq; u32 id, supports_dma, revision; unsigned int num_cs; + int err; match = of_match_device(lantiq_ssc_match, dev); if (!match) { @@ -844,18 +881,6 @@ static int lantiq_ssc_probe(struct platform_device *pdev) } hwcfg = match->data; - rx_irq = platform_get_irq_byname(pdev, LTQ_SPI_RX_IRQ_NAME); - if (rx_irq < 0) - return -ENXIO; - - tx_irq = platform_get_irq_byname(pdev, LTQ_SPI_TX_IRQ_NAME); - if (tx_irq < 0) - return -ENXIO; - - err_irq = platform_get_irq_byname(pdev, LTQ_SPI_ERR_IRQ_NAME); - if (err_irq < 0) - return -ENXIO; - master = spi_alloc_master(dev, sizeof(struct lantiq_ssc_spi)); if (!master) return -ENOMEM; @@ -871,18 +896,7 @@ static int lantiq_ssc_probe(struct platform_device *pdev) goto err_master_put; } - err = devm_request_irq(dev, rx_irq, lantiq_ssc_xmit_interrupt, - 0, LTQ_SPI_RX_IRQ_NAME, spi); - if (err) - goto err_master_put; - - err = devm_request_irq(dev, tx_irq, lantiq_ssc_xmit_interrupt, - 0, LTQ_SPI_TX_IRQ_NAME, spi); - if (err) - goto err_master_put; - - err = devm_request_irq(dev, err_irq, lantiq_ssc_err_interrupt, - 0, LTQ_SPI_ERR_IRQ_NAME, spi); + err = hwcfg->cfg_irq(pdev, spi); if (err) goto err_master_put; -- 2.11.0
[PATCH v2 8/8] spi: lantiq: Add support to Lightning Mountain SoC
Add support to SPI controller on Intel Atom based Lightning Mountain SoC which reuses Lantiq SPI controller IP. Signed-off-by: Dilip Kota --- drivers/spi/Kconfig | 4 ++-- drivers/spi/spi-lantiq-ssc.c | 40 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index 878849a33781b..be40310840d04 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -498,11 +498,11 @@ config SPI_NPCM_PSPI config SPI_LANTIQ_SSC tristate "Lantiq SSC SPI controller" - depends on LANTIQ || COMPILE_TEST + depends on LANTIQ || X86 || COMPILE_TEST help This driver supports the Lantiq SSC SPI controller in master mode. This controller is found on Intel (former Lantiq) SoCs like - the Danube, Falcon, xRX200, xRX300. + the Danube, Falcon, xRX200, xRX300, Lightning Mountain. config SPI_OC_TINY tristate "OpenCores tiny SPI" diff --git a/drivers/spi/spi-lantiq-ssc.c b/drivers/spi/spi-lantiq-ssc.c index 2a433d9b5d8fe..81cb1c06e2ce5 100644 --- a/drivers/spi/spi-lantiq-ssc.c +++ b/drivers/spi/spi-lantiq-ssc.c @@ -703,6 +703,24 @@ static irqreturn_t lantiq_ssc_err_interrupt(int irq, void *data) return IRQ_HANDLED; } +static irqreturn_t intel_lgm_ssc_isr(int irq, void *data) +{ + struct lantiq_ssc_spi *spi = data; + const struct lantiq_ssc_hwcfg *hwcfg = spi->hwcfg; + u32 val = lantiq_ssc_readl(spi, hwcfg->irncr); + + if (!(val & LTQ_SPI_IRNEN_ALL)) + return IRQ_NONE; + + if (val & LTQ_SPI_IRNEN_E) + return lantiq_ssc_err_interrupt(irq, data); + + if ((val & hwcfg->irnen_t) || (val & hwcfg->irnen_r)) + return lantiq_ssc_xmit_interrupt(irq, data); + + return IRQ_HANDLED; +} + static int transfer_start(struct lantiq_ssc_spi *spi, struct spi_device *spidev, struct spi_transfer *t) { @@ -803,6 +821,17 @@ static int lantiq_ssc_transfer_one(struct spi_master *master, return transfer_start(spi, spidev, t); } +static int intel_lgm_cfg_irq(struct platform_device *pdev, struct lantiq_ssc_spi *spi) +{ + int irq; + + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return irq; + + return devm_request_irq(>dev, irq, intel_lgm_ssc_isr, 0, "spi", spi); +} + static int lantiq_cfg_irq(struct platform_device *pdev, struct lantiq_ssc_spi *spi) { int irq, err; @@ -855,10 +884,21 @@ static const struct lantiq_ssc_hwcfg lantiq_ssc_xrx = { .irq_ack= false, }; +static const struct lantiq_ssc_hwcfg intel_ssc_lgm = { + .cfg_irq= intel_lgm_cfg_irq, + .irnen_r= LTQ_SPI_IRNEN_R_XRX, + .irnen_t= LTQ_SPI_IRNEN_T_XRX, + .irnicr = 0xFC, + .irncr = 0xF8, + .fifo_size_mask = GENMASK(7, 0), + .irq_ack= true, +}; + static const struct of_device_id lantiq_ssc_match[] = { { .compatible = "lantiq,ase-spi", .data = _ssc_xway, }, { .compatible = "lantiq,falcon-spi", .data = _ssc_xrx, }, { .compatible = "lantiq,xrx100-spi", .data = _ssc_xrx, }, + { .compatible = "intel,lgm-spi", .data = _ssc_lgm, }, {}, }; MODULE_DEVICE_TABLE(of, lantiq_ssc_match); -- 2.11.0
[PATCH v2 5/8] spi: lantiq: Add fifo size bit mask in SoC specific data structure
On newer chipsets, SPI controller has fifos of larger size. So add the fifo size bit mask entry in SoC specific data structure. Signed-off-by: Dilip Kota --- drivers/spi/spi-lantiq-ssc.c | 19 +-- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/drivers/spi/spi-lantiq-ssc.c b/drivers/spi/spi-lantiq-ssc.c index cbe019f995999..a0d1f82d309f9 100644 --- a/drivers/spi/spi-lantiq-ssc.c +++ b/drivers/spi/spi-lantiq-ssc.c @@ -59,9 +59,7 @@ #define LTQ_SPI_CLC_DISR BIT(0) /* Disable request bit */ #define LTQ_SPI_ID_TXFS_S 24 /* Implemented TX FIFO size */ -#define LTQ_SPI_ID_TXFS_M (0x3F << LTQ_SPI_ID_TXFS_S) #define LTQ_SPI_ID_RXFS_S 16 /* Implemented RX FIFO size */ -#define LTQ_SPI_ID_RXFS_M (0x3F << LTQ_SPI_ID_RXFS_S) #define LTQ_SPI_ID_MOD_S 8 /* Module ID */ #define LTQ_SPI_ID_MOD_M (0xff << LTQ_SPI_ID_MOD_S) #define LTQ_SPI_ID_CFG_S 5 /* DMA interface support */ @@ -124,19 +122,15 @@ LTQ_SPI_WHBSTATE_CLRTUE) #define LTQ_SPI_RXFCON_RXFITL_S8 /* FIFO interrupt trigger level */ -#define LTQ_SPI_RXFCON_RXFITL_M(0x3F << LTQ_SPI_RXFCON_RXFITL_S) #define LTQ_SPI_RXFCON_RXFLU BIT(1) /* FIFO flush */ #define LTQ_SPI_RXFCON_RXFEN BIT(0) /* FIFO enable */ #define LTQ_SPI_TXFCON_TXFITL_S8 /* FIFO interrupt trigger level */ -#define LTQ_SPI_TXFCON_TXFITL_M(0x3F << LTQ_SPI_TXFCON_TXFITL_S) #define LTQ_SPI_TXFCON_TXFLU BIT(1) /* FIFO flush */ #define LTQ_SPI_TXFCON_TXFEN BIT(0) /* FIFO enable */ #define LTQ_SPI_FSTAT_RXFFL_S 0 -#define LTQ_SPI_FSTAT_RXFFL_M (0x3f << LTQ_SPI_FSTAT_RXFFL_S) #define LTQ_SPI_FSTAT_TXFFL_S 8 -#define LTQ_SPI_FSTAT_TXFFL_M (0x3f << LTQ_SPI_FSTAT_TXFFL_S) #define LTQ_SPI_GPOCON_ISCSBN_S8 #define LTQ_SPI_GPOCON_INVOUTN_S 0 @@ -162,6 +156,7 @@ struct lantiq_ssc_hwcfg { unsigned intirncr; unsigned intirnicr; boolirq_ack; + u32 fifo_size_mask; }; struct lantiq_ssc_spi { @@ -211,16 +206,18 @@ static void lantiq_ssc_maskl(const struct lantiq_ssc_spi *spi, u32 clr, static unsigned int tx_fifo_level(const struct lantiq_ssc_spi *spi) { + const struct lantiq_ssc_hwcfg *hwcfg = spi->hwcfg; u32 fstat = lantiq_ssc_readl(spi, LTQ_SPI_FSTAT); - return (fstat & LTQ_SPI_FSTAT_TXFFL_M) >> LTQ_SPI_FSTAT_TXFFL_S; + return (fstat >> LTQ_SPI_FSTAT_TXFFL_S) & hwcfg->fifo_size_mask; } static unsigned int rx_fifo_level(const struct lantiq_ssc_spi *spi) { + const struct lantiq_ssc_hwcfg *hwcfg = spi->hwcfg; u32 fstat = lantiq_ssc_readl(spi, LTQ_SPI_FSTAT); - return fstat & LTQ_SPI_FSTAT_RXFFL_M; + return (fstat >> LTQ_SPI_FSTAT_RXFFL_S) & hwcfg->fifo_size_mask; } static unsigned int tx_fifo_free(const struct lantiq_ssc_spi *spi) @@ -808,6 +805,7 @@ static const struct lantiq_ssc_hwcfg lantiq_ssc_xway = { .irnen_t= LTQ_SPI_IRNEN_T_XWAY, .irnicr = 0xF8, .irncr = 0xFC, + .fifo_size_mask = GENMASK(5, 0), .irq_ack= false, }; @@ -816,6 +814,7 @@ static const struct lantiq_ssc_hwcfg lantiq_ssc_xrx = { .irnen_t= LTQ_SPI_IRNEN_T_XRX, .irnicr = 0xF8, .irncr = 0xFC, + .fifo_size_mask = GENMASK(5, 0), .irq_ack= false, }; @@ -941,8 +940,8 @@ static int lantiq_ssc_probe(struct platform_device *pdev) INIT_WORK(>work, lantiq_ssc_bussy_work); id = lantiq_ssc_readl(spi, LTQ_SPI_ID); - spi->tx_fifo_size = (id & LTQ_SPI_ID_TXFS_M) >> LTQ_SPI_ID_TXFS_S; - spi->rx_fifo_size = (id & LTQ_SPI_ID_RXFS_M) >> LTQ_SPI_ID_RXFS_S; + spi->tx_fifo_size = (id >> LTQ_SPI_ID_TXFS_S) & hwcfg->fifo_size_mask; + spi->rx_fifo_size = (id >> LTQ_SPI_ID_RXFS_S) & hwcfg->fifo_size_mask; supports_dma = (id & LTQ_SPI_ID_CFG_M) >> LTQ_SPI_ID_CFG_S; revision = id & LTQ_SPI_ID_REV_M; -- 2.11.0
Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers
On 5/6/2020 3:40 PM, Dilip Kota wrote: On 5/5/2020 7:23 PM, Mark Brown wrote: On Mon, May 04, 2020 at 06:15:47PM +0800, Dilip Kota wrote: On 4/29/2020 8:13 PM, Mark Brown wrote: I just tried to get the history of removing workqueue in SPI driver, on GRX500 (earlier chipset of LGM) the SPI transfers got timedout with workqueues during regression testing. Once changed to threaded IRQs transfers are working successfully. That doesn't really explain why though, it just explains what. I didnt find more information about it. I will work to reproduce the issue and share the detailed information sooner i get the accessibility of the SoC (because of covid19 doing wfh) I got the GRX500 setup and reproduced the timeout issue. [ 88.721883] spi-loopback-test spi1.2: SPI transfer timed out [ 88.726488] spi-loopback-test spi1.2: spi-message timed out - reruning... [ 88.961786] spi-loopback-test spi1.2: SPI transfer timed out [ 88.966027] spi-loopback-test spi1.2: Failed to execute spi_message: -145 Timeout is happening because of not acknowledging or not clearing the interrupt status registers. Workqueue is not causing any issue, I am working on the changes, will submit the patches for review. Regards, Dilip Regards, Dilip
Re: [PATCH 1/2] dt-bindings: watchdog: intel: Add YAML Schemas for Watchdog timer
On 6/10/2020 9:05 PM, Guenter Roeck wrote: On 6/10/20 12:54 AM, Dilip Kota wrote: On 6/9/2020 9:46 PM, Guenter Roeck wrote: On 6/9/20 1:57 AM, Dilip Kota wrote: On 6/8/2020 9:37 PM, Guenter Roeck wrote: On 6/7/20 10:49 PM, Dilip Kota wrote: [...] + +description: | + Intel Lightning Mountain SoC has General Purpose Timer Counter(GPTC) which can + be configured as Clocksource, real time clock and Watchdog timer. + Each General Purpose Timer Counter has three timers. And total four General + Purpose Timer Counters are present on Lightning Mountain SoC which sums up + to 12 timers. + Lightning Mountain has four CPUs and each CPU is configured with one GPTC + timer as watchdog timer. Total four timers are configured as watchdog timers + on Lightning Mountain SoC. + Why not just one ? The watchdog subsystem does not monitor individual CPUs, it monitors the system. Intel Atom based Lightning Mountain SoC, system has four CPUs. On Lightning Mountain SoC ,Watchdog subsystem is combination of GPTC timers and reset controller unit. On Lightning Mountain SoC, each CPU is configured with one GPTC timer, so that if any of the CPU hangs or freezes, the watchdog daemon running on respective CPU cannot reset/ping or pet the watchdog timer. This causes the watchdog timeout. On watchdog timeout, reset controller triggers the reset to respective CPU. A system watchdog driver should not duplicate functionality from kernel/watchdog.c, which monitors individual CPUs. If the SoC does nto provide a system watchdog timer (which I think is unlikely), it should stick with that. A watchdog resetting an individual CPU instead of the entire system isn't something I would want to see in the watchdog subsystem. My bad here, complete hardware reset happens on watchdog timeout not a single CPU or core. Could you please clarify: The complete system means, you mean, "a watchdog susbsystem should monitor all the cores/cpus in the SoC. Not like each core/cpu in SoC having a wdt". No, the watchdog subsystem does not monitor "all cores". Again, that is the responsibility of kernel/watchdog.c. I am a bit confused here. I have gone through the kernel/watchdog.c code and i see hrtimers are used and panic is triggered for lockup on CPU/core. It looks similar to the watchdog subsystem which uses wdt and triggers hardware reset on timeout, whereas kernel/watchdog.c using hrtimers and triggers panic on timeout. To my understanding Watchdog timer recovers the hardware from software hangs or freeze states on the CPU / cores. Also, what does system mean in your statement " watchdog subsystem monitors the system"? What all comes under the system other than the cores/cpus. And also i see there is no other watchdog subsystem in Lightning Mountain architecture. Regards, Dilip Guenter Regards, Dilip Guenter
Re: [PATCH 1/2] dt-bindings: watchdog: intel: Add YAML Schemas for Watchdog timer
On 6/9/2020 9:46 PM, Guenter Roeck wrote: On 6/9/20 1:57 AM, Dilip Kota wrote: On 6/8/2020 9:37 PM, Guenter Roeck wrote: On 6/7/20 10:49 PM, Dilip Kota wrote: Add YAML schemas for the watchdog timer on Intel Lightning Mountain SoC. Signed-off-by: Dilip Kota --- .../bindings/watchdog/intel,lgm-gptc-wdt.yaml | 75 ++ 1 file changed, 75 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/intel,lgm-gptc-wdt.yaml diff --git a/Documentation/devicetree/bindings/watchdog/intel,lgm-gptc-wdt.yaml b/Documentation/devicetree/bindings/watchdog/intel,lgm-gptc-wdt.yaml new file mode 100644 index 0..83dc39a5090c1 --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/intel,lgm-gptc-wdt.yaml @@ -0,0 +1,75 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/watchdog/intel,lgm-gptc-wdt.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Intel Lightning Mountain Watchdog timer. + +maintainers: + - Dilip Kota + +description: | + Intel Lightning Mountain SoC has General Purpose Timer Counter(GPTC) which can + be configured as Clocksource, real time clock and Watchdog timer. + Each General Purpose Timer Counter has three timers. And total four General + Purpose Timer Counters are present on Lightning Mountain SoC which sums up + to 12 timers. + Lightning Mountain has four CPUs and each CPU is configured with one GPTC + timer as watchdog timer. Total four timers are configured as watchdog timers + on Lightning Mountain SoC. + Why not just one ? The watchdog subsystem does not monitor individual CPUs, it monitors the system. Intel Atom based Lightning Mountain SoC, system has four CPUs. On Lightning Mountain SoC ,Watchdog subsystem is combination of GPTC timers and reset controller unit. On Lightning Mountain SoC, each CPU is configured with one GPTC timer, so that if any of the CPU hangs or freezes, the watchdog daemon running on respective CPU cannot reset/ping or pet the watchdog timer. This causes the watchdog timeout. On watchdog timeout, reset controller triggers the reset to respective CPU. A system watchdog driver should not duplicate functionality from kernel/watchdog.c, which monitors individual CPUs. If the SoC does nto provide a system watchdog timer (which I think is unlikely), it should stick with that. A watchdog resetting an individual CPU instead of the entire system isn't something I would want to see in the watchdog subsystem. My bad here, complete hardware reset happens on watchdog timeout not a single CPU or core. Could you please clarify: The complete system means, you mean, "a watchdog susbsystem should monitor all the cores/cpus in the SoC. Not like each core/cpu in SoC having a wdt". Regards, Dilip Guenter
Re: [PATCH 1/2] dt-bindings: watchdog: intel: Add YAML Schemas for Watchdog timer
On 6/8/2020 9:37 PM, Guenter Roeck wrote: On 6/7/20 10:49 PM, Dilip Kota wrote: Add YAML schemas for the watchdog timer on Intel Lightning Mountain SoC. Signed-off-by: Dilip Kota --- .../bindings/watchdog/intel,lgm-gptc-wdt.yaml | 75 ++ 1 file changed, 75 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/intel,lgm-gptc-wdt.yaml diff --git a/Documentation/devicetree/bindings/watchdog/intel,lgm-gptc-wdt.yaml b/Documentation/devicetree/bindings/watchdog/intel,lgm-gptc-wdt.yaml new file mode 100644 index 0..83dc39a5090c1 --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/intel,lgm-gptc-wdt.yaml @@ -0,0 +1,75 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/watchdog/intel,lgm-gptc-wdt.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Intel Lightning Mountain Watchdog timer. + +maintainers: + - Dilip Kota + +description: | + Intel Lightning Mountain SoC has General Purpose Timer Counter(GPTC) which can + be configured as Clocksource, real time clock and Watchdog timer. + Each General Purpose Timer Counter has three timers. And total four General + Purpose Timer Counters are present on Lightning Mountain SoC which sums up + to 12 timers. + Lightning Mountain has four CPUs and each CPU is configured with one GPTC + timer as watchdog timer. Total four timers are configured as watchdog timers + on Lightning Mountain SoC. + Why not just one ? The watchdog subsystem does not monitor individual CPUs, it monitors the system. Intel Atom based Lightning Mountain SoC, system has four CPUs. On Lightning Mountain SoC ,Watchdog subsystem is combination of GPTC timers and reset controller unit. On Lightning Mountain SoC, each CPU is configured with one GPTC timer, so that if any of the CPU hangs or freezes, the watchdog daemon running on respective CPU cannot reset/ping or pet the watchdog timer. This causes the watchdog timeout. On watchdog timeout, reset controller triggers the reset to respective CPU. ->| | | >| Reset controller unit | | | |___| | | | | __|__|__ | GPTC | | | | ___ |_ __| | | | timer 1 | | timer 2 | | timer 3 | | | |_| |_| |_| | |__| Regards, -Dilip Guenter
Re: [PATCH 2/2] watchdog: intel: Watchdog timer support on Lightning Mountain
On 6/8/2020 9:36 PM, Guenter Roeck wrote: On 6/7/20 10:49 PM, Dilip Kota wrote: On Intel Lightning Mountain SoC, General Purpose Timer Counter(GPTC) programmable as clocksource, real time clock or watchdog timer. This driver configures GPTC as Watchdog timer and triggers reset signal to CPU on timeout. Signed-off-by: Dilip Kota --- drivers/watchdog/Kconfig | 13 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/intel_lgm_gptc_wdt.c | 420 ++ 3 files changed, 434 insertions(+) create mode 100644 drivers/watchdog/intel_lgm_gptc_wdt.c diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 0663c604bd642..8009c11e75dda 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -1789,6 +1789,19 @@ config IMGPDC_WDT To compile this driver as a loadable module, choose M here. The module will be called imgpdc_wdt. +config INTEL_LGM_GPTC_WDT + tristate "INTEL LGM SoC Watchdog" + depends on X86 || COMPILE_TEST + depends on OF && HAS_IOMEM + select REGMAP + select MFD_SYSCON + select WATCHDOG_CORE + help + Driver for Watchdog Timer on Intel Lightning Mountain SoC. + + To compile this driver as a loadable module, choose M here. + The module will be called intel_lgm_gptc_wdt. + config LANTIQ_WDT tristate "Lantiq SoC watchdog" depends on LANTIQ diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 6de2e4ceef190..92c99e4c46eb7 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -166,6 +166,7 @@ obj-$(CONFIG_TXX9_WDT) += txx9wdt.o obj-$(CONFIG_OCTEON_WDT) += octeon-wdt.o octeon-wdt-y := octeon-wdt-main.o octeon-wdt-nmi.o obj-$(CONFIG_LANTIQ_WDT) += lantiq_wdt.o +obj-$(CONFIG_INTEL_LGM_GPTC_WDT) += intel_lgm_gptc_wdt.o obj-$(CONFIG_LOONGSON1_WDT) += loongson1_wdt.o obj-$(CONFIG_RALINK_WDT) += rt2880_wdt.o obj-$(CONFIG_IMGPDC_WDT) += imgpdc_wdt.o diff --git a/drivers/watchdog/intel_lgm_gptc_wdt.c b/drivers/watchdog/intel_lgm_gptc_wdt.c new file mode 100644 index 0..52be7cc194f8f --- /dev/null +++ b/drivers/watchdog/intel_lgm_gptc_wdt.c @@ -0,0 +1,420 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2020 Intel Corporation. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define GPTC_CLC 0x00 +#define GPTC_CLC_SUSPEND BIT(4) +#define GPTC_CLC_RMC GENMASK(15, 8) + +/* divider 10 to produce 200 / 10 = 20 MHz clock */ +#define CLC_RMC_DIV10 + +#define GPTC_CON(X)(0x10 + (X) * 0x20) +#define GPTC_CON_CNT_UPBIT(1) +#define GPTC_CON_ONESHOT BIT(3) +#define GPTC_CON_EXT BIT(4) + +#define GPTC_RUN(X)(0x18 + (X) * 0x20) +#define GPTC_RUN_ENBIT(0) +#define GPTC_RUN_STOP BIT(1) +#define GPTC_RUN_RELOADBIT(2) + +#define GPTC_RLD(X)(0x20 + (X) * 0x20) +#define GPTC_CNT(X)(0x28 + (X) * 0x20) + +#define GPTC_IRNENCLR 0xF0 +#define GPTC_IRNEN 0xF4 +#define GPTC_IRNCR 0xFC + +/* Watchdog Timeout Reset register offset and bitfeilds */ +#define BIA_WDT_RST_EN 0x1E0 +#define BIA_WDTBIT(6) + +#define MAX_TIMERID2 +#define MAX_CPUID 3 +#define TIMER_MARGIN_SEC 300 + +static bool nowayout = WATCHDOG_NOWAYOUT; +module_param(nowayout, bool, 0); +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started\n" + " (default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); + +struct lgm_gptc_timer { + struct lgm_gptc_wdt *wdt_node; + struct watchdog_device wdd; + unsigned inttid; + unsigned intcpuid; + unsigned intfrequency; + unsigned intcycles; + boolenable; +}; + +struct lgm_gptc_wdt { + struct device *dev; + void __iomem*gptc_base; + struct regmap *rst_hndl; + struct clk *freqclk; + struct clk *gateclk; + unsigned intfpifreq; + enum cpuhp_statestate; +}; + +DEFINE_PER_CPU(struct lgm_gptc_timer, lgm_timer_per_cpu); + This is unusual. You'll have to provide a very detailed explanation why this is needed. Sure will add it. It is required for the hotplug cpu support, and hotplug cpu is added because, the cpus on Lightning Mountain SoC can be online and offline dynamically. If CPUs come to online after the watchdog driver probe, hotplug CPU support helps to configure watchdog timer once CPU is online. Regards, -Dilip Guenter
[PATCH 0/2] Driver for watchdog timer on Intel Lightning Mountain SoC
This patch series adds watchdog timer driver and respective yaml schemas for watchdog timer on Intel Lightning Mountain SoC. This patch series is rebased and tested on mainline linux kernel 5.7: base-commit: 3d77e6a8804a ("Linux 5.7") tags: v5.7 Dilip Kota (2): dt-bindings: watchdog: intel: Add YAML Schemas for Watchdog timer watchdog: intel: Watchdog timer support on Lightning Mountain .../bindings/watchdog/intel,lgm-gptc-wdt.yaml | 75 drivers/watchdog/Kconfig | 13 + drivers/watchdog/Makefile | 1 + drivers/watchdog/intel_lgm_gptc_wdt.c | 420 + 4 files changed, 509 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/intel,lgm-gptc-wdt.yaml create mode 100644 drivers/watchdog/intel_lgm_gptc_wdt.c -- 2.11.0
[PATCH 1/2] dt-bindings: watchdog: intel: Add YAML Schemas for Watchdog timer
Add YAML schemas for the watchdog timer on Intel Lightning Mountain SoC. Signed-off-by: Dilip Kota --- .../bindings/watchdog/intel,lgm-gptc-wdt.yaml | 75 ++ 1 file changed, 75 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/intel,lgm-gptc-wdt.yaml diff --git a/Documentation/devicetree/bindings/watchdog/intel,lgm-gptc-wdt.yaml b/Documentation/devicetree/bindings/watchdog/intel,lgm-gptc-wdt.yaml new file mode 100644 index 0..83dc39a5090c1 --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/intel,lgm-gptc-wdt.yaml @@ -0,0 +1,75 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/watchdog/intel,lgm-gptc-wdt.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Intel Lightning Mountain Watchdog timer. + +maintainers: + - Dilip Kota + +description: | + Intel Lightning Mountain SoC has General Purpose Timer Counter(GPTC) which can + be configured as Clocksource, real time clock and Watchdog timer. + Each General Purpose Timer Counter has three timers. And total four General + Purpose Timer Counters are present on Lightning Mountain SoC which sums up + to 12 timers. + Lightning Mountain has four CPUs and each CPU is configured with one GPTC + timer as watchdog timer. Total four timers are configured as watchdog timers + on Lightning Mountain SoC. + +allOf: + - $ref: "watchdog.yaml#" + +properties: + compatible: +enum: + - intel,lgm-gptc-wdt + + reg: +maxItems: 1 + + clocks: +items: + - description: Frequency clock + - description: Core clock + + clock-names: +items: + - const: freq + - const: gptc + + intel,wdt-rst-hndl: +$ref: /schemas/types.yaml#/definitions/phandle +description: Watchdog timer registers handle + + intel,timer-cfg: +description: Watchdog Timer id and CPU id +allOf: + - $ref: /schemas/types.yaml#/definitions/uint32-array + - minItems: 2 +maxItems: 4 +items: + minimum: 0 + maximum: 3 + +required: + - compatible + - reg + - clocks + - clock-names + - intel,wdt-rst-hndl + - intel,timer-cfg + +examples: + - | +watchdog@e250 { + compatible = "intel,lgm-gptc-wdt"; + reg = <0xe250 0x1>; + intel,wdt-rst-hndl = <>; + clocks = < 31>, < 136>; + clock-names = "freq", "gptc"; + timeout-sec = <30>; + intel,timer-cfg = <1 0 2 1>; +}; -- 2.11.0
[PATCH 2/2] watchdog: intel: Watchdog timer support on Lightning Mountain
On Intel Lightning Mountain SoC, General Purpose Timer Counter(GPTC) programmable as clocksource, real time clock or watchdog timer. This driver configures GPTC as Watchdog timer and triggers reset signal to CPU on timeout. Signed-off-by: Dilip Kota --- drivers/watchdog/Kconfig | 13 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/intel_lgm_gptc_wdt.c | 420 ++ 3 files changed, 434 insertions(+) create mode 100644 drivers/watchdog/intel_lgm_gptc_wdt.c diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 0663c604bd642..8009c11e75dda 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -1789,6 +1789,19 @@ config IMGPDC_WDT To compile this driver as a loadable module, choose M here. The module will be called imgpdc_wdt. +config INTEL_LGM_GPTC_WDT + tristate "INTEL LGM SoC Watchdog" + depends on X86 || COMPILE_TEST + depends on OF && HAS_IOMEM + select REGMAP + select MFD_SYSCON + select WATCHDOG_CORE + help + Driver for Watchdog Timer on Intel Lightning Mountain SoC. + + To compile this driver as a loadable module, choose M here. + The module will be called intel_lgm_gptc_wdt. + config LANTIQ_WDT tristate "Lantiq SoC watchdog" depends on LANTIQ diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 6de2e4ceef190..92c99e4c46eb7 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -166,6 +166,7 @@ obj-$(CONFIG_TXX9_WDT) += txx9wdt.o obj-$(CONFIG_OCTEON_WDT) += octeon-wdt.o octeon-wdt-y := octeon-wdt-main.o octeon-wdt-nmi.o obj-$(CONFIG_LANTIQ_WDT) += lantiq_wdt.o +obj-$(CONFIG_INTEL_LGM_GPTC_WDT) += intel_lgm_gptc_wdt.o obj-$(CONFIG_LOONGSON1_WDT) += loongson1_wdt.o obj-$(CONFIG_RALINK_WDT) += rt2880_wdt.o obj-$(CONFIG_IMGPDC_WDT) += imgpdc_wdt.o diff --git a/drivers/watchdog/intel_lgm_gptc_wdt.c b/drivers/watchdog/intel_lgm_gptc_wdt.c new file mode 100644 index 0..52be7cc194f8f --- /dev/null +++ b/drivers/watchdog/intel_lgm_gptc_wdt.c @@ -0,0 +1,420 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2020 Intel Corporation. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define GPTC_CLC 0x00 +#define GPTC_CLC_SUSPEND BIT(4) +#define GPTC_CLC_RMC GENMASK(15, 8) + +/* divider 10 to produce 200 / 10 = 20 MHz clock */ +#define CLC_RMC_DIV10 + +#define GPTC_CON(X)(0x10 + (X) * 0x20) +#define GPTC_CON_CNT_UPBIT(1) +#define GPTC_CON_ONESHOT BIT(3) +#define GPTC_CON_EXT BIT(4) + +#define GPTC_RUN(X)(0x18 + (X) * 0x20) +#define GPTC_RUN_ENBIT(0) +#define GPTC_RUN_STOP BIT(1) +#define GPTC_RUN_RELOADBIT(2) + +#define GPTC_RLD(X)(0x20 + (X) * 0x20) +#define GPTC_CNT(X)(0x28 + (X) * 0x20) + +#define GPTC_IRNENCLR 0xF0 +#define GPTC_IRNEN 0xF4 +#define GPTC_IRNCR 0xFC + +/* Watchdog Timeout Reset register offset and bitfeilds */ +#define BIA_WDT_RST_EN 0x1E0 +#define BIA_WDTBIT(6) + +#define MAX_TIMERID2 +#define MAX_CPUID 3 +#define TIMER_MARGIN_SEC 300 + +static bool nowayout = WATCHDOG_NOWAYOUT; +module_param(nowayout, bool, 0); +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started\n" + " (default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); + +struct lgm_gptc_timer { + struct lgm_gptc_wdt *wdt_node; + struct watchdog_device wdd; + unsigned inttid; + unsigned intcpuid; + unsigned intfrequency; + unsigned intcycles; + boolenable; +}; + +struct lgm_gptc_wdt { + struct device *dev; + void __iomem*gptc_base; + struct regmap *rst_hndl; + struct clk *freqclk; + struct clk *gateclk; + unsigned intfpifreq; + enum cpuhp_statestate; +}; + +DEFINE_PER_CPU(struct lgm_gptc_timer, lgm_timer_per_cpu); + +static void lgm_gptc_wdt_cfg_timer(struct lgm_gptc_timer *timer) +{ + struct lgm_gptc_wdt *wdt_node = timer->wdt_node; + void __iomem *base = wdt_node->gptc_base; + u32 val; + + val = readl(base + GPTC_CON(timer->tid)); + val &= ~GPTC_CON_CNT_UP; + val |= GPTC_CON_EXT; + val &= ~GPTC_CON_ONESHOT; + writel(val, base + GPTC_CON(timer->tid)); + writel(U32_MAX, base + GPTC_RLD(timer->tid)); + writel(BIT(timer->tid * 2), base + GPTC_IRNEN); +} + +static void lgm_gptc_wdt_init(struct lgm_gptc_wdt *wdt_node) +{ + void __iomem *bas
Re: [PATCH] phy: intel: fix enum type mismatch warning
On 5/28/2020 12:40 AM, Nathan Chancellor wrote: On Wed, May 27, 2020 at 03:45:06PM +0200, Arnd Bergmann wrote: clang points out that a local variable is initialized with an enum value of the wrong type: drivers/phy/intel/phy-intel-combo.c:202:34: error: implicit conversion from enumeration type 'enum intel_phy_mode' to different enumeration type 'enum intel_combo_mode' [-Werror,-Wenum-conversion] enum intel_combo_mode cb_mode = PHY_PCIE_MODE; ~~~ ^ From reading the code, it seems that this was not only the wrong type, but not even supposed to be a code path that can happen in practice. Change the code to have no default phy mode but instead return an error for invalid input. Fixes: ac0a95a3ea78 ("phy: intel: Add driver support for ComboPhy") Signed-off-by: Arnd Bergmann --- drivers/phy/intel/phy-intel-combo.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/phy/intel/phy-intel-combo.c b/drivers/phy/intel/phy-intel-combo.c index c2a35be4cdfb..04f7b0d08742 100644 --- a/drivers/phy/intel/phy-intel-combo.c +++ b/drivers/phy/intel/phy-intel-combo.c @@ -199,7 +199,7 @@ static int intel_cbphy_pcie_dis_pad_refclk(struct intel_cbphy_iphy *iphy) static int intel_cbphy_set_mode(struct intel_combo_phy *cbphy) { - enum intel_combo_mode cb_mode = PHY_PCIE_MODE; + enum intel_combo_mode cb_mode; enum aggregated_mode aggr = cbphy->aggr_mode; struct device *dev = cbphy->dev; enum intel_phy_mode mode; @@ -224,6 +224,8 @@ static int intel_cbphy_set_mode(struct intel_combo_phy *cbphy) cb_mode = SATA0_SATA1_MODE; break; + default: + return -EINVAL; } ret = regmap_write(cbphy->hsiocfg, REG_COMBO_MODE(cbphy->bid), cb_mode); -- 2.26.2 I sent an almost identical patch: https://lore.kernel.org/lkml/20200523035043.3305846-1-natechancel...@gmail.com/ I left out the default case since clang warns when a switch on an enum does not handle all the values (compile time scream) versus a run time scream like yours. I don't have a preference for either so: Reviewed-by: Nathan Chancellor Thanks for fixing it. I wrongly initiated it with PHY_PCIE_MODE instead of PCIE0_PCIE1_MODE to fix the compiler warnings. (On real time use case, cb_mode gets initialized with one of the switch case values, it never hits the default case, so I didn't add the default case.) This patch looks good to fix the warnings. Reviewed-by: Dilip Kota Regards, Dilip
[PATCH v2 1/1] phy: intel: Fix compilation error on FIELD_PREP usage
FIELD_PREP expects constant arguments. Istead of doing FIELD_PREP operation on the arguments of combo_phy_w32_off_mask(), pass the final FIELD_PREP value as an argument. Error reported as: In file included from include/linux/build_bug.h:5, from include/linux/bitfield.h:10, from drivers/phy/intel/phy-intel-combo.c:8: drivers/phy/intel/phy-intel-combo.c: In function 'combo_phy_w32_off_mask': include/linux/bitfield.h:52:28: warning: comparison is always false due to limited range of data type [-Wtype-limits] include/linux/compiler.h:350:38: error: call to '__compiletime_assert_37' declared with attribute error: FIELD_PREP: mask is not constant 94 | __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); | ^~~~ drivers/phy/intel/phy-intel-combo.c:137:13: note: in expansion of macro 'FIELD_PREP' 137 | reg_val |= FIELD_PREP(mask, val); | ^~ ../include/linux/compiler.h:392:38: error: call to__compiletime_assert_137 declared with attribute error: BUILD_BUG_ON failed: (((mask) + (1ULL << (__builtin_ffsll(mask) - 1))) & (((mask) + (1ULL << (__builtin_ffsll(mask) - 1))) - 1)) != 0 _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) ../include/linux/bitfield.h:94:3: note: in expansion of macro __BF_FIELD_CHECK __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \ ^~~~ ../drivers/phy/intel/phy-intel-combo.c:137:13: note: in expansion of macro FIELD_PREP reg_val |= FIELD_PREP(mask, val); ^~ Fixes: ac0a95a3ea78 ("phy: intel: Add driver support for ComboPhy") Signed-off-by: Dilip Kota Reported-by: kbuild test robot Reported-by: Randy Dunlap --- drivers/phy/intel/phy-intel-combo.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/phy/intel/phy-intel-combo.c b/drivers/phy/intel/phy-intel-combo.c index c2a35be4cdfb..254ea7cba7ca 100644 --- a/drivers/phy/intel/phy-intel-combo.c +++ b/drivers/phy/intel/phy-intel-combo.c @@ -134,7 +134,7 @@ static inline void combo_phy_w32_off_mask(void __iomem *base, unsigned int reg, reg_val = readl(base + reg); reg_val &= ~mask; - reg_val |= FIELD_PREP(mask, val); + reg_val |= val; writel(reg_val, base + reg); } @@ -169,7 +169,7 @@ static int intel_cbphy_pcie_en_pad_refclk(struct intel_cbphy_iphy *iphy) return 0; combo_phy_w32_off_mask(cbphy->app_base, PCIE_PHY_GEN_CTRL, - PCIE_PHY_CLK_PAD, 0); + PCIE_PHY_CLK_PAD, FIELD_PREP(PCIE_PHY_CLK_PAD, 0)); /* Delay for stable clock PLL */ usleep_range(50, 100); @@ -192,7 +192,7 @@ static int intel_cbphy_pcie_dis_pad_refclk(struct intel_cbphy_iphy *iphy) return 0; combo_phy_w32_off_mask(cbphy->app_base, PCIE_PHY_GEN_CTRL, - PCIE_PHY_CLK_PAD, 1); + PCIE_PHY_CLK_PAD, FIELD_PREP(PCIE_PHY_CLK_PAD, 1)); return 0; } @@ -385,7 +385,7 @@ static int intel_cbphy_calibrate(struct phy *phy) /* trigger auto RX adaptation */ combo_phy_w32_off_mask(cr_base, CR_ADDR(PCS_XF_ATE_OVRD_IN_2, id), - ADAPT_REQ_MSK, 3); + ADAPT_REQ_MSK, FIELD_PREP(ADAPT_REQ_MSK, 3)); /* Wait RX adaptation to finish */ ret = readl_poll_timeout(cr_base + CR_ADDR(PCS_XF_RX_ADAPT_ACK, id), val, val & RX_ADAPT_ACK_BIT, 10, 5000); @@ -396,7 +396,7 @@ static int intel_cbphy_calibrate(struct phy *phy) /* Stop RX adaptation */ combo_phy_w32_off_mask(cr_base, CR_ADDR(PCS_XF_ATE_OVRD_IN_2, id), - ADAPT_REQ_MSK, 0); + ADAPT_REQ_MSK, FIELD_PREP(ADAPT_REQ_MSK, 0)); return ret; } -- 2.11.0
[PATCH 1/1] phy: intel: Fix compilation error on FIELD_PREP usage
FIELD_PREP expects mask variable datatype as unsigned long and constant. Make the mask argument in combo_phy_w32_off_mask () as unsigned long const datatype. Error reported as: In file included from include/linux/build_bug.h:5, from include/linux/bitfield.h:10, from drivers/phy/intel/phy-intel-combo.c:8: drivers/phy/intel/phy-intel-combo.c: In function 'combo_phy_w32_off_mask': include/linux/bitfield.h:52:28: warning: comparison is always false due to limited range of data type [-Wtype-limits] include/linux/compiler.h:350:38: error: call to '__compiletime_assert_37' declared with attribute error: FIELD_PREP: mask is not constant 94 | __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); | ^~~~ drivers/phy/intel/phy-intel-combo.c:137:13: note: in expansion of macro 'FIELD_PREP' 137 | reg_val |= FIELD_PREP(mask, val); | ^~ Fixes: ac0a95a3ea78 ("phy: intel: Add driver support for ComboPhy") Signed-off-by: Dilip Kota Reported-by: kbuild test robot --- drivers/phy/intel/phy-intel-combo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/phy/intel/phy-intel-combo.c b/drivers/phy/intel/phy-intel-combo.c index c2a35be4cdfb..05b7d724ceb0 100644 --- a/drivers/phy/intel/phy-intel-combo.c +++ b/drivers/phy/intel/phy-intel-combo.c @@ -128,7 +128,7 @@ static int intel_cbphy_pcie_refclk_cfg(struct intel_cbphy_iphy *iphy, bool set) } static inline void combo_phy_w32_off_mask(void __iomem *base, unsigned int reg, - u32 mask, u32 val) + unsigned long const mask, u32 val) { u32 reg_val; -- 2.11.0
[PATCH v9 1/3] dt-bindings: phy: Add PHY_TYPE_XPCS definition
Add definition for Ethernet PCS phy type. Signed-off-by: Dilip Kota Acked-by: Rob Herring --- Changes on v9: No Change Changes on v8: No Change Changes on v7: No Change Changes on v6: Add Acked-by: Rob Herring include/dt-bindings/phy/phy.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/dt-bindings/phy/phy.h b/include/dt-bindings/phy/phy.h index 1f3f866fae7b..3727ef72138b 100644 --- a/include/dt-bindings/phy/phy.h +++ b/include/dt-bindings/phy/phy.h @@ -17,5 +17,6 @@ #define PHY_TYPE_USB3 4 #define PHY_TYPE_UFS 5 #define PHY_TYPE_DP6 +#define PHY_TYPE_XPCS 7 #endif /* _DT_BINDINGS_PHY */ -- 2.11.0
[PATCH v9 2/3] dt-bindings: phy: Add YAML schemas for Intel ComboPhy
ComboPhy subsystem provides PHY support to various controllers, viz. PCIe, SATA and EMAC. Adding YAML schemas for the same. Signed-off-by: Dilip Kota Reviewed-by: Rob Herring --- Changes on v9: No Change Changes on v8: No Change. Changes on v7: No Change. Changes on v6: Add Reviewed-by: Rob Herring .../devicetree/bindings/phy/intel,combo-phy.yaml | 101 + 1 file changed, 101 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml diff --git a/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml new file mode 100644 index ..347d0cdfb80d --- /dev/null +++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml @@ -0,0 +1,101 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/phy/intel,combo-phy.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Intel ComboPhy Subsystem + +maintainers: + - Dilip Kota + +description: | + Intel Combophy subsystem supports PHYs for PCIe, EMAC and SATA + controllers. A single Combophy provides two PHY instances. + +properties: + $nodename: +pattern: "combophy(@.*|-[0-9a-f])*$" + + compatible: +items: + - const: intel,combophy-lgm + - const: intel,combo-phy + + clocks: +maxItems: 1 + + reg: +items: + - description: ComboPhy core registers + - description: PCIe app core control registers + + reg-names: +items: + - const: core + - const: app + + resets: +maxItems: 4 + + reset-names: +items: + - const: phy + - const: core + - const: iphy0 + - const: iphy1 + + intel,syscfg: +$ref: /schemas/types.yaml#/definitions/phandle-array +description: Chip configuration registers handle and ComboPhy instance id + + intel,hsio: +$ref: /schemas/types.yaml#/definitions/phandle-array +description: HSIO registers handle and ComboPhy instance id on NOC + + intel,aggregation: +type: boolean +description: | + Specify the flag to configure ComboPHY in dual lane mode. + + intel,phy-mode: +$ref: /schemas/types.yaml#/definitions/uint32 +description: | + Mode of the two phys in ComboPhy. + See dt-bindings/phy/phy.h for values. + + "#phy-cells": +const: 1 + +required: + - compatible + - clocks + - reg + - reg-names + - intel,syscfg + - intel,hsio + - intel,phy-mode + - "#phy-cells" + +additionalProperties: false + +examples: + - | +#include +combophy@d0a0 { +compatible = "intel,combophy-lgm", "intel,combo-phy"; +clocks = < 1>; +#phy-cells = <1>; +reg = <0xd0a0 0x4>, + <0xd0a4 0x1000>; +reg-names = "core", "app"; +resets = < 0x50 6>, + < 0x50 17>, + < 0x50 23>, + < 0x50 24>; +reset-names = "phy", "core", "iphy0", "iphy1"; +intel,syscfg = < 0>; +intel,hsio = < 0>; +intel,phy-mode = ; +intel,aggregation; +}; -- 2.11.0
[PATCH v9 0/3] Add Intel ComboPhy driver
This patch series adds Intel ComboPhy driver, respective yaml schemas Changes on v9: Add Acked-By: Vinod Koul Fix compiler warning drivers/phy/intel/phy-intel-combo.c:229:6: warning: cb_mode may be used uninitialized in this function [-Wmaybe-uninitialized] ret = regmap_write(cbphy->hsiocfg, REG_COMBO_MODE(cbphy->bid), cb_mode); ^~~ drivers/phy/intel/phy-intel-combo.c:204:24: note: cb_mode was declared here enum intel_combo_mode cb_mode; Changes on v8: As per PHY Maintainer's request add description in comments for doing register access through register map framework. Changes on v7: As per System control driver maintainer's inputs remove fwnode_to_regmap() definition and use device_node_get_regmap() Changes on v6: Rebase patches on the latest maintainer's branch https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git/?h=phy-for-5.7 Dilip Kota (3): dt-bindings: phy: Add PHY_TYPE_XPCS definition dt-bindings: phy: Add YAML schemas for Intel ComboPhy phy: intel: Add driver support for ComboPhy .../devicetree/bindings/phy/intel,combo-phy.yaml | 101 drivers/phy/intel/Kconfig | 14 + drivers/phy/intel/Makefile | 1 + drivers/phy/intel/phy-intel-combo.c| 632 + include/dt-bindings/phy/phy.h | 1 + 5 files changed, 749 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml create mode 100644 drivers/phy/intel/phy-intel-combo.c -- 2.11.0
[PATCH v9 3/3] phy: intel: Add driver support for ComboPhy
ComboPhy subsystem provides PHYs for various controllers like PCIe, SATA and EMAC. Signed-off-by: Dilip Kota --- Changes on v9: Add Acked-By: Vinod Koul Fix compiler warning drivers/phy/intel/phy-intel-combo.c:229:6: warning: cb_mode may be used uninitialized in this function [-Wmaybe-uninitialized] ret = regmap_write(cbphy->hsiocfg, REG_COMBO_MODE(cbphy->bid), cb_mode); ^~~ drivers/phy/intel/phy-intel-combo.c:204:24: note: cb_mode was declared here enum intel_combo_mode cb_mode; Changes on v8: As per PHY Maintainer's request add description for doing register access through regmap in comments. Changes on v7: Use device_node_to_regmap instead of fwnode_to_regmap Changes on v6: No changes Changes on v5: Add changes as per inputs from Andy and Rob: DT node uses phy-mode values as defined in "include/dt-bindings/phy/phy.h", add changes to handle it. ComboPhy no longer has children nodes, and children node properties(reset) moved to parent node, so do the code changes accordingly. Add _xlate() function to pass the appropriate phy handle. Fix couple of nitpicks. Changes on v4: Address review comments Remove dependency on OF config Update copyright to 2019-2020 Define register macro PAD_DIS_CFG instead of const variable inside function. Improve the error prints, and error returns. Call put_device(dev), for get_dev_from_fwnode() Move platform_set_drvdata() at the end of the probe(). Correct alignment in phy_ops intel_cbphy_ops. Correct commented lines with proper vocabulary and punctuation. Add/remove commas for the required constant arrays and enums. Remove in driver: linux/kernel.h, not required macros: PCIE_PHY_MPLLA_CTRL, PCIE_PHY_MPLLB_CTRL temp variable u32 prop; Change function names: intel_cbphy_iphy_dt_parse() -> intel_cbphy_iphy_fwnode_parse() intel_cbphy_dt_sanity_check() -> intel_cbphy_sanity_check() intel_cbphy_dt_parse() -> intel_cbphy_fwnode_parse() Changes on v3: Remove intel_iphy_names Remove struct phy in struct intel_cbphy_iphy Imporve if conditions logic Use fwnode_to_regmap() Call devm_of_platform_populate() to populate child nodes Fix reset sequence during phy_init Add SoC specific compatible "intel,combophy-lgm" Add description for enums Remove default case in switch {} intel_cbphy_set_mode() as it never happens. Use mutex_lock to synchronise combophy initialization across two phys. Change init_cnt to u32 datatype as it is within mutex lock. Correct error handling of fwnode_property_read_u32_array(fwnode, "intel,phy-mode", ...) drivers/phy/intel/Kconfig | 14 + drivers/phy/intel/Makefile | 1 + drivers/phy/intel/phy-intel-combo.c | 632 3 files changed, 647 insertions(+) create mode 100644 drivers/phy/intel/phy-intel-combo.c diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig index 4ea6a8897cd7..3b40eb7b4fb4 100644 --- a/drivers/phy/intel/Kconfig +++ b/drivers/phy/intel/Kconfig @@ -2,6 +2,20 @@ # # Phy drivers for Intel Lightning Mountain(LGM) platform # +config PHY_INTEL_COMBO + bool "Intel ComboPHY driver" + depends on X86 || COMPILE_TEST + depends on OF && HAS_IOMEM + select MFD_SYSCON + select GENERIC_PHY + select REGMAP + help + Enable this to support Intel ComboPhy. + + This driver configures ComboPhy subsystem on Intel gateway + chipsets which provides PHYs for various controllers, EMAC, + SATA and PCIe. + config PHY_INTEL_EMMC tristate "Intel EMMC PHY driver" select GENERIC_PHY diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile index 6b876a75599d..233d530dadde 100644 --- a/drivers/phy/intel/Makefile +++ b/drivers/phy/intel/Makefile @@ -1,2 +1,3 @@ # SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_PHY_INTEL_COMBO) += phy-intel-combo.o obj-$(CONFIG_PHY_INTEL_EMMC)+= phy-intel-emmc.o diff --git a/drivers/phy/intel/phy-intel-combo.c b/drivers/phy/intel/phy-intel-combo.c new file mode 100644 index ..c2a35be4cdfb --- /dev/null +++ b/drivers/phy/intel/phy-intel-combo.c @@ -0,0 +1,632 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Intel Combo-PHY driver + * + * Copyright (C) 2019-2020 Intel Corporation. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#define PCIE_PHY_GEN_CTRL 0x00 +#define PCIE_PHY_CLK_PAD BIT(17) + +#define PAD_DIS_CFG0x174 + +#define PCS_XF_ATE_OVRD_IN_2 0x3008 +#define ADAPT_REQ_MSK GENMASK(5, 4) + +#define PCS_XF_RX_ADAPT_ACK0x3010 +#define RX_ADAPT_ACK_BIT BIT(0) + +#define CR_ADDR(addr, lane)(((addr) + (lane) * 0x100) <&
Re: [RESEND PATCH v8 0/3] Add Intel ComboPhy driver
On 5/19/2020 1:17 PM, Kishon Vijay Abraham I wrote: Dilip, On 5/19/2020 9:26 AM, Dilip Kota wrote: On 5/18/2020 9:49 PM, Kishon Vijay Abraham I wrote: Dilip, On 5/15/2020 1:43 PM, Dilip Kota wrote: This patch series adds Intel ComboPhy driver, respective yaml schemas Changes on v8: As per PHY Maintainer's request add description in comments for doing register access through register map framework. Changes on v7: As per System control driver maintainer's inputs remove fwnode_to_regmap() definition and use device_node_get_regmap() Can you fix this warning and resend the patch? drivers/phy/intel/phy-intel-combo.c:229:6: warning: ‘cb_mode’ may be used uninitialized in this function [-Wmaybe-uninitialized] ret = regmap_write(cbphy->hsiocfg, REG_COMBO_MODE(cbphy->bid), cb_mode); ^~~ drivers/phy/intel/phy-intel-combo.c:204:24: note: ‘cb_mode’ was declared here enum intel_combo_mode cb_mode; ^~~ I noticed this warning while preparing the patch. It sounds like false warning because: 1.) "cb_mode" is initialized in the switch case based on the "mode = cbphy->phy_mode;" 2.) cbphy->phy_mode is initialized during the probe in "intel_cbphy_fwnode_parse()" with one of the 3 values. PHY_PCIE_MODE, PHY_SATA_MODE, PHY_XPCS_MODE. 3.) There is no chance of "cbphy->phy_mode" having different value. 4.) And "cb_mode" will be initialized according to the "mode = cbphy->phy_mode;" 5.) Hence, there is no chance of "cb_mode" getting accessed uninitialized. Let's try to keep the compiler happy. Please fix this warning. Sure, will fix it and send the patch series. Thanks Kishon
Re: [RESEND PATCH v8 0/3] Add Intel ComboPhy driver
On 5/18/2020 9:49 PM, Kishon Vijay Abraham I wrote: Dilip, On 5/15/2020 1:43 PM, Dilip Kota wrote: This patch series adds Intel ComboPhy driver, respective yaml schemas Changes on v8: As per PHY Maintainer's request add description in comments for doing register access through register map framework. Changes on v7: As per System control driver maintainer's inputs remove fwnode_to_regmap() definition and use device_node_get_regmap() Can you fix this warning and resend the patch? drivers/phy/intel/phy-intel-combo.c:229:6: warning: ‘cb_mode’ may be used uninitialized in this function [-Wmaybe-uninitialized] ret = regmap_write(cbphy->hsiocfg, REG_COMBO_MODE(cbphy->bid), cb_mode); ^~~ drivers/phy/intel/phy-intel-combo.c:204:24: note: ‘cb_mode’ was declared here enum intel_combo_mode cb_mode; ^~~ I noticed this warning while preparing the patch. It sounds like false warning because: 1.) "cb_mode" is initialized in the switch case based on the "mode = cbphy->phy_mode;" 2.) cbphy->phy_mode is initialized during the probe in "intel_cbphy_fwnode_parse()" with one of the 3 values. PHY_PCIE_MODE, PHY_SATA_MODE, PHY_XPCS_MODE. 3.) There is no chance of "cbphy->phy_mode" having different value. 4.) And "cb_mode" will be initialized according to the "mode = cbphy->phy_mode;" 5.) Hence, there is no chance of "cb_mode" getting accessed uninitialized. Regards, Dilip Thanks Kishon Changes on v6: Rebase patches on the latest maintainer's branch https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git/?h=phy-for-5.7 Dilip Kota (3): dt-bindings: phy: Add PHY_TYPE_XPCS definition dt-bindings: phy: Add YAML schemas for Intel ComboPhy phy: intel: Add driver support for ComboPhy .../devicetree/bindings/phy/intel,combo-phy.yaml | 101 drivers/phy/intel/Kconfig | 14 + drivers/phy/intel/Makefile | 1 + drivers/phy/intel/phy-intel-combo.c| 632 + include/dt-bindings/phy/phy.h | 1 + 5 files changed, 749 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml create mode 100644 drivers/phy/intel/phy-intel-combo.c
[RESEND PATCH v8 1/3] dt-bindings: phy: Add PHY_TYPE_XPCS definition
Add definition for Ethernet PCS phy type. Signed-off-by: Dilip Kota Acked-by: Rob Herring --- Changes on v8: No Change Changes on v7: No Change Changes on v6: Add Acked-by: Rob Herring include/dt-bindings/phy/phy.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/dt-bindings/phy/phy.h b/include/dt-bindings/phy/phy.h index 1f3f866fae7b..3727ef72138b 100644 --- a/include/dt-bindings/phy/phy.h +++ b/include/dt-bindings/phy/phy.h @@ -17,5 +17,6 @@ #define PHY_TYPE_USB3 4 #define PHY_TYPE_UFS 5 #define PHY_TYPE_DP6 +#define PHY_TYPE_XPCS 7 #endif /* _DT_BINDINGS_PHY */ -- 2.11.0
[RESEND PATCH v8 2/3] dt-bindings: phy: Add YAML schemas for Intel ComboPhy
ComboPhy subsystem provides PHY support to various controllers, viz. PCIe, SATA and EMAC. Adding YAML schemas for the same. Signed-off-by: Dilip Kota Reviewed-by: Rob Herring --- Changes on v8: No Change. Changes on v7: No Change. Changes on v6: Add Reviewed-by: Rob Herring .../devicetree/bindings/phy/intel,combo-phy.yaml | 101 + 1 file changed, 101 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml diff --git a/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml new file mode 100644 index ..347d0cdfb80d --- /dev/null +++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml @@ -0,0 +1,101 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/phy/intel,combo-phy.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Intel ComboPhy Subsystem + +maintainers: + - Dilip Kota + +description: | + Intel Combophy subsystem supports PHYs for PCIe, EMAC and SATA + controllers. A single Combophy provides two PHY instances. + +properties: + $nodename: +pattern: "combophy(@.*|-[0-9a-f])*$" + + compatible: +items: + - const: intel,combophy-lgm + - const: intel,combo-phy + + clocks: +maxItems: 1 + + reg: +items: + - description: ComboPhy core registers + - description: PCIe app core control registers + + reg-names: +items: + - const: core + - const: app + + resets: +maxItems: 4 + + reset-names: +items: + - const: phy + - const: core + - const: iphy0 + - const: iphy1 + + intel,syscfg: +$ref: /schemas/types.yaml#/definitions/phandle-array +description: Chip configuration registers handle and ComboPhy instance id + + intel,hsio: +$ref: /schemas/types.yaml#/definitions/phandle-array +description: HSIO registers handle and ComboPhy instance id on NOC + + intel,aggregation: +type: boolean +description: | + Specify the flag to configure ComboPHY in dual lane mode. + + intel,phy-mode: +$ref: /schemas/types.yaml#/definitions/uint32 +description: | + Mode of the two phys in ComboPhy. + See dt-bindings/phy/phy.h for values. + + "#phy-cells": +const: 1 + +required: + - compatible + - clocks + - reg + - reg-names + - intel,syscfg + - intel,hsio + - intel,phy-mode + - "#phy-cells" + +additionalProperties: false + +examples: + - | +#include +combophy@d0a0 { +compatible = "intel,combophy-lgm", "intel,combo-phy"; +clocks = < 1>; +#phy-cells = <1>; +reg = <0xd0a0 0x4>, + <0xd0a4 0x1000>; +reg-names = "core", "app"; +resets = < 0x50 6>, + < 0x50 17>, + < 0x50 23>, + < 0x50 24>; +reset-names = "phy", "core", "iphy0", "iphy1"; +intel,syscfg = < 0>; +intel,hsio = < 0>; +intel,phy-mode = ; +intel,aggregation; +}; -- 2.11.0
[RESEND PATCH v8 3/3] phy: intel: Add driver support for ComboPhy
ComboPhy subsystem provides PHYs for various controllers like PCIe, SATA and EMAC. Signed-off-by: Dilip Kota --- Changes on v8: As per PHY Maintainer's request add description for doing register access through regmap in comments. Changes on v7: Use device_node_to_regmap instead of fwnode_to_regmap Changes on v6: No changes Changes on v5: Add changes as per inputs from Andy and Rob: DT node uses phy-mode values as defined in "include/dt-bindings/phy/phy.h", add changes to handle it. ComboPhy no longer has children nodes, and children node properties(reset) moved to parent node, so do the code changes accordingly. Add _xlate() function to pass the appropriate phy handle. Fix couple of nitpicks. Changes on v4: Address review comments Remove dependency on OF config Update copyright to 2019-2020 Define register macro PAD_DIS_CFG instead of const variable inside function. Improve the error prints, and error returns. Call put_device(dev), for get_dev_from_fwnode() Move platform_set_drvdata() at the end of the probe(). Correct alignment in phy_ops intel_cbphy_ops. Correct commented lines with proper vocabulary and punctuation. Add/remove commas for the required constant arrays and enums. Remove in driver: linux/kernel.h, not required macros: PCIE_PHY_MPLLA_CTRL, PCIE_PHY_MPLLB_CTRL temp variable u32 prop; Change function names: intel_cbphy_iphy_dt_parse() -> intel_cbphy_iphy_fwnode_parse() intel_cbphy_dt_sanity_check() -> intel_cbphy_sanity_check() intel_cbphy_dt_parse() -> intel_cbphy_fwnode_parse() Changes on v3: Remove intel_iphy_names Remove struct phy in struct intel_cbphy_iphy Imporve if conditions logic Use fwnode_to_regmap() Call devm_of_platform_populate() to populate child nodes Fix reset sequence during phy_init Add SoC specific compatible "intel,combophy-lgm" Add description for enums Remove default case in switch {} intel_cbphy_set_mode() as it never happens. Use mutex_lock to synchronise combophy initialization across two phys. Change init_cnt to u32 datatype as it is within mutex lock. Correct error handling of fwnode_property_read_u32_array(fwnode, "intel,phy-mode", ...) drivers/phy/intel/Kconfig | 14 + drivers/phy/intel/Makefile | 1 + drivers/phy/intel/phy-intel-combo.c | 632 3 files changed, 647 insertions(+) create mode 100644 drivers/phy/intel/phy-intel-combo.c diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig index 4ea6a8897cd7..3b40eb7b4fb4 100644 --- a/drivers/phy/intel/Kconfig +++ b/drivers/phy/intel/Kconfig @@ -2,6 +2,20 @@ # # Phy drivers for Intel Lightning Mountain(LGM) platform # +config PHY_INTEL_COMBO + bool "Intel ComboPHY driver" + depends on X86 || COMPILE_TEST + depends on OF && HAS_IOMEM + select MFD_SYSCON + select GENERIC_PHY + select REGMAP + help + Enable this to support Intel ComboPhy. + + This driver configures ComboPhy subsystem on Intel gateway + chipsets which provides PHYs for various controllers, EMAC, + SATA and PCIe. + config PHY_INTEL_EMMC tristate "Intel EMMC PHY driver" select GENERIC_PHY diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile index 6b876a75599d..233d530dadde 100644 --- a/drivers/phy/intel/Makefile +++ b/drivers/phy/intel/Makefile @@ -1,2 +1,3 @@ # SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_PHY_INTEL_COMBO) += phy-intel-combo.o obj-$(CONFIG_PHY_INTEL_EMMC)+= phy-intel-emmc.o diff --git a/drivers/phy/intel/phy-intel-combo.c b/drivers/phy/intel/phy-intel-combo.c new file mode 100644 index ..4bc1276ecf23 --- /dev/null +++ b/drivers/phy/intel/phy-intel-combo.c @@ -0,0 +1,632 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Intel Combo-PHY driver + * + * Copyright (C) 2019-2020 Intel Corporation. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#define PCIE_PHY_GEN_CTRL 0x00 +#define PCIE_PHY_CLK_PAD BIT(17) + +#define PAD_DIS_CFG0x174 + +#define PCS_XF_ATE_OVRD_IN_2 0x3008 +#define ADAPT_REQ_MSK GENMASK(5, 4) + +#define PCS_XF_RX_ADAPT_ACK0x3010 +#define RX_ADAPT_ACK_BIT BIT(0) + +#define CR_ADDR(addr, lane)(((addr) + (lane) * 0x100) << 2) +#define REG_COMBO_MODE(x) ((x) * 0x200) +#define REG_CLK_DISABLE(x) ((x) * 0x200 + 0x124) + +#define COMBO_PHY_ID(x)((x)->parent->id) +#define PHY_ID(x) ((x)->id) + +#define CLK_100MHZ 1 +#define CLK_156_25MHZ 15625 + +static const unsigned long intel_iphy_clk_rates[] = { + CLK_100MHZ, CLK_156_25MHZ, CLK_100MHZ, +}; + +enum { + PHY_0, + PHY_1, + PHY
[RESEND PATCH v8 0/3] Add Intel ComboPhy driver
This patch series adds Intel ComboPhy driver, respective yaml schemas Changes on v8: As per PHY Maintainer's request add description in comments for doing register access through register map framework. Changes on v7: As per System control driver maintainer's inputs remove fwnode_to_regmap() definition and use device_node_get_regmap() Changes on v6: Rebase patches on the latest maintainer's branch https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git/?h=phy-for-5.7 Dilip Kota (3): dt-bindings: phy: Add PHY_TYPE_XPCS definition dt-bindings: phy: Add YAML schemas for Intel ComboPhy phy: intel: Add driver support for ComboPhy .../devicetree/bindings/phy/intel,combo-phy.yaml | 101 drivers/phy/intel/Kconfig | 14 + drivers/phy/intel/Makefile | 1 + drivers/phy/intel/phy-intel-combo.c| 632 + include/dt-bindings/phy/phy.h | 1 + 5 files changed, 749 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml create mode 100644 drivers/phy/intel/phy-intel-combo.c -- 2.11.0
Re: [PATCH v8 3/3] phy: intel: Add driver support for ComboPhy
On 5/15/2020 3:39 PM, Vinod Koul wrote: On 15-05-20, 12:30, Dilip Kota wrote: ComboPhy subsystem provides PHYs for various controllers like PCIe, SATA and EMAC. Signed-off-by: Dilip Kota --- Changes on v8: As per PHY Maintainer's request add description for doing register access through regmap in comments. I dont see any comments below, and my script tells me this is *same* as v7, did you send the wrong version? My bad. I missed the git add step and sent patch series. Will resend the patch series. Regards, Dilip Changes on v7: Use device_node_to_regmap instead of fwnode_to_regmap Changes on v6: No changes Changes on v5: Add changes as per inputs from Andy and Rob: DT node uses phy-mode values as defined in "include/dt-bindings/phy/phy.h", add changes to handle it. ComboPhy no longer has children nodes, and children node properties(reset) moved to parent node, so do the code changes accordingly. Add _xlate() function to pass the appropriate phy handle. Fix couple of nitpicks. Changes on v4: Address review comments Remove dependency on OF config Update copyright to 2019-2020 Define register macro PAD_DIS_CFG instead of const variable inside function. Improve the error prints, and error returns. Call put_device(dev), for get_dev_from_fwnode() Move platform_set_drvdata() at the end of the probe(). Correct alignment in phy_ops intel_cbphy_ops. Correct commented lines with proper vocabulary and punctuation. Add/remove commas for the required constant arrays and enums. Remove in driver: linux/kernel.h, not required macros: PCIE_PHY_MPLLA_CTRL, PCIE_PHY_MPLLB_CTRL temp variable u32 prop; Change function names: intel_cbphy_iphy_dt_parse() -> intel_cbphy_iphy_fwnode_parse() intel_cbphy_dt_sanity_check() -> intel_cbphy_sanity_check() intel_cbphy_dt_parse() -> intel_cbphy_fwnode_parse() Changes on v3: Remove intel_iphy_names Remove struct phy in struct intel_cbphy_iphy Imporve if conditions logic Use fwnode_to_regmap() Call devm_of_platform_populate() to populate child nodes Fix reset sequence during phy_init Add SoC specific compatible "intel,combophy-lgm" Add description for enums Remove default case in switch {} intel_cbphy_set_mode() as it never happens. Use mutex_lock to synchronise combophy initialization across two phys. Change init_cnt to u32 datatype as it is within mutex lock. Correct error handling of fwnode_property_read_u32_array(fwnode, "intel,phy-mode", ...) drivers/phy/intel/Kconfig | 14 + drivers/phy/intel/Makefile | 1 + drivers/phy/intel/phy-intel-combo.c | 627 3 files changed, 642 insertions(+) create mode 100644 drivers/phy/intel/phy-intel-combo.c diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig index 4ea6a8897cd7..3b40eb7b4fb4 100644 --- a/drivers/phy/intel/Kconfig +++ b/drivers/phy/intel/Kconfig @@ -2,6 +2,20 @@ # # Phy drivers for Intel Lightning Mountain(LGM) platform # +config PHY_INTEL_COMBO + bool "Intel ComboPHY driver" + depends on X86 || COMPILE_TEST + depends on OF && HAS_IOMEM + select MFD_SYSCON + select GENERIC_PHY + select REGMAP + help + Enable this to support Intel ComboPhy. + + This driver configures ComboPhy subsystem on Intel gateway + chipsets which provides PHYs for various controllers, EMAC, + SATA and PCIe. + config PHY_INTEL_EMMC tristate "Intel EMMC PHY driver" select GENERIC_PHY diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile index 6b876a75599d..233d530dadde 100644 --- a/drivers/phy/intel/Makefile +++ b/drivers/phy/intel/Makefile @@ -1,2 +1,3 @@ # SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_PHY_INTEL_COMBO) += phy-intel-combo.o obj-$(CONFIG_PHY_INTEL_EMMC)+= phy-intel-emmc.o diff --git a/drivers/phy/intel/phy-intel-combo.c b/drivers/phy/intel/phy-intel-combo.c new file mode 100644 index ..04ad595e21e4 --- /dev/null +++ b/drivers/phy/intel/phy-intel-combo.c @@ -0,0 +1,627 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Intel Combo-PHY driver + * + * Copyright (C) 2019-2020 Intel Corporation. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#define PCIE_PHY_GEN_CTRL 0x00 +#define PCIE_PHY_CLK_PAD BIT(17) + +#define PAD_DIS_CFG0x174 + +#define PCS_XF_ATE_OVRD_IN_2 0x3008 +#define ADAPT_REQ_MSK GENMASK(5, 4) + +#define PCS_XF_RX_ADAPT_ACK0x3010 +#define RX_ADAPT_ACK_BIT BIT(0) + +#define CR_ADDR(addr, lane)(((addr) + (lane) * 0x100) << 2) +#define REG_COMBO_MODE(x) ((x) * 0x200) +#define REG_CLK_DISABLE(x) ((x) * 0x200 + 0x124) +
[PATCH v8 1/3] dt-bindings: phy: Add PHY_TYPE_XPCS definition
Add definition for Ethernet PCS phy type. Signed-off-by: Dilip Kota Acked-by: Rob Herring --- Changes on v8: No Change Changes on v7: No Change Changes on v6: Add Acked-by: Rob Herring include/dt-bindings/phy/phy.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/dt-bindings/phy/phy.h b/include/dt-bindings/phy/phy.h index 1f3f866fae7b..3727ef72138b 100644 --- a/include/dt-bindings/phy/phy.h +++ b/include/dt-bindings/phy/phy.h @@ -17,5 +17,6 @@ #define PHY_TYPE_USB3 4 #define PHY_TYPE_UFS 5 #define PHY_TYPE_DP6 +#define PHY_TYPE_XPCS 7 #endif /* _DT_BINDINGS_PHY */ -- 2.11.0
[PATCH v8 2/3] dt-bindings: phy: Add YAML schemas for Intel ComboPhy
ComboPhy subsystem provides PHY support to various controllers, viz. PCIe, SATA and EMAC. Adding YAML schemas for the same. Signed-off-by: Dilip Kota Reviewed-by: Rob Herring --- Changes on v8: No Change. Changes on v7: No Change. Changes on v6: Add Reviewed-by: Rob Herring .../devicetree/bindings/phy/intel,combo-phy.yaml | 101 + 1 file changed, 101 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml diff --git a/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml new file mode 100644 index ..347d0cdfb80d --- /dev/null +++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml @@ -0,0 +1,101 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/phy/intel,combo-phy.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Intel ComboPhy Subsystem + +maintainers: + - Dilip Kota + +description: | + Intel Combophy subsystem supports PHYs for PCIe, EMAC and SATA + controllers. A single Combophy provides two PHY instances. + +properties: + $nodename: +pattern: "combophy(@.*|-[0-9a-f])*$" + + compatible: +items: + - const: intel,combophy-lgm + - const: intel,combo-phy + + clocks: +maxItems: 1 + + reg: +items: + - description: ComboPhy core registers + - description: PCIe app core control registers + + reg-names: +items: + - const: core + - const: app + + resets: +maxItems: 4 + + reset-names: +items: + - const: phy + - const: core + - const: iphy0 + - const: iphy1 + + intel,syscfg: +$ref: /schemas/types.yaml#/definitions/phandle-array +description: Chip configuration registers handle and ComboPhy instance id + + intel,hsio: +$ref: /schemas/types.yaml#/definitions/phandle-array +description: HSIO registers handle and ComboPhy instance id on NOC + + intel,aggregation: +type: boolean +description: | + Specify the flag to configure ComboPHY in dual lane mode. + + intel,phy-mode: +$ref: /schemas/types.yaml#/definitions/uint32 +description: | + Mode of the two phys in ComboPhy. + See dt-bindings/phy/phy.h for values. + + "#phy-cells": +const: 1 + +required: + - compatible + - clocks + - reg + - reg-names + - intel,syscfg + - intel,hsio + - intel,phy-mode + - "#phy-cells" + +additionalProperties: false + +examples: + - | +#include +combophy@d0a0 { +compatible = "intel,combophy-lgm", "intel,combo-phy"; +clocks = < 1>; +#phy-cells = <1>; +reg = <0xd0a0 0x4>, + <0xd0a4 0x1000>; +reg-names = "core", "app"; +resets = < 0x50 6>, + < 0x50 17>, + < 0x50 23>, + < 0x50 24>; +reset-names = "phy", "core", "iphy0", "iphy1"; +intel,syscfg = < 0>; +intel,hsio = < 0>; +intel,phy-mode = ; +intel,aggregation; +}; -- 2.11.0
[PATCH v8 0/3] Add Intel ComboPhy driver
This patch series adds Intel ComboPhy driver, respective yaml schemas Changes on v8: As per PHY Maintainer's request add description in comments for doing register access through register map framework. Changes on v7: As per System control driver maintainer's inputs remove fwnode_to_regmap() definition and use device_node_get_regmap() Changes on v6: Rebase patches on the latest maintainer's branch https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git/?h=phy-for-5.7 Dilip Kota (3): dt-bindings: phy: Add PHY_TYPE_XPCS definition dt-bindings: phy: Add YAML schemas for Intel ComboPhy phy: intel: Add driver support for ComboPhy .../devicetree/bindings/phy/intel,combo-phy.yaml | 101 drivers/phy/intel/Kconfig | 14 + drivers/phy/intel/Makefile | 1 + drivers/phy/intel/phy-intel-combo.c| 627 + include/dt-bindings/phy/phy.h | 1 + 5 files changed, 744 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml create mode 100644 drivers/phy/intel/phy-intel-combo.c -- 2.11.0
[PATCH v8 3/3] phy: intel: Add driver support for ComboPhy
ComboPhy subsystem provides PHYs for various controllers like PCIe, SATA and EMAC. Signed-off-by: Dilip Kota --- Changes on v8: As per PHY Maintainer's request add description for doing register access through regmap in comments. Changes on v7: Use device_node_to_regmap instead of fwnode_to_regmap Changes on v6: No changes Changes on v5: Add changes as per inputs from Andy and Rob: DT node uses phy-mode values as defined in "include/dt-bindings/phy/phy.h", add changes to handle it. ComboPhy no longer has children nodes, and children node properties(reset) moved to parent node, so do the code changes accordingly. Add _xlate() function to pass the appropriate phy handle. Fix couple of nitpicks. Changes on v4: Address review comments Remove dependency on OF config Update copyright to 2019-2020 Define register macro PAD_DIS_CFG instead of const variable inside function. Improve the error prints, and error returns. Call put_device(dev), for get_dev_from_fwnode() Move platform_set_drvdata() at the end of the probe(). Correct alignment in phy_ops intel_cbphy_ops. Correct commented lines with proper vocabulary and punctuation. Add/remove commas for the required constant arrays and enums. Remove in driver: linux/kernel.h, not required macros: PCIE_PHY_MPLLA_CTRL, PCIE_PHY_MPLLB_CTRL temp variable u32 prop; Change function names: intel_cbphy_iphy_dt_parse() -> intel_cbphy_iphy_fwnode_parse() intel_cbphy_dt_sanity_check() -> intel_cbphy_sanity_check() intel_cbphy_dt_parse() -> intel_cbphy_fwnode_parse() Changes on v3: Remove intel_iphy_names Remove struct phy in struct intel_cbphy_iphy Imporve if conditions logic Use fwnode_to_regmap() Call devm_of_platform_populate() to populate child nodes Fix reset sequence during phy_init Add SoC specific compatible "intel,combophy-lgm" Add description for enums Remove default case in switch {} intel_cbphy_set_mode() as it never happens. Use mutex_lock to synchronise combophy initialization across two phys. Change init_cnt to u32 datatype as it is within mutex lock. Correct error handling of fwnode_property_read_u32_array(fwnode, "intel,phy-mode", ...) drivers/phy/intel/Kconfig | 14 + drivers/phy/intel/Makefile | 1 + drivers/phy/intel/phy-intel-combo.c | 627 3 files changed, 642 insertions(+) create mode 100644 drivers/phy/intel/phy-intel-combo.c diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig index 4ea6a8897cd7..3b40eb7b4fb4 100644 --- a/drivers/phy/intel/Kconfig +++ b/drivers/phy/intel/Kconfig @@ -2,6 +2,20 @@ # # Phy drivers for Intel Lightning Mountain(LGM) platform # +config PHY_INTEL_COMBO + bool "Intel ComboPHY driver" + depends on X86 || COMPILE_TEST + depends on OF && HAS_IOMEM + select MFD_SYSCON + select GENERIC_PHY + select REGMAP + help + Enable this to support Intel ComboPhy. + + This driver configures ComboPhy subsystem on Intel gateway + chipsets which provides PHYs for various controllers, EMAC, + SATA and PCIe. + config PHY_INTEL_EMMC tristate "Intel EMMC PHY driver" select GENERIC_PHY diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile index 6b876a75599d..233d530dadde 100644 --- a/drivers/phy/intel/Makefile +++ b/drivers/phy/intel/Makefile @@ -1,2 +1,3 @@ # SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_PHY_INTEL_COMBO) += phy-intel-combo.o obj-$(CONFIG_PHY_INTEL_EMMC)+= phy-intel-emmc.o diff --git a/drivers/phy/intel/phy-intel-combo.c b/drivers/phy/intel/phy-intel-combo.c new file mode 100644 index ..04ad595e21e4 --- /dev/null +++ b/drivers/phy/intel/phy-intel-combo.c @@ -0,0 +1,627 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Intel Combo-PHY driver + * + * Copyright (C) 2019-2020 Intel Corporation. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#define PCIE_PHY_GEN_CTRL 0x00 +#define PCIE_PHY_CLK_PAD BIT(17) + +#define PAD_DIS_CFG0x174 + +#define PCS_XF_ATE_OVRD_IN_2 0x3008 +#define ADAPT_REQ_MSK GENMASK(5, 4) + +#define PCS_XF_RX_ADAPT_ACK0x3010 +#define RX_ADAPT_ACK_BIT BIT(0) + +#define CR_ADDR(addr, lane)(((addr) + (lane) * 0x100) << 2) +#define REG_COMBO_MODE(x) ((x) * 0x200) +#define REG_CLK_DISABLE(x) ((x) * 0x200 + 0x124) + +#define COMBO_PHY_ID(x)((x)->parent->id) +#define PHY_ID(x) ((x)->id) + +#define CLK_100MHZ 1 +#define CLK_156_25MHZ 15625 + +static const unsigned long intel_iphy_clk_rates[] = { + CLK_100MHZ, CLK_156_25MHZ, CLK_100MHZ, +}; + +enum { + PHY_0, + PHY_1, + PHY
Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy
Hi Vinod, On 5/5/2020 3:54 PM, Dilip Kota wrote: On 5/5/2020 1:21 PM, Vinod Koul wrote: On 04-05-20, 17:32, Dilip Kota wrote: On 5/4/2020 5:20 PM, Vinod Koul wrote: On 04-05-20, 16:26, Dilip Kota wrote: On 5/4/2020 3:29 PM, Vinod Koul wrote: On 30-04-20, 15:15, Dilip Kota wrote: + u32 mask, u32 val) +{ + u32 reg_val; + + reg_val = readl(base + reg); + reg_val &= ~mask; + reg_val |= FIELD_PREP(mask, val); + writel(reg_val, base + reg); bypassing regmap here... why? It is not regmap address, one of the below two addresses are passed to this function. okay, perhaps add a comment somewhere that regmap is not used for this base? I dont see a need of adding a comment, describing don't do regmap here. Driver uses regmap except here, which seems odd hence explanation required for this. During the driver Probe, the register phandles are stored in regmap datatype variables and PHY core addresses are stored in iomem datatype. Since then, regmap access is performed for the regmap datatype variables and readl/writel access is performed on the iomem datatype variables. And nowhere in the driver iomem datatype address are converted to regmap address and performed regmap access. Driver is not doing any 'regmap_init' on any physical address. Driver is getting the register address phandle from the device tree node and performing the regmap access. ret = fwnode_property_get_reference_args(fwnode, "intel,syscfg", NULL, 1, 0, ); [...] cbphy->syscfg = device_node_to_regmap(to_of_node(ref.fwnode)); [...] ret = fwnode_property_get_reference_args(fwnode, "intel,hsio", NULL, 1, 0, ); [...] cbphy->hsiocfg = device_node_to_regmap(to_of_node(ref.fwnode)); [...] cbphy->app_base = devm_platform_ioremap_resource_byname(pdev, "app"); [...] cbphy->cr_base = devm_platform_ioremap_resource_byname(pdev, "core"); The DT parsing logic in the driver is explaining why the PHY driver should do regmap access and to whom should be done. For this reason i am a bit puzzled to what more is needed to explain in the comments and where to add it. Please let me know your view. Gentle Reminder! Could you please update on this. Regards, Dilip Regards, Dilip
Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers
On 5/5/2020 7:23 PM, Mark Brown wrote: On Mon, May 04, 2020 at 06:15:47PM +0800, Dilip Kota wrote: On 4/29/2020 8:13 PM, Mark Brown wrote: I just tried to get the history of removing workqueue in SPI driver, on GRX500 (earlier chipset of LGM) the SPI transfers got timedout with workqueues during regression testing. Once changed to threaded IRQs transfers are working successfully. That doesn't really explain why though, it just explains what. I didnt find more information about it. I will work to reproduce the issue and share the detailed information sooner i get the accessibility of the SoC (because of covid19 doing wfh) Regards, Dilip
Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy
On 5/5/2020 1:21 PM, Vinod Koul wrote: On 04-05-20, 17:32, Dilip Kota wrote: On 5/4/2020 5:20 PM, Vinod Koul wrote: On 04-05-20, 16:26, Dilip Kota wrote: On 5/4/2020 3:29 PM, Vinod Koul wrote: On 30-04-20, 15:15, Dilip Kota wrote: + u32 mask, u32 val) +{ + u32 reg_val; + + reg_val = readl(base + reg); + reg_val &= ~mask; + reg_val |= FIELD_PREP(mask, val); + writel(reg_val, base + reg); bypassing regmap here... why? It is not regmap address, one of the below two addresses are passed to this function. okay, perhaps add a comment somewhere that regmap is not used for this base? I dont see a need of adding a comment, describing don't do regmap here. Driver uses regmap except here, which seems odd hence explanation required for this. During the driver Probe, the register phandles are stored in regmap datatype variables and PHY core addresses are stored in iomem datatype. Since then, regmap access is performed for the regmap datatype variables and readl/writel access is performed on the iomem datatype variables. And nowhere in the driver iomem datatype address are converted to regmap address and performed regmap access. Driver is not doing any 'regmap_init' on any physical address. Driver is getting the register address phandle from the device tree node and performing the regmap access. ret = fwnode_property_get_reference_args(fwnode, "intel,syscfg", NULL, 1, 0, ); [...] cbphy->syscfg = device_node_to_regmap(to_of_node(ref.fwnode)); [...] ret = fwnode_property_get_reference_args(fwnode, "intel,hsio", NULL, 1, 0, ); [...] cbphy->hsiocfg = device_node_to_regmap(to_of_node(ref.fwnode)); [...] cbphy->app_base = devm_platform_ioremap_resource_byname(pdev, "app"); [...] cbphy->cr_base = devm_platform_ioremap_resource_byname(pdev, "core"); The DT parsing logic in the driver is explaining why the PHY driver should do regmap access and to whom should be done. For this reason i am a bit puzzled to what more is needed to explain in the comments and where to add it. Please let me know your view. Regards, Dilip
Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers
On 4/29/2020 8:13 PM, Mark Brown wrote: On Wed, Apr 29, 2020 at 04:20:53PM +0800, Dilip Kota wrote: On 4/28/2020 7:10 PM, Daniel Schwierzeck wrote: actually there is no real bottom half. Reading or writing the FIFOs is fast and is therefore be done in hard IRQ context. But as the comment Doing FIFO r/w in threaded irqs shouldn't cause any impact on maximum transfer rate i think. Have you actually tested this? Generally adding extra latency is going to lead to some opportunity for the hardware to idle and the longer the hardware is idle the lower the throughput. Also the ISR should be quick enough, doing FIFO r/w in ISR adds up more latency to ISR. Handling the FIFOs r/w in threaded irq will be a better way. Consider what happens on a heavily loaded system - the threaded interrupt will have to be scheduled along with other tasks. for lantiq_ssc_bussy_work() state, the driver needs some busy-waiting after the last interrupt. I don't think it's worth to replace this with threaded interrupts which add more runtime overhead and likely decrease the maximum transfer speed. Workqueue has a higher chances of causing SPI transfers timedout. because...? I just tried to get the history of removing workqueue in SPI driver, on GRX500 (earlier chipset of LGM) the SPI transfers got timedout with workqueues during regression testing. Once changed to threaded IRQs transfers are working successfully. Regards, Dilip
Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy
On 5/4/2020 5:20 PM, Vinod Koul wrote: On 04-05-20, 16:26, Dilip Kota wrote: On 5/4/2020 3:29 PM, Vinod Koul wrote: On 30-04-20, 15:15, Dilip Kota wrote: + u32 mask, u32 val) +{ + u32 reg_val; + + reg_val = readl(base + reg); + reg_val &= ~mask; + reg_val |= FIELD_PREP(mask, val); + writel(reg_val, base + reg); bypassing regmap here... why? It is not regmap address, one of the below two addresses are passed to this function. okay, perhaps add a comment somewhere that regmap is not used for this base? I dont see a need of adding a comment, describing don't do regmap here. struct intel_combo_phy { ... void __iomem *app_base; void __iomem *cr_base; ... } +static int intel_cbphy_calibrate(struct phy *phy) +{ + struct intel_cbphy_iphy *iphy = phy_get_drvdata(phy); + struct intel_combo_phy *cbphy = iphy->parent; + void __iomem *cr_base = cbphy->cr_base; + int val, ret, id; + + if (cbphy->phy_mode != PHY_XPCS_MODE) + return 0; + + id = PHY_ID(iphy); + + /* trigger auto RX adaptation */ + combo_phy_w32_off_mask(cr_base, CR_ADDR(PCS_XF_ATE_OVRD_IN_2, id), + ADAPT_REQ_MSK, 3); + /* Wait RX adaptation to finish */ + ret = readl_poll_timeout(cr_base + CR_ADDR(PCS_XF_RX_ADAPT_ACK, id), +val, val & RX_ADAPT_ACK_BIT, 10, 5000); + if (ret) + dev_err(cbphy->dev, "RX Adaptation failed!\n"); you want to continue her and not return error? Next step is stopping the Adaptation, it should be done in both error and success case. Again documenting this helps, pls add some comments on this behaviour Comments are already in place, mentioning Start and Stop of Rx Adaptation. And Stop is being is done as Start is triggered, so not needed to mention error and success. Regards, Dilip
Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy
On 5/4/2020 3:29 PM, Vinod Koul wrote: On 30-04-20, 15:15, Dilip Kota wrote: +enum { + PHY_0, + PHY_1, + PHY_MAX_NUM PHY_MAX_NUM = PHY_1? Driver is using it for no. of PHYs/maximum PHY id. Code snippets: struct intel_combo_phy { ... struct reset_control *phy_rst; struct reset_control *core_rst; struct intel_cbphy_iphy iphy[PHY_MAX_NUM]; ... } static int intel_cbphy_create(struct intel_combo_phy *cbphy) { struct phy_provider *phy_provider; struct device *dev = cbphy->dev; struct intel_cbphy_iphy *iphy; int i; for (i = 0; i < PHY_MAX_NUM; i++) { ... +static inline void combo_phy_w32_off_mask(void __iomem *base, unsigned int reg, + u32 mask, u32 val) +{ + u32 reg_val; + + reg_val = readl(base + reg); + reg_val &= ~mask; + reg_val |= FIELD_PREP(mask, val); + writel(reg_val, base + reg); bypassing regmap here... why? It is not regmap address, one of the below two addresses are passed to this function. struct intel_combo_phy { ... void __iomem *app_base; void __iomem *cr_base; ... } +static int intel_cbphy_calibrate(struct phy *phy) +{ + struct intel_cbphy_iphy *iphy = phy_get_drvdata(phy); + struct intel_combo_phy *cbphy = iphy->parent; + void __iomem *cr_base = cbphy->cr_base; + int val, ret, id; + + if (cbphy->phy_mode != PHY_XPCS_MODE) + return 0; + + id = PHY_ID(iphy); + + /* trigger auto RX adaptation */ + combo_phy_w32_off_mask(cr_base, CR_ADDR(PCS_XF_ATE_OVRD_IN_2, id), + ADAPT_REQ_MSK, 3); + /* Wait RX adaptation to finish */ + ret = readl_poll_timeout(cr_base + CR_ADDR(PCS_XF_RX_ADAPT_ACK, id), +val, val & RX_ADAPT_ACK_BIT, 10, 5000); + if (ret) + dev_err(cbphy->dev, "RX Adaptation failed!\n"); you want to continue her and not return error? Next step is stopping the Adaptation, it should be done in both error and success case. Regards, Dilip
Re: [PATCH v7 0/3] Add Intel ComboPhy driver
On 4/30/2020 4:25 PM, Lee Jones wrote: On Thu, 30 Apr 2020, Dilip Kota wrote: This patch series adds Intel ComboPhy driver, respective yaml schemas Changes on v7: As per System control driver maintainer's inputs remove fwnode_to_regmap() definition and use device_node_get_regmap() Changes on v6: Rebase patches on the latest maintainer's branch https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git/?h=phy-for-5.7 Dilip Kota (3): dt-bindings: phy: Add PHY_TYPE_XPCS definition dt-bindings: phy: Add YAML schemas for Intel ComboPhy phy: intel: Add driver support for ComboPhy .../devicetree/bindings/phy/intel,combo-phy.yaml | 101 drivers/phy/intel/Kconfig | 14 + drivers/phy/intel/Makefile | 1 + drivers/phy/intel/phy-intel-combo.c| 627 + include/dt-bindings/phy/phy.h | 1 + Why have you sent this to me? The main reason for this patch series is removing fwnode_to_regmap() and using device_node_get_regmap() compared to previous patch series. This has been done as per your review comments. To keep you updated that changes are done as per your review comments, sent to you along with PHY maintainers. Regards, Dilip 5 files changed, 744 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml create mode 100644 drivers/phy/intel/phy-intel-combo.c
[PATCH v7 0/3] Add Intel ComboPhy driver
This patch series adds Intel ComboPhy driver, respective yaml schemas Changes on v7: As per System control driver maintainer's inputs remove fwnode_to_regmap() definition and use device_node_get_regmap() Changes on v6: Rebase patches on the latest maintainer's branch https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git/?h=phy-for-5.7 Dilip Kota (3): dt-bindings: phy: Add PHY_TYPE_XPCS definition dt-bindings: phy: Add YAML schemas for Intel ComboPhy phy: intel: Add driver support for ComboPhy .../devicetree/bindings/phy/intel,combo-phy.yaml | 101 drivers/phy/intel/Kconfig | 14 + drivers/phy/intel/Makefile | 1 + drivers/phy/intel/phy-intel-combo.c| 627 + include/dt-bindings/phy/phy.h | 1 + 5 files changed, 744 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml create mode 100644 drivers/phy/intel/phy-intel-combo.c -- 2.11.0
[PATCH v7 2/3] dt-bindings: phy: Add YAML schemas for Intel ComboPhy
ComboPhy subsystem provides PHY support to various controllers, viz. PCIe, SATA and EMAC. Adding YAML schemas for the same. Signed-off-by: Dilip Kota Reviewed-by: Rob Herring --- Changes on v7: No Change. Changes on v6: Add Reviewed-by: Rob Herring .../devicetree/bindings/phy/intel,combo-phy.yaml | 101 + 1 file changed, 101 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml diff --git a/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml new file mode 100644 index ..347d0cdfb80d --- /dev/null +++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml @@ -0,0 +1,101 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/phy/intel,combo-phy.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Intel ComboPhy Subsystem + +maintainers: + - Dilip Kota + +description: | + Intel Combophy subsystem supports PHYs for PCIe, EMAC and SATA + controllers. A single Combophy provides two PHY instances. + +properties: + $nodename: +pattern: "combophy(@.*|-[0-9a-f])*$" + + compatible: +items: + - const: intel,combophy-lgm + - const: intel,combo-phy + + clocks: +maxItems: 1 + + reg: +items: + - description: ComboPhy core registers + - description: PCIe app core control registers + + reg-names: +items: + - const: core + - const: app + + resets: +maxItems: 4 + + reset-names: +items: + - const: phy + - const: core + - const: iphy0 + - const: iphy1 + + intel,syscfg: +$ref: /schemas/types.yaml#/definitions/phandle-array +description: Chip configuration registers handle and ComboPhy instance id + + intel,hsio: +$ref: /schemas/types.yaml#/definitions/phandle-array +description: HSIO registers handle and ComboPhy instance id on NOC + + intel,aggregation: +type: boolean +description: | + Specify the flag to configure ComboPHY in dual lane mode. + + intel,phy-mode: +$ref: /schemas/types.yaml#/definitions/uint32 +description: | + Mode of the two phys in ComboPhy. + See dt-bindings/phy/phy.h for values. + + "#phy-cells": +const: 1 + +required: + - compatible + - clocks + - reg + - reg-names + - intel,syscfg + - intel,hsio + - intel,phy-mode + - "#phy-cells" + +additionalProperties: false + +examples: + - | +#include +combophy@d0a0 { +compatible = "intel,combophy-lgm", "intel,combo-phy"; +clocks = < 1>; +#phy-cells = <1>; +reg = <0xd0a0 0x4>, + <0xd0a4 0x1000>; +reg-names = "core", "app"; +resets = < 0x50 6>, + < 0x50 17>, + < 0x50 23>, + < 0x50 24>; +reset-names = "phy", "core", "iphy0", "iphy1"; +intel,syscfg = < 0>; +intel,hsio = < 0>; +intel,phy-mode = ; +intel,aggregation; +}; -- 2.11.0
[PATCH v7 3/3] phy: intel: Add driver support for ComboPhy
ComboPhy subsystem provides PHYs for various controllers like PCIe, SATA and EMAC. Signed-off-by: Dilip Kota --- Changes on v7: Use device_node_to_regmap instead of fwnode_to_regmap Changes on v6: No changes Changes on v5: Add changes as per inputs from Andy and Rob: DT node uses phy-mode values as defined in "include/dt-bindings/phy/phy.h", add changes to handle it. ComboPhy no longer has children nodes, and children node properties(reset) moved to parent node, so do the code changes accordingly. Add _xlate() function to pass the appropriate phy handle. Fix couple of nitpicks. Changes on v4: Address review comments Remove dependency on OF config Update copyright to 2019-2020 Define register macro PAD_DIS_CFG instead of const variable inside function. Improve the error prints, and error returns. Call put_device(dev), for get_dev_from_fwnode() Move platform_set_drvdata() at the end of the probe(). Correct alignment in phy_ops intel_cbphy_ops. Correct commented lines with proper vocabulary and punctuation. Add/remove commas for the required constant arrays and enums. Remove in driver: linux/kernel.h, not required macros: PCIE_PHY_MPLLA_CTRL, PCIE_PHY_MPLLB_CTRL temp variable u32 prop; Change function names: intel_cbphy_iphy_dt_parse() -> intel_cbphy_iphy_fwnode_parse() intel_cbphy_dt_sanity_check() -> intel_cbphy_sanity_check() intel_cbphy_dt_parse() -> intel_cbphy_fwnode_parse() Changes on v3: Remove intel_iphy_names Remove struct phy in struct intel_cbphy_iphy Imporve if conditions logic Use fwnode_to_regmap() Call devm_of_platform_populate() to populate child nodes Fix reset sequence during phy_init Add SoC specific compatible "intel,combophy-lgm" Add description for enums Remove default case in switch {} intel_cbphy_set_mode() as it never happens. Use mutex_lock to synchronise combophy initialization across two phys. Change init_cnt to u32 datatype as it is within mutex lock. Correct error handling of fwnode_property_read_u32_array(fwnode, "intel,phy-mode", ...) drivers/phy/intel/Kconfig | 14 + drivers/phy/intel/Makefile | 1 + drivers/phy/intel/phy-intel-combo.c | 627 3 files changed, 642 insertions(+) create mode 100644 drivers/phy/intel/phy-intel-combo.c diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig index 4ea6a8897cd7..3b40eb7b4fb4 100644 --- a/drivers/phy/intel/Kconfig +++ b/drivers/phy/intel/Kconfig @@ -2,6 +2,20 @@ # # Phy drivers for Intel Lightning Mountain(LGM) platform # +config PHY_INTEL_COMBO + bool "Intel ComboPHY driver" + depends on X86 || COMPILE_TEST + depends on OF && HAS_IOMEM + select MFD_SYSCON + select GENERIC_PHY + select REGMAP + help + Enable this to support Intel ComboPhy. + + This driver configures ComboPhy subsystem on Intel gateway + chipsets which provides PHYs for various controllers, EMAC, + SATA and PCIe. + config PHY_INTEL_EMMC tristate "Intel EMMC PHY driver" select GENERIC_PHY diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile index 6b876a75599d..233d530dadde 100644 --- a/drivers/phy/intel/Makefile +++ b/drivers/phy/intel/Makefile @@ -1,2 +1,3 @@ # SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_PHY_INTEL_COMBO) += phy-intel-combo.o obj-$(CONFIG_PHY_INTEL_EMMC)+= phy-intel-emmc.o diff --git a/drivers/phy/intel/phy-intel-combo.c b/drivers/phy/intel/phy-intel-combo.c new file mode 100644 index ..04ad595e21e4 --- /dev/null +++ b/drivers/phy/intel/phy-intel-combo.c @@ -0,0 +1,627 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Intel Combo-PHY driver + * + * Copyright (C) 2019-2020 Intel Corporation. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#define PCIE_PHY_GEN_CTRL 0x00 +#define PCIE_PHY_CLK_PAD BIT(17) + +#define PAD_DIS_CFG0x174 + +#define PCS_XF_ATE_OVRD_IN_2 0x3008 +#define ADAPT_REQ_MSK GENMASK(5, 4) + +#define PCS_XF_RX_ADAPT_ACK0x3010 +#define RX_ADAPT_ACK_BIT BIT(0) + +#define CR_ADDR(addr, lane)(((addr) + (lane) * 0x100) << 2) +#define REG_COMBO_MODE(x) ((x) * 0x200) +#define REG_CLK_DISABLE(x) ((x) * 0x200 + 0x124) + +#define COMBO_PHY_ID(x)((x)->parent->id) +#define PHY_ID(x) ((x)->id) + +#define CLK_100MHZ 1 +#define CLK_156_25MHZ 15625 + +static const unsigned long intel_iphy_clk_rates[] = { + CLK_100MHZ, CLK_156_25MHZ, CLK_100MHZ, +}; + +enum { + PHY_0, + PHY_1, + PHY_MAX_NUM +}; + +/* + * Clock Register bit fields to enable clocks + * for ComboPhy according to the mode. + */ +enum intel_
[PATCH v7 1/3] dt-bindings: phy: Add PHY_TYPE_XPCS definition
Add definition for Ethernet PCS phy type. Signed-off-by: Dilip Kota Acked-by: Rob Herring --- Changes on v7: No Change Changes on v6: Add Acked-by: Rob Herring include/dt-bindings/phy/phy.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/dt-bindings/phy/phy.h b/include/dt-bindings/phy/phy.h index 1f3f866fae7b..3727ef72138b 100644 --- a/include/dt-bindings/phy/phy.h +++ b/include/dt-bindings/phy/phy.h @@ -17,5 +17,6 @@ #define PHY_TYPE_USB3 4 #define PHY_TYPE_UFS 5 #define PHY_TYPE_DP6 +#define PHY_TYPE_XPCS 7 #endif /* _DT_BINDINGS_PHY */ -- 2.11.0
Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers
On 4/28/2020 7:30 PM, Hauke Mehrtens wrote: On 4/28/20 1:10 PM, Daniel Schwierzeck wrote: Am 24.04.20 um 12:42 schrieb Dilip Kota: ... Hi, The Interrupt controller found on Danube till xrx300 which is probably from Infineon like this SPI controller IP acknowledges the interrupts also inside this SPI controller IP automatically, this has to be done manually on the xrx500 and probably also LGM as they use a different interrupt controller. I prepared patches for this internally 2.5 years ago but did not send them upstream because of internal processes. I would suggest to only do this ack on the newer platforms starting with the xrx500 and not on the older. On SMP systems a lock is needed in lantiq_ssc_xmit_interrupt() to protect against an other thread reading from the RX buffer or writing to the TX buffer in parallel. @Dilip. Did you try the patches I send you one months ago on the LGM? All the cases you mentioned are taken care in the patch, could you please have a look once. And the patch you shared internally, has done below change. By referring it i have updated the offsets, mentioning offsets are wrong. But actual case is vrx200 are having different offsets and xrx500, latest chipsets are having different offsets. I think this could be the reason for SPI transfer timeouts when you run test on vrx200 with my patches. -#define LTQ_SPI_IRNICR 0xf8 -#define LTQ_SPI_IRNCR 0xfc +#define LTQ_SPI_IRNCR 0xf8 +#define LTQ_SPI_IRNICR 0xfc These offsets need to be defined in SoC data structure as they are different across the chipsets(which i have done in initial phase of the patch which i submitted for internal review. I hope you had got a chance to review it). Regards, Dilip
Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers
On 4/28/2020 7:10 PM, Daniel Schwierzeck wrote: Am 24.04.20 um 12:42 schrieb Dilip Kota: Synchronize tx, rx and error interrupts by registering to the same interrupt handler. Interrupt handler will recognize and process the appropriate interrupt on the basis of interrupt status register. Also, establish synchronization between the interrupt handler and transfer operation by taking the locks and registering the interrupt handler as thread IRQ which avoids the bottom half. actually there is no real bottom half. Reading or writing the FIFOs is fast and is therefore be done in hard IRQ context. But as the comment Doing FIFO r/w in threaded irqs shouldn't cause any impact on maximum transfer rate i think. Also the ISR should be quick enough, doing FIFO r/w in ISR adds up more latency to ISR. Handling the FIFOs r/w in threaded irq will be a better way. for lantiq_ssc_bussy_work() state, the driver needs some busy-waiting after the last interrupt. I don't think it's worth to replace this with threaded interrupts which add more runtime overhead and likely decrease the maximum transfer speed. Workqueue has a higher chances of causing SPI transfers timedout. Fixes the wrongly populated interrupt register offsets too. Fixes: 17f84b793c01 ("spi: lantiq-ssc: add support for Lantiq SSC SPI controller") Fixes: ad2fca0721d1 ("spi: lantiq-ssc: add LTQ_ prefix to defines") Signed-off-by: Dilip Kota --- drivers/spi/spi-lantiq-ssc.c | 89 ++-- 1 file changed, 45 insertions(+), 44 deletions(-) diff --git a/drivers/spi/spi-lantiq-ssc.c b/drivers/spi/spi-lantiq-ssc.c index 1fd7ee53d451..b67f5925bcb0 100644 --- a/drivers/spi/spi-lantiq-ssc.c +++ b/drivers/spi/spi-lantiq-ssc.c @@ -6,6 +6,7 @@ #include #include +#include #include #include #include @@ -13,7 +14,6 @@ #include #include #include -#include #include #include #include @@ -50,8 +50,8 @@ #define LTQ_SPI_RXCNT 0x84 #define LTQ_SPI_DMACON0xec #define LTQ_SPI_IRNEN 0xf4 -#define LTQ_SPI_IRNICR 0xf8 -#define LTQ_SPI_IRNCR 0xfc +#define LTQ_SPI_IRNCR 0xf8 +#define LTQ_SPI_IRNICR 0xfc the values are matching the datasheets for Danube and VRX200 family. AFAICS the registers have been swapped for some newer SoCs like GRX330 or GRX550. It didn't matter until now because those registers were unused by the driver. So if you want to use those registers, you have to deal somehow with the register offset swap in struct lantiq_ssc_hwcfg. In the initial phase of the patch, i have written the code considering the interrupt offsets in latest chipsets are different from the older chipsets. But, when i refered the Hauke's changes to add support for xrx500(which he done internally), offsets are corrected . So i did the same. I will define these offsets in the SoC data structure which i have done initially. Regards, Dilip
Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers
On 4/28/2020 6:00 PM, Mark Brown wrote: On Tue, Apr 28, 2020 at 01:39:06PM +0800, Dilip Kota wrote: Do you suggest to use different ISRs for multiple interrupt lines and single ISR for single interrupt line? I see, this results in writing repetitive code lines. It looks like the shared case is mainly a handler that calls the two other handlers? Yes. Does single ISR looks erroneous! Please let me know. The change was not entirely clear, I was having trouble convincing myself that all the transformations were OK partly because I kept on finding little extra changes in there and partly because there were several things going on. In theory it could work. You want me to split this in to multiple patches? Regards, Dilip
Re: [RESEND PATCH v6 1/4] mfd: syscon: Add fwnode_to_regmap
On 4/28/2020 6:29 PM, Arnd Bergmann wrote: On Tue, Apr 28, 2020 at 12:05 PM Lee Jones wrote: On Tue, 21 Apr 2020, Dilip Kota wrote: But, i feel return error for ACPI or oother, looks better because 'device_node' has fwnode pointer. And provide description in the header file, mentioning function is success for 'OF' and returns error for the rest. I don't think this patch adds much to be honest. Better to just do: device_node_get_regmap(to_of_node(fwnode), false); ... from the call site I think. Agreed, or just use the of_node pointer consistently in the driver that uses it and avoid the use of the fwnode interface entirely when dealing with a modern driver that does not need to support board files any more. Arnd Ok, I will do it in the driver itself and remove the fwnode_to_regmap() definition. Regards, Dilip
Re: [PATCH v4 2/3] dwc: PCI: intel: PCIe RC controller driver
Hi Gustavo Pimentel, On 10/21/2019 6:44 PM, Dilip Kota wrote: Hi Gustavo Pimentel, On 10/21/2019 4:29 PM, Gustavo Pimentel wrote: Hi On Mon, Oct 21, 2019 at 7:39:19, Dilip Kota wrote: Add support to PCIe RC controller on Intel Gateway SoCs. PCIe controller is based of Synopsys DesignWare pci core. Intel PCIe driver requires Upconfig support, fast training sequence configuration and link speed change. So adding the respective helper functions in the pcie DesignWare framework. It also programs hardware autonomous speed during speed configuration so defining it in pci_regs.h. Please do the replacement in all of your patches s/pcie/PCIe s/pci/PCI Also I think the correct term is Upconfigure and not Upconfig Yes, i will update it. Changes on v4: Rename the driver naming and description to "PCIe RC controller on Intel Gateway SoCs". Use PCIe core register macros defined in pci_regs.h and remove respective local definitions. Remove pcie core interrupt handling. Move pcie link control speed change, upconfig and FTS. configuration functions to DesignWare framework. Use of_pci_get_max_link_speed(). Mark dependency on X86 and COMPILE_TEST in Kconfig. Remove lanes and add n_fts variables in intel_pcie_port structure. Rename rst_interval variable to rst_intrvl in intel_pcie_port structure. Remove intel_pcie_mem_iatu() as it is already perfomed in dw_setup_rc() Move sysfs attributes specific code to separate patch. Remove redundant error handling. Reduce LoCs by doing variable initializations while declaration itself. Add extra line after closing parenthesis. Move intel_pcie_ep_rst_init() out of get_resources() 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() Signed-off-by: Dilip Kota --- drivers/pci/controller/dwc/Kconfig | 10 + drivers/pci/controller/dwc/Makefile | 1 + drivers/pci/controller/dwc/pcie-designware.c | 34 ++ drivers/pci/controller/dwc/pcie-designware.h | 12 + drivers/pci/controller/dwc/pcie-intel-gw.c | 590 +++ include/uapi/linux/pci_regs.h | 1 + 6 files changed, 648 insertions(+) create mode 100644 drivers/pci/controller/dwc/pcie-intel-gw.c diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig index 0ba988b5b5bc..b33ed1cc873d 100644 --- a/drivers/pci/controller/dwc/Kconfig +++ b/drivers/pci/controller/dwc/Kconfig @@ -82,6 +82,16 @@ config PCIE_DW_PLAT_EP order to enable device-specific features PCI_DW_PLAT_EP must be selected. +config PCIE_INTEL_GW + bool "Intel Gateway PCIe host controller support" + depends on OF && (X86 || COMPILE_TEST) + select PCIE_DW_HOST + help + Say 'Y' here to enable support for PCIe Host controller driver. + The PCIe controller on Intel Gateway SoCs is based on the Synopsys + DesignWare pcie core and therefore uses the DesignWare core + functions for the driver implementation. + 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 b30336181d46..99db83cd2f35 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_GW) += pcie-intel-gw.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/controlle
Re: [PATCH v4 1/3] dt-bindings: PCI: intel: Add YAML schemas for the PCIe RC controller
Hi Andrew Murray, On 10/21/2019 7:19 PM, Andrew Murray wrote: On Mon, Oct 21, 2019 at 02:39:18PM +0800, Dilip Kota wrote: Add YAML shcemas for PCIe RC controller on Intel Gateway SoCs s/shcemas/schemas/ which is Synopsys DesignWare based PCIe core. The revision history below doesn't need to be in the commit mesage and so you should add a '---' before the following (and thanks for the detailed history). Besides that: Reviewed-by: Andrew Murray Thank you for the review. I will fix the conventions Regards, Dilip changes on v4: Add "snps,dw-pcie" compatible. Rename phy-names property value to pcie. And maximum and minimum values to num-lanes. Add ref for reset-assert-ms entry and update the description for easy understanding. Remove pcie core interrupt entry. changes on v3: Add the appropriate License-Identifier Rename intel,rst-interval to 'reset-assert-us' 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 Signed-off-by: Dilip Kota --- .../devicetree/bindings/pci/intel-gw-pcie.yaml | 135 + 1 file changed, 135 insertions(+) create mode 100644 Documentation/devicetree/bindings/pci/intel-gw-pcie.yaml diff --git a/Documentation/devicetree/bindings/pci/intel-gw-pcie.yaml b/Documentation/devicetree/bindings/pci/intel-gw-pcie.yaml new file mode 100644 index ..49dd87ec1e3d --- /dev/null +++ b/Documentation/devicetree/bindings/pci/intel-gw-pcie.yaml @@ -0,0 +1,135 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pci/intel-gw-pcie.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: PCIe RC controller on Intel Gateway SoCs + +maintainers: + - Dilip Kota + +properties: + compatible: +items: + - const: intel,lgm-pcie + - const: snps,dw-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: pcie + + reset-gpios: +maxItems: 1 + + num-lanes: +minimum: 1 +maximum: 2 +description: Number of lanes to use for this port. + + linux,pci-domain: +$ref: /schemas/types.yaml#/definitions/uint32 +description: PCI domain ID. + + '#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: +$ref: /schemas/types.yaml#/definitions/uint32 +description: | + Delay after asserting reset to the PCIe device. + 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 + - interrupt-map + - interrupt-map-mask + +additionalProperties: false + +examples: + - | +pcie10:pcie@d0e0 { + compatible = "intel,lgm-pcie", "snps,dw-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 = <>; + #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 = "pcie"; + status = "okay"; + reset-assert-ms = <500>; + reset-gpios = < 3 GPIO_ACTIVE_LOW>; + num-lanes = <2>; +}; -- 2.11.0
Re: [PATCH v4 3/3] pci: intel: Add sysfs attributes to configure pcie link
Hi Bjorn Helgaas, On 10/22/2019 1:18 AM, Bjorn Helgaas wrote: On Mon, Oct 21, 2019 at 02:38:50PM +0100, Andrew Murray wrote: On Mon, Oct 21, 2019 at 02:39:20PM +0800, Dilip Kota wrote: PCIe RC driver on Intel Gateway SoCs have a requirement of changing link width and speed on the fly. Please add more details about why this is needed. Since you're adding sysfs files, it sounds like it's not actually the *driver* that needs this; it's something in userspace? We have use cases to change the link speed and width on the fly. One is EMI check and other is power saving. Some battery backed applications have to switch PCIe link from higher GEN to GEN1 and width to x1. During the cases like external power supply got disconnected or broken. Once external power supply is connected then switch PCIe link to higher GEN and width. The normal scenario is that the hardware negotiates link widths and speeds without any software involvement (PCIe r5.0, sec 1.2). If this is to work around hardware defects, we should try to do that inside the kernel because we can't expect userspace to do it reliably. As Andrew points out below, this all sounds like it should be generic rather than Intel-specific. So add the sysfs attributes to show and store the link properties. Add the respective link resize function in pcie DesignWare framework so that Intel PCIe driver can use during link width configuration on the fly. ... +static ssize_t pcie_link_status_show(struct device *dev, +struct device_attribute *attr, char *buf) +{ + struct intel_pcie_port *lpp = dev_get_drvdata(dev); + u32 reg, width, gen; + + reg = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL); + width = FIELD_GET(PCI_EXP_LNKSTA_NLW, reg >> 16); + gen = FIELD_GET(PCI_EXP_LNKSTA_CLS, reg >> 16); + + if (gen > lpp->max_speed) + return -EINVAL; + + return sprintf(buf, "Port %2u Width x%u Speed %s GT/s\n", lpp->id, + width, pcie_link_gen_to_str(gen)); +} +static DEVICE_ATTR_RO(pcie_link_status); We already have generic current_link_speed and current_link_width files. Thanks for pointing it. I will remove the pcie_link_status. Regards, Dilip +static ssize_t pcie_speed_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t len) +{ + struct intel_pcie_port *lpp = dev_get_drvdata(dev); + unsigned long val; + int ret; + + ret = kstrtoul(buf, 10, ); + if (ret) + return ret; + + if (val > lpp->max_speed) + return -EINVAL; + + lpp->link_gen = val; + intel_pcie_max_speed_setup(lpp); + dw_pcie_link_speed_change(>pci, false); + dw_pcie_link_speed_change(>pci, true); + + return len; +} +static DEVICE_ATTR_WO(pcie_speed); + +/* + * Link width change on the fly is not always successful. + * It also depends on the partner. + */ +static ssize_t pcie_width_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t len) +{ + struct intel_pcie_port *lpp = dev_get_drvdata(dev); + unsigned long val; + int ret; + + lpp = dev_get_drvdata(dev); + + ret = kstrtoul(buf, 10, ); + if (ret) + return ret; + + if (val > lpp->max_width) + return -EINVAL; + + /* HW auto bandwidth negotiation must be enabled */ + pcie_rc_cfg_wr_mask(lpp, PCI_EXP_LNKCTL_HAWD, 0, + PCIE_CAP_OFST + PCI_EXP_LNKCTL); + dw_pcie_link_width_resize(>pci, val); + + return len; +} +static DEVICE_ATTR_WO(pcie_width); + +static struct attribute *pcie_cfg_attrs[] = { + _attr_pcie_link_status.attr, + _attr_pcie_speed.attr, + _attr_pcie_width.attr, + NULL, +}; Is there a reason that these are limited only to the Intel driver and not the wider set of DWC drivers? Is there anything specific here about the Intel GW driver?
Re: [PATCH v4 3/3] pci: intel: Add sysfs attributes to configure pcie link
Hi Andrew Murray, On 10/21/2019 9:38 PM, Andrew Murray wrote: On Mon, Oct 21, 2019 at 02:39:20PM +0800, Dilip Kota wrote: PCIe RC driver on Intel Gateway SoCs have a requirement of changing link width and speed on the fly. So add the sysfs attributes to show and store the link properties. Add the respective link resize function in pcie DesignWare framework so that Intel PCIe driver can use during link width configuration on the fly. Signed-off-by: Dilip Kota --- drivers/pci/controller/dwc/pcie-designware.c | 9 +++ drivers/pci/controller/dwc/pcie-designware.h | 3 + drivers/pci/controller/dwc/pcie-intel-gw.c | 112 ++- 3 files changed, 123 insertions(+), 1 deletion(-) diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c index 4c391bfd681a..662fdcb4f2d6 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -474,6 +474,15 @@ int dw_pcie_link_up(struct dw_pcie *pci) (!(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING))); } +void dw_pcie_link_width_resize(struct dw_pcie *pci, u32 lane_width) +{ + u32 val; + + val = dw_pcie_readl_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL); + val &= ~(PORT_MLTI_LNK_WDTH_CHNG | PORT_MLTI_LNK_WDTH); + val |= PORT_MLTI_LNK_WDTH_CHNG | lane_width; + dw_pcie_writel_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL, val); +} void dw_pcie_upconfig_setup(struct dw_pcie *pci) { diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index 3beac10e4a4c..fcf0442341fd 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -67,6 +67,8 @@ #define PCIE_MSI_INTR0_STATUS 0x830 #define PCIE_PORT_MULTI_LANE_CTRL 0x8C0 +#define PORT_MLTI_LNK_WDTH GENMASK(5, 0) +#define PORT_MLTI_LNK_WDTH_CHNGBIT(6) #define PORT_MLTI_UPCFG_SUPPORT BIT(7) #define PCIE_ATU_VIEWPORT 0x900 @@ -282,6 +284,7 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val); u32 dw_pcie_read_atu(struct dw_pcie *pci, u32 reg, size_t size); void dw_pcie_write_atu(struct dw_pcie *pci, u32 reg, size_t size, u32 val); int dw_pcie_link_up(struct dw_pcie *pci); +void dw_pcie_link_width_resize(struct dw_pcie *pci, u32 lane_width); void dw_pcie_upconfig_setup(struct dw_pcie *pci); void dw_pcie_link_speed_change(struct dw_pcie *pci, bool enable); void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts); diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c index 9142c70db808..b9be0921671d 100644 --- a/drivers/pci/controller/dwc/pcie-intel-gw.c +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c @@ -146,6 +146,22 @@ static void intel_pcie_ltssm_disable(struct intel_pcie_port *lpp) pcie_app_wr_mask(lpp, PCIE_APP_CCR_LTSSM_ENABLE, 0, PCIE_APP_CCR); } +static const char *pcie_link_gen_to_str(int gen) +{ + switch (gen) { + case PCIE_LINK_SPEED_GEN1: + return "2.5"; + case PCIE_LINK_SPEED_GEN2: + return "5.0"; + case PCIE_LINK_SPEED_GEN3: + return "8.0"; + case PCIE_LINK_SPEED_GEN4: + return "16.0"; + default: + return "???"; + } +} + static void intel_pcie_link_setup(struct intel_pcie_port *lpp) { u32 val; @@ -444,6 +460,91 @@ static int intel_pcie_host_setup(struct intel_pcie_port *lpp) return ret; } +static ssize_t pcie_link_status_show(struct device *dev, +struct device_attribute *attr, char *buf) +{ + struct intel_pcie_port *lpp = dev_get_drvdata(dev); + u32 reg, width, gen; + + reg = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL); + width = FIELD_GET(PCI_EXP_LNKSTA_NLW, reg >> 16); + gen = FIELD_GET(PCI_EXP_LNKSTA_CLS, reg >> 16); + + if (gen > lpp->max_speed) + return -EINVAL; + + return sprintf(buf, "Port %2u Width x%u Speed %s GT/s\n", lpp->id, + width, pcie_link_gen_to_str(gen)); +} +static DEVICE_ATTR_RO(pcie_link_status); + +static ssize_t pcie_speed_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t len) +{ + struct intel_pcie_port *lpp = dev_get_drvdata(dev); + unsigned long val; + int ret; + + ret = kstrtoul(buf, 10, ); + if (ret) + return ret; + + if (val > lpp->max_speed) + return -EINVAL; + + lpp->link_gen = val; + intel_pcie_max_speed_setup(lpp); + dw_pcie_link_speed_change(>pci, false); + dw_pcie_link_speed_change(>pci, true); + + return len
Re: [PATCH v4 2/3] dwc: PCI: intel: PCIe RC controller driver
Hi Bjorn Helgaas, On 10/22/2019 1:17 AM, Bjorn Helgaas wrote: On Mon, Oct 21, 2019 at 02:39:19PM +0800, Dilip Kota wrote: Add support to PCIe RC controller on Intel Gateway SoCs. PCIe controller is based of Synopsys DesignWare pci core. Intel PCIe driver requires Upconfig support, fast training sequence configuration and link speed change. So adding the respective helper functions in the pcie DesignWare framework. It also programs hardware autonomous speed during speed configuration so defining it in pci_regs.h. +static void intel_pcie_link_setup(struct intel_pcie_port *lpp) +{ + u32 val; + + val = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCAP); + lpp->max_speed = FIELD_GET(PCI_EXP_LNKCAP_SLS, val); + lpp->max_width = FIELD_GET(PCI_EXP_LNKCAP_MLW, val); + + val = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL); + + val &= ~(PCI_EXP_LNKCTL_LD | PCI_EXP_LNKCTL_ASPMC); + val |= (PCI_EXP_LNKSTA_SLC << 16) | PCI_EXP_LNKCTL_CCC | + PCI_EXP_LNKCTL_RCB; Link Control is only 16 bits wide, so "PCI_EXP_LNKSTA_SLC << 16" wouldn't make sense. But I guess you're writing a device-specific register that is not actually the Link Control as documented in PCIe r5.0, sec 7.5.3.7, even though the bits are similar? It is not device specific. You are correct about register size that pcie spec mentions PCIE_EXP_LNKCTL at 0x10 and PCIE_EXP_LNKSTS at 0x12 each of 2bytes. Accessing 4bytes at offset 0x10 ends up accessing LNK control and status register. In Synopsys PCIe controller LINK_CONTROL_LINK_STATUS_REG is of 4bytes size at offset 0x10, In both the cases everything is same except the size of the register, so i used PCIE_EXP_LNKCTL macro which is already defined in pci_regs.h Likewise, PCI_EXP_LNKCTL_RCB is RO for Root Ports, but maybe you're telling the device what it should advertise in its Link Control? You are correct, PCI_EXP_LNKCTL_RCB is RO. I will remove it. PCI_EXP_LNKCTL_CCC is RW. But doesn't it depend on the components on both ends of the link? Do you know what device is at the other end? I would have assumed that you'd have to start with CCC==0, which should be most conservative, then set CCC=1 only if you know both ends have a common clock. PCIe RC and endpoint device are having the common clock so set the CCC=1. + pcie_rc_cfg_wr(lpp, val, PCIE_CAP_OFST + PCI_EXP_LNKCTL); +} + +static void intel_pcie_max_speed_setup(struct intel_pcie_port *lpp) +{ + u32 reg, val; + + reg = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL2); + switch (lpp->link_gen) { + case PCIE_LINK_SPEED_GEN1: + reg &= ~PCI_EXP_LNKCTL2_TLS; + reg |= PCI_EXP_LNKCTL2_HASD| + PCI_EXP_LNKCTL2_TLS_2_5GT; + break; + case PCIE_LINK_SPEED_GEN2: + reg &= ~PCI_EXP_LNKCTL2_TLS; + reg |= PCI_EXP_LNKCTL2_HASD| + PCI_EXP_LNKCTL2_TLS_5_0GT; + break; + case PCIE_LINK_SPEED_GEN3: + reg &= ~PCI_EXP_LNKCTL2_TLS; + reg |= PCI_EXP_LNKCTL2_HASD| + PCI_EXP_LNKCTL2_TLS_8_0GT; + break; + case PCIE_LINK_SPEED_GEN4: + reg &= ~PCI_EXP_LNKCTL2_TLS; + reg |= PCI_EXP_LNKCTL2_HASD| + PCI_EXP_LNKCTL2_TLS_16_0GT; + break; + default: + /* Use hardware capability */ + val = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCAP); + val = FIELD_GET(PCI_EXP_LNKCAP_SLS, val); + reg &= ~PCI_EXP_LNKCTL2_HASD; + reg |= val; + break; + } + + pcie_rc_cfg_wr(lpp, reg, PCIE_CAP_OFST + PCI_EXP_LNKCTL2); + dw_pcie_link_set_n_fts(>pci, lpp->n_fts); There are other users of of_pci_get_max_link_speed() that look sort of similar to this (dra7xx_pcie_establish_link(), ks_pcie_set_link_speed(), tegra_pcie_prepare_host()). Do these *need* to be different, or is there something that could be factored out? dra7xx_pcie_establish_link() and ks_pcie_set_link_speed() are doing speed configuration for GEN1 and GEN1 &2 respectively. intel_pcie_max_speed_setup() can be moved to dwc framework and dra7xx and ks_pcie drivers can call. +} + + + Remove extra blank lines here. i will remove it. +static void intel_pcie_port_logic_setup(struct intel_pcie_port *lpp) ... + /* Intel PCIe doesn't configure IO region, so configure +* viewport to not to access IO region during register +* read write operations. +*/ This comment doesn't describe the code. Is there supposed to be some code here that configures the viewport? Where do we tell the viewport not to access IO? I guess maybe this means something like "tell dw_pcie_access_other_conf() not to program an outbound ATU for I/O"? I don't know that s
Re: [PATCH v4 2/3] dwc: PCI: intel: PCIe RC controller driver
Hi Andrew Murray, On 10/21/2019 9:03 PM, Andrew Murray wrote: On Mon, Oct 21, 2019 at 02:39:19PM +0800, Dilip Kota wrote: Add support to PCIe RC controller on Intel Gateway SoCs. PCIe controller is based of Synopsys DesignWare pci core. Intel PCIe driver requires Upconfig support, fast training sequence configuration and link speed change. So adding the respective helper functions in the pcie DesignWare framework. It also programs hardware autonomous speed during speed configuration so defining it in pci_regs.h. Changes on v4: Rename the driver naming and description to "PCIe RC controller on Intel Gateway SoCs". Use PCIe core register macros defined in pci_regs.h and remove respective local definitions. Remove pcie core interrupt handling. Move pcie link control speed change, upconfig and FTS. configuration functions to DesignWare framework. Use of_pci_get_max_link_speed(). Mark dependency on X86 and COMPILE_TEST in Kconfig. Remove lanes and add n_fts variables in intel_pcie_port structure. Rename rst_interval variable to rst_intrvl in intel_pcie_port structure. Remove intel_pcie_mem_iatu() as it is already perfomed in dw_setup_rc() Move sysfs attributes specific code to separate patch. Remove redundant error handling. Reduce LoCs by doing variable initializations while declaration itself. Add extra line after closing parenthesis. Move intel_pcie_ep_rst_init() out of get_resources() 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() Signed-off-by: Dilip Kota --- drivers/pci/controller/dwc/Kconfig | 10 + drivers/pci/controller/dwc/Makefile | 1 + drivers/pci/controller/dwc/pcie-designware.c | 34 ++ drivers/pci/controller/dwc/pcie-designware.h | 12 + drivers/pci/controller/dwc/pcie-intel-gw.c | 590 +++ include/uapi/linux/pci_regs.h| 1 + 6 files changed, 648 insertions(+) create mode 100644 drivers/pci/controller/dwc/pcie-intel-gw.c diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig index 0ba988b5b5bc..b33ed1cc873d 100644 --- a/drivers/pci/controller/dwc/Kconfig +++ b/drivers/pci/controller/dwc/Kconfig @@ -82,6 +82,16 @@ config PCIE_DW_PLAT_EP order to enable device-specific features PCI_DW_PLAT_EP must be selected. +config PCIE_INTEL_GW +bool "Intel Gateway PCIe host controller support" + depends on OF && (X86 || COMPILE_TEST) + select PCIE_DW_HOST + help + Say 'Y' here to enable support for PCIe Host controller driver. This sentence is very generic, we don't even say what controller driver. + The PCIe controller on Intel Gateway SoCs is based on the Synopsys + DesignWare pcie core and therefore uses the DesignWare core + functions for the driver implementation. Thanks for changing the driver name, though the description hasn't really changed since the last revision. In particular, Christoph's feedback mentioned that it would be useful to describe where this IP block may be used - is there any reason why we can't make this more useful for users? I will add more information. + 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 b30336181d46..99db83cd2f35 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) += p
Re: [PATCH v4 2/3] dwc: PCI: intel: PCIe RC controller driver
Hi Gustavo Pimentel, On 10/21/2019 4:29 PM, Gustavo Pimentel wrote: Hi On Mon, Oct 21, 2019 at 7:39:19, Dilip Kota wrote: Add support to PCIe RC controller on Intel Gateway SoCs. PCIe controller is based of Synopsys DesignWare pci core. Intel PCIe driver requires Upconfig support, fast training sequence configuration and link speed change. So adding the respective helper functions in the pcie DesignWare framework. It also programs hardware autonomous speed during speed configuration so defining it in pci_regs.h. Please do the replacement in all of your patches s/pcie/PCIe s/pci/PCI Also I think the correct term is Upconfigure and not Upconfig Yes, i will update it. Changes on v4: Rename the driver naming and description to "PCIe RC controller on Intel Gateway SoCs". Use PCIe core register macros defined in pci_regs.h and remove respective local definitions. Remove pcie core interrupt handling. Move pcie link control speed change, upconfig and FTS. configuration functions to DesignWare framework. Use of_pci_get_max_link_speed(). Mark dependency on X86 and COMPILE_TEST in Kconfig. Remove lanes and add n_fts variables in intel_pcie_port structure. Rename rst_interval variable to rst_intrvl in intel_pcie_port structure. Remove intel_pcie_mem_iatu() as it is already perfomed in dw_setup_rc() Move sysfs attributes specific code to separate patch. Remove redundant error handling. Reduce LoCs by doing variable initializations while declaration itself. Add extra line after closing parenthesis. Move intel_pcie_ep_rst_init() out of get_resources() 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() Signed-off-by: Dilip Kota --- drivers/pci/controller/dwc/Kconfig | 10 + drivers/pci/controller/dwc/Makefile | 1 + drivers/pci/controller/dwc/pcie-designware.c | 34 ++ drivers/pci/controller/dwc/pcie-designware.h | 12 + drivers/pci/controller/dwc/pcie-intel-gw.c | 590 +++ include/uapi/linux/pci_regs.h| 1 + 6 files changed, 648 insertions(+) create mode 100644 drivers/pci/controller/dwc/pcie-intel-gw.c diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig index 0ba988b5b5bc..b33ed1cc873d 100644 --- a/drivers/pci/controller/dwc/Kconfig +++ b/drivers/pci/controller/dwc/Kconfig @@ -82,6 +82,16 @@ config PCIE_DW_PLAT_EP order to enable device-specific features PCI_DW_PLAT_EP must be selected. +config PCIE_INTEL_GW +bool "Intel Gateway PCIe host controller support" + depends on OF && (X86 || COMPILE_TEST) + select PCIE_DW_HOST + help + Say 'Y' here to enable support for PCIe Host controller driver. + The PCIe controller on Intel Gateway SoCs is based on the Synopsys + DesignWare pcie core and therefore uses the DesignWare core + functions for the driver implementation. + 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 b30336181d46..99db83cd2f35 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_GW) += pcie-intel-gw.o obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o obj-$(CONFIG_PCI_EXYNOS) +
Re: [PATCH v4 3/3] pci: intel: Add sysfs attributes to configure pcie link
Hi Gustavo Pimentel, On 10/21/2019 4:40 PM, Gustavo Pimentel wrote: On Mon, Oct 21, 2019 at 7:39:20, Dilip Kota wrote: PCIe RC driver on Intel Gateway SoCs have a requirement of changing link width and speed on the fly. So add the sysfs attributes to show and store the link properties. Add the respective link resize function in pcie DesignWare framework so that Intel PCIe driver can use during link width configuration on the fly. Signed-off-by: Dilip Kota --- drivers/pci/controller/dwc/pcie-designware.c | 9 +++ drivers/pci/controller/dwc/pcie-designware.h | 3 + drivers/pci/controller/dwc/pcie-intel-gw.c | 112 ++- 3 files changed, 123 insertions(+), 1 deletion(-) diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c index 4c391bfd681a..662fdcb4f2d6 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -474,6 +474,15 @@ int dw_pcie_link_up(struct dw_pcie *pci) (!(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING))); } +void dw_pcie_link_width_resize(struct dw_pcie *pci, u32 lane_width) +{ + u32 val; + + val = dw_pcie_readl_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL); + val &= ~(PORT_MLTI_LNK_WDTH_CHNG | PORT_MLTI_LNK_WDTH); + val |= PORT_MLTI_LNK_WDTH_CHNG | lane_width; + dw_pcie_writel_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL, val); +} void dw_pcie_upconfig_setup(struct dw_pcie *pci) { diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index 3beac10e4a4c..fcf0442341fd 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -67,6 +67,8 @@ #define PCIE_MSI_INTR0_STATUS 0x830 #define PCIE_PORT_MULTI_LANE_CTRL 0x8C0 +#define PORT_MLTI_LNK_WDTH GENMASK(5, 0) +#define PORT_MLTI_LNK_WDTH_CHNGBIT(6) #define PORT_MLTI_UPCFG_SUPPORT BIT(7) #define PCIE_ATU_VIEWPORT 0x900 @@ -282,6 +284,7 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val); u32 dw_pcie_read_atu(struct dw_pcie *pci, u32 reg, size_t size); void dw_pcie_write_atu(struct dw_pcie *pci, u32 reg, size_t size, u32 val); int dw_pcie_link_up(struct dw_pcie *pci); +void dw_pcie_link_width_resize(struct dw_pcie *pci, u32 lane_width); void dw_pcie_upconfig_setup(struct dw_pcie *pci); void dw_pcie_link_speed_change(struct dw_pcie *pci, bool enable); void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts); diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c index 9142c70db808..b9be0921671d 100644 --- a/drivers/pci/controller/dwc/pcie-intel-gw.c +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c @@ -146,6 +146,22 @@ static void intel_pcie_ltssm_disable(struct intel_pcie_port *lpp) pcie_app_wr_mask(lpp, PCIE_APP_CCR_LTSSM_ENABLE, 0, PCIE_APP_CCR); } +static const char *pcie_link_gen_to_str(int gen) +{ + switch (gen) { + case PCIE_LINK_SPEED_GEN1: + return "2.5"; + case PCIE_LINK_SPEED_GEN2: + return "5.0"; + case PCIE_LINK_SPEED_GEN3: + return "8.0"; + case PCIE_LINK_SPEED_GEN4: + return "16.0"; + default: + return "???"; + } +} + static void intel_pcie_link_setup(struct intel_pcie_port *lpp) { u32 val; @@ -444,6 +460,91 @@ static int intel_pcie_host_setup(struct intel_pcie_port *lpp) return ret; } +static ssize_t pcie_link_status_show(struct device *dev, +struct device_attribute *attr, char *buf) +{ + struct intel_pcie_port *lpp = dev_get_drvdata(dev); + u32 reg, width, gen; + + reg = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL); + width = FIELD_GET(PCI_EXP_LNKSTA_NLW, reg >> 16); + gen = FIELD_GET(PCI_EXP_LNKSTA_CLS, reg >> 16); + + if (gen > lpp->max_speed) + return -EINVAL; + + return sprintf(buf, "Port %2u Width x%u Speed %s GT/s\n", lpp->id, + width, pcie_link_gen_to_str(gen)); +} +static DEVICE_ATTR_RO(pcie_link_status); Dilip please check pci.h there are there already enums and strings relatively to PCIe speed and width, that you can use. Yes i can see a global array "pcie_link_speed[]" and a macro PCIE_SPEED2STR[]. I will update the driver. Whereas width enum, it is not required here as directly storing the register value. Thanks for pointing it. Regards, Dilip + +static ssize_t pcie_speed_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t len) +{ + struct intel_pcie_port *lpp = dev_get_drvdata(dev); + unsigned long val
Re: [PATCH v4 0/3] PCI: Add Intel PCIe Driver and respective dt-binding yaml file
Hi Gustavo Pimentel, On 10/21/2019 4:08 PM, Gustavo Pimentel wrote: Hi, On Mon, Oct 21, 2019 at 7:39:17, Dilip Kota wrote: Intel PCIe is synopsys based controller utilizes the Designware Please do this general replacement in all your patches. s/synopsys/Synopsys and s/Designware/DesignWare Sure, i will update it in the next patch version. (In the all other patches naming is proper, i got missed it here.) I will ensure and take care of it. Regards, Dilip framework for host initialization and intel application specific register configurations. Changes on v4: Add lane resizing API in PCIe DesignWare driver. Intel PCIe driver uses it for lane resizing which is being exposed through sysfs attributes. Add Intel PCIe sysfs attributes is in separate patch. Address review comments given on v3. Changes on v3: Compared to v2, map_irq() patch is removed as it is no longer required for Intel PCIe driver. Intel PCIe driver does platform specific interrupt configuration during core initialization. So changed the subject line too. Address v2 review comments for DT binding and PCIe driver Dilip Kota (3): dt-bindings: PCI: intel: Add YAML schemas for the PCIe RC controller dwc: PCI: intel: PCIe RC controller driver pci: intel: Add sysfs attributes to configure pcie link .../devicetree/bindings/pci/intel-gw-pcie.yaml | 135 drivers/pci/controller/dwc/Kconfig | 10 + drivers/pci/controller/dwc/Makefile| 1 + drivers/pci/controller/dwc/pcie-designware.c | 43 ++ drivers/pci/controller/dwc/pcie-designware.h | 15 + drivers/pci/controller/dwc/pcie-intel-gw.c | 700 +++ include/uapi/linux/pci_regs.h | 1 + 7 files changed, 905 insertions(+) create mode 100644 Documentation/devicetree/bindings/pci/intel-gw-pcie.yaml create mode 100644 drivers/pci/controller/dwc/pcie-intel-gw.c -- 2.11.0
[PATCH v4 0/3] PCI: Add Intel PCIe Driver and respective dt-binding yaml file
Intel PCIe is synopsys based controller utilizes the Designware framework for host initialization and intel application specific register configurations. Changes on v4: Add lane resizing API in PCIe DesignWare driver. Intel PCIe driver uses it for lane resizing which is being exposed through sysfs attributes. Add Intel PCIe sysfs attributes is in separate patch. Address review comments given on v3. Changes on v3: Compared to v2, map_irq() patch is removed as it is no longer required for Intel PCIe driver. Intel PCIe driver does platform specific interrupt configuration during core initialization. So changed the subject line too. Address v2 review comments for DT binding and PCIe driver Dilip Kota (3): dt-bindings: PCI: intel: Add YAML schemas for the PCIe RC controller dwc: PCI: intel: PCIe RC controller driver pci: intel: Add sysfs attributes to configure pcie link .../devicetree/bindings/pci/intel-gw-pcie.yaml | 135 drivers/pci/controller/dwc/Kconfig | 10 + drivers/pci/controller/dwc/Makefile| 1 + drivers/pci/controller/dwc/pcie-designware.c | 43 ++ drivers/pci/controller/dwc/pcie-designware.h | 15 + drivers/pci/controller/dwc/pcie-intel-gw.c | 700 +++ include/uapi/linux/pci_regs.h | 1 + 7 files changed, 905 insertions(+) create mode 100644 Documentation/devicetree/bindings/pci/intel-gw-pcie.yaml create mode 100644 drivers/pci/controller/dwc/pcie-intel-gw.c -- 2.11.0
[PATCH v4 3/3] pci: intel: Add sysfs attributes to configure pcie link
PCIe RC driver on Intel Gateway SoCs have a requirement of changing link width and speed on the fly. So add the sysfs attributes to show and store the link properties. Add the respective link resize function in pcie DesignWare framework so that Intel PCIe driver can use during link width configuration on the fly. Signed-off-by: Dilip Kota --- drivers/pci/controller/dwc/pcie-designware.c | 9 +++ drivers/pci/controller/dwc/pcie-designware.h | 3 + drivers/pci/controller/dwc/pcie-intel-gw.c | 112 ++- 3 files changed, 123 insertions(+), 1 deletion(-) diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c index 4c391bfd681a..662fdcb4f2d6 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -474,6 +474,15 @@ int dw_pcie_link_up(struct dw_pcie *pci) (!(val & PCIE_PORT_DEBUG1_LINK_IN_TRAINING))); } +void dw_pcie_link_width_resize(struct dw_pcie *pci, u32 lane_width) +{ + u32 val; + + val = dw_pcie_readl_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL); + val &= ~(PORT_MLTI_LNK_WDTH_CHNG | PORT_MLTI_LNK_WDTH); + val |= PORT_MLTI_LNK_WDTH_CHNG | lane_width; + dw_pcie_writel_dbi(pci, PCIE_PORT_MULTI_LANE_CTRL, val); +} void dw_pcie_upconfig_setup(struct dw_pcie *pci) { diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index 3beac10e4a4c..fcf0442341fd 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -67,6 +67,8 @@ #define PCIE_MSI_INTR0_STATUS 0x830 #define PCIE_PORT_MULTI_LANE_CTRL 0x8C0 +#define PORT_MLTI_LNK_WDTH GENMASK(5, 0) +#define PORT_MLTI_LNK_WDTH_CHNGBIT(6) #define PORT_MLTI_UPCFG_SUPPORTBIT(7) #define PCIE_ATU_VIEWPORT 0x900 @@ -282,6 +284,7 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val); u32 dw_pcie_read_atu(struct dw_pcie *pci, u32 reg, size_t size); void dw_pcie_write_atu(struct dw_pcie *pci, u32 reg, size_t size, u32 val); int dw_pcie_link_up(struct dw_pcie *pci); +void dw_pcie_link_width_resize(struct dw_pcie *pci, u32 lane_width); void dw_pcie_upconfig_setup(struct dw_pcie *pci); void dw_pcie_link_speed_change(struct dw_pcie *pci, bool enable); void dw_pcie_link_set_n_fts(struct dw_pcie *pci, u32 n_fts); diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c b/drivers/pci/controller/dwc/pcie-intel-gw.c index 9142c70db808..b9be0921671d 100644 --- a/drivers/pci/controller/dwc/pcie-intel-gw.c +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c @@ -146,6 +146,22 @@ static void intel_pcie_ltssm_disable(struct intel_pcie_port *lpp) pcie_app_wr_mask(lpp, PCIE_APP_CCR_LTSSM_ENABLE, 0, PCIE_APP_CCR); } +static const char *pcie_link_gen_to_str(int gen) +{ + switch (gen) { + case PCIE_LINK_SPEED_GEN1: + return "2.5"; + case PCIE_LINK_SPEED_GEN2: + return "5.0"; + case PCIE_LINK_SPEED_GEN3: + return "8.0"; + case PCIE_LINK_SPEED_GEN4: + return "16.0"; + default: + return "???"; + } +} + static void intel_pcie_link_setup(struct intel_pcie_port *lpp) { u32 val; @@ -444,6 +460,91 @@ static int intel_pcie_host_setup(struct intel_pcie_port *lpp) return ret; } +static ssize_t pcie_link_status_show(struct device *dev, +struct device_attribute *attr, char *buf) +{ + struct intel_pcie_port *lpp = dev_get_drvdata(dev); + u32 reg, width, gen; + + reg = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL); + width = FIELD_GET(PCI_EXP_LNKSTA_NLW, reg >> 16); + gen = FIELD_GET(PCI_EXP_LNKSTA_CLS, reg >> 16); + + if (gen > lpp->max_speed) + return -EINVAL; + + return sprintf(buf, "Port %2u Width x%u Speed %s GT/s\n", lpp->id, + width, pcie_link_gen_to_str(gen)); +} +static DEVICE_ATTR_RO(pcie_link_status); + +static ssize_t pcie_speed_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t len) +{ + struct intel_pcie_port *lpp = dev_get_drvdata(dev); + unsigned long val; + int ret; + + ret = kstrtoul(buf, 10, ); + if (ret) + return ret; + + if (val > lpp->max_speed) + return -EINVAL; + + lpp->link_gen = val; + intel_pcie_max_speed_setup(lpp); + dw_pcie_link_speed_change(>pci, false); + dw_pcie_link_speed_change(>pci, true); + + return len; +} +static DEVICE_ATTR_WO(pcie_speed); + +/* + * Link width change on the fly is not always successful. + * It also depends on the p
[PATCH v4 1/3] dt-bindings: PCI: intel: Add YAML schemas for the PCIe RC controller
Add YAML shcemas for PCIe RC controller on Intel Gateway SoCs which is Synopsys DesignWare based PCIe core. changes on v4: Add "snps,dw-pcie" compatible. Rename phy-names property value to pcie. And maximum and minimum values to num-lanes. Add ref for reset-assert-ms entry and update the description for easy understanding. Remove pcie core interrupt entry. changes on v3: Add the appropriate License-Identifier Rename intel,rst-interval to 'reset-assert-us' 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 Signed-off-by: Dilip Kota --- .../devicetree/bindings/pci/intel-gw-pcie.yaml | 135 + 1 file changed, 135 insertions(+) create mode 100644 Documentation/devicetree/bindings/pci/intel-gw-pcie.yaml diff --git a/Documentation/devicetree/bindings/pci/intel-gw-pcie.yaml b/Documentation/devicetree/bindings/pci/intel-gw-pcie.yaml new file mode 100644 index ..49dd87ec1e3d --- /dev/null +++ b/Documentation/devicetree/bindings/pci/intel-gw-pcie.yaml @@ -0,0 +1,135 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pci/intel-gw-pcie.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: PCIe RC controller on Intel Gateway SoCs + +maintainers: + - Dilip Kota + +properties: + compatible: +items: + - const: intel,lgm-pcie + - const: snps,dw-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: pcie + + reset-gpios: +maxItems: 1 + + num-lanes: +minimum: 1 +maximum: 2 +description: Number of lanes to use for this port. + + linux,pci-domain: +$ref: /schemas/types.yaml#/definitions/uint32 +description: PCI domain ID. + + '#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: +$ref: /schemas/types.yaml#/definitions/uint32 +description: | + Delay after asserting reset to the PCIe device. + 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 + - interrupt-map + - interrupt-map-mask + +additionalProperties: false + +examples: + - | +pcie10:pcie@d0e0 { + compatible = "intel,lgm-pcie", "snps,dw-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 = <>; + #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 = "pcie"; + status = "okay"; + reset-assert-ms = <500>; + reset-gpios = < 3 GPIO_ACTIVE_LOW>; + num-lanes = <2>; +}; -- 2.11.0
[PATCH v4 2/3] dwc: PCI: intel: PCIe RC controller driver
Add support to PCIe RC controller on Intel Gateway SoCs. PCIe controller is based of Synopsys DesignWare pci core. Intel PCIe driver requires Upconfig support, fast training sequence configuration and link speed change. So adding the respective helper functions in the pcie DesignWare framework. It also programs hardware autonomous speed during speed configuration so defining it in pci_regs.h. Changes on v4: Rename the driver naming and description to "PCIe RC controller on Intel Gateway SoCs". Use PCIe core register macros defined in pci_regs.h and remove respective local definitions. Remove pcie core interrupt handling. Move pcie link control speed change, upconfig and FTS. configuration functions to DesignWare framework. Use of_pci_get_max_link_speed(). Mark dependency on X86 and COMPILE_TEST in Kconfig. Remove lanes and add n_fts variables in intel_pcie_port structure. Rename rst_interval variable to rst_intrvl in intel_pcie_port structure. Remove intel_pcie_mem_iatu() as it is already perfomed in dw_setup_rc() Move sysfs attributes specific code to separate patch. Remove redundant error handling. Reduce LoCs by doing variable initializations while declaration itself. Add extra line after closing parenthesis. Move intel_pcie_ep_rst_init() out of get_resources() 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() Signed-off-by: Dilip Kota --- drivers/pci/controller/dwc/Kconfig | 10 + drivers/pci/controller/dwc/Makefile | 1 + drivers/pci/controller/dwc/pcie-designware.c | 34 ++ drivers/pci/controller/dwc/pcie-designware.h | 12 + drivers/pci/controller/dwc/pcie-intel-gw.c | 590 +++ include/uapi/linux/pci_regs.h| 1 + 6 files changed, 648 insertions(+) create mode 100644 drivers/pci/controller/dwc/pcie-intel-gw.c diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig index 0ba988b5b5bc..b33ed1cc873d 100644 --- a/drivers/pci/controller/dwc/Kconfig +++ b/drivers/pci/controller/dwc/Kconfig @@ -82,6 +82,16 @@ config PCIE_DW_PLAT_EP order to enable device-specific features PCI_DW_PLAT_EP must be selected. +config PCIE_INTEL_GW +bool "Intel Gateway PCIe host controller support" + depends on OF && (X86 || COMPILE_TEST) + select PCIE_DW_HOST + help + Say 'Y' here to enable support for PCIe Host controller driver. + The PCIe controller on Intel Gateway SoCs is based on the Synopsys + DesignWare pcie core and therefore uses the DesignWare core + functions for the driver implementation. + 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 b30336181d46..99db83cd2f35 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_GW) += pcie-intel-gw.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-designware.c b/drivers/pci/controller/dwc/pcie-designware.c index 820488dfeaed..4c391bfd681a 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -47
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Philipp, On 10/8/2019 11:56 PM, Philipp Zabel wrote: Hi Martin, On Mon, 2019-10-07 at 21:53 +0200, Martin Blumenstingl wrote: Hi Philipp, On Thu, Oct 3, 2019 at 4:19 PM Philipp Zabel wrote: [...] because the register layout was greatly simplified for the newer SoCs (for which there is reset-intel) compared to the older ones (reset-lantiq). Dilip's suggestion (in my own words) is that you take his new reset-intel driver, then we will work on porting reset-lantiq over to that so in the end we can drop the reset-lantiq driver. Just to be sure, you are suggesting to add support for the current lantiq,reset binding to the reset-intel driver at a later point? I see no reason not to do that, but I'm also not quite sure what the benefit will be over just keeping reset-lantiq as is? according to Chuan and Dilip the current reset-lantiq implementation is wrong [0]. The only issue seems to be the .reset callback, which doesn't have any users anway. The DT binding of reset-lantiq driver is also having issue. I have explained here [1]. my understanding is that the Lantiq and Intel LGM reset controllers are identical except: - the Lantiq variant uses a weird register layout (reset and status registers not at consecutive offsets) - the bits of the reset and status registers sometimes don't match on the Lantiq variant Thank you, so these are a good explanation for why the DT bindings should be different. - the Intel variant has a dedicated registers area for the reset controller registers, while the Lantiq variant mixes them with various other functionality (for example: USB2 PHYs) I'm not quite sure I understand why the intel driver is using syscon, then. Either way, it shouldn't make a big difference if regmap is used anyway. Yes, we decided to remove the syscon and use the regmap.[2] This approach means more work for me (as I am probably the one who then has to do the work to port reset-lantiq over to reset-intel). More work than what alternative? compared to "fixing" the existing reset-lantiq driver (reset callback) That is still something you could do, or just drop the .reset callback because there are no reset consumers using it anyway. One correct thing to do would be to identify those self-clearing reset bits and to disallow calling assert/deassert on them. and then (instead of adding a new driver) integrating Intel LGM support into reset-lantiq Since at this point I'm not even sure whether merging the two at all is better than keeping them separate, I have no opinion on whether merging intel support into the lantiq driver or the other way around is preferable. I'm happy to do that work if you think that it's worth following this approach. So I want your opinion on this before I spend any effort on porting reset-lantiq over to reset-intel. Reset drivers are typically so simple, I'm not quite sure whether it is worth to integrate multiple drivers if it complicates matters too much. In this case though I expect it would just be adding support for a custom .of_xlate and lantiq specific register property parsing? yes, that's how I understand the Lantiq and Intel reset controllers: - reset/status/assert/deassert callbacks would be shared across all variants - register parsing and of_xlate are SoC specific Ok. If that turns out to be less rather than more boilerplate than two separate drivers, that should be fine. Thanks Philipp for your time and briefly explaining your view. Regards, Dilip [1]: https://www.spinics.net/lists/devicetree/msg308930.html [2]: https://lkml.org/lkml/2019/9/2/289 regards Philipp
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin,Philipp, On 10/8/2019 3:53 AM, Martin Blumenstingl wrote: Hi Philipp, On Thu, Oct 3, 2019 at 4:19 PM Philipp Zabel wrote: [...] because the register layout was greatly simplified for the newer SoCs (for which there is reset-intel) compared to the older ones (reset-lantiq). Dilip's suggestion (in my own words) is that you take his new reset-intel driver, then we will work on porting reset-lantiq over to that so in the end we can drop the reset-lantiq driver. Just to be sure, you are suggesting to add support for the current lantiq,reset binding to the reset-intel driver at a later point? I see no reason not to do that, but I'm also not quite sure what the benefit will be over just keeping reset-lantiq as is? according to Chuan and Dilip the current reset-lantiq implementation is wrong [0]. my understanding is that the Lantiq and Intel LGM reset controllers are identical except: - the Lantiq variant uses a weird register layout (reset and status registers not at consecutive offsets) - the bits of the reset and status registers sometimes don't match on the Lantiq variant - the Intel variant has a dedicated registers area for the reset controller registers, while the Lantiq variant mixes them with various other functionality (for example: USB2 PHYs) This approach means more work for me (as I am probably the one who then has to do the work to port reset-lantiq over to reset-intel). More work than what alternative? compared to "fixing" the existing reset-lantiq driver (reset callback) and then (instead of adding a new driver) integrating Intel LGM support into reset-lantiq Integrating Intel LGM support into reset-lantiq boils down to re-writing reset-lantiq driver as intel-reset driver and adding Lantiq variant support. Why because reset-lantiq driver is not according to hardware design[1]. I see the final best solution is to integrate Lantiq variant driver to intel-reset driver.[1] I hope you guys are ok with it. Please let me know your view. Regards, Dilip [1]: https://www.spinics.net/lists/devicetree/msg308930.html I'm happy to do that work if you think that it's worth following this approach. So I want your opinion on this before I spend any effort on porting reset-lantiq over to reset-intel. Reset drivers are typically so simple, I'm not quite sure whether it is worth to integrate multiple drivers if it complicates matters too much. In this case though I expect it would just be adding support for a custom .of_xlate and lantiq specific register property parsing? yes, that's how I understand the Lantiq and Intel reset controllers: - reset/status/assert/deassert callbacks would be shared across all variants - register parsing and of_xlate are SoC specific Martin [0] https://www.spinics.net/lists/devicetree/msg305951.html
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin and Philipp, On 20/9/2019 10:47 AM, Dilip Kota wrote: Hi Martin, On 9/20/2019 3:51 AM, Martin Blumenstingl wrote: Hi Dilip, (sorry for the late reply) On Thu, Sep 12, 2019 at 8:38 AM Dilip Kota wrote: [...] The major difference between the vrx200 and lgm is: 1.) RCU in vrx200 is having multiple register regions wheres RCU in lgm has one single register region. 2.) Register offsets and bit offsets are different. So enhancing the intel-reset-syscon.c to provide compatibility/support for vrx200. Please check the below dtsi binding proposal and let me know your view. rcu0:reset-controller@ { compatible= "intel,rcu-lgm"; reg = <0x000 0x8>, , , ; I'm not sure that I understand what are reg_set2/3/4 for the first resource (0x8 at 0x0) already covers the whole LGM RCU, so what is the purpose of the other register resources Yes, as you said the first register resource is enough for LGM RCU as registers are at one single region. Whereas in older SoCs RCU registers are at different regions, so for that reason reg_set2/3/4 are used. Driver will decide in reading the no. of register resources based on the "struct of_device_id". Regards, Dilip intel,global-reset = <0x10 30>; #reset-cells = <3>; }; "#reset-cells": const:3 description: | The 1st cell is the reset register offset. The 2nd cell is the reset set bit offset. The 3rd cell is the reset status bit offset. I think this will work fine for VRX200 (and even older SoCs) as you have described in your previous emails we can determine the status offset from the reset offset using a simple if/else for LGM I like your initial suggestion with #reset-cells = <2> because it's easier to read and write. Reset driver takes care of parsing the register address "reg" as per the ".data" structure in struct of_device_id. Reset driver takes care of traversing the status register offset. the differentiation between two and three #reset-cells can also happen based on the struct of_device_id: - the LGM implementation would simply also use the reset bit as status bit (only two cells are needed) - the implementation for earlier SoCs would parse the third cell and use that as status bit Philipp, can you please share your opinion on how to move forward with the reset-intel driver from this series? The reset_control_ops from the reset-intel driver are (in my opinion) a bug-fixed and improved version of what we already have in drivers/reset/reset-lantiq.c. The driver is NOT simply copy and paste because the register layout was greatly simplified for the newer SoCs (for which there is reset-intel) compared to the older ones (reset-lantiq). Dilip's suggestion (in my own words) is that you take his new reset-intel driver, then we will work on porting reset-lantiq over to that so in the end we can drop the reset-lantiq driver. This approach means more work for me (as I am probably the one who then has to do the work to port reset-lantiq over to reset-intel). I'm happy to do that work if you think that it's worth following this approach. So I want your opinion on this before I spend any effort on porting reset-lantiq over to reset-intel. I will start implementing this design in the next patch version along with the other changes suggested in this patch review, please let me know if you have other thoughts in this design. Regards, Dilip Martin
Re: Fwd: Re: [PATCH v3 1/2] dt-bindings: PCI: intel: Add YAML schemas for the PCIe RC controller
Hi Rob, On 18/9/2019 2:56 PM, Dilip Kota wrote: On 9/18/2019 2:40 AM, Rob Herring wrote: On Wed, Sep 04, 2019 at 06:10:30PM +0800, 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' 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 These all belong in a common schema. + + 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. And this. + + 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. These 2 also should be common. + + interrupts: + description: PCIe core integrated miscellaneous interrupt. How many? No need for description if there's only 1. + + '#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. All common. You mean to remove all the common schema entries description!. In most of the Documention/devicetree/binding/pci documents all the common entries are described so I followed the same. If common schemas are removed, getting below warning during the dt_binding_check. Documentation/devicetree/bindings/pci/intel,lgm-pcie.example.dt.yaml: pcie@d0e0: '#address-cells', '#interrupt-cells', '#size-cells', 'bus-range', 'device_type', 'interrupt-map', 'interrupt-map-mask', 'interrupt-parent', 'linux,pci-domain', 'ranges', 'reset-gpios' do not match any of the regexes: 'pinctrl-[0-9]+' Regards, Dilip + + reset-assert-ms: + description: | + Device reset interval in ms. + Some devices need an interval upto 500ms. By default it is 100ms. This is a property of a device, so it belongs in a device node. How would you deal with this without DT? This property is for the PCIe RC to keep a delay before notifying the reset to the device. If this entry is not present, PCIe driver will set a default value of 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>; + }; -- 2.11.0
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin, On 9/20/2019 3:51 AM, Martin Blumenstingl wrote: Hi Dilip, (sorry for the late reply) On Thu, Sep 12, 2019 at 8:38 AM Dilip Kota wrote: [...] The major difference between the vrx200 and lgm is: 1.) RCU in vrx200 is having multiple register regions wheres RCU in lgm has one single register region. 2.) Register offsets and bit offsets are different. So enhancing the intel-reset-syscon.c to provide compatibility/support for vrx200. Please check the below dtsi binding proposal and let me know your view. rcu0:reset-controller@ { compatible= "intel,rcu-lgm"; reg = <0x000 0x8>, , , ; I'm not sure that I understand what are reg_set2/3/4 for the first resource (0x8 at 0x0) already covers the whole LGM RCU, so what is the purpose of the other register resources Yes, as you said the first register resource is enough for LGM RCU as registers are at one single region. Whereas in older SoCs RCU registers are at different regions, so for that reason reg_set2/3/4 are used. Driver will decide in reading the no. of register resources based on the "struct of_device_id". Regards, Dilip intel,global-reset = <0x10 30>; #reset-cells = <3>; }; "#reset-cells": const:3 description: | The 1st cell is the reset register offset. The 2nd cell is the reset set bit offset. The 3rd cell is the reset status bit offset. I think this will work fine for VRX200 (and even older SoCs) as you have described in your previous emails we can determine the status offset from the reset offset using a simple if/else for LGM I like your initial suggestion with #reset-cells = <2> because it's easier to read and write. Reset driver takes care of parsing the register address "reg" as per the ".data" structure in struct of_device_id. Reset driver takes care of traversing the status register offset. the differentiation between two and three #reset-cells can also happen based on the struct of_device_id: - the LGM implementation would simply also use the reset bit as status bit (only two cells are needed) - the implementation for earlier SoCs would parse the third cell and use that as status bit Philipp, can you please share your opinion on how to move forward with the reset-intel driver from this series? The reset_control_ops from the reset-intel driver are (in my opinion) a bug-fixed and improved version of what we already have in drivers/reset/reset-lantiq.c. The driver is NOT simply copy and paste because the register layout was greatly simplified for the newer SoCs (for which there is reset-intel) compared to the older ones (reset-lantiq). Dilip's suggestion (in my own words) is that you take his new reset-intel driver, then we will work on porting reset-lantiq over to that so in the end we can drop the reset-lantiq driver. This approach means more work for me (as I am probably the one who then has to do the work to port reset-lantiq over to reset-intel). I'm happy to do that work if you think that it's worth following this approach. So I want your opinion on this before I spend any effort on porting reset-lantiq over to reset-intel. Martin
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Thomas, On 9/19/2019 4:36 PM, Langer, Thomas wrote: Hi Dilip, -Original Message- From: devicetree-ow...@vger.kernel.org On Behalf Of Dilip Kota Sent: Donnerstag, 19. September 2019 10:06 To: Martin Blumenstingl Cc: Chuan Hua, Lei ; Kim, Cheol Yong ; devicet...@vger.kernel.org; linux- ker...@vger.kernel.org; p.za...@pengutronix.de; Wu, Qiming ; r...@kernel.org; Hauke Mehrtens Subject: Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC Hi Martin, On 9/12/2019 2:38 PM, Dilip Kota wrote: Re-sending the mail, because of delivery failure. sorry for the spam. Hi Martin, On 9/6/2019 4:53 AM, Martin Blumenstingl wrote: Hi, On Thu, Sep 5, 2019 at 4:38 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. indeed, chiptop should not have any child nodes (based on your explanation). I was referring to VRX200 where the RCU syscon has various children (one child node for each hardware module that's part of RCU: reset controller, 2x USB PHY, ...) back to LGM: you said that the LGM RCU registers only contain the reset controller. thus I see no need for the syscon compatible 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/XRX5
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Hi Martin, On 9/12/2019 2:38 PM, Dilip Kota wrote: Re-sending the mail, because of delivery failure. sorry for the spam. Hi Martin, On 9/6/2019 4:53 AM, Martin Blumenstingl wrote: Hi, On Thu, Sep 5, 2019 at 4:38 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. indeed, chiptop should not have any child nodes (based on your explanation). I was referring to VRX200 where the RCU syscon has various children (one child node for each hardware module that's part of RCU: reset controller, 2x USB PHY, ...) back to LGM: you said that the LGM RCU registers only contain the reset controller. thus I see no need for the syscon compatible 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 st
Re: [PATCH v3 1/2] dt-bindings: PCI: intel: Add YAML schemas for the PCIe RC controller
On 9/18/2019 2:40 AM, Rob Herring wrote: On Wed, Sep 04, 2019 at 06:10:30PM +0800, 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' 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 These all belong in a common schema. + + 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. And this. + + 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. These 2 also should be common. + + interrupts: +description: PCIe core integrated miscellaneous interrupt. How many? No need for description if there's only 1. + + '#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. All common. You mean to remove all the common schema entries description!. In most of the Documention/devicetree/binding/pci documents all the common entries are described so I followed the same. + + reset-assert-ms: +description: | + Device reset interval in ms. + Some devices need an interval upto 500ms. By default it is 100ms. This is a property of a device, so it belongs in a device node. How would you deal with this without DT? This property is for the PCIe RC to keep a delay before notifying the reset to the device. If this entry is not present, PCIe driver will set a default value of 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>; +}; -- 2.11.0
Re: [PATCH v3 1/2] dt-bindings: PCI: intel: Add YAML schemas for the PCIe RC controller
Hi Rob, On 9/18/2019 2:33 AM, Rob Herring wrote: On Fri, Sep 06, 2019 at 12:19:50PM +0300, Andy Shevchenko wrote: On Thu, Sep 05, 2019 at 10:31:29PM +0200, Martin Blumenstingl wrote: On Wed, Sep 4, 2019 at 12:11 PM Dilip Kota wrote: + 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 I'm not Rob, though I consider more practical to have a hyphenated variant. *-names is kind of pointless when there is only one entry and 'foo' in the values of 'foo-names' is redundant. Ok, i will change it to: phy-name: const: pcie Regards, Dilip Rob
Re: [PATCH v3 2/2] dwc: PCI: intel: Intel PCIe RC controller driver
On 9/13/2019 6:12 PM, andriy.shevche...@intel.com wrote: On Fri, Sep 13, 2019 at 05:20:26PM +0800, Dilip Kota wrote: On 9/12/2019 6:49 PM, Gustavo Pimentel wrote: On Thu, Sep 12, 2019 at 10:23:31, Dilip Kota wrote: Hi, I just return from parental leave, therefore I still trying to get the pace in mailing list discussion. However your suggestion looks good, I agree that can go into DesignWare driver to be available to all. Thanks Gustavo for the confirmation, i will add it in the next patch version Just a small request, please do in general: s/designware/DesignWare Sorry, i didnt understand this. It means the reviewer asks you to name DesignWare in this form, i.o.w. designware -> DesignWare. `man 1 sed` gives you more about it :-) Thanks Andy for clarifying it. Could you please also comment let me know your opinion on the driver naming. Below is the mail snippet. == >>> Add support to PCIe RC controller on Intel Universal >>> Gateway SoC. PCIe controller is based of Synopsys >>> Designware pci core. >>> +config PCIE_INTEL_AXI > [Andy]: I think that name here is too generic. Classical x86 seems not using this. [Dilip Kota]: This PCIe driver is for the Intel Gateway SoCs. So how about naming it is as "pcie-intel-gw"; pcie-intel-gw.c and Kconfig as PCIE_INTEL_GW. Andrew Murray is ok with this naming, please let me know your view. === Regards, Dilip
Re: [PATCH v3 2/2] dwc: PCI: intel: Intel PCIe RC controller driver
On 9/12/2019 6:49 PM, Gustavo Pimentel wrote: On Thu, Sep 12, 2019 at 10:23:31, Dilip Kota wrote: Quoting Andrew Murray: Quoting Gustavo Pimentel: On 9/12/2019 4:25 PM, Andrew Murray wrote: [...] +static void intel_pcie_max_link_width_setup(struct intel_pcie_port *lpp) +{ + u32 mask, val; + + /* HW auto bandwidth negotiation must be enabled */ + pcie_rc_cfg_wr_mask(lpp, PCIE_LCTLSTS_HW_AW_DIS, 0, PCIE_LCTLSTS); + + mask = PCIE_DIRECT_LINK_WIDTH_CHANGE | PCIE_TARGET_LINK_WIDTH; + val = PCIE_DIRECT_LINK_WIDTH_CHANGE | lpp->lanes; + pcie_rc_cfg_wr_mask(lpp, mask, val, PCIE_MULTI_LANE_CTRL); Is this identical functionality to the writing of PCIE_PORT_LINK_CONTROL in dw_pcie_setup? I ask because if the user sets num-lanes in the DT, will it have the desired effect? intel_pcie_max_link_width_setup() function will be called by sysfs attribute pcie_width_store() to change on the fly. Indeed, but a user may also set num-lanes in the device tree. I'm wondering if, when set in device-tree, it will have the desired effect. Because I don't see anything similar to PCIE_LCTLSTS_HW_AW_DIS in dw_pcie_setup which is what your function does here. I guess I'm trying to avoid the suitation where num-lanes doesn't have the desired effect and the only way to set the num-lanes is throught the sysfs control. I will check this and get back to you. intel_pcie_max_link_width_setup() is doing the lane resizing which is different from the link up/establishment happening during probe. Also PCIE_LCTLSTS_HW_AW_DIS default value is 0 so not setting during the probe or dw_pcie_setup. intel_pcie_max_link_width_setup() is programming as per the designware PCIe controller databook instructions for lane resizing. Below lines are from Designware PCIe databook for lane resizing. Program the TARGET_LINK_WIDTH[5:0] field of the MULTI_LANE_CONTROL_OFF register. Program the DIRECT_LINK_WIDTH_CHANGE2 field of the MULTI_LANE_CONTROL_OFF register. It is assumed that the PCIE_CAP_HW_AUTO_WIDTH_DISABLE field in the LINK_CONTROL_LINK_STATUS_REG register is 0. OK, so there is a difference between initial training and then resizing of the link. Given that this is not Intel specific, should this function exist within the designware driver for the benefit of others? I am ok to add if maintainer agrees with it. Gustavo Pimentel, Could you please let us know your opinion on this. Hi, I just return from parental leave, therefore I still trying to get the pace in mailing list discussion. However your suggestion looks good, I agree that can go into DesignWare driver to be available to all. Thanks Gustavo for the confirmation, i will add it in the next patch version Just a small request, please do in general: s/designware/DesignWare Sorry, i didnt understand this. Regards, Dilip Regards, Gustavo [...] +} + +static void intel_pcie_port_logic_setup(struct intel_pcie_port *lpp) +{ + u32 val, mask, fts; + + switch (lpp->max_speed) { + case PCIE_LINK_SPEED_GEN1: + case PCIE_LINK_SPEED_GEN2: + fts = PCIE_AFR_GEN12_FTS_NUM_DFT; + break; [...] + + if (device_property_read_u32(dev, "max-link-speed", >link_gen)) + lpp->link_gen = 0; /* Fallback to auto */ Is it possible to use of_pci_get_max_link_speed here instead? Thanks for pointing it. of_pci_get_max_link_speed() can be used here. I will update it in the next patch revision. I just remember, earlier we were using of_pci_get_max_link_speed() itself. As per reviewer comments changed to device_property_read_u32() to maintain symmetry in parsing device tree properties from device node. Let me know your view. I couldn't find an earlier version of this series that used of_pci_get_max_link_speed, have I missed it somewhere? It happened in our internal review. What's your suggestion please, either to go with symmetry in parsing "device_property_read_u32()" or with pci helper function "of_pci_get_max_link_speed"? I'd prefer the helper, the added benefit of this is additional error checking. It also means users can be confident that max-link-speed will behave in the same way as other host controllers that use this field. Ok, i will update it in the next patch version. Regards, Dilip Thanks, Andrew Murray + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "app"); + if (!res) + return -ENXIO; + + lpp->app_base = devm_ioremap_resource(dev, res); + if (IS_ERR(lpp->app_base)) + return PTR_ERR(lpp->app_base); + + ret = intel_pcie_ep_rst_init(lpp); + if (ret) + return ret; Given that this is called from a function '..._get_resources' I don't think we should be resetting anything here. Agree. I will move it out of get_resources(). + + lpp->phy = devm_phy_get(dev, "pciephy"); + if (IS_ERR(lpp
Re: [PATCH v3 2/2] dwc: PCI: intel: Intel PCIe RC controller driver
Quoting Andrew Murray: Quoting Gustavo Pimentel: On 9/12/2019 4:25 PM, Andrew Murray wrote: [...] +static void intel_pcie_max_link_width_setup(struct intel_pcie_port *lpp) +{ + u32 mask, val; + + /* HW auto bandwidth negotiation must be enabled */ + pcie_rc_cfg_wr_mask(lpp, PCIE_LCTLSTS_HW_AW_DIS, 0, PCIE_LCTLSTS); + + mask = PCIE_DIRECT_LINK_WIDTH_CHANGE | PCIE_TARGET_LINK_WIDTH; + val = PCIE_DIRECT_LINK_WIDTH_CHANGE | lpp->lanes; + pcie_rc_cfg_wr_mask(lpp, mask, val, PCIE_MULTI_LANE_CTRL); Is this identical functionality to the writing of PCIE_PORT_LINK_CONTROL in dw_pcie_setup? I ask because if the user sets num-lanes in the DT, will it have the desired effect? intel_pcie_max_link_width_setup() function will be called by sysfs attribute pcie_width_store() to change on the fly. Indeed, but a user may also set num-lanes in the device tree. I'm wondering if, when set in device-tree, it will have the desired effect. Because I don't see anything similar to PCIE_LCTLSTS_HW_AW_DIS in dw_pcie_setup which is what your function does here. I guess I'm trying to avoid the suitation where num-lanes doesn't have the desired effect and the only way to set the num-lanes is throught the sysfs control. I will check this and get back to you. intel_pcie_max_link_width_setup() is doing the lane resizing which is different from the link up/establishment happening during probe. Also PCIE_LCTLSTS_HW_AW_DIS default value is 0 so not setting during the probe or dw_pcie_setup. intel_pcie_max_link_width_setup() is programming as per the designware PCIe controller databook instructions for lane resizing. Below lines are from Designware PCIe databook for lane resizing. Program the TARGET_LINK_WIDTH[5:0] field of the MULTI_LANE_CONTROL_OFF register. Program the DIRECT_LINK_WIDTH_CHANGE2 field of the MULTI_LANE_CONTROL_OFF register. It is assumed that the PCIE_CAP_HW_AUTO_WIDTH_DISABLE field in the LINK_CONTROL_LINK_STATUS_REG register is 0. OK, so there is a difference between initial training and then resizing of the link. Given that this is not Intel specific, should this function exist within the designware driver for the benefit of others? I am ok to add if maintainer agrees with it. Gustavo Pimentel, Could you please let us know your opinion on this. [...] +} + +static void intel_pcie_port_logic_setup(struct intel_pcie_port *lpp) +{ + u32 val, mask, fts; + + switch (lpp->max_speed) { + case PCIE_LINK_SPEED_GEN1: + case PCIE_LINK_SPEED_GEN2: + fts = PCIE_AFR_GEN12_FTS_NUM_DFT; + break; [...] + + if (device_property_read_u32(dev, "max-link-speed", >link_gen)) + lpp->link_gen = 0; /* Fallback to auto */ Is it possible to use of_pci_get_max_link_speed here instead? Thanks for pointing it. of_pci_get_max_link_speed() can be used here. I will update it in the next patch revision. I just remember, earlier we were using of_pci_get_max_link_speed() itself. As per reviewer comments changed to device_property_read_u32() to maintain symmetry in parsing device tree properties from device node. Let me know your view. I couldn't find an earlier version of this series that used of_pci_get_max_link_speed, have I missed it somewhere? It happened in our internal review. What's your suggestion please, either to go with symmetry in parsing "device_property_read_u32()" or with pci helper function "of_pci_get_max_link_speed"? I'd prefer the helper, the added benefit of this is additional error checking. It also means users can be confident that max-link-speed will behave in the same way as other host controllers that use this field. Ok, i will update it in the next patch version. Regards, Dilip Thanks, Andrew Murray + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "app"); + if (!res) + return -ENXIO; + + lpp->app_base = devm_ioremap_resource(dev, res); + if (IS_ERR(lpp->app_base)) + return PTR_ERR(lpp->app_base); + + ret = intel_pcie_ep_rst_init(lpp); + if (ret) + return ret; Given that this is called from a function '..._get_resources' I don't think we should be resetting anything here. Agree. I will move it out of get_resources(). + + lpp->phy = devm_phy_get(dev, "pciephy"); + if (IS_ERR(lpp->phy)) { + ret = PTR_ERR(lpp->phy); + if (ret != -EPROBE_DEFER) + dev_err(dev, "couldn't get pcie-phy: %d\n", ret); + return ret; + } + return 0; +} + +static void intel_pcie_deinit_phy(struct intel_pcie_port *lpp) +{ + phy_exit(lpp->phy); +} + +static int intel_pcie_wait_l2(struct intel_pcie_port *lpp) +{ + u32 value; + int ret; + + if (lpp->max_speed < PCIE_LINK_SPEED_GEN3) + return 0; + + /* Send PME_TURN_OFF message */ + pcie_app_wr_mask(lpp, PCIE_APP_MSG_XMT_PM_TURNOFF, +
Re: [PATCH v3 2/2] dwc: PCI: intel: Intel PCIe RC controller driver
Hi Andy, On 9/5/2019 7:40 PM, Andy Shevchenko wrote: On Thu, Sep 05, 2019 at 11:45:18AM +0100, Andrew Murray wrote: On Wed, Sep 04, 2019 at 06:10:31PM +0800, Dilip Kota wrote: Add support to PCIe RC controller on Intel Universal Gateway SoC. PCIe controller is based of Synopsys Designware pci core. +config PCIE_INTEL_AXI I think that name here is too generic. Classical x86 seems not using this. This PCIe driver is for the Intel Gateway SoCs. So how about naming it is as "pcie-intel-gw"; pcie-intel-gw.c and Kconfig as PCIE_INTEL_GW. Andrew Murray is ok with this naming, please let me know your view. +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.
Re: [PATCH v3 2/2] dwc: PCI: intel: Intel PCIe RC controller driver
Hi Andrew Murray, On 9/11/2019 6:30 PM, Andrew Murray wrote: On Tue, Sep 10, 2019 at 03:46:17PM +0800, Dilip Kota wrote: Hi Andrew Murray, Please find my response inline. On 9/9/2019 4:31 PM, Andrew Murray wrote: On Mon, Sep 09, 2019 at 02:51:03PM +0800, Dilip Kota wrote: On 9/6/2019 7:20 PM, Andrew Murray wrote: On Fri, Sep 06, 2019 at 06:58:11PM +0800, Dilip Kota wrote: Hi Andrew Murray, Thanks for the review. Please find my response inline. On 9/5/2019 6:45 PM, Andrew Murray wrote: On Wed, Sep 04, 2019 at 06:10:31PM +0800, 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 --- Hi Dilip, Thanks for the patch, initial feedback below: 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 I don't think the PCI dependency is required here. I'm also not sure why PCI_MSI is required, we select PCIE_DW_HOST which depends on PCI_MSI_IRQ_DOMAIN which depends on PCI_MSI. Agree, dependency on PCI and PCI_MSI can be removed. I will remove it in next patch revision. +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. I can see this description is similar to others in the same Kconfig, however I'm not sure what value a user gains by knowing implementation details - it's helpful to know that PCIE_INTEL_AXI is based on the Designware core, but is it helpful to know that the Designware core functions are used? + 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 +#in
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
Re-sending the mail, because of delivery failure. sorry for the spam. Hi Martin, On 9/6/2019 4:53 AM, Martin Blumenstingl wrote: Hi, On Thu, Sep 5, 2019 at 4:38 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. indeed, chiptop should not have any child nodes (based on your explanation). I was referring to VRX200 where the RCU syscon has various children (one child node for each hardware module that's part of RCU: reset controller, 2x USB PHY, ...) back to LGM: you said that the LGM RCU registers only contain the reset controller. thus I see no need for the syscon compatible 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
Re: [PATCH v3 2/2] dwc: PCI: intel: Intel PCIe RC controller driver
[Got delivery failure mail; so re-sending the mail] Hi Andrew Murray, Please find my response inline. On 9/9/2019 4:31 PM, Andrew Murray wrote: On Mon, Sep 09, 2019 at 02:51:03PM +0800, Dilip Kota wrote: On 9/6/2019 7:20 PM, Andrew Murray wrote: On Fri, Sep 06, 2019 at 06:58:11PM +0800, Dilip Kota wrote: Hi Andrew Murray, Thanks for the review. Please find my response inline. On 9/5/2019 6:45 PM, Andrew Murray wrote: On Wed, Sep 04, 2019 at 06:10:31PM +0800, 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 --- Hi Dilip, Thanks for the patch, initial feedback below: 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 I don't think the PCI dependency is required here. I'm also not sure why PCI_MSI is required, we select PCIE_DW_HOST which depends on PCI_MSI_IRQ_DOMAIN which depends on PCI_MSI. Agree, dependency on PCI and PCI_MSI can be removed. I will remove it in next patch revision. +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. I can see this description is similar to others in the same Kconfig, however I'm not sure what value a user gains by knowing implementation details - it's helpful to know that PCIE_INTEL_AXI is based on the Designware core, but is it helpful to know that the Designware core functions are used? + 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 "../
Re: [PATCH v3 1/2] dt-bindings: PCI: intel: Add YAML schemas for the PCIe RC controller
On 9/6/2019 5:19 PM, Andy Shevchenko wrote: On Thu, Sep 05, 2019 at 10:31:29PM +0200, Martin Blumenstingl wrote: On Wed, Sep 4, 2019 at 12:11 PM Dilip Kota wrote: + 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 I'm not Rob, though I consider more practical to have a hyphenated variant. I am okay to "pcie-phy". I will update it in next patch version. Regards, Dilip
Re: [PATCH v3 2/2] dwc: PCI: intel: Intel PCIe RC controller driver
On 9/6/2019 7:20 PM, Andrew Murray wrote: On Fri, Sep 06, 2019 at 06:58:11PM +0800, Dilip Kota wrote: Hi Andrew Murray, Thanks for the review. Please find my response inline. On 9/5/2019 6:45 PM, Andrew Murray wrote: On Wed, Sep 04, 2019 at 06:10:31PM +0800, 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 --- Hi Dilip, Thanks for the patch, initial feedback below: 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 I don't think the PCI dependency is required here. I'm also not sure why PCI_MSI is required, we select PCIE_DW_HOST which depends on PCI_MSI_IRQ_DOMAIN which depends on PCI_MSI. Agree, dependency on PCI and PCI_MSI can be removed. I will remove it in next patch revision. +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. I can see this description is similar to others in the same Kconfig, however I'm not sure what value a user gains by knowing implementation details - it's helpful to know that PCIE_INTEL_AXI is based on the Designware core, but is it helpful to know that the Designware core functions are used? + 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" Please remove this - it isn't needed. Ok, will remove it in next patch revision. +#include "pcie-designware.h" + +#define PCIE_CCRID 0x8 + +#define PCIE_LCAP
Re: [PATCH v3 2/2] dwc: PCI: intel: Intel PCIe RC controller driver
Hi Andrew Murray, Thanks for the review. Please find my response inline. On 9/5/2019 6:45 PM, Andrew Murray wrote: On Wed, Sep 04, 2019 at 06:10:31PM +0800, 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 --- Hi Dilip, Thanks for the patch, initial feedback below: 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 I don't think the PCI dependency is required here. I'm also not sure why PCI_MSI is required, we select PCIE_DW_HOST which depends on PCI_MSI_IRQ_DOMAIN which depends on PCI_MSI. Agree, dependency on PCI and PCI_MSI can be removed. I will remove it in next patch revision. +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. I can see this description is similar to others in the same Kconfig, however I'm not sure what value a user gains by knowing implementation details - it's helpful to know that PCIE_INTEL_AXI is based on the Designware core, but is it helpful to know that the Designware core functions are used? + 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" Please remove this - it isn't needed. Ok, will remove it in next patch revision. +#include "pcie-designware.h" + +#define PCIE_CCRID 0x8 + +#define PCIE_LCAP 0x7C +#define PCIE_LCAP_MAX_LINK_SPEED GENMASK(3, 0) +#define PCIE_LCAP_M
Re: [PATCH v3 2/2] dwc: PCI: intel: Intel PCIe RC controller driver
Hi Andy, Thanks for the review comments, please find my response inline. On 9/4/2019 9:05 PM, Andy Shevchenko wrote: On Wed, Sep 04, 2019 at 06:10:31PM +0800, Dilip Kota wrote: Add support to PCIe RC controller on Intel Universal Gateway SoC. PCIe controller is based of Synopsys Designware pci core. Thanks for an update. My comments below. +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. This hunk is full of indentation issues. Please, see how it's done in other cases which are already part of upstream kernel. Sure, I will fix it. +#define PCIE_APP_INTX_OFST 12 +#define PCIE_APP_IRN_INT (PCIE_APP_IRN_AER_REPORT | PCIE_APP_IRN_PME | \ It would be slightly easier to read if the value starts from the next line. Agree, i will update it in the next patch revision. + PCIE_APP_IRN_RX_VDM_MSG | PCIE_APP_IRN_SYS_ERR_RC | \ + PCIE_APP_IRN_PM_TO_ACK | PCIE_APP_IRN_MSG_LTR | \ + PCIE_APP_IRN_BW_MGT | PCIE_APP_IRN_LINK_AUTO_BW_STAT | \ + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTA) | \ + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTB) | \ + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTC) | \ + (PCIE_APP_INTX_OFST + PCI_INTERRUPT_INTD)) +static void intel_pcie_link_setup(struct intel_pcie_port *lpp) +{ + u32 val; + + val = pcie_rc_cfg_rd(lpp, PCIE_LCAP); + lpp->max_speed = FIELD_GET(PCIE_LCAP_MAX_LINK_SPEED, val); + lpp->max_width = FIELD_GET(PCIE_LCAP_MAX_LENGTH_WIDTH, val); + + val = pcie_rc_cfg_rd(lpp, PCIE_LCTLSTS); + + val &= ~(PCIE_LCTLSTS_LINK_DISABLE | PCIE_LCTLSTS_ASPM_ENABLE); + val |= (PCIE_LCTLSTS_SLOT_CLK_CFG | PCIE_LCTLSTS_COM_CLK_CFG | + PCIE_LCTLSTS_RCB128); Unnecessary parentheses. Sure, i will fix it. + pcie_rc_cfg_wr(lpp, val, PCIE_LCTLSTS); +} + switch (lpp->max_speed) { + case PCIE_LINK_SPEED_GEN1: + case PCIE_LINK_SPEED_GEN2: + fts = PCIE_AFR_GEN12_FTS_NUM_DFT; + break; You may group this with default case, right? Sure, i am ok to group it with default case. I will update it in the next patch revision. + case PCIE_LINK_SPEED_GEN3: + fts = PCIE_AFR_GEN3_FTS_NUM_DFT; + break; + case PCIE_LINK_SPEED_GEN4: + fts = PCIE_AFR_GEN4_FTS_NUM_DFT; + break; + default: + fts = PCIE_AFR_GEN12_FTS_NUM_DFT; + break; + } + trace_printk("PCIe misc interrupt status 0x%x\n", reg); Hmm... Doesn't it give you a BIG warning? kernel/trace/trace.c:trace_printk_init_buffers() I hope this should be fine as it come in debug builds only. + return IRQ_HANDLED; +} Perhaps the PCI needs some trace points instead. +static int intel_pcie_setup_irq(struct intel_pcie_port *lpp) +{ + struct device *dev = lpp->pci.dev; + struct platform_device *pdev; + char *irq_name; + int irq, ret; + + pdev = to_platform_device(dev); + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + dev_err(dev, "missing sys integrated irq resource\n"); Redundant since this cycle. platform_get_irq() is not printing any error message, so kept a error message here. + return irq; + } + irq_name = devm_kasprintf(dev, GFP_KERNEL, "pcie_misc%d", lpp->id); + if (!irq_name) { + dev_err(dev, "failed to alloc irq name\n"); Not very useful. initcall_debug shows an error code. Thanks for pointing it. I will remove this debug print. + return -ENOMEM; + } + + ret = devm_request_irq(dev, irq, intel_pcie_core_isr, + IRQF_SHARED, irq_name, lpp); + if (ret) { + dev_err(dev, "request irq %d failed\n", irq); + return ret; + } + blank line. Agree, will fix it. + /* Enable integrated interrupts */ + pcie_app_wr_mask(lpp, PCIE_APP_IRN_INT, +PCIE_APP_IRN_INT, PCIE_APP_IRNEN); At least one parameter can be located on the previous line. Agree, will fix it. + return ret; +} +static int intel_pcie_get_resources(struct platform_device *pdev) +{ + struct intel_pcie_port *lpp; + struct resource *res; + struct dw_pcie *pci; + struct device *dev; + int ret; + + lpp = platform_get_drvdata(pdev);
Re: [PATCH v3 1/2] dt-bindings: PCI: intel: Add YAML schemas for the PCIe RC controller
Hi Chuan Hua, On 9/5/2019 10:23 AM, Chuan Hua, Lei wrote: 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) Sure, i will fix it. That's a typo error. Thanks for pointing it. Regards Dilip
[PATCH v3 1/2] dt-bindings: PCI: intel: Add YAML schemas for the PCIe RC controller
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' 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>; +}; -- 2.11.0
[PATCH v3 2/2] dwc: PCI: intel: Intel PCIe RC controller driver
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_SPEE
[PATCH v3 0/2] PCI: Add Intel PCIe Driver and respective dt-binding yaml file
Intel PCIe is synopsys based controller utilizes the Designware framework for host initialization and intel application specific register configurations. Changes on v3: Compared to v2, map_irq() patch is removed as it is no longer required for Intel PCIe driver. Intel PCIe driver does platform specific interrupt configuration during core initialization. So changed the subject line too. Address v2 review comments for DT binding and PCIe driver Dilip Kota (2): dt-bindings: PCI: intel: Add YAML schemas for the PCIe RC controller dwc: PCI: intel: Intel PCIe RC controller driver .../devicetree/bindings/pci/intel,lgm-pcie.yaml| 137 drivers/pci/controller/dwc/Kconfig | 13 + drivers/pci/controller/dwc/Makefile| 1 + drivers/pci/controller/dwc/pcie-intel-axi.c| 840 + 4 files changed, 991 insertions(+) create mode 100644 Documentation/devicetree/bindings/pci/intel,lgm-pcie.yaml create mode 100644 drivers/pci/controller/dwc/pcie-intel-axi.c -- 2.11.0
Re: [PATCH v2 1/2] dt-bindings: reset: Add YAML schemas for the Intel Reset controller
Hi Rob, On 8/27/2019 10:04 PM, Dilip Kota wrote: Hi Rob, On 8/26/2019 7:23 PM, Rob Herring wrote: On Mon, Aug 26, 2019 at 4:52 AM Dilip Kota wrote: Hi Rob, On 8/23/2019 8:25 PM, Rob Herring wrote: On Fri, Aug 23, 2019 at 12:28 AM Dilip Kota wrote: Add YAML schemas for the reset controller on Intel Lightening Mountain (LGM) SoC. Signed-off-by: Dilip Kota --- Changes on v2: Address review comments Update the compatible property definition Add description for reset-cells Add 'additionalProperties: false' property .../bindings/reset/intel,syscon-reset.yaml | 53 ++ 1 file changed, 53 insertions(+) create mode 100644 Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml diff --git a/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml b/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml new file mode 100644 index ..3403a967190a --- /dev/null +++ b/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml @@ -0,0 +1,53 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/reset/intel,syscon-reset.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Intel Lightening Mountain SoC System Reset Controller + +maintainers: + - Dilip Kota + +properties: + compatible: + items: + - const: intel,rcu-lgm + - const: syscon + + reg: + description: Reset controller register base address and size + + intel,global-reset: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: Global reset register offset and bit offset. + + "#reset-cells": + const: 2 + description: | + The 1st cell is the register offset. + The 2nd cell is the bit offset in the register. + +required: + - compatible + - reg + - intel,global-reset + - "#reset-cells" + +additionalProperties: false + +examples: + - | + rcu0: reset-controller@ { + compatible = "intel,rcu-lgm", "syscon"; + reg = <0x00 0x8>; + intel,global-reset = <0x10 30>; + #reset-cells = <2>; + }; + + pcie_phy0: pciephy@... { + ... You need to run 'make dt_binding_check' and fix the warnings. The example has to be buildable and it is not. Sure, i will correct this pcie_phy0 node. But i didn't get any warnings for make dt_binding_check CHKDT Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml DTC Documentation/devicetree/bindings/arm/renesas.example.dt.yaml FATAL ERROR: Unknown output format "yaml" Will DTC report about the example node errors? But, DTC is failing with FATAL_ERROR. I tried it even after installing libyaml and headers in my local directory and export the path, but no luck.(ref: https://lkml.org/lkml/2018/12/3/951) Could you please let me know if i miss anything and help me to proceed further. See Documentation/devicetree/writing-schema.md I have followed all the steps mentioned in the document before keeping the mail itself. Does the dtc script looks for libyaml and its header files at any default or specific location? DTC is working for me now. It is working for me after updating the libyaml and header paths in scripts/dtc/Makefile and yamltree.c (since i have installed libyaml and header files in my local directories) I have fixed all the warnings and DTC checks are successful. I will push the changes in the next patch version. DTC Documentation/devicetree/bindings/reset/intel,syscon-reset.example.dt.yaml CHECK Documentation/devicetree/bindings/reset/intel,syscon-reset.example.dt.yaml Regards, Dilip
Re: [PATCH v2 1/2] dt-bindings: reset: Add YAML schemas for the Intel Reset controller
Hi Rob, On 8/26/2019 7:23 PM, Rob Herring wrote: On Mon, Aug 26, 2019 at 4:52 AM Dilip Kota wrote: Hi Rob, On 8/23/2019 8:25 PM, Rob Herring wrote: On Fri, Aug 23, 2019 at 12:28 AM Dilip Kota wrote: Add YAML schemas for the reset controller on Intel Lightening Mountain (LGM) SoC. Signed-off-by: Dilip Kota --- Changes on v2: Address review comments Update the compatible property definition Add description for reset-cells Add 'additionalProperties: false' property .../bindings/reset/intel,syscon-reset.yaml | 53 ++ 1 file changed, 53 insertions(+) create mode 100644 Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml diff --git a/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml b/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml new file mode 100644 index ..3403a967190a --- /dev/null +++ b/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml @@ -0,0 +1,53 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/reset/intel,syscon-reset.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Intel Lightening Mountain SoC System Reset Controller + +maintainers: + - Dilip Kota + +properties: + compatible: +items: + - const: intel,rcu-lgm + - const: syscon + + reg: +description: Reset controller register base address and size + + intel,global-reset: +$ref: /schemas/types.yaml#/definitions/uint32-array +description: Global reset register offset and bit offset. + + "#reset-cells": +const: 2 +description: | + The 1st cell is the register offset. + The 2nd cell is the bit offset in the register. + +required: + - compatible + - reg + - intel,global-reset + - "#reset-cells" + +additionalProperties: false + +examples: + - | +rcu0: reset-controller@ { +compatible = "intel,rcu-lgm", "syscon"; +reg = <0x00 0x8>; +intel,global-reset = <0x10 30>; +#reset-cells = <2>; +}; + +pcie_phy0: pciephy@... { +... You need to run 'make dt_binding_check' and fix the warnings. The example has to be buildable and it is not. Sure, i will correct this pcie_phy0 node. But i didn't get any warnings for make dt_binding_check CHKDT Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml DTC Documentation/devicetree/bindings/arm/renesas.example.dt.yaml FATAL ERROR: Unknown output format "yaml" Will DTC report about the example node errors? But, DTC is failing with FATAL_ERROR. I tried it even after installing libyaml and headers in my local directory and export the path, but no luck.(ref: https://lkml.org/lkml/2018/12/3/951) Could you please let me know if i miss anything and help me to proceed further. See Documentation/devicetree/writing-schema.md I have followed all the steps mentioned in the document before keeping the mail itself. Does the dtc script looks for libyaml and its header files at any default or specific location? Regards, Dilip Rob
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
On 8/27/2019 4:14 AM, Martin Blumenstingl wrote: second example: pcie-tegra194 (only in -next, will be part of v5.4) struct tegra_pcie_dw { ... struct dw_pcie pci; ... }; so some drivers store a pointer pointer to the dw_pcie struct vs. embedding the dw_pcie struct directly. as far as I know the result will be equal, except that you don't have to use a second devm_kzalloc for struct dw_pcie (and thus reducing memory fragmentation). Okay, i will change it to "struct dw_pcie pci;" Thanks for the feedback. Regards, Dilip
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
Hi Martin, On 8/27/2019 11:09 AM, Chuan Hua, Lei wrote: [...] now I am wondering: - if we don't have to disable the interrupt line (once it is enabled), why can't we enable all of these interrupts at initialization time (instead of doing it on-demand)? Good point! we even can remote map_irq patch, directly call of_irq_parse_and_map_pci as other drivers do. Irrespective of disabling, imo interrupts(A/B/C/D) should be enabled when they are requested; which happens during map_irq() call. - if the interrupts do have to be disabled again (that is what I assumed so far) then where is this supposed to happen? (my solution for this was to implement a simple interrupt controller within the PCIe driver which only supports enable/disable. disclaimer: I didn't ask the PCI or interrupt maintainers for feedback on this yet) [...] We can implement one interrupt controller, but personally, it has too much overhead. If we follow this way, almost all modules of all old lantiq SoCs can implement one interrupt controller. Maybe you can check with PCI maintainer for their comments. [...] This is needed. In the old driver, we fixed this by fixup. The original comment as follows, /* * The root complex has a hardwired class of PCI_CLASS_NETWORK_OTHER or * PCI_CLASS_BRIDGE_HOST, when it is operating as a root complex this * needs to be switched to * PCI_CLASS_BRIDGE_PCI */ that would be a good comment to add if you really need it can you please look at dw_pcie_setup_rc (from pcie-designware-host.c), it does: /* Enable write permission for the DBI read-only register */ dw_pcie_dbi_ro_wr_en(pci); /* Program correct class for RC */ dw_pcie_wr_own_conf(pp, PCI_CLASS_DEVICE, 2, PCI_CLASS_BRIDGE_PCI); /* Better disable write permission right after the update */ dw_pcie_dbi_ro_wr_dis(pci); so my understanding is that there is a functional requirement to set the class to PCI_CLASS_BRIDGE_PCI however, that requirement is already covered by pcie-designware-host.c I will task Dilip to check if we can use dwc one. dw_pcie_setup_rc () cannot be called here because, it is not doing PCI_CLASS_BRIDGE_PCI set alone, it is configuring many other things. [...] Regards, Dilip
Re: [PATCH v2 1/2] dt-bindings: reset: Add YAML schemas for the Intel Reset controller
Hi Rob, On 8/23/2019 8:25 PM, Rob Herring wrote: On Fri, Aug 23, 2019 at 12:28 AM Dilip Kota wrote: Add YAML schemas for the reset controller on Intel Lightening Mountain (LGM) SoC. Signed-off-by: Dilip Kota --- Changes on v2: Address review comments Update the compatible property definition Add description for reset-cells Add 'additionalProperties: false' property .../bindings/reset/intel,syscon-reset.yaml | 53 ++ 1 file changed, 53 insertions(+) create mode 100644 Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml diff --git a/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml b/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml new file mode 100644 index ..3403a967190a --- /dev/null +++ b/Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml @@ -0,0 +1,53 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/reset/intel,syscon-reset.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Intel Lightening Mountain SoC System Reset Controller + +maintainers: + - Dilip Kota + +properties: + compatible: +items: + - const: intel,rcu-lgm + - const: syscon + + reg: +description: Reset controller register base address and size + + intel,global-reset: +$ref: /schemas/types.yaml#/definitions/uint32-array +description: Global reset register offset and bit offset. + + "#reset-cells": +const: 2 +description: | + The 1st cell is the register offset. + The 2nd cell is the bit offset in the register. + +required: + - compatible + - reg + - intel,global-reset + - "#reset-cells" + +additionalProperties: false + +examples: + - | +rcu0: reset-controller@ { +compatible = "intel,rcu-lgm", "syscon"; +reg = <0x00 0x8>; +intel,global-reset = <0x10 30>; +#reset-cells = <2>; +}; + +pcie_phy0: pciephy@... { +... You need to run 'make dt_binding_check' and fix the warnings. The example has to be buildable and it is not. Sure, i will correct this pcie_phy0 node. But i didn't get any warnings for make dt_binding_check CHKDT Documentation/devicetree/bindings/reset/intel,syscon-reset.yaml DTC Documentation/devicetree/bindings/arm/renesas.example.dt.yaml FATAL ERROR: Unknown output format "yaml" Will DTC report about the example node errors? But, DTC is failing with FATAL_ERROR. I tried it even after installing libyaml and headers in my local directory and export the path, but no luck.(ref: https://lkml.org/lkml/2018/12/3/951) Could you please let me know if i miss anything and help me to proceed further. Regards, Dilip +/* address offset: 0x10, bit offset: 12 */ +resets = < 0x10 12>; +... +}; -- 2.11.0
Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC
On 8/23/2019 6:09 PM, Philipp Zabel wrote: On Fri, 2019-08-23 at 17:47 +0800, Dilip Kota wrote: [...] Thanks for pointing it out. Reset is not supported on LGM platform. I will update the reset_device() definition to "return -EOPNOTSUPP" In that case you can just drop intel_reset_device() completely, the core checks whether ops->reset is set before trying to call it. Agree, will do the same. Regards, Dilip regards Philipp
Re: [PATCH v2 3/3] dwc: PCI: intel: Intel PCIe RC controller driver
[Got delivery failure mail , so re-sending the mail] Hi Martin, Thanks for review comments, please find my response inline. On 8/26/2019 11:30 AM, Chuan Hua, Lei wrote: 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_RCB128 BIT(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_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_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_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_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 change speed and link width on the fly which is not supported by dwc