[RFC] How to rename SSB_TMSLOW_*, B43_TMSLOW_*?

[RFC] How to rename SSB_TMSLOW_*, B43_TMSLOW_*?

[Rafał Miłecki]

Except for following 3 defines:
#define  SSB_TMSLOW_RESET	0x00000001 /* Reset */
#define  SSB_TMSLOW_REJECT_22	0x00000002 /* Reject (Backplane rev 2.2) */
#define  SSB_TMSLOW_REJECT_23	0x00000004 /* Reject (Backplane rev 2.3) */

All our SSB_TMSLOW_* and B43_TMSLOW_* defines are some core control
bits. As we now know, core control bits are not SSB specific or TMSLOW
specific.

Should we (and how) define that names in this situation?

For b43 I propose (quite obvious?) B43_CORE_CTL_*.

However what about SSB_TMSLOW_*? George proposed SSB_CORECTL_*, but it
contains "SSB", while that bits are not SSB specific. Same bits are
used on AI bus. Should we use some SSBAI_CORE_CTL_* then? Any other
ideas? Some better maybe?

P.S.
Personally I prefer CORE_CTL over CORECTL (George). Which one should we use?

-- 
Rafał

Re: [RFC] How to rename SSB_TMSLOW_*, B43_TMSLOW_*?

[George Kashperko]

Hi,

> Except for following 3 defines:
> #define  SSB_TMSLOW_RESET	0x00000001 /* Reset */
> #define  SSB_TMSLOW_REJECT_22	0x00000002 /* Reject (Backplane rev 2.2) */
> #define  SSB_TMSLOW_REJECT_23	0x00000004 /* Reject (Backplane rev 2.3) */
> 
> All our SSB_TMSLOW_* and B43_TMSLOW_* defines are some core control
> bits. As we now know, core control bits are not SSB specific or TMSLOW
> specific.
> 
> Should we (and how) define that names in this situation?
> 
> For b43 I propose (quite obvious?) B43_CORE_CTL_*.
> 
> However what about SSB_TMSLOW_*? George proposed SSB_CORECTL_*, but it
> contains "SSB", while that bits are not SSB specific. Same bits are
> used on AI bus. Should we use some SSBAI_CORE_CTL_* then? Any other
> ideas? Some better maybe?
I'm still sure AI us much more SSB rather than lets say BCMAI. In
current SSB architecture each core software-wise is represented by 4k
registers' space with own core registers and bus-specific registers in
that single 4k page. AI cores keep own core registers same as SSB in
single 4k space whereas bus-specific registers are in separate page and
have somewhat another layout. But among all the bus specific registers
for both SSB and AI despite the fact they located in different places
they either share the same meaning with just different layout
(TMSLOW/TMSHIGH) or can be abstracted by appropriate handlers
(admatch/irqflag). It looks to me much like pcie and pci which share
single software bus ideology rather than introducing two different
buses. Therefore I still don't really convinced with idea that just for
that difference in where bus-specific registers are located we should
introduce one more bus. Actually it looks somewhat similar to
core-wrappers' technique already in SSB - those admatch thingys used to
access sub-cores withing SSB core. We don't expose any sw buildups for
just those, same as we could do for AI if only let it share SSB code
base.

> 
> P.S.
> Personally I prefer CORE_CTL over CORECTL (George). Which one should we use?
> 
I decided on CORECTL/CORESTAT over CORE_CTL/CORE_STATE just to get long
things to be bits shorter.

Have nice day,
George

Re: [RFC] How to rename SSB_TMSLOW_*, B43_TMSLOW_*?

[Michael Büsch]

On Wed, 2011-02-16 at 14:13 +0100, Rafał Miłecki wrote: 
> Except for following 3 defines:
> #define  SSB_TMSLOW_RESET	0x00000001 /* Reset */
> #define  SSB_TMSLOW_REJECT_22	0x00000002 /* Reject (Backplane rev 2.2) */
> #define  SSB_TMSLOW_REJECT_23	0x00000004 /* Reject (Backplane rev 2.3) */
> 
> All our SSB_TMSLOW_* and B43_TMSLOW_* defines are some core control
> bits. As we now know, core control bits are not SSB specific or TMSLOW
> specific.
> 
> Should we (and how) define that names in this situation?
> 
> For b43 I propose (quite obvious?) B43_CORE_CTL_*.
> 
> However what about SSB_TMSLOW_*? George proposed SSB_CORECTL_*, but it
> contains "SSB", while that bits are not SSB specific. Same bits are
> used on AI bus. Should we use some SSBAI_CORE_CTL_* then? Any other
> ideas? Some better maybe?
> 
> P.S.
> Personally I prefer CORE_CTL over CORECTL (George). Which one should we use?

Let's simply put those bits into the drivers and call them
DRIVERNAME_TMSLOW_FOOBAR

The "TMSLOW" part seems rather important to me, because that makes it
obvious what register these bits belong to. Note that's there's also
TMSHIGH. It also follows current naming convention.

-- 
Greetings Michael.

Re: [RFC] How to rename SSB_TMSLOW_*, B43_TMSLOW_*?

[Rafał Miłecki]

[-- Attachment #1: Type: text/plain, Size: 3996 bytes --]

W dniu 16 lutego 2011 23:17 użytkownik Michael Büsch <mb@bu3sch.de> napisał:
> On Wed, 2011-02-16 at 14:13 +0100, Rafał Miłecki wrote:
>> Except for following 3 defines:
>> #define  SSB_TMSLOW_RESET     0x00000001 /* Reset */
>> #define  SSB_TMSLOW_REJECT_22 0x00000002 /* Reject (Backplane rev 2.2) */
>> #define  SSB_TMSLOW_REJECT_23 0x00000004 /* Reject (Backplane rev 2.3) */
>>
>> All our SSB_TMSLOW_* and B43_TMSLOW_* defines are some core control
>> bits. As we now know, core control bits are not SSB specific or TMSLOW
>> specific.
>>
>> Should we (and how) define that names in this situation?
>>
>> For b43 I propose (quite obvious?) B43_CORE_CTL_*.
>>
>> However what about SSB_TMSLOW_*? George proposed SSB_CORECTL_*, but it
>> contains "SSB", while that bits are not SSB specific. Same bits are
>> used on AI bus. Should we use some SSBAI_CORE_CTL_* then? Any other
>> ideas? Some better maybe?
>>
>> P.S.
>> Personally I prefer CORE_CTL over CORECTL (George). Which one should we use?
>
> Let's simply put those bits into the drivers and call them
> DRIVERNAME_TMSLOW_FOOBAR

That's what we already have...
B43_TMSLOW_GMODE
B43_TMSLOW_PLLREFSEL
B43_TMSLOW_MACPHYCLKEN
B43_TMSLOW_PHYRESET
B43_TMSLOW_PHYCLKEN
And it's fine for now, but not if we want to have AI support in b43.
See my next comment.


> The "TMSLOW" part seems rather important to me, because that makes it
> obvious what register these bits belong to. Note that's there's also
> TMSHIGH. It also follows current naming convention.

I skip TMSHIGH for now, to simplify discussion.

Your proposal is fine as long as we use that flags for TMSLOW only.
But we may need to use that for AI boards in future, which does not
contain TMSLOW. So that way we would finish with something so ugly:

#define B43_SSB_TMSLOW_GMODE			0x20000000	/* G Mode Enable */
#define B43_SSB_TMSLOW_PHY_BANDWIDTH		0x00C00000	/* PHY band width and
clock speed mask (N-PHY only) */
#define  B43_SSB_TMSLOW_PHY_BANDWIDTH_10MHZ	0x00000000	/* 10 MHz
bandwidth, 40 MHz PHY */
#define  B43_SSB_TMSLOW_PHY_BANDWIDTH_20MHZ	0x00400000	/* 20 MHz
bandwidth, 80 MHz PHY */
#define  B43_SSB_TMSLOW_PHY_BANDWIDTH_40MHZ	0x00800000	/* 40 MHz
bandwidth, 160 MHz PHY */
#define B43_SSB_TMSLOW_PLLREFSEL		0x00200000	/* PLL Frequency
Reference Select (rev >= 5) */
#define B43_SSB_TMSLOW_MACPHYCLKEN		0x00100000	/* MAC PHY Clock
Control Enable (rev >= 5) */
#define B43_SSB_TMSLOW_PHYRESET			0x00080000	/* PHY Reset */
#define B43_SSB_TMSLOW_PHYCLKEN			0x00040000	/* PHY Clock Enable */

#define B43_AI_REGNAME_GMODE			0x2000	/* G Mode Enable */
#define B43_AI_REGNAME_BANDWIDTH		0x00C0	/* PHY band width and clock
speed mask (N-PHY only) */
#define  B43_AI_REGNAME_BANDWIDTH_10MHZ		0x0000	/* 10 MHz bandwidth,
40 MHz PHY */
#define  B43_AI_REGNAME_BANDWIDTH_20MHZ		0x0040	/* 20 MHz bandwidth,
80 MHz PHY */
#define  B43_AI_REGNAME_BANDWIDTH_40MHZ		0x0080	/* 40 MHz bandwidth,
160 MHz PHY */
#define B43_AI_REGNAME_PLLREFSEL		0x0020	/* PLL Frequency Reference
Select (rev >= 5) */
#define B43_AI_REGNAME_MACPHYCLKEN		0x0010	/* MAC PHY Clock Control
Enable (rev >= 5) */
#define B43_AI_REGNAME_RESET			0x0008	/* PHY Reset */
#define B43_AI_REGNAME_CLKEN			0x0004	/* PHY Clock Enable */

if (boardtype == SSB) {
	flags |= B43_SSB_TMSLOW_PHYCLKEN;
	flags |= B43_SSB_TMSLOW_PHYRESET;
	if (dev->phy.type == B43_PHYTYPE_N)
		flags |= B43_SSB_TMSLOW_PHY_BANDWIDTH_20MHZ; /* Make 20 MHz def */
} else if (boardtype == AI) {
	flags |= B43_AI_REGNAME_PHYCLKEN;
	flags |= B43_AI_REGNAME_PHYRESET;
	if (dev->phy.type == B43_PHYTYPE_N)
		flags |= B43_AI_REGNAME_PHY_BANDWIDTH_20MHZ; /* Make 20 MHz def */
}

That's why I wanted to introduce some SSBAI_CORECTL_FOOBAR and make
b43 unaware it that flags are going to be written in register A or B.
I wanted b43 to just pass flags to correct function and do not care
about place where they are getting written.

-- 
Rafał

[-- Attachment #2: ugly.code.txt --]
[-- Type: text/plain, Size: 1859 bytes --]

#define B43_SSB_TMSLOW_GMODE			0x20000000	/* G Mode Enable */
#define B43_SSB_TMSLOW_PHY_BANDWIDTH		0x00C00000	/* PHY band width and clock speed mask (N-PHY only) */
#define  B43_SSB_TMSLOW_PHY_BANDWIDTH_10MHZ	0x00000000	/* 10 MHz bandwidth, 40 MHz PHY */
#define  B43_SSB_TMSLOW_PHY_BANDWIDTH_20MHZ	0x00400000	/* 20 MHz bandwidth, 80 MHz PHY */
#define  B43_SSB_TMSLOW_PHY_BANDWIDTH_40MHZ	0x00800000	/* 40 MHz bandwidth, 160 MHz PHY */
#define B43_SSB_TMSLOW_PLLREFSEL		0x00200000	/* PLL Frequency Reference Select (rev >= 5) */
#define B43_SSB_TMSLOW_MACPHYCLKEN		0x00100000	/* MAC PHY Clock Control Enable (rev >= 5) */
#define B43_SSB_TMSLOW_PHYRESET			0x00080000	/* PHY Reset */
#define B43_SSB_TMSLOW_PHYCLKEN			0x00040000	/* PHY Clock Enable */

#define B43_AI_REGNAME_GMODE			0x2000	/* G Mode Enable */
#define B43_AI_REGNAME_BANDWIDTH		0x00C0	/* PHY band width and clock speed mask (N-PHY only) */
#define  B43_AI_REGNAME_BANDWIDTH_10MHZ		0x0000	/* 10 MHz bandwidth, 40 MHz PHY */
#define  B43_AI_REGNAME_BANDWIDTH_20MHZ		0x0040	/* 20 MHz bandwidth, 80 MHz PHY */
#define  B43_AI_REGNAME_BANDWIDTH_40MHZ		0x0080	/* 40 MHz bandwidth, 160 MHz PHY */
#define B43_AI_REGNAME_PLLREFSEL		0x0020	/* PLL Frequency Reference Select (rev >= 5) */
#define B43_AI_REGNAME_MACPHYCLKEN		0x0010	/* MAC PHY Clock Control Enable (rev >= 5) */
#define B43_AI_REGNAME_RESET			0x0008	/* PHY Reset */
#define B43_AI_REGNAME_CLKEN			0x0004	/* PHY Clock Enable */

if (boardtype == SSB) {
	flags |= B43_SSB_TMSLOW_PHYCLKEN;
	flags |= B43_SSB_TMSLOW_PHYRESET;
	if (dev->phy.type == B43_PHYTYPE_N)
		flags |= B43_SSB_TMSLOW_PHY_BANDWIDTH_20MHZ; /* Make 20 MHz def */
} else if (boardtype == AI) {
	flags |= B43_AI_REGNAME_PHYCLKEN;
	flags |= B43_AI_REGNAME_PHYRESET;
	if (dev->phy.type == B43_PHYTYPE_N)
		flags |= B43_AI_REGNAME_PHY_BANDWIDTH_20MHZ; /* Make 20 MHz def */
}

Re: [RFC] How to rename SSB_TMSLOW_*, B43_TMSLOW_*?

[Michael Büsch]

On Thu, 2011-02-17 at 13:14 +0100, Rafał Miłecki wrote: 
> That's why I wanted to introduce some SSBAI_CORECTL_FOOBAR and make
> b43 unaware it that flags are going to be written in register A or B.
> I wanted b43 to just pass flags to correct function and do not care
> about place where they are getting written.

Ok, well. That's basically what I am talking about for several
days now.

1) Keep SSB and AI separated.
2) Introduce an additional thin layer on top of this to abstract the
bus to the drivers.

-- 
Greetings Michael.

Re: [RFC] How to rename SSB_TMSLOW_*, B43_TMSLOW_*?

[Rafał Miłecki]

W dniu 17 lutego 2011 23:00 użytkownik Michael Büsch <mb@bu3sch.de> napisał:
> On Thu, 2011-02-17 at 13:14 +0100, Rafał Miłecki wrote:
>> That's why I wanted to introduce some SSBAI_CORECTL_FOOBAR and make
>> b43 unaware it that flags are going to be written in register A or B.
>> I wanted b43 to just pass flags to correct function and do not care
>> about place where they are getting written.
>
> Ok, well. That's basically what I am talking about for several
> days now.
>
> 1) Keep SSB and AI separated.
> 2) Introduce an additional thin layer on top of this to abstract the
> bus to the drivers.

OK, but how do you see core flags implemented in such a layout? The
ugly way I proposed seems to match your idea but... is ugly.

Can you say sth more about that? What about such a shared things that
differs in a minimal way?

-- 
Rafał