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

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.

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.

Quoted 4 lines · Viresh KUMAR

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?

Quoted 14 lines · Russell King - ARM Linux

Quoted 4 lines · Viresh KUMAR

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.

Quoted 4 lines

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.

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

Quoted 15 lines · Viresh KUMAR

Quoted 14 lines · Russell King - ARM Linux

Quoted 4 lines · Viresh KUMAR

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.

Quoted 7 lines

Quoted 4 lines

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.

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

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.
(...)

2010/5/14 Viresh KUMAR <[email protected]>:

Quoted 6 lines

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.