Re: [PATCH net-next v3 6/8] net: phy: mscc: timestamping and PHC support
Hi Quentin, Quoting Quentin Schulz (2020-06-21 19:35:20) > On 2020-06-19 14:22, Antoine Tenart wrote: > [...] > > @@ -999,9 +1553,35 @@ int vsc8584_ptp_probe(struct phy_device *phydev) > > if (!vsc8531->ptp) > > return -ENOMEM; > > > > + mutex_init(>phc_lock); > > mutex_init(>ts_lock); > > > > + /* Retrieve the shared load/save GPIO. Request it as non exclusive as > > + * the same GPIO can be requested by all the PHYs of the same > > package. > > + * Ths GPIO must be used with the phc_lock taken (the lock is shared > > Typo + wrong lock named in the comment, instead: > > * This GPIO must be used with the gpio_lock taken (the lock is shared > > Though technically both are taken when access to the GPIO is requested > AFAICT. That's right, thanks for pointing this out! I'll fix it for v4. > Also on another note, maybe we could actually make vsc8531->base_addr > be a part of vsc85xx_shared_private structure. > > We would still need to compute it to pass it to devm_phy_package_join > but it can easily be returned by vsc8584_get_base_addr instead of the > current void and it'd put all the things used for all PHYs in the > package at the same place. We actually do not use directly the base_addr anymore from within the driver, thanks to the shared package conversion. We're now using __phy_package_write/__phy_package_read which are using the base address. So the move could be to remove it from the vsc8531_private. If we were to do it, we would need to find a clean solution: it's still part of a structure as multiple values are computed in vsc8584_get_base_addr, and it's a lot easier and cleaner to have them all in the same struct. Also, that have nothing to do with the current series :) Thanks, Antoine -- Antoine Ténart, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Re: [PATCH net-next v3 6/8] net: phy: mscc: timestamping and PHC support
Hi Andrew,
Quoting Andrew Lunn (2020-06-20 17:21:42)
> > + /* Retrieve the shared load/save GPIO. Request it as non exclusive as
> > + * the same GPIO can be requested by all the PHYs of the same package.
> > + * Ths GPIO must be used with the phc_lock taken (the lock is shared
> > + * between all PHYs).
> > + */
> > + vsc8531->load_save = devm_gpiod_get_optional(>mdio.dev,
> > "load-save",
> > +
> > GPIOD_FLAGS_BIT_NONEXCLUSIVE |
> > + GPIOD_OUT_LOW);
> > + if (IS_ERR(vsc8531->load_save)) {
> > + phydev_err(phydev, "Can't get load-save GPIO (%ld)\n",
> > +PTR_ERR(vsc8531->load_save));
> > + return PTR_ERR(vsc8531->load_save);
> > + }
> > +
>
> I can understand the GPIO being optional, it is only needed when PTP
> is being used. But i don't see a test anywhere that when PTP is being
> used the GPIO is provided. What actually happens if it is missing and
> somebody tries to use the PTP?
Not much would happen, the time set/get wouldn't be correct.
> Maybe only register the PTP parts with the core if the GPIO has been
> found in DT?
It's not easy, as some versions of the PHY (or the way it's integrated,
I'm not sure) do not need the GPIO. I'll double check, and if it is
required for PHC operations in this series, I'll do that.
Thanks,
Antoine
--
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [PATCH net-next v3 6/8] net: phy: mscc: timestamping and PHC support
Hi Andrew, Quoting Andrew Lunn (2020-06-20 17:40:08) > On Fri, Jun 19, 2020 at 02:22:58PM +0200, Antoine Tenart wrote: > > To get and set the PHC time, a GPIO has to be used and changes are only > > retrieved or committed when on a rising edge. The same GPIO is shared by > > all PHYs, so the granularity of the lock protecting it has to be > > different from the ones protecting the 1588 registers (the VSC8584 PHY > > has 2 1588 blocks, and a single load/save pin). > > I guess you thought about this GPIO quite a bit. > > It appears you have the mutex in the shared structure, but each PHY > has its own gpio_desc, even though it should be for the same GPIO? Yes, that's right. I had an early solution were I was sharing the GPIO in the shared structure, allowing to have a single gpio_desc. (dt would still needs to have one GPIO per PHY). That turned out to be a lot more complex that the current solution, having to unregister and free the GPIO desc manually when the driver of the last PHY was unregistered. I had to add lots of logic (and not the nicely looking one) to the shared PHY helpers in the core. > The binding requires each PHY has the GPIO, even though it is the same > GPIO. And there does not appear to be any checking that each PHY > really does have the same GPIO. Right. I don't see a clean and nice way to do this. Do you have an idea? On another hand, this would only lead to not being able to set/get (a correct) time from the PHC. > Ideally there would be a section in DT for the package, and this GPIO > would be there. But i don't see an good way to do this. Yes, I agree. That wasn't the design chosen when PHY packages were added. This PHY already has a shared description, duplicated for each PHY of the same package (for the interrupt line). If we were to change this one day, we probably would have to break dt compatibility anyway as there's already a property that is shared. > This does not feel right to me, but i've no good idea how it can be > made better :-( I think this kind of rework is out of this series scope; but I would be happy to discuss a better solution for someday. Thanks! Antoine -- Antoine Ténart, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Re: [PATCH net-next v3 6/8] net: phy: mscc: timestamping and PHC support
Hello Richard,
Quoting Richard Cochran (2020-06-20 17:00:45)
> On Fri, Jun 19, 2020 at 02:22:58PM +0200, Antoine Tenart wrote:
>
> > +static void vsc85xx_dequeue_skb(struct vsc85xx_ptp *ptp)
> > +{
> > + struct skb_shared_hwtstamps shhwtstamps;
> > + struct vsc85xx_ts_fifo fifo;
> > + struct sk_buff *skb;
> > + u8 skb_sig[16], *p;
> > + int i, len;
> > + u32 reg;
> > +
> > + memset(, 0, sizeof(fifo));
> > + p = (u8 *)
> > +
> > + reg = vsc85xx_ts_read_csr(ptp->phydev, PROCESSOR,
> > + MSCC_PHY_PTP_EGR_TS_FIFO(0));
> > + if (reg & PTP_EGR_TS_FIFO_EMPTY)
> > + return;
> > +
> > + *p++ = reg & 0xff;
> > + *p++ = (reg >> 8) & 0xff;
> > +
> > + /* Read the current FIFO item. Reading FIFO6 pops the next one. */
> > + for (i = 1; i < 7; i++) {
> > + reg = vsc85xx_ts_read_csr(ptp->phydev, PROCESSOR,
> > + MSCC_PHY_PTP_EGR_TS_FIFO(i));
> > + *p++ = reg & 0xff;
> > + *p++ = (reg >> 8) & 0xff;
> > + *p++ = (reg >> 16) & 0xff;
> > + *p++ = (reg >> 24) & 0xff;
> > + }
> > +
> > + len = skb_queue_len(>tx_queue);
> > + if (len < 1)
> > + return;
> > +
> > + while (len--) {
> > + skb = __skb_dequeue(>tx_queue);
> > + if (!skb)
> > + return;
> > +
> > + /* Can't get the signature of the packet, won't ever
> > + * be able to have one so let's dequeue the packet.
> > + */
> > + if (get_sig(skb, skb_sig) < 0)
> > + continue;
>
> This leaks the skb.
That's right, thanks for pointing this out! I'll fix it.
Thanks,
Antoine
--
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [PATCH net-next v3 6/8] net: phy: mscc: timestamping and PHC support
Hi Antoine, On 2020-06-19 14:22, Antoine Tenart wrote: [...] @@ -999,9 +1553,35 @@ int vsc8584_ptp_probe(struct phy_device *phydev) if (!vsc8531->ptp) return -ENOMEM; + mutex_init(>phc_lock); mutex_init(>ts_lock); + /* Retrieve the shared load/save GPIO. Request it as non exclusive as + * the same GPIO can be requested by all the PHYs of the same package. +* Ths GPIO must be used with the phc_lock taken (the lock is shared Typo + wrong lock named in the comment, instead: * This GPIO must be used with the gpio_lock taken (the lock is shared Though technically both are taken when access to the GPIO is requested AFAICT. Also on another note, maybe we could actually make vsc8531->base_addr be a part of vsc85xx_shared_private structure. We would still need to compute it to pass it to devm_phy_package_join but it can easily be returned by vsc8584_get_base_addr instead of the current void and it'd put all the things used for all PHYs in the package at the same place. Thanks, Quentin
Re: [PATCH net-next v3 6/8] net: phy: mscc: timestamping and PHC support
On Fri, Jun 19, 2020 at 02:22:58PM +0200, Antoine Tenart wrote: > To get and set the PHC time, a GPIO has to be used and changes are only > retrieved or committed when on a rising edge. The same GPIO is shared by > all PHYs, so the granularity of the lock protecting it has to be > different from the ones protecting the 1588 registers (the VSC8584 PHY > has 2 1588 blocks, and a single load/save pin). I guess you thought about this GPIO quite a bit. It appears you have the mutex in the shared structure, but each PHY has its own gpio_desc, even though it should be for the same GPIO? The binding requires each PHY has the GPIO, even though it is the same GPIO. And there does not appear to be any checking that each PHY really does have the same GPIO. Ideally there would be a section in DT for the package, and this GPIO would be there. But i don't see an good way to do this. This does not feel right to me, but i've no good idea how it can be made better :-( Andrew
Re: [PATCH net-next v3 6/8] net: phy: mscc: timestamping and PHC support
> + /* Retrieve the shared load/save GPIO. Request it as non exclusive as
> + * the same GPIO can be requested by all the PHYs of the same package.
> + * Ths GPIO must be used with the phc_lock taken (the lock is shared
> + * between all PHYs).
> + */
> + vsc8531->load_save = devm_gpiod_get_optional(>mdio.dev,
> "load-save",
> +
> GPIOD_FLAGS_BIT_NONEXCLUSIVE |
> + GPIOD_OUT_LOW);
> + if (IS_ERR(vsc8531->load_save)) {
> + phydev_err(phydev, "Can't get load-save GPIO (%ld)\n",
> +PTR_ERR(vsc8531->load_save));
> + return PTR_ERR(vsc8531->load_save);
> + }
> +
I can understand the GPIO being optional, it is only needed when PTP
is being used. But i don't see a test anywhere that when PTP is being
used the GPIO is provided. What actually happens if it is missing and
somebody tries to use the PTP? Maybe only register the PTP parts with
the core if the GPIO has been found in DT?
Andrew
Re: [PATCH net-next v3 6/8] net: phy: mscc: timestamping and PHC support
> + /* Retrieve the shared load/save GPIO. Request it as non exclusive as > + * the same GPIO can be requested by all the PHYs of the same package. > + * Ths GPIO must be used with the phc_lock taken (the lock is shared This Andrew
Re: [PATCH net-next v3 6/8] net: phy: mscc: timestamping and PHC support
On Fri, Jun 19, 2020 at 02:22:58PM +0200, Antoine Tenart wrote:
> +static void vsc85xx_dequeue_skb(struct vsc85xx_ptp *ptp)
> +{
> + struct skb_shared_hwtstamps shhwtstamps;
> + struct vsc85xx_ts_fifo fifo;
> + struct sk_buff *skb;
> + u8 skb_sig[16], *p;
> + int i, len;
> + u32 reg;
> +
> + memset(, 0, sizeof(fifo));
> + p = (u8 *)
> +
> + reg = vsc85xx_ts_read_csr(ptp->phydev, PROCESSOR,
> + MSCC_PHY_PTP_EGR_TS_FIFO(0));
> + if (reg & PTP_EGR_TS_FIFO_EMPTY)
> + return;
> +
> + *p++ = reg & 0xff;
> + *p++ = (reg >> 8) & 0xff;
> +
> + /* Read the current FIFO item. Reading FIFO6 pops the next one. */
> + for (i = 1; i < 7; i++) {
> + reg = vsc85xx_ts_read_csr(ptp->phydev, PROCESSOR,
> + MSCC_PHY_PTP_EGR_TS_FIFO(i));
> + *p++ = reg & 0xff;
> + *p++ = (reg >> 8) & 0xff;
> + *p++ = (reg >> 16) & 0xff;
> + *p++ = (reg >> 24) & 0xff;
> + }
> +
> + len = skb_queue_len(>tx_queue);
> + if (len < 1)
> + return;
> +
> + while (len--) {
> + skb = __skb_dequeue(>tx_queue);
> + if (!skb)
> + return;
> +
> + /* Can't get the signature of the packet, won't ever
> + * be able to have one so let's dequeue the packet.
> + */
> + if (get_sig(skb, skb_sig) < 0)
> + continue;
This leaks the skb.
> + /* Check if we found the signature we were looking for. */
> + if (!memcmp(skb_sig, fifo.sig, sizeof(fifo.sig))) {
> + memset(, 0, sizeof(shhwtstamps));
> + shhwtstamps.hwtstamp = ktime_set(fifo.secs, fifo.ns);
> + skb_complete_tx_timestamp(skb, );
> +
> + return;
> + }
> +
> + /* Valid signature but does not match the one of the
> + * packet in the FIFO right now, reschedule it for later
> + * packets.
> + */
> + __skb_queue_tail(>tx_queue, skb);
> + }
> +}
Thanks,
Richard
