| Message ID | 1331861451-15427-1-git-send-email-poonam.aggrwal@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 03/15/2012 08:30 PM, Poonam Aggrwal wrote: > From: Poonam Aggrwal <poonam.aggrwal@freescale.com> > > This TDM controller is available in various Freescale SOCs like MPC8315, P1020, > P1022, P1010. > > Signed-off-by: Sandeep Singh <Sandeep@freescale.com> > Signed-off-by: Poonam Aggrwal <poonam.aggrwal@freescale.com> > --- > Documentation/devicetree/bindings/tdm/fsl-tdm.txt | 71 +++++++++++++++++++++ > 1 files changed, 71 insertions(+), 0 deletions(-) > create mode 100644 Documentation/devicetree/bindings/tdm/fsl-tdm.txt > > diff --git a/Documentation/devicetree/bindings/tdm/fsl-tdm.txt b/Documentation/devicetree/bindings/tdm/fsl-tdm.txt > new file mode 100644 > index 0000000..61431e3 > --- /dev/null > +++ b/Documentation/devicetree/bindings/tdm/fsl-tdm.txt > @@ -0,0 +1,71 @@ > +===================================================================== > +TDM Device Tree Binding > +Copyright (C) 2012 Freescale Semiconductor Inc. > + > +NOTE: The bindings described in this document are preliminary > +and subject to change. > + > +===================================================================== > +TDM (Time Division Multiplexing) > + > +DESCRIPTION > + > +The TDM is full duplex serial port designed to allow various devices including > +digital signal processors (DSPs) to communicate with a variety of serial devices > +including industry standard framers, codecs, other DSPs and microprocessors. > + > +The below properties describe the device tree bindings for Freescale TDM > +controller. > +This TDM controller is available on various Freescale Processors like > +MPC8313, P1020, P1022 and P1010. > + > +PROPERTIES > + > + - compatible > + Usage: required > + Value type: <string> > + Definition: Should contain "fsl,mpc8315-tdm". > + So mpc8313 will have compatible = "fsl,mpc8315-tdm"; > + p1010 will have compatible "fsl,p1010-tdm", "fsl,mpc8315-tdm"; Shouldn't mpc8313 have: compatible = "fsl,mpc8313-tdm", "fsl,mpc8315-tdm"? I thought we were going to use 8313 as the canonical implementation, not 8315. > + - reg > + Usage: required > + Value type: <tdm-reg-offset tdm-reg-size dmac-reg-offset dmac-reg-size> > + Definition: A standard property. Specifies the physical address > + offset and length of the TDM registers and TDM DMAC registers for > + the device. Just say there's two reg resources, and that the first is the TDM registers and the second is the TDM DMAC registers. It's typically not going to be the actual physical address, but rather an offset that gets translated through a parent node's ranges. Remove "value type"; it's standard. > + - clock-frequency > + Usage: optional > + Value type: <u32> > + Definition: The frequency at which the TDM block is operating. Will this frequency ever need to be > 4GHz? Might want to specify as u32 or u64, as ePAPR suggests. > + - interrupts > + Usage: required > + Value type: <tdm-err-intr tdm-err-intr-type dmac-intr dmac-intr-type> > + Definition: This field defines two interrupt specifiers namely interrupt > + number and interrupt type for TDM error and TDM DMAC. What is "tdm-err-intr-type"? The interrupt specifier encoding is defined by the interrupt controller. There might be one cell, two cells, four cells, etc. Remove "value type", it's standard. > + - phy-handle > + Usage: optional > + Value type: <phandle> > + Definition: Phandle of the line controller node or framer node eg. SLIC, > + E1\T1 etc. Use a forward slash -- this isn't a Windows filesystem path. :-) > + - fsl-max-time-slots > + Usage: required > + Value type: <u32> > + Definition: Maximum number of 8-bit time slots in one TDM frame. > + This is the maximum number which TDM hardware supports. fsl,tdm-max-time-slots > + > +EXAMPLE > + > + tdm@16000 { > + device_type = "tdm"; No device_type > + compatible = "fsl,p1010-tdm", "fsl,mpc8315-tdm"; > + reg = <0x16000 0x200 0x2c000 0x2000>; > + clock-frequency = <0>; Show a real clock-frequency, perhaps with a comment saying it's typically filled in by boot software. > + interrupts = <16 8 62 8>; > + phy-handle = <zarlink1> That phy-handle is invalid syntax, perhaps you meant: phy-handle = <&zarlink1>; > + fsl-max-time-slots = <128> Missing semicolons on the last two properties. -Scott
Thanks Scott for the review. Will send an updated revision with the comments taken care. Regards Poonam > -----Original Message----- > From: Wood Scott-B07421 > Sent: Saturday, March 17, 2012 12:00 AM > To: Aggrwal Poonam-B10812 > Cc: devicetree-discuss@lists.ozlabs.org; linuxppc-dev@lists.ozlabs.org; > Singh Sandeep-B37400 > Subject: Re: [PATCH] Device Tree Bindings for Freescale TDM controller > > On 03/15/2012 08:30 PM, Poonam Aggrwal wrote: > > From: Poonam Aggrwal <poonam.aggrwal@freescale.com> > > > > This TDM controller is available in various Freescale SOCs like > > MPC8315, P1020, P1022, P1010. > > > > Signed-off-by: Sandeep Singh <Sandeep@freescale.com> > > Signed-off-by: Poonam Aggrwal <poonam.aggrwal@freescale.com> > > --- > > Documentation/devicetree/bindings/tdm/fsl-tdm.txt | 71 > +++++++++++++++++++++ > > 1 files changed, 71 insertions(+), 0 deletions(-) create mode 100644 > > Documentation/devicetree/bindings/tdm/fsl-tdm.txt > > > > diff --git a/Documentation/devicetree/bindings/tdm/fsl-tdm.txt > > b/Documentation/devicetree/bindings/tdm/fsl-tdm.txt > > new file mode 100644 > > index 0000000..61431e3 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/tdm/fsl-tdm.txt > > @@ -0,0 +1,71 @@ > > +===================================================================== > > +TDM Device Tree Binding > > +Copyright (C) 2012 Freescale Semiconductor Inc. > > + > > +NOTE: The bindings described in this document are preliminary and > > +subject to change. > > + > > +===================================================================== > > +TDM (Time Division Multiplexing) > > + > > +DESCRIPTION > > + > > +The TDM is full duplex serial port designed to allow various devices > > +including digital signal processors (DSPs) to communicate with a > > +variety of serial devices including industry standard framers, codecs, > other DSPs and microprocessors. > > + > > +The below properties describe the device tree bindings for Freescale > > +TDM controller. > > +This TDM controller is available on various Freescale Processors like > > +MPC8313, P1020, P1022 and P1010. > > + > > +PROPERTIES > > + > > + - compatible > > + Usage: required > > + Value type: <string> > > + Definition: Should contain "fsl,mpc8315-tdm". > > + So mpc8313 will have compatible = "fsl,mpc8315-tdm"; > > + p1010 will have compatible "fsl,p1010-tdm", "fsl,mpc8315-tdm"; > > Shouldn't mpc8313 have: > compatible = "fsl,mpc8313-tdm", "fsl,mpc8315-tdm"? > > I thought we were going to use 8313 as the canonical implementation, not > 8315. MPC8315 was the first FSL platform to have this controller. MPC8313 does not have TDM. > > > + - reg > > + Usage: required > > + Value type: <tdm-reg-offset tdm-reg-size dmac-reg-offset dmac- > reg-size> > > + Definition: A standard property. Specifies the physical address > > + offset and length of the TDM registers and TDM DMAC registers for > > + the device. > > Just say there's two reg resources, and that the first is the TDM > registers and the second is the TDM DMAC registers. > > It's typically not going to be the actual physical address, but rather an > offset that gets translated through a parent node's ranges. Okay, I think we missed this comment, you already gave this earlier. Sorry for that. > > Remove "value type"; it's standard. > Okay. So just definition must suffice, right? > > + - clock-frequency > > + Usage: optional > > + Value type: <u32> > > + Definition: The frequency at which the TDM block is operating. > > Will this frequency ever need to be > 4GHz? Don't think so, at max this will be CCB, not sure if CCB on our platforms may get bigger than 4G ever. > > Might want to specify as u32 or u64, as ePAPR suggests. Means Value type: <u32 or u64>? In this case the driver must always use 64bit data structure to read this. Is this correct? > > > + - interrupts > > + Usage: required > > + Value type: <tdm-err-intr tdm-err-intr-type dmac-intr dmac-intr- > type> > > + Definition: This field defines two interrupt specifiers namely > interrupt > > + number and interrupt type for TDM error and TDM DMAC. > > What is "tdm-err-intr-type"? The interrupt specifier encoding is defined > by the interrupt controller. There might be one cell, two cells, four > cells, etc. Remove "value type", it's standard. > okay > > + - phy-handle > > + Usage: optional > > + Value type: <phandle> > > + Definition: Phandle of the line controller node or framer node > eg. SLIC, > > + E1\T1 etc. > > Use a forward slash -- this isn't a Windows filesystem path. :-) > Okay, agreed. > > + - fsl-max-time-slots > > + Usage: required > > + Value type: <u32> > > + Definition: Maximum number of 8-bit time slots in one TDM frame. > > + This is the maximum number which TDM hardware supports. > > fsl,tdm-max-time-slots Sure. This again got missed. > > > + > > +EXAMPLE > > + > > + tdm@16000 { > > + device_type = "tdm"; > > No device_type Okay. > > > + compatible = "fsl,p1010-tdm", "fsl,mpc8315-tdm"; > > + reg = <0x16000 0x200 0x2c000 0x2000>; > > + clock-frequency = <0>; > > Show a real clock-frequency, perhaps with a comment saying it's typically > filled in by boot software. Okay. > > > + interrupts = <16 8 62 8>; > > + phy-handle = <zarlink1> > > That phy-handle is invalid syntax, perhaps you meant: > > phy-handle = <&zarlink1>; Yes. Will correct it. > > > + fsl-max-time-slots = <128> > > Missing semicolons on the last two properties. > Ok. > -Scott
On Sat, Mar 17, 2012 at 2:33 AM, Aggrwal Poonam-B10812 <B10812@freescale.com> wrote: > >> > + - clock-frequency >> > + Usage: optional >> > + Value type: <u32> >> > + Definition: The frequency at which the TDM block is operating. >> >> Will this frequency ever need to be > 4GHz? > Don't think so, at max this will be CCB, not sure if CCB on our platforms may get bigger than 4G ever. Apparently, 4GB is the new 640K. In Poonam's defense, every clock frequency property in the device tree is a 32-bit integer. I've never seen a 64-bit one.
On Sat, Mar 17, 2012 at 2:33 AM, Aggrwal Poonam-B10812 <B10812@freescale.com> wrote: > >> > + compatible = "fsl,p1010-tdm", "fsl,mpc8315-tdm"; >> > + reg = <0x16000 0x200 0x2c000 0x2000>; >> > + clock-frequency = <0>; >> >> Show a real clock-frequency, perhaps with a comment saying it's typically >> filled in by boot software. > Okay. Scott, are you suggesting that Poonam put a non-zero number in the DTS for clock-frequency? If so, then I don't think that's a good idea, if U-Boot will always override it.
On 03/17/2012 11:07 AM, Tabi Timur-B04825 wrote: > On Sat, Mar 17, 2012 at 2:33 AM, Aggrwal Poonam-B10812 > <B10812@freescale.com> wrote: >> >>>> + compatible = "fsl,p1010-tdm", "fsl,mpc8315-tdm"; >>>> + reg = <0x16000 0x200 0x2c000 0x2000>; >>>> + clock-frequency = <0>; >>> >>> Show a real clock-frequency, perhaps with a comment saying it's typically >>> filled in by boot software. > >> Okay. > > Scott, are you suggesting that Poonam put a non-zero number in the DTS > for clock-frequency? If so, then I don't think that's a good idea, if > U-Boot will always override it. This is a device tree binding document, not U-Boot specific. It describes what Linux (or another OS) can expect to see, not how it gets there. -Scott
Scott Wood wrote: >> > Scott, are you suggesting that Poonam put a non-zero number in the DTS >> > for clock-frequency? If so, then I don't think that's a good idea, if >> > U-Boot will always override it. > This is a device tree binding document, not U-Boot specific. It > describes what Linux (or another OS) can expect to see, not how it gets > there. That doesn't really answer my question. We currently have many properties that define a clock frequency, and the DTS sets them all to 0, with the intent of having U-Boot update them. Now maybe these should all be deleted, but it seems that setting them to a non-zero value is wrong, because it might mislead people into thinking that the property is not updated by U-Boot. When you see something like this: clock-frequency = <0>; It's pretty obvious that U-boot will fill it in.
On 03/17/2012 02:33 AM, Aggrwal Poonam-B10812 wrote: >>> + - compatible >>> + Usage: required >>> + Value type: <string> >>> + Definition: Should contain "fsl,mpc8315-tdm". >>> + So mpc8313 will have compatible = "fsl,mpc8315-tdm"; >>> + p1010 will have compatible "fsl,p1010-tdm", "fsl,mpc8315-tdm"; >> >> Shouldn't mpc8313 have: >> compatible = "fsl,mpc8313-tdm", "fsl,mpc8315-tdm"? >> >> I thought we were going to use 8313 as the canonical implementation, not >> 8315. > MPC8315 was the first FSL platform to have this controller. > MPC8313 does not have TDM. OK, so no example for mpc8313 then. :-) >> Will this frequency ever need to be > 4GHz? > Don't think so, at max this will be CCB, not sure if CCB on our platforms may get bigger than 4G ever. Still, I think we should always make clock-frequency properties be 32/64 as the ePAPR describes, just in case. >> Might want to specify as u32 or u64, as ePAPR suggests. > Means Value type: <u32 or u64>? Yes. > In this case the driver must always use 64bit data structure to read this. Is this correct? No, you'd use of_read_number(). Maybe factor out an of_get_frequency(). -Scott
On 03/19/2012 12:32 PM, Timur Tabi wrote: > Scott Wood wrote: >>>> Scott, are you suggesting that Poonam put a non-zero number in the DTS >>>> for clock-frequency? If so, then I don't think that's a good idea, if >>>> U-Boot will always override it. > >> This is a device tree binding document, not U-Boot specific. It >> describes what Linux (or another OS) can expect to see, not how it gets >> there. > > That doesn't really answer my question. We currently have many properties > that define a clock frequency, and the DTS sets them all to 0, with the > intent of having U-Boot update them. Ideally these trees should be in U-Boot rather than Linux. > Now maybe these should all be > deleted, but it seems that setting them to a non-zero value is wrong, > because it might mislead people into thinking that the property is not > updated by U-Boot. When you see something like this: > > clock-frequency = <0>; > > It's pretty obvious that U-boot will fill it in. You're assuming that this is a guide for writing dts files. If you look at it as a guide for writing Linux code to interpret the device tree, you'd come to the opposite conclusion. I suggested a comment about common boot software behavior (but otherwise show it as Linux would see it) as middle ground. -Scott
diff --git a/Documentation/devicetree/bindings/tdm/fsl-tdm.txt b/Documentation/devicetree/bindings/tdm/fsl-tdm.txt new file mode 100644 index 0000000..61431e3 --- /dev/null +++ b/Documentation/devicetree/bindings/tdm/fsl-tdm.txt @@ -0,0 +1,71 @@ +===================================================================== +TDM Device Tree Binding +Copyright (C) 2012 Freescale Semiconductor Inc. + +NOTE: The bindings described in this document are preliminary +and subject to change. + +===================================================================== +TDM (Time Division Multiplexing) + +DESCRIPTION + +The TDM is full duplex serial port designed to allow various devices including +digital signal processors (DSPs) to communicate with a variety of serial devices +including industry standard framers, codecs, other DSPs and microprocessors. + +The below properties describe the device tree bindings for Freescale TDM +controller. +This TDM controller is available on various Freescale Processors like +MPC8313, P1020, P1022 and P1010. + +PROPERTIES + + - compatible + Usage: required + Value type: <string> + Definition: Should contain "fsl,mpc8315-tdm". + So mpc8313 will have compatible = "fsl,mpc8315-tdm"; + p1010 will have compatible "fsl,p1010-tdm", "fsl,mpc8315-tdm"; + + - reg + Usage: required + Value type: <tdm-reg-offset tdm-reg-size dmac-reg-offset dmac-reg-size> + Definition: A standard property. Specifies the physical address + offset and length of the TDM registers and TDM DMAC registers for + the device. + + - clock-frequency + Usage: optional + Value type: <u32> + Definition: The frequency at which the TDM block is operating. + + - interrupts + Usage: required + Value type: <tdm-err-intr tdm-err-intr-type dmac-intr dmac-intr-type> + Definition: This field defines two interrupt specifiers namely interrupt + number and interrupt type for TDM error and TDM DMAC. + + - phy-handle + Usage: optional + Value type: <phandle> + Definition: Phandle of the line controller node or framer node eg. SLIC, + E1\T1 etc. + + - fsl-max-time-slots + Usage: required + Value type: <u32> + Definition: Maximum number of 8-bit time slots in one TDM frame. + This is the maximum number which TDM hardware supports. + +EXAMPLE + + tdm@16000 { + device_type = "tdm"; + compatible = "fsl,p1010-tdm", "fsl,mpc8315-tdm"; + reg = <0x16000 0x200 0x2c000 0x2000>; + clock-frequency = <0>; + interrupts = <16 8 62 8>; + phy-handle = <zarlink1> + fsl-max-time-slots = <128> + };