The 02/23/2021 15:50, Vladimir Oltean wrote:
> On Tue, Feb 23, 2021 at 02:30:28PM +0100, Horatiu Vultur wrote:
> > The 02/22/2021 22:25, Vladimir Oltean wrote:
> > >
> > Hi Vladimir,
> > > Hi Horatiu,
> > >
> > > On Mon, Feb 22, 2021 at 08:46:26PM +0100, Horatiu Vultur wrote:
> > > > > - Why does ocelot support a single MRP ring if all it does is trap the
> > > > > MRP PDUs to the CPU? What is stopping it from supporting more than
> > > > > one ring?
> > > >
> > > > So the HW can support to run multiple rings. But to have an initial
> > > > basic implementation I have decided to support only one ring. So
> > > > basically is just a limitation in the driver.
> > >
> > > What should change in the current sw_backup implementation such that
> > > multiple rings are supported?
> >
> > Instead of single mrp_ring_id, mrp_p_port and mrp_s_port is to have a
> > list of these. And then when a new MRP instance is added/removed this
> > list should be updated. When the role is changed then find the MRP ports
> > from this list and put the rules to these ports.
>
> A physical port can't offload more than one ring id under any
> circumstance, no? So why keep a list and not just keep the MRP ring id
> in the ocelot_port structure, then when the ring role changes, just
> iterate through all ports and update the trapping rule on those having
> the same ring id?
Yes, a port can be part of only one ring. Yes, you should be able to do
it also like that, I don't see any issues with that approach.
>
> Also, why is it important to know which port is primary and which is
> secondary?
In this context is not important. It is important when MRM role is
offloaded to HW.
>
> > > > > - Why does ocelot not look at the MRM/MRC ring role at all, and it
> > > > > traps
> > > > > all MRP PDUs to the CPU, even those which it could forward as an
> > > > > MRC?
> > > > > I understood from your commit d8ea7ff3995e ("net: mscc: ocelot: Add
> > > > > support for MRP") description that the hardware should be able of
> > > > > forwarding the Test PDUs as a client, however it is obviously not
> > > > > doing that.
> > > >
> > > > It doesn't look at the role because it doesn't care. Because in both
> > > > cases is looking at the sw_backup because it doesn't support any role
> > > > completely. Maybe comment was misleading but I have put it under
> > > > 'current limitations' meaning that the HW can do that but the driver
> > > > doesn't take advantage of that yet. The same applies to multiple rings
> > > > support.
> > > >
> > > > The idea is to remove these limitations in the next patches and
> > > > to be able to remove these limitations then the driver will look also
> > > > at the role.
> > > >
> > > > [1] https://github.com/microchip-ung/mrp
> > >
> > > By the way, how can Ocelot trap some PDUs to the CPU but forward others?
> > > Doesn't it need to parse the MRP TLVs in order to determine whether they
> > > are Test packets or something else?
> >
> > No it doesn't need to do that. Because Test packets are send to dmac
> > 01:15:4e:00:00:01 while the other ring MRP frames are send to
> > 01:15:4e:00:00:02. And Ocelot can trap frames based on the dmac.
>
> Interesting, so I think with a little bit more forethought, the
> intentions with this MRP hardware assist would have been much clearer.
> From what you explained, the better implementation wouldn't have been
> more complicated than the current one is, just cleaner.
A better implementation will be to have also interconnect support. Again
the idea of the patch was to add minimum support for Ocelot and from
there to build on.
--
/Horatiu
