| Message ID | 1442326206-30192-1-git-send-email-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Paul, On 09/15/2015 05:10 PM, Peter Ujfalusi wrote: > McASP3 is used by default on DRA7x based boards for audio. Can you take a look at this patch? It would be great to have this one reviewed so the audio support for the dra7 family could be applied by Tony. Thanks, Péter > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > --- > Hi Paul, > > this patch is part of my earlier series and as Tony suggested I'll resend the > hwmod patch for you to review since I missed you from the TO in the series. > > The original series: > https://www.mail-archive.com/linux-omap@vger.kernel.org/msg119319.html > > Regards, > Peter > > arch/arm/mach-omap2/omap_hwmod_7xx_data.c | 41 +++++++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c > index 562247bced49..c38b7fa30c27 100644 > --- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c > +++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c > @@ -1298,6 +1298,38 @@ static struct omap_hwmod dra7xx_mcspi4_hwmod = { > }; > > /* > + * 'mcasp' class > + * > + */ > +static struct omap_hwmod_class_sysconfig dra7xx_mcasp_sysc = { > + .sysc_offs = 0x0004, > + .sysc_flags = SYSC_HAS_SIDLEMODE, > + .idlemodes = (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART), > + .sysc_fields = &omap_hwmod_sysc_type3, > +}; > + > +static struct omap_hwmod_class dra7xx_mcasp_hwmod_class = { > + .name = "mcasp", > + .sysc = &dra7xx_mcasp_sysc, > +}; > + > +/* mcasp3 */ > +static struct omap_hwmod dra7xx_mcasp3_hwmod = { > + .name = "mcasp3", > + .class = &dra7xx_mcasp_hwmod_class, > + .clkdm_name = "l4per2_clkdm", > + .main_clk = "mcasp3_ahclkx_mux", > + .flags = HWMOD_SWSUP_SIDLE, > + .prcm = { > + .omap4 = { > + .clkctrl_offs = DRA7XX_CM_L4PER2_MCASP3_CLKCTRL_OFFSET, > + .context_offs = DRA7XX_RM_L4PER2_MCASP3_CONTEXT_OFFSET, > + .modulemode = MODULEMODE_SWCTRL, > + }, > + }, > +}; > + > +/* > * 'mmc' class > * > */ > @@ -2566,6 +2598,14 @@ static struct omap_hwmod_ocp_if dra7xx_l3_main_1__hdmi = { > .user = OCP_USER_MPU | OCP_USER_SDMA, > }; > > +/* l4_per2 -> mcasp3 */ > +static struct omap_hwmod_ocp_if dra7xx_l4_per2__mcasp3 = { > + .master = &dra7xx_l4_per2_hwmod, > + .slave = &dra7xx_mcasp3_hwmod, > + .clk = "l3_iclk_div", > + .user = OCP_USER_MPU | OCP_USER_SDMA, > +}; > + > static struct omap_hwmod_addr_space dra7xx_elm_addrs[] = { > { > .pa_start = 0x48078000, > @@ -3338,6 +3378,7 @@ static struct omap_hwmod_ocp_if *dra7xx_hwmod_ocp_ifs[] __initdata = { > &dra7xx_l4_wkup__dcan1, > &dra7xx_l4_per2__dcan2, > &dra7xx_l4_per2__cpgmac0, > + &dra7xx_l4_per2__mcasp3, > &dra7xx_gmac__mdio, > &dra7xx_l4_cfg__dma_system, > &dra7xx_l3_main_1__dss, >
Hi Péter, a few comments: On Tue, 15 Sep 2015, Peter Ujfalusi wrote: > McASP3 is used by default on DRA7x based boards for audio. > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > --- > Hi Paul, > > this patch is part of my earlier series and as Tony suggested I'll resend the > hwmod patch for you to review since I missed you from the TO in the series. > > The original series: > https://www.mail-archive.com/linux-omap@vger.kernel.org/msg119319.html > > Regards, > Peter > > arch/arm/mach-omap2/omap_hwmod_7xx_data.c | 41 +++++++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c > index 562247bced49..c38b7fa30c27 100644 > --- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c > +++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c > @@ -1298,6 +1298,38 @@ static struct omap_hwmod dra7xx_mcspi4_hwmod = { > }; > > /* > + * 'mcasp' class > + * > + */ > +static struct omap_hwmod_class_sysconfig dra7xx_mcasp_sysc = { > + .sysc_offs = 0x0004, > + .sysc_flags = SYSC_HAS_SIDLEMODE, > + .idlemodes = (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART), > + .sysc_fields = &omap_hwmod_sysc_type3, > +}; > + > +static struct omap_hwmod_class dra7xx_mcasp_hwmod_class = { > + .name = "mcasp", > + .sysc = &dra7xx_mcasp_sysc, > +}; > + > +/* mcasp3 */ > +static struct omap_hwmod dra7xx_mcasp3_hwmod = { > + .name = "mcasp3", > + .class = &dra7xx_mcasp_hwmod_class, > + .clkdm_name = "l4per2_clkdm", > + .main_clk = "mcasp3_ahclkx_mux", I'd expect this clock to be something derived from mcasp3_aux_gfclk, according to Table 24-408 "Clocks and Resets" of SPRUHZ6. Could you please doublecheck this? > + .flags = HWMOD_SWSUP_SIDLE, Is this needed? If it is, please add a brief comment describing the issue or bug that it's working around. > + .prcm = { > + .omap4 = { > + .clkctrl_offs = DRA7XX_CM_L4PER2_MCASP3_CLKCTRL_OFFSET, > + .context_offs = DRA7XX_RM_L4PER2_MCASP3_CONTEXT_OFFSET, > + .modulemode = MODULEMODE_SWCTRL, > + }, > + }, > +}; > + > +/* > * 'mmc' class > * > */ > @@ -2566,6 +2598,14 @@ static struct omap_hwmod_ocp_if dra7xx_l3_main_1__hdmi = { > .user = OCP_USER_MPU | OCP_USER_SDMA, > }; > > +/* l4_per2 -> mcasp3 */ > +static struct omap_hwmod_ocp_if dra7xx_l4_per2__mcasp3 = { > + .master = &dra7xx_l4_per2_hwmod, > + .slave = &dra7xx_mcasp3_hwmod, So this is the low-speed control/register access port, where the MPU writes to the McASP3 config registers... > + .clk = "l3_iclk_div", ... and thus this interface clock doesn't look right for this port, since it's most likely generated from the L4PER2, where this port is connected. So it should probably be "l4_iclk_div". > + .user = OCP_USER_MPU | OCP_USER_SDMA, > +}; There's another struct omap_hwmod_ocp_if record missing: the high-speed bus-master port that the McASP3 uses to DMA audio data. This port should most likely be clocked with "l3_iclk_div" per Table 24-408 "Clocks and Resets". This port is also where the registers described in Table 24-555 "MCASP_DAT Register Summary 3" L3_MAIN column are exposed. You've got that address map range blocked out in your DT data reg property, and associated with this device, right? 0x46000000? > + > static struct omap_hwmod_addr_space dra7xx_elm_addrs[] = { > { > .pa_start = 0x48078000, > @@ -3338,6 +3378,7 @@ static struct omap_hwmod_ocp_if *dra7xx_hwmod_ocp_ifs[] __initdata = { > &dra7xx_l4_wkup__dcan1, > &dra7xx_l4_per2__dcan2, > &dra7xx_l4_per2__cpgmac0, > + &dra7xx_l4_per2__mcasp3, > &dra7xx_gmac__mdio, > &dra7xx_l4_cfg__dma_system, > &dra7xx_l3_main_1__dss, > -- > 2.5.0 > - Paul
Paul, On 09/27/2015 10:02 AM, Paul Walmsley wrote: >> /* >> + * 'mcasp' class >> + * >> + */ >> +static struct omap_hwmod_class_sysconfig dra7xx_mcasp_sysc = { >> + .sysc_offs = 0x0004, >> + .sysc_flags = SYSC_HAS_SIDLEMODE, >> + .idlemodes = (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART), >> + .sysc_fields = &omap_hwmod_sysc_type3, >> +}; >> + >> +static struct omap_hwmod_class dra7xx_mcasp_hwmod_class = { >> + .name = "mcasp", >> + .sysc = &dra7xx_mcasp_sysc, >> +}; >> + >> +/* mcasp3 */ >> +static struct omap_hwmod dra7xx_mcasp3_hwmod = { >> + .name = "mcasp3", >> + .class = &dra7xx_mcasp_hwmod_class, >> + .clkdm_name = "l4per2_clkdm", >> + .main_clk = "mcasp3_ahclkx_mux", > > I'd expect this clock to be something derived from mcasp3_aux_gfclk, > according to Table 24-408 "Clocks and Resets" of SPRUHZ6. Could you > please doublecheck this? I can not explain this. If I change the main_clk to "mcasp3_aux_gfclk_mux" then I can not access to McASP3 register at all. I don't see anything popping out in the clock data, nor in other places. >> + .flags = HWMOD_SWSUP_SIDLE, Not sure why this has been added, I can not find any pointers regarding to this and everything is working w/o this flag. Will remove it in v2. > Is this needed? If it is, please add a brief comment describing the issue > or bug that it's working around. > >> + .prcm = { >> + .omap4 = { >> + .clkctrl_offs = DRA7XX_CM_L4PER2_MCASP3_CLKCTRL_OFFSET, >> + .context_offs = DRA7XX_RM_L4PER2_MCASP3_CONTEXT_OFFSET, >> + .modulemode = MODULEMODE_SWCTRL, >> + }, >> + }, >> +}; >> + >> +/* >> * 'mmc' class >> * >> */ >> @@ -2566,6 +2598,14 @@ static struct omap_hwmod_ocp_if dra7xx_l3_main_1__hdmi = { >> .user = OCP_USER_MPU | OCP_USER_SDMA, >> }; >> >> +/* l4_per2 -> mcasp3 */ >> +static struct omap_hwmod_ocp_if dra7xx_l4_per2__mcasp3 = { >> + .master = &dra7xx_l4_per2_hwmod, >> + .slave = &dra7xx_mcasp3_hwmod, > > So this is the low-speed control/register access port, where the MPU > writes to the McASP3 config registers... > >> + .clk = "l3_iclk_div", > > ... and thus this interface clock doesn't look right for this port, since > it's most likely generated from the L4PER2, where this port is connected. > So it should probably be "l4_iclk_div". There is no "l4_iclk_div" for dra7xx. Looking around the file all other script generated data uses "l3_iclk_div" for IPs under dra7xx_l4_per2_hwmod. Tero: do you know the reason for this? > >> + .user = OCP_USER_MPU | OCP_USER_SDMA, >> +}; > > There's another struct omap_hwmod_ocp_if record missing: the high-speed > bus-master port that the McASP3 uses to DMA audio data. This port should > most likely be clocked with "l3_iclk_div" per Table 24-408 "Clocks and > Resets". This port is also where the registers described in Table 24-555 > "MCASP_DAT Register Summary 3" L3_MAIN column are exposed. You've got > that address map range blocked out in your DT data reg property, and > associated with this device, right? 0x46000000? Yes, the McASP3-dat port is not used ATM. This is over the L3 interconnect and due to a feature we can not use it with sDMA (constant addressing is not supported through L3 interconnect for DMAs). We could use eDMA, but there are complications regarding to that. At the moment we are using the sDMA through the L4 interconnect address space. > >> + >> static struct omap_hwmod_addr_space dra7xx_elm_addrs[] = { >> { >> .pa_start = 0x48078000, >> @@ -3338,6 +3378,7 @@ static struct omap_hwmod_ocp_if *dra7xx_hwmod_ocp_ifs[] __initdata = { >> &dra7xx_l4_wkup__dcan1, >> &dra7xx_l4_per2__dcan2, >> &dra7xx_l4_per2__cpgmac0, >> + &dra7xx_l4_per2__mcasp3, >> &dra7xx_gmac__mdio, >> &dra7xx_l4_cfg__dma_system, >> &dra7xx_l3_main_1__dss, >> -- >> 2.5.0 >> > > > - Paul >
On 09/30/2015 01:06 PM, Peter Ujfalusi wrote: > Paul, > > On 09/27/2015 10:02 AM, Paul Walmsley wrote: >>> /* >>> + * 'mcasp' class >>> + * >>> + */ >>> +static struct omap_hwmod_class_sysconfig dra7xx_mcasp_sysc = { >>> + .sysc_offs = 0x0004, >>> + .sysc_flags = SYSC_HAS_SIDLEMODE, >>> + .idlemodes = (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART), >>> + .sysc_fields = &omap_hwmod_sysc_type3, >>> +}; >>> + >>> +static struct omap_hwmod_class dra7xx_mcasp_hwmod_class = { >>> + .name = "mcasp", >>> + .sysc = &dra7xx_mcasp_sysc, >>> +}; >>> + >>> +/* mcasp3 */ >>> +static struct omap_hwmod dra7xx_mcasp3_hwmod = { >>> + .name = "mcasp3", >>> + .class = &dra7xx_mcasp_hwmod_class, >>> + .clkdm_name = "l4per2_clkdm", >>> + .main_clk = "mcasp3_ahclkx_mux", >> >> I'd expect this clock to be something derived from mcasp3_aux_gfclk, >> according to Table 24-408 "Clocks and Resets" of SPRUHZ6. Could you >> please doublecheck this? > > I can not explain this. If I change the main_clk to "mcasp3_aux_gfclk_mux" > then I can not access to McASP3 register at all. > I don't see anything popping out in the clock data, nor in other places. > >>> + .flags = HWMOD_SWSUP_SIDLE, > > Not sure why this has been added, I can not find any pointers regarding to > this and everything is working w/o this flag. Will remove it in v2. > > >> Is this needed? If it is, please add a brief comment describing the issue >> or bug that it's working around. >> >>> + .prcm = { >>> + .omap4 = { >>> + .clkctrl_offs = DRA7XX_CM_L4PER2_MCASP3_CLKCTRL_OFFSET, >>> + .context_offs = DRA7XX_RM_L4PER2_MCASP3_CONTEXT_OFFSET, >>> + .modulemode = MODULEMODE_SWCTRL, >>> + }, >>> + }, >>> +}; >>> + >>> +/* >>> * 'mmc' class >>> * >>> */ >>> @@ -2566,6 +2598,14 @@ static struct omap_hwmod_ocp_if dra7xx_l3_main_1__hdmi = { >>> .user = OCP_USER_MPU | OCP_USER_SDMA, >>> }; >>> >>> +/* l4_per2 -> mcasp3 */ >>> +static struct omap_hwmod_ocp_if dra7xx_l4_per2__mcasp3 = { >>> + .master = &dra7xx_l4_per2_hwmod, >>> + .slave = &dra7xx_mcasp3_hwmod, >> >> So this is the low-speed control/register access port, where the MPU >> writes to the McASP3 config registers... >> >>> + .clk = "l3_iclk_div", >> >> ... and thus this interface clock doesn't look right for this port, since >> it's most likely generated from the L4PER2, where this port is connected. >> So it should probably be "l4_iclk_div". > > There is no "l4_iclk_div" for dra7xx. Looking around the file all other script > generated data uses "l3_iclk_div" for IPs under dra7xx_l4_per2_hwmod. > > Tero: do you know the reason for this? This comes from the autogen generated data. Looking at the hwdb data for dra7, it seems l3 clock is defined as the OCP input clock for most of the modules. Looking at TRM, we also have L3 ICK defined as the interface clock for GPIO modules for example, and also mcasp modules. I think this is just a documentation issue and we are missing a divide by 2 from all interface clocks, the interface clocks are coming from l4 interconnects and the interconnect chapter still clearly states that the l4 clock is l3 clock / 2. -Tero > >> >>> + .user = OCP_USER_MPU | OCP_USER_SDMA, >>> +}; >> >> There's another struct omap_hwmod_ocp_if record missing: the high-speed >> bus-master port that the McASP3 uses to DMA audio data. This port should >> most likely be clocked with "l3_iclk_div" per Table 24-408 "Clocks and >> Resets". This port is also where the registers described in Table 24-555 >> "MCASP_DAT Register Summary 3" L3_MAIN column are exposed. You've got >> that address map range blocked out in your DT data reg property, and >> associated with this device, right? 0x46000000? > > Yes, the McASP3-dat port is not used ATM. This is over the L3 interconnect and > due to a feature we can not use it with sDMA (constant addressing is not > supported through L3 interconnect for DMAs). > We could use eDMA, but there are complications regarding to that. > At the moment we are using the sDMA through the L4 interconnect address space. > >> >>> + >>> static struct omap_hwmod_addr_space dra7xx_elm_addrs[] = { >>> { >>> .pa_start = 0x48078000, >>> @@ -3338,6 +3378,7 @@ static struct omap_hwmod_ocp_if *dra7xx_hwmod_ocp_ifs[] __initdata = { >>> &dra7xx_l4_wkup__dcan1, >>> &dra7xx_l4_per2__dcan2, >>> &dra7xx_l4_per2__cpgmac0, >>> + &dra7xx_l4_per2__mcasp3, >>> &dra7xx_gmac__mdio, >>> &dra7xx_l4_cfg__dma_system, >>> &dra7xx_l3_main_1__dss, >>> -- >>> 2.5.0 >>> >> >> >> - Paul >> > >
On 09/30/2015 04:00 PM, Tero Kristo wrote: >>>> +/* l4_per2 -> mcasp3 */ >>>> +static struct omap_hwmod_ocp_if dra7xx_l4_per2__mcasp3 = { >>>> + .master = &dra7xx_l4_per2_hwmod, >>>> + .slave = &dra7xx_mcasp3_hwmod, >>> >>> So this is the low-speed control/register access port, where the MPU >>> writes to the McASP3 config registers... >>> >>>> + .clk = "l3_iclk_div", >>> >>> ... and thus this interface clock doesn't look right for this port, since >>> it's most likely generated from the L4PER2, where this port is connected. >>> So it should probably be "l4_iclk_div". >> >> There is no "l4_iclk_div" for dra7xx. Looking around the file all other script >> generated data uses "l3_iclk_div" for IPs under dra7xx_l4_per2_hwmod. >> >> Tero: do you know the reason for this? > > This comes from the autogen generated data. Looking at the hwdb data for dra7, > it seems l3 clock is defined as the OCP input clock for most of the modules. > > Looking at TRM, we also have L3 ICK defined as the interface clock for GPIO > modules for example, and also mcasp modules. > > I think this is just a documentation issue and we are missing a divide by 2 > from all interface clocks, the interface clocks are coming from l4 > interconnects and the interconnect chapter still clearly states that the l4 > clock is l3 clock / 2. It seams that we have the clock node for the l4_iclk_div, but it is called as l4_root_clk_div. I will use this in the mcasp3 hwmod patch.
Hello Péter, On Wed, 30 Sep 2015, Peter Ujfalusi wrote: > On 09/27/2015 10:02 AM, Paul Walmsley wrote: > >> /* > >> + * 'mcasp' class > >> + * > >> + */ > >> +static struct omap_hwmod_class_sysconfig dra7xx_mcasp_sysc = { > >> + .sysc_offs = 0x0004, > >> + .sysc_flags = SYSC_HAS_SIDLEMODE, > >> + .idlemodes = (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART), > >> + .sysc_fields = &omap_hwmod_sysc_type3, > >> +}; > >> + > >> +static struct omap_hwmod_class dra7xx_mcasp_hwmod_class = { > >> + .name = "mcasp", > >> + .sysc = &dra7xx_mcasp_sysc, > >> +}; > >> + > >> +/* mcasp3 */ > >> +static struct omap_hwmod dra7xx_mcasp3_hwmod = { > >> + .name = "mcasp3", > >> + .class = &dra7xx_mcasp_hwmod_class, > >> + .clkdm_name = "l4per2_clkdm", > >> + .main_clk = "mcasp3_ahclkx_mux", > > > > I'd expect this clock to be something derived from mcasp3_aux_gfclk, > > according to Table 24-408 "Clocks and Resets" of SPRUHZ6. Could you > > please doublecheck this? > > I can not explain this. If I change the main_clk to "mcasp3_aux_gfclk_mux" > then I can not access to McASP3 register at all. > I don't see anything popping out in the clock data, nor in other places. OK thank you for testing that. Maybe just add a brief comment along those lines in the hwmod data, above the structure record declaration? > >> + .flags = HWMOD_SWSUP_SIDLE, > > Not sure why this has been added, I can not find any pointers regarding to > this and everything is working w/o this flag. Will remove it in v2. OK thanks. > >> @@ -2566,6 +2598,14 @@ static struct omap_hwmod_ocp_if dra7xx_l3_main_1__hdmi = { > >> .user = OCP_USER_MPU | OCP_USER_SDMA, > >> }; > >> > >> +/* l4_per2 -> mcasp3 */ > >> +static struct omap_hwmod_ocp_if dra7xx_l4_per2__mcasp3 = { > >> + .master = &dra7xx_l4_per2_hwmod, > >> + .slave = &dra7xx_mcasp3_hwmod, > > > > So this is the low-speed control/register access port, where the MPU > > writes to the McASP3 config registers... > > > >> + .clk = "l3_iclk_div", > > > > ... and thus this interface clock doesn't look right for this port, since > > it's most likely generated from the L4PER2, where this port is connected. > > So it should probably be "l4_iclk_div". > > There is no "l4_iclk_div" for dra7xx. Looking around the file all other script > generated data uses "l3_iclk_div" for IPs under dra7xx_l4_per2_hwmod. > > Tero: do you know the reason for this? Sounds like from your followup E-mail that the clock name to use in the kernel is "l4_root_clk_div", which sounds fine to me. (Haven't looked closely at the clock data, though.) > >> + .user = OCP_USER_MPU | OCP_USER_SDMA, > >> +}; > > > > There's another struct omap_hwmod_ocp_if record missing: the high-speed > > bus-master port that the McASP3 uses to DMA audio data. This port should > > most likely be clocked with "l3_iclk_div" per Table 24-408 "Clocks and > > Resets". This port is also where the registers described in Table 24-555 > > "MCASP_DAT Register Summary 3" L3_MAIN column are exposed. You've got > > that address map range blocked out in your DT data reg property, and > > associated with this device, right? 0x46000000? > > Yes, the McASP3-dat port is not used ATM. This is over the L3 interconnect and > due to a feature we can not use it with sDMA (constant addressing is not > supported through L3 interconnect for DMAs). > We could use eDMA, but there are complications regarding to that. > At the moment we are using the sDMA through the L4 interconnect address space. OK thanks for the explanation. The hwmod code prevents links from being registered if no initiator 'users' are listed, so sounds like we should leave it out for now. Could you add a brief comment, similar to your paragraph above, in the data, in case others wonder about the L3 link? After those changes are made, feel free to add my ack. Köszönöm, - Paul
Paul, On 10/02/2015 12:07 PM, Paul Walmsley wrote: > Hello Péter, > > On Wed, 30 Sep 2015, Peter Ujfalusi wrote: > >> On 09/27/2015 10:02 AM, Paul Walmsley wrote: >>>> /* >>>> + * 'mcasp' class >>>> + * >>>> + */ >>>> +static struct omap_hwmod_class_sysconfig dra7xx_mcasp_sysc = { >>>> + .sysc_offs = 0x0004, >>>> + .sysc_flags = SYSC_HAS_SIDLEMODE, >>>> + .idlemodes = (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART), >>>> + .sysc_fields = &omap_hwmod_sysc_type3, >>>> +}; >>>> + >>>> +static struct omap_hwmod_class dra7xx_mcasp_hwmod_class = { >>>> + .name = "mcasp", >>>> + .sysc = &dra7xx_mcasp_sysc, >>>> +}; >>>> + >>>> +/* mcasp3 */ >>>> +static struct omap_hwmod dra7xx_mcasp3_hwmod = { >>>> + .name = "mcasp3", >>>> + .class = &dra7xx_mcasp_hwmod_class, >>>> + .clkdm_name = "l4per2_clkdm", >>>> + .main_clk = "mcasp3_ahclkx_mux", >>> >>> I'd expect this clock to be something derived from mcasp3_aux_gfclk, >>> according to Table 24-408 "Clocks and Resets" of SPRUHZ6. Could you >>> please doublecheck this? >> >> I can not explain this. If I change the main_clk to "mcasp3_aux_gfclk_mux" >> then I can not access to McASP3 register at all. >> I don't see anything popping out in the clock data, nor in other places. > > OK thank you for testing that. Maybe just add a brief comment along those > lines in the hwmod data, above the structure record declaration? Now I more or less figured out the root of the issue. According to the documentations the McASP in dra7xx family has following clocks: ICLK - interface clock GFCLK - functional clock AHCLKX - Transmit high-frequency master clock AHCLKR - Receive high-frequency master clock (on selected instances) In order to access registers all of these clock lines must have valid clock signal. No other SoC with McASP has this setup. As for why things seams to work when mcasp3_ahclkx_mux is set as main_clk and mcasp3_aux_gfclk_mux is not handled at all? The mcasp3_aux_gfclk_mux by default is from PER_ABE_X1GFCLK we never change this. On dra7/72 evm the AHCLKX is reparented to ATL2 clock. When with pm_runtime we enable the device, the mcasp3_ahclkx_mux will be enabled by the SW (and ATL as well) _and_ the mcasp3_aux_gfclk_mux path will be enabled by HW w/o SW. The reason why I have had crash when I switched the main_clk to mcasp3_aux_gfclk_mux is that even the path for the mcasp3_ahclkx_mux is enabled by the HW, the ATL itself was not enabled, so it was not generating the needed clocks. Same goes backwards for the gfclk: if I reparent it to let's say HDMI clock - which is not present, and handle the ahclkx clock I have similar crash. All in all: the McASP3 needs ICLK and both FCLK and AHCLKX to be running in order to be able to access registers. This current setup works fine, the only issue I see is that the refcounts for the mcasp3_aux_gfclk_mux path is not reflecting the reality. With Tero we looked at different angles of this and how to solve it w/o considering it as a hack. Either we go with the hwmod data with main_clk set to mcasp3_ahclkx_mux and document it in a comment, or: Add new flag HWMOD_OPT_CLKS_NEEDED to hwmods to use to tell that the optional clocks are not really optional as they are needed to be enabled in order to access to the IP. In omap_hwmod.c's _enable_clocks() and _disable_clocks() we call _enable_optional_clocks()/_disable_optional_clocks() if the flag is set for the hwmod and add the ahclkx_mux as optional clock for McASP3. >>>> + .user = OCP_USER_MPU | OCP_USER_SDMA, >>>> +}; >>> >>> There's another struct omap_hwmod_ocp_if record missing: the high-speed >>> bus-master port that the McASP3 uses to DMA audio data. This port should >>> most likely be clocked with "l3_iclk_div" per Table 24-408 "Clocks and >>> Resets". This port is also where the registers described in Table 24-555 >>> "MCASP_DAT Register Summary 3" L3_MAIN column are exposed. You've got >>> that address map range blocked out in your DT data reg property, and >>> associated with this device, right? 0x46000000? >> >> Yes, the McASP3-dat port is not used ATM. This is over the L3 interconnect and >> due to a feature we can not use it with sDMA (constant addressing is not >> supported through L3 interconnect for DMAs). >> We could use eDMA, but there are complications regarding to that. >> At the moment we are using the sDMA through the L4 interconnect address space. > > OK thanks for the explanation. The hwmod code prevents links from being > registered if no initiator 'users' are listed, so sounds like we should > leave it out for now. Could you add a brief comment, similar to your > paragraph above, in the data, in case others wonder about the L3 link? Sure, I'll do that. > > After those changes are made, feel free to add my ack. I'll wait for your comment regarding to the multiple main_clk need for the McASP3 before I'll send the next version. > Köszönöm, :D > > - Paul >
On 10/02/2015 02:22 PM, Peter Ujfalusi wrote: > Paul, > > On 10/02/2015 12:07 PM, Paul Walmsley wrote: >> Hello Péter, >> >> On Wed, 30 Sep 2015, Peter Ujfalusi wrote: >> >>> On 09/27/2015 10:02 AM, Paul Walmsley wrote: >>>>> /* >>>>> + * 'mcasp' class >>>>> + * >>>>> + */ >>>>> +static struct omap_hwmod_class_sysconfig dra7xx_mcasp_sysc = { >>>>> + .sysc_offs = 0x0004, >>>>> + .sysc_flags = SYSC_HAS_SIDLEMODE, >>>>> + .idlemodes = (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART), >>>>> + .sysc_fields = &omap_hwmod_sysc_type3, >>>>> +}; >>>>> + >>>>> +static struct omap_hwmod_class dra7xx_mcasp_hwmod_class = { >>>>> + .name = "mcasp", >>>>> + .sysc = &dra7xx_mcasp_sysc, >>>>> +}; >>>>> + >>>>> +/* mcasp3 */ >>>>> +static struct omap_hwmod dra7xx_mcasp3_hwmod = { >>>>> + .name = "mcasp3", >>>>> + .class = &dra7xx_mcasp_hwmod_class, >>>>> + .clkdm_name = "l4per2_clkdm", >>>>> + .main_clk = "mcasp3_ahclkx_mux", >>>> >>>> I'd expect this clock to be something derived from mcasp3_aux_gfclk, >>>> according to Table 24-408 "Clocks and Resets" of SPRUHZ6. Could you >>>> please doublecheck this? >>> >>> I can not explain this. If I change the main_clk to "mcasp3_aux_gfclk_mux" >>> then I can not access to McASP3 register at all. >>> I don't see anything popping out in the clock data, nor in other places. >> >> OK thank you for testing that. Maybe just add a brief comment along those >> lines in the hwmod data, above the structure record declaration? > > Now I more or less figured out the root of the issue. According to the > documentations the McASP in dra7xx family has following clocks: > ICLK - interface clock > GFCLK - functional clock > AHCLKX - Transmit high-frequency master clock > AHCLKR - Receive high-frequency master clock (on selected instances) > > In order to access registers all of these clock lines must have valid clock > signal. No other SoC with McASP has this setup. > As for why things seams to work when mcasp3_ahclkx_mux is set as main_clk and > mcasp3_aux_gfclk_mux is not handled at all? > The mcasp3_aux_gfclk_mux by default is from PER_ABE_X1GFCLK we never change > this. On dra7/72 evm the AHCLKX is reparented to ATL2 clock. > When with pm_runtime we enable the device, the mcasp3_ahclkx_mux will be > enabled by the SW (and ATL as well) _and_ the mcasp3_aux_gfclk_mux path will > be enabled by HW w/o SW. > The reason why I have had crash when I switched the main_clk to > mcasp3_aux_gfclk_mux is that even the path for the mcasp3_ahclkx_mux is > enabled by the HW, the ATL itself was not enabled, so it was not generating > the needed clocks. Same goes backwards for the gfclk: if I reparent it to > let's say HDMI clock - which is not present, and handle the ahclkx clock I > have similar crash. > All in all: the McASP3 needs ICLK and both FCLK and AHCLKX to be running in > order to be able to access registers. > > This current setup works fine, the only issue I see is that the refcounts for > the mcasp3_aux_gfclk_mux path is not reflecting the reality. > > With Tero we looked at different angles of this and how to solve it w/o > considering it as a hack. > Either we go with the hwmod data with main_clk set to mcasp3_ahclkx_mux and > document it in a comment, or: > Add new flag HWMOD_OPT_CLKS_NEEDED to hwmods to use to tell that the optional > clocks are not really optional as they are needed to be enabled in order to > access to the IP. In omap_hwmod.c's _enable_clocks() and _disable_clocks() we > call _enable_optional_clocks()/_disable_optional_clocks() if the flag is set > for the hwmod and add the ahclkx_mux as optional clock for McASP3. This might be awkward to say that the optional clocks are not optional, probably would be better to add: struct omap_hwmod { ... struct omap_hwmod_opt_clk *needed_clks; ... u8 needed_clks_cnt; ... }; and use the needed_clks in _init_main_clk()/_enable_clocks()/_disable_clocks() ?
On 10/02/2015 02:55 PM, Peter Ujfalusi wrote: > On 10/02/2015 02:22 PM, Peter Ujfalusi wrote: >> Paul, >> >> On 10/02/2015 12:07 PM, Paul Walmsley wrote: >>> Hello Péter, >>> >>> On Wed, 30 Sep 2015, Peter Ujfalusi wrote: >>> >>>> On 09/27/2015 10:02 AM, Paul Walmsley wrote: >>>>>> /* >>>>>> + * 'mcasp' class >>>>>> + * >>>>>> + */ >>>>>> +static struct omap_hwmod_class_sysconfig dra7xx_mcasp_sysc = { >>>>>> + .sysc_offs = 0x0004, >>>>>> + .sysc_flags = SYSC_HAS_SIDLEMODE, >>>>>> + .idlemodes = (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART), >>>>>> + .sysc_fields = &omap_hwmod_sysc_type3, >>>>>> +}; >>>>>> + >>>>>> +static struct omap_hwmod_class dra7xx_mcasp_hwmod_class = { >>>>>> + .name = "mcasp", >>>>>> + .sysc = &dra7xx_mcasp_sysc, >>>>>> +}; >>>>>> + >>>>>> +/* mcasp3 */ >>>>>> +static struct omap_hwmod dra7xx_mcasp3_hwmod = { >>>>>> + .name = "mcasp3", >>>>>> + .class = &dra7xx_mcasp_hwmod_class, >>>>>> + .clkdm_name = "l4per2_clkdm", >>>>>> + .main_clk = "mcasp3_ahclkx_mux", >>>>> >>>>> I'd expect this clock to be something derived from mcasp3_aux_gfclk, >>>>> according to Table 24-408 "Clocks and Resets" of SPRUHZ6. Could you >>>>> please doublecheck this? >>>> >>>> I can not explain this. If I change the main_clk to "mcasp3_aux_gfclk_mux" >>>> then I can not access to McASP3 register at all. >>>> I don't see anything popping out in the clock data, nor in other places. >>> >>> OK thank you for testing that. Maybe just add a brief comment along those >>> lines in the hwmod data, above the structure record declaration? >> >> Now I more or less figured out the root of the issue. According to the >> documentations the McASP in dra7xx family has following clocks: >> ICLK - interface clock >> GFCLK - functional clock >> AHCLKX - Transmit high-frequency master clock >> AHCLKR - Receive high-frequency master clock (on selected instances) >> >> In order to access registers all of these clock lines must have valid clock >> signal. No other SoC with McASP has this setup. >> As for why things seams to work when mcasp3_ahclkx_mux is set as main_clk and >> mcasp3_aux_gfclk_mux is not handled at all? >> The mcasp3_aux_gfclk_mux by default is from PER_ABE_X1GFCLK we never change >> this. On dra7/72 evm the AHCLKX is reparented to ATL2 clock. >> When with pm_runtime we enable the device, the mcasp3_ahclkx_mux will be >> enabled by the SW (and ATL as well) _and_ the mcasp3_aux_gfclk_mux path will >> be enabled by HW w/o SW. >> The reason why I have had crash when I switched the main_clk to >> mcasp3_aux_gfclk_mux is that even the path for the mcasp3_ahclkx_mux is >> enabled by the HW, the ATL itself was not enabled, so it was not generating >> the needed clocks. Same goes backwards for the gfclk: if I reparent it to >> let's say HDMI clock - which is not present, and handle the ahclkx clock I >> have similar crash. >> All in all: the McASP3 needs ICLK and both FCLK and AHCLKX to be running in >> order to be able to access registers. >> >> This current setup works fine, the only issue I see is that the refcounts for >> the mcasp3_aux_gfclk_mux path is not reflecting the reality. >> >> With Tero we looked at different angles of this and how to solve it w/o >> considering it as a hack. >> Either we go with the hwmod data with main_clk set to mcasp3_ahclkx_mux and >> document it in a comment, or: >> Add new flag HWMOD_OPT_CLKS_NEEDED to hwmods to use to tell that the optional >> clocks are not really optional as they are needed to be enabled in order to >> access to the IP. In omap_hwmod.c's _enable_clocks() and _disable_clocks() we >> call _enable_optional_clocks()/_disable_optional_clocks() if the flag is set >> for the hwmod and add the ahclkx_mux as optional clock for McASP3. > > This might be awkward to say that the optional clocks are not optional, > probably would be better to add: > struct omap_hwmod { > ... > struct omap_hwmod_opt_clk *needed_clks; > ... > u8 needed_clks_cnt; > ... > }; > > and use the needed_clks in _init_main_clk()/_enable_clocks()/_disable_clocks() ? Arrgh, but how to deal with this via DT bindings? It will be better to mark the optional clocks as needed.
diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c index 562247bced49..c38b7fa30c27 100644 --- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c +++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c @@ -1298,6 +1298,38 @@ static struct omap_hwmod dra7xx_mcspi4_hwmod = { }; /* + * 'mcasp' class + * + */ +static struct omap_hwmod_class_sysconfig dra7xx_mcasp_sysc = { + .sysc_offs = 0x0004, + .sysc_flags = SYSC_HAS_SIDLEMODE, + .idlemodes = (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART), + .sysc_fields = &omap_hwmod_sysc_type3, +}; + +static struct omap_hwmod_class dra7xx_mcasp_hwmod_class = { + .name = "mcasp", + .sysc = &dra7xx_mcasp_sysc, +}; + +/* mcasp3 */ +static struct omap_hwmod dra7xx_mcasp3_hwmod = { + .name = "mcasp3", + .class = &dra7xx_mcasp_hwmod_class, + .clkdm_name = "l4per2_clkdm", + .main_clk = "mcasp3_ahclkx_mux", + .flags = HWMOD_SWSUP_SIDLE, + .prcm = { + .omap4 = { + .clkctrl_offs = DRA7XX_CM_L4PER2_MCASP3_CLKCTRL_OFFSET, + .context_offs = DRA7XX_RM_L4PER2_MCASP3_CONTEXT_OFFSET, + .modulemode = MODULEMODE_SWCTRL, + }, + }, +}; + +/* * 'mmc' class * */ @@ -2566,6 +2598,14 @@ static struct omap_hwmod_ocp_if dra7xx_l3_main_1__hdmi = { .user = OCP_USER_MPU | OCP_USER_SDMA, }; +/* l4_per2 -> mcasp3 */ +static struct omap_hwmod_ocp_if dra7xx_l4_per2__mcasp3 = { + .master = &dra7xx_l4_per2_hwmod, + .slave = &dra7xx_mcasp3_hwmod, + .clk = "l3_iclk_div", + .user = OCP_USER_MPU | OCP_USER_SDMA, +}; + static struct omap_hwmod_addr_space dra7xx_elm_addrs[] = { { .pa_start = 0x48078000, @@ -3338,6 +3378,7 @@ static struct omap_hwmod_ocp_if *dra7xx_hwmod_ocp_ifs[] __initdata = { &dra7xx_l4_wkup__dcan1, &dra7xx_l4_per2__dcan2, &dra7xx_l4_per2__cpgmac0, + &dra7xx_l4_per2__mcasp3, &dra7xx_gmac__mdio, &dra7xx_l4_cfg__dma_system, &dra7xx_l3_main_1__dss,
McASP3 is used by default on DRA7x based boards for audio. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- Hi Paul, this patch is part of my earlier series and as Tony suggested I'll resend the hwmod patch for you to review since I missed you from the TO in the series. The original series: https://www.mail-archive.com/linux-omap@vger.kernel.org/msg119319.html Regards, Peter arch/arm/mach-omap2/omap_hwmod_7xx_data.c | 41 +++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)