Re: [PATCH v2] net: cpsw: fix obtaining mac address for am3517

Re: [PATCH v2] net: cpsw: fix obtaining mac address for am3517

[Tony Lindgren]

* Jeroen Hofstee <jhofstee@victronenergy.com> [161028 08:33]:
> Commit b6745f6e4e63 ("drivers: net: cpsw: davinci_emac: move reading mac
> id to common file") did not only move the code for an am3517, it also
> added the slave parameter, resulting in an invalid (all zero) mac address
> being returned for an am3517, since it only has a single emac and the slave
> parameter is pointing to the second. So simply always read the first and
> valid mac-address for a ti,am3517-emac.

And others davinci_emac.c users can have more than one. So is the
reason the slave parameter points to the second instance because
of the location in the hardware?

Regards,

Tony

Re: [PATCH v2] net: cpsw: fix obtaining mac address for am3517

[Jeroen Hofstee]

Hello Tony,

On 28-10-16 17:52, Tony Lindgren wrote:
> * Jeroen Hofstee <jhofstee@victronenergy.com> [161028 08:33]:
>> Commit b6745f6e4e63 ("drivers: net: cpsw: davinci_emac: move reading mac
>> id to common file") did not only move the code for an am3517, it also
>> added the slave parameter, resulting in an invalid (all zero) mac address
>> being returned for an am3517, since it only has a single emac and the slave
>> parameter is pointing to the second. So simply always read the first and
>> valid mac-address for a ti,am3517-emac.
> And others davinci_emac.c users can have more than one. So is the
> reason the slave parameter points to the second instance because
> of the location in the hardware?

Sort of, the slave parameter gets determined by the fact if there is one
or two register range(s) associated with the davinci_emac. In davinci_emac.c

     res_ctrl = platform_get_resource(pdev, IORESOURCE_MEM, 1);
     ...
     rc = davinci_emac_try_get_mac(pdev, res_ctrl ? 0 : 1,
                           priv->mac_addr);

So it there are two ranges, the slave param becomes 0. It there is only 
one, it
will be 1. Since the am3517 only has a single regs entry it ends up with 
slave 1,
while there is only a single davinci_emac.

Regards,
Jeroen

arch/arm/boot/dts/dm816x.dtsi
-----------------------------
eth0: ethernet@4a100000 {
     compatible = "ti,dm816-emac";
     ti,hwmods = "emac0";
     reg = <0x4a100000 0x800
            0x4a100900 0x3700>;
     clocks = <&sysclk24_ck>;
     syscon = <&scm_conf>;
     ti,davinci-ctrl-reg-offset = <0>;
     ti,davinci-ctrl-mod-reg-offset = <0x900>;
     ti,davinci-ctrl-ram-offset = <0x2000>;
     ti,davinci-ctrl-ram-size = <0x2000>;
     interrupts = <40 41 42 43>;
     phy-handle = <&phy0>;
};

eth1: ethernet@4a120000 {
     compatible = "ti,dm816-emac";
     ti,hwmods = "emac1";
     reg = <0x4a120000 0x4000>;
     clocks = <&sysclk24_ck>;
     syscon = <&scm_conf>;
     ti,davinci-ctrl-reg-offset = <0>;
     ti,davinci-ctrl-mod-reg-offset = <0x900>;
     ti,davinci-ctrl-ram-offset = <0x2000>;
     ti,davinci-ctrl-ram-size = <0x2000>;
     interrupts = <44 45 46 47>;
     phy-handle = <&phy1>;
};

arch/arm/boot/dts/am3517.dtsi
-------------------------------
davinci_emac: ethernet@0x5c000000 {
     compatible = "ti,am3517-emac";
     ti,hwmods = "davinci_emac";
     status = "disabled";
     reg = <0x5c000000 0x30000>;
     interrupts = <67 68 69 70>;
     syscon = <&scm_conf>;
     ti,davinci-ctrl-reg-offset = <0x10000>;
     ti,davinci-ctrl-mod-reg-offset = <0>;
     ti,davinci-ctrl-ram-offset = <0x20000>;
     ti,davinci-ctrl-ram-size = <0x2000>;
     ti,davinci-rmii-en = /bits/ 8 <1>;
     local-mac-address = [ 00 00 00 00 00 00 ];
};

Re: [PATCH v2] net: cpsw: fix obtaining mac address for am3517

[Tony Lindgren]

* Jeroen Hofstee <jhofstee@victronenergy.com> [161028 11:19]:
> Hello Tony,
> 
> On 28-10-16 17:52, Tony Lindgren wrote:
> > * Jeroen Hofstee <jhofstee@victronenergy.com> [161028 08:33]:
> > > Commit b6745f6e4e63 ("drivers: net: cpsw: davinci_emac: move reading mac
> > > id to common file") did not only move the code for an am3517, it also
> > > added the slave parameter, resulting in an invalid (all zero) mac address
> > > being returned for an am3517, since it only has a single emac and the slave
> > > parameter is pointing to the second. So simply always read the first and
> > > valid mac-address for a ti,am3517-emac.
> > And others davinci_emac.c users can have more than one. So is the
> > reason the slave parameter points to the second instance because
> > of the location in the hardware?
> 
> Sort of, the slave parameter gets determined by the fact if there is one
> or two register range(s) associated with the davinci_emac. In davinci_emac.c
> 
>     res_ctrl = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>     ...
>     rc = davinci_emac_try_get_mac(pdev, res_ctrl ? 0 : 1,
>                           priv->mac_addr);
> 
> So it there are two ranges, the slave param becomes 0. It there is only one,
> it
> will be 1. Since the am3517 only has a single regs entry it ends up with
> slave 1,
> while there is only a single davinci_emac.

OK thanks for clarifying it:

Acked-by: Tony Lindgren <tony@atomide.com>

Re: [PATCH v2] net: cpsw: fix obtaining mac address for am3517

[Måns Rullgård]

Tony Lindgren <tony@atomide.com> writes:

> * Jeroen Hofstee <jhofstee@victronenergy.com> [161028 11:19]:
>> Hello Tony,
>> 
>> On 28-10-16 17:52, Tony Lindgren wrote:
>> > * Jeroen Hofstee <jhofstee@victronenergy.com> [161028 08:33]:
>> > > Commit b6745f6e4e63 ("drivers: net: cpsw: davinci_emac: move reading mac
>> > > id to common file") did not only move the code for an am3517, it also
>> > > added the slave parameter, resulting in an invalid (all zero) mac address
>> > > being returned for an am3517, since it only has a single emac and the slave
>> > > parameter is pointing to the second. So simply always read the first and
>> > > valid mac-address for a ti,am3517-emac.
>> > And others davinci_emac.c users can have more than one. So is the
>> > reason the slave parameter points to the second instance because
>> > of the location in the hardware?
>> 
>> Sort of, the slave parameter gets determined by the fact if there is one
>> or two register range(s) associated with the davinci_emac. In davinci_emac.c
>> 
>>     res_ctrl = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>     ...
>>     rc = davinci_emac_try_get_mac(pdev, res_ctrl ? 0 : 1,
>>                           priv->mac_addr);
>> 
>> So it there are two ranges, the slave param becomes 0. It there is only one,
>> it
>> will be 1. Since the am3517 only has a single regs entry it ends up with
>> slave 1,
>> while there is only a single davinci_emac.
>
> OK thanks for clarifying it:
>
> Acked-by: Tony Lindgren <tony@atomide.com>

What happened to this patch?

-- 
Måns Rullgård

Re: [PATCH v2] net: cpsw: fix obtaining mac address for am3517

[Måns Rullgård]

Tony Lindgren <tony@atomide.com> writes:

> * Jeroen Hofstee <jhofstee@victronenergy.com> [161028 11:19]:
>> Hello Tony,
>> 
>> On 28-10-16 17:52, Tony Lindgren wrote:
>> > * Jeroen Hofstee <jhofstee@victronenergy.com> [161028 08:33]:
>> > > Commit b6745f6e4e63 ("drivers: net: cpsw: davinci_emac: move reading mac
>> > > id to common file") did not only move the code for an am3517, it also
>> > > added the slave parameter, resulting in an invalid (all zero) mac address
>> > > being returned for an am3517, since it only has a single emac and the slave
>> > > parameter is pointing to the second. So simply always read the first and
>> > > valid mac-address for a ti,am3517-emac.
>> > And others davinci_emac.c users can have more than one. So is the
>> > reason the slave parameter points to the second instance because
>> > of the location in the hardware?
>> 
>> Sort of, the slave parameter gets determined by the fact if there is one
>> or two register range(s) associated with the davinci_emac. In davinci_emac.c
>> 
>>     res_ctrl = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>     ...
>>     rc = davinci_emac_try_get_mac(pdev, res_ctrl ? 0 : 1,
>>                           priv->mac_addr);
>> 
>> So it there are two ranges, the slave param becomes 0. It there is only one,
>> it
>> will be 1. Since the am3517 only has a single regs entry it ends up with
>> slave 1,
>> while there is only a single davinci_emac.
>
> OK thanks for clarifying it:
>
> Acked-by: Tony Lindgren <tony@atomide.com>

Is there some reason this patch was never picked up, or was it simply
forgotten?

-- 
Måns Rullgård

Re: [PATCH v2] net: cpsw: fix obtaining mac address for am3517

[Simon Horman]

On Fri, Jun 23, 2023 at 03:58:03PM +0100, Måns Rullgård wrote:
> Tony Lindgren <tony@atomide.com> writes:
> 
> > * Jeroen Hofstee <jhofstee@victronenergy.com> [161028 11:19]:
> >> Hello Tony,
> >> 
> >> On 28-10-16 17:52, Tony Lindgren wrote:
> >> > * Jeroen Hofstee <jhofstee@victronenergy.com> [161028 08:33]:
> >> > > Commit b6745f6e4e63 ("drivers: net: cpsw: davinci_emac: move reading mac
> >> > > id to common file") did not only move the code for an am3517, it also
> >> > > added the slave parameter, resulting in an invalid (all zero) mac address
> >> > > being returned for an am3517, since it only has a single emac and the slave
> >> > > parameter is pointing to the second. So simply always read the first and
> >> > > valid mac-address for a ti,am3517-emac.
> >> > And others davinci_emac.c users can have more than one. So is the
> >> > reason the slave parameter points to the second instance because
> >> > of the location in the hardware?
> >> 
> >> Sort of, the slave parameter gets determined by the fact if there is one
> >> or two register range(s) associated with the davinci_emac. In davinci_emac.c
> >> 
> >>     res_ctrl = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> >>     ...
> >>     rc = davinci_emac_try_get_mac(pdev, res_ctrl ? 0 : 1,
> >>                           priv->mac_addr);
> >> 
> >> So it there are two ranges, the slave param becomes 0. It there is only one,
> >> it
> >> will be 1. Since the am3517 only has a single regs entry it ends up with
> >> slave 1,
> >> while there is only a single davinci_emac.
> >
> > OK thanks for clarifying it:
> >
> > Acked-by: Tony Lindgren <tony@atomide.com>
> 
> Is there some reason this patch was never picked up, or was it simply
> forgotten?

I feel like I am missing something here.

The patch possibly dates back to 2016 - but I can't tell because
lore.kernel.org doesn't know either.

https://lore.kernel.org/netdev/20161028155213.2t3nwwe3lqaynaer@atomide.com/

And I see you asked almost the same question in 2019.

If it is still relevant, perhaps it would be good to repost it
for review.

Re: [PATCH v2] net: cpsw: fix obtaining mac address for am3517

[Jeroen Hofstee]

On 23-06-2023 22:14, Simon Horman wrote:
> On Fri, Jun 23, 2023 at 03:58:03PM +0100, Måns Rullgård wrote:
>> Tony Lindgren <tony@atomide.com> writes:
>>
>>> * Jeroen Hofstee <jhofstee@victronenergy.com> [161028 11:19]:
>>>> Hello Tony,
>>>>
>>>> On 28-10-16 17:52, Tony Lindgren wrote:
>>>>> * Jeroen Hofstee <jhofstee@victronenergy.com> [161028 08:33]:
>>>>>> Commit b6745f6e4e63 ("drivers: net: cpsw: davinci_emac: move reading mac
>>>>>> id to common file") did not only move the code for an am3517, it also
>>>>>> added the slave parameter, resulting in an invalid (all zero) mac address
>>>>>> being returned for an am3517, since it only has a single emac and the slave
>>>>>> parameter is pointing to the second. So simply always read the first and
>>>>>> valid mac-address for a ti,am3517-emac.
>>>>> And others davinci_emac.c users can have more than one. So is the
>>>>> reason the slave parameter points to the second instance because
>>>>> of the location in the hardware?
>>>> Sort of, the slave parameter gets determined by the fact if there is one
>>>> or two register range(s) associated with the davinci_emac. In davinci_emac.c
>>>>
>>>>      res_ctrl = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>>>      ...
>>>>      rc = davinci_emac_try_get_mac(pdev, res_ctrl ? 0 : 1,
>>>>                            priv->mac_addr);
>>>>
>>>> So it there are two ranges, the slave param becomes 0. It there is only one,
>>>> it
>>>> will be 1. Since the am3517 only has a single regs entry it ends up with
>>>> slave 1,
>>>> while there is only a single davinci_emac.
>>> OK thanks for clarifying it:
>>>
>>> Acked-by: Tony Lindgren <tony@atomide.com>
>> Is there some reason this patch was never picked up, or was it simply
>> forgotten?
> I feel like I am missing something here.

That is a weird response, you feel like something is missing, while
it is a recognized bug, acknowledged by the maintainer at that time
and apparently still not fixed and Måns simply asked it there is a
reason for that.

This is not a really helpful response,

Jeroen

Re: [PATCH v2] net: cpsw: fix obtaining mac address for am3517

[Andrew Lunn]

> > I feel like I am missing something here.
> 
> That is a weird response, you feel like something is missing

There is. The patch.

Maintainers have a slightly better memory than a goldfish, but given
the high volume of patches, we don't remember threads from 2016. Also,
all our infrastructure has limited memory, this patch is not in lore,
and it is not in patchworks. So in terms of getting merged, it does
not exist.

We do however recommend that if a patch has not been merged within 2
weeks, it is rebased, any Acked-by: etc tags are added and the patch
reposted.

	Andrew

Re: [PATCH v2] net: cpsw: fix obtaining mac address for am3517

[Simon Horman]

On Fri, Jun 23, 2023 at 11:41:10PM +0200, Andrew Lunn wrote:
> > > I feel like I am missing something here.
> > 
> > That is a weird response, you feel like something is missing
> 
> There is. The patch.
> 
> Maintainers have a slightly better memory than a goldfish, but given
> the high volume of patches, we don't remember threads from 2016. Also,
> all our infrastructure has limited memory, this patch is not in lore,
> and it is not in patchworks. So in terms of getting merged, it does
> not exist.
> 
> We do however recommend that if a patch has not been merged within 2
> weeks, it is rebased, any Acked-by: etc tags are added and the patch
> reposted.

Thanks Andrew, that is also my position.

A ping for a multi-year old patch is unusual (for me).
I was wondering if there was a back story. I guess not.

Re: [PATCH v2] net: cpsw: fix obtaining mac address for am3517

[Måns Rullgård]

Simon Horman <simon.horman@corigine.com> writes:

> On Fri, Jun 23, 2023 at 11:41:10PM +0200, Andrew Lunn wrote:
>> > > I feel like I am missing something here.
>> > 
>> > That is a weird response, you feel like something is missing
>> 
>> There is. The patch.
>> 
>> Maintainers have a slightly better memory than a goldfish, but given
>> the high volume of patches, we don't remember threads from 2016. Also,
>> all our infrastructure has limited memory, this patch is not in lore,
>> and it is not in patchworks. So in terms of getting merged, it does
>> not exist.
>> 
>> We do however recommend that if a patch has not been merged within 2
>> weeks, it is rebased, any Acked-by: etc tags are added and the patch
>> reposted.
>
> Thanks Andrew, that is also my position.
>
> A ping for a multi-year old patch is unusual (for me).
> I was wondering if there was a back story. I guess not.

The only story here is that I was reviewing the set of patches we apply
to our kernels, and I noticed that this one, judging by the discussion,
should have been applied to some tree or other ages ago.

Now if it takes 6 years to get a one-line patch (a fix for a regression,
no less) accepted, I have better things to spend my time on.

-- 
Måns Rullgård

Re: [PATCH v2] net: cpsw: fix obtaining mac address for am3517

[Simon Horman]

On Sat, Jun 24, 2023 at 04:02:07PM +0100, Måns Rullgård wrote:
> Simon Horman <simon.horman@corigine.com> writes:
> 
> > On Fri, Jun 23, 2023 at 11:41:10PM +0200, Andrew Lunn wrote:
> >> > > I feel like I am missing something here.
> >> > 
> >> > That is a weird response, you feel like something is missing
> >> 
> >> There is. The patch.
> >> 
> >> Maintainers have a slightly better memory than a goldfish, but given
> >> the high volume of patches, we don't remember threads from 2016. Also,
> >> all our infrastructure has limited memory, this patch is not in lore,
> >> and it is not in patchworks. So in terms of getting merged, it does
> >> not exist.
> >> 
> >> We do however recommend that if a patch has not been merged within 2
> >> weeks, it is rebased, any Acked-by: etc tags are added and the patch
> >> reposted.
> >
> > Thanks Andrew, that is also my position.
> >
> > A ping for a multi-year old patch is unusual (for me).
> > I was wondering if there was a back story. I guess not.
> 
> The only story here is that I was reviewing the set of patches we apply
> to our kernels, and I noticed that this one, judging by the discussion,
> should have been applied to some tree or other ages ago.
> 
> Now if it takes 6 years to get a one-line patch (a fix for a regression,
> no less) accepted, I have better things to spend my time on.

A long time to be sure. As Andrew explained, the patch is now stale.
It will need to be rebased and reposted in ordered to be considered for
upstream.

Re: [PATCH v2] net: cpsw: fix obtaining mac address for am3517

[Måns Rullgård]

Simon Horman <simon.horman@corigine.com> writes:

> On Sat, Jun 24, 2023 at 04:02:07PM +0100, Måns Rullgård wrote:
>> Simon Horman <simon.horman@corigine.com> writes:
>> 
>> > On Fri, Jun 23, 2023 at 11:41:10PM +0200, Andrew Lunn wrote:
>> >> > > I feel like I am missing something here.
>> >> > 
>> >> > That is a weird response, you feel like something is missing
>> >> 
>> >> There is. The patch.
>> >> 
>> >> Maintainers have a slightly better memory than a goldfish, but given
>> >> the high volume of patches, we don't remember threads from 2016. Also,
>> >> all our infrastructure has limited memory, this patch is not in lore,
>> >> and it is not in patchworks. So in terms of getting merged, it does
>> >> not exist.
>> >> 
>> >> We do however recommend that if a patch has not been merged within 2
>> >> weeks, it is rebased, any Acked-by: etc tags are added and the patch
>> >> reposted.
>> >
>> > Thanks Andrew, that is also my position.
>> >
>> > A ping for a multi-year old patch is unusual (for me).
>> > I was wondering if there was a back story. I guess not.
>> 
>> The only story here is that I was reviewing the set of patches we apply
>> to our kernels, and I noticed that this one, judging by the discussion,
>> should have been applied to some tree or other ages ago.
>> 
>> Now if it takes 6 years to get a one-line patch (a fix for a regression,
>> no less) accepted, I have better things to spend my time on.
>
> A long time to be sure. As Andrew explained, the patch is now stale.
> It will need to be rebased and reposted in ordered to be considered for
> upstream.

I already did that, only to get more snark in return.

-- 
Måns Rullgård