diff mbox series

[net-next,v2] net: ethernet: ti: davinci_mdio: Update K3 SoCs list for errata i2329

Message ID 20231020111738.14671-1-r-gunasekaran@ti.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next,v2] net: ethernet: ti: davinci_mdio: Update K3 SoCs list for errata i2329 | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1363 this patch: 1363
netdev/cc_maintainers warning 4 maintainers not CCed: grygorii.strashko@ti.com robh@kernel.org bhupesh.sharma@linaro.org u.kleine-koenig@pengutronix.de
netdev/build_clang success Errors and warnings before: 1388 this patch: 1388
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1388 this patch: 1388
netdev/checkpatch warning WARNING: Unknown link reference 'AM62x:', use 'Link:' or 'Closes:' instead WARNING: Unknown link reference 'AM64X:', use 'Link:' or 'Closes:' instead WARNING: Unknown link reference 'AM65X:', use 'Link:' or 'Closes:' instead WARNING: Unknown link reference 'J7200:', use 'Link:' or 'Closes:' instead WARNING: Unknown link reference 'J721E:', use 'Link:' or 'Closes:' instead WARNING: Unknown link reference 'J721S2:', use 'Link:' or 'Closes:' instead
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Ravi Gunasekaran Oct. 20, 2023, 11:17 a.m. UTC
The errata i2329 affects certain K3 SoC versions. The k3-socinfo.c
driver generates the revision string for different variants of the
same SoC in an incremental fashion. This is not true for all SoCs.
An example case being J721E, for which the actual silicon revision
names are 1.0, 1.1 for its variants, while the k3-socinfo.c driver
interprets these variants as revisions 1.0, 2.0 respectively,
which is incorrect.

While the work to fixup the silicon revision string is posted
to the soc tree, this patch serves as a fail-safe step by maintaining
a list of correct and incorrect revision strings, so that the fixup
work does not break the errata workaround for such corrected SoCs.

The silicon revisions affected by the errata i2329 can be found under
the MDIO module in the "Advisories by Modules" section of each
SoC errata document listed below

AM62x: https://www.ti.com/lit/er/sprz487c/sprz487c.pdf
AM64X: https://www.ti.com/lit/er/sprz457g/sprz457g.pdf
AM65X: https://www.ti.com/lit/er/sprz452i/sprz452i.pdf
J7200: https://www.ti.com/lit/er/sprz491d/sprz491d.pdf
J721E: https://www.ti.com/lit/er/sprz455d/sprz455d.pdf
J721S2: https://www.ti.com/lit/er/sprz530b/sprz530b.pdf

Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
---

Changes since v1:
* For J721E, retained the incorrect SR ID and added the correct one
* Add AM65x SR2.1 to the workaround list

v1: https://lore.kernel.org/all/20231018140009.1725-1-r-gunasekaran@ti.com/

 drivers/net/ethernet/ti/davinci_mdio.c | 2 ++
 1 file changed, 2 insertions(+)


base-commit: 2030579113a1b1b5bfd7ff24c0852847836d8fd1

Comments

Nishanth Menon Oct. 20, 2023, 12:23 p.m. UTC | #1
On 16:47-20231020, Ravi Gunasekaran wrote:
> The errata i2329 affects certain K3 SoC versions. The k3-socinfo.c
> driver generates the revision string for different variants of the
> same SoC in an incremental fashion. This is not true for all SoCs.
> An example case being J721E, for which the actual silicon revision
> names are 1.0, 1.1 for its variants, while the k3-socinfo.c driver
> interprets these variants as revisions 1.0, 2.0 respectively,
> which is incorrect.
> 
> While the work to fixup the silicon revision string is posted
> to the soc tree, this patch serves as a fail-safe step by maintaining
> a list of correct and incorrect revision strings, so that the fixup
> work does not break the errata workaround for such corrected SoCs.
> 
> The silicon revisions affected by the errata i2329 can be found under
> the MDIO module in the "Advisories by Modules" section of each
> SoC errata document listed below
> 
> AM62x: https://www.ti.com/lit/er/sprz487c/sprz487c.pdf
> AM64X: https://www.ti.com/lit/er/sprz457g/sprz457g.pdf
> AM65X: https://www.ti.com/lit/er/sprz452i/sprz452i.pdf
> J7200: https://www.ti.com/lit/er/sprz491d/sprz491d.pdf
> J721E: https://www.ti.com/lit/er/sprz455d/sprz455d.pdf
> J721S2: https://www.ti.com/lit/er/sprz530b/sprz530b.pdf
> 
> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
> ---
> 
> Changes since v1:
> * For J721E, retained the incorrect SR ID and added the correct one
> * Add AM65x SR2.1 to the workaround list
> 
> v1: https://lore.kernel.org/all/20231018140009.1725-1-r-gunasekaran@ti.com/
> 
>  drivers/net/ethernet/ti/davinci_mdio.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c
> index 628c87dc1d28..25aaef502edc 100644
> --- a/drivers/net/ethernet/ti/davinci_mdio.c
> +++ b/drivers/net/ethernet/ti/davinci_mdio.c
> @@ -516,9 +516,11 @@ static const struct soc_device_attribute k3_mdio_socinfo[] = {
>  	{ .family = "AM64X", .revision = "SR2.0", .data = &am65_mdio_soc_data },
>  	{ .family = "AM65X", .revision = "SR1.0", .data = &am65_mdio_soc_data },
>  	{ .family = "AM65X", .revision = "SR2.0", .data = &am65_mdio_soc_data },
> +	{ .family = "AM65X", .revision = "SR2.1", .data = &am65_mdio_soc_data },
>  	{ .family = "J7200", .revision = "SR1.0", .data = &am65_mdio_soc_data },
>  	{ .family = "J7200", .revision = "SR2.0", .data = &am65_mdio_soc_data },
>  	{ .family = "J721E", .revision = "SR1.0", .data = &am65_mdio_soc_data },
> +	{ .family = "J721E", .revision = "SR1.1", .data = &am65_mdio_soc_data },
>  	{ .family = "J721E", .revision = "SR2.0", .data = &am65_mdio_soc_data },
>  	{ .family = "J721S2", .revision = "SR1.0", .data = &am65_mdio_soc_data},
>  	{ /* sentinel */ },
> 

Looks like every device is impacted -> so, why not just flip the
logic to indicate devices that are NOT impacted? is'nt that a smaller
list?
Ravi Gunasekaran Oct. 20, 2023, 6:05 p.m. UTC | #2
On 10/20/2023 5:53 PM, Nishanth Menon wrote:
> On 16:47-20231020, Ravi Gunasekaran wrote:
>> The errata i2329 affects certain K3 SoC versions. The k3-socinfo.c
>> driver generates the revision string for different variants of the
>> same SoC in an incremental fashion. This is not true for all SoCs.
>> An example case being J721E, for which the actual silicon revision
>> names are 1.0, 1.1 for its variants, while the k3-socinfo.c driver
>> interprets these variants as revisions 1.0, 2.0 respectively,
>> which is incorrect.
>>
>> While the work to fixup the silicon revision string is posted
>> to the soc tree, this patch serves as a fail-safe step by maintaining
>> a list of correct and incorrect revision strings, so that the fixup
>> work does not break the errata workaround for such corrected SoCs.
>>
>> The silicon revisions affected by the errata i2329 can be found under
>> the MDIO module in the "Advisories by Modules" section of each
>> SoC errata document listed below
>>
>> AM62x: https://www.ti.com/lit/er/sprz487c/sprz487c.pdf
>> AM64X: https://www.ti.com/lit/er/sprz457g/sprz457g.pdf
>> AM65X: https://www.ti.com/lit/er/sprz452i/sprz452i.pdf
>> J7200: https://www.ti.com/lit/er/sprz491d/sprz491d.pdf
>> J721E: https://www.ti.com/lit/er/sprz455d/sprz455d.pdf
>> J721S2: https://www.ti.com/lit/er/sprz530b/sprz530b.pdf
>>
>> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
>> ---
>>
>> Changes since v1:
>> * For J721E, retained the incorrect SR ID and added the correct one
>> * Add AM65x SR2.1 to the workaround list
>>
>> v1: https://lore.kernel.org/all/20231018140009.1725-1-r-gunasekaran@ti.com/
>>
>>  drivers/net/ethernet/ti/davinci_mdio.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c
>> index 628c87dc1d28..25aaef502edc 100644
>> --- a/drivers/net/ethernet/ti/davinci_mdio.c
>> +++ b/drivers/net/ethernet/ti/davinci_mdio.c
>> @@ -516,9 +516,11 @@ static const struct soc_device_attribute k3_mdio_socinfo[] = {
>>  	{ .family = "AM64X", .revision = "SR2.0", .data = &am65_mdio_soc_data },
>>  	{ .family = "AM65X", .revision = "SR1.0", .data = &am65_mdio_soc_data },
>>  	{ .family = "AM65X", .revision = "SR2.0", .data = &am65_mdio_soc_data },
>> +	{ .family = "AM65X", .revision = "SR2.1", .data = &am65_mdio_soc_data },
>>  	{ .family = "J7200", .revision = "SR1.0", .data = &am65_mdio_soc_data },
>>  	{ .family = "J7200", .revision = "SR2.0", .data = &am65_mdio_soc_data },
>>  	{ .family = "J721E", .revision = "SR1.0", .data = &am65_mdio_soc_data },
>> +	{ .family = "J721E", .revision = "SR1.1", .data = &am65_mdio_soc_data },
>>  	{ .family = "J721E", .revision = "SR2.0", .data = &am65_mdio_soc_data },
>>  	{ .family = "J721S2", .revision = "SR1.0", .data = &am65_mdio_soc_data},
>>  	{ /* sentinel */ },
>>
> Looks like every device is impacted -> so, why not just flip the
> logic to indicate devices that are NOT impacted? is'nt that a smaller
> list?
>

At the moment, the list of unaffected devices is small. But as and when we
introduce more devices,
this list will need update. Also I feel that few years down the line, when
someone looks at the code,
a list of affected devices provides a better context as it is easier to trace it
back to the errata document.

Regards,
Ravi
Nishanth Menon Oct. 20, 2023, 6:12 p.m. UTC | #3
On 23:35-20231020, Ravi Gunasekaran wrote:
> 
> 
> On 10/20/2023 5:53 PM, Nishanth Menon wrote:
> > On 16:47-20231020, Ravi Gunasekaran wrote:
> >> The errata i2329 affects certain K3 SoC versions. The k3-socinfo.c
> >> driver generates the revision string for different variants of the
> >> same SoC in an incremental fashion. This is not true for all SoCs.
> >> An example case being J721E, for which the actual silicon revision
> >> names are 1.0, 1.1 for its variants, while the k3-socinfo.c driver
> >> interprets these variants as revisions 1.0, 2.0 respectively,
> >> which is incorrect.
> >>
> >> While the work to fixup the silicon revision string is posted
> >> to the soc tree, this patch serves as a fail-safe step by maintaining
> >> a list of correct and incorrect revision strings, so that the fixup
> >> work does not break the errata workaround for such corrected SoCs.
> >>
> >> The silicon revisions affected by the errata i2329 can be found under
> >> the MDIO module in the "Advisories by Modules" section of each
> >> SoC errata document listed below
> >>
> >> AM62x: https://www.ti.com/lit/er/sprz487c/sprz487c.pdf
> >> AM64X: https://www.ti.com/lit/er/sprz457g/sprz457g.pdf
> >> AM65X: https://www.ti.com/lit/er/sprz452i/sprz452i.pdf
> >> J7200: https://www.ti.com/lit/er/sprz491d/sprz491d.pdf
> >> J721E: https://www.ti.com/lit/er/sprz455d/sprz455d.pdf
> >> J721S2: https://www.ti.com/lit/er/sprz530b/sprz530b.pdf
> >>
> >> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
> >> ---
> >>
> >> Changes since v1:
> >> * For J721E, retained the incorrect SR ID and added the correct one
> >> * Add AM65x SR2.1 to the workaround list
> >>
> >> v1: https://lore.kernel.org/all/20231018140009.1725-1-r-gunasekaran@ti.com/
> >>
> >>  drivers/net/ethernet/ti/davinci_mdio.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c
> >> index 628c87dc1d28..25aaef502edc 100644
> >> --- a/drivers/net/ethernet/ti/davinci_mdio.c
> >> +++ b/drivers/net/ethernet/ti/davinci_mdio.c
> >> @@ -516,9 +516,11 @@ static const struct soc_device_attribute k3_mdio_socinfo[] = {
> >>  	{ .family = "AM64X", .revision = "SR2.0", .data = &am65_mdio_soc_data },
> >>  	{ .family = "AM65X", .revision = "SR1.0", .data = &am65_mdio_soc_data },
> >>  	{ .family = "AM65X", .revision = "SR2.0", .data = &am65_mdio_soc_data },
> >> +	{ .family = "AM65X", .revision = "SR2.1", .data = &am65_mdio_soc_data },
> >>  	{ .family = "J7200", .revision = "SR1.0", .data = &am65_mdio_soc_data },
> >>  	{ .family = "J7200", .revision = "SR2.0", .data = &am65_mdio_soc_data },
> >>  	{ .family = "J721E", .revision = "SR1.0", .data = &am65_mdio_soc_data },
> >> +	{ .family = "J721E", .revision = "SR1.1", .data = &am65_mdio_soc_data },
> >>  	{ .family = "J721E", .revision = "SR2.0", .data = &am65_mdio_soc_data },
> >>  	{ .family = "J721S2", .revision = "SR1.0", .data = &am65_mdio_soc_data},
> >>  	{ /* sentinel */ },
> >>
> > Looks like every device is impacted -> so, why not just flip the
> > logic to indicate devices that are NOT impacted? is'nt that a smaller
> > list?
> >
> 
> At the moment, the list of unaffected devices is small. But as and when we
> introduce more devices,
> this list will need update. Also I feel that few years down the line, when
> someone looks at the code,
> a list of affected devices provides a better context as it is easier to trace it
> back to the errata document.

Just handle it with a different compatible if needed. There is no loss
of readability as the check is still readable based on soc_data. but
this removes this entire mess of interdependency of merges completely
out. There are still ROM only spins that are happening and as far as I
see this mess just keeps growing. Alternatively, reading some IP level
version register helps detect the fixed versions, uses that (infact
you should probably insist to the design team to update the revision
for the fix for this very purpose) - that way, the ones that may have
been missed could be limited by soc_data management.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c
index 628c87dc1d28..25aaef502edc 100644
--- a/drivers/net/ethernet/ti/davinci_mdio.c
+++ b/drivers/net/ethernet/ti/davinci_mdio.c
@@ -516,9 +516,11 @@  static const struct soc_device_attribute k3_mdio_socinfo[] = {
 	{ .family = "AM64X", .revision = "SR2.0", .data = &am65_mdio_soc_data },
 	{ .family = "AM65X", .revision = "SR1.0", .data = &am65_mdio_soc_data },
 	{ .family = "AM65X", .revision = "SR2.0", .data = &am65_mdio_soc_data },
+	{ .family = "AM65X", .revision = "SR2.1", .data = &am65_mdio_soc_data },
 	{ .family = "J7200", .revision = "SR1.0", .data = &am65_mdio_soc_data },
 	{ .family = "J7200", .revision = "SR2.0", .data = &am65_mdio_soc_data },
 	{ .family = "J721E", .revision = "SR1.0", .data = &am65_mdio_soc_data },
+	{ .family = "J721E", .revision = "SR1.1", .data = &am65_mdio_soc_data },
 	{ .family = "J721E", .revision = "SR2.0", .data = &am65_mdio_soc_data },
 	{ .family = "J721S2", .revision = "SR1.0", .data = &am65_mdio_soc_data},
 	{ /* sentinel */ },