>> @@ -182,37 +105,67 @@ struct GpuModels {
>> prod_major: 7,
>> }];
>>
>> -#[allow(dead_code)]
>> -pub(crate) struct GpuId {
>> - pub(crate) arch_major: u32,
>> - pub(crate) arch_minor: u32,
>> - pub(crate) arch_rev: u32,
>> - pub(crate) prod_major: u32,
>> - pub(crate) ver_major: u32,
>> - pub(crate) ver_minor: u32,
>> - pub(crate) ver_status: u32,
>> -}
>> -
>> -impl From<u32> for GpuId {
>> - fn from(value: u32) -> Self {
>> - GpuId {
>> - arch_major: (value & genmask_u32(28..=31)) >> 28,
>> - arch_minor: (value & genmask_u32(24..=27)) >> 24,
>> - arch_rev: (value & genmask_u32(20..=23)) >> 20,
>> - prod_major: (value & genmask_u32(16..=19)) >> 16,
>> - ver_major: (value & genmask_u32(12..=15)) >> 12,
>> - ver_minor: (value & genmask_u32(4..=11)) >> 4,
>> - ver_status: value & genmask_u32(0..=3),
>> - }
>> - }
>> +pub(crate) fn gpu_info_log(dev: &Device<Bound>, iomem: &Devres<IoMem>) ->
>> Result {
>> + let io = (*iomem).access(dev)?;
>> + let gpu_id = io.read(GPU_ID);
>> +
>> + let model_name = if let Some(model) = GPU_MODELS.iter().find(|&f| {
>> + f.arch_major == gpu_id.arch_major().get() && f.prod_major ==
>> gpu_id.prod_major().get()
>> + }) {
>> + model.name
>> + } else {
>> + "unknown"
>> + };
>> +
>> + // Create canonical product ID with only arch/product fields, excluding
>> version
>> + // fields. This ensures the same product at different revisions has the
>> same ID.
>> + let id = GPU_ID::zeroed()
>> + .with_arch_major(gpu_id.arch_major())
>> + .with_arch_minor(gpu_id.arch_minor())
>> + .with_arch_rev(gpu_id.arch_rev())
>> + .with_prod_major(gpu_id.prod_major())
>> + .into_raw();
>> +
>> + dev_info!(
>> + dev,
>> + "mali-{} id 0x{:x} major 0x{:x} minor 0x{:x} status 0x{:x}",
>> + model_name,
>> + id,
>
> This was previously right-shifted by 16. Now, I'm questioning this
> decision to filter out the version fields. I think it'd be better to
> print the raw ID directly. We're already extracting and printing the
> arch major.minor and version status, but if there's anything else
> we want to clearly extract from this raw ID, we could add more.
>
> TLDR; that's probably one place where I think it's not such a bad idea
> to diverge from Panthor and print an unmodified GPU_ID. Maybe s/id/raw
> GPU ID/ to make the distincting clear. None of this should happen in
> this commit though. Either we do that in a preliminary commit that
> drops the `>> 16`, or we keep the `>> 16` here, and change that in a
> follow-up.
IIRC, Onur recently cleaned this up, hasn’t he?
>
>> + gpu_id.ver_major().get(),
>> + gpu_id.ver_minor().get(),
>> + gpu_id.ver_status().get()
>> + );
>
> [...]
>
>> +
>> + impl GPU_COMMAND {
>> + /// No operation. This is the default value.
>> + pub(crate) const NOP: u32 = 0;
>> + /// Reset the GPU.
>> + pub(crate) const RESET: u32 = 1;
>> + /// Flush caches.
>> + pub(crate) const FLUSH_CACHES: u32 = 4;
>> + /// Clear GPU faults.
>> + pub(crate) const CLEAR_FAULT: u32 = 7;
>> + }
>> +
>> + register! {
>> + /// GPU command register in reset mode.
>> + /// Set command to [`GPU_COMMAND::RESET`] to set reset_mode.
>> + pub(crate) GPU_COMMAND_RESET (u32) => GPU_COMMAND {
>> + 7:0 command;
>
> Alexandre, dunno how hard it would be to extend this alias syntax to
> provide auto-initialization/expected-value of certain fields, like:
>
> pub(crate) GPU_COMMAND_RESET (u32) => GPU_COMMAND {
> 7:0 command <- GPU_COMMAND::RESET;
> 11:8 reset_mode;
> }
>
+1 to the syntax above. This looks quite ergonomic IMHO.
> so that when you instantiate a GPU_COMMAND_RESET, all you have to set is
> reset_mode and the command gets set to GPU_COMMAND::RESET for you.
> (that's for the write path, for the read path, you'll need some sort of
> match to do the re-interpretation anyway).
>
> Just to be clear, I'm not asking for any of that in the current
> register!() patchset. It's more a suggestion for a potential future
> improvement.
>
>> + 11:8 reset_mode;
>> + }
>> + }
— Daniel
