Message-ID: <aef63f6a367896950f9e61041cfcff4b99bd6c7d.camel@redhat.com>
Subject: Re: wilc1000 MAC address is 00:00:00:00:00:00
From:   Dan Williams <dcbw@redhat.com>
To:     Ajay.Kathat@microchip.com, kvalo@kernel.org, heiko.thiery@gmail.com
Cc:     michael@walle.cc, kuba@kernel.org, Claudiu.Beznea@microchip.com,
        linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
        Amisha.Patel@microchip.com, thaller@redhat.com,
        bgalvani@redhat.com, Sripad.Balwadgi@microchip.com
Date:   Fri, 10 Feb 2023 15:28:14 -0600
In-Reply-To: <2c2e656e-6dad-67d9-8da0-d507804e7df3@microchip.com>
References: <CAEyMn7aV-B4OEhHR4Ad0LM3sKCz1-nDqSb9uZNmRWR-hMZ=z+A@mail.gmail.com>
         <e027bfcf-1977-f2fa-a362-8faed91a19f9@microchip.com>
         <20230209094825.49f59208@kernel.org>
         <51134d12-1b06-6d6f-e798-7dd681a8f3ae@microchip.com>
         <20230209130725.0b04a424@kernel.org>
         <2d548e01b266f7b1ad19a5ea979d00bf@walle.cc>
         <CAEyMn7bpwusVarzHa262maJHf6XTpCW4SL0-o+YH4DGZx94+hw@mail.gmail.com>
         <87bkm1x47n.fsf@kernel.org>
         <2c2e656e-6dad-67d9-8da0-d507804e7df3@microchip.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.3 (3.46.3-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Fri, 2023-02-10 at 19:12 +0000, Ajay.Kathat@microchip.com wrote:
> Hi Kalle,
>=20
> On 2/10/23 02:25, Kalle Valo wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> >=20
> > Heiko Thiery <heiko.thiery@gmail.com> writes:
> >=20
> > > HI,
> > >=20
> > > Am Do., 9. Feb. 2023 um 22:19 Uhr schrieb Michael Walle
> > > <michael@walle.cc>:
> > > >=20
> > > > Am 2023-02-09 22:07, schrieb Jakub Kicinski:
> > > > > On Thu, 9 Feb 2023 18:51:58 +0000
> > > > > Ajay.Kathat@microchip.com=C2=A0wrote:
> > > > > > > netdev should be created with a valid lladdr, is there
> > > > > > > something
> > > > > > > wifi-specific here that'd prevalent that? The canonical
> > > > > > > flow is
> > > > > > > to this before registering the netdev:
> > > > > >=20
> > > > > > Here it's the timing in wilc1000 by when the MAC address is
> > > > > > available
> > > > > > to
> > > > > > read from NV. NV read is available in "mac_open"
> > > > > > net_device_ops
> > > > > > instead
> > > > > > of bus probe function. I think, mostly the operations on
> > > > > > netdev which
> > > > > > make use of mac address are performed after the "mac_open"
> > > > > > (I may be
> > > > > > missing something).
> > > > > >=20
> > > > > > Does it make sense to assign a random address in probe and
> > > > > > later read
> > > > > > back from NV in mac_open to make use of stored value?
> > > > >=20
> > > > > Hard to say, I'd suspect that may be even more confusing than
> > > > > starting with zeroes. There aren't any hard rules around the
> > > > > addresses AFAIK, but addrs are visible to user space. So user
> > > > > space will likely make assumptions based on the most commonly
> > > > > observed sequence (reading real addr at probe).
> > > >=20
> > > > Maybe we should also ask the NetworkManager guys. IMHO random
> > > > MAC address sounds bogus.
> > >=20
> > > Maybe it would be a "workaround" with loading the firmware while
> > > probing the device to set the real hw address.
> > >=20
> > > probe()
> > > =C2=A0 load_fw()
> > > =C2=A0 read_hw_addr_from_nv()
> > > =C2=A0 eth_hw_addr_set(ndev, addr)
> > > =C2=A0 unload_fw()
> > >=20
> > > mac_open()
> > > =C2=A0 load_fw()
> > >=20
> > > mac_close()
> > > =C2=A0 unload_fw()
> >=20
> > This is exactly what many wireless drivers already do and I
> > recommend
> > that wilc1000 would do the same.
> >=20
>=20
> In wilc1000, the bus interface is up in probe but we don't have
> access
> to mac address via register until the driver starts the wilc firmware
> because of design limitation. This information is only available
> after
> the MAC layer is initialized.

So... initialize the MAC layer and read the address, then stop the card
until dev open which reloads and reinits? That's what eg Atmel does
(though it has a special "read the MAC only" firmware to do that):

	/* No stored firmware so load a small stub which just
	   tells us the MAC address */
	int i;
	priv->card_type =3D CARD_TYPE_EEPROM;
	atmel_write16(dev, BSR, BSS_IRAM);
	atmel_copy_to_card(dev, 0, mac_reader, sizeof(mac_reader));
	atmel_set_gcr(dev, GCR_REMAP);
	atmel_clear_gcr(priv->dev, 0x0040);
	atmel_write16(dev, BSR, BSS_SRAM);
	for (i =3D LOOP_RETRY_LIMIT; i; i--)
		if (atmel_read16(dev, MR3) & MAC_BOOT_COMPLETE)
			break;
	if (i =3D=3D 0) {
		printk(KERN_ALERT "%s: MAC failed to boot MAC address reader.\n", dev->na=
me);
	} else {

		atmel_copy_to_host(dev, addr, atmel_read16(dev, MR2), 6);
		eth_hw_addr_set(dev, addr);
		/* got address, now squash it again until the network
		   interface is opened */
		if (priv->bus_type =3D=3D BUS_TYPE_PCCARD)
			atmel_write16(dev, GCR, 0x0060);
		atmel_write16(dev, GCR, 0x0040);
		rc =3D 1;
	}

Dan

>=20
>=20
> Regards,
> Ajay