On Tue, Jun 18, 2019 at 4:24 PM Chen-Yu Tsai [off-list ref] wrote:

On Tue, Jun 18, 2019 at 6:34 PM Jagan Teki [off-list ref] wrote:

quoted

On Tue, Jun 18, 2019 at 1:23 PM Chen-Yu Tsai [off-list ref] wrote:

quoted

On Tue, Jun 18, 2019 at 3:45 PM Jagan Teki [off-list ref] wrote:

quoted

On Tue, Jun 18, 2019 at 12:49 PM Chen-Yu Tsai [off-list ref] wrote:

quoted

On Mon, Jun 17, 2019 at 6:30 PM Jagan Teki [off-list ref] wrote:

quoted

On Sun, Jun 16, 2019 at 11:01 AM Chen-Yu Tsai [off-list ref] wrote:

quoted

On Sat, Jun 15, 2019 at 12:44 AM Jagan Teki [off-list ref] wrote:

quoted

TCON TOP have clock gates for TV0, TV1, dsi and right
now these are register during bind call.

Of which, dsi clock gate would required during DPHY probe
but same can miss to get since tcon top is not bound at
that time.

To solve, this circular dependency move the clock gate
registration from bind to probe so-that DPHY can get the
dsi gate clock on time.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 94 ++++++++++++++------------
 1 file changed, 49 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
index 465e9b0cdfee..a8978b3fe851 100644
--- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
+++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c

@@ -124,7 +124,53 @@ static struct clk_hw *sun8i_tcon_top_register_gate(struct device *dev,
 static int sun8i_tcon_top_bind(struct device *dev, struct device *master,
                               void *data)
 {
-       struct platform_device *pdev = to_platform_device(dev);
+       struct sun8i_tcon_top *tcon_top = dev_get_drvdata(dev);
+       int ret;
+
+       ret = reset_control_deassert(tcon_top->rst);
+       if (ret) {
+               dev_err(dev, "Could not deassert ctrl reset control\n");
+               return ret;
+       }
+
+       ret = clk_prepare_enable(tcon_top->bus);
+       if (ret) {
+               dev_err(dev, "Could not enable bus clock\n");
+               goto err_assert_reset;
+       }

You have to de-assert the reset control and enable the clock before the
clocks it provides are registered. Otherwise a consumer may come in and
ask for the provided clock to be enabled, but since the TCON TOP's own
reset and clock are still disabled, you can't actually access the registers
that controls the provided clock.

These rst and bus are common reset and bus clocks not tcon top clocks
that are trying to register here. ie reason I have not moved it in
top.

And you're sure that toggling bits in the TCON TOP block doesn't require
the reset to be de-asserted and the bus clock enabled?

Somehow I doubt that.

Once the driver register the clocks it provides, they absolutely must work.
They can't only work after the bind phase when the reset gets de-asserted
and the bus clock enabled. Or you should provide proper error reporting
in the clock ops. I doubt you want to go that way either.

Why would they won't work after bind phase? unlike tcon top gates,
these reset, and bus are common like  what we have in other DE block
so enable them in bind won't be an issue as per as I understand. let
me know if you want me to check in other directions.

You misunderstood. When you moved the clock registering parts to the probe
phase, but didn't move the clock enable and reset de-assert parts to go with,
the clock ops will not work as expected between probe and bind time.

If I understand correctly, I have moved tcon clock gates, not the bus
clock or the reset. Both have independent enablement phase, the bus
clock is enable in tcon top bind and the clock gate ("dsi") enable in
init call of phy_ops. is both bus clock and clock gates are same and
related that is what you are saying?

I am saying that you may need the tcon top bus gates and resets properly
configured to be able to read/write the tcon top address range. That includes
enabling/disabling the clocks that the tcon top driver registers.

In other words, the TCON TOP's bus gate and reset control have everything to do
with what you can do within the TCON TOP block or address range.

quoted

quoted

Simple way to verify it: Just use devmem to disable the TCON TOP bus gate
and/or assert its reset control. Then try to toggle any of the bits in the
TCON TOP block and see if it works, or if the bits stick.

Yes I have verified "dsi" gate enablement before via devmem. Below is
the bus, reset disablement and re-enablement and result is similar for
the reset, bus clock in bind and even in probe.

00. get the existing value

# devmem 0x1c70020
0x00010000
# devmem 0x1c20064
0x44021000
# devmem 0x1c202c4
0x44021000

01: disable bus, and assert reset

# devmem 0x1c20064 32 0x4021000
# devmem 0x1c202c4 32 0x4021000
# devmem 0x1c20064
0x04021000
# devmem 0x1c202c4
0x04021000
# devmem 0x1c70020
0x00000000

See here. The value became 0 when it was still 0x10000 in the previous phase.
Any guesses to why this happened, assuming you didn't touch it?

Yes, I didn't touch anything here. and it indeed expected since the
bus and reset line goes disabled and asserted.

Now if you keep the bus gate disabled and the reset control asserted, and
try to write some non-zero value to 0x1c70020, and read it back, does the
value stick?

No, value is not stick. what ever I wrote on on 0x1c70020 it is not taking.

If you don't have the bus gate enabled and the reset control de-asserted,
any operations you do to the TCON TOP is essentially not happening. Including
bit operations that the clocks you registered are required to do.

Get what I'm saying?

I understand it, the for accessing tcon space we have bus and reset
line to be enabled and desserted. But the thing is I didn't see any
difference in the behavior even If I enable or deassert the bus and
reset in probe or in bind.

The devmem numbers which I have listed above is same for both the
cases, one with this patch and another one is handle via probe
https://paste.ubuntu.com/p/ndHj9wHzvX/

You need to have the bus gate enabled and the reset control de-asserted
BEFORE you register the clocks you are providing, or something is going
to go very wrong.

Worst case scenario: the reset control was left de-asserted by the bootloader
but the bus gate was disabled. When you register the clocks, the CCF tries
to read back the current status of the clocks, and the I/O stalls because
the bus gate wasn't enabled. System stalls.

Do I need to draw a time flow chart for you?

Sure, please.

Also see the very simple example:

    https://elixir.bootlin.com/linux/latest/source/drivers/clk/sunxi-ng/ccu-sun9i-a80-usb.c#L113

where the bus gate is enabled before registering the clocks. This hardware
block doesn't have a reset control for it, but the same principle applies.

Got it, thanks.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel