Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
Date:   Fri, 23 Sep 2022 12:03:14 +0300
From:   maukka@ext.kapsi.fi
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        arnd@arndb.de, olof@lixom.net, sebastian.hesselbarth@gmail.com,
        gregory.clement@bootlin.com, linux@armlinux.org.uk,
        pali@kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
In-Reply-To: <YyzPVMrfcOkvngxl@lunn.ch>
References: <20220427162123.110458-1-maukka@ext.kapsi.fi>
 <20220922202458.7592-1-maukka@ext.kapsi.fi>
 <20220922202458.7592-4-maukka@ext.kapsi.fi> <YyzPVMrfcOkvngxl@lunn.ch>
Message-ID: <6dc27862f8460f875c31ad2de56baa9f@ext.kapsi.fi>
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [PATCH v2 3/3] ARM: orion5x: Add D-Link DNS-323 based on Device
 Tree
Precedence: bulk

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.