| Message ID | 1377267365-24057-4-git-send-email-zonque@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Regards, Santosh
Hi Santosh, [...] >> +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. Hehe, welcome to the club! I keep arguing about that as well :-) I think we should create a Facebook group: "against-the-soc-version-in-the-compatible-string". Regards, Benoit
On Friday 23 August 2013 11:22 AM, Benoit Cousson wrote: > Hi Santosh, > > [...] > >>> +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. > > Hehe, welcome to the club! > I keep arguing about that as well :-) > > I think we should create a Facebook group: "against-the-soc-version-in-the-compatible-string". > Indeed !! Regards, Santosh
On 23.08.2013 16:23, Santosh Shilimkar wrote: > On Friday 23 August 2013 10:16 AM, Daniel Mack wrote: >> +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. As I already mentioned in the cover letter and in the commit message, I just don't know which criteria makes most sense here. On a general note, I would say that chances that this exactly IP core with the same version number will appear on some other silicon which doesn't support the control mode register in an AM33xx fashion, is not necessarily negligible. So what that new compatible string denotes is the cpsw in a version as found on am3352 SoCs, which is actually exactly what it does. I don't have a strong opinion here, but see your point. I just don't have a better idea on how to treat that. Daniel
On 8/23/2013 7: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. The objection then should not just be to the name of compatible string, but to handling AM335x control module register in CPSW driver. What would be the alternate method of doing what Daniel is doing? I vaguely remember there were attempts to develop an independent control module driver that were abandoned. Thanks, Sekhar
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. Regards Mugunthan V N
On Friday 23 August 2013 12:30 PM, Daniel Mack wrote: > On 23.08.2013 16:23, Santosh Shilimkar wrote: >> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote: > >>> +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. > > As I already mentioned in the cover letter and in the commit message, I > just don't know which criteria makes most sense here. > > On a general note, I would say that chances that this exactly IP core > with the same version number will appear on some other silicon which > doesn't support the control mode register in an AM33xx fashion, is not > necessarily negligible. > > So what that new compatible string denotes is the cpsw in a version as > found on am3352 SoCs, which is actually exactly what it does. > > I don't have a strong opinion here, but see your point. I just don't > have a better idea on how to treat that. > So just stick the IP version or call it cpsw-v1... cpsw-v2 etc. That way if in future if someone uses those features, they can use this compatible if they don't they use the one which suites that SOC. Regards, Santosh
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
On 8/23/2013 10:26 PM, Santosh Shilimkar wrote: > On Friday 23 August 2013 12:30 PM, Daniel Mack wrote: >> On 23.08.2013 16:23, Santosh Shilimkar wrote: >>> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote: >> >>>> +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. >> >> As I already mentioned in the cover letter and in the commit message, I >> just don't know which criteria makes most sense here. >> >> On a general note, I would say that chances that this exactly IP core >> with the same version number will appear on some other silicon which >> doesn't support the control mode register in an AM33xx fashion, is not >> necessarily negligible. >> >> So what that new compatible string denotes is the cpsw in a version as >> found on am3352 SoCs, which is actually exactly what it does. >> >> I don't have a strong opinion here, but see your point. I just don't >> have a better idea on how to treat that. >> > So just stick the IP version or call it cpsw-v1... cpsw-v2 etc. If this could be handled using IP version then the right way would be to just read the IP version from hardware and use it. No need of DT property. Thanks, Sekhar
On 23.08.2013 19:09, Sekhar Nori wrote: > On 8/23/2013 10:26 PM, Santosh Shilimkar wrote: >> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc. > > If this could be handled using IP version then the right way would be to > just read the IP version from hardware and use it. No need of DT property. Yes, exactly. The information we need to support the phy interface setting is completely disconnected from the cpsw core itself. Daniel
On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote: > > > On 8/23/2013 10:26 PM, Santosh Shilimkar wrote: >> On Friday 23 August 2013 12:30 PM, Daniel Mack wrote: >>> On 23.08.2013 16:23, Santosh Shilimkar wrote: >>>> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote: >>> >>>>> +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. >>> >>> As I already mentioned in the cover letter and in the commit message, I >>> just don't know which criteria makes most sense here. >>> >>> On a general note, I would say that chances that this exactly IP core >>> with the same version number will appear on some other silicon which >>> doesn't support the control mode register in an AM33xx fashion, is not >>> necessarily negligible. >>> >>> So what that new compatible string denotes is the cpsw in a version as >>> found on am3352 SoCs, which is actually exactly what it does. >>> >>> I don't have a strong opinion here, but see your point. I just don't >>> have a better idea on how to treat that. >>> >> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc. > > If this could be handled using IP version then the right way would be to > just read the IP version from hardware and use it. No need of DT property. > Thats fine as well but I thought the patch needed additional properties like CM reg-address come from DT and hence the separate compatible. If you can manage without that, thats even better. Regards, Santosh
On Friday 23 August 2013 10:26 PM, Santosh Shilimkar wrote: > On Friday 23 August 2013 12:30 PM, Daniel Mack wrote: >> On 23.08.2013 16:23, Santosh Shilimkar wrote: >>> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote: >>>> +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. >> As I already mentioned in the cover letter and in the commit message, I >> just don't know which criteria makes most sense here. >> >> On a general note, I would say that chances that this exactly IP core >> with the same version number will appear on some other silicon which >> doesn't support the control mode register in an AM33xx fashion, is not >> necessarily negligible. >> >> So what that new compatible string denotes is the cpsw in a version as >> found on am3352 SoCs, which is actually exactly what it does. >> >> I don't have a strong opinion here, but see your point. I just don't >> have a better idea on how to treat that. >> > So just stick the IP version or call it cpsw-v1... cpsw-v2 etc. > That way if in future if someone uses those features, they can use > this compatible if they don't they use the one which suites that > SOC. > We cannot map control module register with CPSW IP version as both comes from different design team and CPSW ip version can be same across SoC and gmii sel register definition can be different. Control module defines may vary in different SoC as per SoC requirements. Adding Pekon Gupta who had worked in Silicon team before. Regards Mugunthan V N
On 23.08.2013 19:19, Santosh Shilimkar wrote: > On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote: >> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote: >>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc. >> >> If this could be handled using IP version then the right way would be to >> just read the IP version from hardware and use it. No need of DT property. >> > Thats fine as well but I thought the patch needed additional properties like > CM reg-address come from DT and hence the separate compatible. If you can > manage without that, thats even better. We can't, that's the whole point :) Well, theoretically, we could for now, but that's not a clean solution. Again: the problem here is that the control port is separated from the cpsw core, and so we have to implement something specific for the AM3352 SoC. I know that's a violation of clean and generic driver ideas, but there's no way we can assume that every cpsw v2 ip block has a control port that is compatible to the one found on am335x chips. Daniel
On Friday 23 August 2013 01:24 PM, Daniel Mack wrote: > On 23.08.2013 19:19, Santosh Shilimkar wrote: >> On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote: >>> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote: > >>>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc. >>> >>> If this could be handled using IP version then the right way would be to >>> just read the IP version from hardware and use it. No need of DT property. >>> >> Thats fine as well but I thought the patch needed additional properties like >> CM reg-address come from DT and hence the separate compatible. If you can >> manage without that, thats even better. > > We can't, that's the whole point :) > I saw that from the patch :) > Well, theoretically, we could for now, but that's not a clean solution. > Again: the problem here is that the control port is separated from the > cpsw core, and so we have to implement something specific for the AM3352 > SoC. I know that's a violation of clean and generic driver ideas, but > there's no way we can assume that every cpsw v2 ip block has a control > port that is compatible to the one found on am335x chips. > But there is a possibility that other SOC will just use the same control module approach. So using a revision IP is just fine. BTW, CPSW is not the only driver where control module is used. There are many examples like USB, MMC etc There is nothing wrong in using the version info and associating the additional functionality with it. Regards, Santosh
On 8/23/2013 10:58 PM, Santosh Shilimkar wrote: > On Friday 23 August 2013 01:24 PM, Daniel Mack wrote: >> On 23.08.2013 19:19, Santosh Shilimkar wrote: >>> On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote: >>>> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote: >> >>>>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc. >>>> >>>> If this could be handled using IP version then the right way would be to >>>> just read the IP version from hardware and use it. No need of DT property. >>>> >>> Thats fine as well but I thought the patch needed additional properties like >>> CM reg-address come from DT and hence the separate compatible. If you can >>> manage without that, thats even better. >> >> We can't, that's the whole point :) >> > I saw that from the patch :) > >> Well, theoretically, we could for now, but that's not a clean solution. >> Again: the problem here is that the control port is separated from the >> cpsw core, and so we have to implement something specific for the AM3352 >> SoC. I know that's a violation of clean and generic driver ideas, but >> there's no way we can assume that every cpsw v2 ip block has a control >> port that is compatible to the one found on am335x chips. >> > But there is a possibility that other SOC will just use the same > control module approach. So using a revision IP is just fine. BTW, But this is misleading because it makes appear like the same compatible can be used on on another SoC like DRA7 which probably has the same version of IP but a different control module implementation, when in practice it cannot. The fact is we are doing something SoC specific in the driver and we cannot hide that behind IP versions. If really in practice there comes another SoC with the same control module definition then it can always use ti,am3352-cpsw compatible as well. The compatible name does not preclude its usage. Thanks, Sekhar
On Friday 23 August 2013 01:39 PM, Sekhar Nori wrote: > On 8/23/2013 10:58 PM, Santosh Shilimkar wrote: >> On Friday 23 August 2013 01:24 PM, Daniel Mack wrote: >>> On 23.08.2013 19:19, Santosh Shilimkar wrote: >>>> On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote: >>>>> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote: >>> >>>>>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc. >>>>> >>>>> If this could be handled using IP version then the right way would be to >>>>> just read the IP version from hardware and use it. No need of DT property. >>>>> >>>> Thats fine as well but I thought the patch needed additional properties like >>>> CM reg-address come from DT and hence the separate compatible. If you can >>>> manage without that, thats even better. >>> >>> We can't, that's the whole point :) >>> >> I saw that from the patch :) >> >>> Well, theoretically, we could for now, but that's not a clean solution. >>> Again: the problem here is that the control port is separated from the >>> cpsw core, and so we have to implement something specific for the AM3352 >>> SoC. I know that's a violation of clean and generic driver ideas, but >>> there's no way we can assume that every cpsw v2 ip block has a control >>> port that is compatible to the one found on am335x chips. >>> >> But there is a possibility that other SOC will just use the same >> control module approach. So using a revision IP is just fine. BTW, > > But this is misleading because it makes appear like the same compatible > can be used on on another SoC like DRA7 which probably has the same > version of IP but a different control module implementation, when in > practice it cannot. > > The fact is we are doing something SoC specific in the driver and we > cannot hide that behind IP versions. If really in practice there comes > another SoC with the same control module definition then it can always > use ti,am3352-cpsw compatible as well. The compatible name does not > preclude its usage. > My point was the CPSW needs a feature which is implemented using control module rather than within the IP itself. Its an implementation detail. As such the additional feature makes sense for that IP. O.w there was no need to do any monkeying with control module. E.g MMC card detect is a basic functionality, implemented by various types like control module, PMIC or MMC IP itself. As such the driver need that support and all the implementation details needs to still handled to make that part work. Regards, Santosh
On Friday 23 August 2013 11:40 PM, Santosh Shilimkar wrote: > On Friday 23 August 2013 01:39 PM, Sekhar Nori wrote: >> On 8/23/2013 10:58 PM, Santosh Shilimkar wrote: >>> On Friday 23 August 2013 01:24 PM, Daniel Mack wrote: >>>> On 23.08.2013 19:19, Santosh Shilimkar wrote: >>>>> On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote: >>>>>> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote: >>>>>>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc. >>>>>> If this could be handled using IP version then the right way would be to >>>>>> just read the IP version from hardware and use it. No need of DT property. >>>>>> >>>>> Thats fine as well but I thought the patch needed additional properties like >>>>> CM reg-address come from DT and hence the separate compatible. If you can >>>>> manage without that, thats even better. >>>> We can't, that's the whole point :) >>>> >>> I saw that from the patch :) >>> >>>> Well, theoretically, we could for now, but that's not a clean solution. >>>> Again: the problem here is that the control port is separated from the >>>> cpsw core, and so we have to implement something specific for the AM3352 >>>> SoC. I know that's a violation of clean and generic driver ideas, but >>>> there's no way we can assume that every cpsw v2 ip block has a control >>>> port that is compatible to the one found on am335x chips. >>>> >>> But there is a possibility that other SOC will just use the same >>> control module approach. So using a revision IP is just fine. BTW, >> But this is misleading because it makes appear like the same compatible >> can be used on on another SoC like DRA7 which probably has the same >> version of IP but a different control module implementation, when in >> practice it cannot. >> >> The fact is we are doing something SoC specific in the driver and we >> cannot hide that behind IP versions. If really in practice there comes >> another SoC with the same control module definition then it can always >> use ti,am3352-cpsw compatible as well. The compatible name does not >> preclude its usage. >> > My point was the CPSW needs a feature which is implemented using > control module rather than within the IP itself. Its an implementation > detail. As such the additional feature makes sense for that IP. O.w > there was no need to do any monkeying with control module. > > E.g > MMC card detect is a basic functionality, implemented by various types > like control module, PMIC or MMC IP itself. As such the driver need > that support and all the implementation details needs to still handled > to make that part work. > > CPSW core as such understands only GMII/MII signals, there is an additional module which converts GMII/MII signals to RGMII/RMII signals respectively which is called as CPRGMII/CPRMII as specified in the AM335x TRM in Figure 14-1. Ethernet Switch Integration. So to control this sub-module, the control register is used and this has to be configured according to the EVM design like what mode of phy is connected. CPRGMII and CPRMII is no way related to CPSW core. Regards Mugunthan V N
On Friday 23 August 2013 02:29 PM, Mugunthan V N wrote: > On Friday 23 August 2013 11:40 PM, Santosh Shilimkar wrote: >> On Friday 23 August 2013 01:39 PM, Sekhar Nori wrote: >>> On 8/23/2013 10:58 PM, Santosh Shilimkar wrote: >>>> On Friday 23 August 2013 01:24 PM, Daniel Mack wrote: >>>>> On 23.08.2013 19:19, Santosh Shilimkar wrote: >>>>>> On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote: >>>>>>> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote: >>>>>>>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc. >>>>>>> If this could be handled using IP version then the right way would be to >>>>>>> just read the IP version from hardware and use it. No need of DT property. >>>>>>> >>>>>> Thats fine as well but I thought the patch needed additional properties like >>>>>> CM reg-address come from DT and hence the separate compatible. If you can >>>>>> manage without that, thats even better. >>>>> We can't, that's the whole point :) >>>>> >>>> I saw that from the patch :) >>>> >>>>> Well, theoretically, we could for now, but that's not a clean solution. >>>>> Again: the problem here is that the control port is separated from the >>>>> cpsw core, and so we have to implement something specific for the AM3352 >>>>> SoC. I know that's a violation of clean and generic driver ideas, but >>>>> there's no way we can assume that every cpsw v2 ip block has a control >>>>> port that is compatible to the one found on am335x chips. >>>>> >>>> But there is a possibility that other SOC will just use the same >>>> control module approach. So using a revision IP is just fine. BTW, >>> But this is misleading because it makes appear like the same compatible >>> can be used on on another SoC like DRA7 which probably has the same >>> version of IP but a different control module implementation, when in >>> practice it cannot. >>> >>> The fact is we are doing something SoC specific in the driver and we >>> cannot hide that behind IP versions. If really in practice there comes >>> another SoC with the same control module definition then it can always >>> use ti,am3352-cpsw compatible as well. The compatible name does not >>> preclude its usage. >>> >> My point was the CPSW needs a feature which is implemented using >> control module rather than within the IP itself. Its an implementation >> detail. As such the additional feature makes sense for that IP. O.w >> there was no need to do any monkeying with control module. >> >> E.g >> MMC card detect is a basic functionality, implemented by various types >> like control module, PMIC or MMC IP itself. As such the driver need >> that support and all the implementation details needs to still handled >> to make that part work. >> >> > > CPSW core as such understands only GMII/MII signals, there is an > additional module which converts GMII/MII signals to RGMII/RMII signals > respectively which is called as CPRGMII/CPRMII as specified in the > AM335x TRM in Figure 14-1. Ethernet Switch Integration. > > So to control this sub-module, the control register is used and this has > to be configured according to the EVM design like what mode of phy is > connected. CPRGMII and CPRMII is no way related to CPSW core. > Ok then why are you polluting cpsw driver with that code which not realted to CPSW as you said above. You are contradicting what you said by supporting the SOC usage in the core CPSW driver. Regards, Santosh
> > On Friday 23 August 2013 10:26 PM, Santosh Shilimkar wrote: > > On Friday 23 August 2013 12:30 PM, Daniel Mack wrote: > >> On 23.08.2013 16:23, Santosh Shilimkar wrote: > >>> On Friday 23 August 2013 10:16 AM, Daniel Mack wrote: > >>>> +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. > >> As I already mentioned in the cover letter and in the commit message, I > >> just don't know which criteria makes most sense here. > >> > >> On a general note, I would say that chances that this exactly IP core > >> with the same version number will appear on some other silicon which > >> doesn't support the control mode register in an AM33xx fashion, is not > >> necessarily negligible. > >> > >> So what that new compatible string denotes is the cpsw in a version as > >> found on am3352 SoCs, which is actually exactly what it does. > >> > >> I don't have a strong opinion here, but see your point. I just don't > >> have a better idea on how to treat that. > >> > > So just stick the IP version or call it cpsw-v1... cpsw-v2 etc. > > That way if in future if someone uses those features, they can use > > this compatible if they don't they use the one which suites that > > SOC. > > > We cannot map control module register with CPSW IP version as both comes > from different design team and CPSW ip version can be same across SoC > and gmii sel register definition can be different. Control module > defines may vary in different SoC as per SoC requirements. > > Adding Pekon Gupta who had worked in Silicon team before. > [Pekon]: My opinion here.. Uniformity in control module cannot be guaranteed, neither in register offsets nor their functionality. Uniformity is usually maintained till the point same person is writing the spec, or it’s a derivative device. Control-module should not be categorized as IP, instead it’s a group of miscellaneous logic usually consisting of following: (a) SoC bug-fixes across silicon revisions. (b) SoC specific logic like device_type, JTAG-ID. (c) IP bug fixes which could not be accommodated in IP address-map. (d) IP logic which depends on SoC configurations. Due to this un-deterministic composition of control-module, having a dedicated driver for control module might not work either, as it has to be updated | re-written for every new device. So, Following can be used as guideline to determine compatibility string for DT bindings.. - If binding maps to (a) and (c), .i.e., bug-fixes for SoC or IP, then its most likely that these would change either in next silicon revision or in next devices. Hence such bindings should use *not* use IP compatibility strings, they _may_ use SoC-name based compatibility string. - Else if binding maps to (b),.i.e., SoC specific configurations. then their offsets and functionality should remain same across the family of devices at-least, so it should use SoC-name based compatibility string. - And if binding maps to (d), .i.e., IP feature but depending on SoC Then it can use IP based compatibility string, along with IP version. (same approach can be used for CPSW binding used here, with regards, pekon
On Saturday 24 August 2013 01:24 AM, Santosh Shilimkar wrote: > On Friday 23 August 2013 02:29 PM, Mugunthan V N wrote: >> On Friday 23 August 2013 11:40 PM, Santosh Shilimkar wrote: >>> On Friday 23 August 2013 01:39 PM, Sekhar Nori wrote: >>>> On 8/23/2013 10:58 PM, Santosh Shilimkar wrote: >>>>> On Friday 23 August 2013 01:24 PM, Daniel Mack wrote: >>>>>> On 23.08.2013 19:19, Santosh Shilimkar wrote: >>>>>>> On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote: >>>>>>>> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote: >>>>>>>>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc. >>>>>>>> If this could be handled using IP version then the right way would be to >>>>>>>> just read the IP version from hardware and use it. No need of DT property. >>>>>>>> >>>>>>> Thats fine as well but I thought the patch needed additional properties like >>>>>>> CM reg-address come from DT and hence the separate compatible. If you can >>>>>>> manage without that, thats even better. >>>>>> We can't, that's the whole point :) >>>>>> >>>>> I saw that from the patch :) >>>>> >>>>>> Well, theoretically, we could for now, but that's not a clean solution. >>>>>> Again: the problem here is that the control port is separated from the >>>>>> cpsw core, and so we have to implement something specific for the AM3352 >>>>>> SoC. I know that's a violation of clean and generic driver ideas, but >>>>>> there's no way we can assume that every cpsw v2 ip block has a control >>>>>> port that is compatible to the one found on am335x chips. >>>>>> >>>>> But there is a possibility that other SOC will just use the same >>>>> control module approach. So using a revision IP is just fine. BTW, >>>> But this is misleading because it makes appear like the same compatible >>>> can be used on on another SoC like DRA7 which probably has the same >>>> version of IP but a different control module implementation, when in >>>> practice it cannot. >>>> >>>> The fact is we are doing something SoC specific in the driver and we >>>> cannot hide that behind IP versions. If really in practice there comes >>>> another SoC with the same control module definition then it can always >>>> use ti,am3352-cpsw compatible as well. The compatible name does not >>>> preclude its usage. >>>> >>> My point was the CPSW needs a feature which is implemented using >>> control module rather than within the IP itself. Its an implementation >>> detail. As such the additional feature makes sense for that IP. O.w >>> there was no need to do any monkeying with control module. >>> >>> E.g >>> MMC card detect is a basic functionality, implemented by various types >>> like control module, PMIC or MMC IP itself. As such the driver need >>> that support and all the implementation details needs to still handled >>> to make that part work. >>> >>> >> CPSW core as such understands only GMII/MII signals, there is an >> additional module which converts GMII/MII signals to RGMII/RMII signals >> respectively which is called as CPRGMII/CPRMII as specified in the >> AM335x TRM in Figure 14-1. Ethernet Switch Integration. >> >> So to control this sub-module, the control register is used and this has >> to be configured according to the EVM design like what mode of phy is >> connected. CPRGMII and CPRMII is no way related to CPSW core. >> > Ok then why are you polluting cpsw driver with that code which > not realted to CPSW as you said above. You are contradicting what > you said by supporting the SOC usage in the core CPSW driver. This patch series is not from me and because of the reason I mentioned about control module driver, so that cpsw driver can make use control module apis to select phy mode and control module driver takes care of SoC specific register offsets and definitions, but now it is not possible as it is not acceptable in mainline. So other way is to keep these in driver itself as it is done in this patch series with SoC compatibilities. Regards Mugunthan V N
On Monday 26 August 2013 11:29 AM, Mugunthan V N wrote: > On Saturday 24 August 2013 01:24 AM, Santosh Shilimkar wrote: >> On Friday 23 August 2013 02:29 PM, Mugunthan V N wrote: >>> On Friday 23 August 2013 11:40 PM, Santosh Shilimkar wrote: >>>> On Friday 23 August 2013 01:39 PM, Sekhar Nori wrote: >>>>> On 8/23/2013 10:58 PM, Santosh Shilimkar wrote: >>>>>> On Friday 23 August 2013 01:24 PM, Daniel Mack wrote: >>>>>>> On 23.08.2013 19:19, Santosh Shilimkar wrote: >>>>>>>> On Friday 23 August 2013 01:09 PM, Sekhar Nori wrote: >>>>>>>>> On 8/23/2013 10:26 PM, Santosh Shilimkar wrote: >>>>>>>>>> So just stick the IP version or call it cpsw-v1... cpsw-v2 etc. >>>>>>>>> If this could be handled using IP version then the right way would be to >>>>>>>>> just read the IP version from hardware and use it. No need of DT property. >>>>>>>>> >>>>>>>> Thats fine as well but I thought the patch needed additional properties like >>>>>>>> CM reg-address come from DT and hence the separate compatible. If you can >>>>>>>> manage without that, thats even better. >>>>>>> We can't, that's the whole point :) >>>>>>> >>>>>> I saw that from the patch :) >>>>>> >>>>>>> Well, theoretically, we could for now, but that's not a clean solution. >>>>>>> Again: the problem here is that the control port is separated from the >>>>>>> cpsw core, and so we have to implement something specific for the AM3352 >>>>>>> SoC. I know that's a violation of clean and generic driver ideas, but >>>>>>> there's no way we can assume that every cpsw v2 ip block has a control >>>>>>> port that is compatible to the one found on am335x chips. >>>>>>> >>>>>> But there is a possibility that other SOC will just use the same >>>>>> control module approach. So using a revision IP is just fine. BTW, >>>>> But this is misleading because it makes appear like the same compatible >>>>> can be used on on another SoC like DRA7 which probably has the same >>>>> version of IP but a different control module implementation, when in >>>>> practice it cannot. >>>>> >>>>> The fact is we are doing something SoC specific in the driver and we >>>>> cannot hide that behind IP versions. If really in practice there comes >>>>> another SoC with the same control module definition then it can always >>>>> use ti,am3352-cpsw compatible as well. The compatible name does not >>>>> preclude its usage. >>>>> >>>> My point was the CPSW needs a feature which is implemented using >>>> control module rather than within the IP itself. Its an implementation >>>> detail. As such the additional feature makes sense for that IP. O.w >>>> there was no need to do any monkeying with control module. >>>> >>>> E.g >>>> MMC card detect is a basic functionality, implemented by various types >>>> like control module, PMIC or MMC IP itself. As such the driver need >>>> that support and all the implementation details needs to still handled >>>> to make that part work. >>>> >>>> >>> CPSW core as such understands only GMII/MII signals, there is an >>> additional module which converts GMII/MII signals to RGMII/RMII signals >>> respectively which is called as CPRGMII/CPRMII as specified in the >>> AM335x TRM in Figure 14-1. Ethernet Switch Integration. >>> >>> So to control this sub-module, the control register is used and this has >>> to be configured according to the EVM design like what mode of phy is >>> connected. CPRGMII and CPRMII is no way related to CPSW core. >>> >> Ok then why are you polluting cpsw driver with that code which >> not realted to CPSW as you said above. You are contradicting what >> you said by supporting the SOC usage in the core CPSW driver. > This patch series is not from me and because of the reason I mentioned > about control module driver, so that cpsw driver can make use control > module apis to select phy mode and control module driver takes care of > SoC specific register offsets and definitions, but now it is not > possible as it is not acceptable in mainline. So other way is to keep > these in driver itself as it is done in this patch series with SoC > compatibilities. What is done in this patch is _not_ "SoC compatibilities". SoC compatibility would be what was done in v1 of this patch ie, explicit check for of_machine_is_compatible("ti,am33xx") "ti,am3352-cpsw" says "CPSW as implemented on AM3352". This is not the same as checking if SoC is AM3352. The example quoted on ePAPR spec for a compatible string is: compatible = “fsl,mpc8641-uart”, “ns16550"; MPC8641 is freescale PowerPC based SoC[1]. This shows that it is not unnatural to use SoC names in compatibles for IPs. That, or the ePAPR specification needs to be updated to show the right example of how a compatible could be defined. Until then I see no reason of changing what is implemented in this patch. In short, even if there was no control module handling in the driver, using "ti,am3352-cpsw" would be just fine. I have also CCed the DT maintainers for their opinion. They should have been explicitly CCed anyway. Thanks, Sekhar [1] http://www.freescale.com/files/32bit/doc/data_sheet/MPC8641DEC.pdf
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", + .data = (void *) CPSW_TYPE_AM33XX + }, { + .compatible = "ti,cpsw", + .data = (void *) CPSW_TYPE_GENERIC + }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, cpsw_of_mtable); + static int cpsw_probe_dt(struct cpsw_platform_data *data, struct platform_device *pdev) { struct device_node *node = pdev->dev.of_node; + const struct of_device_id *match; struct device_node *slave_node; + unsigned long match_data; int i = 0, ret; u32 prop; - if (!node) + match = of_match_device(cpsw_of_mtable, &pdev->dev); + + if (!node || !match) return -EINVAL; + match_data = (unsigned long) match->data; + data->hw_type = match_data; + if (of_property_read_u32(node, "slaves", &prop)) { pr_err("Missing slaves property in the DT.\n"); return -EINVAL; @@ -2228,12 +2252,6 @@ static const struct dev_pm_ops cpsw_pm_ops = { .resume = cpsw_resume, }; -static const struct of_device_id cpsw_of_mtable[] = { - { .compatible = "ti,cpsw", }, - { /* sentinel */ }, -}; -MODULE_DEVICE_TABLE(of, cpsw_of_mtable); - static struct platform_driver cpsw_driver = { .driver = { .name = "cpsw", diff --git a/drivers/net/ethernet/ti/cpsw.h b/drivers/net/ethernet/ti/cpsw.h index eb3e101..96c374a 100644 --- a/drivers/net/ethernet/ti/cpsw.h +++ b/drivers/net/ethernet/ti/cpsw.h @@ -37,6 +37,7 @@ struct cpsw_platform_data { u32 mac_control; /* Mac control register */ u16 default_vlan; /* Def VLAN for ALE lookup in VLAN aware mode*/ bool dual_emac; /* Enable Dual EMAC mode */ + u32 hw_type; /* hardware type as specified in 'compatible' */ }; #endif /* __CPSW_H__ */
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(-)