Hello,
On Fri Jan 26, 2024 at 12:52 PM CET, Krzysztof Kozlowski wrote:
> On 25/01/2024 12:40, Théo Lebrun wrote:
> > Hello,
> >
> > On Wed Jan 24, 2024 at 8:22 PM CET, Rob Herring wrote:
> >> On Wed, Jan 24, 2024 at 11:40 AM Théo Lebrun <theo.lebrun@xxxxxxxxxxx> wrote:
> >>> On Wed Jan 24, 2024 at 6:28 PM CET, Théo Lebrun wrote:
> >>>> On Wed Jan 24, 2024 at 4:14 PM CET, Rob Herring wrote:
> >>>>> On Tue, Jan 23, 2024 at 07:46:49PM +0100, Théo Lebrun wrote:
> >
> > [...]
> >
> >>>>>> + };
> >>>>>> +
> >>>>>> + pinctrl-b {
> >>>>>> + compatible = "mobileye,eyeq5-b-pinctrl";
> >>>>>> + #pinctrl-cells = <1>;
> >>>>>> + };
> >>>>>> + };
> >>>>>
> >>>>> This can all be simplified to:
> >>>>>
> >>>>> system-controller@e00000 {
> >>>>> compatible = "mobileye,eyeq5-olb", "syscon";
> >>>>> reg = <0xe00000 0x400>;
> >>>>> #reset-cells = <2>;
> >>>>> #clock-cells = <1>;
> >>>>> clocks = <&xtal>;
> >>>>> clock-names = "ref";
> >>>>>
> >>>>> pins { ... };
> >>>>> };
> >>>>>
> >>>>> There is no need for sub nodes unless you have reusable blocks or each
> >>>>> block has its own resources in DT.
> >>>>
> >>>> That is right, and it does simplify the devicetree as you have shown.
> >>>> However, the split nodes gives the following advantages:
> >>>>
> >>>> - Devicetree-wise, it allows for one alias per function.
> >>>> `clocks = <&clocks EQ5C_PLL_CPU>` is surely more intuitive
> >>>> than `clocks = <&olb EQ5C_PLL_CPU>;`. Same for reset.
> >>
> >> clocks: resets: pinctrl: system-controller@e00000 {
> >>
> >>>>
> >>>> - It means an MFD driver must be implemented, adding between 100 to 200
> >>>> lines of boilerplate code to the kernel.
> >>
> >> From a binding perspective, not my problem... That's Linux details
> >> defining the binding. What about u-boot, BSD, future versions of Linux
> >> with different structure?
> >>
> >> I don't think an MFD is required here. A driver should be able to be
> >> both clock and reset provider. That's pretty common. pinctrl less so.
> >
> > @Rob & @Krzysztof: following Krzysztof's question about the memory map
> > and adding ressources to the system-controller, I was wondering if the
> > following approach would be more suitable:
>
> More or less (missing ranges, unit addresses, lower-case hex etc).
Yeah the details are not really on point, it was only a proposal
highlighting a different way of dealing with the current situation.
Looks like it is suitable to you.
> > olb: system-controller@e00000 {
> > compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd";
> > reg = <0 0xe00000 0x0 0x400>;
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > clocks: clock-controller {
> > compatible = "mobileye,eyeq5-clk";
> > reg = <0x02c 0x7C>;
> > #clock-cells = <1>;
> > clocks = <&xtal>;
> > clock-names = "ref";
> > };
> >
> > reset: reset-controller {
> > compatible = "mobileye,eyeq5-reset";
> > reg = <0x004 0x08>, <0x120 0x04>, <0x200 0x34>;
> > reg-names = "d0", "d2", "d1";
> > #reset-cells = <2>;
> > };
> >
> > pinctrl0: pinctrl-a {
> > compatible = "mobileye,eyeq5-a-pinctrl";
> > reg = <0x0B0 0x30>;
> > };
> >
> > pinctrl1: pinctrl-b {
> > compatible = "mobileye,eyeq5-b-pinctrl";
> > reg = <0x0B0 0x30>;
>
> Duplicate reg?
Yes, the mapping is intertwined. Else it could be three ressources per
pinctrl. Just really small ones.
- 0xB0 mapping A
- 0xB4 mapping B
- 0xB8
- 0xBC
- 0xC0 pull-down A
- 0xC4 pull-up A
- 0xC8 pull-down B
- 0xCC pull-up B
- 0xD0 drive-strength lo A
- 0xD4 drive-strength hi A
- 0xD8 drive-strength lo B
- 0xDC drive-strength hi B
0xB8 is unrelated (I2C speed & SPI CS). 0xBC is a hole.
Thanks,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
