Re: [PATCH v2 2/3] drm/panthor: Add initial support for CIX Sky1

[Cunyuan Liu <cunyuan.liu@cixtech.com>]

Hi Liviu,

Thanks for your review.

On 2026-11-10 10:03:46 +0000, Liviu Dudau wrote:
> > The GPU on CIX Sky1 needs an external reset before the driver first
> > accesses the hardware. Without this reset, the GPU hardware cannot be
> > accessed correctly. Acquire an optional exclusive reset for all platforms
> > and apply it after enabling the clocks during initial probe. Platforms
> > without a reset keep working through the reset framework NULL handling.
> 
> Adding support for an external reset should be separate from the enablement
> of Sky1 GPU.

I will move the external reset support into a separate patch.

> >
> > Configure SYSC_ALLOC before L2 power-on. Without these settings, GPU
> > rendering results are displayed incorrectly.
> 
> Adding support for SYSC_ALLOC should be a separate patch. It can be used
> by other SoCs as well, not just Sky1.

I will also move the SYSC_ALLOC support into a separate patch.

> >
> > Keep the clock settings provided by the PM firmware. This first version
> > does not read OPPs or enable devfreq. It does not register a GPU cooling
> > device.
> 
> This looks like a stop gap measure. What is your plan here? When do you
> think you might be able to provide OPP and devfreq support and what is
> the hold up?

The PM firmware already provides GPU performance states through SCMI.
However, some states describe both a clock frequency and a shader-core
configuration.

For example, the vendor driver interprets levels 72 and 216, exposed
as nominal 72 MHz and 216 MHz OPPs, as 350 MHz with two and six shader
cores. They are not actual clock rates of 72 MHz and 216 MHz.

Importing the firmware OPP table alone is therefore not enough.
Panthor also needs to distinguish these performance values from the
actual clock rates and safely change the shader-core configuration
during performance-state transitions.

I plan to implement and validate this in a separate patch series.
I do not yet have a firm submission date.

> > --- a/drivers/gpu/drm/panthor/panthor_devfreq.c
> > +++ b/drivers/gpu/drm/panthor/panthor_devfreq.c
> > @@ -154,6 +154,12 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
> >
> >       ptdev->devfreq = pdevfreq;
> >
> > +     /* Keep the firmware clock settings until Sky1 devfreq is supported. */
> > +     if (ptdev->soc_data && ptdev->soc_data->skip_devfreq) {
> > +             ptdev->fast_rate = clk_get_rate(ptdev->clks.core);
> > +             return 0;
> > +     }
> 
> This should not be in the driver. If you want to have a fixed frequency just use a fixed
> clock provider, or make the clock provider query the firmware for the value.

Sky1 already uses the SCMI clock provider to query the actual clock
rate and control clock enable and disable through the PM firmware.

I tested removing the skip_devfreq early return while keeping the
SCMI clocks. The driver correctly read the core clock as 900 MHz,
but probe failed because the normal initialization path requires an
OPP table. Changing to a fixed-clock provider alone would still leave
this issue.

I have now tried the approach used by Panfrost: if adding the OPP
table returns -ENODEV, continue without devfreq. Other errors are
still returned:

  ret = devm_pm_opp_of_add_table(dev);
  if (ret == -ENODEV) {
    /* Keep the clock provider's rate when no OPP table is present. */
    ptdev->fast_rate = clk_get_rate(ptdev->clks.core);
    return 0;
  }
  if (ret)
    return ret;

The current frequency is read from the clock provider, and the
Sky1-specific skip_devfreq flag is removed.

This passed probe, rendering, runtime suspend/resume and module
reload tests on Orion O6. Would this generic no-OPP approach be
acceptable for the initial support?

> >
> > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
> > index dc8d0191aef4..4d7b1230cd7b 100644
> > --- a/drivers/gpu/drm/panthor/panthor_drv.c
> > +++ b/drivers/gpu/drm/panthor/panthor_drv.c
> > @@ -1872,12 +1872,26 @@ static struct attribute *panthor_attrs[] = {
> >
> >  ATTRIBUTE_GROUPS(panthor);
> >
> > +static const struct panthor_sysc_config sysc_config_cix_sky1 = {
> > +     /* Sky1 needs these SYSC_ALLOC policies for correct display of GPU output. */
> > +     .alloc = {
> > +             0x00230000, 0x00000023, 0x00000000, 0x00000000,
> > +             0x00523222, 0x00523200, 0x00000022, 0x00000032,
> > +     },
> > +};
> 
> These should not be in the driver but in the device tree bindings. If you extract
> the system cache code into a separate patch we can have a proper discussion there.

I will move the eight SYSC_ALLOC values into a DT property, such as
arm,sysc-alloc, and submit the system-cache support as a separate patch.

> >
> > +static void panthor_gpu_sysc_config_set(struct panthor_device *ptdev)
> > +{
> > +     const struct panthor_sysc_config *config;
> > +     unsigned int i;
> > +
> > +     if (!ptdev->soc_data || !ptdev->soc_data->sysc_config)
> > +             return;
> > +
> > +     config = ptdev->soc_data->sysc_config;
> > +     for (i = 0; i < ARRAY_SIZE(config->alloc); i++)
> > +             gpu_write(ptdev->gpu->iomem, GPU_SYSC_ALLOC(i), config->alloc[i]);
> 
> This will crash for arch10 GPUs. I know that you're relying here on soc_data not having
> the fields filled in, but if we move the sysc_config data to being provided by DT, we
> need to check here.

I will add an architecture check to reject the configuration on
arch10 GPUs before accessing SYSC_ALLOC. I will also guard the
register-write function.

I will wait for your feedback on the no-OPP approach before sending v3.

Best regards,
Cunyuan