Hi Hauke:
Comments are in line.
On Tue, 2012-04-24 at 23:52 +0200, Hauke Mehrtens wrote:
> Hi Nathan,
>
> On 04/11/2012 09:06 AM, Nathan Hintz wrote:
> > In the absence of the attached patch, my device (Linksys E3000) does not
> > consistently boot. Roughly 25% of the time following a power cycle (and
> > occasionally on reboot), it will hang just after completion of pre-init.
> > There is no stack trace when this occurs. I'm guessing it is occurring
> > when the brcm-wl module is loaded. At first I thought there may be some
> > extended config space access of 1 or 2 bytes (instead of always being 4
> > bytes), but when I added logging to detect them, none were ever encountered.
> I have never seen this with my bcm4718 based device. Do you also have
> these problems when not loading brcm-wl, but b43?
I have never tried B43 to date, but will try both of your suggestions
and report back.
>
> > The existing code (prior to the patch) does an "ioremap_nocache" on a
> > subset of a memory region that I think has already been mapped, and then
> > un-maps that smaller region when it is finished. Could the un-mapping of
> > the smaller region affect adversely the mapping of the larger region?
> Yes I also saw that, but I do not know if it is a biǵ problem, but it
> should get fixed.
>
> > Any thoughts on how I might go about isolating the problem further if the
> > un-mapping of the smaller region is not the cause?
> How far do you exactly get? Have yu tried without loading brcm-wl and
> some other modules?
I'll reproduce on a clean build and post console output for the failure.
> >
> > Thanks,
> >
> > Nathan
> >
> > --- /dev/null 2012-04-07 22:03:23.120698218 -0700
> > +++ target/linux/brcm47xx/patches-3.2/240-bcma-pcie-config-access.patch
> > 2012-03-29 23:03:47.966705769 -0700
> > @@ -0,0 +1,104 @@
> > +--- a/drivers/bcma/driver_pci_host.c
> > ++++ b/drivers/bcma/driver_pci_host.c
> > +@@ -98,19 +98,19 @@ static int bcma_extpci_read_config(struc
> > + if (dev == 0) {
> > + /* we support only two functions on device 0 */
> > + if (func > 1)
> > +- return -EINVAL;
> > ++ goto out;
> > +
> > + /* accesses to config registers with offsets >= 256
> > + * requires indirect access.
> > + */
> > + if (off >= PCI_CONFIG_SPACE_SIZE) {
> > + addr = (func << 12);
> > +- addr |= (off & 0x0FFF);
> > ++ addr |= (off & 0x0FFC);
> Why do you set this to 0x0FFC ?
This adds support for 1 and 2 byte access of the extended PCI config
space; although I haven't encountered any to date (and the Broadcom SDK
doesn't support them either). If it is safe to assume all extended PCI
config space accesses will always be 4 bytes (on 4 byte boundaries),
then it can be left at 0xFFF. I didn't see any harm in supporting them
though (with just a change to the mask).
> > + val = bcma_pcie_read_config(pc, addr);
> > + } else {
> > + addr = BCMA_CORE_PCI_PCICFG0;
> > + addr |= (func << 8);
> > +- addr |= (off & 0xfc);
> > ++ addr |= (off & 0xFC);
> > + val = pcicore_read32(pc, addr);
> > + }
> > + } else {
> > +@@ -126,8 +126,6 @@ static int bcma_extpci_read_config(struc
> > + val = 0xffffffff;
> > + goto unmap;
> > + }
> > +-
> > +- val = readl(mmio);
> Does this still work with this readl removed? I think the mmio mem does
> not have a problem here as it is from bcma_get_cfgspace_addr() and that
> is returning host_cfg_addr which is not ioremap.
The "val" has already been read by the "mips_busprobe" called just
prior, so the "readl" is redundant.
> > + }
> > + val >>= (8 * (off & 3));
> > +
> > +@@ -155,7 +153,7 @@ static int bcma_extpci_write_config(stru
> > + const void *buf, int len)
> > + {
> > + int err = -EINVAL;
> > +- u32 addr = 0, val = 0;
> > ++ u32 addr, val;
> > + void __iomem *mmio = 0;
> > + u16 chipid = pc->core->bus->chipinfo.id;
> > +
> > +@@ -163,16 +161,22 @@ static int bcma_extpci_write_config(stru
> > + if (unlikely(len != 1 && len != 2 && len != 4))
> > + goto out;
> > + if (dev == 0) {
> > ++ /* we support only two functions on device 0 */
> > ++ if (func > 1)
> > ++ goto out;
> > ++
> > + /* accesses to config registers with offsets >= 256
> > + * requires indirect access.
> > + */
> > +- if (off < PCI_CONFIG_SPACE_SIZE) {
> > +- addr = pc->core->addr + BCMA_CORE_PCI_PCICFG0;
> > ++ if (off >= PCI_CONFIG_SPACE_SIZE) {
> > ++ addr = (func << 12);
> > ++ addr |= (off & 0x0FFC);
> Why 0x0FFC ?
See previous comment.
> > ++ val = bcma_pcie_read_config(pc, addr);
> > ++ } else {
> > ++ addr = BCMA_CORE_PCI_PCICFG0;
> > + addr |= (func << 8);
> > +- addr |= (off & 0xfc);
> > +- mmio = ioremap_nocache(addr, sizeof(val));
> > +- if (!mmio)
> > +- goto out;
> > ++ addr |= (off & 0xFC);
> Why 0xFC ?
Masking with "0xFC" is consistent with the way "addr" is calculated in
bcma_extpci_read_config (earlier in file), as well as in the Broadcom
SDK. PCI config space access for offsets less than 256 can be 1, 2 or 4
bytes, and the code that follows assumes that we always read/write 4
bytes on a 4 byte boundary.
> > ++ val = pcicore_read32(pc, addr);
> > + }
> > + } else {
> > + addr = bcma_get_cfgspace_addr(pc, dev, func, off);
> This part looks good to me.
> > +@@ -191,12 +195,10 @@ static int bcma_extpci_write_config(stru
> > +
> > + switch (len) {
> > + case 1:
> > +- val = readl(mmio);
> > + val &= ~(0xFF << (8 * (off & 3)));
> > + val |= *((const u8 *)buf) << (8 * (off & 3));
> > + break;
> > + case 2:
> > +- val = readl(mmio);
> > + val &= ~(0xFFFF << (8 * (off & 3)));
> > + val |= *((const u16 *)buf) << (8 * (off & 3));
> > + break;
> > +@@ -204,13 +206,14 @@ static int bcma_extpci_write_config(stru
> > + val = *((const u32 *)buf);
> > + break;
> > + }
> > +- if (dev == 0 && !addr) {
> > ++ if (dev == 0) {
> > + /* accesses to config registers with offsets >= 256
> > + * requires indirect access.
> > + */
> > +- addr = (func << 12);
> > +- addr |= (off & 0x0FFF);
> > +- bcma_pcie_write_config(pc, addr, val);
> > ++ if (off >= PCI_CONFIG_SPACE_SIZE)
> > ++ bcma_pcie_write_config(pc, addr, val);
> > ++ else
> > ++ pcicore_write32(pc, addr, val);
> > + } else {
> > + writel(val, mmio);
> > +
>
> Hauke
Thanks for taking the time to look at this and providing feedback.
Nathan
> _______________________________________________
> openwrt-devel mailing list
> [email protected]
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
_______________________________________________
openwrt-devel mailing list
[email protected]
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
