2021-12-07 01:56:23

by Jammy Huang

[permalink] [raw]
Subject: [PATCH] media: aspeed: Use runtime configuration

The aspeed video IP has some differences between SoC families. Currently
the driver decides which registers to use at compile time, which means
a single kernel can not be used between platforms.

Switch to using runtime configuration of the registers that vary between
SoC families.

Signed-off-by: Joel Stanley <[email protected]>
Signed-off-by: Jammy Huang <[email protected]>
---
drivers/media/platform/aspeed-video.c | 71 ++++++++++++++++++++-------
1 file changed, 52 insertions(+), 19 deletions(-)

diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index d2335d669fb3..ba8ee82b38c3 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -75,11 +75,8 @@
#define VE_SEQ_CTRL_CAP_BUSY BIT(16)
#define VE_SEQ_CTRL_COMP_BUSY BIT(18)

-#ifdef CONFIG_MACH_ASPEED_G4
-#define VE_SEQ_CTRL_JPEG_MODE BIT(8) /* AST2400 */
-#else
-#define VE_SEQ_CTRL_JPEG_MODE BIT(13) /* AST2500/2600 */
-#endif
+#define AST2500_VE_SEQ_CTRL_JPEG_MODE BIT(13)
+#define AST2400_VE_SEQ_CTRL_JPEG_MODE BIT(8)

#define VE_CTRL 0x008
#define VE_CTRL_HSYNC_POL BIT(0)
@@ -136,9 +133,8 @@
#define VE_COMP_CTRL_HQ_DCT_CHR GENMASK(26, 22)
#define VE_COMP_CTRL_HQ_DCT_LUM GENMASK(31, 27)

-#define VE_OFFSET_COMP_STREAM 0x078
-
-#define VE_JPEG_COMP_SIZE_READ_BACK 0x084
+#define AST2400_VE_COMP_SIZE_READ_BACK 0x078
+#define AST2600_VE_COMP_SIZE_READ_BACK 0x084

#define VE_SRC_LR_EDGE_DET 0x090
#define VE_SRC_LR_EDGE_DET_LEFT GENMASK(11, 0)
@@ -233,6 +229,8 @@ struct aspeed_video {
struct video_device vdev;
struct mutex video_lock; /* v4l2 and videobuf2 lock */

+ struct aspeed_video_config config;
+
wait_queue_head_t wait;
spinlock_t lock; /* buffer list lock */
struct delayed_work res_work;
@@ -258,6 +256,30 @@ struct aspeed_video {

#define to_aspeed_video(x) container_of((x), struct aspeed_video, v4l2_dev)

+struct aspeed_video_config {
+ u32 version;
+ u32 jpeg_mode;
+ u32 comp_size_read;
+};
+
+static const struct aspeed_video_config ast2400_config = {
+ .version = 4,
+ .jpeg_mode = AST2400_VE_SEQ_CTRL_JPEG_MODE,
+ .comp_size_read = AST2400_VE_COMP_SIZE_READ_BACK,
+};
+
+static const struct aspeed_video_config ast2500_config = {
+ .version = 5,
+ .jpeg_mode = AST2500_VE_SEQ_CTRL_JPEG_MODE,
+ .comp_size_read = AST2400_VE_COMP_SIZE_READ_BACK,
+};
+
+static const struct aspeed_video_config ast2600_config = {
+ .version = 6,
+ .jpeg_mode = AST2500_VE_SEQ_CTRL_JPEG_MODE,
+ .comp_size_read = AST2600_VE_COMP_SIZE_READ_BACK,
+};
+
static const u32 aspeed_video_jpeg_header[ASPEED_VIDEO_JPEG_HEADER_SIZE] = {
0xe0ffd8ff, 0x464a1000, 0x01004649, 0x60000101, 0x00006000, 0x0f00feff,
0x00002d05, 0x00000000, 0x00000000, 0x00dbff00
@@ -640,7 +662,7 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
if (sts & VE_INTERRUPT_COMP_COMPLETE) {
struct aspeed_video_buffer *buf;
u32 frame_size = aspeed_video_read(video,
- VE_JPEG_COMP_SIZE_READ_BACK);
+ video->config.comp_size_read);

update_perf(&video->perf);

@@ -973,7 +995,7 @@ static void aspeed_video_update_regs(struct aspeed_video *video)
FIELD_PREP(VE_COMP_CTRL_DCT_LUM, video->jpeg_quality) |
FIELD_PREP(VE_COMP_CTRL_DCT_CHR, video->jpeg_quality | 0x10);
u32 ctrl = 0;
- u32 seq_ctrl = VE_SEQ_CTRL_JPEG_MODE;
+ u32 seq_ctrl = video->config.jpeg_mode;

v4l2_dbg(1, debug, &video->v4l2_dev, "framerate(%d)\n",
video->frame_rate);
@@ -993,7 +1015,7 @@ static void aspeed_video_update_regs(struct aspeed_video *video)

/* Set control registers */
aspeed_video_update(video, VE_SEQ_CTRL,
- VE_SEQ_CTRL_JPEG_MODE | VE_SEQ_CTRL_YUV420,
+ video->config.jpeg_mode | VE_SEQ_CTRL_YUV420,
seq_ctrl);
aspeed_video_update(video, VE_CTRL, VE_CTRL_FRC, ctrl);
aspeed_video_update(video, VE_COMP_CTRL,
@@ -1790,8 +1812,18 @@ static int aspeed_video_init(struct aspeed_video *video)
return rc;
}

+static const struct of_device_id aspeed_video_of_match[] = {
+ { .compatible = "aspeed,ast2400-video-engine", .data = &ast2400_config },
+ { .compatible = "aspeed,ast2500-video-engine", .data = &ast2500_config },
+ { .compatible = "aspeed,ast2600-video-engine", .data = &ast2600_config },
+ {}
+};
+MODULE_DEVICE_TABLE(of, aspeed_video_of_match);
+
static int aspeed_video_probe(struct platform_device *pdev)
{
+ const struct aspeed_video_config *config;
+ const struct of_device_id *match;
int rc;
struct resource *res;
struct aspeed_video *video =
@@ -1815,6 +1847,13 @@ static int aspeed_video_probe(struct platform_device *pdev)
if (IS_ERR(video->base))
return PTR_ERR(video->base);

+ match = of_match_node(aspeed_video_of_match, pdev->dev.of_node);
+ if (!match)
+ return -EINVAL;
+
+ config = match->data;
+ video->config = *config;
+
rc = aspeed_video_init(video);
if (rc)
return rc;
@@ -1828,6 +1867,8 @@ static int aspeed_video_probe(struct platform_device *pdev)

aspeed_video_debugfs_create(video);

+ dev_info(video->dev, "compatible for g%d\n", config->version);
+
return 0;
}

@@ -1860,14 +1901,6 @@ static int aspeed_video_remove(struct platform_device *pdev)
return 0;
}

-static const struct of_device_id aspeed_video_of_match[] = {
- { .compatible = "aspeed,ast2400-video-engine" },
- { .compatible = "aspeed,ast2500-video-engine" },
- { .compatible = "aspeed,ast2600-video-engine" },
- {}
-};
-MODULE_DEVICE_TABLE(of, aspeed_video_of_match);
-
static struct platform_driver aspeed_video_driver = {
.driver = {
.name = DEVICE_NAME,
--
2.25.1



2021-12-14 15:12:27

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [PATCH] media: aspeed: Use runtime configuration

Hi Jammy,

A different version of this patch was already applied. If it still makes
change, please rebase this one on the top of the media upstream and
re-send.

Thanks,
Mauro

Em Tue, 7 Dec 2021 09:55:44 +0800
Jammy Huang <[email protected]> escreveu:

> The aspeed video IP has some differences between SoC families. Currently
> the driver decides which registers to use at compile time, which means
> a single kernel can not be used between platforms.
>
> Switch to using runtime configuration of the registers that vary between
> SoC families.
>
> Signed-off-by: Joel Stanley <[email protected]>
> Signed-off-by: Jammy Huang <[email protected]>
> ---
> drivers/media/platform/aspeed-video.c | 71 ++++++++++++++++++++-------
> 1 file changed, 52 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
> index d2335d669fb3..ba8ee82b38c3 100644
> --- a/drivers/media/platform/aspeed-video.c
> +++ b/drivers/media/platform/aspeed-video.c
> @@ -75,11 +75,8 @@
> #define VE_SEQ_CTRL_CAP_BUSY BIT(16)
> #define VE_SEQ_CTRL_COMP_BUSY BIT(18)
>
> -#ifdef CONFIG_MACH_ASPEED_G4
> -#define VE_SEQ_CTRL_JPEG_MODE BIT(8) /* AST2400 */
> -#else
> -#define VE_SEQ_CTRL_JPEG_MODE BIT(13) /* AST2500/2600 */
> -#endif
> +#define AST2500_VE_SEQ_CTRL_JPEG_MODE BIT(13)
> +#define AST2400_VE_SEQ_CTRL_JPEG_MODE BIT(8)
>
> #define VE_CTRL 0x008
> #define VE_CTRL_HSYNC_POL BIT(0)
> @@ -136,9 +133,8 @@
> #define VE_COMP_CTRL_HQ_DCT_CHR GENMASK(26, 22)
> #define VE_COMP_CTRL_HQ_DCT_LUM GENMASK(31, 27)
>
> -#define VE_OFFSET_COMP_STREAM 0x078
> -
> -#define VE_JPEG_COMP_SIZE_READ_BACK 0x084
> +#define AST2400_VE_COMP_SIZE_READ_BACK 0x078
> +#define AST2600_VE_COMP_SIZE_READ_BACK 0x084
>
> #define VE_SRC_LR_EDGE_DET 0x090
> #define VE_SRC_LR_EDGE_DET_LEFT GENMASK(11, 0)
> @@ -233,6 +229,8 @@ struct aspeed_video {
> struct video_device vdev;
> struct mutex video_lock; /* v4l2 and videobuf2 lock */
>
> + struct aspeed_video_config config;
> +
> wait_queue_head_t wait;
> spinlock_t lock; /* buffer list lock */
> struct delayed_work res_work;
> @@ -258,6 +256,30 @@ struct aspeed_video {
>
> #define to_aspeed_video(x) container_of((x), struct aspeed_video, v4l2_dev)
>
> +struct aspeed_video_config {
> + u32 version;
> + u32 jpeg_mode;
> + u32 comp_size_read;
> +};
> +
> +static const struct aspeed_video_config ast2400_config = {
> + .version = 4,
> + .jpeg_mode = AST2400_VE_SEQ_CTRL_JPEG_MODE,
> + .comp_size_read = AST2400_VE_COMP_SIZE_READ_BACK,
> +};
> +
> +static const struct aspeed_video_config ast2500_config = {
> + .version = 5,
> + .jpeg_mode = AST2500_VE_SEQ_CTRL_JPEG_MODE,
> + .comp_size_read = AST2400_VE_COMP_SIZE_READ_BACK,
> +};
> +
> +static const struct aspeed_video_config ast2600_config = {
> + .version = 6,
> + .jpeg_mode = AST2500_VE_SEQ_CTRL_JPEG_MODE,
> + .comp_size_read = AST2600_VE_COMP_SIZE_READ_BACK,
> +};
> +
> static const u32 aspeed_video_jpeg_header[ASPEED_VIDEO_JPEG_HEADER_SIZE] = {
> 0xe0ffd8ff, 0x464a1000, 0x01004649, 0x60000101, 0x00006000, 0x0f00feff,
> 0x00002d05, 0x00000000, 0x00000000, 0x00dbff00
> @@ -640,7 +662,7 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
> if (sts & VE_INTERRUPT_COMP_COMPLETE) {
> struct aspeed_video_buffer *buf;
> u32 frame_size = aspeed_video_read(video,
> - VE_JPEG_COMP_SIZE_READ_BACK);
> + video->config.comp_size_read);
>
> update_perf(&video->perf);
>
> @@ -973,7 +995,7 @@ static void aspeed_video_update_regs(struct aspeed_video *video)
> FIELD_PREP(VE_COMP_CTRL_DCT_LUM, video->jpeg_quality) |
> FIELD_PREP(VE_COMP_CTRL_DCT_CHR, video->jpeg_quality | 0x10);
> u32 ctrl = 0;
> - u32 seq_ctrl = VE_SEQ_CTRL_JPEG_MODE;
> + u32 seq_ctrl = video->config.jpeg_mode;
>
> v4l2_dbg(1, debug, &video->v4l2_dev, "framerate(%d)\n",
> video->frame_rate);
> @@ -993,7 +1015,7 @@ static void aspeed_video_update_regs(struct aspeed_video *video)
>
> /* Set control registers */
> aspeed_video_update(video, VE_SEQ_CTRL,
> - VE_SEQ_CTRL_JPEG_MODE | VE_SEQ_CTRL_YUV420,
> + video->config.jpeg_mode | VE_SEQ_CTRL_YUV420,
> seq_ctrl);
> aspeed_video_update(video, VE_CTRL, VE_CTRL_FRC, ctrl);
> aspeed_video_update(video, VE_COMP_CTRL,
> @@ -1790,8 +1812,18 @@ static int aspeed_video_init(struct aspeed_video *video)
> return rc;
> }
>
> +static const struct of_device_id aspeed_video_of_match[] = {
> + { .compatible = "aspeed,ast2400-video-engine", .data = &ast2400_config },
> + { .compatible = "aspeed,ast2500-video-engine", .data = &ast2500_config },
> + { .compatible = "aspeed,ast2600-video-engine", .data = &ast2600_config },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_video_of_match);
> +
> static int aspeed_video_probe(struct platform_device *pdev)
> {
> + const struct aspeed_video_config *config;
> + const struct of_device_id *match;
> int rc;
> struct resource *res;
> struct aspeed_video *video =
> @@ -1815,6 +1847,13 @@ static int aspeed_video_probe(struct platform_device *pdev)
> if (IS_ERR(video->base))
> return PTR_ERR(video->base);
>
> + match = of_match_node(aspeed_video_of_match, pdev->dev.of_node);
> + if (!match)
> + return -EINVAL;
> +
> + config = match->data;
> + video->config = *config;
> +
> rc = aspeed_video_init(video);
> if (rc)
> return rc;
> @@ -1828,6 +1867,8 @@ static int aspeed_video_probe(struct platform_device *pdev)
>
> aspeed_video_debugfs_create(video);
>
> + dev_info(video->dev, "compatible for g%d\n", config->version);
> +
> return 0;
> }
>
> @@ -1860,14 +1901,6 @@ static int aspeed_video_remove(struct platform_device *pdev)
> return 0;
> }
>
> -static const struct of_device_id aspeed_video_of_match[] = {
> - { .compatible = "aspeed,ast2400-video-engine" },
> - { .compatible = "aspeed,ast2500-video-engine" },
> - { .compatible = "aspeed,ast2600-video-engine" },
> - {}
> -};
> -MODULE_DEVICE_TABLE(of, aspeed_video_of_match);
> -
> static struct platform_driver aspeed_video_driver = {
> .driver = {
> .name = DEVICE_NAME,



Thanks,
Mauro

2021-12-15 01:05:20

by Jammy Huang

[permalink] [raw]
Subject: Re: [PATCH] media: aspeed: Use runtime configuration

Hi Mauro,

OK, I didn't notice previouso one has been applied.
Thank you.

On 2021/12/14 下午 11:12, Mauro Carvalho Chehab wrote:
> Hi Jammy,
>
> A different version of this patch was already applied. If it still makes
> change, please rebase this one on the top of the media upstream and
> re-send.
>
> Thanks,
> Mauro
>
> Em Tue, 7 Dec 2021 09:55:44 +0800
> Jammy Huang <[email protected]> escreveu:
>
>> The aspeed video IP has some differences between SoC families. Currently
>> the driver decides which registers to use at compile time, which means
>> a single kernel can not be used between platforms.
>>
>> Switch to using runtime configuration of the registers that vary between
>> SoC families.
>>
>> Signed-off-by: Joel Stanley <[email protected]>
>> Signed-off-by: Jammy Huang <[email protected]>
>> ---
>> drivers/media/platform/aspeed-video.c | 71 ++++++++++++++++++++-------
>> 1 file changed, 52 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
>> index d2335d669fb3..ba8ee82b38c3 100644
>> --- a/drivers/media/platform/aspeed-video.c
>> +++ b/drivers/media/platform/aspeed-video.c
>> @@ -75,11 +75,8 @@
>> #define VE_SEQ_CTRL_CAP_BUSY BIT(16)
>> #define VE_SEQ_CTRL_COMP_BUSY BIT(18)
>>
>> -#ifdef CONFIG_MACH_ASPEED_G4
>> -#define VE_SEQ_CTRL_JPEG_MODE BIT(8) /* AST2400 */
>> -#else
>> -#define VE_SEQ_CTRL_JPEG_MODE BIT(13) /* AST2500/2600 */
>> -#endif
>> +#define AST2500_VE_SEQ_CTRL_JPEG_MODE BIT(13)
>> +#define AST2400_VE_SEQ_CTRL_JPEG_MODE BIT(8)
>>
>> #define VE_CTRL 0x008
>> #define VE_CTRL_HSYNC_POL BIT(0)
>> @@ -136,9 +133,8 @@
>> #define VE_COMP_CTRL_HQ_DCT_CHR GENMASK(26, 22)
>> #define VE_COMP_CTRL_HQ_DCT_LUM GENMASK(31, 27)
>>
>> -#define VE_OFFSET_COMP_STREAM 0x078
>> -
>> -#define VE_JPEG_COMP_SIZE_READ_BACK 0x084
>> +#define AST2400_VE_COMP_SIZE_READ_BACK 0x078
>> +#define AST2600_VE_COMP_SIZE_READ_BACK 0x084
>>
>> #define VE_SRC_LR_EDGE_DET 0x090
>> #define VE_SRC_LR_EDGE_DET_LEFT GENMASK(11, 0)
>> @@ -233,6 +229,8 @@ struct aspeed_video {
>> struct video_device vdev;
>> struct mutex video_lock; /* v4l2 and videobuf2 lock */
>>
>> + struct aspeed_video_config config;
>> +
>> wait_queue_head_t wait;
>> spinlock_t lock; /* buffer list lock */
>> struct delayed_work res_work;
>> @@ -258,6 +256,30 @@ struct aspeed_video {
>>
>> #define to_aspeed_video(x) container_of((x), struct aspeed_video, v4l2_dev)
>>
>> +struct aspeed_video_config {
>> + u32 version;
>> + u32 jpeg_mode;
>> + u32 comp_size_read;
>> +};
>> +
>> +static const struct aspeed_video_config ast2400_config = {
>> + .version = 4,
>> + .jpeg_mode = AST2400_VE_SEQ_CTRL_JPEG_MODE,
>> + .comp_size_read = AST2400_VE_COMP_SIZE_READ_BACK,
>> +};
>> +
>> +static const struct aspeed_video_config ast2500_config = {
>> + .version = 5,
>> + .jpeg_mode = AST2500_VE_SEQ_CTRL_JPEG_MODE,
>> + .comp_size_read = AST2400_VE_COMP_SIZE_READ_BACK,
>> +};
>> +
>> +static const struct aspeed_video_config ast2600_config = {
>> + .version = 6,
>> + .jpeg_mode = AST2500_VE_SEQ_CTRL_JPEG_MODE,
>> + .comp_size_read = AST2600_VE_COMP_SIZE_READ_BACK,
>> +};
>> +
>> static const u32 aspeed_video_jpeg_header[ASPEED_VIDEO_JPEG_HEADER_SIZE] = {
>> 0xe0ffd8ff, 0x464a1000, 0x01004649, 0x60000101, 0x00006000, 0x0f00feff,
>> 0x00002d05, 0x00000000, 0x00000000, 0x00dbff00
>> @@ -640,7 +662,7 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
>> if (sts & VE_INTERRUPT_COMP_COMPLETE) {
>> struct aspeed_video_buffer *buf;
>> u32 frame_size = aspeed_video_read(video,
>> - VE_JPEG_COMP_SIZE_READ_BACK);
>> + video->config.comp_size_read);
>>
>> update_perf(&video->perf);
>>
>> @@ -973,7 +995,7 @@ static void aspeed_video_update_regs(struct aspeed_video *video)
>> FIELD_PREP(VE_COMP_CTRL_DCT_LUM, video->jpeg_quality) |
>> FIELD_PREP(VE_COMP_CTRL_DCT_CHR, video->jpeg_quality | 0x10);
>> u32 ctrl = 0;
>> - u32 seq_ctrl = VE_SEQ_CTRL_JPEG_MODE;
>> + u32 seq_ctrl = video->config.jpeg_mode;
>>
>> v4l2_dbg(1, debug, &video->v4l2_dev, "framerate(%d)\n",
>> video->frame_rate);
>> @@ -993,7 +1015,7 @@ static void aspeed_video_update_regs(struct aspeed_video *video)
>>
>> /* Set control registers */
>> aspeed_video_update(video, VE_SEQ_CTRL,
>> - VE_SEQ_CTRL_JPEG_MODE | VE_SEQ_CTRL_YUV420,
>> + video->config.jpeg_mode | VE_SEQ_CTRL_YUV420,
>> seq_ctrl);
>> aspeed_video_update(video, VE_CTRL, VE_CTRL_FRC, ctrl);
>> aspeed_video_update(video, VE_COMP_CTRL,
>> @@ -1790,8 +1812,18 @@ static int aspeed_video_init(struct aspeed_video *video)
>> return rc;
>> }
>>
>> +static const struct of_device_id aspeed_video_of_match[] = {
>> + { .compatible = "aspeed,ast2400-video-engine", .data = &ast2400_config },
>> + { .compatible = "aspeed,ast2500-video-engine", .data = &ast2500_config },
>> + { .compatible = "aspeed,ast2600-video-engine", .data = &ast2600_config },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, aspeed_video_of_match);
>> +
>> static int aspeed_video_probe(struct platform_device *pdev)
>> {
>> + const struct aspeed_video_config *config;
>> + const struct of_device_id *match;
>> int rc;
>> struct resource *res;
>> struct aspeed_video *video =
>> @@ -1815,6 +1847,13 @@ static int aspeed_video_probe(struct platform_device *pdev)
>> if (IS_ERR(video->base))
>> return PTR_ERR(video->base);
>>
>> + match = of_match_node(aspeed_video_of_match, pdev->dev.of_node);
>> + if (!match)
>> + return -EINVAL;
>> +
>> + config = match->data;
>> + video->config = *config;
>> +
>> rc = aspeed_video_init(video);
>> if (rc)
>> return rc;
>> @@ -1828,6 +1867,8 @@ static int aspeed_video_probe(struct platform_device *pdev)
>>
>> aspeed_video_debugfs_create(video);
>>
>> + dev_info(video->dev, "compatible for g%d\n", config->version);
>> +
>> return 0;
>> }
>>
>> @@ -1860,14 +1901,6 @@ static int aspeed_video_remove(struct platform_device *pdev)
>> return 0;
>> }
>>
>> -static const struct of_device_id aspeed_video_of_match[] = {
>> - { .compatible = "aspeed,ast2400-video-engine" },
>> - { .compatible = "aspeed,ast2500-video-engine" },
>> - { .compatible = "aspeed,ast2600-video-engine" },
>> - {}
>> -};
>> -MODULE_DEVICE_TABLE(of, aspeed_video_of_match);
>> -
>> static struct platform_driver aspeed_video_driver = {
>> .driver = {
>> .name = DEVICE_NAME,
>
>
> Thanks,
> Mauro

--
Best Regards
Jammy