On 24/02/2025 09:01, Tudor Ambarus wrote:
> The ACPM firmware saves debug information to SRAM. Add debugfs entries
> in order to expose the ACPM logs.
>
> acpm_framework/logb_gprio_level controls the ACPM print verbosity to
> the SRAM log buffer. It encodes a 64 bit value, 4 bits for each of the
> 16 Plugin IDs, with verbosity levels from 0xf (log error) to
> 0x0 (log debug).
>
> echo 0xffffffffffffff1f > /sys/kernel/debug/acpm_framework/logb_gprio_level
> Will allow only LOG_ERR prints for all Plugin IDs but Plugin ID 1,
> which will issue prints for any log levels greater or equal to 1.
> On the ACPM firmware side, logb_gprio_level has a default value of zero,
> all logs enabled for all Plugin IDs.
>
> acpm_framework/log_level has a maximum value of 2 and controls which
> SRAM log buffers are printed.
>
> Finally, acpm_framework/acpm_debug_cmd provides a way to issue
> ACPM DEBUG commands to the firmware.
Please add something like above also as a comment to the driver, so the
interface will be documented.
>
> Add ACPM debugfs support with the above capabilities.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>
> ---
...
> +
> +union acpm_log_entry {
> + u32 raw[4];
> + struct {
> + u32 systicks0 : 24;
> + u32 dummy : 2;
> + u32 is_err : 1;
> + u32 is_raw : 1;
> + u32 plugin_id : 4;
> + u32 systicks24;
> + u32 msg : 24;
> + u32 systicks56 : 8;
> + u32 data;
> + } __packed;
> +};
> +
> +static struct dentry *rootdir;
exynos-apcm.c is not a singleton, so neither should this be. You should
create entries per device (so with device name as subdirectory), just
for correctness.
> +
> +static DEFINE_MUTEX(acpm_log_level_mutex);
And this also looks per-device-instance.
> +
> +static void acpm_log_print_entry(struct acpm_info *acpm,
> + const union acpm_log_entry *log_entry)
> +{
> + u64 systicks, time, msg;
> +
> + if (log_entry->is_err)
> + return;
> +
> + if (log_entry->is_raw) {
> + dev_info(acpm->dev, "[ACPM_FW raw] : id:%u, %x, %x, %x\n",
> + log_entry->plugin_id, log_entry->raw[1],
> + log_entry->raw[2], log_entry->raw[3]);
> + } else {
> + systicks = ((u64)(log_entry->systicks56) << 56) +
> + ((u64)(log_entry->systicks24) << 24) +
> + log_entry->systicks0;
> +
> + /* report time in ns */
> + time = mul_u64_u32_div(systicks, ACPM_APM_SYSTICK_PERIOD_PS,
> + 1000);
> +
> + msg = readl(acpm->sram_base + log_entry->msg);
> +
> + dev_info(acpm->dev, "[ACPM_FW] : %llu id:%u, %s, %x\n", time,
> + log_entry->plugin_id, (char *)&msg, log_entry->data);
I don't think these should be printed to dmesg - these are not system
logs. You either return the contents to the caller's read() on debugfs
entry or, if this is anyhow crashdump related, it goes to
pstore/minidump once triggered. Or to ramoops.
Depends what these logs are (so please also explain what do you find
there in the commit msg).
Maybe something like CHROMEOS_PSTORE?
IOW, if enabled, this should go to ramoops/pstore unconditionally. For
runtime debugging this should be returned somehow to the userspace
reading the file. I think usually debugfs and sysfs is not expected to
provide more than PAGE_SIZE data, so this second part has to be
rethinked still.
> + }
> +}
> +
> +static void acpm_log_print_entries(struct acpm_info *acpm,
> + struct acpm_log_buf *lbuf)
> +{
> + union acpm_log_entry log_entry = {0};
> + u32 front, rear;
> +
> + front = readl(lbuf->q.front);
> + rear = lbuf->rear_index;
> +
> + while (rear != front) {
> + __ioread32_copy(&log_entry, lbuf->q.base + lbuf->mlen * rear,
> + sizeof(log_entry) / 4);
> +
> + acpm_log_print_entry(acpm, &log_entry);
> +
> + if (lbuf->qlen == rear + 1)
> + rear = 0;
> + else
> + rear++;
> +
> + lbuf->rear_index = rear;
> + front = readl(lbuf->q.front);
> + }
> +}
> +
> +static void acpm_log_print(struct acpm_info *acpm)
> +{
> + struct acpm_log_info *acpm_log = acpm->log;
> +
> + guard(mutex)(&acpm_log_level_mutex);
> +
> + if (acpm_log->level == 0)
> + return;
> +
> + if (acpm_log->level == ACPM_LOG_LEVEL_MAX)
> + acpm_log_print_entries(acpm, &acpm_log->preempt);
> +
> + acpm_log_print_entries(acpm, &acpm_log->normal);
> +}
> +
> +static void acpm_work_fn(struct work_struct *work)
> +{
> + struct acpm_log_info *acpm_log =
> + container_of(work, struct acpm_log_info, work.work);
> + struct acpm_info *acpm = acpm_log->acpm;
> +
> + acpm_log_print(acpm);
> +
> + queue_delayed_work(acpm_log->wq, &acpm_log->work,
> + msecs_to_jiffies(acpm_log->poll_period));
> +}
> +
> +static int acpm_log_level_get(void *data, u64 *val)
> +{
> + struct acpm_info *acpm = data;
> +
> + *val = acpm->log->level;
> +
> + return 0;
> +}
> +
> +static int acpm_log_level_set(void *data, u64 val)
> +{
> + struct acpm_info *acpm = data;
> + struct acpm_log_info *acpm_log = acpm->log;
> +
> + if (val > ACPM_LOG_LEVEL_MAX) {
> + dev_err(acpm->dev, "Log level %llu out of range [0:%u]!\n",
> + val, ACPM_LOG_LEVEL_MAX);
> + return -EINVAL;
> + }
> +
> + scoped_guard(mutex, &acpm_log_level_mutex)
> + acpm_log->level = val;
> +
> + if (acpm_log->level == 0)
> + cancel_delayed_work_sync(&acpm_log->work);
> + else
> + queue_delayed_work(acpm_log->wq, &acpm_log->work,
> + msecs_to_jiffies(acpm_log->poll_period));
> + return 0;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(acpm_log_level_fops, acpm_log_level_get,
> + acpm_log_level_set, "0%llu\n");
I also do not think debugfs is a knob to control loglevel of messages
going to dmesg.
Best regards,
Krzysztof
