| Message ID | 20170119002933.7529-2-code@mmayer.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/18, Markus Mayer wrote: > diff --git a/Documentation/devicetree/bindings/clock/brcm,brcmstb-cpu-clk-div.txt b/Documentation/devicetree/bindings/clock/brcm,brcmstb-cpu-clk-div.txt > new file mode 100644 > index 0000000..c4acb53 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/brcm,brcmstb-cpu-clk-div.txt > @@ -0,0 +1,27 @@ > +The CPU divider node serves as the sole clock for the CPU complex. It supports > +power-of-2 clock division, with a divider of "1" as the default highest-speed > +setting. > + > +Required properties: > +- compatible: shall be "brcm,brcmstb-cpu-clk-div" > +- reg: address and width of the divider configuration register > +- #clock-cells: shall be set to 0 > +- clocks: phandle of clock provider which provides the source clock > + (this would typically be a "fixed-clock" type PLL) > +- div-table: list of (raw_value,divider) ordered pairs that correspond to the > + allowed clock divider settings > +- div-shift-width: least-significant bit position and width of divider value Are these properties used? Please don't put these types of details in DT. > + > +Optional properties: > +- clock-names: the clock may be named > + > +Example: > + cpuclkdiv: cpu-clk-div@f03e257c { > + compatible = "brcm,brcmstb-cpu-clk-div"; > + reg = <0xf03e257c 0x4>; This register really looks like some offset in something larger. Is there some clock controller? What's the hw block at 0xf03e2000? Maybe I already asked this. > + div-table = <0x00 1>; > + div-shift-width = <0 5>; > + #clock-cells = <0>; > + clocks = <&cpupll>; > + clock-names = "cpupll"; > + }; >
On Wed, Jan 18, 2017 at 04:29:32PM -0800, Markus Mayer wrote: > From: Markus Mayer <mmayer@broadcom.com> > > Add binding document for brcm,brcmstb-cpu-clk-div. > > Signed-off-by: Markus Mayer <mmayer@broadcom.com> > --- > .../bindings/clock/brcm,brcmstb-cpu-clk-div.txt | 27 ++++++++++++++++++++++ > MAINTAINERS | 1 + > 2 files changed, 28 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/brcm,brcmstb-cpu-clk-div.txt > > diff --git a/Documentation/devicetree/bindings/clock/brcm,brcmstb-cpu-clk-div.txt b/Documentation/devicetree/bindings/clock/brcm,brcmstb-cpu-clk-div.txt > new file mode 100644 > index 0000000..c4acb53 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/brcm,brcmstb-cpu-clk-div.txt > @@ -0,0 +1,27 @@ > +The CPU divider node serves as the sole clock for the CPU complex. It supports > +power-of-2 clock division, with a divider of "1" as the default highest-speed > +setting. > + > +Required properties: > +- compatible: shall be "brcm,brcmstb-cpu-clk-div" > +- reg: address and width of the divider configuration register > +- #clock-cells: shall be set to 0 > +- clocks: phandle of clock provider which provides the source clock > + (this would typically be a "fixed-clock" type PLL) > +- div-table: list of (raw_value,divider) ordered pairs that correspond to the > + allowed clock divider settings Why do you need 2 values? Can't you use the index or calculate the register value from the divider value? > +- div-shift-width: least-significant bit position and width of divider value > + > +Optional properties: > +- clock-names: the clock may be named > + > +Example: > + cpuclkdiv: cpu-clk-div@f03e257c { clock-controller@... > + compatible = "brcm,brcmstb-cpu-clk-div"; > + reg = <0xf03e257c 0x4>; > + div-table = <0x00 1>; > + div-shift-width = <0 5>; > + #clock-cells = <0>; > + clocks = <&cpupll>; > + clock-names = "cpupll"; > + }; > diff --git a/MAINTAINERS b/MAINTAINERS > index cfff2c9..690761d 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -2786,6 +2786,7 @@ M: bcm-kernel-feedback-list@broadcom.com > L: linux-pm@vger.kernel.org > S: Maintained > F: Documentation/devicetree/bindings/cpufreq/brcm,stb-avs-cpu-freq.txt > +F: Documentation/devicetree/bindings/clock/brcm,brcmstb-cpu-clk-div.txt > F: drivers/cpufreq/brcmstb* > > BROADCOM SPECIFIC AMBA DRIVER (BCMA) > -- > 2.7.4 >
On 20 January 2017 at 16:52, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 01/18, Markus Mayer wrote: >> diff --git a/Documentation/devicetree/bindings/clock/brcm,brcmstb-cpu-clk-div.txt b/Documentation/devicetree/bindings/clock/brcm,brcmstb-cpu-clk-div.txt >> new file mode 100644 >> index 0000000..c4acb53 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/clock/brcm,brcmstb-cpu-clk-div.txt >> @@ -0,0 +1,27 @@ >> +The CPU divider node serves as the sole clock for the CPU complex. It supports >> +power-of-2 clock division, with a divider of "1" as the default highest-speed >> +setting. >> + >> +Required properties: >> +- compatible: shall be "brcm,brcmstb-cpu-clk-div" >> +- reg: address and width of the divider configuration register >> +- #clock-cells: shall be set to 0 >> +- clocks: phandle of clock provider which provides the source clock >> + (this would typically be a "fixed-clock" type PLL) >> +- div-table: list of (raw_value,divider) ordered pairs that correspond to the >> + allowed clock divider settings >> +- div-shift-width: least-significant bit position and width of divider value > > Are these properties used? Please don't put these types of > details in DT. Yeah, unfortunately they are. Luckily, I think the issue can be resolved quite easily, because the user of those properties isn't involved in this series. They are currently being used by a clock driver ("drivers/clk/clk-brcmstb.c") that hasn't been upstreamed yet. I performed some code archeology. While I wasn't 100% successful in tracking down the origins of this interface, it looks like it was designed this way a while back (4+ years or so), probably before device tree best practices were developed or, at least, before they were widely known. So, what I can do is to remove the properties from the official binding. (I'll send an update to that effect shortly.) Once the binding is accepted upstream, we can work on fixing up the design of clk-brcmstb.c, so it doesn't rely on these properties anymore (and derives them from the compatible string instead), and then proceed to upstream that, as well. I don't think this approach would cause any conflicts or problems. >> + >> +Optional properties: >> +- clock-names: the clock may be named >> + >> +Example: >> + cpuclkdiv: cpu-clk-div@f03e257c { >> + compatible = "brcm,brcmstb-cpu-clk-div"; >> + reg = <0xf03e257c 0x4>; > > This register really looks like some offset in something larger. > Is there some clock controller? What's the hw block at > 0xf03e2000? Maybe I already asked this. It looks this way, but in this case, looks are deceiving. The address and the length are really correct the way they are. This memory area holds a range of only loosely related configuration registers. It's called the Bus Interface Unit Register Set and deals with configuring the CPU in general. At address 0xf03e257c, there happens to be the clock divider register we need, and it's really just one register, i.e. 4 bytes. >> + div-table = <0x00 1>; >> + div-shift-width = <0 5>; >> + #clock-cells = <0>; >> + clocks = <&cpupll>; >> + clock-names = "cpupll"; >> + }; >> > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project
On 02/01, Markus Mayer wrote: > On 20 January 2017 at 16:52, Stephen Boyd <sboyd@codeaurora.org> wrote: > > > > Are these properties used? Please don't put these types of > > details in DT. > > Yeah, unfortunately they are. Luckily, I think the issue can be > resolved quite easily, because the user of those properties isn't > involved in this series. > > They are currently being used by a clock driver > ("drivers/clk/clk-brcmstb.c") that hasn't been upstreamed yet. I > performed some code archeology. While I wasn't 100% successful in > tracking down the origins of this interface, it looks like it was > designed this way a while back (4+ years or so), probably before > device tree best practices were developed or, at least, before they > were widely known. > > So, what I can do is to remove the properties from the official > binding. (I'll send an update to that effect shortly.) Once the > binding is accepted upstream, we can work on fixing up the design of > clk-brcmstb.c, so it doesn't rely on these properties anymore (and > derives them from the compatible string instead), and then proceed to > upstream that, as well. Ok. Sounds like some cleanup needs to be done on the way upstream. > > This register really looks like some offset in something larger. > > Is there some clock controller? What's the hw block at > > 0xf03e2000? Maybe I already asked this. > > It looks this way, but in this case, looks are deceiving. The address > and the length are really correct the way they are. > > This memory area holds a range of only loosely related configuration > registers. It's called the Bus Interface Unit Register Set and deals > with configuring the CPU in general. At address 0xf03e257c, there > happens to be the clock divider register we need, and it's really just > one register, i.e. 4 bytes. We've seen this style of hardware design before. I'd prefer we make the "Bus Interface Unit Register Set" into one device node and have a driver probe for it that registers this clock. If other things need to be controlled in there then the driver will do more than just register one clock, possibly hooking into multiple frameworks. The compatible string can indicate which SoC it is if the divider register offset changes or if the register layout is a total free for all. Either way, having reg properties end in non-zero values is suspect on ARM systems because a device is usually aligned to at least a 1k boundary for proper CPU addressing and mapping of the device. We can't possibly make a smaller mapping than a page anyway, so the registers around this one register will need to be mapped with the same attributes, hence the desire to make one device for the hardware block.
On 02/03/2017 12:06 PM, Stephen Boyd wrote: > On 02/01, Markus Mayer wrote: >> On 20 January 2017 at 16:52, Stephen Boyd <sboyd@codeaurora.org> wrote: >>> >>> Are these properties used? Please don't put these types of >>> details in DT. >> >> Yeah, unfortunately they are. Luckily, I think the issue can be >> resolved quite easily, because the user of those properties isn't >> involved in this series. >> >> They are currently being used by a clock driver >> ("drivers/clk/clk-brcmstb.c") that hasn't been upstreamed yet. I >> performed some code archeology. While I wasn't 100% successful in >> tracking down the origins of this interface, it looks like it was >> designed this way a while back (4+ years or so), probably before >> device tree best practices were developed or, at least, before they >> were widely known. >> >> So, what I can do is to remove the properties from the official >> binding. (I'll send an update to that effect shortly.) Once the >> binding is accepted upstream, we can work on fixing up the design of >> clk-brcmstb.c, so it doesn't rely on these properties anymore (and >> derives them from the compatible string instead), and then proceed to >> upstream that, as well. > > Ok. Sounds like some cleanup needs to be done on the way > upstream. > >>> This register really looks like some offset in something larger. >>> Is there some clock controller? What's the hw block at >>> 0xf03e2000? Maybe I already asked this. >> >> It looks this way, but in this case, looks are deceiving. The address >> and the length are really correct the way they are. >> >> This memory area holds a range of only loosely related configuration >> registers. It's called the Bus Interface Unit Register Set and deals >> with configuring the CPU in general. At address 0xf03e257c, there >> happens to be the clock divider register we need, and it's really just >> one register, i.e. 4 bytes. > > We've seen this style of hardware design before. I'd prefer we > make the "Bus Interface Unit Register Set" into one device node > and have a driver probe for it that registers this clock. If > other things need to be controlled in there then the driver will > do more than just register one clock, possibly hooking into > multiple frameworks. The compatible string can indicate which SoC > it is if the divider register offset changes or if the register > layout is a total free for all. We already have another piece of drive code that manipulates registers in the Bus Interface Unit located in drivers/soc/bcm/brcmstb/biuctrl.c and which has little to nothing to do with the CPU's clock ratio. And actually another one being submitted that deals with the CPU's read-ahead cache. I would very much prefer we keep all of them separate and dealing with just the register offset they need to do, but that does not mean the Device Tree binding has to look that way though. The binding for the BIUCTRL register made it here: Documentation/devicetree/bindings/arm/bcm/brcm,brcmstb.txt so we should re-use that, and have a small piece of clock provided that just uses the relevant register range within that larger register space and provide the CLOCK_RATIO. Does that work? > > Either way, having reg properties end in non-zero values is > suspect on ARM systems because a device is usually aligned to at > least a 1k boundary for proper CPU addressing and mapping of the > device. We can't possibly make a smaller mapping than a page > anyway, so the registers around this one register will need to be > mapped with the same attributes, hence the desire to make one > device for the hardware block. Yes that's absolutely valid, but this is not really a problem, since ioremap() is smart enough to re-use existing mappings (or AFAIR).
On 02/03, Florian Fainelli wrote: > On 02/03/2017 12:06 PM, Stephen Boyd wrote: > > On 02/01, Markus Mayer wrote: > >> On 20 January 2017 at 16:52, Stephen Boyd <sboyd@codeaurora.org> wrote: > >>> > >>> Are these properties used? Please don't put these types of > >>> details in DT. > >> > >> Yeah, unfortunately they are. Luckily, I think the issue can be > >> resolved quite easily, because the user of those properties isn't > >> involved in this series. > >> > >> They are currently being used by a clock driver > >> ("drivers/clk/clk-brcmstb.c") that hasn't been upstreamed yet. I > >> performed some code archeology. While I wasn't 100% successful in > >> tracking down the origins of this interface, it looks like it was > >> designed this way a while back (4+ years or so), probably before > >> device tree best practices were developed or, at least, before they > >> were widely known. > >> > >> So, what I can do is to remove the properties from the official > >> binding. (I'll send an update to that effect shortly.) Once the > >> binding is accepted upstream, we can work on fixing up the design of > >> clk-brcmstb.c, so it doesn't rely on these properties anymore (and > >> derives them from the compatible string instead), and then proceed to > >> upstream that, as well. > > > > Ok. Sounds like some cleanup needs to be done on the way > > upstream. > > > >>> This register really looks like some offset in something larger. > >>> Is there some clock controller? What's the hw block at > >>> 0xf03e2000? Maybe I already asked this. > >> > >> It looks this way, but in this case, looks are deceiving. The address > >> and the length are really correct the way they are. > >> > >> This memory area holds a range of only loosely related configuration > >> registers. It's called the Bus Interface Unit Register Set and deals > >> with configuring the CPU in general. At address 0xf03e257c, there > >> happens to be the clock divider register we need, and it's really just > >> one register, i.e. 4 bytes. > > > > We've seen this style of hardware design before. I'd prefer we > > make the "Bus Interface Unit Register Set" into one device node > > and have a driver probe for it that registers this clock. If > > other things need to be controlled in there then the driver will > > do more than just register one clock, possibly hooking into > > multiple frameworks. The compatible string can indicate which SoC > > it is if the divider register offset changes or if the register > > layout is a total free for all. > > We already have another piece of drive code that manipulates registers > in the Bus Interface Unit located in drivers/soc/bcm/brcmstb/biuctrl.c > and which has little to nothing to do with the CPU's clock ratio. And > actually another one being submitted that deals with the CPU's > read-ahead cache. I would very much prefer we keep all of them separate > and dealing with just the register offset they need to do, but that does > not mean the Device Tree binding has to look that way though. > > The binding for the BIUCTRL register made it here: > > Documentation/devicetree/bindings/arm/bcm/brcm,brcmstb.txt > > so we should re-use that, and have a small piece of clock provided that > just uses the relevant register range within that larger register space > and provide the CLOCK_RATIO. Does that work? > Ok. That's fine. The existing binding will be updated to include this new subnode then for the clock component?
On 02/06/2017 02:59 PM, Stephen Boyd wrote: > On 02/03, Florian Fainelli wrote: >> On 02/03/2017 12:06 PM, Stephen Boyd wrote: >>> On 02/01, Markus Mayer wrote: >>>> On 20 January 2017 at 16:52, Stephen Boyd <sboyd@codeaurora.org> wrote: >>>>> >>>>> Are these properties used? Please don't put these types of >>>>> details in DT. >>>> >>>> Yeah, unfortunately they are. Luckily, I think the issue can be >>>> resolved quite easily, because the user of those properties isn't >>>> involved in this series. >>>> >>>> They are currently being used by a clock driver >>>> ("drivers/clk/clk-brcmstb.c") that hasn't been upstreamed yet. I >>>> performed some code archeology. While I wasn't 100% successful in >>>> tracking down the origins of this interface, it looks like it was >>>> designed this way a while back (4+ years or so), probably before >>>> device tree best practices were developed or, at least, before they >>>> were widely known. >>>> >>>> So, what I can do is to remove the properties from the official >>>> binding. (I'll send an update to that effect shortly.) Once the >>>> binding is accepted upstream, we can work on fixing up the design of >>>> clk-brcmstb.c, so it doesn't rely on these properties anymore (and >>>> derives them from the compatible string instead), and then proceed to >>>> upstream that, as well. >>> >>> Ok. Sounds like some cleanup needs to be done on the way >>> upstream. >>> >>>>> This register really looks like some offset in something larger. >>>>> Is there some clock controller? What's the hw block at >>>>> 0xf03e2000? Maybe I already asked this. >>>> >>>> It looks this way, but in this case, looks are deceiving. The address >>>> and the length are really correct the way they are. >>>> >>>> This memory area holds a range of only loosely related configuration >>>> registers. It's called the Bus Interface Unit Register Set and deals >>>> with configuring the CPU in general. At address 0xf03e257c, there >>>> happens to be the clock divider register we need, and it's really just >>>> one register, i.e. 4 bytes. >>> >>> We've seen this style of hardware design before. I'd prefer we >>> make the "Bus Interface Unit Register Set" into one device node >>> and have a driver probe for it that registers this clock. If >>> other things need to be controlled in there then the driver will >>> do more than just register one clock, possibly hooking into >>> multiple frameworks. The compatible string can indicate which SoC >>> it is if the divider register offset changes or if the register >>> layout is a total free for all. >> >> We already have another piece of drive code that manipulates registers >> in the Bus Interface Unit located in drivers/soc/bcm/brcmstb/biuctrl.c >> and which has little to nothing to do with the CPU's clock ratio. And >> actually another one being submitted that deals with the CPU's >> read-ahead cache. I would very much prefer we keep all of them separate >> and dealing with just the register offset they need to do, but that does >> not mean the Device Tree binding has to look that way though. >> >> The binding for the BIUCTRL register made it here: >> >> Documentation/devicetree/bindings/arm/bcm/brcm,brcmstb.txt >> >> so we should re-use that, and have a small piece of clock provided that >> just uses the relevant register range within that larger register space >> and provide the CLOCK_RATIO. Does that work? >> > > Ok. That's fine. The existing binding will be updated to include > this new subnode then for the clock component? Humm, I suppose we could do that yes, my original thought was to just have this CPU clock provider remap the entire BIUCTRL register range (of_iomap() etc.) and manipulate just the relevant register range of interest (CPU_CLOCK_CONFIG_REG), but if you want a sub-node to appear, we could probably do that as well.
On 02/06, Florian Fainelli wrote: > On 02/06/2017 02:59 PM, Stephen Boyd wrote: > > On 02/03, Florian Fainelli wrote: > >> > >> We already have another piece of drive code that manipulates registers > >> in the Bus Interface Unit located in drivers/soc/bcm/brcmstb/biuctrl.c > >> and which has little to nothing to do with the CPU's clock ratio. And > >> actually another one being submitted that deals with the CPU's > >> read-ahead cache. I would very much prefer we keep all of them separate > >> and dealing with just the register offset they need to do, but that does > >> not mean the Device Tree binding has to look that way though. > >> > >> The binding for the BIUCTRL register made it here: > >> > >> Documentation/devicetree/bindings/arm/bcm/brcm,brcmstb.txt > >> > >> so we should re-use that, and have a small piece of clock provided that > >> just uses the relevant register range within that larger register space > >> and provide the CLOCK_RATIO. Does that work? > >> > > > > Ok. That's fine. The existing binding will be updated to include > > this new subnode then for the clock component? > > Humm, I suppose we could do that yes, my original thought was to just > have this CPU clock provider remap the entire BIUCTRL register range > (of_iomap() etc.) and manipulate just the relevant register range of > interest (CPU_CLOCK_CONFIG_REG), but if you want a sub-node to appear, > we could probably do that as well. One node is fine as well. I thought the plan was many subnodes based on the binding document you mentioned earlier.
diff --git a/Documentation/devicetree/bindings/clock/brcm,brcmstb-cpu-clk-div.txt b/Documentation/devicetree/bindings/clock/brcm,brcmstb-cpu-clk-div.txt new file mode 100644 index 0000000..c4acb53 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/brcm,brcmstb-cpu-clk-div.txt @@ -0,0 +1,27 @@ +The CPU divider node serves as the sole clock for the CPU complex. It supports +power-of-2 clock division, with a divider of "1" as the default highest-speed +setting. + +Required properties: +- compatible: shall be "brcm,brcmstb-cpu-clk-div" +- reg: address and width of the divider configuration register +- #clock-cells: shall be set to 0 +- clocks: phandle of clock provider which provides the source clock + (this would typically be a "fixed-clock" type PLL) +- div-table: list of (raw_value,divider) ordered pairs that correspond to the + allowed clock divider settings +- div-shift-width: least-significant bit position and width of divider value + +Optional properties: +- clock-names: the clock may be named + +Example: + cpuclkdiv: cpu-clk-div@f03e257c { + compatible = "brcm,brcmstb-cpu-clk-div"; + reg = <0xf03e257c 0x4>; + div-table = <0x00 1>; + div-shift-width = <0 5>; + #clock-cells = <0>; + clocks = <&cpupll>; + clock-names = "cpupll"; + }; diff --git a/MAINTAINERS b/MAINTAINERS index cfff2c9..690761d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2786,6 +2786,7 @@ M: bcm-kernel-feedback-list@broadcom.com L: linux-pm@vger.kernel.org S: Maintained F: Documentation/devicetree/bindings/cpufreq/brcm,stb-avs-cpu-freq.txt +F: Documentation/devicetree/bindings/clock/brcm,brcmstb-cpu-clk-div.txt F: drivers/cpufreq/brcmstb* BROADCOM SPECIFIC AMBA DRIVER (BCMA)