Hi Jacopo,
On Wednesday 11 Jan 2017 16:15:30 jacopo mondi wrote:
> On 11/01/2017 12:22, Laurent Pinchart wrote:
> > Hi Jacopo,
>
> [snip]
>
> >>>> Here is my general question: Which of these 2 approaches are better?
> >>>>
> >>>> A) In the DT, the user ask "enable Ethernet please" and magic happens
> >>>> in the pfc driver.
> >>>>
> >>>> B) In the DT, the user looks up the correct pin/function assignments in
> >>>> the SoC Hardware Manual and manually spells out what they need.
> >>>>
> >>>> R-Car looks more like A. I've been using a driver that looks more like
> >>>> B.
> >>>>
> >>>> For most drivers (USB, MMC, SDHI, etc..,), I'm happy when 'magic
> >>>> happens' and I don't really have to open a HW manual at all.
> >>>> But, for something like setting up the PFC when someone gets a shiny
> >>>> new board, making people actually open a HW manual seems acceptable to
> >>>> me.
> >>>
> >>> From a user point of view, option A looks better to me. However, it has
> >>> two drawbacks:
> >>>
> >>> - Through deciding what pin groups we make available we create a DT ABI
> >>> that will make it difficult to fix mistakes in case the groups are not
> >>> fine-grained enough.
> >>>
> >>> - The data tables in C code are large, and we end up compiling many of
> >>> them in multi-platform kernel, significantly increasing the kernel size.
> >>>
> >>> I would thus favour option B.
> >>
> >> That would be saner, I agree.
> >>
> >> And a much saner way of doing this would be, in my understanding, not
> >> using the group-based sh-pfc drivers used for R-Car and back it up with
> >> a pin-based equivalent, where to hook drivers for each specific hardware.
> >
> > We can't really do that for the existing SoCs as the concept of groups is
> > present in the hardware. Not only do we need to select per-pin functions,
> > but there are also register bits that perform pin muxing for whole
> > groups. If this changes in future generations of the SoCs we then could
> > do away with the data tables, but for now we're stuck with them.
>
> Well, for the RZ/A platform "groups" are really pins themselves for what
> I see in dts and pinctrl driver.
On RZ/A, yes. My comment was related to the R-Car Gen2 and Gen3 families.
> -------------------------------------------------------------------
> Eg. SCIF2
>
> From: arch/arm/boot/dts/r7s72100-genmai.dts
> scif2_pins: serial2 {
> renesas,groups = "scif2_txd_p3_0", "scif2_rxd_p3_2";
> renesas,function = "scif2";
> };
>
> From: drivers/pinctrl/sh-pfc/pfc-r7s72100.c
> #define SCIF2(fn) \
> fn(scif2, clk, 3, 0, 4) \
> fn(scif2, txd, 3, 1, 4) \
> *fn(scif2, rxd, 3, 2, 4) \*
> *fn(scif2, txd, 3, 0, 6) \*
> fn(scif2, clk, 4, 1, 5) \
> fn(scif2, txd, 4, 2, 5) \
> fn(scif2, rxd, 4, 3, 5) \
> fn(scif2, txd, 4, 14, 7) \
> fn(scif2, rxd, 4, 15, 7) \
> fn(scif2, txd, 6, 2, 7) \
> fn(scif2, rxd, 6, 3, 7) \
> fn(scif2, clk, 8, 3, 7) \
> fn(scif2, rxd, 8, 4, 7) \
> fn(scif2, txd, 8, 6, 7)
> -------------------------------------------------------------------
>
> It seems to me that "renesas,function" is used to identify all "groups"
> (aka pins here) that can be made to work in SCIF2 mode (the 14 pins
> above) and "renesas,groups" identifies which pins to configure for the
> desired function (the alternate function number is the last entry of the
> "fn(function, name, bank, pin, function)" macro).
>
> Long story short: this ends up writing a bunch of registers for each
> single "group" (pin)
>
> -------------------------------------------------------------------
> Eg.
>
> scif2_rxd_p3_2 configuration as function #4 writes registers
>
> PMC3[2] = 1, PFC3[2] = 1, PFCE3[2] = 1, PFCAE3[2] = 0
>
> scif2_txd_p3_0 configuration as function #6 writes registers
>
> PMC3[2] = 1, PFC3[2] = 1, PFCE3[2] = 0, PFCAE3[2] = 1
>
> And no global group configuration register has to be set, as it happens
> instead for R-Car series.
> -------------------------------------------------------------------
>
> As shown in the RFC series "pinctrl: sh-pfc: r7s72100: Add IO mode
> selection" I sent out along with this one, this model is not flexible
> enough, and the attempt I proposed to extend it to support additional
> pin configuration options ends up multiplying the already gigantic macro
> tables.
>
> I was wondering if that should not be be turned into something more
> similar to the pinctrl-single defined ABI. In that case it would look
> like this (please excuse the pseduo-dts syntax here)
>
> scif2_pins: serial2 {
> renesas,pin = <3, 2, MUX_MODE4, PIN_CONFIG_OPTIONS,
> 3, 0, MUX_MODE6, PIN_CONFIG_OPTIONS>
This should just be "pin", not "renesas,pin".
> };
Yes, I believe that should be done, especially given that the datasheet for
the SoC is public. For other SoCs the current PFC driver model exposes the
information needed to configure pin muxing even if you can't access the
datasheet, which is an option that I'd hate losing.
> I'm under the impression that pinctrl-single would almost do, if not for
> the write function bit, that for OMAP platform results in a write to a
> single register, while (for RZ/A at least) we have to spread it on a
> bunch of different registers.
That's correct, but a pinctrl-single-like driver could be implemented
relatively easily without all the data tables. We might still need some data
if we want to validate DT entries though, for instance to store the valid pin
config and/or options for each pin.
> Also, Chris has some examples of this in his BSPs iirc, so I guess there
> is code for the RZ series out there already implementing this per-pin ABI...
>
> Sorry for the long email, hope it did not frustrate anyone.
>
> >> Looks like pinmux-single, yes, but that driver bets on the ability to
> >> set pin functions and configurations with a write to a single register
> >> while, at least for RZ/A, the same is scattered among several registers
> >> (I may be wrong on the single register requirement for pinctrl-single
> >> though)
> >>
> >> So, I guess what direction to take depends on whether or not we're going
> >> to see more hardware with a per-pin configuration that would benefit
> >> from this new rz-pfc driver (it seems so) and if this justify splitting
> >> sh-pfc in two, a group-based one for R-Car devices (and all devices
> >> there already) and a new pin-based one.
> >>
> >> Or maybe we can tie pin-based configuration in sh-pfc and it's me not
> >> seeing how to do that.
--
Regards,
Laurent Pinchart
