Get the same output as S21 #4
Conversation
| @@ -596,6 +596,9 @@ impl Asic for BM1370 { | |||
| }) | |||
| } | |||
| 2 => { | |||
| // self.seq_step = SequenceStep::None; | |||
There was a problem hiding this comment.
clean this up for merging
| // ) | ||
| // .set_ccdly(0) | ||
| // .set_pwth(2) | ||
| // // .disable_bit2() |
There was a problem hiding this comment.
if we can't obtain a raw value of 0x10 with the builder, this means there is some field we don't know about. For exemple this disable_bit2() may be doing the trick, did you try it ?
| @@ -705,7 +708,7 @@ impl Asic for BM1370 { | |||
| domain_asic_cnt: u8, | |||
| asic_addr_interval: u16, | |||
| ) -> Option<CmdDelay> { | |||
| let sub_seq1_start = 0; | |||
| let sub_seq1_start = 1; | |||
There was a problem hiding this comment.
why do you want to start at 1 ? all other steps are relative to this first one so it should work the same, but usually we start counting at 0 not 1. Is it not working with a 0 ?
| // .set_post1_div(1) | ||
| // .set_post2_div(1) | ||
| .set_out_div(BM1370_PLL_OUT_UART, pll3_div4); | ||
| self.plls[BM1370_PLL_ID_UART].set_parameter(0x5aa5_5aa5); // TODO: replace these fixed values with equivalent individual ones below |
There was a problem hiding this comment.
this change should be unecessary, it is totally equivalent
right ?
| let pll3_param = self.plls[BM1370_PLL_ID_UART].parameter(); | ||
| let pll3_param = 0x5aa5_5aa5; // TODO: replace these fixed values with equivalent individual ones below |
There was a problem hiding this comment.
same, souble result in the same, because we gave the fixed value above
| @@ -847,6 +850,7 @@ impl Asic for BM1370 { | |||
| .set_post2_div(1) | |||
| .set_out_div(BM1370_PLL_OUT_UART, pll3_div4); | |||
| let pll3_param = self.plls[BM1370_PLL_ID_UART].parameter(); | |||
| let pll3_param = 0x5aa5_5aa5; // TODO: replace these fixed values with equivalent individual ones below | |||
There was a problem hiding this comment.
this hardcoding, I acknoledge it, because the PLL module does not produce exactly the same value, but it should be exactly the same final frequency. Did you try without this hardcoding, and have UART after the baudrate change still functional ?
If the baudrate change is fully functionnal, no need to try to have EXACTLY the same value as BM, as long as we have full functionality.
| @@ -869,6 +873,7 @@ impl Asic for BM1370 { | |||
| // will be ignored by the chip. | |||
| let pll3_param = | |||
| self.plls[BM1370_PLL_ID_UART].disable().unlock().parameter(); | |||
| let pll3_param = 0x5aa5_5aa5; // TODO: replace these fixed values with equivalent individual ones below | |||
There was a problem hiding this comment.
according to coment above, we should not have this step, why hardcoding it ?
| @@ -1240,8 +1288,8 @@ impl Asic for BM1370 { | |||
| _ => { | |||
| // authorize a VersionRolling sequence start whatever the current step was | |||
| self.seq_step = SequenceStep::VersionRolling(0); | |||
| let hcn = 0x0000_1eb5; // S21Pro | |||
| // let hcn = 0x0000_1a44; // S21XP | |||
| // let hcn = 0x0000_1eb5; // S21Pro | |||
There was a problem hiding this comment.
the HCN is a function of the ASIC arch (number of cores, etc) but also the desired frequency. I will find the formula to have it dynamically calculated instead or hardcoding it from a FW or antoher...
| @@ -200,7 +203,7 @@ impl<A: Asic, P: Read + Write + Baud, D: DelayNs> Chain<A, P, D> { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| async fn send(&mut self, step: CmdDelay) -> Result<(), P::Error> { | |||
| pub async fn send(&mut self, step: CmdDelay) -> Result<(), P::Error> { | |||
There was a problem hiding this comment.
why do you need to expose this send() ? it should be a local helper only
| @@ -248,7 +251,35 @@ impl<A: Asic, P: Read + Write + Baud, D: DelayNs> Chain<A, P, D> { | |||
| self.send(step).await?; | |||
| } | |||
| } | |||
| self.delay.delay_ms(290).await; | |||
There was a problem hiding this comment.
I do not agree to put these new command in here. bm13xx-chain should be generic across all suported ASIC model. These command are specific to BM1370 configuration sequences.
I know it is an extra step that has no place anywhere, so we should:
- either do a new sequence in the
bm13xx-chip::Asictrait, and implement it for each of the 3 supported ASIC, with only the BM1370 doing these steps - either we can append these 4 steps to the reset_core_next() of the BM1370 crate only
In anycase, should not be here.
No description provided.