Re: [PATCH v4 3/5] net: ethernet: cpsw: introduce ti,am3352-cpsw compatible string

[Santosh Shilimkar <santosh.shilimkar@ti.com>]

On Friday 23 August 2013 12:45 PM, Mugunthan V N wrote:
> On Friday 23 August 2013 07:53 PM, Santosh Shilimkar wrote:
>> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote:
>>> In order to support features that are specific to the AM335x IP, we have
>>> to add hardware types and another compatible string.
>>>
>>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>>> ---
>>>  Documentation/devicetree/bindings/net/cpsw.txt |  3 ++-
>>>  drivers/net/ethernet/ti/cpsw.c                 | 32 ++++++++++++++++++++------
>>>  drivers/net/ethernet/ti/cpsw.h                 |  1 +
>>>  3 files changed, 28 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
>>> index 4e5ca54..b717458 100644
>>> --- a/Documentation/devicetree/bindings/net/cpsw.txt
>>> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
>>> @@ -2,7 +2,8 @@ TI SoC Ethernet Switch Controller Device Tree Bindings
>>>  ------------------------------------------------------
>>>  
>>>  Required properties:
>>> -- compatible		: Should be "ti,cpsw"
>>> +- compatible		: Should be "ti,cpsw" for generic cpsw support, or
>>> +			  "ti,am3352-cpsw" for AM3352 SoCs
>>>  - reg			: physical base address and size of the cpsw
>>>  			  registers map.
>>>  			  An optional third memory region can be supplied if
>>> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
>>> index 7a25ff4..73c44cb6 100644
>>> --- a/drivers/net/ethernet/ti/cpsw.c
>>> +++ b/drivers/net/ethernet/ti/cpsw.c
>>> @@ -155,6 +155,11 @@ do {								\
>>>  		((priv->data.dual_emac) ? priv->emac_port :	\
>>>  		priv->data.active_slave)
>>>  
>>> +enum {
>>> +	CPSW_TYPE_GENERIC,
>>> +	CPSW_TYPE_AM33XX
>>> +};
>>> +
>>>  static int debug_level;
>>>  module_param(debug_level, int, 0);
>>>  MODULE_PARM_DESC(debug_level, "cpsw debug level (NETIF_MSG bits)");
>>> @@ -1692,17 +1697,36 @@ static void cpsw_slave_init(struct cpsw_slave *slave, struct cpsw_priv *priv,
>>>  	slave->port_vlan = data->dual_emac_res_vlan;
>>>  }
>>>  
>>> +static const struct of_device_id cpsw_of_mtable[] = {
>>> +	{
>>> +		.compatible	= "ti,am3352-cpsw",
>> I didn't notice this earlier, but can't you use the IP version
>> as a compatible instead of using a SOC name. Whats really SOC specific
>> on this IP ? Sorry i have missed any earlier discussion on this but
>> this approach doesn't seem good. Its like adding SOC checks in the
>> driver subsystem.
>>
>>
> But the same IP can be used in different SoC as well where the control
> register may be different as per the Silicon Integration team's decision?
> 
> Ideally there should be a separate control module driver so that it can
> take care of different SoC related needs.
> 
Instead of having one control module driver to address all the requirements
in various IP's, you could specifically address that in subsystem driver layer.
SOC guys invariably use control modules for certain functionality since it
is easy and quick fix. And their is uniformity in the way they are
implemented for different IP needs.

In this specific case, if the CPSW IP version has some part of the
functionality implemented using control module that is just harfaware
choice. Its not mandatory to partition that in two sub-system. Rather
consider it as a special IP version and treat it accordingly in the
driver.

Regards,
Santosh