[PATCH v4 0/6] staging: mt7621-pci: re-do reset boot process

[PATCH v4 0/6] staging: mt7621-pci: re-do reset boot process

[Sergio Paracuellos]

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 v4:
* Make use of 'devm_gpiod_get_index_optional' instead of 'devm_gpiod_get_index'.
* Use 'dev_err' instead of 'dev_notice' and return ERR_PTR if 
'devm_gpiod_get_index_optional' fails.
* Rename pers dealy macro to PERST_DELAY_MS.
* Add new patch 6 which removes function 'mt7621_reset_port' not needed anymore.

Changes in v3:
* Avoid to fail if gpio descriptor fails on get.
* re-do PATCH 1 commit message.
* Take into account gpio low polarity on request and assert and deassert.
* Review error path of driver to properly release gpio's resources.

Changes in v2:
* restore configuration for pers mode to GPIO.
* Avoid to read FTS_NUM register in reset state.
* Release gpio's patch added

Best regards,
    Sergio Paracuellos


Sergio Paracuellos (6):
  staging: mt7621-pci: use gpios for properly reset
  staging: mt7621-pci: change value for 'PERST_DELAY_MS'
  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
  staging: mt7621-pci: delete no more needed 'mt7621_reset_port'

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

-- 
2.25.1

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

[PATCH v4 1/6] staging: mt7621-pci: use gpios for properly reset

[Sergio Paracuellos]

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.
According to the mediatek aplication note v0.1 there are
three gpios used for pcie ports reset control: gpio#19,
gpio#8 and gpio#7 for slots 0, 1 and 2 respectively.
This schema can be used separately for mt7621A but in some
boards due to pin share issue, if the PCM and I2S function
are enable at the same time, there are no enough GPIO to
control per-port PCIe reset. In those cases gpio#19 is enought
for reset the three ports together. Because of this we just
try to get the three gpios but if some of them fail we are not
failing in boot process, just prints a kernel notice and take
after into account if the descriptor is or not valid in order
to use it. All of them are set as GPIO output low configuration.
The gpio descriptor's API takes device tree property into account
and invert value if the pin is configured as active low.
So we also have to properly request pins from device tree
and set values correct in assert and deassert functions.
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 | 84 +++++++++++++++----------
 1 file changed, 51 insertions(+), 33 deletions(-)

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index 770dabd3a70d..9b4fe8d31101 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,16 @@ 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);
+	if (port->gpio_rst)
+		gpiod_set_value(port->gpio_rst, 1);
 }
 
-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);
+	if (port->gpio_rst)
+		gpiod_set_value(port->gpio_rst, 0);
 }
 
 static inline bool mt7621_pcie_port_is_linkup(struct mt7621_pcie_port *port)
@@ -367,6 +365,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_optional(dev, "reset", slot,
+						       GPIOD_OUT_LOW);
+	if (IS_ERR(port->gpio_rst)) {
+		dev_err(dev, "Failed to get GPIO for PCIe%d\n", slot);
+		return PTR_ERR(port->gpio_rst);
+	}
+
 	port->slot = slot;
 	port->pcie = pcie;
 
@@ -383,12 +388,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 +398,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 +451,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 +502,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 +519,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

[PATCH v4 2/6] staging: mt7621-pci: change value for 'PERST_DELAY_MS'

[Sergio Paracuellos]

Value of 'PERST_DELAY_MS' is too high and it is ok just
to set up to 100 ms. Update also define name from
'PERST_DELAY_US' into 'PERST_DELAY_MS'

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

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index 9b4fe8d31101..3d85ce788f9f 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_MS			100
 
 /**
  * struct mt7621_pcie_port - PCIe port information
@@ -463,7 +463,7 @@ static void mt7621_pcie_reset_assert(struct mt7621_pcie *pcie)
 		mt7621_rst_gpio_pcie_assert(port);
 	}
 
-	mdelay(PERST_DELAY_US);
+	mdelay(PERST_DELAY_MS);
 }
 
 static void mt7621_pcie_reset_rc_deassert(struct mt7621_pcie *pcie)
@@ -481,7 +481,7 @@ static void mt7621_pcie_reset_ep_deassert(struct mt7621_pcie *pcie)
 	list_for_each_entry(port, &pcie->ports, list)
 		mt7621_rst_gpio_pcie_deassert(port);
 
-	mdelay(PERST_DELAY_US);
+	mdelay(PERST_DELAY_MS);
 }
 
 static void mt7621_pcie_init_ports(struct mt7621_pcie *pcie)
-- 
2.25.1

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

[PATCH v4 3/6] staging: mt7621-dts: make use of 'reset-gpios' property for pci

[Sergio Paracuellos]

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

[PATCH v4 4/6] staging: mt7621-pci: bindings: update doc accordly to last changes

[Sergio Paracuellos]

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

[PATCH v4 5/6] staging: mt7621-pci: release gpios after pci initialization

[Sergio Paracuellos]

R3G's LEDs fail to initialize because one of them uses GPIO8
Hence, release the GPIO resources after PCIe initialization
and properly release also in driver error path.

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

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index 3d85ce788f9f..0d9dd14f6bec 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -484,6 +484,15 @@ static void mt7621_pcie_reset_ep_deassert(struct mt7621_pcie *pcie)
 	mdelay(PERST_DELAY_MS);
 }
 
+static void mt7621_pcie_release_gpios(struct mt7621_pcie *pcie)
+{
+	struct mt7621_pcie_port *port;
+
+	list_for_each_entry(port, &pcie->ports, list)
+		if (port->gpio_rst)
+			gpiod_put(port->gpio_rst);
+}
+
 static void mt7621_pcie_init_ports(struct mt7621_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
@@ -683,7 +692,8 @@ static int mt7621_pci_probe(struct platform_device *pdev)
 	err = mt7621_pcie_init_virtual_bridges(pcie);
 	if (err) {
 		dev_err(dev, "Nothing is connected in virtual bridges. Exiting...");
-		return 0;
+		err = 0;
+		goto out_release_gpios;
 	}
 
 	mt7621_pcie_enable_ports(pcie);
@@ -691,7 +701,7 @@ static int mt7621_pci_probe(struct platform_device *pdev)
 	err = mt7621_pci_parse_request_of_pci_ranges(pcie);
 	if (err) {
 		dev_err(dev, "Error requesting pci resources from ranges");
-		return err;
+		goto out_release_gpios;
 	}
 
 	setup_cm_memory_region(pcie);
@@ -699,16 +709,19 @@ static int mt7621_pci_probe(struct platform_device *pdev)
 	err = mt7621_pcie_request_resources(pcie, &res);
 	if (err) {
 		dev_err(dev, "Error requesting resources\n");
-		return err;
+		goto out_release_gpios;
 	}
 
 	err = mt7621_pcie_register_host(bridge, &res);
 	if (err) {
 		dev_err(dev, "Error registering host\n");
-		return err;
+		goto out_release_gpios;
 	}
 
-	return 0;
+out_release_gpios:
+	mt7621_pcie_release_gpios(pcie);
+
+	return err;
 }
 
 static const struct of_device_id mt7621_pci_ids[] = {
-- 
2.25.1

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

[PATCH v4 6/6] staging: mt7621-pci: delete no more needed 'mt7621_reset_port'

[Sergio Paracuellos]

After review all the resets at the beggining the function
'mt7621_reset_port' is not needed anymore. Hence delete it
and its uses.

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

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index 0d9dd14f6bec..973be9aa7bb2 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -255,13 +255,6 @@ static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
 		reset_control_assert(port->pcie_rst);
 }
 
-static void mt7621_reset_port(struct mt7621_pcie_port *port)
-{
-	mt7621_control_assert(port);
-	msleep(100);
-	mt7621_control_deassert(port);
-}
-
 static void setup_cm_memory_region(struct mt7621_pcie *pcie)
 {
 	struct resource *mem_resource = &pcie->mem;
@@ -427,12 +420,6 @@ static int mt7621_pcie_init_port(struct mt7621_pcie_port *port)
 	u32 slot = port->slot;
 	int err;
 
-	/*
-	 * Any MT7621 Ralink pcie controller that doesn't have 0x0101 at
-	 * the end of the chip_id has inverted PCI resets.
-	 */
-	mt7621_reset_port(port);
-
 	err = phy_init(port->phy);
 	if (err) {
 		dev_err(dev, "failed to initialize port%d phy\n", slot);
-- 
2.25.1

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

Re: [PATCH v4 0/6] staging: mt7621-pci: re-do reset boot process

[Sergio Paracuellos]

Hi,

On Fri, Mar 13, 2020 at 9:09 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> 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.
>
> 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 v4:
> * Make use of 'devm_gpiod_get_index_optional' instead of 'devm_gpiod_get_index'.
> * Use 'dev_err' instead of 'dev_notice' and return ERR_PTR if
> 'devm_gpiod_get_index_optional' fails.
> * Rename pers dealy macro to PERST_DELAY_MS.
> * Add new patch 6 which removes function 'mt7621_reset_port' not needed anymore.

It seems this series work but due to an unknow bug set / clear gpio
registers are not properly working.
So maybe this patch is also necessary:
https://github.com/openwrt/openwrt/pull/2798/commits/823d41e28b4e15734560508b29df726b16c51dab

Should this patch be also properly added to the tree? Added Linus
Walleij and René van Dorst in order to get feedback about this issue.

I also add all the thread to get openwrt working with 5.4 in where
this patches and the gpio one have been added (just in case is
interesting for anyone):
https://github.com/openwrt/openwrt/pull/2798

Best regards,
    Sergio Paracuellos
>
> Changes in v3:
> * Avoid to fail if gpio descriptor fails on get.
> * re-do PATCH 1 commit message.
> * Take into account gpio low polarity on request and assert and deassert.
> * Review error path of driver to properly release gpio's resources.
>
> Changes in v2:
> * restore configuration for pers mode to GPIO.
> * Avoid to read FTS_NUM register in reset state.
> * Release gpio's patch added
>
> Best regards,
>     Sergio Paracuellos
>
>
> Sergio Paracuellos (6):
>   staging: mt7621-pci: use gpios for properly reset
>   staging: mt7621-pci: change value for 'PERST_DELAY_MS'
>   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
>   staging: mt7621-pci: delete no more needed 'mt7621_reset_port'
>
>  drivers/staging/mt7621-dts/mt7621.dtsi        |  11 +-
>  .../mt7621-pci/mediatek,mt7621-pci.txt        |   7 +-
>  drivers/staging/mt7621-pci/pci-mt7621.c       | 122 ++++++++++--------
>  3 files changed, 82 insertions(+), 58 deletions(-)
>
> --
> 2.25.1
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 0/6] staging: mt7621-pci: re-do reset boot process

[Sergio Paracuellos]

Hi,

On Sat, Mar 14, 2020 at 11:42 AM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
[snip]

> >
> > Changes in v4:
> > * Make use of 'devm_gpiod_get_index_optional' instead of 'devm_gpiod_get_index'.
> > * Use 'dev_err' instead of 'dev_notice' and return ERR_PTR if
> > 'devm_gpiod_get_index_optional' fails.
> > * Rename pers dealy macro to PERST_DELAY_MS.
> > * Add new patch 6 which removes function 'mt7621_reset_port' not needed anymore.
>
> It seems this series work but due to an unknow bug set / clear gpio
> registers are not properly working.
> So maybe this patch is also necessary:
> https://github.com/openwrt/openwrt/pull/2798/commits/823d41e28b4e15734560508b29df726b16c51dab

An update on this. It seems gpio-mmio driver upstream is broken for
this arch because bgpio_dir_out sets pin value before specifying pin
mode and set/clear register only works when pin is at output mode.

The following patches have been sent to linux-gpio mail list by Chuanhong Guo:

https://marc.info/?l=linux-gpio&m=158427446906895&w=2

This issue does not affect this pcie driver.

Best regards,
     Sergio Paracuellos
>
> Should this patch be also properly added to the tree? Added Linus
> Walleij and René van Dorst in order to get feedback about this issue.
>
> I also add all the thread to get openwrt working with 5.4 in where
> this patches and the gpio one have been added (just in case is
> interesting for anyone):
> https://github.com/openwrt/openwrt/pull/2798
>
> Best regards,
>     Sergio Paracuellos
> >
> > Changes in v3:
> > * Avoid to fail if gpio descriptor fails on get.
> > * re-do PATCH 1 commit message.
> > * Take into account gpio low polarity on request and assert and deassert.
> > * Review error path of driver to properly release gpio's resources.
> >
> > Changes in v2:
> > * restore configuration for pers mode to GPIO.
> > * Avoid to read FTS_NUM register in reset state.
> > * Release gpio's patch added
> >
> > Best regards,
> >     Sergio Paracuellos
> >
> >
> > Sergio Paracuellos (6):
> >   staging: mt7621-pci: use gpios for properly reset
> >   staging: mt7621-pci: change value for 'PERST_DELAY_MS'
> >   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
> >   staging: mt7621-pci: delete no more needed 'mt7621_reset_port'
> >
> >  drivers/staging/mt7621-dts/mt7621.dtsi        |  11 +-
> >  .../mt7621-pci/mediatek,mt7621-pci.txt        |   7 +-
> >  drivers/staging/mt7621-pci/pci-mt7621.c       | 122 ++++++++++--------
> >  3 files changed, 82 insertions(+), 58 deletions(-)
> >
> > --
> > 2.25.1
> >
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 0/6] staging: mt7621-pci: re-do reset boot process

[Greg Ungerer]

Hi Sergio,

On 14/3/20 6:09 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.
> 
> 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 v4:
> * Make use of 'devm_gpiod_get_index_optional' instead of 'devm_gpiod_get_index'.
> * Use 'dev_err' instead of 'dev_notice' and return ERR_PTR if
> 'devm_gpiod_get_index_optional' fails.
> * Rename pers dealy macro to PERST_DELAY_MS.
> * Add new patch 6 which removes function 'mt7621_reset_port' not needed anymore.

Testing this v4 series always fails during boot with:

...
NET: Registered protocol family 17
NET: Registered protocol family 15
8021q: 802.1Q VLAN Support v1.8
Loading compiled-in X.509 certificates
AppArmor: AppArmor sha1 policy hashing enabled

rt2880-pinmux pinctrl: pcie is already enabled
mt7621-pci 1e140000.pcie: Error applying setting, reverse things back
mt7621-pci 1e140000.pcie: Failed to get GPIO for PCIe1
mt7621-pci 1e140000.pcie: Parsing DT failed
mt7621-pci: probe of 1e140000.pcie failed with error -16

UBI error: cannot open mtd 3, error -19
hctosys: unable to open rtc device (rtc0)
cfg80211: Loading compiled-in X.509 certificates for regulatory database
cfg80211: Loaded X.509 cert 'sforshee: 00b28ddf47aef9cea7'
...

It never hangs, always boots up all the way. But always the same failure
with PCIe.

Regards
Greg



> Changes in v3:
> * Avoid to fail if gpio descriptor fails on get.
> * re-do PATCH 1 commit message.
> * Take into account gpio low polarity on request and assert and deassert.
> * Review error path of driver to properly release gpio's resources.
> 
> Changes in v2:
> * restore configuration for pers mode to GPIO.
> * Avoid to read FTS_NUM register in reset state.
> * Release gpio's patch added
> 
> Best regards,
>      Sergio Paracuellos
> 
> 
> Sergio Paracuellos (6):
>    staging: mt7621-pci: use gpios for properly reset
>    staging: mt7621-pci: change value for 'PERST_DELAY_MS'
>    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
>    staging: mt7621-pci: delete no more needed 'mt7621_reset_port'
> 
>   drivers/staging/mt7621-dts/mt7621.dtsi        |  11 +-
>   .../mt7621-pci/mediatek,mt7621-pci.txt        |   7 +-
>   drivers/staging/mt7621-pci/pci-mt7621.c       | 122 ++++++++++--------
>   3 files changed, 82 insertions(+), 58 deletions(-)
> 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 0/6] staging: mt7621-pci: re-do reset boot process

[Sergio Paracuellos]

Hi Greg,

On Thu, Mar 19, 2020 at 1:42 PM Greg Ungerer <gerg@kernel.org> wrote:
>
> Hi Sergio,
>
> On 14/3/20 6:09 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.
> >
> > 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 v4:
> > * Make use of 'devm_gpiod_get_index_optional' instead of 'devm_gpiod_get_index'.
> > * Use 'dev_err' instead of 'dev_notice' and return ERR_PTR if
> > 'devm_gpiod_get_index_optional' fails.
> > * Rename pers dealy macro to PERST_DELAY_MS.
> > * Add new patch 6 which removes function 'mt7621_reset_port' not needed anymore.
>
> Testing this v4 series always fails during boot with:
>
> ...
> NET: Registered protocol family 17
> NET: Registered protocol family 15
> 8021q: 802.1Q VLAN Support v1.8
> Loading compiled-in X.509 certificates
> AppArmor: AppArmor sha1 policy hashing enabled
>
> rt2880-pinmux pinctrl: pcie is already enabled
> mt7621-pci 1e140000.pcie: Error applying setting, reverse things back
> mt7621-pci 1e140000.pcie: Failed to get GPIO for PCIe1
> mt7621-pci 1e140000.pcie: Parsing DT failed
> mt7621-pci: probe of 1e140000.pcie failed with error -16

Looks like the gpio is valid but has been assigned to anything else.
It looks like a device-tree issue for me.
Does your hardware follows the indications of the mediatek application note?

https://github.com/openwrt/openwrt/files/4317436/an-mt7621-pcie-application-note-v0.1.pdf

To be able to test this you can just change the device tree and set
reset gpios to only perst-reset pin

reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>;

or avoid the "return PTR_ERR(port->gpio_rst);" after the call to
'devm_gpiod_get_index_optional'.

Or just make an exception if the pin is busy, which seems to be the
problem here:

>
> UBI error: cannot open mtd 3, error -19
> hctosys: unable to open rtc device (rtc0)
> cfg80211: Loading compiled-in X.509 certificates for regulatory database
> cfg80211: Loaded X.509 cert 'sforshee: 00b28ddf47aef9cea7'
> ...
>
> It never hangs, always boots up all the way. But always the same failure
> with PCIe.

This series has been applied to the staging tree and are properly
running for me in gnubee pc1.

You should test using all confirmed changes in staging-next branch and
this patch which fix a wrong register usage issue:

http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2020-March/142472.html


>
> Regards
> Greg
>
>
>
> > Changes in v3:
> > * Avoid to fail if gpio descriptor fails on get.
> > * re-do PATCH 1 commit message.
> > * Take into account gpio low polarity on request and assert and deassert.
> > * Review error path of driver to properly release gpio's resources.
> >
> > Changes in v2:
> > * restore configuration for pers mode to GPIO.
> > * Avoid to read FTS_NUM register in reset state.
> > * Release gpio's patch added
> >
> > Best regards,
> >      Sergio Paracuellos
> >
> >
> > Sergio Paracuellos (6):
> >    staging: mt7621-pci: use gpios for properly reset
> >    staging: mt7621-pci: change value for 'PERST_DELAY_MS'
> >    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
> >    staging: mt7621-pci: delete no more needed 'mt7621_reset_port'
> >
> >   drivers/staging/mt7621-dts/mt7621.dtsi        |  11 +-
> >   .../mt7621-pci/mediatek,mt7621-pci.txt        |   7 +-
> >   drivers/staging/mt7621-pci/pci-mt7621.c       | 122 ++++++++++--------
> >   3 files changed, 82 insertions(+), 58 deletions(-)
> >
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 0/6] staging: mt7621-pci: re-do reset boot process

[Sergio Paracuellos]

Sorry the mail was sent while I was still writing :)

On Thu, Mar 19, 2020 at 2:43 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> Hi Greg,
>
> On Thu, Mar 19, 2020 at 1:42 PM Greg Ungerer <gerg@kernel.org> wrote:
> >
> > Hi Sergio,
> >
> > On 14/3/20 6:09 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.
> > >
> > > 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 v4:
> > > * Make use of 'devm_gpiod_get_index_optional' instead of 'devm_gpiod_get_index'.
> > > * Use 'dev_err' instead of 'dev_notice' and return ERR_PTR if
> > > 'devm_gpiod_get_index_optional' fails.
> > > * Rename pers dealy macro to PERST_DELAY_MS.
> > > * Add new patch 6 which removes function 'mt7621_reset_port' not needed anymore.
> >
> > Testing this v4 series always fails during boot with:
> >
> > ...
> > NET: Registered protocol family 17
> > NET: Registered protocol family 15
> > 8021q: 802.1Q VLAN Support v1.8
> > Loading compiled-in X.509 certificates
> > AppArmor: AppArmor sha1 policy hashing enabled
> >
> > rt2880-pinmux pinctrl: pcie is already enabled
> > mt7621-pci 1e140000.pcie: Error applying setting, reverse things back
> > mt7621-pci 1e140000.pcie: Failed to get GPIO for PCIe1
> > mt7621-pci 1e140000.pcie: Parsing DT failed
> > mt7621-pci: probe of 1e140000.pcie failed with error -16
>
> Looks like the gpio is valid but has been assigned to anything else.
> It looks like a device-tree issue for me.
> Does your hardware follows the indications of the mediatek application note?
>
> https://github.com/openwrt/openwrt/files/4317436/an-mt7621-pcie-application-note-v0.1.pdf
>
> To be able to test this you can just change the device tree and set
> reset gpios to only perst-reset pin
>
> reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>;
>
> or avoid the "return PTR_ERR(port->gpio_rst);" after the call to
> 'devm_gpiod_get_index_optional'.
>
> Or just make an exception if the pin is busy, which seems to be the
> problem here:

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c
b/drivers/staging/mt7621-pci/pci-mt7621.c
index 13b272597442..767b10fce18f 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -369,7 +369,8 @@ static int mt7621_pcie_parse_port(struct mt7621_pcie *pcie,
                                                       GPIOD_OUT_LOW);
        if (IS_ERR(port->gpio_rst)) {
                dev_err(dev, "Failed to get GPIO for PCIe%d\n", slot);
-               return PTR_ERR(port->gpio_rst);
+               if (PTR_ERR(port->gpio_rst) != -EBUSY)
+                       return PTR_ERR(port->gpio_rst);
        }

        port->slot = slot;

>
> >
> > UBI error: cannot open mtd 3, error -19
> > hctosys: unable to open rtc device (rtc0)
> > cfg80211: Loading compiled-in X.509 certificates for regulatory database
> > cfg80211: Loaded X.509 cert 'sforshee: 00b28ddf47aef9cea7'
> > ...
> >
> > It never hangs, always boots up all the way. But always the same failure
> > with PCIe.

If it hangs it should hang on the pci initilization process...

>
> This series has been applied to the staging tree and are properly
> running for me in gnubee pc1.
>
> You should test using all confirmed changes in staging-next branch and
> this patch which fix a wrong register usage issue:
>
> http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2020-March/142472.html
>
>
> >
> > Regards
> > Greg

Hope this helps.

Best regards,
    Sergio Paracuellos
> >
> >
> >
> > > Changes in v3:
> > > * Avoid to fail if gpio descriptor fails on get.
> > > * re-do PATCH 1 commit message.
> > > * Take into account gpio low polarity on request and assert and deassert.
> > > * Review error path of driver to properly release gpio's resources.
> > >
> > > Changes in v2:
> > > * restore configuration for pers mode to GPIO.
> > > * Avoid to read FTS_NUM register in reset state.
> > > * Release gpio's patch added
> > >
> > > Best regards,
> > >      Sergio Paracuellos
> > >
> > >
> > > Sergio Paracuellos (6):
> > >    staging: mt7621-pci: use gpios for properly reset
> > >    staging: mt7621-pci: change value for 'PERST_DELAY_MS'
> > >    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
> > >    staging: mt7621-pci: delete no more needed 'mt7621_reset_port'
> > >
> > >   drivers/staging/mt7621-dts/mt7621.dtsi        |  11 +-
> > >   .../mt7621-pci/mediatek,mt7621-pci.txt        |   7 +-
> > >   drivers/staging/mt7621-pci/pci-mt7621.c       | 122 ++++++++++--------
> > >   3 files changed, 82 insertions(+), 58 deletions(-)
> > >
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 0/6] staging: mt7621-pci: re-do reset boot process

[Greg Ungerer]

Hi Sergio,

On 19/3/20 11:43 pm, Sergio Paracuellos wrote:
> On Thu, Mar 19, 2020 at 1:42 PM Greg Ungerer <gerg@kernel.org> wrote:
>> On 14/3/20 6:09 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.
>>>
>>> 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 v4:
>>> * Make use of 'devm_gpiod_get_index_optional' instead of 'devm_gpiod_get_index'.
>>> * Use 'dev_err' instead of 'dev_notice' and return ERR_PTR if
>>> 'devm_gpiod_get_index_optional' fails.
>>> * Rename pers dealy macro to PERST_DELAY_MS.
>>> * Add new patch 6 which removes function 'mt7621_reset_port' not needed anymore.
>>
>> Testing this v4 series always fails during boot with:
>>
>> ...
>> NET: Registered protocol family 17
>> NET: Registered protocol family 15
>> 8021q: 802.1Q VLAN Support v1.8
>> Loading compiled-in X.509 certificates
>> AppArmor: AppArmor sha1 policy hashing enabled
>>
>> rt2880-pinmux pinctrl: pcie is already enabled
>> mt7621-pci 1e140000.pcie: Error applying setting, reverse things back
>> mt7621-pci 1e140000.pcie: Failed to get GPIO for PCIe1
>> mt7621-pci 1e140000.pcie: Parsing DT failed
>> mt7621-pci: probe of 1e140000.pcie failed with error -16
> 
> Looks like the gpio is valid but has been assigned to anything else.
> It looks like a device-tree issue for me.
> Does your hardware follows the indications of the mediatek application note?

Yes, as per table 2-2. GPIO pins 7 and 8 are used for other purposes
on my hardware - and my devicetree has those assigned for those
other purposes. They are not available for, or used for, PCI reset.

Regards
Greg



> https://github.com/openwrt/openwrt/files/4317436/an-mt7621-pcie-application-note-v0.1.pdf
> 
> To be able to test this you can just change the device tree and set
> reset gpios to only perst-reset pin
> 
> reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>;
> 
> or avoid the "return PTR_ERR(port->gpio_rst);" after the call to
> 'devm_gpiod_get_index_optional'.
> 
> Or just make an exception if the pin is busy, which seems to be the
> problem here:
> 
>>
>> UBI error: cannot open mtd 3, error -19
>> hctosys: unable to open rtc device (rtc0)
>> cfg80211: Loading compiled-in X.509 certificates for regulatory database
>> cfg80211: Loaded X.509 cert 'sforshee: 00b28ddf47aef9cea7'
>> ...
>>
>> It never hangs, always boots up all the way. But always the same failure
>> with PCIe.
> 
> This series has been applied to the staging tree and are properly
> running for me in gnubee pc1.
> 
> You should test using all confirmed changes in staging-next branch and
> this patch which fix a wrong register usage issue:
> 
> http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2020-March/142472.html
> 
> 
>>
>> Regards
>> Greg
>>
>>
>>
>>> Changes in v3:
>>> * Avoid to fail if gpio descriptor fails on get.
>>> * re-do PATCH 1 commit message.
>>> * Take into account gpio low polarity on request and assert and deassert.
>>> * Review error path of driver to properly release gpio's resources.
>>>
>>> Changes in v2:
>>> * restore configuration for pers mode to GPIO.
>>> * Avoid to read FTS_NUM register in reset state.
>>> * Release gpio's patch added
>>>
>>> Best regards,
>>>       Sergio Paracuellos
>>>
>>>
>>> Sergio Paracuellos (6):
>>>     staging: mt7621-pci: use gpios for properly reset
>>>     staging: mt7621-pci: change value for 'PERST_DELAY_MS'
>>>     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
>>>     staging: mt7621-pci: delete no more needed 'mt7621_reset_port'
>>>
>>>    drivers/staging/mt7621-dts/mt7621.dtsi        |  11 +-
>>>    .../mt7621-pci/mediatek,mt7621-pci.txt        |   7 +-
>>>    drivers/staging/mt7621-pci/pci-mt7621.c       | 122 ++++++++++--------
>>>    3 files changed, 82 insertions(+), 58 deletions(-)
>>>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 5/6] staging: mt7621-pci: release gpios after pci initialization

[Chuanhong Guo]

Hi!

On Sat, Mar 14, 2020 at 4:10 AM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> R3G's LEDs fail to initialize because one of them uses GPIO8

A gpio can't be used as pcie reset and led simultaneously.

> Hence, release the GPIO resources after PCIe initialization
> and properly release also in driver error path.

This pin conflict comes from incorrectly occupying pins that are not
used by pcie,
and should be fixed by not occupying those pins in the first place.
Releasing all
gpios isn't the proper way to go.

-- 
Regards,
Chuanhong Guo
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 5/6] staging: mt7621-pci: release gpios after pci initialization

[Sergio Paracuellos]

Hi Chuanhong,

On Fri, Mar 20, 2020 at 3:59 PM Chuanhong Guo <gch981213@gmail.com> wrote:
>
> Hi!
>
> On Sat, Mar 14, 2020 at 4:10 AM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> >
> > R3G's LEDs fail to initialize because one of them uses GPIO8
>
> A gpio can't be used as pcie reset and led simultaneously.
>

Obviously it can't. That was the reason for the release :)

> > Hence, release the GPIO resources after PCIe initialization
> > and properly release also in driver error path.
>
> This pin conflict comes from incorrectly occupying pins that are not
> used by pcie,
> and should be fixed by not occupying those pins in the first place.
> Releasing all
> gpios isn't the proper way to go.

So, you are saying we just have to get gpio for the pin 19 and forget
about the others?

>
> --
> Regards,
> Chuanhong Guo

Best regards,
    Sergio Paracuellos
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 5/6] staging: mt7621-pci: release gpios after pci initialization

[Chuanhong Guo]

Hi!

On Fri, Mar 20, 2020 at 11:29 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
> > This pin conflict comes from incorrectly occupying pins that are not
> > used by pcie,
> > and should be fixed by not occupying those pins in the first place.
> > Releasing all
> > gpios isn't the proper way to go.
>
> So, you are saying we just have to get gpio for the pin 19 and forget
> about the others?

Not really "forget about the others". We should use only gpio19
in mt7621.dtsi and others should be added to device dts if it's
actually needed. e.g. if pcie cards can't be detected on a specific
board without gpio7 and/or gpio8, override gpio-resets in dts of
that board.

-- 
Regards,
Chuanhong Guo
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 5/6] staging: mt7621-pci: release gpios after pci initialization

[Sergio Paracuellos]

Hi,

On Fri, Mar 20, 2020 at 4:39 PM Chuanhong Guo <gch981213@gmail.com> wrote:
>
> Hi!
>
> On Fri, Mar 20, 2020 at 11:29 PM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> > > This pin conflict comes from incorrectly occupying pins that are not
> > > used by pcie,
> > > and should be fixed by not occupying those pins in the first place.
> > > Releasing all
> > > gpios isn't the proper way to go.
> >
> > So, you are saying we just have to get gpio for the pin 19 and forget
> > about the others?
>
> Not really "forget about the others". We should use only gpio19
> in mt7621.dtsi and others should be added to device dts if it's
> actually needed. e.g. if pcie cards can't be detected on a specific
> board without gpio7 and/or gpio8, override gpio-resets in dts of
> that board.

If I am understanding correctly for example for my gnubee this should
be as follows:

diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi
b/drivers/staging/mt7621-dts/mt7621.dtsi
index 10fb497cf81a..9e5cf68731bb 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -538,9 +538,7 @@ pcie: pcie@1e140000 {
                phys = <&pcie0_phy 1>, <&pcie2_phy 0>;
                phy-names = "pcie-phy0", "pcie-phy2";

-               reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>,
-                               <&gpio 8 GPIO_ACTIVE_LOW>,
-                               <&gpio 7 GPIO_ACTIVE_LOW>;
+               reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>;

                pcie@0,0 {
                        reg = <0x0000 0 0 0 0>;
sergio@camaron:~/staging$ git diff drivers/staging/mt7621-dts/gbpc1.dts
diff --git a/drivers/staging/mt7621-dts/gbpc1.dts
b/drivers/staging/mt7621-dts/gbpc1.dts
index 1fb560ff059c..a7c0d3115d72 100644
--- a/drivers/staging/mt7621-dts/gbpc1.dts
+++ b/drivers/staging/mt7621-dts/gbpc1.dts
@@ -114,6 +114,10 @@ &cpuclock {
 &pcie {
        pinctrl-names = "default";
        pinctrl-0 = <&pcie_pins>;
+
+       reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>,
+                       <&gpio 8 GPIO_ACTIVE_LOW>,
+                       <&gpio 7 GPIO_ACTIVE_LOW>;
        status = "okay";
 };

Is this true? So changes will be only in the device tree but driver is
ok as it is.

>
> --
> Regards,
> Chuanhong Guo

Best regards,
    Sergio Paracuellos
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 5/6] staging: mt7621-pci: release gpios after pci initialization

[Sergio Paracuellos]

On Fri, Mar 20, 2020 at 5:32 PM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
>
> Hi,
>
> On Fri, Mar 20, 2020 at 4:39 PM Chuanhong Guo <gch981213@gmail.com> wrote:
> >
> > Hi!
> >
> > On Fri, Mar 20, 2020 at 11:29 PM Sergio Paracuellos
> > <sergio.paracuellos@gmail.com> wrote:
> > > > This pin conflict comes from incorrectly occupying pins that are not
> > > > used by pcie,
> > > > and should be fixed by not occupying those pins in the first place.
> > > > Releasing all
> > > > gpios isn't the proper way to go.
> > >
> > > So, you are saying we just have to get gpio for the pin 19 and forget
> > > about the others?
> >
> > Not really "forget about the others". We should use only gpio19
> > in mt7621.dtsi and others should be added to device dts if it's
> > actually needed. e.g. if pcie cards can't be detected on a specific
> > board without gpio7 and/or gpio8, override gpio-resets in dts of
> > that board.
>
> If I am understanding correctly for example for my gnubee this should
> be as follows:
>
> diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi
> b/drivers/staging/mt7621-dts/mt7621.dtsi
> index 10fb497cf81a..9e5cf68731bb 100644
> --- a/drivers/staging/mt7621-dts/mt7621.dtsi
> +++ b/drivers/staging/mt7621-dts/mt7621.dtsi
> @@ -538,9 +538,7 @@ pcie: pcie@1e140000 {
>                 phys = <&pcie0_phy 1>, <&pcie2_phy 0>;
>                 phy-names = "pcie-phy0", "pcie-phy2";
>
> -               reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>,
> -                               <&gpio 8 GPIO_ACTIVE_LOW>,
> -                               <&gpio 7 GPIO_ACTIVE_LOW>;
> +               reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>;
>
>                 pcie@0,0 {
>                         reg = <0x0000 0 0 0 0>;
> sergio@camaron:~/staging$ git diff drivers/staging/mt7621-dts/gbpc1.dts
> diff --git a/drivers/staging/mt7621-dts/gbpc1.dts
> b/drivers/staging/mt7621-dts/gbpc1.dts
> index 1fb560ff059c..a7c0d3115d72 100644
> --- a/drivers/staging/mt7621-dts/gbpc1.dts
> +++ b/drivers/staging/mt7621-dts/gbpc1.dts
> @@ -114,6 +114,10 @@ &cpuclock {
>  &pcie {
>         pinctrl-names = "default";
>         pinctrl-0 = <&pcie_pins>;
> +
> +       reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>,
> +                       <&gpio 8 GPIO_ACTIVE_LOW>,
> +                       <&gpio 7 GPIO_ACTIVE_LOW>;
>         status = "okay";
>  };
>
> Is this true? So changes will be only in the device tree but driver is
> ok as it is.

Well, I mean I should only remove the release part for gpios, right?
>
> >
> > --
> > Regards,
> > Chuanhong Guo
>
> Best regards,
>     Sergio Paracuellos
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 5/6] staging: mt7621-pci: release gpios after pci initialization

[Chuanhong Guo]

Hi!

On Sat, Mar 21, 2020 at 12:34 AM Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:
> > If I am understanding correctly for example for my gnubee this should
> > be as follows:
> >
> > diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi
> > b/drivers/staging/mt7621-dts/mt7621.dtsi
> > index 10fb497cf81a..9e5cf68731bb 100644
> > --- a/drivers/staging/mt7621-dts/mt7621.dtsi
> > +++ b/drivers/staging/mt7621-dts/mt7621.dtsi
> > @@ -538,9 +538,7 @@ pcie: pcie@1e140000 {
> >                 phys = <&pcie0_phy 1>, <&pcie2_phy 0>;
> >                 phy-names = "pcie-phy0", "pcie-phy2";
> >
> > -               reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>,
> > -                               <&gpio 8 GPIO_ACTIVE_LOW>,
> > -                               <&gpio 7 GPIO_ACTIVE_LOW>;
> > +               reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>;
> >
> >                 pcie@0,0 {
> >                         reg = <0x0000 0 0 0 0>;
> > sergio@camaron:~/staging$ git diff drivers/staging/mt7621-dts/gbpc1.dts
> > diff --git a/drivers/staging/mt7621-dts/gbpc1.dts
> > b/drivers/staging/mt7621-dts/gbpc1.dts
> > index 1fb560ff059c..a7c0d3115d72 100644
> > --- a/drivers/staging/mt7621-dts/gbpc1.dts
> > +++ b/drivers/staging/mt7621-dts/gbpc1.dts
> > @@ -114,6 +114,10 @@ &cpuclock {
> >  &pcie {
> >         pinctrl-names = "default";
> >         pinctrl-0 = <&pcie_pins>;
> > +
> > +       reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>,
> > +                       <&gpio 8 GPIO_ACTIVE_LOW>,
> > +                       <&gpio 7 GPIO_ACTIVE_LOW>;
> >         status = "okay";
> >  };
> >
> > Is this true? So changes will be only in the device tree but driver is
> > ok as it is.
>
> Well, I mean I should only remove the release part for gpios, right?

Correct :)

-- 
Regards,
Chuanhong Guo
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 5/6] staging: mt7621-pci: release gpios after pci initialization

[Sergio Paracuellos]

Hi,

On Sat, Mar 21, 2020 at 7:36 AM Chuanhong Guo <gch981213@gmail.com> wrote:
>
> Hi!
>
> On Sat, Mar 21, 2020 at 12:34 AM Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> > > If I am understanding correctly for example for my gnubee this should
> > > be as follows:
> > >
> > > diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi
> > > b/drivers/staging/mt7621-dts/mt7621.dtsi
> > > index 10fb497cf81a..9e5cf68731bb 100644
> > > --- a/drivers/staging/mt7621-dts/mt7621.dtsi
> > > +++ b/drivers/staging/mt7621-dts/mt7621.dtsi
> > > @@ -538,9 +538,7 @@ pcie: pcie@1e140000 {
> > >                 phys = <&pcie0_phy 1>, <&pcie2_phy 0>;
> > >                 phy-names = "pcie-phy0", "pcie-phy2";
> > >
> > > -               reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>,
> > > -                               <&gpio 8 GPIO_ACTIVE_LOW>,
> > > -                               <&gpio 7 GPIO_ACTIVE_LOW>;
> > > +               reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>;
> > >
> > >                 pcie@0,0 {
> > >                         reg = <0x0000 0 0 0 0>;
> > > sergio@camaron:~/staging$ git diff drivers/staging/mt7621-dts/gbpc1.dts
> > > diff --git a/drivers/staging/mt7621-dts/gbpc1.dts
> > > b/drivers/staging/mt7621-dts/gbpc1.dts
> > > index 1fb560ff059c..a7c0d3115d72 100644
> > > --- a/drivers/staging/mt7621-dts/gbpc1.dts
> > > +++ b/drivers/staging/mt7621-dts/gbpc1.dts
> > > @@ -114,6 +114,10 @@ &cpuclock {
> > >  &pcie {
> > >         pinctrl-names = "default";
> > >         pinctrl-0 = <&pcie_pins>;
> > > +
> > > +       reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>,
> > > +                       <&gpio 8 GPIO_ACTIVE_LOW>,
> > > +                       <&gpio 7 GPIO_ACTIVE_LOW>;
> > >         status = "okay";
> > >  };
> > >
> > > Is this true? So changes will be only in the device tree but driver is
> > > ok as it is.
> >
> > Well, I mean I should only remove the release part for gpios, right?
>
> Correct :)

Thanks for let me know the correct way of doing this. Sent:

http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2020-March/142567.html

>
> --
> Regards,
> Chuanhong Guo

Best regards,
    Sergio Paracuellos
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel