Re: How to implement PSC driver on Au1550

Thanks, Garett and Tsutsui!

Garrett D'Amore wrote:
> This looks good.  One obvious point, you misspelled "unable" in the
> error message below.  (search for "unbale").  :-)  Also there was an
> indent error in aupsc_attach (see the line that starts bus_space_write.)

Thank you for your advice. I'll fix.

> What exactly are you planning on locking, though?  It looks to me like
> the registers for each PSC are independent from each other, so you
> shouldn't need to lock them.

For now, we can't decide it. I'll remove at once.

> Hmm... I'm also not sure about the clock selection logic in general. 
> How do you know which clock to select?  That seems to be a board
> specific configuration option.  Maybe another set of board_info() ops? 
> Or another locator?  I don't know what the right option is.

I think so. We may need to prepare board-specific clock configuration
if BIOS(ROM monitor) such as YAMON and so didn't/couldn't initialize
registers related with clock.

> (In any
> case,  it seems likely that you need expose some aupsc operations to
> individual drivers -- e.g. aupsc_enable()/disable()/suspend(),
> aupsc_set_protocol(), aupsc_set_clock() maybe?


Izumi Tsutsui wrote:
> More cosmetics:
> - some more space/tab pasto
> - use uintXX_t rather than u_intXX_t
> - no parenthesis are needed for return values
>   (even if other sources have them ;-)

Thank you for your advice. I'll fix it.

> Maybe we should think how much board specific clock variables
> are needed:
> - master clock frequency for internal clock source (peripheral clock?)
> - PSCn_EXTCLK frequencies
> - etc.
> Or we should also have devprop_set() (or so) to define frequencies
> and divider values for clock generater etc. in board specific
> init functions?

Maybe OMS-AL400 uses internal clock source (pscn_intclk).
On OMS-AL400, Initial value of PSC_SEL register is 0x0.

>> (In any
>> case,  it seems likely that you need expose some aupsc operations to
>> individual drivers -- e.g. aupsc_enable()/disable()/suspend(),
>> aupsc_set_protocol(), aupsc_set_clock() maybe?
>
> For now, we don't have to have enable/disable/suspend, and
> set_protocol() and set_clock() can be done in attach functions
> in children, I think.

Ok. Anyway put off this problem later.

-- 
Kind Regards,
--- shige
Shigeyuki Fukushima <shige@{FreeBSD,jp.FreeBSD,NetBSD}.org>