[RFC,net-next,1/6] net: ethernet: implement OPEN Alliance control transaction interface

Commit Message

Parthiban Veerasooran Sept. 8, 2023, 2:29 p.m. UTC

Implement register read/write interface according to the control
communication specified in the OPEN Alliance 10BASE-T1x MACPHY Serial
Interface document. Control transactions consist of one or more control
commands. Control commands are used by the SPI host to read and write
registers within the MAC-PHY. Each control commands are composed of a
32-bit control command header followed by register data.

Control write commands can write either a single register or multiple
consecutive registers. When multiple consecutive registers are written,
the address is automatically post-incremented by the MAC-PHY. The write
command and data is also echoed from the MAC-PHY back to the SPI host to
enable the SPI host to identify which register write failed in the case
of any bus errors.

Control read commands can read either a single register or multiple
consecutive registers. When multiple consecutive registers are read, the
address is automatically post-incremented by the MAC-PHY.

The register data being read or written can be protected against simple
bit errors. When enabled by setting the Protection Enable (PROTE) bit in
the CONFIG0 register, protection is accomplished by duplication of each
32-bit word containing register data with its ones’ complement. Errors
are detected at the receiver by performing a simple exclusive-OR (XOR) of
each received 32-bit word containing register data with its received
complement and detecting if there are any zeros in the result.

Signed-off-by: Parthiban Veerasooran <[e-mail]>
---
 Documentation/networking/oa-tc6-framework.rst | 231 ++++++++++++++++++
 MAINTAINERS                                   |   8 +
 drivers/net/ethernet/oa_tc6.c                 | 222 +++++++++++++++++
 include/linux/oa_tc6.h                        |  31 +++
 4 files changed, 492 insertions(+)
 create mode 100644 Documentation/networking/oa-tc6-framework.rst
 create mode 100644 drivers/net/ethernet/oa_tc6.c
 create mode 100644 include/linux/oa_tc6.h

Comments

Andrew Lunn Sept. 9, 2023, 1:33 p.m. UTC | #1

On Fri, Sep 08, 2023 at 07:59:14PM +0530, Parthiban Veerasooran wrote:
> Implement register read/write interface according to the control
> communication specified in the OPEN Alliance 10BASE-T1x MACPHY Serial
> Interface document. Control transactions consist of one or more control
> commands. Control commands are used by the SPI host to read and write
> registers within the MAC-PHY. Each control commands are composed of a
> 32-bit control command header followed by register data.
> 
> Control write commands can write either a single register or multiple
> consecutive registers. When multiple consecutive registers are written,
> the address is automatically post-incremented by the MAC-PHY. The write
> command and data is also echoed from the MAC-PHY back to the SPI host to
> enable the SPI host to identify which register write failed in the case
> of any bus errors.
> 
> Control read commands can read either a single register or multiple
> consecutive registers. When multiple consecutive registers are read, the
> address is automatically post-incremented by the MAC-PHY.
> 
> The register data being read or written can be protected against simple
> bit errors. When enabled by setting the Protection Enable (PROTE) bit in
> the CONFIG0 register, protection is accomplished by duplication of each
> 32-bit word containing register data with its ones’ complement. Errors
> are detected at the receiver by performing a simple exclusive-OR (XOR) of
> each received 32-bit word containing register data with its received
> complement and detecting if there are any zeros in the result.
> 
> Signed-off-by: Parthiban Veerasooran <[e-mail]>
> ---
>  Documentation/networking/oa-tc6-framework.rst | 231 ++++++++++++++++++
>  MAINTAINERS                                   |   8 +
>  drivers/net/ethernet/oa_tc6.c                 | 222 +++++++++++++++++
>  include/linux/oa_tc6.h                        |  31 +++

I'm surprised there is no kconfig and Makefile changes here. I would
expect this is compiled as a module, which the vendor code can then
make use of. 

     Andrew

Parthiban Veerasooran Sept. 12, 2023, 1:03 p.m. UTC | #2

Hi Andrew,

On 09/09/23 7:03 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Sep 08, 2023 at 07:59:14PM +0530, Parthiban Veerasooran wrote:
>> Implement register read/write interface according to the control
>> communication specified in the OPEN Alliance 10BASE-T1x MACPHY Serial
>> Interface document. Control transactions consist of one or more control
>> commands. Control commands are used by the SPI host to read and write
>> registers within the MAC-PHY. Each control commands are composed of a
>> 32-bit control command header followed by register data.
>>
>> Control write commands can write either a single register or multiple
>> consecutive registers. When multiple consecutive registers are written,
>> the address is automatically post-incremented by the MAC-PHY. The write
>> command and data is also echoed from the MAC-PHY back to the SPI host to
>> enable the SPI host to identify which register write failed in the case
>> of any bus errors.
>>
>> Control read commands can read either a single register or multiple
>> consecutive registers. When multiple consecutive registers are read, the
>> address is automatically post-incremented by the MAC-PHY.
>>
>> The register data being read or written can be protected against simple
>> bit errors. When enabled by setting the Protection Enable (PROTE) bit in
>> the CONFIG0 register, protection is accomplished by duplication of each
>> 32-bit word containing register data with its ones’ complement. Errors
>> are detected at the receiver by performing a simple exclusive-OR (XOR) of
>> each received 32-bit word containing register data with its received
>> complement and detecting if there are any zeros in the result.
>>
>> Signed-off-by: Parthiban Veerasooran <[e-mail]>
>> ---
>>   Documentation/networking/oa-tc6-framework.rst | 231 ++++++++++++++++++
>>   MAINTAINERS                                   |   8 +
>>   drivers/net/ethernet/oa_tc6.c                 | 222 +++++++++++++++++
>>   include/linux/oa_tc6.h                        |  31 +++
> 
> I'm surprised there is no kconfig and Makefile changes here. I would
> expect this is compiled as a module, which the vendor code can then
> make use of.
Ok, actually this framework is used by vendor specific driver 
(lan865x.c) later with the Makefile update like below in the directory 
drivers/net/ethernet/microchip/,

obj-$(CONFIG_LAN865X) += lan865x_t1s.o 

lan865x_t1s-objs := lan865x.o ../oa_tc6.o

If I understand you correctly, this framework has to include the module 
initialization as well using the below APIs and has to be compiled as a 
loadable module so that other vendors module can make use of this, isn't it?

module_init(oa_tc6_init);
module_exit(oa_tc6_exit);

Best Regards,
Parthiban V
> 
>       Andrew
>

Andrew Lunn Sept. 13, 2023, 1:32 a.m. UTC | #3

> If I understand you correctly, this framework has to include the module 
> initialization as well using the below APIs and has to be compiled as a 
> loadable module so that other vendors module can make use of this, isn't it?
> 
> module_init(oa_tc6_init);
> module_exit(oa_tc6_exit);

You should not need these, unless there is actions which need to be
taken when the module is loaded. If there are no actions, it is purely
a library, don't have them. The module dependency tracking code will
see that the MAC driver modules has dependencies on symbols in this
library module, and will load it first. The MAC driver is then loaded,
and the kernel linker will resolve the missing symbols in the MAC
driver to those in the library. It also means that there is only ever
one copy of the library in the kernel, even if there is multiple MAC
drivers using it.

       Andrew

Andrew Lunn Sept. 13, 2023, 1:36 a.m. UTC | #4

> +int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len,
> +			bool wnr, bool ctrl_prot)

Please add some kerneldoc headers for the API method exposed
here. These are what the MAC driver should be using, so they should be
reasonably well documented.

Thanks
	   Andrew

Andrew Lunn Sept. 13, 2023, 2:11 a.m. UTC | #5

> +static bool oa_tc6_get_parity(u32 p)
> +{
> +	bool parity = true;
> +
> +	/* This function returns an odd parity bit */
> +	while (p) {
> +		parity = !parity;
> +		p = p & (p - 1);
> +	}
> +	return parity;

Please take a look around and see if you can find another
implementation in the kernel which can be used. If not, you could copy/paste:

https://elixir.bootlin.com/linux/latest/source/lib/bch.c#L348

which is probably more efficient.

> +static void oa_tc6_prepare_ctrl_buf(struct oa_tc6 *tc6, u32 addr, u32 val[],
> +				    u8 len, bool wnr, u8 *buf, bool ctrl_prot)
> +{
> +	u32 hdr;
> +
> +	/* Prepare the control header with the required details */
> +	hdr = FIELD_PREP(CTRL_HDR_DNC, 0) |
> +	      FIELD_PREP(CTRL_HDR_WNR, wnr) |
> +	      FIELD_PREP(CTRL_HDR_AID, 0) |
> +	      FIELD_PREP(CTRL_HDR_MMS, addr >> 16) |
> +	      FIELD_PREP(CTRL_HDR_ADDR, addr) |
> +	      FIELD_PREP(CTRL_HDR_LEN, len - 1);
> +	hdr |= FIELD_PREP(CTRL_HDR_P, oa_tc6_get_parity(hdr));
> +	*(u32 *)buf = cpu_to_be32(hdr);
> +
> +	if (wnr) {

What does wnr mean? Maybe give it a more meaningful name, unless it is
actually something in the standard. Kerneldoc would also help.

> +static int oa_tc6_check_control(struct oa_tc6 *tc6, u8 *ptx, u8 *prx, u8 len,
> +				bool wnr, bool ctrl_prot)
> +{
> +	/* 1st 4 bytes of rx chunk data can be discarded */
> +	u32 rx_hdr = *(u32 *)&prx[TC6_HDR_SIZE];
> +	u32 tx_hdr = *(u32 *)ptx;
> +	u32 rx_data_complement;
> +	u32 tx_data;
> +	u32 rx_data;
> +	u16 pos1;
> +	u16 pos2;
> +
> +	/* If tx hdr and echoed hdr are not equal then there might be an issue
> +	 * with the connection between SPI host and MAC-PHY. Here this case is
> +	 * considered as MAC-PHY is not connected.

I could understand ENODEV on the first transaction during probe. But
after that -EIO seems more appropriate. I've also seen USB use -EPROTO
to indicate a protocol error, which a corrupt message would be.

> +int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len,
> +			bool wnr, bool ctrl_prot)
> +{
> +	u8 *tx_buf;
> +	u8 *rx_buf;
> +	u16 size;
> +	u16 pos;
> +	int ret;
> +
> +	if (ctrl_prot)
> +		size = (TC6_HDR_SIZE * 2) + (len * (TC6_HDR_SIZE * 2));
> +	else
> +		size = (TC6_HDR_SIZE * 2) + (len * TC6_HDR_SIZE);

Do you have an idea how big the biggest control message is? Rather
than allocate these buffers for every transaction, maybe allocate
maximum size buffers one at startup and keep them in tc6? That will
reduce overhead and simplify the code.

> +struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
> +{
> +	struct oa_tc6 *tc6;
> +
> +	if (!spi)
> +		return NULL;

This is defensive programming which is generally not liked. You cannot
do anything without an SPI device, so just assume it is passed, and if
not, let is explode later and the driver write will quickly fix there
broken code.

> +
> +	tc6 = kzalloc(sizeof(*tc6), GFP_KERNEL);
> +	if (!tc6)
> +		return NULL;
> +
> +	tc6->spi = spi;
> +
> +	return tc6;
> +}
> +EXPORT_SYMBOL_GPL(oa_tc6_init);
> +
> +void oa_tc6_deinit(struct oa_tc6 *tc6)
> +{
> +	kfree(tc6);
> +}
> +EXPORT_SYMBOL_GPL(oa_tc6_deinit);

Maybe consider a devm_ API to make the MAC driver simpler.

      Andrew

Andrew Lunn Sept. 13, 2023, 2:16 a.m. UTC | #6

> +struct oa_tc6 {
> +	struct spi_device *spi;
> +	bool ctrl_prot;
> +};

Should this be considered an opaque structure which the MAC driver
should not access the members?

I don't see anything setting ctrl_prot here. Does it need a setter and
a getter?

	Andrew

Parthiban Veerasooran Sept. 19, 2023, 11:13 a.m. UTC | #7

Hi Andrew,

On 13/09/23 7:46 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> +struct oa_tc6 {
>> +     struct spi_device *spi;
>> +     bool ctrl_prot;
>> +};
> 
> Should this be considered an opaque structure which the MAC driver
> should not access the members?
> 
> I don't see anything setting ctrl_prot here. Does it need a setter and
> a getter?
Ah ok, it is supposed to be done in the oa_tc6_init() function. Somehow 
missed it. Will correct it in the next version.

Best Regards,
Parthiban V
> 
>          Andrew

Parthiban Veerasooran Sept. 19, 2023, 11:38 a.m. UTC | #8

Hi Andrew,

On 13/09/23 7:41 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> +static bool oa_tc6_get_parity(u32 p)
>> +{
>> +     bool parity = true;
>> +
>> +     /* This function returns an odd parity bit */
>> +     while (p) {
>> +             parity = !parity;
>> +             p = p & (p - 1);
>> +     }
>> +     return parity;
> 
> Please take a look around and see if you can find another
> implementation in the kernel which can be used. If not, you could copy/paste:
> 
> https://elixir.bootlin.com/linux/latest/source/lib/bch.c#L348
> 
> which is probably more efficient.
Sure, will check out it.
> 
>> +static void oa_tc6_prepare_ctrl_buf(struct oa_tc6 *tc6, u32 addr, u32 val[],
>> +                                 u8 len, bool wnr, u8 *buf, bool ctrl_prot)
>> +{
>> +     u32 hdr;
>> +
>> +     /* Prepare the control header with the required details */
>> +     hdr = FIELD_PREP(CTRL_HDR_DNC, 0) |
>> +           FIELD_PREP(CTRL_HDR_WNR, wnr) |
>> +           FIELD_PREP(CTRL_HDR_AID, 0) |
>> +           FIELD_PREP(CTRL_HDR_MMS, addr >> 16) |
>> +           FIELD_PREP(CTRL_HDR_ADDR, addr) |
>> +           FIELD_PREP(CTRL_HDR_LEN, len - 1);
>> +     hdr |= FIELD_PREP(CTRL_HDR_P, oa_tc6_get_parity(hdr));
>> +     *(u32 *)buf = cpu_to_be32(hdr);
>> +
>> +     if (wnr) {
> 
> What does wnr mean? Maybe give it a more meaningful name, unless it is
> actually something in the standard. Kerneldoc would also help.
Ah, it is "write not read". Shall I name it as "write_not_read" ?
> 
>> +static int oa_tc6_check_control(struct oa_tc6 *tc6, u8 *ptx, u8 *prx, u8 len,
>> +                             bool wnr, bool ctrl_prot)
>> +{
>> +     /* 1st 4 bytes of rx chunk data can be discarded */
>> +     u32 rx_hdr = *(u32 *)&prx[TC6_HDR_SIZE];
>> +     u32 tx_hdr = *(u32 *)ptx;
>> +     u32 rx_data_complement;
>> +     u32 tx_data;
>> +     u32 rx_data;
>> +     u16 pos1;
>> +     u16 pos2;
>> +
>> +     /* If tx hdr and echoed hdr are not equal then there might be an issue
>> +      * with the connection between SPI host and MAC-PHY. Here this case is
>> +      * considered as MAC-PHY is not connected.
> 
> I could understand ENODEV on the first transaction during probe. But
> after that -EIO seems more appropriate. I've also seen USB use -EPROTO
> to indicate a protocol error, which a corrupt message would be.
Ah ok, then in this case I will consider -EIO.
> 
>> +int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len,
>> +                     bool wnr, bool ctrl_prot)
>> +{
>> +     u8 *tx_buf;
>> +     u8 *rx_buf;
>> +     u16 size;
>> +     u16 pos;
>> +     int ret;
>> +
>> +     if (ctrl_prot)
>> +             size = (TC6_HDR_SIZE * 2) + (len * (TC6_HDR_SIZE * 2));
>> +     else
>> +             size = (TC6_HDR_SIZE * 2) + (len * TC6_HDR_SIZE);
> 
> Do you have an idea how big the biggest control message is? Rather
> than allocate these buffers for every transaction, maybe allocate
> maximum size buffers one at startup and keep them in tc6? That will
> reduce overhead and simplify the code.
Ok, as per OA spec, up to 128 consecutive registers read or write can be 
possible. So the maximum possible size would be 1032. As you suggested 
will allocate this size of memory in the startup.
> 
>> +struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
>> +{
>> +     struct oa_tc6 *tc6;
>> +
>> +     if (!spi)
>> +             return NULL;
> 
> This is defensive programming which is generally not liked. You cannot
> do anything without an SPI device, so just assume it is passed, and if
> not, let is explode later and the driver write will quickly fix there
> broken code.
Ah yes, will remove this check.
> 
>> +
>> +     tc6 = kzalloc(sizeof(*tc6), GFP_KERNEL);
>> +     if (!tc6)
>> +             return NULL;
>> +
>> +     tc6->spi = spi;
>> +
>> +     return tc6;
>> +}
>> +EXPORT_SYMBOL_GPL(oa_tc6_init);
>> +
>> +void oa_tc6_deinit(struct oa_tc6 *tc6)
>> +{
>> +     kfree(tc6);
>> +}
>> +EXPORT_SYMBOL_GPL(oa_tc6_deinit);
> 
> Maybe consider a devm_ API to make the MAC driver simpler.
Sorry I don't get your point. Could you please explain bit more?

Best Regards,
Parthiban V

> 
>        Andrew
>

Parthiban Veerasooran Sept. 19, 2023, 11:40 a.m. UTC | #9

Hi Andrew,

On 13/09/23 7:06 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> +int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len,
>> +                     bool wnr, bool ctrl_prot)
> 
> Please add some kerneldoc headers for the API method exposed
> here. These are what the MAC driver should be using, so they should be
> reasonably well documented.
Sure, will do that.

Best Regards,
Parthiban V
> 
> Thanks
>             Andrew

Andrew Lunn Sept. 19, 2023, 12:58 p.m. UTC | #10

On Tue, Sep 19, 2023 at 11:13:13AM +0000, [e-mail] wrote:
> Hi Andrew,
> 
> On 13/09/23 7:46 am, Andrew Lunn wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> >> +struct oa_tc6 {
> >> +     struct spi_device *spi;
> >> +     bool ctrl_prot;
> >> +};
> > 
> > Should this be considered an opaque structure which the MAC driver
> > should not access the members?

Opaque vs not opaque is an important design decision. If the MAC
driver is allowed to directly access this structure, you should
document the locking concept. If the MAC is not supposed to access it
directly, only uses getters/setters, that also needs documenting, and
maybe even make it a void * in the MAC driver.

      Andrew

Andrew Lunn Sept. 19, 2023, 3:13 p.m. UTC | #11

> >> +static void oa_tc6_prepare_ctrl_buf(struct oa_tc6 *tc6, u32 addr, u32 val[],
> >> +                                 u8 len, bool wnr, u8 *buf, bool ctrl_prot)
> >> +{
> >> +     u32 hdr;
> >> +
> >> +     /* Prepare the control header with the required details */
> >> +     hdr = FIELD_PREP(CTRL_HDR_DNC, 0) |
> >> +           FIELD_PREP(CTRL_HDR_WNR, wnr) |
> >> +           FIELD_PREP(CTRL_HDR_AID, 0) |
> >> +           FIELD_PREP(CTRL_HDR_MMS, addr >> 16) |
> >> +           FIELD_PREP(CTRL_HDR_ADDR, addr) |
> >> +           FIELD_PREP(CTRL_HDR_LEN, len - 1);
> >> +     hdr |= FIELD_PREP(CTRL_HDR_P, oa_tc6_get_parity(hdr));
> >> +     *(u32 *)buf = cpu_to_be32(hdr);
> >> +
> >> +     if (wnr) {
> > 
> > What does wnr mean? Maybe give it a more meaningful name, unless it is
> > actually something in the standard. Kerneldoc would also help.
> Ah, it is "write not read". Shall I name it as "write_not_read" ?

You might want to describe the high level concept as well in this
file. What i _think_ this is about is that SPI is sort of a full
duplex bus. While you are sending data to the SPI device, the device
could also be sending a data to the CPU? And 'write not read' here
means ignore what we receive from the device?

> Ok, as per OA spec, up to 128 consecutive registers read or write can be 
> possible. So the maximum possible size would be 1032. As you suggested 
> will allocate this size of memory in the startup.

Yes, 1032 bytes it not huge, so allocate it once and keep it for the
lifetime of the device.

> >> +void oa_tc6_deinit(struct oa_tc6 *tc6)
> >> +{
> >> +     kfree(tc6);
> >> +}
> >> +EXPORT_SYMBOL_GPL(oa_tc6_deinit);
> > 
> > Maybe consider a devm_ API to make the MAC driver simpler.
> Sorry I don't get your point. Could you please explain bit more?

At least at this stage in the patch series, all you are doing is
allocating memory. You add more code later, which might invalidate my
point. But if all you are doing is allocating memory, you could use
devm_kmalloc(). The driver core will then take care of releasing the
memory when the driver is unloaded, or probe fails. That makes cleanup
simpler and memory leaks less likely. There are a lot of devm_
helpers, see if you can use them.

	Andrew

Parthiban Veerasooran Sept. 20, 2023, 12:40 p.m. UTC | #12

Hi Andrew,

On 19/09/23 8:43 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>>>> +static void oa_tc6_prepare_ctrl_buf(struct oa_tc6 *tc6, u32 addr, u32 val[],
>>>> +                                 u8 len, bool wnr, u8 *buf, bool ctrl_prot)
>>>> +{
>>>> +     u32 hdr;
>>>> +
>>>> +     /* Prepare the control header with the required details */
>>>> +     hdr = FIELD_PREP(CTRL_HDR_DNC, 0) |
>>>> +           FIELD_PREP(CTRL_HDR_WNR, wnr) |
>>>> +           FIELD_PREP(CTRL_HDR_AID, 0) |
>>>> +           FIELD_PREP(CTRL_HDR_MMS, addr >> 16) |
>>>> +           FIELD_PREP(CTRL_HDR_ADDR, addr) |
>>>> +           FIELD_PREP(CTRL_HDR_LEN, len - 1);
>>>> +     hdr |= FIELD_PREP(CTRL_HDR_P, oa_tc6_get_parity(hdr));
>>>> +     *(u32 *)buf = cpu_to_be32(hdr);
>>>> +
>>>> +     if (wnr) {
>>>
>>> What does wnr mean? Maybe give it a more meaningful name, unless it is
>>> actually something in the standard. Kerneldoc would also help.
>> Ah, it is "write not read". Shall I name it as "write_not_read" ?
> 
> You might want to describe the high level concept as well in this
> file. What i _think_ this is about is that SPI is sort of a full
> duplex bus. While you are sending data to the SPI device, the device
> could also be sending a data to the CPU? And 'write not read' here
> means ignore what we receive from the device?
Ah ok, I think there is a misunderstanding here. This is related to OPEN 
Alliance protocol. Control transactions consist of one or more control 
commands. Control commands are used by the SPI host to read and write 
registers within the MAC-PHY. Each control commands are composed of a 
32-bit control command header followed by register data. WNR (write not 
read) bit in the control command header indicates if data is to be 
written to registers (when set) or read from registers (when clear). so 
basically it indicates the type of the control command on the registers.
> 
>> Ok, as per OA spec, up to 128 consecutive registers read or write can be
>> possible. So the maximum possible size would be 1032. As you suggested
>> will allocate this size of memory in the startup.
> 
> Yes, 1032 bytes it not huge, so allocate it once and keep it for the
> lifetime of the device.
Sure, will do that.
> 
>>>> +void oa_tc6_deinit(struct oa_tc6 *tc6)
>>>> +{
>>>> +     kfree(tc6);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(oa_tc6_deinit);
>>>
>>> Maybe consider a devm_ API to make the MAC driver simpler.
>> Sorry I don't get your point. Could you please explain bit more?
> 
> At least at this stage in the patch series, all you are doing is
> allocating memory. You add more code later, which might invalidate my
> point. But if all you are doing is allocating memory, you could use
> devm_kmalloc(). The driver core will then take care of releasing the
> memory when the driver is unloaded, or probe fails. That makes cleanup
> simpler and memory leaks less likely. There are a lot of devm_
> helpers, see if you can use them.
Sure, noted.

Best Regards,
Parthiban V
> 
>          Andrew
>

Andrew Lunn Sept. 20, 2023, 1:37 p.m. UTC | #13

> Ah ok, I think there is a misunderstanding here. This is related to OPEN 
> Alliance protocol. Control transactions consist of one or more control 
> commands. Control commands are used by the SPI host to read and write 
> registers within the MAC-PHY. Each control commands are composed of a 
> 32-bit control command header followed by register data. WNR (write not 
> read) bit in the control command header indicates if data is to be 
> written to registers (when set) or read from registers (when clear). so 
> basically it indicates the type of the control command on the registers.

OK, so this clearly indicates the names are bad and documentation is
missing if i got this completely wrong. Adding kerneldoc to these
functions should help.

	Andrew

Parthiban Veerasooran Sept. 21, 2023, 9:15 a.m. UTC | #14

Hi Andrew,

On 20/09/23 7:07 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> Ah ok, I think there is a misunderstanding here. This is related to OPEN
>> Alliance protocol. Control transactions consist of one or more control
>> commands. Control commands are used by the SPI host to read and write
>> registers within the MAC-PHY. Each control commands are composed of a
>> 32-bit control command header followed by register data. WNR (write not
>> read) bit in the control command header indicates if data is to be
>> written to registers (when set) or read from registers (when clear). so
>> basically it indicates the type of the control command on the registers.
> 
> OK, so this clearly indicates the names are bad and documentation is
> missing if i got this completely wrong. Adding kerneldoc to these
> functions should help.
Sure, I will do that.
> 
>          Andrew
> 
>

Parthiban Veerasooran Sept. 21, 2023, 12:27 p.m. UTC | #15

Hi Andrew,

On 13/09/23 7:02 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> If I understand you correctly, this framework has to include the module
>> initialization as well using the below APIs and has to be compiled as a
>> loadable module so that other vendors module can make use of this, isn't it?
>>
>> module_init(oa_tc6_init);
>> module_exit(oa_tc6_exit);
> 
> You should not need these, unless there is actions which need to be
> taken when the module is loaded. If there are no actions, it is purely
> a library, don't have them. The module dependency tracking code will
> see that the MAC driver modules has dependencies on symbols in this
> library module, and will load it first. The MAC driver is then loaded,
> and the kernel linker will resolve the missing symbols in the MAC
> driver to those in the library. It also means that there is only ever
> one copy of the library in the kernel, even if there is multiple MAC
> drivers using it.
Ah ok. Actually I missed including this library in the Kconfig and Makefile.

So drivers/net/ethernet/Kconfig file should contain the below,

config OA_TC6
          tristate "OPEN Alliance TC6 10BASE-T1x MAC-PHY support"
          depends on SPI
          select PHYLIB 

          help
            This library implements OPEN Alliance TC6 10BASE-T1x MAC-PHY
            Serial Interface protocol for supporting 10BASE-T1x MAC-PHYs.

The drivers/net/ethernet/Makefile file should contain the below,

obj-$(CONFIG_OA_TC6) += oa_tc6.o

Is this you expected right?

Best Regards,
Parthiban V
> 
>         Andrew
>

Parthiban Veerasooran Sept. 21, 2023, 12:36 p.m. UTC | #16

Hi Andrew,

On 19/09/23 6:28 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, Sep 19, 2023 at 11:13:13AM +0000, [e-mail] wrote:
>> Hi Andrew,
>>
>> On 13/09/23 7:46 am, Andrew Lunn wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>>> +struct oa_tc6 {
>>>> +     struct spi_device *spi;
>>>> +     bool ctrl_prot;
>>>> +};
>>>
>>> Should this be considered an opaque structure which the MAC driver
>>> should not access the members?
> 
> Opaque vs not opaque is an important design decision. If the MAC
> driver is allowed to directly access this structure, you should
> document the locking concept. If the MAC is not supposed to access it
> directly, only uses getters/setters, that also needs documenting, and
> maybe even make it a void * in the MAC driver.
Sorry that I missed to reply in the previous email. Thanks for the 
details on this topic.

Yes, as "struct oa_tc6" and its members are not or going to be accessed 
in the MAC driver, I will consider this as an opaque structure and 
declare it as void *opaque_oa_tc6 in the MAC driver private structure 
"struct lan865x_priv" and will pass to the APIs exported from oa_tc6.c lib.

Is my understanding correct?

Best Regards,
Parthiban V
> 
>        Andrew

Andrew Lunn Sept. 21, 2023, 7:16 p.m. UTC | #17

> So drivers/net/ethernet/Kconfig file should contain the below,
> 
> config OA_TC6
>           tristate "OPEN Alliance TC6 10BASE-T1x MAC-PHY support"
>           depends on SPI
>           select PHYLIB 
> 
>           help
>             This library implements OPEN Alliance TC6 10BASE-T1x MAC-PHY
>             Serial Interface protocol for supporting 10BASE-T1x MAC-PHYs.
> 
> The drivers/net/ethernet/Makefile file should contain the below,
> 
> obj-$(CONFIG_OA_TC6) += oa_tc6.o

That looks about right, but i'm not a kconfig expert.

I would expect drivers using this to then have a

	depends on OA_TC6

	Andrew

Andrew Lunn Sept. 21, 2023, 7:19 p.m. UTC | #18

> Yes, as "struct oa_tc6" and its members are not or going to be accessed 
> in the MAC driver, I will consider this as an opaque structure and 
> declare it as void *opaque_oa_tc6 in the MAC driver private structure 
> "struct lan865x_priv" and will pass to the APIs exported from oa_tc6.c lib.
> 
> Is my understanding correct?

Yes.

If the structure is going to be truly opaque, i suggest having it in
the C file, not the H file.

	Andrew

Parthiban Veerasooran Sept. 22, 2023, 4:31 a.m. UTC | #19

Hi Andrew,

On 22/09/23 12:46 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> So drivers/net/ethernet/Kconfig file should contain the below,
>>
>> config OA_TC6
>>            tristate "OPEN Alliance TC6 10BASE-T1x MAC-PHY support"
>>            depends on SPI
>>            select PHYLIB
>>
>>            help
>>              This library implements OPEN Alliance TC6 10BASE-T1x MAC-PHY
>>              Serial Interface protocol for supporting 10BASE-T1x MAC-PHYs.
>>
>> The drivers/net/ethernet/Makefile file should contain the below,
>>
>> obj-$(CONFIG_OA_TC6) += oa_tc6.o
> 
> That looks about right, but i'm not a kconfig expert.
> 
> I would expect drivers using this to then have a
> 
>          depends on OA_TC6
Yes, that's for sure. The MAC driver (Ex: lan865x.c) Kconfig file will 
contain the above "depends on OA_TC6" if it supports OPEN Alliance TC6.

Best Regards,
Parthiban V
> 
>          Andrew

Parthiban Veerasooran Sept. 22, 2023, 4:39 a.m. UTC | #20

Hi Andrew,

On 22/09/23 12:49 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> Yes, as "struct oa_tc6" and its members are not or going to be accessed
>> in the MAC driver, I will consider this as an opaque structure and
>> declare it as void *opaque_oa_tc6 in the MAC driver private structure
>> "struct lan865x_priv" and will pass to the APIs exported from oa_tc6.c lib.
>>
>> Is my understanding correct?
> 
> Yes.
> 
> If the structure is going to be truly opaque, i suggest having it in
> the C file, not the H file.

Yes for sure, I will move it to oa_tc6.c file.

Andrew, thanks a lot for all your comments. They are all really helping 
me to know/learn many things to improve my patches and coding style. 
Please keep supporting.

Best Regards,
Parthiban V
> 
>          Andrew

Parthiban Veerasooran Sept. 26, 2023, 12:54 p.m. UTC | #21

Hi Andrew,

As I didn't receive any more comments for past couple days, shall I 
start preparing v2 patch series with comments already given?

Best Regards,
Parthiban V

-------- Forwarded Message --------
Subject: Re: [RFC PATCH net-next 1/6] net: ethernet: implement OPEN 
Alliance control transaction interface
Date: Fri, 22 Sep 2023 10:09:09 +0530
From: Parthiban Veerasooran <[e-mail]>
To: Andrew Lunn <[e-mail]>
CC: [e-mail], [e-mail], [e-mail], 
[e-mail], [e-mail], 
[e-mail], [e-mail], [e-mail], 
[e-mail], [e-mail], [e-mail], 
[e-mail], [e-mail], 
[e-mail], [e-mail], 
[e-mail], [e-mail], 
[e-mail], [e-mail], 
[e-mail], [e-mail]

Hi Andrew,

On 22/09/23 12:49 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> Yes, as "struct oa_tc6" and its members are not or going to be accessed
>> in the MAC driver, I will consider this as an opaque structure and
>> declare it as void *opaque_oa_tc6 in the MAC driver private structure
>> "struct lan865x_priv" and will pass to the APIs exported from oa_tc6.c lib.
>>
>> Is my understanding correct?
> 
> Yes.
> 
> If the structure is going to be truly opaque, i suggest having it in
> the C file, not the H file.

Yes for sure, I will move it to oa_tc6.c file.

Andrew, thanks a lot for all your comments. They are all really helping 
me to know/learn many things to improve my patches and coding style. 
Please keep supporting.

Best Regards,
Parthiban V
> 
>          Andrew

Patch

diff --git a/Documentation/networking/oa-tc6-framework.rst b/Documentation/networking/oa-tc6-framework.rst
new file mode 100644
index 000000000000..5a8af2398f3c
--- /dev/null
+++ b/Documentation/networking/oa-tc6-framework.rst
@@ -0,0 +1,231 @@ 
+.. SPDX-License-Identifier: GPL-2.0+
+
+=========================================================================
+OPEN Alliance 10BASE-T1x MAC-PHY Serial Interface (TC6) Framework Support
+=========================================================================
+
+:Copyright: |copy| 2023 MICROCHIP
+
+Introduction
+------------
+
+The IEEE 802.3cg project defines two 10 Mbit/s PHYs operating over a
+single pair of conductors. The 10BASE-T1L (Clause 146) is a long reach
+PHY supporting full duplex point-to-point operation over 1 km of single
+balanced pair of conductors. The 10BASE-T1S (Clause 147) is a short reach
+PHY supporting full / half duplex point-to-point operation over 15 m of
+single balanced pair of conductors, or half duplex multidrop bus
+operation over 25 m of single balanced pair of conductors.
+
+Furthermore, the IEEE 802.3cg project defines the new Physical Layer
+Collision Avoidance (PLCA) Reconciliation Sublayer (Clause 148) meant to
+provide improved determinism to the CSMA/CD media access method. PLCA
+works in conjunction with the 10BASE-T1S PHY operating in multidrop mode.
+
+The aforementioned PHYs are intended to cover the low-speed / low-cost
+applications in industrial and automotive environment. The large number
+of pins (16) required by the MII interface, which is specified by the
+IEEE 802.3 in Clause 22, is one of the major cost factors that need to be
+addressed to fulfil this objective.
+
+The MAC-PHY solution integrates an IEEE Clause 4 MAC and a 10BASE-T1x PHY
+exposing a low pin count Serial Peripheral Interface (SPI) to the host
+microcontroller. This also enables the addition of Ethernet functionality
+to existing low-end microcontrollers which do not integrate a MAC
+controller.
+
+Overview
+--------
+
+The MAC-PHY is specified to carry both data (Ethernet frames) and control
+(register access) transactions over a single full-duplex serial
+peripheral interface.
+
+Protocol Overview
+-----------------
+
+Two types of transactions are defined in the protocol: data transactions
+for Ethernet frame transfers and control transactions for register
+read/write transfers. A chunk is the basic element of data transactions
+and is composed of 4 bytes of overhead plus the configured payload size
+for each chunk. Ethernet frames are transferred over one or more data
+chunks. Control transactions consist of one or more register read/write
+control commands.
+
+SPI transactions are initiated by the SPI host with the assertion of CSn
+low to the MAC-PHY and ends with the deassertion of CSn high. In between
+each SPI transaction, the SPI host may need time for additional
+processing and to setup the next SPI data or control transaction.
+
+SPI data transactions consist of an equal number of transmit (TX) and
+receive (RX) chunks. Chunks in both transmit and receive directions may
+or may not contain valid frame data independent from each other, allowing
+for the simultaneous transmission and reception of different length
+frames.
+
+Each transmit data chunk begins with a 32-bit data header followed by a
+data chunk payload on MOSI. The data header indicates whether transmit
+frame data is present and provides the information to determine which
+bytes of the payload contain valid frame data.
+
+In parallel, receive data chunks are received on MISO. Each receive data
+chunk consists of a data chunk payload ending with a 32-bit data footer.
+The data footer indicates if there is receive frame data present within
+the payload or not and provides the information to determine which bytes
+of the payload contain valid frame data.
+
+Reference
+---------
+
+10BASE-T1x MAC-PHY Serial Interface Specification,
+
+Link: https://www.opensig.org/about/specifications/
+
+Hardware Architecture
+---------------------
+
+.. code-block:: none
+                    +-------------------------------------+
+                    |                MAC-PHY              |
+  +----------+      | +-----------+  +-------+  +-------+ |
+  | SPI Host |<---->| | SPI Slave |  |  MAC  |  |  PHY  | |
+  +----------+      | +-----------+  +-------+  +-------+ |
+                    +-------------------------------------+
+
+Software Architecture
+---------------------
+
+.. code-block:: none
+
+  +----------------------------------------------------------+
+  |                 Networking Subsystem                     |
+  +----------------------------------------------------------+
+             |                               |
+             |                               |
+  +----------------------+     +-----------------------------+
+  |     MAC Driver       |<--->| OPEN Alliance TC6 Framework |
+  +----------------------+     +-----------------------------+
+             |                               |
+             |                               |
+  +----------------------+     +-----------------------------+
+  |      PHYLIB          |     |       SPI Subsystem         |
+  +----------------------+     +-----------------------------+
+                                             |
+                                             |
+  +----------------------------------------------------------+
+  |                10BASE-T1x MAC-PHY Device                 |
+  +----------------------------------------------------------+
+
+Implementation
+--------------
+
+MAC Driver
+~~~~~~~~~~
+- Initializes and configures the OA TC6 framework for the MAC-PHY.
+
+- Initializes PHYLIB interface.
+
+- Registers and configures the network device.
+
+- Sends the tx ethernet frame from n/w subsystem to OA TC6 framework.
+
+OPEN Alliance TC6 Framework
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
+- Registers macphy interrupt which is used to indicate receive data
+  available, any communication errors and tx credit count availability in
+  case it was 0 already.
+
+- Prepares SPI chunks from the tx ethernet frame enqueued by the MAC
+  driver and sends to MAC-PHY.
+
+- Receives SPI chunks from MAC-PHY and prepares ethernet frame and sends
+  to n/w subsystem.
+
+- Prepares and performs control read/write commands.
+
+Data Transaction
+~~~~~~~~~~~~~~~~
+
+Tx ethernet frame from the n/w layer is divided into multiple chunks in
+the oa_tc6_prepare_tx_chunks() function. Each tx chunk will have 4 bytes
+header and 64/32 bytes chunk payload.
+
+.. code-block:: none
+
+  +---------------------------------------------------+
+  |                     Tx Chunk                      |
+  | +---------------------------+  +----------------+ |   MOSI
+  | | 64/32 bytes chunk payload |  | 4 bytes header | |------------>
+  | +---------------------------+  +----------------+ |
+  +---------------------------------------------------+
+
+The number of buffers available in the MAC-PHY to store the incoming tx
+chunk payloads is represented as tx credit count (txc). This txc can be
+read either from the Buffer Status Register or footer (Refer below in the
+rx case for the footer info) received from the MAC-PHY. The number of txc
+is needed to transport the ethernet frame is calculated from the size of
+the ethernet frame. The header in the each chunk is updated with the
+chunk payload details like data not control, data valid, start valid,
+start word offset, end byte offset, end valid and parity bit.
+
+Once the tx chunks are ready, oa_tc6_handler() task is triggered to
+perform SPI transfer. This task checks for the txc availability in the
+MAC-PHY and sends the number of chunks equal to the txc. If there is no
+txc is available then the task waits for the interrupt to indicate the
+txc availability. If the available txc is less than the needed txc then
+the SPI transfer is performed for the available txc and the rx chunks
+footer is processed for the txc availability again. For every SPI
+transfer the received rx chunks will be processed for the rx ethernet
+frame (if any), txc and rca.
+
+Rx ethernet frame from the MAC-PHY is framed from the rx chunks received
+from the MAC-PHY and will be transferred to the n/w layer. Each rx chunk
+will have 64/32 bytes chunk payload and 4 bytes footer.
+
+.. code-block:: none
+
+  +---------------------------------------------------+
+  |                     Rx Chunk                      |
+  | +----------------+  +---------------------------+ |   MISO
+  | | 4 bytes footer |  | 64/32 bytes chunk payload | |------------>
+  | +----------------+  +---------------------------+ |
+  +---------------------------------------------------+
+
+In case of interrupt, the oa_tc6_handler() task performs an empty chunk
+SPI transfer to get the reason for the interrupt in the received chunk
+footer. The reason can be receive chunk available (rca) or extended block
+status (exst) or txc availability. Based on this the corresponding
+operation is performed. If it is for rca then the SPI transfer is
+performed with the empty chunks equal to the rca to get the rx chunks.
+Rx ethernet frame is framed from the rx chunks received and transferred
+to n/w layer. If it is for exst then the STATUS0 register will be read
+for the error detail.
+
+In the beginning the interrupt occurs for indicating the reset complete
+from the MAC-PHY and is ready for the configuration. oa_tc6_handler() task
+handles this interrupt to get and clear the reset complete status.
+
+Control Transaction
+~~~~~~~~~~~~~~~~~~~
+
+Control transactions are performed to read/write the registers in the
+MAC-PHY. Each control command headers are 4 bytes length with the
+necessary details to read/write the registers.
+
+In case of a register write command, the write command header has the
+information like data not control, write not read, address increment
+disable, memory map selector, address, length and parity followed by the
+data to be written. If the protected mode is enabled in the CONFIG0
+register then each data to be written will be sent first followed by its
+ones' complement value to ensure the error free transfer. For every write
+command, both the write command header and the data will be echoed back in
+the rx path to confirm the error free transaction.
+
+In case of a register read command, the read command is preferred with the
+above mentioned details and the echoed rx data will be processed for the
+register data. In case of protected mode enabled the echoed rx data
+contains the ones' complemented data also for verifying the data error.
+
+oa_tc6_perform_ctrl() function prepares control commands based on the
+read/write request, performs SPI transfer, checks for the error and
+returns read register data in case of control read command.
diff --git a/MAINTAINERS b/MAINTAINERS
index 9cc15c50c2c6..c54454c7e7a1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15835,6 +15835,14 @@  L:	[e-mail]
 S:	Supported
 F:	drivers/infiniband/ulp/opa_vnic
 
+OPEN ALLIANCE 10BASE-T1S MACPHY SERIAL INTERFACE FRAMEWORK
+M:	Parthiban Veerasooran <[e-mail]>
+L:	[e-mail]
+S:	Maintained
+F:	Documentation/networking/oa-tc6-framework.rst
+F:	drivers/include/linux/oa_tc6.h
+F:	drivers/net/ethernet/oa_tc6.c
+
 OPEN FIRMWARE AND FLATTENED DEVICE TREE
 M:	Rob Herring <[e-mail]>
 M:	Frank Rowand <[e-mail]>
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
new file mode 100644
index 000000000000..613cf034430a
--- /dev/null
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -0,0 +1,222 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * OPEN Alliance 10BASE‑T1x MAC‑PHY Serial Interface framework
+ *
+ * Author: Parthiban Veerasooran <[e-mail]>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/oa_tc6.h>
+
+static int oa_tc6_spi_transfer(struct spi_device *spi, u8 *ptx, u8 *prx,
+			       u16 len)
+{
+	struct spi_transfer xfer = {
+		.tx_buf = ptx,
+		.rx_buf = prx,
+		.len = len,
+	};
+	struct spi_message msg;
+
+	spi_message_init(&msg);
+	spi_message_add_tail(&xfer, &msg);
+
+	return spi_sync(spi, &msg);
+}
+
+static bool oa_tc6_get_parity(u32 p)
+{
+	bool parity = true;
+
+	/* This function returns an odd parity bit */
+	while (p) {
+		parity = !parity;
+		p = p & (p - 1);
+	}
+	return parity;
+}
+
+static void oa_tc6_prepare_ctrl_buf(struct oa_tc6 *tc6, u32 addr, u32 val[],
+				    u8 len, bool wnr, u8 *buf, bool ctrl_prot)
+{
+	u32 hdr;
+
+	/* Prepare the control header with the required details */
+	hdr = FIELD_PREP(CTRL_HDR_DNC, 0) |
+	      FIELD_PREP(CTRL_HDR_WNR, wnr) |
+	      FIELD_PREP(CTRL_HDR_AID, 0) |
+	      FIELD_PREP(CTRL_HDR_MMS, addr >> 16) |
+	      FIELD_PREP(CTRL_HDR_ADDR, addr) |
+	      FIELD_PREP(CTRL_HDR_LEN, len - 1);
+	hdr |= FIELD_PREP(CTRL_HDR_P, oa_tc6_get_parity(hdr));
+	*(u32 *)buf = cpu_to_be32(hdr);
+
+	if (wnr) {
+		for (u8 i = 0; i < len; i++) {
+			u16 pos;
+
+			if (!ctrl_prot) {
+				/* Send the value to be written followed by the
+				 * header.
+				 */
+				pos = (i + 1) * TC6_HDR_SIZE;
+				*(u32 *)&buf[pos] = cpu_to_be32(val[i]);
+			} else {
+				/* If protected then send complemented value
+				 * also followed by actual value.
+				 */
+				pos = TC6_HDR_SIZE + (i * (TC6_HDR_SIZE * 2));
+				*(u32 *)&buf[pos] = cpu_to_be32(val[i]);
+				pos = (i + 1) * (TC6_HDR_SIZE * 2);
+				*(u32 *)&buf[pos] = cpu_to_be32(~val[i]);
+			}
+		}
+	}
+}
+
+static int oa_tc6_check_control(struct oa_tc6 *tc6, u8 *ptx, u8 *prx, u8 len,
+				bool wnr, bool ctrl_prot)
+{
+	/* 1st 4 bytes of rx chunk data can be discarded */
+	u32 rx_hdr = *(u32 *)&prx[TC6_HDR_SIZE];
+	u32 tx_hdr = *(u32 *)ptx;
+	u32 rx_data_complement;
+	u32 tx_data;
+	u32 rx_data;
+	u16 pos1;
+	u16 pos2;
+
+	/* If tx hdr and echoed hdr are not equal then there might be an issue
+	 * with the connection between SPI host and MAC-PHY. Here this case is
+	 * considered as MAC-PHY is not connected.
+	 */
+	if (tx_hdr != rx_hdr)
+		return -ENODEV;
+
+	if (wnr) {
+		if (!ctrl_prot) {
+			/* In case of ctrl write, both tx data & echoed
+			 * data are compared for the error.
+			 */
+			pos1 = TC6_HDR_SIZE;
+			pos2 = TC6_HDR_SIZE * 2;
+			for (u8 i = 0; i < len; i++) {
+				tx_data = *(u32 *)&ptx[pos1 + (i * TC6_HDR_SIZE)];
+				rx_data = *(u32 *)&prx[pos2 + (i * TC6_HDR_SIZE)];
+				if (tx_data != rx_data)
+					return -ENODEV;
+			}
+			return 0;
+		}
+	} else {
+		if (!ctrl_prot)
+			return 0;
+	}
+
+	/* In case of ctrl read or ctrl write in protected mode, the rx data and
+	 * the complement of rx data are compared for the error.
+	 */
+	pos1 = TC6_HDR_SIZE * 2;
+	pos2 = TC6_HDR_SIZE * 3;
+	for (u8 i = 0; i < len; i++) {
+		rx_data = *(u32 *)&prx[pos1 + (i * TC6_HDR_SIZE * 2)];
+		rx_data_complement = *(u32 *)&prx[pos2 + (i * TC6_HDR_SIZE * 2)];
+		if (rx_data != ~rx_data_complement)
+			return -ENODEV;
+	}
+
+	return 0;
+}
+
+int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len,
+			bool wnr, bool ctrl_prot)
+{
+	u8 *tx_buf;
+	u8 *rx_buf;
+	u16 size;
+	u16 pos;
+	int ret;
+
+	if (ctrl_prot)
+		size = (TC6_HDR_SIZE * 2) + (len * (TC6_HDR_SIZE * 2));
+	else
+		size = (TC6_HDR_SIZE * 2) + (len * TC6_HDR_SIZE);
+
+	tx_buf = kzalloc(size, GFP_KERNEL);
+	if (!tx_buf)
+		return -ENOMEM;
+
+	rx_buf = kzalloc(size, GFP_KERNEL);
+	if (!rx_buf) {
+		ret = -ENOMEM;
+		goto err_rx_buf_kzalloc;
+	}
+
+	/* Prepare control command */
+	oa_tc6_prepare_ctrl_buf(tc6, addr, val, len, wnr, tx_buf, ctrl_prot);
+
+	/* Perform SPI transfer */
+	ret = oa_tc6_spi_transfer(tc6->spi, tx_buf, rx_buf, size);
+	if (ret)
+		goto err_ctrl;
+
+	/* Check echoed/received control reply */
+	ret = oa_tc6_check_control(tc6, tx_buf, rx_buf, len, wnr, ctrl_prot);
+	if (ret)
+		goto err_ctrl;
+
+	if (!wnr) {
+		/* Copy read data from the rx data in case of ctrl read */
+		for (u8 i = 0; i < len; i++) {
+			if (!ctrl_prot) {
+				pos = (TC6_HDR_SIZE * 2) + (i * TC6_HDR_SIZE);
+				val[i] = be32_to_cpu(*(u32 *)&rx_buf[pos]);
+			} else {
+				pos = (TC6_HDR_SIZE * 2) +
+				       (i * (TC6_HDR_SIZE * 2));
+				val[i] = be32_to_cpu(*(u32 *)&rx_buf[pos]);
+			}
+		}
+	}
+
+err_ctrl:
+	kfree(rx_buf);
+err_rx_buf_kzalloc:
+	kfree(tx_buf);
+	return ret;
+}
+
+int oa_tc6_write_register(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len)
+{
+	return oa_tc6_perform_ctrl(tc6, addr, val, len, true, tc6->ctrl_prot);
+}
+EXPORT_SYMBOL_GPL(oa_tc6_write_register);
+
+int oa_tc6_read_register(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len)
+{
+	return oa_tc6_perform_ctrl(tc6, addr, val, len, false, tc6->ctrl_prot);
+}
+EXPORT_SYMBOL_GPL(oa_tc6_read_register);
+
+struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
+{
+	struct oa_tc6 *tc6;
+
+	if (!spi)
+		return NULL;
+
+	tc6 = kzalloc(sizeof(*tc6), GFP_KERNEL);
+	if (!tc6)
+		return NULL;
+
+	tc6->spi = spi;
+
+	return tc6;
+}
+EXPORT_SYMBOL_GPL(oa_tc6_init);
+
+void oa_tc6_deinit(struct oa_tc6 *tc6)
+{
+	kfree(tc6);
+}
+EXPORT_SYMBOL_GPL(oa_tc6_deinit);
diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h
new file mode 100644
index 000000000000..5e0a58ab1dcd
--- /dev/null
+++ b/include/linux/oa_tc6.h
@@ -0,0 +1,31 @@ 
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * OPEN Alliance 10BASE‑T1x MAC‑PHY Serial Interface framework
+ *
+ * Author: Parthiban Veerasooran <[e-mail]>
+ */
+
+#include <linux/spi/spi.h>
+
+/* Control header */
+#define CTRL_HDR_DNC	BIT(31)		/* Data-Not-Control */
+#define CTRL_HDR_HDRB	BIT(30)		/* Received Header Bad */
+#define CTRL_HDR_WNR	BIT(29)		/* Write-Not-Read */
+#define CTRL_HDR_AID	BIT(28)		/* Address Increment Disable */
+#define CTRL_HDR_MMS	GENMASK(27, 24)	/* Memory Map Selector */
+#define CTRL_HDR_ADDR	GENMASK(23, 8)	/* Address */
+#define CTRL_HDR_LEN	GENMASK(7, 1)	/* Length */
+#define CTRL_HDR_P	BIT(0)		/* Parity Bit */
+
+#define TC6_HDR_SIZE	4		/* Ctrl command header size as per OA */
+#define TC6_FTR_SIZE	4		/* Ctrl command footer size ss per OA */
+
+struct oa_tc6 {
+	struct spi_device *spi;
+	bool ctrl_prot;
+};
+
+struct oa_tc6 *oa_tc6_init(struct spi_device *spi);
+void 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);