Re: [PATCH] staging: mt7621-pci: simplify 'mt7621_pcie_init_virtual_bridges' function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Mar 08 2020, Sergio Paracuellos wrote:

> Function 'mt7621_pcie_init_virtual_bridges' is a bit mess and can be
> refactorized properly in a cleaner way. Introduce new 'pcie_rmw' inline
> function helper to do clear and set the correct bits this function needs
> to work.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>
> ---
> Changes are only compile tested.
>  drivers/staging/mt7621-pci/pci-mt7621.c | 85 ++++++++++---------------
>  1 file changed, 33 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
> index 3633c924848e..1f860c5ef588 100644
> --- a/drivers/staging/mt7621-pci/pci-mt7621.c
> +++ b/drivers/staging/mt7621-pci/pci-mt7621.c
> @@ -57,13 +57,13 @@
>  #define RALINK_PCI_IOBASE		0x002C
>  
>  /* PCICFG virtual bridges */
> -#define MT7621_BR0_MASK			GENMASK(19, 16)
> -#define MT7621_BR1_MASK			GENMASK(23, 20)
> -#define MT7621_BR2_MASK			GENMASK(27, 24)
> -#define MT7621_BR_ALL_MASK		GENMASK(27, 16)
> -#define MT7621_BR0_SHIFT		16
> -#define MT7621_BR1_SHIFT		20
> -#define MT7621_BR2_SHIFT		24
> +#define PCIE_P2P_MAX			3

This is one of my bug-bears.  The number "3" here is not a MAXimum.
It is a count or a number.  It is how many masks there are.
The masks are numbered 0, 1, 2 so the maximum is 2.
I would rename this  PCI_P2P_CNT.


> +#define PCIE_P2P_BR_DEVNUM_SHIFT(p)	(16 + (p) * 4)
> +#define PCIE_P2P_BR_DEVNUM0_SHIFT	PCIE_P2P_BR_DEVNUM_SHIFT(0)
> +#define PCIE_P2P_BR_DEVNUM1_SHIFT	PCIE_P2P_BR_DEVNUM_SHIFT(1)
> +#define PCIE_P2P_BR_DEVNUM2_SHIFT	PCIE_P2P_BR_DEVNUM_SHIFT(2)
> +#define PCIE_P2P_BR_DEVNUM_MASK		0xf
> +#define PCIE_P2P_BR_DEVNUM_MASK_FULL	(0xfff << PCIE_P2P_BR_DEVNUM0_SHIFT)
>  
>  /* PCIe RC control registers */
>  #define MT7621_PCIE_OFFSET		0x2000
> @@ -154,6 +154,15 @@ static inline void pcie_write(struct mt7621_pcie *pcie, u32 val, u32 reg)
>  	writel(val, pcie->base + reg);
>  }
>  
> +static inline void pcie_rmw(struct mt7621_pcie *pcie, u32 reg, u32 clr, u32 set)
> +{
> +	u32 val = readl(pcie->base + reg);
> +
> +	val &= ~clr;
> +	val |= set;
> +	writel(val, pcie->base + reg);
> +}
> +
>  static inline u32 pcie_port_read(struct mt7621_pcie_port *port, u32 reg)
>  {
>  	return readl(port->base + reg);
> @@ -554,7 +563,9 @@ static void mt7621_pcie_enable_ports(struct mt7621_pcie *pcie)
>  static int mt7621_pcie_init_virtual_bridges(struct mt7621_pcie *pcie)
>  {
>  	u32 pcie_link_status = 0;
> -	u32 val = 0;
> +	u32 n;
> +	int i;
> +	u32 p2p_br_devnum[PCIE_P2P_MAX];
>  	struct mt7621_pcie_port *port;
>  
>  	list_for_each_entry(port, &pcie->ports, list) {
> @@ -567,50 +578,20 @@ static int mt7621_pcie_init_virtual_bridges(struct mt7621_pcie *pcie)
>  	if (pcie_link_status == 0)
>  		return -1;
>  
> -	/*
> -	 * pcie(2/1/0) link status	pcie2_num	pcie1_num	pcie0_num
> -	 * 3'b000			x		x		x
> -	 * 3'b001			x		x		0
> -	 * 3'b010			x		0		x
> -	 * 3'b011			x		1		0
> -	 * 3'b100			0		x		x
> -	 * 3'b101			1		x		0
> -	 * 3'b110			1		0		x
> -	 * 3'b111			2		1		0
> -	 */
> -	switch (pcie_link_status) {
> -	case 2:
> -		val = pcie_read(pcie, RALINK_PCI_PCICFG_ADDR);
> -		val &= ~(MT7621_BR0_MASK | MT7621_BR1_MASK);
> -		val |= 0x1 << MT7621_BR0_SHIFT;
> -		val |= 0x0 << MT7621_BR1_SHIFT;
> -		pcie_write(pcie, val, RALINK_PCI_PCICFG_ADDR);
> -		break;
> -	case 4:
> -		val = pcie_read(pcie, RALINK_PCI_PCICFG_ADDR);
> -		val &= ~MT7621_BR_ALL_MASK;
> -		val |= 0x1 << MT7621_BR0_SHIFT;
> -		val |= 0x2 << MT7621_BR1_SHIFT;
> -		val |= 0x0 << MT7621_BR2_SHIFT;
> -		pcie_write(pcie, val, RALINK_PCI_PCICFG_ADDR);
> -		break;
> -	case 5:
> -		val = pcie_read(pcie, RALINK_PCI_PCICFG_ADDR);
> -		val &= ~MT7621_BR_ALL_MASK;
> -		val |= 0x0 << MT7621_BR0_SHIFT;
> -		val |= 0x2 << MT7621_BR1_SHIFT;
> -		val |= 0x1 << MT7621_BR2_SHIFT;
> -		pcie_write(pcie, val, RALINK_PCI_PCICFG_ADDR);
> -		break;
> -	case 6:
> -		val = pcie_read(pcie, RALINK_PCI_PCICFG_ADDR);
> -		val &= ~MT7621_BR_ALL_MASK;
> -		val |= 0x2 << MT7621_BR0_SHIFT;
> -		val |= 0x0 << MT7621_BR1_SHIFT;
> -		val |= 0x1 << MT7621_BR2_SHIFT;
> -		pcie_write(pcie, val, RALINK_PCI_PCICFG_ADDR);
> -		break;
> -	}
> +	n = 0;
> +	for (i = 0; i < PCIE_P2P_MAX; i++)

Here, for example, 'i' never reaches the MAX value.  Surely that is wrong.

> +		if (pcie_link_status & BIT(i))
> +			p2p_br_devnum[i] = n++;
> +
> +	for (i = 0; i < PCIE_P2P_MAX; i++)
> +		if ((pcie_link_status & BIT(i)) == 0)
> +			p2p_br_devnum[i] = n++;

This second for loop seems like a change in functionality to what we had
before.  Is that correct?  I seems to make sense but as you didn't flag
the change in the commit message I thought I would ask.

Also I feel it would help to have a comment explaining what was going
on.  There was a big comment here before.  It wasn't particularly
helpful, but it was a little better than nothing.
Maybe:

 /* Assign device numbers from zero to the enabled ports, then assigning
  * remaining device numbers to any disabled ports
  */

Thanks,
NeilBrown


> +
> +	pcie_rmw(pcie, RALINK_PCI_CONFIG_ADDR,
> +		 PCIE_P2P_BR_DEVNUM_MASK_FULL,
> +		 (p2p_br_devnum[0] << PCIE_P2P_BR_DEVNUM0_SHIFT) |
> +		 (p2p_br_devnum[1] << PCIE_P2P_BR_DEVNUM1_SHIFT) |
> +		 (p2p_br_devnum[2] << PCIE_P2P_BR_DEVNUM2_SHIFT));
>  
>  	return 0;
>  }
> -- 
> 2.19.1

Attachment: signature.asc
Description: PGP signature

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

[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux