by Jernej Skrabec

[permalink] [raw]

Subject: Re: [PATCH 06/11] dt-bindings: display: sun4i-drm: Add A83T HDMI pipeline

Hi Rob,

Dne sreda, 03. januar 2018 ob 21:21:54 CET je Rob Herring napisal(a):
> On Sat, Dec 30, 2017 at 10:01:58PM +0100, Jernej Skrabec wrote:
> > This commit adds all necessary compatibles and descriptions needed to
> > implement A83T HDMI pipeline.
> >
> > Mixer is already properly described, so only compatible is added.
> >
> > However, A83T TCON1, which is connected to HDMI, doesn't have channel 0,
> > contrary to all TCONs currently described. Because of that, TCON
> > documentation is extended.
> >
> > A83T features Synopsys DW HDMI controller with a custom PHY which looks
> > like Synopsys Gen2 PHY with few additions. Since there is no
> > documentation, needed properties were found out through experimentation
> > and reading BSP code.
> >
> > At the end, example is added for newer SoCs, which features DE2 and DW
> > HDMI.
> >
> > Signed-off-by: Jernej Skrabec <[email protected]>
> > ---
> >
> > .../bindings/display/sunxi/sun4i-drm.txt | 188
> > ++++++++++++++++++++- 1 file changed, 181 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> > b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt index
> > 9f073af4c711..3eca258096a5 100644
> > --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> > +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt
> >
> > @@ -64,6 +64,40 @@ Required properties:
> > first port should be the input endpoint. The second should be the
> > output, usually to an HDMI connector.
> >
> > +DWC HDMI TX Encoder
> > +-----------------------------
> > +
> > +The HDMI transmitter is a Synopsys DesignWare HDMI 1.4 TX controller IP
> > +with Allwinner's own PHY IP. It supports audio and video outputs and CEC.
> > +
> > +These DT bindings follow the Synopsys DWC HDMI TX bindings defined in
> > +Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt with the
> > +following device-specific properties.
> > +
> > +Required properties:
> > +
> > + - compatible: value must be one of:
> > + * "allwinner,sun8i-a83t-dw-hdmi"
> > + - reg: two pairs of base address and size of memory-mapped region,
> > first
> > + for controller and second for PHY
> > + registers.
>
> Seems like the phy should be a separate node and use the phy binding.
> You can use the phy binding even if you don't use the kernel phy
> framework...

Unfortunately, it's not so straighforward. Phy is actually accessed through
I2C implemented in HDMI controller. Second memory region in this case has
small influence on phy. However, it has big influence on controller. For
example, magic number has to be written in one register in second memory
region in order to unlock read access to any register from first memory region
(controller). However, they shouldn't be merged to one region, because first
memory region requires byte access while second memory region can be accessed
per byte or word.

To complicate things more, later I want to add support for another SoC which
has same glue layer (unlocking read access, etc.) and uses memory mapped phy
registers in second memory region.

I think current binding is the least complicated way to represent this.

>
> > + - reg-io-width: See dw_hdmi.txt. Shall be 1.
> > + - interrupts: HDMI interrupt number
> > + - clocks: phandles to the clocks feeding the HDMI encoder
> > + * iahb: the HDMI bus clock
> > + * isfr: the HDMI register clock
> > + * tmds: the HDMI tmds clock
> > + - clock-names: the clock names mentioned above
> > + - resets: phandles to the reset controllers driving the encoder
> > + * ctrl: the reset line for the controller
> > + * phy: the reset line for the PHY
> > + - reset-names: the reset names mentioned above
> > +
> > + - ports: A ports node with endpoint definitions as defined in
> > + Documentation/devicetree/bindings/media/video-interfaces.txt. The
> > + first port should be the input endpoint. The second should be the
> > + output, usually to an HDMI connector.
> > +
> >
> > TV Encoder
> > ----------
> >
> > @@ -94,18 +128,17 @@ Required properties:
> > * allwinner,sun7i-a20-tcon
> > * allwinner,sun8i-a33-tcon
> > * allwinner,sun8i-a83t-tcon-lcd
> >
> > + * allwinner,sun8i-a83t-tcon-tv
> >
> > * allwinner,sun8i-v3s-tcon
> >
> > - reg: base address and size of memory-mapped region
> > - interrupts: interrupt associated to this IP
> >
> > - - clocks: phandles to the clocks feeding the TCON. Three are needed:
> >
> > + - clocks: phandles to the clocks feeding the TCON. One is needed:
> > - 'ahb': the interface clocks
> >
> > - - 'tcon-ch0': The clock driving the TCON channel 0
> >
> > - resets: phandles to the reset controllers driving the encoder
> >
> > - "lcd": the reset line for the TCON channel 0
> >
> > - clock-names: the clock names mentioned above
> > - reset-names: the reset names mentioned above
> >
> > - - clock-output-names: Name of the pixel clock created
> >
> > - ports: A ports node with endpoint definitions as defined in
> >
> > Documentation/devicetree/bindings/media/video-interfaces.txt. The
> >
> > @@ -119,11 +152,31 @@ Required properties:
> > channel the endpoint is associated to. If that property is not
> > present, the endpoint number will be used as the channel number.
> >
> > -On SoCs other than the A33 and V3s, there is one more clock required:
> > +Following compatibles:
> > + * allwinner,sun4i-a10-tcon
> > + * allwinner,sun5i-a13-tcon
> > + * allwinner,sun6i-a31-tcon
> > + * allwinner,sun6i-a31s-tcon
> > + * allwinner,sun7i-a20-tcon
> > + * allwinner,sun8i-a33-tcon
> > + * allwinner,sun8i-a83t-tcon-lcd
> > + * allwinner,sun8i-v3s-tcon
> > +have additional required properties:
> > + - 'tcon-ch0': The clock driving the TCON channel 0
>
> tcon-ch0 is a clock name, not a property.

right.

Best regards,
Jernej

2018-01-04 14:25:34

Hi,

On Sat, Dec 30, 2017 at 10:01:59PM +0100, Jernej Skrabec wrote:
> This TCON doesn't have channel 0, so quirk has_channel_0 is added in the
> process.
>
> Signed-off-by: Jernej Skrabec <[email protected]>

Ideally, this should be split in two patches, one to add the new flag,
and one to add the support for the A83t TCON-TV.

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Attachments:

(No filename) (444.00 B)
signature.asc (833.00 B)
Download all attachments

2018-01-04 15:50:43

Hi Laurent,

Dne torek, 09. januar 2018 ob 17:08:55 CET je Laurent Pinchart napisal(a):
> Hello,
>
> On Tuesday, 9 January 2018 17:58:46 EET Jernej Škrabec wrote:
> > Dne torek, 09. januar 2018 ob 11:43:08 CET je Archit Taneja napisal(a):
> > > On 12/31/2017 02:31 AM, Jernej Skrabec wrote:
> > >> Parts of PHY code could be useful also for custom PHYs. For example,
> > >> Allwinner A83T has custom PHY which is probably Synopsys gen2 PHY
> > >> with few additional memory mapped registers, so most of the Synopsys
> > >> PHY
> > >> related code could be reused.
> > >>
> > >> It turns out that even completely custom HDMI PHYs, such as the one
> > >> found in Allwinner H3, can reuse some of those functions. This would
> > >> suggest that (some?) functions exported in this commit are actually
> > >> part
> > >> of generic PHY interface and not really specific to Synopsys PHYs.
> > >>
> > >> Export useful PHY functions.
> > >>
> > >> Signed-off-by: Jernej Skrabec <[email protected]>
> > >> ---
> > >>
> > >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 45 +++++++++++++++++-------
> > >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 2 ++
> > >> include/drm/bridge/dw_hdmi.h | 10 +++++++
> > >> 3 files changed, 44 insertions(+), 13 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > >> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index
> > >> 7ca14d7325b5..67467d0b683a 100644
> > >> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>
> [snip]
>
> > >> @@ -1065,6 +1067,23 @@ static void
> > >> dw_hdmi_phy_sel_interface_control(struct dw_hdmi *hdmi, u8 enable)
> > >>
> > >> HDMI_PHY_CONF0_SELDIPIF_MASK);
> > >>
> > >> }
> > >>
> > >> +void dw_hdmi_phy_gen2_reset(struct dw_hdmi *hdmi, u8 enable)
> > >> +{
> > >> + hdmi_mask_writeb(hdmi, enable, HDMI_MC_PHYRSTZ,
> > >> + HDMI_MC_PHYRSTZ_PHYRSTZ_OFFSET,
> > >> + HDMI_MC_PHYRSTZ_PHYRSTZ_MASK);
> > >> +}
> > >> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_reset);
>
> I don't remember the details, is the reset signal Gen2-specific ?

According to this comment:

/* PHY reset. The reset signal is active high on Gen2 PHYs. */

I would guess that it is not specific to Gen2, otherwise the comment wouldn't
be needed. I will remove "gen2" from the name.

>
> How about asserting and deasserting the reset signal in the same call
> instead of having to call this function twice ?

It works on A83T if reset signal is asserted and deasserted immediately. I
will change function according to your proposal.

Best regards,
Jernej

>
> > >> +void dw_hdmi_phy_set_slave_addr(struct dw_hdmi *hdmi)
> > >> +{
> > >> + hdmi_phy_test_clear(hdmi, 1);
> > >> + hdmi_writeb(hdmi, HDMI_PHY_I2CM_SLAVE_ADDR_PHY_GEN2,
> > >> + HDMI_PHY_I2CM_SLAVE_ADDR);
> > >> + hdmi_phy_test_clear(hdmi, 0);
> > >> +}
> > >> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_set_slave_addr);
> > >
> > > Should this be called dw_hdmi_phy_gen2_set_slave_addr?
> >
> > Probably. I will rename it in v2 to be consistent with other phy
> > functions.
>
> The I2C write function is called dw_hdmi_phy_i2c_write(). If we want to be
> conosistent we should either rename this one to dw_hdmi_phy_i2c_set_addr()
> or rename them both to dw_hdmi_phy_gen2_i2c_write() and
> dw_hdmi_phy_gen2_i2c_set_addr(). I think I'd prefer the former, and we could
> even drop gen2 from dw_hdmi_phy_gen2_pddq() and dw_hdmi_phy_gen2_txpwron()
> if desired.
>
> > > Looks good otherwise. Same for patches 3 and 4 in this series.
> > >
> > >> +
> > >>
> > >> static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi)
> > >> {
> > >>
> > >> const struct dw_hdmi_phy_data *phy = hdmi->phy.data;
>
> [snip]
>
> --
> Regards,
>
> Laurent Pinchart