Re: [PATCH 1/2] dt-bindings: reset: introduce toshiba,tc9564-reset.yaml

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 11937 bytes --]

On Wed, Oct 21, 2026 at 02:50:41PM -0500, Alex Elder wrote:
> On 10/13/26 12:51 PM, Conor Dooley wrote:
> > On Tue, Oct 13, 2026 at 08:22:50AM -0500, Alex Elder wrote:
> > > On 10/12/26 4:22 PM, Conor Dooley wrote:
> > > > On Mon, Oct 12, 2026 at 02:10:01PM -0500, Alex Elder wrote:
> > > > > On 10/12/26 1:51 PM, Conor Dooley wrote:
> > > > > > On Mon, Oct 12, 2026 at 11:43:52AM -0500, Alex Elder wrote:
> > > > > > > Define the binding for the reset controller functionality present in
> > > > > > > the Toshiba TC9564 SoC.
> > > > > > > 
> > > > > > > Co-developed-by: Daniel Thompson <daniel@riscstar.com>
> > > > > > > Signed-off-by: Daniel Thompson <daniel@riscstar.com>
> > > > > > > Signed-off-by: Alex Elder <elder@riscstar.com>
> > > > > > > ---
> 
> Finally back to looking at this again.  Sorry for the delay.
> 
> > > > > > >     .../bindings/reset/toshiba,tc9564-reset.yaml  | 56 +++++++++++++++++++
> > > > > > >     MAINTAINERS                                   |  7 +++
> > > > > > >     include/dt-bindings/reset/toshiba,tc9564.h    | 26 +++++++++
> > > > > > >     3 files changed, 89 insertions(+)
> > > > > > >     create mode 100644 Documentation/devicetree/bindings/reset/toshiba,tc9564-reset.yaml
> > > > > > >     create mode 100644 include/dt-bindings/reset/toshiba,tc9564.h
> > > > > > > 
> > > > > > > diff --git a/Documentation/devicetree/bindings/reset/toshiba,tc9564-reset.yaml b/Documentation/devicetree/bindings/reset/toshiba,tc9564-reset.yaml
> > > > > > > new file mode 100644
> > > > > > > index 0000000000000..1908e9539abac
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/devicetree/bindings/reset/toshiba,tc9564-reset.yaml
> > > > > > > @@ -0,0 +1,56 @@
> > > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > > > +%YAML 1.2
> > > > > > > +---
> > > > > > > +$id: http://devicetree.org/schemas/reset/toshiba,tc9564-reset.yaml#
> > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > +
> > > > > > > +title: Toshiba TC9564 Reset Controller
> > > > > > > +
> > > > > > > +maintainers:
> > > > > > > +  - Alex Elder <elder@riscstar.com>
> > > > > > > +  - Daniel Thompson <daniel@riscstar.com>
> > > > > > > +
> > > > > > > +description:
> > > > > > > +  The Toshiba TC9564 is an SoC accessed by a host system through the
> > > > > > > +  upstream PCIe port on the PCIe switch it implements.  The switch
> > > > > > > +  includes an embedded PCIe endpoint that provides access to various
> > > > > > > +  SoC peripherals (including a reset controller) via its BARs.
> > > > > > > +
> > > > > > > +  A total of 13 resets are implemented, though two of these are not
> > > > > > > +  controllable.  Access to the reset controller relies on PCIe being
> > > > > > > +  functional, so the PCIe reset is assumed to be deasserted.  Similarly,
> > > > > > > +  the PCIe controller relies on I2C, so the I2C reset is also assumed
> > > > > > > +  to be deasserted.
> > > > > > > +
> > > > > > > +  Reset ids are defined in <dt-bindings/reset/toshiba,tc9564.h>.
> > > > > > > +
> > > > > > > +  The reset controller uses registers within the same page as a
> > > > > > > +  clock controller, so both devices are defined as children of a
> > > > > > > +  "config" syscon.
> > > > > > > +
> > > > > > > +properties:
> > > > > > > +  compatible:
> > > > > > > +    const: toshiba,tc9564-reset
> > > > > > 
> > > > > > Why does this need to exist as a standalone device, rather than be
> > > > > > rolled up into the parent syscon node? It doesn't have any resources, so
> > > > > > it should be doable.
> > > > > 
> > > > > This is along the lines of what Krzysztof has mentioned, and it
> > > > > seems clear I must be missing something.
> > > > > 
> > > > > > This kind of thing for a syscon is always difficult to review when only
> > > > > > one element is present - where is the rest of the syscon's
> > > > > > functionality?
> > > > > 
> > > > > The reason a syscon was used was simply for the benefit of
> > > > > having a shared regmap that the reset, clock, and DWMAC drivers
> > > > > can use.  All three of those drivers use registers that reside
> > > > > within a single page of addressible MMIO space.
> > > > 
> > > > What does the dwmac use the region for? I assume it's for some misc
> > > > register and you don't mean that the dwmac lies within the region?
> > > 
> > > That's right.  There is one register (per DWMAC--there are two)
> > > that contains one bit (bit 21) that is used to signal that initialization of
> > > the PMA is done.  We assert reset, set up
> > > clocks, then deassert, and finally wait for this bit to be set.
> > > The DWMAC code uses a phandle property to refer to the syscon,
> > > like:
> > >      toshiba,config-syscon = <&tc9564_config_syscon0 0x1070>
> > > 
> > > So again, the purpose of the syscon was to get the automatic
> > > regmap for managing the region, which contains registers used
> > > for distinct purposes.
> > > 
> > > After making the syscon be the parent of both the reset and
> > > clock nodes, I *removed* the "reg" properties and just defined
> > > those offsets in the driver.  Maybe that was a mistake.
> > 
> > No, that's typically what happens in the setup I describe below, so
> > that's not abnormal.
> 
> Thank you for confirming this.
> 
> > > > There's nothing inherently wrong with the act of using of a syscon here,
> > > > what I am actually complaining about is how you are using it.
> > > > In cases where a syscon region contains clock and reset, the typical way
> > > > we want people to do this is via something like
> > > > 
> > > > syscon@f00 {
> > > > 	compatible = "toshiba,tc9564-bla-syscon", "syscon", "simple-mfd";
> > > > 	reg = <0x0 0x2000>;
> > > > 	#reset-cells = <1>;
> > > > 	#clock-cells = <1>;
> > > > };
> > > 
> > > In this case it seems I must create a syscon driver that provides
> > > combined reset and clock functionality.  Is that right?  (This
> > > series is the result of separating the reset code from the clock
> > > code in a combined driver.)
> > 
> > There's two ways to do it, depending on what sort of device you have.
> > If you have a clock controller that also does reset, the preferred thing
> > to do is something like
> > 
> > clock-controller@f00 {
> > 	compatible = "toshiba,tc9564-clk";
> > 	reg = <0x0 0x2000>;
> > 	#reset-cells = <1>;
> > 	#clock-cells = <1>;
> > };
> > 
> > and in the clock controller driver probe the reset controller driver as
> > a auxiliary bus device.
> 
> My last version combined these two but did not make the
> reset an auxiliary device.
> 
> > You have something else though, because there's assorted other functions
> > in here, including the dwmac and things you don't use yet. In that case,
> > I would make a node like
> > 
> > syscon@f00 {
> > 	compatible = "toshiba,tc9564-bla-syscon", "syscon";
> > 	reg = <0x0 0x2000>;
> > 	#reset-cells = <1>;
> > 	#clock-cells = <1>;
> > };
> > 
> > and in the syscon driver use MFD_CELL_NAME() to probe both clock and
> > reset drivers, and can use device_node_to_regmap(). Your dwmac stuff
> > (and any other future stuff that needs a syscon reference) can use the
> > automagic syscon regmap stuff that we've already talked about.
> 
> This sounds like the way to go.
> 
> > Note, there's no "simple-mfd" here, because you have no child nodes.
> > The point of "simple-mfd" is to probe those kinds of devices.
> 
> OK.  Let's see if I understand what you're saying.
> 
> This approach uses:
> - A single top-level syscon driver (under drivers/mfd?) which
>   probes based on a devicetree node compatible string ("toshiba,...")

I'm not sure if it needs to be under mfd (there's some in soc/foo too)
but I guess so.

> - That compatible string will require a (fairly simple) devicetree
>   binding
> - That driver initializes a regmap which will cover the space
>   defined in its devicetree node (<0x0 0x2000>)

I don't think the driver has to even do this at all, e.g.
sg2044-topsys.c doesn't. Whenever someone requests the regmap it will
get created automatically since you're a syscon.

> - That driver has an array that uses MFD_CELL_NAME() to define
>   cells for clock and reset
> - That driver will call some form of mfd_add_devices() using the
>   MFD cell array
> - The stmmac driver can refer to this device node with a phandle
>   property to specify the syscon that provides the regmap it needs
> 
> And then, for the clock and reset devices:
> - A clock platform driver (under drivers/clk) will match based
>   on the driver name (matching what's supplied by MFD_CELL_NAME())
> - A reset driver will be similar (under drivers/reset)
> - As you mentioned, these drivers get at the regmap they need using
>   device_node_to_regmap() (or dev_get_regmap() or something)

I think "regmap = device_node_to_regmap(pdev->dev.parent->of_node);" is
the correct way to do it.

> - And despite these being separate drivers (from the syscon driver)
>   they don't have devicetree nodes, and the syscon node itself
>   defines the clock-controller and reset-controller properties.
> 
> Does that sound right?  Am I missing anything?

Sounds about right.

> 
> 
> > > To be honest I don't have a lot of experience with proper use of
> > > syscons.  I have no problem writing or restructuring the code,
> > > it just seems I need help knowing what the final structure should
> > > look like.
> > > 
> > > > because the child node exists for no other reason than to access linux
> > > > driver probing in the most trivial way. If there's some features that
> > > > actually need a child node, that's fine, but those will end up being
> > > > things with significantly more properties or things with competing
> > > > property use (e.g. if there were devices with buses that need
> > > > address/size cells).
> > > 
> > > I'm not sure I understand this statement (and that might be part
> > > of the problem).
> > > 
> > > There are other devices (currently just two DWMACs, plus an
> > > interrupt controller that generates MSIs for each PCI function)
> > > that use the reset and clock signals.  I haven't yet
> > > posted the reworked DWMAC code but I just sent the first version
> > > of the interrupt driver yesterday.
> > >    https://lore.kernel.org/lkml/20261012190245.3988619-1-elder@riscstar.com/
> > 
> > Here I am talking about why child nodes aren't correct in your case.
> > Neither clock nor reset needs anything other than the relevant #cells
> > property, so the only benefit of a child node over having these in the
> > syscon node itself are to use the compatible to probe the driver.
> 
> Yes I now see what you mean.  By using a MFD model the child device
> is just a function provided by the top-level syscon device.  The
> driver code still benefits from the device model, but the subordinate
> devices match based on the device name and not a devicetree compatible
> string.  And no nodes exposed in devicetree are needed for the clock
> and reset.
> 
> > Child nodes should only be used when the functions provided by the
> > syscon have conflicting properties or are very complicated (I would
> > probably always use a child node for pinctrl for example). It'd also be
> > reasonable if the functions are reused across devices, but that's
> > rarely the case with clock or reset controllers (or really anything
> > that appears in these kinds of "random register regions" in SoCs).
> > 
> > Does that make more sense?
> 
> Yes, it definitely helps a lot.  I realize I might not have every
> detail right, but I feel much better about how to organize this
> (and presumably this also addresses what Krzysztof was asking for).

Yeah presumably!

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]