Implement register read operation according to the control communication
specified in the OPEN Alliance 10BASE-T1x MACPHY Serial Interface
document. Control read commands are used by the SPI host to read
registers within the MAC-PHY. Each control read commands are composed of
a 32 bits control command header.
The MAC-PHY ignores all data from the SPI host following the control
header for the remainder of the control read command. 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. Reading any unimplemented
or undefined registers shall return zero.
Signed-off-by: Parthiban Veerasooran <[email protected]>
---
drivers/net/ethernet/oa_tc6.c | 84 ++++++++++++++++++++++++++++++++++-
include/linux/oa_tc6.h | 3 ++
2 files changed, 85 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index 82131f865fe7..35e377577ba4 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -38,6 +38,7 @@ enum oa_tc6_header_type {
};
enum oa_tc6_register_op {
+ OA_TC6_CTRL_REG_READ = 0,
OA_TC6_CTRL_REG_WRITE = 1,
};
@@ -113,7 +114,8 @@ static void oa_tc6_prepare_ctrl_spi_buf(struct oa_tc6 *tc6, u32 address,
*tx_buf = oa_tc6_prepare_ctrl_header(address, length, reg_op);
- oa_tc6_update_ctrl_write_data(tc6, value, length);
+ if (reg_op == OA_TC6_CTRL_REG_WRITE)
+ oa_tc6_update_ctrl_write_data(tc6, value, length);
}
static int oa_tc6_check_ctrl_write_reply(struct oa_tc6 *tc6, u8 size)
@@ -132,6 +134,30 @@ static int oa_tc6_check_ctrl_write_reply(struct oa_tc6 *tc6, u8 size)
return 0;
}
+static int oa_tc6_check_ctrl_read_reply(struct oa_tc6 *tc6, u8 size)
+{
+ u32 *tx_buf = tc6->spi_ctrl_tx_buf;
+ u32 *rx_buf = tc6->spi_ctrl_rx_buf + OA_TC6_CTRL_IGNORED_SIZE;
+
+ /* The echoed control read header must match with the one that was
+ * transmitted.
+ */
+ if (*tx_buf != *rx_buf)
+ return -ENODEV;
+
+ return 0;
+}
+
+static void oa_tc6_copy_ctrl_read_data(struct oa_tc6 *tc6, u32 value[],
+ u8 length)
+{
+ __be32 *rx_buf = tc6->spi_ctrl_rx_buf + OA_TC6_CTRL_IGNORED_SIZE +
+ OA_TC6_CTRL_HEADER_SIZE;
+
+ for (int i = 0; i < length; i++)
+ value[i] = be32_to_cpu(*rx_buf++);
+}
+
static int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 address, u32 value[],
u8 length, enum oa_tc6_register_op reg_op)
{
@@ -152,8 +178,62 @@ static int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 address, u32 value[],
}
/* Check echoed/received control write command reply for errors */
- return oa_tc6_check_ctrl_write_reply(tc6, size);
+ if (reg_op == OA_TC6_CTRL_REG_WRITE)
+ return oa_tc6_check_ctrl_write_reply(tc6, size);
+
+ /* Check echoed/received control read command reply for errors */
+ ret = oa_tc6_check_ctrl_read_reply(tc6, size);
+ if (ret)
+ return ret;
+
+ oa_tc6_copy_ctrl_read_data(tc6, value, length);
+
+ return 0;
+}
+
+/**
+ * oa_tc6_read_registers - function for reading multiple consecutive registers.
+ * @tc6: oa_tc6 struct.
+ * @address: address of the first register to be read in the MAC-PHY.
+ * @value: values to be read from the starting register address @address.
+ * @length: number of consecutive registers to be read from @address.
+ *
+ * Maximum of 128 consecutive registers can be read starting at @address.
+ *
+ * Returns 0 on success otherwise failed.
+ */
+int oa_tc6_read_registers(struct oa_tc6 *tc6, u32 address, u32 value[],
+ u8 length)
+{
+ int ret;
+
+ if (!length || length > OA_TC6_CTRL_MAX_REGISTERS) {
+ dev_err(&tc6->spi->dev, "Invalid register length parameter\n");
+ return -EINVAL;
+ }
+
+ mutex_lock(&tc6->spi_ctrl_lock);
+ ret = oa_tc6_perform_ctrl(tc6, address, value, length,
+ OA_TC6_CTRL_REG_READ);
+ mutex_unlock(&tc6->spi_ctrl_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(oa_tc6_read_registers);
+
+/**
+ * oa_tc6_read_register - function for reading a MAC-PHY register.
+ * @tc6: oa_tc6 struct.
+ * @address: register address of the MAC-PHY to be read.
+ * @value: value read from the @address register address of the MAC-PHY.
+ *
+ * Returns 0 on success otherwise failed.
+ */
+int oa_tc6_read_register(struct oa_tc6 *tc6, u32 address, u32 *value)
+{
+ return oa_tc6_read_registers(tc6, address, value, 1);
}
+EXPORT_SYMBOL_GPL(oa_tc6_read_register);
/**
* oa_tc6_write_registers - function for writing multiple consecutive registers.
diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h
index 99c490f1c8a8..85aeecf87306 100644
--- a/include/linux/oa_tc6.h
+++ b/include/linux/oa_tc6.h
@@ -15,3 +15,6 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi);
int oa_tc6_write_register(struct oa_tc6 *tc6, u32 address, u32 value);
int oa_tc6_write_registers(struct oa_tc6 *tc6, u32 address, u32 value[],
u8 length);
+int oa_tc6_read_register(struct oa_tc6 *tc6, u32 address, u32 *value);
+int oa_tc6_read_registers(struct oa_tc6 *tc6, u32 address, u32 value[],
+ u8 length);
--
2.34.1
> enum oa_tc6_register_op {
> + OA_TC6_CTRL_REG_READ = 0,
> OA_TC6_CTRL_REG_WRITE = 1,
> };
I thought it looked a little odd when the enum was added in the
previous patch with the first value of 1, and only one value. Now it
makes more sense.
The actual value appears to not matter? It is always
> + if (reg_op == OA_TC6_CTRL_REG_WRITE)
So i would drop the numbering, and leave it to the compiler. The
patches will then look less odd.
> +static int oa_tc6_check_ctrl_read_reply(struct oa_tc6 *tc6, u8 size)
> +{
> + u32 *tx_buf = tc6->spi_ctrl_tx_buf;
> + u32 *rx_buf = tc6->spi_ctrl_rx_buf + OA_TC6_CTRL_IGNORED_SIZE;
> +
> + /* The echoed control read header must match with the one that was
> + * transmitted.
> + */
> + if (*tx_buf != *rx_buf)
> + return -ENODEV;
Another case where -EPROTO might be better?
Andrew
Hi Andrew,
On 07/03/24 5:49 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> enum oa_tc6_register_op {
>> + OA_TC6_CTRL_REG_READ = 0,
>> OA_TC6_CTRL_REG_WRITE = 1,
>> };
>
> I thought it looked a little odd when the enum was added in the
> previous patch with the first value of 1, and only one value. Now it
> makes more sense.
Ok.
>
> The actual value appears to not matter? It is always
>
>> + if (reg_op == OA_TC6_CTRL_REG_WRITE)
>
> So i would drop the numbering, and leave it to the compiler. The
> patches will then look less odd.
"drop the numbering", do you refer to this patch alone or previous patch
also? If it is for this patch alone then it makes sense as they are
going to be 0 and 1 anyway. But if we drop the numbering in the previous
patch it will become 0 which will create an issue in the below line as
it needs 1,
FIELD_PREP(OA_TC6_CTRL_HEADER_WRITE, reg_op)
Could you clarify? or do I have any misunderstanding here?
>
>> +static int oa_tc6_check_ctrl_read_reply(struct oa_tc6 *tc6, u8 size)
>> +{
>> + u32 *tx_buf = tc6->spi_ctrl_tx_buf;
>> + u32 *rx_buf = tc6->spi_ctrl_rx_buf + OA_TC6_CTRL_IGNORED_SIZE;
>> +
>> + /* The echoed control read header must match with the one that was
>> + * transmitted.
>> + */
>> + if (*tx_buf != *rx_buf)
>> + return -ENODEV;
>
> Another case where -EPROTO might be better?
Yes, I will use -EPROTO in the next version as it is likely "Protocol error"
Best regards,
Parthiban V
>
> Andrew
>
On Thu, Mar 07, 2024 at 07:04:20AM +0000, [email protected] wrote:
> Hi Andrew,
>
> On 07/03/24 5:49 am, Andrew Lunn wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> >> enum oa_tc6_register_op {
> >> + OA_TC6_CTRL_REG_READ = 0,
> >> OA_TC6_CTRL_REG_WRITE = 1,
> >> };
> >
> > I thought it looked a little odd when the enum was added in the
> > previous patch with the first value of 1, and only one value. Now it
> > makes more sense.
> Ok.
> >
> > The actual value appears to not matter? It is always
> >
> >> + if (reg_op == OA_TC6_CTRL_REG_WRITE)
> >
> > So i would drop the numbering, and leave it to the compiler. The
> > patches will then look less odd.
> "drop the numbering", do you refer to this patch alone or previous patch
> also? If it is for this patch alone then it makes sense as they are
> going to be 0 and 1 anyway. But if we drop the numbering in the previous
> patch it will become 0 which will create an issue in the below line as
> it needs 1,
>
> FIELD_PREP(OA_TC6_CTRL_HEADER_WRITE, reg_op)
That is why i asked:
> The actual value appears to not matter? It is always
>
> + if (reg_op == OA_TC6_CTRL_REG_WRITE)
So the actual value does matter, so keep it in the previous patch.
Does the value of OA_TC6_CTRL_REG_READ matter? Is it also used in
FIELD_PREP etc? If not, taking away the = 0 will emphasise that
OA_TC6_CTRL_REG_WRITE has to be 1.
Andrew
Hi Andrew,
On 07/03/24 6:52 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Thu, Mar 07, 2024 at 07:04:20AM +0000, [email protected] wrote:
>> Hi Andrew,
>>
>> On 07/03/24 5:49 am, Andrew Lunn wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>>> enum oa_tc6_register_op {
>>>> + OA_TC6_CTRL_REG_READ = 0,
>>>> OA_TC6_CTRL_REG_WRITE = 1,
>>>> };
>>>
>>> I thought it looked a little odd when the enum was added in the
>>> previous patch with the first value of 1, and only one value. Now it
>>> makes more sense.
>> Ok.
>>>
>>> The actual value appears to not matter? It is always
>>>
>>>> + if (reg_op == OA_TC6_CTRL_REG_WRITE)
>>>
>>> So i would drop the numbering, and leave it to the compiler. The
>>> patches will then look less odd.
>> "drop the numbering", do you refer to this patch alone or previous patch
>> also? If it is for this patch alone then it makes sense as they are
>> going to be 0 and 1 anyway. But if we drop the numbering in the previous
>> patch it will become 0 which will create an issue in the below line as
>> it needs 1,
>>
>> FIELD_PREP(OA_TC6_CTRL_HEADER_WRITE, reg_op)
>
> That is why i asked:
>
>> The actual value appears to not matter? It is always
>>
>> + if (reg_op == OA_TC6_CTRL_REG_WRITE)
>
> So the actual value does matter, so keep it in the previous patch.
> Does the value of OA_TC6_CTRL_REG_READ matter? Is it also used in
> FIELD_PREP etc? If not, taking away the = 0 will emphasise that
> OA_TC6_CTRL_REG_WRITE has to be 1.
Sorry, I have done a mistake here that's confusing. The define name
OA_TC6_CTRL_HEADER_WRITE in the FIELD_PREP is supposed to be
OA_TC6_CTRL_HEADER_WRITE_NOT_READ. This bit field in the control command
header differentiates the type of operation. If it is 0, then register
read command and if it is 1, then register write command. So regop in
the FIELD_PREP actually sets the type of operation.
The values of both OA_TC6_CTRL_REG_READ and OA_TC6_CTRL_REG_WRITE are
matters here. So let's keep the numbering for both as it is now. But I
will change the bit field define name as
OA_TC6_CTRL_HEADER_WRITE_NOT_READ in the next version. Hope you are fine
with it?
Best regards,
Parthiban V
>
> Andrew
>