| Message ID | 1392358613-19962-1-git-send-email-swarren@wwwdotorg.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 14, 2014 at 06:16:52AM +0000, Stephen Warren wrote: > clk-fixed-rate currently names clocks according to a node's name without > the unit address. When faced with the legal and technically correct DT > structure below, this causes rgistration attempts for 3 clocks with the > same name, 2 of which fail. > > clocks { > compatible = "simple-bus"; > #address-cells = <1>; > #size-cells = <0>; > > clk_mmc: clock@0 { > compatible = "fixed-clock"; > reg = <0>; > ... > clk_i2c: clock@1 { > compatible = "fixed-clock"; > reg = <1>; > ... > clk_spi: clock@2 { > compatible = "fixed-clock"; > reg = <2>; > ... I'd argue that this case isn't valid. The fixed-clock binding doesn't define a reg, yet simple bus binding implies that the reg property of child nodes should be interpretted as the same address space as their parent (MMIO in this case?). The fixed-clock nodes reg proeprties clearly aren't MMIO addresses. Additionally, the _requred_ ranges property is missing. It's just nonsensical; rename them to clock_{0,1,..} instead and get rid of the reg properties. Then they're named uniquely. However, for cases where the reg value is meaningful the below patch makes sense. Thanks, Mark. > > Solve this by naming the clocks after the full node name rather than the > short version (e.g. /clocks/clock@0). > > Signed-off-by: Stephen Warren <swarren@wwwdotorg.org> > --- > Note that if this is accepted, I intend to submit a patch for the RPi DTS > which uses the naming structure above, so it might be useful to place this > patch in its own branch. Or, I could submit the cleanup after 3.15-rc1. > --- > drivers/clk/clk-fixed-rate.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c > index 0fc56ab..3335b3c 100644 > --- a/drivers/clk/clk-fixed-rate.c > +++ b/drivers/clk/clk-fixed-rate.c > @@ -115,7 +115,7 @@ EXPORT_SYMBOL_GPL(clk_register_fixed_rate); > void of_fixed_clk_setup(struct device_node *node) > { > struct clk *clk; > - const char *clk_name = node->name; > + const char *clk_name = node->full_name; > u32 rate; > u32 accuracy = 0; > > -- > 1.8.3.2 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" 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/2014 03:35 AM, Mark Rutland wrote: > On Fri, Feb 14, 2014 at 06:16:52AM +0000, Stephen Warren wrote: >> clk-fixed-rate currently names clocks according to a node's name without >> the unit address. When faced with the legal and technically correct DT >> structure below, this causes rgistration attempts for 3 clocks with the >> same name, 2 of which fail. >> >> clocks { >> compatible = "simple-bus"; >> #address-cells = <1>; >> #size-cells = <0>; >> >> clk_mmc: clock@0 { >> compatible = "fixed-clock"; >> reg = <0>; >> ... >> clk_i2c: clock@1 { >> compatible = "fixed-clock"; >> reg = <1>; >> ... >> clk_spi: clock@2 { >> compatible = "fixed-clock"; >> reg = <2>; >> ... > > I'd argue that this case isn't valid. Well, it's very widely used, and was the result of numerous discussions of how this kind of thing should be represented:-/ > The fixed-clock binding doesn't define a reg, yet simple bus binding > implies that the reg property of child nodes should be interpretted as > the same address space as their parent (MMIO in this case?). The > fixed-clock nodes reg proeprties clearly aren't MMIO addresses. > > Additionally, the _requred_ ranges property is missing. Perhaps we need to invent a simple-container instead then? > It's just nonsensical; rename them to clock_{0,1,..} instead and get rid > of the reg properties. Then they're named uniquely. That's not legal either. DT node names are supposed to represent the type of device/object (i.e. just "clock"), not the identity of the device/object (i.e. not include IDs etc.). Hence, the node name needs to be "clock" for all of them, and the unit address must be used to differentiate them.
On 02/14/2014 03:35 AM, Mark Rutland wrote: > On Fri, Feb 14, 2014 at 06:16:52AM +0000, Stephen Warren wrote: >> clk-fixed-rate currently names clocks according to a node's name without >> the unit address. When faced with the legal and technically correct DT >> structure below, this causes rgistration attempts for 3 clocks with the >> same name, 2 of which fail. >> >> clocks { >> compatible = "simple-bus"; >> #address-cells = <1>; >> #size-cells = <0>; >> >> clk_mmc: clock@0 { >> compatible = "fixed-clock"; >> reg = <0>; >> ... >> clk_i2c: clock@1 { >> compatible = "fixed-clock"; >> reg = <1>; >> ... >> clk_spi: clock@2 { >> compatible = "fixed-clock"; >> reg = <2>; >> ... > > I'd argue that this case isn't valid. > > The fixed-clock binding doesn't define a reg, yet simple bus binding > implies that the reg property of child nodes should be interpretted as > the same address space as their parent (MMIO in this case?). The > fixed-clock nodes reg proeprties clearly aren't MMIO addresses. > > Additionally, the _requred_ ranges property is missing. Oh, IIRC that was deliberate to indicate that the child address space was disjoint from the parent address space.
Hello Stephen, El 14/02/14 03:16, Stephen Warren escribió: > clk-fixed-rate currently names clocks according to a node's name without > the unit address. When faced with the legal and technically correct DT > structure below, this causes rgistration attempts for 3 clocks with the > same name, 2 of which fail. > > clocks { > compatible = "simple-bus"; > #address-cells = <1>; > #size-cells = <0>; > > clk_mmc: clock@0 { > compatible = "fixed-clock"; > reg = <0>; > ... > clk_i2c: clock@1 { > compatible = "fixed-clock"; > reg = <1>; > ... > clk_spi: clock@2 { > compatible = "fixed-clock"; > reg = <2>; > ... > > Solve this by naming the clocks after the full node name rather than the > short version (e.g. /clocks/clock@0). An alternative that doesn't require any change in the driver would be to use the optional but recommended clock-output-names property from the common clock binding. Your DT would then look something like the following > ... > clk_mmc: clk@0 { > compatible = "fixed-clock"; > clock-output-names = "mmc"; > ... > clk_i2c: clk@1 { > compatible = "fixed-clock"; > clock-output-names = "i2c"; > ... > clk_spi: clk@2 { > compatible = "fixed-clock"; > clock-output-names = "spi"; > ... Cheers, Emilio
On Fri, Feb 14, 2014 at 04:43:04PM +0000, Stephen Warren wrote: > On 02/14/2014 03:35 AM, Mark Rutland wrote: > > On Fri, Feb 14, 2014 at 06:16:52AM +0000, Stephen Warren wrote: > >> clk-fixed-rate currently names clocks according to a node's name without > >> the unit address. When faced with the legal and technically correct DT > >> structure below, this causes rgistration attempts for 3 clocks with the > >> same name, 2 of which fail. > >> > >> clocks { > >> compatible = "simple-bus"; > >> #address-cells = <1>; > >> #size-cells = <0>; > >> > >> clk_mmc: clock@0 { > >> compatible = "fixed-clock"; > >> reg = <0>; > >> ... > >> clk_i2c: clock@1 { > >> compatible = "fixed-clock"; > >> reg = <1>; > >> ... > >> clk_spi: clock@2 { > >> compatible = "fixed-clock"; > >> reg = <2>; > >> ... > > > > I'd argue that this case isn't valid. > > Well, it's very widely used, and was the result of numerous discussions > of how this kind of thing should be represented:-/ Maybe we have to live with it then. :/ > > > The fixed-clock binding doesn't define a reg, yet simple bus binding > > implies that the reg property of child nodes should be interpretted as > > the same address space as their parent (MMIO in this case?). The > > fixed-clock nodes reg proeprties clearly aren't MMIO addresses. > > > > Additionally, the _requred_ ranges property is missing. > > Perhaps we need to invent a simple-container instead then? As you mention in your other reply, it makes some sense to omit ranges if the address space is disjoint. My concern is that the address space is meaningless and arbitrary. If we had a real disjoint address space we'd have another kind of bus node as the container. > > > It's just nonsensical; rename them to clock_{0,1,..} instead and get rid > > of the reg properties. Then they're named uniquely. > > That's not legal either. DT node names are supposed to represent the > type of device/object (i.e. just "clock"), not the identity of the > device/object (i.e. not include IDs etc.). Hence, the node name needs to > be "clock" for all of them, and the unit address must be used to > differentiate them. As far as I can see from ePAPR, the only requriement is: The node-name shall start with a lower or uppercase character and should describe the general class of device. IMO clock_1 describes the general class of device as well as clock@1, while also not filling a unexpected property with a meaningless value. Thanks, Mark.
On 02/18/2014 04:23 AM, Mark Rutland wrote: > On Fri, Feb 14, 2014 at 04:43:04PM +0000, Stephen Warren wrote: >> On 02/14/2014 03:35 AM, Mark Rutland wrote: >>> On Fri, Feb 14, 2014 at 06:16:52AM +0000, Stephen Warren wrote: >>>> clk-fixed-rate currently names clocks according to a node's name without >>>> the unit address. When faced with the legal and technically correct DT >>>> structure below, this causes rgistration attempts for 3 clocks with the >>>> same name, 2 of which fail. >>>> >>>> clocks { >>>> compatible = "simple-bus"; >>>> #address-cells = <1>; >>>> #size-cells = <0>; >>>> >>>> clk_mmc: clock@0 { >>>> compatible = "fixed-clock"; >>>> reg = <0>; >>>> ... >>>> clk_i2c: clock@1 { >>>> compatible = "fixed-clock"; >>>> reg = <1>; >>>> ... >>>> clk_spi: clock@2 { >>>> compatible = "fixed-clock"; >>>> reg = <2>; >>>> ... >>> >>> I'd argue that this case isn't valid. >> >> Well, it's very widely used, and was the result of numerous discussions >> of how this kind of thing should be represented:-/ > > Maybe we have to live with it then. :/ Great:-) ... >>> It's just nonsensical; rename them to clock_{0,1,..} instead and get rid >>> of the reg properties. Then they're named uniquely. >> >> That's not legal either. DT node names are supposed to represent the >> type of device/object (i.e. just "clock"), not the identity of the >> device/object (i.e. not include IDs etc.). Hence, the node name needs to >> be "clock" for all of them, and the unit address must be used to >> differentiate them. > > As far as I can see from ePAPR, the only requriement is: > > The node-name shall start with a lower or uppercase character and > should describe the general class of device. > > IMO clock_1 describes the general class of device as well as clock@1, > while also not filling a unexpected property with a meaningless value. I believe section 2.2.2 "Generic Names Recommendation" is the source of the rule that nodes should be named after the type of object rather than the identity. """ The name of a node should be somewhat generic, reflecting the function of the device and not its precise programming model. If appropriate, the name should be one of the following choices: * atm * cache-controller * compact-flash * can * cpu ... """ (I note e.g. "cpu" not "cpu-1", "cpu_2", etc.)
diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c index 0fc56ab..3335b3c 100644 --- a/drivers/clk/clk-fixed-rate.c +++ b/drivers/clk/clk-fixed-rate.c @@ -115,7 +115,7 @@ EXPORT_SYMBOL_GPL(clk_register_fixed_rate); void of_fixed_clk_setup(struct device_node *node) { struct clk *clk; - const char *clk_name = node->name; + const char *clk_name = node->full_name; u32 rate; u32 accuracy = 0;
clk-fixed-rate currently names clocks according to a node's name without the unit address. When faced with the legal and technically correct DT structure below, this causes rgistration attempts for 3 clocks with the same name, 2 of which fail. clocks { compatible = "simple-bus"; #address-cells = <1>; #size-cells = <0>; clk_mmc: clock@0 { compatible = "fixed-clock"; reg = <0>; ... clk_i2c: clock@1 { compatible = "fixed-clock"; reg = <1>; ... clk_spi: clock@2 { compatible = "fixed-clock"; reg = <2>; ... Solve this by naming the clocks after the full node name rather than the short version (e.g. /clocks/clock@0). Signed-off-by: Stephen Warren <swarren@wwwdotorg.org> --- Note that if this is accepted, I intend to submit a patch for the RPi DTS which uses the naming structure above, so it might be useful to place this patch in its own branch. Or, I could submit the cleanup after 3.15-rc1. --- drivers/clk/clk-fixed-rate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)