RE: Amba bus definition (amba/bus.h)

[<Nick.Thompson@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Russell King - ARM Linux
>
> On Mon, Nov 10, 2008 at 02:32:33PM +0100, Nick.Thompson@xxxxxxxxxxxx
> wrote:
> > > No, drivers can have multiple consumers, and some chips do.  OMAP for
> > > instance has one 'ick' and one 'fck' per peripheral device.
> > >
> > > Most primecells actually have two clock inputs - the bus clock and the
> > > function clock.  In all cases so far, the bus clock isn't visible to
> > > software, so we ignore it.
> > >
> > > So it's not universally true that there is a one-to-one relationship
> > > between struct device and struct clk.
> >
> > This seems counter intuitive to your intention to make AMBA device
> > drivers pass a NULL id to get_clk().
>
> Where there is only one clock per device, the ID is redundant.  So passing
> a driver unique ID just means that we're encouraging the wrong approach
> for clk_get() - encouraging implementations which are contary to the
> API documentation.

True. But drivers that require more than one clock will have to pass the consumer ID in each of several get_clk calls. So some drivers will pass an ID and some won't. That was the inconsistency I was referring to - requiring an extra test in get_clk.

>
> > Are you not creating a inconsistency between AMBA devices and other
> > devices which also call get_clk()?
>
> No, because the API already calls for the device and the consumer ID.

One of which you will no longer pass.

>
> > > If they were to have to pass in the clock names via platform data,
> > > they'd be having to create lots of platform_data structures for each
> > > device to pass the clock names in, and then they've got to maintain:
> > >
> > > 1. a list of struct clk's.
> > > 2. a set of platform devices.
> > > 3. a set of platform device data for every platform device to specify
> > >    which clks to use.
> > >
> > > So, why have the additional names for struct clks and additional
> > > structures that require manual maintanence when you already have a
> > > set of static names for devices and can match by those names instead?
> >
> > Great, but then the get_clk() code has to maintain a list of device name
> > to clk struct mappings. I wouldn't necessarily expect to have device
> > related data, arguably hidden, in the clock code.
>
> They already maintain a consumer ID name to clk mapping.  It's just a
> different (but more correct) string.

Agreed. Many get_clk implementations will break when you do it :(

>
> > The device definition seems like a natural place to put data about
> > device connectivity information, just like irq numbers and device
> > bus addresses (chip select connectivity). Clock usage just seems
> > to me like similar connectivity data for the device.
>
> So you want to see drivers contain code like I've showed you in my
> previous mail?

If you're going to have differing clock names on each machine/cpu/platform/whatever, you need to have that conditional code somewhere, no? So get_clk will have:

If dev->bus_id = "xyz"
        If cpu_is_a()
                Clk = &clk_one;
        Else if cpu_is_b()
                Clk = &clk_two;
        Etc.
Else if dev->bus_id = ...
        ...

Return clk

But no, I don't what it in the driver, or the get_clk code - I'm suggesting I'd want it in the device definition. But that can't be done, so I'll implement get_clk code as above.

>
> > Having this info in the device allows for the device definitions to
> > change as the SoC evolves, quite possibly without recompiling the
> > kernel.
>
> You mean like the mess which OMAP has ended up with.
>
> > The devices can be recompiled and dynamically loaded or the irq and
> > clk data even set as module parameters.
>
> That's quite insane actually - that means that userspace needs to be
> configured, or even the boot loader with platform specific information
> which is also kernel specific as well.  So, when you update the kernel
> you could well be facing having to also update the boot loader and the
> filesystem together.

I'm not sure I see what the bootloader has to do with it, but yes, I am suggesting it could require a filesystem change - I install and modprobe new device drivers on PC based Linux from time to time and don't even reboot the kernel. You are right that I need to recompile the drivers when the kernel changes though. I might be insane though - I'll give you that.

>
> The vast majority of people are scared of touching the boot loader!
>
> > The clock code would only change if the clock architecture changed.
> > Even new devices, not accounted for in get_clk, could be added
> > without recompiling or even rebooting the kernel.
>
> I don't that the the clk API stands in the way of that at present.

Because it will not work unless the clock code already knows about the device name or consumer ID and what the correct clock mapping is. It would work if the device can supply the mapping at run time - but I understand that it can't.

As the solution to clock handling is clear (write a better get_clk), can I ask what you think the way forward might be for the IRQF flags? Should I just add device specific code to reconfigure the irq registration, or do you think there is something that could be done to accommodate alternative IRQF flag settings in the drivers?

Many thanks,
Nick.

-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php