[PATCH 0/3] staging: mt7621-pci: avoid custom pci config read and writes

[PATCH 0/3] staging: mt7621-pci: avoid custom pci config read and writes

[Sergio Paracuellos]

This patch series include an attempt to avoid the use of custom
read and writes in driver code and use PCI subsystem common ones.

In order to do this map_bus callback is implemented and also a 
data structure for driver is included. registers base address
is being readed from device tree and the driver gets clean a lot
of code.

I know some commits can be split better, but this series are sent
to get a feedback about if this is the correct way to clean this.

I cannot test this code also, only compile it, so it would be nice 
to know if it runs and what should I check to complete it if not.

Thanks in advance for your time in reviewing this.

Best regards,
    Sergio Paracuellos

Sergio Paracuellos (3):
  staging: mt7621-pci: add data struct for mt7621 pci controller
  staging: mt7621-pci: use generic kernel pci subsystem read and write
  staging: mt7621-pci: remove dead code derived to not use custom reads
    and writes

 drivers/staging/mt7621-pci/pci-mt7621.c | 194 ++++++++++++--------------------
 1 file changed, 72 insertions(+), 122 deletions(-)

-- 
2.7.4

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

[PATCH 1/3] staging: mt7621-pci: add data struct for mt7621 pci controller

[Sergio Paracuellos]

Add new 'mt7621_pcie_port' struct as data for the pci controller.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-pci/pci-mt7621.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index 4f56840..58c77bd 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -177,6 +177,16 @@ static int pcie_link_status = 0;
 #define PCI_ACCESS_WRITE_2 4
 #define PCI_ACCESS_WRITE_4 5
 
+/**
+ * struct mt7621_pcie_port - PCIe port information
+ * @reg_base: IO Mapped Register Base
+ * @dev: Device pointer
+ */
+struct mt7621_pcie_port {
+	void __iomem *reg_base;
+	struct device *dev;
+};
+
 static inline u32 mt7621_pci_get_cfgaddr(unsigned int bus, unsigned int slot,
 					 unsigned int func, unsigned int where)
 {
-- 
2.7.4

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

[PATCH 2/3] staging: mt7621-pci: use generic kernel pci subsystem read and write

[Sergio Paracuellos]

map_bus callback is called before every .read/.write operation.
Implement it and change custom read write operations for the
pci subsystem generics. Make the probe function to assign data
for controller data and get pci register base from device tree.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-pci/pci-mt7621.c | 76 +++++++++++++++++++++++++++++++--
 1 file changed, 72 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index 58c77bd..7bd06a0 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -52,6 +52,9 @@
 #include <linux/delay.h>
 #include <linux/of.h>
 #include <linux/of_pci.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 
 #include <ralink_regs.h>
@@ -306,15 +309,31 @@ pci_config_write(struct pci_bus *bus, unsigned int devfn, int where, int size, u
 	}
 }
 
+static void __iomem *mt7621_pcie_map_bus(struct pci_bus *bus,
+					 unsigned int devfn, int where)
+{
+	struct mt7621_pcie_port *port = bus->sysdata;
+	u32 address = mt7621_pci_get_cfgaddr(bus->number, PCI_SLOT(devfn),
+					     PCI_FUNC(devfn), where);
+	u32 address_reg, data_reg;
+
+	address_reg = RALINK_PCI_CONFIG_ADDR;
+	data_reg = RALINK_PCI_CONFIG_DATA_VIRTUAL_REG;
+
+	writel(address, port->reg_base + address_reg);
+
+	return port->reg_base + data_reg;
+}
+
 struct pci_ops mt7621_pci_ops = {
-	.read		= pci_config_read,
-	.write		= pci_config_write,
+	.map_bus	= mt7621_pcie_map_bus,
+	.read		= pci_generic_config_read32,
+	.write		= pci_generic_config_write32,
 };
 
 static struct resource mt7621_res_pci_mem1;
 static struct resource mt7621_res_pci_io1;
 static struct pci_controller mt7621_controller = {
-	.pci_ops	= &mt7621_pci_ops,
 	.mem_resource	= &mt7621_res_pci_mem1,
 	.io_resource	= &mt7621_res_pci_io1,
 };
@@ -489,10 +508,60 @@ void setup_cm_memory_region(struct resource *mem_resource)
 	}
 }
 
+static int mt7621_pcie_parse_dt(struct mt7621_pcie_port *port)
+{
+	struct device *dev = port->dev;
+	struct device_node *node = dev->of_node;
+	struct resource regs;
+	const char *type;
+	int err;
+
+	type = of_get_property(node, "device_type", NULL);
+	if (!type || strcmp(type, "pci")) {
+		dev_err(dev, "invalid \"device_type\" %s\n", type);
+		return -EINVAL;
+	}
+
+	err = of_address_to_resource(node, 0, &regs);
+	if (err) {
+		dev_err(dev, "missing \"reg\" property\n");
+		return err;
+	}
+
+	port->reg_base = devm_pci_remap_cfg_resource(dev, &regs);
+	if (IS_ERR(port->reg_base))
+		return PTR_ERR(port->reg_base);
+
+	return 0;
+}
+
 static int mt7621_pci_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
+	struct mt7621_pcie_port *port;
+	struct pci_host_bridge *bridge;
+	int err;
 	unsigned long val = 0;
 
+	bridge = devm_pci_alloc_host_bridge(dev, sizeof(*port));
+	if (!bridge)
+		return -ENODEV;
+
+	port = pci_host_bridge_priv(bridge);
+	port->dev = dev;
+
+	err = mt7621_pcie_parse_dt(port);
+	if (err) {
+		dev_err(dev, "Parsing DT failed\n");
+		return err;
+	}
+
+	bridge->dev.parent = dev;
+	bridge->sysdata = port;
+	bridge->ops = &mt7621_pci_ops;
+	mt7621_controller.bus = bridge->bus;
+	mt7621_controller.bus->ops = bridge->ops;
+
 	iomem_resource.start = 0;
 	iomem_resource.end = ~0;
 	ioport_resource.start = 0;
@@ -678,7 +747,6 @@ pcie(2/1/0) link status	pcie2_num	pcie1_num	pcie0_num
 	setup_cm_memory_region(mt7621_controller.mem_resource);
 	register_pci_controller(&mt7621_controller);
 	return 0;
-
 }
 
 int pcibios_plat_dev_init(struct pci_dev *dev)
-- 
2.7.4

[PATCH 3/3] staging: mt7621-pci: remove dead code derived to not use custom reads and writes

[Sergio Paracuellos]

Driver is using now pci subsystem generics reads and writes. Because
of this there is a lot of dead code that can be removed.

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

diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
index 7bd06a0..f9255b0 100644
--- a/drivers/staging/mt7621-pci/pci-mt7621.c
+++ b/drivers/staging/mt7621-pci/pci-mt7621.c
@@ -120,15 +120,6 @@
 	*(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_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		0x60000000
 #define RALINK_PCI_IO_MAP_BASE		0x1e160000
@@ -173,13 +164,6 @@
 #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
-
 /**
  * struct mt7621_pcie_port - PCIe port information
  * @reg_base: IO Mapped Register Base
@@ -197,118 +181,6 @@ static inline u32 mt7621_pci_get_cfgaddr(unsigned int bus, unsigned int slot,
 		(func << 8) | (where & 0xfc) | 0x80000000;
 }
 
-static int config_access(unsigned char access_type, struct pci_bus *bus,
-			unsigned int devfn, unsigned int where, u32 *data)
-{
-	unsigned int slot = PCI_SLOT(devfn);
-	u8 func = PCI_FUNC(devfn);
-	u32 address_reg, data_reg;
-	unsigned int address;
-
-	address_reg = RALINK_PCI_CONFIG_ADDR;
-	data_reg = RALINK_PCI_CONFIG_DATA_VIRTUAL_REG;
-
-	address = mt7621_pci_get_cfgaddr(bus->number, slot, func, where);
-
-	MV_WRITE(address_reg, address);
-
-	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);
-		break;
-	case PCI_ACCESS_READ_4:
-		MV_READ(data_reg, data);
-		break;
-	default:
-		printk("no specify access type\n");
-		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
-read_config_dword(struct pci_bus *bus, unsigned int devfn, int where, u32 *val)
-{
-	return config_access(PCI_ACCESS_READ_4, bus, devfn, (unsigned int)where, (u32 *)val);
-}
-
-static int
-write_config_byte(struct pci_bus *bus, unsigned int devfn, int where, u8 val)
-{
-	if (config_access(PCI_ACCESS_WRITE_1, bus, devfn, (unsigned int)where, (u32 *)&val))
-		return -1;
-
-	return PCIBIOS_SUCCESSFUL;
-}
-
-static int
-write_config_word(struct pci_bus *bus, unsigned int devfn, int where, u16 val)
-{
-	if (config_access(PCI_ACCESS_WRITE_2, bus, devfn, where, (u32 *)&val))
-		return -1;
-
-	return PCIBIOS_SUCCESSFUL;
-}
-
-static int
-write_config_dword(struct pci_bus *bus, unsigned int devfn, int where, u32 val)
-{
-	if (config_access(PCI_ACCESS_WRITE_4, bus, devfn, where, &val))
-		return -1;
-
-	return PCIBIOS_SUCCESSFUL;
-}
-
-static int
-pci_config_read(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *val)
-{
-	switch (size) {
-	case 1:
-		return read_config_byte(bus, devfn, where, (u8 *) val);
-	case 2:
-		return read_config_word(bus, devfn, where, (u16 *) val);
-	default:
-		return read_config_dword(bus, devfn, where, val);
-	}
-}
-
-static int
-pci_config_write(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val)
-{
-	switch (size) {
-	case 1:
-		return write_config_byte(bus, devfn, where, (u8) val);
-	case 2:
-		return write_config_word(bus, devfn, where, (u16) val);
-	default:
-		return write_config_dword(bus, devfn, where, val);
-	}
-}
-
 static void __iomem *mt7621_pcie_map_bus(struct pci_bus *bus,
 					 unsigned int devfn, int where)
 {
-- 
2.7.4

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

Re: [PATCH 2/3] staging: mt7621-pci: use generic kernel pci subsystem read and write

[Dan Carpenter]

On Tue, Jul 10, 2018 at 09:33:47PM +0200, Sergio Paracuellos wrote:
> +static int mt7621_pcie_parse_dt(struct mt7621_pcie_port *port)
> +{
> +	struct device *dev = port->dev;
> +	struct device_node *node = dev->of_node;
> +	struct resource regs;
> +	const char *type;
> +	int err;
> +
> +	type = of_get_property(node, "device_type", NULL);
> +	if (!type || strcmp(type, "pci")) {

Instead of testing for pci, can you test for pcie?

I always like to use the == 0 and != 0 with strcmp() because it's easier
to think about type == pci or type != pcie.

regards,
dan carpenter

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

Re: [PATCH 1/3] staging: mt7621-pci: add data struct for mt7621 pci controller

[Greg KH]

On Tue, Jul 10, 2018 at 09:33:46PM +0200, Sergio Paracuellos wrote:
> Add new 'mt7621_pcie_port' struct as data for the pci controller.
> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  drivers/staging/mt7621-pci/pci-mt7621.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
> index 4f56840..58c77bd 100644
> --- a/drivers/staging/mt7621-pci/pci-mt7621.c
> +++ b/drivers/staging/mt7621-pci/pci-mt7621.c
> @@ -177,6 +177,16 @@ static int pcie_link_status = 0;
>  #define PCI_ACCESS_WRITE_2 4
>  #define PCI_ACCESS_WRITE_4 5
>  
> +/**
> + * struct mt7621_pcie_port - PCIe port information
> + * @reg_base: IO Mapped Register Base
> + * @dev: Device pointer
> + */
> +struct mt7621_pcie_port {
> +	void __iomem *reg_base;
> +	struct device *dev;
> +};

Ok, something is really odd here.

A PCI driver should not have to mess with structures like this and have
to create "fake" struct pci_dev, right?

Or is this code really the PCI bridge code to create PCI devices?  I
can't figure it out.  It has a pcibios override function, which the pci
core calls to set things up, and should never be in a driver, but should
be in pci core code for the platform.  Is that what this is?

What exactly is this code supposed to be doing?

thanks,

greg k-h

Re: [PATCH 2/3] staging: mt7621-pci: use generic kernel pci subsystem read and write

[Greg KH]

On Wed, Jul 11, 2018 at 10:29:43AM +0300, Dan Carpenter wrote:
> On Tue, Jul 10, 2018 at 09:33:47PM +0200, Sergio Paracuellos wrote:
> > +static int mt7621_pcie_parse_dt(struct mt7621_pcie_port *port)
> > +{
> > +	struct device *dev = port->dev;
> > +	struct device_node *node = dev->of_node;
> > +	struct resource regs;
> > +	const char *type;
> > +	int err;
> > +
> > +	type = of_get_property(node, "device_type", NULL);
> > +	if (!type || strcmp(type, "pci")) {
> 
> Instead of testing for pci, can you test for pcie?

Does this platform support pcie?

thanks,

greg k-h

Re: [PATCH 2/3] staging: mt7621-pci: use generic kernel pci subsystem read and write

[Greg KH]

On Tue, Jul 10, 2018 at 09:33:47PM +0200, Sergio Paracuellos wrote:
> map_bus callback is called before every .read/.write operation.
> Implement it and change custom read write operations for the
> pci subsystem generics. Make the probe function to assign data
> for controller data and get pci register base from device tree.

Ok, now this makes a bit more sense, I should have read this before
reviewing the first patch, sorry.

You have tested this out, right?  If it works, great, I'll gladly take
this series then, it starts to make the platform a bit more sane, nice
work.

thanks,

greg k-h

Re: [PATCH 2/3] staging: mt7621-pci: use generic kernel pci subsystem read and write

[NeilBrown]

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

On Wed, Jul 11 2018, Greg KH wrote:

> On Wed, Jul 11, 2018 at 10:29:43AM +0300, Dan Carpenter wrote:
>> On Tue, Jul 10, 2018 at 09:33:47PM +0200, Sergio Paracuellos wrote:
>> > +static int mt7621_pcie_parse_dt(struct mt7621_pcie_port *port)
>> > +{
>> > +	struct device *dev = port->dev;
>> > +	struct device_node *node = dev->of_node;
>> > +	struct resource regs;
>> > +	const char *type;
>> > +	int err;
>> > +
>> > +	type = of_get_property(node, "device_type", NULL);
>> > +	if (!type || strcmp(type, "pci")) {
>> 
>> Instead of testing for pci, can you test for pcie?
>
> Does this platform support pcie?

Yes, the platform has 3 pcie ports.

I hope to test these patches by Saturday morning (GMT+10).  I'll report
any problems, or success.

Thanks,
NeilBrown

[-- 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 2/3] staging: mt7621-pci: use generic kernel pci subsystem read and write

[Sergio Paracuellos]

On Wed, Jul 11, 2018 at 07:51:05PM +1000, NeilBrown wrote:
> On Wed, Jul 11 2018, Greg KH wrote:
> 
> > On Wed, Jul 11, 2018 at 10:29:43AM +0300, Dan Carpenter wrote:
> >> On Tue, Jul 10, 2018 at 09:33:47PM +0200, Sergio Paracuellos wrote:
> >> > +static int mt7621_pcie_parse_dt(struct mt7621_pcie_port *port)
> >> > +{
> >> > +	struct device *dev = port->dev;
> >> > +	struct device_node *node = dev->of_node;
> >> > +	struct resource regs;
> >> > +	const char *type;
> >> > +	int err;
> >> > +
> >> > +	type = of_get_property(node, "device_type", NULL);
> >> > +	if (!type || strcmp(type, "pci")) {
> >> 
> >> Instead of testing for pci, can you test for pcie?
> >
> > Does this platform support pcie?
> 
> Yes, the platform has 3 pcie ports.
> 
> I hope to test these patches by Saturday morning (GMT+10).  I'll report
> any problems, or success.

Thanks, Neil. That would be awesome.
> 
> Thanks,
> NeilBrown

Best regards,
    Sergio Paracuellos


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

Re: [PATCH 2/3] staging: mt7621-pci: use generic kernel pci subsystem read and write

[Sergio Paracuellos]

On Wed, Jul 11, 2018 at 11:16:14AM +0200, Greg KH wrote:
> On Tue, Jul 10, 2018 at 09:33:47PM +0200, Sergio Paracuellos wrote:
> > map_bus callback is called before every .read/.write operation.
> > Implement it and change custom read write operations for the
> > pci subsystem generics. Make the probe function to assign data
> > for controller data and get pci register base from device tree.
> 
> Ok, now this makes a bit more sense, I should have read this before
> reviewing the first patch, sorry.

I though in send only two patches one introducing all (including the 
data struct) and the second removing dead code to avoid confusion but 
I ended up in this three. Sorry.
> 
> You have tested this out, right?  If it works, great, I'll gladly take
> this series then, it starts to make the platform a bit more sane, nice
> work.

No, I don't have the hardware to test this :-(. Let's wait for the Neil's
feedback then.

Good to hear that this is the correct way to start the cleaning properly,
thanks. 

> 
> thanks,
> 
> greg k-h

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

Re: [PATCH 2/3] staging: mt7621-pci: use generic kernel pci subsystem read and write

[Sergio Paracuellos]

On Wed, Jul 11, 2018 at 10:29:43AM +0300, Dan Carpenter wrote:
> On Tue, Jul 10, 2018 at 09:33:47PM +0200, Sergio Paracuellos wrote:
> > +static int mt7621_pcie_parse_dt(struct mt7621_pcie_port *port)
> > +{
> > +	struct device *dev = port->dev;
> > +	struct device_node *node = dev->of_node;
> > +	struct resource regs;
> > +	const char *type;
> > +	int err;
> > +
> > +	type = of_get_property(node, "device_type", NULL);
> > +	if (!type || strcmp(type, "pci")) {
> 
> Instead of testing for pci, can you test for pcie?

All device tree's I've checked looking for what this should be done
has "pci" as device_type defined and checked accordly in code (current
mt7621 DTS also). That's why "pci" is checked.

> 
> I always like to use the == 0 and != 0 with strcmp() because it's easier
> to think about type == pci or type != pcie.

Thanks for the advice, maybe you are right and is more clear in the way
you are describing here.

> 
> regards,
> dan carpenter
> 

Best regards,
    Sergio Paracuellos

Re: [PATCH 2/3] staging: mt7621-pci: use generic kernel pci subsystem read and write

[NeilBrown]

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

On Tue, Jul 10 2018, Sergio Paracuellos wrote:

> map_bus callback is called before every .read/.write operation.
> Implement it and change custom read write operations for the
> pci subsystem generics. Make the probe function to assign data
> for controller data and get pci register base from device tree.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  drivers/staging/mt7621-pci/pci-mt7621.c | 76 +++++++++++++++++++++++++++++++--
>  1 file changed, 72 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
> index 58c77bd..7bd06a0 100644
> --- a/drivers/staging/mt7621-pci/pci-mt7621.c
> +++ b/drivers/staging/mt7621-pci/pci-mt7621.c
> @@ -52,6 +52,9 @@
>  #include <linux/delay.h>
>  #include <linux/of.h>
>  #include <linux/of_pci.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
>  #include <linux/platform_device.h>
>  
>  #include <ralink_regs.h>
> @@ -306,15 +309,31 @@ pci_config_write(struct pci_bus *bus, unsigned int devfn, int where, int size, u
>  	}
>  }
>  
> +static void __iomem *mt7621_pcie_map_bus(struct pci_bus *bus,
> +					 unsigned int devfn, int where)
> +{
> +	struct mt7621_pcie_port *port = bus->sysdata;
> +	u32 address = mt7621_pci_get_cfgaddr(bus->number, PCI_SLOT(devfn),
> +					     PCI_FUNC(devfn), where);
> +	u32 address_reg, data_reg;
> +
> +	address_reg = RALINK_PCI_CONFIG_ADDR;
> +	data_reg = RALINK_PCI_CONFIG_DATA_VIRTUAL_REG;
> +
> +	writel(address, port->reg_base + address_reg);
> +
> +	return port->reg_base + data_reg;
> +}
> +
>  struct pci_ops mt7621_pci_ops = {
> -	.read		= pci_config_read,
> -	.write		= pci_config_write,
> +	.map_bus	= mt7621_pcie_map_bus,
> +	.read		= pci_generic_config_read32,
> +	.write		= pci_generic_config_write32,
>  };
>  
>  static struct resource mt7621_res_pci_mem1;
>  static struct resource mt7621_res_pci_io1;
>  static struct pci_controller mt7621_controller = {
> -	.pci_ops	= &mt7621_pci_ops,
>  	.mem_resource	= &mt7621_res_pci_mem1,
>  	.io_resource	= &mt7621_res_pci_io1,
>  };
> @@ -489,10 +508,60 @@ void setup_cm_memory_region(struct resource *mem_resource)
>  	}
>  }
>  
> +static int mt7621_pcie_parse_dt(struct mt7621_pcie_port *port)
> +{
> +	struct device *dev = port->dev;
> +	struct device_node *node = dev->of_node;
> +	struct resource regs;
> +	const char *type;
> +	int err;
> +
> +	type = of_get_property(node, "device_type", NULL);
> +	if (!type || strcmp(type, "pci")) {
> +		dev_err(dev, "invalid \"device_type\" %s\n", type);
> +		return -EINVAL;
> +	}
> +
> +	err = of_address_to_resource(node, 0, &regs);
> +	if (err) {
> +		dev_err(dev, "missing \"reg\" property\n");
> +		return err;
> +	}
> +
> +	port->reg_base = devm_pci_remap_cfg_resource(dev, &regs);
> +	if (IS_ERR(port->reg_base))
> +		return PTR_ERR(port->reg_base);
> +
> +	return 0;
> +}
> +
>  static int mt7621_pci_probe(struct platform_device *pdev)
>  {
> +	struct device *dev = &pdev->dev;
> +	struct mt7621_pcie_port *port;
> +	struct pci_host_bridge *bridge;
> +	int err;
>  	unsigned long val = 0;
>  
> +	bridge = devm_pci_alloc_host_bridge(dev, sizeof(*port));
> +	if (!bridge)
> +		return -ENODEV;
> +
> +	port = pci_host_bridge_priv(bridge);
> +	port->dev = dev;
> +
> +	err = mt7621_pcie_parse_dt(port);
> +	if (err) {
> +		dev_err(dev, "Parsing DT failed\n");
> +		return err;
> +	}
> +
> +	bridge->dev.parent = dev;
> +	bridge->sysdata = port;
> +	bridge->ops = &mt7621_pci_ops;
> +	mt7621_controller.bus = bridge->bus;

bridge->bus hasn't been initialized yet, so it is NULL.

> +	mt7621_controller.bus->ops = bridge->ops;

So this is a NULL-pointer deref.

Maybe you need to call  pci_host_probe() somewhere? or
pci_scan_root_bus_bridge()?
Inserting either of those calls just after setting bridge->ops above
results in a hang, so something more subtle must been needed.

Thanks,
NeilBrown

[-- 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 2/3] staging: mt7621-pci: use generic kernel pci subsystem read and write

[Sergio Paracuellos]

On Sat, Jul 14, 2018 at 10:04:32AM +1000, NeilBrown wrote:
> On Tue, Jul 10 2018, Sergio Paracuellos wrote:
> 
> > map_bus callback is called before every .read/.write operation.
> > Implement it and change custom read write operations for the
> > pci subsystem generics. Make the probe function to assign data
> > for controller data and get pci register base from device tree.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > ---
> >  drivers/staging/mt7621-pci/pci-mt7621.c | 76 +++++++++++++++++++++++++++++++--
> >  1 file changed, 72 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
> > index 58c77bd..7bd06a0 100644
> > --- a/drivers/staging/mt7621-pci/pci-mt7621.c
> > +++ b/drivers/staging/mt7621-pci/pci-mt7621.c
> > @@ -52,6 +52,9 @@
> >  #include <linux/delay.h>
> >  #include <linux/of.h>
> >  #include <linux/of_pci.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> >  #include <linux/platform_device.h>
> >  
> >  #include <ralink_regs.h>
> > @@ -306,15 +309,31 @@ pci_config_write(struct pci_bus *bus, unsigned int devfn, int where, int size, u
> >  	}
> >  }
> >  
> > +static void __iomem *mt7621_pcie_map_bus(struct pci_bus *bus,
> > +					 unsigned int devfn, int where)
> > +{
> > +	struct mt7621_pcie_port *port = bus->sysdata;
> > +	u32 address = mt7621_pci_get_cfgaddr(bus->number, PCI_SLOT(devfn),
> > +					     PCI_FUNC(devfn), where);
> > +	u32 address_reg, data_reg;
> > +
> > +	address_reg = RALINK_PCI_CONFIG_ADDR;
> > +	data_reg = RALINK_PCI_CONFIG_DATA_VIRTUAL_REG;
> > +
> > +	writel(address, port->reg_base + address_reg);
> > +
> > +	return port->reg_base + data_reg;
> > +}
> > +
> >  struct pci_ops mt7621_pci_ops = {
> > -	.read		= pci_config_read,
> > -	.write		= pci_config_write,
> > +	.map_bus	= mt7621_pcie_map_bus,
> > +	.read		= pci_generic_config_read32,
> > +	.write		= pci_generic_config_write32,
> >  };
> >  
> >  static struct resource mt7621_res_pci_mem1;
> >  static struct resource mt7621_res_pci_io1;
> >  static struct pci_controller mt7621_controller = {
> > -	.pci_ops	= &mt7621_pci_ops,
> >  	.mem_resource	= &mt7621_res_pci_mem1,
> >  	.io_resource	= &mt7621_res_pci_io1,
> >  };
> > @@ -489,10 +508,60 @@ void setup_cm_memory_region(struct resource *mem_resource)
> >  	}
> >  }
> >  
> > +static int mt7621_pcie_parse_dt(struct mt7621_pcie_port *port)
> > +{
> > +	struct device *dev = port->dev;
> > +	struct device_node *node = dev->of_node;
> > +	struct resource regs;
> > +	const char *type;
> > +	int err;
> > +
> > +	type = of_get_property(node, "device_type", NULL);
> > +	if (!type || strcmp(type, "pci")) {
> > +		dev_err(dev, "invalid \"device_type\" %s\n", type);
> > +		return -EINVAL;
> > +	}
> > +
> > +	err = of_address_to_resource(node, 0, &regs);
> > +	if (err) {
> > +		dev_err(dev, "missing \"reg\" property\n");
> > +		return err;
> > +	}
> > +
> > +	port->reg_base = devm_pci_remap_cfg_resource(dev, &regs);
> > +	if (IS_ERR(port->reg_base))
> > +		return PTR_ERR(port->reg_base);
> > +
> > +	return 0;
> > +}
> > +
> >  static int mt7621_pci_probe(struct platform_device *pdev)
> >  {
> > +	struct device *dev = &pdev->dev;
> > +	struct mt7621_pcie_port *port;
> > +	struct pci_host_bridge *bridge;
> > +	int err;
> >  	unsigned long val = 0;
> >  
> > +	bridge = devm_pci_alloc_host_bridge(dev, sizeof(*port));
> > +	if (!bridge)
> > +		return -ENODEV;
> > +
> > +	port = pci_host_bridge_priv(bridge);
> > +	port->dev = dev;
> > +
> > +	err = mt7621_pcie_parse_dt(port);
> > +	if (err) {
> > +		dev_err(dev, "Parsing DT failed\n");
> > +		return err;
> > +	}
> > +
> > +	bridge->dev.parent = dev;
> > +	bridge->sysdata = port;
> > +	bridge->ops = &mt7621_pci_ops;
> > +	mt7621_controller.bus = bridge->bus;
> 
> bridge->bus hasn't been initialized yet, so it is NULL.
> 
> > +	mt7621_controller.bus->ops = bridge->ops;
> 
> So this is a NULL-pointer deref.
> 
> Maybe you need to call  pci_host_probe() somewhere? or
> pci_scan_root_bus_bridge()?
> Inserting either of those calls just after setting bridge->ops above
> results in a hang, so something more subtle must been needed.

Thanks for your time and feedback, Neil.

pci_scan_root_bus_bridge should be called, yes. I think we should just
avoid the actual pci_controller_register legacy stuff and call pci_bus_add_devices()
at the end of the probe function after. I'll give this a new try this afternoon and see
what happends. Hope to send new series during this weekend.

> 
> Thanks,
> NeilBrown

Best regards,
    Sergio Paracuellos


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