[RFC] smsc911x device tree binding questions

[RFC] smsc911x device tree binding questions

[Lorenzo Pieralisi]

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.


Comments very much welcome, thank you very much indeed.

Cheers,
Lorenzo

Re: [RFC] smsc911x device tree binding questions

[Grant Likely]

On Fri, Aug 27, 2010 at 5:01 AM, Lorenzo Pieralisi
<Lorenzo.Pieralisi-5wv7dgnIgG8@public.gmane.org> wrote:
> 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.

On other devices we've has a single property to describe the access
width.  It would be appropriate to have an empty property named
"smsc,access-width-16bit", that when present selects 16 bit access,
but defaults to 32 when it isn't present.  For an example, see
drivers/block/xsysace.c and search for "8-bit".

>
> * 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).

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).

> 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.

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.

g.

Re: [RFC] smsc911x device tree binding questions

[Steve.Glendinning-sdUf+H5yV5I]

Hi Lorenzo,

> > - 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.

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?
 
> > * 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).
> 
> 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).
> 
> > 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 ?

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

> > * 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.
> 
> 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.

--
Steve

RE: [RFC] smsc911x device tree binding questions

[Lorenzo Pieralisi]

> -----Original Message-----
> From: Steve.Glendinning-sdUf+H5yV5I@public.gmane.org [mailto:Steve.Glendinning-sdUf+H5yV5I@public.gmane.org]
> Sent: 29 August 2010 13:00
> To: Grant Likely; Lorenzo Pieralisi
> Cc: Catalin Marinas; David Miller; devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org;
> glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org; Ian.Saturley-sdUf+H5yV5I@public.gmane.org
> Subject: Re: [RFC] smsc911x device tree binding questions
> 
> Hi Lorenzo,
> 
> > > - 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.
> 
> 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?

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.
 
> > > * 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).
> >
> > 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).
> >
> > > 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 ?
> 
> Yes, bearing in mind that the majority of users do use the internal PHY
> this
> sounds like a sensible plan.
> 
> > > * 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.
> >
> > 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.

Ok, all points taken, binding and relative code in progress.

RE: [RFC] smsc911x device tree binding questions

[Steve.Glendinning-sdUf+H5yV5I]

> 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 ?

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. 

If the device is strapped for 16-bit we must ALWAYS read or write in 
pairs.
Actually, one of the two reads is *supposed* to have A[1] set and one is 
supposed
to have it cleared to read the two halves.  The data sheet says that 
reading
the same 16-bit word twice gives invalid results but is not fatal.

> 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.

Quite a few device variants are 16-bit only (e.g. 9221), so there are 
plenty out
there.

Re: [RFC] smsc911x device tree binding questions

[Grant Likely]

On Fri, Sep 3, 2010 at 6:02 AM, Lorenzo Pieralisi
<Lorenzo.Pieralisi-5wv7dgnIgG8@public.gmane.org> wrote:
>> -----Original Message-----
>> From: Steve.Glendinning-sdUf+H5yV5I@public.gmane.org [mailto:Steve.Glendinning-sdUf+H5yV5I@public.gmane.org]
>> Sent: 29 August 2010 13:00
>> To: Grant Likely; Lorenzo Pieralisi
>> Cc: Catalin Marinas; David Miller; devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org;
>> glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org; Ian.Saturley-sdUf+H5yV5I@public.gmane.org
>> Subject: Re: [RFC] smsc911x device tree binding questions
>>
>> Hi Lorenzo,
>>
>> > > - 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.
>>
>> 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?
>
> 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.

It probably is not worth the effort.  Anyone bringing up this driver
on a new board will know what the bus access width is, and that will
never change.  Just encode it into a device tree property and be done
with it.

g.