Re: [PATCH v2 3/3] ARM: orion5x: Add D-Link DNS-323 based on Device Tree

[maukka]

On 23.9.2022 00:10, Andrew Lunn wrote:
> > +static void __init dns323_dt_eth_fixup(void)
> > +{
> > +	struct device_node *np;
> > +	u8 addr[ETH_ALEN];
> > +	int ret;
> > +
> > +	/*
> > +	 * The ethernet interfaces forget the MAC address assigned by u-boot
> > +	 * if the clocks are turned off. Usually, u-boot on orion boards
> > +	 * has no DT support to properly set local-mac-address property.
> > +	 * As a workaround, we get the MAC address that is stored in flash
> > +	 * and update the port device node if no valid MAC address is set.
> > +	 */
>
> This is true for Kirkwood, but orion5x does not have any clocks to
> gate. So i'm pretty sure this is not true. You copied this code for a
> different reason. Please document here the real reason for this code.

Yes, will do. To my understanding it looks like uboot does not pass 
anything
to the kernel.

> > +	ret = dns323_read_mac_addr(addr);
> > +
> > +	if (ret) {
> > +		pr_warn("Unable to find MAC address in flash memory\n");
> > +		return;
> > +	}
> > +
> > +	np = of_find_compatible_node(NULL, NULL, "marvell,orion-eth-port");
> > +
> > +	if (!IS_ERR(np)) {
> > +		struct device_node *pnp = of_get_parent(np);
> > +		struct clk *clk;
> > +		struct property *pmac;
> > +		u8 tmpmac[ETH_ALEN];
> > +		u8 *macaddr;
> > +		int i;
> > +
> > +		if (!pnp)
> > +			return;
> > +
> > +		/* skip disabled nodes or nodes with valid MAC address*/
> > +		if (!of_device_is_available(pnp) ||
> > +		    !of_get_mac_address(np, tmpmac))
> > +			goto eth_fixup_skip;
> > +
> > +		clk = of_clk_get(pnp, 0);
> > +		if (IS_ERR(clk))
> > +			goto eth_fixup_skip;
> > +
> > +		/* ensure port clock is not gated to not hang CPU */
> > +		clk_prepare_enable(clk);
>
> I'm pretty sure this clock stuff is not needed. Please comment it out
> and see if the machine locks up. Kirkwood just stops dead if you
> access registers when there clocks are disabled. For Orion5x, the
> ethernet should always have a clock.

Will do.

> > +
> > +		/* store MAC address register contents in local-mac-address */
> > +		pmac = kzalloc(sizeof(*pmac) + 6, GFP_KERNEL);
> > +		if (!pmac)
> > +			goto eth_fixup_no_mem;
> > +
> > +		pmac->value = pmac + 1;
> > +		pmac->length = ETH_ALEN;
> > +		pmac->name = kstrdup("local-mac-address", GFP_KERNEL);
> > +		if (!pmac->name) {
> > +			kfree(pmac);
> > +			goto eth_fixup_no_mem;
> > +		}
> > +
> > +		macaddr = pmac->value;
> > +		for (i = 0; i < ETH_ALEN; i++)
> > +			macaddr[i] = addr[i];
> > +
> > +		of_update_property(np, pmac);
> > +
> > +eth_fixup_no_mem:
> > +		clk_disable_unprepare(clk);
> > +		clk_put(clk);
> > +eth_fixup_skip:
> > +		of_node_put(pnp);
> > +	}
> > +}
> > +
> > +void __init dns323_init_dt(void)
> > +{
> > +	if (of_machine_is_compatible("dlink,dns323a1")) {
> > +		writel(0, MPP_DEV_CTRL);		/* DEV_D[31:16] */
>
> I spotted this in dns323-setup.c as well. Do you have any idea what it
> does?

No idea. I have tried to replicate what was in dns323-setup.c as exactly 
as possible.
I can try to leave it out and see if anything changes.