| Message ID | 20201210134648.272857-1-maxime@cerno.tech |
|---|---|
| Headers | show |
| Series | drm/vc4: hdmi: Add CEC support for the BCM2711 | expand |
Hi Dom & Maxime On Thu, 10 Dec 2020 at 13:46, Maxime Ripard <maxime@cerno.tech> wrote: > > From: Dom Cobley <popcornmix@gmail.com> > > The code prior to 311e305fdb4e ("drm/vc4: hdmi: Implement a register > layout abstraction") was relying on the fact that the register offset > was incremented by 4 for each readl call. That worked since the register > width is 4 bytes. > > However, since that commit the HDMI_READ macro is now taking an enum, > and the offset doesn't increment by 4 but 1 now. Divide the index by 4 > to fix this. > > Fixes: 311e305fdb4e ("drm/vc4: hdmi: Implement a register layout abstraction") > Signed-off-by: Dom Cobley <popcornmix@gmail.com> > Signed-off-by: Maxime Ripard <maxime@cerno.tech> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > drivers/gpu/drm/vc4/vc4_hdmi.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > index 3df1747dd917..28b78ea885ea 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > @@ -1434,13 +1434,20 @@ static irqreturn_t vc4_cec_irq_handler_thread(int irq, void *priv) > > static void vc4_cec_read_msg(struct vc4_hdmi *vc4_hdmi, u32 cntrl1) > { > + struct drm_device *dev = vc4_hdmi->connector.dev; > struct cec_msg *msg = &vc4_hdmi->cec_rx_msg; > unsigned int i; > > msg->len = 1 + ((cntrl1 & VC4_HDMI_CEC_REC_WRD_CNT_MASK) >> > VC4_HDMI_CEC_REC_WRD_CNT_SHIFT); > + > + if (msg->len > 16) { > + drm_err(dev, "Attempting to read too much data (%d)\n", msg->len); > + return; > + } > + > for (i = 0; i < msg->len; i += 4) { > - u32 val = HDMI_READ(HDMI_CEC_RX_DATA_1 + i); > + u32 val = HDMI_READ(HDMI_CEC_RX_DATA_1 + (i >> 2)); > > msg->msg[i] = val & 0xff; > msg->msg[i + 1] = (val >> 8) & 0xff; > @@ -1533,11 +1540,17 @@ static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts, > u32 signal_free_time, struct cec_msg *msg) > { > struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap); > + struct drm_device *dev = vc4_hdmi->connector.dev; > u32 val; > unsigned int i; > > + if (msg->len > 16) { > + drm_err(dev, "Attempting to transmit too much data (%d)\n", msg->len); > + return -ENOMEM; > + } > + > for (i = 0; i < msg->len; i += 4) > - HDMI_WRITE(HDMI_CEC_TX_DATA_1 + i, > + HDMI_WRITE(HDMI_CEC_TX_DATA_1 + (i >> 2), > (msg->msg[i]) | > (msg->msg[i + 1] << 8) | > (msg->msg[i + 2] << 16) | > -- > 2.28.0 >
Hi Maxime, On 10/12/2020 14:46, Maxime Ripard wrote: > Hi, > > Here's a series introducing the CEC support for the BCM2711 found on the > RaspberryPi4. > > The BCM2711 HDMI controller uses a similar layout for the CEC registers, the > main difference being that the interrupt handling part is now shared between > both HDMI controllers. > > This series is mainly about fixing a couple of bugs, reworking the driver to > support having two different interrupts, one for each direction, provided by an > external irqchip, and enables the irqchip driver for the controller we have. > > This has been tested on an RPi3 and RPi4, but requires the latest firmware. > It's is based on the 10 and 12 bpc series. This series looks good to me. Before I give my Acked-by for this series, can you confirm that it is possible to transmit the Image View On message on both outputs of the RPi4 when the HPD is low? See section "CEC Without HPD" in https://hverkuil.home.xs4all.nl/cec-status.txt on how to test this with a Pulse-Eight device. This should work. Regards, Hans > > Here is the cec-compliance output: > > $ cec-ctl --tuner -p 1.0.0.0 > The CEC adapter doesn't allow setting the physical address manually, ignore this option. > > Driver Info: > Driver Name : vc4_hdmi > Adapter Name : vc4 > Capabilities : 0x0000010e > Logical Addresses > Transmit > Passthrough > Driver version : 5.10.0 > Available Logical Addresses: 1 > Physical Address : 1.0.0.0 > Logical Address Mask : 0x0008 > CEC Version : 2.0 > Vendor ID : 0x000c03 (HDMI) > OSD Name : Tuner > Logical Addresses : 1 (Allow RC Passthrough) > > Logical Address : 3 (Tuner 1) > Primary Device Type : Tuner > Logical Address Type : Tuner > All Device Types : Tuner > RC TV Profile : None > Device Features : > None > > $ cec-compliance > cec-compliance SHA : not available > Driver Info: > Driver Name : vc4_hdmi > Adapter Name : vc4 > Capabilities : 0x0000010e > Logical Addresses > Transmit > Passthrough > Driver version : 5.10.0 > Available Logical Addresses: 1 > Physical Address : 1.0.0.0 > Logical Address Mask : 0x0008 > CEC Version : 2.0 > Vendor ID : 0x000c03 (HDMI) > OSD Name : Tuner > Logical Addresses : 1 (Allow RC Passthrough) > > Logical Address : 3 (Tuner 1) > Primary Device Type : Tuner > Logical Address Type : Tuner > All Device Types : Tuner > RC TV Profile : None > Device Features : > None > > Compliance test for vc4_hdmi device /dev/cec0: > > The test results mean the following: > OK Supported correctly by the device. > OK (Not Supported) Not supported and not mandatory for the device. > OK (Presumed) Presumably supported. Manually check to confirm. > OK (Unexpected) Supported correctly but is not expected to be supported for this device. > OK (Refused) Supported by the device, but was refused. > FAIL Failed and was expected to be supported by this device. > > Find remote devices: > Polling: OK > > Network topology: > System Information for device 0 (TV) from device 3 (Tuner 1): > CEC Version : 2.0 > Physical Address : 0.0.0.0 > Primary Device Type : TV > Vendor ID : 0x000c03 (HDMI) > OSD Name : 'test-124' > Power Status : Tx, OK, Rx, OK, Feature Abort > > Total for vc4_hdmi device /dev/cec0: 1, Succeeded: 1, Failed: 0, Warnings: 0 > > Let me know what you think, > Maxime > > Dom Cobley (5): > drm/vc4: hdmi: Move hdmi reset to bind > drm/vc4: hdmi: Fix register offset with longer CEC messages > drm/vc4: hdmi: Fix up CEC registers > drm/vc4: hdmi: Restore cec physical address on reconnect > drm/vc4: hdmi: Remove cec_available flag > > Maxime Ripard (10): > irqchip: Allow to compile bcmstb on other platforms > drm/vc4: hdmi: Compute the CEC clock divider from the clock rate > drm/vc4: hdmi: Update the CEC clock divider on HSM rate change > drm/vc4: hdmi: Introduce a CEC clock > drm/vc4: hdmi: Split the interrupt handlers > drm/vc4: hdmi: Support BCM2711 CEC interrupt setup > drm/vc4: hdmi: Don't register the CEC adapter if there's no interrupts > dt-binding: display: bcm2711-hdmi: Add CEC and hotplug interrupts > ARM: dts: bcm2711: Add the BSC interrupt controller > ARM: dts: bcm2711: Add the CEC interrupt controller > > .../bindings/display/brcm,bcm2711-hdmi.yaml | 20 +- > arch/arm/boot/dts/bcm2711.dtsi | 30 +++ > drivers/gpu/drm/vc4/vc4_hdmi.c | 224 +++++++++++++----- > drivers/gpu/drm/vc4/vc4_hdmi.h | 11 +- > drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 4 +- > drivers/irqchip/Kconfig | 2 +- > 6 files changed, 232 insertions(+), 59 deletions(-) >
Hi Hans, On Wed, Dec 16, 2020 at 01:35:43PM +0100, Hans Verkuil wrote: > Hi Maxime, > > On 10/12/2020 14:46, Maxime Ripard wrote: > > Hi, > > > > Here's a series introducing the CEC support for the BCM2711 found on the > > RaspberryPi4. > > > > The BCM2711 HDMI controller uses a similar layout for the CEC registers, the > > main difference being that the interrupt handling part is now shared between > > both HDMI controllers. > > > > This series is mainly about fixing a couple of bugs, reworking the driver to > > support having two different interrupts, one for each direction, provided by an > > external irqchip, and enables the irqchip driver for the controller we have. > > > > This has been tested on an RPi3 and RPi4, but requires the latest firmware. > > It's is based on the 10 and 12 bpc series. > > This series looks good to me. Before I give my Acked-by for this series, can you > confirm that it is possible to transmit the Image View On message on both outputs > of the RPi4 when the HPD is low? > > See section "CEC Without HPD" in https://hverkuil.home.xs4all.nl/cec-status.txt > on how to test this with a Pulse-Eight device. > > This should work. This is the output on the RPi4: # cec-ctl --playback Driver Info: Driver Name : vc4_hdmi Adapter Name : vc4 Capabilities : 0x0000010e Logical Addresses Transmit Passthrough Driver version : 5.10.0 Available Logical Addresses: 1 Physical Address : f.f.f.f Logical Address Mask : 0x0000 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : Playback Logical Addresses : 1 (Allow RC Passthrough) Logical Address : Not Allocated Primary Device Type : Playback Logical Address Type : Playback All Device Types : Playback RC TV Profile : None Device Features : None # cec-ctl -t0 --image-view-on Driver Info: Driver Name : vc4_hdmi Adapter Name : vc4 Capabilities : 0x0000010e Logical Addresses Transmit Passthrough Driver version : 5.10.0 Available Logical Addresses: 1 Physical Address : f.f.f.f Logical Address Mask : 0x0000 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : Playback Logical Addresses : 1 (Allow RC Passthrough) Logical Address : Not Allocated Primary Device Type : Playback Logical Address Type : Playback All Device Types : Playback RC TV Profile : None Device Features : None Transmit from Unregistered to TV (15 to 0): CEC_MSG_IMAGE_VIEW_ON (0x04) Sequence: 1 Tx Timestamp: 77.631s And this is the output on my desktop with the Pulse-Eight: $ sudo cec-ctl -p0.0.0.0 --tv Driver Info: Driver Name : pulse8-cec Adapter Name : serio0 Capabilities : 0x0000003f Physical Address Logical Addresses Transmit Passthrough Remote Control Support Monitor All Driver version : 5.9.8 Available Logical Addresses: 1 Connector Info : None Physical Address : 0.0.0.0 Logical Address Mask : 0x0001 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : 'TV ' Logical Addresses : 1 (Allow RC Passthrough) Logical Address : 0 (TV) Primary Device Type : TV Logical Address Type : TV All Device Types : TV RC TV Profile : None Device Features : None $ sudo cec-ctl -M Driver Info: Driver Name : pulse8-cec Adapter Name : serio0 Capabilities : 0x0000003f Physical Address Logical Addresses Transmit Passthrough Remote Control Support Monitor All Driver version : 5.9.8 Available Logical Addresses: 1 Connector Info : None Physical Address : 0.0.0.0 Logical Address Mask : 0x0001 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : 'TV ' Logical Addresses : 1 (Allow RC Passthrough) Logical Address : 0 (TV) Primary Device Type : TV Logical Address Type : TV All Device Types : TV RC TV Profile : None Device Features : None Initial Event: State Change: PA: 0.0.0.0, LA mask: 0x0001, Conn Info: no Received from Unregistered to TV (15 to 0): IMAGE_VIEW_ON (0x04) So it looks like it's working as expected? Maxime
On 17/12/2020 11:49, Maxime Ripard wrote: > Hi Hans, > > On Wed, Dec 16, 2020 at 01:35:43PM +0100, Hans Verkuil wrote: >> Hi Maxime, >> >> On 10/12/2020 14:46, Maxime Ripard wrote: >>> Hi, >>> >>> Here's a series introducing the CEC support for the BCM2711 found on the >>> RaspberryPi4. >>> >>> The BCM2711 HDMI controller uses a similar layout for the CEC registers, the >>> main difference being that the interrupt handling part is now shared between >>> both HDMI controllers. >>> >>> This series is mainly about fixing a couple of bugs, reworking the driver to >>> support having two different interrupts, one for each direction, provided by an >>> external irqchip, and enables the irqchip driver for the controller we have. >>> >>> This has been tested on an RPi3 and RPi4, but requires the latest firmware. >>> It's is based on the 10 and 12 bpc series. >> >> This series looks good to me. Before I give my Acked-by for this series, can you >> confirm that it is possible to transmit the Image View On message on both outputs >> of the RPi4 when the HPD is low? >> >> See section "CEC Without HPD" in https://hverkuil.home.xs4all.nl/cec-status.txt >> on how to test this with a Pulse-Eight device. >> >> This should work. > > This is the output on the RPi4: > > # cec-ctl --playback > Driver Info: > Driver Name : vc4_hdmi > Adapter Name : vc4 > Capabilities : 0x0000010e > Logical Addresses > Transmit > Passthrough > Driver version : 5.10.0 > Available Logical Addresses: 1 > Physical Address : f.f.f.f > Logical Address Mask : 0x0000 > CEC Version : 2.0 > Vendor ID : 0x000c03 (HDMI) > OSD Name : Playback > Logical Addresses : 1 (Allow RC Passthrough) > > Logical Address : Not Allocated > Primary Device Type : Playback > Logical Address Type : Playback > All Device Types : Playback > RC TV Profile : None > Device Features : > None > > # cec-ctl -t0 --image-view-on > Driver Info: > Driver Name : vc4_hdmi > Adapter Name : vc4 > Capabilities : 0x0000010e > Logical Addresses > Transmit > Passthrough > Driver version : 5.10.0 > Available Logical Addresses: 1 > Physical Address : f.f.f.f > Logical Address Mask : 0x0000 > CEC Version : 2.0 > Vendor ID : 0x000c03 (HDMI) > OSD Name : Playback > Logical Addresses : 1 (Allow RC Passthrough) > > Logical Address : Not Allocated > Primary Device Type : Playback > Logical Address Type : Playback > All Device Types : Playback > RC TV Profile : None > Device Features : > None > > > Transmit from Unregistered to TV (15 to 0): > CEC_MSG_IMAGE_VIEW_ON (0x04) > Sequence: 1 Tx Timestamp: 77.631s > > > And this is the output on my desktop with the Pulse-Eight: > $ sudo cec-ctl -p0.0.0.0 --tv > Driver Info: > Driver Name : pulse8-cec > Adapter Name : serio0 > Capabilities : 0x0000003f > Physical Address > Logical Addresses > Transmit > Passthrough > Remote Control Support > Monitor All > Driver version : 5.9.8 > Available Logical Addresses: 1 > Connector Info : None > Physical Address : 0.0.0.0 > Logical Address Mask : 0x0001 > CEC Version : 2.0 > Vendor ID : 0x000c03 (HDMI) > OSD Name : 'TV ' > Logical Addresses : 1 (Allow RC Passthrough) > > Logical Address : 0 (TV) > Primary Device Type : TV > Logical Address Type : TV > All Device Types : TV > RC TV Profile : None > Device Features : > None > > $ sudo cec-ctl -M > Driver Info: > Driver Name : pulse8-cec > Adapter Name : serio0 > Capabilities : 0x0000003f > Physical Address > Logical Addresses > Transmit > Passthrough > Remote Control Support > Monitor All > Driver version : 5.9.8 > Available Logical Addresses: 1 > Connector Info : None > Physical Address : 0.0.0.0 > Logical Address Mask : 0x0001 > CEC Version : 2.0 > Vendor ID : 0x000c03 (HDMI) > OSD Name : 'TV ' > Logical Addresses : 1 (Allow RC Passthrough) > > Logical Address : 0 (TV) > Primary Device Type : TV > Logical Address Type : TV > All Device Types : TV > RC TV Profile : None > Device Features : > None > > > > Initial Event: State Change: PA: 0.0.0.0, LA mask: 0x0001, Conn Info: no > Received from Unregistered to TV (15 to 0): IMAGE_VIEW_ON (0x04) > > So it looks like it's working as expected? Yes, it looks good. Make sure you test this for both outputs of the RPi4. If it works for both, then you can add my Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> for this series. Very nice work, thank you for doing this! Regards, Hans > > Maxime >
Hi Hans, On Thu, Dec 17, 2020 at 11:53:42AM +0100, Hans Verkuil wrote: > On 17/12/2020 11:49, Maxime Ripard wrote: > > Hi Hans, > > > > On Wed, Dec 16, 2020 at 01:35:43PM +0100, Hans Verkuil wrote: > >> Hi Maxime, > >> > >> On 10/12/2020 14:46, Maxime Ripard wrote: > >>> Hi, > >>> > >>> Here's a series introducing the CEC support for the BCM2711 found on the > >>> RaspberryPi4. > >>> > >>> The BCM2711 HDMI controller uses a similar layout for the CEC registers, the > >>> main difference being that the interrupt handling part is now shared between > >>> both HDMI controllers. > >>> > >>> This series is mainly about fixing a couple of bugs, reworking the driver to > >>> support having two different interrupts, one for each direction, provided by an > >>> external irqchip, and enables the irqchip driver for the controller we have. > >>> > >>> This has been tested on an RPi3 and RPi4, but requires the latest firmware. > >>> It's is based on the 10 and 12 bpc series. > >> > >> This series looks good to me. Before I give my Acked-by for this series, can you > >> confirm that it is possible to transmit the Image View On message on both outputs > >> of the RPi4 when the HPD is low? > >> > >> See section "CEC Without HPD" in https://hverkuil.home.xs4all.nl/cec-status.txt > >> on how to test this with a Pulse-Eight device. > >> > >> This should work. > > > > This is the output on the RPi4: > > > > # cec-ctl --playback > > Driver Info: > > Driver Name : vc4_hdmi > > Adapter Name : vc4 > > Capabilities : 0x0000010e > > Logical Addresses > > Transmit > > Passthrough > > Driver version : 5.10.0 > > Available Logical Addresses: 1 > > Physical Address : f.f.f.f > > Logical Address Mask : 0x0000 > > CEC Version : 2.0 > > Vendor ID : 0x000c03 (HDMI) > > OSD Name : Playback > > Logical Addresses : 1 (Allow RC Passthrough) > > > > Logical Address : Not Allocated > > Primary Device Type : Playback > > Logical Address Type : Playback > > All Device Types : Playback > > RC TV Profile : None > > Device Features : > > None > > > > # cec-ctl -t0 --image-view-on > > Driver Info: > > Driver Name : vc4_hdmi > > Adapter Name : vc4 > > Capabilities : 0x0000010e > > Logical Addresses > > Transmit > > Passthrough > > Driver version : 5.10.0 > > Available Logical Addresses: 1 > > Physical Address : f.f.f.f > > Logical Address Mask : 0x0000 > > CEC Version : 2.0 > > Vendor ID : 0x000c03 (HDMI) > > OSD Name : Playback > > Logical Addresses : 1 (Allow RC Passthrough) > > > > Logical Address : Not Allocated > > Primary Device Type : Playback > > Logical Address Type : Playback > > All Device Types : Playback > > RC TV Profile : None > > Device Features : > > None > > > > > > Transmit from Unregistered to TV (15 to 0): > > CEC_MSG_IMAGE_VIEW_ON (0x04) > > Sequence: 1 Tx Timestamp: 77.631s > > > > > > And this is the output on my desktop with the Pulse-Eight: > > $ sudo cec-ctl -p0.0.0.0 --tv > > Driver Info: > > Driver Name : pulse8-cec > > Adapter Name : serio0 > > Capabilities : 0x0000003f > > Physical Address > > Logical Addresses > > Transmit > > Passthrough > > Remote Control Support > > Monitor All > > Driver version : 5.9.8 > > Available Logical Addresses: 1 > > Connector Info : None > > Physical Address : 0.0.0.0 > > Logical Address Mask : 0x0001 > > CEC Version : 2.0 > > Vendor ID : 0x000c03 (HDMI) > > OSD Name : 'TV ' > > Logical Addresses : 1 (Allow RC Passthrough) > > > > Logical Address : 0 (TV) > > Primary Device Type : TV > > Logical Address Type : TV > > All Device Types : TV > > RC TV Profile : None > > Device Features : > > None > > > > $ sudo cec-ctl -M > > Driver Info: > > Driver Name : pulse8-cec > > Adapter Name : serio0 > > Capabilities : 0x0000003f > > Physical Address > > Logical Addresses > > Transmit > > Passthrough > > Remote Control Support > > Monitor All > > Driver version : 5.9.8 > > Available Logical Addresses: 1 > > Connector Info : None > > Physical Address : 0.0.0.0 > > Logical Address Mask : 0x0001 > > CEC Version : 2.0 > > Vendor ID : 0x000c03 (HDMI) > > OSD Name : 'TV ' > > Logical Addresses : 1 (Allow RC Passthrough) > > > > Logical Address : 0 (TV) > > Primary Device Type : TV > > Logical Address Type : TV > > All Device Types : TV > > RC TV Profile : None > > Device Features : > > None > > > > > > > > Initial Event: State Change: PA: 0.0.0.0, LA mask: 0x0001, Conn Info: no > > Received from Unregistered to TV (15 to 0): IMAGE_VIEW_ON (0x04) > > > > So it looks like it's working as expected? > > Yes, it looks good. Make sure you test this for both outputs of the RPi4. It's a good thing you asked, I don't appear to get CEC interrupts from HDMI1. I'll fix it and send another version (probably not before the end of december though). > If it works for both, then you can add my > > Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > for this series. > > Very nice work, thank you for doing this! Thanks! I'll hold your a-b until the next version though, fixing hdmi1 might change a few things. Maxime
Hi Maxime & Dom On Thu, 10 Dec 2020 at 13:46, Maxime Ripard <maxime@cerno.tech> wrote: > > From: Dom Cobley <popcornmix@gmail.com> > > The hdmi reset got moved to a later point in the commit 9045e91a476b > ("drm/vc4: hdmi: Add reset callback"). > > However, the reset now occurs after vc4_hdmi_cec_init and so tramples > the setup of registers like HDMI_CEC_CNTRL_1 > > This only affects pi0-3 as on pi4 the cec registers are in a separate > block It does mean that this reset only happens once on bind rather than on every pre_crtc_configure, but as this really is the big reset the entire block I don't see it needing to be triggered on every configure. > Fixes: 9045e91a476b ("drm/vc4: hdmi: Add reset callback") > Signed-off-by: Dom Cobley <popcornmix@gmail.com> > Signed-off-by: Maxime Ripard <maxime@cerno.tech> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > drivers/gpu/drm/vc4/vc4_hdmi.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > index 8006bddc8fbb..3df1747dd917 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > @@ -773,9 +773,6 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, > return; > } > > - if (vc4_hdmi->variant->reset) > - vc4_hdmi->variant->reset(vc4_hdmi); > - > if (vc4_hdmi->variant->phy_init) > vc4_hdmi->variant->phy_init(vc4_hdmi, vc4_conn_state); > > @@ -1865,6 +1862,9 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) > vc4_hdmi->disable_wifi_frequencies = > of_property_read_bool(dev->of_node, "wifi-2.4ghz-coexistence"); > > + if (vc4_hdmi->variant->reset) > + vc4_hdmi->variant->reset(vc4_hdmi); > + > pm_runtime_enable(dev); > > drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS); > -- > 2.28.0 >
Hi Maxime On Thu, 10 Dec 2020 at 13:47, Maxime Ripard <maxime@cerno.tech> wrote: > > The CEC clock divider needs to output a frequency of 40kHz from the HSM > rate on the BCM2835. The driver used to have a fixed frequency for it, > but that changed and we now need to compute it dynamically to maintain > the proper rate. > > Fixes: cd4cb49dc5bb ("drm/vc4: hdmi: Adjust HSM clock rate depending on pixel rate") > Signed-off-by: Maxime Ripard <maxime@cerno.tech> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> (To be a total pedant it's still a fixed frequency on vc4, but it's configurable via the variant entry). > --- > drivers/gpu/drm/vc4/vc4_hdmi.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > index eff3bac562c6..0c53d7427d15 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > @@ -1586,6 +1586,7 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) > { > struct cec_connector_info conn_info; > struct platform_device *pdev = vc4_hdmi->pdev; > + u16 clk_cnt; > u32 value; > int ret; > > @@ -1611,8 +1612,9 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) > * divider: the hsm_clock rate and this divider setting will > * give a 40 kHz CEC clock. > */ > + clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ; > value |= VC4_HDMI_CEC_ADDR_MASK | > - (4091 << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT); > + (clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT); > HDMI_WRITE(HDMI_CEC_CNTRL_1, value); > ret = devm_request_threaded_irq(&pdev->dev, platform_get_irq(pdev, 0), > vc4_cec_irq_handler, > -- > 2.28.0 >
Hi Maxime On Thu, 10 Dec 2020 at 13:47, Maxime Ripard <maxime@cerno.tech> wrote: > > While the BCM2835 had the CEC clock derived from the HSM clock, the > BCM2711 has a dedicated parent clock for it. > > Let's introduce a separate clock for it so that we can handle both > cases. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > drivers/gpu/drm/vc4/vc4_hdmi.c | 9 ++++++++- > drivers/gpu/drm/vc4/vc4_hdmi.h | 1 + > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > index b93ee3e26e2b..0debd22bc992 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > @@ -145,7 +145,7 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) > * Set the clock divider: the hsm_clock rate and this divider > * setting will give a 40 kHz CEC clock. > */ > - clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ; > + clk_cnt = clk_get_rate(vc4_hdmi->cec_clock) / CEC_CLOCK_FREQ; > value |= clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT; > HDMI_WRITE(HDMI_CEC_CNTRL_1, value); > } > @@ -1740,6 +1740,7 @@ static int vc4_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi) > return PTR_ERR(vc4_hdmi->hsm_clock); > } > vc4_hdmi->audio_clock = vc4_hdmi->hsm_clock; > + vc4_hdmi->cec_clock = vc4_hdmi->hsm_clock; > > return 0; > } > @@ -1833,6 +1834,12 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi) > return PTR_ERR(vc4_hdmi->audio_clock); > } > > + vc4_hdmi->cec_clock = devm_clk_get(dev, "cec"); > + if (IS_ERR(vc4_hdmi->cec_clock)) { > + DRM_ERROR("Failed to get CEC clock\n"); > + return PTR_ERR(vc4_hdmi->cec_clock); > + } Aren't we adding to the DT binding here and breaking backwards compatibility? Admittedly CEC didn't work before (and was masked out) for vc5, but do we need to worry about those with existing DT files that currently work happily? Otherwise I'm happy with the patch. Dave > + > vc4_hdmi->reset = devm_reset_control_get(dev, NULL); > if (IS_ERR(vc4_hdmi->reset)) { > DRM_ERROR("Failed to get HDMI reset line\n"); > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h > index 720914761261..adc4bf33ff15 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.h > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h > @@ -155,6 +155,7 @@ struct vc4_hdmi { > bool cec_tx_ok; > bool cec_irq_was_rx; > > + struct clk *cec_clock; > struct clk *pixel_clock; > struct clk *hsm_clock; > struct clk *audio_clock; > -- > 2.28.0 >
Hi Dave, On Fri, Dec 18, 2020 at 11:37:50AM +0000, Dave Stevenson wrote: > Hi Maxime > > On Thu, 10 Dec 2020 at 13:47, Maxime Ripard <maxime@cerno.tech> wrote: > > > > While the BCM2835 had the CEC clock derived from the HSM clock, the > > BCM2711 has a dedicated parent clock for it. > > > > Let's introduce a separate clock for it so that we can handle both > > cases. > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > --- > > drivers/gpu/drm/vc4/vc4_hdmi.c | 9 ++++++++- > > drivers/gpu/drm/vc4/vc4_hdmi.h | 1 + > > 2 files changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > index b93ee3e26e2b..0debd22bc992 100644 > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > > @@ -145,7 +145,7 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) > > * Set the clock divider: the hsm_clock rate and this divider > > * setting will give a 40 kHz CEC clock. > > */ > > - clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ; > > + clk_cnt = clk_get_rate(vc4_hdmi->cec_clock) / CEC_CLOCK_FREQ; > > value |= clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT; > > HDMI_WRITE(HDMI_CEC_CNTRL_1, value); > > } > > @@ -1740,6 +1740,7 @@ static int vc4_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi) > > return PTR_ERR(vc4_hdmi->hsm_clock); > > } > > vc4_hdmi->audio_clock = vc4_hdmi->hsm_clock; > > + vc4_hdmi->cec_clock = vc4_hdmi->hsm_clock; > > > > return 0; > > } > > @@ -1833,6 +1834,12 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi) > > return PTR_ERR(vc4_hdmi->audio_clock); > > } > > > > + vc4_hdmi->cec_clock = devm_clk_get(dev, "cec"); > > + if (IS_ERR(vc4_hdmi->cec_clock)) { > > + DRM_ERROR("Failed to get CEC clock\n"); > > + return PTR_ERR(vc4_hdmi->cec_clock); > > + } > > Aren't we adding to the DT binding here and breaking backwards compatibility? > Admittedly CEC didn't work before (and was masked out) for vc5, but do > we need to worry about those with existing DT files that currently > work happily? The DT compatibility is not a worry here: I made sure the CEC clock and range were part of the binding since it's been introduced: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2e3725b05b785e73482a194b99bff3d5a1c85140 So we were not using it so far, but it was in the DT all along Maxime
On Fri, 18 Dec 2020 at 12:23, Maxime Ripard <maxime@cerno.tech> wrote: > > Hi Dave, > > On Fri, Dec 18, 2020 at 11:37:50AM +0000, Dave Stevenson wrote: > > Hi Maxime > > > > On Thu, 10 Dec 2020 at 13:47, Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > While the BCM2835 had the CEC clock derived from the HSM clock, the > > > BCM2711 has a dedicated parent clock for it. > > > > > > Let's introduce a separate clock for it so that we can handle both > > > cases. > > > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > > --- > > > drivers/gpu/drm/vc4/vc4_hdmi.c | 9 ++++++++- > > > drivers/gpu/drm/vc4/vc4_hdmi.h | 1 + > > > 2 files changed, 9 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > > index b93ee3e26e2b..0debd22bc992 100644 > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > > > @@ -145,7 +145,7 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) > > > * Set the clock divider: the hsm_clock rate and this divider > > > * setting will give a 40 kHz CEC clock. > > > */ > > > - clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ; > > > + clk_cnt = clk_get_rate(vc4_hdmi->cec_clock) / CEC_CLOCK_FREQ; > > > value |= clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT; > > > HDMI_WRITE(HDMI_CEC_CNTRL_1, value); > > > } > > > @@ -1740,6 +1740,7 @@ static int vc4_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi) > > > return PTR_ERR(vc4_hdmi->hsm_clock); > > > } > > > vc4_hdmi->audio_clock = vc4_hdmi->hsm_clock; > > > + vc4_hdmi->cec_clock = vc4_hdmi->hsm_clock; > > > > > > return 0; > > > } > > > @@ -1833,6 +1834,12 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi) > > > return PTR_ERR(vc4_hdmi->audio_clock); > > > } > > > > > > + vc4_hdmi->cec_clock = devm_clk_get(dev, "cec"); > > > + if (IS_ERR(vc4_hdmi->cec_clock)) { > > > + DRM_ERROR("Failed to get CEC clock\n"); > > > + return PTR_ERR(vc4_hdmi->cec_clock); > > > + } > > > > Aren't we adding to the DT binding here and breaking backwards compatibility? > > Admittedly CEC didn't work before (and was masked out) for vc5, but do > > we need to worry about those with existing DT files that currently > > work happily? > > The DT compatibility is not a worry here: I made sure the CEC clock and > range were part of the binding since it's been introduced: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2e3725b05b785e73482a194b99bff3d5a1c85140 > > So we were not using it so far, but it was in the DT all along I guess I should have read it then :-) In which case Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Hi Maxime On Thu, 10 Dec 2020 at 13:47, Maxime Ripard <maxime@cerno.tech> wrote: > > As part of the enable sequence we might change the HSM clock rate if the > pixel rate is different than the one we were already dealing with. > > On the BCM2835 however, the CEC clock derives from the HSM clock so any > rate change will need to be reflected in the CEC clock divider to output > 40kHz. > > Fixes: cd4cb49dc5bb ("drm/vc4: hdmi: Adjust HSM clock rate depending on pixel rate") > Signed-off-by: Maxime Ripard <maxime@cerno.tech> I thought we'd got a duplicate patch here, but it's moving code that was changed in patch 6/15 so it can be called from vc4_hdmi_encoder_pre_crtc_configure too. Good for confusing me! Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > drivers/gpu/drm/vc4/vc4_hdmi.c | 39 +++++++++++++++++++++++++--------- > 1 file changed, 29 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > index 0c53d7427d15..b93ee3e26e2b 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > @@ -132,6 +132,27 @@ static void vc5_hdmi_reset(struct vc4_hdmi *vc4_hdmi) > HDMI_READ(HDMI_CLOCK_STOP) | VC4_DVP_HT_CLOCK_STOP_PIXEL); > } > > +#ifdef CONFIG_DRM_VC4_HDMI_CEC > +static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) > +{ > + u16 clk_cnt; > + u32 value; > + > + value = HDMI_READ(HDMI_CEC_CNTRL_1); > + value &= ~VC4_HDMI_CEC_DIV_CLK_CNT_MASK; > + > + /* > + * Set the clock divider: the hsm_clock rate and this divider > + * setting will give a 40 kHz CEC clock. > + */ > + clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ; > + value |= clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT; > + HDMI_WRITE(HDMI_CEC_CNTRL_1, value); > +} > +#else > +static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) {} > +#endif > + > static enum drm_connector_status > vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) > { > @@ -761,6 +782,8 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, > return; > } > > + vc4_hdmi_cec_update_clk_div(vc4_hdmi); > + > /* > * FIXME: When the pixel freq is 594MHz (4k60), this needs to be setup > * at 300MHz. > @@ -1586,7 +1609,6 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) > { > struct cec_connector_info conn_info; > struct platform_device *pdev = vc4_hdmi->pdev; > - u16 clk_cnt; > u32 value; > int ret; > > @@ -1605,17 +1627,14 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) > cec_s_conn_info(vc4_hdmi->cec_adap, &conn_info); > > HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, 0xffffffff); > + > value = HDMI_READ(HDMI_CEC_CNTRL_1); > - value &= ~VC4_HDMI_CEC_DIV_CLK_CNT_MASK; > - /* > - * Set the logical address to Unregistered and set the clock > - * divider: the hsm_clock rate and this divider setting will > - * give a 40 kHz CEC clock. > - */ > - clk_cnt = clk_get_rate(vc4_hdmi->hsm_clock) / CEC_CLOCK_FREQ; > - value |= VC4_HDMI_CEC_ADDR_MASK | > - (clk_cnt << VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT); > + /* Set the logical address to Unregistered */ > + value |= VC4_HDMI_CEC_ADDR_MASK; > HDMI_WRITE(HDMI_CEC_CNTRL_1, value); > + > + vc4_hdmi_cec_update_clk_div(vc4_hdmi); > + > ret = devm_request_threaded_irq(&pdev->dev, platform_get_irq(pdev, 0), > vc4_cec_irq_handler, > vc4_cec_irq_handler_thread, 0, > -- > 2.28.0 >
On Thu, 10 Dec 2020 at 13:47, Maxime Ripard <maxime@cerno.tech> wrote: > > We introduced the BCM2711 support to the vc4 HDMI controller with 5.10, > but this was lacking any of the interrupts of the CEC controller so we > have to deal with the backward compatibility. > > Do so by simply ignoring the CEC setup if the DT doesn't have the > interrupts property. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > drivers/gpu/drm/vc4/vc4_hdmi.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > index 327638d93032..69217c68d3a4 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > @@ -1655,9 +1655,15 @@ static int vc4_hdmi_cec_init(struct vc4_hdmi *vc4_hdmi) > { > struct cec_connector_info conn_info; > struct platform_device *pdev = vc4_hdmi->pdev; > + struct device *dev = &pdev->dev; > u32 value; > int ret; > > + if (!of_find_property(dev->of_node, "interrupts", NULL)) { > + dev_warn(dev, "'interrupts' DT property is missing, no CEC\n"); > + return 0; > + } > + > vc4_hdmi->cec_adap = cec_allocate_adapter(&vc4_hdmi_cec_adap_ops, > vc4_hdmi, "vc4", > CEC_CAP_DEFAULTS | > -- > 2.28.0 >
Hi, Here's a series introducing the CEC support for the BCM2711 found on the RaspberryPi4. The BCM2711 HDMI controller uses a similar layout for the CEC registers, the main difference being that the interrupt handling part is now shared between both HDMI controllers. This series is mainly about fixing a couple of bugs, reworking the driver to support having two different interrupts, one for each direction, provided by an external irqchip, and enables the irqchip driver for the controller we have. This has been tested on an RPi3 and RPi4, but requires the latest firmware. It's is based on the 10 and 12 bpc series. Here is the cec-compliance output: $ cec-ctl --tuner -p 1.0.0.0 The CEC adapter doesn't allow setting the physical address manually, ignore this option. Driver Info: Driver Name : vc4_hdmi Adapter Name : vc4 Capabilities : 0x0000010e Logical Addresses Transmit Passthrough Driver version : 5.10.0 Available Logical Addresses: 1 Physical Address : 1.0.0.0 Logical Address Mask : 0x0008 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : Tuner Logical Addresses : 1 (Allow RC Passthrough) Logical Address : 3 (Tuner 1) Primary Device Type : Tuner Logical Address Type : Tuner All Device Types : Tuner RC TV Profile : None Device Features : None $ cec-compliance cec-compliance SHA : not available Driver Info: Driver Name : vc4_hdmi Adapter Name : vc4 Capabilities : 0x0000010e Logical Addresses Transmit Passthrough Driver version : 5.10.0 Available Logical Addresses: 1 Physical Address : 1.0.0.0 Logical Address Mask : 0x0008 CEC Version : 2.0 Vendor ID : 0x000c03 (HDMI) OSD Name : Tuner Logical Addresses : 1 (Allow RC Passthrough) Logical Address : 3 (Tuner 1) Primary Device Type : Tuner Logical Address Type : Tuner All Device Types : Tuner RC TV Profile : None Device Features : None Compliance test for vc4_hdmi device /dev/cec0: The test results mean the following: OK Supported correctly by the device. OK (Not Supported) Not supported and not mandatory for the device. OK (Presumed) Presumably supported. Manually check to confirm. OK (Unexpected) Supported correctly but is not expected to be supported for this device. OK (Refused) Supported by the device, but was refused. FAIL Failed and was expected to be supported by this device. Find remote devices: Polling: OK Network topology: System Information for device 0 (TV) from device 3 (Tuner 1): CEC Version : 2.0 Physical Address : 0.0.0.0 Primary Device Type : TV Vendor ID : 0x000c03 (HDMI) OSD Name : 'test-124' Power Status : Tx, OK, Rx, OK, Feature Abort Total for vc4_hdmi device /dev/cec0: 1, Succeeded: 1, Failed: 0, Warnings: 0 Let me know what you think, Maxime Dom Cobley (5): drm/vc4: hdmi: Move hdmi reset to bind drm/vc4: hdmi: Fix register offset with longer CEC messages drm/vc4: hdmi: Fix up CEC registers drm/vc4: hdmi: Restore cec physical address on reconnect drm/vc4: hdmi: Remove cec_available flag Maxime Ripard (10): irqchip: Allow to compile bcmstb on other platforms drm/vc4: hdmi: Compute the CEC clock divider from the clock rate drm/vc4: hdmi: Update the CEC clock divider on HSM rate change drm/vc4: hdmi: Introduce a CEC clock drm/vc4: hdmi: Split the interrupt handlers drm/vc4: hdmi: Support BCM2711 CEC interrupt setup drm/vc4: hdmi: Don't register the CEC adapter if there's no interrupts dt-binding: display: bcm2711-hdmi: Add CEC and hotplug interrupts ARM: dts: bcm2711: Add the BSC interrupt controller ARM: dts: bcm2711: Add the CEC interrupt controller .../bindings/display/brcm,bcm2711-hdmi.yaml | 20 +- arch/arm/boot/dts/bcm2711.dtsi | 30 +++ drivers/gpu/drm/vc4/vc4_hdmi.c | 224 +++++++++++++----- drivers/gpu/drm/vc4/vc4_hdmi.h | 11 +- drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 4 +- drivers/irqchip/Kconfig | 2 +- 6 files changed, 232 insertions(+), 59 deletions(-) -- 2.28.0