| Message ID | 20230908142919.14849-4-Parthiban.Veerasooran@microchip.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support for OPEN Alliance 10BASE-T1x MACPHY Serial Interface | expand |
> +int oa_tc6_configure(struct oa_tc6 *tc6, u8 cps, bool ctrl_prot, bool tx_cut_thr, > + bool rx_cut_thr) > +{ > + u32 regval; > + int ret; > + > + /* Read and configure the IMASK0 register for unmasking the interrupts */ > + ret = oa_tc6_read_register(tc6, OA_TC6_IMASK0, ®val, 1); > + if (ret) > + return ret; > + > + regval &= TXPEM & TXBOEM & TXBUEM & RXBOEM & LOFEM & HDREM; > + ret = oa_tc6_write_register(tc6, OA_TC6_IMASK0, ®val, 1); It is not so obvious what this 1 means. Maybe change to regval[1], and user ARRAY_SIZE(). What also does not help is the function name, oa_tc6_write_register(). Singular. So it appears to write one register, not multiple registers. It might even make sense to make oa_tc6_write_register() truly access a single register, and add oa_tc6_write_registers() for multiple registers. > +/* Unmasking interrupt fields in IMASK0 */ > +#define HDREM ~BIT(5) /* Header Error Mask */ > +#define LOFEM ~BIT(4) /* Loss of Framing Error Mask */ > +#define RXBOEM ~BIT(3) /* Rx Buffer Overflow Error Mask */ > +#define TXBUEM ~BIT(2) /* Tx Buffer Underflow Error Mask */ > +#define TXBOEM ~BIT(1) /* Tx Buffer Overflow Error Mask */ > +#define TXPEM ~BIT(0) /* Tx Protocol Error Mask */ Using ~BIT(X) is very usual. I would not do this, Principle of Least Surprise. > struct oa_tc6 { > - struct spi_device *spi; > - bool ctrl_prot; > + struct completion rst_complete; > struct task_struct *tc6_task; > wait_queue_head_t tc6_wq; > + struct spi_device *spi; > + bool tx_cut_thr; > + bool rx_cut_thr; > + bool ctrl_prot; > bool int_flag; > - struct completion rst_complete; > + u8 cps; > }; Please try not to move stuff around. It makes the diff bigger than it should be. Andrew
Hi Andrew, On 14/09/23 6:16 am, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> +int oa_tc6_configure(struct oa_tc6 *tc6, u8 cps, bool ctrl_prot, bool tx_cut_thr, >> + bool rx_cut_thr) >> +{ >> + u32 regval; >> + int ret; >> + >> + /* Read and configure the IMASK0 register for unmasking the interrupts */ >> + ret = oa_tc6_read_register(tc6, OA_TC6_IMASK0, ®val, 1); >> + if (ret) >> + return ret; >> + >> + regval &= TXPEM & TXBOEM & TXBUEM & RXBOEM & LOFEM & HDREM; >> + ret = oa_tc6_write_register(tc6, OA_TC6_IMASK0, ®val, 1); > > It is not so obvious what this 1 means. Maybe change to regval[1], and > user ARRAY_SIZE(). What also does not help is the function name, > oa_tc6_write_register(). Singular. So it appears to write one register, > not multiple registers. It might even make sense to make > oa_tc6_write_register() truly access a single register, and add > oa_tc6_write_registers() for multiple registers. Ok, I will implement two functions to serve their purposes. > >> +/* Unmasking interrupt fields in IMASK0 */ >> +#define HDREM ~BIT(5) /* Header Error Mask */ >> +#define LOFEM ~BIT(4) /* Loss of Framing Error Mask */ >> +#define RXBOEM ~BIT(3) /* Rx Buffer Overflow Error Mask */ >> +#define TXBUEM ~BIT(2) /* Tx Buffer Underflow Error Mask */ >> +#define TXBOEM ~BIT(1) /* Tx Buffer Overflow Error Mask */ >> +#define TXPEM ~BIT(0) /* Tx Protocol Error Mask */ > > Using ~BIT(X) is very usual. I would not do this, Principle of Least > Surprise. Sorry, I don't get your point. Could you please explain bit more? > >> struct oa_tc6 { >> - struct spi_device *spi; >> - bool ctrl_prot; >> + struct completion rst_complete; >> struct task_struct *tc6_task; >> wait_queue_head_t tc6_wq; >> + struct spi_device *spi; >> + bool tx_cut_thr; >> + bool rx_cut_thr; >> + bool ctrl_prot; >> bool int_flag; >> - struct completion rst_complete; >> + u8 cps; >> }; > > Please try not to move stuff around. It makes the diff bigger than it > should be. Ah ok, will take care in the next version. Best Regards, Parthiban V > > Andrew >
> >> +/* Unmasking interrupt fields in IMASK0 */ > >> +#define HDREM ~BIT(5) /* Header Error Mask */ > >> +#define LOFEM ~BIT(4) /* Loss of Framing Error Mask */ > >> +#define RXBOEM ~BIT(3) /* Rx Buffer Overflow Error Mask */ > >> +#define TXBUEM ~BIT(2) /* Tx Buffer Underflow Error Mask */ > >> +#define TXBOEM ~BIT(1) /* Tx Buffer Overflow Error Mask */ > >> +#define TXPEM ~BIT(0) /* Tx Protocol Error Mask */ > > > > Using ~BIT(X) is very usual. I would not do this, Principle of Least > > Surprise. > Sorry, I don't get your point. Could you please explain bit more? Look around kernel header files. How often do you see ~BIT(5)? My guess it is approximately 0. So i'm suggesting you remove the ~ and have the user of the #define assemble the mask and then do the ~ . Andrew
Hi Andrew, On 19/09/23 6:24 pm, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >>>> +/* Unmasking interrupt fields in IMASK0 */ >>>> +#define HDREM ~BIT(5) /* Header Error Mask */ >>>> +#define LOFEM ~BIT(4) /* Loss of Framing Error Mask */ >>>> +#define RXBOEM ~BIT(3) /* Rx Buffer Overflow Error Mask */ >>>> +#define TXBUEM ~BIT(2) /* Tx Buffer Underflow Error Mask */ >>>> +#define TXBOEM ~BIT(1) /* Tx Buffer Overflow Error Mask */ >>>> +#define TXPEM ~BIT(0) /* Tx Protocol Error Mask */ >>> >>> Using ~BIT(X) is very usual. I would not do this, Principle of Least >>> Surprise. >> Sorry, I don't get your point. Could you please explain bit more? > > Look around kernel header files. How often do you see ~BIT(5)? My > guess it is approximately 0. So i'm suggesting you remove the ~ and > have the user of the #define assemble the mask and then do the ~ . Ah ok ok, thanks for the clarification. Best Regards, Parthiban V > > Andrew
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c index 0019f70345b6..65a7317f768d 100644 --- a/drivers/net/ethernet/oa_tc6.c +++ b/drivers/net/ethernet/oa_tc6.c @@ -287,6 +287,45 @@ int oa_tc6_read_register(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len) } EXPORT_SYMBOL_GPL(oa_tc6_read_register); +int oa_tc6_configure(struct oa_tc6 *tc6, u8 cps, bool ctrl_prot, bool tx_cut_thr, + bool rx_cut_thr) +{ + u32 regval; + int ret; + + /* Read and configure the IMASK0 register for unmasking the interrupts */ + ret = oa_tc6_read_register(tc6, OA_TC6_IMASK0, ®val, 1); + if (ret) + return ret; + + regval &= TXPEM & TXBOEM & TXBUEM & RXBOEM & LOFEM & HDREM; + ret = oa_tc6_write_register(tc6, OA_TC6_IMASK0, ®val, 1); + if (ret) + return ret; + + /* Configure the CONFIG0 register with the required configurations */ + regval = SYNC; + if (ctrl_prot) + regval |= PROTE; + if (tx_cut_thr) + regval |= TXCTE; + if (rx_cut_thr) + regval |= RXCTE; + regval |= FIELD_PREP(CPS, ilog2(cps) / ilog2(2)); + + ret = oa_tc6_write_register(tc6, OA_TC6_CONFIG0, ®val, 1); + if (ret) + return ret; + + tc6->cps = cps; + tc6->ctrl_prot = ctrl_prot; + tc6->tx_cut_thr = tx_cut_thr; + tc6->rx_cut_thr = rx_cut_thr; + + return 0; +} +EXPORT_SYMBOL_GPL(oa_tc6_configure); + struct oa_tc6 *oa_tc6_init(struct spi_device *spi) { struct oa_tc6 *tc6; diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h index 315f061c2dfe..fa29c4e09720 100644 --- a/include/linux/oa_tc6.h +++ b/include/linux/oa_tc6.h @@ -19,11 +19,28 @@ /* Open Alliance TC6 Standard Control and Status Registers */ #define OA_TC6_RESET 0x0003 /* Reset Control and Status Register */ +#define OA_TC6_CONFIG0 0x0004 /* Configuration Register #0 */ #define OA_TC6_STS0 0x0008 /* Status Register #0 */ +#define OA_TC6_IMASK0 0x000C /* Interrupt Mask Register #0 */ /* RESET register field */ #define SW_RESET BIT(0) /* Software Reset */ +/* CONFIG0 register fields */ +#define SYNC BIT(15) /* Configuration Synchronization */ +#define TXCTE BIT(9) /* Tx cut-through enable */ +#define RXCTE BIT(8) /* Rx cut-through enable */ +#define PROTE BIT(5) /* Ctrl read/write Protection Enable */ +#define CPS GENMASK(2, 0) /* Chunk Payload Size */ + +/* Unmasking interrupt fields in IMASK0 */ +#define HDREM ~BIT(5) /* Header Error Mask */ +#define LOFEM ~BIT(4) /* Loss of Framing Error Mask */ +#define RXBOEM ~BIT(3) /* Rx Buffer Overflow Error Mask */ +#define TXBUEM ~BIT(2) /* Tx Buffer Underflow Error Mask */ +#define TXBOEM ~BIT(1) /* Tx Buffer Overflow Error Mask */ +#define TXPEM ~BIT(0) /* Tx Protocol Error Mask */ + /* STATUS0 register field */ #define RESETC BIT(6) /* Reset Complete */ @@ -31,15 +48,20 @@ #define TC6_FTR_SIZE 4 /* Ctrl command footer size ss per OA */ struct oa_tc6 { - struct spi_device *spi; - bool ctrl_prot; + struct completion rst_complete; struct task_struct *tc6_task; wait_queue_head_t tc6_wq; + struct spi_device *spi; + bool tx_cut_thr; + bool rx_cut_thr; + bool ctrl_prot; bool int_flag; - struct completion rst_complete; + u8 cps; }; struct oa_tc6 *oa_tc6_init(struct spi_device *spi); int oa_tc6_deinit(struct oa_tc6 *tc6); int oa_tc6_write_register(struct oa_tc6 *tc6, u32 addr, u32 value[], u8 len); int oa_tc6_read_register(struct oa_tc6 *tc6, u32 addr, u32 value[], u8 len); +int oa_tc6_configure(struct oa_tc6 *tc6, u8 cps, bool ctrl_prot, bool tx_cut_thr, + bool rx_cut_thr);
Read STDCAP register for the MAC-PHY capability and check against the configuration parameters such as chunk payload, tx cut through and rx cut through to configure the MAC-PHY. It also configures the control command protected/unprotected mode. In cut through mode configuration the MAC-PHY doesn't buffer the incoming data. In tx case, it passes the data to the network if the configured cps of data available. In rx case, it passes the data to the SPI host if the configured cps of data available from the network. Also disables all the errors mask in the IMASK0 register to check for the errors. Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> --- drivers/net/ethernet/oa_tc6.c | 39 +++++++++++++++++++++++++++++++++++ include/linux/oa_tc6.h | 28 ++++++++++++++++++++++--- 2 files changed, 64 insertions(+), 3 deletions(-)