| Message ID | 1360868369-20093-2-git-send-email-hdoyu@nvidia.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On 02/14/2013 11:59 AM, Hiroshi Doyu wrote: > To replace magic number in tegra_car: > > - clocks = <&tegra_car 28>; > + clocks = <&tegra_car CLK_HOST1X>; > diff --git a/arch/arm/boot/dts/tegra20-car.h b/arch/arm/boot/dts/tegra20-car.h Sorry, forgot a couple small comments the last time around. This file should probably have some header indicating which binding it describes, rather like the GPIO header in my patch series. > +#define CLK_CPU 0 I'd suggest naming that TEGRA20_CLK_CPU, so that the various different clock headers don't conflict. It's not too likely that more than one of the /Tegra/ clock headers will be included at once, but it doesn't seem that unlikely that a board file could end up having a Tegra clock header included plus various other clock headers for some other chip that has some clock outputs. Oh, and I don't think you updated e.g. nvidia,tegra20-car.txt to remove the list of clocks. BTW, I assume this patch includes the changes from my recent "clk: tegra: fix driver to match DT binding" and anything else similar that's in the most recent Tegra for-next? -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Stephen Warren <swarren@wwwdotorg.org> wrote @ Thu, 14 Feb 2013 21:15:28 +0100: > Oh, and I don't think you updated e.g. nvidia,tegra20-car.txt to remove > the list of clocks. Not yet removed. I think that this could be done with the patch which allows kernel source to include DT header files. > BTW, I assume this patch includes the changes from my recent "clk: > tegra: fix driver to match DT binding" and anything else similar that's > in the most recent Tegra for-next? Of course not, that's a little bit too recent. I'll rerun my script against the laste Tegra for-next and post them as v3(inc. Tegra114) -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/14/2013 01:34 PM, Hiroshi Doyu wrote: > Stephen Warren <swarren@wwwdotorg.org> wrote @ Thu, 14 Feb 2013 21:15:28 +0100: > >> Oh, and I don't think you updated e.g. nvidia,tegra20-car.txt to remove >> the list of clocks. > > Not yet removed. I think that this could be done with the patch which > allows kernel source to include DT header files. By defining the header file, you're really causing that header to be part of the binding definition. I think we should move the code from the binding .txt file to the header, rather than duplicating it and then removing it. The kernel doesn't include the binding .txt file right now, so I don't see any correlation with making it include the header vs. when we remove the list from the binding document. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 14, 2013 at 09:15:28PM +0100, Stephen Warren wrote: > On 02/14/2013 11:59 AM, Hiroshi Doyu wrote: > > To replace magic number in tegra_car: > > > > - clocks = <&tegra_car 28>; > > + clocks = <&tegra_car CLK_HOST1X>; > > > diff --git a/arch/arm/boot/dts/tegra20-car.h b/arch/arm/boot/dts/tegra20-car.h > > Sorry, forgot a couple small comments the last time around. > > This file should probably have some header indicating which binding it > describes, rather like the GPIO header in my patch series. > > > +#define CLK_CPU 0 > > I'd suggest naming that TEGRA20_CLK_CPU, so that the various different > clock headers don't conflict. It's not too likely that more than one of > the /Tegra/ clock headers will be included at once, but it doesn't seem > that unlikely that a board file could end up having a Tegra clock header > included plus various other clock headers for some other chip that has > some clock outputs. > I would suggest removing this clock. It's not actually implemented in the CCF and rather useless. If you would gate the CPU clock from the CPU by writing to this register, how would you ungate it? :) Note that this would gate the clock to all CPUs. Cheers, Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/15/2013 02:24 AM, Peter De Schrijver wrote: > On Thu, Feb 14, 2013 at 09:15:28PM +0100, Stephen Warren wrote: >> On 02/14/2013 11:59 AM, Hiroshi Doyu wrote: >>> To replace magic number in tegra_car: >>> >>> - clocks = <&tegra_car 28>; >>> + clocks = <&tegra_car CLK_HOST1X>; >> >>> diff --git a/arch/arm/boot/dts/tegra20-car.h b/arch/arm/boot/dts/tegra20-car.h >> >> Sorry, forgot a couple small comments the last time around. >> >> This file should probably have some header indicating which binding it >> describes, rather like the GPIO header in my patch series. >> >>> +#define CLK_CPU 0 >> >> I'd suggest naming that TEGRA20_CLK_CPU, so that the various different >> clock headers don't conflict. It's not too likely that more than one of >> the /Tegra/ clock headers will be included at once, but it doesn't seem >> that unlikely that a board file could end up having a Tegra clock header >> included plus various other clock headers for some other chip that has >> some clock outputs. >> > > I would suggest removing this clock. It's not actually implemented in the CCF > and rather useless. If you would gate the CPU clock from the CPU by writing to > this register, how would you ungate it? :) Note that this would gate the clock > to all CPUs. (Note that my comment was re: all clocks, not just that one clock) Can't the PMC or flow-controller ungate the clock based on some event? Either way, that clock definition exists in HW, right? So I don't think there's actually any harm in including the definition in the binding even if we never implement/use it. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 15, 2013 at 05:45:45PM +0100, Stephen Warren wrote: ... > > I would suggest removing this clock. It's not actually implemented in the CCF > > and rather useless. If you would gate the CPU clock from the CPU by writing to > > this register, how would you ungate it? :) Note that this would gate the clock > > to all CPUs. > > (Note that my comment was re: all clocks, not just that one clock) > > Can't the PMC or flow-controller ungate the clock based on some event? I don't think the flow-controller controls this gate. The usual way of clockgating a core is to execute a WFI instruction. That will trigger clockgating the core, unless the flow-controller has been programmed to do something else. The flow-controller will ungate the clock when there is an interrupt. > Either way, that clock definition exists in HW, right? So I don't think > there's actually any harm in including the definition in the binding > even if we never implement/use it. The clock definition seems to exist in HW yes, the corresponding resetbit however is marked as 'reserved' in the Tegra114 documentation. Cheers, Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 21, 2013 at 01:25:36PM +0100, Peter De Schrijver wrote: > On Fri, Feb 15, 2013 at 05:45:45PM +0100, Stephen Warren wrote: > > ... > > > > I would suggest removing this clock. It's not actually implemented in the CCF > > > and rather useless. If you would gate the CPU clock from the CPU by writing to > > > this register, how would you ungate it? :) Note that this would gate the clock > > > to all CPUs. > > > > (Note that my comment was re: all clocks, not just that one clock) > > > > Can't the PMC or flow-controller ungate the clock based on some event? > > I don't think the flow-controller controls this gate. The usual way of > clockgating a core is to execute a WFI instruction. That will trigger > clockgating the core, unless the flow-controller has been programmed to do > something else. The flow-controller will ungate the clock when there is an > interrupt. > > > Either way, that clock definition exists in HW, right? So I don't think > > there's actually any harm in including the definition in the binding > > even if we never implement/use it. > > The clock definition seems to exist in HW yes, the corresponding resetbit > however is marked as 'reserved' in the Tegra114 documentation. Apparently from Tegra114 onwards, this bit no longer exists. In previous chips, it is just turned by the bootloader running on the AVP and stays on from there on. So at least for Tegra114 I think we remove this clock from the DT binding. Cheers, Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/boot/dts/tegra20-car.h b/arch/arm/boot/dts/tegra20-car.h new file mode 100644 index 0000000..0659414 --- /dev/null +++ b/arch/arm/boot/dts/tegra20-car.h @@ -0,0 +1,114 @@ +#define CLK_CPU 0 +#define CLK_AC97 3 +#define CLK_RTC 4 +#define CLK_TIMER 5 +#define CLK_UARTA 6 +#define CLK_GPIO 8 +#define CLK_SDMMC2 9 +#define CLK_I2S1 11 +#define CLK_I2C1 12 +#define CLK_NDFLASH 13 +#define CLK_SDMMC1 14 +#define CLK_SDMMC4 15 +#define CLK_TWC 16 +#define CLK_PWM 17 +#define CLK_I2S2 18 +#define CLK_EPP 19 +#define CLK_GR2D 21 +#define CLK_USBD 22 +#define CLK_ISP 23 +#define CLK_GR3D 24 +#define CLK_IDE 25 +#define CLK_DISP2 26 +#define CLK_DISP1 27 +#define CLK_HOST1X 28 +#define CLK_VCP 29 +#define CLK_CACHE2 31 +#define CLK_MEM 32 +#define CLK_AHBDMA 33 +#define CLK_APBDMA 34 +#define CLK_KBC 36 +#define CLK_STAT_MON 37 +#define CLK_PMC 38 +#define CLK_FUSE 39 +#define CLK_KFUSE 40 +#define CLK_SBC1 41 +#define CLK_NOR 42 +#define CLK_SPI 43 +#define CLK_SBC2 44 +#define CLK_XIO 45 +#define CLK_SBC3 46 +#define CLK_DVC 47 +#define CLK_DSI 48 +#define CLK_MIPI 50 +#define CLK_HDMI 51 +#define CLK_CSI 52 +#define CLK_TVDAC 53 +#define CLK_I2C2 54 +#define CLK_UARTC 55 +#define CLK_EMC 57 +#define CLK_USB2 58 +#define CLK_USB3 59 +#define CLK_MPE 60 +#define CLK_VDE 61 +#define CLK_BSEA 62 +#define CLK_BSEV 63 +#define CLK_SPEEDO 64 +#define CLK_UARTD 65 +#define CLK_UARTE 66 +#define CLK_I2C3 67 +#define CLK_SBC4 68 +#define CLK_SDMMC3 69 +#define CLK_PEX 70 +#define CLK_OWR 71 +#define CLK_AFI 72 +#define CLK_CSITE 73 +#define CLK_PCIE_XCLK 74 +#define CLK_AVPUCQ 75 +#define CLK_LA 76 +#define CLK_IRAMA 84 +#define CLK_IRAMB 85 +#define CLK_IRAMC 86 +#define CLK_IRAMD 87 +#define CLK_CRAM2 88 +#define CLK_AUDIO_2X 89 +#define CLK_CLK_D 90 +#define CLK_CSUS 92 +#define CLK_CDEV1 93 +#define CLK_CDEV2 94 +#define CLK_UARTB 96 +#define CLK_VFIR 97 +#define CLK_SPDIF_IN 98 +#define CLK_SPDIF_OUT 99 +#define CLK_VI 100 +#define CLK_VI_SENSOR 101 +#define CLK_TVO 102 +#define CLK_CVE 103 +#define CLK_OSC 104 +#define CLK_CLK_32K 105 +#define CLK_CLK_M 106 +#define CLK_SCLK 107 +#define CLK_CCLK 108 +#define CLK_HCLK 109 +#define CLK_PCLK 110 +#define CLK_BLINK 111 +#define CLK_PLL_A 112 +#define CLK_PLL_A_OUT0 113 +#define CLK_PLL_C 114 +#define CLK_PLL_C_OUT1 115 +#define CLK_PLL_D 116 +#define CLK_PLL_D_OUT0 117 +#define CLK_PLL_E 118 +#define CLK_PLL_M 119 +#define CLK_PLL_M_OUT1 120 +#define CLK_PLL_P 121 +#define CLK_PLL_P_OUT1 122 +#define CLK_PLL_P_OUT2 123 +#define CLK_PLL_P_OUT3 124 +#define CLK_PLL_P_OUT4 125 +#define CLK_PLL_U 126 +#define CLK_PLL_X 127 +#define CLK_AUDIO 128 +#define CLK_PLL_REF 129 +#define CLK_TWD 130 +#define CLK_CLK_MAX 131
To replace magic number in tegra_car: - clocks = <&tegra_car 28>; + clocks = <&tegra_car CLK_HOST1X>; Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> --- arch/arm/boot/dts/tegra20-car.h | 114 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+) create mode 100644 arch/arm/boot/dts/tegra20-car.h