[PATCH v2 0/7] staging: mt7621-pci: some fixes after test previous series

[PATCH v2 0/7] staging: mt7621-pci: some fixes after test previous series

[Sergio Paracuellos]

Previous cleanup series was added to the staging tree without any
testing. After get testing feedback some issues appear and this patch
series should make the driver works properly again.

Previous series are here:
* http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/128200.html

Feedback after testing from Neil Brown is here:
* http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/129031.html

There is one issue with chip revision and reset lines where those
are inverted. I achieve this including some wrappers for checking
the version in driver code and use proper reset_control_* functions.
I checked the 'arch/mips/ralink/reset.c' and think a good way to add
a quirk there but I ended up handling those inside the driver.

Changes in v2:
    - PATCH 7: In commit message: 's/mt7621-pcie/mt7621-pci/g'

Hope this helps.

Best regards,
    Sergio Paracuellos

Sergio Paracuellos (7):
  staging: mt7621-pci: avoid mapping sysctls registers
  staging: mt7621-dts: remove sysctl registers from pcie bindings
  staging: mt7621-pci: dt-bindings: update bindings doc removing sysctls
    registers
  staging: mt7621-pci: fix reset lines for each pcie port
  staging: mt7621-pci: avoid using clk_* operations
  staging: mt7621-dts: remove clocks for pcie bindings
  staging: mt7621-pci: dt-bindings: update bindings doc removing clocks

 drivers/staging/mt7621-dts/mt7621.dtsi        |  5 +-
 .../mt7621-pci/mediatek,mt7621-pci.txt        |  9 +--
 drivers/staging/mt7621-pci/pci-mt7621.c       | 67 +++++++++----------
 3 files changed, 34 insertions(+), 47 deletions(-)

-- 
2.19.1

[PATCH v2 1/7] staging: mt7621-pci: avoid mapping sysctls registers

[Sergio Paracuellos]

The sysctl register are already claimed by palmbus, so
pci fails to claim it. The best way to access the sysc registers
is to use rt_sysc_[rwm]32().

Fixes: 89e9f6e6adfc: staging: mt7621-pci: remap and use sysctl from device tree
Reported-by: NeilBrown <neil@brown.name>
Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Tested-by: NeilBrown <neil@brown.name>
---
 drivers/staging/mt7621-pci/pci-mt7621.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index 14cec231946c..ba81b34dc1b7 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -185,7 +185,6 @@ struct mt7621_pcie_port {
 /**
  * struct mt7621_pcie - PCIe host information
  * @base: IO Mapped Register Base
- * @sysctl: system control mapped register base
  * @io: IO resource
  * @mem: non-prefetchable memory resource
  * @busn: bus range
@@ -195,7 +194,6 @@ struct mt7621_pcie_port {
  */
 struct mt7621_pcie {
 	void __iomem *base;
-	void __iomem *sysctl;
 	struct device *dev;
 	struct resource io;
 	struct resource mem;
@@ -407,8 +405,7 @@ static void set_phy_for_ssc(struct mt7621_pcie_port *port)
 
 static void mt7621_enable_phy(struct mt7621_pcie_port *port)
 {
-	struct mt7621_pcie *pcie = port->pcie;
-	u32 chip_rev_id = ioread32(pcie->sysctl + MT7621_CHIP_REV_ID);
+	u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
 
 	if ((chip_rev_id & 0xFFFF) == CHIP_REV_MT7621_E2)
 		bypass_pipe_rst(port);
@@ -548,16 +545,6 @@ static int mt7621_pcie_parse_dt(struct mt7621_pcie *pcie)
 	if (IS_ERR(pcie->base))
 		return PTR_ERR(pcie->base);
 
-	err = of_address_to_resource(node, 4, &regs);
-	if (err) {
-		dev_err(dev, "missing \"reg\" property\n");
-		return err;
-	}
-
-	pcie->sysctl = devm_ioremap_resource(dev, &regs);
-	if (IS_ERR(pcie->sysctl))
-		return PTR_ERR(pcie->sysctl);
-
 	for_each_available_child_of_node(node, child) {
 		int slot;
 
-- 
2.19.1

[PATCH v2 2/7] staging: mt7621-dts: remove sysctl registers from pcie bindings

[Sergio Paracuellos]

The sysctl register are already claimed by palmbus, so
pci fails to claim it. Remove registers accordly from DT
bindings.

Fixes: 624c5227ed0a: staging: mt7621-dts: add sysctl registers base address to pcie
Reported-by: NeilBrown <neil@brown.name>
Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Tested-by: NeilBrown <neil@brown.name>
---
 drivers/staging/mt7621-dts/mt7621.dtsi | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
index 6b4bc43d8eb6..ebf6667bbc54 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -397,8 +397,7 @@
 		reg = <0x1e140000 0x100     /* host-pci bridge registers */
 			0x1e142000 0x100    /* pcie port 0 RC control registers */
 			0x1e143000 0x100    /* pcie port 1 RC control registers */
-			0x1e144000 0x100    /* pcie port 2 RC control registers */
-			0x1e000000 0x100>;  /* sysctl */
+			0x1e144000 0x100>;  /* pcie port 2 RC control registers */
 		#address-cells = <3>;
 		#size-cells = <2>;
 
-- 
2.19.1

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

[PATCH v2 3/7] staging: mt7621-pci: dt-bindings: update bindings doc removing sysctls registers

[Sergio Paracuellos]

The sysctl register are already claimed by palmbus, so
pci fails to claim it. Device tree has been updated to not
use it at all with pcie. Update bindings documentation.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Tested-by: NeilBrown <neil@brown.name>
---
 drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt b/drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt
index 4f3b082606c5..5a6ee4103cd5 100644
--- a/drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt
+++ b/drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt
@@ -43,8 +43,7 @@ Example for MT7621:
         reg = <0x1e140000 0x100    /* host-pci bridge registers */
                0x1e142000 0x100    /* pcie port 0 RC control registers */
                0x1e143000 0x100    /* pcie port 1 RC control registers */
-               0x1e144000 0x100    /* pcie port 2 RC control registers */
-               0x1e000000 0x100>;  /* sysctl */
+               0x1e144000 0x100>;  /* pcie port 2 RC control registers */
 
 		#address-cells = <3>;
 		#size-cells = <2>;
-- 
2.19.1

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

[PATCH v2 4/7] staging: mt7621-pci: fix reset lines for each pcie port

[Sergio Paracuellos]

Depending of chip revision reset lines are inverted. It is also
necessary to read PCIE_FTS_NUM register before enabling the phy.
Hence update the code to achieve this.

Fixes: 745eeeac68d7 ("staging: mt7621-pci: factor out 'mt7621_pcie_enable_port' function")
Reported-by: NeilBrown <neil@brown.name>
Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Tested-by: NeilBrown <neil@brown.name>
---
 drivers/staging/mt7621-pci/pci-mt7621.c | 38 +++++++++++++++++++++----
 1 file changed, 32 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index ba81b34dc1b7..1b63706e129b 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -412,6 +412,33 @@ static void mt7621_enable_phy(struct mt7621_pcie_port *port)
 	set_phy_for_ssc(port);
 }
 
+static inline void mt7621_control_assert(struct mt7621_pcie_port *port)
+{
+	u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
+
+	if ((chip_rev_id & 0xFFFF) == CHIP_REV_MT7621_E2)
+		reset_control_assert(port->pcie_rst);
+	else
+		reset_control_deassert(port->pcie_rst);
+}
+
+static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
+{
+	u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
+
+	if ((chip_rev_id & 0xFFFF) == CHIP_REV_MT7621_E2)
+		reset_control_deassert(port->pcie_rst);
+	else
+		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;
@@ -578,12 +605,14 @@ static int mt7621_pcie_init_port(struct mt7621_pcie_port *port)
 		return err;
 	}
 
-	reset_control_assert(port->pcie_rst);
-	reset_control_deassert(port->pcie_rst);
+	mt7621_reset_port(port);
+
+	val = read_config(pcie, slot, PCIE_FTS_NUM);
+	dev_info(dev, "Port %d N_FTS = %x\n", (unsigned int)val, slot);
 
 	if ((pcie_port_read(port, RALINK_PCI_STATUS) & PCIE_PORT_LINKUP) == 0) {
 		dev_err(dev, "pcie%d no card, disable it (RST & CLK)\n", slot);
-		reset_control_assert(port->pcie_rst);
+		mt7621_control_assert(port);
 		rt_sysc_m32(PCIE_PORT_CLK_EN(slot), 0, RALINK_CLKCFG1);
 		port->enabled = false;
 	} else {
@@ -592,9 +621,6 @@ static int mt7621_pcie_init_port(struct mt7621_pcie_port *port)
 
 	mt7621_enable_phy(port);
 
-	val = read_config(pcie, slot, PCIE_FTS_NUM);
-	dev_info(dev, "Port %d N_FTS = %x\n", (unsigned int)val, slot);
-
 	return 0;
 }
 
-- 
2.19.1

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

[PATCH v2 5/7] staging: mt7621-pci: avoid using clk_* operations

[Sergio Paracuellos]

There is no clock driver for ralink mips and clk_enable
are no-ops for this architecture. This has been also tested
without using clocks and seems to work so avoid to use them
in this driver.

Fixes: ad9c87e129d1: "staging: mt7621-pci: parse and init
port data from device tree"
Reported-by: NeilBrown <neil@brown.name>

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

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index 1b63706e129b..c5e33fbdf225 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -16,7 +16,6 @@
  */
 
 #include <linux/bitops.h>
-#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/iopoll.h>
 #include <linux/module.h>
@@ -167,7 +166,6 @@
  * @pcie: pointer to PCIe host info
  * @phy_reg_offset: offset to related phy registers
  * @pcie_rst: pointer to port reset control
- * @pcie_clk: PCIe clock
  * @slot: port slot
  * @enabled: indicates if port is enabled
  */
@@ -177,7 +175,6 @@ struct mt7621_pcie_port {
 	struct mt7621_pcie *pcie;
 	u32 phy_reg_offset;
 	struct reset_control *pcie_rst;
-	struct clk *pcie_clk;
 	u32 slot;
 	bool enabled;
 };
@@ -531,12 +528,6 @@ static int mt7621_pcie_parse_port(struct mt7621_pcie *pcie,
 		return PTR_ERR(port->base);
 
 	snprintf(name, sizeof(name), "pcie%d", slot);
-	port->pcie_clk = devm_clk_get(dev, name);
-	if (IS_ERR(port->pcie_clk)) {
-		dev_err(dev, "failed to get pcie%d clock\n", slot);
-		return PTR_ERR(port->pcie_clk);
-	}
-
 	port->pcie_rst = devm_reset_control_get_exclusive(dev, name);
 	if (PTR_ERR(port->pcie_rst) == -EPROBE_DEFER) {
 		dev_err(dev, "failed to get pcie%d reset control\n", slot);
@@ -597,13 +588,6 @@ static int mt7621_pcie_init_port(struct mt7621_pcie_port *port)
 	struct device *dev = pcie->dev;
 	u32 slot = port->slot;
 	u32 val = 0;
-	int err;
-
-	err = clk_prepare_enable(port->pcie_clk);
-	if (err) {
-		dev_err(dev, "failed to enable pcie%d clock\n", slot);
-		return err;
-	}
 
 	mt7621_reset_port(port);
 
-- 
2.19.1

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

[PATCH v2 6/7] staging: mt7621-dts: remove clocks for pcie bindings

[Sergio Paracuellos]

There are no real need to use clocks for pcie ports for
this driver. There is no clock driver for ralink and driver
can work without them.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Tested-by: NeilBrown <neil@brown.name>
---
 drivers/staging/mt7621-dts/mt7621.dtsi | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
index ebf6667bbc54..e30e3c837d3b 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -422,8 +422,6 @@
 
 		resets = <&rstctrl 24 &rstctrl 25 &rstctrl 26>;
 		reset-names = "pcie0", "pcie1", "pcie2";
-		clocks = <&clkctrl 24 &clkctrl 25 &clkctrl 26>;
-		clock-names = "pcie0", "pcie1", "pcie2";
 
 		pcie@0,0 {
 			reg = <0x0000 0 0 0 0>;
-- 
2.19.1

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

[PATCH v2 7/7] staging: mt7621-pci: dt-bindings: update bindings doc removing clocks

[Sergio Paracuellos]

Clocks are not necessary for the driver to work. Remove them also for
DT binding documentation.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Tested-by: NeilBrown <neil@brown.name>
---
 drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt b/drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt
index 5a6ee4103cd5..559246646659 100644
--- a/drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt
+++ b/drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt
@@ -19,10 +19,6 @@ Required properties:
   See ../reset/reset.txt for details.
 - reset-names: Must be "pcie0", "pcie1", "pcieN"... based on the number of
   root ports.
-- clocks: Must contain an entry for each entry in clock-names.
-  See ../clocks/clock-bindings.txt for details.
-- clock-names: Must be "pcie0", "pcie1", "pcieN"... based on the number of
-  root ports.
 
 In addition, the device tree node must have sub-nodes describing each PCIe port
 interface, having the following mandatory properties:
@@ -69,8 +65,6 @@ Example for MT7621:
 
 		resets = <&rstctrl 24 &rstctrl 25 &rstctrl 26>;
 		reset-names = "pcie0", "pcie1", "pcie2";
-		clocks = <&clkctrl 24 &clkctrl 25 &clkctrl 26>;
-		clock-names = "pcie0", "pcie1", "pcie2";
 
 		pcie@0,0 {
 			reg = <0x0000 0 0 0 0>;
-- 
2.19.1

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

Re: [PATCH v2 0/7] staging: mt7621-pci: some fixes after test previous series

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2474 bytes --]

On Sat, Nov 24 2018, Sergio Paracuellos wrote:

> Previous cleanup series was added to the staging tree without any
> testing. After get testing feedback some issues appear and this patch
> series should make the driver works properly again.
>
> Previous series are here:
> * http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/128200.html
>
> Feedback after testing from Neil Brown is here:
> * http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/129031.html
>
> There is one issue with chip revision and reset lines where those
> are inverted. I achieve this including some wrappers for checking
> the version in driver code and use proper reset_control_* functions.
> I checked the 'arch/mips/ralink/reset.c' and think a good way to add
> a quirk there but I ended up handling those inside the driver.
>
> Changes in v2:
>     - PATCH 7: In commit message: 's/mt7621-pcie/mt7621-pci/g'
>
> Hope this helps.
>
> Best regards,
>     Sergio Paracuellos
>
> Sergio Paracuellos (7):
>   staging: mt7621-pci: avoid mapping sysctls registers
>   staging: mt7621-dts: remove sysctl registers from pcie bindings
>   staging: mt7621-pci: dt-bindings: update bindings doc removing sysctls
>     registers
>   staging: mt7621-pci: fix reset lines for each pcie port
>   staging: mt7621-pci: avoid using clk_* operations

all above:
   Tested-by: NeilBrown <neil@brown.name>

Thanks!

>   staging: mt7621-dts: remove clocks for pcie bindings
>   staging: mt7621-pci: dt-bindings: update bindings doc removing clocks

I don't think we really want these - at least, not yet.
The clock numbers do appear in the driver as

#define PCIE_PORT_CLK_EN(x)		BIT(24 + (x))

and
		rt_sysc_m32(PCIE_PORT_CLK_EN(slot), 0, RALINK_CLKCFG1);
Maybe that can be made generic...

It is odd that the driver disables the clock, but never enables it.
There is other clock-related code in the pci driver.  Maybe it should
just stay there, maybe it should go to a clock driver.  I don't really
know.  But this dts stuff "looks" right, so I'd rather leave it until we
know that it will be useless.

Thanks,
NeilBrown




>
>  drivers/staging/mt7621-dts/mt7621.dtsi        |  5 +-
>  .../mt7621-pci/mediatek,mt7621-pci.txt        |  9 +--
>  drivers/staging/mt7621-pci/pci-mt7621.c       | 67 +++++++++----------
>  3 files changed, 34 insertions(+), 47 deletions(-)
>
> -- 
> 2.19.1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v2 0/7] staging: mt7621-pci: some fixes after test previous series

[Sergio Paracuellos]

On Sat, Nov 24, 2018 at 9:05 PM NeilBrown <neil@brown.name> wrote:
>
> On Sat, Nov 24 2018, Sergio Paracuellos wrote:
>
> > Previous cleanup series was added to the staging tree without any
> > testing. After get testing feedback some issues appear and this patch
> > series should make the driver works properly again.
> >
> > Previous series are here:
> > * http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/128200.html
> >
> > Feedback after testing from Neil Brown is here:
> > * http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/129031.html
> >
> > There is one issue with chip revision and reset lines where those
> > are inverted. I achieve this including some wrappers for checking
> > the version in driver code and use proper reset_control_* functions.
> > I checked the 'arch/mips/ralink/reset.c' and think a good way to add
> > a quirk there but I ended up handling those inside the driver.
> >
> > Changes in v2:
> >     - PATCH 7: In commit message: 's/mt7621-pcie/mt7621-pci/g'
> >
> > Hope this helps.
> >
> > Best regards,
> >     Sergio Paracuellos
> >
> > Sergio Paracuellos (7):
> >   staging: mt7621-pci: avoid mapping sysctls registers
> >   staging: mt7621-dts: remove sysctl registers from pcie bindings
> >   staging: mt7621-pci: dt-bindings: update bindings doc removing sysctls
> >     registers
> >   staging: mt7621-pci: fix reset lines for each pcie port
> >   staging: mt7621-pci: avoid using clk_* operations
>
> all above:
>    Tested-by: NeilBrown <neil@brown.name>
>
> Thanks!
>
> >   staging: mt7621-dts: remove clocks for pcie bindings
> >   staging: mt7621-pci: dt-bindings: update bindings doc removing clocks
>
> I don't think we really want these - at least, not yet.
> The clock numbers do appear in the driver as
>
> #define PCIE_PORT_CLK_EN(x)             BIT(24 + (x))
>
> and
>                 rt_sysc_m32(PCIE_PORT_CLK_EN(slot), 0, RALINK_CLKCFG1);
> Maybe that can be made generic...
>
> It is odd that the driver disables the clock, but never enables it.
> There is other clock-related code in the pci driver.  Maybe it should
> just stay there, maybe it should go to a clock driver.  I don't really
> know.  But this dts stuff "looks" right, so I'd rather leave it until we
> know that it will be useless.

Ok! I understand your point. Because those are the last two in the series, I
think there is no need to send v3 of this series without them. Greg, let me know
if you prefer me to send v3 without last two patches.

> Thanks,
> NeilBrown

Thanks to you for testing this.

Best regards,
    Sergio Paracuellos
>
>
>
>
> >
> >  drivers/staging/mt7621-dts/mt7621.dtsi        |  5 +-
> >  .../mt7621-pci/mediatek,mt7621-pci.txt        |  9 +--
> >  drivers/staging/mt7621-pci/pci-mt7621.c       | 67 +++++++++----------
> >  3 files changed, 34 insertions(+), 47 deletions(-)
> >
> > --
> > 2.19.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 4/7] staging: mt7621-pci: fix reset lines for each pcie port

[Dan Carpenter]

On Sat, Nov 24, 2018 at 06:54:54PM +0100, Sergio Paracuellos wrote:
> Depending of chip revision reset lines are inverted. It is also
> necessary to read PCIE_FTS_NUM register before enabling the phy.
> Hence update the code to achieve this.
> 
> Fixes: 745eeeac68d7: "staging: mt7621-pci: factor out 'mt7621_pcie_enable_port'
> function"
> Reported-by: NeilBrown <neil@brown.name>
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  drivers/staging/mt7621-pci/pci-mt7621.c | 38 +++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
> index ba81b34dc1b7..1b63706e129b 100644
> --- a/drivers/staging/mt7621-pci/pci-mt7621.c
> +++ b/drivers/staging/mt7621-pci/pci-mt7621.c
> @@ -412,6 +412,33 @@ static void mt7621_enable_phy(struct mt7621_pcie_port *port)
>  	set_phy_for_ssc(port);
>  }
>  
> +static inline void mt7621_control_assert(struct mt7621_pcie_port *port)
> +{
> +	u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
> +
> +	if ((chip_rev_id & 0xFFFF) == CHIP_REV_MT7621_E2)
> +		reset_control_assert(port->pcie_rst);
> +	else
> +		reset_control_deassert(port->pcie_rst);
> +}
> +
> +static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
> +{
> +	u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
> +
> +	if ((chip_rev_id & 0xFFFF) == CHIP_REV_MT7621_E2)
> +		reset_control_deassert(port->pcie_rst);
> +	else
> +		reset_control_assert(port->pcie_rst);
> +}

The commit message is very good that on some chips assert and deassert
mean the opposite but I feel like this should be commented in the code
as well or people reading this code will be very confused.

Also it would be better if we could change this from a white list to a
black list.  In other words, if they were to come out with new revs
of the hardware, we should assume that assert means assert and deassert
means deassert.

regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 5/7] staging: mt7621-pci: avoid using clk_* operations

[Dan Carpenter]

On Sat, Nov 24, 2018 at 06:54:55PM +0100, Sergio Paracuellos wrote:
> There is no clock driver for ralink mips and clk_enable
> are no-ops for this architecture. This has been also tested
> without using clocks and seems to work so avoid to use them
> in this driver.
> 
> Fixes: ad9c87e129d1: "staging: mt7621-pci: parse and init
> port data from device tree"
> Reported-by: NeilBrown <neil@brown.name>
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

This is fine, but in the future just let the Fixes tag go over the 75
character limit without wrapping it.  checkpatch.pl shouldn't complain.

Also you don't need to put a blank line between the Reported-by and the
S-o-b.

regards,
dan carpenter

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

Re: [PATCH v2 0/7] staging: mt7621-pci: some fixes after test previous series

[Greg KH]

On Sun, Nov 25, 2018 at 07:59:48AM +0100, Sergio Paracuellos wrote:
> On Sat, Nov 24, 2018 at 9:05 PM NeilBrown <neil@brown.name> wrote:
> >
> > On Sat, Nov 24 2018, Sergio Paracuellos wrote:
> >
> > > Previous cleanup series was added to the staging tree without any
> > > testing. After get testing feedback some issues appear and this patch
> > > series should make the driver works properly again.
> > >
> > > Previous series are here:
> > > * http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/128200.html
> > >
> > > Feedback after testing from Neil Brown is here:
> > > * http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/129031.html
> > >
> > > There is one issue with chip revision and reset lines where those
> > > are inverted. I achieve this including some wrappers for checking
> > > the version in driver code and use proper reset_control_* functions.
> > > I checked the 'arch/mips/ralink/reset.c' and think a good way to add
> > > a quirk there but I ended up handling those inside the driver.
> > >
> > > Changes in v2:
> > >     - PATCH 7: In commit message: 's/mt7621-pcie/mt7621-pci/g'
> > >
> > > Hope this helps.
> > >
> > > Best regards,
> > >     Sergio Paracuellos
> > >
> > > Sergio Paracuellos (7):
> > >   staging: mt7621-pci: avoid mapping sysctls registers
> > >   staging: mt7621-dts: remove sysctl registers from pcie bindings
> > >   staging: mt7621-pci: dt-bindings: update bindings doc removing sysctls
> > >     registers
> > >   staging: mt7621-pci: fix reset lines for each pcie port
> > >   staging: mt7621-pci: avoid using clk_* operations
> >
> > all above:
> >    Tested-by: NeilBrown <neil@brown.name>
> >
> > Thanks!
> >
> > >   staging: mt7621-dts: remove clocks for pcie bindings
> > >   staging: mt7621-pci: dt-bindings: update bindings doc removing clocks
> >
> > I don't think we really want these - at least, not yet.
> > The clock numbers do appear in the driver as
> >
> > #define PCIE_PORT_CLK_EN(x)             BIT(24 + (x))
> >
> > and
> >                 rt_sysc_m32(PCIE_PORT_CLK_EN(slot), 0, RALINK_CLKCFG1);
> > Maybe that can be made generic...
> >
> > It is odd that the driver disables the clock, but never enables it.
> > There is other clock-related code in the pci driver.  Maybe it should
> > just stay there, maybe it should go to a clock driver.  I don't really
> > know.  But this dts stuff "looks" right, so I'd rather leave it until we
> > know that it will be useless.
> 
> Ok! I understand your point. Because those are the last two in the series, I
> think there is no need to send v3 of this series without them. Greg, let me know
> if you prefer me to send v3 without last two patches.

I'll just drop the last two from my queue, no need to resend.

thanks,

greg k-h

Re: [PATCH v2 0/7] staging: mt7621-pci: some fixes after test previous series

[Sergio Paracuellos]

On Mon, Nov 26, 2018 at 4:28 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Sun, Nov 25, 2018 at 07:59:48AM +0100, Sergio Paracuellos wrote:
> > On Sat, Nov 24, 2018 at 9:05 PM NeilBrown <neil@brown.name> wrote:
> > >
> > > On Sat, Nov 24 2018, Sergio Paracuellos wrote:
> > >
> > > > Previous cleanup series was added to the staging tree without any
> > > > testing. After get testing feedback some issues appear and this patch
> > > > series should make the driver works properly again.
> > > >
> > > > Previous series are here:
> > > > * http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/128200.html
> > > >
> > > > Feedback after testing from Neil Brown is here:
> > > > * http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/129031.html
> > > >
> > > > There is one issue with chip revision and reset lines where those
> > > > are inverted. I achieve this including some wrappers for checking
> > > > the version in driver code and use proper reset_control_* functions.
> > > > I checked the 'arch/mips/ralink/reset.c' and think a good way to add
> > > > a quirk there but I ended up handling those inside the driver.
> > > >
> > > > Changes in v2:
> > > >     - PATCH 7: In commit message: 's/mt7621-pcie/mt7621-pci/g'
> > > >
> > > > Hope this helps.
> > > >
> > > > Best regards,
> > > >     Sergio Paracuellos
> > > >
> > > > Sergio Paracuellos (7):
> > > >   staging: mt7621-pci: avoid mapping sysctls registers
> > > >   staging: mt7621-dts: remove sysctl registers from pcie bindings
> > > >   staging: mt7621-pci: dt-bindings: update bindings doc removing sysctls
> > > >     registers
> > > >   staging: mt7621-pci: fix reset lines for each pcie port
> > > >   staging: mt7621-pci: avoid using clk_* operations
> > >
> > > all above:
> > >    Tested-by: NeilBrown <neil@brown.name>
> > >
> > > Thanks!
> > >
> > > >   staging: mt7621-dts: remove clocks for pcie bindings
> > > >   staging: mt7621-pci: dt-bindings: update bindings doc removing clocks
> > >
> > > I don't think we really want these - at least, not yet.
> > > The clock numbers do appear in the driver as
> > >
> > > #define PCIE_PORT_CLK_EN(x)             BIT(24 + (x))
> > >
> > > and
> > >                 rt_sysc_m32(PCIE_PORT_CLK_EN(slot), 0, RALINK_CLKCFG1);
> > > Maybe that can be made generic...
> > >
> > > It is odd that the driver disables the clock, but never enables it.
> > > There is other clock-related code in the pci driver.  Maybe it should
> > > just stay there, maybe it should go to a clock driver.  I don't really
> > > know.  But this dts stuff "looks" right, so I'd rather leave it until we
> > > know that it will be useless.
> >
> > Ok! I understand your point. Because those are the last two in the series, I
> > think there is no need to send v3 of this series without them. Greg, let me know
> > if you prefer me to send v3 without last two patches.
>
> I'll just drop the last two from my queue, no need to resend.

Greg, It seems last two patches were finally applied and they
shouldn't be. Let me know if you need
me to send patches to revert them.

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

Re: [PATCH v2 5/7] staging: mt7621-pci: avoid using clk_* operations

[Sergio Paracuellos]

On Mon, Nov 26, 2018 at 11:01 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Sat, Nov 24, 2018 at 06:54:55PM +0100, Sergio Paracuellos wrote:
> > There is no clock driver for ralink mips and clk_enable
> > are no-ops for this architecture. This has been also tested
> > without using clocks and seems to work so avoid to use them
> > in this driver.
> >
> > Fixes: ad9c87e129d1: "staging: mt7621-pci: parse and init
> > port data from device tree"
> > Reported-by: NeilBrown <neil@brown.name>
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>
> This is fine, but in the future just let the Fixes tag go over the 75
> character limit without wrapping it.  checkpatch.pl shouldn't complain.
>
> Also you don't need to put a blank line between the Reported-by and the
> S-o-b.

Thanks for advices, Dan.
I'll take those into account from know.

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

Re: [PATCH v2 4/7] staging: mt7621-pci: fix reset lines for each pcie port

[Sergio Paracuellos]

On Mon, Nov 26, 2018 at 10:57 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Sat, Nov 24, 2018 at 06:54:54PM +0100, Sergio Paracuellos wrote:
> > Depending of chip revision reset lines are inverted. It is also
> > necessary to read PCIE_FTS_NUM register before enabling the phy.
> > Hence update the code to achieve this.
> >
> > Fixes: 745eeeac68d7: "staging: mt7621-pci: factor out 'mt7621_pcie_enable_port'
> > function"
> > Reported-by: NeilBrown <neil@brown.name>
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > ---
> >  drivers/staging/mt7621-pci/pci-mt7621.c | 38 +++++++++++++++++++++----
> >  1 file changed, 32 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
> > index ba81b34dc1b7..1b63706e129b 100644
> > --- a/drivers/staging/mt7621-pci/pci-mt7621.c
> > +++ b/drivers/staging/mt7621-pci/pci-mt7621.c
> > @@ -412,6 +412,33 @@ static void mt7621_enable_phy(struct mt7621_pcie_port *port)
> >       set_phy_for_ssc(port);
> >  }
> >
> > +static inline void mt7621_control_assert(struct mt7621_pcie_port *port)
> > +{
> > +     u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
> > +
> > +     if ((chip_rev_id & 0xFFFF) == CHIP_REV_MT7621_E2)
> > +             reset_control_assert(port->pcie_rst);
> > +     else
> > +             reset_control_deassert(port->pcie_rst);
> > +}
> > +
> > +static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
> > +{
> > +     u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
> > +
> > +     if ((chip_rev_id & 0xFFFF) == CHIP_REV_MT7621_E2)
> > +             reset_control_deassert(port->pcie_rst);
> > +     else
> > +             reset_control_assert(port->pcie_rst);
> > +}
>
> The commit message is very good that on some chips assert and deassert
> mean the opposite but I feel like this should be commented in the code
> as well or people reading this code will be very confused.
>

Ok, Dan. Agreed. I will add some comment in next series.

> Also it would be better if we could change this from a white list to a
> black list.  In other words, if they were to come out with new revs
> of the hardware, we should assume that assert means assert and deassert
> means deassert.

I understand what you are saying but I don't know how to handle those
white and black lists... Is there some kind of example where I can
take a look to handle this in a proper way?

>
> regards,
> dan carpenter

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

Re: [PATCH v2 0/7] staging: mt7621-pci: some fixes after test previous series

[Greg KH]

On Mon, Nov 26, 2018 at 08:46:10PM +0100, Sergio Paracuellos wrote:
> On Mon, Nov 26, 2018 at 4:28 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Sun, Nov 25, 2018 at 07:59:48AM +0100, Sergio Paracuellos wrote:
> > > On Sat, Nov 24, 2018 at 9:05 PM NeilBrown <neil@brown.name> wrote:
> > > >
> > > > On Sat, Nov 24 2018, Sergio Paracuellos wrote:
> > > >
> > > > > Previous cleanup series was added to the staging tree without any
> > > > > testing. After get testing feedback some issues appear and this patch
> > > > > series should make the driver works properly again.
> > > > >
> > > > > Previous series are here:
> > > > > * http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/128200.html
> > > > >
> > > > > Feedback after testing from Neil Brown is here:
> > > > > * http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/129031.html
> > > > >
> > > > > There is one issue with chip revision and reset lines where those
> > > > > are inverted. I achieve this including some wrappers for checking
> > > > > the version in driver code and use proper reset_control_* functions.
> > > > > I checked the 'arch/mips/ralink/reset.c' and think a good way to add
> > > > > a quirk there but I ended up handling those inside the driver.
> > > > >
> > > > > Changes in v2:
> > > > >     - PATCH 7: In commit message: 's/mt7621-pcie/mt7621-pci/g'
> > > > >
> > > > > Hope this helps.
> > > > >
> > > > > Best regards,
> > > > >     Sergio Paracuellos
> > > > >
> > > > > Sergio Paracuellos (7):
> > > > >   staging: mt7621-pci: avoid mapping sysctls registers
> > > > >   staging: mt7621-dts: remove sysctl registers from pcie bindings
> > > > >   staging: mt7621-pci: dt-bindings: update bindings doc removing sysctls
> > > > >     registers
> > > > >   staging: mt7621-pci: fix reset lines for each pcie port
> > > > >   staging: mt7621-pci: avoid using clk_* operations
> > > >
> > > > all above:
> > > >    Tested-by: NeilBrown <neil@brown.name>
> > > >
> > > > Thanks!
> > > >
> > > > >   staging: mt7621-dts: remove clocks for pcie bindings
> > > > >   staging: mt7621-pci: dt-bindings: update bindings doc removing clocks
> > > >
> > > > I don't think we really want these - at least, not yet.
> > > > The clock numbers do appear in the driver as
> > > >
> > > > #define PCIE_PORT_CLK_EN(x)             BIT(24 + (x))
> > > >
> > > > and
> > > >                 rt_sysc_m32(PCIE_PORT_CLK_EN(slot), 0, RALINK_CLKCFG1);
> > > > Maybe that can be made generic...
> > > >
> > > > It is odd that the driver disables the clock, but never enables it.
> > > > There is other clock-related code in the pci driver.  Maybe it should
> > > > just stay there, maybe it should go to a clock driver.  I don't really
> > > > know.  But this dts stuff "looks" right, so I'd rather leave it until we
> > > > know that it will be useless.
> > >
> > > Ok! I understand your point. Because those are the last two in the series, I
> > > think there is no need to send v3 of this series without them. Greg, let me know
> > > if you prefer me to send v3 without last two patches.
> >
> > I'll just drop the last two from my queue, no need to resend.
> 
> Greg, It seems last two patches were finally applied and they
> shouldn't be. Let me know if you need
> me to send patches to revert them.

Ugh, my fault, let me go redo that branch and drop them, thanks for
catching this...

greg k-h

Re: [PATCH v2 0/7] staging: mt7621-pci: some fixes after test previous series

[Sergio Paracuellos]

On Mon, Nov 26, 2018 at 8:58 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Nov 26, 2018 at 08:46:10PM +0100, Sergio Paracuellos wrote:
> > On Mon, Nov 26, 2018 at 4:28 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Sun, Nov 25, 2018 at 07:59:48AM +0100, Sergio Paracuellos wrote:
> > > > On Sat, Nov 24, 2018 at 9:05 PM NeilBrown <neil@brown.name> wrote:
> > > > >
> > > > > On Sat, Nov 24 2018, Sergio Paracuellos wrote:
> > > > >
> > > > > > Previous cleanup series was added to the staging tree without any
> > > > > > testing. After get testing feedback some issues appear and this patch
> > > > > > series should make the driver works properly again.
> > > > > >
> > > > > > Previous series are here:
> > > > > > * http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/128200.html
> > > > > >
> > > > > > Feedback after testing from Neil Brown is here:
> > > > > > * http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-November/129031.html
> > > > > >
> > > > > > There is one issue with chip revision and reset lines where those
> > > > > > are inverted. I achieve this including some wrappers for checking
> > > > > > the version in driver code and use proper reset_control_* functions.
> > > > > > I checked the 'arch/mips/ralink/reset.c' and think a good way to add
> > > > > > a quirk there but I ended up handling those inside the driver.
> > > > > >
> > > > > > Changes in v2:
> > > > > >     - PATCH 7: In commit message: 's/mt7621-pcie/mt7621-pci/g'
> > > > > >
> > > > > > Hope this helps.
> > > > > >
> > > > > > Best regards,
> > > > > >     Sergio Paracuellos
> > > > > >
> > > > > > Sergio Paracuellos (7):
> > > > > >   staging: mt7621-pci: avoid mapping sysctls registers
> > > > > >   staging: mt7621-dts: remove sysctl registers from pcie bindings
> > > > > >   staging: mt7621-pci: dt-bindings: update bindings doc removing sysctls
> > > > > >     registers
> > > > > >   staging: mt7621-pci: fix reset lines for each pcie port
> > > > > >   staging: mt7621-pci: avoid using clk_* operations
> > > > >
> > > > > all above:
> > > > >    Tested-by: NeilBrown <neil@brown.name>
> > > > >
> > > > > Thanks!
> > > > >
> > > > > >   staging: mt7621-dts: remove clocks for pcie bindings
> > > > > >   staging: mt7621-pci: dt-bindings: update bindings doc removing clocks
> > > > >
> > > > > I don't think we really want these - at least, not yet.
> > > > > The clock numbers do appear in the driver as
> > > > >
> > > > > #define PCIE_PORT_CLK_EN(x)             BIT(24 + (x))
> > > > >
> > > > > and
> > > > >                 rt_sysc_m32(PCIE_PORT_CLK_EN(slot), 0, RALINK_CLKCFG1);
> > > > > Maybe that can be made generic...
> > > > >
> > > > > It is odd that the driver disables the clock, but never enables it.
> > > > > There is other clock-related code in the pci driver.  Maybe it should
> > > > > just stay there, maybe it should go to a clock driver.  I don't really
> > > > > know.  But this dts stuff "looks" right, so I'd rather leave it until we
> > > > > know that it will be useless.
> > > >
> > > > Ok! I understand your point. Because those are the last two in the series, I
> > > > think there is no need to send v3 of this series without them. Greg, let me know
> > > > if you prefer me to send v3 without last two patches.
> > >
> > > I'll just drop the last two from my queue, no need to resend.
> >
> > Greg, It seems last two patches were finally applied and they
> > shouldn't be. Let me know if you need
> > me to send patches to revert them.
>
> Ugh, my fault, let me go redo that branch and drop them, thanks for
> catching this...

No problem at all. Thanks for redo this.

Best regards,
    Sergio Paracuellos

>
> greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 4/7] staging: mt7621-pci: fix reset lines for each pcie port

[Dan Carpenter]

On Mon, Nov 26, 2018 at 08:57:09PM +0100, Sergio Paracuellos wrote:
> On Mon, Nov 26, 2018 at 10:57 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Sat, Nov 24, 2018 at 06:54:54PM +0100, Sergio Paracuellos wrote:
> > > Depending of chip revision reset lines are inverted. It is also
> > > necessary to read PCIE_FTS_NUM register before enabling the phy.
> > > Hence update the code to achieve this.
> > >
> > > Fixes: 745eeeac68d7: "staging: mt7621-pci: factor out 'mt7621_pcie_enable_port'
> > > function"
> > > Reported-by: NeilBrown <neil@brown.name>
> > >
> > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > ---
> > >  drivers/staging/mt7621-pci/pci-mt7621.c | 38 +++++++++++++++++++++----
> > >  1 file changed, 32 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
> > > index ba81b34dc1b7..1b63706e129b 100644
> > > --- a/drivers/staging/mt7621-pci/pci-mt7621.c
> > > +++ b/drivers/staging/mt7621-pci/pci-mt7621.c
> > > @@ -412,6 +412,33 @@ static void mt7621_enable_phy(struct mt7621_pcie_port *port)
> > >       set_phy_for_ssc(port);
> > >  }
> > >
> > > +static inline void mt7621_control_assert(struct mt7621_pcie_port *port)
> > > +{
> > > +     u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
> > > +
> > > +     if ((chip_rev_id & 0xFFFF) == CHIP_REV_MT7621_E2)
> > > +             reset_control_assert(port->pcie_rst);
> > > +     else
> > > +             reset_control_deassert(port->pcie_rst);
> > > +}
> > > +
> > > +static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
> > > +{
> > > +     u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
> > > +
> > > +     if ((chip_rev_id & 0xFFFF) == CHIP_REV_MT7621_E2)
> > > +             reset_control_deassert(port->pcie_rst);
> > > +     else
> > > +             reset_control_assert(port->pcie_rst);
> > > +}
> >
> > The commit message is very good that on some chips assert and deassert
> > mean the opposite but I feel like this should be commented in the code
> > as well or people reading this code will be very confused.
> >
> 
> Ok, Dan. Agreed. I will add some comment in next series.
> 
> > Also it would be better if we could change this from a white list to a
> > black list.  In other words, if they were to come out with new revs
> > of the hardware, we should assume that assert means assert and deassert
> > means deassert.
> 
> I understand what you are saying but I don't know how to handle those
> white and black lists... Is there some kind of example where I can
> take a look to handle this in a proper way?
> 

What I mean is flip it around so that it becomes:

static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
{
	u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);

	/* Some revisions have assert and deassert reversed.  */
	if (chip_in_list_of_reversed_revs) {
		reset_control_assert(port->pcie_rst);
		return;
	}

	reset_control_deassert(port->pcie_rst);
}

regards,
dan carpenter

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

Re: [PATCH v2 4/7] staging: mt7621-pci: fix reset lines for each pcie port

[NeilBrown]

[-- Attachment #1.1: Type: text/plain, Size: 3847 bytes --]

On Tue, Nov 27 2018, Dan Carpenter wrote:

> On Mon, Nov 26, 2018 at 08:57:09PM +0100, Sergio Paracuellos wrote:
>> On Mon, Nov 26, 2018 at 10:57 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> >
>> > On Sat, Nov 24, 2018 at 06:54:54PM +0100, Sergio Paracuellos wrote:
>> > > Depending of chip revision reset lines are inverted. It is also
>> > > necessary to read PCIE_FTS_NUM register before enabling the phy.
>> > > Hence update the code to achieve this.
>> > >
>> > > Fixes: 745eeeac68d7: "staging: mt7621-pci: factor out 'mt7621_pcie_enable_port'
>> > > function"
>> > > Reported-by: NeilBrown <neil@brown.name>
>> > >
>> > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>> > > ---
>> > >  drivers/staging/mt7621-pci/pci-mt7621.c | 38 +++++++++++++++++++++----
>> > >  1 file changed, 32 insertions(+), 6 deletions(-)
>> > >
>> > > diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
>> > > index ba81b34dc1b7..1b63706e129b 100644
>> > > --- a/drivers/staging/mt7621-pci/pci-mt7621.c
>> > > +++ b/drivers/staging/mt7621-pci/pci-mt7621.c
>> > > @@ -412,6 +412,33 @@ static void mt7621_enable_phy(struct mt7621_pcie_port *port)
>> > >       set_phy_for_ssc(port);
>> > >  }
>> > >
>> > > +static inline void mt7621_control_assert(struct mt7621_pcie_port *port)
>> > > +{
>> > > +     u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
>> > > +
>> > > +     if ((chip_rev_id & 0xFFFF) == CHIP_REV_MT7621_E2)
>> > > +             reset_control_assert(port->pcie_rst);
>> > > +     else
>> > > +             reset_control_deassert(port->pcie_rst);
>> > > +}
>> > > +
>> > > +static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
>> > > +{
>> > > +     u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
>> > > +
>> > > +     if ((chip_rev_id & 0xFFFF) == CHIP_REV_MT7621_E2)
>> > > +             reset_control_deassert(port->pcie_rst);
>> > > +     else
>> > > +             reset_control_assert(port->pcie_rst);
>> > > +}
>> >
>> > The commit message is very good that on some chips assert and deassert
>> > mean the opposite but I feel like this should be commented in the code
>> > as well or people reading this code will be very confused.
>> >
>> 
>> Ok, Dan. Agreed. I will add some comment in next series.
>> 
>> > Also it would be better if we could change this from a white list to a
>> > black list.  In other words, if they were to come out with new revs
>> > of the hardware, we should assume that assert means assert and deassert
>> > means deassert.
>> 
>> I understand what you are saying but I don't know how to handle those
>> white and black lists... Is there some kind of example where I can
>> take a look to handle this in a proper way?
>> 
>
> What I mean is flip it around so that it becomes:
>
> static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
> {
> 	u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
>
> 	/* Some revisions have assert and deassert reversed.  */
> 	if (chip_in_list_of_reversed_revs) {

Unfortunately we don't have that list.
All we have comes from:
 https://github.com/mqmaker/linux/blob/master/arch/mips/ralink/pci.c#L97
which suggests that any MT7621 RAlink pci controller that doesn't have 0x0101 at
the end of the chip_id has inverted PCI resets.
We don't have a list of all MT7621 CHIP_IDs to produce a blacklist from.

The only other info we have is that 0x030103 is one particular MT7621
CHIP_ID that is inverted (the one that I own).

So while I agree that a black-list would be best, that facts are that we
don't have one.

Thanks,
NeilBrown


> 		reset_control_assert(port->pcie_rst);
> 		return;
> 	}
>
> 	reset_control_deassert(port->pcie_rst);
> }
>
> regards,
> dan carpenter

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

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

Re: [PATCH v2 4/7] staging: mt7621-pci: fix reset lines for each pcie port

[Dan Carpenter]

On Tue, Nov 27, 2018 at 01:12:42PM +1100, NeilBrown wrote:
> On Tue, Nov 27 2018, Dan Carpenter wrote:
> 
> > On Mon, Nov 26, 2018 at 08:57:09PM +0100, Sergio Paracuellos wrote:
> >> On Mon, Nov 26, 2018 at 10:57 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >> >
> >> > On Sat, Nov 24, 2018 at 06:54:54PM +0100, Sergio Paracuellos wrote:
> >> > > Depending of chip revision reset lines are inverted. It is also
> >> > > necessary to read PCIE_FTS_NUM register before enabling the phy.
> >> > > Hence update the code to achieve this.
> >> > >
> >> > > Fixes: 745eeeac68d7: "staging: mt7621-pci: factor out 'mt7621_pcie_enable_port'
> >> > > function"
> >> > > Reported-by: NeilBrown <neil@brown.name>
> >> > >
> >> > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> >> > > ---
> >> > >  drivers/staging/mt7621-pci/pci-mt7621.c | 38 +++++++++++++++++++++----
> >> > >  1 file changed, 32 insertions(+), 6 deletions(-)
> >> > >
> >> > > diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
> >> > > index ba81b34dc1b7..1b63706e129b 100644
> >> > > --- a/drivers/staging/mt7621-pci/pci-mt7621.c
> >> > > +++ b/drivers/staging/mt7621-pci/pci-mt7621.c
> >> > > @@ -412,6 +412,33 @@ static void mt7621_enable_phy(struct mt7621_pcie_port *port)
> >> > >       set_phy_for_ssc(port);
> >> > >  }
> >> > >
> >> > > +static inline void mt7621_control_assert(struct mt7621_pcie_port *port)
> >> > > +{
> >> > > +     u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
> >> > > +
> >> > > +     if ((chip_rev_id & 0xFFFF) == CHIP_REV_MT7621_E2)
> >> > > +             reset_control_assert(port->pcie_rst);
> >> > > +     else
> >> > > +             reset_control_deassert(port->pcie_rst);
> >> > > +}
> >> > > +
> >> > > +static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
> >> > > +{
> >> > > +     u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
> >> > > +
> >> > > +     if ((chip_rev_id & 0xFFFF) == CHIP_REV_MT7621_E2)
> >> > > +             reset_control_deassert(port->pcie_rst);
> >> > > +     else
> >> > > +             reset_control_assert(port->pcie_rst);
> >> > > +}
> >> >
> >> > The commit message is very good that on some chips assert and deassert
> >> > mean the opposite but I feel like this should be commented in the code
> >> > as well or people reading this code will be very confused.
> >> >
> >> 
> >> Ok, Dan. Agreed. I will add some comment in next series.
> >> 
> >> > Also it would be better if we could change this from a white list to a
> >> > black list.  In other words, if they were to come out with new revs
> >> > of the hardware, we should assume that assert means assert and deassert
> >> > means deassert.
> >> 
> >> I understand what you are saying but I don't know how to handle those
> >> white and black lists... Is there some kind of example where I can
> >> take a look to handle this in a proper way?
> >> 
> >
> > What I mean is flip it around so that it becomes:
> >
> > static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
> > {
> > 	u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
> >
> > 	/* Some revisions have assert and deassert reversed.  */
> > 	if (chip_in_list_of_reversed_revs) {
> 
> Unfortunately we don't have that list.
> All we have comes from:
>  https://github.com/mqmaker/linux/blob/master/arch/mips/ralink/pci.c#L97
> which suggests that any MT7621 RAlink pci controller that doesn't have 0x0101 at
> the end of the chip_id has inverted PCI resets.
> We don't have a list of all MT7621 CHIP_IDs to produce a blacklist from.
> 
> The only other info we have is that 0x030103 is one particular MT7621
> CHIP_ID that is inverted (the one that I own).
> 
> So while I agree that a black-list would be best, that facts are that we
> don't have one.

Ah...  :(

regards,
dan carpenter