-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello,

On Dec 8, 2007, at 05:57, Marco Trillo wrote:

> On 12/7/07, Michael Lorenz <macallan@netbsd.org> wrote:
>> On Dec 7, 2007, at 09:06, Marco Trillo wrote:
>>> On 12/7/07, Michael Lorenz <macallan@netbsd.org> wrote:
>>>> On Dec 6, 2007, at 17:45, Marco Trillo wrote:
>>>>> The problem is that the sound-objects parser uses these callbacks
>>>>> to inform
>>>>> the driver that a device has changed its state (for example when
>>>>> updating the
>>>>> state as a result of a port change interrupt). So the driver
>>>>> registers a callback
>>>>> for each device it cares about. This callback is called when the
>>>>> device changes
>>>>> its state.
>>>>
>>>> So, it registers an interrupt handler? I don't know, my gut feeling
>>>> is that the parser should just parse the property, nothing else.  
>>>> But
>>>> that's just a gut feeling.
>>>
>>> It registers an interrupt handler if the property contains  
>>> references
>>> to extint-gpio detect nodes. In the case of the screamer, the awacs
>>> driver calls the aoa_cintr() handler from its port change interrupt
>>> handler (these type of detects have a 'model' attribute with the  
>>> value
>>> 'InSenseBitsDetect' which differences them of the gpio detects which
>>> have the value 'GPIODetect' or 'GenericGPIODetect').
>>>
>>> Since the property contains all the necessary masks/match values,  
>>> the
>>> intention was for the 'aoa' module to take care of all of this.
>>
>> That might Just Work with i2s but awacs is rather different here, the
>> status interrupt can signal a few more things than just gpio changes.
>> We don't use any of that just yet but we might want to at some point.
>
> The current code should already handle that:
>
> awacs_status_intr(void *cookie)
> {
>  	[...]
> 	reg = awacs_read_reg(sc, AWACS_SOUND_CTRL);
> 	if (reg & AWACS_PORTCHG) {
> 		if (sc->sc_aoa)
> 			aoa_cintr(sc->sc_aoa);
> 		else
> 			awacs_select_output(sc, awacs_check_output(sc));
> 			
> 		/* clear the interrupt */
> 		awacs_write_reg(sc, AWACS_SOUND_CTRL,
> 				sc->sc_soundctl | AWACS_PORTCHG);
> 	}
> 	[...]
> }
>
> So if the interrupt is not AWACS_PORTCHG, the aoa_cintr() function
> should not be called.

*nod*

>>> By the way, my original plan of making the codec drivers
>>> machine-independent files in sys/dev/i2c/ is starting to being  
>>> botched
>>> by the 'daca' setups requiring some configuration for the output
>>> device selection: instead of using gpio mutes like the other codecs,
>>> they use a control in the codec to switch the polarity of the output
>>> depending on what device is being used, the internal mono speaker or
>>> headphones.
>>
>> One more reason to let the codec drivers fiddle with gpios.
>
> Yes, the 'i2s' driver still has to deal with gpios anyway because it
> needs the 'amp-mute', 'headphone-mute' and other mute gpios which are
> not provided in the sound-objects. On the other hand, the i2c codec
> drivers should not need to deal with the gpio stuff, instead they
> (actually only daca) implement a method to 'configure the output
> device'.

I don't know i2s well enough to have a strong opinion here - my gut  
feeling is that the gpios depend more on i2s than anything else  
though. Also, I'm not so sure anymore that putting the DMA stuff into  
a separate driver is such a good idea - too many layers. Maybe just  
have i2s and awacs pull the DMA code in as a dependency?

> Talking about the 'i2s' driver, there is some code in it that looks  
> like this:
>
> out32rb(sc->sc_baseaddr + KEYLARGO_FCR1, x);
>
> However, 'sc_baseaddr' is the physical address of the mac-io space
> (which should be 0x80000000 in the KeyLargo). I think this is working
> because the mac-io space is being mapped by machdep.c using the BAT
> facility:
>
> oea_batinit(0x80000000, BAT_BL_256M, 0xf0000000, BAT_BL_256M,
>     0x90000000, BAT_BL_256M, 0xa0000000, BAT_BL_256M,
>     0xb0000000, BAT_BL_256M, 0);

Exactly, and yes it should go away. obio should map its feature  
control register(s) and provide a function to mess with it or maybe  
something more high level. In an ideal world we would replace all  
direct IO stuff with bus_space_*
On the other hand, this driver is macppc-specific anyway so relying  
on PCI stuff being always accessible at bus address == physical  
address ( for bus address >= 0x80000000 etc. ) won't stop working any  
time soon, at least on 32bit Macs.

> (Well, that's in netbsd-4, I don't know where that code moved to in
> -current, but the snapper driver in -current still uses the physical
> address so I assume that this BAT mapping is still being done.)

It is. The code just moved over to arch/powerpc/oea and is now shared  
at least with prep.

> However I think that BAT mapping will not work on the G5, since it  
> does not provide the BAT facility.

Yeah, we should really finich bus_space-ification of at least those  
drivers we need on the G5. Unfortunately I've never had one in my  
hands so I'm rather clueless what exactly the hw looks like as far as  
NetBSD is concerned. I doubt the KeyLargo FCR will work there though.

> What do you think of making obio(4) map the FCR space and provide
> abstraction functions to read and write to the registers:
>
> obio_write_fcr(OBIO_FCR1, x);

Eh, talk about reading before answering :)
The G5 doesn't have a KeyLargo, does it? We might need something more  
abstract then.

> Another thing related to the codecs... some codecs provide an 'analog
> power down' or 'low power' feature; the current i2s code sets a
> timeout of 20 seconds since the device is closed and then calls the
> poweroff function of the codec.
>
> However I don't think that this policy should be implemented by the
> i2s driver, but with the current audio(4) infrastructure is the only
> possible way (since the audio_hw_if::powerstate member alone is
> unusable, since for example we don't want to power on the codec each
> time a mixer value is changed, and it does not implement any sort of
> timeout).
> Maybe the 'jmcneill-pm' branch provides a better framework in audio(4)
> for implementing this.

I think we should follow Jared's branch right away, one of these days  
I'll hopefully have time for it :/

>>>>> Using properties would work to see the list of available devices,
>>>>> but will not allow
>>>>> the parser to take care of the device state changes (but I may be
>>>>> missing something).
>>>>
>>>> Shouldn't individual drivers take care of that?
>>>
>>> Maybe instead of making the 'aoa' module take care of the interrupt,
>>> the driver can register the handler based on the data from the
>>> property, read the sense bits and then pass the sense bits to an aoa
>>> function.
>>>
>>> This way may be cleaner especially for the awacs driver.
>>
>> Couldn't agree more.
>
> I'm still not very convinced; while this would work fine for awacs
> which does only have a single source of the 'sense bits', gpio-based
> detects can have multiple addresses to read. In the current code all
> of the process is centralized in a single place in aoa.c.

I think the codecs know best which gpios they need and where to look  
for them. Then again, unifying i2s and awacs doesn't look all that  
straight forward anymore.

>>>>>>> At the moment I've tested it with the awacs driver in a  
>>>>>>> screamer-
>>>>>>> based
>>>>>>> G4 (which is also a plain screamer with only an internal speaker
>>>>>>> and a
>>>>>>> headphones port). The good thing about the sound-objects
>>>>>>> property is
>>>>>>> that it eliminates the need to hardcode different GPIO  
>>>>>>> wirings for
>>>>>>> different machines.
>>>>>>
>>>>>> Neither the PB3400c nor the beige G3 have sound-objects but both
>>>>>> need
>>>>>> special wiring for their gpios ( it's reversed on the  
>>>>>> powerbook and
>>>>>> uses a different pin on the G3 as you probably saw in awacs.c )
>>>>>
>>>>> Yeah, I forgot about these especial cases. Well, at least we don't
>>>>> need more
>>>>> special cases for some G4s or some other screamer-based models.
>>>>
>>>> I wouldn't bet on it. Actually I'm pretty sure some more *Books  
>>>> will
>>>> need special attention.
>>>
>>> Hmm... with Apple's inconsistency it will not be a surprise. But
>>> looking at the Darwin's screamer driver the only special case I can
>>> see is the 'init operation' I commented (the programmable outputs).
>>> This apparently only affects G4's.
>>
>> The new world powerbooks will likely have something i2s-based or a
>> Screamer, the older ones on the other hand might be weirder and
>> likely have additional inputs. On the other hand that's probably not
>> much to worry about until one of us actually gets his hands on such
>> hardware. One of these days I'll probably figure out how to talk to
>> the 3400c's modem just for the hell of it ( there's a 2nd ohare mac-
>> io, its interrupt controller cascaded to the onboard ohare. I suspect
>> one of its serial ports is wired to the modem but it appears to need
>> some fiddling in some register somewhere to power up ) - I bet the
>> modem's audio output is wired somehow to the onboard awacs even
>> though it's not a screamer.
>
> Yeah, and it probably uses the loopthrough feature of the awacs so the
> modem sounds can be heared from the standard outputs.

Exactly, Or it might be more Screamer-like than OF tells us, if I  
remember correctly the pb3400c and the beige G3 were more or less  
contemporary so it might even be a Screamer.

>>>>> I can try hardcoding irq 58 for daca-based machines... and if the
>>>>> testing succeeds, then fine.
>>>>
>>>> Hmm, the headphone detect gpio probably doesn't have much to do  
>>>> with
>>>> the codec, more with the mac-io model? My iBook has:
>>>> model                   4141504c 2c4b6579 6c617267 6f00....
>>>> "AAPL,Keylargo"
>>>> probably too generic to be useful.
>>>
>>> Yeah... the tress I know that have this 'extint-gpio12' stuff are  
>>> the
>>> PowerBook2,1 and the PowerBook2,2 daca-based iBook G3's.
>>> But of course the parser should handle this automatically, so no
>>> special cases are required -- except for this 'irq 58' thing.
>>
>> *nod*
>
> So let's hardcode irq 58. If it doesn't work then we can try with irq
> 61, and if that still doesn't work ... either don't implement
> automatic switch at all or use some sort of polling.
> I see that the awacs driver in -current has an 'awacs_thread' that
> seems to be related to this. Maybe a solution like that would be
> feasible for daca.

Awacs uses a thread for that so it can call iic_exec() for the SGS  
mixer found in the beige G3. The interrupt handler just clears the  
interrupt and wakeup()s the thread. Which reminds me, I need to do  
something similar for the PMU, it spends way too much time in its  
interrupt handler.

>>> The 'onyx' and 'topaz' codecs are used in newer G5-based models.  
>>> There
>>> also seem to be machines with two codecs, for example PowerMac G5s
>>> which have a TAS3004 for analog output and a 'topaz' for digital
>>> output.
>>
>> Yeah, the DMA machinery looks like it supports at least 4 channels
>> even on awacs models - probably for video IO on the beige G3 and AV
>> models?
>
> The problem is that the current i2s driver assumes only one codec.
> Well, at least the TAS3004 analog output should work on the G5s.  If
> someone with a G5 is interested, support for the 'onyx' codec in
> analog mode shouldn't be difficult to write either. The digital stuff
> may require some twiddling with the i2s driver...

Hmm, G5s are out of production long enough that it might make sense  
to ask if anyone wants to donate one.

have fun
Michael
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)

iQEVAwUBR1q46cpnzkX8Yg2nAQL5wQf/ZNRM9TIrS6LTDxAHekgjJz7bZzVnsznj
wIy0yt9Q822MLEZeIUQPVBtK2O9dSCkkWcSF5Bypcwj35cFgG8p1hE396ikZgVF9
iECQPicBm/j8qsG98nrv4FqAxTcrqOtX7+FVaCSPcif0GTQxgMGOPVw+PeaLVWJJ
tjN0dPWHShbBaaORRveTSI+gg/c1Zg3AFMgqqpBqQKloExerNMCxLHPb4ay8CV2q
7grg5tWF6gvivN++Ws7yWrA4rP6Mgnumj/cEbCl8OHDizgjAITtqRAVqvvFuuXRR
UeAkADi8Xt4zKHTYo5YUTfEeQoMoSNrVwBpPQpCDdF+eaFniF/2TkQ==
=T0O7
-----END PGP SIGNATURE-----