Re: [PATCH 5/5] [powerpc] Fix up fsl-flexcan device tree binding.
On Aug 9, 2011, at 3:59 PM, Robin Holt wrote: I guess my poor wording may have gotten me in trouble. I am getting ready to repost this patch, but I want to ensure I am getting it as right as possible. I think I should reword the commit message to indicate we are removing the Documentation/.../fsl-flexcan.txt file which has essentially become empty and change the p1010si.dtsi file's can nodes to fsl,p1010-flexcan, fsl,flexcan. Is that correct? Thanks, Robin This is wrong. Again, what binding covers fsl,flexcan if you remove fsl-flexcan.txt: [galak@right powerpc]$ git grep flexcan Documentation/devicetree/bindings Documentation/devicetree/bindings/net/can/fsl-flexcan.txt:fsl,flexcan-v1.0 nodes Documentation/devicetree/bindings/net/can/fsl-flexcan.txt:- fsl,flexcan-clock-source : CAN Engine Clock Source.This property selects Documentation/devicetree/bindings/net/can/fsl-flexcan.txt:- fsl,flexcan-clock-divider : for the reference and system clock, an additional Documentation/devicetree/bindings/net/can/fsl-flexcan.txt: - v1.0 of flexcan-v1.0 represent the IP block version for P1010 SOC. Documentation/devicetree/bindings/net/can/fsl-flexcan.txt: compatible = fsl,flexcan-v1.0; Documentation/devicetree/bindings/net/can/fsl-flexcan.txt: fsl,flexcan-clock-source = platform; Documentation/devicetree/bindings/net/can/fsl-flexcan.txt: fsl,flexcan-clock-divider = 2; Not seeing anything that covers it. I think the issue should be resolved by patching fsl-flexcan.txt to remove wording or update it. - k ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 5/5] [powerpc] Fix up fsl-flexcan device tree binding.
On Wed, Aug 10, 2011 at 09:52:07AM -0500, Kumar Gala wrote: On Aug 9, 2011, at 3:59 PM, Robin Holt wrote: I guess my poor wording may have gotten me in trouble. I am getting ready to repost this patch, but I want to ensure I am getting it as right as possible. I think I should reword the commit message to indicate we are removing the Documentation/.../fsl-flexcan.txt file which has essentially become empty and change the p1010si.dtsi file's can nodes to fsl,p1010-flexcan, fsl,flexcan. Is that correct? Thanks, Robin This is wrong. Again, what binding covers fsl,flexcan if you remove fsl-flexcan.txt: [galak@right powerpc]$ git grep flexcan Documentation/devicetree/bindings Documentation/devicetree/bindings/net/can/fsl-flexcan.txt:fsl,flexcan-v1.0 nodes Documentation/devicetree/bindings/net/can/fsl-flexcan.txt:- fsl,flexcan-clock-source : CAN Engine Clock Source.This property selects Documentation/devicetree/bindings/net/can/fsl-flexcan.txt:- fsl,flexcan-clock-divider : for the reference and system clock, an additional Documentation/devicetree/bindings/net/can/fsl-flexcan.txt: - v1.0 of flexcan-v1.0 represent the IP block version for P1010 SOC. Documentation/devicetree/bindings/net/can/fsl-flexcan.txt: compatible = fsl,flexcan-v1.0; Documentation/devicetree/bindings/net/can/fsl-flexcan.txt: fsl,flexcan-clock-source = platform; Documentation/devicetree/bindings/net/can/fsl-flexcan.txt: fsl,flexcan-clock-divider = 2; Not seeing anything that covers it. I think the issue should be resolved by patching fsl-flexcan.txt to remove wording or update it. Done. Robin ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 5/5] [powerpc] Fix up fsl-flexcan device tree binding.
In working with the socketcan developers, we have come to the conclusion
the fsl-flexcan device tree bindings need to be cleaned up. The driver
does not depend upon any properties other than the required properties
so we are removing the file. Additionally, the p1010*dts files are not
following the standard for node naming in that they have a trailing -v1.0.
Signed-off-by: Robin Holt h...@sgi.com
To: Marc Kleine-Budde m...@pengutronix.de,
To: Wolfgang Grandegger w...@grandegger.com,
To: U Bhaskar-B22300 b22...@freescale.com
Cc: socketcan-c...@lists.berlios.de,
Cc: net...@vger.kernel.org,
Cc: PPC list linuxppc-dev@lists.ozlabs.org
Cc: Kumar Gala ga...@kernel.crashing.org
---
.../devicetree/bindings/net/can/fsl-flexcan.txt| 61
arch/powerpc/boot/dts/p1010rdb.dts |8 ---
arch/powerpc/boot/dts/p1010si.dtsi |6 +-
3 files changed, 2 insertions(+), 73 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
deleted file mode 100644
index 1a729f0..000
--- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt
+++ /dev/null
@@ -1,61 +0,0 @@
-CAN Device Tree Bindings
-
-2011 Freescale Semiconductor, Inc.
-
-fsl,flexcan-v1.0 nodes
-In addition to the required compatible-, reg- and interrupt-properties, you can
-also specify which clock source shall be used for the controller.
-
-CPI Clock- Can Protocol Interface Clock
- This CLK_SRC bit of CTRL(control register) selects the clock source to
- the CAN Protocol Interface(CPI) to be either the peripheral clock
- (driven by the PLL) or the crystal oscillator clock. The selected clock
- is the one fed to the prescaler to generate the Serial Clock (Sclock).
- The PRESDIV field of CTRL(control register) controls a prescaler that
- generates the Serial Clock (Sclock), whose period defines the
- time quantum used to compose the CAN waveform.
-
-Can Engine Clock Source
- There are two sources for CAN clock
- - Platform Clock It represents the bus clock
- - Oscillator Clock
-
- Peripheral Clock (PLL)
- --
-|
- - -
- | |CPI Clock| Prescaler | Sclock
- | || (1.. 256) |
- - -
- | |
- -- -CLK_SRC
- Oscillator Clock
-
-- fsl,flexcan-clock-source : CAN Engine Clock Source.This property selects
-the peripheral clock. PLL clock is fed to the
-prescaler to generate the Serial Clock (Sclock).
-Valid values are oscillator and platform
-oscillator: CAN engine clock source is
oscillator clock.
-platform The CAN engine clock source is the bus
clock
-(platform clock).
-
-- fsl,flexcan-clock-divider : for the reference and system clock, an additional
- clock divider can be specified.
-- clock-frequency: frequency required to calculate the bitrate for FlexCAN.
-
-Note:
- - v1.0 of flexcan-v1.0 represent the IP block version for P1010 SOC.
- - P1010 does not have oscillator as the Clock Source.So the default
- Clock Source is platform clock.
-Examples:
-
- can0@1c000 {
- compatible = fsl,flexcan-v1.0;
- reg = 0x1c000 0x1000;
- interrupts = 48 0x2;
- interrupt-parent = mpic;
- fsl,flexcan-clock-source = platform;
- fsl,flexcan-clock-divider = 2;
- clock-frequency = fixed by u-boot;
- };
diff --git a/arch/powerpc/boot/dts/p1010rdb.dts
b/arch/powerpc/boot/dts/p1010rdb.dts
index 6b33b73..d6a0bb2 100644
--- a/arch/powerpc/boot/dts/p1010rdb.dts
+++ b/arch/powerpc/boot/dts/p1010rdb.dts
@@ -169,14 +169,6 @@
};
};
- can0@1c000 {
- fsl,flexcan-clock-source = platform;
- };
-
- can1@1d000 {
- fsl,flexcan-clock-source = platform;
- };
-
usb@22000 {
phy_type = utmi;
};
diff --git a/arch/powerpc/boot/dts/p1010si.dtsi
b/arch/powerpc/boot/dts/p1010si.dtsi
index 7f51104..37e47cd 100644
--- a/arch/powerpc/boot/dts/p1010si.dtsi
+++ b/arch/powerpc/boot/dts/p1010si.dtsi
@@ -141,19 +141,17 @@
};
can0@1c000 {
- compatible = fsl,flexcan-v1.0;
+ compatible = fsl,flexcan;
Re: [PATCH 5/5] [powerpc] Fix up fsl-flexcan device tree binding.
On 08/09/2011 09:43 AM, Robin Holt wrote: In working with the socketcan developers, we have come to the conclusion the fsl-flexcan device tree bindings need to be cleaned up. The driver does not depend upon any properties other than the required properties so we are removing the file. That is not the criterion for whether something should be expresed in the device tree. It's a description of the hardware, not a Linux driver configuration file. If there are integration parameters that can not be inferred from this is FSL flexcan v1.0, they should be expressed in the node. Removing the binding altogether seems extreme as well -- we should have bindings for all devices, even if there are no special properties. Additionally, the p1010*dts files are not following the standard for node naming in that they have a trailing -v1.0. What standard for node naming? There's nothing wrong with putting a block version number in the compatible string, and it looks like the p1010 dts files were following the binding document in this regard. It is common practice when the block version is publicly documented but there's no register it can be read from at runtime. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 5/5] [powerpc] Fix up fsl-flexcan device tree binding.
On Tue, Aug 09, 2011 at 01:17:47PM -0500, Scott Wood wrote: On 08/09/2011 09:43 AM, Robin Holt wrote: In working with the socketcan developers, we have come to the conclusion the fsl-flexcan device tree bindings need to be cleaned up. The driver does not depend upon any properties other than the required properties so we are removing the file. That is not the criterion for whether something should be expresed in the device tree. It's a description of the hardware, not a Linux driver configuration file. If there are integration parameters that can not be inferred from this is FSL flexcan v1.0, they should be expressed in the node. There are no properties other than the required properties. The others were wrongly introduced and are not needed by the driver. When we removed the other properties and the wrong documentation of the mscan oscillator source in the fsl-flexcan.txt file, we were left with an Example: section and a one-line statement The only properties supported are the required properties. That seemed like the fsl-flexcan.txt file was then pointless. Removing the binding altogether seems extreme as well -- we should have bindings for all devices, even if there are no special properties. Ok. I can do that too. Who is the definitive source for that answer? I assume we are talking about the fsl-flexcan.txt file when we say binding. Is that correct? Additionally, the p1010*dts files are not following the standard for node naming in that they have a trailing -v1.0. What standard for node naming? There's nothing wrong with putting a For the answer to that, you will need to ask Wolfgang Grandegger. I was working from his feedback. Looking at the plethora of other node names, the vast majority do not have any -v#.#, and the ones that do also tend to have multiple versions. Based upon that, I suspect he is correct, but I do not know where the documentation is or if it even exists. block version number in the compatible string, and it looks like the p1010 dts files were following the binding document in this regard. It is common practice when the block version is publicly documented but there's no register it can be read from at runtime. Thanks, Robin ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 5/5] [powerpc] Fix up fsl-flexcan device tree binding.
On 08/09/2011 01:45 PM, Robin Holt wrote: On Tue, Aug 09, 2011 at 01:17:47PM -0500, Scott Wood wrote: On 08/09/2011 09:43 AM, Robin Holt wrote: In working with the socketcan developers, we have come to the conclusion the fsl-flexcan device tree bindings need to be cleaned up. The driver does not depend upon any properties other than the required properties so we are removing the file. That is not the criterion for whether something should be expresed in the device tree. It's a description of the hardware, not a Linux driver configuration file. If there are integration parameters that can not be inferred from this is FSL flexcan v1.0, they should be expressed in the node. There are no properties other than the required properties. The others were wrongly introduced and are not needed by the driver. Not needed by this driver, or will never be needed by any reasonable driver (or is not a good description of the hardware)? The device tree is not an internal Linux implementation detail. It is shared by other OSes, firmwares, hypervisors, etc. Bindings should be created with care, and kept stable unless there's a good reason to break compatibility. devicetree-disc...@lists.ozlabs.org should be CCed on device tree discussions. When we removed the other properties and the wrong documentation of the mscan oscillator source in the fsl-flexcan.txt file, we were left with an Example: section and a one-line statement The only properties supported are the required properties. That seemed like the fsl-flexcan.txt file was then pointless. There is the compatible string, and you could mention that there is a single reg resource and a single interrupt. Removing the binding altogether seems extreme as well -- we should have bindings for all devices, even if there are no special properties. Ok. I can do that too. Who is the definitive source for that answer? For policy questions on device tree bindings? Grant Likely is the maintainer for device tree stuff. A lot of the simpler bindings have been left undocumented so far, IMHO it should be a goal to document them all. There are some existing ones that are documented despite not having special properties, e.g. Documentation/devicetree/bindings/serio/altera_ps2.txt, Documentation/devicetree/bindings/arm/sirf.txt, Documentation/devicetree/bindings/powerpc/nintendo/wii.txt, etc. I assume we are talking about the fsl-flexcan.txt file when we say binding. Is that correct? Yes, although devicetree.org is another possibility. Additionally, the p1010*dts files are not following the standard for node naming in that they have a trailing -v1.0. What standard for node naming? There's nothing wrong with putting a For the answer to that, you will need to ask Wolfgang Grandegger. I was working from his feedback. Looking at the plethora of other node names, the vast majority do not have any -v#.#, and the ones that do also tend to have multiple versions. Based upon that, I suspect he is correct, but I do not know where the documentation is or if it even exists. There's a lot of crap in old bindings, plus it's not appropriate for all circumstances (specifying bindings should be done a little more carefully than what do most other bindings do?). It's something we've been doing lately for blocks that have a version number, but it's not dynamically readable. Looking in the FlexCAN chapter of the p1010 manual, I don't see any reference to a block version, and I do see references to previous FlexCAN versions. So I suggest fsl,p1010-flexcan. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 5/5] [powerpc] Fix up fsl-flexcan device tree binding.
On 08/09/2011 08:17 PM, Scott Wood wrote: On 08/09/2011 09:43 AM, Robin Holt wrote: In working with the socketcan developers, we have come to the conclusion the fsl-flexcan device tree bindings need to be cleaned up. The driver does not depend upon any properties other than the required properties so we are removing the file. That is not the criterion for whether something should be expresed in the device tree. It's a description of the hardware, not a Linux driver configuration file. If there are integration parameters that can not be inferred from this is FSL flexcan v1.0, they should be expressed in the node. Removing the binding altogether seems extreme as well -- we should have bindings for all devices, even if there are no special properties. Yes, of course. The commit message misleading. We do not intend to remove the binding but just a few unused and confusing properties. Concerning the compatible string, Freescale introduced for the Flexcan on the P1010 fsl,flexcan-v1.0. That's not the usual convention also because the v1.0 if for the PowerPC cores only, I assume, but we have ARM cores as well. If we need to distinguish I think we should use: fsl,p1010-flexcan, fsl,flexcan Do you agree? Additionally, the p1010*dts files are not following the standard for node naming in that they have a trailing -v1.0. What standard for node naming? There's nothing wrong with putting a block version number in the compatible string, and it looks like the p1010 dts files were following the binding document in this regard. It is common practice when the block version is publicly documented but there's no register it can be read from at runtime. See above. Furthermore I must admit, that the bindings shown up mainline Linux have never been presented on any mailing list. Wolfgang. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 5/5] [powerpc] Fix up fsl-flexcan device tree binding.
On 08/09/2011 09:13 PM, Scott Wood wrote: On 08/09/2011 01:45 PM, Robin Holt wrote: On Tue, Aug 09, 2011 at 01:17:47PM -0500, Scott Wood wrote: On 08/09/2011 09:43 AM, Robin Holt wrote: In working with the socketcan developers, we have come to the conclusion the fsl-flexcan device tree bindings need to be cleaned up. The driver does not depend upon any properties other than the required properties so we are removing the file. That is not the criterion for whether something should be expresed in the device tree. It's a description of the hardware, not a Linux driver configuration file. If there are integration parameters that can not be inferred from this is FSL flexcan v1.0, they should be expressed in the node. There are no properties other than the required properties. The others were wrongly introduced and are not needed by the driver. Not needed by this driver, or will never be needed by any reasonable driver (or is not a good description of the hardware)? The device tree is not an internal Linux implementation detail. It is shared by other OSes, firmwares, hypervisors, etc. Bindings should be created with care, and kept stable unless there's a good reason to break compatibility. devicetree-disc...@lists.ozlabs.org should be CCed on device tree discussions. Yes. The doc for the bindings we speak about http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt sneaked into the kernel without been presented on any mailing list and without the corresponding driver patch. When we removed the other properties and the wrong documentation of the mscan oscillator source in the fsl-flexcan.txt file, we were left with an Example: section and a one-line statement The only properties supported are the required properties. That seemed like the fsl-flexcan.txt file was then pointless. There is the compatible string, and you could mention that there is a single reg resource and a single interrupt. Removing the binding altogether seems extreme as well -- we should have bindings for all devices, even if there are no special properties. Ok. I can do that too. Who is the definitive source for that answer? For policy questions on device tree bindings? Grant Likely is the maintainer for device tree stuff. A lot of the simpler bindings have been left undocumented so far, IMHO it should be a goal to document them all. There are some existing ones that are documented despite not having special properties, e.g. Documentation/devicetree/bindings/serio/altera_ps2.txt, Documentation/devicetree/bindings/arm/sirf.txt, Documentation/devicetree/bindings/powerpc/nintendo/wii.txt, etc. I assume we are talking about the fsl-flexcan.txt file when we say binding. Is that correct? Yes, although devicetree.org is another possibility. Additionally, the p1010*dts files are not following the standard for node naming in that they have a trailing -v1.0. What standard for node naming? There's nothing wrong with putting a For the answer to that, you will need to ask Wolfgang Grandegger. I was working from his feedback. Looking at the plethora of other node names, the vast majority do not have any -v#.#, and the ones that do also tend to have multiple versions. Based upon that, I suspect he is correct, but I do not know where the documentation is or if it even exists. There's a lot of crap in old bindings, plus it's not appropriate for all circumstances (specifying bindings should be done a little more carefully than what do most other bindings do?). It's something we've been doing lately for blocks that have a version number, but it's not dynamically readable. Looking in the FlexCAN chapter of the p1010 manual, I don't see any reference to a block version, and I do see references to previous FlexCAN versions. So I suggest fsl,p1010-flexcan. OK, just fsl,p1010-flexcan or fsl,p1010-flexcan, fsl,flexcan Note that the Flexcan is used on Freescale ARM cores as well (and device tree for ARM will show up soon). Wolfgang. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 5/5] [powerpc] Fix up fsl-flexcan device tree binding.
On 08/09/2011 02:49 PM, Wolfgang Grandegger wrote: Yes. The doc for the bindings we speak about http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt sneaked into the kernel without been presented on any mailing list and without the corresponding driver patch. It was posted on linuxppc-dev: http://patchwork.ozlabs.org/patch/91980/ Though I agree it should have been posted more widely. OK, just fsl,p1010-flexcan or fsl,p1010-flexcan, fsl,flexcan I'm ok with the latter, if there's enough in common that it's conceivable that a driver wouldn't care. The more specific compatible will be there if the driver wants to make use of it later. -Scot ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 5/5] [powerpc] Fix up fsl-flexcan device tree binding.
On 08/09/2011 02:32 PM, Wolfgang Grandegger wrote: On 08/09/2011 08:17 PM, Scott Wood wrote: On 08/09/2011 09:43 AM, Robin Holt wrote: In working with the socketcan developers, we have come to the conclusion the fsl-flexcan device tree bindings need to be cleaned up. The driver does not depend upon any properties other than the required properties so we are removing the file. That is not the criterion for whether something should be expresed in the device tree. It's a description of the hardware, not a Linux driver configuration file. If there are integration parameters that can not be inferred from this is FSL flexcan v1.0, they should be expressed in the node. Removing the binding altogether seems extreme as well -- we should have bindings for all devices, even if there are no special properties. Yes, of course. The commit message misleading. We do not intend to remove the binding but just a few unused and confusing properties. Is it a matter of the current driver not caring, or the properties just not making sense for any reasonable driver (ambiguous, inferrable from the flexcan version, software configuration, etc)? -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 5/5] [powerpc] Fix up fsl-flexcan device tree binding.
I guess my poor wording may have gotten me in trouble. I am getting ready to repost this patch, but I want to ensure I am getting it as right as possible. I think I should reword the commit message to indicate we are removing the Documentation/.../fsl-flexcan.txt file which has essentially become empty and change the p1010si.dtsi file's can nodes to fsl,p1010-flexcan, fsl,flexcan. Is that correct? Thanks, Robin On Tue, Aug 09, 2011 at 02:58:56PM -0500, Scott Wood wrote: On 08/09/2011 02:49 PM, Wolfgang Grandegger wrote: Yes. The doc for the bindings we speak about http://lxr.linux.no/#linux+v3.0.1/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt sneaked into the kernel without been presented on any mailing list and without the corresponding driver patch. It was posted on linuxppc-dev: http://patchwork.ozlabs.org/patch/91980/ Though I agree it should have been posted more widely. OK, just fsl,p1010-flexcan or fsl,p1010-flexcan, fsl,flexcan I'm ok with the latter, if there's enough in common that it's conceivable that a driver wouldn't care. The more specific compatible will be there if the driver wants to make use of it later. -Scot ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
