Message ID | 20131231054427.GA22383@S2101-09.ap.freescale.net |
---|---|
State | New |
Headers | show |
Hi, On Tue, Dec 31, 2013 at 01:44:29PM +0800, Shawn Guo wrote: > This pull-request has the following two dependencies: > > - The first pull-request, i.e. [GIT PULL 1/2] ARM: imx: soc changes for 3.14 > > - The pinctrl 'devel' branch below. > > git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel > > Linus Walleij promised that the branch will be stable at least from > the point I pulled into my tree, that is commit 31d610f (pinctrl: > imx1-core populate subdevices). > > Please pull, thanks. > > Shawn > > .../devicetree/bindings/vendor-prefixes.txt | 3 + > arch/arm/boot/dts/imx25-pinfunc.h | 494 +++++++++++ > arch/arm/boot/dts/imx25-pingrp.h | 81 ++ > arch/arm/boot/dts/imx27-pinfunc.h | 526 +++++++++++ > arch/arm/boot/dts/imx27-pingrp.h | 151 ++++ > arch/arm/boot/dts/imx35-pingrp.h | 104 +++ > arch/arm/boot/dts/imx50-pinfunc.h | 923 ++++++++++++++++++++ > arch/arm/boot/dts/imx50-pingrp.h | 146 ++++ > arch/arm/boot/dts/imx51-pingrp.h | 249 ++++++ > arch/arm/boot/dts/imx53-pingrp.h | 352 ++++++++ > arch/arm/boot/dts/imx6dl-pinfunc.h | 2 + > arch/arm/boot/dts/imx6q-pinfunc.h | 2 + > arch/arm/boot/dts/imx6qdl-pingrp.h | 532 +++++++++++ > arch/arm/boot/dts/imx6sl-pingrp.h | 148 ++++ > arch/arm/boot/dts/vf610-pingrp.h | 127 +++ Hm, these don't quite use include files the way include files were originally meant to be used -- initially the idea was to use them to define mostly simple constants instead of full properties like this. I'm not against the idea of using it this way, but I also want to make sure the DT maintainers are OK with it. So I've cc:d them on this reply. I'm also not crazy about the insanely long identifiers used here, but I guess they correlate with some user manual tables? -Olof
Hi Olof, On Thu, Jan 02, 2014 at 12:21:08PM -0800, Olof Johansson wrote: > > .../devicetree/bindings/vendor-prefixes.txt | 3 + > > arch/arm/boot/dts/imx25-pinfunc.h | 494 +++++++++++ > > arch/arm/boot/dts/imx25-pingrp.h | 81 ++ > > arch/arm/boot/dts/imx27-pinfunc.h | 526 +++++++++++ > > arch/arm/boot/dts/imx27-pingrp.h | 151 ++++ > > arch/arm/boot/dts/imx35-pingrp.h | 104 +++ > > arch/arm/boot/dts/imx50-pinfunc.h | 923 ++++++++++++++++++++ > > arch/arm/boot/dts/imx50-pingrp.h | 146 ++++ > > arch/arm/boot/dts/imx51-pingrp.h | 249 ++++++ > > arch/arm/boot/dts/imx53-pingrp.h | 352 ++++++++ > > arch/arm/boot/dts/imx6dl-pinfunc.h | 2 + > > arch/arm/boot/dts/imx6q-pinfunc.h | 2 + > > arch/arm/boot/dts/imx6qdl-pingrp.h | 532 +++++++++++ > > arch/arm/boot/dts/imx6sl-pingrp.h | 148 ++++ > > arch/arm/boot/dts/vf610-pingrp.h | 127 +++ > > Hm, these don't quite use include files the way include files were > originally meant to be used -- initially the idea was to use them to > define mostly simple constants instead of full properties like this. The DT macro support was introduced to improve the readability of device tree sources by replacing those magic numbers with readable macros. I think the usage in imx pinctrl binding perfectly fits the purpose. You can get details of the binding in Documentation/devicetree/bindings/pinctrl/fsl,imx-pinctrl.txt. > > I'm not against the idea of using it this way, but I also want to make sure the > DT maintainers are OK with it. So I've cc:d them on this reply. This is not a new thing. It was firstly adopted for imx6q in v3.10 release with commit e164153 (pinctrl: imx: move hard-coding data into device tree), which had been posted to devicetree list for sure. We're just moving more i.MX SoCs to it. > > I'm also not crazy about the insanely long identifiers used here, but I guess > they correlate with some user manual tables? Yes, the identifiers follows the pad and function names from reference manual. Shawn
On Thu, Jan 2, 2014 at 6:32 PM, Shawn Guo <shawn.guo@linaro.org> wrote: > Hi Olof, > > On Thu, Jan 02, 2014 at 12:21:08PM -0800, Olof Johansson wrote: >> > .../devicetree/bindings/vendor-prefixes.txt | 3 + >> > arch/arm/boot/dts/imx25-pinfunc.h | 494 +++++++++++ >> > arch/arm/boot/dts/imx25-pingrp.h | 81 ++ >> > arch/arm/boot/dts/imx27-pinfunc.h | 526 +++++++++++ >> > arch/arm/boot/dts/imx27-pingrp.h | 151 ++++ >> > arch/arm/boot/dts/imx35-pingrp.h | 104 +++ >> > arch/arm/boot/dts/imx50-pinfunc.h | 923 ++++++++++++++++++++ >> > arch/arm/boot/dts/imx50-pingrp.h | 146 ++++ >> > arch/arm/boot/dts/imx51-pingrp.h | 249 ++++++ >> > arch/arm/boot/dts/imx53-pingrp.h | 352 ++++++++ >> > arch/arm/boot/dts/imx6dl-pinfunc.h | 2 + >> > arch/arm/boot/dts/imx6q-pinfunc.h | 2 + >> > arch/arm/boot/dts/imx6qdl-pingrp.h | 532 +++++++++++ >> > arch/arm/boot/dts/imx6sl-pingrp.h | 148 ++++ >> > arch/arm/boot/dts/vf610-pingrp.h | 127 +++ >> >> Hm, these don't quite use include files the way include files were >> originally meant to be used -- initially the idea was to use them to >> define mostly simple constants instead of full properties like this. > > The DT macro support was introduced to improve the readability of device > tree sources by replacing those magic numbers with readable macros. I > think the usage in imx pinctrl binding perfectly fits the purpose. You > can get details of the binding in > Documentation/devicetree/bindings/pinctrl/fsl,imx-pinctrl.txt. To be honest I didn't follow that discussion closely. If DT maintainers are OK with that style, then I'm OK. :) >> I'm not against the idea of using it this way, but I also want to make sure the >> DT maintainers are OK with it. So I've cc:d them on this reply. > > This is not a new thing. It was firstly adopted for imx6q in v3.10 > release with commit e164153 (pinctrl: imx: move hard-coding data into > device tree), which had been posted to devicetree list for sure. We're > just moving more i.MX SoCs to it. Ok, then it's probably just the location of the header files that should be adjusted. Other subsystems have placed them under include/dt-bindings/<subsystem>, so that's likely a better place for these as well, don't you think? >> I'm also not crazy about the insanely long identifiers used here, but I guess >> they correlate with some user manual tables? > > Yes, the identifiers follows the pad and function names from reference > manual. Ok, fair enough. -Olof
On Thu, Jan 02, 2014 at 06:41:30PM -0800, Olof Johansson wrote: > Ok, then it's probably just the location of the header files that > should be adjusted. Other subsystems have placed them under > include/dt-bindings/<subsystem>, so that's likely a better place for > these as well, don't you think? I had a little discussion with DT people when the headers were firstly created. These pinctrl headers are a little different from the headers in include/dt-bindings/<subsystem>. The latter are used by both kernel and device tree sources, while the pinctrl headers are used by device tree sources only, so I chose to put them just in the same folder as dts files. And DT people are fine with my take. Shawn
On Thu, Jan 2, 2014 at 7:04 PM, Shawn Guo <shawn.guo@linaro.org> wrote: > On Thu, Jan 02, 2014 at 06:41:30PM -0800, Olof Johansson wrote: >> Ok, then it's probably just the location of the header files that >> should be adjusted. Other subsystems have placed them under >> include/dt-bindings/<subsystem>, so that's likely a better place for >> these as well, don't you think? > > I had a little discussion with DT people when the headers were firstly > created. These pinctrl headers are a little different from the headers > in include/dt-bindings/<subsystem>. The latter are used by both kernel > and device tree sources, while the pinctrl headers are used by device > tree sources only, so I chose to put them just in the same folder as > dts files. And DT people are fine with my take. Since you don't provide references to this I had to go searching for it. All I find is some discussion from 8 months ago, and quite a bit of that seems to have been about changing bindings, and some about the preprocessor behavior. Also, the patches seem to have been too big to make it out on the lists. I'd like a fresh look from DT people on this just to make sure no opinions have changed -- lots of things have changed in the last 8 months w.r.t. DT. I've pinged them on IRC, let's see if they wake up. Adding explicit cc too. -Olof
On Fri, Jan 03, 2014 at 11:29:35AM -0800, Olof Johansson wrote: > On Thu, Jan 2, 2014 at 7:04 PM, Shawn Guo <shawn.guo@linaro.org> wrote: > > On Thu, Jan 02, 2014 at 06:41:30PM -0800, Olof Johansson wrote: > >> Ok, then it's probably just the location of the header files that > >> should be adjusted. Other subsystems have placed them under > >> include/dt-bindings/<subsystem>, so that's likely a better place for > >> these as well, don't you think? > > > > I had a little discussion with DT people when the headers were firstly > > created. These pinctrl headers are a little different from the headers > > in include/dt-bindings/<subsystem>. The latter are used by both kernel > > and device tree sources, while the pinctrl headers are used by device > > tree sources only, so I chose to put them just in the same folder as > > dts files. And DT people are fine with my take. > > Since you don't provide references to this I had to go searching for > it. All I find is some discussion from 8 months ago, and quite a bit > of that seems to have been about changing bindings, and some about the > preprocessor behavior. Also, the patches seem to have been too big to > make it out on the lists. > > I'd like a fresh look from DT people on this just to make sure no > opinions have changed -- lots of things have changed in the last 8 > months w.r.t. DT. Indeed, it's been quite a long time. Let me restate my point. The include/dt-bindings is introduced as a folder to hold headers that are referenced by both kernel and DTS. That's why we create the folder in the kernel include folder and have arch/arm/boot/dts/include/dt-bindings being a symbol link to it. All the headers in there need to be duplicated between kernel and DTS tree, when we move DTS files into a separated repository. Putting DTS local headers into the folder is absolutely unnecessary, and will only confuse people and bother ourselves when moving DTS files out of kernel tree. Shawn > > I've pinged them on IRC, let's see if they wake up. Adding explicit cc too. > > > -Olof
On Sat, Jan 04, 2014 at 09:10:58AM +0800, Shawn Guo wrote: > On Fri, Jan 03, 2014 at 11:29:35AM -0800, Olof Johansson wrote: > > On Thu, Jan 2, 2014 at 7:04 PM, Shawn Guo <shawn.guo@linaro.org> wrote: > > > On Thu, Jan 02, 2014 at 06:41:30PM -0800, Olof Johansson wrote: > > >> Ok, then it's probably just the location of the header files that > > >> should be adjusted. Other subsystems have placed them under > > >> include/dt-bindings/<subsystem>, so that's likely a better place for > > >> these as well, don't you think? > > > > > > I had a little discussion with DT people when the headers were firstly > > > created. These pinctrl headers are a little different from the headers > > > in include/dt-bindings/<subsystem>. The latter are used by both kernel > > > and device tree sources, while the pinctrl headers are used by device > > > tree sources only, so I chose to put them just in the same folder as > > > dts files. And DT people are fine with my take. > > > > Since you don't provide references to this I had to go searching for > > it. All I find is some discussion from 8 months ago, and quite a bit > > of that seems to have been about changing bindings, and some about the > > preprocessor behavior. Also, the patches seem to have been too big to > > make it out on the lists. > > > > I'd like a fresh look from DT people on this just to make sure no > > opinions have changed -- lots of things have changed in the last 8 > > months w.r.t. DT. > > Indeed, it's been quite a long time. Let me restate my point. The > include/dt-bindings is introduced as a folder to hold headers that are > referenced by both kernel and DTS. That's why we create the folder in > the kernel include folder and have arch/arm/boot/dts/include/dt-bindings > being a symbol link to it. All the headers in there need to be > duplicated between kernel and DTS tree, when we move DTS files into > a separated repository. Putting DTS local headers into the folder is > absolutely unnecessary, and will only confuse people and bother > ourselves when moving DTS files out of kernel tree. Just a gentle ping to ensure we do not get the pull request lost. Or do you have any further comment? Shawn
On Thu, Jan 9, 2014 at 6:41 PM, Shawn Guo <shawn.guo@linaro.org> wrote: > On Sat, Jan 04, 2014 at 09:10:58AM +0800, Shawn Guo wrote: >> On Fri, Jan 03, 2014 at 11:29:35AM -0800, Olof Johansson wrote: >> > On Thu, Jan 2, 2014 at 7:04 PM, Shawn Guo <shawn.guo@linaro.org> wrote: >> > > On Thu, Jan 02, 2014 at 06:41:30PM -0800, Olof Johansson wrote: >> > >> Ok, then it's probably just the location of the header files that >> > >> should be adjusted. Other subsystems have placed them under >> > >> include/dt-bindings/<subsystem>, so that's likely a better place for >> > >> these as well, don't you think? >> > > >> > > I had a little discussion with DT people when the headers were firstly >> > > created. These pinctrl headers are a little different from the headers >> > > in include/dt-bindings/<subsystem>. The latter are used by both kernel >> > > and device tree sources, while the pinctrl headers are used by device >> > > tree sources only, so I chose to put them just in the same folder as >> > > dts files. And DT people are fine with my take. >> > >> > Since you don't provide references to this I had to go searching for >> > it. All I find is some discussion from 8 months ago, and quite a bit >> > of that seems to have been about changing bindings, and some about the >> > preprocessor behavior. Also, the patches seem to have been too big to >> > make it out on the lists. >> > >> > I'd like a fresh look from DT people on this just to make sure no >> > opinions have changed -- lots of things have changed in the last 8 >> > months w.r.t. DT. >> >> Indeed, it's been quite a long time. Let me restate my point. The >> include/dt-bindings is introduced as a folder to hold headers that are >> referenced by both kernel and DTS. That's why we create the folder in >> the kernel include folder and have arch/arm/boot/dts/include/dt-bindings >> being a symbol link to it. All the headers in there need to be >> duplicated between kernel and DTS tree, when we move DTS files into >> a separated repository. Putting DTS local headers into the folder is >> absolutely unnecessary, and will only confuse people and bother >> ourselves when moving DTS files out of kernel tree. > > Just a gentle ping to ensure we do not get the pull request lost. Or do > you have any further comment? Still waiting on DT maintainers to chime in. -Olof
Hi, On 10.01.2014 03:41, Olof Johansson wrote: > On Thu, Jan 9, 2014 at 6:41 PM, Shawn Guo <shawn.guo@linaro.org> wrote: >> On Sat, Jan 04, 2014 at 09:10:58AM +0800, Shawn Guo wrote: >>> On Fri, Jan 03, 2014 at 11:29:35AM -0800, Olof Johansson wrote: >>>> On Thu, Jan 2, 2014 at 7:04 PM, Shawn Guo <shawn.guo@linaro.org> wrote: >>>>> On Thu, Jan 02, 2014 at 06:41:30PM -0800, Olof Johansson wrote: >>>>>> Ok, then it's probably just the location of the header files that >>>>>> should be adjusted. Other subsystems have placed them under >>>>>> include/dt-bindings/<subsystem>, so that's likely a better place for >>>>>> these as well, don't you think? >>>>> >>>>> I had a little discussion with DT people when the headers were firstly >>>>> created. These pinctrl headers are a little different from the headers >>>>> in include/dt-bindings/<subsystem>. The latter are used by both kernel >>>>> and device tree sources, while the pinctrl headers are used by device >>>>> tree sources only, so I chose to put them just in the same folder as >>>>> dts files. And DT people are fine with my take. >>>> >>>> Since you don't provide references to this I had to go searching for >>>> it. All I find is some discussion from 8 months ago, and quite a bit >>>> of that seems to have been about changing bindings, and some about the >>>> preprocessor behavior. Also, the patches seem to have been too big to >>>> make it out on the lists. >>>> >>>> I'd like a fresh look from DT people on this just to make sure no >>>> opinions have changed -- lots of things have changed in the last 8 >>>> months w.r.t. DT. >>> >>> Indeed, it's been quite a long time. Let me restate my point. The >>> include/dt-bindings is introduced as a folder to hold headers that are >>> referenced by both kernel and DTS. That's why we create the folder in >>> the kernel include folder and have arch/arm/boot/dts/include/dt-bindings >>> being a symbol link to it. All the headers in there need to be >>> duplicated between kernel and DTS tree, when we move DTS files into >>> a separated repository. Putting DTS local headers into the folder is >>> absolutely unnecessary, and will only confuse people and bother >>> ourselves when moving DTS files out of kernel tree. >> >> Just a gentle ping to ensure we do not get the pull request lost. Or do >> you have any further comment? > > Still waiting on DT maintainers to chime in. I'm not officially a DT maintainer, but let me share my thoughts on this. So what options we have for this: 1) include/dt-bindings - this directory is designed to contain headers that define the ABI between firmware and kernel code, in other words - DT bindings. 2) arch/*/boot/dts - we already have files that can be included by other files in this directory, i.e. *.dtsi. Some are used to implement hierarchies of devices (e.g. s3c64xx.dtsi), but some are purely used as headers (s3c64xx-pinctrl.dtsi). 3) include/dts? - I'm not sure if this have any benefits, but I'm listing it since it's one of the ideas that came to my mind. Now 2) seems to be already used and doesn't seem to generate any problems, so I'm all for it. Still, there is one more issue. For files to be included merely by DTS files and so limited by DTS+CPP syntax, I don't think it's too good idea to call them *.h. Let's stay with *.dtsi, since that's the file extension supposed to be used for files that can be included from device tree sources. Best regards, Tomasz
On Fri, Jan 10, 2014 at 7:28 AM, Tomasz Figa <t.figa@samsung.com> wrote: > Hi, > > On 10.01.2014 03:41, Olof Johansson wrote: >> >> On Thu, Jan 9, 2014 at 6:41 PM, Shawn Guo <shawn.guo@linaro.org> wrote: >>> >>> On Sat, Jan 04, 2014 at 09:10:58AM +0800, Shawn Guo wrote: >>>> >>>> On Fri, Jan 03, 2014 at 11:29:35AM -0800, Olof Johansson wrote: >>>>> >>>>> On Thu, Jan 2, 2014 at 7:04 PM, Shawn Guo <shawn.guo@linaro.org> wrote: >>>>>> >>>>>> On Thu, Jan 02, 2014 at 06:41:30PM -0800, Olof Johansson wrote: >>>>>>> >>>>>>> Ok, then it's probably just the location of the header files that >>>>>>> should be adjusted. Other subsystems have placed them under >>>>>>> include/dt-bindings/<subsystem>, so that's likely a better place for >>>>>>> these as well, don't you think? >>>>>> >>>>>> >>>>>> I had a little discussion with DT people when the headers were firstly >>>>>> created. These pinctrl headers are a little different from the >>>>>> headers >>>>>> in include/dt-bindings/<subsystem>. The latter are used by both >>>>>> kernel >>>>>> and device tree sources, while the pinctrl headers are used by device >>>>>> tree sources only, so I chose to put them just in the same folder as >>>>>> dts files. And DT people are fine with my take. >>>>> >>>>> >>>>> Since you don't provide references to this I had to go searching for >>>>> it. All I find is some discussion from 8 months ago, and quite a bit >>>>> of that seems to have been about changing bindings, and some about the >>>>> preprocessor behavior. Also, the patches seem to have been too big to >>>>> make it out on the lists. >>>>> >>>>> I'd like a fresh look from DT people on this just to make sure no >>>>> opinions have changed -- lots of things have changed in the last 8 >>>>> months w.r.t. DT. >>>> >>>> >>>> Indeed, it's been quite a long time. Let me restate my point. The >>>> include/dt-bindings is introduced as a folder to hold headers that are >>>> referenced by both kernel and DTS. That's why we create the folder in >>>> the kernel include folder and have arch/arm/boot/dts/include/dt-bindings >>>> being a symbol link to it. All the headers in there need to be >>>> duplicated between kernel and DTS tree, when we move DTS files into >>>> a separated repository. Putting DTS local headers into the folder is >>>> absolutely unnecessary, and will only confuse people and bother >>>> ourselves when moving DTS files out of kernel tree. >>> >>> >>> Just a gentle ping to ensure we do not get the pull request lost. Or do >>> you have any further comment? >> >> >> Still waiting on DT maintainers to chime in. It helps (slightly) to be copied directly. > I'm not officially a DT maintainer, but let me share my thoughts on this. We can fix that... :) > So what options we have for this: > > 1) include/dt-bindings - this directory is designed to contain headers > that define the ABI between firmware and kernel code, in other > words - DT bindings. > > 2) arch/*/boot/dts - we already have files that can be included by > other files in this directory, i.e. *.dtsi. Some are used to > implement hierarchies of devices (e.g. s3c64xx.dtsi), but some are > purely used as headers (s3c64xx-pinctrl.dtsi). > > 3) include/dts? - I'm not sure if this have any benefits, but I'm > listing it since it's one of the ideas that came to my mind. > > Now 2) seems to be already used and doesn't seem to generate any problems, > so I'm all for it. Still, there is one more issue. For files to be included > merely by DTS files and so limited by DTS+CPP syntax, I don't think it's too > good idea to call them *.h. Let's stay with *.dtsi, since that's the file > extension supposed to be used for files that can be included from device > tree sources. I think having 1 and 2 is the right way. Minimizing the number of shared headers between dts and the kernel is a good thing. As for *.h vs. *.dtsi naming, if the include is only pre-processor defines, then I think using .h is the right way. Otherwise, if there is any dts syntax, then .dtsi is the right name. It looks to me like this style has been followed in both the imx and s3c64xx cases. On a side note, I'm not really a fan of this pattern: #define FOO1 1 2 3 #define BAR FOO1 FOO2 FOO3 While it definitely simplifies the dts files, it would never be used in C and obfuscates what the actual property size is. Reading a dts file, I would naturally assume the define was a single value while in fact it could expand to a very large property size. Rob
On Fri, Jan 10, 2014 at 09:30 -0600, Rob Herring wrote: > > As for *.h vs. *.dtsi naming, if the include is only pre-processor > defines, then I think using .h is the right way. Otherwise, if there > is any dts syntax, then .dtsi is the right name. It looks to me like > this style has been followed in both the imx and s3c64xx cases. > > On a side note, I'm not really a fan of this pattern: > > #define FOO1 1 2 3 > > #define BAR FOO1 FOO2 FOO3 > > While it definitely simplifies the dts files, it would never be used > in C and obfuscates what the actual property size is. Reading a dts > file, I would naturally assume the define was a single value while in > fact it could expand to a very large property size. For more complex yet tedious repetitive cases like GPIO banks and pins I've seen #define directives which introduce "functions" (macros with parameters). They appear to be rather useful, can reflect very well the essence of the information, don't necessarily pretend to be single values, but still may hide how many cells they expand to. For example: arch/arm/boot/dts/tegra30-cardhu.dtsi: interrupts = <TEGRA_GPIO(L, 0) IRQ_TYPE_LEVEL_HIGH>; virtually yours Gerhard Sittig
On Fri, Jan 10, 2014 at 7:30 AM, Rob Herring <robherring2@gmail.com> wrote: > On Fri, Jan 10, 2014 at 7:28 AM, Tomasz Figa <t.figa@samsung.com> wrote: >> Hi, >> >> On 10.01.2014 03:41, Olof Johansson wrote: >>> >>> On Thu, Jan 9, 2014 at 6:41 PM, Shawn Guo <shawn.guo@linaro.org> wrote: >>>> >>>> On Sat, Jan 04, 2014 at 09:10:58AM +0800, Shawn Guo wrote: >>>>> >>>>> On Fri, Jan 03, 2014 at 11:29:35AM -0800, Olof Johansson wrote: >>>>>> >>>>>> On Thu, Jan 2, 2014 at 7:04 PM, Shawn Guo <shawn.guo@linaro.org> wrote: >>>>>>> >>>>>>> On Thu, Jan 02, 2014 at 06:41:30PM -0800, Olof Johansson wrote: >>>>>>>> >>>>>>>> Ok, then it's probably just the location of the header files that >>>>>>>> should be adjusted. Other subsystems have placed them under >>>>>>>> include/dt-bindings/<subsystem>, so that's likely a better place for >>>>>>>> these as well, don't you think? >>>>>>> >>>>>>> >>>>>>> I had a little discussion with DT people when the headers were firstly >>>>>>> created. These pinctrl headers are a little different from the >>>>>>> headers >>>>>>> in include/dt-bindings/<subsystem>. The latter are used by both >>>>>>> kernel >>>>>>> and device tree sources, while the pinctrl headers are used by device >>>>>>> tree sources only, so I chose to put them just in the same folder as >>>>>>> dts files. And DT people are fine with my take. >>>>>> >>>>>> >>>>>> Since you don't provide references to this I had to go searching for >>>>>> it. All I find is some discussion from 8 months ago, and quite a bit >>>>>> of that seems to have been about changing bindings, and some about the >>>>>> preprocessor behavior. Also, the patches seem to have been too big to >>>>>> make it out on the lists. >>>>>> >>>>>> I'd like a fresh look from DT people on this just to make sure no >>>>>> opinions have changed -- lots of things have changed in the last 8 >>>>>> months w.r.t. DT. >>>>> >>>>> >>>>> Indeed, it's been quite a long time. Let me restate my point. The >>>>> include/dt-bindings is introduced as a folder to hold headers that are >>>>> referenced by both kernel and DTS. That's why we create the folder in >>>>> the kernel include folder and have arch/arm/boot/dts/include/dt-bindings >>>>> being a symbol link to it. All the headers in there need to be >>>>> duplicated between kernel and DTS tree, when we move DTS files into >>>>> a separated repository. Putting DTS local headers into the folder is >>>>> absolutely unnecessary, and will only confuse people and bother >>>>> ourselves when moving DTS files out of kernel tree. >>>> >>>> >>>> Just a gentle ping to ensure we do not get the pull request lost. Or do >>>> you have any further comment? >>> >>> >>> Still waiting on DT maintainers to chime in. > > It helps (slightly) to be copied directly. I should have copied you on some of the thread, but this was also right around when your calxeda address was going away so I might have missed you. > >> I'm not officially a DT maintainer, but let me share my thoughts on this. > > We can fix that... :) > >> So what options we have for this: >> >> 1) include/dt-bindings - this directory is designed to contain headers >> that define the ABI between firmware and kernel code, in other >> words - DT bindings. >> >> 2) arch/*/boot/dts - we already have files that can be included by >> other files in this directory, i.e. *.dtsi. Some are used to >> implement hierarchies of devices (e.g. s3c64xx.dtsi), but some are >> purely used as headers (s3c64xx-pinctrl.dtsi). >> >> 3) include/dts? - I'm not sure if this have any benefits, but I'm >> listing it since it's one of the ideas that came to my mind. >> >> Now 2) seems to be already used and doesn't seem to generate any problems, >> so I'm all for it. Still, there is one more issue. For files to be included >> merely by DTS files and so limited by DTS+CPP syntax, I don't think it's too >> good idea to call them *.h. Let's stay with *.dtsi, since that's the file >> extension supposed to be used for files that can be included from device >> tree sources. > > I think having 1 and 2 is the right way. Minimizing the number of > shared headers between dts and the kernel is a good thing. > > As for *.h vs. *.dtsi naming, if the include is only pre-processor > defines, then I think using .h is the right way. Otherwise, if there > is any dts syntax, then .dtsi is the right name. It looks to me like > this style has been followed in both the imx and s3c64xx cases. I'm mostly reacting to the use of .h for something that isn't C preprocessing. Since dtc/dts has their own namespace for everything else, it seems appropriate to have something unique. But this is pretty far up bikeshed alley. > On a side note, I'm not really a fan of this pattern: > > #define FOO1 1 2 3 > > #define BAR FOO1 FOO2 FOO3 > > While it definitely simplifies the dts files, it would never be used > in C and obfuscates what the actual property size is. Reading a dts > file, I would naturally assume the define was a single value while in > fact it could expand to a very large property size. Hmm. Shawn? -Olof
On Fri, Jan 10, 2014 at 10:37:01AM -0800, Olof Johansson wrote: > > On a side note, I'm not really a fan of this pattern: > > > > #define FOO1 1 2 3 > > > > #define BAR FOO1 FOO2 FOO3 > > > > While it definitely simplifies the dts files, it would never be used > > in C and obfuscates what the actual property size is. Reading a dts > > file, I would naturally assume the define was a single value while in > > fact it could expand to a very large property size. > > Hmm. Shawn? What I read from Rob is that he does not like it but he does not hate it so much either. And that's probably why I did not get either ACK or NACK from him when the usage was firstly introduced :) While I agree such macro usage should be avoided or limited, it really helps IMX pinctrl case to improve the readability and make the handwriting of these pinctrl entries less error prone. Isn't it the whole point of introducing macro support for DTC? Shawn
On Saturday 11 January 2014, Shawn Guo wrote: > While I agree such macro usage should be avoided or limited, it really > helps IMX pinctrl case to improve the readability and make the handwriting > of these pinctrl entries less error prone. Isn't it the whole point of > introducing macro support for DTC? The macros are useful for all cases where the binding defines an arbitrary number space, e.g. for the interrupt flags or when a clock or pin controller provides a set of pins/clocks that don't already have a number assigned to them. In this case, the header file can be shared between the dts and kernel source files to ensure that they are talking about the same things. However, I don't like to see uses of such macros where the definition is done to match a hardware constant (bit number, register index, etc), like in the recently posted eDMA driver case. Here the macro /reduces/ readability of the dts source IMHO, because you cannot easily check the dts file against the data sheet without cross-referencing the header file as well. Arnd
On Sat, Jan 11, 2014 at 02:15:28PM +0100, Arnd Bergmann wrote: > On Saturday 11 January 2014, Shawn Guo wrote: > > While I agree such macro usage should be avoided or limited, it really > > helps IMX pinctrl case to improve the readability and make the handwriting > > of these pinctrl entries less error prone. Isn't it the whole point of > > introducing macro support for DTC? > > The macros are useful for all cases where the binding defines an > arbitrary number space, e.g. for the interrupt flags or when a > clock or pin controller provides a set of pins/clocks that don't > already have a number assigned to them. In this case, the header > file can be shared between the dts and kernel source files to ensure > that they are talking about the same things. > > However, I don't like to see uses of such macros where the definition > is done to match a hardware constant (bit number, register index, etc), > like in the recently posted eDMA driver case. Here the macro /reduces/ > readability of the dts source IMHO, because you cannot easily check > the dts file against the data sheet without cross-referencing the > header file as well. I agree with you that we shouldn't use macros for IP block resources like irq number and dma channel. Once we code these SoC specific data/number in <soc>.dtsi, all that board level dts files need to do is to include <soc>.dtsi, nothing else. However, pinctrl data is a different case. Which pin is in use, which mux option is selected for the pin, how the pin should be configured, all these are board specific. So these pinctrl configuration should be defined by individual board level dts file rather than <soc>.dtsi. Let's say the mux option SPDIF_OUT of pin GPIO_17 is used on a few board designs, hummingboard, nitrogen6x and wandboard. If we do not use macro, every author of these board dts files need to look into reference manual to find out the following numbers - The mux register offset of pin GPIO_17 - The select input register offset of pin GPIO_17 - The config register offset of pin GPIO_17 - The value of SPDIF_OUT mux option for GPIO_17 - The select input value of SPDIF_OUT mux option for GPIO_17 , and then code these numbers in their <board>.dts by hand. It's boring and error prone. As a comparison, we generate these numbers from reference manual database using a tool and define a macro MX6QDL_PAD_GPIO_17__SPDIF_OUT for these numbers. The authors only need to find out this macro from imx6q-pinfunc.dtsi and fill it into his dts. Isn't the macro use here helping to ease everyone's life and make the coding less error prone, since the macro is generated from database? Shawn
On Sunday 12 January 2014, Shawn Guo wrote: > Let's say the mux option SPDIF_OUT of pin GPIO_17 is used on a few board > designs, hummingboard, nitrogen6x and wandboard. If we do not use > macro, every author of these board dts files need to look into reference > manual to find out the following numbers > > - The mux register offset of pin GPIO_17 > - The select input register offset of pin GPIO_17 > - The config register offset of pin GPIO_17 > - The value of SPDIF_OUT mux option for GPIO_17 > - The select input value of SPDIF_OUT mux option for GPIO_17 > > , and then code these numbers in their <board>.dts by hand. It's > boring and error prone. As a comparison, we generate these numbers > from reference manual database using a tool and define a macro > MX6QDL_PAD_GPIO_17__SPDIF_OUT for these numbers. The authors only need > to find out this macro from imx6q-pinfunc.dtsi and fill it into his dts. > Isn't the macro use here helping to ease everyone's life and make the > coding less error prone, since the macro is generated from database? I was under the impression that the generic pinctrl binding was designed in a way to let you assign labels to each possible (reasonable) configuration so you didn't have get to this level of detail at the driver. I'm also surprised that you have to know multiple constants (mux register, input register, config register offsets) to select a particular pin. I would have expected that you could have one constant from which the driver is able to compute the other ones. If you actually need the complexity that you describe in the board files, the macros don't seem worse than the alternative, I would just be happier if the pinctrl driver binding could do without them. Arnd
On Sun, Jan 12, 2014 at 9:21 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Sunday 12 January 2014, Shawn Guo wrote: >> , and then code these numbers in their <board>.dts by hand. It's >> boring and error prone. As a comparison, we generate these numbers >> from reference manual database using a tool and define a macro >> MX6QDL_PAD_GPIO_17__SPDIF_OUT for these numbers. The authors only need >> to find out this macro from imx6q-pinfunc.dtsi and fill it into his dts. >> Isn't the macro use here helping to ease everyone's life and make the >> coding less error prone, since the macro is generated from database? > > I was under the impression that the generic pinctrl binding was designed > in a way to let you assign labels to each possible (reasonable) > configuration so you didn't have get to this level of detail at the > driver. There is a binding for handles and states tied to a device. There is no generic pinctrl binding for the definition of groups and functions and how they map, i.e. muxing. The discussion around this ended up with everyone disagreeing and eventually everybody started writing their own bindings and have them merged. There is a generic pin config binding, not everyone uses it, as they defined their bindings before I got that in place. It is mostly a requirement for newer drivers. This Freescale driver does not use the generic binding but instead it's homebrew thing. The reason being that there was no generic binding around when it was defined and written. It is possible to migrate it to generic pinconf, which would be nice. > I'm also surprised that you have to know multiple constants > (mux register, input register, config register offsets) to select a > particular pin. I would have expected that you could have one constant > from which the driver is able to compute the other ones. I might have merged this but I was younger then :-/ Now I don't like the looks of this. I think the DT typically shall not store register offsets and bitfield information. I think doing so is taking away the responsibility of the driver knowing the hardware and moving too close to Open Firmware. pinctrl-single does store such things, the reason being that the OMAP people did not even have a datasheet for these regs, all they get is datafiles from the ASIC engineers, so there is nothing sensible to encode in the driver in C. But I'm reluctant also about this driver. Yours, Linus Walleij
On Sun, Jan 12, 2014 at 09:21:19PM +0100, Arnd Bergmann wrote: > I was under the impression that the generic pinctrl binding was designed > in a way to let you assign labels to each possible (reasonable) > configuration so you didn't have get to this level of detail at the > driver. The generic part of pinctrl binding only covers the procedure how a client device get its pinctrl state configuration from a pin controller node. That's what we defined in bindings/pinctrl/pinctrl-bindings.txt and implemented in pinctrl core. But the details of how a pinctrl state configuration should be interpreted for a particular pin controller is defined by individual pin controller binding like fsl,imx-pinctrl.txt, and implemented in the pin controller driver like drivers/pinctrl/pinctrl-imx.c. > I'm also surprised that you have to know multiple constants > (mux register, input register, config register offsets) to select a > particular pin. I would have expected that you could have one constant > from which the driver is able to compute the other ones. That's what we do before. We define a constant in the binding and have the driver to maintain these data and look up the data using the constant. See commit below for imx6q example. d8fe357 pinctrl: pinctrl-imx: add imx6q pinctrl driver The biggest problem with that approach is we have huge data to maintain in the driver because of the complexity and flexibility of IMX pin controller design. And these data can not be init data. Check that big array of struct imx_pin_reg in commit above for what I'm talking about. So when we build a v7 kernel image for IMX, we will have such big array for each of these SoCs, imx50, imx51, imx53, imx6sl, imx6dl, imx6q, and more to come. That's why we went through the pain of breaking DT compatibility to move all these data into device tree one year ago with the commit below. e164153 pinctrl: imx: move hard-coding data into device tree Now kernel gets released from the floating and we do not even need to touch kernel to add these data when new SoC support is added. Shawn
On Mon, Jan 13, 2014 at 12:16:10AM +0100, Linus Walleij wrote: > > I'm also surprised that you have to know multiple constants > > (mux register, input register, config register offsets) to select a > > particular pin. I would have expected that you could have one constant > > from which the driver is able to compute the other ones. > > I might have merged this but I was younger then :-/ > > Now I don't like the looks of this. I think the DT typically shall not > store register offsets and bitfield information. I think doing > so is taking away the responsibility of the driver knowing the > hardware and moving too close to Open Firmware. We only have data in device tree, and drivers, at least imx pinctrl driver, still need to know hardware for how to apply these data into hardware. As I just replied to Arnd, all the motivation of having these data in device tree is to release kernel from being bloated by those huge mount of data due to the complexity and flexibility of IMX pin controller. Shawn
On 01/10/2014 10:03 AM, Gerhard Sittig wrote: > On Fri, Jan 10, 2014 at 09:30 -0600, Rob Herring wrote: >> >> As for *.h vs. *.dtsi naming, if the include is only pre-processor >> defines, then I think using .h is the right way. Otherwise, if there >> is any dts syntax, then .dtsi is the right name. It looks to me like >> this style has been followed in both the imx and s3c64xx cases. >> >> On a side note, I'm not really a fan of this pattern: >> >> #define FOO1 1 2 3 >> >> #define BAR FOO1 FOO2 FOO3 >> >> While it definitely simplifies the dts files, it would never be used >> in C and obfuscates what the actual property size is. Reading a dts >> file, I would naturally assume the define was a single value while in >> fact it could expand to a very large property size. > > For more complex yet tedious repetitive cases like GPIO banks and > pins I've seen #define directives which introduce "functions" > (macros with parameters). They appear to be rather useful, can > reflect very well the essence of the information, don't > necessarily pretend to be single values, but still may hide how > many cells they expand to. For example: > > arch/arm/boot/dts/tegra30-cardhu.dtsi: > interrupts = <TEGRA_GPIO(L, 0) IRQ_TYPE_LEVEL_HIGH>; I'm not sure I entirely understand your point, but for the record, both TEGRA_GPIO() and IRQ_TYPE_LEVEL_HIGH expand to a single cell, as I would expect (almost?) any DT macro to do.
On Mon, Jan 13, 2014 at 09:48 -0700, Stephen Warren wrote: > > On 01/10/2014 10:03 AM, Gerhard Sittig wrote: > > On Fri, Jan 10, 2014 at 09:30 -0600, Rob Herring wrote: > >> > >> As for *.h vs. *.dtsi naming, if the include is only pre-processor > >> defines, then I think using .h is the right way. Otherwise, if there > >> is any dts syntax, then .dtsi is the right name. It looks to me like > >> this style has been followed in both the imx and s3c64xx cases. > >> > >> On a side note, I'm not really a fan of this pattern: > >> > >> #define FOO1 1 2 3 > >> > >> #define BAR FOO1 FOO2 FOO3 > >> > >> While it definitely simplifies the dts files, it would never be used > >> in C and obfuscates what the actual property size is. Reading a dts > >> file, I would naturally assume the define was a single value while in > >> fact it could expand to a very large property size. > > > > For more complex yet tedious repetitive cases like GPIO banks and > > pins I've seen #define directives which introduce "functions" > > (macros with parameters). They appear to be rather useful, can > > reflect very well the essence of the information, don't > > necessarily pretend to be single values, but still may hide how > > many cells they expand to. For example: > > > > arch/arm/boot/dts/tegra30-cardhu.dtsi: > > interrupts = <TEGRA_GPIO(L, 0) IRQ_TYPE_LEVEL_HIGH>; > > I'm not sure I entirely understand your point, but for the record, both > TEGRA_GPIO() and IRQ_TYPE_LEVEL_HIGH expand to a single cell, as I would > expect (almost?) any DT macro to do. It turns out I slightly misunderstood Rob's concern. The issue is not that the use of the C language preprocessor is discouraged or frouned upon, but instead that obfuscating invariant data or spilling random parts of the information across the place in unneeded ways is unwanted. While there is agreement that the use of symbolic identifiers can help readability and maintainability in certain situations. Regarding your specific reply: I haven't noticed before that all of the macros I cited do expand to a single cell. Before that I saw the potential to generate more cells when required for a single value (like "wide" addresses, or multi cell pins or channels or whatever). Can't tell how much of a concern there is against such an approach, don't have strong feelings about it. Actually I wasn't picking at those specific macros above, quite the contrary. Those were the first I came across searching for demonstration of useful preprocessor use. :) I just did not bother to cite several hundred other useful phrases as well. virtually yours Gerhard Sittig
Hi Shawn, did this topic get any final resolution - especially in terms of the pingrp.h header? As I'm currently preparing two board files (imx50 and imx6sl) this discussion made me unsure if using the pin-group definitions is still the preferred style. Thanks Heiko On Monday 13 January 2014 10:19:14 Shawn Guo wrote: > On Sun, Jan 12, 2014 at 09:21:19PM +0100, Arnd Bergmann wrote: > > I was under the impression that the generic pinctrl binding was designed > > in a way to let you assign labels to each possible (reasonable) > > configuration so you didn't have get to this level of detail at the > > driver. > > The generic part of pinctrl binding only covers the procedure how a > client device get its pinctrl state configuration from a pin controller > node. That's what we defined in bindings/pinctrl/pinctrl-bindings.txt > and implemented in pinctrl core. But the details of how a pinctrl state > configuration should be interpreted for a particular pin controller is > defined by individual pin controller binding like fsl,imx-pinctrl.txt, > and implemented in the pin controller driver like > drivers/pinctrl/pinctrl-imx.c. > > > I'm also surprised that you have to know multiple constants > > (mux register, input register, config register offsets) to select a > > particular pin. I would have expected that you could have one constant > > from which the driver is able to compute the other ones. > > That's what we do before. We define a constant in the binding and have > the driver to maintain these data and look up the data using the > constant. See commit below for imx6q example. > > d8fe357 pinctrl: pinctrl-imx: add imx6q pinctrl driver > > The biggest problem with that approach is we have huge data to maintain > in the driver because of the complexity and flexibility of IMX pin > controller design. And these data can not be init data. Check that big > array of struct imx_pin_reg in commit above for what I'm talking about. > So when we build a v7 kernel image for IMX, we will have such big array > for each of these SoCs, imx50, imx51, imx53, imx6sl, imx6dl, imx6q, and > more to come. > > That's why we went through the pain of breaking DT compatibility to move > all these data into device tree one year ago with the commit below. > > e164153 pinctrl: imx: move hard-coding data into device tree > > Now kernel gets released from the floating and we do not even need to > touch kernel to add these data when new SoC support is added. > > Shawn
On Fri, Jan 24, 2014 at 09:02:09AM +0100, Heiko Stübner wrote: > Hi Shawn, > > did this topic get any final resolution - especially in terms of the pingrp.h > header? > > As I'm currently preparing two board files (imx50 and imx6sl) this discussion > made me unsure if using the pin-group definitions is still the preferred style. Yes, we need to get a conclusion ASAP. The whole imx-dt-3.14 pull request has been held for a few weeks because of that, and I'm asking for direction there [1]. Shawn [1] http://thread.gmane.org/gmane.linux.drivers.devicetree/58685/focus=296761