| Message ID | 1527884768-22392-2-git-send-email-vgarodia@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Vikash, On 1.06.2018 23:26, Vikash Garodia wrote: > Add a new routine to reset the ARM9 and brings it > out of reset. This is in preparation to add PIL > functionality in venus driver. please squash this patch with 4/5. I don't see a reason to add a function which is not used. Shouldn't this produce gcc warnings? > > Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> > --- > drivers/media/platform/qcom/venus/firmware.c | 26 ++++++++++++++++++++++++ > drivers/media/platform/qcom/venus/hfi_venus_io.h | 5 +++++ > 2 files changed, 31 insertions(+) > > diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c > index 521d4b3..7d89b5a 100644 > --- a/drivers/media/platform/qcom/venus/firmware.c > +++ b/drivers/media/platform/qcom/venus/firmware.c > @@ -14,6 +14,7 @@ > > #include <linux/device.h> > #include <linux/firmware.h> > +#include <linux/delay.h> > #include <linux/kernel.h> > #include <linux/io.h> > #include <linux/of.h> > @@ -22,11 +23,36 @@ > #include <linux/sizes.h> > #include <linux/soc/qcom/mdt_loader.h> > > +#include "core.h" > #include "firmware.h" > +#include "hfi_venus_io.h" > > #define VENUS_PAS_ID 9 > #define VENUS_FW_MEM_SIZE (6 * SZ_1M) > > +static void venus_reset_hw(struct venus_core *core) can we rename this to venus_reset_cpu? reset_hw sounds like we reset vcodec IPs, so I think we can be more exact here. > +{ > + void __iomem *reg_base = core->base; just 'base', please. > + > + writel(0, reg_base + WRAPPER_FW_START_ADDR); > + writel(VENUS_FW_MEM_SIZE, reg_base + WRAPPER_FW_END_ADDR); > + writel(0, reg_base + WRAPPER_CPA_START_ADDR); > + writel(VENUS_FW_MEM_SIZE, reg_base + WRAPPER_CPA_END_ADDR); > + writel(0x0, reg_base + WRAPPER_CPU_CGC_DIS); > + writel(0x0, reg_base + WRAPPER_CPU_CLOCK_CONFIG); > + > + /* Make sure all register writes are committed. */ > + mb(); the comment says "register writes" hence shouldn't this be wmb? Also if we are going to have explicit memory barrier why not use writel_relaxed? > + > + /* > + * Need to wait 10 cycles of internal clocks before bringing ARM9 Do we know what is the minimum frequency of the internal ARM9 clocks? I.e does 1us is enough for all venus versions. > + * out of reset. > + */ > + udelay(1); > + > + /* Bring Arm9 out of reset */ ARM9 > + writel_relaxed(0, reg_base + WRAPPER_A9SS_SW_RESET); in my opinion we should have a wmb here too regards, Stan
On 02-06-18, 01:15, Stanimir Varbanov wrote: > Hi Vikash, > > On 1.06.2018 23:26, Vikash Garodia wrote: > > Add a new routine to reset the ARM9 and brings it > > out of reset. This is in preparation to add PIL > > functionality in venus driver. > > please squash this patch with 4/5. I don't see a reason to add a function > which is not used. Shouldn't this produce gcc warnings? Yes this would but in a multi patch series that is okay as subsequent patches would use that and end result in no warning. Splitting logically is good and typical practice in kernel to add the routine followed by usages..
On Tue, Jun 5, 2018 at 7:57 PM Vinod <vkoul@kernel.org> wrote: > > On 02-06-18, 01:15, Stanimir Varbanov wrote: > > Hi Vikash, > > > > On 1.06.2018 23:26, Vikash Garodia wrote: > > > Add a new routine to reset the ARM9 and brings it > > > out of reset. This is in preparation to add PIL > > > functionality in venus driver. > > > > please squash this patch with 4/5. I don't see a reason to add a function > > which is not used. Shouldn't this produce gcc warnings? > > Yes this would but in a multi patch series that is okay as subsequent > patches would use that and end result in no warning. Except during bisect. > > Splitting logically is good and typical practice in kernel to add the > routine followed by usages.. Is it in this case though? If this code was shared across multiple users I could understand but this function is only used locally (and only in one place IIUC). Also the patch is not so big that the code would become confusing if this was squashed into 4/5. I don't see any reason to keep this separate.
On Fri 01 Jun 13:26 PDT 2018, Vikash Garodia wrote: > +static void venus_reset_hw(struct venus_core *core) > +{ > + void __iomem *reg_base = core->base; > + > + writel(0, reg_base + WRAPPER_FW_START_ADDR); > + writel(VENUS_FW_MEM_SIZE, reg_base + WRAPPER_FW_END_ADDR); > + writel(0, reg_base + WRAPPER_CPA_START_ADDR); > + writel(VENUS_FW_MEM_SIZE, reg_base + WRAPPER_CPA_END_ADDR); > + writel(0x0, reg_base + WRAPPER_CPU_CGC_DIS); > + writel(0x0, reg_base + WRAPPER_CPU_CLOCK_CONFIG); > + > + /* Make sure all register writes are committed. */ > + mb(); wmb() doesn't wait until the writes are completed, it simply ensures that any writes before it are performed before any writes after it. If you really want to ensure that these configs has hit the hardware before you sleep, read back the value of the WRAPPER_CPU_CLOCK_CONFIG register. > + > + /* > + * Need to wait 10 cycles of internal clocks before bringing ARM9 > + * out of reset. > + */ > + udelay(1); > + > + /* Bring Arm9 out of reset */ > + writel_relaxed(0, reg_base + WRAPPER_A9SS_SW_RESET); There's no harm in using writel() here... > +} Regards, Bjorn
Hi Stanimir, Thanks for the review. On 2018-06-02 03:45, Stanimir Varbanov wrote: > Hi Vikash, > > On 1.06.2018 23:26, Vikash Garodia wrote: >> Add a new routine to reset the ARM9 and brings it >> out of reset. This is in preparation to add PIL >> functionality in venus driver. > > please squash this patch with 4/5. I don't see a reason to add a > function which is not used. Shouldn't this produce gcc warnings? Will squash the api definition with the patch using it. >> >> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> >> --- >> drivers/media/platform/qcom/venus/firmware.c | 26 >> ++++++++++++++++++++++++ >> drivers/media/platform/qcom/venus/hfi_venus_io.h | 5 +++++ >> 2 files changed, 31 insertions(+) >> >> diff --git a/drivers/media/platform/qcom/venus/firmware.c >> b/drivers/media/platform/qcom/venus/firmware.c >> index 521d4b3..7d89b5a 100644 >> --- a/drivers/media/platform/qcom/venus/firmware.c >> +++ b/drivers/media/platform/qcom/venus/firmware.c >> @@ -14,6 +14,7 @@ >> #include <linux/device.h> >> #include <linux/firmware.h> >> +#include <linux/delay.h> >> #include <linux/kernel.h> >> #include <linux/io.h> >> #include <linux/of.h> >> @@ -22,11 +23,36 @@ >> #include <linux/sizes.h> >> #include <linux/soc/qcom/mdt_loader.h> >> +#include "core.h" >> #include "firmware.h" >> +#include "hfi_venus_io.h" >> #define VENUS_PAS_ID 9 >> #define VENUS_FW_MEM_SIZE (6 * SZ_1M) >> +static void venus_reset_hw(struct venus_core *core) > > can we rename this to venus_reset_cpu? reset_hw sounds like we reset > vcodec IPs, so I think we can be more exact here. > >> +{ >> + void __iomem *reg_base = core->base; > > just 'base', please. Ok. >> + >> + writel(0, reg_base + WRAPPER_FW_START_ADDR); >> + writel(VENUS_FW_MEM_SIZE, reg_base + WRAPPER_FW_END_ADDR); >> + writel(0, reg_base + WRAPPER_CPA_START_ADDR); >> + writel(VENUS_FW_MEM_SIZE, reg_base + WRAPPER_CPA_END_ADDR); >> + writel(0x0, reg_base + WRAPPER_CPU_CGC_DIS); >> + writel(0x0, reg_base + WRAPPER_CPU_CLOCK_CONFIG); >> + >> + /* Make sure all register writes are committed. */ >> + mb(); > > the comment says "register writes" hence shouldn't this be wmb? Also > if we are going to have explicit memory barrier why not use > writel_relaxed? >> + >> + /* >> + * Need to wait 10 cycles of internal clocks before bringing ARM9 > > Do we know what is the minimum frequency of the internal ARM9 clocks? > I.e does 1us is enough for all venus versions. ARM9 is yet not brought out of reset at this point. Sleep might be added to ensure that the register writes are completed. But as Bjorn commented, only way to ensure registers are written is to read them back. I do not have proper justification of sleep as this code was used for many chipset bringup. I can try by removing the sleep and if goes well, we can live without it. >> + * out of reset. >> + */ >> + udelay(1); >> + >> + /* Bring Arm9 out of reset */ > > ARM9 > >> + writel_relaxed(0, reg_base + WRAPPER_A9SS_SW_RESET); > > in my opinion we should have a wmb here too I guess if sleep is removed, we will be left with only the above instruction. so no need to ensure the access order. > regards, > Stan
diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c index 521d4b3..7d89b5a 100644 --- a/drivers/media/platform/qcom/venus/firmware.c +++ b/drivers/media/platform/qcom/venus/firmware.c @@ -14,6 +14,7 @@ #include <linux/device.h> #include <linux/firmware.h> +#include <linux/delay.h> #include <linux/kernel.h> #include <linux/io.h> #include <linux/of.h> @@ -22,11 +23,36 @@ #include <linux/sizes.h> #include <linux/soc/qcom/mdt_loader.h> +#include "core.h" #include "firmware.h" +#include "hfi_venus_io.h" #define VENUS_PAS_ID 9 #define VENUS_FW_MEM_SIZE (6 * SZ_1M) +static void venus_reset_hw(struct venus_core *core) +{ + void __iomem *reg_base = core->base; + + writel(0, reg_base + WRAPPER_FW_START_ADDR); + writel(VENUS_FW_MEM_SIZE, reg_base + WRAPPER_FW_END_ADDR); + writel(0, reg_base + WRAPPER_CPA_START_ADDR); + writel(VENUS_FW_MEM_SIZE, reg_base + WRAPPER_CPA_END_ADDR); + writel(0x0, reg_base + WRAPPER_CPU_CGC_DIS); + writel(0x0, reg_base + WRAPPER_CPU_CLOCK_CONFIG); + + /* Make sure all register writes are committed. */ + mb(); + + /* + * Need to wait 10 cycles of internal clocks before bringing ARM9 + * out of reset. + */ + udelay(1); + + /* Bring Arm9 out of reset */ + writel_relaxed(0, reg_base + WRAPPER_A9SS_SW_RESET); +} int venus_boot(struct device *dev, const char *fwname) { const struct firmware *mdt; diff --git a/drivers/media/platform/qcom/venus/hfi_venus_io.h b/drivers/media/platform/qcom/venus/hfi_venus_io.h index 76f4793..39afa5d 100644 --- a/drivers/media/platform/qcom/venus/hfi_venus_io.h +++ b/drivers/media/platform/qcom/venus/hfi_venus_io.h @@ -109,6 +109,11 @@ #define WRAPPER_CPU_CGC_DIS (WRAPPER_BASE + 0x2010) #define WRAPPER_CPU_STATUS (WRAPPER_BASE + 0x2014) #define WRAPPER_SW_RESET (WRAPPER_BASE + 0x3000) +#define WRAPPER_CPA_START_ADDR (WRAPPER_BASE + 0x1020) +#define WRAPPER_CPA_END_ADDR (WRAPPER_BASE + 0x1024) +#define WRAPPER_FW_START_ADDR (WRAPPER_BASE + 0x1028) +#define WRAPPER_FW_END_ADDR (WRAPPER_BASE + 0x102C) +#define WRAPPER_A9SS_SW_RESET (WRAPPER_BASE + 0x3000) /* Venus 4xx */ #define WRAPPER_VCODEC0_MMCC_POWER_STATUS (WRAPPER_BASE + 0x90)
Add a new routine to reset the ARM9 and brings it out of reset. This is in preparation to add PIL functionality in venus driver. Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org> --- drivers/media/platform/qcom/venus/firmware.c | 26 ++++++++++++++++++++++++ drivers/media/platform/qcom/venus/hfi_venus_io.h | 5 +++++ 2 files changed, 31 insertions(+)