Skip to content

fix for https://github.com/arduino/ArduinoCore-renesas/issues/12 #31

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

fix for https://github.com/arduino/ArduinoCore-renesas/issues/12 #31

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 40 additions & 9 deletions libraries/SPI/SPI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,17 @@ void ArduinoSPI::configSpiSettings(arduino::SPISettings const & settings)
configSpi(settings);
}


#define ONE_BIT 1
#define TWO_BITS 3

#define CPHA_POS 0
#define CPOL_POS 1
#define BRDV_POS 2

#define BIT_ORDER_POS 12


void ArduinoSPI::configSpi(arduino::SPISettings const & settings)
{
auto [clk_phase, clk_polarity, bit_order] = toFspSpiConfig(settings);
Expand All @@ -374,23 +385,35 @@ void ArduinoSPI::configSpi(arduino::SPISettings const & settings)
uint32_t spcmd0 = _spi_ctrl.p_regs->SPCMD[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be clearer to just modify the register in-place to make use of Renesas' struct definitions, so all of the masking and bit shifting would be handled implicitly. Like so:

auto& spcmd0 = _spi_ctrl.p_regs->SPCMD_b[0]; // note SPCMD_b not SPCMD
spcmd0.CPHA = clk_phase;
spcmd0.CPOL = clk_polarity;
spcmd0.LSBF = bit_order;
spcmd0.BRDV = spck_div.brdv;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See definition here


/* Configure CPHA setting. */
spcmd0 |= (uint32_t) clk_phase;
uint32_t mask = (ONE_BIT << CPHA_POS);
spcmd0 &= ~(uint32_t)mask;
spcmd0 |= (clk_phase << CPHA_POS);

/* Configure CPOL setting. */
spcmd0 |= (uint32_t) clk_polarity << 1;
mask = (ONE_BIT << CPOL_POS);
spcmd0 &= ~(uint32_t)mask;
spcmd0 |= ((uint32_t) clk_polarity << CPOL_POS);

/* Configure Bit Order (MSB,LSB) */
spcmd0 |= (uint32_t) bit_order << 12;
mask = (ONE_BIT << BIT_ORDER_POS);
spcmd0 &= ~(uint32_t)mask;
spcmd0 |= ((uint32_t) bit_order << BIT_ORDER_POS);

/* Configure the Bit Rate Division Setting */
spcmd0 &= ~(((uint32_t) 3) << 2);
spcmd0 |= (uint32_t) spck_div.brdv << 2;
mask = (TWO_BITS << BRDV_POS);
spcmd0 &= ~(uint32_t)mask;
spcmd0 |= ((uint32_t) spck_div.brdv << BRDV_POS);

/* Update settings. */
_spi_ctrl.p_regs->SPCMD[0] = (uint16_t) spcmd0;
_spi_ctrl.p_regs->SPBR = (uint8_t) spck_div.spbr;
}

#define SCI_CKPH_POS 7
#define SCI_CKPOL_POS 6
#define SCI_DIRECTION_POS 3
#define SCI_CK_SEL_POS 0

void ArduinoSPI::configSpiSci(arduino::SPISettings const & settings)
{
auto [clk_phase, clk_polarity, bit_order] = toFspSpiConfig(settings);
Expand All @@ -403,16 +426,24 @@ void ArduinoSPI::configSpiSci(arduino::SPISettings const & settings)
uint32_t smr = R_SCI0_SMR_CM_Msk;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, maybe this could be:

auto& spmr = _spi_sci_ctrl.p_reg->SPMR_b;
spmr.CKPH = clk_phase;
spmr.CKPOL = clk_polarity;

auto& scmr = _spi_sci_ctrl.p_reg->SCMR_b;
scmr.SDIR = bit_order;

auto& smr = _spi_sci_ctrl.p_reg->SMR_b;
smr.CKS = clk_div.cks;


/* Configure CPHA setting. */
spmr |= (uint32_t) clk_phase << 7;
uint32_t mask = (ONE_BIT << SCI_CKPH_POS);
spmr &= ~(uint32_t)mask; // reset bit 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment // reset bit 0 true? It looks to me like its copy and paste from the baud rate generator clock divider configuration.

spmr |= ((uint32_t) clk_phase << SCI_CKPH_POS);

/* Configure CPOL setting. */
spmr |= (uint32_t) clk_polarity << 6;
mask = (ONE_BIT << SCI_CKPOL_POS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really happy with the generic ONE_BIT, TWO__BITS defines. Maybe consider using the FSP layer provided constants, i.e.

-mask = (ONE_BIT << SCI_CKPOL_POS);
+mask = (SPI_CLK_POLARITY_HIGH << SCI_CKPOL_POS);

? This one's a matter of taste though.

spmr &= ~(uint32_t)mask; // reset bit 0
spmr |= ((uint32_t) clk_polarity << SCI_CKPOL_POS);

/* Configure Bit Order (MSB,LSB) */
scmr |= (uint32_t) bit_order << 3;
mask = (ONE_BIT << SCI_DIRECTION_POS);
scmr &= ~(uint32_t)mask; // reset bit 0
scmr |= ((uint32_t) bit_order << SCI_DIRECTION_POS);

/* Select the baud rate generator clock divider. */
smr |= (uint32_t) clk_div.cks;
mask = (TWO_BITS << SCI_CK_SEL_POS);
smr &= ~(uint32_t)mask; // reset bit 0
smr |= ((uint32_t) clk_div.cks << SCI_CK_SEL_POS);

/* Update settings. */
_spi_sci_ctrl.p_reg->SMR = (uint8_t) smr;
Expand Down