Should we pass amba device peripheral id with device structure or not?

Should we pass amba device peripheral id with device structure or not?

[Viresh KUMAR]

Hello,

amba_device_register function reads and updates peripheral id from hardware
registers, whenever we register any amba device. If clock to device is disabled,
then amba_device_register will not be able to read and update this value.

Thus device registration will fail. Now we can also pass peripheral
id with the device structure, which will let it pass.
But i remember in SPEAr patches review, i got this comment to remove
peripheral id from device structure as it will be populated
automatically.

This is happening in our platform as we don't have separate control of
enabling/disabling interface and functional clocks of peripherals.

What should we do now? Enabling clocks from uboot doesn't looks a good idea.
Should we pass peripheral id with device structure only?

regards,
viresh kumar.

Should we pass amba device peripheral id with device structure or not?

[Catalin Marinas]

On Fri, 2010-05-14 at 08:02 +0100, Viresh KUMAR wrote:
> amba_device_register function reads and updates peripheral id from hardware
> registers, whenever we register any amba device. If clock to device is disabled,
> then amba_device_register will not be able to read and update this value.
> 
> Thus device registration will fail. Now we can also pass peripheral
> id with the device structure, which will let it pass.
> But i remember in SPEAr patches review, i got this comment to remove
> peripheral id from device structure as it will be populated
> automatically.

The peripheral id is usually only present in PrimeCell devices and it's
not a feature of the AMBA bus. So if your device doesn't have such id
(or it isn't readable) I think it makes sense to pass one in the
amba_device structure.

-- 
Catalin

Should we pass amba device peripheral id with device structure or not?

[Russell King - ARM Linux]

On Fri, May 14, 2010 at 12:32:18PM +0530, Viresh KUMAR wrote:
> amba_device_register function reads and updates peripheral id from
> hardware registers, whenever we register any amba device. If clock
> to device is disabled, then amba_device_register will not be able
> to read and update this value.

This is a potential problem - if the drivers are already initialized
in the kernel, then the drivers will try to initialize as soon as
amba_device_register() is called.  If the registers aren't accessible
at amba_device_register() time, the driver initialization could fail.

I think it's better to understand what's going on here before making
suggestions.

The clks in the primecell drivers are for the external side clocks
only; these drivers all make the assumption that the AMBA bus clock
is always enabled.  Does your SoC turn the AMBA bus clock to peripherals
on and off?

If so, we need to expand drivers/amba/bus.c to deal with this.

Should we pass amba device peripheral id with device structure or not?

[Viresh KUMAR]

On 5/22/2010 1:08 AM, Russell King - ARM Linux wrote:
> On Fri, May 14, 2010 at 12:32:18PM +0530, Viresh KUMAR wrote:
>> amba_device_register function reads and updates peripheral id from
>> hardware registers, whenever we register any amba device. If clock
>> to device is disabled, then amba_device_register will not be able
>> to read and update this value.
> 
> This is a potential problem - if the drivers are already initialized
> in the kernel, then the drivers will try to initialize as soon as
> amba_device_register() is called.  If the registers aren't accessible
> at amba_device_register() time, the driver initialization could fail.
> 
> I think it's better to understand what's going on here before making
> suggestions.
> 
> The clks in the primecell drivers are for the external side clocks
> only; these drivers all make the assumption that the AMBA bus clock
> is always enabled.  Does your SoC turn the AMBA bus clock to peripherals
> on and off?

There is only one bit per peripheral to enable/disable clock.
So with clocks disabled, we get 0x00000000 on read from device registers.

Should we pass amba device peripheral id with device structure or not?

[Linus Walleij]

2010/5/14 Viresh KUMAR <viresh.kumar@st.com>:

> amba_device_register function reads and updates peripheral id from hardware
> registers, whenever we register any amba device. If clock to device is disabled,
> then amba_device_register will not be able to read and update this value.
>
> Thus device registration will fail.
> (...)

In the U300 arch/arm/mach-u300/clock.c we have this code:

/*
 * These are the clocks for cells registered as primecell drivers
 * on the AMBA bus. These must be on during AMBA device registration
 * since the bus probe will attempt to read magic configuration
 * registers for these devices. If they are deactivated these probes
 * will fail.
 *
 *
 * Please note that on emif, both RAM and NAND is connected in dual
 * RAM phones. On single RAM phones, ram is on semi and NAND on emif.
 *
 */
void u300_clock_primecells(void)
{
        clk_enable(&intcon_clk);
        clk_enable(&uart_clk);
#ifdef CONFIG_MACH_U300_BS335
        clk_enable(&uart1_clk);
#endif
        clk_enable(&spi_clk);

        clk_enable(&mmcsd_clk);

}
EXPORT_SYMBOL(u300_clock_primecells);

void u300_unclock_primecells(void)
{

        clk_disable(&intcon_clk);
        clk_disable(&uart_clk);
#ifdef CONFIG_MACH_U300_BS335
        clk_disable(&uart1_clk);
#endif
        clk_disable(&spi_clk);
        clk_disable(&mmcsd_clk);

}
EXPORT_SYMBOL(u300_unclock_primecells);


When we add the primecells we have this piece:

        /* Register the AMBA devices in the AMBA bus abstraction layer */
        u300_clock_primecells();
        for (i = 0; i < ARRAY_SIZE(amba_devs); i++) {
                struct amba_device *d = amba_devs[i];
                amba_device_register(d, &iomem_resource);
        }
        u300_unclock_primecells();

This way the reference counter is still zero and the clocks are off
when the drivers probe, so they need to enable their own clocks
at the latter stage.

It's a bit brutal but it works. (And yes, we shouldn't be exporting the
symbols, no point, will fix it someday.) However it messes with the
internals of your clock implementation indeed.

One more elegant way would be to pass the master clock along
in struct amba_device like we do with IRQs, so the probe code in
drivers/amba/bus.c know how to clock the device before attempting
to read the PrimeCell IDs. I don't know if that's particularly elegant
though.

Yours,
Linus Walleij

Should we pass amba device peripheral id with device structure or not?

[Russell King - ARM Linux]

On Mon, May 24, 2010 at 10:08:52AM +0530, Viresh KUMAR wrote:
> On 5/22/2010 1:08 AM, Russell King - ARM Linux wrote:
> > On Fri, May 14, 2010 at 12:32:18PM +0530, Viresh KUMAR wrote:
> >> amba_device_register function reads and updates peripheral id from
> >> hardware registers, whenever we register any amba device. If clock
> >> to device is disabled, then amba_device_register will not be able
> >> to read and update this value.
> > 
> > This is a potential problem - if the drivers are already initialized
> > in the kernel, then the drivers will try to initialize as soon as
> > amba_device_register() is called.  If the registers aren't accessible
> > at amba_device_register() time, the driver initialization could fail.
> > 
> > I think it's better to understand what's going on here before making
> > suggestions.
> > 
> > The clks in the primecell drivers are for the external side clocks
> > only; these drivers all make the assumption that the AMBA bus clock
> > is always enabled.  Does your SoC turn the AMBA bus clock to peripherals
> > on and off?
> 
> There is only one bit per peripheral to enable/disable clock.
> So with clocks disabled, we get 0x00000000 on read from device registers.

So... that must mean your hardware gates both the peripheral clock and
the per-primecell bus clock together.  Let's hope that the bus clock
control takes notice of any in-progress bus transaction...

However, I'm still concerned - the driver's use of clk_enable/clk_disable
is based on the assumption that these calls do not affect the bus clock -
we expect to be able to write to registers before the first clk_enable()
call.

And as I've said (and you cut off of the quote) if we have SoCs where the
bus clock is controllable, we need amba/bus.c to deal with that situation.

Okay, I'll look at addressing that _after_ this merge window has closed.

Should we pass amba device peripheral id with device structure or not?

[Linus Walleij]

[Viresh]

2010/5/24 Viresh KUMAR <viresh.kumar@st.com>:

> There is only one bit per peripheral to enable/disable clock.
> So with clocks disabled, we get 0x00000000 on read from device registers.

You're lucky to get zero, I get an AHB bus error and everything
locks up.

Yours,
Linus Walleij

Should we pass amba device peripheral id with device structure or not?

[Linus Walleij]

[Russell]

> So... that must mean your hardware gates both the peripheral clock and
> the per-primecell bus clock together. ?Let's hope that the bus clock
> control takes notice of any in-progress bus transaction...
>
> However, I'm still concerned - the driver's use of clk_enable/clk_disable
> is based on the assumption that these calls do not affect the bus clock -
> we expect to be able to write to registers before the first clk_enable()
> call.
>
> And as I've said (and you cut off of the quote) if we have SoCs where the
> bus clock is controllable, we need amba/bus.c to deal with that situation.

This is an interesting thing.

In the U300 the bus clock is gated by one control bit per block.

However we cannot gate the peripheral clock. (Or it's just wired
up to be gated by the same control bit, I'd have to check.)

So through the clock framework we enable/disable the bus
clock, and if we issue get_rate() we return the rate of the peripheral
clock.

Thus clk enable/disable and get_rate are actually orthogonal in
our implementation which is not so nice.

We'd have to add a proper bus clock to all PrimeCells to have this in
some sane state I believe.

Yours,
Linus Walleij

Should we pass amba device peripheral id with device structure or not?

[Russell King - ARM Linux]

On Tue, May 25, 2010 at 12:17:02AM +0200, Linus Walleij wrote:
> [Russell]
> We'd have to add a proper bus clock to all PrimeCells to have this in
> some sane state I believe.

That's basically what I'm saying.  At the bus/driver level, we need to
deal with the bus clock dependency separately from the peripheral clock
if the bus clock can be shut off.

I had originally anticipated that all primecells on a bus would have
their bus clock fed from a common unmaskable source, and that shutting
off the bus clock to the primecell would be a very bad thing to happen
to the bus - but it seems that it's permitted after all.

This will be trivial to deal with for clkdev-based platforms...  The
non-clkdev are an entirely separate proposition - but I don't think we
have any primecell-using platforms which don't use clkdev.

Should we pass amba device peripheral id with device structure or not?

[Viresh KUMAR]

On 5/25/2010 3:02 AM, Russell King - ARM Linux wrote:
> On Mon, May 24, 2010 at 10:08:52AM +0530, Viresh KUMAR wrote:
>> On 5/22/2010 1:08 AM, Russell King - ARM Linux wrote:
>>> On Fri, May 14, 2010 at 12:32:18PM +0530, Viresh KUMAR wrote:
>>>> amba_device_register function reads and updates peripheral id from
>>>> hardware registers, whenever we register any amba device. If clock
>>>> to device is disabled, then amba_device_register will not be able
>>>> to read and update this value.
>>>
>>> This is a potential problem - if the drivers are already initialized
>>> in the kernel, then the drivers will try to initialize as soon as
>>> amba_device_register() is called.  If the registers aren't accessible
>>> at amba_device_register() time, the driver initialization could fail.
>>>
>>> I think it's better to understand what's going on here before making
>>> suggestions.
>>>
>>> The clks in the primecell drivers are for the external side clocks
>>> only; these drivers all make the assumption that the AMBA bus clock
>>> is always enabled.  Does your SoC turn the AMBA bus clock to peripherals
>>> on and off?
>>
>> There is only one bit per peripheral to enable/disable clock.
>> So with clocks disabled, we get 0x00000000 on read from device registers.
> 
> So... that must mean your hardware gates both the peripheral clock and
> the per-primecell bus clock together.  Let's hope that the bus clock
> control takes notice of any in-progress bus transaction...
> 
> However, I'm still concerned - the driver's use of clk_enable/clk_disable
> is based on the assumption that these calls do not affect the bus clock -
> we expect to be able to write to registers before the first clk_enable()
> call.
> 
> And as I've said (and you cut off of the quote) if we have SoCs where the
> bus clock is controllable, we need amba/bus.c to deal with that situation.
> 
> Okay, I'll look at addressing that _after_ this merge window has closed.
> 

OK.

Should we pass amba device peripheral id with device structure or not?

[Viresh KUMAR]

On 5/25/2010 1:44 AM, Linus Walleij wrote:
> 2010/5/14 Viresh KUMAR <viresh.kumar@st.com>:
> 
>> amba_device_register function reads and updates peripheral id from hardware
>> registers, whenever we register any amba device. If clock to device is disabled,
>> then amba_device_register will not be able to read and update this value.
>>
>> Thus device registration will fail.
>> (...)
> 
> In the U300 arch/arm/mach-u300/clock.c we have this code:
> 
> /*
>  * These are the clocks for cells registered as primecell drivers
>  * on the AMBA bus. These must be on during AMBA device registration
>  * since the bus probe will attempt to read magic configuration
>  * registers for these devices. If they are deactivated these probes
>  * will fail.
>  *
>  *
>  * Please note that on emif, both RAM and NAND is connected in dual
>  * RAM phones. On single RAM phones, ram is on semi and NAND on emif.
>  *
>  */
> void u300_clock_primecells(void)
> {
>         clk_enable(&intcon_clk);
>         clk_enable(&uart_clk);
> #ifdef CONFIG_MACH_U300_BS335
>         clk_enable(&uart1_clk);
> #endif
>         clk_enable(&spi_clk);
> 
>         clk_enable(&mmcsd_clk);
> 
> }
> EXPORT_SYMBOL(u300_clock_primecells);
> 
> void u300_unclock_primecells(void)
> {
> 
>         clk_disable(&intcon_clk);
>         clk_disable(&uart_clk);
> #ifdef CONFIG_MACH_U300_BS335
>         clk_disable(&uart1_clk);
> #endif
>         clk_disable(&spi_clk);
>         clk_disable(&mmcsd_clk);
> 
> }
> EXPORT_SYMBOL(u300_unclock_primecells);
> 
> 
> When we add the primecells we have this piece:
> 
>         /* Register the AMBA devices in the AMBA bus abstraction layer */
>         u300_clock_primecells();
>         for (i = 0; i < ARRAY_SIZE(amba_devs); i++) {
>                 struct amba_device *d = amba_devs[i];
>                 amba_device_register(d, &iomem_resource);
>         }
>         u300_unclock_primecells();
> 
> This way the reference counter is still zero and the clocks are off
> when the drivers probe, so they need to enable their own clocks
> at the latter stage.
> 
> It's a bit brutal but it works. (And yes, we shouldn't be exporting the
> symbols, no point, will fix it someday.) However it messes with the
> internals of your clock implementation indeed.
> 
> One more elegant way would be to pass the master clock along
> in struct amba_device like we do with IRQs, so the probe code in
> drivers/amba/bus.c know how to clock the device before attempting
> to read the PrimeCell IDs. I don't know if that's particularly elegant
> though.
> 

Thanks Linus,

I will check which one will be more suitable for us.

viresh.