| Message ID | cbfd45e3-310b-db40-d52b-daf79ce8743d@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Conflict between video-lut and pmu on meson-g12 | expand |
Hello Marc, On Tue, Feb 28, 2023 at 5:48 PM Marc Gonzalez <marc.w.gonzalez@free.fr> wrote: > > Hello everyone, > > Running 6.2.0-rc8 on a S905X2 board, I note the following error > in the kernel log: > > [ 1.059175] meson-g12-ddr-pmu ff638000.pmu: can't request region for resource [mem 0xff638000-0xff6380ff] > [ 1.068647] meson-g12-ddr-pmu: probe of ff638000.pmu failed with error -16 [...] ouch, it seems we missed that during the driver/bindings review > A simple solution might be to specify the "actual" base of > the register set, and count from 0 in the driver? I think your fix is correct (formally it would need to be split into a driver and a .dtsi patch - but let's do things step by step). It would be great to get some confirmation from Jiucheng Xu on this as I think we should be quick with fixing before this bug makes it elsewhere (.dtsis can be shared with u-boot and other systems for example). Apart from the driver and .dtsi changes we should also update Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml But again: let's do this one step at a time. Thanks for the analysis and your work on this! Best regards, Martin
Hello Jiucheng Xu, On Tue, Feb 28, 2023 at 10:04 PM Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: [...] > > A simple solution might be to specify the "actual" base of > > the register set, and count from 0 in the driver? > I think your fix is correct (formally it would need to be split into a > driver and a .dtsi patch - but let's do things step by step). While thinking more about this - I think the whole .dtsi code should be improved. Both of the PMU IO regions are part of the &dmc region. So I think &pmu should be moved inside &dmc (with the offsets adjusted accordingly of course). Also I think the dt-bindings are incomplete: according to the driver code we're using XTAL as input clock. But this is not described anywhere in the dt-bindings. dt-bindings should always describe the hardware. The driver can decide not to use it but the bindings must always be complete. And with this comes the question: is the DMC PLL specific to the PMU or is it shared with something else (e.g. the actual memory controller)? On the 32-bit SoCs (Meson8b/S805 for example) there's a whole DDR clock controller (used by the DDR memory controller), so I'm wondering if these newer SoCs are still following that approach. Best regards, Martin
+ jiuchengxu@163.com Amlogic mail sending server has problem. Add my personal email. Comments inline. On 2023/3/1 5:49, Martin Blumenstingl wrote: > [ EXTERNAL EMAIL ] > > Hello Jiucheng Xu, > > On Tue, Feb 28, 2023 at 10:04 PM Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: > [...] >>> A simple solution might be to specify the "actual" base of >>> the register set, and count from 0 in the driver? >> I think your fix is correct (formally it would need to be split into a >> driver and a .dtsi patch - but let's do things step by step). > While thinking more about this - I think the whole .dtsi code should > be improved. Both of the PMU IO regions are part of the &dmc region. > So I think &pmu should be moved inside &dmc (with the offsets adjusted > accordingly of course). Okay I agree with you.I will put &pmu node into &dmc node. > > Also I think the dt-bindings are incomplete: according to the driver > code we're using XTAL as input clock. > But this is not described anywhere in the dt-bindings. > dt-bindings should always describe the hardware. The driver can decide > not to use it but the bindings must always be complete. Sorry, I couldn't get your point. I didn't know the relationship between XTAL and DMC. Would you please share more the information? > And with this comes the question: is the DMC PLL specific to the PMU > or is it shared with something else (e.g. the actual memory > controller)? On the 32-bit SoCs (Meson8b/S805 for example) there's a > whole DDR clock controller (used by the DDR memory controller), so I'm > wondering if these newer SoCs are still following that approach. Yes, the PLL is the base input clock of DMC and PMU inside. I have never seen the meson8b board. I think G12 is a newer generation which has some improvement than old SoCs. My English is not well. Maybe I doesn't answer your point since I got miss-understanding. Thanks & Best regards Jiucheng > > Best regards, > Martin >
On 28/02/2023 22:49, Martin Blumenstingl wrote: > While thinking more about this - I think the whole .dtsi code should > be improved. Both of the PMU IO regions are part of the &dmc region. > So I think &pmu should be moved inside &dmc (with the offsets adjusted > accordingly of course). > > Also I think the dt-bindings are incomplete: according to the driver > code we're using XTAL as input clock. > But this is not described anywhere in the dt-bindings. > dt-bindings should always describe the hardware. The driver can decide > not to use it but the bindings must always be complete. > And with this comes the question: is the DMC PLL specific to the PMU > or is it shared with something else (e.g. the actual memory > controller)? On the 32-bit SoCs (Meson8b/S805 for example) there's a > whole DDR clock controller (used by the DDR memory controller), so I'm > wondering if these newer SoCs are still following that approach. FWIW, the vendor device-tree specifies the following nodes: https://android.googlesource.com/kernel/arm64/+/f5269100977385d1fd4a5ef68e49631892cf4fe4/arch/arm64/boot/dts/amlogic/g12a_s905x2_u215.dts canvas { compatible = "amlogic, meson, canvas"; dev_name = "amlogic-canvas"; status = "okay"; reg = <0x0 0xff638000 0x0 0x2000>; phandle = <0x111>; }; codec_io { compatible = "amlogic, codec_io"; status = "okay"; #address-cells = <0x2>; #size-cells = <0x2>; ranges; phandle = <0x112>; [...] io_dmc_base { reg = <0x0 0xff638000 0x0 0x2000>; }; }; ddr_bandwidth { compatible = "amlogic, ddr-bandwidth"; status = "okay"; reg = <0x0 0xff638000 0x0 0x100 0x0 0xff638c00 0x0 0x100>; sec_base = <0xff639000>; interrupts = <0x0 0x34 0x1>; interrupt-names = "ddr_bandwidth"; }; I don't understand how it's possible to have 3 overlapping ranges? Unless the respective drivers know to map only specific ranges? Regarding your DMC (DDR memory controller) clock question, clock tree seems to be: 24 MHz XTAL feeds DDR_PLL block, which outputs DDR_CLK pulse for the DMC. Something I do not understand is that the datasheet states: DMC unsecure register. Base address 0xFF638000. Offset 0 = AM_DDR_PLL_CNTL0 Offset 4 = AM_DDR_PLL_CNTL1 ... And then also states: The following registers' base address is 0xff638000. Offset 0 = DMC_REQ_CTRL Offset 4 = DMC_SOFT_RST ... And these two register sets have nothing in common (except the SAME base address...) https://android.googlesource.com/kernel/arm64/+/f5269100977385d1fd4a5ef68e49631892cf4fe4/include/linux/amlogic/media/registers/regs/dmc_regs.h Is there perhaps a typo in one of the base addresses? Regards.
On 01/03/2023 14:28, Marc Gonzalez wrote: > On 28/02/2023 22:49, Martin Blumenstingl wrote: > >> While thinking more about this - I think the whole .dtsi code should >> be improved. Both of the PMU IO regions are part of the &dmc region. >> So I think &pmu should be moved inside &dmc (with the offsets adjusted >> accordingly of course). >> >> Also I think the dt-bindings are incomplete: according to the driver >> code we're using XTAL as input clock. >> But this is not described anywhere in the dt-bindings. >> dt-bindings should always describe the hardware. The driver can decide >> not to use it but the bindings must always be complete. >> And with this comes the question: is the DMC PLL specific to the PMU >> or is it shared with something else (e.g. the actual memory >> controller)? On the 32-bit SoCs (Meson8b/S805 for example) there's a >> whole DDR clock controller (used by the DDR memory controller), so I'm >> wondering if these newer SoCs are still following that approach. > > FWIW, the vendor device-tree specifies the following nodes: > https://android.googlesource.com/kernel/arm64/+/f5269100977385d1fd4a5ef68e49631892cf4fe4/arch/arm64/boot/dts/amlogic/g12a_s905x2_u215.dts > > canvas { > compatible = "amlogic, meson, canvas"; > dev_name = "amlogic-canvas"; > status = "okay"; > reg = <0x0 0xff638000 0x0 0x2000>; > phandle = <0x111>; > }; > > codec_io { > compatible = "amlogic, codec_io"; > status = "okay"; > #address-cells = <0x2>; > #size-cells = <0x2>; > ranges; > phandle = <0x112>; > [...] > io_dmc_base { > reg = <0x0 0xff638000 0x0 0x2000>; > }; > }; > > ddr_bandwidth { > compatible = "amlogic, ddr-bandwidth"; > status = "okay"; > reg = <0x0 0xff638000 0x0 0x100 0x0 0xff638c00 0x0 0x100>; > sec_base = <0xff639000>; > interrupts = <0x0 0x34 0x1>; > interrupt-names = "ddr_bandwidth"; > }; > > > I don't understand how it's possible to have 3 overlapping ranges? > Unless the respective drivers know to map only specific ranges? > > > Regarding your DMC (DDR memory controller) clock question, > clock tree seems to be: > 24 MHz XTAL feeds DDR_PLL block, > which outputs DDR_CLK pulse for the DMC. > > > Something I do not understand is that the datasheet states: > DMC unsecure register. Base address 0xFF638000. > Offset 0 = AM_DDR_PLL_CNTL0 > Offset 4 = AM_DDR_PLL_CNTL1 > ... > > And then also states: > The following registers' base address is 0xff638000. > Offset 0 = DMC_REQ_CTRL > Offset 4 = DMC_SOFT_RST > ... > > And these two register sets have nothing in common > (except the SAME base address...) > > https://android.googlesource.com/kernel/arm64/+/f5269100977385d1fd4a5ef68e49631892cf4fe4/include/linux/amlogic/media/registers/regs/dmc_regs.h > > Is there perhaps a typo in one of the base addresses? Any ideas/suggestions? How do we proceed? Regards
Hi Marc, On Thu, Mar 9, 2023 at 10:48 AM Marc Gonzalez <marc.w.gonzalez@free.fr> wrote: [...] > Any ideas/suggestions? > How do we proceed? I suggest you go ahead and send your original diff as formal patches. It would be best to have two/three patches (that can go through different maintainers repositories if needed): - move &pmu into &dmc, update the register size (as you suggested) and also update the offset (since they will have to be calculated based on &dmc) - fix up the #defines in the driver as you suggested - nice to have: update the example in Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml Before we work on the XTAL clock input I think it's best if we know about the actual hardware setup. amlogic,g12-ddr-pmu.yaml mentions that the second register set is for the "DMC PLL register space". I suspect that the DMC PLL has two purposes: - it's used as timer/counter for the PMU - it is also used for clocking the DDR memory If this assumption is correct then I think that the DMC PLL should get a separate node (with XTAL as clock input). Then the DMC PLL output should be used as clock input for the &pmu node. The reason for this is that the device tree should describe the hardware, not drivers. Also device tree is not Linux specific, it can be used by u-boot, etc. So if someone would implement the DDR setup in u-boot (or another bootloader - which may even share the device tree with Linux) then it has to be described correctly. I'm hoping that Jiucheng can provide his input on this topic. By the way: we already have a DDR (PLL) driver in drivers/clk/meson/meson8-ddr.c. At this point in time it only has Meson8/Meson8b/Meson8m2 support. I suspect it will be easy to extend the existing driver or just write a new one. Best regards, Martin
diff --git a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi index 534178eaaf373..cf37eecfba5b2 100644 --- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi +++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi @@ -1712,7 +1712,7 @@ internal_ephy: ethernet-phy@8 { }; pmu: pmu@ff638000 { - reg = <0x0 0xff638000 0x0 0x100>, + reg = <0x0 0xff638080 0x0 0x40>, <0x0 0xff638c00 0x0 0x100>; interrupts = <GIC_SPI 52 IRQ_TYPE_EDGE_RISING>; }; diff --git a/drivers/perf/amlogic/meson_g12_ddr_pmu.c b/drivers/perf/amlogic/meson_g12_ddr_pmu.c index a78fdb15e26c2..8b643888d5036 100644 --- a/drivers/perf/amlogic/meson_g12_ddr_pmu.c +++ b/drivers/perf/amlogic/meson_g12_ddr_pmu.c @@ -21,23 +21,23 @@ #define DMC_QOS_IRQ BIT(30) /* DMC bandwidth monitor register address offset */ -#define DMC_MON_G12_CTRL0 (0x20 << 2) -#define DMC_MON_G12_CTRL1 (0x21 << 2) -#define DMC_MON_G12_CTRL2 (0x22 << 2) -#define DMC_MON_G12_CTRL3 (0x23 << 2) -#define DMC_MON_G12_CTRL4 (0x24 << 2) -#define DMC_MON_G12_CTRL5 (0x25 << 2) -#define DMC_MON_G12_CTRL6 (0x26 << 2) -#define DMC_MON_G12_CTRL7 (0x27 << 2) -#define DMC_MON_G12_CTRL8 (0x28 << 2) - -#define DMC_MON_G12_ALL_REQ_CNT (0x29 << 2) -#define DMC_MON_G12_ALL_GRANT_CNT (0x2a << 2) -#define DMC_MON_G12_ONE_GRANT_CNT (0x2b << 2) -#define DMC_MON_G12_SEC_GRANT_CNT (0x2c << 2) -#define DMC_MON_G12_THD_GRANT_CNT (0x2d << 2) -#define DMC_MON_G12_FOR_GRANT_CNT (0x2e << 2) -#define DMC_MON_G12_TIMER (0x2f << 2) +#define DMC_MON_G12_CTRL0 (0x0 << 2) +#define DMC_MON_G12_CTRL1 (0x1 << 2) +#define DMC_MON_G12_CTRL2 (0x2 << 2) +#define DMC_MON_G12_CTRL3 (0x3 << 2) +#define DMC_MON_G12_CTRL4 (0x4 << 2) +#define DMC_MON_G12_CTRL5 (0x5 << 2) +#define DMC_MON_G12_CTRL6 (0x6 << 2) +#define DMC_MON_G12_CTRL7 (0x7 << 2) +#define DMC_MON_G12_CTRL8 (0x8 << 2) + +#define DMC_MON_G12_ALL_REQ_CNT (0x9 << 2) +#define DMC_MON_G12_ALL_GRANT_CNT (0xa << 2) +#define DMC_MON_G12_ONE_GRANT_CNT (0xb << 2) +#define DMC_MON_G12_SEC_GRANT_CNT (0xc << 2) +#define DMC_MON_G12_THD_GRANT_CNT (0xd << 2) +#define DMC_MON_G12_FOR_GRANT_CNT (0xe << 2) +#define DMC_MON_G12_TIMER (0xf << 2) /* Each bit represent a axi line */ PMU_FORMAT_ATTR(event, "config:0-7");
Hello everyone, Running 6.2.0-rc8 on a S905X2 board, I note the following error in the kernel log: [ 1.059175] meson-g12-ddr-pmu ff638000.pmu: can't request region for resource [mem 0xff638000-0xff6380ff] [ 1.068647] meson-g12-ddr-pmu: probe of ff638000.pmu failed with error -16 Relevant DT node from decompiled DTB: (arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi + arch/arm64/boot/dts/amlogic/meson-g12a.dtsi) pmu@ff638000 { reg = <0x0 0xff638000 0x0 0x100 0x0 0xff638c00 0x0 0x100>; interrupts = <0x0 0x34 0x1>; compatible = "amlogic,g12a-ddr-pmu"; }; However, according to /proc/iomem ff638048-ff63805b : ff638048.video-lut video-lut@48 Corresponding DT node: (arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi) video-lut@48 { compatible = "amlogic,canvas"; reg = <0x0 0x48 0x0 0x14>; phandle = <0x22>; }; (with a base of 0xff600000 + 0x38000 = 0xff638000) Unless I'm mistaken, ff638000-0xff6380ff and ff638048-ff63805b cannot co-exist? A simple solution might be to specify the "actual" base of the register set, and count from 0 in the driver? With the above patch, /proc/iomem becomes: ff638048-ff63805b : ff638048.video-lut video-lut@48 ff638080-ff6380bf : ff638080.pmu pmu@ff638000 ff638c00-ff638cff : ff638080.pmu pmu@ff638000 (I didn't test that the driver actually works. I suppose one needs perf for that?) Regards.