diff mbox series

[v5,80/80] ARM: dts: bcm2711: Enable the display pipeline

Message ID cfce2276d172d3d9c4d34d966b58fd47f77c4e46.1599120059.git-series.maxime@cerno.tech (mailing list archive)
State New, archived
Headers show
Series drm/vc4: Support BCM2711 Display Pipeline | expand

Commit Message

Maxime Ripard Sept. 3, 2020, 8:01 a.m. UTC
Now that all the drivers have been adjusted for it, let's bring in the
necessary device tree changes.

The VEC and PV3 are left out for now, since it will require a more specific
clock setup.

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Tested-by: Chanwoo Choi <cw00.choi@samsung.com>
Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 arch/arm/boot/dts/bcm2711-rpi-4-b.dts |  48 +++++++++++-
 arch/arm/boot/dts/bcm2711.dtsi        | 122 ++++++++++++++++++++++++++-
 2 files changed, 169 insertions(+), 1 deletion(-)

Comments

Hoegeun Kwon Sept. 7, 2020, 12:03 p.m. UTC | #1
Hi Maxime,

On 9/3/20 5:01 PM, Maxime Ripard wrote:
> Now that all the drivers have been adjusted for it, let's bring in the
> necessary device tree changes.
>
> The VEC and PV3 are left out for now, since it will require a more specific
> clock setup.
>
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Tested-by: Chanwoo Choi <cw00.choi@samsung.com>
> Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Thanks for your v5 patch


Reviewed-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>


Best regards,

Hoegeun

> ---
>   arch/arm/boot/dts/bcm2711-rpi-4-b.dts |  48 +++++++++++-
>   arch/arm/boot/dts/bcm2711.dtsi        | 122 ++++++++++++++++++++++++++-
>   2 files changed, 169 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/bcm2711-rpi-4-b.dts b/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
> index e94244a215af..09a1182c2936 100644
> --- a/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
> +++ b/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
> @@ -70,6 +70,14 @@
>   	};
>   };
>   
> +&ddc0 {
> +	status = "okay";
> +};
> +
> +&ddc1 {
> +	status = "okay";
> +};
> +
>   &firmware {
>   	firmware_clocks: clocks {
>   		compatible = "raspberrypi,firmware-clocks";
> @@ -170,6 +178,38 @@
>   			  "RGMII_TXD3";
>   };
>   
> +&hdmi0 {
> +	clocks = <&firmware_clocks 13>, <&firmware_clocks 14>, <&dvp 0>, <&clk_27MHz>;
> +	clock-names = "hdmi", "bvb", "audio", "cec";
> +	status = "okay";
> +};
> +
> +&hdmi1 {
> +	clocks = <&firmware_clocks 13>, <&firmware_clocks 14>, <&dvp 1>, <&clk_27MHz>;
> +	clock-names = "hdmi", "bvb", "audio", "cec";
> +	status = "okay";
> +};
> +
> +&hvs {
> +	clocks = <&firmware_clocks 4>;
> +};
> +
> +&pixelvalve0 {
> +	status = "okay";
> +};
> +
> +&pixelvalve1 {
> +	status = "okay";
> +};
> +
> +&pixelvalve2 {
> +	status = "okay";
> +};
> +
> +&pixelvalve4 {
> +	status = "okay";
> +};
> +
>   &pwm1 {
>   	pinctrl-names = "default";
>   	pinctrl-0 = <&pwm1_0_gpio40 &pwm1_1_gpio41>;
> @@ -253,3 +293,11 @@
>   &vchiq {
>   	interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>;
>   };
> +
> +&vc4 {
> +	status = "okay";
> +};
> +
> +&vec {
> +	status = "disabled";
> +};
> diff --git a/arch/arm/boot/dts/bcm2711.dtsi b/arch/arm/boot/dts/bcm2711.dtsi
> index 00bcaed1be32..4847dd305317 100644
> --- a/arch/arm/boot/dts/bcm2711.dtsi
> +++ b/arch/arm/boot/dts/bcm2711.dtsi
> @@ -12,6 +12,18 @@
>   
>   	interrupt-parent = <&gicv2>;
>   
> +	vc4: gpu {
> +		compatible = "brcm,bcm2711-vc5";
> +		status = "disabled";
> +	};
> +
> +	clk_27MHz: clk-27M {
> +		#clock-cells = <0>;
> +		compatible = "fixed-clock";
> +		clock-frequency = <27000000>;
> +		clock-output-names = "27MHz-clock";
> +	};
> +
>   	clk_108MHz: clk-108M {
>   		#clock-cells = <0>;
>   		compatible = "fixed-clock";
> @@ -238,6 +250,27 @@
>   			status = "disabled";
>   		};
>   
> +		pixelvalve0: pixelvalve@7e206000 {
> +			compatible = "brcm,bcm2711-pixelvalve0";
> +			reg = <0x7e206000 0x100>;
> +			interrupts = <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>;
> +			status = "disabled";
> +		};
> +
> +		pixelvalve1: pixelvalve@7e207000 {
> +			compatible = "brcm,bcm2711-pixelvalve1";
> +			reg = <0x7e207000 0x100>;
> +			interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
> +			status = "disabled";
> +		};
> +
> +		pixelvalve2: pixelvalve@7e20a000 {
> +			compatible = "brcm,bcm2711-pixelvalve2";
> +			reg = <0x7e20a000 0x100>;
> +			interrupts = <GIC_SPI 101 IRQ_TYPE_LEVEL_HIGH>;
> +			status = "disabled";
> +		};
> +
>   		pwm1: pwm@7e20c800 {
>   			compatible = "brcm,bcm2835-pwm";
>   			reg = <0x7e20c800 0x28>;
> @@ -248,10 +281,25 @@
>   			status = "disabled";
>   		};
>   
> -		hvs@7e400000 {
> +		pixelvalve4: pixelvalve@7e216000 {
> +			compatible = "brcm,bcm2711-pixelvalve4";
> +			reg = <0x7e216000 0x100>;
> +			interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
> +			status = "disabled";
> +		};
> +
> +		hvs: hvs@7e400000 {
> +			compatible = "brcm,bcm2711-hvs";
>   			interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
>   		};
>   
> +		pixelvalve3: pixelvalve@7ec12000 {
> +			compatible = "brcm,bcm2711-pixelvalve3";
> +			reg = <0x7ec12000 0x100>;
> +			interrupts = <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>;
> +			status = "disabled";
> +		};
> +
>   		dvp: clock@7ef00000 {
>   			compatible = "brcm,brcm2711-dvp";
>   			reg = <0x7ef00000 0x10>;
> @@ -259,6 +307,78 @@
>   			#clock-cells = <1>;
>   			#reset-cells = <1>;
>   		};
> +
> +		hdmi0: hdmi@7ef00700 {
> +			compatible = "brcm,bcm2711-hdmi0";
> +			reg = <0x7ef00700 0x300>,
> +			      <0x7ef00300 0x200>,
> +			      <0x7ef00f00 0x80>,
> +			      <0x7ef00f80 0x80>,
> +			      <0x7ef01b00 0x200>,
> +			      <0x7ef01f00 0x400>,
> +			      <0x7ef00200 0x80>,
> +			      <0x7ef04300 0x100>,
> +			      <0x7ef20000 0x100>;
> +			reg-names = "hdmi",
> +				    "dvp",
> +				    "phy",
> +				    "rm",
> +				    "packet",
> +				    "metadata",
> +				    "csc",
> +				    "cec",
> +				    "hd";
> +			clock-names = "hdmi", "bvb", "audio", "cec";
> +			resets = <&dvp 0>;
> +			ddc = <&ddc0>;
> +			dmas = <&dma 10>;
> +			dma-names = "audio-rx";
> +			status = "disabled";
> +		};
> +
> +		ddc0: i2c@7ef04500 {
> +			compatible = "brcm,bcm2711-hdmi-i2c";
> +			reg = <0x7ef04500 0x100>, <0x7ef00b00 0x300>;
> +			reg-names = "bsc", "auto-i2c";
> +			clock-frequency = <97500>;
> +			status = "disabled";
> +		};
> +
> +		hdmi1: hdmi@7ef05700 {
> +			compatible = "brcm,bcm2711-hdmi1";
> +			reg = <0x7ef05700 0x300>,
> +			      <0x7ef05300 0x200>,
> +			      <0x7ef05f00 0x80>,
> +			      <0x7ef05f80 0x80>,
> +			      <0x7ef06b00 0x200>,
> +			      <0x7ef06f00 0x400>,
> +			      <0x7ef00280 0x80>,
> +			      <0x7ef09300 0x100>,
> +			      <0x7ef20000 0x100>;
> +			reg-names = "hdmi",
> +				    "dvp",
> +				    "phy",
> +				    "rm",
> +				    "packet",
> +				    "metadata",
> +				    "csc",
> +				    "cec",
> +				    "hd";
> +			ddc = <&ddc1>;
> +			clock-names = "hdmi", "bvb", "audio", "cec";
> +			resets = <&dvp 1>;
> +			dmas = <&dma 17>;
> +			dma-names = "audio-rx";
> +			status = "disabled";
> +		};
> +
> +		ddc1: i2c@7ef09500 {
> +			compatible = "brcm,bcm2711-hdmi-i2c";
> +			reg = <0x7ef09500 0x100>, <0x7ef05b00 0x300>;
> +			reg-names = "bsc", "auto-i2c";
> +			clock-frequency = <97500>;
> +			status = "disabled";
> +		};
>   	};
>   
>   	/*
Nicolas Saenz Julienne Sept. 8, 2020, 4:31 p.m. UTC | #2
On Thu, 2020-09-03 at 10:01 +0200, Maxime Ripard wrote:
> Now that all the drivers have been adjusted for it, let's bring in the
> necessary device tree changes.
> 
> The VEC and PV3 are left out for now, since it will require a more specific
> clock setup.
> 
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Tested-by: Chanwoo Choi <cw00.choi@samsung.com>
> Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Applied for-next.

Thanks!
Nicolas
Nathan Chancellor Sept. 29, 2020, 10:15 p.m. UTC | #3
On Thu, Sep 03, 2020 at 10:01:52AM +0200, Maxime Ripard wrote:
> Now that all the drivers have been adjusted for it, let's bring in the
> necessary device tree changes.
> 
> The VEC and PV3 are left out for now, since it will require a more specific
> clock setup.
> 
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Tested-by: Chanwoo Choi <cw00.choi@samsung.com>
> Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Apologies if this has already been reported or have a solution but this
patch (and presumably series) breaks output to the serial console after
a certain point during init. On Raspbian, I see systemd startup messages
then the output just turns into complete garbage. It looks like this
patch is merged first in linux-next, which is why my bisect fell on the
DRM merge. I am happy to provide whatever information could be helpful
for debugging this. I am on the latest version of the firmware
(currently 26620cc9a63c6cb9965374d509479b4ee2c30241).

$ git bisect log
# bad: [49e7e3e905e437a02782019570f70997e2da9101] Add linux-next specific files for 20200929
# good: [fb0155a09b0224a7147cb07a4ce6034c8d29667f] Merge tag 'nfs-for-5.9-3' of git://git.linux-nfs.org/projects/trondmy/linux-nfs
git bisect start '49e7e3e905e437a02782019570f70997e2da9101' 'fb0155a09b0224a7147cb07a4ce6034c8d29667f'
# good: [a07bf9042465c0e4ab28947daf70517f99ef021f] Merge remote-tracking branch 'bluetooth/master' into master
git bisect good a07bf9042465c0e4ab28947daf70517f99ef021f
# bad: [546d06883722dfc5823d53042b924f4b4f76a459] Merge remote-tracking branch 'spi/for-next' into master
git bisect bad 546d06883722dfc5823d53042b924f4b4f76a459
# good: [06c14f5c2d311100a447caf60ecbcf558e4e60fe] Merge tag 'mediatek-drm-next-5.10' of https://git.kernel.org/pub/scm/linux/kernel/git/chunkuang.hu/linux into drm-next
git bisect good 06c14f5c2d311100a447caf60ecbcf558e4e60fe
# bad: [606865c11f2ed6429c7eddcbc59d3295771d41a4] Merge remote-tracking branch 'sound-asoc/for-next' into master
git bisect bad 606865c11f2ed6429c7eddcbc59d3295771d41a4
# bad: [7492f2f52acc72a1d08ad1f1d722237ba66189b9] Merge remote-tracking branch 'regmap/for-next' into master
git bisect bad 7492f2f52acc72a1d08ad1f1d722237ba66189b9
# good: [0b7e44d39c8aa7536352b57af2265e92fc253e4f] integrity: Asymmetric digsig supports SM2-with-SM3 algorithm
git bisect good 0b7e44d39c8aa7536352b57af2265e92fc253e4f
# good: [2ce595ba1cd7a2bc053fcc937b7bbbf743c21818] Merge remote-tracking branch 'nand/nand/next' into master
git bisect good 2ce595ba1cd7a2bc053fcc937b7bbbf743c21818
# good: [10e07ca312548f90d5e0fc1d862209285c9a858c] gpu/drm/radeon: fix spelling typo in comments
git bisect good 10e07ca312548f90d5e0fc1d862209285c9a858c
# bad: [be877462417f05af69729a9eeda1332b15e81de8] Merge remote-tracking branch 'imx-drm/imx-drm/next' into master
git bisect bad be877462417f05af69729a9eeda1332b15e81de8
# bad: [7a80fa2ada067aa633a91bce67f6c3e39aad7504] Merge remote-tracking branch 'drm-intel/for-linux-next' into master
git bisect bad 7a80fa2ada067aa633a91bce67f6c3e39aad7504
# good: [f4a336053725b689a65021edca26f8d058c0d6b4] drm/amdgpu/display: fix CFLAGS setup for DCN30
git bisect good f4a336053725b689a65021edca26f8d058c0d6b4
# bad: [64e05a0ebd7e6047c9f67c685fe8d4ec79a397ba] Merge remote-tracking branch 'drm/drm-next' into master
git bisect bad 64e05a0ebd7e6047c9f67c685fe8d4ec79a397ba
# good: [a4b0c1f80de8ec3f473b02918556650b683f044d] Merge remote-tracking branch 'crypto/master' into master
git bisect good a4b0c1f80de8ec3f473b02918556650b683f044d
# first bad commit: [64e05a0ebd7e6047c9f67c685fe8d4ec79a397ba] Merge remote-tracking branch 'drm/drm-next' into master

Cheers,
Nathan
Maxime Ripard Sept. 30, 2020, 2:07 p.m. UTC | #4
Hi Nathan,

On Tue, Sep 29, 2020 at 03:15:26PM -0700, Nathan Chancellor wrote:
> On Thu, Sep 03, 2020 at 10:01:52AM +0200, Maxime Ripard wrote:
> > Now that all the drivers have been adjusted for it, let's bring in the
> > necessary device tree changes.
> > 
> > The VEC and PV3 are left out for now, since it will require a more specific
> > clock setup.
> > 
> > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Tested-by: Chanwoo Choi <cw00.choi@samsung.com>
> > Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> > Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> 
> Apologies if this has already been reported or have a solution but this
> patch (and presumably series) breaks output to the serial console after
> a certain point during init. On Raspbian, I see systemd startup messages
> then the output just turns into complete garbage. It looks like this
> patch is merged first in linux-next, which is why my bisect fell on the
> DRM merge. I am happy to provide whatever information could be helpful
> for debugging this. I am on the latest version of the firmware
> (currently 26620cc9a63c6cb9965374d509479b4ee2c30241).

Unfortunately, the miniUART is in the same clock tree than the core
clock and will thus have those kind of issues when the core clock is
changed (which is also something that one should expect when using the
DRM or other drivers).

The only real workaround there would be to switch to one of the PL011
UARTs. I guess we can also somehow make the UART react to the core clock
frequency changes, but that's going to require some effort

Maxime
Nathan Chancellor Sept. 30, 2020, 4:38 p.m. UTC | #5
On Wed, Sep 30, 2020 at 04:07:58PM +0200, Maxime Ripard wrote:
> Hi Nathan,
> 
> On Tue, Sep 29, 2020 at 03:15:26PM -0700, Nathan Chancellor wrote:
> > On Thu, Sep 03, 2020 at 10:01:52AM +0200, Maxime Ripard wrote:
> > > Now that all the drivers have been adjusted for it, let's bring in the
> > > necessary device tree changes.
> > > 
> > > The VEC and PV3 are left out for now, since it will require a more specific
> > > clock setup.
> > > 
> > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > Tested-by: Chanwoo Choi <cw00.choi@samsung.com>
> > > Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> > > Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
> > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > 
> > Apologies if this has already been reported or have a solution but this
> > patch (and presumably series) breaks output to the serial console after
> > a certain point during init. On Raspbian, I see systemd startup messages
> > then the output just turns into complete garbage. It looks like this
> > patch is merged first in linux-next, which is why my bisect fell on the
> > DRM merge. I am happy to provide whatever information could be helpful
> > for debugging this. I am on the latest version of the firmware
> > (currently 26620cc9a63c6cb9965374d509479b4ee2c30241).
> 
> Unfortunately, the miniUART is in the same clock tree than the core
> clock and will thus have those kind of issues when the core clock is
> changed (which is also something that one should expect when using the
> DRM or other drivers).
> 
> The only real workaround there would be to switch to one of the PL011
> UARTs. I guess we can also somehow make the UART react to the core clock
> frequency changes, but that's going to require some effort
> 
> Maxime

Ack, thank you for the reply! There does not really seem to be a whole
ton of documentation around using one of the other PL011 UARTs so for
now, I will just revert this commit locally.

Cheers,
Nathan
Stefan Wahren Sept. 30, 2020, 4:52 p.m. UTC | #6
Hi,

Am 30.09.20 um 18:38 schrieb Nathan Chancellor:
> On Wed, Sep 30, 2020 at 04:07:58PM +0200, Maxime Ripard wrote:
>> Hi Nathan,
>>
>> On Tue, Sep 29, 2020 at 03:15:26PM -0700, Nathan Chancellor wrote:
>>> On Thu, Sep 03, 2020 at 10:01:52AM +0200, Maxime Ripard wrote:
>>>> Now that all the drivers have been adjusted for it, let's bring in the
>>>> necessary device tree changes.
>>>>
>>>> The VEC and PV3 are left out for now, since it will require a more specific
>>>> clock setup.
>>>>
>>>> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>>> Tested-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>> Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
>>>> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
>>>> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>>> Apologies if this has already been reported or have a solution but this
>>> patch (and presumably series) breaks output to the serial console after
>>> a certain point during init. On Raspbian, I see systemd startup messages
>>> then the output just turns into complete garbage. It looks like this
>>> patch is merged first in linux-next, which is why my bisect fell on the
>>> DRM merge. I am happy to provide whatever information could be helpful
>>> for debugging this. I am on the latest version of the firmware
>>> (currently 26620cc9a63c6cb9965374d509479b4ee2c30241).
>> Unfortunately, the miniUART is in the same clock tree than the core
>> clock and will thus have those kind of issues when the core clock is
>> changed (which is also something that one should expect when using the
>> DRM or other drivers).
>>
>> The only real workaround there would be to switch to one of the PL011
>> UARTs. I guess we can also somehow make the UART react to the core clock
>> frequency changes, but that's going to require some effort
>>
>> Maxime
> Ack, thank you for the reply! There does not really seem to be a whole
> ton of documentation around using one of the other PL011 UARTs so for
> now, I will just revert this commit locally.

there was a patch series & discussion about this topic, but we finally
didn't find a rock solid solution.

You can have a look at "[RFC 5/5] serial: 8250: bcm2835aux: add notifier
to follow clock changes" from 3.4.2019 on linux-rpi-kernel.

Best regards

>
> Cheers,
> Nathan
Maxime Ripard Oct. 1, 2020, 6:48 a.m. UTC | #7
Hi Stefan,

On Wed, Sep 30, 2020 at 06:52:13PM +0200, Stefan Wahren wrote:
> Am 30.09.20 um 18:38 schrieb Nathan Chancellor:
> > On Wed, Sep 30, 2020 at 04:07:58PM +0200, Maxime Ripard wrote:
> >> Hi Nathan,
> >>
> >> On Tue, Sep 29, 2020 at 03:15:26PM -0700, Nathan Chancellor wrote:
> >>> On Thu, Sep 03, 2020 at 10:01:52AM +0200, Maxime Ripard wrote:
> >>>> Now that all the drivers have been adjusted for it, let's bring in the
> >>>> necessary device tree changes.
> >>>>
> >>>> The VEC and PV3 are left out for now, since it will require a more specific
> >>>> clock setup.
> >>>>
> >>>> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >>>> Tested-by: Chanwoo Choi <cw00.choi@samsung.com>
> >>>> Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> >>>> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
> >>>> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> >>> Apologies if this has already been reported or have a solution but this
> >>> patch (and presumably series) breaks output to the serial console after
> >>> a certain point during init. On Raspbian, I see systemd startup messages
> >>> then the output just turns into complete garbage. It looks like this
> >>> patch is merged first in linux-next, which is why my bisect fell on the
> >>> DRM merge. I am happy to provide whatever information could be helpful
> >>> for debugging this. I am on the latest version of the firmware
> >>> (currently 26620cc9a63c6cb9965374d509479b4ee2c30241).
> >> Unfortunately, the miniUART is in the same clock tree than the core
> >> clock and will thus have those kind of issues when the core clock is
> >> changed (which is also something that one should expect when using the
> >> DRM or other drivers).
> >>
> >> The only real workaround there would be to switch to one of the PL011
> >> UARTs. I guess we can also somehow make the UART react to the core clock
> >> frequency changes, but that's going to require some effort
> >>
> >> Maxime
> > Ack, thank you for the reply! There does not really seem to be a whole
> > ton of documentation around using one of the other PL011 UARTs so for
> > now, I will just revert this commit locally.
> 
> there was a patch series & discussion about this topic, but we finally
> didn't find a rock solid solution.
> 
> You can have a look at "[RFC 5/5] serial: 8250: bcm2835aux: add notifier
> to follow clock changes" from 3.4.2019 on linux-rpi-kernel.

I couldn't find that discussion on the archive, but based on the title I
guess there's some patches that have been merged this cycle for the 8250
driver to do just that (868f3ee6e452 ("serial: 8250: Add 8250 port clock
update method") and cc816969d7b5 ("serial: 8250_dw: Fix common clocks
usage race condition")).

However, I'm not entirely sure the clock notifier works in our case with
the firmware / MMIO clocks duality

Maxime
Maxime Ripard Oct. 1, 2020, 8:54 a.m. UTC | #8
On Thu, Oct 01, 2020 at 08:48:43AM +0200, Maxime Ripard wrote:
> Hi Stefan,
> 
> On Wed, Sep 30, 2020 at 06:52:13PM +0200, Stefan Wahren wrote:
> > Am 30.09.20 um 18:38 schrieb Nathan Chancellor:
> > > On Wed, Sep 30, 2020 at 04:07:58PM +0200, Maxime Ripard wrote:
> > >> Hi Nathan,
> > >>
> > >> On Tue, Sep 29, 2020 at 03:15:26PM -0700, Nathan Chancellor wrote:
> > >>> On Thu, Sep 03, 2020 at 10:01:52AM +0200, Maxime Ripard wrote:
> > >>>> Now that all the drivers have been adjusted for it, let's bring in the
> > >>>> necessary device tree changes.
> > >>>>
> > >>>> The VEC and PV3 are left out for now, since it will require a more specific
> > >>>> clock setup.
> > >>>>
> > >>>> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > >>>> Tested-by: Chanwoo Choi <cw00.choi@samsung.com>
> > >>>> Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> > >>>> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
> > >>>> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > >>> Apologies if this has already been reported or have a solution but this
> > >>> patch (and presumably series) breaks output to the serial console after
> > >>> a certain point during init. On Raspbian, I see systemd startup messages
> > >>> then the output just turns into complete garbage. It looks like this
> > >>> patch is merged first in linux-next, which is why my bisect fell on the
> > >>> DRM merge. I am happy to provide whatever information could be helpful
> > >>> for debugging this. I am on the latest version of the firmware
> > >>> (currently 26620cc9a63c6cb9965374d509479b4ee2c30241).
> > >> Unfortunately, the miniUART is in the same clock tree than the core
> > >> clock and will thus have those kind of issues when the core clock is
> > >> changed (which is also something that one should expect when using the
> > >> DRM or other drivers).
> > >>
> > >> The only real workaround there would be to switch to one of the PL011
> > >> UARTs. I guess we can also somehow make the UART react to the core clock
> > >> frequency changes, but that's going to require some effort
> > >>
> > >> Maxime
> > > Ack, thank you for the reply! There does not really seem to be a whole
> > > ton of documentation around using one of the other PL011 UARTs so for
> > > now, I will just revert this commit locally.
> > 
> > there was a patch series & discussion about this topic, but we finally
> > didn't find a rock solid solution.
> > 
> > You can have a look at "[RFC 5/5] serial: 8250: bcm2835aux: add notifier
> > to follow clock changes" from 3.4.2019 on linux-rpi-kernel.
> 
> I couldn't find that discussion on the archive, but based on the title I
> guess there's some patches that have been merged this cycle for the 8250
> driver to do just that (868f3ee6e452 ("serial: 8250: Add 8250 port clock
> update method") and cc816969d7b5 ("serial: 8250_dw: Fix common clocks
> usage race condition")).
> 
> However, I'm not entirely sure the clock notifier works in our case with
> the firmware / MMIO clocks duality

I was a bit intrigued by this, so I looked into it, and it seems that
it's worth that it used to be. The core clock is supposed to be running
at 500 Mhz in most cases, and that's what we're setting so it shouldn't
cause any pratical issue.

However, it looks like on my board now the firmware reports that the
core clock is running at either 311MHz or 233MHz with hdmi_enable_4k60
(which seems odd?) and that contradicts the documentation here:
https://www.raspberrypi.org/documentation/configuration/config-txt/overclocking.md

Linux then comes in, changes the frequency to 500MHz and breaks the
UART. So either the doc is wrong, or the clock driver is.

vcgencmd measure_clock core reports that it's indeed 233Mhz and I'm
running a year-old firmware (built on the 2019-11-29), so I'd be
inclined to think that the doc is wrong here or we're misinterpreting
something.

Dave, Tim, any idea?

Thanks!
Maxime
Nicolas Saenz Julienne Oct. 1, 2020, 9:22 a.m. UTC | #9
On Wed, 2020-09-30 at 09:38 -0700, Nathan Chancellor wrote:
> On Wed, Sep 30, 2020 at 04:07:58PM +0200, Maxime Ripard wrote:
> > Hi Nathan,
> > 
> > On Tue, Sep 29, 2020 at 03:15:26PM -0700, Nathan Chancellor wrote:
> > > On Thu, Sep 03, 2020 at 10:01:52AM +0200, Maxime Ripard wrote:
> > > > Now that all the drivers have been adjusted for it, let's bring in the
> > > > necessary device tree changes.
> > > > 
> > > > The VEC and PV3 are left out for now, since it will require a more specific
> > > > clock setup.
> > > > 
> > > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > Tested-by: Chanwoo Choi <cw00.choi@samsung.com>
> > > > Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> > > > Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
> > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > > 
> > > Apologies if this has already been reported or have a solution but this
> > > patch (and presumably series) breaks output to the serial console after
> > > a certain point during init. On Raspbian, I see systemd startup messages
> > > then the output just turns into complete garbage. It looks like this
> > > patch is merged first in linux-next, which is why my bisect fell on the
> > > DRM merge. I am happy to provide whatever information could be helpful
> > > for debugging this. I am on the latest version of the firmware
> > > (currently 26620cc9a63c6cb9965374d509479b4ee2c30241).
> > 
> > Unfortunately, the miniUART is in the same clock tree than the core
> > clock and will thus have those kind of issues when the core clock is
> > changed (which is also something that one should expect when using the
> > DRM or other drivers).
> > 
> > The only real workaround there would be to switch to one of the PL011
> > UARTs. I guess we can also somehow make the UART react to the core clock
> > frequency changes, but that's going to require some effort
> > 
> > Maxime
> 
> Ack, thank you for the reply! There does not really seem to be a whole
> ton of documentation around using one of the other PL011 UARTs so for
> now, I will just revert this commit locally.

Nathan, a less intrusive solution would be to add 'core_freq_min=500' into your
config.txt.

Funnily enough core_freq=500 doesn't seem to work in that regard. It might be
related with what Maxime is commenting.

Regards,
Nicolas
Maxime Ripard Oct. 1, 2020, 9:33 a.m. UTC | #10
On Thu, Oct 01, 2020 at 11:22:03AM +0200, Nicolas Saenz Julienne wrote:
> On Wed, 2020-09-30 at 09:38 -0700, Nathan Chancellor wrote:
> > On Wed, Sep 30, 2020 at 04:07:58PM +0200, Maxime Ripard wrote:
> > > Hi Nathan,
> > > 
> > > On Tue, Sep 29, 2020 at 03:15:26PM -0700, Nathan Chancellor wrote:
> > > > On Thu, Sep 03, 2020 at 10:01:52AM +0200, Maxime Ripard wrote:
> > > > > Now that all the drivers have been adjusted for it, let's bring in the
> > > > > necessary device tree changes.
> > > > > 
> > > > > The VEC and PV3 are left out for now, since it will require a more specific
> > > > > clock setup.
> > > > > 
> > > > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > > Tested-by: Chanwoo Choi <cw00.choi@samsung.com>
> > > > > Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> > > > > Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
> > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > > > 
> > > > Apologies if this has already been reported or have a solution but this
> > > > patch (and presumably series) breaks output to the serial console after
> > > > a certain point during init. On Raspbian, I see systemd startup messages
> > > > then the output just turns into complete garbage. It looks like this
> > > > patch is merged first in linux-next, which is why my bisect fell on the
> > > > DRM merge. I am happy to provide whatever information could be helpful
> > > > for debugging this. I am on the latest version of the firmware
> > > > (currently 26620cc9a63c6cb9965374d509479b4ee2c30241).
> > > 
> > > Unfortunately, the miniUART is in the same clock tree than the core
> > > clock and will thus have those kind of issues when the core clock is
> > > changed (which is also something that one should expect when using the
> > > DRM or other drivers).
> > > 
> > > The only real workaround there would be to switch to one of the PL011
> > > UARTs. I guess we can also somehow make the UART react to the core clock
> > > frequency changes, but that's going to require some effort
> > > 
> > > Maxime
> > 
> > Ack, thank you for the reply! There does not really seem to be a whole
> > ton of documentation around using one of the other PL011 UARTs so for
> > now, I will just revert this commit locally.
> 
> Nathan, a less intrusive solution would be to add 'core_freq_min=500' into your
> config.txt.
> 
> Funnily enough core_freq=500 doesn't seem to work in that regard. It might be
> related with what Maxime is commenting.

Yeah, it fixes it here too

Maxime
Tim Gover Oct. 1, 2020, 10:15 a.m. UTC | #11
hdmi_enable_4k60=1 causes the firmware to select 3.3 GHz for the PLLC
VCO to support a core-frequency of 550 MHz which is the minimum
frequency required by the HVS at 4Kp60. The side effect is that if the
display clock requirements are lower than 4Kp60 then you will see
different core frequencies selected by DVFS.

If enable_uart=1 and the mini-uart is selected (default unless
bluetooth is disabled) then the firmware will pin the core-frequency
to either core_freq max (500 or 550). Although, I think there is a way
of pinning it to a lower fixed frequency.

The table in overclocking.md defines options for setting the maximum
core frequency but unless core_freq_min is specified DVFS will
automatically pick the lowest idle frequency required by the display
resolution.



On Thu, 1 Oct 2020 at 09:54, Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Thu, Oct 01, 2020 at 08:48:43AM +0200, Maxime Ripard wrote:
> > Hi Stefan,
> >
> > On Wed, Sep 30, 2020 at 06:52:13PM +0200, Stefan Wahren wrote:
> > > Am 30.09.20 um 18:38 schrieb Nathan Chancellor:
> > > > On Wed, Sep 30, 2020 at 04:07:58PM +0200, Maxime Ripard wrote:
> > > >> Hi Nathan,
> > > >>
> > > >> On Tue, Sep 29, 2020 at 03:15:26PM -0700, Nathan Chancellor wrote:
> > > >>> On Thu, Sep 03, 2020 at 10:01:52AM +0200, Maxime Ripard wrote:
> > > >>>> Now that all the drivers have been adjusted for it, let's bring in the
> > > >>>> necessary device tree changes.
> > > >>>>
> > > >>>> The VEC and PV3 are left out for now, since it will require a more specific
> > > >>>> clock setup.
> > > >>>>
> > > >>>> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > >>>> Tested-by: Chanwoo Choi <cw00.choi@samsung.com>
> > > >>>> Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> > > >>>> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
> > > >>>> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > > >>> Apologies if this has already been reported or have a solution but this
> > > >>> patch (and presumably series) breaks output to the serial console after
> > > >>> a certain point during init. On Raspbian, I see systemd startup messages
> > > >>> then the output just turns into complete garbage. It looks like this
> > > >>> patch is merged first in linux-next, which is why my bisect fell on the
> > > >>> DRM merge. I am happy to provide whatever information could be helpful
> > > >>> for debugging this. I am on the latest version of the firmware
> > > >>> (currently 26620cc9a63c6cb9965374d509479b4ee2c30241).
> > > >> Unfortunately, the miniUART is in the same clock tree than the core
> > > >> clock and will thus have those kind of issues when the core clock is
> > > >> changed (which is also something that one should expect when using the
> > > >> DRM or other drivers).
> > > >>
> > > >> The only real workaround there would be to switch to one of the PL011
> > > >> UARTs. I guess we can also somehow make the UART react to the core clock
> > > >> frequency changes, but that's going to require some effort
> > > >>
> > > >> Maxime
> > > > Ack, thank you for the reply! There does not really seem to be a whole
> > > > ton of documentation around using one of the other PL011 UARTs so for
> > > > now, I will just revert this commit locally.
> > >
> > > there was a patch series & discussion about this topic, but we finally
> > > didn't find a rock solid solution.
> > >
> > > You can have a look at "[RFC 5/5] serial: 8250: bcm2835aux: add notifier
> > > to follow clock changes" from 3.4.2019 on linux-rpi-kernel.
> >
> > I couldn't find that discussion on the archive, but based on the title I
> > guess there's some patches that have been merged this cycle for the 8250
> > driver to do just that (868f3ee6e452 ("serial: 8250: Add 8250 port clock
> > update method") and cc816969d7b5 ("serial: 8250_dw: Fix common clocks
> > usage race condition")).
> >
> > However, I'm not entirely sure the clock notifier works in our case with
> > the firmware / MMIO clocks duality
>
> I was a bit intrigued by this, so I looked into it, and it seems that
> it's worth that it used to be. The core clock is supposed to be running
> at 500 Mhz in most cases, and that's what we're setting so it shouldn't
> cause any pratical issue.
>
> However, it looks like on my board now the firmware reports that the
> core clock is running at either 311MHz or 233MHz with hdmi_enable_4k60
> (which seems odd?) and that contradicts the documentation here:
> https://www.raspberrypi.org/documentation/configuration/config-txt/overclocking.md
>
> Linux then comes in, changes the frequency to 500MHz and breaks the
> UART. So either the doc is wrong, or the clock driver is.
>
> vcgencmd measure_clock core reports that it's indeed 233Mhz and I'm
> running a year-old firmware (built on the 2019-11-29), so I'd be
> inclined to think that the doc is wrong here or we're misinterpreting
> something.
>
> Dave, Tim, any idea?
>
> Thanks!
> Maxime
Nicolas Saenz Julienne Oct. 1, 2020, 4:47 p.m. UTC | #12
Hi Tim, thanks for the info!

On Thu, 2020-10-01 at 11:15 +0100, Tim Gover wrote:
> hdmi_enable_4k60=1 causes the firmware to select 3.3 GHz for the PLLC
> VCO to support a core-frequency of 550 MHz which is the minimum
> frequency required by the HVS at 4Kp60. The side effect is that if the
> display clock requirements are lower than 4Kp60 then you will see
> different core frequencies selected by DVFS.
> 
> If enable_uart=1 and the mini-uart is selected (default unless

What is the actual test made to check if mini-uart is selected? I can't get
firmware to trigger this behaviour with 64-bit upstream kernel/dts. Note that I
see the core clk setup at 200MHz just before having VC4 set it to 500MHz.

The only thing I've got on my config.txt is:

enable_uart=1
arm_64bit=1

Maybe we're missing some kind of DT alias upstream?

Regards,
Nicolas

> bluetooth is disabled) then the firmware will pin the core-frequency
> to either core_freq max (500 or 550). Although, I think there is a way
> of pinning it to a lower fixed frequency.
> 
> The table in overclocking.md defines options for setting the maximum
> core frequency but unless core_freq_min is specified DVFS will
> automatically pick the lowest idle frequency required by the display
> resolution.
Nathan Chancellor Oct. 1, 2020, 6:09 p.m. UTC | #13
On Thu, Oct 01, 2020 at 11:22:03AM +0200, Nicolas Saenz Julienne wrote:
> On Wed, 2020-09-30 at 09:38 -0700, Nathan Chancellor wrote:
> > On Wed, Sep 30, 2020 at 04:07:58PM +0200, Maxime Ripard wrote:
> > > Hi Nathan,
> > > 
> > > On Tue, Sep 29, 2020 at 03:15:26PM -0700, Nathan Chancellor wrote:
> > > > On Thu, Sep 03, 2020 at 10:01:52AM +0200, Maxime Ripard wrote:
> > > > > Now that all the drivers have been adjusted for it, let's bring in the
> > > > > necessary device tree changes.
> > > > > 
> > > > > The VEC and PV3 are left out for now, since it will require a more specific
> > > > > clock setup.
> > > > > 
> > > > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > > Tested-by: Chanwoo Choi <cw00.choi@samsung.com>
> > > > > Tested-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> > > > > Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
> > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > > > 
> > > > Apologies if this has already been reported or have a solution but this
> > > > patch (and presumably series) breaks output to the serial console after
> > > > a certain point during init. On Raspbian, I see systemd startup messages
> > > > then the output just turns into complete garbage. It looks like this
> > > > patch is merged first in linux-next, which is why my bisect fell on the
> > > > DRM merge. I am happy to provide whatever information could be helpful
> > > > for debugging this. I am on the latest version of the firmware
> > > > (currently 26620cc9a63c6cb9965374d509479b4ee2c30241).
> > > 
> > > Unfortunately, the miniUART is in the same clock tree than the core
> > > clock and will thus have those kind of issues when the core clock is
> > > changed (which is also something that one should expect when using the
> > > DRM or other drivers).
> > > 
> > > The only real workaround there would be to switch to one of the PL011
> > > UARTs. I guess we can also somehow make the UART react to the core clock
> > > frequency changes, but that's going to require some effort
> > > 
> > > Maxime
> > 
> > Ack, thank you for the reply! There does not really seem to be a whole
> > ton of documentation around using one of the other PL011 UARTs so for
> > now, I will just revert this commit locally.
> 
> Nathan, a less intrusive solution would be to add 'core_freq_min=500' into your
> config.txt.
> 
> Funnily enough core_freq=500 doesn't seem to work in that regard. It might be
> related with what Maxime is commenting.
> 
> Regards,
> Nicolas
> 

Excellent, thank you for the tip, that worked well!

Cheers,
Nathan
Tim Gover Oct. 1, 2020, 7:45 p.m. UTC | #14
Sorry, my previous statement was misleading.

enable_uart will select the mini_uart for gpio14,15 unless the
disable-bt device tree overlay is loaded. As well as disabling
bluetooth disable-bt swaps the uart0 pin configs to point the regular
UART to gpio 14,15. After resolving the DT overlays the firmware does
the initial UART setup according to which controller is pointed at
pins 14,15.

I'll have to speak to others about exactly when the fixing of the core
clock takes effect. There have been a few changes related to the
initial turbo frequency configuration and how this is reported via the
mbox APIs

On Thu, 1 Oct 2020 at 17:47, Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> Hi Tim, thanks for the info!
>
> On Thu, 2020-10-01 at 11:15 +0100, Tim Gover wrote:
> > hdmi_enable_4k60=1 causes the firmware to select 3.3 GHz for the PLLC
> > VCO to support a core-frequency of 550 MHz which is the minimum
> > frequency required by the HVS at 4Kp60. The side effect is that if the
> > display clock requirements are lower than 4Kp60 then you will see
> > different core frequencies selected by DVFS.
> >
> > If enable_uart=1 and the mini-uart is selected (default unless
>
> What is the actual test made to check if mini-uart is selected? I can't get
> firmware to trigger this behaviour with 64-bit upstream kernel/dts. Note that I
> see the core clk setup at 200MHz just before having VC4 set it to 500MHz.
>
> The only thing I've got on my config.txt is:
>
> enable_uart=1
> arm_64bit=1
>
> Maybe we're missing some kind of DT alias upstream?
>
> Regards,
> Nicolas
>
> > bluetooth is disabled) then the firmware will pin the core-frequency
> > to either core_freq max (500 or 550). Although, I think there is a way
> > of pinning it to a lower fixed frequency.
> >
> > The table in overclocking.md defines options for setting the maximum
> > core frequency but unless core_freq_min is specified DVFS will
> > automatically pick the lowest idle frequency required by the display
> > resolution.
>
Maxime Ripard Oct. 2, 2020, 3:19 p.m. UTC | #15
Hi Tim,

On Thu, Oct 01, 2020 at 11:15:46AM +0100, Tim Gover wrote:
> hdmi_enable_4k60=1 causes the firmware to select 3.3 GHz for the PLLC
> VCO to support a core-frequency of 550 MHz which is the minimum
> frequency required by the HVS at 4Kp60. The side effect is that if the
> display clock requirements are lower than 4Kp60 then you will see
> different core frequencies selected by DVFS.
> 
> If enable_uart=1 and the mini-uart is selected (default unless
> bluetooth is disabled) then the firmware will pin the core-frequency
> to either core_freq max (500 or 550). Although, I think there is a way
> of pinning it to a lower fixed frequency.
> 
> The table in overclocking.md defines options for setting the maximum
> core frequency but unless core_freq_min is specified DVFS will
> automatically pick the lowest idle frequency required by the display
> resolution.

I'm wondering if there's some way to detect this from Linux? I guess it
would be nice to be able to at least detect a broken config to warn /
prevent an user that their situation is not going to be reliable / work
really well (like if they have a 4k display without hdmi_enable_4kp60
set, or the issue we're discussing here)

Thanks!
Maxime
Dave Stevenson Oct. 2, 2020, 3:57 p.m. UTC | #16
Hi Maxime

On Fri, 2 Oct 2020 at 16:19, Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi Tim,
>
> On Thu, Oct 01, 2020 at 11:15:46AM +0100, Tim Gover wrote:
> > hdmi_enable_4k60=1 causes the firmware to select 3.3 GHz for the PLLC
> > VCO to support a core-frequency of 550 MHz which is the minimum
> > frequency required by the HVS at 4Kp60. The side effect is that if the
> > display clock requirements are lower than 4Kp60 then you will see
> > different core frequencies selected by DVFS.
> >
> > If enable_uart=1 and the mini-uart is selected (default unless
> > bluetooth is disabled) then the firmware will pin the core-frequency
> > to either core_freq max (500 or 550). Although, I think there is a way
> > of pinning it to a lower fixed frequency.
> >
> > The table in overclocking.md defines options for setting the maximum
> > core frequency but unless core_freq_min is specified DVFS will
> > automatically pick the lowest idle frequency required by the display
> > resolution.
>
> I'm wondering if there's some way to detect this from Linux? I guess it
> would be nice to be able to at least detect a broken config to warn /
> prevent an user that their situation is not going to be reliable / work
> really well (like if they have a 4k display without hdmi_enable_4kp60
> set, or the issue we're discussing here)

The main filter in the firmware is the parameter
hdmi_pixel_freq_limit. That can either be set manually from
config.txt, or defaults appropriately based on hdmi_enable_4kp60.
Under firmware_kms [1] I read back those values to use as a filter
within crtc_mode_valid[2].
I can't think of a nice way of exposing that without the vc4 driver
gaining a DT link to the firmware, and that starts to get ugly.

  Dave

[1] https://github.com/raspberrypi/linux/blob/rpi-5.9.y/drivers/gpu/drm/vc4/vc4_firmware_kms.c#L1859
[2] https://github.com/raspberrypi/linux/blob/rpi-5.9.y/drivers/gpu/drm/vc4/vc4_firmware_kms.c#L1077
Maxime Ripard Oct. 6, 2020, 3:26 p.m. UTC | #17
Hi Dave,

On Fri, Oct 02, 2020 at 04:57:05PM +0100, Dave Stevenson wrote:
> Hi Maxime
> 
> On Fri, 2 Oct 2020 at 16:19, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Hi Tim,
> >
> > On Thu, Oct 01, 2020 at 11:15:46AM +0100, Tim Gover wrote:
> > > hdmi_enable_4k60=1 causes the firmware to select 3.3 GHz for the PLLC
> > > VCO to support a core-frequency of 550 MHz which is the minimum
> > > frequency required by the HVS at 4Kp60. The side effect is that if the
> > > display clock requirements are lower than 4Kp60 then you will see
> > > different core frequencies selected by DVFS.
> > >
> > > If enable_uart=1 and the mini-uart is selected (default unless
> > > bluetooth is disabled) then the firmware will pin the core-frequency
> > > to either core_freq max (500 or 550). Although, I think there is a way
> > > of pinning it to a lower fixed frequency.
> > >
> > > The table in overclocking.md defines options for setting the maximum
> > > core frequency but unless core_freq_min is specified DVFS will
> > > automatically pick the lowest idle frequency required by the display
> > > resolution.
> >
> > I'm wondering if there's some way to detect this from Linux? I guess it
> > would be nice to be able to at least detect a broken config to warn /
> > prevent an user that their situation is not going to be reliable / work
> > really well (like if they have a 4k display without hdmi_enable_4kp60
> > set, or the issue we're discussing here)
> 
> The main filter in the firmware is the parameter
> hdmi_pixel_freq_limit. That can either be set manually from
> config.txt, or defaults appropriately based on hdmi_enable_4kp60.
> Under firmware_kms [1] I read back those values to use as a filter
> within crtc_mode_valid[2].
> I can't think of a nice way of exposing that without the vc4 driver
> gaining a DT link to the firmware, and that starts to get ugly.

I had in mind something like if the clock driver can infer that somehow
through some the boundaries reported by the firmware maybe? IIRC,
hdmi_enable_4kp60 will already change the max frequency reported to
550MHz instead of 500MHz

Maxime
Dave Stevenson Oct. 6, 2020, 5:14 p.m. UTC | #18
Hi Maxime

On Tue, 6 Oct 2020 at 16:26, Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi Dave,
>
> On Fri, Oct 02, 2020 at 04:57:05PM +0100, Dave Stevenson wrote:
> > Hi Maxime
> >
> > On Fri, 2 Oct 2020 at 16:19, Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > Hi Tim,
> > >
> > > On Thu, Oct 01, 2020 at 11:15:46AM +0100, Tim Gover wrote:
> > > > hdmi_enable_4k60=1 causes the firmware to select 3.3 GHz for the PLLC
> > > > VCO to support a core-frequency of 550 MHz which is the minimum
> > > > frequency required by the HVS at 4Kp60. The side effect is that if the
> > > > display clock requirements are lower than 4Kp60 then you will see
> > > > different core frequencies selected by DVFS.
> > > >
> > > > If enable_uart=1 and the mini-uart is selected (default unless
> > > > bluetooth is disabled) then the firmware will pin the core-frequency
> > > > to either core_freq max (500 or 550). Although, I think there is a way
> > > > of pinning it to a lower fixed frequency.
> > > >
> > > > The table in overclocking.md defines options for setting the maximum
> > > > core frequency but unless core_freq_min is specified DVFS will
> > > > automatically pick the lowest idle frequency required by the display
> > > > resolution.
> > >
> > > I'm wondering if there's some way to detect this from Linux? I guess it
> > > would be nice to be able to at least detect a broken config to warn /
> > > prevent an user that their situation is not going to be reliable / work
> > > really well (like if they have a 4k display without hdmi_enable_4kp60
> > > set, or the issue we're discussing here)
> >
> > The main filter in the firmware is the parameter
> > hdmi_pixel_freq_limit. That can either be set manually from
> > config.txt, or defaults appropriately based on hdmi_enable_4kp60.
> > Under firmware_kms [1] I read back those values to use as a filter
> > within crtc_mode_valid[2].
> > I can't think of a nice way of exposing that without the vc4 driver
> > gaining a DT link to the firmware, and that starts to get ugly.
>
> I had in mind something like if the clock driver can infer that somehow
> through some the boundaries reported by the firmware maybe? IIRC,
> hdmi_enable_4kp60 will already change the max frequency reported to
> 550MHz instead of 500MHz

Yes, that's plausible, but I don't know enough about the clock
infrastructure for advertising limits to know what works there.
Tell me what you need from the mailbox service and I'll see what I can do.

We do already have RPI_FIRMWARE_GET_MAX_CLOCK_RATE and
RPI_FIRMWARE_GET_MIN_CLOCK_RATE. It'd take a few minutes of staring at
the code (or a quick test) to confirm if they definitely are changed
for CORE clock by hdmi_enable_4kp60 - I think it does.

  Dave
Nicolas Saenz Julienne Oct. 8, 2020, 9:35 a.m. UTC | #19
Hi Dave, sorry for the late reply.

On Tue, 2020-10-06 at 18:14 +0100, Dave Stevenson wrote:
> Hi Maxime
> 
> On Tue, 6 Oct 2020 at 16:26, Maxime Ripard <maxime@cerno.tech> wrote:
> > Hi Dave,
> > 
> > On Fri, Oct 02, 2020 at 04:57:05PM +0100, Dave Stevenson wrote:
> > > Hi Maxime
> > > 
> > > On Fri, 2 Oct 2020 at 16:19, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > Hi Tim,
> > > > 
> > > > On Thu, Oct 01, 2020 at 11:15:46AM +0100, Tim Gover wrote:
> > > > > hdmi_enable_4k60=1 causes the firmware to select 3.3 GHz for the PLLC
> > > > > VCO to support a core-frequency of 550 MHz which is the minimum
> > > > > frequency required by the HVS at 4Kp60. The side effect is that if the
> > > > > display clock requirements are lower than 4Kp60 then you will see
> > > > > different core frequencies selected by DVFS.
> > > > > 
> > > > > If enable_uart=1 and the mini-uart is selected (default unless
> > > > > bluetooth is disabled) then the firmware will pin the core-frequency
> > > > > to either core_freq max (500 or 550). Although, I think there is a way
> > > > > of pinning it to a lower fixed frequency.
> > > > > 
> > > > > The table in overclocking.md defines options for setting the maximum
> > > > > core frequency but unless core_freq_min is specified DVFS will
> > > > > automatically pick the lowest idle frequency required by the display
> > > > > resolution.
> > > > 
> > > > I'm wondering if there's some way to detect this from Linux? I guess it
> > > > would be nice to be able to at least detect a broken config to warn /
> > > > prevent an user that their situation is not going to be reliable / work
> > > > really well (like if they have a 4k display without hdmi_enable_4kp60
> > > > set, or the issue we're discussing here)
> > > 
> > > The main filter in the firmware is the parameter
> > > hdmi_pixel_freq_limit. That can either be set manually from
> > > config.txt, or defaults appropriately based on hdmi_enable_4kp60.
> > > Under firmware_kms [1] I read back those values to use as a filter
> > > within crtc_mode_valid[2].
> > > I can't think of a nice way of exposing that without the vc4 driver
> > > gaining a DT link to the firmware, and that starts to get ugly.
> > 
> > I had in mind something like if the clock driver can infer that somehow
> > through some the boundaries reported by the firmware maybe? IIRC,
> > hdmi_enable_4kp60 will already change the max frequency reported to
> > 550MHz instead of 500MHz
> 
> Yes, that's plausible, but I don't know enough about the clock
> infrastructure for advertising limits to know what works there.
> Tell me what you need from the mailbox service and I'll see what I can do.
> 
> We do already have RPI_FIRMWARE_GET_MAX_CLOCK_RATE and
> RPI_FIRMWARE_GET_MIN_CLOCK_RATE. It'd take a few minutes of staring at
> the code (or a quick test) to confirm if they definitely are changed
> for CORE clock by hdmi_enable_4kp60 - I think it does.

Tim commented earlier that you meant to pin CORE frequency when enable_uart=1.
In practice it doesn't seem to be the case, but it would solve the UART
problem for most use cases. Would that be feasible?

If we have to change the CORE frequency during runtime we could, while we
search for a proper solution, print a warning that we're about to break the low
speed peripherals.

Regards,
Nicolas
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/bcm2711-rpi-4-b.dts b/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
index e94244a215af..09a1182c2936 100644
--- a/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
+++ b/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
@@ -70,6 +70,14 @@ 
 	};
 };
 
+&ddc0 {
+	status = "okay";
+};
+
+&ddc1 {
+	status = "okay";
+};
+
 &firmware {
 	firmware_clocks: clocks {
 		compatible = "raspberrypi,firmware-clocks";
@@ -170,6 +178,38 @@ 
 			  "RGMII_TXD3";
 };
 
+&hdmi0 {
+	clocks = <&firmware_clocks 13>, <&firmware_clocks 14>, <&dvp 0>, <&clk_27MHz>;
+	clock-names = "hdmi", "bvb", "audio", "cec";
+	status = "okay";
+};
+
+&hdmi1 {
+	clocks = <&firmware_clocks 13>, <&firmware_clocks 14>, <&dvp 1>, <&clk_27MHz>;
+	clock-names = "hdmi", "bvb", "audio", "cec";
+	status = "okay";
+};
+
+&hvs {
+	clocks = <&firmware_clocks 4>;
+};
+
+&pixelvalve0 {
+	status = "okay";
+};
+
+&pixelvalve1 {
+	status = "okay";
+};
+
+&pixelvalve2 {
+	status = "okay";
+};
+
+&pixelvalve4 {
+	status = "okay";
+};
+
 &pwm1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pwm1_0_gpio40 &pwm1_1_gpio41>;
@@ -253,3 +293,11 @@ 
 &vchiq {
 	interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>;
 };
+
+&vc4 {
+	status = "okay";
+};
+
+&vec {
+	status = "disabled";
+};
diff --git a/arch/arm/boot/dts/bcm2711.dtsi b/arch/arm/boot/dts/bcm2711.dtsi
index 00bcaed1be32..4847dd305317 100644
--- a/arch/arm/boot/dts/bcm2711.dtsi
+++ b/arch/arm/boot/dts/bcm2711.dtsi
@@ -12,6 +12,18 @@ 
 
 	interrupt-parent = <&gicv2>;
 
+	vc4: gpu {
+		compatible = "brcm,bcm2711-vc5";
+		status = "disabled";
+	};
+
+	clk_27MHz: clk-27M {
+		#clock-cells = <0>;
+		compatible = "fixed-clock";
+		clock-frequency = <27000000>;
+		clock-output-names = "27MHz-clock";
+	};
+
 	clk_108MHz: clk-108M {
 		#clock-cells = <0>;
 		compatible = "fixed-clock";
@@ -238,6 +250,27 @@ 
 			status = "disabled";
 		};
 
+		pixelvalve0: pixelvalve@7e206000 {
+			compatible = "brcm,bcm2711-pixelvalve0";
+			reg = <0x7e206000 0x100>;
+			interrupts = <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+		};
+
+		pixelvalve1: pixelvalve@7e207000 {
+			compatible = "brcm,bcm2711-pixelvalve1";
+			reg = <0x7e207000 0x100>;
+			interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+		};
+
+		pixelvalve2: pixelvalve@7e20a000 {
+			compatible = "brcm,bcm2711-pixelvalve2";
+			reg = <0x7e20a000 0x100>;
+			interrupts = <GIC_SPI 101 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+		};
+
 		pwm1: pwm@7e20c800 {
 			compatible = "brcm,bcm2835-pwm";
 			reg = <0x7e20c800 0x28>;
@@ -248,10 +281,25 @@ 
 			status = "disabled";
 		};
 
-		hvs@7e400000 {
+		pixelvalve4: pixelvalve@7e216000 {
+			compatible = "brcm,bcm2711-pixelvalve4";
+			reg = <0x7e216000 0x100>;
+			interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+		};
+
+		hvs: hvs@7e400000 {
+			compatible = "brcm,bcm2711-hvs";
 			interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
+		pixelvalve3: pixelvalve@7ec12000 {
+			compatible = "brcm,bcm2711-pixelvalve3";
+			reg = <0x7ec12000 0x100>;
+			interrupts = <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+		};
+
 		dvp: clock@7ef00000 {
 			compatible = "brcm,brcm2711-dvp";
 			reg = <0x7ef00000 0x10>;
@@ -259,6 +307,78 @@ 
 			#clock-cells = <1>;
 			#reset-cells = <1>;
 		};
+
+		hdmi0: hdmi@7ef00700 {
+			compatible = "brcm,bcm2711-hdmi0";
+			reg = <0x7ef00700 0x300>,
+			      <0x7ef00300 0x200>,
+			      <0x7ef00f00 0x80>,
+			      <0x7ef00f80 0x80>,
+			      <0x7ef01b00 0x200>,
+			      <0x7ef01f00 0x400>,
+			      <0x7ef00200 0x80>,
+			      <0x7ef04300 0x100>,
+			      <0x7ef20000 0x100>;
+			reg-names = "hdmi",
+				    "dvp",
+				    "phy",
+				    "rm",
+				    "packet",
+				    "metadata",
+				    "csc",
+				    "cec",
+				    "hd";
+			clock-names = "hdmi", "bvb", "audio", "cec";
+			resets = <&dvp 0>;
+			ddc = <&ddc0>;
+			dmas = <&dma 10>;
+			dma-names = "audio-rx";
+			status = "disabled";
+		};
+
+		ddc0: i2c@7ef04500 {
+			compatible = "brcm,bcm2711-hdmi-i2c";
+			reg = <0x7ef04500 0x100>, <0x7ef00b00 0x300>;
+			reg-names = "bsc", "auto-i2c";
+			clock-frequency = <97500>;
+			status = "disabled";
+		};
+
+		hdmi1: hdmi@7ef05700 {
+			compatible = "brcm,bcm2711-hdmi1";
+			reg = <0x7ef05700 0x300>,
+			      <0x7ef05300 0x200>,
+			      <0x7ef05f00 0x80>,
+			      <0x7ef05f80 0x80>,
+			      <0x7ef06b00 0x200>,
+			      <0x7ef06f00 0x400>,
+			      <0x7ef00280 0x80>,
+			      <0x7ef09300 0x100>,
+			      <0x7ef20000 0x100>;
+			reg-names = "hdmi",
+				    "dvp",
+				    "phy",
+				    "rm",
+				    "packet",
+				    "metadata",
+				    "csc",
+				    "cec",
+				    "hd";
+			ddc = <&ddc1>;
+			clock-names = "hdmi", "bvb", "audio", "cec";
+			resets = <&dvp 1>;
+			dmas = <&dma 17>;
+			dma-names = "audio-rx";
+			status = "disabled";
+		};
+
+		ddc1: i2c@7ef09500 {
+			compatible = "brcm,bcm2711-hdmi-i2c";
+			reg = <0x7ef09500 0x100>, <0x7ef05b00 0x300>;
+			reg-names = "bsc", "auto-i2c";
+			clock-frequency = <97500>;
+			status = "disabled";
+		};
 	};
 
 	/*