[PATCH v2 03/18] staging: mt7621-pci: avoid pointer arithmetics in some macros

NeilBrown neil at brown.name
Sun Jul 8 23:13:02 UTC 2018


On Sun, Jul 08 2018, Sergio Paracuellos wrote:

> RALINK_PCI_MEMBASE, RALINK_PCI_IOBASE, RALINK_PCI_PCICFG_ADDR and
> RALINK_PCI_PCIMSK_ADDR are using very ugly pointer arithmetics to
> read and write along the code. Instead of doing this, use the
> mt7621_pci_reg_read and mt7621_pci_reg_write functions making
> this a bit cleaner.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos at gmail.com>
> ---
>  drivers/staging/mt7621-pci/pci-mt7621.c | 59 ++++++++++++++-------------------
>  1 file changed, 25 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
> index 32c37e8..f7defa7 100644
> --- a/drivers/staging/mt7621-pci/pci-mt7621.c
> +++ b/drivers/staging/mt7621-pci/pci-mt7621.c
> @@ -68,14 +68,14 @@
>  
>  #define RALINK_PCI_CONFIG_ADDR		0x20
>  #define RALINK_PCI_CONFIG_DATA_VIRTUAL_REG	0x24
> -#define RALINK_PCI_MEMBASE		*(volatile u32 *)(RALINK_PCI_BASE + 0x0028)
> -#define RALINK_PCI_IOBASE		*(volatile u32 *)(RALINK_PCI_BASE + 0x002C)
> +#define RALINK_PCI_MEMBASE		0x0028
> +#define RALINK_PCI_IOBASE		0x002C
>  #define RALINK_PCIE0_RST		(1<<24)
>  #define RALINK_PCIE1_RST		(1<<25)
>  #define RALINK_PCIE2_RST		(1<<26)
>  
> -#define RALINK_PCI_PCICFG_ADDR		*(volatile u32 *)(RALINK_PCI_BASE + 0x0000)
> -#define RALINK_PCI_PCIMSK_ADDR		*(volatile u32 *)(RALINK_PCI_BASE + 0x000C)
> +#define RALINK_PCI_PCICFG_ADDR		0x0000
> +#define RALINK_PCI_PCIMSK_ADDR		0x000C
>  #define RALINK_PCI_BASE	0xBE140000
>  
>  #define RALINK_PCIEPHY_P0P1_CTL_OFFSET	(RALINK_PCI_BASE + 0x9000)
> @@ -408,6 +408,7 @@ void setup_cm_memory_region(struct resource *mem_resource)
>  
>  static int mt7621_pci_probe(struct platform_device *pdev)
>  {
> +	u32 mask;
>  	unsigned long val = 0;
>  
>  	mt7621_pci_base = (void __iomem *)RALINK_PCI_BASE;
> @@ -471,7 +472,9 @@ static int mt7621_pci_probe(struct platform_device *pdev)
>  		pcie_link_status &= ~(1<<0);
>  	} else {
>  		pcie_link_status |= 1<<0;
> -		RALINK_PCI_PCIMSK_ADDR |= (1<<20); // enable pcie1 interrupt
> +		mask = mt7621_pci_reg_read(RALINK_PCI_PCIMSK_ADDR);
> +		mask |= (1<<20); // enable pcie1 interrupt
> +		mt7621_pci_reg_write(mask, RALINK_PCI_PCIMSK_ADDR);
>  	}
>  
>  	if ((mt7621_pci_reg_read(RALINK_PCI_STATUS(1)) & 0x1) == 0) {
> @@ -481,7 +484,9 @@ static int mt7621_pci_probe(struct platform_device *pdev)
>  		pcie_link_status &= ~(1<<1);
>  	} else {
>  		pcie_link_status |= 1<<1;
> -		RALINK_PCI_PCIMSK_ADDR |= (1<<21); // enable pcie1 interrupt
> +		mask = mt7621_pci_reg_read(RALINK_PCI_PCIMSK_ADDR);
> +		mask |= (1<<21); // enable pcie1 interrupt
> +		mt7621_pci_reg_write(mask, RALINK_PCI_PCIMSK_ADDR);
>  	}
>  
>  	if ((mt7621_pci_reg_read(RALINK_PCI_STATUS(2)) & 0x1) == 0) {
> @@ -491,7 +496,9 @@ static int mt7621_pci_probe(struct platform_device *pdev)
>  		pcie_link_status &= ~(1<<2);
>  	} else {
>  		pcie_link_status |= 1<<2;
> -		RALINK_PCI_PCIMSK_ADDR |= (1<<22); // enable pcie2 interrupt
> +		mask = mt7621_pci_reg_read(RALINK_PCI_PCIMSK_ADDR);
> +		mask |= (1<<22); // enable pcie2 interrupt
> +		mt7621_pci_reg_write(mask, RALINK_PCI_PCIMSK_ADDR);
>  	}
>  
>  	if (pcie_link_status == 0)
> @@ -508,39 +515,23 @@ pcie(2/1/0) link status	pcie2_num	pcie1_num	pcie0_num
>  3'b110			1		0		x
>  3'b111			2		1		0
>  */
> -	switch (pcie_link_status) {
> -	case 2:
> -		RALINK_PCI_PCICFG_ADDR &= ~0x00ff0000;
> -		RALINK_PCI_PCICFG_ADDR |= 0x1 << 16;	//port0
> -		RALINK_PCI_PCICFG_ADDR |= 0x0 << 20;	//port1
> -		break;
> -	case 4:
> -		RALINK_PCI_PCICFG_ADDR &= ~0x0fff0000;
> -		RALINK_PCI_PCICFG_ADDR |= 0x1 << 16;	//port0
> -		RALINK_PCI_PCICFG_ADDR |= 0x2 << 20;	//port1
> -		RALINK_PCI_PCICFG_ADDR |= 0x0 << 24;	//port2
> -		break;
> -	case 5:
> -		RALINK_PCI_PCICFG_ADDR &= ~0x0fff0000;
> -		RALINK_PCI_PCICFG_ADDR |= 0x0 << 16;	//port0
> -		RALINK_PCI_PCICFG_ADDR |= 0x2 << 20;	//port1
> -		RALINK_PCI_PCICFG_ADDR |= 0x1 << 24;	//port2
> -		break;
> -	case 6:
> -		RALINK_PCI_PCICFG_ADDR &= ~0x0fff0000;
> -		RALINK_PCI_PCICFG_ADDR |= 0x2 << 16;	//port0
> -		RALINK_PCI_PCICFG_ADDR |= 0x0 << 20;	//port1
> -		RALINK_PCI_PCICFG_ADDR |= 0x1 << 24;	//port2
> -		break;
> -	}
> +	mask = mt7621_pci_reg_read(RALINK_PCI_PCICFG_ADDR);
> +	mask &= ~0x00ff0000;
> +	mask |= (0x1 << 16); // port0
> +	mask |= (0x0 << 20); // port1
> +
> +	if (pcie_link_status != 2)
> +		mask |= (0x1 << 24); // port2
> +
> +	mt7621_pci_reg_write(mask, RALINK_PCI_PCICFG_ADDR);

You've discarded the switch statement, and not replaced much of it.
There were two different masks applied to RALINK_PCI_CFG_ADDR (one with
2 'f's, one with 3) but you now only have the one with ff.
The values <<16 is sometimes 0, sometimes 1, sometimes 2. etc.

I think you really need that switch statement back.

Below is what I'm working with

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index d92df914b16f..3d4c0fa27481 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -522,12 +522,33 @@ pcie(2/1/0) link status	pcie2_num	pcie1_num	pcie0_num
 3'b111			2		1		0
 */
 	mask = mt7621_pci_reg_read(RALINK_PCI_PCICFG_ADDR);
-	mask &= ~0x00ff0000;
-	mask |= (0x1 << 16); // port0
-	mask |= (0x0 << 20); // port1
+	switch (pcie_link_status) {
+	case 2:
+		mask &= ~0x00ff0000;
+		mask |= (0x1 << 16); // port0
+		mask |= (0x0 << 20); // port1
+		break;
 
-	if (pcie_link_status != 2)
+	case 4:
+		mask &= ~0x0fff0000;
+		mask |= (0x1 << 16); // port0
+		mask |= (0x2 << 20); // port1
+		break;
+
+	case 5:
+		mask &= ~0x0fff0000;
+		mask |= (0x0 << 16); // port0
+		mask |= (0x2 << 20); // port1
 		mask |= (0x1 << 24); // port2
+		break;
+
+	case 6:
+		mask &= ~0x0fff0000;
+		mask |= (0x2 << 16); // port0
+		mask |= (0x0 << 20); // port1
+		mask |= (0x1 << 24); // port2
+		break;
+	}
 
 	mt7621_pci_reg_write(mask, RALINK_PCI_PCICFG_ADDR);
 

Thanks,
NeilBrown
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/attachments/20180709/0ffc8413/attachment.asc>


More information about the devel mailing list