Re: [PATCH 1/2] dt-bindings: reset: Updated binding for Versal-NET reset driver

From: Michal Simek
Date: Tue Jul 18 2023 - 09:11:36 EST



On 7/18/23 09:39, Krzysztof Kozlowski wrote:
On 18/07/2023 09:10, Michal Simek wrote:


On 7/17/23 22:47, Krzysztof Kozlowski wrote:
On 17/07/2023 20:40, Conor Dooley wrote:
On Mon, Jul 17, 2023 at 04:53:47PM +0530, Piyush Mehta wrote:
Added documentation and Versal-NET reset indices to describe about
Versal-Net reset driver bindings.

In Versal-NET all reset indices includes Class, SubClass, Type, Index
information whereas class refers to clock, reset, power etc.,
Underlying firmware in Versal have such classification and expects
the ID to be this way.
[13:0] - Index bits
[19:14] - Type bits
[25:20] - SubClass bits
[31:26] - Class bits.

Riight.. I'm not sure that describing these as "indices" is really all
that valid, given only 13:0 are actually the index.
I'd be inclined to say that the type/class/subclass stuff should not be
part of the dt-bindings, and instead looked up inside the driver
depending on the index.

Hopefully Rob or Krzysztof can comment further.

This confuses me as well. I don't understand why do you need it in the
bindings. Nothing uses these values, so storing them as bindings seems
pointless.

Power Management team wants to use these NodeID with format describe above to
identify that elements. And I already told them that ID (0-max) translation to
internal NodeID format should be done in firmware but they don't pretty much
agree with it.

Too bad for them. They can join the discussion, though :)


From DT binding perspective I think it doesn't really matter if numbers are
from 0 to max or they are from random high number to another random number
without step equal to 1.
And it is driver implementation detail if driver itself is checking that
requested ID is bigger than number of pins passed.

In connection to reset driver in Linux.

Core has:
static int of_reset_simple_xlate(struct reset_controller_dev *rcdev,
const struct of_phandle_args *reset_spec)
{
if (reset_spec->args[0] >= rcdev->nr_resets)
return -EINVAL;

return reset_spec->args[0];
}

/**
* reset_controller_register - register a reset controller device
* @rcdev: a pointer to the initialized reset controller device
*/
int reset_controller_register(struct reset_controller_dev *rcdev)
{
if (!rcdev->of_xlate) {
rcdev->of_reset_n_cells = 1;
rcdev->of_xlate = of_reset_simple_xlate;
}
...

If you use both, it means you don't need bindings.


And zynqmp reset driver already defines of_xlate function that's why checking
against nr_resets is not done as is visible from code below.


static int zynqmp_reset_of_xlate(struct reset_controller_dev *rcdev,
const struct of_phandle_args *reset_spec)
{
return reset_spec->args[0];
}

Exactly, there is no xlate. These IDs are not suitable nor needed for
bindings.



And actually Xilinx Versal platform is using this for a while and you can find
IDs description here.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/reset/xlnx-versal-resets.h?h=v6.5-rc2

We cannot fix existing bindings, but we can fix future ones.


Xilinx ZynqMP is using IDs from 0 to 119
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/reset/xlnx-zynqmp-resets.h?h=v6.5-rc2


but IDs itself are shifted by 1000:
include/linux/firmware/xlnx-zynqmp.h:217: ZYNQMP_PM_RESET_START = 1000,
#define ZYNQMP_RESET_ID ZYNQMP_PM_RESET_START

static const struct zynqmp_reset_soc_data zynqmp_reset_data = {
.reset_id = ZYNQMP_RESET_ID,
.num_resets = ZYNQMP_NR_RESETS,
};

static int zynqmp_reset_assert(struct reset_controller_dev *rcdev,
unsigned long id)
{
struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);

return zynqmp_pm_reset_assert(priv->data->reset_id + id,
PM_RESET_ACTION_ASSERT);
}


That numbers in DT are virtual no matter if you use ID from 0 to max or random
values it is up to code to handle them. Checking nr_pins against ID is done in
core but it is up to drivers.

No, you confuse "virtual" and "ID". IDs are not virtual. IDs are real
and have representation in Linux driver. You do not need to define
anything virtual in the bindings.

Not sure how you define ID itself. But HW doesn't know ID. HW knows only register which you can use to perform the reset. It is not really 128bit register where every bit targets to different IP.

And this is SW-firmware interface like SCMI reset driver.

Firmware is saying that ID 0 is QSPI, ID 1 is MMC.
Their Linux driver is asking for nr_reset via firmware call which can be different for different SOC and that's fine and I have no problem with it.
But only SCMI server is dictating that ID 0 is QSPI and ID 1 is MMC. Different SCMI server implementation can map it differently.


In our case that IDs are coming from firmware and driver itself is just matching
them.

So they are the same as if coming from hardware - no need for IDs.

It is hard to say what hardware here exactly is. From my perspective and I am not advocating not using IDs from 0 to max, it is just a number.

If my firmware knows that QSPI reset is 0xc10402dU then I will just pass it to reach my goal which is reset QSPI IP.

If you think that we should use IDs from 0 to max NR I am happy to pass this message to PM team and we should extend any SW to do translation between.

Thanks,
Michal