[PATCHv2 15/15] clk: ti: convert to use proper register definition for all accesses

[Adam Ford]

On Thu, Jan 18, 2018 at 1:34 AM, Tero Kristo <t-kristo at ti.com> wrote:
> On 17/01/18 23:44, Adam Ford wrote:
>>
>> On Wed, Jan 17, 2018 at 3:19 PM, Tony Lindgren <tony at atomide.com> wrote:
>>>
>>> * Adam Ford <aford173 at gmail.com> [180117 15:15]:
>>>>
>>>> On Wed, Jan 17, 2018 at 8:02 AM, Tero Kristo <t-kristo at ti.com> wrote:
>>>>>
>>>>> On 17/01/18 15:27, Adam Ford wrote:
>>>>>>
>>>>>>
>>>>>> On Sat, Mar 11, 2017 at 6:50 AM, Tero Kristo <t-kristo at ti.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>> Currently, TI clock driver uses an encapsulated struct that is cast
>>>>>>> into
>>>>>>> a void pointer to store all register addresses. This can be
>>>>>>> considered
>>>>>>> as rather nasty hackery, and prevents from expanding the register
>>>>>>> address field also. Instead, replace all the code to use proper
>>>>>>> struct
>>>>>>> in place for this, which contains all the previously used data.
>>>>>>>
>>>>>>> This patch is rather large as it is touching multiple files, but this
>>>>>>> can't be split up as we need to avoid any boot breakage.
>>>>>>>
>>>>>>
>>>>>> I know it's late coming, but according to git bisect, this patch is
>>>>>> causing some problems with Logic PD Torpedo 37xx Dev kit.
>>>>>
>>>>>
>>>>>
>>>>> Oh reporting bugs is never too late, thanks for posting this out.
>>>>>
>>>>>>
>>>>>> It it is a DM3730 that has a WL1283 chipset attached to the SDIO
>>>>>> interface on MMC3.  The driver seems to load properly, but when
>>>>>> loading wpa_supplicant to activate the WL1283, we get a giant crash.
>>>>>> I checked kernel revisions starting at 4.14 and working back to when
>>>>>> it worked, then used git bisect from there.
>>>>>>
>>>>>> I am hoping it might be a simple fix for something that just needs to
>>>>>> get added or tweaked in the device tree.
>>>>>
>>>>>
>>>>>
>>>>> I don't have access to the specific hw, but can you try to dig out
>>>>> which
>>>>> hwmod is causing the crash? Just print out the oh->name from the
>>>>> _wait_softreset_complete. That would help root causing the issue.
>>>>>
>>>>
>>>> With one small patch, I was able to make it work again.
>>>>
>>>> diff --git a/arch/arm/mach-omap2/omap_hwmod.c
>>>> b/arch/arm/mach-omap2/omap_hwmod.c
>>>> index 2dbd632..ed1f625 100644
>>>> --- a/arch/arm/mach-omap2/omap_hwmod.c
>>>> +++ b/arch/arm/mach-omap2/omap_hwmod.c
>>>> @@ -477,7 +477,7 @@ static int _wait_softreset_complete(struct
>>>> omap_hwmod *oh)
>>>>          int c = 0;
>>>>
>>>>          sysc = oh->class->sysc;
>>>> -
>>>> +pr_warn("_wait_softreset_complete: %s\n", oh->name);
>>>>          if (sysc->sysc_flags & SYSS_HAS_RESET_STATUS)
>>>>                  omap_test_timeout((omap_hwmod_read(oh, sysc->syss_offs)
>>>>                                     & SYSS_RESETDONE_MASK),
>>>>
>>>>
>>>> This leads me to believe that the omap_test_timeout functions might
>>>> not be working quite right.
>>>
>>>
>>> There may be a srst_udelay needed for some module, see commit
>>> ebf244148092 ("ARM: OMAP2+: Use srst_udelay for USB on dm814x")
>>> for example.
>>>
>>> You might be able to find which module it is by commenting out
>>> postcore_initcall_sync(omap3_l3_init) in drivers/bus/omap_l3_smx.c
>>> temporarily as the system will most likely hang right there.
>>
>>
>> I commented out that line as you suggested, but the system boots as
>> normal and I get the crash (as normal)
>>
>> I am looking through the DM3730 and OMAP3630 TRM now.  Any thought on
>> a keyword search I should use to see which hwmods might require
>> srst_udelay?
>
>
> Looking at the log you provided, it looks like only mmc1, mmc3 and i2c1 are
> reset during the wlan probe. Based on the prints coming out in the failing
> case, it looks like the culprit might be mmc3 for some reason.
>
> [   18.239746] _wait_softreset_complete: mmc3
> [   18.638580] _wait_softreset_complete: mmc3
> [   18.657562] _wait_softreset_complete: mmc1
> [   18.833374] wlcore: firmware booted (Rev 7.3.10.0.141)
>
> ^ the firmware notification above does not come out in the crash.
>

I agree with your assessment.  Any ideas why moving the debug
statement before the if statement would make it start working?  I
added some artificial udelay at around 100, but I still got the crash.
It seems like there is some timing issue, but at the same time just
adding a delay isn't enough.

> -Tero
>

adam

>>
>> adam
>>
>>>
>>> Regards,
>>>
>>> Tony
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki