On Fri, Jul 07, 2023 at 11:35:49AM -0600, Simon Glass wrote:
Hi,
On Tue, 4 Jul 2023 at 03:35, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Hi Simon,
On Mon, Jul 03, 2023 at 02:30:57PM +0100, Simon Glass wrote:
Hi,
On Mon, 3 Jul 2023 at 01:57, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Thu, Jun 29, 2023 at 08:09:58PM +0100, Simon Glass wrote:
Hi AKASHI,
On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Added is a new unit test for SCMI base protocol, which will exercise all the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS. $ ut dm scmi_base It is assumed that test.dtb is used as sandbox's device tree.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
test/dm/scmi.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+)
diff --git a/test/dm/scmi.c b/test/dm/scmi.c index 881be3171b7c..563017bb63e0 100644 --- a/test/dm/scmi.c +++ b/test/dm/scmi.c @@ -16,6 +16,9 @@ #include <clk.h> #include <dm.h> #include <reset.h> +#include <scmi_agent.h> +#include <scmi_agent-uclass.h> +#include <scmi_protocols.h> #include <asm/scmi_test.h> #include <dm/device-internal.h> #include <dm/test.h> @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct unit_test_state *uts) } DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT);
+static int dm_test_scmi_base(struct unit_test_state *uts) +{
struct udevice *agent_dev, *base;struct scmi_agent_priv *priv;const struct scmi_base_ops *ops;u32 version, num_agents, num_protocols, impl_version;u32 attributes, agent_id;char vendor[SCMI_BASE_NAME_LENGTH_MAX],agent_name[SCMI_BASE_NAME_LENGTH_MAX];u8 *protocols;int ret;/* preparation */ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi",&agent_dev));ut_assertnonnull(agent_dev);ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev));ut_assertnonnull(base = scmi_get_protocol(agent_dev,SCMI_PROTOCOL_ID_BASE));ut_assertnonnull(ops = dev_get_driver_ops(base));/* version */ret = (*ops->protocol_version)(base, &version);Can you add uclass helpers to call each of the methods? That is how it is commonly done. You should not be calling ops->xxx directly here.
Yes, I will add inline functions instead.
I don't mean inline...see all the other uclasses which define a
Okay, I will *real* functions.
function which is implemented in the uclass. It is confusing when one uclass does something different. People might copy this style and then the code base diverges. Did you not notice this when looking around the source tree?
But one concern came up in my mind. Contrary to ordinary "device controllers", there exists only a single implementation of driver for each of "udevice"'s associated with SCMI protocols including the base protocol.
So if I follow your suggestion, the code (base.c) might look like:
static int __scmi_base_discover_vendor(struct udevice *dev, u8 *vendor) { ... }
struct scmi_base_ops scmi_base_ops = {
.base_discover_vendor = __scmi_base_discover_vendor,
}
int scmi_base_discover_vendor(struct udevice *dev, u8 *vendor) { struct scmi_base_ops *ops;
ops = scmi_base_dev_ops(dev);
return ops->base_discover_vendor(dev, vendor); } ===
We will have to have similar definitions for every operation in ops. It looks quite weird to me as there are always pairs of functions, like __scmi_base_discover_vendor() and scmi_base_discover_vendor().
Yes I understand that you only have one driver at present. Is there not a sandbox driver?
No. Please remember that SCMI protocol drivers on U-Boot are nothing but stubs that makes a call to SCMI servers, supporting common communication channel interfaces for different transports (either OP-TEE, SMCCC or mailbox).
Sandbox driver, if is properly named, is also implemented as a sort of transport layer, where a invocation is replaced with a function call which mimicks one of specific commands in SCMI protocol on behalf of a real SCMI server.
In this sense, there will exist only a single driver under the current form of framework forever.
We can avoid this redundant code easily by eliminating "ops" abstraction. But as far as I remember, you insist that every driver that complies to U-Boot driver model should have a "ops".
What do you make of this?
Well there are some exceptions, but yes that is the idea. Operations should be in a 'ops' struct and documented and implemented in a consistent way.
Is it your choice that I should keep "ops" structure in this specific implementation?
-Takahiro Akashi
Regards, Simon