> -----Original Message-----
> From: Pankaj Bansal
> Sent: Thursday, October 18, 2018 10:00 AM
> To: Florian Fainelli <f.faine...@gmail.com>; Andrew Lunn <and...@lunn.ch>
> Cc: netdev@vger.kernel.org
> Subject: RE: [PATCH v2 1/2] dt-bindings: net: add MDIO bus multiplexer driven
> by
> a regmap device
>
> Hi Florian
>
> > -----Original Message-----
> > From: Florian Fainelli [mailto:f.faine...@gmail.com]
> > Sent: Sunday, October 7, 2018 11:32 PM
> > To: Pankaj Bansal <pankaj.ban...@nxp.com>; Andrew Lunn
> > <and...@lunn.ch>
> > Cc: netdev@vger.kernel.org
> > Subject: Re: [PATCH v2 1/2] dt-bindings: net: add MDIO bus multiplexer
> > driven by a regmap device
> >
> >
> >
> > On 10/07/18 11:24, Pankaj Bansal wrote:
> > > Add support for an MDIO bus multiplexer controlled by a regmap
> > > device, like an FPGA.
> > >
> > > Tested on a NXP LX2160AQDS board which uses the "QIXIS" FPGA
> > > attached to the i2c bus.
> > >
> > > Signed-off-by: Pankaj Bansal <pankaj.ban...@nxp.com>
> > > ---
> > >
> > > Notes:
> > > V2:
> > > - Fixed formatting error caused by using space instead of tab
> > > - Add newline between property list and subnode
> > > - Add newline between two subnodes
> > >
> > > .../bindings/net/mdio-mux-regmap.txt | 95 ++++++++++++++++++
> > > 1 file changed, 95 insertions(+)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
> > > b/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
> > > new file mode 100644
> > > index 000000000000..88ebac26c1c5
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/net/mdio-mux-regmap.txt
> > > @@ -0,0 +1,95 @@
> > > +Properties for an MDIO bus multiplexer controlled by a regmap
> > > +
> > > +This is a special case of a MDIO bus multiplexer. A regmap device,
> > > +like an FPGA, is used to control which child bus is connected. The
> > > +mdio-mux node must be a child of the device that is controlled by a
> regmap.
> > > +The driver currently only supports devices with upto 32-bit registers.
> >
> > I would omit any sort of details about Linux constructs designed to
> > support specific needs (e.g: regmap) as well as putting driver
> > limitations into a binding document because it really ought to be
> > restricted to
> describing hardware.
> >
>
> Actually the platform driver mdio-mux-regmap.c, is generalization of mdio-mux-
> mmioreg.c As such, it doesn't describe any new hardware, so no new properties
> are described by it.
> The only new property is compatible field.
> I don't know how to describe this driver otherwise. Can you please help?
I further thought about it. mdio-mux-regmap.c is not meant to control a
specific device.
It is meant to control some registers of parent device. Therefore, IMO this
should not be a
Platform driver and there should not be any "compatible" property to which this
driver is associated.
Rather this driver should expose the APIs, which should be called by parent
devices' driver.
What is your thought on this ?
>
> > > +
> > > +Required properties in addition to the generic multiplexer properties:
> > > +
> > > +- compatible : string, must contain "mdio-mux-regmap"
> > > +
> > > +- reg : integer, contains the offset of the register that controls the
> > > bus
> > > + multiplexer. it can be 32 bit number.
> >
> > Technically it could be any "reg" property size, the only requirement
> > is that it must be of the appropriate size with respect to what the
> > parent node of this "mdio-mux-regmap" node has, determined by #address-
> cells/#size-cells width.
>
> We are reading only single cell of this property using "of_propert_read_u32".
> That is why I put the size in this.
>
> >
> > > +
> > > +- mux-mask : integer, contains an 32 bit mask that specifies which
> > > + bits in the register control the actual bus multiplexer. The
> > > + 'reg' property of each child mdio-mux node must be constrained by
> > > + this mask.
> >
> > Same thing here.
>
> We are reading only single cell of this property using "of_propert_read_u32".
> That is why I put the size in this.
>
> >
> > Since this is a MDIO mux, I would invite you to specify what the
> > correct #address-cells/#size-cells values should be (1, and 0
> > respectively as your example correctly shows).
> >
>
> I will mention #address-cells/#size-cells values
>
> > > +
> > > +Example:
> > > +
> > > +The FPGA node defines a i2c connected FPGA with a register space of
> > > +0x30
> > bytes.
> > > +For the "EMI2" MDIO bus, register 0x54 (BRDCFG4) controls the mux
> > > +on that
> > bus.
> > > +A bitmask of 0x07 means that bits 0, 1 and 2 (bit 0 is lsb) are the
> > > +bits on
> > > +BRDCFG4 that control the actual mux.
> > > +
> > > +i2c@2000000 {
> > > + compatible = "fsl,vf610-i2c";
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + reg = <0x0 0x2000000 0x0 0x10000>;
> > > + interrupts = <0 34 0x4>; // Level high type
> > > + clock-names = "i2c";
> > > + clocks = <&clockgen 4 7>;
> > > + fsl-scl-gpio = <&gpio2 15 0>;
> > > + status = "okay";
> > > +
> > > + /* The FPGA node */
> > > + fpga@66 {
> > > + compatible = "fsl,lx2160aqds-fpga", "fsl,fpga-qixis-i2c";
> > > + reg = <0x66>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + mdio1_mux@54 {
> > > + compatible = "mdio-mux-regmap", "mdio-mux";
> > > + mdio-parent-bus = <&emdio2>; /* MDIO bus */
> > > + reg = <0x54>; /* BRDCFG4 */
> > > + mux-mask = <0x07>; /* EMI2_MDIO */
> > > + #address-cells=<1>;
> > > + #size-cells = <0>;
> > > +
> > > + mdio1_ioslot1@0 { // Slot 1
> > > + reg = <0x00>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + phy1@1 {
> > > + reg = <1>;
> > > + compatible = "ethernet-phy-
> > id0210.7441";
> > > + };
> > > +
> > > + phy1@0 {
> > > + reg = <0>;
> > > + compatible = "ethernet-phy-
> > id0210.7441";
> > > + };
> > > + };
> > > +
> > > + mdio1_ioslot2@1 { // Slot 2
> > > + reg = <0x01>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + };
> > > +
> > > + mdio1_ioslot3@2 { // Slot 3
> > > + reg = <0x02>;
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + };
> > > + };
> > > + };
> > > +};
> > > +
> > > + /* The parent MDIO bus. */
> > > + emdio2: mdio@0x8B97000 {
> > > + compatible = "fsl,fman-memac-mdio";
> > > + reg = <0x0 0x8B97000 0x0 0x1000>;
> > > + device_type = "mdio";
> > > + little-endian;
> > > +
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + };
> > >
> >
> > --
> > Florian
