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 > > > > > > > Signed-off-by: Daniel Thompson > > > > > > > Signed-off-by: Alex Elder > > > > > > > --- > > 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 > > > > > > > + - Daniel Thompson > > > > > > > + > > > > > > > +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 . > > > > > > > + > > > > > > > + 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.