All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/5] staging: mt7621-pci: re-do reset boot process
@ 2020-03-11 18:58 Sergio Paracuellos
  2020-03-11 18:58 ` [PATCH v2 1/5] staging: mt7621-pci: use gpios for properly reset Sergio Paracuellos
                   ` (5 more replies)
  0 siblings, 6 replies; 8+ messages in thread
From: Sergio Paracuellos @ 2020-03-11 18:58 UTC (permalink / raw)
  To: gregkh; +Cc: ryder.lee, driverdev-devel, weijie.gao, gerg, neil

Some time ago Greg Ungerer reported some random hangs using
the staging mt7621-pci driver:

See:
* http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2019-June/134947.html 

Try to fix that is the main motivation of this patch series.

Also in openwrt there is a driver for mt7621-pci which seems was rewritten
from scratch (for kernel 4.14) by Ryder Lee and Weijie Gao from mediatek. 
There the approach for reset assert-deassert process is to set as 'gpio'
the function for all the 'pcie' group for the pinctrl driver and use those
gpio's as a reset for the end points. The driver I am talking about is still
using legacy pci and legacy gpio kernel interfaces. IMHO, the correct thing
to do is make this staging driver properly clean and functional and put it
in its correct place in the mainline.

See:
* https://gist.github.com/dengqf6/7a9e9b4032d99f1a91dd9256c8a65c36

Because of all of this this patch series tries to avoid random hangs of boot
trying to use the 'reset-gpios' approach.

Changes are being tested by openwrt people and seems to work.

Hope this helps.

Changes in v2:
    * restore configuration for pers mode to GPIO.
    * Avoid to read FTS_NUM register in reset state.

Best regards,
    Sergio Paracuellos


Sergio Paracuellos (5):
  staging: mt7621-pci: use gpios for properly reset
  staging: mt7621-pci: change value for 'PERST_DELAY_US'
  staging: mt7621-dts: make use of 'reset-gpios' property for pci
  staging: mt7621-pci: bindings: update doc accordly to last changes
  staging: mt7621-pci: release gpios after pci initialization

 drivers/staging/mt7621-dts/mt7621.dtsi        | 11 ++-
 .../mt7621-pci/mediatek,mt7621-pci.txt        |  7 +-
 drivers/staging/mt7621-pci/pci-mt7621.c       | 94 ++++++++++++-------
 3 files changed, 72 insertions(+), 40 deletions(-)

-- 
2.25.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/5] staging: mt7621-pci: use gpios for properly reset
  2020-03-11 18:58 [PATCH v2 0/5] staging: mt7621-pci: re-do reset boot process Sergio Paracuellos
@ 2020-03-11 18:58 ` Sergio Paracuellos
  2020-03-11 18:58 ` [PATCH v2 2/5] staging: mt7621-pci: change value for 'PERST_DELAY_US' Sergio Paracuellos
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Sergio Paracuellos @ 2020-03-11 18:58 UTC (permalink / raw)
  To: gregkh; +Cc: ryder.lee, driverdev-devel, weijie.gao, gerg, neil

Original driver code was using three gpio's for reset
asserts and deasserts the pcis. Instead of using that
a general reset control with a perst gpio was introduced
and it seems it is partially working but sometimes there
are some unexpected hangs on boot. This commit make use of
the three original gpios using 'reset-gpios' property of
the device tree and removes the reset line and perst gpio.
After this changes the order to make all assert and
deassert in the 'probe' process makes more sense:
* Parse device tree.
* make assert of the RC's and EP's before doing anything else.
* make deassert of the RC's before initializing the phy.
* Init the phy.
* make deassert of the EP's before initialize pci ports.
* Normal PCI initialization.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-pci/pci-mt7621.c | 82 +++++++++++++++----------
 1 file changed, 49 insertions(+), 33 deletions(-)

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index 770dabd3a70d..45ebd880ef6c 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -95,6 +95,7 @@
  * @pcie: pointer to PCIe host info
  * @phy: pointer to PHY control block
  * @pcie_rst: pointer to port reset control
+ * @gpio_rst: gpio reset
  * @slot: port slot
  * @enabled: indicates if port is enabled
  */
@@ -104,6 +105,7 @@ struct mt7621_pcie_port {
 	struct mt7621_pcie *pcie;
 	struct phy *phy;
 	struct reset_control *pcie_rst;
+	struct gpio_desc *gpio_rst;
 	u32 slot;
 	bool enabled;
 };
@@ -117,8 +119,6 @@ struct mt7621_pcie_port {
  * @offset: IO / Memory offset
  * @dev: Pointer to PCIe device
  * @ports: pointer to PCIe port information
- * @perst: gpio reset
- * @rst: pointer to pcie reset
  * @resets_inverted: depends on chip revision
  * reset lines are inverted.
  */
@@ -133,8 +133,6 @@ struct mt7621_pcie {
 		resource_size_t io;
 	} offset;
 	struct list_head ports;
-	struct gpio_desc *perst;
-	struct reset_control *rst;
 	bool resets_inverted;
 };
 
@@ -210,16 +208,14 @@ static void write_config(struct mt7621_pcie *pcie, unsigned int dev,
 	pcie_write(pcie, val, RALINK_PCI_CONFIG_DATA);
 }
 
-static inline void mt7621_perst_gpio_pcie_assert(struct mt7621_pcie *pcie)
+static inline void mt7621_rst_gpio_pcie_assert(struct mt7621_pcie_port *port)
 {
-	gpiod_set_value(pcie->perst, 0);
-	mdelay(PERST_DELAY_US);
+	gpiod_set_value(port->gpio_rst, 0);
 }
 
-static inline void mt7621_perst_gpio_pcie_deassert(struct mt7621_pcie *pcie)
+static inline void mt7621_rst_gpio_pcie_deassert(struct mt7621_pcie_port *port)
 {
-	gpiod_set_value(pcie->perst, 1);
-	mdelay(PERST_DELAY_US);
+	gpiod_set_value(port->gpio_rst, 1);
 }
 
 static inline bool mt7621_pcie_port_is_linkup(struct mt7621_pcie_port *port)
@@ -367,6 +363,13 @@ static int mt7621_pcie_parse_port(struct mt7621_pcie *pcie,
 	if (IS_ERR(port->phy))
 		return PTR_ERR(port->phy);
 
+	port->gpio_rst = devm_gpiod_get_index(dev, "reset", slot,
+					      GPIOD_OUT_HIGH);
+	if (IS_ERR(port->gpio_rst)) {
+		dev_err(dev, "Unable to request GPIO reset in slot %d\n", slot);
+		return PTR_ERR(port->gpio_rst);
+	}
+
 	port->slot = slot;
 	port->pcie = pcie;
 
@@ -383,12 +386,6 @@ static int mt7621_pcie_parse_dt(struct mt7621_pcie *pcie)
 	struct resource regs;
 	int err;
 
-	pcie->perst = devm_gpiod_get(dev, "perst", GPIOD_OUT_HIGH);
-	if (IS_ERR(pcie->perst)) {
-		dev_err(dev, "failed to get gpio perst\n");
-		return PTR_ERR(pcie->perst);
-	}
-
 	err = of_address_to_resource(node, 0, &regs);
 	if (err) {
 		dev_err(dev, "missing \"reg\" property\n");
@@ -399,12 +396,6 @@ static int mt7621_pcie_parse_dt(struct mt7621_pcie *pcie)
 	if (IS_ERR(pcie->base))
 		return PTR_ERR(pcie->base);
 
-	pcie->rst = devm_reset_control_get_exclusive(dev, "pcie");
-	if (PTR_ERR(pcie->rst) == -EPROBE_DEFER) {
-		dev_err(dev, "failed to get pcie reset control\n");
-		return PTR_ERR(pcie->rst);
-	}
-
 	for_each_available_child_of_node(node, child) {
 		int slot;
 
@@ -458,16 +449,49 @@ static int mt7621_pcie_init_port(struct mt7621_pcie_port *port)
 	return 0;
 }
 
+static void mt7621_pcie_reset_assert(struct mt7621_pcie *pcie)
+{
+	struct mt7621_pcie_port *port;
+
+	list_for_each_entry(port, &pcie->ports, list) {
+		/* PCIe RC reset assert */
+		mt7621_control_assert(port);
+
+		/* PCIe EP reset assert */
+		mt7621_rst_gpio_pcie_assert(port);
+	}
+
+	mdelay(PERST_DELAY_US);
+}
+
+static void mt7621_pcie_reset_rc_deassert(struct mt7621_pcie *pcie)
+{
+	struct mt7621_pcie_port *port;
+
+	list_for_each_entry(port, &pcie->ports, list)
+		mt7621_control_deassert(port);
+}
+
+static void mt7621_pcie_reset_ep_deassert(struct mt7621_pcie *pcie)
+{
+	struct mt7621_pcie_port *port;
+
+	list_for_each_entry(port, &pcie->ports, list)
+		mt7621_rst_gpio_pcie_deassert(port);
+
+	mdelay(PERST_DELAY_US);
+}
+
 static void mt7621_pcie_init_ports(struct mt7621_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
 	struct mt7621_pcie_port *port, *tmp;
-	u32 val = 0;
 	int err;
 
 	rt_sysc_m32(PERST_MODE_MASK, PERST_MODE_GPIO, MT7621_GPIO_MODE);
 
-	mt7621_perst_gpio_pcie_assert(pcie);
+	mt7621_pcie_reset_assert(pcie);
+	mt7621_pcie_reset_rc_deassert(pcie);
 
 	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
 		u32 slot = port->slot;
@@ -476,16 +500,10 @@ static void mt7621_pcie_init_ports(struct mt7621_pcie *pcie)
 		if (err) {
 			dev_err(dev, "Initiating port %d failed\n", slot);
 			list_del(&port->list);
-		} else {
-			val = read_config(pcie, slot, PCIE_FTS_NUM);
-			dev_info(dev, "Port %d N_FTS = %x\n", slot,
-				 (unsigned int)val);
 		}
 	}
 
-	reset_control_assert(pcie->rst);
-
-	mt7621_perst_gpio_pcie_deassert(pcie);
+	mt7621_pcie_reset_ep_deassert(pcie);
 
 	list_for_each_entry(port, &pcie->ports, list) {
 		u32 slot = port->slot;
@@ -499,8 +517,6 @@ static void mt7621_pcie_init_ports(struct mt7621_pcie *pcie)
 			port->enabled = false;
 		}
 	}
-
-	reset_control_deassert(pcie->rst);
 }
 
 static void mt7621_pcie_enable_port(struct mt7621_pcie_port *port)
-- 
2.25.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/5] staging: mt7621-pci: change value for 'PERST_DELAY_US'
  2020-03-11 18:58 [PATCH v2 0/5] staging: mt7621-pci: re-do reset boot process Sergio Paracuellos
  2020-03-11 18:58 ` [PATCH v2 1/5] staging: mt7621-pci: use gpios for properly reset Sergio Paracuellos
@ 2020-03-11 18:58 ` Sergio Paracuellos
  2020-03-11 18:58 ` [PATCH v2 3/5] staging: mt7621-dts: make use of 'reset-gpios' property for pci Sergio Paracuellos
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Sergio Paracuellos @ 2020-03-11 18:58 UTC (permalink / raw)
  To: gregkh; +Cc: ryder.lee, driverdev-devel, weijie.gao, gerg, neil

Value of 'PERST_DELAY_US' is too high and it is ok just
to set up to 100 us.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-pci/pci-mt7621.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index 45ebd880ef6c..0880a21f2620 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -86,7 +86,7 @@
 #define MEMORY_BASE			0x0
 #define PERST_MODE_MASK			GENMASK(11, 10)
 #define PERST_MODE_GPIO			BIT(10)
-#define PERST_DELAY_US			1000
+#define PERST_DELAY_US			100
 
 /**
  * struct mt7621_pcie_port - PCIe port information
-- 
2.25.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 3/5] staging: mt7621-dts: make use of 'reset-gpios' property for pci
  2020-03-11 18:58 [PATCH v2 0/5] staging: mt7621-pci: re-do reset boot process Sergio Paracuellos
  2020-03-11 18:58 ` [PATCH v2 1/5] staging: mt7621-pci: use gpios for properly reset Sergio Paracuellos
  2020-03-11 18:58 ` [PATCH v2 2/5] staging: mt7621-pci: change value for 'PERST_DELAY_US' Sergio Paracuellos
@ 2020-03-11 18:58 ` Sergio Paracuellos
  2020-03-11 18:58 ` [PATCH v2 4/5] staging: mt7621-pci: bindings: update doc accordly to last changes Sergio Paracuellos
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Sergio Paracuellos @ 2020-03-11 18:58 UTC (permalink / raw)
  To: gregkh; +Cc: ryder.lee, driverdev-devel, weijie.gao, gerg, neil

Properly set pins for group pcie as 'gpio' function and declare
gpio's in the pci node to make reset stuff properly functional.
Delete no more needed general reset and previous pers gpio which
is now being used in 'reset-gpios' property.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-dts/mt7621.dtsi | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
index d89d68ffa7bc..488474153535 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -286,7 +286,7 @@ mdio0 {
 		pcie_pins: pcie0 {
 			pcie0 {
 				groups = "pcie";
-				function = "pcie rst";
+				function = "gpio";
 			};
 		};
 
@@ -512,7 +512,6 @@ pcie: pcie@1e140000 {
 		#address-cells = <3>;
 		#size-cells = <2>;
 
-		perst-gpio = <&gpio 19 GPIO_ACTIVE_HIGH>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&pcie_pins>;
 
@@ -532,13 +531,17 @@ pcie: pcie@1e140000 {
 
 		status = "disabled";
 
-		resets = <&rstctrl 23 &rstctrl 24 &rstctrl 25 &rstctrl 26>;
-		reset-names = "pcie", "pcie0", "pcie1", "pcie2";
+		resets = <&rstctrl 24 &rstctrl 25 &rstctrl 26>;
+		reset-names = "pcie0", "pcie1", "pcie2";
 		clocks = <&clkctrl 24 &clkctrl 25 &clkctrl 26>;
 		clock-names = "pcie0", "pcie1", "pcie2";
 		phys = <&pcie0_phy 0>, <&pcie0_phy 1>, <&pcie1_phy 0>;
 		phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2";
 
+		reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>,
+				<&gpio 8 GPIO_ACTIVE_LOW>,
+				<&gpio 7 GPIO_ACTIVE_LOW>;
+
 		pcie@0,0 {
 			reg = <0x0000 0 0 0 0>;
 			#address-cells = <3>;
-- 
2.25.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 4/5] staging: mt7621-pci: bindings: update doc accordly to last changes
  2020-03-11 18:58 [PATCH v2 0/5] staging: mt7621-pci: re-do reset boot process Sergio Paracuellos
                   ` (2 preceding siblings ...)
  2020-03-11 18:58 ` [PATCH v2 3/5] staging: mt7621-dts: make use of 'reset-gpios' property for pci Sergio Paracuellos
@ 2020-03-11 18:58 ` Sergio Paracuellos
  2020-03-11 18:58 ` [PATCH v2 5/5] staging: mt7621-pci: release gpios after pci initialization Sergio Paracuellos
  2020-03-12  4:56 ` [PATCH v2 0/5] staging: mt7621-pci: re-do reset boot process Greg Ungerer
  5 siblings, 0 replies; 8+ messages in thread
From: Sergio Paracuellos @ 2020-03-11 18:58 UTC (permalink / raw)
  To: gregkh; +Cc: ryder.lee, driverdev-devel, weijie.gao, gerg, neil

Properly update bindings documentation with added 'reset-gpios'
property. Delete also 'perst-gpio' which is not being used anymore.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt b/drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt
index 604ec813bd45..327a68267309 100644
--- a/drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt
+++ b/drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt
@@ -6,7 +6,6 @@ Required properties:
 - reg: Base addresses and lengths of the PCIe subsys and root ports.
 - bus-range: Range of bus numbers associated with this controller.
 - #address-cells: Address representation for root ports (must be 3)
-- perst-gpio: PCIe reset signal line.
 - pinctrl-names : The pin control state names.
 - pinctrl-0: The "default" pinctrl state.
 - #size-cells: Size representation for root ports (must be 2)
@@ -24,6 +23,7 @@ Required properties:
   See ../clocks/clock-bindings.txt for details.
 - clock-names: Must be "pcie0", "pcie1", "pcieN"... based on the number of
   root ports.
+- reset-gpios: GPIO specs for the reset pins.
 
 In addition, the device tree node must have sub-nodes describing each PCIe port
 interface, having the following mandatory properties:
@@ -49,7 +49,6 @@ Example for MT7621:
 		#address-cells = <3>;
 		#size-cells = <2>;
 
-		perst-gpio = <&gpio 19 GPIO_ACTIVE_HIGH>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&pcie_pins>;
 
@@ -74,6 +73,10 @@ Example for MT7621:
 		clocks = <&clkctrl 24 &clkctrl 25 &clkctrl 26>;
 		clock-names = "pcie0", "pcie1", "pcie2";
 
+		reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>,
+				<&gpio 8 GPIO_ACTIVE_LOW>,
+				<&gpio 7 GPIO_ACTIVE_LOW>;
+
 		pcie@0,0 {
 			reg = <0x0000 0 0 0 0>;
 			#address-cells = <3>;
-- 
2.25.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 5/5] staging: mt7621-pci: release gpios after pci initialization
  2020-03-11 18:58 [PATCH v2 0/5] staging: mt7621-pci: re-do reset boot process Sergio Paracuellos
                   ` (3 preceding siblings ...)
  2020-03-11 18:58 ` [PATCH v2 4/5] staging: mt7621-pci: bindings: update doc accordly to last changes Sergio Paracuellos
@ 2020-03-11 18:58 ` Sergio Paracuellos
  2020-03-12  4:56 ` [PATCH v2 0/5] staging: mt7621-pci: re-do reset boot process Greg Ungerer
  5 siblings, 0 replies; 8+ messages in thread
From: Sergio Paracuellos @ 2020-03-11 18:58 UTC (permalink / raw)
  To: gregkh; +Cc: ryder.lee, driverdev-devel, weijie.gao, gerg, neil

R3G's LEDs fail to initialize because one of them uses GPIO8
Hence, release the GPIO resources after PCIe initialization.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-pci/pci-mt7621.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index 0880a21f2620..8399e4629e38 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -482,6 +482,14 @@ static void mt7621_pcie_reset_ep_deassert(struct mt7621_pcie *pcie)
 	mdelay(PERST_DELAY_US);
 }
 
+static void mt7621_pcie_release_gpios(struct mt7621_pcie *pcie)
+{
+	struct mt7621_pcie_port *port;
+
+	list_for_each_entry(port, &pcie->ports, list)
+		gpiod_put(port->gpio_rst);
+}
+
 static void mt7621_pcie_init_ports(struct mt7621_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
@@ -706,6 +714,8 @@ static int mt7621_pci_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	mt7621_pcie_release_gpios(pcie);
+
 	return 0;
 }
 
-- 
2.25.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/5] staging: mt7621-pci: re-do reset boot process
  2020-03-11 18:58 [PATCH v2 0/5] staging: mt7621-pci: re-do reset boot process Sergio Paracuellos
                   ` (4 preceding siblings ...)
  2020-03-11 18:58 ` [PATCH v2 5/5] staging: mt7621-pci: release gpios after pci initialization Sergio Paracuellos
@ 2020-03-12  4:56 ` Greg Ungerer
  2020-03-12  6:29   ` Sergio Paracuellos
  5 siblings, 1 reply; 8+ messages in thread
From: Greg Ungerer @ 2020-03-12  4:56 UTC (permalink / raw)
  To: Sergio Paracuellos, gregkh; +Cc: neil, driverdev-devel, weijie.gao, ryder.lee

Hi Sergio,

On 12/3/20 4:58 am, Sergio Paracuellos wrote:
> Some time ago Greg Ungerer reported some random hangs using
> the staging mt7621-pci driver:
> 
> See:
> * http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2019-June/134947.html
> 
> Try to fix that is the main motivation of this patch series.

Excellent!  I am glad to see an effort to get these problems resolved.
I still have to switch back to much earlier version of this PCI
driving code to get reliable working behavior.


> Also in openwrt there is a driver for mt7621-pci which seems was rewritten
> from scratch (for kernel 4.14) by Ryder Lee and Weijie Gao from mediatek.
> There the approach for reset assert-deassert process is to set as 'gpio'
> the function for all the 'pcie' group for the pinctrl driver and use those
> gpio's as a reset for the end points. The driver I am talking about is still
> using legacy pci and legacy gpio kernel interfaces. IMHO, the correct thing
> to do is make this staging driver properly clean and functional and put it
> in its correct place in the mainline.
> 
> See:
> * https://gist.github.com/dengqf6/7a9e9b4032d99f1a91dd9256c8a65c36
> 
> Because of all of this this patch series tries to avoid random hangs of boot
> trying to use the 'reset-gpios' approach.
> 
> Changes are being tested by openwrt people and seems to work.
> 
> Hope this helps.

What kernel did you generate these patches against?
They didn't apply completely cleanly for me against 5.5 or 5.6.0-rc5.
Minor reject and some fuzzing, easy enough to fix for testing.

Running 5.6.0-rc5 I get the following failure on my hardware during boot:

...
rt2880-pinmux pinctrl: pcie is already enabled
mt7621-pci 1e140000.pcie: Error applying setting, reverse things back
mt7621-pci 1e140000.pcie: Unable to request GPIO reset in slot 1
mt7621-pci 1e140000.pcie: Parsing DT failed
mt7621-pci: probe of 1e140000.pcie failed with error -16


FWIW: running the original 5.6.0-rc5 code gives a few different
PCI startup failures - but PCI devices never work properly. I can
send the error trace output if useful, but it is similar to what
I have posted in the past.

Regards
Greg


> Changes in v2:
>      * restore configuration for pers mode to GPIO.
>      * Avoid to read FTS_NUM register in reset state.
> 
> Best regards,
>      Sergio Paracuellos
> 
> 
> Sergio Paracuellos (5):
>    staging: mt7621-pci: use gpios for properly reset
>    staging: mt7621-pci: change value for 'PERST_DELAY_US'
>    staging: mt7621-dts: make use of 'reset-gpios' property for pci
>    staging: mt7621-pci: bindings: update doc accordly to last changes
>    staging: mt7621-pci: release gpios after pci initialization
> 
>   drivers/staging/mt7621-dts/mt7621.dtsi        | 11 ++-
>   .../mt7621-pci/mediatek,mt7621-pci.txt        |  7 +-
>   drivers/staging/mt7621-pci/pci-mt7621.c       | 94 ++++++++++++-------
>   3 files changed, 72 insertions(+), 40 deletions(-)
> 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/5] staging: mt7621-pci: re-do reset boot process
  2020-03-12  4:56 ` [PATCH v2 0/5] staging: mt7621-pci: re-do reset boot process Greg Ungerer
@ 2020-03-12  6:29   ` Sergio Paracuellos
  0 siblings, 0 replies; 8+ messages in thread
From: Sergio Paracuellos @ 2020-03-12  6:29 UTC (permalink / raw)
  To: Greg Ungerer; +Cc: ryder.lee, Greg KH, driverdev-devel, weijie.gao, NeilBrown

Hi Greg,

On Thu, Mar 12, 2020 at 5:56 AM Greg Ungerer <gerg@kernel.org> wrote:
>
> Hi Sergio,
>
> On 12/3/20 4:58 am, Sergio Paracuellos wrote:
> > Some time ago Greg Ungerer reported some random hangs using
> > the staging mt7621-pci driver:
> >
> > See:
> > * http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2019-June/134947.html
> >
> > Try to fix that is the main motivation of this patch series.
>
> Excellent!  I am glad to see an effort to get these problems resolved.
> I still have to switch back to much earlier version of this PCI
> driving code to get reliable working behavior.
>
>
> > Also in openwrt there is a driver for mt7621-pci which seems was rewritten
> > from scratch (for kernel 4.14) by Ryder Lee and Weijie Gao from mediatek.
> > There the approach for reset assert-deassert process is to set as 'gpio'
> > the function for all the 'pcie' group for the pinctrl driver and use those
> > gpio's as a reset for the end points. The driver I am talking about is still
> > using legacy pci and legacy gpio kernel interfaces. IMHO, the correct thing
> > to do is make this staging driver properly clean and functional and put it
> > in its correct place in the mainline.
> >
> > See:
> > * https://gist.github.com/dengqf6/7a9e9b4032d99f1a91dd9256c8a65c36
> >
> > Because of all of this this patch series tries to avoid random hangs of boot
> > trying to use the 'reset-gpios' approach.
> >
> > Changes are being tested by openwrt people and seems to work.
> >
> > Hope this helps.
>
> What kernel did you generate these patches against?
> They didn't apply completely cleanly for me against 5.5 or 5.6.0-rc5.
> Minor reject and some fuzzing, easy enough to fix for testing.
>

This is rebased on the top of staging-testing branch of git staging tree.

> Running 5.6.0-rc5 I get the following failure on my hardware during boot:
>
> ...
> rt2880-pinmux pinctrl: pcie is already enabled
> mt7621-pci 1e140000.pcie: Error applying setting, reverse things back
> mt7621-pci 1e140000.pcie: Unable to request GPIO reset in slot 1
> mt7621-pci 1e140000.pcie: Parsing DT failed
> mt7621-pci: probe of 1e140000.pcie failed with error -16
>

Ok, so it seems depending of boards gpio's can or not can be
requested, so maybe we have to do this optional
an don't fail on parsing the device tree.

>
> FWIW: running the original 5.6.0-rc5 code gives a few different
> PCI startup failures - but PCI devices never work properly. I can
> send the error trace output if useful, but it is similar to what
> I have posted in the past.

Mmmm. This patches were tested yesterday and I was told they were
working. I will change this patches to
avoid gpio's to be mandatory and send v3, thanks.

See:
https://github.com/openwrt/openwrt/pull/2798

>
> Regards
> Greg

Best regards,
    Sergio Paracuellos
>
>
> > Changes in v2:
> >      * restore configuration for pers mode to GPIO.
> >      * Avoid to read FTS_NUM register in reset state.
> >
> > Best regards,
> >      Sergio Paracuellos
> >
> >
> > Sergio Paracuellos (5):
> >    staging: mt7621-pci: use gpios for properly reset
> >    staging: mt7621-pci: change value for 'PERST_DELAY_US'
> >    staging: mt7621-dts: make use of 'reset-gpios' property for pci
> >    staging: mt7621-pci: bindings: update doc accordly to last changes
> >    staging: mt7621-pci: release gpios after pci initialization
> >
> >   drivers/staging/mt7621-dts/mt7621.dtsi        | 11 ++-
> >   .../mt7621-pci/mediatek,mt7621-pci.txt        |  7 +-
> >   drivers/staging/mt7621-pci/pci-mt7621.c       | 94 ++++++++++++-------
> >   3 files changed, 72 insertions(+), 40 deletions(-)
> >
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-03-12  6:29 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-11 18:58 [PATCH v2 0/5] staging: mt7621-pci: re-do reset boot process Sergio Paracuellos
2020-03-11 18:58 ` [PATCH v2 1/5] staging: mt7621-pci: use gpios for properly reset Sergio Paracuellos
2020-03-11 18:58 ` [PATCH v2 2/5] staging: mt7621-pci: change value for 'PERST_DELAY_US' Sergio Paracuellos
2020-03-11 18:58 ` [PATCH v2 3/5] staging: mt7621-dts: make use of 'reset-gpios' property for pci Sergio Paracuellos
2020-03-11 18:58 ` [PATCH v2 4/5] staging: mt7621-pci: bindings: update doc accordly to last changes Sergio Paracuellos
2020-03-11 18:58 ` [PATCH v2 5/5] staging: mt7621-pci: release gpios after pci initialization Sergio Paracuellos
2020-03-12  4:56 ` [PATCH v2 0/5] staging: mt7621-pci: re-do reset boot process Greg Ungerer
2020-03-12  6:29   ` Sergio Paracuellos

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.