From: Lukasz Stelmach <l.stelmach@samsung.com>
To: Andrew Lunn <andrew@lunn.ch>
Cc: "David S. Miller" <davem@davemloft.net>,
"Jakub Kicinski" <kuba@kernel.org>,
"Krzysztof Kozlowski" <krzk@kernel.org>,
"Kukjin Kim" <kgene@kernel.org>,
"Rob Herring" <robh+dt@kernel.org>,
"Russell King" <linux@armlinux.org.uk>,
jim.cromie@gmail.com, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-samsung-soc@vger.kernel.org,
netdev@vger.kernel.org, devicetree@vger.kernel.org,
"Bartłomiej Żolnierkiewicz" <b.zolnierkie@samsung.com>,
"Marek Szyprowski" <m.szyprowski@samsung.com>
Subject: Re: [PATCH v2 2/4] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver
Date: Fri, 16 Oct 2020 21:18:54 +0200 [thread overview]
Message-ID: <dleftj5z7a568x.fsf%l.stelmach@samsung.com> (raw)
In-Reply-To: <20201016180100.GF139700@lunn.ch> (Andrew Lunn's message of "Fri, 16 Oct 2020 20:01:00 +0200")
[-- Attachment #1: Type: text/plain, Size: 5869 bytes --]
It was <2020-10-16 pią 20:01>, when Andrew Lunn wrote:
>> >> +static void
>> >> +ax88796c_get_regs(struct net_device *ndev, struct ethtool_regs *regs, void *_p)
>> >> +{
>> >> + struct ax88796c_device *ax_local = to_ax88796c_device(ndev);
>> >> + u16 *p = _p;
>> >> + int offset, i;
>> >> +
>> >> + memset(p, 0, AX88796C_REGDUMP_LEN);
>> >> +
>> >> + for (offset = 0; offset < AX88796C_REGDUMP_LEN; offset += 2) {
>> >> + if (!test_bit(offset / 2, ax88796c_no_regs_mask))
>> >> + *p = AX_READ(&ax_local->ax_spi, offset);
>> >> + p++;
>> >> + }
>> >> +
>> >> + for (i = 0; i < AX88796C_PHY_REGDUMP_LEN / 2; i++) {
>> >> + *p = phy_read(ax_local->phydev, i);
>> >> + p++;
>> >
>> > Depending on the PHY, that can be dangerous.
>>
>> This is a built-in generic PHY. The chip has no lines to attach any
>> other external one.
>>
>> > phylib could be busy doing things with the PHY. It could be looking at
>>
>> How does phylib prevent concurrent access to a PHY?
>
> phydev->lock. All access to the PHY should go through the phylib,
> which will take the lock before calling into the driver.
>
>> > a different page for example.
>>
>> Different page?
>
> I was talking about the general case. A number of PHYs have more than
> 32 registers. So they implement pages to give access to more
> registers. For that to work, you need to ensure you don't have
> concurrent access.
>
It is not the case, this one has got only 7 and one only needs to make
sure the transaction in ax88796c_mdio_read() is not messed up.
>> > miitool(1) can give you the same functionally without the MAC driver
>> > doing anything, other than forwarding the IOCTL call on.
>>
>> No, I am afraid mii-tool is not able to dump registers.
>
> It should be able to.
>
> sudo mii-tool -vv eth0
> Using SIOCGMIIPHY=0x8947
> eth0: negotiated 1000baseT-FD flow-control, link ok
> registers for MII PHY 0:
> 1040 79ed 001c c800 0de1 c5e1 006d 0000
> 0000 0200 0800 0000 0000 0000 0000 2000
> 0000 0000 ffff 0000 0000 0400 0f00 0f00
> 318b 0053 31ec 8012 bf1f 0000 0000 0000
> product info: vendor 00:07:32, model 0 rev 0
> basic mode: autonegotiation enabled
> basic status: autonegotiation complete, link ok
> capabilities: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
> advertising: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD flow-control
> link partner: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD flow-control
Indeed. However, mii-tool(1) simply calls ioctl(SIOCGMIIREG) number of
times. Is looping in the userspace any better than in kernel? Anyway,
thanks for pointing out possible problems, I will make sure to avoid them.
>> >> +ax88796c_mdio_write(struct mii_bus *mdiobus, int phy_id, int loc, u16 val)
>> >> +{
>> >> + struct ax88796c_device *ax_local = mdiobus->priv;
>> >> + int ret;
>> >> +
>> >> + AX_WRITE(&ax_local->ax_spi, val, P2_MDIODR);
>> >> +
>> >> + AX_WRITE(&ax_local->ax_spi,
>> >> + MDIOCR_RADDR(loc) | MDIOCR_FADDR(phy_id)
>> >> + | MDIOCR_WRITE, P2_MDIOCR);
>> >> +
>> >> + ret = read_poll_timeout(AX_READ, ret,
>> >> + ((ret & MDIOCR_VALID) != 0), 0,
>> >> + jiffies_to_usecs(HZ / 100), false,
>> >> + &ax_local->ax_spi, P2_MDIOCR);
>> >> + if (ret)
>> >> + return -EIO;
>> >> +
>> >> + if (loc == MII_ADVERTISE) {
>> >> + AX_WRITE(&ax_local->ax_spi, (BMCR_FULLDPLX | BMCR_ANRESTART |
>> >> + BMCR_ANENABLE | BMCR_SPEED100), P2_MDIODR);
>> >> + AX_WRITE(&ax_local->ax_spi, (MDIOCR_RADDR(MII_BMCR) |
>> >> + MDIOCR_FADDR(phy_id) | MDIOCR_WRITE),
>> >> + P2_MDIOCR);
>> >>
>> >
>> > What is this doing?
>> >
>>
>> Well… it turns autonegotiation when changing advertised link modes. But
>> this is obvious. As to why this code is here, I will honestly say — I am
>> not sure (Reminder: this is a vendor driver I am porting, I am more than
>> happy to receive any comments, thank you). Apparently it is not required
>> and I am willing to remove it.
>
> Please do remove it.
>
Done.
>> >> +
>> >> + ret = devm_register_netdev(&spi->dev, ndev);
>> >> + if (ret) {
>> >> + dev_err(&spi->dev, "failed to register a network device\n");
>> >> + destroy_workqueue(ax_local->ax_work_queue);
>> >> + goto err;
>> >> + }
>> >
>> > The device is not live. If this is being used for NFS root, the kernel
>> > will start using it. So what sort of mess will it get into, if there
>> > is no PHY yet? Nothing important should happen after register_netdev().
>> >
>>
>> But, with an unregistered network device ndev_owner in
>> phy_attach_direct() is NULL. Thus, phy_connect_direct() below fails.
>>
>> --8<---------------cut here---------------start------------->8---
>> 1332 if (dev)
>> 1333 ndev_owner = dev->dev.parent->driver->owner;
>> 1334 if (ndev_owner != bus->owner && !try_module_get(bus->owner)) {
>> 1335 phydev_err(phydev, "failed to get the bus module\n");
>> 1336 return -EIO;
>> 1337 }
>> --8<---------------cut here---------------end--------------->8---
>
> Which is probably why most drivers actually attach the PHY in open()
> and detach it in close().
>
> It can be done in probe, just look around for a driver which does and
> copy it.
>
I will. Thanks for the info.
>> No problem. Do you have any recommendation how to express this
>>
>> #define PSR_RESET (0 << 15)
>>
>> I know it equals 0, but shows explicitly the bit number.
>
> Yes, that is useful for documentation. How about:
>
> #define PSR_NOT_RESET BIT(15)
>
> And then turn the logic around.
OK. This is an idea. I'll look around some more.
--
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]
next prev parent reply other threads:[~2020-10-16 19:19 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20201002192215eucas1p2c8f4a4bf8e411ed8ba75383fd58e85ac@eucas1p2.samsung.com>
2020-10-02 19:22 ` [PATCH v2 0/4] AX88796C SPI Ethernet Adapter Łukasz Stelmach
[not found] ` <CGME20201002192215eucas1p2c1d2baebfe2a9caa11d88175a2899fea@eucas1p2.samsung.com>
2020-10-02 19:22 ` [PATCH v2 1/4] dt-bindings: net: Add bindings for " Łukasz Stelmach
2020-10-03 10:09 ` Krzysztof Kozlowski
2020-10-05 14:03 ` Rob Herring
2020-10-05 13:59 ` Rob Herring
2020-10-05 14:01 ` Krzysztof Kozlowski
[not found] ` <CGME20201002192216eucas1p16f54584cf50fff56edc278102a66509e@eucas1p1.samsung.com>
2020-10-02 19:22 ` [PATCH v2 2/4] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver Łukasz Stelmach
2020-10-02 20:36 ` Andrew Lunn
[not found] ` <CGME20201013200453eucas1p1b77c93275b518422429ff1481f88a4be@eucas1p1.samsung.com>
2020-10-13 20:04 ` Lukasz Stelmach
2020-10-16 18:01 ` Andrew Lunn
[not found] ` <CGME20201016191905eucas1p235fb430a8f330a39e64f4fd6b81decb2@eucas1p2.samsung.com>
2020-10-16 19:18 ` Lukasz Stelmach [this message]
[not found] ` <CGME20201019125624eucas1p257a76c307adfb27202332658f93c9aba@eucas1p2.samsung.com>
2020-10-19 12:56 ` Lukasz Stelmach
2020-10-19 13:02 ` Andrew Lunn
2020-10-03 12:59 ` Heiner Kallweit
[not found] ` <CGME20201013200250eucas1p2445005531b86f246d7a14b7fc8016e80@eucas1p2.samsung.com>
2020-10-13 20:02 ` Lukasz Stelmach
[not found] ` <CGME20201002192216eucas1p16933608dcb0fb8ceee21caa3455cbaf1@eucas1p1.samsung.com>
2020-10-02 19:22 ` [PATCH v2 3/4] ARM: dts: exynos: Add Ethernet to Artik 5 board Łukasz Stelmach
2020-10-03 10:13 ` Krzysztof Kozlowski
[not found] ` <CGME20201006100556eucas1p2b69f76968a7a5901b5e9c66338c388d4@eucas1p2.samsung.com>
2020-10-06 10:05 ` Lukasz Stelmach
2020-10-06 10:17 ` Krzysztof Kozlowski
[not found] ` <CGME20201006140300eucas1p2006a96630d4a825500e5fc72016cf9d7@eucas1p2.samsung.com>
2020-10-06 14:02 ` Lukasz Stelmach
[not found] ` <CGME20201002192217eucas1p2357f80b100fb130d1ef1ac281042ff7c@eucas1p2.samsung.com>
2020-10-02 19:22 ` [PATCH v2 4/4] ARM: defconfig: Enable ax88796c driver Łukasz Stelmach
2020-10-02 19:45 ` [PATCH v2 0/4] AX88796C SPI Ethernet Adapter Andrew Lunn
[not found] ` <CGME20201006083749eucas1p160a3bed4cdb67cc8e05ca4a57d8907ca@eucas1p1.samsung.com>
2020-10-06 8:37 ` Lukasz Stelmach
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=dleftj5z7a568x.fsf%l.stelmach@samsung.com \
--to=l.stelmach@samsung.com \
--cc=andrew@lunn.ch \
--cc=b.zolnierkie@samsung.com \
--cc=davem@davemloft.net \
--cc=devicetree@vger.kernel.org \
--cc=jim.cromie@gmail.com \
--cc=kgene@kernel.org \
--cc=krzk@kernel.org \
--cc=kuba@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=m.szyprowski@samsung.com \
--cc=netdev@vger.kernel.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).