Re: [PATCH v6 00/15] staging: mt7621-pci: avoid custom pci config read and writes

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

 



On Tue, Jul 31, 2018 at 03:25:43PM +1000, NeilBrown wrote:
> On Tue, Jul 31 2018, Sergio Paracuellos wrote:
> 
> > On Tue, Jul 31, 2018 at 08:55:52AM +1000, NeilBrown wrote:
> >> On Mon, Jul 30 2018, Sergio Paracuellos wrote:
> >> 
> >> > 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
> >> > data structures for driver are included. The regs base address
> >> > is being readed from device tree and the driver gets clean a lot
> >> > of code.
> >> >
> >> > Also the driver get removes all legacy PCI code using now PCI_DRIVERS_GENERIC.
> >> >
> >> > Changes in v6:
> >> >     - Reorder patches to be each patch correct in itself.
> >> >     - PATCH 1 adds also Kconfig to do the step from legacy to generic code
> >> >     - PATCH 1 remaps io space using devm_pci_remap_iospace for io resource in 
> >> >       a new function called 'mt7621_pci_parse_request_of_pci_ranges'.
> >> >     - Other patches rebased and adapted with this changes.
> >> 
> >> No noticeable difference.
> >> Still hangs after
> >> [    8.630000] ahci 0000:01:00.0: enabling device (0000 -> 0002)
> >> 
> >> the readl() at the start of ahci_enable_ahci() hangs, reading c4017004.
> >> 
> >> I built on a merge of
> >>  Merge: 527838d470e3 b9f13084580c
> >> 
> >> linus' master + staging/staging-testing
> >> 
> >> dmesg below.
> >
> > Thanks for this.
> >
> ....
> >> [    8.430000] bootconsole [early0] disabled
> >> [    8.430000] bootconsole [early0] disabled
> >> [    8.450000] cacheinfo: Failed to find cpu0 device node
> >> [    8.460000] cacheinfo: Unable to detect cache hierarchy for CPU 0
> >> [    8.550000] loop: module loaded
> >> [    8.560000] ahci 0000:01:00.0: enabling device (0000 -> 0002)
> >> 
> >> 
> >
> > So this last dmesg is the new one with last v6, right? Because I can see 
> 
> Right.
> 
> 
> > that at least now the assigned resources are pretty much the same of the ones in the dmesg of the
> > original code talking in terms of assigned BAR's and bridge windows. No weird BAR 9 anymore, which I think is 
> > good. We can try to get back the hack which I removed at first and see what happend. The hack is
> > this function removed in PATCH 2:
> >
> > -void setup_cm_memory_region(struct resource *mem_resource)
> > -{
> > -	resource_size_t mask;
> > -	if (mips_cps_numiocu(0)) {
> > -		/* FIXME: hardware doesn't accept mask values with 1s after
> > -		 * 0s (e.g. 0xffef), so it would be great to warn if that's
> > -		 * about to happen */
> > -		mask = ~(mem_resource->end - mem_resource->start);
> > -
> > -		write_gcr_reg1_base(mem_resource->start);
> > -		write_gcr_reg1_mask(mask | CM_GCR_REGn_MASK_CMTGT_IOCU0);
> > -		printk("PCI coherence region base: 0x%08llx, mask/settings: 0x%08llx\n",
> > -			(unsigned long long)read_gcr_reg1_base(),
> > -			(unsigned long long)read_gcr_reg1_mask());
> > -	}
> > -}
> >
> > We can try to add it again and and call it from 'mt7621_pci_parse_request_of_pci_ranges'
> > in the label of the case of the case 'IORESOURCE_MEM' (as you can see this was intentionally
> > without code just to test this if this v6 fails):
> >
> > 		break;
> > 		case IORESOURCE_MEM:
> > +           setup_cm_memory_region(res);
> > 			break;
> 
> I added setup_cm_memory_region() back in and called it from
> mt7621_pci_parse_request_of_pci_ranges()
> as suggested.
> Now we don't hang at the same place, but crash shortly after.
> 
> [    8.750000] WARNING: CPU: 2 PID: 1 at ../drivers/ata/libahci.c:227 ahci_save_initial_config+0x3c/0x3e0
> 
> This is at the end of ahci_enable_ahci(). the HOST_AHCI_EN bit never was
> set.

So it seems this hack is really needed. Does something changed in dmesg with this?

> 
> 
> >
> > If this also fails we can try to move the call to 'mt7621_pcie_parse_dt(pcie, &res);' after the
> > HW initialization code just before the list_splice_init(&res, &bridge->windows); line:
> >
> > -   err = mt7621_pcie_parse_dt(pcie, &res);
> > -	if (err) {
> > -		dev_err(dev, "Parsing DT failed\n");
> > -		return err;
> > -	}
> > -
> > -	/*
> >  	iomem_resource.start = 0;
> >  	iomem_resource.end = ~0;
> > ...
> >
> >         write_config(0, 0, 0, 0x70c, val);
> >  	}
> >
> > +   err = mt7621_pcie_parse_dt(pcie, &res);
> > +	if (err) {
> > +		dev_err(dev, "Parsing DT failed\n");
> > +		return err;
> > +	}
> > +
> >     list_splice_init(&res, &bridge->windows);
> 
> If I move the call to mt7621_pcie_parse_dt() anywhere after
> the call to set_phy_for_ssc() I get a crash at the start of
> set_pcie_phy() called from set_phy_for_ssc(), presumably because
> pcie->base isn't set.
> 
> Keep trying, I'm sure we'll get there.

True, sorry. Maybe we can get out the mt7621_pci_parse_request_of_pci_ranges
call from mt7621_pcie_parse_dt and put it after the initialization code
just before the list_splice_init to try to reproduce the original code trace.

I never give up :). I only concern if I am not doing you to waste your time
with this tests. I really like to be able to test this by myself but this
board gets its price really increase buying it from Spain :-(.

> 
> NeilBrown

Best regards,
    Sergio Paracuellos

> 
> >
> > (I don't have access now to the laptop with the code and cannot diff properly, sorry).
> >
> > Hopefully it boots now?
> >
> > Thanks in advance.
> >
> > Best regards,
> >     Sergio Paracuellos


_______________________________________________
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