On Thu, Sep 5, 2019 at 4:38 AM Chuan Hua, Lei
indeed, chiptop should not have any child nodes (based on your explanation).I am not sure if syscon parent really matches hardware implementation.I think we should drop the syscon compatible for LGM thenIf we don't support legacy SoC with the same driver, we don't needOK, I think I understand now: chiptop != RCUFor LGM, RCU is clean. There would be no MISC register after software'sOK, just be aware that there are also rules for syscon compatibleI'm not surprised that we got some of the IP block layout for theAs I mentioned, some other misc registers are put into RCU even they
VRX200 RCU "wrong" - all "documentation" we have is the old Lantiq UGW
with proper documentation (as in a "public datasheet for the SoC") it
would be easy to spot these mistakes (at least I assume that the
quality of the Infineon / Lantiq datasheets is excellent).
back to reset-intel-syscon:
assigning only one job to the RCU hardware is a good idea (in my opinion).
that brings up a question: why do we need the "syscon" compatible for
the RCU node?
this is typically used when registers are accessed by another IP block
and the other driver has to access these registers as well. does this
mean that there's more hidden in the RCU registers?
don't belong to reset functions.
drivers, see for example: 
if Rob (dt-bindings maintainer) is happy with the documentation in
patch 1 then I'm fine with it as well.
for my own education I would appreciate if you could describe these
"other misc registers" with a few sentences (I assume that this can
also help Rob)
feedback. These misc registers will be moved to chiptop/misc
groups(implemented by syscon). For legacy SoC, we do have a lot MISC
registers for different SoCs.
so RCU really only has one purpose: handling resets
while chiptop manages all the random bits
does this means we don't need RCU to match "syscon"?
syscon, just regmap. Regmap is a must for us since we will use regmap
proxy to implement secure rest via secure processor.
even for the legacy SoCs the reset controller should not have a syscon
compatible: instead it should have a syscon parent (as the current
"lantiq,xrx200-reset" binding requires and as suggested by Rob for
another IP block: )
In all our Networking SoCs, chiptop is kind of misc register collection.
Some registers can't belong to any particular group, or they need to
work together with other modules(therefore, these misc registers would
be accessed by two or more modules). However, chiptop is not a hardware
I was referring to VRX200 where the RCU syscon has various children
(one child node for each hardware module that's part of RCU: reset
controller, 2x USB PHY, ...)
back to LGM:
you said that the LGM RCU registers only contain the reset controller.
thus I see no need for the syscon compatible
agreed, let's define the binding firstkeeping regmap is great in my opinion because it's a nice API and getsI will check with Dilip about how we should move forward. syscon parent
rid of some boilerplate
even better if it makes things easier for accessing the secure processor
just so I know how the documentation looks like:When we implement this driver, we checked other drivers(hisilicon/*,have you already discussed using only one reset cell?[...]As I mentioned, VRX200 and all legacy SoCs (MIPS based) can be solved
I see, thank you for the explanationReset status check is there. regmap_read_poll_timeout to check statusonly now I realized that the reset-intel-syscon driver does not seemYes. We have reset pulse(assert, then check the reset status).what about the _reset callback on the XRX350/XRX500/PRX300 SoCs - doOur internal version supports XRX350/XRX500/PRX300(MIPS based) and4. Code not optimized and intel internal review not assessed.insights from you (like the issue with the reset callback) are very
valuable - this shows that we should focus on having one driver.
Based on the above findings, I would suggest reset-lantiq.c to move tomy concern with having two separate drivers is that it will be hard to
migrate from reset-lantiq to the "optimized" reset-intel-syscon
I don't have access to the datasheets for the any Lantiq/Intel SoC
(VRX200 and even older).
so debugging issues after switching from one driver to another is
tedious because I cannot tell which part of the driver is causing a
problem (it's either "all code from driver A" vs "all code from driver
B", meaning it's hard to narrow it down).
with separate commits/patches that are improving the reset-lantiq
driver I can do git bisect to find the cause of a problem on the older
SoCs (VRX200 for example)
latest Lighting Mountain(X86 based). Migration to reset-intel-syscon.c
should be straight forward.
they only use level resets (_assert and _deassert) or are some reset
lines using reset pulses (_reset)?
when we wanted to switch from reset-lantiq.c to reset-intel-syscon.c
we still had to add support for the _reset callback as this is missing
in reset-intel-syscon.c currently
to use the status registers (instead it's looking at the reset
registers when checking the status).
what happened to the status registers - do they still exist in newer
SoCs (like LGM)? why are they not used?
big. Status register offset <4) from request register. For legacy, there
is one exception, we can add soc specific data to handle it.
this won't work on VRX200 for example because the status register is
not always at (reset register - 0x4)
with one soc data in the compatible array.
For example(not same as upstream, but idea is similar)
static u32 intel_stat_reg_off(struct intel_reset_data *data, u32 req_off)
if (data->soc_data->legacy && req_off == RCU_RST_REQ)
return req_off + 0x4;
LGM reset organization is more clean compared with legacy SoCs. We havewith reset-lantiq we have the following register information:on VRX200 for example there seem to be some cases where the bits inThis is most tricky and ugly part for VRX200/Danube. Do you have any
the reset and status registers are different (for example: the first
GPHY seems to use reset bit 31 but status bit 30)
this is currently not supported in reset-intel-syscon
idea to handle this nicely?
a) reset offset: first reg property
b) status offset: second reg property
c) reset bit: first #reset-cell
d) status bit: second #reset-cell
reset-intel-syscon derives half of this information from the two #reset-cells:
a) reset offset: first #reset-cell
b) status offset: reset offset - 0x4
c) reset bit: second #reset-cell
d) status bit: same as reset bit
I cannot make any suggestion (yet) how to handle VRX200 and LGM in one
driver because I don't know enough about LGM (yet).
on VRX200 my understanding is that we have 64 reset bits (2x 32bit
registers) and 64 status bits (also 2x 32bit registers). each reset
bit has a corresponding status bit but the numbering may be different
it's not clear to me how many resets LGM supports and how they are
organized. for example: I think it makes a difference if "there are 64
registers with each one reset bit" versus "there are two registers
with 32 bits each"
please share some details how it's organized internally, then I can
try to come up with a suggestion.
8 x 32bit reset and status registers(more modules need to be reset,
overall ideas are similar without big change). Their request and status
bit is at the same register bit position. Hope this will help you.
if there's only one big reset controller in RCU then why not let the
reset controller driver do it's job of translating a reset line? also
this represents the hardware best (dt-bindings should describe the
hardware, drivers then translate that into the various subsystems
offered by the kernel).
we have to translate it into:
- status register and bit
- reset register and bit
for LGM the implementation seems to be the easiest because the reset
line can be mapped easily to the registers and bit offsets (for
example like reset-meson.c does it, which also supports 256 reset
lines together with for example
include/dt-bindings/reset/amlogic,meson-g12a-reset.h. the latter is
nice to have but optional)
reset-berlin.c and etc). After evaluation, we think register offset and
register bit are easier for users to understand and use if they follow
the hardware spec.
does the hardware spec document 8 registers, each with (up to) the 32
reset lines in it?
reset-meson.c does it like that, but the difference there is that the
reset registers are continuous because there's no status register in
so your existing way of describing the reset line seems fine if Rob is
happy with it as well
OK, let me summarize what we have so far.we can then implement special translation logic (in other words: aBut we still don't have a good solution for VRX200 status bit issues.
separate of_xlate callback) for VRX200 which then has to do more
"magic" (like you have shown in your example code above: "if the reset
line belongs to the second set of 32 reset lines then use reset offset
X and status offset Y" - or even use a translation table as
the current binding is a mix of specifying reset register and bit in
.dts but calculating the status register.
I missed the calculation of the status register until you pointed it out earlier
Before we solve this issue, it is very difficult to use one driver for
all SoC have the following "shared" logic so far:
- all reset_control_ops callbacks are the same on VRX200 and LGM
(assuming we fix the issues you found in the reset-lantiq.c
- internally we should use regmap (LGM for accessing the secure
processor, earlier SoCs because the parent is a syscon)
- each reset line consists of a reset register offset and bit as well
as a status register offset and bit
however, we have differences in:
- how to map the registers (LGM maps the RCU registers directly while
earlier SoCs fetch the parent syscon)
- calculation of the status register
- calculation of the status bit
I see two ways to use one common driver for LGM and the earlier SoCs:
1) use a reset line mapping table as for example reset-imx7.c does.
this would include reset register, reset bit, status register and status bit.
LGM can use a macro to get rid of the duplication between status bit
and reset bit (and the status register offset if you prefer)
this case would use #reset-cells = <1> and we wouldn't need to
implement the of_xlate callback
2) on VRX200 (and probably the older SoCs as well) we can encode the
following information in one 32-bit value:
- reset register (max value: 0x48)
- status register (max value: 0x24)
- reset bit (max value: 32)
- status bit (max value: 32)
if this also works for LGM we can determine all required information
for a reset line in the of_xlate callback and translate it to one
LGM and earlier SoCs would each use it's own of_xlate implementation.
the reset_control_ops callback would then unpack the 32-bit value
("unsigned long id") into the reset register and bit as well as status
register and bit (as needed)
both ways can work, but it depends on what the dt-bindings maintainers
(like Rob) think of the binding itself.
(dt-bindings follow what the hardware implements, the driver only does
the translation between a vendor specific binding and a given
so we first need Rob's ACK on the binding, then we can figure out the
best driver implementation for that binding
is one issue that we have to solve first.