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

[maukka@ext.kapsi.fi]

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.