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@xxxxxxxxx>
> > ---
> > 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, ®s);
> > + if (err) {
> > + dev_err(dev, "missing \"reg\" property\n");
> > + return err;
> > + }
> > +
> > + port->reg_base = devm_pci_remap_cfg_resource(dev, ®s);
> > + 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@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
