* Re: [PATCH v3 09/25] media: i2c: imx258: Add support for running on 2 CSI data lanes
From: Pavel Machek @ 2024-04-03 18:45 UTC (permalink / raw)
To: git
Cc: linux-media, dave.stevenson, jacopo.mondi, mchehab, robh,
krzysztof.kozlowski+dt, conor+dt, shawnguo, s.hauer, kernel,
festevam, sakari.ailus, devicetree, imx, linux-arm-kernel,
linux-kernel, phone-devel
In-Reply-To: <20240403150355.189229-10-git@luigi311.com>
[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]
Hi!
> +/*
> + * 4208x3120 @ 30 fps needs 1267Mbps/lane, 4 lanes.
> + * To avoid further computation of clock settings, adopt the same per
> + * lane data rate when using 2 lanes, thus allowing a maximum of 15fps.
> + */
> +static const struct imx258_reg mipi_1267mbps_19_2mhz_2l[] = {
> + { 0x0136, 0x13 },
> + { 0x0137, 0x33 },
> + { 0x0301, 0x0A },
> + { 0x0303, 0x02 },
> + { 0x0305, 0x03 },
> + { 0x0306, 0x00 },
> + { 0x0307, 0xC6 },
> + { 0x0309, 0x0A },
> + { 0x030B, 0x01 },
> + { 0x030D, 0x02 },
> + { 0x030E, 0x00 },
> + { 0x030F, 0xD8 },
> + { 0x0310, 0x00 },
> +
> + { 0x0114, 0x01 },
> + { 0x0820, 0x09 },
> + { 0x0821, 0xa6 },
> + { 0x0822, 0x66 },
> + { 0x0823, 0x66 },
> +};
> +
> +static const struct imx258_reg mipi_1267mbps_19_2mhz_4l[] = {
> { 0x0136, 0x13 },
> { 0x0137, 0x33 },
> { 0x0301, 0x05 },
I wish we did not have to copy all the magic values like this.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [PATCH v3 10/25] media: i2c: imx258: Follow normal V4L2 behaviours for clipping exposure
From: Pavel Machek @ 2024-04-03 18:45 UTC (permalink / raw)
To: git
Cc: linux-media, dave.stevenson, jacopo.mondi, mchehab, robh,
krzysztof.kozlowski+dt, conor+dt, shawnguo, s.hauer, kernel,
festevam, sakari.ailus, devicetree, imx, linux-arm-kernel,
linux-kernel, phone-devel
In-Reply-To: <20240403150355.189229-11-git@luigi311.com>
[-- Attachment #1: Type: text/plain, Size: 329 bytes --]
On Wed 2024-04-03 09:03:39, git@luigi311.com wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> V4L2 sensor drivers are expected are expected to clip the supported
Remove one copy of "are expected".
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [PATCH v3 11/25] media: i2c: imx258: Add get_selection for pixel array information
From: Pavel Machek @ 2024-04-03 18:46 UTC (permalink / raw)
To: git
Cc: linux-media, dave.stevenson, jacopo.mondi, mchehab, robh,
krzysztof.kozlowski+dt, conor+dt, shawnguo, s.hauer, kernel,
festevam, sakari.ailus, devicetree, imx, linux-arm-kernel,
linux-kernel, phone-devel
In-Reply-To: <20240403150355.189229-12-git@luigi311.com>
[-- Attachment #1: Type: text/plain, Size: 559 bytes --]
Hi!
> Libcamera requires the cropping information for each mode, so
> add this information to the driver.
> @@ -116,6 +124,9 @@ struct imx258_mode {
> u32 link_freq_index;
> /* Default register values */
> struct imx258_reg_list reg_list;
> +
> + /* Analog crop rectangle. */
No need for "." at the end, as it is not above.
> + struct v4l2_rect crop;
> };
If the crop is same in all modes, should we have it in common place?
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [PATCH v3 12/25] media: i2c: imx258: Allow configuration of clock lane behaviour
From: Pavel Machek @ 2024-04-03 18:48 UTC (permalink / raw)
To: git
Cc: linux-media, dave.stevenson, jacopo.mondi, mchehab, robh,
krzysztof.kozlowski+dt, conor+dt, shawnguo, s.hauer, kernel,
festevam, sakari.ailus, devicetree, imx, linux-arm-kernel,
linux-kernel, phone-devel
In-Reply-To: <20240403150355.189229-13-git@luigi311.com>
[-- Attachment #1: Type: text/plain, Size: 622 bytes --]
Hi!
> The sensor supports the clock lane either remaining in HS mode
> during frame blanking, or dropping to LP11.
>
> Add configuration of the mode via V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK.
> + ret = imx258_write_reg(imx258, IMX258_CLK_BLANK_STOP,
> + IMX258_REG_VALUE_08BIT,
> + imx258->csi2_flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK ?
> + 1 : 0);
!! can be used to turn value into 1/0. I find it easier to read than ?
1 : 0 combination, but possibly that's fine, too.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [PATCH v3 22/25] dt-bindings: media: imx258: Add binding for powerdown-gpio
From: Pavel Machek @ 2024-04-03 18:49 UTC (permalink / raw)
To: git
Cc: linux-media, dave.stevenson, jacopo.mondi, mchehab, robh,
krzysztof.kozlowski+dt, conor+dt, shawnguo, s.hauer, kernel,
festevam, sakari.ailus, devicetree, imx, linux-arm-kernel,
linux-kernel, phone-devel, Ondrej Jirman
In-Reply-To: <20240403150355.189229-23-git@luigi311.com>
[-- Attachment #1: Type: text/plain, Size: 408 bytes --]
Hi!
> From: Luis Garcia <git@luigi311.com>
>
> Add powerdown-gpio binding as it is required for some boards
"." at end of sentence.
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Signed-off-by: Luis Garcia <git@luigi311.com>
If the patch is from Ondrej, he should be in From:, I believe.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [PATCH v2] arm64: dts: qcom: msm8916-samsung-fortuna: Add touchscreen
From: Bjorn Andersson @ 2024-04-03 18:49 UTC (permalink / raw)
To: Raymond Hackley
Cc: linux-kernel, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Stephan Gerhold, Nikita Travkin, linux-arm-msm,
devicetree, ~postmarketos/upstreaming, Joe Mason
In-Reply-To: <20240312074536.62964-1-raymondhackley@protonmail.com>
On Tue, Mar 12, 2024 at 07:45:42AM +0000, Raymond Hackley wrote:
> diff --git a/arch/arm64/boot/dts/qcom/msm8916-samsung-fortuna-common.dtsi b/arch/arm64/boot/dts/qcom/msm8916-samsung-fortuna-common.dtsi
[..]
> +&blsp_i2c5 {
> + status = "okay";
> +
> + touchscreen: touchscreen@20 {
> + compatible = "zinitix,bt541";
> + reg = <0x20>;
> +
> + interrupts-extended = <&tlmm 13 IRQ_TYPE_EDGE_FALLING>;
> +
> + touchscreen-size-x = <540>;
> + touchscreen-size-y = <960>;
> +
> + vcca-supply = <®_vdd_tsp_a>;
> + vdd-supply = <&pm8916_l6>;
> +
> + pinctrl-0 = <&tsp_int_default>;
> + pinctrl-names = "default";
> +
> + linux,keycodes = <KEY_APPSELECT KEY_BACK>;
linux,keycodes is not a valid property of zinitix,bt541 according to the
DeviceTree binding. Is there a binding update for this somewhere?
Regards,
Bjorn
^ permalink raw reply
* Re: [PATCH v3 00/25] imx258 improvement series
From: Pavel Machek @ 2024-04-03 18:50 UTC (permalink / raw)
To: git
Cc: linux-media, dave.stevenson, jacopo.mondi, mchehab, robh,
krzysztof.kozlowski+dt, conor+dt, shawnguo, s.hauer, kernel,
festevam, sakari.ailus, devicetree, imx, linux-arm-kernel,
linux-kernel, phone-devel
In-Reply-To: <20240403150355.189229-1-git@luigi311.com>
[-- Attachment #1: Type: text/plain, Size: 257 bytes --]
Hi!
Thanks for doing this. I had some comments on 5, 9, 10, 11, 12, 21
and 22. You can add "Reviewed-by: Pavel Machek <pavel@ucw.cz>" to the
rest.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [PATCH v3 10/25] media: i2c: imx258: Follow normal V4L2 behaviours for clipping exposure
From: Luigi311 @ 2024-04-03 19:14 UTC (permalink / raw)
To: Pavel Machek
Cc: linux-media, dave.stevenson, jacopo.mondi, mchehab, robh,
krzysztof.kozlowski+dt, conor+dt, shawnguo, s.hauer, kernel,
festevam, sakari.ailus, devicetree, imx, linux-arm-kernel,
linux-kernel, phone-devel
In-Reply-To: <Zg2j1XeOhFwO2Nf2@duo.ucw.cz>
On 4/3/24 12:45, Pavel Machek wrote:
> On Wed 2024-04-03 09:03:39, git@luigi311.com wrote:
>> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>
>> V4L2 sensor drivers are expected are expected to clip the supported
>
> Remove one copy of "are expected".
>
> Best regards,
> Pavel
>
Done
^ permalink raw reply
* Re: [PATCH v3 21/25] drivers: media: i2c: imx258: Use macros
From: Luigi311 @ 2024-04-03 19:17 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, dave.stevenson, jacopo.mondi, mchehab, robh,
krzysztof.kozlowski+dt, conor+dt, shawnguo, s.hauer, kernel,
festevam, devicetree, imx, linux-arm-kernel, linux-kernel, pavel,
phone-devel, Ondrej Jirman
In-Reply-To: <Zg2CirmwL3JfjA8s@kekkonen.localdomain>
On 4/3/24 10:23, Sakari Ailus wrote:
> Hi Luis,
>
> On Wed, Apr 03, 2024 at 09:03:50AM -0600, git@luigi311.com wrote:
>> From: Luis Garcia <git@luigi311.com>
>>
>> Use understandable macros instead of raw values.
>>
>> Signed-off-by: Ondrej Jirman <megi@xff.cz>
>> Signed-off-by: Luis Garcia <git@luigi311.com>
>> ---
>> drivers/media/i2c/imx258.c | 434 ++++++++++++++++++-------------------
>> 1 file changed, 207 insertions(+), 227 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
>> index e2ecf6109516..30352c33f63c 100644
>> --- a/drivers/media/i2c/imx258.c
>> +++ b/drivers/media/i2c/imx258.c
>> @@ -33,8 +33,6 @@
>> #define IMX258_VTS_30FPS_VGA 0x034c
>> #define IMX258_VTS_MAX 65525
>>
>> -#define IMX258_REG_VTS 0x0340
>> -
>> /* HBLANK control - read only */
>> #define IMX258_PPL_DEFAULT 5352
>>
>> @@ -90,6 +88,53 @@
>> #define IMX258_PIXEL_ARRAY_WIDTH 4208U
>> #define IMX258_PIXEL_ARRAY_HEIGHT 3120U
>>
>> +/* regs */
>> +#define IMX258_REG_PLL_MULT_DRIV 0x0310
>> +#define IMX258_REG_IVTPXCK_DIV 0x0301
>> +#define IMX258_REG_IVTSYCK_DIV 0x0303
>> +#define IMX258_REG_PREPLLCK_VT_DIV 0x0305
>> +#define IMX258_REG_IOPPXCK_DIV 0x0309
>> +#define IMX258_REG_IOPSYCK_DIV 0x030b
>> +#define IMX258_REG_PREPLLCK_OP_DIV 0x030d
>> +#define IMX258_REG_PHASE_PIX_OUTEN 0x3030
>> +#define IMX258_REG_PDPIX_DATA_RATE 0x3032
>> +#define IMX258_REG_SCALE_MODE 0x0401
>> +#define IMX258_REG_SCALE_MODE_EXT 0x3038
>> +#define IMX258_REG_AF_WINDOW_MODE 0x7bcd
>> +#define IMX258_REG_FRM_LENGTH_CTL 0x0350
>> +#define IMX258_REG_CSI_LANE_MODE 0x0114
>> +#define IMX258_REG_X_EVN_INC 0x0381
>> +#define IMX258_REG_X_ODD_INC 0x0383
>> +#define IMX258_REG_Y_EVN_INC 0x0385
>> +#define IMX258_REG_Y_ODD_INC 0x0387
>> +#define IMX258_REG_BINNING_MODE 0x0900
>> +#define IMX258_REG_BINNING_TYPE_V 0x0901
>> +#define IMX258_REG_FORCE_FD_SUM 0x300d
>> +#define IMX258_REG_DIG_CROP_X_OFFSET 0x0408
>> +#define IMX258_REG_DIG_CROP_Y_OFFSET 0x040a
>> +#define IMX258_REG_DIG_CROP_IMAGE_WIDTH 0x040c
>> +#define IMX258_REG_DIG_CROP_IMAGE_HEIGHT 0x040e
>> +#define IMX258_REG_SCALE_M 0x0404
>> +#define IMX258_REG_X_OUT_SIZE 0x034c
>> +#define IMX258_REG_Y_OUT_SIZE 0x034e
>> +#define IMX258_REG_X_ADD_STA 0x0344
>> +#define IMX258_REG_Y_ADD_STA 0x0346
>> +#define IMX258_REG_X_ADD_END 0x0348
>> +#define IMX258_REG_Y_ADD_END 0x034a
>> +#define IMX258_REG_EXCK_FREQ 0x0136
>> +#define IMX258_REG_CSI_DT_FMT 0x0112
>> +#define IMX258_REG_LINE_LENGTH_PCK 0x0342
>> +#define IMX258_REG_SCALE_M_EXT 0x303a
>> +#define IMX258_REG_FRM_LENGTH_LINES 0x0340
>> +#define IMX258_REG_FINE_INTEG_TIME 0x0200
>> +#define IMX258_REG_PLL_IVT_MPY 0x0306
>> +#define IMX258_REG_PLL_IOP_MPY 0x030e
>> +#define IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H 0x0820
>> +#define IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L 0x0822
>> +
>> +#define REG8(a, v) { a, v }
>> +#define REG16(a, v) { a, ((v) >> 8) & 0xff }, { (a) + 1, (v) & 0xff }
>
> The patch is nice but these macros are better replaced by the V4L2 CCI
> helper that also offers register access functions. Could you add a patch to
> convert the driver to use it (maybe after this one)?
>
Ohh perfect, using something else would be great. Ill go ahead and see
if I can get that working.
>> +
>> struct imx258_reg {
>> u16 address;
>> u8 val;
>> @@ -145,179 +190,139 @@ struct imx258_mode {
>> * lane data rate when using 2 lanes, thus allowing a maximum of 15fps.
>> */
>> static const struct imx258_reg mipi_1267mbps_19_2mhz_2l[] = {
>> - { 0x0136, 0x13 },
>> - { 0x0137, 0x33 },
>> - { 0x0301, 0x0A },
>> - { 0x0303, 0x02 },
>> - { 0x0305, 0x03 },
>> - { 0x0306, 0x00 },
>> - { 0x0307, 0xC6 },
>> - { 0x0309, 0x0A },
>> - { 0x030B, 0x01 },
>> - { 0x030D, 0x02 },
>> - { 0x030E, 0x00 },
>> - { 0x030F, 0xD8 },
>> - { 0x0310, 0x00 },
>> -
>> - { 0x0114, 0x01 },
>> - { 0x0820, 0x09 },
>> - { 0x0821, 0xa6 },
>> - { 0x0822, 0x66 },
>> - { 0x0823, 0x66 },
>> + REG16(IMX258_REG_EXCK_FREQ, 0x1333),
>> + REG8(IMX258_REG_IVTPXCK_DIV, 10),
>> + REG8(IMX258_REG_IVTSYCK_DIV, 2),
>> + REG8(IMX258_REG_PREPLLCK_VT_DIV, 3),
>> + REG16(IMX258_REG_PLL_IVT_MPY, 0x00C6),
>> + REG8(IMX258_REG_IOPPXCK_DIV, 10),
>> + REG8(IMX258_REG_IOPSYCK_DIV, 1),
>> + REG8(IMX258_REG_PREPLLCK_OP_DIV, 2),
>> + REG16(IMX258_REG_PLL_IOP_MPY, 0x00D8),
>> + REG8(IMX258_REG_PLL_MULT_DRIV, 0),
>> +
>> + REG8(IMX258_REG_CSI_LANE_MODE, 1),
>> + REG16(IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 0x09A6),
>> + REG16(IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0x6666),
>> };
>>
>> static const struct imx258_reg mipi_1267mbps_19_2mhz_4l[] = {
>> - { 0x0136, 0x13 },
>> - { 0x0137, 0x33 },
>> - { 0x0301, 0x05 },
>> - { 0x0303, 0x02 },
>> - { 0x0305, 0x03 },
>> - { 0x0306, 0x00 },
>> - { 0x0307, 0xC6 },
>> - { 0x0309, 0x0A },
>> - { 0x030B, 0x01 },
>> - { 0x030D, 0x02 },
>> - { 0x030E, 0x00 },
>> - { 0x030F, 0xD8 },
>> - { 0x0310, 0x00 },
>> -
>> - { 0x0114, 0x03 },
>> - { 0x0820, 0x13 },
>> - { 0x0821, 0x4C },
>> - { 0x0822, 0xCC },
>> - { 0x0823, 0xCC },
>> + REG16(IMX258_REG_EXCK_FREQ, 0x1333),
>> + REG8(IMX258_REG_IVTPXCK_DIV, 5),
>> + REG8(IMX258_REG_IVTSYCK_DIV, 2),
>> + REG8(IMX258_REG_PREPLLCK_VT_DIV, 3),
>> + REG16(IMX258_REG_PLL_IVT_MPY, 0x00C6),
>> + REG8(IMX258_REG_IOPPXCK_DIV, 10),
>> + REG8(IMX258_REG_IOPSYCK_DIV, 1),
>> + REG8(IMX258_REG_PREPLLCK_OP_DIV, 2),
>> + REG16(IMX258_REG_PLL_IOP_MPY, 0x00D8),
>> + REG8(IMX258_REG_PLL_MULT_DRIV, 0),
>> +
>> + REG8(IMX258_REG_CSI_LANE_MODE, 3),
>> + REG16(IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 0x134C),
>> + REG16(IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0xCCCC),
>> };
>>
>> static const struct imx258_reg mipi_1272mbps_24mhz_2l[] = {
>> - { 0x0136, 0x18 },
>> - { 0x0137, 0x00 },
>> - { 0x0301, 0x0a },
>> - { 0x0303, 0x02 },
>> - { 0x0305, 0x04 },
>> - { 0x0306, 0x00 },
>> - { 0x0307, 0xD4 },
>> - { 0x0309, 0x0A },
>> - { 0x030B, 0x01 },
>> - { 0x030D, 0x02 },
>> - { 0x030E, 0x00 },
>> - { 0x030F, 0xD8 },
>> - { 0x0310, 0x00 },
>> -
>> - { 0x0114, 0x01 },
>> - { 0x0820, 0x13 },
>> - { 0x0821, 0x4C },
>> - { 0x0822, 0xCC },
>> - { 0x0823, 0xCC },
>> + REG16(IMX258_REG_EXCK_FREQ, 0x1800),
>> + REG8(IMX258_REG_IVTPXCK_DIV, 10),
>> + REG8(IMX258_REG_IVTSYCK_DIV, 2),
>> + REG8(IMX258_REG_PREPLLCK_VT_DIV, 4),
>> + REG16(IMX258_REG_PLL_IVT_MPY, 0x00D4),
>> + REG8(IMX258_REG_IOPPXCK_DIV, 10),
>> + REG8(IMX258_REG_IOPSYCK_DIV, 1),
>> + REG8(IMX258_REG_PREPLLCK_OP_DIV, 2),
>> + REG16(IMX258_REG_PLL_IOP_MPY, 0x00D8),
>> + REG8(IMX258_REG_PLL_MULT_DRIV, 0),
>> +
>> + REG8(IMX258_REG_CSI_LANE_MODE, 1),
>> + REG16(IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 0x134C),
>> + REG16(IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0xCCCC),
>> };
>>
>> static const struct imx258_reg mipi_1272mbps_24mhz_4l[] = {
>> - { 0x0136, 0x18 },
>> - { 0x0137, 0x00 },
>> - { 0x0301, 0x05 },
>> - { 0x0303, 0x02 },
>> - { 0x0305, 0x04 },
>> - { 0x0306, 0x00 },
>> - { 0x0307, 0xD4 },
>> - { 0x0309, 0x0A },
>> - { 0x030B, 0x01 },
>> - { 0x030D, 0x02 },
>> - { 0x030E, 0x00 },
>> - { 0x030F, 0xD8 },
>> - { 0x0310, 0x00 },
>> -
>> - { 0x0114, 0x03 },
>> - { 0x0820, 0x13 },
>> - { 0x0821, 0xE0 },
>> - { 0x0822, 0x00 },
>> - { 0x0823, 0x00 },
>> + REG16(IMX258_REG_EXCK_FREQ, 0x1800),
>> + REG8(IMX258_REG_IVTPXCK_DIV, 5),
>> + REG8(IMX258_REG_IVTSYCK_DIV, 2),
>> + REG8(IMX258_REG_PREPLLCK_VT_DIV, 4),
>> + REG16(IMX258_REG_PLL_IVT_MPY, 0x00D4),
>> + REG8(IMX258_REG_IOPPXCK_DIV, 10),
>> + REG8(IMX258_REG_IOPSYCK_DIV, 1),
>> + REG8(IMX258_REG_PREPLLCK_OP_DIV, 2),
>> + REG16(IMX258_REG_PLL_IOP_MPY, 0x00D8),
>> + REG8(IMX258_REG_PLL_MULT_DRIV, 0),
>> +
>> + REG8(IMX258_REG_CSI_LANE_MODE, 3),
>> + REG16(IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 0x13E0),
>> + REG16(IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0x0000),
>> };
>>
>> static const struct imx258_reg mipi_640mbps_19_2mhz_2l[] = {
>> - { 0x0136, 0x13 },
>> - { 0x0137, 0x33 },
>> - { 0x0301, 0x05 },
>> - { 0x0303, 0x02 },
>> - { 0x0305, 0x03 },
>> - { 0x0306, 0x00 },
>> - { 0x0307, 0x64 },
>> - { 0x0309, 0x0A },
>> - { 0x030B, 0x01 },
>> - { 0x030D, 0x02 },
>> - { 0x030E, 0x00 },
>> - { 0x030F, 0xD8 },
>> - { 0x0310, 0x00 },
>> -
>> - { 0x0114, 0x01 },
>> - { 0x0820, 0x05 },
>> - { 0x0821, 0x00 },
>> - { 0x0822, 0x00 },
>> - { 0x0823, 0x00 },
>> + REG16(IMX258_REG_EXCK_FREQ, 0x1333),
>> + REG8(IMX258_REG_IVTPXCK_DIV, 5),
>> + REG8(IMX258_REG_IVTSYCK_DIV, 2),
>> + REG8(IMX258_REG_PREPLLCK_VT_DIV, 3),
>> + REG16(IMX258_REG_PLL_IVT_MPY, 0x0064),
>> + REG8(IMX258_REG_IOPPXCK_DIV, 10),
>> + REG8(IMX258_REG_IOPSYCK_DIV, 1),
>> + REG8(IMX258_REG_PREPLLCK_OP_DIV, 2),
>> + REG16(IMX258_REG_PLL_IOP_MPY, 0x00D8),
>> + REG8(IMX258_REG_PLL_MULT_DRIV, 0),
>> +
>> + REG8(IMX258_REG_CSI_LANE_MODE, 1),
>> + REG16(IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 0x0500),
>> + REG16(IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0x0000),
>> };
>>
>> static const struct imx258_reg mipi_640mbps_19_2mhz_4l[] = {
>> - { 0x0136, 0x13 },
>> - { 0x0137, 0x33 },
>> - { 0x0301, 0x05 },
>> - { 0x0303, 0x02 },
>> - { 0x0305, 0x03 },
>> - { 0x0306, 0x00 },
>> - { 0x0307, 0x64 },
>> - { 0x0309, 0x0A },
>> - { 0x030B, 0x01 },
>> - { 0x030D, 0x02 },
>> - { 0x030E, 0x00 },
>> - { 0x030F, 0xD8 },
>> - { 0x0310, 0x00 },
>> -
>> - { 0x0114, 0x03 },
>> - { 0x0820, 0x0A },
>> - { 0x0821, 0x00 },
>> - { 0x0822, 0x00 },
>> - { 0x0823, 0x00 },
>> + REG16(IMX258_REG_EXCK_FREQ, 0x1333),
>> + REG8(IMX258_REG_IVTPXCK_DIV, 5),
>> + REG8(IMX258_REG_IVTSYCK_DIV, 2),
>> + REG8(IMX258_REG_PREPLLCK_VT_DIV, 3),
>> + REG16(IMX258_REG_PLL_IVT_MPY, 0x0064),
>> + REG8(IMX258_REG_IOPPXCK_DIV, 10),
>> + REG8(IMX258_REG_IOPSYCK_DIV, 1),
>> + REG8(IMX258_REG_PREPLLCK_OP_DIV, 2),
>> + REG16(IMX258_REG_PLL_IOP_MPY, 0x00D8),
>> + REG8(IMX258_REG_PLL_MULT_DRIV, 0),
>> +
>> + REG8(IMX258_REG_CSI_LANE_MODE, 3),
>> + REG16(IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 0x0A00),
>> + REG16(IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0x0000),
>> };
>>
>> static const struct imx258_reg mipi_642mbps_24mhz_2l[] = {
>> - { 0x0136, 0x18 },
>> - { 0x0137, 0x00 },
>> - { 0x0301, 0x05 },
>> - { 0x0303, 0x02 },
>> - { 0x0305, 0x04 },
>> - { 0x0306, 0x00 },
>> - { 0x0307, 0x6B },
>> - { 0x0309, 0x0A },
>> - { 0x030B, 0x01 },
>> - { 0x030D, 0x02 },
>> - { 0x030E, 0x00 },
>> - { 0x030F, 0xD8 },
>> - { 0x0310, 0x00 },
>> -
>> - { 0x0114, 0x01 },
>> - { 0x0820, 0x0A },
>> - { 0x0821, 0x00 },
>> - { 0x0822, 0x00 },
>> - { 0x0823, 0x00 },
>> + REG16(IMX258_REG_EXCK_FREQ, 0x1800),
>> + REG8(IMX258_REG_IVTPXCK_DIV, 5),
>> + REG8(IMX258_REG_IVTSYCK_DIV, 2),
>> + REG8(IMX258_REG_PREPLLCK_VT_DIV, 4),
>> + REG16(IMX258_REG_PLL_IVT_MPY, 0x006B),
>> + REG8(IMX258_REG_IOPPXCK_DIV, 10),
>> + REG8(IMX258_REG_IOPSYCK_DIV, 1),
>> + REG8(IMX258_REG_PREPLLCK_OP_DIV, 2),
>> + REG16(IMX258_REG_PLL_IOP_MPY, 0x00D8),
>> + REG8(IMX258_REG_PLL_MULT_DRIV, 0),
>> +
>> + REG8(IMX258_REG_CSI_LANE_MODE, 1),
>> + REG16(IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 0x0A00),
>> + REG16(IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0x0000),
>> };
>>
>> static const struct imx258_reg mipi_642mbps_24mhz_4l[] = {
>> - { 0x0136, 0x18 },
>> - { 0x0137, 0x00 },
>> - { 0x0301, 0x05 },
>> - { 0x0303, 0x02 },
>> - { 0x0305, 0x04 },
>> - { 0x0306, 0x00 },
>> - { 0x0307, 0x6B },
>> - { 0x0309, 0x0A },
>> - { 0x030B, 0x01 },
>> - { 0x030D, 0x02 },
>> - { 0x030E, 0x00 },
>> - { 0x030F, 0xD8 },
>> - { 0x0310, 0x00 },
>> -
>> - { 0x0114, 0x03 },
>> - { 0x0820, 0x0A },
>> - { 0x0821, 0x00 },
>> - { 0x0822, 0x00 },
>> - { 0x0823, 0x00 },
>> + REG16(IMX258_REG_EXCK_FREQ, 0x1800),
>> + REG8(IMX258_REG_IVTPXCK_DIV, 5),
>> + REG8(IMX258_REG_IVTSYCK_DIV, 2),
>> + REG8(IMX258_REG_PREPLLCK_VT_DIV, 4),
>> + REG16(IMX258_REG_PLL_IVT_MPY, 0x006B),
>> + REG8(IMX258_REG_IOPPXCK_DIV, 10),
>> + REG8(IMX258_REG_IOPSYCK_DIV, 1),
>> + REG8(IMX258_REG_PREPLLCK_OP_DIV, 2),
>> + REG16(IMX258_REG_PLL_IOP_MPY, 0x00D8),
>> + REG8(IMX258_REG_PLL_MULT_DRIV, 0),
>> +
>> + REG8(IMX258_REG_CSI_LANE_MODE, 3),
>> + REG16(IMX258_REG_REQ_LINK_BIT_RATE_MBPS_H, 0x0A00),
>> + REG16(IMX258_REG_REQ_LINK_BIT_RATE_MBPS_L, 0x0000),
>> };
>>
>> static const struct imx258_reg mode_common_regs[] = {
>> @@ -363,45 +368,29 @@ static const struct imx258_reg mode_common_regs[] = {
>> { 0x7423, 0xD7 },
>> { 0x5F04, 0x00 },
>> { 0x5F05, 0xED },
>> - { 0x0112, 0x0A },
>> - { 0x0113, 0x0A },
>> - { 0x0342, 0x14 },
>> - { 0x0343, 0xE8 },
>> - { 0x0344, 0x00 },
>> - { 0x0345, 0x00 },
>> - { 0x0346, 0x00 },
>> - { 0x0347, 0x00 },
>> - { 0x0348, 0x10 },
>> - { 0x0349, 0x6F },
>> - { 0x034A, 0x0C },
>> - { 0x034B, 0x2F },
>> - { 0x0381, 0x01 },
>> - { 0x0383, 0x01 },
>> - { 0x0385, 0x01 },
>> - { 0x0387, 0x01 },
>> - { 0x0404, 0x00 },
>> - { 0x0408, 0x00 },
>> - { 0x0409, 0x00 },
>> - { 0x040A, 0x00 },
>> - { 0x040B, 0x00 },
>> - { 0x040C, 0x10 },
>> - { 0x040D, 0x70 },
>> - { 0x3038, 0x00 },
>> - { 0x303A, 0x00 },
>> - { 0x303B, 0x10 },
>> - { 0x300D, 0x00 },
>> - { 0x0350, 0x00 },
>> - { 0x0204, 0x00 },
>> - { 0x0205, 0x00 },
>> - { 0x020E, 0x01 },
>> - { 0x020F, 0x00 },
>> - { 0x0210, 0x01 },
>> - { 0x0211, 0x00 },
>> - { 0x0212, 0x01 },
>> - { 0x0213, 0x00 },
>> - { 0x0214, 0x01 },
>> - { 0x0215, 0x00 },
>> - { 0x7BCD, 0x00 },
>> + REG16(IMX258_REG_CSI_DT_FMT, 0x0a0a),
>> + REG16(IMX258_REG_LINE_LENGTH_PCK, 5352),
>> + REG16(IMX258_REG_X_ADD_STA, 0),
>> + REG16(IMX258_REG_Y_ADD_STA, 0),
>> + REG16(IMX258_REG_X_ADD_END, 4207),
>> + REG16(IMX258_REG_Y_ADD_END, 3119),
>> + REG8(IMX258_REG_X_EVN_INC, 1),
>> + REG8(IMX258_REG_X_ODD_INC, 1),
>> + REG8(IMX258_REG_Y_EVN_INC, 1),
>> + REG8(IMX258_REG_Y_ODD_INC, 1),
>> + REG16(IMX258_REG_DIG_CROP_X_OFFSET, 0),
>> + REG16(IMX258_REG_DIG_CROP_Y_OFFSET, 0),
>> + REG16(IMX258_REG_DIG_CROP_IMAGE_WIDTH, 4208),
>> + REG8(IMX258_REG_SCALE_MODE_EXT, 0),
>> + REG16(IMX258_REG_SCALE_M_EXT, 16),
>> + REG8(IMX258_REG_FORCE_FD_SUM, 0),
>> + REG8(IMX258_REG_FRM_LENGTH_CTL, 0),
>> + REG16(IMX258_REG_ANALOG_GAIN, 0),
>> + REG16(IMX258_REG_GR_DIGITAL_GAIN, 256),
>> + REG16(IMX258_REG_R_DIGITAL_GAIN, 256),
>> + REG16(IMX258_REG_B_DIGITAL_GAIN, 256),
>> + REG16(IMX258_REG_GB_DIGITAL_GAIN, 256),
>> + REG8(IMX258_REG_AF_WINDOW_MODE, 0),
>> { 0x94DC, 0x20 },
>> { 0x94DD, 0x20 },
>> { 0x94DE, 0x20 },
>> @@ -414,48 +403,39 @@ static const struct imx258_reg mode_common_regs[] = {
>> { 0x941B, 0x50 },
>> { 0x9519, 0x50 },
>> { 0x951B, 0x50 },
>> - { 0x3030, 0x00 },
>> - { 0x3032, 0x00 },
>> - { 0x0220, 0x00 },
>> + REG8(IMX258_REG_PHASE_PIX_OUTEN, 0),
>> + REG8(IMX258_REG_PDPIX_DATA_RATE, 0),
>> + REG8(IMX258_REG_HDR, 0),
>> };
>>
>> static const struct imx258_reg mode_4208x3120_regs[] = {
>> - { 0x0900, 0x00 },
>> - { 0x0901, 0x11 },
>> - { 0x0401, 0x00 },
>> - { 0x0405, 0x10 },
>> - { 0x040E, 0x0C },
>> - { 0x040F, 0x30 },
>> - { 0x034C, 0x10 },
>> - { 0x034D, 0x70 },
>> - { 0x034E, 0x0C },
>> - { 0x034F, 0x30 },
>> + REG8(IMX258_REG_BINNING_MODE, 0),
>> + REG8(IMX258_REG_BINNING_TYPE_V, 0x11),
>> + REG8(IMX258_REG_SCALE_MODE, 0),
>> + REG16(IMX258_REG_SCALE_M, 16),
>> + REG16(IMX258_REG_DIG_CROP_IMAGE_HEIGHT, 3120),
>> + REG16(IMX258_REG_X_OUT_SIZE, 4208),
>> + REG16(IMX258_REG_Y_OUT_SIZE, 3120),
>> };
>>
>> static const struct imx258_reg mode_2104_1560_regs[] = {
>> - { 0x0900, 0x01 },
>> - { 0x0901, 0x12 },
>> - { 0x0401, 0x01 },
>> - { 0x0405, 0x20 },
>> - { 0x040E, 0x06 },
>> - { 0x040F, 0x18 },
>> - { 0x034C, 0x08 },
>> - { 0x034D, 0x38 },
>> - { 0x034E, 0x06 },
>> - { 0x034F, 0x18 },
>> + REG8(IMX258_REG_BINNING_MODE, 1),
>> + REG8(IMX258_REG_BINNING_TYPE_V, 0x12),
>> + REG8(IMX258_REG_SCALE_MODE, 1),
>> + REG16(IMX258_REG_SCALE_M, 32),
>> + REG16(IMX258_REG_DIG_CROP_IMAGE_HEIGHT, 1560),
>> + REG16(IMX258_REG_X_OUT_SIZE, 2104),
>> + REG16(IMX258_REG_Y_OUT_SIZE, 1560),
>> };
>>
>> static const struct imx258_reg mode_1048_780_regs[] = {
>> - { 0x0900, 0x01 },
>> - { 0x0901, 0x14 },
>> - { 0x0401, 0x01 },
>> - { 0x0405, 0x40 },
>> - { 0x040E, 0x03 },
>> - { 0x040F, 0x0C },
>> - { 0x034C, 0x04 },
>> - { 0x034D, 0x18 },
>> - { 0x034E, 0x03 },
>> - { 0x034F, 0x0C },
>> + REG8(IMX258_REG_BINNING_MODE, 1),
>> + REG8(IMX258_REG_BINNING_TYPE_V, 0x14),
>> + REG8(IMX258_REG_SCALE_MODE, 1),
>> + REG16(IMX258_REG_SCALE_M, 64),
>> + REG16(IMX258_REG_DIG_CROP_IMAGE_HEIGHT, 780),
>> + REG16(IMX258_REG_X_OUT_SIZE, 1048),
>> + REG16(IMX258_REG_Y_OUT_SIZE, 780),
>> };
>>
>> struct imx258_variant_cfg {
>> @@ -923,7 +903,7 @@ static int imx258_set_ctrl(struct v4l2_ctrl *ctrl)
>> }
>> break;
>> case V4L2_CID_VBLANK:
>> - ret = imx258_write_reg(imx258, IMX258_REG_VTS,
>> + ret = imx258_write_reg(imx258, IMX258_REG_FRM_LENGTH_LINES,
>> IMX258_REG_VALUE_16BIT,
>> imx258->cur_mode->height + ctrl->val);
>> break;
>
^ permalink raw reply
* Re: [PATCH v3 22/25] dt-bindings: media: imx258: Add binding for powerdown-gpio
From: Luigi311 @ 2024-04-03 19:26 UTC (permalink / raw)
To: Pavel Machek
Cc: linux-media, dave.stevenson, jacopo.mondi, mchehab, robh,
krzysztof.kozlowski+dt, conor+dt, shawnguo, s.hauer, kernel,
festevam, sakari.ailus, devicetree, imx, linux-arm-kernel,
linux-kernel, phone-devel, Ondrej Jirman
In-Reply-To: <Zg2kn5/5T9ukP4nd@duo.ucw.cz>
On 4/3/24 12:49, Pavel Machek wrote:
> Hi!
>
>> From: Luis Garcia <git@luigi311.com>
>>
>> Add powerdown-gpio binding as it is required for some boards
>
> "." at end of sentence.
>
>> Signed-off-by: Ondrej Jirman <megi@xff.cz>
>> Signed-off-by: Luis Garcia <git@luigi311.com>
>
> If the patch is from Ondrej, he should be in From:, I believe.
>
> Best regards,
> Pavel
Ohh your right, this was broken out from another patch and didn't
set it to be his actual commit same with the other ones. They
were based on his downstream changes and just modified to match
up with what Dave had set the values too. I'll set it to him
for the next revision. Thanks!
^ permalink raw reply
* Re: [PATCH 1/4] dt-bindings: display: bridge: add the Hot-plug MIPI DSI connector
From: Luca Ceresoli @ 2024-04-03 19:33 UTC (permalink / raw)
To: Rob Herring
Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
Jonas Karlman, Jernej Skrabec, David Airlie, Daniel Vetter,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
Krzysztof Kozlowski, Conor Dooley, Paul Kocialkowski,
Hervé Codina, Thomas Petazzoni, dri-devel, devicetree,
linux-kernel, Paul Kocialkowski, Wolfram Sang
In-Reply-To: <20240327160908.GA3460963-robh@kernel.org>
Hello Rob,
[+Cc Wolfram for the I2C discussion below]
thanks for your feedback.
On Wed, 27 Mar 2024 11:09:08 -0500
Rob Herring <robh@kernel.org> wrote:
> On Tue, Mar 26, 2024 at 05:28:11PM +0100, Luca Ceresoli wrote:
> > Add bindings for a physical, hot-pluggable connector allowing the far end
> > of a MIPI DSI bus to be connected and disconnected at runtime.
> >
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > ---
> > .../bridge/hotplug-video-connector-dsi.yaml | 87 ++++++++++++++++++++++
> > MAINTAINERS | 5 ++
> > 2 files changed, 92 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/hotplug-video-connector-dsi.yaml b/Documentation/devicetree/bindings/display/bridge/hotplug-video-connector-dsi.yaml
> > new file mode 100644
> > index 000000000000..05beb8aa9ab4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/hotplug-video-connector-dsi.yaml
> > @@ -0,0 +1,87 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/bridge/hotplug-video-connector-dsi.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Hot-pluggable connector on a MIPI DSI bus
> > +
> > +maintainers:
> > + - Luca Ceresoli <luca.ceresoli@bootlin.com>
> > +
> > +description:
> > + A bridge representing a physical, hot-pluggable connector on a MIPI DSI
> > + video bus. The connector splits the video pipeline in a fixed part and a
> > + removable part.
> > +
> > + The fixed part of the video pipeline includes all components up to the
> > + display controller and 0 or more bridges. The removable part includes one
> > + or more bridges and any other components up to the panel.
> > +
> > + The removable part of the pipeline can be physically disconnected at any
> > + moment, making all of its components not usable anymore. The same or a
> > + different removable part of the pipeline can be reconnected later on.
> > +
> > + Note that the hotplug-video-connector does not describe video busses
> > + having native hotplug capabilities in the hardware, such as HDMI.
> > +
> > +properties:
> > + compatible:
> > + const: hotplug-video-connector-dsi
>
> Got a spec for this connector? How do I know if I have one or not?
>
> The problem here is what else is on this connector? GPIO controls,
> power rails, etc.?
>
> If this is some kind of standard connector, then we need to be able to
> remap everything on the connector not just DSI signals. And for that,
> it's not just DSI signals, so I'd say we would need some sort of generic
> graph remapping that the core graph code handles transparently.
>
> If it is not standard, then you don't need any remapping and can just
> use an overlay that connects the ports directly.
This is not a standardized connector. And it couldn't be: to the best of
my knowledge no standard removable MIPI DSI connector exists at all.
This whole work is unavoidably breaking some long-standing assumptions
and opening some new challenges: giving a proper DT description to
runtime-pluggable hardware and breaking the dogma that a DRM pipeline
is not removable, to name some. So I think it's better to take a step
back and describe the big picture.
As mentioned in the cover letter ("Development roadmap" section),
together with Hervé we are working on a set of patches to allow this
sort of removable hardware to be handled properly by the Linux kernel.
From the cover letter:
> The use case we are working on is to support professional products that
> have a portable "main" part running on battery, with the main SoC and able
> to work autonomously with limited features, and that can be connected to an
> "add-on" part that is not portable and adds more features.
The add-on gets connected via a connector that is not standardized.
There is a well-defined part number for the mechanical connector, but
the pin usage is custom for the product. Connector pins include:
* I2C lines to access various chips on the add-on
- one of these chips is an EEPROM with the add-on product ID
* Some pins to report to the CPU whether the add-on is connected and
add-on power rails are stable (these are wired to SoC GPIOs)
* An interrupt line collecting IRQs from the add-on chips
* MIPI DSI to drive a panel on the add-on
* Gigabit Ethernet (4 pairs)
* USB lines
* A few more accessory pins
Some of these are not problematic: USB is hot-pluggable and
auto-discoverable by nature, Ethernet pins are just connected directly
to the RJ-45 connector so the add-on acts as an Ethernet or USB cable.
For everything else there is going to be a separate patch series
with code to detect add-on connection, read the ID, identify the
appropriate DT overlay and load it, and unload it on disconnection.
Before that, it's good to discuss how the device tree should describe
the whole hardware described above, in these terms:
1. Should device tree describe the connector?
2. If it does, how should the various busses on the connector
(especially I2C and MIPI DSI) be described and associated to the
connector?
To address question 1, I think we _do_ need to describe the connector
itself, because it is a part of the non-discoverable hardware that the
software needs to manage. Among others we need software to know which
GPIOs report the connection and power good statuses, and to know how to
locate the ID EEPROM.
Regarding question 2 I think there are a few open possibilities, and
I must admit the example provided in this series is just too simplistic
to be adequate. Apologies, this series is mostly aiming at discussing
the DRM implementation aspects.
Let me try a more realistic DT description that allows to:
1. associate the various busses to the connector they go through
2. map components of different base board models to the connector pins
3. map components of different add-on models to the connector pins
The 2nd and 3rd items allow decoupling the base and add-on hardware so
that different base board models and different add-on models can be
mixed in all combinations if they use the same connector pinout. This
obviously resembles the Beaglebone capes and RPi hats use case.
My draft proposal is as follows:
---------------------- [my-product.dts] ----------------------
/ {
// [0]
my_hotplug_connector: my-hotplug-connector {
compatible = "vendor-abc,product-xyz-connector";
// [1]
plugged-gpios = <&gpio4 2 GPIO_ACTIVE_LOW>;
powergood-gpios = <&gpio2 4 GPIO_ACTIVE_HIGH>;
// [5]
hotplug_dsi_bus: hotplug-dsi-video-bus@0 {
port@0 {
reg = <0>;
hotplug_dsi_in: endpoint {
remote-endpoint = <&bridge1>;
};
};
port@1 {
reg = <1>;
hotplug_dsi_out: endpoint {
// remote-endpoint to be added by overlay
};
};
};
// [3]
// Map "placeholder" i2cA to "physical" i2c3
i2cA: hotplug-i2c-bus@A {
hotplug-connector,base-bus = <&i2c3>;
};
i2cB: hotplug-i2c-bus@B {
hotplug-connector,base-bus = <&i2c5>;
};
};
};
// Video bridge or display controller on the base board
&bridge1 {
ports {
port@1 {
bridge1_out: endpoint {
remote-endpoint = <&hotplug_dsi_in>;
};
};
};
};
--------------------------------------------------------------
------------------- [my-add-on-common.dtso] ------------------
// [2]
&my_hotplug_connector {
nvmem-cells = <addon_id>;
nvmem-cell-names = "id";
};
// [4]
// i2cA is a "placeholder" name, mapped by the connector
// definition on the base dts to a physical bus on the base board
&i2cA {
eeprom@50 {
compatible = "atmel,24c02";
reg = <0x50>;
nvmem-layout {
compatible = "fixed-layout";
#address-cells = <1>;
#size-cells = <1>;
addon_id: addon-id@10 {
reg = <0x10 0x4>;
};
};
};
};
--------------------------------------------------------------
----------------- [my-add-on-model-xyz.dtso] -----------------
// [6]
&hotplug_dsi_out {
remote-endpoint = <&bridge2_in>; // Fill in the missing piece
};
&{/}
{
panel {
compatible = ...;
ports {
port@0 {
reg = <0>;
panel_in: endpoint {
remote-endpoint = <&bridge2_out>;
};
};
};
};
};
&i2cB {
// Video bridge chip on the add-on
bridge@22 {
// ... compatible, other props/nodes ...
ports {
port@0 {
bridge2_in: endpoint {
remote-endpoint = <&hotplug_dsi_out>;
};
};
port@1 {
bridge2_out: endpoint {
remote-endpoint = <&panel_in>;
};
};
};
};
};
--------------------------------------------------------------
The connector itself [0] is described under the root node because it is
not on any addressable bus, similarly to "sound" and "panel" nodes. I
don't think connectors addressable over I2C or other addressable busses
are going to appear in the forseeable future, but if they did they
could be represented as such in the device tree as well.
The connector GPIOs [1] allow the CPU to know the connection and "power
good" status of the connector. On the implementation side, those are the
inputs triggering overlay loading and unloading.
There is a "common" overlay which describes the common components of
all add-ons at least up to the NVMEM cell [2] holding the add-on ID
that is needed to load the model-specific overlay. This overlay adds
nvmem properties to the hotplug connector node to point to the specific
NVMEM cell. The nvmem-cells and nvmem-cell-names = "id" properties need
to be part of the hotplug-connector bindings.
The "placeholder" hotplug-i2c-bus nodes in the connector node [3] allow
decoupling the I2C bus segment on the base board from the segment on
the add-on. They point to the base bus controller phandle via
hotplug-connector,base-bus, and allow the overlay to use the
"placeholder" [4] instead of naming the "base" I2C bus name. This
allows using the same add-on and overlay on a different base board,
which could even have a totally different SoC, and where the same
connector pins are not wired to the SoC i2c3 but to e.g. i2c4.
In other words:
* [3] means connector pins X and Y are wired to i2c3 on this base board
* [4] means connector pins X and Y are wired to i2cA on this add-on
The specific pins X and Y are not described. Only the bus going through
the connector is.
This can be implemented in the I2C code similarly to a 1-port i2c-mux,
even though it might have an even better ad-hoc implementation that
avoids any unneeded overhead from the mux code, as we are not muxing
anything here, just "connecting wires". Whatever the implementation,
what it needs to do is ensure that i2cA and i2cB appear as regular I2C
busses to their subnodes (e.g. eeprom@50), and when drivers for those
subnodes try to probe they attach to the correct bus (i2c3, i2c5) in
some way -- anything else is an implementation detail. Let's call this
the "I2C decoupler driver".
Once the "common" overlay is loaded, software can read the add-on ID
from NVMEM and find out the overlay that describes the specific add-on
model, see my-add-on-model-xyz.dtso above. This other overlay needs to
be loaded and will populate all the remaining add-on hardware, possibly
using nodes from the first overlay (e.g. a regulator that is used both
by the eeprom@50 and by nodes in the 2nd overlay).
Now to the video bus. Back to the main dts, the hotplug-dsi-video-bus@0
node [5] describes the video pipeline up to the "main" board side of
the connector, represented by port@0. port@1 represents the beginning
of the add-on side of the pipeline, and as such it has no
remote-endpoint because no hardware is present before hotplug and anyway
which hardware will be connected is unknown.
Note DSI is used in the example, but it could as well be LVDS or
parallel video.
The model-specific overlay describes the continuation of the pipeline
up to the panel _and_ fills in the remote-endpoint of &hotplug_dsi_out
to connect the two segments. Just like the i2c example, the overlays
never refer to base board components: it only refers to what appears in
the hotplug connector definition node [0].
An alternative design is that the entire port@1 node is missing from the
hotplug-dsi-video-bus@0 node and is entirely added by the overlay. No
strong opinion on this, the example shows what is tested, but I think
either will be fine.
On the implementation side, the hotplug-dsi-video-bus@0 node does
materialize as the hotplug-bridge driver (patch 4 of this series). The
hotplug-bridge sits in the middle of a previous bridge and the first
bridge in the add-on to let each of them probe normally, and then
passes calls through as transparently as possible.
Still about the implementation, in this example there are 3 main
components that need an implementation:
1. the hotplug-connector driver for /my-hotplug-connector [0]
2. the I2C decoupler driver (possibly based on i2c-mux)
3. the hotplug-bridge driver (this series)
The hotplug-connector driver is responsible for monitoring the
status GPIOs, load the 1st overlay, read the NVMEM ID, load the 2nd
overlay. But it is also in charge of instantiating the "decoupling"
driver for hotplug-i2c-bus nodes [1] and hotplug-dsi-video-bus nodes
[3].
Generalizing the idea to other busses such as SPI should be quite
trivial.
This device tree representation is possibly complicated and verbose.
However I cannot think of a simpler one that can describe a connector
to a removable hardware without losing the ability of decoupling the
base hardware from the add-on hardware.
Your opinion on this will be very appreciated.
Best regards,
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v3 23/25] drivers: media: i2c: imx258: Add support for powerdown gpio
From: Luigi311 @ 2024-04-03 19:34 UTC (permalink / raw)
To: Ondřej Jirman, Sakari Ailus, linux-media, dave.stevenson,
jacopo.mondi, mchehab, robh, krzysztof.kozlowski+dt, conor+dt,
shawnguo, s.hauer, kernel, festevam, devicetree, imx,
linux-arm-kernel, linux-kernel, pavel, phone-devel
In-Reply-To: <wjlcde7yoooygj4hhdmiwrdloh6l4p6i2qbmjek5uwsifyzwgu@xjhutvmsdfou>
On 4/3/24 10:57, Ondřej Jirman wrote:
> Hi Sakari and Luis,
>
> On Wed, Apr 03, 2024 at 04:25:41PM GMT, Sakari Ailus wrote:
>> Hi Luis, Ondrej,
>>
>> On Wed, Apr 03, 2024 at 09:03:52AM -0600, git@luigi311.com wrote:
>>> From: Luis Garcia <git@luigi311.com>
>>>
>>> On some boards powerdown signal needs to be deasserted for this
>>> sensor to be enabled.
>>>
>>> Signed-off-by: Ondrej Jirman <megi@xff.cz>
>>> Signed-off-by: Luis Garcia <git@luigi311.com>
>>> ---
>>> drivers/media/i2c/imx258.c | 13 +++++++++++++
>>> 1 file changed, 13 insertions(+)
>>>
>>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
>>> index 30352c33f63c..163f04f6f954 100644
>>> --- a/drivers/media/i2c/imx258.c
>>> +++ b/drivers/media/i2c/imx258.c
>>> @@ -679,6 +679,8 @@ struct imx258 {
>>> unsigned int lane_mode_idx;
>>> unsigned int csi2_flags;
>>>
>>> + struct gpio_desc *powerdown_gpio;
>>> +
>>> /*
>>> * Mutex for serialized access:
>>> * Protect sensor module set pad format and start/stop streaming safely.
>>> @@ -1213,6 +1215,8 @@ static int imx258_power_on(struct device *dev)
>>> struct imx258 *imx258 = to_imx258(sd);
>>> int ret;
>>>
>>> + gpiod_set_value_cansleep(imx258->powerdown_gpio, 0);
>>
>> What does the spec say? Should this really happen before switching on the
>> supplies below?
>
> There's no powerdown input in the IMX258 manual. The manual only mentions
> that XCLR (reset) should be held low during power on.
>
> https://megous.com/dl/tmp/15b0992a720ab82d.png
>
> https://megous.com/dl/tmp/f2cc991046d97641.png
>
> This sensor doesn’t have a built-in “Power ON Reset” function. The XCLR pin
> is set to “LOW” and the power supplies are brought up. Then the XCLR pin
> should be set to “High” after INCK supplied.
>
> So this input is some feature on camera module itself outside of the
> IMX258 chip, which I think is used to gate power to the module. Eg. on Pinephone
> Pro, there are two modules with shared power rails, so enabling supply to
> one module enables it to the other one, too. So this input becomes the only way
> to really enable/disable power to the chip when both are used at once at some
> point, because regulator_bulk_enable/disable becomes ineffective at that point.
>
> Luis, maybe you saw some other datasheet that mentions this input? IMO,
> it just gates the power rails via some mosfets on the module itself, since
> there's not power down input to the chip itself.
>
> kind regards,
> o.
>
Ondrej, I did not see anything else in the datasheet since I'm pretty sure
I'm looking at the same datasheet as it was supplied to me by Pine64. I'm
not sure what datasheet Dave has access to since he got his for a
completely different module than what we are testing with though.
>>> +
>>> ret = regulator_bulk_enable(IMX258_NUM_SUPPLIES,
>>> imx258->supplies);
>>> if (ret) {
>>> @@ -1224,6 +1228,7 @@ static int imx258_power_on(struct device *dev)
>>> ret = clk_prepare_enable(imx258->clk);
>>> if (ret) {
>>> dev_err(dev, "failed to enable clock\n");
>>> + gpiod_set_value_cansleep(imx258->powerdown_gpio, 1);
>>> regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
>>> }
>>>
>>> @@ -1238,6 +1243,8 @@ static int imx258_power_off(struct device *dev)
>>> clk_disable_unprepare(imx258->clk);
>>> regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
>>>
>>> + gpiod_set_value_cansleep(imx258->powerdown_gpio, 1);
>>> +
>>> return 0;
>>> }
>>>
>>> @@ -1541,6 +1548,12 @@ static int imx258_probe(struct i2c_client *client)
>>> if (!imx258->variant_cfg)
>>> imx258->variant_cfg = &imx258_cfg;
>>>
>>> + /* request optional power down pin */
>>> + imx258->powerdown_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
>>> + GPIOD_OUT_HIGH);
>>> + if (IS_ERR(imx258->powerdown_gpio))
>>> + return PTR_ERR(imx258->powerdown_gpio);
>>> +
>>> /* Initialize subdev */
>>> v4l2_i2c_subdev_init(&imx258->sd, client, &imx258_subdev_ops);
>>>
>>
>> --
>> Regards,
>>
>> Sakari Ailus
^ permalink raw reply
* [PATCH] arm64: dts: imx8m/qxp: Pass the tcpci compatible
From: Fabio Estevam @ 2024-04-03 19:40 UTC (permalink / raw)
To: shawnguo
Cc: robh, krzk+dt, conor+dt, devicetree, linux-arm-kernel,
Fabio Estevam
From: Fabio Estevam <festevam@denx.de>
Per nxp,ptn5110.yaml, also pass the fallback "tcpci" compatible
to fix the following dt-schema warning:
usb-typec@50: compatible: ['nxp,ptn5110'] is too short
from schema $id: http://devicetree.org/schemas/usb/nxp,ptn5110.yaml#
Signed-off-by: Fabio Estevam <festevam@denx.de>
---
arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi | 2 +-
arch/arm64/boot/dts/freescale/imx8mn-evk.dtsi | 2 +-
arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts | 2 +-
arch/arm64/boot/dts/freescale/imx8mq-hummingboard-pulse.dts | 2 +-
arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts | 2 +-
arch/arm64/boot/dts/freescale/imx8qxp-mek.dts | 2 +-
6 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi b/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi
index 888070b8b287..90d1901df2b1 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm-evk.dtsi
@@ -395,7 +395,7 @@ adv7535_out: endpoint {
};
ptn5110: tcpc@50 {
- compatible = "nxp,ptn5110";
+ compatible = "nxp,ptn5110", "tcpci";
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_typec1>;
reg = <0x50>;
diff --git a/arch/arm64/boot/dts/freescale/imx8mn-evk.dtsi b/arch/arm64/boot/dts/freescale/imx8mn-evk.dtsi
index 690da24a4335..9e0259ddf4bc 100644
--- a/arch/arm64/boot/dts/freescale/imx8mn-evk.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mn-evk.dtsi
@@ -244,7 +244,7 @@ adv7535_out: endpoint {
};
ptn5110: tcpc@50 {
- compatible = "nxp,ptn5110";
+ compatible = "nxp,ptn5110", "tcpci";
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_typec1>;
reg = <0x50>;
diff --git a/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts b/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts
index a08057410bde..e5d3901f2913 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts
@@ -340,7 +340,7 @@ pcieclk: clock-generator@68 {
&i2c3 {
/* Connected to USB Hub */
usb-typec@52 {
- compatible = "nxp,ptn5110";
+ compatible = "nxp,ptn5110", "tcpci";
reg = <0x52>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_typec>;
diff --git a/arch/arm64/boot/dts/freescale/imx8mq-hummingboard-pulse.dts b/arch/arm64/boot/dts/freescale/imx8mq-hummingboard-pulse.dts
index 366693f31992..e92b5d5a66b5 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq-hummingboard-pulse.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mq-hummingboard-pulse.dts
@@ -42,7 +42,7 @@ &i2c2 {
status = "okay";
typec_ptn5100: usb-typec@50 {
- compatible = "nxp,ptn5110";
+ compatible = "nxp,ptn5110", "tcpci";
reg = <0x50>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_typec>;
diff --git a/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts
index 8055a2c23035..b268ba7a0e12 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts
@@ -429,7 +429,7 @@ ldo7_reg: LDO7 {
};
typec_ptn5100: usb-typec@52 {
- compatible = "nxp,ptn5110";
+ compatible = "nxp,ptn5110", "tcpci";
reg = <0x52>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_typec>;
diff --git a/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts b/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts
index 8360bb851ac0..83d298c2bfd3 100644
--- a/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts
+++ b/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts
@@ -149,7 +149,7 @@ light-sensor@44 {
};
ptn5110: tcpc@50 {
- compatible = "nxp,ptn5110";
+ compatible = "nxp,ptn5110", "tcpci";
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_typec>;
reg = <0x50>;
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v3 24/25] drivers: media: i2c: imx258: Add support for reset gpio
From: Luigi311 @ 2024-04-03 19:48 UTC (permalink / raw)
To: Ondřej Jirman, Sakari Ailus, linux-media, dave.stevenson,
jacopo.mondi, mchehab, robh, krzysztof.kozlowski+dt, conor+dt,
shawnguo, s.hauer, kernel, festevam, devicetree, imx,
linux-arm-kernel, linux-kernel, pavel, phone-devel
In-Reply-To: <vesqdx7w2sobjnx7tmk6s6i5zplbhsphamoalysx625r4aqffq@hos5otov5ids>
On 4/3/24 11:03, Ondřej Jirman wrote:
> Hi,
>
> On Wed, Apr 03, 2024 at 04:28:59PM GMT, Sakari Ailus wrote:
>> Hi Luis,
>>
>> Could you unify the subject prefix for the driver patches, please? E.g.
>> "media: imx258: " would be fine.
>>
>> On Wed, Apr 03, 2024 at 09:03:53AM -0600, git@luigi311.com wrote:
>>> From: Luis Garcia <git@luigi311.com>
>>>
>>> It was documented in DT, but not implemented.
>>>
>>> Signed-off-by: Ondrej Jirman <megous@megous.com>
>>> Signed-off-by: Luis Garcia <git@luigi311.com>
>>> ---
>>> drivers/media/i2c/imx258.c | 14 +++++++++++++-
>>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
>>> index 163f04f6f954..4c117c4829f1 100644
>>> --- a/drivers/media/i2c/imx258.c
>>> +++ b/drivers/media/i2c/imx258.c
>>> @@ -680,6 +680,7 @@ struct imx258 {
>>> unsigned int csi2_flags;
>>>
>>> struct gpio_desc *powerdown_gpio;
>>> + struct gpio_desc *reset_gpio;
>>>
>>> /*
>>> * Mutex for serialized access:
>>> @@ -1232,7 +1233,11 @@ static int imx258_power_on(struct device *dev)
>>> regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
>>> }
>>>
>>> - return ret;
>>> + gpiod_set_value_cansleep(imx258->reset_gpio, 0);
>>> +
>>> + usleep_range(400, 500);
>>
>> You could mention this at least in the commit message.
>
> This is T6 in the datasheet: https://megous.com/dl/tmp/92c9223ce877216e.png
>
>
>>> +
>>> + return 0;
>>> }
>>>
>>> static int imx258_power_off(struct device *dev)
>>> @@ -1243,6 +1248,7 @@ static int imx258_power_off(struct device *dev)
>>> clk_disable_unprepare(imx258->clk);
>>> regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
>>>
>>> + gpiod_set_value_cansleep(imx258->reset_gpio, 1);
>>
>> Same question than on the other GPIO: does this belong here?
>
> No, this should be before the regulator_bulk_disable.
>
> See: https://megous.com/dl/tmp/c96180b23d7ce63a.png
>
> kind regards,
> o.
>
Since I'm supposed to move the reset up should I also
move the power up with it to match your downstream
driver?
>>> gpiod_set_value_cansleep(imx258->powerdown_gpio, 1);
>>>
>>> return 0;
>>> @@ -1554,6 +1560,12 @@ static int imx258_probe(struct i2c_client *client)
>>> if (IS_ERR(imx258->powerdown_gpio))
>>> return PTR_ERR(imx258->powerdown_gpio);
>>>
>>> + /* request optional reset pin */
>>> + imx258->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
>>> + GPIOD_OUT_HIGH);
>>> + if (IS_ERR(imx258->reset_gpio))
>>> + return PTR_ERR(imx258->reset_gpio);
>>> +
>>> /* Initialize subdev */
>>> v4l2_i2c_subdev_init(&imx258->sd, client, &imx258_subdev_ops);
>>>
>>
>> --
>> Regards,
>>
>> Sakari Ailus
^ permalink raw reply
* [PATCH v2 0/5] Add IAX45 support for RZ/Five SoC
From: Prabhakar @ 2024-04-03 20:34 UTC (permalink / raw)
To: Geert Uytterhoeven, Thomas Gleixner, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Magnus Damm, Paul Walmsley,
Palmer Dabbelt, Albert Ou
Cc: linux-kernel, devicetree, linux-renesas-soc, linux-riscv,
Prabhakar, Lad Prabhakar
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Hi All,
The IAX45 block on RZ/Five SoC is almost identical to the IRQC bock found
on the RZ/G2L family of SoCs.
IAX45 performs various interrupt controls including synchronization for the
external interrupts of NMI, IRQ, and GPIOINT and the interrupts of the
built-in peripheral interrupts output by each module. And it notifies the
interrupt to the PLIC.
- Select 32 TINT from 82 GPIOINT.
- Integration of bus error interrupts from system bus.
- Integration of ECC error interrupts from On-chip RAM.
- Indicate interrupt status. (NMI, IRQ, TINT, integrated bus error interrupt
and integrated ECC error interrupt)
- Setting of interrupt detection method. (NMI, IRQ and TINT)
- All interrupts are masked by INTMASK.
- Mask function for NMI, IRQ and TINT
This patch series adds support for IAX45 in the IRQC driver and enables this
on RZ/Five SoC.
v1: https://patchwork.kernel.org/project/linux-renesas-soc/cover/20240129151618.90922-1-prabhakar.mahadev-lad.rj@bp.renesas.com/
Cheers,
Prabhakar
Lad Prabhakar (5):
dt-bindings: interrupt-controller: renesas,rzg2l-irqc: Document
RZ/Five SoC
irqchip/renesas-rzg2l: Add support for RZ/Five SoC
riscv: dts: renesas: r9a07g043f: Add IRQC node to RZ/Five SoC DTSI
arm64: dts: renesas: r9a07g043: Move interrupt-parent property to
common DTSI
riscv: dts: renesas: rzfive-smarc-som: Drop deleting interrupt
properties from ETH0/1 nodes
.../renesas,rzg2l-irqc.yaml | 17 ++-
arch/arm64/boot/dts/renesas/r9a07g043.dtsi | 1 +
arch/arm64/boot/dts/renesas/r9a07g043u.dtsi | 4 -
arch/riscv/boot/dts/renesas/r9a07g043f.dtsi | 75 ++++++++++
.../boot/dts/renesas/rzfive-smarc-som.dtsi | 16 --
drivers/irqchip/irq-renesas-rzg2l.c | 137 +++++++++++++++++-
6 files changed, 218 insertions(+), 32 deletions(-)
--
2.34.1
^ permalink raw reply
* [PATCH v2 1/5] dt-bindings: interrupt-controller: renesas,rzg2l-irqc: Document RZ/Five SoC
From: Prabhakar @ 2024-04-03 20:34 UTC (permalink / raw)
To: Geert Uytterhoeven, Thomas Gleixner, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Magnus Damm, Paul Walmsley,
Palmer Dabbelt, Albert Ou
Cc: linux-kernel, devicetree, linux-renesas-soc, linux-riscv,
Prabhakar, Lad Prabhakar
In-Reply-To: <20240403203503.634465-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Document RZ/Five (R9A07G043F) IRQC bindings. The IRQC block on the RZ/Five
SoC is almost identical to the one found on the RZ/G2L SoC, with the only
difference being that it has additional mask control registers for
NMI/IRQ/TINT.
Hence new compatible string "renesas,r9a07g043f-irqc" is added for RZ/Five
SoC.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v1->v2
- Dropped the checks for interrupts as its already handled
- Added SoC specific compat string
---
.../renesas,rzg2l-irqc.yaml | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
index daef4ee06f4e..2a871cbf6f87 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
@@ -21,13 +21,16 @@ description: |
properties:
compatible:
- items:
- - enum:
- - renesas,r9a07g043u-irqc # RZ/G2UL
- - renesas,r9a07g044-irqc # RZ/G2{L,LC}
- - renesas,r9a07g054-irqc # RZ/V2L
- - renesas,r9a08g045-irqc # RZ/G3S
- - const: renesas,rzg2l-irqc
+ oneOf:
+ - items:
+ - enum:
+ - renesas,r9a07g043u-irqc # RZ/G2UL
+ - renesas,r9a07g044-irqc # RZ/G2{L,LC}
+ - renesas,r9a07g054-irqc # RZ/V2L
+ - renesas,r9a08g045-irqc # RZ/G3S
+ - const: renesas,rzg2l-irqc
+ - items:
+ - const: renesas,r9a07g043f-irqc # RZ/Five
'#interrupt-cells':
description: The first cell should contain a macro RZG2L_{NMI,IRQX} included in the
--
2.34.1
^ permalink raw reply related
* [PATCH v2 2/5] irqchip/renesas-rzg2l: Add support for RZ/Five SoC
From: Prabhakar @ 2024-04-03 20:35 UTC (permalink / raw)
To: Geert Uytterhoeven, Thomas Gleixner, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Magnus Damm, Paul Walmsley,
Palmer Dabbelt, Albert Ou
Cc: linux-kernel, devicetree, linux-renesas-soc, linux-riscv,
Prabhakar, Lad Prabhakar
In-Reply-To: <20240403203503.634465-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
The IX45 block has additional mask registers (NMSK/IMSK/TMSK) as compared
to the RZ/G2L (family) SoC.
Introduce masking/unmasking support for IRQ and TINT interrupts in IRQC
controller driver. Two new registers, IMSK and TMSK, are defined to
handle masking on RZ/Five SoC. The implementation utilizes a new data
structure, `struct rzg2l_irqc_data`, to determine mask support for a
specific controller instance.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v1->v2
- Added IRQCHIP_MATCH() for RZ/Five
- Retaining a copy of OF data in priv
- Rebased the changes
---
drivers/irqchip/irq-renesas-rzg2l.c | 137 +++++++++++++++++++++++++++-
1 file changed, 132 insertions(+), 5 deletions(-)
diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
index f6484bf15e0b..6fa8d65605dc 100644
--- a/drivers/irqchip/irq-renesas-rzg2l.c
+++ b/drivers/irqchip/irq-renesas-rzg2l.c
@@ -37,6 +37,8 @@
#define TSSEL_SHIFT(n) (8 * (n))
#define TSSEL_MASK GENMASK(7, 0)
#define IRQ_MASK 0x3
+#define IMSK 0x10010
+#define TMSK 0x10020
#define TSSR_OFFSET(n) ((n) % 4)
#define TSSR_INDEX(n) ((n) / 4)
@@ -66,15 +68,25 @@ struct rzg2l_irqc_reg_cache {
u32 titsr[2];
};
+/**
+ * struct rzg2l_irqc_of_data - OF data structure
+ * @mask_supported: Indicates if mask registers are available
+ */
+struct rzg2l_irqc_of_data {
+ bool mask_supported;
+};
+
/**
* struct rzg2l_irqc_priv - IRQ controller private data structure
* @base: Controller's base address
+ * @data: OF data pointer
* @fwspec: IRQ firmware specific data
* @lock: Lock to serialize access to hardware registers
* @cache: Registers cache for suspend/resume
*/
static struct rzg2l_irqc_priv {
void __iomem *base;
+ const struct rzg2l_irqc_of_data *data;
struct irq_fwspec fwspec[IRQC_NUM_IRQ];
raw_spinlock_t lock;
struct rzg2l_irqc_reg_cache cache;
@@ -138,18 +150,102 @@ static void rzg2l_irqc_eoi(struct irq_data *d)
irq_chip_eoi_parent(d);
}
+static void rzg2l_irqc_mask_irq_interrupt(struct rzg2l_irqc_priv *priv,
+ unsigned int hwirq)
+{
+ u32 imsk = readl_relaxed(priv->base + IMSK);
+ u32 bit = BIT(hwirq - IRQC_IRQ_START);
+
+ writel_relaxed(imsk | bit, priv->base + IMSK);
+}
+
+static void rzg2l_irqc_unmask_irq_interrupt(struct rzg2l_irqc_priv *priv,
+ unsigned int hwirq)
+{
+ u32 imsk = readl_relaxed(priv->base + IMSK);
+ u32 bit = BIT(hwirq - IRQC_IRQ_START);
+
+ writel_relaxed(imsk & ~bit, priv->base + IMSK);
+}
+
+static void rzg2l_irqc_mask_tint_interrupt(struct rzg2l_irqc_priv *priv,
+ unsigned int hwirq)
+{
+ u32 tmsk = readl_relaxed(priv->base + TMSK);
+ u32 bit = BIT(hwirq - IRQC_TINT_START);
+
+ writel_relaxed(tmsk | bit, priv->base + TMSK);
+}
+
+static void rzg2l_irqc_unmask_tint_interrupt(struct rzg2l_irqc_priv *priv,
+ unsigned int hwirq)
+{
+ u32 tmsk = readl_relaxed(priv->base + TMSK);
+ u32 bit = BIT(hwirq - IRQC_TINT_START);
+
+ writel_relaxed(tmsk & ~bit, priv->base + TMSK);
+}
+
+/* Must be called while priv->lock is held */
+static void rzg2l_irqc_mask_once(struct rzg2l_irqc_priv *priv, unsigned int hwirq)
+{
+ if (!priv->data->mask_supported)
+ return;
+
+ if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT)
+ rzg2l_irqc_mask_irq_interrupt(priv, hwirq);
+ else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ)
+ rzg2l_irqc_mask_tint_interrupt(priv, hwirq);
+}
+
+static void rzg2l_irqc_mask(struct irq_data *d)
+{
+ struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
+
+ raw_spin_lock(&priv->lock);
+ rzg2l_irqc_mask_once(priv, irqd_to_hwirq(d));
+ raw_spin_unlock(&priv->lock);
+ irq_chip_mask_parent(d);
+}
+
+/* Must be called while priv->lock is held */
+static void rzg2l_irqc_unmask_once(struct rzg2l_irqc_priv *priv, unsigned int hwirq)
+{
+ if (!priv->data->mask_supported)
+ return;
+
+ if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT)
+ rzg2l_irqc_unmask_irq_interrupt(priv, hwirq);
+ else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ)
+ rzg2l_irqc_unmask_tint_interrupt(priv, hwirq);
+}
+
+static void rzg2l_irqc_unmask(struct irq_data *d)
+{
+ struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
+
+ raw_spin_lock(&priv->lock);
+ rzg2l_irqc_unmask_once(priv, irqd_to_hwirq(d));
+ raw_spin_unlock(&priv->lock);
+ irq_chip_unmask_parent(d);
+}
+
static void rzg2l_tint_irq_endisable(struct irq_data *d, bool enable)
{
+ struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
unsigned int hw_irq = irqd_to_hwirq(d);
if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) {
- struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
u32 offset = hw_irq - IRQC_TINT_START;
u32 tssr_offset = TSSR_OFFSET(offset);
u8 tssr_index = TSSR_INDEX(offset);
u32 reg;
raw_spin_lock(&priv->lock);
+ if (enable)
+ rzg2l_irqc_unmask_once(priv, hw_irq);
+ else
+ rzg2l_irqc_mask_once(priv, hw_irq);
reg = readl_relaxed(priv->base + TSSR(tssr_index));
if (enable)
reg |= TIEN << TSSEL_SHIFT(tssr_offset);
@@ -157,6 +253,13 @@ static void rzg2l_tint_irq_endisable(struct irq_data *d, bool enable)
reg &= ~(TIEN << TSSEL_SHIFT(tssr_offset));
writel_relaxed(reg, priv->base + TSSR(tssr_index));
raw_spin_unlock(&priv->lock);
+ } else {
+ raw_spin_lock(&priv->lock);
+ if (enable)
+ rzg2l_irqc_unmask_once(priv, hw_irq);
+ else
+ rzg2l_irqc_mask_once(priv, hw_irq);
+ raw_spin_unlock(&priv->lock);
}
}
@@ -324,8 +427,8 @@ static struct syscore_ops rzg2l_irqc_syscore_ops = {
static const struct irq_chip irqc_chip = {
.name = "rzg2l-irqc",
.irq_eoi = rzg2l_irqc_eoi,
- .irq_mask = irq_chip_mask_parent,
- .irq_unmask = irq_chip_unmask_parent,
+ .irq_mask = rzg2l_irqc_mask,
+ .irq_unmask = rzg2l_irqc_unmask,
.irq_disable = rzg2l_irqc_irq_disable,
.irq_enable = rzg2l_irqc_irq_enable,
.irq_get_irqchip_state = irq_chip_get_parent_state,
@@ -401,7 +504,16 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv,
return 0;
}
-static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
+static const struct rzg2l_irqc_of_data rzg2l_irqc_mask_supported_data = {
+ .mask_supported = true,
+};
+
+static const struct rzg2l_irqc_of_data rzg2l_irqc_default_data = {
+ .mask_supported = false,
+};
+
+static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent,
+ const struct rzg2l_irqc_of_data *of_data)
{
struct irq_domain *irq_domain, *parent_domain;
struct platform_device *pdev;
@@ -422,6 +534,8 @@ static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
if (!rzg2l_irqc_data)
return -ENOMEM;
+ rzg2l_irqc_data->data = of_data;
+
rzg2l_irqc_data->base = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL);
if (IS_ERR(rzg2l_irqc_data->base))
return PTR_ERR(rzg2l_irqc_data->base);
@@ -472,8 +586,21 @@ static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
return ret;
}
+static int __init rzg2l_irqc_default_init(struct device_node *node,
+ struct device_node *parent)
+{
+ return rzg2l_irqc_init(node, parent, &rzg2l_irqc_default_data);
+}
+
+static int __init rzg2l_irqc_mask_supported_init(struct device_node *node,
+ struct device_node *parent)
+{
+ return rzg2l_irqc_init(node, parent, &rzg2l_irqc_mask_supported_data);
+}
+
IRQCHIP_PLATFORM_DRIVER_BEGIN(rzg2l_irqc)
-IRQCHIP_MATCH("renesas,rzg2l-irqc", rzg2l_irqc_init)
+IRQCHIP_MATCH("renesas,rzg2l-irqc", rzg2l_irqc_default_init)
+IRQCHIP_MATCH("renesas,r9a07g043f-irqc", rzg2l_irqc_mask_supported_init)
IRQCHIP_PLATFORM_DRIVER_END(rzg2l_irqc)
MODULE_AUTHOR("Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>");
MODULE_DESCRIPTION("Renesas RZ/G2L IRQC Driver");
--
2.34.1
^ permalink raw reply related
* [PATCH v2 3/5] riscv: dts: renesas: r9a07g043f: Add IRQC node to RZ/Five SoC DTSI
From: Prabhakar @ 2024-04-03 20:35 UTC (permalink / raw)
To: Geert Uytterhoeven, Thomas Gleixner, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Magnus Damm, Paul Walmsley,
Palmer Dabbelt, Albert Ou
Cc: linux-kernel, devicetree, linux-renesas-soc, linux-riscv,
Prabhakar, Lad Prabhakar
In-Reply-To: <20240403203503.634465-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Add the IRQC node to RZ/Five (R9A07G043F) SoC DTSI.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v1->v2
- Dropped using SOC_PERIPHERAL_IRQ() macro
---
arch/riscv/boot/dts/renesas/r9a07g043f.dtsi | 75 +++++++++++++++++++++
1 file changed, 75 insertions(+)
diff --git a/arch/riscv/boot/dts/renesas/r9a07g043f.dtsi b/arch/riscv/boot/dts/renesas/r9a07g043f.dtsi
index f35324b9173c..e0ddf8f602c7 100644
--- a/arch/riscv/boot/dts/renesas/r9a07g043f.dtsi
+++ b/arch/riscv/boot/dts/renesas/r9a07g043f.dtsi
@@ -54,6 +54,81 @@ &soc {
dma-noncoherent;
interrupt-parent = <&plic>;
+ irqc: interrupt-controller@110a0000 {
+ compatible = "renesas,r9a07g043f-irqc";
+ reg = <0 0x110a0000 0 0x20000>;
+ #interrupt-cells = <2>;
+ #address-cells = <0>;
+ interrupt-controller;
+ interrupts = <32 IRQ_TYPE_LEVEL_HIGH>,
+ <33 IRQ_TYPE_LEVEL_HIGH>,
+ <34 IRQ_TYPE_LEVEL_HIGH>,
+ <35 IRQ_TYPE_LEVEL_HIGH>,
+ <36 IRQ_TYPE_LEVEL_HIGH>,
+ <37 IRQ_TYPE_LEVEL_HIGH>,
+ <38 IRQ_TYPE_LEVEL_HIGH>,
+ <39 IRQ_TYPE_LEVEL_HIGH>,
+ <40 IRQ_TYPE_LEVEL_HIGH>,
+ <476 IRQ_TYPE_LEVEL_HIGH>,
+ <477 IRQ_TYPE_LEVEL_HIGH>,
+ <478 IRQ_TYPE_LEVEL_HIGH>,
+ <479 IRQ_TYPE_LEVEL_HIGH>,
+ <480 IRQ_TYPE_LEVEL_HIGH>,
+ <481 IRQ_TYPE_LEVEL_HIGH>,
+ <482 IRQ_TYPE_LEVEL_HIGH>,
+ <483 IRQ_TYPE_LEVEL_HIGH>,
+ <484 IRQ_TYPE_LEVEL_HIGH>,
+ <485 IRQ_TYPE_LEVEL_HIGH>,
+ <486 IRQ_TYPE_LEVEL_HIGH>,
+ <487 IRQ_TYPE_LEVEL_HIGH>,
+ <488 IRQ_TYPE_LEVEL_HIGH>,
+ <489 IRQ_TYPE_LEVEL_HIGH>,
+ <490 IRQ_TYPE_LEVEL_HIGH>,
+ <491 IRQ_TYPE_LEVEL_HIGH>,
+ <492 IRQ_TYPE_LEVEL_HIGH>,
+ <493 IRQ_TYPE_LEVEL_HIGH>,
+ <494 IRQ_TYPE_LEVEL_HIGH>,
+ <495 IRQ_TYPE_LEVEL_HIGH>,
+ <496 IRQ_TYPE_LEVEL_HIGH>,
+ <497 IRQ_TYPE_LEVEL_HIGH>,
+ <498 IRQ_TYPE_LEVEL_HIGH>,
+ <499 IRQ_TYPE_LEVEL_HIGH>,
+ <500 IRQ_TYPE_LEVEL_HIGH>,
+ <501 IRQ_TYPE_LEVEL_HIGH>,
+ <502 IRQ_TYPE_LEVEL_HIGH>,
+ <503 IRQ_TYPE_LEVEL_HIGH>,
+ <504 IRQ_TYPE_LEVEL_HIGH>,
+ <505 IRQ_TYPE_LEVEL_HIGH>,
+ <506 IRQ_TYPE_LEVEL_HIGH>,
+ <507 IRQ_TYPE_LEVEL_HIGH>,
+ <57 IRQ_TYPE_LEVEL_HIGH>,
+ <66 IRQ_TYPE_EDGE_RISING>,
+ <67 IRQ_TYPE_EDGE_RISING>,
+ <68 IRQ_TYPE_EDGE_RISING>,
+ <69 IRQ_TYPE_EDGE_RISING>,
+ <70 IRQ_TYPE_EDGE_RISING>,
+ <71 IRQ_TYPE_EDGE_RISING>;
+ interrupt-names = "nmi",
+ "irq0", "irq1", "irq2", "irq3",
+ "irq4", "irq5", "irq6", "irq7",
+ "tint0", "tint1", "tint2", "tint3",
+ "tint4", "tint5", "tint6", "tint7",
+ "tint8", "tint9", "tint10", "tint11",
+ "tint12", "tint13", "tint14", "tint15",
+ "tint16", "tint17", "tint18", "tint19",
+ "tint20", "tint21", "tint22", "tint23",
+ "tint24", "tint25", "tint26", "tint27",
+ "tint28", "tint29", "tint30", "tint31",
+ "bus-err", "ec7tie1-0", "ec7tie2-0",
+ "ec7tiovf-0", "ec7tie1-1", "ec7tie2-1",
+ "ec7tiovf-1";
+ clocks = <&cpg CPG_MOD R9A07G043_IAX45_CLK>,
+ <&cpg CPG_MOD R9A07G043_IAX45_PCLK>;
+ clock-names = "clk", "pclk";
+ power-domains = <&cpg>;
+ resets = <&cpg R9A07G043_IAX45_RESETN>;
+ };
+
plic: interrupt-controller@12c00000 {
compatible = "renesas,r9a07g043-plic", "andestech,nceplic100";
#interrupt-cells = <2>;
--
2.34.1
^ permalink raw reply related
* [PATCH v2 4/5] arm64: dts: renesas: r9a07g043: Move interrupt-parent property to common DTSI
From: Prabhakar @ 2024-04-03 20:35 UTC (permalink / raw)
To: Geert Uytterhoeven, Thomas Gleixner, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Magnus Damm, Paul Walmsley,
Palmer Dabbelt, Albert Ou
Cc: linux-kernel, devicetree, linux-renesas-soc, linux-riscv,
Prabhakar, Lad Prabhakar
In-Reply-To: <20240403203503.634465-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Now that we have added support for IRQC to both RZ/Five and RZ/G2UL SoCs
we can move the interrupt-parent for pinctrl node back to the common
shared r9a07g043.dtsi file.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v1->v2
- Included RB tag from Geert
---
arch/arm64/boot/dts/renesas/r9a07g043.dtsi | 1 +
arch/arm64/boot/dts/renesas/r9a07g043u.dtsi | 4 ----
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
index 766c54b91acc..6212ee550f33 100644
--- a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi
@@ -598,6 +598,7 @@ pinctrl: pinctrl@11030000 {
gpio-ranges = <&pinctrl 0 0 152>;
#interrupt-cells = <2>;
interrupt-controller;
+ interrupt-parent = <&irqc>;
clocks = <&cpg CPG_MOD R9A07G043_GPIO_HCLK>;
power-domains = <&cpg>;
resets = <&cpg R9A07G043_GPIO_RSTN>,
diff --git a/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi b/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi
index 964b0a475eee..165bfcfef3bc 100644
--- a/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a07g043u.dtsi
@@ -54,10 +54,6 @@ timer {
};
};
-&pinctrl {
- interrupt-parent = <&irqc>;
-};
-
&soc {
interrupt-parent = <&gic>;
--
2.34.1
^ permalink raw reply related
* [PATCH v2 5/5] riscv: dts: renesas: rzfive-smarc-som: Drop deleting interrupt properties from ETH0/1 nodes
From: Prabhakar @ 2024-04-03 20:35 UTC (permalink / raw)
To: Geert Uytterhoeven, Thomas Gleixner, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Magnus Damm, Paul Walmsley,
Palmer Dabbelt, Albert Ou
Cc: linux-kernel, devicetree, linux-renesas-soc, linux-riscv,
Prabhakar, Lad Prabhakar
In-Reply-To: <20240403203503.634465-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Now that we have enabled IRQC support for RZ/Five SoC switch to interrupt
mode for ethernet0/1 PHYs instead of polling mode.
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v1->v2
- Included RB tag from Geert
---
.../riscv/boot/dts/renesas/rzfive-smarc-som.dtsi | 16 ----------------
1 file changed, 16 deletions(-)
diff --git a/arch/riscv/boot/dts/renesas/rzfive-smarc-som.dtsi b/arch/riscv/boot/dts/renesas/rzfive-smarc-som.dtsi
index 72d9b6fba526..86b2f15375ec 100644
--- a/arch/riscv/boot/dts/renesas/rzfive-smarc-som.dtsi
+++ b/arch/riscv/boot/dts/renesas/rzfive-smarc-som.dtsi
@@ -7,22 +7,6 @@
#include <arm64/renesas/rzg2ul-smarc-som.dtsi>
-#if (!SW_ET0_EN_N)
-ð0 {
- phy0: ethernet-phy@7 {
- /delete-property/ interrupt-parent;
- /delete-property/ interrupts;
- };
-};
-#endif
-
-ð1 {
- phy1: ethernet-phy@7 {
- /delete-property/ interrupt-parent;
- /delete-property/ interrupts;
- };
-};
-
&sbc {
status = "disabled";
};
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v3 0/6] Add Synopsys DesignWare HDMI RX Controller
From: Deborah Brouwer @ 2024-04-03 21:13 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Shreeya Patel, mchehab, hverkuil, hverkuil-cisco, heiko, robh,
krzysztof.kozlowski+dt, conor+dt, mturquette, sboyd, p.zabel,
shawn.wen, kernel, linux-kernel, linux-media, devicetree,
linux-arm-kernel, linux-rockchip, linux-clk, linux-arm
In-Reply-To: <86150c89-11d5-4d52-987e-974b1a03018f@linaro.org>
On Wed, Apr 03, 2024 at 01:24:05PM +0200, Krzysztof Kozlowski wrote:
> On 03/04/2024 13:20, Shreeya Patel wrote:
> > On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >
> >> On 03/04/2024 11:24, Shreeya Patel wrote:
> >>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote:
> >>>
> >>>> This series implements support for the Synopsys DesignWare
> >>>> HDMI RX Controller, being compliant with standard HDMI 1.4b
> >>>> and HDMI 2.0.
> >>>>
> >>>
> >>> Hi Mauro and Hans,
> >>>
> >>> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series.
> >>
> >> Why did you put clk changes here? These go via different subsystem. That
> >> might be one of obstacles for your patchset.
> >>
> >
> > I added clock changes in this patch series because HDMIRX driver depends on it.
> > I thought it is wrong to send the driver patches which don't even compile?
>
> Hm, why HDMIRX driver depends on clock? How? This sounds really wrong.
> Please get it reviewed internally first.
>
> >
> > Since you are a more experienced developer, can you help me understand what would
> > be the right way to send patches in such scenarios?
>
> I am not the substitute for your Collabora engineers and peers. You do
> not get free work from the community. First, do the work and review
> internally, to solve all trivial things, like how to submit patches
> upstream or how to make your driver buildable, and then ask community
> for the review.
I don't think Shreeya was asking for "free" work from the community.
Her question wasn't trivial or obvious since reasonable people seem to sometimes
disagree about where to send a patch especially if it's needed to make a series compile.
I heard the issue was already resolved but had to say something since this accusation
seemed so unfair.
>
> Best regards,
> Krzysztof
>
>
^ permalink raw reply
* [PATCH v9 0/4] PCI: brcmstb: Configure appropriate HW CLKREQ# mode
From: Jim Quinlan @ 2024-04-03 21:38 UTC (permalink / raw)
To: linux-pci, Nicolas Saenz Julienne, Bjorn Helgaas,
Lorenzo Pieralisi, Cyril Brulebois, Phil Elwell,
bcm-kernel-feedback-list, james.quinlan
Cc: Conor Dooley,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Florian Fainelli, Jim Quinlan, Krzysztof Kozlowski,
Krzysztof Wilczyński,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Lorenzo Pieralisi, Rob Herring
[-- Attachment #1: Type: text/plain, Size: 5702 bytes --]
v9 -- v8 was setting an internal bus timeout to accomodate large L1 exit
latencies. After meeting the PCIe HW team it was revealed that the
HW default timeout value was set low for the purposes of HW debugging
convenience; for nominal operation it needs to be set to a higher
value independent of this submission's purpose. This is now a
separate commit.
-- With v8, Bjorne asked what was preventing a device from exceeding the
time required for the above internal bus timeout. The answer to this
is for us to set the endpoints' max latency {no-,}snoop value to
something below this internal timeout value. If the endpoint
respects this value as it should, it will not send an LTR request
with a larger latency value and not put itself in a situation
that requires more latency than is possible for the platform.
Typically, ACPI or FW sets these max latency values. In most of our
systems we do not have this happening so it is up to the RC driver to
set these values in the endpoint devices. If the endpoints already
have non-zero values that are lower than what we are setting, we let
them be, as it is possible ACPI or FW set them and knows something
that we do not.
-- The "clkreq" commit has only been changed to remove the code that was
setting the timeout value, as this code is now its own commit.
v8 -- Un-advertise L1SS capability when in "no-l1ss" mode (Bjorn)
-- Squashed last two commits of v7 (Bjorn)
-- Fix DT binding description text wrapping (Bjorn)
-- Fix incorrect Spec reference (Bjorn)
s/PCIe Spec/PCIe Express Mini CEM 2.1 specification/
-- Text substitutions (Bjorn)
s/WRT/With respect to/
s/Tclron/T_CLRon/
v7 -- Manivannan Sadhasivam suggested (a) making the property look like a
network phy-mode and (b) keeping the code simple (not counting clkreq
signal appearances, un-advertising capabilites, etc). This is
what I have done. The property is now "brcm,clkreq-mode" and
the values may be one of "safe", "default", and "no-l1ss". The
default setting is to employ the most capable power savings mode.
v6 -- No code has been changed.
-- Changed commit subject and comment in "#PERST" commit (Bjorn, Cyril)
-- Changed sign-off and author email address for all commits.
This was due to a change in Broadcom's upstreaming policy.
v5 -- Remove DT property "brcm,completion-timeout-us" from
"DT bindings" commit. Although this error may be reported
as a completion timeout, its cause was traced to an
internal bus timeout which may occur even when there is
no PCIe access being processed. We set a timeout of four
seconds only if we are operating in "L1SS CLKREQ#" mode.
-- Correct CEM 2.0 reference provided by HW engineer,
s/3.2.5.2.5/3.2.5.2.2/ (Bjorn)
-- Add newline to dev_info() string (Stefan)
-- Change variable rval to unsigned (Stefan)
-- s/implementaion/implementation/ (Bjorn)
-- s/superpowersave/powersupersave/ (Bjorn)
-- Slightly modify message on "PERST#" commit.
-- Rebase to torvalds master
v4 -- New commit that asserts PERST# for 2711/RPi SOCs at PCIe RC
driver probe() time. This is done in Raspian Linux and its
absence may be the cause of a failing test case.
-- New commit that removes stale comment.
v3 -- Rewrote commit msgs and comments refering panics if L1SS
is enabled/disabled; the code snippet that unadvertises L1SS
eliminates the panic scenario. (Bjorn)
-- Add reference for "400ns of CLKREQ# assertion" blurb (Bjorn)
-- Put binding names in DT commit Subject (Bjorn)
-- Add a verb to a commit's subject line (Bjorn)
-- s/accomodat(\w+)/accommodat$1/g (Bjorn)
-- Rewrote commit msgs and comments refering panics if L1SS
is enabled/disabled; the code snippet that unadvertises L1SS
eliminates the panic scenario. (Bjorn)
v2 -- Changed binding property 'brcm,completion-timeout-msec' to
'brcm,completion-timeout-us'. (StefanW for standard suffix).
-- Warn when clamping timeout value, and include clamped
region in message. Also add min and max in YAML. (StefanW)
-- Qualify description of "brcm,completion-timeout-us" so that
it refers to PCIe transactions. (StefanW)
-- Remvove mention of Linux specifics in binding description. (StefanW)
-- s/clkreq#/CLKREQ#/g (Bjorn)
-- Refactor completion-timeout-us code to compare max and min to
value given by the property (as opposed to the computed value).
v1 -- The current driver assumes the downstream devices can
provide CLKREQ# for ASPM. These commits accomodate devices
w/ or w/o clkreq# and also handle L1SS-capable devices.
-- The Raspian Linux folks have already been using a PCIe RC
property "brcm,enable-l1ss". These commits use the same
property, in a backward-compatible manner, and the implementaion
adds more detail and also automatically identifies devices w/o
a clkreq# signal, i.e. most devices plugged into an RPi CM4
IO board.
Jim Quinlan (4):
dt-bindings: PCI: brcmstb: Add property "brcm,clkreq-mode"
PCI: brcmstb: Set reasonable value for internal bus timeout
PCI: brcmstb: Set downstream maximum {no-}snoop LTR values
PCI: brcmstb: Configure HW CLKREQ# mode appropriate for downstream
device
.../bindings/pci/brcm,stb-pcie.yaml | 18 ++
drivers/pci/controller/pcie-brcmstb.c | 161 +++++++++++++++++-
2 files changed, 170 insertions(+), 9 deletions(-)
base-commit: 9f8413c4a66f2fb776d3dc3c9ed20bf435eb305e
--
2.17.1
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]
^ permalink raw reply
* [PATCH v9 1/4] dt-bindings: PCI: brcmstb: Add property "brcm,clkreq-mode"
From: Jim Quinlan @ 2024-04-03 21:38 UTC (permalink / raw)
To: linux-pci, Nicolas Saenz Julienne, Bjorn Helgaas,
Lorenzo Pieralisi, Cyril Brulebois, Phil Elwell,
bcm-kernel-feedback-list, james.quinlan
Cc: Jim Quinlan, Florian Fainelli, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
In-Reply-To: <20240403213902.26391-1-james.quinlan@broadcom.com>
[-- Attachment #1: Type: text/plain, Size: 2171 bytes --]
The Broadcom STB/CM PCIe HW -- a core that is also used by RPi SOCs --
requires the driver to deliberately place the RC HW one of three CLKREQ#
modes. The "brcm,clkreq-mode" property allows the user to override the
default setting. If this property is omitted, the default mode shall be
"default".
Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
.../devicetree/bindings/pci/brcm,stb-pcie.yaml | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 7e15aae7d69e..22491f7f8852 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -64,6 +64,24 @@ properties:
aspm-no-l0s: true
+ brcm,clkreq-mode:
+ description: A string that determines the operating
+ clkreq mode of the PCIe RC HW with respect to controlling the refclk
+ signal. There are three different modes -- "safe", which drives the
+ refclk signal unconditionally and will work for all devices but does
+ not provide any power savings; "no-l1ss" -- which provides Clock
+ Power Management, L0s, and L1, but cannot provide L1 substate (L1SS)
+ power savings. If the downstream device connected to the RC is L1SS
+ capable AND the OS enables L1SS, all PCIe traffic may abruptly halt,
+ potentially hanging the system; "default" -- which provides L0s, L1,
+ and L1SS, but not compliant to provide Clock Power Management;
+ specifically, may not be able to meet the T_CLRon max timing of 400ns
+ as specified in "Dynamic Clock Control", section 3.2.5.2.2 PCI
+ Express Mini CEM 2.1 specification. This situation is atypical and
+ should happen only with older devices.
+ $ref: /schemas/types.yaml#/definitions/string
+ enum: [ safe, no-l1ss, default ]
+
brcm,scb-sizes:
description: u64 giving the 64bit PCIe memory
viewport size of a memory controller. There may be up to
--
2.17.1
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]
^ permalink raw reply related
* Re: [PATCH net-next v2 0/9] Add support for OPEN Alliance 10BASE-T1x MACPHY Serial Interface
From: Benjamin Bigler @ 2024-04-03 21:40 UTC (permalink / raw)
To: Parthiban.Veerasooran
Cc: netdev, devicetree, linux-kernel, linux-doc, Horatiu.Vultur,
Woojung.Huh, Nicolas.Ferre, UNGLinuxDriver, Thorsten.Kummermehr,
davem, edumazet, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
conor+dt, corbet, Steen.Hegelund, rdunlap, horms, casper.casan,
andrew
In-Reply-To: <0596fce8-223b-494e-907e-f13d75f211cd@microchip.com>
Hi Parthiban,
Sorry for the late answer, I was quite busy the last few days.
On Mon, 2024-03-25 at 13:24 +0000, Parthiban.Veerasooran@microchip.com wrote:
> Hi Benjamin Bigler,
>
> Thank you for your testing and feedback. It would be really helpful to
> bring the driver to a good shape. We really appreciate your efforts on this.
>
> On 24/03/24 5:25 pm, Benjamin Bigler wrote:
> > [Some people who received this message don't often get email from benjamin@bigler.one. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > Hi Parthiban
> >
> > I hope I send this in the right context as it is not related to just one patch or
> > some specific code.
> >
> > I conducted UDP load testing using three i.MX8MM boards in conjunction with the
> > LAN8651. The setup involved one board functioning as a server, which is just
> > echoing back received data, while the remaining two boards acted as clients,
> > sending UDP packets of different sizes in various bursts to the server.
> > Due to hardware constraints, the SPI bus speed was limited to 15 MHz, which might
> > have influenced the results.
> >
> > During the tests I experienced some issues:
> >
> > - The boards just start receiving after first sending something (ping another board).
> > Some measurements showed that the irq stays asserted after init. This makes sense
> > as far as I understand the chapter 7.7 of the specification, the irq is deasserted
> > on reception of the first data header following CSn being asserted. As a workaround
> > I trigger the thread at the end of oa_tc6_init.
> It looks like the IRQ is asserted on RESET completion and expects a data
> chunk from host to deassert the IRQ. I used to test the driver in RPI 4
> using iperf3. For some reason I never faced this issue, may be when the
> network device is being registered there might be some packet
> transmission which leads to deliver a data chunk so that the IRQ is
> deasserted. Thanks for the workaround. I think that would be the
> solution to solve this issue. Adding the below lines in the end of the
> function oa_tc6_init() will trigger the oa_tc6_spi_thread_handler() to
> perform an empty data chunk transfer which will deassert the IRQ before
> starting the actual data transfer.
I have ipv6 disabled and use static ipv4 addresses. That could be the reason why on
my side no packet is sent.
>
> /* oa_tc6_sw_reset_macphy() function resets and clears the MAC-PHY reset
> * complete status. IRQ is also asserted on reset completion and it is
> * remain asserted until MAC-PHY receives a data chunk. So performing an
> * empty data chunk transmission will deassert the IRQ. Refer section
> * 7.7 and 9.2.8.8 in the OPEN Alliance specification for more details.
> */
> tc6->int_flag = true;
> wake_up_interruptible(&tc6->spi_wq);
Perfect, thats the same I added and also works on my side.
> >
> > - If there is a lot of traffic, the receive buffer overflow error spams the log.
> >
> > - If there is a lot of traffic, I got various kernel panics in oa_tc6_update_rx_skb.
> > Mostly because more data to rx_skb is added than allocated and sometimes because
> > rx_skb is null in oa_tc6_update_rx_skb or oa_tc6_prcs_rx_frame_end. Some debugging
> > with a logic analyzer showed that the chip is not behave correctly. There is more
> > bytes between start_valid and end_valid than there should be. Also there
> > seems to be 2 end_valid without a start_valid between. What is common is that the incorrect
> > frame starts in a chunk where end_valid and start_valid is set.
> > In my opinion its a problem in the chip (maybe related to the errata in the next point)
> > but the driver should be resilent and just drop the packet and not cause a kernel panic.
> Usually I run into this issue "receive buffer overflow" when I run RPI 4
> with default cpu governor setting which is "ondemand". In this case,
> even though if I set SPI clock speed as 15 MHz the RPI 4 core clock is
> clocking down when it is idle which leads delivering half of the
> configured SPI clock speed around 5.9 MHz. So the systems like RPI 4
> need performance mode enabled to get the proper clock speed for SPI.
> Refer below link for more details.
>
> https://github.com/raspberrypi/linux/issues/3381#issuecomment-1144723750
>
> I used to enable performance mode using the below command.
>
> echo performance | sudo tee
> /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor > /dev/null
>
> So please ensure the SPI clock speed using a logic analyzer to get the
> maximum throughput without receive buffer overflow.
>
> Of course, I agree that the driver should not crash in case of receive
> buffer overflow. By referring your investigations, I understand that the
> buffers in the MAC-PHY is being continuously overwritten again and again
> as the host is very slow to read the data from the MAC-PHY buffers
> through SPI which alters the descriptors. There might be two reasons why
> we run into this situation.
> 1. The host is busy doing something else and delays to initiate SPI even
> though SPI clock speed is 15 MHz.
> 2. The SPI clock speed is less than 15 MHz.
Sorry there is a missunderstanding between us. The receive buffer overflow is not
causing any harm except filling the log. In my setup I get in one day about 35000
entries. I am not sure if its appropriate to log these errors.
The SPI Frequency is at 14.8 MHz. If I just have 2 boards connected, I am not able
to reproduce this. Only with 3 boards when 2 boards sends multiple big ethernet
frames (1512 byte per Frame) to one, I get these log entries.
The latency seems to be quite low, from IRQ to start reading first frame it takes
always less than 500us. Also the boards are just running the udp test.
>
> I use the below iperf3 setup for my testing and never faced the driver
> crash issue even though faced "receive buffer overflow" error when I run
> RPI 4 with "ondemand" default mode.
>
> Node 0 - Raspberry Pi 4 with LAN8650 MAC-PHY
> $ iperf3 -s
> Node 1 - Raspberry Pi 4 with EVB-LAN8670-USB USB Stick
> $ iperf3 -c 192.168.5.100 -u -b 10M -i 1 -t 0
>
> and vice versa.
>
> I never faced "receive buffer overflow" error when I run RPI 4 with
> "performance" mode enabled and even though all the cores are stressed
> using the below command,
>
> $ yes >/dev/null & yes >/dev/null & yes >/dev/null & yes >/dev/null &
>
> Can you share more details about your testing setup and applications you
> use, so that I will try to reproduce the issue in my setup to debug the
> driver?
I use a internal tool which does some stress tests using udp. Unfortunately,
I am not allowed to publish it, but a colleague works on a rust implementation,
which we can publish, but its not fully ready yet.
On one board the tool is running in server mode. It just echoes back the received
data. On the 2 other boards the tool is running in client mode. It sends various
sized udp-packets in different bursts and then checks if it receives the same
data in the same order.
The crashes only happens when ZARFE is not set (with Rev B0). When the crash
happens, I see on the logic analyzer that there are more bytes than mtu + headers
between the frame where start_valid is set and the frame where end_valid is set.
Then this happens:
[ 437.155673] skbuff: skb_over_panic: text:ffff80007a8c2bd8 len:1600 put:64 head:ffff00000de28080
data:ffff00000de280c0 tail:0x680 end:0x640 dev:eth1
[ 437.168987] kernel BUG at net/core/skbuff.c:192!
[ 437.173612] Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
[ 437.180407] Modules linked in: ppp_async crc_ccitt ppp_generic slhc lan865x oa_tc6 bec_infoo(O)
tpm_tis_spi tpm_tis_core spi_imx imx_sdma
[ 437.196016] CPU: 1 PID: 455 Comm: oa-tc6-spi-thre Tainted: G O 6.6.11-
gce336e2c2bc3-dirty #1
[ 437.205853] Hardware name: Toradex Verdin iMX8M Mini on FUMU (DT)
[ 437.212820] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 437.219790] pc : skb_panic+0x58/0x5c
[ 437.223376] lr : skb_panic+0x58/0x5c
[ 437.226959] sp : ffff80008362bd90
[ 437.230278] x29: ffff80008362bda0 x28: 0000000000000000 x27: ffff000001066878
[ 437.237426] x26: 000000000000001e x25: 00000000000007f8 x24: ffff0000010cea80
[ 437.244571] x23: 00000000f0f0f0f1 x22: 000000000000001f x21: 0000000000000000
[ 437.251720] x20: ffff0000010ceaa8 x19: 000000003f20003f x18: ffffffffffffffff
[ 437.258867] x17: ffff7ffffded9000 x16: ffff800080008000 x15: 073a0764076e0765
[ 437.266015] x14: 0720073007380736 x13: ffff8000823d1f58 x12: 0000000000000534
[ 437.273162] x11: 00000000000001bc x10: ffff800082429f58 x9 : ffff8000823d1f58
[ 437.280310] x8 : 00000000ffffefff x7 : ffff800082429f58 x6 : 0000000000000000
[ 437.287455] x5 : 000000000000bff4 x4 : 0000000000000000 x3 : 0000000000000000
[ 437.294606] x2 : 0000000000000000 x1 : ffff000001223b00 x0 : 0000000000000087
[ 437.301753] Call trace:
[ 437.304203] skb_panic+0x58/0x5c
[ 437.307436] skb_find_text+0x0/0xf0
[ 437.310933] oa_tc6_spi_thread_handler+0x438/0x880 [oa_tc6]
[ 437.316523] kthread+0x118/0x11c
[ 437.319758] ret_from_fork+0x10/0x20
[ 437.323343] Code: f90007e9 b940b908 f90003e8 97ca3c34 (d4210000)
[ 437.329446] ---[ end trace 0000000000000000 ]---
Sometimes there are 2 end_valid after eachother without a start_valid between.
Then this happens:
[ 469.737297] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000074
[ 469.746137] Mem abort info:
[ 469.748950] ESR = 0x0000000096000004
[ 469.752709] EC = 0x25: DABT (current EL), IL = 32 bits
[ 469.758036] SET = 0, FnV = 0
[ 469.761098] EA = 0, S1PTW = 0
[ 469.764252] FSC = 0x04: level 0 translation fault
[ 469.769144] Data abort info:
[ 469.772033] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
[ 469.777529] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[ 469.782594] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[ 469.787921] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000043c32000
[ 469.794377] [0000000000000074] pgd=0000000000000000, p4d=0000000000000000
[ 469.801184] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
[ 469.807459] Modules linked in: ppp_async crc_ccitt ppp_generic slhc lan865x oa_tc6 bec_infoo(O)
tpm_tis_spi tpm_tis_core spi_imx imx_sdma
[ 469.823064] CPU: 2 PID: 456 Comm: oa-tc6-spi-thre Tainted: G O 6.6.11-
g350ed394a6ca-dirty #1
[ 469.832903] Hardware name: Toradex Verdin iMX8M Mini on FUMU (DT)
[ 469.839871] pstate: 00000005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 469.846841] pc : skb_put+0xc/0x6c
[ 469.850169] lr : oa_tc6_spi_thread_handler+0x438/0x880 [oa_tc6]
[ 469.856106] sp : ffff80008376bdb0
[ 469.859424] x29: ffff80008376bdb0 x28: 0000000000000000 x27: ffff00000194c080
[ 469.866573] x26: 0000000000000000 x25: 0000000000000000 x24: ffff000001095c80
[ 469.873720] x23: 00000000f0f0f0f1 x22: 000000000000001f x21: 0000000000000000
[ 469.880870] x20: ffff000001095ca8 x19: 000000003f20003f x18: 0000000000000000
[ 469.888023] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
[ 469.895174] x14: 0000031acf8b86d8 x13: 0000000000000000 x12: 0000000000000000
[ 469.902321] x11: 0000000000000002 x10: 0000000000000a60 x9 : ffff80008376b970
[ 469.909467] x8 : ffff00007fb6e580 x7 : 000000000194b080 x6 : 0000000000000000
[ 469.916616] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 000000000000fc80
[ 469.923765] x2 : 0000000000000001 x1 : 0000000000000040 x0 : 0000000000000000
[ 469.930915] Call trace:
[ 469.933365] skb_put+0xc/0x6c
[ 469.936342] oa_tc6_spi_thread_handler+0x438/0x880 [oa_tc6]
[ 469.941929] kthread+0x118/0x11c
[ 469.945166] ret_from_fork+0x10/0x20
[ 469.948752] Code: d65f03c0 d503233f a9bf7bfd 910003fd (b9407406)
[ 469.954854] ---[ end trace 0000000000000000 ]---
If interested I can try to get a recording with the logic analyzer and send it to you.
By the way in the other answer you attached a screenshot of the logic analyzer and you
have a very nice HLA for oa_tc6. Are they open-source or are there any plans to publish them?
> >
> > - Sometimes the chip stops working. It always asserts the irq but there is no data (rca=0)
> > and also exst is not active. I found out that there is an errata (DS80001075) point s3
> > that explains this. I set the ZARFE bit in CONFIG0. This also fixes the point above.
> > The driver now works since about 2.5 weeks with various load with just one loss of frame
> > error where I had to reboot the system after about 4 days.
> It is good to hear that the driver works fine with the above changes. As
> mentioned in the errata, this continuous interrupt issue is a known
> issue with LAN8651 Rev.B0. Switching to LAN8651 Rev.B1 will solve this
> issue and no need of any workaround. Setting ZARFE bit in the CONFIG0
> will solve the continuous interrupt issue but don't know how the above
> "receive buffer overflow" issue also solved. I think it is a good idea
> to test with LAN8651 Rev.B1 without setting ZARFE bit once. It would be
> interesting to see the result. I am always using LAN8651 Rev.B1 for my
> testing.
Unfortunately I just have LAN8651 Rev. B0 Chips. Are you sure that the Rev B1 has the
issue fixed? The errata here says that B1 is affected too:
https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/Errata/LAN8650-1-Errata-80001075.pdf
>
> I should be able to reproduce the "receive buffer overflow" issue and
> consequently kernel crash in my setup with LAN8651 Rev.B1 so that I can
> investigate the issue further. As I am not able to reproduce in my RPI
> 4, I need your support for the tests and applications you used in your
> setup.
> >
> > Is there a reason why you removed the netdev watchdog which was active in v2?
> When the timeout occurs, there is no further action except increasing
> tx_errors. Not seeing this except USB-to-Ethernet which can be removed
> unexpectedly. But this is SPI interface which will not be removed
> unexpectedly as it is a platform device. That's why we removed this.
>
> Best regards,
> Parthiban V
> >
> > Thanks,
> > Benjamin Bigler
> >
>
Thanks,
Benjamin Bigler
^ permalink raw reply
* Re: [PATCH v3 0/6] Add Synopsys DesignWare HDMI RX Controller
From: Heiko Stübner @ 2024-04-03 22:48 UTC (permalink / raw)
To: Shreeya Patel, Krzysztof Kozlowski
Cc: mchehab, hverkuil, hverkuil-cisco, robh, krzysztof.kozlowski+dt,
conor+dt, mturquette, sboyd, p.zabel, shawn.wen, kernel,
linux-kernel, linux-media, devicetree, linux-arm-kernel,
linux-rockchip, linux-clk, linux-arm
In-Reply-To: <86150c89-11d5-4d52-987e-974b1a03018f@linaro.org>
Am Mittwoch, 3. April 2024, 13:24:05 CEST schrieb Krzysztof Kozlowski:
> On 03/04/2024 13:20, Shreeya Patel wrote:
> > On Wednesday, April 03, 2024 15:51 IST, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >
> >> On 03/04/2024 11:24, Shreeya Patel wrote:
> >>> On Thursday, March 28, 2024 04:20 IST, Shreeya Patel <shreeya.patel@collabora.com> wrote:
> >>>
> >>>> This series implements support for the Synopsys DesignWare
> >>>> HDMI RX Controller, being compliant with standard HDMI 1.4b
> >>>> and HDMI 2.0.
> >>>>
> >>>
> >>> Hi Mauro and Hans,
> >>>
> >>> I haven't received any reviews so far. Hence, this is just a gentle reminder to review this patch series.
> >>
> >> Why did you put clk changes here? These go via different subsystem. That
> >> might be one of obstacles for your patchset.
> >>
> >
> > I added clock changes in this patch series because HDMIRX driver depends on it.
> > I thought it is wrong to send the driver patches which don't even compile?
>
> Hm, why HDMIRX driver depends on clock? How? This sounds really wrong.
> Please get it reviewed internally first.
For the change in question, the clock controller on the soc also handles
the reset controls (hence its name CRU, clock-and-reset-unit) .
There are at least 660 reset lines in the unit and it seems the hdmi-rx one
was overlooked on the initial submission, hence patches 1+2 add the
reset-line.
Of course, here only the "arm64: dts:" patch depends on the clock
change, is it references the new reset-id.
Am Mittwoch, 3. April 2024, 12:22:57 CEST schrieb Krzysztof Kozlowski:
> Please do not engage multiple subsystems in one patchset, if not
> necessary. Especially do not mix DTS into media or USB subsystems. And
> do not put DTS in the middle!
picking up your reply from patch 4/6, there seem to be different "schools
of thought" for this. Some maintainers might want to really only see
patches that are explicitly for their subsystem - I guess networking
might be a prime example for that, who will essentially apply whole series'
if nobody protests in time (including dts patches)
On the other hand I also remember seeing requests for "the full picture"
and individual maintainers then just picking and applying the patches
meant for their subsystem.
The series as it stands right now is nice in that it allows (random)
developers to just pick it up, apply it to a tree and test the actual driver
without needing to hunt for multiple dependant series.
Of course you're right, the "arm64: dts:" patch should be the last in the
series and not be in the middle of it.
Regards
Heiko
^ 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