* Jeroen Hofstee <[email protected]> [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
* Jeroen Hofstee <[email protected]> [161028 11:19]:
> Hello Tony,
>
> On 28-10-16 17:52, Tony Lindgren wrote:
> > * Jeroen Hofstee <[email protected]> [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 <[email protected]>
Hello Tony,
On 28-10-16 17:52, Tony Lindgren wrote:
> * Jeroen Hofstee <[email protected]> [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 ];
};
Tony Lindgren <[email protected]> writes:
> * Jeroen Hofstee <[email protected]> [161028 11:19]:
>> Hello Tony,
>>
>> On 28-10-16 17:52, Tony Lindgren wrote:
>> > * Jeroen Hofstee <[email protected]> [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 <[email protected]>
What happened to this patch?
--
M?ns Rullg?rd
Tony Lindgren <[email protected]> writes:
> * Jeroen Hofstee <[email protected]> [161028 11:19]:
>> Hello Tony,
>>
>> On 28-10-16 17:52, Tony Lindgren wrote:
>> > * Jeroen Hofstee <[email protected]> [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 <[email protected]>
Is there some reason this patch was never picked up, or was it simply
forgotten?
--
M?ns Rullg?rd
On Fri, Jun 23, 2023 at 03:58:03PM +0100, Måns Rullgård wrote:
> Tony Lindgren <[email protected]> writes:
>
> > * Jeroen Hofstee <[email protected]> [161028 11:19]:
> >> Hello Tony,
> >>
> >> On 28-10-16 17:52, Tony Lindgren wrote:
> >> > * Jeroen Hofstee <[email protected]> [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 <[email protected]>
>
> 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/[email protected]/
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.
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 <[email protected]> writes:
>>
>>> * Jeroen Hofstee <[email protected]> [161028 11:19]:
>>>> Hello Tony,
>>>>
>>>> On 28-10-16 17:52, Tony Lindgren wrote:
>>>>> * Jeroen Hofstee <[email protected]> [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 <[email protected]>
>> 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
> > 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
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.
Simon Horman <[email protected]> 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
On Sat, Jun 24, 2023 at 04:02:07PM +0100, Måns Rullgård wrote:
> Simon Horman <[email protected]> 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.
Simon Horman <[email protected]> writes:
> On Sat, Jun 24, 2023 at 04:02:07PM +0100, M?ns Rullg?rd wrote:
>> Simon Horman <[email protected]> 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