Re: [PATCH] staging: mt7621-pci: refactor pci related read and writes functions
On Mon, Jul 09, 2018 at 07:16:41AM +1000, NeilBrown wrote: > On Sun, Jul 08 2018, Sergio Paracuellos wrote: > > > > I can try to split this a bit but I don't really know how to do atomic > > changes because > > of the related things included here. > > Some suggestions: > > 1/ MV_READ_DATA is not used. Discard that any anything else that is >not used. > > 2/ MV_WRITE is replaced with writel(), MV_READ() with readl(). Make that >one patch. > > 3/ unsigned int becomes u32. That is a patch by itself. (maybe this has >to go before 2. > > 4/ Introduce mt7621_pci_get_cfgaddr() and use it where it is a drop-in >replacement. > > 5/ Introduce new implementations of pci_config_read() and >pci_config_write. > > 6/ discard any newly dead code (e.g write_config_* ??) > > Now if there is anything left in the patch is should be clear if it > could benefit from being broken up. > > Thanks for working on this. Thanks to you for this advices. I'll try to split this in the way you are pointing out here. > > NeilBrown Best regards, Sergio Paracuellos ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: mt7621-pci: refactor pci related read and writes functions
On Sun, Jul 08 2018, Sergio Paracuellos wrote: > > I can try to split this a bit but I don't really know how to do atomic > changes because > of the related things included here. Some suggestions: 1/ MV_READ_DATA is not used. Discard that any anything else that is not used. 2/ MV_WRITE is replaced with writel(), MV_READ() with readl(). Make that one patch. 3/ unsigned int becomes u32. That is a patch by itself. (maybe this has to go before 2. 4/ Introduce mt7621_pci_get_cfgaddr() and use it where it is a drop-in replacement. 5/ Introduce new implementations of pci_config_read() and pci_config_write. 6/ discard any newly dead code (e.g write_config_* ??) Now if there is anything left in the patch is should be clear if it could benefit from being broken up. Thanks for working on this. NeilBrown signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: mt7621-pci: refactor pci related read and writes functions
On Sun, Jul 8, 2018 at 5:36 PM, Greg KH wrote: > On Sat, Jul 07, 2018 at 09:48:37PM +0200, Sergio Paracuellos wrote: >> This commit simplifies and clean a lot of stuff related with pci >> reads and writes. It deletes a lot of not needed at all functions >> and use kernel arch operations read[b,w,l] and write[b,w,l] instead >> of use custom macros. It also include one function helper called >> 'mt7621_pci_get_cfgaddr' to easily obtain config address. Also to >> get pci base address a global 'mt7621_pci_base' variable has been >> included and initialized as a pointer to RALINK_PCI_BASE in driver >> probe function. With this changes LOC is clearly decreased and >> readability is increased. > > A lot of different things are happening here in this patch, making it > hard to review. Any chance to split this up into smaller, easier to > review, parts? It can be but all changes are really related and this just delete all of those crazy read and write functions and adding some helpers in the way to make easier the rewrite of real read and write. > > And you adding mt7621_pci_base is a nice start, but that really should > be a device-specific variable, not a global one. I can't belive this > driver works with a hard-coded base address, that's crazy... Shouldn't > that value be read from the PCI device itself instead? I know this shouldn't be a global variable but, as you said, is a nice start to make this driver a little cleaner for be able to do a better cleanups series. Also all of a lot of hardcoded values should be read from device tree in next cleanups in order to have a cleaner driver. Also this patch is the first in my next series and as you can see it contains very ugly hacks, so I though to do a first simple cleanups first for finally end up with easier real cleans. Maybe that is not the best approach. I can try to split this a bit but I don't really know how to do atomic changes because of the related things included here. > > thanks, > > greg k-h Best regards, Sergio Paracuellos ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: mt7621-pci: refactor pci related read and writes functions
On Sat, Jul 07, 2018 at 09:48:37PM +0200, Sergio Paracuellos wrote: > This commit simplifies and clean a lot of stuff related with pci > reads and writes. It deletes a lot of not needed at all functions > and use kernel arch operations read[b,w,l] and write[b,w,l] instead > of use custom macros. It also include one function helper called > 'mt7621_pci_get_cfgaddr' to easily obtain config address. Also to > get pci base address a global 'mt7621_pci_base' variable has been > included and initialized as a pointer to RALINK_PCI_BASE in driver > probe function. With this changes LOC is clearly decreased and > readability is increased. A lot of different things are happening here in this patch, making it hard to review. Any chance to split this up into smaller, easier to review, parts? And you adding mt7621_pci_base is a nice start, but that really should be a device-specific variable, not a global one. I can't belive this driver works with a hard-coded base address, that's crazy... Shouldn't that value be read from the PCI device itself instead? thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: mt7621-pci: refactor pci related read and writes functions
This commit simplifies and clean a lot of stuff related with pci
reads and writes. It deletes a lot of not needed at all functions
and use kernel arch operations read[b,w,l] and write[b,w,l] instead
of use custom macros. It also include one function helper called
'mt7621_pci_get_cfgaddr' to easily obtain config address. Also to
get pci base address a global 'mt7621_pci_base' variable has been
included and initialized as a pointer to RALINK_PCI_BASE in driver
probe function. With this changes LOC is clearly decreased and
readability is increased.
Signed-off-by: Sergio Paracuellos
---
drivers/staging/mt7621-pci/pci-mt7621.c | 178 +++-
1 file changed, 58 insertions(+), 120 deletions(-)
diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c
b/drivers/staging/mt7621-pci/pci-mt7621.c
index c12447d..dabe5c4 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -113,23 +113,6 @@
#define RALINK_PCIEPHY_P0P1_CTL_OFFSET (RALINK_PCI_BASE + 0x9000)
#define RALINK_PCIEPHY_P2_CTL_OFFSET (RALINK_PCI_BASE + 0xA000)
-#define MV_WRITE(ofs, data)\
- *(volatile u32 *)(RALINK_PCI_BASE+(ofs)) = cpu_to_le32(data)
-#define MV_READ(ofs, data) \
- *(data) = le32_to_cpu(*(volatile u32 *)(RALINK_PCI_BASE+(ofs)))
-#define MV_READ_DATA(ofs) \
- le32_to_cpu(*(volatile u32 *)(RALINK_PCI_BASE+(ofs)))
-
-#define MV_WRITE_16(ofs, data) \
- *(volatile u16 *)(RALINK_PCI_BASE+(ofs)) = cpu_to_le16(data)
-#define MV_READ_16(ofs, data) \
- *(data) = le16_to_cpu(*(volatile u16 *)(RALINK_PCI_BASE+(ofs)))
-
-#define MV_WRITE_8(ofs, data) \
- *(volatile u8 *)(RALINK_PCI_BASE+(ofs)) = data
-#define MV_READ_8(ofs, data) \
- *(data) = *(volatile u8 *)(RALINK_PCI_BASE+(ofs))
-
#define RALINK_PCI_MM_MAP_BASE 0x6000
#define RALINK_PCI_IO_MAP_BASE 0x1e16
@@ -173,123 +156,75 @@
#define MEMORY_BASE 0x0
static int pcie_link_status = 0;
-#define PCI_ACCESS_READ_1 0
-#define PCI_ACCESS_READ_2 1
-#define PCI_ACCESS_READ_4 2
-#define PCI_ACCESS_WRITE_1 3
-#define PCI_ACCESS_WRITE_2 4
-#define PCI_ACCESS_WRITE_4 5
+static void __iomem *mt7621_pci_base;
-static int config_access(unsigned char access_type, struct pci_bus *bus,
- unsigned int devfn, unsigned int where, u32 *data)
+static inline u32 mt7621_pci_get_cfgaddr(unsigned int bus, unsigned int slot,
+unsigned int func, unsigned int where)
{
- unsigned int slot = PCI_SLOT(devfn);
- u8 func = PCI_FUNC(devfn);
- uint32_t address_reg, data_reg;
- unsigned int address;
+ return ((bus << 16) | (slot << 11) | (func << 8) | (where & 0xfc) |
+ 0x8000);
+}
+
+static int
+pci_config_read(struct pci_bus *bus, unsigned int devfn,
+ int where, int size, u32 *val)
+{
+ u32 address_reg, data_reg;
+ u32 address;
address_reg = RALINK_PCI_CONFIG_ADDR;
data_reg = RALINK_PCI_CONFIG_DATA_VIRTUAL_REG;
- address = (((where&0xF00)>>8)<<24) |(bus->number << 16) | (slot << 11) |
- (func << 8) | (where & 0xfc) | 0x8000;
- MV_WRITE(address_reg, address);
+ address = (((where & 0xF00) >> 8) << 24) |
+ mt7621_pci_get_cfgaddr(bus->number, PCI_SLOT(devfn),
+ PCI_FUNC(devfn), where);
- switch (access_type) {
- case PCI_ACCESS_WRITE_1:
- MV_WRITE_8(data_reg+(where&0x3), *data);
- break;
- case PCI_ACCESS_WRITE_2:
- MV_WRITE_16(data_reg+(where&0x3), *data);
- break;
- case PCI_ACCESS_WRITE_4:
- MV_WRITE(data_reg, *data);
- break;
- case PCI_ACCESS_READ_1:
- MV_READ_8(data_reg+(where&0x3), data);
- break;
- case PCI_ACCESS_READ_2:
- MV_READ_16(data_reg+(where&0x3), data);
+ writel(address, mt7621_pci_base + address_reg);
+
+ switch (size) {
+ case 1:
+ *val = readb(mt7621_pci_base + data_reg + (where & 0x3));
break;
- case PCI_ACCESS_READ_4:
- MV_READ(data_reg, data);
+ case 2:
+ *val = readw(mt7621_pci_base + data_reg + (where & 0x3));
break;
- default:
- printk("no specify access type\n");
+ case 4:
+ *val = readl(mt7621_pci_base + data_reg);
break;
}
- return 0;
-}
-
-static int
-read_config_byte(struct pci_bus *bus, unsigned int devfn, int where, u8 *val)
-{
- return config_access(PCI_ACCESS_READ_1, bus, devfn, (unsigned
int)where, (u32 *)val);
-}
-
-static int
-read_config_word(struct pci_bus *bus, unsigned int devfn, int where, u16 *val)
-{
- return config_access(PCI_ACCESS_READ_2, bus, devfn, (unsigned
int)where, (u32 *)val);
-}
-
-static int
