mbox series

[v4,0/3] clk: bcm: rpi: Add support for three more clocks

Message ID 20220428065743.94967-1-iivanov@suse.de (mailing list archive)
Headers show
Series clk: bcm: rpi: Add support for three more clocks | expand

Message

Ivan T . Ivanov April 28, 2022, 6:57 a.m. UTC
Add missing clock required by RPiVid video decoder and provide more
reliable and accurate source for HDMI pixel and video encoder clocks.

Changes since v3
- Put back support for VEC clock, which was actually one of
  reasons for this patch-set [1]. I mixed "HEVC" vs. "VEC", sorry.

  [1] https://bugzilla.suse.com/show_bug.cgi?id=1198942

Changes since v2
- Added Acks from Maxime Ripard and Dave Stevenson

Changes since v1
- Drop RPI_FIRMWARE_VEC_CLK_ID clock it doesn't seems to be used.
- Rework downstream changes on top of recent Maxime changes.

Dom Cobley (1):
  clk: bcm: rpi: Add support for VEC clock

Ivan T. Ivanov (2):
  clk: bcm: rpi: Add support HEVC clock
  clk: bcm: rpi: Handle pixel clock in firmware

 drivers/clk/bcm/clk-raspberrypi.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Guillaume Gardet May 10, 2022, 1:20 p.m. UTC | #1
Hi,

May I ask what's the status/plan of  this patch series?

It seems it has not been merged yet, and I know we are a bit late in the 5.18 schedule, but I think this is a good fix for 5.18. 
And, this looks like to be a good candidate for a backport to stable kernels.

Thanks,
Guillaume


> -----Original Message-----
> From: Ivan T. Ivanov <iivanov@suse.de>
> Sent: 28 April 2022 08:58
> To: Michael Turquette <mturquette@baylibre.com>; Stephen Boyd
> <sboyd@kernel.org>; Nicolas Saenz Julienne <nsaenz@kernel.org>
> Cc: Maxime Ripard <maxime@cerno.tech>; Dave Stevenson
> <dave.stevenson@raspberrypi.com>; Guillaume Gardet
> <Guillaume.Gardet@arm.com>; bcm-kernel-feedback-list@broadcom.com;
> linux-clk@vger.kernel.org; linux-rpi-kernel@lists.infradead.org; linux-arm-
> kernel@lists.infradead.org; Ivan T. Ivanov <iivanov@suse.de>
> Subject: [PATCH v4 0/3] clk: bcm: rpi: Add support for three more clocks
> 
> Add missing clock required by RPiVid video decoder and provide more reliable
> and accurate source for HDMI pixel and video encoder clocks.
> 
> Changes since v3
> - Put back support for VEC clock, which was actually one of
>   reasons for this patch-set [1]. I mixed "HEVC" vs. "VEC", sorry.
> 
>   [1] https://bugzilla.suse.com/show_bug.cgi?id=1198942
> 
> Changes since v2
> - Added Acks from Maxime Ripard and Dave Stevenson
> 
> Changes since v1
> - Drop RPI_FIRMWARE_VEC_CLK_ID clock it doesn't seems to be used.
> - Rework downstream changes on top of recent Maxime changes.
> 
> Dom Cobley (1):
>   clk: bcm: rpi: Add support for VEC clock
> 
> Ivan T. Ivanov (2):
>   clk: bcm: rpi: Add support HEVC clock
>   clk: bcm: rpi: Handle pixel clock in firmware
> 
>  drivers/clk/bcm/clk-raspberrypi.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> --
> 2.34.1
Maxime Ripard May 10, 2022, 1:30 p.m. UTC | #2
Hi,

On Tue, May 10, 2022 at 01:20:18PM +0000, Guillaume Gardet wrote:
> May I ask what's the status/plan of  this patch series?

As far as I know it hasn't been merged yet.

> It seems it has not been merged yet, and I know we are a bit late in
> the 5.18 schedule, but I think this is a good fix for 5.18.

Fix for what? I don't think this series fix any bug?

> And, this looks like to be a good candidate for a backport to stable
> kernels.

This is not going to be trivial to backmerge, it depends on 12c90f3f27bb
that isn't marked for stable

maxime
Stefan Wahren May 11, 2022, 6:10 a.m. UTC | #3
Am 10.05.22 um 15:30 schrieb Maxime Ripard:
> Hi,
>
> On Tue, May 10, 2022 at 01:20:18PM +0000, Guillaume Gardet wrote:
>> May I ask what's the status/plan of  this patch series?
> As far as I know it hasn't been merged yet.
>
>> It seems it has not been merged yet, and I know we are a bit late in
>> the 5.18 schedule, but I think this is a good fix for 5.18.
> Fix for what? I don't think this series fix any bug?
This seems to be a "fix" for the Frankenstone scenario: mainline kernel 
+ vendor DT
>> And, this looks like to be a good candidate for a backport to stable
>> kernels.
> This is not going to be trivial to backmerge, it depends on 12c90f3f27bb
> that isn't marked for stable

I don't think this is a good idea to backmerge these changes.

Best regards

>
> maxime
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Maxime Ripard May 12, 2022, 7:57 a.m. UTC | #4
On Wed, May 11, 2022 at 08:10:50AM +0200, Stefan Wahren wrote:
> Am 10.05.22 um 15:30 schrieb Maxime Ripard:
> > Hi,
> > 
> > On Tue, May 10, 2022 at 01:20:18PM +0000, Guillaume Gardet wrote:
> > > May I ask what's the status/plan of  this patch series?
> > As far as I know it hasn't been merged yet.
> > 
> > > It seems it has not been merged yet, and I know we are a bit late in
> > > the 5.18 schedule, but I think this is a good fix for 5.18.
> > Fix for what? I don't think this series fix any bug?
>
> This seems to be a "fix" for the Frankenstone scenario: mainline kernel +
> vendor DT

Did we ever support this?

I don't think we did, so even though it can be nice to improve that
situation, I don't think it's worth sending this to stable

Maxime
Ivan Ivanov May 12, 2022, 8:10 a.m. UTC | #5
Hi,

> On 12 May 2022, at 10:57, Maxime Ripard <maxime@cerno.tech> wrote:
> 
> On Wed, May 11, 2022 at 08:10:50AM +0200, Stefan Wahren wrote:
>> Am 10.05.22 um 15:30 schrieb Maxime Ripard:
>>> Hi,
>>> 
>>> On Tue, May 10, 2022 at 01:20:18PM +0000, Guillaume Gardet wrote:
>>>> May I ask what's the status/plan of  this patch series?
>>> As far as I know it hasn't been merged yet.
>>> 
>>>> It seems it has not been merged yet, and I know we are a bit late in
>>>> the 5.18 schedule, but I think this is a good fix for 5.18.
>>> Fix for what? I don't think this series fix any bug?
>> 
>> This seems to be a "fix" for the Frankenstone scenario: mainline kernel +
>> vendor DT
> 
> Did we ever support this?
> 
> I don't think we did, so even though it can be nice to improve that
> situation, I don't think it's worth sending this to stable

Yes, maybe not stable material, but considering support for devices
which are shipped with upstream Linux and vendor device tree blobs,
saved somewhere on them, should be pretty normal to expect, right?

Regards,
Ivan
Maxime Ripard May 12, 2022, 8:53 a.m. UTC | #6
On Thu, May 12, 2022 at 11:10:00AM +0300, Ivan Ivanov wrote:
> Hi,
> 
> > On 12 May 2022, at 10:57, Maxime Ripard <maxime@cerno.tech> wrote:
> > 
> > On Wed, May 11, 2022 at 08:10:50AM +0200, Stefan Wahren wrote:
> >> Am 10.05.22 um 15:30 schrieb Maxime Ripard:
> >>> Hi,
> >>> 
> >>> On Tue, May 10, 2022 at 01:20:18PM +0000, Guillaume Gardet wrote:
> >>>> May I ask what's the status/plan of  this patch series?
> >>> As far as I know it hasn't been merged yet.
> >>> 
> >>>> It seems it has not been merged yet, and I know we are a bit late in
> >>>> the 5.18 schedule, but I think this is a good fix for 5.18.
> >>> Fix for what? I don't think this series fix any bug?
> >> 
> >> This seems to be a "fix" for the Frankenstone scenario: mainline kernel +
> >> vendor DT
> > 
> > Did we ever support this?
> > 
> > I don't think we did, so even though it can be nice to improve that
> > situation, I don't think it's worth sending this to stable
> 
> Yes, maybe not stable material, but considering support for devices
> which are shipped with upstream Linux and vendor device tree blobs,
> saved somewhere on them, should be pretty normal to expect, right?

Not really?

If the vendor in question uses a binding that has never been reviewed,
accepted, and supported by upstream, then I don't see what upstream
should be doing to accommodate for that situation?

Maxime
Ivan Ivanov May 12, 2022, 9:03 a.m. UTC | #7
Hi,

> On 12 May 2022, at 11:53, Maxime Ripard <maxime@cerno.tech> wrote:
> 
> On Thu, May 12, 2022 at 11:10:00AM +0300, Ivan Ivanov wrote:
>> Hi,
>> 
>>> On 12 May 2022, at 10:57, Maxime Ripard <maxime@cerno.tech> wrote:
>>> 
>>> On Wed, May 11, 2022 at 08:10:50AM +0200, Stefan Wahren wrote:
>>>> Am 10.05.22 um 15:30 schrieb Maxime Ripard:
>>>>> Hi,
>>>>> 
>>>>> On Tue, May 10, 2022 at 01:20:18PM +0000, Guillaume Gardet wrote:
>>>>>> May I ask what's the status/plan of  this patch series?
>>>>> As far as I know it hasn't been merged yet.
>>>>> 
>>>>>> It seems it has not been merged yet, and I know we are a bit late in
>>>>>> the 5.18 schedule, but I think this is a good fix for 5.18.
>>>>> Fix for what? I don't think this series fix any bug?
>>>> 
>>>> This seems to be a "fix" for the Frankenstone scenario: mainline kernel +
>>>> vendor DT
>>> 
>>> Did we ever support this?
>>> 
>>> I don't think we did, so even though it can be nice to improve that
>>> situation, I don't think it's worth sending this to stable
>> 
>> Yes, maybe not stable material, but considering support for devices
>> which are shipped with upstream Linux and vendor device tree blobs,
>> saved somewhere on them, should be pretty normal to expect, right?
> 
> Not really?
> 
> If the vendor in question uses a binding that has never been reviewed,
> accepted, and supported by upstream, then I don't see what upstream
> should be doing to accommodate for that situation?


Ok, let do not generalise this discussion too much :-)

I bringed above as example that some times device tree could not
be changed and it is provided by board supplier, not by Linux OS
supplier.

In this particular case vendor device tree blob is using standard 
clock bindings. Just upstream Linux clock driver don’t have support
for some of the clocks. This is what these patches fix.

Regards,
Ivan
Ivan T . Ivanov May 26, 2022, 11:36 a.m. UTC | #8
On 04-28 09:57, Ivan T. Ivanov wrote:
> Date: Thu, 28 Apr 2022 09:57:40 +0300
> From: "Ivan T. Ivanov" <iivanov@suse.de>
> To: Michael Turquette <mturquette@baylibre.com>, Stephen Boyd
>  <sboyd@kernel.org>, Nicolas Saenz Julienne <nsaenz@kernel.org>
> Cc: Maxime Ripard <maxime@cerno.tech>, Dave Stevenson
>  <dave.stevenson@raspberrypi.com>, Guillaume GARDET
>  <guillaume.gardet@arm.com>, bcm-kernel-feedback-list@broadcom.com,
>  linux-clk@vger.kernel.org, linux-rpi-kernel@lists.infradead.org,
>  linux-arm-kernel@lists.infradead.org, "Ivan T. Ivanov" <iivanov@suse.de>
> Subject: [PATCH v4 0/3] clk: bcm: rpi: Add support for three more clocks
> Message-Id: <20220428065743.94967-1-iivanov@suse.de>
> 
> Add missing clock required by RPiVid video decoder and provide more
> reliable and accurate source for HDMI pixel and video encoder clocks.
> 

Hi Stephen, Michael. What is the plan for this series of patches?

Regards,
Ivan

> Changes since v3
> - Put back support for VEC clock, which was actually one of
>   reasons for this patch-set [1]. I mixed "HEVC" vs. "VEC", sorry.
> 
>   [1] https://bugzilla.suse.com/show_bug.cgi?id=1198942
> 
> Changes since v2
> - Added Acks from Maxime Ripard and Dave Stevenson
> 
> Changes since v1
> - Drop RPI_FIRMWARE_VEC_CLK_ID clock it doesn't seems to be used.
> - Rework downstream changes on top of recent Maxime changes.
> 
> Dom Cobley (1):
>   clk: bcm: rpi: Add support for VEC clock
> 
> Ivan T. Ivanov (2):
>   clk: bcm: rpi: Add support HEVC clock
>   clk: bcm: rpi: Handle pixel clock in firmware
> 
>  drivers/clk/bcm/clk-raspberrypi.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> -- 
> 2.34.1
>
Florian Fainelli June 21, 2022, 3:30 p.m. UTC | #9
On 5/26/2022 4:36 AM, Ivan T. Ivanov wrote:
> On 04-28 09:57, Ivan T. Ivanov wrote:
>> Date: Thu, 28 Apr 2022 09:57:40 +0300
>> From: "Ivan T. Ivanov" <iivanov@suse.de>
>> To: Michael Turquette <mturquette@baylibre.com>, Stephen Boyd
>>   <sboyd@kernel.org>, Nicolas Saenz Julienne <nsaenz@kernel.org>
>> Cc: Maxime Ripard <maxime@cerno.tech>, Dave Stevenson
>>   <dave.stevenson@raspberrypi.com>, Guillaume GARDET
>>   <guillaume.gardet@arm.com>, bcm-kernel-feedback-list@broadcom.com,
>>   linux-clk@vger.kernel.org, linux-rpi-kernel@lists.infradead.org,
>>   linux-arm-kernel@lists.infradead.org, "Ivan T. Ivanov" <iivanov@suse.de>
>> Subject: [PATCH v4 0/3] clk: bcm: rpi: Add support for three more clocks
>> Message-Id: <20220428065743.94967-1-iivanov@suse.de>
>>
>> Add missing clock required by RPiVid video decoder and provide more
>> reliable and accurate source for HDMI pixel and video encoder clocks.
>>
> 
> Hi Stephen, Michael. What is the plan for this series of patches?

Ping?
Stefan Wahren July 10, 2022, 11:12 a.m. UTC | #10
Hi,

Am 10.05.22 um 15:30 schrieb Maxime Ripard:
> Hi,
>
> On Tue, May 10, 2022 at 01:20:18PM +0000, Guillaume Gardet wrote:
>> May I ask what's the status/plan of  this patch series?
> As far as I know it hasn't been merged yet.
>
>> It seems it has not been merged yet, and I know we are a bit late in
>> the 5.18 schedule, but I think this is a good fix for 5.18.
> Fix for what? I don't think this series fix any bug?

i think this series (or at least parts of it) is a workaround for this 
issue [1].

We better fix the root cause of the potential out of bounds access in 
clk-raspberrypi properly. I will send a patch soon.

[1] - https://github.com/raspberrypi/firmware/issues/1688

>
>> And, this looks like to be a good candidate for a backport to stable
>> kernels.
> This is not going to be trivial to backmerge, it depends on 12c90f3f27bb
> that isn't marked for stable
>
> maxime
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Ivan T . Ivanov July 11, 2022, 7:02 a.m. UTC | #11
Hi,

On 2022-07-10 14:12, Stefan Wahren wrote:
> Hi,
> 
> Am 10.05.22 um 15:30 schrieb Maxime Ripard:
>> Hi,
>> 
>> On Tue, May 10, 2022 at 01:20:18PM +0000, Guillaume Gardet wrote:
>>> May I ask what's the status/plan of  this patch series?
>> As far as I know it hasn't been merged yet.
>> 
>>> It seems it has not been merged yet, and I know we are a bit late in
>>> the 5.18 schedule, but I think this is a good fix for 5.18.
>> Fix for what? I don't think this series fix any bug?
> 
> i think this series (or at least parts of it) is a workaround for this
> issue [1].

No, not really. Bug which this patch set fixes is this one [2].
When using downstream dtb and up to date bootloader index 15 is
valid id RPI_FIRMWARE_VEC_CLK_ID which is used by vc4 module.

> 
> We better fix the root cause of the potential out of bounds access in
> clk-raspberrypi properly. I will send a patch soon.

If not mistaken after last rework driver already handle this properly.
clk-raspberrypi.c:362

Regards,
Ivan

> 
> [1] - https://github.com/raspberrypi/firmware/issues/1688

[2] https://bugzilla.suse.com/show_bug.cgi?id=1196632
Stefan Wahren July 11, 2022, 11:55 a.m. UTC | #12
Hi,

Am 11.07.22 um 09:02 schrieb Ivan T. Ivanov:
>
> Hi,
>
> On 2022-07-10 14:12, Stefan Wahren wrote:
>> Hi,
>>
>> Am 10.05.22 um 15:30 schrieb Maxime Ripard:
>>> Hi,
>>>
>>> On Tue, May 10, 2022 at 01:20:18PM +0000, Guillaume Gardet wrote:
>>>> May I ask what's the status/plan of this patch series?
>>> As far as I know it hasn't been merged yet.
>>>
>>>> It seems it has not been merged yet, and I know we are a bit late in
>>>> the 5.18 schedule, but I think this is a good fix for 5.18.
>>> Fix for what? I don't think this series fix any bug?
>>
>> i think this series (or at least parts of it) is a workaround for this
>> issue [1].
>
> No, not really. Bug which this patch set fixes is this one [2].

Both are related. In your case the device tree defines a clock ID which 
isn't define in the kernel driver. So the behavior (abort probing in 
case of invalid DTB) is correct to me. But the log message isn't helpful 
to end users.

This is broken by design, since we need to synchronize videocore 
firmware, kernel and DTB at the same time. Maybe this works for 
Raspberry Pi OS, but not for all users.

We need to fix this driver before add new clocks.

> When using downstream dtb and up to date bootloader index 15 is
> valid id RPI_FIRMWARE_VEC_CLK_ID which is used by vc4 module.
I think this only applies to the downstream kernel. AFAIK the 
bcm2835-clk is still used for VEC in mainline.
>
>>
>> We better fix the root cause of the potential out of bounds access in
>> clk-raspberrypi properly. I will send a patch soon.
>
> If not mistaken after last rework driver already handle this properly.
> clk-raspberrypi.c:362

No, this doesn't prevent the possible out of bounds access, what i mean. 
It's the condition of the while loop.

Best regards

>
> Regards,
> Ivan
>
>>
>> [1] - https://github.com/raspberrypi/firmware/issues/1688
>
> [2] https://bugzilla.suse.com/show_bug.cgi?id=1196632
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel