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().
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?
Thanks -Takahiro Akashi
Regards, Simon