Re: [PATCH] Introduce Tyr

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

Hi Daniel,

On 30/06/25 10:53, Daniel Almeida wrote:
> Hi Maíra, thanks for chiming in :)
> 
>>
>> 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.
> 
> 
> Are you referring to "struct regmap" itself? Because last I checked, this
> abstraction is not available upstream. There was a person working on it, but I
> guess it hasn't seen any traction for a few months. I also don't see it being
> used in panthor_regs.h?

Sorry, I think I didn't express myself clearly. When I say regmap, I
mean using macros to express the register addresses and its fields. From
example, in Panthor, "1 | bit_u32(8)" is expressed as
GPU_IRQ_RESET_COMPLETED, which can make things more readable.

Best Regards,
- Maíra

> 
>>
>>> +    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
>>
>>
> 
> Ah yes, you’re right. I will fix that in v2.
> 
> — Daniel
>