Hi James,
On 9/5/25 19:48, James Morse wrote:
> Hi Ben,
>
> On 27/08/2025 14:03, Ben Horgan wrote:
>> On 8/22/25 16:29, James Morse wrote:
>>> Probing MPAM is convoluted. MSCs that are integrated with a CPU may
>>> only be accessible from those CPUs, and they may not be online.
>>> Touching the hardware early is pointless as MPAM can't be used until
>>> the system-wide common values for num_partid and num_pmg have been
>>> discovered.
>>>
>>> Start with driver probe/remove and mapping the MSC.
>
>>> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c
>>> new file mode 100644
>>> index 000000000000..a0d9a699a6e7
>>> --- /dev/null
>>> +++ b/drivers/resctrl/mpam_devices.c
>>> @@ -0,0 +1,336 @@
>
>>> +static int mpam_dt_parse_resource(struct mpam_msc *msc, struct device_node *np,
>>> + u32 ris_idx)
>>> +{
>>> + int err = 0;
>>> + u32 level = 0;
>>> + unsigned long cache_id;
>>> + struct device_node *cache;
>>> +
>>> + do {
>>> + if (of_device_is_compatible(np, "arm,mpam-cache")) {
>>> + cache = of_parse_phandle(np, "arm,mpam-device", 0);
>>> + if (!cache) {
>>> + pr_err("Failed to read phandle\n");
>>> + break;
>>> + }
>> This looks like this allows "arm,mpam-cache" and "arm,mpam-device" to be
>> used on an msc node when there are no ris children. This usage could be
>> reasonable but doesn't match the schema in the previous patch. Should
>> this usage be rejected or the schema extended?
>
> The DT/ACPI stuff is only going to describe the things that make sense at a high level,
> e.g. the controls for the L3. There may be other controls for stuff that doesn't make
> sense in the hardware - these get discovered, grouped as 'unknown' and left alone.
>
> Another angle on this is where there is an MSC that the OS will never make use of, but
> needs to know about to find the system wide minimum value. (there is a comment about
> this in the ACPI spec...)
>
> I don't think its a problem if the magic dt-binding machinery is overly restrictive, that
> is about validating DTB files...
I agree with your points. However, I was rather thinking that the code
allows more ways to describe the same thing than the schema does. In
that, you could write something like:
msc@80000 {
compatible = "foo,a-standalone-msc";
reg = <0x80000 0x1000>;
...
msc@10000 {
compatible = "arm,mpam-msc arm,mpam-cache";
arm,mpam-device = <&mem>;
...
}
}
Although, now I've written this out, it doesn't seem sensible to worry
about this. Using ris compatibles on an msc, as in my example, is
clearly an error.
>
>
[snip]
>>> + } else if (msc->iface == MPAM_IFACE_PCC) {
>>> + msc->pcc_cl.dev = &pdev->dev;
>>> + msc->pcc_cl.rx_callback = mpam_pcc_rx_callback;
>>> + msc->pcc_cl.tx_block = false;
>>> + msc->pcc_cl.tx_tout = 1000; /* 1s */
>>> + msc->pcc_cl.knows_txdone = false;
>>> +
>>> + msc->pcc_chan = pcc_mbox_request_channel(&msc->pcc_cl,
>>> + msc->pcc_subspace_id);
>>> + if (IS_ERR(msc->pcc_chan)) {
>>> + pr_err("Failed to request MSC PCC channel\n");
>>> + err = PTR_ERR(msc->pcc_chan);
>>> + break;
>>> + }
>> I don't see pcc support added in this series. Should we fail the probe
>> if this interface is specified?
>
> I've got patches from Andre P to support it on DT - but the platforms that need it keeping
> popping in and out of existence. I'll pull these bits out - they were intended to check
> the ACPI table wasn't totally rotten...
>
>
>> (If keeping, there is a missing pcc_mbox_free_channel() on the error path.)
>
> When pcc_mbox_request_channel() fails? It already called mbox_free_channel() itself.
Apologies, this was relating to if the *_parse_resources() call below
failed.
>
>
>>> + }
>>> +
>>> + list_add_rcu(&msc->glbl_list, &mpam_all_msc);
>>> + platform_set_drvdata(pdev, msc);
>>> + } while (0);
>>> + mutex_unlock(&mpam_list_lock);
>>> +
>>> + if (!err) {
>>> + /* Create RIS entries described by firmware */
>>> + if (!acpi_disabled)
>>> + err = acpi_mpam_parse_resources(msc, plat_data);
>>> + else
>>> + err = mpam_dt_parse_resources(msc, plat_data);
>>> + }
>>> +
>>> + if (!err && fw_num_msc == mpam_num_msc)
>>> + mpam_discovery_complete();
>>> +
>>> + if (err && msc)
>>> + mpam_msc_drv_remove(pdev);
>>> +
>>> + return err;
>>> +}
[snip]>
> Thanks,
>
> James
Thanks,
Ben
