lib: system: zephyr: Use default metal_generic_bus

[glneo]

The only difference is the DMA ops, which the generic ops turns into simple cache operation which zephyr does support.

[arnopo]

Code around metal_generic_dev_xxx is quite old and odd
As you mention map/unmap operation in zephyr is not supported. So seems better to having a structure that points to the ops. Except if you see a blocking point that would require this update...

[glneo]

No blocking point, this was just a general cleanup I noticed while fixing other things.

I still think this is valid though, the default handler does work for Zephyr as it just assumes a 1:1 mapping. And the default does the memory fences and cache operations one would expect. Having this set as NULL causes the functions to just return -ENODEV instead.

Zephyr could support an actual implementation of this function, but maybe a better question first, why is metal_dma_(un)map() needed at all? Baremetal and RTOS will almost never have non-1:1 memory mapping, and any caching can be done with metal_cache_*(). HLOS will not allow this from userspace. So in both cases this will always be a NOP. And I see no users of these functions, maybe time to start the process to deprecate these out?

[arnopo]

No blocking point, this was just a general cleanup I noticed while fixing other things.

I still think this is valid though, the default handler does work for Zephyr as it just assumes a 1:1 mapping. And the default does the memory fences and cache operations one would expect. Having this set as NULL causes the functions to just return -ENODEV instead.

I think it is ok to return -ENODEV as calling in such case metal_dma_map does not make sense.

Zephyr could support an actual implementation of this function, but maybe a better question first, why is metal_dma_(un)map() needed at all? Baremetal and RTOS will almost never have non-1:1 memory mapping, and any caching can be done with metal_cache_*(). HLOS will not allow this from userspace. So in both cases this will always be a NOP. And I see no users of these functions, maybe time to start the process to deprecate these out?

The support of the Linux userspace makes the libmetal quite complexe. This is an example: the map unmap is used here https://github.com/OpenAMP/libmetal/blob/main/lib/system/linux/device.c#L537

Apply here a deprecation process seems not simple here. We would have to put a warning in the release to inform user on a risk for cachable memory.

@tnmysh @edmooring any opinion?

[tnmysh]

No blocking point, this was just a general cleanup I noticed while fixing other things.
I still think this is valid though, the default handler does work for Zephyr as it just assumes a 1:1 mapping. And the default does the memory fences and cache operations one would expect. Having this set as NULL causes the functions to just return -ENODEV instead.

I think it is ok to return -ENODEV as calling in such case metal_dma_map does not make sense.

Zephyr could support an actual implementation of this function, but maybe a better question first, why is metal_dma_(un)map() needed at all? Baremetal and RTOS will almost never have non-1:1 memory mapping, and any caching can be done with metal_cache_*(). HLOS will not allow this from userspace. So in both cases this will always be a NOP. And I see no users of these functions, maybe time to start the process to deprecate these out?

The support of the Linux userspace makes the libmetal quite complexe. This is an example: the map unmap is used here https://github.com/OpenAMP/libmetal/blob/main/lib/system/linux/device.c#L537

Apply here a deprecation process seems not simple here. We would have to put a warning in the release to inform user on a risk for cachable memory.

@tnmysh @edmooring any opinion?

Agree with Arnaud.

[edmooring]

No blocking point, this was just a general cleanup I noticed while fixing other things.
I still think this is valid though, the default handler does work for Zephyr as it just assumes a 1:1 mapping. And the default does the memory fences and cache operations one would expect. Having this set as NULL causes the functions to just return -ENODEV instead.

I think it is ok to return -ENODEV as calling in such case metal_dma_map does not make sense.

Zephyr could support an actual implementation of this function, but maybe a better question first, why is metal_dma_(un)map() needed at all? Baremetal and RTOS will almost never have non-1:1 memory mapping, and any caching can be done with metal_cache_*(). HLOS will not allow this from userspace. So in both cases this will always be a NOP. And I see no users of these functions, maybe time to start the process to deprecate these out?

The support of the Linux userspace makes the libmetal quite complexe. This is an example: the map unmap is used here https://github.com/OpenAMP/libmetal/blob/main/lib/system/linux/device.c#L537

Apply here a deprecation process seems not simple here. We would have to put a warning in the release to inform user on a risk for cachable memory.

@tnmysh @edmooring any opinion?

Agreed. We are between a rock and a hard place because of the demands of supporting Linux user space. It may be that the kernel's support for userspace drivers has advanced enough in the 8 years since libmetal was originally implemented that there is a simpler way now, but that is far out of the scope of what this PR is doing.

[edmooring]

This looks good to go.

[github-actions]

This pull request has been marked as a stale pull request because it has been open (more than) 45 days with no activity.

[arnopo]

Hi @glneo,

Sorry for the delay...

Reviewing this PR i still not convince of the benefit. Look to me that it just introduces opacity by defining dev_dma_map/unmap ops, with associated useless extra footprint (even if low).

@tnmysh, @edmooring , could you have a look?

[tnmysh]

If we don't have a real user that actually needs dma_map/unmap ops, then it makes sense not to introduce them. User can override them as needed in their respective application.

[glneo]

It is useless, yes, but so is all the metal_generic_dev_xxx() and metal_shmem_xxx() APIs for all users outside of that one Linux userspace demo.

Plan is to chip away at this, maybe move the bits needed for that Linux userspace demo into that demo and out of libmetal. But I'm taking it one bit at a time (unless you want to me to rip that all out in one go, I don't mind either way).

[tnmysh]

It is useless, yes, but so is all the metal_generic_dev_xxx() and metal_shmem_xxx() APIs for all users outside of that one Linux userspace demo.

Plan is to chip away at this, maybe move the bits needed for that Linux userspace demo into that demo and out of libmetal. But I'm taking it one bit at a time (unless you want to me to rip that all out in one go, I don't mind either way).

Instead of ripping things out it should be possible to provide compile time switch(es) to get what's needed for open-amp and keep rest of it as it is so it doesn't break backward compatibility.

[glneo]

Instead of ripping things out it should be possible to provide compile time switch(es) to get what's needed for open-amp and keep rest of it as it is so it doesn't break backward compatibility.

For anything that would change backwards compat I will follow the usual deprecation processes. For this series that is not an issue as the behavior is not changed.

[github-actions]

This pull request has been marked as a stale pull request because it has been open (more than) 45 days with no activity.

[glneo]

Not stale.

[tnmysh]

@glneo Is there active user or example where metal_dma_(un)map() are used via metal device in zephyr?

[glneo]

There can't be any users on Zephyr right now as calling metal_dma_map() would fail with ENODEV, that is what this series fixes.

[tnmysh]

@glneo ,

We don't expect libmetal bus usage a lot on zephyr. As usually zephyr has its own device structure. And so it is preferred to use zephyr device over metal device and metal bus. In my opinion we can keep this PR open until we see actual requirement. Feel free to link a use case where default ops are overridden and dma ops are assigned. Others can ack and merge this PR if they see fit.

[glneo]

I agree with you, it is not used, neither as it is now, or with the default ops, it will not be used, it is dead code, hence this PR removing it. And yes when this struct is removed it will fall back to a different set of code that will also never be used in Zephyr. Stating that this is never used so it cannot be removed does not make sense to me.

[tnmysh]

@glneo , only problem is it will contribute to increase in final binary size of libmetal because now dma-ops are included that will never be used. So it's better to keep the code in place and having less binary size than to remove the code and increase the binary size. @arnopo has raised the same concern as well.

[github-actions]

This pull request has been marked as a stale pull request because it has been open (more than) 45 days with no activity.