Netdev Archive on lore.kernel.org
 help / color / Atom feed
* Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support
@ 2020-04-01 13:35 Florinel Iordache
  2020-04-01 13:51 ` Andrew Lunn
  0 siblings, 1 reply; 5+ messages in thread
From: Florinel Iordache @ 2020-04-01 13:35 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: davem, netdev, f.fainelli, hkallweit1, linux, devicetree,
	linux-doc, robh+dt, mark.rutland, kuba, corbet, shawnguo, Leo Li,
	Madalin Bucur (OSS),
	Ioana Ciornei, linux-kernel, Florinel Iordache

> On Thu, Mar 26, 2020 at 03:51:19PM +0200, Florinel Iordache wrote:
> > +static void setup_supported_linkmode(struct phy_device *bpphy) {
> > +     struct backplane_phy_info *bp_phy = bpphy->priv;
> 
> I'm not sure it is a good idea to completely take over phydev->priv like this, in
> what is just helper code. What if the PHY driver needs memory of its own? There
> are a few examples of this already in other PHY drivers. Could a KR PHY contain
> a temperature sensor? Could it contain statistics counters which need
> accumulating?
> 
>         Andrew

Backplane KR driver allocates memory for structure backplane_phy_info
which is saved in phydev->priv. After all this is the purpose of priv
according to its description in phy.h: <<private data pointer For use
by PHYs to maintain extra state>>. Here the priv is used to maintain
extra state needed for backplane. This way the backplane specific data
becomes available for all PHY callbacks (defined in struct phy_driver)
that receive a pointer to phy_device structure. This initial version
doesn't include accumulating statistics counters but we have in plan
to add these in future versions. The counters will be kept in specific
structures as members of the main backplane data mentioned above
and entire support will be integrated with ethtool.

Florin.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support
  2020-04-01 13:35 [PATCH net-next 6/9] net: phy: add backplane kr driver support Florinel Iordache
@ 2020-04-01 13:51 ` Andrew Lunn
  2020-04-01 14:21   ` [EXT] " Florinel Iordache
  0 siblings, 1 reply; 5+ messages in thread
From: Andrew Lunn @ 2020-04-01 13:51 UTC (permalink / raw)
  To: Florinel Iordache
  Cc: davem, netdev, f.fainelli, hkallweit1, linux, devicetree,
	linux-doc, robh+dt, mark.rutland, kuba, corbet, shawnguo, Leo Li,
	Madalin Bucur (OSS),
	Ioana Ciornei, linux-kernel

On Wed, Apr 01, 2020 at 01:35:36PM +0000, Florinel Iordache wrote:
> > On Thu, Mar 26, 2020 at 03:51:19PM +0200, Florinel Iordache wrote:
> > > +static void setup_supported_linkmode(struct phy_device *bpphy) {
> > > +     struct backplane_phy_info *bp_phy = bpphy->priv;
> > 
> > I'm not sure it is a good idea to completely take over phydev->priv like this, in
> > what is just helper code. What if the PHY driver needs memory of its own? There
> > are a few examples of this already in other PHY drivers. Could a KR PHY contain
> > a temperature sensor? Could it contain statistics counters which need
> > accumulating?
> > 
> >         Andrew
> 
> Backplane KR driver allocates memory for structure backplane_phy_info
> which is saved in phydev->priv. After all this is the purpose of priv
> according to its description in phy.h: <<private data pointer For use
> by PHYs to maintain extra state>>. Here the priv is used to maintain
> extra state needed for backplane. This way the backplane specific data
> becomes available for all PHY callbacks (defined in struct phy_driver)
> that receive a pointer to phy_device structure. This initial version
> doesn't include accumulating statistics counters but we have in plan
> to add these in future versions. The counters will be kept in specific
> structures as members of the main backplane data mentioned above
> and entire support will be integrated with ethtool.

Hi Florinel

And what about hwmon, or anything else which a driver needs memory
for?

As far as i see it, we have two bodies of code here. There is a set of
helpers which implement most of the backplane functionality. And then
there is an example driver for your hardware. In the future we expect
other drivers to be added for other vendors hardware.

phydev->priv is for the driver. helpers should not assume they have
complete control over it.

Anyway, this may be a mute point. Lets first solve the problem of how
a PCS is represented.

  Andrew


^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [EXT] Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support
  2020-04-01 13:51 ` Andrew Lunn
@ 2020-04-01 14:21   ` Florinel Iordache
  0 siblings, 0 replies; 5+ messages in thread
From: Florinel Iordache @ 2020-04-01 14:21 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: davem, netdev, f.fainelli, hkallweit1, linux, devicetree,
	linux-doc, robh+dt, mark.rutland, kuba, corbet, shawnguo, Leo Li,
	Madalin Bucur (OSS),
	Ioana Ciornei, linux-kernel, Florinel Iordache

> On Wed, Apr 01, 2020 at 01:35:36PM +0000, Florinel Iordache wrote:
> > > On Thu, Mar 26, 2020 at 03:51:19PM +0200, Florinel Iordache wrote:
> > > > +static void setup_supported_linkmode(struct phy_device *bpphy) {
> > > > +     struct backplane_phy_info *bp_phy = bpphy->priv;
> > >
> > > I'm not sure it is a good idea to completely take over phydev->priv
> > > like this, in what is just helper code. What if the PHY driver needs
> > > memory of its own? There are a few examples of this already in other
> > > PHY drivers. Could a KR PHY contain a temperature sensor? Could it
> > > contain statistics counters which need accumulating?
> > >
> > >         Andrew
> >
> > Backplane KR driver allocates memory for structure backplane_phy_info
> > which is saved in phydev->priv. After all this is the purpose of priv
> > according to its description in phy.h: <<private data pointer For use
> > by PHYs to maintain extra state>>. Here the priv is used to maintain
> > extra state needed for backplane. This way the backplane specific data
> > becomes available for all PHY callbacks (defined in struct phy_driver)
> > that receive a pointer to phy_device structure. This initial version
> > doesn't include accumulating statistics counters but we have in plan
> > to add these in future versions. The counters will be kept in specific
> > structures as members of the main backplane data mentioned above and
> > entire support will be integrated with ethtool.
> 
> Hi Florinel
> 
> And what about hwmon, or anything else which a driver needs memory for?
> 
> As far as i see it, we have two bodies of code here. There is a set of helpers
> which implement most of the backplane functionality. And then there is an
> example driver for your hardware. In the future we expect other drivers to be
> added for other vendors hardware.
> 
> phydev->priv is for the driver. helpers should not assume they have
> complete control over it.
> 
> Anyway, this may be a mute point. Lets first solve the problem of how a PCS is
> represented.
> 
>   Andrew

Hi Andrew,

Backplane driver was designed as a generic backplane driver over
the PHY Abstraction Layer, containing standard implementations over
which several specific devices can be added.
(please see the diagram existent in chapter: Ethernet Backplane support
architecture, in the Documentation/networking/backplane.rst)
So according to this design, the backplane driver can use the priv pointer
provided by the layer below which is the PHY to be used as extra state
by the next upper layer which is generic backplane. On the same concept
we can provide another priv pointer in backplane main structure which is
equivalent to phy_device to be used by the specific backplane drivers on
the upper layer.
Actually for this reason, in v2 which I am currently working, I already
renamed the structure 'backplane_phy_info' to 'backplane_device'

Thank you,
Florin.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [EXT] Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support
  2020-03-27 13:02     ` [EXT] " Florinel Iordache
@ 2020-03-27 13:23       ` Andrew Lunn
  0 siblings, 0 replies; 5+ messages in thread
From: Andrew Lunn @ 2020-03-27 13:23 UTC (permalink / raw)
  To: Florinel Iordache
  Cc: davem, netdev, f.fainelli, hkallweit1, linux, devicetree,
	linux-doc, robh+dt, mark.rutland, kuba, corbet, shawnguo, Leo Li,
	Madalin Bucur (OSS),
	Ioana Ciornei, linux-kernel

On Fri, Mar 27, 2020 at 01:02:17PM +0000, Florinel Iordache wrote:
> > > +static u32 le_ioread32(void __iomem *reg) {
> > > +     return ioread32(reg);
> > > +}
> > > +
> > > +static void le_iowrite32(u32 value, void __iomem *reg) {
> > > +     iowrite32(value, reg);
> > > +}
> > > +
> > > +static u32 be_ioread32(void __iomem *reg) {
> > > +     return ioread32be(reg);
> > > +}
> > > +
> > > +static void be_iowrite32(u32 value, void __iomem *reg) {
> > > +     iowrite32be(value, reg);
> > > +}
> > 
> > This is very surprising to me. I've not got my head around the structure of this
> > code yet, but i'm surprised to see memory mapped access functions in generic
> > code.
> > 
> >        Andrew
> 
> Hi Andrew,
> 
> This is part of the framework used to automatically setup desired I/O 
> callbacks for memory access according to device specific endianness 
> which is specified in the specific device tree (DTS).
> This approach (explained below) was used to avoid the potential 
> redundant code related to memory access LE/BE which should be 
> similar for all devices. 

All devices which are using mmio. I assume the standard does not say
anything about memory mapped IO. It talks just about MDIO registers.

I would expect the generic code to just have generic accessors, which
could work for MMIO, yet more MDIO registers, i2c, spi, etc.

So add another support file which adds an MMIO implementation of these
generic access functions. Any driver which uses MMIO can pull it in.
The same should be true for the DT binding. Don't assume MMIO in the
generic binding.

> This portion of code is just preparing these four static IO routines 
> for specific endianness access LE/BE

Linux has a lot of MMIO accessors. Are you sure there is not one which
will do the right thing, making use of cpu_le32() or cpu_be32()
etc. Or are there different variants of the hardware, with some using
BE registers and some using LE registers? Note, this is all about the
endianness of the register, not the endianness of the cpu. cpu_le32()
will be a NOP when the CPU is running LE.

     Andrew

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [EXT] Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support
  2020-03-27  1:07   ` Andrew Lunn
@ 2020-03-27 13:02     ` Florinel Iordache
  2020-03-27 13:23       ` Andrew Lunn
  0 siblings, 1 reply; 5+ messages in thread
From: Florinel Iordache @ 2020-03-27 13:02 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: davem, netdev, f.fainelli, hkallweit1, linux, devicetree,
	linux-doc, robh+dt, mark.rutland, kuba, corbet, shawnguo, Leo Li,
	Madalin Bucur (OSS),
	Ioana Ciornei, linux-kernel, Florinel Iordache

> > +static u32 le_ioread32(void __iomem *reg) {
> > +     return ioread32(reg);
> > +}
> > +
> > +static void le_iowrite32(u32 value, void __iomem *reg) {
> > +     iowrite32(value, reg);
> > +}
> > +
> > +static u32 be_ioread32(void __iomem *reg) {
> > +     return ioread32be(reg);
> > +}
> > +
> > +static void be_iowrite32(u32 value, void __iomem *reg) {
> > +     iowrite32be(value, reg);
> > +}
> 
> This is very surprising to me. I've not got my head around the structure of this
> code yet, but i'm surprised to see memory mapped access functions in generic
> code.
> 
>        Andrew

Hi Andrew,

This is part of the framework used to automatically setup desired I/O 
callbacks for memory access according to device specific endianness 
which is specified in the specific device tree (DTS).
This approach (explained below) was used to avoid the potential 
redundant code related to memory access LE/BE which should be 
similar for all devices. 

This portion of code is just preparing these four static IO routines 
for specific endianness access LE/BE which are then installed as 
callbacks by the generic driver on generic DT parsing routine: 
backplane_parse_dt according to endianness flag:
+    /* setup ioread/iowrite according to endianness */
+    if (bp_phy->bp_dev.is_little_endian) {
+        bp_phy->bp_dev.io.read32 = le_ioread32;
+        bp_phy->bp_dev.io.write32 = le_iowrite32;
+    } else {
+        bp_phy->bp_dev.io.read32 = be_ioread32;
+        bp_phy->bp_dev.io.write32 = be_iowrite32;
+    }

These io callbacks are setup in the following structure:
+/* Endianness specific memory I/O operations */ 
struct mem_io_ops {

which is part of generic structure:
+/* Backplane device info */
+struct backplane_dev_info {
+. . . 
+    struct mem_io_ops io;

which in the end will be used directly by the device specific code for 
specific memory access according to specific endianness specified 
in the DTS. 

The endianness flag must be correctly set by the device specific code 
before calling the generic function backplane_parse_dt, according to 
device specific endianness specified in the specific device tree DTS:
+bp_phy->bp_dev.is_little_endian = of_property_read_bool(serdes_node,
                            "little-endian");

This action to setup desired IO callbacks could also be performed in the 
specific device code but by using this framework the process is more 
automatic, it's reusing the logic and therefore decreasing the overall 
LOC required.
If any specific device is doing this action by itself (which is a similar 
action regardless the specific device) then we will end up with a lot of 
redundant code.

Florin.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-01 13:35 [PATCH net-next 6/9] net: phy: add backplane kr driver support Florinel Iordache
2020-04-01 13:51 ` Andrew Lunn
2020-04-01 14:21   ` [EXT] " Florinel Iordache
  -- strict thread matches above, loose matches on Subject: below --
2020-03-26 13:51 [PATCH net-next 0/9] net: ethernet backplane support Florinel Iordache
2020-03-26 13:51 ` [PATCH net-next 6/9] net: phy: add backplane kr driver support Florinel Iordache
2020-03-27  1:07   ` Andrew Lunn
2020-03-27 13:02     ` [EXT] " Florinel Iordache
2020-03-27 13:23       ` Andrew Lunn

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git