Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
Hi Vinod, On 6/7/2018 2:13 PM, Vinod wrote: > On 06-06-18, 21:24, Bjorn Andersson wrote: >> On Wed 06 Jun 21:11 PDT 2018, Vinod wrote: >> >>> So, wouldn't Kconfig syntax something like where we say: >>> M if RPMSG_QCOM_GLINK_SMEM=m >>> bool if RPMSG_QCOM_GLINK_SMEM=y >>> >> >> If we ignore SMD for a while we have the following combinations: >> >> glink/wcss >> y y - valid >> y m - valid >> y n - valid >> m y - link failure (invalid) >> m m - valid >> m n - valid >> n y - valid (platform uses wcss, but not glink) >> n m - valid (-"-) >> n n - valid >> >> So to distill this we have the two valid cases: >> module/no if RPMSG_QCOM_GLINK_SMEM=m >> yes/module/no if RPMSG_QCOM_GLINK_SMEM=y >> >> and the way you express that in Kconfig is the somewhat awkward >> >> depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > > Understood now :) Yes it is awkward.. > > Btw we seem to have issue with link fail here when glink is m and wcss > is y. Why don't we see link fail for glink being n? Yes I understand that > platform uses wcss but am curious how that works out :) For glink being n, the stub functions gets linked, and not for glink=m. Regards, Sricharan -- "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
Hi Vinod, On 6/7/2018 2:13 PM, Vinod wrote: > On 06-06-18, 21:24, Bjorn Andersson wrote: >> On Wed 06 Jun 21:11 PDT 2018, Vinod wrote: >> >>> So, wouldn't Kconfig syntax something like where we say: >>> M if RPMSG_QCOM_GLINK_SMEM=m >>> bool if RPMSG_QCOM_GLINK_SMEM=y >>> >> >> If we ignore SMD for a while we have the following combinations: >> >> glink/wcss >> y y - valid >> y m - valid >> y n - valid >> m y - link failure (invalid) >> m m - valid >> m n - valid >> n y - valid (platform uses wcss, but not glink) >> n m - valid (-"-) >> n n - valid >> >> So to distill this we have the two valid cases: >> module/no if RPMSG_QCOM_GLINK_SMEM=m >> yes/module/no if RPMSG_QCOM_GLINK_SMEM=y >> >> and the way you express that in Kconfig is the somewhat awkward >> >> depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > > Understood now :) Yes it is awkward.. > > Btw we seem to have issue with link fail here when glink is m and wcss > is y. Why don't we see link fail for glink being n? Yes I understand that > platform uses wcss but am curious how that works out :) For glink being n, the stub functions gets linked, and not for glink=m. Regards, Sricharan -- "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
On 06-06-18, 21:24, Bjorn Andersson wrote: > On Wed 06 Jun 21:11 PDT 2018, Vinod wrote: > > > So, wouldn't Kconfig syntax something like where we say: > > M if RPMSG_QCOM_GLINK_SMEM=m > > bool if RPMSG_QCOM_GLINK_SMEM=y > > > > If we ignore SMD for a while we have the following combinations: > > glink/wcss > y y - valid > y m - valid > y n - valid > m y - link failure (invalid) > m m - valid > m n - valid > n y - valid (platform uses wcss, but not glink) > n m - valid (-"-) > n n - valid > > So to distill this we have the two valid cases: > module/no if RPMSG_QCOM_GLINK_SMEM=m > yes/module/no if RPMSG_QCOM_GLINK_SMEM=y > > and the way you express that in Kconfig is the somewhat awkward > > depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n Understood now :) Yes it is awkward.. Btw we seem to have issue with link fail here when glink is m and wcss is y. Why don't we see link fail for glink being n? Yes I understand that platform uses wcss but am curious how that works out :) Thanks -- ~Vinod
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
On 06-06-18, 21:24, Bjorn Andersson wrote: > On Wed 06 Jun 21:11 PDT 2018, Vinod wrote: > > > So, wouldn't Kconfig syntax something like where we say: > > M if RPMSG_QCOM_GLINK_SMEM=m > > bool if RPMSG_QCOM_GLINK_SMEM=y > > > > If we ignore SMD for a while we have the following combinations: > > glink/wcss > y y - valid > y m - valid > y n - valid > m y - link failure (invalid) > m m - valid > m n - valid > n y - valid (platform uses wcss, but not glink) > n m - valid (-"-) > n n - valid > > So to distill this we have the two valid cases: > module/no if RPMSG_QCOM_GLINK_SMEM=m > yes/module/no if RPMSG_QCOM_GLINK_SMEM=y > > and the way you express that in Kconfig is the somewhat awkward > > depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n Understood now :) Yes it is awkward.. Btw we seem to have issue with link fail here when glink is m and wcss is y. Why don't we see link fail for glink being n? Yes I understand that platform uses wcss but am curious how that works out :) Thanks -- ~Vinod
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
Hi Bjorn, On 6/7/2018 11:18 AM, Bjorn Andersson wrote: > On Wed 06 Jun 22:29 PDT 2018, Sricharan R wrote: > >> Hi Bjorn, >> >> On 6/7/2018 9:54 AM, Bjorn Andersson wrote: >>> On Wed 06 Jun 21:11 PDT 2018, Vinod wrote: >>> On 06-06-18, 09:17, Bjorn Andersson wrote: > On Tue 05 Jun 05:56 PDT 2018, Sricharan R wrote: > >> Hi Vinod, >> >> On 6/5/2018 11:49 AM, Vinod wrote: >>> On 05-06-18, 11:12, Sricharan R wrote: > [..] >>> If we ignore SMD for a while we have the following combinations: >>> >>> glink/wcss >>> y y - valid >>> y m - valid >>> y n - valid >>> m y - link failure (invalid) >>> m m - valid >>> m n - valid >>> n y - valid (platform uses wcss, but not glink) >>> n m - valid (-"-) >>> n n - valid >>> >>> So to distill this we have the two valid cases: >>> module/no if RPMSG_QCOM_GLINK_SMEM=m >>> yes/module/no if RPMSG_QCOM_GLINK_SMEM=y >>> >>> and the way you express that in Kconfig is the somewhat awkward >>> >>> depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n >>> >> >> ok, Having "depends on RPMSG_QCOM_GLINK_SMEM" takes care of the >> first 6 cases in the above list. >> >> But just was thinking that by allowing the last three combinations, >> there is a chance that some config that really needs GLINK_SMEM and WCSS, >> but selects only Q6V5_WCSS and misses to select GLINK_SMEM, >> would still built and make it non-functional, right ? >> > > It would allow you to compile a kernel with GLINk disabled that in > runtime loads a firmware that depends on GLINK being there. > > As it would be convenient to thereby state that "WCSS always depends on > GLINK" we can make the analog to 410 where "MSS always depends on SMD", > which isn't true when the same driver is reused on e.g. 845 - which > uses GLINK. > > > So my recommendation is that we stick with Kconfig options that > describes the build time dependencies of this particular driver, rather > than its runtime dependencies in a particular platform. > ok thanks. It sort of gave an impression that the last three combinations in the above list was only for "compile testing". Hence was thinking to have (COMPILE_TEST && RPMSG_QCOM_GLINK_SMEM=n). Anyways for WCSS, would drop the dependency on SMD and just have RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n. Regards, Sricharan -- "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
Hi Bjorn, On 6/7/2018 11:18 AM, Bjorn Andersson wrote: > On Wed 06 Jun 22:29 PDT 2018, Sricharan R wrote: > >> Hi Bjorn, >> >> On 6/7/2018 9:54 AM, Bjorn Andersson wrote: >>> On Wed 06 Jun 21:11 PDT 2018, Vinod wrote: >>> On 06-06-18, 09:17, Bjorn Andersson wrote: > On Tue 05 Jun 05:56 PDT 2018, Sricharan R wrote: > >> Hi Vinod, >> >> On 6/5/2018 11:49 AM, Vinod wrote: >>> On 05-06-18, 11:12, Sricharan R wrote: > [..] >>> If we ignore SMD for a while we have the following combinations: >>> >>> glink/wcss >>> y y - valid >>> y m - valid >>> y n - valid >>> m y - link failure (invalid) >>> m m - valid >>> m n - valid >>> n y - valid (platform uses wcss, but not glink) >>> n m - valid (-"-) >>> n n - valid >>> >>> So to distill this we have the two valid cases: >>> module/no if RPMSG_QCOM_GLINK_SMEM=m >>> yes/module/no if RPMSG_QCOM_GLINK_SMEM=y >>> >>> and the way you express that in Kconfig is the somewhat awkward >>> >>> depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n >>> >> >> ok, Having "depends on RPMSG_QCOM_GLINK_SMEM" takes care of the >> first 6 cases in the above list. >> >> But just was thinking that by allowing the last three combinations, >> there is a chance that some config that really needs GLINK_SMEM and WCSS, >> but selects only Q6V5_WCSS and misses to select GLINK_SMEM, >> would still built and make it non-functional, right ? >> > > It would allow you to compile a kernel with GLINk disabled that in > runtime loads a firmware that depends on GLINK being there. > > As it would be convenient to thereby state that "WCSS always depends on > GLINK" we can make the analog to 410 where "MSS always depends on SMD", > which isn't true when the same driver is reused on e.g. 845 - which > uses GLINK. > > > So my recommendation is that we stick with Kconfig options that > describes the build time dependencies of this particular driver, rather > than its runtime dependencies in a particular platform. > ok thanks. It sort of gave an impression that the last three combinations in the above list was only for "compile testing". Hence was thinking to have (COMPILE_TEST && RPMSG_QCOM_GLINK_SMEM=n). Anyways for WCSS, would drop the dependency on SMD and just have RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n. Regards, Sricharan -- "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
On Wed 06 Jun 22:29 PDT 2018, Sricharan R wrote: > Hi Bjorn, > > On 6/7/2018 9:54 AM, Bjorn Andersson wrote: > > On Wed 06 Jun 21:11 PDT 2018, Vinod wrote: > > > >> On 06-06-18, 09:17, Bjorn Andersson wrote: > >>> On Tue 05 Jun 05:56 PDT 2018, Sricharan R wrote: > >>> > Hi Vinod, > > On 6/5/2018 11:49 AM, Vinod wrote: > > On 05-06-18, 11:12, Sricharan R wrote: [..] > > If we ignore SMD for a while we have the following combinations: > > > > glink/wcss > > y y - valid > > y m - valid > > y n - valid > > m y - link failure (invalid) > > m m - valid > > m n - valid > > n y - valid (platform uses wcss, but not glink) > > n m - valid (-"-) > > n n - valid > > > > So to distill this we have the two valid cases: > > module/no if RPMSG_QCOM_GLINK_SMEM=m > > yes/module/no if RPMSG_QCOM_GLINK_SMEM=y > > > > and the way you express that in Kconfig is the somewhat awkward > > > > depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > > > > ok, Having "depends on RPMSG_QCOM_GLINK_SMEM" takes care of the > first 6 cases in the above list. > > But just was thinking that by allowing the last three combinations, > there is a chance that some config that really needs GLINK_SMEM and WCSS, > but selects only Q6V5_WCSS and misses to select GLINK_SMEM, > would still built and make it non-functional, right ? > It would allow you to compile a kernel with GLINk disabled that in runtime loads a firmware that depends on GLINK being there. As it would be convenient to thereby state that "WCSS always depends on GLINK" we can make the analog to 410 where "MSS always depends on SMD", which isn't true when the same driver is reused on e.g. 845 - which uses GLINK. So my recommendation is that we stick with Kconfig options that describes the build time dependencies of this particular driver, rather than its runtime dependencies in a particular platform. Regards, Bjorn
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
On Wed 06 Jun 22:29 PDT 2018, Sricharan R wrote: > Hi Bjorn, > > On 6/7/2018 9:54 AM, Bjorn Andersson wrote: > > On Wed 06 Jun 21:11 PDT 2018, Vinod wrote: > > > >> On 06-06-18, 09:17, Bjorn Andersson wrote: > >>> On Tue 05 Jun 05:56 PDT 2018, Sricharan R wrote: > >>> > Hi Vinod, > > On 6/5/2018 11:49 AM, Vinod wrote: > > On 05-06-18, 11:12, Sricharan R wrote: [..] > > If we ignore SMD for a while we have the following combinations: > > > > glink/wcss > > y y - valid > > y m - valid > > y n - valid > > m y - link failure (invalid) > > m m - valid > > m n - valid > > n y - valid (platform uses wcss, but not glink) > > n m - valid (-"-) > > n n - valid > > > > So to distill this we have the two valid cases: > > module/no if RPMSG_QCOM_GLINK_SMEM=m > > yes/module/no if RPMSG_QCOM_GLINK_SMEM=y > > > > and the way you express that in Kconfig is the somewhat awkward > > > > depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > > > > ok, Having "depends on RPMSG_QCOM_GLINK_SMEM" takes care of the > first 6 cases in the above list. > > But just was thinking that by allowing the last three combinations, > there is a chance that some config that really needs GLINK_SMEM and WCSS, > but selects only Q6V5_WCSS and misses to select GLINK_SMEM, > would still built and make it non-functional, right ? > It would allow you to compile a kernel with GLINk disabled that in runtime loads a firmware that depends on GLINK being there. As it would be convenient to thereby state that "WCSS always depends on GLINK" we can make the analog to 410 where "MSS always depends on SMD", which isn't true when the same driver is reused on e.g. 845 - which uses GLINK. So my recommendation is that we stick with Kconfig options that describes the build time dependencies of this particular driver, rather than its runtime dependencies in a particular platform. Regards, Bjorn
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
Hi Bjorn, On 6/7/2018 9:54 AM, Bjorn Andersson wrote: > On Wed 06 Jun 21:11 PDT 2018, Vinod wrote: > >> On 06-06-18, 09:17, Bjorn Andersson wrote: >>> On Tue 05 Jun 05:56 PDT 2018, Sricharan R wrote: >>> Hi Vinod, On 6/5/2018 11:49 AM, Vinod wrote: > On 05-06-18, 11:12, Sricharan R wrote: > >> +config QCOM_Q6V5_WCSS >> +tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader" >> +depends on OF && ARCH_QCOM >> +depends on QCOM_SMEM >> +depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n) >> +depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > > Is there a reason why it depends on RPMSG_QCOM_GLINK_SMEM=n? What would > happen if distro wants both this and RPMSG_QCOM_GLINK_SMEM > >>> >>> It says that QCOM_Q6V5_WCSS either must have a compatible state (i.e. >>> builtin vs builtin, module vs builtin, but not builtin vs module) or >>> that it's disabled, in which case we will hit the stub functions in >>> qcom_glink.h. >>> >>> I.e. this prevents QCOM_Q6V5_WCSS to be compiled builtin when >>> RPMSG_QCOM_GLINK_SMEM is module, as this would give us both stubs and >>> the module. >> >> IIUC, you want to have QCOM_Q6V5_WCSS and RPMSG_QCOM_GLINK_SMEM as >> modules or builtin >> > > RPMSG_QCOM_SMD, RPMSG_QCOM_GLINK_SMEM and QCOM_Q6V5_WCSS are all > tristate. > >> So, wouldn't Kconfig syntax something like where we say: >> M if RPMSG_QCOM_GLINK_SMEM=m >> bool if RPMSG_QCOM_GLINK_SMEM=y >> > > If we ignore SMD for a while we have the following combinations: > > glink/wcss > y y - valid > y m - valid > y n - valid > m y - link failure (invalid) > m m - valid > m n - valid > n y - valid (platform uses wcss, but not glink) > n m - valid (-"-) > n n - valid > > So to distill this we have the two valid cases: > module/no if RPMSG_QCOM_GLINK_SMEM=m > yes/module/no if RPMSG_QCOM_GLINK_SMEM=y > > and the way you express that in Kconfig is the somewhat awkward > > depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > ok, Having "depends on RPMSG_QCOM_GLINK_SMEM" takes care of the first 6 cases in the above list. But just was thinking that by allowing the last three combinations, there is a chance that some config that really needs GLINK_SMEM and WCSS, but selects only Q6V5_WCSS and misses to select GLINK_SMEM, would still built and make it non-functional, right ? Regards, Sricharan >> Which makes it clear that both these have to be same type? >> > > They don't have to be of the same type, only of a compatible type. > -- "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
Hi Bjorn, On 6/7/2018 9:54 AM, Bjorn Andersson wrote: > On Wed 06 Jun 21:11 PDT 2018, Vinod wrote: > >> On 06-06-18, 09:17, Bjorn Andersson wrote: >>> On Tue 05 Jun 05:56 PDT 2018, Sricharan R wrote: >>> Hi Vinod, On 6/5/2018 11:49 AM, Vinod wrote: > On 05-06-18, 11:12, Sricharan R wrote: > >> +config QCOM_Q6V5_WCSS >> +tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader" >> +depends on OF && ARCH_QCOM >> +depends on QCOM_SMEM >> +depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n) >> +depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > > Is there a reason why it depends on RPMSG_QCOM_GLINK_SMEM=n? What would > happen if distro wants both this and RPMSG_QCOM_GLINK_SMEM > >>> >>> It says that QCOM_Q6V5_WCSS either must have a compatible state (i.e. >>> builtin vs builtin, module vs builtin, but not builtin vs module) or >>> that it's disabled, in which case we will hit the stub functions in >>> qcom_glink.h. >>> >>> I.e. this prevents QCOM_Q6V5_WCSS to be compiled builtin when >>> RPMSG_QCOM_GLINK_SMEM is module, as this would give us both stubs and >>> the module. >> >> IIUC, you want to have QCOM_Q6V5_WCSS and RPMSG_QCOM_GLINK_SMEM as >> modules or builtin >> > > RPMSG_QCOM_SMD, RPMSG_QCOM_GLINK_SMEM and QCOM_Q6V5_WCSS are all > tristate. > >> So, wouldn't Kconfig syntax something like where we say: >> M if RPMSG_QCOM_GLINK_SMEM=m >> bool if RPMSG_QCOM_GLINK_SMEM=y >> > > If we ignore SMD for a while we have the following combinations: > > glink/wcss > y y - valid > y m - valid > y n - valid > m y - link failure (invalid) > m m - valid > m n - valid > n y - valid (platform uses wcss, but not glink) > n m - valid (-"-) > n n - valid > > So to distill this we have the two valid cases: > module/no if RPMSG_QCOM_GLINK_SMEM=m > yes/module/no if RPMSG_QCOM_GLINK_SMEM=y > > and the way you express that in Kconfig is the somewhat awkward > > depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > ok, Having "depends on RPMSG_QCOM_GLINK_SMEM" takes care of the first 6 cases in the above list. But just was thinking that by allowing the last three combinations, there is a chance that some config that really needs GLINK_SMEM and WCSS, but selects only Q6V5_WCSS and misses to select GLINK_SMEM, would still built and make it non-functional, right ? Regards, Sricharan >> Which makes it clear that both these have to be same type? >> > > They don't have to be of the same type, only of a compatible type. > -- "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
On Wed 06 Jun 21:11 PDT 2018, Vinod wrote: > On 06-06-18, 09:17, Bjorn Andersson wrote: > > On Tue 05 Jun 05:56 PDT 2018, Sricharan R wrote: > > > > > Hi Vinod, > > > > > > On 6/5/2018 11:49 AM, Vinod wrote: > > > > On 05-06-18, 11:12, Sricharan R wrote: > > > > > > > >> +config QCOM_Q6V5_WCSS > > > >> + tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader" > > > >> + depends on OF && ARCH_QCOM > > > >> + depends on QCOM_SMEM > > > >> + depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n) > > > >> + depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > > > > > > > > Is there a reason why it depends on RPMSG_QCOM_GLINK_SMEM=n? What would > > > > happen if distro wants both this and RPMSG_QCOM_GLINK_SMEM > > > > > > > > It says that QCOM_Q6V5_WCSS either must have a compatible state (i.e. > > builtin vs builtin, module vs builtin, but not builtin vs module) or > > that it's disabled, in which case we will hit the stub functions in > > qcom_glink.h. > > > > I.e. this prevents QCOM_Q6V5_WCSS to be compiled builtin when > > RPMSG_QCOM_GLINK_SMEM is module, as this would give us both stubs and > > the module. > > IIUC, you want to have QCOM_Q6V5_WCSS and RPMSG_QCOM_GLINK_SMEM as > modules or builtin > RPMSG_QCOM_SMD, RPMSG_QCOM_GLINK_SMEM and QCOM_Q6V5_WCSS are all tristate. > So, wouldn't Kconfig syntax something like where we say: > M if RPMSG_QCOM_GLINK_SMEM=m > bool if RPMSG_QCOM_GLINK_SMEM=y > If we ignore SMD for a while we have the following combinations: glink/wcss y y - valid y m - valid y n - valid m y - link failure (invalid) m m - valid m n - valid n y - valid (platform uses wcss, but not glink) n m - valid (-"-) n n - valid So to distill this we have the two valid cases: module/no if RPMSG_QCOM_GLINK_SMEM=m yes/module/no if RPMSG_QCOM_GLINK_SMEM=y and the way you express that in Kconfig is the somewhat awkward depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > Which makes it clear that both these have to be same type? > They don't have to be of the same type, only of a compatible type. Regards, Bjorn
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
On Wed 06 Jun 21:11 PDT 2018, Vinod wrote: > On 06-06-18, 09:17, Bjorn Andersson wrote: > > On Tue 05 Jun 05:56 PDT 2018, Sricharan R wrote: > > > > > Hi Vinod, > > > > > > On 6/5/2018 11:49 AM, Vinod wrote: > > > > On 05-06-18, 11:12, Sricharan R wrote: > > > > > > > >> +config QCOM_Q6V5_WCSS > > > >> + tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader" > > > >> + depends on OF && ARCH_QCOM > > > >> + depends on QCOM_SMEM > > > >> + depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n) > > > >> + depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > > > > > > > > Is there a reason why it depends on RPMSG_QCOM_GLINK_SMEM=n? What would > > > > happen if distro wants both this and RPMSG_QCOM_GLINK_SMEM > > > > > > > > It says that QCOM_Q6V5_WCSS either must have a compatible state (i.e. > > builtin vs builtin, module vs builtin, but not builtin vs module) or > > that it's disabled, in which case we will hit the stub functions in > > qcom_glink.h. > > > > I.e. this prevents QCOM_Q6V5_WCSS to be compiled builtin when > > RPMSG_QCOM_GLINK_SMEM is module, as this would give us both stubs and > > the module. > > IIUC, you want to have QCOM_Q6V5_WCSS and RPMSG_QCOM_GLINK_SMEM as > modules or builtin > RPMSG_QCOM_SMD, RPMSG_QCOM_GLINK_SMEM and QCOM_Q6V5_WCSS are all tristate. > So, wouldn't Kconfig syntax something like where we say: > M if RPMSG_QCOM_GLINK_SMEM=m > bool if RPMSG_QCOM_GLINK_SMEM=y > If we ignore SMD for a while we have the following combinations: glink/wcss y y - valid y m - valid y n - valid m y - link failure (invalid) m m - valid m n - valid n y - valid (platform uses wcss, but not glink) n m - valid (-"-) n n - valid So to distill this we have the two valid cases: module/no if RPMSG_QCOM_GLINK_SMEM=m yes/module/no if RPMSG_QCOM_GLINK_SMEM=y and the way you express that in Kconfig is the somewhat awkward depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > Which makes it clear that both these have to be same type? > They don't have to be of the same type, only of a compatible type. Regards, Bjorn
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
On 06-06-18, 09:17, Bjorn Andersson wrote: > On Tue 05 Jun 05:56 PDT 2018, Sricharan R wrote: > > > Hi Vinod, > > > > On 6/5/2018 11:49 AM, Vinod wrote: > > > On 05-06-18, 11:12, Sricharan R wrote: > > > > > >> +config QCOM_Q6V5_WCSS > > >> +tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader" > > >> +depends on OF && ARCH_QCOM > > >> +depends on QCOM_SMEM > > >> +depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n) > > >> +depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > > > > > > Is there a reason why it depends on RPMSG_QCOM_GLINK_SMEM=n? What would > > > happen if distro wants both this and RPMSG_QCOM_GLINK_SMEM > > > > > It says that QCOM_Q6V5_WCSS either must have a compatible state (i.e. > builtin vs builtin, module vs builtin, but not builtin vs module) or > that it's disabled, in which case we will hit the stub functions in > qcom_glink.h. > > I.e. this prevents QCOM_Q6V5_WCSS to be compiled builtin when > RPMSG_QCOM_GLINK_SMEM is module, as this would give us both stubs and > the module. IIUC, you want to have QCOM_Q6V5_WCSS and RPMSG_QCOM_GLINK_SMEM as modules or builtin So, wouldn't Kconfig syntax something like where we say: M if RPMSG_QCOM_GLINK_SMEM=m bool if RPMSG_QCOM_GLINK_SMEM=y Which makes it clear that both these have to be same type? -- ~Vinod
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
On 06-06-18, 09:17, Bjorn Andersson wrote: > On Tue 05 Jun 05:56 PDT 2018, Sricharan R wrote: > > > Hi Vinod, > > > > On 6/5/2018 11:49 AM, Vinod wrote: > > > On 05-06-18, 11:12, Sricharan R wrote: > > > > > >> +config QCOM_Q6V5_WCSS > > >> +tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader" > > >> +depends on OF && ARCH_QCOM > > >> +depends on QCOM_SMEM > > >> +depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n) > > >> +depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > > > > > > Is there a reason why it depends on RPMSG_QCOM_GLINK_SMEM=n? What would > > > happen if distro wants both this and RPMSG_QCOM_GLINK_SMEM > > > > > It says that QCOM_Q6V5_WCSS either must have a compatible state (i.e. > builtin vs builtin, module vs builtin, but not builtin vs module) or > that it's disabled, in which case we will hit the stub functions in > qcom_glink.h. > > I.e. this prevents QCOM_Q6V5_WCSS to be compiled builtin when > RPMSG_QCOM_GLINK_SMEM is module, as this would give us both stubs and > the module. IIUC, you want to have QCOM_Q6V5_WCSS and RPMSG_QCOM_GLINK_SMEM as modules or builtin So, wouldn't Kconfig syntax something like where we say: M if RPMSG_QCOM_GLINK_SMEM=m bool if RPMSG_QCOM_GLINK_SMEM=y Which makes it clear that both these have to be same type? -- ~Vinod
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
On Tue 05 Jun 05:56 PDT 2018, Sricharan R wrote: > Hi Vinod, > > On 6/5/2018 11:49 AM, Vinod wrote: > > On 05-06-18, 11:12, Sricharan R wrote: > > > >> +config QCOM_Q6V5_WCSS > >> + tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader" > >> + depends on OF && ARCH_QCOM > >> + depends on QCOM_SMEM > >> + depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n) > >> + depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > > > > Is there a reason why it depends on RPMSG_QCOM_GLINK_SMEM=n? What would > > happen if distro wants both this and RPMSG_QCOM_GLINK_SMEM > > It says that QCOM_Q6V5_WCSS either must have a compatible state (i.e. builtin vs builtin, module vs builtin, but not builtin vs module) or that it's disabled, in which case we will hit the stub functions in qcom_glink.h. I.e. this prevents QCOM_Q6V5_WCSS to be compiled builtin when RPMSG_QCOM_GLINK_SMEM is module, as this would give us both stubs and the module. > RPMSG_QCOM_GLINK_SMEM=n should be for the COMPILE_TEST. Probably that > means that it should be corrected here and for ADSP, Q6V5_PIL as well. > Bjorn, is that correct ?, should it be, below ? > There are platforms with SMD, those with GLINK-SMEM and those with both. For the two first we want it to be possible only compile the specific transport being used and the other being stubbed. As Sricharan's particular platform uses GLINK for communicating with the WCSS it's perfectly fine to run this particular driver with RPMSG_QCOM_SMD=n RPMSG_QCOM_GLINK_SMEM=y/m As such I would recommend that you drop COMPILE_TEST from above. Regards, Bjorn
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
On Tue 05 Jun 05:56 PDT 2018, Sricharan R wrote: > Hi Vinod, > > On 6/5/2018 11:49 AM, Vinod wrote: > > On 05-06-18, 11:12, Sricharan R wrote: > > > >> +config QCOM_Q6V5_WCSS > >> + tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader" > >> + depends on OF && ARCH_QCOM > >> + depends on QCOM_SMEM > >> + depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n) > >> + depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > > > > Is there a reason why it depends on RPMSG_QCOM_GLINK_SMEM=n? What would > > happen if distro wants both this and RPMSG_QCOM_GLINK_SMEM > > It says that QCOM_Q6V5_WCSS either must have a compatible state (i.e. builtin vs builtin, module vs builtin, but not builtin vs module) or that it's disabled, in which case we will hit the stub functions in qcom_glink.h. I.e. this prevents QCOM_Q6V5_WCSS to be compiled builtin when RPMSG_QCOM_GLINK_SMEM is module, as this would give us both stubs and the module. > RPMSG_QCOM_GLINK_SMEM=n should be for the COMPILE_TEST. Probably that > means that it should be corrected here and for ADSP, Q6V5_PIL as well. > Bjorn, is that correct ?, should it be, below ? > There are platforms with SMD, those with GLINK-SMEM and those with both. For the two first we want it to be possible only compile the specific transport being used and the other being stubbed. As Sricharan's particular platform uses GLINK for communicating with the WCSS it's perfectly fine to run this particular driver with RPMSG_QCOM_SMD=n RPMSG_QCOM_GLINK_SMEM=y/m As such I would recommend that you drop COMPILE_TEST from above. Regards, Bjorn
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
Hi Vinod, On 6/6/2018 12:19 PM, Vinod wrote: > Hi Sricharan, > > On 06-06-18, 12:09, Sricharan R wrote: > >> +config QCOM_Q6V5_WCSS >> +tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader" >> +depends on OF && ARCH_QCOM >> +depends on QCOM_SMEM >> +depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n) >> +depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > > Is there a reason why it depends on RPMSG_QCOM_GLINK_SMEM=n? What would > happen if distro wants both this and RPMSG_QCOM_GLINK_SMEM > RPMSG_QCOM_GLINK_SMEM=n should be for the COMPILE_TEST. Probably that >>> >>> why would that be a limitation? I am more worried about >>> RPMSG_QCOM_GLINK_SMEM=n being the condition here. In new drivers we >>> should not typically have dependency on some symbol being not there >> >> Without that, if RPMSG_QCOM_GLINK_SMEM=m is compiled as a module, then >> it would break the build. > > Okay I do not know the details, but that doesn't sound correct to me. > Breaking build sounds a bit extreme to me. Can you give details on this > part.. > Having, just, depends on RPMSG_QCOM_GLINK_SMEM || COMPILE_TEST, is going to break when RPMSG_QCOM_GLINK_SMEM=m and COMPILE_TEST=y. Hence the COMPILE_TEST && RPMSG_QCOM_GLINK_SMEM=n. Having said that, COMPILE_TEST is getting tested for RPMSG_QCOM_SMD=n in the previous line. So that's the reason for not having it in below line for RPMSG_QCOM_GLINK_SMEM. means that it should be corrected here and for ADSP, Q6V5_PIL as well. Bjorn, is that correct ?, should it be, below ? depends on (RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n)) || (RPMSG_QCOM_GLINK_SMEM || (COMPILE_TEST && RPMSG_QCOM_GLINK_SMEM=n)) >>> >>> that doesnt really sound good :( >> >> Hmm, but i was thinking it should functionally depend on either SMD or >> GLINK and not both. > > If you are depedent upon a symbol provided by a module you should say > depends on. If a machine is not supposed to have both SMD or GLINK then > the driver will not get probed. > This is where, i was thinking, it should be functional if either of SMD or GLINK is there, but should not require both. Regards, Sricharan -- "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
Hi Vinod, On 6/6/2018 12:19 PM, Vinod wrote: > Hi Sricharan, > > On 06-06-18, 12:09, Sricharan R wrote: > >> +config QCOM_Q6V5_WCSS >> +tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader" >> +depends on OF && ARCH_QCOM >> +depends on QCOM_SMEM >> +depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n) >> +depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > > Is there a reason why it depends on RPMSG_QCOM_GLINK_SMEM=n? What would > happen if distro wants both this and RPMSG_QCOM_GLINK_SMEM > RPMSG_QCOM_GLINK_SMEM=n should be for the COMPILE_TEST. Probably that >>> >>> why would that be a limitation? I am more worried about >>> RPMSG_QCOM_GLINK_SMEM=n being the condition here. In new drivers we >>> should not typically have dependency on some symbol being not there >> >> Without that, if RPMSG_QCOM_GLINK_SMEM=m is compiled as a module, then >> it would break the build. > > Okay I do not know the details, but that doesn't sound correct to me. > Breaking build sounds a bit extreme to me. Can you give details on this > part.. > Having, just, depends on RPMSG_QCOM_GLINK_SMEM || COMPILE_TEST, is going to break when RPMSG_QCOM_GLINK_SMEM=m and COMPILE_TEST=y. Hence the COMPILE_TEST && RPMSG_QCOM_GLINK_SMEM=n. Having said that, COMPILE_TEST is getting tested for RPMSG_QCOM_SMD=n in the previous line. So that's the reason for not having it in below line for RPMSG_QCOM_GLINK_SMEM. means that it should be corrected here and for ADSP, Q6V5_PIL as well. Bjorn, is that correct ?, should it be, below ? depends on (RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n)) || (RPMSG_QCOM_GLINK_SMEM || (COMPILE_TEST && RPMSG_QCOM_GLINK_SMEM=n)) >>> >>> that doesnt really sound good :( >> >> Hmm, but i was thinking it should functionally depend on either SMD or >> GLINK and not both. > > If you are depedent upon a symbol provided by a module you should say > depends on. If a machine is not supposed to have both SMD or GLINK then > the driver will not get probed. > This is where, i was thinking, it should be functional if either of SMD or GLINK is there, but should not require both. Regards, Sricharan -- "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
Hi Sricharan, On 06-06-18, 12:09, Sricharan R wrote: > +config QCOM_Q6V5_WCSS > +tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader" > +depends on OF && ARCH_QCOM > +depends on QCOM_SMEM > +depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n) > +depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > >>> > >>> Is there a reason why it depends on RPMSG_QCOM_GLINK_SMEM=n? What would > >>> happen if distro wants both this and RPMSG_QCOM_GLINK_SMEM > >>> > >> RPMSG_QCOM_GLINK_SMEM=n should be for the COMPILE_TEST. Probably that > > > > why would that be a limitation? I am more worried about > > RPMSG_QCOM_GLINK_SMEM=n being the condition here. In new drivers we > > should not typically have dependency on some symbol being not there > > Without that, if RPMSG_QCOM_GLINK_SMEM=m is compiled as a module, then > it would break the build. Okay I do not know the details, but that doesn't sound correct to me. Breaking build sounds a bit extreme to me. Can you give details on this part.. > >> means that it should be corrected here and for ADSP, Q6V5_PIL as well. > >> Bjorn, is that correct ?, should it be, below ? > >> > >> depends on (RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n)) || > >> (RPMSG_QCOM_GLINK_SMEM || (COMPILE_TEST && RPMSG_QCOM_GLINK_SMEM=n)) > > > > that doesnt really sound good :( > > Hmm, but i was thinking it should functionally depend on either SMD or GLINK > and not both. If you are depedent upon a symbol provided by a module you should say depends on. If a machine is not supposed to have both SMD or GLINK then the driver will not get probed. -- ~Vinod
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
Hi Sricharan, On 06-06-18, 12:09, Sricharan R wrote: > +config QCOM_Q6V5_WCSS > +tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader" > +depends on OF && ARCH_QCOM > +depends on QCOM_SMEM > +depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n) > +depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > >>> > >>> Is there a reason why it depends on RPMSG_QCOM_GLINK_SMEM=n? What would > >>> happen if distro wants both this and RPMSG_QCOM_GLINK_SMEM > >>> > >> RPMSG_QCOM_GLINK_SMEM=n should be for the COMPILE_TEST. Probably that > > > > why would that be a limitation? I am more worried about > > RPMSG_QCOM_GLINK_SMEM=n being the condition here. In new drivers we > > should not typically have dependency on some symbol being not there > > Without that, if RPMSG_QCOM_GLINK_SMEM=m is compiled as a module, then > it would break the build. Okay I do not know the details, but that doesn't sound correct to me. Breaking build sounds a bit extreme to me. Can you give details on this part.. > >> means that it should be corrected here and for ADSP, Q6V5_PIL as well. > >> Bjorn, is that correct ?, should it be, below ? > >> > >> depends on (RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n)) || > >> (RPMSG_QCOM_GLINK_SMEM || (COMPILE_TEST && RPMSG_QCOM_GLINK_SMEM=n)) > > > > that doesnt really sound good :( > > Hmm, but i was thinking it should functionally depend on either SMD or GLINK > and not both. If you are depedent upon a symbol provided by a module you should say depends on. If a machine is not supposed to have both SMD or GLINK then the driver will not get probed. -- ~Vinod
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
Hi Vinod, On 6/5/2018 10:10 PM, Vinod Koul wrote: > On 05-06-18, 18:26, Sricharan R wrote: >> Hi Vinod, >> >> On 6/5/2018 11:49 AM, Vinod wrote: >>> On 05-06-18, 11:12, Sricharan R wrote: >>> +config QCOM_Q6V5_WCSS + tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader" + depends on OF && ARCH_QCOM + depends on QCOM_SMEM + depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n) + depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n >>> >>> Is there a reason why it depends on RPMSG_QCOM_GLINK_SMEM=n? What would >>> happen if distro wants both this and RPMSG_QCOM_GLINK_SMEM >>> >> RPMSG_QCOM_GLINK_SMEM=n should be for the COMPILE_TEST. Probably that > > why would that be a limitation? I am more worried about > RPMSG_QCOM_GLINK_SMEM=n being the condition here. In new drivers we > should not typically have dependency on some symbol being not there > Without that, if RPMSG_QCOM_GLINK_SMEM=m is compiled as a module, then it would break the build. >> means that it should be corrected here and for ADSP, Q6V5_PIL as well. >> Bjorn, is that correct ?, should it be, below ? >> >> depends on (RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n)) || >> (RPMSG_QCOM_GLINK_SMEM || (COMPILE_TEST && RPMSG_QCOM_GLINK_SMEM=n)) > > that doesnt really sound good :( > Hmm, but i was thinking it should functionally depend on either SMD or GLINK and not both. Regards, Sricharan -- "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
Hi Vinod, On 6/5/2018 10:10 PM, Vinod Koul wrote: > On 05-06-18, 18:26, Sricharan R wrote: >> Hi Vinod, >> >> On 6/5/2018 11:49 AM, Vinod wrote: >>> On 05-06-18, 11:12, Sricharan R wrote: >>> +config QCOM_Q6V5_WCSS + tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader" + depends on OF && ARCH_QCOM + depends on QCOM_SMEM + depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n) + depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n >>> >>> Is there a reason why it depends on RPMSG_QCOM_GLINK_SMEM=n? What would >>> happen if distro wants both this and RPMSG_QCOM_GLINK_SMEM >>> >> RPMSG_QCOM_GLINK_SMEM=n should be for the COMPILE_TEST. Probably that > > why would that be a limitation? I am more worried about > RPMSG_QCOM_GLINK_SMEM=n being the condition here. In new drivers we > should not typically have dependency on some symbol being not there > Without that, if RPMSG_QCOM_GLINK_SMEM=m is compiled as a module, then it would break the build. >> means that it should be corrected here and for ADSP, Q6V5_PIL as well. >> Bjorn, is that correct ?, should it be, below ? >> >> depends on (RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n)) || >> (RPMSG_QCOM_GLINK_SMEM || (COMPILE_TEST && RPMSG_QCOM_GLINK_SMEM=n)) > > that doesnt really sound good :( > Hmm, but i was thinking it should functionally depend on either SMD or GLINK and not both. Regards, Sricharan -- "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation --- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
On 05-06-18, 18:26, Sricharan R wrote: > Hi Vinod, > > On 6/5/2018 11:49 AM, Vinod wrote: > > On 05-06-18, 11:12, Sricharan R wrote: > > > >> +config QCOM_Q6V5_WCSS > >> + tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader" > >> + depends on OF && ARCH_QCOM > >> + depends on QCOM_SMEM > >> + depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n) > >> + depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > > > > Is there a reason why it depends on RPMSG_QCOM_GLINK_SMEM=n? What would > > happen if distro wants both this and RPMSG_QCOM_GLINK_SMEM > > > RPMSG_QCOM_GLINK_SMEM=n should be for the COMPILE_TEST. Probably that why would that be a limitation? I am more worried about RPMSG_QCOM_GLINK_SMEM=n being the condition here. In new drivers we should not typically have dependency on some symbol being not there > means that it should be corrected here and for ADSP, Q6V5_PIL as well. > Bjorn, is that correct ?, should it be, below ? > > depends on (RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n)) || > (RPMSG_QCOM_GLINK_SMEM || (COMPILE_TEST && RPMSG_QCOM_GLINK_SMEM=n)) that doesnt really sound good :( -- ~Vinod
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
On 05-06-18, 18:26, Sricharan R wrote: > Hi Vinod, > > On 6/5/2018 11:49 AM, Vinod wrote: > > On 05-06-18, 11:12, Sricharan R wrote: > > > >> +config QCOM_Q6V5_WCSS > >> + tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader" > >> + depends on OF && ARCH_QCOM > >> + depends on QCOM_SMEM > >> + depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n) > >> + depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n > > > > Is there a reason why it depends on RPMSG_QCOM_GLINK_SMEM=n? What would > > happen if distro wants both this and RPMSG_QCOM_GLINK_SMEM > > > RPMSG_QCOM_GLINK_SMEM=n should be for the COMPILE_TEST. Probably that why would that be a limitation? I am more worried about RPMSG_QCOM_GLINK_SMEM=n being the condition here. In new drivers we should not typically have dependency on some symbol being not there > means that it should be corrected here and for ADSP, Q6V5_PIL as well. > Bjorn, is that correct ?, should it be, below ? > > depends on (RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n)) || > (RPMSG_QCOM_GLINK_SMEM || (COMPILE_TEST && RPMSG_QCOM_GLINK_SMEM=n)) that doesnt really sound good :( -- ~Vinod
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
Hi Vinod,
On 6/5/2018 11:49 AM, Vinod wrote:
> On 05-06-18, 11:12, Sricharan R wrote:
>
>> +config QCOM_Q6V5_WCSS
>> +tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader"
>> +depends on OF && ARCH_QCOM
>> +depends on QCOM_SMEM
>> +depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n)
>> +depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n
>
> Is there a reason why it depends on RPMSG_QCOM_GLINK_SMEM=n? What would
> happen if distro wants both this and RPMSG_QCOM_GLINK_SMEM
>
RPMSG_QCOM_GLINK_SMEM=n should be for the COMPILE_TEST. Probably that
means that it should be corrected here and for ADSP, Q6V5_PIL as well.
Bjorn, is that correct ?, should it be, below ?
depends on (RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n)) ||
(RPMSG_QCOM_GLINK_SMEM || (COMPILE_TEST && RPMSG_QCOM_GLINK_SMEM=n))
>> +/* QDSP6SS Register Offsets */
>> +#define QDSP6SS_RESET_REG 0x014
>> +#define QDSP6SS_GFMUX_CTL_REG 0x020
>> +#define QDSP6SS_PWR_CTL_REG 0x030
>> +#define QDSP6SS_MEM_PWR_CTL 0x0B0
>> +
>> +/* AXI Halt Register Offsets */
>> +#define AXI_HALTREQ_REG 0x0
>> +#define AXI_HALTACK_REG 0x4
>> +#define AXI_IDLE_REG0x8
>> +
>> +#define HALT_ACK_TIMEOUT_MS 100
>> +
>> +/* QDSP6SS_RESET */
>> +#define Q6SS_STOP_CORE BIT(0)
>> +#define Q6SS_CORE_ARES BIT(1)
>> +#define Q6SS_BUS_ARES_ENABLEBIT(2)
>
> Wouldn't it be nice if the defines are all consistent, some of them are
> QDSP6SS_xxx, some Q6SS_ some are not
>
> Please do pick one and make it consistent :)
>
ok.
>> +/* QDSP6v56 parameters */
>> +#define QDSP6v56_LDO_BYPBIT(25)
>> +#define QDSP6v56_BHS_ON BIT(24)
>> +#define QDSP6v56_CLAMP_WL BIT(21)
>> +#define QDSP6v56_CLAMP_QMC_MEM BIT(22)
>> +#define HALT_CHECK_MAX_LOOPS200
>> +#define QDSP6SS_XO_CBCR 0x0038
>
> GENMASK() perhaps?
>
ok.
>> +static int q6v5_wcss_reset(struct q6v5_wcss *wcss)
>> +{
>> +int ret;
>> +u32 val;
>> +int i;
>
> int ret, i;
>
>> +static int q6v5_wcss_start(struct rproc *rproc)
>> +{
>> +struct q6v5_wcss *wcss = rproc->priv;
>> +int ret;
>> +
>> +qcom_q6v5_prepare(>q6v5);
>> +
>> +/* Release Q6 and WCSS reset */
>> +ret = reset_control_deassert(wcss->wcss_reset);
>> +if (ret) {
>> +dev_err(wcss->dev, "wcss_reset failed\n");
>> +return ret;
>> +}
>> +
>> +ret = reset_control_deassert(wcss->wcss_q6_reset);
>> +if (ret) {
>> +dev_err(wcss->dev, "wcss_q6_reset failed\n");
>> +goto wcss_reset;
>> +}
>> +
>> +/* Lithium configuration - clock gating and bus arbitration */
>> +ret = regmap_update_bits(wcss->halt_map,
>> + wcss->halt_nc + TCSR_GLOBAL_CFG0,
>> + 0x1F, 0x14);
>
> magic numbers??
>
ok, will find out what it is for this one and below.
But atleast from register map could not find out and
these are sort of hardcoded sequences coming from the hw
folks. So will have to reach out to them to find the specifics.
>> +static int q6v5_wcss_powerdown(struct q6v5_wcss *wcss)
>> +{
>> +int ret;
>> +u32 val;
>> +
>> +/* 1 - Assert WCSS/Q6 HALTREQ */
>> +q6v5_wcss_halt_axi_port(wcss, wcss->halt_map, wcss->halt_wcss);
>> +
>> +/* 2 - Enable WCSSAON_CONFIG */
>> +val = readl(wcss->rmb_base + SSCAON_CONFIG);
>> +val |= SSCAON_ENABLE;
>> +writel(val, wcss->rmb_base + SSCAON_CONFIG);
>> +
>> +/* 3 - Set SSCAON_CONFIG */
>> +val |= BIT(15);
>> +val &= ~BIT(16);
>> +val &= ~BIT(17);
>> +val &= ~BIT(18);
>
> wouldn't it be nice to define these bits?
>
>> +static int q6v5_q6_powerdown(struct q6v5_wcss *wcss)
>> +{
>> +int ret;
>> +u32 val;
>> +int i;
>
> int ret, i;
>
Regards,
Sricharan
--
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
Code Aurora Forum, hosted by The Linux Foundation
---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
Hi Vinod,
On 6/5/2018 11:49 AM, Vinod wrote:
> On 05-06-18, 11:12, Sricharan R wrote:
>
>> +config QCOM_Q6V5_WCSS
>> +tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader"
>> +depends on OF && ARCH_QCOM
>> +depends on QCOM_SMEM
>> +depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n)
>> +depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n
>
> Is there a reason why it depends on RPMSG_QCOM_GLINK_SMEM=n? What would
> happen if distro wants both this and RPMSG_QCOM_GLINK_SMEM
>
RPMSG_QCOM_GLINK_SMEM=n should be for the COMPILE_TEST. Probably that
means that it should be corrected here and for ADSP, Q6V5_PIL as well.
Bjorn, is that correct ?, should it be, below ?
depends on (RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n)) ||
(RPMSG_QCOM_GLINK_SMEM || (COMPILE_TEST && RPMSG_QCOM_GLINK_SMEM=n))
>> +/* QDSP6SS Register Offsets */
>> +#define QDSP6SS_RESET_REG 0x014
>> +#define QDSP6SS_GFMUX_CTL_REG 0x020
>> +#define QDSP6SS_PWR_CTL_REG 0x030
>> +#define QDSP6SS_MEM_PWR_CTL 0x0B0
>> +
>> +/* AXI Halt Register Offsets */
>> +#define AXI_HALTREQ_REG 0x0
>> +#define AXI_HALTACK_REG 0x4
>> +#define AXI_IDLE_REG0x8
>> +
>> +#define HALT_ACK_TIMEOUT_MS 100
>> +
>> +/* QDSP6SS_RESET */
>> +#define Q6SS_STOP_CORE BIT(0)
>> +#define Q6SS_CORE_ARES BIT(1)
>> +#define Q6SS_BUS_ARES_ENABLEBIT(2)
>
> Wouldn't it be nice if the defines are all consistent, some of them are
> QDSP6SS_xxx, some Q6SS_ some are not
>
> Please do pick one and make it consistent :)
>
ok.
>> +/* QDSP6v56 parameters */
>> +#define QDSP6v56_LDO_BYPBIT(25)
>> +#define QDSP6v56_BHS_ON BIT(24)
>> +#define QDSP6v56_CLAMP_WL BIT(21)
>> +#define QDSP6v56_CLAMP_QMC_MEM BIT(22)
>> +#define HALT_CHECK_MAX_LOOPS200
>> +#define QDSP6SS_XO_CBCR 0x0038
>
> GENMASK() perhaps?
>
ok.
>> +static int q6v5_wcss_reset(struct q6v5_wcss *wcss)
>> +{
>> +int ret;
>> +u32 val;
>> +int i;
>
> int ret, i;
>
>> +static int q6v5_wcss_start(struct rproc *rproc)
>> +{
>> +struct q6v5_wcss *wcss = rproc->priv;
>> +int ret;
>> +
>> +qcom_q6v5_prepare(>q6v5);
>> +
>> +/* Release Q6 and WCSS reset */
>> +ret = reset_control_deassert(wcss->wcss_reset);
>> +if (ret) {
>> +dev_err(wcss->dev, "wcss_reset failed\n");
>> +return ret;
>> +}
>> +
>> +ret = reset_control_deassert(wcss->wcss_q6_reset);
>> +if (ret) {
>> +dev_err(wcss->dev, "wcss_q6_reset failed\n");
>> +goto wcss_reset;
>> +}
>> +
>> +/* Lithium configuration - clock gating and bus arbitration */
>> +ret = regmap_update_bits(wcss->halt_map,
>> + wcss->halt_nc + TCSR_GLOBAL_CFG0,
>> + 0x1F, 0x14);
>
> magic numbers??
>
ok, will find out what it is for this one and below.
But atleast from register map could not find out and
these are sort of hardcoded sequences coming from the hw
folks. So will have to reach out to them to find the specifics.
>> +static int q6v5_wcss_powerdown(struct q6v5_wcss *wcss)
>> +{
>> +int ret;
>> +u32 val;
>> +
>> +/* 1 - Assert WCSS/Q6 HALTREQ */
>> +q6v5_wcss_halt_axi_port(wcss, wcss->halt_map, wcss->halt_wcss);
>> +
>> +/* 2 - Enable WCSSAON_CONFIG */
>> +val = readl(wcss->rmb_base + SSCAON_CONFIG);
>> +val |= SSCAON_ENABLE;
>> +writel(val, wcss->rmb_base + SSCAON_CONFIG);
>> +
>> +/* 3 - Set SSCAON_CONFIG */
>> +val |= BIT(15);
>> +val &= ~BIT(16);
>> +val &= ~BIT(17);
>> +val &= ~BIT(18);
>
> wouldn't it be nice to define these bits?
>
>> +static int q6v5_q6_powerdown(struct q6v5_wcss *wcss)
>> +{
>> +int ret;
>> +u32 val;
>> +int i;
>
> int ret, i;
>
Regards,
Sricharan
--
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
Code Aurora Forum, hosted by The Linux Foundation
---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
On 05-06-18, 11:12, Sricharan R wrote:
> +config QCOM_Q6V5_WCSS
> + tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader"
> + depends on OF && ARCH_QCOM
> + depends on QCOM_SMEM
> + depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n)
> + depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n
Is there a reason why it depends on RPMSG_QCOM_GLINK_SMEM=n? What would
happen if distro wants both this and RPMSG_QCOM_GLINK_SMEM
> +/* QDSP6SS Register Offsets */
> +#define QDSP6SS_RESET_REG0x014
> +#define QDSP6SS_GFMUX_CTL_REG0x020
> +#define QDSP6SS_PWR_CTL_REG 0x030
> +#define QDSP6SS_MEM_PWR_CTL 0x0B0
> +
> +/* AXI Halt Register Offsets */
> +#define AXI_HALTREQ_REG 0x0
> +#define AXI_HALTACK_REG 0x4
> +#define AXI_IDLE_REG 0x8
> +
> +#define HALT_ACK_TIMEOUT_MS 100
> +
> +/* QDSP6SS_RESET */
> +#define Q6SS_STOP_CORE BIT(0)
> +#define Q6SS_CORE_ARES BIT(1)
> +#define Q6SS_BUS_ARES_ENABLE BIT(2)
Wouldn't it be nice if the defines are all consistent, some of them are
QDSP6SS_xxx, some Q6SS_ some are not
Please do pick one and make it consistent :)
> +/* QDSP6v56 parameters */
> +#define QDSP6v56_LDO_BYP BIT(25)
> +#define QDSP6v56_BHS_ON BIT(24)
> +#define QDSP6v56_CLAMP_WLBIT(21)
> +#define QDSP6v56_CLAMP_QMC_MEM BIT(22)
> +#define HALT_CHECK_MAX_LOOPS 200
> +#define QDSP6SS_XO_CBCR 0x0038
GENMASK() perhaps?
> +static int q6v5_wcss_reset(struct q6v5_wcss *wcss)
> +{
> + int ret;
> + u32 val;
> + int i;
int ret, i;
> +static int q6v5_wcss_start(struct rproc *rproc)
> +{
> + struct q6v5_wcss *wcss = rproc->priv;
> + int ret;
> +
> + qcom_q6v5_prepare(>q6v5);
> +
> + /* Release Q6 and WCSS reset */
> + ret = reset_control_deassert(wcss->wcss_reset);
> + if (ret) {
> + dev_err(wcss->dev, "wcss_reset failed\n");
> + return ret;
> + }
> +
> + ret = reset_control_deassert(wcss->wcss_q6_reset);
> + if (ret) {
> + dev_err(wcss->dev, "wcss_q6_reset failed\n");
> + goto wcss_reset;
> + }
> +
> + /* Lithium configuration - clock gating and bus arbitration */
> + ret = regmap_update_bits(wcss->halt_map,
> + wcss->halt_nc + TCSR_GLOBAL_CFG0,
> + 0x1F, 0x14);
magic numbers??
> +static int q6v5_wcss_powerdown(struct q6v5_wcss *wcss)
> +{
> + int ret;
> + u32 val;
> +
> + /* 1 - Assert WCSS/Q6 HALTREQ */
> + q6v5_wcss_halt_axi_port(wcss, wcss->halt_map, wcss->halt_wcss);
> +
> + /* 2 - Enable WCSSAON_CONFIG */
> + val = readl(wcss->rmb_base + SSCAON_CONFIG);
> + val |= SSCAON_ENABLE;
> + writel(val, wcss->rmb_base + SSCAON_CONFIG);
> +
> + /* 3 - Set SSCAON_CONFIG */
> + val |= BIT(15);
> + val &= ~BIT(16);
> + val &= ~BIT(17);
> + val &= ~BIT(18);
wouldn't it be nice to define these bits?
> +static int q6v5_q6_powerdown(struct q6v5_wcss *wcss)
> +{
> + int ret;
> + u32 val;
> + int i;
int ret, i;
--
~Vinod
Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
On 05-06-18, 11:12, Sricharan R wrote:
> +config QCOM_Q6V5_WCSS
> + tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader"
> + depends on OF && ARCH_QCOM
> + depends on QCOM_SMEM
> + depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n)
> + depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n
Is there a reason why it depends on RPMSG_QCOM_GLINK_SMEM=n? What would
happen if distro wants both this and RPMSG_QCOM_GLINK_SMEM
> +/* QDSP6SS Register Offsets */
> +#define QDSP6SS_RESET_REG0x014
> +#define QDSP6SS_GFMUX_CTL_REG0x020
> +#define QDSP6SS_PWR_CTL_REG 0x030
> +#define QDSP6SS_MEM_PWR_CTL 0x0B0
> +
> +/* AXI Halt Register Offsets */
> +#define AXI_HALTREQ_REG 0x0
> +#define AXI_HALTACK_REG 0x4
> +#define AXI_IDLE_REG 0x8
> +
> +#define HALT_ACK_TIMEOUT_MS 100
> +
> +/* QDSP6SS_RESET */
> +#define Q6SS_STOP_CORE BIT(0)
> +#define Q6SS_CORE_ARES BIT(1)
> +#define Q6SS_BUS_ARES_ENABLE BIT(2)
Wouldn't it be nice if the defines are all consistent, some of them are
QDSP6SS_xxx, some Q6SS_ some are not
Please do pick one and make it consistent :)
> +/* QDSP6v56 parameters */
> +#define QDSP6v56_LDO_BYP BIT(25)
> +#define QDSP6v56_BHS_ON BIT(24)
> +#define QDSP6v56_CLAMP_WLBIT(21)
> +#define QDSP6v56_CLAMP_QMC_MEM BIT(22)
> +#define HALT_CHECK_MAX_LOOPS 200
> +#define QDSP6SS_XO_CBCR 0x0038
GENMASK() perhaps?
> +static int q6v5_wcss_reset(struct q6v5_wcss *wcss)
> +{
> + int ret;
> + u32 val;
> + int i;
int ret, i;
> +static int q6v5_wcss_start(struct rproc *rproc)
> +{
> + struct q6v5_wcss *wcss = rproc->priv;
> + int ret;
> +
> + qcom_q6v5_prepare(>q6v5);
> +
> + /* Release Q6 and WCSS reset */
> + ret = reset_control_deassert(wcss->wcss_reset);
> + if (ret) {
> + dev_err(wcss->dev, "wcss_reset failed\n");
> + return ret;
> + }
> +
> + ret = reset_control_deassert(wcss->wcss_q6_reset);
> + if (ret) {
> + dev_err(wcss->dev, "wcss_q6_reset failed\n");
> + goto wcss_reset;
> + }
> +
> + /* Lithium configuration - clock gating and bus arbitration */
> + ret = regmap_update_bits(wcss->halt_map,
> + wcss->halt_nc + TCSR_GLOBAL_CFG0,
> + 0x1F, 0x14);
magic numbers??
> +static int q6v5_wcss_powerdown(struct q6v5_wcss *wcss)
> +{
> + int ret;
> + u32 val;
> +
> + /* 1 - Assert WCSS/Q6 HALTREQ */
> + q6v5_wcss_halt_axi_port(wcss, wcss->halt_map, wcss->halt_wcss);
> +
> + /* 2 - Enable WCSSAON_CONFIG */
> + val = readl(wcss->rmb_base + SSCAON_CONFIG);
> + val |= SSCAON_ENABLE;
> + writel(val, wcss->rmb_base + SSCAON_CONFIG);
> +
> + /* 3 - Set SSCAON_CONFIG */
> + val |= BIT(15);
> + val &= ~BIT(16);
> + val &= ~BIT(17);
> + val &= ~BIT(18);
wouldn't it be nice to define these bits?
> +static int q6v5_q6_powerdown(struct q6v5_wcss *wcss)
> +{
> + int ret;
> + u32 val;
> + int i;
int ret, i;
--
~Vinod
[PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
IPQ8074 has an integrated Hexagon dsp core q6v5 and a wireless lan (Lithium) IP. An mdt type single image format is used for the firmware. So the mdt_load function can be directly used to load the firmware. Also add the relevant resets required for this core. Acked-by: Rob Herring (for bindings) Signed-off-by: Sricharan R [bjorn: Rewrote as a separate driver, intead of extending q6v5_pil.c] Signed-off-by: Bjorn Andersson --- Fixed review comments from Vinod. Retained the reg read/update/write sequence instead of modify for readability In q6v5_wcss_powerdown SSCAON_CONFIG bits 16,17,18 documentation have to be updated. .../devicetree/bindings/remoteproc/qcom,q6v5.txt | 7 +- drivers/remoteproc/Kconfig | 16 + drivers/remoteproc/Makefile| 1 + drivers/remoteproc/qcom_q6v5_wcss.c| 588 + 4 files changed, 611 insertions(+), 1 deletion(-) create mode 100644 drivers/remoteproc/qcom_q6v5_wcss.c diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt index 00d3d58..d52d05e 100644 --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt @@ -11,6 +11,7 @@ on the Qualcomm Hexagon core. "qcom,msm8916-mss-pil", "qcom,msm8974-mss-pil" "qcom,msm8996-mss-pil" + "qcom,ipq8074-wcss-pil" - reg: Usage: required @@ -49,11 +50,15 @@ on the Qualcomm Hexagon core. Usage: required Value type: Definition: reference to the reset-controller for the modem sub-system + reference to the list of 3 reset-controllers for the + wcss sub-system - reset-names: Usage: required Value type: - Definition: must be "mss_restart" + Definition: must be "mss_restart" for the modem sub-system + Definition: must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset" + for the wcss syb-system - cx-supply: - mss-supply: diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig index be76619..7b1a9ef 100644 --- a/drivers/remoteproc/Kconfig +++ b/drivers/remoteproc/Kconfig @@ -123,6 +123,22 @@ config QCOM_Q6V5_PIL Say y here to support the Qualcomm Peripherial Image Loader for the Hexagon V5 based remote processors. +config QCOM_Q6V5_WCSS + tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader" + depends on OF && ARCH_QCOM + depends on QCOM_SMEM + depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n) + depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n + depends on QCOM_SYSMON || QCOM_SYSMON=n + select MFD_SYSCON + select QCOM_MDT_LOADER + select QCOM_Q6V5_COMMON + select QCOM_RPROC_COMMON + select QCOM_SCM + help + Say y here to support the Qualcomm Peripheral Image Loader for the + Hexagon V5 based WCSS remote processors. + config QCOM_SYSMON tristate "Qualcomm sysmon driver" depends on RPMSG diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile index 5dd0249..03332fa 100644 --- a/drivers/remoteproc/Makefile +++ b/drivers/remoteproc/Makefile @@ -18,6 +18,7 @@ obj-$(CONFIG_QCOM_ADSP_PIL) += qcom_adsp_pil.o obj-$(CONFIG_QCOM_RPROC_COMMON)+= qcom_common.o obj-$(CONFIG_QCOM_Q6V5_COMMON) += qcom_q6v5.o obj-$(CONFIG_QCOM_Q6V5_PIL)+= qcom_q6v5_pil.o +obj-$(CONFIG_QCOM_Q6V5_WCSS) += qcom_q6v5_wcss.o obj-$(CONFIG_QCOM_SYSMON) += qcom_sysmon.o obj-$(CONFIG_QCOM_WCNSS_PIL) += qcom_wcnss_pil.o qcom_wcnss_pil-y += qcom_wcnss.o diff --git a/drivers/remoteproc/qcom_q6v5_wcss.c b/drivers/remoteproc/qcom_q6v5_wcss.c new file mode 100644 index 000..9c5b3b4 --- /dev/null +++ b/drivers/remoteproc/qcom_q6v5_wcss.c @@ -0,0 +1,588 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2016-2018 Linaro Ltd. + * Copyright (C) 2014 Sony Mobile Communications AB + * Copyright (c) 2012-2018, The Linux Foundation. All rights reserved. + */ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "qcom_common.h" +#include "qcom_q6v5.h" + +#define WCSS_CRASH_REASON 421 + +/* QDSP6SS Register Offsets */ +#define QDSP6SS_RESET_REG 0x014 +#define QDSP6SS_GFMUX_CTL_REG 0x020 +#define QDSP6SS_PWR_CTL_REG0x030 +#define QDSP6SS_MEM_PWR_CTL0x0B0 + +/* AXI Halt Register Offsets */ +#define AXI_HALTREQ_REG0x0 +#define AXI_HALTACK_REG0x4 +#define AXI_IDLE_REG 0x8 + +#define HALT_ACK_TIMEOUT_MS100 + +/* QDSP6SS_RESET */ +#define Q6SS_STOP_CORE
[PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver
IPQ8074 has an integrated Hexagon dsp core q6v5 and a wireless lan (Lithium) IP. An mdt type single image format is used for the firmware. So the mdt_load function can be directly used to load the firmware. Also add the relevant resets required for this core. Acked-by: Rob Herring (for bindings) Signed-off-by: Sricharan R [bjorn: Rewrote as a separate driver, intead of extending q6v5_pil.c] Signed-off-by: Bjorn Andersson --- Fixed review comments from Vinod. Retained the reg read/update/write sequence instead of modify for readability In q6v5_wcss_powerdown SSCAON_CONFIG bits 16,17,18 documentation have to be updated. .../devicetree/bindings/remoteproc/qcom,q6v5.txt | 7 +- drivers/remoteproc/Kconfig | 16 + drivers/remoteproc/Makefile| 1 + drivers/remoteproc/qcom_q6v5_wcss.c| 588 + 4 files changed, 611 insertions(+), 1 deletion(-) create mode 100644 drivers/remoteproc/qcom_q6v5_wcss.c diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt index 00d3d58..d52d05e 100644 --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt @@ -11,6 +11,7 @@ on the Qualcomm Hexagon core. "qcom,msm8916-mss-pil", "qcom,msm8974-mss-pil" "qcom,msm8996-mss-pil" + "qcom,ipq8074-wcss-pil" - reg: Usage: required @@ -49,11 +50,15 @@ on the Qualcomm Hexagon core. Usage: required Value type: Definition: reference to the reset-controller for the modem sub-system + reference to the list of 3 reset-controllers for the + wcss sub-system - reset-names: Usage: required Value type: - Definition: must be "mss_restart" + Definition: must be "mss_restart" for the modem sub-system + Definition: must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset" + for the wcss syb-system - cx-supply: - mss-supply: diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig index be76619..7b1a9ef 100644 --- a/drivers/remoteproc/Kconfig +++ b/drivers/remoteproc/Kconfig @@ -123,6 +123,22 @@ config QCOM_Q6V5_PIL Say y here to support the Qualcomm Peripherial Image Loader for the Hexagon V5 based remote processors. +config QCOM_Q6V5_WCSS + tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader" + depends on OF && ARCH_QCOM + depends on QCOM_SMEM + depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n) + depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n + depends on QCOM_SYSMON || QCOM_SYSMON=n + select MFD_SYSCON + select QCOM_MDT_LOADER + select QCOM_Q6V5_COMMON + select QCOM_RPROC_COMMON + select QCOM_SCM + help + Say y here to support the Qualcomm Peripheral Image Loader for the + Hexagon V5 based WCSS remote processors. + config QCOM_SYSMON tristate "Qualcomm sysmon driver" depends on RPMSG diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile index 5dd0249..03332fa 100644 --- a/drivers/remoteproc/Makefile +++ b/drivers/remoteproc/Makefile @@ -18,6 +18,7 @@ obj-$(CONFIG_QCOM_ADSP_PIL) += qcom_adsp_pil.o obj-$(CONFIG_QCOM_RPROC_COMMON)+= qcom_common.o obj-$(CONFIG_QCOM_Q6V5_COMMON) += qcom_q6v5.o obj-$(CONFIG_QCOM_Q6V5_PIL)+= qcom_q6v5_pil.o +obj-$(CONFIG_QCOM_Q6V5_WCSS) += qcom_q6v5_wcss.o obj-$(CONFIG_QCOM_SYSMON) += qcom_sysmon.o obj-$(CONFIG_QCOM_WCNSS_PIL) += qcom_wcnss_pil.o qcom_wcnss_pil-y += qcom_wcnss.o diff --git a/drivers/remoteproc/qcom_q6v5_wcss.c b/drivers/remoteproc/qcom_q6v5_wcss.c new file mode 100644 index 000..9c5b3b4 --- /dev/null +++ b/drivers/remoteproc/qcom_q6v5_wcss.c @@ -0,0 +1,588 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2016-2018 Linaro Ltd. + * Copyright (C) 2014 Sony Mobile Communications AB + * Copyright (c) 2012-2018, The Linux Foundation. All rights reserved. + */ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "qcom_common.h" +#include "qcom_q6v5.h" + +#define WCSS_CRASH_REASON 421 + +/* QDSP6SS Register Offsets */ +#define QDSP6SS_RESET_REG 0x014 +#define QDSP6SS_GFMUX_CTL_REG 0x020 +#define QDSP6SS_PWR_CTL_REG0x030 +#define QDSP6SS_MEM_PWR_CTL0x0B0 + +/* AXI Halt Register Offsets */ +#define AXI_HALTREQ_REG0x0 +#define AXI_HALTACK_REG0x4 +#define AXI_IDLE_REG 0x8 + +#define HALT_ACK_TIMEOUT_MS100 + +/* QDSP6SS_RESET */ +#define Q6SS_STOP_CORE
