plat: apl: clock gating adjusted for dai, dma, and cores

[mmaka1]

dai clocks ungated and resources allocated on the first use.
dma clocks ungated and resources allocated on the first use.
cores clocks gated in idle.

Signed-off-by: Marcin Maka marcin.maka@linux.intel.com

[lgirdwood]

Just some question on why probe() would be called twice or more on a device. The driver probe can be called more than once (and would create a new device every time it was called) but I would expect probe() would only be called once per device. Unless I'm missing something ?

[lgirdwood]

We should probably pass index in and combine with clock ID, e.g. CPU_CLOCK_CORE0, etc ?

[lgirdwood]

What's the use case here ? I guess topology could create the device twice in error ?

[mmaka1]

(also applies to the multiple probe question) my assumptions:

• some devices may be created on init, some on demand, the client does not need to know whether the probe() must be called before get() or not, otherwise may try to probe the device that is already created.
• there is no api to 'return' the device back to the pool (calling remove() implicitly inside for some of them) yet, so get-probe sequences may be called again by a destroyed and then re-created topology component.

[mmaka1]

Actually the get() could check whether the device exists before calling probe() and the get_drvdata() test could stay there but result in error (probe had not been protected against double allocation before I guess).

[lgirdwood]

ok thanks, I see - I think the simplest approach for us would be to only probe() devices when added by topology and remove() them when topology is unloaded. This probably can be summarized with some rules (that we could also document).

1. we dont automatically probe() any pipeline device unless its's added by topology.
2. we only remove component device when it's unloaded by topology.
3. component added twice would return an error.
4. active component removal would return an error.
5. pipeline trigger start will fail if pipeline has no valid sink and source endpoints.

The driver would manage the component load/unload and ref count components in pipelines that shared components. @ranj063 is working on driver code for this shortly (PM related)

The thing we have to watch for is non pipeline IO drivers like low level DMACs, SSP, these would need reference counted by host.c and dai.c respectively.

[mmaka1]

@lgirdwood Agree. [Edit: missed last paragraph, removed re-phrased, similar conclusion].
However reference counting in host.c and dai.c might not be sufficient if you add another client and there would be a need for a more "global" reference counting. How about doing this inside specific device driver (dw-dma.c for example)? This approach would require each client (host, dai) to "put" the device, but I planned to add dai_put(), dma_put() calls anyway. Atm there is dma_get-channel_get-channel_put sequence implemented and I planned to balance them inside the _free() ops.

[mmaka1]

@lgirdwood Please take a look at the latest implementation that handles ref counting (simple one, protected by early initialized locks) in the lib layer. Done for the get() side, to be completed on the put() side if this is acceptable direction.

[lgirdwood]

I like the locking update, it's a needed simplification, but still not sure about calling probe() twice as it diverges from the Linux model. It could also be due to a recent? change to the probe() API that

static int hda_dma_probe(struct dma *dma)

returns an int instead of returning a "struct device *". If we returned a struct dma device (which can contain a generic base structure) the we can managed devices at a global level.

[lgirdwood]

It looks like we need some infrastructure to create a global device and driver model.

struct device {
    list_head list; /* in global list of devices */
    struct driver *drv; /* probe(), remove(), pm() contained in a generic struct */
    int type; /* device type */
    int id; /* device ID */
};

The struct driver above would be contained in every driver structure (like SSP, DMIC, DMAC etc) and be something like

struct driver {
   struct device* (*probe) (int id);
   int (*remove) (struct device *dev)
   /* other generic driver ops */
};

This would then align the device model to be very linux like and give us some simple device management and device level PM.

[lgirdwood]

fwiw, a minor improvement would be to

if (d->index != index)
    continue;

This would mean you would not need to newline the trace calls further down as indentation will not be changed.

[mmaka1]

Ok

[lgirdwood]

Should we put some trace here as this could be caused by a bad topology ?

[mmaka1]

Yes, will be added

[lgirdwood]

I like the locking update, it's a needed simplification, but still not sure about calling probe() twice as it diverges from the Linux model. It could also be due to a recent? change to the probe() API that

static int hda_dma_probe(struct dma *dma)

returns an int instead of returning a "struct device *". If we returned a struct dma device (which can contain a generic base structure) the we can managed devices at a global level.

[mmaka1]

@lgirdwood With a simple ref count as implemented now, the probe() is called only once, pls note that probe() returns error code (which is rather a logic then run-time error) if the device already exists. Is this test confusing and should not be there?

Regarding the probe() API, it stays unchanged since 2016 in the code. And I believe it is consistent with the Linux kernel APIs as well.

The driver-device model and API separation sounds great. How about completing the put+remove path now and do the drv/dev refactor in the next iteration?

[lgirdwood]

@mmaka1 your right, I must be thinking about the Linux driver probe() :)