| Message ID | 1445964626-6484-5-git-send-email-jenskuske@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jens, Am Dienstag, den 27.10.2015, 17:50 +0100 schrieb Jens Kuske: [...] > --- a/drivers/reset/reset-sunxi.c > +++ b/drivers/reset/reset-sunxi.c > @@ -75,7 +75,9 @@ static struct reset_control_ops sunxi_reset_ops = { > .deassert = sunxi_reset_deassert, > }; > > -static int sunxi_reset_init(struct device_node *np) > +static int sunxi_reset_init(struct device_node *np, > + int (*of_xlate)(struct reset_controller_dev *rcdev, > + const struct of_phandle_args *reset_spec)) I'd add a tab to the indentation and drop the of_xlate parameter names. If you agree to this change, I'll fix it up when I apply it. best regards Philipp
On Tuesday 27 October 2015 17:50:24 Jens Kuske wrote: > > +static int sun8i_h3_bus_reset_xlate(struct reset_controller_dev *rcdev, > + const struct of_phandle_args *reset_spec) > +{ > + unsigned int index = reset_spec->args[0]; > + > + if (index < 96) > + return index; > + else if (index < 128) > + return index + 32; > + else if (index < 160) > + return index + 64; > + else > + return -EINVAL; > +} > + > This looks like you are doing something wrong and should either put the actual number into DT, or use a two-cell representation, with the first cell indicating the block (0, 1 or 2), and the second cell the index. Arnd
Hi, On 30/10/15 09:27, Arnd Bergmann wrote: > On Tuesday 27 October 2015 17:50:24 Jens Kuske wrote: >> >> +static int sun8i_h3_bus_reset_xlate(struct reset_controller_dev *rcdev, >> + const struct of_phandle_args *reset_spec) >> +{ >> + unsigned int index = reset_spec->args[0]; >> + >> + if (index < 96) >> + return index; >> + else if (index < 128) >> + return index + 32; >> + else if (index < 160) >> + return index + 64; >> + else >> + return -EINVAL; >> +} >> + >> > > This looks like you are doing something wrong and should either > put the actual number into DT, or use a two-cell representation, > with the first cell indicating the block (0, 1 or 2), and the > second cell the index. > I tried to fix up the somewhat strange register layout here. >From the datasheet: BUS_SOFT_RST_REG0 0x02C0 Bus Software Reset Register 0 BUS_SOFT_RST_REG1 0x02C4 Bus Software Reset Register 1 BUS_SOFT_RST_REG2 0x02C8 Bus Software Reset Register 2 BUS_SOFT_RST_REG3 0x02D0 Bus Software Reset Register 3 BUS_SOFT_RST_REG4 0x02D8 Bus Software Reset Register 4 0x2cc and 0x2d4 are unused for some reason, but the regs are named 0-4, so it lead to some confusion with the actual numbers in DT. If we shouldn't do this I would be ok with putting the actual number into DT too. Jens
Hi Arnd, On Fri, Oct 30, 2015 at 09:27:03AM +0100, Arnd Bergmann wrote: > On Tuesday 27 October 2015 17:50:24 Jens Kuske wrote: > > > > +static int sun8i_h3_bus_reset_xlate(struct reset_controller_dev *rcdev, > > + const struct of_phandle_args *reset_spec) > > +{ > > + unsigned int index = reset_spec->args[0]; > > + > > + if (index < 96) > > + return index; > > + else if (index < 128) > > + return index + 32; > > + else if (index < 160) > > + return index + 64; > > + else > > + return -EINVAL; > > +} > > + > > > > This looks like you are doing something wrong and should either > put the actual number into DT, This is the actual number, except that there's some useless registers in between. Allwinner documents it like that: 0x0 Reset 0 0x4 Reset 1 0xc Reset 2 So we have to adjust the offset to account with the blank register in between (0x8). > or use a two-cell representation, with the first cell indicating the > block (0, 1 or 2), and the second cell the index. And the missing register is not a block either. That would also imply either changing the bindings of that driver (and all the current DTS that are using it), or introducing a whole new driver just to deal with some extraordinary offset calculation. Maxime
On Wed, 4 Nov 2015 08:30:14 -0800 Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Hi Arnd, > > On Fri, Oct 30, 2015 at 09:27:03AM +0100, Arnd Bergmann wrote: > > On Tuesday 27 October 2015 17:50:24 Jens Kuske wrote: > > > > > > +static int sun8i_h3_bus_reset_xlate(struct reset_controller_dev *rcdev, > > > + const struct of_phandle_args *reset_spec) > > > +{ > > > + unsigned int index = reset_spec->args[0]; > > > + > > > + if (index < 96) > > > + return index; > > > + else if (index < 128) > > > + return index + 32; > > > + else if (index < 160) > > > + return index + 64; > > > + else > > > + return -EINVAL; > > > +} > > > + > > > > > > > This looks like you are doing something wrong and should either > > put the actual number into DT, > > This is the actual number, except that there's some useless registers > in between. Allwinner documents it like that: > > 0x0 Reset 0 > 0x4 Reset 1 > 0xc Reset 2 > > So we have to adjust the offset to account with the blank register in > between (0x8). > > > or use a two-cell representation, with the first cell indicating the > > block (0, 1 or 2), and the second cell the index. > > And the missing register is not a block either. > > That would also imply either changing the bindings of that driver (and > all the current DTS that are using it), or introducing a whole new > driver just to deal with some extraordinary offset calculation. In the H3, the holes are not used, but what would occur if these holes would be used for some other purpose in future SoCs? Double mapping?
On Thu, Nov 5, 2015 at 2:47 PM, Jean-Francois Moine <moinejf@free.fr> wrote: > On Wed, 4 Nov 2015 08:30:14 -0800 > Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > >> Hi Arnd, >> >> On Fri, Oct 30, 2015 at 09:27:03AM +0100, Arnd Bergmann wrote: >> > On Tuesday 27 October 2015 17:50:24 Jens Kuske wrote: >> > > >> > > +static int sun8i_h3_bus_reset_xlate(struct reset_controller_dev *rcdev, >> > > + const struct of_phandle_args *reset_spec) >> > > +{ >> > > + unsigned int index = reset_spec->args[0]; >> > > + >> > > + if (index < 96) >> > > + return index; >> > > + else if (index < 128) >> > > + return index + 32; >> > > + else if (index < 160) >> > > + return index + 64; >> > > + else >> > > + return -EINVAL; >> > > +} >> > > + >> > > >> > >> > This looks like you are doing something wrong and should either >> > put the actual number into DT, >> >> This is the actual number, except that there's some useless registers >> in between. Allwinner documents it like that: >> >> 0x0 Reset 0 >> 0x4 Reset 1 >> 0xc Reset 2 >> >> So we have to adjust the offset to account with the blank register in >> between (0x8). >> >> > or use a two-cell representation, with the first cell indicating the >> > block (0, 1 or 2), and the second cell the index. >> >> And the missing register is not a block either. >> >> That would also imply either changing the bindings of that driver (and >> all the current DTS that are using it), or introducing a whole new >> driver just to deal with some extraordinary offset calculation. > > In the H3, the holes are not used, but what would occur if these holes > would be used for some other purpose in future SoCs? Double mapping? We'd have a different compatible string for it. My suggestion for the resets is to just split them into 3 nodes: AHB (since AHB1 and AHB2 devices are mixed together in the bunch), APB1, and APB2 reset controls. This follows what we have for existing SoCs, and gets rid of the unused hole. We can use the existing "allwinner,sun6i-a31-clock-reset" and "allwinner,sun6i-a31-ahb1-reset" compatibles. Regards ChenYu
Hi, On Mon, Nov 23, 2015 at 03:41:52PM +0800, Chen-Yu Tsai wrote: > On Thu, Nov 5, 2015 at 2:47 PM, Jean-Francois Moine <moinejf@free.fr> wrote: > > On Wed, 4 Nov 2015 08:30:14 -0800 > > Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > > > >> Hi Arnd, > >> > >> On Fri, Oct 30, 2015 at 09:27:03AM +0100, Arnd Bergmann wrote: > >> > On Tuesday 27 October 2015 17:50:24 Jens Kuske wrote: > >> > > > >> > > +static int sun8i_h3_bus_reset_xlate(struct reset_controller_dev *rcdev, > >> > > + const struct of_phandle_args *reset_spec) > >> > > +{ > >> > > + unsigned int index = reset_spec->args[0]; > >> > > + > >> > > + if (index < 96) > >> > > + return index; > >> > > + else if (index < 128) > >> > > + return index + 32; > >> > > + else if (index < 160) > >> > > + return index + 64; > >> > > + else > >> > > + return -EINVAL; > >> > > +} > >> > > + > >> > > > >> > > >> > This looks like you are doing something wrong and should either > >> > put the actual number into DT, > >> > >> This is the actual number, except that there's some useless registers > >> in between. Allwinner documents it like that: > >> > >> 0x0 Reset 0 > >> 0x4 Reset 1 > >> 0xc Reset 2 > >> > >> So we have to adjust the offset to account with the blank register in > >> between (0x8). > >> > >> > or use a two-cell representation, with the first cell indicating the > >> > block (0, 1 or 2), and the second cell the index. > >> > >> And the missing register is not a block either. > >> > >> That would also imply either changing the bindings of that driver (and > >> all the current DTS that are using it), or introducing a whole new > >> driver just to deal with some extraordinary offset calculation. > > > > In the H3, the holes are not used, but what would occur if these holes > > would be used for some other purpose in future SoCs? Double mapping? > > We'd have a different compatible string for it. > > My suggestion for the resets is to just split them into 3 nodes: AHB > (since AHB1 and AHB2 devices are mixed together in the bunch), APB1, > and APB2 reset controls. > > This follows what we have for existing SoCs, and gets rid of the unused > hole. We can use the existing "allwinner,sun6i-a31-clock-reset" and > "allwinner,sun6i-a31-ahb1-reset" compatibles. That seems a bit weird to have a single clock and split resets, but as long as they are not mixed, and you can compute easily the id from the datasheet, ok. Maxime
diff --git a/Documentation/devicetree/bindings/reset/allwinner,sunxi-clock-reset.txt b/Documentation/devicetree/bindings/reset/allwinner,sunxi-clock-reset.txt index c8f7757..e11f023 100644 --- a/Documentation/devicetree/bindings/reset/allwinner,sunxi-clock-reset.txt +++ b/Documentation/devicetree/bindings/reset/allwinner,sunxi-clock-reset.txt @@ -8,6 +8,7 @@ Required properties: - compatible: Should be one of the following: "allwinner,sun6i-a31-ahb1-reset" "allwinner,sun6i-a31-clock-reset" + "allwinner,sun8i-h3-bus-reset" - reg: should be register base and length as documented in the datasheet - #reset-cells: 1, see below diff --git a/drivers/reset/reset-sunxi.c b/drivers/reset/reset-sunxi.c index 3d95c87..c91e146 100644 --- a/drivers/reset/reset-sunxi.c +++ b/drivers/reset/reset-sunxi.c @@ -75,7 +75,9 @@ static struct reset_control_ops sunxi_reset_ops = { .deassert = sunxi_reset_deassert, }; -static int sunxi_reset_init(struct device_node *np) +static int sunxi_reset_init(struct device_node *np, + int (*of_xlate)(struct reset_controller_dev *rcdev, + const struct of_phandle_args *reset_spec)) { struct sunxi_reset_data *data; struct resource res; @@ -108,6 +110,7 @@ static int sunxi_reset_init(struct device_node *np) data->rcdev.nr_resets = size * 32; data->rcdev.ops = &sunxi_reset_ops; data->rcdev.of_node = np; + data->rcdev.of_xlate = of_xlate; reset_controller_register(&data->rcdev); return 0; @@ -117,6 +120,21 @@ err_alloc: return ret; }; +static int sun8i_h3_bus_reset_xlate(struct reset_controller_dev *rcdev, + const struct of_phandle_args *reset_spec) +{ + unsigned int index = reset_spec->args[0]; + + if (index < 96) + return index; + else if (index < 128) + return index + 32; + else if (index < 160) + return index + 64; + else + return -EINVAL; +} + /* * These are the reset controller we need to initialize early on in * our system, before we can even think of using a regular device @@ -124,15 +142,21 @@ err_alloc: */ static const struct of_device_id sunxi_early_reset_dt_ids[] __initdata = { { .compatible = "allwinner,sun6i-a31-ahb1-reset", }, + { .compatible = "allwinner,sun8i-h3-bus-reset", .data = sun8i_h3_bus_reset_xlate, }, { /* sentinel */ }, }; void __init sun6i_reset_init(void) { struct device_node *np; + const struct of_device_id *match; + int (*of_xlate)(struct reset_controller_dev *rcdev, + const struct of_phandle_args *reset_spec); - for_each_matching_node(np, sunxi_early_reset_dt_ids) - sunxi_reset_init(np); + for_each_matching_node_and_match(np, sunxi_early_reset_dt_ids, &match) { + of_xlate = match->data; + sunxi_reset_init(np, of_xlate); + } } /*
The H3 bus resets have some holes between the registers, so we add an of_xlate() function to skip them according to the datasheet. Signed-off-by: Jens Kuske <jenskuske@gmail.com> --- .../bindings/reset/allwinner,sunxi-clock-reset.txt | 1 + drivers/reset/reset-sunxi.c | 30 +++++++++++++++++++--- 2 files changed, 28 insertions(+), 3 deletions(-)