Tony, On 04/02/16 03:17, Tony Lindgren wrote: > Hi, > > * Peter Ujfalusi <peter.ujfalusi@xxxxxx> [160401 02:34]: >> So what shall we do with the OMAP3 McBSP2/3 sidetone? It has been broken in DT >> boot since the first time we booted OMAP3 with DT... Only in legacy mode we >> can have properly working ST. > > Grr. Yes :( The reason for this is that in DT boot we can not provide the enable_st_clock() callback to the mcbsp driver stack. This is done for legacy boot in mach-omap2/mcbsp.c >> I have the second level of patches based on this set (I think I need to resend >> this series since I might have changed it, can not recall) for both arch/arm >> and ASoC to have working ST in legacy and DT boot. We will no longer have >> warning regarding to broken hwmod data in DT boot. >> But all is based on the assumption that we agree at some point that the ST >> block is part of the McBSP module ;) > > The sidetone module is a separate target from the McBSP on the interconnect > but there are also direct lines between sidetone and McBSP devices :) > Here's what I'm seeing looking at the AP table on dm3730 hardware. > > McBSP target module: > 0x49022000, ap 5 06.0, McBSP2 > 0x49024000, ap 7 08.0, McBSP3 > > Sidetone target modules: > 0x49028000, ap 39 0a.0, mcbsp2_sidetone > 0x4902a000, ap 41 12.0, mcbsp3_sidetone > > And that seems to match TRM "21.6.4 SIDETONE Register Description", > "Table 2-5. L4-Peripheral Memory Space Mapping", and "Table 9-114. Region > Allocation for L4-Per Interconnect". I'm aware of this, but even today we have one single driver to handle both McBSP and the sidetone block. >> If I need to write separate driver for the McBSP module's ST block, it would >> mean some sort of API between the McBSP and ST driver. This is not straight >> forward since there are registers both in McBSP block and ST block that needs >> to be configured in specific order -> simple enable_st() would not work >> (probably enable_st_stage1(), enable_st_stage2()) and callbacks from McBSP to >> ST, ST to McBSP also going to be needed. As far as I can see it is going to be >> a huge mess. > > The McBSP and sidetone don't have parent child relationship at the > interconnect level. So I think the best option would be to have the McBSP > driver implement mcbsp_sidetone_register/unregister() etc functions. That > can then set up the necessary callbacks. Then the sidetone driver can call > them on probe/exit and set up the necessary callbacks and whatever might > be needed. > > If they are currently handled in a single driver, you you need to > pm_runtime_get both modules. The ST does not have clocks coming from PRCM level, it only uses the McBSP iclk when it is enabled (the McBSP block of the McBSP). As far as pm_runtime goes I think the ST module should not use it. We can not tell hwmod to enable/disable the McBSP2/3 iclk when we pm_runtime for the ST. It does not help at all. We can have nop action for the ST when pm_runtime is used, but then why would we have it? > So having two separate drivers might make things a lot simpler. Not really. It will make things way more complicated imho. How to handle legacy boot as we still have that supported? When the McBSP driver is loaded we must know if it has sidetone or not so we can create the needed audio controls, sysfs entries. The sysfs and kcontrol registration could be moved out to the new ST driver, true. I actually started with two separate drivers approach first, but decided that it does not worth the effort (legacy boot support, pm_runtime/hwmod hassle, platform data, callback API design, etc). I know, it is not rocket science but it is king of shoot out of cannon into sparrows. I'll think about it for a little while ;) > If you don't treat the McBSP and sidetone as separate modules, things can > easily fail. For example, doing a read-back to flush of posted write to > sidetone registers flushes nothing for McBSP and the other way around. I don't see problem with the need of flushing if we would need it. I don't think we are doing anything proactively to flush writes in the driver today and we do have at least one product using the ST (n900). >> Other option would be to deprecate the ST support as such, but that would >> leave the n900 guys in trouble as they need ST to be functional... > > That does not sound like a nice option at all :( I know. This has been bugging me for a long time. I want to fix this one before my beagleboard-xm gives up and won't boot up anymore since after that I will have no omap3 board to work with :( -- Péter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
