On 05/02/2019 19:58, Tony Lindgren wrote:
> * Tomi Valkeinen <tomi.valkeinen@xxxxxx> [190205 11:07]:
>> Yep... So there's the DSI internal code which needs to deal with ulps
>> and disconnect_lanes, and then the external interface to the DSI PLL (so
>> that DPI can use DSI PLL) without ulps/disconnect.
>>
>> I think your patch breaks this latter one, as disconnect_lanes is zero
>> in that case and would leave the regulator enabled. This would probably
>> be visible on e.g. Pandaboard, which uses DSI PLLs for the TFP410 DVI
>> output, if I recall right.
>
> Sorry I don't quite follow what happens there with dvi
> calling into dsi.. Care to describe a bit more?
We have the DSI PLL, which is "owned" by the DSI driver. Its main purpose is to clock the DSI. However, the output clock from the DSI PLL can also be muxed to go to the DPI (parallel video output, used for DVI on Panda). So for this use, the DPI driver enables, disables and configures the DSI PLL.
When using DSI PLL from DPI, we don't care about this "disconnect_lanes", we always want to fully disable everything. But when using DSI PLL from DSI, we sometimes want to keep the lanes enabled using disconnect_lanes. So the functions these two users use are a bit different.
That said, and looking at the code and trying to remember what all this is about... I think the disconnect_lanes is misplaced, and not related to the DSI PLL. The DSI code has degraded during the years quite a bit...
I think the code should always enable the regulator in pll_enable, and always disable it in pll_disable. The disconnect_lanes should be handled separately from the PLL code, as it's not really related.
Here's a quick edit about what I'm thing about, not tested:
diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
index 00a9c2ab9e6c..7e122d94ad2b 100644
--- a/drivers/gpu/drm/omapdrm/dss/dsi.c
+++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
@@ -352,7 +352,7 @@ struct dsi_data {
struct dss_pll pll;
- bool vdds_dsi_enabled;
+ bool vdds_dsi_enabled; // XXX marks if the regulator has been left on for lanes
struct regulator *vdds_dsi_reg;
struct {
@@ -1342,12 +1342,9 @@ static int dsi_pll_enable(struct dss_pll *pll)
*/
dsi_enable_scp_clk(dsi);
- if (!dsi->vdds_dsi_enabled) {
- r = regulator_enable(dsi->vdds_dsi_reg);
- if (r)
- goto err0;
- dsi->vdds_dsi_enabled = true;
- }
+ r = regulator_enable(dsi->vdds_dsi_reg);
+ if (r)
+ goto err0;
/* XXX PLL does not come out of reset without this... */
dispc_pck_free_enable(dsi->dss->dispc, 1);
@@ -1372,36 +1369,25 @@ static int dsi_pll_enable(struct dss_pll *pll)
return 0;
err1:
- if (dsi->vdds_dsi_enabled) {
- regulator_disable(dsi->vdds_dsi_reg);
- dsi->vdds_dsi_enabled = false;
- }
+ regulator_disable(dsi->vdds_dsi_reg);
err0:
dsi_disable_scp_clk(dsi);
dsi_runtime_put(dsi);
return r;
}
-static void dsi_pll_uninit(struct dsi_data *dsi, bool disconnect_lanes)
+static void dsi_pll_disable(struct dss_pll *pll)
{
+ struct dsi_data *dsi = container_of(pll, struct dsi_data, pll);
+
dsi_pll_power(dsi, DSI_PLL_POWER_OFF);
- if (disconnect_lanes) {
- WARN_ON(!dsi->vdds_dsi_enabled);
- regulator_disable(dsi->vdds_dsi_reg);
- dsi->vdds_dsi_enabled = false;
- }
+
+ regulator_disable(dsi->vdds_dsi_reg);
dsi_disable_scp_clk(dsi);
dsi_runtime_put(dsi);
- DSSDBG("PLL uninit done\n");
-}
-
-static void dsi_pll_disable(struct dss_pll *pll)
-{
- struct dsi_data *dsi = container_of(pll, struct dsi_data, pll);
-
- dsi_pll_uninit(dsi, true);
+ DSSDBG("PLL disable done\n");
}
static int dsi_dump_dsi_clocks(struct seq_file *s, void *p)
@@ -4108,6 +4094,10 @@ static int dsi_display_init_dsi(struct dsi_data *dsi)
DSSDBG("PLL OK\n");
+ // XXX enable the regulator for the lanes
+ regulator_enable(dsi->vdds_dsi_reg);
+ dsi->vdds_dsi_enabled = true;
+
r = dsi_cio_init(dsi);
if (r)
goto err2;
@@ -4136,6 +4126,10 @@ static int dsi_display_init_dsi(struct dsi_data *dsi)
err3:
dsi_cio_uninit(dsi);
err2:
+ // XXX disable the regulator for the lanes
+ regulator_disable(dsi->vdds_dsi_reg);
+ dsi->vdds_dsi_enabled = false;
+
dss_select_dsi_clk_source(dsi->dss, dsi->module_id, DSS_CLK_SRC_FCK);
err1:
dss_pll_disable(&dsi->pll);
@@ -4158,7 +4152,12 @@ static void dsi_display_uninit_dsi(struct dsi_data *dsi, bool disconnect_lanes,
dss_select_dsi_clk_source(dsi->dss, dsi->module_id, DSS_CLK_SRC_FCK);
dsi_cio_uninit(dsi);
- dsi_pll_uninit(dsi, disconnect_lanes);
+ dss_pll_disable(&dsi->pll);
+
+ if (disconnect_lanes) {
+ regulator_disable(dsi->vdds_dsi_reg);
+ dsi->vdds_dsi_enabled = false;
+ }
}
static int dsi_display_enable(struct omap_dss_device *dssdev)
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
