[RFC][PATCH] ssb: separate common scanning functions

[RFC][PATCH] ssb: separate common scanning functions

[Rafał Miłecki]

Signed-off-by: Rafa? Mi?ecki <zajec5@gmail.com>
---
To keep ssb clean and avoid bigger code duplications, my idea is to:
1) Rename ssb directory to bcmb (like Broadcom's bus)
2) Have two *separated* drivers in it: ssb and ai
3) Share common function between ssb and ai
Example of such a common functions can be translating enum to *char and
(re)mapping MMIO.
---
 drivers/ssb/Makefile      |    2 +-
 drivers/ssb/bcmb_scan.c   |  182 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/ssb/bcmb_scan.h   |   13 +++
 drivers/ssb/scan.c        |  163 ----------------------------------------
 drivers/ssb/ssb_private.h |    3 +-
 5 files changed, 197 insertions(+), 166 deletions(-)
 create mode 100644 drivers/ssb/bcmb_scan.c
 create mode 100644 drivers/ssb/bcmb_scan.h

diff --git a/drivers/ssb/Makefile b/drivers/ssb/Makefile
index 656e58b..a91045e 100644
--- a/drivers/ssb/Makefile
+++ b/drivers/ssb/Makefile
@@ -1,5 +1,5 @@
 # core
-ssb-y					+= main.o scan.o
+ssb-y					+= main.o bcmb_scan.o scan.o
 ssb-$(CONFIG_SSB_EMBEDDED)		+= embedded.o
 ssb-$(CONFIG_SSB_SPROM)			+= sprom.o
 
diff --git a/drivers/ssb/bcmb_scan.c b/drivers/ssb/bcmb_scan.c
new file mode 100644
index 0000000..17b93f7
--- /dev/null
+++ b/drivers/ssb/bcmb_scan.c
@@ -0,0 +1,182 @@
+/*
+ * Sonics Silicon Backplane
+ * Bus scanning
+ *
+ * Copyright (C) 2005-2007 Michael Buesch <mb@bu3sch.de>
+ * Copyright (C) 2005 Martin Langer <martin-langer@gmx.de>
+ * Copyright (C) 2005 Stefano Brivio <st3@riseup.net>
+ * Copyright (C) 2005 Danny van Dyk <kugelfang@gentoo.org>
+ * Copyright (C) 2005 Andreas Jaggi <andreas.jaggi@waterwave.ch>
+ * Copyright (C) 2006 Broadcom Corporation.
+ *
+ * Licensed under the GNU/GPL. See COPYING for details.
+ */
+
+#include <linux/ssb/ssb.h>
+#include <linux/pci.h>
+#include <linux/io.h>
+
+#include "bcmb_scan.h"
+
+#include "ssb_private.h"
+
+const char *ssb_core_name(u16 coreid)
+{
+	switch (coreid) {
+	case SSB_DEV_CHIPCOMMON:
+		return "ChipCommon";
+	case SSB_DEV_ILINE20:
+		return "ILine 20";
+	case SSB_DEV_SDRAM:
+		return "SDRAM";
+	case SSB_DEV_PCI:
+		return "PCI";
+	case SSB_DEV_MIPS:
+		return "MIPS";
+	case SSB_DEV_ETHERNET:
+		return "Fast Ethernet";
+	case SSB_DEV_V90:
+		return "V90";
+	case SSB_DEV_USB11_HOSTDEV:
+		return "USB 1.1 Hostdev";
+	case SSB_DEV_ADSL:
+		return "ADSL";
+	case SSB_DEV_ILINE100:
+		return "ILine 100";
+	case SSB_DEV_IPSEC:
+		return "IPSEC";
+	case SSB_DEV_PCMCIA:
+		return "PCMCIA";
+	case SSB_DEV_INTERNAL_MEM:
+		return "Internal Memory";
+	case SSB_DEV_MEMC_SDRAM:
+		return "MEMC SDRAM";
+	case SSB_DEV_EXTIF:
+		return "EXTIF";
+	case SSB_DEV_80211:
+		return "IEEE 802.11";
+	case SSB_DEV_MIPS_3302:
+		return "MIPS 3302";
+	case SSB_DEV_USB11_HOST:
+		return "USB 1.1 Host";
+	case SSB_DEV_USB11_DEV:
+		return "USB 1.1 Device";
+	case SSB_DEV_USB20_HOST:
+		return "USB 2.0 Host";
+	case SSB_DEV_USB20_DEV:
+		return "USB 2.0 Device";
+	case SSB_DEV_SDIO_HOST:
+		return "SDIO Host";
+	case SSB_DEV_ROBOSWITCH:
+		return "Roboswitch";
+	case SSB_DEV_PARA_ATA:
+		return "PATA";
+	case SSB_DEV_SATA_XORDMA:
+		return "SATA XOR-DMA";
+	case SSB_DEV_ETHERNET_GBIT:
+		return "GBit Ethernet";
+	case SSB_DEV_PCIE:
+		return "PCI-E";
+	case SSB_DEV_MIMO_PHY:
+		return "MIMO PHY";
+	case SSB_DEV_SRAM_CTRLR:
+		return "SRAM Controller";
+	case SSB_DEV_MINI_MACPHY:
+		return "Mini MACPHY";
+	case SSB_DEV_ARM_1176:
+		return "ARM 1176";
+	case SSB_DEV_ARM_7TDMI:
+		return "ARM 7TDMI";
+	}
+	return "UNKNOWN";
+}
+
+u32 scan_read32(struct ssb_bus *bus, u8 current_coreidx,
+		       u16 offset)
+{
+	u32 lo, hi;
+
+	switch (bus->bustype) {
+	case SSB_BUSTYPE_SSB:
+		offset += current_coreidx * SSB_CORE_SIZE;
+		break;
+	case SSB_BUSTYPE_PCI:
+		break;
+	case SSB_BUSTYPE_PCMCIA:
+		if (offset >= 0x800) {
+			ssb_pcmcia_switch_segment(bus, 1);
+			offset -= 0x800;
+		} else
+			ssb_pcmcia_switch_segment(bus, 0);
+		lo = readw(bus->mmio + offset);
+		hi = readw(bus->mmio + offset + 2);
+		return lo | (hi << 16);
+	case SSB_BUSTYPE_SDIO:
+		offset += current_coreidx * SSB_CORE_SIZE;
+		return ssb_sdio_scan_read32(bus, offset);
+	}
+	return readl(bus->mmio + offset);
+}
+
+int scan_switchcore(struct ssb_bus *bus, u8 coreidx)
+{
+	switch (bus->bustype) {
+	case SSB_BUSTYPE_SSB:
+		break;
+	case SSB_BUSTYPE_PCI:
+		return ssb_pci_switch_coreidx(bus, coreidx);
+	case SSB_BUSTYPE_PCMCIA:
+		return ssb_pcmcia_switch_coreidx(bus, coreidx);
+	case SSB_BUSTYPE_SDIO:
+		return ssb_sdio_scan_switch_coreidx(bus, coreidx);
+	}
+	return 0;
+}
+
+void ssb_iounmap(struct ssb_bus *bus)
+{
+	switch (bus->bustype) {
+	case SSB_BUSTYPE_SSB:
+	case SSB_BUSTYPE_PCMCIA:
+		iounmap(bus->mmio);
+		break;
+	case SSB_BUSTYPE_PCI:
+#ifdef CONFIG_SSB_PCIHOST
+		pci_iounmap(bus->host_pci, bus->mmio);
+#else
+		SSB_BUG_ON(1); /* Can't reach this code. */
+#endif
+		break;
+	case SSB_BUSTYPE_SDIO:
+		break;
+	}
+	bus->mmio = NULL;
+	bus->mapped_device = NULL;
+}
+
+void __iomem *ssb_ioremap(struct ssb_bus *bus, unsigned long baseaddr)
+{
+	void __iomem *mmio = NULL;
+
+	switch (bus->bustype) {
+	case SSB_BUSTYPE_SSB:
+		/* Only map the first core for now. */
+		/* fallthrough... */
+	case SSB_BUSTYPE_PCMCIA:
+		mmio = ioremap(baseaddr, SSB_CORE_SIZE);
+		break;
+	case SSB_BUSTYPE_PCI:
+#ifdef CONFIG_SSB_PCIHOST
+		mmio = pci_iomap(bus->host_pci, 0, ~0UL);
+#else
+		SSB_BUG_ON(1); /* Can't reach this code. */
+#endif
+		break;
+	case SSB_BUSTYPE_SDIO:
+		/* Nothing to ioremap in the SDIO case, just fake it */
+		mmio = (void __iomem *)baseaddr;
+		break;
+	}
+
+	return mmio;
+}
diff --git a/drivers/ssb/bcmb_scan.h b/drivers/ssb/bcmb_scan.h
new file mode 100644
index 0000000..f1fd16c
--- /dev/null
+++ b/drivers/ssb/bcmb_scan.h
@@ -0,0 +1,13 @@
+#ifndef BCMB_SCAN_H_
+#define BCMB_SCAN_H_
+
+extern const char *ssb_core_name(u16 coreid);
+
+extern u32 scan_read32(struct ssb_bus *bus, u8 current_coreidx,
+			    u16 offset);
+extern int scan_switchcore(struct ssb_bus *bus, u8 coreidx);
+
+extern void ssb_iounmap(struct ssb_bus *bus);
+extern void __iomem *ssb_ioremap(struct ssb_bus *bus, unsigned long baseaddr);
+
+#endif
diff --git a/drivers/ssb/scan.c b/drivers/ssb/scan.c
index 29884c0..0087795 100644
--- a/drivers/ssb/scan.c
+++ b/drivers/ssb/scan.c
@@ -22,78 +22,6 @@
 
 #include "ssb_private.h"
 
-
-const char *ssb_core_name(u16 coreid)
-{
-	switch (coreid) {
-	case SSB_DEV_CHIPCOMMON:
-		return "ChipCommon";
-	case SSB_DEV_ILINE20:
-		return "ILine 20";
-	case SSB_DEV_SDRAM:
-		return "SDRAM";
-	case SSB_DEV_PCI:
-		return "PCI";
-	case SSB_DEV_MIPS:
-		return "MIPS";
-	case SSB_DEV_ETHERNET:
-		return "Fast Ethernet";
-	case SSB_DEV_V90:
-		return "V90";
-	case SSB_DEV_USB11_HOSTDEV:
-		return "USB 1.1 Hostdev";
-	case SSB_DEV_ADSL:
-		return "ADSL";
-	case SSB_DEV_ILINE100:
-		return "ILine 100";
-	case SSB_DEV_IPSEC:
-		return "IPSEC";
-	case SSB_DEV_PCMCIA:
-		return "PCMCIA";
-	case SSB_DEV_INTERNAL_MEM:
-		return "Internal Memory";
-	case SSB_DEV_MEMC_SDRAM:
-		return "MEMC SDRAM";
-	case SSB_DEV_EXTIF:
-		return "EXTIF";
-	case SSB_DEV_80211:
-		return "IEEE 802.11";
-	case SSB_DEV_MIPS_3302:
-		return "MIPS 3302";
-	case SSB_DEV_USB11_HOST:
-		return "USB 1.1 Host";
-	case SSB_DEV_USB11_DEV:
-		return "USB 1.1 Device";
-	case SSB_DEV_USB20_HOST:
-		return "USB 2.0 Host";
-	case SSB_DEV_USB20_DEV:
-		return "USB 2.0 Device";
-	case SSB_DEV_SDIO_HOST:
-		return "SDIO Host";
-	case SSB_DEV_ROBOSWITCH:
-		return "Roboswitch";
-	case SSB_DEV_PARA_ATA:
-		return "PATA";
-	case SSB_DEV_SATA_XORDMA:
-		return "SATA XOR-DMA";
-	case SSB_DEV_ETHERNET_GBIT:
-		return "GBit Ethernet";
-	case SSB_DEV_PCIE:
-		return "PCI-E";
-	case SSB_DEV_MIMO_PHY:
-		return "MIMO PHY";
-	case SSB_DEV_SRAM_CTRLR:
-		return "SRAM Controller";
-	case SSB_DEV_MINI_MACPHY:
-		return "Mini MACPHY";
-	case SSB_DEV_ARM_1176:
-		return "ARM 1176";
-	case SSB_DEV_ARM_7TDMI:
-		return "ARM 7TDMI";
-	}
-	return "UNKNOWN";
-}
-
 static u16 pcidev_to_chipid(struct pci_dev *pci_dev)
 {
 	u16 chipid_fallback = 0;
@@ -157,97 +85,6 @@ static u8 chipid_to_nrcores(u16 chipid)
 	return 1;
 }
 
-static u32 scan_read32(struct ssb_bus *bus, u8 current_coreidx,
-		       u16 offset)
-{
-	u32 lo, hi;
-
-	switch (bus->bustype) {
-	case SSB_BUSTYPE_SSB:
-		offset += current_coreidx * SSB_CORE_SIZE;
-		break;
-	case SSB_BUSTYPE_PCI:
-		break;
-	case SSB_BUSTYPE_PCMCIA:
-		if (offset >= 0x800) {
-			ssb_pcmcia_switch_segment(bus, 1);
-			offset -= 0x800;
-		} else
-			ssb_pcmcia_switch_segment(bus, 0);
-		lo = readw(bus->mmio + offset);
-		hi = readw(bus->mmio + offset + 2);
-		return lo | (hi << 16);
-	case SSB_BUSTYPE_SDIO:
-		offset += current_coreidx * SSB_CORE_SIZE;
-		return ssb_sdio_scan_read32(bus, offset);
-	}
-	return readl(bus->mmio + offset);
-}
-
-static int scan_switchcore(struct ssb_bus *bus, u8 coreidx)
-{
-	switch (bus->bustype) {
-	case SSB_BUSTYPE_SSB:
-		break;
-	case SSB_BUSTYPE_PCI:
-		return ssb_pci_switch_coreidx(bus, coreidx);
-	case SSB_BUSTYPE_PCMCIA:
-		return ssb_pcmcia_switch_coreidx(bus, coreidx);
-	case SSB_BUSTYPE_SDIO:
-		return ssb_sdio_scan_switch_coreidx(bus, coreidx);
-	}
-	return 0;
-}
-
-void ssb_iounmap(struct ssb_bus *bus)
-{
-	switch (bus->bustype) {
-	case SSB_BUSTYPE_SSB:
-	case SSB_BUSTYPE_PCMCIA:
-		iounmap(bus->mmio);
-		break;
-	case SSB_BUSTYPE_PCI:
-#ifdef CONFIG_SSB_PCIHOST
-		pci_iounmap(bus->host_pci, bus->mmio);
-#else
-		SSB_BUG_ON(1); /* Can't reach this code. */
-#endif
-		break;
-	case SSB_BUSTYPE_SDIO:
-		break;
-	}
-	bus->mmio = NULL;
-	bus->mapped_device = NULL;
-}
-
-static void __iomem *ssb_ioremap(struct ssb_bus *bus,
-				 unsigned long baseaddr)
-{
-	void __iomem *mmio = NULL;
-
-	switch (bus->bustype) {
-	case SSB_BUSTYPE_SSB:
-		/* Only map the first core for now. */
-		/* fallthrough... */
-	case SSB_BUSTYPE_PCMCIA:
-		mmio = ioremap(baseaddr, SSB_CORE_SIZE);
-		break;
-	case SSB_BUSTYPE_PCI:
-#ifdef CONFIG_SSB_PCIHOST
-		mmio = pci_iomap(bus->host_pci, 0, ~0UL);
-#else
-		SSB_BUG_ON(1); /* Can't reach this code. */
-#endif
-		break;
-	case SSB_BUSTYPE_SDIO:
-		/* Nothing to ioremap in the SDIO case, just fake it */
-		mmio = (void __iomem *)baseaddr;
-		break;
-	}
-
-	return mmio;
-}
-
 static int we_support_multiple_80211_cores(struct ssb_bus *bus)
 {
 	/* More than one 802.11 core is only supported by special chips.
diff --git a/drivers/ssb/ssb_private.h b/drivers/ssb/ssb_private.h
index 0331139..929a722 100644
--- a/drivers/ssb/ssb_private.h
+++ b/drivers/ssb/ssb_private.h
@@ -4,6 +4,7 @@
 #include <linux/ssb/ssb.h>
 #include <linux/types.h>
 
+#include "bcmb_scan.h"
 
 #define PFX	"ssb: "
 
@@ -156,10 +157,8 @@ static inline int ssb_sdio_init(struct ssb_bus *bus)
 
 
 /* scan.c */
-extern const char *ssb_core_name(u16 coreid);
 extern int ssb_bus_scan(struct ssb_bus *bus,
 			unsigned long baseaddr);
-extern void ssb_iounmap(struct ssb_bus *ssb);
 
 
 /* sprom.c */
-- 
1.7.3.4

[RFC][PATCH] ssb: separate common scanning functions

[Rafał Miłecki]

2011/3/18 George Kashperko <george@znau.edu.ua>:
> Current ssb code ideology isn't really good place to start with support
> for ai backplanes. Its really great to see you willing to get ai
> supported in mainline but I'm sure this should be done apart from the
> ssb code and not even with one as the design decisions origin. Several
> concepts the ssb is based on are not designed to support anything else
> than ocp/sb and will require workarounds to suport ai.
> Thanks to Michael I had a time to think over the possibly code
> abstraction for shared sb and ai support. And while I'm still sure the
> patchwork for ai over ssb support is of good use as some intermediate
> decision to support ai-based hardware in sertain distributions but now I
> support Michael in that such (hopefully) a temporary buildups should not
> be in mainline.

Please, give some concrete arguments, which part of design does not match AI.

My patch has shown we need to duplicate 40% of SSB's scanning code.
What for example about pci.c? Which functions from that file won't be
duplicated in totally separated?

-- 
Rafa?

[RFC][PATCH] ssb: separate common scanning functions

[Rafał Miłecki]

2011/3/18 Rafa? Mi?ecki <zajec5@gmail.com>:
> What for example about pci.c? Which functions from that file won't be
> duplicated in totally separated?

At first I though we will need to duplicate all pci routines like
ssb_pci_switch_coreidx and ssb_pci_xtal. But now I've checked
brcm80211 and it seems it reads SPROM even from AI bus-cards.

Do you really want to duplicate all the SPROM code in *totally
separated* driver for AI?!

-- 
Rafa?

[RFC][PATCH] ssb: separate common scanning functions

[Rafał Miłecki]

2011/3/18 George Kashperko <george@znau.edu.ua>:
>> 2011/3/18 George Kashperko <george@znau.edu.ua>:
>> > Current ssb code ideology isn't really good place to start with support
>> > for ai backplanes. Its really great to see you willing to get ai
>> > supported in mainline but I'm sure this should be done apart from the
>> > ssb code and not even with one as the design decisions origin. Several
>> > concepts the ssb is based on are not designed to support anything else
>> > than ocp/sb and will require workarounds to suport ai.
>> > Thanks to Michael I had a time to think over the possibly code
>> > abstraction for shared sb and ai support. And while I'm still sure the
>> > patchwork for ai over ssb support is of good use as some intermediate
>> > decision to support ai-based hardware in sertain distributions but now I
>> > support Michael in that such (hopefully) a temporary buildups should not
>> > be in mainline.
>>
>> Please, give some concrete arguments, which part of design does not match AI.
> SSB design is core-centric, where all the bus activities are made in
> regard to the cores. This mostly is correct as most of the time
> bus/drivers code work with cores.

So finally, is there anything wrong about that?


> At the same time ssb model completely
> ignores the fact that each sb backplane core is a composite device
> consisting of the core itself and the core agent. In sb the agent
> registers space is referenced by manuals available and the Broadcom SDK
> code as one starting at 0x0F00 offset from the main core registers space
> beginning (actually I beleive this is true for sb revisions prior to 2.3
> and this range is actually 0x0C00-0x0FFF at least for sb rev. 2.3 and
> above). This is neglible for embedded backplanes as the whole backplane
> physical address space is accessed simultaneously, completely ignored by
> the ssb code for pci-hosted sb backplanes as only sliding bar0 window #1
> is utilised for backplane access, workarounded with
> select_core_and_segment in pcmcia host code.
> With axi introduced we have agents registers' spaces in completely
> different physical backplane pages, apart from the main core registers
> space. This is still have no impact for embeddables, will require bar0
> window #2 adjustment with each core switch (and here you will be
> required to have backplane specific processing in pci host code) and I
> don't really know how this could be workarounded for other hosts without
> them to know which exactly backplane they run and how should it be
> treaten.
> The clean solution I see here is to have host code apart from
> backplane-specific processing. The way to accomplish this is obvious as
> actual host device responsibility is not to switch cores but rather
> provide mappings into physical address space of the backplane regardless
> of the target request being made to core, sprom/otp, agent or whatever.
> This makes much cleaner model than that provided by ssb.

Yes, I noticed this already. Sill, after analyzing pci.c it seems we
will duplicate ssb_pci_xtal, ssb_pci_switch_coreidx and all SPROM
functions. We will just have different ssb_pci_switch_core and we will
drop old SPROMs layouts support.

My idea: separate functions we can share (ssb_pci_xtal,
ssb_pci_switch_coreidx, most of the SPROM functions) and create
separated PCI host drivers.


>> My patch has shown we need to duplicate 40% of SSB's scanning code.
>> What for example about pci.c? Which functions from that file won't be
>> duplicated in totally separated?
>>
> Going further in extending ssb to support ai will either require the
> host to be confident of the backplane type and layout (see
> drivers/staging/brcm80211/utils for the good example of messup it will
> lead to) or will reguire the backplane-specific code to break into the
> host code (brcm80211/utils again).

I don't want to extend ssb, I said that with my patch message. I
wanted separated drivers sharing some functions, please take a look at
my comment included in patch. I think we could also create *separated*
host drivers sharing most of the code.


> Current ssb code is two-layer - in two words its about cores on host.
> This makes sertain hosts to know everyting about cores, but at the same
> time it let the cores to be abstract from the hosts. Building ai support
> over this model will make hosts to know even more about cores, but if
> done right it will still keep the cores autonomous.
> At the same time host-backplane-core model provide better code
> separation, little-to-nothing code duplication and let host, backplane
> and core code writers avoid workarounds required for current ssb model.

-- 
Rafa?

[RFC][PATCH] ssb: separate common scanning functions

[Rafał Miłecki]

2011/3/18 George Kashperko <george@znau.edu.ua>:
> My english is awful therefore seems we missunderstood each other. I'm
> sure I got your point right - you plan to start up the new (bcmb)
> project for both sb and ai support.

No, no, no. My bcmb is not expected to be project for both buses. It
is expected to provide helper functions for *two separated* drivers:
ai and ssb.

I don't want to duplicate ssb_core_name (already implemented in ssb)
in my ai. That way I put it in bcmb_scan.c. I can now use
ssb_core_name in both: ssb and ai without double, duplicated
implementation.

I'm still reading your earlier e-mail...

-- 
Rafa?

[RFC][PATCH] ssb: separate common scanning functions

[Rafał Miłecki]

2011/3/18 George Kashperko <george@znau.edu.ua>:
>> 2011/3/18 Rafa? Mi?ecki <zajec5@gmail.com>:
>> > What for example about pci.c? Which functions from that file won't be
>> > duplicated in totally separated?
>>
>> At first I though we will need to duplicate all pci routines like
>> ssb_pci_switch_coreidx and ssb_pci_xtal. But now I've checked
>> brcm80211 and it seems it reads SPROM even from AI bus-cards.
> Btw ssb_pci_xtal is for PCI. PCIE hosts don't need this.

Not even all PCI devices. Just 4306 I believe:
/* kludge to enable the clock on the 4306 which lacks a slowclock */
if (bustype == PCI_BUS && !si_ispcie(sii))
	si_clkctl_xtal(&sii->pub, XTAL | PLL, ON);


>> Do you really want to duplicate all the SPROM code in *totally
>> separated* driver for AI?!
>>
> Every sb/ai backplane known to me at the moment are featuring the
> following hardware design:
> System backplane with individual devices on it (called cores)
> communicating with the means of agents. The agents for sb are in the
> main core registers space, and apart from the core registers for axi.
> The backplane itself is "mastered" by buscore device. This is mips core
> for embeddables, pci(e) core for pci(e) hosted backplanes, pcmcia core
> for backplanes on pcmcia/sdio. Might OCP core is some sort of buscore as
> well used to bridge two sb backplanes.
> Buscore is responsible for interrupts management, backplane-to-host-bus
> operations, agent-to-agent transfers.
>
> Apart from the buscore, there is another "special" device on the
> backplane - buscommon. Unlike buscore this one seems to be optional, and
> not present on some old pcmcia-hosted backplanes. The buscommon is
> responsible for managing bus clocks, can serve uarts, also is a source
> of misc. configuration information for the backplane. These buscommons
> are chipcommon and extif cores.
>
> Here it would be great to have more technical background on the subject
> but unfortunately apart from the staging brcm80211 and GPL packages for
> respective embeddables the only open doc on the subject available to me
> is www.broadcom.com/collateral/pg/440X-PG02-R.pdf

Well, OK, it's generally well known already. Maybe I just prefer to
call "buscore" a "hostcore" and "buscommon" a "chipcommon". I believe
that are the names we hot used to.


> So software model I see here looks like following:
> * Backplane-type handler responsible for
> ?~ initial scanning;
> ?~ agent-specific operations (core enable/disable, irq flags management,
> etc.);
>
> * The bus driver itself responsible for initial detection and assignment
> of backplane handler and also managig driver registration/binding/etc
> for
> ?~ buscommon;
> ?~ buscore;
> ?~ regular cores;
>
> * Host driver managing:
> ?~ requests to the physical address space of backplane;
> ?~ host interrupt management;
> ?~ host-specific workarounds (ssb_pci_xtal is one of them);
>
> This requires generic interfaces for:
> * host (like those ssb_bus_ops which are actually not bus but host ops -
> handling not core accesses but physical backplane addresses requests;
> iterrupt management ops);
> * backplane (scan, enable, disable, irq_flag etc.);
> * buscore (backplane irq/errors/etc. management);
> * buscommon (backplane clocks/etc. management, capabilities queries);
>
> Buscommon and buscore unlike current ssb model could be separate
> drivers. This will help to break apart all that mess of versions
> checking and revision-specific processing. This will provide clean way
> of obsoleting and removing the support for old hardware and introducing
> newer one.
> Regular bus core devices thus are to be registered with linux once
> buscommon, buscore and host drivers are bound and set up making the bus
> operational.
> Buscore and buscommon as separate drivers will require some code to be
> replicated over close versions but overall I've already tested this
> approach with chipcommons with pmu r0, r1 and r5 on pcie and mips hosts
> and final drivers' code is clean and manageable unlike all that mess in
> hndpmu.c
> Same stands for mips/pci host cores.

OK, I generally agree. We can try moving to such a layout. The only
thing I hate in your view is "obsoleting and removing the support for
old hardware".

Answer me this question, please:
Why do you think my proposed patch conflicts with layout proposed by you?

You said you want to have "Backplane-type handler". So according to
this we will need two drivers/handlers: 1 for scanning SSB and 1 for
scanning AI. That's what I try to implement. I just want to share some
functions between the one for SSB and the one for AI.

I don't see the point where my patch is in conflict with your idea.

-- 
Rafa?

[RFC][PATCH] ssb: separate common scanning functions

[Larry Finger]

On 03/18/2011 11:25 AM, Rafa? Mi?ecki wrote:
> 2011/3/18 George Kashperko<george@znau.edu.ua>:
>>> 2011/3/18 Rafa? Mi?ecki<zajec5@gmail.com>:
>>>> What for example about pci.c? Which functions from that file won't be
>>>> duplicated in totally separated?
>>>
>>> At first I though we will need to duplicate all pci routines like
>>> ssb_pci_switch_coreidx and ssb_pci_xtal. But now I've checked
>>> brcm80211 and it seems it reads SPROM even from AI bus-cards.
>> Btw ssb_pci_xtal is for PCI. PCIE hosts don't need this.
>
> Not even all PCI devices. Just 4306 I believe:
> /* kludge to enable the clock on the 4306 which lacks a slowclock */
> if (bustype == PCI_BUS&&  !si_ispcie(sii))
> 	si_clkctl_xtal(&sii->pub, XTAL | PLL, ON);
>
>
>>> Do you really want to duplicate all the SPROM code in *totally
>>> separated* driver for AI?!
>>>
>> Every sb/ai backplane known to me at the moment are featuring the
>> following hardware design:
>> System backplane with individual devices on it (called cores)
>> communicating with the means of agents. The agents for sb are in the
>> main core registers space, and apart from the core registers for axi.
>> The backplane itself is "mastered" by buscore device. This is mips core
>> for embeddables, pci(e) core for pci(e) hosted backplanes, pcmcia core
>> for backplanes on pcmcia/sdio. Might OCP core is some sort of buscore as
>> well used to bridge two sb backplanes.
>> Buscore is responsible for interrupts management, backplane-to-host-bus
>> operations, agent-to-agent transfers.
>>
>> Apart from the buscore, there is another "special" device on the
>> backplane - buscommon. Unlike buscore this one seems to be optional, and
>> not present on some old pcmcia-hosted backplanes. The buscommon is
>> responsible for managing bus clocks, can serve uarts, also is a source
>> of misc. configuration information for the backplane. These buscommons
>> are chipcommon and extif cores.
>>
>> Here it would be great to have more technical background on the subject
>> but unfortunately apart from the staging brcm80211 and GPL packages for
>> respective embeddables the only open doc on the subject available to me
>> is www.broadcom.com/collateral/pg/440X-PG02-R.pdf
>
> Well, OK, it's generally well known already. Maybe I just prefer to
> call "buscore" a "hostcore" and "buscommon" a "chipcommon". I believe
> that are the names we hot used to.
>
>
>> So software model I see here looks like following:
>> * Backplane-type handler responsible for
>>   ~ initial scanning;
>>   ~ agent-specific operations (core enable/disable, irq flags management,
>> etc.);
>>
>> * The bus driver itself responsible for initial detection and assignment
>> of backplane handler and also managig driver registration/binding/etc
>> for
>>   ~ buscommon;
>>   ~ buscore;
>>   ~ regular cores;
>>
>> * Host driver managing:
>>   ~ requests to the physical address space of backplane;
>>   ~ host interrupt management;
>>   ~ host-specific workarounds (ssb_pci_xtal is one of them);
>>
>> This requires generic interfaces for:
>> * host (like those ssb_bus_ops which are actually not bus but host ops -
>> handling not core accesses but physical backplane addresses requests;
>> iterrupt management ops);
>> * backplane (scan, enable, disable, irq_flag etc.);
>> * buscore (backplane irq/errors/etc. management);
>> * buscommon (backplane clocks/etc. management, capabilities queries);
>>
>> Buscommon and buscore unlike current ssb model could be separate
>> drivers. This will help to break apart all that mess of versions
>> checking and revision-specific processing. This will provide clean way
>> of obsoleting and removing the support for old hardware and introducing
>> newer one.
>> Regular bus core devices thus are to be registered with linux once
>> buscommon, buscore and host drivers are bound and set up making the bus
>> operational.
>> Buscore and buscommon as separate drivers will require some code to be
>> replicated over close versions but overall I've already tested this
>> approach with chipcommons with pmu r0, r1 and r5 on pcie and mips hosts
>> and final drivers' code is clean and manageable unlike all that mess in
>> hndpmu.c
>> Same stands for mips/pci host cores.
>
> OK, I generally agree. We can try moving to such a layout. The only
> thing I hate in your view is "obsoleting and removing the support for
> old hardware".

I also do not like the thought of removing support of old hardware. Given the 
lack of support from some vendors, Linux may be somewhat slow in providing 
drivers for new devices; however, we can and should keep the support for legacy 
devices. I built a sandbox computer just for testing a BCM4306/3 that uses b43 
and a BCM4303 using b43legacy.

Larry

[RFC][PATCH] ssb: separate common scanning functions

[Rafał Miłecki]

2011/3/18 George Kashperko <george@znau.edu.ua>:
>> I don't see the point where my patch is in conflict with your idea.
> Not your patch, just scan_read32, scan_switchcore, ssb_iounmap,
> ssb_ioremap. So far ssb_core_name is fine.
> Yet again, ssb code model is core-sentric which is not actually true
> from the hardware pint of view. With few workarounds it works fine for
> sb. For axi we will have more of these.
>
> scan_read32, scan_switchcore, ssb_iounmap, ssb_ioremap are all
> host-driven services. In ssb model host is operating cores and therefore
> requires all that switch, scan_read, map, unmap etc. While with model
> where host is providing not core but backplane access the only routine
> among those above you need is scan_read32 and nothing more (actually you
> don't need it too as host ops with read(8|16|32) are completely
> sufficient for scanning). The purpose of scan_read32 for ssb model is
> that ssb_bus_ops are operating cores which you don't have at the moment
> when scanning. With host ops designed as u32 (*read32)(struct host
> *host, phys_addr_t addr) host will do what it supposed to - manage its
> means of backplane physical access and will have absolutely no need to
> break into understanding what the core/sprom/otp/etc. is.

That's what I wanted to hear from the beginning, thanks! :)

The reason I implemented scan_switchcore is early stage, when I
prepare bus for reading EROM:
erombase = ai_scan_read32(bus, 0, SSB_CHIPCO_EROM);
pci_write_config_dword(bus->host_pci, PCI_BAR0_WIN, erombase);
In above you can see my hack which works for PCI host only. This hack
I wanted to replace with scan_switchcore and that functions seems to
perform exactly this operation.

AFAIU we need to adjust PCI_BAR0_WIN and PCI_BAR0_WIN2 later, when we
already have info about core from EROM. In early stage we can not use
that.

Anyway with what you said now, I'll re-read your mails again, to fully
understand your POV.

-- 
Rafa?

[RFC][PATCH] ssb: separate common scanning functions

[Rafał Miłecki]

2011/3/18 George Kashperko <george@znau.edu.ua>:
> The clean solution I see here is to have host code apart from
> backplane-specific processing. The way to accomplish this is obvious as
> actual host device responsibility is not to switch cores but rather
> provide mappings into physical address space of the backplane regardless
> of the target request being made to core, sprom/otp, agent or whatever.
> This makes much cleaner model than that provided by ssb.

How do you think, what interface would fit for that?

Something like:
struct host_ops {
        bool (*init)(struct device *dev);
        bool (*exit)(struct device *dev);
        bool (*set_agent_addr)(struct device *dev, u16 addr);
        bool (*set_core_addr)(struct device *dev, u16 addr);
};
?

-- 
Rafa?

[RFC][PATCH] ssb: separate common scanning functions

[Rafał Miłecki]

2011/3/18 George Kashperko <george@znau.edu.ua>:
> ?- read pci revision id/vendor id (pci_read_config_word), if required
> to decide if second window is sliding or sprom (actually rev_id should
> be sufficient for that) read offset 0x0000 from bar0 offset to get
> chipid register value

Could you say something about WIN1 and WIN2? What are they for, can we
use them somehow separately? What do you mean by second window being
SPROM? We read sprom without any special window magic:
sprom[i] = ioread16(bus->mmio + bus->sprom_offset + (i * 2));

My understanding is limited to that:
If we want to use device with CORE_ADDR and WRAP_ADDR we have to write
CORE_ADDR to BAR0_WIN and WRAP_ADDR to BAR0_WIN2. Then we can use
readl/writel.

-- 
Rafa?

[RFC][PATCH] ssb: separate common scanning functions

[Rafał Miłecki]

2011/3/18 George Kashperko <george@znau.edu.ua>:
> Well, I see this as following. In generic host life time there are
> several states. These states are following:
> 1. Host just started up, underlying backplane is powered up, we issued
> backplane detect/scan. At this point at least some minimal windowed
> access is up (if such required), not yet cores/devices are known.
> 2. Backplane got identified, scanned, individual cores/devices
> recognised, buscommon and buscore are registered with kernel to get them
> matched with drivers, and then probed and set up.
> 3. Buscommon and buscore are driven, host can finish with host specific
> workarounds, both buscommon and buscore can get their _init entry points
> called, we can setup host device irq routine, finally we can expose the
> rest cores/devices to kernel.
>
> With that in mind here is my general host ops design pseudo code:
> struct host_ops {
> ? ? ? ?/* Init call we should get once both buscommon and buscore drivers are bound (state #3) */
> ? ? ? ?int (*init)(struct bcmb_bus *bus);
>
> ? ? ? ?/* Regular backplane access ops */
> ? ? ? ?u8 (*read(8|16|32))(struct bcmb_bus *bus, bcmb_addr_t addr);
> ? ? ? ?void (*write(8|16|32))(struct bcmb_bus *bus, bcmb_addr_t addr, u(8|16|32) val);
>
> ? ? ? ?/* For some theoretically hard-to-set-up before scan hosts we could keep scan_read32 */
> ? ? ? ?u32 scan_read32(struct bcmb_bus *bus, bcmb_addr_t addr);
> };

I don't think it makes much sense. If init is going to be called in
state #3, what about some pre-init? set_master, request_region, xtal,
iomap?

-- 
Rafa?

[RFC][PATCH] ssb: separate common scanning functions

[Rafał Miłecki]

2011/3/18 George Kashperko <george@znau.edu.ua>:
> Well, here you are:
> 1. PCI hosts with PCI buscores prior rev. 13 (8Kb window)
> offset ?size ? ?type ? ?description
> 0x0000 ?0x1000 ?sliding controlled with BAR0_WIN1 register
> 0x1000 ?0x0800 ?fixed ? SPROM
> 0x1800 ?0x0400 ?fixed ? PCI core (core registers 0x0000 to 0x03FF range)
> 0x1C00 ?0x0400 ?fixed ? PCI core (agent registers, 0x0C00 to 0x0FFF range)
>
> 2. PCI hosts with PCI buscores rev. 13 and above and PCIE with
> Chipcommon rev. up to 31 (16Kb window)
> offset ?size ? ?type ? ?description
> 0x0000 ?0x1000 ?sliding controlled with BAR0_WIN1 register
> 0x1000 ?0x1000 ?fixed ? SPROM
> 0x2000 ?0x1000 ?fixed ? PCI core
> 0x3000 ?0x1000 ?fixed ? Chipcommon core
>
> 3. PCIE hosts with Chipcommon rev. 32 and above
> offset ?size ? ?type ? ?description
> 0x0000 ?0x1000 ?sliding controlled with BAR0_WIN1 register
> 0x1000 ?0x1000 ?sliding controlled with BAR0_WIN2 register
> 0x2000 ?0x1000 ?fixed ? PCI core
> 0x3000 ?0x1000 ?fixed ? Chipcommon core
>
> As you can see even if PCI_REVISION can't feed us with guaranteed choice
> of either of these 3 layouts we can distingiush between them easily like
> following:
>
> if (pci_is_pcie(dev)) {
> ? ? ? ?u32 chipid = ioread32(bar0_base + 0x3000);
> ? ? ? ?if (chipid & 0x10000000)
> ? ? ? ? ? ? ? ?/* AXI, Chipcommon rev. is 32+ */
> ? ? ? ? ? ? ? ?goto win_3_setup;
> ? ? ? ?else
> ? ? ? ? ? ? ? ?/* SB, Chipcommon rev. is <= 31 */
> ? ? ? ? ? ? ? ?goto win_2_setup;
> } else {
> ? ? ? ?u32 idhi = ioread32(bar0_base + 0x1C00 + 0x03FC);
> ? ? ? ?if (((idhi & 0x00008FF0) >> 4) == 0x804)
> ? ? ? ? ? ? ? ?/* PCI core id */
> ? ? ? ? ? ? ? ?goto win_1_setup;
> ? ? ? ?else
> ? ? ? ? ? ? ? ?/* Some crap from SPROM area */
> ? ? ? ? ? ? ? ?goto win_2_setup;
> }
>
> Therefore we can setup PCI(e) host windows for backplane access prior to
> scanning.
>
> As for BAR0_WIN[12] registers, they point to physical base address on
> backplane that will be mapped into corresponding 0x1000-size window. And
> here again registers don't care what exactly you want to see there. Both
> the windows can be controlled independently.
>
> Finally, you might noticed we don't have SPROM in last 3rd layout. Thats
> because it is in Chipcommon registers' space. For layouts #1 & #2 BAR0
> range 0x1000 to 0x2000 is either mapped to actual SPROM or to SPROM
> shadow in PCI core.

Wohoo, and this is second part of info I really needed, thanks a lot!
I think there are mistakes in it, but I just wanted to get the idea of
windows. So thanks a lot.

As for mistakes:
1) The split is not 1-31 vs. 32-... I believe it is 1-30 vs. 31-...
2) On chipco >= 31 SSB SPROM seems to be 0x800
Maybe sth more...

-- 
Rafa?

[RFC][PATCH] ssb: separate common scanning functions

[Rafał Miłecki]

2011/3/19 George Kashperko <george@znau.edu.ua>:
>
>> 2011/3/18 George Kashperko <george@znau.edu.ua>:
>> > Well, here you are:
>> > 1. PCI hosts with PCI buscores prior rev. 13 (8Kb window)
>> > offset ?size ? ?type ? ?description
>> > 0x0000 ?0x1000 ?sliding controlled with BAR0_WIN1 register
>> > 0x1000 ?0x0800 ?fixed ? SPROM
>> > 0x1800 ?0x0400 ?fixed ? PCI core (core registers 0x0000 to 0x03FF range)
>> > 0x1C00 ?0x0400 ?fixed ? PCI core (agent registers, 0x0C00 to 0x0FFF range)
>> >
>> > 2. PCI hosts with PCI buscores rev. 13 and above and PCIE with
>> > Chipcommon rev. up to 31 (16Kb window)
>> > offset ?size ? ?type ? ?description
>> > 0x0000 ?0x1000 ?sliding controlled with BAR0_WIN1 register
>> > 0x1000 ?0x1000 ?fixed ? SPROM
>> > 0x2000 ?0x1000 ?fixed ? PCI core
>> > 0x3000 ?0x1000 ?fixed ? Chipcommon core
>> >
>> > 3. PCIE hosts with Chipcommon rev. 32 and above
>> > offset ?size ? ?type ? ?description
>> > 0x0000 ?0x1000 ?sliding controlled with BAR0_WIN1 register
>> > 0x1000 ?0x1000 ?sliding controlled with BAR0_WIN2 register
>> > 0x2000 ?0x1000 ?fixed ? PCI core
>> > 0x3000 ?0x1000 ?fixed ? Chipcommon core
>> >
>> > As you can see even if PCI_REVISION can't feed us with guaranteed choice
>> > of either of these 3 layouts we can distingiush between them easily like
>> > following:
>> >
>> > if (pci_is_pcie(dev)) {
>> > ? ? ? ?u32 chipid = ioread32(bar0_base + 0x3000);
>> > ? ? ? ?if (chipid & 0x10000000)
>> > ? ? ? ? ? ? ? ?/* AXI, Chipcommon rev. is 32+ */
>> > ? ? ? ? ? ? ? ?goto win_3_setup;
>> > ? ? ? ?else
>> > ? ? ? ? ? ? ? ?/* SB, Chipcommon rev. is <= 31 */
>> > ? ? ? ? ? ? ? ?goto win_2_setup;
>> > } else {
>> > ? ? ? ?u32 idhi = ioread32(bar0_base + 0x1C00 + 0x03FC);
>> > ? ? ? ?if (((idhi & 0x00008FF0) >> 4) == 0x804)
>> > ? ? ? ? ? ? ? ?/* PCI core id */
>> > ? ? ? ? ? ? ? ?goto win_1_setup;
>> > ? ? ? ?else
>> > ? ? ? ? ? ? ? ?/* Some crap from SPROM area */
>> > ? ? ? ? ? ? ? ?goto win_2_setup;
>> > }
>> >
>> > Therefore we can setup PCI(e) host windows for backplane access prior to
>> > scanning.
>> >
>> > As for BAR0_WIN[12] registers, they point to physical base address on
>> > backplane that will be mapped into corresponding 0x1000-size window. And
>> > here again registers don't care what exactly you want to see there. Both
>> > the windows can be controlled independently.
>> >
>> > Finally, you might noticed we don't have SPROM in last 3rd layout. Thats
>> > because it is in Chipcommon registers' space. For layouts #1 & #2 BAR0
>> > range 0x1000 to 0x2000 is either mapped to actual SPROM or to SPROM
>> > shadow in PCI core.
>>
>> Wohoo, and this is second part of info I really needed, thanks a lot!
>> I think there are mistakes in it, but I just wanted to get the idea of
>> windows. So thanks a lot.
>>
>> As for mistakes:
>> 1) The split is not 1-31 vs. 32-... I believe it is 1-30 vs. 31-...
>> 2) On chipco >= 31 SSB SPROM seems to be 0x800
>> Maybe sth more...
>>
> Watch bcmsrom.c for #define SROM_OFFSET
>
> For Chipcommons rev. >31 (the same as >=32) SPROM is in Chipcommon at
> 0x0800 offset - 0x3800 offset of BAR0 (if CAP_SPROM capability bit is
> set), otherwise SPROM is unavailable.
> For Chipcommons rev. <=31 (the same as <32) SPROM is mapped to 0x1000
> offset of BAR0.

I guess we may have errors in:
http://bcm-v4.sipsolutions.net/SPROM
http://bcm-v4.sipsolutions.net/802.11/IsSpromAvailable
then.

-- 
Rafa?