| Message ID | 20220711123519.217219-1-tmaimon77@gmail.com |
|---|---|
| Headers | show |
| Series | Introduce Nuvoton Arbel NPCM8XX BMC SoC | expand |
Quoting Tomer Maimon (2022-07-19 03:04:43) > On Mon, 18 Jul 2022 at 22:14, Stephen Boyd <sboyd@kernel.org> wrote: > > > > > > So the clk and reset driver should be the same driver, or one driver > > should register the other and use the auxiliary bus to express the > > relationship. That way we know that the drivers are tightly coupled and > > aren't going to stomp over each other. > I think it is very problematic to use the same driver for the reset > and the clocks also because The NPCM reset driver is an old driver > that was used also to the older NPCM BMC SoC so it will be problematic > to use the clock and reset driver in the same space. > indeed the reset and clocks are using the same memory region but they > are not using the same registers, is it not enough? > Please be aware that the NPCM reset driver is checking that it is > using the reset registers before calling I/O functions. To put it simply, platform device drivers should use platform device APIs. The platform device APIs hide the fact that the firmware is ACPI or DT or nothing at all. The usage of of_address_to_resource() is problematic. After converting that to platform APIs you'll get janitor style cleanups trying to convert to devm_platform_ioremap_resource(). We'll have to discuss this again when that happens, even if there's a comment in the code indicating we can't reserve the IO space because there's another driver. These problems have happened in the past, fun times! Furthermore, in DT, reg properties aren't supposed to overlap. When that happens it usually indicates the DT is being written to describe driver structure instead of the IP blocks that are delivered by the hardware engineer. In this case it sounds like a combined clk and reset IP block because they piled all the SoC glue stuff into a register range. Are there more features in this IO range?
Hi Stephen, Thanks for your detailed explanation! On Sat, 23 Jul 2022 at 06:02, Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Tomer Maimon (2022-07-19 03:04:43) > > On Mon, 18 Jul 2022 at 22:14, Stephen Boyd <sboyd@kernel.org> wrote: > > > > > > > > > So the clk and reset driver should be the same driver, or one driver > > > should register the other and use the auxiliary bus to express the > > > relationship. That way we know that the drivers are tightly coupled and > > > aren't going to stomp over each other. > > I think it is very problematic to use the same driver for the reset > > and the clocks also because The NPCM reset driver is an old driver > > that was used also to the older NPCM BMC SoC so it will be problematic > > to use the clock and reset driver in the same space. > > indeed the reset and clocks are using the same memory region but they > > are not using the same registers, is it not enough? > > Please be aware that the NPCM reset driver is checking that it is > > using the reset registers before calling I/O functions. > > To put it simply, platform device drivers should use platform device > APIs. The platform device APIs hide the fact that the firmware is ACPI > or DT or nothing at all. The usage of of_address_to_resource() is > problematic. > > After converting that to platform APIs you'll get janitor style cleanups > trying to convert to devm_platform_ioremap_resource(). We'll have to > discuss this again when that happens, even if there's a comment in the > code indicating we can't reserve the IO space because there's another > driver. These problems have happened in the past, fun times! > > Furthermore, in DT, reg properties aren't supposed to overlap. When that > happens it usually indicates the DT is being written to describe driver > structure instead of the IP blocks that are delivered by the hardware > engineer. In this case it sounds like a combined clk and reset IP block > because they piled all the SoC glue stuff into a register range. Are > there more features in this IO range? No, this range only combined the reset and clock together, but it combined in a way that we cannot split it to two or even three different registers... I do see a way to combine the clock and the reset driver, the NPCM reset driver is serving other NPCM BMC's. Should we use regmap to handle the clock registers instead of ioremap? Best regards, Tomer
Quoting Tomer Maimon (2022-07-24 02:06:54) > On Sat, 23 Jul 2022 at 06:02, Stephen Boyd <sboyd@kernel.org> wrote: > > Furthermore, in DT, reg properties aren't supposed to overlap. When that > > happens it usually indicates the DT is being written to describe driver > > structure instead of the IP blocks that are delivered by the hardware > > engineer. In this case it sounds like a combined clk and reset IP block > > because they piled all the SoC glue stuff into a register range. Are > > there more features in this IO range? > > No, this range only combined the reset and clock together, but it > combined in a way that we cannot split it to two or even three > different registers... Because it is jumbled in some range? > > I do see a way to combine the clock and the reset driver, the NPCM > reset driver is serving other NPCM BMC's. > Should we use regmap to handle the clock registers instead of ioremap? Sure? Using regmap or not looks like a parallel discussion. How does it help use platform APIs?
Quoting Tomer Maimon (2022-08-08 06:08:08) > On Mon, 8 Aug 2022 at 15:37, Tomer Maimon <tmaimon77@gmail.com> wrote: > > > Using platform APIs means using platform_*() functions, not of_*() > > > functions, which are open-firmware/DT related. Regmap can be used to > > > operate on registers mapped as __iomem, which is different from platform > > > APIs. > > I will use platform_get_resource() and devm_ioremap_resource() > > functions in the next version. > I will use platform_get_resource() and ioremap() function next > veriosn, is it fine? As stated earlier it will work for now but eventually you'll get patches from janitors trying to convert to a devm based API that reserves the register region. Can you ioremap the register once and register an auxiliary device and driver for the reset (or clk) part so that the driver can be moved out to the drivers/reset/ path?