Re: [PATCH] Introduce Tyr

["Maíra Canal" <mcanal@igalia.com>]

Hi Daniel,

On 27/06/25 19:34, Daniel Almeida wrote:

[...]

> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..2443620e10620585eae3d57978e64d2169a1b2d1
> --- /dev/null
> +++ b/drivers/gpu/drm/tyr/driver.rs
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0 or MIT
> +
> +use core::pin::Pin;
> +
> +use kernel::bits::bit_u32;
> +use kernel::c_str;
> +use kernel::clk::Clk;
> +use kernel::device::Core;
> +use kernel::devres::Devres;
> +use kernel::drm;
> +use kernel::drm::ioctl;
> +use kernel::io;
> +use kernel::io::mem::IoMem;
> +use kernel::new_mutex;
> +use kernel::of;
> +use kernel::platform;
> +use kernel::prelude::*;
> +use kernel::regulator;
> +use kernel::regulator::Regulator;
> +use kernel::sync::Arc;
> +use kernel::sync::Mutex;
> +use kernel::time;
> +use kernel::types::ARef;
> +
> +use crate::file::File;
> +use crate::gem::TyrObject;
> +use crate::gpu;
> +use crate::gpu::GpuInfo;
> +use crate::regs;
> +
> +/// Convienence type alias for the DRM device type for this driver
> +pub(crate) type TyrDevice = drm::device::Device<TyrDriver>;
> +
> +#[pin_data(PinnedDrop)]
> +pub(crate) struct TyrDriver {
> +    device: ARef<TyrDevice>,
> +}
> +
> +#[pin_data]
> +pub(crate) struct TyrData {
> +    pub(crate) pdev: ARef<platform::Device>,
> +
> +    #[pin]
> +    clks: Mutex<Clocks>,
> +
> +    #[pin]
> +    regulators: Mutex<Regulators>,
> +
> +    // Some inforation on the GPU. This is mainly queried by userspace (mesa).

s/inforation/information

> +    pub(crate) gpu_info: GpuInfo,
> +}
> +
> +unsafe impl Send for TyrData {}
> +unsafe impl Sync for TyrData {}
> +
> +fn issue_soft_reset(iomem: &Devres<IoMem<0>>) -> Result<()> {
> +    let irq_enable_cmd = 1 | bit_u32(8);

To enhance readability, consider using a regmap similar to
panthor_regs.h. This would help avoid 'magic numbers' and make the
code's intent much clearer.

> +    regs::GPU_CMD.write(iomem, irq_enable_cmd)?;
> +
> +    let op = || regs::GPU_INT_RAWSTAT.read(iomem);
> +    let cond = |raw_stat: &u32| -> bool { (*raw_stat >> 8) & 1 == 1 };
> +    let res = io::poll::read_poll_timeout(
> +        op,
> +        cond,
> +        time::Delta::from_millis(100),
> +        Some(time::Delta::from_micros(20000)),
> +    );
> +
> +    if let Err(e) = res {
> +        pr_err!("GPU reset failed with errno {}\n", e.to_errno());
> +        pr_err!(
> +            "GPU_INT_RAWSTAT is {}\n",
> +            regs::GPU_INT_RAWSTAT.read(iomem)?
> +        );
> +    }
> +
> +    Ok(())
> +}
> +
> +kernel::of_device_table!(
> +    OF_TABLE,
> +    MODULE_OF_TABLE,
> +    <TyrDriver as platform::Driver>::IdInfo,
> +    [
> +        (of::DeviceId::new(c_str!("rockchip,rk3588-mali")), ()),
> +        (of::DeviceId::new(c_str!("arm,mali-valhall-csf")), ())
> +    ]
> +);
> +
> +impl platform::Driver for TyrDriver {
> +    type IdInfo = ();
> +    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
> +
> +    fn probe(
> +        pdev: &platform::Device<Core>,
> +        _info: Option<&Self::IdInfo>,
> +    ) -> Result<Pin<KBox<Self>>> {
> +        dev_dbg!(pdev.as_ref(), "Probed Tyr\n");
> +
> +        let core_clk = Clk::get(pdev.as_ref(), Some(c_str!("core")))?;
> +        let stacks_clk = Clk::get(pdev.as_ref(), Some(c_str!("stacks")))?;

Shouldn't it be OptionalClk::get? From the DT schema for "arm,mali-
valhall-csf", I see that "stacks" and "coregroups" are optional.

> +        let coregroup_clk = Clk::get(pdev.as_ref(), Some(c_str!("coregroup")))?;

Same.

Best Regards,
- Maíra