* Re: Re: [PATCH v7 2/2] pwm: dwc: add of/platform support
From: Xuyang Dong @ 2026-06-12 9:30 UTC (permalink / raw)
To: sashiko-reviews
Cc: ukleinek, robh, krzk+dt, conor+dt, ben-linux, ben.dooks, p.zabel,
linux-pwm, devicetree, linux-kernel, ningyu, linmin, xuxiang,
wangguosheng, pinkesh.vaghela
In-Reply-To: <20260605083829.60F021F00893@smtp.kernel.org>
>
> Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
>
> New issues:
> - [High] Registering the PWM chip before completing runtime PM initialization exposes the hardware to consumers while clocks might be turned off.
> - [High] Manual hardware teardown in `.remove()` precedes `pwm_chip` deregistration, leading to potential bus faults.
> - [High] TOCTOU race in `remove()` when conditionally acquiring a runtime PM reference allows concurrent runtime suspend while accessing registers.
> - [High] System resume is incorrectly aborted if expected runtime PM idle operations return `-EAGAIN`.
> - [High] Unconditional assertion of the reset control on the probe error path and during module removal permanently kills active hardware and unbalances the reset API.
> - [High] `remove()` artificially drops runtime PM references that belong to active consumers, causing an underflow.
> - [Medium] Integer overflow in timer configuration allows invalid large duty cycle/period values to bypass validation.
> - [Medium] Clock references are leaked during module removal if the device is held in an active PM state but no PWM channels are enabled.
> - [Medium] The driver rounds UP duty cycle and period calculations instead of rounding DOWN, violating PWM subsystem rules.
> - [Medium] The driver evaluates the static device tree polarity instead of the dynamically requested runtime polarity, mapping the duty cycle incorrectly.
>
> Pre-existing issues:
> - [High] `dwc_pwm_get_state()` ignores the return value of `pm_runtime_get_sync()`, allowing unprotected hardware access.
> --
>
> commit 517966ff9e8e50806716056dd4bd3f2f88b832c0
> Author: Xuyang Dong <dongxuyang@eswincomputing.com>
>
> pwm: dwc: add of/platform support
>
> The dwc pwm controller can be used in non-PCI systems, so allow
> either platform or OF based probing.
>
> > diff --git a/drivers/pwm/pwm-dwc-core.c b/drivers/pwm/pwm-dwc-core.c
> > index 6dabec93a3c64..a6de05e273216 100644
> > --- a/drivers/pwm/pwm-dwc-core.c
> > +++ b/drivers/pwm/pwm-dwc-core.c
> [ ... ]
> > @@ -44,21 +45,52 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
> [ ... ]
> > + if (dwc->features & DWC_TIM_CTRL_0N100PWM_EN) {
> > + /*
> > + * Calculate width of low and high period in terms of input
> > + * clock periods and check are the result within HW limits
> > + * between 0 and 2^32 periods.
> > + */
> > + tmp = state->duty_cycle * dwc->clk_rate;
>
> [Severity: Medium]
> Could this multiplication overflow the 64-bit boundary before the hardware
> limit check if a consumer requests an extremely large period? Both duty_cycle
> and period are u64 nanoseconds, while clk_rate is an unsigned long. A
> wrapped-around small value would evade the hardware limit check entirely.
> Should this use mul_u64_u64_div_u64() to scale the values safely?
>
Hi Sashiko,
Use mul_u64_u64_div_u64() to scale the values safely.
Fix this issue in the next version.
> > + tmp = DIV_ROUND_UP_ULL(tmp, NSEC_PER_SEC);
>
> [Severity: Medium]
> Does this violate the PWM subsystem rules by rounding up instead of down?
> The PWM core framework strictly requires that the .apply callback rounds down
> the requested period and duty cycle to the nearest achievable hardware value.
>
Fix this issue in the next version with mul_u64_u64_div_u64().
> > + if (tmp >= (1ULL << 32))
> > + return -ERANGE;
> > +
> > + if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> > + high = tmp;
> > + else
> > + low = tmp;
>
> [Severity: Medium]
> Should this evaluate the dynamically requested state->polarity instead of the
> static device tree polarity in pwm->args.polarity? Since dwc_pwm_apply()
> enforces PWM_POLARITY_INVERSED, if the device tree was configured with
> PWM_POLARITY_NORMAL, this will program the hardware with the inverted duty
> cycle relative to what the consumer actually requested.
>
Here, we expect to support both polarity scenarios, PWM_POLARITY_INVERSED
and PWM_POLARITY_NORMAL, passed in via the device tree.
Therefore, we prefer to keep it. Do you have any thoughts or suggestions?
> [ ... ]
> > @@ -121,11 +157,17 @@ static int dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > struct pwm_state *state)
> > {
> > struct dwc_pwm *dwc = to_dwc_pwm(chip);
> > + unsigned long clk_rate;
> > u64 duty, period;
> > u32 ctrl, ld, ld2;
> >
> > pm_runtime_get_sync(pwmchip_parent(chip));
>
> [Severity: High]
> This isn't a bug introduced by this patch, but the return value of
> pm_runtime_get_sync() is ignored here. If the function fails (for example,
> returning -EACCES when PM runtime is disabled during driver removal, or before
> it is enabled in probe), the hardware might not be powered on and clocks could
> remain gated. The function will then proceed to read the DWC_TIM_CTRL and
> DWC_TIM_LD_CNT registers, which can cause a synchronous external abort when
> accessing unclocked hardware. Can we add a check for the return value here?
>
Add a check for pm_runtime_get_sync() in the next version.
> > diff --git a/drivers/pwm/pwm-dwc-of.c b/drivers/pwm/pwm-dwc-of.c
> > new file mode 100644
> > index 0000000000000..d8b1606e51af4
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-dwc-of.c
> [ ... ]
> > +static int dwc_pwm_plat_probe(struct platform_device *pdev)
> > +{
> [ ... ]
> > + ret = devm_pwmchip_add(dev, chip);
> > + if (ret) {
> > + dev_err(dev, "failed to add pwm chip\n");
> > + goto reset_assert;
> > + }
> > +
> > + data->chips[0] = chip;
> > + dev_set_drvdata(dev, data);
> > +
> > + /*
> > + * If any PWM channel is enabled, mark device active and hold runtime PM
> > + * references for each enabled channel. Otherwise, gate the clocks.
> > + */
> > + if (pwm_en) {
> > + pm_runtime_set_active(dev);
> > + for (i = 0; i < chip->npwm; i++) {
> > + if (ctrl[i])
> > + pm_runtime_get_noresume(dev);
> > + }
> > + } else {
> > + clk_disable_unprepare(dwc->clk);
> > + clk_disable_unprepare(dwc->bus_clk);
> > + }
> > +
> > + pm_runtime_enable(dev);
>
> [Severity: High]
> Does registering the PWM chip before enabling PM expose the hardware to
> consumers prematurely? devm_pwmchip_add() is called before pm_runtime_enable().
> If a consumer accesses the PWM during this window, the core driver's
> pm_runtime_get_sync() call would fail with -EACCES, but still proceed to access
> registers. At the same time, probe might gate the clocks if pwm_en is false,
> leading to a bus fault.
>
Move devm_pwmchip_add() after pm_runtime_enable().
> > +
> > + return 0;
> > +
> > +reset_assert:
> > + reset_control_assert(dwc->rst);
>
> [Severity: High]
> Does this unconditionally assert the reset control on the error path? The
> commit message mentions resetting only when no PWM channel is enabled. If
> pwm_en was true, this forcibly kills actively running hardware. If pwm_en was
> false, probe used reset_control_reset() (which is a pulse), so asserting it
> here leaves the line permanently asserted in an unbalanced state.
>
Remove reset_assert label and reset_control_assert().
> > +disable_clk:
> > + clk_disable_unprepare(dwc->clk);
> > +disable_busclk:
> > + clk_disable_unprepare(dwc->bus_clk);
> > +
> > + return ret;
> > +}
> > +
> > +static void dwc_pwm_plat_remove(struct platform_device *pdev)
> > +{
> [ ... ]
> > + /*
> > + * Resume the device if it is runtime suspended to allow
> > + * safe register access.
> > + */
> > + pm_flags = pm_runtime_status_suspended(&pdev->dev);
> > + if (pm_flags)
> > + pm_runtime_get_sync(&pdev->dev);
>
> [Severity: High]
> Is there a race condition when conditionally acquiring this PM reference? If
> pm_runtime_status_suspended() returns false, the code proceeds to read
> registers without taking a reference. An auto-suspend timer or a concurrent
> consumer could suspend the device immediately after this check, turning off the
> clocks and causing the subsequent DWC_TIM_CTRL reads to trigger a bus fault.
> Should pm_runtime_get_sync() be called unconditionally?
>
Remove pm_runtime_status_suspended() and the state check, and directly use
pm_runtime_get_sync() instead.
> > +
> > + for (idx = 0; idx < chip->npwm; idx++) {
> > + if (dwc_pwm_readl(dwc, DWC_TIM_CTRL(idx)) & DWC_TIM_CTRL_EN) {
> > + pwm_en = true;
> > + pm_runtime_put_noidle(&pdev->dev);
> > + }
> > + }
>
> [Severity: High]
> Does dropping the PM reference here underflow the usage count for active
> consumers? If a consumer has taken over the PWM channel, it logically owns that
> reference via dwc_pwm_apply(). Because devres unregisters the PWM chip after
> remove() finishes, the PWM core will eventually call
> dwc_pwm_apply(..., enabled=false), which calls pm_runtime_put_sync(). Dropping
> it here artificially might lead to a usage count underflow.
>
Remove pwm_en, but keep pm_runtime_put_noidle().
When the PWM is enabled, pm_runtime_get_sync() is called to increase the
usage count. When the PWM is disabled, pm_runtime_put_sync() is called to
decrease the usage count. However, if the PWM is unloaded directly after
being enabled, the PWM remains in the enabled state and the usage count
will not be decreased. Therefore, when unloading the device, if the PWM
enable bit is still 1, pm_runtime_put_noidle() is used to decrease the
usage count.
> > +
> > + /*
> > + * Re-suspend the device if it was runtime suspended prior to
> > + * the register access.
> > + */
> > + if (pm_flags)
> > + pm_runtime_put_sync(&pdev->dev);
> > +
> > + if (pwm_en) {
> > + clk_disable_unprepare(dwc->clk);
> > + clk_disable_unprepare(dwc->bus_clk);
> > + }
>
> [Severity: Medium]
> Will this leak clock references if the device is active but no PWM channels are
> enabled? If pwm_en is false but the device's PM runtime state is RPM_ACTIVE
> (due to an open sysfs handle or a pending auto-suspend), skipping this block
> leaves the clocks on. The subsequent pm_runtime_disable() does not turn off
> clocks, permanently leaking them.
>
Use pm_runtime_status_suspended() to check the status. If the device is
not suspended (i.e., active), call clk_disable_unprepare(). If it is
suspended, skip this block.
> > +
> > + pm_runtime_disable(&pdev->dev);
> > + reset_control_assert(dwc->rst);
> > +}
>
> [Severity: High]
> Does manually tearing down the hardware here lead to bus faults during devres
> cleanup? Since the driver registers the chip using devm_pwmchip_add(), devres
> will unregister the PWM chip after dwc_pwm_plat_remove() returns. During that
> window, the device is fully accessible to consumers, but clocks are off and
> reset is asserted. Additionally, unconditionally asserting the reset here
> ignores the earlier logic to keep active hardware running if pwm_en was true.
>
Would it be more appropriate to use pwmchip_add() along with an explicit
pwmchip_remove() before turning off the clocks, instead of relying on
devm_pwmchip_add()? I'm concerned that the devres cleanup might lead to
bus faults. What do you think?
Add a check of dwc->rst before calling reset_control_assert().
> [ ... ]
> > +static int dwc_pwm_resume(struct device *dev)
> > +{
> [ ... ]
> > + for (idx = 0; idx < chip->npwm; idx++) {
> > + dwc_pwm_writel(dwc, dwc->ctx[idx].cnt, DWC_TIM_LD_CNT(idx));
> > + dwc_pwm_writel(dwc, dwc->ctx[idx].cnt2, DWC_TIM_LD_CNT2(idx));
> > + dwc_pwm_writel(dwc, dwc->ctx[idx].ctrl, DWC_TIM_CTRL(idx));
> > + }
> > +
> > + if (pm_flags) {
> > + /* Balance the refcount taken by pm_runtime_get_sync
> > + * if it was used
> > + */
> > + ret = pm_runtime_put_sync(dev);
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
>
> [Severity: High]
> Should a legitimate -EAGAIN from pm_runtime_put_sync() abort the system resume?
> According to PM subsystem rules, pm_runtime_put_sync() can return -EAGAIN if
> another thread holds a usage reference. Returning this expected error code from
> a system resume callback will unexpectedly abort the entire system resume
> process for the device.
>
Drop the check of pm_runtime_put_sync().
Best regards,
Xuyang Dong
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260605082242.1541-1-dongxuyang@eswincomputing.com?part=2
^ permalink raw reply
* Re: [PATCH v3 1/3] dt-bindings: media: i2c: Add os02g10 sensor
From: Vladimir Zapolskiy @ 2026-06-12 9:35 UTC (permalink / raw)
To: Elgin Perumbilly, sakari.ailus, laurent.pinchart
Cc: Tarang Raval, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Hans Verkuil, Hans de Goede,
Mehdi Djait, Sylvain Petinot, Benjamin Mugnier, Svyatoslav Ryhel,
Hardevsinh Palaniya, Himanshu Bhavani, linux-media, devicetree,
linux-kernel
In-Reply-To: <20260424092554.26130-2-elgin.perumbilly@siliconsignals.io>
On 4/24/26 12:25, Elgin Perumbilly wrote:
> Add bindings for Omnivision OS02G10 sensor.
>
> Signed-off-by: Elgin Perumbilly <elgin.perumbilly@siliconsignals.io>
> ---
> .../bindings/media/i2c/ovti,os02g10.yaml | 91 +++++++++++++++++++
> MAINTAINERS | 7 ++
> 2 files changed, 98 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,os02g10.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,os02g10.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,os02g10.yaml
> new file mode 100644
> index 000000000000..252059459390
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,os02g10.yaml
> @@ -0,0 +1,91 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ovti,os02g10.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: OmniVision OS02G10 Image Sensor
> +
> +maintainers:
> + - Tarang Raval <tarang.raval@siliconsignals.io>
> +
> +description:
> + The OmniVision OS02G10 is a 2MP (1920x1080) color CMOS image sensor controlled
> + through an I2C-compatible SCCB bus. It outputs RAW10 format data and supports
> + a 2-lane MIPI interface.
> +
> +properties:
> + compatible:
> + const: ovti,os02g10
> +
> + reg:
> + maxItems: 1
> +
> + clocks:
> + items:
> + - description: XCLK clock
> +
> + avdd-supply:
> + description: Analog Domain Power Supply (2.8v)
> +
> + dovdd-supply:
> + description: I/O Domain Power Supply (1.8v)
> +
> + dvdd-supply:
> + description: Digital core Power Supply (1.5v)
> +
> + reset-gpios:
> + maxItems: 1
> + description: Reset Pin GPIO Control (active low)
> +
> + port:
> + description: MIPI CSI-2 transmitter port
> + $ref: /schemas/graph.yaml#/$defs/port-base
> + additionalProperties: false
> +
> + properties:
> + endpoint:
> + $ref: /schemas/media/video-interfaces.yaml#
> + unevaluatedProperties: false
> +
> + required:
> + - link-frequencies
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - avdd-supply
> + - dovdd-supply
> + - dvdd-supply
> + - port
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> +
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + camera-sensor@3c {
> + compatible = "ovti,os02g10";
> + reg = <0x3c>;
> + clocks = <&os02g10_clk>;
> + reset-gpios = <&gpio1 7 GPIO_ACTIVE_LOW>;
> +
> + avdd-supply = <&os02g10_avdd_2v8>;
> + dvdd-supply = <&os02g10_dvdd_1v2>;
> + dovdd-supply = <&os2gb10_dovdd_1v8>;
> +
> + port {
> + cam_out: endpoint {
> + remote-endpoint = <&mipi_in_cam>;
> + data-lanes = <1 2>;
> + link-frequencies = /bits/ 64 <720000000>;
> + };
> + };
> + };
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 900fc00b73e6..8a0a55073c30 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19443,6 +19443,13 @@ T: git git://linuxtv.org/media_tree.git
> F: Documentation/devicetree/bindings/media/i2c/ovti,og0ve1b.yaml
> F: drivers/media/i2c/og0ve1b.c
>
> +OMNIVISION OS02G10 SENSOR DRIVER
> +M: Tarang Raval <tarang.raval@siliconsignals.io>
> +M: Elgin Perumbilly <elgin.perumbilly@siliconsignals.io>
> +L: linux-media@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/media/i2c/ovti,os02g10.yaml
> +
> OMNIVISION OS05B10 SENSOR DRIVER
> M: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
> M: Elgin Perumbilly <elgin.perumbilly@siliconsignals.io>
> --
> 2.34.1
>
Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
--
Best wishes,
Vladimir
^ permalink raw reply
* Re: [PATCH v4 02/16] media: iris: Filter UBWC raw formats based on hardware capabilities
From: sashiko-bot @ 2026-06-12 9:36 UTC (permalink / raw)
To: Dmitry Baryshkov; +Cc: conor+dt, robh, devicetree
In-Reply-To: <20260612-iris-ar50lt-v4-2-0abfb74d5b3c@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The logic to dynamically filter out UBWC raw formats in the video decoder is incomplete and fails to omit the 8-bit UBWC format (QC08C).
- [Medium] Incorrect use of the encoder UBWC version field to validate decoder capabilities.
--
commit a8db7d07b5e1a7229c20919b2735cd3483c3c3ab
Author: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
media: iris: Filter UBWC raw formats based on hardware capabilities
The raw formats supported by Iris were previously advertised unconditionally,
assuming UBWC support on all platforms. Use the UBWC configuration provided
by the platform to dynamically filter raw formats at runtime. If UBWC is
not supported, UBWC-based formats are omitted from the advertised capability
list.
> diff --git a/drivers/media/platform/qcom/iris/iris_vdec.c b/drivers/media/platform/qcom/iris/iris_vdec.c
> index 9e228b70420e4..7da43f312ba98 100644
> --- a/drivers/media/platform/qcom/iris/iris_vdec.c
> +++ b/drivers/media/platform/qcom/iris/iris_vdec.c
[ ... ]
> @@ -82,6 +84,9 @@ static bool check_format(struct iris_inst *inst, u32 pixfmt, u32 type)
> case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> fmt = iris_vdec_formats_cap;
> size = ARRAY_SIZE(iris_vdec_formats_cap);
> + /* Last format is UBWC; drop it if UBWC is unsupported */
> + if (!ubwc->ubwc_enc_version)
> + size--;
[Severity: High]
Does this logic successfully drop all UBWC formats?
Looking at the definition of iris_vdec_formats_cap, it appears there are
multiple UBWC formats interspersed:
static const u32 iris_vdec_formats_cap[] = {
[IRIS_FMT_NV12] = V4L2_PIX_FMT_NV12,
[IRIS_FMT_QC08C] = V4L2_PIX_FMT_QC08C,
[IRIS_FMT_TP10] = V4L2_PIX_FMT_P010,
[IRIS_FMT_QC10C] = V4L2_PIX_FMT_QC10C,
};
By just decrementing the size, it seems we only omit the last format
(QC10C), but QC08C would remain within the array bounds. Could this
cause QC08C to be incorrectly accepted by check_format() on platforms
without UBWC support?
[Severity: Medium]
Should this check use the decoder version instead?
Since this is the video decoder implementation in iris_vdec.c, checking
ubwc->ubwc_enc_version might lead to incorrect filtering on future hardware
platforms where UBWC support differs between the encoder and decoder. Would
ubwc->ubwc_dec_version be more appropriate here?
> break;
> default:
> return false;
> @@ -121,6 +127,9 @@ static u32 find_format_by_index(struct iris_inst *inst, u32 index, u32 type)
> case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> fmt = iris_vdec_formats_cap;
> size = ARRAY_SIZE(iris_vdec_formats_cap);
> + /* Last format is UBWC; drop it if UBWC is unsupported */
> + if (!ubwc->ubwc_enc_version)
> + size--;
[Severity: High]
Does this also result in find_format_by_index() returning a UBWC format?
Because QC08C remains within the decremented array bounds, would querying
index 1 still return V4L2_PIX_FMT_QC08C to userspace even when UBWC is
unsupported?
[Severity: Medium]
Similar to the issue above, should this use ubwc->ubwc_dec_version instead
of ubwc->ubwc_enc_version to properly validate decoder capabilities?
> break;
> default:
> return 0;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-iris-ar50lt-v4-0-0abfb74d5b3c@oss.qualcomm.com?part=2
^ permalink raw reply
* Re: [PATCH v4 06/16] media: iris: Add platform data field for watchdog interrupt mask
From: sashiko-bot @ 2026-06-12 9:41 UTC (permalink / raw)
To: Dmitry Baryshkov; +Cc: robh, conor+dt, devicetree
In-Reply-To: <20260612-iris-ar50lt-v4-6-0abfb74d5b3c@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly introduced `wd_intr_mask` field is not initialized for the `x1p42100_data` platform, causing it to default to 0 and completely breaking watchdog interrupt handling for this platform.
--
commit 0965d657e0a8904cc63b8cbd790144e332d59492
Author: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
media: iris: Add platform data field for watchdog interrupt mask
This commit introduces the wd_intr_mask platform data field to capture the
watchdog interrupt bitmask per platform. It also replaces hardcoded
watchdog bitmasks with this new field during interrupt handling.
> diff --git a/drivers/media/platform/qcom/iris/iris_platform_vpu3x.c b/drivers/media/platform/qcom/iris/iris_platform_vpu3x.c
> index 2c63adbc55791..aeffd96a4e1cb 100644
> --- a/drivers/media/platform/qcom/iris/iris_platform_vpu3x.c
> +++ b/drivers/media/platform/qcom/iris/iris_platform_vpu3x.c
[ ... ]
> @@ -208,6 +213,7 @@ const struct iris_platform_data sm8750_data = {
> .tz_cp_config_data = tz_cp_config_vpu3,
> .tz_cp_config_data_size = ARRAY_SIZE(tz_cp_config_vpu3),
> .num_vpp_pipe = 4,
> + .wd_intr_mask = WRAPPER_INTR_STATUS_A2HWD_BMSK,
> .max_session_count = 16,
> .max_core_mbpf = NUM_MBS_8K * 2,
> .max_core_mbps = ((7680 * 4320) / 256) * 60,
[Severity: High]
Does this patch miss updating x1p42100_data?
It appears x1p42100_data is defined further down in this file but does not
have its wd_intr_mask initialized in this commit.
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_common.c b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> index 375bcd9234766..41498f94480e8 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu_common.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_common.c
> @@ -109,11 +109,11 @@ void iris_vpu_raise_interrupt(struct iris_core *core)
>
> void iris_vpu_clear_interrupt(struct iris_core *core)
> {
> + u32 wd_intr_mask = core->iris_platform_data->wd_intr_mask;
> u32 intr_status, mask;
>
> intr_status = readl(core->reg_base + WRAPPER_INTR_STATUS);
> - mask = (WRAPPER_INTR_STATUS_A2H_BMSK |
> - WRAPPER_INTR_STATUS_A2HWD_BMSK |
> + mask = (WRAPPER_INTR_STATUS_A2H_BMSK | wd_intr_mask |
> CTRL_INIT_IDLE_MSG_BMSK);
>
> if (intr_status & mask)
[Severity: High]
Since x1p42100_data is a static global variable, its uninitialized
wd_intr_mask will default to 0.
Will this cause the watchdog interrupt bit to be omitted from this mask,
preventing watchdog interrupts from being captured into core->intr_status
for the x1p42100 platform?
> @@ -124,7 +124,9 @@ void iris_vpu_clear_interrupt(struct iris_core *core)
>
> int iris_vpu_watchdog(struct iris_core *core, u32 intr_status)
> {
> - if (intr_status & WRAPPER_INTR_STATUS_A2HWD_BMSK) {
> + u32 wd_intr_mask = core->iris_platform_data->wd_intr_mask;
> +
> + if (intr_status & wd_intr_mask) {
> dev_err(core->dev, "received watchdog interrupt\n");
> return -ETIME;
> }
[Severity: High]
Similarly, will this cause intr_status & wd_intr_mask to always evaluate
to 0 for x1p42100, silently dropping all watchdog timeouts and breaking
firmware hang recovery?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-iris-ar50lt-v4-0-0abfb74d5b3c@oss.qualcomm.com?part=6
^ permalink raw reply
* Re: [PATCH net-next v2 3/3] arm64: dts: renesas: r8a779g0: Add gPTP node
From: Geert Uytterhoeven @ 2026-06-12 9:44 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm, Richard Cochran, Andrew Lunn,
DavidS. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-renesas-soc, devicetree, linux-kernel, netdev
In-Reply-To: <20260612092851.2141782-4-niklas.soderlund+renesas@ragnatech.se>
Hi Niklas,
On Fri, 12 Jun 2026 at 11:29, Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> The gPTP module is shared between the RAVB and RTSN Ethernet devices on
> the SoC.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> * Changes since v1
> - Rename node ptp.
Thanks for the update!
> --- a/arch/arm64/boot/dts/renesas/r8a779g0.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a779g0.dtsi
> @@ -603,6 +603,15 @@ tsn0: ethernet@e6460000 {
> status = "disabled";
> };
>
> + gptp: ptp@e6449000 {
Please preserve sort order (by unit-address).
> + compatible = "renesas,r8a779g0-gptp", "renesas,rcar-gen4-gptp";
> + reg = <0 0xe6449000 0 0x500>;
> + clocks = <&cpg CPG_MOD 2723>;
> + power-domains = <&sysc R8A779G0_PD_ALWAYS_ON>;
> + resets = <&cpg 2723>;
> + status = "disabled";
> + };
> +
> i2c0: i2c@e6500000 {
> compatible = "renesas,i2c-r8a779g0",
> "renesas,rcar-gen4-i2c";
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v4 07/16] media: iris: Add platform flag for instantaneous bandwidth voting
From: sashiko-bot @ 2026-06-12 9:46 UTC (permalink / raw)
To: Dmitry Baryshkov; +Cc: devicetree, robh, conor+dt
In-Reply-To: <20260612-iris-ar50lt-v4-7-0abfb74d5b3c@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] The `x1p42100_data` platform configuration is missing the `icc_ib_multiplier` initialization, causing it to incorrectly vote for 0 instantaneous bandwidth.
Pre-existing issues:
- [High] The `wd_intr_mask` field is missing in `x1p42100_data`, silently disabling watchdog interrupt handling for this platform.
--
commit 341a438b98027e7efd3119d0595101f6ec929a49
Author: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
media: iris: Add platform flag for instantaneous bandwidth voting
AR50LT require explicit instantaneous bandwidth (IB) voting in addition
to average bandwidth (AB) when configuring interconnect QoS. This
requirement is due to QSB (Qualcomm System Bus) 128b to
QNS ( Qualcomm Network Switch) 256b conversion at video noc in AR50LT
which is not needed for other IRIS cores.
> diff --git a/drivers/media/platform/qcom/iris/iris_platform_vpu3x.c b/drivers/media/platform/qcom/iris/iris_platform_vpu3x.c
> index aeffd96a4e1cb..8706989d4da45 100644
> --- a/drivers/media/platform/qcom/iris/iris_platform_vpu3x.c
> +++ b/drivers/media/platform/qcom/iris/iris_platform_vpu3x.c
[ ... ]
> @@ -214,6 +217,7 @@ const struct iris_platform_data sm8750_data = {
> .tz_cp_config_data_size = ARRAY_SIZE(tz_cp_config_vpu3),
> .num_vpp_pipe = 4,
> .wd_intr_mask = WRAPPER_INTR_STATUS_A2HWD_BMSK,
> + .icc_ib_multiplier = 1,
> .max_session_count = 16,
> .max_core_mbpf = NUM_MBS_8K * 2,
> .max_core_mbps = ((7680 * 4320) / 256) * 60,
> };
[Severity: High]
Did this miss updating x1p42100_data, which is defined right after sm8750_data
in this file?
Since x1p42100_data is missing the .icc_ib_multiplier initialization, it will
default to 0.
[Severity: High]
This is a pre-existing issue, but x1p42100_data is also missing the
.wd_intr_mask initialization, causing it to default to 0.
Does this mean watchdog interrupts triggered by the VPU hardware on the
x1p42100 SoC will be silently ignored? Looking at iris_vpu_watchdog():
u32 wd_intr_mask = core->iris_platform_data->wd_intr_mask;
if (intr_status & wd_intr_mask) {
...
}
Since wd_intr_mask is 0, the check will always evaluate to 0, which means
the driver will fail to detect VPU hangs.
> diff --git a/drivers/media/platform/qcom/iris/iris_resources.c b/drivers/media/platform/qcom/iris/iris_resources.c
> index 773f6548370a2..caeaf199cef74 100644
> --- a/drivers/media/platform/qcom/iris/iris_resources.c
> +++ b/drivers/media/platform/qcom/iris/iris_resources.c
[ ... ]
> @@ -36,6 +37,7 @@ int iris_set_icc_bw(struct iris_core *core, unsigned long icc_bw)
> return ret;
>
> core->icc_tbl[i].avg_bw = bw_kbps;
> + core->icc_tbl[i].peak_bw = bw_kbps * icc_ib_multiplier;
[Severity: High]
Could this inadvertently vote for 0 peak bandwidth on the x1p42100 SoC?
Because the platform configuration for x1p42100_data is missing the
icc_ib_multiplier initialization added to the other platforms, it defaults
to 0.
This causes the peak_bw calculation here to evaluate to 0, which could
starve the hardware of instantaneous bandwidth.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-iris-ar50lt-v4-0-0abfb74d5b3c@oss.qualcomm.com?part=7
^ permalink raw reply
* [PATCH v3 0/2] cpufreq: spacemit: Add cpufreq support for K1 SoC
From: Shuwei Wu @ 2026-06-12 9:51 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Alexandre Ghiti, Yixun Lan, Yixun Lan
Cc: linux-pm, linux-kernel, linux-riscv, spacemit, devicetree,
Shuwei Wu
This series enables CPU frequency scaling for the SpacemiT K1 SoC.
K1 has two CPU cluster clocks but one shared CPU voltage rail. Add a
small K1-specific cpufreq driver so the two clocks and the shared OPP
transition can be handled together.
Tested on Banana Pi BPI-F3. The system boots with all eight CPUs online,
the K1 cpufreq driver registers one policy for CPUs 0-7, and both CPU
cluster clocks follow the selected cpufreq rate.
~ # cat /sys/devices/system/cpu/online
0-7
~ # ls /sys/devices/system/cpu/cpufreq
policy0
~ # cat /sys/devices/system/cpu/cpufreq/policy0/affected_cpus
0 1 2 3 4 5 6 7
~ # cat /sys/devices/system/cpu/cpufreq/policy0/scaling_available_frequencies
614400 819000 1000000 1228800 1600000
With the userspace governor:
----------------------------------------------------------------------------
Frequency | scaling_cur | cpu_c0_core | cpu_c1_core | Real | User
(kHz) | (kHz) | (Hz) | (Hz) | (s) | (s)
-------------+-------------+-------------+-------------+---------+----------
1,600,000 | 1,600,000 | 1600000000 | 1600000000 | 1.80s | 1.80s
1,228,800 | 1,228,800 | 1228800000 | 1228800000 | 2.33s | 2.33s
1,000,000 | 1,000,000 | 1000000000 | 1000000000 | 2.88s | 2.87s
819,000 | 819,000 | 819200000 | 819200000 | 3.53s | 3.52s
614,400 | 614,400 | 614400000 | 614400000 | 4.72s | 4.72s
----------------------------------------------------------------------------
Signed-off-by: Shuwei Wu <shuwei.wu@mailbox.org>
---
Changes in v3:
- Add a K1-specific cpufreq driver for the shared-rail, dual-clock topology
- Use one shared CPU OPP table and one cpufreq policy for all CPUs
- Link to v2: https://lore.kernel.org/r/20260410-shadow-deps-v2-0-4e16b8c0f60e@mailbox.org
Changes in v2:
- Move OPP tables to dedicated k1-opp.dtsi
- Enable OPP only on BPI-F3 with cpu-supply present
- Link to v1: https://lore.kernel.org/r/20260308-shadow-deps-v1-0-0ceb5c7c07eb@mailbox.org
---
Shuwei Wu (2):
cpufreq: spacemit: Add K1 cpufreq driver
riscv: dts: spacemit: Add cpu scaling for K1 SoC
arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts | 35 +++-
arch/riscv/boot/dts/spacemit/k1-opp.dtsi | 70 +++++++
arch/riscv/boot/dts/spacemit/k1.dtsi | 8 +
drivers/cpufreq/Kconfig | 4 +
drivers/cpufreq/Kconfig.riscv | 15 ++
drivers/cpufreq/Makefile | 3 +
drivers/cpufreq/cpufreq-dt-platdev.c | 2 +
drivers/cpufreq/spacemit-k1-cpufreq.c | 251 ++++++++++++++++++++++++
8 files changed, 387 insertions(+), 1 deletion(-)
---
base-commit: 5164e95565d3fd508ca8a95351323f5716dfb695
change-id: 20260307-shadow-deps-3582a78aa756
prerequisite-patch-id: 154bd4f720ce5065d58b988de8f273207b44572e
prerequisite-message-id: <20260206-spacemit-p1-v4-0-8f695d93811e@riscstar.com>
prerequisite-patch-id: 5da3e75b18291a5540d4f66d7a0600fb8975ef62
prerequisite-patch-id: bcf41917414ecef8cf743095d130f6004c32f6a5
prerequisite-patch-id: cfe3800f8c791ec4c63e070af9628e88e0fc31b9
prerequisite-message-id: <20260305-k1-clk-fix-v1-1-abca85d6e266@mailbox.org>
prerequisite-patch-id: 7c7fb9f87dba019ece4c97c45750349a7cd28f3a
Best regards,
--
Shuwei Wu <shuwei.wu@mailbox.org>
^ permalink raw reply
* [PATCH v3 1/2] cpufreq: spacemit: Add K1 cpufreq driver
From: Shuwei Wu @ 2026-06-12 9:51 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Alexandre Ghiti, Yixun Lan, Yixun Lan
Cc: linux-pm, linux-kernel, linux-riscv, spacemit, devicetree,
Shuwei Wu
In-Reply-To: <20260612-shadow-deps-v3-0-2f3ba88611ff@mailbox.org>
K1 has two CPU cluster clocks but one shared CPU voltage rail. Use one
cpufreq policy for all CPUs, let the OPP core handle the shared regulator
and the cluster0 clock, and update the cluster1 clock explicitly.
Block spacemit,k1 from cpufreq-dt probing when OPP v2 nodes are present,
as K1 needs the dedicated driver for safe transitions.
Signed-off-by: Shuwei Wu <shuwei.wu@mailbox.org>
---
Changes in v3:
- Add a K1-specific cpufreq driver for the shared-rail, dual-clock topology
- Add spacemit,k1 to the cpufreq-dt blocklist
---
drivers/cpufreq/Kconfig | 4 +
drivers/cpufreq/Kconfig.riscv | 15 ++
drivers/cpufreq/Makefile | 3 +
drivers/cpufreq/cpufreq-dt-platdev.c | 2 +
drivers/cpufreq/spacemit-k1-cpufreq.c | 251 ++++++++++++++++++++++++++++++++++
5 files changed, 275 insertions(+)
diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
index 78702a08364f..19bbd1bd2f36 100644
--- a/drivers/cpufreq/Kconfig
+++ b/drivers/cpufreq/Kconfig
@@ -259,6 +259,10 @@ endif
source "drivers/cpufreq/Kconfig.arm"
+if RISCV
+source "drivers/cpufreq/Kconfig.riscv"
+endif
+
if PPC32 || PPC64
source "drivers/cpufreq/Kconfig.powerpc"
endif
diff --git a/drivers/cpufreq/Kconfig.riscv b/drivers/cpufreq/Kconfig.riscv
new file mode 100644
index 000000000000..3d5b59fbcd4c
--- /dev/null
+++ b/drivers/cpufreq/Kconfig.riscv
@@ -0,0 +1,15 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# RISC-V CPU Frequency scaling drivers
+#
+
+config RISCV_SPACEMIT_K1_CPUFREQ
+ tristate "SpacemiT K1 CPUFreq driver"
+ depends on ARCH_SPACEMIT || COMPILE_TEST
+ depends on OF && COMMON_CLK
+ select PM_OPP
+ help
+ Enable CPU frequency scaling for SpacemiT K1 SoC.
+ K1 has two CPU cluster clocks and one shared CPU voltage rail.
+ The driver keeps all CPUs in one policy and updates both cluster
+ clocks with the shared OPP transition.
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 385c9fcc65c6..9680f420e824 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -89,6 +89,9 @@ obj-$(CONFIG_ARM_TEGRA194_CPUFREQ) += tegra194-cpufreq.o
obj-$(CONFIG_ARM_TI_CPUFREQ) += ti-cpufreq.o
obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
+##################################################################################
+# RISC-V SoC drivers
+obj-$(CONFIG_RISCV_SPACEMIT_K1_CPUFREQ) += spacemit-k1-cpufreq.o
##################################################################################
# PowerPC platform drivers
diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
index 25fd3b191b7e..69bbe739a627 100644
--- a/drivers/cpufreq/cpufreq-dt-platdev.c
+++ b/drivers/cpufreq/cpufreq-dt-platdev.c
@@ -185,6 +185,8 @@ static const struct of_device_id blocklist[] __initconst = {
{ .compatible = "qcom,sm8550", },
{ .compatible = "qcom,sm8650", },
+ { .compatible = "spacemit,k1", },
+
{ .compatible = "st,stih407", },
{ .compatible = "st,stih410", },
{ .compatible = "st,stih418", },
diff --git a/drivers/cpufreq/spacemit-k1-cpufreq.c b/drivers/cpufreq/spacemit-k1-cpufreq.c
new file mode 100644
index 000000000000..631897eb8022
--- /dev/null
+++ b/drivers/cpufreq/spacemit-k1-cpufreq.c
@@ -0,0 +1,251 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * CPU frequency scaling driver for SpacemiT K1 SoC.
+ *
+ * Copyright (c) 2026 Shuwei Wu <shuwei.wu@mailbox.org>
+ */
+
+#include <linux/clk.h>
+#include <linux/cpu.h>
+#include <linux/cpufreq.h>
+#include <linux/cpumask.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pm_opp.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+struct k1_cpufreq_priv {
+ struct device *cpu_dev;
+ struct clk *cluster0_clk;
+ struct clk *cluster1_clk;
+ struct cpufreq_frequency_table *freq_table;
+ cpumask_var_t cpus;
+ int opp_token;
+};
+
+static struct platform_device *k1_cpufreq_pdev;
+
+static int k1_cpufreq_set_target(struct cpufreq_policy *policy,
+ unsigned int index)
+{
+ struct k1_cpufreq_priv *priv = policy->driver_data;
+ unsigned long old_freq = policy->cur * 1000UL;
+ unsigned long new_freq = policy->freq_table[index].frequency * 1000UL;
+ int ret;
+
+ if (!old_freq)
+ old_freq = clk_get_rate(priv->cluster0_clk);
+
+ if (new_freq > old_freq) {
+ ret = dev_pm_opp_set_rate(priv->cpu_dev, new_freq);
+ if (ret)
+ return ret;
+
+ ret = clk_set_rate(priv->cluster1_clk, new_freq);
+ if (ret)
+ dev_pm_opp_set_rate(priv->cpu_dev, old_freq);
+
+ return ret;
+ }
+
+ ret = clk_set_rate(priv->cluster1_clk, new_freq);
+ if (ret)
+ return ret;
+
+ ret = dev_pm_opp_set_rate(priv->cpu_dev, new_freq);
+ if (ret)
+ clk_set_rate(priv->cluster1_clk, old_freq);
+
+ return ret;
+}
+
+static int k1_cpufreq_init_policy(struct cpufreq_policy *policy)
+{
+ struct k1_cpufreq_priv *priv = cpufreq_get_driver_data();
+ unsigned int transition_latency;
+
+ cpumask_copy(policy->cpus, priv->cpus);
+ policy->clk = priv->cluster0_clk;
+ policy->freq_table = priv->freq_table;
+ policy->driver_data = priv;
+ policy->dvfs_possible_from_any_cpu = true;
+
+ transition_latency = dev_pm_opp_get_max_transition_latency(priv->cpu_dev);
+ if (!transition_latency)
+ transition_latency = CPUFREQ_DEFAULT_TRANSITION_LATENCY_NS;
+ policy->cpuinfo.transition_latency = transition_latency;
+
+ return 0;
+}
+
+static struct cpufreq_driver k1_cpufreq_driver = {
+ .flags = CPUFREQ_NEED_INITIAL_FREQ_CHECK | CPUFREQ_IS_COOLING_DEV,
+ .verify = cpufreq_generic_frequency_table_verify,
+ .target_index = k1_cpufreq_set_target,
+ .get = cpufreq_generic_get,
+ .init = k1_cpufreq_init_policy,
+ .register_em = cpufreq_register_em_with_opp,
+ .name = "k1-cpufreq",
+};
+
+static int k1_cpufreq_probe(struct platform_device *pdev)
+{
+ struct k1_cpufreq_priv *priv;
+ struct device *cpu4_dev;
+ static const char * const reg_names[] = { "cpu", NULL };
+ int cpu, ret;
+
+ priv = kzalloc_obj(*priv);
+ if (!priv)
+ return -ENOMEM;
+
+ if (!zalloc_cpumask_var(&priv->cpus, GFP_KERNEL)) {
+ ret = -ENOMEM;
+ goto free_data;
+ }
+
+ priv->cpu_dev = get_cpu_device(0);
+ cpu4_dev = get_cpu_device(4);
+ if (!priv->cpu_dev || !cpu4_dev) {
+ ret = -EPROBE_DEFER;
+ goto free_cpumask;
+ }
+
+ for_each_present_cpu(cpu)
+ cpumask_set_cpu(cpu, priv->cpus);
+
+ priv->cluster0_clk = clk_get(priv->cpu_dev, NULL);
+ if (IS_ERR(priv->cluster0_clk)) {
+ ret = PTR_ERR(priv->cluster0_clk);
+ dev_err_probe(priv->cpu_dev, ret, "failed to get cluster0 clock\n");
+ goto free_cpumask;
+ }
+
+ priv->cluster1_clk = clk_get(cpu4_dev, NULL);
+ if (IS_ERR(priv->cluster1_clk)) {
+ ret = PTR_ERR(priv->cluster1_clk);
+ dev_err_probe(cpu4_dev, ret, "failed to get cluster1 clock\n");
+ goto put_clk_c0;
+ }
+
+ priv->opp_token = dev_pm_opp_set_regulators(priv->cpu_dev, reg_names);
+ if (priv->opp_token < 0) {
+ ret = priv->opp_token;
+ dev_err_probe(priv->cpu_dev, ret, "failed to set regulators\n");
+ goto put_clk_c1;
+ }
+
+ ret = dev_pm_opp_of_cpumask_add_table(priv->cpus);
+ if (ret) {
+ dev_err_probe(priv->cpu_dev, ret, "failed to add OPP table\n");
+ goto put_opp_regulators;
+ }
+
+ ret = dev_pm_opp_get_opp_count(priv->cpu_dev);
+ if (ret <= 0) {
+ dev_err(priv->cpu_dev, "OPP table can't be empty\n");
+ ret = -ENODEV;
+ goto remove_opp_table;
+ }
+
+ ret = dev_pm_opp_init_cpufreq_table(priv->cpu_dev, &priv->freq_table);
+ if (ret) {
+ dev_err(priv->cpu_dev, "failed to init cpufreq table: %d\n", ret);
+ goto remove_opp_table;
+ }
+
+ k1_cpufreq_driver.driver_data = priv;
+ ret = cpufreq_register_driver(&k1_cpufreq_driver);
+ if (ret)
+ goto free_freq_table;
+
+ platform_set_drvdata(pdev, priv);
+
+ return 0;
+
+free_freq_table:
+ k1_cpufreq_driver.driver_data = NULL;
+ dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &priv->freq_table);
+remove_opp_table:
+ dev_pm_opp_of_cpumask_remove_table(priv->cpus);
+put_opp_regulators:
+ dev_pm_opp_put_regulators(priv->opp_token);
+put_clk_c1:
+ clk_put(priv->cluster1_clk);
+put_clk_c0:
+ clk_put(priv->cluster0_clk);
+free_cpumask:
+ free_cpumask_var(priv->cpus);
+free_data:
+ kfree(priv);
+
+ return ret;
+}
+
+static void k1_cpufreq_remove(struct platform_device *pdev)
+{
+ struct k1_cpufreq_priv *priv = platform_get_drvdata(pdev);
+
+ if (!priv)
+ return;
+
+ cpufreq_unregister_driver(&k1_cpufreq_driver);
+ k1_cpufreq_driver.driver_data = NULL;
+ dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &priv->freq_table);
+ dev_pm_opp_of_cpumask_remove_table(priv->cpus);
+ dev_pm_opp_put_regulators(priv->opp_token);
+ clk_put(priv->cluster1_clk);
+ clk_put(priv->cluster0_clk);
+ free_cpumask_var(priv->cpus);
+ kfree(priv);
+}
+
+static struct platform_driver k1_cpufreq_platdrv = {
+ .probe = k1_cpufreq_probe,
+ .remove = k1_cpufreq_remove,
+ .driver = {
+ .name = "spacemit-k1-cpufreq",
+ },
+};
+
+static const struct of_device_id k1_cpufreq_match_list[] __initconst = {
+ { .compatible = "spacemit,k1" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, k1_cpufreq_match_list);
+
+/*
+ * K1 has no dedicated cpufreq controller device. Register a logical platform
+ * device so clock/regulator dependencies can defer probe.
+ */
+static int __init k1_cpufreq_init(void)
+{
+ int ret;
+
+ if (!of_machine_device_match(k1_cpufreq_match_list))
+ return -ENODEV;
+
+ ret = platform_driver_register(&k1_cpufreq_platdrv);
+ if (ret)
+ return ret;
+
+ k1_cpufreq_pdev = platform_device_register_simple("spacemit-k1-cpufreq", -1, NULL, 0);
+ ret = PTR_ERR_OR_ZERO(k1_cpufreq_pdev);
+ if (ret)
+ platform_driver_unregister(&k1_cpufreq_platdrv);
+
+ return ret;
+}
+module_init(k1_cpufreq_init);
+
+static void __exit k1_cpufreq_exit(void)
+{
+ platform_device_unregister(k1_cpufreq_pdev);
+ platform_driver_unregister(&k1_cpufreq_platdrv);
+}
+module_exit(k1_cpufreq_exit);
+
+MODULE_DESCRIPTION("SpacemiT K1 CPUFreq driver");
+MODULE_AUTHOR("Shuwei Wu <shuwei.wu@mailbox.org>");
+MODULE_LICENSE("GPL");
--
2.53.0
^ permalink raw reply related
* [PATCH v3 2/2] riscv: dts: spacemit: Add cpu scaling for K1 SoC
From: Shuwei Wu @ 2026-06-12 9:51 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Alexandre Ghiti, Yixun Lan, Yixun Lan
Cc: linux-pm, linux-kernel, linux-riscv, spacemit, devicetree,
Shuwei Wu
In-Reply-To: <20260612-shadow-deps-v3-0-2f3ba88611ff@mailbox.org>
Use one shared CPU OPP table for all CPUs and add CPU clock properties
for the two CPU clock clusters.
Enable CPU DVFS on Banana Pi BPI-F3 by including the OPP table and
wiring the CPU nodes to the CPU regulator supply.
Signed-off-by: Shuwei Wu <shuwei.wu@mailbox.org>
---
Changes in v3:
- Use one shared CPU OPP table for all CPUs
Changes in v2:
- Add k1-opp.dtsi with OPP tables for both CPU clusters
- Assign CPU supplies and include OPP table for Banana Pi BPI-F3
---
arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts | 35 ++++++++++++-
arch/riscv/boot/dts/spacemit/k1-opp.dtsi | 70 +++++++++++++++++++++++++
arch/riscv/boot/dts/spacemit/k1.dtsi | 8 +++
3 files changed, 112 insertions(+), 1 deletion(-)
diff --git a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
index 444c3b1e6f44..81ab5f70176b 100644
--- a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
+++ b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
@@ -5,6 +5,7 @@
#include "k1.dtsi"
#include "k1-pinctrl.dtsi"
+#include "k1-opp.dtsi"
/ {
model = "Banana Pi BPI-F3";
@@ -86,6 +87,38 @@ &combo_phy {
status = "okay";
};
+&cpu_0 {
+ cpu-supply = <&buck1_0v9>;
+};
+
+&cpu_1 {
+ cpu-supply = <&buck1_0v9>;
+};
+
+&cpu_2 {
+ cpu-supply = <&buck1_0v9>;
+};
+
+&cpu_3 {
+ cpu-supply = <&buck1_0v9>;
+};
+
+&cpu_4 {
+ cpu-supply = <&buck1_0v9>;
+};
+
+&cpu_5 {
+ cpu-supply = <&buck1_0v9>;
+};
+
+&cpu_6 {
+ cpu-supply = <&buck1_0v9>;
+};
+
+&cpu_7 {
+ cpu-supply = <&buck1_0v9>;
+};
+
&emmc {
bus-width = <8>;
mmc-hs400-1_8v;
@@ -201,7 +234,7 @@ pmic@41 {
dldoin2-supply = <&buck5>;
regulators {
- buck1 {
+ buck1_0v9: buck1 {
regulator-min-microvolt = <500000>;
regulator-max-microvolt = <3450000>;
regulator-ramp-delay = <5000>;
diff --git a/arch/riscv/boot/dts/spacemit/k1-opp.dtsi b/arch/riscv/boot/dts/spacemit/k1-opp.dtsi
new file mode 100644
index 000000000000..3627812e1344
--- /dev/null
+++ b/arch/riscv/boot/dts/spacemit/k1-opp.dtsi
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+
+/ {
+ cpu_opp_table: opp-table-cpu {
+ compatible = "operating-points-v2";
+ opp-shared;
+
+ opp-614400000 {
+ opp-hz = /bits/ 64 <614400000>;
+ opp-microvolt = <950000>;
+ clock-latency-ns = <200000>;
+ };
+
+ opp-819000000 {
+ opp-hz = /bits/ 64 <819000000>;
+ opp-microvolt = <950000>;
+ clock-latency-ns = <200000>;
+ };
+
+ opp-1000000000 {
+ opp-hz = /bits/ 64 <1000000000>;
+ opp-microvolt = <950000>;
+ clock-latency-ns = <200000>;
+ };
+
+ opp-1228800000 {
+ opp-hz = /bits/ 64 <1228800000>;
+ opp-microvolt = <950000>;
+ clock-latency-ns = <200000>;
+ };
+
+ opp-1600000000 {
+ opp-hz = /bits/ 64 <1600000000>;
+ opp-microvolt = <1050000>;
+ clock-latency-ns = <200000>;
+ };
+ };
+};
+
+&cpu_0 {
+ operating-points-v2 = <&cpu_opp_table>;
+};
+
+&cpu_1 {
+ operating-points-v2 = <&cpu_opp_table>;
+};
+
+&cpu_2 {
+ operating-points-v2 = <&cpu_opp_table>;
+};
+
+&cpu_3 {
+ operating-points-v2 = <&cpu_opp_table>;
+};
+
+&cpu_4 {
+ operating-points-v2 = <&cpu_opp_table>;
+};
+
+&cpu_5 {
+ operating-points-v2 = <&cpu_opp_table>;
+};
+
+&cpu_6 {
+ operating-points-v2 = <&cpu_opp_table>;
+};
+
+&cpu_7 {
+ operating-points-v2 = <&cpu_opp_table>;
+};
diff --git a/arch/riscv/boot/dts/spacemit/k1.dtsi b/arch/riscv/boot/dts/spacemit/k1.dtsi
index 529ec68e9c23..bdd109b81730 100644
--- a/arch/riscv/boot/dts/spacemit/k1.dtsi
+++ b/arch/riscv/boot/dts/spacemit/k1.dtsi
@@ -54,6 +54,7 @@ cpu_0: cpu@0 {
compatible = "spacemit,x60", "riscv";
device_type = "cpu";
reg = <0>;
+ clocks = <&syscon_apmu CLK_CPU_C0_CORE>;
riscv,isa = "rv64imafdcbv_zicbom_zicbop_zicboz_zicntr_zicond_zicsr_zifencei_zihintpause_zihpm_zfh_zba_zbb_zbc_zbs_zkt_zvfh_zvkt_sscofpmf_sstc_svinval_svnapot_svpbmt";
riscv,isa-base = "rv64i";
riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "b", "v", "zicbom",
@@ -84,6 +85,7 @@ cpu_1: cpu@1 {
compatible = "spacemit,x60", "riscv";
device_type = "cpu";
reg = <1>;
+ clocks = <&syscon_apmu CLK_CPU_C0_CORE>;
riscv,isa = "rv64imafdcbv_zicbom_zicbop_zicboz_zicntr_zicond_zicsr_zifencei_zihintpause_zihpm_zfh_zba_zbb_zbc_zbs_zkt_zvfh_zvkt_sscofpmf_sstc_svinval_svnapot_svpbmt";
riscv,isa-base = "rv64i";
riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "b", "v", "zicbom",
@@ -114,6 +116,7 @@ cpu_2: cpu@2 {
compatible = "spacemit,x60", "riscv";
device_type = "cpu";
reg = <2>;
+ clocks = <&syscon_apmu CLK_CPU_C0_CORE>;
riscv,isa = "rv64imafdcbv_zicbom_zicbop_zicboz_zicntr_zicond_zicsr_zifencei_zihintpause_zihpm_zfh_zba_zbb_zbc_zbs_zkt_zvfh_zvkt_sscofpmf_sstc_svinval_svnapot_svpbmt";
riscv,isa-base = "rv64i";
riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "b", "v", "zicbom",
@@ -144,6 +147,7 @@ cpu_3: cpu@3 {
compatible = "spacemit,x60", "riscv";
device_type = "cpu";
reg = <3>;
+ clocks = <&syscon_apmu CLK_CPU_C0_CORE>;
riscv,isa = "rv64imafdcbv_zicbom_zicbop_zicboz_zicntr_zicond_zicsr_zifencei_zihintpause_zihpm_zfh_zba_zbb_zbc_zbs_zkt_zvfh_zvkt_sscofpmf_sstc_svinval_svnapot_svpbmt";
riscv,isa-base = "rv64i";
riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "b", "v", "zicbom",
@@ -174,6 +178,7 @@ cpu_4: cpu@4 {
compatible = "spacemit,x60", "riscv";
device_type = "cpu";
reg = <4>;
+ clocks = <&syscon_apmu CLK_CPU_C1_CORE>;
riscv,isa = "rv64imafdcbv_zicbom_zicbop_zicboz_zicntr_zicond_zicsr_zifencei_zihintpause_zihpm_zfh_zba_zbb_zbc_zbs_zkt_zvfh_zvkt_sscofpmf_sstc_svinval_svnapot_svpbmt";
riscv,isa-base = "rv64i";
riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "b", "v", "zicbom",
@@ -204,6 +209,7 @@ cpu_5: cpu@5 {
compatible = "spacemit,x60", "riscv";
device_type = "cpu";
reg = <5>;
+ clocks = <&syscon_apmu CLK_CPU_C1_CORE>;
riscv,isa = "rv64imafdcbv_zicbom_zicbop_zicboz_zicntr_zicond_zicsr_zifencei_zihintpause_zihpm_zfh_zba_zbb_zbc_zbs_zkt_zvfh_zvkt_sscofpmf_sstc_svinval_svnapot_svpbmt";
riscv,isa-base = "rv64i";
riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "b", "v", "zicbom",
@@ -234,6 +240,7 @@ cpu_6: cpu@6 {
compatible = "spacemit,x60", "riscv";
device_type = "cpu";
reg = <6>;
+ clocks = <&syscon_apmu CLK_CPU_C1_CORE>;
riscv,isa = "rv64imafdcbv_zicbom_zicbop_zicboz_zicntr_zicond_zicsr_zifencei_zihintpause_zihpm_zfh_zba_zbb_zbc_zbs_zkt_zvfh_zvkt_sscofpmf_sstc_svinval_svnapot_svpbmt";
riscv,isa-base = "rv64i";
riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "b", "v", "zicbom",
@@ -264,6 +271,7 @@ cpu_7: cpu@7 {
compatible = "spacemit,x60", "riscv";
device_type = "cpu";
reg = <7>;
+ clocks = <&syscon_apmu CLK_CPU_C1_CORE>;
riscv,isa = "rv64imafdcbv_zicbom_zicbop_zicboz_zicntr_zicond_zicsr_zifencei_zihintpause_zihpm_zfh_zba_zbb_zbc_zbs_zkt_zvfh_zvkt_sscofpmf_sstc_svinval_svnapot_svpbmt";
riscv,isa-base = "rv64i";
riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "b", "v", "zicbom",
--
2.53.0
^ permalink raw reply related
* Re: [PATCH v4 09/16] media: iris: Add framework support for AR50_LITE video core
From: sashiko-bot @ 2026-06-12 9:54 UTC (permalink / raw)
To: Dmitry Baryshkov; +Cc: conor+dt, devicetree, robh
In-Reply-To: <20260612-iris-ar50lt-v4-9-0abfb74d5b3c@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The VPU clock frequency calculation ignores the actual stream frame rate and uses a hardcoded 30 FPS.
- [High] Calculation of VSP frequency causes severe 32-bit integer overflows on 32-bit architectures, leading to under-provisioned clocks.
--
commit 117c5206f4fac821d317f597cfe56c462e1556c1
Author: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
media: iris: Add framework support for AR50_LITE video core
Add power sequence for ar5lt core.
Add register handling for ar50lt by hooking up vpu op with ar50lt
specific implemtation or resue from earlier generation wherever
feasible.
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_ar50lt.c b/drivers/media/platform/qcom/iris/iris_vpu_ar50lt.c
> new file mode 100644
> index 0000000000000..1af20b067c032
> --- /dev/null
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_ar50lt.c
[ ... ]
> +static u64 iris_vpu_ar50lt_calc_freq(struct iris_inst *inst, size_t data_size)
> +{
> + struct platform_inst_caps *caps = inst->core->iris_platform_data->inst_caps;
> + struct v4l2_format *inp_f = inst->fmt_src;
> + u32 mbs_per_second, mbpf, height, width;
> + unsigned long vpp_freq, vsp_freq;
> + u32 fps = DEFAULT_FPS;
[Severity: High]
Does this hardcode the frame rate to 30 FPS instead of using the actual
stream frame rate from the session (e.g., inst->frame_rate)? For
high-framerate streams, this might under-provision the VPU clocks and lead
to dropped frames or hardware stalls.
> +
> + width = max(inp_f->fmt.pix_mp.width, inst->crop.width);
> + height = max(inp_f->fmt.pix_mp.height, inst->crop.height);
> +
> + mbpf = NUM_MBS_PER_FRAME(height, width);
> + mbs_per_second = mbpf * fps;
> +
> + vpp_freq = mbs_per_second * caps->mb_cycles_vpp;
> +
> + /* 21 / 20 is overhead factor */
> + vpp_freq += vpp_freq / 20;
> + vsp_freq = mbs_per_second * caps->mb_cycles_vsp;
> +
> + /* 10 / 7 is overhead factor */
> + vsp_freq += ((fps * data_size * 8) * 10) / 7;
[Severity: High]
Can this calculation overflow on 32-bit architectures? Since size_t is 32-bit
on these platforms, the intermediate product could exceed the 32-bit unsigned
maximum before the division by 7, resulting in a wrap-around and an incorrectly
low requested clock frequency.
> +
> + return max(vpp_freq, vsp_freq);
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-iris-ar50lt-v4-0-0abfb74d5b3c@oss.qualcomm.com?part=9
^ permalink raw reply
* Re: [PATCH v4 10/16] media: iris: add minimal GET_PROPERTY implementation
From: sashiko-bot @ 2026-06-12 9:56 UTC (permalink / raw)
To: Dmitry Baryshkov; +Cc: devicetree, conor+dt, robh
In-Reply-To: <20260612-iris-ar50lt-v4-10-0abfb74d5b3c@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Possible Use-After-Free in firmware response handler due to lockless instance lookup and lack of reference counting.
--
commit ea864f98c9ad1ba57f2027e2f0fc7f82f044f629
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
media: iris: add minimal GET_PROPERTY implementation
AR50Lt with the Gen1 firmware requires host to read
HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS property, otherwie it doesn't
update internal data and fails the HFI_CMD_SESSION_LOAD_RESOURCES
command. Implement minimal support for querying the properties from the
firmware.
> AR50Lt with the Gen1 firmware requires host to read
> HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS property, otherwie it doesn't
This isn't a bug, but there's a typo (otherwie) in the commit message.
> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
> index bfd7495bf44f0..23fc7194b1e3a 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
[ ... ]
> @@ -652,6 +656,8 @@ static void iris_hfi_gen1_handle_response(struct iris_core *core, void *response
> iris_hfi_gen1_session_etb_done(inst, hdr);
> } else if (hdr->pkt_type == HFI_MSG_SESSION_FILL_BUFFER) {
> iris_hfi_gen1_session_ftb_done(inst, hdr);
> + } else if (hdr->pkt_type == HFI_MSG_SESSION_PROPERTY_INFO) {
> + complete(&inst->completion);
[Severity: High]
This is a pre-existing issue, but could there be a use-after-free race condition
when handling firmware responses here?
Earlier in iris_hfi_gen1_handle_response(), the instance is looked up using
iris_get_instance():
inst = iris_get_instance(core, pkt->shdr.session_id);
...
mutex_lock(&inst->lock);
iris_get_instance() finds the instance in the core->instances list and returns
a raw pointer after dropping core->lock, without incrementing a reference count.
Concurrently, if a userspace process closes the device node, iris_close() might
remove the instance from the list, unlock inst->lock, destroy the mutex, and
call kfree(inst).
If the response handler thread calls iris_get_instance() just before iris_close()
removes the instance from the list, the response thread could receive the pointer
and block on mutex_lock(&inst->lock) while iris_close() holds it.
After iris_close() finishes teardown and frees the instance, the response thread
would be unblocked and attempt to access the freed memory and destroyed mutex.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-iris-ar50lt-v4-0-0abfb74d5b3c@oss.qualcomm.com?part=10
^ permalink raw reply
* [PATCH v2 2/3] perf: marvell: Add CN20K LLC-TAD PMU support
From: Geetha sowjanya @ 2026-06-12 9:57 UTC (permalink / raw)
To: linux-perf-users, linux-kernel, linux-arm-kernel, devicetree
Cc: mark.rutland, will, krzk+dt, gakula
In-Reply-To: <20260612095746.19679-1-gakula@marvell.com>
Add support for the LLC Tag-and-Data (TAD) PMU present in
Marvell CN20K SoCs.
The CN20K TAD PMU is based on the CN10K design but differs in the
layout of PFC/PRF register offsets relative to each TAD base, and
introduces additional events. These offsets are selected by the driver
based on the compatible string and are not described via DT properties.
Because of this, "marvell,cn10k-tad-pmu" cannot be used as a fallback
for CN20K, as it would result in incorrect register programming.
Add support for "marvell,cn20k-tad-pmu" by:
- Introducing a TAD_PMU_V3 profile with CN20K-specific register bases
- Extending the event map for new CN20K events
- Matching the PMU via OF and ACPI (MRVL000F)
Signed-off-by: Geetha sowjanya <gakula@marvell.com>
---
Changelog (since v1)
--------------------
- Hide V3-only events on CN10K via sysfs is_visible and reject them in
event_init.
- Use CN20K-specific MPAM PRF bits (MATCH_MPAMNS, partid << 10) for V3;
software partid is limited to nine bits so this does not collide with
the fixed bit at 25.
- Reset hwc->prev_count when starting counters so reads match cleared HW.
drivers/perf/marvell_cn10k_tad_pmu.c | 54 ++++++++++++++++++++++++++--
1 file changed, 52 insertions(+), 2 deletions(-)
diff --git a/drivers/perf/marvell_cn10k_tad_pmu.c b/drivers/perf/marvell_cn10k_tad_pmu.c
index af706b890bf1..e43598a52859 100644
--- a/drivers/perf/marvell_cn10k_tad_pmu.c
+++ b/drivers/perf/marvell_cn10k_tad_pmu.c
@@ -17,11 +17,14 @@
#define TAD_PRF_OFFSET 0x900
#define TAD_PFC_OFFSET 0x800
+#define TAD_PRF_NS_OFFSET 0x30900
+#define TAD_PFC_NS_OFFSET 0x30800
#define TAD_PFC(base, counter) ((base) | ((u64)(counter) << 3))
#define TAD_PRF(base, counter) ((base) | ((u64)(counter) << 3))
#define TAD_PRF_CNTSEL_MASK 0xFF
#define TAD_PRF_MATCH_PARTID BIT(8)
#define TAD_PRF_PARTID_NS BIT(10)
+#define TAD_PRF_MATCH_MPAMNS BIT(25)
/*
* config1: bits 0..8 MPAM partition id (including 0); bit 9 requests
* filtering for MPAM-capable events. All-zero config1 means no filter.
@@ -38,6 +41,7 @@ struct tad_region {
enum mrvl_tad_pmu_version {
TAD_PMU_V1 = 1,
TAD_PMU_V2,
+ TAD_PMU_V3,
};
struct tad_pmu_data {
@@ -85,8 +89,15 @@ static void tad_pmu_start_counter(struct tad_pmu *pmu,
if (use_mpam && event_idx > 0x19 && event_idx < 0x21) {
partid_filter = TAD_PRF_MATCH_PARTID | TAD_PRF_PARTID_NS |
((u64)partid << 11);
+
+ if (pdata->id == TAD_PMU_V3)
+ partid_filter = TAD_PRF_MATCH_PARTID | TAD_PRF_MATCH_MPAMNS |
+ ((u64)partid << 10);
}
+ /* CN10K support events 0:24*/
+ if (pdata->id == TAD_PMU_V1 && event_idx >= 0x25)
+ return;
for (i = 0; i < pmu->region_cnt; i++) {
reg_val = event_idx & 0xFF;
@@ -159,6 +170,7 @@ static void tad_pmu_event_counter_start(struct perf_event *event, int flags)
struct hw_perf_event *hwc = &event->hw;
hwc->state = 0;
+ local64_set(&hwc->prev_count, 0);
tad_pmu->ops->start_counter(tad_pmu, event);
}
@@ -216,6 +228,8 @@ static int tad_pmu_event_init(struct perf_event *event)
if (cfg1)
return -EINVAL;
} else {
+ if (pdata->id == TAD_PMU_V1 && event_idx >= 0x25)
+ return -EINVAL;
if ((cfg1 & GENMASK(8, 0)) && !(cfg1 & TAD_PARTID_FILTER_EN))
return -EINVAL;
if (cfg1 & TAD_PARTID_FILTER_EN) {
@@ -242,6 +256,22 @@ static ssize_t tad_pmu_event_show(struct device *dev,
return sysfs_emit(page, "event=0x%02llx\n", pmu_attr->id);
}
+static umode_t tad_pmu_event_attr_is_visible(struct kobject *kobj,
+ struct attribute *attr, int unused)
+{
+ struct pmu *pmu = dev_get_drvdata(kobj_to_dev(kobj));
+ struct tad_pmu *t = to_tad_pmu(pmu);
+ struct device_attribute *da = container_of(attr, struct device_attribute,
+ attr);
+ struct perf_pmu_events_attr *e = container_of(da, struct perf_pmu_events_attr,
+ attr);
+ u64 id = e->id;
+
+ if (t->pdata->id != TAD_PMU_V3 && id >= 0x25)
+ return 0;
+ return attr->mode;
+}
+
#define TAD_PMU_EVENT_ATTR(name, config) \
PMU_EVENT_ATTR_ID(name, tad_pmu_event_show, config)
@@ -283,12 +313,25 @@ static struct attribute *tad_pmu_event_attrs[] = {
TAD_PMU_EVENT_ATTR(tad_dat_rd_byp, 0x22),
TAD_PMU_EVENT_ATTR(tad_ifb_occ, 0x23),
TAD_PMU_EVENT_ATTR(tad_req_occ, 0x24),
+ TAD_PMU_EVENT_ATTR(tad_req_msh_out_dtg_evict, 0x25),
+ TAD_PMU_EVENT_ATTR(tad_req_msh_out_ltg_evict, 0x26),
+ TAD_PMU_EVENT_ATTR(tad_rsp_msh_out_mpam, 0x28),
+ TAD_PMU_EVENT_ATTR(tad_replays, 0x29),
+ TAD_PMU_EVENT_ATTR(tad_req_byp0, 0x2a),
+ TAD_PMU_EVENT_ATTR(tad_req_byp1, 0x2b),
+ TAD_PMU_EVENT_ATTR(tad_txreq_byp, 0x2c),
+ TAD_PMU_EVENT_ATTR(tad_time_in_dslp, 0x2d),
+ TAD_PMU_EVENT_ATTR(tad_time_elapsed, 0x2e),
+ TAD_PMU_EVENT_ATTR(tad_req_msh_out_dss_rd_128mrg, 0x2f),
+ TAD_PMU_EVENT_ATTR(tad_req_msh_out_dss_wr_128mrg, 0x30),
+ TAD_PMU_EVENT_ATTR(tad_tot_cycle, 0xff),
NULL
};
static const struct attribute_group tad_pmu_events_attr_group = {
.name = "events",
.attrs = tad_pmu_event_attrs,
+ .is_visible = tad_pmu_event_attr_is_visible,
};
static struct attribute *ody_tad_pmu_event_attrs[] = {
@@ -474,7 +517,7 @@ static int tad_pmu_probe(struct platform_device *pdev)
.read = tad_pmu_event_counter_read,
};
- if (version == TAD_PMU_V1) {
+ if (version == TAD_PMU_V1 || version == TAD_PMU_V3) {
tad_pmu->pmu.attr_groups = tad_pmu_attr_groups;
tad_pmu->ops = &tad_pmu_ops;
} else {
@@ -517,6 +560,11 @@ static const struct tad_pmu_data tad_pmu_data = {
.tad_pfc_offset = TAD_PFC_OFFSET,
};
+static const struct tad_pmu_data tad_pmu_cn20k_data = {
+ .id = TAD_PMU_V3,
+ .tad_prf_offset = TAD_PRF_NS_OFFSET,
+ .tad_pfc_offset = TAD_PFC_NS_OFFSET,
+};
#endif
#ifdef CONFIG_ACPI
@@ -530,6 +578,7 @@ static const struct tad_pmu_data tad_pmu_v2_data = {
#ifdef CONFIG_OF
static const struct of_device_id tad_pmu_of_match[] = {
{ .compatible = "marvell,cn10k-tad-pmu", .data = &tad_pmu_data },
+ { .compatible = "marvell,cn20k-tad-pmu", .data = &tad_pmu_cn20k_data },
{},
};
#endif
@@ -538,6 +587,7 @@ static const struct of_device_id tad_pmu_of_match[] = {
static const struct acpi_device_id tad_pmu_acpi_match[] = {
{"MRVL000B", (kernel_ulong_t)&tad_pmu_data},
{"MRVL000D", (kernel_ulong_t)&tad_pmu_v2_data},
+ {"MRVL000F", (kernel_ulong_t)&tad_pmu_cn20k_data},
{},
};
MODULE_DEVICE_TABLE(acpi, tad_pmu_acpi_match);
@@ -599,6 +649,6 @@ static void __exit tad_pmu_exit(void)
module_init(tad_pmu_init);
module_exit(tad_pmu_exit);
-MODULE_DESCRIPTION("Marvell CN10K LLC-TAD perf driver");
+MODULE_DESCRIPTION("Marvell CN10K/CN20K LLC-TAD perf driver");
MODULE_AUTHOR("Bhaskara Budiredla <bbudiredla@marvell.com>");
MODULE_LICENSE("GPL v2");
--
2.25.1
^ permalink raw reply related
* [PATCH v2 1/3] perf: marvell: Add MPAM partid filtering to CN10K TAD PMU
From: Geetha sowjanya @ 2026-06-12 9:57 UTC (permalink / raw)
To: linux-perf-users, linux-kernel, linux-arm-kernel, devicetree
Cc: mark.rutland, will, krzk+dt, gakula
In-Reply-To: <20260612095746.19679-1-gakula@marvell.com>
From: Tanmay Jagdale <tanmay@marvell.com>
The TAD PMU exposes counters that can be filtered by MPAM partition id
for a subset of allocation and hit events.
Add a 9-bit partid format attribute (config1) and route counter programming
through variant-specific ops so CN10K keeps MPAM-capable programming while
Odyssey keeps the reduced event set without advertising partid in sysfs.
Probe no longer mutates the platform_device MMIO resource (walk a local
map_start), rejects tad-cnt / page sizes of zero, validates the memory
window against tad-cnt, and registers the perf PMU before hotplug with
correct unwind.
Example:
perf stat -e tad/tad_alloc_any,partid=0x12,partid_en=1/ -- <program>
Signed-off-by: Tanmay Jagdale <tanmay@marvell.com>
---
Changelog (since v1)
--------------------
- Fix config1 filter enable to use bit 9 consistently with the PMU format
string (partid_en) and reject reserved bits with GENMASK(9, 0).
- Register perf_pmu_register before cpuhp_state_add_instance_nocalls and
unregister on hotplug failure.
drivers/perf/marvell_cn10k_tad_pmu.c | 212 ++++++++++++++++++++-------
1 file changed, 160 insertions(+), 52 deletions(-)
diff --git a/drivers/perf/marvell_cn10k_tad_pmu.c b/drivers/perf/marvell_cn10k_tad_pmu.c
index 51ccb0befa05..af706b890bf1 100644
--- a/drivers/perf/marvell_cn10k_tad_pmu.c
+++ b/drivers/perf/marvell_cn10k_tad_pmu.c
@@ -7,6 +7,7 @@
#define pr_fmt(fmt) "tad_pmu: " fmt
#include <linux/io.h>
+#include <linux/bits.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/cpuhotplug.h>
@@ -14,11 +15,18 @@
#include <linux/platform_device.h>
#include <linux/acpi.h>
-#define TAD_PFC_OFFSET 0x800
-#define TAD_PFC(counter) (TAD_PFC_OFFSET | (counter << 3))
#define TAD_PRF_OFFSET 0x900
-#define TAD_PRF(counter) (TAD_PRF_OFFSET | (counter << 3))
+#define TAD_PFC_OFFSET 0x800
+#define TAD_PFC(base, counter) ((base) | ((u64)(counter) << 3))
+#define TAD_PRF(base, counter) ((base) | ((u64)(counter) << 3))
#define TAD_PRF_CNTSEL_MASK 0xFF
+#define TAD_PRF_MATCH_PARTID BIT(8)
+#define TAD_PRF_PARTID_NS BIT(10)
+/*
+ * config1: bits 0..8 MPAM partition id (including 0); bit 9 requests
+ * filtering for MPAM-capable events. All-zero config1 means no filter.
+ */
+#define TAD_PARTID_FILTER_EN BIT(9)
#define TAD_MAX_COUNTERS 8
#define to_tad_pmu(p) (container_of(p, struct tad_pmu, pmu))
@@ -27,30 +35,92 @@ struct tad_region {
void __iomem *base;
};
+enum mrvl_tad_pmu_version {
+ TAD_PMU_V1 = 1,
+ TAD_PMU_V2,
+};
+
+struct tad_pmu_data {
+ int id;
+ u64 tad_prf_offset;
+ u64 tad_pfc_offset;
+};
+
struct tad_pmu {
struct pmu pmu;
struct tad_region *regions;
u32 region_cnt;
unsigned int cpu;
+ const struct tad_pmu_ops *ops;
+ const struct tad_pmu_data *pdata;
struct hlist_node node;
struct perf_event *events[TAD_MAX_COUNTERS];
DECLARE_BITMAP(counters_map, TAD_MAX_COUNTERS);
};
-enum mrvl_tad_pmu_version {
- TAD_PMU_V1 = 1,
- TAD_PMU_V2,
-};
-
-struct tad_pmu_data {
- int id;
+struct tad_pmu_ops {
+ void (*start_counter)(struct tad_pmu *pmu, struct perf_event *event);
};
static int tad_pmu_cpuhp_state;
+static void tad_pmu_start_counter(struct tad_pmu *pmu,
+ struct perf_event *event)
+{
+ const struct tad_pmu_data *pdata = pmu->pdata;
+ struct hw_perf_event *hwc = &event->hw;
+ u32 event_idx = event->attr.config;
+ u32 counter_idx = hwc->idx;
+ u64 partid_filter = 0;
+ u64 reg_val;
+ u64 cfg1 = event->attr.config1;
+ bool use_mpam = cfg1 & TAD_PARTID_FILTER_EN;
+ u32 partid = (u32)(cfg1 & GENMASK(8, 0));
+ int i;
+
+ for (i = 0; i < pmu->region_cnt; i++)
+ writeq_relaxed(0, pmu->regions[i].base +
+ TAD_PFC(pdata->tad_pfc_offset, counter_idx));
+
+ if (use_mpam && event_idx > 0x19 && event_idx < 0x21) {
+ partid_filter = TAD_PRF_MATCH_PARTID | TAD_PRF_PARTID_NS |
+ ((u64)partid << 11);
+ }
+
+
+ for (i = 0; i < pmu->region_cnt; i++) {
+ reg_val = event_idx & 0xFF;
+ reg_val |= partid_filter;
+ writeq_relaxed(reg_val, pmu->regions[i].base +
+ TAD_PRF(pdata->tad_prf_offset, counter_idx));
+ }
+}
+
+static void tad_pmu_v2_start_counter(struct tad_pmu *pmu,
+ struct perf_event *event)
+{
+ const struct tad_pmu_data *pdata = pmu->pdata;
+ struct hw_perf_event *hwc = &event->hw;
+ u32 event_idx = event->attr.config;
+ u32 counter_idx = hwc->idx;
+ u64 reg_val;
+ int i;
+
+ for (i = 0; i < pmu->region_cnt; i++)
+ writeq_relaxed(0, pmu->regions[i].base +
+ TAD_PFC(pdata->tad_pfc_offset, counter_idx));
+
+ for (i = 0; i < pmu->region_cnt; i++) {
+ reg_val = event_idx & 0xFF;
+ writeq_relaxed(reg_val, pmu->regions[i].base +
+ TAD_PRF(pdata->tad_prf_offset, counter_idx));
+ }
+}
+
static void tad_pmu_event_counter_read(struct perf_event *event)
{
struct tad_pmu *tad_pmu = to_tad_pmu(event->pmu);
+ const struct tad_pmu_data *pdata = tad_pmu->pdata;
struct hw_perf_event *hwc = &event->hw;
u32 counter_idx = hwc->idx;
u64 prev, new;
@@ -60,7 +130,7 @@ static void tad_pmu_event_counter_read(struct perf_event *event)
prev = local64_read(&hwc->prev_count);
for (i = 0, new = 0; i < tad_pmu->region_cnt; i++)
new += readq(tad_pmu->regions[i].base +
- TAD_PFC(counter_idx));
+ TAD_PFC(pdata->tad_pfc_offset, counter_idx));
} while (local64_cmpxchg(&hwc->prev_count, prev, new) != prev);
local64_add(new - prev, &event->count);
@@ -69,16 +139,14 @@ static void tad_pmu_event_counter_read(struct perf_event *event)
static void tad_pmu_event_counter_stop(struct perf_event *event, int flags)
{
struct tad_pmu *tad_pmu = to_tad_pmu(event->pmu);
+ const struct tad_pmu_data *pdata = tad_pmu->pdata;
struct hw_perf_event *hwc = &event->hw;
u32 counter_idx = hwc->idx;
int i;
- /* TAD()_PFC() stop counting on the write
- * which sets TAD()_PRF()[CNTSEL] == 0
- */
for (i = 0; i < tad_pmu->region_cnt; i++) {
writeq_relaxed(0, tad_pmu->regions[i].base +
- TAD_PRF(counter_idx));
+ TAD_PRF(pdata->tad_prf_offset, counter_idx));
}
tad_pmu_event_counter_read(event);
@@ -89,26 +157,10 @@ static void tad_pmu_event_counter_start(struct perf_event *event, int flags)
{
struct tad_pmu *tad_pmu = to_tad_pmu(event->pmu);
struct hw_perf_event *hwc = &event->hw;
- u32 event_idx = event->attr.config;
- u32 counter_idx = hwc->idx;
- u64 reg_val;
- int i;
hwc->state = 0;
- /* Typically TAD_PFC() are zeroed to start counting */
- for (i = 0; i < tad_pmu->region_cnt; i++)
- writeq_relaxed(0, tad_pmu->regions[i].base +
- TAD_PFC(counter_idx));
-
- /* TAD()_PFC() start counting on the write
- * which sets TAD()_PRF()[CNTSEL] != 0
- */
- for (i = 0; i < tad_pmu->region_cnt; i++) {
- reg_val = event_idx & 0xFF;
- writeq_relaxed(reg_val, tad_pmu->regions[i].base +
- TAD_PRF(counter_idx));
- }
+ tad_pmu->ops->start_counter(tad_pmu, event);
}
static void tad_pmu_event_counter_del(struct perf_event *event, int flags)
@@ -128,7 +180,6 @@ static int tad_pmu_event_counter_add(struct perf_event *event, int flags)
struct hw_perf_event *hwc = &event->hw;
int idx;
- /* Get a free counter for this event */
idx = find_first_zero_bit(tad_pmu->counters_map, TAD_MAX_COUNTERS);
if (idx == TAD_MAX_COUNTERS)
return -EAGAIN;
@@ -148,6 +199,9 @@ static int tad_pmu_event_counter_add(struct perf_event *event, int flags)
static int tad_pmu_event_init(struct perf_event *event)
{
struct tad_pmu *tad_pmu = to_tad_pmu(event->pmu);
+ const struct tad_pmu_data *pdata = tad_pmu->pdata;
+ u32 event_idx = (u32)(event->attr.config & GENMASK(7, 0));
+ u64 cfg1 = event->attr.config1;
if (event->attr.type != event->pmu->type)
return -ENOENT;
@@ -158,6 +212,20 @@ static int tad_pmu_event_init(struct perf_event *event)
if (event->state != PERF_EVENT_STATE_OFF)
return -EINVAL;
+ if (pdata->id == TAD_PMU_V2) {
+ if (cfg1)
+ return -EINVAL;
+ } else {
+ if ((cfg1 & GENMASK(8, 0)) && !(cfg1 & TAD_PARTID_FILTER_EN))
+ return -EINVAL;
+ if (cfg1 & TAD_PARTID_FILTER_EN) {
+ if (event_idx <= 0x19 || event_idx >= 0x21)
+ return -EINVAL;
+ }
+ if (cfg1 & ~GENMASK(9, 0))
+ return -EINVAL;
+ }
+
event->cpu = tad_pmu->cpu;
event->hw.idx = -1;
event->hw.config_base = event->attr.config;
@@ -232,7 +300,7 @@ static struct attribute *ody_tad_pmu_event_attrs[] = {
TAD_PMU_EVENT_ATTR(tad_hit_ltg, 0x1e),
TAD_PMU_EVENT_ATTR(tad_hit_any, 0x1f),
TAD_PMU_EVENT_ATTR(tad_tag_rd, 0x20),
- TAD_PMU_EVENT_ATTR(tad_tot_cycle, 0xFF),
+ TAD_PMU_EVENT_ATTR(tad_tot_cycle, 0xff),
NULL
};
@@ -242,9 +310,13 @@ static const struct attribute_group ody_tad_pmu_events_attr_group = {
};
PMU_FORMAT_ATTR(event, "config:0-7");
+PMU_FORMAT_ATTR(partid, "config1:0-8");
+PMU_FORMAT_ATTR(partid_en, "config1:9-9");
static struct attribute *tad_pmu_format_attrs[] = {
&format_attr_event.attr,
+ &format_attr_partid.attr,
+ &format_attr_partid_en.attr,
NULL
};
@@ -253,6 +325,16 @@ static struct attribute_group tad_pmu_format_attr_group = {
.attrs = tad_pmu_format_attrs,
};
+static struct attribute *ody_tad_pmu_format_attrs[] = {
+ &format_attr_event.attr,
+ NULL
+};
+
+static struct attribute_group ody_tad_pmu_format_attr_group = {
+ .name = "format",
+ .attrs = ody_tad_pmu_format_attrs,
+};
+
static ssize_t tad_pmu_cpumask_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -281,16 +363,25 @@ static const struct attribute_group *tad_pmu_attr_groups[] = {
static const struct attribute_group *ody_tad_pmu_attr_groups[] = {
&ody_tad_pmu_events_attr_group,
- &tad_pmu_format_attr_group,
+ &ody_tad_pmu_format_attr_group,
&tad_pmu_cpumask_attr_group,
NULL
};
+static const struct tad_pmu_ops tad_pmu_ops = {
+ .start_counter = tad_pmu_start_counter,
+};
+
+static const struct tad_pmu_ops tad_pmu_v2_ops = {
+ .start_counter = tad_pmu_v2_start_counter,
+};
+
static int tad_pmu_probe(struct platform_device *pdev)
{
const struct tad_pmu_data *dev_data;
struct device *dev = &pdev->dev;
struct tad_region *regions;
+ resource_size_t map_start;
struct tad_pmu *tad_pmu;
struct resource *res;
u32 tad_pmu_page_size;
@@ -298,7 +389,6 @@ static int tad_pmu_probe(struct platform_device *pdev)
u32 tad_cnt;
int version;
int i, ret;
- char *name;
tad_pmu = devm_kzalloc(&pdev->dev, sizeof(*tad_pmu), GFP_KERNEL);
if (!tad_pmu)
@@ -312,6 +402,7 @@ static int tad_pmu_probe(struct platform_device *pdev)
return -ENODEV;
}
version = dev_data->id;
+ tad_pmu->pdata = dev_data;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!res) {
@@ -338,22 +429,31 @@ static int tad_pmu_probe(struct platform_device *pdev)
dev_err(&pdev->dev, "Can't find tad-cnt property\n");
return ret;
}
+ if (!tad_cnt || !tad_page_size || !tad_pmu_page_size) {
+ dev_err(&pdev->dev, "Invalid tad-cnt or page size\n");
+ return -EINVAL;
+ }
regions = devm_kcalloc(&pdev->dev, tad_cnt,
sizeof(*regions), GFP_KERNEL);
if (!regions)
return -ENOMEM;
- /* ioremap the distributed TAD pmu regions */
- for (i = 0; i < tad_cnt && res->start < res->end; i++) {
- regions[i].base = devm_ioremap(&pdev->dev,
- res->start,
+ map_start = res->start;
+ for (i = 0; i < tad_cnt; i++) {
+ if (map_start > res->end ||
+ tad_pmu_page_size > (resource_size_t)(res->end - map_start + 1)) {
+ dev_err(&pdev->dev, "TAD PMU mem window too small for tad-cnt=%u\n",
+ tad_cnt);
+ return -EINVAL;
+ }
+ regions[i].base = devm_ioremap(&pdev->dev, map_start,
tad_pmu_page_size);
if (!regions[i].base) {
dev_err(&pdev->dev, "TAD%d ioremap fail\n", i);
return -ENOMEM;
}
- res->start += tad_page_size;
+ map_start += tad_page_size;
}
tad_pmu->regions = regions;
@@ -374,28 +474,31 @@ static int tad_pmu_probe(struct platform_device *pdev)
.read = tad_pmu_event_counter_read,
};
- if (version == TAD_PMU_V1)
+ if (version == TAD_PMU_V1) {
tad_pmu->pmu.attr_groups = tad_pmu_attr_groups;
- else
+ tad_pmu->ops = &tad_pmu_ops;
+ } else {
tad_pmu->pmu.attr_groups = ody_tad_pmu_attr_groups;
+ tad_pmu->ops = &tad_pmu_v2_ops;
+ }
tad_pmu->cpu = raw_smp_processor_id();
- /* Register pmu instance for cpu hotplug */
+ ret = perf_pmu_register(&tad_pmu->pmu, "tad", -1);
+ if (ret) {
+ dev_err(&pdev->dev, "Error %d registering perf PMU\n", ret);
+ return ret;
+ }
+
ret = cpuhp_state_add_instance_nocalls(tad_pmu_cpuhp_state,
&tad_pmu->node);
if (ret) {
dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
+ perf_pmu_unregister(&tad_pmu->pmu);
return ret;
}
- name = "tad";
- ret = perf_pmu_register(&tad_pmu->pmu, name, -1);
- if (ret)
- cpuhp_state_remove_instance_nocalls(tad_pmu_cpuhp_state,
- &tad_pmu->node);
-
- return ret;
+ return 0;
}
static void tad_pmu_remove(struct platform_device *pdev)
@@ -410,12 +513,17 @@ static void tad_pmu_remove(struct platform_device *pdev)
#if defined(CONFIG_OF) || defined(CONFIG_ACPI)
static const struct tad_pmu_data tad_pmu_data = {
.id = TAD_PMU_V1,
+ .tad_prf_offset = TAD_PRF_OFFSET,
+ .tad_pfc_offset = TAD_PFC_OFFSET,
};
+
#endif
#ifdef CONFIG_ACPI
static const struct tad_pmu_data tad_pmu_v2_data = {
.id = TAD_PMU_V2,
+ .tad_prf_offset = TAD_PRF_OFFSET,
+ .tad_pfc_offset = TAD_PFC_OFFSET,
};
#endif
@@ -491,6 +599,6 @@ static void __exit tad_pmu_exit(void)
module_init(tad_pmu_init);
module_exit(tad_pmu_exit);
-MODULE_DESCRIPTION("Marvell CN10K LLC-TAD Perf driver");
+MODULE_DESCRIPTION("Marvell CN10K LLC-TAD perf driver");
MODULE_AUTHOR("Bhaskara Budiredla <bbudiredla@marvell.com>");
MODULE_LICENSE("GPL v2");
--
2.25.1
^ permalink raw reply related
* [PATCH v2 0/3] perf: marvell: LLC-TAD PMU MPAM filtering support
From: Geetha sowjanya @ 2026-06-12 9:57 UTC (permalink / raw)
To: linux-perf-users, linux-kernel, linux-arm-kernel, devicetree
Cc: mark.rutland, will, krzk+dt, gakula
This series extends the Marvell LLC-TAD PMU driver for CN10K and CN20K
platforms by adding MPAM-based filtering support and introducing CN20K
hardware support.
Patch 1 adds optional MPAM partition-id (partid) filtering for the subset
of events that support it. The partid and partid_en fields are exposed via
the PMU format attribute, while platforms that do not support filtering
continue to expose a reduced event set without these fields.
This patch also includes several fixes and cleanups:
- Avoid modifying platform_get_resource() bounds in-place
- Validate the MMIO window size against tad-cnt
- Correct ordering of perf registration and CPU hotplug with proper unwind
- Align the filter-enable bit in config1 with the sysfs format (bit 9)
Patch 2 adds support for the CN20K LLC-TAD PMU. Compared to CN10K, CN20K
uses different PFC/PRF register offsets and introduces additional events.
This patch:
- Adds a CN20K (V3) profile with platform-specific register offsets
- Extends the event map and hides CN20K-only events on CN10K
- Implements CN20K-specific MPAM encoding for filtering
- Ensures correct counter initialization using local64_set(prev_count)
- Adds device discovery via OF and ACPI (MRVL000F)
Patch 3 updates the Devicetree binding documentation to add support for
"marvell,cn20k-tad-pmu"
Changes since v1
----------------
- config1: use bit 9 for MPAM filter enable consistently with partid_en in
the PMU format; allow only bits 0..9 in event_init on CN10K/CN20K paths.
- Hide V3-only sysfs events on V1.
- Reset prev_count when starting counters after clearing hardware.
- DT binding: explain non-fallback compatibles for CN10K vs CN20K.
Tanmay Jagdale (1):
perf: marvell: Add MPAM partid filtering to CN10K TAD PMU
Geetha sowjanya (2):
perf: marvell: Add CN20K LLC-TAD PMU support
dt-bindings: perf: marvell: Extend CN10K TAD PMU binding for CN20K
Signed-off-by: Geetha sowjanya <gakula@marvell.com>
--
2.25.1
^ permalink raw reply
* [PATCH v2 3/3] dt-bindings: perf: marvell: add CN20K TAD PMU support
From: Geetha sowjanya @ 2026-06-12 9:57 UTC (permalink / raw)
To: linux-perf-users, linux-kernel, linux-arm-kernel, devicetree
Cc: mark.rutland, will, krzk+dt, gakula
In-Reply-To: <20260612095746.19679-1-gakula@marvell.com>
Marvell CN20K SoCs integrate a Performance Monitoring Unit (PMU)
associated with the LLC Tag-and-Data (TAD) blocks. The PMU provides
hardware counters to monitor cache traffic and performance events
via a dedicated MMIO region.
The CN20K LLC-TAD PMU is largely similar to CN10K, but differs in the
layout of PFC/PRF register offsets relative to each TAD base. These
offsets are derived from the compatible string in the driver and are
not described through Devicetree properties.
Because of this, using "marvell,cn10k-tad-pmu" as a fallback for CN20K
would result in incorrect register programming. Therefore, add a
separate compatible string:
"marvell,cn20k-tad-pmu"
Update the binding to document CN20K alongside CN10K.
Signed-off-by: Geetha sowjanya <gakula@marvell.com>
---
Documentation/devicetree/bindings/perf/marvell-cn10k-tad.yaml | 17 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/perf/marvell-cn10k-tad.yaml b/Documentation/devicetree/bindings/perf/marvell-cn10k-tad.yaml
index 362142252667..d11121a1e2c9 100644
--- a/Documentation/devicetree/bindings/perf/marvell-cn10k-tad.yaml
+++ b/Documentation/devicetree/bindings/perf/marvell-cn10k-tad.yaml
@@ -4,23 +4,32 @@
$id: http://devicetree.org/schemas/perf/marvell-cn10k-tad.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#
-title: Marvell CN10K LLC-TAD performance monitor
+title: Marvell CN10K / CN20K LLC-TAD performance monitor
maintainers:
- Bhaskara Budiredla <bbudiredla@marvell.com>
+ - Geetha sowjanya <gakula@marvell.com>
description: |
- The Tag-and-Data units (TADs) maintain coherence and contain CN10K
- shared on-chip last level cache (LLC). The tad pmu measures the
- performance of last-level cache. Each tad pmu supports up to eight
- counters.
+ The Tag-and-Data units (TADs) maintain coherence and contain the
+ shared on-chip last level cache (LLC) on Marvell CN10K and CN20K SoCs.
+ The TAD PMU measures last-level cache performance. Each TAD PMU
+ supports up to eight counters.
- The DT setup comprises of number of tad blocks, the sizes of pmu
- regions, tad blocks and overall base address of the HW.
+ The DT setup describes the number of TAD blocks, the sizes of PMU
+ regions and TAD pages, and the overall MMIO base of the hardware.
+
+ marvell,cn20k-tad-pmu is not a compatible fallback for
+ marvell,cn10k-tad-pmu (and vice versa): the driver selects different
+ PFC/PRF MMIO offsets from the compatible string, and those offsets are
+ not described by separate DT properties today.
properties:
compatible:
- const: marvell,cn10k-tad-pmu
+ items:
+ - enum:
+ - marvell,cn10k-tad-pmu
+ - marvell,cn20k-tad-pmu
reg:
maxItems: 1
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v4 6/8] Bluetooth: hci_sync: Add NVMEM-backed BD address retrieval
From: Loic Poulain @ 2026-06-12 10:00 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bjorn Andersson, Konrad Dybcio, Jens Axboe, Johannes Berg,
Jeff Johnson, Bartosz Golaszewski, Marcel Holtmann,
Luiz Augusto von Dentz, Balakrishna Godavarthi, Rocky Liao,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Srinivas Kandagatla, Andrew Lunn, Heiner Kallweit,
Russell King, Saravana Kannan, linux-mmc, devicetree,
linux-kernel, linux-arm-msm, linux-block, linux-wireless, ath10k,
linux-bluetooth, netdev, daniel, Bartosz Golaszewski
In-Reply-To: <sy2ofvdbcxspxtmfdavjvdz7oes5ieuep4znf4ayknmuwhrlgk@7lp3bkegaeif>
On Fri, Jun 12, 2026 at 11:11 AM Dmitry Baryshkov
<dmitry.baryshkov@oss.qualcomm.com> wrote:
>
> On Tue, Jun 09, 2026 at 09:52:31AM +0200, Loic Poulain wrote:
> > Some devices store the Bluetooth BD address in non-volatile
> > memory, which can be accessed through the NVMEM framework.
> > Similar to Ethernet or WiFi MAC addresses, add support for
> > reading the BD address from a 'local-bd-address' NVMEM cell.
> >
> > As with the device-tree provided BD address, add a quirk to
> > indicate whether a device or platform should attempt to read
> > the address from NVMEM when no valid in-chip address is present.
> > Also add a quirk to indicate if the address is stored in
> > big-endian byte order.
> >
> > Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> > Signed-off-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
> > ---
> > include/net/bluetooth/hci.h | 18 ++++++++++++++++++
> > net/bluetooth/hci_sync.c | 39 ++++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 56 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 572b1c620c5d653a1fe10b26c1b0ba33e8f4968f..7686466d1109253b0d75edeb5f6a99fb98ce4cc6 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -164,6 +164,24 @@ enum {
> > */
> > HCI_QUIRK_BDADDR_PROPERTY_BROKEN,
> >
> > + /* When this quirk is set, the public Bluetooth address
> > + * initially reported by HCI Read BD Address command
> > + * is considered invalid. The public BD Address can be
> > + * retrieved via a 'local-bd-address' NVMEM cell.
>
> Why do we need a quirk here? Can't we always assume that if there is an
> NVMEM cell, it contains a correct address, even if HCI command returned
> a seemingly-sensible one?
The pattern follows HCI_QUIRK_USE_BDADDR_PROPERTY, the quirk indicates
that the address returned by the HCI Read BD Address command is
invalid and should be overridden using a fwnode property. Without this
quirk, even a valid fwnode-provided address is ignored. So here this
is primarily done to align with that established behavior, although
whether that design choice is ideal is a good question.
This also raises the question of why an explicit HCI_QUIRK_USE_* flag
is required to allow reading from NVMEM when the controller-provided
address is known to be invalid, rather than attempting to use any
available backend (fwnode-prop or NVMEM). but this remains
consistent with the behavior established by the fwnode-based quirk.
So, I think these aspects could be revisited in a Bluetooth follow-up
series if there is interest in reworking the overall addr fallback
design.
Regards,
Loic
>
> > + *
> > + * This quirk can be set before hci_register_dev is called or
> > + * during the hdev->setup vendor callback.
> > + */
> > + HCI_QUIRK_USE_BDADDR_NVMEM,
> > +
> > + /* When this quirk is set, the Bluetooth Device Address provided by
> > + * the 'local-bd-address' NVMEM is stored in big-endian order.
> > + *
> > + * This quirk can be set before hci_register_dev is called or
> > + * during the hdev->setup vendor callback.
> > + */
> > + HCI_QUIRK_BDADDR_NVMEM_BE,
>
> Also, is this necessary? Are the devices which store the address in the
> wrong format in the NVMEM?
>
> > +
> > /* When this quirk is set, the duplicate filtering during
> > * scanning is based on Bluetooth devices addresses. To allow
> > * RSSI based updates, restart scanning if needed.
>
> --
> With best wishes
> Dmitry
^ permalink raw reply
* Re: [PATCH v3 1/2] cpufreq: spacemit: Add K1 cpufreq driver
From: sashiko-bot @ 2026-06-12 10:05 UTC (permalink / raw)
To: Shuwei Wu; +Cc: conor+dt, robh, devicetree
In-Reply-To: <20260612-shadow-deps-v3-1-2f3ba88611ff@mailbox.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unaligned initial cluster frequencies can cause cluster 1 to undervoltage and crash during the first cpufreq transition.
--
commit de194d59d1b015bd0b960104ac00570cfd3ba072
Author: Shuwei Wu <shuwei.wu@mailbox.org>
cpufreq: spacemit: Add K1 cpufreq driver
This commit adds a CPU frequency scaling driver for the SpacemiT K1 SoC.
It handles two cluster clocks and one shared CPU voltage rail within a single
cpufreq policy.
> diff --git a/drivers/cpufreq/spacemit-k1-cpufreq.c b/drivers/cpufreq/spacemit-k1-cpufreq.c
> new file mode 100644
> index 0000000000000..631897eb80225
> --- /dev/null
> +++ b/drivers/cpufreq/spacemit-k1-cpufreq.c
[ ... ]
> +static int k1_cpufreq_set_target(struct cpufreq_policy *policy,
> + unsigned int index)
> +{
> + struct k1_cpufreq_priv *priv = policy->driver_data;
> + unsigned long old_freq = policy->cur * 1000UL;
> + unsigned long new_freq = policy->freq_table[index].frequency * 1000UL;
> + int ret;
> +
> + if (!old_freq)
> + old_freq = clk_get_rate(priv->cluster0_clk);
> +
> + if (new_freq > old_freq) {
> + ret = dev_pm_opp_set_rate(priv->cpu_dev, new_freq);
[Severity: High]
Could this cause an undervoltage crash on cluster 1 during the first transition?
The transition logic assumes old_freq (which comes from cluster 0) represents
the current speed of both clusters. If the bootloader leaves cluster 1 running
at a higher frequency than cluster 0 (e.g., cluster 0 at 500 MHz, cluster 1 at
1.2 GHz), and new_freq is an intermediate value like 800 MHz:
new_freq (800) > old_freq (500), so dev_pm_opp_set_rate() sets the shared
regulator to the 800 MHz voltage level. Since cluster 1 is invisible to the
OPP and regulator core, its voltage drops to the 800 MHz level while it is
still physically running at 1.2 GHz.
[ ... ]
> +static int k1_cpufreq_probe(struct platform_device *pdev)
> +{
[ ... ]
> + priv->cluster0_clk = clk_get(priv->cpu_dev, NULL);
> + if (IS_ERR(priv->cluster0_clk)) {
> + ret = PTR_ERR(priv->cluster0_clk);
> + dev_err_probe(priv->cpu_dev, ret, "failed to get cluster0 clock\n");
> + goto free_cpumask;
> + }
> +
> + priv->cluster1_clk = clk_get(cpu4_dev, NULL);
> + if (IS_ERR(priv->cluster1_clk)) {
[Severity: High]
Should the driver synchronize both cluster clocks to a safe state (like the
maximum of their initial frequencies) here during probe before registering
the cpufreq driver?
The clocks are acquired here but their initial rates are never read or aligned,
leaving them vulnerable to the mismatch described above.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-shadow-deps-v3-0-2f3ba88611ff@mailbox.org?part=1
^ permalink raw reply
* Re: [PATCH 0/6] arm64: ti: Use syscon for the Control Module
From: Tomi Valkeinen @ 2026-06-12 10:06 UTC (permalink / raw)
To: Andrew Davis
Cc: linux-arm-kernel, devicetree, linux-kernel, Nishanth Menon,
Vignesh Raghavendra, Tero Kristo, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Abraham I, Roger Quadros,
Devarsh Thakkar, Swamil Jain
In-Reply-To: <29c9bd27-df32-4c56-8df2-987722d02b9a@ti.com>
Hi Andrew,
On 29/05/2026 01:59, Andrew Davis wrote:
> On 5/28/26 7:53 AM, Tomi Valkeinen wrote:
>> I have been trying to get BeagleY-AI display support to upstream:
>>
>> 20260513-beagley-ai-display-v2-0-9e9bcefde6bc@ideasonboard.com
>>
>> One difficulty has been the handling of the Control Module region, as
>> we need access to a single in that region, surrounded by registers for
>> other subsystems. In my series I made the related node a syscon, thus
>> allowing versatile access to the registers:
>>
>> https://lore.kernel.org/all/20260513-beagley-ai-display-
>> v2-14-9e9bcefde6bc@ideasonboard.com/
>>
>> However, that's not a correct way to handle it. I realized we already
>> have ti,j721e-system-controller.yaml binding for older SoCs, which has
>> syscon but it's not used for the newer TI SoCs. This series takes the
>> same binding into use for the newer SoCs.
>>
>
> We moved away from this system-controller thing because it was always
> a hack to allow us to poke into random control registers from nodes
> throughout the DT. This was a mess and also caused issues with multiple
> mappings to the same registers (some sub nodes inside the control space
> also make their own mappings). If you need access to registers then make
> a node with those registers in the `reg` property.
Like in this patch (dss_dpi0_clk_ctrl)?
https://lore.kernel.org/all/20260420-beagley-ai-display-v1-14-f628543dfd14%40ideasonboard.com/
My understanding is that that's NACKed, as we end up adding new syscon
compats and syscon nodes for (possibly many) single register blocks.
Tomi
^ permalink raw reply
* Re: [PATCH v4 11/16] media: iris: update buffer requirements based on received info
From: sashiko-bot @ 2026-06-12 10:08 UTC (permalink / raw)
To: Dmitry Baryshkov; +Cc: robh, devicetree, conor+dt
In-Reply-To: <20260612-iris-ar50lt-v4-11-0abfb74d5b3c@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] The driver incorrectly assigns `req->hold_count` and `req->count_actual` from `struct hfi_buffer_requirements` instead of `req->count_min` to instance buffer counts, leading to insufficient buffer allocations.
Pre-existing issues:
- [High] Preexisting Use-After-Free (UAF) race condition due to missing reference counting in `iris_get_instance`.
- [High] Preexisting Use-After-Free (UAF) of `inst->m2m_ctx` during session teardown in `iris_close()`.
--
commit 07ef5efeab4196fd6eaca945bfba49a95c55f702
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
media: iris: update buffer requirements based on received info
Upon receiving data for HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS the
driver should update buffer sizes and counts from the received data.
Implement corresponding functionality updating buffers data. This will
be used for upcoming support of AR50Lt platforms with Gen1 firmware.
> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
> index 23fc7194b1e3a..ee996eb1f41fa 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
[ ... ]
> +static void iris_hfi_gen1_session_buffer_requirements(struct iris_inst *inst,
> + void *data, size_t size)
> +{
> + struct hfi_buffer_requirements *req;
> +
> + if (!size || size % sizeof(*req))
> + return;
> +
> + for (req = data; size; size -= sizeof(*req), req++) {
> + enum iris_buffer_type type = iris_hfi_gen1_buf_type(inst, req->type);
> +
> + if (type == -EINVAL)
> + continue;
> +
> + inst->buffers[type].min_count = req->hold_count;
[Severity: High]
Is this assignment correct? The existing firmware semantics appear to expect
req->count_min to represent the minimum required buffers for forward progress.
Will assigning req->hold_count (which is typically lower) cause the driver to
under-allocate buffers for V4L2, leading to firmware stalls?
> + inst->buffers[type].size = req->size;
> +
> + if (type == BUF_OUTPUT)
> + inst->fw_min_count = req->count_actual;
[Severity: High]
Similarly, should this be assigned to req->count_min instead of
req->count_actual to maintain consistent minimum buffer count derivations?
> + }
> +}
[ ... ]
> @@ -657,7 +729,7 @@ static void iris_hfi_gen1_handle_response(struct iris_core *core, void *response
> } else if (hdr->pkt_type == HFI_MSG_SESSION_EMPTY_BUFFER) {
> iris_hfi_gen1_session_etb_done(inst, hdr);
> } else if (hdr->pkt_type == HFI_MSG_SESSION_FILL_BUFFER) {
> iris_hfi_gen1_session_ftb_done(inst, hdr);
[Severity: High]
This isn't a bug introduced by this patch, but can this lead to a
use-after-free regression?
During session teardown, iris_close() releases the M2M context before
shutting down the firmware session:
iris_vidc.c:iris_close() {
...
v4l2_m2m_ctx_release(inst->m2m_ctx);
...
}
If an HFI_MSG_SESSION_FILL_BUFFER event arrives before the session is fully
closed, the threaded IRQ handler calls iris_hfi_gen1_session_ftb_done() which
subsequently accesses the freed context in v4l2_m2m_for_each_dst_buf_safe().
Is there a way to defer freeing the M2M context until the session is safely
stopped?
> } else if (hdr->pkt_type == HFI_MSG_SESSION_PROPERTY_INFO) {
> - complete(&inst->completion);
> + iris_hfi_gen1_session_property_info(inst, hdr);
[Severity: High]
This is a pre-existing issue, but is it safe for
iris_hfi_gen1_handle_response() to use the inst pointer here without a
reference count?
When iris_get_instance() looks up the instance, it returns the pointer without
taking a reference. If a userspace thread executing iris_close() frees the
instance:
iris_vidc.c:iris_close() {
...
kfree(inst);
}
Could the threaded IRQ handler concurrently attempt to lock the destroyed
inst->lock or dereference inst while handling responses, resulting in a
use-after-free?
> } else {
> struct hfi_msg_session_hdr_pkt *shdr;
>
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-iris-ar50lt-v4-0-0abfb74d5b3c@oss.qualcomm.com?part=11
^ permalink raw reply
* Re: [PATCH net-next v6 01/12] net: phylink: keep and use MAC supported_interfaces in phylink struct
From: Christian Marangi @ 2026-06-12 10:10 UTC (permalink / raw)
To: sashiko-reviews; +Cc: devicetree, conor+dt, robh
In-Reply-To: <20260610161620.99C121F00893@smtp.kernel.org>
On Wed, Jun 10, 2026 at 04:16:20PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Late modification to `config->supported_interfaces` by drivers is silently ignored due to by-value copying in `phylink_create()`, breaking support for devices like MTK GMAC2.
> --
>
> commit eaea53b185dbdc4c75406c542c97543c4c451c9d
> Author: Christian Marangi <ansuelsmth@gmail.com>
>
> net: phylink: keep and use MAC supported_interfaces in phylink struct
>
> This patch copies supported_interfaces from phylink_config into the phylink
> structure to prepare for internal PCS handling. The internal copy is then
> used in place of the config structure's bitmap.
>
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index 087ac63f9193d..4d59c0dd78dbf 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> [ ... ]
> > @@ -1868,6 +1873,9 @@ struct phylink *phylink_create(struct phylink_config *config,
> > mutex_init(&pl->state_mutex);
> > INIT_WORK(&pl->resolve, phylink_resolve);
> >
> > + phy_interface_copy(pl->supported_interfaces,
> > + config->supported_interfaces);
> > +
>
> [Severity: High]
> Will this by-value copy break drivers that modify supported_interfaces after
> calling phylink_create()?
>
> For example, looking at the mtk_eth_soc driver in mtk_add_mac(), the
> supported_interfaces bitmap is modified after phylink_create() returns:
>
> drivers/net/ethernet/mediatek/mtk_eth_soc.c:mtk_add_mac() {
> ...
> mac->phylink = phylink_create(&mac->phylink_config, ...);
> ...
> if (MTK_HAS_CAPS(mac->hw->soc->caps, MTK_2P5GPHY) &&
> id == MTK_GMAC2_ID)
> __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> mac->phylink_config.supported_interfaces);
> ...
> }
>
> Since phylink_create() has already taken a snapshot of the interfaces,
> this late addition of PHY_INTERFACE_MODE_INTERNAL will be dropped.
> Could this cause phylink_validate() to incorrectly reject the internal PHY
> capability for the MTK GMAC2?
>
> > pl->config = config;
> > if (config->type == PHYLINK_NETDEV) {
> > pl->netdev = to_net_dev(config->dev);
>
I was with the idea that value of phylink_config should be used only for
phylink_create and then in theory they should not be changed.
Isn't this a bug in MTK code to modify the phylink_config AFTER
phylink_create?
It was never a problem since it's it's INTERNAL interface mode and a PCS is
not needed for it but still I feel this should be fixed and this change
doesn't introduce some kind of regression.
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260609151212.29469-1-ansuelsmth@gmail.com?part=1
--
Ansuel
^ permalink raw reply
* Re: [PATCH v3 2/3] media: i2c: add os02g10 image sensor driver
From: Vladimir Zapolskiy @ 2026-06-12 10:12 UTC (permalink / raw)
To: Elgin Perumbilly, sakari.ailus, laurent.pinchart
Cc: Tarang Raval, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Hans Verkuil, Hans de Goede,
Mehdi Djait, Sylvain Petinot, Benjamin Mugnier,
Bryan O'Donoghue, Himanshu Bhavani, Heimir Thor Sverrisson,
Jingjing Xiong, Svyatoslav Ryhel, linux-media, devicetree,
linux-kernel
In-Reply-To: <20260424092554.26130-3-elgin.perumbilly@siliconsignals.io>
On 4/24/26 12:25, Elgin Perumbilly wrote:
> Add a v4l2 subdevice driver for the Omnivision os02g10 sensor.
>
> The Omnivision os02g10 is a CMOS image sensor with an active array size of
> 1920 x 1080.
>
> The following features are supported:
> - Manual exposure an gain control support
> - vblank/hblank control support
> - vflip/hflip control support
> - Test pattern control support
> - Supported resolution: 1920 x 1080 @ 30fps (SBGGR10)
>
> Signed-off-by: Elgin Perumbilly <elgin.perumbilly@siliconsignals.io>
> Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io>
> ---
> MAINTAINERS | 1 +
> drivers/media/i2c/Kconfig | 10 +
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/os02g10.c | 949 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 961 insertions(+)
> create mode 100644 drivers/media/i2c/os02g10.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8a0a55073c30..693e71b51926 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19449,6 +19449,7 @@ M: Elgin Perumbilly <elgin.perumbilly@siliconsignals.io>
> L: linux-media@vger.kernel.org
> S: Maintained
> F: Documentation/devicetree/bindings/media/i2c/ovti,os02g10.yaml
> +F: drivers/media/i2c/os02g10.c
>
> OMNIVISION OS05B10 SENSOR DRIVER
> M: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 5eb1e0e0a87a..dd6e9562acf6 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -372,6 +372,16 @@ config VIDEO_OG0VE1B
> To compile this driver as a module, choose M here: the
> module will be called og0ve1b.
>
> +config VIDEO_OS02G10
> + tristate "OmniVision OS02G10 sensor support"
> + select V4L2_CCI_I2C
> + help
> + This is a Video4Linux2 sensor driver for Omnivision
> + OS02G10 camera sensor.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called os02g10.
> +
> config VIDEO_OS05B10
> tristate "OmniVision OS05B10 sensor support"
> select V4L2_CCI_I2C
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index a3a6396df3c4..a7554d2eb140 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -84,6 +84,7 @@ obj-$(CONFIG_VIDEO_MT9V032) += mt9v032.o
> obj-$(CONFIG_VIDEO_MT9V111) += mt9v111.o
> obj-$(CONFIG_VIDEO_OG01A1B) += og01a1b.o
> obj-$(CONFIG_VIDEO_OG0VE1B) += og0ve1b.o
> +obj-$(CONFIG_VIDEO_OS02G10) += os02g10.o
> obj-$(CONFIG_VIDEO_OS05B10) += os05b10.o
> obj-$(CONFIG_VIDEO_OV01A10) += ov01a10.o
> obj-$(CONFIG_VIDEO_OV02A10) += ov02a10.o
> diff --git a/drivers/media/i2c/os02g10.c b/drivers/media/i2c/os02g10.c
> new file mode 100644
> index 000000000000..fad2dd0ad7aa
> --- /dev/null
> +++ b/drivers/media/i2c/os02g10.c
> @@ -0,0 +1,949 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * V4L2 Support for the OS02G10
> + *
> + * Copyright (C) 2026 Silicon Signals Pvt. Ltd.
> + *
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bitops.h>
> +#include <linux/cleanup.h>
> +#include <linux/clk.h>
> +#include <linux/container_of.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/units.h>
> +#include <linux/types.h>
> +#include <linux/time.h>
> +#include <linux/regmap.h>
Please sort the list of includes in alphabetical order, also you
may consider to shrink the list by removing quite many inherited
includes.
> +
> +#include <media/v4l2-cci.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-mediabus.h>
> +
> +#define OS02G10_XCLK_FREQ (24 * HZ_PER_MHZ)
Please put this "24MHZ" information into the macro name, like it to be
OS02G10_XCLK_FREQ_24MHZ or so.
> +
> +/* Page 0 */
> +#define OS02G10_REG_CHIPID CCI_REG24(0x002)
> +#define OS02G10_CHIPID 0x560247
> +
> +#define OS02G10_REG_PLL_DIV_CTRL CCI_REG8(0x030)
> +#define OS02G10_REG_PLL_DCTL_BIAS_CTRL CCI_REG8(0x035)
> +#define OS02G10_REG_GATE_EN_CTRL CCI_REG8(0x038)
> +#define OS02G10_REG_DPLL_NC CCI_REG8(0x041)
> +#define OS02G10_REG_MP_PHASE_CTRL CCI_REG8(0x044)
> +
> +/* Page 1 */
> +#define OS02G10_REG_STREAM_CTRL CCI_REG8(0x1b1)
> +#define OS02G10_STREAM_CTRL_ON 0x03
> +#define OS02G10_STREAM_CTRL_OFF 0x00
> +
> +#define OS02G10_REG_FRAME_SYNC CCI_REG8(0x101)
> +
> +#define OS02G10_REG_FRAME_LENGTH CCI_REG16(0x10e)
> +#define OS02G10_FRAME_LENGTH_MAX 0xffff
> +#define OS02G10_REG_HBLANK CCI_REG16(0x109)
> +
> +#define OS02G10_REG_FRAME_TEST_CTRL CCI_REG8(0x10d)
> +#define OS02G10_FRAME_EXP_SEPERATE_EN BIT(4)
> +#define OS02G10_TEST_PATTERN_ENABLE BIT(0)
> +
> +#define OS02G10_REG_ULP_PWD_DUMMY_CTRL CCI_REG8(0x13c)
> +#define OS02G10_REG_DC_LEVEL_LIMIT_EN CCI_REG8(0x146)
> +#define OS02G10_REG_DC_LEVEL_LIMIT_L CCI_REG8(0x147)
> +#define OS02G10_REG_BLC_DATA_LIMIT_L CCI_REG8(0x148)
> +#define OS02G10_REG_DC_BLC_LIMIT_H CCI_REG8(0x149)
> +
> +#define OS02G10_REG_HS_LP_CTRL CCI_REG8(0x192)
> +#define OS02G10_REG_HS_LEVEL CCI_REG8(0x19d)
> +#define OS02G10_REG_HS_DRV CCI_REG8(0x19e)
> +
> +#define OS02G10_REG_GB_SUBOFFSET CCI_REG8(0x1f0)
> +#define OS02G10_REG_BLUE_SUBOFFSET CCI_REG8(0x1f1)
> +#define OS02G10_REG_RED_SUBOFFSET CCI_REG8(0x1f2)
> +#define OS02G10_REG_GR_SUBOFFSET CCI_REG8(0x1f3)
> +
> +#define OS02G10_REG_ABL_TRIGGER CCI_REG8(0x1fa)
> +#define OS02G10_REG_ABL CCI_REG8(0x1fb)
> +
> +#define OS02G10_REG_H_SIZE_MIPI CCI_REG16(0x18e)
> +#define OS02G10_REG_V_SIZE_MIPI CCI_REG16(0x190)
> +#define OS02G10_REG_MIPI_TX_SPEED_CTRL CCI_REG8(0x1a1)
> +
> +#define OS02G10_REG_LONG_EXPOSURE CCI_REG16(0x103)
> +#define OS02G10_EXPOSURE_MIN 4
> +#define OS02G10_EXPOSURE_STEP 1
> +#define OS02G10_EXPOSURE_MARGIN 9
> +
> +#define OS02G10_REG_ANALOG_GAIN CCI_REG8(0x124)
> +#define OS02G10_ANALOG_GAIN_MIN 0x10
> +#define OS02G10_ANALOG_GAIN_MAX 0xf8
> +#define OS02G10_ANALOG_GAIN_STEP 1
> +#define OS02G10_ANALOG_GAIN_DEFAULT 0x10
> +
> +#define OS02G10_REG_DIGITAL_GAIN_H CCI_REG8(0x137)
> +#define OS02G10_REG_DIGITAL_GAIN_L CCI_REG8(0x139)
> +#define OS02G10_DIGITAL_GAIN_MIN 0x40
> +#define OS02G10_DIGITAL_GAIN_MAX 0x800
> +#define OS02G10_DIGITAL_GAIN_STEP 64
> +#define OS02G10_DIGITAL_GAIN_DEFAULT 0x40
> +
> +#define OS02G10_REG_FLIP_MIRROR CCI_REG8(0x13f)
> +#define OS02G10_FLIP BIT(1)
> +#define OS02G10_MIRROR BIT(0)
Please sort the registers by address value, then it will make clear
which position to insert any new added macro definitions.
> +
> +/* Page 2 */
> +#define OS02G10_REG_V_START CCI_REG16(0x2a0)
> +#define OS02G10_REG_V_SIZE CCI_REG16(0x2a2)
> +#define OS02G10_REG_H_START CCI_REG16(0x2a4)
> +#define OS02G10_REG_H_SIZE CCI_REG16(0x2a6)
> +
> +#define OS02G10_REG_SIF_CTRL CCI_REG8(0x25e)
> +#define OS02G10_ORIENTATION_BAYER_FIX 0x32
The same applies here.
> +
> +#define OS02G10_LINK_FREQ_720MHZ (720 * HZ_PER_MHZ)
> +
> +/* OS02G10 native and active pixel array size */
> +static const struct v4l2_rect os02g10_native_area = {
> + .top = 0,
> + .left = 0,
> + .width = 1928,
> + .height = 1088,
> +};
> +
> +static const struct v4l2_rect os02g10_active_area = {
> + .top = 4,
> + .left = 4,
> + .width = 1920,
> + .height = 1080,
> +};
> +
> +static const char * const os02g10_supply_name[] = {
> + "avdd", /* Analog power */
> + "dovdd", /* Digital I/O power */
> + "dvdd", /* Digital core power */
> +};
> +
> +struct os02g10 {
> + struct device *dev;
> + struct regmap *cci;
> + struct v4l2_subdev sd;
> + struct media_pad pad;
> + struct clk *xclk;
> + struct gpio_desc *reset_gpio;
> + struct regulator_bulk_data supplies[ARRAY_SIZE(os02g10_supply_name)];
> +
> + /* V4L2 Controls */
> + struct v4l2_ctrl_handler handler;
> + struct v4l2_ctrl *link_freq;
> + struct v4l2_ctrl *vblank;
> + struct v4l2_ctrl *exposure;
> + struct v4l2_ctrl *vflip;
> + struct v4l2_ctrl *hflip;
> +};
> +
> +struct os02g10_mode {
> + u32 width;
> + u32 height;
> + u32 vts_def;
> + u32 exp_def;
> + u32 x_start;
> + u32 y_start;
> +};
> +
> +static const struct cci_reg_sequence os02g10_common_regs[] = {
> + { OS02G10_REG_PLL_DIV_CTRL, 0x0a},
> + { OS02G10_REG_PLL_DCTL_BIAS_CTRL, 0x04},
> + { OS02G10_REG_GATE_EN_CTRL, 0x11},
> + { OS02G10_REG_DPLL_NC, 0x06},
> + { OS02G10_REG_MP_PHASE_CTRL, 0x20},
> + { CCI_REG8(0x119), 0x50},
> + { CCI_REG8(0x11a), 0x0c},
> + { CCI_REG8(0x11b), 0x0d},
> + { CCI_REG8(0x11c), 0x00},
> + { CCI_REG8(0x11d), 0x75},
> + { CCI_REG8(0x11e), 0x52},
> + { CCI_REG8(0x122), 0x14},
> + { CCI_REG8(0x125), 0x44},
> + { CCI_REG8(0x126), 0x0f},
> + { OS02G10_REG_ULP_PWD_DUMMY_CTRL, 0xca},
> + { CCI_REG8(0x13d), 0x4a},
> + { CCI_REG8(0x140), 0x0f},
> + { CCI_REG8(0x143), 0x38},
> + { OS02G10_REG_DC_LEVEL_LIMIT_EN, 0x01},
> + { OS02G10_REG_DC_LEVEL_LIMIT_L, 0x00},
> + { OS02G10_REG_DC_BLC_LIMIT_H, 0x32},
> + { CCI_REG8(0x150), 0x01},
> + { CCI_REG8(0x151), 0x28},
> + { CCI_REG8(0x152), 0x20},
> + { CCI_REG8(0x153), 0x03},
> + { CCI_REG8(0x157), 0x16},
> + { CCI_REG8(0x159), 0x01},
> + { CCI_REG8(0x15a), 0x01},
> + { CCI_REG8(0x15d), 0x04},
> + { CCI_REG8(0x16a), 0x04},
> + { CCI_REG8(0x16b), 0x03},
> + { CCI_REG8(0x16e), 0x28},
> + { CCI_REG8(0x171), 0xc2},
> + { CCI_REG8(0x172), 0x04},
> + { CCI_REG8(0x173), 0x38},
> + { CCI_REG8(0x174), 0x04},
> + { CCI_REG8(0x179), 0x00},
> + { CCI_REG8(0x17a), 0xb2},
> + { CCI_REG8(0x17b), 0x10},
> + { OS02G10_REG_HS_LP_CTRL, 0x02},
> + { OS02G10_REG_HS_LEVEL, 0x03},
> + { OS02G10_REG_HS_DRV, 0x55},
> + { CCI_REG8(0x1b8), 0x70},
> + { CCI_REG8(0x1b9), 0x70},
> + { CCI_REG8(0x1ba), 0x70},
> + { CCI_REG8(0x1bb), 0x70},
> + { CCI_REG8(0x1bc), 0x00},
> + { CCI_REG8(0x1c4), 0x6d},
> + { CCI_REG8(0x1c5), 0x6d},
> + { CCI_REG8(0x1c6), 0x6d},
> + { CCI_REG8(0x1c7), 0x6d},
> + { CCI_REG8(0x1cc), 0x11},
> + { CCI_REG8(0x1cd), 0xe0},
> + { CCI_REG8(0x1d0), 0x1b},
> + { CCI_REG8(0x1d2), 0x76},
> + { CCI_REG8(0x1d3), 0x68},
> + { CCI_REG8(0x1d4), 0x68},
> + { CCI_REG8(0x1d5), 0x73},
> + { CCI_REG8(0x1d6), 0x73},
> + { CCI_REG8(0x1e8), 0x55},
> + { OS02G10_REG_GB_SUBOFFSET, 0x40},
> + { OS02G10_REG_BLUE_SUBOFFSET, 0x40},
> + { OS02G10_REG_RED_SUBOFFSET, 0x40},
> + { OS02G10_REG_GR_SUBOFFSET, 0x40},
> + { OS02G10_REG_ABL_TRIGGER, 0x1c},
> + { OS02G10_REG_ABL, 0x33},
> + { CCI_REG8(0x1fc), 0x80},
> + { CCI_REG8(0x1fe), 0x80},
> + { CCI_REG8(0x303), 0x67},
> + { CCI_REG8(0x300), 0x59},
> + { CCI_REG8(0x304), 0x11},
> + { CCI_REG8(0x305), 0x04},
> + { CCI_REG8(0x306), 0x0c},
> + { CCI_REG8(0x307), 0x08},
> + { CCI_REG8(0x308), 0x08},
> + { CCI_REG8(0x309), 0x4f},
> + { CCI_REG8(0x30b), 0x08},
> + { CCI_REG8(0x30d), 0x26},
> + { CCI_REG8(0x30f), 0x00},
> + { CCI_REG8(0x234), 0xfe},
> + { OS02G10_REG_MIPI_TX_SPEED_CTRL, 0x05},
> +};
> +
> +static const struct os02g10_mode supported_modes[] = {
> + {
> + .width = 1920,
> + .height = 1080,
> + .vts_def = 1246,
> + .exp_def = 1100,
> + .x_start = 2,
> + .y_start = 6,
> + },
> +};
> +
> +static const s64 link_freq_menu_items[] = {
> + OS02G10_LINK_FREQ_720MHZ,
> +};
> +
> +static const char * const os02g10_test_pattern_menu[] = {
> + "Disabled",
> + "Colorbar",
> +};
> +
> +static inline struct os02g10 *to_os02g10(struct v4l2_subdev *sd)
> +{
> + return container_of_const(sd, struct os02g10, sd);
> +}
> +
> +static u32 os02g10_get_format_code(struct os02g10 *os02g10)
> +{
> + static const u32 codes[2][2] = {
> + { MEDIA_BUS_FMT_SBGGR10_1X10, MEDIA_BUS_FMT_SGBRG10_1X10, },
> + { MEDIA_BUS_FMT_SGRBG10_1X10, MEDIA_BUS_FMT_SRGGB10_1X10, },
> + };
> + u32 code = codes[os02g10->vflip->val][os02g10->hflip->val];
> +
> + return code;
> +}
> +
> +static int os02g10_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct os02g10 *os02g10 = container_of_const(ctrl->handler,
> + struct os02g10, handler);
> + struct v4l2_subdev_state *state;
> + struct v4l2_mbus_framefmt *fmt;
> + int ret = 0;
> +
> + state = v4l2_subdev_get_locked_active_state(&os02g10->sd);
> + fmt = v4l2_subdev_state_get_format(state, 0);
> +
> + if (ctrl->id == V4L2_CID_VBLANK) {
> + /* Honour the VBLANK limits when setting exposure */
> + s64 max = fmt->height + ctrl->val - OS02G10_EXPOSURE_MARGIN;
> +
> + ret = __v4l2_ctrl_modify_range(os02g10->exposure,
> + os02g10->exposure->minimum, max,
> + os02g10->exposure->step,
> + os02g10->exposure->default_value);
> + if (ret)
> + return ret;
> + }
> +
> + if (pm_runtime_get_if_active(os02g10->dev) == 0)
> + return 0;
> +
> + switch (ctrl->id) {
> + case V4L2_CID_EXPOSURE:
> + cci_write(os02g10->cci, OS02G10_REG_LONG_EXPOSURE,
> + ctrl->val, &ret);
> + break;
> + case V4L2_CID_ANALOGUE_GAIN:
> + cci_write(os02g10->cci, OS02G10_REG_ANALOG_GAIN,
> + ctrl->val, &ret);
> + break;
> + case V4L2_CID_DIGITAL_GAIN:
> + cci_write(os02g10->cci, OS02G10_REG_DIGITAL_GAIN_L,
> + (ctrl->val & 0xff), &ret);
> + cci_write(os02g10->cci, OS02G10_REG_DIGITAL_GAIN_H,
> + ((ctrl->val >> 8) & 0x7), &ret);
> + break;
> + case V4L2_CID_VBLANK:
> + u64 vts = ctrl->val + fmt->height;
> +
> + cci_update_bits(os02g10->cci, OS02G10_REG_FRAME_TEST_CTRL,
> + OS02G10_FRAME_EXP_SEPERATE_EN,
> + OS02G10_FRAME_EXP_SEPERATE_EN, &ret);
> + cci_write(os02g10->cci, OS02G10_REG_FRAME_LENGTH, vts, &ret);
> + break;
> + case V4L2_CID_HFLIP:
> + case V4L2_CID_VFLIP:
> + cci_write(os02g10->cci, OS02G10_REG_FLIP_MIRROR,
> + os02g10->hflip->val | os02g10->vflip->val << 1,
> + &ret);
> + cci_write(os02g10->cci, OS02G10_REG_SIF_CTRL,
> + OS02G10_ORIENTATION_BAYER_FIX, &ret);
> + break;
> + case V4L2_CID_TEST_PATTERN:
> + cci_update_bits(os02g10->cci,
> + OS02G10_REG_FRAME_TEST_CTRL,
> + OS02G10_TEST_PATTERN_ENABLE,
> + ctrl->val ? OS02G10_TEST_PATTERN_ENABLE : 0,
> + &ret);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + cci_write(os02g10->cci, OS02G10_REG_FRAME_SYNC, 0x01, &ret);
> +
> + pm_runtime_put(os02g10->dev);
> +
> + return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops os02g10_ctrl_ops = {
> + .s_ctrl = os02g10_set_ctrl,
> +};
> +
> +static int os02g10_init_controls(struct os02g10 *os02g10)
> +{
> + const struct os02g10_mode *mode = &supported_modes[0];
> + struct v4l2_fwnode_device_properties props;
> + u64 vblank_def, exp_max, pixel_rate;
> + struct v4l2_ctrl_handler *ctrl_hdlr;
> + int ret;
> +
> + ctrl_hdlr = &os02g10->handler;
> + v4l2_ctrl_handler_init(ctrl_hdlr, 11);
> +
> + /* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> + pixel_rate = div_u64(OS02G10_LINK_FREQ_720MHZ * 2 * 2, 10);
Please define a macro representing the fixed number of data lanes,
it'll be used in os02g10_parse_endpoint() also.
> + v4l2_ctrl_new_std(ctrl_hdlr, &os02g10_ctrl_ops, V4L2_CID_PIXEL_RATE, 0,
> + pixel_rate, 1, pixel_rate);
> +
> + os02g10->link_freq =
> + v4l2_ctrl_new_int_menu(ctrl_hdlr, &os02g10_ctrl_ops,
> + V4L2_CID_LINK_FREQ,
> + ARRAY_SIZE(link_freq_menu_items) - 1,
> + 0, link_freq_menu_items);
> + if (os02g10->link_freq)
> + os02g10->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> + vblank_def = mode->vts_def - mode->height;
> + os02g10->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &os02g10_ctrl_ops,
> + V4L2_CID_VBLANK, vblank_def,
> + OS02G10_FRAME_LENGTH_MAX - mode->height,
> + 1, vblank_def);
> +
> + exp_max = mode->vts_def - OS02G10_EXPOSURE_MARGIN;
> + os02g10->exposure =
> + v4l2_ctrl_new_std(ctrl_hdlr, &os02g10_ctrl_ops,
> + V4L2_CID_EXPOSURE,
> + OS02G10_EXPOSURE_MIN, exp_max,
> + OS02G10_EXPOSURE_STEP, mode->exp_def);
> +
> + v4l2_ctrl_new_std(ctrl_hdlr, &os02g10_ctrl_ops,
> + V4L2_CID_ANALOGUE_GAIN, OS02G10_ANALOG_GAIN_MIN,
> + OS02G10_ANALOG_GAIN_MAX, OS02G10_ANALOG_GAIN_STEP,
> + OS02G10_ANALOG_GAIN_DEFAULT);
> +
> + v4l2_ctrl_new_std(ctrl_hdlr, &os02g10_ctrl_ops,
> + V4L2_CID_DIGITAL_GAIN, OS02G10_DIGITAL_GAIN_MIN,
> + OS02G10_DIGITAL_GAIN_MAX, OS02G10_DIGITAL_GAIN_STEP,
> + OS02G10_DIGITAL_GAIN_DEFAULT);
> +
> + os02g10->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &os02g10_ctrl_ops,
> + V4L2_CID_HFLIP, 0, 1, 1, 0);
> + if (os02g10->hflip)
> + os02g10->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> + os02g10->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &os02g10_ctrl_ops,
> + V4L2_CID_VFLIP, 0, 1, 1, 0);
> + if (os02g10->vflip)
> + os02g10->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> +
> + v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &os02g10_ctrl_ops,
> + V4L2_CID_TEST_PATTERN,
> + ARRAY_SIZE(os02g10_test_pattern_menu) - 1,
> + 0, 0, os02g10_test_pattern_menu);
> +
> + ret = v4l2_fwnode_device_parse(os02g10->dev, &props);
> + if (ret)
> + goto err_handler_free;
> +
> + ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr,
> + &os02g10_ctrl_ops, &props);
> + if (ret)
> + goto err_handler_free;
> +
> + os02g10->sd.ctrl_handler = ctrl_hdlr;
> +
> + return 0;
> +
> +err_handler_free:
> + v4l2_ctrl_handler_free(ctrl_hdlr);
> +
> + return ret;
> +}
> +
> +static int os02g10_set_framefmt(struct os02g10 *os02g10,
> + struct v4l2_subdev_state *state)
> +{
> + const struct v4l2_mbus_framefmt *format;
> + const struct os02g10_mode *mode;
> + int ret = 0;
> +
> + format = v4l2_subdev_state_get_format(state, 0);
> + mode = v4l2_find_nearest_size(supported_modes,
> + ARRAY_SIZE(supported_modes), width,
> + height, format->width, format->height);
> +
> + cci_write(os02g10->cci, OS02G10_REG_V_START, mode->y_start, &ret);
> + cci_write(os02g10->cci, OS02G10_REG_V_SIZE, mode->height, &ret);
> + cci_write(os02g10->cci, OS02G10_REG_V_SIZE_MIPI, mode->height, &ret);
> + cci_write(os02g10->cci, OS02G10_REG_H_START, mode->x_start, &ret);
> + cci_write(os02g10->cci, OS02G10_REG_H_SIZE, mode->width, &ret);
> + cci_write(os02g10->cci, OS02G10_REG_H_SIZE_MIPI, mode->width, &ret);
> +
> + return ret;
Just "return 0" here, and remove the local variable.
> +}
> +
> +static int os02g10_enable_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state, u32 pad,
> + u64 streams_mask)
> +{
> + struct os02g10 *os02g10 = to_os02g10(sd);
> + int ret;
> +
> + ret = pm_runtime_resume_and_get(os02g10->dev);
> + if (ret < 0)
> + return ret;
> +
> + ret = cci_multi_reg_write(os02g10->cci, os02g10_common_regs,
> + ARRAY_SIZE(os02g10_common_regs), NULL);
> + if (ret) {
> + dev_err(os02g10->dev, "failed to write common registers\n");
> + goto err_rpm_put;
> + }
> +
> + ret = os02g10_set_framefmt(os02g10, state);
> + if (ret) {
> + dev_err(os02g10->dev, "failed to set frame foramt\n");
> + goto err_rpm_put;
> + }
> +
> + /* Apply customized values from user */
> + ret = __v4l2_ctrl_handler_setup(os02g10->sd.ctrl_handler);
> + if (ret)
> + goto err_rpm_put;
> +
> + ret = cci_write(os02g10->cci, OS02G10_REG_STREAM_CTRL,
> + OS02G10_STREAM_CTRL_ON, NULL);
> + if (ret)
> + goto err_rpm_put;
> +
> + /* vflip and hflip cannot change during streaming */
> + __v4l2_ctrl_grab(os02g10->vflip, true);
> + __v4l2_ctrl_grab(os02g10->hflip, true);
> +
> + return 0;
> +
> +err_rpm_put:
> + pm_runtime_put(os02g10->dev);
> + return ret;
> +}
> +
> +static int os02g10_disable_streams(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state, u32 pad,
> + u64 streams_mask)
> +{
> + struct os02g10 *os02g10 = to_os02g10(sd);
> + int ret;
> +
> + ret = cci_write(os02g10->cci, OS02G10_REG_STREAM_CTRL,
> + OS02G10_STREAM_CTRL_OFF, NULL);
> + if (ret)
> + dev_err(os02g10->dev, "Failed to stop stream\n");
> +
> + __v4l2_ctrl_grab(os02g10->vflip, false);
> + __v4l2_ctrl_grab(os02g10->hflip, false);
> +
> + pm_runtime_put(os02g10->dev);
> +
> + return ret;
> +}
> +
> +static int os02g10_get_selection(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_selection *sel)
> +{
> + switch (sel->target) {
> + case V4L2_SEL_TGT_CROP_BOUNDS:
> + case V4L2_SEL_TGT_NATIVE_SIZE:
> + sel->r = os02g10_native_area;
> + return 0;
> + case V4L2_SEL_TGT_CROP:
> + case V4L2_SEL_TGT_CROP_DEFAULT:
> + sel->r = os02g10_active_area;
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int os02g10_enum_mbus_code(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_mbus_code_enum *code)
> +{
> + struct os02g10 *os02g10 = to_os02g10(sd);
> +
> + if (code->index)
> + return -EINVAL;
> +
> + code->code = os02g10_get_format_code(os02g10);
> +
> + return 0;
> +}
> +
> +static int os02g10_enum_frame_size(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_frame_size_enum *fse)
> +{
> + struct os02g10 *os02g10 = to_os02g10(sd);
> +
> + if (fse->index >= ARRAY_SIZE(supported_modes))
> + return -EINVAL;
> +
> + if (fse->code != os02g10_get_format_code(os02g10))
> + return -EINVAL;
> +
> + fse->min_width = supported_modes[fse->index].width;
> + fse->max_width = fse->min_width;
> + fse->min_height = supported_modes[fse->index].height;
> + fse->max_height = fse->min_height;
> +
> + return 0;
> +}
> +
> +static int os02g10_set_pad_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_format *fmt)
> +{
> + struct os02g10 *os02g10 = to_os02g10(sd);
> + struct v4l2_mbus_framefmt *format;
> + const struct os02g10_mode *mode;
> +
> + format = v4l2_subdev_state_get_format(sd_state, 0);
> +
> + mode = v4l2_find_nearest_size(supported_modes,
> + ARRAY_SIZE(supported_modes),
> + width, height,
> + fmt->format.width, fmt->format.height);
> +
> + fmt->format.code = os02g10_get_format_code(os02g10);
> + fmt->format.width = mode->width;
> + fmt->format.height = mode->height;
> + fmt->format.field = V4L2_FIELD_NONE;
> + fmt->format.colorspace = V4L2_COLORSPACE_RAW;
> + fmt->format.quantization = V4L2_QUANTIZATION_FULL_RANGE;
> + fmt->format.xfer_func = V4L2_XFER_FUNC_NONE;
> +
> + *format = fmt->format;
> +
> + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> + u32 vblank_def = mode->vts_def - mode->height;
> +
> + int ret = __v4l2_ctrl_modify_range(os02g10->vblank, vblank_def,
> + OS02G10_FRAME_LENGTH_MAX -
> + mode->height, 1, vblank_def);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int os02g10_init_state(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *state)
> +{
> + struct os02g10 *os02g10 = to_os02g10(sd);
> + struct v4l2_subdev_format fmt = {
> + .which = V4L2_SUBDEV_FORMAT_TRY,
> + .format = {
> + .code = os02g10_get_format_code(os02g10),
I'm uncertain that the default zero values of vflip/hflip represent
the initial state of a sensor correctly, it requires runtime testing.
Anyway you may consider to drop it, its immediately rewritten by
os02g10_set_pad_format().
> + .width = supported_modes[0].width,
> + .height = supported_modes[0].height,
> + },
> + };
> +
> + os02g10_set_pad_format(sd, state, &fmt);
> +
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_video_ops os02g10_video_ops = {
> + .s_stream = v4l2_subdev_s_stream_helper,
> +};
> +
> +static const struct v4l2_subdev_pad_ops os02g10_pad_ops = {
> + .enum_mbus_code = os02g10_enum_mbus_code,
> + .get_fmt = v4l2_subdev_get_fmt,
> + .set_fmt = os02g10_set_pad_format,
> + .get_selection = os02g10_get_selection,
> + .enum_frame_size = os02g10_enum_frame_size,
Please group two enum callbacks on the list.
> + .enable_streams = os02g10_enable_streams,
> + .disable_streams = os02g10_disable_streams,
> +};
> +
> +static const struct v4l2_subdev_ops os02g10_subdev_ops = {
> + .video = &os02g10_video_ops,
> + .pad = &os02g10_pad_ops,
> +};
> +
> +static const struct v4l2_subdev_internal_ops os02g10_internal_ops = {
> + .init_state = os02g10_init_state,
> +};
> +
> +static int os02g10_power_on(struct device *dev)
> +{
> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct os02g10 *os02g10 = to_os02g10(sd);
> + int ret;
> +
> + ret = regulator_bulk_enable(ARRAY_SIZE(os02g10_supply_name),
> + os02g10->supplies);
> + if (ret) {
> + dev_err(os02g10->dev, "failed to enable regulators\n");
> + return ret;
> + }
> +
> + /* T4: delay from DOVDD stable to MCLK on */
> + fsleep(5 * USEC_PER_MSEC);
> +
> + ret = clk_prepare_enable(os02g10->xclk);
> + if (ret) {
> + dev_err(os02g10->dev, "failed to enable clock\n");
> + goto err_regulator_off;
> + }
> +
> + /* T3: delay from DVDD stable to sensor power up stable */
> + fsleep(5 * USEC_PER_MSEC);
> +
> + gpiod_set_value_cansleep(os02g10->reset_gpio, 0);
> +
> + /* T5: delay from sensor power up stable to SCCB initialization */
> + fsleep(5 * USEC_PER_MSEC);
> +
> + return 0;
> +
> +err_regulator_off:
> + regulator_bulk_disable(ARRAY_SIZE(os02g10_supply_name), os02g10->supplies);
> + return ret;
> +}
> +
> +static int os02g10_power_off(struct device *dev)
> +{
> + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> + struct os02g10 *os02g10 = to_os02g10(sd);
> +
> + clk_disable_unprepare(os02g10->xclk);
> + gpiod_set_value_cansleep(os02g10->reset_gpio, 1);
> + regulator_bulk_disable(ARRAY_SIZE(os02g10_supply_name), os02g10->supplies);
> +
> + return 0;
> +}
> +
> +static int os02g10_identify_module(struct os02g10 *os02g10)
> +{
> + u64 chip_id;
> + int ret;
> +
> + ret = cci_read(os02g10->cci, OS02G10_REG_CHIPID, &chip_id, NULL);
> + if (ret)
> + return dev_err_probe(os02g10->dev, ret,
> + "failed to read chip id %x\n",
> + OS02G10_CHIPID);
> +
> + if (chip_id != OS02G10_CHIPID)
> + return dev_err_probe(os02g10->dev, -EIO,
> + "chip id mismatch: %x!=%llx\n",
> + OS02G10_CHIPID, chip_id);
> +
> + return 0;
> +}
> +
> +static int os02g10_parse_endpoint(struct os02g10 *os02g10)
> +{
> + struct v4l2_fwnode_endpoint bus_cfg = {
> + .bus_type = V4L2_MBUS_CSI2_DPHY,
> + };
> + unsigned long link_freq_bitmap;
> + struct fwnode_handle *ep;
> + int ret;
> +
> + ep = fwnode_graph_get_next_endpoint(dev_fwnode(os02g10->dev), NULL);
> + ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> + fwnode_handle_put(ep);
> + if (ret)
> + return ret;
> +
> + if (bus_cfg.bus.mipi_csi2.num_data_lanes != 2) {
Macro for the number of data lanes is wanted.
> + ret = dev_err_probe(os02g10->dev, -EINVAL,
> + "only 2 data lanes are supported\n");
> + goto error_out;
> + }
> +
> + ret = v4l2_link_freq_to_bitmap(os02g10->dev, bus_cfg.link_frequencies,
> + bus_cfg.nr_of_link_frequencies,
> + link_freq_menu_items,
> + ARRAY_SIZE(link_freq_menu_items),
> + &link_freq_bitmap);
> + if (ret) {
> + ret = dev_err_probe(os02g10->dev, -EINVAL,
> + "only 720MHz frequency is available\n");
> + goto error_out;
> + }
> +
> +error_out:
> + v4l2_fwnode_endpoint_free(&bus_cfg);
> +
> + return ret;
> +};
> +
> +static const struct regmap_range_cfg os02g10_ranges[] = {
> + {
> + .range_min = 0x0000,
> + .range_max = 0x03ff,
> + .selector_reg = 0xfd,
> + .selector_mask = 0x03,
> + .selector_shift = 0,
> + .window_start = 0x00,
> + .window_len = 0x100,
> + },
> +};
> +
> +static const struct regmap_config os02g10_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .reg_format_endian = REGMAP_ENDIAN_BIG,
> + .max_register = 0x3ff,
> + .ranges = os02g10_ranges,
> + .num_ranges = ARRAY_SIZE(os02g10_ranges),
> + .disable_locking = true,
> +};
> +
> +static int os02g10_probe(struct i2c_client *client)
> +{
> + struct os02g10 *os02g10;
> + unsigned int xclk_freq;
> + int ret;
> +
> + os02g10 = devm_kzalloc(&client->dev, sizeof(*os02g10), GFP_KERNEL);
> + if (!os02g10)
> + return -ENOMEM;
> +
> + os02g10->dev = &client->dev;
> +
> + v4l2_i2c_subdev_init(&os02g10->sd, client, &os02g10_subdev_ops);
> + os02g10->sd.internal_ops = &os02g10_internal_ops;
> +
> + /*
> + * This is not using devm_cci_regmap_init_i2c(), because the driver
> + * makes use of regmap's pagination feature. The chosen settings are
> + * compatible with the CCI helpers.
> + */
> + os02g10->cci = devm_regmap_init_i2c(client, &os02g10_regmap_config);
> + if (IS_ERR(os02g10->cci))
> + return dev_err_probe(os02g10->dev, PTR_ERR(os02g10->cci),
> + "failed to initialize CCI\n");
> +
> + ret = os02g10_parse_endpoint(os02g10);
> + if (ret)
> + return dev_err_probe(os02g10->dev, ret,
> + "failed to parse endpoint configuration\n");
> +
> + /* Get system clock (xvclk) */
> + os02g10->xclk = devm_v4l2_sensor_clk_get(os02g10->dev, NULL);
> + if (IS_ERR(os02g10->xclk))
> + return dev_err_probe(os02g10->dev, PTR_ERR(os02g10->xclk),
> + "failed to get xclk\n");
> +
> + xclk_freq = clk_get_rate(os02g10->xclk);
> + if (xclk_freq != OS02G10_XCLK_FREQ)
> + return dev_err_probe(os02g10->dev, -EINVAL,
> + "xclk frequency not supported: %u Hz\n",
> + xclk_freq);
> +
> + for (unsigned int i = 0; i < ARRAY_SIZE(os02g10_supply_name); i++)
> + os02g10->supplies[i].supply = os02g10_supply_name[i];
> +
> + ret = devm_regulator_bulk_get(os02g10->dev,
> + ARRAY_SIZE(os02g10_supply_name),
> + os02g10->supplies);
> + if (ret)
> + return dev_err_probe(os02g10->dev, ret,
> + "failed to get regulators\n");
> +
> + os02g10->reset_gpio = devm_gpiod_get_optional(os02g10->dev,
> + "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(os02g10->reset_gpio))
> + return dev_err_probe(os02g10->dev, PTR_ERR(os02g10->reset_gpio),
> + "failed to get reset GPIO\n");
> +
> + ret = os02g10_power_on(os02g10->dev);
> + if (ret)
> + return ret;
> +
> + ret = os02g10_identify_module(os02g10);
> + if (ret)
> + goto error_power_off;
> +
> + ret = os02g10_init_controls(os02g10);
> + if (ret)
> + goto error_power_off;
> +
> + /* Initialize subdev */
> + os02g10->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + os02g10->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> + os02g10->pad.flags = MEDIA_PAD_FL_SOURCE;
> +
> + ret = media_entity_pads_init(&os02g10->sd.entity, 1, &os02g10->pad);
> + if (ret) {
> + dev_err_probe(os02g10->dev, ret, "failed to init entity pads\n");
> + goto error_handler_free;
> + }
> +
> + os02g10->sd.state_lock = os02g10->handler.lock;
> + ret = v4l2_subdev_init_finalize(&os02g10->sd);
> + if (ret) {
> + dev_err_probe(os02g10->dev, ret, "subdev init error\n");
> + goto error_media_entity;
> + }
> +
> + pm_runtime_set_active(os02g10->dev);
> + pm_runtime_enable(os02g10->dev);
> +
> + ret = v4l2_async_register_subdev_sensor(&os02g10->sd);
> + if (ret) {
> + dev_err_probe(os02g10->dev, ret,
> + "failed to register os02g10 sub-device\n");
> + goto error_subdev_cleanup;
> + }
> +
> + pm_runtime_idle(os02g10->dev);
> +
> + return 0;
> +
> +error_subdev_cleanup:
> + v4l2_subdev_cleanup(&os02g10->sd);
> + pm_runtime_disable(os02g10->dev);
> + pm_runtime_set_suspended(os02g10->dev);
> +
> +error_media_entity:
> + media_entity_cleanup(&os02g10->sd.entity);
> +
> +error_handler_free:
> + v4l2_ctrl_handler_free(os02g10->sd.ctrl_handler);
> +
> +error_power_off:
> + os02g10_power_off(os02g10->dev);
> +
> + return ret;
> +}
> +
> +static void os02g10_remove(struct i2c_client *client)
> +{
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct os02g10 *os02g10 = to_os02g10(sd);
> +
> + v4l2_async_unregister_subdev(sd);
> + v4l2_subdev_cleanup(&os02g10->sd);
> + media_entity_cleanup(&sd->entity);
> + v4l2_ctrl_handler_free(os02g10->sd.ctrl_handler);
> +
> + pm_runtime_disable(&client->dev);
> + if (!pm_runtime_status_suspended(&client->dev)) {
> + os02g10_power_off(&client->dev);
> + pm_runtime_set_suspended(&client->dev);
> + }
> +}
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(os02g10_pm_ops,
> + os02g10_power_off, os02g10_power_on, NULL);
> +
> +static const struct of_device_id os02g10_id[] = {
> + { .compatible = "ovti,os02g10" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, os02g10_id);
> +
> +static struct i2c_driver os02g10_driver = {
> + .driver = {
> + .name = "os02g10",
> + .pm = pm_ptr(&os02g10_pm_ops),
> + .of_match_table = os02g10_id,
> + },
> + .probe = os02g10_probe,
> + .remove = os02g10_remove,
> +};
> +module_i2c_driver(os02g10_driver);
> +
> +MODULE_DESCRIPTION("OS02G10 Camera Sensor Driver");
> +MODULE_AUTHOR("Tarang Raval <tarang.raval@siliconsignals.io>");
> +MODULE_AUTHOR("Elgin Perumbilly <elgin.perumbilly@siliconsignals.io>");
> +MODULE_LICENSE("GPL");
In my opinion the driver looks good, nothing beyond cosmetic changes is needed.
--
Best wishes,
Vladimir
^ permalink raw reply
* Re: [PATCH v2 1/3] perf: marvell: Add MPAM partid filtering to CN10K TAD PMU
From: sashiko-bot @ 2026-06-12 10:14 UTC (permalink / raw)
To: Geetha sowjanya; +Cc: robh, conor+dt, devicetree, linux-perf-users
In-Reply-To: <20260612095746.19679-2-gakula@marvell.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Inconsistent masking of `event->attr.config` causes valid MPAM partition ID filters to be silently bypassed (or hardware programming to be skipped entirely) when upper bits are set.
--
commit f4f2ecc176ec6e4641448761b6600d2344554e81
Author: Tanmay Jagdale <tanmay@marvell.com>
perf: marvell: Add MPAM partid filtering to CN10K TAD PMU
The TAD PMU exposes counters that can be filtered by MPAM partition id
for a subset of allocation and hit events.
> diff --git a/drivers/perf/marvell_cn10k_tad_pmu.c b/drivers/perf/marvell_cn10k_tad_pmu.c
> index 51ccb0befa05d..af706b890bf17 100644
> --- a/drivers/perf/marvell_cn10k_tad_pmu.c
> +++ b/drivers/perf/marvell_cn10k_tad_pmu.c
[ ... ]
> @@ -27,30 +35,92 @@ struct tad_region {
[ ... ]
> +static void tad_pmu_start_counter(struct tad_pmu *pmu,
> + struct perf_event *event)
> +{
> + const struct tad_pmu_data *pdata = pmu->pdata;
> + struct hw_perf_event *hwc = &event->hw;
> + u32 event_idx = event->attr.config;
[Severity: Medium]
Does this cause valid MPAM partition ID filters to be silently bypassed if
upper bits are set in event->attr.config?
In tad_pmu_event_init(), the event_idx is masked to 8 bits:
u32 event_idx = (u32)(event->attr.config & GENMASK(7, 0));
Since it is masked there, an event with upper bits set successfully passes
the validation checks. However, because it is unmasked here in
tad_pmu_start_counter(), it will fail the bounds checks below while the
lower 8 bits are still eventually programmed into the hardware counter:
> + u32 counter_idx = hwc->idx;
> + u64 partid_filter = 0;
> + u64 reg_val;
> + u64 cfg1 = event->attr.config1;
> + bool use_mpam = cfg1 & TAD_PARTID_FILTER_EN;
> + u32 partid = (u32)(cfg1 & GENMASK(8, 0));
> + int i;
> +
> + for (i = 0; i < pmu->region_cnt; i++)
> + writeq_relaxed(0, pmu->regions[i].base +
> + TAD_PFC(pdata->tad_pfc_offset, counter_idx));
> +
> + if (use_mpam && event_idx > 0x19 && event_idx < 0x21) {
> + partid_filter = TAD_PRF_MATCH_PARTID | TAD_PRF_PARTID_NS |
> + ((u64)partid << 11);
> + }
> +
> +
> + for (i = 0; i < pmu->region_cnt; i++) {
> + reg_val = event_idx & 0xFF;
> + reg_val |= partid_filter;
> + writeq_relaxed(reg_val, pmu->regions[i].base +
> + TAD_PRF(pdata->tad_prf_offset, counter_idx));
> + }
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612095746.19679-1-gakula@marvell.com?part=1
^ permalink raw reply
* Re: [PATCH v4 13/16] media: iris: Introduce buffer size calculations for AR50LT
From: sashiko-bot @ 2026-06-12 10:22 UTC (permalink / raw)
To: Dmitry Baryshkov; +Cc: robh, devicetree, conor+dt
In-Reply-To: <20260612-iris-ar50lt-v4-13-0abfb74d5b3c@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The AR50LT line buffer calculations ignore the `is_opb` parameter and fail to allocate the required VPSS line buffer size when split mode is enabled.
- [High] AR50LT H264 and HEVC bin buffer size calculations lack a minimum lower-bound threshold.
--
commit 3d321c841d8d8ce0ec81cd70b48ab049db102de4
Author: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>
media: iris: Introduce buffer size calculations for AR50LT
Introduces AR50LT buffer size calculation for both encoder and
decoder. Reuse the buffer size calculation which are common, while
adding the AR50LT specific ones separately.
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_buffer.c b/drivers/media/platform/qcom/iris/iris_vpu_buffer.c
> index 4a39b8fef52b0..ca03d65705136 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu_buffer.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_buffer.c
> @@ -50,6 +50,32 @@ static u32 hfi_buffer_bin_h264d(u32 frame_width, u32 frame_height, u32 num_vpp_p
> return size_h264d_hw_bin_buffer(n_aligned_w, n_aligned_h, num_vpp_pipes);
> }
>
> +static u32 size_h264d_hw_bin_buffer_ar50lt(u32 frame_width, u32 frame_height, u32 num_vpp_pipes)
> +{
> + u32 size_yuv, size_bin_hdr, size_bin_res;
> +
> + size_yuv = ((frame_width * frame_height * 3) >> 1);
> + if (size_yuv <= 1920 * 1088 * 3 / 2) {
[Severity: High]
Does this calculation lack a minimum safe threshold for size_yuv?
The generic implementations clamp the base pixel area against
BIN_BUFFER_THRESHOLD, and the AR50LT VP9 implementation correctly applies a
lower bound. If a stream with a very small resolution is provided, could
this allocate a dangerously small bin buffer and lead to hardware memory
corruption?
[ ... ]
> @@ -111,6 +152,32 @@ static u32 hfi_buffer_bin_h265d(u32 frame_width, u32 frame_height, u32 num_vpp_p
> return size_h265d_hw_bin_buffer(n_aligned_w, n_aligned_h, num_vpp_pipes);
> }
>
> +static u32 size_h265d_hw_bin_buffer_ar50lt(u32 frame_width, u32 frame_height, u32 num_vpp_pipes)
> +{
> + u32 size_yuv, size_bin_hdr, size_bin_res;
> +
> + size_yuv = ((frame_width * frame_height * 3) >> 1);
> + if (size_yuv <= ((BIN_BUFFER_THRESHOLD * 3) >> 1)) {
[Severity: High]
Similar to the H.264 case above, does this calculation also need a
lower-bound threshold applied to prevent hardware issues with extremely
small stream resolutions?
[ ... ]
> @@ -404,6 +584,29 @@ u32 hfi_buffer_line_h265d(u32 frame_width, u32 frame_height, bool is_opb, u32 nu
> return ALIGN((_size + vpss_lb_size), DMA_ALIGNMENT);
> }
>
> +static inline
> +u32 hfi_buffer_line_h265d_ar50lt(u32 frame_width, u32 frame_height, bool is_opb, u32 num_vpp_pipes)
> +{
> + u32 size;
> +
> + size = ALIGN(size_h265d_lb_fe_top_data_ar50lt(frame_width, frame_height), DMA_ALIGNMENT) +
> + ALIGN(size_h265d_lb_fe_top_ctrl(frame_width, frame_height), DMA_ALIGNMENT) +
> + ALIGN(size_h265d_lb_fe_left_ctrl(frame_width, frame_height),
> + DMA_ALIGNMENT) * num_vpp_pipes +
> + ALIGN(size_h265d_lb_se_left_ctrl_ar50lt(frame_width, frame_height),
> + DMA_ALIGNMENT) * num_vpp_pipes +
> + ALIGN(size_h265d_lb_se_top_ctrl(frame_width, frame_height), DMA_ALIGNMENT) +
> + ALIGN(size_h265d_lb_pe_top_data_ar50lt(frame_width, frame_height), DMA_ALIGNMENT) +
> + ALIGN(size_h265d_lb_vsp_top(frame_width, frame_height), DMA_ALIGNMENT) +
> + ALIGN(size_h265d_lb_vsp_left(frame_width, frame_height),
> + DMA_ALIGNMENT) * num_vpp_pipes +
> + ALIGN(size_h265d_lb_recon_dma_metadata_wr(frame_width, frame_height),
> + DMA_ALIGNMENT) * 4 +
> + ALIGN(size_h265d_qp(frame_width, frame_height), DMA_ALIGNMENT);
> +
> + return ALIGN(size, DMA_ALIGNMENT);
> +}
[Severity: High]
Does this function completely ignore the is_opb parameter?
In the generic hfi_buffer_line_h265d(), is_opb is used to add the VPSS line
buffer size via size_vpss_lb(). Could missing this buffer size lead to
out-of-bounds DMA memory writes when split mode is enabled and hardware
expects the additional allocation?
[ ... ]
> @@ -507,6 +734,13 @@ u32 hfi_buffer_line_vp9d(u32 frame_width, u32 frame_height, u32 _yuv_bufcount_mi
> return _lb_size + vpss_lb_size + 4096;
> }
>
> +static inline
> +u32 hfi_buffer_line_vp9d_ar50lt(u32 frame_width, u32 frame_height, u32 _yuv_bufcount_min,
> + bool is_opb, u32 num_vpp_pipes)
> +{
> + return hfi_ar50lt_vp9d_lb_size(frame_width, frame_height, num_vpp_pipes);
> +}
[Severity: High]
Similar to the HEVC case, the is_opb parameter is accepted here but unused.
Should size_vpss_lb() be factored in for VP9 line buffers as well when
split mode is active?
[ ... ]
> @@ -529,6 +763,25 @@ static u32 hfi_buffer_line_h264d(u32 frame_width, u32 frame_height,
> return ALIGN((size + vpss_lb_size), DMA_ALIGNMENT);
> }
>
> +static u32 hfi_buffer_line_h264d_ar50lt(u32 frame_width, u32 frame_height,
> + bool is_opb, u32 num_vpp_pipes)
> +{
> + u32 size;
> +
> + size = ALIGN(size_h264d_lb_fe_top_data_ar50lt(frame_width), DMA_ALIGNMENT) +
> + ALIGN(size_h264d_lb_fe_top_ctrl_ar50lt(frame_width), DMA_ALIGNMENT) +
> + ALIGN(size_h264d_lb_fe_left_ctrl(frame_height), DMA_ALIGNMENT) * num_vpp_pipes +
> + ALIGN(size_h264d_lb_se_top_ctrl_ar50lt(frame_width), DMA_ALIGNMENT) +
> + ALIGN(size_h264d_lb_se_left_ctrl_ar50lt(frame_height), DMA_ALIGNMENT) *
> + num_vpp_pipes +
> + ALIGN(size_h264d_lb_pe_top_data_ar50lt(frame_width), DMA_ALIGNMENT) +
> + ALIGN(size_h264d_lb_vsp_top(frame_width), DMA_ALIGNMENT) +
> + ALIGN(size_h264d_lb_recon_dma_metadata_wr(frame_height), DMA_ALIGNMENT) * 2 +
> + ALIGN(size_h264d_qp(frame_width, frame_height), DMA_ALIGNMENT);
> +
> + return ALIGN(size, DMA_ALIGNMENT);
> +}
[Severity: High]
Once again, is_opb is ignored in this AR50LT H.264 line buffer calculation.
Does the hardware still expect the VPSS line buffer allocation when split
mode is enabled here?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-iris-ar50lt-v4-0-0abfb74d5b3c@oss.qualcomm.com?part=13
^ permalink raw reply
* Re: [PATCH v4 12/16] media: iris: implement support for the Agatti platform
From: sashiko-bot @ 2026-06-12 10:25 UTC (permalink / raw)
To: Dmitry Baryshkov; +Cc: conor+dt, devicetree, robh
In-Reply-To: <20260612-iris-ar50lt-v4-12-0abfb74d5b3c@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Calling a synchronous HFI command from the ISR response thread causes a deadlock.
--
commit 960bbe7059755e03232b477bd06562c01b126fdd
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
media: iris: implement support for the Agatti platform
Port support for the AR50Lt video codec core (present for example on the
Agatti platform) to the Iris driver. Unlike more recent cores this
generation doesn't have the PIPE property (as it always has only one
pipe). Also, unlike newer platforms, buffer sizes are requested from the
firmware instead of being calculated by the driver.
> diff --git a/drivers/media/platform/qcom/iris/iris_vpu_buffer.c b/drivers/media/platform/qcom/iris/iris_vpu_buffer.c
> index fb6f1016415e2..4a39b8fef52b0 100644
> --- a/drivers/media/platform/qcom/iris/iris_vpu_buffer.c
> +++ b/drivers/media/platform/qcom/iris/iris_vpu_buffer.c
> @@ -2194,6 +2194,19 @@ u32 iris_vpu4x_buf_size(struct iris_inst *inst, enum iris_buffer_type buffer_typ
> return size;
> }
>
> +u32 iris_vpu_ar50lt_gen1_buf_size(struct iris_inst *inst, enum iris_buffer_type buffer_type)
> +{
> + const struct iris_hfi_session_ops *hfi_ops = inst->hfi_session_ops;
> + int ret;
> +
> + /* return 0 on error to let the driver cope */
> + ret = hfi_ops->session_get_property(inst, HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS);
[Severity: High]
Could this lead to a regression by waiting for a response in the response
handler thread?
When iris_vpu_ar50lt_gen1_buf_size() calls hfi_ops->session_get_property(),
it synchronously blocks until the firmware responds:
drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c:iris_hfi_gen1_session_get_property() {
...
return iris_wait_for_session_response(inst, false);
}
However, iris_vpu_ar50lt_gen1_buf_size() (via iris_get_buffer_size()) is
invoked from iris_hfi_gen1_read_changed_params() during event processing:
drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c:iris_hfi_gen1_read_changed_params() {
...
pixmp_op->plane_fmt[0].sizeimage = iris_get_buffer_size(inst, BUF_OUTPUT);
...
}
This event processing runs inside the driver's response handler:
drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c:iris_hfi_gen1_response_handler() {
...
iris_hfi_gen1_handle_response(core, core->response_packet);
...
}
If the response handler thread blocks itself waiting for a response, how will
that response ever be dequeued and processed? It appears this will stall the
response polling loop until the wait times out.
> + if (ret)
> + return 0;
> +
> + return inst->buffers[buffer_type].size;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-iris-ar50lt-v4-0-0abfb74d5b3c@oss.qualcomm.com?part=12
^ permalink raw reply
* Re: [PATCH v2 2/3] perf: marvell: Add CN20K LLC-TAD PMU support
From: sashiko-bot @ 2026-06-12 10:26 UTC (permalink / raw)
To: Geetha sowjanya; +Cc: robh, devicetree, linux-perf-users, conor+dt
In-Reply-To: <20260612095746.19679-3-gakula@marvell.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Inconsistent masking of `event_idx` between validation and execution allows userspace to induce an aborted hardware programming state or bypass MPAM filters.
--
commit 13a14a3d137a1e1e8ddf65c82eb4ff187ff64ca5
Author: Geetha sowjanya <gakula@marvell.com>
perf: marvell: Add CN20K LLC-TAD PMU support
This commit adds support for the LLC Tag-and-Data (TAD) PMU on Marvell
CN20K SoCs. It introduces a new PMU profile, extended event maps, and
specific register base offsets.
> diff --git a/drivers/perf/marvell_cn10k_tad_pmu.c b/drivers/perf/marvell_cn10k_tad_pmu.c
> index af706b890bf17..e43598a52859e 100644
> --- a/drivers/perf/marvell_cn10k_tad_pmu.c
> +++ b/drivers/perf/marvell_cn10k_tad_pmu.c
[ ... ]
> @@ -85,8 +89,15 @@ static void tad_pmu_start_counter(struct tad_pmu *pmu,
> if (use_mpam && event_idx > 0x19 && event_idx < 0x21) {
> partid_filter = TAD_PRF_MATCH_PARTID | TAD_PRF_PARTID_NS |
> ((u64)partid << 11);
> +
> + if (pdata->id == TAD_PMU_V3)
> + partid_filter = TAD_PRF_MATCH_PARTID | TAD_PRF_MATCH_MPAMNS |
> + ((u64)partid << 10);
> }
>
> + /* CN10K support events 0:24*/
> + if (pdata->id == TAD_PMU_V1 && event_idx >= 0x25)
> + return;
[Severity: Medium]
In tad_pmu_event_init(), the event configuration is validated using a masked
value:
tad_pmu_event_init() {
...
u32 event_idx = (u32)(event->attr.config & GENMASK(7, 0));
...
}
However, in tad_pmu_start_counter(), event_idx is assigned directly from the
unmasked event->attr.config.
If a user provides an event configuration with upper bits set (e.g., 0x124),
it will pass the masked initialization checks, but won't it fail the unmasked
bounds checks here?
For V1 hardware, an unmasked config like 0x124 would evaluate as >= 0x25 and
trigger this new early return after clearing the counter but before
programming it, leaving the hardware in an aborted state.
Similarly, for V3 hardware, wouldn't the unmasked event_idx fail the
MPAM filter condition (event_idx > 0x19 && event_idx < 0x21) earlier in this
function, causing the counter to incorrectly bypass the filter and record all
partition IDs?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260612095746.19679-1-gakula@marvell.com?part=2
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox