> On Fri, Jan 24, 2025 at 07:10:52PM +0530, Basharath Hussain Khaja wrote:
>> From: Roger Quadros <rogerq@xxxxxx>
>>
>> PRU-ICSS IEP module, which is capable of timestamping RX and
>> TX packets at HW level, is used for time synchronization by PTP4L.
>>
>> This change includes interaction between firmware and user space
>> application (ptp4l) with required packet timestamps. The driver
>> initializes the PRU firmware with appropriate mode and configuration
>> flags. Firmware updates local registers with the flags set by driver
>> and uses for further operation. RX SOF timestamp comes along with
>> packet and firmware will rise interrupt with TX SOF timestamp after
>> pushing the packet on to the wire.
>>
>> IEP driver is available in upstream and we are reusing for hardware
>> configuration for ICSSM as well. On top of that we have extended it
>> with the changes for AM57xx SoC.
>>
>> Extended ethtool for reading HW timestamping capability of the PRU
>> interfaces.
>>
>> Currently ordinary clock (OC) configuration has been validated with
>> Linux ptp4l.
>>
>> Signed-off-by: Roger Quadros <rogerq@xxxxxx>
>> Signed-off-by: Andrew F. Davis <afd@xxxxxx>
>> Signed-off-by: Parvathi Pudi <parvathi@xxxxxxxxxxx>
>> Signed-off-by: Basharath Hussain Khaja <basharath@xxxxxxxxxxx>
>
> ...
>
>> diff --git a/drivers/net/ethernet/ti/icssm/icssm_prueth.c
>> b/drivers/net/ethernet/ti/icssm/icssm_prueth.c
>
> ...
>
>> @@ -682,9 +899,22 @@ int icssm_emac_rx_packet(struct prueth_emac *emac, u16
>> *bd_rd_ptr,
>> src_addr += actual_pkt_len;
>> }
>>
>> + if (pkt_info->timestamp) {
>> + src_addr = (void *)roundup((uintptr_t)src_addr,
>> + ICSS_BLOCK_SIZE);
>
> Can PTR_ALIGN() be used here?
>
We are making sure Both roundup() and PTR_ALIGN() will point to
the same address location. We will change this in the next version.
>> + dst_addr = &ts;
>> + memcpy(dst_addr, src_addr, sizeof(ts));
>> + }
>> +
>> if (!pkt_info->sv_frame) {
>> skb_put(skb, actual_pkt_len);
>>
>> + if (icssm_prueth_ptp_rx_ts_is_enabled(emac) &&
>> + pkt_info->timestamp) {
>> + ssh = skb_hwtstamps(skb);
>> + memset(ssh, 0, sizeof(*ssh));
>> + ssh->hwtstamp = ns_to_ktime(ts);
>> + }
>> /* send packet up the stack */
>> skb->protocol = eth_type_trans(skb, ndev);
>> local_bh_disable();
>
> The code preceding the hunk below is:
>
> static int icssm_emac_request_irqs(struct prueth_emac *emac)
> {
> struct net_device *ndev = emac->ndev;
> int ret;
>
> ret = request_threaded_irq(emac->rx_irq, NULL, icssm_emac_rx_thread,
> IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> ndev->name, ndev);
> if (ret) {
> netdev_err(ndev, "unable to request RX IRQ\n");
> return ret;
> }
>
>> @@ -855,9 +1085,64 @@ static int icssm_emac_request_irqs(struct prueth_emac
>> *emac)
>> return ret;
>> }
>>
>> + if (emac->emac_ptp_tx_irq) {
>> + ret = request_threaded_irq(emac->emac_ptp_tx_irq,
>> + icssm_prueth_ptp_tx_irq_handle,
>> + icssm_prueth_ptp_tx_irq_work,
>> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
>> + ndev->name, ndev);
>> + if (ret) {
>> + netdev_err(ndev, "unable to request PTP TX IRQ\n");
>> + free_irq(emac->rx_irq, ndev);
>> + free_irq(emac->tx_irq, ndev);
>
> This seems somewhat asymmetric. This function does request emac->rx_irq
> but not emac->tx_irq. So I don't think it is appropriate to free emac->tx_irq
> here.
>
> Also, I would suggest using a goto label for unwind here.
>
Sure. We will clean this in the next version.
>> + }
>> + }
>> +
>> return ret;
>> }
>>
>
> ...
Thanks & Best Regards,
Basharath
