On Mon, Sep 26, 2022 at 08:12:14AM +0000, Yoshihiro Shimoda wrote:
> Hi Andrew,
>
> > From: Andrew Lunn, Sent: Friday, September 23, 2022 10:12 PM
> >
> > > +/* Forwarding engine block (MFWD) */
> > > +static void rswitch_fwd_init(struct rswitch_private *priv)
> > > +{
> > > + int i;
> > > +
> > > + for (i = 0; i < RSWITCH_NUM_HW; i++) {
> > > + iowrite32(FWPC0_DEFAULT, priv->addr + FWPC0(i));
> > > + iowrite32(0, priv->addr + FWPBFC(i));
> > > + }
> >
> > What is RSWITCH_NUM_HW?
>
> I think the name is unclear...
> Anyway, this hardware has 3 ethernet ports and 2 CPU ports.
> So that the RSWITCH_NUM_HW is 5. Perhaps, RSWITCH_NUM_ALL_PORTS
> is better name.
How do the CPU ports differ to the other ports? When you mention CPU
ports, it makes me wonder if this should be a DSA driver?
Is there a public data sheet for this device?
> Perhaps, since the current driver supports 1 ethernet port and 1 CPU port only,
> I should modify this driver for the current condition strictly.
I would suggest you support all three user ports. For an initial
driver you don't need to support any sort of acceleration. You don't
need any hardware bridging etc. That can be added later. Just three
separated ports.
> > > +
> > > + for (i = 0; i < RSWITCH_NUM_ETHA; i++) {
> >
> > RSWITCH_NUM_ETHA appears to be the number of ports?
>
> Yes, this is number of ethernet ports.
In the DSA world we call these user ports.
> > > + kfree(c->skb);
> > > + c->skb = NULL;
> >
> > When i see code like this, i wonder why an API call like
> > dev_kfree_skb() is not being used. I would suggest reaming this to
> > something other than skb, which has a very well understood meaning.
>
> Perhaps, c->skbs is better name than just c->skb.
Yes, that is O.K.
Andrew
