On Mon, Jul 09, 2018 at 09:13:02AM +1000, NeilBrown wrote:
> 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 <[email protected]>
> > ---
> > 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.
My eyes said to my head all the mask were the same :-). You
are totally right I need the switch statement back. I'll
re-do this in a proper way.
Thanks,
>
> 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
Best regards,
Sergio Paracuellos
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
