smsc911x device tree binding questions

Hi Steve, all

I am in the process of defining a binding and relative code to parse it, in order to initialize the smsc911x series of controllers through the device tree instead of resorting to static data instantiated within the platform device struct (of course static config will still be there for platforms which do not rely on device tree probing, no worries, it is not disruptive at all). I have a couple of questions on the flags passed with the platform_data pointer (smsc911x_platform_config - flags member) and the way we may encode these bits of information in the tree:

/* Constants for flags */
#define SMSC911X_USE_16BIT                      (BIT(0)) #define SMSC911X_USE_32BIT                      (BIT(1)) #define SMSC911X_FORCE_INTERNAL_PHY             (BIT(2)) #define SMSC911X_FORCE_EXTERNAL_PHY             (BIT(3)) #define SMSC911X_SAVE_MAC_ADDRESS               (BIT(4))

* 16-32bit bus IF

#define SMSC911X_USE_16BIT              (BIT(0))
#define SMSC911X_USE_32BIT              (BIT(1))

- the SMSC911x host bus size can be retrieved from the chip idrev (for chip idrev having just 16-bit bus IF) or for controllers like 9118 that can have both (32/16), there is a D32/D16 HW strap readable from HW_CFG. The problem with these flags is that they define bus size from a CPU perspective if I got it right, in particular if the CPU can access the SMSC with 32-bit load/store to access 32 bit registers or have to resort to two "locked" 16-bit accesses to ensure atomicity. I think it is possible on some architectures to use a 32-bit load/store even if the smsc911x IF is 16-bit (e.g. bus downsize converters, which ensure 16-bit load/store locking).
It depends on the bus interconnect architecture. I think we need a property to define this, thoughts much appreciated.

* forcing internal/ext PHY

#define SMSC911X_FORCE_INTERNAL_PHY             (BIT(2)) #define SMSC911X_FORCE_EXTERNAL_PHY     (BIT(3))

- The device tree allows to describe the mdio bus and all PHYs which are present on a given controller. Again for controllers having just an internal PHY there is nothing to force, just use the internal PHY and that's it, that's what the driver does. If the controller supports both EXT and internal PHY, since the internal PHY is always at address 0x1 (am I correct ?), we may define something like:
if the tree defines a PHY @ 0x1 ("reg" property) let's use it (force it or at least higher priority than ext phy if both are defined), otherwise if the tree defines a PHY which is not @ 0x1 let's use it (force ext).
If no PHY is specified in the tree, let's resort to HW probing (EXT_PHY_DET in HW_CFG reg).

All in all, this means: just use the info from the tree, if there are no PHYs specified resort to HW probing for EXT PHY, if it fails use internal.

Acceptable ?

* save MAC ADDRESS

#define SMSC911X_SAVE_MAC_ADDRESS               (BIT(4))

-  Since we can encode the mac address in the device tree ("local-mac-address" property) I am not sure this flag is useful if probing from the tree is enabled. It is used if the chip is not fitted with an EEPROM, to save the MAC address set by a boot-loader before resetting the chip (at probe). Since we can still retrieve the MAC from the tree and reprogram it when the content of the registers is gone at reset, maybe there is no point in using the option (if probing from the tree) or at least no point in defining the flag in the tree.

Quoted 2 lines

- the SMSC911x host bus size can be retrieved from the chip idrev (for chip idrev having just 16-bit bus IF) or for controllers like 9118

Quoted 1 line

can have both (32/16), there is a D32/D16 HW strap readable from

Quoted 5 lines

The problem with these flags is that they define bus size from a CPU perspective if I got it right, in particular if the CPU can access the SMSC with 32-bit load/store to access 32 bit registers or have to resort to two "locked" 16-bit accesses to ensure atomicity. I think it is possible on some architectures to use a

Quoted 1 line

load/store even if the smsc911x IF is 16-bit (e.g. bus downsize

Quoted 3 lines

which ensure 16-bit load/store locking).
It depends on the bus interconnect architecture. I think we need a property to define this, thoughts much appreciated.

Quoted 6 lines

* forcing internal/ext PHY

#define SMSC911X_FORCE_INTERNAL_PHY (BIT(2))
#define SMSC911X_FORCE_EXTERNAL_PHY (BIT(3))

- The device tree allows to describe the mdio bus and all PHYs which

Quoted 1 line

on a given controller. Again for controllers having just an internal

Quoted 1 line

is nothing to force, just use the internal PHY and that's it, that's

Quoted 2 lines

the driver does. If the controller supports both EXT and internal PHY, since the internal PHY is always at address 0x1 (am I correct ?), we

Quoted 2 lines

something like:
if the tree defines a PHY @ 0x1 ("reg" property) let's use it (force

Quoted 1 line

least higher priority than ext phy if both are defined), otherwise if

Quoted 2 lines

defines a PHY which is not @ 0x1 let's use it (force ext). If no PHY is specified in the tree, let's resort to HW probing

Quoted 1 line

HW_CFG reg).

I'd say, if there is no phy data provided in the tree then default to the internal phy. Otherwise, use the phy pointed to by the phy-device property. It is also possible to specify a phy in the tree without an address to make the kernel probe for the address (although I need to double check that the support for it has actually been merged).

Quoted 1 line

All in all, this means: just use the info from the tree, if there are

Quoted 1 line

PHYs specified resort to HW probing for EXT PHY, if it fails use

Quoted 2 lines

Acceptable ?

Quoted 5 lines

* save MAC ADDRESS

#define SMSC911X_SAVE_MAC_ADDRESS (BIT(4))

- Since we can encode the mac address in the device tree

Quoted 1 line

property) I am not sure this flag is useful if probing from the tree

Quoted 1 line

It is used if the chip is not fitted with an EEPROM, to save the MAC

Quoted 1 line

set by a boot-loader before resetting the chip (at probe). Since we

Quoted 1 line

retrieve the MAC from the tree and reprogram it when the content of

Quoted 1 line

registers is gone at reset, maybe there is no point in using the

Quoted 1 line

(if probing from the tree) or at least no point in defining the flag

Quoted 1 line

the tree.

I'd agree, this isn't really useful in the device tree use case; but a property can be added for it if it was really necessary.

-----Original Message-----
From: [email protected] [mailto:[email protected]]
Sent: 29 August 2010 13:00
To: Grant Likely; Lorenzo Pieralisi
Cc: Catalin Marinas; David Miller; devicetree-discuss-uLR06cmDAlY/[email protected]; glikely-s3s/[email protected]; [email protected]
Subject: Re: [RFC] smsc911x device tree binding questions

Hi Lorenzo,

Quoted 2 lines

Quoted 2 lines

- the SMSC911x host bus size can be retrieved from the chip idrev (for chip idrev having just 16-bit bus IF) or for controllers like 9118

that

Quoted 1 line

Quoted 1 line

can have both (32/16), there is a D32/D16 HW strap readable from

HW_CFG.

Quoted 5 lines

Quoted 5 lines

The problem with these flags is that they define bus size from a CPU perspective if I got it right, in particular if the CPU can access the SMSC with 32-bit load/store to access 32 bit registers or have to resort to two "locked" 16-bit accesses to ensure atomicity. I think it is possible on some architectures to use a

32-bit

Quoted 1 line

Quoted 1 line

load/store even if the smsc911x IF is 16-bit (e.g. bus downsize

converters,

Quoted 3 lines

Quoted 3 lines

which ensure 16-bit load/store locking).
It depends on the bus interconnect architecture. I think we need a property to define this, thoughts much appreciated.

Absolutely right - this flag defines whether the DRIVER has to use 16 or 32 bit accesses in order to succesfully access the chip. If the bus hardware is capable of performing atomic pairs of 16-bit operations when we make a 32-bit read then this is very much preferred. The 16-bit path is MUCH slower as it has to take a spinlock on every access to ensure the two reads/writes are atomic. It's there as a last resort, and its use is discouraged if avoidable.

Autodetecting the bus width from register values is interesting, you'd have to test that you can reliably read the registers in all of the above modes. It's my understanding that if the device is strapped for 16-bit access it expects to see atomic pairs of reads, so I guess you could make a pair of 32-bit reads (of something known like BYTE_TEST) and analyse what you get back to determine how you're connected?

Quoted 6 lines

Quoted 6 lines

* forcing internal/ext PHY

#define SMSC911X_FORCE_INTERNAL_PHY (BIT(2))
#define SMSC911X_FORCE_EXTERNAL_PHY (BIT(3))

- The device tree allows to describe the mdio bus and all PHYs which

are present

Quoted 1 line

Quoted 1 line

on a given controller. Again for controllers having just an internal

PHY there

Quoted 1 line

Quoted 1 line

is nothing to force, just use the internal PHY and that's it, that's

what

Quoted 2 lines

Quoted 2 lines

the driver does. If the controller supports both EXT and internal PHY, since the internal PHY is always at address 0x1 (am I correct ?), we

may define

Quoted 2 lines

Quoted 2 lines

something like:
if the tree defines a PHY @ 0x1 ("reg" property) let's use it (force

it or at

Quoted 1 line

Quoted 1 line

least higher priority than ext phy if both are defined), otherwise if

the tree

Quoted 2 lines

Quoted 2 lines

defines a PHY which is not @ 0x1 let's use it (force ext). If no PHY is specified in the tree, let's resort to HW probing

(EXT_PHY_DET in

Quoted 9 lines

Quoted 1 line

HW_CFG reg).

I'd say, if there is no phy data provided in the tree then default to the internal phy. Otherwise, use the phy pointed to by the phy-device property. It is also possible to specify a phy in the tree without an address to make the kernel probe for the address (although I need to double check that the support for it has actually been merged).

Quoted 1 line

All in all, this means: just use the info from the tree, if there are

no

Quoted 1 line

Quoted 1 line

PHYs specified resort to HW probing for EXT PHY, if it fails use

internal.

Quoted 2 lines

Quoted 2 lines

Acceptable ?

Yes, bearing in mind that the majority of users do use the internal PHY this
sounds like a sensible plan.

Quoted 5 lines

Quoted 5 lines

* save MAC ADDRESS

#define SMSC911X_SAVE_MAC_ADDRESS (BIT(4))

- Since we can encode the mac address in the device tree

("local-mac-address"

Quoted 1 line

Quoted 1 line

property) I am not sure this flag is useful if probing from the tree

is enabled.

Quoted 1 line

Quoted 1 line

It is used if the chip is not fitted with an EEPROM, to save the MAC

address

Quoted 1 line

Quoted 1 line

set by a boot-loader before resetting the chip (at probe). Since we

can still

Quoted 1 line

Quoted 1 line

retrieve the MAC from the tree and reprogram it when the content of

the

Quoted 1 line

Quoted 1 line

registers is gone at reset, maybe there is no point in using the

option

Quoted 1 line

Quoted 1 line

(if probing from the tree) or at least no point in defining the flag

in

Quoted 2 lines

Quoted 1 line

the tree.

Quoted 2 lines

I'd agree, this isn't really useful in the device tree use case; but a property can be added for it if it was really necessary.

Yes, this is just a workaround for a specific use case: the bootloader "owns" the
MAC address configuration data and it's relatively hard to access this storage once
Linux boots.

A question though(16-bit/32-bit controllers, e.g. 9118):

If the device is strapped to 16-bit host bus mode, a single read 32-bit

reveal us if the bus converts the load 32-bit to two load 16-bit or not because the D[31:16] data bus lines are left in a high impedance

Right ?

Maybe a pair of reads - same address - is safer, the controller

the read (what does it mean in terms of bytes read ?) if it samples the

address (load 32 not split in 16-bit loads) on consecutive reads. If the conversion is correct the smsc returns the BYTE_TEST value

we would notice the problem.

I can write the code in the DT port context, but I do not have any 16-bit strapped smsc lan controller at hand, so I cannot test it, but I

the patch for review and testing on the list.

Quoted 35 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]]
Sent: 29 August 2010 13:00
To: Grant Likely; Lorenzo Pieralisi
Cc: Catalin Marinas; David Miller; devicetree-discuss-uLR06cmDAlY/[email protected]; glikely-s3s/[email protected]; [email protected]
Subject: Re: [RFC] smsc911x device tree binding questions

Hi Lorenzo,

Quoted 2 lines

Quoted 2 lines

- the SMSC911x host bus size can be retrieved from the chip idrev (for chip idrev having just 16-bit bus IF) or for controllers like 9118

that

Quoted 1 line

Quoted 1 line

can have both (32/16), there is a D32/D16 HW strap readable from

HW_CFG.

Quoted 5 lines

Quoted 5 lines

The problem with these flags is that they define bus size from a CPU perspective if I got it right, in particular if the CPU can access the SMSC with 32-bit load/store to access 32 bit registers or have to resort to two "locked" 16-bit accesses to ensure atomicity. I think it is possible on some architectures to use a

32-bit

Quoted 1 line

Quoted 1 line

load/store even if the smsc911x IF is 16-bit (e.g. bus downsize

converters,

Quoted 3 lines

Quoted 3 lines

which ensure 16-bit load/store locking).
It depends on the bus interconnect architecture. I think we need a property to define this, thoughts much appreciated.

Absolutely right - this flag defines whether the DRIVER has to use 16 or 32 bit accesses in order to succesfully access the chip.  If the bus hardware is capable of performing atomic pairs of 16-bit operations when we make a 32-bit read then this is very much preferred.  The 16-bit path is MUCH slower as it has to take a spinlock on every access to ensure the two reads/writes are atomic.  It's there as a last resort, and its use is discouraged if avoidable.

Quoted 6 lines

Autodetecting the bus width from register values is interesting, you'd have to test that you can reliably read the registers in all of the above modes.  It's my understanding that if the device is strapped for 16-bit access it expects to see atomic pairs of reads, so I guess you could make a pair of 32-bit reads (of something known like BYTE_TEST) and analyse what you get back to determine how you're connected?

Yes, we could try something like the code in smsc911x_init (BYTE_TEST pair of reads) but more generic to "probe" for the interconnection and set the flags accordingly.

A question though(16-bit/32-bit controllers, e.g. 9118):

If the device is strapped to 16-bit host bus mode, a single read 32-bit would reveal us if the bus converts the load 32-bit to two load 16-bit or not because the D[31:16] data bus lines are left in a high impedance state. Right ?

Maybe a pair of reads - same address -  is safer, the controller invalidate the read (what does it mean in terms of bytes read ?) if it samples the same address (load 32 not split in 16-bit loads) on consecutive reads. If the conversion is correct the smsc returns the BYTE_TEST value otherwise we would notice the problem.

I can write the code in the DT port context, but I do not have any 16-bit strapped smsc lan controller at hand, so I cannot test it, but I can post the patch for review and testing on the list.