| Message ID | E1X3NMM-0000A5-N5@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/05/2014 12:38 PM, Russell King wrote: > Move the variant initialisation entirely to the CRTC init function - > the variant support is really about the CRTC properties than the whole > system, and we want to treat each CRTC individually when we support DT. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > --- [...] > diff --git a/drivers/gpu/drm/armada/armada_crtc.h b/drivers/gpu/drm/armada/armada_crtc.h > index 531a9b0bdcfb..3f0e70bb2e9c 100644 > --- a/drivers/gpu/drm/armada/armada_crtc.h > +++ b/drivers/gpu/drm/armada/armada_crtc.h > @@ -38,6 +38,7 @@ struct armada_crtc { > unsigned num; > void __iomem *base; > struct clk *clk; > + struct clk *extclk[2]; Russell, I wonder, if we should rename above array srcclk instead of extclk while moving it anyway. That way we can use it for the other variant specific clocks, too. FWIW, I totally agree the it was the right thing to wait for you to sort out the dependencies. Good work, great patience. Sebastian
On Sat, Jul 05, 2014 at 01:58:37PM +0200, Sebastian Hesselbarth wrote: > On 07/05/2014 12:38 PM, Russell King wrote: > > Move the variant initialisation entirely to the CRTC init function - > > the variant support is really about the CRTC properties than the whole > > system, and we want to treat each CRTC individually when we support DT. > > > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > --- > [...] > > diff --git a/drivers/gpu/drm/armada/armada_crtc.h b/drivers/gpu/drm/armada/armada_crtc.h > > index 531a9b0bdcfb..3f0e70bb2e9c 100644 > > --- a/drivers/gpu/drm/armada/armada_crtc.h > > +++ b/drivers/gpu/drm/armada/armada_crtc.h > > @@ -38,6 +38,7 @@ struct armada_crtc { > > unsigned num; > > void __iomem *base; > > struct clk *clk; > > + struct clk *extclk[2]; > > Russell, > > I wonder, if we should rename above array srcclk instead of extclk > while moving it anyway. That way we can use it for the other variant > specific clocks, too. pixelclk may be a better name for it. I would like to think about the clock handling further though - the issues surrounding clock selection are not limited to just Armada - imx-drm has the exact same problem. The issue with clocking of CRTCs is that it seems to be common that: 1. you have multiple clocks to choose from, some of which may be more suitable than others depending on the type of output. 2. clocks end up being shared between multiple CRTCs, and one CRTC can (at the moment) interfere with the clock rate delivered to another CRTC. This happens on imx-drm today, where the two DIs (CRTCs) are in use - one for HDMI, the other for LVDS. We end up with HDMI set first to 148.5MHz, and then LVDS sets it's clock to 65MHz, which results in HDMI receiving a clock at over 500MHz! At the moment, there are hacks to solve this by adjusting the muxes in the clock paths to ensure that they both derive from different PLLs - moving the LVDS onto the USB OTG PLL rather than the video PLL. That works fine until USB OTG wants to change the OTG PLL. There's also the issue whether the output can cope with fractional clock-skipping dividers - entirely synchronous display systems can (such as synchronously clocked LCD panels), but asynchronous display systems (such as HDMI, TV out, etc) can't. That said, the other parameter that needs to be taken account of here is that even with the fractional divider, the minimum output clock period isn't the average frequency, but the maximum frequency, which may violate a panel's minimum clock period specification. I think there's lots to do on the clocking side, and as it's a fairly complex problem which is common to multiple implementations, I think that any solution should not be specific. However, this topic isn't one which I want to work on until I have reduced down my patch sets to something more manageable - something which I'm desperate to do. (I've been trying to avoid adding any further patches to any tree for some time now.) This is why (eg) I'm not going to fix the kernel oops-able bugs I found in the SGTL5000 codec - someone else can do that.
On 07/05/2014 02:21 PM, Russell King - ARM Linux wrote: > On Sat, Jul 05, 2014 at 01:58:37PM +0200, Sebastian Hesselbarth wrote: >> On 07/05/2014 12:38 PM, Russell King wrote: >>> Move the variant initialisation entirely to the CRTC init function - >>> the variant support is really about the CRTC properties than the whole >>> system, and we want to treat each CRTC individually when we support DT. >>> >>> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> >>> --- >> [...] >>> diff --git a/drivers/gpu/drm/armada/armada_crtc.h b/drivers/gpu/drm/armada/armada_crtc.h >>> index 531a9b0bdcfb..3f0e70bb2e9c 100644 >>> --- a/drivers/gpu/drm/armada/armada_crtc.h >>> +++ b/drivers/gpu/drm/armada/armada_crtc.h >>> @@ -38,6 +38,7 @@ struct armada_crtc { >>> unsigned num; >>> void __iomem *base; >>> struct clk *clk; >>> + struct clk *extclk[2]; >> >> I wonder, if we should rename above array srcclk instead of extclk >> while moving it anyway. That way we can use it for the other variant >> specific clocks, too. > > pixelclk may be a better name for it. I would like to think about the The name was derived from the "SCLK_SOURCE_SELECT" bits in Dove FS, but any other name not limited to external clocks is fine, too. > clock handling further though - the issues surrounding clock selection > are not limited to just Armada - imx-drm has the exact same problem. > > The issue with clocking of CRTCs is that it seems to be common that: > > 1. you have multiple clocks to choose from, some of which may be more > suitable than others depending on the type of output. Given the limited capabilities of the internal clock dividers, on Dove the heuristic seems to be fairly simple: always prefer the external clock. This is true for all actively supported Dove boards (Cubox and {d2,d3}plug) as all have an external PLL connected. I'd even say to make the external clock mandatory. Hitting a standard pixclk frequency with one of the internal clocks is pure coincidence. As long as there is no board using anything else than HDMI transmitter on dumb RGB, we shouldn't try to foresee any suitable heuristic. > 2. clocks end up being shared between multiple CRTCs, and one CRTC > can (at the moment) interfere with the clock rate delivered to > another CRTC. > > This happens on imx-drm today, where the two DIs (CRTCs) are in use - > one for HDMI, the other for LVDS. We end up with HDMI set first to > 148.5MHz, and then LVDS sets it's clock to 65MHz, which results in > HDMI receiving a clock at over 500MHz! At the moment, there are hacks > to solve this by adjusting the muxes in the clock paths to ensure that > they both derive from different PLLs - moving the LVDS onto the USB OTG > PLL rather than the video PLL. That works fine until USB OTG wants > to change the OTG PLL. Again, luckily on Cubox we have 2 VCOs for the two external clocks (audio and video) so we won't have to deal with it. For the {d2,d3}plug I'll have to check the IDT datasheet again. In particular, for imx maybe it is possible to identify some clock tree configurations for specific use-cases. I don't think there is any non- manual way to tell the best clock tree config. > There's also the issue whether the output can cope with fractional > clock-skipping dividers - entirely synchronous display systems can > (such as synchronously clocked LCD panels), but asynchronous display > systems (such as HDMI, TV out, etc) can't. That said, the other > parameter that needs to be taken account of here is that even with the > fractional divider, the minimum output clock period isn't the average > frequency, but the maximum frequency, which may violate a panel's minimum > clock period specification. Yeah, the fractional divider isn't made for external HDMI transmitters for sure. I have seen from your branch that there is some Armada 610 stub for OLPC, do they have a dumb panel directly connected? I also saw that they do have an external PLL, so maybe we should stick to the external clock inputs as long as no other configurations pops-up (which may never happen). Sebastian > I think there's lots to do on the clocking side, and as it's a fairly > complex problem which is common to multiple implementations, I think > that any solution should not be specific. > > However, this topic isn't one which I want to work on until I have > reduced down my patch sets to something more manageable - something > which I'm desperate to do. (I've been trying to avoid adding any > further patches to any tree for some time now.) This is why (eg) I'm > not going to fix the kernel oops-able bugs I found in the SGTL5000 > codec - someone else can do that.
On Sun, Jul 06, 2014 at 11:46:56AM +0200, Sebastian Hesselbarth wrote: > On 07/05/2014 02:21 PM, Russell King - ARM Linux wrote: > > There's also the issue whether the output can cope with fractional > > clock-skipping dividers - entirely synchronous display systems can > > (such as synchronously clocked LCD panels), but asynchronous display > > systems (such as HDMI, TV out, etc) can't. That said, the other > > parameter that needs to be taken account of here is that even with the > > fractional divider, the minimum output clock period isn't the average > > frequency, but the maximum frequency, which may violate a panel's minimum > > clock period specification. > > Yeah, the fractional divider isn't made for external HDMI transmitters > for sure. I have seen from your branch that there is some Armada 610 > stub for OLPC, do they have a dumb panel directly connected? I believe they do have a dumb panel connected directly. I've asked Jon what the status of OLPC is with mainline kernels, and how difficult it would be to get to the stage where the 610 stuff could be finished off.
On Sat, Jul 05, 2014 at 01:58:37PM +0200, Sebastian Hesselbarth wrote: > On 07/05/2014 12:38 PM, Russell King wrote: > > Move the variant initialisation entirely to the CRTC init function - > > the variant support is really about the CRTC properties than the whole > > system, and we want to treat each CRTC individually when we support DT. > > > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > --- > [...] > > diff --git a/drivers/gpu/drm/armada/armada_crtc.h b/drivers/gpu/drm/armada/armada_crtc.h > > index 531a9b0bdcfb..3f0e70bb2e9c 100644 > > --- a/drivers/gpu/drm/armada/armada_crtc.h > > +++ b/drivers/gpu/drm/armada/armada_crtc.h > > @@ -38,6 +38,7 @@ struct armada_crtc { > > unsigned num; > > void __iomem *base; > > struct clk *clk; > > + struct clk *extclk[2]; > > Russell, > > I wonder, if we should rename above array srcclk instead of extclk > while moving it anyway. That way we can use it for the other variant > specific clocks, too. As the patches are prepared with this change, I'd prefer to submit them as-is, and then we can update that as and when the support for things like the MMP/610 is finished off. I think they're good to go, so I'll send them off later today to David. This leaves the TDA998x componentisation patches which I need to kick out, and the initial DT changes. Once those are in place, we should have almost all ducks lined up for working DRM support - it'll certainly be advanced enough to describe the LCD controllers and the TDA998x as three separate DT entities using the of graph helpers. What's left is the display-subsystem { } entity to describe the makeup of the subsystem. That's not included as we currently need to pass a block of memory, and the DT support for reserving chunks of memory appeared (last time I looked) to only be botch-merged (only half of it seems to have been merged making the whole reserved memory thing totally useless - why people only half-merge features I've no idea.)
On 07/11/2014 04:37 PM, Russell King - ARM Linux wrote: > On Sat, Jul 05, 2014 at 01:58:37PM +0200, Sebastian Hesselbarth wrote: >> On 07/05/2014 12:38 PM, Russell King wrote: >>> Move the variant initialisation entirely to the CRTC init function - >>> the variant support is really about the CRTC properties than the whole >>> system, and we want to treat each CRTC individually when we support DT. >>> >>> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> >>> --- >> [...] >>> diff --git a/drivers/gpu/drm/armada/armada_crtc.h b/drivers/gpu/drm/armada/armada_crtc.h >>> index 531a9b0bdcfb..3f0e70bb2e9c 100644 >>> --- a/drivers/gpu/drm/armada/armada_crtc.h >>> +++ b/drivers/gpu/drm/armada/armada_crtc.h >>> @@ -38,6 +38,7 @@ struct armada_crtc { >>> unsigned num; >>> void __iomem *base; >>> struct clk *clk; >>> + struct clk *extclk[2]; >> >> Russell, >> >> I wonder, if we should rename above array srcclk instead of extclk >> while moving it anyway. That way we can use it for the other variant >> specific clocks, too. > > As the patches are prepared with this change, I'd prefer to submit them > as-is, and then we can update that as and when the support for things > like the MMP/610 is finished off. I think they're good to go, so I'll > send them off later today to David. Ok, sounds fine to me. > This leaves the TDA998x componentisation patches which I need to kick > out, and the initial DT changes. Once those are in place, we should > have almost all ducks lined up for working DRM support - it'll certainly > be advanced enough to describe the LCD controllers and the TDA998x as > three separate DT entities using the of graph helpers. Ok. > What's left is the display-subsystem { } entity to describe the makeup > of the subsystem. That's not included as we currently need to pass > a block of memory, and the DT support for reserving chunks of memory > appeared (last time I looked) to only be botch-merged (only half of it > seems to have been merged making the whole reserved memory thing > totally useless - why people only half-merge features I've no idea.) There was a follow-up patch set for this some days ago http://comments.gmane.org/gmane.linux.ports.arm.kernel/337686 Sebastian
On Fri, Jul 11, 2014 at 05:18:50PM +0200, Sebastian Hesselbarth wrote: > On 07/11/2014 04:37 PM, Russell King - ARM Linux wrote: >> What's left is the display-subsystem { } entity to describe the makeup >> of the subsystem. That's not included as we currently need to pass >> a block of memory, and the DT support for reserving chunks of memory >> appeared (last time I looked) to only be botch-merged (only half of it >> seems to have been merged making the whole reserved memory thing >> totally useless - why people only half-merge features I've no idea.) > > There was a follow-up patch set for this some days ago > http://comments.gmane.org/gmane.linux.ports.arm.kernel/337686 Yes, I did a bit of digging a while back and found the outstanding stuff, but it wasn't clear what's happening with it. As it isn't part of mainline, and I don't want to pick up further patches to add dependencies, I decided it was better to stick with old proven ways of a manually declared platform device for the time being.
diff --git a/drivers/gpu/drm/armada/armada_510.c b/drivers/gpu/drm/armada/armada_510.c index 59948eff6095..a9dabcaef92e 100644 --- a/drivers/gpu/drm/armada/armada_510.c +++ b/drivers/gpu/drm/armada/armada_510.c @@ -15,20 +15,19 @@ #include "armada_drm.h" #include "armada_hw.h" -static int armada510_init(struct armada_private *priv, struct device *dev) +static int armada510_crtc_init(struct armada_crtc *dcrtc, struct device *dev) { - priv->extclk[0] = devm_clk_get(dev, "ext_ref_clk_1"); + struct clk *clk; - if (IS_ERR(priv->extclk[0]) && PTR_ERR(priv->extclk[0]) == -ENOENT) - priv->extclk[0] = ERR_PTR(-EPROBE_DEFER); + clk = devm_clk_get(dev, "ext_ref_clk_1"); + if (IS_ERR(clk)) + return PTR_ERR(clk) == -ENOENT ? -EPROBE_DEFER : PTR_ERR(clk); - return PTR_RET(priv->extclk[0]); -} + dcrtc->extclk[0] = clk; -static int armada510_crtc_init(struct armada_crtc *dcrtc) -{ /* Lower the watermark so to eliminate jitter at higher bandwidths */ armada_updatel(0x20, (1 << 11) | 0xff, dcrtc->base + LCD_CFG_RDREG4F); + return 0; } @@ -45,8 +44,7 @@ static int armada510_crtc_init(struct armada_crtc *dcrtc) static int armada510_crtc_compute_clock(struct armada_crtc *dcrtc, const struct drm_display_mode *mode, uint32_t *sclk) { - struct armada_private *priv = dcrtc->crtc.dev->dev_private; - struct clk *clk = priv->extclk[0]; + struct clk *clk = dcrtc->extclk[0]; int ret; if (dcrtc->num == 1) @@ -81,7 +79,6 @@ static int armada510_crtc_compute_clock(struct armada_crtc *dcrtc, const struct armada_variant armada510_ops = { .has_spu_adv_reg = true, .spu_adv_reg = ADV_HWC32ENABLE | ADV_HWC32ARGB | ADV_HWC32BLEND, - .init = armada510_init, .crtc_init = armada510_crtc_init, .crtc_compute_clock = armada510_crtc_compute_clock, }; diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c index 4336dfd3585d..3adddabc3626 100644 --- a/drivers/gpu/drm/armada/armada_crtc.c +++ b/drivers/gpu/drm/armada/armada_crtc.c @@ -1108,7 +1108,7 @@ int armada_drm_crtc_create(struct drm_device *dev, struct resource *res, } if (priv->variant->crtc_init) { - ret = priv->variant->crtc_init(dcrtc); + ret = priv->variant->crtc_init(dcrtc, dev->dev); if (ret) { kfree(dcrtc); return ret; diff --git a/drivers/gpu/drm/armada/armada_crtc.h b/drivers/gpu/drm/armada/armada_crtc.h index 531a9b0bdcfb..3f0e70bb2e9c 100644 --- a/drivers/gpu/drm/armada/armada_crtc.h +++ b/drivers/gpu/drm/armada/armada_crtc.h @@ -38,6 +38,7 @@ struct armada_crtc { unsigned num; void __iomem *base; struct clk *clk; + struct clk *extclk[2]; struct { uint32_t spu_v_h_total; uint32_t spu_v_porch; diff --git a/drivers/gpu/drm/armada/armada_drm.h b/drivers/gpu/drm/armada/armada_drm.h index a72cae03b99b..a5452ae883d1 100644 --- a/drivers/gpu/drm/armada/armada_drm.h +++ b/drivers/gpu/drm/armada/armada_drm.h @@ -59,10 +59,9 @@ void armada_drm_vbl_event_remove_unlocked(struct armada_crtc *, struct armada_private; struct armada_variant { - bool has_spu_adv_reg; + bool has_spu_adv_reg; uint32_t spu_adv_reg; - int (*init)(struct armada_private *, struct device *); - int (*crtc_init)(struct armada_crtc *); + int (*crtc_init)(struct armada_crtc *, struct device *); int (*crtc_compute_clock)(struct armada_crtc *, const struct drm_display_mode *, uint32_t *); @@ -78,7 +77,6 @@ struct armada_private { struct drm_fb_helper *fbdev; struct armada_crtc *dcrtc[2]; struct drm_mm linear; - struct clk *extclk[2]; struct drm_property *csc_yuv_prop; struct drm_property *csc_rgb_prop; struct drm_property *colorkey_prop; diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c index add8b101fa9e..4939a86a2afc 100644 --- a/drivers/gpu/drm/armada/armada_drv.c +++ b/drivers/gpu/drm/armada/armada_drv.c @@ -130,10 +130,6 @@ static int armada_drm_load(struct drm_device *dev, unsigned long flags) priv->variant = (struct armada_variant *)id->driver_data; - ret = priv->variant->init(priv, dev->dev); - if (ret) - return ret; - INIT_WORK(&priv->fb_unref_work, armada_drm_unref_work); INIT_KFIFO(priv->fb_unref);
Move the variant initialisation entirely to the CRTC init function - the variant support is really about the CRTC properties than the whole system, and we want to treat each CRTC individually when we support DT. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- drivers/gpu/drm/armada/armada_510.c | 19 ++++++++----------- drivers/gpu/drm/armada/armada_crtc.c | 2 +- drivers/gpu/drm/armada/armada_crtc.h | 1 + drivers/gpu/drm/armada/armada_drm.h | 6 ++---- drivers/gpu/drm/armada/armada_drv.c | 4 ---- 5 files changed, 12 insertions(+), 20 deletions(-)