From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755135AbcDDMqi (ORCPT ); Mon, 4 Apr 2016 08:46:38 -0400 Received: from devils.ext.ti.com ([198.47.26.153]:41785 "EHLO devils.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751995AbcDDMqg (ORCPT ); Mon, 4 Apr 2016 08:46:36 -0400 Subject: Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone To: Tony Lindgren References: <1458311007-19168-1-git-send-email-peter.ujfalusi@ti.com> <56EFB794.5010505@ti.com> <56FE407E.9030803@ti.com> <20160402001753.GR9329@atomide.com> CC: Paul Walmsley , , , , , , From: Peter Ujfalusi X-Enigmail-Draft-Status: N1110 Message-ID: <57026205.6020105@ti.com> Date: Mon, 4 Apr 2016 15:45:57 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.1 MIME-Version: 1.0 In-Reply-To: <20160402001753.GR9329@atomide.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Tony, On 04/02/16 03:17, Tony Lindgren wrote: > Hi, > > * Peter Ujfalusi [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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Ujfalusi Subject: Re: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone Date: Mon, 4 Apr 2016 15:45:57 +0300 Message-ID: <57026205.6020105@ti.com> References: <1458311007-19168-1-git-send-email-peter.ujfalusi@ti.com> <56EFB794.5010505@ti.com> <56FE407E.9030803@ti.com> <20160402001753.GR9329@atomide.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <20160402001753.GR9329-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Tony Lindgren Cc: Paul Walmsley , jarkko.nikula-FVTvWyuFUl3QT0dZR+AlfA@public.gmane.org, t-kristo-l0cyMroinI0@public.gmane.org, linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: devicetree@vger.kernel.org Tony, On 04/02/16 03:17, Tony Lindgren wrote: > Hi, >=20 > * Peter Ujfalusi [160401 02:34]: >> So what shall we do with the OMAP3 McBSP2/3 sidetone? It has been br= oken in DT >> boot since the first time we booted OMAP3 with DT... Only in legacy = mode we >> can have properly working ST. >=20 > 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 ;) >=20 > The sidetone module is a separate target from the McBSP on the interc= onnect > 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. >=20 > McBSP target module: > 0x49022000, ap 5 06.0, McBSP2 > 0x49024000, ap 7 08.0, McBSP3 >=20 > Sidetone target modules: > 0x49028000, ap 39 0a.0, mcbsp2_sidetone > 0x4902a000, ap 41 12.0, mcbsp3_sidetone >=20 > And that seems to match TRM "21.6.4 SIDETONE Register Description", > "Table 2-5. L4-Peripheral Memory Space Mapping", and "Table 9-114. Re= gion > Allocation for L4-Per Interconnect". I'm aware of this, but even today we have one single driver to handle b= oth 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 s= traight >> forward since there are registers both in McBSP block and ST block t= hat needs >> to be configured in specific order -> simple enable_st() would not w= ork >> (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 g= oing 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 M= cBSP > 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 mi= ght > be needed. >=20 > 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 Mc= BSP iclk when it is enabled (the McBSP block of the McBSP). As far as pm_ru= ntime 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 handl= e legacy boot as we still have that supported? When the McBSP driver is l= oaded we must know if it has sidetone or not so we can create the needed audi= o controls, sysfs entries. The sysfs and kcontrol registration could be m= oved out to the new ST driver, true. I actually started with two separate drivers approach first, but decide= d that it does not worth the effort (legacy boot support, pm_runtime/hwmod has= sle, 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= =2E I don't see problem with the need of flushing if we would need it. I do= n't think we are doing anything proactively to flush writes in the driver t= oday 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... >=20 > 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 on= e before my beagleboard-xm gives up and won't boot up anymore since after= that I will have no omap3 board to work with :( --=20 P=E9ter -- To unsubscribe from this list: send the line "unsubscribe devicetree" i= n the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: peter.ujfalusi@ti.com (Peter Ujfalusi) Date: Mon, 4 Apr 2016 15:45:57 +0300 Subject: [PATCH v2 0/3] ARM: OMAP3: Fix McBSP2/3 hwmod setup for sidetone In-Reply-To: <20160402001753.GR9329@atomide.com> References: <1458311007-19168-1-git-send-email-peter.ujfalusi@ti.com> <56EFB794.5010505@ti.com> <56FE407E.9030803@ti.com> <20160402001753.GR9329@atomide.com> Message-ID: <57026205.6020105@ti.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Tony, On 04/02/16 03:17, Tony Lindgren wrote: > Hi, > > * Peter Ujfalusi [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