| Message ID | 1384872347-11724-4-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 19, 2013 at 02:45:42PM +0000, Laurent Pinchart wrote: > MSTP clocks are gate clocks controlled through a register that handles > up to 32 clocks. The register is often sparsely populated. Does that mean some clocks aren't wired up, or that some clocks don't exist at all? What is the behaviour of the unpopulated bits? > > Those clocks are found on Renesas ARM SoCs. > > Cc: devicetree@vger.kernel.org > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > .../bindings/clock/renesas,cpg-mstp-clocks.txt | 48 +++++ > drivers/clk/shmobile/Makefile | 1 + > drivers/clk/shmobile/clk-mstp.c | 229 +++++++++++++++++++++ > 3 files changed, 278 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt > create mode 100644 drivers/clk/shmobile/clk-mstp.c > > diff --git a/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt > new file mode 100644 > index 0000000..126b17e > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt > @@ -0,0 +1,48 @@ > +* Renesas CPG Module Stop (MSTP) Clocks > + > +The CPG can gate SoC device clocks. The gates are organized in groups of up to > +32 gates. > + > +This device tree binding describes a single 32 gate clocks group per node. > +Clocks are referenced by user nodes by the MSTP node phandle and the clock > +index in the group, from 0 to 31. > + > +Required Properties: > + > + - compatible: Must be one of the following > + - "renesas,r8a7790-mstp-clocks" for R8A7790 (R-Car H2) MSTP gate clocks > + - "renesas,r8a7791-mstp-clocks" for R8A7791 (R-Car M2) MSTP gate clocks > + - "renesas,cpg-mstp-clock" for generic MSTP gate clocks > + - reg: Base address and length of the memory resource used by the MSTP > + clocks There are two entries in the example, the code seems to assume they are particular registers, but this implies one. Are these not part of a larger bank of registers? > + - clocks: Reference to the parent clocks How many, and what do they correspond to? > + - #clock-cells: Must be 1 > + - clock-output-names: The name of the clocks as free-form strings > + - renesas,indices: Indices of the gate clocks into the group (0 to 31) The description of this property doesn't describe half of what it means. I believe something like the below does: - renesas,indices: A list of clock IDs (single cells), one for each clock present. Each entry may correspond to a clock-output-names entry at the same index, and its location in the list defines the corresponding clock-specifier for the entry. I'd imagine we have a few sparse clocks by now, and we might be able to make this more uniform. But I may be mistaken. [...] > +static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable) > +{ > + struct mstp_clock *clock = to_mstp_clock(hw); > + struct mstp_clock_group *group = clock->group; > + u32 bitmask = BIT(clock->bit_index); > + unsigned long flags; > + unsigned int i; > + u32 value; > + > + spin_lock_irqsave(&group->lock, flags); > + > + value = clk_readl(group->smstpcr); > + if (enable) > + value &= ~bitmask; > + else > + value |= bitmask; > + clk_writel(value, group->smstpcr); > + > + spin_unlock_irqrestore(&group->lock, flags); > + > + if (!enable || !group->mstpsr) > + return 0; > + > + for (i = 1000; i > 0; --i) { > + if (!(clk_readl(group->mstpsr) & bitmask)) > + break; > + cpu_relax(); > + } Any particular reason for 1000 times? Is there a known minimum time to switch a clock between disabled and enabled? A comment would be nice. Cheers, Mark.
Hi Mark, Thank you for the quick review, much appreciated. On Tuesday 19 November 2013 16:28:21 Mark Rutland wrote: > On Tue, Nov 19, 2013 at 02:45:42PM +0000, Laurent Pinchart wrote: > > MSTP clocks are gate clocks controlled through a register that handles > > up to 32 clocks. The register is often sparsely populated. > > Does that mean some clocks aren't wired up, or that some clocks don't > exist at all? It means that some of the bits don't control anything. > What is the behaviour of the unpopulated bits? According to the datasheet they're reserved, read-only, and read an unspecified value that is to be written back without modification when writing the register. > > Those clocks are found on Renesas ARM SoCs. > > > > Cc: devicetree@vger.kernel.org > > Signed-off-by: Laurent Pinchart > > <laurent.pinchart+renesas@ideasonboard.com> > > --- > > > > .../bindings/clock/renesas,cpg-mstp-clocks.txt | 48 +++++ > > drivers/clk/shmobile/Makefile | 1 + > > drivers/clk/shmobile/clk-mstp.c | 229 ++++++++++++++++ > > 3 files changed, 278 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt > > create mode 100644 drivers/clk/shmobile/clk-mstp.c > > > > diff --git > > a/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt > > b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt new > > file mode 100644 > > index 0000000..126b17e > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt > > @@ -0,0 +1,48 @@ > > +* Renesas CPG Module Stop (MSTP) Clocks > > + > > +The CPG can gate SoC device clocks. The gates are organized in groups of > > up to > > +32 gates. > > + > > +This device tree binding describes a single 32 gate clocks group per > > node. > > +Clocks are referenced by user nodes by the MSTP node phandle and the > > clock > > +index in the group, from 0 to 31. > > + > > +Required Properties: > > + > > + - compatible: Must be one of the following > > + - "renesas,r8a7790-mstp-clocks" for R8A7790 (R-Car H2) MSTP gate > > clocks > > + - "renesas,r8a7791-mstp-clocks" for R8A7791 (R-Car M2) MSTP gate > > clocks > > + - "renesas,cpg-mstp-clock" for generic MSTP gate clocks > > + - reg: Base address and length of the memory resource used by the MSTP > > + clocks > > There are two entries in the example, the code seems to assume they are > particular registers, but this implies one. There can be one or two registers, depending on the hardware. The first register is always required and controls the clocks, the second register is optional (in the sense that it's not implemented for some of the MSTP instances) and reports the clock status. > Are these not part of a larger bank of registers? They are. Here's the memory map for the r8a7791 for instance. MSTPSR0 H'E615 0030 MSTPSR1 H'E615 0038 MSTPSR2 H'E615 0040 MSTPSR3 H'E615 0048 MSTPSR4 H'E615 004C MSTPSR5 H'E615 003C MSTPSR6 H'E615 01C0 MSTPSR7 H'E615 01C4 MSTPSR8 H'E615 09A0 MSTPSR9 H'E615 09A4 MSTPSR10 H'E615 09A8 MSTPSR11 H'E615 09AC SMSTPCR0 H'E615 0130 SMSTPCR1 H'E615 0134 SMSTPCR2 H'E615 0138 SMSTPCR3 H'E615 013C SMSTPCR4 H'E615 0140 SMSTPCR5 H'E615 0144 SMSTPCR6 H'E615 0148 SMSTPCR7 H'E615 014C SMSTPCR8 H'E615 0990 SMSTPCR9 H'E615 0994 SMSTPCR10 H'E615 0998 SMSTPCR11 H'E615 099C I've pondered whether we should use a single device node for all the MSTP clocks, but I don't think it would be very practical. Not only would we need to encode all bit positions, we also would have to encode register offsets in DT. This would become pretty messy in a single node. > > + - clocks: Reference to the parent clocks > > How many, and what do they correspond to? What about - clocks: Reference to the parent clocks, one per output clock. The parents must appear in the same order as the output clocks. > > + - #clock-cells: Must be 1 > > + - clock-output-names: The name of the clocks as free-form strings > > + - renesas,indices: Indices of the gate clocks into the group (0 to 31) > > The description of this property doesn't describe half of what it means. > I believe something like the below does: > > - renesas,indices: A list of clock IDs (single cells), one for each > clock present. Each entry may correspond to a clock-output-names entry > at the same index, and its location in the list defines the > corresponding clock-specifier for the entry. I might be mistaken, but doesn't "its location in the list defines the corresponding clock-specifier for the entry" imply that clock specifiers use the location in the list as the clock ID instead of the numerical value of the indices entry ? Each entry in the renesas,indices list corresponds to one clocks entry and one clock-output-names entry, and clock users reference an MSTP clock using the value of its indices entry, not the position of the entry. > I'd imagine we have a few sparse clocks by now, and we might be able to > make this more uniform. But I may be mistaken. > > [...] > > > +static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable) > > +{ > > + struct mstp_clock *clock = to_mstp_clock(hw); > > + struct mstp_clock_group *group = clock->group; > > + u32 bitmask = BIT(clock->bit_index); > > + unsigned long flags; > > + unsigned int i; > > + u32 value; > > + > > + spin_lock_irqsave(&group->lock, flags); > > + > > + value = clk_readl(group->smstpcr); > > + if (enable) > > + value &= ~bitmask; > > + else > > + value |= bitmask; > > + clk_writel(value, group->smstpcr); > > + > > + spin_unlock_irqrestore(&group->lock, flags); > > + > > + if (!enable || !group->mstpsr) > > + return 0; > > + > > + for (i = 1000; i > 0; --i) { > > + if (!(clk_readl(group->mstpsr) & bitmask)) > > + break; > > + cpu_relax(); > > + } > > Any particular reason for 1000 times? Is there a known minimum time to > switch a clock between disabled and enabled? A comment would be nice. There's no particular reason, the datasheet doesn't provide any useful information. I've just copied that value from existing C code.
On Tue, Nov 19, 2013 at 05:00:40PM +0000, Laurent Pinchart wrote: > Hi Mark, > > Thank you for the quick review, much appreciated. > > On Tuesday 19 November 2013 16:28:21 Mark Rutland wrote: > > On Tue, Nov 19, 2013 at 02:45:42PM +0000, Laurent Pinchart wrote: > > > MSTP clocks are gate clocks controlled through a register that handles > > > up to 32 clocks. The register is often sparsely populated. > > > > Does that mean some clocks aren't wired up, or that some clocks don't > > exist at all? > > It means that some of the bits don't control anything. > > > What is the behaviour of the unpopulated bits? > > According to the datasheet they're reserved, read-only, and read an > unspecified value that is to be written back without modification when writing > the register. Ok. > > > > Those clocks are found on Renesas ARM SoCs. > > > > > > Cc: devicetree@vger.kernel.org > > > Signed-off-by: Laurent Pinchart > > > <laurent.pinchart+renesas@ideasonboard.com> > > > --- > > > > > > .../bindings/clock/renesas,cpg-mstp-clocks.txt | 48 +++++ > > > drivers/clk/shmobile/Makefile | 1 + > > > drivers/clk/shmobile/clk-mstp.c | 229 ++++++++++++++++ > > > 3 files changed, 278 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt > > > create mode 100644 drivers/clk/shmobile/clk-mstp.c > > > > > > diff --git > > > a/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt > > > b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt new > > > file mode 100644 > > > index 0000000..126b17e > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt > > > @@ -0,0 +1,48 @@ > > > +* Renesas CPG Module Stop (MSTP) Clocks > > > + > > > +The CPG can gate SoC device clocks. The gates are organized in groups of > > > up to > > > +32 gates. > > > + > > > +This device tree binding describes a single 32 gate clocks group per > > > node. > > > +Clocks are referenced by user nodes by the MSTP node phandle and the > > > clock > > > +index in the group, from 0 to 31. > > > + > > > +Required Properties: > > > + > > > + - compatible: Must be one of the following > > > + - "renesas,r8a7790-mstp-clocks" for R8A7790 (R-Car H2) MSTP gate > > > clocks > > > + - "renesas,r8a7791-mstp-clocks" for R8A7791 (R-Car M2) MSTP gate > > > clocks > > > + - "renesas,cpg-mstp-clock" for generic MSTP gate clocks > > > + - reg: Base address and length of the memory resource used by the MSTP > > > + clocks > > > > There are two entries in the example, the code seems to assume they are > > particular registers, but this implies one. > > There can be one or two registers, depending on the hardware. The first > register is always required and controls the clocks, the second register is > optional (in the sense that it's not implemented for some of the MSTP > instances) and reports the clock status. Ok. The documentation should note this. > > > Are these not part of a larger bank of registers? > > They are. Here's the memory map for the r8a7791 for instance. > > MSTPSR0 H'E615 0030 > MSTPSR1 H'E615 0038 > MSTPSR2 H'E615 0040 > MSTPSR3 H'E615 0048 > MSTPSR4 H'E615 004C > MSTPSR5 H'E615 003C > MSTPSR6 H'E615 01C0 > MSTPSR7 H'E615 01C4 > MSTPSR8 H'E615 09A0 > MSTPSR9 H'E615 09A4 > MSTPSR10 H'E615 09A8 > MSTPSR11 H'E615 09AC > > SMSTPCR0 H'E615 0130 > SMSTPCR1 H'E615 0134 > SMSTPCR2 H'E615 0138 > SMSTPCR3 H'E615 013C > SMSTPCR4 H'E615 0140 > SMSTPCR5 H'E615 0144 > SMSTPCR6 H'E615 0148 > SMSTPCR7 H'E615 014C > SMSTPCR8 H'E615 0990 > SMSTPCR9 H'E615 0994 > SMSTPCR10 H'E615 0998 > SMSTPCR11 H'E615 099C > > I've pondered whether we should use a single device node for all the MSTP > clocks, but I don't think it would be very practical. Not only would we need > to encode all bit positions, we also would have to encode register offsets in > DT. This would become pretty messy in a single node. > > > > + - clocks: Reference to the parent clocks > > > > How many, and what do they correspond to? > > What about > > - clocks: Reference to the parent clocks, one per output clock. The parents > must appear in the same order as the output clocks. That sounds reasonable. > > > > + - #clock-cells: Must be 1 > > > + - clock-output-names: The name of the clocks as free-form strings > > > + - renesas,indices: Indices of the gate clocks into the group (0 to 31) > > > > The description of this property doesn't describe half of what it means. > > I believe something like the below does: > > > > - renesas,indices: A list of clock IDs (single cells), one for each > > clock present. Each entry may correspond to a clock-output-names entry > > at the same index, and its location in the list defines the > > corresponding clock-specifier for the entry. > > I might be mistaken, but doesn't "its location in the list defines the > corresponding clock-specifier for the entry" imply that clock specifiers use > the location in the list as the clock ID instead of the numerical value of the > indices entry ? Each entry in the renesas,indices list corresponds to one > clocks entry and one clock-output-names entry, and clock users reference an > MSTP clock using the value of its indices entry, not the position of the > entry. Sorry, the code confused me. Having read over it again, I'm even more confused. You said the registers were sparesly populated, but the code assumes that the bits start at 0 and the set of populated bits are contiguous, which is not what I would describe that as sparse. Are the bits always contiguous? I thought the bits in the register were truly sparse (i.e. you could have two bits which were at either end of the register), and the indices property allowed you to define the set of bits which were valid. As far as I can see from cpg_mstp_clocks_init and cpg_mstp_clock_register, the indices property assings the set of IDs for consumers to use, and I don't see what the point of that is if we already have a well-defined linear index (the bit position in the register) which we could use instead, which also makes the binding and code simpler. > > > I'd imagine we have a few sparse clocks by now, and we might be able to > > make this more uniform. But I may be mistaken. > > > > [...] > > > > > +static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable) > > > +{ > > > + struct mstp_clock *clock = to_mstp_clock(hw); > > > + struct mstp_clock_group *group = clock->group; > > > + u32 bitmask = BIT(clock->bit_index); > > > + unsigned long flags; > > > + unsigned int i; > > > + u32 value; > > > + > > > + spin_lock_irqsave(&group->lock, flags); > > > + > > > + value = clk_readl(group->smstpcr); > > > + if (enable) > > > + value &= ~bitmask; > > > + else > > > + value |= bitmask; > > > + clk_writel(value, group->smstpcr); > > > + > > > + spin_unlock_irqrestore(&group->lock, flags); > > > + > > > + if (!enable || !group->mstpsr) > > > + return 0; > > > + > > > + for (i = 1000; i > 0; --i) { > > > + if (!(clk_readl(group->mstpsr) & bitmask)) > > > + break; > > > + cpu_relax(); > > > + } > > > > Any particular reason for 1000 times? Is there a known minimum time to > > switch a clock between disabled and enabled? A comment would be nice. > > There's no particular reason, the datasheet doesn't provide any useful > information. I've just copied that value from existing C code. Any chance it might work without? Thanks, Mark.
Hi Mark, On Tuesday 19 November 2013 18:19:36 Mark Rutland wrote: > On Tue, Nov 19, 2013 at 05:00:40PM +0000, Laurent Pinchart wrote: > > On Tuesday 19 November 2013 16:28:21 Mark Rutland wrote: > > > On Tue, Nov 19, 2013 at 02:45:42PM +0000, Laurent Pinchart wrote: > > > > MSTP clocks are gate clocks controlled through a register that handles > > > > up to 32 clocks. The register is often sparsely populated. > > > > > > Does that mean some clocks aren't wired up, or that some clocks don't > > > exist at all? > > > > It means that some of the bits don't control anything. > > > > > What is the behaviour of the unpopulated bits? > > > > According to the datasheet they're reserved, read-only, and read an > > unspecified value that is to be written back without modification when > > writing the register. > > Ok. > > > > > Those clocks are found on Renesas ARM SoCs. > > > > > > > > Cc: devicetree@vger.kernel.org > > > > Signed-off-by: Laurent Pinchart > > > > <laurent.pinchart+renesas@ideasonboard.com> > > > > --- > > > > > > > > .../bindings/clock/renesas,cpg-mstp-clocks.txt | 48 +++++ > > > > drivers/clk/shmobile/Makefile | 1 + > > > > drivers/clk/shmobile/clk-mstp.c | 229 ++++++++++++ > > > > 3 files changed, 278 insertions(+) > > > > create mode 100644 > > > > Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt > > > > create mode 100644 drivers/clk/shmobile/clk-mstp.c > > > > > > > > diff --git > > > > a/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt > > > > b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt > > > > new file mode 100644 > > > > index 0000000..126b17e > > > > --- /dev/null > > > > +++ > > > > b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt > > > > @@ -0,0 +1,48 @@ > > > > +* Renesas CPG Module Stop (MSTP) Clocks > > > > + > > > > +The CPG can gate SoC device clocks. The gates are organized in groups > > > > of up to > > > > +32 gates. > > > > + > > > > +This device tree binding describes a single 32 gate clocks group per > > > > node. > > > > +Clocks are referenced by user nodes by the MSTP node phandle and the > > > > clock > > > > +index in the group, from 0 to 31. > > > > + > > > > +Required Properties: > > > > + > > > > + - compatible: Must be one of the following > > > > + - "renesas,r8a7790-mstp-clocks" for R8A7790 (R-Car H2) MSTP gate > > > > clocks > > > > + - "renesas,r8a7791-mstp-clocks" for R8A7791 (R-Car M2) MSTP gate > > > > clocks > > > > + - "renesas,cpg-mstp-clock" for generic MSTP gate clocks > > > > + - reg: Base address and length of the memory resource used by the > > > > MSTP > > > > + clocks > > > > > > There are two entries in the example, the code seems to assume they are > > > particular registers, but this implies one. > > > > There can be one or two registers, depending on the hardware. The first > > register is always required and controls the clocks, the second register > > is optional (in the sense that it's not implemented for some of the MSTP > > instances) and reports the clock status. > > Ok. The documentation should note this. What about - reg: Base address and length of the I/O mapped registers used by the MSTP clocks. The first register is the clock control register and is mandatory. The second register is the clock status register and is optional when not implemented in hardware. > > > Are these not part of a larger bank of registers? > > > > They are. Here's the memory map for the r8a7791 for instance. > > > > MSTPSR0 H'E615 0030 > > MSTPSR1 H'E615 0038 > > MSTPSR2 H'E615 0040 > > MSTPSR3 H'E615 0048 > > MSTPSR4 H'E615 004C > > MSTPSR5 H'E615 003C > > MSTPSR6 H'E615 01C0 > > MSTPSR7 H'E615 01C4 > > MSTPSR8 H'E615 09A0 > > MSTPSR9 H'E615 09A4 > > MSTPSR10 H'E615 09A8 > > MSTPSR11 H'E615 09AC > > > > SMSTPCR0 H'E615 0130 > > SMSTPCR1 H'E615 0134 > > SMSTPCR2 H'E615 0138 > > SMSTPCR3 H'E615 013C > > SMSTPCR4 H'E615 0140 > > SMSTPCR5 H'E615 0144 > > SMSTPCR6 H'E615 0148 > > SMSTPCR7 H'E615 014C > > SMSTPCR8 H'E615 0990 > > SMSTPCR9 H'E615 0994 > > SMSTPCR10 H'E615 0998 > > SMSTPCR11 H'E615 099C > > > > I've pondered whether we should use a single device node for all the MSTP > > clocks, but I don't think it would be very practical. Not only would we > > need to encode all bit positions, we also would have to encode register > > offsets in DT. This would become pretty messy in a single node. > > > > > > + - clocks: Reference to the parent clocks > > > > > > How many, and what do they correspond to? > > > > What about > > > > - clocks: Reference to the parent clocks, one per output clock. The > > parents must appear in the same order as the output clocks. > > That sounds reasonable. > > > > > + - #clock-cells: Must be 1 > > > > + - clock-output-names: The name of the clocks as free-form strings > > > > + - renesas,indices: Indices of the gate clocks into the group (0 to > > > > 31) > > > > > > The description of this property doesn't describe half of what it means. > > > I believe something like the below does: > > > > > > - renesas,indices: A list of clock IDs (single cells), one for each > > > > > > clock present. Each entry may correspond to a clock-output-names entry > > > at the same index, and its location in the list defines the > > > corresponding clock-specifier for the entry. > > > > I might be mistaken, but doesn't "its location in the list defines the > > corresponding clock-specifier for the entry" imply that clock specifiers > > use the location in the list as the clock ID instead of the numerical > > value of the indices entry ? Each entry in the renesas,indices list > > corresponds to one clocks entry and one clock-output-names entry, and > > clock users reference an MSTP clock using the value of its indices entry, > > not the position of the entry. > > Sorry, the code confused me. Having read over it again, I'm even more > confused. > > You said the registers were sparesly populated, but the code assumes > that the bits start at 0 and the set of populated bits are contiguous, > which is not what I would describe that as sparse. Are the bits always > contiguous? They're not, and the code doesn't assume so. The driver loops over all the clocks using the position in the renesas,indices array as the loop counter i. It then retrieves the output name, the parent and the index and registers the clock. When registration succeeds, the clock is stored in the clks array at the index corresponding to the renesas,indices value. The clks array is thus sparsely populated, exactly as the MSTP register. > I thought the bits in the register were truly sparse (i.e. you could > have two bits which were at either end of the register), and the indices > property allowed you to define the set of bits which were valid. > > As far as I can see from cpg_mstp_clocks_init and > cpg_mstp_clock_register, the indices property assings the set of IDs for > consumers to use, and I don't see what the point of that is if we > already have a well-defined linear index (the bit position in the > register) which we could use instead, which also makes the binding and > code simpler. > > > > I'd imagine we have a few sparse clocks by now, and we might be able to > > > make this more uniform. But I may be mistaken. > > > > > > [...] > > > > > > > +static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable) > > > > +{ > > > > + struct mstp_clock *clock = to_mstp_clock(hw); > > > > + struct mstp_clock_group *group = clock->group; > > > > + u32 bitmask = BIT(clock->bit_index); > > > > + unsigned long flags; > > > > + unsigned int i; > > > > + u32 value; > > > > + > > > > + spin_lock_irqsave(&group->lock, flags); > > > > + > > > > + value = clk_readl(group->smstpcr); > > > > + if (enable) > > > > + value &= ~bitmask; > > > > + else > > > > + value |= bitmask; > > > > + clk_writel(value, group->smstpcr); > > > > + > > > > + spin_unlock_irqrestore(&group->lock, flags); > > > > + > > > > + if (!enable || !group->mstpsr) > > > > + return 0; > > > > + > > > > + for (i = 1000; i > 0; --i) { > > > > + if (!(clk_readl(group->mstpsr) & bitmask)) > > > > + break; > > > > + cpu_relax(); > > > > + } > > > > > > Any particular reason for 1000 times? Is there a known minimum time to > > > switch a clock between disabled and enabled? A comment would be nice. > > > > There's no particular reason, the datasheet doesn't provide any useful > > information. I've just copied that value from existing C code. > > Any chance it might work without? Yes, with roughly a 75% chance :-) Without this look I get a crash around once every four times with the video processing devices. Other devices are affected as well, but delays in their respective drivers hide the problem.
Hi Mark, I'd like to send v4 of this series soon, hopefully for the last time. Would you be able to reply to the two issues that are still left and discussed below ? That would be really appreciated. On Wednesday 20 November 2013 22:54:58 Laurent Pinchart wrote: > On Tuesday 19 November 2013 18:19:36 Mark Rutland wrote: > > On Tue, Nov 19, 2013 at 05:00:40PM +0000, Laurent Pinchart wrote: > > > On Tuesday 19 November 2013 16:28:21 Mark Rutland wrote: > > > > On Tue, Nov 19, 2013 at 02:45:42PM +0000, Laurent Pinchart wrote: > > > > > MSTP clocks are gate clocks controlled through a register that > > > > > handles up to 32 clocks. The register is often sparsely populated. > > > > > > > > Does that mean some clocks aren't wired up, or that some clocks don't > > > > exist at all? > > > > > > It means that some of the bits don't control anything. > > > > > > > What is the behaviour of the unpopulated bits? > > > > > > According to the datasheet they're reserved, read-only, and read an > > > unspecified value that is to be written back without modification when > > > writing the register. > > > > Ok. > > > > > > > Those clocks are found on Renesas ARM SoCs. > > > > > > > > > > Cc: devicetree@vger.kernel.org > > > > > Signed-off-by: Laurent Pinchart > > > > > <laurent.pinchart+renesas@ideasonboard.com> > > > > > --- > > > > > > > > > > .../bindings/clock/renesas,cpg-mstp-clocks.txt | 48 +++++ > > > > > drivers/clk/shmobile/Makefile | 1 + > > > > > drivers/clk/shmobile/clk-mstp.c | 229 ++++++++++ > > > > > 3 files changed, 278 insertions(+) > > > > > create mode 100644 > > > > > Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt > > > > > create mode 100644 drivers/clk/shmobile/clk-mstp.c > > > > > > > > > > diff --git > > > > > a/Documentation/devicetree/bindings/clock/renesas,cpg-mstp- > > > > > clocks.txt > > > > > b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp- > > > > > clocks.txt > > > > > new file mode 100644 > > > > > index 0000000..126b17e > > > > > --- /dev/null > > > > > +++ > > > > > b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp- > > > > > clocks.txt > > > > > @@ -0,0 +1,48 @@ > > > > > +* Renesas CPG Module Stop (MSTP) Clocks > > > > > + > > > > > +The CPG can gate SoC device clocks. The gates are organized in > > > > > groups of up to > > > > > +32 gates. > > > > > + > > > > > +This device tree binding describes a single 32 gate clocks group > > > > > per node. > > > > > +Clocks are referenced by user nodes by the MSTP node phandle and > > > > > the clock > > > > > +index in the group, from 0 to 31. > > > > > + > > > > > +Required Properties: > > > > > + > > > > > + - compatible: Must be one of the following > > > > > + - "renesas,r8a7790-mstp-clocks" for R8A7790 (R-Car H2) MSTP > > > > > gate clocks > > > > > + - "renesas,r8a7791-mstp-clocks" for R8A7791 (R-Car M2) MSTP > > > > > gate clocks > > > > > + - "renesas,cpg-mstp-clock" for generic MSTP gate clocks > > > > > + - reg: Base address and length of the memory resource used by the > > > > > MSTP > > > > > + clocks > > > > > > > > There are two entries in the example, the code seems to assume they > > > > are particular registers, but this implies one. > > > > > > There can be one or two registers, depending on the hardware. The first > > > register is always required and controls the clocks, the second register > > > is optional (in the sense that it's not implemented for some of the MSTP > > > instances) and reports the clock status. > > > > Ok. The documentation should note this. > > What about > > - reg: Base address and length of the I/O mapped registers used by the > MSTP clocks. The first register is the clock control register and is > mandatory. The second register is the clock status register and is optional > when not implemented in hardware. > > > > > Are these not part of a larger bank of registers? > > > > > > They are. Here's the memory map for the r8a7791 for instance. > > > > > > MSTPSR0 H'E615 0030 > > > MSTPSR1 H'E615 0038 > > > MSTPSR2 H'E615 0040 > > > MSTPSR3 H'E615 0048 > > > MSTPSR4 H'E615 004C > > > MSTPSR5 H'E615 003C > > > MSTPSR6 H'E615 01C0 > > > MSTPSR7 H'E615 01C4 > > > MSTPSR8 H'E615 09A0 > > > MSTPSR9 H'E615 09A4 > > > MSTPSR10 H'E615 09A8 > > > MSTPSR11 H'E615 09AC > > > > > > SMSTPCR0 H'E615 0130 > > > SMSTPCR1 H'E615 0134 > > > SMSTPCR2 H'E615 0138 > > > SMSTPCR3 H'E615 013C > > > SMSTPCR4 H'E615 0140 > > > SMSTPCR5 H'E615 0144 > > > SMSTPCR6 H'E615 0148 > > > SMSTPCR7 H'E615 014C > > > SMSTPCR8 H'E615 0990 > > > SMSTPCR9 H'E615 0994 > > > SMSTPCR10 H'E615 0998 > > > SMSTPCR11 H'E615 099C > > > > > > I've pondered whether we should use a single device node for all the > > > MSTP clocks, but I don't think it would be very practical. Not only > > > would we need to encode all bit positions, we also would have to encode > > > register offsets in DT. This would become pretty messy in a single node. > > > > > > > > + - clocks: Reference to the parent clocks > > > > > > > > How many, and what do they correspond to? > > > > > > What about > > > > > > - clocks: Reference to the parent clocks, one per output clock. The > > > parents must appear in the same order as the output clocks. > > > > That sounds reasonable. > > > > > > > + - #clock-cells: Must be 1 > > > > > + - clock-output-names: The name of the clocks as free-form strings > > > > > + - renesas,indices: Indices of the gate clocks into the group (0 > > > > > to 31) > > > > > > > > The description of this property doesn't describe half of what it > > > > means. I believe something like the below does: > > > > > > > > - renesas,indices: A list of clock IDs (single cells), one for each > > > > clock present. Each entry may correspond to a clock-output-names > > > > entry at the same index, and its location in the list defines the > > > > corresponding clock-specifier for the entry. > > > > > > I might be mistaken, but doesn't "its location in the list defines the > > > corresponding clock-specifier for the entry" imply that clock specifiers > > > use the location in the list as the clock ID instead of the numerical > > > value of the indices entry ? Each entry in the renesas,indices list > > > corresponds to one clocks entry and one clock-output-names entry, and > > > clock users reference an MSTP clock using the value of its indices > > > entry, not the position of the entry. > > > > Sorry, the code confused me. Having read over it again, I'm even more > > confused. > > > > You said the registers were sparesly populated, but the code assumes > > that the bits start at 0 and the set of populated bits are contiguous, > > which is not what I would describe that as sparse. Are the bits always > > contiguous? > > They're not, and the code doesn't assume so. The driver loops over all the > clocks using the position in the renesas,indices array as the loop counter > i. It then retrieves the output name, the parent and the index and > registers the clock. When registration succeeds, the clock is stored in the > clks array at the index corresponding to the renesas,indices value. The > clks array is thus sparsely populated, exactly as the MSTP register. > > > I thought the bits in the register were truly sparse (i.e. you could > > have two bits which were at either end of the register), and the indices > > property allowed you to define the set of bits which were valid. > > > > As far as I can see from cpg_mstp_clocks_init and > > cpg_mstp_clock_register, the indices property assings the set of IDs for > > consumers to use, and I don't see what the point of that is if we > > already have a well-defined linear index (the bit position in the > > register) which we could use instead, which also makes the binding and > > code simpler. > > > > > > I'd imagine we have a few sparse clocks by now, and we might be able > > > > to make this more uniform. But I may be mistaken. > > > > > > > > [...] > > > > > > > > > +static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable) > > > > > +{ > > > > > + struct mstp_clock *clock = to_mstp_clock(hw); > > > > > + struct mstp_clock_group *group = clock->group; > > > > > + u32 bitmask = BIT(clock->bit_index); > > > > > + unsigned long flags; > > > > > + unsigned int i; > > > > > + u32 value; > > > > > + > > > > > + spin_lock_irqsave(&group->lock, flags); > > > > > + > > > > > + value = clk_readl(group->smstpcr); > > > > > + if (enable) > > > > > + value &= ~bitmask; > > > > > + else > > > > > + value |= bitmask; > > > > > + clk_writel(value, group->smstpcr); > > > > > + > > > > > + spin_unlock_irqrestore(&group->lock, flags); > > > > > + > > > > > + if (!enable || !group->mstpsr) > > > > > + return 0; > > > > > + > > > > > + for (i = 1000; i > 0; --i) { > > > > > + if (!(clk_readl(group->mstpsr) & bitmask)) > > > > > + break; > > > > > + cpu_relax(); > > > > > + } > > > > > > > > Any particular reason for 1000 times? Is there a known minimum time to > > > > switch a clock between disabled and enabled? A comment would be nice. > > > > > > There's no particular reason, the datasheet doesn't provide any useful > > > information. I've just copied that value from existing C code. > > > > Any chance it might work without? > > Yes, with roughly a 75% chance :-) Without this look I get a crash around > once every four times with the video processing devices. Other devices are > affected as well, but delays in their respective drivers hide the problem.
diff --git a/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt new file mode 100644 index 0000000..126b17e --- /dev/null +++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt @@ -0,0 +1,48 @@ +* Renesas CPG Module Stop (MSTP) Clocks + +The CPG can gate SoC device clocks. The gates are organized in groups of up to +32 gates. + +This device tree binding describes a single 32 gate clocks group per node. +Clocks are referenced by user nodes by the MSTP node phandle and the clock +index in the group, from 0 to 31. + +Required Properties: + + - compatible: Must be one of the following + - "renesas,r8a7790-mstp-clocks" for R8A7790 (R-Car H2) MSTP gate clocks + - "renesas,r8a7791-mstp-clocks" for R8A7791 (R-Car M2) MSTP gate clocks + - "renesas,cpg-mstp-clock" for generic MSTP gate clocks + - reg: Base address and length of the memory resource used by the MSTP + clocks + - clocks: Reference to the parent clocks + - #clock-cells: Must be 1 + - clock-output-names: The name of the clocks as free-form strings + - renesas,indices: Indices of the gate clocks into the group (0 to 31) + +The clocks, clock-output-names and renesas,indices properties contain one +entry per gate clock. The MSTP groups are sparsely populated. Unimplemented +gate clocks must not be declared. + + +Example +------- + + #include <dt-bindings/clock/r8a7790-clock.h> + + mstp3_clks: mstp3_clks@e615013c { + compatible = "renesas,r8a7790-mstp-clocks", "renesas,cpg-mstp-clocks"; + reg = <0 0xe615013c 0 4>, <0 0xe6150048 0 4>; + clocks = <&cp_clk>, <&mmc1_clk>, <&sd3_clk>, <&sd2_clk>, + <&cpg_clocks R8A7790_CLK_SD1>, <&cpg_clocks R8A7790_CLK_SD0>, + <&mmc0_clk>; + #clock-cells = <1>; + clock-output-names = + "tpu0", "mmcif1", "sdhi3", "sdhi2", + "sdhi1", "sdhi0", "mmcif0"; + renesas,clock-indices = < + R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1 R8A7790_CLK_SDHI3 + R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0 + R8A7790_CLK_MMCIF0 + >; + }; diff --git a/drivers/clk/shmobile/Makefile b/drivers/clk/shmobile/Makefile index eded163..4df35a0 100644 --- a/drivers/clk/shmobile/Makefile +++ b/drivers/clk/shmobile/Makefile @@ -2,6 +2,7 @@ obj-$(CONFIG_ARCH_EMEV2) += clk-emev2.o obj-$(CONFIG_ARCH_R8A7790) += clk-rcar-gen2.o obj-$(CONFIG_ARCH_R8A7791) += clk-rcar-gen2.o obj-$(CONFIG_ARCH_SHMOBILE_MULTI) += clk-div6.o +obj-$(CONFIG_ARCH_SHMOBILE_MULTI) += clk-mstp.o # for emply built-in.o obj-n := dummy diff --git a/drivers/clk/shmobile/clk-mstp.c b/drivers/clk/shmobile/clk-mstp.c new file mode 100644 index 0000000..e576b60 --- /dev/null +++ b/drivers/clk/shmobile/clk-mstp.c @@ -0,0 +1,229 @@ +/* + * R-Car MSTP clocks + * + * Copyright (C) 2013 Ideas On Board SPRL + * + * Contact: Laurent Pinchart <laurent.pinchart@ideasonboard.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + */ + +#include <linux/clk-provider.h> +#include <linux/clkdev.h> +#include <linux/io.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/spinlock.h> + +/* + * MSTP clocks. We can't use standard gate clocks as we need to poll on the + * status register when enabling the clock. + */ + +#define MSTP_MAX_CLOCKS 32 + +/** + * struct mstp_clock_group - MSTP gating clocks group + * + * @data: clocks in this group + * @smstpcr: module stop control register + * @mstpsr: module stop status register (optional) + * @lock: protects writes to SMSTPCR + */ +struct mstp_clock_group { + struct clk_onecell_data data; + void __iomem *smstpcr; + void __iomem *mstpsr; + spinlock_t lock; +}; + +/** + * struct mstp_clock - MSTP gating clock + * @hw: handle between common and hardware-specific interfaces + * @bit_index: control bit index + * @group: MSTP clocks group + */ +struct mstp_clock { + struct clk_hw hw; + u32 bit_index; + struct mstp_clock_group *group; +}; + +#define to_mstp_clock(_hw) container_of(_hw, struct mstp_clock, hw) + +static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable) +{ + struct mstp_clock *clock = to_mstp_clock(hw); + struct mstp_clock_group *group = clock->group; + u32 bitmask = BIT(clock->bit_index); + unsigned long flags; + unsigned int i; + u32 value; + + spin_lock_irqsave(&group->lock, flags); + + value = clk_readl(group->smstpcr); + if (enable) + value &= ~bitmask; + else + value |= bitmask; + clk_writel(value, group->smstpcr); + + spin_unlock_irqrestore(&group->lock, flags); + + if (!enable || !group->mstpsr) + return 0; + + for (i = 1000; i > 0; --i) { + if (!(clk_readl(group->mstpsr) & bitmask)) + break; + cpu_relax(); + } + + if (!i) { + pr_err("%s: failed to enable %p[%d]\n", __func__, + group->smstpcr, clock->bit_index); + return -ETIMEDOUT; + } + + return 0; +} + +static int cpg_mstp_clock_enable(struct clk_hw *hw) +{ + return cpg_mstp_clock_endisable(hw, true); +} + +static void cpg_mstp_clock_disable(struct clk_hw *hw) +{ + cpg_mstp_clock_endisable(hw, false); +} + +static int cpg_mstp_clock_is_enabled(struct clk_hw *hw) +{ + struct mstp_clock *clock = to_mstp_clock(hw); + struct mstp_clock_group *group = clock->group; + u32 value; + + if (group->mstpsr) + value = clk_readl(group->mstpsr); + else + value = clk_readl(group->smstpcr); + + return !!(value & BIT(clock->bit_index)); +} + +static const struct clk_ops cpg_mstp_clock_ops = { + .enable = cpg_mstp_clock_enable, + .disable = cpg_mstp_clock_disable, + .is_enabled = cpg_mstp_clock_is_enabled, +}; + +static struct clk * __init +cpg_mstp_clock_register(const char *name, const char *parent_name, + unsigned int index, struct mstp_clock_group *group) +{ + struct clk_init_data init; + struct mstp_clock *clock; + struct clk *clk; + + clock = kzalloc(sizeof(*clock), GFP_KERNEL); + if (!clock) { + pr_err("%s: failed to allocate MSTP clock.\n", __func__); + return ERR_PTR(-ENOMEM); + } + + init.name = name; + init.ops = &cpg_mstp_clock_ops; + init.flags = CLK_IS_BASIC; + init.parent_names = &parent_name; + init.num_parents = 1; + + clock->bit_index = index; + clock->group = group; + clock->hw.init = &init; + + clk = clk_register(NULL, &clock->hw); + + if (IS_ERR(clk)) + kfree(clock); + + return clk; +} + +static void __init cpg_mstp_clocks_init(struct device_node *np) +{ + struct mstp_clock_group *group; + struct clk **clks; + unsigned int i; + + group = kzalloc(sizeof(*group), GFP_KERNEL); + clks = kzalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL); + if (group == NULL || clks == NULL) { + kfree(group); + kfree(clks); + pr_err("%s: failed to allocate group\n", __func__); + return; + } + + spin_lock_init(&group->lock); + group->data.clks = clks; + + group->smstpcr = of_iomap(np, 0); + group->mstpsr = of_iomap(np, 1); + + if (group->smstpcr == NULL) { + pr_err("%s: failed to remap SMSTPCR\n", __func__); + kfree(group); + kfree(clks); + return; + } + + for (i = 0; i < MSTP_MAX_CLOCKS; ++i) { + const char *parent_name; + const char *name; + u32 clkidx; + int ret; + + /* Skip clocks with no name. */ + ret = of_property_read_string_index(np, "clock-output-names", + i, &name); + if (ret < 0 || strlen(name) == 0) + continue; + + parent_name = of_clk_get_parent_name(np, i); + ret = of_property_read_u32_index(np, "renesas,clock-indices", i, + &clkidx); + if (parent_name == NULL || ret < 0) + break; + + if (clkidx >= MSTP_MAX_CLOCKS) { + pr_err("%s: invalid clock %s %s index %u)\n", + __func__, np->name, name, clkidx); + continue; + } + + clks[clkidx] = cpg_mstp_clock_register(name, parent_name, i, + group); + if (!IS_ERR(clks[clkidx])) { + group->data.clk_num = max(group->data.clk_num, clkidx); + /* + * Register a clkdev to let board code retrieve the + * clock by name and register aliases for non-DT + * devices. + * + * FIXME: Remove this when all devices that require a + * clock will be instantiated from DT. + */ + clk_register_clkdev(clks[clkidx], name, NULL); + } else { + pr_err("%s: failed to register %s %s clock (%ld)\n", + __func__, np->name, name, PTR_ERR(clks[clkidx])); + } + } + + of_clk_add_provider(np, of_clk_src_onecell_get, &group->data); +} +CLK_OF_DECLARE(cpg_mstp_clks, "renesas,cpg-mstp-clocks", cpg_mstp_clocks_init);
MSTP clocks are gate clocks controlled through a register that handles up to 32 clocks. The register is often sparsely populated. Those clocks are found on Renesas ARM SoCs. Cc: devicetree@vger.kernel.org Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- .../bindings/clock/renesas,cpg-mstp-clocks.txt | 48 +++++ drivers/clk/shmobile/Makefile | 1 + drivers/clk/shmobile/clk-mstp.c | 229 +++++++++++++++++++++ 3 files changed, 278 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt create mode 100644 drivers/clk/shmobile/clk-mstp.c